graph visuals update

[pedramamini]

No description provided.

[claude]

Code Review - PR #127: Graph Visuals Update

This is an impressive refactor that replaces the physics-based force-directed graph with a deterministic canvas-based mind map. The implementation is well-structured and follows the codebase conventions. Here's my detailed review:

✅ Strengths

1. Architecture & Design

• Clean separation of concerns: The new MindMap.tsx (1,141 lines) is well-organized with clear sections: types, constants, utilities, layout algorithm, and rendering
• Deterministic layout: The BFS-based layout algorithm provides predictable, reproducible positioning - a major UX improvement over physics simulations
• Performance: Canvas-based rendering is the right choice for graph visualization, avoiding DOM overhead
• Type safety: Comprehensive TypeScript interfaces (MindMapNode, MindMapLink, MindMapProps)

2. Code Quality

• Excellent documentation: Every function has clear JSDoc comments explaining purpose and behavior
• Consistent naming: Uses standard graph terminology (nodes, edges/links, depth, BFS)
• Constants organization: Well-defined layout constants (spacing, sizes, offsets) at the top make tuning easy
• Test updates: GraphLegend.test.tsx properly updated (89 additions, 70 deletions) to match new mind map styling

3. Accessibility & UX

• Keyboard navigation: Comprehensive arrow key navigation with spatial awareness (lines 922-1034)
• Visual feedback: Hover states, selection highlighting, and search dimming
• Accessibility: Proper tabIndex, ARIA labels updated in tests
• Progressive disclosure: Depth slider for focused ego-network views

4. Integration

• Clean removal: Properly removed react-force-graph-2d dependency (16 packages removed from lock file)
• Backwards compatibility: No breaking changes to external APIs
• Settings cleanup: Removed obsolete documentGraphLayoutMode setting throughout the stack

⚠️ Issues & Concerns

1. Security: Potential XSS in External Link Handling ⚠️
In MindMap.tsx:991:

window.open(focusedNode.urls?.[0], '_blank');

Issue: No validation of URL scheme before opening. Malicious markdown could use javascript: URLs.
Fix: Add URL validation:

const url = focusedNode.urls?.[0];
if (url && (url.startsWith('http://') || url.startsWith('https://'))) {
  window.open(url, '_blank');
}

2. Performance: Inefficient Node Searching 🐌
Multiple places use find() in event handlers (lines 827, 866, 891):

const node = findNodeAtPoint(x, y);
// findNodeAtPoint does: nodesWithState.find(n => ...)

Issue: O(n) search on every mouse move event
Suggestion: Consider spatial indexing (quadtree) if node count grows beyond ~100 nodes, or at minimum cache the result when mouse position hasn't changed

3. Code Duplication: Left/Right Column Logic
Lines 360-394 have nearly identical logic for left and right columns:

// Calculate positions for left column
const leftX = centerX - (HORIZONTAL_SPACING * depth);
const leftTotalHeight = leftNodes.length * VERTICAL_SPACING;
// ... repeat for right

Suggestion: Extract to a positionColumn() helper function to reduce duplication

4. Magic Numbers
Line 909: const delta = -e.deltaY * 0.001;
Line 225: const controlOffset = Math.min(dx * 0.5, 100);
Line 942: const leftColumn = nodesWithState.filter(n => n.x < focusedNode.x - 50);

Suggestion: Define as named constants (e.g., ZOOM_SENSITIVITY, BEZIER_CONTROL_FACTOR, COLUMN_DETECTION_THRESHOLD)

5. Missing Error Boundary
MindMap.tsx is a complex component with canvas operations but lacks error handling for:

• Canvas context failures (getContext('2d') can return null)
• Layout calculation errors (empty nodes array, missing center node)

Suggestion: Add error boundaries or null checks, especially around canvas operations

6. Test Coverage Gaps
While GraphLegend.test.tsx was updated, I don't see tests for:

• MindMap.tsx core functionality (layout algorithm, keyboard navigation, mouse interactions)
• calculateMindMapLayout() function
• convertToMindMapData() utility

Recommendation: Add unit tests for the layout algorithm and integration tests for user interactions

7. Accessibility: Keyboard Focus Indication
The keyboard navigation is well-implemented, but there's no visual focus ring for keyboard users when navigating with arrow keys. The focused node is only marked internally (focusedNodeId state).

Suggestion: Add a distinct visual indicator (e.g., dotted outline) for keyboard-focused nodes, separate from mouse-selected nodes

📝 Minor Issues

1. Type Strictness
Line 294: const { id, depth } = queue.shift()!;
The non-null assertion (!) is safe here due to the while (queue.length > 0) check, but consider:

const item = queue.shift();
if (!item) continue;
const { id, depth } = item;

2. Inconsistent Naming

• NODE_HEIGHT_BASE vs NODE_HEIGHT_WITH_DESC (abbreviated)
• EXTERNAL_CLUSTER_OFFSET vs full names elsewhere

3. Comment Typo
Line 2: "A complete rewrite from force-directed graph to a clean, centered mind map layout."
Consider adding "Replaces ForceGraph.tsx" for clarity in git history.

4. Unused Import
Check if all imports in DocumentGraphView.tsx are used after the refactor (saw RotateCcw, Settings2 removed).

🎯 Performance Considerations

1. Canvas Rendering

• Good: Single canvas element, no DOM thrashing
• Good: useMemo for layout calculations (line 468)
• Good: useCallback for event handlers

2. Potential Optimization
Consider adding a shouldComponentUpdate equivalent if re-renders become expensive with large graphs:

const prevNodesRef = useRef(nodes);
if (prevNodesRef.current === nodes) return; // Skip render if data unchanged

🔒 Security Assessment

✅ No SQL injection risks (no database queries)
✅ No command injection (no shell commands)
✅ File path handling: Appears safe, uses relative paths
⚠️ XSS risk: External URL handling (see issue #1 above)
✅ Input sanitization: Search query appears safe (no innerHTML usage)

📊 Code Metrics

Metric	Value	Assessment
Files changed	16	Reasonable for refactor
Additions	1,517	Large but expected
Deletions	1,355	Good cleanup
Net change	+162	Minimal bloat
New component size	1,141 lines	Consider splitting if grows
Test coverage	Partial	Needs improvement

🚀 Recommendations

High Priority:

1. ✅ Fix XSS vulnerability in URL handling
2. ✅ Add unit tests for MindMap component
3. ✅ Add error boundaries for canvas operations

Medium Priority:
4. Extract duplicate column layout logic
5. Add keyboard focus visual indicator
6. Convert magic numbers to named constants

Low Priority:
7. Consider spatial indexing for large graphs
8. Add performance monitoring for render time
9. Document the layout algorithm with diagrams

✨ Conclusion

This is a well-executed refactor that significantly improves the Document Graph feature. The code is clean, well-documented, and follows the project's conventions (as outlined in CLAUDE.md). The main concerns are:

1. Security: XSS vulnerability in external URL handling (must fix)
2. Testing: Missing test coverage for core new component (should add)
3. Performance: Minor optimization opportunities for large graphs

After addressing the XSS issue and adding basic tests, this PR is ready to merge. The deterministic layout is a major UX improvement over the previous physics-based approach.

Approval Status: ⚠️ Approve with changes - Fix XSS issue before merging

Great work on this refactor! The mind map approach is much more intuitive than force-directed graphs for document relationships.