From 714d273ba52b9bd90fc41127cc5d3438b63d5808 Mon Sep 17 00:00:00 2001 From: abhay-codes07 Date: Mon, 13 Jul 2026 14:58:32 +0530 Subject: [PATCH] fix(memory-graph): stop hit-testing against stale nodes after the array regenerates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpatialIndex.rebuild() early-returns on an unchanged content hash, and the hash only covers node ids and rounded positions. useGraphData produces a brand-new generation of node objects whenever its memo deps change — including on container resize and light/dark theme switches — with identical ids and coordinates. The GraphCanvas [nodes] effect then called rebuild(), the hash matched, and the grid kept serving the previous generation of objects while the renderer and nodeMap moved on. The first drag after any resize or theme toggle therefore grabbed a detached object: onMouseDown pinned fx/fy on a node the renderer no longer draws, the visible node ignored the cursor, and the drag-start reheat() let it drift freely. Hover and click were unaffected (they only use node.id), which is what kept this hidden. Give rebuild() a force parameter and pass it from the identity-keyed effect, keeping the cheap hash guard for the per-frame rebuilds in the rAF loop. Covered with SpatialIndex tests that pin both behaviours: the unforced hash guard keeps the old generation (documenting why force exists), and a forced rebuild swaps the grid to the new objects and refreshes the hash. Also remove the colors re-merge in MemoryGraph: useGraphTheme already returns the overrides merged and referentially stable by value, and re-merging keyed on the raw prop identity made `colors` a fresh object every render for inline `colors={{...}}` consumers — rebuilding the entire node array each render and multiplying how often the stale-index window occurred. --- .../src/__tests__/spatial-index.test.ts | 30 +++++++++++++++++++ packages/memory-graph/src/canvas/hit-test.ts | 14 +++++++-- .../src/components/graph-canvas.tsx | 8 +++-- .../src/components/memory-graph.tsx | 15 ++++------ 4 files changed, 54 insertions(+), 13 deletions(-) diff --git a/packages/memory-graph/src/__tests__/spatial-index.test.ts b/packages/memory-graph/src/__tests__/spatial-index.test.ts index 46d7beabe..29cba50d8 100644 --- a/packages/memory-graph/src/__tests__/spatial-index.test.ts +++ b/packages/memory-graph/src/__tests__/spatial-index.test.ts @@ -54,6 +54,36 @@ describe("SpatialIndex", () => { expect(result).toBe(true) }) + it("hash guard keeps stale objects for a new generation at the same positions", () => { + // Documents why force exists: a regenerated node array (resize, + // theme change) carries identical ids/positions, so the content + // hash cannot see it. + const idx = new SpatialIndex() + const genA = [makeNode("a", 100, 100)] + idx.rebuild(genA) + + const genB = [makeNode("a", 100, 100)] + expect(idx.rebuild(genB)).toBe(false) + expect(idx.queryPoint(100, 100)).toBe(genA[0]) + }) + + it("force rebuild swaps the grid to the new node generation", () => { + const idx = new SpatialIndex() + const genA = [makeNode("a", 100, 100)] + idx.rebuild(genA) + + const genB = [makeNode("a", 100, 100)] + expect(idx.rebuild(genB, true)).toBe(true) + expect(idx.queryPoint(100, 100)).toBe(genB[0]) + }) + + it("force rebuild still refreshes the hash for subsequent unforced calls", () => { + const idx = new SpatialIndex() + const nodes = [makeNode("a", 100, 100)] + idx.rebuild(nodes, true) + expect(idx.rebuild(nodes)).toBe(false) + }) + it("rebuild detects sub-pixel movements (10x granularity)", () => { const idx = new SpatialIndex() const nodes = [makeNode("a", 100.0, 100.0)] diff --git a/packages/memory-graph/src/canvas/hit-test.ts b/packages/memory-graph/src/canvas/hit-test.ts index ac20bca72..04af3295b 100644 --- a/packages/memory-graph/src/canvas/hit-test.ts +++ b/packages/memory-graph/src/canvas/hit-test.ts @@ -5,9 +5,19 @@ export class SpatialIndex { private cellSize = 200 private lastHash = 0 - rebuild(nodes: GraphNode[]): boolean { + /** + * Rebuild the grid from the given nodes. + * + * The content hash only covers node ids and positions, so it cannot + * detect a new generation of node objects at the same coordinates + * (e.g. after useGraphData re-runs on resize or theme change). Callers + * reacting to a node-array identity change must pass force=true, or + * hit-testing keeps returning stale objects the renderer no longer + * draws. + */ + rebuild(nodes: GraphNode[], force = false): boolean { const hash = this.computeHash(nodes) - if (hash === this.lastHash) return false + if (!force && hash === this.lastHash) return false this.lastHash = hash this.grid.clear() diff --git a/packages/memory-graph/src/components/graph-canvas.tsx b/packages/memory-graph/src/components/graph-canvas.tsx index c5aea9101..b645cedef 100644 --- a/packages/memory-graph/src/components/graph-canvas.tsx +++ b/packages/memory-graph/src/components/graph-canvas.tsx @@ -94,12 +94,16 @@ export const GraphCanvas = memo(function GraphCanvas({ simulation, } - // Rebuild nodeMap + spatial index when nodes change + // Rebuild nodeMap + spatial index when nodes change. Force the spatial + // rebuild: a new node generation can carry identical ids/positions + // (resize, theme change), which the content hash cannot distinguish, + // and stale objects in the grid break dragging — the hit-test returns + // a node the renderer no longer draws. useEffect(() => { const map = nodeMapRef.current map.clear() for (const n of nodes) map.set(n.id, n) - spatialRef.current.rebuild(nodes) + spatialRef.current.rebuild(nodes, true) renderNeeded.current = true }, [nodes]) diff --git a/packages/memory-graph/src/components/memory-graph.tsx b/packages/memory-graph/src/components/memory-graph.tsx index 9d0c2ef65..00124fb8d 100644 --- a/packages/memory-graph/src/components/memory-graph.tsx +++ b/packages/memory-graph/src/components/memory-graph.tsx @@ -10,7 +10,6 @@ import { useGraphData } from "../hooks/use-graph-data" import { useGraphTheme } from "../hooks/use-graph-theme" import type { GraphApiDocument, - GraphThemeColors, MemoryGraphProps, ResolvedMemoryGraphLabels, } from "../types" @@ -48,14 +47,12 @@ export function MemoryGraph({ ) const hoverPopoverZIndex = layering?.hoverPopoverZIndex ?? DEFAULT_HOVER_POPOVER_Z_INDEX - const resolvedColors = useGraphTheme(colorOverrides) - const colors = useMemo( - () => - colorOverrides - ? { ...resolvedColors, ...colorOverrides } - : resolvedColors, - [resolvedColors, colorOverrides], - ) + // useGraphTheme already merges the overrides and keeps the result + // referentially stable while the override values are unchanged. + // Re-merging here keyed on the raw prop identity made `colors` a new + // object on every render for inline `colors={{...}}` consumers, which + // rebuilt the entire node array (useGraphData dependency) each render. + const colors = useGraphTheme(colorOverrides) const [containerSize, setContainerSize] = useState({ width: 0, height: 0 }) const [containerBounds, setContainerBounds] = useState(null)