diff --git a/.changeset/record-alert-row-binding-4807.md b/.changeset/record-alert-row-binding-4807.md new file mode 100644 index 000000000..ecca389da --- /dev/null +++ b/.changeset/record-alert-row-binding-4807.md @@ -0,0 +1,43 @@ +--- +'@object-ui/plugin-detail': minor +--- + +`record:alert` binds the row through `usePredicateRecordContext`, so an +author-declared `properties.visible` is actually consulted. + +`renderers/record-alert.tsx` was the last predicate face in the repo still +handing `useCondition` a root-only `{ record }` bag. Every other row-scoped +predicate — the four generic action renderers (objectui#4075) and app-shell's +`DeclaredActionsBar` (objectui#4077) — binds the row through the shared +`usePredicateRecordContext(record)` helper, which resolves the three spellings +objectui#5330 ruled on: canonical `record.status`, the deprecated row-action +shorthand `status`, and deprecated legacy `data.status`. + +Under the root-only bag only the canonical spelling worked, and the two others +failed in **opposite** directions — both of them silently, because this call +site is fail-soft: + +- **row-action shorthand** (`status == 'x'`) resolved nothing, so the evaluator + threw. The legacy `${…}` path answers a throw with its own source text, a + non-empty and therefore truthy string, so the verdict was **SHOWN on every + row**. A banner the author had gated was permanently on screen. +- **legacy `data.*`** (`data.status == 'x'`) did not throw at all. App-shell's + ambient predicate scope (`providers/ExpressionProvider.tsx`) carries + `data: {}`, so the predicate read that object instead of the row, compared + `undefined`, and the verdict was a constant false — **never shown**. + +**Behaviour change, stated plainly:** a shipped `record:alert` whose `visible` +was written in either deprecated spelling was inert and is now live. A banner +that was permanently visible may begin to hide, and one that never appeared may +begin to show — that is the point of the fix, but it is a verdict change rather +than a no-op. Canonical `record.*` predicates are unaffected in verdict: they +resolved before and resolve now, pinned on both polarities. An in-tree census +found no `record:alert` `visible` predicate outside this package's own tests. + +A node-level `visibleWhen` is a separate gate one tier up in `SchemaRenderer`, +with its own deliberate bindings (`data` is the data-source adapter there, not +the row). This change does not touch it; the two still compose as AND. + +The renderer's header comment described the shared-scope behaviour it did not +have. It now describes what the file does, including the fail-soft policy and +the two-gate composition. diff --git a/packages/plugin-detail/src/renderers/__tests__/record-alert.rowBinding.test.tsx b/packages/plugin-detail/src/renderers/__tests__/record-alert.rowBinding.test.tsx new file mode 100644 index 000000000..e9f65d997 --- /dev/null +++ b/packages/plugin-detail/src/renderers/__tests__/record-alert.rowBinding.test.tsx @@ -0,0 +1,228 @@ +/** + * 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. + * + * ══════════════════════════════════════════════════════════════════════════ + * `record:alert` binds the row the THREE canonical ways (objectui#4807) + * ══════════════════════════════════════════════════════════════════════════ + * + * `record:alert` was the last predicate face in the repo still handing + * `useCondition` a ROOT-ONLY bag (`{ record }`) instead of the shared + * `usePredicateRecordContext(record)` that objectui#4075 / #4077 put under the + * four generic action renderers and app-shell's `DeclaredActionsBar`. The + * consequence was user-visible, and it is what this file measures: + * + * • row-action shorthand (`status == 'x'`) resolved NOTHING, so the + * evaluator threw `status is not defined`. This call site is FAIL-SOFT — + * the legacy `${…}` path answers a throw with its own source text, which is + * a non-empty (truthy) string — so an author-declared gate came out as + * SHOWN on every row. A banner the author gated was permanently on screen, + * with nothing but a console line to say so. + * • legacy `data.*` did not throw at all, which is worse than it sounds: the + * ambient scope app-shell mounts (`providers/ExpressionProvider.tsx`) puts + * `data: {}` in the bag, so `data.status` resolved to `undefined` against + * the wrong object and the comparison was a constant `false` — the banner + * was permanently OFF screen instead. Same defect, opposite polarity. + * + * objectui#5330 (maintainer, 2026-08-20) ruled **B**: `record.*` is the canon; + * the row-action shorthand and legacy `data.*` are DEPRECATED but kept behind a + * survey-sized window. So all three must resolve, and the pins below name + * `record.*` as the one an author should write today. + * + * ── Why groups B and C mount different shapes ────────────────────────────── + * + * There are TWO gates on the way to this banner and they live at different + * tiers, each with its own binding: + * + * 1. `SchemaRenderer`'s node chain. It hoists `properties.visible` onto the + * node and evaluates it on an evaluator that binds `record` (root-only) + * and, deliberately, `data` = the DATA-SOURCE ADAPTER — see the docblock + * on `SchemaRenderer.tsx`'s `evaluatedSchema`, which states that binding + * and why the row must not overwrite it. + * 2. `record-alert.tsx`'s own `useCondition`, one layer below. THIS is the + * tier objectui#4075's rule governs and the tier this card fixes. + * + * The two compose as AND (pinned in `record-alert.visibleWhen.evidence.test.tsx` + * group 4). Group B therefore holds gate 1 open with a declared + * `visibleWhen: cel('true')` — the node chain tests `visibleWhen` FIRST and + * returns without ever consulting `visible` — so that every verdict it records + * is gate 2's, i.e. this renderer's. Group C then drops the isolation and mounts + * the plain authored shape, so the end-to-end user-visible outcome is pinned too. + * + * Group C covers the canon and the shorthand only. The legacy `data.*` spelling + * is NOT asserted end-to-end, and that is deliberate: through the plain shape + * gate 1 decides it on the adapter binding above, one tier up and out of this + * card's reach. Fixing this renderer does not (and must not) move that. See the + * out-of-scope finding filed alongside this change. + * + * ── Non-vacuity ──────────────────────────────────────────────────────────── + * + * This renderer has four `return null` paths (dismissed, empty record, the + * props gate, and the node gate above it), so "nothing rendered" is not by + * itself "the gate said no". Group A is the control: it proves the harness + * really mounts and paints the banner, and that this exact channel can hide it. + * Every verdict below is asserted on what a USER sees — is the banner's text in + * the document — never on computed styles or a predicate's return value. + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup, type RenderResult } from '@testing-library/react'; +import { RecordContextProvider, SchemaRenderer, PredicateScopeProvider } from '@object-ui/react'; +import '../../index'; + +/** + * The ambient scope app-shell actually mounts on a record page, reproduced from + * `packages/app-shell/src/providers/ExpressionProvider.tsx` (`data` defaults to + * `{}` and is `{}` at both of its call sites). The absence of `record` here — and + * the PRESENCE of an unrelated `data` — are properties of production, not + * omissions in the fixture: they are precisely what made the two deprecated + * spellings fail in opposite directions. + */ +const APP_SCOPE = { + current_user: { id: 'u1', name: 'Ada', email_verified: true }, + user: { id: 'u1', email_verified: true }, + ctx: { user: { id: 'u1' } }, + os: { user: { id: 'u1' } }, + app: {}, + data: {}, + features: { multiOrgEnabled: true }, +}; + +const IN_REVIEW = { id: 'r1', status: 'in_review' }; +const DONE = { id: 'r1', status: 'done' }; +const TITLE = 'Awaiting review'; + +const cel = (source: string) => ({ dialect: 'cel', source }); + +/** Holds SchemaRenderer's node gate open so the verdict recorded is this renderer's. */ +const NODE_GATE_OPEN = { visibleWhen: cel('true') }; + +/** + * The three spellings objectui#5330 ruled on, written the way an author writes + * them: a BARE expression string, which is the spec spelling and which + * `SchemaRenderer`'s per-value `properties` evaluation passes through untouched + * (only `${…}` values are interpolated there). + */ +const CANON = "record.status == 'in_review'"; +const SHORTHAND = "status == 'in_review'"; +const LEGACY = "data.status == 'in_review'"; + +function alertNode(visible?: string, extra: Record = {}) { + return { + type: 'record:alert', + properties: { title: TITLE, ...(visible === undefined ? {} : { visible }) }, + ...extra, + }; +} + +function mount(node: unknown, record: Record): RenderResult { + return render( + + + + + , + ); +} + +/** What the user sees: is the banner's own text in the document? */ +const bannerInDocument = (r: RenderResult) => r.queryByText(TITLE) !== null; + +/** Mount the same node against both rows and report the pair of verdicts. */ +function verdicts(node: unknown): { onMatchingRow: boolean; onOtherRow: boolean } { + const onMatchingRow = bannerInDocument(mount(node, IN_REVIEW)); + cleanup(); + const onOtherRow = bannerInDocument(mount(node, DONE)); + cleanup(); + return { onMatchingRow, onOtherRow }; +} + +afterEach(() => cleanup()); + +describe('#4807 group A — controls: the harness paints the banner, and this channel can hide it', () => { + it('with no predicate declared, the banner is on screen for every row', () => { + // If this goes red, nothing below means anything: a `false` verdict there + // would be "never rendered", not "the gate said no". + expect(bannerInDocument(mount(alertNode(), IN_REVIEW))).toBe(true); + cleanup(); + expect(bannerInDocument(mount(alertNode(), DONE))).toBe(true); + }); + + it('a constant `properties.visible` decides it in both directions', () => { + // The paired control for group B: same key, same tier, same mount — so a + // hidden verdict below is this gate's answer and not an inert surface. + expect(bannerInDocument(mount(alertNode('false', NODE_GATE_OPEN), DONE))).toBe(false); + cleanup(); + expect(bannerInDocument(mount(alertNode('true', NODE_GATE_OPEN), DONE))).toBe(true); + }); +}); + +describe('#4807 group B — all THREE bindings resolve on this renderer own gate', () => { + // The two rows differ ONLY in `status`, so a pair of opposite verdicts IS the + // binding: the predicate reached the row. A pair of EQUAL verdicts means the + // author's gate was never consulted, whichever way it landed. + + it('canon `record.*` — the spelling objectui#5330 ruled canonical', () => { + expect(verdicts(alertNode(CANON, NODE_GATE_OPEN))).toEqual({ + onMatchingRow: true, + onOtherRow: false, + }); + }); + + it('row-action shorthand `status` — deprecated by objectui#5330, still resolves', () => { + // Before objectui#4807 both mounts were `true`: `status` was unbound, the + // evaluator threw, and this fail-soft call site turned the throw into SHOWN. + expect(verdicts(alertNode(SHORTHAND, NODE_GATE_OPEN))).toEqual({ + onMatchingRow: true, + onOtherRow: false, + }); + }); + + it('legacy `data.*` — deprecated by objectui#5330, still resolves', () => { + // Before objectui#4807 both mounts were `false`, not `true`: the ambient + // `data: {}` from app-shell answered instead of the row, so the comparison + // was constantly false and the banner never appeared at all. + expect(verdicts(alertNode(LEGACY, NODE_GATE_OPEN))).toEqual({ + onMatchingRow: true, + onOtherRow: false, + }); + }); + + it('the row wins over an ambient `record` / `data` the host also supplied', () => { + // `usePredicateRecordContext` writes `record` and `data` AFTER the spread + // for this reason; APP_SCOPE carries a `data` of its own, and a host may + // carry a `record` too. Pinned on the user-visible verdict so the + // precedence cannot regress silently. + const hostScope = { ...APP_SCOPE, record: DONE, data: DONE }; + const withHostScope = (record: Record) => + render( + + + + + , + ); + expect(bannerInDocument(withHostScope(IN_REVIEW))).toBe(true); + cleanup(); + expect(bannerInDocument(withHostScope(DONE))).toBe(false); + }); +}); + +describe('#4807 group C — end to end, on the plain authored node', () => { + // No `visibleWhen` isolation here: this is the shape an author writes, and + // these are the verdicts a user gets. + + it('canon `record.*` gates the banner on the row', () => { + expect(verdicts(alertNode(CANON))).toEqual({ onMatchingRow: true, onOtherRow: false }); + }); + + it('row-action shorthand gates the banner on the row', () => { + // THE defect of objectui#4807 as a user met it: this pair used to be + // `{ true, true }` — an author-declared gate that never once hid the banner. + expect(verdicts(alertNode(SHORTHAND))).toEqual({ onMatchingRow: true, onOtherRow: false }); + }); +}); diff --git a/packages/plugin-detail/src/renderers/record-alert.tsx b/packages/plugin-detail/src/renderers/record-alert.tsx index f3dfe634c..950f6969f 100644 --- a/packages/plugin-detail/src/renderers/record-alert.tsx +++ b/packages/plugin-detail/src/renderers/record-alert.tsx @@ -31,12 +31,35 @@ * * Visibility model * ---------------- - * Reuses `useCondition` + `toPredicateInput` (same pipeline as every - * `` / ``), so the predicate evaluates against - * the same scope: `record`, `user`, `objectName`, `features`, plus - * `ctx.*` namespace mirror. Missing predicate → always visible. + * `properties.visible` is normalized by `toPredicateInput` and evaluated by + * `useCondition` against `usePredicateRecordContext(record)` — the repo's one + * row-binding rule (objectui#4075 / #4077), shared with `` / + * `` / `` / `` and app-shell's + * `DeclaredActionsBar`, so this banner cannot disagree with the buttons it + * pairs with. The row therefore resolves the three ways objectui#5330 ruled + * on (maintainer, 2026-08-20): `record.status` — the CANON, what an author + * should write — plus the deprecated-but-kept row-action shorthand `status` + * and legacy `data.status`. + * + * Merged UNDER the row is whatever the host put in the ambient predicate + * scope (`PredicateScopeProvider`; app-shell's `ExpressionProvider` supplies + * `current_user` / `user` / `ctx.user` / `os.user` / `app` / `data` / + * `features`). The row wins, so a host-supplied `record` / `data` cannot + * shadow it. `objectName` is NOT in the predicate scope — it is read from + * `useRecordContext()` for the metadata lookup and the dismiss key only. + * + * Missing predicate → always visible. A predicate that cannot be evaluated → + * also visible: this call site is FAIL-SOFT (it does not pass + * `throwOnError`), which is why the unbound spellings above were a + * user-visible defect rather than a console line — objectui#4807. * Empty `record` (loading) → hidden (no flash of stale alert). * + * A node-level `visibleWhen` is a SEPARATE gate one tier up, evaluated by + * `SchemaRenderer` on its own bindings (notably `data` = the data-source + * ADAPTER, not the row). The two compose as AND. Both facts are pinned in + * `__tests__/record-alert.visibleWhen.evidence.test.tsx` (group 4) and + * `__tests__/record-alert.rowBinding.test.tsx`. + * * CTA wiring * ---------- * The optional `action.actionName` is resolved from the object's @@ -51,6 +74,7 @@ import { useMetadataItem, useCondition, toPredicateInput, + usePredicateRecordContext, useActionEngine, } from '@object-ui/react'; import { Alert, AlertTitle, AlertDescription, Button, cn, LazyIcon } from '@object-ui/components'; @@ -161,12 +185,16 @@ export const RecordAlertRenderer: React.FC = ({ schema = {}, c const styles = SEVERITY_STYLES[severity]; const iconName = props.icon || styles.icon; - // Always-call hooks (Rules of Hooks). Evaluate the visibility predicate - // against the record / user / ctx scope using the same canonical helper - // every action button uses, so this banner can't disagree with the - // Salesforce Lightning-style buttons it commonly pairs with. + // Always-call hooks (Rules of Hooks). Bind the row through the shared + // helper — NOT a local `{ record }` bag (objectui#4807). A root-only bag + // resolves the canonical `record.*` spelling and nothing else, so the two + // spellings objectui#5330 kept never reached the row: the shorthand threw + // and this fail-soft site answered SHOWN, while `data.*` silently read the + // host's ambient `data` and answered a constant false. Either way the + // author's gate was never consulted. See `usePredicateRecordContext`. + const predicateRecord = usePredicateRecordContext(record); const predicateInput = toPredicateInput(props.visible); - const passesPredicate = useCondition(predicateInput, { record }); + const passesPredicate = useCondition(predicateInput, predicateRecord); // Dismissed-state persistence. Keyed by `::` // so an admin viewing a different record sees the alert fresh, and so