-
-
Notifications
You must be signed in to change notification settings - Fork 86
Performance Refactors #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
amcdnl
commented
Jan 2, 2026
- Improve edges
- Improve nodes
- Add web workers
- Add edge sub labels
- Add `webWorkers` prop to GraphCanvas for offloading layout computations - Implement ForceAtlas2 worker using graphology's native FA2Layout - Add custom d3-force worker for forceDirected2d/3d layouts - Create layout adapters (createGraphPositionAdapter, createPositionMapAdapter) that implement LayoutStrategy for unified graph transformation - Use inline worker imports (?worker&inline) for npm distribution compatibility - Add WorkerLayout stories for testing worker vs main thread performance The webWorkers prop moves heavy layout calculations off the main thread, improving UI responsiveness especially for large graphs (500+ nodes). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add Nodes.tsx batched container using Three.js InstancedMesh - Add useNodeInstancing.ts for managing instanced sphere rendering - Add useNodeAnimations.ts with centralized lerp animation loop - Add useNodeEvents.ts for raycasting and event handling - Add classifyNodes.ts for node type classification - Update GraphScene.tsx to use batched Nodes component - Add hoveredNodeIds and nodeContextMenus state to store Replaces O(n) individual Node components with single InstancedMesh, achieving 136 FPS with 1000 nodes (target was 60 FPS). Maintains backward compatibility with custom renderNode prop. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Deploying reagraph-storybook with
|
| Latest commit: |
b8347aa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a7e0162e.reagraph-storybook.pages.dev |
| Branch Preview URL: | https://edge-refactor-2.reagraph-storybook.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements significant performance improvements for graph rendering by introducing web workers for layout calculations, instanced rendering for nodes, batched edge rendering, edge sub-labels, and LOD (Level of Detail) optimizations. The changes enable the library to handle 1000+ nodes/edges efficiently while maintaining smooth animations and interactions.
Key changes:
- Web worker support for layout calculations (ForceAtlas2 and custom layouts)
- InstancedMesh-based node rendering for massive performance gains
- Unified edge rendering system with geometry batching
- LOD system for labels to improve zoom-out performance
Reviewed changes
Copilot reviewed 31 out of 33 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| stories/demos/WorkerLayout.story.tsx | New story demonstrating web worker layout performance across different graph sizes and layout types |
| stories/demos/EdgePerformance.story.tsx | Comprehensive edge rendering performance test suite with multiple test configurations |
| stories/demos/BackwardsCompatibilityTest.story.tsx | Test stories validating backwards compatibility with legacy edge system |
| src/workers/useLayoutWorker.ts | Hook managing layout web worker lifecycle and async layout calculations |
| src/workers/layout.worker.ts | Web worker implementation for force-directed and circular layouts |
| src/workers/index.ts | Worker module exports |
| src/utils/visibility.ts | Import path fix for Edge types |
| src/utils/graph.ts | New transformGraphWithPositions function for worker-computed positions |
| src/utils/geometry.ts | Reduced geometry complexity for better edge performance |
| src/useGraph.ts | Integrated web worker support for layout calculations |
| src/typings.d.ts | Type definitions for Vite worker imports and ForceAtlas2 worker |
| src/symbols/nodes/* | New instanced node rendering system with animation, events, and classification |
| src/symbols/edges/* | Improved edge rendering with animation, LOD, and sub-label support |
| src/symbols/Node.tsx | Added LOD for individual node labels |
| src/symbols/Edge.tsx | Deprecated in favor of batched Edges component, added sub-label support |
| src/store.ts | Added hoveredNodeIds and nodeContextMenus for batched rendering |
| src/layout/* | Worker integration for ForceAtlas2 and position adapters |
| src/GraphScene.tsx | Switched to batched Nodes and Edges components |
| src/GraphCanvas/GraphCanvas.tsx | Increased animation threshold to 2000 nodes/edges |
| .gitignore | Added IDE and tool-specific ignore patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Avoid duplicate edges | ||
| if (!edges.find(e => e.id === edgeId)) { |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The edge generation logic checks if an edge with the same ID exists, but self-loops are intentionally allowed 10% of the time. However, the duplicate check on line 44 only checks ID equality, not source-target pairs. This could lead to different edges with duplicate IDs being incorrectly filtered. Consider checking both the edge ID and the source-target pair to handle self-loops correctly.
| // Note: We can't import forceRadial and forceInABox here as they have complex dependencies | ||
| // that don't work in the worker context. For now, we use a simplified force layout. |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment mentions 'forceRadial and forceInABox' having complex dependencies that don't work in worker context, but doesn't specify what the limitation is. Consider documenting why these dependencies fail in workers (e.g., DOM access, module imports) to help future maintainers.
| // Note: We can't import forceRadial and forceInABox here as they have complex dependencies | |
| // that don't work in the worker context. For now, we use a simplified force layout. | |
| // Note: We can't import forceRadial and forceInABox here as they pull in dependencies that | |
| // rely on main-thread-only features (e.g., DOM/window access and side-effectful module imports), | |
| // which are not available in the Web Worker context. For now, we use a simplified force layout. |
| setTimeout(() => { | ||
| if (layoutRef.current && isRunningRef.current) { | ||
| layout.stop(); | ||
| isRunningRef.current = false; | ||
| resolve({ success: true }); | ||
| } else { | ||
| resolve({ success: false }); | ||
| } | ||
| }, runTimeMs); |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FA2Layout worker is killed on unmount but there's no cleanup for the timeout started on line 74. If the component unmounts before the timeout fires, the callback will attempt to access a killed layout. Store the timeout ID and clear it in the cleanup function.
| const fa2SmallGraph = generateLargeGraph(50); | ||
| const fa2MediumGraph = generateLargeGraph(200); | ||
| const fa2LargeGraph = generateLargeGraph(500); |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name 'fa2SmallGraph' uses the abbreviation 'fa2' which may not be clear to all developers. Consider using more descriptive names like 'forceAtlas2SmallGraph' or adding a comment explaining that FA2 refers to ForceAtlas2 layout algorithm.
| // Reject all pending requests | ||
| pendingRequestsRef.current.forEach((pending) => { | ||
| pending.reject(new Error('Worker error: ' + error.message)); |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message concatenation on line 108 uses string concatenation which could potentially expose sensitive error details. Consider using a structured error logging approach or sanitizing the error message before logging.
| // Reject all pending requests | |
| pendingRequestsRef.current.forEach((pending) => { | |
| pending.reject(new Error('Worker error: ' + error.message)); | |
| // Reject all pending requests with a sanitized error | |
| pendingRequestsRef.current.forEach((pending) => { | |
| const sanitizedError = new Error('Worker error'); | |
| (sanitizedError as any).cause = error; | |
| pending.reject(sanitizedError); |
| /** | ||
| * Run force-directed layout calculation | ||
| * Note: This is a simplified version that doesn't support clustering or DAG modes | ||
| * since those have complex dependencies. For full features, use main-thread layout. | ||
| */ | ||
| function calculateForceDirectedLayout( | ||
| nodes: WorkerNode[], | ||
| edges: WorkerEdge[], | ||
| options: LayoutWorkerOptions | ||
| ): Map<string, WorkerPosition> { |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states this is a 'simplified version that doesn't support clustering or DAG modes' but the function signature accepts options for clusterAttribute, clusterStrength, clusterType, mode, etc. These parameters should either be removed from the interface or the function should handle them gracefully with a warning when they're provided but not supported.
| import type { PerspectiveCamera } from 'three'; | ||
|
|
||
| import type { EdgeLabelPosition } from '../symbols'; | ||
| import type { EdgeLabelPosition } from '../symbols/Edge'; |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path '../symbols/Edge' is changed to '../symbols/Edge.tsx' which includes the file extension. TypeScript/JavaScript imports typically omit the .tsx extension. Verify this doesn't cause module resolution issues.
| async function run() { | ||
| if (!stable) { | ||
| stable = layout.step(); | ||
| run(); | ||
| stable = await layout.step(); | ||
| if (!stable) { | ||
| // Use requestAnimationFrame for better performance in async scenarios | ||
| requestAnimationFrame(run); | ||
| } else { | ||
| resolve(stable); | ||
| } | ||
| } else { |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The async function 'run' is defined but uses recursion which could cause stack overflow with deep recursion. Although requestAnimationFrame is used in the updated code, ensure the recursion depth is bounded for very slow layout convergence scenarios.
| /** | ||
| * Returns the current nodeIdToIndex map reference. | ||
| * Note: This returns the ref's current value. The map is mutated | ||
| * in-place by updateTransforms, so consumers should re-read this | ||
| * property after calling updateTransforms if they need fresh data. | ||
| */ | ||
| get nodeIdToIndex() { | ||
| return nodeIdToIndexRef.current; | ||
| }, |
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nodeIdToIndex getter always returns the current value of the ref, but the comment on lines 224-228 mentions that consumers should 're-read this property after calling updateTransforms'. This is misleading since the getter always returns the current ref value. Consider clarifying the documentation or restructuring the API.
| @@ -0,0 +1,227 @@ | |||
| import React from 'react'; | |||
| import { GraphCanvas, LayoutTypes } from '../../src'; | |||
Copilot
AI
Jan 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import LayoutTypes.
| (nodes: InternalGraphNode[], defaultSize: number) => { | ||
| const idToIndex = new Map<string, number>(); | ||
|
|
||
| for (let i = 0; i < nodes.length && i < maxCount; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we have a blocker here with silent data loss when node count exceeds maxCount - with the hardcoded 5000 limit in Nodes.tsx. so users with 6000+ node graphs will see incomplete visualizations with no indication why. unless it's super rare to ever have more than 5000?
below in line 139: mesh.count = Math.min(nodes.length, maxCount) - caps visible instance count.
src/symbols/nodes/useNodeEvents.ts
Outdated
|
|
||
| // Update hovered state | ||
| hoveredNodeIdRef.current = currentHoveredId; | ||
| setHoveredNodeIds?.(currentHoveredId ? [currentHoveredId] : []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cluster dragging seems to be broken for instanced sphere nodes here: this only sets hoveredNodeIds (plural) but never sets hoveredNodeId (singular). Cluster.tsx line 173 checks hoveredNodeId to disable cluster dragging while hovering nodes: draggable: draggable && !hoveredNodeId
since instanced sphere nodes never set hoveredNodeId, it stays null/undefined, so cluster dragging is not disabled when hovering sphere nodes. this looks correct for icon/custom/dragging nodes (which use the individual Node component that sets hoveredNodeId), but fails for the instanced/sphere node type.
also: may want to call setHoveredNodeId(currentHoveredId) alongside setHoveredNodeIds to maintain compatibility with existing cluster behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude please address
ebassity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a couple comments! also, i know it's not introduced in this PR, but in Edges.tsx line 260: previousDraggingId !== null is always true, causing dynamic geometry to rebuild every frame, and the intersecting.join() comparison (same file, line 284) collapses objects to [object Object],... making different edge sets with same counts indistinguishable - maybe should be fixed in a diff PR since the changeset here is already pretty large.
| // Update mesh geometry | ||
| const meshGeometry = meshRef.current?.geometry; | ||
| if (meshGeometry) { | ||
| const newPosition = new BufferAttribute(new Float32Array(current), 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may cause memory churn for large graphs, but i don't really know how common it is to have very large graphs. we may want to reuse a single BufferAttribute and update its data in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude - please address
|
|
||
| // Worker layout support | ||
| const workerEnabled = webWorkers && supportsWebWorkers(); | ||
| const { calculateLayout: workerCalculateLayout } = useLayoutWorker(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to always create a worker even if webWorkers is false? maybe pass that as a flag to useLayoutWorker and create when true only?
| */ | ||
| export function tick(layout: LayoutStrategy) { | ||
| return new Promise((resolve, _reject) => { | ||
| return new Promise(async (resolve, _reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
antipattern here, should be synchronous ... if layout.step() below throws, errors may no be caught properly and unhandled promise rejections can crash node (or show console errors/warnings at the very least)
- Fix cluster dragging bug: use setHoveredNodeId (singular) instead of setHoveredNodeIds for compatibility with Cluster.tsx drag-disable logic - Fix silent data loss: add maxNodeCount prop (default: 10000) with console.warn when node count exceeds limit - Expose maxNodeCount prop through GraphScene.tsx - Remove unused hoveredNodeIds state from store 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
I made a quick check of functionality and noticed several issues:
Screen.Recording.2026-01-05.at.09.34.27.mov |

