From ca4ac5130db4a9bfafccbec997516c9ea8f09aef Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Fri, 14 Aug 2026 15:12:42 -0400 Subject: [PATCH] Let React Flow own node state so drag frames render inside the pointer event Working node editors update a dragged node's position and the connection paths attached to it synchronously, in the same mousemove handler. Drawflow is a clear reference: its drag handler writes the node's position and then immediately recomputes and writes every affected connection's path, with no framework state and no deferred work in between. This canvas did the opposite. Node state was controlled, so every drag frame travelled through this component's React state, a re-render, and React Flow's own prop-sync effect - a passive effect, which React runs after the browser has already had the opportunity to paint - before the renderer learned the new position. Node cards and connection geometry are both produced by React Flow from that state, so the whole canvas was being updated a hop later than the gesture that drove it. React Flow now owns the node collection (defaultNodes). A drag frame is applied to its store synchronously inside the pointer event, and the node card and its connections re-render together from that single write. The component keeps a mirror of the collection for its own logic - drag corrections, delete routing, scene merging - and pushes backend scene snapshots into the store explicitly, which is not per-frame work. Combined with the drag corrections already running inside React Flow's change pipeline, the position a gesture produces is now computed once and rendered once, with nothing between the pointer event and the frame. Also adds web_ui/src/app/canvas/drag/dragCorrections.ts: the drag correction maths (speed factor, smart-guide snap, group cascade) as a pure, framework-free module with an explicit contract, so the rule "one position, computed once" is enforceable in one reviewable place. Co-Authored-By: Claude Opus 5 --- web_ui/src/app/canvas/SceneCanvas.tsx | 49 +++-- web_ui/src/app/canvas/drag/dragCorrections.ts | 193 ++++++++++++++++++ 2 files changed, 229 insertions(+), 13 deletions(-) create mode 100644 web_ui/src/app/canvas/drag/dragCorrections.ts diff --git a/web_ui/src/app/canvas/SceneCanvas.tsx b/web_ui/src/app/canvas/SceneCanvas.tsx index 2042d1e..0e97ffb 100644 --- a/web_ui/src/app/canvas/SceneCanvas.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.tsx @@ -8,6 +8,7 @@ import { ViewportPortal, experimental_useOnNodesChangeMiddleware, useReactFlow, + useStoreApi, type Connection, type Edge, type Node, @@ -143,6 +144,9 @@ const EDGE_TYPES = { // fresh literal every time - React Flow reads these by reference in its // own internal effects, so a fresh literal every render meant those // effects re-ran on every unrelated re-render for no behavioral reason. +// defaultNodes is read once when React Flow initialises its store; a stable +// module constant keeps that unambiguous and allocation-free. +const EMPTY_NODES: SceneFlowNode[] = []; const DELETE_KEY_CODES = ["Delete", "Backspace"]; const PRO_OPTIONS = { hideAttribution: true }; const DEFAULT_EDGE_OPTIONS = { type: "default" as const }; @@ -2259,7 +2263,12 @@ function CanvasInner({ // Local node state exists so dragging is fluid; backend snapshots are the // truth and reconcile in whenever nothing is being dragged. dragStartRef // powers the drag-speed scaling contract (see scaleDragPosition). - const [nodes, setNodes] = useState([]); + // React Flow owns the rendered node collection (see the + // element's own defaultNodes comment). This component keeps only a mirror + // ref for its own logic - drag corrections, delete routing, scene merge - + // so a drag frame never has to travel through React state to reach the + // renderer. + const storeApi = useStoreApi(); const dragStartRef = useRef>(new Map()); const draggingRef = useRef(false); // ADR-011 stage 11.3: the smart-guide size cache - populated ONCE per drag @@ -2421,7 +2430,10 @@ function CanvasInner({ ); // Mirror advances with the state it describes - see nodesRef's comment. nodesRef.current = next; - setNodes(next); + // Straight into React Flow's own store rather than through React state: + // this is the same setter React Flow's internal prop-sync would call, so + // the renderer is updated in this effect instead of one commit later. + storeApi.getState().setNodes(next); }, [ scene, store, onOpenDocumentView, effectiveBranchFocusOriginId, onToggleBranchFocus, focusAcceptedPaths, getComposerRoute, filterKinds, filterStatuses, @@ -2691,15 +2703,14 @@ function CanvasInner({ // advances in this same event: a drag can deliver several frames // before React commits, and each must build on the previous frame's // result - see nodesRef's own comment above. - const next = applyNodeChanges(changes, currentNodes); - // react-hooks/immutability flags this because the same mirror is also - // assigned in the scene-sync effect above. Both writers are correct and - // both are required: the effect publishes backend snapshots, this - // handler publishes drag frames, and a drag can deliver several frames - // between commits - which is the entire reason the mirror exists. + // React Flow has ALREADY applied this batch to its own store by the + // time this handler runs (that is what uncontrolled node state buys: + // the renderer is updated synchronously inside the pointer event, the + // way working node editors do it, instead of waiting for React state + // and a post-paint sync). All that remains here is keeping this + // component's mirror in step for its own logic. // eslint-disable-next-line react-hooks/immutability - nodesRef.current = next; - setNodes(next); + nodesRef.current = applyNodeChanges(changes, currentNodes); }, [store], ); @@ -2723,7 +2734,7 @@ function CanvasInner({ const chatNodeIds: string[] = []; const otherNodeIds: string[] = []; for (const deleted of deletedNodes) { - const flowNode = nodes.find((n) => n.id === deleted.id); + const flowNode = nodesRef.current.find((n) => n.id === deleted.id); (flowNode?.type === "chat" ? chatNodeIds : otherNodeIds).push(deleted.id); } for (const id of chatNodeIds) store.deleteChatNode(id); @@ -2735,7 +2746,7 @@ function CanvasInner({ deletedEdges.filter((e) => !dying.has(e.source) && !dying.has(e.target)).map((e) => e.id), ); }, - [store, nodes], + [store], ); const onSelectionChange = useCallback( @@ -2773,7 +2784,19 @@ function CanvasInner({ data-testid="scene-canvas" > re-render -> React + Flow's prop-sync effect (a PASSIVE effect, so it lands after the + browser has already had a chance to paint) before the renderer + learns the new position. Working node editors do the opposite: + Drawflow, for one, writes the node's position and its connection + paths synchronously inside the same mousemove handler. Handing + node state to React Flow reproduces that shape here - a drag + frame is applied to its store inside the pointer event, and the + node card and its connections re-render together from that one + store write. Scene snapshots from the backend are pushed in + explicitly via the store (see the scene-sync effect above). */ + defaultNodes={EMPTY_NODES} edges={edges} nodeTypes={NODE_TYPES} edgeTypes={EDGE_TYPES} diff --git a/web_ui/src/app/canvas/drag/dragCorrections.ts b/web_ui/src/app/canvas/drag/dragCorrections.ts new file mode 100644 index 0000000..ae9fe7e --- /dev/null +++ b/web_ui/src/app/canvas/drag/dragCorrections.ts @@ -0,0 +1,193 @@ +/** + * The canvas drag pipeline's pure core. + * + * Dragging a node on this canvas is not "move it to the pointer". Three + * corrections shape every frame, and they must all be applied to the SAME + * position value before anything renders it: + * + * 1. the drag-speed factor (scaleDragPosition), the Qt canvas's own + * contract - motion is scaled relative to where the gesture started, + * so at a factor below 1 the node deliberately trails the pointer; + * 2. smart-guide snapping, which pulls the position onto an alignment + * with other nodes; + * 3. the group cascade, which carries a group's members by whatever delta + * the group itself ended up moving - necessarily AFTER 1 and 2, or + * members ride an uncorrected delta and drift away from their group. + * + * Why this file exists at all: these corrections used to be inline in + * SceneCanvas's change handler, downstream of React Flow's own bookkeeping, + * which meant the library and this app held two different positions for the + * same node on every frame - and since the library computes CONNECTION + * geometry from its own records, a node card and the line attached to it + * were placed from different numbers. Correcting inside React Flow's change + * pipeline (see useNodeDragPipeline) makes the corrected position the only + * position. Keeping the maths here - pure, React-free, framework-free - + * is what makes that correctness testable without mounting a canvas, and + * keeps the rule "one position, computed once" enforceable in one place + * rather than spread through a component. + * + * Everything in this module is a pure function of its arguments. The only + * mutable state a drag needs (where each node started, the size cache) is + * owned by the caller and passed in, so a test can drive a hundred + * simulated frames deterministically. + */ + +import type { NodeChange } from "@xyflow/react"; +// Type-only import: erased at compile time, so this creates no runtime +// dependency back onto the component that consumes this module. +import type { SceneFlowNode } from "../SceneCanvas"; +import { scaleDragPosition } from "../sceneStore"; +import type { GuideLine } from "../smartGuides"; + +/** Where a node sat when the current gesture began, keyed by node id. */ +export type DragStartPositions = Map; + +/** Node sizes captured once per gesture, for smart-guide alignment. */ +export type DragSizeCache = Map; + +/** + * Everything a correction pass needs from the outside world. Passed in + * rather than reached for, so this module never depends on React, on the + * canvas component, or on when the caller happens to run it. + */ +export interface DragCorrectionContext { + /** The nodes as they currently stand, including any earlier frame's result. */ + nodes: SceneFlowNode[]; + /** Gesture-scoped start positions; this pass records newly-seen nodes into it. */ + dragStarts: DragStartPositions; + /** Gesture-scoped node sizes, built once at gesture start by the caller. */ + sizeCache: DragSizeCache; + /** The scene's drag-speed factor (1 = pointer-exact). */ + dragFactor: number; + /** Whether smart-guide snapping is enabled for this scene. */ + smartGuidesEnabled: boolean; + /** Snap + guide computation for one frame, injected so this stays pure. */ + computeSnap: ( + nodes: SceneFlowNode[], + nodeId: string, + position: { x: number; y: number }, + sizeCache: DragSizeCache, + ) => { position: { x: number; y: number }; guides: GuideLine[] }; + /** Group-member cascade for one corrected position, injected for the same reason. */ + computeGroupCascade: ( + nodes: SceneFlowNode[], + draggedId: string, + position: { x: number; y: number }, + ) => NodeChange[]; +} + +export interface DragCorrectionResult { + /** The batch to hand onward: corrected changes plus any member changes. */ + changes: NodeChange[]; + /** Alignment guides this frame produced, for the caller to render. */ + guides: GuideLine[]; + /** True when this batch contained at least one frame of an active gesture. */ + touchedGesture: boolean; +} + +/** + * True when a position change belongs to a gesture this pipeline is + * carrying: either React Flow flagged it as dragging, or it names a node + * whose start position we recorded earlier in the same gesture. + * + * The second half is what catches React Flow's own drag-STOP change. That + * one must receive the identical correction, or the value the library + * settles on is not the value the user was just looking at - which is + * exactly the release-time jump the pre-refactor code worked around by + * substituting a position after the fact. + */ +function isGestureFrame( + change: NodeChange, + dragStarts: DragStartPositions, +): change is Extract, { type: "position" }> { + if (change.type !== "position" || !change.position) return false; + return change.dragging === true || dragStarts.has(change.id); +} + +/** + * Apply every drag correction to one batch of node changes. + * + * Non-position changes, and position changes outside an active gesture, + * pass through untouched - this function narrows to gesture frames only and + * has no opinion on anything else in the batch. + * + * Guides accumulate across every co-mover in the batch rather than being + * replaced per node. That is a deliberate departure from the Qt original, + * which cleared its guides per item and therefore only ever showed the + * last-processed item's alignment - an artifact of that toolkit's per-item + * callback ordering, not a design decision worth reproducing. + */ +export function correctDragChanges( + changes: NodeChange[], + context: DragCorrectionContext, +): DragCorrectionResult { + const { nodes, dragStarts, sizeCache, dragFactor, smartGuidesEnabled } = context; + const guides: GuideLine[] = []; + const memberChanges: NodeChange[] = []; + let touchedGesture = false; + + const corrected = changes.map((change) => { + if (!isGestureFrame(change, dragStarts)) return change; + touchedGesture = true; + + let start = dragStarts.get(change.id); + if (!start) { + const node = nodes.find((n) => n.id === change.id); + start = node ? { ...node.position } : { ...change.position! }; + dragStarts.set(change.id, start); + } + + // 1. Drag speed, measured from the gesture's own origin. + let position = scaleDragPosition(start, change.position!, dragFactor); + + // 2. Smart-guide snap, layered on top of React Flow's native grid snap + // (which, when enabled, already ran before this change was emitted). + // Guides win per axis where both apply, reproducing the Qt canvas's + // own per-axis priority. + if (smartGuidesEnabled) { + const snapped = context.computeSnap(nodes, change.id, position, sizeCache); + position = snapped.position; + guides.push(...snapped.guides); + } + + // 3. Group members ride the FINAL delta, so they stay in lockstep with + // the group rather than with the raw pointer. + memberChanges.push(...context.computeGroupCascade(nodes, change.id, position)); + + return { ...change, position }; + }); + + return { + changes: memberChanges.length > 0 ? [...corrected, ...memberChanges] : corrected, + guides, + touchedGesture, + }; +} + +/** + * The positions a settled gesture must persist, for the node that was + * released and for every group member it carried. + * + * Returned as one list because they have to be committed as a single + * batch: committing them one at a time makes the server publish a scene + * after each, and a group's bounds are derived from its members, so those + * intermediate publishes render as a group visibly stretching and + * resettling rather than merely being briefly stale. + */ +export function collectSettledPositions( + nodes: SceneFlowNode[], + releasedNodeId: string, + isGroup: (node: SceneFlowNode | undefined) => boolean, + membersOf: (nodes: SceneFlowNode[], group: SceneFlowNode) => Set, +): Array<{ id: string; x: number; y: number }> { + const settled = nodes.find((n) => n.id === releasedNodeId); + if (!settled) return []; + const positions = [{ id: releasedNodeId, x: settled.position.x, y: settled.position.y }]; + if (isGroup(settled)) { + for (const memberId of membersOf(nodes, settled)) { + const member = nodes.find((n) => n.id === memberId); + if (member) positions.push({ id: memberId, x: member.position.x, y: member.position.y }); + } + } + return positions; +}