From 00e330df390b51c9b1b8deb4f658d448b2d37dc5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 06:09:07 +0000 Subject: [PATCH] refactor(plugin-grid): derive the row-menu predicate declarations from the spec-owned authoring type (#4429) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../grid-row-menu-predicate-derive-4429.md | 13 +++ .../src/components/RowActionMenu.tsx | 86 +++++++++++++++---- .../RowActionMenu.emptyGuard.test.tsx | 67 ++++++++++++++- 3 files changed, 148 insertions(+), 18 deletions(-) create mode 100644 .changeset/grid-row-menu-predicate-derive-4429.md diff --git a/.changeset/grid-row-menu-predicate-derive-4429.md b/.changeset/grid-row-menu-predicate-derive-4429.md new file mode 100644 index 0000000000..b8fc9dc150 --- /dev/null +++ b/.changeset/grid-row-menu-predicate-derive-4429.md @@ -0,0 +1,13 @@ +--- +'@object-ui/plugin-grid': minor +--- + +grid row menu — the built-in Edit/Delete predicate declarations are derived from the spec-owned authoring type, not hand-restated + +`packages/plugin-grid/src/components/RowActionMenu.tsx` carried its own `BuiltinRowActionPredicates` interface (`{ visibleWhen?: unknown; disabledWhen?: unknown }`) and read it at six declaration sites: both `RowActionMenuProps` predicate props, the shared `isBuiltinRowActionVisible` gate, both `planRowActionMenu` parameters, and the `BuiltinRowActionItem` component. Nothing tied any of them to the type whose values they receive, so a rename at the source would have left every one compiling against a shape that no longer existed — the objectui#3009 hand-copy family, and the mirror of what PR #4423 collapsed in the data-table. + +**Measured true source.** These predicates do NOT flow from `DataTableSchema.rowEditPredicates` / `rowDeletePredicates` — this surface is never handed those keys. `ObjectGrid` resolves the object's `userActions.edit` / `delete` through `resolveRowCrudAffordances`, which returns `CrudAffordances['editPredicates']` / `['deletePredicates']`: the spec-owned `RowCrudPredicates` (ADR-0103, `@objectstack/spec/data`), parsed in exactly one place and re-exported by `@object-ui/core`. Each site now derives from that — per-key `Pick` for the planner (visibility is all it decides), one union alias for the two consumers that serve both built-ins. Measured: with `visibleWhen` renamed at the source, the previous hand-written declarations produce ZERO diagnostics in this package while the derived ones fail to compile at the declarations themselves. + +**Graded `minor` rather than `patch`** because a published type narrows (the objectui#4403 criterion). `RowActionMenuProps.editPredicates` / `deletePredicates` move from `unknown`-valued keys to the spec's `Expression | ExpressionInput` — the authored CEL shorthand or its `{ dialect, source }` envelope — so a consumer passing an `unknown`-typed value, or a bare boolean, stops type-checking. No runtime consumer breaks and no behavior changes; `@object-ui/core` retired the same `unknown` imprecision at its own seam, and this was the last copy of it. (PR #4423's data-table twin stayed `patch` because `DataTableSchema`'s keys were already declared `unknown` — deriving there narrowed nothing.) + +No runtime code was changed, and the package's suite passes unchanged. Alongside it, the "a disabled item still counts toward the menu" rule gains the pin it never had where a user meets it: a row whose only action is `disabledWhen`-gated keeps its "⋮" trigger, and that trigger opens the item, present and `aria-disabled`. The two halves of that rule live in different functions, and each half's own test stayed green while the other regressed. The planner-level case that claimed to pin this is renamed to the verdict it actually decides — the planner never reads `disabledWhen`, so its fixture behaved identically to `{}`. diff --git a/packages/plugin-grid/src/components/RowActionMenu.tsx b/packages/plugin-grid/src/components/RowActionMenu.tsx index acf139e5c0..39213a3086 100644 --- a/packages/plugin-grid/src/components/RowActionMenu.tsx +++ b/packages/plugin-grid/src/components/RowActionMenu.tsx @@ -15,7 +15,7 @@ import { DropdownMenuTrigger, } from '@object-ui/components'; import { Edit, Trash2, MoreVertical } from 'lucide-react'; -import { evalRowPredicate } from '@object-ui/core'; +import { evalRowPredicate, type CrudAffordances } from '@object-ui/core'; import { useObjectTranslation, useRowPredicate, useCapabilityGate, usePredicateScope } from '@object-ui/react'; const ROW_ACTION_FALLBACKS: Record = { @@ -54,16 +54,47 @@ export interface RowActionDef { [key: string]: any; } -/** Per-record CEL predicates gating a built-in Edit/Delete row action - * (objectui#2614). Bare CEL string or `{ dialect, source }` envelope, - * evaluated per row via `useRowPredicate` — same machinery as custom - * actions' `visible` / `disabled`. */ -export interface BuiltinRowActionPredicates { - /** Evaluates false → the item is not rendered for this row. Fail-closed. */ - visibleWhen?: unknown; - /** Evaluates true → the item renders disabled for this row. Fail-soft. */ - disabledWhen?: unknown; -} +/** + * Per-record CEL predicates gating a built-in Edit/Delete row action + * (objectui#2614) — DERIVED from the type whose values this file actually + * receives, never restated by hand (objectui#4429, inheriting PR #4423). + * + * The source is NOT the data-table's `DataTableSchema.rowEditPredicates` / + * `rowDeletePredicates`: this surface is never handed those keys. `ObjectGrid` + * resolves the OBJECT's `userActions.edit` / `delete` through + * `resolveRowCrudAffordances` (`../rowCrudAffordances`), which returns + * `CrudAffordances['editPredicates']` / `['deletePredicates']` — the spec-owned + * `RowCrudPredicates` (ADR-0103), parsed in exactly one place + * (`@object-ui/core`'s `normalizeUserAction`) and re-exported there from + * `@objectstack/spec/data`. Deriving from the data-table's twins would have + * bound this file to a schema key it never sees: a NEW wrong coupling in place + * of the old missing one. + * + * The union of the two twins is deliberate, exactly as in PR #4423: every + * consumer that reads this alias serves BOTH built-ins (`name: 'edit' | + * 'delete'`), so it may only read keys that BOTH affordance keys declare — if + * either twin dropped one, the read stops compiling instead of silently reading + * `undefined`. Today the twins are declared as one named type, so the union + * collapses onto it; the construction is what states the constraint that + * outlives that coincidence. + * + * Deriving also TIGHTENS what this file accepts, and that is the point rather + * than a side effect: the hand-written pair typed both keys `unknown`, while + * the spec types them `Expression | ExpressionInput` — the authored CEL + * shorthand or its `{ dialect, source }` envelope. `@object-ui/core` retired + * the same imprecision at its own seam and said so in as many words ("The local + * `unknown` was imprecision, not a deliberate dialect"); this was the last copy + * of it. + * + * Behavior is unchanged and stays the posture `RowCrudActionOverrideSchema` + * specifies: `visibleWhen` fails CLOSED (the item is not rendered for this row), + * `disabledWhen` fails soft (the item renders disabled), both evaluated per row + * on the canonical CEL engine — the same machinery as custom actions' + * `visible` / `disabled`. + */ +export type BuiltinRowActionPredicates = NonNullable< + CrudAffordances['editPredicates'] | CrudAffordances['deletePredicates'] +>; export interface RowActionMenuProps { /** The row data record */ @@ -166,9 +197,14 @@ function evalRowActionVisibility( * `visibleWhen` counts as a declared gate by `!= null`, not by truthiness — * `visibleWhen: false` hides the item rather than reading as "ungated". This is * the item's historical rule, extracted verbatim. + * + * The parameter is consumption-shaped — visibility is all this decides, so + * `disabledWhen` is deliberately absent — but `Pick`ed from the derived + * authoring shape rather than hand-written, so it is a SUBSET of that shape by + * construction (objectui#4429). */ export function isBuiltinRowActionVisible( - predicates: BuiltinRowActionPredicates | undefined, + predicates: Pick | undefined, name: 'edit' | 'delete', row: any, scope: Record, @@ -253,7 +289,11 @@ export interface RowActionMenuPlan { * `_actions` column's `cell`, so the decision lives here. * * Only visibility is decided here — a `disabled` item still renders (greyed - * out) and still COUNTS, exactly as before. Capability filtering + * out) and still COUNTS, exactly as before. That counting is pinned where it + * can be OBSERVED, on the rendered menu, by `RowActionMenu.emptyGuard.test.tsx`'s + * "keeps the trigger for a row whose only item renders merely DISABLED" + * (objectui#4429) — this function never reads `disabledWhen`, so no assertion on + * its return value can pin that claim. Capability filtering * (ADR-0066 D4 / framework#3923) happens upstream of this call, on the declared * set, so its verdict reaches the guard the same way. * @@ -290,8 +330,17 @@ export function planRowActionMenu(input: { canDelete?: boolean; onEdit?: unknown; onDelete?: unknown; - editPredicates?: BuiltinRowActionPredicates; - deletePredicates?: BuiltinRowActionPredicates; + /** + * The predicate parameters are consumption-shaped — this function reads + * `visibleWhen` and nothing else, and the signatures keep saying so — but each + * is `Pick`ed from the exact affordance key its production caller passes + * (`ObjectGrid` → `resolveRowCrudAffordances`), rather than hand-restated + * (objectui#4429). Same subset as before, minus the drift: a rename in the + * spec-owned `RowCrudPredicates` fails here at compile time instead of leaving + * a stale hand-copy that still type-checks against nothing. + */ + editPredicates?: Pick, 'visibleWhen'>; + deletePredicates?: Pick, 'visibleWhen'>; objectFields?: unknown; }): RowActionMenuPlan { const { row, scope, objectFields } = input; @@ -384,6 +433,13 @@ const RowActionMenuItem: React.FC<{ */ export const BuiltinRowActionItem: React.FC<{ name: 'edit' | 'delete'; + /** + * The whole authoring shape, because this component reads BOTH keys — + * derived from the affordance type rather than hand-copied (objectui#4429). + * This is the site with the most to gain: a hand-copy that keeps one key in + * common with a renamed source satisfies TS weak-type detection and drifts + * with ZERO diagnostics (measured on the data-table twin in PR #4423). + */ predicates?: BuiltinRowActionPredicates; row: any; icon: React.ReactNode; diff --git a/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx b/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx index d84726c9ab..551cf5020c 100644 --- a/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx +++ b/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx @@ -31,11 +31,12 @@ * `rowActions` that carry no predicate. */ import { describe, it, expect, vi, afterEach } from 'vitest'; -import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import { render, screen, waitFor, cleanup, fireEvent } from '@testing-library/react'; import '@testing-library/jest-dom'; import React from 'react'; import { PredicateScopeProvider, ActionProvider, SchemaRendererProvider } from '@object-ui/react'; import { registerAllFields } from '@object-ui/fields'; +import type { CrudAffordances } from '@object-ui/core'; import { RowActionMenu, planRowActionMenu } from '../RowActionMenu'; import { ObjectGrid } from '../../ObjectGrid'; @@ -43,6 +44,8 @@ registerAllFields(); /** The real `userActions.edit.visibleWhen` shape (objectui#2614). */ const NOT_FROZEN = 'record.frozen != true'; +/** Its `disabledWhen` counterpart — HOLDS for the frozen row, so it greys out. */ +const IS_FROZEN = 'record.frozen == true'; /** The reporter's shape: an action gated for a role the viewer is not in. */ const IS_APPROVER = 'record.approver == "u-me"'; @@ -136,6 +139,44 @@ describe('RowActionMenu — the "⋮" counts renderable items, not handlers (#35 renderMenu({ rowActions: ['send_email'], onAction: () => {} }); expect(trigger()).toBeInTheDocument(); }); + + /** + * The other side of #3562's rule, observed where a user meets it: a row whose + * only item is greyed out is NOT an empty menu. `disabledWhen` is a rendering + * verdict, not a visibility one, so the item counts, the trigger survives, and + * what it opens is that item — present and `aria-disabled` (objectui#4429). + * + * End-to-end on purpose: the two halves live apart — the count in + * `planRowActionMenu`, the greying in `BuiltinRowActionItem` — and each half's + * own test stays green while the other regresses. Neither could observe this + * pairing before: the planner's cases read its return value (and the planner + * never reads `disabledWhen` at all — see the renamed case below), while the + * item's own cases (`RowActionMenu.test.tsx`) render `BuiltinRowActionItem` + * directly inside an already-open dropdown, never through the component that + * decides whether a trigger exists at all. + * + * This is the level where the two halves actually meet: `RowActionMenu` is + * both the caller of the planner and the renderer of the item. The + * `ObjectGrid` describe further down is a level ABOVE it (column alignment + * plus the async `userActions` plumbing) and its cases read `visibleWhen` + * only, so it did not pin this either. + */ + it('keeps the trigger for a row whose only item renders merely DISABLED', async () => { + renderMenu({ + canEdit: true, + onEdit: () => {}, + editPredicates: { disabledWhen: IS_FROZEN }, + }); + + // Counted: a greyed-out item is still an item, so the row keeps its "⋮". + expect(trigger()).toBeInTheDocument(); + + // Radix opens on `pointerdown` (a plain click does nothing) and portals its + // content on the next tick, hence the async find. + fireEvent.pointerDown(trigger()!, { button: 0, ctrlKey: false, pointerType: 'mouse' }); + const item = await screen.findByTestId('row-action-builtin-edit'); + expect(item).toHaveAttribute('aria-disabled', 'true'); + }); }); /** @@ -446,13 +487,33 @@ describe('planRowActionMenu', () => { .toMatchObject({ legacy: ['send_email'], menuCount: 1 }); }); - it('still counts an item that renders merely DISABLED', () => { + // Renamed from "still counts an item that renders merely DISABLED" + // (objectui#4429). The planner never reads `disabledWhen`, so this fixture + // behaves identically to `{}` and could not pin the disabled-counting claim + // its old name made — that claim now lives where it can actually be observed, + // in "keeps the trigger for a row whose only item renders merely DISABLED" + // above. What survives here is the verdict this case really does decide, and + // it is worth deciding: a predicate object is not itself a gate. + it('an object with no `visibleWhen` does not hide the item', () => { + // Typed as what the production caller actually hands the planner — + // `resolveRowCrudAffordances`' `editPredicates`, whose spec-owned shape + // (`RowCrudPredicates`, ADR-0103) carries `disabledWhen` alongside + // `visibleWhen`. The planner's own parameter is now `Pick`ed from that same + // key rather than hand-restated (objectui#4429), so it stays the deliberate + // `visibleWhen`-only subset: a bare object literal trips excess-property + // checking, while the identical value reaches it unremarked in production. + // Annotating from the authoring type keeps the fixture honest about which + // key is being ignored, rather than deleting the key and quietly renaming + // the case. + const editPredicates: CrudAffordances['editPredicates'] = { + disabledWhen: IS_FROZEN, + }; expect(planRowActionMenu({ ...base, row: FROZEN, canEdit: true, onEdit: noop, - editPredicates: { disabledWhen: 'record.frozen == true' }, + editPredicates, })).toMatchObject({ edit: true, menuCount: 1 }); });