fix(app-shell)!: converge flow node geometry to spec FlowNode.position (#3172)

.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.

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.
 

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<string> {
@@ -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<Equal<IsAny<SpecFlowNode>, false>>;
+ type _LocalNotAny = Assert<Equal<IsAny<FlowNodePosition>, false>>;
+
+ // The local geometry type IS the spec's `position`, not a copy that agrees.
+ type _IsSpecPosition = Assert<Equal<FlowNodePosition, NonNullable<SpecFlowNode['position']>>>;
+ // `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<SpecFlowNode['position']>, NonNullable<SpecFlowNodeParsed['position']>>
+ >;
+ // Both coordinates required: a half-position is not representable.
+ type _BothRequired = Assert<Equal<FlowNodePosition, { x: number; y: number }>>;
+ type _HalfIsNotAPosition = Assert<Equal<Extends<{ x: number }, FlowNodePosition>, 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<FlowDesignerNode['position'], SpecFlowNode['position']>
+ >;
+
+ 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

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<typeof vi.fn>): Array<Record<string, unknown>> => {
+ expect(onPatch).toHaveBeenCalledTimes(1);
+ const patch = onPatch.mock.calls[0][0] as { nodes?: Array<Record<string, unknown>> };
+ 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<Record<string, unknown>>) => {
+ 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<typeof vi.fn>,
+ { nodes = LEGACY_NODES, selectedId = null }: { nodes?: typeof LEGACY_NODES; selectedId?: string | null } = {},
+ ) =>
+ render(
+ <FlowCanvas
+ nodes={nodes}
+ edges={LEGACY_EDGES}
+ editable
+ designMode
+ selectedId={selectedId}
+ onSelect={() => {}}
+ 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);
+ });
+});

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<HTMLDivElement>(null);
  const [zoom, setZoom] = React.useState(1);
  const [pan, setPan] = React.useState<Point>({ 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<string, unknown> = { 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 };