diff --git a/.changeset/8167-conditionbuilder-mount-scope.md b/.changeset/8167-conditionbuilder-mount-scope.md new file mode 100644 index 0000000000..12c8f618a6 --- /dev/null +++ b/.changeset/8167-conditionbuilder-mount-scope.md @@ -0,0 +1,43 @@ +--- +'@object-ui/app-shell': patch +--- + +`ConditionBuilder` takes an optional `scope`, and three metadata-admin mounts now +declare `record` (objectui#8167). + +**The defect.** The builder's raw CEL editor had no `scope` prop at all, so every +mount fell through to `celAuthoring`'s own default — spelled `hint.scope ?? 'flattened'` +— and no caller could override it. A bare `status == 'done'` typed into an action's +**Visible when** therefore linted CLEAN and then never matched: `usePredicateRecordContext` +binds `record` and nothing else, and objectui#5741 Phase 2 retired the bare shorthand +on runtime record surfaces. This is objectui#7727's defect at a component +objectui#7727 does not touch; PR #8164 turned the same defect at the +conditional-formatting mount. + +**What changed.** + +- `scope?: 'record' | 'flattened'` is now an optional prop, forwarded verbatim to + `CelPredicateField`. Omitting it forwards `undefined`, so the engine hint is exactly + what it was and every mount that passes nothing is unchanged. It deliberately does + **not** derive the scope from `subjects.fieldPrefix` — that would silently decide the + mounts whose tier is still an open question. +- An action's **Visible when** and **Disabled when** (`ActionDefaultInspector`) declare + `scope="record"`. Conformance: the row-predicate canon in `@object-ui/core` + (`rowPredicateCanon.ts`) names `visible` / `disabled` on an action renderer as a row + surface in its own words. +- An object validation rule's `condition` (`ObjectValidationsPanel`) declares + `scope="record"`. The authority there is the server: objectql's rule validator + evaluates a `script` / `cross_field` condition with `{ record, previous }` and nothing + else, and since objectstack#4649 an unevaluable predicate is fail-CLOSED — so a bare + reference authored here did not merely fail to match, it rejected every write to the + object while the editor linted it green. + +**Author-visible effect.** At those three editors a bare field reference is now a +blocking lint error naming the `record.` rewrite, instead of a silent pass. The +row builder at those mounts was already emitting `record.`; this makes the raw +expression editor agree with the rows its own sibling mode produces. + +**Deliberately unchanged.** The page-block `visibleWhen`, hook `condition` and +schema-driven `ConditionWidget` mounts still pass nothing and still lint flattened. +Their tier is a real open question on objectui#8167 and an explicit value would be a +claim about it. The flow-node condition stays as-is — flow tier is not a row surface. 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 37c121c1b2..ceffbc6a26 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx @@ -668,8 +668,18 @@ export function ActionDefaultInspector({ {/* Both are `ExpressionInputSchema` in the spec (`disabled` as `boolean | ExpressionInput`), so a persisted action carries the ADR-0089 envelope — same read/write pair as the hook guard (#3218). */} - onPatch({ visible: writeExpressionSource(draft.visible, v) })} objectName={objectName} disabled={readOnly} onBlockingIssuesChange={(n) => reportCel('visible', n)} /> - onPatch({ disabled: writeExpressionSource(draft.disabled, v) })} objectName={objectName} disabled={readOnly} onBlockingIssuesChange={(n) => reportCel('disabled', n)} /> + {/* `scope="record"` is CONFORMANCE, not taste (objectui#8167). The + row-predicate canon in `@object-ui/core` (`rowPredicateCanon.ts`) + names an action renderer's `visible` / `disabled` as a row surface + in its own words, and `usePredicateRecordContext` binds `record` + and nothing else — so a bare `status == 'done'` can never match. + Without the scope these two editors linted it CLEAN: the default is + `celAuthoring`'s `hint.scope ?? 'flattened'`, which is right for RLS + 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)} /> {/* 7 ─ AI exposure */} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.mountScope.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.mountScope.test.tsx new file mode 100644 index 0000000000..2022578099 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.mountScope.test.tsx @@ -0,0 +1,263 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `ConditionBuilder`'s raw CEL editor must lint in the scope its MOUNT SITE + * evaluates in — objectui#8167, the objectui#7727 defect at a component + * objectui#7727 does not touch. + * + * ## The defect these cases reproduce + * + * The scope was not a prop at all. Every mount fell through to `celAuthoring`'s + * own default — spelled `hint.scope ?? 'flattened'` — and nothing could + * override it, so a bare `status == 'done'` typed into an action's **Visible + * when** linted CLEAN. It never matches: `usePredicateRecordContext` binds + * `record` and nothing else, and objectui#5741 Phase 2 retired the bare + * shorthand on runtime record surfaces, so the predicate is simply false + * forever with no author-time signal anywhere. + * + * That is why the red leg below types a BARE SHORTHAND and asserts the editor + * rejects it, rather than asserting that a prop arrived. A "the prop is + * forwarded" assertion would pass against a scope value that means nothing to + * the engine; this one can only pass if the engine was actually asked the + * record-scoped question. + * + * ## Real engine, on purpose + * + * These cases run against the REAL `@objectstack/formula` — the same choice + * `ConditionalFormattingEditor.test.tsx` made when PR #8164 turned this same + * defect at the conditional-formatting mount. A stub would let the suite stay + * green while asking the engine the wrong question, which is precisely the + * failure being pinned. Measured on the installed engine, both directions: + * + * scope 'flattened' · `status == 'done'` -> ok, no findings + * scope 'record' · `status == 'done'` -> error, names `record.status` + * scope 'record' · `record.status == 'done'` -> ok, no findings + * + * ## The control half + * + * Two of them, because "unchanged" is the load-bearing half of the ruling on + * objectui#8167: three further mounts (page block, hook, and the schema-driven + * `ConditionWidget`) were deliberately left passing nothing while their tier + * question is open. `HookDefaultInspector` stands for that set as a real + * rendered mount, and a bare `ConditionBuilder` with no `scope` stands for the + * default itself. Both must still lint the bare shorthand CLEAN. If a later + * change makes the prop default to `'record'` — or derives it from + * `subjects.fieldPrefix` — these two go red, which is the point. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor, within } from '@testing-library/react'; + +// Module-scope import of the CEL engine, per AGENTS.md's flaky-test rule: the +// lint runs behind a dynamic `import('@objectstack/formula')` inside +// `celAuthoring`, and a cold first load has been measured near a `waitFor`'s +// whole budget. Paying it at import time takes it out of every bounded window +// below. The specifier must match `loadFormula`'s exactly — ESM caches by +// resolved specifier, and a different spelling warms a different entry. +import '@objectstack/formula'; + +// objectui#4697 — these inspectors call `useObjectFields(objectName)` / +// `useObjectOptions()` unconditionally, so a mount-time fetch would escape to +// the real network. The verdict under test does not depend on the catalog: the +// engine reports a bare reference from the SCOPE, not from the field list +// (measured with `fields: []` and with `fields: undefined` — identical). +const state = vi.hoisted(() => ({ + metadataClient: { get: vi.fn(async () => undefined), list: vi.fn(async () => [] as unknown[]) }, +})); +vi.mock('../useMetadata', () => ({ + useMetadataClient: () => state.metadataClient, +})); + +import { ConditionBuilder } from './ConditionBuilder'; +import { ActionDefaultInspector } from './ActionDefaultInspector'; +import { HookDefaultInspector } from './HookDefaultInspector'; +import { ObjectValidationsPanel } from '../../studio-design/ObjectValidationsPanel'; + +afterEach(cleanup); + +/** The bare shorthand the card is about: retired, clean under `flattened`. */ +const BARE = "status == 'done'"; +/** Its canonical twin — the must-not-break half of every narrowing. */ +const CANONICAL = "record.status == 'done'"; + +/** `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 — inspectors mount several. */ +function builderLabelled(label: string): HTMLElement { + return screen.getByText(label).parentElement!.parentElement! as HTMLElement; +} + +/** + * The editor REJECTED what was typed. + * + * `aria-invalid` is the structural assertion — it is what the editor sets from + * its own error count, and what a save gate counts. The message check is + * deliberately narrowed to the canonical spelling the engine prescribes rather + * than to its sentence: the fix an author must apply is the contract here, the + * wording is not. + */ +async function expectRejected(box: HTMLTextAreaElement) { + await waitFor(() => expect(box.getAttribute('aria-invalid')).toBe('true'), { timeout: 4000 }); + expect(await screen.findByText(/record\.status/, {}, { timeout: 4000 })).toBeTruthy(); +} + +/** The editor ACCEPTED what was typed — no error, and it says so. */ +async function expectAccepted(box: HTMLTextAreaElement) { + expect(await screen.findByText('Valid CEL', {}, { timeout: 4000 })).toBeTruthy(); + expect(box.getAttribute('aria-invalid')).not.toBe('true'); +} + +/* ── Mount 1 & 2 — an action's `visible` / `disabled` ──────────────────── */ + +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} + /> + ); +} + +describe("ActionDefaultInspector — an action guard is a ROW surface (objectui#8167)", () => { + // `rowPredicateCanon.ts` names `visible` / `disabled` on an action renderer + // as a row predicate in its own words, and that is the whole basis for these + // two: conformance, not taste. + for (const label of ['Visible when', 'Disabled when'] as const) { + it(`rejects the bare shorthand in "${label}" and names the record. fix`, async () => { + render(); + const box = rawEditorIn(builderLabelled(label)); + fireEvent.change(box, { target: { value: BARE } }); + await expectRejected(box); + }); + + it(`still accepts the canonical spelling in "${label}"`, async () => { + // The other half of a narrowing: rejecting the retired spelling must not + // cost the author the one they are being sent to. + render(); + const box = rawEditorIn(builderLabelled(label)); + fireEvent.change(box, { target: { value: CANONICAL } }); + await expectAccepted(box); + }); + } +}); + +/* ── Mount 3 — an object validation rule's `condition` ─────────────────── */ + +function ValidationsHarness() { + const [draft, setDraft] = React.useState>({ + name: 'invoice', + fields: { status: { type: 'text' } }, + validations: [ + { + type: 'script', + name: 'rule_a', + label: 'rule_a', + message: 'nope', + severity: 'error', + active: true, + }, + ], + }); + return ( + setDraft((d) => ({ ...d, ...patch }))} + /> + ); +} + +describe('ObjectValidationsPanel — the SERVER binds a rule condition to `record` (objectui#8167)', () => { + // objectql's rule validator evaluates a `script` / `cross_field` condition + // with `{ record, previous }` and nothing else, and since objectstack#4649 an + // unevaluable predicate there is fail-CLOSED. So a bare reference authored + // here does not merely fail to match — it rejects every write to the object, + // and this editor used to lint it clean. + it('rejects the bare shorthand in a rule condition and names the record. fix', async () => { + const { container } = render(); + const box = rawEditorIn(container as HTMLElement); + fireEvent.change(box, { target: { value: BARE } }); + await expectRejected(box); + }); + + it('still accepts the canonical spelling in a rule condition', async () => { + const { container } = render(); + const box = rawEditorIn(container as HTMLElement); + fireEvent.change(box, { target: { value: CANONICAL } }); + await expectAccepted(box); + }); +}); + +/* ── Controls — every mount that passes nothing is unchanged ───────────── */ + +function HookHarness() { + const [draft, setDraft] = React.useState>({ + name: 'stamp', + label: 'Stamp', + object: 'invoice', + events: ['beforeInsert'], + }); + return ( + setDraft((d) => ({ ...d, ...patch }))} + readOnly={false} + locale={'en-US' as never} + /> + ); +} + +function BareBuilderHarness() { + const [value, setValue] = React.useState(''); + return ( + + ); +} + +describe('mounts that pass no `scope` are byte-for-byte unchanged (objectui#8167)', () => { + it('a hook guard — an OUT mount — still lints the bare shorthand clean', async () => { + // Server-trigger tier. objectui#8167 left it passing nothing on purpose: + // an explicit value is a claim, and that claim is what is unsettled. This + // case is what makes "left alone" falsifiable rather than asserted. + render(); + const box = rawEditorIn(builderLabelled('Run only when (optional CEL)')); + fireEvent.change(box, { target: { value: BARE } }); + await expectAccepted(box); + }); + + it('the component default is still the engine default, not `record`', async () => { + // Omitting `scope` must forward `undefined`, so `celAuthoring`'s + // `hint.scope ?? 'flattened'` answers exactly what it answered before the + // prop existed. This reddens if the default is ever changed, or derived + // from `subjects.fieldPrefix`. + render(); + const box = rawEditorIn(screen.getByText('Condition').parentElement!.parentElement! as HTMLElement); + fireEvent.change(box, { target: { value: BARE } }); + await expectAccepted(box); + }); +}); 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 94761de738..ec84f2f778 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx @@ -234,7 +234,7 @@ function initFrom(value: string): { rows: Row[]; join: '&&' | '||'; raw: boolean return { rows: [], join: '&&', raw: !!value }; } -export function ConditionBuilder({ label, value, onCommit, objectName, fields: fieldsProp, disabled, onBlockingIssuesChange, subjects }: { +export function ConditionBuilder({ label, value, onCommit, objectName, fields: fieldsProp, disabled, onBlockingIssuesChange, subjects, scope }: { label?: string; value: string; onCommit: (cel: string) => void; @@ -259,6 +259,42 @@ export function ConditionBuilder({ label, value, onCommit, objectName, fields: f * record-scoped default every existing consumer relies on. */ subjects?: ConditionSubjectVocabulary; + /** + * Evaluation scope the raw-expression editor lints and completes against + * (objectui#8167) — forwarded verbatim to `CelPredicateField`, which mirrors + * `CelSchemaHint.scope`. + * + * ## Why this had to become a prop + * + * It was not one. Every mount ran the `celAuthoring` default, whose own + * spelling is `hint.scope ?? 'flattened'`, and no caller could say otherwise + * — so a bare `status == 'done'` typed into an action's **Visible when** + * linted CLEAN. It never matches: `usePredicateRecordContext` binds `record` + * and nothing else, and objectui#5741 Phase 2 retired the bare shorthand on + * runtime record surfaces. The row-predicate canon in `@object-ui/core` + * (`rowPredicateCanon.ts`) names an action renderer's `visible` / `disabled` + * as such a surface in its own words. That is objectui#7727's defect, at a + * component objectui#7727 does not touch. + * + * ## Undefined is the default, and that is deliberate + * + * Omitting it forwards `undefined`, so the engine hint stays exactly what it + * was and every mount that passes nothing is unchanged byte for byte. In + * particular this does **not** derive the scope from + * {@link ConditionSubjectVocabulary.fieldPrefix}: that would silently flip + * every record-prefixed mount, including the three whose tier is still an + * open question (the page-block, hook and schema-driven `ConditionWidget` + * sites — see objectui#8167). Deriving a verdict is the same thing as + * making one, and those are not this component's to make. + * + * ## It governs the RAW editor only + * + * The row builder compiles subjects itself from `fieldPrefix`, so passing + * `'record'` does not move a single emitted byte — it makes the raw editor + * agree with the rows the builder was already emitting at that mount, which + * before this prop existed it did not. + */ + scope?: 'record' | 'flattened'; }) { const { fields: hookFields } = useObjectFields(objectName); const fields = fieldsProp ?? hookFields; @@ -377,6 +413,10 @@ export function ConditionBuilder({ label, value, onCommit, objectName, fields: f placeholder="record.status != 'done' && user.isAdmin" objectName={objectName} fieldNames={fieldNames} + /* Forwarded verbatim, `undefined` included — see the `scope` prop's + own note. An omitted scope must reach `celAuthoring` as absent so + its `hint.scope ?? 'flattened'` default answers unchanged. */ + scope={scope} t={tLocal} /> {value && !parse(value) && ( diff --git a/packages/app-shell/src/views/studio-design/ObjectValidationsPanel.tsx b/packages/app-shell/src/views/studio-design/ObjectValidationsPanel.tsx index 86ca6181ff..073fe37b69 100644 --- a/packages/app-shell/src/views/studio-design/ObjectValidationsPanel.tsx +++ b/packages/app-shell/src/views/studio-design/ObjectValidationsPanel.tsx @@ -310,11 +310,36 @@ function RuleTypeFields({ false {t('engine.studio.rules.celPost', locale)} + {/* `scope="record"` (objectui#8167). The authority for a validation rule + is the SERVER: objectql's rule validator evaluates a `script` / + `cross_field` `condition` with `{ record, previous }` and nothing + else, and since objectstack#4649 an unevaluable predicate there is + fail-CLOSED — it rejects the write. So a bare `amount > 100` authored + here does not merely fail to match, it makes every write to the + object fail, while this editor linted it clean under `celAuthoring`'s + `hint.scope ?? 'flattened'` default. + + ⚠️ What this is NOT resting on. The dispatch that ordered this change + said the draft-level validator for THIS surface already runs + `scope: 'record'`. It does not: `clientValidation`'s + `validateObjectFieldRules` lints `visibleWhen` / `readonlyWhen` / + `requiredWhen` and `formula` expressions — a sibling surface, which + is the word the filing card itself uses. Nothing validates a + validation rule's `condition` at draft level at all. The server + binding above is the reason, and it is the stronger one. + + The counter-datum, recorded so it is not re-discovered as an + objection: `ObjectValidationEngine.scopeFor` in `@object-ui/core` + binds the bare field names AND `record`. That engine is deprecated + and deliberately unwired — `validation-engine-stays-unwired.test.ts` + reddens if a production module imports it — so it is not an + authority on what an author should type. */} patch({ condition: writeExpressionSource(rule.condition, cel) })} fields={fields} disabled={disabled} + scope="record" onBlockingIssuesChange={onBlockingIssuesChange} />