Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions .changeset/6487-visibility-advice-per-tier.md
Original file line numberDiff line numberDiff line change
@@ -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.<var>` — 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.<var>` (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.<var>` 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.
6 changes: 6 additions & 0 deletions packages/app-shell/src/providers/ExpressionProvider.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -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.<var>` — 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 {
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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.<var>`. 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.<var>`', () => {
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.<var>` 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:');
});
});
Original file line numberDiff line numberDiff line change
Expand Up@@ -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.<var>` — 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.<var>`, and not the app-shell roots', () => {
const warn = spyWarn();
const fault = 'nosuchroot6487tabs.x > 1';
const { getByText } = render(
<SchemaRenderer
schema={tabsSchema([
{ label: 'Details', value: 'details', children: [] },
{ label: 'Contracts', value: 'contracts', visibleWhen: fault, children: [] },
])}
/>,
);
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.<var>`');
// 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');
});
});
15 changes: 14 additions & 1 deletion packages/components/src/renderers/layout/containers.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -470,7 +470,20 @@ const PageTabsRenderer: React.FC<any> = ({ 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.<var>` — 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);
Expand Down
22 changes: 20 additions & 2 deletions packages/react/src/SchemaRenderer.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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.<var>`');
// 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');
});
});
5 changes: 5 additions & 0 deletions packages/react/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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).
Expand Down
Loading
Loading