From 03edb29220d4614fcaf0307c0e1e95b39e1568b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 19 Sep 2026 15:25:28 +0000 Subject: [PATCH] fix(app-shell): client-evaluated condition mounts declare the roots their host binds objectui#9645 narrowed a `scope="record"` `ConditionBuilder` mount to the roots every host of a record-scoped condition binds, and named this cost on its face: a mount whose host binds MORE loses the offers its evaluator really answers. An action's `visible` / `disabled` is evaluated in the browser, where the shell's expression scope publishes the identity roots and the feature flags. The two curated action mounts and the generic schema-driven mount now declare what their host binds. Which tier evaluates which metadata type is read from the ruled host table rather than decided again here, through a third derivation beside the lint scope and the subject vocabulary. `previous` is withdrawn at those mounts in the same act, because no browser host binds it. Server-evaluated mounts and every mount whose tier that table does not measure are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018HrVaotisyhgmot9o2MLRq --- .../9856-client-mount-condition-roots.md | 40 +++ .../ConditionWidget.conditionRoots.test.tsx | 177 ++++++++++++ .../views/metadata-admin/ResourceEditPage.tsx | 8 + .../views/metadata-admin/conditionScope.ts | 32 ++- .../inspectors/ActionDefaultInspector.tsx | 18 +- ...ConditionBuilder.clientMountRoots.test.tsx | 266 ++++++++++++++++++ .../inspectors/ConditionBuilder.tsx | 51 ++++ .../src/views/metadata-admin/widgets.tsx | 30 ++ 8 files changed, 618 insertions(+), 4 deletions(-) create mode 100644 .changeset/9856-client-mount-condition-roots.md create mode 100644 packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionRoots.test.tsx create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.clientMountRoots.test.tsx diff --git a/.changeset/9856-client-mount-condition-roots.md b/.changeset/9856-client-mount-condition-roots.md new file mode 100644 index 0000000000..35ff0ce22f --- /dev/null +++ b/.changeset/9856-client-mount-condition-roots.md @@ -0,0 +1,40 @@ +--- +'@object-ui/app-shell': minor +--- + +Client-evaluated `ConditionBuilder` mounts declare the scope roots their own host binds +(objectui#9856) — the declared cost of objectui#9645, now paid. + +objectui#9645 made a `scope="record"` mount advertise `RECORD_CONDITION_ROOTS` rather +than the CEL engine's whole default list, and named this follow-up on its own face: the +narrowed set is the one EVERY host of a record-scoped condition binds, which is right +for a mount whose host the component cannot see and wrong for a mount whose host binds +more. An action's `visible` / `disabled` is evaluated in the BROWSER, where the shell's +own expression scope publishes the identity roots and the deployment feature flags, so +those mounts had been advertising two roots while their evaluator answered six — with +the inspector's own section hint still promising predicates "over the record / user / +ctx". + +**What changes for an author.** Editing an action's **Visible when** / **Disabled when** +— through the curated inspector or through the generic metadata form — the raw CEL +editor again suggests `user`, `current_user`, `os`, `ctx` and `features`, and its worked +example teaches the `user` clause again. Nothing an author could already type stops +working anywhere: the accept set is untouched, so this moves SUGGESTIONS only. + +**One root is withdrawn at those same mounts, deliberately.** `previous` is no longer +offered where a browser evaluates the predicate. No client host binds it — the row +arrives alone and the ambient scope publishes no `previous` — so suggesting it there +built a predicate that could only fault. Server-evaluated mounts (a hook `condition`, an +object validation rule's guard) keep it and keep their narrowing exactly as +objectui#9645 left it. + +**Which tier evaluates which metadata type is not re-decided here.** It is read from +`CONDITION_HOST_BY_METADATA_TYPE` (objectui#9953), the ruled table, through a new +`conditionRootsForMetadataType` — the third derivation off it, beside the lint scope and +the subject vocabulary. Mounts whose tier that table does not measure (a page block's +`visibleWhen`, a flow node's entry condition) declare nothing and are unchanged byte for +byte. + +The advertised list is never retyped: its pin rebuilds it from the two producers that +decide it and lints every member at the `record` scope, so it reddens both when a host +binding moves and when a root is advertised that the engine would refuse. diff --git a/packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionRoots.test.tsx b/packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionRoots.test.tsx new file mode 100644 index 0000000000..0d5e5a1adc --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionRoots.test.tsx @@ -0,0 +1,177 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The generic condition mount advertises the roots ITS OWN metadata type's + * evaluator binds — objectui#9856, the polymorphic mount the curated + * declaration could not be copied onto. + * + * ## The defect these cases reproduce + * + * `SchemaForm` routes every predicate-named field to one widget, so one mount + * serves every metadata type. objectui#8167 gave that mount the host's LINT + * scope and objectui#9953 gave it the host's SUBJECT vocabulary; the third + * question — what the raw editor's autocomplete may OFFER — still had no + * channel. So an `action` edited through the generic form inherited the + * `scope="record"` default `RECORD_CONDITION_ROOTS`, the set every host of a + * record-scoped condition binds, while its own host is the browser, where + * `buildExpressionScope` binds more. The same loss as the curated mounts, one + * route over. + * + * ## Why this is a DERIVATION and not a third literal + * + * The two curated action mounts can name the list because they serve one tier. + * This mount serves every tier, and the two arms disagree: the same value would + * be right for an action and would hand a hook's author roots its server host + * never binds — re-opening the trap objectui#9645 closed. So the answer comes + * from `CONDITION_HOST_BY_METADATA_TYPE`, the one place the client/server + * verdict is ruled, through `conditionRootsForMetadataType`. + * + * ## Read through the WIRING, not from the helper + * + * The two rendered cases feed the derivation's own answer to the real widget + * and read the real suggestion menu. Asserting on `conditionRootsForMetadataType` + * alone would leave the JSX free to drop the prop with every case still green — + * the forwarding is half of what is under test. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +// Module-scope import of the CEL engine, per AGENTS.md's flaky-test rule: the +// scope introspection runs behind a dynamic `import('@objectstack/formula')` +// inside `celAuthoring`, and a cold first load has been measured near a +// `waitFor`'s whole budget. The specifier must match `loadFormula`'s exactly — +// ESM caches by resolved specifier. +import '@objectstack/formula'; + +// `ConditionBuilder` calls `useObjectFields` unconditionally (objectui#4697), +// so an unmocked client would let a mount-time fetch escape to the real network. +const state = vi.hoisted(() => ({ + metadataClient: { get: vi.fn(async () => undefined), list: vi.fn(async () => [] as unknown[]) }, +})); +vi.mock('./useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, useMetadataClient: () => state.metadataClient }; +}); + +import { SchemaForm } from './SchemaForm'; +import type { WidgetContext } from './widgets'; +import { + CONDITION_HOST_BY_METADATA_TYPE, + conditionRootsForMetadataType, +} from './conditionScope'; +import { CLIENT_CONDITION_ROOTS } from './inspectors/ConditionBuilder'; + +afterEach(cleanup); + +/** A schema whose one field is routed to the condition widget BY NAME. */ +const SCHEMA = { + type: 'object', + properties: { condition: { type: 'string', title: 'Run only when' } }, +} as never; + +function Harness({ context }: { context: WidgetContext }) { + const [value, setValue] = React.useState>({ condition: '' }); + return ( + setValue(next as Record)} + widgetContext={context} + /> + ); +} + +/** The LABEL of each open suggestion — the first span; the second is its kind tag. */ +function offeredLabels(): string[] { + return screen + .queryAllByRole('option') + .map((o) => (o.querySelector('span')?.textContent ?? '').trim()); +} + +/** + * Mount the generic form with `roots`, warm the suggestion machinery, and read + * what it offers for `prefix`. + * + * ⚠️ The warm-up is load-bearing: the identifier catalog arrives + * asynchronously, and a menu that has not opened YET offers nothing — which + * would satisfy the "is not offered" case below no matter what the widget + * forwards. `record` is offered on both arms, so completing it begs no question. + */ +async function offeredAtGenericMount( + roots: string[] | undefined, + prefix: string, +): Promise { + // One mount per reading: a case that takes two readings would otherwise leave + // two forms on screen and every `byRole` query would match both. + cleanup(); + const user = userEvent.setup(); + render(); + const group = await screen.findByRole('group', { name: /Run only when/ }); + fireEvent.click(within(group).getByText('Expression')); + const box = within(group) + .getAllByRole('combobox') + .find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement; + await user.click(box); + await user.type(box, 'rec'); + expect(await screen.findByRole('option', { name: /record/ }, { timeout: 4000 })).toBeTruthy(); + await user.clear(box); + await user.type(box, prefix); + return offeredLabels(); +} + +/* ── The two arms, both fed the derivation's own answer ────────────────── */ + +describe('the generic condition mount, editing a CLIENT-evaluated type (objectui#9856)', () => { + it('offers the roots only a browser host binds', async () => { + // THE GATE. `conditionRootsForMetadataType('action')` is what + // `ResourceEditPage` hands this widget for that type, so this case fails if + // the derivation stops answering, if the widget stops forwarding, or if the + // list stops carrying what the browser evaluator binds. + const offered = await offeredAtGenericMount(conditionRootsForMetadataType('action'), 'o'); + expect(offered).toContain('os'); + }); + + it('does NOT offer them for a server-evaluated type — the narrowing stays put', async () => { + // The must-not-widen half, and the reason this member could not have been + // one value for the whole mount. `undefined` is what the derivation hands + // back for `hook`, and it leaves `ConditionBuilder`'s record-scoped default + // exactly where objectui#9645 put it. + const offered = await offeredAtGenericMount(conditionRootsForMetadataType('hook'), 'o'); + expect(offered).not.toContain('os'); + // Non-vacuity: this arm still completes SOMETHING, so the case above is not + // passing against a menu that simply never opened. + expect(await offeredAtGenericMount(conditionRootsForMetadataType('hook'), 'rec')) + .toContain('record'); + }); +}); + +/* ── The seam between the table and the list ───────────────────────────── */ + +describe('conditionRootsForMetadataType — derived from the ruled host table (objectui#9856)', () => { + it('answers with the advertised list itself for every client-evaluated type', () => { + // Identity, and over the TABLE rather than over one type: a `client` row + // added tomorrow is covered the day it lands. + const clientTypes = Object.entries(CONDITION_HOST_BY_METADATA_TYPE) + .filter(([, host]) => host === 'client') + .map(([type]) => type); + expect(clientTypes.length).toBeGreaterThan(0); + for (const type of clientTypes) { + expect(conditionRootsForMetadataType(type)).toBe(CLIENT_CONDITION_ROOTS); + } + }); + + it('declares nothing for every type that is not client-evaluated', () => { + // Including the unmeasured ones, which is the arm that keeps a tier nobody + // has put to an evaluator exactly as it was rather than widened on a guess. + for (const [type, host] of Object.entries(CONDITION_HOST_BY_METADATA_TYPE)) { + if (host === 'client') continue; + expect(conditionRootsForMetadataType(type)).toBeUndefined(); + } + expect(conditionRootsForMetadataType('flow')).toBeUndefined(); + expect(conditionRootsForMetadataType('a-type-this-build-never-heard-of')).toBeUndefined(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index 2f3011a7bd..05b0a8e1c4 100644 --- a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx @@ -106,6 +106,7 @@ import { } from './widgets.js'; import { conditionScopeForMetadataType, + conditionRootsForMetadataType, conditionSubjectsForMetadataType, } from './conditionScope.js'; import { mapLoaded, usePickerLoad } from './loadState.js'; @@ -983,6 +984,13 @@ function MetadataResourceEditPageImpl({ // same move as the line above, applied to the fact that line cannot // carry. conditionSubjects: conditionSubjectsForMetadataType(type), + // objectui#9856 — the same move once more, on the question the two lines + // above cannot answer between them: what the raw editor's autocomplete + // may OFFER. The builder's own default narrows a record-scoped mount to + // what every host binds, so the `action` tier — evaluated in the browser + // — is the one that has to declare that it binds more. Derived per type + // here for the reason the scope is: this page edits every metadata type. + conditionRoots: conditionRootsForMetadataType(type), objectNames: objectsState, objectFields: mapLoaded(objectCatalogState, (catalog) => catalog.fields), objectActions: mapLoaded(objectCatalogState, (catalog) => catalog.actions), diff --git a/packages/app-shell/src/views/metadata-admin/conditionScope.ts b/packages/app-shell/src/views/metadata-admin/conditionScope.ts index 9885602486..f34b0f48f5 100644 --- a/packages/app-shell/src/views/metadata-admin/conditionScope.ts +++ b/packages/app-shell/src/views/metadata-admin/conditionScope.ts @@ -1,6 +1,9 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. -import { RECORD_CONDITION_SUBJECTS } from './inspectors/ConditionBuilder.js'; +import { + CLIENT_CONDITION_ROOTS, + RECORD_CONDITION_SUBJECTS, +} from './inspectors/ConditionBuilder.js'; /** * Which lint scope a SCHEMA-DRIVEN condition editor claims, decided by the @@ -272,3 +275,30 @@ export function conditionSubjectsForMetadataType( ? RECORD_CONDITION_SUBJECTS : undefined; } + +/** + * The scope roots a condition editor at a host editing `type` may OFFER, or + * `undefined` to declare no narrowing (objectui#9856). + * + * The third derivation off {@link CONDITION_HOST_BY_METADATA_TYPE}, and the one + * that reads it the other way round from + * {@link conditionSubjectsForMetadataType}. That one narrows the SERVER tier, + * because the row builder's subject dropdown offers `user.*` by default and a + * server host binds no `user`. This one widens the CLIENT tier, because + * `ConditionBuilder` narrows a `scope="record"` mount to + * `RECORD_CONDITION_ROOTS` by default and a browser host binds more than that. + * Same table, opposite defaults, one ruling — which is why neither can be + * derived from `scope === 'record'`, and why the answer has to come from the + * host that knows which metadata type is on screen. + * + * `undefined` is a decision here too, and it covers both remaining arms: + * + * - a `server` tier keeps the builder's own record-scoped narrowing, byte for + * byte what objectui#9645 landed; + * - an UNMEASURED tier keeps whatever it had, because a root list is a reading + * taken at an evaluator and there is none to hand back for a tier nobody has + * put to one. + */ +export function conditionRootsForMetadataType(type: string): string[] | undefined { + return conditionHostForMetadataType(type) === 'client' ? CLIENT_CONDITION_ROOTS : undefined; +} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx index dbc4011386..8cfd645d69 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx @@ -56,7 +56,7 @@ import { import { useObjectOptions } from '../previews/useObjectOptions.js'; import { useObjectFields } from '../previews/useObjectFields.js'; import { useMetaOptions } from '../previews/useMetaOptions.js'; -import { ConditionBuilder } from './ConditionBuilder.js'; +import { ConditionBuilder, CLIENT_CONDITION_ROOTS } from './ConditionBuilder.js'; import { expressionSource, writeExpressionSource } from './expression-envelope.js'; import { IconPickerWidget } from '../widgets.js'; @@ -898,8 +898,20 @@ export function ActionDefaultInspector({ and wrong here. It also ends a disagreement inside this very control — the row builder was already emitting `record.` while its own raw editor accepted the retired bare spelling. */} - onPatch({ visible: writeExpressionSource(draft.visible, v) })} objectName={objectName} disabled={readOnly} scope="record" onBlockingIssuesChange={(n) => reportCel('visible', n)} /> - onPatch({ disabled: writeExpressionSource(draft.disabled, v) })} objectName={objectName} disabled={readOnly} scope="record" onBlockingIssuesChange={(n) => reportCel('disabled', n)} /> + {/* `roots` is the second declaration this pair owes, and it answers a + different question than `scope` does (objectui#9856). `scope` says + how the CEL is LINTED; `roots` says what the HOST binds, and + objectui#9645 could not derive the second from the first — so a + `scope="record"` mount that declares nothing inherits + `RECORD_CONDITION_ROOTS`, the set every host of a record-scoped + condition binds. These two are evaluated in the BROWSER, where + `buildExpressionScope` binds more than that, and + `CONDITION_HOST_BY_METADATA_TYPE` rules the `action` tier `client` + from a reading taken at that evaluator. Declared here rather than + defaulted, for the reason `RECORD_CONDITION_ROOTS` gives: the + component cannot see which host is on the other end. */} + onPatch({ visible: writeExpressionSource(draft.visible, v) })} objectName={objectName} disabled={readOnly} scope="record" roots={CLIENT_CONDITION_ROOTS} onBlockingIssuesChange={(n) => reportCel('visible', n)} /> + onPatch({ disabled: writeExpressionSource(draft.disabled, v) })} objectName={objectName} disabled={readOnly} scope="record" roots={CLIENT_CONDITION_ROOTS} onBlockingIssuesChange={(n) => reportCel('disabled', n)} /> {/* 7 ─ AI exposure */} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.clientMountRoots.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.clientMountRoots.test.tsx new file mode 100644 index 0000000000..5e872c63a9 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.clientMountRoots.test.tsx @@ -0,0 +1,266 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A CLIENT-evaluated `ConditionBuilder` mount advertises what its browser host + * really binds — objectui#9856, the declared cost of objectui#9645. + * + * ## The defect these cases reproduce + * + * objectui#9645 made a `scope="record"` mount advertise `RECORD_CONDITION_ROOTS` + * instead of the engine's whole default list, and that is right for a mount + * whose host this component cannot see: it is the set EVERY host of a + * record-scoped condition binds. The two action mounts are not that mount. + * `CONDITION_HOST_BY_METADATA_TYPE` rules the `action` tier `client` from a + * reading taken at its evaluator, and an action's `visible` / `disabled` is + * evaluated in the browser, where `buildExpressionScope` publishes the identity + * roots and the feature flags. Declaring nothing there cost the author the + * OFFER of every root beyond `record` — while the section's own hint text goes + * on promising predicates "over the record / user / ctx". + * + * ## The pins are DERIVED, not retyped — both directions + * + * ⛔ No case below spells the advertised list. The membership case rebuilds it + * from the two producers that decide it (`buildExpressionScope`, and the real + * `usePredicateRecordContext` hook rendered in a probe), so it reddens when + * either side moves rather than when someone forgets to update a copy. The + * lint case runs each member through the same path the editor lints with, so a + * root added here that the `record` scope REFUSES reddens as the objectui#8167 + * defect it would be, rather than reaching an author. + * + * ## The narrowing half is pinned too + * + * This list DROPS `previous` relative to the record-scoped default, because no + * browser host binds it. That is the same ruling as the additions and the half + * a reader is most likely to take for an oversight, so it has its own case at + * the mount rather than only in a docblock. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +// Module-scope import of the CEL engine, per AGENTS.md's flaky-test rule and +// for the reason `ConditionBuilder.mountRoots.test.tsx` states: the lint and +// the scope introspection both run behind a dynamic +// `import('@objectstack/formula')` inside `celAuthoring`, and a cold first load +// has been measured near a `waitFor`'s whole budget. The specifier must match +// `loadFormula`'s exactly — ESM caches by resolved specifier. +import '@objectstack/formula'; + +// objectui#4697 — this inspector calls `useObjectFields(objectName)` / +// `useObjectOptions()` unconditionally, so a mount-time fetch would escape to +// the real network. +const state = vi.hoisted(() => ({ + metadataClient: { get: vi.fn(async () => undefined), list: vi.fn(async () => [] as unknown[]) }, +})); +vi.mock('../useMetadata', () => ({ + useMetadataClient: () => state.metadataClient, +})); + +import { usePredicateRecordContext } from '@object-ui/react'; +import { buildExpressionScope } from '../../../providers/ExpressionProvider'; +import { CLIENT_CONDITION_ROOTS, RECORD_CONDITION_ROOTS } from './ConditionBuilder'; +import { ActionDefaultInspector } from './ActionDefaultInspector'; +import { conditionRootsForMetadataType } from '../conditionScope'; +import { lintCelPredicate } from '../celAuthoring'; + +afterEach(cleanup); + +/** `CelPredicateField` renders its editor as a combobox TEXTAREA. */ +function rawEditorIn(root: HTMLElement): HTMLTextAreaElement { + fireEvent.click(within(root).getByText('Expression')); + return within(root) + .getAllByRole('combobox') + .find((el) => el.tagName === 'TEXTAREA') as HTMLTextAreaElement; +} + +/** A builder's own root, located by its label — this inspector mounts two. */ +function builderLabelled(label: string): HTMLElement { + return screen.getByText(label).parentElement!.parentElement! as HTMLElement; +} + +/** The LABEL of each open suggestion — the first span; the second is its kind tag. */ +function offeredLabels(): string[] { + return screen + .queryAllByRole('option') + .map((o) => (o.querySelector('span')?.textContent ?? '').trim()); +} + +/** + * The shortest prefix that REACHES `root` — derived from the root itself. + * + * `filterCandidates` excludes an exact match, so asking for `os` would prove + * nothing whatever the editor offers. Computing the prefix instead of tabling + * it is what lets the cases below loop over the advertised list rather than + * carry a second copy of it keyed by hand. + */ +const prefixFor = (root: string) => root.slice(0, Math.max(1, Math.min(3, root.length - 1))); + +/** + * Prove the suggestion machinery works in THIS render, then hand back the box. + * + * ⚠️ Load-bearing, not politeness. The identifier catalog arrives + * asynchronously and a menu that has not opened YET offers nothing — which + * would satisfy every "is not offered" assertion below no matter what the + * editor advertises. `record` is the root every arm of this file keeps, so + * completing it is the warm-up that cannot beg any question under test. + */ +async function warmedEditor( + user: ReturnType, + label: string, +): Promise { + const box = rawEditorIn(builderLabelled(label)); + await user.click(box); + await user.type(box, 'rec'); + expect(await screen.findByRole('option', { name: /record/ }, { timeout: 4000 })).toBeTruthy(); + return box; +} + +/** What a warmed editor offers for `prefix`. */ +async function offeredFor( + user: ReturnType, + box: HTMLTextAreaElement, + prefix: string, +): Promise { + await user.clear(box); + await user.type(box, prefix); + return offeredLabels(); +} + +function ActionHarness() { + const [draft, setDraft] = React.useState>({ + name: 'approve', + label: 'Approve', + type: 'script', + objectName: 'invoice', + }); + return ( + setDraft((d) => ({ ...d, ...patch }))} + readOnly={false} + locale={'en-US' as never} + /> + ); +} + +/** Renders the real hook so its binding is MEASURED rather than quoted. */ +function RecordBindingProbe({ onRead }: { onRead: (keys: string[]) => void }) { + const bag = usePredicateRecordContext({ id: 'inv-1', status: 'open' }); + onRead(Object.keys(bag)); + return null; +} + +function recordContextKeys(): string[] { + let keys: string[] = []; + render( { keys = k; }} />); + cleanup(); + return keys; +} + +/* ── The advertised list, read against its producers ───────────────────── */ + +describe('CLIENT_CONDITION_ROOTS ↔ what a browser host binds (objectui#9856)', () => { + it('is exactly the two producers’ union — rebuilt here, never retyped', () => { + // The whole list, derived: the row binding comes from the real hook (a + // probe render, so the measurement is of the code and not of a sentence + // about it) and the ambient roots are the keys the shell's own scope + // builder returns. A binding added or removed on either side reddens here, + // which is the failure mode a hand-copied list cannot have. + const bound = [...recordContextKeys(), ...Object.keys(buildExpressionScope())]; + expect([...CLIENT_CONDITION_ROOTS].sort()).toEqual([...bound].sort()); + }); + + it('advertises no root the record scope REFUSES — every member lints clean', async () => { + // The objectui#8167 direction, and the reason this list is not simply "what + // the host binds": a root this editor offers that its own linter rejects + // costs the author the write, which is strictly worse than the offer this + // card restores. Derived from the list, so a member added tomorrow is + // linted tomorrow. + for (const root of CLIENT_CONDITION_ROOTS) { + const issues = await lintCelPredicate(`${root}.status == 'done'`, { scope: 'record' }); + expect( + issues.filter((i) => i.severity === 'error'), + `advertised root "${root}" must be accepted at scope record`, + ).toEqual([]); + } + }); + + it('really is wider than the default it replaces — otherwise the mounts below prove nothing', () => { + // Non-vacuity. Both cases above would pass against a list that said exactly + // what `RECORD_CONDITION_ROOTS` says, and so would every "offers X" case at + // the mount if X happened to be `record`. + expect(CLIENT_CONDITION_ROOTS.filter((r) => !RECORD_CONDITION_ROOTS.includes(r)).length) + .toBeGreaterThan(0); + }); + + it('drops `previous`, and neither producer binds it', () => { + // The narrowing half, derived from the same two producers rather than + // asserted. `previous` is bound by the SERVER hosts of a record-scoped + // condition, which is why the default carries it; offering it where nothing + // answers it is the `app` shape objectui#8155 ruled on. + const bound = [...recordContextKeys(), ...Object.keys(buildExpressionScope())]; + expect(bound).not.toContain('previous'); + expect(CLIENT_CONDITION_ROOTS).not.toContain('previous'); + // …and it IS in the default, so this is a real difference and not a shared + // absence that would hold however the two lists were written. + expect(RECORD_CONDITION_ROOTS).toContain('previous'); + }); +}); + +/* ── The mounts — an action's `visible` / `disabled` ───────────────────── */ + +describe('ActionDefaultInspector — the browser-evaluated mounts offer what they bind (objectui#9856)', () => { + for (const label of ['Visible when', 'Disabled when'] as const) { + it(`offers every advertised root in "${label}"`, async () => { + // THE CASE THIS CARD IS FOR. Before the declaration these two mounts + // forwarded no `roots`, so `scope="record"` narrowed them to the set + // every host binds and an author lost `user.*` / `os.*` from the dropdown + // while the host that runs the predicate really does bind them. The loop + // is over the advertised list itself, so a root added to it has to be + // reachable at the mount before this stays green. + const user = userEvent.setup(); + render(); + const box = await warmedEditor(user, label); + for (const root of CLIENT_CONDITION_ROOTS) { + expect(await offeredFor(user, box, prefixFor(root)), `"${root}" at "${label}"`) + .toContain(root); + } + }); + } + + it('still withholds `previous`, which no browser host answers', async () => { + // The narrowing, at the mount. A root the editor offers and the evaluator + // leaves unbound builds a predicate that can only fault, and this is the + // one root this declaration takes away — so it is pinned where an author + // would meet it, not only where it is declared. + const user = userEvent.setup(); + render(); + const box = await warmedEditor(user, 'Visible when'); + expect(await offeredFor(user, box, prefixFor('previous'))).not.toContain('previous'); + }); +}); + +/* ── The table's third derivation, at its own seam ─────────────────────── */ + +describe('conditionRootsForMetadataType — the per-type derivation (objectui#9856)', () => { + it('hands back the advertised list ITSELF for a client-evaluated type', () => { + // Identity, not equality: a second list that merely looks the same is the + // drift this codebase has already paid for at three other controls. + expect(conditionRootsForMetadataType('action')).toBe(CLIENT_CONDITION_ROOTS); + }); + + it('declares nothing for a server-evaluated type, and nothing for an unmeasured one', () => { + // The must-not-widen half. `undefined` leaves the builder's record-scoped + // default exactly where objectui#9645 put it, so the narrowing this card + // does not touch cannot leak away through the new member. + expect(conditionRootsForMetadataType('hook')).toBeUndefined(); + expect(conditionRootsForMetadataType('validation')).toBeUndefined(); + expect(conditionRootsForMetadataType('page')).toBeUndefined(); + expect(conditionRootsForMetadataType('a-type-this-build-never-heard-of')).toBeUndefined(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx index 87a5b91b56..42e2035a40 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx @@ -241,6 +241,57 @@ const REFERENCE_RE = new RegExp( */ export const RECORD_CONDITION_ROOTS = ['record', 'previous']; +/** + * Scope roots a mount ADVERTISES when its predicate is evaluated in the + * BROWSER — the client half of the ruling {@link RECORD_CONDITION_ROOTS} + * states for the server (objectui#9856). + * + * ## Why the record-scoped default is wrong at these mounts, and only these + * + * {@link RECORD_CONDITION_ROOTS} is the set EVERY host of a record-scoped + * condition binds, which is the only safe answer for a mount whose host this + * component cannot see. It is not the RIGHT answer for a mount whose host is + * KNOWN to bind more. An action's `visible` / `disabled` is evaluated in the + * browser: `buildExpressionScope` (`ExpressionProvider`) publishes the identity + * roots and the deployment feature flags, and the row arrives separately + * through `usePredicateRecordContext`. Such a mount forwarding no `roots` loses + * the offers its own host really answers — the cost objectui#9645 declared on + * its face and left for this card to pay. + * + * Which tier evaluates which metadata type is NOT re-derived here: + * `CONDITION_HOST_BY_METADATA_TYPE` (`conditionScope.ts`) rules it per type + * from readings taken at each evaluator, and this list is what its `client` + * verdict declares. + * + * ## Every member, and the producer it is read back from + * + * `record` is the one binding `usePredicateRecordContext` makes — it returns + * `{ record }` and nothing else. The rest are exactly the keys + * `buildExpressionScope` returns. ⛔ Neither half is retyped from a docblock: + * `ConditionBuilder.clientMountRoots.test.tsx` rebuilds the list from both + * producers, so the reading fails when either side moves rather than leaving + * this paragraph to rot. + * + * ## `previous` is deliberately NOT here — this list narrows as well as widens + * + * It is the one root this list DROPS relative to the record-scoped default, and + * the drop is the same ruling as the additions rather than an exception to it. + * No browser host binds it: `usePredicateRecordContext` binds the row alone and + * `buildExpressionScope` publishes no `previous`. Offering it here would be the + * `app` shape objectui#8155 ruled on — an editor advertising a root nothing + * answers — and the predicate an author built from it could only fault. The + * pin derives its absence from the two producers rather than asserting it. + * + * ## Every member must also LINT clean — the objectui#8167 direction + * + * A root advertised here that the `record` scope REFUSES would re-create that + * defect at the other door: the engine refuses `org` and `app` at this scope + * today, and an editor that offers what its own linter rejects costs the author + * the write. So the pin lints each member through the same path the editor + * uses, rather than snapshotting a list. + */ +export const CLIENT_CONDITION_ROOTS = ['record', 'current_user', 'user', 'ctx', 'os', 'features']; + /** * CEL's own word-shaped literals. Spelled like identifiers, bound by nobody, so * {@link celRootsMentioned} must not read one as a root. diff --git a/packages/app-shell/src/views/metadata-admin/widgets.tsx b/packages/app-shell/src/views/metadata-admin/widgets.tsx index ddf6946ec2..b705ebad7e 100644 --- a/packages/app-shell/src/views/metadata-admin/widgets.tsx +++ b/packages/app-shell/src/views/metadata-admin/widgets.tsx @@ -179,6 +179,29 @@ export interface WidgetContext { * scope silently claimed `flattened`, a missing vocabulary claims nothing. */ conditionSubjects?: ReadonlyArray<{ value: string; label?: string }>; + /** + * The scope roots a condition editor at this host may OFFER (objectui#9856) — + * `undefined` to declare no narrowing. + * + * ## Why a THIRD member, and not a widening of either of the two above + * + * `conditionScope` is how the predicate is LINTED and `conditionSubjects` is + * what the row builder's dropdown may offer. This is what the RAW editor's + * autocomplete may offer, and it is the only one of the three whose default + * is already a narrowing: `ConditionBuilder` answers a `scope="record"` mount + * with `RECORD_CONDITION_ROOTS` — the set every host of a record-scoped + * condition binds — so the tier that needs a declaration here is the CLIENT + * one, the mirror of the tier `conditionSubjects` narrows. One member could + * not have carried both directions. + * + * ⛔ Not this widget's own knob, for the reason the two above give: a host + * editing one fixed surface states its verdict, and a host editing many + * derives it with `conditionRootsForMetadataType`. + * + * `undefined` changes nothing — the builder keeps the default its `scope` + * implies, which is what every mount here offered before this member existed. + */ + conditionRoots?: string[]; /** Names of all object metadata records (for `ref:object`, `object-selector`). */ objectNames?: LoadState; /** @@ -2651,6 +2674,12 @@ function ConditionWidget({ value, onChange, readOnly, context, ariaLabelledBy }: // construction — an omitted `subjects` leaves `ConditionBuilder` on // `CONTEXT_SUBJECTS`, exactly what every mount here offered before. const conditionSubjects = context?.conditionSubjects; + // objectui#9856 — the THIRD half, and the one that widens rather than + // narrows: the builder already answers `scope="record"` with the roots every + // host of a record-scoped condition binds, so a client-evaluated tier is the + // one that has to say it binds more. `undefined` is the unchanged case by + // construction — it leaves that default exactly where objectui#9645 put it. + const conditionRoots = context?.conditionRoots; return ( // `ConditionBuilder` is a multi-control composite (field / operator / value // rows plus add-condition buttons) shared with the curated inspectors, so @@ -2679,6 +2708,7 @@ function ConditionWidget({ value, onChange, readOnly, context, ariaLabelledBy }: disabled={readOnly} scope={conditionScope} subjects={conditionSubjects ? { context: conditionSubjects } : undefined} + roots={conditionRoots} /> )}