fix: memory graph hover highlight and click detection on Windows#491
Open
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Open
fix: memory graph hover highlight and click detection on Windows#491TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
Contributor
WalkthroughModified the MemoryGraph component to manage hover state using a ref instead of relying solely on React state. Added a synchronization effect that updates the ref and triggers Sigma refresh. Updated the container div styling from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The nodeReducer captured hoveredNode state in a closure at Sigma creation time, so it was always null — preventing fade/highlight effects. Fixed by using a ref that stays in sync with state. Also fixed the Sigma container positioning so its canvas properly receives pointer events above the stacking context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TheDarkSkyXD
force-pushed
the
fix/memory-graph-hover-click
branch
from
e6cff59 to
96d245c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/MemoryGraph.tsx`:
- Around line 281-285: The current useEffect syncing hoveredNode to
hoveredNodeRef lets stale hover IDs linger across graph rebuilds; add logic to
clear the hover state when the underlying graph/query changes by either
expanding this effect or adding a new effect that watches the query (or graph
data) prop and sets hoveredNode to null and hoveredNodeRef.current = null, then
calls sigmaRef.current?.refresh(); update references to hoveredNodeRef,
hoveredNode, setHoveredNode (or equivalent state setter), and sigmaRef to
implement this clearing so the new graph mounts without
pre-dimmed/pre-highlighted nodes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 727f15a8-3b94-42fd-a68a-d20609239d21
📒 Files selected for processing (1)
interface/src/components/MemoryGraph.tsx
Comment on lines
+281
to
285
| // Sync ref and re-render sigma when hoveredNode changes (for fade effect) | ||
| useEffect(() => { | ||
| hoveredNodeRef.current = hoveredNode; | ||
| sigmaRef.current?.refresh(); | ||
| }, [hoveredNode]); |
Contributor
There was a problem hiding this comment.
Clear hover state before rebuilding the graph.
If the query changes while a node is hovered, the old id survives in both state and hoveredNodeRef. The next graph can therefore mount already dimmed or pre-highlighted until another hover event fires.
🩹 Suggested fix
async function loadGraph() {
setIsLoading(true);
setError(null);
setSelectedNode(null);
+ setHoveredNode(null);
+ hoveredNodeRef.current = null;
cleanup();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/MemoryGraph.tsx` around lines 281 - 285, The current
useEffect syncing hoveredNode to hoveredNodeRef lets stale hover IDs linger
across graph rebuilds; add logic to clear the hover state when the underlying
graph/query changes by either expanding this effect or adding a new effect that
watches the query (or graph data) prop and sets hoveredNode to null and
hoveredNodeRef.current = null, then calls sigmaRef.current?.refresh(); update
references to hoveredNodeRef, hoveredNode, setHoveredNode (or equivalent state
setter), and sigmaRef to implement this clearing so the new graph mounts without
pre-dimmed/pre-highlighted nodes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nodeReducerthat prevented hover highlighting —hoveredNodewas captured at Sigma creation time and always read asnullhoveredNodeRefref that stays in sync with state, so the reducer reads the current value on each refreshhighlighted: true,zIndex: 1) on the hovered node itself, in addition to fading non-connected nodesabsolute inset-0 z-0) so the canvas properly receives pointer events above the stacking contextTest plan
🤖 Generated with Claude Code