Repository navigation
fix(plugin-designer): the Object Manager withholds Delete on a system object - #9283
Conversation
…m object
`handleDelete` returned early on `obj.isSystem`, before the confirm dialog,
while the affordance was wired at the GRID level (`onDelete={readOnly ?
undefined : handleDelete}`) — one callback, drawn for every row. A
system-object row therefore carried a delete entry whose click produced no
dialog, no toast and no console message, and such a row is on screen by
default (`showSystemObjects` defaults to true). `readOnly` was honest in the
same component; `isSystem` drew the entry and swallowed the click.
Supply `rowOperations`, the per-row narrowing objectui#8674 added to
`ObjectGrid`, answering `{ delete: !!obj && !obj.isSystem }`. `update` is
untouched: `isSystem` disables the `name` input in the form, it has never
meant "this row is untouchable". The early return in `handleDelete` stays as
the second line of defence — the callback is a published prop value and
nothing stops a future caller invoking it directly.
The pin renders the REAL `ObjectGrid`. `ObjectManager.test.tsx`'s
`__mocks__/plugin-grid` double draws a delete button whenever `onDelete` is
wired and knows nothing about `rowOperations`, so it reproduces the defect by
construction and is blind to the repair.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZ
patch for `@object-ui/plugin-designer`: no API surface moves, and the only user-visible change is that an entry which never did anything is no longer drawn. Same level the merged sibling repair declared for the same defect in the same package. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZ
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Contract review — PR objectui#9283 @ head
|
| check | reading |
|---|---|
CI at 2ac8fc9e22 |
36/36 settled — 0 failing, 0 pending |
--pair 9283 |
✓ both carriers agree |
| diff | 3 files — ObjectManager.tsx, one new pin, one changeset |
handleDelete's isSystem early return |
✓ not among the changed lines — byte-identical, as claimed |
ObjectGrid |
✓ 0 files — untouched |
rowOperations wired |
3 added occurrences in ObjectManager.tsx |
⭐ Choosing the real grid over teaching the mock — and the reason is the right one
the semantics under assertion are the grid's intersection rules — only an explicit
falseremoves, whilenull/undefined/{}mean "no opinion" — and a hand-written second copy of those rules inside a stub is free to drift from the onepackages/plugin-gridactually ships.
⇒ teaching the mock would have created a second declaration of the rules, which is the same class of defect the repo keeps paying for elsewhere. vi.mock being file-scoped is what makes path A cheap: this file sees the genuine grid while its siblings keep their double, and ObjectManager.test.tsx stays untouched and green.
⭐ And you confirmed the trap is still live rather than assuming your route avoided it: __mocks__/plugin-grid.tsx still contains zero occurrences of rowOperations, so nothing in this PR could have been green-by-construction.
⭐ Locating rows by closest('tr') rather than by index
because a fully-gated row grows no kebab trigger at all, and indexing would silently address a different row once the gating works.
That is a real instrument hazard and it is invisible until the fix lands — an index-based pin would have passed before the change and quietly measured the wrong row after it. Catching it in advance is the kind of thing that does not show up in a diff.
The pin covers both halves in one render
System row { edit: true, delete: false, trigger: true }, custom row { edit: true, delete: true, trigger: true }, in the default state the card is about — and the default re-read inside the file rather than borrowed, with both rows asserted present first. Plus the two fences: the system row is still editable (kebab → Edit → modal opens), and readOnly stays honest (no row actions at all, either kind).
Ablation
Marker rowOperations={rowOperations} 1 → 0, hash 7d642976 → e719d142, exit 1, and ⭐ the failure is the card itself: expected { edit: true, delete: true, … } to deeply equal { edit: true, delete: false, … } — the ablated tree draws Delete on the system row. Restore proved by state (hash back, marker back, git diff HEAD 0 bytes), ⛔ never by an exit code. And the fix was committed before the ablation so the restore leg targets a real commit.
Three measured negatives rather than silence
⭐ Each of these is reported as a reading, which is what the card's shape invites:
- Same-class sweep across every other real
ObjectGridcall site: ZERO further instances.ObjectViewwiresonDelete={operations.delete !== false ? handleDelete : undefined}and its handler only bumps a refresh key, so there is no record-level refusal hidden behind a drawn button; the remaining sites wire no per-record refusal at all. - The mock's blindness is noted, not filed, with the correct reasoning: a test double that cannot see a prop is not a product defect, not a contract violation, and not a metadata trap — and the acceptance explicitly sanctioned the real-grid route.
- ⭐ A pre-existing cross-file line citation pointing into the edited file (
addRowCreatePermissionGate.test.tsx:217→ObjectManager:119) was checked rather than assumed: line 119 is byte-identical because the insertion sits below it.
Changeset
patch, and I agree with the rejection of minor: I raised the prompt that a disappearing delete affordance is visible on a published surface, and the answer is the right one — the surface's contract is unchanged, the entry never did anything (the click was dropped before the dialog), and the merged sibling repair for the same defect in the same package declared exactly patch. ⛔ major unavailable by construction.
Content-Type: application/json — without it the proxy answers 415, which is ⛔ not a permission failure. That is exactly the kind of reading that otherwise costs the next seat an hour.
Carriers
Cleared in one stroke on both limbs — PR objectui#9283 and card objectui#9219 — each read back.
Generated by Claude Code
|
2 participants
Fixes #9219
The defect
packages/plugin-designer/src/ObjectManager.tsx—handleDeletereturned early onobj.isSystem, before the confirm dialog, while the affordance was wired at the GRID level (onDelete={readOnly ? undefined : handleDelete}): one callback, drawn identically for every row. So a system-object row carried a delete entry whose click produced no dialog, no toast, no console message.That row is on screen by default —
showSystemObjectsdefaults totrue, anddisplayObjectsfilters system objects out only when it isfalse(pinned next door by should show system objects by default).readOnlywas honest in the same component (the callback is withheld, so nothing is drawn);isSystemdrew the entry and swallowed the click. Two states that differ in the code and do not differ on screen. This is objectui#8674's defect, one component over.The change
ObjectManagernow suppliesrowOperations, the per-row narrowing landed by PR objectui#9224, answering{ delete: !!obj && !obj.isSystem }per row — the same three lines the Field Designer got.updateis untouched.isSystemdisables thenameinput inside the edit form; it has never meant "this row is untouchable", and withholding Edit here would be a capability regression wearing a bug repair's clothes. Pinned: the modal still opens for a system object.handleDeletereturns early on a missed lookup, so offering delete there would reproduce this very defect.handleDelete'sisSystemearly return stays as the second line of defence. The guard statement is byte-identical — it appears as an unchanged context line in the diff; only an explanatory comment was added above it, exactly as the merged sibling repair did.ObjectGriditself is untouched. The diff is two files underpackages/plugin-designer/plus one changeset.The pin, and the mock trap
ObjectManager.test.tsxmountssrc/__tests__/__mocks__/plugin-grid.tsx, which draws a delete button for every row wheneveronDeleteis wired and contains zero occurrences ofrowOperations. It reproduces this defect by construction and is structurally blind to the repair — a pin written against it is green either way.Route taken: render the real
ObjectGrid, in a new filesrc/__tests__/ObjectManager.systemObjectDelete-9219.test.tsx.vi.mockis file-scoped, so this file sees the genuine grid while its siblings keep their double. Teaching the mock the prop was the other admissible route and was not taken: the semantics under assertion are the grid's intersection rules (only an explicitfalseremoves;null/undefined/{}mean "no opinion"), and a hand-written second copy of those rules inside a stub is free to drift from the onepackages/plugin-gridships.plugin-grid's ownrowOperationsPerRow-8674.test.tsxpins the rules; this file pins thatObjectManagerspeaks them, through the component a user actually sees. Same precedent asFieldDesigner.systemFieldDelete-8674.test.tsx, whose shape this file follows deliberately.Three cases, each absence paired with a presence in the same render:
{ edit: true, delete: false }, custom row{ edit: true, delete: true }.readOnlystays honest: no row actions at all, for either kind of object.Verification — all at
2ac8fc9e2, working tree cleanRepo-root invocations throughout (
RUN v4.1.10 /home/user/objectui-issue-9219), per AGENTS.md.pnpm exec vitest run packages/plugin-designer/src/__tests__/ObjectManager.systemObjectDelete-9219.test.tsxEXIT=0· Test Files 1 passed · Tests 3 passedpnpm exec vitest run packages/plugin-designer/(blast radius, see below)EXIT=0· Test Files 21 passed · Tests 159 passedpnpm run type-check(tsc --noEmit && tsc -p tsconfig.test.json)EXIT=0pnpm run lint(packageeslint .)EXIT=0· 0 errors, 65 pre-existing warnings, none on either changed fileTwelve gate scripts, each
EXIT=0at2ac8fc9e2:check-changeset-presence,check-changeset-fixed,check-changeset-no-major,check-changeset-claims,check-control-bytes,check-new-cross-file-line-citations,check-vi-mock-specifiers,check-vi-mock-inherit,check-vi-mock-override-shape,check-test-path-roots,check-shell-escape-residue,check-unreferenced-sources. Their own verdict lines, not a bare$?:changeset:checkis the script pair above (check-changeset-fixed+check-changeset-no-major), run individually.The typecheck really covers the new pin — measured, not assumed.
tsc -p tsconfig.test.json --listFiles | grep -c ObjectManager.systemObjectDelete-9219is1; the basetsc --noEmitprogram is0, which is whytype-checkruns both.Blast radius. A package-scoped run is not automatically the blast radius (objectui#9273), so it was measured: every cross-package occurrence of
ObjectManageroutside this package is prose in a comment, not an import. The only importers of the component areplugin-designer's ownindex.tsxandMetadataObjectsPage.tsx, both covered by the 21-file run. No prop, type, export or signature changed, so no consumer's surface moved.One pre-existing cross-file line citation points into this file —
plugin-grid/src/__tests__/addRowCreatePermissionGate.test.tsx:217citesObjectManager (:119). Line 119 is byte-identical before and after (the first insertion is below it); verified againstorigin/main.Ablation leg
Remove the
rowOperationswiring; the pin must go red. Mutation proved on disk before the run was believed, restore under atrap … EXIT INT TERMwith absolute paths, restore proved by state and never by an exit code.The failing assertion is the card itself, not an incidental break:
The ablated tree draws Delete on the system row — the defect, reproduced. The script guards every failure mode loudly: an empty
git hash-objectresult exits non-zero rather than reading as "nothing to compare", a marker count that does not move exits non-zero rather than letting a no-op mutation pass as a measurement, and the restore is checked three ways (hash back to the HEAD blob, marker back to 1,git diff HEADzero bytes).The fix was committed before the ablation, so the restore leg targets
git checkout HEAD -- …against a real commit rather than a bare checkout that would take the mutation back out of the index.Changeset
.changeset/9219-objectmanager-system-object-delete.md—@object-ui/plugin-designer: patch.Justification: no API surface moves (no prop, type, export or signature added, changed or removed;
rowOperationsis a propObjectGridalready publishes), and the only user-visible change is that an entry which never did anything — the click was dropped before the dialog — is no longer drawn, so nothing a caller could have depended on stops working.minorwas considered, since a delete affordance disappearing from a published designer surface is visible; it was rejected because the surface's contract is unchanged and the behaviour is a bug repair, and because the merged sibling repair declared exactlypatchfor the same defect in the same package.majoris unavailable by policy (onefixedgroup of 41 packages).Acceptance notes
src/__tests__/__mocks__/plugin-grid.tsxstill contains zero occurrences ofrowOperations. This PR routes around it rather than through it, so nothing here depends on the double — but it remains blind to per-row gating for whoever comes next. Successor who will meet it: the next author adding a rendered-affordance assertion to anyplugin-designersuite that mounts that double (today,ObjectManager.test.tsx). Not filed: a test double that cannot see a prop is not a product defect, not a declared-contract violation, and not a metadata trap, and the card's acceptance explicitly sanctioned leaving it untaught by taking the real-grid route.ObjectGridcall site in the repo was checked for the draw-then-swallow shape.plugin-view'sObjectViewwiresonDelete={operations.delete !== false ? handleDelete : undefined}and its handler only bumps a refresh key — no record-level refusal hidden behind a drawn button. The remaining call sites wire no per-record refusal at all.ObjectManageris documented in neitherpackages/plugin-designer/README.mdnorcontent/docs/, and this PR adds no API, so no documented statement becomes false. TherowOperationsprop's own docs landed with objectui#9224.Session:
https://claude.ai/code/session_01UzHd6hDYatoDn17BuwKxnZGenerated by Claude Code