diff --git a/.changeset/9953-condition-widget-context-derivation.md b/.changeset/9953-condition-widget-context-derivation.md new file mode 100644 index 0000000000..49d210ae6c --- /dev/null +++ b/.changeset/9953-condition-widget-context-derivation.md @@ -0,0 +1,30 @@ +--- +'@object-ui/app-shell': patch +--- + +Derive the condition editor's subject vocabulary per metadata type at the generic +metadata-admin mount (objectui#9953). + +`CONDITION_SCOPE_BY_METADATA_TYPE` rules `action`, `hook` and `validation` all +`'record'`, and each verdict is right — every one of those evaluators binds the row +as the `record` root. But that table answers how a predicate is **linted**, not what +its host **binds**, and on the second question the three disagree: a hook's +`condition` and a validation rule's guard are evaluated on the server against +`record` and `previous` alone, while an action's `visible` is evaluated in the +browser, where `user` is bound. + +The generic `ConditionWidget` mount is polymorphic over all of them, so it took the +default subject vocabulary — `record.id` plus `user.id` / `user.email` / `user.role` +/ `user.isAdmin` — for every type. Measured: a hook has a registered default +inspector but **no** registered preview, so `ResourceEditPage` renders its plain +whole-draft form and the curated `HookDefaultInspector` (which hides `condition` from +its own fallback) never runs. An author editing a hook could therefore pick +`user.isAdmin` from the dropdown, watch it lint clean at record scope, and have the +hook wrapper throw on every write the condition was supposed to gate. + +`conditionSubjectsForMetadataType` — the sibling of `conditionScopeForMetadataType` +— now derives that vocabulary from the type on screen, backed by a table of which +host evaluates each type's condition. Server-evaluated types get the narrowed list +the two curated inspectors already declare; the client-evaluated tier keeps the +default, because `user` really is bound there and narrowing it would take a working +subject away. A type with no measured host declares nothing and is unchanged. diff --git a/packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionSubjects.test.tsx b/packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionSubjects.test.tsx new file mode 100644 index 0000000000..c864518d1d --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/ConditionWidget.conditionSubjects.test.tsx @@ -0,0 +1,283 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The generic condition mount offers the subjects ITS OWN metadata type's + * evaluator binds — objectui#9953, the polymorphic mount the two curated ones + * could not be copied onto. + * + * ## The defect these cases reproduce + * + * `CONDITION_SCOPE_BY_METADATA_TYPE` rules `action`, `hook` and `validation` + * all `'record'`, and all three verdicts are right — every one of those + * evaluators binds the row as the `record` ROOT. But that table answers how a + * predicate is LINTED, and the hosts disagree about something it does not + * carry: what they BIND. A hook's condition and a validation rule's guard are + * evaluated on the SERVER against `record` and `previous` alone; an action's + * `visible` is evaluated in the BROWSER, where `user` really is bound. + * + * So the one-line `subjects={{ context: RECORD_CONDITION_SUBJECTS }}` the two + * curated inspectors took could not be copied here a third time: at this mount + * it would be right for a hook and would take a working subject away from an + * action. `RECORD_CONDITION_SUBJECTS` states the same refusal from the + * component's side — it is declared by a mount, never derived from + * `scope === 'record'`. `conditionSubjectsForMetadataType` is the missing half: + * the per-type derivation that lets one mount answer for many hosts. + * + * ## Reachability is MEASURED here, not assumed + * + * The card left it open whether a server-evaluated condition reaches this + * generic mount at all, since `HookDefaultInspector` hides `condition` from its + * own fallback form. It does reach it, by the route the first case takes: + * `hook` has no registered preview, so `ResourceEditPage` renders its plain + * branch — the whole-draft `SchemaForm` carrying the derived `WidgetContext` — + * and the curated inspector, which lives on the canvas branch, never runs. That + * case mounts the real host so the route is re-measured on every run rather + * than recorded in this paragraph. + * + * ## Derived, not retyped + * + * The narrowed list is never spelled out here. Each case reads what the default + * vocabulary actually renders, then asserts about ROOTS against + * `RECORD_CONDITION_ROOTS` — the binding set the server hosts were measured to + * have. A subject added to the default list reddens these cases if the hook + * mount starts offering it, and the client-tier case reddens if a narrowing + * ever reaches the mounts that legitimately bind `user`. + */ + +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'; +import { MemoryRouter } from 'react-router-dom'; + +// Module-scope import of the CEL engine, per AGENTS.md's flaky-test rule: the +// lint behind `celAuthoring`'s dynamic `import('@objectstack/formula')` is +// mounted by the raw editor this page also renders, 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'; + +/** A hook whose condition PARSES into one builder row, so a subject dropdown exists. */ +const HOOK = { + name: 'stamp', + label: 'Stamp', + object: 'invoice', + events: ['beforeInsert'], + condition: "record.id == 'x'", +}; + +/** + * The server's `/meta/types` row for `hook`, reduced to what these cases read. + * + * ⚠️ A fixture, and the honest part to name: these cases pin the DERIVATION and + * the route, not which keys a hook's published schema carries. + */ +const HOOK_ENTRY = { + type: 'hook', + name: 'hook', + label: 'Hook', + // What makes the item writable, and therefore what makes an Edit button exist. + allowOrgOverride: true, + schema: { + type: 'object', + properties: { + label: { type: 'string', title: 'Label' }, + // Routed to the condition widget BY NAME — `condition` is in + // `SchemaForm`'s `CONDITION_FIELD_NAMES`. + condition: { type: 'string', title: 'Run only when' }, + }, + }, +}; + +const mockClient = { + list: vi.fn(async () => []), + listDrafts: vi.fn(async () => []), + layered: vi.fn(async () => ({ effective: HOOK, code: HOOK, editable: true })), + getDraft: vi.fn(async () => null), + get: vi.fn(async () => null), + saveDraft: vi.fn(async () => ({})), +}; + +// One mock serves both harnesses below: the page needs the type row, and +// `ConditionBuilder` calls `useObjectFields` unconditionally (objectui#4697), so +// an unmocked client would let a mount-time fetch escape to the real network. +vi.mock('./useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ entries: [HOOK_ENTRY] }), + }; +}); + +import { SchemaForm } from './SchemaForm'; +import type { WidgetContext } from './widgets'; +import { + CONDITION_SCOPE_BY_METADATA_TYPE, + CONDITION_HOST_BY_METADATA_TYPE, + conditionSubjectsForMetadataType, +} from './conditionScope'; +import { + RECORD_CONDITION_ROOTS, + RECORD_CONDITION_SUBJECTS, +} from './inspectors/ConditionBuilder'; +import { MetadataResourceEditPage } from './ResourceEditPage'; +// The load-time registrations, exactly as the package entry runs them, so the +// branch the page takes below is the branch production takes. +import './register-builtins'; + +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: HOOK.condition, + }); + return ( + setValue(next as Record)} + widgetContext={context} + /> + ); +} + +/** The root of a subject spelling — `user` for `user.isAdmin`. */ +const rootOf = (subject: string) => subject.split('.')[0]; + +/** + * Open the subject dropdown inside the condition group and read what it offers. + * + * The subject `Select` is the first combobox in the row; Radix renders its items + * into a portal, so the options are read off the document rather than the group. + */ +async function offeredSubjects(): Promise { + const group = await screen.findByRole('group', { name: /Run only when/ }); + await userEvent.click(within(group).getAllByRole('combobox')[0]); + return (await screen.findAllByRole('option')).map((o) => o.textContent ?? ''); +} + +/** Open the hook editor at the real host and enter edit mode. */ +async function openHookEditor(): Promise { + render( + + + , + ); + // The host renders an Edit affordance twice (page header and toolbar); either + // enters edit mode, and WHICH one is not these cases' subject. + fireEvent.click((await screen.findAllByRole('button', { name: 'Edit' }))[0]); +} + +/* ── The measurement: the route, and what it offers at the far end ───────── */ + +describe('the generic condition mount, editing a SERVER-evaluated type (objectui#9953)', () => { + it('reaches the generic widget for a hook at all — the route the card left unmeasured', async () => { + // Reachability, taken rather than reasoned about. `hook` has a registered + // default inspector but NO registered preview, and the inspector panel that + // would host it only exists on the canvas branch — so the plain branch runs + // and routes `condition` to the generic widget by name. If that ever stops + // being true, this case is what says so, and the narrowing below becomes + // dead weight rather than silently guarding nothing. + await openHookEditor(); + const group = await screen.findByRole('group', { name: /Run only when/ }); + // The builder's raw/visual toggle is its signature — the plain `none` + // editor has none, and neither does a curated inspector's hidden field. + expect(within(group).getByText('Expression')).toBeInTheDocument(); + }); + + it('offers no subject whose root the server host leaves unbound', async () => { + // THE GATE. Derived from the binding set the hook wrapper was measured to + // have, not from a copy of the narrowed list: `wrapDeclarativeHook` + // evaluates the condition against `record` and `previous` and throws when + // it cannot, so a subject under any other root compiles a row that can only + // abort the write. + await openHookEditor(); + const offered = await offeredSubjects(); + expect(offered.length).toBeGreaterThan(0); + expect(offered.filter((s) => !RECORD_CONDITION_ROOTS.includes(rootOf(s)))).toEqual([]); + }); + + it('still offers the subject the server host DOES bind', async () => { + // The must-not-break half of every narrowing. A mount that offered nothing + // would pass the case above for free. + await openHookEditor(); + const offered = await offeredSubjects(); + for (const s of RECORD_CONDITION_SUBJECTS) expect(offered).toContain(s.value); + }); +}); + +/* ── The control: the client tier keeps the subject it really binds ──────── */ + +describe('the generic condition mount, editing a CLIENT-evaluated type (objectui#9953)', () => { + it('keeps every default subject, including the roots only a browser host binds', async () => { + // This is the case that makes the derivation a derivation rather than a + // third copy of the curated declaration. An action's `visible` is evaluated + // where `buildExpressionScope` binds `user`, so narrowing here would take a + // subject an author can legitimately pick — and a row that really matches — + // away from a working tier. It is fed the derivation's OWN answer for that + // tier, so it reddens if the narrowing ever reaches it. + render( + , + ); + const offered = await offeredSubjects(); + const unbound = offered.filter((s) => !RECORD_CONDITION_ROOTS.includes(rootOf(s))); + // Non-vacuity: the default vocabulary really does carry subjects the server + // hosts do not bind, which is what the case above has to remove and this + // one has to keep. Without this the two cases could both pass against an + // empty dropdown. + expect(unbound.length).toBeGreaterThan(0); + }); +}); + +/* ── The table, and the seam nothing else watches ────────────────────────── */ + +describe('conditionSubjectsForMetadataType — the per-type derivation (objectui#9953)', () => { + it('hands back the ruled list itself for every server-evaluated type', () => { + // Identity, not equality: a second list that merely looks the same is the + // drift this codebase has already paid for three times — autocomplete, + // subject dropdown and placeholder, each with its own literal. + for (const [type, host] of Object.entries(CONDITION_HOST_BY_METADATA_TYPE)) { + if (host !== 'server') continue; + expect(conditionSubjectsForMetadataType(type)).toBe(RECORD_CONDITION_SUBJECTS); + } + }); + + it('declares nothing for a client-evaluated type, and nothing for an unmeasured one', () => { + // `undefined` is the unchanged arm on purpose: the builder keeps its own + // default, so a tier with no reading behind it is left exactly as it was + // rather than narrowed on a guess. + for (const [type, host] of Object.entries(CONDITION_HOST_BY_METADATA_TYPE)) { + if (host === 'server') continue; + expect(conditionSubjectsForMetadataType(type)).toBeUndefined(); + } + expect(conditionSubjectsForMetadataType('page')).toBeUndefined(); + expect(conditionSubjectsForMetadataType('a-type-this-build-never-heard-of')).toBeUndefined(); + }); + + it('has a measured host for every type the scope table rules a row surface', () => { + // What makes a new row-surface type LOUD instead of a silent inheritance of + // `user.*`. `'record'` is exactly the set of tiers where this mount renders + // a subject dropdown at all, so it is the set that needs a reading; the + // `flattened` and `none` tiers deliberately have none. + const rowSurfaces = Object.entries(CONDITION_SCOPE_BY_METADATA_TYPE) + .filter(([, scope]) => scope === 'record') + .map(([type]) => type); + expect(rowSurfaces.length).toBeGreaterThan(0); + for (const type of rowSurfaces) { + expect(CONDITION_HOST_BY_METADATA_TYPE).toHaveProperty(type); + } + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index a88941c103..2f3011a7bd 100644 --- a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx @@ -104,7 +104,10 @@ import { type ObjectFieldOption, type WidgetContext, } from './widgets.js'; -import { conditionScopeForMetadataType } from './conditionScope.js'; +import { + conditionScopeForMetadataType, + conditionSubjectsForMetadataType, +} from './conditionScope.js'; import { mapLoaded, usePickerLoad } from './loadState.js'; import { useMetadataClient, @@ -973,6 +976,13 @@ function MetadataResourceEditPageImpl({ // would put one tier's scope in front of every other tier's authors, // which is the shape the ruling refused by name. conditionScope: conditionScopeForMetadataType(type), + // objectui#9953 — the scope above and this vocabulary answer two + // DIFFERENT questions, and this page is where both have to be answered + // per type: `action`, `hook` and `validation` all rule `record`, but only + // the first is evaluated where `user` is bound. Deriving it here is the + // same move as the line above, applied to the fact that line cannot + // carry. + conditionSubjects: conditionSubjectsForMetadataType(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 12070c5290..9885602486 100644 --- a/packages/app-shell/src/views/metadata-admin/conditionScope.ts +++ b/packages/app-shell/src/views/metadata-admin/conditionScope.ts @@ -1,5 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +import { RECORD_CONDITION_SUBJECTS } from './inspectors/ConditionBuilder.js'; + /** * Which lint scope a SCHEMA-DRIVEN condition editor claims, decided by the * metadata type being edited (objectui#8167). @@ -165,3 +167,108 @@ export function conditionScopeForMetadataType(type: string): ConditionScope { const table: Record = CONDITION_SCOPE_BY_METADATA_TYPE; return table[type] ?? 'none'; } + +/* ========================================================================== */ +/* The SECOND question: who evaluates the condition (objectui#9953) */ +/* ========================================================================== */ + +/** + * Which host evaluates a condition authored at a mount editing this metadata + * type. + * + * ## Why {@link ConditionScope} cannot answer this, and a second table must + * + * The scope table gives `action`, `hook` and `validation` the same `'record'`, + * and that verdict is right for all three — each evaluator binds the row as the + * `record` ROOT. But `'record'` is a claim about how the CEL is LINTED, not a + * claim about what the host BINDS, and on that second question the three + * disagree: two are evaluated on the SERVER, one in the BROWSER. So one value + * covers two different binding sets, and any single subject vocabulary declared + * at the generic mount would be correct for one tier and wrong for the other. + * + * `RECORD_CONDITION_SUBJECTS` refuses to derive its narrowing from + * `scope === 'record'` for precisely this reason, and says so in its own words. + * This table is the other end of that refusal: the fact the component cannot + * see, supplied by the one place that knows which metadata type is on screen. + */ +export type ConditionEvaluationHost = 'server' | 'client'; + +/** + * The measured host per metadata type, PARTIAL over the ruled table on purpose. + * + * ## Why partial, and what keeps it honest anyway + * + * A row here is a MEASUREMENT of a real evaluator, so a type whose condition + * nobody has put to an evaluator must have no row rather than a guessed one. + * Every absent type therefore derives no narrowing at all and the builder keeps + * its own default vocabulary — byte for byte what it offered before this table + * existed. + * + * Two things stop that from becoming the silent-default defect this card is + * about. The `satisfies Partial>` below ties the + * KEY SET to the ruled scope table, so a key that is not a ruled metadata type + * is a compile error here. And `ConditionWidget.conditionSubjects.test.tsx` + * requires a row for every type the scope table rules `'record'` — the tiers + * where a subject vocabulary is actually offered — so a new row-surface type + * reddens there instead of inheriting `user.*` unremarked. + * + * ## The readings behind each row, re-derived at source in objectstack + * + * • `hook` → `server`. `wrapDeclarativeHook`'s pre-compiled condition calls + * `ExpressionEngine.evaluate(expr, { record: record ?? {}, previous })` + * and throws when the result is not `ok`, so an unevaluable predicate aborts + * the operation rather than resolving false. Two bindings, no `user`. + * • `validation` → `server`. A rule's `condition` goes through `checkPredicate` + * and its `when` through `checkConditional`; both evaluate against + * `{ record, previous }` and both return an `unevaluableRuleError` when the + * predicate cannot be evaluated — the rule validator's own log line calls + * that outcome "rejected, not skipped". Two bindings, no `user`. + * • `action` → `client`. An action's `visible` / `disabled` are evaluated in + * the browser, where `buildExpressionScope` (`ExpressionProvider`) returns a + * bag carrying `user` (alongside `current_user`, `ctx`, `os` and `features`) + * and the row arrives through `usePredicateRecordContext`. `user` is a + * subject an author can legitimately pick here, so this row exists to keep + * the narrowing OFF this tier — an omission would read the same way and + * claim nothing. + * + * ⛔ Do not add a row from a docblock or a card. A row is a reading taken at + * the evaluator. + */ +export const CONDITION_HOST_BY_METADATA_TYPE = { + action: 'client', + hook: 'server', + validation: 'server', +} satisfies Partial>; + +/** The host that evaluates `type`'s condition, or `undefined` when unmeasured. */ +export function conditionHostForMetadataType( + type: string, +): ConditionEvaluationHost | undefined { + const table: Partial> = + CONDITION_HOST_BY_METADATA_TYPE; + return table[type]; +} + +/** + * The context subjects a host editing `type` must declare on its + * {@link WidgetContext}, or `undefined` to declare nothing. + * + * The sibling of {@link conditionScopeForMetadataType}, and the one place the + * per-type answer is derived: `ResourceEditPage` is polymorphic over every + * metadata type, so it can no more hard-code a vocabulary than it could + * hard-code a scope. + * + * `undefined` is a decision and not an absence, the same way `'none'` is in the + * scope table — it means this mount declares no narrowing, so `ConditionBuilder` + * keeps `CONTEXT_SUBJECTS`, which is what every generic mount offered before + * this derivation existed. The `client` tier takes that arm deliberately: its + * predicates really are evaluated where `user` is bound, so narrowing there + * would take a working subject away rather than withdraw an unbound one. + */ +export function conditionSubjectsForMetadataType( + type: string, +): ReadonlyArray<{ value: string; label?: string }> | undefined { + return conditionHostForMetadataType(type) === 'server' + ? RECORD_CONDITION_SUBJECTS + : undefined; +} diff --git a/packages/app-shell/src/views/metadata-admin/widgets.tsx b/packages/app-shell/src/views/metadata-admin/widgets.tsx index 3040ca18a2..ddf6946ec2 100644 --- a/packages/app-shell/src/views/metadata-admin/widgets.tsx +++ b/packages/app-shell/src/views/metadata-admin/widgets.tsx @@ -154,6 +154,31 @@ export interface WidgetContext { * carries the per-tier rulings and the reading behind each one. */ conditionScope: ConditionScope; + /** + * The context subjects a condition editor at this host may offer + * (objectui#9953) — `undefined` to declare no narrowing. + * + * ## Why this is a SECOND member and not a widening of `conditionScope` + * + * `conditionScope` is a claim about how the CEL is LINTED; it is not a claim + * about what the host BINDS, and the two come apart on exactly the tiers this + * widget serves. `action`, `hook` and `validation` all rule `'record'`, yet a + * hook's condition is evaluated by a server host binding `record` and + * `previous` alone while an action's `visible` is evaluated in the browser, + * where `user` really is bound. One value could not have carried both + * answers, which is why `RECORD_CONDITION_SUBJECTS` refuses to derive itself + * from `scope === 'record'`. + * + * ⛔ Not this widget's own knob, for the same reason `conditionScope` is not: + * a host editing one fixed surface states its verdict, and a host editing + * many derives it with `conditionSubjectsForMetadataType`. + * + * `undefined` changes nothing — `ConditionBuilder` keeps `CONTEXT_SUBJECTS`, + * so a host that declares no narrowing offers what it always offered. That is + * why this member is optional where `conditionScope` is required: a missing + * scope silently claimed `flattened`, a missing vocabulary claims nothing. + */ + conditionSubjects?: ReadonlyArray<{ value: string; label?: string }>; /** Names of all object metadata records (for `ref:object`, `object-selector`). */ objectNames?: LoadState; /** @@ -2620,6 +2645,12 @@ function ConditionWidget({ value, onChange, readOnly, context, ariaLabelledBy }: // and swapping the editor under a host that never asked for it would be this // change reaching mounts nobody ruled on. const conditionScope = context?.conditionScope; + // objectui#9953 — the OTHER half of the host's verdict, and a separate one: + // the scope above says how this predicate is linted, this says which subjects + // its evaluator actually binds. `undefined` is the unchanged case by + // construction — an omitted `subjects` leaves `ConditionBuilder` on + // `CONTEXT_SUBJECTS`, exactly what every mount here offered before. + const conditionSubjects = context?.conditionSubjects; return ( // `ConditionBuilder` is a multi-control composite (field / operator / value // rows plus add-condition buttons) shared with the curated inspectors, so @@ -2647,6 +2678,7 @@ function ConditionWidget({ value, onChange, readOnly, context, ariaLabelledBy }: fields={conditionFields} disabled={readOnly} scope={conditionScope} + subjects={conditionSubjects ? { context: conditionSubjects } : undefined} /> )}