From df9de783675bbf3b27aa90124f25086e7cb7e00c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 05:36:40 +0000 Subject: [PATCH] fix(react,app-shell,components): choose the visibility-fault advice by scope tier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `formatUnresolvableVisibilityMessage` closed every report with the NODE tier's roots — `record`, `current_user`, `page.`. Since the app-shell chrome gate was wired onto the same reporter, that paragraph also printed for an evaluator built from `{ current_user, user, ctx: { user }, os: { user }, app, data, features }`, which binds neither `record` nor `page`. A nav author whose `visible` predicate faulted was sent to check two roots that cannot exist at their tier, while the roots that do exist went unnamed. Both exported functions now take an optional `PredicateScopeTier` selecting the closing paragraph, and all three in-repo call sites state their tier rather than inherit the default. Each tier's root set is derived from the code that builds the bag: `SchemaRenderer` and the `page:tabs` item gate bind the three roots the spec declares for a page component, `ExpressionProvider` binds the ADR-0068 identity aliases plus `app` and `features`. The concrete root names are deliberately kept at both tiers. Generalising the copy would have made one paragraph true everywhere and useful nowhere: an author who mistyped a root needs to know which roots exist at their tier. No verdict moves, on any surface. Everything above the advice paragraph — the prefix, the node, the gate key, the predicate source, the engine's reason and the "gate did NOT bite" sentence — is unchanged, and so is the objectui#6038 dedupe key. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .changeset/6487-visibility-advice-per-tier.md | 45 +++ .../src/providers/ExpressionProvider.tsx | 6 + ...ionProvider.visibleFaultDiagnostic.test.ts | 69 +++++ ...e-tabs-visible-when-fault-warning.test.tsx | 42 +++ .../src/renderers/layout/containers.tsx | 15 +- packages/react/src/SchemaRenderer.tsx | 22 +- ...Renderer.visibleWhenRecordBinding.test.tsx | 53 ++++ packages/react/src/index.ts | 5 + .../visibilityDiagnostic.adviceTier.test.ts | 282 ++++++++++++++++++ .../react/src/utils/visibilityDiagnostic.ts | 101 ++++++- 10 files changed, 633 insertions(+), 7 deletions(-) create mode 100644 .changeset/6487-visibility-advice-per-tier.md create mode 100644 packages/react/src/utils/visibilityDiagnostic.adviceTier.test.ts diff --git a/.changeset/6487-visibility-advice-per-tier.md b/.changeset/6487-visibility-advice-per-tier.md new file mode 100644 index 000000000..7750f26b9 --- /dev/null +++ b/.changeset/6487-visibility-advice-per-tier.md @@ -0,0 +1,45 @@ +--- +'@object-ui/react': minor +'@object-ui/app-shell': minor +'@object-ui/components': minor +--- + +**The unresolvable-visibility-predicate report now names the roots of the tier the +predicate was actually evaluated against** (objectui#6487). An app-shell author +whose nav, area or field `visible` faulted was told to check `record` and +`page.` — two roots that tier does not bind at all. + +`formatUnresolvableVisibilityMessage` and `reportUnresolvableVisibilityPredicate` +(both exported from `@object-ui/react`) take a new **optional sixth argument**, a +`PredicateScopeTier` — also exported — selecting the closing advice paragraph. +Everything above that paragraph is unchanged on every surface, and so is every +verdict: this is diagnostics copy only. + +**The published signature grew; nothing existing breaks.** The argument defaults +to `'page-component'`, so a five-argument call keeps printing the bytes it +printed before. All three in-repo call sites pass their tier explicitly rather +than lean on that default. + +Each tier's root set was derived from the code that builds the bag, not from the +prose that described it: + +- **`'page-component'`** — `SchemaRenderer`'s node gate and `page:tabs` item + predicates. Both bind `record`, `current_user` and `page.` (the roots + `@objectstack/spec`'s `ui/page.zod.ts` declares for the tier). Its paragraph is + byte-for-byte what it was. +- **`'app-shell'`** — the chrome gate `ExpressionProvider.evaluateVisibility` + runs, wired onto this reporter by objectui#6443. Its evaluator is built from + `{ current_user, user, ctx: { user }, os: { user }, app, data, features }`, so + the line now names `current_user` with its three ADR-0068 alias spellings, + `app`, and `features` — the deployment-flag root that provider documents for + exactly this kind of predicate — and states outright that `record` and + `page.` do not exist there. + +**Why not generalise the copy instead.** Dropping the concrete root names would +have made one paragraph true everywhere at the cost of making it useful nowhere: +an author who mistyped a root needs to know which roots exist *at their tier*, +which is the whole reason the paragraph is read. + +`data` is bound at the app-shell tier but is deliberately not advertised there — +every mount of `ExpressionProvider` in this repo passes `data={{}}` or omits it, +so naming it would point an author at a root that answers nothing. diff --git a/packages/app-shell/src/providers/ExpressionProvider.tsx b/packages/app-shell/src/providers/ExpressionProvider.tsx index 1c18358ca..fa5e1578e 100644 --- a/packages/app-shell/src/providers/ExpressionProvider.tsx +++ b/packages/app-shell/src/providers/ExpressionProvider.tsx @@ -201,6 +201,12 @@ export function evaluateVisibility( 'visible', expression, reason, + // objectui#6487. Until this argument existed the line closed with the + // NODE tier's advice, telling an author whose nav predicate faulted to + // check `record` and `page.` — two roots the bag built in + // `ExpressionProvider` above does not contain at all — while the identity + // aliases, `app` and `features` that it DOES contain went unnamed. + 'app-shell', ); try { diff --git a/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts b/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts index 4d2391518..4cb4d26f5 100644 --- a/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts +++ b/packages/app-shell/src/providers/ExpressionProvider.visibleFaultDiagnostic.test.ts @@ -314,3 +314,72 @@ describe('objectui#6443 — the rate limit, measured in both directions', () => expect(reports(warn)).toHaveLength(1); }); }); + +/* -------------------------------------------------------------------------- * + * objectui#6487 — the advice paragraph is this tier's, not the node tier's. + * + * The card #6443 made visible: once this site started printing, it printed the + * NODE gate's closing paragraph, telling a nav author to check `record` and + * `page.`. The bag `ExpressionProvider` builds is + * `{ current_user, user, ctx: { user }, os: { user }, app, data, features }` — + * it contains neither. + * + * These cells are the END-TO-END half of the pin: the unit matrix in + * `@object-ui/react` proves the two paragraphs differ, and these prove the + * paragraph a REAL faulting nav predicate reaches the console with is this + * tier's. A fault is asserted to have reached the reporter in the same run, so + * neither can pass on a site that stopped reporting. + * -------------------------------------------------------------------------- */ + +describe('objectui#6487 — the line carries the APP-SHELL tier`s roots', () => { + it('a faulting nav predicate is NOT told to check `record` or `page.`', () => { + const warn = spyWarn(); + const evaluator = makeEvaluator(); + const fault = 'nosuchroot6487shell.x > 1'; + + expect(evaluateVisibility(fault, evaluator)).toBe(true); // fail-open, unchanged + const lines = reports(warn); + expect(lines).toHaveLength(1); // the fault reached the reporter in THIS run + expect(lines[0]).not.toContain('Page-component predicates bind'); + expect(lines[0]).toContain('Neither `record` nor `page.` exists at this tier.'); + }); + + it('it names the roots this provider really binds — including `features`', () => { + // `features` is the deployment-flag root this provider's own docblock + // documents for exactly this kind of predicate, and it was unnamed. + // Asserted against the bag `makeEvaluator` builds, which is a copy of the + // provider's: every root named below is a key of it. + const warn = spyWarn(); + const evaluator = makeEvaluator(); + + evaluateVisibility('nosuchroot6487shellroots.x > 1', evaluator); + const [line] = reports(warn); + expect(line).toBeDefined(); + for (const root of ['`current_user`', '`user`', '`ctx.user`', '`os.user`', '`app`', '`features`']) { + expect(line).toContain(root); + } + }); + + it('CONTROL: the first paragraph is UNCHANGED — it is true on this fail-open surface too', () => { + // Green both ways, deliberately. Only the LAST paragraph is per-tier; the + // "gate did NOT bite" sentence is true on every surface wired to this + // reporter, and objectui#6445 owns the separate question of its polarity on + // the `disabled` gate. A fix that re-tiered the whole message would take + // this cell red and would be out of this card's scope. + const warn = spyWarn(); + evaluateVisibility('nosuchroot6487shellctl.x > 1', makeEvaluator()); + const [line] = reports(warn); + expect(line).toContain('gate did NOT bite'); + }); + + it('the surface label, gate key, source and reason all survive the tier split', () => { + const warn = spyWarn(); + const fault = 'nosuchroot6487shellshape.x > 1'; + evaluateVisibility(fault, makeEvaluator()); + const [line] = reports(warn); + expect(line).toContain(SURFACE); + expect(line).toContain('visible:'); + expect(line).toContain(fault); + expect(line).toContain('Reason:'); + }); +}); diff --git a/packages/components/src/__tests__/page-tabs-visible-when-fault-warning.test.tsx b/packages/components/src/__tests__/page-tabs-visible-when-fault-warning.test.tsx index e5f959c81..fc08e10f3 100644 --- a/packages/components/src/__tests__/page-tabs-visible-when-fault-warning.test.tsx +++ b/packages/components/src/__tests__/page-tabs-visible-when-fault-warning.test.tsx @@ -194,3 +194,45 @@ describe('objectui#6038 — a faulting `page:tabs` item predicate is reported', expect(reports(warn)).toHaveLength(1); }); }); + +/* -------------------------------------------------------------------------- * + * objectui#6487 — this surface stays on the PAGE-COMPONENT tier's advice. + * + * `page:tabs` items are the third caller of the shared reporter, and the card + * that re-tiered its closing paragraph had to decide where they sit. Measured + * on `isItemVisible` (`renderers/layout/containers.tsx`): the bag it builds + * binds `record`, `current_user` and `page.` — the three roots the node + * tier's paragraph names — so this surface keeps that paragraph and must not + * drift onto the app-shell one when the reporter learned to tell them apart. + * + * This is the cell where the two tiers DISAGREE, run at a real call site: it is + * red both if this site started printing the app-shell copy and if the fix had + * been bought by deleting the root names from both. + * -------------------------------------------------------------------------- */ + +describe('objectui#6487 — the `page:tabs` item gate keeps the node tier`s roots', () => { + it('names `record` / `current_user` / `page.`, and not the app-shell roots', () => { + const warn = spyWarn(); + const fault = 'nosuchroot6487tabs.x > 1'; + const { getByText } = render( + , + ); + expect(getByText('Contracts')).toBeTruthy(); // fail-open, unchanged + + const lines = reports(warn); + expect(lines).toHaveLength(1); // the fault reached the reporter in THIS run + expect(lines[0]).toContain('Page-component predicates bind'); + expect(lines[0]).toContain('`record`'); + expect(lines[0]).toContain('`page.`'); + // The disagreeing half: `features` and the ADR-0068 alias spellings belong + // to the app-shell bag, which this site does not build. + expect(lines[0]).not.toContain('`features`'); + expect(lines[0]).not.toContain('`ctx.user`'); + expect(lines[0]).not.toContain('Neither `record` nor'); + }); +}); diff --git a/packages/components/src/renderers/layout/containers.tsx b/packages/components/src/renderers/layout/containers.tsx index f3c8e995e..236ed5ccd 100644 --- a/packages/components/src/renderers/layout/containers.tsx +++ b/packages/components/src/renderers/layout/containers.tsx @@ -470,7 +470,20 @@ const PageTabsRenderer: React.FC = ({ schema, className, ...props }) => { // predicate is one line no matter which surface evaluates it. return evaluator.evaluateCondition(it.visibleWhen, { onFault: (reason) => - reportUnresolvableVisibilityPredicate('page:tabs', schema?.id, 'visibleWhen', it.visibleWhen, reason), + // `'page-component'`, not a tier of its own (objectui#6487): the bag + // built above binds `record`, `current_user` and `page.` — the + // three roots the node tier's advice paragraph names — so an author who + // faults here needs exactly that paragraph. The extra breadth this site + // adds (the row spread flat, `data` aliased to the row) is undeclared on + // both surfaces, so it is not advertised on either. + reportUnresolvableVisibilityPredicate( + 'page:tabs', + schema?.id, + 'visibleWhen', + it.visibleWhen, + reason, + 'page-component', + ), }); }; const visibleFlags = rawItems.map(isItemVisible); diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 1b16513bd..1adb17fd1 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -672,7 +672,18 @@ export const SchemaRenderer: ForwardRefExoticComponent< if (!__DEV__) { return evaluator.evaluateCondition(raw, { onFault: (reason) => - reportUnresolvableVisibilityPredicate(newSchema.type, newSchema.id, key, raw, reason), + // `'page-component'` stated rather than defaulted (objectui#6487): + // this is the tier whose roots the spec declares for a node gate, + // and saying so here is what keeps the advice paragraph a decision + // this call site owns. + reportUnresolvableVisibilityPredicate( + newSchema.type, + newSchema.id, + key, + raw, + reason, + 'page-component', + ), }); } try { @@ -687,7 +698,14 @@ export const SchemaRenderer: ForwardRefExoticComponent< reportAdapterOnlyDataPredicate(newSchema.type, newSchema.id, key, raw, dataSource); return verdict; } catch (err) { - reportUnresolvableVisibilityPredicate(newSchema.type, newSchema.id, key, raw, err); + reportUnresolvableVisibilityPredicate( + newSchema.type, + newSchema.id, + key, + raw, + err, + 'page-component', + ); // The historical fail-soft answer, unchanged — and identical to what // the production branch above returns for the same input, which is what // keeps the two branches one behaviour rather than two. See the diff --git a/packages/react/src/__tests__/SchemaRenderer.visibleWhenRecordBinding.test.tsx b/packages/react/src/__tests__/SchemaRenderer.visibleWhenRecordBinding.test.tsx index 4e8e956a1..d00cbd64f 100644 --- a/packages/react/src/__tests__/SchemaRenderer.visibleWhenRecordBinding.test.tsx +++ b/packages/react/src/__tests__/SchemaRenderer.visibleWhenRecordBinding.test.tsx @@ -399,3 +399,56 @@ describe('#5454 leg 3 — an unresolvable predicate is loud, and its verdict is expect(warn.mock.calls.map(c => String(c[0])).filter(m => m.includes(UNRESOLVABLE_VISIBILITY_PREFIX))).toHaveLength(1); }); }); + +/* -------------------------------------------------------------------------- * + * objectui#6487 — the node gate names ITS OWN roots, and only those. + * + * The reporter's closing paragraph became per-tier when objectui#6443 wired the + * app-shell chrome gate onto it and that surface started printing the node + * tier's roots for a bag that has neither. This cell is the node tier's end of + * that split, asserted where a REAL faulting node predicate reaches the + * console: the paragraph here is unchanged, and it must not drift onto the + * app-shell copy. + * + * The `not` half is the load-bearing one. `current_user` is named at both tiers, + * so asserting on it alone would be green whichever paragraph printed; the cells + * below assert the roots the two tiers DISAGREE about. + * -------------------------------------------------------------------------- */ + +describe('#6487 — the node tier`s advice paragraph', () => { + it('names the three roots the spec declares, and not the app-shell bag', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + // No row is bound, so `record.*` cannot resolve — a real fault, at the real + // call site, on the same run as the assertions below. + mount({ visibleWhen: cel("record.nosuchroot6487node == 'x'") }, undefined); + expect(shown()).toBe(true); // fail-soft verdict, unchanged + + const msg = warn.mock.calls + .map((c) => String(c[0])) + .find((m) => m.includes(UNRESOLVABLE_VISIBILITY_PREFIX)); + expect(msg).toBeDefined(); // the fault reached the reporter in THIS run + expect(msg).toContain('Page-component predicates bind'); + expect(msg).toContain('`record`'); + expect(msg).toContain('`page.`'); + // The app-shell tier's roots, which this bag does not promise. + expect(msg).not.toContain('`features`'); + expect(msg).not.toContain('`ctx.user`'); + expect(msg).not.toContain('`os.user`'); + }); + + it('the ambient app scope being mounted does not move the node gate onto the app-shell copy', () => { + // `APP_SCOPE` is the bag app-shell's `ExpressionProvider` really publishes, + // and `mount` spreads it into the node evaluator — so a fix that had + // deduced the tier from what happens to be in scope, rather than from the + // call site, would print the app-shell paragraph here. The tier is an + // argument precisely so that it cannot. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mount({ visibleWhen: 'nosuchroot6487ambient.x > 1' }, IN_REVIEW, APP_SCOPE); + const msg = warn.mock.calls + .map((c) => String(c[0])) + .find((m) => m.includes(UNRESOLVABLE_VISIBILITY_PREFIX)); + expect(msg).toBeDefined(); + expect(msg).toContain('Page-component predicates bind'); + expect(msg).not.toContain('Neither `record` nor'); + }); +}); diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 559790ede..b23df9022 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -33,6 +33,11 @@ export { UNRESOLVABLE_VISIBILITY_PREFIX, __resetVisibilityPredicateWarnings, } from './utils/visibilityDiagnostic.js'; +// The per-surface scope hint those two take (objectui#6487). Exported because +// `@object-ui/app-shell` — a caller in another package — has to name its tier, +// and a caller that cannot spell the argument would be back on the node tier's +// advice by default, which is the defect that card fixed. +export type { PredicateScopeTier } from './utils/visibilityDiagnostic.js'; // Write-error surfacing utilities (shared by drag-write plugins so a failed // PATCH — e.g. an RLS 403 — is never silently swallowed). diff --git a/packages/react/src/utils/visibilityDiagnostic.adviceTier.test.ts b/packages/react/src/utils/visibilityDiagnostic.adviceTier.test.ts new file mode 100644 index 000000000..31d3e41ad --- /dev/null +++ b/packages/react/src/utils/visibilityDiagnostic.adviceTier.test.ts @@ -0,0 +1,282 @@ +/** + * 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#6487 — the advice paragraph is chosen by the SCOPE the predicate was + * evaluated against, not printed in one tier's spelling for every surface. + * + * ## The defect this file pins + * + * The reporter closed every line with the NODE tier's roots — `record`, + * `current_user`, `page.`. Since objectui#6443 the same reporter also + * prints for the app-shell chrome gate, whose evaluator + * (`ExpressionProvider.tsx`) is built from + * `{ current_user, user, ctx: { user }, os: { user }, app, data, features }`. + * There is no `record` and no `page` in that bag at all, so an author whose nav + * or area `visible` faulted was sent to check two roots that cannot exist at + * their tier, while the identity aliases, `app` and `features` — which do — + * went unnamed. + * + * ## Why the assertions sit where the tiers DISAGREE + * + * `current_user` is named at BOTH tiers, so a cell asserting "the advice + * mentions `current_user`" is green against the defect and green after it: it + * measures nothing. Every load-bearing cell below is therefore a root that one + * tier names and the other must not, and the one agreeing root is kept, marked + * as degenerate, for exactly the reason it cannot be trusted alone. + * + * ## Reverse verification — direction predicted BEFORE the run + * + * Restoring `SCOPE_TIER_ADVICE['app-shell']` to the node tier's paragraph (the + * pre-fix state, reached by passing `'page-component'` at + * `ExpressionProvider.tsx`'s call site) turns RED every cell in the + * `app-shell tier` group and the two cross-tier cells, and leaves the whole + * `page-component tier` group and every control GREEN. The asymmetry is the + * card restated: the node tier's copy was correct and is unchanged; only the + * surface it was wrongly reused on moves. + * + * Deleting the root NAMES instead — the "generalise the copy" fork the card + * measured and the triage fenced off — turns red BOTH groups at once, which is + * how this file also refuses that shape: advice that names no roots is correct + * everywhere and useful nowhere. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { + formatUnresolvableVisibilityMessage, + reportUnresolvableVisibilityPredicate, + __resetVisibilityPredicateWarnings, + UNRESOLVABLE_VISIBILITY_PREFIX, +} from './visibilityDiagnostic'; + +/** Unique to this file: the dedupe `Set` is module state shared with every other suite. */ +const SOURCE = 'nosuchroot6487.x > 1'; +const REASON = 'unknown identifier: nosuchroot6487'; + +const nodeTier = (source = SOURCE) => + formatUnresolvableVisibilityMessage('element:text', 'el1', 'visibleWhen', source, REASON, 'page-component'); +const appShellTier = (source = SOURCE) => + formatUnresolvableVisibilityMessage('app-shell:visible', undefined, 'visible', source, REASON, 'app-shell'); + +type WarnSpy = { mock: { calls: unknown[][] } }; +const spyWarn = () => vi.spyOn(console, 'warn').mockImplementation(() => {}); +const reports = (warn: WarnSpy): string[] => + warn.mock.calls.map((c) => String(c[0])).filter((m) => m.includes(UNRESOLVABLE_VISIBILITY_PREFIX)); +const allWarnings = (warn: WarnSpy): string[] => warn.mock.calls.map((c) => String(c[0])); + +beforeEach(() => { + __resetVisibilityPredicateWarnings(); +}); +afterEach(() => { + vi.restoreAllMocks(); +}); + +/* -------------------------------------------------------------------------- * + * Group 0 — capture controls. Green both ways, and without them every + * `toHaveLength(0)` and every `not.toContain` in this file is equally green on + * a reporter that stopped reporting. + * -------------------------------------------------------------------------- */ + +describe('#6487 group 0 — controls', () => { + it('POSITIVE CONTROL: the spy observes a line carrying the prefix', () => { + const warn = spyWarn(); + console.warn(`${UNRESOLVABLE_VISIBILITY_PREFIX} - synthetic control line 6487`); + expect(reports(warn)).toHaveLength(1); + }); + + it('DEGENERATE CONTROL: unrelated console output does not satisfy the pin', () => { + const warn = spyWarn(); + console.warn('[object-ui] an entirely unrelated warning 6487'); + expect(allWarnings(warn)).toHaveLength(1); + expect(reports(warn)).toHaveLength(0); + }); + + it('DEGENERATE CELL, kept and labelled: `current_user` is named at BOTH tiers', () => { + // The shape the card's census clause warns about. This cell is green + // against the defect and green after it, so it is evidence about NOTHING on + // its own — it is here so the file states which root is shared, and so a + // future reader cannot mistake the cells below for an arbitrary split. + expect(nodeTier()).toContain('`current_user`'); + expect(appShellTier()).toContain('`current_user`'); + }); +}); + +/* -------------------------------------------------------------------------- * + * Group 1 — the node tier's paragraph, unchanged by this card. + * -------------------------------------------------------------------------- */ + +describe('#6487 group 1 — page-component tier', () => { + it('names the three roots the spec declares for a node gate', () => { + // `ui/page.zod.ts`: "Binds `record`, `current_user`, `page.`" — and + // `SchemaRenderer` builds exactly those (plus `data` = the adapter). + const msg = nodeTier(); + expect(msg).toContain('`record`'); + expect(msg).toContain('`current_user`'); + expect(msg).toContain('`page.`'); + }); + + it('does NOT name the app-shell-only roots', () => { + // The other half of the tier split. `features` and the ADR-0068 alias + // spellings reach a node predicate only when a host mounted them ambiently; + // the node tier's own contract does not promise them, so its advice does + // not send an author to check them. + const msg = nodeTier(); + expect(msg).not.toContain('`features`'); + expect(msg).not.toContain('`ctx.user`'); + expect(msg).not.toContain('`os.user`'); + }); + + it('is the DEFAULT, byte for byte: the five-argument call is unchanged', () => { + // The compatibility half of the signature change. A caller outside this + // repo that has not been updated must keep printing what it printed before + // — not approximately, exactly. + const withoutTier = formatUnresolvableVisibilityMessage( + 'element:text', 'el1', 'visibleWhen', SOURCE, REASON, + ); + expect(withoutTier).toBe(nodeTier()); + }); +}); + +/* -------------------------------------------------------------------------- * + * Group 2 — the app-shell tier. Every cell here is RED against the defect. + * -------------------------------------------------------------------------- */ + +describe('#6487 group 2 — app-shell tier', () => { + it('THE acceptance criterion: the node tier`s paragraph is gone from this tier', () => { + // The sentence that did the damage, asserted by its own text rather than by + // a root name: this tier still MENTIONS `record`, and must, because saying + // it is absent is the single most useful thing to tell an author who copied + // a node predicate into a nav item. "Sent to check it" and "told it is not + // here" are opposite guidance that share a word, so the pin is on which. + const msg = appShellTier(); + expect(msg).not.toContain('Page-component predicates bind'); + expect(msg).toContain('Neither `record` nor `page.` exists at this tier.'); + }); + + it('the ONLY mention of either absent root is the sentence declaring it absent', () => { + // The precise form of "does not send the author there". A future edit that + // reintroduced `record` as a bindable root anywhere in this tier`s copy adds + // a second matching line and turns this red, while the negation sentence + // above keeps passing — which is why both cells exist. + const msg = appShellTier(); + const ABSENT = 'Neither `record` nor `page.` exists at this tier.'; + for (const root of ['`record`', '`page.`']) { + const lines = msg.split('\n').filter((l) => l.includes(root)); + expect(lines).toEqual([ABSENT]); + } + }); + + it('names every root the provider actually binds and the node tier left out', () => { + // Derived from `ExpressionProvider.tsx`'s own bag: + // `{ current_user, user, ctx: { user }, os: { user }, app, data, features }`. + const msg = appShellTier(); + expect(msg).toContain('`current_user`'); + expect(msg).toContain('`user`'); + expect(msg).toContain('`ctx.user`'); + expect(msg).toContain('`os.user`'); + expect(msg).toContain('`app`'); + expect(msg).toContain('`features`'); + }); + + it('still names CONCRETE roots — it is not the generalisation the card refused', () => { + // The fence the triage put on this card, pinned rather than trusted: the + // fix is not allowed to buy correctness at every tier by naming roots at + // none. Six concrete roots is measurably not "check whatever this surface + // binds". + const msg = appShellTier(); + const named = ['`current_user`', '`user`', '`ctx.user`', '`os.user`', '`app`', '`features`']; + expect(named.filter((root) => msg.includes(root))).toHaveLength(named.length); + }); +}); + +/* -------------------------------------------------------------------------- * + * Group 3 — what the tiers must NOT disagree about. + * -------------------------------------------------------------------------- */ + +describe('#6487 group 3 — only the last paragraph is per-tier', () => { + it('everything above the advice is identical on both tiers', () => { + // The card's own boundary: the first paragraph ("the gate did NOT bite") is + // true on every surface wired to this reporter, INCLUDING the fail-open + // app-shell gate, and this card does not get to touch it. Compared as + // TEXT — the two messages are built from different (type, id, key) inputs, + // so the shared part is the two trailing sentences of the fault paragraph. + const shared = [ + 'The node was treated as its safe default, which on this surface means the', + 'gate did NOT bite - a predicate that cannot be evaluated reads on screen', + 'exactly like one that said yes.', + ].join('\n'); + expect(nodeTier()).toContain(shared); + expect(appShellTier()).toContain(shared); + }); + + it('both tiers still carry the prefix, the key, the source and the engine reason', () => { + for (const msg of [nodeTier(), appShellTier()]) { + expect(msg).toContain(UNRESOLVABLE_VISIBILITY_PREFIX); + expect(msg).toContain(SOURCE); + expect(msg).toContain(REASON); + } + expect(nodeTier()).toContain('visibleWhen:'); + expect(appShellTier()).toContain('visible:'); + }); + + it('both tiers close on the same instruction', () => { + expect(nodeTier()).toContain('Check those roots and the'); + expect(appShellTier()).toContain('Check those roots and the'); + }); +}); + +/* -------------------------------------------------------------------------- * + * Group 4 — the EMITTER, not the formatter. A fault has to actually reach the + * console in the same run, or every string assertion above is a statement about + * a function nobody calls. + * -------------------------------------------------------------------------- */ + +describe('#6487 group 4 — the tier reaches the console', () => { + it('the app-shell tier prints ONE line, and it is the app-shell paragraph', () => { + const warn = spyWarn(); + reportUnresolvableVisibilityPredicate( + 'app-shell:visible', undefined, 'visible', 'nosuchroot6487emit.x > 1', REASON, 'app-shell', + ); + const lines = reports(warn); + // Ghost-assertion guard: the fault reached the reporter in THIS run. + expect(lines).toHaveLength(1); + expect(lines[0]).toContain('`features`'); + expect(lines[0]).not.toContain('Page-component predicates bind'); + }); + + it('the page-component tier prints ONE line, and it is the node paragraph', () => { + const warn = spyWarn(); + reportUnresolvableVisibilityPredicate( + 'element:text', 'el2', 'visibleWhen', 'nosuchroot6487node.x > 1', REASON, 'page-component', + ); + const lines = reports(warn); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain('`record`'); + expect(lines[0]).not.toContain('`features`'); + }); + + it('the tier is NOT in the dedupe key — the objectui#6038 rate limit is untouched', () => { + // Stated as a consequence rather than discovered later. Adding `tier` to the + // key could only LOOSEN the limit, and it would take one `type` shared + // across two tiers to loosen anything — which production cannot produce, + // since the app-shell site's `type` is the constant `'app-shell:visible'`. + // This cell reaches it only by calling the reporter directly. + const warn = spyWarn(); + const shared = 'nosuchroot6487dedupe.x > 1'; + reportUnresolvableVisibilityPredicate('app-shell:visible', undefined, 'visible', shared, REASON, 'app-shell'); + reportUnresolvableVisibilityPredicate('app-shell:visible', undefined, 'visible', shared, REASON, 'page-component'); + expect(reports(warn)).toHaveLength(1); + }); + + it('a SECOND distinct source still reports — a dedupe that suppressed everything would look the same', () => { + const warn = spyWarn(); + reportUnresolvableVisibilityPredicate('app-shell:visible', undefined, 'visible', 'nosuchroot6487two_a.x > 1', REASON, 'app-shell'); + reportUnresolvableVisibilityPredicate('app-shell:visible', undefined, 'visible', 'nosuchroot6487two_b.y == 3', REASON, 'app-shell'); + expect(reports(warn)).toHaveLength(2); + }); +}); diff --git a/packages/react/src/utils/visibilityDiagnostic.ts b/packages/react/src/utils/visibilityDiagnostic.ts index 673b2fe52..0cd8da061 100644 --- a/packages/react/src/utils/visibilityDiagnostic.ts +++ b/packages/react/src/utils/visibilityDiagnostic.ts @@ -62,9 +62,93 @@ function predicateSourceText(raw: unknown): string { return typeof raw === 'string' ? raw : String(raw); } +/** + * Which SCOPE the faulting predicate was evaluated against (objectui#6487). + * + * ## Why the reporter needs to be told, rather than deducing it + * + * The closing paragraph of the message names the ROOTS an author can bind. That + * is the half of the report an author acts on: a fault is usually a mistyped or + * unbound root, and "which roots exist here" is the answer. It is therefore the + * one part of the copy that is NOT true on every surface — the callers below + * build genuinely different bags — and it was printed unconditionally in the + * node tier's spelling, so an app-shell author was sent to check `record` and + * `page.`, which nothing at that tier binds. + * + * The alternative fix — generalising the copy to "whatever this surface binds" + * plus a docs link — was refused deliberately: the concrete root names are the + * whole value of the paragraph, and advice that names no roots is correct + * everywhere and useful nowhere. + * + * Deducing the tier from the `type` slot was refused too. `type` carries the + * SCHEMA NODE TYPE at the node tier and a surface label at the other two + * (`page:tabs`, `app-shell:visible`), and `page:tabs` is a real registry key — + * so a `page:tabs` NODE faulting its own `visibleWhen` inside `SchemaRenderer` + * is indistinguishable, by `type` alone, from one of its ITEM predicates. An + * explicit argument cannot collide that way. + * + * ## The tiers, each derived from the code that builds the bag + * + * `'page-component'` — the node gate. `SchemaRenderer.tsx` builds + * `new ExpressionEvaluator({ ...predicateScope, current_user, [record], data, + * page })`, so the roots the SPEC declares for the tier (`ui/page.zod.ts`: + * *"Binds `record`, `current_user`, `page.`"*) are the roots it binds. + * `page:tabs` ITEM predicates (`containers.tsx`) are this tier as well: they + * build the same three roots (plus the row spread flat, and `data` aliased to + * the row rather than to the adapter — both undeclared breadth that the spec + * does not promise and this paragraph therefore does not advertise). + * + * `'app-shell'` — the chrome gate. `ExpressionProvider.tsx` builds + * `{ current_user, user, ctx: { user }, os: { user }, app, data, features }`. + * There is no `record` and no `page` in it at all, which is the defect this + * type exists to fix. The four identity spellings are the ADR-0068 aliases and + * all four resolve; `features` is the deployment-flag root that provider's own + * docblock documents for exactly this kind of predicate. + * + * `data` is bound at the app-shell tier but is deliberately NOT advertised + * there: every mount of `ExpressionProvider` in this repo passes `data={{}}` + * or omits it, so naming it would point an author at a root that answers + * nothing. + */ +export type PredicateScopeTier = 'page-component' | 'app-shell'; + +/** + * The closing paragraph, per tier. The ONLY part of the message that varies — + * everything above it (the node, the key, the source, the engine's reason, and + * the "gate did NOT bite" sentence) is true on every surface wired to this + * reporter, including the fail-open app-shell gate. + * + * Both entries end on the same sentence on purpose: whichever tier an author is + * on, the two things to check are the roots and the CEL syntax. + * + * Indexed WITHOUT a `??` fallback to the node tier, deliberately (AGENTS.md + * #0.1). A tier this table does not answer is a type violation, and quietly + * substituting the node tier's paragraph for it would reproduce, one caller + * further along, the exact defect this card fixes: confident advice about a + * scope the predicate was never evaluated against. The default lives on the + * PARAMETER, where it is a stated compatibility choice, not in the lookup. + */ +const SCOPE_TIER_ADVICE: Record = { + 'page-component': + 'Page-component predicates bind `record` (the row on a record page),\n' + + '`current_user`, and page state as `page.`. Check those roots and the\n' + + 'CEL syntax.', + 'app-shell': + 'App-shell predicates bind `current_user` - also spelled `user`, `ctx.user`\n' + + 'and `os.user` - plus `app` and `features` (the deployment flags).\n' + + 'Neither `record` nor `page.` exists at this tier.\n' + + 'Check those roots and the CEL syntax.', +}; + /** * Build the message. Separate from the emit so a test can assert the words, * not merely that something was logged. + * + * `tier` defaults to `'page-component'` so the historical five-argument call + * keeps printing the exact bytes it printed before (objectui#6487) — the + * default is a compatibility shim for callers outside this repo, not something + * this repo leans on: all three in-repo call sites pass their tier explicitly, + * which is what makes each one a stated decision rather than an inherited one. */ export function formatUnresolvableVisibilityMessage( type: unknown, @@ -72,6 +156,7 @@ export function formatUnresolvableVisibilityMessage( key: string, raw: unknown, reason: string, + tier: PredicateScopeTier = 'page-component', ): string { const node = typeof type === 'string' && type ? '"' + type + '"' : '(untyped node)'; const where = typeof id === 'string' && id ? ' (id: "' + id + '")' : ''; @@ -82,9 +167,7 @@ export function formatUnresolvableVisibilityMessage( 'The node was treated as its safe default, which on this surface means the\n' + 'gate did NOT bite - a predicate that cannot be evaluated reads on screen\n' + 'exactly like one that said yes.\n' + - 'Page-component predicates bind `record` (the row on a record page),\n' + - '`current_user`, and page state as `page.`. Check those roots and the\n' + - 'CEL syntax.' + SCOPE_TIER_ADVICE[tier] ); } @@ -122,6 +205,15 @@ const _warnedVisibilityPredicates = new Set(); * evaluation). `String(err)` already covered that shape, so both callers reach * the same `Reason:` text and the same dedupe entry — which is what makes "dev * and production print the identical line" true rather than approximately true. + * + * ## `tier` is NOT in the dedupe key, and that is not an oversight + * + * The key stays `(type, key, predicate source)` — the rate limit objectui#6038 + * pinned in both directions. Adding `tier` could only ever LOOSEN it, and it + * would take a `type` shared across two tiers to loosen anything: the app-shell + * site's `type` is the constant `'app-shell:visible'`, which no node tier can + * produce, and the two `'page-component'` callers are the same tier by + * definition. So the tier is free of the key by measurement, not by assumption. */ export function reportUnresolvableVisibilityPredicate( type: unknown, @@ -129,12 +221,13 @@ export function reportUnresolvableVisibilityPredicate( key: string, raw: unknown, err: unknown, + tier: PredicateScopeTier = 'page-component', ): void { const reason = err instanceof Error ? err.message : String(err); const dedupeKey = JSON.stringify([type, key, predicateSourceText(raw)]); if (_warnedVisibilityPredicates.has(dedupeKey)) return; _warnedVisibilityPredicates.add(dedupeKey); - console.warn(formatUnresolvableVisibilityMessage(type, id, key, raw, reason)); + console.warn(formatUnresolvableVisibilityMessage(type, id, key, raw, reason, tier)); } /**