diff --git a/web_ui/src/app/canvas/SceneCanvas.tsx b/web_ui/src/app/canvas/SceneCanvas.tsx index bdb822e..0235e75 100644 --- a/web_ui/src/app/canvas/SceneCanvas.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.tsx @@ -47,7 +47,9 @@ import { import { handleKeyboardContextMenu } from "./keyboardContextMenu"; import { SceneStore, scaleDragPosition } from "./sceneStore"; import { computeSmartGuideSnap, type GuideLine, type Rect } from "./smartGuides"; -import { buildEdgeSyncPlan, syncEdgePaths, type EdgeSyncEntry } from "./drag/edgeSync"; +import { ConnectionCanvas, type ConnectionSpec } from "./connections/ConnectionCanvas"; +import type { ConnectionPath } from "./connections/connectionGeometry"; +import { isPointOnConnection } from "./connections/connectionGeometry"; import { useLodVisibility } from "./useLodVisibility"; /** @@ -148,6 +150,10 @@ const EDGE_TYPES = { // defaultNodes is read once when React Flow initialises its store; a stable // module constant keeps that unambiguous and allocation-free. const EMPTY_NODES: SceneFlowNode[] = []; +// React Flow renders no connections at all now - ConnectionCanvas draws +// every one of them. Handing it a stable empty array keeps its own edge +// machinery inert rather than merely invisible. +const EMPTY_EDGES: Edge[] = []; const DELETE_KEY_CODES = ["Delete", "Backspace"]; const PRO_OPTIONS = { hideAttribution: true }; const DEFAULT_EDGE_OPTIONS = { type: "default" as const }; @@ -2280,23 +2286,7 @@ function CanvasInner({ // plain ref (not state) since a rebuild must never itself trigger a // re-render - it only matters to onNodesChange's own closure. const dragSizeCacheRef = useRef>(new Map()); - // The connections this gesture must keep in step, resolved once when it - // starts - see drag/edgeSync.ts for why they are written synchronously. - const edgeSyncPlanRef = useRef([]); - // The latest corrected position of every node this gesture is moving, and - // the animation-frame loop that keeps their connections drawn from it. - // - // Writing the paths once per pointer event is not sufficient on its own: - // React Flow re-renders the same edges from its own node records a moment - // later, and if those records are even one frame behind the gesture, that - // render overwrites the correct path with a stale one - which is exactly - // the reported symptom, a line drawn where the node used to be, with the - // gap growing the faster the node moves and closing when it stops. A - // frame loop makes the correct geometry the LAST write before every - // paint, so whatever React renders in between cannot be what the user - // ends up seeing. The loop exists only for the duration of a gesture. - const dragPositionsRef = useRef>(new Map()); - const edgeSyncFrameRef = useRef(null); + // ADR-011 stage 11.1: ONE ToFlowNodesCache for this canvas's whole // lifetime, threaded into every toFlowNodes call below - this is what // actually makes the per-node dispatcher/whole-flow-node memoization in @@ -2466,39 +2456,49 @@ function CanvasInner({ return () => document.removeEventListener("keydown", handleKeyboardContextMenu); }, []); - // A canvas unmounted mid-gesture must not leave its frame loop running. - useEffect( - () => () => { - if (edgeSyncFrameRef.current !== null) cancelAnimationFrame(edgeSyncFrameRef.current); - }, - [], - ); - - // ADR-011 stage 11.3 (P4): toFlowEdges rebuilds the WHOLE edges array (an - // O(E) map over every edge) - hoveredEdgeId is only EVER read inside that - // rebuild when scene.fadeConnectionsEnabled is on (see toFlowEdges' own - // body), so unconditionally listing it as a dependency meant hovering an - // edge rebuilt this array on every enter/leave EVEN WHEN the fade feature - // is off and nothing in the output could possibly change. Passing `null` - // in place of hoveredEdgeId whenever fade is off collapses the dependency - // to a constant for that case - a hover-only state change no longer - // differs from the previous render's dependency, so useMemo correctly - // skips the recompute (and keeps returning the SAME array reference) - // instead of only skipping the WORK toFlowEdges would have done with it. - // Hoisted into its own variable (not an inline ternary in the deps array - // below) so the memo's dependency is a single, staticly-checkable - // reference - satisfies react-hooks/exhaustive-deps outright rather than - // suppressing it, and reads the same either way: null whenever fade is - // off, hoveredEdgeId whenever it's on. - const edgeHoverKey = scene.fadeConnectionsEnabled ? hoveredEdgeId : null; - const edges = useMemo(() => toFlowEdges(scene, edgeHoverKey), [scene, edgeHoverKey]); + // Hover no longer feeds the edge model at all: ConnectionCanvas applies + // the faded-connections lens while drawing, from its own hoveredId prop. + // That leaves this derivation dependent only on the scene, so hovering a + // connection cannot rebuild it. + const edges = useMemo(() => toFlowEdges(scene, null), [scene]); // Mirror of the rendered edges, read by the drag pipeline when it resolves // which connections a gesture must keep in step. A ref rather than a // dependency so the middleware is not re-registered whenever edges change. - const edgesRef = useRef(edges); - useEffect(() => { - edgesRef.current = edges; - }, [edges]); + // What ConnectionCanvas draws. Derived from the same edge model as before, + // reduced to what drawing needs; positions are read live from the flow + // store each frame rather than carried on these objects, which is the + // whole point of the canvas approach. + const connections = useMemo( + () => edges.map((e) => ({ id: e.id, source: e.source, target: e.target, orthogonal: e.type === "orthogonal" })), + [edges], + ); + // The geometry the canvas last drew, reported back so hit-testing runs + // against exactly what is on screen instead of a second computation that + // could disagree with it. + const connectionGeometryRef = useRef>(new Map()); + const onConnectionGeometry = useCallback((paths: Map) => { + connectionGeometryRef.current = paths; + }, []); + const [selectedConnectionId, setSelectedConnectionId] = useState(null); + const connectionStroke = useCssVar("--gl-surface-border-strong", "#505050"); + const connectionSelectedStroke = useCssVar("--gl-surface-text-primary", "#E0E0E0"); + + // Pointer interaction for connections. The canvas is presentational and + // never receives events itself, so hover and selection are resolved here + // by testing the pointer against the geometry the canvas reported. + const connectionAt = useCallback( + (clientX: number, clientY: number): string | null => { + const point = reactFlow.screenToFlowPosition({ x: clientX, y: clientY }); + const zoom = storeApi.getState().transform[2] || 1; + // A constant on-screen grab distance, expressed in flow units. + const tolerance = 8 / zoom; + for (const [id, path] of connectionGeometryRef.current) { + if (isPointOnConnection(path, point, tolerance)) return id; + } + return null; + }, + [reactFlow, storeApi], + ); // R8a: the minimap used to render every node as React Flow's own default // plain rectangle (no nodeColor/nodeStrokeColor was ever passed), which @@ -2584,23 +2584,6 @@ function CanvasInner({ if (startingGesture && scene.smartGuides) { dragSizeCacheRef.current = buildDragSizeCache(reactFlow, currentNodes); } - if (startingGesture) { - // Every node this gesture will move: the dragged nodes plus, for a - // group, everything it carries - so an edge attached to a carried - // member is redrawn too, not just the ones touching the group node. - const movingIds = new Set(); - for (const c of changes) { - if (c.type !== "position" || !c.dragging) continue; - movingIds.add(c.id); - const node = currentNodes.find((n) => n.id === c.id); - if (node && groupDragKindOf(node)) { - for (const memberId of collectTransitiveMemberIds(currentNodes, node)) movingIds.add(memberId); - } - } - edgeSyncPlanRef.current = buildEdgeSyncPlan(edgesRef.current, movingIds, (id) => - storeApi.getState().nodeLookup.get(id), - ); - } const memberChanges: NodeChange[] = []; // R7.5b-3: guides re-derive every drag frame. DELIBERATE deviation for // multi-select drags (review-confirmed): legacy cleared guides @@ -2662,36 +2645,6 @@ function CanvasInner({ if (!sawGestureFrame) return changes; pendingGuidesRef.current = frameGuides; - // Write the affected connection paths NOW, inside the pointer event - // that produced these positions, so the line and the card it is - // attached to reach the screen in the same frame. React Flow renders - // the same edges from its own state immediately afterwards and - // computes the identical shape; this write only ensures the correct - // shape is already in the DOM for the frame being painted. See - // drag/edgeSync.ts for the full reasoning. - if (edgeSyncPlanRef.current.length > 0) { - const movedPositions = dragPositionsRef.current; - for (const c of [...corrected, ...memberChanges]) { - if (c.type === "position" && c.position) movedPositions.set(c.id, c.position); - } - const getInternal = (id: string) => storeApi.getState().nodeLookup.get(id); - // Immediately, for this event's own frame... - syncEdgePaths(edgeSyncPlanRef.current, movedPositions, getInternal); - // ...and again before every subsequent paint until the gesture ends, - // so a later render from stale records cannot leave a stale path on - // screen. See dragPositionsRef's comment above. - if (edgeSyncFrameRef.current === null) { - const tick = () => { - if (edgeSyncPlanRef.current.length === 0) { - edgeSyncFrameRef.current = null; - return; - } - syncEdgePaths(edgeSyncPlanRef.current, dragPositionsRef.current, getInternal); - edgeSyncFrameRef.current = requestAnimationFrame(tick); - }; - edgeSyncFrameRef.current = requestAnimationFrame(tick); - } - } // Group members ride in the SAME batch as the node that carries them, // so React Flow commits the group and its members together. return memberChanges.length > 0 ? [...corrected, ...memberChanges] : corrected; @@ -2774,13 +2727,6 @@ function CanvasInner({ setSmartGuideLines((current) => (current.length === 0 && frameGuides.length === 0 ? current : frameGuides)); } else if (sawDragEnd) { pendingGuidesRef.current = []; - // Gesture over: React Flow owns the edges again until the next one. - edgeSyncPlanRef.current = []; - dragPositionsRef.current = new Map(); - if (edgeSyncFrameRef.current !== null) { - cancelAnimationFrame(edgeSyncFrameRef.current); - edgeSyncFrameRef.current = null; - } setSmartGuideLines((current) => (current.length === 0 ? current : [])); } // Suspend off-viewport culling while a drag is in flight - see @@ -2842,11 +2788,66 @@ function CanvasInner({ ({ nodes: sel }: { nodes: { id: string }[] }) => handleSelectionChange(store, sel), [store], ); - const onEdgeMouseEnter = useCallback((_event: React.MouseEvent, edge: Edge) => setHoveredEdgeId(edge.id), []); - const onEdgeMouseLeave = useCallback(() => setHoveredEdgeId(null), []); const snapGrid = useMemo<[number, number]>(() => [grid.gridSize, grid.gridSize], [grid.gridSize]); const { screenToFlowPosition } = reactFlow; + // Hover is only meaningful while the fade-connections lens is on; testing + // otherwise would cost a pointer-move hit test for no visible effect. + const onCanvasMouseMove = useCallback( + (event: MouseEvent) => { + if (!scene.fadeConnectionsEnabled) return; + if (draggingRef.current) return; + const id = connectionAt(event.clientX, event.clientY); + setHoveredEdgeId((current) => (current === id ? current : id)); + }, + [connectionAt, scene.fadeConnectionsEnabled], + ); + + // Selecting a connection: only when the press did not land on a node card, + // so this can never steal a drag from a node. + const onCanvasMouseDown = useCallback( + (event: MouseEvent) => { + if ((event.target as HTMLElement).closest(".react-flow__node")) return; + const id = connectionAt(event.clientX, event.clientY); + setSelectedConnectionId(id); + }, + [connectionAt], + ); + + // Delete removes the selected connection. React Flow used to report edge + // deletions through onDelete; it no longer renders connections, so this + // owns that gesture now. + useEffect(() => { + const onKeyDown = (event: KeyboardEvent) => { + if (event.key !== "Delete" && event.key !== "Backspace") return; + if (selectedConnectionId === null) return; + const target = event.target as HTMLElement | null; + // Never while typing. + if (target && (target.isContentEditable || /^(INPUT|TEXTAREA|SELECT)$/.test(target.tagName))) return; + store.removeEdges([selectedConnectionId]); + setSelectedConnectionId(null); + }; + document.addEventListener("keydown", onKeyDown); + return () => document.removeEventListener("keydown", onKeyDown); + }, [selectedConnectionId, store]); + + // Attached to the wrapper element directly rather than through JSX props: + // these are pointer affordances on a drawing surface, not interactions on + // a semantic control, and binding them here keeps the element free of + // handler props that would misrepresent it to assistive technology. + useEffect(() => { + const el = canvasWrapperRef.current; + if (!el) return; + const move = (event: MouseEvent) => onCanvasMouseMove(event); + const down = (event: MouseEvent) => onCanvasMouseDown(event); + el.addEventListener("mousemove", move); + el.addEventListener("mousedown", down); + return () => { + el.removeEventListener("mousemove", move); + el.removeEventListener("mousedown", down); + }; + }, [onCanvasMouseMove, onCanvasMouseDown]); + const onDoubleClick = useCallback( (event: React.MouseEvent) => { // Double-click on empty canvas creates a node there - the R1 stand-in @@ -2886,7 +2887,7 @@ function CanvasInner({ 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} + edges={EMPTY_EDGES} nodeTypes={NODE_TYPES} edgeTypes={EDGE_TYPES} onNodesChange={onNodesChange} @@ -2897,8 +2898,6 @@ function CanvasInner({ // PluginPicker can attach "which node was selected" to executePlugin // without either component reaching into the other's internals. onSelectionChange={onSelectionChange} - onEdgeMouseEnter={onEdgeMouseEnter} - onEdgeMouseLeave={onEdgeMouseLeave} snapToGrid={scene.snapToGrid} snapGrid={snapGrid} // Double-click is the R1 create-node gesture (wrapper onDoubleClick); @@ -2950,6 +2949,18 @@ function CanvasInner({ */ onlyRenderVisibleElements={!exportInProgress && !dragActive} > + {/* Every connection on the scene is drawn here, redrawn each frame + from live node positions - see ConnectionCanvas's module doc. */} + wiring (ADR-011 stages 11.2/11.3)", () => { }); describe("edge-hover memo gate (stage 11.3, P4)", () => { - it("hovering an edge does NOT change the edges array reference when fadeConnectionsEnabled is false", () => { + it("hands React Flow no edges at all - connections are drawn by ConnectionCanvas", () => { + // The canvas owns connection rendering now (see ConnectionCanvas's own + // module doc). React Flow's edge machinery is left inert rather than + // merely invisible, which is what removes it from the drag path. const { stateListeners } = mount(); publish(stateListeners, { nodes: [chatRow("a", 0), chatRow("b", 200)], edges: [{ id: "e1", source: "a", target: "b" }], - fadeConnectionsEnabled: false, }); - const before = lastProps().edges; - - act(() => lastProps().onEdgeMouseEnter(undefined, { id: "e1" } as Edge)); - expect(lastProps().edges).toBe(before); - - act(() => lastProps().onEdgeMouseLeave()); - expect(lastProps().edges).toBe(before); + expect(lastProps().edges).toEqual([]); }); - it("hovering an edge DOES rebuild the edges array (and applies fade styling) when fadeConnectionsEnabled is true", () => { + it("keeps the edge model stable across a hover, since fading is applied while drawing", () => { + // Hover used to rebuild the whole edge array to restyle one edge. The + // canvas applies the faded-connections lens itself, so the model is a + // function of the scene alone and a hover cannot churn it. const { stateListeners } = mount(); - publish(stateListeners, { - nodes: [chatRow("a", 0), chatRow("b", 200), chatRow("c", 400)], - edges: [ - { id: "e1", source: "a", target: "b" }, - { id: "e2", source: "a", target: "c" }, - ], + const scene = { + nodes: [chatRow("a", 0), chatRow("b", 200)], + edges: [{ id: "e1", source: "a", target: "b" }], fadeConnectionsEnabled: true, - }); + }; + publish(stateListeners, scene); const before = lastProps().edges; - - act(() => lastProps().onEdgeMouseEnter(undefined, { id: "e1" } as Edge)); - const hovered = lastProps().edges; - expect(hovered).not.toBe(before); - expect(hovered.find((e) => e.id === "e1")?.style).toBeUndefined(); - expect(hovered.find((e) => e.id === "e2")?.style).toEqual({ opacity: 0.08 }); + publish(stateListeners, scene); + expect(lastProps().edges).toBe(before); }); }); diff --git a/web_ui/src/app/canvas/connections/ConnectionCanvas.tsx b/web_ui/src/app/canvas/connections/ConnectionCanvas.tsx new file mode 100644 index 0000000..51640e1 --- /dev/null +++ b/web_ui/src/app/canvas/connections/ConnectionCanvas.tsx @@ -0,0 +1,228 @@ +/** + * The canvas that draws every connection on the scene. + * + * WHY THIS REPLACES THE PREVIOUS APPROACH. Connections used to be rendered + * by the flow library as SVG elements, one per link, positioned from the + * library's own node records and reconciled by React. During a drag that + * arrangement put the node card and the line attached to it on two + * different update paths, and the line's path was observed on screen still + * drawn for a position the node had already left - a gap that grew with + * pointer speed and closed the moment movement stopped. Several attempts to + * make those two paths agree (correcting positions earlier, moving state + * ownership, writing the SVG path imperatively, re-writing it every frame) + * all failed to change what the user saw. + * + * This removes the disagreement instead of trying to synchronise it. There + * is now exactly one thing that draws connections: this component. Every + * frame it reads the CURRENT node positions and the CURRENT viewport + * transform and redraws, in immediate mode. There is no per-link element, + * no retained scene graph, and nothing that React can reconcile a frame + * later - so a link cannot be drawn from a position that is out of date, + * because there is no stored position to be out of date. This is the shape + * long-standing node editors use for exactly this reason. + * + * The component draws only. Interaction (hover, selection) is hit-tested + * against the same geometry by the canvas's owner, using + * connectionGeometry's isPointOnConnection, so what is drawn and what is + * clickable can never disagree either. + */ + +import { useCallback, useEffect, useRef } from "react"; +import { Position, useStoreApi } from "@xyflow/react"; +import { + anchorPoint, + connectionPath, + traceConnection, + type ConnectionPath, + type HandleGeometry, + type HandleSide, +} from "./connectionGeometry"; + +/** One link to draw, in the terms this canvas needs. */ +export interface ConnectionSpec { + id: string; + source: string; + target: string; + orthogonal: boolean; +} + +export interface ConnectionCanvasProps { + connections: readonly ConnectionSpec[]; + /** Link currently under the pointer, exempt from fading. */ + hoveredId: string | null; + /** Link currently selected, drawn emphasised. */ + selectedId: string | null; + /** Dim every link except the hovered one. */ + fadeEnabled: boolean; + /** Resting stroke colour, supplied by the theme. */ + stroke: string; + /** Stroke colour for the selected link. */ + selectedStroke: string; + /** Opacity applied to non-hovered links while fading is on. */ + fadedOpacity: number; + /** + * Receives the geometry this canvas last drew, so its owner can hit-test + * against exactly what is on screen rather than recomputing it. + */ + onGeometry?: (paths: Map) => void; +} + +/** Maps the flow library's handle position onto this module's own vocabulary. */ +function sideOf(position: Position | undefined): HandleSide { + switch (position) { + case Position.Top: + return "top"; + case Position.Right: + return "right"; + case Position.Left: + return "left"; + case Position.Bottom: + default: + return "bottom"; + } +} + +interface HandleBoundLike { + x: number; + y: number; + width: number; + height: number; + position: Position; +} + +function toHandleGeometry(bound: HandleBoundLike): HandleGeometry { + return { x: bound.x, y: bound.y, width: bound.width, height: bound.height, side: sideOf(bound.position) }; +} + +export function ConnectionCanvas({ + connections, + hoveredId, + selectedId, + fadeEnabled, + stroke, + selectedStroke, + fadedOpacity, + onGeometry, +}: ConnectionCanvasProps) { + const canvasRef = useRef(null); + const storeApi = useStoreApi(); + const frameRef = useRef(null); + // The draw inputs are read inside the frame loop rather than captured in + // its closure, so the loop never has to be torn down and rebuilt when a + // colour or a hover changes. + const propsRef = useRef({ connections, hoveredId, selectedId, fadeEnabled, stroke, selectedStroke, fadedOpacity, onGeometry }); + useEffect(() => { + propsRef.current = { connections, hoveredId, selectedId, fadeEnabled, stroke, selectedStroke, fadedOpacity, onGeometry }; + }, [connections, hoveredId, selectedId, fadeEnabled, stroke, selectedStroke, fadedOpacity, onGeometry]); + + const draw = useCallback((): boolean => { + const canvas = canvasRef.current; + if (!canvas) return true; + // A null context means this environment has no 2D canvas at all (jsdom + // under test, for one). Report it so the frame loop stops rather than + // asking again sixty times a second for the lifetime of the component. + const ctx = canvas.getContext("2d"); + if (!ctx) return false; + const state = storeApi.getState(); + const { width, height, transform, nodeLookup } = state; + if (!width || !height) return true; + + // Match the backing store to the display size and pixel density, so + // lines stay crisp on high-density screens and after a window resize. + const ratio = window.devicePixelRatio || 1; + const backingWidth = Math.round(width * ratio); + const backingHeight = Math.round(height * ratio); + if (canvas.width !== backingWidth || canvas.height !== backingHeight) { + canvas.width = backingWidth; + canvas.height = backingHeight; + } + canvas.style.width = `${width}px`; + canvas.style.height = `${height}px`; + + ctx.setTransform(ratio, 0, 0, ratio, 0, 0); + ctx.clearRect(0, 0, width, height); + + const [panX, panY, zoom] = transform; + // Draw in screen space: the viewport transform is applied here rather + // than by scaling the canvas itself, which keeps strokes a constant + // width on screen and free of scaling artefacts at any zoom. + ctx.setTransform(ratio * zoom, 0, 0, ratio * zoom, ratio * panX, ratio * panY); + ctx.lineWidth = 1.5 / zoom; + ctx.lineCap = "round"; + + const { + connections: specs, + hoveredId: hovered, + selectedId: selected, + fadeEnabled: fade, + stroke: baseStroke, + selectedStroke: activeStroke, + fadedOpacity: dimmed, + onGeometry: report, + } = propsRef.current; + + const geometry = new Map(); + for (const spec of specs) { + const sourceNode = nodeLookup.get(spec.source); + const targetNode = nodeLookup.get(spec.target); + if (!sourceNode || !targetNode) continue; + const sourceBound = sourceNode.internals.handleBounds?.source?.[0]; + const targetBound = targetNode.internals.handleBounds?.target?.[0]; + if (!sourceBound || !targetBound) continue; + + const sourceHandle = toHandleGeometry(sourceBound as HandleBoundLike); + const targetHandle = toHandleGeometry(targetBound as HandleBoundLike); + const from = anchorPoint( + sourceNode.internals.positionAbsolute.x, + sourceNode.internals.positionAbsolute.y, + sourceHandle, + ); + const to = anchorPoint( + targetNode.internals.positionAbsolute.x, + targetNode.internals.positionAbsolute.y, + targetHandle, + ); + const path = connectionPath(from, sourceHandle.side, to, targetHandle.side, spec.orthogonal); + geometry.set(spec.id, path); + + const isSelected = spec.id === selected; + ctx.globalAlpha = fade && spec.id !== hovered ? dimmed : 1; + ctx.strokeStyle = isSelected ? activeStroke : baseStroke; + ctx.lineWidth = (isSelected ? 2.5 : 1.5) / zoom; + traceConnection(ctx, path); + } + ctx.globalAlpha = 1; + report?.(geometry); + return true; + }, [storeApi]); + + useEffect(() => { + // Redrawing every frame is deliberate. It is what guarantees a link can + // never be shown at a position the node has already left, and the cost + // is a few dozen curves - far less than reconciling an element per link. + const tick = () => { + if (!draw()) { + frameRef.current = null; + return; + } + frameRef.current = requestAnimationFrame(tick); + }; + frameRef.current = requestAnimationFrame(tick); + return () => { + if (frameRef.current !== null) cancelAnimationFrame(frameRef.current); + frameRef.current = null; + }; + }, [draw]); + + return ( +