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; +}