refactor: update dependencies and improve graph handling#697
Open
Anchel123 wants to merge 11 commits into
Open
refactor: update dependencies and improve graph handling#697Anchel123 wants to merge 11 commits into
Anchel123 wants to merge 11 commits into
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR replaces cooldown-tick-based canvas control with animation/manual-dimming state, adds full-model canvas refresh ( ChangesGraph UI and interaction updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CodeGraph
participant GraphModel as graph.Elements
participant ForceGraph
participant Canvas as canvasRef
User->>CodeGraph: expand/collapse/remove/reset node
CodeGraph->>GraphModel: update visibility/expand/path flags
CodeGraph->>ForceGraph: convertToCanvasData(graph.Elements)
ForceGraph->>Canvas: setData/setGraphData(...)
Canvas-->>User: refresh()/zoomToFit()
sequenceDiagram
participant User
participant Chat
participant GraphModel as graph.Elements
participant ForceGraph
participant Canvas as canvasRef
User->>Chat: select path / submit query
Chat->>Chat: createMessage(...)
Chat->>GraphModel: mark isPath / isPathSelected
Chat->>ForceGraph: convertToCanvasData(graph.Elements)
ForceGraph->>Canvas: setGraphData(...)
Canvas-->>User: zoomToFit()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
![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
- Updated Combobox component to enforce string array type for options. - Added container height state to ElementMenu for improved positioning. - Refactored ElementMenu positioning logic to ensure it remains within parent boundaries. - Improved GraphView component by removing unused props and optimizing node/link selection logic. - Introduced manual dimming feature for nodes and links in GraphView based on selection. - Enhanced Toolbar with zoom controls and manual dimming toggle. - Updated Graph model to include collapsed state for links and modified link removal logic. - Added e2e tests to validate node visibility and position preservation after hiding/unhiding.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/src/components/model.ts (1)
188-192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRefresh
collapsedwhen extending an existing link.New links get
collapsed, but existing links keep their old value. If a link was first added as collapsed and later appears in a non-collapsed extension/path,removeLinks(ids)can still remove it as collapsed.Proposed fix
if (link) { link.isPath = !!path + link.collapsed = link.collapsed && collapsed return }Also applies to: 244-250
🤖 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/model.ts` around lines 188 - 192, When extending an existing link in the edge-processing path, `link.isPath` is updated but `collapsed` is left stale on the existing `link` object. Update the existing-link branch in the same logic that handles `results.edges.forEach(...)` and the related `removeLinks(ids)` flow so that `collapsed` is refreshed from the current edge/path state just like newly created links, ensuring `linksMap` entries don’t retain an old collapsed value across subsequent non-collapsed extensions.app/src/components/toolbar.tsx (1)
143-170: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTurn animation off when disabling its control.
Pinning or switching to non-force layouts disables the switch, but leaves
animationandcanvas.setAnimationunchanged. This can leave animation running with a disabled checked control.Proposed fix
const handlePinToggle = () => { const next = !pinned; setPinned(next); canvasRef.current?.setPinOnDragEnd(next); + if (next) { + setAnimation(false); + canvasRef.current?.setAnimation(false); + } }const nextPinned = mode !== 'force'; setPinned(nextPinned); canvasRef.current?.setPinOnDragEnd(nextPinned); + if (nextPinned) { + setAnimation(false); + canvasRef.current?.setAnimation(false); + }Also applies to: 184-193
🤖 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/toolbar.tsx` around lines 143 - 170, The toolbar’s pin/layout handlers disable the animation control state without turning off the underlying animation, so update the animation state whenever non-force layouts or pinning effectively disable the toggle. In handlePinToggle and handleLayoutChange in toolbar.tsx, ensure animation is set to false and canvasRef.current?.setAnimation(false) is called when the control becomes disabled, while keeping the UI state in sync with the canvas. Also apply the same change to the matching logic around the other referenced block so animation cannot remain running under a disabled checked control.app/src/components/code-graph.tsx (1)
196-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire
node.idon one endpoint ofisConnected.
The current predicate can match links whose source and target are both innodeseven whennodeis not an endpoint, which can delete unrelated collapsed nodes during recursion.🤖 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/code-graph.tsx` around lines 196 - 222, The deleteNeighbors logic is over-matching links in the isConnected check, which can cause unrelated collapsed nodes to be removed during recursion. Update the predicate inside deleteNeighbors in code-graph.tsx so that one endpoint must be the current node.id and only the other endpoint may come from nodes, then keep the recursive expandedNodes flow and graph.removeLinks behavior unchanged.app/src/App.tsx (1)
219-284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep React
datain sync whengraph.Elementsis reassigned
Graph.extendmutates in place, so the search/category updates stay aligned today. The breakage isdeleteNeighbors, wheregraph.Elementsis replaced with a new object andsetDatais not called. After that,ForceGraph.tsxstill resolves clicks/hover through staledata.nodes/data.links, so new or removed elements can stop mapping correctly. AddsetData({ ...graph.Elements })after the model update, as in “Reset Graph”.🤖 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/App.tsx` around lines 219 - 284, Keep the React graph data in sync whenever graph.Elements is replaced, because ForceGraph.tsx still reads stale data.nodes/data.links after operations like deleteNeighbors. Update handleSearchSubmit/onCategoryClick flow only if needed, but make sure the model mutation path that reassigns graph.Elements also calls setData with the refreshed graph.Elements, matching the existing Reset Graph behavior. Use graph.Elements, setData, and the deleteNeighbors update path to locate the fix.
🧹 Nitpick comments (5)
app/src/components/graphView.tsx (2)
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for new frontend constants.
The new constants are variables under
app/src, so they should follow the frontend camelCase guideline.Also applies to: 67-69, 207-208
🤖 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 37 - 42, The new constants in graphView.tsx use SCREAMING_SNAKE_CASE instead of the frontend camelCase convention. Rename the related constants near the top of the file, and also update the other affected constant declarations referenced in the comment, so their identifiers follow camelCase consistently. Make sure every usage in graphView.tsx is updated to the new names, including any references in the graph rendering/theme logic and the double-click timing helper.Source: Coding guidelines
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
selectedObjectsto include links.This state now stores link objects too, but the prop/setter remain
Node[], forcing unsafe casts and weakening tsc coverage.Proposed fix
- selectedObjects: Node[] - setSelectedObjects: Dispatch<SetStateAction<Node[]>> + selectedObjects: (Node | Link)[] + setSelectedObjects: Dispatch<SetStateAction<(Node | Link)[]>>- setSelectedObjects([element as Node]) + setSelectedObjects([element])Also applies to: 99-100, 195-200
🤖 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 23 - 24, The selectedObjects state and its setter are still typed as Node[] even though they now also hold link objects, so update the relevant GraphView props/types to use a shared union type that includes links instead of forcing casts. Adjust the type declarations near selectedObjects/setSelectedObjects and any related uses in GraphView so tsc can validate both nodes and links consistently.Source: Coding guidelines
app/src/App.tsx (2)
271-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated canvas-visibility-sync logic.
This exact "mutate
canvas.getGraphData()nodes/links from the app model thencanvas.refresh()" pattern is repeated verbatim incode-graph.tsx'shandleRemoveand "Unhide Nodes" handler. Consider extracting a shared helper (e.g.,syncCanvasVisibility(canvas, graph)) to avoid triple-maintained logic.🤖 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/App.tsx` around lines 271 - 282, The canvas visibility sync logic is duplicated across App.tsx, code-graph.tsx handleRemove, and the Unhide Nodes handler; extract this repeated “read canvas.getGraphData(), copy visibility from graph.NodesMap/LinksMap, then canvas.refresh()” flow into a shared helper such as syncCanvasVisibility(canvas, graph). Update the App component and the code-graph.tsx handlers to call the helper instead of maintaining separate copies, using the existing canvas and graph symbols to keep the behavior identical.
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cooldownTicks/setCooldownTicksare now dead state threaded through props.
ForceGraph.tsx's config no longer accepts or usescooldownTicks(removed per this PR's own stated goal), yetApp.tsxstill declares this state (Line 83) and passescooldownTicks/setCooldownTicksinto both desktop and mobileCodeGraphinstances. Sincecode-graph.tsxalso only destructures these without further use, this is leftover dead code that should be removed for consistency with the PR's intent to replace cooldown-tick management withanimation/manualDimmed.♻️ Proposed cleanup
- const [cooldownTicks, setCooldownTicks] = useState<number | undefined>(undefined) const [animation, setAnimation] = useState(false)and remove the
cooldownTicks/setCooldownTicksprops at bothCodeGraphcall sites.Also applies to: 494-499, 624-629
🤖 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/App.tsx` around lines 83 - 85, Remove the leftover cooldown tick state from App and the graph wiring: `cooldownTicks`/`setCooldownTicks` in `App.tsx` are no longer used by `CodeGraph` or `ForceGraph`, so delete the `useState` declaration and stop passing those props to both `CodeGraph` call sites. Also update `code-graph.tsx` to remove the unused `cooldownTicks`/`setCooldownTicks` destructuring so the props and state match the new `animation`/`manualDimmed` flow.app/src/components/ForceGraph.tsx (1)
33-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffAvoid rebuilding the full canvas graph for these updates.
convertToCanvasDatadrops layout fields, so thesetGraphData(...)calls inApp.tsx,code-graph.tsx, andchat.tsxcan reinitialize node positions on search/add, expand/collapse, and reset. The visibility-only paths already mutate the canvas in place; keeping this path incremental would avoid extra work and preserve layout state.🤖 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/ForceGraph.tsx` around lines 33 - 52, The `convertToCanvasData` path is being used too broadly and rebuilds the full canvas graph, which drops layout state and reinitializes node positions. Update the `setGraphData(...)` flows in `App.tsx`, `code-graph.tsx`, and `chat.tsx` so search/add, expand/collapse, and reset use incremental canvas updates instead of recreating data through `convertToCanvasData`. Keep the existing in-place visibility mutation behavior for these cases, and only use `convertToCanvasData` when a full graph rehydration is truly needed.
🤖 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/src/components/chat.tsx`:
- Around line 331-347: Clear any existing path state before marking the new
results in chat.tsx: the current path-highlighting block only sets isPath and
PATH_COLOR on the latest formattedPaths, so stale isPath, isPathSelected, and
link color values can survive across responses. Update the path-marking flow
around formattedPaths, graph.Elements.nodes, graph.Elements.links, and
convertToCanvasData(graph.Elements) to reset previous path flags/colors on the
full model first, then apply the new path markings before calling setGraphData.
In `@app/src/components/code-graph.tsx`:
- Around line 224-262: After mutating graph.Elements in handleExpand,
handleRemove, and the Unhide Nodes flow, also call setData(...) with the updated
graph data before redrawing the canvas. The issue is that ForceGraph.tsx
resolves interactions from data.nodes and data.links, so it can keep using stale
entries unless the source data is synced. Update the state in the same places
you already call canvasRef.current?.setGraphData(...) so the canvas model and
ForceGraph data stay aligned.
In `@app/src/components/elementMenu.tsx`:
- Around line 41-45: Rename the new local constants in elementMenu.tsx from
uppercase to camelCase to match the frontend TypeScript style guide; update the
declarations used in the menu positioning logic near the zoom/node radius
calculations, and also the related constants referenced later in the same
component so all usages consistently switch from EDGE_MARGIN and GAP to
camelCase names.
In `@app/src/components/graphView.tsx`:
- Around line 161-175: The zoom logic in handleLinkClick is reading stale
path-selection state because setSelectedPathId(link.id) is async and
getGraphData() still reflects the previous selection. Move the zoom-to-fit
behavior out of handleLinkClick and into the selectedPathId-driven effect, or
compute the clicked path nodes directly from the current link data before
updating state. Use the existing selectedPathId, canvasRef, and getGraphData
flow to ensure zooming happens only after the canvas has been updated with the
new path selection.
In `@app/src/components/toolbar.tsx`:
- Around line 61-67: The icon-only controls in toolbar.tsx are missing
accessible names, so screen readers can’t identify them. Update the button
elements used for zoom, animation, dimming, and pin actions in the toolbar
component to include descriptive aria-label values while keeping the existing
icons and click handlers intact. Use the relevant button/handler symbols in this
file, such as handleZoomClick and canvasRef.current?.zoomToFit, to find and
label each control consistently.
---
Outside diff comments:
In `@app/src/App.tsx`:
- Around line 219-284: Keep the React graph data in sync whenever graph.Elements
is replaced, because ForceGraph.tsx still reads stale data.nodes/data.links
after operations like deleteNeighbors. Update handleSearchSubmit/onCategoryClick
flow only if needed, but make sure the model mutation path that reassigns
graph.Elements also calls setData with the refreshed graph.Elements, matching
the existing Reset Graph behavior. Use graph.Elements, setData, and the
deleteNeighbors update path to locate the fix.
In `@app/src/components/code-graph.tsx`:
- Around line 196-222: The deleteNeighbors logic is over-matching links in the
isConnected check, which can cause unrelated collapsed nodes to be removed
during recursion. Update the predicate inside deleteNeighbors in code-graph.tsx
so that one endpoint must be the current node.id and only the other endpoint may
come from nodes, then keep the recursive expandedNodes flow and
graph.removeLinks behavior unchanged.
In `@app/src/components/model.ts`:
- Around line 188-192: When extending an existing link in the edge-processing
path, `link.isPath` is updated but `collapsed` is left stale on the existing
`link` object. Update the existing-link branch in the same logic that handles
`results.edges.forEach(...)` and the related `removeLinks(ids)` flow so that
`collapsed` is refreshed from the current edge/path state just like newly
created links, ensuring `linksMap` entries don’t retain an old collapsed value
across subsequent non-collapsed extensions.
In `@app/src/components/toolbar.tsx`:
- Around line 143-170: The toolbar’s pin/layout handlers disable the animation
control state without turning off the underlying animation, so update the
animation state whenever non-force layouts or pinning effectively disable the
toggle. In handlePinToggle and handleLayoutChange in toolbar.tsx, ensure
animation is set to false and canvasRef.current?.setAnimation(false) is called
when the control becomes disabled, while keeping the UI state in sync with the
canvas. Also apply the same change to the matching logic around the other
referenced block so animation cannot remain running under a disabled checked
control.
---
Nitpick comments:
In `@app/src/App.tsx`:
- Around line 271-282: The canvas visibility sync logic is duplicated across
App.tsx, code-graph.tsx handleRemove, and the Unhide Nodes handler; extract this
repeated “read canvas.getGraphData(), copy visibility from
graph.NodesMap/LinksMap, then canvas.refresh()” flow into a shared helper such
as syncCanvasVisibility(canvas, graph). Update the App component and the
code-graph.tsx handlers to call the helper instead of maintaining separate
copies, using the existing canvas and graph symbols to keep the behavior
identical.
- Around line 83-85: Remove the leftover cooldown tick state from App and the
graph wiring: `cooldownTicks`/`setCooldownTicks` in `App.tsx` are no longer used
by `CodeGraph` or `ForceGraph`, so delete the `useState` declaration and stop
passing those props to both `CodeGraph` call sites. Also update `code-graph.tsx`
to remove the unused `cooldownTicks`/`setCooldownTicks` destructuring so the
props and state match the new `animation`/`manualDimmed` flow.
In `@app/src/components/ForceGraph.tsx`:
- Around line 33-52: The `convertToCanvasData` path is being used too broadly
and rebuilds the full canvas graph, which drops layout state and reinitializes
node positions. Update the `setGraphData(...)` flows in `App.tsx`,
`code-graph.tsx`, and `chat.tsx` so search/add, expand/collapse, and reset use
incremental canvas updates instead of recreating data through
`convertToCanvasData`. Keep the existing in-place visibility mutation behavior
for these cases, and only use `convertToCanvasData` when a full graph
rehydration is truly needed.
In `@app/src/components/graphView.tsx`:
- Around line 37-42: The new constants in graphView.tsx use SCREAMING_SNAKE_CASE
instead of the frontend camelCase convention. Rename the related constants near
the top of the file, and also update the other affected constant declarations
referenced in the comment, so their identifiers follow camelCase consistently.
Make sure every usage in graphView.tsx is updated to the new names, including
any references in the graph rendering/theme logic and the double-click timing
helper.
- Around line 23-24: The selectedObjects state and its setter are still typed as
Node[] even though they now also hold link objects, so update the relevant
GraphView props/types to use a shared union type that includes links instead of
forcing casts. Adjust the type declarations near
selectedObjects/setSelectedObjects and any related uses in GraphView so tsc can
validate both nodes and links consistently.
🪄 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: 110b048b-7677-471d-a2a8-20c3117c279a
⛔ Files ignored due to path filters (1)
app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
app/package.jsonapp/src/App.tsxapp/src/components/ForceGraph.tsxapp/src/components/chat.tsxapp/src/components/code-graph.tsxapp/src/components/combobox.tsxapp/src/components/elementMenu.tsxapp/src/components/graphView.tsxapp/src/components/model.tsapp/src/components/toolbar.tsxe2e/logic/POM/codeGraph.tse2e/tests/canvas.spec.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the frontend graph rendering stack to the newer @falkordb/canvas API and refactors how graph state (visibility, path highlighting, animation/dimming, layouts) is translated and applied across the React components. It also adds E2E coverage for the “hidden nodes should preserve positions” behavior by ensuring invisible nodes remain in canvas data with visible=false.
Changes:
- Upgraded
@falkordb/canvasand refactored ForceGraph integration (config/eventHandlers, dimming APIs, canvas-data conversion). - Reworked selection/focus + element-menu positioning, and expanded the toolbar with layout + animation/pin/dim controls.
- Added E2E tests to validate hidden node retention and position preservation when toggling visibility.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/tests/canvas.spec.ts | Adds E2E tests validating hidden nodes/links remain in canvas data and preserve positions across hide/unhide. |
| e2e/logic/POM/codeGraph.ts | Makes graph selection locator deterministic by selecting the first match. |
| app/src/lib/utils.ts | Adds Message.id and createMessage() helper for stable chat message identity. |
| app/src/components/toolbar.tsx | Introduces layout + direction controls, pin/unpin, dim toggle, and animation toggle; adds ZoomControls. |
| app/src/components/model.ts | Extends Link model with collapsed; updates link removal logic for collapse behavior. |
| app/src/components/graphView.tsx | Refactors selection/dimming logic, right-click centering/menu positioning, and path-fit zoom behavior. |
| app/src/components/ForceGraph.tsx | Centralizes GraphData→Canvas Data conversion and updates canvas configuration/event binding. |
| app/src/components/elementMenu.tsx | Reworks element menu positioning to be zoom-aware and clamp to container bounds. |
| app/src/components/combobox.tsx | Tightens typing for repositories response. |
| app/src/components/code-graph.tsx | Shifts visibility/path/canvas refresh flows to model-driven updates via shared conversion logic. |
| app/src/components/chat.tsx | Uses createMessage() for unique IDs; refactors path highlighting to update the model then re-render canvas. |
| app/src/App.tsx | Wires new toolbar controls (animation/manual dimming); refactors several canvas updates to shared conversion. |
| app/package.json | Upgrades @falkordb/canvas dependency version. |
| app/package-lock.json | Updates lockfile to reflect the new canvas version and transitive dependency changes. |
Files not reviewed (1)
- app/package-lock.json: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix missing closing brace in graphView.tsx setTimeout handler - Remove unused cooldownTicks state from App.tsx and CodeGraph - Fix chat.tsx path markings to iterate nodes and links separately - Remove stale EDGE_MARGIN references in elementMenu.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/components/code-graph.tsx (1)
224-262: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
datastate still not synced after graph mutations.
handleExpand(line 245),handleRemove(line 288), and "Unhide Nodes" (line 380) update the canvas but never callsetData(...). "Reset Graph" (line 349) correctly does. IfForceGraphresolves click/hover callbacks fromdata.nodes/data.links, stale or removed nodes can still be looked up until a later reset/reload.This was raised on a prior commit and remains unresolved after the rewrite.
🔧 Add setData calls after canvas mutations
const handleExpand = async (nodes: Node[], expand: boolean) => { // ... existing logic ... canvasRef.current?.setGraphData(convertToCanvasData(graph.Elements)) + setData({ ...graph.Elements }) } else { // ... existing logic ... canvasRef.current?.setGraphData(convertToCanvasData(graph.Elements)) + setData({ ...graph.Elements }) } setSelectedObjects([]) }const handleRemove = (ids: number[], type: "nodes" | "links") => { // ... existing logic ... canvas.refresh() + setData({ ...graph.Elements }) setHasHiddenElements(true) setSelectedObjects([]) }// Unhide Nodes onClick canvas.refresh(); +setData({ ...graph.Elements }); setHasHiddenElements(false);Also applies to: 264-293, 367-382
🤖 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/code-graph.tsx` around lines 224 - 262, The graph canvas updates in handleExpand, handleRemove, and the “Unhide Nodes” path are not syncing the data state, leaving ForceGraph callbacks working off stale data. After each mutation that changes graph.Elements, also call setData(...) with the latest nodes/links (as Reset Graph already does) before or alongside canvasRef.current?.setGraphData(convertToCanvasData(graph.Elements)); use the existing handleExpand, handleRemove, and unhide/reset-related logic in code-graph.tsx to keep data and canvas aligned.
🧹 Nitpick comments (2)
e2e/tests/elementMenu.spec.ts (1)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double right-click on the same node.
nodeClickalready right-clicks and waits for the "View Node" button to appear (confirming the menu is open). The subsequentrightClickAtNoderight-clicks again at the same coordinates, which is unnecessary and may cause a brief menu flicker. Use only one of the two methods.♻️ Proposed fix: remove redundant nodeClick call
// Right-click on the node to trigger element menu - await codeGraph.nodeClick(targetNode.screenX, targetNode.screenY); - await codeGraph.rightClickAtNode(targetNode.screenX, targetNode.screenY); + await codeGraph.rightClickAtNode(targetNode.screenX, targetNode.screenY);🤖 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 `@e2e/tests/elementMenu.spec.ts` around lines 39 - 40, The test is performing a redundant second right-click on the same node: `codeGraph.nodeClick(...)` already opens the context menu and waits for the “View Node” button, so the following `codeGraph.rightClickAtNode(...)` is unnecessary. Update the flow in `e2e/tests/elementMenu.spec.ts` to use only one of these actions (preferably `nodeClick` if it already verifies the menu state) and keep the rest of the assertions unchanged.e2e/logic/POM/codeGraph.ts (1)
877-892: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
rightClickAtNodeis less robust than the existingnodeClickhelper.Unlike
nodeClick(lines 522–546),rightClickAtNodemakes a single attempt with no bounds validation and usesisVisible({ timeout: 3000 }), which waits for DOM attachment rather than full visibility. This can cause flakiness when the menu is briefly hidden during animation. Consider mirroringnodeClick's retry loop or usingwaitFor({ state: 'visible' }).♻️ Proposed refactor: add retry loop and bounds validation
async rightClickAtNode(x: number, y: number): Promise<void> { await this.waitForCanvasAnimationToEnd(); const boundingBox = await this.canvasElement.boundingBox(); if (!boundingBox) throw new Error("Canvas bounding box not found"); - // Move to node position and right-click - await this.page.mouse.move(x, y); - await this.page.waitForTimeout(300); - await this.page.mouse.click(x, y, { button: 'right' }); - - // Wait for element menu to appear - const isMenuVisible = await this.elementMenu.isVisible({ timeout: 3000 }).catch(() => false); - if (!isMenuVisible) { - throw new Error(`Element menu not visible after right-clicking at (${x}, ${y})`); + const maxX = boundingBox.x + boundingBox.width; + const maxY = boundingBox.y + boundingBox.height; + if (x < boundingBox.x || x > maxX || y < boundingBox.y || y > maxY) { + throw new Error( + `Right-click coordinates (${x}, ${y}) are outside canvas bounds ` + + `[${boundingBox.x}, ${maxX}] x [${boundingBox.y}, ${maxY}]` + ); + } + + for (let attempt = 1; attempt <= 3; attempt++) { + await this.page.mouse.move(x, y); + await this.page.waitForTimeout(300); + await this.page.mouse.click(x, y, { button: 'right' }); + try { + await this.elementMenu.waitFor({ state: 'visible', timeout: 3000 }); + return; + } catch { + await this.page.waitForTimeout(1000); + } } + + throw new Error(`Element menu not visible after right-clicking at (${x}, ${y})`); }🤖 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 `@e2e/logic/POM/codeGraph.ts` around lines 877 - 892, rightClickAtNode is flakier than nodeClick because it does a single right-click attempt without validating the canvas bounds and checks menu state with isVisible rather than waiting for true visibility. Update rightClickAtNode to mirror the retry/bounds handling pattern used by nodeClick, and switch the menu assertion to a visibility wait on elementMenu so transient animation states do not cause false failures.
🤖 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 `@e2e/tests/elementMenu.spec.ts`:
- Around line 77-94: The element-menu removal test is asserting against the
total array length from getGraphNodes(), but that includes hidden nodes so it
may not change after removal. Update the test in elementMenu.spec.ts to validate
the affected node’s visible flag, or filter getGraphNodes() to only visible
nodes, using the existing CodeGraph helpers around rightClickAtCanvasCenter()
and clickOnRemoveNodeViaElementMenu() to locate the removed item.
---
Duplicate comments:
In `@app/src/components/code-graph.tsx`:
- Around line 224-262: The graph canvas updates in handleExpand, handleRemove,
and the “Unhide Nodes” path are not syncing the data state, leaving ForceGraph
callbacks working off stale data. After each mutation that changes
graph.Elements, also call setData(...) with the latest nodes/links (as Reset
Graph already does) before or alongside
canvasRef.current?.setGraphData(convertToCanvasData(graph.Elements)); use the
existing handleExpand, handleRemove, and unhide/reset-related logic in
code-graph.tsx to keep data and canvas aligned.
---
Nitpick comments:
In `@e2e/logic/POM/codeGraph.ts`:
- Around line 877-892: rightClickAtNode is flakier than nodeClick because it
does a single right-click attempt without validating the canvas bounds and
checks menu state with isVisible rather than waiting for true visibility. Update
rightClickAtNode to mirror the retry/bounds handling pattern used by nodeClick,
and switch the menu assertion to a visibility wait on elementMenu so transient
animation states do not cause false failures.
In `@e2e/tests/elementMenu.spec.ts`:
- Around line 39-40: The test is performing a redundant second right-click on
the same node: `codeGraph.nodeClick(...)` already opens the context menu and
waits for the “View Node” button, so the following
`codeGraph.rightClickAtNode(...)` is unnecessary. Update the flow in
`e2e/tests/elementMenu.spec.ts` to use only one of these actions (preferably
`nodeClick` if it already verifies the menu state) and keep the rest of the
assertions unchanged.
🪄 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: d0b81ea1-5d4e-4f2b-9661-39e3c174c829
📒 Files selected for processing (12)
app/src/App.tsxapp/src/components/chat.tsxapp/src/components/code-graph.tsxapp/src/components/elementMenu.tsxapp/src/components/graphView.tsxapp/src/components/model.tsapp/src/components/toolbar.tsxapp/src/lib/utils.tsapp/vite.config.tse2e/logic/POM/codeGraph.tse2e/tests/elementMenu.spec.tse2e/tests/toolbar.spec.ts
💤 Files with no reviewable changes (1)
- app/src/App.tsx
✅ Files skipped from review due to trivial changes (1)
- app/vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/components/elementMenu.tsx
- app/src/components/model.ts
- app/src/components/toolbar.tsx
- app/src/components/graphView.tsx
- app/src/components/chat.tsx
Comment on lines
+77
to
+94
| test("Verify element menu 'Remove' button removes node/link from canvas", async () => { | ||
| const codeGraph = await browser.createNewPage(CodeGraph, urls.baseUrl); | ||
| await browser.setPageToFullScreen(); | ||
| await codeGraph.selectGraph(GRAPHRAG_SDK); | ||
|
|
||
| // Get initial node count | ||
| const initialGraphData = await codeGraph.getGraphNodes(); | ||
| const initialNodeCount = initialGraphData.length; | ||
|
|
||
| // Right-click at canvas center and remove | ||
| await codeGraph.rightClickAtCanvasCenter(); | ||
| await codeGraph.clickOnRemoveNodeViaElementMenu(); | ||
|
|
||
| // Wait for animation and verify node count decreased | ||
| await codeGraph.waitForCanvasAnimationToEnd(); | ||
| const updatedGraphData = await codeGraph.getGraphNodes(); | ||
| expect(updatedGraphData.length).toBeLessThan(initialNodeCount); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getGraphNodes or the window graph getter filters by visibility
rg -n 'visible' e2e/logic/POM/codeGraph.ts | head -20
rg -n 'getGraphNodes\|graphDesktop\|graphMobile\|getActiveGraphData' e2e/logic/POM/codeGraph.tsRepository: FalkorDB/code-graph
Length of output: 801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the page object and the graph node getter implementation
fd -a 'codeGraph.ts' e2e
fd -a 'CodeGraph.ts' e2e
rg -n 'getGraphNodes|getActiveGraphData|graphDesktop|graphMobile|visible|rightClickAtCanvasCenter|clickOnRemoveNodeViaElementMenu' e2e -S
# Inspect the relevant sections with line numbers
for f in $(fd -a 'codeGraph.ts' e2e); do
echo "===== $f ====="
grep -nE 'getGraphNodes|getActiveGraphData|graphDesktop|graphMobile|visible|rightClickAtCanvasCenter|clickOnRemoveNodeViaElementMenu' "$f" || true
doneRepository: FalkorDB/code-graph
Length of output: 7137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the implementation of the graph getter and the right-click/remove helpers
sed -n '640,760p' e2e/logic/POM/codeGraph.ts
printf '\n===== canvas.spec.ts =====\n'
sed -n '240,320p' e2e/tests/canvas.spec.ts
printf '\n===== elementMenu.spec.ts =====\n'
sed -n '1,130p' e2e/tests/elementMenu.spec.tsRepository: FalkorDB/code-graph
Length of output: 14197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the remove action and any tests that exercise it
sed -n '500,560p' e2e/logic/POM/codeGraph.ts
printf '\n===== remove-related usages =====\n'
rg -n 'clickOnRemoveNodeViaElementMenu|Remove" button|visible=false|toBeLessThan|rightClickAtCanvasCenter' e2e/tests e2e/logic/POM -SRepository: FalkorDB/code-graph
Length of output: 5421
Assert visible instead of array length
getGraphNodes() keeps hidden nodes in the returned data, so removing an element won’t necessarily reduce updatedGraphData.length. This test should check the target node’s visible flag (or filter visible nodes) rather than expecting a smaller array.
🤖 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 `@e2e/tests/elementMenu.spec.ts` around lines 77 - 94, The element-menu removal
test is asserting against the total array length from getGraphNodes(), but that
includes hidden nodes so it may not change after removal. Update the test in
elementMenu.spec.ts to validate the affected node’s visible flag, or filter
getGraphNodes() to only visible nodes, using the existing CodeGraph helpers
around rightClickAtCanvasCenter() and clickOnRemoveNodeViaElementMenu() to
locate the removed item.
- Add aria-labels to animation, dimming, and pin toggle switches for accessibility - Add setData() calls after graph mutations in handleExpand and handleRemove to keep React state synced - Remove unused dimmed dependency from ForceGraph setConfig effect - All changes maintain existing code style and pass TypeScript compilation
- Move path zoom logic from graphView useEffect to chat.tsx for better stability - Remove unnecessary setData() calls - setGraphData() is sufficient for canvas updates - Cap zoom multiplier for path links (1.3x) since they take less visual space than nodes - All zoom handling now happens in chat.tsx where path responses originate
…om selected path - Remove default path selection on response - Add zoom to fit all paths (multiplier 1.2) when paths arrive - Add zoom to fit selected path nodes (multiplier 1.5) when path is clicked - Use setTimeout to ensure canvas is ready before zooming
- Use canvas.setData() only when graph identity changes (full reset) - Use canvas.setGraphData() for same-graph data updates (preserves positions) - Pass graphId from graph.Id through graphView to ForceGraph - Prevents unnecessary canvas remounting when data changes - Fixes z-index for element menu (z-[15] above toolbar, below data panel)
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
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
Bug Fixes