diff --git a/web_ui/src/app/canvas/SceneCanvas.test.tsx b/web_ui/src/app/canvas/SceneCanvas.test.tsx index d46042e..ac49138 100644 --- a/web_ui/src/app/canvas/SceneCanvas.test.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.test.tsx @@ -19,7 +19,7 @@ import { SceneCanvas, toFlowEdges, toFlowNodes, - withPreservedSelection, + withPreservedFlowState, type MeasuredSizeSource, type SceneFlowNode, } from "./SceneCanvas"; @@ -2671,15 +2671,18 @@ describe("toFlowEdges (R7.5b-2 orthogonal routing)", () => { // R7.5c: found live, not by a test - Ctrl+Arrow's setCenter round-trips a // viewport report through the backend, the echoed snapshot rebuilt every // node, and the selection the keystroke had just made disappeared. Without -// this, branch navigation worked for exactly one hop. -describe("withPreservedSelection (R7.5c snapshot-rebuild selection wipe)", () => { - const node = (id: string, selected?: boolean) => - ({ id, selected, position: { x: 0, y: 0 }, data: {} }) as unknown as SceneFlowNode; +// this, branch navigation worked for exactly one hop. Extended later to +// also carry React Flow's measured node size across the same rebuild - see +// withPreservedFlowState's own doc comment for the node/edge blink that +// losing it caused. +describe("withPreservedFlowState (snapshot-rebuild selection + measurement wipe)", () => { + const node = (id: string, selected?: boolean, measured?: { width: number; height: number }) => + ({ id, selected, measured, position: { x: 0, y: 0 }, data: {} }) as unknown as SceneFlowNode; it("re-applies the selection onto the freshly rebuilt nodes", () => { const rebuilt = [node("a"), node("b"), node("c")]; const current = [node("a"), node("b", true), node("c")]; - const merged = withPreservedSelection(rebuilt, current); + const merged = withPreservedFlowState(rebuilt, current); expect(merged.map((n) => [n.id, !!n.selected])).toEqual([ ["a", false], ["b", true], @@ -2688,20 +2691,20 @@ describe("withPreservedSelection (R7.5c snapshot-rebuild selection wipe)", () => }); it("preserves a multi-node selection, not just a single id", () => { - const merged = withPreservedSelection( + const merged = withPreservedFlowState( [node("a"), node("b"), node("c")], [node("a", true), node("b"), node("c", true)], ); expect(merged.filter((n) => n.selected).map((n) => n.id)).toEqual(["a", "c"]); }); - it("returns the rebuilt array untouched when nothing was selected", () => { + it("returns the rebuilt array untouched when nothing needs carrying over", () => { const rebuilt = [node("a"), node("b")]; - expect(withPreservedSelection(rebuilt, [node("a"), node("b")])).toBe(rebuilt); + expect(withPreservedFlowState(rebuilt, [node("a"), node("b")])).toBe(rebuilt); }); it("cannot resurrect a node the backend deleted - it is simply absent from the rebuild", () => { - const merged = withPreservedSelection([node("a")], [node("a"), node("gone", true)]); + const merged = withPreservedFlowState([node("a")], [node("a"), node("gone", true)]); expect(merged.map((n) => n.id)).toEqual(["a"]); expect(merged.some((n) => n.selected)).toBe(false); }); @@ -2709,9 +2712,46 @@ describe("withPreservedSelection (R7.5c snapshot-rebuild selection wipe)", () => it("does not mutate the node objects it was handed", () => { const current = [node("a", true)]; const rebuilt = [node("a")]; - withPreservedSelection(rebuilt, current); + withPreservedFlowState(rebuilt, current); expect(rebuilt[0].selected).toBeUndefined(); }); + + it("carries the measured size from the current node onto a rebuilt replacement", () => { + const measured = { width: 420, height: 180 }; + const merged = withPreservedFlowState([node("a")], [node("a", false, measured)]); + expect(merged[0].measured).toEqual(measured); + }); + + it("carries selection and measured size together in one clone", () => { + const measured = { width: 300, height: 120 }; + const merged = withPreservedFlowState([node("a")], [node("a", true, measured)]); + expect(merged[0].selected).toBe(true); + expect(merged[0].measured).toEqual(measured); + }); + + it("keeps a reference-identical node untouched instead of cloning it", () => { + // A toFlowNodes cache hit hands back the exact object React Flow already + // adopted - cloning it would defeat adoptUserNodes' reference-equality + // fast path, so it must pass through by reference even when it carries + // state worth preserving. + const same = node("a", true, { width: 100, height: 50 }); + const merged = withPreservedFlowState([same], [same]); + expect(merged[0]).toBe(same); + }); + + it("does not invent a measured size the current node never had", () => { + const merged = withPreservedFlowState([node("a")], [node("a", true)]); + expect(merged[0].measured).toBeUndefined(); + }); + + it("prefers the rebuilt node's own measured size when it already has one", () => { + const rebuiltMeasured = { width: 999, height: 999 }; + const merged = withPreservedFlowState( + [node("a", false, rebuiltMeasured)], + [node("a", true, { width: 1, height: 1 })], + ); + expect(merged[0].measured).toEqual(rebuiltMeasured); + }); }); // -- R8a: "Hide Other Branches" (computeDimmedNodeIds + its toFlowNodes wiring) -- diff --git a/web_ui/src/app/canvas/SceneCanvas.tsx b/web_ui/src/app/canvas/SceneCanvas.tsx index 3fe2628..b33d79e 100644 --- a/web_ui/src/app/canvas/SceneCanvas.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.tsx @@ -2074,31 +2074,69 @@ export function computeSmartGuideFrame( } /** - * R7.5c: carry the current selection across a snapshot rebuild. + * R7.5c (extended): carry React Flow's own per-node runtime state - the + * selection flag AND the measured node size - across a snapshot rebuild. * * toFlowNodes mints brand-new node objects from every scene snapshot, so - * anything React Flow keeps on the node object rather than in the scene - - * selection above all - is dropped unless copied over explicitly. + * anything React Flow keeps on the node object rather than in the scene is + * dropped unless copied over explicitly. Two fields matter: * - * Found live, not by a test: Ctrl+Arrow calls setCenter, setCenter fires - * onMove, onMove reports the viewport to the backend, the backend echoes a - * fresh scene, and the selection the keystroke had just made vanished about - * 300ms later. That left the NEXT Ctrl+Arrow with no single selected node, - * so branch-walking died after exactly one hop. The same wipe hits any - * selection that merely overlaps a snapshot - an autosave tick, a streaming - * token - which is why it is fixed here at the rebuild rather than inside - * the shortcut handler. + * SELECTION (the original R7.5c fix): found live, not by a test - Ctrl+Arrow + * calls setCenter, setCenter fires onMove, onMove reports the viewport to + * the backend, the backend echoes a fresh scene, and the selection the + * keystroke had just made vanished about 300ms later. That left the NEXT + * Ctrl+Arrow with no single selected node, so branch-walking died after + * exactly one hop. * - * A node the backend actually removed is simply absent from `rebuilt`, so a - * stale id can never resurrect one. + * MEASURED SIZE (the node/edge blink fix): React Flow writes each node's + * measured dimensions back onto OUR node objects through ordinary + * `dimensions` changes (applyNodeChanges' own 'dimensions' case sets + * `node.measured`), and its adoptUserNodes preserves a node's internal + * measurement/handle geometry across a nodes-prop change ONLY IF the + * incoming node object either is reference-identical to the last one or + * still carries that `measured` field (verified against the installed + * @xyflow/system source: adoptUserNodes rebuilds internals for any + * new-reference node, taking `measured` from the user object - undefined + * for a fresh toFlowNodes product - and parseHandles keeps the previous + * handleBounds only when `userNode.measured` is set). A node whose + * measurement gets wiped this way is re-rendered with `visibility: hidden` + * (NodeWrapper's own `hasDimensions` gate) and every edge touching it + * unmounts entirely (getEdgePosition returns null for an uninitialized + * node) until the resize-observer cycle re-measures it a frame or more + * later. Since every scene publish rebuilds the changed rows' node objects + * (and this function's own selection clone used to mint a fresh object for + * every SELECTED node on every publish), the user-visible result was nodes + * and their connections blinking for a frame after every drag drop, + * viewport echo, and streaming patch. Carrying `measured` over closes the + * whole chain: dimensions survive, handleBounds survive, nothing unmounts. + * + * Reference-identical nodes (a toFlowNodes cache hit that is literally the + * same object React Flow already adopted) pass through untouched - cloning + * them would defeat adoptUserNodes' reference-equality fast path for no + * benefit. A node the backend actually removed is simply absent from + * `rebuilt`, so a stale id can never resurrect one; a genuinely NEW node + * has no prior state to carry and measures normally on first mount. */ -export function withPreservedSelection( +export function withPreservedFlowState( rebuilt: SceneFlowNode[], current: SceneFlowNode[], ): SceneFlowNode[] { - const selectedIds = new Set(current.filter((n) => n.selected).map((n) => n.id)); - if (selectedIds.size === 0) return rebuilt; - return rebuilt.map((n) => (selectedIds.has(n.id) ? { ...n, selected: true } : n)); + if (current.length === 0) return rebuilt; + const currentById = new Map(current.map((n) => [n.id, n])); + let changed = false; + const merged = rebuilt.map((n) => { + const prev = currentById.get(n.id); + if (!prev || prev === n) return n; + const selected = prev.selected === true; + const measured = n.measured === undefined ? prev.measured : undefined; + if (!selected && measured === undefined) return n; + changed = true; + const clone: SceneFlowNode = { ...n }; + if (selected) clone.selected = true; + if (measured !== undefined) clone.measured = measured; + return clone; + }); + return changed ? merged : rebuilt; } // Exported standalone for direct unit testing, same posture as toFlowNodes @@ -2346,7 +2384,7 @@ function CanvasInner({ useEffect(() => { if (draggingRef.current) return; setNodes((current) => - withPreservedSelection( + withPreservedFlowState( toFlowNodes( scene, store,