From a199748872a28b64c1e10e5698b03928875a26f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 12:50:59 +0000 Subject: [PATCH] fix(app-shell)!: converge flow node geometry to spec `FlowNode.position` (#3172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flow designer wrote node geometry as its own `ui: { x, y }` while `@objectstack/spec` has modelled the same object as `FlowNode.position` all along. Since objectstack#4001 the spec's node schema is `.strict()`, so `ui` is an `unrecognized_keys` error: dragging a node made the flow fail live client validation and be rejected on save with a 422. Converging the spelling is therefore a behaviour fix, not a rename. - All three write paths (drag / add-at-point / insert-on-edge) emit `position: { x, y }`. - The canvas migrates on write: `withCanonicalGeometry` lifts a stored `ui` onto `position` and strips the key at the canvas's input boundary, so every patch it emits is `ui`-free — including patches unrelated to geometry. - Reading stays backwards-compatible: `manualPosition()` prefers `position` and falls back to a legacy `ui`, keeping the "x and y both finite" predicate. - The geometry type is derived from the spec by reference (`FlowNodePosition`), the shape-copy in `FlowPreview.tsx` is gone, and `spec-symbol-parity.test.ts` gains a positive parity pin plus a runtime assertion that the spec rejects the retired spelling. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NVPjPzmmAJ2Ngtvgg5MSRa --- .../flow-node-geometry-is-spec-position.md | 38 ++++++ packages/app-shell/README.md | 9 +- .../src/__tests__/spec-symbol-parity.test.ts | 77 ++++++++++- .../previews/FlowCanvas.test.tsx | 121 +++++++++++++++++ .../metadata-admin/previews/FlowCanvas.tsx | 26 +++- .../metadata-admin/previews/FlowPreview.tsx | 26 ++-- .../previews/flow-canvas-layout.test.ts | 114 +++++++++++++++- .../previews/flow-canvas-layout.ts | 128 ++++++++++++++---- 8 files changed, 487 insertions(+), 52 deletions(-) create mode 100644 .changeset/flow-node-geometry-is-spec-position.md diff --git a/.changeset/flow-node-geometry-is-spec-position.md b/.changeset/flow-node-geometry-is-spec-position.md new file mode 100644 index 0000000000..b61ea0f19b --- /dev/null +++ b/.changeset/flow-node-geometry-is-spec-position.md @@ -0,0 +1,38 @@ +--- +"@object-ui/app-shell": minor +--- + +The flow designer writes node geometry as the spec's `FlowNode.position`, not its +own `ui: { x, y }` (objectui#3172). + +FROM: dragging, adding-at-a-point and insert-on-edge each wrote `node.ui = {x, y}` +— a fourth-generation local spelling of a concept `@objectstack/spec` has modelled +as `FlowNode.position` all along. TO: all three write `position: { x, y }`, and the +canvas migrates on write: a stored flow's legacy `ui` is lifted onto `position` and +the key removed in the first patch the canvas emits, geometry-related or not. + +**This is a behaviour fix, not a rename.** `FlowNodeSchema` has been `.strict()` +since objectstack#4001, so `ui` is an `unrecognized_keys` error: the live client +validation flagged the draft on every keystroke and the server rejected the save +with a 422. In other words, dragging a node made the flow unsavable — the +convergence is what makes the designer's most basic gesture round-trip again. A +test now parses `{ …node, ui: {x, y} }` through the spec's own schema and asserts +the rejection, so the claim is executed rather than argued. + +Reading is backwards-compatible: `manualPosition()` prefers `position` and falls +back to a legacy `ui`, so a flow stored before this change still opens with its +nodes exactly where the author left them (pinned by a test that lays out both +spellings and compares the maps). The fallback is a migration path, not a second +contract — nothing writes `ui`, and the canvas strips it at its input boundary, so +no patch can re-emit it. + +The geometry type is now derived from the spec by reference +(`FlowNodePosition = NonNullable< SpecFlowNode['position'] >`), and +`spec-symbol-parity.test.ts` pins the equality in both directions — including that +both coordinates are required, so a half-position stays unrepresentable. The +shape-copy in `FlowPreview.tsx` is gone; it reads the canvas's own node type, the +way it already read the canvas's edge type. + +Breaking for anyone reading `node.ui` off a flow draft: after the author's first +edit the key is gone and the coordinates live under `node.position`. Nothing in +this repo or the engine read it — it was a designer-local key the schema rejected. diff --git a/packages/app-shell/README.md b/packages/app-shell/README.md index eecfa21c54..a59190326e 100644 --- a/packages/app-shell/README.md +++ b/packages/app-shell/README.md @@ -332,10 +332,15 @@ Salesforce Flow Builder) instead of a flat step list. It is **dependency-free** } ``` -- **Layout** — nodes without a `ui` hint are placed by a deterministic layered +- **Layout** — nodes without a `position` are placed by a deterministic layered auto-layout (cycle-guarded), so a flow always renders cleanly even before any - manual positioning. Dragging a node persists its position to `node.ui.{x,y}`; + manual positioning. Dragging a node persists its position to the spec's + `node.position.{x,y}` (`FlowNode.position` — `x` and `y` both required); positions degrade gracefully (they are layout hints, not required data). + Flows stored with the designer's retired `node.ui.{x,y}` spelling still render + pinned, and the canvas lifts them onto `position` in the first patch it emits + (objectui#3172) — `FlowNodeSchema` is `.strict()`, so a draft that still + carries `ui` fails client-side validation and is rejected on save with a 422. - **Edges** — branch semantics (`condition`, `label`, `isDefault`) are rendered as labels on the connectors and preserved when a node is inserted on an edge. diff --git a/packages/app-shell/src/__tests__/spec-symbol-parity.test.ts b/packages/app-shell/src/__tests__/spec-symbol-parity.test.ts index 2e52c9357b..b47bd6c9bc 100644 --- a/packages/app-shell/src/__tests__/spec-symbol-parity.test.ts +++ b/packages/app-shell/src/__tests__/spec-symbol-parity.test.ts @@ -65,14 +65,24 @@ import { resolve, dirname } from 'node:path'; import { isAggregatedViewContainer } from '../views/metadata-admin/view-item-normalize'; +import { FlowNodeSchema } from '@objectstack/spec/automation'; + import type { ScreenSpec } from '../views/ScreenView'; import type { DecisionOutputDef } from '../utils/decisionOutputParams'; import type { ObjectFieldGroup } from '../views/metadata-admin/previews/object-fields-io'; +import type { + FlowNodePosition, + FlowDesignerNode, +} from '../views/metadata-admin/previews/flow-canvas-layout'; import type { ScreenSpec as SpecScreenSpec, ScreenFieldSpec as SpecScreenFieldSpec, } from '@objectstack/spec/contracts'; -import type { DecisionOutputDef as SpecDecisionOutputDef } from '@objectstack/spec/automation'; +import type { + DecisionOutputDef as SpecDecisionOutputDef, + FlowNode as SpecFlowNode, + FlowNodeParsed as SpecFlowNodeParsed, +} from '@objectstack/spec/automation'; /** Every name `@objectstack/spec` exports from any subpath — types AND values. */ function specExportNames(): Set { @@ -206,9 +216,10 @@ describe('re-exported values are the spec binding itself', () => { }); /* -------------------------------------------------------------------------- */ -/* Structural derivations — the three symbols that are neither a plain */ -/* re-export nor a rename. Each pins its ONE documented divergence, so the */ -/* divergence cannot silently grow and cannot silently outlive its reason. */ +/* Structural derivations — the symbols that are neither a plain re-export nor */ +/* a rename. Each pins its ONE documented divergence, so the divergence cannot */ +/* silently grow and cannot silently outlive its reason. `FlowNodePosition` */ +/* (objectui#3172) pins the opposite: a derivation with NO divergence at all. */ /* -------------------------------------------------------------------------- */ /** @@ -282,6 +293,64 @@ describe('DecisionOutputDef is the spec type, with no local divergence left', () }); }); +/** + * The flow designer's node GEOMETRY is the spec's `FlowNode.position`, taken by + * reference (objectui#3172). This is the positive half of the pin above: the + * canvas's node type is deliberately NOT the spec's node (it holds mid-edit + * state the spec cannot represent), but its geometry has no such excuse — it is + * the same object, so it is the same type. + * + * The runtime half asserts what makes this a behaviour fix and not a rename: the + * spec's node schema is `.strict()`, so the designer's retired `ui: { x, y }` + * spelling is REJECTED (`unrecognized_keys` in the live client validation, 422 + * on save). If that ever stops being true, the migration in + * `withCanonicalGeometry` is no longer load-bearing and should be re-argued. + */ +describe('flow node geometry IS the spec `FlowNode.position` (#3172)', () => { + it('is pinned at compile time', () => { + type _NotAny = Assert, false>>; + type _LocalNotAny = Assert, false>>; + + // The local geometry type IS the spec's `position`, not a copy that agrees. + type _IsSpecPosition = Assert>>; + // `position` carries no `.default()`, so authoring and parsed agree — the + // z.input/z.infer trap that bites `ObjectFieldGroup` cannot bite here. + type _InputEqualsParsed = Assert< + Equal, NonNullable> + >; + // Both coordinates required: a half-position is not representable. + type _BothRequired = Assert>; + type _HalfIsNotAPosition = Assert, false>>; + + // …and the designer's node carries exactly that key, optional exactly as the + // spec's is (an un-dragged node is auto-laid, in both vocabularies). + type _NodeCarriesSpecPosition = Assert< + Equal + >; + + expect(true).toBe(true); + }); + + it('the spec still spells it `position` with both coordinates required', () => { + const base = { id: 'n1', type: 'script', label: 'Do the thing' }; + expect(FlowNodeSchema.safeParse({ ...base, position: { x: 12, y: 34 } }).success).toBe(true); + expect(FlowNodeSchema.safeParse({ ...base, position: { x: 12 } }).success).toBe(false); + // No position at all is fine — the designer auto-lays those out. + expect(FlowNodeSchema.safeParse(base).success).toBe(true); + }); + + it('the spec REJECTS the designer’s retired `ui` spelling (the 422 gate)', () => { + const parsed = FlowNodeSchema.safeParse({ + id: 'n1', + type: 'script', + label: 'Do the thing', + ui: { x: 12, y: 34 }, + }); + expect(parsed.success).toBe(false); + expect(JSON.stringify(parsed.error?.issues)).toContain('unrecognized_keys'); + }); +}); + describe('ObjectFieldGroup derives from the spec schema INPUT side', () => { it('keeps `collapse` authorable (the z.input vs z.infer trap)', () => { // `collapse` carries `.default('none')`, so it is optional to AUTHOR and 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 005b3e6b24..9965d1739d 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 @@ -344,3 +344,124 @@ describe('FlowCanvas — nested-node selection on the inline canvas (#2670 Phase expect(onSelectNested).not.toHaveBeenCalled(); }); }); + +/** + * objectui#3172 — node geometry is the spec's `FlowNode.position`. + * + * `FlowNodeSchema` is `.strict()` (objectstack#4001), so the designer's retired + * `ui: { x, y }` spelling is not a cosmetic divergence: a draft carrying it is + * flagged by the live client validation and rejected on save with a 422. These + * tests therefore assert BOTH halves of the fix — every write path emits + * `position`, and no patch may carry `ui` anywhere, including the legacy nodes a + * patch merely passes through (migrate-on-write). + * + * The compiler cannot help here: `FlowDesignerNode` has an index signature, so + * `ui: {…}` type-checks. These assertions are the guard. + */ +describe('FlowCanvas — geometry writes are spec-canonical `position` (#3172)', () => { + /** A stored flow from before the convergence: node `b` is pinned via `ui`. */ + const LEGACY_NODES = [ + { id: 'a', type: 'start', label: 'Start' }, + { id: 'b', type: 'script', label: 'Do the thing', ui: { x: 400, y: 120 } }, + ]; + const LEGACY_EDGES = [{ source: 'a', target: 'b' }]; + + const patchedNodes = (onPatch: ReturnType): Array> => { + expect(onPatch).toHaveBeenCalledTimes(1); + const patch = onPatch.mock.calls[0][0] as { nodes?: Array> }; + expect(Array.isArray(patch.nodes)).toBe(true); + return patch.nodes!; + }; + + /** No node in the patch may carry the retired key — the strict-schema gate. */ + const expectNoLegacyKey = (nodes: Array>) => { + for (const n of nodes) expect(Object.keys(n)).not.toContain('ui'); + }; + + const dragCard = (container: HTMLElement, id: string, dx: number, dy: number) => { + const card = container.querySelector(`[data-node-id="${id}"] [role="button"]`) as HTMLElement; + expect(card).not.toBeNull(); + fireEvent.pointerDown(card, { button: 0, clientX: 0, clientY: 0 }); + fireEvent.pointerMove(card, { clientX: dx, clientY: dy }); + fireEvent.pointerUp(card, { clientX: dx, clientY: dy }); + }; + + const renderCanvas = ( + onPatch: ReturnType, + { nodes = LEGACY_NODES, selectedId = null }: { nodes?: typeof LEGACY_NODES; selectedId?: string | null } = {}, + ) => + render( + {}} + onPatch={onPatch} + />, + ); + + it('renders a legacy `ui`-pinned node at its stored point (compat read)', () => { + const { container } = renderCanvas(vi.fn()); + const card = container.querySelector('[data-node-id="b"]') as HTMLElement; + expect(card.style.left).toBe('400px'); + expect(card.style.top).toBe('120px'); + }); + + it('drag → the dragged node gets `position`, and no node keeps `ui`', () => { + const onPatch = vi.fn(); + const { container } = renderCanvas(onPatch); + dragCard(container, 'b', 30, 20); + const nodes = patchedNodes(onPatch); + const dragged = nodes.find((n) => n.id === 'b')!; + // Dropped 30/20 px from its stored (400, 120) pin — committed as `position`. + expect(dragged.position).toEqual({ x: 430, y: 140 }); + expectNoLegacyKey(nodes); + }); + + it('drag of ANOTHER node still heals the legacy one (migrate-on-write)', () => { + const onPatch = vi.fn(); + const { container } = renderCanvas(onPatch); + dragCard(container, 'a', 25, 25); + const nodes = patchedNodes(onPatch); + // The untouched legacy node is lifted onto `position` at the same point… + expect(nodes.find((n) => n.id === 'b')!.position).toEqual({ x: 400, y: 120 }); + // …and the dragged node carries a fresh finite position. + const moved = nodes.find((n) => n.id === 'a')!.position as { x: number; y: number }; + expect(Number.isFinite(moved.x) && Number.isFinite(moved.y)).toBe(true); + expectNoLegacyKey(nodes); + }); + + it('insert-on-edge → the new node is pinned via `position`', () => { + const onPatch = vi.fn(); + renderCanvas(onPatch); + fireEvent.click(screen.getByRole('button', { name: 'Insert node here' })); + const nodes = patchedNodes(onPatch); + expect(nodes).toHaveLength(3); + const inserted = nodes[2].position as { x: number; y: number }; + expect(Number.isFinite(inserted.x) && Number.isFinite(inserted.y)).toBe(true); + expectNoLegacyKey(nodes); + }); + + it('append (+ handle) → new node is auto-laid, legacy node still healed', () => { + const onPatch = vi.fn(); + renderCanvas(onPatch); + fireEvent.click(screen.getAllByRole('button', { name: 'Add connected node' })[0]); + const nodes = patchedNodes(onPatch); + expect(nodes).toHaveLength(3); + // An appended node is deliberately unpinned (the layered layout slots it). + expect(nodes[2].position).toBeUndefined(); + expect(nodes.find((n) => n.id === 'b')!.position).toEqual({ x: 400, y: 120 }); + expectNoLegacyKey(nodes); + }); + + it('a patch that is not about geometry at all is still `ui`-free (delete)', () => { + const onPatch = vi.fn(); + renderCanvas(onPatch, { selectedId: 'a' }); + fireEvent.keyDown(screen.getByRole('application', { name: 'Flow canvas' }), { key: 'Delete' }); + const nodes = patchedNodes(onPatch); + expect(nodes.map((n) => n.id)).toEqual(['b']); + expectNoLegacyKey(nodes); + }); +}); 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 dc05068104..897ad88c15 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx @@ -8,7 +8,8 @@ * Shadcn cards over an SVG edge layer, laid out top-to-bottom by a * deterministic layered algorithm (`flow-canvas-layout`). Authors can: * - * - drag to reposition nodes (committed to `node.ui = {x,y}` on drop), + * - drag to reposition nodes (committed to the spec's `node.position = {x,y}` + * on drop — objectui#3172), * - add nodes from a palette (toolbar or a node's bottom "+" handle), * - insert a node on an edge ("+" at the edge midpoint splits A→B), * - delete the selected node (Delete/Backspace) with full edge cleanup, @@ -41,6 +42,7 @@ import { edgeKey, conditionText, extractRegions, + withCanonicalGeometry, type FlowDesignerNode, type FlowDesignerEdge, type Point, @@ -127,7 +129,7 @@ export interface FlowCanvasProps { } export function FlowCanvas({ - nodes, + nodes: storedNodes, edges, editable, designMode, @@ -148,6 +150,14 @@ export function FlowCanvas({ onSelectNested, onPatch, }: FlowCanvasProps) { + // objectui#3172 — the ONE geometry boundary: nodes enter the canvas with the + // retired `ui: {x,y}` spelling already lifted onto the spec's `position`, so + // every patch below is built from canonical nodes and no write path can + // re-emit `ui` (the compiler cannot catch that for us — `FlowDesignerNode` + // has an index signature). Same reference when nothing needed migrating, so + // the memos keyed on `nodes` are unaffected. + const nodes = React.useMemo(() => withCanonicalGeometry(storedNodes), [storedNodes]); + const viewportRef = React.useRef(null); const [zoom, setZoom] = React.useState(1); const [pan, setPan] = React.useState({ x: 0, y: 0 }); @@ -243,7 +253,7 @@ export function FlowCanvas({ const idx = nodes.findIndex((n) => n.id === id); if (idx < 0) return; const node = nodes[idx]; - const nextNode: FlowDesignerNode = { ...node, ui: { ...(node.ui ?? {}), x, y } }; + const nextNode: FlowDesignerNode = { ...node, position: { x, y } }; onPatch({ nodes: spliceArray(nodes, idx, nextNode) }); }, [nodes, onPatch], @@ -260,7 +270,13 @@ export function FlowCanvas({ // spaces it horizontally among siblings — pinning it directly under the // parent (the old behavior) made every sibling stack on the same spot. const at = opts?.at; - const newNode: FlowDesignerNode = { id, type, label, ...defaultNodeExtras(type), ...(at ? { ui: { x: at.x, y: at.y } } : {}) }; + const newNode: FlowDesignerNode = { + id, + type, + label, + ...defaultNodeExtras(type), + ...(at ? { position: { x: at.x, y: at.y } } : {}), + }; const nextNodes = appendArray(nodes, newNode); const patch: Record = { nodes: nextNodes }; if (opts?.from) { @@ -313,7 +329,7 @@ export function FlowCanvas({ type, label: defaultNodeLabel(type, locale), ...defaultNodeExtras(type), - ui: { x: at.x, y: at.y }, + position: { x: at.x, y: at.y }, }; // A→N inherits the original edge's branch semantics; N→B is plain. const firstSegment: FlowDesignerEdge = { ...edge, target: id }; 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 247ad55272..da568c0771 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx @@ -37,30 +37,24 @@ import { uniqueId, appendArray } from '../inspectors/_shared'; import { t as tr, translateFlowMeta } from '../i18n'; import { FlowCanvas } from './FlowCanvas'; import { defaultNodeLabel } from './flow-canvas-parts'; -import { edgeKey, type FlowDesignerEdge } from './flow-canvas-layout'; +import { edgeKey, type FlowDesignerEdge, type FlowDesignerNode } 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'; import { buildFlowProblems, deriveInvalidElements, type FlowProblem } from './flow-problems'; -interface FlowNode { - id: string; - type: string; - label?: string; - config?: Record; - ui?: { x?: number; y?: number }; - [k: string]: unknown; -} - /** - * This preview reads the draft's edges and hands them straight to - * {@link FlowCanvas}, so it reads them as the canvas's own type rather than - * restating the shape. It used to restate it — including a `condition` typed - * `string | { source?: string }`, an envelope the spec's `FlowEdgeSchema` - * rejects for want of `dialect`. Two copies of one shape is how the wrong one - * survives being fixed (objectui#3202). + * This preview reads the draft's nodes and edges and hands them straight to + * {@link FlowCanvas}, so it reads them as the canvas's own types rather than + * restating the shapes. It used to restate both — the edge including a + * `condition` typed `string | { source?: string }`, an envelope the spec's + * `FlowEdgeSchema` rejects for want of `dialect`; the node including a + * `ui?: { x?: number; y?: number }` geometry key the spec's `.strict()` + * `FlowNodeSchema` rejects outright (objectui#3172). Two copies of one shape is + * how the wrong one survives being fixed (objectui#3202). */ +type FlowNode = FlowDesignerNode; type FlowEdge = FlowDesignerEdge; interface FlowVariable { 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 aa08411857..519b66a928 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 @@ -11,6 +11,9 @@ import { backEdgeLabelAnchor, bottomAnchor, extractRegions, + hasManualPosition, + manualPosition, + withCanonicalGeometry, NODE_W, NODE_H, V_GAP, @@ -187,7 +190,16 @@ describe('computeLayoutWithGeometry — constant-height invariance (the regressi ], }, { - name: 'manual ui position', + name: 'manual position (spec-canonical)', + nodes: [ + { id: 's', type: 'start' }, + { id: 'pin', type: 'script', position: { x: 400, y: 10 } }, + { id: 'e', type: 'end' }, + ], + edges: [{ source: 's', target: 'pin' }, { source: 'pin', target: 'e' }], + }, + { + name: 'manual position (legacy `ui`, read-compat)', nodes: [ { id: 's', type: 'start' }, { id: 'pin', type: 'script', ui: { x: 400, y: 10 } }, @@ -248,7 +260,7 @@ describe('computeLayoutWithGeometry — cumulative variable-height offsets (#267 it('a manually-pinned tall node does not push auto rows (accepted-overlap rule)', () => { const nodes: FlowDesignerNode[] = [ { id: 's', type: 'start' }, - { id: 'pin', type: 'loop', ui: { x: 400, y: 10 } }, + { id: 'pin', type: 'loop', position: { x: 400, y: 10 } }, { id: 'e', type: 'end' }, ]; const edges: FlowDesignerEdge[] = [{ source: 's', target: 'pin' }, { source: 'pin', target: 'e' }]; @@ -265,3 +277,101 @@ describe('bottomAnchor with explicit height (#2670)', () => { expect(bottomAnchor({ x: 10, y: 20 }, 200)).toEqual({ x: 10 + NODE_W / 2, y: 220 }); }); }); + +// ── objectui#3172: node geometry IS the spec's `FlowNode.position` ─────────── + +describe('manualPosition — `position` is canonical, `ui` is legacy read-only', () => { + it('reads the spec-canonical `position`', () => { + expect(manualPosition({ id: 'n', type: 'script', position: { x: 12, y: 34 } })).toEqual({ x: 12, y: 34 }); + }); + + it('falls back to a legacy `ui` so stored flows still open pinned', () => { + expect(manualPosition({ id: 'n', type: 'script', ui: { x: 12, y: 34 } })).toEqual({ x: 12, y: 34 }); + }); + + it('prefers `position` when a legacy node carries both', () => { + expect( + manualPosition({ id: 'n', type: 'script', position: { x: 1, y: 2 }, ui: { x: 90, y: 90 } }), + ).toEqual({ x: 1, y: 2 }); + }); + + it('is null unless BOTH coordinates are finite (either spelling)', () => { + expect(manualPosition({ id: 'n', type: 'script' })).toBeNull(); + expect(manualPosition({ id: 'n', type: 'script', ui: { x: 400 } })).toBeNull(); + expect(manualPosition({ id: 'n', type: 'script', ui: { y: 400 } })).toBeNull(); + expect(manualPosition({ id: 'n', type: 'script', position: { x: NaN, y: 1 } })).toBeNull(); + // A half-position must not shadow a usable legacy one either. + expect( + manualPosition({ id: 'n', type: 'script', position: { x: 5 } as never, ui: { x: 7, y: 8 } }), + ).toEqual({ x: 7, y: 8 }); + expect(hasManualPosition({ id: 'n', type: 'script', ui: { x: 400 } })).toBe(false); + expect(hasManualPosition({ id: 'n', type: 'script', position: { x: 4, y: 0 } })).toBe(true); + }); + + it('pins a legacy node at exactly the same point as the canonical spelling', () => { + const edges: FlowDesignerEdge[] = [{ source: 's', target: 'pin' }]; + const legacy = computeLayout( + [{ id: 's', type: 'start' }, { id: 'pin', type: 'script', ui: { x: 400, y: 10 } }], + edges, + ); + const canonical = computeLayout( + [{ id: 's', type: 'start' }, { id: 'pin', type: 'script', position: { x: 400, y: 10 } }], + edges, + ); + expect(legacy.get('pin')).toEqual({ x: 400, y: 10 }); + expect(legacy).toEqual(canonical); + }); +}); + +describe('withCanonicalGeometry — migrate-on-write (the strict-schema gate)', () => { + it('lifts a legacy `ui` onto `position` and drops the `ui` key', () => { + const out = withCanonicalGeometry([{ id: 'n', type: 'script', ui: { x: 7, y: 9 } }]); + expect(out[0]).toEqual({ id: 'n', type: 'script', position: { x: 7, y: 9 } }); + expect('ui' in out[0]).toBe(false); + }); + + it('keeps an existing `position` and still drops a stale `ui`', () => { + const out = withCanonicalGeometry([ + { id: 'n', type: 'script', position: { x: 1, y: 2 }, ui: { x: 90, y: 90 } }, + ]); + expect(out[0].position).toEqual({ x: 1, y: 2 }); + expect('ui' in out[0]).toBe(false); + }); + + it('drops an unusable `ui` without inventing a position', () => { + const out = withCanonicalGeometry([{ id: 'n', type: 'script', ui: { x: 5 } }]); + expect(out[0]).toEqual({ id: 'n', type: 'script' }); + }); + + it('preserves every other key, including ones the canvas does not understand', () => { + const out = withCanonicalGeometry([ + { + id: 'n', + type: 'http', + label: 'Call', + config: { url: 'https://x' }, + connectorConfig: { connectorId: 'c', actionId: 'a' }, + ui: { x: 3, y: 4 }, + }, + ]); + expect(out[0]).toEqual({ + id: 'n', + type: 'http', + label: 'Call', + config: { url: 'https://x' }, + connectorConfig: { connectorId: 'c', actionId: 'a' }, + position: { x: 3, y: 4 }, + }); + }); + + it('returns the SAME array when nothing needs migrating (memo identity)', () => { + const nodes: FlowDesignerNode[] = [ + { id: 's', type: 'start' }, + { id: 'n', type: 'script', position: { x: 1, y: 2 } }, + ]; + expect(withCanonicalGeometry(nodes)).toBe(nodes); + // …and it is idempotent: migrating twice is migrating once. + const once = withCanonicalGeometry([{ id: 'n', type: 'script', ui: { x: 7, y: 9 } }]); + expect(withCanonicalGeometry(once)).toBe(once); + }); +}); 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 90cce2f53e..1c33f0f2c9 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 @@ -11,15 +11,39 @@ * Salesforce Flow Builder). Origin is the top-left of the diagram bounding * box after normalization, so every node sits at x >= PADDING, y >= PADDING. * - * "Dependency-free" means no RUNTIME dependency: the one import below is a - * `import type`, erased at compile time, and it is here precisely so the + * "Dependency-free" means no RUNTIME dependency: the imports below are + * `import type`, erased at compile time, and they are here precisely so the * designer's edge guard cannot drift from the spec's expression envelope - * (see {@link FlowDesignerEdge}). + * (see {@link FlowDesignerEdge}) and its node geometry cannot drift from the + * spec's `FlowNode.position` (see {@link FlowNodePosition}). */ import type { ExpressionInput } from '@objectstack/spec/shared'; +import type { FlowNode as SpecFlowNode } from '@objectstack/spec/automation'; -export interface FlowNodeUI { +/** + * Canvas geometry of a node — **the spec's own `FlowNode.position`**, taken by + * reference rather than restated (objectui#3172). + * + * `{ x: number; y: number }`, both REQUIRED: a half-coordinate is not a + * position, and the spec's node schema rejects one. Derived from the spec's + * authoring type so the two cannot drift; `__tests__/spec-symbol-parity.test.ts` + * pins the equality in both directions. + */ +export type FlowNodePosition = NonNullable; + +/** + * The designer's pre-objectui#3172 geometry spelling, kept for READING stored + * flows only. + * + * @deprecated Never write this. `FlowSchema`'s node is `.strict()` + * (objectstack#4001), so a node carrying `ui` is rejected by client-side + * validation and by the server with a 422 — this spelling could not round-trip + * even before it was retired. {@link withCanonicalGeometry} lifts it onto + * {@link FlowNodePosition} at the canvas boundary, so a stored flow heals on the + * author's first edit. + */ +export interface LegacyFlowNodeUI { x?: number; y?: number; } @@ -48,23 +72,29 @@ export interface FlowNodeUI { * every missing member (objectstack#4075) — which is precisely why the NAME had * to stop claiming they are the same thing. * - * KNOWN DIVERGENCE, left alone deliberately: this designer persists geometry as - * `ui: { x, y }` while the spec already models it twice - * (`FlowNode.position`, and `FlowCanvasNode`). Three spellings of one concept - * is a real defect, but reconciling it changes what gets written to metadata — - * a behaviour change that does not belong in a symbol burn-down. See the PR - * description; filed as a follow-up. + * Geometry is NOT a layer difference and no longer diverges: the canvas writes + * the spec's own `position` (objectui#3172). The designer's old `ui: { x, y }` + * spelling is read-only legacy — see {@link LegacyFlowNodeUI} and + * {@link withCanonicalGeometry}. (`@objectstack/spec/studio`'s `FlowCanvasNode` + * is a third name but not a third geometry: it is the visual overlay keyed BY + * node id, with no consumer in this repo.) * * `__tests__/spec-symbol-parity.test.ts` pins that the spec owns neither - * `FlowDesignerNode` nor `FlowDesignerEdge`. + * `FlowDesignerNode` nor `FlowDesignerEdge`, and that `position` here IS the + * spec's. */ export interface FlowDesignerNode { id: string; type: string; label?: string; config?: Record; - /** UI-only layout hint persisted via onPatch; ignored by the runtime. */ - ui?: FlowNodeUI; + /** + * Canvas position, spec-canonical (`FlowNode.position`). Optional because an + * un-dragged node is placed by the auto-layout, exactly as in the spec. + */ + position?: FlowNodePosition; + /** @deprecated Read-only legacy geometry — see {@link LegacyFlowNodeUI}. */ + ui?: LegacyFlowNodeUI; [k: string]: unknown; } @@ -124,9 +154,59 @@ function isFiniteNum(v: unknown): v is number { return typeof v === 'number' && Number.isFinite(v); } +/** + * The persisted manual position of a node — **the** reader for node geometry. + * + * Spec-canonical `position` first; the retired `ui: { x, y }` spelling is read + * ONLY as a fallback so a flow stored before objectui#3172 still opens with its + * nodes where the author left them. Nothing writes `ui` any more, and + * {@link withCanonicalGeometry} strips it at the canvas boundary, so the + * fallback is a migration path and not a second contract. + * + * Both coordinates must be finite — a half or NaN coordinate is not a position + * (and the spec's node schema rejects one), so such a node is auto-laid out + * instead of being pinned at a garbage point. + */ +export function manualPosition(node: FlowDesignerNode): FlowNodePosition | null { + const p = node.position; + if (isFiniteNum(p?.x) && isFiniteNum(p?.y)) return { x: p.x, y: p.y }; + const legacy = node.ui; + if (isFiniteNum(legacy?.x) && isFiniteNum(legacy?.y)) return { x: legacy.x, y: legacy.y }; + return null; +} + /** A node carries a persisted manual position when both x and y are finite. */ export function hasManualPosition(node: FlowDesignerNode): boolean { - return isFiniteNum(node.ui?.x) && isFiniteNum(node.ui?.y); + return manualPosition(node) !== null; +} + +/** + * Migrate-on-write boundary (objectui#3172): return `nodes` with every legacy + * `ui: { x, y }` lifted onto the spec's `position` and the `ui` key REMOVED. + * + * The canvas runs its own `nodes` prop through this, so every patch it emits is + * built from already-canonical nodes and can never re-emit `ui` — including the + * patches that have nothing to do with geometry (delete, revise loop). That + * matters because a draft carrying `ui` is unsavable, not merely untidy: + * `FlowSchema`'s node is `.strict()` (objectstack#4001), so the key surfaces as + * `unrecognized_keys` in the live client validation and as a 422 on save. A + * stored flow therefore heals on the author's first edit. + * + * A node whose legacy `ui` is unusable (missing or non-finite coordinate) loses + * the key without gaining a position: it was never rendered as pinned, and + * keeping it would keep the draft unsavable. + * + * Returns the SAME array reference when nothing needed migrating, so callers can + * use it inside a `useMemo` without invalidating downstream memos every render. + */ +export function withCanonicalGeometry(nodes: FlowDesignerNode[]): FlowDesignerNode[] { + if (!nodes.some((n) => n && 'ui' in n)) return nodes; + return nodes.map((n) => { + if (!n || !('ui' in n)) return n; + const { ui: _legacy, ...rest } = n; + const p = manualPosition(n); + return p ? { ...rest, position: p } : rest; + }); } /** @@ -208,17 +288,18 @@ export interface FlowLayoutGeometry { * - Nodes never reached from a root are dropped into a trailing layer so the * author still sees them. * - 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. + * - A node with a persisted manual position ({@link manualPosition}) 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 + * tests). Manually-positioned 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 computeLayoutWithGeometry( @@ -316,8 +397,8 @@ export function computeLayoutWithGeometry( ids.forEach((id, i) => { 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). + // Manually-positioned 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); } @@ -344,8 +425,9 @@ export function computeLayoutWithGeometry( // Apply persisted manual overrides on top of the normalized frame. for (const n of nodes) { - if (hasManualPosition(n)) { - positions.set(n.id, { x: Math.max(0, n.ui!.x!), y: Math.max(0, n.ui!.y!) }); + const manual = manualPosition(n); + if (manual) { + positions.set(n.id, { x: Math.max(0, manual.x), y: Math.max(0, manual.y) }); } }