diff --git a/.changeset/action-record-predicate-root-4075.md b/.changeset/action-record-predicate-root-4075.md new file mode 100644 index 0000000000..a84eeef146 --- /dev/null +++ b/.changeset/action-record-predicate-root-4075.md @@ -0,0 +1,60 @@ +--- +"@object-ui/components": patch +"@object-ui/react": patch +--- + +Action-face predicates written against the canonical `record.` root now evaluate + +`action:button`, `action:icon`, `action:menu` and `action:group` gated their +actions on `useCondition(pred, context)`, which evaluates on +`new ExpressionEvaluator({ ...scope, ...context })` — and the context each of +them passed was the row spread flat, or nothing at all. Only the shorthand +spelling resolved: + +| predicate | verdict, before | +|---|---| +| `status == "pending"` | evaluates (`action:button` only) | +| `record.status == "pending"` | throws `record is not defined` | +| `data.status == "pending"` | throws `data is not defined` | + +`record.` is not a mistaken spelling — it is the canonical one. It is what +`ExpressionEvaluator`'s CEL path binds (`bag.record` as the record namespace), +what `evalRowPredicate` binds on the record header, list rows, the row kebab +and conditional formatting (`record.status` / bare `status` / `data.status`), +and what the server enforces with. A `visible` that fails CLOSED turns the throw +into "hidden", so a correctly-authored predicate deleted its own button — +indistinguishable from the gate having said no. On the fail-soft legs the same +throw lands the other way: `disabled` greyed a control out for everyone. + +Live rather than theoretical: every declared action on framework's +`sys_approval_request` gates on `record.viewer.*`, so the whole server-declared +approval decision set was invisible wherever the declared-action bar rendered +until objectui#4077 fixed that bar. These four generic renderers carried the +same binding. + +What changed: + +- all four bind the row the three canonical ways, through one named helper + (`usePredicateRecordContext`, exported from `@object-ui/react` beside + `useCondition`), so the action face and the row surfaces answer an author's + `visible:` the same way; +- `action:icon` reads the row at all. It evaluated against an empty bag, so not + even the bare-field shorthand resolved — and its `data` prop was landing in + the props spread onto the DOM button; +- `action:menu`'s items and `action:group`'s two leaves receive the row from + their host, which they previously never got; +- `action:bar` forwards the row into the overflow menu it builds, not just to + its inline members. An action's predicate had been answering a different + question purely because it spilled past `maxVisible` — which on mobile + defaults to 1, making the verdict a function of the viewport. + +Deliberately unchanged: the evaluation entry and each site's error policy. A +predicate that genuinely faults still fails closed on `action:button` / +`action:menu` `visible` and still fails soft on the other legs, exactly as +before; `toPredicateInput`, `hasDeclaredVisibilityGate` and the empty-predicate +rules keep their pinned semantics. Binding the row is a separate question from +what to do when the predicate faults. + +A surface with no row of its own binds nothing rather than an empty record, so +a host that supplies the row through the ambient predicate scope is not blanked +out; a row passed explicitly still wins over the scope. diff --git a/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx b/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx new file mode 100644 index 0000000000..dd56703071 --- /dev/null +++ b/packages/components/src/renderers/action/__tests__/action-record-predicate-root.test.tsx @@ -0,0 +1,463 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4075 — the row must be bound the THREE canonical ways on the action + * face, not just at the bare root. + * + * `useCondition(pred, ctx)` evaluates on `new ExpressionEvaluator({ ...scope, + * ...ctx })`, so a context bag that is the row spread flat resolves the + * shorthand spelling (`status == 'pending'`) and nothing else. The CANONICAL + * spelling is the `record.` root — it is what `ExpressionEvaluator`'s CEL path + * binds (`bag.record` as the record namespace), what `evalRowPredicate` binds + * on the record header and on list rows (`record.status` / bare `status` / + * `data.status`), and what the server enforces with. Under a root-only bag + * `record.viewer.can_act` does not read as `false`: the legacy evaluator throws + * `record is not defined`, and on a fail-closed `visible` that throw becomes + * "hidden" — a correctly-authored predicate silently deletes its own button. + * + * `DeclaredActionsBar` carried the same root-only binding and suppressed the + * ENTIRE server-declared approval decision set (every `sys_approval_request` + * action gates on `record.viewer.*`); PR #4077 fixed it there by binding the + * row all three ways. These four generic renderers still carried the original + * binding, and two of them — `action:menu`'s item and `action:group`'s two + * leaves — had no record in scope at all, so even the shorthand could not + * resolve. + * + * ## What each block pins, and in which direction + * + * Per site, three roots × two polarities, plus the fault case: + * + * • `record.*` true / false — THE defect. Before the fix the true case is + * the red one on a fail-CLOSED `visible` (the throw hides a holding gate); + * on a fail-SOFT leg it is the FALSE case that is red (the throw returns + * the fail-soft `true`, i.e. shown / disabled / enabled). Both polarities + * are asserted at every site precisely because only one of them can be red + * at a time, and which one depends on that site's error policy. + * • bare `status` true / false — the shorthand is ALSO legal and must keep + * working; these are the anti-regression half. On `action:menu` / + * `action:group` they are red before the fix too (no record at all). + * • `data.*` true / false — the third root `evalRowPredicate` binds, so the + * action face and the row surfaces answer one authoring question one way. + * • a genuinely faulting predicate (`nope.deep == 1`, an unbound root) keeps + * each site's EXISTING error policy. Fail-closed is the policy on + * `action:button` / `action:menu` `visible`; the fault case is not what + * this PR changes, and the fail-soft legs are pinned as fail-soft rather + * than quietly converted (the per-site policy table is #3871's, in + * `action-template-predicate-gate.test.tsx`). + * + * Every "not rendered" assertion carries an ungated companion, so a green can + * never mean "the host itself vanished". + */ + +import { describe, it, expect } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import { PredicateScopeProvider } from '@object-ui/react'; +// Module-scope side-effect imports so the renderers are registered before the +// first render — the light `dom` project deliberately does not load the +// `@object-ui/components` graph. Module scope, not a `beforeAll`, per AGENTS.md +// §测试纪律: the cost lands in the import phase, unbounded by any hook timeout. +import '../action-button'; +import '../action-icon'; +import '../action-bar'; +import '../action-menu'; +import '../action-group'; + +/** + * The row every case evaluates against. `viewer.can_act` is the real shape + * from the approval family (framework#3310 / #3424) — nested, so a predicate + * that only ever reads a flat scalar cannot pass by accident. + */ +const ROW = { id: 'r1', status: 'pending', viewer: { can_act: true } }; + +/** The same question asked through each of the three canonical roots. */ +const HOLDS = { + record: "record.status == 'pending'", + bare: "status == 'pending'", + data: "data.status == 'pending'", +} as const; +const FAILS = { + record: "record.status == 'closed'", + bare: "status == 'closed'", + data: "data.status == 'closed'", +} as const; +/** The nested read the approval actions actually ship. */ +const NESTED = 'record.viewer.can_act == true'; +/** A genuinely faulting predicate — `nope` is bound nowhere. */ +const FAULT = 'nope.deep == 1'; + +const LABEL = 'Act'; +const COMPANION = { name: 'view', label: 'View', type: 'script' }; + +function getRenderer(type: string) { + const R = ComponentRegistry.get(type); + if (!R) throw new Error(`${type} is not registered`); + return R; +} + +const shown = () => expect(screen.queryByText(LABEL)).toBeInTheDocument(); +const hidden = () => { + expect(screen.queryByText(LABEL)).not.toBeInTheDocument(); + expect(screen.getByText('View')).toBeInTheDocument(); +}; + +/** + * Open a real Radix dropdown host. Radix opens on `pointerdown` (a plain + * `.click()` does nothing), and the content mounts in a portal on the next + * tick — so the companion is awaited, which doubles as the "the host itself is + * alive" check every `hidden()` assertion below depends on. + */ +async function openMenu(name: string | RegExp) { + const trigger = screen.getByRole('button', { name }); + fireEvent.pointerDown(trigger, { button: 0, ctrlKey: false, pointerType: 'mouse' }); + await waitFor(() => expect(screen.getByText('View')).toBeInTheDocument()); +} + +// --------------------------------------------------------------------------- +// action:button — the record arrives as the `data` prop (action:bar forwards it) +// --------------------------------------------------------------------------- + +/** Mounted the way `action:bar` mounts a member: action spread onto `schema`. */ +function mountButton(action: any) { + const Renderer = getRenderer('action:button'); + return render( +
+ + +
, + ); +} + +describe('action:button — the row binds three ways (objectui#4075)', () => { + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ['record.* nested', NESTED], + ])('a holding `visible` written as %s renders the button', (_root, visible) => { + mountButton({ name: 'act', label: LABEL, visible }); + shown(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `visible` written as %s hides the button', (_root, visible) => { + mountButton({ name: 'act', label: LABEL, visible }); + hidden(); + }); + + it('`visible` still fails CLOSED on a genuinely faulting predicate', () => { + mountButton({ name: 'act', label: LABEL, visible: FAULT }); + hidden(); + }); + + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ])('a holding `disabled` written as %s greys the button', (_root, disabled) => { + mountButton({ name: 'act', label: LABEL, disabled }); + expect(screen.getByText(LABEL).closest('button')).toBeDisabled(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `disabled` written as %s leaves the button clickable', (_root, disabled) => { + mountButton({ name: 'act', label: LABEL, disabled }); + expect(screen.getByText(LABEL).closest('button')).not.toBeDisabled(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing legacy `enabled` written as %s greys the button', (_root, enabled) => { + mountButton({ name: 'act', label: LABEL, enabled }); + expect(screen.getByText(LABEL).closest('button')).toBeDisabled(); + }); +}); + +// --------------------------------------------------------------------------- +// action:icon — read the `data` prop at all (it used to land in `...rest`) +// --------------------------------------------------------------------------- + +function mountIcon(action: any) { + const Renderer = getRenderer('action:icon'); + return render( +
+ + +
, + ); +} + +const iconShown = () => expect(screen.queryByLabelText(LABEL)).toBeInTheDocument(); +const iconHidden = () => { + expect(screen.queryByLabelText(LABEL)).not.toBeInTheDocument(); + expect(screen.getByLabelText('View')).toBeInTheDocument(); +}; + +describe('action:icon — the row binds three ways (objectui#4075)', () => { + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ['record.* nested', NESTED], + ])('a holding `visible` written as %s renders the icon', (_root, visible) => { + mountIcon({ name: 'act', label: LABEL, visible }); + iconShown(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `visible` written as %s hides the icon', (_root, visible) => { + mountIcon({ name: 'act', label: LABEL, visible }); + iconHidden(); + }); + + it('`visible` keeps its EXISTING fail-soft policy on a faulting predicate', () => { + // Not what this PR decides: `action:icon` has never passed `throwOnError` + // on `visible` (#3871's table). Pinned so the binding fix cannot be read as + // having quietly changed the error policy too. + mountIcon({ name: 'act', label: LABEL, visible: FAULT }); + iconShown(); + }); + + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ])('a holding `disabled` written as %s greys the icon', (_root, disabled) => { + mountIcon({ name: 'act', label: LABEL, disabled }); + expect(screen.getByLabelText(LABEL)).toBeDisabled(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `disabled` written as %s leaves the icon clickable', (_root, disabled) => { + mountIcon({ name: 'act', label: LABEL, disabled }); + expect(screen.getByLabelText(LABEL)).not.toBeDisabled(); + }); +}); + +// --------------------------------------------------------------------------- +// action:menu — its items had NO record in scope; the host must supply the row +// --------------------------------------------------------------------------- + +/** The real `action:menu` host, mounted with the row and opened. */ +async function mountMenu(action: any) { + const Renderer = getRenderer('action:menu'); + const r = render( + , + ); + await openMenu('More'); + return r; +} + +describe('action:menu item — the row binds three ways (objectui#4075)', () => { + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ['record.* nested', NESTED], + ])('a holding `visible` written as %s renders the menu item', async (_root, visible) => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', visible }); + shown(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `visible` written as %s hides the menu item', async (_root, visible) => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', visible }); + hidden(); + }); + + it('a menu item `visible` still fails CLOSED on a faulting predicate', async () => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', visible: FAULT }); + hidden(); + }); + + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ])('a holding `disabled` written as %s greys the menu item', async (_root, disabled) => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', disabled }); + expect(screen.getByText(LABEL).closest('[role="menuitem"]')).toHaveAttribute( + 'data-disabled', + ); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `disabled` written as %s leaves the menu item live', async (_root, disabled) => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', disabled }); + expect(screen.getByText(LABEL).closest('[role="menuitem"]')).not.toHaveAttribute( + 'data-disabled', + ); + }); +}); + +// --------------------------------------------------------------------------- +// action:group — both display modes; its two leaves had no record either +// --------------------------------------------------------------------------- + +async function mountInlineGroup(action: any) { + const Group = getRenderer('action:group'); + return render( + , + ); +} + +async function mountDropdownGroup(action: any) { + const Group = getRenderer('action:group'); + const r = render( + , + ); + await openMenu(/More/); + return r; +} + +describe.each([ + ['inline', mountInlineGroup], + ['dropdown', mountDropdownGroup], +])('action:group %s leaf — the row binds three ways (objectui#4075)', (_mode, mount) => { + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ['record.* nested', NESTED], + ])('a holding `visible` written as %s renders the action', async (_root, visible) => { + await mount({ name: 'act', label: LABEL, type: 'script', visible }); + shown(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `visible` written as %s hides the action', async (_root, visible) => { + await mount({ name: 'act', label: LABEL, type: 'script', visible }); + hidden(); + }); + + it('`visible` keeps its EXISTING fail-soft policy on a faulting predicate', async () => { + // As with `action:icon`: `action:group`'s leaves have never passed + // `throwOnError` (#3871's table). The binding fix does not change it. + await mount({ name: 'act', label: LABEL, type: 'script', visible: FAULT }); + shown(); + }); +}); + +// --------------------------------------------------------------------------- +// No row at the render site — binding must not blank out an ambient `record` +// --------------------------------------------------------------------------- + +describe('a host with no row of its own does not shadow the ambient scope (objectui#4075)', () => { + /** + * A host may supply the row through the predicate SCOPE instead of a prop + * (that is how `action-group-dropdown-visible.test.tsx` drives these leaves). + * `useCondition` merges the local context OVER the scope, so binding an empty + * record for "no row here" would blank that out — "this surface has no row" + * and "this surface's row is empty" are different statements, and only the + * second may shadow the scope. Caught as a regression by the existing suite, + * pinned here so the distinction is stated where the binding rule is. + */ + it('an ambient `record` still resolves when the renderer is given no `data`', () => { + const Renderer = getRenderer('action:button'); + render( + + + + , + ); + shown(); + }); + + it('a `data` prop still WINS over an ambient `record` when both are present', () => { + const Renderer = getRenderer('action:button'); + render( + + + + , + ); + shown(); + }); +}); + +// --------------------------------------------------------------------------- +// action:bar overflow — the bar forwards `data` inline but not into the +// overflow menu it builds, so an overflowed action lost the row entirely +// --------------------------------------------------------------------------- + +describe('action:bar overflow menu — the row reaches an overflowed action (objectui#4075)', () => { + async function mountBarOverflow(action: any) { + const Bar = getRenderer('action:bar'); + const r = render( + , + ); + await openMenu('More actions'); + return r; + } + + it.each([ + ['record.*', HOLDS.record], + ['bare', HOLDS.bare], + ['data.*', HOLDS.data], + ['record.* nested', NESTED], + ])('a holding `visible` written as %s renders the overflowed action', async (_root, visible) => { + await mountBarOverflow({ name: 'act', label: LABEL, type: 'script', visible }); + shown(); + }); + + it.each([ + ['record.*', FAILS.record], + ['bare', FAILS.bare], + ['data.*', FAILS.data], + ])('a failing `visible` written as %s hides the overflowed action', async (_root, visible) => { + await mountBarOverflow({ name: 'act', label: LABEL, type: 'script', visible }); + hidden(); + }); +}); diff --git a/packages/components/src/renderers/action/action-bar.tsx b/packages/components/src/renderers/action/action-bar.tsx index 3b438be019..f13c30d297 100644 --- a/packages/components/src/renderers/action/action-bar.tsx +++ b/packages/components/src/renderers/action/action-bar.tsx @@ -238,6 +238,12 @@ const ActionBarRenderer = forwardRef ) : null; diff --git a/packages/components/src/renderers/action/action-button.tsx b/packages/components/src/renderers/action/action-button.tsx index 5e910641ee..e4b3bad0f3 100644 --- a/packages/components/src/renderers/action/action-button.tsx +++ b/packages/components/src/renderers/action/action-button.tsx @@ -21,7 +21,7 @@ import React, { forwardRef, useCallback, useEffect, useRef, useState } from 'rea import { ComponentRegistry } from '@object-ui/core'; import type { ActionSchema } from '@object-ui/types'; import { useAction } from '@object-ui/react'; -import { useCondition, toPredicateInput } from '@object-ui/react'; +import { useCondition, toPredicateInput, usePredicateRecordContext } from '@object-ui/react'; import { Button } from '../../ui'; import { cn } from '../../lib/utils'; import { Loader2 } from 'lucide-react'; @@ -49,8 +49,12 @@ const ActionButtonRenderer = forwardRef( const { execute } = useAction(); const [loading, setLoading] = useState(false); - // Record data may be passed from SchemaRenderer (e.g. DetailView passes record data) - const recordData = data != null && typeof data === 'object' ? data as Record : {}; + // Record data may be passed from SchemaRenderer (e.g. DetailView passes + // record data), bound the three canonical ways — `record.status`, bare + // `status`, `data.status` (objectui#4075). This used to be the row spread + // flat, so the CANONICAL `record.` root threw `record is not defined` and + // the fail-closed `visible` below turned that into "hidden". + const recordData = usePredicateRecordContext(data); // Evaluate visibility and disabled conditions with record data context. // `visible` fails CLOSED on a throwing predicate (mirrors ActionEngine's diff --git a/packages/components/src/renderers/action/action-group.tsx b/packages/components/src/renderers/action/action-group.tsx index ce58f6f4e2..544009b62a 100644 --- a/packages/components/src/renderers/action/action-group.tsx +++ b/packages/components/src/renderers/action/action-group.tsx @@ -21,7 +21,7 @@ import { ComponentRegistry } from '@object-ui/core'; import type { ActionSchema, ActionGroup, ActionLocation } from '@object-ui/types'; import { actionRendersAt } from '@object-ui/types'; import { useAction } from '@object-ui/react'; -import { useCondition, toPredicateInput } from '@object-ui/react'; +import { useCondition, toPredicateInput, usePredicateRecordContext } from '@object-ui/react'; import { Button } from '../../ui'; import { DropdownMenu, @@ -68,15 +68,21 @@ const InlineActionButton: React.FC<{ variant?: string; size?: string; onExecute: (action: ActionSchema) => Promise; -}> = ({ action, variant, size, onExecute }) => { + /** The row the group is mounted over — see `DropdownActionItem` (objectui#4075). */ + record?: unknown; +}> = ({ action, variant, size, onExecute, record }) => { const [loading, setLoading] = useState(false); - const isVisible = useCondition(toPredicateInput(action.visible)); + // The row bound the three canonical ways — `record.status`, bare `status`, + // `data.status`. This leaf used to evaluate against nothing at all, so a + // row-scoped predicate faulted on its root (objectui#4075). + const recordData = usePredicateRecordContext(record); + const isVisible = useCondition(toPredicateInput(action.visible), recordData); // Spec field is `disabled` (boolean | CEL — disabled when TRUE). #1885 wired // it in action-button only; this leaf kept reading the legacy non-spec // `enabled`, so a spec-authored `disabled` guard did nothing here. `disabled` // is now the primary control; `enabled` stays as a deprecated fallback. - const isDisabledPred = useCondition(toPredicateInput((action as any).disabled)); - const isEnabled = useCondition(toPredicateInput(action.enabled)); + const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData); + const isEnabled = useCondition(toPredicateInput(action.enabled), recordData); const Icon = resolveIcon(action.icon); const btnVariant = (action.variant as string) === 'primary' ? 'default' : (action.variant || variant || 'outline'); @@ -144,12 +150,22 @@ export const DropdownActionItem: React.FC<{ action: ActionSchema; index: number; onSelect: (action: ActionSchema) => void | Promise; -}> = ({ action, index, onSelect }) => { - const isVisible = useCondition(toPredicateInput(action.visible)); + /** + * The row this group is mounted over, forwarded by the host. Optional: an + * object-level group genuinely has no row, and a predicate over an empty + * record is still evaluable (objectui#4075). + */ + record?: unknown; +}> = ({ action, index, onSelect, record }) => { + // Same three-way binding as `InlineActionButton` — one action cannot resolve + // its predicate in one display mode and fault in the other (objectui#4075, + // the binding half of the objectui#3812 / #3842 "one leaf, one answer" rule). + const recordData = usePredicateRecordContext(record); + const isVisible = useCondition(toPredicateInput(action.visible), recordData); // Spec `disabled` primary, legacy non-spec `enabled` fallback (see // InlineActionButton above — #1885 follow-through). - const isDisabledPred = useCondition(toPredicateInput((action as any).disabled)); - const isEnabled = useCondition(toPredicateInput(action.enabled)); + const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData); + const isEnabled = useCondition(toPredicateInput(action.enabled), recordData); // Same declared-gate rule as `InlineActionButton` above — one action cannot be // hidden in one display mode and shown in the other (objectui#3812). if (hasDeclaredVisibilityGate(action.visible) && !isVisible) return null; @@ -197,13 +213,21 @@ const ActionGroupRenderer = forwardRef ))} @@ -306,6 +331,7 @@ const ActionGroupRenderer = forwardRef ))} diff --git a/packages/components/src/renderers/action/action-icon.tsx b/packages/components/src/renderers/action/action-icon.tsx index 6fd28e0a86..9a1b97dccf 100644 --- a/packages/components/src/renderers/action/action-icon.tsx +++ b/packages/components/src/renderers/action/action-icon.tsx @@ -16,7 +16,7 @@ import React, { forwardRef, useCallback, useState } from 'react'; import { ComponentRegistry } from '@object-ui/core'; import type { ActionSchema } from '@object-ui/types'; import { useAction } from '@object-ui/react'; -import { useCondition, toPredicateInput } from '@object-ui/react'; +import { useCondition, toPredicateInput, usePredicateRecordContext } from '@object-ui/react'; import { Button } from '../../ui'; import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from '../../ui'; import { cn } from '../../lib/utils'; @@ -37,18 +37,28 @@ const ActionIconRenderer = forwardRef( 'data-obj-id': dataObjId, 'data-obj-type': dataObjType, style, + // The row `action:bar` forwards to its members. Destructured for TWO + // reasons: it is the predicate context below (objectui#4075 — this + // renderer evaluated `visible` / `disabled` / `enabled` against NOTHING, + // so not even the bare-field shorthand resolved, let alone the canonical + // `record.` root), and it must not reach `...rest`, which is spread onto + // the DOM button. + data, ...rest } = props; const { execute } = useAction(); const [loading, setLoading] = useState(false); - const isVisible = useCondition(toPredicateInput(schema.visible)); + // The row bound the three canonical ways — see `usePredicateRecordContext`. + const recordData = usePredicateRecordContext(data); + + const isVisible = useCondition(toPredicateInput(schema.visible), recordData); // Spec `disabled` (boolean | CEL — disabled when TRUE) primary, legacy // non-spec `enabled` fallback (#1885 follow-through — only action-button // was wired; this renderer ignored a spec-authored `disabled`). - const isDisabledPred = useCondition(toPredicateInput((schema as any).disabled)); - const isEnabled = useCondition(toPredicateInput(schema.enabled)); + const isDisabledPred = useCondition(toPredicateInput((schema as any).disabled), recordData); + const isEnabled = useCondition(toPredicateInput(schema.enabled), recordData); const Icon = resolveIcon(schema.icon); const variant = schema.variant === 'primary' ? 'default' : (schema.variant || 'ghost'); diff --git a/packages/components/src/renderers/action/action-menu.tsx b/packages/components/src/renderers/action/action-menu.tsx index 87ec84569b..853150ba40 100644 --- a/packages/components/src/renderers/action/action-menu.tsx +++ b/packages/components/src/renderers/action/action-menu.tsx @@ -17,7 +17,7 @@ import React, { forwardRef, useCallback, useMemo, useState } from 'react'; import { ComponentRegistry } from '@object-ui/core'; import type { ActionSchema } from '@object-ui/types'; import { useAction } from '@object-ui/react'; -import { useCondition, toPredicateInput } from '@object-ui/react'; +import { useCondition, toPredicateInput, usePredicateRecordContext } from '@object-ui/react'; import { useObjectTranslation } from '@object-ui/i18n'; import { Button } from '../../ui'; import { @@ -68,18 +68,31 @@ export interface ActionMenuSchema { export const ActionMenuItem: React.FC<{ action: ActionSchema; onExecute: (action: ActionSchema) => Promise; -}> = ({ action, onExecute }) => { + /** + * The row this menu is mounted over, forwarded by the host. Optional: an + * object-level menu genuinely has no row, and a predicate over an empty + * record is still evaluable (objectui#4075 — the failure being fixed is a + * predicate that FAULTS on an unbound root, not one that reads a missing + * field). + */ + record?: unknown; +}> = ({ action, onExecute, record }) => { + // The row bound the three canonical ways — `record.status`, bare `status`, + // `data.status`. This item used to pass `undefined`, i.e. no record at all, + // so every row-scoped predicate an author wrote here faulted on its root + // (objectui#4075). See `usePredicateRecordContext`. + const recordData = usePredicateRecordContext(record); // Fails CLOSED on a throwing predicate — mirrors ActionEngine's // getActionsForLocation contract (see action-button.tsx for rationale). - const isVisible = useCondition(toPredicateInput(action.visible), undefined, { + const isVisible = useCondition(toPredicateInput(action.visible), recordData, { throwOnError: true, label: `action "${action.name ?? action.label ?? 'action:menu item'}" (visible)`, }); // Spec `disabled` (boolean | CEL — disabled when TRUE) primary, legacy // non-spec `enabled` fallback (#1885 follow-through — only action-button // was wired; this renderer ignored a spec-authored `disabled`). - const isDisabledPred = useCondition(toPredicateInput((action as any).disabled)); - const isEnabled = useCondition(toPredicateInput(action.enabled)); + const isDisabledPred = useCondition(toPredicateInput((action as any).disabled), recordData); + const isEnabled = useCondition(toPredicateInput(action.enabled), recordData); const iconElement = useMemo(() => { const Icon = resolveIcon(action.icon); @@ -130,6 +143,11 @@ const ActionMenuRenderer = forwardRef {showSeparator && } - + ); })} diff --git a/packages/react/src/hooks/useExpression.ts b/packages/react/src/hooks/useExpression.ts index 6c9f758f1e..2d7346e1f3 100644 --- a/packages/react/src/hooks/useExpression.ts +++ b/packages/react/src/hooks/useExpression.ts @@ -82,6 +82,71 @@ export function usePredicateScope(): Record { */ export { toPredicateInput } from '@object-ui/core'; +/** + * Build the predicate context for a **row record** — the bag to hand + * {@link useCondition} when the thing being gated is scoped to one record. + * + * The row is bound the THREE ways the platform's row surfaces bind it: + * `record.status` (spec/canonical), bare `status` (row-action shorthand), and + * `data.status` (legacy). This is not three dialects; it is one rule, and it is + * `evalRowPredicate`'s rule (`core/evaluator/listConditional.ts` — the record + * header, list rows, the row kebab and conditional formatting all evaluate + * through it), restated here for the `useCondition` tier so both tiers answer + * an author's `visible:` the same way. + * + * ## Why a helper and not "just spread the row" (objectui#4075) + * + * `useCondition` evaluates on `new ExpressionEvaluator({ ...scope, ...context })`, + * so a caller that passes the row spread flat resolves the shorthand spelling + * and NOTHING else. The canonical spelling is the `record.` root — it is what + * `ExpressionEvaluator`'s CEL path binds (`bag.record` as the record + * namespace), what `evalRowPredicate` binds, and what the server enforces + * with. Under a root-only bag `record.viewer.can_act` does not read as `false`: + * the evaluator throws `record is not defined`, and a fail-closed `visible` + * turns that throw into "hidden". A correctly-authored predicate then deletes + * its own button, indistinguishably from the gate having said no. + * + * That was live, not theoretical: every declared action on framework's + * `sys_approval_request` gates on `record.viewer.*` (framework#3310 / #3424), + * so the whole server-declared approval decision set was invisible wherever + * `DeclaredActionsBar` rendered until objectui#4077 bound the row this way — + * and the four generic action renderers carried the same root-only binding + * until objectui#4075. + * + * `record` and `data` are written AFTER the spread deliberately: a row that + * happens to carry a field literally named `record` or `data` must not shadow + * the root every predicate is written against. Same precedence as + * `evalRowPredicate`. + * + * This is the BINDING rule only. It deliberately does not touch the evaluation + * entry — `useCondition` / `toPredicateInput` / `hasDeclaredVisibilityGate` + * keep their pinned semantics (objectui#3492 / #3842 / #3850 / #3871), and each + * caller keeps its own error policy (fail-closed `visible` vs the fail-soft + * legs). Binding the row is a different question from what to do when the + * predicate faults. + * + * ## No row → bind NOTHING, do not bind an empty one + * + * `null` / `undefined` / a non-object returns an EMPTY bag, not + * `{ record: {}, data: {} }`. The difference is load-bearing: `useCondition` + * merges this context OVER the ambient predicate scope, so binding an empty + * record would blank out a `record` that a host had put in the scope itself — + * which is exactly how `action:group`'s dropdown leaf is driven in + * `action-group-dropdown-visible.test.tsx`, and it is a legitimate way for a + * host to supply the row. "This surface has no row of its own" must stay + * distinct from "this surface's row is empty"; only the latter is entitled to + * shadow the scope. + * + * @param record The row, or nothing. + */ +export function usePredicateRecordContext(record: unknown): Record { + return useMemo(() => { + if (record == null || typeof record !== 'object' || Array.isArray(record)) return {}; + const row = record as Record; + return { ...row, record: row, data: row }; + }, [record]); +} + /** * Hook for evaluating expressions with dynamic context *