diff --git a/.changeset/flow-nested-node-inspector.md b/.changeset/flow-nested-node-inspector.md new file mode 100644 index 000000000..c4fd431ab --- /dev/null +++ b/.changeset/flow-nested-node-inspector.md @@ -0,0 +1,36 @@ +--- +"@object-ui/app-shell": minor +--- + +feat(studio): select and edit nested container nodes through the schema-driven flow inspector (#2670) + +Phase 2 (#2680) expanded a container's regions (`loop.body` / +`parallel.branches[]` / `try_catch.try`/`catch`) inline on the flow designer +canvas, but the nested nodes were read-only — changing one still meant editing +the container's Advanced JSON by hand. A nested node is now a first-class +selection: click it on the expanded canvas and it opens in the SAME +schema-driven inspector as a top-level node, with a `container › region › node` +breadcrumb. Edits (label / type / description / typed config fields / Advanced +JSON) write straight back into `config..nodes[i]` — the write rebuilds +the container with explicit spreads so the `config.branches` array stays an +array and each region's own `edges` / a branch's `name` are preserved. + +Scope resolves correctly for the region's outer context (ADR-0031): a loop +body node sees the loop's `iteratorVariable` in its data picker even though the +container's own outputs are otherwise out of scope at its id. + +This phase is edit-only by design. A nested node keeps its id read-only (rename +it in the container's Advanced JSON), has no delete, and — for a nested +decision — drops the virtual Target column, since a region sub-graph's internal +routing is not managed by the inspector yet (nested region-edge editing, +structural add/remove, and drag are follow-ups). A stale nested deep link +(the draft moved on) resolves to a harmless empty-state rather than writing to +the wrong node. + +Also fixes an expression/template validation split now that the engine +publishes a loop `configSchema`: a string property can carry an `xExpression: +'expression' | 'template'` marker so the designer renders bare-CEL vs +`interpolate()` `{var}` semantics (mono editor, data-picker brace mode, and +whether the CEL brace-trap applies) instead of guessing from the field name. A +loop / map `collection` (`{leadList}`) is a template, so it no longer +false-positives as a malformed condition inline or on the canvas badge. diff --git a/apps/console/src/preview-samples.ts b/apps/console/src/preview-samples.ts index 309bf9c97..e52732dcf 100644 --- a/apps/console/src/preview-samples.ts +++ b/apps/console/src/preview-samples.ts @@ -118,6 +118,27 @@ export const SAMPLES: Record> = { nodes: [ { id: 'start', type: 'start', label: 'Schedule (daily)', config: { triggerType: 'schedule', objectName: 'contract', condition: 'status == "active"', cron: '0 9 * * *' } }, { id: 'find', type: 'get_record', label: 'Find expiring contracts', config: { objectName: 'contract', filter: { status: 'active' }, outputVariable: 'contracts' } }, + // ADR-0031 structured container: a loop whose `config.body` region is a + // nested sub-graph. On the designer canvas it expands inline (#2680) and + // its body nodes are selectable + editable through the same schema-driven + // inspector as top-level nodes (#2670 Phase 3). + { + id: 'each_contract', + type: 'loop', + label: 'For each expiring contract', + config: { + collection: '{contracts}', + iteratorVariable: 'contract', + body: { + nodes: [ + { id: 'charge_fee', type: 'http_request', label: 'Charge renewal fee', config: { method: 'POST', url: 'https://api.example.com/billing/charge', outputVariable: 'charge' } }, + ], + edges: [], + }, + }, + }, + { id: 'fan_out', type: 'parallel', label: 'Notify in parallel', config: { branches: [ { name: 'Email the owner', nodes: [ { id: 'email_owner', type: 'script', label: 'Email Owner', config: { actionType: 'email', template: 'renewal_reminder', recipients: ['owner.email'] } } ], edges: [] }, { name: 'Post to Slack', nodes: [ { id: 'slack_post', type: 'script', label: 'Slack Notify', config: { actionType: 'slack' } } ], edges: [] } ] } }, + { id: 'guard', type: 'try_catch', label: 'Push with retry', config: { errorVariable: '$error', try: { nodes: [ { id: 'push', type: 'http_request', label: 'Push to CRM', config: { method: 'POST', url: 'https://api.example.com/v1/tasks' } } ], edges: [] }, catch: { nodes: [ { id: 'record_failure', type: 'update_record', label: 'Flag Sync Failure', config: { objectName: 'contract', fields: { reminded: false } } } ], edges: [] } } }, { id: 'check', type: 'decision', label: 'Within reminder window?', config: { conditions: [ { label: 'Within window', expression: 'daysToExpiry <= daysBefore' }, { label: 'Else', expression: 'true' } ] } }, { id: 'email', type: 'connector_action', label: 'Send renewal email', connectorConfig: { connectorId: 'email', actionId: 'send', input: { to: 'owner.email', template: 'renewal_reminder' } } }, { id: 'wait', type: 'wait', label: 'Wait 3 days', waitEventConfig: { eventType: 'timer', timerDuration: 'P3D', onTimeout: 'continue' } }, @@ -130,7 +151,10 @@ export const SAMPLES: Record> = { ], edges: [ { source: 'start', target: 'find' }, - { source: 'find', target: 'check' }, + { source: 'find', target: 'each_contract' }, + { source: 'each_contract', target: 'fan_out' }, + { source: 'fan_out', target: 'guard' }, + { source: 'guard', target: 'check' }, { source: 'check', target: 'email', condition: 'daysToExpiry <= daysBefore' }, { source: 'check', target: 'skip', isDefault: true }, { source: 'email', target: 'wait' }, diff --git a/packages/app-shell/src/views/metadata-admin/i18n.ts b/packages/app-shell/src/views/metadata-admin/i18n.ts index ca891c74f..6c3be55d0 100644 --- a/packages/app-shell/src/views/metadata-admin/i18n.ts +++ b/packages/app-shell/src/views/metadata-admin/i18n.ts @@ -338,6 +338,7 @@ const ENGINE_STRINGS_EN: Record = { 'engine.inspector.flowNode.advanced': 'Advanced (JSON)', 'engine.inspector.flowNode.advancedHint': 'Optional custom keys not covered by the form above — most flows don\u2019t need this.', 'engine.inspector.flowNode.noConfig': 'No configuration needed for this node type.', + 'engine.inspector.flowNode.nestedIdHint': 'A node inside a container region keeps its id here — rename it in the container’s Advanced JSON.', 'engine.inspector.flowNode.kv.add': 'Add entry', 'engine.inspector.flowNode.kv.key': 'Key', 'engine.inspector.flowNode.kv.value': 'Value', @@ -1715,6 +1716,7 @@ const ENGINE_STRINGS_ZH: Record = { 'engine.inspector.flowNode.advanced': '高级（JSON）', 'engine.inspector.flowNode.advancedHint': '上方表单未覆盖的可选自定义键 —— 大多数流程无需填写。', 'engine.inspector.flowNode.noConfig': '此节点类型无需配置。', + 'engine.inspector.flowNode.nestedIdHint': '容器区域内的节点 ID 在此只读 —— 请在容器的高级 JSON 中重命名。', 'engine.inspector.flowNode.kv.add': '添加条目', 'engine.inspector.flowNode.kv.key': '键', 'engine.inspector.flowNode.kv.value': '值', diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.test.tsx new file mode 100644 index 000000000..f89642e5b --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.test.tsx @@ -0,0 +1,102 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * FlowNodeConfigField — inline expression/template validation gating (#2670 + * Phase 3 B). The malformed-condition brace-trap (ADR-0032) must fire on a real + * CEL predicate but stay silent on a legitimate `interpolate()` `{var}` template + * — an expression field flagged `refMode: 'template'` (e.g. a loop/map + * collection). The scope-aware unknown-reference note still applies to a + * template, using `{…}`-hole semantics rather than CEL predicate semantics. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { FlowNodeConfigField } from './FlowNodeConfigField'; +import type { FlowConfigField } from './flow-node-config'; +import type { ScopeGroup } from './useFlowScope'; + +afterEach(cleanup); + +/** A minimal scope with the given variable tokens (all in the `variables` group). */ +function scope(tokens: string[]): ScopeGroup[] { + return [ + { + id: 'variables', + label: 'Flow variables', + refs: tokens.map((token) => ({ token, label: token, group: 'variables' as const })), + }, + ]; +} + +const TEMPLATE_COLLECTION: FlowConfigField = { + id: 'collection', + path: ['config', 'collection'], + label: 'Collection', + kind: 'expression', + refMode: 'template', +}; + +const CEL_CONDITION: FlowConfigField = { + id: 'condition', + path: ['config', 'condition'], + label: 'Condition', + kind: 'expression', +}; + +describe('FlowNodeConfigField — expression vs template validation gating', () => { + it('does NOT flag a `{leadList}` template on an expression field in template mode', () => { + render( + {}} + scopeGroups={scope(['leadList'])} + />, + ); + // The pre-fix bug: the CEL brace-trap fired on the legal single-brace hole. + expect(screen.queryByRole('alert')).toBeNull(); + // In scope → no unknown-reference note either. + expect(screen.queryByRole('note')).toBeNull(); + }); + + it('surfaces an out-of-scope hole as a template NOTE (not a CEL error)', () => { + render( + {}} + scopeGroups={scope(['leadList'])} + />, + ); + // No brace-trap error… + expect(screen.queryByRole('alert')).toBeNull(); + // …but a gentle "did you mean" note, scanned as a `{…}` template hole. + const note = screen.getByRole('note'); + expect(note.textContent).toMatch(/leadList/); + }); + + it('still flags a genuine `{record.x}` brace-in-CEL mistake on a predicate field', () => { + render( + {}} + scopeGroups={scope(['record'])} + />, + ); + expect(screen.getByRole('alert')).toBeInTheDocument(); + }); + + it('does not flag a well-formed CEL predicate with an in-scope reference', () => { + render( + {}} + scopeGroups={scope(['amount'])} + />, + ); + expect(screen.queryByRole('alert')).toBeNull(); + expect(screen.queryByRole('note')).toBeNull(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx index 3f6b21bbd..101dd7843 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx @@ -182,17 +182,24 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, // ADR-0032 — surface a malformed condition (e.g. the `{record.x}` brace-in-CEL // mistake) inline, with the same corrective message the build/agent emit. Only - // for expression fields (a genuine template uses single-brace `{var}` legally). + // for expression fields in a *predicate* mode — an expression field flagged + // `refMode: 'template'` (e.g. a loop/map collection authored as `{leadList}`) + // is an `interpolate()` single-brace template where `{var}` is legal, so the + // CEL brace-trap must be gated off or it false-positives on every `{…}`. + const isTemplate = refMode === 'template'; const exprIssue = - field.kind === 'expression' ? validateExpressionClient('predicate', value) : null; + field.kind === 'expression' && !isTemplate ? validateExpressionClient('predicate', value) : null; // #1934 — pair the picker with a gentle, scope-aware "unknown reference" - // warning: CEL for expression fields, `{…}` holes for template fields. Skipped - // for free-form code (refMode 'expression' on a textarea, e.g. a script body) - // and when scope is unknown. The brace error above takes precedence. + // warning: CEL for predicate expression fields, `{…}` holes for template + // fields (including an expression field in template mode). Skipped for + // free-form code (refMode 'expression' on a textarea, e.g. a script body) and + // when scope is unknown. The brace error above takes precedence. const scopeRole: 'predicate' | 'template' | null = field.kind === 'expression' - ? 'predicate' + ? isTemplate + ? 'template' + : 'predicate' : refMode === 'template' && (field.kind === 'text' || field.kind === 'textarea') ? 'template' : null; diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.test.tsx new file mode 100644 index 000000000..8a2255f73 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.test.tsx @@ -0,0 +1,177 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * FlowNodeInspector — nested-node editing (#2670 Phase 3 C2). A node inside a + * container region routes to the SAME schema-driven inspector as a top-level + * node, and edits write back into `config..nodes[i]` (explicit spreads, + * never a path walk that would objectify the `config.branches` array). Nested + * nodes are edit-only this phase: read-only id, no delete, and a nested decision + * drops the virtual Target column + never mirrors top-level edges. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; + +// The engine config-schema hook is stubbed empty so the inspector uses its +// hardcoded field groups (fieldsForNodeType); the trigger field catalog is +// stubbed so useFlowScope resolves without a network client. +vi.mock('../previews/useFlowNodePalette', () => ({ + useActionConfigSchemas: () => ({}), + useFlowNodePalette: () => [], +})); +vi.mock('../previews/useObjectFields', () => ({ + useObjectFields: () => ({ fields: [], loading: false, error: null }), +})); + +import { FlowNodeInspector } from './FlowNodeInspector'; +import { encodeNestedNodeId, NESTED_NODE_KIND } from './flow-nested-selection'; +import type { MetadataSelection } from '../preview-registry'; + +afterEach(cleanup); + +function makeDraft() { + return { + nodes: [ + { id: 'start', type: 'start' }, + { + id: 'each', + type: 'loop', + label: 'For each', + config: { + collection: '{items}', + iteratorVariable: 'contract', + body: { + nodes: [ + { id: 'charge', type: 'http_request', label: 'Charge', config: { method: 'POST', url: 'https://x/charge', outputVariable: 'chargeResult' } }, + ], + edges: [{ source: 'charge', target: 'charge' }], + }, + }, + }, + { + id: 'fan', + type: 'parallel', + config: { + branches: [ + { name: 'Slack', nodes: [{ id: 's', type: 'http_request', label: 'Slack' }], edges: [] }, + { nodes: [{ id: 'c', type: 'http_request', label: 'CRM' }], edges: [] }, + ], + }, + }, + { + id: 'decide', + type: 'loop', + label: 'Loop w/ decision', + config: { body: { nodes: [{ id: 'branchpoint', type: 'decision', label: 'Branch?', config: { conditions: [{ label: 'Yes', expression: 'x > 1' }] } }], edges: [] } }, + }, + ], + edges: [{ source: 'start', target: 'each' }], + }; +} + +function renderInspector(selection: MetadataSelection, draft: Record = makeDraft()) { + const onPatch = vi.fn(); + const onClearSelection = vi.fn(); + const utils = render( + , + ); + return { onPatch, onClearSelection, ...utils }; +} + +const lastPatch = (onPatch: ReturnType) => onPatch.mock.calls.at(-1)![0] as any; + +describe('FlowNodeInspector — top-level regression', () => { + it('edits a top-level node and offers delete', () => { + const { onPatch } = renderInspector({ kind: 'node', id: 'each' }); + expect(screen.getByRole('button', { name: /remove node/i })).toBeInTheDocument(); + fireEvent.change(screen.getByDisplayValue('For each'), { target: { value: 'Each order' } }); + expect(lastPatch(onPatch).nodes[1].label).toBe('Each order'); + }); +}); + +describe('FlowNodeInspector — nested node editing', () => { + const bodyChargeId = encodeNestedNodeId({ containerId: 'each', regionKey: 'body', nodeId: 'charge' }); + + it('opens a nested node with a container › region › node breadcrumb and its config fields', () => { + const { container } = renderInspector({ kind: NESTED_NODE_KIND, id: bodyChargeId, label: 'Charge' }); + const crumb = container.querySelector('[aria-label="nested node location"]')!; + expect(crumb).not.toBeNull(); + expect(crumb.textContent).toContain('For each'); // container label + expect(crumb.textContent).toContain('Body'); // region label + expect(crumb.textContent).toContain('Charge'); // node label + // The http_request schema field renders (its URL value is editable inline). + expect(screen.getByDisplayValue('https://x/charge')).toBeInTheDocument(); + }); + + it('writes a nested body node into config.body.nodes[i] without touching edges', () => { + const { onPatch } = renderInspector({ kind: NESTED_NODE_KIND, id: bodyChargeId }); + fireEvent.change(screen.getByDisplayValue('Charge'), { target: { value: 'Charge card' } }); + const patch = lastPatch(onPatch); + expect(patch.edges).toBeUndefined(); // no top-level edge mirroring + expect(patch.nodes[1].config.body.nodes[0].label).toBe('Charge card'); + expect(patch.nodes[1].config.body.edges).toEqual([{ source: 'charge', target: 'charge' }]); // region edges preserved + expect(patch.nodes[1].config.collection).toBe('{items}'); // sibling config preserved + }); + + it('writes a nested parallel branch node, keeping config.branches an ARRAY (D5)', () => { + const { onPatch } = renderInspector({ + kind: NESTED_NODE_KIND, + id: encodeNestedNodeId({ containerId: 'fan', regionKey: 'branch-1', nodeId: 'c' }), + }); + fireEvent.change(screen.getByDisplayValue('CRM'), { target: { value: 'Notify CRM' } }); + const patch = lastPatch(onPatch); + expect(Array.isArray(patch.nodes[2].config.branches)).toBe(true); + expect(patch.nodes[2].config.branches[1].nodes[0].label).toBe('Notify CRM'); + expect(patch.nodes[2].config.branches[0].name).toBe('Slack'); // sibling branch untouched + }); + + it('locks the id and hides delete for a nested node, with a hint', () => { + renderInspector({ kind: NESTED_NODE_KIND, id: bodyChargeId }); + expect(screen.queryByRole('button', { name: /remove node/i })).toBeNull(); + expect(screen.getByDisplayValue('charge')).toBeDisabled(); // the id field + expect(screen.getByText(/rename it in the container/i)).toBeInTheDocument(); + }); + + it('drops the Target column for a nested decision and never mirrors top-level edges', () => { + const { onPatch } = renderInspector({ + kind: NESTED_NODE_KIND, + id: encodeNestedNodeId({ containerId: 'decide', regionKey: 'body', nodeId: 'branchpoint' }), + }); + // The Branches editor shows Label + Expression but no Target column. + expect(screen.getByText('Expression')).toBeInTheDocument(); + expect(screen.queryByText('Target')).toBeNull(); + // A node edit commits only a nodes patch — no phantom top-level edges. + fireEvent.change(screen.getByDisplayValue('Branch?'), { target: { value: 'Gate' } }); + const patch = lastPatch(onPatch); + expect(patch.edges).toBeUndefined(); + expect(patch.nodes[3].config.body.nodes[0].label).toBe('Gate'); + }); + + it('shows an empty state (naming the node id) for a stale nested path', () => { + const { container } = renderInspector({ + kind: NESTED_NODE_KIND, + id: encodeNestedNodeId({ containerId: 'each', regionKey: 'body', nodeId: 'ghost' }), + label: 'Ghost', + }); + expect(container.querySelector('[aria-label="nested node location"]')).toBeNull(); + expect(screen.getByText('ghost')).toBeInTheDocument(); + }); + + it('keeps the Target column for a TOP-LEVEL decision (contrast to the nested strip)', () => { + const draft = { + nodes: [{ id: 'd', type: 'decision', label: 'D', config: { conditions: [{ label: 'Yes', expression: 'x > 1' }] } }], + edges: [], + }; + renderInspector({ kind: 'node', id: 'd' }, draft); + expect(screen.getByText('Target')).toBeInTheDocument(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx index 6e2c9a1dc..9093e54b8 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.tsx @@ -3,13 +3,20 @@ /** * FlowNodeInspector — scoped editor for the selected flow node. * - * Selection shape: { kind: 'node', id: } - * Patches: draft.nodes[i] = {...node, ...updates} + * Selection shape: { kind: 'node', id: } — a top-level node + * { kind: 'nested-node', id: } — a node inside a + * container region (loop/parallel/try_catch, #2670) + * Patches: via `locateFlowNode(...).write` — top-level nodes splice + * `draft.nodes[i]`; nested nodes rebuild the container's + * `config..nodes[i]` with explicit spreads. * - * Beyond id / label / type / description, each node type exposes a set of - * typed form fields (see `flow-node-config`) that edit scalar keys on - * `node.config`. Any remaining config keys (objects, arrays, bespoke flags) - * are surfaced in an "Advanced (JSON)" block so authors are never locked out. + * Both share the SAME schema-driven form. Beyond id / label / type / + * description, each node type exposes a set of typed form fields (see + * `flow-node-config`, or the engine-published configSchema) that edit scalar + * keys on `node.config`; remaining keys go to an "Advanced (JSON)" block so + * authors are never locked out. A nested node is edit-only this phase: its id is + * read-only, it has no delete, and (for a nested decision) the virtual Target + * column is dropped — region-internal routing is not managed here. */ import * as React from 'react'; @@ -22,7 +29,6 @@ import { InspectorSelectField, InspectorRemoveButton, InspectorEmptyState, - spliceArray, } from './_shared'; import { fieldsForNodeType, @@ -37,6 +43,8 @@ import { applyDecisionBranches, syncDecisionEdgesByOrder, withBranchTargets } fr import { useActionConfigSchemas } from '../previews/useFlowNodePalette'; import { FlowNodeConfigField } from './FlowNodeConfigField'; import { useFlowScope } from './useFlowScope'; +import { nodeOutputRefs, type ScopeRef } from './flow-scope'; +import { NESTED_NODE_KIND, parseNestedNodeId, locateFlowNode } from './flow-nested-selection'; import { ScreenPreview } from '../previews/ScreenPreview'; interface FlowNode { @@ -101,9 +109,16 @@ function setAtPath(obj: Record, path: string[], value: unknown) } export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, locale, readOnly }: MetadataInspectorProps) { - const nodes = Array.isArray((draft as any).nodes) ? ((draft as any).nodes as FlowNode[]) : []; - const index = nodes.findIndex((n) => n?.id === selection.id); - const node = index >= 0 ? nodes[index] : null; + // Resolve the selection to a node + how to write it back — a top-level draft + // node, or a node nested inside a container region (#2670). Every edit goes + // through loc.write, so the inspector never branches on where the node lives. + // Memoized on (draft, selection) so its `write` closure and identity stay + // stable between edits (keeps the nested-scope memo below from thrashing). + const loc = React.useMemo( + () => locateFlowNode(draft as Record, selection), + [draft, selection], + ); + const node = loc?.node ?? null; // Server-driven property form: when the running engine publishes a config // JSON Schema for this node type (ADR-0018 §configSchema — e.g. the ADR-0019 @@ -111,8 +126,15 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, // with the backend. Falls back to the hardcoded field group when no schema is // published (offline / plugin absent / older backend). const configSchemas = useActionConfigSchemas(); + // A nested node anchors its scope on the container (ADR-0031 outer scope). The + // container's own outputs — a loop's iteratorVariable — are excluded from the + // graph walk at its id, so inject the loop group explicitly for a body node. + const nestedLoopRefs = React.useMemo( + () => (loc?.nested && loc.container ? nodeOutputRefs(loc.container).filter((r) => r.group === 'loop') : []), + [loc], + ); // In-scope variable references for this node, for the data-picker (#1934). - const { groups: scopeGroups } = useFlowScope(draft as Record, node?.id); + const { groups: scopeGroups } = useFlowScope(draft as Record, loc?.scopeAnchorId, nestedLoopRefs); const fields = React.useMemo(() => { const schema = node?.type ? configSchemas[node.type] : undefined; const serverFields = schema !== undefined ? jsonSchemaToFlowFields(schema) : null; @@ -163,15 +185,21 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, }, [extraJson]); if (!node) { + // Stale selection (deleted node, or a deep link the draft has moved past). + // For a nested path, show the node's own id — not the encoded + // container::region::node string — as the empty-state identity. + const nestedPath = selection.kind === NESTED_NODE_KIND ? parseNestedNodeId(selection.id) : null; + const emptyId = nestedPath?.nodeId ?? selection.id; return ( - - + + ); } const patchNode = (updates: Partial) => { - onPatch({ nodes: spliceArray(nodes, index, { ...node, ...updates }) }); + const patch = loc?.write({ ...node, ...updates }); + if (patch) onPatch(patch); }; const hasExtras = extraJson.trim() !== ''; @@ -180,32 +208,37 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, const isScreen = node.type === 'screen' || node.type === 'user_task'; const setField = (field: FlowConfigField, value: unknown) => { + if (!loc) return; const path = field.path; - const draftEdges = Array.isArray((draft as { edges?: unknown }).edges) - ? ((draft as { edges: FlowEdge[] }).edges) - : []; - // Decision branches drive routing — mirror them onto the node's outgoing - // edges so the engine/simulator can actually branch (they read - // edge.condition, not node.config.conditions). The Branches editor's - // Target column (#1942) additionally wires each branch to its downstream - // node: applyDecisionBranches strips the virtual `target` cells from the - // stored conditions and creates / updates / detaches the matching edges. let stored = value; let nextEdges: FlowEdge[] | undefined; - if (isBranchTargetField(field)) { - const applied = applyDecisionBranches(node.id, value, draftEdges); - stored = applied.conditions.length ? applied.conditions : undefined; - nextEdges = applied.edges; - } else if (node.type === 'decision' && path.length === 2 && path[0] === 'config' && path[1] === 'conditions') { - // A decision branch list without a Target column (engine-published - // configSchema form) keeps the legacy by-order mirror. - nextEdges = syncDecisionEdgesByOrder(node.id, value, draftEdges); + // Decision→edge mirroring is TOP-LEVEL only. A top-level decision drives + // routing via its out-edges (the engine/simulator read edge.condition, not + // node.config.conditions), and the Branches editor's Target column (#1942) + // wires each branch to its downstream node. A NESTED decision routes within + // its region sub-graph, not on draft.edges — mirroring there would forge + // phantom top-level edges pointing at nested ids, so it is skipped entirely + // (the virtual Target column is also stripped from nested fields below). + if (!loc.nested) { + const draftEdges = Array.isArray((draft as { edges?: unknown }).edges) + ? ((draft as { edges: FlowEdge[] }).edges) + : []; + if (isBranchTargetField(field)) { + const applied = applyDecisionBranches(node.id, value, draftEdges); + stored = applied.conditions.length ? applied.conditions : undefined; + nextEdges = applied.edges; + } else if (node.type === 'decision' && path.length === 2 && path[0] === 'config' && path[1] === 'conditions') { + // A decision branch list without a Target column (engine-published + // configSchema form) keeps the legacy by-order mirror. + nextEdges = syncDecisionEdgesByOrder(node.id, value, draftEdges); + } } let nextNode = setAtPath(node, path, stored); // Migrate-on-edit: writing the canonical path drops any looser fallback // location, so the node never carries a stale duplicate (engine + designer agree). if (field.fallbackPath) nextNode = setAtPath(nextNode, field.fallbackPath, undefined); - const patch: Record = { nodes: spliceArray(nodes, index, nextNode) }; + const patch = loc.write(nextNode); + if (!patch) return; if (nextEdges) patch.edges = nextEdges; onPatch(patch); }; @@ -222,17 +255,19 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, ); const merged = { ...knownPart, ...extrasPart }; setAdvError(null); - const nextNode = { ...node }; - if (Object.keys(merged).length === 0) delete (nextNode as FlowNode).config; - else (nextNode as FlowNode).config = merged; - onPatch({ nodes: spliceArray(nodes, index, nextNode) }); + const nextNode: Record = { ...node }; + if (Object.keys(merged).length === 0) delete nextNode.config; + else nextNode.config = merged; + const patch = loc?.write(nextNode); + if (patch) onPatch(patch); } catch (e) { setAdvError(String((e as Error).message)); } }; const remove = () => { - onPatch({ nodes: spliceArray(nodes, index, null) }); + const patch = loc?.write(null); + if (patch) onPatch(patch); onClearSelection(); }; @@ -240,15 +275,31 @@ export function FlowNodeInspector({ selection, draft, onPatch, onClearSelection, ? [...FLOW_NODE_TYPE_OPTIONS] : [...FLOW_NODE_TYPE_OPTIONS, node.type ?? ''].filter(Boolean); + // A nested node has no structural editing this phase (no delete, id is + // read-only — those live on the container's Advanced JSON). + const nested = !!loc?.nested; + return ( } + footer={nested ? undefined : } > - patchNode({ id: v })} disabled={readOnly} mono /> + {nested && ( +
+ {loc?.container?.label || loc?.container?.id} + › + {loc?.regionLabel} + › + {node.label || node.id} +
+ )} + patchNode({ id: v })} disabled={readOnly || nested} mono /> + {nested && ( +

{t('engine.inspector.flowNode.nestedIdHint', locale)}

+ )} patchNode({ label: v })} disabled={readOnly} /> ( - setField(field, v)} - disabled={readOnly} - locale={locale} - context={{ draft, node }} - scopeGroups={scopeGroups} - /> - ))} + {visibleFields.map((field) => { + const branchTarget = isBranchTargetField(field); + // A NESTED node can't wire top-level edges, so drop the virtual Target + // column from its Branches editor and skip the withBranchTargets augment + // (its region routing is out of scope this phase). + const effField = + nested && branchTarget + ? { ...field, columns: (field.columns ?? []).filter((c) => c.key !== 'target') } + : field; + // The Branches editor's Target column (#1942) is virtual: derived from the + // node's out-edges, never stored on the branch rows (top-level only). + const value = + branchTarget && !nested + ? withBranchTargets( + node.id, + getFieldValue(node, field), + Array.isArray((draft as { edges?: unknown }).edges) ? ((draft as { edges: FlowEdge[] }).edges) : [], + ) + : getFieldValue(node, effField); + return ( + setField(field, v)} + disabled={readOnly} + locale={locale} + context={{ draft, node }} + scopeGroups={scopeGroups} + /> + ); + })} {isScreen && } diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.test.ts new file mode 100644 index 000000000..56779900e --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.test.ts @@ -0,0 +1,200 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + NESTED_NODE_KIND, + encodeNestedNodeId, + parseNestedNodeId, + regionConfigPathOf, + regionLabelOf, + locateFlowNode, + type NestedNodePath, +} from './flow-nested-selection'; +import { extractRegions, type FlowNode } from '../previews/flow-canvas-layout'; + +describe('flow-nested-selection — id codec', () => { + it('exports a stable, distinct selection kind', () => { + expect(NESTED_NODE_KIND).toBe('nested-node'); + }); + + it('round-trips a body / branch / try / catch path', () => { + const paths: NestedNodePath[] = [ + { containerId: 'each', regionKey: 'body', nodeId: 'charge' }, + { containerId: 'fan', regionKey: 'branch-7', nodeId: 'notify' }, + { containerId: 'guard', regionKey: 'try', nodeId: 'call' }, + { containerId: 'guard', regionKey: 'catch', nodeId: 'rollback' }, + ]; + for (const p of paths) { + expect(parseNestedNodeId(encodeNestedNodeId(p))).toEqual(p); + } + }); + + it('rejects a plain (non-nested) id and an unknown region token', () => { + expect(parseNestedNodeId('plainNode')).toBeNull(); + expect(parseNestedNodeId('c::bogus::n')).toBeNull(); // region not in the closed set + expect(parseNestedNodeId('c::body')).toBeNull(); // no node segment + expect(parseNestedNodeId('c::branch-x::n')).toBeNull(); // branch index not numeric + }); + + it('tolerates a `::`-bearing node id (the anchored middle keeps the split deterministic)', () => { + // A mis-split can only ever mis-name a segment — locateFlowNode matches each + // against the draft, so a wrong split resolves to "not found", never a write. + const parsed = parseNestedNodeId('c::body::a::b'); + expect(parsed).toEqual({ containerId: 'c', regionKey: 'body', nodeId: 'a::b' }); + }); +}); + +describe('flow-nested-selection — region config path + label', () => { + it('maps region keys to their structured config location', () => { + expect(regionConfigPathOf('body')).toEqual({ kind: 'key', key: 'body' }); + expect(regionConfigPathOf('try')).toEqual({ kind: 'key', key: 'try' }); + expect(regionConfigPathOf('catch')).toEqual({ kind: 'key', key: 'catch' }); + expect(regionConfigPathOf('branch-3')).toEqual({ kind: 'branch', index: 3 }); + expect(regionConfigPathOf('bogus')).toBeNull(); + }); + + it('labels regions, reading a parallel branch name (or its 1-based fallback) off the container', () => { + const parallel = { + id: 'p', + type: 'parallel', + config: { branches: [{ name: 'Slack', nodes: [], edges: [] }, { nodes: [], edges: [] }] }, + }; + expect(regionLabelOf('body')).toBe('Body'); + expect(regionLabelOf('try')).toBe('Try'); + expect(regionLabelOf('catch')).toBe('Catch'); + expect(regionLabelOf('branch-0', parallel)).toBe('Slack'); // authored name + expect(regionLabelOf('branch-1', parallel)).toBe('Branch 2'); // unnamed → 1-based + expect(regionLabelOf('branch-5')).toBe('Branch 6'); // no container → fallback + }); + + /** + * Anti-drift: every region key `extractRegions` can emit must be resolvable by + * both regionConfigPathOf and regionLabelOf — the codec and the canvas layout + * must never disagree on the set of region keys. + */ + it('resolves every key extractRegions emits (loop / parallel / try_catch)', () => { + const containers: FlowNode[] = [ + { id: 'each', type: 'loop', config: { body: { nodes: [{ id: 'x', type: 'http' }], edges: [] } } }, + { + id: 'fan', + type: 'parallel', + config: { + branches: [ + { name: 'A', nodes: [{ id: 'a', type: 'http' }], edges: [] }, + { nodes: [{ id: 'b', type: 'http' }], edges: [] }, + ], + }, + }, + { + id: 'guard', + type: 'try_catch', + config: { try: { nodes: [{ id: 't', type: 'http' }], edges: [] }, catch: { nodes: [{ id: 'c', type: 'http' }], edges: [] } }, + }, + ]; + for (const container of containers) { + for (const region of extractRegions(container)) { + expect(regionConfigPathOf(region.key)).not.toBeNull(); + const label = regionLabelOf(region.key, container); + // extractRegions leaves a loop body header-less; regionLabelOf still + // names it 'Body' for the breadcrumb. Every other region's label matches. + expect(label).toBe(region.label ?? 'Body'); + } + } + }); +}); + +describe('flow-nested-selection — locateFlowNode + write-back', () => { + const draft = () => ({ + nodes: [ + { id: 'start', type: 'start' }, + { + id: 'each', + type: 'loop', + label: 'For each', + config: { + collection: '{items}', + body: { + nodes: [{ id: 'charge', type: 'http_request', label: 'Charge' }], + edges: [{ source: 'charge', target: 'charge' }], + }, + }, + }, + { + id: 'fan', + type: 'parallel', + config: { + branches: [ + { name: 'Slack', nodes: [{ id: 's', type: 'http_request', label: 'Slack' }], edges: [] }, + { nodes: [{ id: 'c', type: 'http_request', label: 'CRM' }], edges: [] }, + ], + }, + }, + ], + edges: [{ source: 'start', target: 'each' }], + }); + + it('locates a top-level node and writes it back by splicing draft.nodes', () => { + const d = draft(); + const loc = locateFlowNode(d, { kind: 'node', id: 'each' })!; + expect(loc.nested).toBe(false); + expect(loc.scopeAnchorId).toBe('each'); + const patch = loc.write({ ...loc.node, label: 'Renamed' })!; + const nodes = patch.nodes as Array>; + expect(nodes[1].label).toBe('Renamed'); + expect(nodes[0].id).toBe('start'); // siblings untouched + }); + + it('locates a nested loop-body node, anchoring scope on the container', () => { + const d = draft(); + const id = encodeNestedNodeId({ containerId: 'each', regionKey: 'body', nodeId: 'charge' }); + const loc = locateFlowNode(d, { kind: NESTED_NODE_KIND, id })!; + expect(loc.nested).toBe(true); + expect(loc.scopeAnchorId).toBe('each'); // container, not the nested node + expect(loc.container?.id).toBe('each'); + expect(loc.regionLabel).toBe('Body'); + expect(loc.node.id).toBe('charge'); + }); + + it('writes a nested body node back into config.body.nodes[i], preserving region edges', () => { + const d = draft(); + const id = encodeNestedNodeId({ containerId: 'each', regionKey: 'body', nodeId: 'charge' }); + const loc = locateFlowNode(d, { kind: NESTED_NODE_KIND, id })!; + const patch = loc.write({ ...loc.node, label: 'Charge card' })!; + const nodes = patch.nodes as Array>; + const container = nodes[1] as { config: { body: { nodes: Array<{ label: string }>; edges: unknown[] }; collection: string } }; + expect(container.config.body.nodes[0].label).toBe('Charge card'); + expect(container.config.body.edges).toEqual([{ source: 'charge', target: 'charge' }]); // region edges kept + expect(container.config.collection).toBe('{items}'); // sibling config kept + // Only the nodes key is patched — a nested edit never touches top-level edges. + expect(patch.edges).toBeUndefined(); + }); + + it('writes a nested PARALLEL branch node, keeping config.branches an ARRAY (D5 trap)', () => { + const d = draft(); + const id = encodeNestedNodeId({ containerId: 'fan', regionKey: 'branch-1', nodeId: 'c' }); + const loc = locateFlowNode(d, { kind: NESTED_NODE_KIND, id })!; + expect(loc.regionLabel).toBe('Branch 2'); + const patch = loc.write({ ...loc.node, label: 'Notify CRM' })!; + const nodes = patch.nodes as Array>; + const fan = nodes[2] as { config: { branches: Array<{ name?: string; nodes: Array<{ label: string }> }> } }; + // The array must remain an array — a setAtPath walk would objectify it. + expect(Array.isArray(fan.config.branches)).toBe(true); + expect(fan.config.branches[1].nodes[0].label).toBe('Notify CRM'); + // The sibling branch (and its name) are untouched. + expect(fan.config.branches[0].name).toBe('Slack'); + expect(fan.config.branches[0].nodes[0].label).toBe('Slack'); + }); + + it('returns null for an unparseable / stale nested selection', () => { + const d = draft(); + expect(locateFlowNode(d, { kind: NESTED_NODE_KIND, id: 'not-a-path' })).toBeNull(); + expect( + locateFlowNode(d, { kind: NESTED_NODE_KIND, id: encodeNestedNodeId({ containerId: 'gone', regionKey: 'body', nodeId: 'x' }) }), + ).toBeNull(); + expect( + locateFlowNode(d, { kind: NESTED_NODE_KIND, id: encodeNestedNodeId({ containerId: 'each', regionKey: 'body', nodeId: 'ghost' }) }), + ).toBeNull(); + // Top-level miss too. + expect(locateFlowNode(d, { kind: 'node', id: 'nope' })).toBeNull(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts new file mode 100644 index 000000000..10325b81d --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-nested-selection.ts @@ -0,0 +1,236 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * flow-nested-selection — the selection contract for a node that lives INSIDE a + * structured control-flow region (ADR-0031 `loop.body` / `parallel.branches[]` / + * `try_catch.try`/`catch`) on the flow designer canvas (#2670 Phase 3). + * + * Phase 2 (#2680) made a container's regions render inline on the canvas, but a + * nested node was read-only. To route a nested node to the shared + * schema-driven inspector we need a selection that survives the flat + * `MetadataSelection { kind, id }` channel yet still names a *path* into the + * draft: `{ containerId, regionKey, nodeId }`. + * + * The id is encoded as `containerId::regionKey::nodeId`. Parsing anchors the + * middle segment to the CLOSED set of real region keys (`body` / `try` / + * `catch` / `branch-N`) so an ambiguous id (a `::`-bearing node id) still parses + * deterministically; and because the caller (`locateFlowNode`, Phase 3 C2) + * matches every segment EXACTLY against the draft, a mis-parse can only ever + * resolve to "not found → empty shell", never to a wrong write. Selection is + * never persisted (a deep link stores navigation, not flow content), so this + * codec is not a backward-compat contract. + */ + +import { spliceArray } from './_shared'; + +/** The `MetadataSelection.kind` for a node nested inside a container region. */ +export const NESTED_NODE_KIND = 'nested-node'; + +/** A node addressed by its container, region, and own id — decoded from a selection. */ +export interface NestedNodePath { + /** The structured container node (`loop` / `parallel` / `try_catch`) id. */ + containerId: string; + /** Region key within the container: `body` / `try` / `catch` / `branch-N`. */ + regionKey: string; + /** The nested node's own id, within that region's sub-graph. */ + nodeId: string; +} + +/** + * Where a region lives in its container's `config`. A *structured* path (rather + * than a flat string path) so the write-back (Phase 3 C2) can rebuild the + * container with explicit spreads — a generic `setAtPath` through a region path + * would objectify the `config.branches` array. + */ +export type RegionConfigPath = + | { kind: 'branch'; index: number } + | { kind: 'key'; key: 'body' | 'try' | 'catch' }; + +/** Anchored to the closed set of region keys `extractRegions` can emit. */ +const NESTED_ID_RE = /^(.+?)::(body|try|catch|branch-\d+)::(.+)$/; +const BRANCH_KEY_RE = /^branch-(\d+)$/; + +/** Encode a nested-node path into a flat selection id. */ +export function encodeNestedNodeId(path: NestedNodePath): string { + return `${path.containerId}::${path.regionKey}::${path.nodeId}`; +} + +/** Decode a selection id into a nested-node path, or null when it is not one. */ +export function parseNestedNodeId(id: string): NestedNodePath | null { + const m = NESTED_ID_RE.exec(id); + if (!m) return null; + return { containerId: m[1], regionKey: m[2], nodeId: m[3] }; +} + +/** + * Resolve a region key to its structured location in `container.config`, or null + * for an unrecognized key. Mirrors `extractRegions`: `body` → `config.body`, + * `try`/`catch` → `config.try`/`config.catch`, `branch-N` → `config.branches[N]`. + */ +export function regionConfigPathOf(regionKey: string): RegionConfigPath | null { + if (regionKey === 'body' || regionKey === 'try' || regionKey === 'catch') { + return { kind: 'key', key: regionKey }; + } + const m = BRANCH_KEY_RE.exec(regionKey); + if (m) return { kind: 'branch', index: Number(m[1]) }; + return null; +} + +/** + * Human label for a region — for the inspector breadcrumb. Mirrors + * `extractRegions`' header labels: `Body` / `Try` / `Catch`, and for a parallel + * branch the authored `name` or the 1-based `Branch N` fallback (read from the + * container so the label matches what the canvas header shows). + */ +export function regionLabelOf(regionKey: string, container?: { config?: unknown } | null): string { + if (regionKey === 'body') return 'Body'; + if (regionKey === 'try') return 'Try'; + if (regionKey === 'catch') return 'Catch'; + const m = BRANCH_KEY_RE.exec(regionKey); + if (m) { + const index = Number(m[1]); + const cfg = container && typeof container.config === 'object' && container.config + ? (container.config as Record) + : {}; + const branches = Array.isArray(cfg.branches) ? cfg.branches : []; + const branch = branches[index]; + const name = branch && typeof branch === 'object' ? (branch as { name?: unknown }).name : undefined; + return typeof name === 'string' && name ? name : `Branch ${index + 1}`; + } + return regionKey; +} + +// ── C2: node location + write-back ───────────────────────────────────────── + +/** A flow node, loose enough for both draft.nodes and a region sub-graph. */ +export interface FlowNodeLike { + id: string; + type?: string; + label?: string; + description?: string; + config?: Record; + [k: string]: unknown; +} + +/** + * A resolved node together with how to write it back — the single abstraction + * the inspector edits through, so it never branches on top-level vs nested. + */ +export interface NodeLocation { + /** The resolved node (a member of draft.nodes, or of a region sub-graph). */ + node: FlowNodeLike; + /** True when the node lives inside a container region (not draft.nodes). */ + nested: boolean; + /** + * The node id whose graph scope applies. A nested node runs in its container's + * OUTER scope (ADR-0031), so the anchor is the container id, not the node's. + */ + scopeAnchorId: string; + /** The enclosing container node — only when nested. */ + container?: FlowNodeLike; + /** Human region label for the inspector breadcrumb — only when nested. */ + regionLabel?: string; + /** + * Produce the draft patch that writes `next` back in place (or removes it with + * `null`). Returns null when the location is STALE — the draft changed under a + * deep link — so the caller no-ops instead of writing to the wrong node. + * `next` is a plain object (what `setAtPath` returns), not a strict node. + */ + write: (next: Record | null) => Record | null; +} + +function asNodeArray(v: unknown): FlowNodeLike[] { + return Array.isArray(v) ? (v as FlowNodeLike[]) : []; +} + +function configOf(node: FlowNodeLike): Record { + const c = node.config; + return c && typeof c === 'object' && !Array.isArray(c) ? (c as Record) : {}; +} + +/** A region object (`{ nodes, edges, name? }`) with a usable `nodes` array, or null. */ +function asRegion(v: unknown): (Record & { nodes: FlowNodeLike[] }) | null { + if (!v || typeof v !== 'object' || Array.isArray(v)) return null; + const r = v as Record; + if (!Array.isArray(r.nodes)) return null; + return r as Record & { nodes: FlowNodeLike[] }; +} + +/** Resolve a region object out of a container's config by its structured path. */ +function regionFromConfig(cfg: Record, rp: RegionConfigPath) { + if (rp.kind === 'key') return asRegion(cfg[rp.key]); + const branches = Array.isArray(cfg.branches) ? cfg.branches : []; + return asRegion(branches[rp.index]); +} + +/** + * Rebuild a container with one nested node replaced (or removed) — using + * EXPLICIT SPREADS, never a generic setAtPath through the region path. A path + * walk would objectify the `config.branches` array (turn `[…]` into `{0:…}`) and + * mis-prune; spliceArray keeps every array an array and preserves sibling keys + * (`region.edges`, `branch.name`). + */ +function writeNestedNode( + nodes: FlowNodeLike[], + containerIdx: number, + container: FlowNodeLike, + rp: RegionConfigPath, + nodeIdx: number, + next: Record | null, +): Record | null { + const cfg = configOf(container); + const region = regionFromConfig(cfg, rp); + if (!region || nodeIdx < 0 || nodeIdx >= region.nodes.length) return null; + const nextRegion = { ...region, nodes: spliceArray(region.nodes, nodeIdx, next) }; + const nextCfg: Record = + rp.kind === 'key' + ? { ...cfg, [rp.key]: nextRegion } + : { ...cfg, branches: spliceArray(Array.isArray(cfg.branches) ? (cfg.branches as unknown[]) : [], rp.index, nextRegion) }; + const nextContainer = { ...container, config: nextCfg }; + return { nodes: spliceArray(nodes, containerIdx, nextContainer) }; +} + +/** + * Resolve a selection (top-level `node` or a `NESTED_NODE_KIND` deep path) to a + * {@link NodeLocation}, or null when it cannot be found (unparseable id, missing + * container / region / node — a stale deep link). The caller renders an + * empty-state for null; a non-null location's `write` may still return null if + * the draft shifts between resolve and commit. + */ +export function locateFlowNode( + draft: Record, + selection: { kind: string; id: string }, +): NodeLocation | null { + const nodes = asNodeArray(draft.nodes); + + if (selection.kind === NESTED_NODE_KIND) { + const path = parseNestedNodeId(selection.id); + const rp = path ? regionConfigPathOf(path.regionKey) : null; + if (!path || !rp) return null; + const containerIdx = nodes.findIndex((n) => n?.id === path.containerId); + const container = containerIdx >= 0 ? nodes[containerIdx] : null; + if (!container) return null; + const region = regionFromConfig(configOf(container), rp); + const nodeIdx = region ? region.nodes.findIndex((n) => n?.id === path.nodeId) : -1; + const node = region && nodeIdx >= 0 ? region.nodes[nodeIdx] : null; + if (!node) return null; + return { + node, + nested: true, + scopeAnchorId: container.id, + container, + regionLabel: regionLabelOf(path.regionKey, container), + write: (next) => writeNestedNode(nodes, containerIdx, container, rp, nodeIdx, next), + }; + } + + const index = nodes.findIndex((n) => n?.id === selection.id); + const node = index >= 0 ? nodes[index] : null; + if (!node) return null; + return { + node, + nested: false, + scopeAnchorId: node.id, + write: (next) => ({ nodes: spliceArray(nodes, index, next) }), + }; +} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts index 1c7e97056..5084a8a5c 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts @@ -160,3 +160,30 @@ describe('wait node loose-config fallback (ADR-0044 showcase parity)', () => { expect(getFieldValue(node, eventType)).toBe('timer'); }); }); + +describe('loop / map collection is a template, not a CEL predicate', () => { + // The collection placeholders (`{leadList}` / `{items}`) are single-brace + // interpolate() templates, not bare CEL — so the field is flagged + // refMode:'template' to keep the mono expression editor + data-picker while + // suppressing the CEL brace-trap that would otherwise flag `{leadList}`. + it('flags loop.collection as an expression field in template mode', () => { + const collection = fieldsForNodeType('loop').find((f) => f.id === 'collection')!; + expect(collection.kind).toBe('expression'); + expect(collection.refMode).toBe('template'); + }); + + it('flags map.collection as an expression field in template mode', () => { + const collection = fieldsForNodeType('map').find((f) => f.id === 'collection')!; + expect(collection.kind).toBe('expression'); + expect(collection.refMode).toBe('template'); + }); + + it('leaves genuine CEL predicates (start / decision condition) in bare-expression mode', () => { + const startCond = fieldsForNodeType('start').find((f) => f.id === 'condition')!; + expect(startCond.kind).toBe('expression'); + expect(startCond.refMode).toBeUndefined(); + const decisionCond = fieldsForNodeType('decision').find((f) => f.id === 'condition')!; + expect(decisionCond.kind).toBe('expression'); + expect(decisionCond.refMode).toBeUndefined(); + }); +}); 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 4ff639923..e42f74d35 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 @@ -166,9 +166,14 @@ export interface FlowConfigField { ref?: FlowReferenceSpec; /** * Data-picker brace mode override (#1934). Defaults by kind (`expression` → - * bare CEL, `text` / `textarea` → `{var}` template). Set `'expression'` on a - * code field (e.g. a script body) so the picker inserts bare references, not - * `{var}` — `{x}` is a syntax error in a JS/TS script. + * bare CEL, `text` / `textarea` → `{var}` template). Two overrides: + * • `'expression'` on a code field (e.g. a script body) so the picker inserts + * bare references, not `{var}` — `{x}` is a syntax error in a JS/TS script. + * • `'template'` on an *expression* field (e.g. a loop/map `collection`) whose + * value is really an `interpolate()` single-brace template — it keeps the + * mono expression styling and the data-picker, but the picker inserts + * `{var}` and the CEL predicate brace-trap is suppressed (`{leadList}` is a + * legal template here, not a malformed condition). */ refMode?: 'expression' | 'template'; } @@ -352,13 +357,13 @@ const FLOW_NODE_CONFIG: Record = { }), ], loop: [ - cfg('collection', 'Collection', 'expression', { placeholder: '{leadList}', help: 'Expression resolving to the items to iterate.' }), + cfg('collection', 'Collection', 'expression', { placeholder: '{leadList}', refMode: 'template', help: 'Expression resolving to the items to iterate.' }), cfg('iteratorVariable', 'Item variable', 'text', { placeholder: 'currentItem' }), ], // Sequential multi-instance (ADR-0037 A2): a per-item subflow, one at a time; // each item may durably pause (e.g. a per-item approval). map: [ - cfg('collection', 'Collection', 'expression', { placeholder: '{items}', help: 'Expression resolving to the array to process, one item at a time.' }), + cfg('collection', 'Collection', 'expression', { placeholder: '{items}', refMode: 'template', help: 'Expression resolving to the array to process, one item at a time.' }), cfg('flowName', 'Per-item flow', 'reference', { ref: { kind: 'flow' }, placeholder: 'one_task_signoff', help: 'Subflow run for each item — it may pause (e.g. an approval).' }), cfg('iteratorVariable', 'Item variable', 'text', { placeholder: 'item' }), cfg('itemObject', 'Item object', 'reference', { ref: { kind: 'object' }, placeholder: 'showcase_task', help: 'When items are records, the object they belong to (exposes each item as the child’s record).' }), diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts index 239f4cbf2..15ec74c2a 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.test.ts @@ -264,3 +264,106 @@ describe('jsonSchemaToFlowFields', () => { expect(escalateTo.showWhen).toEqual({ field: 'escalation.enabled', equals: ['true'] }); }); }); + +describe('jsonSchemaToFlowFields — xExpression authoring-mode marker', () => { + const field = (schema: unknown, id: string) => jsonSchemaToFlowFields(schema)!.find((f) => f.id === id)!; + + it("maps xExpression:'expression' to a CEL expression field (predicate-validated, no refMode)", () => { + const f = field( + { type: 'object', properties: { condition: { type: 'string', xExpression: 'expression' } } }, + 'condition', + ); + expect(f.kind).toBe('expression'); + expect(f.refMode).toBeUndefined(); + }); + + it("maps xExpression:'template' to an expression field flagged refMode:'template'", () => { + const f = field( + { type: 'object', properties: { collection: { type: 'string', xExpression: 'template' } } }, + 'collection', + ); + // mono expression styling + data-picker, but `{var}` mode and no CEL brace-trap. + expect(f.kind).toBe('expression'); + expect(f.refMode).toBe('template'); + }); + + it("maps a multiline xExpression:'expression' to a textarea in bare-ref (script-body) mode", () => { + const f = field( + { type: 'object', properties: { script: { type: 'string', format: 'multiline', xExpression: 'expression' } } }, + 'script', + ); + expect(f.kind).toBe('textarea'); + expect(f.refMode).toBe('expression'); + }); + + it("maps a multiline xExpression:'template' to a textarea in template mode", () => { + const f = field( + { type: 'object', properties: { body: { type: 'string', format: 'multiline', xExpression: 'template' } } }, + 'body', + ); + expect(f.kind).toBe('textarea'); + expect(f.refMode).toBe('template'); + }); + + it('degrades an unknown xExpression value to plain text (no refMode)', () => { + const f = field( + { type: 'object', properties: { note: { type: 'string', xExpression: 'bogus' } } }, + 'note', + ); + expect(f.kind).toBe('text'); + expect(f.refMode).toBeUndefined(); + }); + + it('gives precedence to xRef and enum over xExpression', () => { + // enum wins → select (a marked string that is also an enum is still a picker). + const withEnum = field( + { type: 'object', properties: { mode: { type: 'string', enum: ['a', 'b'], xExpression: 'expression' } } }, + 'mode', + ); + expect(withEnum.kind).toBe('select'); + expect(withEnum.refMode).toBeUndefined(); + // xRef wins → reference. + const withRef = field( + { type: 'object', properties: { obj: { type: 'string', xRef: { kind: 'object' }, xExpression: 'expression' } } }, + 'obj', + ); + expect(withRef.kind).toBe('reference'); + expect(withRef.refMode).toBeUndefined(); + }); + + it('flattens a nested xExpression string, carrying its refMode', () => { + const f = field( + { + type: 'object', + properties: { + group: { type: 'object', properties: { where: { type: 'string', xExpression: 'template' } } }, + }, + }, + 'group.where', + ); + expect(f.kind).toBe('expression'); + expect(f.refMode).toBe('template'); + }); + + it('maps xExpression columns: expression → CEL column, template → text (columns carry no refMode)', () => { + const cols = jsonSchemaToFlowFields({ + type: 'object', + properties: { + rows: { + type: 'array', + items: { + type: 'object', + properties: { + guard: { type: 'string', xExpression: 'expression' }, + label: { type: 'string', xExpression: 'template' }, + plain: { type: 'string' }, + }, + }, + }, + }, + })!.find((f) => f.id === 'rows')!.columns!; + expect(cols.find((c) => c.key === 'guard')!.kind).toBe('expression'); + expect(cols.find((c) => c.key === 'label')!.kind).toBe('text'); + expect(cols.find((c) => c.key === 'plain')!.kind).toBe('text'); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts index 3bbfa5109..624b27ab1 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/json-schema-to-fields.ts @@ -21,7 +21,8 @@ * block, so authors are never locked out. * * Scope mirrors what `z.toJSONSchema` emits for real node configs: - * • string → text (enum → select) + * • string → text (enum → select; an `xExpression` marker + * makes it a CEL expression or `{var}` template) * • number / integer → number * • boolean → boolean * • array of string → stringList @@ -62,6 +63,16 @@ interface JsonSchemaNode { * sibling field/column value (e.g. an approver's `value` follows its `type`). */ xRef?: { kind?: string; objectSource?: string; kindFrom?: string; map?: Record }; + /** + * Authoring-semantics marker carried from the executor's Zod + * `.meta({ xExpression })` (ADR-0018), on a string property. Declares whether + * the string is a bare-CEL `'expression'` (a predicate / value expression) or + * an `interpolate()` single-brace `'template'` — so the designer renders the + * right editor (mono styling, data-picker brace mode, whether the CEL + * brace-trap applies) instead of guessing from the field name. An unknown + * value degrades to a plain text box. + */ + xExpression?: unknown; } const REFERENCE_KINDS: ReadonlySet = new Set([ @@ -156,13 +167,51 @@ function defaultString(node: JsonSchemaNode): string | undefined { return undefined; } -/** Scalar (non-object, non-array) → field kind. */ -function scalarKind(node: JsonSchemaNode): FlowConfigFieldKind | undefined { - if (Array.isArray(node.enum)) return 'select'; +/** + * Read a valid `xExpression` marker off a schema node, or undefined. The engine + * publishes this on a string property (Zod `.meta({ xExpression })`, ADR-0018) + * to declare its authoring semantics: + * • `'expression'` → bare CEL (a predicate / value expression) + * • `'template'` → an `interpolate()` single-brace `{var}` template + * Any other value degrades to undefined, so the string renders as a plain text + * box rather than a mis-moded editor. + */ +function exprModeOf(node: JsonSchemaNode): 'expression' | 'template' | undefined { + return node.xExpression === 'expression' || node.xExpression === 'template' + ? node.xExpression + : undefined; +} + +/** + * Scalar (non-object, non-array) → field kind, plus an optional data-picker + * `refMode` when the schema marks the string as an expression / template surface + * ({@link exprModeOf}). Enum still wins (→ select). String mapping: + * • xExpression 'expression' → expression (bare CEL, predicate-validated); + * multiline → textarea + refMode 'expression' + * (script-body mode — bare refs, no predicate check) + * • xExpression 'template' → expression + refMode 'template' (mono, `{var}` + * picker, no predicate check); multiline → textarea + * + refMode 'template' + * • plain string → text; multiline → textarea (unchanged) + */ +function scalarField( + node: JsonSchemaNode, +): { kind: FlowConfigFieldKind; refMode?: 'expression' | 'template' } | undefined { + if (Array.isArray(node.enum)) return { kind: 'select' }; const t = schemaType(node); - if (t === 'boolean') return 'boolean'; - if (t === 'number' || t === 'integer') return 'number'; - if (t === 'string') return node.format === 'multiline' ? 'textarea' : 'text'; + if (t === 'boolean') return { kind: 'boolean' }; + if (t === 'number' || t === 'integer') return { kind: 'number' }; + if (t === 'string') { + const multiline = node.format === 'multiline'; + const mode = exprModeOf(node); + if (mode === 'expression') { + return multiline ? { kind: 'textarea', refMode: 'expression' } : { kind: 'expression' }; + } + if (mode === 'template') { + return multiline ? { kind: 'textarea', refMode: 'template' } : { kind: 'expression', refMode: 'template' }; + } + return { kind: multiline ? 'textarea' : 'text' }; + } return undefined; } @@ -186,7 +235,11 @@ function columnsFor(item: JsonSchemaNode): FlowConfigColumn[] { } else if (t === 'boolean') { kind = 'boolean'; } else { - kind = 'text'; + // A string column marked xExpression:'expression' becomes a CEL column + // (mono, predicate-checked by flow-expr-problems). 'template' and plain + // strings stay text — a text column already treats `{var}` as a template, + // and objectList columns carry no refMode to distinguish the two. + kind = exprModeOf(prop) === 'expression' ? 'expression' : 'text'; } cols.push({ key, @@ -248,7 +301,8 @@ export function jsonSchemaToFlowFields(schema: unknown): FlowConfigField[] | nul for (const [subKey, subProp] of subProps) { const sp = subProp as JsonSchemaNode; const subRef = refOf(sp); - const kind = subRef ? 'reference' : scalarKind(sp); + const scalar = subRef ? undefined : scalarField(sp); + const kind = subRef ? 'reference' : scalar?.kind; if (!kind) continue; // deeper nesting / unsupported → Advanced block const id = `${key}.${subKey}`; const isGate = hasEnabled && subKey === 'enabled'; @@ -259,6 +313,7 @@ export function jsonSchemaToFlowFields(schema: unknown): FlowConfigField[] | nul // The gate adopts the parent group's label; siblings keep their own. ...(isGate ? { label: prop.title || humanizeKey(key), ...(prop.description ? { help: prop.description } : {}), ...(defaultString(sp) ? { defaultValue: defaultString(sp) } : {}) } : meta(sp, subKey)), ...(subRef ? { ref: subRef } : {}), + ...(scalar?.refMode ? { refMode: scalar.refMode } : {}), ...(kind === 'select' && Array.isArray(sp.enum) ? { options: enumOptions(sp.enum, sp.xEnumDeprecated) } : {}), ...(hasEnabled && !isGate ? { showWhen: { field: `${key}.enabled`, equals: ['true'] } } : {}), }; @@ -275,14 +330,15 @@ export function jsonSchemaToFlowFields(schema: unknown): FlowConfigField[] | nul fields.push({ id: key, path: ['config', key], kind: 'reference', ref, ...meta(prop, key) }); continue; } - const kind = scalarKind(prop); - if (!kind) continue; // unrepresentable → Advanced block + const scalar = scalarField(prop); + if (!scalar) continue; // unrepresentable → Advanced block fields.push({ id: key, path: ['config', key], - kind, + kind: scalar.kind, ...meta(prop, key), - ...(kind === 'select' && Array.isArray(prop.enum) ? { options: enumOptions(prop.enum, prop.xEnumDeprecated) } : {}), + ...(scalar.refMode ? { refMode: scalar.refMode } : {}), + ...(scalar.kind === 'select' && Array.isArray(prop.enum) ? { options: enumOptions(prop.enum, prop.xEnumDeprecated) } : {}), }); } diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.test.tsx new file mode 100644 index 000000000..522e1adea --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.test.tsx @@ -0,0 +1,56 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * useFlowScope — the `extraRefs` merge (#2670 Phase 3). A nested node anchors its + * scope on its container; the container's own loop `iteratorVariable` is + * excluded from the graph walk at its id, so the inspector injects it via + * `extraRefs`. Verify the injected ref lands in the "Loop item" group and that + * the global token de-dup still holds. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { renderHook, cleanup } from '@testing-library/react'; + +vi.mock('../previews/useObjectFields', () => ({ + useObjectFields: () => ({ fields: [], loading: false, error: null }), +})); + +import { useFlowScope } from './useFlowScope'; +import type { ScopeRef } from './flow-scope'; + +afterEach(cleanup); + +const draft = { + variables: [{ name: 'daysBefore', type: 'number' }], + nodes: [ + { id: 'start', type: 'start' }, + { id: 'each', type: 'loop', config: { iteratorVariable: 'contract' } }, + ], + edges: [{ source: 'start', target: 'each' }], +}; + +describe('useFlowScope — extraRefs', () => { + it('merges an injected loop ref into the "Loop item" group', () => { + const extra: ScopeRef[] = [{ token: 'contract', label: 'contract', detail: 'Loop item', group: 'loop' }]; + const { result } = renderHook(() => useFlowScope(draft, 'each', extra)); + const loop = result.current.groups.find((g) => g.id === 'loop'); + expect(loop?.label).toBe('Loop item'); + expect(loop?.refs.map((r) => r.token)).toContain('contract'); + // The declared flow variable is still there, in its own group. + expect(result.current.groups.find((g) => g.id === 'variables')?.refs.map((r) => r.token)).toContain('daysBefore'); + }); + + it('de-dups by token — an extra ref that collides with a variable appears once', () => { + const extra: ScopeRef[] = [{ token: 'daysBefore', label: 'daysBefore', group: 'loop' }]; + const { result } = renderHook(() => useFlowScope(draft, 'each', extra)); + expect(result.current.refs.filter((r) => r.token === 'daysBefore')).toHaveLength(1); + // First occurrence (the variable) wins, so no phantom 'Loop item' group. + expect(result.current.groups.find((g) => g.id === 'loop')).toBeUndefined(); + }); + + it('is a no-op when no extraRefs are passed (unchanged top-level behavior)', () => { + const { result } = renderHook(() => useFlowScope(draft, 'each')); + expect(result.current.groups.find((g) => g.id === 'loop')).toBeUndefined(); + expect(result.current.refs.map((r) => r.token)).toContain('daysBefore'); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts b/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts index 217e4ae80..186cb4d57 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/useFlowScope.ts @@ -50,16 +50,24 @@ const GROUP_LABELS: Record = { * the whole flow draft; `nodeId` the node being edited (for an edge, pass its * source node id — references available on an edge are those in scope at its * source). + * + * `extraRefs` are merged in before de-dup / grouping — used for a NESTED node, + * whose scope anchor is its container (ADR-0031 outer scope): the container's + * own outputs are excluded from the graph walk at its id, so a loop's + * `iteratorVariable` must be injected explicitly for a body node to see it. Pass + * a memoized array (a fresh one every render would thrash the memo). */ export function useFlowScope( draft: Record | undefined, nodeId: string | undefined, + extraRefs?: ReadonlyArray, ): UseFlowScopeResult { const scope = React.useMemo(() => resolveFlowScope(draft ?? {}, nodeId), [draft, nodeId]); const { fields, loading } = useObjectFields(scope.trigger?.objectName); return React.useMemo(() => { const all: ScopeRef[] = [...scope.refs]; + if (extraRefs && extraRefs.length) all.push(...extraRefs); if (scope.trigger) all.push(...triggerFieldRefs(scope.trigger, fields)); // Global de-dup by token (a declared var also written upstream shows once). const seen = new Set(); @@ -70,5 +78,5 @@ export function useFlowScope( refs: refs.filter((r) => r.group === id), })).filter((g) => g.refs.length > 0); return { groups, refs, loading: !!scope.trigger && loading, isEmpty: refs.length === 0 }; - }, [scope, fields, loading]); + }, [scope, fields, loading, extraRefs]); } diff --git a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.test.tsx b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.test.tsx index a68e6b726..005b3e6b2 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.test.tsx @@ -209,3 +209,138 @@ describe('FlowCanvas — inline nested container regions (#2670 Phase 2)', () => expect(screen.queryByRole('button', { name: /nested regions/i })).not.toBeInTheDocument(); }); }); + +describe('FlowCanvas — nested-node selection on the inline canvas (#2670 Phase 3)', () => { + const LOOP_NODES = [ + { id: 'start', type: 'start' }, + { + id: 'each', + type: 'loop', + label: 'For each order', + config: { body: { nodes: [{ id: 'charge', type: 'http', label: 'Charge card' }], edges: [] } }, + }, + { id: 'after', type: 'end', label: 'After' }, + ]; + const LOOP_EDGES = [ + { source: 'start', target: 'each' }, + { source: 'each', target: 'after' }, + ]; + + it('emits a structured nested path (not the container) when a body node is clicked', () => { + const onSelect = vi.fn(); + const onSelectNested = vi.fn(); + render( + , + ); + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + onSelect.mockClear(); // the expand toggle itself must not have selected anything + fireEvent.click(screen.getByText('Charge card')); + // The nested node routes to onSelectNested with its full path + node… + expect(onSelectNested).toHaveBeenCalledTimes(1); + const [path, node] = onSelectNested.mock.calls[0]; + expect(path).toEqual({ containerId: 'each', regionKey: 'body', nodeId: 'charge' }); + expect(node.id).toBe('charge'); + // …and the click never bubbles up to select the container node. + expect(onSelect).not.toHaveBeenCalled(); + }); + + it('paints the selection ring (aria-pressed) on the matching nested node', () => { + const { container } = render( + {}} + onSelectNested={() => {}} + selectedNestedPath={{ containerId: 'each', regionKey: 'body', nodeId: 'charge' }} + />, + ); + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + const nested = container.querySelector('[data-region-node-id="charge"]') as HTMLElement; + expect(nested).not.toBeNull(); + expect(nested.getAttribute('aria-pressed')).toBe('true'); + }); + + it('clears the nested selection when its container is collapsed', () => { + const onSelectNested = vi.fn(); + render( + {}} + onSelectNested={onSelectNested} + selectedNestedPath={{ containerId: 'each', regionKey: 'body', nodeId: 'charge' }} + />, + ); + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + fireEvent.click(screen.getByRole('button', { name: 'Collapse nested regions' })); + expect(onSelectNested).toHaveBeenCalledWith(null); + }); + + it('tags a parallel branch node with its indexed region key', () => { + const onSelectNested = vi.fn(); + render( + {}} + onSelectNested={onSelectNested} + />, + ); + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + fireEvent.click(screen.getByText('Notify CRM')); + expect(onSelectNested).toHaveBeenCalledTimes(1); + const [path] = onSelectNested.mock.calls[0]; + expect(path).toEqual({ containerId: 'fan', regionKey: 'branch-1', nodeId: 'b' }); + }); + + it('keeps the tray read-only (no nested selection) outside design mode', () => { + const onSelectNested = vi.fn(); + const { container } = render( + {}} + onSelectNested={onSelectNested} + />, + ); + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + const nested = container.querySelector('[data-region-node-id="charge"]') as HTMLElement; + // The hook is always present, but read-only → not a button, no click routing. + expect(nested).not.toBeNull(); + expect(nested.getAttribute('role')).toBeNull(); + fireEvent.click(screen.getByText('Charge card')); + expect(onSelectNested).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx index 6011f514c..5dcbac6af 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx @@ -50,6 +50,7 @@ import { predictExpandedNodeHeight } from './flow-region-metrics'; import { NodeCard, NodePalette, defaultNodeLabel, defaultNodeExtras } from './flow-canvas-parts'; import { useFlowNodePalette } from './useFlowNodePalette'; import { indexProblemBadges, edgeProblemKey, type FlowProblem } from './flow-problems'; +import type { NestedNodePath } from '../inspectors/flow-nested-selection'; const MIN_ZOOM = 0.4; const MAX_ZOOM = 1.6; @@ -110,6 +111,18 @@ export interface FlowCanvasProps { onSelect: (node: FlowNode | null) => void; /** Select an edge (its `edgeKey`), or clear selection with `null`. */ onSelectEdge?: (edge: FlowEdge | null, key: string) => void; + /** + * #2670 Phase 3: the selected NESTED node (inside an expanded container's + * region), or null. Drives the selection ring on the matching container's + * tray node. + */ + selectedNestedPath?: NestedNodePath | null; + /** + * #2670 Phase 3: select a nested node (pass its full path + the node), or + * clear the nested selection with `null`. Absent → the region trays stay + * read-only (Phase 2 behavior). + */ + onSelectNested?: (path: NestedNodePath | null, node?: FlowNode) => void; onPatch?: (partial: Record) => void; } @@ -131,6 +144,8 @@ export function FlowCanvas({ revealSignal, onSelect, onSelectEdge, + selectedNestedPath, + onSelectNested, onPatch, }: FlowCanvasProps) { const viewportRef = React.useRef(null); @@ -155,14 +170,23 @@ export function FlowCanvas({ // the geometry-aware layout (expanded container = predicted card height; // everything else = NODE_H, keeping the historical layout byte-identical). const [expandedIds, setExpandedIds] = React.useState>(() => new Set()); - const toggleExpanded = React.useCallback((id: string) => { - setExpandedIds((prev) => { - const next = new Set(prev); - if (next.has(id)) next.delete(id); - else next.add(id); - return next; - }); - }, []); + const toggleExpanded = React.useCallback( + (id: string) => { + // Read the current expand state OUTSIDE the updater so we can clear a + // now-hidden nested selection on collapse (D6) — a plain effect keyed on + // expandedIds would also fire when the canvas remounts (expandedIds resets + // to empty) and wrongly clear a still-valid selection. + const collapsing = expandedIds.has(id); + setExpandedIds((prev) => { + const next = new Set(prev); + if (next.has(id)) next.delete(id); + else next.add(id); + return next; + }); + if (collapsing && selectedNestedPath?.containerId === id) onSelectNested?.(null); + }, + [expandedIds, selectedNestedPath, onSelectNested], + ); const regionsByNode = React.useMemo(() => { const map = new Map(); for (const n of nodes) { @@ -866,6 +890,17 @@ export function FlowCanvas({ expanded={expandedIds.has(node.id)} onToggleExpand={regionsByNode.has(node.id) ? () => toggleExpanded(node.id) : undefined} height={heights.get(node.id)} + selectedNestedNode={ + selectedNestedPath?.containerId === node.id + ? { regionKey: selectedNestedPath.regionKey, nodeId: selectedNestedPath.nodeId } + : null + } + onSelectNestedNode={ + designMode && onSelectNested + ? (regionKey, nested) => + onSelectNested({ containerId: node.id, regionKey, nodeId: nested.id }, nested) + : undefined + } /> ); })} diff --git a/packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx b/packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx index b58d2ae97..b363e39a4 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx @@ -37,6 +37,7 @@ import { uniqueId, appendArray } from '../inspectors/_shared'; import { t as tr } from '../i18n'; import { FlowCanvas } from './FlowCanvas'; import { edgeKey } from './flow-canvas-layout'; +import { NESTED_NODE_KIND, parseNestedNodeId, encodeNestedNodeId } from '../inspectors/flow-nested-selection'; import { FlowSimulatorPanel } from './FlowSimulatorPanel'; import { FlowRunsPanel } from './FlowRunsPanel'; import { ProblemsPanel } from './ProblemsPanel'; @@ -82,6 +83,13 @@ export function FlowPreview({ draft, editing, selection, onSelectionChange, onPa const canEdit = designMode && !!onPatch; const selectedId = selection && selection.kind === 'node' ? selection.id : null; const selectedEdgeId = selection && selection.kind === 'edge' ? selection.id : null; + // #2670 Phase 3: a nested-node selection carries an encoded container path in + // the flat selection id. Decode it HERE — FlowPreview is the only place that + // speaks the codec; the canvas only ever handles the structured path. + const selectedNestedPath = React.useMemo( + () => (selection && selection.kind === NESTED_NODE_KIND ? parseNestedNodeId(selection.id) : null), + [selection], + ); const [showDebug, setShowDebug] = React.useState(false); // Variables panel is opt-in: opening a flow should show the full-width canvas, @@ -293,6 +301,16 @@ export function FlowPreview({ draft, editing, selection, onSelectionChange, onPa ? onSelectionChange?.({ kind: 'edge', id: key, label: `${e.source} → ${e.target}` }) : onSelectionChange?.(null) } + selectedNestedPath={selectedNestedPath} + onSelectNested={(path, node) => + path + ? onSelectionChange?.({ + kind: NESTED_NODE_KIND, + id: encodeNestedNodeId(path), + label: node?.label || path.nodeId, + }) + : onSelectionChange?.(null) + } onPatch={onPatch} /> diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-parts.tsx b/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-parts.tsx index e5bc3732f..cb26dedbe 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-parts.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-parts.tsx @@ -49,7 +49,7 @@ import { CommandList, } from '@object-ui/components'; import { t as tr } from '../i18n'; -import { NODE_W, NODE_H, type Point, type LabeledRegion } from './flow-canvas-layout'; +import { NODE_W, NODE_H, type Point, type LabeledRegion, type FlowNode } from './flow-canvas-layout'; import { FlowRegionView } from './flow-region-view'; import { EXPANDED_REGION_MAX_W, NODE_REGION_GAP, REGION_PANEL_PAD } from './flow-region-metrics'; import { useFlowPaletteRecents } from '../../../context/FlowPaletteRecentsProvider'; @@ -427,6 +427,17 @@ export interface NodeCardProps { expanded?: boolean; /** #2670: toggle the region tray. Absent → no expand affordance. */ onToggleExpand?: () => void; + /** + * #2670 Phase 3: the selected nested node within THIS container's tray, + * scoped to its region key — drives the selection ring. `null` when nothing + * (or a node in another container) is selected. + */ + selectedNestedNode?: { regionKey: string; nodeId: string } | null; + /** + * #2670 Phase 3: select a nested node in the tray, tagged with its region + * key. Absent → the tray stays read-only (Phase 2 behavior). + */ + onSelectNestedNode?: (regionKey: string, node: FlowNode) => void; /** * #2670: the card's rendered height from the layout geometry — the SAME * number that positioned every card below this one, so the DOM can never @@ -460,6 +471,8 @@ export function NodeCard({ expanded, onToggleExpand, height, + selectedNestedNode, + onSelectNestedNode, }: NodeCardProps) { const tone = nodeTone(type); const hasRegions = !!regions && regions.length > 0; @@ -563,7 +576,12 @@ export function NodeCard({ onPointerDown={(e) => e.stopPropagation()} onClick={(e) => e.stopPropagation()} > - + )} {badge && ( 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 0d4740024..b2875433f 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 @@ -80,4 +80,38 @@ describe('flowExpressionProblems', () => { }; expect(flowExpressionProblems(draft)).toEqual([]); }); + + it('does NOT flag a loop collection `{leadList}` — it is a template surface, not a CEL predicate', () => { + // The collection field is refMode:'template', so its single-brace `{var}` + // template is legal and must not trip the CEL brace-trap (the pre-fix bug). + const draft = { + variables: [{ name: 'leadList' }], + nodes: [ + startUpdate, + { id: 'each', type: 'loop', config: { collection: '{leadList}', iteratorVariable: 'lead' } }, + ], + edges: [{ source: 'start', target: 'each' }], + }; + expect(flowExpressionProblems(draft)).toEqual([]); + }); + + it('still flags a genuine CEL predicate on the same flow (decision condition)', () => { + // Guards against over-broadening the template skip: real predicate fields + // keep their brace-trap. + const draft = { + variables: [], + nodes: [ + startUpdate, + { id: 'each', type: 'loop', config: { collection: '{leadList}' } }, + { id: 'd', type: 'decision', config: { condition: '{record.amount} > 10' } }, + ], + edges: [ + { source: 'start', target: 'each' }, + { source: 'each', target: 'd' }, + ], + }; + const ps = flowExpressionProblems(draft); + expect(ps).toHaveLength(1); + expect(ps[0]).toMatchObject({ level: 'error', target: { kind: 'node', nodeId: 'd' } }); + }); }); 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 cbee35aa6..de1bb911a 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 @@ -15,7 +15,9 @@ * inline inspector check, which does fetch, still covers it. * * Only CEL (`expression`) surfaces are scanned — template (`{var}`) values use - * single braces legally and are left to the inline check. + * 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. */ import { fieldsForNodeType, getFieldValue } from '../inspectors/flow-node-config'; @@ -71,7 +73,7 @@ export function flowExpressionProblems(draft: Record): ExprProb const roots = nodeId === startId ? null : scopeRoots(resolveFlowScope(draft, nodeId).refs); for (const field of fieldsForNodeType(type)) { - if (field.kind === 'expression') { + if (field.kind === 'expression' && field.refMode !== 'template') { const hit = checkCel(getFieldValue(node, field), roots); if (hit) out.push({ target: { kind: 'node', nodeId }, level: hit.level, message: hit.message }); } else if (field.kind === 'objectList' && field.columns) { diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-region-view.tsx b/packages/app-shell/src/views/metadata-admin/previews/flow-region-view.tsx index c6ba9d205..908c9c448 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/flow-region-view.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-region-view.tsx @@ -33,12 +33,58 @@ import { import { NodeTypeIcon, nodeTone } from './flow-canvas-parts'; import { REGION_BLOCK_PAD, REGION_GAP, REGION_LABEL_H } from './flow-region-metrics'; -/** Read-only node box inside a region — a compact echo of `NodeCard`. */ -function RegionNode({ node, x, y }: { node: FlowNode; x: number; y: number }) { +/** + * Node box inside a region — a compact echo of `NodeCard`. Read-only by default + * (identical to Phase 1/2); when `onSelect` is supplied it becomes a selectable + * control (button role, keyboard, selection ring) so a nested node can be routed + * to the shared inspector (#2670 Phase 3). The `data-region-node-id` hook is + * always emitted for test / deep-link targeting. + */ +function RegionNode({ + node, + x, + y, + selected, + onSelect, +}: { + node: FlowNode; + x: number; + y: number; + selected?: boolean; + onSelect?: () => void; +}) { const tone = nodeTone(node.type); + const interactive = !!onSelect; return (
{ + e.stopPropagation(); + onSelect(); + } + : undefined + } + onKeyDown={ + interactive + ? (e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + e.stopPropagation(); + onSelect(); + } + } + : undefined + } + className={cn( + 'absolute flex items-center gap-2 rounded-lg border bg-card px-2 py-1.5 shadow-sm', + interactive && 'cursor-pointer', + selected ? 'border-primary ring-2 ring-primary/30' : 'border-border/70', + )} style={{ left: x, top: y, width: NODE_W, height: NODE_H }} >
@@ -55,7 +101,17 @@ function RegionNode({ node, x, y }: { node: FlowNode; x: number; y: number }) { } /** One region laid out with the shared engine, then scaled to fit `maxWidth`. */ -function RegionCanvas({ region, maxWidth }: { region: LabeledRegion; maxWidth: number }) { +function RegionCanvas({ + region, + maxWidth, + selectedNodeId, + onSelectNode, +}: { + region: LabeledRegion; + maxWidth: number; + selectedNodeId?: string | null; + onSelectNode?: (node: FlowNode) => void; +}) { const layout = React.useMemo(() => computeLayout(region.nodes, region.edges), [region.nodes, region.edges]); const { width, height } = React.useMemo(() => diagramSize(layout), [layout]); const scale = Math.min(1, maxWidth / Math.max(width, 1)); @@ -86,7 +142,16 @@ function RegionCanvas({ region, maxWidth }: { region: LabeledRegion; maxWidth: n {region.nodes.map((n) => { const p = layout.get(n.id); - return p ? : null; + return p ? ( + onSelectNode(n) : undefined} + /> + ) : null; })}
@@ -97,7 +162,19 @@ function RegionCanvas({ region, maxWidth }: { region: LabeledRegion; maxWidth: n * Render a container's nested regions read-only, each fit to `maxWidth`, with a * header per region (`Branch N` / `Try` / `Catch`; a loop body has no header). */ -export function FlowRegionView({ regions, maxWidth }: { regions: LabeledRegion[]; maxWidth: number }) { +export function FlowRegionView({ + regions, + maxWidth, + selected, + onSelectNode, +}: { + regions: LabeledRegion[]; + maxWidth: number; + /** The selected nested node, scoped to its region — highlighted with a ring. */ + selected?: { regionKey: string; nodeId: string } | null; + /** Selecting a nested node, tagged with its region key (for the container path). */ + onSelectNode?: (regionKey: string, node: FlowNode) => void; +}) { // #2670: every dimension in this height stack is an explicit px from // flow-region-metrics so the layout height PREDICTOR matches the DOM exactly // (rem-based Tailwind spacing and font-metric line-heights would drift). @@ -117,7 +194,12 @@ export function FlowRegionView({ regions, maxWidth }: { regions: LabeledRegion[] {region.label} )} - + onSelectNode(region.key, node) : undefined} + /> ))}