Skip to content

refactor(components): derive the row-menu predicate parameters from the authoring type (#4354) - #4423

Merged
yinlianghui merged 1 commit into
mainfrom
claude/issue-4354-row-menu-predicate-derive
Aug 12, 2026
Merged

refactor(components): derive the row-menu predicate parameters from the authoring type (#4354)#4423
yinlianghui merged 1 commit into
mainfrom
claude/issue-4354-row-menu-predicate-derive

Conversation

@yinlianghui

Copy link
Copy Markdown
Collaborator

Fixes #4354

Three hand-copies (four, once counted) collapse to one derivation

One authoring shape — DataTableSchema.rowEditPredicates / rowDeletePredicates (objectui#2614) — had grown four independent declarations inside packages/components/src/renderers/complex/data-table.tsx, none of which referenced the type whose values they receive:

site was reads
isBuiltinRowActionVisible { visibleWhen?: unknown } visibleWhen
planDataTableRowMenu (x2 params) { visibleWhen?: unknown } visibleWhen
DataTableBuiltinRowActionItem { visibleWhen?: unknown; disabledWhen?: unknown } both

The issue counted three; the shared gate is a fourth, three lines above the planner, and leaving it hand-written would have kept a copy of the very shape this change exists to stop duplicating.

Per the ruling (option b, derivation over widening), each site now derives, and the signatures keep telling the truth about what each function reads:

  • planDataTableRowMenu stays consumption-shaped — it decides visibility and nothing else — but each parameter is Picked from the exact schema key its production caller passes: editPredicates?: Pick< NonNullable< DataTableSchema['rowEditPredicates'] >, 'visibleWhen' >, and the delete twin from rowDeletePredicates. Same subset as before, minus the drift.
  • The two consumers that serve both built-ins (isBuiltinRowActionVisible, and the item component whose prop is name: 'edit' | 'delete') share one derived alias taken from the union of the twins, so they may only read keys that both schema keys declare — if either twin dropped one, the read stops compiling instead of silently reading undefined.

The ITEM component: derived, not left

It was a hand-copy of the full pair with no reason to be one — it evaluates both keys, so it derives the whole authoring shape (BuiltinRowPredicates) rather than a Pick. This is the site with the most to gain; see the measurement below.

No runtime code was touched. @object-ui/types is untouched — this derives from it.

The rider: measured unpinned, now pinned

Measurement. The rule "an item that renders merely disabled still counts toward the menu's non-empty decision" was not pinned anywhere that could observe it:

  • data-table-row-menu-empty-guard.test.tsx's still counts an item that renders merely DISABLED asserted the planner's return value, and the planner never reads disabledWhen — the fixture behaved identically to {}, which is what made it vacuous.
  • data-table-builtin-row-action-predicates.test.tsx does pin the disabled rendering observably (aria-disabled + click suppressed), but it renders the item directly inside an already-open dropdown — it never goes through the table, so it cannot see whether the row got a trigger at all.
  • The table-level DOM cases in the guard file used only visibleWhen. Nothing joined the two halves.

Added (keeps the trigger for a row whose only item renders merely DISABLED): a table whose single action is disabledWhen-gated keeps its per-row trigger, and opening that trigger finds the item present and aria-disabled="true". End-to-end, because the two halves live in different functions and each half's own test stays green while the other regresses — demonstrated below.

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. The derived annotation on its fixture (tranche 4's work) is kept and still necessary: the planner's parameter remains a deliberate visibleWhen-only subset, so a bare object literal still trips excess-property checking.

Reverse verification

1. The derivation tracks its source — and the reported direction is not the one the template presumes. Renaming visibleWhen to visibleWhenRenamed in @object-ui/types (scratch, reverted), then type-checking @object-ui/components both ways:

  • Derived (this PR): 7 errors, three of them TS2344 pointed straight at the derived declarations themselves — the compiler names the line that must be updated.
  • Hand-written (origin/main): 2 errors, and this is the interesting half. I predicted green; it is not quite green, but the two errors are incidental — TS weak type detection firing at a call site ("has no properties in common"), never at a declaration. The declarations all compile clean against a type that no longer has the key.
  • The item component drifts completely silently in the hand-written form: its hand-copy keeps disabledWhen in common with the renamed type, so weak-type detection is satisfied, and neither its declaration nor either of its two call sites produces a single diagnostic. It would go on reading predicates.visibleWhen === undefined forever — i.e. the built-in gate silently becomes "always visible". Derived, that exact path is TS2345 at the line where the item hands its predicates to the visibility gate.

That is the real value here, and it is stronger than "before green, after red": the compiler goes from silent on the site that matters to naming it.

(Recorded for the next reader: this package's tsconfig.json overrides the root paths, so @object-ui/types resolves through the built .d.ts, not sources. The first run of this simulation was a false green until @object-ui/types was rebuilt.)

2. The new pin is red-first against the counting half. Scratch: make a disabledWhen-gated item not count toward the trigger. Predicted the new pin red and every planner-level case green — measured exactly that: 1 failed | 21 passed, the failure being expected [] to have a length of 2. Every pre-existing case, including the renamed planner one, stayed green — which is precisely why the pin was needed.

3. And against the rendering half. Scratch: let the item return null when disabled. Predicted two reds — the new pin plus the pre-existing item-level case. Measured 2 failed | 20 passed, both Unable to find an element by: [data-testid="row-action-builtin-edit"].

Both scratches reverted; the tree was re-verified green afterwards.

Verification

  • pnpm exec vitest run packages/components/ --maxWorkers=2 (repo root) — 121 files / 1078 tests passed, no failures.
  • tsc --noEmit and tsc -p tsconfig.test.json for @object-ui/components — both exit 0 (build closure built first).
  • eslint on both touched files — exit 0, 0 errors; the 5 remaining no-explicit-any warnings are pre-existing as any casts on untouched lines.
  • check-control-bytes / check-phantom-dependencies / check-spec-symbol-derivation / check-changeset-presence / check-changeset-no-major — all green.
  • Changeset: patch for @object-ui/components, never major.

Scope

Only packages/components plus the changeset. Nothing in packages/fields, plugin-charts, plugin-chatbot or plugin-dashboard was touched. No out-of-scope findings to file.


Generated by Claude Code

…ataTableSchema (#4354)

One authoring shape had four declarations in data-table.tsx: the shared
`isBuiltinRowActionVisible` gate and `planDataTableRowMenu` each hand-wrote
`{ visibleWhen?: unknown }`, and `DataTableBuiltinRowActionItem` hand-wrote the
full pair. None referenced the type whose values they receive.

Each now derives from `DataTableSchema`. The planner keeps its deliberate
visibility-only subset, `Pick`ed from the key its caller passes; the two
consumers that serve both built-ins share one alias derived from the union of
the twins, so they may only read keys both declare.

Adds the observable pin the disabled-item counting rule never had: a row whose
only action is `disabledWhen`-gated keeps its trigger, and that trigger opens
the item, present and aria-disabled. The planner-level case that used to claim
this is renamed to the verdict it actually decides.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectui Ignored Ignored Aug 12, 2026 5:22am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

✅ Console Performance Budget

Metric Value Budget
Main entry (gzip) 24.7 KB 350 KB
Entry file index-BCq74OTP.js
Status PASS

📦 Bundle Size Report

Package Size Gzipped
app-shell (index.js) 9.56KB 3.59KB
app-shell (runtime-config.js) 7.42KB 2.32KB
app-shell (types.js) 0.01KB 0.04KB
app-shell (urlParams.js) 8.92KB 3.41KB
auth (AuthContext.js) 0.31KB 0.24KB
auth (AuthGuard.js) 1.17KB 0.53KB
auth (AuthProvider.js) 22.10KB 4.37KB
auth (AuthShell.js) 3.49KB 1.40KB
auth (ForgotPasswordForm.js) 12.21KB 3.45KB
auth (LoginForm.js) 18.13KB 5.39KB
auth (PreviewBanner.js) 0.90KB 0.50KB
auth (RegisterForm.js) 6.64KB 2.21KB
auth (SocialSignInButtons.js) 9.60KB 3.89KB
auth (UserMenu.js) 3.40KB 1.22KB
auth (auth-gate-events.js) 1.29KB 0.66KB
auth (authStyles.js) 5.04KB 1.72KB
auth (createAuthClient.js) 35.76KB 9.11KB
auth (createAuthenticatedFetch.js) 4.37KB 1.69KB
auth (index.js) 2.35KB 1.07KB
auth (org-roles.js) 6.66KB 2.78KB
auth (phone-identifier.js) 1.11KB 0.66KB
auth (types.js) 0.59KB 0.35KB
auth (useAuth.js) 4.91KB 0.87KB
auth (useIsWorkspaceAdmin.js) 1.61KB 0.85KB
collaboration (CommentThread.js) 26.07KB 7.56KB
collaboration (LiveCursors.js) 3.17KB 1.27KB
collaboration (PresenceAvatars.js) 6.49KB 2.64KB
collaboration (PresenceProvider.js) 2.79KB 1.13KB
collaboration (index.js) 1.65KB 0.73KB
collaboration (useCollaborationTranslation.js) 6.05KB 2.52KB
collaboration (useCommentSearch.js) 1.98KB 0.88KB
collaboration (useConflictResolution.js) 7.75KB 1.86KB
collaboration (useMentionNotifications.js) 1.81KB 0.68KB
collaboration (usePresence.js) 6.33KB 1.84KB
collaboration (useRealtimeSubscription.js) 7.91KB 2.01KB
components (index.js) 489.20KB 108.43KB
core (index.js) 2.99KB 1.14KB
create-plugin (index.js) 10.08KB 3.26KB
data-objectstack (index.js) 153.42KB 41.19KB
fields (index.js) 228.69KB 56.74KB
i18n (LocalizationContext.js) 1.76KB 0.96KB
i18n (currency.js) 1.22KB 0.64KB
i18n (i18n.js) 4.32KB 1.77KB
i18n (index.js) 3.35KB 1.38KB
i18n (pickLocalized.js) 3.69KB 1.73KB
i18n (provider.js) 23.12KB 7.62KB
i18n (useDisplayLocale.js) 2.33KB 1.20KB
i18n (useObjectLabel.js) 27.59KB 6.63KB
i18n (useSafeTranslation.js) 7.77KB 3.13KB
layout (index.js) 38.98KB 10.85KB
mobile (MobileProvider.js) 0.92KB 0.49KB
mobile (ResponsiveContainer.js) 0.94KB 0.38KB
mobile (breakpoints.js) 1.51KB 0.70KB
mobile (createOfflineDataSource.js) 5.61KB 1.74KB
mobile (index.js) 1.50KB 0.62KB
mobile (offlineQueue.js) 3.91KB 1.35KB
mobile (pwa.js) 0.97KB 0.49KB
mobile (serviceWorker.js) 1.48KB 0.62KB
mobile (serviceWorkerSource.js) 3.41KB 1.48KB
mobile (useBreakpoint.js) 1.54KB 0.65KB
mobile (useGesture.js) 6.96KB 1.98KB
mobile (useOfflineSync.js) 1.99KB 0.72KB
mobile (usePullToRefresh.js) 2.53KB 0.85KB
mobile (useResponsive.js) 0.71KB 0.42KB
mobile (useResponsiveConfig.js) 1.36KB 0.63KB
mobile (useSpecGesture.js) 4.32KB 1.64KB
mobile (useTouchTarget.js) 1.01KB 0.54KB
permissions (MePermissionsProvider.js) 8.75KB 3.06KB
permissions (PermissionContext.js) 0.31KB 0.25KB
permissions (PermissionGuard.js) 0.89KB 0.45KB
permissions (PermissionProvider.js) 3.67KB 1.12KB
permissions (evaluator.js) 4.41KB 1.44KB
permissions (index.js) 0.91KB 0.41KB
permissions (store.js) 0.91KB 0.42KB
permissions (useFieldPermissions.js) 1.28KB 0.52KB
permissions (usePermissions.js) 1.55KB 0.71KB
plugin-ai (index.js) 15.71KB 3.79KB
plugin-calendar (index.js) 45.23KB 12.45KB
plugin-charts (index.js) 62.01KB 17.63KB
plugin-chatbot (index.js) 180.33KB 42.79KB
plugin-dashboard (index.js) 120.57KB 31.32KB
plugin-designer (index.js) 211.16KB 42.76KB
plugin-detail (index.js) 239.03KB 59.77KB
plugin-editor (index.js) 2.46KB 1.10KB
plugin-form (index.js) 114.58KB 27.68KB
plugin-gantt (index.js) 164.14KB 39.98KB
plugin-grid (index.js) 187.99KB 49.92KB
plugin-kanban (index.js) 48.60KB 13.41KB
plugin-list (index.js) 110.21KB 26.79KB
plugin-map (index.js) 18.05KB 5.80KB
plugin-markdown (index.js) 13.72KB 4.69KB
plugin-report (index.js) 40.99KB 10.74KB
plugin-timeline (index.js) 26.21KB 7.52KB
plugin-tree (index.js) 8.50KB 2.88KB
plugin-view (index.js) 84.03KB 20.55KB
providers (DataSourceProvider.js) 0.75KB 0.39KB
providers (MetadataProvider.js) 1.37KB 0.59KB
providers (ThemeProvider.js) 1.90KB 0.85KB
providers (UploadProvider.js) 11.71KB 3.53KB
providers (index.js) 0.44KB 0.22KB
providers (types.js) 0.01KB 0.04KB
react-runtime (index.js) 5.67KB 2.37KB
react (LazyPluginLoader.js) 3.77KB 1.33KB
react (SchemaRenderer.js) 23.71KB 7.96KB
react (data-invalidation.js) 5.05KB 2.08KB
react (index.js) 1.23KB 0.66KB
react (spec-input.js) 0.20KB 0.18KB
sdui-parser (codegen.js) 4.09KB 1.74KB
sdui-parser (index.js) 4.47KB 2.03KB
sdui-parser (parse.js) 10.04KB 2.82KB
sdui-parser (types.js) 0.29KB 0.24KB
sdui-parser (validate.js) 4.69KB 1.48KB
types (ai.js) 0.20KB 0.17KB
types (api-types.js) 0.20KB 0.18KB
types (app.js) 2.87KB 0.99KB
types (base.js) 0.20KB 0.18KB
types (blocks.js) 0.20KB 0.18KB
types (complex.js) 0.20KB 0.18KB
types (crud.js) 0.20KB 0.18KB
types (dashboard-filter-alias.js) 6.23KB 2.74KB
types (data-display.js) 0.20KB 0.18KB
types (data-protocol.js) 0.20KB 0.19KB
types (data.js) 0.20KB 0.18KB
types (designer.js) 1.87KB 0.85KB
types (disclosure.js) 0.20KB 0.18KB
types (error-code.js) 1.54KB 0.88KB
types (feedback.js) 0.20KB 0.18KB
types (field-types.js) 0.20KB 0.18KB
types (form.js) 0.20KB 0.18KB
types (http-retry.js) 4.32KB 2.02KB
types (index.js) 3.05KB 1.52KB
types (layout.js) 0.20KB 0.18KB
types (managed-by.js) 0.19KB 0.18KB
types (mobile.js) 2.59KB 1.31KB
types (navigation.js) 0.20KB 0.18KB
types (objectql.js) 0.20KB 0.18KB
types (overlay.js) 0.20KB 0.18KB
types (permissions.js) 0.20KB 0.18KB
types (plugin-scope.js) 0.20KB 0.18KB
types (record-components.js) 0.20KB 0.19KB
types (record-semantics.js) 1.28KB 0.67KB
types (registry.js) 0.20KB 0.18KB
types (reports.js) 0.20KB 0.18KB
types (spec-report.js) 5.05KB 1.93KB
types (system-fields.js) 3.33KB 1.54KB
types (theme.js) 0.20KB 0.18KB
types (ui-action.js) 3.40KB 1.71KB
types (views.js) 0.20KB 0.18KB
types (widget.js) 0.20KB 0.18KB

Size Limits

  • ✅ Core packages should be < 50KB gzipped
  • ✅ Component packages should be < 100KB gzipped
  • ⚠️ Plugin packages should be < 150KB gzipped

Copy link
Copy Markdown
Collaborator Author

ACCEPT — PM 复核 (session session_017Qqyix2QcnpUC9XeYVDzx3), closes #4354.

  • The scope+1 to the FOURTH restatement is accepted as the card's own logic completed: deriving the planner while leaving the actual visibleWhen READER hand-written three lines away would have kept the defect in the file — and the drift simulation's finding (the hand-written item path emits ZERO diagnostics against a renamed authoring type, thanks to weak-type detection, silently turning the visibility gate always-on) is the measured proof that this site had the most to gain.
  • RV1's honest direction beats the template: hand-written was "not-quite-green" (two incidental weak-type errors at call sites, silence at every declaration), derived is red AT the declarations — "the compiler goes from silent on the site that matters to naming it" is the right framing, and the built-.d.ts false-green trap (rebuild @object-ui/types before trusting a rename simulation) is method capture the loop keeps.
  • The rider's both-halves outcome is correct: the new DOM pin joins counting to rendering where trigger-existence is actually decided, and renaming rather than converting the planner case respects what each layer can structurally claim. RV2's "every pre-existing case stayed green" is the demonstration that nothing could see this regression before.
  • The principled two-styles note (ruling-literal Picks for the planner, one union alias for the shared consumers, reason in the comment) and the patch grading are both right. The plugin-grid mirror is filed by the PM as its own card — flagging without re-filing was the correct call.

Flipping ready + arming auto-merge.


Generated by Claude Code

@yinlianghui
yinlianghui marked this pull request as ready for review August 12, 2026 05:31
@yinlianghui
yinlianghui added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 4b70d28 Aug 12, 2026
21 checks passed
@yinlianghui
yinlianghui deleted the claude/issue-4354-row-menu-predicate-derive branch August 12, 2026 05:32
github-merge-queue Bot pushed a commit that referenced this pull request Aug 12, 2026
…m the spec-owned authoring type (#4429) (#4430)

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.


Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

finding(components): planDataTableRowMenu's predicate parameter is narrower than the object its only caller hands it

2 participants