Fix #342 fix canvas component #342
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces enhancements to the graph visualization and interaction components across multiple files. The changes focus on improving node and chart interactions, particularly in the Changes
Sequence DiagramsequenceDiagram
participant User
participant GraphView
participant Chart
participant NodeManager
User->>GraphView: Single Click Node
GraphView->>NodeManager: Select Node
User->>GraphView: Double Click Node
GraphView->>NodeManager: Expand/Collapse Node
GraphView->>Chart: Zoom to Fit
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Anchel123
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR moves edge curve calculations from render-time to creation-time in the graph visualization system, improving performance and maintainability.
- Removed dynamic curve calculations from
app/components/graphView.tsxduring rendering phase - Added curve pre-calculation logic in
app/components/model.tsduring edge creation - Added fallback node creation in
model.tswhen edges reference missing nodes - Introduced default value (0) for
getCategoryColorValue()to prevent undefined colors - Left debug
console.log(link.curve)that should be removed frommodel.ts
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
2 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
Anchel123
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (5)
app/components/model.ts:233
- The 'index' variable should be checked for 'undefined' before using it. Suggestion: 'const index = sameNodesLinks.findIndex(l => l.id === link.id); const safeIndex = index === -1 ? 0 : index;'
const index = sameNodesLinks.findIndex(l => l.id === link.id) ?? 0
app/components/code-graph.tsx:217
- The function name 'handelSearchSubmit' is misspelled. It should be 'handleSearchSubmit'.
const handelSearchSubmit = (node: any) => {
app/components/code-graph.tsx:233
- The use of the non-null assertion operator 'chartNode!' can be risky. Ensure 'chartNode' is not null or undefined before using it.
graph.visibleLinks(true, [chartNode!.id])
app/components/chat.tsx:6
- The slice operation is incorrect when
indexis 0 or 1. It should be adjusted to handle these cases properly.
RemoveLastPath = (messages: Message[]) => {
app/components/chat.tsx:136
- The function name
handelSetSelectedPathshould be corrected tohandleSetSelectedPath.
handelSetSelectedPath(p)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
app/components/code-graph.tsx (1)
Line range hint
74-94: Fix React Hook dependency warnings.The useEffect hooks are missing dependencies which could lead to stale closures.
Add the missing dependencies to the dependency arrays:
useEffect(() => { setData({ ...graph.Elements }) -}, [graph.Id]) +}, [graph.Id, graph.Elements, setData]) useEffect(() => { if (!selectedValue) return handleSelectedValue(selectedValue) -}, [selectedValue]) +}, [selectedValue, handleSelectedValue]) useEffect(() => { const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Delete') { if (selectedObj && selectedObjects.length === 0) return handelRemove([...selectedObjects.map(obj => obj.id), selectedObj?.id].filter(id => id !== undefined)); } }; document.addEventListener('keydown', handleKeyDown); return () => { document.removeEventListener('keydown', handleKeyDown); }; -}, [selectedObjects, selectedObj]); +}, [selectedObjects, selectedObj, handelRemove]);app/components/chat.tsx (1)
Line range hint
137-155: Fix missing dependencies in useEffect hooks.The following useEffect hooks have missing dependencies that should be included:
- Line 137: Add 'handelSetSelectedPath' and 'paths'
- Line 148: Add 'handleSubmit'
- Line 155: Add 'setIsPathResponse'
- useEffect(() => { + useEffect(() => { const p = paths.find((path) => [...path.links, ...path.nodes].some((e: any) => e.id === selectedPathId)) if (!p) return handelSetSelectedPath(p) - }, [selectedPathId]) + }, [selectedPathId, handelSetSelectedPath, paths]) - useEffect(() => { + useEffect(() => { handleSubmit() - }, [path]) + }, [path, handleSubmit]) - useEffect(() => { + useEffect(() => { if (isPathResponse || isPathResponse === undefined) return setIsPathResponse(false) setSelectedPath(undefined) setPaths([]) - }, [isPathResponse]) + }, [isPathResponse, setIsPathResponse])Note: If any of these dependencies cause infinite loops, consider using useCallback for the functions or moving state updates outside the effect.
🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🪛 GitHub Actions: Playwright Tests
[warning] 155-155: React Hook useEffect has a missing dependency: 'setIsPathResponse'. Either include it or remove the dependency array.
app/components/graphView.tsx (2)
Line range hint
65-69: Fix missing dependencies in useEffect hooksBoth useEffect hooks are missing dependencies in their dependency arrays.
Apply these changes to fix the dependency warnings:
useEffect(() => { setCooldownTime(4000) setCooldownTicks(undefined) - }, [graph.Id]) + }, [graph.Id, setCooldownTime, setCooldownTicks]) useEffect(() => { setCooldownTime(1000) setCooldownTicks(undefined) - }, [graph.getElements().length]) + }, [graph.getElements().length, setCooldownTime, setCooldownTicks])Also applies to: 70-74
Line range hint
266-269: Update event handler referencesUpdate the event handler references to match the renamed functions.
- onNodeClick={handelNodeClick} + onNodeClick={handleNodeClick} onNodeDragEnd={(n, translate) => setPosition(prev => { return prev && { x: prev.x + translate.x * chartRef.current.zoom(), y: prev.y + translate.y * chartRef.current.zoom() } })} - onNodeRightClick={handelNodeRightClick} + onNodeRightClick={handleNodeRightClick}
🧹 Nitpick comments (8)
app/components/model.ts (2)
179-210: Improve error handling for missing nodes.The node creation logic handles missing source/target nodes by creating placeholder nodes. While this prevents runtime errors, it might hide underlying data integrity issues.
Consider adding warning logs when placeholder nodes are created:
if (!source) { + console.warn(`Creating placeholder node for missing source: ${edgeData.src_node}`); source = { id: edgeData.src_node, name: edgeData.src_node, // ... rest of the properties } this.nodesMap.set(edgeData.src_node, source) }
237-253: Consider extracting curve calculation constants.The curve calculation uses magic numbers that could be made more maintainable.
Consider this refactor:
+const CURVE_CONFIG = { + SELF_LOOP: { + BASE_OFFSET: 3, + SPACING: 1 + }, + EDGE: { + SPACING: 1 + }, + SCALE: 0.1 +}; if (start.id === end.id) { if (even) { - curve = Math.floor(-(index / 2)) - 3 + curve = Math.floor(-(index / 2)) - CURVE_CONFIG.SELF_LOOP.BASE_OFFSET } else { - curve = Math.floor((index + 1) / 2) + 2 + curve = Math.floor((index + 1) / 2) + (CURVE_CONFIG.SELF_LOOP.BASE_OFFSET - 1) } } -link.curve = curve * 0.1 +link.curve = curve * CURVE_CONFIG.SCALEapp/components/code-graph.tsx (1)
219-240: Fix typo in function name and improve search node handling.The implementation has a few issues:
- The function name has a typo:
handelSearchSubmitshould behandleSearchSubmit- The setTimeout with 0ms delay could be replaced with requestAnimationFrame
Consider this refactor:
-const handelSearchSubmit = (node: any) => { +const handleSearchSubmit = (node: any) => { const chart = chartRef.current if (chart) { const n = { name: node.properties.name, id: node.id } let chartNode = graph.Elements.nodes.find(n => n.id == node.id) if (!chartNode?.visible) { if (!chartNode) { chartNode = graph.extend({ nodes: [node], edges: [] }).nodes[0] } else { chartNode.visible = true setCooldownTicks(undefined) setCooldownTime(1000) } graph.visibleLinks(true, [chartNode!.id]) setData({ ...graph.Elements }) } setSearchNode(n) - setTimeout(() => { + requestAnimationFrame(() => { chart.zoomToFit(1000, 150, (n: NodeObject<Node>) => n.id === chartNode!.id); - }, 0) + }) } }app/components/chat.tsx (4)
88-88: Consider using a more specific type for chartRef.Instead of using
any, consider using the proper type from react-force-graph-2d for better type safety.- chartRef: any + chartRef: React.RefObject<ForceGraphMethods>
158-160: Fix typo in function name.The function name has a typo: "handel" should be "handle".
-const handelSetSelectedPath = (p: PathData) => { +const handleSetSelectedPath = (p: PathData) => {
213-215: Add error handling for chart zoom operation.The zoom operation should be wrapped in a try-catch block to handle potential errors gracefully.
setTimeout(() => { + try { chart.zoomToFit(1000, 150, (n: NodeObject<Node>) => p.nodes.some(node => node.id === n.id)); + } catch (error) { + console.error('Failed to zoom chart:', error); + } }, 0)
311-313: Add consistent error handling for chart zoom operation.Similar to the previous zoom operation, this should also be wrapped in a try-catch block.
setTimeout(() => { + try { chart.zoomToFit(1000, 150, (n: NodeObject<Node>) => formattedPaths.some(p => p.nodes.some(node => node.id === n.id))); + } catch (error) { + console.error('Failed to zoom chart:', error); + } }, 0)app/components/graphView.tsx (1)
Line range hint
81-95: Fix typo in function name 'handel'The function name should be 'handle' instead of 'handel'.
- const handelNodeRightClick = (node: Node, evt: MouseEvent) => { + const handleNodeRightClick = (node: Node, evt: MouseEvent) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/chat.tsx(7 hunks)app/components/code-graph.tsx(2 hunks)app/components/graphView.tsx(3 hunks)app/components/model.ts(3 hunks)app/page.tsx(1 hunks)tailwind.config.js(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Playwright Tests
app/components/graphView.tsx
[warning] 68-68: React Hook useEffect has missing dependencies: 'setCooldownTicks' and 'setCooldownTime'. Either include them or remove the dependency array.
[warning] 73-73: React Hook useEffect has missing dependencies: 'setCooldownTicks' and 'setCooldownTime'. Either include them or remove the dependency array.
app/components/code-graph.tsx
[warning] 74-74: React Hook useEffect has missing dependencies: 'graph.Elements' and 'setData'. Either include them or remove the dependency array.
[warning] 79-79: React Hook useEffect has a missing dependency: 'handleSelectedValue'. Either include it or remove the dependency array.
[warning] 94-94: React Hook useEffect has a missing dependency: 'handelRemove'. Either include it or remove the dependency array.
[warning] 148-148: React Hook useEffect has a missing dependency: 'fetchCount'. Either include it or remove the dependency array.
app/components/chat.tsx
[warning] 137-137: React Hook useEffect has missing dependencies: 'handelSetSelectedPath' and 'paths'. Either include them or remove the dependency array.
[warning] 148-148: React Hook useEffect has a missing dependency: 'handleSubmit'. Either include it or remove the dependency array.
[warning] 155-155: React Hook useEffect has a missing dependency: 'setIsPathResponse'. Either include it or remove the dependency array.
🔇 Additional comments (6)
tailwind.config.js (1)
82-82: LGTM! Verify if the spin animation is needed.The animation configuration is correctly defined. However, since this change appears unrelated to the PR's objective of checking multi-edges, please verify if this animation is actually used in the codebase.
app/components/model.ts (1)
229-253: LGTM! Efficient multi-edge handling.The curve calculation for multiple edges between the same nodes is now handled during creation instead of render time, which is more efficient. The implementation correctly:
- Identifies edges between the same nodes
- Calculates appropriate curve values based on edge index
- Handles both self-loops and regular edges differently
app/page.tsx (1)
284-284: LGTM! Clean prop addition.The chartRef prop is correctly passed to the Chat component, enabling graph manipulation capabilities.
app/components/chat.tsx (1)
Line range hint
1-500: Verify alignment with PR objectives.The PR's objective is to "Fix #342 check for multi edges on create instead per render", but the changes appear to focus on chart zooming functionality. Could you clarify how these changes address the multi-edge checking issue?
app/components/graphView.tsx (2)
64-64: LGTM: lastClick ref implementationThe ref is correctly initialized and used for double-click detection.
Line range hint
1-275: Verify PR objective implementationWhile the changes improve node interactions, they don't appear to address the stated PR objective of "checking for multi edges on create instead per render". Consider adding logic to check for and handle multiple edges during node/edge creation rather than during the render phase.
Would you like me to:
- Help identify where multi-edge checking should be implemented
- Generate a solution for handling multi-edges during creation
app/components/graphView.tsx
Outdated
| const handelNodeClick = async (node: Node) => { | ||
| const now = new Date() | ||
|
|
||
| if (elements.nodes.length === 0) { | ||
| toast({ | ||
| title: `No neighbors found`, | ||
| description: `No neighbors found`, | ||
| }) | ||
| return | ||
| const isDoubleClick = now.getTime() - lastClick.current.getTime() < 1000 | ||
| lastClick.current = now | ||
|
|
||
| if (isDoubleClick) { | ||
| const expand = !node.expand | ||
|
|
||
| if (expand) { | ||
| const elements = await onFetchNode([node.id]) | ||
|
|
||
| if (elements.nodes.length === 0) { | ||
| toast({ | ||
| title: `No neighbors found`, | ||
| description: `No neighbors found`, | ||
| }) | ||
|
|
||
| return | ||
| } | ||
| } else { | ||
| deleteNeighbors([node]); | ||
| } | ||
| } else { | ||
| deleteNeighbors([node]); | ||
| } | ||
|
|
||
| node.expand = expand | ||
| node.expand = expand | ||
|
|
||
| setSelectedObj(undefined) | ||
| setData({ ...graph.Elements }) | ||
| setSelectedObj(undefined) | ||
| setData({ ...graph.Elements }) | ||
|
|
||
| } else if (isShowPath) { | ||
| setPath(prev => { | ||
| if (!prev?.start?.name || (prev.end?.name && prev.end?.name !== "")) { | ||
| return ({ start: { id: Number(node.id), name: node.name } }) | ||
| } else { | ||
| return ({ end: { id: Number(node.id), name: node.name }, start: prev.start }) | ||
| } | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Improve error handling and fix naming convention
The review comment is accurate. The code needs the suggested improvements:
- The function name typo should be fixed ('handel' → 'handle')
- The toast message is indeed duplicated and should be more descriptive
- The implementation lacks multi-edge validation during edge creation, which should be addressed as per the PR objective
🔗 Analysis chain
Improve error handling and fix naming convention
The node click handler needs improvements:
- Function name has a typo ('handel' → 'handle')
- Toast error message is duplicated
- The implementation doesn't address the PR objective of checking multi-edges on create
Apply these changes:
- const handelNodeClick = async (node: Node) => {
+ const handleNodeClick = async (node: Node) => {
const now = new Date()
const isDoubleClick = now.getTime() - lastClick.current.getTime() < 1000
lastClick.current = now
if (isDoubleClick) {
const expand = !node.expand
if (expand) {
const elements = await onFetchNode([node.id])
if (elements.nodes.length === 0) {
toast({
title: `No neighbors found`,
- description: `No neighbors found`,
+ description: `No connected nodes found for ${node.name}`,
})
return
}The PR objective mentions checking for multi-edges on create, but this implementation seems to handle it during render. Let's verify the implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for multi-edge checking logic in the codebase
rg -l "multi.*edge|parallel.*edge" --type ts --type tsx
Length of output: 85
Script:
#!/bin/bash
# Search for edge-related logic with broader terms
rg -l "edge|createEdge|addEdge|validateEdge" --type ts
# Search for edge type definitions
ast-grep --pattern 'type $_ = {
$$$
edge$_
$$$
}'
# Search for edge-related functions
ast-grep --pattern 'function $_($$$) {
$$$
edge$_
$$$
}'
Length of output: 436
Script:
#!/bin/bash
# Check edge-related code in model.ts
rg "edge|Edge" -A 5 -B 5 app/components/model.ts
# Look for edge creation/validation in code-graph.tsx
rg "edge|Edge" -A 5 -B 5 app/components/code-graph.tsx
# Search for specific edge validation or creation functions
ast-grep --pattern 'function $_($$$) {
$$$
new Edge($$$)
$$$
}'
Length of output: 4937
Anchel123
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Refactor