From 4dcf3b2a624e6a37981de5ededd18e53c29039e2 Mon Sep 17 00:00:00 2001 From: Vorflux AI Date: Thu, 26 Mar 2026 18:04:25 +0000 Subject: [PATCH] Fix review bugs: slideshow re-mount, edges cascade, stale popover, maxNodes - Fix slideshow useEffect: use nodesRef instead of nodes in deps to prevent interval teardown/recreation on every render - Fix edges useMemo: compute allNodeIds from normalizedDocs directly instead of depending on nodes (which changes identity every render) - Fix popover position: add zoomDisplay to deps so position updates on pan/zoom - Implement maxNodes prop: limit documents before passing to useGraphData - Fix SpatialIndex hash: include node IDs and use 10x position granularity to prevent false cache hits during physics simulation --- packages/memory-graph/src/canvas/hit-test.ts | 12 +++++-- .../src/components/memory-graph.tsx | 33 +++++++++++++------ .../memory-graph/src/hooks/use-graph-data.ts | 10 ++++-- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/packages/memory-graph/src/canvas/hit-test.ts b/packages/memory-graph/src/canvas/hit-test.ts index e71e4643..ac20bca7 100644 --- a/packages/memory-graph/src/canvas/hit-test.ts +++ b/packages/memory-graph/src/canvas/hit-test.ts @@ -58,8 +58,16 @@ export class SpatialIndex { private computeHash(nodes: GraphNode[]): number { let hash = nodes.length for (const n of nodes) { - hash = (hash * 31 + (Math.round(n.x) | 0)) | 0 - hash = (hash * 31 + (Math.round(n.y) | 0)) | 0 + // Use finer granularity (10x) to detect sub-pixel movements + // and incorporate a simple string hash of the ID to avoid + // false matches when nodes swap positions + let idHash = 0 + for (let i = 0; i < n.id.length; i++) { + idHash = ((idHash << 5) - idHash + n.id.charCodeAt(i)) | 0 + } + hash = (hash * 31 + idHash) | 0 + hash = (hash * 31 + (Math.round(n.x * 10) | 0)) | 0 + hash = (hash * 31 + (Math.round(n.y * 10) | 0)) | 0 } return hash } diff --git a/packages/memory-graph/src/components/memory-graph.tsx b/packages/memory-graph/src/components/memory-graph.tsx index 7af2cfab..3b382a76 100644 --- a/packages/memory-graph/src/components/memory-graph.tsx +++ b/packages/memory-graph/src/components/memory-graph.tsx @@ -27,6 +27,7 @@ export function MemoryGraph({ highlightDocumentIds = [], highlightsVisible = true, showFps = false, + maxNodes, isSlideshowActive = false, onSlideshowNodeChange, onSlideshowStop: _onSlideshowStop, @@ -51,8 +52,14 @@ export function MemoryGraph({ const [selectedNode, setSelectedNode] = useState(null) const [zoomDisplay, setZoomDisplay] = useState(50) + // Limit documents if maxNodes is set + const limitedDocuments = useMemo(() => { + if (!maxNodes || documents.length <= maxNodes) return documents + return documents.slice(0, maxNodes) + }, [documents, maxNodes]) + const { nodes, edges } = useGraphData( - documents, + limitedDocuments, apiEdges, null, containerSize.width, @@ -62,8 +69,8 @@ export function MemoryGraph({ // Rebuild version chain index when documents change useEffect(() => { - chainIndex.current.rebuild(documents) - }, [documents]) + chainIndex.current.rebuild(limitedDocuments) + }, [limitedDocuments]) // Smart simulation re-init: track node ID set, only init() when IDs change const prevSimIdsRef = useRef("") @@ -386,7 +393,10 @@ export function MemoryGraph({ return () => window.removeEventListener("keydown", handler) }, [navigateUp, navigateDown, navigateNext, navigatePrev]) - // Slideshow + // Slideshow — use a ref for nodes to avoid re-creating the interval on every render + const nodesRef = useRef(nodes) + nodesRef.current = nodes + useEffect(() => { if (!isSlideshowActive || nodes.length === 0) { if (!isSlideshowActive) { @@ -398,17 +408,18 @@ export function MemoryGraph({ let lastIdx = -1 const pick = () => { - if (nodes.length === 0) return + const currentNodes = nodesRef.current + if (currentNodes.length === 0) return let idx: number - if (nodes.length > 1) { + if (currentNodes.length > 1) { do { - idx = Math.floor(Math.random() * nodes.length) + idx = Math.floor(Math.random() * currentNodes.length) } while (idx === lastIdx) } else { idx = 0 } lastIdx = idx - const n = nodes[idx]! + const n = currentNodes[idx]! setSelectedNode(n.id) viewportRef.current?.centerOn( n.x, @@ -426,7 +437,7 @@ export function MemoryGraph({ return () => clearInterval(interval) }, [ isSlideshowActive, - nodes, + nodes.length, containerSize.width, containerSize.height, onSlideshowNodeChange, @@ -448,7 +459,9 @@ export function MemoryGraph({ screenY: screen.y, nodeRadius: (activeNodeData.size * vp.zoom) / 2, } - }, [activeNodeData]) + // zoomDisplay triggers re-computation on viewport changes (pan/zoom) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [activeNodeData, zoomDisplay]) const activeVersionChain = useMemo(() => { if (!activeNodeData || activeNodeData.type !== "memory") return null diff --git a/packages/memory-graph/src/hooks/use-graph-data.ts b/packages/memory-graph/src/hooks/use-graph-data.ts index 675c7601..ea436d6a 100644 --- a/packages/memory-graph/src/hooks/use-graph-data.ts +++ b/packages/memory-graph/src/hooks/use-graph-data.ts @@ -183,7 +183,13 @@ export function useGraphData( if (!normalizedDocs || normalizedDocs.length === 0) return [] const result: GraphEdge[] = [] - const allNodeIds = new Set(nodes.map((n) => n.id)) + // Build allNodeIds from normalizedDocs directly to avoid depending on `nodes` + // (which changes identity on every render due to draggingNodeId/colors deps) + const allNodeIds = new Set() + for (const doc of normalizedDocs) { + allNodeIds.add(doc.id) + for (const mem of doc.memories) allNodeIds.add(mem.id) + } for (const doc of normalizedDocs) { for (const mem of doc.memories) { @@ -229,7 +235,7 @@ export function useGraphData( } return result - }, [normalizedDocs, apiEdges, nodes]) + }, [normalizedDocs, apiEdges]) return { nodes, edges, scale, offsetX, offsetY } }