From 39fbfac15527cf018db22acd9888752532ff2057 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 05:57:48 +0000 Subject: [PATCH 1/3] fix(app-shell): the Studio decides a screen field's visibleWhen live, through the runner's renderer, over the screen's declared fields The Studio screen preview and the Debug run's screen pause judged each field's `visibleWhen` once, against the run's variables, dropped the fields judged hidden and stripped the predicate from the rest, so a sibling-field predicate (`createOpportunity == true`, `discount > 0`) stayed hidden however the author ticked or typed. `buildScreenSpec` now carries the predicate raw and `ScreenView` decides it live; the Studio's own evaluator for this key (`isFieldVisibleWhen`, `fieldVisibility`, `evalVisibleWhen`) is removed. The Debug run's screen step names, as an error, a predicate that references a name that is not a field declared on this screen, or a shape `registerFlow` refuses; a sibling predicate is no longer reported. `ScreenView` exports `screenPredicateScope`, unchanged. The runner's fallback direction is untouched. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_014mXUNuFomfj24w7s1pZzhN --- .changeset/10692-debug-run-routing-parity.md | 12 + .../10743-studio-screen-visiblewhen-live.md | 53 +++++ packages/app-shell/README.md | 11 +- packages/app-shell/src/views/ScreenView.tsx | 7 +- ...ScreenPreview.debugRunScope-10743.test.tsx | 181 +++++++++++++++ .../previews/ScreenPreview.test.tsx | 139 ++++++++---- .../metadata-admin/previews/ScreenPreview.tsx | 83 ++++--- .../metadata-admin/previews/screen-spec.ts | 206 ++++++++++++------ .../flow-simulator.parity-10692.test.ts | 93 ++++---- .../previews/simulator/flow-sim-validate.ts | 35 +-- .../previews/simulator/flow-simulator.ts | 26 ++- 11 files changed, 611 insertions(+), 235 deletions(-) create mode 100644 .changeset/10743-studio-screen-visiblewhen-live.md create mode 100644 packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.debugRunScope-10743.test.tsx diff --git a/.changeset/10692-debug-run-routing-parity.md b/.changeset/10692-debug-run-routing-parity.md index 6133cd8b5f..e958d431d9 100644 --- a/.changeset/10692-debug-run-routing-parity.md +++ b/.changeset/10692-debug-run-routing-parity.md @@ -46,6 +46,18 @@ so the field is hidden. A non-string `visibleWhen` is refused the same way. A bl as `false`: that is the installed spec 17.4.0 reading. objectstack main refuses both at `registerFlow`, so such a flow never runs there. +⚠️ **Dated note, 2026-09-27 — the Studio no longer decides a screen field's visibility up +front — objectui#10743.** Later in this same release the screen preview and the Debug run's +screen pause hand `visibleWhen` to `ScreenView` raw, and the flow runner's own renderer +decides it live over the screen's declared fields and the values being collected — the +spec's scope, "evaluated by the CLIENT against the screen's live collected values". A +predicate that cannot be evaluated is no longer read as hidden by the Studio: one that +references a name that is not a field on this screen is an error on the screen step, and +whether the renderer shows or hides its field is `ScreenView`'s fallback (objectui#8069). +The rest of this entry is kept as the reading of this change; the objectui#10743 entry +states what the Studio now does, and that the runtime's resume door still reads the run's +variables until objectstack-ai/objectstack#20178 lands. + **Template tokens.** A `NOW()`, `$User.*`, function or arithmetic token inside a longer string rendered as `''`, so `'at {NOW()}'` became `'at '` where the runtime writes the time. The Debug run still does not model these tokens; it now keeps them as written, diff --git a/.changeset/10743-studio-screen-visiblewhen-live.md b/.changeset/10743-studio-screen-visiblewhen-live.md new file mode 100644 index 0000000000..713dbf4379 --- /dev/null +++ b/.changeset/10743-studio-screen-visiblewhen-live.md @@ -0,0 +1,53 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(app-shell): the Studio screen preview and the Debug run decide a screen field's `visibleWhen` live, through the flow runner's own renderer, over the screen's declared fields + +**What was wrong.** The Studio's screen preview (`ScreenPreview`) and the Debug run's screen +pause judged each field's `visibleWhen` once, up front, against the run's variables — the +reading the runtime's resume door takes when a screen is resumed — dropped every field they +judged hidden and stripped the predicate from the ones they kept. A predicate over a sibling +screen field, the only shape any shipped producer writes (`createOpportunity == true` in the +flows guide, `discount > 0` in the console sample), names a value the run does not hold at +the pause, so the field was judged hidden and stayed hidden however the author ticked or +typed: the preview's checkbox changed state and nothing appeared. Where the flow's own +variable of the same name was declared `false`, as the guide recommends, the field was judged +`false` and frozen just the same. Measured through the real `FlowSimulator` and +`ScreenPreview` on both sample shapes, with a no-predicate field as the control. + +**What changed.** The spec declares the contract: `ScreenFieldSpec.visibleWhen` is "bare CEL +over the screen's own field names", "evaluated by the CLIENT against the screen's live +collected values — not by the server". The Studio now follows it with one client evaluator: + +- `buildScreenSpec` keeps every authored field and carries its `visibleWhen` raw, as the + runtime `screen` executor sends it to the client. `ScreenView` — the renderer the flow + runner uses — decides it live over the screen's declared fields and the values being + collected, so ticking the sibling checkbox or typing a discount above zero reveals the + field in the preview exactly as it does for the end user, and the "N fields hidden by + their visible when condition" hint follows the values. +- The Studio no longer has an evaluator of its own for this key: `isFieldVisibleWhen`, + `fieldVisibility` and the simulator's `evalVisibleWhen` are removed. `buildScreenSpec` + takes no `variables`; `hiddenFieldCount` takes the built spec and the collected values. +- The Debug run's screen step judges SCOPE, never value: `unevaluableVisibleWhen` names, as + an error on the paused step, a field whose predicate references an identifier that is not + a field declared on this screen (a run variable such as `needsApproval == true`, the + runtime's `vars.*` root), does not parse as CEL (a `{var}` brace), or is a shape + `registerFlow` refuses — with the nearest declared field where one is close. A predicate + over a sibling field (`discount > 0`, `record.discount > 0`) is not reported; it used to + be named as "hidden because it could not be evaluated". +- `ScreenView` exports `screenPredicateScope`, unchanged, so the Studio reads the roots a + predicate may name off the renderer's own scope instead of restating them. + +**What did not change.** Whether the renderer shows or hides a predicate it cannot evaluate +is `ScreenView`'s fallback and objectui#8069's open question; the Studio now renders through +that fallback and rules no direction of its own. The runner (`ScreenView`, `FlowRunner`) has +no behaviour change. + +**The server side.** The runtime's resume door (`refuseInvalidScreenInput` in +`@objectstack/service-automation`) still evaluates `visibleWhen` over the run's variables +with the submitted values layered on top; narrowing it to the declared scope, and refusing +an undeclared identifier at `registerFlow` and `objectstack validate`, is +objectstack-ai/objectstack#20178 and is not on objectstack main at this change. Until it +lands, a predicate over a run variable is an error in the Studio and shown by the runner, +while the server waives its `required`. diff --git a/packages/app-shell/README.md b/packages/app-shell/README.md index 3f0ce08fc8..196f205747 100644 --- a/packages/app-shell/README.md +++ b/packages/app-shell/README.md @@ -603,10 +603,13 @@ be faithfully modelled is surfaced loudly instead of faked. registration (objectui#10615); an assignment interpolates `{var}` tokens inside nested objects and arrays too, as the runtime's `interpolate` does, and a token it does not model (`NOW()`, `$User.*`, arithmetic) is kept as - written and named on the step; a paused screen gates each field's - `visibleWhen` with the same CEL call, and one that cannot be evaluated hides - the field, as the runtime's resume door reads it (the flow runner's own - `ScreenView` still shows it); + written and named on the step; a paused screen renders through the flow + runner's own `ScreenView`, which decides each field's `visibleWhen` live over + the screen's declared fields and the values being typed (one client + evaluator, objectui#10743), and the screen step names as an error a predicate + that references a name that is not a field on this screen or a shape + `registerFlow` refuses — the runtime's resume door still evaluates over the + run's variables until objectstack#20178 lands; side-effect nodes write their mock to `outputVariable` (the legacy script `outputVariables[]` list is ignored — the engine never binds those names, framework#4278); diff --git a/packages/app-shell/src/views/ScreenView.tsx b/packages/app-shell/src/views/ScreenView.tsx index bf60e49c5a..5c59fe9733 100644 --- a/packages/app-shell/src/views/ScreenView.tsx +++ b/packages/app-shell/src/views/ScreenView.tsx @@ -117,8 +117,13 @@ export function visibleScreenFields( * Fail-open is still the behaviour we want for a genuinely broken predicate — * a syntax error, or a name that is not a field on this screen. Seeding only * the declared names keeps that split intact. + * + * Exported for the Studio's screen diagnostics + * (`metadata-admin/previews/screen-spec.ts`), which read its keys as the names + * a predicate may reference (objectui#10743); the evaluation itself stays in + * {@link visibleScreenFields}. */ -function screenPredicateScope( +export function screenPredicateScope( screen: ScreenSpec, values: Record, ): Record { diff --git a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.debugRunScope-10743.test.tsx b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.debugRunScope-10743.test.tsx new file mode 100644 index 0000000000..359858a7ec --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.debugRunScope-10743.test.tsx @@ -0,0 +1,181 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The Debug run's paused screen decides a field's `visibleWhen` live, through + * the flow runner's own renderer, over the screen's declared fields and the + * values the author is collecting — never over the run's variables + * (objectui#10743). Every run goes through the real `FlowSimulator`, and every + * screen through the real `ScreenPreview` → `ScreenView`, the way + * `FlowSimulatorPanel` mounts them (`node` + `snapshot.variables`). + * + * The three cases are the ruling's three pins, each with a control field that + * declares no predicate in the same tree: + * + * 1. a sibling-field predicate reveals its field live (the console sample's + * `note`, `visibleWhen: 'discount > 0'`, and the docs shape + * `createOpportunity == true`) — the Debug run used to judge the predicate + * once against the run's variables, drop the field and strip the predicate, + * so nothing the author typed or ticked ever brought it back; + * 2. a predicate over a run variable is not read from the run's variables, and + * the screen step names it as an error; + * 3. the control: a field with no predicate is drawn throughout. + * + * ⛔ Whether the renderer SHOWS or HIDES a predicate it cannot evaluate is its + * own fallback and objectui#8069's question. Pin 2 therefore reads the SCOPE + * only: the render is the same whether the run variable is `true` or `false`. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; + +vi.mock('../../../providers/AdapterProvider', () => ({ + useAdapter: () => ({ fake: 'adapter' }), +})); + +vi.mock('../../../providers/MetadataProvider', () => ({ + useMetadata: () => ({ objects: [] }), +})); + +import { ScreenPreview } from './ScreenPreview'; +import { FlowSimulator } from './simulator/flow-simulator'; +import type { SimEdge, SimNode } from './simulator/flow-sim-types'; + +afterEach(() => cleanup()); + +/** The console preview sample's `review` screen (apps/console `preview-samples.ts`), plus a control field. */ +const consoleSample: SimNode = { + id: 'review', + type: 'screen', + label: 'CSM review', + config: { + fields: [ + { name: 'discount', label: 'Discount %', type: 'number', required: false }, + { name: 'note', label: 'Note', type: 'text', required: true, visibleWhen: 'discount > 0' }, + { name: 'comment', label: 'Comment', type: 'text' }, + ], + }, +}; + +/** The docs' lead-conversion screen (`content/docs/automation/flows.mdx`), plus a control field. */ +const docsShape: SimNode = { + id: 'screen_1', + type: 'screen', + label: 'Conversion Details', + config: { + fields: [ + { name: 'createOpportunity', label: 'Create Opportunity?', type: 'boolean', required: true, defaultValue: false }, + { name: 'opportunityName', label: 'Opportunity Name', type: 'text', required: true, visibleWhen: 'createOpportunity == true' }, + { name: 'plain', label: 'Plain', type: 'text' }, + ], + }, +}; + +/** A predicate over a run variable — nothing on this screen declares `needsApproval`. */ +const runVariableShape: SimNode = { + id: 'gate', + type: 'screen', + label: 'Gate', + config: { + fields: [ + { name: 'reason', label: 'Reason', type: 'text', required: true, visibleWhen: 'needsApproval == true' }, + { name: 'plain', label: 'Plain', type: 'text' }, + ], + }, +}; + +/** start → `node` → end, run to the screen pause with `seed` as the run's variables. */ +function pauseAt(node: SimNode, seed: Record = {}) { + const nodes: SimNode[] = [{ id: 'start', type: 'start' }, node, { id: 'end', type: 'end' }]; + const edges: SimEdge[] = [ + { id: 'e1', source: 'start', target: node.id }, + { id: 'e2', source: node.id, target: 'end' }, + ]; + const sim = new FlowSimulator(nodes, edges); + sim.reset(seed); + sim.runToEnd(); + expect(sim.state.status).toBe('paused'); + expect(sim.state.pausedReason).toBe('screen'); + const step = sim.state.steps.find((st) => st.nodeId === node.id); + return { sim, step, node, variables: sim.state.variables }; +} + +describe('the Debug run screen decides a sibling-field visibleWhen live (objectui#10743)', () => { + it('console sample: typing a discount above zero reveals Note; zero hides it; Comment (no predicate) is drawn throughout', () => { + const { node, variables, step } = pauseAt(consoleSample); + const { container } = render(); + const discount = container.querySelector('#ff-discount') as HTMLInputElement | null; + expect(discount).not.toBeNull(); + expect(screen.getByText('Comment')).toBeInTheDocument(); + + fireEvent.change(discount!, { target: { value: '5' } }); + expect(screen.getByText('Note')).toBeInTheDocument(); + expect(screen.getByText('Comment')).toBeInTheDocument(); + + fireEvent.change(discount!, { target: { value: '0' } }); + expect(screen.queryByText('Note')).not.toBeInTheDocument(); + expect(screen.getByText('Comment')).toBeInTheDocument(); + + // The sibling predicate is the renderer's to decide, not a step error, and + // the pause never names `note` as a field it hid. + expect(step?.status).toBe('paused'); + expect(step?.error).toBeUndefined(); + expect(step?.note ?? '').not.toMatch(/"note"/); + }); + + it('docs shape: ticking Create Opportunity reveals Opportunity Name; Plain (no predicate) is drawn throughout', () => { + const { node, variables, step } = pauseAt(docsShape); + const { container } = render(); + expect(screen.queryByText('Opportunity Name')).not.toBeInTheDocument(); + expect(screen.getByText('Plain')).toBeInTheDocument(); + expect(screen.getByText(/hidden by .*visible when/i)).toBeInTheDocument(); + + fireEvent.click(container.querySelector('#ff-createOpportunity')!); + expect(screen.getByText('Opportunity Name')).toBeInTheDocument(); + expect(screen.getByText('Plain')).toBeInTheDocument(); + expect(screen.queryByText(/hidden by .*visible when/i)).not.toBeInTheDocument(); + + expect(step?.error).toBeUndefined(); + expect(step?.note ?? '').not.toMatch(/"opportunityName"/); + }); + + it('docs shape with the flow variable declared (`createOpportunity: false` in the run, as the docs recommend): the tick still reveals the field', () => { + // This is the shape that froze hardest: the run held `false`, the predicate + // evaluated false against it, the field was dropped, and the checkbox the + // author ticked in the preview changed nothing. + const { node, variables } = pauseAt(docsShape, { createOpportunity: false }); + expect(variables).toEqual({ createOpportunity: false }); + const { container } = render(); + expect(screen.queryByText('Opportunity Name')).not.toBeInTheDocument(); + fireEvent.click(container.querySelector('#ff-createOpportunity')!); + expect(screen.getByText('Opportunity Name')).toBeInTheDocument(); + expect(screen.getByText('Plain')).toBeInTheDocument(); + }); +}); + +describe('the Debug run screen does not read a visibleWhen from the run variables, and names it (objectui#10743)', () => { + it('the render is the same whether the run holds needsApproval true or false; Plain (no predicate) is drawn in both', () => { + const t = pauseAt(runVariableShape, { needsApproval: true }); + const withTrue = render(); + const reasonWithTrue = withTrue.queryByText('Reason') !== null; + expect(withTrue.getByText('Plain')).toBeInTheDocument(); + withTrue.unmount(); + + const f = pauseAt(runVariableShape, { needsApproval: false }); + const withFalse = render(); + const reasonWithFalse = withFalse.queryByText('Reason') !== null; + expect(withFalse.getByText('Plain')).toBeInTheDocument(); + + // Scope, not direction: the run variable is not what decides the field. + expect(reasonWithFalse).toBe(reasonWithTrue); + }); + + it('the screen step names the field whose visibleWhen references a name that is not a field on this screen, whatever the run holds', () => { + for (const seed of [{}, { needsApproval: true }, { needsApproval: false }]) { + const { step } = pauseAt(runVariableShape, seed); + expect(step?.status).toBe('paused'); + expect(step?.error).toMatch(/"reason"/); + expect(step?.error).toMatch(/needsApproval/); + expect(step?.error).not.toMatch(/"plain"/); + } + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx index d7d78f9a75..c6f4afc037 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, vi, afterEach } from 'vitest'; -import { render, screen, cleanup } from '@testing-library/react'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; // Capture the schema ObjectForm receives so object-form mode can be asserted // without standing up the real plugin-form runtime. @@ -30,7 +30,7 @@ vi.mock('../../../providers/MetadataProvider', () => ({ })); import { ScreenPreview } from './ScreenPreview'; -import { buildScreenSpec, isFieldVisibleWhen, hiddenFieldCount } from './screen-spec'; +import { buildScreenSpec, hiddenFieldCount, unevaluableVisibleWhen } from './screen-spec'; afterEach(() => { cleanup(); @@ -134,65 +134,118 @@ describe('ScreenPreview — object-form mode', () => { }); }); -describe('ScreenPreview — visibleWhen field gating', () => { +describe('ScreenPreview — visibleWhen is decided live by the screen renderer (objectui#10743)', () => { + // The docs' lead-conversion shape (`createOpportunity == true`), plus a + // control field that declares no predicate. The preview used to judge the + // predicate once against `variables`, drop the field and strip the predicate, + // so no tick ever brought it back. const node = { id: 's1', config: { fields: [ { name: 'createOpp', label: 'Create Opportunity?', type: 'boolean' }, - // Bare CEL: this slot is declared bare CEL, and a `{createOpp}` brace is - // the brace trap `registerFlow` refuses (objectui#10692). { name: 'oppName', label: 'Opportunity Name', type: 'text', visibleWhen: 'createOpp == true' }, + { name: 'plain', label: 'Plain', type: 'text' }, ], }, }; - it('hides a conditional field when its visibleWhen is false, with a hint', () => { - render(); + it('a sibling-field predicate hides its field until the sibling is ticked, then shows it; the hint follows; the control is drawn throughout', () => { + const { container } = render(); expect(screen.getByText('Create Opportunity?')).toBeInTheDocument(); expect(screen.queryByText('Opportunity Name')).not.toBeInTheDocument(); + expect(screen.getByText('Plain')).toBeInTheDocument(); expect(screen.getByText(/hidden by .*visible when/i)).toBeInTheDocument(); - }); - it('shows the conditional field when its visibleWhen is true (no hint)', () => { - render(); + fireEvent.click(container.querySelector('#ff-createOpp')!); expect(screen.getByText('Opportunity Name')).toBeInTheDocument(); + expect(screen.getByText('Plain')).toBeInTheDocument(); expect(screen.queryByText(/hidden by .*visible when/i)).not.toBeInTheDocument(); }); - // Re-judged in objectui#10692: this used to expect the field shown (fail-open). - // A predicate that cannot be evaluated is read as the runtime reads it on - // resume: hidden (`validateScreenInputs`), and counted in the hint. - it('an undecidable condition (the variable is not set) hides the field, with a hint', () => { - render(); + it('the variables handed in for {var} interpolation do not decide a sibling-field predicate: a flow variable of the same name, false, used to freeze the field hidden', () => { + const { container } = render(); expect(screen.queryByText('Opportunity Name')).not.toBeInTheDocument(); - expect(screen.getByText(/hidden by .*visible when/i)).toBeInTheDocument(); + fireEvent.click(container.querySelector('#ff-createOpp')!); + expect(screen.getByText('Opportunity Name')).toBeInTheDocument(); + }); + + it('a numeric sibling predicate follows what is typed (the console sample: note, visibleWhen discount > 0)', () => { + const sample = { + id: 'review', + config: { + fields: [ + { name: 'discount', label: 'Discount %', type: 'number' }, + { name: 'note', label: 'Note', type: 'text', required: true, visibleWhen: 'discount > 0' }, + { name: 'comment', label: 'Comment', type: 'text' }, + ], + }, + }; + const { container } = render(); + const discount = container.querySelector('#ff-discount')!; + fireEvent.change(discount, { target: { value: '5' } }); + expect(screen.getByText('Note')).toBeInTheDocument(); + expect(screen.getByText('Comment')).toBeInTheDocument(); + fireEvent.change(discount, { target: { value: '0' } }); + expect(screen.queryByText('Note')).not.toBeInTheDocument(); + expect(screen.getByText('Comment')).toBeInTheDocument(); + }); + + it('a predicate over a run variable is not read from the run variables: the render is the same whether it holds true or false', () => { + const gated = { + id: 'g', + config: { + fields: [ + { name: 'reason', label: 'Reason', type: 'text', visibleWhen: 'needsApproval == true' }, + { name: 'plain', label: 'Plain', type: 'text' }, + ], + }, + }; + const withTrue = render(); + const reasonWithTrue = withTrue.queryByText('Reason') !== null; + expect(withTrue.getByText('Plain')).toBeInTheDocument(); + withTrue.unmount(); + const withFalse = render(); + const reasonWithFalse = withFalse.queryByText('Reason') !== null; + expect(withFalse.getByText('Plain')).toBeInTheDocument(); + // Whether the renderer shows or hides a predicate it cannot evaluate is its + // own fallback (objectui#8069); this pin reads the SCOPE only. + expect(reasonWithFalse).toBe(reasonWithTrue); }); }); -describe('isFieldVisibleWhen', () => { - it('shows when there is no condition or no variables', () => { - expect(isFieldVisibleWhen(undefined, { x: 1 })).toBe(true); - expect(isFieldVisibleWhen('', { x: 1 })).toBe(true); - expect(isFieldVisibleWhen('discount > 0', undefined)).toBe(true); +describe('unevaluableVisibleWhen — the names a screen predicate may reference (objectui#10743)', () => { + const withPredicate = (visibleWhen: unknown) => ({ + id: 's', + config: { fields: [{ name: 'discount', type: 'number' }, { name: 'note', visibleWhen }, { name: 'plain' }] }, }); - // Re-judged in objectui#10692: the `{createOpp}` spelling used to be - // normalised to a bare name. It is the brace trap `registerFlow` refuses in - // this bare-CEL slot, so the field is hidden; the bare spelling evaluates. - it('evaluates bare CEL conditions against the variables; a {var} brace is refused', () => { - expect(isFieldVisibleWhen('createOpp == true', { createOpp: true })).toBe(true); - expect(isFieldVisibleWhen('createOpp == true', { createOpp: false })).toBe(false); - expect(isFieldVisibleWhen('{createOpp} == true', { createOpp: true })).toBe(false); - expect(isFieldVisibleWhen('stage == "review"', { stage: 'review' })).toBe(true); - expect(isFieldVisibleWhen('stage == "review"', { stage: 'draft' })).toBe(false); - expect(isFieldVisibleWhen('discount > 0', { discount: 5 })).toBe(true); - expect(isFieldVisibleWhen('discount > 0', { discount: 0 })).toBe(false); + it.each([ + ['a sibling field', 'discount > 0'], + ['a sibling field under the record namespace the renderer binds', 'record.discount > 0'], + ['two sibling fields', 'discount > 0 && plain != ""'], + ['control: no predicate', undefined], + ['control: a blank predicate', ' '], + ])('%s is not reported', (_name, visibleWhen) => { + expect(unevaluableVisibleWhen(withPredicate(visibleWhen))).toEqual([]); }); - // Re-judged in objectui#10692: this used to expect `true` (fail-open). - it('hides the field when a referenced variable is not set (the runtime reading of a fault)', () => { - expect(isFieldVisibleWhen('createOpp == true', {})).toBe(false); + it.each([ + ['a run variable', 'needsApproval == true', /needsApproval/], + ['a run variable behind a stdlib call', 'size(tags) > 0', /tags/], + ["the runtime's vars root, which the renderer does not bind", 'vars.discount > 0', /vars/], + ['a {var} brace, the brace trap in a bare-CEL slot', '{discount} > 0', /./], + ['a non-string', true, /./], + ])('%s is reported on the field, by name', (_name, visibleWhen, reason) => { + const out = unevaluableVisibleWhen(withPredicate(visibleWhen)); + expect(out.map((u) => u.name)).toEqual(['note']); + expect(out[0].error).toMatch(reason); + }); + + it('names the nearest declared field for a typo', () => { + const [only] = unevaluableVisibleWhen(withPredicate('dicount > 0')); + expect(only.name).toBe('note'); + expect(only.error).toMatch(/discount/); }); }); @@ -220,7 +273,7 @@ describe('buildScreenSpec', () => { expect(spec.mode).toBe('create'); }); - it('gates fields by visibleWhen against the supplied variables', () => { + it('keeps every field and carries its visibleWhen raw; the renderer decides visibility', () => { const cfg = { id: 'n1', config: { @@ -230,11 +283,13 @@ describe('buildScreenSpec', () => { ], }, }; - expect(buildScreenSpec(cfg, { createOpp: true }).fields.map((f) => f.name)).toEqual(['createOpp', 'oppName']); - expect(buildScreenSpec(cfg, { createOpp: false }).fields.map((f) => f.name)).toEqual(['createOpp']); - // No variables → keep every field (design preview never hides on missing data). - expect(buildScreenSpec(cfg).fields.map((f) => f.name)).toEqual(['createOpp', 'oppName']); - expect(hiddenFieldCount(cfg, { createOpp: false })).toBe(1); - expect(hiddenFieldCount(cfg, { createOpp: true })).toBe(0); + const spec = buildScreenSpec(cfg); + expect(spec.fields).toEqual([ + { name: 'createOpp', label: 'Create?', type: 'boolean', required: false }, + { name: 'oppName', label: 'Name', type: 'text', required: false, visibleWhen: 'createOpp == true' }, + ]); + // The hint's count is the renderer's own verdict for the values collected so far. + expect(hiddenFieldCount(spec, {})).toBe(1); + expect(hiddenFieldCount(spec, { createOpp: true })).toBe(0); }); }); diff --git a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.tsx b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.tsx index 33e982f184..df0f2f2218 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.tsx @@ -8,10 +8,14 @@ * it to the shared {@link ScreenView} — the SAME renderer the runtime * FlowRunner uses — so the preview can never drift from runtime (the * design↔runtime divergence #1927 set out to kill). `{var}` references in the - * title/description are interpolated, and `fields` are gated by their - * `visibleWhen`, against the supplied `variables` (the flow's declared defaults - * in the inspector, or the live simulated values when paused in the Debug - * simulator). + * title/description are interpolated against the supplied `variables` (the + * flow's declared defaults in the inspector, or the live simulated values when + * paused in the Debug simulator). A field's `visibleWhen` is NOT decided here: + * it reaches `ScreenView` raw, and the renderer decides it live over the + * screen's declared fields and the values the author collects in the preview, + * exactly as it does for the end user (objectui#10743). The preview used to + * judge it once against `variables` and drop the field — which froze every + * sibling-field predicate hidden, whatever the author ticked or typed. * * Object-form mode is fed the SAME enriched object list the runtime uses * (`useMetadata().objects`, which derives inline master-detail `subforms` from @@ -34,13 +38,12 @@ export interface ScreenPreviewProps { /** The screen node to preview. */ node: ScreenPreviewNode; /** - * Variable values for `{var}` interpolation in the title/description AND for - * evaluating each field's `visibleWhen`. The inspector passes the flow's - * declared defaults; the simulator passes the live run state at the pause - * point. Unknown `{var}` refs stay literal. A field whose `visibleWhen` can't - * be evaluated against these variables is hidden, the reading the runtime's - * resume door takes of a predicate it cannot evaluate (`fieldVisibility` in - * `./screen-spec.ts`, objectui#10692). + * Variable values for `{var}` interpolation in the title/description. The + * inspector passes the flow's declared defaults; the simulator passes the live + * run state at the pause point. Unknown `{var}` refs stay literal. They play + * no part in a field's `visibleWhen`: a screen predicate binds the screen's + * own declared fields plus the values being collected, and `ScreenView` + * decides it live (objectui#10743). */ variables?: Record; className?: string; @@ -49,7 +52,7 @@ export interface ScreenPreviewProps { export function ScreenPreview({ node, variables, className }: ScreenPreviewProps) { const adapter = useAdapter(); const meta = useMetadata(); - const spec = React.useMemo(() => buildScreenSpec(node, variables), [node, variables]); + const spec = React.useMemo(() => buildScreenSpec(node), [node]); const isObjectForm = isObjectFormScreen(spec); // Enriched object defs (incl. derived master-detail `subforms`) — the exact // list the runtime FlowRunner gets. Only read in object-form mode so a plain @@ -57,16 +60,18 @@ export function ScreenPreview({ node, variables, className }: ScreenPreviewProps const objects = isObjectForm ? meta.objects : undefined; const title = interpolate(spec.title, variables); const description = interpolate(spec.description, variables); - const hidden = isObjectForm ? 0 : hiddenFieldCount(node, variables); // Reset transient input state when the screen's STRUCTURE changes (fields - // added/removed/retyped/gated, or object-form target/mode); typing survives a - // label/title-only edit. + // added/removed/retyped/regated, or object-form target/mode); typing survives + // a label/title-only edit. const structKey = isObjectForm ? `obj:${spec.objectName}:${spec.mode ?? 'create'}` - : 'fields:' + screenFields(spec).map((f) => `${f.name}:${f.type ?? ''}:${f.required ? 1 : 0}`).join('|'); + : 'fields:' + + screenFields(spec) + .map((f) => `${f.name}:${f.type ?? ''}:${f.required ? 1 : 0}:${f.visibleWhen ?? ''}`) + .join('|'); - const empty = !title && !description && !isObjectForm && screenFields(spec).length === 0 && hidden === 0; + const empty = !title && !description && !isObjectForm && screenFields(spec).length === 0; return (
@@ -85,12 +90,6 @@ export function ScreenPreview({ node, variables, className }: ScreenPreviewProps

{description}

)} - {!isObjectForm && hidden > 0 && ( -

- {hidden} field{hidden === 1 ? '' : 's'} hidden by {hidden === 1 ? 'its' : 'their'} “visible when” - {' '}condition{hidden === 1 ? '' : 's'}. -

- )} {!isObjectForm && screenFields(spec).length > 0 && (
{/* Non-functional — the preview never resumes a real run. */} @@ -106,22 +105,34 @@ export function ScreenPreview({ node, variables, className }: ScreenPreviewProps /** * Holds the transient field values so the preview is interactive (the author - * can type into it) while never persisting anything. + * can type into it) while never persisting anything. The "hidden by visible + * when" hint lives here because it is a fact about THESE values: the count is + * the renderer's own verdict (`hiddenFieldCount` over `visibleScreenFields`), + * so it follows every tick and keystroke the way the form does. */ function ScreenFormPreview({ spec, adapter, objects }: { spec: ScreenSpec; adapter: unknown; objects?: unknown[] }) { const [values, setValues] = React.useState>(() => initialScreenValues(spec)); + const hidden = hiddenFieldCount(spec, values); return ( - setValues((p) => ({ ...p, [name]: v }))} - dataSource={adapter ?? undefined} - objects={objects} - objectForm={{ - showSubmit: false, - showCancel: false, - noDataSourceMessage: 'Connect to a backend to preview this object form.', - }} - /> + <> + setValues((p) => ({ ...p, [name]: v }))} + dataSource={adapter ?? undefined} + objects={objects} + objectForm={{ + showSubmit: false, + showCancel: false, + noDataSourceMessage: 'Connect to a backend to preview this object form.', + }} + /> + {hidden > 0 && ( +

+ {hidden} field{hidden === 1 ? '' : 's'} hidden by {hidden === 1 ? 'its' : 'their'} “visible when” + {' '}condition{hidden === 1 ? '' : 's'}. +

+ )} + ); } diff --git a/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts b/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts index 98443645bf..d9632ed8ee 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts @@ -3,13 +3,56 @@ /** * screen-spec — pure helpers that map a flow `screen` node's authored `config` * onto the runtime `ScreenSpec` (the contract {@link ScreenView} renders), plus - * `{var}` interpolation for the title/description and `visibleWhen` field - * gating. Kept framework-free so {@link ScreenPreview} stays a thin component - * and these stay unit-testable. + * `{var}` interpolation for the title/description and the Studio's step-level + * diagnostics for a field's `visibleWhen`. Kept framework-free so + * {@link ScreenPreview} stays a thin component and these stay unit-testable. + * + * ## One client evaluator for `visibleWhen` (objectui#10743) + * + * A screen field's `visibleWhen` is bare CEL over the screen's own declared + * fields plus the values being collected — the spec's `ScreenFieldSpec.visibleWhen` + * docblock: "evaluated by the CLIENT against the screen's live collected + * values — not by the server". The Studio therefore does NOT decide a field's + * visibility up front. {@link buildScreenSpec} hands the predicate to + * `ScreenView` raw, and the renderer's `visibleScreenFields` / + * `screenPredicateScope` decide it live as the author ticks and types into the + * preview — the same code path that decides it for the end user in the flow + * runner. There is no second copy of that evaluation in this module. + * + * Before objectui#10743 this module judged each predicate once, at the pause, + * against the run's variables (the reading the runtime's resume door takes), + * dropped every field it judged hidden and stripped the predicate from the + * rest. A predicate over a sibling screen field — the only shape any shipped + * producer writes (`createOpportunity == true`, `discount > 0`) — names a value + * the run does not hold at the pause, so the field was judged hidden and stayed + * hidden however the author ticked or typed: the "frozen visible-or-hidden for + * the life of the screen" outcome the runtime's `screen` executor forwards the + * predicate raw to avoid. + * + * What this module still judges is the SCOPE, never the value: + * {@link unevaluableVisibleWhen} names a predicate the screen renderer cannot + * bind — one that references an identifier that is not a field declared on + * this screen, or one whose shape `registerFlow` refuses — so the Debug run's + * screen step can say so. Whether a faulting predicate shows or hides its field + * is the renderer's fallback and objectui#8069's question; the Studio renders + * through that renderer and states no direction of its own. + * + * ⚠️ The server's resume door (`refuseInvalidScreenInput` in + * `@objectstack/service-automation`) still evaluates `visibleWhen` over the + * run's variables with the submitted values layered on top until + * objectstack#20178 lands. That is the divergence objectui#10743 measured; it is + * closed on the server side, not by widening the client's scope. */ -import type { ScreenSpec, ScreenFieldSpec } from '../../ScreenView.js'; -import { evalVisibleWhen } from './simulator/flow-sim-validate.js'; +import { collectCelRootIdentifiers, nearestName, validateExpression } from '@objectstack/formula'; +import { predicateSlotRefusal } from '@objectstack/spec/automation'; +import { + screenFields, + screenPredicateScope, + visibleScreenFields, + type ScreenFieldSpec, + type ScreenSpec, +} from '../../ScreenView.js'; /** Minimal node shape the preview needs (id + authored config). */ export interface ScreenPreviewNode { @@ -32,109 +75,126 @@ export function interpolate(text: string | undefined, vars: Record | undefined): FieldVisibility { - if (!variables) return { visible: true }; - const g = evalVisibleWhen(visibleWhen, variables); - if (g.kind === 'absent') return { visible: true }; - if (g.kind === 'fault') return { visible: false, error: g.error }; - return { visible: g.result }; +function declaredPredicateRoots(spec: ScreenSpec): ReadonlySet { + return new Set([...Object.keys(screenPredicateScope(spec, {})), RECORD_ROOT]); } -/** Whether a field is shown: {@link fieldVisibility}'s `visible`. */ -export function isFieldVisibleWhen(visibleWhen: unknown, variables: Record | undefined): boolean { - return fieldVisibility(visibleWhen, variables).visible; +/** One field whose `visibleWhen` the screen renderer cannot evaluate, and why. */ +export interface UnevaluableVisibleWhen { + name: string; + error: string; } /** - * The authored field rows of a screen whose `visibleWhen` is refused or fails - * to evaluate against `variables`, with the error. Each is hidden (see - * {@link fieldVisibility}); the Debug run names them on the screen's step. + * Why the screen renderer cannot evaluate one `visibleWhen`, or `undefined` + * when it can: the predicate is absent or blank (no predicate — the installed + * spec admits a blank one and the field is then always shown), or every root it + * names is declared on this screen. + * + * Judged in the order `registerFlow` judges the slot, minus the evaluation: + * the spec's shape refusal (a non-string), the CEL parse (`validateExpression`, + * which refuses a `{var}` brace — the brace trap in a bare-CEL slot), then the + * roots. A root that is not a declared field is named, with the nearest + * declared field when one is close (`dicount` → `discount`). */ -export function unevaluableVisibleWhen( - node: ScreenPreviewNode, - variables: Record | undefined, -): Array<{ name: string; error: string }> { +function visibleWhenScopeError( + visibleWhen: unknown, + roots: ReadonlySet, + declared: readonly string[], +): string | undefined { + if (visibleWhen === undefined || visibleWhen === null) return undefined; + if (typeof visibleWhen === 'string' && !visibleWhen.trim()) return undefined; + const shape = predicateSlotRefusal(visibleWhen); + if (shape) return shape.message; + const source = visibleWhen as string; + const parsed = validateExpression('predicate', source); + if (parsed.errors.length > 0) return parsed.errors.map((e) => e.message).join(' '); + const collected = collectCelRootIdentifiers(source); + if (!collected.ok) return collected.error; + const undeclared = collected.roots.filter((r) => !roots.has(r)); + if (undeclared.length === 0) return undefined; + return undeclared + .map((r) => { + const near = nearestName(r, declared); + return `\`${r}\` is not a field on this screen${near ? ` (did you mean \`${near}\`?)` : ''}`; + }) + .join('; '); +} + +/** + * The authored field rows of a screen whose `visibleWhen` the screen renderer + * cannot evaluate, with the reason — see {@link visibleWhenScopeError}. A + * predicate over a sibling field (`createOpportunity == true`, `discount > 0`, + * `record.discount > 0`) is not reported: the renderer decides it live. One + * over a name the renderer never binds — a run variable such as + * `needsApproval == true` on a screen with no `needsApproval` field, the `vars` + * root the runtime's own scope carries — is, by field name, so the Debug run's + * screen step can name it (objectui#10743). + */ +export function unevaluableVisibleWhen(node: ScreenPreviewNode): UnevaluableVisibleWhen[] { const raw = (node.config as Record | undefined)?.fields; if (!Array.isArray(raw)) return []; - const out: Array<{ name: string; error: string }> = []; + const spec = buildScreenSpec(node); + const roots = declaredPredicateRoots(spec); + const declared = screenFields(spec).map((f) => f.name); + const out: UnevaluableVisibleWhen[] = []; for (const f of raw) { if (!f || typeof f !== 'object') continue; const row = f as Record; if (typeof row.name !== 'string' || !row.name) continue; - const { error } = fieldVisibility(row.visibleWhen, variables); + const error = visibleWhenScopeError(row.visibleWhen, roots, declared); if (error) out.push({ name: row.name, error }); } return out; } /** - * Coerce the authored `config.fields` rows into runtime `ScreenFieldSpec`s, - * dropping any that {@link fieldVisibility} reads as hidden against - * `variables`. (The runtime `screen` executor sends `visibleWhen` to the - * client raw; this preview decides it up front, with the runtime's resume-door reading.) + * Coerce the authored `config.fields` rows into runtime `ScreenFieldSpec`s. A + * field's `visibleWhen` is carried RAW, as the runtime `screen` executor sends + * it to the client, so `ScreenView` decides it live against the values the + * author is collecting in the preview (objectui#10743). Nothing is dropped + * here: a field whose predicate is false right now is hidden by the renderer + * and comes back the moment the values make it true. */ -function toScreenFields(raw: unknown, variables: Record | undefined): ScreenFieldSpec[] { +function toScreenFields(raw: unknown): ScreenFieldSpec[] { if (!Array.isArray(raw)) return []; const out: ScreenFieldSpec[] = []; for (const f of raw) { if (!f || typeof f !== 'object') continue; const row = f as Record; if (typeof row.name !== 'string' || !row.name) continue; - if (!isFieldVisibleWhen(row.visibleWhen, variables)) continue; out.push({ name: row.name, label: typeof row.label === 'string' ? row.label : undefined, type: typeof row.type === 'string' ? row.type : undefined, required: row.required === true, + ...(typeof row.visibleWhen === 'string' ? { visibleWhen: row.visibleWhen } : {}), }); } return out; } -/** Count of authored field rows hidden by their `visibleWhen` against `variables`. */ -export function hiddenFieldCount(node: ScreenPreviewNode, variables: Record | undefined): number { - const raw = (node.config as Record | undefined)?.fields; - if (!Array.isArray(raw)) return 0; - let hidden = 0; - for (const f of raw) { - if (!f || typeof f !== 'object') continue; - const row = f as Record; - if (typeof row.name !== 'string' || !row.name) continue; - if (!isFieldVisibleWhen(row.visibleWhen, variables)) hidden++; - } - return hidden; -} - /** - * Map a screen node's authored `config` onto the runtime `ScreenSpec` — the - * same keys the engine's `screen` executor reads (title / description / fields - * / objectName / mode / defaults / idVariable). `fields` are gated by their - * `visibleWhen` against `variables` (omit `variables` to keep every field). + * How many of the screen's fields their `visibleWhen` hides for the values + * collected so far — the renderer's own verdict (`visibleScreenFields`), so the + * preview's hint and the form it sits under cannot disagree. */ +export function hiddenFieldCount(spec: ScreenSpec, values: Record): number { + return screenFields(spec).length - visibleScreenFields(spec, values).length; +} + /** * A designed screen always carries a (possibly empty) field list — `fields` is * optional on the WIRE type only, where a message-only / object-form pause may @@ -142,7 +202,13 @@ export function hiddenFieldCount(node: ScreenPreviewNode, variables: Record): DesignedScreenSpec { +/** + * Map a screen node's authored `config` onto the runtime `ScreenSpec` — the + * same keys the engine's `screen` executor reads (title / description / fields + * / objectName / mode / defaults / idVariable). Every authored field is kept, + * with its `visibleWhen` raw; the renderer decides visibility. + */ +export function buildScreenSpec(node: ScreenPreviewNode): DesignedScreenSpec { const c = (node.config && typeof node.config === 'object' ? node.config : {}) as Record; const objectName = typeof c.objectName === 'string' && c.objectName ? c.objectName : undefined; const mode = c.mode === 'edit' ? 'edit' : c.mode === 'create' ? 'create' : undefined; @@ -154,7 +220,7 @@ export function buildScreenSpec(node: ScreenPreviewNode, variables?: Record ({ id, type: 'end' }); @@ -224,50 +227,58 @@ describe('an approval resumes through the same successor selection (objectui#106 }); }); -describe('a screen field visibleWhen is the runtime CEL predicate (objectui#10692, objectui#10693)', () => { - const rows: Array<[string, unknown, Record, boolean]> = [ - ['the CEL stdlib: size() of an empty list', 'size(tags) > 0', { tags: [] }, false], - ['the `vars` root', 'vars.n == 2', { n: 3 }, false], - ['CEL equality does not convert types', 'n == "2"', { n: 2 }, false], - ['a variable that is not set faults: hidden', 'createOpp == true', {}, false], - ['a {var} brace is refused: hidden', '{createOpp} == true', { createOpp: true }, false], - ['a non-string is refused: hidden', true, {}, false], - ['control: true', 'createOpp == true', { createOpp: true }, true], - ['control: the `vars` root, true', 'vars.n == 2', { n: 2 }, true], - ['control: no predicate', undefined, {}, true], - ['control: a blank predicate', ' ', {}, true], - ]; - - it.each(rows)('%s', (_name, visibleWhen, vars, visible) => { - expect(isFieldVisibleWhen(visibleWhen, vars)).toBe(visible); - }); - - it('the screen preview drops a field whose visibleWhen faults', () => { - const spec = buildScreenSpec( - { id: 'scr', config: { fields: [{ name: 'a' }, { name: 'b', visibleWhen: 'missing > 1' }] } }, - {}, - ); - expect(spec.fields.map((f) => f.name)).toEqual(['a']); +describe('a screen field visibleWhen is decided live by the screen renderer; the Debug run judges its scope (objectui#10692, objectui#10693, objectui#10743)', () => { + const screenNode = (visibleWhen: unknown): SimNode => ({ + id: 'scr', + type: 'screen', + config: { fields: [{ name: 'a', type: 'boolean' }, { name: 'b', visibleWhen }, { name: 'c' }] }, }); - - it("the Debug run's screen pause names the field it hid for a faulting visibleWhen", () => { - const sim = new FlowSimulator( - [ - { id: 's', type: 'start' }, - { id: 'scr', type: 'screen', config: { fields: [{ name: 'a' }, { name: 'b', visibleWhen: 'missing > 1' }] } }, - end('e'), - ], + const pauseOn = (visibleWhen: unknown, seed: Record = {}) => { + const sim = run( + [{ id: 's', type: 'start' }, screenNode(visibleWhen), end('e')], [ { source: 's', target: 'scr' }, { source: 'scr', target: 'e' }, ], + seed, ); - sim.reset(); - sim.runToEnd(); - const step = sim.state.steps.find((st) => st.nodeId === 'scr'); + return sim.state.steps.find((st) => st.nodeId === 'scr'); + }; + + it('the screen preview keeps every field and hands the predicate to ScreenView raw', () => { + const spec = buildScreenSpec({ + id: 'scr', + config: { fields: [{ name: 'a' }, { name: 'b', visibleWhen: 'a == true' }, { name: 'c', visibleWhen: 'missing > 1' }] }, + }); + expect(spec.fields.map((f) => [f.name, f.visibleWhen])).toEqual([ + ['a', undefined], + ['b', 'a == true'], + ['c', 'missing > 1'], + ]); + }); + + it.each([ + ['a sibling field', 'a == true'], + ['a sibling field under the record namespace', 'record.a == true'], + ['control: no predicate', undefined], + ])('%s: the pause carries no error and names no field', (_name, visibleWhen) => { + const step = pauseOn(visibleWhen); + expect(step?.status).toBe('paused'); + expect(step?.error).toBeUndefined(); + expect(step?.note ?? '').not.toMatch(/"b"/); + }); + + it.each([ + ['a run variable the run holds', 'missing > 1', { missing: 2 }], + ['a run variable the run does not hold', 'missing > 1', {}], + ["the runtime's vars root", 'vars.a == true', { a: true }], + ['a {var} brace', '{a} == true', {}], + ['a non-string', true, {}], + ])('%s: the pause names the field as an error, and the run still pauses', (_name, visibleWhen, seed) => { + const step = pauseOn(visibleWhen, seed); expect(step?.status).toBe('paused'); - expect(step?.note).toMatch(/"b"/); - expect(step?.note).not.toMatch(/"a"/); + expect(step?.error).toMatch(/"b"/); + expect(step?.error).not.toMatch(/"a"|"c"/); }); }); diff --git a/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-sim-validate.ts b/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-sim-validate.ts index 3149d20678..3d0664759b 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-sim-validate.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-sim-validate.ts @@ -180,10 +180,12 @@ export function evalGuard(condition: unknown, variables: Record } /** - * The one CEL call every predicate in the simulator goes through: an edge - * guard ({@link evalGuard}), a screen field's `visibleWhen` - * ({@link evalVisibleWhen}) and a decision branch's `expression` + * The one CEL call every predicate the simulator evaluates goes through: an + * edge guard ({@link evalGuard}) and a decision branch's `expression` * ({@link evalBranchPredicate}). There is no second evaluator (objectui#10692). + * A screen field's `visibleWhen` is not evaluated by the simulator at all: the + * screen renderer decides it live over the screen's declared fields + * (`ScreenView`, objectui#10743), and `screen-spec.ts` judges only its scope. * * 1. `validateExpression('predicate', …)`, the parse `registerFlow` refuses a * flow with. A `{var}` or `${…}` template is not CEL and is refused here, @@ -210,33 +212,6 @@ function evalCelPredicate( } } -/** - * Evaluate a screen field's `visibleWhen` the way the runtime evaluates it - * when a screen is resumed (`refuseInvalidScreenInput` in `AutomationEngine`, - * which calls `evaluateCondition` over the run's variables), through - * {@link evalCelPredicate} (objectui#10692). - * - * 1. Absent: `undefined`, `null` or a blank string. This is the runtime's own - * test for "declares a predicate" (`validateScreenInputs`), and the field is - * then always shown. objectstack main refuses a blank string at - * `registerFlow`; the installed 17.4.0 admits it. - * 2. Shape: the spec's `predicateSlotRefusal`. The slot is declared bare CEL - * text, so an envelope, a boolean or any other non-string is refused, as - * `registerFlow` refuses it. - * 3. The CEL call: a `{var}` brace is refused (it is the brace trap in a - * bare-CEL slot, and `registerFlow` refuses it), and a CEL error is a fault. - * - * What a fault means is the caller's to state: the screen preview reads it as - * hidden (see `isFieldVisibleWhen`). - */ -export function evalVisibleWhen(visibleWhen: unknown, variables: Record): GuardEvaluation { - if (visibleWhen === undefined || visibleWhen === null) return { kind: 'absent' }; - if (typeof visibleWhen === 'string' && !visibleWhen.trim()) return { kind: 'absent' }; - const shape = predicateSlotRefusal(visibleWhen); - if (shape) return { kind: 'fault', error: shape.message }; - return evalCelPredicate(visibleWhen as string, visibleWhen as string, variables); -} - /** * Evaluate one entry of a decision's `config.conditions` (`{ label, * expression }`), the way the runtime's `decision` executor does diff --git a/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts b/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts index 89730fe13a..01f9d4580a 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts @@ -315,18 +315,22 @@ export class FlowSimulator { if (shouldPause) { this.state.status = 'paused'; this.state.pausedReason = 'screen'; - // A field whose `visibleWhen` cannot be evaluated is hidden, as the runtime's - // resume door reads it (`fieldVisibility` in `../screen-spec.ts`); say which. - const unevaluable = unevaluableVisibleWhen(node, this.state.variables); + // The screen renderer decides each field's `visibleWhen` live, over the + // screen's declared fields and the values being collected (`ScreenView`, + // objectui#10743); the Debug run does not decide it here. What it judges + // is the SCOPE: a predicate that names something the renderer cannot + // bind — an identifier that is not a field on this screen, or a shape + // `registerFlow` refuses — is an error on the step, per field. Whether + // the renderer then shows or hides that field is its own fallback + // (objectui#8069), not a reading the Debug run makes. + const unevaluable = unevaluableVisibleWhen(node); return this.record(node.id, type, node.label, 'paused', { - note: joinNotes( - 'Screen reached — provide inputs, then continue.', - unevaluable.length - ? `Hidden because its visibleWhen could not be evaluated, as the runtime's resume door treats such a field (the flow runner's own ScreenView still shows it): ${unevaluable - .map((u) => `"${u.name}" (${u.error.split('\n')[0]})`) - .join('; ')}.` - : undefined, - ), + note: 'Screen reached — provide inputs, then continue.', + error: unevaluable.length + ? `A screen field's visibleWhen may reference only the fields declared on this screen; the screen cannot evaluate ${unevaluable + .map((u) => `"${u.name}" (${u.error.split('\n')[0]})`) + .join('; ')}.` + : undefined, }); } return this.proceed(node, 'ok', { note: 'Screen has no input — passed through (matches runtime).' }); From cd8e9713e7d567f6f976f2230b4288f900bf6e80 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 11:01:45 +0000 Subject: [PATCH 2/3] fix(app-shell): the Problems panel judges a screen field's visibleWhen over the screen's declared fields, by the Debug run's rule Round-2 review items on the same branch. `flow-expr-problems.ts` judged the `screen` node's `fields[].visibleWhen` column over the FLOW scope, so it warned on a sibling-field predicate and stayed silent on a run variable, the inverse of the Debug run's screen step. It now applies `screenVisibleWhenScopeError` from `screen-spec.ts`, the one rule both surfaces share: the roots are the keys of the renderer's own `screenPredicateScope` plus `record`. A comprehension's iteration variable is bound, not a root (walked over `parseCelToAst`, scoped to the macro's arguments). The screen field inspector's `visibleWhen` placeholder is a sibling-field example. The changeset's server sentence states when the resume door enforces `required`; the `screen-spec.ts` docblock says the server side is to be closed and that `record` is restated by name because the renderer binds it inside `evalFieldPredicate`. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_014mXUNuFomfj24w7s1pZzhN --- .../10743-studio-screen-visiblewhen-live.md | 19 +- packages/app-shell/README.md | 6 +- .../inspectors/flow-node-config.ts | 4 +- .../previews/ScreenPreview.test.tsx | 26 ++- .../previews/flow-expr-problems.test.ts | 60 +++++++ .../previews/flow-expr-problems.ts | 37 +++- .../metadata-admin/previews/screen-spec.ts | 166 +++++++++++++----- 7 files changed, 269 insertions(+), 49 deletions(-) diff --git a/.changeset/10743-studio-screen-visiblewhen-live.md b/.changeset/10743-studio-screen-visiblewhen-live.md index 713dbf4379..b6b24f6aea 100644 --- a/.changeset/10743-studio-screen-visiblewhen-live.md +++ b/.changeset/10743-studio-screen-visiblewhen-live.md @@ -35,7 +35,15 @@ collected values — not by the server". The Studio now follows it with one clie runtime's `vars.*` root), does not parse as CEL (a `{var}` brace), or is a shape `registerFlow` refuses — with the nearest declared field where one is close. A predicate over a sibling field (`discount > 0`, `record.discount > 0`) is not reported; it used to - be named as "hidden because it could not be evaluated". + be named as "hidden because it could not be evaluated". A comprehension's iteration + variable (`["a","b"].exists(t, t == note)`) is bound, not a root, and is not reported. +- The Problems panel (`flow-expr-problems.ts`) judges the `screen` node's `fields[].visibleWhen` + column by that same rule instead of the flow scope: it used to warn that `discount` "is not + a reference in scope at this step" on the sibling-field shape and stay silent on a run + variable; it now warns on the run variable (`needsApproval` is not a field on this screen) + and is silent on the sibling. Every other expression slot keeps the flow scope. +- The screen field inspector's `visibleWhen` placeholder is a sibling-field example + (`createOpportunity == true`); it used to read `stage == "review"`, a run-variable shape. - `ScreenView` exports `screenPredicateScope`, unchanged, so the Studio reads the roots a predicate may name off the renderer's own scope instead of restating them. @@ -50,4 +58,11 @@ with the submitted values layered on top; narrowing it to the declared scope, an an undeclared identifier at `registerFlow` and `objectstack validate`, is objectstack-ai/objectstack#20178 and is not on objectstack main at this change. Until it lands, a predicate over a run variable is an error in the Studio and shown by the runner, -while the server waives its `required`. +while the server enforces its `required` only when the run's variables make the predicate +true, and waives it when they make it false or leave it unevaluable. + +**Still on the flow scope.** The inline inspector's `visibleWhen` cell (`FlowObjectListField`) +still offers the flow scope picker and its inline note still reads the flow scope, so on the +sibling-field shape it says "`discount` is not a reference in scope at this step" while the +Problems panel and the Debug run are silent; aligning that cell is a follow-up outside this +change. diff --git a/packages/app-shell/README.md b/packages/app-shell/README.md index 196f205747..3f20c6c3a3 100644 --- a/packages/app-shell/README.md +++ b/packages/app-shell/README.md @@ -608,8 +608,10 @@ be faithfully modelled is surfaced loudly instead of faked. the screen's declared fields and the values being typed (one client evaluator, objectui#10743), and the screen step names as an error a predicate that references a name that is not a field on this screen or a shape - `registerFlow` refuses — the runtime's resume door still evaluates over the - run's variables until objectstack#20178 lands; + `registerFlow` refuses — the Problems panel judges that column by the same + rule (a warning), while the inline inspector's `visibleWhen` cell still reads + the flow scope (a follow-up); the runtime's resume door still evaluates over + the run's variables until objectstack#20178 lands; side-effect nodes write their mock to `outputVariable` (the legacy script `outputVariables[]` list is ignored — the engine never binds those names, framework#4278); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts index 79d4e922a1..e44281d179 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts @@ -740,7 +740,9 @@ const FLOW_NODE_CONFIG: Record = { { key: 'label', label: 'Label', kind: 'text', placeholder: 'Discount %' }, { key: 'type', label: 'Type', kind: 'text', placeholder: 'number' }, { key: 'required', label: 'Required', kind: 'boolean' }, - { key: 'visibleWhen', label: 'Visible when', kind: 'expression', placeholder: 'stage == "review"' }, + // A sibling-field shape (the flows guide's example): the predicate binds + // this screen's own fields, never a run variable (objectui#10743). + { key: 'visibleWhen', label: 'Visible when', kind: 'expression', placeholder: 'createOpportunity == true' }, ], }), cfg('waitForInput', 'Wait for input', 'boolean', { diff --git a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx index c6f4afc037..eb4fd8fddb 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/ScreenPreview.test.tsx @@ -30,7 +30,8 @@ vi.mock('../../../providers/MetadataProvider', () => ({ })); import { ScreenPreview } from './ScreenPreview'; -import { buildScreenSpec, hiddenFieldCount, unevaluableVisibleWhen } from './screen-spec'; +import { buildScreenSpec, hiddenFieldCount, screenPredicateRoots, unevaluableVisibleWhen } from './screen-spec'; +import { screenPredicateScope } from '../../ScreenView'; afterEach(() => { cleanup(); @@ -247,6 +248,29 @@ describe('unevaluableVisibleWhen — the names a screen predicate may reference expect(only.name).toBe('note'); expect(only.error).toMatch(/discount/); }); + + it.each([ + ['exists', '["a","b"].exists(t, t == note)'], + ['all over a sibling', '[discount].all(d, d > 0)'], + ['nested map and exists_one over siblings', '[discount].map(i, i + 1).exists_one(q, q > discount)'], + ])("a comprehension's iteration variable is bound, not a root: %s is not reported", (_name, visibleWhen) => { + expect(unevaluableVisibleWhen(withPredicate(visibleWhen))).toEqual([]); + }); + + it('a comprehension does not hide an undeclared root inside its body, nor the same name used bare outside it', () => { + expect(unevaluableVisibleWhen(withPredicate('["a","b"].exists(t, t == tags)')).map((u) => u.error)).toEqual([ + expect.stringMatching(/`tags` is not a field on this screen/), + ]); + const outer = unevaluableVisibleWhen(withPredicate('t == 1 && ["a"].exists(t, t == note)')); + expect(outer.map((u) => u.error)).toEqual([expect.stringMatching(/`t` is not a field on this screen/)]); + }); + + it('one rule: the roots are the keys of the renderer\'s own predicate scope plus record', () => { + const node = withPredicate(undefined); + const fromRenderer = [...Object.keys(screenPredicateScope(buildScreenSpec(node), {})), 'record'].sort(); + expect([...screenPredicateRoots(node)].sort()).toEqual(fromRenderer); + expect(fromRenderer).toEqual(['discount', 'note', 'plain', 'record']); + }); }); describe('buildScreenSpec', () => { diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.test.ts b/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.test.ts index b2875433f7..c5772934be 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.test.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.test.ts @@ -115,3 +115,63 @@ describe('flowExpressionProblems', () => { expect(ps[0]).toMatchObject({ level: 'error', target: { kind: 'node', nodeId: 'd' } }); }); }); + +describe("a screen field's visibleWhen is judged over the screen's declared fields, not the flow scope (objectui#10743)", () => { + // `needsApproval` is a flow variable, so it IS in the flow scope at the screen; + // `discount` is a sibling field on the screen and is NOT in the flow scope. + const draft = (visibleWhen: unknown, extraNodes: Array> = [], extraEdges: Array> = []) => ({ + variables: [{ name: 'needsApproval', type: 'boolean' }], + nodes: [ + startUpdate, + { + id: 'review', + type: 'screen', + config: { + fields: [ + { name: 'discount', label: 'Discount %', type: 'number' }, + { name: 'note', label: 'Note', type: 'text', visibleWhen }, + { name: 'comment', label: 'Comment', type: 'text' }, + ], + }, + }, + ...extraNodes, + ], + edges: [{ source: 'start', target: 'review' }, ...extraEdges], + }); + + it.each([ + ['a sibling field', 'discount > 0'], + ['a sibling field under the record namespace', 'record.discount > 0'], + ['a comprehension over a sibling field', '["a","b"].exists(t, t == note)'], + ['control: no predicate', undefined], + ])('%s: no problem on the screen', (_name, visibleWhen) => { + expect(flowExpressionProblems(draft(visibleWhen))).toEqual([]); + }); + + it('a run variable the flow scope holds is the unknown reference on the screen, named by field and root', () => { + const ps = flowExpressionProblems(draft('needsApproval == true')); + expect(ps).toHaveLength(1); + expect(ps[0]).toMatchObject({ level: 'warning', target: { kind: 'node', nodeId: 'review' } }); + expect(ps[0].message).toMatch(/^Note: /); + expect(ps[0].message).toMatch(/`needsApproval` is not a field on this screen/); + }); + + it('a {var} brace in the column is still the brace error, ahead of the scope', () => { + const ps = flowExpressionProblems(draft('{discount} > 0')); + expect(ps).toHaveLength(1); + expect(ps[0]).toMatchObject({ level: 'error', target: { kind: 'node', nodeId: 'review' } }); + }); + + it('control: a decision condition downstream keeps the FLOW scope — the run variable is in scope there, and so is the screen output the field becomes; an unknown root still warns with the flow wording', () => { + // The same identifier is clean on the decision and the unknown reference on + // the screen: `needsApproval` is a flow variable; `discount`, collected by + // the screen, is a flow variable downstream of it (`nodeOutputRefs`). + const inScope = { id: 'd', type: 'decision', config: { condition: 'needsApproval == true && discount > 0' } }; + expect(flowExpressionProblems(draft('discount > 0', [inScope], [{ source: 'review', target: 'd' }]))).toEqual([]); + const unknown = { id: 'd', type: 'decision', config: { condition: 'needsAproval == true' } }; + const ps = flowExpressionProblems(draft(undefined, [unknown], [{ source: 'review', target: 'd' }])); + expect(ps).toHaveLength(1); + expect(ps[0]).toMatchObject({ level: 'warning', target: { kind: 'node', nodeId: 'd' } }); + expect(ps[0].message).toMatch(/did you mean `needsApproval`/); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.ts b/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.ts index 48ee96df28..07fe145254 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-expr-problems.ts @@ -18,12 +18,27 @@ * single braces legally and are left to the inline check. An `expression` field * flagged `refMode: 'template'` (e.g. a loop/map collection like `{leadList}`) is * such a template surface and is likewise skipped here. + * + * ## A screen field's `visibleWhen` is not a flow-scope slot (objectui#10743) + * + * The `screen` node's `fields[].visibleWhen` column binds the screen's OWN + * declared fields plus `record`, never the flow scope: the renderer evaluates + * it over the values being collected (spec `ScreenFieldSpec.visibleWhen`). So + * that column is judged by `screenVisibleWhenScopeError` (`./screen-spec.ts`), + * the same rule the Debug run's screen step applies — a sibling-field predicate + * (`discount > 0`) is clean, and a run variable (`needsApproval == true`) is + * the unknown reference, however rich the flow scope is. Reported at + * `warning`, this panel's level for a reference nothing binds: the runtime + * accepts such a flow today (the `registerFlow` refusal is objectstack#20178) + * and the runner falls back on it. Every other `expression` slot keeps the + * flow scope. */ import { fieldsForNodeType, getFieldValue } from '../inspectors/flow-node-config.js'; import { resolveFlowScope } from '../inspectors/flow-scope.js'; import { scopeRoots, findUnknownRefs, describeUnknownRefs } from '../inspectors/flow-ref-check.js'; import { validateExpressionClient } from '../inspectors/expression-validate.js'; +import { screenVisibleWhenScopeError, type ScreenPreviewNode } from './screen-spec.js'; import type { DiagnosticLevel } from './simulator/flow-sim-types.js'; export interface ExprProblem { @@ -53,6 +68,23 @@ function checkCel(value: unknown, roots: Set | null, locale?: string): { return null; } +/** + * Brace error (error) else an undeclared screen-field root (warning) for one + * screen field's `visibleWhen` — the declared screen scope, shared with the + * Debug run's screen step (objectui#10743), never the flow scope. + */ +function checkScreenVisibleWhen(value: unknown, node: ScreenPreviewNode): { level: DiagnosticLevel; message: string } | null { + const issue = validateExpressionClient('predicate', value); + if (issue) return { level: 'error', message: issue.message }; + const scope = screenVisibleWhenScopeError(value, node); + return scope ? { level: 'warning', message: scope } : null; +} + +/** The `screen` node's `fields[].visibleWhen` column — the one `objectList` expression cell that is not a flow-scope slot. */ +function isScreenVisibleWhenColumn(type: string, fieldId: string, colKey: string): boolean { + return type === 'screen' && fieldId === 'fields' && colKey === 'visibleWhen'; +} + /** * Scan a flow draft for expression problems, resolved onto node / edge targets. * Pure: no network — the trigger object's fields are not expanded (root-only @@ -79,11 +111,14 @@ export function flowExpressionProblems(draft: Record, locale?: } else if (field.kind === 'objectList' && field.columns) { const exprCols = field.columns.filter((c) => c.kind === 'expression'); if (exprCols.length === 0) continue; + const screenNode: ScreenPreviewNode = { id: nodeId, config: asRecord(node.config) }; for (const row of asArray(getFieldValue(node, field))) { const r = asRecord(row); const rowLabel = str(r.label); for (const col of exprCols) { - const hit = checkCel(r[col.key], roots, locale); + const hit = isScreenVisibleWhenColumn(type, field.id, col.key) + ? checkScreenVisibleWhen(r[col.key], screenNode) + : checkCel(r[col.key], roots, locale); if (hit) { const prefix = rowLabel || col.label; out.push({ target: { kind: 'node', nodeId }, level: hit.level, message: prefix ? `${prefix}: ${hit.message}` : hit.message }); diff --git a/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts b/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts index d9632ed8ee..ac1e825245 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/screen-spec.ts @@ -3,9 +3,9 @@ /** * screen-spec — pure helpers that map a flow `screen` node's authored `config` * onto the runtime `ScreenSpec` (the contract {@link ScreenView} renders), plus - * `{var}` interpolation for the title/description and the Studio's step-level - * diagnostics for a field's `visibleWhen`. Kept framework-free so - * {@link ScreenPreview} stays a thin component and these stay unit-testable. + * `{var}` interpolation for the title/description and the Studio's diagnostics + * for a field's `visibleWhen`. Kept framework-free so {@link ScreenPreview} + * stays a thin component and these stay unit-testable. * * ## One client evaluator for `visibleWhen` (objectui#10743) * @@ -30,21 +30,23 @@ * predicate raw to avoid. * * What this module still judges is the SCOPE, never the value: - * {@link unevaluableVisibleWhen} names a predicate the screen renderer cannot - * bind — one that references an identifier that is not a field declared on - * this screen, or one whose shape `registerFlow` refuses — so the Debug run's - * screen step can say so. Whether a faulting predicate shows or hides its field - * is the renderer's fallback and objectui#8069's question; the Studio renders - * through that renderer and states no direction of its own. + * {@link screenVisibleWhenScopeError} names a predicate the screen renderer + * cannot bind — one that references an identifier that is not a field declared + * on this screen, or one whose shape `registerFlow` refuses — and + * {@link unevaluableVisibleWhen} lists the fields it applies to, so the Debug + * run's screen step and the Problems panel (`flow-expr-problems.ts`) can say so + * from ONE rule. Whether a faulting predicate shows or hides its field is the + * renderer's fallback and objectui#8069's question; the Studio renders through + * that renderer and states no direction of its own. * * ⚠️ The server's resume door (`refuseInvalidScreenInput` in * `@objectstack/service-automation`) still evaluates `visibleWhen` over the * run's variables with the submitted values layered on top until * objectstack#20178 lands. That is the divergence objectui#10743 measured; it is - * closed on the server side, not by widening the client's scope. + * to be closed on the server side, not by widening the client's scope. */ -import { collectCelRootIdentifiers, nearestName, validateExpression } from '@objectstack/formula'; +import { collectCelRootIdentifiers, nearestName, parseCelToAst, validateExpression } from '@objectstack/formula'; import { predicateSlotRefusal } from '@objectstack/spec/automation'; import { screenFields, @@ -76,25 +78,105 @@ export function interpolate(text: string | undefined, vars: Record { - return new Set([...Object.keys(screenPredicateScope(spec, {})), RECORD_ROOT]); +export function screenPredicateRoots(node: ScreenPreviewNode): Set { + return new Set([...Object.keys(screenPredicateScope(buildScreenSpec(node), {})), RECORD_ROOT]); } -/** One field whose `visibleWhen` the screen renderer cannot evaluate, and why. */ -export interface UnevaluableVisibleWhen { - name: string; - error: string; +/** + * CEL's comprehension macros. Their first argument declares an iteration + * variable that is bound inside the macro's body — `["a","b"].exists(t, t == + * note)` binds `t` — and is not a reference to anything outside it. + */ +const CEL_COMPREHENSION_MACROS: ReadonlySet = new Set(['all', 'exists', 'exists_one', 'map', 'filter']); + +/** The parsed-AST node shape `parseCelToAst` hands back: an `op` and its `args`. */ +interface CelNode { + op: string; + args: unknown; +} + +function isCelNode(v: unknown): v is CelNode { + return !!v && typeof v === 'object' && typeof (v as CelNode).op === 'string'; +} + +/** + * The FREE root identifiers of a parsed predicate: every `id` node that is not + * bound by an enclosing comprehension macro. `collectCelRootIdentifiers` reports + * a macro's iteration variable as a root (`t` in `["a","b"].exists(t, t == + * note)`), which the runner then evaluates fine — so it was false-reported as + * "not a field on this screen". Walked over the same canonical AST + * (`parseCelToAst`), with the binding scoped to the macro's own arguments: an + * outer bare `t` in `t == 1 && ["a"].exists(t, t == note)` is still a root. + * + * Shape read: `rcall` is a receiver-style call `[name, receiver, [args…]]`, + * `call` a plain call `[name, [args…]]`, `.` a member access `[object, name]`; + * every other op carries its operands in `args`. Strings inside `args` are + * names, never identifiers, and are skipped. + */ +function collectFreeRoots(node: unknown, bound: ReadonlySet, out: Set): void { + if (Array.isArray(node)) { + for (const n of node) collectFreeRoots(n, bound, out); + return; + } + if (!isCelNode(node)) return; + if (node.op === 'id') { + if (typeof node.args === 'string' && !bound.has(node.args)) out.add(node.args); + return; + } + if (node.op === 'rcall' && Array.isArray(node.args)) { + const [macro, receiver, rest] = node.args as [unknown, unknown, unknown]; + collectFreeRoots(receiver, bound, out); + if ( + typeof macro === 'string' && + CEL_COMPREHENSION_MACROS.has(macro) && + Array.isArray(rest) && + isCelNode(rest[0]) && + rest[0].op === 'id' && + typeof rest[0].args === 'string' + ) { + const inner = new Set(bound); + inner.add(rest[0].args); + collectFreeRoots(rest.slice(1), inner, out); + return; + } + collectFreeRoots(rest, bound, out); + return; + } + collectFreeRoots(node.args, bound, out); +} + +/** + * The root identifiers a predicate references, comprehension variables + * excluded. Falls back to `collectCelRootIdentifiers` — which reports them — + * when the canonical parse hands back no AST, so an unexpected shape is never + * read as "no roots". + */ +function predicateRoots(source: string): { ok: true; roots: string[] } | { ok: false; error: string } { + const ast = parseCelToAst(source); + if (ast) { + const out = new Set(); + collectFreeRoots(ast, new Set(), out); + return { ok: true, roots: [...out] }; + } + return collectCelRootIdentifiers(source); } /** @@ -106,14 +188,13 @@ export interface UnevaluableVisibleWhen { * Judged in the order `registerFlow` judges the slot, minus the evaluation: * the spec's shape refusal (a non-string), the CEL parse (`validateExpression`, * which refuses a `{var}` brace — the brace trap in a bare-CEL slot), then the - * roots. A root that is not a declared field is named, with the nearest - * declared field when one is close (`dicount` → `discount`). + * roots against {@link screenPredicateRoots}. A root that is not a declared + * field is named, with the nearest declared field when one is close + * (`dicount` → `discount`). A predicate over a sibling field (`discount > 0`, + * `record.discount > 0`) is fine; one over a name the renderer never binds — a + * run variable such as `needsApproval`, the runtime's `vars` root — is not. */ -function visibleWhenScopeError( - visibleWhen: unknown, - roots: ReadonlySet, - declared: readonly string[], -): string | undefined { +export function screenVisibleWhenScopeError(visibleWhen: unknown, node: ScreenPreviewNode): string | undefined { if (visibleWhen === undefined || visibleWhen === null) return undefined; if (typeof visibleWhen === 'string' && !visibleWhen.trim()) return undefined; const shape = predicateSlotRefusal(visibleWhen); @@ -121,10 +202,12 @@ function visibleWhenScopeError( const source = visibleWhen as string; const parsed = validateExpression('predicate', source); if (parsed.errors.length > 0) return parsed.errors.map((e) => e.message).join(' '); - const collected = collectCelRootIdentifiers(source); + const collected = predicateRoots(source); if (!collected.ok) return collected.error; + const roots = screenPredicateRoots(node); const undeclared = collected.roots.filter((r) => !roots.has(r)); if (undeclared.length === 0) return undefined; + const declared = screenFields(buildScreenSpec(node)).map((f) => f.name); return undeclared .map((r) => { const near = nearestName(r, declared); @@ -133,28 +216,27 @@ function visibleWhenScopeError( .join('; '); } +/** One field whose `visibleWhen` the screen renderer cannot evaluate, and why. */ +export interface UnevaluableVisibleWhen { + name: string; + error: string; +} + /** * The authored field rows of a screen whose `visibleWhen` the screen renderer - * cannot evaluate, with the reason — see {@link visibleWhenScopeError}. A - * predicate over a sibling field (`createOpportunity == true`, `discount > 0`, - * `record.discount > 0`) is not reported: the renderer decides it live. One - * over a name the renderer never binds — a run variable such as - * `needsApproval == true` on a screen with no `needsApproval` field, the `vars` - * root the runtime's own scope carries — is, by field name, so the Debug run's - * screen step can name it (objectui#10743). + * cannot evaluate, with the reason — {@link screenVisibleWhenScopeError} per + * row, by field name, so the Debug run's screen step can name them + * (objectui#10743). */ export function unevaluableVisibleWhen(node: ScreenPreviewNode): UnevaluableVisibleWhen[] { const raw = (node.config as Record | undefined)?.fields; if (!Array.isArray(raw)) return []; - const spec = buildScreenSpec(node); - const roots = declaredPredicateRoots(spec); - const declared = screenFields(spec).map((f) => f.name); const out: UnevaluableVisibleWhen[] = []; for (const f of raw) { if (!f || typeof f !== 'object') continue; const row = f as Record; if (typeof row.name !== 'string' || !row.name) continue; - const error = visibleWhenScopeError(row.visibleWhen, roots, declared); + const error = screenVisibleWhenScopeError(row.visibleWhen, node); if (error) out.push({ name: row.name, error }); } return out; From c06451c56b24204dd3a08155021f0fbdc2f56853 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 11:43:40 +0000 Subject: [PATCH 3/3] docs(changeset): the record-restated-by-name clause and the run-variable half of the inline-cell statement (objectui#10743) Prose only, from the round-2 contract review: the Studio reads the bare roots off the renderer's own scope and restates only `record`, which the renderer binds inside `evalFieldPredicate`; the inline inspector's visibleWhen cell also says nothing on a run variable the Problems panel and the Debug run report. Frontmatter untouched. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_014mXUNuFomfj24w7s1pZzhN --- .changeset/10743-studio-screen-visiblewhen-live.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.changeset/10743-studio-screen-visiblewhen-live.md b/.changeset/10743-studio-screen-visiblewhen-live.md index b6b24f6aea..a0fd9b2246 100644 --- a/.changeset/10743-studio-screen-visiblewhen-live.md +++ b/.changeset/10743-studio-screen-visiblewhen-live.md @@ -44,8 +44,9 @@ collected values — not by the server". The Studio now follows it with one clie and is silent on the sibling. Every other expression slot keeps the flow scope. - The screen field inspector's `visibleWhen` placeholder is a sibling-field example (`createOpportunity == true`); it used to read `stage == "review"`, a run-variable shape. -- `ScreenView` exports `screenPredicateScope`, unchanged, so the Studio reads the roots a - predicate may name off the renderer's own scope instead of restating them. +- `ScreenView` exports `screenPredicateScope`, unchanged, so the Studio reads the bare roots a + predicate may name off the renderer's own scope; only the `record` namespace, which the + renderer binds inside `evalFieldPredicate`, is restated by name. **What did not change.** Whether the renderer shows or hides a predicate it cannot evaluate is `ScreenView`'s fallback and objectui#8069's open question; the Studio now renders through @@ -64,5 +65,5 @@ true, and waives it when they make it false or leave it unevaluable. **Still on the flow scope.** The inline inspector's `visibleWhen` cell (`FlowObjectListField`) still offers the flow scope picker and its inline note still reads the flow scope, so on the sibling-field shape it says "`discount` is not a reference in scope at this step" while the -Problems panel and the Debug run are silent; aligning that cell is a follow-up outside this -change. +Problems panel and the Debug run are silent, and says nothing on a run variable the other two +report; aligning that cell is a follow-up outside this change.