Skip to content

Commit 5cc47d1

Browse files
committed
fix(automation): put a screen field's visibleWhen on the wire (#3528)
`visibleWhen` has been on the `screen` node's designer form since #3304 — declared `xExpression`, documented as bare CEL, offered in Studio — but the executor dropped it when building the paused payload. `ScreenFieldSpec` stopped at `placeholder`, so no client ever received the predicate and none could honour it. Authors wrote conditional visibility; every field rendered unconditionally; nothing errored. `required` *is* honoured, which is what makes this fatal rather than cosmetic. A field that is optional-by-design but required when shown becomes permanently required once its predicate is dropped, and a runner validating the full field list then blocks Submit on input the user was never asked for — no resume request is issued and the run stays paused. That is the reported #3528 symptom, reproduced in a browser against HotCRM's lead-conversion screen on both the console shipped with 16.1.0 and current objectui main: trigger fires, the screen renders all three fields, Submit issues zero network requests. - `ScreenFieldSpec.visibleWhen` joins the contract, documented as client-evaluated bare CEL over the screen's own field names, and states the rule an implementor needs: a hidden field must not be enforced as required. - The executor forwards it raw. Interpolating here would resolve it once against flow variables, freezing a predicate the client must re-evaluate against values only it can see. - Tests for the screen wire payload, which had none for this key; both new assertions verified to fail without the forwarding line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0122xvfanLmniNbsAPtg5hk3
1 parent 6877e9a commit 5cc47d1

4 files changed

Lines changed: 156 additions & 0 deletions

File tree

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/service-automation": minor
4+
---
5+
6+
fix(automation): a screen field's `visibleWhen` reaches the client (#3528)
7+
8+
`visibleWhen` has been on the `screen` node's designer form since #3304
9+
declared as an expression (`xExpression`), documented as bare CEL, offered to
10+
authors in Studio. The executor never put it on the wire. `ScreenFieldSpec`
11+
carried `name` / `label` / `type` / `required` / `options` / `defaultValue` /
12+
`placeholder` and nothing else, so no client could honour a predicate it never
13+
received. Authors wrote conditional visibility; every field rendered
14+
unconditionally; nothing errored.
15+
16+
That is worse than a cosmetic miss, because `required` **is** honoured. A field
17+
that is optional-by-design but required *when shown* becomes permanently
18+
required once its predicate is dropped — and a runner that validates the full
19+
field list then blocks Submit on input the user was never asked for. No resume
20+
request is issued and the run sits paused forever. HotCRM's lead-conversion
21+
screen is exactly that shape:
22+
23+
```ts
24+
{ name: 'createOpportunity', type: 'boolean', required: true },
25+
{ name: 'opportunityName', type: 'text', required: true,
26+
visibleWhen: 'createOpportunity == true' },
27+
```
28+
29+
Leave the checkbox unticked and `opportunityName` — which should not be on
30+
screen at all — blocks the whole conversion.
31+
32+
- `ScreenFieldSpec.visibleWhen` is now part of the contract, documented as
33+
client-evaluated bare CEL over the screen's own field names, with the
34+
`required`-must-follow-visibility rule stated where implementors will read it.
35+
- The `screen` executor forwards it **raw**, deliberately uninterpolated: the
36+
predicate is re-evaluated per keystroke against values only the client has, so
37+
resolving it server-side against flow variables would freeze the field.
38+
- Covered by tests — the screen wire payload had none for this key.
39+
40+
Clients must evaluate the predicate and skip hidden fields when enforcing
41+
`required`. Honouring one without the other reproduces the dead-end above.

packages/services/service-automation/src/builtin/screen-nodes.test.ts

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,3 +151,89 @@ it('resolves config.functionName as an alias for function (#1870 DX)', async ()
151151
expect(seen).toEqual([{ cat: 'billing', conf: 0.9 }]);
152152
});
153153
});
154+
155+
/** A one-`screen`-node flow whose screen node carries `config`. */
156+
function screenFlow(config: Record<string, unknown>) {
157+
return {
158+
name: 'screen_flow',
159+
label: 'Screen Flow',
160+
type: 'screen' as const,
161+
nodes: [
162+
{ id: 'start', type: 'start' as const, label: 'Start' },
163+
{ id: 'collect', type: 'screen' as const, label: 'Collect', config },
164+
{ id: 'end', type: 'end' as const, label: 'End' },
165+
],
166+
edges: [
167+
{ id: 'e1', source: 'start', target: 'collect' },
168+
{ id: 'e2', source: 'collect', target: 'end' },
169+
],
170+
};
171+
}
172+
173+
describe('screen node — the field wire payload (#3528)', () => {
174+
let engine: AutomationEngine;
175+
176+
beforeEach(() => {
177+
engine = new AutomationEngine(createTestLogger());
178+
registerScreenNodes(engine, createCtx());
179+
});
180+
181+
/**
182+
* `visibleWhen` has been on the screen node's designer form since #3304 but
183+
* was dropped when the executor built the paused payload, so it reached no
184+
* client and nothing honoured it. HotCRM's lead-conversion screen is the
185+
* shape that made it fatal: an optional-by-design field that is `required`
186+
* *when shown*. Rendered unconditionally, it blocks Submit on input the
187+
* user was never asked for, and the run never resumes.
188+
*/
189+
it('forwards visibleWhen to the paused screen so the client can honour it', async () => {
190+
engine.registerFlow('screen_flow', screenFlow({
191+
title: 'Conversion Details',
192+
fields: [
193+
{ name: 'createOpportunity', label: 'Create Opportunity?', type: 'boolean', required: true },
194+
{ name: 'opportunityName', label: 'Opportunity Name', type: 'text', required: true, visibleWhen: 'createOpportunity == true' },
195+
{ name: 'opportunityAmount', label: 'Opportunity Amount', type: 'currency', visibleWhen: 'createOpportunity == true' },
196+
],
197+
}) as any);
198+
199+
const paused = await engine.execute('screen_flow');
200+
expect(paused.status).toBe('paused');
201+
const fields = paused.screen!.fields;
202+
expect(fields.map((f) => f.visibleWhen)).toEqual([
203+
undefined,
204+
'createOpportunity == true',
205+
'createOpportunity == true',
206+
]);
207+
// The conditional field keeps `required` — it is required *when shown*.
208+
// Honouring one without the other is what dead-ends the run.
209+
expect(fields[1]).toMatchObject({ name: 'opportunityName', required: true });
210+
});
211+
212+
it('leaves visibleWhen undefined when the author declared none', async () => {
213+
engine.registerFlow('screen_flow', screenFlow({
214+
fields: [{ name: 'subject', label: 'Subject', type: 'text', required: true }],
215+
}) as any);
216+
217+
const paused = await engine.execute('screen_flow');
218+
expect(paused.screen!.fields[0].visibleWhen).toBeUndefined();
219+
});
220+
221+
/**
222+
* The predicate is re-evaluated by the client on every keystroke against
223+
* the values collected so far, which the server cannot see. Interpolating
224+
* it here would bake in a verdict from flow variables and freeze the field.
225+
*/
226+
it('forwards the predicate RAW — it is not interpolated against flow variables', async () => {
227+
engine.registerFlow('screen_flow', screenFlow({
228+
fields: [
229+
{ name: 'tier', label: 'Tier', type: 'text' },
230+
// `{recordId}` is a live flow variable; were the predicate
231+
// interpolated it would come back with the value substituted.
232+
{ name: 'note', label: 'Note', type: 'text', visibleWhen: 'tier == "gold" && "{recordId}" != ""' },
233+
],
234+
}) as any);
235+
236+
const paused = await engine.execute('screen_flow', { params: { recordId: 'lead_1' } } as any);
237+
expect(paused.screen!.fields[1].visibleWhen).toBe('tier == "gold" && "{recordId}" != ""');
238+
});
239+
});

packages/services/service-automation/src/builtin/screen-nodes.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,20 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext
137137
options: Array.isArray(f.options) ? (f.options as Array<{ value: unknown; label: string }>) : undefined,
138138
defaultValue: f.defaultValue !== undefined ? interpolate(f.defaultValue, variables, context) : undefined,
139139
placeholder: f.placeholder != null ? String(f.placeholder) : undefined,
140+
// Forwarded RAW — deliberately not interpolated. `visibleWhen` is a
141+
// predicate the client re-evaluates on every keystroke against the
142+
// values collected SO FAR; the server has no view of those, so
143+
// resolving it here (to a constant, against flow variables) would
144+
// freeze the field visible-or-hidden for the life of the screen.
145+
//
146+
// It was declared on the designer form from the start but never put
147+
// on the wire, so no client could honour it: authors wrote a
148+
// predicate, Studio offered the input, and the field rendered
149+
// unconditionally. Where that field was also `required`, a runner
150+
// validating the full list blocked Submit on a field the user was
151+
// never shown — the run then sat paused with no resume request ever
152+
// issued (#3528).
153+
visibleWhen: f.visibleWhen != null ? String(f.visibleWhen) : undefined,
140154
})).filter((f) => f.name.length > 0);
141155
return {
142156
success: true,

packages/spec/src/contracts/automation-service.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,21 @@ export interface ScreenFieldSpec {
9595
options?: Array<{ value: unknown; label: string }>;
9696
defaultValue?: unknown;
9797
placeholder?: string;
98+
/**
99+
* Conditional-visibility predicate (ADR-0089's canonical key), evaluated by
100+
* the CLIENT against the screen's live collected values — not by the server,
101+
* which has no view of what the user has typed so far.
102+
*
103+
* Bare CEL over the screen's own field names, e.g. `createOpportunity ==
104+
* true` shows the field only once that checkbox is ticked. Omit = always
105+
* visible.
106+
*
107+
* A hidden field is not collected, so it must not be enforced as `required`
108+
* either — a client that renders this predicate but validates over the full
109+
* field list dead-ends the run: Submit blocks on a field the user was never
110+
* shown, and no resume is ever issued (#3528).
111+
*/
112+
visibleWhen?: string;
98113
}
99114

100115
/**

0 commit comments

Comments
 (0)