[P1 correctness] Performance branch can restore stale positions after node drag #130

New issue

Open

opened 2026-07-28 22:59:20 +00:00 by lost-rob0t · 0 comments

lost-rob0t commented

2026-07-28 22:59:20 +00:00

(Migrated from github.com)

Finding

PR #124 correctly moves high-frequency drag writes out of React rendering, but the graph projection still reads positions from stale React state.

persistGraphPosition() updates workspaceRef.current with { render: false }, so workspace is intentionally not refreshed:

const persistGraphPosition = useCallback((id, position) => {
  const current = workspaceRef.current || {};
  const active = getActiveGraph(current);
  return commitWorkspace(
    updateActiveGraph(current, {
      positions: { ...(active.positions || {}), [id]: position }
    }),
    { render: false }
  );
}, [commitWorkspace]);

GraphPage tracks the new position in positionsRef, but never uses that ref when rebuilding the graph:

const graph = useMemo(
  () => buildGraph(graphDocuments, workspace?.positions || {}),
  [graphDocuments, workspace?.positions]
);

After a node is dragged, an unrelated document change, import, actor result, replication update, or review-filter change can rebuild graph using the old workspace.positions. The reconciler then sees an incoming position difference and moves the live Cytoscape node back to the stale position.

Impact

Dragged nodes can snap back after unrelated corpus updates.
Saved state and rendered state diverge until remount.
A renderer-tier remount can restore an old position or viewport.
The bug is timing-dependent and can be missed by isolated drag tests.

Required fix

Keep the latest active-graph positions in the non-React store, and ensure document-data reconciliation does not treat stale projection coordinates as authoritative for existing nodes.

Projection source

const positionsRef = useRef({});

useEffect(() => {
  positionsRef.current = activeGraph?.positions || {};
}, [activeGraph?.id, activeGraph?.positions]);

const graph = useMemo(
  () => buildGraph(graphDocuments, positionsRef.current),
  [graphDocuments, activeGraph?.id]
);

The drag callback continues updating the same ref before persistence:

const onMove = useCallback((id, position) => {
  positionsRef.current = {
    ...positionsRef.current,
    [id]: position
  };
  persistGraphPosition(id, position);
}, [persistGraphPosition]);

Reconciler guard

Only apply incoming positions to existing nodes when switching/restoring a graph, not when document content changes:

export function diffGraphElements(cy, graph, {
  applyIncomingPositions = false
} = {}) {
  // ...
  const positionChanged = Boolean(
    applyIncomingPositions
      && element.position
      && !samePosition(current.position(), element.position)
  );
}

reconcileGraphElements(cy, graph, {
  retainedNodes: retainedNodes.current,
  applyIncomingPositions: graphChanged
});

New nodes may still receive saved positions when added.

Acceptance criteria

Drag a node, edit its document title, and verify its rendered position does not change.
Drag a node, ingest/import another document, and verify the position does not change.
Switch graphs and return; the latest saved position is restored.
Unmount/remount restores the latest position and viewport.
Tests cover corpus refresh, filter changes, graph switching, and renderer-tier remounts after dragging.

## Finding PR #124 correctly moves high-frequency drag writes out of React rendering, but the graph projection still reads positions from stale React state. `persistGraphPosition()` updates `workspaceRef.current` with `{ render: false }`, so `workspace` is intentionally not refreshed: ```js const persistGraphPosition = useCallback((id, position) => { const current = workspaceRef.current || {}; const active = getActiveGraph(current); return commitWorkspace( updateActiveGraph(current, { positions: { ...(active.positions || {}), [id]: position } }), { render: false } ); }, [commitWorkspace]); ``` `GraphPage` tracks the new position in `positionsRef`, but never uses that ref when rebuilding the graph: ```js const graph = useMemo( () => buildGraph(graphDocuments, workspace?.positions || {}), [graphDocuments, workspace?.positions] ); ``` After a node is dragged, an unrelated document change, import, actor result, replication update, or review-filter change can rebuild `graph` using the old `workspace.positions`. The reconciler then sees an incoming position difference and moves the live Cytoscape node back to the stale position. ## Impact - Dragged nodes can snap back after unrelated corpus updates. - Saved state and rendered state diverge until remount. - A renderer-tier remount can restore an old position or viewport. - The bug is timing-dependent and can be missed by isolated drag tests. ## Required fix Keep the latest active-graph positions in the non-React store, and ensure document-data reconciliation does not treat stale projection coordinates as authoritative for existing nodes. ### Projection source ```js const positionsRef = useRef({}); useEffect(() => { positionsRef.current = activeGraph?.positions || {}; }, [activeGraph?.id, activeGraph?.positions]); const graph = useMemo( () => buildGraph(graphDocuments, positionsRef.current), [graphDocuments, activeGraph?.id] ); ``` The drag callback continues updating the same ref before persistence: ```js const onMove = useCallback((id, position) => { positionsRef.current = { ...positionsRef.current, [id]: position }; persistGraphPosition(id, position); }, [persistGraphPosition]); ``` ### Reconciler guard Only apply incoming positions to existing nodes when switching/restoring a graph, not when document content changes: ```js export function diffGraphElements(cy, graph, { applyIncomingPositions = false } = {}) { // ... const positionChanged = Boolean( applyIncomingPositions && element.position && !samePosition(current.position(), element.position) ); } ``` ```js reconcileGraphElements(cy, graph, { retainedNodes: retainedNodes.current, applyIncomingPositions: graphChanged }); ``` New nodes may still receive saved positions when added. ## Acceptance criteria - Drag a node, edit its document title, and verify its rendered position does not change. - Drag a node, ingest/import another document, and verify the position does not change. - Switch graphs and return; the latest saved position is restored. - Unmount/remount restores the latest position and viewport. - Tests cover corpus refresh, filter changes, graph switching, and renderer-tier remounts after dragging.