From f7a18bf83f2c3fc778ecf1ad79972b5737e77f66 Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Fri, 14 Aug 2026 09:44:06 -0400 Subject: [PATCH] Fix nodes and connections blinking after every scene update Every scene update from the backend rebuilds the canvas's node objects. React Flow stores each node's measured size on those objects, and when a rebuilt object arrives without that measurement, React Flow discards the node's size and connection-point geometry and has to measure the node again from scratch. Until that re-measurement lands - at least one frame later - the node is rendered invisible and every connection touching it is removed outright. Because updates arrive after every drag, every pan or zoom report, and every streamed response chunk, the practical effect was nodes and their connections flickering during ordinary use, worst on whichever node the user had just selected or moved. The selection-preserving step that already runs on every rebuild (withPreservedSelection) made this worse: it cloned every selected node into a fresh object on every update, so the selected node lost its measurement even when nothing about it had changed. Fix: that step is now withPreservedFlowState, and it carries the measured size across the rebuild alongside the selection flag. With the measurement present, React Flow keeps the node's existing geometry, so nothing turns invisible and no connection is removed while a node waits to be measured again. Nodes that kept their exact object identity pass through untouched, so the fast path for unchanged nodes is preserved. Verified against the installed @xyflow/system source at each step: adoptUserNodes only rebuilds a node's internals when the object reference changes, keeps handle geometry only when the incoming object carries a measurement, getEdgePosition returns null for an unmeasured node (unmounting the edge), and the node wrapper renders visibility:hidden until dimensions exist. Co-Authored-By: Claude Fable 5 --- web_ui/src/app/canvas/SceneCanvas.test.tsx | 62 ++++++++++++++---- web_ui/src/app/canvas/SceneCanvas.tsx | 74 ++++++++++++++++------ 2 files changed, 107 insertions(+), 29 deletions(-) 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,