Skip to content

Commit 0b9c96c

Browse files
os-zhuangclaude
andauthored
fix(metadata-admin): flow data-picker — scope-aware ref validation + array assignments + script bare mode (#1934) (#1975)
Follow-ups to the variable data-picker (#1973): - Scope-aware "unknown reference" warning — pairs the picker with the inline validation the issue asked for: flags a typed reference whose ROOT isn't in scope at the node, with a nearest-match "did you mean?" hint. Deliberately conservative to avoid false positives (root-only so `record.<anything>` is fine when `record` is in scope; skips function/macro calls, string literals, CEL keywords and runtime globals like `env` / `$error`; typo hint only within edit distance 2). Non-blocking amber; the ADR-0032 brace error still wins. CEL for expression fields, `{…}` holes for templates. New pure `flow-ref-check` module with unit tests. - Assignment values authored in the array form `[{ variable, value }]` now render in the key/value editor (and so get the picker) instead of falling back to the Advanced JSON block. The editor reads BOTH the object-map and array shapes and preserves whichever was authored. Round-trip unit tests. - A script `code` body (JS/TS, not a `{var}` template) now inserts BARE references via a new `refMode` field-schema override — `{x}` is a syntax error in a script. Fixes a brace-mode regression from #1973 where every textarea was treated as a template. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 104d181 commit 0b9c96c

8 files changed

Lines changed: 404 additions & 30 deletions

File tree

.changeset/flow-ref-validation.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@object-ui/app-shell': patch
3+
---
4+
5+
Flow builder data-picker follow-ups (#1934): (1) a scope-aware "unknown reference" warning pairs the picker with inline validation — a typed reference whose root isn't in scope at the node is flagged with a nearest-match "did you mean?" hint (conservative: root-only, skips function calls / string literals / runtime globals; non-blocking amber). (2) Assignment values authored in the array form `[{ variable, value }]` now render in the key/value editor (and get the picker) instead of falling back to Advanced JSON; the editor reads both the object-map and array shapes and preserves whichever was authored. (3) A script `code` body (JS/TS, not a `{var}` template) now inserts bare references via a `refMode` field override — `{x}` is a syntax error in a script.

packages/app-shell/src/views/metadata-admin/inspectors/FlowEdgeInspector.tsx

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import { edgeKey, conditionText } from '../previews/flow-canvas-layout';
3131
import { validateExpressionClient } from './expression-validate';
3232
import { useFlowScope } from './useFlowScope';
3333
import { VariableTextInput } from './VariableTextInput';
34+
import { findUnknownRefs, scopeRoots, describeUnknownRefs } from './flow-ref-check';
3435

3536
interface FlowEdge {
3637
id?: string;
@@ -233,9 +234,21 @@ export function FlowEdgeInspector({ selection, draft, onPatch, onClearSelection,
233234
// ADR-0032 — flag a malformed edge guard (e.g. `{record.x}` brace-in-CEL)
234235
// inline, with the same corrective message as build/agent validation.
235236
const issue = isDefault ? null : validateExpressionClient('predicate', edge.condition);
236-
return issue ? (
237-
<p className="text-[11px] leading-snug text-destructive" role="alert">
238-
{issue.message}
237+
if (issue) {
238+
return (
239+
<p className="text-[11px] leading-snug text-destructive" role="alert">
240+
{issue.message}
241+
</p>
242+
);
243+
}
244+
// #1934 — gentle scope-aware "unknown reference" warning (refs in scope
245+
// at the edge's SOURCE node), once the guard is structurally valid.
246+
const unknown = isDefault
247+
? []
248+
: findUnknownRefs(conditionText(edge.condition), 'predicate', scopeRoots(scopeGroups.flatMap((g) => g.refs)));
249+
return unknown.length > 0 ? (
250+
<p className="text-[11px] leading-snug text-amber-600 dark:text-amber-400" role="note">
251+
{describeUnknownRefs(unknown)}
239252
</p>
240253
) : null;
241254
})()}
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { toEntries, rowsToValue, type Row } from './FlowKeyValueField';
5+
6+
const row = (key: string, raw: string): Row => ({ id: key, key, raw });
7+
8+
describe('FlowKeyValueField shape handling (#1934 — assignment array form)', () => {
9+
it('reads the object-map shape into entries', () => {
10+
expect(toEntries({ a: 1, b: 'x' })).toEqual([
11+
['a', 1],
12+
['b', 'x'],
13+
]);
14+
});
15+
16+
it('reads the assignment ARRAY shape ({variable|name|key, value}) into entries', () => {
17+
expect(toEntries([{ variable: 'lead_score', value: 0 }, { variable: 'qualified', value: false }])).toEqual([
18+
['lead_score', 0],
19+
['qualified', false],
20+
]);
21+
expect(toEntries([{ name: 'a', value: 1 }, { key: 'b', value: 2 }])).toEqual([
22+
['a', 1],
23+
['b', 2],
24+
]);
25+
});
26+
27+
it('ignores a non-object / non-array value', () => {
28+
expect(toEntries('nope')).toEqual([]);
29+
expect(toEntries(null)).toEqual([]);
30+
});
31+
32+
it('writes back the OBJECT shape, smart-parsing + de-duping', () => {
33+
const out = rowsToValue([row('amount', '30'), row('flag', 'true'), row('', 'skip'), row('amount', 'dupe')], false);
34+
expect(out).toEqual({ amount: 30, flag: true });
35+
});
36+
37+
it('writes back the ARRAY shape, preserving [{variable, value}]', () => {
38+
const out = rowsToValue([row('lead_score', '0'), row('qualified', 'false'), row('ref', '{record.id}')], true);
39+
expect(out).toEqual([
40+
{ variable: 'lead_score', value: 0 },
41+
{ variable: 'qualified', value: false },
42+
{ variable: 'ref', value: '{record.id}' },
43+
]);
44+
});
45+
46+
it('round-trips an array-shape assignment without changing its shape', () => {
47+
const stored = [{ variable: 'lead_score', value: 0 }, { variable: 'enrichment_data', value: null }];
48+
const rows = toEntries(stored).map(([k, v]) => row(k, v == null ? '' : String(v)));
49+
const out = rowsToValue(rows, /* arrayShape */ true);
50+
expect(Array.isArray(out)).toBe(true);
51+
expect((out as Array<Record<string, unknown>>).map((e) => e.variable)).toEqual(['lead_score', 'enrichment_data']);
52+
});
53+
});

packages/app-shell/src/views/metadata-admin/inspectors/FlowKeyValueField.tsx

Lines changed: 67 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import { uniqueId } from './_shared';
2424
import { VariableTextInput } from './VariableTextInput';
2525
import type { ScopeGroup } from './useFlowScope';
2626

27-
interface Row {
27+
export interface Row {
2828
id: string;
2929
key: string;
3030
/** Display string for the value cell. */
@@ -64,27 +64,64 @@ function parseValue(raw: string): unknown {
6464
return raw;
6565
}
6666

67-
function toRows(obj: Record<string, unknown>, existingIds: string[]): Row[] {
67+
/**
68+
* Read the stored value as `[key, value]` entries, accepting BOTH shapes a
69+
* key/value config field can hold: the common object map (`{ var: value }`) and
70+
* the assignment-node ARRAY form (`[{ variable|name|key, value }]`). The shape
71+
* is preserved on write (see {@link rowsToValue}).
72+
*/
73+
export function toEntries(value: unknown): Array<[string, unknown]> {
74+
if (Array.isArray(value)) {
75+
return value
76+
.filter((it): it is Record<string, unknown> => isPlainObject(it))
77+
.map((it) => {
78+
const k = it.variable ?? it.name ?? it.key;
79+
return [typeof k === 'string' ? k : '', it.value] as [string, unknown];
80+
});
81+
}
82+
if (isPlainObject(value)) return Object.entries(value);
83+
return [];
84+
}
85+
86+
function toRows(value: unknown, existingIds: string[]): Row[] {
6887
const ids = [...existingIds];
69-
return Object.entries(obj).map(([key, value]) => {
88+
return toEntries(value).map(([key, val]) => {
7089
const id = uniqueId('kv', ids);
7190
ids.push(id);
72-
return { id, key, raw: toRaw(value) };
91+
return { id, key, raw: toRaw(val) };
7392
});
7493
}
7594

76-
/** Flush rows to an object, skipping empty/duplicate keys (first wins). */
77-
function rowsToObject(rows: Row[]): Record<string, unknown> {
95+
/** Flush rows back to the SAME shape, skipping empty/duplicate keys (first wins). */
96+
export function rowsToValue(
97+
rows: Row[],
98+
arrayShape: boolean,
99+
): Record<string, unknown> | Array<Record<string, unknown>> {
100+
const seen = new Set<string>();
101+
if (arrayShape) {
102+
const list: Array<Record<string, unknown>> = [];
103+
for (const r of rows) {
104+
const k = r.key.trim();
105+
if (!k || seen.has(k)) continue;
106+
seen.add(k);
107+
list.push({ variable: k, value: parseValue(r.raw) });
108+
}
109+
return list;
110+
}
78111
const out: Record<string, unknown> = {};
79112
for (const r of rows) {
80113
const k = r.key.trim();
81-
if (!k || k in out) continue;
114+
if (!k || seen.has(k)) continue;
115+
seen.add(k);
82116
out[k] = parseValue(r.raw);
83117
}
84118
return out;
85119
}
86120

87-
function serialize(obj: Record<string, unknown>): string {
121+
/** Stable serialization for the resync guard (order-insensitive for objects). */
122+
function serialize(value: Record<string, unknown> | Array<Record<string, unknown>> | undefined): string {
123+
if (Array.isArray(value)) return JSON.stringify(value);
124+
const obj = value ?? {};
88125
const sorted = Object.keys(obj).sort().reduce<Record<string, unknown>>((acc, k) => {
89126
acc[k] = obj[k];
90127
return acc;
@@ -95,7 +132,7 @@ function serialize(obj: Record<string, unknown>): string {
95132
export interface FlowKeyValueFieldProps {
96133
label: string;
97134
value: unknown;
98-
onCommit: (value: Record<string, unknown> | undefined) => void;
135+
onCommit: (value: Record<string, unknown> | Array<Record<string, unknown>> | undefined) => void;
99136
disabled?: boolean;
100137
help?: string;
101138
addLabel: string;
@@ -120,24 +157,32 @@ export function FlowKeyValueField({
120157
emptyLabel,
121158
scopeGroups,
122159
}: FlowKeyValueFieldProps) {
123-
const external = isPlainObject(value) ? value : {};
124-
const [rows, setRows] = React.useState<Row[]>(() => toRows(external, []));
125-
// Track the last value we committed so an external change (node switch) can
126-
// resync rows without clobbering an in-progress edit of the same node.
127-
const lastCommitted = React.useRef(serialize(external));
160+
// Preserve whichever shape the value was authored in (object map vs the
161+
// assignment-node array form) across edits.
162+
const arrayShape = Array.isArray(value);
163+
// Normalized serialization of the stored value — used only to detect an
164+
// EXTERNAL change (node switch) that should resync the rows.
165+
const external = React.useMemo(
166+
() => serialize(rowsToValue(toRows(value, []), arrayShape)),
167+
[value, arrayShape],
168+
);
169+
const [rows, setRows] = React.useState<Row[]>(() => toRows(value, []));
170+
// Track the last value we committed so an external change can resync rows
171+
// without clobbering an in-progress edit of the same node.
172+
const lastCommitted = React.useRef(external);
128173

129174
React.useEffect(() => {
130-
const next = serialize(external);
131-
if (next !== lastCommitted.current) {
132-
setRows(toRows(external, []));
133-
lastCommitted.current = next;
175+
if (external !== lastCommitted.current) {
176+
setRows(toRows(value, []));
177+
lastCommitted.current = external;
134178
}
135-
}, [external]);
179+
}, [external, value]);
136180

137181
const flush = (nextRows: Row[]) => {
138-
const obj = rowsToObject(nextRows);
139-
lastCommitted.current = serialize(obj);
140-
onCommit(Object.keys(obj).length ? obj : undefined);
182+
const out = rowsToValue(nextRows, arrayShape);
183+
lastCommitted.current = serialize(out);
184+
const empty = Array.isArray(out) ? out.length === 0 : Object.keys(out).length === 0;
185+
onCommit(empty ? undefined : out);
141186
};
142187

143188
const setRowField = (id: string, patch: Partial<Row>) => {

packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { FlowReferenceField, type FlowReferenceContext } from './FlowReferenceFi
2222
import { validateExpressionClient } from './expression-validate';
2323
import { VariableTextInput } from './VariableTextInput';
2424
import type { ScopeGroup } from './useFlowScope';
25+
import { findUnknownRefs, scopeRoots, describeUnknownRefs } from './flow-ref-check';
2526

2627
export interface FlowNodeConfigFieldProps {
2728
field: FlowConfigField;
@@ -36,6 +37,8 @@ export interface FlowNodeConfigFieldProps {
3637
}
3738

3839
export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, context, scopeGroups }: FlowNodeConfigFieldProps) {
40+
const refMode: 'expression' | 'template' =
41+
field.refMode ?? (field.kind === 'expression' ? 'expression' : 'template');
3942
const control = (() => {
4043
switch (field.kind) {
4144
case 'reference':
@@ -127,7 +130,7 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale,
127130
<VariableTextInput
128131
multiline
129132
rows={4}
130-
mode="template"
133+
mode={refMode}
131134
value={value != null ? String(value) : ''}
132135
onValueChange={(v) => onCommit(v)}
133136
groups={scopeGroups ?? []}
@@ -143,7 +146,7 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale,
143146
<div className="space-y-1">
144147
<Label className="text-xs text-muted-foreground">{field.label}</Label>
145148
<VariableTextInput
146-
mode={field.kind === 'expression' ? 'expression' : 'template'}
149+
mode={refMode}
147150
mono={field.kind === 'expression'}
148151
value={value != null ? String(value) : ''}
149152
onValueChange={(v) => onCommit(v)}
@@ -157,11 +160,26 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale,
157160
})();
158161

159162
// ADR-0032 — surface a malformed condition (e.g. the `{record.x}` brace-in-CEL
160-
// mistake) inline, as the author types, with the same corrective message the
161-
// build and the agent tool emit. Only checked for expression-bearing fields.
163+
// mistake) inline, with the same corrective message the build/agent emit. Only
164+
// for expression fields (a genuine template uses single-brace `{var}` legally).
162165
const exprIssue =
163166
field.kind === 'expression' ? validateExpressionClient('predicate', value) : null;
164167

168+
// #1934 — pair the picker with a gentle, scope-aware "unknown reference"
169+
// warning: CEL for expression fields, `{…}` holes for template fields. Skipped
170+
// for free-form code (refMode 'expression' on a textarea, e.g. a script body)
171+
// and when scope is unknown. The brace error above takes precedence.
172+
const scopeRole: 'predicate' | 'template' | null =
173+
field.kind === 'expression'
174+
? 'predicate'
175+
: refMode === 'template' && (field.kind === 'text' || field.kind === 'textarea')
176+
? 'template'
177+
: null;
178+
const unknownRefs =
179+
!exprIssue && scopeRole && scopeGroups && scopeGroups.length > 0
180+
? findUnknownRefs(value, scopeRole, scopeRoots(scopeGroups.flatMap((g) => g.refs)))
181+
: [];
182+
165183
return (
166184
<div className="space-y-1">
167185
{control}
@@ -170,7 +188,12 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale,
170188
{exprIssue.message}
171189
</p>
172190
)}
173-
{field.help && !exprIssue && (
191+
{!exprIssue && unknownRefs.length > 0 && (
192+
<p className="text-[11px] leading-snug text-amber-600 dark:text-amber-400" role="note">
193+
{describeUnknownRefs(unknownRefs)}
194+
</p>
195+
)}
196+
{field.help && !exprIssue && unknownRefs.length === 0 && (
174197
<p className="text-[11px] leading-snug text-muted-foreground">{field.help}</p>
175198
)}
176199
</div>

packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,13 @@ export interface FlowConfigField {
147147
columns?: FlowConfigColumn[];
148148
/** Reference target for `reference` fields — drives the combobox data source. */
149149
ref?: FlowReferenceSpec;
150+
/**
151+
* Data-picker brace mode override (#1934). Defaults by kind (`expression` →
152+
* bare CEL, `text` / `textarea` → `{var}` template). Set `'expression'` on a
153+
* code field (e.g. a script body) so the picker inserts bare references, not
154+
* `{var}` — `{x}` is a syntax error in a JS/TS script.
155+
*/
156+
refMode?: 'expression' | 'template';
150157
}
151158

152159
/** Convenience: a `['config', key]`-rooted field (the common case). */
@@ -327,6 +334,7 @@ const FLOW_NODE_CONFIG: Record<string, FlowConfigField[]> = {
327334
cfg('script', 'Code', 'textarea', {
328335
placeholder: 'return { ok: true };',
329336
help: 'Script body (JS/TS).',
337+
refMode: 'expression',
330338
showWhen: { field: 'actionType', equals: ['code'] },
331339
}),
332340
cfg('outputVariables', 'Output variables', 'stringList', {

0 commit comments

Comments
 (0)