refactor(plugin-grid): derive the row-menu predicate declarations from the spec-owned authoring type (#4429) - #4430
Merged
Conversation
…m the spec-owned authoring type (#4429) RowActionMenu.tsx carried its own `BuiltinRowActionPredicates` interface (`{ visibleWhen?: unknown; disabledWhen?: unknown }`) read at six declaration sites, tied to nothing. Measured true source: these predicates do NOT come from `DataTableSchema`'s twins — `ObjectGrid` resolves the object's `userActions` through `resolveRowCrudAffordances`, which returns `CrudAffordances`' `editPredicates` / `deletePredicates`, i.e. the spec-owned `RowCrudPredicates` (ADR-0103) re-exported by `@object-ui/core`. Each site now derives from that: per-key `Pick` for the planner's visibility-only parameters, one union alias for the two consumers that serve both built-ins. Inherits PR #4423's pattern wholesale, including its rider: the "a merely disabled item still counts toward the menu" rule was measured unpinned here too, so it gains a DOM pin where a user meets it (trigger survives, and opening it finds the item `aria-disabled`), and the planner-level case that claimed to pin it is renamed to the verdict it actually decides — the planner never reads `disabledWhen`. No runtime change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — PM 复核 (session
Flipping ready + arming auto-merge. Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4429
Inherits PR #4423's pattern wholesale — the per-key
Pickderivations for the planner, one union alias for the shared both-builtin consumers, the two principled styles, the DOM pin joining counting to rendering, the vacuous-case rename, and the RV method including the built-.d.tsrebuild trap.The card said four hand-restated shapes; the file has ONE, read at SIX sites
Re-verified at this branch point (
4dadf0d3d). The defect is real and is exactly the kind #4423 collapsed, but its shape differs from the data-table's and the reading governs:packages/componentshad four independent inline literals, while plugin-grid has a single hand-writtenexport interface BuiltinRowActionPredicatesconsumed at six declaration sites. One definition, six readers — not four definitions.BuiltinRowActionPredicates(the one hand-written declaration){ visibleWhen?: unknown; disabledWhen?: unknown }RowActionMenuProps.editPredicatesRowActionMenuProps.deletePredicatesisBuiltinRowActionVisiblevisibleWhenPick< …, 'visibleWhen' >planRowActionMenu.editPredicatesvisibleWhenPickfrom the edit twinplanRowActionMenu.deletePredicatesvisibleWhenPickfrom the delete twinBuiltinRowActionItem.predicatesThe card's fourth bullet ("a local
disabledWhen?: unknown, ~line 65") is not a fifth site — it is one key inside that single interface, at exactly line 65. Counting it separately would double-count the declaration.The measured TRUE source — not
DataTableSchemaThe card asked whether these predicates flow from
DataTableSchema's twins or a grid-own key. Neither. This surface is never handed aDataTableSchemakey at all:So the authoring source is the spec-owned
RowCrudPredicates, reached throughCrudAffordances['editPredicates']/['deletePredicates']— the very keys the production caller passes. Deriving fromDataTableSchema.rowEditPredicateswould have bound this file to a schema key it never sees: a NEW wrong coupling in place of the old missing one.@object-ui/typesis untouched, and so is@object-ui/core— this derives from them.The union of the two twins is kept for the reason #4423 gave: every consumer reading the alias serves BOTH built-ins (
name: 'edit' | 'delete'), so it may only read keys that both affordance keys declare. Today the twins are declared as one named type, so the union collapses onto it — stated plainly rather than dressed up; the construction is what carries the constraint if that coincidence ever ends.The derivation TIGHTENS, and that is the point
The hand-written pair typed both keys
unknown; the spec types themExpression | ExpressionInput(the authored CEL shorthand, or its{ dialect, source }envelope).@object-ui/coreretired precisely this imprecision at its own seam and said so in as many words — "The localunknownwas imprecision, not a deliberate dialect" (core/utils/managedBy.ts:43-52). This was the last copy of it.Reverse verification
Direction predicted before running, and the fix taken out with
git diff+git checkout --/git apply— nevergit stash.RV1 — the rename simulation
Renamed
RowCrudPredicates.visibleWhentovisibleWhenRenamedat the source, then type-checked plugin-grid both ways, in both of its tsc projects.Mechanical note, recorded for the next reader. The rename had to land in
@objectstack/spec's declaration files, and those are pnpm-hardlinked (9 links each) into the shared store, so an in-place edit would have corrupted every parallel agent's tree. Both files were therefore replaced through a new inode (cpbackup, write elsewhere,rename), verified at 1 link afterwards and restored the same way. The rebuild trap #4423 recorded did not bite here, and the reason is worth knowing: plugin-grid'stsconfig.test.jsonsets"paths": {}, so@object-ui/coreresolves through its built.d.ts— but that built.d.tsre-exports the spec type rather than inlining it, so the rename propagated with no@object-ui/corerebuild. Verified empirically by the test project going red.origin/main)Predicted: derived red at the declarations; hand-written fully silent. Measured exactly that — and the silence is total, stronger than #4423's data-table half (which at least emitted two incidental weak-type errors at call sites):
The three
TS2344s are the three derived declarations themselves — the compiler names the lines that must be updated. Onorigin/mainthe hand-written interface keepsdisabledWhenin common with the renamed type, so TS weak-type detection is satisfied at every call site while the declarations reference the source nowhere at all. The consequence is not cosmetic:isBuiltinRowActionVisiblewould readpredicates?.visibleWhenasundefined, hitif (pred == null) return true, and the built-in Edit/Delete gate would silently become "always visible" — an affordance the author gated, shown to everyone, with zero diagnostics anywhere in the package.RV2 / RV3 — the new DOM pin, red-first against each half
Broken surgically at the trigger decision (
hasMenu), not atplan.menuCount, so the planner's return value stays untouched and the break isolates exactly what the pin adds.1 failed | 53 passed— only the pin (expect(received).toBeInTheDocument())BuiltinRowActionItemreturnsnullwhen disabled2 failed | 52 passed, in two different files, bothUnable to find an element by: [data-testid="row-action-builtin-edit"]That asymmetry is the whole argument for the pin: breaking the counting half is invisible to the rendering half's own test file (all 54 of its cases green except the new pin), and the renamed planner case stayed green through both scratches. Both scratches reverted; tree re-verified green.
The rider: measured unpinned, now pinned
The rule "an item that renders merely disabled still counts toward the menu's non-empty decision" was not observably pinned anywhere on this surface:
RowActionMenu.emptyGuard.test.tsx'sstill counts an item that renders merely DISABLEDasserted the planner's return value, andplanRowActionMenunever readsdisabledWhen— the fixture behaved identically to{}, which is what made it vacuous.RowActionMenu.test.tsxdoes pin the disabled rendering observably (aria-disabled+ click suppressed), but it rendersBuiltinRowActionItemdirectly inside an already-open dropdown — it never goes through the component that decides whether a trigger exists at all.visibleWhen. Nothing joined the two halves.Added (
keeps the trigger for a row whose only item renders merely DISABLED): a row whose single action isdisabledWhen-gated keeps its "⋮", and opening that trigger finds the item present andaria-disabled="true".Why at
RowActionMenuand not throughObjectGrid. This is the level where the two halves actually meet —RowActionMenuis both the caller of the planner and the renderer of the item, the exact structural mirror of #4423'srenderTable. TheObjectGriddescribe in the same file sits a level above it (column alignment plus asyncuserActionsplumbing) and its cases readvisibleWhenonly, so it did not pin this either; routing the pin through it would have added fixture surface without strengthening the counting-to-rendering join.The vacuous case is renamed, not deleted:
an object with no visibleWhen does not hide the item. That is the verdict it actually decides, it is worth deciding, and the misleading name is gone; its comment now points at where the disabled-counting claim really lives. Its fixture is annotated from the production key (CrudAffordances['editPredicates']) — still necessary, because the planner's parameter remains a deliberatevisibleWhen-only subset, so a bare object literal still trips excess-property checking.Changeset graded
minor, notpatch— a deviation, made as instructedThe dispatch's default was
patch, conditioned on verifying nothing published moves. Something published does move, so it is graded by the objectui#4403 criterion instead.RowActionMenuPropsis exported from the package barrel, and itseditPredicates/deletePredicatesnarrow fromunknown-valued keys toExpression | ExpressionInput— a consumer passing anunknown-typed value, or a bare boolean, stops type-checking. #4403's rule is explicit that a narrowed exported type isminoreven when no runtime consumer breaks and no behavior changes. Nevermajor, per the version-alignment rule.This is also exactly why PR #4423's twin stayed
patch:DataTableSchema's keys were already declared{ visibleWhen?: unknown; disabledWhen?: unknown }, so deriving there narrowed nothing. The grades differ because the sources differ, not by feel.Verification
packages/plugin-grid/— 59 files / 560 tests passed. Baseline on the branch point was 59 / 559: exactly+1, the new pin, no file count change.tsc --noEmitandtsc -p tsconfig.test.jsonfor@object-ui/plugin-grid— both exit 0, dependency closure ('@object-ui/plugin-grid^...') built first.eslinton both touched files — exit 0, 0 errors; the 24 warnings are pre-existingno-explicit-any/react-refreshon untouched lines.check:control-bytes(4134 files),check:phantom-deps,check:spec-symbols,check-changeset-presence,check-changeset-no-major— all green.Scope
Only
packages/plugin-grid/**plus the changeset. Nothing inplugin-chatbot(#4424),plugin-dashboard(#4357),fields(#4414) orcomponents(#4417 / #4423) was modified —packages/componentswas read as the pattern reference only. No runtime code changed. No out-of-scope findings to file.Generated by Claude Code