mirror of
https://github.com/supermemoryai/supermemory.git
synced 2026-10-21 05:18:00 +00:00
fix(memory-graph): stop hit-testing against stale nodes after the array regenerates
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.
This commit is contained in:
parent
2cebe81512
commit
714d273ba5
4 changed files with 54 additions and 13 deletions
|
|
@ -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)]
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
|
|
|||
|
|
@ -94,12 +94,16 @@ export const GraphCanvas = memo<ExtendedGraphCanvasProps>(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])
|
||||
|
||||
|
|
|
|||
|
|
@ -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<GraphThemeColors>(
|
||||
() =>
|
||||
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<DOMRect | null>(null)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue