diff --git a/.changeset/row-action-inline-slot-survivors-3762.md b/.changeset/row-action-inline-slot-survivors-3762.md new file mode 100644 index 000000000..b831dd67a --- /dev/null +++ b/.changeset/row-action-inline-slot-survivors-3762.md @@ -0,0 +1,39 @@ +--- +"@object-ui/plugin-grid": patch +--- + +Grid row actions: the inline button budget is now spent on the primaries that actually render + +`RowActionMenu` allocated its inline slots on the **declared** row actions, before +any `visible` predicate ran: + +```ts +const primaryDefs = gatedActionDefs.filter(d => d.variant === 'primary'); +const inlineDefs = primaryDefs.slice(0, Math.max(0, maxInlineActions)); +``` + +So on a row where the *leading* `variant: 'primary'` action was suppressed by its +own `visible`, that action still held the slot — `RowActionInlineButton` returned +`null` into it — while the next primary, the one that *did* survive the row's +predicates, had already been sliced into the overflow list. The row then rendered +**no inline button and a "⋮" hiding its main CTA**, even though exactly one primary +was visible and `maxInlineActions` (default 1) allowed exactly one inline button. + +Slot allocation now happens inside `planRowActionMenu`, after visibility, so the +budget is only ever spent on a primary that renders. `maxInlineActions` is +unchanged in meaning and default — it is a width budget for real buttons, and +counting an invisible action against it protected no layout. + +Behaviour change surface, deliberately narrow: + +- a row with 2 or more primaries where a *leading* one is suppressed for that row — + the surviving primary moves from the "⋮" menu to an inline button, and the "⋮" + disappears if nothing else is left to fold; +- unchanged: how many primaries may go inline, the menu order (folded primaries + above secondaries), which items render at all, the ADR-0066 D4 capability gate + (still applied once to the declared set, upstream of this decision), and the + #3562 empty-menu guard — a row with nothing renderable still grows no trigger. + +Rows whose primaries are all ungated (the `sys_environment` Open + Upgrade Plan +shape that motivated `maxInlineActions`) are bit-for-bit unaffected: declared order +and surviving order coincide. diff --git a/packages/plugin-grid/src/components/RowActionMenu.tsx b/packages/plugin-grid/src/components/RowActionMenu.tsx index 1d4ee4550..acf139e5c 100644 --- a/packages/plugin-grid/src/components/RowActionMenu.tsx +++ b/packages/plugin-grid/src/components/RowActionMenu.tsx @@ -93,6 +93,12 @@ export interface RowActionMenuProps { * before the rest fold into the "⋮" overflow menu. Bounds the row's inline * width so multiple primary actions (e.g. Open + Upgrade Plan) can't crowd * and clip each other in the narrow actions column. Defaults to 1. + * + * Counts the primaries that actually RENDER for this row, not the ones + * declared: a primary suppressed by its own `visible` occupies no slot + * (objectui#3762). The bound is a width budget for real buttons, so spending + * it on an action that renders nothing helps no layout and hides the row's CTA + * behind the "⋮". */ maxInlineActions?: number; /** @@ -207,13 +213,21 @@ export function isCustomRowActionVisible( /** What this row's action cell will actually render. */ export interface RowActionMenuPlan { - /** Inline `variant:'primary'` buttons surviving their own `visible`. */ + /** + * The `variant:'primary'` actions rendering as inline buttons: the first + * `maxInlineActions` primaries that SURVIVE their own `visible` for this row, + * in declared order (objectui#3762). + */ inline: RowActionDef[]; /** The built-in Edit item renders in the menu for this row. */ edit: boolean; /** The built-in Delete item renders in the menu for this row. */ remove: boolean; - /** Menu-bound defs surviving their own `visible`, in declared order. */ + /** + * Menu-bound defs surviving their own `visible`: the surviving primaries that + * did not fit an inline slot, then the surviving non-primaries — each group in + * declared order, primaries above secondaries as before. + */ custom: RowActionDef[]; /** Legacy string row actions. Predicate-free, so all of them render. */ legacy: string[]; @@ -243,18 +257,33 @@ export interface RowActionMenuPlan { * (ADR-0066 D4 / framework#3923) happens upstream of this call, on the declared * set, so its verdict reaches the guard the same way. * - * Inline slot allocation is deliberately NOT re-derived from visibility: the - * `maxInlineActions` slice runs on the declared primaries, exactly as before, - * so a suppressed primary does not promote the next one into its slot. Which - * items render, and where, is unchanged — only whether the trigger renders. + * Inline slot allocation happens HERE, after visibility, and that ordering is + * the whole of objectui#3762: an inline slot is spent only on a primary that + * actually renders for this row. It used to be sliced off the DECLARED primaries + * before any predicate ran, so a suppressed leading primary held a slot that + * rendered nothing (`RowActionInlineButton` returned `null` into it) while the + * next primary — the one that DID survive — had already been folded into the "⋮". + * A row with exactly one visible primary and a budget of one inline button + * showed no button and a "⋮" hiding its main CTA. Deciding survival and + * placement in one function is also why they cannot drift: there is no second + * place that partitions the defs. */ export function planRowActionMenu(input: { row: any; scope: Record; - /** Primaries occupying the inline slots, already sliced by `maxInlineActions`. */ - inlineDefs: readonly RowActionDef[]; - /** Defs bound for the "⋮" menu (folded primaries + non-primaries). */ - menuDefs: readonly RowActionDef[]; + /** + * The row's action defs in DECLARED order, already capability-gated + * (ADR-0066 D4) but NOT partitioned — this function splits primary from + * non-primary and allocates the inline slots, because both decisions depend on + * which defs survive `visible` (objectui#3762). + */ + actionDefs?: readonly RowActionDef[]; + /** + * Inline-button budget, i.e. how many SURVIVING primaries render inline before + * the rest fold into the menu. Defaults to 1, matching + * {@link RowActionMenuProps.maxInlineActions}; negative values are clamped to 0. + */ + maxInlineActions?: number; /** Legacy string row actions (no predicates). */ rowActions?: readonly string[]; canEdit?: boolean; @@ -266,12 +295,24 @@ export function planRowActionMenu(input: { objectFields?: unknown; }): RowActionMenuPlan { const { row, scope, objectFields } = input; - const inline = input.inlineDefs.filter((def) => isCustomRowActionVisible(def, row, scope, objectFields)); const edit = Boolean(input.canEdit && input.onEdit) && isBuiltinRowActionVisible(input.editPredicates, 'edit', row, scope, objectFields); const remove = Boolean(input.canDelete && input.onDelete) && isBuiltinRowActionVisible(input.deletePredicates, 'delete', row, scope, objectFields); - const custom = input.menuDefs.filter((def) => isCustomRowActionVisible(def, row, scope, objectFields)); + // One `visible` evaluation per def, as before — the partition below is pure + // array work on the survivors, so honoring the fix costs nothing extra. + const surviving = (input.actionDefs ?? []).filter( + (def) => isCustomRowActionVisible(def, row, scope, objectFields), + ); + const survivingPrimaries = surviving.filter((def) => def.variant === 'primary'); + const inlineBudget = Math.max(0, input.maxInlineActions ?? 1); + const inline = survivingPrimaries.slice(0, inlineBudget); + const custom = [ + // Primaries that outran the inline budget stay above the secondaries, the + // menu order this surface has always had. + ...survivingPrimaries.slice(inlineBudget), + ...surviving.filter((def) => def.variant !== 'primary'), + ]; const legacy = [...(input.rowActions ?? [])]; return { inline, @@ -456,33 +497,22 @@ export const RowActionMenu: React.FC = ({ () => (rowActionDefs ?? []).filter(d => mayInvoke((d as any)?.requiredPermissions)), [rowActionDefs, mayInvoke], ); - // Surface `variant: 'primary'` row actions inline (as the row's main CTA); - // everything else stays in the "⋮" overflow menu. Only the first - // `maxInlineActions` primaries render inline — any extra primaries fold into - // the menu (kept above secondary actions) so a row never renders more inline - // buttons than the actions column can show, which previously clipped the - // leftmost button (e.g. "Open" hidden behind "Upgrade Plan"). - // - // The slice runs on the DECLARED primaries, before any `visible` predicate is - // evaluated — unchanged on purpose. Re-deriving the slots from the surviving - // primaries would promote the next primary into a suppressed one's slot, i.e. - // change WHERE an item renders; #3562 is only about whether the "⋮" renders. - const { inlineDefs, menuDefs } = React.useMemo(() => { - const primaryDefs = gatedActionDefs.filter(d => d.variant === 'primary'); - const max = Math.max(0, maxInlineActions); - return { - inlineDefs: primaryDefs.slice(0, max), - menuDefs: [ - ...primaryDefs.slice(max), - ...gatedActionDefs.filter(d => d.variant !== 'primary'), - ], - }; - }, [gatedActionDefs, maxInlineActions]); // What this row will ACTUALLY render, per-record predicates applied — the // guard and the items read one visibility source, so they cannot disagree. // + // `variant: 'primary'` actions surface inline (as the row's main CTA) and + // everything else stays in the "⋮" overflow menu; only `maxInlineActions` of + // them render inline, so a row never shows more inline buttons than the actions + // column can fit — which previously clipped the leftmost button ("Open" hidden + // behind "Upgrade Plan"). That partition lives inside `planRowActionMenu` + // rather than here (objectui#3762) precisely because the budget must be spent + // on the primaries that SURVIVE this row's predicates: slicing the declared + // primaries first left a suppressed one holding an empty slot while the + // surviving next primary was already folded behind the "⋮". + // // objectui#3562: the old guard asked whether handlers were supplied and how - // many actions were DECLARED (`(canEdit && onEdit) || … || menuDefs.length > 0`), + // many actions were DECLARED (`(canEdit && onEdit) || … || menuDefs.length > 0` + // — `menuDefs` being the pre-partitioned slice that #3762 moved into the plan), // while the items were filtered a second time, per row, by `visibleWhen` / // `visible`. Handlers present + every item suppressed for this record = a "⋮" // that opens an empty box — measured at 128×10 with zero `[role=menu]` @@ -496,8 +526,8 @@ export const RowActionMenu: React.FC = ({ planRowActionMenu({ row, scope, - inlineDefs, - menuDefs, + actionDefs: gatedActionDefs, + maxInlineActions, rowActions, canEdit, canDelete, @@ -510,8 +540,8 @@ export const RowActionMenu: React.FC = ({ [ row, scope, - inlineDefs, - menuDefs, + gatedActionDefs, + maxInlineActions, rowActions, canEdit, canDelete, 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 02ed883e6..d84726c9a 100644 --- a/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx +++ b/packages/plugin-grid/src/components/__tests__/RowActionMenu.emptyGuard.test.tsx @@ -175,20 +175,75 @@ describe('RowActionMenu — the inline primary button shares one visibility sour expect(trigger()).not.toBeInTheDocument(); }); - it('a suppressed primary does NOT promote the next primary into its inline slot', () => { - // Slot allocation still runs on the DECLARED primaries: #3562 is about - // whether the trigger renders, not about relocating items. `upgrade` stays - // folded into the menu (and so the menu, holding one item, keeps its "⋮"). + // objectui#3762 replaced the fixture that used to live here. It pinned the + // status quo — `a suppressed primary does NOT promote the next primary into its + // inline slot` — because slot allocation ran on the DECLARED primaries and + // #3562's ruling covered only whether the trigger renders, not where an item + // lands. #3762 then decided the placement question, the other way: a slot is a + // width budget for a button that renders, so a suppressed primary holds none. + // The old expectations (`upgrade` not inline, a "⋮" present) are now the wrong + // verdicts, so the case is replaced rather than re-spelled — see the describe + // below, which asserts the opposite direction on the same fixture. +}); + +/** + * objectui#3762 — the inline budget belongs to the primaries that RENDER. + * + * Same shape as the cloud `sys_environment` list that motivated + * `maxInlineActions` in the first place (two `variant:'primary'` actions, Open + + * Upgrade Plan), with the leading one gated for this row. Slicing the declared + * primaries left that suppressed action holding the single inline slot — + * `RowActionInlineButton` returned `null` into it — while the surviving primary + * had already been folded into the "⋮". The row then showed no inline button at + * all and hid its main CTA one click deep, even though exactly one primary was + * visible and the budget was exactly one. + */ +describe('RowActionMenu — inline slots go to SURVIVING primaries (objectui#3762)', () => { + const APPROVE_IF_APPROVER = { + name: 'approve', + label: 'Approve', + variant: 'primary' as const, + visible: IS_APPROVER, + }; + const UPGRADE = { name: 'upgrade', label: 'Upgrade', variant: 'primary' as const }; + + it('a suppressed leading primary yields its inline slot to the surviving one', () => { + renderMenu({ rowActionDefs: [APPROVE_IF_APPROVER, UPGRADE] }); + // The gated primary renders nowhere, as before. + expect(screen.queryByTestId('row-action-inline-approve')).not.toBeInTheDocument(); + // …and the primary that DOES survive now takes the slot instead of folding. + expect(screen.getByTestId('row-action-inline-upgrade')).toBeInTheDocument(); + // With the budget spent on a real button there is nothing left to fold, so + // this row has no "⋮" at all. Before #3762 this row rendered ONLY a "⋮". + expect(trigger()).not.toBeInTheDocument(); + }); + + it('both primaries surviving → the second still folds into the menu (budget unchanged)', () => { + // The guard against over-correcting: #3762 changes WHICH primaries compete + // for the slots, never how many there are. `maxInlineActions` still defaults + // to 1, so the clipped-column regression the budget exists for stays fixed. renderMenu({ - rowActionDefs: [ - OPEN_IF_APPROVER, - { name: 'upgrade', label: 'Upgrade', variant: 'primary' }, - ], + row: { ...DRAFT, approver: 'u-me' }, + rowActionDefs: [APPROVE_IF_APPROVER, UPGRADE], }); - expect(screen.queryByTestId('row-action-inline-approve')).not.toBeInTheDocument(); + expect(screen.getByTestId('row-action-inline-approve')).toBeInTheDocument(); expect(screen.queryByTestId('row-action-inline-upgrade')).not.toBeInTheDocument(); + // `upgrade` is folded, so the trigger is back — and holds it. expect(trigger()).toBeInTheDocument(); }); + + it('every primary suppressed → no inline button and no "⋮" (guard cross-check)', () => { + // The empty-guard invariant (#3562) still holds under survivor-based slots: + // reallocating slots must not conjure a trigger for a row with nothing to + // show. Both primaries gate on being the approver, and FROZEN is not. + renderMenu({ + rowActionDefs: [APPROVE_IF_APPROVER, { ...UPGRADE, visible: IS_APPROVER }], + }); + expect(screen.queryByTestId('row-action-inline-approve')).not.toBeInTheDocument(); + expect(screen.queryByTestId('row-action-inline-upgrade')).not.toBeInTheDocument(); + expect(trigger()).not.toBeInTheDocument(); + expect(screen.queryByRole('menu')).toBeNull(); + }); }); /** @@ -285,14 +340,21 @@ describe('ObjectGrid actions column stays aligned when a row loses its menu (#35 /** * The resolution the guard counts with, exercised directly: the DOM tests above - * pin whether a trigger exists, these pin WHICH items it would hold. Both read - * the same `planRowActionMenu`, and the item components re-read the same - * visibility functions — so the trigger and its contents cannot drift apart. + * pin whether a trigger exists, these pin WHICH items it would hold — and, since + * objectui#3762, WHERE each one lands. Both read the same `planRowActionMenu`, + * and the item components re-read the same visibility functions — so the trigger + * and its contents cannot drift apart. + * + * `actionDefs` is the whole declared (capability-gated) set in declared order; + * the function partitions primary from non-primary itself, because the inline + * budget may only be spent on defs that survive `visible` (#3762). Defs without + * a `variant` are non-primary, so the menu-side cases below read the same as when + * this suite handed the split in pre-sliced. */ describe('planRowActionMenu', () => { const scope = {}; const noop = () => {}; - const base = { scope, inlineDefs: [], menuDefs: [] } as const; + const base = { scope, actionDefs: [] } as const; it('counts nothing when nothing is wired', () => { expect(planRowActionMenu({ ...base, row: DRAFT })).toMatchObject({ @@ -349,7 +411,7 @@ describe('planRowActionMenu', () => { it('keeps the menu defs whose `visible` passes, in declared order', () => { const plan = planRowActionMenu({ ...base, - menuDefs: [ + actionDefs: [ { name: 'unfreeze', visible: 'record.frozen == true' }, { name: 'publish', visible: NOT_FROZEN }, { name: 'view' }, @@ -363,7 +425,7 @@ describe('planRowActionMenu', () => { it('filters the inline primaries too, without counting them toward the trigger', () => { const plan = planRowActionMenu({ ...base, - inlineDefs: [{ name: 'approve', variant: 'primary', visible: IS_APPROVER }], + actionDefs: [{ name: 'approve', variant: 'primary', visible: IS_APPROVER }], row: FROZEN, }); expect(plan.inline).toEqual([]); @@ -372,7 +434,7 @@ describe('planRowActionMenu', () => { expect(plan.menuCount).toBe(0); const passing = planRowActionMenu({ ...base, - inlineDefs: [{ name: 'approve', variant: 'primary', visible: IS_APPROVER }], + actionDefs: [{ name: 'approve', variant: 'primary', visible: IS_APPROVER }], row: { ...DRAFT, approver: 'u-me' }, }); expect(passing.inline.map((a) => a.name)).toEqual(['approve']); @@ -403,7 +465,7 @@ describe('planRowActionMenu', () => { canEdit: true, onEdit: noop, editPredicates: { visibleWhen: 'record.frozen ==' }, - menuDefs: [{ name: 'broken', visible: 'record.frozen ==' }], + actionDefs: [{ name: 'broken', visible: 'record.frozen ==' }], })).toMatchObject({ edit: false, custom: [], menuCount: 0 }); } finally { warn.mockRestore(); @@ -421,7 +483,7 @@ describe('planRowActionMenu', () => { const plan = planRowActionMenu({ ...base, row: FROZEN, - menuDefs: [{ name: 'ghost', visible: false }], + actionDefs: [{ name: 'ghost', visible: false }], }); expect(plan.custom).toEqual([]); expect(plan.menuCount).toBe(0); @@ -433,7 +495,7 @@ describe('planRowActionMenu', () => { const plan = planRowActionMenu({ ...base, row: FROZEN, - menuDefs: [{ name: 'always', visible: true }], + actionDefs: [{ name: 'always', visible: true }], }); expect(plan.custom.map((a) => a.name)).toEqual(['always']); expect(plan.menuCount).toBe(1); @@ -443,11 +505,77 @@ describe('planRowActionMenu', () => { const plan = planRowActionMenu({ ...base, row: FROZEN, - menuDefs: [{ name: 'compiled_away', visible: '' }], + actionDefs: [{ name: 'compiled_away', visible: '' }], }); expect(plan.custom.map((a) => a.name)).toEqual(['compiled_away']); expect(plan.menuCount).toBe(1); }); + + // --- inline slot allocation (objectui#3762) -------------------------------- + // `base` declares no `maxInlineActions`, so these read the default budget of + // 1 — the same default `RowActionMenuProps` documents. + + it('spends the inline slot on the surviving primary, not the declared first', () => { + const plan = planRowActionMenu({ + ...base, + row: FROZEN, + actionDefs: [ + { name: 'approve', variant: 'primary', visible: IS_APPROVER }, + { name: 'upgrade', variant: 'primary' }, + ], + }); + expect(plan.inline.map((a) => a.name)).toEqual(['upgrade']); + // Nothing folded, so nothing is left for the "⋮" to hold. + expect(plan.custom).toEqual([]); + expect(plan.menuCount).toBe(0); + }); + + it('two surviving primaries still put the second in the menu', () => { + const plan = planRowActionMenu({ + ...base, + row: { ...DRAFT, approver: 'u-me' }, + actionDefs: [ + { name: 'approve', variant: 'primary', visible: IS_APPROVER }, + { name: 'upgrade', variant: 'primary' }, + ], + }); + expect(plan.inline.map((a) => a.name)).toEqual(['approve']); + expect(plan.custom.map((a) => a.name)).toEqual(['upgrade']); + expect(plan.menuCount).toBe(1); + }); + + it('fills EVERY slot from the survivors, and keeps folded primaries above secondaries', () => { + const plan = planRowActionMenu({ + ...base, + row: FROZEN, + maxInlineActions: 2, + actionDefs: [ + { name: 'approve', variant: 'primary', visible: IS_APPROVER }, + { name: 'open', variant: 'primary' }, + { name: 'upgrade', variant: 'primary' }, + { name: 'archive', variant: 'secondary' }, + ], + }); + // Both slots go to survivors — the suppressed leading primary consumes none. + expect(plan.inline.map((a) => a.name)).toEqual(['open', 'upgrade']); + expect(plan.custom.map((a) => a.name)).toEqual(['archive']); + expect(plan.menuCount).toBe(1); + }); + + it('maxInlineActions: 0 keeps every surviving primary in the menu, above the secondaries', () => { + const plan = planRowActionMenu({ + ...base, + row: FROZEN, + maxInlineActions: 0, + actionDefs: [ + { name: 'archive', variant: 'secondary' }, + { name: 'upgrade', variant: 'primary' }, + ], + }); + expect(plan.inline).toEqual([]); + expect(plan.custom.map((a) => a.name)).toEqual(['upgrade', 'archive']); + expect(plan.menuCount).toBe(2); + }); }); /** diff --git a/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx b/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx index 1786f1a5c..fe3b65664 100644 --- a/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx +++ b/packages/plugin-grid/src/components/__tests__/RowActionMenu.test.tsx @@ -14,6 +14,13 @@ * the leftmost ("Open") overflowed the fixed-width cell and was clipped to a * sliver. Only the first `maxInlineActions` primaries now stay inline; the rest * fold into the "⋮" overflow menu. + * + * The budget counts the primaries that SURVIVE their own `visible` for the row + * (objectui#3762). Every fixture in this file is ungated, so declared order and + * surviving order coincide and these cases pin the same behavior as before — + * which is the point of leaving them untouched: the fix must not move the + * clipping regression this suite exists for. The gated cases live in + * `RowActionMenu.emptyGuard.test.tsx`. */ import { describe, it, expect, vi } from 'vitest'; import { render, screen, fireEvent } from '@testing-library/react';