diff --git a/.changeset/row-action-declared-visible-gate-3758.md b/.changeset/row-action-declared-visible-gate-3758.md new file mode 100644 index 000000000..4de3afb38 --- /dev/null +++ b/.changeset/row-action-declared-visible-gate-3758.md @@ -0,0 +1,33 @@ +--- +"@object-ui/plugin-grid": patch +"@object-ui/components": patch +--- + +Row actions declaring `visible: false` are now hidden instead of rendered + +A custom row action's visibility **gate** was detected by truthiness, so +`visible: false` — the most explicit way an author can say "never show this" — +fell into the "no gate declared" branch and the action rendered for every row. +Both surfaces of the ObjectGrid row cell (the "⋮" overflow item and the inline +`variant:'primary'` button) and the data-table's row overflow menu read the same +gate, so all three rendered it; the `#3562` emptiness guard counts with that same +gate, so a row whose only action was `visible: false` also grew a "⋮" it could +not fill. + +The gate now detects a **declared** gate by `!= null && !== ''` and lets the +declaration itself decide — a boolean short-circuits to its own verdict rather +than being handed to the CEL engine. This is the invariant objectui#3492 already +established for the selection bar, whose `hasVisibilityGate` spells out why +truthiness cannot answer the question, and the same `!= null` posture the +built-in `visibleWhen` gate has always had. `visible: true` still renders, +`''` and an absent `visible` are still no gate at all, and no expression-valued +`visible` changes verdict. + +Behaviour change surface, deliberately narrow: only an action whose `visible` is +the literal boolean `false` (or another falsy non-empty value) changes — it goes +from rendered to hidden, which is what the declaration asked for. +`ActionSchema.visible` is `ExpressionInputSchema` with no boolean member, so +`objectstack build` cannot emit this shape; hand-written view JSON and +in-process callers constructing defs can, and did. The three row surfaces now +reach the same verdict as the selection bar and the record page header for every +non-expression shape, which `predicate-surface-parity` pins. diff --git a/packages/components/src/renderers/complex/__tests__/data-table-row-action-visible.test.tsx b/packages/components/src/renderers/complex/__tests__/data-table-row-action-visible.test.tsx index c1fc965b0..59e4b6e1f 100644 --- a/packages/components/src/renderers/complex/__tests__/data-table-row-action-visible.test.tsx +++ b/packages/components/src/renderers/complex/__tests__/data-table-row-action-visible.test.tsx @@ -114,3 +114,36 @@ describe('data-table row action — visible / disabled CEL evaluation', () => { expect(screen.getByTestId('row-action-locked_action')).toHaveAttribute('data-disabled'); }); }); + +/** + * objectui#3758 — a DECLARED boolean `visible` is a verdict, not a missing gate. + * + * `isCustomRowActionVisible` used to ask truthiness (`if (!action?.visible) + * return true`), so `visible: false` — the most explicit way to say "never show + * this" — answered "no gate declared" and the item rendered for every row. + * Declaration is now detected by `!= null && !== ''`, the invariant + * objectui#3492 established for the selection bar (`hasVisibilityGate`) and the + * one the built-in `visibleWhen` gate has always used, so a declared boolean + * reaches the evaluator and decides. + * + * The `visible: true` and empty-string cases are asserted alongside on purpose: + * they are what separates "detect the declaration" from "hide unconditionally". + */ +describe('data-table row action — declared boolean `visible` (objectui#3758)', () => { + it('hides an action declaring `visible: false`', () => { + renderRowActionItem({ name: 'ghost', label: 'Ghost', visible: false }, { id: '2', role: 'member' }); + expect(screen.queryByTestId('row-action-ghost')).toBeNull(); + expect(screen.queryByText('Ghost')).toBeNull(); + }); + + it('renders an action declaring `visible: true`', () => { + renderRowActionItem({ name: 'always', label: 'Always', visible: true }, { id: '2', role: 'member' }); + expect(screen.getByTestId('row-action-always')).toBeInTheDocument(); + expect(screen.getByText('Always')).toBeInTheDocument(); + }); + + it('treats an empty-string `visible` as no gate at all, matching `hasVisibilityGate`', () => { + renderRowActionItem({ name: 'compiled_away', label: 'Compiled Away', visible: '' }, { id: '2', role: 'member' }); + expect(screen.getByTestId('row-action-compiled_away')).toBeInTheDocument(); + }); +}); diff --git a/packages/components/src/renderers/complex/__tests__/data-table-row-menu-empty-guard.test.tsx b/packages/components/src/renderers/complex/__tests__/data-table-row-menu-empty-guard.test.tsx index f03f18bbe..c5f98878b 100644 --- a/packages/components/src/renderers/complex/__tests__/data-table-row-menu-empty-guard.test.tsx +++ b/packages/components/src/renderers/complex/__tests__/data-table-row-menu-empty-guard.test.tsx @@ -243,3 +243,46 @@ describe('planDataTableRowMenu', () => { expect(plan).toMatchObject({ edit: false, count: 0 }); }); }); + +/** + * objectui#3758 reaching the guard: `planDataTableRowMenu` counts with the same + * `isCustomRowActionVisible` the item gates itself on, so correcting the gate + * propagates to whether the row gets a "⋮" at all — no separate change, and no + * way for the trigger and its contents to disagree about a boolean `visible`. + */ +describe('planDataTableRowMenu — declared boolean `visible` (objectui#3758)', () => { + const scope = {}; + const row = MIXED_ROWS[1]; + + it('drops a custom action declaring `visible: false`, reaching zero items', () => { + const plan = planDataTableRowMenu({ + customActions: [{ name: 'ghost', visible: false }] as any, + row, + scope, + }); + expect(plan.custom).toEqual([]); + expect(plan.count).toBe(0); + }); + + // Keeps the assertion above from passing for the empty reason: a gate + // rewritten to "always hide" would also reach `count: 0`. + it('keeps a custom action declaring `visible: true` — declaration detection, not "always hide"', () => { + const plan = planDataTableRowMenu({ + customActions: [{ name: 'always', visible: true }] as any, + row, + scope, + }); + expect(plan.custom.map((a) => a.name)).toEqual(['always']); + expect(plan.count).toBe(1); + }); + + it('treats an empty-string `visible` as no gate at all, matching `hasVisibilityGate`', () => { + const plan = planDataTableRowMenu({ + customActions: [{ name: 'compiled_away', visible: '' }] as any, + row, + scope, + }); + expect(plan.custom.map((a) => a.name)).toEqual(['compiled_away']); + expect(plan.count).toBe(1); + }); +}); diff --git a/packages/components/src/renderers/complex/data-table.tsx b/packages/components/src/renderers/complex/data-table.tsx index 0ad6e3f28..f8193a0a9 100644 --- a/packages/components/src/renderers/complex/data-table.tsx +++ b/packages/components/src/renderers/complex/data-table.tsx @@ -210,6 +210,11 @@ function evalRowActionVisibility( label: string, ): boolean { if (typeof pred === 'boolean') return pred; + // Not dead, and not the place that decides "was a gate declared?" — the two + // callers below answer that first, each by its own rule. `''` still arrives + // here from `isBuiltinRowActionVisible`, whose gate is `!= null` alone; a + // nothing-to-evaluate predicate that reached an evaluator fails CLOSED like + // any other unevaluable one. if (pred == null || pred === '') return false; return evalRowPredicate(pred as never, row ?? {}, { fallback: false, @@ -245,19 +250,28 @@ export function isBuiltinRowActionVisible( * Does this schema-driven custom row action render for THIS row? Same * single-definition rule as the built-ins above. * - * The gate is truthiness, which is what the item has always applied: a - * `visible: false` def renders (the objectui#3492 shape). Preserved verbatim — - * the contract this function exists for is that the guard and the item agree, - * and re-deciding `visible: false` would change WHICH items render, a separate - * question tracked on its own issue. + * A gate counts as DECLARED by `!= null && !== ''`, never by truthiness + * (objectui#3758). Truthiness cannot answer the question: `visible: false` is a + * declared gate that excludes every row, and testing `!action.visible` + * classified it as *ungated* — so the most explicit way to say "never show this" + * rendered the item for everyone, and counted toward the "⋮" guard. This is the + * invariant objectui#3492 established for the selection bar (plugin-grid's + * `hasVisibilityGate`), and the same `!= null` posture the built-in `visibleWhen` + * gate above has always had. The boolean then decides in + * {@link evalRowActionVisibility}, which short-circuits it instead of handing it + * to the engine. + * + * `''` is grouped with `null` deliberately: an empty predicate is nothing to + * evaluate, so it must not hide the item from everyone either. */ export function isCustomRowActionVisible( action: { name?: string; visible?: unknown } | undefined, row: any, scope: Record, ): boolean { - if (!action?.visible) return true; - return evalRowActionVisibility(action.visible, row, scope, action.name ?? 'row-action'); + const pred = action?.visible; + if (pred == null || pred === '') return true; + return evalRowActionVisibility(pred, row, scope, action?.name ?? 'row-action'); } /** What the row overflow menu will actually render for ONE row. */ diff --git a/packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx b/packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx index f213767fc..e3a893b8b 100644 --- a/packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx +++ b/packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx @@ -21,11 +21,14 @@ * "⋮" guard share) and the selection bar (`partitionBulkRows`), and depends on * `@object-ui/components`, which owns `page:header`. * - * Scope of the claim, deliberately: these fixtures are all non-empty predicate - * STRINGS / envelopes, i.e. the dialect question. Boolean and empty `visible` - * are a separate, still-open divergence — the kebab renders a `visible: false` - * def (objectui#3758) while the header hides it — and pinning them here would - * read as blessing a difference this issue did not fix. + * Scope of the claim: most fixtures are non-empty predicate STRINGS / envelopes, + * i.e. the dialect question #3521 asked. The boolean and empty-string shapes were + * excluded while they still diverged — the kebab rendered a `visible: false` def + * while the bar and the header hid it — and pinning them then would have read as + * blessing a difference #3521 did not fix. objectui#3758 closed that divergence + * by making the kebab's gate detect a DECLARED gate (`!= null && !== ''`) instead + * of a truthy one, so they belong in the table now: the shapes an author can + * write with no expression at all reach one verdict on all three surfaces too. */ import { describe, it, expect } from 'vitest'; @@ -203,6 +206,13 @@ const CASES: Case[] = [ }, // A predicate that cannot be evaluated: fail-CLOSED on all three. { what: 'faulting predicate fails closed', name: 'par_fault', visible: 'no_such_var_par == 1', expected: false }, + // The non-expression shapes, parity-checked since objectui#3758. A declared + // BOOLEAN is a verdict every surface short-circuits rather than evaluating; + // `''` and an absent `visible` are no gate at all on every surface. + { what: 'declared boolean false excludes every row', name: 'par_bool_false', visible: false, expected: false }, + { what: 'declared boolean true offers every row', name: 'par_bool_true', visible: true, expected: true }, + { what: 'empty-string `visible` is no gate', name: 'par_empty_string', visible: '', expected: true }, + { what: 'absent `visible` is no gate', name: 'par_absent', visible: undefined, expected: true }, ]; describe('one predicate, one verdict on all three action surfaces (#3521)', () => { diff --git a/packages/plugin-grid/src/components/RowActionMenu.tsx b/packages/plugin-grid/src/components/RowActionMenu.tsx index 5cabb9a26..1d4ee4550 100644 --- a/packages/plugin-grid/src/components/RowActionMenu.tsx +++ b/packages/plugin-grid/src/components/RowActionMenu.tsx @@ -132,6 +132,11 @@ function evalRowActionVisibility( fields: unknown, ): boolean { if (typeof pred === 'boolean') return pred; + // Not dead, and not the place that decides "was a gate declared?" — the two + // callers below answer that first, each by its own rule. `''` still arrives + // here from `isBuiltinRowActionVisible`, whose gate is `!= null` alone; a + // nothing-to-evaluate predicate that reached an evaluator fails CLOSED like + // any other unevaluable one. if (pred == null || pred === '') return false; return evalRowPredicate(pred as never, row ?? {}, { fallback: false, @@ -175,11 +180,19 @@ export function isBuiltinRowActionVisible( * ({@link RowActionInlineButton}). One function, so a def cannot be visible on * one surface and hidden on the other. * - * The gate is truthiness, which is what both items have always applied: a - * `visible: false` def RENDERS (the objectui#3492 shape). Preserved verbatim — - * the contract this function exists for is that the guard and the items agree, - * and re-deciding `visible: false` would change WHICH items render, a separate - * question tracked as objectui#3758. + * A gate counts as DECLARED by `!= null && !== ''`, never by truthiness + * (objectui#3758). Truthiness cannot answer the question: `visible: false` is a + * declared gate that excludes every row, and testing `!def.visible` classified + * it as *ungated* — so the most explicit way to say "never show this" rendered + * the action for everyone, on both surfaces and in the "⋮" guard's count. This + * is `bulkEligibility`'s `hasVisibilityGate` verbatim, the invariant + * objectui#3492 established for the selection bar, and the same `!= null` + * posture the built-in `visibleWhen` gate above has always had. The boolean then + * decides in {@link evalRowActionVisibility}, which short-circuits it instead of + * handing it to the engine. + * + * `''` is grouped with `null` deliberately: an empty predicate is nothing to + * evaluate, so it must not hide the action from everyone either. */ export function isCustomRowActionVisible( def: RowActionDef | undefined, @@ -187,8 +200,9 @@ export function isCustomRowActionVisible( scope: Record, fields?: unknown, ): boolean { - if (!def?.visible) return true; - return evalRowActionVisibility(def.visible, row, scope, def.name ?? 'row-action', fields); + const pred = def?.visible; + if (pred == null || pred === '') return true; + return evalRowActionVisibility(pred, row, scope, def?.name ?? 'row-action', fields); } /** What this row's action cell will actually render. */ 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 389b39786..02ed883e6 100644 --- a/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx +++ b/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx @@ -410,17 +410,60 @@ describe('planRowActionMenu', () => { } }); - it('preserves the `visible: false` truthy gate verbatim (objectui#3758, not this issue)', () => { - // `!def.visible` reads `false` as "ungated", so the def renders and counts — - // exactly what the item components have always done. Re-deciding this would - // change WHICH items render, which is out of #3562's scope; the point of the - // shared function is only that the guard and the items agree. + // objectui#3758 replaced the fixture that used to live here. It pinned the + // truthiness gate verbatim — `!def.visible` read `false` as "ungated", so the + // def rendered and counted — because re-deciding `visible: false` changes + // WHICH items render and was out of #3562's scope. #3758 then decided it, the + // other way: a declared boolean is a verdict (the #3492 invariant), so the + // fixture's expectations (`['ghost']` / `1`) are now the wrong verdicts and + // are replaced rather than re-spelled. + it('a declared `visible: false` excludes the def and leaves no trigger (objectui#3758)', () => { const plan = planRowActionMenu({ ...base, row: FROZEN, menuDefs: [{ name: 'ghost', visible: false }], }); - expect(plan.custom.map((a) => a.name)).toEqual(['ghost']); + expect(plan.custom).toEqual([]); + expect(plan.menuCount).toBe(0); + }); + + // The counterpart that keeps the assertion above from passing for the empty + // reason: a gate rewritten to "always hide" would also reach `menuCount: 0`. + it('a declared `visible: true` still counts — declaration detection, not "always hide"', () => { + const plan = planRowActionMenu({ + ...base, + row: FROZEN, + menuDefs: [{ name: 'always', visible: true }], + }); + expect(plan.custom.map((a) => a.name)).toEqual(['always']); expect(plan.menuCount).toBe(1); }); + + it('an empty-string `visible` is no gate at all, matching `hasVisibilityGate`', () => { + const plan = planRowActionMenu({ + ...base, + row: FROZEN, + menuDefs: [{ name: 'compiled_away', visible: '' }], + }); + expect(plan.custom.map((a) => a.name)).toEqual(['compiled_away']); + expect(plan.menuCount).toBe(1); + }); +}); + +/** + * The DOM half of objectui#3758 on this surface: a row whose only custom action + * declares `visible: false` renders no "⋮" at all. This is the #3562 guard and + * the #3758 gate meeting — the guard counts what the items will render, so + * correcting the gate propagates to the trigger with no separate change. + */ +describe('declared `visible: false` reaches the "⋮" guard (objectui#3758)', () => { + it('renders no trigger when the row\'s only custom action declares `visible: false`', () => { + renderMenu({ rowActionDefs: [{ name: 'ghost', label: 'Ghost', variant: 'secondary', visible: false }] }); + expect(trigger()).not.toBeInTheDocument(); + }); + + it('keeps the trigger when that same action declares `visible: true`', () => { + renderMenu({ rowActionDefs: [{ name: 'ghost', label: 'Ghost', variant: 'secondary', visible: true }] }); + expect(trigger()).toBeInTheDocument(); + }); }); diff --git a/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx b/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx index b680e6083..1786f1a5c 100644 --- a/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx +++ b/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx @@ -17,6 +17,7 @@ */ import { describe, it, expect, vi } from 'vitest'; import { render, screen, fireEvent } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import '@testing-library/jest-dom'; import React from 'react'; import { PredicateScopeProvider } from '@object-ui/react'; @@ -196,3 +197,75 @@ describe('BuiltinRowActionItem per-record CEL predicates (#2614)', () => { } }); }); + +/** + * objectui#3758 — a DECLARED boolean `visible` is a verdict, not a missing gate. + * + * Both of this component's custom-action surfaces read one gate + * (`isCustomRowActionVisible`), and that gate used to ask truthiness: `visible: + * false` — the most explicit way to say "never show this" — answered "no gate + * declared" and the action rendered for everyone. Declaration is detected by + * `!= null && !== ''`, the invariant objectui#3492 already established for the + * selection bar (`hasVisibilityGate`) and the one the built-in `visibleWhen` + * gate has always used, so a boolean reaches the evaluator and decides. + * + * The `visible: true` and undeclared cases are asserted alongside on purpose: + * they are what separates "detect the declaration" from "hide unconditionally", + * a rewrite that would satisfy every `visible: false` assertion on its own. + */ +describe('declared boolean `visible` on a custom row action (objectui#3758)', () => { + const UNGATED = { name: 'archive', label: 'Archive', variant: 'secondary' as const }; + const GHOST = { name: 'ghost', label: 'Ghost', variant: 'secondary' as const, visible: false }; + + // --- surface 1: the "⋮" overflow menu item ------------------------------- + // A second, UNGATED action rides along so the "⋮" trigger survives its own + // #3562 emptiness guard — otherwise a passing assertion could not tell "the + // item was suppressed" from "the whole menu disappeared". + + it('visible:false → the overflow menu item does not render', async () => { + renderMenu({ rowActionDefs: [GHOST, UNGATED] }); + await userEvent.click(screen.getByTestId('row-action-trigger')); + expect(screen.getByTestId('row-action-archive')).toBeInTheDocument(); + expect(screen.queryByTestId('row-action-ghost')).not.toBeInTheDocument(); + }); + + it('visible:true → the overflow menu item renders', async () => { + renderMenu({ rowActionDefs: [{ ...GHOST, name: 'always', label: 'Always', visible: true }, UNGATED] }); + await userEvent.click(screen.getByTestId('row-action-trigger')); + expect(screen.getByTestId('row-action-always')).toBeInTheDocument(); + }); + + it('no `visible` at all → the overflow menu item renders (ungated stays ungated)', async () => { + renderMenu({ rowActionDefs: [UNGATED] }); + await userEvent.click(screen.getByTestId('row-action-trigger')); + expect(screen.getByTestId('row-action-archive')).toBeInTheDocument(); + }); + + // --- surface 2: the inline `variant:'primary'` button -------------------- + // Always mounted, so it needs no menu interaction to observe. + + it('visible:false → the inline primary button does not render', () => { + renderMenu({ rowActionDefs: [{ ...OPEN, visible: false }] }); + expect(screen.queryByTestId('row-action-inline-open')).not.toBeInTheDocument(); + }); + + it('visible:true → the inline primary button renders', () => { + renderMenu({ rowActionDefs: [{ ...OPEN, visible: true }] }); + expect(screen.getByTestId('row-action-inline-open')).toBeInTheDocument(); + }); + + it('no `visible` at all → the inline primary button renders (ungated stays ungated)', () => { + renderMenu({ rowActionDefs: [OPEN] }); + expect(screen.getByTestId('row-action-inline-open')).toBeInTheDocument(); + }); + + // --- the other half of "declared": empty string is NOT a gate ------------ + // `hasVisibilityGate` excludes `''` for the selection bar; the row surfaces + // match it, so an action whose predicate compiled away to an empty string is + // not silently hidden from everyone. + + it('an empty-string `visible` is not a declared gate — the action still renders', () => { + renderMenu({ rowActionDefs: [{ ...OPEN, visible: '' }] }); + expect(screen.getByTestId('row-action-inline-open')).toBeInTheDocument(); + }); +});