Fix #1270 Falkordb canvas #1293
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces react-force-graph-2d with a FalkorDB canvas, migrates graph model from wrapper objects to ID-based plain shapes with style-driven color/size/caption, threads Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Providers as App Providers
participant ForceCtx as ForceGraphContext
participant View as GraphView / Panels
participant Canvas as falkordb-canvas
participant Store as graphData
Providers->>ForceCtx: provide canvasRef, graphData, setGraphData, setIsLoading
ForceCtx-->>View: supply canvasRef, graphData, setters
View->>Canvas: mount <falkordb-canvas/> with canvasRef + graphData
Canvas->>Canvas: initialize engine & internal graph
Note over Canvas: User actions (click / hover / right-click / background)
Canvas-->>View: emit events (node/link click, hover, background)
View->>ForceCtx: update selection or call setGraphData(updated)
ForceCtx->>Store: setGraphData(updated)
Store-->>Canvas: Canvas receives new graphData and re-renders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
types/falkordb-canvas.d.ts (1)
6-13: Consider adding more specific prop types if applicable.The current typing uses generic
React.HTMLAttributes<HTMLElement>, which is functional for basic HTML attributes. If thefalkordb-canvasweb component accepts specific attributes or properties beyond standard HTML attributes, consider defining a more specific interface.However, if the component primarily uses imperative methods (like
setConfig(),getData()shown in the example), the current typing is appropriate.falkordb-canvas/falkordb-canvas.d.ts (1)
7-7: Use specific return type forgetGraph().The
getGraph()method returnsany, but the implementation infalkordb-canvas.tsreturnsForceGraphInstance. Use the specific type for better type safety.- getGraph(): any; + getGraph(): ForceGraphInstance;falkordb-canvas/falkordb-cnavas-utils.ts (2)
72-105: Duplicate key lookup logic.The same
Object.keys(nodeData).find()pattern appears twice (lines 78-81 and 93-96). Consider extracting to a helper.+const findKeyIgnoreCase = (obj: Record<string, unknown>, name: string) => + Object.keys(obj).find((k) => k.toLowerCase() === name.toLowerCase()); + export const getNodeDisplayText = ( node: Node, displayTextPriority: TextPriority[] ) => { const { data: nodeData } = node; const displayText = displayTextPriority.find(({ name, ignore }) => { - const key = ignore - ? Object.keys(nodeData).find( - (k) => k.toLowerCase() === name.toLowerCase() - ) - : name; + const key = ignore ? findKeyIgnoreCase(nodeData, name) : name; return ( key && nodeData[key] && typeof nodeData[key] === "string" && nodeData[key].trim().length > 0 ); }); if (displayText) { - const key = displayText.ignore - ? Object.keys(nodeData).find( - (k) => k.toLowerCase() === displayText.name.toLowerCase() - ) - : displayText.name; + const key = displayText.ignore + ? findKeyIgnoreCase(nodeData, displayText.name) + : displayText.name;
110-121: Document magic number for text height calculation.The value
1.125forhalfTextHeightappears to be derived from font metrics but isn't documented.+ // Approximate half text height based on 2px font size (see drawNode ctx.font) + const HALF_TEXT_HEIGHT_APPROX = 1.125; const ellipsis = "..."; const ellipsisWidth = ctx.measureText(ellipsis).width; - const halfTextHeight = 1.125; + const halfTextHeight = HALF_TEXT_HEIGHT_APPROX;app/components/ForceGraph.tsx (1)
189-193: Consider a type-safe helper for extracting link endpoint IDs.The
as anycasts handle the force-graph quirk wheresource/targetcan be either the resolvedGraphNodeobject or the original numeric ID. A small helper would improve clarity and type safety.const getLinkEndpointId = (endpoint: GraphNode | number): number => typeof endpoint === 'object' ? endpoint.id : endpoint;Then use:
-const targetId = typeof (link.target as any) === 'object' ? (link.target as any).id : link.target; -const sourceId = typeof (link.source as any) === 'object' ? (link.source as any).id : link.source; +const targetId = getLinkEndpointId(link.target as GraphNode | number); +const sourceId = getLinkEndpointId(link.source as GraphNode | number);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.eslintrc.json(1 hunks)app/api/graph/model.ts(10 hunks)app/components/ForceGraph.tsx(8 hunks)app/graph/GraphView.tsx(2 hunks)app/graph/page.tsx(3 hunks)app/graph/toolbar.tsx(5 hunks)app/schema/_page.tsx(3 hunks)falkordb-canvas/falkordb-canvas-types.ts(1 hunks)falkordb-canvas/falkordb-canvas.d.ts(1 hunks)falkordb-canvas/falkordb-canvas.example.html(1 hunks)falkordb-canvas/falkordb-canvas.ts(1 hunks)falkordb-canvas/falkordb-cnavas-utils.ts(1 hunks)lib/utils.ts(4 hunks)package.json(1 hunks)types/falkordb-canvas.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
falkordb-canvas/falkordb-canvas.d.tsfalkordb-canvas/falkordb-canvas.tsfalkordb-canvas/falkordb-cnavas-utils.tstypes/falkordb-canvas.d.tslib/utils.tsapp/api/graph/model.tsfalkordb-canvas/falkordb-canvas-types.ts
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/toolbar.tsxapp/schema/_page.tsxapp/graph/GraphView.tsxapp/graph/page.tsxapp/components/ForceGraph.tsx
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/graph/model.ts
🧬 Code graph analysis (5)
app/schema/_page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
falkordb-canvas/falkordb-cnavas-utils.ts (3)
app/api/graph/model.ts (6)
Data(108-108)Data(400-402)Data(404-406)GraphData(79-82)Node(53-63)Link(65-77)falkordb-canvas/falkordb-canvas-types.ts (7)
Data(68-71)GraphData(53-56)GraphNode(21-37)GraphLink(39-51)Node(58-61)Link(63-66)TextPriority(73-76)lib/utils.ts (1)
TextPriority(25-28)
lib/utils.ts (2)
falkordb-canvas/falkordb-canvas-types.ts (3)
ForceGraphInstance(80-80)GraphNode(21-37)Node(58-61)app/api/graph/model.ts (1)
Node(53-63)
app/api/graph/model.ts (1)
falkordb-canvas/falkordb-canvas-types.ts (2)
Node(58-61)Link(63-66)
app/graph/page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup and cache dependencies
🔇 Additional comments (29)
.eslintrc.json (1)
43-45: LGTM!Excluding declaration files from ESLint is appropriate, especially with the new TypeScript declaration files added in this PR.
app/graph/GraphView.tsx (2)
110-110: LGTM! Safer access pattern for node visibility.The change from
link.source.visibletograph.NodesMap.get(link.source)?.visiblecorrectly adapts to the new data model where source/target are numeric IDs rather than object references. The optional chaining provides additional safety.
133-133: LGTM! Improved type guard.Using
"labels" in eis a more robust type guard than!!e.labelsfor distinguishing between Node and Link types in TypeScript.app/schema/_page.tsx (3)
4-4: LGTM! Import aligns with new graph reference type.The addition of
GraphRefimport supports the transition away fromForceGraphMethodsto the new canvas-based architecture.
44-44: LGTM! Type change aligns with new architecture.The change from
ForceGraphMethodstoGraphRef["current"]is consistent with the new canvas-based graph rendering system introduced across this PR.
165-165: LGTM! Improved type narrowing.Using
!("source" in element)is a more robust way to distinguish between Node and Link types compared to checking the truthiness ofelement.source. This pattern is consistent with similar changes throughout the PR.app/graph/page.tsx (3)
4-4: LGTM! Consistent with schema page changes.The
GraphRefimport supports the architectural transition to the new canvas-based rendering system.
70-70: LGTM! Type change aligns with new architecture.Consistent with the changes in
app/schema/_page.tsx, this adopts the newGraphReftype for canvas-based graph rendering.
248-248: LGTM! Improved type guard.Using the
"in"operator for type narrowing is more robust and consistent with the pattern established inapp/schema/_page.tsx.falkordb-canvas/falkordb-canvas.example.html (1)
1-221: LGTM! Well-structured example demonstrates the web component API.This example HTML file effectively demonstrates how to use the FalkorDB Force Graph web component, including configuration, event handling, and dynamic graph manipulation. The code is clear and serves as good documentation for developers integrating the component.
Note: Confirm this file is intended as documentation/example code and not accidentally included as production code (its location in
falkordb-canvas/suggests it's intentional).app/graph/toolbar.tsx (3)
92-93: LGTM! Safer property access in search filter.Using the
"in"operator to check forrelationshipandlabelsproperties before accessing them provides better type safety and prevents potential runtime errors.
108-108: LGTM! Type-safe element detection for zoom.The use of
"labels" in elementcorrectly distinguishes between Node and Link types, enabling appropriate zoom behavior for each.
225-225: LGTM! Proper type narrowing with safe casts.The pattern of using
"source" in suggestionas a type guard on line 225, followed by safe type casts on lines 243 and 253, is a correct TypeScript approach. The casts are safe because they follow the runtime type check.Also applies to: 243-243, 253-253
package.json (1)
62-62: Theforce-graphpackage at version 1.51.0 exists and has no known security vulnerabilities. No action required.lib/utils.ts (1)
384-386: Improved type guard usinginoperator.The change from
!!e.labelsto"labels" in eis more robust. It correctly checks for property existence rather than truthiness, avoiding false negatives whenlabelsis an empty array.falkordb-canvas/falkordb-canvas.d.ts (1)
12-22: JSX intrinsic element augmentation looks correct.The global JSX namespace extension properly maps the custom element tag to React-compatible props.
falkordb-canvas/falkordb-canvas.ts (3)
68-73: Good cleanup indisconnectedCallback.Properly destroying the graph instance when the element is removed from the DOM prevents memory leaks.
287-291: Mutating node coordinates with fallback to zero.The fallback to
0for undefined coordinates is reasonable for initial render, but be aware this places unpositioned nodes at the origin, which may cause visual overlap until the simulation runs.
453-458: Good SSR-safe custom element registration.The
typeof window !== "undefined"check prevents errors during server-side rendering.falkordb-canvas/falkordb-cnavas-utils.ts (1)
52-70: Clean bidirectional data conversion.The
graphDataToDatafunction properly strips runtime properties and converts object references back to numeric IDs.app/api/graph/model.ts (4)
457-488:calculateLinkCurvecorrectly updated for numeric IDs.The comparison logic now properly uses numeric
sourceandtargetvalues instead of object references.
798-830:visibleLinksandremoveLinksuse correct numeric comparisons.The methods properly use
link.sourceandlink.targetas numeric IDs with.find()lookups against node IDs.
903-904: Type guards updated consistently.The filters correctly use
"labels" in nfor nodes and"source" in lfor links, consistent with the updated type structure.
53-77: Breaking change: Link type now uses numericsource/targetinstead of object references.The
Linktype has been updated to use numeric IDs forsourceandtarget(lines 69-70) rather than object references. This aligns with the canvas-based rendering approach. All internal usages inmodel.ts(lines 458, 798-830) and consumers inGraphView.tsx,toolbar.tsx, andForceGraph.tsxhave been updated to handle numeric ID comparisons correctly.Note:
ForceGraph.tsxincludes defensive type guards (lines 190-191) to handle both object and numeric types—ensure this defensive logic remains if backward compatibility is needed.falkordb-canvas/falkordb-canvas-types.ts (1)
1-80: LGTM! Well-structured type definitions.The types clearly distinguish between the simplified input types (
Node,Link,Data) and the canvas-runtime types (GraphNode,GraphLink,GraphData) with physics properties. Theeslint-disableforanyon the data payload is acceptable for flexible graph metadata.app/components/ForceGraph.tsx (4)
291-314: Configuration sync looks correct.The callbacks are properly memoized via
useCallback, so the frequentsetConfigcalls are intentional and necessary to keep the canvas in sync with React state.
37-54: Clean data transformation.The explicit property mapping ensures only canvas-relevant fields are passed through, avoiding accidental leakage of internal model properties.
234-270: Handlers correctly bridge canvas and model elements.The lookups via
graph.NodesMapandgraph.LinksMapproperly translate the lightweight canvas types back to full model objects. The'source' in elementcheck is a reliable discriminator.
324-334: LGTM! Clean web component integration.The
<falkordb-canvas>custom element with ref pattern is the correct approach for integrating web components in React.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/providers.tsx (1)
206-211: Missing dependencies in useMemo for viewport context.The dependency array
[viewport, data]is incomplete. The context object includessetViewportandsetData, which should also be included:[viewport, data, setViewport, setData]. WhileuseStatesetters are typically stable, including all values from the closure in the dependency array is required for correctness and follows React hooks best practices.Also, the comment about removing
isSavedappears to be incorrect—isSaveddoesn't exist in theViewportContextTypedefinition and no consumers reference it.
♻️ Duplicate comments (5)
falkordb-canvas/falkordb-canvas.ts (3)
13-18: Typo in import path.Already noted in previous review - filename should be
falkordb-canvas-utils.tsnotfalkordb-cnavas-utils.ts.
57-58: Uninitializedgraphproperty.Already noted in previous review - consider initializing or adding null checks.
104-106: Type alignment with declaration file.Already noted in previous review - ensure
getData()/setData()types match between.d.tsand implementation.lib/utils.ts (1)
21-21: GraphRef type correctly permits undefined via ForceGraphInstance.The past review concern about removing the
undefinedunion is addressed by theForceGraphInstancetype definition itself (line 82 in falkordb-canvas-types.ts), which includes| undefined. Consumers can safely accesschartRef.currentwith appropriate null checks, as demonstrated in line 229 of this file.app/components/ForceGraph.tsx (1)
86-94: Fix useEffect dependency array: should be empty.The past review comment is correct. This effect initializes
chartRef.currentfrom the canvas instance after mount, but the dependency array[chartRef, data]causes it to re-run on every data change. Refs are stable objects and should not be in dependency arrays. Additionally, re-initializing the ref on data changes is unnecessary and could cause issues.}, [chartRef, data]) + }, [])
🧹 Nitpick comments (5)
app/schema/DataPanel.tsx (1)
69-72: Consider removingtypefrom dependency array.Since
typeis computed fromobjecton every render (Line 51), including both in the dependency array is redundant. Whenobjectchanges,typewill be recalculated automatically.- }, [object, type]) + }, [object])falkordb-canvas/falkordb-canvas.ts (1)
583-623: Event handlers are duplicated betweeninitGraphandupdateEventHandlers.The same event handler registrations appear in both
initGraph(Lines 263-304) andupdateEventHandlers. Consider extracting to a single method called from both places.private initGraph() { // ... graph setup ... - .onNodeClick((node: GraphNode, event: MouseEvent) => { - if (this.config.onNodeClick) { - this.config.onNodeClick(node, event); - } - }) - // ... other handlers ... + this.updateEventHandlers(); this.setupForces(); }falkordb-canvas/falkordb-canvas-types.ts (1)
60-73: Type name collision: Node, Link, and Data conflict with model types.This file exports
Node,Link, andDatatypes that have different shapes than the identically-named types inapp/api/graph/model.ts. This creates ambiguity—consumers must use path-specific imports to avoid confusion, and TypeScript errors may occur if the wrong type is imported.Consider prefixing these types to clarify their purpose:
-export type Node = Omit< +export type CanvasNode = Omit< GraphNode, "x" | "y" | "vx" | "vy" | "fx" | "fy" | "displayName" >; -export type Link = Omit<GraphLink, "curve" | "source" | "target"> & { +export type CanvasLink = Omit<GraphLink, "curve" | "source" | "target"> & { source: number; target: number; }; -export interface Data { - nodes: Node[]; - links: Link[]; +export interface CanvasData { + nodes: CanvasNode[]; + links: CanvasLink[]; }Then update consumers (e.g.,
ForceGraph.tsxline 10 and line 33) accordingly.app/components/ForceGraph.tsx (2)
108-108: Remove chartRef from useEffect dependencies.Including
chartRefin the dependency array is unnecessary because refs are stable objects that don't trigger re-renders. The effect should depend only on values that actually change.- }, [chartRef, graph.Id, currentTab, graph.Elements.nodes.length, isLoading]) + }, [graph.Id, currentTab, graph.Elements.nodes.length, isLoading])
159-163: Simplify link filtering with proper type handling.The type casting
(link.target as any)and(link.source as any)suggests uncertainty about the runtime shape of link source/target. This is error-prone and defeats TypeScript's type safety.- const isTarget = graph.Elements.links.some(link => { - const targetId = typeof (link.target as any) === 'object' ? (link.target as any).id : link.target; - const sourceId = typeof (link.source as any) === 'object' ? (link.source as any).id : link.source; - return targetId === node.id && nodes.some(n => n.id === sourceId); - }); + const isTarget = graph.Elements.links.some(link => + link.target === node.id && nodes.some(n => n.id === link.source) + );If links can have both numeric IDs and object references, add a helper:
const getLinkId = (ref: number | { id: number }): number => typeof ref === 'object' ? ref.id : ref;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/components/ForceGraph.tsx(7 hunks)app/components/provider.ts(0 hunks)app/graph/CreateElementPanel.tsx(1 hunks)app/graph/DataPanel.tsx(1 hunks)app/graph/DataTable.tsx(1 hunks)app/graph/GraphView.tsx(4 hunks)app/graph/Selector.tsx(1 hunks)app/graph/page.tsx(4 hunks)app/providers.tsx(1 hunks)app/schema/CreateElementPanel.tsx(1 hunks)app/schema/DataPanel.tsx(2 hunks)app/schema/SchemaView.tsx(1 hunks)app/schema/_page.tsx(3 hunks)falkordb-canvas/falkordb-canvas-types.ts(1 hunks)falkordb-canvas/falkordb-canvas.d.ts(1 hunks)falkordb-canvas/falkordb-canvas.ts(1 hunks)lib/utils.ts(4 hunks)
💤 Files with no reviewable changes (1)
- app/components/provider.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/Selector.tsxapp/graph/DataTable.tsxapp/schema/DataPanel.tsxapp/graph/CreateElementPanel.tsxapp/schema/SchemaView.tsxapp/schema/_page.tsxapp/graph/page.tsxapp/components/ForceGraph.tsxapp/providers.tsxapp/schema/CreateElementPanel.tsxapp/graph/DataPanel.tsxapp/graph/GraphView.tsx
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
falkordb-canvas/falkordb-canvas.tsfalkordb-canvas/falkordb-canvas.d.tsfalkordb-canvas/falkordb-canvas-types.tslib/utils.ts
🧬 Code graph analysis (8)
falkordb-canvas/falkordb-canvas.ts (3)
falkordb-canvas/falkordb-canvas-types.ts (5)
GraphData(55-58)ForceGraphConfig(3-21)Data(70-73)GraphNode(23-39)GraphLink(41-53)app/api/graph/model.ts (4)
GraphData(79-82)Data(108-108)Data(400-402)Data(404-406)falkordb-canvas/falkordb-cnavas-utils.ts (4)
graphDataToData(52-70)dataToGraphData(15-46)getNodeDisplayText(72-105)wrapTextForCircularNode(110-166)
falkordb-canvas/falkordb-canvas.d.ts (2)
falkordb-canvas/falkordb-canvas-types.ts (3)
ForceGraphConfig(3-21)Data(70-73)ForceGraphInstance(82-82)app/api/graph/model.ts (3)
Data(108-108)Data(400-402)Data(404-406)
app/schema/_page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
app/graph/page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
falkordb-canvas/falkordb-canvas-types.ts (2)
lib/utils.ts (1)
TextPriority(25-28)app/api/graph/model.ts (6)
GraphData(79-82)Node(53-63)Link(65-77)Data(108-108)Data(400-402)Data(404-406)
app/schema/CreateElementPanel.tsx (2)
falkordb-canvas/falkordb-canvas-types.ts (1)
Node(60-63)app/api/graph/model.ts (1)
Node(53-63)
lib/utils.ts (2)
falkordb-canvas/falkordb-canvas-types.ts (3)
ForceGraphInstance(82-82)GraphNode(23-39)Node(60-63)app/api/graph/model.ts (1)
Node(53-63)
app/graph/GraphView.tsx (1)
app/components/provider.ts (1)
ViewportContext(281-286)
🔇 Additional comments (23)
app/graph/DataTable.tsx (1)
1-662: LGTM! Clean removal of display text priority logic.The removal of
BrowserSettingsContextdependency andgetNodeDisplayKeyhelper function aligns with the broader refactor to move display text responsibilities to the FalkorDB canvas utilities. This simplifies the component's concerns.app/graph/page.tsx (2)
70-70: LGTM! Improved type safety with GraphRef.The use of
GraphRef["current"]provides better type safety compared to the previousForceGraphMethods-based typing, aligning with the canvas-based rendering changes.
246-246: LGTM! Safer type guard with property existence check.The change from
!element.sourceto!("source" in element)is a more robust way to distinguish between Node and Link types. This structural check is consistent with similar updates across the codebase and avoids potential issues with falsy values.app/graph/Selector.tsx (1)
619-619: LGTM! Consistent type guard improvement.The change to
"labels" in eprovides a safer structural check for Node types. The logic correctly ensures edge creation is only enabled when exactly 2 nodes are selected.app/graph/CreateElementPanel.tsx (1)
11-12: LGTM! Import reorganization aligns with canvas utilities.The split of imports—keeping
cninlib/utilsand movinggetNodeDisplayTexttofalkordb-canvas/falkordb-cnavas-utils—is consistent with the broader refactor moving display text responsibilities to the FalkorDB canvas layer.app/schema/CreateElementPanel.tsx (1)
636-637: LGTM! Improved type narrowing for node swap.The change to
"labels" in eprovides safer type narrowing when filtering to Node types before swapping their order. This structural check is more reliable than truthiness checks and aligns with similar updates across the codebase.app/graph/DataPanel.tsx (1)
37-37: LGTM! Safer type determination.The change from
!object.sourceto!("source" in object)is a more reliable way to distinguish between Node and Link types. This structural check avoids potential issues with falsy values and aligns with the broader pattern of safer type guards across the PR.app/schema/SchemaView.tsx (1)
5-5: LGTM! Simplified state management.The removal of
useStateand parent dimension state management aligns with the transition to canvas-based rendering, where ForceGraph no longer requires parent width/height props. This simplifies the component.app/schema/DataPanel.tsx (1)
51-51: Safer type discrimination usinginoperator.This correctly identifies Node vs Link by checking property existence rather than truthiness. Handles edge cases where
sourcecould be0or another falsy value.app/schema/_page.tsx (2)
44-44: Ref typing is consistent withGraphRefdefinition.Using
GraphRef["current"]extracts theForceGraphInstancetype correctly. The uninitialized ref aligns with optional graph initialization.
165-165: Consistent type guard pattern.Same safer
"source" in elementcheck used across the codebase for Node vs Link discrimination.app/graph/GraphView.tsx (4)
5-5: Import cleanup looks good.Removed unused
useStateafter removing parentHeight/parentWidth state variables.
63-63: Context destructuring aligns with provider changes.Removed
isSavedfrom ViewportContext, consistent withapp/components/provider.tsupdates.
108-108: Correct adaptation to new Link type.Since
Link.sourceandLink.targetare now numeric IDs, usinggraph.NodesMap.get(link.source)is the correct way to retrieve the actual Node object for visibility checks.
131-131: Safer type guard for element discrimination.Using
"labels" in eproperly checks for Node type membership instead of relying on truthiness.falkordb-canvas/falkordb-canvas.ts (3)
228-308: Well-structured graph initialization with force simulation.The force constants, collision handling, and rendering setup are comprehensive. The degree-based link distance/strength adjustments handle crowded graphs nicely.
385-430: Node rendering with text wrapping looks solid.Correctly handles missing coordinates, caches display names, and wraps text for circular nodes.
658-661: Safe custom element registration.Good defensive check for SSR environments and duplicate registration.
lib/utils.ts (1)
385-385: LGTM: Type guard correctly identifies nodes.The
"labels" in echeck properly distinguishes nodes from links, as GraphNode haslabelswhile GraphLink hasrelationship.falkordb-canvas/falkordb-canvas-types.ts (2)
23-39: LGTM: GraphNode type properly extends NodeObject.The type correctly combines force-graph's
NodeObjectwith domain-specific properties and optional simulation coordinates.
3-21: LGTM: Comprehensive configuration interface.The
ForceGraphConfiginterface provides a well-structured API for configuring the canvas, including event handlers, styling, and simulation controls.app/components/ForceGraph.tsx (2)
33-50: LGTM: Data conversion correctly maps between type systems.The
convertToCanvasDatafunction properly transformsGraphData(model types with expand/collapsed) toData(canvas types), omitting simulation-specific properties.
257-279: Large dependency array is acceptable for reactive configuration.While the dependency array is extensive, each dependency is legitimately referenced in the configuration object. The callbacks (
handleNodeClick, etc.) depend on state, so frequent updates are expected. This is acceptable for maintaining correct canvas configuration.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 (3)
app/schema/DataPanel.tsx (1)
51-72: Avoid non-narrowing union access (object.labels/object.relationship).Use explicit casts (or a shared type guard) so this doesn’t rely on structural typing quirks.
-const type = !("source" in object) +const type = !("source" in object) useEffect(() => { setAttributes(Object.entries(object.data).filter(([key, val]) => !(key === "name" && Number(val) === object.id)).map(([key, val]) => [key, Array.isArray(val) ? val : (val as string).split(',')])) - setLabel(type ? [...object.labels] : [object.relationship]) + setLabel(type ? [...(object as Node).labels] : [(object as Link).relationship]) }, [object, type])app/schema/_page.tsx (1)
4-45: StandardizechartReftype to prevent undefined ref from being passed as non-undefined prop.
chartRefis declared asuseRef<GraphRef["current"]>()(which defaults toMutableRefObject<ForceGraphInstance | undefined>), but all consuming components expectchartRef: GraphRefwhereGraphRef = MutableRefObject<ForceGraphInstance>. This creates a type safety gap.Update
lib/utils.tsto allownull:-export type GraphRef = MutableRefObject<ForceGraphInstance>; +export type GraphRef = MutableRefObject<ForceGraphInstance | null>;Then initialize
chartRefwithnullinapp/schema/_page.tsxandapp/graph/page.tsx:-const chartRef = useRef<GraphRef["current"]>() +const chartRef = useRef<GraphRef["current"]>(null)app/graph/page.tsx (1)
4-71: FixchartRefinitialization to use explicit null and proper type union.The current initialization
useRef<GraphRef["current"]>()defaults toundefinedand misses the null initialization pattern used throughout the codebase. This ref is passed to components expecting a properly typed ref object and should initialize withnullfor consistency and type safety:-const chartRef = useRef<GraphRef["current"]>() +const chartRef = useRef<GraphRef["current"] | null>(null)This pattern matches how other refs in the app are initialized (e.g.,
panelRef,editorRef,parentRef) and aligns with strict type checking.
♻️ Duplicate comments (9)
falkordb-canvas/falkordb-canvas.d.ts (1)
12-22: JSX augmentation enables React TypeScript support.The global
JSX.IntrinsicElementsaugmentation correctly maps the'falkordb-canvas'tag name toFalkorDBForceGraph, enabling proper TypeScript support in React components.Note: A previous review identified a duplicate JSX augmentation in
types/falkordb-canvas.d.tsthat should be removed to avoid type conflicts.lib/utils.ts (2)
223-243: Shadow DOM access is fragile and tightly coupled.Directly querying
document.querySelector("falkordb-canvas")and accessing its shadow DOM assumes a single canvas instance and specific internal structure.A previous review suggested adding a public API method like
getCanvasDimensions()to the canvas component to avoid breaking encapsulation. This approach would be more robust and handle multiple instances correctly.
21-21: GraphRef type no longer permitsundefined.The previous type allowed
undefined, but the newGraphReftype requiresForceGraphInstanceto always be present. This could cause runtime issues ifchartRef.currentis accessed before initialization.A previous review suggested restoring the optional behavior:
MutableRefObject<ForceGraphInstance | undefined>. Alternatively, ensure all consumers null-check before accessing.falkordb-canvas/falkordb-canvas.ts (3)
13-18: Import path contains typo.The import path
"./falkordb-cnavas-utils"has a typo ("cnavas" should be "canvas"). This should be corrected for clarity.Note: A previous review identified this typo. Consider renaming the file from
falkordb-cnavas-utils.tstofalkordb-canvas-utils.tsand updating all references.
57-83: Uninitializedgraphproperty may cause runtime errors.The
graphproperty is declared but not initialized, andgetGraph()returns it without null checks. If called beforeconnectedCallback()completes, it will returnundefined.A previous review suggested making the property nullable or adding initialization guards. Consider updating the type to
ForceGraphInstance | null = nulland adding appropriate checks ingetGraph().
89-94: Clean up timeout on disconnect to prevent memory leaks.The
handleEngineStopmethod schedules asetTimeout(line 573) that could fire after the component is disconnected, leading to memory leaks or stale references.A previous review suggested tracking the timeout ID and clearing it in
disconnectedCallback(). Add a property for the timeout ID and clear it here.falkordb-canvas/falkordb-cnavas-utils.ts (1)
15-46: Non-null assertions could cause runtime errors.If a link references a node ID that doesn't exist in the nodes array (lines 39-40),
sourceNodeortargetNodewill beundefined, and the non-null assertion (!) will mask the error.A previous review suggested adding validation or filtering:
- Skip links with missing endpoints
- Throw a descriptive error with the missing node IDs
- Log a warning for debugging
This would make data integrity issues discoverable rather than causing silent failures downstream.
falkordb-canvas/falkordb-canvas-types.ts (1)
75-78: Remove duplicate TextPriority type definition.This type is already defined in
lib/utils.ts(lines 25-28). Duplicating it creates maintenance burden and potential for inconsistency.A previous review suggested removing this definition and importing from
lib/utils.tsinstead:+import { TextPriority } from "@/lib/utils"; + export interface ForceGraphConfig { // ... } // ... rest of file ... -export type TextPriority = { - name: string; - ignore: boolean; -};app/components/ForceGraph.tsx (1)
85-95: InitializechartRef.currentonce (removedatadep); current deps can thrash and still race element readiness.This is the same underlying issue as the earlier review: depending on
datawill re-run the initialization even though the graph instance should be stable, and it may still run before the web component is ready.useEffect(() => { if (canvasRef.current) { const forceGraph = canvasRef.current.getGraph(); if (forceGraph) { chartRef.current = forceGraph; } } - }, [chartRef, data]) + }, [])If readiness is async, consider wiring an explicit
onReady/event from the web component instead of polling via React effects.
🧹 Nitpick comments (6)
types/falkordb-canvas.d.ts (1)
6-13: Consider adding custom props interface for better type safety.The current declaration allows
<falkordb-canvas>in JSX but doesn't expose any custom attributes or event handlers specific to this web component. If the component accepts custom props (e.g.,config,data, event callbacks), consider extending the interface:declare namespace JSX { interface IntrinsicElements { 'falkordb-canvas': React.DetailedHTMLProps< - React.HTMLAttributes<HTMLElement>, - HTMLElement + React.HTMLAttributes<HTMLElement> & { + ref?: React.Ref<FalkorDBForceGraph>; + // Add custom props here if needed + }, + FalkorDBForceGraph >; } } + +declare class FalkorDBForceGraph extends HTMLElement { + setConfig(config: Partial<import('../falkordb-canvas/falkordb-canvas-types').ForceGraphConfig>): void; + getData(): import('../falkordb-canvas/falkordb-canvas-types').Data; + setData(data: import('../falkordb-canvas/falkordb-canvas-types').Data): void; + getGraph(): import('force-graph').ForceGraphInstance | undefined; +}app/api/graph/model.ts (2)
793-816: Consider optimizing node lookups for better performance.The repeated
.find()calls inside the loop create O(n×m) complexity. For larger graphs, pre-compute visible node IDs:public visibleLinks(visible: boolean) { + const visibleNodeIds = new Set( + this.elements.nodes.filter((n) => n.visible).map((n) => n.id) + ); + this.elements.links.forEach((link) => { + const sourceVisible = visibleNodeIds.has(link.source); + const targetVisible = visibleNodeIds.has(link.target); + if ( this.RelationshipsMap.get(link.relationship)!.show && visible && - this.elements.nodes.find((n) => n.id === link.source)?.visible && - this.elements.nodes.find((n) => n.id === link.target)?.visible + sourceVisible && + targetVisible ) { link.visible = true; } if ( !visible && - (this.elements.nodes.find((n) => n.id === link.source)?.visible === - false || - this.elements.nodes.find((n) => n.id === link.target)?.visible === - false) + (!sourceVisible || !targetVisible) ) { link.visible = false; } }); }
818-859: Avoid repeated.map().includes()inside filter.Creating a new array on every iteration is inefficient. Pre-compute the node ID set:
public removeLinks(ids: number[] = []): Relationship[] { + const nodeIds = new Set(this.elements.nodes.map((n) => n.id)); const links = this.elements.links.filter( (link) => ids.includes(link.source) || ids.includes(link.target) ); this.elements = { nodes: this.elements.nodes, links: this.elements.links .map((link) => { if ( (ids.length !== 0 && !links.includes(link)) || - (this.elements.nodes.map((n) => n.id).includes(link.source) && - this.elements.nodes.map((n) => n.id).includes(link.target)) + (nodeIds.has(link.source) && nodeIds.has(link.target)) ) { return link; } // ... rest unchangedapp/schema/SchemaView.tsx (1)
5-5:handleCooldowntyping looks overly restrictive (ticks?: 0).If
handleCooldownis intended to accept only0 | undefinedto “stop”, rename it to reflect that (e.g.,stopCooldown) or widen the type to what callers may reasonably pass (commonlynumber | undefined) to avoid accidental misuse as the canvas API evolves.app/graph/Selector.tsx (1)
610-624: Prefer a shared type guard over inline"labels" in e"checks.This is correct, but consider using a helper (e.g.,
isNode(el): el is Node) to keep the runtime/type-narrowing behavior consistent across Schema/Graph UIs.app/schema/CreateElementPanel.tsx (1)
635-638: Guard against unexpected selection shape before swapping.Even if the UI should only allow 2 nodes,
nodes[1]can still beundefinedif state gets out of sync. Consider a defensive fallback.onClick={() => setSelectedNodes && setSelectedNodes(prev => { const nodes = prev.filter((e): e is Node => "labels" in e) - return [nodes[1], nodes[0]] + if (nodes.length !== 2) return prev + return [nodes[1], nodes[0]] })}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.eslintrc.json(1 hunks)app/api/graph/model.ts(10 hunks)app/components/ForceGraph.tsx(7 hunks)app/components/provider.ts(0 hunks)app/graph/CreateElementPanel.tsx(1 hunks)app/graph/DataPanel.tsx(1 hunks)app/graph/DataTable.tsx(1 hunks)app/graph/GraphView.tsx(4 hunks)app/graph/Selector.tsx(1 hunks)app/graph/page.tsx(4 hunks)app/graph/toolbar.tsx(5 hunks)app/providers.tsx(1 hunks)app/schema/CreateElementPanel.tsx(1 hunks)app/schema/DataPanel.tsx(2 hunks)app/schema/SchemaView.tsx(1 hunks)app/schema/_page.tsx(3 hunks)falkordb-canvas/falkordb-canvas-types.ts(1 hunks)falkordb-canvas/falkordb-canvas.d.ts(1 hunks)falkordb-canvas/falkordb-canvas.example.html(1 hunks)falkordb-canvas/falkordb-canvas.ts(1 hunks)falkordb-canvas/falkordb-cnavas-utils.ts(1 hunks)lib/utils.ts(4 hunks)package.json(1 hunks)types/falkordb-canvas.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/components/provider.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/DataTable.tsxapp/schema/DataPanel.tsxapp/graph/DataPanel.tsxapp/graph/toolbar.tsxapp/schema/CreateElementPanel.tsxapp/graph/Selector.tsxapp/schema/SchemaView.tsxapp/providers.tsxapp/graph/page.tsxapp/schema/_page.tsxapp/components/ForceGraph.tsxapp/graph/CreateElementPanel.tsxapp/graph/GraphView.tsx
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
falkordb-canvas/falkordb-cnavas-utils.tslib/utils.tsfalkordb-canvas/falkordb-canvas.d.tsfalkordb-canvas/falkordb-canvas-types.tsapp/api/graph/model.tsfalkordb-canvas/falkordb-canvas.tstypes/falkordb-canvas.d.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/graph/model.ts
🧬 Code graph analysis (11)
app/graph/toolbar.tsx (2)
falkordb-canvas/falkordb-canvas-types.ts (2)
Link(65-68)Node(60-63)app/api/graph/model.ts (2)
Link(65-77)Node(53-63)
app/schema/CreateElementPanel.tsx (2)
falkordb-canvas/falkordb-canvas-types.ts (1)
Node(60-63)app/api/graph/model.ts (1)
Node(53-63)
app/graph/page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
lib/utils.ts (2)
falkordb-canvas/falkordb-canvas-types.ts (3)
ForceGraphInstance(82-82)GraphNode(23-39)Node(60-63)app/api/graph/model.ts (1)
Node(53-63)
app/schema/_page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
falkordb-canvas/falkordb-canvas.d.ts (2)
falkordb-canvas/falkordb-canvas-types.ts (3)
ForceGraphConfig(3-21)Data(70-73)ForceGraphInstance(82-82)app/api/graph/model.ts (3)
Data(108-108)Data(400-402)Data(404-406)
falkordb-canvas/falkordb-canvas-types.ts (2)
lib/utils.ts (1)
TextPriority(25-28)app/api/graph/model.ts (6)
GraphData(79-82)Node(53-63)Link(65-77)Data(108-108)Data(400-402)Data(404-406)
app/api/graph/model.ts (1)
falkordb-canvas/falkordb-canvas-types.ts (2)
Node(60-63)Link(65-68)
app/components/ForceGraph.tsx (5)
falkordb-canvas/falkordb-canvas-types.ts (6)
GraphData(55-58)Data(70-73)Node(60-63)Link(65-68)GraphNode(23-39)GraphLink(41-53)app/api/graph/model.ts (6)
GraphData(79-82)Data(108-108)Data(400-402)Data(404-406)Node(53-63)Link(65-77)app/components/provider.ts (2)
IndicatorContext(266-269)BrowserSettingsContext(155-214)lib/utils.ts (1)
getTheme(366-380)falkordb-canvas/falkordb-canvas.ts (1)
setData(108-113)
falkordb-canvas/falkordb-canvas.ts (3)
falkordb-canvas/falkordb-canvas-types.ts (5)
ForceGraphInstance(82-82)GraphData(55-58)Data(70-73)GraphNode(23-39)GraphLink(41-53)app/api/graph/model.ts (4)
GraphData(79-82)Data(108-108)Data(400-402)Data(404-406)app/components/ForceGraph.tsx (1)
ForceGraph(52-294)
app/graph/GraphView.tsx (1)
app/components/provider.ts (1)
ViewportContext(281-286)
🔇 Additional comments (38)
.eslintrc.json (1)
43-45: LGTM!Excluding
*.d.tsfiles from ESLint is a sensible choice—declaration files often contain ambient type definitions that don't conform to typical linting rules. This aligns well with the new canvas type declarations added in this PR.app/providers.tsx (1)
206-211: LGTM!The dependency array correctly reflects the context value after removing
isSaved. This aligns with the broader refactor moving cooldown handling out of the viewport context.app/graph/DataTable.tsx (1)
15-15: LGTM!Clean removal of
BrowserSettingsContextimport—consistent with dropping the display text priority logic that's no longer needed with the new canvas-based approach.app/api/graph/model.ts (3)
53-77: LGTM! Clean type simplification.The migration from wrapper classes to plain object shapes with numeric IDs for
source/targetis a solid choice. This improves serialization, aligns with the canvas component's data model, and reduces complexity.
861-904: Type discrimination approach works but is coupled to type shape.Using
"labels" in elementand"source" in elementfor type guards is functional given the current type definitions. Consider adding type guard utilities inlib/utils.ts(e.g.,isNode(element),isLink(element)) to centralize this logic and make future type changes safer.
457-488: LGTM!The link curve calculation is cleaner with direct numeric ID comparisons. Self-loop handling (
start === end) and bidirectional link detection work correctly.package.json (1)
62-62: Confirm bothforce-graphandreact-force-graph-2dare needed long-term.The version
^1.51.0is current (latest on npm) with no known security concerns. However, verify whether both packages should coexist permanently—force-graph(the lower-level canvas library) alongsidereact-force-graph-2d(line 77) suggests a transition phase. Clarify the long-term dependency strategy in the PR description.app/schema/_page.tsx (1)
163-170: Good: element type detection via property presence.
const type = !("source" in element)is a safer discriminator than falsy checks (e.g.,!element.source).app/graph/page.tsx (2)
197-231: Good:handleCreateElementno longer depends onhandleCooldown.This should reduce unnecessary callback re-creation while keeping the logic intact.
245-251: Good: element type detection via"source" in element".This aligns with the refactor to primitive link/node shapes and avoids falsy pitfalls.
app/graph/DataPanel.tsx (1)
35-59: Good: safer node/link discrimination with"source" in object".Also nice that label derivation keeps the cast on the node branch.
falkordb-canvas/falkordb-canvas.example.html (3)
94-108: LGTM! Sample data structure is correct.The node and link shapes match the
Datatype fromfalkordb-canvas-types.ts, with numeric source/target references and proper initialization of display properties.
116-157: LGTM! Configuration setup is correct.The configuration object matches the
ForceGraphConfiginterface, with proper event handler signatures and display text priority.
163-217: LGTM! Control functions demonstrate the canvas API correctly.The functions properly use
getData(),setData(),getZoom(),zoom(), andzoomToFit()methods from the canvas component API.app/graph/toolbar.tsx (3)
89-94: LGTM! Runtime type guards are correctly implemented.The search filtering now uses
"relationship" in eland"labels" in elchecks before casting, which is the proper pattern for discriminating union types at runtime.
107-112: LGTM! Zoom logic correctly handles nodes and links.The conditional check
"labels" in elementproperly discriminates between nodes and links, and the zoom filter correctly identifies relevant nodes for both cases.
223-258: LGTM! Suggestion rendering uses correct type discrimination.The
"source" in suggestioncheck properly discriminates links from nodes, and the subsequent casts toLinkorNodeare safe.app/graph/GraphView.tsx (4)
5-5: LGTM! Import cleanup aligns with state removal.Removing
useStateis correct since the component no longer manages local dimension state.
63-63: LGTM! Viewport context usage updated.Removing
isSavedfrom the destructured context aligns with the broader refactor to simplify viewport state management.
108-110: LGTM! Relationship visibility check updated correctly.Now that
link.sourceandlink.targetare numeric IDs, looking them up ingraph.NodesMapis the correct approach. The optional chaining safely handles missing nodes.
131-131: LGTM! Type guard updated to use runtime property check.Using
"labels" in einstead of!!e.labelsis the correct pattern for discriminating Node types at runtime.falkordb-canvas/falkordb-canvas.d.ts (1)
3-10: LGTM! Web component declaration structure is correct.The class declaration properly extends
HTMLElementand declares the public API methods with appropriate types, includinggetGraph()returningForceGraphInstance | undefinedto handle uninitialized state.falkordb-canvas/falkordb-canvas.ts (5)
104-113: LGTM! getData/setData types are consistent.The implementation uses
Datatype, and the declaration file (falkordb-canvas.d.tslines 5-6) correctly declaresgetData(): DataandsetData(data: Data), so the types are aligned.Note: A previous review flagged a type mismatch, but the current code is consistent.
228-383: LGTM! Graph initialization and force configuration look correct.The ForceGraph initialization properly configures all event handlers and rendering callbacks. The dynamic force calculations based on node degree are a nice touch for handling varying graph densities.
385-430: LGTM! Node drawing implementation is efficient.The caching of
displayNameon the node avoids redundant text wrapping calculations, and the two-line text rendering handles long labels well.
432-532: LGTM! Link drawing with efficient text caching.The relationship text metrics are cached to avoid redundant measurements, and the background rectangle ensures labels remain readable over edges. The bezier curve positioning math is correct.
561-581: LGTM! Engine stop handler logic is correct.The zoom-to-fit behavior with dynamic padding based on node count is appropriate, and the delayed loading state update gives the simulation time to settle.
Note: A previous review identified that the
setTimeoutID should be stored and cleared indisconnectedCallback()to prevent memory leaks.lib/utils.ts (2)
1-11: LGTM! Import changes align with canvas refactor.The imports now reference
ForceGraphInstanceandGraphNodefrom the canvas types module, correctly reflecting the shift to canvas-based rendering.
383-386: LGTM! Type guard updated to use runtime property check.Using
"labels" in einstead of!!e.labelsis the correct pattern for discriminating Node types at runtime.falkordb-canvas/falkordb-cnavas-utils.ts (3)
52-70: LGTM! GraphData to Data conversion is clean.The function correctly strips runtime properties and converts node references back to numeric IDs.
72-105: LGTM! Display text extraction logic is robust.The function properly handles the
ignoreflag for case-insensitive key matching and validates that the result is a non-empty string before using it.
110-166: LGTM! Text wrapping algorithm is well-implemented.The function correctly calculates available width using the Pythagorean theorem, handles word splitting for long words, and adds ellipsis when needed. The two-line wrapping is appropriate for circular node labels.
falkordb-canvas/falkordb-canvas-types.ts (4)
3-21: LGTM! ForceGraphConfig interface is well-structured.The configuration interface provides a clean API surface with optional properties for dimensions, colors, display priorities, event handlers, and loading/selection state.
23-53: LGTM! GraphNode and GraphLink types are comprehensive.The types properly extend
NodeObjectfrom force-graph and include all necessary runtime properties for simulation (positions, velocities, forces) plus display state.
55-73: LGTM! Data types cleanly separate runtime from serializable state.The
NodeandLinktypes appropriately strip runtime properties, making them suitable for API boundaries. The conversion betweenDataandGraphDatais handled by utility functions.
80-82: LGTM! ForceGraphInstance type correctly includes undefined.The type properly represents that a graph instance may not be initialized, and the generic parameters ensure type safety for node and link operations.
app/components/ForceGraph.tsx (2)
281-287: Verify<falkordb-canvas>fills the container; consider explicit sizing styles if the custom element defaults to inline.Depending on how the web component is implemented, you may need
display: block; width: 100%; height: 100%on the element (or have the component respect the passedwidth/heightvia config). If users report a “0px tall canvas”, this is the first place I’d look.Also applies to: 290-292
7-16: This review comment is incorrect. The typinguseRef<FalkorDBForceGraph>(null)on line 77 is correct and does not need modification.The
FalkorDBForceGraphclass extendsHTMLElementand is properly declared in the JSXIntrinsicElementsmapping for the<falkordb-canvas>custom element. Understrict: truein tsconfig.json,useRef<T>(null)correctly infers the ref type asRefObject<T>, which is the appropriate type for a custom element that will call methods like.getGraph(),.setConfig(), and.setData(). Adding| nullto the generic parameter would be semantically incorrect and is unnecessary.The side-effect import on line 15 (
import "@/falkordb-canvas/falkordb-canvas") is also correct for custom element registration and is not duplicated.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/schema/DataPanel.tsx (1)
210-222: Potential duplicate entries when adding attributes.The
handleAddAttributefunction spreads the current object into a new array entry ([...schema.Elements.nodes, { ...object as Node, ... }]) rather than updating the existing element. This appears to add a duplicate node/link with the new attribute instead of modifying the existing one in place. Compare withhandleSetAttribute(lines 112-126), which correctly uses.map()to update the existing element.Apply this diff to fix the logic:
if (ok) { if (type) { // eslint-disable-next-line no-param-reassign schema.Elements = { ...schema.Elements, - nodes: [...schema.Elements.nodes, { ...object as Node, data: { ...object.data, [newAttribute[0]]: newAttribute[1] } }] + nodes: schema.Elements.nodes.map(element => + element.id === object.id ? { ...element, data: { ...element.data, [newAttribute[0]]: newAttribute[1] } } : element + ) } } else { // eslint-disable-next-line no-param-reassign schema.Elements = { ...schema.Elements, - links: [...schema.Elements.links, { ...object as Link, data: { ...object.data, [newAttribute[0]]: newAttribute[1] } }] + links: schema.Elements.links.map(element => + element.id === object.id ? { ...element, data: { ...element.data, [newAttribute[0]]: newAttribute[1] } } : element + ) } } + setData({ ...schema.Elements }) setAttributes(prev => [...prev, newAttribute])
♻️ Duplicate comments (7)
lib/utils.ts (2)
21-21:GraphRefshould allowundefinedto prevent runtime errors before initialization.The type removes the
| undefinedunion, which can cause issues whenchartRef.currentis accessed before the canvas initializes.-export type GraphRef = MutableRefObject<ForceGraphInstance>; +export type GraphRef = MutableRefObject<ForceGraphInstance | undefined>;
230-232: Shadow DOM access is fragile and assumes a single canvas instance.Directly querying
document.querySelector("falkordb-canvas")and accessingshadowRootis tightly coupled to implementation details. Consider exposing a public API method on the canvas component for retrieving dimensions.app/components/ForceGraph.tsx (5)
32-49: Normalizesource/targetto numeric IDs to prevent type mismatch.
force-graphcan mutatesource/targetto node objects at runtime. The canvas expects numeric IDs, so normalization is needed:links: graphData.links.map(({ id, relationship, color, visible, source, target, data }) => ({ id, relationship, color, visible, - source, - target, + source: typeof source === "object" && source !== null ? (source as any).id : source, + target: typeof target === "object" && target !== null ? (target as any).id : target, data }))
88-96: Dependency array should be[]for one-time initialization.Including
chartRefanddatacauses this effect to re-run unnecessarily. Since refs are stable and this is initialization logic, use an empty array:- }, [chartRef, data]) + }, [])
98-112: Missingviewportin deps and uncleaned timeout can fire post-unmount.Add cleanup and include
viewportin dependencies:useEffect(() => { if (!isLoading && viewport && chartRef.current && !mount) { const { zoom, centerX, centerY } = viewport; - setTimeout(() => { + const t = window.setTimeout(() => { if (chartRef.current) { chartRef.current.zoom(zoom, 0); chartRef.current.centerAt(centerX, centerY, 0); setMount(true) } }, 100); + return () => window.clearTimeout(t); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [chartRef, graph.Id, currentTab, graph.Elements.nodes.length, isLoading]) + }, [graph.Id, currentTab, graph.Elements.nodes.length, isLoading, viewport, mount])
238-245: Selection uses reference equality; prefer ID matching.
selectedElements.includes(fullElement)fails after data refresh since objects are re-created. Use ID-based comparison:- if (selectedElements.includes(fullElement)) { - setSelectedElements(selectedElements.filter((el) => el !== fullElement)) + const sameKind = (a: Node | Link, b: Node | Link) => ("source" in a) === ("source" in b); + const isSelected = selectedElements.some(el => sameKind(el, fullElement) && el.id === fullElement.id); + if (isSelected) { + setSelectedElements(selectedElements.filter(el => !(sameKind(el, fullElement) && el.id === fullElement.id)))
273-278: Usingref.currentin dependency array is an anti-pattern.React doesn't track mutations to ref objects. Use
ResizeObserverinstead:+ useEffect(() => { + if (!canvasRef.current || !parentRef.current) return; + const parent = parentRef.current; + const canvas = canvasRef.current; + + const observer = new ResizeObserver(() => { + canvas.setWidth(parent.clientWidth); + canvas.setHeight(parent.clientHeight); + }); + observer.observe(parent); + + // Initial set + canvas.setWidth(parent.clientWidth); + canvas.setHeight(parent.clientHeight); + + return () => observer.disconnect(); + }, []); - useEffect(() => { - if (!canvasRef.current || !parentRef.current) return; - canvasRef.current.setWidth(parentRef.current.clientWidth); - canvasRef.current.setHeight(parentRef.current.clientHeight); - }, [parentRef.current?.clientWidth, parentRef.current?.clientHeight]);
🧹 Nitpick comments (12)
app/components/ForceGraph.tsx (1)
341-346: Consider removingdefaultProps- deprecated in React 18+.
defaultPropsis deprecated for function components. Default parameters in the signature (already present at line 63-64) are preferred.-ForceGraph.defaultProps = { - type: "graph", - currentTab: "Graph", - viewport: undefined, - setViewport: undefined, -}app/graph/page.tsx (1)
69-69: Consider narrowing chartRef type.The type
GraphRef["current"]results inForceGraphInstancewhich is not optional, but the ref is initialized asundefined. Consider usingGraphRef["current"] | undefinedorGraphRefdirectly for type safety.- const chartRef = useRef<GraphRef["current"]>() + const chartRef = useRef<GraphRef["current"] | undefined>(undefined)app/schema/_page.tsx (4)
4-5: ClarifychartReftyping versusGraphRef
GraphRefis already aMutableRefObject<ForceGraphInstance>, whilechartRefis inferred asMutableRefObject<GraphRef["current"] | undefined>. If consumers (e.g.Selector,SchemaView) expectGraphRef, this subtle mismatch can make TS unhappy.Consider tightening this to something like:
-const chartRef = useRef<GraphRef["current"]>() +const chartRef = useRef<GraphRef["current"] | null>(null)and updating the
chartRefprop types to acceptMutableRefObject<GraphRef["current"] | null>for consistency.Also applies to: 43-45, 287-288, 306-307
47-48: BroadenhandleCooldownargument type
const handleCooldown = (ticks?: 0)restricts callers to passing only0orundefined, even thoughcooldownTicksisnumber | undefined. IfSchemaViewever wants to pass a real tick count, this will be a type error.-const handleCooldown = (ticks?: 0) => { +const handleCooldown = (ticks?: number) => { setCooldownTicks(ticks) }This keeps types aligned with the state you store.
Also applies to: 128-130, 301-308
301-317:setIsLoadingprop cleanly exposes canvas loading controlWiring
isLoading={isCanvasLoading}andsetIsLoading={setIsCanvasLoading}intoSchemaViewkeeps loading concerns colocated with the page state while letting the canvas drive transitions.Looks good; just ensure
SchemaViewalways toggles loading in response to the actual request lifecycle to avoid stale spinners.
275-340: Suggested PR title (conventional commit)Given the changes here and in the graph model, a concise conventional-commit style title could be:
feat: migrate schema graph view to FalkorDB canvasapp/api/graph/model.ts (6)
53-77: NewNode/Linkshapes andMemoryValueunion look consistentMoving to plain
Node/Linkobjects with numericsource/targetaligns with howextendNode,extendEdge, and the rest ofGraphnow operate. The recursiveMemoryValue = number | Map<string, MemoryValue>also matches its use inGraphInfo.MemoryUsage.Only minor nit: if you ever want stronger typing for
data, consider makingNode/Linkgeneric instead of usingany, but that’s not required for this refactor.Also applies to: 110-111
451-455: Minor: simplifyGraph.createby passingidintoGraph.emptyYou currently create an empty graph with an empty id and then overwrite
graph.id:const graph = Graph.empty(undefined, currentLimit, graphInfo); graph.extend(...); graph.id = id;For clarity, you could pass
iddirectly:-const graph = Graph.empty( - undefined, - currentLimit, - graphInfo -); +const graph = Graph.empty( + id, + currentLimit, + graphInfo +);Behavior is equivalent but makes the constructor intent more obvious.
736-738: RebuildingnodesMap/linksMapinextendkeeps maps in syncRefreshing the maps from
this.elements.nodes/linksafter extending ensures lookups stay consistent even as elements are added.One follow-up you might consider is also pruning
nodesMap/linksMapin mutation helpers likeremoveElements, so they don’t hold references to removed nodes/links betweenextendcalls.
800-823: Optional: cache node visibility when toggling link visibility
visibleLinksrepeatedly doesthis.elements.nodes.find(...)per link, which is O(VE). If graphs can get large, you could precompute aMap<number, boolean>(orSetof visible IDs) outside the loop and reuse it:const visibleNodes = new Map(this.elements.nodes.map(n => [n.id, n.visible])); this.elements.links.forEach(link => { const srcVisible = visibleNodes.get(link.source); const tgtVisible = visibleNodes.get(link.target); // ... });Not required, but a simple optimization if this becomes hot.
825-866: ClarifyremoveLinksparameter semantics and avoid repeatedmapWith
Link.source/targetnow numeric IDs,idsappears to represent node IDs whose incident links should be removed. That matches the usage from the schema page, but it’s not obvious from the signature.Consider:
- Renaming
idstonodeIdsfor clarity.- Precomputing node IDs once instead of calling
this.elements.nodes.map(...)twice per link, e.g.:const nodeIds = new Set(this.elements.nodes.map(n => n.id)); ... (nodeIds.has(link.source) && nodeIds.has(link.target))This keeps intent clearer and avoids repeated allocations.
868-912:removeElementstype guards match newNode/LinkshapesUsing
"labels" in elementto detect nodes and"source" in elementto detect links is a clean way to discriminate the union now thatLink.sourceis an ID. The final filter that rebuildsthis.elementsfromnodes/linksis slightly redundant with the earlierspliceoperations, but functionally harmless.No functional issues spotted in this refactor.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
app/api/graph/model.ts(12 hunks)app/components/ForceGraph.tsx(7 hunks)app/components/provider.ts(6 hunks)app/graph/CreateElementPanel.tsx(1 hunks)app/graph/DataPanel.tsx(5 hunks)app/graph/DataTable.tsx(4 hunks)app/graph/GraphView.tsx(6 hunks)app/graph/page.tsx(5 hunks)app/providers.tsx(6 hunks)app/schema/DataPanel.tsx(9 hunks)app/schema/SchemaView.tsx(4 hunks)app/schema/_page.tsx(6 hunks)lib/utils.ts(4 hunks)package.json(1 hunks)types/falkordb-canvas.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- types/falkordb-canvas.d.ts
- package.json
- app/graph/CreateElementPanel.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/DataTable.tsxapp/schema/SchemaView.tsxapp/graph/DataPanel.tsxapp/components/ForceGraph.tsxapp/schema/DataPanel.tsxapp/providers.tsxapp/graph/page.tsxapp/graph/GraphView.tsxapp/schema/_page.tsx
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
lib/utils.tsapp/api/graph/model.tsapp/components/provider.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/graph/model.ts
🧬 Code graph analysis (7)
app/graph/DataPanel.tsx (1)
app/components/provider.ts (1)
ForceGraphContext(294-299)
lib/utils.ts (1)
app/api/graph/model.ts (1)
Node(53-63)
app/schema/DataPanel.tsx (1)
app/api/graph/model.ts (1)
GraphData(79-82)
app/providers.tsx (1)
app/components/provider.ts (1)
ForceGraphContext(294-299)
app/graph/page.tsx (2)
app/components/provider.ts (1)
ForceGraphContext(294-299)lib/utils.ts (1)
GraphRef(21-21)
app/graph/GraphView.tsx (1)
app/components/provider.ts (2)
GraphContext(228-249)ForceGraphContext(294-299)
app/schema/_page.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
🔇 Additional comments (31)
lib/utils.ts (1)
383-386: LGTM!The updated
isTwoNodestype guard using"labels" in eis a safer runtime check than the previous!!e.labelsapproach, correctly discriminating betweenNodeandLinktypes.app/graph/DataTable.tsx (3)
15-15: LGTM!Clean addition of
ForceGraphContextimport to accesssetDatafor propagating graph mutations.
229-229: LGTM!Proper data propagation after property set - spreading
graph.Elementsensures React detects the state change.
289-289: LGTM!Consistent data propagation pattern after property removal.
app/schema/SchemaView.tsx (2)
29-29: LGTM!Addition of
setIsLoadingprop cleanly integrates the loading state control with the canvas-based rendering flow.
120-134: LGTM!
ForceGraphreceives all required props including the newsetIsLoading, properly wiring the canvas loading state.app/graph/DataPanel.tsx (4)
14-14: LGTM!Clean import of
ForceGraphContextfor accessing the sharedsetDatafunction.
38-38: LGTM!Using
"source" in objectis a proper type discriminator betweenNodeandLinktypes, replacing the previous truthy check.
93-93: LGTM!Correct data propagation after adding a label to ensure UI reflects the change.
126-126: LGTM!Consistent data propagation after label removal.
app/schema/DataPanel.tsx (2)
52-52: Improved type guard using property existence check.The change from
!object.sourceto!("source" in object)is a safer approach for discriminating betweenNodeandLinktypes. The previous check could fail ifsourcewas falsy (e.g.,0or"").
127-127: Consistent data refresh pattern after mutations.The
setData({ ...schema.Elements })calls ensure the graph UI stays in sync after attribute and label mutations. This aligns with the broader PR pattern of propagating graph state viaForceGraphContext.Also applies to: 175-175, 287-287, 319-319
app/graph/page.tsx (3)
44-44: Context migration to ForceGraphContext.The switch from
ViewportContexttoForceGraphContextis consistent with the provider changes. ThesetDatais now correctly consumed from the new context.Also applies to: 61-61
243-245: Consistent type guard improvement.The change from
!element.sourceto!("source" in element)provides safer type narrowing, consistent with the same pattern applied inDataPanel.tsx.
226-229: Data update after element creation.The
setDatacall ensures the graph UI refreshes after creating new elements. The dependency array is correctly updated.app/components/provider.ts (3)
107-107: Added setIsLoading to GraphContextType.This addition enables child components to control the loading state, supporting the new data flow pattern where loading is triggered from various graph operations.
139-144: Context type renamed and simplified.The rename from
ViewportContextTypetoForceGraphContextTypebetter reflects the context's purpose. RemovingisSavedsimplifies the API surface.
294-299: ForceGraphContext export with proper defaults.The context is properly initialized with sensible defaults for viewport and data state.
app/providers.tsx (3)
206-211: Simplified viewportContext dependencies.The removal of
isSavedfrom the dependency array aligns with the type changes inForceGraphContextType.
386-387: setIsLoading added to graphContext.The
setIsLoadingis now exposed throughGraphContext, enabling child components to control loading state.
613-613: ForceGraphContext.Provider wiring.The provider tree correctly uses
ForceGraphContext.ProviderwithviewportContextvalue, replacing the previousViewportContext.Also applies to: 667-667
app/graph/GraphView.tsx (6)
5-5: Updated imports for context migration.The removal of
useStateand addition ofForceGraphContextimport reflects the component's shift to context-based state management.Also applies to: 9-9
60-61: Context consumption split between GraphContext and ForceGraphContext.The component correctly consumes
isLoadingandsetIsLoadingfromGraphContext, while graph data and viewport state come fromForceGraphContext. This separation aligns with the new context architecture.
106-108: Improved relationship visibility filtering.Using
graph.NodesMap.get(link.source)?.visibleis safer than direct property access, handling cases where the node might not exist in the map.
129-129: Safer type guard for add-edge predicate.Using
"labels" in eis more reliable than!!e.labelsfor distinguishing nodes from links, consistent with the type guard pattern used elsewhere in this PR.
208-223: ForceGraph props updated for new API.The component correctly passes
setIsLoadingand the updatedhandleCooldowntoForceGraph, aligning with the component's new prop interface.
29-29: handleCooldown signature is intentionally restrictive and correct.The signature
(ticks?: 0) => voidallows onlyundefinedor literal0, which matches all call sites in the codebase (handleCooldown(0)andhandleCooldown(cooldownTicks === undefined ? 0 : undefined)). This restrictive typing is intentional and enforces valid cooldown states correctly.app/schema/_page.tsx (2)
153-205: Node/link type guard in delete handler matches newLinkshapeUsing
const type = !("source" in element)to distinguish nodes from links is compatible with the newLinktype (which always hassource) and keeps the subsequent mutation logic correct for both branches.No change requested; just a note that this relies on
Nodenever gaining asourcefield in the future.
237-246:DataPanelreceivingsetDatakeeps schema edits in syncPassing
setDataintoDataPanelensures mutations performed in the side panel can immediately update the shareddataused bySchemaView/canvas, avoiding local copies or manual refreshes.This wiring looks correct and consistent with how
datais used elsewhere in the component.app/api/graph/model.ts (2)
461-492:calculateLinkCurvestill correctly groups multi-edges by endpointsUsing numeric
start = link.source/end = link.targetand comparing them directly keeps the curve calculation consistent with the newLinkshape, including self-loops (start === end) and multi-edge offsetting.No issues spotted here.
573-655:extendEdgeuses numeric node IDs consistentlyAssigning:
source: cell.sourceId, target: cell.destinationId,and creating placeholder nodes when
source/targetare missing matches the newLinkstructure and preserves label/relationship bookkeeping.Just double-check that all consumers have been updated away from
link.source.id/link.target.idto use the numeric IDs directly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
app/components/ForceGraph.tsx (5)
87-96: Canvas →chartRefinitialization effect doesn’t need reactive deps.This effect’s job is to wire
chartRef.currentfromcanvasRef.current.getGraph()once the canvas is available. IncludingchartRefand especiallydatain the dependency array causes unnecessary re-runs without any functional benefit (both are stable ref/data inputs for this wiring step).You can simplify to a one-time mount effect:
- useEffect(() => { - if (canvasRef.current) { - const forceGraph = canvasRef.current.getGraph(); - - if (forceGraph) { - chartRef.current = forceGraph; - } - } - }, [chartRef, data]) + useEffect(() => { + if (!canvasRef.current) return; + const graphInstance = canvasRef.current.getGraph(); + if (graphInstance) { + chartRef.current = graphInstance; + } + }, [])If there’s a known case where the internal graph becomes available only after some later event, we can instead key this off a dedicated “ready” signal from the canvas rather than
data.
32-49: Normalizelink.source/link.targetto numeric IDs before passing to canvas.Right now
convertToCanvasDataforwardssourceandtargetas-is. Withfalkordb-canvas, theLinkshape expects numeric IDs, and any upstream mutation that turnssource/targetinto node objects will break the canvas at runtime.You can defensively normalize to IDs when building
Data:const convertToCanvasData = (graphData: GraphData): Data => ({ @@ - links: graphData.links.map(({ id, relationship, color, visible, source, target, data }) => ({ - id, - relationship, - color, - visible, - source, - target, - data - })) + links: graphData.links.map(({ id, relationship, color, visible, source, target, data }) => ({ + id, + relationship, + color, + visible, + source: typeof source === "object" && source !== null ? (source as any).id : (source as any), + target: typeof target === "object" && target !== null ? (target as any).id : (target as any), + data + })) });This keeps the canvas
Datashape aligned with its type contract and avoids subtle failures if upstream code ever passes link objects instead of ids.
98-112: Viewport restore effect: missingviewportin deps andsetTimeoutcan fire after unmount.This effect reads
viewportand schedules asetTimeout, but:
viewportis not in the dependency array, so viewport changes while the component is mounted won’t be respected.- The timeout isn’t cleared, so it can fire after unmount and still touch
chartRef.current.You can make the effect reactive to viewport and add proper cleanup:
- // Load saved viewport on mount - useEffect(() => { - if (!isLoading && viewport && chartRef.current && !mount) { - const { zoom, centerX, centerY } = viewport; - - setTimeout(() => { - if (chartRef.current) { - chartRef.current.zoom(zoom, 0); - chartRef.current.centerAt(centerX, centerY, 0); - setMount(true) - } - }, 100); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [chartRef, graph.Id, currentTab, graph.Elements.nodes.length, isLoading]) + // Load saved viewport on mount / graph change + useEffect(() => { + if (!isLoading && viewport && chartRef.current && !mount) { + const { zoom, centerX, centerY } = viewport; + const timeoutId = window.setTimeout(() => { + if (chartRef.current) { + chartRef.current.zoom(zoom, 0); + chartRef.current.centerAt(centerX, centerY, 0); + setMount(true); + } + }, 100); + return () => window.clearTimeout(timeoutId); + } + }, [graph.Id, currentTab, graph.Elements.nodes.length, isLoading, viewport, mount])This keeps viewport restore in sync with its inputs and prevents post-unmount access to the chart.
226-247: Selection relies on reference equality; prefer id-based comparison for robustness.
selectedElements.includes(fullElement)and filtering viael !== fullElementassume the same object instances are always reused. After data refreshes or graph re-instantiation, logically identical nodes/links with new object identities won’t compare equal, leading to “can’t unselect” or duplicated selection entries.You can switch to id- and kind-based matching:
- if (!fullElement) return; - - if (evt.ctrlKey) { - if (selectedElements.includes(fullElement)) { - setSelectedElements(selectedElements.filter((el) => el !== fullElement)) - } else { - setSelectedElements([...selectedElements, fullElement]) - } - } else { - setSelectedElements([fullElement]) - } + if (!fullElement) return; + + const sameKind = (a: Node | Link, b: Node | Link) => ("source" in a) === ("source" in b); + const isSelected = selectedElements.some( + el => sameKind(el, fullElement) && el.id === fullElement.id + ); + + if (evt.ctrlKey) { + setSelectedElements( + isSelected + ? selectedElements.filter(el => !(sameKind(el, fullElement) && el.id === fullElement.id)) + : [...selectedElements, fullElement] + ); + } else { + setSelectedElements([fullElement]); + }This keeps selection stable across data updates while preserving the ctrl-click behavior.
273-279: Canvas dimensions are not reacting to container resize; preferResizeObserver.This effect sets width/height once based on
parentRef.current.clientWidth/Height. UsingparentRef.current?.clientWidthin the dependency array won’t detect DOM resizes, so the canvas won’t respond to window resizes or layout changes (sidebar toggle, panel resize, etc.).Adopting a
ResizeObserver(mirroring patterns elsewhere in the app) would make this robust:- // Update dimensions - useEffect(() => { - if (!canvasRef.current || !parentRef.current) return; - canvasRef.current.setWidth(parentRef.current.clientWidth); - canvasRef.current.setHeight(parentRef.current.clientHeight); - }, [parentRef.current?.clientWidth, parentRef.current?.clientHeight]); + // Update dimensions on container resize + useEffect(() => { + const canvas = canvasRef.current; + const parent = parentRef.current; + if (!canvas || !parent || typeof ResizeObserver === "undefined") return; + + const applySize = () => { + canvas.setWidth(parent.clientWidth); + canvas.setHeight(parent.clientHeight); + }; + + applySize(); + const observer = new ResizeObserver(() => applySize()); + observer.observe(parent); + + return () => observer.disconnect(); + }, [])That way, the canvas always matches its container without relying on unrelated re-renders.
🧹 Nitpick comments (1)
app/components/ForceGraph.tsx (1)
51-67: Overall: solid canvas migration; address a few integration edge cases + suggested PR title.The migration to
falkordb-canvasis generally well-structured: props surface is clear, hooks usage is mostly sound, and the new effects cleanly bridge React state/context into the canvas API. The remaining issues are mainly around data normalization (source/target), viewport restore, selection robustness, and resize handling.Suggested PR title (conventional commit):
refactor: migrate ForceGraph to FalkorDB canvas rendererAlso applies to: 334-346
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/ForceGraph.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/components/ForceGraph.tsx
🔇 Additional comments (1)
app/components/ForceGraph.tsx (1)
309-332: Event wiring and data flow tofalkordb-canvaslook clean and idiomatic.The separation of concerns here is good: config (callbacks/selection predicates/loading handler) is set via
setConfig, while color/text/loading/cooldown andDataupdates are handled in dedicated effects keyed off their respective inputs. That keeps the React side simple and makes the canvas the single source of truth for rendering.No changes needed in this block.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/schema/_page.tsx (1)
240-275: Unreachable code:isTwoNodesblock will never execute.The
isTwoNodes(selectedElements)check at line 262 is unreachable because the preceding condition at line 240 (selectedElements.length !== 0) already returns early for any non-empty array, including when there are exactly two nodes.If this branch is intended for the "Add Edge" flow when two nodes are selected, the logic needs restructuring:
const getCurrentPanel = () => { + if (isAddEdge && isTwoNodes(selectedElements)) { + return ( + <SchemaCreateElement + onCreate={handleCreateElement} + setIsAdd={handleSetIsAddEdge} + selectedNodes={selectedElements} + setSelectedNodes={setSelectedElements} + type={false} + /> + ) + } + if (selectedElements.length !== 0) { return ( <DataPanel ... /> ) } if (isAddNode) { return ( <SchemaCreateElement ... type /> ) } - if (isTwoNodes(selectedElements)) { - return ( - <SchemaCreateElement - ... - /> - ) - } - return null }
♻️ Duplicate comments (4)
app/components/ForceGraph.tsx (3)
34-51: Normalizesource/targetto numeric IDs inconvertToCanvasData.The canvas
Linktype expectssource: numberandtarget: number, but force-graph can mutate these to node objects at runtime. Pass them through without normalization risks type mismatch.links: graphData.links.map(({ id, relationship, color, visible, source, target, data }) => ({ id, relationship, color, visible, - source, - target, + source: typeof source === "object" && source !== null ? (source as { id: number }).id : source, + target: typeof target === "object" && target !== null ? (target as { id: number }).id : target, data }))
197-217: Selection uses reference equality; consider id-based matching.
selectedElements.includes(fullElement)will fail after data refresh when element objects are re-created. Use id + type matching instead.- if (selectedElements.includes(fullElement)) { - setSelectedElements(selectedElements.filter((el) => el !== fullElement)) + const isLink = 'source' in fullElement; + const isSelected = selectedElements.some(el => + el.id === fullElement.id && ('source' in el) === isLink + ); + if (isSelected) { + setSelectedElements(selectedElements.filter((el) => + !(el.id === fullElement.id && ('source' in el) === isLink) + ))
244-249: Avoid ref properties in useEffect dependency array.
parentRef.current?.clientWidthandparentRef.current?.clientHeightin deps won't trigger re-renders when the container resizes. Use aResizeObserverinstead.+ useEffect(() => { + if (!canvasRef.current || !parentRef.current) return; + const parent = parentRef.current; + const observer = new ResizeObserver(() => { + canvasRef.current?.setWidth(parent.clientWidth); + canvasRef.current?.setHeight(parent.clientHeight); + }); + observer.observe(parent); + canvasRef.current.setWidth(parent.clientWidth); + canvasRef.current.setHeight(parent.clientHeight); + return () => observer.disconnect(); + }, [canvasRef]); - useEffect(() => { - if (!canvasRef.current || !parentRef.current) return; - canvasRef.current.setWidth(parentRef.current.clientWidth); - canvasRef.current.setHeight(parentRef.current.clientHeight); - }, [parentRef.current?.clientWidth, parentRef.current?.clientHeight, canvasRef]);app/providers.tsx (1)
369-369: RemovehandleCooldownfromrunQuerydependency array.
handleCooldownis listed in deps but never called insiderunQuery. This causes unnecessary callback recreation.- }, [graphName, limit, timeout, fetchInfo, fetchCount, handleCooldown, handelGetNewQueries]); + }, [graphName, limit, timeout, fetchInfo, fetchCount, handelGetNewQueries]);
🧹 Nitpick comments (3)
app/schema/_page.tsx (1)
45-45: Consider using standard ref typing.Using
GraphRef["current"]as the ref type is unconventional. The standard pattern would be:- const canvasRef = useRef<GraphRef["current"]>(null) + const canvasRef = useRef<FalkorDBCanvas | null>(null)This is clearer and doesn't require the indexed access type.
app/components/provider.ts (1)
297-304: Minor inconsistency:graphDatadefault vs type.The type declares
graphData: CanvasData | undefined, but the default value is{ nodes: [], links: [] }. Consider aligning these:Either change the default to match the type:
- graphData: { nodes: [], links: [] }, + graphData: undefined,Or update the type if
undefinedis never expected:- graphData: CanvasData | undefined; + graphData: CanvasData;app/graph/toolbar.tsx (1)
225-225: RenametypetoisLinkfor clarity.The variable holds a boolean indicating whether the suggestion is a link, but
typereads like it should be a string or enum.- const type = "source" in suggestion + const isLink = "source" in suggestionThen update usages on lines 243 and 253 accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
app/components/ForceGraph.tsx(4 hunks)app/components/provider.ts(7 hunks)app/graph/DataTable.tsx(5 hunks)app/graph/GraphView.tsx(8 hunks)app/graph/Selector.tsx(5 hunks)app/graph/controls.tsx(2 hunks)app/graph/layout.tsx(1 hunks)app/graph/page.tsx(6 hunks)app/graph/toolbar.tsx(8 hunks)app/providers.tsx(9 hunks)app/schema/SchemaView.tsx(3 hunks)app/schema/_page.tsx(7 hunks)app/settings/browserSettings.tsx(1 hunks)lib/utils.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/graph/DataTable.tsx
- app/schema/SchemaView.tsx
- app/graph/page.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/settings/browserSettings.tsxapp/providers.tsxapp/graph/toolbar.tsxapp/graph/controls.tsxapp/graph/layout.tsxapp/graph/Selector.tsxapp/components/ForceGraph.tsxapp/graph/GraphView.tsxapp/schema/_page.tsx
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
lib/utils.tsapp/components/provider.ts
🧬 Code graph analysis (7)
lib/utils.ts (1)
app/api/graph/model.ts (1)
Node(53-63)
app/providers.tsx (1)
app/components/provider.ts (1)
ForceGraphContext(297-304)
app/graph/toolbar.tsx (2)
lib/utils.ts (1)
GraphRef(21-21)app/api/graph/model.ts (2)
Link(65-77)Node(53-63)
app/graph/controls.tsx (3)
lib/utils.ts (1)
GraphRef(21-21)app/components/ui/Button.tsx (1)
Props(11-21)app/components/provider.ts (1)
IndicatorContext(282-285)
app/graph/Selector.tsx (1)
lib/utils.ts (1)
GraphRef(21-21)
app/graph/GraphView.tsx (3)
app/components/ForceGraph.tsx (1)
ForceGraph(53-319)lib/utils.ts (1)
GraphRef(21-21)app/components/provider.ts (1)
GraphContext(231-252)
app/components/provider.ts (1)
app/api/graph/model.ts (1)
GraphData(79-82)
🔇 Additional comments (18)
app/settings/browserSettings.tsx (1)
11-11: LGTM!The
TextPrioritytype import is correctly migrated from the local utils to thefalkordb-canvaslibrary, aligning with the broader canvas integration.app/graph/layout.tsx (1)
1-9: Verify ifforce-dynamicis intentional for all graph routes.The layout correctly exports
dynamic = 'force-dynamic', which opts out of static generation for all routes under/graph. This is appropriate if all graph pages require real-time data, but be aware it applies to all child routes.If only specific pages need dynamic rendering, consider moving the export to individual
page.tsxfiles instead.lib/utils.ts (2)
21-21: LGTM!The
GraphReftype now usesFalkorDBCanvas | null, which is the standard React ref pattern. This addresses the previous review concern about missing the nullable union type.
340-342: Improved type guard usinginoperator.The change from
!!e.labelsto"labels" in eis a more robust type guard. Theinoperator properly narrows the type without risking false negatives from empty arrays.app/schema/_page.tsx (1)
160-160: LGTM!The type check using
!("source" in element)is consistent with the updatedisTwoNodesguard pattern and properly discriminates betweenNodeandLinktypes.app/components/provider.ts (2)
140-147: LGTM on the context type rename.The rename from
ViewportContextTypetoForceGraphContextTypeand the addition ofgraphData/setGraphDataproperly supports the canvas-based rendering architecture.
108-108: LGTM!The
setIsLoadingfunction is properly added toGraphContextTypewith appropriate typing.app/graph/controls.tsx (1)
32-38: LGTM!Clean migration to
canvasRefwith proper null safety via optional chaining.app/graph/toolbar.tsx (1)
107-112: LGTM!The
zoomToFitcall correctly uses"labels" in elementto distinguish Node from Link when filtering nodes for focus.app/providers.tsx (2)
256-260: LGTM!Simplified
handleCooldownby removing the unusedisSetLoadingparameter.
618-618: LGTM!Clean replacement of
ViewportContextwithForceGraphContextthroughout the provider tree.Also applies to: 672-672
app/graph/Selector.tsx (2)
619-620: LGTM!Using
"labels" in eis a correct type guard to verify both selected elements are nodes before enabling edge creation.
35-50: LGTM!Interface updates cleanly separate
SchemaProps(requirescanvasRef) fromGraphProps(excludes it) with proper discriminated union typing.Also applies to: 52-67
app/graph/GraphView.tsx (5)
21-24: Good use of dynamic import for client-side rendering.The dynamic import with
ssr: falsecorrectly ensures the canvas-based ForceGraph only loads in the browser, and the Spinning fallback provides good UX during load.
66-67: Context refactoring correctly separates concerns.The split between GraphContext (graph state, loading) and ForceGraphContext (simulation data, viewport) cleanly organizes the canvas-based rendering state.
112-112: Relationship filtering logic is correct.The visibility check correctly ensures relationships are only shown when both source and target nodes are visible, using optional chaining to safely handle missing nodes.
135-135: Property existence check correctly distinguishes nodes from links.Changing from
!!e.labelsto"labels" in eis semantically better: it checks for the property's presence rather than truthiness, correctly distinguishing nodes (which have alabelsproperty) from links (which don't).
214-230: ForceGraph integration correctly wired with new canvas-based data flow.The props for
graphData,setGraphData,canvasRef, andsetIsLoadingalign with the canvas-based rendering architecture and match the ForceGraph component signature.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
app/components/ForceGraph.tsx (6)
33-50: Still need to normalizelink.source/link.targetto numeric IDs.This was flagged in a previous review but remains unaddressed. The canvas expects
source: number; target: number, but runtime mutations can convert these to objects, causing type mismatches.
92-98: Remove debug console.log (previously flagged).Line 93 still contains the debug log that was flagged for removal in a previous review.
101-112: Remove debug console.log in cleanup (previously flagged).Line 106 still contains the debug log in the cleanup that was flagged for removal.
217-225: Selection reference equality issue remains (previously flagged).Lines 218-221 still use
includes(fullElement)which breaks when objects are recreated after data refresh.
254-259: ResizeObserver still missing (previously flagged).Dimensions effect won't trigger on actual resize because
parentRef.current?.clientWidthin dependencies doesn't cause re-renders when the DOM changes. AResizeObserveris needed as flagged in previous review.
308-322: MissinggraphDatain dependencies (previously flagged).Line 313 reads
graphDatabut it's not in the dependency array, causing potential stale closure issues.
🧹 Nitpick comments (1)
app/graph/page.tsx (1)
15-17: Consider adding a loading fallback for consistency.
SelectorandGraphViewboth have loading placeholders, butCreateElementPaneldoesn't. This could cause a minor layout shift when the panel loads.const CreateElementPanel = dynamicImport(() => import("./CreateElementPanel"), { ssr: false, + loading: () => <div className="h-full animate-pulse rounded-md border border-border bg-background" />, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/ForceGraph.tsx(4 hunks)app/graph/page.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/page.tsxapp/components/ForceGraph.tsx
🧬 Code graph analysis (1)
app/graph/page.tsx (2)
app/components/provider.ts (1)
ForceGraphContext(297-304)lib/utils.ts (1)
GraphRef(21-21)
🔇 Additional comments (18)
app/graph/page.tsx (7)
4-10: LGTM on import changes.The migration to
ForceGraphContextand addition ofGraphReftype aligns with the canvas-based rendering approach. ThedynamicImportrename avoids potential shadowing with reserved/common identifiers.
47-47: LGTM on context migration.Correctly extracts only
setDatafromForceGraphContext, which is all that's needed for the element creation and deletion flows.
72-72: LGTM on ref typing.Using
GraphRef["current"]maintains type consistency with the centralizedGraphRefdefinition, ensuring the ref type stays synchronized if the underlying canvas type changes.
229-232: LGTM on callback update.The
setDatadependency is correctly included, and spreadinggraph.Elementsensures React detects the state change.
248-248: Good improvement on type narrowing.Using
"source" in elementis the correct way to discriminate union types. The previous!element.sourcewould incorrectly identify a Link as a Node ifsourcehappened to be falsy.
374-392: LGTM on GraphView prop update.The
canvasRefprop correctly replaceschartRef, aligning with the canvas-based rendering migration. All other props remain appropriately threaded through.
285-299: LGTM on deletion handler updates.The
setDatadependency is correctly included, and the cleanup flow properly updates the graph state after element removal.app/components/ForceGraph.tsx (11)
84-89: LGTM: Dynamic import setup is correct.The effect correctly loads the web component once on mount with an appropriate empty dependency array.
114-134: LGTM: Callback dependencies are complete.All referenced values are properly included in the dependency array.
170-188: LGTM: Dependency array correctly updated.The unnecessary
displayTextPriorityhas been removed from dependencies as flagged in previous review.
190-204: LGTM: Hover handling is well-structured.Proper type discrimination and minimal dependencies.
228-231: LGTM: Callback dependencies are correct.
233-241: LGTM: Selection check logic is sound.Both callbacks correctly include their dependencies.
243-247: LGTM: Engine stop handler is correct.
249-251: LGTM: Stale closure issue fixed.The dependency array now correctly includes
setIsLoadingas flagged in previous review.
262-289: LGTM: Granular config effects are well-structured.Each effect correctly updates a single canvas property with appropriate dependencies.
291-305: LGTM: Event handler wiring is complete.All callback dependencies are properly tracked.
324-328: LGTM: Clean render structure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
app/components/ForceGraph.tsx (5)
33-50: Still missing source/target normalization to numeric IDs.Past review flagged this:
falkordb-canvasexpectssource: number; target: number, but force-graph can mutate these to node objects at runtime. Lines 46-47 still pass them through unchanged, creating a type mismatch.Apply this diff to normalize:
const convertToCanvasData = (graphData: GraphData): Data => ({ nodes: graphData.nodes.map(({ id, labels, color, visible, data }) => ({ id, labels, color, visible, data })), links: graphData.links.map(({ id, relationship, color, visible, source, target, data }) => ({ id, relationship, color, visible, - source, - target, + source: typeof source === "object" && source !== null ? (source as any).id : source, + target: typeof target === "object" && target !== null ? (target as any).id : target, data })) });
141-145: Unsafe link.source/target comparison - still not fixed.Past review flagged this:
link.targetandlink.sourcecan be node objects at runtime. Lines 142-143 assign them directly but line 144 compares as if they're always numeric IDs.Extract IDs safely:
const isTarget = graph.Elements.links.some(link => { - const targetId = link.target; - const sourceId = link.source; + const targetId = typeof link.target === 'object' ? link.target.id : link.target; + const sourceId = typeof link.source === 'object' ? link.source.id : link.source; return targetId === node.id && nodes.some(n => n.id === sourceId); });
202-222: Selection still uses reference equality - past issue unresolved.Lines 214-216 use
.includes(fullElement)which fails when element objects are re-created (common aftersetData({ ...graph.Elements })).Use ID-based matching:
if (evt.ctrlKey) { - if (selectedElements.includes(fullElement)) { - setSelectedElements(selectedElements.filter((el) => el !== fullElement)) + const isSelected = selectedElements.some(el => + el.id === fullElement.id && ('source' in el) === ('source' in fullElement) + ); + if (isSelected) { + setSelectedElements(selectedElements.filter(el => + !(el.id === fullElement.id && ('source' in el) === ('source' in fullElement)) + )) } else { setSelectedElements([...selectedElements, fullElement]) }
249-255: ResizeObserver still not implemented - past issue unresolved.Past review flagged that canvas dimensions are set once but won't update on container resize. Using
parentRef.current?.clientWidthin deps doesn't work—refs don't trigger re-renders. Implement ResizeObserver like other files in the codebase (CreateElementPanel.tsx,EditorComponent.tsx).// Update dimensions useEffect(() => { - if (!canvasRef.current || !parentRef.current || !canvasLoaded) return; + if (!parentRef.current || !canvasLoaded) return; - canvasRef.current.setWidth(parentRef.current.clientWidth); - canvasRef.current.setHeight(parentRef.current.clientHeight); - }, [parentRef.current?.clientWidth, parentRef.current?.clientHeight, canvasRef, canvasLoaded]); + const observer = new ResizeObserver((entries) => { + const entry = entries[0]; + if (entry && canvasRef.current) { + canvasRef.current.setWidth(entry.contentRect.width); + canvasRef.current.setHeight(entry.contentRect.height); + } + }); + + observer.observe(parentRef.current); + return () => observer.disconnect(); + }, [canvasRef, canvasLoaded]);
303-318: MissinggraphDatain dependency array - stale closure risk.Past review flagged this: line 309 reads
graphDatabut it's not in the deps (line 318). The eslint-disable on line 317 suggests awareness, but this creates a stale closure bug.- // eslint-disable-next-line react-hooks/exhaustive-deps - }, [canvasRef, data, setGraphData, canvasLoaded]) + }, [data, graphData, setGraphData, canvasLoaded])
🧹 Nitpick comments (3)
app/components/ForceGraph.tsx (3)
92-96: RemovecanvasReffrom dependency array - refs are stable.Refs don't change between renders, so including
canvasRefin the deps is unnecessary and can cause confusion.- }, [canvasRef, viewport, canvasLoaded]) + }, [viewport, canvasLoaded])
98-108: RemovecanvasReffrom dependency array.The ref object is stable and shouldn't trigger re-runs. The cleanup function captures the current canvas instance correctly.
- }, [canvasRef, graph.Id, setGraphData, setViewport, canvasLoaded]) + }, [graph.Id, setGraphData, setViewport, canvasLoaded])
257-301: RemovecanvasReffrom all effect dependencies.Lines 261, 266, 272, 278, 284, 301 include
canvasRefin dependency arrays, but refs are stable and shouldn't trigger re-runs. This is an optional cleanup.- }, [canvasRef, background, canvasLoaded]); + }, [background, canvasLoaded]); - }, [canvasRef, foreground, canvasLoaded]); + }, [foreground, canvasLoaded]); - }, [canvasRef, displayTextPriority, canvasLoaded]); + }, [displayTextPriority, canvasLoaded]); - }, [canvasRef, isLoading, canvasLoaded]); + }, [isLoading, canvasLoaded]); - }, [canvasRef, cooldownTicks, canvasLoaded]); + }, [cooldownTicks, canvasLoaded]); - }, [handleNodeClick, handleRightClick, handleHover, handleUnselected, checkIsNodeSelected, checkIsLinkSelected, handleEngineStop, handleLoadingChange, canvasRef, canvasLoaded]) + }, [handleNodeClick, handleRightClick, handleHover, handleUnselected, checkIsNodeSelected, checkIsLinkSelected, handleEngineStop, handleLoadingChange, canvasLoaded])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/ForceGraph.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/components/ForceGraph.tsx
🧬 Code graph analysis (1)
app/components/ForceGraph.tsx (2)
app/api/graph/model.ts (7)
Graph(294-1089)GraphData(79-82)Node(53-63)Link(65-77)Data(108-108)Data(400-402)Data(404-406)lib/utils.ts (2)
GraphRef(21-21)getTheme(323-337)
🔇 Additional comments (2)
app/components/ForceGraph.tsx (2)
84-89: LGTM - dynamic import pattern is correct.The falkordb-canvas web component is loaded client-side only, and
canvasLoadedstate properly gates subsequent effects.
229-237: LGTM - ID-based selection checks are correct.Unlike the right-click handler, these callbacks properly use
el.id === node.idcomparisons and discriminate node vs link with'source' in el.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/providers.tsx:
- Around line 447-482: During plain-text secret key migration (when
storedSecretKey is non-encrypted), ensure failures don't leave the plain text in
localStorage: after calling encryptValue(storedSecretKey) inside the try/catch,
if encryption fails or an exception is thrown, remove the plain text via
localStorage.removeItem("secretKey"), clear the in-memory key with
setSecretKey(''), and surface an error to the user (use toast.error(...) if
toast is available, otherwise stash the error to display after mount). Update
the catch block and the branch handling a falsy encryptedKey to perform these
removals and notification instead of only logging.
🧹 Nitpick comments (6)
e2e/tests/customizeStyle.spec.ts (2)
469-469: Consider adding proper types for canvas node objects.The use of
anywitheslint-disablebypasses type safety. Define an interface or type for canvas node objects to improve maintainability and catch potential issues at compile time.Example type definition
interface CanvasNode { labels?: string[]; color: string; // Add other properties as needed } // Usage const targetNode = nodes.find((n: CanvasNode) => n.labels?.includes("person1"));Also applies to: 496-496
455-455: Note: Extended timeout may indicate performance concerns.The 40-second timeout is significantly longer than typical e2e tests. While the comment explains this is for canvas operations in Firefox CI, consider investigating whether the canvas rendering performance can be optimized or if more efficient waiting strategies could reduce this timeout.
lib/encryption.ts (2)
102-135: Consider explicit error propagation instead of silent empty string return.Returning an empty string on encryption failure (line 133) makes it difficult for callers to distinguish between legitimate empty input and encryption errors. While the error is logged, consider either throwing the error or returning a Result type to enable proper error handling upstream.
♻️ Alternative error handling pattern
-export async function encryptValue(value: string): Promise<string> { +export async function encryptValue(value: string): Promise<string | null> { if (!value) return ''; try { // ... encryption logic ... return ENCRYPTED_PREFIX + arrayBufferToBase64(combined.buffer); } catch (error) { console.error('Error encrypting value:', error); - // Return empty string on error to avoid storing unencrypted data - return ''; + // Return null to signal encryption failure + return null; } }Callers can then check for
nullto detect encryption failures explicitly.
171-176: Comment mentions clearing corrupted value but doesn't implement it.The comment on line 173 states "Clear the corrupted encrypted value from localStorage if possible" but the implementation only returns an empty string. If clearing is intended, implement it; otherwise, remove or clarify the comment.
♻️ Implement cleanup if intended
If you want to clear corrupted values:
} catch (error) { console.warn('Failed to decrypt value - possibly due to key mismatch or corrupted data. Clearing stored value.', error); - // Clear the corrupted encrypted value from localStorage if possible - // Return empty string on error + // Note: Cannot clear from here as we don't have the key name + // Caller should handle cleanup if needed return ''; }Alternatively, accept a storage key parameter to enable cleanup within this function.
app/components/ForceGraph.tsx (1)
240-251: Global debug helper exposes internal graph data.Line 246 assigns
window[type]as a debug function to access graph data. While useful for development, consider:
- Documenting this debug interface
- Gating it behind a development flag
- Adding a comment explaining its purpose
Example:
const handleEngineStop = useCallback(() => { const canvas = canvasRef.current; if (!canvas) return; - // eslint-disable-next-line @typescript-eslint/no-explicit-any + // Debug helper: allows inspecting graph data in browser console via window.graph() or window.schema() + // eslint-disable-next-line @typescript-eslint/no-explicit-any (window as any)[type] = () => canvas.getGraphData();app/providers.tsx (1)
139-198: Early returns in saveSettings prevent saving other settings on encryption failure.Lines 159 and 172 return early when encryption is unavailable or fails, preventing the update of other settings (timeout, limit, etc.). This could confuse users who expect non-secret settings to save successfully.
Consider one of these approaches:
Option 1: Save other settings even if encryption fails
// Only encrypt and save secret key if it has changed + let secretKeyError = false; if (newSecretKey !== secretKey) { if (newSecretKey) { // Key has a value - encrypt it if (!isCryptoAvailable()) { toast({ title: "Error", description: "Encryption not available in this browser. Cannot save secret key.", variant: "destructive", }); - return; // Stop saving if crypto not available + secretKeyError = true; } - try { + if (!secretKeyError) { + try { const encryptedKey = await encryptValue(newSecretKey); localStorage.setItem("secretKey", encryptedKey); - } catch (error) { + } catch (error) { console.error('Failed to encrypt secret key:', error); toast({ title: "Error", description: "Could not encrypt secret key. Please try again.", variant: "destructive", }); - return; // Stop saving if encryption fails + secretKeyError = true; + } } } else { // Key is empty - remove it from storage localStorage.removeItem("secretKey"); } } // Update context // ... (save other settings) - setSecretKey(newSecretKey); + if (!secretKeyError) { + setSecretKey(newSecretKey); + }Option 2: Show different toast for partial save
if (secretKeyError) { toast({ title: "Settings partially saved", description: "Other settings saved, but secret key could not be encrypted.", variant: "warning", }); } else { toast({ title: "Settings saved", description: "Your settings have been saved.", }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/ForceGraph.tsxapp/providers.tsxe2e/tests/customizeStyle.spec.tslib/encryption.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
e2e/tests/customizeStyle.spec.tslib/encryption.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/tests/customizeStyle.spec.ts
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/components/ForceGraph.tsxapp/providers.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-13T09:41:15.717Z
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: .github/workflows/playwright.yml:264-264
Timestamp: 2025-11-13T09:41:15.717Z
Learning: The hardcoded ENCRYPTION_KEY value "a3f5d8c2b7e4f1a0c9d6e8b2f5a7c3d1e9f2a4b8c6d3e7f1a5b9c2d8e4f0a6b3" in .github/workflows/playwright.yml at the run-token-tests job is a test-only encryption key used for CI token tests and is not a production secret.
Applied to files:
lib/encryption.tsapp/providers.tsx
🧬 Code graph analysis (2)
e2e/tests/customizeStyle.spec.ts (1)
e2e/infra/utils.ts (1)
CREATE_PERSON_RELATIONSHIP(16-17)
app/components/ForceGraph.tsx (3)
app/api/graph/model.ts (8)
Graph(339-1180)GraphData(84-87)Node(56-68)Link(70-82)Relationship(183-189)Data(113-113)Data(445-447)Data(449-451)lib/utils.ts (2)
GraphRef(27-27)getTheme(337-351)app/components/provider.ts (1)
IndicatorContext(275-278)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: security-scan
🔇 Additional comments (10)
e2e/tests/customizeStyle.spec.ts (5)
8-8: LGTM: Import addition aligns with test requirements.The new import supports the expanded test coverage for relationship-specific scenarios.
180-180: LGTM: Property name updates align with canvas migration.The changes from
customCaption/customSize/customColortocaption/size/colorare consistent with the @falkordb/canvas API refactoring described in the PR objectives.Also applies to: 280-280, 320-320, 385-386, 447-447
322-329: LGTM: Enhanced color verification improves test coverage.The addition of label button color verification complements localStorage checks, ensuring color persistence is validated both in storage and in the rendered UI. RGB conversion is correct.
159-180: No action required. The test correctly usesCREATE_PERSON_RELATIONSHIPbecause the test validates caption selection with the "occupation" property, which only exists in this richer query definition.CREATE_QUERYlacks this property, so it's intentionally not used here. The selective use across tests is appropriate and justified.
361-361: Verify "ID" is explicitly available as a selectable caption property.The test selects "ID" as a caption, but this property is not explicitly defined in the test data (CREATE_PERSON_RELATIONSHIP defines: name, age, gender, occupation). According to FalkorDB documentation, node identifiers must be explicit properties to be selectable as captions—internal node IDs alone are not sufficient. Confirm that "ID" is defined in the test data setup or available through another mechanism before this test runs.
lib/encryption.ts (1)
1-194: LGTM - Solid encryption implementation with Web Crypto API.The encryption utilities follow security best practices:
- AES-GCM with 256-bit keys
- Proper random IV generation (12 bytes)
- Key persistence via localStorage
- Crypto availability detection
Error handling returns empty strings which is acceptable for preventing unencrypted data storage, though explicit error propagation could improve caller experience.
app/components/ForceGraph.tsx (3)
298-312: Verify data synchronization pattern with disabled deps warning.The effect on line 298-312 disables the
exhaustive-depswarning. The pattern appears intentional:
- External code sets
graphDatato trigger a full data load- Effect detects
graphData, applies it, then clears it (line 305)- Otherwise, it converts and applies
dataHowever, excluding
graphDatafrom dependencies means changes tographDatawhile the effect is inactive won't trigger updates. Verify this matches the intended data flow.If this pattern is correct, add a comment explaining why
graphDatais intentionally excluded:// Update canvas data useEffect(() => { const canvas = canvasRef.current; if (!canvas || !canvasLoaded) return; + // Note: graphData is intentionally excluded from deps to prevent loops. + // External code sets graphData to trigger reload, effect clears it after use. if (graphData) { canvas.setGraphData(graphData); setGraphData(undefined); } else { const canvasData = convertToCanvasData(data); canvas.setData(canvasData); } // eslint-disable-next-line react-hooks/exhaustive-deps }, [canvasRef, data, setGraphData, canvasLoaded]);
314-317: Ensure TypeScript declaration for custom web component.The
<falkordb-canvas />element is a custom web component. Verify that TypeScript has proper declarations to avoid type errors. If not already present, add to a.d.tsfile:declare namespace JSX { interface IntrinsicElements { 'falkordb-canvas': React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement>; } }Check if
@falkordb/canvaspackage exports these declarations, or if they need to be added locally.
1-317: LGTM - Well-structured canvas migration with proper async handling.The refactor successfully replaces ForceGraph2D with @falkordb/canvas:
- Clean data transformation via
convertToCanvasData- Proper async loading with
canvasLoadedguard- Event handlers use stable callbacks
- State synchronization via focused useEffect hooks
The dynamic import pattern and viewport persistence are correctly implemented.
app/providers.tsx (1)
1-752: LGTM - Solid encryption integration with backward-compatible migration.The provider updates successfully integrate encryption utilities:
- Secret keys are encrypted using AES-GCM before localStorage persistence
- Existing plain text keys are migrated to encrypted format
- Crypto availability is checked before operations
- ForceGraphContext properly wires canvas state
The initialization flow handles three cases correctly: no crypto available, already encrypted, and plain text migration. Error handling is present throughout with appropriate user feedback via toasts.
…dling in graph operations
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/graph.spec.ts (1)
147-161: Removetest.only()before merging — this will skip all other tests.
test.only()restricts Playwright to run only these two tests, causing the entire remaining test suite to be skipped in CI. This appears to be leftover debugging instrumentation.🐛 Proposed fix
- test.only(`@readwrite validate that deleting graph relation doesn't decreases node count`, async () => { + test(`@readwrite validate that deleting graph relation doesn't decreases node count`, async () => {- test.only(`@readwrite validate that deleting graph connected node decreases relation count by one`, async () => { + test(`@readwrite validate that deleting graph connected node decreases relation count by one`, async () => {
🤖 Fix all issues with AI agents
In @app/components/ForceGraph.tsx:
- Around line 306-311: Remove the debug console.log call in the else block of
ForceGraph.tsx: after calling convertToCanvasData(data) and assigning to
canvasData, delete the console.log(canvasData) statement so only
canvas.setData(canvasData) runs (or replace with a conditional debug logger if
you prefer environment-gated logging); the relevant symbols to locate are
convertToCanvasData, canvasData, and canvas.setData.
- Around line 214-219: The selection check has an operator precedence bug: e.id
=== fullElement.id only applies to the second branch due to && binding tighter
than ||; update the conditional inside the ctrlKey branch (the logic that
inspects selectedElements, e, and fullElement) so the id equality is checked for
both cases by grouping the type-matching OR into parentheses and then ANDing
with e.id === fullElement.id (i.e. (( "source" in e && "source" in fullElement)
|| (!("source" in e) && !("source" in fullElement))) && e.id ===
fullElement.id), then keep the existing setSelectedElements toggling behavior.
In @app/providers.tsx:
- Line 211: The useMemo dependency array contains a duplicated entry
`navigateToSettings`; remove the redundant `navigateToSettings` from the
dependency array in the useMemo call (the array that currently lists
displayChat, navigateToSettings, contentPersistence, ..., showMemoryUsage) so
each dependency appears only once, leaving a unique list of dependencies for the
memoization.
In @package.json:
- Line 16: The package.json dependency for "@falkordb/canvas" points to a
non-existent published version "^0.0.24"; change the dependency entry for
"@falkordb/canvas" to use the published version "^0.0.23" (or an exact "0.0.23"
if you want to avoid caret ranges) so installs succeed and consider locking to
0.0.23 due to pre-1.0 instability.
🧹 Nitpick comments (4)
e2e/tests/queryHistory.spec.ts (1)
58-63: Consider using thequeryvariable in the assertion.Line 63 duplicates the query string literal. Using the variable improves maintainability:
- expect(await graph.getQueryHistoryEditorContent(query)).toBe("CREATE (n:Person { name: 'Alice' }) RETURN n"); + expect(await graph.getQueryHistoryEditorContent(query)).toBe(query);app/components/ForceGraph.tsx (1)
244-247: Debug helper exposed onwindowobject.This exposes graph data retrieval via
window[type](). Consider guarding this behind a development environment check or removing it before production.♻️ Optional: Guard with environment check
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - (window as any)[type] = () => canvas.getGraphData(); + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any)[type] = () => canvas.getGraphData(); + }app/api/graph/model.ts (2)
1038-1045: Minor: Trailing whitespace and empty lines.Lines 1038 and 1044-1045 contain trailing whitespace or redundant empty lines.
♻️ Cleanup
category.elements = category.elements.filter( (element) => element.id !== selectedElement.id ); - if (category.elements.length === 0) { this.Labels.splice( this.Labels.findIndex((c) => c.name === category.name), 1 ); - this.LabelsMap.delete(category.name); }
1112-1121: Minor: Redundant empty lines.Multiple consecutive empty lines reduce code density unnecessarily.
♻️ Cleanup
selectedElement.labels.splice(emptyCategoryIndex, 1); - - - const emptyCategory = this.labelsMap.get(""); - if (emptyCategory) { emptyCategory.elements = emptyCategory.elements.filter( (e) => e.id !== selectedElement.id ); - if (emptyCategory.elements.length === 0) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
app/api/graph/model.tsapp/components/ForceGraph.tsxapp/providers.tsxe2e/tests/canvas.spec.tse2e/tests/graph.spec.tse2e/tests/queryHistory.spec.tspackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests/canvas.spec.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/providers.tsxapp/components/ForceGraph.tsx
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
e2e/tests/queryHistory.spec.tse2e/tests/graph.spec.tsapp/api/graph/model.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/tests/queryHistory.spec.tse2e/tests/graph.spec.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/graph/model.ts
🧠 Learnings (1)
📚 Learning: 2025-11-13T09:41:15.717Z
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: .github/workflows/playwright.yml:264-264
Timestamp: 2025-11-13T09:41:15.717Z
Learning: The hardcoded ENCRYPTION_KEY value "a3f5d8c2b7e4f1a0c9d6e8b2f5a7c3d1e9f2a4b8c6d3e7f1a5b9c2d8e4f0a6b3" in .github/workflows/playwright.yml at the run-token-tests job is a test-only encryption key used for CI token tests and is not a production secret.
Applied to files:
app/providers.tsx
🧬 Code graph analysis (1)
e2e/tests/queryHistory.spec.ts (2)
e2e/infra/utils.ts (1)
getRandomString(198-202)e2e/logic/POM/queryHistoryComponent.ts (1)
QueryHistory(5-83)
🔇 Additional comments (15)
e2e/tests/graph.spec.ts (1)
84-84: Minor formatting normalization — LGTM.Semicolon addition aligns with the codebase style.
e2e/tests/queryHistory.spec.ts (3)
5-5: Formatting consistency improvements look good.Semicolon additions align with the codebase style.
Also applies to: 16-16, 20-20, 67-67
28-33: Good assertion addition.Adding
getQueryHistory(query)assertion properly validates the core functionality being tested.
46-49: Verify tooltip now displays node ID instead of node data.The assertion expects
"0"(likely the internal node ID) rather than node-related content like"Alice". This appears to be a behavioral change from the@falkordb/canvasmigration.While this should be stable for a fresh graph, hardcoding
"0"is fragile if:
- Node ID assignment changes
- The graph has pre-existing data
Consider adding a comment explaining this is the expected node ID, or verify this is the intended tooltip behavior with the new canvas library.
app/components/ForceGraph.tsx (2)
82-87: LGTM!Dynamic import pattern for the web component is appropriate for client-side only loading.
316-318: LGTM!Clean JSX rendering of the
falkordb-canvasweb component with proper ref binding.app/providers.tsx (4)
148-178: LGTM!Solid encryption handling with proper availability checks, error handling, and user feedback via toast notifications.
447-478: LGTM!Good migration path for existing plain text keys to encrypted format, with proper fallback handling for corrupted or unavailable encryption.
21-23: LGTM!Dynamic import with SSR disabled is appropriate for the GraphInfoPanel component that depends on browser APIs.
233-241: Dependency array incomplete inforceGraphContextmemo.The memoized object includes
setViewportandsetGraphData, but the dependency array is missing these state setters. Update the dependency array to[canvasRef, viewport, setViewport, data, setData, graphData, setGraphData]to prevent stale closures.Likely an incorrect or invalid review comment.
app/api/graph/model.ts (4)
56-82: LGTM!Clean migration of
NodeandLinktypes from wrapper-based to plain objects with numeric IDs. The simplified structure aligns well with the canvas-based rendering requirements.
155-162: LGTM!The
LabelStyleinterface properly extendsLinkStylewith optionalsizeandcaptionproperties, maintaining type safety for the style-driven approach.
781-782: LGTM!Rebuilding the maps after
extendensures data consistency between the arrays and lookup maps.
866-888: LGTM!The
visibleLinksmethod correctly usesnodesMaplookups with optional chaining for safe access when checking node visibility.package.json (1)
15-16: Remove the suspicious"-"dependency.The
"-"package at line 15 appears to be a typo or placeholder. A package name consisting of a single hyphen is not a standard npm package name and is highly suspicious. It should be removed.🔧 Suggested fix
"dependencies": { - "-": "^0.0.1", "@falkordb/canvas": "^0.0.24",
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/graph/Chat.tsx (2)
137-216: Wrap switch case declarations in blocks.Variables declared in switch cases without blocks can be accessed from other cases, leading to potential bugs. Biome correctly flags
message(line 139) andstatusCode(line 197).♻️ Proposed fix
switch (eventType) { - case "Status": - const message = { + case "Status": { + const message = { role: "assistant" as const, content: eventData.trim(), type: eventType }; setMessages(prev => [...prev, message]); break; + } case "CypherQuery": // ... unchanged ... - case "error": - const statusCode = Number(line.split("status:")[1].split(" ")[0]); + case "error": { + const statusCode = Number(line.split("status:")[1].split(" ")[0]); if (statusCode === 401 || statusCode >= 500) setIndicator("offline"); toast({ title: "Error", description: eventData, variant: "destructive", }); isResult = true; break; + }
236-317: Wrap switch case declarations in blocks ingetMessage.Same issue here:
content(line 239) andi(line 257) need block scoping.♻️ Proposed fix
const getMessage = (message: Message, index?: number) => { switch (message.type) { - case "Status": - const content = <> + case "Status": { + const content = <> { messages[messages.length - 1].type === "Status" && messages[messages.length - 1] === message && <Loader2 className="animate-spin" size={15} /> } <p className="text-sm">{message.content}</p> </>; return index !== undefined ? ( <li className="flex gap-2 items-center" key={index}> {content} </li> ) : ( <div className="flex gap-2 items-center"> {content} </div> ); - case "CypherQuery": - const i = messages.findIndex(m => m === message); + } + case "CypherQuery": { + const i = messages.findIndex(m => m === message); return ( // ... JSX unchanged ... ); + } default: return ( <p className="text-wrap whitespace-pre-wrap">{message.content}</p> ); } };
🤖 Fix all issues with AI agents
In @app/api/chat/route.tsx:
- Line 62: The SSE messages are malformed because multiple fields are
concatenated on one line; update each writer.write(encoder.encode(...)) call
(e.g., the error write at writer.write(encoder.encode(`event: error status:
${400} data: ${JSON.stringify(validation.error)}\n\n`)) and the similar writes
around the code paths you were pointed to) to emit proper SSE lines: write an
"event: <name>\n" line, then any other fields each on their own line (e.g.,
"status: <code>\n" if you need it), then "data: <json-string>\n\n" to terminate
the event; apply this change to the occurrences mentioned (lines where
writer.write/encoder.encode are used at the error/data sends: 62, 78, 113, 117,
121, 125, 141, 146) so the client can parse each SSE event correctly.
- Around line 150-152: The abort listener registered via
request.signal.addEventListener currently calls writer.close() unconditionally
and can throw if the writer was already closed; fix this by tracking closure
state or guarding the call: add a boolean (e.g., writerClosed) set to true
wherever writer.close() is called/when stream finishes, and in the abort handler
check if (!writerClosed) { writerClosed = true; try { writer.close(); } catch
(e) { /* ignore TypeError if already closed */ } }, or alternatively wrap
writer.close() in a try/catch and ignore the specific TypeError; also remove or
noop the abort listener when normal completion occurs to avoid racing.
In @app/components/ForceGraph.tsx:
- Around line 297-313: The effect in useEffect uses graphData (and calls
canvas.setGraphData and setGraphData) but omits graphData from the dependency
array, so add graphData to the dependency list for this effect to ensure it
re-runs when graphData changes; update the dependencies to include graphData
(i.e., include canvasRef, data, graphData, setGraphData, canvasLoaded) and
remove the eslint-disable comment so React can validate dependencies for
convertToCanvasData and the canvas update logic.
In @app/graph/Chat.tsx:
- Around line 344-351: The click handler is querying
`.status-group[key="${index}"]` but React's `key` isn't rendered to the DOM; add
a data attribute to the corresponding <li> (e.g., data-index or data-user-index)
in the element that renders the status group (the same element currently using
key) and update the selector in the onClick handler to use that data attribute
(e.g., `.status-group[data-user-index="${index}"]`); keep the existing
setMessagesList logic and only change the DOM attribute and query string.
In @app/providers.tsx:
- Line 58: canvasRef is typed incorrectly as GraphRef["current"] though useRef
should be given the ref object type; change the generic to GraphRef (or
React.RefObject<GraphRef>) when calling useRef so canvasRef has the correct
RefObject type and can be passed to children expecting GraphRef; update the
declaration that creates canvasRef (the useRef call) to use GraphRef (or import
and use React.RefObject<GraphRef>) instead of GraphRef["current"].
- Around line 447-478: The decryptValue(storedSecretKey) call can throw but is
not wrapped in try/catch; wrap the decryption branch (the
isEncrypted(storedSecretKey) path) in a try/catch, catch any error from
decryptValue, log the error via console.warn/console.error, clear state with
setSecretKey('') and remove the stored secret with
localStorage.removeItem("secretKey") (matching the existing falsy-decryption
behavior), and keep the existing null/undefined check for decryptedKey as a
fallback.
🧹 Nitpick comments (9)
app/api/chat/route.tsx (3)
23-23: Event name casing inconsistency.
EventTypedefines"Error"but the SSE events emitted use lowercase"error"(lines 62, 78, 141, 146). Consider aligning them for consistency and type safety.
128-128: Consider awaitingwriter.close()before returning.
writer.close()returns a Promise. Not awaiting it means the Response may be returned while the close operation is still pending, which could lead to subtle timing issues.♻️ Proposed fix
- writer.close(); + await writer.close();Apply to all
writer.close()calls (lines 63, 79, 128, 142, 147).Also applies to: 142-142, 147-147
129-143: Consider differentiating error status codes.The inner catch block always emits status 400, but errors like network failures or internal exceptions should return 500. API key errors are appropriately 400, but other errors conflate client and server issues.
app/graph/Chat.tsx (1)
1-2: Consider removing theno-case-declarationseslint-disable.The static analysis correctly flags switch declarations that should be block-scoped (lines 139, 197, 239, 257). Rather than disabling the lint rule, wrap those declarations in blocks. This prevents accidental access across case clauses.
app/providers.tsx (2)
282-286: Unusual type annotation forticksparameter.The type
ticks?: 0restricts the parameter to only acceptundefinedor the literal0. If this is intentional (only allowing reset to 0), consider adding a comment. Otherwise, usenumber | undefined.Proposed fix if broader number type intended
- const handleCooldown = useCallback((ticks?: 0) => { + const handleCooldown = useCallback((ticks?: number) => {
139-178: Encryption error handling prevents saving other settings.When encryption fails (lines 159, 172), the function returns early, preventing other settings from being saved. Consider saving non-sensitive settings first, then attempting encryption.
Proposed restructure
saveSettings: async () => { // Save settings to local storage localStorage.setItem("runDefaultQuery", newRunDefaultQuery.toString()); localStorage.setItem("contentPersistence", newContentPersistence.toString()); localStorage.setItem("timeout", newTimeout.toString()); localStorage.setItem("defaultQuery", newDefaultQuery); localStorage.setItem("limit", newLimit.toString()); localStorage.setItem("refreshInterval", newRefreshInterval.toString()); localStorage.setItem("model", newModel); + // Update non-sensitive context state first + setContentPersistence(newContentPersistence); + setRunDefaultQuery(newRunDefaultQuery); + setDefaultQuery(newDefaultQuery); + setTimeout(newTimeout); + setLimit(newLimit); + setLastLimit(limit); + setModel(newModel); + setRefreshInterval(newRefreshInterval); // Only encrypt and save secret key if it has changed if (newSecretKey !== secretKey) { if (newSecretKey) { // Key has a value - encrypt it if (!isCryptoAvailable()) { toast({ title: "Error", description: "Encryption not available in this browser. Cannot save secret key.", variant: "destructive", }); - return; // Stop saving if crypto not available + setHasChanges(false); + return; } // ... rest of encryption logicapp/components/ForceGraph.tsx (3)
167-185: Double-click detection uses 1000ms threshold.The 1000ms window for detecting double-clicks (line 175) is unusually long. Standard double-click intervals are typically 200-500ms. This may lead to unintended node expansions.
Proposed fix
- if (now.getTime() - date.getTime() < 1000 && name === node.id) { + if (now.getTime() - date.getTime() < 400 && name === node.id) {
214-222: Complex nested condition is hard to follow.The condition at line 215 combines multiple checks in a single expression. Consider extracting to a helper function for readability.
Proposed refactor
+ const isSameElementType = (a: Node | Link, b: Node | Link) => + ('source' in a && 'source' in b) || (!('source' in a) && !('source' in b)); const handleRightClick = useCallback((element: GraphNode | GraphLink, evt: MouseEvent) => { // Find the full element from the graph let fullElement: Node | Link | undefined; if ('source' in element) { fullElement = graph.LinksMap.get(element.id); } else { fullElement = graph.NodesMap.get(element.id); } if (!fullElement) return; if (evt.ctrlKey) { - if (selectedElements.find(e => (("source" in e && "source" in fullElement) || (!("source" in e) && !("source" in fullElement))) && e.id === fullElement.id)) { + const isAlreadySelected = selectedElements.find(e => + isSameElementType(e, fullElement) && e.id === fullElement.id + ); + if (isAlreadySelected) { setSelectedElements(selectedElements.filter((el) => el !== fullElement)); } else { setSelectedElements([...selectedElements, fullElement]); }
244-246: Debug code assigns to global window object.Assigning
canvas.getGraphData()towindow[type]appears to be debug code. Consider removing or guarding with a development-only check.Proposed fix
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - (window as any)[type] = () => canvas.getGraphData(); + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any)[type] = () => canvas.getGraphData(); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/playwright.ymlapp/api/chat/route.tsxapp/components/ForceGraph.tsxapp/components/Header.tsxapp/graph/Chat.tsxapp/providers.tsxapp/settings/browserSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/components/Header.tsx
- app/settings/browserSettings.tsx
- .github/workflows/playwright.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/providers.tsxapp/components/ForceGraph.tsxapp/api/chat/route.tsxapp/graph/Chat.tsx
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/chat/route.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-13T09:41:15.717Z
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: .github/workflows/playwright.yml:264-264
Timestamp: 2025-11-13T09:41:15.717Z
Learning: The hardcoded ENCRYPTION_KEY value "a3f5d8c2b7e4f1a0c9d6e8b2f5a7c3d1e9f2a4b8c6d3e7f1a5b9c2d8e4f0a6b3" in .github/workflows/playwright.yml at the run-token-tests job is a test-only encryption key used for CI token tests and is not a production secret.
Applied to files:
app/providers.tsx
🧬 Code graph analysis (3)
app/components/ForceGraph.tsx (2)
app/api/graph/model.ts (8)
Graph(339-1182)GraphData(84-87)Node(56-68)Link(70-82)Relationship(183-189)Data(113-113)Data(445-447)Data(449-451)lib/utils.ts (2)
GraphRef(27-27)getTheme(337-351)
app/api/chat/route.tsx (2)
app/api/auth/[...nextauth]/options.ts (2)
session(382-401)getClient(405-483)app/api/validate-body.ts (2)
validateBody(294-310)chatRequest(201-226)
app/graph/Chat.tsx (3)
app/components/provider.ts (2)
IndicatorContext(275-278)BrowserSettingsContext(156-222)lib/utils.ts (2)
Message(63-74)cn(84-86)app/api/chat/route.tsx (1)
EventType(23-23)
🪛 Biome (2.1.2)
app/graph/Chat.tsx
[error] 139-143: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 197-197: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 239-245: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 257-257: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run cluster tests
- GitHub Check: Run tests part 3 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run token tests
- GitHub Check: Run TLS tests
- GitHub Check: security-scan
🔇 Additional comments (10)
app/api/chat/route.tsx (2)
6-21: LGTM!GET handler correctly validates authentication and returns appropriate responses.
28-40: LGTM!Proper URL encoding of credentials prevents injection issues and handles special characters correctly.
app/graph/Chat.tsx (1)
31-58: LGTM!The status message grouping logic and scroll behavior are implemented correctly.
app/providers.tsx (2)
233-241: LGTM!The
forceGraphContextmemoization correctly includes all dependencies and properly exposescanvasRef,graphData, andsetGraphDatafor the new canvas-based architecture.
679-733: LGTM!The provider hierarchy is correctly wired with
ForceGraphContext.Providerreplacing the previous viewport context, maintaining proper nesting order.app/components/ForceGraph.tsx (5)
96-109: Potential stale closure in cleanup effect.The cleanup function captures
canvasfrom the effect body, butsetViewportandsetGraphDatain the dependency array may cause the effect to re-run, makingcanvasreference potentially outdated. The current pattern is acceptable since you're re-capturingcanvasfromcanvasRef.currenton each run.
82-87: LGTM!Dynamic import ensures the web component is only loaded client-side, preventing SSR issues. The
canvasLoadedstate correctly gates subsequent canvas operations.
280-295: LGTM!The config setup effect properly updates all event handlers and selection callbacks, with correct dependency tracking.
315-317: No action needed—@falkordb/canvasprovides proper TypeScript declarations.The package
@falkordb/canvas(v0.0.24) is installed and provides TypeScript type definitions. The component correctly imports types from the package (GraphRef,Data,GraphNode, etc.) and thecanvasRefis properly typed. The<falkordb-canvas>web component has the necessary JSX intrinsic element declarations for TypeScript.
33-50: Verify canvas API support for node/link style properties.
convertToCanvasDataomitsexpand,collapsed,size, andcaptionfrom nodes and links. These properties are actively used throughout the app (style customization, node expansion, persistence). Confirm whether the@falkordb/canvasData type supports these fields. If not, add a comment explaining the intentional omission.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
e2e/tests/chat.spec.ts (1)
317-320: Consider using a stricter assertion once tooltip behavior stabilizes.The disjunctive check
tooltip.includes("Bob") || tooltip.includes("1")accommodates both the old (name-based) and new (ID-based) tooltip formats during the canvas migration. However, this permissiveness could mask regressions if the expected behavior changes unexpectedly.Once the migration is complete and tooltip behavior is deterministic, consider tightening this assertion to expect only the correct value. If both are valid outputs depending on configuration, add a comment explaining when each case applies.
♻️ Suggested improvement (post-migration)
- // Get the tooltip content and verify it contains "Bob" or "1" (node ID) - const tooltip = await chat.getNodeCanvasToolTip(); - expect(tooltip).toBeDefined(); - expect(tooltip && (tooltip.includes("Bob") || tooltip.includes("1"))).toBeTruthy(); + // Get the tooltip content and verify it contains the expected node identifier + const tooltip = await chat.getNodeCanvasToolTip(); + expect(tooltip).toBeDefined(); + // TODO: Once canvas migration is complete, assert the specific expected value + expect(tooltip).toContain("1"); // Node ID in new canvas renderingapp/graph/Chat.tsx (8)
1-2: ESLint disable comments don't suppress external static analysis.The
/* eslint-disable no-case-declarations */pragma only affects ESLint. Biome (the static analysis tool) still flags switch declaration issues at multiple locations. Consider wrapping case block variables in braces to satisfy both tools and improve code clarity.
138-146: Wrap case declarations in block scope.The
messageconstant is declared without block scope, which Biome flags asnoSwitchDeclarations. This can lead to accidental access from other case clauses.♻️ Proposed fix
case "Status": + { const message = { role: "assistant" as const, content: eventData.trim(), type: eventType }; setMessages(prev => [...prev, message]); - break; + break; + }
196-208: Wrap case declarations in block scope.The
statusCodeconstant at line 197 needs block scope to prevent accidental access from other cases.♻️ Proposed fix
case "error": + { const statusCode = Number(line.split("status:")[1].split(" ")[0]); if (statusCode === 401 || statusCode >= 500) setIndicator("offline"); toast({ title: "Error", description: eventData, variant: "destructive", }); isResult = true; - break; + break; + }
106-126: Consider adding AbortController for fetch cleanup.If the component unmounts during an active stream (e.g., user closes the chat panel),
processStreamwill continue attempting state updates on an unmounted component. This can cause memory leaks and React warnings.♻️ Suggested approach
// In component body, create ref for AbortController const abortControllerRef = useRef<AbortController | null>(null); // In handleSubmit, before fetch: abortControllerRef.current = new AbortController(); const response = await fetch("/api/chat", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body), signal: abortControllerRef.current.signal }); // Add useEffect cleanup: useEffect(() => { return () => { abortControllerRef.current?.abort(); }; }, []);
238-255: Wrap case declarations in block scope.The
contentconstant in case"Status"needs block scope per static analysis.♻️ Proposed fix
case "Status": + { const content = <> { messages[messages.length - 1].type === "Status" && messages[messages.length - 1] === message && <Loader2 className="animate-spin" size={15} /> } <p className="text-sm">{message.content}</p> </>; return index !== undefined ? ( <li className="flex gap-2 items-center" key={index}> {content} </li> ) : ( <div className="flex gap-2 items-center"> {content} </div> ); + }
256-311: Wrap case declarations in block scope.The
iconstant in case"CypherQuery"needs block scope.♻️ Proposed fix
case "CypherQuery": + { const i = messages.findIndex(m => m === message); return ( // ... rest of JSX ); + }
400-417: Consider adding aria-label for accessibility.The input field relies solely on a placeholder for context. Screen reader users would benefit from an explicit label.
♻️ Optional improvement
<Input data-testid="chatInput" className="w-1 grow bg-transparent border-none text-foreground text-lg SofiaSans" placeholder="Type your message here..." + aria-label="Chat message" value={newMessage} onChange={(e) => setNewMessage(e.target.value)} />
53-58: Consider using useRef instead of querySelector.Direct DOM queries work but are less idiomatic in React. A ref would be more reliable if the class name changes.
♻️ Optional improvement
const chatContainerRef = useRef<HTMLUListElement>(null); const scrollToBottom = () => { if (chatContainerRef.current) { chatContainerRef.current.scrollTop = chatContainerRef.current.scrollHeight; } }; // In JSX: <ul ref={chatContainerRef} className="... chat-container">
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/graph/Chat.tsxe2e/tests/chat.spec.tse2e/tests/graph.spec.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/tests/graph.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
e2e/tests/chat.spec.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/tests/chat.spec.ts
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/Chat.tsx
🧬 Code graph analysis (1)
app/graph/Chat.tsx (3)
app/components/provider.ts (4)
IndicatorContext(275-278)GraphContext(224-245)QueryLoadingContext(285-288)BrowserSettingsContext(156-222)lib/utils.ts (2)
Message(63-74)cn(84-86)app/api/chat/route.tsx (1)
EventType(23-23)
🪛 Biome (2.1.2)
app/graph/Chat.tsx
[error] 139-143: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 197-197: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 239-245: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 257-257: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run settings tests
- GitHub Check: Run cluster tests
- GitHub Check: Run token tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 3 (except settings)
- GitHub Check: Run TLS tests
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: security-scan
🔇 Additional comments (2)
app/graph/Chat.tsx (2)
31-51: LGTM!The message grouping logic correctly consolidates consecutive Status messages. The effect has proper dependencies.
160-180: LGTM!Good defensive parsing with graceful fallback to an error message when JSON parsing fails.
Anchel123
requested review from
Naseem77 and
barakb
and removed request for
Naseem77
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
PR Summary by Typo
Overview
This pull request migrates the graph visualization component from
react-force-graph-2dto the new@falkordb/canvaslibrary. This significant change updates the underlying graph rendering technology, streamlines data handling, and modernizes the visualization stack.Key Changes
react-force-graph-2dwith@falkordb/canvasas the primary graph visualization library.Node,Link) and utility functions to align with the@falkordb/canvasAPI.ForceGraphcomponent to utilize the new web component, removing custom D3 force calculations and canvas rendering logic.DataPanel,DataTable,GraphView,SchemaView,Toolbar) to interact with the new canvas component and its data model.ForceGraphContextto manage graph visualization state, replacing the previousViewportContext.Work Breakdown
To turn off PR summary, please visit Notification settings.