Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPR refactors core graph/model/types and replaces ForceGraph2D with a new canvas-based ForceGraph, expands Chat/CodeGraph public props to drive state via refs, adds drawer/carousel UI, updates e2e tests and tooling, and introduces docker-compose plus infrastructure/config updates. Changes
Sequence DiagramsequenceDiagram
participant User
participant Page as Page
participant CodeGraph
participant Chat
participant ForceGraph
participant Canvas as `@falkordb/canvas`
participant API as Backend
User->>Page: search/select node
Page->>CodeGraph: handleSearchSubmit(searchNode)
CodeGraph->>ForceGraph: update data via canvasRef
ForceGraph->>Canvas: convertToCanvasData / setConfig
Canvas-->>User: render graph
User->>Canvas: click/right-click node
Canvas-->>ForceGraph: onNodeClick/onRightClick
ForceGraph->>CodeGraph: selected Node|Link
CodeGraph->>Chat: update selectedPath/messages
Chat->>API: request path data
API-->>Chat: path response
Chat->>CodeGraph: addGraphElements
CodeGraph->>ForceGraph: merge new nodes/links
ForceGraph->>Canvas: update data / re-render
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 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
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Hi! Looks like you've reached your API usage limit. You can increase it from your account settings page here: app.greptile.com/settings/billing/code-review
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
9 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
app/components/graphView.tsx (1)
Line range hint
105-166: Improve double-click detection implementation.The current implementation has potential issues:
- The double-click detection uses a fixed 1000ms threshold which might not match the system's double-click speed setting.
- The implementation doesn't handle the case where a user clicks different nodes within the threshold.
Consider this improved implementation:
-const lastClick = useRef<{ date: Date, name: string }>({ date: new Date(), name: "" }) +const lastClick = useRef<{ time: number, id: number }>({ time: 0, id: 0 }) -const isDoubleClick = now.getTime() - date.getTime() < 1000 && name === node.name +const isDoubleClick = (now.getTime() - lastClick.current.time < 500) && (lastClick.current.id === node.id) +lastClick.current = { time: now.getTime(), id: node.id }app/components/chat.tsx (1)
Line range hint
157-215: Improve zoom functionality implementation.Several improvements can be made to the zoom functionality:
- The setTimeout with 0ms delay is unnecessary
- Magic numbers should be constants
- Add proper error handling for undefined chart
Apply these changes:
+const ZOOM_DURATION = 1000; +const ZOOM_PADDING = 150; const handleSetSelectedPath = (p: PathData) => { const chart = chartRef.current - if (!chart) return + if (!chart) { + console.warn('Chart reference is not available'); + return; + } // ... rest of the function ... - setTimeout(() => { - chart.zoomToFit(1000, 150, (n: NodeObject<Node>) => p.nodes.some(node => node.id === n.id)); - }, 0) + chart.zoomToFit(ZOOM_DURATION, ZOOM_PADDING, (n: NodeObject<Node>) => p.nodes.some(node => node.id === n.id)); }🧰 Tools
🪛 Biome (1.9.4)
[error] 163-163: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🧹 Nitpick comments (5)
e2e/tests/chat.spec.ts (1)
113-113: Remove unnecessary empty line within the loop.The empty line within the loop affects readability without adding value.
for (let i = 0; i < 3; i++) { await chat.sendMessage(Node_Question); const result = await chat.getTextInLastChatElement(); const number = result.match(/\d+/g)?.[0]!; responses.push(number); - }app/components/model.ts (2)
179-215: Add error handling for edge cases.While the node creation logic is sound, it would benefit from additional error handling for edge cases.
let source = this.nodesMap.get(edgeData.src_node) let target = this.nodesMap.get(edgeData.dest_node) + if (edgeData.src_node === undefined || edgeData.dest_node === undefined) { + console.warn('Invalid edge data: Missing source or destination node ID'); + return; + } if (!source) {
229-253: Add documentation for curve calculation logic.The curve calculation logic is complex and would benefit from documentation explaining the mathematical reasoning.
Add a comment block explaining the curve calculation:
+ // Calculate curve values for graph edges: + // - For self-loops (start === end): + // - Even indices: negative values starting from -3 + // - Odd indices: positive values starting from 2 + // - For regular edges: + // - Even indices: negative values starting from 0 + // - Odd indices: positive values starting from 1 + // The final curve value is scaled by 0.1 to create subtle curves newElements.links.forEach(link => {e2e/logic/POM/codeGraph.ts (1)
284-285: Consider removing redundant delay.The code waits for the loading image to be hidden and then adds an additional 2-second delay, which might be unnecessary.
Consider removing the redundant delay:
async getTextInLastChatElement(): Promise<string>{ await this.page.waitForSelector('img[alt="Waiting for response"]', { state: 'hidden' }); - await delay(2000); return (await this.lastElementInChat.textContent())!; }app/components/chat.tsx (1)
Line range hint
272-313: Extract common zoom functionality.The zoom logic is duplicated between
handleSetSelectedPathandhandleSubmit. Consider extracting it into a reusable function.Apply these changes:
+const ZOOM_DURATION = 1000; +const ZOOM_PADDING = 150; + +const zoomToNodes = (chart: ForceGraphMethods, nodes: any[]) => { + if (!chart) { + console.warn('Chart reference is not available'); + return; + } + chart.zoomToFit(ZOOM_DURATION, ZOOM_PADDING, (n: NodeObject<Node>) => + nodes.some(node => node.id === n.id) + ); +}; const handleSubmit = async () => { const chart = chartRef.current if (!chart) return // ... rest of the function ... - setTimeout(() => { - chart.zoomToFit(1000, 150, (n: NodeObject<Node>) => - formattedPaths.some(p => p.nodes.some(node => node.id === n.id)) - ); - }, 0) + zoomToNodes(chart, formattedPaths.flatMap(p => p.nodes)); } const handleSetSelectedPath = (p: PathData) => { // ... rest of the function ... - setTimeout(() => { - chart.zoomToFit(1000, 150, (n: NodeObject<Node>) => - p.nodes.some(node => node.id === n.id) - ); - }, 0) + zoomToNodes(chart, p.nodes); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/components/chat.tsx(9 hunks)app/components/code-graph.tsx(5 hunks)app/components/elementMenu.tsx(3 hunks)app/components/graphView.tsx(5 hunks)app/components/model.ts(3 hunks)app/page.tsx(1 hunks)e2e/logic/POM/codeGraph.ts(4 hunks)e2e/tests/canvas.spec.ts(1 hunks)e2e/tests/chat.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (11)
e2e/tests/chat.spec.ts (1)
120-121: LGTM! Good test isolation.Moving the API calls inside the test scope improves test isolation and follows testing best practices.
app/components/elementMenu.tsx (1)
17-17: LGTM! Consistent function naming.The typo fix from
handelExpandtohandleExpandis applied consistently across the component.Also applies to: 22-22, 71-71, 77-77, 117-117, 123-123
app/components/model.ts (1)
55-55: LGTM! Safe default parameter.Adding a default value of 0 ensures the function always returns a valid color.
app/page.tsx (1)
284-284: LGTM! Enhanced graph interaction.Adding the chartRef prop to the Chat component enables improved graph visualization capabilities.
e2e/tests/canvas.spec.ts (1)
46-46: LGTM! Good improvement to test reliability.Adding
clickCenter()before zoom operations ensures consistent test behavior by establishing a known starting position.app/components/graphView.tsx (2)
64-87: Good implementation of responsive graph sizing.The resize observer implementation properly handles both window resizing and parent container changes. The cleanup in the
useEffecthook correctly removes event listeners and disconnects the observer.
173-174: LGTM! Proper usage of dynamic dimensions.Using state-managed dimensions ensures the graph properly responds to container size changes.
app/components/code-graph.tsx (1)
Line range hint
244-357: LGTM! Good cleanup of handler names.The renaming from 'handel*' to 'handle*' improves code consistency and fixes typos.
e2e/logic/POM/codeGraph.ts (2)
155-157: LGTM! Good addition of loading state locator.The new locator helps track loading states consistently across tests.
426-426: Verify the impact of changing to right-click.The change from left-click to right-click for node selection might affect other tests that depend on this behavior.
Let's verify the impact:
✅ Verification successful
Right-click change appears safe to implement
The tests use the
nodeClick()abstraction method rather than direct click events, focusing on outcomes rather than the click mechanism. The change from left to right click is an implementation detail that won't affect the test behavior as long as thenodeClick()method maintains its core functionality of node selection.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests that might be affected by the click behavior change rg -l "nodeClick" "e2e/tests"Length of output: 87
Script:
#!/bin/bash # Check the content of test files for click-related code echo "=== canvas.spec.ts ===" rg -A 5 -B 5 "click" "e2e/tests/canvas.spec.ts" echo -e "\n=== nodeDetailsPanel.spec.ts ===" rg -A 5 -B 5 "click" "e2e/tests/nodeDetailsPanel.spec.ts"Length of output: 8469
app/components/chat.tsx (1)
110-110: LGTM!The function signature has been correctly updated to include the new
chartRefprop.
… handling and neighbor deletion logic. Updated deleteNeighbors function to handle expanded nodes correctly and replaced direct calls with handleExpand in GraphView for better clarity and maintainability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/components/Input.tsx (1)
Line range hint
201-201: Warning: Dynamic Tailwind class names may not work in production.The dynamic class name
bg-${colorName}might not survive Tailwind's purge in production. Consider using style objects or safelist these color classes in your Tailwind config.Example safelist configuration in
tailwind.config.js:module.exports = { safelist: [ { pattern: /bg-(red|blue|green|yellow|purple|pink|indigo|gray)-/, variants: ['hover', 'focus', 'active'] } ] }
🧹 Nitpick comments (1)
app/components/Input.tsx (1)
Line range hint
147-160: Consider extracting input validation logic.While the validation works correctly, consider extracting it into a reusable function for better maintainability.
+const INVALID_CHARS_PATTERN = /[%*()\-\[\]{};:"|~]/; +const getInvalidCharacterMessage = (char: string) => + `The character "${char}" is not allowed in this field.`; + +const validateInput = (value: string): { isValid: boolean; message: string } => { + const match = value.match(INVALID_CHARS_PATTERN); + return match + ? { isValid: false, message: getInvalidCharacterMessage(match[0]) } + : { isValid: true, message: '' }; +}; + onChange={(e) => { const newVal = e.target.value - const invalidChars = /[%*()\-\[\]{};:"|~]/; - - if (invalidChars.test(newVal)) { - e.target.setCustomValidity(`The character "${newVal.match(invalidChars)?.[0]}" is not allowed in this field.`); + const { isValid, message } = validateInput(newVal); + if (!isValid) { + e.target.setCustomValidity(message); e.target.reportValidity(); return; } e.target.setCustomValidity(''); onValueChange({ name: newVal }) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/Input.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (2)
app/components/Input.tsx (2)
18-18: LGTM! Good architectural improvement.The removal of the
valueprop in favor of usingnode?.namemakes the component more focused and reduces prop drilling.
Line range hint
48-79: LGTM! Well-implemented search functionality.The implementation includes:
- Proper debouncing with timeout
- Clean error handling with user feedback
- Appropriate cleanup in useEffect
Contributor
✅ Actions performedFull review triggered. |
…ality and performance
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/chat.tsx (1)
64-69:⚠️ Potential issue | 🟡 MinorMissing
pathsin useEffect dependency array.The effect accesses
pathsto find the selected path, butpathsis not in the dependency array. This could cause stale closure issues where updates topathsdon't trigger re-evaluation.Proposed fix
useEffect(() => { const p = paths.find((path) => [...path.links, ...path.nodes].some((e: any) => e.id === selectedPathId)) if (!p) return handleSetSelectedPath(p) - }, [selectedPathId]) + }, [selectedPathId, paths])Note: You may also need to add
handleSetSelectedPathto the dependency array or wrap it inuseCallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/chat.tsx` around lines 64 - 69, The effect using useEffect reads `paths` to find the selected path but only lists `selectedPathId` in its dependency array, causing stale closures; update the dependency array for that useEffect to include `paths` (and also include or memoize `handleSetSelectedPath`) so the effect re-runs whenever `paths` or the setter change; alternatively wrap `handleSetSelectedPath` in useCallback and include it in the dependency list to avoid unnecessary re-renders.
♻️ Duplicate comments (9)
playwright.config.ts (1)
22-23:⚠️ Potential issue | 🟡 MinorUpdate the stale CI parallelism comment.
Line 22 says CI opts out of parallel tests, but Line 23 explicitly sets 3 workers on CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 22 - 23, The comment above the Playwright config is stale: it claims CI opts out of parallel tests but the workers property explicitly sets workers: process.env.CI ? 3 : undefined. Fix by making them consistent: either change the comment to state that CI runs with 3 workers (e.g., "Use 3 workers on CI") or change the code to opt out (set workers: process.env.CI ? undefined : undefined or workers: process.env.CI ? 1 : undefined) depending on intent; update the comment near the workers property to accurately describe the behavior of the workers setting.README.md (2)
109-109:⚠️ Potential issue | 🟡 MinorFix the GitHub Issues link.
The link points to
GraphRAG-SDKbut this README is for thecode-graphrepository.🔧 Proposed fix
-* [GitHub Issues](https://github.com/FalkorDB/GraphRAG-SDK/issues) +* [GitHub Issues](https://github.com/FalkorDB/code-graph/issues)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 109, Update the GitHub Issues link in README.md: replace the existing link text "[GitHub Issues](https://github.com/FalkorDB/GraphRAG-SDK/issues)" so it points to the code-graph repository issues URL (e.g., https://github.com/FalkorDB/code-graph/issues), ensuring the "[GitHub Issues]" entry now references the correct repository; keep the link label unchanged.
9-9:⚠️ Potential issue | 🟡 MinorRemove stray character.
Line 9 contains a lone
-which appears to be a leftover from editing.🔧 Proposed fix
-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 9, Remove the stray lone '-' character found in the README (the isolated dash on line 9) by deleting that character so the file contains only intended content; ensure no other stray punctuation remains and run a quick preview to confirm formatting is unchanged.app/globals.css (1)
132-139:⚠️ Potential issue | 🟡 MinorRemove duplicate
@layer basedeclaration.This block duplicates the
@layer basedeclaration at lines 85-94. Additionally, line 137 missesfont-robertothat exists in the original at line 91.🔧 Proposed fix
-@layer base { - * { - `@apply` border-border; - } - body { - `@apply` bg-background text-foreground; - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/globals.css` around lines 132 - 139, Remove the duplicate `@layer` base block and merge its rules into the existing `@layer` base (avoid repeating the `@layer` base declaration); specifically ensure the body selector inside the retained `@layer` base includes the missing font utility by adding the font-roberto utility to the existing body `@apply` (so body uses bg-background text-foreground and font-roberto) and keep the universal * { `@apply` border-border } rule in that single `@layer` base.app/components/dataPanel.tsx (1)
21-27:⚠️ Potential issue | 🟡 MinorRemove duplicated
isPathSelectedfromexcludedProperties.The same key appears twice, which is redundant and easy to miss in future edits.
Suggested fix
"curve", "__indexColor", - "isPathSelected", "__controlPoints",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/dataPanel.tsx` around lines 21 - 27, The excludedProperties array contains a duplicate string "isPathSelected"; remove the redundant "isPathSelected" entry from the excludedProperties definition (leave a single occurrence) to avoid redundancy and potential confusion when editing, locating the array by the symbol name excludedProperties in dataPanel.tsx and updating that list accordingly.e2e/tests/canvas.spec.ts (1)
222-225:⚠️ Potential issue | 🟡 MinorRemove unsafe non-null assertions before API/path assertions.
Lines 222, 224, and 225 still rely on
!for node ids; this was already flagged and remains brittle.Suggested fix
expect(firstNodeRes).toBeDefined(); expect(secondNodeRes).toBeDefined(); + if (!firstNodeRes || !secondNodeRes) throw new Error("Path nodes were not found in graph payload"); const api = new ApiCalls(); - const response = await api.showPath(GRAPHRAG_SDK ,firstNodeRes!.id, secondNodeRes!.id); + const response = await api.showPath(GRAPHRAG_SDK, firstNodeRes.id, secondNodeRes.id); const callsRelationObject = response.result.paths[0].find(item => item.relation === "CALLS") - expect(callsRelationObject?.src_node).toBe(firstNodeRes!.id); - expect(callsRelationObject?.dest_node).toBe(secondNodeRes!.id); + expect(callsRelationObject?.src_node).toBe(firstNodeRes.id); + expect(callsRelationObject?.dest_node).toBe(secondNodeRes.id);#!/bin/bash # Verify remaining non-null id assertions in canvas tests rg -nP '!\.id' e2e/tests/canvas.spec.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/canvas.spec.ts` around lines 222 - 225, Replace unsafe non-null assertions by asserting presence and guarding before accessing ids: before calling api.showPath(GRAPHRAG_SDK, firstNodeRes!.id, secondNodeRes!.id) ensure firstNodeRes and secondNodeRes are defined (e.g., expect(firstNodeRes).toBeDefined(); expect(secondNodeRes).toBeDefined()) and then call showPath with their .id without using `!`; after getting response, assert response.result.paths exists and has at least one path, find callsRelationObject, and assert callsRelationObject is defined before checking callsRelationObject.src_node and callsRelationObject.dest_node (use expect(callsRelationObject).toBeDefined(); expect(callsRelationObject!.src_node).toBe(firstNodeRes.id); etc.).e2e/tests/chat.spec.ts (1)
69-87:⚠️ Potential issue | 🟡 MinorAvoid non-null assertions after optional chaining in regex extraction.
Lines 69 and 85 can fail with unclear errors when the regex does not match.
Suggested fix
const result = await chat.getTextInLastChatElement(); - const number = result.match(/\d+/g)?.[0]!; + const match = result.match(/\d+/g); + expect(match?.[0]).toBeDefined(); + const number = match![0]; @@ const uiResponse = await chat.getTextInLastChatElement(); - const number = uiResponse.match(/\d+/g)?.[0]!; - - expect(number).toEqual(apiResponse.result.response.match(/\d+/g)?.[0]); + const uiMatch = uiResponse.match(/\d+/g); + const apiMatch = apiResponse.result.response.match(/\d+/g); + expect(uiMatch?.[0]).toBeDefined(); + expect(apiMatch?.[0]).toBeDefined(); + expect(uiMatch![0]).toEqual(apiMatch![0]);#!/bin/bash # Verify remaining optional-chain non-null assertions in this file rg -nP '\.match\(/\\d\+\/g\)\?\.\[0\]!' e2e/tests/chat.spec.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/chat.spec.ts` around lines 69 - 87, The regex extractions using optional chaining plus non-null assertion (e.g., uiResponse.match(/\d+/g)?.[0]! and apiResponse.result.response.match(/\d+/g)?.[0]!) can throw unclear runtime errors when there's no match; update the test to safely handle missing matches by removing the non-null assertions and checking that the match exists before using it: capture the match arrays (from uiResponse.match and apiResponse.result.response.match), assert they are defined/contain at least one element (or fail the test with a clear message) and then compare the first element; look for the variables uiResponse, apiResponse, number and the test "Validate UI response matches API response for a given question in chat" to apply this change.package.json (1)
33-33:⚠️ Potential issue | 🟠 Major
nextversion concern appears unresolved from prior review.Line 33 still targets
^15.5.8, which was previously flagged for advisories. Please move to the first patched 15.x version (or newer supported line).Check official Next.js advisories/release notes for next@15.5.8 and identify the first patched 15.x version that resolves those advisories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 33, Update the "next" dependency in package.json (the "next" entry currently set to "^15.5.8") to the first patched 15.x release that resolves the flagged advisories (or to a newer supported release line if preferred); consult Next.js release notes/security advisories to identify that exact patched 15.x version and replace the version string accordingly, then run your lockfile update (npm/yarn/pnpm) and verify the new version is installed and tests/builds succeed.app/components/code-graph.tsx (1)
106-110:⚠️ Potential issue | 🟠 MajorFix Delete-key handling for single selection and link deletion.
Line 108 is still inverted, and Line 109 always deletes as
"nodes"even whenselectedObjis aLink. This blocks valid single-delete flows and can target the wrong entity type.Proposed fix
const handleKeyDown = (event: KeyboardEvent) => { if (event.key === 'Delete') { - if (selectedObj && selectedObjects.length === 0) return - handleRemove([...selectedObjects.map(obj => obj.id), selectedObj?.id].filter(id => id !== undefined), "nodes"); + if (!selectedObj && selectedObjects.length === 0) return + + const isSelectedObjLink = !!selectedObj && "source" in selectedObj + if (isSelectedObjLink && selectedObjects.length === 0) { + handleRemove([selectedObj.id], "links") + return + } + + const nodeIds = [ + ...selectedObjects.map(obj => obj.id), + ...(!isSelectedObjLink && selectedObj ? [selectedObj.id] : []), + ] + if (nodeIds.length === 0) return + handleRemove(nodeIds, "nodes") } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/code-graph.tsx` around lines 106 - 110, The Delete-key handler in handleKeyDown incorrectly returns when a single selection exists and always calls handleRemove with "nodes"; invert the early-return check so it returns only when there is no selection (i.e., if (!selectedObj && selectedObjects.length === 0) return), and determine the correct removal type by inspecting the selected item(s): if selectedObj?.type === "Link" (or instanceof Link depending on your model) call handleRemove(..., "links"), otherwise call handleRemove(..., "nodes"); when mixing selections, compute ids and types accordingly (e.g., map selectedObjects to their ids and ensure you pass the appropriate entity type rather than hardcoding "nodes").
🟡 Minor comments (12)
.github/dependabot.yml-13-16 (1)
13-16:⚠️ Potential issue | 🟡 MinorGroup name doesn't match the configured update types.
The group is named
npm-minor-patch, suggesting it should include both minor and patch updates, but onlypatchis specified inupdate-types. Either:
- Add
- "minor"if the intent is to group both minor and patch updates, or- Rename the group to
npm-patchto accurately reflect its scope.Option A: Include minor updates
groups: npm-minor-patch: update-types: - "patch" + - "minor"Option B: Rename group to match scope
groups: - npm-minor-patch: + npm-patch: update-types: - "patch"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/dependabot.yml around lines 13 - 16, The group name "npm-minor-patch" in the dependabot configuration doesn't match its configured update-types; either add "- \"minor\"" to the update-types list to include minor and patch updates, or rename the group to "npm-patch" so the group name reflects that only patch updates are included—update the groups entry for npm-minor-patch and the update-types key accordingly.components/ui/carousel.tsx-109-121 (1)
109-121:⚠️ Potential issue | 🟡 MinorMissing cleanup for
reInitevent listener.The effect adds listeners for both
reInitandselectevents, but the cleanup only removes theselectlistener. This could cause a memory leak.🔧 Proposed fix
return () => { + api?.off("reInit", onSelect) api?.off("select", onSelect) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/carousel.tsx` around lines 109 - 121, The effect registers both "reInit" and "select" listeners on the carousel API but only removes "select" on cleanup; update the cleanup inside the React.useEffect that references api and onSelect to also call api?.off("reInit", onSelect) (i.e., remove both listeners) so both event handlers are detached when the effect tears down.docker-compose.yml-9-10 (1)
9-10:⚠️ Potential issue | 🟡 MinorReview the volume mount scope.
Mounting the entire current directory (
./) to/data/could inadvertently expose sensitive files (e.g.,.env,.git, credentials) to the container. Consider mounting only the specific directories needed.🛡️ Proposed fix to limit volume scope
volumes: - - ./:/data/ + - ./data:/data/Create a dedicated
datadirectory for FalkorDB persistence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 9 - 10, The docker-compose volume mounts the entire repo (./) into the container as /data/, risking exposure of sensitive files; change the volume to mount only a dedicated data directory (e.g., create a local "data" folder) or specific required subfolders instead of "./", and update the volumes entry (the current "- ./:/data/") to reference that dedicated directory (or explicit paths) so only FalkorDB persistence data is exposed to the container.e2e/tests/nodeDetailsPanel.spec.ts-44-50 (1)
44-50:⚠️ Potential issue | 🟡 MinorAssert
targetNodeexists before clicking by coordinates.In this flow,
targetNodeis used directly forscreenX/screenY; add a guard to avoid accidental runtime failures.Suggested fix
const graphData = await codeGraph.getGraphNodes(); const targetNode = findNodeByName(graphData, node.nodeName); + expect(targetNode).toBeDefined(); + if (!targetNode) throw new Error(`Node not found: ${node.nodeName}`); await codeGraph.nodeClick(targetNode.screenX, targetNode.screenY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/nodeDetailsPanel.spec.ts` around lines 44 - 50, The test uses targetNode (returned from findNodeByName) directly for coordinates which can be undefined; add an explicit guard/assert after obtaining graphData and targetNode (from getGraphNodes and findNodeByName) to fail fast if targetNode is missing (e.g., expect(targetNode).toBeDefined() or throw a clear Error) before calling codeGraph.nodeClick(targetNode.screenX, targetNode.screenY), ensuring the test doesn't crash with an unclear runtime exception.e2e/logic/utils.ts-16-35 (1)
16-35:⚠️ Potential issue | 🟡 Minor
waitForStableTextcan silently return unstable text on timeout.Line 34 returns the last sampled text even if stabilization never occurred. That weakens test determinism.
Suggested fix
export const waitForStableText = async (locator: Locator, timeout: number = 5000): Promise<string> => { @@ - return stableText; + throw new Error(`Text did not stabilize within ${timeout}ms. Last value: "${stableText}"`); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logic/utils.ts` around lines 16 - 35, The waitForStableText helper (function waitForStableText) currently returns the last sampled text on timeout which can hide flaky failures; change it so that if the loop finishes without observing a stable, non-empty value it throws an Error (include the last sampled text and the timeout value in the message for debugging) instead of returning stableText, and keep the existing pollingInterval/maxChecks logic and the locator usage (locator.textContent(), locator.page().waitForTimeout()) intact.e2e/tests/canvas.spec.ts-128-131 (1)
128-131:⚠️ Potential issue | 🟡 MinorGuard fallback arrays before reading
.length.If both shapes are missing,
nodes/linksbecomeundefinedand the length checks crash.Suggested fix
- const nodes = result.elements?.nodes || result.nodes; - const links = result.elements?.links || result.links; + const nodes = result.elements?.nodes ?? result.nodes ?? []; + const links = result.elements?.links ?? result.links ?? []; expect(nodes.length).toBeGreaterThan(1); expect(links.length).toBeGreaterThan(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/canvas.spec.ts` around lines 128 - 131, The test currently assumes nodes and links are arrays and reads .length directly, which will throw if both result.elements?.nodes and result.nodes are undefined; update the extraction to ensure a safe array fallback (e.g., use nullish coalescing or an explicit array check) so nodes and links are always arrays before asserting lengths—refer to the variables nodes and links and the expressions result.elements?.nodes || result.nodes (or replace with result.elements?.nodes ?? result.nodes ?? []) and similarly for links, then assert against the guaranteed array length.app/components/toolbar.tsx-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorUnusual type signature for
setCooldownTicks.The type
(ticks?: 0) => voidonly allowsundefinedor the literal0. Based on usage inapp/page.tsxandapp/components/graphView.tsx, the function is called with-1as well (e.g.,setCooldownTicks(-1)). This type should be widened.Proposed fix
- setCooldownTicks: (ticks?: 0) => void + setCooldownTicks: (ticks: number | undefined) => void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/toolbar.tsx` at line 10, The type for setCooldownTicks is too narrow — it currently permits only undefined or the literal 0; update its signature (in app/components/toolbar.tsx where setCooldownTicks is declared) to accept a number (e.g., ticks?: number or ticks: number | undefined) so callers like setCooldownTicks(-1) are allowed; ensure any related usages and prop types (where ToolbarProps or similar is defined) are updated accordingly to reflect the widened numeric type.app/page.tsx-338-346 (1)
338-346:⚠️ Potential issue | 🟡 MinorViewport check may exclude valid canvases.
The visibility check requires the canvas to be fully within the viewport (
rect.bottom <= window.innerHeight && rect.right <= window.innerWidth). If the canvas is larger than the viewport or partially scrolled, no canvas will match, causing a silent early return. Consider checking for intersection rather than full containment.Proposed fix for partial visibility
// Check if element is actually in viewport const rect = container.getBoundingClientRect(); - const isInViewport = rect.width > 0 && - rect.height > 0 && - rect.top >= 0 && - rect.left >= 0 && - rect.bottom <= window.innerHeight && - rect.right <= window.innerWidth; + const isInViewport = rect.width > 0 && + rect.height > 0 && + rect.top < window.innerHeight && + rect.left < window.innerWidth && + rect.bottom > 0 && + rect.right > 0; return isInViewport;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/page.tsx` around lines 338 - 346, The current isInViewport check in app/page.tsx requires full containment and will miss canvases that are larger than or partially in view; update the logic around the isInViewport calculation (the rect checks using rect.top/left/bottom/right and window.innerHeight/window.innerWidth) to test for intersection instead of full containment: keep rect.width > 0 && rect.height > 0, then require rect.bottom > 0 && rect.right > 0 && rect.top < window.innerHeight && rect.left < window.innerWidth so a canvas that is partially visible still counts as in-viewport.app/components/graphView.tsx-138-138 (1)
138-138:⚠️ Potential issue | 🟡 MinorDouble-click threshold of 1000ms is unusually long.
The typical double-click threshold is 300-500ms. A 1000ms threshold may cause unintended double-click detection from separate intentional clicks.
Proposed fix
- const isDoubleClick = now.getTime() - date.getTime() < 1000 && name === node.data.name + const isDoubleClick = now.getTime() - date.getTime() < 400 && name === node.data.name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/graphView.tsx` at line 138, The double-click detection uses a 1000ms window which is too long; update the comparison in the isDoubleClick computation to use a standard threshold (e.g., 300–500ms) and preferably extract it to a named constant for clarity. Replace the hardcoded 1000 in the isDoubleClick expression (where now, date, name and node.data.name are used) with a constant like DOUBLE_CLICK_THRESHOLD = 500 and compare now.getTime() - date.getTime() < DOUBLE_CLICK_THRESHOLD so the threshold is easy to adjust and consistent.app/components/toolbar.tsx-62-66 (1)
62-66:⚠️ Potential issue | 🟡 MinorGuard against undefined
handleDownloadImagebefore invoking.
handleDownloadImageis optional (handleDownloadImage?: () => void), but theonClickhandler invokes it without a check. Clicking this button when the prop is undefined will cause a runtime error.Proposed fix
<button className="hidden md:block control-button" title="downloadImage" - onClick={handleDownloadImage} + onClick={() => handleDownloadImage?.()} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/toolbar.tsx` around lines 62 - 66, The Download button calls the optional handler directly and can throw if undefined; update the onClick usage in the Toolbar component to guard against undefined handleDownloadImage (e.g., use a safe call or only attach the handler when present), and consider disabling the button when handleDownloadImage is not provided so the UI reflects absence of an action; modify the JSX around the button (the element using className "hidden md:block control-button" and title "downloadImage") to use handleDownloadImage?.() or conditionally set onClick/disabled accordingly.app/components/elementMenu.tsx-125-136 (1)
125-136:⚠️ Potential issue | 🟡 MinorRedundant
onClickhandler on anchor element.The
<a>tag already hashrefandtarget="_blank", so theonClickhandler that callswindow.openwith the same URL is redundant and will cause the link to open twice in some browsers.Proposed fix
<a className="p-2" href={objURL} target="_blank" + rel="noopener noreferrer" title="Go to repo" - onClick={() => { - window.open(objURL, '_blank'); - }} > <Globe color="white" /> </a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/elementMenu.tsx` around lines 125 - 136, Remove the redundant onClick handler on the anchor in elementMenu.tsx that calls window.open(objURL, '_blank'); the <a> already uses href={objURL} and target="_blank", so delete the onClick={() => { window.open(objURL, '_blank'); }} from the anchor (the element rendering the Globe icon) to prevent the link from opening twice in some browsers.app/components/code-graph.tsx-207-207 (1)
207-207:⚠️ Potential issue | 🟡 MinorUse block syntax for forEach callbacks to avoid implicit return values.
Lines 207 and 411 have arrow function callbacks with implicit returns (from
Set.add(...)and assignment), making the intent unclear and inconsistent with other forEach calls in the file.Proposed fixes
- deleteNeighbors(expandedNodes)?.forEach(id => deleteIdsMap.add(id)) + deleteNeighbors(expandedNodes)?.forEach(id => { + deleteIdsMap.add(id) + })- graph.Categories.forEach(c => c.show = true); + graph.Categories.forEach(c => { + c.show = true + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/code-graph.tsx` at line 207, The forEach callbacks use concise arrow expressions that rely on implicit returns (e.g., deleteNeighbors(expandedNodes)?.forEach(id => deleteIdsMap.add(id))), which is inconsistent and unclear; update these to use block-bodied arrow functions with explicit statements (e.g., id => { deleteIdsMap.add(id); }) so intent is clear and matches other forEach usages — locate the occurrences around deleteNeighbors and the similar callback at line ~411 and replace the concise arrow bodies with block bodies containing explicit calls or assignments.
🧹 Nitpick comments (9)
e2e/config/constants.ts (1)
3-3: Fix the exported constant typo before it spreads (CHAT_OPTTIONS_COUNT).
Line 3 exports a misspelled public identifier. Consider adding a correctly spelled alias and keeping the old name temporarily for compatibility.Proposed non-breaking cleanup
-export const CHAT_OPTTIONS_COUNT = 6; +export const CHAT_OPTIONS_COUNT = 6; +export const CHAT_OPTTIONS_COUNT = CHAT_OPTIONS_COUNT; // backward compatibility alias🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/config/constants.ts` at line 3, Export a correctly spelled constant name and keep the misspelled one as a backward-compatible alias: add a new exported symbol CHAT_OPTIONS_COUNT with the same value as the existing CHAT_OPTTIONS_COUNT and re-export the old CHAT_OPTTIONS_COUNT (or assign it from the new constant) so consumers using the misspelled identifier keep working while code is migrated to CHAT_OPTIONS_COUNT.lib/utils.ts (1)
7-10: Avoidany[]in shared graph/message contracts.
Lines 8-9 and Line 34 remove most type safety from the new centralized model. Use concrete types and reusePathDatainMessageto prevent drift.Proposed typing cleanup
export type PathData = { - nodes: any[] - links: any[] + nodes: Node[] + links: Link[] } @@ export interface Message { type: MessageTypes; text?: string; - paths?: { nodes: any[], links: any[] }[]; + paths?: PathData[]; graphName?: string; }Also applies to: 31-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils.ts` around lines 7 - 10, The PathData type uses any[] for nodes and links and the Message type (referenced in this file) duplicates loose typing; replace any[] with concrete, shared types (e.g., define Node and Link interfaces or reuse existing ones) and update PathData to nodes: Node[] and links: Link[]; then change Message to reference PathData instead of duplicating fields so both the graph contract and Message reuse the same strongly typed structure (update all occurrences of PathData, Message, Node, and Link in this module).components/ui/carousel.tsx (2)
88-99: Keyboard navigation doesn't account for vertical orientation.The
handleKeyDownusesArrowLeft/ArrowRightregardless of orientation. For vertical carousels,ArrowUp/ArrowDownwould be more intuitive.♻️ Proposed enhancement
const handleKeyDown = React.useCallback( (event: React.KeyboardEvent<HTMLDivElement>) => { - if (event.key === "ArrowLeft") { + const prevKey = orientation === "horizontal" ? "ArrowLeft" : "ArrowUp" + const nextKey = orientation === "horizontal" ? "ArrowRight" : "ArrowDown" + if (event.key === prevKey) { event.preventDefault() scrollPrev() - } else if (event.key === "ArrowRight") { + } else if (event.key === nextKey) { event.preventDefault() scrollNext() } }, - [scrollPrev, scrollNext] + [orientation, scrollPrev, scrollNext] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/carousel.tsx` around lines 88 - 99, handleKeyDown always maps ArrowLeft/ArrowRight to scrollPrev/scrollNext, which ignores vertical carousels; update handleKeyDown to branch on the carousel orientation (e.g., an orientation prop or isVertical flag) and use ArrowUp/ArrowDown for vertical mode while keeping ArrowLeft/ArrowRight for horizontal mode, still calling scrollPrev and scrollNext and calling event.preventDefault() for handled keys; locate the callback by name handleKeyDown and update its dependency array if you reference any new orientation variable.
208-214: Minor: Remove extra space in className.There's a double space between
absoluteandh-8.🔧 Proposed fix
- "absolute h-8 w-8 rounded-full", + "absolute h-8 w-8 rounded-full",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/carousel.tsx` around lines 208 - 214, The className string passed to the cn() call in the Carousel component has an extra space between "absolute" and "h-8"; update the class string inside the className prop (the cn(...) expression in components/ui/carousel.tsx where the element builds "absolute h-8 w-8 rounded-full") to remove the double space so it becomes "absolute h-8 w-8 rounded-full".README.md (1)
3-3: Consider using h2 for consistent heading levels.The heading jumps from h1 (
#) directly to h3 (###). For proper document structure, consider using h2 (##) instead.📝 Proposed fix
-### Visualize your repository with our graph for code analysis +## Visualize your repository with our graph for code analysis🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 3, The heading "Visualize your repository with our graph for code analysis" is an h3 while the document starts with h1; change this header from "### Visualize your repository with our graph for code analysis" to an h2 by replacing the three hashes with two ("##") so heading levels are consistent and follow the h1 -> h2 structure in README.md.e2e/logic/POM/codeGraph.ts (1)
403-403: Magic number for animation delay.The 2000ms timeout is a hardcoded magic number. Consider extracting it to a named constant or making it configurable for better maintainability.
Proposed fix
+const GRAPH_ANIMATION_DELAY = 2000; + // In selectGraph method: - await this.page.waitForTimeout(2000); // graph animation delay + await this.page.waitForTimeout(GRAPH_ANIMATION_DELAY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logic/POM/codeGraph.ts` at line 403, The hardcoded 2000ms in the call to this.page.waitForTimeout(2000) is a magic number; extract it into a named constant (e.g., ANIMATION_DELAY_MS) or make it configurable (pass as a parameter or read from a config) and replace the literal in the method in codeGraph.ts so the delay is maintainable and easy to adjust; update any related tests or usages to reference the new constant/config option.app/components/ForceGraph.tsx (2)
154-156: Remove unusedcanvasReffromhandleEngineStopdependencies.
canvasRefis listed in the dependency array but is not used in the callback body. This can cause unnecessary re-creations of the callback.Proposed fix
const handleEngineStop = useCallback(() => { onEngineStop() - }, [canvasRef, onEngineStop]) + }, [onEngineStop])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ForceGraph.tsx` around lines 154 - 156, The handleEngineStop useCallback includes an unused dependency canvasRef which causes needless re-creations; update the dependency array for handleEngineStop to only include onEngineStop (i.e., useCallback(() => { onEngineStop() }, [onEngineStop])) so the callback only re-creates when onEngineStop changes and remove canvasRef from the dependency list.
106-151: Consider memoizing node/link lookups for better performance.The event handlers use
data.nodes.find()anddata.links.find()which are O(n) operations. For large graphs, this could impact performance during rapid interactions like hovering. Consider using a Map for O(1) lookups.Proposed optimization
+// Add memoized maps +const nodesMap = useMemo(() => new Map(data.nodes.map(n => [n.id, n])), [data.nodes]) +const linksMap = useMemo(() => new Map(data.links.map(l => [l.id, l])), [data.links]) // Map node click handler const handleNodeClick = useCallback((node: GraphNode, event: MouseEvent) => { - const originalNode = data.nodes.find(n => n.id === node.id) + const originalNode = nodesMap.get(node.id) if (originalNode) onNodeClick(originalNode, event) -}, [onNodeClick, data.nodes]) +}, [onNodeClick, nodesMap])Apply the same pattern to all other handlers (
handleNodeHover,handleNodeRightClick,handleLinkClick,handleLinkHover,handleLinkRightClick).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ForceGraph.tsx` around lines 106 - 151, Replace repeated O(n) searches in handlers by building memoized lookup maps for nodes and links (e.g., nodeById and linkById) using useMemo derived from data.nodes and data.links, then change handleNodeClick, handleNodeHover, handleNodeRightClick, handleLinkClick, handleLinkHover, and handleLinkRightClick to use nodeById.get(node.id) / linkById.get(link.id) for O(1) lookups; update each handler's dependency array to depend on the corresponding memoized map (not the raw arrays) and preserve the current null-check behavior in handleNodeHover/handleLinkHover.app/components/elementMenu.tsx (1)
145-161: Redundant type cast after type guard.After the
"category" in objcheck on line 146, TypeScript narrowsobjtoNode. The explicit castobj as Nodeon lines 150 and 156 is redundant and can be removed for cleaner code.Proposed refactor
{ "category" in obj && <> <button className="p-2" - onClick={() => handleExpand([obj as Node], true)} + onClick={() => handleExpand([obj], true)} > <Maximize2 color="white" /> </button> <button className="p-2" - onClick={() => handleExpand([obj as Node], false)} + onClick={() => handleExpand([obj], false)} > <Minimize2 color="white" /> </button> </> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/elementMenu.tsx` around lines 145 - 161, The "category" in obj type guard already narrows obj to Node, so remove the redundant casts obj as Node in the two onClick handlers and pass obj directly to handleExpand; update both calls inside the fragment (the Maximize2 and Minimize2 buttons) to use handleExpand([obj], true/false) so the code is cleaner and leverages TypeScript's narrowing (symbols: obj, Node, handleExpand).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/code-graph-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (39)
.github/dependabot.yml.github/workflows/nextjs.yml.github/workflows/playwright.ymlDockerfileREADME.mdapp/api/repo/route.tsapp/components/ForceGraph.tsxapp/components/Input.tsxapp/components/chat.tsxapp/components/code-graph.tsxapp/components/combobox.tsxapp/components/dataPanel.tsxapp/components/elementMenu.tsxapp/components/graphView.tsxapp/components/model.tsapp/components/toolbar.tsxapp/globals.cssapp/layout.tsxapp/page.tsxcomponents.jsoncomponents/ui/carousel.tsxcomponents/ui/drawer.tsxcomponents/ui/switch.tsxdocker-compose.ymle2e/config/constants.tse2e/config/testData.tse2e/infra/ui/basePage.tse2e/infra/ui/browserWrapper.tse2e/logic/POM/codeGraph.tse2e/logic/utils.tse2e/tests/canvas.spec.tse2e/tests/chat.spec.tse2e/tests/navBar.spec.tse2e/tests/nodeDetailsPanel.spec.tse2e/tests/searchBar.spec.tslib/utils.tspackage.jsonplaywright.config.tstailwind.config.js
💤 Files with no reviewable changes (1)
- app/layout.tsx
Contributor
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/components/Input.tsx (1)
46-57:⚠️ Potential issue | 🟠 MajorHandle network exceptions in the debounced fetch path.
If
fetchthrows (network/CORS/abort), this block can produce unhandled rejections and leave stale suggestions visible.Proposed fix
- const result = await fetch(`/api/repo/${prepareArg(graph.Id)}/?prefix=${prepareArg(node?.name)}`, { - method: 'POST' - }) - - if (!result.ok) { - toast({ - variant: "destructive", - title: "Uh oh! Something went wrong.", - description: "Please try again later.", - }) - return - } + let result: Response + try { + result = await fetch(`/api/repo/${prepareArg(graph.Id)}/?prefix=${prepareArg(node?.name)}`, { + method: 'POST' + }) + } catch { + setOptions([]) + setOpen(false) + toast({ + variant: "destructive", + title: "Uh oh! Something went wrong.", + description: "Please try again later.", + }) + return + } + + if (!result.ok) { + setOptions([]) + setOpen(false) + toast({ + variant: "destructive", + title: "Uh oh! Something went wrong.", + description: "Please try again later.", + }) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Input.tsx` around lines 46 - 57, The debounced fetch call using fetch(`/api/repo/${prepareArg(graph.Id)}/?prefix=${prepareArg(node?.name)}`) can throw network/CORS/abort exceptions and currently has no try/catch; wrap the await fetch in a try/catch inside the debounced path (the function where this fetch lives in Input.tsx), and on catch call the same toast error UI, clear or hide suggestions (e.g., call the local setter used to store suggestions like setSuggestions([]) or setIsSuggestionsVisible(false)), and ensure any loading/abort state (e.g., setIsLoading) is reset so stale suggestions aren’t left visible.app/components/chat.tsx (1)
211-221:⚠️ Potential issue | 🟠 MajorHandle thrown fetch errors in both chat request flows.
Both
sendQueryandhandleSubmitonly handle!result.ok. Iffetchthrows (network/offline), pending state/messages are not reconciled.Also applies to: 249-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/chat.tsx` around lines 211 - 221, Both chat flows (sendQuery and handleSubmit) only check result.ok and don't handle fetch throwing; wrap the fetch calls in try/catch in both functions (sendQuery and handleSubmit), and in the catch block reconcile UI by removing the pending user message via setMessages (same logic used for !result.ok), append a MessageTypes.Response error message like "Sorry but I couldn't answer your question, please try rephrasing.", and clear any loading/pending state flags so the UI doesn't stay stuck; ensure the catch mirrors the existing error-path behavior at the fetch success check so both network errors and non-OK responses behave identically.app/page.tsx (1)
102-128:⚠️ Potential issue | 🟠 MajorFix
isSubmitstate handling in repo creation flow.Line 105 sets loading before validation, and Line 113 can return without resetting it. Also, fetch exceptions are not caught, so loading can remain stuck.
🐛 Suggested fix
async function onCreateRepo(e: React.FormEvent<HTMLFormElement>) { e.preventDefault() - setIsSubmit(true) - if (!createURL) { toast({ variant: "destructive", title: "Uh oh! Something went wrong.", description: "Please enter a URL.", }) + setIsSubmit(false) return } - const result = await fetch(`/api/repo/?url=${prepareArg(createURL)}`, { - method: 'POST', - }) + setIsSubmit(true) + try { + const result = await fetch(`/api/repo/?url=${prepareArg(createURL)}`, { + method: 'POST', + }) - if (!result.ok) { - toast({ - variant: "destructive", - title: "Uh oh! Something went wrong.", - description: await result.text(), - }) - setIsSubmit(false) - return - } + if (!result.ok) { + toast({ + variant: "destructive", + title: "Uh oh! Something went wrong.", + description: await result.text(), + }) + return + } - const graphName = createURL.split('/').pop()! + const graphName = createURL.split('/').pop()! - setOptions(prev => [...prev, graphName]) - setSelectedValue(graphName) - setCreateURL("") - setCreateOpen(false) - setIsSubmit(false) + setOptions(prev => [...prev, graphName]) + setSelectedValue(graphName) + setCreateURL("") + setCreateOpen(false) - toast({ - title: "Success", - description: `Project ${graphName} created successfully`, - }) + toast({ + title: "Success", + description: `Project ${graphName} created successfully`, + }) + } catch { + toast({ + variant: "destructive", + title: "Uh oh! Something went wrong.", + description: "Failed to create project. Please try again.", + }) + } finally { + setIsSubmit(false) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/page.tsx` around lines 102 - 128, The onCreateRepo handler sets setIsSubmit(true) before validation and can return early without resetting it and also doesn't catch fetch errors; move the loading toggling so validation happens before setting setIsSubmit(true) OR ensure every early return resets it; wrap the fetch call in a try/catch/finally around the await fetch so any exception triggers setIsSubmit(false) in the catch or finally, and keep the existing non-ok branch calling setIsSubmit(false); update onCreateRepo, setIsSubmit usage, and the fetch block (referencing onCreateRepo, setIsSubmit, prepareArg and the fetch result handling) so loading state is always cleared on all paths.
♻️ Duplicate comments (18)
.github/workflows/playwright.yml (1)
39-39:⚠️ Potential issue | 🟠 MajorFix startup race and remove CLI reporter override at Line 39.
The job still starts the app and runs Playwright immediately in one command, which is race-prone. Also,
--reporter=dot,listoverrides reporter config and can suppress expected HTML outputs.Suggested fix
- NEXTAUTH_SECRET=SECRET npm start & npx playwright test --shard=${{ matrix.shard }}/2 --reporter=dot,list + NEXTAUTH_SECRET=SECRET npm start & + for i in {1..60}; do + curl -fsS http://127.0.0.1:3000 >/dev/null && break + sleep 1 + done + npx playwright test --shard=${{ matrix.shard }}/2#!/bin/bash set -euo pipefail echo "Workflow segment:" cat -n .github/workflows/playwright.yml | sed -n '34,45p' echo echo "Reporter config in Playwright config files:" fd -t f 'playwright.config.*' -x cat -n {} | sed -n '1,220p' | rg -n 'reporter|outputFolder|html'Based on learnings: In Playwright, CLI --reporter flags completely override the reporter configuration in playwright.config.ts, even when the config file has conditional reporter settings for CI environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/playwright.yml at line 39, The workflow currently starts the app and runs Playwright in one command which causes a race and also forces CLI reporter overrides; change the job to (1) start the server in the background and wait for it to be healthy (use wait-on or start-server-and-test), e.g., run the server (the current NEXTAUTH_SECRET=... npm start) in its own step or use start-server-and-test to wait for the ready URL, and (2) remove the CLI reporter override (--reporter=dot,list) from the Playwright invocation so the reporters configured in playwright.config.* are respected; ensure the Playwright step runs something like `npx playwright test --shard=${{ matrix.shard }}/2` after the readiness wait.README.md (2)
109-109:⚠️ Potential issue | 🟡 MinorPoint GitHub Issues to this repository.
Line 109 still links to
FalkorDB/GraphRAG-SDK/issuesinstead ofFalkorDB/code-graph/issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 109, Update the GitHub Issues link in the README: find the list entry that reads "* [GitHub Issues](https://github.com/FalkorDB/GraphRAG-SDK/issues)" and change the URL to "https://github.com/FalkorDB/code-graph/issues" so it points to the code-graph repository instead of GraphRAG-SDK.
9-9:⚠️ Potential issue | 🟡 MinorRemove stray hyphen line.
Line 9 contains an isolated
-and should be deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 9, Delete the stray isolated hyphen line (a single "-" character) present in the README content (the lone line containing just "-") so the file no longer contains that extraneous line; simply remove that line from the README to fix the duplicate/stray hyphen.app/api/repo/route.ts (3)
43-51:⚠️ Potential issue | 🟠 MajorAdd a timeout to backend
fetchin POST.Lines 43-51 can hang indefinitely if the backend stalls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/repo/route.ts` around lines 43 - 51, The POST fetch call that assigns to result (calling `${url}/${isLocal ? "analyze_folder" : "analyze_repo"}`) can hang indefinitely; wrap this fetch in an AbortController with a timeout (e.g., 30s) and pass the controller.signal into the fetch options, and ensure you clear the timeout after the request completes and call controller.abort() on timeout so the request is canceled and errors are handled upstream (update the code around the existing fetch usage and error handling for result accordingly).
53-60:⚠️ Potential issue | 🟠 MajorDo not return raw upstream error text to clients.
Lines 53-60 still expose backend error bodies directly in API responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/repo/route.ts` around lines 53 - 60, The catch block and the earlier throw use raw upstream response bodies (await result.text() and returning err.message) which exposes internal error details to clients; change the code so you log the full upstream error/response body server-side (console.error or processLogger) and throw or return a sanitized client-facing message instead (e.g., "Upstream service error" or a generic failure string) via NextResponse.json; specifically update the throw in the block that calls result.text() and the catch return that currently uses err.message/String(err) so the client only receives the generic message while full error details remain in server logs for debugging.
41-43:⚠️ Potential issue | 🔴 CriticalBlock untrusted
file://input from selectinganalyze_folder.Line 41 still allows arbitrary clients to route requests to local-folder analysis.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/repo/route.ts` around lines 41 - 43, Currently the client-controlled check using repo_url and isLocal lets callers pick "analyze_folder" by sending file:// URLs; instead enforce the decision server-side by rejecting untrusted file:// inputs: update the handler that reads repo_url and the isLocal logic so that if repo_url.startsWith("file://") you either return a 400 error (deny) or only allow it when a server-only safeguard is present (e.g., process.env.ALLOW_LOCAL_ANALYSIS === "true" AND the request is authenticated/has an admin token); then use the sanitized isLocal value to select between "analyze_folder" and "analyze_repo" in the fetch call. Ensure you reference and modify the repo_url/isLocal check and the fetch call that builds `${url}/${isLocal ? "analyze_folder" : "analyze_repo"}` so clients cannot force local-folder analysis.docker-compose.yml (1)
33-34:⚠️ Potential issue | 🔴 CriticalDo not commit secrets in Compose environment values.
Line 34 still hardcodes
SECRET_TOKEN; this is a credential leak risk.Proposed fix
- - OPENAI_API_KEY=YOUR_OPENAI_API_KEY - - SECRET_TOKEN=sC0tTerMania + - OPENAI_API_KEY=${OPENAI_API_KEY} + - SECRET_TOKEN=${SECRET_TOKEN}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 33 - 34, Remove the hardcoded SECRET_TOKEN environment value from the Compose environment section and replace it with a reference to an external environment variable (e.g., use variable substitution ${SECRET_TOKEN} or an env_file), ensure the runtime reads SECRET_TOKEN from the environment rather than committing it; update any README or deployment docs to instruct maintainers to place the secret in a local .env or CI secret store and add that local .env to .gitignore so SECRET_TOKEN (and OPENAI_API_KEY) are never committed in docker-compose.yml.package.json (1)
33-33:⚠️ Potential issue | 🟠 MajorUpgrade
nextfrom^15.5.8to a patched 15.x release.Line 33 still uses the same version previously flagged for advisories.
Is next@15.5.8 affected by known security advisories, and what is the first patched version in the 15.x line?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 33, The package.json still pins the "next" dependency at "^15.5.8" which is flagged by advisories; update the "next" entry in package.json to the first patched 15.x release (replace the version string for the "next" dependency) and then run your package manager (npm/yarn/pnpm) to reinstall and re-run audit tests; ensure the change targets the 15.x patched version (i.e., set "next" to the patched 15.x release) and verify by running npm/yarn audit or checking the advisory that the new version resolves the issues.e2e/tests/chat.spec.ts (2)
123-124:⚠️ Potential issue | 🟠 MajorQuestion-option index is off by one.
This currently selects options
2..6instead of1..5and can overflow available options.🔧 Proposed fix
- for (let index = 1; index < 6; index++) { - const questionNumber = index + 1; + for (let index = 1; index <= CHAT_OPTTIONS_COUNT; index++) { + const questionNumber = index;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/chat.spec.ts` around lines 123 - 124, The for-loop selecting question options is off-by-one: it iterates for (let index = 1; index < 6; index++) and computes questionNumber = index + 1, causing selection of options 2..6 instead of 1..5 and risking overflow; change the iteration so the selected option range is 1..5 by either starting index at 0 and using questionNumber = index + 1, or start index at 1 and set questionNumber = index (update the loop bounds accordingly), locating the change around the for loop and the questionNumber constant in chat.spec.ts.
69-70:⚠️ Potential issue | 🟡 MinorRemove unsafe
?.[0]!extraction in response parsing.Both places can fail with unclear errors when regex does not match.
🔧 Proposed fix
- const number = result.match(/\d+/g)?.[0]!; - responses.push(number); + const match = result.match(/\d+/g); + if (!match?.[0]) throw new Error("UI response does not contain a number"); + responses.push(match[0]); @@ - const number = uiResponse.match(/\d+/g)?.[0]!; - - expect(number).toEqual(apiResponse.result.response.match(/\d+/g)?.[0]); + const uiMatch = uiResponse.match(/\d+/g); + const apiMatch = apiResponse.result.response.match(/\d+/g); + if (!uiMatch?.[0] || !apiMatch?.[0]) { + throw new Error("Unable to extract numeric token from UI/API response"); + } + expect(uiMatch[0]).toEqual(apiMatch[0]);#!/bin/bash # Verify unsafe optional-chain non-null assertions in this spec. rg -nP '\?\.\[0\]!' e2e/tests/chat.spec.tsAlso applies to: 85-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/chat.spec.ts` around lines 69 - 70, The extraction using result.match(/\d+/g)?.[0]! is unsafe — replace it by explicitly calling const matches = result.match(/\d+/g) and checking matches && matches.length > 0 before using matches[0]; if no match is found throw or fail the test with a clear error message including the original result string. Apply the same change to both occurrences referenced (the one assigning number from result and the similar code at lines ~85-87) so responses.push uses a validated matches[0] rather than an optional-chain non-null assertion.app/components/dataPanel.tsx (3)
94-112:⚠️ Potential issue | 🟠 MajorFix
valueRenderersignature/keyPath usage forreact-json-tree.
keyPathshould be consumed from rest parameters and checked viakeyPath[0], not compared as a scalar string.🔧 Proposed fix
- valueRenderer={(valueAsString, value, keyPath) => { - if (keyPath === "src") { + valueRenderer={(valueAsString, value, ...keyPath) => { + if (keyPath[0] === "src") { return <SyntaxHighlighter language="python" style={{ @@ - return <span className="text-white">{value as string}</span> + return <span className="text-white">{String(valueAsString)}</span> }}In react-json-tree, what is the exact function signature of valueRenderer, and is keyPath passed as rest parameters (...keyPath)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/dataPanel.tsx` around lines 94 - 112, The valueRenderer passed to react-json-tree is using keyPath as a scalar but react-json-tree supplies it via rest parameters; update the valueRenderer signature to accept the rest params (e.g., valueRenderer={(valueAsString, value, ...keyPath) => { ... }}) and then check keyPath[0] === "src" when deciding to render SyntaxHighlighter (references: valueRenderer, keyPath, SyntaxHighlighter); ensure other branches still return the plain span when keyPath[0] is not "src".
132-146:⚠️ Potential issue | 🟠 MajorUse a single navigation path for “Go to repo” and add
relsecurity.Current anchor can open two tabs (
href+window.open) and relies onnewTab.scrollagainst a likely cross-origin page.🔧 Proposed fix
<a className="flex items-center gap-2 p-2" - href={url} + href={obj.data.src_start ? `${url}#L${obj.data.src_start}` : url} target="_blank" + rel="noopener noreferrer" title="Go to repo" - onClick={() => { - const newTab = window.open(url, '_blank'); - - if (!obj.data.src_start || !obj.data.src_end || !newTab) return - - newTab.scroll({ - top: obj.data.src_start, - behavior: 'smooth' - }) - }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/dataPanel.tsx` around lines 132 - 146, The anchor currently opens two navigation paths (href + window.open) and calls newTab.scroll which will fail cross-origin; remove the window.open call and any newTab.scroll usage inside the onClick so the link uses a single navigation path via href + target="_blank", and add rel="noopener noreferrer" to the <a> to fix the security issue; if scrolling is required only for same-origin repos, replace the unconditional window.open/newTab.scroll logic with a safe same-origin check (or a separate handler) that uses window.open and scroll only when location.origin === new URL(url).origin, otherwise fall back to the simple href behavior — look for the anchor with target="_blank", its onClick handler, and references to obj.data.src_start / obj.data.src_end / newTab.scroll to modify.
73-76:⚠️ Potential issue | 🟠 MajorAdd a null guard before rendering nested object JSON.
typeof null === "object", soObject.entries(value)can throw here.🔧 Proposed fix
- : typeof value === "object" ? + : value !== null && typeof value === "object" ? <JSONTree🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/dataPanel.tsx` around lines 73 - 76, The rendering condition that checks typeof value === "object" can crash because typeof null === "object"; update the conditional used before rendering JSONTree in dataPanel.tsx to explicitly guard against null (e.g., check value !== null && typeof value === "object") so Object.entries(value) is only called for non-null objects; keep the same filtering using excludedProperties and ensure the JSONTree data prop only receives the safe Object.fromEntries(Object.entries(value)....e2e/tests/nodeDetailsPanel.spec.ts (1)
76-80:⚠️ Potential issue | 🟠 MajorGuard empty graph state before deriving
nodeName.This block still assumes
graphDatahas at least one node and can dereferenceundefined.🔧 Proposed fix
const graphData = await codeGraph.getGraphNodes(); +expect(graphData.length).toBeGreaterThan(0); const targetNode = graphData.find(node => node.src) || graphData[0]; -const nodeName = targetNode.name || targetNode.data?.name; +expect(targetNode).toBeDefined(); +const nodeName = targetNode?.name || targetNode?.data?.name; +if (!nodeName) throw new Error("Unable to resolve node name for search"); await codeGraph.fillSearchBar(nodeName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/nodeDetailsPanel.spec.ts` around lines 76 - 80, The test assumes graphData has nodes and dereferences targetNode without guarding; update the logic around codeGraph.getGraphNodes() to handle an empty or undefined graph (graphData) and a missing targetNode before deriving nodeName and calling codeGraph.fillSearchBar/selectSearchBarOptionBtn; for example, check if graphData is falsy or length===0 and early-return or fail the test with a clear message, and also ensure targetNode is defined before using targetNode.name or targetNode.data?.name so fillSearchBar is only called with a valid string.e2e/logic/POM/codeGraph.ts (2)
151-153:⚠️ Potential issue | 🟠 MajorFix malformed XPath interpolation in
locateNodeInLastChatPath.
contains(text(), ${node})injects an unquoted value and produces invalid XPath for normal string node names.🔧 Proposed fix
private get locateNodeInLastChatPath(): (node: string) => Locator { - return (node: string) => this.page.locator(`(//main[`@data-name`='main-chat']//button//span[contains(text(), ${node})])[last()]`); + return (node: string) => + this.page.locator(`(//main[`@data-name`='main-chat']//button//span[contains(text(), ${JSON.stringify(node)})])[last()]`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logic/POM/codeGraph.ts` around lines 151 - 153, The XPath built in locateNodeInLastChatPath injects an unquoted node string causing invalid XPath; change the interpolation to produce a properly quoted and escaped XPath string (e.g. use JSON.stringify(node) when building the template literal) so the locator becomes something like this.page.locator(`(//main[`@data-name`='main-chat']//button//span[contains(text(), ${JSON.stringify(node)})])[last()]`), ensuring node values with quotes are escaped; update the locateNodeInLastChatPath function accordingly.
591-605:⚠️ Potential issue | 🟠 MajorUse mobile-aware graph accessor in
waitForGraphData.The method still hardcodes
graphDesktop, so mobile-mode tests can read the wrong graph instance.🔧 Proposed fix
private async waitForGraphData(): Promise<any> { + const graphAccessor = this.isMobile ? "graphMobile" : "graphDesktop"; await this.waitForCanvasAnimationToEnd(); - await this.page.waitForFunction(() => { - const data = (window as any).graphDesktop?.(); + await this.page.waitForFunction((accessor) => { + const data = (window as any)[accessor]?.(); return data && ((Array.isArray(data.nodes) && data.nodes.length > 0) || (data.elements && Array.isArray(data.elements.nodes) && data.elements.nodes.length > 0)); - }, { timeout: 5000 }); + }, graphAccessor, { timeout: 5000 }); await this.page.waitForTimeout(3000); await this.waitForCanvasAnimationToEnd(); - return await this.page.evaluate(() => (window as any).graphDesktop()); + return await this.page.evaluate((accessor) => (window as any)[accessor]?.(), graphAccessor); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logic/POM/codeGraph.ts` around lines 591 - 605, The waitForGraphData method currently hardcodes (window as any).graphDesktop which breaks mobile-mode tests; update waitForGraphData to use a mobile-aware accessor: inside the page.waitForFunction check and the final page.evaluate call, resolve the graph instance by preferring (window as any).graphMobile if present/active (or any runtime flag that indicates mobile) and fall back to (window as any).graphDesktop, and use that result for node/elements existence checks and the returned value; ensure both the wait condition and the returned evaluation use the same mobile-aware accessor logic so mobile tests read the correct graph instance.e2e/tests/canvas.spec.ts (1)
222-225:⚠️ Potential issue | 🟡 MinorRemove non-null assertions in path/API validation.
This still relies on
firstNodeRes!.id/secondNodeRes!.id, which can crash before giving a clear test diagnosis.🔧 Proposed fix
expect(firstNodeRes).toBeDefined(); expect(secondNodeRes).toBeDefined(); +if (!firstNodeRes || !secondNodeRes) throw new Error("Path nodes were not resolved from graph data"); const api = new ApiCalls(); -const response = await api.showPath(GRAPHRAG_SDK ,firstNodeRes!.id, secondNodeRes!.id); +const response = await api.showPath(GRAPHRAG_SDK, firstNodeRes.id, secondNodeRes.id); const callsRelationObject = response.result.paths[0].find(item => item.relation === "CALLS") -expect(callsRelationObject?.src_node).toBe(firstNodeRes!.id); -expect(callsRelationObject?.dest_node).toBe(secondNodeRes!.id); +expect(callsRelationObject?.src_node).toBe(firstNodeRes.id); +expect(callsRelationObject?.dest_node).toBe(secondNodeRes.id);#!/bin/bash # Verify non-null assertion usage in canvas spec. rg -nP '!\.id' e2e/tests/canvas.spec.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/canvas.spec.ts` around lines 222 - 225, The test is using non-null assertions (firstNodeRes!.id / secondNodeRes!.id) which can throw before the test reports a clear failure; update the api.showPath call and subsequent expectations to explicitly assert presence of the node responses and their ids first (e.g., add expect(firstNodeRes).toBeDefined() and expect(firstNodeRes?.id).toBeDefined() and same for secondNodeRes) and then use the extracted ids (firstId and secondId) for api.showPath and assertions against callsRelationObject; also guard that response.result.paths and response.result.paths[0] exist before calling find on them and assert that callsRelationObject is defined before checking its src_node/dest_node, referencing api.showPath, GRAPHRAG_SDK, firstNodeRes, secondNodeRes, and callsRelationObject.app/components/code-graph.tsx (1)
175-178:⚠️ Potential issue | 🟠 MajorAwait
onFetchGraphand handle failures in selection flow.Line 177 starts an async operation without awaiting/catch, so rejections can escape and leave state partially updated.
🐛 Suggested fix
async function handleSelectedValue(value: string) { setGraphName(value) - onFetchGraph(value) + try { + await onFetchGraph(value) + } catch (error) { + toast({ + variant: "destructive", + title: "Uh oh! Something went wrong.", + description: "Failed to fetch graph.", + }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/code-graph.tsx` around lines 175 - 178, handleSelectedValue updates UI state then calls onFetchGraph without awaiting or error handling, allowing rejections to leave state inconsistent; change handleSelectedValue to await onFetchGraph(value) inside a try/catch, and either setGraphName only after successful fetch or roll back the previous graph name in the catch and surface the error (e.g., set an error state or call an onError handler). Specifically modify the async function handleSelectedValue to: await onFetchGraph(value) in a try block, update setGraphName after success (or revert on failure), and handle/log the caught error so promise rejections do not escape.
🧹 Nitpick comments (10)
app/components/combobox.tsx (1)
54-54: Prefer theme token classes over hardcoded gray borders.At Line 54,
border-gray-400 md:border-gray-100hardcodes palette values and can become inconsistent with theme/dark-mode styling. Using semantic border tokens from your UI system will be more maintainable.♻️ Proposed refactor
- <SelectTrigger className="z-10 md:z-0 rounded-md border border-gray-400 md:border-gray-100 focus:ring-0 focus:ring-offset-0"> + <SelectTrigger className="z-10 md:z-0 rounded-md border border-input focus:ring-0 focus:ring-offset-0">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/combobox.tsx` at line 54, Replace the hardcoded color utility classes on the SelectTrigger element (currently "border-gray-400 md:border-gray-100") with your design-system semantic border token classes so borders respond to theme/dark-mode changes; locate the SelectTrigger JSX in app/components/combobox.tsx and swap those classes for the appropriate semantic tokens used across the codebase (e.g., border-default, border-muted, border-surface or whatever token names your tailwind/design system exposes) ensuring you preserve existing responsive breakpoint usage (md:...) and other classes like rounded-md and focus:ring-0.e2e/infra/ui/basePage.ts (1)
1-1: Use a type-only import forPage.
Pageis only used for typing in property and constructor parameter annotations, so this should be atypeimport.Suggested diff
-import { Page } from '@playwright/test'; +import { type Page } from '@playwright/test';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/infra/ui/basePage.ts` at line 1, The import of Page should be a type-only import because it's used only for typing in BasePage; change the import statement to a type import (e.g., use "import type { Page } from '@playwright/test'") and keep the rest of the class/constructor/property annotations (references to Page inside the BasePage constructor and as the page property) unchanged so the compiler treats it as erased-only type usage.e2e/infra/ui/browserWrapper.ts (1)
1-1: Prefer type-only imports for Playwright types.
Browser,BrowserContext, andPageare used exclusively in type annotations. Converting them to type-only imports enables better tree-shaking and clarifies intent.Suggested diff
-import { chromium, Browser, BrowserContext, Page } from '@playwright/test'; +import { chromium, type Browser, type BrowserContext, type Page } from '@playwright/test';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/infra/ui/browserWrapper.ts` at line 1, Import the Playwright runtime separately from the types: change the combined import so that chromium remains a normal import but Browser, BrowserContext, and Page are imported as type-only imports (e.g., use "import { chromium } from '@playwright/test';" and "import type { Browser, BrowserContext, Page } from '@playwright/test';") so those symbols are treated solely as types and enable better tree-shaking and intent clarity in browserWrapper.ts.components/ui/carousel.tsx (1)
88-99: Support Up/Down keys for vertical orientation.For vertical carousels, Line 90 and Line 93 still bind only left/right arrows. Mapping Up/Down in vertical mode improves keyboard accessibility and expected behavior.
♿ Suggested adjustment
const handleKeyDown = React.useCallback( (event: React.KeyboardEvent<HTMLDivElement>) => { - if (event.key === "ArrowLeft") { + const prevKey = orientation === "horizontal" ? "ArrowLeft" : "ArrowUp" + const nextKey = orientation === "horizontal" ? "ArrowRight" : "ArrowDown" + if (event.key === prevKey) { event.preventDefault() scrollPrev() - } else if (event.key === "ArrowRight") { + } else if (event.key === nextKey) { event.preventDefault() scrollNext() } }, - [scrollPrev, scrollNext] + [orientation, scrollPrev, scrollNext] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/carousel.tsx` around lines 88 - 99, The keyboard handler handleKeyDown currently only maps ArrowLeft/ArrowRight to scrollPrev/scrollNext; update it to also map ArrowUp/ArrowDown when the carousel is in vertical mode (e.g., check the orientation prop or isVertical flag) so Up triggers scrollPrev and Down triggers scrollNext, still calling event.preventDefault(); ensure you reference and use the existing scrollPrev and scrollNext functions and add the orientation/isVertical variable to the React.useCallback dependency array so the handler updates when orientation changes.lib/utils.ts (2)
7-10: Consider using stronger types instead ofany[].
PathDatausesany[]for bothnodesandlinks, butNodeandLinktypes are already imported. Using proper types would improve type safety and IDE support.Suggested fix
export type PathData = { - nodes: any[] - links: any[] + nodes: Node[] + links: Link[] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils.ts` around lines 7 - 10, PathData currently types nodes and links as any[], weakening type safety; change the type to use the imported Node and Link types (e.g., nodes: Node[] and links: Link[]) by updating the PathData type definition so it references the Node and Link symbols instead of any[] to improve typings and IDE support.
31-36: Consider stronger typing forMessage.paths.The
pathsproperty usesany[]for nodes and links. If these are the same asPathData, consider reusing that type for consistency.Suggested fix
export interface Message { type: MessageTypes; text?: string; - paths?: { nodes: any[], links: any[] }[]; + paths?: PathData[]; graphName?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils.ts` around lines 31 - 36, The Message interface currently types paths as { nodes: any[], links: any[] }[] which is too loose; change Message.paths to reuse the existing PathData type (or define a PathsEntry type) so nodes and links are strongly typed—update the Message declaration (symbol: Message and property: paths) to reference PathData (or the correct node/link types) and adjust any call sites that construct paths to match the stronger type.Dockerfile (1)
2-2: Consider using Node.js 22 LTS for production stability.Node.js 24 is the "current" release line but not yet LTS. Based on learnings, Node.js 22 (codename: Jod) is an LTS version and is suitable for production use in Dockerfiles. Using an LTS version provides longer support and greater stability for production deployments.
Suggested fix to use LTS version
# Use a Node.js base image -FROM node:24 +FROM node:22🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 2, The Dockerfile currently pins the base image to the non-LTS line ("FROM node:24"); update the base image to the Node.js 22 LTS release by replacing that reference with the Node 22 LTS tag (e.g., "node:22" or your preferred node:22 variant) so the container uses the stable LTS runtime for production deployments.components/ui/drawer.tsx (1)
47-47: Avoid forcing transparent overlay by default.Line 47 overrides the overlay’s default backdrop and removes modal dimming globally; consider letting
DrawerOverlaykeep its default unless explicitly overridden by callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/drawer.tsx` at line 47, The Drawer currently forces a transparent backdrop by hardcoding "bg-transparent" into the className on DrawerOverlay, which removes modal dimming globally; change the DrawerOverlay usage in the Drawer component so it does not include "bg-transparent" by default — simply pass overlayClassName (if any) or no extra classes so the underlying DrawerOverlay default styling remains intact; if you need a transparent overlay make it opt-in by applying overlayClassName from callers rather than hardcoding inside the Drawer component.e2e/logic/POM/codeGraph.ts (1)
713-732: Fail explicitly when canvas engine never reachesstopped.Current timeout path exits silently, so callers may continue on a moving graph and create flaky tests.
♻️ Suggested improvement
async waitForCanvasAnimationToEnd(timeout = 4500): Promise<void> { // ... while (Date.now() - startTime < timeout) { const status = await this.canvasElement.getAttribute("data-engine-status"); if (status === "stopped") { return; } await this.page.waitForTimeout(500); } + throw new Error(`Canvas engine did not reach "stopped" within ${timeout}ms`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logic/POM/codeGraph.ts` around lines 713 - 732, The waitForCanvasAnimationToEnd method silently returns when the canvas engine doesn't reach "stopped"; update waitForCanvasAnimationToEnd (which uses this.canvasElement and this.page.locator("falkordb-canvas")) to throw a descriptive error if the loop times out (i.e., after the timeout parameter elapses) instead of returning silently — include the timeout value and current status in the error message so callers fail fast when the canvas never reaches "stopped".app/components/ForceGraph.tsx (1)
83-89: Clean up the global graph accessor on unmount.Line 88 writes a function to
window, but this effect never removes it. After remounts, stale closures can point to detached canvas instances.♻️ Suggested fix
useEffect(() => { const canvas = canvasRef.current if (!canvas) return - (window as any)[id === "desktop" ? "graphDesktop" : "graphMobile"] = () => canvas.getGraphData(); + const key = id === "desktop" ? "graphDesktop" : "graphMobile" + ;(window as any)[key] = () => canvas.getGraphData() + + return () => { + delete (window as any)[key] + } }, [canvasRef, id])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ForceGraph.tsx` around lines 83 - 89, The effect in ForceGraph.tsx assigns a global accessor on window using id (graphDesktop or graphMobile) but never removes it, causing stale closures; update the useEffect that references canvasRef and id to return a cleanup function that removes or undefines (delete or set to undefined) (window as any)[id === "desktop" ? "graphDesktop" : "graphMobile"], ensuring the global is cleared on unmount or when id/canvasRef changes so canvasRef.current.getGraphData() no longer points to a detached canvas.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/code-graph-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (38)
.github/workflows/nextjs.yml.github/workflows/playwright.ymlDockerfileREADME.mdapp/api/repo/route.tsapp/components/ForceGraph.tsxapp/components/Input.tsxapp/components/chat.tsxapp/components/code-graph.tsxapp/components/combobox.tsxapp/components/dataPanel.tsxapp/components/elementMenu.tsxapp/components/graphView.tsxapp/components/model.tsapp/components/toolbar.tsxapp/globals.cssapp/layout.tsxapp/page.tsxcomponents.jsoncomponents/ui/carousel.tsxcomponents/ui/drawer.tsxcomponents/ui/switch.tsxdocker-compose.ymle2e/config/constants.tse2e/config/testData.tse2e/infra/ui/basePage.tse2e/infra/ui/browserWrapper.tse2e/logic/POM/codeGraph.tse2e/logic/utils.tse2e/tests/canvas.spec.tse2e/tests/chat.spec.tse2e/tests/navBar.spec.tse2e/tests/nodeDetailsPanel.spec.tse2e/tests/searchBar.spec.tslib/utils.tspackage.jsonplaywright.config.tstailwind.config.js
💤 Files with no reviewable changes (1)
- app/layout.tsx
Update SECRET_TOKEN in docker-compose.yml to use placeholder
…y and performance
…ove message handling
Fix ai comments
Contributor
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
barakb
approved these changes
Mar 3, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/canvas.spec.ts (1)
111-121:⚠️ Potential issue | 🟡 MinorAssert lookup results before accessing properties.
firstNode,secondNode,firstNode1, andsecondNode1could beundefined. Add guards to get clear failure messages.const firstNode = findNodeByName(initialGraph, path.firstNode); const secondNode = findNodeByName(initialGraph, path.secondNode); + expect(firstNode).toBeDefined(); + expect(secondNode).toBeDefined(); expect(firstNode.isPath).toBe(true); expect(secondNode.isPath).toBe(true); await codeGraph.clickOnClearGraphBtn(); const updateGraph = await codeGraph.getGraphNodes(); const firstNode1 = findNodeByName(updateGraph, path.firstNode); const secondNode1 = findNodeByName(updateGraph, path.secondNode); + expect(firstNode1).toBeDefined(); + expect(secondNode1).toBeDefined(); expect(firstNode1.isPath).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/canvas.spec.ts` around lines 111 - 121, The test accesses properties on variables that may be undefined (firstNode, secondNode, firstNode1, secondNode1); add explicit existence assertions before property access by using expect(...).toBeDefined() or guard checks after calling findNodeByName (e.g., expect(firstNode).toBeDefined(); expect(secondNode).toBeDefined(); then assert .isPath), and do the same for firstNode1 and secondNode1 after updateGraph so failures show clear messages; update the test around findNodeByName and the subsequent expects to include these guards.
♻️ Duplicate comments (14)
playwright.config.ts (1)
22-23:⚠️ Potential issue | 🟡 MinorUpdate the stale CI parallelism comment.
The comment says CI opts out of parallel tests, but Line 23 explicitly sets
workersto3.📝 Suggested fix
- /* Opt out of parallel tests on CI. */ + /* Limit CI parallelism to 3 workers. */ workers: process.env.CI ? 3 : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 22 - 23, The comment is misleading: the workers config actually forces CI parallelism to 3 via the workers: process.env.CI ? 3 : undefined line. Update the comment to accurately describe that CI runs with 3 workers (or change the expression if you intend to opt out); locate the workers setting (workers and process.env.CI) and either revise the comment to "Set CI workers to 3" or modify the ternary to the intended behavior.README.md (2)
9-9:⚠️ Potential issue | 🟡 MinorRemove stray hyphen line.
Line 9 is an orphan
-and should be deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 9, Remove the stray orphan line that contains only a single hyphen ("-") from the README content; locate the standalone line with just "-" and delete it so the document no longer contains that lone hyphen.
109-109:⚠️ Potential issue | 🟡 MinorPoint the issues link to this repository.
Line 109 currently links to
FalkorDB/GraphRAG-SDK/issues; this README is forFalkorDB/code-graph.Suggested change
-* [GitHub Issues](https://github.com/FalkorDB/GraphRAG-SDK/issues) +* [GitHub Issues](https://github.com/FalkorDB/code-graph/issues)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 109, Update the issues link in README (the bullet entry "* [GitHub Issues](https://github.com/FalkorDB/GraphRAG-SDK/issues)") to point to this repository's issues URL for FalkorDB/code-graph (i.e., replace the GraphRAG-SDK issues URL with https://github.com/FalkorDB/code-graph/issues) so the "GitHub Issues" link references the correct repository.app/api/repo/route.ts (3)
41-45:⚠️ Potential issue | 🔴 CriticalBlock untrusted
file://requests from selecting local-folder analysis.Line 41 lets any caller route to
analyze_folderby sending afile://URL. That should be denied by default (or strictly gated by a server-only allow flag/authz check).Suggested hardening
- const isLocal = repo_url.startsWith("file://") + const parsed = new URL(repo_url); + const allowLocalAnalysis = process.env.ENABLE_LOCAL_ANALYSIS === "true"; + const isLocal = parsed.protocol === "file:"; + if (isLocal && !allowLocalAnalysis) { + return NextResponse.json("Local file analysis is disabled", { status: 403 }); + } + if (!["http:", "https:", "file:"].includes(parsed.protocol)) { + return NextResponse.json("Unsupported URL scheme", { status: 400 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/repo/route.ts` around lines 41 - 45, Currently the handler in route.ts lets callers choose analyze_folder when repo_url.startsWith("file://"); change that logic so local file:// analysis is denied unless a server-side allow condition is met (e.g., an explicit environment flag like ALLOW_LOCAL_ANALYSIS or a server-only auth check), and return a 400/403 if file:// is requested without the allow flag; update the isLocal determination in the same block that builds the fetch URL and ensure analyze_folder is only used when the allow flag/check passes (otherwise force use of analyze_repo or reject the request).
43-51:⚠️ Potential issue | 🟠 MajorAdd a timeout to upstream
fetchto prevent hung API requests.This call has no abort/timeout path; a stalled backend can hang the route indefinitely.
Suggested change
const result = await fetch(`${url}/${isLocal ? "analyze_folder" : "analyze_repo"}`, { method: 'POST', body: JSON.stringify({ repo_url, ignore: ["./.github", "./sbin", "./.git", "./deps", "./bin", "./build"] }), headers: { "Authorization": token, 'Content-Type': 'application/json' }, + signal: AbortSignal.timeout(30_000), cache: 'no-store' });#!/bin/bash # Verify missing timeout in this route cat -n app/api/repo/route.ts | sed -n '39,53p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/repo/route.ts` around lines 43 - 51, The fetch call that assigns result (the code around result = await fetch(...)) has no timeout/abort and can hang; fix it by creating an AbortController, pass controller.signal into the fetch options (signal: controller.signal), start a timer (e.g., setTimeout) that calls controller.abort() after a sensible timeout (e.g., 10-30s), and clear that timer after fetch resolves or in finally; also handle the abort case in the surrounding async handler (catch the abort error and return an appropriate 504/timeout response). Ensure you modify the fetch invocation in route.ts where result is created and add the controller/timer setup and cleanup.
17-18:⚠️ Potential issue | 🟠 MajorDo not return raw upstream/internal errors to clients.
Throwing
await result.text()and returningerr.messageleaks backend details. Log full errors server-side, but return sanitized client messages.Suggested change
- if (!result.ok) { - throw new Error(await result.text()) - } + if (!result.ok) { + const upstream = await result.text(); + console.error("Repo API upstream failure", { status: result.status, upstream }); + throw new Error("Upstream service error"); + } ... - return NextResponse.json(err instanceof Error ? err.message : String(err), { status: 400 }) + return NextResponse.json("Request failed", { status: 502 })Also applies to: 25-25, 54-55, 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/repo/route.ts` around lines 17 - 18, The code is throwing raw upstream/internal error text (e.g., throw new Error(await result.text()) and rethrowing err.message), which leaks backend details to clients; change those sites so you log the full error server-side (use the existing logger or console.error) and throw/return a sanitized message to the client (e.g., new Error("Upstream service error" or "Internal error processing request")). Update every occurrence mentioned (the throw new Error(await result.text()) call and the places that rethrow err.message) to log the original error and return a generic client-facing error string instead.package.json (1)
33-35:⚠️ Potential issue | 🟠 MajorAlign App Router stack to React 19.
next@15.xwithreact@18.3.1/react-dom@18.3.1is a risky pairing for App Router usage. Please bump React/ReactDOM to 19-compatible versions to avoid runtime/build inconsistencies.Suggested change
- "react": "^18.3.1", - "react-dom": "^18.3.1", + "react": "^19.0.0", + "react-dom": "^19.0.0",#!/bin/bash # Verify current versions and App Router usage before bumping rg -n '^\s*"next"\s*:\s*' package.json rg -n '^\s*"react"\s*:\s*' package.json rg -n '^\s*"react-dom"\s*:\s*' package.json fd -t d '^app$' . fd -t f '^route\.ts$' app || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 33 - 35, package.json currently pins "next" at ^15.5.8 while "react" and "react-dom" are ^18.3.1 which is incompatible with the App Router—update the "react" and "react-dom" entries to React 19–compatible versions that match your Next 15.x peer requirements (e.g., bump both to a 19.x release), run package manager install and rebuild to ensure no peer conflicts, and run the provided checks (search for "next", "react", "react-dom" and verify an "app" directory or route.ts usage) to confirm App Router usage; adjust any code that relies on React 18-only APIs if build/runtime errors appear..github/workflows/playwright.yml (1)
39-39:⚠️ Potential issue | 🟠 MajorSplit app startup from test execution and drop CLI reporter override.
Line 39 introduces two issues: no readiness wait before tests (flaky CI) and
--reporter=dot,listoverridesplaywright.config.tsreporters.Suggested change
- NEXTAUTH_SECRET=SECRET npm start & npx playwright test --shard=${{ matrix.shard }}/2 --reporter=dot,list + NEXTAUTH_SECRET=SECRET npm start & + for i in {1..60}; do + curl -fsS http://127.0.0.1:3000 >/dev/null && break + sleep 1 + done + npx playwright test --shard=${{ matrix.shard }}/2#!/bin/bash # Verify workflow invocation and configured reporters cat -n .github/workflows/playwright.yml | sed -n '34,45p' fd -t f 'playwright.config.*' -x cat -n {}Based on learnings: In Playwright, CLI --reporter flags completely override the reporter configuration in playwright.config.ts, even when the config file has conditional reporter settings for CI environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/playwright.yml at line 39, The workflow currently starts the app and launches tests in one combined command using NEXTAUTH_SECRET=SECRET npm start & npx playwright test --shard=${{ matrix.shard }}/2 --reporter=dot,list which causes flaky tests (no readiness wait) and forcibly overrides reporters in playwright.config.ts; fix by splitting startup and test steps: run npm start in background (or use a proper background launcher), wait for an explicit readiness probe (e.g., loop/poll a health endpoint or use wait-for script against your app host/port) before running npx playwright test --shard=${{ matrix.shard }}/2 (remove --reporter=dot,list so reporters from playwright.config.ts are honored), and ensure the background server is terminated/cleaned up after tests complete.app/globals.css (1)
97-97:⚠️ Potential issue | 🟠 MajorFix Biome parsing for Tailwind
@applydirectives.Line 97 and Line 109 use valid Tailwind syntax, but Biome is currently flagging them as parse errors. This should be fixed in Biome config so CI/lint does not fail on valid CSS.
#!/bin/bash # Confirm failing directives and inspect Biome config rg -n '@apply' app/globals.css fd 'biome\.json(c)?$|\.biomerc(\.json)?$' cat -n biome.json 2>/dev/null || trueAlso applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/globals.css` at line 97, Biome is flagging valid Tailwind `@apply` directives (e.g. the "@apply text-blue flex gap-2 p-2 border border-blue hover:opacity-80 rounded-lg text-left w-fit;" line) because the CSS parser/rules aren't configured for PostCSS/Tailwind at-rules; update your Biome config (biome.json or .biomerc.json) to use the PostCSS/Tailwind-aware CSS parser and/or relax unknown at-rule checks for Tailwind so `@apply` is accepted (for example, set the CSS parser/syntax to postcss and disable or adjust the at-rule-unknown rule), then re-run the linter to verify lines containing "@apply" no longer error.e2e/tests/chat.spec.ts (2)
123-124:⚠️ Potential issue | 🟡 MinorQuestion-option indexing is off by one.
Line 124 shifts selection to
2..6while the loop label implies1..N. This skips the first option and misaligns the assertion intent.🔢 Fix index mapping
- for (let index = 1; index < 6; index++) { - const questionNumber = index + 1; + for (let index = 1; index <= CHAT_OPTIONS_COUNT; index++) { + const questionNumber = index;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/chat.spec.ts` around lines 123 - 124, The loop in chat.spec.ts uses for (let index = 1; index < 6; index++) but then sets const questionNumber = index + 1 which shifts selection to 2..6 and skips the first option; change the mapping so questionNumber aligns with the loop intent—either start index at 0 and keep questionNumber = index + 1, or keep index starting at 1 and set questionNumber = index—update the loop variables/assignment where index and questionNumber are defined to restore 1..N indexing for the assertions in this test.
69-69:⚠️ Potential issue | 🟡 MinorRemove unsafe non-null assertions after optional chaining.
Line 69 and Line 85 use
?.[0]!, which can still crash when regex matching fails. Assert the match explicitly before indexing.✅ Safer extraction pattern
- const number = result.match(/\d+/g)?.[0]!; + const match = result.match(/\d+/g); + expect(match?.[0]).toBeDefined(); + const number = match![0]; ... - const number = uiResponse.match(/\d+/g)?.[0]!; - - expect(number).toEqual(apiResponse.result.response.match(/\d+/g)?.[0]); + const uiMatch = uiResponse.match(/\d+/g); + const apiMatch = apiResponse.result.response.match(/\d+/g); + expect(uiMatch?.[0]).toBeDefined(); + expect(apiMatch?.[0]).toBeDefined(); + expect(uiMatch![0]).toEqual(apiMatch![0]);#!/bin/bash # Verify no forbidden non-null assertions remain in this test file rg -nP '\?\.\[0\]!' e2e/tests/chat.spec.tsAlso applies to: 85-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/chat.spec.ts` at line 69, The code uses unsafe non-null assertions after optional chaining in expressions like const number = result.match(/\d+/g)?.[0]! which can still throw if match returns null; update both occurrences (the const named number derived from result.match and the similar occurrence at line 85) to explicitly check the match result before indexing—e.g., call const match = result.match(/\d+/g); if (!match) throw or fail the test with a clear message, then use match[0]; ensure you replace the ?.[0]! usage with this explicit null-check pattern referencing result.match and the local match variable.e2e/tests/nodeDetailsPanel.spec.ts (1)
79-82:⚠️ Potential issue | 🟠 MajorGuard empty graph and unresolved
nodeNamebefore search input.Line 79/Line 80 can dereference
undefinedwhengraphDatais empty or malformed, causing an early crash unrelated to the test objective.🛠️ Proposed guard
const graphData = await codeGraph.getGraphNodes(); + expect(graphData.length).toBeGreaterThan(0); const targetNode = graphData.find(node => node.src) || graphData[0]; + expect(targetNode).toBeDefined(); + if (!targetNode) throw new Error("No suitable node found in FLASK graph"); const nodeName = targetNode.name || targetNode.data?.name; + expect(nodeName).toBeDefined(); + if (!nodeName) throw new Error("Unable to resolve node name for search"); await codeGraph.fillSearchBar(nodeName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/nodeDetailsPanel.spec.ts` around lines 79 - 82, The test dereferences graphData and targetNode without guards which can throw if graphData is empty or malformed; update the block that computes targetNode and nodeName (references: graphData, targetNode, nodeName) to first assert or return early if graphData is falsy/empty, ensure targetNode exists, and ensure nodeName is a defined non-empty string before calling codeGraph.fillSearchBar and codeGraph.selectSearchBarOptionBtn; if no valid nodeName is available, either skip the search steps or fail the test with a clear message so the test doesn't crash with an unrelated exception.app/components/elementMenu.tsx (2)
47-48:⚠️ Potential issue | 🟠 MajorUse container height for vertical clamping.
Line 48 clamps
topusingcontainerWidth, which applies horizontal size to vertical bounds and can place the menu incorrectly on Y-axis.Suggested fix
-const [containerWidth, setContainerWidth] = useState(0); +const [containerWidth, setContainerWidth] = useState(0); +const [containerHeight, setContainerHeight] = useState(0); ref={(ref) => { if (!ref) return setContainerWidth(ref.clientWidth) + setContainerHeight(ref.clientHeight) }} style={{ left: Math.max(8, Math.min(position.x - containerWidth / 2, (parentRef?.current?.clientWidth || 0) - containerWidth - 8)), - top: Math.max(8, Math.min(position.y - 153, (parentRef?.current?.clientHeight || 0) - containerWidth - 8)), + top: Math.max(8, Math.min(position.y - 153, (parentRef?.current?.clientHeight || 0) - containerHeight - 8)), }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/elementMenu.tsx` around lines 47 - 48, The vertical clamp for top uses containerWidth by mistake; update the top calculation to use the container's height instead (e.g., replace containerWidth with containerHeight) so the expression becomes Math.max(8, Math.min(position.y - 153, (parentRef?.current?.clientHeight || 0) - containerHeight - 8)); ensure containerHeight is defined/derived the same way containerWidth is (or compute it from the same DOM ref) and keep the other identifiers (position.y, parentRef) unchanged.
125-132:⚠️ Potential issue | 🟠 MajorHarden external link behavior and avoid duplicate tab opens.
Lines 128-132 combine
target="_blank"with manualwindow.open, which can open two tabs. Also addrel="noopener noreferrer"for security.Suggested fix
<a className="p-2" href={objURL} target="_blank" + rel="noopener noreferrer" title="Go to repo" - onClick={() => { - window.open(objURL, '_blank'); - }} > <Globe color="white" /> </a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/elementMenu.tsx` around lines 125 - 132, The anchor element (className="p-2", href={objURL}, target="_blank"}, onClick handler) is opening the same URL twice and lacks secure rel attributes; either remove the manual window.open call or remove target="_blank" and use window.open with rel="noopener noreferrer" — if keeping the onClick for analytics, call event.preventDefault() then window.open(objURL, '_blank', 'noopener,noreferrer'); otherwise keep target="_blank" and add rel="noopener noreferrer" and delete the onClick to avoid duplicate tabs.
🧹 Nitpick comments (9)
components/ui/carousel.tsx (2)
88-99: Keyboard navigation does not adapt to vertical orientation.For vertical carousels, users intuitively expect
ArrowUpandArrowDownkeys for navigation, but this handler always usesArrowLeft/ArrowRightregardless of orientation. This may cause accessibility and usability issues for vertical carousels.Proposed fix to support orientation-aware keyboard navigation
const handleKeyDown = React.useCallback( (event: React.KeyboardEvent<HTMLDivElement>) => { - if (event.key === "ArrowLeft") { + const prevKey = orientation === "horizontal" ? "ArrowLeft" : "ArrowUp" + const nextKey = orientation === "horizontal" ? "ArrowRight" : "ArrowDown" + + if (event.key === prevKey) { event.preventDefault() scrollPrev() - } else if (event.key === "ArrowRight") { + } else if (event.key === nextKey) { event.preventDefault() scrollNext() } }, - [scrollPrev, scrollNext] + [orientation, scrollPrev, scrollNext] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/carousel.tsx` around lines 88 - 99, handleKeyDown currently only handles "ArrowLeft"/"ArrowRight" which breaks keyboard navigation for vertical carousels; update the handleKeyDown callback to branch on the carousel orientation (e.g., check the orientation prop or state used by this component) and use "ArrowUp"/"ArrowDown" when orientation === "vertical" and "ArrowLeft"/"ArrowRight" otherwise, calling scrollPrev()/scrollNext() and event.preventDefault() accordingly; also add the orientation identifier to the hook dependency array so the callback updates when orientation changes (refer to handleKeyDown, scrollPrev, scrollNext, and the orientation prop/state).
210-210: Minor: Double space in className.There's an extra space in the className string (
"absolute h-8 w-8").Fix
className={cn( - "absolute h-8 w-8 rounded-full", + "absolute h-8 w-8 rounded-full",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/carousel.tsx` at line 210, In the Carousel component update the offending className string that contains a double space ("absolute h-8 w-8 rounded-full") to remove the extra space so it becomes a single-space separated class list (e.g., "absolute h-8 w-8 rounded-full"); locate the JSX element or prop where that string is used (the carousel navigation button/className) and correct the spacing in the literal..github/workflows/playwright.yml (1)
26-27: Use a single install strategy in CI.
npm ciis already executed, thennpm installruns again in the test step. Remove the second install to keep runs deterministic and faster.Suggested change
run: | - npm install npm run build NEXTAUTH_SECRET=SECRET npm start & npx playwright test --shard=${{ matrix.shard }}/2 --reporter=dot,listAlso applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/playwright.yml around lines 26 - 27, The workflow runs npm ci already, so remove the redundant npm install invocation in the test step (the second install that appears after the "run: npm ci" and before/within the "Install Playwright Browsers" step) to keep CI deterministic and faster; keep the dedicated "Install Playwright Browsers" step (or replace its npm install call with only the Playwright browser install command such as the npx playwright install call) so browsers are still installed but no extra npm install is executed.components/ui/drawer.tsx (1)
56-56: Mark the drag handle as decorative for assistive technology.Line 56 renders a non-semantic visual handle. Add
aria-hidden="true"to avoid unnecessary screen-reader noise.♿ Suggested tweak
- <div className={cn("mx-auto mt-4 h-2 w-[100px] rounded-full bg-muted", handleClassName)} /> + <div + aria-hidden="true" + className={cn("mx-auto mt-4 h-2 w-[100px] rounded-full bg-muted", handleClassName)} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/drawer.tsx` at line 56, The visual drag handle div rendered in the Drawer component (the element with className built from "mx-auto mt-4 h-2 w-[100px] rounded-full bg-muted" and handleClassName) should be marked decorative for assistive tech; update that div to include aria-hidden="true" so screen readers ignore it (locate the div in components/ui/drawer.tsx and add the aria attribute to the same JSX element).app/components/ForceGraph.tsx (1)
88-89: Clean up global graph accessors on unmount.Line 88 sets a global function but never removes it. This can leave stale references after component teardown/remount.
🧹 Suggested cleanup
useEffect(() => { const canvas = canvasRef.current if (!canvas) return - (window as any)[id === "desktop" ? "graphDesktop" : "graphMobile"] = () => canvas.getGraphData(); + const key = id === "desktop" ? "graphDesktop" : "graphMobile" + ;(window as any)[key] = () => canvas.getGraphData() + + return () => { + delete (window as any)[key] + } }, [canvasRef, id])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ForceGraph.tsx` around lines 88 - 89, The effect in ForceGraph.tsx sets a global accessor (window[ id === "desktop" ? "graphDesktop" : "graphMobile" ] = () => canvas.getGraphData()) but never removes it; add a cleanup function returned from the useEffect that deletes or nulls that global key on unmount (guarding for the same key computed from id) so stale references to canvas.getGraphData() are removed; reference the existing symbols canvas.getGraphData, canvasRef, id and the window keys "graphDesktop"/"graphMobile" when implementing the cleanup.e2e/tests/searchBar.spec.ts (1)
85-86: Canvas-center right-click is brittle for node verification.Line 85 assumes the selected node is exactly centered after animation. This can intermittently miss the node and fail Line 86. Prefer clicking resolved node coordinates (from graph data) before asserting tooltip content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/searchBar.spec.ts` around lines 85 - 86, The test is brittle because codeGraph.rightClickAtCanvasCenter() may miss the intended node; instead resolve the target node's coordinates and click there before asserting the tooltip. Replace the center click with a deterministic click using the node's coordinates (obtainable from the graph data or a helper like codeGraph.getNodePosition(nodeName) or graphData.getNodeByName(nodeName).x/y), call the click at those coordinates (e.g., codeGraph.clickAt(x,y)), then assert via codeGraph.getNodeToolTipContent() contains nodeName.app/components/code-graph.tsx (2)
206-213: Avoid returning values fromforEachcallbacks.The
forEachat line 208 returns the result ofdeleteIdsMap.add(), which Biome flags as suspicious. While harmless here, it's clearer to use an explicit statement.♻️ Suggested fix
- deleteNeighbors(expandedNodes)?.forEach(id => deleteIdsMap.add(id)) + deleteNeighbors(expandedNodes)?.forEach(id => { + deleteIdsMap.add(id) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/code-graph.tsx` around lines 206 - 213, The forEach callback currently implicitly returns the result of deleteIdsMap.add(), which is flagged as suspicious; change the call site (where deleteNeighbors(expandedNodes)?.forEach(...)) to avoid returning values from the callback by first capturing the result of deleteNeighbors(expandedNodes) into a variable and then iterating with an explicit statement (e.g., if the result is truthy, call .forEach(id => { deleteIdsMap.add(id); }) or use a for...of loop) so the callback body contains a clear statement instead of an expression — target the deleteNeighbors(...) invocation and the deleteIdsMap.add usage to implement this.
410-416: Avoid returning values fromforEachcallbacks.The assignment
c.show = truereturnstrue, which Biome flags as suspicious in aforEachcallback.♻️ Suggested fix
- graph.Categories.forEach(c => c.show = true); + graph.Categories.forEach(c => { + c.show = true + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/code-graph.tsx` around lines 410 - 416, The forEach callback on graph.Categories returns the assignment result (c.show = true) which Biome flags; change the callback to use a statement block or an explicit loop so it doesn't return a value — e.g., replace graph.Categories.forEach(c => c.show = true) with a block-style callback (graph.Categories.forEach(c => { c.show = true; })) or a for..of loop over graph.Categories; keep the existing graph.getElements().forEach((element) => { element.visible = true }) as-is.app/components/graphView.tsx (1)
253-255: Consider a more sensible default for mobile breakpoint.The fallback value of
0means ifNEXT_PUBLIC_MOBILE_BREAKPOINTis unset,isDesktopwill always betrue(anyscreenSize > 0). This is likely the intended behavior but a common breakpoint like768might be safer.const mobileBreakpointRaw = Number(process.env.NEXT_PUBLIC_MOBILE_BREAKPOINT) -const mobileBreakpoint = Number.isFinite(mobileBreakpointRaw) ? mobileBreakpointRaw : 0 +const mobileBreakpoint = Number.isFinite(mobileBreakpointRaw) ? mobileBreakpointRaw : 768🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/graphView.tsx` around lines 253 - 255, The fallback mobile breakpoint of 0 makes isDesktop (computed from screenSize > mobileBreakpoint) true whenever NEXT_PUBLIC_MOBILE_BREAKPOINT is unset; change the fallback to a sensible default like 768 by updating how mobileBreakpoint is computed (use mobileBreakpointRaw and mobileBreakpoint) so that Number(process.env.NEXT_PUBLIC_MOBILE_BREAKPOINT) falls back to 768 when not valid, ensuring isDesktop behaves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/chat.tsx`:
- Around line 118-119: Guard against empty or missing path nodes before indexing
p.nodes in the Chat component: before computing firstNodeId and lastNodeId (and
the similar use around the later occurrence at the p.nodes[...]-location) check
that p.nodes exists and p.nodes.length > 0, and handle the empty case (e.g.,
skip indexing, set safe defaults, or bail out of that iteration). Update the
logic around the expressions that reference p.nodes[0].id and
p.nodes[p.nodes.length - 1].id to use this guard so malformed/empty path
responses do not throw at runtime.
- Around line 170-177: The loop that marks path elements uses e.isPathSelected =
true for endpoints and never clears isPathSelected for shared non-endpoint
nodes, causing stale selection state; update the logic in the forEach that
iterates over elements (the block referencing pLinkIds, pNodeIds, firstNodeId,
lastNodeId) to explicitly set both e.isPathSelected = false and e.isPath = false
for elements that are not part of the new path, and only set e.isPathSelected =
true for endpoints (firstNodeId/lastNodeId) or e.isPath = true for other path
elements; apply the same deterministic reset to the analogous loop at the other
occurrence (the block around lines 209-217) to ensure no stale flags remain.
In `@app/components/combobox.tsx`:
- Line 9: onSelectedValue (typed as (value: string) => Promise<void>) is passed
directly to the Select's onValueChange, which lets async rejections escape the
event handler; update the call site so the Select receives a non-async callback
that invokes handleSelectedValue/onSelectedValue and catches errors (e.g., wrap
with () => handleSelectedValue(value).catch(err => { /* surface error: set error
state, show toast, or log via processLogger */ })), ensuring any rejection from
onFetchGraph is handled and user-visible feedback is provided; keep the original
function signatures (onSelectedValue, handleSelectedValue, onFetchGraph) but add
the catching wrapper where you pass the callback to onValueChange/Select.
In `@app/components/model.ts`:
- Around line 173-175: When building Node objects where you currently spread
nodeData.properties into data (the occurrences around nodeData and the Node.data
construction), ensure Node.data.name is always set and is a string: if
nodeData.properties.name is missing or falsy, set name to a derived string
(e.g., String(nodeData.id) or a computed label) and avoid assigning raw numeric
ids. Update the spots that currently do "...nodeData.properties" so they
explicitly set data.name = nodeData.properties.name ?? String(nodeData.id) (or
similar) and coerce to string to prevent numeric names breaking name-dependent
UI paths.
In `@e2e/logic/utils.ts`:
- Around line 34-35: The timeout path in waitForStableText currently returns
stableText which can hide failures; change the timeout handling in
waitForStableText so that instead of returning stableText when the timeout
elapses it throws a clear Error (including context like the last observed
stableText and the timeout duration) to fail tests explicitly; update references
to stableText and the timeout variable inside waitForStableText so callers see a
thrown exception rather than a silent return on timeout.
- Around line 5-13: waitToBeEnabled currently throws if locator.elementHandle()
returns null because it doesn't wait for the element to be attached; modify
waitToBeEnabled to first await locator.waitFor({ state: "attached", timeout })
(using the same timeout parameter) before calling locator.elementHandle(), then
proceed to use locator.page().waitForFunction(...) to wait until the element
does not have the "disabled" attribute; keep existing error handling but rely on
locator.waitFor to surface a timeout if the element never attaches.
In `@e2e/tests/canvas.spec.ts`:
- Around line 86-88: The test currently reads the graph and immediately accesses
targetNodeForUpdateGraph.visible, which can throw if findNodeByName returns
undefined; update the assertions around codeGraph.getGraphNodes()/findNodeByName
so you first assert targetNodeForUpdateGraph is defined (e.g.,
expect(targetNodeForUpdateGraph).toBeDefined()) using the result of
findNodeByName(updatedGraph, nodes[0].nodeName), then assert its visible
property—this ensures a clear test failure when the node is missing and prevents
a runtime error.
- Around line 97-99: The test currently assumes findItem exists after calling
codeGraph.getGraphNodes(); change the assertion to first verify the lookup
result (the variable findItem from result.find using category) is defined before
accessing its .visible property—e.g., add an assertion like
expect(findItem).toBeDefined() or expect(findItem).not.toBeUndefined()
immediately after the find, then assert expect(findItem.visible).toBeFalsy();
this prevents a runtime error when no node matches the category.
In `@lib/utils.ts`:
- Around line 6-9: The exported type PathData uses any[] which weakens typing;
change PathData.nodes and PathData.links from any[] to unknown[] and then update
the Message type to reference PathData for its paths property (reuse PathData
instead of duplicating types) so Message.paths: PathData[] (and remove
duplicated any[] usage) to improve downstream type safety.
In `@playwright.config.ts`:
- Around line 25-26: The CLI `--reporter=dot,list` flag is overriding the
Playwright config reporters so the reporter setting (reporter) and artifact
paths (outputFolder/outputDir) in playwright.config.ts are ignored; fix by
either removing the `--reporter=dot,list` from the CI command in the workflow so
Playwright uses the config's reporter and respects outputFolder/outputDir, or
else update the reporter entry in playwright.config.ts to match the CI reporter
(e.g., change the reporter setting used by the `reporter` symbol to include dot
and list) so CI and config align.
---
Outside diff comments:
In `@e2e/tests/canvas.spec.ts`:
- Around line 111-121: The test accesses properties on variables that may be
undefined (firstNode, secondNode, firstNode1, secondNode1); add explicit
existence assertions before property access by using expect(...).toBeDefined()
or guard checks after calling findNodeByName (e.g.,
expect(firstNode).toBeDefined(); expect(secondNode).toBeDefined(); then assert
.isPath), and do the same for firstNode1 and secondNode1 after updateGraph so
failures show clear messages; update the test around findNodeByName and the
subsequent expects to include these guards.
---
Duplicate comments:
In @.github/workflows/playwright.yml:
- Line 39: The workflow currently starts the app and launches tests in one
combined command using NEXTAUTH_SECRET=SECRET npm start & npx playwright test
--shard=${{ matrix.shard }}/2 --reporter=dot,list which causes flaky tests (no
readiness wait) and forcibly overrides reporters in playwright.config.ts; fix by
splitting startup and test steps: run npm start in background (or use a proper
background launcher), wait for an explicit readiness probe (e.g., loop/poll a
health endpoint or use wait-for script against your app host/port) before
running npx playwright test --shard=${{ matrix.shard }}/2 (remove
--reporter=dot,list so reporters from playwright.config.ts are honored), and
ensure the background server is terminated/cleaned up after tests complete.
In `@app/api/repo/route.ts`:
- Around line 41-45: Currently the handler in route.ts lets callers choose
analyze_folder when repo_url.startsWith("file://"); change that logic so local
file:// analysis is denied unless a server-side allow condition is met (e.g., an
explicit environment flag like ALLOW_LOCAL_ANALYSIS or a server-only auth
check), and return a 400/403 if file:// is requested without the allow flag;
update the isLocal determination in the same block that builds the fetch URL and
ensure analyze_folder is only used when the allow flag/check passes (otherwise
force use of analyze_repo or reject the request).
- Around line 43-51: The fetch call that assigns result (the code around result
= await fetch(...)) has no timeout/abort and can hang; fix it by creating an
AbortController, pass controller.signal into the fetch options (signal:
controller.signal), start a timer (e.g., setTimeout) that calls
controller.abort() after a sensible timeout (e.g., 10-30s), and clear that timer
after fetch resolves or in finally; also handle the abort case in the
surrounding async handler (catch the abort error and return an appropriate
504/timeout response). Ensure you modify the fetch invocation in route.ts where
result is created and add the controller/timer setup and cleanup.
- Around line 17-18: The code is throwing raw upstream/internal error text
(e.g., throw new Error(await result.text()) and rethrowing err.message), which
leaks backend details to clients; change those sites so you log the full error
server-side (use the existing logger or console.error) and throw/return a
sanitized message to the client (e.g., new Error("Upstream service error" or
"Internal error processing request")). Update every occurrence mentioned (the
throw new Error(await result.text()) call and the places that rethrow
err.message) to log the original error and return a generic client-facing error
string instead.
In `@app/components/elementMenu.tsx`:
- Around line 47-48: The vertical clamp for top uses containerWidth by mistake;
update the top calculation to use the container's height instead (e.g., replace
containerWidth with containerHeight) so the expression becomes Math.max(8,
Math.min(position.y - 153, (parentRef?.current?.clientHeight || 0) -
containerHeight - 8)); ensure containerHeight is defined/derived the same way
containerWidth is (or compute it from the same DOM ref) and keep the other
identifiers (position.y, parentRef) unchanged.
- Around line 125-132: The anchor element (className="p-2", href={objURL},
target="_blank"}, onClick handler) is opening the same URL twice and lacks
secure rel attributes; either remove the manual window.open call or remove
target="_blank" and use window.open with rel="noopener noreferrer" — if keeping
the onClick for analytics, call event.preventDefault() then window.open(objURL,
'_blank', 'noopener,noreferrer'); otherwise keep target="_blank" and add
rel="noopener noreferrer" and delete the onClick to avoid duplicate tabs.
In `@app/globals.css`:
- Line 97: Biome is flagging valid Tailwind `@apply` directives (e.g. the "@apply
text-blue flex gap-2 p-2 border border-blue hover:opacity-80 rounded-lg
text-left w-fit;" line) because the CSS parser/rules aren't configured for
PostCSS/Tailwind at-rules; update your Biome config (biome.json or
.biomerc.json) to use the PostCSS/Tailwind-aware CSS parser and/or relax unknown
at-rule checks for Tailwind so `@apply` is accepted (for example, set the CSS
parser/syntax to postcss and disable or adjust the at-rule-unknown rule), then
re-run the linter to verify lines containing "@apply" no longer error.
In `@e2e/tests/chat.spec.ts`:
- Around line 123-124: The loop in chat.spec.ts uses for (let index = 1; index <
6; index++) but then sets const questionNumber = index + 1 which shifts
selection to 2..6 and skips the first option; change the mapping so
questionNumber aligns with the loop intent—either start index at 0 and keep
questionNumber = index + 1, or keep index starting at 1 and set questionNumber =
index—update the loop variables/assignment where index and questionNumber are
defined to restore 1..N indexing for the assertions in this test.
- Line 69: The code uses unsafe non-null assertions after optional chaining in
expressions like const number = result.match(/\d+/g)?.[0]! which can still throw
if match returns null; update both occurrences (the const named number derived
from result.match and the similar occurrence at line 85) to explicitly check the
match result before indexing—e.g., call const match = result.match(/\d+/g); if
(!match) throw or fail the test with a clear message, then use match[0]; ensure
you replace the ?.[0]! usage with this explicit null-check pattern referencing
result.match and the local match variable.
In `@e2e/tests/nodeDetailsPanel.spec.ts`:
- Around line 79-82: The test dereferences graphData and targetNode without
guards which can throw if graphData is empty or malformed; update the block that
computes targetNode and nodeName (references: graphData, targetNode, nodeName)
to first assert or return early if graphData is falsy/empty, ensure targetNode
exists, and ensure nodeName is a defined non-empty string before calling
codeGraph.fillSearchBar and codeGraph.selectSearchBarOptionBtn; if no valid
nodeName is available, either skip the search steps or fail the test with a
clear message so the test doesn't crash with an unrelated exception.
In `@package.json`:
- Around line 33-35: package.json currently pins "next" at ^15.5.8 while "react"
and "react-dom" are ^18.3.1 which is incompatible with the App Router—update the
"react" and "react-dom" entries to React 19–compatible versions that match your
Next 15.x peer requirements (e.g., bump both to a 19.x release), run package
manager install and rebuild to ensure no peer conflicts, and run the provided
checks (search for "next", "react", "react-dom" and verify an "app" directory or
route.ts usage) to confirm App Router usage; adjust any code that relies on
React 18-only APIs if build/runtime errors appear.
In `@playwright.config.ts`:
- Around line 22-23: The comment is misleading: the workers config actually
forces CI parallelism to 3 via the workers: process.env.CI ? 3 : undefined line.
Update the comment to accurately describe that CI runs with 3 workers (or change
the expression if you intend to opt out); locate the workers setting (workers
and process.env.CI) and either revise the comment to "Set CI workers to 3" or
modify the ternary to the intended behavior.
In `@README.md`:
- Line 9: Remove the stray orphan line that contains only a single hyphen ("-")
from the README content; locate the standalone line with just "-" and delete it
so the document no longer contains that lone hyphen.
- Line 109: Update the issues link in README (the bullet entry "* [GitHub
Issues](https://github.com/FalkorDB/GraphRAG-SDK/issues)") to point to this
repository's issues URL for FalkorDB/code-graph (i.e., replace the GraphRAG-SDK
issues URL with https://github.com/FalkorDB/code-graph/issues) so the "GitHub
Issues" link references the correct repository.
---
Nitpick comments:
In @.github/workflows/playwright.yml:
- Around line 26-27: The workflow runs npm ci already, so remove the redundant
npm install invocation in the test step (the second install that appears after
the "run: npm ci" and before/within the "Install Playwright Browsers" step) to
keep CI deterministic and faster; keep the dedicated "Install Playwright
Browsers" step (or replace its npm install call with only the Playwright browser
install command such as the npx playwright install call) so browsers are still
installed but no extra npm install is executed.
In `@app/components/code-graph.tsx`:
- Around line 206-213: The forEach callback currently implicitly returns the
result of deleteIdsMap.add(), which is flagged as suspicious; change the call
site (where deleteNeighbors(expandedNodes)?.forEach(...)) to avoid returning
values from the callback by first capturing the result of
deleteNeighbors(expandedNodes) into a variable and then iterating with an
explicit statement (e.g., if the result is truthy, call .forEach(id => {
deleteIdsMap.add(id); }) or use a for...of loop) so the callback body contains a
clear statement instead of an expression — target the deleteNeighbors(...)
invocation and the deleteIdsMap.add usage to implement this.
- Around line 410-416: The forEach callback on graph.Categories returns the
assignment result (c.show = true) which Biome flags; change the callback to use
a statement block or an explicit loop so it doesn't return a value — e.g.,
replace graph.Categories.forEach(c => c.show = true) with a block-style callback
(graph.Categories.forEach(c => { c.show = true; })) or a for..of loop over
graph.Categories; keep the existing graph.getElements().forEach((element) => {
element.visible = true }) as-is.
In `@app/components/ForceGraph.tsx`:
- Around line 88-89: The effect in ForceGraph.tsx sets a global accessor
(window[ id === "desktop" ? "graphDesktop" : "graphMobile" ] = () =>
canvas.getGraphData()) but never removes it; add a cleanup function returned
from the useEffect that deletes or nulls that global key on unmount (guarding
for the same key computed from id) so stale references to canvas.getGraphData()
are removed; reference the existing symbols canvas.getGraphData, canvasRef, id
and the window keys "graphDesktop"/"graphMobile" when implementing the cleanup.
In `@app/components/graphView.tsx`:
- Around line 253-255: The fallback mobile breakpoint of 0 makes isDesktop
(computed from screenSize > mobileBreakpoint) true whenever
NEXT_PUBLIC_MOBILE_BREAKPOINT is unset; change the fallback to a sensible
default like 768 by updating how mobileBreakpoint is computed (use
mobileBreakpointRaw and mobileBreakpoint) so that
Number(process.env.NEXT_PUBLIC_MOBILE_BREAKPOINT) falls back to 768 when not
valid, ensuring isDesktop behaves correctly.
In `@components/ui/carousel.tsx`:
- Around line 88-99: handleKeyDown currently only handles
"ArrowLeft"/"ArrowRight" which breaks keyboard navigation for vertical
carousels; update the handleKeyDown callback to branch on the carousel
orientation (e.g., check the orientation prop or state used by this component)
and use "ArrowUp"/"ArrowDown" when orientation === "vertical" and
"ArrowLeft"/"ArrowRight" otherwise, calling scrollPrev()/scrollNext() and
event.preventDefault() accordingly; also add the orientation identifier to the
hook dependency array so the callback updates when orientation changes (refer to
handleKeyDown, scrollPrev, scrollNext, and the orientation prop/state).
- Line 210: In the Carousel component update the offending className string that
contains a double space ("absolute h-8 w-8 rounded-full") to remove the extra
space so it becomes a single-space separated class list (e.g., "absolute h-8 w-8
rounded-full"); locate the JSX element or prop where that string is used (the
carousel navigation button/className) and correct the spacing in the literal.
In `@components/ui/drawer.tsx`:
- Line 56: The visual drag handle div rendered in the Drawer component (the
element with className built from "mx-auto mt-4 h-2 w-[100px] rounded-full
bg-muted" and handleClassName) should be marked decorative for assistive tech;
update that div to include aria-hidden="true" so screen readers ignore it
(locate the div in components/ui/drawer.tsx and add the aria attribute to the
same JSX element).
In `@e2e/tests/searchBar.spec.ts`:
- Around line 85-86: The test is brittle because
codeGraph.rightClickAtCanvasCenter() may miss the intended node; instead resolve
the target node's coordinates and click there before asserting the tooltip.
Replace the center click with a deterministic click using the node's coordinates
(obtainable from the graph data or a helper like
codeGraph.getNodePosition(nodeName) or graphData.getNodeByName(nodeName).x/y),
call the click at those coordinates (e.g., codeGraph.clickAt(x,y)), then assert
via codeGraph.getNodeToolTipContent() contains nodeName.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/code-graph-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (38)
.github/workflows/nextjs.yml.github/workflows/playwright.ymlDockerfileREADME.mdapp/api/repo/route.tsapp/components/ForceGraph.tsxapp/components/Input.tsxapp/components/chat.tsxapp/components/code-graph.tsxapp/components/combobox.tsxapp/components/dataPanel.tsxapp/components/elementMenu.tsxapp/components/graphView.tsxapp/components/model.tsapp/components/toolbar.tsxapp/globals.cssapp/layout.tsxapp/page.tsxcomponents.jsoncomponents/ui/carousel.tsxcomponents/ui/drawer.tsxcomponents/ui/switch.tsxdocker-compose.ymle2e/config/constants.tse2e/config/testData.tse2e/infra/ui/basePage.tse2e/infra/ui/browserWrapper.tse2e/logic/POM/codeGraph.tse2e/logic/utils.tse2e/tests/canvas.spec.tse2e/tests/chat.spec.tse2e/tests/navBar.spec.tse2e/tests/nodeDetailsPanel.spec.tse2e/tests/searchBar.spec.tslib/utils.tspackage.jsonplaywright.config.tstailwind.config.js
💤 Files with no reviewable changes (1)
- app/layout.tsx
barakb
approved these changes
Mar 3, 2026
AviAvni
approved these changes
Mar 4, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores