diff --git a/.changeset/row-predicate-phase2-record-only-5741.md b/.changeset/row-predicate-phase2-record-only-5741.md new file mode 100644 index 0000000000..2ce7fefa07 --- /dev/null +++ b/.changeset/row-predicate-phase2-record-only-5741.md @@ -0,0 +1,60 @@ +--- +'@object-ui/core': minor +'@object-ui/react': minor +--- + +row predicates on runtime record surfaces resolve `record.*` only; the bare-field and `data.*` spellings are no longer bound + +Phase 2 of the row-predicate canon (objectui#5330, ruled 2026-08-20, option B; +Phase 2 ruled 2026-09-02 and amended 2026-09-05 on objectui#5741). Until now a +row predicate — `visible` / `disabled` / `enabled` on an action renderer, a row +action, a `record:alert`, a `page:header` action, a conditional-formatting +`condition` — bound the row three ways: canonical `record.status`, bare +`status`, and `data.status`. The two non-canonical spellings are retired on +every runtime record surface, in both evaluation tiers (`evalRowPredicate` / +`partitionRowsByPredicate` in `@object-ui/core`; `usePredicateRecordContext` + +`useCondition` in `@object-ui/react`) and for both dialects: a legacy +`${data.x}` / `${x}` string on a row surface retires with the CEL spellings. + +**What a retired spelling does now: it faults, exactly as it already did on the +server** (`buildScope({ record })` mounts exactly `['record']`, so `status` and +`data` are unknown variables there), and each surface applies its EXISTING +fault policy — no runtime detector, no "treat as absent" special case, no +uniform override: + +- `evalRowPredicate` / `partitionRowsByPredicate` (row kebab, selection bar, + `page:header` actions, conditional formatting): the caller's `fallback` — + hidden / every row excluded / no style — reported once by the existing fault + warning, which names the unknown variable (`Unknown variable: status`) and, + on the fast route, carries the `record.` hint. +- `useCondition` legs that opt into `throwOnError` (`action:button` and + `action:menu` `visible`, `DeclaredActionsBar` `visible`): fail-closed — + hidden on every row, reported once as `was hidden/disabled: its predicate + threw — status is not defined`. +- the non-throwing `useCondition` legs (`action:icon` / `action:group` + `visible`, every `disabled` / `enabled`, `record:alert`): fail-soft — shown / + greyed / enabled on every row, with the evaluator's own console line. +- a host scope that carries its OWN `data` (app-shell's ambient `data: {}`) is + left standing: `data.*` on a record surface then reads the host's object — a + constant, silent `false` — which is what "no longer bound to the row" means. + +The Phase-1 deprecation warning is removed with the bindings: +`warnNonCanonicalRowSpelling` and `resetRowPredicateCanonWarnings` are no +longer exported from `@object-ui/core`. `detectNonCanonicalRowSpelling`, +`ROW_PREDICATE_CANONICAL_ROOT` and the `NonCanonicalRowSpelling` type stay +exported — the offline instrument for sweeping authored metadata. + +The layer rule is unchanged: `data` remains the canonical root on +metadata-editing surfaces (ADR-0089 D3, `CANONICAL_ROOT_BY_LAYER`), and +app-shell's metadata-admin `SchemaForm` / `predicate.ts` keep binding +`{ data: row }` through their own evaluator. + +No stored-metadata survey, export or migration rewrite was run (the maintainer +ruled the stored population out of scope, 「不考虑存量」); the Phase-1 warning +period was the notice. + +Release note: Phase 1 (PR #5737 — the canon statement plus the warning) shipped +in `@object-ui/core@17.6.0` (npm, 2026-08-24) although its changeset +`.changeset/row-predicate-record-canon-5330.md` is still pending on `main`, so +the next CHANGELOG section lists Phase 1 and this Phase 2 together: the warning +it describes was live from 17.6.0 and is gone from this release on. diff --git a/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx b/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx index b01c91c404..bb75bf7b67 100644 --- a/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx +++ b/packages/app-shell/src/views/__tests__/DeclaredActionsBar.test.tsx @@ -511,7 +511,7 @@ describe('DeclaredActionsBar — declared `visible` on a server-declared action }); it('an expression-valued `visible` keeps its verdict — false hides, true shows', () => { - const gated = { ...APPROVE, visible: 'status == "pending"' }; + const gated = { ...APPROVE, visible: 'record.status == "pending"' }; const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'approved' }); expect(screen.queryByTestId('declared-action-approval_approve')).toBeNull(); expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); @@ -520,6 +520,33 @@ describe('DeclaredActionsBar — declared `visible` on a server-declared action expect(screen.getByTestId('declared-action-approval_approve')).toBeInTheDocument(); }); + it('a bare-field `visible` no longer discriminates (objectui#5741) — hidden on BOTH rows, reported once', () => { + // Phase 2 of the objectui#5330 canon: the row is bound as `record.*` only, + // so `status` is an unknown variable. This leg opts into `throwOnError`, so + // its EXISTING policy is fail-closed: the same verdict on the holding row + // and the failing one, and one console line naming the variable. The + // ungated companion proves the bar rendered. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const gated = { ...APPROVE, visible: 'status == "pending"' }; + const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'pending' }); + expect(screen.queryByTestId('declared-action-approval_approve')).toBeNull(); + expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); + unmount(); + renderWithGate(gated, { ...REQUEST, status: 'approved' }); + expect(screen.queryByTestId('declared-action-approval_approve')).toBeNull(); + expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); + const reports = warn.mock.calls + .map((c) => String(c[0])) + .filter((m) => m.includes('was hidden/disabled: its predicate threw')); + expect(reports).toHaveLength(1); + expect(reports[0]).toContain('status is not defined'); + expect(reports[0]).toContain('declared action "approval_approve" (visible)'); + } finally { + warn.mockRestore(); + } + }); + it('a `${…}`-spelled `visible` keeps its verdict — true shows, false hides (objectui#3871)', () => { // This leg opts into `throwOnError`, so the double wrap did not read as // truthy here: `'${${…}}'` THREW inside the evaluator and `useCondition`'s @@ -533,7 +560,7 @@ describe('DeclaredActionsBar — declared `visible` on a server-declared action // Reverse verification: the `pending` half is the detector; the `approved` // half stays green either way (hidden is hidden), and the companion // assertion is what keeps that half from meaning "the bar vanished". - const gated = { ...APPROVE, visible: '${status === "pending"}' }; + const gated = { ...APPROVE, visible: '${record.status === "pending"}' }; const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'approved' }); expect(screen.queryByTestId('declared-action-approval_approve')).toBeNull(); expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); @@ -623,7 +650,7 @@ describe('DeclaredActionsBar — declared `disabled` on a server-declared action }); it('an expression-valued `disabled` keeps its verdict — true disables, false does not', () => { - const gated = { ...APPROVE, disabled: 'status == "approved"' }; + const gated = { ...APPROVE, disabled: 'record.status == "approved"' }; const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'approved' }); expect(approve()).toBeDisabled(); unmount(); @@ -631,6 +658,19 @@ describe('DeclaredActionsBar — declared `disabled` on a server-declared action expect(approve()).not.toBeDisabled(); }); + it('a bare-field `disabled` no longer discriminates (objectui#5741) — disabled on BOTH rows (fail-soft leg)', () => { + // The `disabled` leg does not opt into `throwOnError`, so its EXISTING + // policy is fail-soft `true`: an unbound `status` faults and greys Approve + // on the holding row and the failing one alike. Pinned as the fail-soft + // half of the ruling, next to the fail-closed `visible` half above. + const gated = { ...APPROVE, disabled: 'status == "approved"' }; + const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'approved' }); + expect(approve()).toBeDisabled(); + unmount(); + renderWithGate(gated, { ...REQUEST, status: 'pending' }); + expect(approve()).toBeDisabled(); + }); + it('a `${…}`-spelled `disabled` keeps its verdict — false leaves Approve clickable (objectui#3871)', () => { // `toPredicateInput` used to wrap EVERY string, so `'${…}'` — a spelling // AGENTS.md §4 documents and this bar's own `visible` sibling accepts — @@ -643,7 +683,7 @@ describe('DeclaredActionsBar — declared `disabled` on a server-declared action // disabled before the fix); the `approved` half was green already, since // "disabled because the predicate holds" and "disabled because it could not // be parsed" look identical from here. - const gated = { ...APPROVE, disabled: '${status === "approved"}' }; + const gated = { ...APPROVE, disabled: '${record.status === "approved"}' }; const { unmount } = renderWithGate(gated, { ...REQUEST, status: 'approved' }); expect(approve()).toBeDisabled(); unmount(); @@ -670,16 +710,17 @@ describe('DeclaredActionsBar — declared `disabled` on a server-declared action * objectui#4077 fixed the root-only binding here with an inline * `{ ...row, record: row, data: row }`; objectui#4079 fixed the same fault on * the four generic action renderers and gave the rule one name instead of a - * fifth copy. The two copies agreed on every row the bar has ever been mounted - * over — which is why this is a convergence card and not a defect report, and - * why the two cases below are deliberately different in kind: + * fifth copy. objectui#5741 (Phase 2 of the objectui#5330 canon) then narrowed + * that shared rule to `{ record: row }`: the bare-field and `data.*` spellings + * are no longer bound anywhere, and this bar follows the helper. The two cases + * below are deliberately different in kind: * - * • the ROW-PRESENT case is an EQUIVALENCE pin. It was green before the - * migration and is green after it, because the copies agree wherever a row - * exists. It is not a mutation detector for this change and must not be - * read as one; it is here so the reachable path — the only path any host - * drives today — is pinned against a future edit to the helper, which now - * owns the verdict for this bar too. + * • the ROW-PRESENT case pins the helper's CURRENT rule as this bar sees it: + * `record.*` discriminates, the two retired spellings reach the SAME + * verdict on the holding row and the failing one (they fault, and this + * leg's existing policy is fail-closed). It is here so the reachable path + * — the only path any host drives today — is pinned against a future edit + * to the helper, which owns the verdict for this bar too. * • the NO-ROW case is the CONVERGENCE detector, and the one difference the * two copies ever had. `usePredicateRecordContext` binds NOTHING when there * is no row; the inline copy bound `{ record: {}, data: {} }`. Since @@ -700,24 +741,42 @@ describe('DeclaredActionsBar — the row binds through the shared helper (object locations: ['record_section'], }; - it('with a row present, all three spellings reach the same verdict', () => { - // Both halves per spelling: "renders" alone is satisfied by a bar that - // ignores `visible` entirely, which is the mutation objectui#3835 was. - for (const visible of [ - 'record.status == "pending"', - 'status == "pending"', - 'data.status == "pending"', - ]) { - const shown = renderWithGate({ ...APPROVE, visible }, { ...REQUEST, status: 'pending' }); - expect(screen.getByTestId('declared-action-approval_approve'), visible).toBeInTheDocument(); - shown.unmount(); - - const hidden = renderWithGate({ ...APPROVE, visible }, { ...REQUEST, status: 'approved' }); - expect(screen.queryByTestId('declared-action-approval_approve'), visible).toBeNull(); - // The ungated companion proves the bar itself rendered — "not found" here - // must mean the gate said no, not that the located set was empty. - expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); - hidden.unmount(); + it('with a row present, `record.*` reaches both verdicts through the shared helper', () => { + // Both halves: "renders" alone is satisfied by a bar that ignores `visible` + // entirely, which is the mutation objectui#3835 was. + const visible = 'record.status == "pending"'; + const shown = renderWithGate({ ...APPROVE, visible }, { ...REQUEST, status: 'pending' }); + expect(screen.getByTestId('declared-action-approval_approve'), visible).toBeInTheDocument(); + shown.unmount(); + + const hidden = renderWithGate({ ...APPROVE, visible }, { ...REQUEST, status: 'approved' }); + expect(screen.queryByTestId('declared-action-approval_approve'), visible).toBeNull(); + // The ungated companion proves the bar itself rendered — "not found" here + // must mean the gate said no, not that the located set was empty. + expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); + hidden.unmount(); + }); + + it('with a row present, the two retired spellings reach the SAME verdict on both rows (objectui#5741)', () => { + // The helper binds `{ record: row }` only, so these fault; the bar's + // `visible` leg is fail-closed, so "the same verdict" is hidden twice — + // with the companion present both times, so hidden means the gate faulted, + // not that the bar vanished. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + for (const visible of ['status == "pending"', 'data.status == "pending"']) { + const holding = renderWithGate({ ...APPROVE, visible }, { ...REQUEST, status: 'pending' }); + expect(screen.queryByTestId('declared-action-approval_approve'), visible).toBeNull(); + expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); + holding.unmount(); + + const failing = renderWithGate({ ...APPROVE, visible }, { ...REQUEST, status: 'approved' }); + expect(screen.queryByTestId('declared-action-approval_approve'), visible).toBeNull(); + expect(screen.getByTestId('declared-action-approval_reassign')).toBeInTheDocument(); + failing.unmount(); + } + } finally { + warn.mockRestore(); } }); diff --git a/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx b/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx index f32d693fec..c739cd282a 100644 --- a/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx +++ b/packages/components/src/__tests__/page-header-predicate-dialect.test.tsx @@ -32,7 +32,7 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { render, screen } from '@testing-library/react'; +import { render, screen, cleanup } from '@testing-library/react'; import { ComponentRegistry } from '@object-ui/core'; import { ActionProvider, RecordContextProvider, PredicateScopeProvider } from '@object-ui/react'; @@ -204,14 +204,24 @@ describe('page:header — CEL-only constructs in action predicates (#3521)', () }); describe('bindings the row surfaces already offered', () => { - it('binds bare field names', () => { + // objectui#5741 (Phase 2 of the objectui#5330 canon): the bare-field and + // `data.*` spellings are no longer bound on a record surface. They fault, + // and this surface's existing policy is fail-closed — the SAME verdict on a + // matching and a non-matching row is what "unbound" looks like from here. + it('no longer binds bare field names (objectui#5741) — hidden on both rows', () => { renderHeader({ name: 'zoo_bare', visible: 'f_status == "open"' }); - expect(shown()).toBe(true); + expect(shown()).toBe(false); + cleanup(); + renderHeader({ name: 'zoo_bare_other', visible: 'f_status == "open"' }, { ...RECORD, f_status: 'closed' }); + expect(shown()).toBe(false); }); - it('binds `data.*`', () => { + it('no longer binds `data.*` (objectui#5741) — hidden on both rows', () => { renderHeader({ name: 'zoo_data', visible: 'data.f_status == "open"' }); - expect(shown()).toBe(true); + expect(shown()).toBe(false); + cleanup(); + renderHeader({ name: 'zoo_data_other', visible: 'data.f_status == "open"' }, { ...RECORD, f_status: 'closed' }); + expect(shown()).toBe(false); }); it('binds the host scope (`os.user.*`) alongside the record', () => { 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 index dd56703071..9eeafee397 100644 --- 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 @@ -7,56 +7,51 @@ */ /** - * objectui#4075 — the row must be bound the THREE canonical ways on the action - * face, not just at the bare root. + * objectui#4075 — the row must be bound on the action face, through the shared + * `usePredicateRecordContext` — and objectui#5741 — that binding is `record.*` + * ONLY. * * `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. + * ...ctx })`. 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, and what the server enforces with. #4075 / PR #4079 put the shared + * helper under these four renderers because two of them — `action:menu`'s item + * and `action:group`'s two leaves — had no record in scope at all, and + * `DeclaredActionsBar` carried the same root-only fault (PR #4077). That helper + * bound the row three ways (`record.*`, bare `status`, `data.*`) until + * objectui#5330 ruled `record.*` the canon and objectui#5741 (Phase 2) retired + * the other two: the helper now binds `{ record: row }` and nothing else, with + * no survey and no special case. * * ## What each block pins, and in which direction * - * Per site, three roots × two polarities, plus the fault case: + * Per site, the canon in two polarities, the two RETIRED spellings on both + * rows, 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. + * • `record.*` true / false — THE binding. Both polarities are asserted at + * every site because only one of them can be red at a time, and which one + * depends on that site's error policy (a throw hides on a fail-CLOSED + * `visible`; it shows / disables / enables on a fail-SOFT leg). + * • bare `status` and `data.*`, each on the HOLDING row and the FAILING row — + * the same verdict on both. A retired spelling is an unknown variable, so + * it takes the site's EXISTING fault policy (#3871's table, in + * `action-template-predicate-gate.test.tsx`): hidden on the fail-closed + * `visible` legs (`action:button`, `action:menu` item, and therefore the + * `action:bar` overflow, which IS an `action:menu`); shown / greyed / + * enabled on the fail-soft legs. "The same verdict on both rows" is what + * "no longer bound" looks like from outside, and the ruling's cost + * statement is exactly that pair. * • 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`). + * each site's EXISTING error policy — unchanged by either card, and pinned + * so neither can be read as having quietly converted a fail-soft leg. * * 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 { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; import '@testing-library/jest-dom'; import React from 'react'; import { ComponentRegistry } from '@object-ui/core'; @@ -78,7 +73,7 @@ import '../action-group'; */ const ROW = { id: 'r1', status: 'pending', viewer: { can_act: true } }; -/** The same question asked through each of the three canonical roots. */ +/** The same question asked through the canon and the two retired roots. */ const HOLDS = { record: "record.status == 'pending'", bare: "status == 'pending'", @@ -89,6 +84,15 @@ const FAILS = { bare: "status == 'closed'", data: "data.status == 'closed'", } as const; +/** + * The two spellings objectui#5741 retired, each with its holding and failing + * form: a site that still bound them would answer the two differently, a site + * that does not answers them the same way. + */ +const RETIRED = [ + ['bare', HOLDS.bare, FAILS.bare], + ['data.*', HOLDS.data, FAILS.data], +] 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. */ @@ -136,23 +140,17 @@ function mountButton(action: any) { ); } -describe('action:button — the row binds three ways (objectui#4075)', () => { +describe('action:button — the row binds as `record.*` (objectui#4075 / #5741)', () => { 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 }); + it('a failing `visible` written as record.* hides the button', () => { + mountButton({ name: 'act', label: LABEL, visible: FAILS.record }); hidden(); }); @@ -161,32 +159,44 @@ describe('action:button — the row binds three ways (objectui#4075)', () => { 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 }); + it.each(RETIRED)('a `visible` written as %s no longer discriminates — hidden on BOTH rows (fail-closed leg)', (_root, holding, failing) => { + mountButton({ name: 'act', label: LABEL, visible: holding }); + hidden(); + cleanup(); + mountButton({ name: 'act', label: LABEL, visible: failing }); + hidden(); + }); + + it('a holding `disabled` written as record.* greys the button', () => { + mountButton({ name: 'act', label: LABEL, disabled: HOLDS.record }); 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 }); + it('a failing `disabled` written as record.* leaves the button clickable', () => { + mountButton({ name: 'act', label: LABEL, disabled: FAILS.record }); 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 }); + it.each(RETIRED)('a `disabled` written as %s no longer discriminates — greyed on BOTH rows (fail-soft leg)', (_root, holding, failing) => { + mountButton({ name: 'act', label: LABEL, disabled: holding }); + expect(screen.getByText(LABEL).closest('button')).toBeDisabled(); + cleanup(); + mountButton({ name: 'act', label: LABEL, disabled: failing }); expect(screen.getByText(LABEL).closest('button')).toBeDisabled(); }); + + it('a failing legacy `enabled` written as record.* greys the button', () => { + mountButton({ name: 'act', label: LABEL, enabled: FAILS.record }); + expect(screen.getByText(LABEL).closest('button')).toBeDisabled(); + }); + + it.each(RETIRED)('a legacy `enabled` written as %s no longer discriminates — clickable on BOTH rows (fail-soft leg)', (_root, holding, failing) => { + mountButton({ name: 'act', label: LABEL, enabled: holding }); + expect(screen.getByText(LABEL).closest('button')).not.toBeDisabled(); + cleanup(); + mountButton({ name: 'act', label: LABEL, enabled: failing }); + expect(screen.getByText(LABEL).closest('button')).not.toBeDisabled(); + }); }); // --------------------------------------------------------------------------- @@ -209,26 +219,28 @@ const iconHidden = () => { expect(screen.getByLabelText('View')).toBeInTheDocument(); }; -describe('action:icon — the row binds three ways (objectui#4075)', () => { +describe('action:icon — the row binds as `record.*` (objectui#4075 / #5741)', () => { 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 }); + it('a failing `visible` written as record.* hides the icon', () => { + mountIcon({ name: 'act', label: LABEL, visible: FAILS.record }); iconHidden(); }); + it.each(RETIRED)('a `visible` written as %s no longer discriminates — shown on BOTH rows (fail-soft leg)', (_root, holding, failing) => { + mountIcon({ name: 'act', label: LABEL, visible: holding }); + iconShown(); + cleanup(); + mountIcon({ name: 'act', label: LABEL, visible: failing }); + iconShown(); + }); + 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 @@ -237,23 +249,23 @@ describe('action:icon — the row binds three ways (objectui#4075)', () => { 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 }); + it('a holding `disabled` written as record.* greys the icon', () => { + mountIcon({ name: 'act', label: LABEL, disabled: HOLDS.record }); 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 }); + it('a failing `disabled` written as record.* leaves the icon clickable', () => { + mountIcon({ name: 'act', label: LABEL, disabled: FAILS.record }); expect(screen.getByLabelText(LABEL)).not.toBeDisabled(); }); + + it.each(RETIRED)('a `disabled` written as %s no longer discriminates — greyed on BOTH rows (fail-soft leg)', (_root, holding, failing) => { + mountIcon({ name: 'act', label: LABEL, disabled: holding }); + expect(screen.getByLabelText(LABEL)).toBeDisabled(); + cleanup(); + mountIcon({ name: 'act', label: LABEL, disabled: failing }); + expect(screen.getByLabelText(LABEL)).toBeDisabled(); + }); }); // --------------------------------------------------------------------------- @@ -273,23 +285,25 @@ async function mountMenu(action: any) { return r; } -describe('action:menu item — the row binds three ways (objectui#4075)', () => { +describe('action:menu item — the row binds as `record.*` (objectui#4075 / #5741)', () => { 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 }); + it('a failing `visible` written as record.* hides the menu item', async () => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', visible: FAILS.record }); + hidden(); + }); + + it.each(RETIRED)('a `visible` written as %s no longer discriminates — hidden on BOTH rows (fail-closed leg)', async (_root, holding, failing) => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', visible: holding }); + hidden(); + cleanup(); + await mountMenu({ name: 'act', label: LABEL, type: 'script', visible: failing }); hidden(); }); @@ -298,27 +312,27 @@ describe('action:menu item — the row binds three ways (objectui#4075)', () => 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 }); + it('a holding `disabled` written as record.* greys the menu item', async () => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', disabled: HOLDS.record }); 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 }); + it('a failing `disabled` written as record.* leaves the menu item live', async () => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', disabled: FAILS.record }); expect(screen.getByText(LABEL).closest('[role="menuitem"]')).not.toHaveAttribute( 'data-disabled', ); }); + + it.each(RETIRED)('a `disabled` written as %s no longer discriminates — greyed on BOTH rows (fail-soft leg)', async (_root, holding, failing) => { + await mountMenu({ name: 'act', label: LABEL, type: 'script', disabled: holding }); + expect(screen.getByText(LABEL).closest('[role="menuitem"]')).toHaveAttribute('data-disabled'); + cleanup(); + await mountMenu({ name: 'act', label: LABEL, type: 'script', disabled: failing }); + expect(screen.getByText(LABEL).closest('[role="menuitem"]')).toHaveAttribute('data-disabled'); + }); }); // --------------------------------------------------------------------------- @@ -350,26 +364,28 @@ async function mountDropdownGroup(action: any) { describe.each([ ['inline', mountInlineGroup], ['dropdown', mountDropdownGroup], -])('action:group %s leaf — the row binds three ways (objectui#4075)', (_mode, mount) => { +])('action:group %s leaf — the row binds as `record.*` (objectui#4075 / #5741)', (_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 }); + it('a failing `visible` written as record.* hides the action', async () => { + await mount({ name: 'act', label: LABEL, type: 'script', visible: FAILS.record }); hidden(); }); + it.each(RETIRED)('a `visible` written as %s no longer discriminates — shown on BOTH rows (fail-soft leg)', async (_root, holding, failing) => { + await mount({ name: 'act', label: LABEL, type: 'script', visible: holding }); + shown(); + cleanup(); + await mount({ name: 'act', label: LABEL, type: 'script', visible: failing }); + shown(); + }); + 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. @@ -444,20 +460,22 @@ describe('action:bar overflow menu — the row reaches an overflowed action (obj 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 }); + it('a failing `visible` written as record.* hides the overflowed action', async () => { + await mountBarOverflow({ name: 'act', label: LABEL, type: 'script', visible: FAILS.record }); + hidden(); + }); + + it.each(RETIRED)('a `visible` written as %s no longer discriminates — hidden on BOTH rows (the overflow is an action:menu, fail-closed)', async (_root, holding, failing) => { + await mountBarOverflow({ name: 'act', label: LABEL, type: 'script', visible: holding }); + hidden(); + cleanup(); + await mountBarOverflow({ name: 'act', label: LABEL, type: 'script', visible: failing }); hidden(); }); }); 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 59e4b6e1fb..616e6c325b 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 @@ -21,7 +21,7 @@ * `DataTableRowActionItem` subcomponent that now evaluates the predicate. */ import { describe, it, expect } from 'vitest'; -import { render, screen } from '@testing-library/react'; +import { render, screen, cleanup } from '@testing-library/react'; import '@testing-library/jest-dom'; import React from 'react'; import { PredicateScopeProvider } from '@object-ui/react'; @@ -72,12 +72,17 @@ describe('data-table row action — visible / disabled CEL evaluation', () => { expect(screen.getByText('Transfer Ownership')).toBeInTheDocument(); }); - it('supports the `record.` scope for the visible predicate', () => { - // Same predicate, referenced via the bare-field scope should behave the - // same as `record.` — assert the bare-field convention also resolves. + it('no longer binds the bare-field spelling (objectui#5741) — hidden on BOTH rows, fail-closed', () => { + // `role != 'owner'` is unbound since Phase 2 of the objectui#5330 canon: it + // faults, and this surface's existing policy is fail-closed, so the verdict + // is the same whichever row it meets. The `record.` spelling above is what + // discriminates. const bareField = { name: 'transfer_ownership', label: 'Transfer Ownership', visible: "role != 'owner'" }; renderRowActionItem(bareField, { id: '1', role: 'owner' }); expect(screen.queryByTestId('row-action-transfer_ownership')).toBeNull(); + cleanup(); + renderRowActionItem(bareField, { id: '2', role: 'member' }); + expect(screen.queryByTestId('row-action-transfer_ownership')).toBeNull(); }); it('renders an action with no `visible` predicate unconditionally', () => { diff --git a/packages/core/src/evaluator/__tests__/listConditional.test.ts b/packages/core/src/evaluator/__tests__/listConditional.test.ts index 055122cce9..46f08b8fb9 100644 --- a/packages/core/src/evaluator/__tests__/listConditional.test.ts +++ b/packages/core/src/evaluator/__tests__/listConditional.test.ts @@ -38,10 +38,12 @@ describe('isLegacyDialectSource', () => { }); describe('evalRowPredicate', () => { - it('evaluates a canonical CEL predicate over the row (record.* and bare)', () => { + it('evaluates a canonical CEL predicate over the row (`record.*`; the bare shorthand retired in objectui#5741)', () => { expect(evalRowPredicate("record.status == 'active'", { status: 'active' })).toBe(true); - expect(evalRowPredicate("status == 'active'", { status: 'active' })).toBe(true); expect(evalRowPredicate("record.status == 'active'", { status: 'closed' })).toBe(false); + // The bare shorthand is unbound: it faults into the fallback on EITHER row. + expect(evalRowPredicate("status == 'active'", { status: 'active' })).toBe(false); + expect(evalRowPredicate("status == 'active'", { status: 'closed' })).toBe(false); }); it('supports the CEL `in` operator (which the legacy engine lacks)', () => { @@ -94,15 +96,26 @@ describe('evalRowPredicate', () => { afterEach(() => warn.mockRestore()); it('routes a `${…}` template string to the legacy engine and warns once', () => { - expect(evalRowPredicate('${data.status === "active"}', { status: 'active' })).toBe(true); - expect(evalRowPredicate('${data.status === "active"}', { status: 'closed' })).toBe(false); + expect(evalRowPredicate('${record.status === "active"}', { status: 'active' })).toBe(true); + expect(evalRowPredicate('${record.status === "active"}', { status: 'closed' })).toBe(false); // Same source warns only once. expect(warn).toHaveBeenCalledTimes(1); expect(String(warn.mock.calls[0][0])).toContain('legacy expression dialect'); }); it('routes a bare `===` string to the legacy engine', () => { - expect(evalRowPredicate("status === 'active'", { status: 'active' })).toBe(true); + expect(evalRowPredicate("record.status === 'active'", { status: 'active' })).toBe(true); + }); + + it('a legacy `${data.x}` / `${x}` string on a row surface retired with the CEL spellings (objectui#5741)', () => { + // One scope shape per surface, both dialects: `data` and the bare name + // are unbound on the legacy path too, so these fault into the fallback + // on either row — the same verdict on a matching and a non-matching one. + for (const pred of ['${data.status === "active"}', '${status === "active"}', "status === 'active'"]) { + expect(evalRowPredicate(pred, { status: 'active' }), pred).toBe(false); + expect(evalRowPredicate(pred, { status: 'closed' }), pred).toBe(false); + expect(evalRowPredicate(pred, { status: 'active' }, { fallback: true }), pred).toBe(true); + } }); }); @@ -253,11 +266,17 @@ describe('evalRowPredicate — row wins over host scope (both dialect paths)', ( expect(evalRowPredicate("record.tag === 'SCOPE'", ROW, { scope: DECOY })).toBe(false); }); - it('binds `data` and the bare name to the row on both paths too', () => { - expect(evalRowPredicate("data.tag == 'ROW'", ROW, { scope: DECOY })).toBe(true); - expect(evalRowPredicate("data.tag === 'ROW'", ROW, { scope: DECOY })).toBe(true); - expect(evalRowPredicate("tag == 'ROW'", ROW, { scope: DECOY })).toBe(true); - expect(evalRowPredicate("tag === 'ROW'", ROW, { scope: DECOY })).toBe(true); + it('no longer binds `data` or the bare name to the row — on either path (objectui#5741)', () => { + // `data` is the HOST's own now: the decoy answers, on both dialect paths. + expect(evalRowPredicate("data.tag == 'SCOPE'", ROW, { scope: DECOY })).toBe(true); + expect(evalRowPredicate("data.tag === 'SCOPE'", ROW, { scope: DECOY })).toBe(true); + expect(evalRowPredicate("data.tag == 'ROW'", ROW, { scope: DECOY })).toBe(false); + expect(evalRowPredicate("data.tag === 'ROW'", ROW, { scope: DECOY })).toBe(false); + // The bare name is bound nowhere: it faults into the fallback on both paths. + expect(evalRowPredicate("tag == 'ROW'", ROW, { scope: DECOY })).toBe(false); + expect(evalRowPredicate("tag === 'ROW'", ROW, { scope: DECOY })).toBe(false); + expect(evalRowPredicate("tag == 'ROW'", ROW, { scope: DECOY, fallback: true })).toBe(true); + expect(evalRowPredicate("tag === 'ROW'", ROW, { scope: DECOY, fallback: true })).toBe(true); }); it('still resolves host-scope keys the row does not shadow', () => { @@ -279,11 +298,13 @@ describe('evalRowPredicate — row wins over host scope (both dialect paths)', ( }); it('a row FIELD named `record` does not become the subject either', () => { - // The pin sits after the bare-field spread, so a column literally called - // `record` is addressable as `data.record`, never as the row root. + // `record` is the row's one root, so a column literally called `record` is + // addressable as `record.record`, never as the row root (objectui#5741: the + // `data.record` spelling it used to have retired with `data.*`). const row = { record: { tag: 'FIELD' }, tag: 'ROW' }; expect(evalRowPredicate("record.tag == 'ROW'", row, { scope: DECOY })).toBe(true); - expect(evalRowPredicate("data.record.tag == 'FIELD'", row, { scope: DECOY })).toBe(true); + expect(evalRowPredicate("record.record.tag == 'FIELD'", row, { scope: DECOY })).toBe(true); + expect(evalRowPredicate("data.record.tag == 'FIELD'", row, { scope: DECOY })).toBe(false); }); it('applies through conditional formatting, which shares the entry point', () => { @@ -433,11 +454,12 @@ describe('evalRowPredicate — relation fields', () => { warn.mockRestore(); }); - it('binds the collapsed value under the bare and `data.` spellings too', () => { + it('binds the collapsed value under `record.` only — the bare and `data.` spellings retired (objectui#5741)', () => { const expanded = { account: { id: 'A1', name: 'Acme' } }; - expect(evalRowPredicate('account == "A1"', expanded, { fields: FIELDS })).toBe(true); - expect(evalRowPredicate('data.account == "A1"', expanded, { fields: FIELDS })).toBe(true); + expect(evalRowPredicate('record.account == "A1"', expanded, { fields: FIELDS })).toBe(true); + expect(evalRowPredicate('account == "A1"', expanded, { fields: FIELDS })).toBe(false); + expect(evalRowPredicate('data.account == "A1"', expanded, { fields: FIELDS })).toBe(false); }); it('leaves a non-relational object field addressable as an object', () => { diff --git a/packages/core/src/evaluator/__tests__/rowPredicateCanon.schemaCatalog.test.ts b/packages/core/src/evaluator/__tests__/rowPredicateCanon.schemaCatalog.test.ts new file mode 100644 index 0000000000..73583030e5 --- /dev/null +++ b/packages/core/src/evaluator/__tests__/rowPredicateCanon.schemaCatalog.test.ts @@ -0,0 +1,122 @@ +/** + * 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#5741 — the in-repo authored corpus carries no retired row-predicate + * spelling (Phase 2 of the objectui#5330 canon; the objectui#5738 sweep, kept). + * + * PR #5758 (Phase 0) swept the repo key-agnostically — every string literal + * carrying a comparison / boolean operator, classified by CEL root with + * `@objectstack/formula`'s own oracles and by `detectNonCanonicalRowSpelling` — + * and reported the schema-catalog clean. That sweep was a one-shot. This pin is + * the part of it that is MECHANICAL: the catalog is JSON, every string in it is + * authored metadata or display text, and a predicate-shaped string rooted at + * `data` or at a bare undeclared identifier is exactly the retired spelling an + * author (or an AI author) would copy from here. The prose corpus (`content/docs`, + * `apps`, `skills`, package READMEs) is deliberately NOT pinned: there the same + * scan needs tier judgement — flow-tier conditions, formula-field expressions, + * view filters, JS expressions in test files — and stays a PR-body reading. + * + * Reads are rooted at this file (objectui#7799), never at the cwd, and the + * corpus is enumerated from disk rather than listed here, so a new catalog + * document is in scope the day it lands. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync } from 'node:fs'; +import { dirname, join, relative } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { collectCelRootIdentifiers, firstUndeclaredReference } from '@objectstack/formula'; +import { detectNonCanonicalRowSpelling } from '../rowPredicateCanon.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(HERE, '..', '..', '..', '..', '..'); +const CATALOG = join(REPO_ROOT, 'examples', 'schema-catalog', 'src', 'schemas'); + +/** A string that could be a predicate at all: it carries an operator. */ +const PREDICATE_SHAPED = /(==|!=|<=|>=|<|>|&&|\|\||\bin\b|^!)/; + +function jsonFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const p = join(dir, entry.name); + if (entry.isDirectory()) out.push(...jsonFiles(p)); + else if (entry.isFile() && entry.name.endsWith('.json')) out.push(p); + } + return out.sort(); +} + +function collectStrings(value: unknown, out: string[]): void { + if (typeof value === 'string') out.push(value); + else if (Array.isArray(value)) value.forEach((v) => collectStrings(v, out)); + else if (value && typeof value === 'object') Object.values(value).forEach((v) => collectStrings(v, out)); +} + +interface Sweep { + files: number; + strings: number; + predicateShaped: number; + parsedAsCel: number; + /** `file: "source" → kind`, one per retired spelling found. */ + hits: string[]; +} + +function sweep(): Sweep { + const files = jsonFiles(CATALOG); + const result: Sweep = { files: files.length, strings: 0, predicateShaped: 0, parsedAsCel: 0, hits: [] }; + for (const file of files) { + const strings: string[] = []; + collectStrings(JSON.parse(readFileSync(file, 'utf8')), strings); + result.strings += strings.length; + for (const source of strings) { + if (source.length < 3 || !PREDICATE_SHAPED.test(source)) continue; + result.predicateShaped++; + const where = `${relative(REPO_ROOT, file)}: ${JSON.stringify(source)}`; + // Legacy `${…}` strings are not CEL; on a row surface `${data.x}` retired + // with the CEL spellings (objectui#5741 Q3), so a `data.`-rooted one is a + // hit by inspection. A bare name inside a template cannot be classified + // without a JS parser and is left to the CEL arm below. + if (source.includes('${')) { + if (/\bdata\./.test(source)) result.hits.push(`${where} → legacy data.*`); + continue; + } + const roots = collectCelRootIdentifiers(source); + if (!roots || roots.ok !== true) continue; // display text, not a predicate + result.parsedAsCel++; + const finding = detectNonCanonicalRowSpelling(source, {}, true); + if (finding) result.hits.push(`${where} → ${finding.kind}`); + const bare = firstUndeclaredReference(source); + if (typeof bare === 'string') result.hits.push(`${where} → bare root ${JSON.stringify(bare)}`); + } + } + return result; +} + +describe('[#5741] the schema-catalog corpus carries no retired row-predicate spelling', () => { + const result = sweep(); + + it('reaches the corpus (positive control): hundreds of documents, and real CEL predicates among them', () => { + // Measured on landing: 432 documents, 7302 strings, 20 predicate-shaped, + // 5 parsed as CEL (`record.*` and `current_user.*`). A zero below would be + // a scanner that never arrived, not a clean corpus. + expect(result.files).toBeGreaterThan(300); + expect(result.strings).toBeGreaterThan(1000); + expect(result.parsedAsCel).toBeGreaterThan(0); + }); + + it('finds no `data.*`, no legacy `${data.x}`, and no bare-field predicate-shaped string', () => { + expect(result.hits).toEqual([]); + }); + + it('control: the same classifier does report the retired spellings, and stands down on the canon', () => { + expect(detectNonCanonicalRowSpelling("data.status == 'x'", {}, true)?.kind).toBe('metadata-layer-root'); + expect(firstUndeclaredReference("status == 'x'")).toBe('status'); + expect(detectNonCanonicalRowSpelling("record.status == 'x'", {}, true)).toBeNull(); + expect(typeof firstUndeclaredReference("record.status == 'x' && current_user.id == 'u1'")).not.toBe('string'); + }); +}); diff --git a/packages/core/src/evaluator/__tests__/rowPredicateCanon.test.ts b/packages/core/src/evaluator/__tests__/rowPredicateCanon.test.ts index 07947ee079..add813aed4 100644 --- a/packages/core/src/evaluator/__tests__/rowPredicateCanon.test.ts +++ b/packages/core/src/evaluator/__tests__/rowPredicateCanon.test.ts @@ -7,35 +7,39 @@ */ /** - * objectui#5330 — the row-predicate spelling CANON (`record.*`) and its Phase-1 - * deprecation warning. + * objectui#5330 — the row-predicate spelling CANON (`record.*`), and + * objectui#5741 — Phase 2, where the two other spellings stopped being bound. * - * These pins are deliberately split in two, because the card's two halves fail - * in opposite directions: + * Two halves, and the second is pinned in the OPPOSITE direction from the + * Phase-1 file this replaces: * - * - the CANON pins assert the binding is UNCHANGED. The ruling defers every - * removal behind a stored-metadata survey, so a test that stopped resolving - * the shorthand would be the regression, not the feature. - * - the WARNING pins assert the tolerance is no longer silent — the ADR-0078 - * reason a tolerance nothing reports can never be retired. + * - the DETECTOR pins are unchanged. `detectNonCanonicalRowSpelling` stays + * exported as the OFFLINE instrument (the objectui#5738 corpus sweep runs on + * it), and its three stand-downs still hold. + * - the BINDING pins now assert the removal. `record.*` still discriminates; + * a bare-field or `data.*` predicate — and a legacy `${data.x}` / `${x}` + * string, one scope shape for both dialects — reaches the SAME verdict on a + * matching and a non-matching row (it faults and takes the caller's + * fallback); the fault warning names the unknown variable; the Phase-1 + * deprecation warning is gone, together with its runtime half of the module. * - * The `record-alert` renderer's own three-spelling pins landed separately with - * PR #5688 (`plugin-detail/.../record-alert.rowBinding.test.tsx`) and are NOT - * duplicated here; this file pins the shared evaluator tier those renderers sit - * on, plus the detector itself. + * The `record-alert` renderer's own pins live in + * `plugin-detail/.../record-alert.rowBinding.test.tsx`, the `useCondition` + * tier's in `react/.../useCondition.canonSpelling.test.tsx`; this file pins the + * shared evaluator tier those renderers sit on, plus the detector itself. */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; +import * as core from '../index.js'; import { detectNonCanonicalRowSpelling, - resetRowPredicateCanonWarnings, ROW_PREDICATE_CANONICAL_ROOT, evalRowPredicate, + partitionRowsByPredicate, } from '../index.js'; const row = { status: 'in_review', amount: 10 }; - -beforeEach(() => resetRowPredicateCanonWarnings()); +const other = { status: 'draft', amount: 1 }; describe('[#5330] the canon is `record.*`', () => { it('names `record` as the one canonical root', () => { @@ -47,7 +51,7 @@ describe('[#5330] the canon is `record.*`', () => { }); }); -describe('[#5330] non-canonical spellings are DETECTED', () => { +describe('[#5330] non-canonical spellings are DETECTED (the offline instrument, kept by #5741)', () => { it('reports the bare shorthand, and names the canonical rewrite', () => { expect(detectNonCanonicalRowSpelling("status == 'in_review'", row, true)).toEqual({ kind: 'bare-shorthand', @@ -68,8 +72,8 @@ describe('[#5330] non-canonical spellings are DETECTED', () => { /** * Every case here is a spelling the detector must NOT report. They are the * whole reason it consults the row and the caller's binding rather than - * pattern-matching the source: a false deprecation warning sends an author to - * rewrite a predicate that was correct. + * pattern-matching the source: a false finding sends an author to rewrite a + * predicate that was correct. */ describe('[#5330] the detector stands down rather than guessing', () => { it('leaves `data.*` alone when `data` is NOT this row (rowless / metadata-editing layer)', () => { @@ -92,59 +96,94 @@ describe('[#5330] the detector stands down rather than guessing', () => { }); }); -describe('[#5330] `evalRowPredicate` — the binding is UNCHANGED (no removal before the survey)', () => { - it('still resolves all three spellings against the row', () => { - expect(evalRowPredicate("record.status == 'in_review'", row)).toBe(true); - expect(evalRowPredicate("status == 'in_review'", row)).toBe(true); - expect(evalRowPredicate("data.status == 'in_review'", row)).toBe(true); - }); - - it('still tells the three spellings apart on a NON-matching row (not vacuously true)', () => { - const other = { status: 'draft', amount: 1 }; - expect(evalRowPredicate("record.status == 'in_review'", other)).toBe(false); - expect(evalRowPredicate("status == 'in_review'", other)).toBe(false); - expect(evalRowPredicate("data.status == 'in_review'", other)).toBe(false); - }); -}); - -describe('[#5330] `evalRowPredicate` — the tolerance is no longer silent', () => { - let warn: ReturnType; +describe('[#5741] `evalRowPredicate` — the row is bound as `record.*` only', () => { + let warn: MockInstance; beforeEach(() => { warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); }); afterEach(() => warn.mockRestore()); - const deprecationWarnings = (): string[] => - warn.mock.calls.map((c: unknown[]) => String(c[0])).filter((m: string) => m.includes('DEPRECATED spelling')); + const messages = (): string[] => warn.mock.calls.map((c) => String(c[0])); + const faultReports = (): string[] => messages().filter((m) => m.includes('failed to evaluate')); - it('warns on the bare shorthand and prescribes `record.status`', () => { + it('`record.*` still discriminates: true on the matching row, false on the other, silently', () => { + expect(evalRowPredicate("record.status == 'in_review'", row)).toBe(true); + expect(evalRowPredicate("record.status == 'in_review'", other)).toBe(false); + expect(messages()).toHaveLength(0); + }); + + it.each([ + ['bare shorthand', "status == 'in_review'"], + ['`data.*`', "data.status == 'in_review'"], + ['legacy `${data.x}`', '${data.status === "in_review"}'], + ['legacy `${x}`', '${status === "in_review"}'], + ])('a %s predicate no longer discriminates — the caller fallback on BOTH rows', (_what, pred) => { + // A retired spelling is unbound: it faults, and the fault takes the + // caller's fallback, so the verdict is the same whichever row is bound. + // Both fallbacks are driven, so a constant that happened to equal one of + // them could not pass by accident. + expect(evalRowPredicate(pred, row)).toBe(false); + expect(evalRowPredicate(pred, other)).toBe(false); + expect(evalRowPredicate(pred, row, { fallback: true })).toBe(true); + expect(evalRowPredicate(pred, other, { fallback: true })).toBe(true); + }); + + it('the fault warning on the fast route names the unknown variable and carries the `record.` hint', () => { + evalRowPredicate('amount > 5', row, { label: 'row action "approve"' }); + const faults = faultReports(); + expect(faults).toHaveLength(1); + expect(faults[0]).toContain('Unknown variable: amount'); + expect(faults[0]).toContain("bound under 'record.'"); + expect(faults[0]).toContain('row action "approve"'); + }); + + it('… and on the fail-closed route (`warnOnError`) it names the variable (no `record.` hint there)', () => { + expect(evalRowPredicate('data.amount > 5', row, { warnOnError: true, label: 'grid' })).toBe(false); + const faults = faultReports(); + expect(faults).toHaveLength(1); + expect(faults[0]).toContain('Unknown variable: data'); + expect(faults[0]).toContain('(grid)'); + expect(faults[0]).not.toContain("bound under 'record.'"); + }); + + it('a legacy `${…}` string on a row surface lands in the SAME fallback / warning path', () => { + expect(evalRowPredicate('${amount > 5}', row, { warnOnError: true, label: 'kanban' })).toBe(false); + const faults = faultReports(); + expect(faults).toHaveLength(1); + expect(faults[0]).toContain('[legacy]'); + expect(faults[0]).toContain('amount is not defined'); + expect(faults[0]).toContain('(kanban)'); + }); + + it('the Phase-1 deprecation warning is gone', () => { evalRowPredicate("status == 'in_review'", row, { label: 'row action "approve"' }); - const msgs = deprecationWarnings(); - expect(msgs).toHaveLength(1); - expect(msgs[0]).toContain('record.status'); - expect(msgs[0]).toContain('objectui#5330'); - expect(msgs[0]).toContain('row action "approve"'); - }); - - it('warns on `data.*` and says the server binds no `data` at all', () => { - evalRowPredicate("data.status == 'in_review'", row); - const msgs = deprecationWarnings(); - expect(msgs).toHaveLength(1); - expect(msgs[0]).toContain('metadata-editing-form root'); + evalRowPredicate("data.status == 'in_review'", row, { label: 'row action "approve"' }); + expect(messages().filter((m) => m.includes('DEPRECATED spelling'))).toHaveLength(0); }); - it('stays silent for the canonical spelling', () => { - evalRowPredicate("record.status == 'in_review'", row); - expect(deprecationWarnings()).toHaveLength(0); + it('… and so is its runtime half of the module: only the offline detector is exported', () => { + expect('warnNonCanonicalRowSpelling' in core).toBe(false); + expect('resetRowPredicateCanonWarnings' in core).toBe(false); + expect(typeof core.detectNonCanonicalRowSpelling).toBe('function'); + expect(core.ROW_PREDICATE_CANONICAL_ROOT).toBe('record'); }); - it('warns ONCE per (label, predicate) — these run on every row of every frame', () => { - for (let i = 0; i < 5; i++) evalRowPredicate("status == 'in_review'", row, { label: 'grid' }); - expect(deprecationWarnings()).toHaveLength(1); - }); - - it('does NOT report a legacy `${…}`-dialect predicate, where `data.*` is the normal spelling', () => { - evalRowPredicate('${data.status === "in_review"}', row); - expect(deprecationWarnings()).toHaveLength(0); + it('`partitionRowsByPredicate`: `record.*` still partitions; a retired spelling excludes EVERY row', () => { + expect(partitionRowsByPredicate("record.status == 'in_review'", [row, other])).toEqual({ + eligible: [row], + skipped: 1, + }); + expect(partitionRowsByPredicate("status == 'in_review'", [row, other])).toEqual({ eligible: [], skipped: 2 }); + expect(partitionRowsByPredicate("data.status == 'in_review'", [row, other])).toEqual({ eligible: [], skipped: 2 }); + }); + + it("a host's own `data` is left standing: `data.*` reads the HOST object, never the row", () => { + const host = { data: { status: 'draft' } }; + // The same verdict on both rows — the row never enters it. + expect(evalRowPredicate("data.status == 'draft'", row, { scope: host })).toBe(true); + expect(evalRowPredicate("data.status == 'draft'", other, { scope: host })).toBe(true); + expect(evalRowPredicate("data.status == 'in_review'", row, { scope: host })).toBe(false); + // …while `record` is still the row, pinned over any host key of that name. + expect(evalRowPredicate("record.status == 'in_review'", row, { scope: { ...host, record: other } })).toBe(true); }); }); diff --git a/packages/core/src/evaluator/listConditional.ts b/packages/core/src/evaluator/listConditional.ts index e081912c21..62f0181b09 100644 --- a/packages/core/src/evaluator/listConditional.ts +++ b/packages/core/src/evaluator/listConditional.ts @@ -37,7 +37,6 @@ import { evalFieldPredicate, type FieldRulePredicate } from './fieldRules.js'; import { ExpressionEvaluator } from './ExpressionEvaluator.js'; import { toPredicateRecord, type FieldContainerLike } from '../utils/predicate-record.js'; -import { warnNonCanonicalRowSpelling } from './rowPredicateCanon.js'; /** * Syntax that only the legacy JS-dialect evaluator understands and that is NOT @@ -163,7 +162,9 @@ export interface RowPredicateOptions { fallback?: boolean; /** Extra top-level scope merged alongside the row — e.g. the global predicate * scope (`features` / `user` / `app`) a host shell provides. The row wins on - * collision: a `record` or `data` key here never shadows the row. */ + * collision: a `record` key here never shadows the row. Every OTHER key — + * a host's own `data` included — reaches the predicate as the host's own + * (objectui#5741: `data` no longer names the row on a record surface). */ scope?: Record; /** When true, log a one-time warning if a *present* predicate faults. */ warnOnError?: boolean; @@ -171,16 +172,16 @@ export interface RowPredicateOptions { label?: string; /** * This surface has **no row of its own** — bind NOTHING for the row instead - * of binding an empty one, so a `record` / `data` the HOST SCOPE carries - * survives to the predicate (objectui#4640). + * of binding an empty one, so a `record` the HOST SCOPE carries survives to + * the predicate (objectui#4640). * * The default (`false`) is this function's whole subject rule: the row is - * pinned over the scope, on both dialect paths, so `record` / `data` always - * name THIS row even when the host scope carries keys of those names + * pinned over the scope as `record`, on both dialect paths, so `record` + * always names THIS row even when the host scope carries a key of that name * (objectui#3796). That is right for every row surface — and exactly wrong * for a surface that has no row, because the pin then writes an EMPTY - * `record` / `data` over the host's real ones and every `record.*` predicate - * faults with "No such key". "This surface has no row of its own" and "this + * `record` over the host's real one and every `record.*` predicate faults + * with "No such key". "This surface has no row of its own" and "this * surface's row is empty" are different facts; only the latter is entitled to * shadow the scope. Same distinction, same words, as `usePredicateRecordContext` * in `@object-ui/react` (objectui#4075) — which is the BINDING half of this @@ -205,32 +206,37 @@ export interface RowPredicateOptions { /** * Evaluate a single boolean predicate against a row record on the canonical CEL - * engine (with a legacy-dialect fallback — see the module note). The row's - * fields are bound three ways so every authoring convention resolves: - * `record.status`, bare `status` (row-action shorthand), and `data.status`. + * engine (with a legacy-dialect fallback — see the module note). The row is + * bound ONE way: as `record.*` — **the canon** (maintainer ruling 2026-08-20 on + * objectui#5330, option B; Phase 2 executed by objectui#5741). * - * ⚠️ Those three are NOT peers, and this doc comment used to read as though - * they were. **The canon is `record.*`** (maintainer ruling 2026-08-20 on - * objectui#5330, option B); the other two are client tolerances in a - * deprecation window, kept because stored metadata carries them and warned - * about — from the CEL path below — by `warnNonCanonicalRowSpelling`. The - * server accepts `record.*` and NOTHING else: measured on - * `@objectstack/formula@17.1.0`, `buildScope({ record })` mounts exactly - * `['record']`, so a bare field faults `Unknown variable: status` there and a - * `data.*` predicate faults `Unknown variable: data`. See - * {@link ./rowPredicateCanon.ts} for the full measurement, for why `data.*` is - * the dangerous one (it is silently ACCEPTED by the server's authoring oracle - * and still binds nothing at runtime), and for why the deprecation is scoped to - * this runtime layer rather than declared platform-wide (`data` is the - * canonical root of a metadata-editing form — ADR-0089 D3). + * Until Phase 2 the row was also bound as bare fields (`status`, the row-action + * shorthand) and as `data.*`, and Phase 1 (PR #5737) warned once per + * non-canonical spelling. Both bindings and that warning are gone. A bare-field + * or `data.*` predicate on a record surface now FAULTS here exactly as it always + * did on the server — measured on `@objectstack/formula@17.1.0`, + * `buildScope({ record })` mounts exactly `['record']`, so a bare field faults + * `Unknown variable: status` and `data.*` faults `Unknown variable: data` — and + * takes this function's EXISTING fault policy: the caller's `fallback`, reported + * once by `warnEvalError` (when `warnOnError` is set) or by the canonical + * helper's own one-time warning, either of which names the unknown variable. + * Nothing detects a retired spelling on this path; it is simply unbound. The + * same holds on the legacy `${…}` path below — one scope shape per surface, + * both dialects — so `${data.x}` / `${x}` strings on a row surface fault there + * too, and land in the same fallback / warning. * - * ⛔ No spelling is removed from the binding, and none may be before a - * stored-metadata survey sizes the window — that is part of the same ruling. + * The retirement is scoped to the RUNTIME RECORD layer. `data` is the canonical + * root one layer over, in a metadata-editing form (ADR-0089 D3, + * `CANONICAL_ROOT_BY_LAYER` = `{ runtime: 'record', metadata: 'data' }`), and + * that layer evaluates through its own entry (`app-shell`'s metadata-admin + * `predicate.ts`), never through this one. See {@link ./rowPredicateCanon.ts} + * for the canon statement, the server measurement and the offline detector. * - * The optional `scope` (host predicate scope) is bound - * alongside so `features.*` / `user.*` predicates keep working — but the row is - * the subject: `record` and `data` always name THIS row, on both dialect paths, - * even when the host scope carries keys of those names (objectui#3796). + * The optional `scope` (host predicate scope) is bound alongside so + * `features.*` / `user.*` predicates keep working — but the row is the subject: + * `record` always names THIS row, on both dialect paths, even when the host + * scope carries a key of that name (objectui#3796). Every OTHER host key — + * a host's own `data` included — reaches the predicate as the host's own. * * A caller with no row at all passes {@link RowPredicateOptions.rowless}, and * then nothing is bound over the scope — see that option for why an absent row @@ -253,24 +259,26 @@ export function evalRowPredicate( const rowObj = opts.rowless ? {} : toPredicateRecord(row && typeof row === 'object' ? row : {}, opts.fields); - // Bare fields + `data.*` + `record.*` + the host scope, all top-level. + // `record.*` + the host scope, all top-level. The row is bound ONE way + // (objectui#5741, Phase 2 of the objectui#5330 canon): no bare-field spread + // and no `data` — a predicate spelled either way is simply unbound here and + // faults, on both dialect paths, exactly as it does on the server. // - // `data` AND `record` are pinned AFTER the spread, so a host scope that - // happens to carry either key is background and the ROW stays the subject of - // this function — on BOTH dialect paths. `data` always had that protection; - // `record` did not, and relied instead on each engine's own binding, which - // disagreed: the legacy evaluator re-pinned `record` (row won) while the CEL - // engine takes `extra` over its `record` binding (host scope won). Same - // function, same predicate text, opposite subjects — decided by whether the - // string happens to contain `===`/`${…}`, which no author is choosing - // deliberately (objectui#3796). Pinning here fixes both paths at the merge, + // `record` is pinned AFTER the spread, so a host scope that happens to carry + // that key is background and the ROW stays the subject of this function — on + // BOTH dialect paths. Before objectui#3796 it relied instead on each engine's + // own binding, which disagreed: the legacy evaluator re-pinned `record` (row + // won) while the CEL engine takes `extra` over its `record` binding (host + // scope won). Same function, same predicate text, opposite subjects — decided + // by whether the string happens to contain `===`/`${…}`, which no author is + // choosing deliberately. Pinning here fixes both paths at the merge, // independently of either engine's precedence. // // …UNLESS the caller has no row (`rowless`), in which case there is nothing - // to pin and the host scope keeps its own `record` / `data` — see the option. + // to pin and the host scope keeps its own `record` — see the option. const scope = opts.rowless ? { ...(opts.scope ?? {}) } - : { ...(opts.scope ?? {}), ...rowObj, data: rowObj, record: rowObj }; + : { ...(opts.scope ?? {}), record: rowObj }; // The predicate TEXT for diagnostics: a bare string is itself, an envelope is // its `source`. Reported separately from `source` above, which is `undefined` @@ -307,17 +315,14 @@ export function evalRowPredicate( } } - // CEL path — everything reaching here is CEL, which is what makes this the - // right place for the objectui#5330 spelling warning: in the legacy `${…}` - // dialect above, `data.*` is the NORMAL spelling, so reporting it there would - // be a false positive on every legacy predicate. `data` names THIS row unless - // the caller is `rowless` (then the host scope keeps its own — see the - // option), which is exactly the condition the detector needs. - if (predicateText !== '(expression)') { - warnNonCanonicalRowSpelling(predicateText, rowObj, !opts.rowless, opts.label); - } - - // CEL path. The fault-aware `evalCel` costs two evaluations (to tell a fault + // CEL path — everything reaching here is CEL. No spelling detector runs here + // (objectui#5741 removed the Phase-1 warning with the bindings): a retired + // bare-field / `data.*` spelling is unbound above and faults in the engine + // like any other unknown variable, and the fault report below is what names + // it. `detectNonCanonicalRowSpelling` stays exported for OFFLINE sweeps of + // authored metadata, not for this hot path. + // + // The fault-aware `evalCel` costs two evaluations (to tell a fault // from a genuine `false`), so only pay it when a caller wants the labelled // fail-closed warning — the formatting hot path takes the single-eval fast // route. Since #5149 the fast route is no longer silent either: the diff --git a/packages/core/src/evaluator/rowPredicateCanon.ts b/packages/core/src/evaluator/rowPredicateCanon.ts index 748af51ee1..2dcefdf73f 100644 --- a/packages/core/src/evaluator/rowPredicateCanon.ts +++ b/packages/core/src/evaluator/rowPredicateCanon.ts @@ -7,70 +7,76 @@ */ /** - * The row-predicate spelling CANON, and the Phase-1 deprecation warning for the - * two spellings that are not it (objectui#5330). + * The row-predicate spelling CANON: on a runtime record surface the row is bound + * as `record.*` and nothing else (objectui#5330, maintainer ruling 2026-08-20, + * option B; Phase 2 executed by objectui#5741). * - * ## The canon (maintainer ruling, 2026-08-20 — option B) + * ## The canon * * A row predicate — `visible` / `disabled` / `enabled` on an action renderer, a - * row scope, a `record:alert` — binds the row THREE ways today: canonical - * `record.status`, bare shorthand `status`, and legacy `data.status` - * (objectui#4075 / PR #4079 bound all four action renderers all three ways to - * restore consistency, deliberately without deciding which of them is - * CONTRACT). This module is where that decision now lives: - * - * > **The canon is `record.*`.** The bare shorthand and `data.*` are - * > client-side tolerances in a deprecation window, kept because stored - * > metadata carries them, warned about here, and removable only after a - * > stored-metadata survey sizes the window (⛔ no removal before the survey). - * - * It mirrors the objectstack#7917 option-② precedent for the identical shape (a - * renderer tolerance quietly becoming a second de-facto contract — AGENTS.md - * #0.1), whose objectui half is `utils/dashboard-filters.ts`' bare-string - * `options` shorthand: same three phases, same reason the warning is not - * decoration (ADR-0078 — nothing silently inert; a tolerance nothing ever - * reports can never be retired, because nothing would ever show that the last - * document carrying it is gone). - * - * ## The canon states the SERVER's accept set, not this client's - * - * The ruling made that the dev's first measurement, because a canon that only - * describes the renderer would be the very thing it exists to end. Measured - * against `@objectstack/formula@17.1.0` — the engine the server evaluates with, - * and the one `fieldRules.ts` already delegates to: + * row scope, a `record:alert`, a conditional-formatting `condition` — used to + * bind the row THREE ways: canonical `record.status`, bare shorthand `status`, + * and legacy `data.status` (objectui#4075 / PR #4079 bound all four action + * renderers all three ways to restore consistency, deliberately without + * deciding which of them was CONTRACT). The ruling decided it, in two phases: + * + * > **The canon is `record.*`.** Phase 1 (PR #5737) declared it and warned + * > once, in dev, on the two other spellings. Phase 2 (objectui#5741, ruled + * > 2026-09-02 and amended 2026-09-05) retired them: the bare shorthand and + * > `data.*` are no longer bound on runtime record surfaces, and the Phase-1 + * > warning went with them. No stored-metadata survey was run (「不考虑存量」); + * > the Phase-1 warning period was the notice. + * + * ## What a retired spelling does now: it FAULTS, as it always did on the server + * + * The canon states the SERVER's accept set. Measured against + * `@objectstack/formula@17.1.0` — the engine the server evaluates with, and the + * one `fieldRules.ts` delegates to: * * | spelling | server runtime (`buildScope` + `celEngine`) | server authoring oracle | * |---|---|---| * | `record.status` | ✅ `{ ok: true, value: true }` | ✅ accepted | * | bare `status` | ❌ `Unknown variable: status` | ❌ refused (`'status'`) | - * | `data.status` | ❌ `Unknown variable: data` | ⚠️ **silently accepted** | - * - * `buildScope({ record })` mounts exactly `['record']` — `data` is never bound - * and the row's fields are never flattened to top level. So **the server - * accepts `record.*` and nothing else**; both other spellings fault there. The - * three-way binding is a client tolerance with no server counterpart, which is - * precisely why it is the client's job to warn. - * - * ⚠️ The `data.*` row is the dangerous one and the reason this warning exists - * at all. `data` IS in `@objectstack/formula`'s `SCOPE_ROOTS`, so the server's - * bare-identifier oracle waves `data.status` through — that list is a - * deliberately generous "never faults" LINT BASELINE, not the runtime accept - * set. A `data.*` row predicate therefore passes every authoring gate the - * platform has and then binds nothing at runtime: it is not an error, it is a - * constant `false`, and a `visible` that is constantly false is a button that - * silently never appears. That is the #4075 fail-closed family's exact - * signature, and this client is the only layer positioned to catch it. - * - * ## `data.*` is DEPRECATED HERE, not everywhere — the canon is layer-scoped + * | `data.status` | ❌ `Unknown variable: data` | ⚠️ silently accepted | + * + * `buildScope({ record })` mounts exactly `['record']`. Since Phase 2 the client + * binds the same set (`listConditional.ts`' scope bag and `@object-ui/react`'s + * `usePredicateRecordContext`), so both retired spellings fault on the client + * with the server's verdict, and each surface applies its EXISTING fault policy: + * fail-closed `visible` on the throwing `useCondition` legs, the caller's + * `fallback` on `evalRowPredicate` / `partitionRowsByPredicate`, fail-soft on + * the non-throwing `useCondition` legs. There is no runtime detector, no + * "treat as absent" special case and no uniform override — a retired spelling + * is not a recognised-and-rejected thing, it is an unknown variable like any + * other, and the existing fault warnings are what name it. + * + * One consequence, stated so it is not read back as a bug: a host scope may + * legitimately carry its OWN `data` (a rowless dialog's, or app-shell's ambient + * `data: {}`), and it is left standing. `data.*` on a record surface then reads + * the host's object rather than the row — which is exactly what "no longer bound + * to the row" means, and, against an ambient `data: {}`, the constant-false + * signature the Phase-1 warning text already described for the server. + * + * ## `data.*` is retired HERE, not everywhere — the canon is layer-scoped * * `data` is the CANONICAL root one layer over, in a metadata-editing form (the * row under edit): objectstack's `CANONICAL_ROOT_BY_LAYER` reads * `{ runtime: 'record', metadata: 'data' }` (ADR-0089 D3), and objectui's own - * `app-shell` metadata-admin `SchemaForm` binds `{ data: row }` on purpose. So - * `data.*` is not a legacy alias to be deprecated platform-wide — it is a - * WRONG-LAYER paste on a runtime record surface, and only that is what this - * module reports. Stating the deprecation unqualified would contradict - * ADR-0089 D3 and break the metadata-editing layer's own contract. + * `app-shell` metadata-admin `SchemaForm` binds `{ data: row }` on purpose, + * through its own evaluator (`views/metadata-admin/predicate.ts`) — never + * through `evalRowPredicate` or `usePredicateRecordContext`. So `data.*` is not + * retired platform-wide: it is a WRONG-LAYER spelling on a runtime record + * surface, and that is the only thing the detector below reports. + * + * ## What is left in this module, and why + * + * {@link detectNonCanonicalRowSpelling} is the OFFLINE instrument: it classifies + * an authored predicate's spelling against a row without evaluating it, so a + * sweep over stored or in-repo metadata (the objectui#5738 corpus sweep, + * PR #5758's recipe) can find the documents that still need rewriting. It is + * exported for that purpose and nothing on the hot path calls it — the runtime + * warning half Phase 1 built on it (`warnNonCanonicalRowSpelling`, + * `resetRowPredicateCanonWarnings`) was removed with the bindings. * * ## Why the detection reuses the server's oracle instead of a regex * @@ -116,8 +122,8 @@ export type NonCanonicalRowSpelling = * predicate is already canonical (or is not this module's verdict to give). * * Deliberately conservative in both arms — every condition below can only - * REMOVE a finding, never invent one, because a false deprecation warning sends - * an author to rewrite a predicate that was correct: + * REMOVE a finding, never invent one, because a false finding sends an author + * to rewrite a predicate that was correct: * * - **Unparseable source** → `null`. Syntax is another gate's verdict. * - **Bare shorthand** is reported only when the undeclared identifier is an own @@ -135,10 +141,11 @@ export type NonCanonicalRowSpelling = * predicate manages both it is the one the author must fix to have anything * evaluate at all. * - * @param source The predicate's CEL text. Callers must have already - * routed legacy `${…}`-dialect strings elsewhere — in that - * dialect `data.*` is the NORMAL spelling, and reporting it - * here would be a false positive on every legacy predicate. + * @param source The predicate's CEL text. A legacy `${…}`-dialect string + * is not CEL and returns `null` here (unparseable); classify + * it by its own dialect's rules — on a runtime record surface + * it retired with the CEL spellings (objectui#5741), while on + * the schema/widget tier `data` is a different scope entirely. * @param row The row the predicate is bound against. * @param dataNamesRow Whether `data` is bound to that same row on this surface. */ @@ -179,73 +186,3 @@ export function detectNonCanonicalRowSpelling( return null; } - -/** - * Dev-mode gate, matching `utils/dashboard-filters.ts` and `actions/actionKeys.ts` - * — a deprecation warning that floods a production console is a warning that - * gets muted. - */ -const isDev = (): boolean => - (globalThis as { process?: { env?: Record } }).process?.env - ?.NODE_ENV !== 'production'; - -/** - * Warn-once memo, keyed by the `(label, predicate source)` pair — the same - * identity `warnEvalError` uses, and JSON-encoded for the same reason: the - * separator that boundary once used was a raw U+0000, which made the file - * carrying it binary to grep (objectstack#5450). Keying on the source alone - * would report the first surface carrying a shorthand `status` predicate and - * stay silent about every other one; the label is what sends the author to the - * right screen. - * - * Module scope, not per-call: these predicates are re-evaluated on every row of - * every render, so per-call state would warn once per frame — the flood the - * dedupe exists to prevent. - */ -const warnedSpellings = new Set(); - -/** Reset the row-predicate spelling warn-once memo. Exported for tests. */ -export function resetRowPredicateCanonWarnings(): void { - warnedSpellings.clear(); -} - -/** - * Report a non-canonical row-predicate spelling once, in dev. - * - * Phase 1 of the objectui#5330 window: the binding is UNCHANGED and every - * spelling still resolves — this only says so out loud, so the stored - * population stops growing and a later survey has something to count. It is a - * warning and deliberately not a refusal: turning it into one would move the - * accept/reject set, which this card is explicitly not entitled to do. - */ -export function warnNonCanonicalRowSpelling( - source: string, - row: Record | null | undefined, - dataNamesRow: boolean, - label?: string, -): void { - if (!isDev()) return; - const finding = detectNonCanonicalRowSpelling(source, row, dataNamesRow); - if (!finding) return; - - const key = JSON.stringify([label ?? '', source, finding.kind]); - if (warnedSpellings.has(key)) return; - warnedSpellings.add(key); - - const where = label ? ` (${label})` : ''; - const detail = - finding.kind === 'bare-shorthand' - ? `it references the bare field ${JSON.stringify(finding.identifier)}; ` + - `the server refuses that spelling outright ("Unknown variable: ${finding.identifier}")` - : 'it is rooted at `data.`, which is the metadata-editing-form root — on a ' + - 'record surface the server binds no `data` at all, so the predicate is a ' + - 'constant false there rather than an error'; - - console.warn( - `[object-ui] A row predicate${where} uses a DEPRECATED spelling: ` + - `${JSON.stringify(source)} — ${detail}. The canon is \`record.*\` ` + - `(objectui#5330, ruled 2026-08-20): write \`${finding.canonical}\`. ` + - 'This still evaluates here for now; the tolerance retires after a ' + - 'stored-metadata survey.', - ); -} 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 index e9f65d9976..1ccc62af62 100644 --- a/packages/plugin-detail/src/renderers/__tests__/record-alert.rowBinding.test.tsx +++ b/packages/plugin-detail/src/renderers/__tests__/record-alert.rowBinding.test.tsx @@ -6,31 +6,34 @@ * LICENSE file in the root directory of this source tree. * * ══════════════════════════════════════════════════════════════════════════ - * `record:alert` binds the row the THREE canonical ways (objectui#4807) + * `record:alert` binds the row through the SHARED helper (objectui#4807), and + * the helper binds `record.*` only (objectui#5741) * ══════════════════════════════════════════════════════════════════════════ * * `record:alert` was the last predicate face in the repo still handing - * `useCondition` a ROOT-ONLY bag (`{ record }`) instead of the shared + * `useCondition` a local 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: + * four generic action renderers and app-shell's `DeclaredActionsBar`; #4807 + * moved it onto the helper. objectui#5330 (maintainer, 2026-08-20) then ruled + * **B** — `record.*` is the canon, the row-action shorthand and legacy `data.*` + * deprecated — and objectui#5741 (Phase 2, ruled 2026-09-02 / amended + * 2026-09-05) retired them: the shared helper now binds `{ record: row }` and + * nothing else, no survey, no special case. What this file measures is what + * that looks like on THIS renderer, whose `useCondition` call is FAIL-SOFT: * - * • 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. + * • canon `record.*` gates the banner on the row — true on the matching row, + * false on the other. + * • row-action shorthand (`status == 'x'`) resolves NOTHING, so the evaluator + * throws `status is not defined` and this fail-soft site answers SHOWN on + * every row — the same verdict on both rows, i.e. the gate is not consulted. + * This is the pre-#4807 signature by design: it is the ruled cost of the + * retirement, and the console line is the notice. + * • legacy `data.*` does not throw: the ambient scope app-shell mounts + * (`providers/ExpressionProvider.tsx`) puts `data: {}` in the bag, and that + * `data` is the HOST's own, left standing, so `data.status` reads an + * undefined VALUE on the wrong object and the comparison is a constant + * `false` — the banner is OFF on every row, silently. Same retirement, + * opposite polarity, and no fault to report because nothing faulted. * * ── Why groups B and C mount different shapes ────────────────────────────── * @@ -161,10 +164,11 @@ describe('#4807 group A — controls: the harness paints the banner, and this ch }); }); -describe('#4807 group B — all THREE bindings resolve on this renderer own gate', () => { +describe('#4807 group B — only the canon binds on this renderer own gate (objectui#5741)', () => { // 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. + // author's gate was never consulted, whichever way it landed — and since + // objectui#5741 that pair is the RULED outcome for the two retired spellings. it('canon `record.*` — the spelling objectui#5330 ruled canonical', () => { expect(verdicts(alertNode(CANON, NODE_GATE_OPEN))).toEqual({ @@ -173,42 +177,49 @@ describe('#4807 group B — all THREE bindings resolve on this renderer own gate }); }); - 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. + it('row-action shorthand `status` — retired by objectui#5741: SHOWN on both rows (fail-soft)', () => { + // `status` is unbound, the evaluator throws, and this fail-soft call site + // turns the throw into SHOWN — on the matching row and the other alike. expect(verdicts(alertNode(SHORTHAND, NODE_GATE_OPEN))).toEqual({ onMatchingRow: true, - onOtherRow: false, + onOtherRow: true, }); }); - 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. + it('legacy `data.*` — retired by objectui#5741: OFF on both rows (the host `data: {}` answers)', () => { + // No throw: the ambient `data: {}` from app-shell is the host's own and is + // left standing, so the comparison is constantly false and the banner never + // appears — on either row, and with nothing faulting to report. expect(verdicts(alertNode(LEGACY, NODE_GATE_OPEN))).toEqual({ - onMatchingRow: true, + onMatchingRow: false, 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) => + it('the row wins over an ambient `record`; a host `data` is left standing (objectui#5741)', () => { + // `usePredicateRecordContext` binds `{ record: row }` OVER the ambient + // scope, so a host-supplied `record` never shadows the row — pinned on the + // user-visible verdict so the precedence cannot regress silently. A host + // `data`, by contrast, is the host's own now: `data.*` reads IT, on every + // row, which is what "no longer bound to the row" looks like from here. + const hostScope = { ...APP_SCOPE, record: DONE, data: IN_REVIEW }; + const withHostScope = (visible: string, record: Record) => render( - + , ); - expect(bannerInDocument(withHostScope(IN_REVIEW))).toBe(true); + expect(bannerInDocument(withHostScope(CANON, IN_REVIEW))).toBe(true); + cleanup(); + expect(bannerInDocument(withHostScope(CANON, DONE))).toBe(false); + cleanup(); + // `data.status == 'in_review'` against the HOST's `data` (IN_REVIEW): true + // whichever row is bound — the row never enters this verdict. + expect(bannerInDocument(withHostScope(LEGACY, IN_REVIEW))).toBe(true); cleanup(); - expect(bannerInDocument(withHostScope(DONE))).toBe(false); + expect(bannerInDocument(withHostScope(LEGACY, DONE))).toBe(true); }); }); @@ -220,9 +231,11 @@ describe('#4807 group C — end to end, on the plain authored node', () => { 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 }); + it('row-action shorthand no longer gates the banner (objectui#5741) — SHOWN on both rows', () => { + // The pair objectui#4807 fixed is back by ruling: `status` is unbound on + // every runtime record surface, so an author-declared gate spelled this way + // never hides the banner, and the console line naming the variable is the + // notice. The canon case above is the control that the gate still works. + expect(verdicts(alertNode(SHORTHAND))).toEqual({ onMatchingRow: true, onOtherRow: true }); }); }); 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 e3a893b8b8..7c4673937e 100644 --- a/packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx +++ b/packages/plugin-grid/src/__tests__/predicate-surface-parity.test.tsx @@ -188,9 +188,21 @@ const CASES: Case[] = [ record: { owner: { id: 'U2', name: 'Grace' } }, expected: false, }, - // Bare-field and `data.*` shorthands. - { what: 'bare field shorthand', name: 'par_bare', visible: 'f_status == "open"', expected: true }, - { what: '`data.*` binding', name: 'par_data', visible: 'data.f_status == "open"', expected: true }, + // The bare-field and `data.*` spellings retired in objectui#5741 (Phase 2 of + // the objectui#5330 canon): unbound on every record surface, they fault and + // fail CLOSED on all three — the parity claim now holds for the retirement. + { + what: 'bare field shorthand — retired (objectui#5741), fails closed on all three', + name: 'par_bare', + visible: 'f_status == "open"', + expected: false, + }, + { + what: '`data.*` — retired (objectui#5741), fails closed on all three', + name: 'par_data', + visible: 'data.f_status == "open"', + expected: false, + }, // The legacy fallback lives inside the shared entry, so parity covers it too. { what: 'legacy `${…}` template — true', diff --git a/packages/plugin-kanban/src/cardPredicateScope.test.tsx b/packages/plugin-kanban/src/cardPredicateScope.test.tsx index 5a81319b96..5e311a603f 100644 --- a/packages/plugin-kanban/src/cardPredicateScope.test.tsx +++ b/packages/plugin-kanban/src/cardPredicateScope.test.tsx @@ -76,12 +76,18 @@ describe('kanban card conditional formatting · host predicate scope (ADR-0058)' expect(findByBg(container, HOT_BG)).toBeUndefined(); }); - it('bare-field conditions keep working without any provider (row spread)', () => { + it('a bare-field condition no longer binds the card (objectui#5741) — no style; `record.*` does', () => { + // The bare shorthand retired with Phase 2 of the objectui#5330 canon: it is + // unbound, faults, and conditional formatting's existing policy is fail-soft + // to "no style". The canonical spelling on the same card is the control + // that proves the rule was consulted at all. const bareRules = [{ condition: 'id == "c1"', style: { backgroundColor: HOT_BG } }] as any; - const { container } = render( - , - ); - expect(findByBg(container, HOT_BG)).toBeTruthy(); + const bare = render(); + expect(findByBg(bare.container, HOT_BG)).toBeUndefined(); + cleanup(); + const canonRules = [{ condition: 'record.id == "c1"', style: { backgroundColor: HOT_BG } }] as any; + const canon = render(); + expect(findByBg(canon.container, HOT_BG)).toBeTruthy(); }); }); diff --git a/packages/plugin-list/src/__tests__/ListView.test.tsx b/packages/plugin-list/src/__tests__/ListView.test.tsx index 506fb77206..a4861d311c 100644 --- a/packages/plugin-list/src/__tests__/ListView.test.tsx +++ b/packages/plugin-list/src/__tests__/ListView.test.tsx @@ -2277,11 +2277,20 @@ describe('ListView', () => { it('should evaluate spec format with condition and style', () => { const result = evaluateConditionalFormatting( { status: 'active', amount: 200 }, - [{ condition: '${data.status === "active"}', style: { backgroundColor: '#e0ffe0', color: '#0a0' } }] as any, + [{ condition: '${record.status === "active"}', style: { backgroundColor: '#e0ffe0', color: '#0a0' } }] as any, ); expect(result).toEqual({ backgroundColor: '#e0ffe0', color: '#0a0' }); }); + it('a legacy `${data.x}` condition no longer binds the row (objectui#5741) — no style on either row', () => { + // One scope shape per surface for both dialects: `data` is unbound on the + // legacy path too, so the rule faults and fails soft to "no style" + // whichever row it meets. + const rules = [{ condition: '${data.status === "active"}', style: { backgroundColor: '#e0ffe0' } }] as any; + expect(evaluateConditionalFormatting({ status: 'active', amount: 200 }, rules)).toEqual({}); + expect(evaluateConditionalFormatting({ status: 'closed', amount: 200 }, rules)).toEqual({}); + }); + it('binds a host predicate scope alongside the row (grid/kanban parity)', () => { const rules = [ { condition: 'features.highlight && record.status == "active"', style: { backgroundColor: '#fee' } }, diff --git a/packages/react/src/hooks/__tests__/useCondition.canonSpelling.test.tsx b/packages/react/src/hooks/__tests__/useCondition.canonSpelling.test.tsx index 27e4a6d3d9..35ceea69e6 100644 --- a/packages/react/src/hooks/__tests__/useCondition.canonSpelling.test.tsx +++ b/packages/react/src/hooks/__tests__/useCondition.canonSpelling.test.tsx @@ -7,88 +7,118 @@ */ /** - * objectui#5330 — the row-predicate canon on the `useCondition` TIER. + * objectui#5741 — Phase 2 of the row-predicate canon (objectui#5330) on the + * `useCondition` TIER. * * `packages/core`'s `rowPredicateCanon.test.ts` pins the detector and the * `evalRowPredicate` tier. This file exists because the two tiers are genuinely - * separate evaluation entries and a warning wired into only one of them misses - * the surfaces this card is actually about: the four generic action renderers - * and `record:alert` go through `usePredicateRecordContext` + `useCondition`, - * NOT through `evalRowPredicate`. Pinning only the core tier would have left - * them silent while reading as covered. + * separate evaluation entries: the four generic action renderers, `record:alert` + * and `DeclaredActionsBar` go through `usePredicateRecordContext` + + * `useCondition`, NOT through `evalRowPredicate`. Pinning only the core tier + * would have left them unmeasured while reading as covered. * - * The legacy-dialect case is the one that would make this warning unshippable - * if it were wrong: `useCondition`'s own documented example is - * `'${data.status === "active"}'`, where `data.*` is the CORRECT spelling. A - * detector that reported it would fire on essentially every legacy predicate in - * the wild. + * What this tier does with a retired spelling is this tier's EXISTING fault + * policy, nothing new: the bag binds `record` and nothing else, so a bare field + * or `data.*` — and a legacy `${data.x}` / `${x}` string, one bag for both + * dialects — is an unknown variable. The throwing leg (`throwOnError`) hides and + * reports once; the non-throwing leg fails soft to `true`. Both legs are pinned + * on BOTH a matching and a non-matching row, because "the same verdict on both" + * is what "no longer bound" looks like from outside. */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; import { renderHook } from '@testing-library/react'; -import { resetRowPredicateCanonWarnings } from '@object-ui/core'; -import { useCondition, usePredicateRecordContext } from '../useExpression'; +import { createElement, type ReactNode } from 'react'; +import { useCondition, usePredicateRecordContext, PredicateScopeProvider } from '../useExpression'; const row = { status: 'in_review', amount: 10 }; +const other = { status: 'draft', amount: 1 }; + +type Options = { throwOnError?: boolean; label?: string }; /** Drive the real pairing the action renderers use: bind the row, then evaluate. */ -const evaluate = (pred: unknown, record: unknown = row, label?: string) => +const evaluate = (pred: unknown, record: unknown = row, options?: Options) => renderHook(() => { const ctx = usePredicateRecordContext(record); - return useCondition(pred as never, ctx, label ? { label } : undefined); + return useCondition(pred as never, ctx, options); }).result.current; -describe('[#5330] useCondition tier — the canon and its deprecation warning', () => { - let warn: ReturnType; +describe('[#5741] useCondition tier — the row is bound as `record.*` only', () => { + let warn: MockInstance; beforeEach(() => { - resetRowPredicateCanonWarnings(); warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); }); afterEach(() => warn.mockRestore()); - const deprecationWarnings = (): string[] => - warn.mock.calls.map((c: unknown[]) => String(c[0])).filter((m: string) => m.includes('DEPRECATED spelling')); + const messages = (): string[] => warn.mock.calls.map((c) => String(c[0])); - it('still resolves ALL THREE spellings — no removal before the survey', () => { - expect(evaluate("record.status == 'in_review'")).toBe(true); - expect(evaluate("status == 'in_review'")).toBe(true); - expect(evaluate("data.status == 'in_review'")).toBe(true); + it('binds the row as `{ record }` — no spread, no `data`', () => { + const { result } = renderHook(() => usePredicateRecordContext(row)); + expect(result.current).toEqual({ record: row }); + expect(result.current.record).toBe(row); + expect('data' in result.current).toBe(false); + expect('status' in result.current).toBe(false); }); - it('is not vacuous — the same three spellings are false on a non-matching row', () => { - const other = { status: 'draft', amount: 1 }; + it('`record.*` still discriminates, on both legs, silently', () => { + expect(evaluate("record.status == 'in_review'")).toBe(true); expect(evaluate("record.status == 'in_review'", other)).toBe(false); - expect(evaluate("status == 'in_review'", other)).toBe(false); - expect(evaluate("data.status == 'in_review'", other)).toBe(false); + expect(evaluate("record.status == 'in_review'", row, { throwOnError: true })).toBe(true); + expect(evaluate("record.status == 'in_review'", other, { throwOnError: true })).toBe(false); + expect(messages()).toHaveLength(0); }); - it('warns once on the bare shorthand, naming the canonical rewrite', () => { - evaluate("status == 'in_review'", row, 'action:button "approve"'); - const msgs = deprecationWarnings(); - expect(msgs).toHaveLength(1); - expect(msgs[0]).toContain('record.status'); - expect(msgs[0]).toContain('action:button "approve"'); - }); + describe.each([ + ['bare shorthand', "status == 'in_review'", 'status'], + ['`data.*`', "data.status == 'in_review'", 'data'], + ['legacy `${data.x}`', '${data.status === "in_review"}', 'data'], + ['legacy `${x}`', '${status === "in_review"}', 'status'], + ])('a %s predicate no longer discriminates', (_what, pred, variable) => { + it('throwing leg (`throwOnError`): hidden on BOTH rows, reported once, naming the variable', () => { + // The label carries the predicate so the hook's warn-once key (label, + // source) is unique per case — the registry is module-global. + const label = `action "approve" (visible) [${pred}]`; + expect(evaluate(pred, row, { throwOnError: true, label })).toBe(false); + expect(evaluate(pred, other, { throwOnError: true, label })).toBe(false); + const reports = messages().filter((m) => m.includes('was hidden/disabled: its predicate threw')); + expect(reports).toHaveLength(1); + expect(reports[0]).toContain(`${variable} is not defined`); + expect(reports[0]).toContain(label); + }); - it('warns on a `data.`-rooted CEL predicate', () => { - evaluate("data.status == 'in_review'"); - expect(deprecationWarnings()).toHaveLength(1); - }); + it('non-throwing leg: fail-soft `true` on BOTH rows', () => { + expect(evaluate(pred, row)).toBe(true); + expect(evaluate(pred, other)).toBe(true); + }); - it('stays silent on the canonical spelling', () => { - evaluate("record.status == 'in_review'"); - expect(deprecationWarnings()).toHaveLength(0); + it('and the Phase-1 deprecation warning is gone', () => { + evaluate(pred, row); + evaluate(pred, row, { throwOnError: true, label: `gone [${pred}]` }); + expect(messages().filter((m) => m.includes('DEPRECATED spelling'))).toHaveLength(0); + }); }); - it('stays silent on a legacy `${…}` predicate, where `data.*` is CORRECT', () => { - evaluate('${data.status === "in_review"}'); - expect(deprecationWarnings()).toHaveLength(0); + it("a host's own ambient `data` is left standing: `data.*` reads the HOST object — constant, and silent", () => { + // app-shell's `ExpressionProvider` mounts `data: {}`; a record page under it + // hands this tier exactly this bag. Not a fault — `data.status` is an + // undefined VALUE on the host's object — so no leg reports it, and the row + // never enters the verdict. Pinned so the constant-false is a measured + // consequence of the ruling, not a surprise. + const wrapper = ({ children }: { children: ReactNode }) => + createElement(PredicateScopeProvider, { scope: { data: {} }, children }); + const under = (pred: string, record: unknown, options?: Options) => + renderHook(() => useCondition(pred as never, usePredicateRecordContext(record), options), { wrapper }) + .result.current; + expect(under("data.status == 'in_review'", row)).toBe(false); + expect(under("data.status == 'in_review'", other)).toBe(false); + expect(under("data.status == 'in_review'", row, { throwOnError: true })).toBe(false); + // …while the row still reaches `record`, which this bag pins over the host. + expect(under("record.status == 'in_review'", row)).toBe(true); + expect(messages()).toHaveLength(0); }); - it('stays silent when there is no row bound (a non-row `useCondition` call)', () => { - // `usePredicateRecordContext(null)` binds NOTHING, so `record`/`data` are - // absent and this is not a row predicate at all. - renderHook(() => useCondition("status == 'in_review'" as never, usePredicateRecordContext(null))); - expect(deprecationWarnings()).toHaveLength(0); + it('binds NOTHING when there is no row (a non-row `useCondition` call)', () => { + const { result } = renderHook(() => usePredicateRecordContext(null)); + expect(result.current).toEqual({}); }); }); diff --git a/packages/react/src/hooks/__tests__/useExpression.test.ts b/packages/react/src/hooks/__tests__/useExpression.test.ts index 348edf4611..cdd4e2ffa0 100644 --- a/packages/react/src/hooks/__tests__/useExpression.test.ts +++ b/packages/react/src/hooks/__tests__/useExpression.test.ts @@ -149,9 +149,13 @@ describe('useRowPredicate (canonical CEL row predicate — issue #1584)', () => expect(renderHook(() => useRowPredicate(undefined, { a: 1 }, { fallback: false })).result.current).toBe(false); }); - it('evaluates a CEL predicate over the row (record.* and bare)', () => { + it('evaluates a CEL predicate over the row (`record.*`; the bare shorthand retired in objectui#5741)', () => { expect(renderHook(() => useRowPredicate("record.status == 'active'", { status: 'active' })).result.current).toBe(true); - expect(renderHook(() => useRowPredicate("status == 'active'", { status: 'closed' })).result.current).toBe(false); + expect(renderHook(() => useRowPredicate("record.status == 'active'", { status: 'closed' })).result.current).toBe(false); + // A bare field is unbound: it faults into this hook's fallback (default + // `true`) on EITHER row, so it no longer discriminates. + expect(renderHook(() => useRowPredicate("status == 'active'", { status: 'closed' })).result.current).toBe(true); + expect(renderHook(() => useRowPredicate("status == 'active'", { status: 'active' }, { fallback: false })).result.current).toBe(false); }); it('supports the CEL `in` operator (legacy engine could not)', () => { @@ -182,10 +186,11 @@ describe('useRowPredicate (canonical CEL row predicate — issue #1584)', () => * `usePredicateRecordContext` — the no-row half of the binding rule * (objectui#4075 / #4080). * - * The row-PRESENT half (all three spellings resolving) is pinned behaviorally - * where the rule is consumed: `action-record-predicate-root.test.tsx` for the - * four generic renderers, `DeclaredActionsBar.test.tsx` for the bar. What is - * pinned HERE is the return shape itself, one level below any renderer — the + * The row-PRESENT half (`record.*` resolving; the bare and `data.*` spellings + * retired in objectui#5741 and faulting) is pinned behaviorally where the rule + * is consumed: `action-record-predicate-root.test.tsx` for the four generic + * renderers, `DeclaredActionsBar.test.tsx` for the bar. What is pinned HERE is + * the return shape itself, one level below any renderer — the * distinction between "this surface has no row" and "this surface's row is * empty", which is the single point on which the helper and the inline copies * it replaced ever differed. Its consumers can only fail it through a rendered @@ -193,7 +198,15 @@ describe('useRowPredicate (canonical CEL row predicate — issue #1584)', () => * the helper) would not be caught by them at all. */ describe('usePredicateRecordContext — no row binds NOTHING (objectui#4075)', () => { - it('returns an EMPTY bag for no row — not `{ record: {}, data: {} }`', () => { + it('with a row, binds `{ record: row }` and nothing else (objectui#5741)', () => { + const row = { status: 'pending' }; + const { result } = renderHook(() => usePredicateRecordContext(row)); + expect(result.current).toEqual({ record: row }); + expect('data' in result.current).toBe(false); + expect('status' in result.current).toBe(false); + }); + + it('returns an EMPTY bag for no row — not `{ record: {} }`', () => { for (const noRow of [null, undefined, 'a string', 42, [1, 2]]) { const { result } = renderHook(() => usePredicateRecordContext(noRow)); expect(result.current, String(noRow)).toEqual({}); diff --git a/packages/react/src/hooks/useExpression.ts b/packages/react/src/hooks/useExpression.ts index 3f7b919862..320a8da7c4 100644 --- a/packages/react/src/hooks/useExpression.ts +++ b/packages/react/src/hooks/useExpression.ts @@ -7,12 +7,7 @@ */ import { createContext, createElement, useContext, useMemo, type ReactNode } from 'react'; -import { - ExpressionEvaluator, - evalRowPredicate, - isLegacyDialectSource, - warnNonCanonicalRowSpelling, -} from '@object-ui/core'; +import { ExpressionEvaluator, evalRowPredicate } from '@object-ui/core'; /** * Global predicate scope — populated by host shells (e.g. app-shell's @@ -91,56 +86,48 @@ 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. + * The row is bound ONE way: `record.status` — the canon (objectui#5330, ruled + * 2026-08-20; Phase 2 executed by objectui#5741). This is not a dialect choice; + * 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) + * ## The two retired spellings (objectui#5741) * - * `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. + * Until Phase 2 this bag also carried the row spread flat (bare `status`, the + * row-action shorthand) and as `data` (legacy), and `useCondition` below warned + * once per non-canonical spelling (Phase 1, PR #5737). Both bindings and the + * warning are gone. A bare-field or `data.*` predicate against this bag now + * faults exactly as it always did on the server (`buildScope({ record })` + * mounts exactly `['record']`: `Unknown variable: status` / `Unknown variable: + * data`), and each `useCondition` leg applies its EXISTING fault policy — the + * throwing legs (`throwOnError`: `action:button` / `action:menu` `visible`, + * `DeclaredActionsBar`'s `visible`) hide and report `was hidden/disabled: its + * predicate threw`, naming the variable; the non-throwing legs fail soft to + * `true`. The same holds for a legacy `${data.x}` / `${x}` string: one bag + * shape, both dialects. Nothing detects a retired spelling here; it is simply + * unbound. `@object-ui/core`'s `evaluator/rowPredicateCanon.ts` carries the + * canon statement, the server measurement, the layer scoping (`data` stays + * canonical one layer over, in a metadata-editing form — ADR-0089 D3) and the + * offline detector. * - * 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. + * ## Why a helper and not "just `{ record }`" (objectui#4075 / #4080) * - * ## The canon is `record.*` (objectui#5330, ruled 2026-08-20) + * Because the rule has two halves and every predicate face has to get both: + * the SHAPE — `record` is the row's one name, so `record.viewer.can_act`, what + * every declared action on framework's `sys_approval_request` gates on + * (framework#3310 / #3424), reaches the row — and the NO-ROW case below. + * `DeclaredActionsBar` and the four generic action renderers once carried + * inline copies of this bag and drifted (objectui#4077 / #4079); one named + * helper is what keeps a fifth copy from drifting again. * - * All three spellings are bound here, and that is unchanged — but they are NOT - * peers. `record.*` is the CONTRACT; the bare shorthand and `data.*` are - * tolerances in a deprecation window, kept because stored metadata carries - * them, reported once by `warnNonCanonicalRowSpelling` from `useCondition` - * below, and removable only after a stored-metadata survey sizes the window - * (⛔ no removal before the survey — part of the same ruling). - * - * The canon states the SERVER's accept set, which is narrower than this bag: - * measured on `@objectstack/formula@17.1.0`, `buildScope({ record })` mounts - * exactly `['record']`, so of the three spellings this helper binds, only - * `record.*` evaluates server-side — a bare field faults `Unknown variable: - * status` and `data.*` faults `Unknown variable: data`. This three-way bag has - * no server counterpart at all, which is exactly why the warning belongs on - * this side. `@object-ui/core`'s `evaluator/rowPredicateCanon.ts` carries the - * full measurement and the layer scoping (`data` stays canonical one layer - * over, in a metadata-editing form — ADR-0089 D3). - * - * `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`. + * Note the direction of the merge: `useCondition` evaluates on + * `new ExpressionEvaluator({ ...scope, ...context })`, so this bag's `record` + * shadows an ambient `record` a host put in the predicate scope — the row is + * the subject — while every OTHER ambient key (`features`, `user`, a host's own + * `data`) survives to the predicate as the host's own. * * This is the BINDING rule only. It deliberately does not touch the evaluation * entry — `useCondition` / `toPredicateInput` / `hasDeclaredVisibilityGate` @@ -152,10 +139,10 @@ export { toPredicateInput } from '@object-ui/core'; * ## 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 + * `{ record: {} }`. 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 @@ -166,8 +153,7 @@ export { toPredicateInput } from '@object-ui/core'; 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 }; + return { record: record as Record }; }, [record]); } @@ -218,40 +204,11 @@ export function useCondition( // We evaluate directly without caching the evaluator to avoid issues with context changes return useMemo( () => { - // objectui#5330 Phase 1 — report a deprecated row-predicate spelling on - // the `useCondition` tier. This is the BINDING half's evaluation entry: - // `usePredicateRecordContext` sees the row but never the predicate text, - // so it structurally cannot detect a spelling, and this is the first - // point where the two meet. - // - // Two guards, both of which can only remove a report: - // - the bag must carry the `usePredicateRecordContext` signature, `data` - // and `record` being the SAME object by identity. A host scope that - // merely happens to carry a `data` key cannot satisfy that, so a - // non-row `useCondition` call is never judged as a row predicate. - // - the source must be CEL. In this tier's legacy `${…}` dialect - // (`'${data.status === "active"}'` — this hook's own doc example) - // `data.*` is the NORMAL spelling, and reporting it would be a false - // positive on every legacy predicate. - const bag = context as { record?: unknown; data?: unknown }; - const boundRow = - bag.record != null && typeof bag.record === 'object' && bag.record === bag.data - ? (bag.record as Record) - : undefined; - if (boundRow !== undefined) { - const celSource = - typeof condition === 'string' - ? isLegacyDialectSource(condition) - ? undefined - : condition - : condition && typeof condition === 'object' && condition.dialect === 'cel' - ? condition.source - : undefined; - if (typeof celSource === 'string') { - warnNonCanonicalRowSpelling(celSource, boundRow, true, options?.label); - } - } - + // No row-spelling detector on this tier (objectui#5741 removed the + // Phase-1 warning with the bindings): a retired bare-field / `data.*` + // spelling against a `usePredicateRecordContext` bag is simply unbound + // and faults like any other unknown variable, and each leg's existing + // fault report below is what names it. const evaluator = new ExpressionEvaluator({ ...scope, ...context }); if (options?.throwOnError) { // Fail-closed: a predicate that can't be evaluated hides/disables @@ -293,11 +250,11 @@ export function useCondition( * routes to `@object-ui/core`'s `evalRowPredicate`: a bare string is CEL (the * spec contract for `ActionSchema.visible`), a `{ dialect: 'cel', source }` * envelope is always CEL, and only a legacy-dialect string falls back to the - * old engine (with a deprecation warning). The row is bound as `record.*` — the - * CANON (objectui#5330) — and, for stored metadata only, as bare fields and - * `data.*`, both of which now warn once; the ambient predicate scope - * (`features` / `user` / …) is merged alongside so deployment-level gates keep - * resolving. + * old engine (with a deprecation warning). The row is bound as `record.*` only + * — the canon (objectui#5330; the bare-field and `data.*` spellings retired in + * objectui#5741 and fault here like any unknown variable); the ambient + * predicate scope (`features` / `user` / …) is merged alongside so + * deployment-level gates keep resolving. * * @param pred The raw predicate: `boolean` (returned as-is), a CEL string, * an `{ dialect, source }` envelope, or `null`/`undefined`/`''`.