refactor: update dependencies and improve graph handling#697
Open
Anchel123 wants to merge 1 commit into
Open
Conversation
- Changed the dependency for @falkordb/canvas to a local file reference. - Introduced graphDataToData function to standardize graph data handling across components. - Removed cooldownTicks state management and replaced it with animation state in multiple components. - Enhanced the Chat component to utilize createMessage for message creation, ensuring unique IDs. - Updated the Toolbar component to include animation controls and layout options. - Modified the Vite configuration to point to the production API endpoint.
Contributor
📝 WalkthroughWalkthroughThe PR modernizes the code graph canvas system by replacing cooldown-based timing with explicit animation state, reorganizing canvas event configuration, adding auto-increment message IDs, wrapping graph data conversions, and redesigning the toolbar with layout and pin controls while improving node label rendering with better typography and color contrast. ChangesAnimation & Graph Canvas Modernization
Sequence Diagram(s)sequenceDiagram
participant Toolbar
participant CodeGraph
participant Canvas as Canvas (via<br/>ForceGraph)
Toolbar->>Toolbar: user toggles animation
Toolbar->>CodeGraph: setAnimation(boolean)
CodeGraph->>CodeGraph: update local state
CodeGraph->>Canvas: pass animation to ForceGraph
Canvas->>Canvas: setAnimation effect runs
Canvas->>Canvas: animation state updated
Note over Canvas: physics engine uses animation flag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| const isHovered = !!hoverElement && !('source' in hoverElement) && hoverElement.id === node.id | ||
| const isSelected = selectedObjects.some(obj => obj.id === node.id) || selectedObj?.id === node.id | ||
| const nodeSelected = isSelected || isHovered |
![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
🧹 Nitpick comments (4)
app/src/components/graphView.tsx (2)
238-240: ⚖️ Poor tradeoffConsider if zoom threshold is appropriate.
Labels are skipped when
zoom < 1, which optimizes performance for large graphs. However, this threshold might hide labels at zoom levels where they're still readable. Consider if0.5or0.75might be more appropriate, or make it configurable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/graphView.tsx` around lines 238 - 240, The label-skipping threshold uses a hardcoded check (const zoom = ctx.getTransform().a; if (zoom < 1) return;) which may hide labels too aggressively; replace this magic number with a configurable constant or prop (e.g., ZOOM_LABEL_THRESHOLD / prop like labelZoomThreshold) and use that in the check (if (zoom < labelZoomThreshold) return;), expose a sensible default (0.75 or 0.5) and update any relevant component props or config in the GraphView rendering code so callers can tune the threshold.
159-162: 💤 Low valueVerify intentional debouncing behavior.
lastClick.current.nameis now reset to an empty string in both the double-click andisShowPathpaths. This prevents rapid consecutive actions on the same node. Confirm this debouncing is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/graphView.tsx` around lines 159 - 162, The change resets lastClick.current.name to an empty string in both the double-click branch and the isShowPath branch which effectively disables rapid consecutive actions on the same node; if this debouncing is not intended, remove the line that clears lastClick.current.name from the isShowPath branch (keep it only where you call handleExpand([node], !node.expand) or where double-click behavior is handled); if it is intentional, add a brief code comment near lastClick/current and the isShowPath branch documenting that clearing name is deliberate to debounce repeated actions and reference the lastClick and handleExpand logic so future readers understand the behavior.app/src/components/chat.tsx (2)
12-15: ⚡ Quick winMove the
@falkordb/canvasimport above the executable code.This
importis placed after theAUTH_HEADERSconst declaration. While ES module imports are hoisted so this runs correctly, it's a code smell and will typically trip theimport/firstlint rule.♻️ Proposed reorder
import { Button } from "`@/components/ui/button`"; +import { dataToGraphData, graphDataToData, GraphLink, GraphNode } from "`@falkordb/canvas`"; const AUTH_HEADERS: HeadersInit = import.meta.env.VITE_SECRET_TOKEN ? { 'Authorization': `Bearer ${import.meta.env.VITE_SECRET_TOKEN}` } : {}; -import { dataToGraphData, graphDataToData, GraphLink, GraphNode } from "`@falkordb/canvas`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/chat.tsx` around lines 12 - 15, Move the import statement for "`@falkordb/canvas`" above any executable top-level code so imports come first; specifically, relocate the line importing dataToGraphData, graphDataToData, GraphLink, and GraphNode so it appears before the AUTH_HEADERS constant declaration, ensuring imports are at the top to satisfy the import/first lint rule and avoid mixing module imports with executable code.
230-235: ⚡ Quick winRemove the
console.logdebug artifacts.The two
[zoomToFit]logs look like leftover debugging and will run on every path selection in production.🧹 Proposed cleanup
setTimeout(() => { - const filteredNodes = currentData.nodes.filter((n: any) => pNodeIds.has(n.id)); - console.log('[zoomToFit] filtered nodes:', filteredNodes.map((n: any) => ({ id: n.id, x: n.x, y: n.y }))); - console.log('[zoomToFit] pNodeIds:', [...pNodeIds]); canvas.zoomToFit(2, (n: GraphNode) => pNodeIds.has(n.id)); }, 300)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/chat.tsx` around lines 230 - 235, Remove the debug console.log calls inside the setTimeout: eliminate both console.log('[zoomToFit] filtered nodes:...' ) and console.log('[zoomToFit] pNodeIds:' ...), leaving the logic that computes filteredNodes and the canvas.zoomToFit(2, (n: GraphNode) => pNodeIds.has(n.id)) call intact; ensure no other stray console logging remains in this block (references: filteredNodes, pNodeIds, currentData.nodes, canvas.zoomToFit).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/package.json`:
- Line 14: The package dependency "`@falkordb/canvas`" in app/package.json
currently points to a missing local path "file:../../falkordb-canvas" causing
TS2307; fix it by updating the dependency to a valid reference (either correct
the relative file: path to the actual local package location, replace it with
the workspace / monorepo package name if using workspaces, or point to a
published version on the registry), then ensure the target package exports
TypeScript declarations (include "types" or .d.ts files or set "declaration":
true in its tsconfig) so imports resolve correctly; look for the dependency
entry in app/package.json and adjust it and the referenced package's
package.json/tsconfig accordingly.
---
Nitpick comments:
In `@app/src/components/chat.tsx`:
- Around line 12-15: Move the import statement for "`@falkordb/canvas`" above any
executable top-level code so imports come first; specifically, relocate the line
importing dataToGraphData, graphDataToData, GraphLink, and GraphNode so it
appears before the AUTH_HEADERS constant declaration, ensuring imports are at
the top to satisfy the import/first lint rule and avoid mixing module imports
with executable code.
- Around line 230-235: Remove the debug console.log calls inside the setTimeout:
eliminate both console.log('[zoomToFit] filtered nodes:...' ) and
console.log('[zoomToFit] pNodeIds:' ...), leaving the logic that computes
filteredNodes and the canvas.zoomToFit(2, (n: GraphNode) => pNodeIds.has(n.id))
call intact; ensure no other stray console logging remains in this block
(references: filteredNodes, pNodeIds, currentData.nodes, canvas.zoomToFit).
In `@app/src/components/graphView.tsx`:
- Around line 238-240: The label-skipping threshold uses a hardcoded check
(const zoom = ctx.getTransform().a; if (zoom < 1) return;) which may hide labels
too aggressively; replace this magic number with a configurable constant or prop
(e.g., ZOOM_LABEL_THRESHOLD / prop like labelZoomThreshold) and use that in the
check (if (zoom < labelZoomThreshold) return;), expose a sensible default (0.75
or 0.5) and update any relevant component props or config in the GraphView
rendering code so callers can tune the threshold.
- Around line 159-162: The change resets lastClick.current.name to an empty
string in both the double-click branch and the isShowPath branch which
effectively disables rapid consecutive actions on the same node; if this
debouncing is not intended, remove the line that clears lastClick.current.name
from the isShowPath branch (keep it only where you call handleExpand([node],
!node.expand) or where double-click behavior is handled); if it is intentional,
add a brief code comment near lastClick/current and the isShowPath branch
documenting that clearing name is deliberate to debounce repeated actions and
reference the lastClick and handleExpand logic so future readers understand the
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5b0f6a2-2798-4305-a437-a072f011bb0e
⛔ Files ignored due to path filters (1)
app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app/package.jsonapp/src/App.tsxapp/src/components/ForceGraph.tsxapp/src/components/chat.tsxapp/src/components/code-graph.tsxapp/src/components/graphView.tsxapp/src/components/toolbar.tsxapp/src/lib/utils.ts
| }, | ||
| "dependencies": { | ||
| "@falkordb/canvas": "^0.0.44", | ||
| "@falkordb/canvas": "file:../../falkordb-canvas", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the local `@falkordb/canvas` package exists and has required files
# Check if the directory exists
if [ -d "../../falkordb-canvas" ]; then
echo "✓ Directory exists at ../../falkordb-canvas"
# Check for package.json
if [ -f "../../falkordb-canvas/package.json" ]; then
echo "✓ package.json found"
cat ../../falkordb-canvas/package.json | jq '{name, version, main, types, exports}'
else
echo "✗ package.json NOT found"
fi
# Check for TypeScript definitions
fd -e d.ts . ../../falkordb-canvas -x echo "Found: {}"
# List directory structure
tree -L 2 ../../falkordb-canvas || ls -la ../../falkordb-canvas
else
echo "✗ Directory does NOT exist at ../../falkordb-canvas"
echo "Current directory structure:"
pwd
ls -la ../..
fiRepository: FalkorDB/code-graph
Length of output: 329
Critical: @falkordb/canvas local file:../../falkordb-canvas path is missing in this checkout (causes TS2307).
File: app/package.json (line 14)
The file:../../falkordb-canvas directory isn’t present in the repo checkout, so @falkordb/canvas can’t be resolved and TypeScript fails with TS2307. Fix the file: path to the actual location (or wire it via the workspace setup) and ensure the package exports/provides TypeScript type declarations.
🧰 Tools
🪛 GitHub Actions: Playwright Tests / test (2)
[error] Build step failed: 'npm run build' (runs 'tsc -b && vite build'). Command exited with a non-zero status due to the TypeScript error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/package.json` at line 14, The package dependency "`@falkordb/canvas`" in
app/package.json currently points to a missing local path
"file:../../falkordb-canvas" causing TS2307; fix it by updating the dependency
to a valid reference (either correct the relative file: path to the actual local
package location, replace it with the workspace / monorepo package name if using
workspaces, or point to a published version on the registry), then ensure the
target package exports TypeScript declarations (include "types" or .d.ts files
or set "declaration": true in its tsconfig) so imports resolve correctly; look
for the dependency entry in app/package.json and adjust it and the referenced
package's package.json/tsconfig accordingly.
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 by CodeRabbit
New Features
Improvements