From b8c113290808fed4dfa6267c642f8532e1582269 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Jul 2026 11:38:19 +0000 Subject: [PATCH 1/3] feat(flow-designer): geometry-aware layered layout (computeLayoutWithGeometry) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2670 Phase 2 groundwork. Cards are about to become variable-height (an expanded loop/parallel/try_catch container grows to embed its regions), so the layered layout's fixed `l * (NODE_H + V_GAP)` pitch becomes CUMULATIVE: each layer starts below the tallest auto-laid card of the previous occupied layer (empty layer indices keep the historical pitch so degenerate cyclic graphs are unchanged too). - New `computeLayoutWithGeometry(nodes, edges, heightOf?)` returning { positions, heights, size }; `computeLayout` is now a thin constant-height wrapper — its output is IDENTICAL to before, locked by invariance tests over five graph shapes (linear / diamond / back-edge / cycle island / manual ui). - Manual-`ui` nodes are excluded from a row's height (they don't render in their computed slot) — a pinned node at/below an expanded container can overlap it; accepted + documented. - `bottomAnchor(p, height = NODE_H)` so an expanded container's outgoing edge can leave from its true bottom; default keeps every existing call site and the symbolic anchor tests untouched. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VuvxWgoadqryqBcjs7TpVi --- .../previews/flow-canvas-layout.test.ts | 134 ++++++++++++++++++ .../previews/flow-canvas-layout.ts | 79 +++++++++-- 2 files changed, 203 insertions(+), 10 deletions(-) diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.test.ts b/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.test.ts index 4a6780f72..1a7ace51e 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.test.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.test.ts @@ -3,6 +3,8 @@ import { describe, it, expect } from 'vitest'; import { computeLayout, + computeLayoutWithGeometry, + diagramSize, isBackEdge, rightAnchor, backEdgePath, @@ -11,6 +13,8 @@ import { extractRegions, NODE_W, NODE_H, + V_GAP, + PADDING, type FlowNode, type FlowEdge, } from './flow-canvas-layout'; @@ -131,3 +135,133 @@ describe('extractRegions (#2670 structured containers)', () => { expect(extractRegions({ id: 't', type: 'try_catch', config: { try: region('risky') } }).map((r) => r.label)).toEqual(['Try']); }); }); + +// ── #2670 Phase 2: geometry-aware layered layout ───────────────────────────── + +/** heightOf making exactly one node tall — the expanded-container shape. */ +const tallOnly = (id: string, h: number) => (n: FlowNode) => (n.id === id ? h : NODE_H); + +describe('computeLayoutWithGeometry — constant-height invariance (the regression lock)', () => { + const GRAPHS: { name: string; nodes: FlowNode[]; edges: FlowEdge[] }[] = [ + { + name: 'linear chain', + nodes: [{ id: 's', type: 'start' }, { id: 'a', type: 'script' }, { id: 'e', type: 'end' }], + edges: [{ source: 's', target: 'a' }, { source: 'a', target: 'e' }], + }, + { + name: 'diamond branch + join', + nodes: [ + { id: 's', type: 'start' }, + { id: 'l', type: 'script' }, + { id: 'r', type: 'script' }, + { id: 'j', type: 'end' }, + ], + edges: [ + { source: 's', target: 'l' }, + { source: 's', target: 'r' }, + { source: 'l', target: 'j' }, + { source: 'r', target: 'j' }, + ], + }, + { + name: 'declared back-edge (ADR-0044)', + nodes: [{ id: 's', type: 'start' }, { id: 'a', type: 'approval' }, { id: 'w', type: 'wait' }], + edges: [ + { source: 's', target: 'a' }, + { source: 'a', target: 'w', label: 'revise' }, + { source: 'w', target: 'a', label: 'resubmit', type: 'back' }, + ], + }, + { + name: 'unreached cycle island (trailing layers)', + nodes: [ + { id: 's', type: 'start' }, + { id: 'a', type: 'script' }, + { id: 'c1', type: 'script' }, + { id: 'c2', type: 'script' }, + ], + edges: [ + { source: 's', target: 'a' }, + { source: 'c1', target: 'c2' }, + { source: 'c2', target: 'c1' }, + ], + }, + { + name: 'manual ui position', + nodes: [ + { id: 's', type: 'start' }, + { id: 'pin', type: 'script', ui: { x: 400, y: 10 } }, + { id: 'e', type: 'end' }, + ], + edges: [{ source: 's', target: 'pin' }, { source: 'pin', target: 'e' }], + }, + ]; + + for (const g of GRAPHS) { + it(`matches computeLayout + diagramSize on: ${g.name}`, () => { + const geo = computeLayoutWithGeometry(g.nodes, g.edges); + expect(geo.positions).toEqual(computeLayout(g.nodes, g.edges)); + expect(geo.size).toEqual(diagramSize(geo.positions)); + for (const n of g.nodes) expect(geo.heights.get(n.id)).toBe(NODE_H); + }); + } +}); + +describe('computeLayoutWithGeometry — cumulative variable-height offsets (#2670)', () => { + const chain: { nodes: FlowNode[]; edges: FlowEdge[] } = { + nodes: [{ id: 's', type: 'start' }, { id: 'c', type: 'loop' }, { id: 'b', type: 'end' }], + edges: [{ source: 's', target: 'c' }, { source: 'c', target: 'b' }], + }; + + it('pushes the layer below a tall card down by its full height', () => { + const geo = computeLayoutWithGeometry(chain.nodes, chain.edges, tallOnly('c', 200)); + const s = geo.positions.get('s')!; + const c = geo.positions.get('c')!; + const b = geo.positions.get('b')!; + expect(c.y - s.y).toBe(NODE_H + V_GAP); // layer above the tall card: unchanged pitch + expect(b.y - c.y).toBe(200 + V_GAP); // layer below: pushed by the tall card + }); + + it('same-layer siblings share y; the row is as tall as its tallest card', () => { + const nodes: FlowNode[] = [ + { id: 's', type: 'start' }, + { id: 'l', type: 'loop' }, + { id: 'r', type: 'script' }, + { id: 'j', type: 'end' }, + ]; + const edges: FlowEdge[] = [ + { source: 's', target: 'l' }, + { source: 's', target: 'r' }, + { source: 'l', target: 'j' }, + { source: 'r', target: 'j' }, + ]; + const geo = computeLayoutWithGeometry(nodes, edges, tallOnly('l', 200)); + expect(geo.positions.get('l')!.y).toBe(geo.positions.get('r')!.y); + expect(geo.positions.get('j')!.y - geo.positions.get('l')!.y).toBe(200 + V_GAP); + }); + + it('size.height accounts for a tall bottom card', () => { + const geo = computeLayoutWithGeometry(chain.nodes, chain.edges, tallOnly('b', 200)); + expect(geo.size.height).toBe(geo.positions.get('b')!.y + 200 + PADDING); + }); + + it('a manually-pinned tall node does not push auto rows (accepted-overlap rule)', () => { + const nodes: FlowNode[] = [ + { id: 's', type: 'start' }, + { id: 'pin', type: 'loop', ui: { x: 400, y: 10 } }, + { id: 'e', type: 'end' }, + ]; + const edges: FlowEdge[] = [{ source: 's', target: 'pin' }, { source: 'pin', target: 'e' }]; + const tall = computeLayoutWithGeometry(nodes, edges, tallOnly('pin', 300)); + const constant = computeLayoutWithGeometry(nodes, edges); + expect(tall.positions.get('s')).toEqual(constant.positions.get('s')); + expect(tall.positions.get('e')).toEqual(constant.positions.get('e')); + expect(tall.heights.get('pin')).toBe(300); // still reported for rendering/size + }); +}); + +describe('bottomAnchor with explicit height (#2670)', () => { + it('anchors at the true bottom of a tall card', () => { + expect(bottomAnchor({ x: 10, y: 20 }, 200)).toEqual({ x: 10 + NODE_W / 2, y: 220 }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.ts b/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.ts index 3bba852bc..1d920d216 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-canvas-layout.ts @@ -120,6 +120,15 @@ export function extractRegions(node: FlowNode): LabeledRegion[] { } } +/** Full geometry of a laid-out flow: positions, per-node heights, canvas size. */ +export interface FlowLayoutGeometry { + positions: Map; + /** Rendered height of every node (`heightOf(node)`; {@link NODE_H} default). */ + heights: Map; + /** Bounding box of the diagram incl. node extents + {@link PADDING}. */ + size: { width: number; height: number }; +} + /** * Compute a deterministic layered (top-to-bottom) layout. * @@ -132,10 +141,25 @@ export function extractRegions(node: FlowNode): LabeledRegion[] { * - Within a layer, nodes keep their original `nodes[]` order (stable). * - A node with a persisted `ui` position overrides its computed slot, but is * still included in the returned map so callers can size the canvas. + * + * #2670: cards are no longer necessarily {@link NODE_H} tall — an expanded + * `loop`/`parallel`/`try_catch` container grows to embed its region(s), so + * layer spacing is **cumulative**: each layer starts below the tallest + * (auto-laid) card of the previous one. With the default constant `heightOf` + * the output is IDENTICAL to the historical fixed-pitch layout (pinned by + * tests). Manual-`ui` nodes are excluded from a row's height (they don't render + * in their computed slot; counting them would open phantom gaps) — so a pinned + * node sitting at/below an expanded container can overlap it. Accepted + * limitation: the author can drag it clear. */ -export function computeLayout(nodes: FlowNode[], edges: FlowEdge[]): Map { +export function computeLayoutWithGeometry( + nodes: FlowNode[], + edges: FlowEdge[], + heightOf: (node: FlowNode) => number = () => NODE_H, +): FlowLayoutGeometry { const positions = new Map(); - if (nodes.length === 0) return positions; + const heights = new Map(nodes.map((n) => [n.id, heightOf(n)])); + if (nodes.length === 0) return { positions, heights, size: { width: PADDING, height: PADDING } }; const byId = new Map(nodes.map((n) => [n.id, n])); const indexOf = new Map(nodes.map((n, i) => [n.id, i])); @@ -204,17 +228,33 @@ export function computeLayout(nodes: FlowNode[], edges: FlowEdge[]): Map (indexOf.get(a) ?? 0) - (indexOf.get(b) ?? 0)); } + // Place layers top-to-bottom with CUMULATIVE y: each layer starts below the + // tallest auto-laid card of the previous occupied layer. Empty layer indices + // (possible only in degenerate cyclic graphs) keep their historical + // fixed-pitch gap so the constant-height output matches the old + // `l * (NODE_H + V_GAP)` formula exactly. const sortedLayers = [...byLayer.keys()].sort((a, b) => a - b); + let y = 0; + let prevLayer: number | undefined; for (const l of sortedLayers) { + if (prevLayer !== undefined) { + y += (l - prevLayer - 1) * (NODE_H + V_GAP); + } const ids = byLayer.get(l)!; const rowWidth = ids.length * NODE_W + (ids.length - 1) * H_GAP; const startX = -rowWidth / 2; + let rowMaxHeight = NODE_H; ids.forEach((id, i) => { - positions.set(id, { - x: startX + i * (NODE_W + H_GAP), - y: l * (NODE_H + V_GAP), - }); + positions.set(id, { x: startX + i * (NODE_W + H_GAP), y }); + const n = byId.get(id); + // Manual-`ui` nodes don't render in this slot — exclude from the row + // height so they can't open phantom gaps (see JSDoc caveat). + if (n && !hasManualPosition(n)) { + rowMaxHeight = Math.max(rowMaxHeight, heights.get(id) ?? NODE_H); + } }); + y += rowMaxHeight + V_GAP; + prevLayer = l; } // Normalize the auto-computed slots so the diagram starts at @@ -240,7 +280,22 @@ export function computeLayout(nodes: FlowNode[], edges: FlowEdge[]): Map { + return computeLayoutWithGeometry(nodes, edges).positions; } /** Bounding box of the laid-out diagram, including node extents + padding. */ @@ -254,9 +309,13 @@ export function diagramSize(positions: Map): { width: number; hei return { width: maxX + PADDING, height: maxY + PADDING }; } -/** Bottom-center anchor of a node — where its outgoing edges originate. */ -export function bottomAnchor(p: Point): Point { - return { x: p.x + NODE_W / 2, y: p.y + NODE_H }; +/** + * Bottom-center anchor of a node — where its outgoing edges originate. + * #2670: pass the node's rendered height for an expanded container so the edge + * leaves from its true bottom; defaults to {@link NODE_H} (plain cards). + */ +export function bottomAnchor(p: Point, height: number = NODE_H): Point { + return { x: p.x + NODE_W / 2, y: p.y + height }; } /** Top-center anchor of a node — where its incoming edges terminate. */ From f8260e1a138799927de7527b9d10812a1734a72c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Jul 2026 11:39:59 +0000 Subject: [PATCH 2/3] feat(flow-designer): deterministic region metrics + px-pinned region renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2670 Phase 2. An expanded container's height must be known BEFORE render (the layout pushes lower layers down with no measure-then-reflow pass), so predictor and renderer must agree to the pixel: - New pure `flow-region-metrics.ts` — the shared box-model constants (block border/padding, pinned 16px label row, region gap, tray chrome, EXPANDED_REGION_MAX_W) plus `predictRegionsHeight` / `predictExpandedNodeHeight`, mirroring the renderer with the same float expressions (no rounding skew). Imports only from flow-canvas-layout — dependency stays one-directional. - `flow-region-view.tsx` — every height-stack dimension becomes an explicit px style sourced from the metrics module (rem-based `gap-2`/`p-1.5` and the font-metric label line-height would drift): visual no-op at the 16px root font, but now exactly predictable. - Unit tests pin the formula (empty → NODE_H; single unlabeled body ≈187.4px; per-branch label row + gap; never-upscale; wide-region downscale). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VuvxWgoadqryqBcjs7TpVi --- .../previews/flow-region-metrics.test.ts | 66 +++++++++++++++ .../previews/flow-region-metrics.ts | 81 +++++++++++++++++++ .../previews/flow-region-view.tsx | 17 +++- 3 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.test.ts create mode 100644 packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.ts diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.test.ts b/packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.test.ts new file mode 100644 index 000000000..b07430b12 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.test.ts @@ -0,0 +1,66 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + predictRegionsHeight, + predictExpandedNodeHeight, + REGION_BLOCK_BORDER, + REGION_BLOCK_PAD, + REGION_LABEL_H, + REGION_GAP, + NODE_REGION_GAP, + REGION_PANEL_BORDER, + REGION_PANEL_PAD, + EXPANDED_REGION_MAX_W, +} from './flow-region-metrics'; +import { NODE_W, NODE_H, H_GAP, PADDING, type LabeledRegion } from './flow-canvas-layout'; + +/** A 1-node region: mini-canvas is (NODE_W + 2·PADDING) × (NODE_H + 2·PADDING). */ +const oneNode = (label?: string): LabeledRegion => ({ + key: label ?? 'body', + label, + nodes: [{ id: 'x', type: 'http' }], + edges: [], +}); + +const INNER_W = NODE_W + 2 * PADDING; // 1-node region canvas width (296) +const INNER_H = NODE_H + 2 * PADDING; // 1-node region canvas height (122) +const BLOCK_CHROME = 2 * (REGION_BLOCK_BORDER + REGION_BLOCK_PAD); +const PANEL_CHROME = NODE_REGION_GAP + 2 * (REGION_PANEL_BORDER + REGION_PANEL_PAD); + +describe('predictExpandedNodeHeight (#2670 — the layout/renderer contract)', () => { + it('is NODE_H for a node with no regions (plain / collapsed card)', () => { + expect(predictExpandedNodeHeight([])).toBe(NODE_H); + }); + + it('predicts a single unlabeled loop body exactly (scaled to fit width)', () => { + const scaledH = INNER_H * (EXPANDED_REGION_MAX_W / INNER_W); // downscaled, no label row + const expected = NODE_H + PANEL_CHROME + BLOCK_CHROME + scaledH; + expect(predictExpandedNodeHeight([oneNode()])).toBeCloseTo(expected, 6); + expect(expected).toBeCloseTo(187.38, 1); // human-scale sanity: ≈187px card + expect(expected).toBeGreaterThan(NODE_H); + }); + + it('adds a label row and a gap per extra labeled region (two branches)', () => { + const single = predictRegionsHeight([oneNode()], EXPANDED_REGION_MAX_W); + const two = predictRegionsHeight([oneNode('Branch 1'), oneNode('Branch 2')], EXPANDED_REGION_MAX_W); + expect(two).toBeCloseTo(2 * (single + REGION_LABEL_H) + REGION_GAP, 6); + }); +}); + +describe('predictRegionsHeight — scaling', () => { + it('never upscales: at a huge maxWidth the region renders 1:1', () => { + expect(predictRegionsHeight([oneNode()], 10_000)).toBeCloseTo(BLOCK_CHROME + INNER_H, 6); + }); + + it('downscales a wide region (two side-by-side roots) to fit', () => { + const wide: LabeledRegion = { + key: 'body', + nodes: [{ id: 'a', type: 'http' }, { id: 'b', type: 'http' }], + edges: [], + }; + const wideW = 2 * NODE_W + H_GAP + 2 * PADDING; // both roots share one layer + const expected = BLOCK_CHROME + INNER_H * (EXPANDED_REGION_MAX_W / wideW); + expect(predictRegionsHeight([wide], EXPANDED_REGION_MAX_W)).toBeCloseTo(expected, 6); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.ts b/packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.ts new file mode 100644 index 000000000..6d079a333 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/flow-region-metrics.ts @@ -0,0 +1,81 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * flow-region-metrics — the deterministic box model shared by the inline + * region renderer (`flow-region-view.tsx` + the NodeCard region tray) and the + * layout height predictor (#2670 Phase 2). + * + * An expanded container card's height must be known BEFORE render so + * `computeLayoutWithGeometry` can push the layers below it down — there is no + * measure-then-reflow pass (no ResizeObserver). That only works if predictor + * and renderer agree to the pixel, so: + * + * ⚠️ These constants ARE the region renderer's box model. Every height-stack + * dimension in the renderer is an explicit px style sourced from here — + * change either side only together. + * + * Pure module: imports only from `flow-canvas-layout` (no React), keeping the + * dependency chain one-directional: layout ← metrics ← {region-view, parts, + * FlowCanvas}. + */ + +import { + computeLayout, + diagramSize, + NODE_W, + NODE_H, + type LabeledRegion, +} from './flow-canvas-layout'; + +/** Dashed border of one region block, per side (`border` = 1px). */ +export const REGION_BLOCK_BORDER = 1; +/** Padding inside one region block (replaces rem-based `p-1.5`). */ +export const REGION_BLOCK_PAD = 6; +/** Region label row: 12px pinned line-height + 4px bottom padding (border-box). */ +export const REGION_LABEL_H = 16; +/** Vertical gap between region blocks (replaces `gap-2`). */ +export const REGION_GAP = 8; +/** Gap between the NODE_H header card and the region tray below it. */ +export const NODE_REGION_GAP = 6; +/** Region tray border, per side. */ +export const REGION_PANEL_BORDER = 1; +/** Region tray padding. */ +export const REGION_PANEL_PAD = 6; +/** Width handed to FlowRegionView inside an expanded card (tray + block chrome off NODE_W). */ +export const EXPANDED_REGION_MAX_W = + NODE_W - 2 * (REGION_PANEL_BORDER + REGION_PANEL_PAD) - 2 * (REGION_BLOCK_BORDER + REGION_BLOCK_PAD); + +/** + * Predicted rendered height of ``. + * Mirrors the renderer exactly — same float expressions, so no rounding skew: + * each block = border + padding + optional label row + the region mini-canvas + * scaled down (never up) to fit `canvasMaxWidth`; blocks separated by REGION_GAP. + */ +export function predictRegionsHeight(regions: LabeledRegion[], canvasMaxWidth: number): number { + let total = 0; + regions.forEach((region, i) => { + const { width, height } = diagramSize(computeLayout(region.nodes, region.edges)); + const scale = Math.min(1, canvasMaxWidth / Math.max(width, 1)); + total += + (i > 0 ? REGION_GAP : 0) + + 2 * (REGION_BLOCK_BORDER + REGION_BLOCK_PAD) + + (region.label ? REGION_LABEL_H : 0) + + height * scale; + }); + return total; +} + +/** + * Predicted total height of an EXPANDED container card: the NODE_H header band + * plus the region tray (gap + tray chrome + regions). `NODE_H` when there is + * nothing to expand — the collapsed/plain-card height. + */ +export function predictExpandedNodeHeight(regions: LabeledRegion[]): number { + if (regions.length === 0) return NODE_H; + return ( + NODE_H + + NODE_REGION_GAP + + 2 * (REGION_PANEL_BORDER + REGION_PANEL_PAD) + + predictRegionsHeight(regions, EXPANDED_REGION_MAX_W) + ); +} 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 050f9afb4..c6ba9d205 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 @@ -31,6 +31,7 @@ import { type LabeledRegion, } from './flow-canvas-layout'; 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 }) { @@ -97,12 +98,22 @@ function RegionCanvas({ region, maxWidth }: { region: LabeledRegion; maxWidth: n * header per region (`Branch N` / `Try` / `Catch`; a loop body has no header). */ export function FlowRegionView({ regions, maxWidth }: { regions: LabeledRegion[]; maxWidth: number }) { + // #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). return ( -
+
{regions.map((region) => ( -
+
{region.label && ( -
+
{region.label}
)} From 69c9717db7712192c8faa5425730eba634aec0fc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Jul 2026 11:44:28 +0000 Subject: [PATCH 3/3] feat(flow-designer): inline push-down expansion of container regions (replaces Phase-1 popover) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2670 Phase 2. A loop/parallel/try_catch container card now expands IN PLACE: the chevron grows the card to embed its region(s) as a read-only mini-canvas (FlowRegionView at EXPANDED_REGION_MAX_W), and the geometry-aware layout pushes the layers below down by the card's true height. Collapsed by default; expansion is session-only FlowCanvas state (never written to the draft). - NodeCard: controlled `expanded`/`onToggleExpand` + `height` (the SAME number the layout used to position everything below — DOM and layout cannot disagree); header band pinned at NODE_H; `data-node-id` hook; the Phase-1 popover block is removed (chevron now toggles inline; Popover imports stay — NodePalette uses them). - FlowCanvas: `expandedIds` view state, memoized `regionsByNode` (also fixes per-render extractRegions identity churn), `nodeHeights` via predictExpandedNodeHeight, layout switched to computeLayoutWithGeometry; expanded source cards' outgoing edges leave from their true bottom (back-edges stay header-anchored so toggling never swings arcs); reveal centering uses real heights. - Tests: inline expand/collapse containment (not portaled), parallel branch labels, legacy flat loop unchanged, and a push-down assertion — the layer below moves by exactly predictExpandedNodeHeight − NODE_H and the card renders at exactly the predicted height. - Changeset rewritten for the shipped behavior (Phase-1 popover was never released); documents the manual-position overlap limitation. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VuvxWgoadqryqBcjs7TpVi --- .changeset/flow-designer-nested-regions.md | 26 ++-- .../previews/FlowCanvas.test.tsx | 115 ++++++++++++------ .../metadata-admin/previews/FlowCanvas.tsx | 58 +++++++-- .../previews/flow-canvas-parts.tsx | 90 ++++++++------ 4 files changed, 200 insertions(+), 89 deletions(-) diff --git a/.changeset/flow-designer-nested-regions.md b/.changeset/flow-designer-nested-regions.md index d06f6b60d..ca3b4f421 100644 --- a/.changeset/flow-designer-nested-regions.md +++ b/.changeset/flow-designer-nested-regions.md @@ -2,21 +2,25 @@ "@object-ui/app-shell": minor --- -feat(studio): visualize loop / parallel / try_catch nested regions on the flow designer canvas (#2670) +feat(studio): expand loop / parallel / try_catch regions inline on the flow designer canvas (#2670) The flow designer rendered ADR-0031 structured control-flow containers (`loop` / `parallel` / `try_catch`) as opaque single node cards — their nested regions (`config.body` / `config.branches[]` / `config.try`/`catch`) were only visible, and only editable, as raw JSON in the inspector's Advanced block. -A container card now carries a **"show nested regions"** control that opens a -read-only popover rendering the region(s) as a mini-canvas — the same -top-to-bottom node/edge layout as the parent graph, produced by the shared -`computeLayout` and scaled to fit — with a header per region (a named branch or -`Branch N`, and `Try` / `Catch`; a loop body has none). Legacy flat loops (a -`loop` with no `config.body`) and all ordinary nodes render exactly as before. +A container card now carries an expand chevron that grows the card **in place** +to embed its region(s) as a read-only mini-canvas — the same top-to-bottom +node/edge layout as the parent graph, scaled to fit the card width — with a +header per region (a named branch or `Branch N`, and `Try` / `Catch`; a loop +body has none). The canvas layout is geometry-aware: the layers below an +expanded container are **pushed down** by its real height and its outgoing edge +leaves from its true bottom. Collapsed by default; expansion is session-only +view state (never written to the flow draft). Legacy flat loops (a `loop` with +no `config.body`) and all ordinary nodes render exactly as before — with no +expanded container the layout is identical to the previous release, locked by +invariance tests. -Read-only for now (Phase 1 of #2670): the region renders in a floating popover, -so it needs **no change to the canvas layout or edge routing** — zero regression -risk to existing flows. Inline push-down nesting on the canvas and nested editing -are tracked as the next increments on #2670. +Known limitation: a node pinned via a manual drag position sitting at/below an +expanded container can overlap it (manual positions are absolute); drag it +clear or collapse the container. 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 8d4a7ec66..a68e6b726 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 @@ -3,6 +3,8 @@ import { describe, it, expect, afterEach, vi } from 'vitest'; import { render, screen, cleanup, fireEvent } from '@testing-library/react'; import { FlowCanvas } from './FlowCanvas'; +import { extractRegions, NODE_H } from './flow-canvas-layout'; +import { predictExpandedNodeHeight } from './flow-region-metrics'; import type { FlowProblem } from './flow-problems'; afterEach(cleanup); @@ -99,53 +101,96 @@ describe('FlowCanvas — inline cycle/error surfacing', () => { }); }); -describe('FlowCanvas — nested container regions (#2670)', () => { - it('reveals a loop body region in a popover when the container control is opened', async () => { - const nodes = [ - { id: 'start', type: 'start' }, - { - id: 'each', - type: 'loop', - label: 'For each order', - config: { body: { nodes: [{ id: 'charge', type: 'http', label: 'Charge card' }], edges: [] } }, - }, - ]; - render( +describe('FlowCanvas — inline nested container regions (#2670 Phase 2)', () => { + 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('expands a loop body INLINE inside the container card, and collapses back', () => { + const { container } = render( {}} />, ); - // Closed by default: the body step is not rendered, but a "show regions" control is. + // Collapsed by default: no body step, an expand control with aria-expanded=false. expect(screen.queryByText('Charge card')).not.toBeInTheDocument(); - fireEvent.click(screen.getByRole('button', { name: /show nested regions/i })); - // Open: the loop body node now renders nested in the popover. - expect(await screen.findByText('Charge card')).toBeInTheDocument(); + const toggle = screen.getByRole('button', { name: 'Expand nested regions' }); + expect(toggle).toHaveAttribute('aria-expanded', 'false'); + fireEvent.click(toggle); + // Expanded: the body node renders INLINE inside the container card (not portaled). + const card = container.querySelector('[data-node-id="each"]') as HTMLElement; + expect(card).not.toBeNull(); + expect(card.textContent).toContain('Charge card'); + const collapse = screen.getByRole('button', { name: 'Collapse nested regions' }); + expect(collapse).toHaveAttribute('aria-expanded', 'true'); + fireEvent.click(collapse); + expect(screen.queryByText('Charge card')).not.toBeInTheDocument(); + }); + + it('pushes the layer below down by exactly the predicted height delta', () => { + const { container } = render( + {}} + />, + ); + const topOfAfter = () => + parseFloat((container.querySelector('[data-node-id="after"]') as HTMLElement).style.top); + const before = topOfAfter(); + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + const predicted = predictExpandedNodeHeight(extractRegions(LOOP_NODES[1] as never)); + // The card renders at the predicted height, and the node below moved down + // by exactly (predicted − NODE_H) — DOM and layout share one number. + const card = container.querySelector('[data-node-id="each"]') as HTMLElement; + expect(parseFloat(card.style.height)).toBeCloseTo(predicted, 3); + expect(topOfAfter() - before).toBeCloseTo(predicted - NODE_H, 3); }); - it('labels parallel branches and try/catch handlers when opened', async () => { - const nodes = [ - { - id: 'p', - type: 'parallel', - label: 'Fan out', - config: { - branches: [ - { name: 'Slack', nodes: [{ id: 'a', type: 'http', label: 'Notify Slack' }], edges: [] }, - { nodes: [{ id: 'b', type: 'http', label: 'Notify CRM' }], edges: [] }, - ], - }, - }, - ]; + it('labels parallel branches when expanded inline', () => { render( - {}} />, + {}} + />, ); - fireEvent.click(screen.getByRole('button', { name: /show nested regions/i })); - expect(await screen.findByText('Slack')).toBeInTheDocument(); // named branch + fireEvent.click(screen.getByRole('button', { name: 'Expand nested regions' })); + expect(screen.getByText('Slack')).toBeInTheDocument(); // named branch expect(screen.getByText('Branch 2')).toBeInTheDocument(); // unnamed → indexed expect(screen.getByText('Notify CRM')).toBeInTheDocument(); }); 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 c7e5c8cca..6011f514c 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx @@ -27,8 +27,7 @@ import { cn } from '@object-ui/components'; import { uniqueId, appendArray, spliceArray } from '../inspectors/_shared'; import { t as tr } from '../i18n'; import { - computeLayout, - diagramSize, + computeLayoutWithGeometry, NODE_W, NODE_H, bottomAnchor, @@ -45,7 +44,9 @@ import { type FlowNode, type FlowEdge, type Point, + type LabeledRegion, } from './flow-canvas-layout'; +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'; @@ -148,8 +149,41 @@ export function FlowCanvas({ const dragRef = React.useRef(null); const panRef = React.useRef(null); - const layout = React.useMemo(() => computeLayout(nodes, edges), [nodes, edges]); - const size = React.useMemo(() => diagramSize(layout), [layout]); + // #2670: structured-region containers. Which nodes carry regions (memoized so + // NodeCard props keep a stable identity), which are expanded (session-only + // view state — never written to the draft), and the per-node height feeding + // 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 regionsByNode = React.useMemo(() => { + const map = new Map(); + for (const n of nodes) { + const regions = extractRegions(n); + if (regions.length > 0) map.set(n.id, regions); + } + return map; + }, [nodes]); + const nodeHeights = React.useMemo(() => { + const map = new Map(); + for (const n of nodes) { + const regions = expandedIds.has(n.id) ? regionsByNode.get(n.id) : undefined; + map.set(n.id, regions ? predictExpandedNodeHeight(regions) : NODE_H); + } + return map; + }, [nodes, regionsByNode, expandedIds]); + + const { positions: layout, heights, size } = React.useMemo( + () => computeLayoutWithGeometry(nodes, edges, (n) => nodeHeights.get(n.id) ?? NODE_H), + [nodes, edges, nodeHeights], + ); // Simulation overlay sets (display-only; never drives engine behavior). const visitedSet = React.useMemo(() => new Set(visitedNodeIds ?? []), [visitedNodeIds]); @@ -449,11 +483,13 @@ export function FlowCanvas({ let pt: Point | null = null; if (t.kind === 'node') { const p = layout.get(t.nodeId); - if (p) pt = { x: p.x + NODE_W / 2, y: p.y + NODE_H / 2 }; + // #2670: center on the card's true (possibly expanded) height. + if (p) pt = { x: p.x + NODE_W / 2, y: p.y + (heights.get(t.nodeId) ?? NODE_H) / 2 }; } else if (t.kind === 'edge') { const s = layout.get(t.source); const d = layout.get(t.target); - if (s && d) pt = { x: (s.x + d.x) / 2 + NODE_W / 2, y: (s.y + d.y) / 2 + NODE_H / 2 }; + // Midpoint of the edge as actually drawn (source bottom → target top). + if (s && d) pt = edgeMidpoint(bottomAnchor(s, heights.get(t.source) ?? NODE_H), topAnchor(d)); } if (pt) setPan({ x: vp.clientWidth / 2 - pt.x * zoom, y: vp.clientHeight / 2 - pt.y * zoom }); // eslint-disable-next-line react-hooks/exhaustive-deps @@ -661,7 +697,10 @@ export function FlowCanvas({ const invalid = !back && !!invalidEdges?.has(`${edge.source}->${edge.target}`); const sPos = dragPos?.id === edge.source ? positionOf(edge.source) : sp; const tPos = dragPos?.id === edge.target ? positionOf(edge.target) : tp; - const from = back ? rightAnchor(sPos) : bottomAnchor(sPos); + // #2670: an expanded container's outgoing edge leaves from its + // TRUE bottom (heights map; drag-independent). Back-edges stay on + // the header band via rightAnchor, so toggling never swings arcs. + const from = back ? rightAnchor(sPos) : bottomAnchor(sPos, heights.get(edge.source) ?? NODE_H); const to = back ? rightAnchor(tPos) : topAnchor(tPos); const labelPos = back ? backEdgeLabelAnchor(from, to) : edgeMidpoint(from, to); const cond = conditionText(edge.condition); @@ -823,7 +862,10 @@ export function FlowCanvas({ } invalid={invalidNodeSet.has(node.id)} badge={nodeBadges.get(node.id)} - regions={extractRegions(node)} + regions={regionsByNode.get(node.id)} + expanded={expandedIds.has(node.id)} + onToggleExpand={regionsByNode.has(node.id) ? () => toggleExpanded(node.id) : undefined} + height={heights.get(node.id)} /> ); })} 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 cd4bf04ad..e5bc3732f 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 @@ -51,6 +51,7 @@ import { import { t as tr } from '../i18n'; import { NODE_W, NODE_H, type Point, type LabeledRegion } 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'; export function nodeIcon(type: string): LucideIcon { @@ -417,10 +418,21 @@ export interface NodeCardProps { /** * #2670: structured-region sub-graphs this node contains (`loop.body`, * `parallel.branches`, `try_catch.try`/`catch`). When present, the card gains - * an expand toggle that renders them read-only, nested beneath the header. - * Empty / absent for ordinary nodes and legacy flat loops → a plain card. + * an expand toggle that renders them read-only, INLINE beneath the header — + * the card grows and the canvas pushes lower layers down. Empty / absent for + * ordinary nodes and legacy flat loops → a plain card. */ regions?: LabeledRegion[]; + /** #2670: whether the region tray is expanded (controlled by FlowCanvas). */ + expanded?: boolean; + /** #2670: toggle the region tray. Absent → no expand affordance. */ + onToggleExpand?: () => 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 + * disagree with the layout. `NODE_H` (or absent) when collapsed/plain. + */ + height?: number; } /** @@ -429,6 +441,7 @@ export interface NodeCardProps { * (edit mode only) appends a connected child without ambiguity. */ export function NodeCard({ + id, type, label, summary, @@ -444,6 +457,9 @@ export function NodeCard({ onAppend, onAddReviseLoop, regions, + expanded, + onToggleExpand, + height, }: NodeCardProps) { const tone = nodeTone(type); const hasRegions = !!regions && regions.length > 0; @@ -454,7 +470,8 @@ export function NodeCard({ // tiny button itself. className="group absolute transition-opacity duration-200" data-invalid={invalid || undefined} - style={{ left: position.x, top: position.y, width: NODE_W, height: NODE_H, opacity: dimmed ? 0.35 : 1 }} + data-node-id={id} + style={{ left: position.x, top: position.y, width: NODE_W, height: height ?? NODE_H, opacity: dimmed ? 0.35 : 1 }} >
- {hasRegions && ( - // #2670: the container's nested regions render read-only in a Popover - // anchored to the card — floating (portaled) so it never overlaps the - // canvas's other nodes and needs no change to the layout / edge routing. - // (Inline, push-down nesting on the canvas is the next increment.) - - - - - e.stopPropagation()} - className="max-h-[60vh] w-[280px] overflow-auto p-2" - > -
- {type === 'loop' ? 'Loop body' : type === 'parallel' ? 'Parallel branches' : 'Try / Catch'} -
- - - + {hasRegions && onToggleExpand && ( + // #2670 Phase 2: the container's regions render INLINE — the card grows + // and the geometry-aware layout pushes lower layers down (replaces the + // Phase-1 read-only popover). The chevron toggles view-only state owned + // by FlowCanvas; height comes in via the `height` prop so DOM and + // layout can never disagree. + + )} + {hasRegions && expanded && ( +
e.stopPropagation()} + onClick={(e) => e.stopPropagation()} + > + +
)} {badge && (