Staging #915
Staging #915
Conversation
…and improve input handling for infinite values. Adjust button click behavior to prevent negative values.
… persistence for timeout and limit values. Initialize state from localStorage in LoginVerification and update localStorage on timeout and limit adjustments in QuerySettings.
…and schema API routes
…rdb-browser into fix-vulnerability
…QuerySettings component. Add methods for decreasing limits and timeouts, and update tests to validate behavior for negative values and button interactions.
… with other tests
Fix #887 Implement role-based query handling for Read-Only users across graph and schema API routes
fix api vulnerability
Fix #898 Enhance QuerySettings component with tooltips for Timeout and Limit
Fix #901 fix duplicate
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces extensive architectural and UI refactoring across the application. Major changes include a new context-driven state management system for both graph and schema entities, enabling multi-selection and batch operations. The user interface is overhauled with improved accessibility, testability (via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Context
participant API
participant DB
User->>UI: Interacts (select, add, delete, edit)
UI->>Context: Reads/Writes state (graph, schema, selection)
UI->>API: Sends request (CRUD, run query, duplicate, etc.)
API->>Context: Extracts user/session info
API->>DB: Executes query (roQuery if Read-Only, else query)
DB-->>API: Returns result
API-->>UI: Responds with data/status
UI->>Context: Updates state (elements, categories, labels)
UI-->>User: Updates view, shows toast/feedback
sequenceDiagram
participant Test
participant POM
participant UI
participant API
Test->>POM: Calls high-level method (e.g., addGraph, deleteNode)
POM->>UI: Locates element via test ID, performs action
UI->>API: Sends request if needed
API-->>UI: Responds
UI-->>POM: Updates DOM/state
POM-->>Test: Returns result/assertion
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
app/api/schema/[schema]/[node]/route.ts (1)
52-67:⚠️ Potential issueConsider handling read-only access differently in the DELETE handler
Similar to the POST handler, the DELETE handler conditionally uses
graph.roQueryfor read-only users, which is contradictory to deletion operations.Read-only users should be explicitly prevented from performing delete operations. Consider modifying the code:
-const result = user.role === "Read-Only" - ? await graph.roQuery(query, { params: { nodeId } }) - : await graph.query(query, { params: { nodeId } }) +if (user.role === "Read-Only") { + return NextResponse.json( + { error: "Operation not permitted for read-only users" }, + { status: 403 } + ) +} +const result = await graph.query(query, { params: { nodeId } })app/api/schema/[schema]/route.ts (1)
66-70:⚠️ Potential issueMissing role-based access control in DELETE handler
Unlike the GET and POST handlers, the DELETE handler doesn't implement the role-based permission check. This could allow "Read-Only" users to delete schemas, which contradicts the purpose of having a read-only role.
- const { client } = session + const { client, user } = session const { schema } = await params const schemaName = `${schema}_schema` try { + if (user.role === "Read-Only") { + throw new Error("Operation not permitted for read-only users") + } const graph = client.selectGraph(schemaName)
🧹 Nitpick comments (2)
next.config.js (1)
8-40: Strong security headers implementation.Adding these security headers is an excellent security practice. The configuration includes essential protections like Content-Security-Policy, HSTS, X-Frame-Options, and others.
Consider expanding the Content-Security-Policy beyond just frame-ancestors to also control script, style, and other resource loading for comprehensive protection.
Example of a more comprehensive CSP:
- value: "frame-ancestors 'none';" + value: "default-src 'self'; script-src 'self'; style-src 'self'; frame-ancestors 'none'; img-src 'self' data:; connect-src 'self';"app/settings/QuerySettings.tsx (1)
18-31: Enhanced UI with informative tooltips.Adding tooltips with explanatory text and documentation links improves the user experience by providing context for the settings.
One minor issue in the tooltip text:
- <p>Shows a `Timed Out` error if the query takes longer than the timeout in seconds.</p> + <p>Shows a "Timed Out" error if the query takes longer than the timeout in seconds.</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/api/graph/[graph]/[node]/[key]/route.ts(4 hunks)app/api/graph/[graph]/[node]/label/route.ts(4 hunks)app/api/graph/[graph]/[node]/route.ts(3 hunks)app/api/graph/[graph]/count/route.ts(1 hunks)app/api/graph/[graph]/duplicate/route.tsx(1 hunks)app/api/graph/[graph]/route.ts(1 hunks)app/api/graph/[graph]/suggestions/route.ts(2 hunks)app/api/schema/[schema]/[node]/[key]/route.ts(4 hunks)app/api/schema/[schema]/[node]/label/route.ts(3 hunks)app/api/schema/[schema]/[node]/route.ts(4 hunks)app/api/schema/[schema]/count/route.ts(1 hunks)app/api/schema/[schema]/duplicate/route.tsx(1 hunks)app/api/schema/[schema]/route.ts(4 hunks)app/components/graph/DeleteGraph.tsx(1 hunks)app/components/ui/combobox.tsx(1 hunks)app/graph/Duplicate.tsx(1 hunks)app/settings/QuerySettings.tsx(6 hunks)e2e/logic/POM/graphPage.ts(2 hunks)e2e/logic/POM/settingsQueryPage.ts(3 hunks)e2e/tests/queryHistory.spec.ts(1 hunks)e2e/tests/settingsQuery.spec.ts(2 hunks)next.config.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (17)
app/api/schema/[schema]/count/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/graph/[graph]/[node]/label/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/graph/[graph]/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/schema/[schema]/[node]/[key]/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/graph/[graph]/duplicate/route.tsx (2)
app/api/schema/[schema]/duplicate/route.tsx (1)
PATCH(5-28)app/api/auth/[...nextauth]/options.ts (2)
session(149-167)getClient(171-200)
app/api/graph/[graph]/[node]/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/graph/[graph]/[node]/[key]/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/graph/[graph]/suggestions/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
e2e/logic/POM/graphPage.ts (1)
e2e/infra/utils.ts (1)
interactWhenVisible(118-122)
app/graph/Duplicate.tsx (1)
lib/utils.ts (2)
securedFetch(42-66)prepareArg(68-70)
app/api/schema/[schema]/duplicate/route.tsx (3)
app/api/graph/[graph]/duplicate/route.tsx (1)
PATCH(5-26)app/api/schema/[schema]/route.ts (1)
PATCH(83-109)app/api/auth/[...nextauth]/options.ts (2)
session(149-167)getClient(171-200)
app/api/graph/[graph]/count/route.ts (1)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)
app/api/schema/[schema]/[node]/route.ts (2)
app/api/auth/[...nextauth]/options.ts (1)
session(149-167)app/api/schema/[schema]/[node]/utils.ts (1)
formatAttributes(10-10)
app/components/ui/combobox.tsx (1)
lib/utils.ts (2)
securedFetch(42-66)prepareArg(68-70)
app/settings/QuerySettings.tsx (1)
lib/utils.ts (1)
cn(38-40)
e2e/logic/POM/settingsQueryPage.ts (1)
e2e/infra/utils.ts (1)
interactWhenVisible(118-122)
app/api/schema/[schema]/route.ts (4)
app/api/auth/[...nextauth]/options.ts (2)
session(149-167)getClient(171-200)app/api/schema/[schema]/duplicate/route.tsx (1)
PATCH(5-28)app/api/schema/[schema]/[node]/[key]/route.ts (1)
PATCH(5-37)app/api/graph/[graph]/route.ts (1)
PATCH(71-95)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build
🔇 Additional comments (48)
e2e/logic/POM/graphPage.ts (2)
13-19: Well-structured locator implementation for navigation buttons.The new button locators follow the established pattern in this class, using XPath selectors to find elements by their text content.
269-275: Good implementation of navigation methods.These new methods properly use the
interactWhenVisibleutility to ensure elements are visible before interaction, consistent with the class's existing patterns.app/settings/QuerySettings.tsx (6)
2-5: Good import organization.Added imports for required UI components and utility functions.
42-55: Improved input handling for timeout.The input now correctly handles the infinity symbol and empty values. The use of the
cnutility for class composition is also a good practice.
60-60: Prevents negative timeout values.The updated onClick handler prevents the timeout from going below zero, which is a good validation.
67-80: Consistent UI pattern for limit settings.The tooltip implementation for the limit setting is consistent with the timeout section, providing a good user experience.
91-104: Improved input handling for limit.Similar to the timeout input, this correctly handles the infinity symbol and empty values.
109-109: Prevents negative limit values.The updated onClick handler prevents the limit from going below zero, providing proper validation.
e2e/tests/queryHistory.spec.ts (1)
3-3: Import reordering.Moving the BATCH_CREATE_PERSONS import is a minor change that doesn't affect functionality. This reordering may improve readability or follow a style convention.
app/api/graph/[graph]/count/route.ts (1)
12-12: Added role-based query execution - good security practice!The changes correctly implement role-based access control by extracting the user's role from the session and conditionally using either the read-only query method or the standard query method. This is a good security enhancement that ensures users with "Read-Only" permissions can only perform non-mutating operations.
Also applies to: 18-20
app/api/graph/[graph]/route.ts (1)
52-52: Added role-based query execution to POST handler - good consistencyThis change properly extends the role-based query execution pattern to the POST handler, maintaining consistency with other route handlers. The implementation correctly checks the user's role and uses the appropriate query method.
However, since this is a POST handler that might typically be used for creating resources, consider whether read-only users should be attempting this operation at all. While the current implementation safely uses
roQueryfor read-only users, you might want to return an error for read-only users attempting to access POST endpoints.Also applies to: 58-60
app/api/graph/[graph]/suggestions/route.ts (1)
12-12: Implemented role-based query execution - good security practiceThe changes correctly implement role-based access control by extracting the user's role from the session and conditionally using either the read-only query method or the standard query method based on that role. This is a good security enhancement that ensures users with "Read-Only" permissions can only perform non-mutating operations.
Also applies to: 35-37
app/api/schema/[schema]/count/route.ts (1)
12-12: Added role-based query execution - good security practiceThe implementation correctly uses role-based access control by extracting the user's role from the session and conditionally using either the read-only query method or the standard query method. This ensures users with "Read-Only" permissions can only perform non-mutating operations, which is a good security practice.
Also applies to: 19-21
app/api/schema/[schema]/[node]/[key]/route.ts (4)
12-12: Role-based access control implementation looks good.The extraction of both
clientanduserfrom the session is correctly implemented.
26-28: Good implementation of role-based query execution.This conditional logic correctly restricts users with "Read-Only" role to using the read-only query method (
roQuery), preventing them from making mutations to the database. This is a good security practice.
46-46: Consistent extraction of user role.The extraction of both
clientanduserfrom the session is consistent with the PATCH handler above.
58-60: Well-implemented role-based query execution.This conditional query execution correctly enforces read-only restrictions for users with the "Read-Only" role, consistent with the implementation in the PATCH handler.
app/api/graph/[graph]/[node]/[key]/route.ts (4)
11-11: Properly extracting user with role information.The destructuring now correctly extracts both the client and user from the session.
27-29: Good implementation of role-based query execution.This conditional logic properly handles parameterized queries based on user role. For "Read-Only" users, it correctly uses
roQueryto prevent mutations.
46-46: Consistent extraction of user role.The extraction of both
clientanduserfrom the session is consistent with the POST handler implementation.
61-63: Well-implemented parameterized query with role-based access control.The conditional execution based on user role is correctly implemented, ensuring that "Read-Only" users can't perform mutation operations while maintaining the proper parameterization.
app/api/schema/[schema]/[node]/label/route.ts (4)
11-11: Properly extracting user with role information.The destructuring correctly extracts both the client and user from the session.
21-23: Good implementation of role-based query execution.The conditional logic correctly determines which query method to use based on the user's role, enhancing security by preventing "Read-Only" users from making mutations.
41-41: Consistent extraction of user role.The extraction of both
clientanduserfrom the session is consistently implemented across handlers.
52-54: Well-implemented role-based query execution.The conditional query execution correctly enforces read-only restrictions for users with the "Read-Only" role.
app/api/graph/[graph]/[node]/label/route.ts (4)
12-12: Properly extracting user with role information.The destructuring now correctly extracts both the client and user from the session.
22-24: Good implementation of role-based query execution with parameters.This conditional logic properly handles parameterized queries based on user role, ensuring "Read-Only" users can't perform mutations while maintaining the proper query parameters.
43-43: Consistent extraction of user role.The extraction of both
clientanduserfrom the session is consistent with the DELETE handler implementation.
53-55: Well-implemented parameterized query with role-based access control.The conditional execution based on user role is correctly implemented, ensuring that "Read-Only" users can't perform mutation operations.
app/components/ui/combobox.tsx (2)
60-60: Update correctly handles dynamic endpoint selectionThe API URL now dynamically selects the appropriate endpoint based on the type prop, enhancing reusability of the component.
60-65:❌ Incorrect review comment
Missing request body in PATCH request
The PATCH request is missing a request body, which might be required by the backend API. Verify that the endpoint doesn't expect a body parameter.
🏁 Script executed:
#!/bin/bash # Check if the API handlers for graph/schema rename expect a body echo "Checking API handlers for graph and schema rename operations..." fd -t f "route.ts" app/api/graph app/api/schema | xargs grep -l "PATCH" | xargs catLength of output: 13971
No request body needed for PATCH rename endpoints
The Graph and Schema PATCH handlers in app/api/graph/[graph]/route.ts and app/api/schema/[schema]/route.ts both read
sourceNamefromrequest.nextUrl.searchParamsand do not callrequest.json(). No JSON body is expected or used—yoursecuredFetchcall is correct as-is.Likely an incorrect or invalid review comment.
app/components/graph/DeleteGraph.tsx (1)
42-42: Improved selection handling after deletionThe updated condition properly checks if the selected value is still present in the updated list of options before potentially changing it.
app/api/graph/[graph]/[node]/route.ts (1)
12-12: GET handler correctly implements role-based query executionThe GET handler properly extracts the user's role from the session and conditionally uses the appropriate query method based on the role.
Also applies to: 24-26
e2e/tests/settingsQuery.spec.ts (4)
27-27: Method name update looks goodThe change from
addTimeouttofillTimeoutbetter reflects the action of setting a specific timeout value.
43-43: Method name update looks goodThe change from
addLimittofillLimitbetter reflects the action of setting a specific limit value.
51-51: Good addition of settings navigationAdding
clickOnSettings()after verifying limited query results provides a cleaner test flow and properly closes the interaction cycle.
54-97: Well-structured boundary testsThese new tests comprehensively cover important edge cases for limit and timeout settings:
- Negative value handling via direct input
- Negative value handling via decrease buttons
- Value increment behavior via increase buttons
The tests follow a consistent pattern and provide good coverage of UI interactions.
app/api/graph/[graph]/duplicate/route.tsx (1)
1-26: Well-implemented duplication endpointThe new PATCH handler for graph duplication follows good practices:
- Proper authentication via
getClient()- Clear parameter extraction and validation
- Good error handling with appropriate status codes
- Consistent pattern with similar endpoints
app/graph/Duplicate.tsx (1)
28-30: API endpoint update looks goodThe change to use PATCH endpoints for duplication is well-implemented:
- Properly handles both graph and schema types
- Uses URL encoding through
prepareArg- Passes source name as a query parameter
- Aligns with the new backend endpoints
app/api/schema/[schema]/duplicate/route.tsx (2)
1-28: Well-implemented schema duplication endpointThe PATCH handler for schema duplication is consistently implemented:
- Proper authentication via
getClient()- Clear parameter extraction and validation
- Appropriate schema name handling with "_schema" suffix
- Good error handling with appropriate status codes
19-21: Consistent schema naming conventionGood implementation of the "_schema" suffix for both source and target schema names, maintaining consistency with the application's naming conventions.
app/api/schema/[schema]/route.ts (2)
11-11: Well-implemented role-based query executionThe GET handler now correctly extracts the user role from the session and conditionally executes a read-only query for users with the "Read-Only" role. This ensures proper access control at the query level.
Also applies to: 22-24
40-40: Consistent implementation of role-based access controlThe POST handler follows the same pattern as the GET handler, ensuring read-only users can only execute non-mutating queries. This maintains a consistent security model throughout the API.
Also applies to: 46-48
e2e/logic/POM/settingsQueryPage.ts (5)
23-29: Well-structured locators for decrement buttonsThese locators follow the same pattern as the existing increment button locators, making the code consistent and maintainable.
39-45: Good implementation of click methods for decrement buttonsThese methods properly use the
interactWhenVisibleutility to ensure elements are visible before interaction, following the established pattern in the class.
55-61: Clear and concise fill methodsThe
fillLimitandfillTimeoutmethods provide a clean API for setting numeric values in the inputs, simplifying the test code.
63-77: Well-designed wrapper methods for interactionsThese wrapper methods create a more semantic API for tests, making test code more readable by providing clear intent through method names rather than implementation details.
79-87: Useful getter methods for assertionsThe
getLimitandgetTimeoutmethods will be valuable for assertions in tests, allowing verification of input values after interactions.
app/components/graph/DeleteGraph.tsx
Outdated
|
|
||
| setOpen(false) | ||
| setOpenMenage(false) | ||
| handleSetRows(options.filter(opt => !opts.includes(opt))) | ||
| handleSetRows(options.filter(opt => newNames.includes(opt))) |
There was a problem hiding this comment.
Potential logic issue with filtered rows
The update changes the filtering logic to use options.filter(opt => newNames.includes(opt)) instead of filtering based on deleted items. This logic might be incorrect since options contains the original list of options before deletion.
Consider correcting the logic to use newNames directly instead of filtering the original options:
-handleSetRows(options.filter(opt => newNames.includes(opt)))
+handleSetRows(newNames)Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| if (!sourceName) throw new Error("Missing parameter sourceName") | ||
|
|
||
| const result = await client.selectGraph(sourceName).copy(graphId) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding validation for target graph name
The current implementation validates the source graph name but doesn't check if the target graph already exists. This could potentially lead to overwriting an existing graph.
🏁 Script executed:
#!/bin/bash
# Check if there's any validation in the client.selectGraph().copy() method
# that prevents overwriting existing graphs
# Find any graph copy implementation in the codebase
rg -A 5 -B 5 "\.copy\(" --type tsLength of output: 1785
Add validation for target graph name to prevent overwriting
Ensure you validate the graphId parameter and check that a graph with this name doesn’t already exist before calling .copy().
• app/api/graph/[graph]/duplicate/route.tsx (around lines 16–19):
– Throw if graphId is missing
– Query the client to see if a graph with graphId already exists and error out if so
Example diff:
try {
- if (!sourceName) throw new Error("Missing parameter sourceName")
-
- const result = await client.selectGraph(sourceName).copy(graphId)
+ if (!sourceName) throw new Error("Missing parameter sourceName")
+
+ const graphId = request.nextUrl.searchParams.get("graphId")
+ if (!graphId) throw new Error("Missing parameter graphId")
+
+ // Prevent overwriting an existing graph
+ const exists = await client
+ .selectGraph(graphId)
+ .get()
+ .then(() => true)
+ .catch(err => err.message.includes("NotFound") ? false : Promise.reject(err))
+ if (exists) throw new Error(`Graph "${graphId}" already exists`)
+
+ const result = await client.selectGraph(sourceName).copy(graphId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| if (!sourceName) throw new Error("Missing parameter sourceName") | |
| const result = await client.selectGraph(sourceName).copy(graphId) | |
| try { | |
| if (!sourceName) throw new Error("Missing parameter sourceName") | |
| const graphId = request.nextUrl.searchParams.get("graphId") | |
| if (!graphId) throw new Error("Missing parameter graphId") | |
| // Prevent overwriting an existing graph | |
| const exists = await client | |
| .selectGraph(graphId) | |
| .get() | |
| .then(() => true) | |
| .catch(err => err.message.includes("NotFound") ? false : Promise.reject(err)) | |
| if (exists) throw new Error(`Graph "${graphId}" already exists`) | |
| const result = await client.selectGraph(sourceName).copy(graphId) |
| export async function PATCH(request: NextRequest, { params }: { params: Promise<{ schema: string }> }) { | ||
|
|
||
| const session = await getClient() | ||
| if (session instanceof NextResponse) { | ||
| return session | ||
| } | ||
|
|
||
| const { client } = session | ||
|
|
||
| const { schema } = await params | ||
| const schemaName = `${schema}_schema` | ||
| const source = request.nextUrl.searchParams.get("sourceName") | ||
|
|
||
| try { | ||
| if (!source) throw new Error("Missing parameter sourceName") | ||
|
|
||
| const sourceName = `${source}_schema` | ||
| const data = await (await client.connection).renameNX(sourceName, schemaName); | ||
|
|
||
| if (!data) throw new Error(`${schema} already exists`) | ||
|
|
||
| return NextResponse.json({ data }) | ||
| } catch (err: unknown) { | ||
| console.error(err) | ||
| return NextResponse.json({ message: (err as Error).message }, { status: 400 }) | ||
| } | ||
| } |
There was a problem hiding this comment.
PATCH handler needs role-based access control
The newly added PATCH handler for renaming schemas doesn't implement the role-based permission check that exists in the GET and POST handlers. "Read-Only" users should not be able to rename schemas.
- const { client } = session
+ const { client, user } = session
const { schema } = await params
const schemaName = `${schema}_schema`
const source = request.nextUrl.searchParams.get("sourceName")
try {
+ if (user.role === "Read-Only") {
+ throw new Error("Operation not permitted for read-only users")
+ }
if (!source) throw new Error("Missing parameter sourceName")Additionally, the error message on line 102 should use schemaName instead of schema for consistency with how the code refers to the schema with its _schema suffix:
- if (!data) throw new Error(`${schema} already exists`)
+ if (!data) throw new Error(`${schemaName} already exists`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function PATCH(request: NextRequest, { params }: { params: Promise<{ schema: string }> }) { | |
| const session = await getClient() | |
| if (session instanceof NextResponse) { | |
| return session | |
| } | |
| const { client } = session | |
| const { schema } = await params | |
| const schemaName = `${schema}_schema` | |
| const source = request.nextUrl.searchParams.get("sourceName") | |
| try { | |
| if (!source) throw new Error("Missing parameter sourceName") | |
| const sourceName = `${source}_schema` | |
| const data = await (await client.connection).renameNX(sourceName, schemaName); | |
| if (!data) throw new Error(`${schema} already exists`) | |
| return NextResponse.json({ data }) | |
| } catch (err: unknown) { | |
| console.error(err) | |
| return NextResponse.json({ message: (err as Error).message }, { status: 400 }) | |
| } | |
| } | |
| export async function PATCH( | |
| request: NextRequest, | |
| { params }: { params: Promise<{ schema: string }> } | |
| ) { | |
| const session = await getClient() | |
| if (session instanceof NextResponse) { | |
| return session | |
| } | |
| const { client, user } = session | |
| const { schema } = await params | |
| const schemaName = `${schema}_schema` | |
| const source = request.nextUrl.searchParams.get("sourceName") | |
| try { | |
| if (user.role === "Read-Only") { | |
| throw new Error("Operation not permitted for read-only users") | |
| } | |
| if (!source) throw new Error("Missing parameter sourceName") | |
| const sourceName = `${source}_schema` | |
| const data = await (await client.connection).renameNX(sourceName, schemaName) | |
| if (!data) throw new Error(`${schemaName} already exists`) | |
| return NextResponse.json({ data }) | |
| } catch (err: unknown) { | |
| console.error(err) | |
| return NextResponse.json( | |
| { message: (err as Error).message }, | |
| { status: 400 } | |
| ) | |
| } | |
| } |
add tutorial tests (#879)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (14)
app/schema/page.tsx (3)
75-82: Still mutating React state array in place (duplicate of earlier review)
stateSelectedElements.push(selectedElement)mutates the original array becauseObject.valuesreturns the same reference for arrays.
This breaks immutability and can cause missed renders.-const stateSelectedElements = Object.values(selectedElements) +let stateSelectedElements = [...selectedElements] // clone ... -stateSelectedElements.push(selectedElement) +stateSelectedElements = [...stateSelectedElements, selectedElement]
83-100:schema.removeLinksstill fed with staleselectedElements(duplicate)After computing
stateSelectedElements, the call underneath still uses the oldselectedElements, so links for the additionally-pushed element are never removed. Use the updated list.-schema.removeLinks(setLabels, selectedElements.map((element) => element.id)) +schema.removeLinks(setLabels, stateSelectedElements.map((element) => element.id))
83-100: No error handling insidePromise.allloop (duplicate)Failures in any DELETE request silently fail and stop subsequent clean-up logic from running. Wrap the fetch in
try/catchor checkresult.okand surface errors to the user.app/graph/toolbar.tsx (2)
50-55: Null/undefined values inel.datacan crash the search filter
Object.values(el.data).some(value => value.toString().toLowerCase()...)will throw a TypeError when a property is null or undefined, aborting the whole search.-Object.values(el.data).some(value => value.toString().toLowerCase().startsWith(searchElement.toLowerCase())) +Object.values(el.data).some(value => + value !== null && value !== undefined && + String(value).toLowerCase().startsWith(searchElement.toLowerCase()) +)Using
String(value)is both safe and concise.
60-66: ResetsuggestionIndexwhen the suggestions list changesIf the previous index is larger than the new list's length,
suggestions[suggestionIndex]becomesundefined, breaking Enter and arrow-navigation.useEffect(() => { - const timeout = setTimeout(handleOnChange, 500) + const timeout = setTimeout(() => { + handleOnChange() + setSuggestionIndex(0) // <- keep index in range + }, 500) return () => { clearTimeout(timeout) } }, [graph, handleOnChange, searchElement])app/graph/page.tsx (2)
115-126:handleCooldowntreatsundefinedas "running"When called with no argument (e.g., after
runQuery),ticksisundefined, yet the else-branch flags the canvas as running. Pass an explicit 0 or guard forundefinedto reflect the real engine state.- if (ticks === 0) { + if (ticks === undefined || ticks === 0) { canvas.setAttribute('data-engine-status', 'stop') } else { canvas.setAttribute('data-engine-status', 'running') }
154-158:⚠️ Potential issueState array mutated in place – breaks React invariants
selectedElements.push(selectedElement)alters the current state array without using the setter. This may not trigger rerenders and causes unpredictable behavior.- selectedElements.push(selectedElement) + const toDelete = [...selectedElements, selectedElement] + setSelectedElements(toDelete)Remember to use the new
toDeletereference throughout the rest of the function.app/graph/GraphDataPanel.tsx (2)
83-87: Update categories immutably after adding a labelAfter adding a category to the graph, the state should be updated immutably.
graph.addCategory(newLabel, node) -setCategories([...graph.Categories]) -setLabel([...node.category]) +setCategories(prevCategories => { + const updatedCategories = [...prevCategories] + const newCategoryIndex = graph.Categories.findIndex(c => c.name === newLabel) + if (newCategoryIndex !== -1) { + updatedCategories.push({...graph.Categories[newCategoryIndex]}) + } + return updatedCategories +}) +setLabel(prevLabels => [...prevLabels, newLabel])
104-107: Update categories immutably after removing a labelSimilar to the add label issue, removing a label should update state immutably.
graph.removeCategory(removeLabel, node) -setCategories([...graph.Categories]) -setLabel([...node.category]) +setCategories(prevCategories => + prevCategories.filter(c => c.name !== removeLabel) +) +setLabel(prevLabels => prevLabels.filter(l => l !== removeLabel))app/graph/GraphView.tsx (3)
190-202: Tight coupling between data layer and React stateThe ForceGraph component receives
setLabelsdirectly, which tightly couples the graph data layer to React state management. This makes it harder to maintain separation of concerns and could lead to unexpected side effects.Consider introducing a higher-level API that abstracts the state updates:
<ForceGraph graph={graph} chartRef={chartRef} data={data} setData={setData} selectedElement={selectedElement} setSelectedElement={setSelectedElement} selectedElements={selectedElements} setSelectedElements={setSelectedElements} cooldownTicks={cooldownTicks} handleCooldown={handleCooldown} - setLabels={setLabels} + onLabelsChange={(newLabels) => setLabels(newLabels)} />Then, modify the ForceGraph component to call this callback rather than directly updating state.
97-113: 🛠️ Refactor suggestionState mutation breaks React immutability principles
The
onCategoryClickfunction directly mutates the category object and its elements. This can lead to unpredictable behavior in React since it violates React's immutability principles for state management.Consider using an immutable approach:
const onCategoryClick = (category: Category<Node>) => { - category.show = !category.show + const updatedCategory = { ...category, show: !category.show } - category.elements.forEach((node) => { + const updatedElements = category.elements.map((node) => { if (node.category[0] !== category.name) return node if (category.show) { - node.visible = true + return { ...node, visible: true } } else { - node.visible = false + return { ...node, visible: false } } + return node }) + updatedCategory.elements = updatedElements graph.visibleLinks(category.show) - graph.CategoriesMap.set(category.name, category) + graph.CategoriesMap.set(category.name, updatedCategory) setData({ ...graph.Elements }) }
115-128: 🛠️ Refactor suggestionDirect mutations in onLabelClick function
Similar to
onCategoryClick, this function directly mutates label objects, violating React's immutability principles.const onLabelClick = (label: Category<Link>) => { - label.show = !label.show + const updatedLabel = { ...label, show: !label.show } - label.elements.filter((link) => link.source.visible && link.target.visible).forEach((link) => { + const updatedElements = label.elements.map((link) => { + if (!link.source.visible || !link.target.visible) return link if (label.show) { - link.visible = true + return { ...link, visible: true } } else { - link.visible = false + return { ...link, visible: false } } + return link }) + updatedLabel.elements = updatedElements - graph.LabelsMap.set(label.name, label) + graph.LabelsMap.set(label.name, updatedLabel) setData({ ...graph.Elements }) }app/schema/SchemaView.tsx (2)
92-103: 🛠️ Refactor suggestionEnsure immutable state updates for categories
The
onCategoryClickfunction directly mutates the category object and updates nodes in place. This violates React's immutability principles and can lead to unexpected behaviors.const onCategoryClick = (category: Category<Node>) => { - category.show = !category.show + const updatedCategory = { ...category, show: !category.show } - schema.Elements.nodes.forEach((node) => { + const updatedNodes = schema.Elements.nodes.map((node) => { if (node.category[0] !== category.name) return node - node.visible = category.show + return { ...node, visible: updatedCategory.show } }) + schema.Elements.nodes = updatedNodes - schema.visibleLinks(category.show) + schema.visibleLinks(updatedCategory.show) - schema.CategoriesMap.set(category.name, category) + schema.CategoriesMap.set(category.name, updatedCategory) setData({ ...schema.Elements }) }
105-115: 🛠️ Refactor suggestionEnsure immutable state updates for labels
Similar to the category issue, the
onLabelClickfunction directly mutates label objects. Apply immutable state update patterns for consistency with React's state management principles.const onLabelClick = (label: Category<Link>) => { - label.show = !label.show + const updatedLabel = { ...label, show: !label.show } - schema.Elements.links.forEach((link) => { + const updatedLinks = schema.Elements.links.map((link) => { if (link.label !== label.name) return link - link.visible = label.show + return { ...link, visible: updatedLabel.show } }) + schema.Elements.links = updatedLinks - schema.LabelsMap.set(label.name, label) + schema.LabelsMap.set(label.name, updatedLabel) setData({ ...schema.Elements }) }
🧹 Nitpick comments (26)
app/graph/RemoveLabel.tsx (2)
19-19: Add title attribute for improved accessibility.The empty title attribute on the button should be populated with a meaningful description.
- title="" + title="Remove selected label"
43-66: Add data-testid attributes for testing.The component has good structure but lacks test IDs for automated testing, which would be consistent with other components in the codebase.
<DialogComponent + data-testid="removeLabelDialog" title="Remove Label" trigger={trigger} open={open} onOpenChange={setOpen} description="Are you sure you want to remove this label?" > <div className="flex gap-4 justify-end"> <Button + data-testid="confirmRemoveLabel" variant="Delete" label="Remove Label" title="" onClick={handleRemoveLabel} isLoading={isLoading} indicator={indicator} />app/graph/addLabel.tsx (3)
18-18: Add title attribute for improved accessibility.The empty title attribute on the button should be populated with a meaningful description.
- title="" + title="Add a new label"
53-63: Consider auto-focusing with a safer approach.The current implementation uses a ref callback to focus the input, which works but doesn't account for potential rendering scenarios where focusing might fail or be undesirable (like mobile devices).
<Input data-testid="addLabelInput" variant="primary" - ref={ref => ref?.focus()} + autoFocus value={label} onChange={(e) => setLabel(e.target.value)} />
64-78: Add data-testid attributes for testing.The component has a test ID for the input field but lacks them for buttons, which would be helpful for consistent automated testing.
<div className="flex gap-4 justify-end"> <Button + data-testid="confirmAddLabel" indicator={indicator} variant="Primary" label="Add label" title="Add a new label" type="submit" isLoading={isLoading} /> <CloseDialog + data-testid="cancelAddLabel" variant="Cancel" label="Cancel" type="button" /> </div>app/components/ui/combobox.tsx (1)
4-7: Clean up unused Dialog import.The component imports Dialog but only uses it for openMenage state, which doesn't appear to be fully utilized in the component.
-import { Dialog } from "@/components/ui/dialog" import { cn } from "@/lib/utils" import { useContext, useEffect, useState } from "react" import { Select, SelectContent, SelectGroup, SelectItem, SelectTrigger, SelectValue } from "@/components/ui/select"app/components/PaginationList.tsx (2)
59-59: Make search input test ID dynamic.The search input uses a fixed test ID "queryHistorySearch" regardless of component purpose. Consider making this dynamic based on the component's context.
- data-testid="queryHistorySearch" + data-testid={`${dataTestId}Search`}
106-128: Add comments for complex pagination logic.The pagination calculation logic is complex and would benefit from explanatory comments to make it more maintainable.
{ + // Calculate which page numbers to display + // Shows a window of pages centered around current page + // with appropriate bounds handling Array(pageCount).fill(0).map((_, index) => index) .slice( Math.max(0, Math.min( stepCounter - 1, pageCount - 3 )), Math.min( Math.max(3, stepCounter + 2), pageCount ) ) .map((index) => (app/graph/GraphDataTable.tsx (3)
95-97:isSetLoadingnever resets for “add” actionsBecause the flag is toggled only when
actionType === "set", it staysfalseduring an “add” request, so the confirm button never shows a spinner.
Move the loading toggle out of the conditional or introduce a separateisAddLoadingflag inside this helper.
17-23:type: booleanis cryptic – use a discriminated union or enum
true⇢ Node,false⇢ Link is not self-documenting and invites misuse. At minimum:type ElementKind = "node" | "link"; ... type: ElementKind;This also gives better IntelliSense & compile-time safety.
151-162: Keyboard handlers: prevent default to avoid accidental form submitsWhen the user presses Enter you call
handleAddValue/setProperty, but the event may still bubble and submit an ancestor<form>.
Adde.preventDefault()after detecting the key.Also applies to: 170-173
app/graph/selectGraph.tsx (2)
131-134: Dialog title is hard-coded to “Manage Graphs”When
type === "Schema"the modal still reads “Manage Graphs”, confusing users.-<DialogTitle className="text-2xl font-medium">Manage Graphs</DialogTitle> +<DialogTitle className="text-2xl font-medium">{`Manage ${type}s`}</DialogTitle>
38-40: Typo:openMenage→openManageMinor naming typo that spreads through the file; fix for readability & consistency.
app/graph/toolbar.tsx (1)
93-127: Reduce duplication by extracting keyboard navigation logicThe same keyboard event handling code is duplicated between the input and suggestion list elements. Extract this into a reusable function to improve maintainability.
+const handleKeyNavigation = (e: React.KeyboardEvent) => { + if (e.key === 'Escape') { + e.preventDefault() + setSearchElement("") + } + + if (e.key === 'Enter' && suggestions[suggestionIndex]) { + e.preventDefault() + handleSearchElement(suggestions[suggestionIndex]) + setSearchElement("") + } + + if (e.key === 'ArrowDown') { + e.preventDefault() + const index = suggestionIndex === suggestions.length - 1 ? 0 : suggestionIndex + 1 + setSuggestionIndex(index) + scrollToSuggestion(index) + } + + if (e.key === 'ArrowUp') { + e.preventDefault() + const index = suggestionIndex === 0 ? suggestions.length - 1 : suggestionIndex - 1 + setSuggestionIndex(index) + scrollToSuggestion(index) + } +} // Then use this function in both onKeyDown handlers -onKeyDown={(e) => { /* existing code */ }} +onKeyDown={handleKeyNavigation}Also applies to: 137-162
app/graph/page.tsx (1)
197-214: Avoid duplicate cleanup operationsThe function sets
setSelectedElements([])andsetSelectedElement(undefined)twice - once at the beginning of the cleanup section and again at the end. This duplication is unnecessary.graph.removeElements(selectedElements) fetchCount() -setSelectedElements([]) -setSelectedElement(undefined) graph.removeLinks(setLabels, selectedElements.map((element) => element.id)) setData({ ...graph.Elements }) toast({ title: "Success", description: `${selectedElements.length > 1 ? "Elements" : "Element"} deleted`, }) handleCooldown() setSelectedElement(undefined) setSelectedElements([])app/components/EditorComponent.tsx (3)
19-42: Consider movingsetThemeto a separate utility fileThe
setThemefunction is exported but defined within this component file. Since it's a pure utility function that configures Monaco themes, consider moving it to a dedicated utilities file for better code organization.// New file: lib/editorUtils.ts import { Monaco } from "@monaco-editor/react" export const setTheme = (monacoI: Monaco, themeName: string, backgroundColor: string) => { // ... existing implementation } // Then import in EditorComponent.tsx import { setTheme } from "@/lib/editorUtils"
611-623: Simplify onChange handler with destructuring and conditionalThe current implementation uses two separate conditionals to update the historyQuery state. This can be simplified using destructuring and a conditional expression.
onChange={(val) => { - if (!historyQuery.counter) { - setHistoryQuery(prev => ({ - ...prev, - currentQuery: val || "", - query: val || "", - })) - } else { - setHistoryQuery(prev => ({ - ...prev, - query: val || "", - })) - } + setHistoryQuery(prev => { + const newQuery = val || ""; + return { + ...prev, + query: newQuery, + ...(prev.counter ? {} : { currentQuery: newQuery }) + }; + }); }}
682-696: Duplicate state update logic in editor onChange handlersThe same conditional logic for updating history state appears in both editors. Extract this to a shared function to avoid duplication and ensure consistent behavior.
+const handleEditorChange = (val: string | undefined) => { + setHistoryQuery(prev => { + const newQuery = val || ""; + return { + ...prev, + query: newQuery, + ...(!prev.counter ? { currentQuery: newQuery } : {}) + }; + }); +}; // Then use this in both onChange handlers -onChange={(val) => { /* existing code */ }} +onChange={handleEditorChange}app/graph/GraphView.tsx (1)
86-90: Check current tab value before triggering cooldownThe useEffect for tab changes will run
handleCooldown()every timetabsValuechanges to "Graph", even if it's already set to "Graph". This could lead to unnecessary cooldown operations.useEffect(() => { - if (tabsValue === "Graph") { + if (tabsValue === "Graph" && prevTabValue.current !== "Graph") { handleCooldown() } + prevTabValue.current = tabsValue }, [tabsValue])You'd need to add a ref to track the previous tab value:
const prevTabValue = useRef<string>("");app/schema/SchemaView.tsx (2)
78-81: Optimize dependency arrays in useEffect hooksThe dependency array includes
schema.Categories.lengthandschema.Labels.length, which could cause unnecessary re-renders if the arrays' contents change but their lengths remain the same.Consider using a more stable dependency or implementing a deep comparison for categories and labels:
useEffect(() => { setCategories([...schema.Categories]) setLabels([...schema.Labels]) -}, [schema.Id, schema.Categories.length, schema.Labels.length]) +}, [schema.Id])Or use a memo to track if the actual contents have changed.
117-147: Improved error handling for element creationThe
onCreateElementfunction doesn't handle potential errors ifschema.extendNodeorschema.extendEdgereturn undefined.const onCreateElement = async (attributes: [string, string[]][], elementLabel?: string[]) => { // ... if (result.ok) { const json = await result.json() if (isAddEntity) { - const { category } = schema.extendNode(json.result.data[0].n, false, true)! + const extendResult = schema.extendNode(json.result.data[0].n, false, true) + if (extendResult) { + const { category } = extendResult setCategories(prev => [...prev, ...category.map(c => schema.CategoriesMap.get(c)!)]) setIsAddEntity(false) + } } else { - const { label } = schema.extendEdge(json.result.data[0].e, false, true)! + const extendResult = schema.extendEdge(json.result.data[0].e, false, true) + if (extendResult) { + const { label } = extendResult setLabels(prev => [...prev, schema.LabelsMap.get(label)!]) setIsAddRelation(false) + } } // ... } }app/graph/Selector.tsx (5)
56-56: Potential fragility in type determinationThe type is determined based on the presence of certain props, which could be error-prone if the component is used in different contexts.
Consider making the type explicit:
- const type = runQuery && historyQuery && setHistoryQuery ? "Graph" : "Schema" + type Props = { + // ...existing props + type: "Graph" | "Schema" + } // Then remove the type definition from the component bodyThis makes the component's behavior more predictable and explicit.
144-152: Guard against null references in handleSubmitThe function uses non-null assertions (
!) which could lead to runtime errors if the props are undefined.const handleSubmit = async () => { + if (!runQuery || !historyQuery) return; try { setIsLoading(true) - await runQuery!(historyQuery!.query.trim()) + await runQuery(historyQuery.query.trim()) setQueriesOpen(false) } finally { setIsLoading(false) } }
102-116: Add error handling in focusEditorAtEndThe function directly manipulates the DOM without any error handling, which could cause issues if the editor structure changes.
const focusEditorAtEnd = () => { if (editorRef.current) { - editorRef.current.focus(); + try { + editorRef.current.focus(); - const model = editorRef.current.getModel(); + const model = editorRef.current.getModel(); if (model) { const lastLine = model.getLineCount(); const lastColumn = model.getLineMaxColumn(lastLine); editorRef.current.setPosition({ lineNumber: lastLine, column: lastColumn }); editorRef.current.revealPositionInCenter({ lineNumber: lastLine, column: lastColumn }); } + } catch (error) { + console.error("Error focusing editor:", error); + } } };
226-237: Optimize query history search callbackThe search callback in PaginationList has a condition that could be simplified for better readability.
afterSearchCallback={(newFilteredList) => { - if (newFilteredList.every(q => q.text !== historyQuery.query)) { + const currentQueryFound = newFilteredList.some(q => q.text === historyQuery.query); + if (!currentQueryFound) { setHistoryQuery(prev => ({ ...prev, counter: 0 })) } focusEditorAtEnd() }}
167-321: Extract complex conditional rendering into separate componentsThe component has complex conditional rendering logic that makes it hard to read and maintain. Consider breaking it down into smaller, more focused components.
For example, extract the query editor and history dialog into separate components:
// In a new file QueryEditor.tsx export function QueryEditor({ graph, maximize, setMaximize, runQuery, historyQuery, setHistoryQuery }) { // ... editor code } // In a new file QueryHistory.tsx export function QueryHistory({ open, onOpenChange, historyQuery, setHistoryQuery, currentQuery, fetchCount, handleSubmit, isLoading }) { // ... dialog code } // Then in Selector.tsx return ( <div className="z-20 absolute top-5 inset-x-24 h-[56px] flex flex-row gap-4 items-center"> <SelectGraph options={options} // ... other props /> { runQuery && historyQuery && setHistoryQuery ? ( <> <QueryEditor graph={graph} maximize={maximize} setMaximize={setMaximize} runQuery={runQuery} historyQuery={historyQuery} setHistoryQuery={setHistoryQuery} /> <QueryHistory open={queriesOpen} onOpenChange={setQueriesOpen} historyQuery={historyQuery} setHistoryQuery={setHistoryQuery} currentQuery={currentQuery} fetchCount={fetchCount} handleSubmit={handleSubmit} isLoading={isLoading} /> </> ) : ( <Toolbar // ... toolbar props /> ) } </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (16)
app/components/EditorComponent.tsx(8 hunks)app/components/PaginationList.tsx(1 hunks)app/components/ui/Button.tsx(4 hunks)app/components/ui/combobox.tsx(4 hunks)app/graph/GraphDataPanel.tsx(2 hunks)app/graph/GraphDataTable.tsx(1 hunks)app/graph/GraphView.tsx(1 hunks)app/graph/RemoveLabel.tsx(1 hunks)app/graph/Selector.tsx(4 hunks)app/graph/TableView.tsx(1 hunks)app/graph/addLabel.tsx(1 hunks)app/graph/page.tsx(4 hunks)app/graph/selectGraph.tsx(1 hunks)app/graph/toolbar.tsx(1 hunks)app/schema/SchemaView.tsx(2 hunks)app/schema/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/ui/Button.tsx
- app/graph/TableView.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/graph/toolbar.tsx (5)
app/components/ui/Button.tsx (1)
Props(11-20)app/api/graph/model.ts (3)
Graph(127-709)Node(45-55)Link(57-70)lib/utils.ts (3)
GraphRef(20-20)handleZoomToFit(132-144)cn(52-54)components/ui/tooltip.tsx (3)
Tooltip(34-34)TooltipTrigger(34-34)TooltipContent(34-34)app/graph/DeleteElement.tsx (1)
DeleteElement(22-80)
app/components/EditorComponent.tsx (4)
app/components/ui/Button.tsx (1)
Props(11-20)app/api/graph/model.ts (2)
Graph(127-709)HistoryQuery(9-14)app/components/provider.ts (1)
IndicatorContext(6-6)app/components/CloseDialog.tsx (1)
CloseDialog(13-29)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
🔇 Additional comments (20)
app/graph/RemoveLabel.tsx (3)
1-12: Clean, well-structured component interface.The component has a clear interface with appropriate props for handling label removal. The optional trigger pattern allows for flexible UI integration.
14-25: Strong default implementation with accessibility consideration.The default trigger button is properly configured with a descriptive label and disabled state when no label is selected, which prevents invalid operations.
31-41: Well-implemented async handler with proper state management.The handler correctly manages loading state and handles the async operation flow. The success check ensures the dialog only closes when the operation succeeds.
app/graph/addLabel.tsx (3)
1-12: Well-structured component for label addition.The component has a clean interface with appropriate props for handling label addition. The optional trigger pattern provides flexibility.
14-23: Good default implementation with clear visual cues.The default trigger button is properly configured with a descriptive label and visual indicator (Plus icon).
31-44: Well-implemented form submission handler.The handler correctly prevents default form submission, manages loading state, and properly handles the async flow. Good practice to clear the input and close dialog on success.
app/components/ui/combobox.tsx (7)
17-17: Type constraint is appropriately specific.Limiting the label prop to "Role" or "Type" provides clear type safety and prevents misuse of the component.
25-28: Good implementation of default props within the function signature.The component handles default props clearly in the function signature while still providing explicit defaults at the end of the file, which is good for documentation.
44-119: Well-structured select component with appropriate accessibility features.The component provides a robust implementation with tooltips for guidance, proper handling of disabled states, and test IDs for key elements.
64-64: Consistent conditional rendering for tooltip content.Good use of the indicator context to provide appropriate tooltip content based on server status.
99-116: Well-implemented pagination controls.The pagination controls provide clear user feedback and appropriate disabling of buttons when they're not applicable.
124-128: Good use of defaultProps for documentation.Even though defaults are defined in the function signature, the explicit defaultProps helps with documentation and type checking.
30-30:❌ Incorrect review comment
Verify if openMenage state is necessary.
The component defines an openMenage state that's passed to the Dialog component, but it doesn't appear to be set or used meaningfully in the component.
🏁 Script executed:
#!/bin/bash # Check if openMenage is used elsewhere in the codebase grep -r "setOpenMenage" --include="*.tsx" --include="*.ts" app/Length of output: 710
openMenage state is required for Dialog control
The
openMenageflag and its setter are indeed being used to manage the visibility of your Dialog:
- In
app/components/ui/combobox.tsx, you pass
<Dialog open={openMenage} onOpenChange={setOpenMenage}>
so the Dialog’s open/close is driven by this state.- In
app/components/graph/DeleteGraph.tsx, you receivesetOpenMenageas a prop and callsetOpenMenage(false)to close the Dialog after deletion.- Similarly, in
app/graph/selectGraph.tsx, the same pattern is used.Since both
openMenageandsetOpenMenageare wired into your Dialog components for open/close control, this state is necessary and should be kept.Likely an incorrect or invalid review comment.
app/components/PaginationList.tsx (4)
8-22: Well-defined generic interface with appropriate props.The component has a clean, generic interface that handles both string and Query items. The props provide all necessary functionality for pagination, selection, and customization.
42-53: Well-implemented debounced search with appropriate cleanup.The search implementation uses a timeout for debouncing and properly cleans up on unmount. The callback notification is a good pattern for parent components to react to filtered results.
138-142: Good use of defaultProps for documentation.The explicit defaultProps helps with documentation and type checking for optional properties.
129-132:❌ Incorrect review comment
Pagination logic might have edge cases.
The disabled conditions for next/previous buttons might have edge cases that need verification. For example, the "Next page" button is disabled when stepCounter > pageCount - 2, which means it's still enabled when there are no more pages.
🏁 Script executed:
#!/bin/bash # Test edge cases in pagination logic echo "Testing edge case where stepCounter = pageCount - 1" echo "Button should be disabled when stepCounter = pageCount - 1" echo "Current condition: disabled={stepCounter > pageCount - 2}" echo "This might mean the button is still enabled when at the last page" # Recommend thorough testing of these conditions: echo "1. When there are 0 or 1 pages total" echo "2. When at the first page" echo "3. When at the last page" echo "4. When near the beginning/end for multi-page jumps"Length of output: 821
Pagination disabled conditions are correct
The expressions
disabled={stepCounter > pageCount - 2}disabled={stepCounter > pageCount - 6}properly disable the “Next page” and “Next 5 pages” buttons once you reach the last page (or when fewer than five pages remain). The corresponding
onClickguards also prevent stepping pastpageCount - 1. No changes are needed here.Likely an incorrect or invalid review comment.
app/graph/GraphDataTable.tsx (1)
78-83: Directly mutatingobject.dataandgraphcan desynchronise React state
object.data[key] = valanddelete object.data[key]mutate props passed from the parent and mutate the globalgraphinstance.
If another component relies on the same references, React will not detect the change → stale UI or memo caches.Consider cloning and passing the new object upward (or exposing dedicated setters on
Graph) instead of in-place mutation.
If mutation is unavoidable (performance), wrap it ingraph’s own mutator so all observers (e.g. context, redux store) are notified.
[ suggest_optional_refactor ]Also applies to: 132-135
app/graph/selectGraph.tsx (1)
67-70: Immutable row generation can capture stalesession
handleSetRowsusessession?.user?.rolebut is recreated on every render; when the session changes the previously stored row objects retain the old closures.
Addsessionto the dependency list or compute rows inside theuseEffectthat already depends onoptions.app/graph/GraphDataPanel.tsx (1)
115-159: LGTM! Dialog UI implementationThe implementation of the modal dialog view for the data panel when it has many labels is a good improvement for the user experience. It provides better use of space and maintains all functionality.
| const setProperty = async (key: string, val: string, isUndo: boolean, actionType: ("added" | "set") = "set") => { | ||
| const { id } = object | ||
| if (!val || val === "") { | ||
| toast({ | ||
| title: "Error", | ||
| description: "Please fill in the value field", | ||
| variant: "destructive" | ||
| }) | ||
| return false | ||
| } | ||
| try { | ||
| if (actionType === "set") setIsSetLoading(true) | ||
| const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${id}/${key}`, { | ||
| method: "POST", | ||
| body: JSON.stringify({ | ||
| value: val, | ||
| type | ||
| }) | ||
| }, toast, setIndicator) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing Content-Type header & ambiguous HTTP verb in setProperty API call
securedFetch sends a JSON body but no Content-Type: application/json header, leaving content negotiation to the server and risking a 415 error.
Additionally, POST is normally used for creation, whereas this branch updates an existing key. PATCH (preferred) or PUT would make the intent clearer.
-const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${id}/${key}`, {
- method: "POST",
- body: JSON.stringify({ value: val, type })
-}, toast, setIndicator)
+const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${id}/${key}`, {
+ method: "PATCH",
+ headers: { "Content-Type": "application/json" },
+ body: JSON.stringify({ value: val, type })
+}, toast, setIndicator)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setProperty = async (key: string, val: string, isUndo: boolean, actionType: ("added" | "set") = "set") => { | |
| const { id } = object | |
| if (!val || val === "") { | |
| toast({ | |
| title: "Error", | |
| description: "Please fill in the value field", | |
| variant: "destructive" | |
| }) | |
| return false | |
| } | |
| try { | |
| if (actionType === "set") setIsSetLoading(true) | |
| const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${id}/${key}`, { | |
| method: "POST", | |
| body: JSON.stringify({ | |
| value: val, | |
| type | |
| }) | |
| }, toast, setIndicator) | |
| const setProperty = async (key: string, val: string, isUndo: boolean, actionType: ("added" | "set") = "set") => { | |
| const { id } = object | |
| if (!val || val === "") { | |
| toast({ | |
| title: "Error", | |
| description: "Please fill in the value field", | |
| variant: "destructive" | |
| }) | |
| return false | |
| } | |
| try { | |
| if (actionType === "set") setIsSetLoading(true) | |
| const result = await securedFetch( | |
| `api/graph/${prepareArg(graph.Id)}/${id}/${key}`, | |
| { | |
| method: "PATCH", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ | |
| value: val, | |
| type | |
| }) | |
| }, | |
| toast, | |
| setIndicator | |
| ) |
🤖 Prompt for AI Agents
In app/graph/GraphDataTable.tsx between lines 58 and 77, the securedFetch call
sends a JSON body without specifying the Content-Type header, which can cause
server errors. Add a headers object with 'Content-Type': 'application/json' to
the fetch options. Also, change the HTTP method from 'POST' to 'PATCH' to better
reflect that this request updates an existing property rather than creating a
new one.
| const handleSetOption = async (option: string, optionName: string) => { | ||
| const result = await securedFetch(`api/${type === "Graph" ? "graph" : "schema"}/${prepareArg(option)}/?sourceName=${prepareArg(optionName)}`, { | ||
| method: "PATCH", | ||
| headers: { | ||
| "Content-Type": "application/json" | ||
| }, | ||
| }, toast, setIndicator) | ||
|
|
||
| if (result.ok) { | ||
|
|
||
| const newOptions = options.map((opt) => opt === optionName ? option : opt) | ||
| setOptions!(newOptions) | ||
|
|
||
| if (setSelectedValue && optionName === selectedValue) setSelectedValue(option) | ||
|
|
||
| handleSetRows(newOptions) | ||
| } | ||
|
|
||
| return result.ok | ||
| } |
There was a problem hiding this comment.
PATCH request lacks a body but still declares Content-Type
Declaring Content-Type: application/json without a body violates the spec and may trigger CORS pre-flight failures in some browsers.
Either supply a JSON body (e.g. { newName: option }) or remove the header for this query-parameter-only call.
🤖 Prompt for AI Agents
In app/graph/selectGraph.tsx between lines 46 and 65, the PATCH request sets the
Content-Type header to application/json but does not include a request body,
which can cause CORS pre-flight issues. To fix this, either add a JSON body with
the updated data (e.g., { newName: option }) or remove the Content-Type header
entirely since the request only uses query parameters.
…point Fix #970 Add pulling to fetch count end point
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
app/schema/page.tsx (3)
87-94:⚠️ Potential issueAvoid mutating React state directly
stateSelectedElements.push(selectedElement)mutates the original array instance ifselectedElementsis empty (becauseObject.valuesreturns the same reference for arrays). Mutating state outsidesetStatebreaks React's immutability guarantees and may cause subtle rendering bugs.- const stateSelectedElements = Object.values(selectedElements) + let stateSelectedElements = [...selectedElements] if (stateSelectedElements.length === 0 && selectedElement) { - stateSelectedElements.push(selectedElement) + stateSelectedElements = [...stateSelectedElements, selectedElement] setSelectedElement(undefined) }
95-104:⚠️ Potential issueAdd error handling for API calls in handleDeleteElement
The Promise.all doesn't handle API failures, and individual fetch operations don't have proper error handling beyond checking result.ok.
await Promise.all(stateSelectedElements.map(async (element) => { const { id } = element const type = !("source" in element) - const result = await securedFetch(`api/schema/${prepareArg(schema.Id)}/${prepareArg(id.toString())}`, { - method: "DELETE", - body: JSON.stringify({ type }), - }, toast, setIndicator) + try { + const result = await securedFetch(`api/schema/${prepareArg(schema.Id)}/${prepareArg(id.toString())}`, { + method: "DELETE", + body: JSON.stringify({ type }), + }, toast, setIndicator) + + if (!result.ok) { + throw new Error(`Failed to delete element: ${result.statusText}`) + } + } catch (error) { + console.error("Error deleting element:", error) + toast({ + title: "Error", + description: `Failed to delete element: ${error.message || "Unknown error"}`, + variant: "destructive" + }) + return // Continue with other deletions even if one fails + } - if (!result.ok) return
140-140:⚠️ Potential issue
stateSelectedElementsvsselectedElements– wrong collection ends up inremoveLinksYou build a local
stateSelectedElementscopy, possibly augment it withselectedElement, but the subsequent logic (schema.removeLinks) still references the oldselectedElementsarray. As a result, links belonging only to the additionally-pushed element are never removed.- schema.removeLinks(setLabels, selectedElements.map((element) => element.id)) + schema.removeLinks(setLabels, stateSelectedElements.map((element) => element.id))app/graph/page.tsx (2)
126-137:⚠️ Potential issue
handleCooldowntreatsundefinedas "running"When called with no argument (e.g., after
runQuery),ticksisundefined, yet the else-branch flags the canvas as running. Pass an explicit 0 or guard forundefinedto reflect the real engine state.if (ticks === 0) { canvas.setAttribute('data-engine-status', 'stop') } else { canvas.setAttribute('data-engine-status', 'running') }Proper fix:
- if (ticks === 0) { + if (ticks === undefined || ticks === 0) { canvas.setAttribute('data-engine-status', 'stop') } else { canvas.setAttribute('data-engine-status', 'running') }
165-169:⚠️ Potential issueState array mutated in place – breaks React invariants
selectedElements.push(selectedElement)alters the current state array without using the setter. This may not trigger rerenders and causes unpredictable behavior.- if (selectedElements.length === 0 && selectedElement) { - selectedElements.push(selectedElement) - setSelectedElement(undefined) - } + let elementsToDelete = [...selectedElements]; + if (elementsToDelete.length === 0 && selectedElement) { + elementsToDelete = [...elementsToDelete, selectedElement]; + setSelectedElement(undefined) + }Make sure to use
elementsToDeleteinstead ofselectedElementsthroughout the rest of the function.
🧹 Nitpick comments (5)
app/api/schema/[schema]/count/route.ts (1)
18-23: Good refactoring approach, but consider query size limitationsThe approach of centralizing graph querying through the internal API endpoint is good for consistency and maintainability. However, be aware that very long Cypher queries might exceed URL length limits when passed as query parameters.
For more complex queries, consider using a POST request with the query in the request body:
- const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, { - method: "GET", + const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/`, { + method: "POST", headers: { cookie: request.headers.get('cookie') || '', + 'Content-Type': 'application/json', } + body: JSON.stringify({ query }) })app/schema/page.tsx (2)
44-54: Add a retry limit to prevent infinite pollingWhile the polling mechanism is functional, it could benefit from a maximum retry limit to avoid potential infinite loops in case of persistent partial results.
+ let retryCount = 0; + const MAX_RETRIES = 5; let json = await result.json() - while (typeof json.result === "number") { + while (typeof json.result === "number" && retryCount < MAX_RETRIES) { + retryCount++;
147-147: Missing success notification after element deletionUnlike the graph page implementation that provides a success toast after deletion, the schema page doesn't notify users of successful deletion operations.
setSelectedElement(undefined) setSelectedElements([]) setData({ ...schema.Elements }) + toast({ + title: "Success", + description: `${stateSelectedElements.length > 1 ? "Elements" : "Element"} deleted`, + })app/graph/page.tsx (2)
58-68: Add a retry limit to prevent infinite pollingSimilar to the schema page, the polling logic should have a maximum retry limit to prevent potential infinite loops.
+ let retryCount = 0; + const MAX_RETRIES = 5; let json = await result.json() - while (typeof json.result === "number") { + while (typeof json.result === "number" && retryCount < MAX_RETRIES) { + retryCount++;
223-224: Redundant state setting
setSelectedElement(undefined)is called twice (lines 213 and 223) which is redundant. Also,setSelectedElements([])is called twice (lines 212 and 224).handleCooldown() - setSelectedElement(undefined) - setSelectedElements([])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
app/api/graph/[graph]/count/route.ts(1 hunks)app/api/schema/[schema]/count/route.ts(1 hunks)app/components/PaginationList.tsx(1 hunks)app/graph/page.tsx(4 hunks)app/graph/selectGraph.tsx(1 hunks)app/schema/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/api/graph/[graph]/count/route.ts
- app/components/PaginationList.tsx
- app/graph/selectGraph.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run settings tests
🔇 Additional comments (5)
app/schema/page.tsx (3)
16-30: State initialization looks good with React Context integrationThe component successfully transitions from local state to React Context for schema-related state management, and properly initializes component-specific state for multi-selection, cooldown, and other UI features.
152-165: Component composition with props drilling looks cleanThe Selector component receives all necessary props for managing selections and handling element deletion, which promotes good component composition.
166-189: SchemaView receives proper props for multi-selection and element managementThe SchemaView component is now correctly configured with all props needed for multi-selection, element deletion, and cooldown handling.
app/graph/page.tsx (2)
229-244: Clean and consistent Selector component propsThe Selector component receives appropriate props for managing selections, history, and handling element deletion.
246-264: GraphView receives comprehensive props for multi-selection and state managementThe GraphView component now receives all necessary props for handling multi-selection, cooldown, and element deletion.
| const data = json.result.data[0] | ||
|
|
||
| return NextResponse.json({ result }, { status: 200 }) | ||
| return NextResponse.json({ result: data }, { status: 200 }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add response structure validation
The code assumes a specific response structure without validation, which could break if the API response format changes.
Add validation before accessing nested properties:
const json = await result.json()
- const data = json.result.data[0]
+ if (!json.result?.data?.length) {
+ throw new Error("Unexpected response format: missing result data");
+ }
+ const data = json.result.data[0]
return NextResponse.json({ result: data }, { status: 200 })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data = json.result.data[0] | |
| return NextResponse.json({ result }, { status: 200 }) | |
| return NextResponse.json({ result: data }, { status: 200 }) | |
| const json = await result.json() | |
| if (!json.result?.data?.length) { | |
| throw new Error("Unexpected response format: missing result data") | |
| } | |
| const data = json.result.data[0] | |
| return NextResponse.json({ result: data }, { status: 200 }) |
🤖 Prompt for AI Agents
In app/api/schema/[schema]/count/route.ts around lines 29 to 31, the code
directly accesses nested properties of the API response without validating the
structure, which risks runtime errors if the response format changes. Add checks
to confirm that json.result and json.result.data exist and that data is an array
with at least one element before accessing json.result.data[0]. If validation
fails, handle the error gracefully, such as returning an appropriate error
response or default value.
| } | ||
| }) | ||
|
|
||
| if (!result.ok) throw new Error("Something went wrong") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with more specific messaging
The current error message is too generic and doesn't provide useful debugging information.
Include error details from the response for better troubleshooting:
- if (!result.ok) throw new Error("Something went wrong")
+ if (!result.ok) {
+ const errorText = await result.text();
+ throw new Error(`Failed to fetch graph data: ${result.status} ${result.statusText}. ${errorText}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!result.ok) throw new Error("Something went wrong") | |
| if (!result.ok) { | |
| const errorText = await result.text(); | |
| throw new Error(`Failed to fetch graph data: ${result.status} ${result.statusText}. ${errorText}`); | |
| } |
🤖 Prompt for AI Agents
In app/api/schema/[schema]/count/route.ts at line 25, the error handling uses a
generic message "Something went wrong" which lacks useful debugging information.
Modify the error throw to include specific error details from the response
object, such as an error message or status code, to provide clearer context for
troubleshooting.
| await Promise.all(selectedElements.map(async (element) => { | ||
| const type = !element.source | ||
| const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, { | ||
| method: "DELETE", | ||
| body: JSON.stringify({ type }) | ||
| }, toast, setIndicator) | ||
|
|
||
| if (!result.ok) return | ||
|
|
||
| if (type) { | ||
| (element as Node).category.forEach((category) => { | ||
| const cat = graph.CategoriesMap.get(category) | ||
| if (cat) { | ||
| cat.elements = cat.elements.filter((e) => e.id !== element.id) | ||
| if (cat.elements.length === 0) { | ||
| const index = graph.Categories.findIndex(c => c.name === cat.name) | ||
| if (index !== -1) { | ||
| graph.Categories.splice(index, 1) | ||
| graph.CategoriesMap.delete(cat.name) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| } else { | ||
| const category = graph.LabelsMap.get((element as Link).label) | ||
| if (category) { | ||
| category.elements = category.elements.filter((e) => e.id !== element.id) | ||
| if (category.elements.length === 0) { | ||
| const index = graph.Labels.findIndex(l => l.name === category.name) | ||
| if (index !== -1) { | ||
| graph.Labels.splice(index, 1) | ||
| graph.LabelsMap.delete(category.name) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API calls in handleDeleteElement
Similar to the schema page, the Promise.all for element deletion lacks proper error handling, which could lead to silent failures.
await Promise.all(selectedElements.map(async (element) => {
const type = !element.source
- const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, {
- method: "DELETE",
- body: JSON.stringify({ type })
- }, toast, setIndicator)
+ try {
+ const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, {
+ method: "DELETE",
+ body: JSON.stringify({ type })
+ }, toast, setIndicator)
+
+ if (!result.ok) {
+ throw new Error(`Failed to delete element: ${result.statusText}`)
+ }
+ } catch (error) {
+ console.error("Error deleting element:", error)
+ toast({
+ title: "Error",
+ description: `Failed to delete element: ${error.message || "Unknown error"}`,
+ variant: "destructive"
+ })
+ return // Continue with other deletions even if one fails
+ }
- if (!result.ok) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.all(selectedElements.map(async (element) => { | |
| const type = !element.source | |
| const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, { | |
| method: "DELETE", | |
| body: JSON.stringify({ type }) | |
| }, toast, setIndicator) | |
| if (!result.ok) return | |
| if (type) { | |
| (element as Node).category.forEach((category) => { | |
| const cat = graph.CategoriesMap.get(category) | |
| if (cat) { | |
| cat.elements = cat.elements.filter((e) => e.id !== element.id) | |
| if (cat.elements.length === 0) { | |
| const index = graph.Categories.findIndex(c => c.name === cat.name) | |
| if (index !== -1) { | |
| graph.Categories.splice(index, 1) | |
| graph.CategoriesMap.delete(cat.name) | |
| } | |
| } | |
| } | |
| }) | |
| } else { | |
| const category = graph.LabelsMap.get((element as Link).label) | |
| if (category) { | |
| category.elements = category.elements.filter((e) => e.id !== element.id) | |
| if (category.elements.length === 0) { | |
| const index = graph.Labels.findIndex(l => l.name === category.name) | |
| if (index !== -1) { | |
| graph.Labels.splice(index, 1) | |
| graph.LabelsMap.delete(category.name) | |
| } | |
| } | |
| } | |
| } | |
| })) | |
| await Promise.all(selectedElements.map(async (element) => { | |
| const type = !element.source | |
| try { | |
| const result = await securedFetch( | |
| `api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, | |
| { | |
| method: "DELETE", | |
| body: JSON.stringify({ type }) | |
| }, | |
| toast, | |
| setIndicator | |
| ) | |
| if (!result.ok) { | |
| throw new Error(`Failed to delete element: ${result.statusText}`) | |
| } | |
| } catch (error) { | |
| console.error("Error deleting element:", error) | |
| toast({ | |
| title: "Error", | |
| description: `Failed to delete element: ${error.message || "Unknown error"}`, | |
| variant: "destructive" | |
| }) | |
| return | |
| } | |
| if (type) { | |
| (element as Node).category.forEach((category) => { | |
| const cat = graph.CategoriesMap.get(category) | |
| if (cat) { | |
| cat.elements = cat.elements.filter((e) => e.id !== element.id) | |
| if (cat.elements.length === 0) { | |
| const index = graph.Categories.findIndex(c => c.name === cat.name) | |
| if (index !== -1) { | |
| graph.Categories.splice(index, 1) | |
| graph.CategoriesMap.delete(cat.name) | |
| } | |
| } | |
| } | |
| }) | |
| } else { | |
| const category = graph.LabelsMap.get((element as Link).label) | |
| if (category) { | |
| category.elements = category.elements.filter((e) => e.id !== element.id) | |
| if (category.elements.length === 0) { | |
| const index = graph.Labels.findIndex(l => l.name === category.name) | |
| if (index !== -1) { | |
| graph.Labels.splice(index, 1) | |
| graph.LabelsMap.delete(category.name) | |
| } | |
| } | |
| } | |
| } | |
| })) |
🤖 Prompt for AI Agents
In app/graph/page.tsx around lines 171 to 207, the Promise.all call deleting
elements does not handle errors from the API calls, risking silent failures.
Wrap the securedFetch call in a try-catch block inside the map callback to catch
any exceptions. In the catch block, log the error or use the toast to notify the
user of the failure. This ensures any API call errors are properly handled and
surfaced.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${graph}/?query=${encodeURIComponent(query)}`, { | ||
| method: "GET", | ||
| headers: { | ||
| cookie: request.headers.get('cookie') || '', | ||
| } | ||
| }) |
Check failure
Code scanning / CodeQL
Server-side request forgery
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the issue, we need to validate and sanitize the graph parameter before using it in the URL. The best approach is to restrict the graph parameter to a predefined set of allowed values (an allow-list). This ensures that only expected and safe values are used in the URL. If the graph parameter does not match any of the allowed values, the request should be rejected.
Steps to implement the fix:
- Define an allow-list of valid
graphvalues. - Check if the
graphparameter matches one of the allowed values. - If the
graphparameter is invalid, return an error response. - Use the validated
graphvalue in the URL.
| @@ -13,2 +13,9 @@ | ||
|
|
||
| // Define an allow-list of valid graph names | ||
| const allowedGraphs = ["graph1", "graph2", "graph3"]; | ||
|
|
||
| if (!allowedGraphs.includes(graph)) { | ||
| return NextResponse.json({ error: "Invalid graph parameter" }, { status: 400 }); | ||
| } | ||
|
|
||
| try { |
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, { | ||
| method: "GET", | ||
| headers: { | ||
| cookie: request.headers.get('cookie') || '', | ||
| } | ||
| }) |
Check failure
Code scanning / CodeQL
Server-side request forgery
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 20 hours ago
To fix the SSRF vulnerability:
- Restrict the
schemaparameter to a predefined allow-list of valid schema names. This ensures that only known and safe values are used in the URL. - Avoid directly using
request.nextUrl.originin the outgoing request. Instead, use a fixed, trusted base URL for the API endpoint. - Validate and sanitize all user-controlled inputs before incorporating them into the URL.
The changes will involve:
- Adding an allow-list of valid schema names.
- Replacing
request.nextUrl.originwith a trusted base URL. - Updating the
fetchcall to use the validatedschemaName.
| @@ -12,8 +12,13 @@ | ||
| const { schema } = await params | ||
| const schemaName = `${schema}_schema` | ||
| const allowedSchemas = ["schema1", "schema2", "schema3"]; // Define valid schema names | ||
| if (!allowedSchemas.includes(schema)) { | ||
| return NextResponse.json({ error: "Invalid schema" }, { status: 400 }); | ||
| } | ||
| const schemaName = `${schema}_schema`; | ||
|
|
||
| try { | ||
| const query = "MATCH (n) OPTIONAL MATCH (n)-[e]->() WITH count(n) as nodes, count(e) as edges RETURN nodes, edges" | ||
| const query = "MATCH (n) OPTIONAL MATCH (n)-[e]->() WITH count(n) as nodes, count(e) as edges RETURN nodes, edges"; | ||
| const baseUrl = "https://trusted-api.example.com"; // Use a fixed, trusted base URL | ||
|
|
||
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, { | ||
| const result = await fetch(`${baseUrl}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, { | ||
| method: "GET", |
…point Fix #970 fix pulling on fetch
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
app/api/graph/[graph]/count/route.ts (1)
17-22:⚠️ Potential issueAddress SSRF vulnerability by validating the graph parameter
The URL construction uses the user-provided
graphparameter directly without validation, creating a potential Server-Side Request Forgery (SSRF) vulnerability.Apply this fix to validate the graph parameter before using it in a URL:
const { graph } = await params + // Validate graph parameter to prevent SSRF attacks + if (!/^[a-zA-Z0-9_-]+$/.test(graph)) { + return NextResponse.json({ error: "Invalid graph name" }, { status: 400 }) + } const result = await fetch(`${request.nextUrl.origin}/api/graph/${graph}/?query=${encodeURIComponent(query)}`, {🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 17-22: Server-side request forgery
The URL of this request depends on a user-provided value.app/api/schema/[schema]/count/route.ts (3)
18-23:⚠️ Potential issueAddress SSRF vulnerability by validating the schema parameter
The URL construction uses the user-provided
schemaparameter directly without validation, creating a potential Server-Side Request Forgery (SSRF) vulnerability.Apply this fix to validate the schema parameter before using it in a URL:
const { schema } = await params const schemaName = `${schema}_schema` + // Validate schema parameter to prevent SSRF attacks + if (!/^[a-zA-Z0-9_-]+$/.test(schema)) { + return NextResponse.json({ error: "Invalid schema name" }, { status: 400 }) + } const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, {🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 18-23: Server-side request forgery
The URL of this request depends on a user-provided value.
25-25: 🛠️ Refactor suggestionImprove error handling with specific error details
The current error message is too generic, making debugging difficult.
- if (!result.ok) throw new Error("Something went wrong") + if (!result.ok) { + const errorText = await result.text().catch(() => "Could not read error response"); + throw new Error(`Failed to fetch schema data: ${result.status} ${result.statusText}. ${errorText}`); + }
29-29: 🛠️ Refactor suggestionAdd validation for response structure
The code accesses nested properties without validating the response structure, risking runtime errors if the format changes.
- const data = typeof json.result === "number" ? json.result : json.result.data[0] || { nodes: 0, edges: 0 } + let data = { nodes: 0, edges: 0 }; + if (typeof json.result === "number") { + data = json.result; + } else if (json.result?.data?.length > 0) { + data = json.result.data[0]; + }app/schema/page.tsx (3)
91-95:⚠️ Potential issueAvoid mutating React state directly
Directly pushing to
stateSelectedElementscould mutate the original array ifselectedElementsis empty, breaking React's immutability principles.- const stateSelectedElements = Object.values(selectedElements) + let stateSelectedElements = [...selectedElements] if (stateSelectedElements.length === 0 && selectedElement) { - stateSelectedElements.push(selectedElement) + stateSelectedElements = [...stateSelectedElements, selectedElement] setSelectedElement(undefined) }
142-142:⚠️ Potential issueFix inconsistent state reference
selectedElementsis used here, but the deletion logic usesstateSelectedElements, which could cause inconsistencies if they differ.- schema.removeLinks(setLabels, selectedElements.map((element) => element.id)) + schema.removeLinks(setLabels, stateSelectedElements.map((element) => element.id))
97-105: 🛠️ Refactor suggestionAdd proper error handling for API calls
The API call lacks proper error handling, which could lead to silent failures and inconsistent application state.
await Promise.all(stateSelectedElements.map(async (element) => { const { id } = element const type = !("source" in element) - const result = await securedFetch(`api/schema/${prepareArg(schema.Id)}/${prepareArg(id.toString())}`, { - method: "DELETE", - body: JSON.stringify({ type }), - }, toast, setIndicator) + try { + const result = await securedFetch(`api/schema/${prepareArg(schema.Id)}/${prepareArg(id.toString())}`, { + method: "DELETE", + body: JSON.stringify({ type }), + }, toast, setIndicator) + + if (!result.ok) { + throw new Error(`Failed to delete element: ${result.statusText}`) + } + } catch (error) { + console.error("Error deleting element:", error) + toast({ + title: "Error", + description: `Failed to delete element: ${(error as Error).message || "Unknown error"}`, + variant: "destructive" + }) + return // Continue with other deletions even if one fails + } - if (!result.ok) returnapp/graph/page.tsx (3)
167-171:⚠️ Potential issueAvoid mutating React state directly
Directly pushing to
selectedElementsbreaks React's immutability principles and may cause unpredictable behavior.const handleDeleteElement = async () => { if (selectedElements.length === 0 && selectedElement) { - selectedElements.push(selectedElement) + const newSelectedElements = [...selectedElements, selectedElement] + setSelectedElements(newSelectedElements) setSelectedElement(undefined) + + // Continue with deletion using the new array + await deleteSelectedElements(newSelectedElements) + return } + + await deleteSelectedElements(selectedElements) } + // Helper function to encapsulate the deletion logic + const deleteSelectedElements = async (elementsToDelete) => { + // Move the deletion logic here, using elementsToDelete instead of selectedElements + // ... + }
128-139:⚠️ Potential issueFix incorrect engine status handling for undefined ticks
The function doesn't properly handle
undefinedticks, incorrectly marking the engine as running.const handleCooldown = (ticks?: number) => { setCooldownTicks(ticks) const canvas = document.querySelector('.force-graph-container canvas'); if (!canvas) return - if (ticks === 0) { + if (ticks === undefined || ticks === 0) { canvas.setAttribute('data-engine-status', 'stop') } else { canvas.setAttribute('data-engine-status', 'running') } }
173-180: 🛠️ Refactor suggestionAdd proper error handling for API calls
The API call lacks proper error handling, which could lead to silent failures and inconsistent application state.
await Promise.all(selectedElements.map(async (element) => { const type = !element.source - const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, { - method: "DELETE", - body: JSON.stringify({ type }) - }, toast, setIndicator) + try { + const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, { + method: "DELETE", + body: JSON.stringify({ type }) + }, toast, setIndicator) + + if (!result.ok) { + throw new Error(`Failed to delete element: ${result.statusText}`) + } + } catch (error) { + console.error("Error deleting element:", error) + toast({ + title: "Error", + description: `Failed to delete element: ${(error as Error).message || "Unknown error"}`, + variant: "destructive" + }) + return // Continue with other deletions even if one fails + } - if (!result.ok) return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/api/graph/[graph]/count/route.ts(1 hunks)app/api/schema/[schema]/count/route.ts(1 hunks)app/graph/GraphDetails.tsx(1 hunks)app/graph/page.tsx(4 hunks)app/schema/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/graph/GraphDetails.tsx
🧰 Additional context used
🪛 GitHub Check: CodeQL
app/api/graph/[graph]/count/route.ts
[failure] 17-22: Server-side request forgery
The URL of this request depends on a user-provided value.
app/api/schema/[schema]/count/route.ts
[failure] 18-23: Server-side request forgery
The URL of this request depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
| } | ||
| }) | ||
|
|
||
| if (!result.ok) throw new Error("Something went wrong") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling with specific error details
The current error message is too generic, making debugging difficult.
- if (!result.ok) throw new Error("Something went wrong")
+ if (!result.ok) {
+ const errorText = await result.text().catch(() => "Could not read error response");
+ throw new Error(`Failed to fetch graph data: ${result.status} ${result.statusText}. ${errorText}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!result.ok) throw new Error("Something went wrong") | |
| if (!result.ok) { | |
| const errorText = await result.text().catch(() => "Could not read error response"); | |
| throw new Error(`Failed to fetch graph data: ${result.status} ${result.statusText}. ${errorText}`); | |
| } |
🤖 Prompt for AI Agents
In app/api/graph/[graph]/count/route.ts at line 24, the error thrown uses a
generic message "Something went wrong" which lacks specific details. Modify the
error handling to include more specific information from the result object, such
as an error message or status code, to provide clearer context for debugging
when the operation fails.
app/api/graph/[graph]/count/route.ts
Outdated
|
|
||
| const json = await result.json() | ||
|
|
||
| const data = typeof json.result === "number" ? json.result : json.result.data[0] || { edges: 0, nodes: 0 } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for response structure
The code accesses nested properties without validating the response structure, risking runtime errors if the format changes.
- const data = typeof json.result === "number" ? json.result : json.result.data[0] || { edges: 0, nodes: 0 }
+ let data = { edges: 0, nodes: 0 };
+ if (typeof json.result === "number") {
+ data = json.result;
+ } else if (json.result?.data?.length > 0) {
+ data = json.result.data[0];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data = typeof json.result === "number" ? json.result : json.result.data[0] || { edges: 0, nodes: 0 } | |
| // Replace the original one‐liner with explicit checks | |
| let data = { edges: 0, nodes: 0 }; | |
| if (typeof json.result === "number") { | |
| data = json.result; | |
| } else if (json.result?.data?.length > 0) { | |
| data = json.result.data[0]; | |
| } |
🤖 Prompt for AI Agents
In app/api/graph/[graph]/count/route.ts at line 28, the code accesses nested
properties of the response without validating the structure, which can cause
runtime errors if the response format changes. Add checks to confirm that
json.result exists and has the expected properties before accessing
json.result.data[0]. Use conditional statements or optional chaining combined
with default values to safely handle unexpected or missing data structures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/graph/GraphDataPanel.tsx (2)
84-86: State updates should use immutable patternsThe code still directly spreads the graph.Categories array after modification, which doesn't follow React's immutable update pattern and was previously flagged in code reviews.
- setCategories([...graph.Categories]) - setLabel([...node.category]) + setCategories(prevCategories => { + const updatedCategories = [...prevCategories] + const newCategoryIndex = graph.Categories.findIndex(c => c.name === newLabel) + if (newCategoryIndex !== -1) { + updatedCategories.push({...graph.Categories[newCategoryIndex]}) + } + return updatedCategories + }) + setLabel(prevLabels => [...prevLabels, newLabel])
104-107: State updates should use immutable patterns when removing labelsSimilar to the add label case, the remove label functionality should update state immutably rather than spreading arrays that were modified externally.
- setCategories([...graph.Categories]) - setLabel([...node.category]) + setCategories(prevCategories => + prevCategories.filter(c => c.name !== removeLabel) + ) + setLabel(prevLabels => prevLabels.filter(l => l !== removeLabel))app/graph/toolbar.tsx (2)
50-55: Fix potential null/undefined handling in search filterThe current implementation could crash if any value in
el.datais null or undefined sincetoString()would be called on them.While I see this was already identified in a previous review and marked as addressed, the fix doesn't appear in this version of the code. For safety, the filter should check for null/undefined values before calling
toString().const elements = graph.getElements().filter(el => - Object.values(el.data).some(value => value.toString().toLowerCase().startsWith(searchElement.toLowerCase())) + Object.values(el.data).some(value => + value !== null && value !== undefined && + String(value).toLowerCase().startsWith(searchElement.toLowerCase()) + ) || el.id.toString().toLowerCase().includes(searchElement.toLowerCase()) || el.label && (el as Link).label.toLowerCase().includes(searchElement.toLowerCase()) || el.category && (el as Node).category.some(c => c.toLowerCase().includes(searchElement.toLowerCase())) )
60-66: Reset suggestionIndex when suggestions changeIf the previous index is larger than the new suggestions list's length,
suggestions[suggestionIndex]could becomeundefined, breaking Enter and arrow-navigation functionality.useEffect(() => { - const timeout = setTimeout(handleOnChange, 500) + const timeout = setTimeout(() => { + handleOnChange() + setSuggestionIndex(0) // <- reset index when suggestions change + }, 500) return () => { clearTimeout(timeout) } }, [graph, handleOnChange, searchElement])
🧹 Nitpick comments (4)
app/graph/GraphDataPanel.tsx (3)
115-159: Consider extracting view components to reduce complexityThe component has two entirely different rendering paths based on showAsDialog. This creates a large, complex component that's harder to test and maintain.
Consider extracting each view into its own component to improve readability and maintainability:
function GraphDataPanelDialog({ object, setObject, label, selectedLabel, setSelectedLabel, handleAddLabel, handleRemoveLabel, onDeleteElement }) { // Dialog implementation } function GraphDataPanelInline({ object, setObject, label, labelsListRef, labelsHover, setLabelsHover, handleAddLabel, handleRemoveLabel, onDeleteElement }) { // Inline implementation } // Main component just handles logic and conditionally renders the appropriate viewAlso applies to: 160-236
44-48: Consider debouncing the dialog decisionThe effect that determines whether to show labels as a dialog runs on every clientHeight change, which could cause flickering if the height is near the threshold.
Consider adding debounce logic or a small buffer around the threshold to prevent rapid toggling:
useEffect(() => { if (labelsListRef.current) { // Add a small buffer to prevent flickering const shouldShowDialog = labelsListRef.current.clientHeight > 85; if (shouldShowDialog !== showAsDialog) { setShowAsDialog(shouldShowDialog); } } }, [labelsListRef.current?.clientHeight, showAsDialog]);
58-91: Extract API operations to a custom hookThe label management API operations could be extracted to a custom hook to improve reusability and separation of concerns.
Consider creating a custom hook like
useGraphLabelsthat encapsulates the API logic:function useGraphLabels(graph, object, toast, setIndicator, setCategories, setLabel) { const addLabel = async (newLabel) => { // Implementation of handleAddLabel }; const removeLabel = async (removeLabel) => { // Implementation of handleRemoveLabel }; return { addLabel, removeLabel }; } // In component: const { addLabel, removeLabel } = useGraphLabels( graph, object, toast, setIndicator, setCategories, setLabel );Also applies to: 93-113
app/graph/toolbar.tsx (1)
68-73: Consider optimizing element zoom logicThe current implementation of
handleSearchElementlooks good, but the filter function passed tohandleZoomToFitcould be more efficient.Consider memoizing the filter function or optimizing the condition for complex graphs:
const handleSearchElement = (element: Node | Link) => { - handleZoomToFit(chartRef, (node: Node) => element.category ? element.id === node.id : node.id === element.source.id || node.id === element.target.id) + // For nodes, zoom to the node itself; for links, zoom to both connected nodes + const zoomFilter = element.category + ? (node: Node) => element.id === node.id + : (node: Node) => node.id === element.source.id || node.id === element.target.id + + handleZoomToFit(chartRef, zoomFilter) setSelectedElement(element) setSearchElement("") setSuggestions([]) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/api/graph/model.ts(14 hunks)app/graph/DeleteElement.tsx(2 hunks)app/graph/GraphDataPanel.tsx(2 hunks)app/graph/GraphDataTable.tsx(1 hunks)app/graph/addLabel.tsx(1 hunks)app/graph/toolbar.tsx(1 hunks)app/schema/SchemaCreateElement.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/graph/addLabel.tsx
- app/graph/DeleteElement.tsx
- app/graph/GraphDataTable.tsx
- app/api/graph/model.ts
- app/schema/SchemaCreateElement.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
🔇 Additional comments (11)
app/graph/GraphDataPanel.tsx (6)
38-41: Good implementation of component state managementThe component properly separates concerns with distinct state variables for UI behavior (showAsDialog, labelsHover) and data (selectedLabel, label). This aligns well with the PR's focus on improved state management.
29-30: Effective use of context for global stateUsing GraphContext and IndicatorContext instead of prop drilling improves code maintainability and aligns with the architectural refactoring described in the PR objectives.
76-91: Good pattern for API error handling and feedbackThe implementation properly handles API responses, provides user feedback via toast notifications, and updates the UI state accordingly. The function also correctly returns booleans to indicate success/failure to callers.
148-154: Effective use of component composition with GraphDataTableExtracting attribute management into a dedicated GraphDataTable component is a good refactoring that improves separation of concerns and maintainability, aligning with the PR's architectural improvements.
Also applies to: 228-234
189-204: Well-implemented role-based UI permissionsThe code correctly implements role-based UI permissions by conditionally rendering the remove label button based on the user's role. This is a good security practice that aligns with the PR's objectives.
176-178: Good use of data-testid attributesAdding data-testid attributes throughout the component improves testability, which aligns with the PR objectives of enhanced testability via data-testid attributes.
app/graph/toolbar.tsx (5)
75-80: LGTM: Good keyboard navigation implementationThe
scrollToSuggestionfunction provides proper keyboard navigation by automatically scrolling to the highlighted suggestion, which greatly improves accessibility.
83-200: Great search interface implementationThe implementation of the search input and suggestion list provides excellent user experience with:
- Debounced search filtering
- Keyboard navigation support
- Accessible ARIA roles
- Visual highlighting of selected items
- Tooltips for additional information
206-215: LGTM: Good conditional rendering of delete buttonThe deletion UI is only shown when multiple elements are selected, which is a sensible design choice that prevents accidental deletions and keeps the UI clean when not needed.
217-260: Good implementation of Add Element dropdownThe dropdown menu for adding nodes and edges is well-implemented with:
- Proper reset of selected element when changing modes
- Clear test IDs for automated testing
- Appropriate button labels and icons
268-272: Good use of defaultPropsProviding default values for optional props is a good practice that improves component reusability and prevents potential runtime errors.
| onKeyDown={(e) => { | ||
| if (e.key === 'Escape') { | ||
| e.preventDefault() | ||
| setSearchElement("") | ||
| } | ||
|
|
||
| if (e.key === 'Enter' && suggestions[suggestionIndex]) { | ||
| e.preventDefault() | ||
| handleSearchElement(suggestions[suggestionIndex]) | ||
| setSearchElement("") | ||
| } | ||
|
|
||
| if (e.key === 'ArrowDown') { | ||
| e.preventDefault() | ||
| const index = suggestionIndex === suggestions.length - 1 ? 0 : suggestionIndex + 1 | ||
| setSuggestionIndex(index) | ||
| scrollToSuggestion(index) | ||
| } | ||
|
|
||
| if (e.key === 'ArrowUp') { | ||
| e.preventDefault() | ||
| const index = suggestionIndex === 0 ? suggestions.length - 1 : suggestionIndex - 1 | ||
| setSuggestionIndex(index) | ||
| scrollToSuggestion(index) | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate duplicated keyboard navigation logic
The same keyboard event handling code is duplicated for both the input element and the suggestion list. This creates maintenance issues and increases the risk of inconsistencies.
Extract the shared keyboard handling logic into a reusable function:
+ const handleKeyboardNavigation = (e: React.KeyboardEvent) => {
+ if (e.key === 'Escape') {
+ e.preventDefault()
+ setSearchElement("")
+ }
+
+ if (e.key === 'Enter' && suggestions[suggestionIndex]) {
+ e.preventDefault()
+ handleSearchElement(suggestions[suggestionIndex])
+ setSearchElement("")
+ }
+
+ if (e.key === 'ArrowDown') {
+ e.preventDefault()
+ const index = suggestionIndex === suggestions.length - 1 ? 0 : suggestionIndex + 1
+ setSuggestionIndex(index)
+ scrollToSuggestion(index)
+ }
+
+ if (e.key === 'ArrowUp') {
+ e.preventDefault()
+ const index = suggestionIndex === 0 ? suggestions.length - 1 : suggestionIndex - 1
+ setSuggestionIndex(index)
+ scrollToSuggestion(index)
+ }
+ }
// Then replace both event handlers with the shared function:
// In the Input component:
- onKeyDown={(e) => {
- if (e.key === 'Escape') {
- e.preventDefault()
- setSearchElement("")
- }
-
- if (e.key === 'Enter' && suggestions[suggestionIndex]) {
- e.preventDefault()
- handleSearchElement(suggestions[suggestionIndex])
- setSearchElement("")
- }
-
- if (e.key === 'ArrowDown') {
- e.preventDefault()
- const index = suggestionIndex === suggestions.length - 1 ? 0 : suggestionIndex + 1
- setSuggestionIndex(index)
- scrollToSuggestion(index)
- }
-
- if (e.key === 'ArrowUp') {
- e.preventDefault()
- const index = suggestionIndex === 0 ? suggestions.length - 1 : suggestionIndex - 1
- setSuggestionIndex(index)
- scrollToSuggestion(index)
- }
- }}
+ onKeyDown={handleKeyboardNavigation}
// And in the suggestions list:
- onKeyDown={(e) => {
- // Same duplicate code
- }}
+ onKeyDown={handleKeyboardNavigation}Also applies to: 137-162
🤖 Prompt for AI Agents
In app/graph/toolbar.tsx around lines 93 to 118 and also 137 to 162, the
keyboard event handling logic for navigation and selection is duplicated for
both the input element and the suggestion list. To fix this, extract the shared
onKeyDown handler code into a single reusable function that takes the keyboard
event and handles Escape, Enter, ArrowDown, and ArrowUp keys consistently. Then
replace the duplicated inline handlers with calls to this new function to
centralize and simplify the logic.
…ceholder. Update usages in Configurations and Users components to pass entityName prop. Adjust DialogContent height in SelectGraph for better layout.
Fix #972 Enhance TableComponent to support dynamic entity names for search placeholder.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/graph/selectGraph.tsx (1)
46-52:PATCHrequest lacks a body but still declaresContent-TypeDeclaring
Content-Type: application/jsonwithout a body violates the spec and may trigger CORS pre-flight failures in some browsers.
Either supply a JSON body (e.g.{ newName: option }) or remove the header for this query-parameter-only call.const handleSetOption = async (option: string, optionName: string) => { const result = await securedFetch(`api/${type === "Graph" ? "graph" : "schema"}/${prepareArg(option)}/?sourceName=${prepareArg(optionName)}`, { method: "PATCH", - headers: { - "Content-Type": "application/json" - }, }, toast, setIndicator)
🧹 Nitpick comments (3)
app/graph/selectGraph.tsx (3)
1-2: Consider removing ESLint disable directiveThe
eslint-disablecomment at the top of the file suppresses warnings about using variables before they're defined. Instead of disabling this rule, consider refactoring the code to maintain a logical flow where functions are defined before use.
67-69: Break down complex row mapping logic for better readabilityThe current implementation packs too much logic into a single line, making it difficult to read and maintain. Consider breaking this down into smaller, more focused functions.
const handleSetRows = (opts: string[]) => { - setRows(opts.map(opt => session?.user?.role === "Admin" ? ({ checked: false, name: opt, cells: [{ value: opt, onChange: (value: string) => handleSetOption(value, opt), type: "text" }] }) : ({ checked: false, name: opt, cells: [{ value: opt, type: "readonly" }] }))) + setRows(opts.map(opt => { + const isAdmin = session?.user?.role === "Admin"; + return { + checked: false, + name: opt, + cells: [ + isAdmin + ? { value: opt, onChange: (value: string) => handleSetOption(value, opt), type: "text" } + : { value: opt, type: "readonly" } + ] + }; + })); }
99-105: Consider using a single chevron component with dynamic rotationThe current implementation uses two separate components (ChevronUp and ChevronDown) based on the open state. A more maintainable approach would be to use a single component with dynamic styling.
-{ - open ? - <ChevronUp className="min-w-4 min-h-4" /> - : - <ChevronDown className="min-w-4 min-h-4" /> -} +<ChevronDown className={`min-w-4 min-h-4 transform transition-transform ${open ? 'rotate-180' : ''}`} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/TableComponent.tsx(10 hunks)app/graph/selectGraph.tsx(1 hunks)app/settings/Configurations.tsx(2 hunks)app/settings/users/Users.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/settings/users/Users.tsx
- app/components/TableComponent.tsx
- app/settings/Configurations.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
🔇 Additional comments (2)
app/graph/selectGraph.tsx (2)
116-116: Empty callback function may indicate missing implementationThe
afterSearchCallbackis set to an empty function. This might indicate an incomplete implementation or a missed requirement. Consider implementing proper functionality or documenting why this is intentionally empty.-afterSearchCallback={() => { }} +afterSearchCallback={(searchResult) => { + // Add appropriate implementation or + // TODO: Implement search result handling +}}
147-172: Duplicate button may be enabled in inconsistent statesThe
DuplicateGraphcomponent's disabled prop only checks if exactly one row is checked, but doesn't verify if the table is empty. This could lead to inconsistent UI states where the button appears enabled but doesn't have valid data to work with.Consider enhancing the disabled condition to handle more edge cases:
<DuplicateGraph selectedValue={rows.filter(opt => opt.checked).map(opt => opt.cells[0].value as string)[0]} type={type} open={openDuplicate} onOpenChange={setOpenDuplicate} onDuplicate={(duplicateName) => { setSelectedValue(duplicateName) setOptions!([...options, duplicateName]) }} - disabled={rows.filter(opt => opt.checked).length !== 1} + disabled={rows.length === 0 || rows.filter(opt => opt.checked).length !== 1} />
| </DropdownMenu> | ||
| <DialogContent disableClose className="flex flex-col border-none rounded-lg max-w-none h-[90dvh]"> | ||
| <DialogHeader className="flex-row justify-between items-center border-b border-secondary pb-4"> | ||
| <DialogTitle className="text-2xl font-medium">Manage Graphs</DialogTitle> |
There was a problem hiding this comment.
Hardcoded dialog title should use the type prop
The dialog title is hardcoded as "Manage Graphs" when it should dynamically use the type prop to show either "Manage Graphs" or "Manage Schemas".
-<DialogTitle className="text-2xl font-medium">Manage Graphs</DialogTitle>
+<DialogTitle className="text-2xl font-medium">Manage {type}s</DialogTitle>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <DialogTitle className="text-2xl font-medium">Manage Graphs</DialogTitle> | |
| <DialogTitle className="text-2xl font-medium">Manage {type}s</DialogTitle> |
🤖 Prompt for AI Agents
In app/graph/selectGraph.tsx at line 131, the DialogTitle is hardcoded as
"Manage Graphs" but should dynamically display the title based on the `type`
prop. Update the DialogTitle to conditionally render "Manage Graphs" or "Manage
Schemas" depending on the value of the `type` prop passed to the component.
| useEffect(() => { | ||
| handleSetRows(options) | ||
| }, [options]) |
There was a problem hiding this comment.
Missing dependencies in useEffect dependency array
The useEffect dependency array is missing handleSetRows which depends on session, handleSetOption, and potentially other variables. This could lead to stale closures and unexpected behavior.
useEffect(() => {
handleSetRows(options)
-}, [options])
+}, [options, session, handleSetOption])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| handleSetRows(options) | |
| }, [options]) | |
| useEffect(() => { | |
| handleSetRows(options) | |
| }, [options, session, handleSetOption]) |
🤖 Prompt for AI Agents
In app/graph/selectGraph.tsx around lines 71 to 73, the useEffect hook's
dependency array is missing the handleSetRows function, which relies on session,
handleSetOption, and possibly other variables. To fix this, include
handleSetRows in the dependency array to ensure the effect runs correctly when
handleSetRows or its dependencies change, preventing stale closures and
unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
app/components/Header.tsx (3)
10-11: Importing the wholepackage.jsonbloats the client bundle & leaks internals
import pkg from '@/package.json'in a client component ships the entirepackage.json(including dependency list) to every visitor.
Consider creating a small file that only exports the version string.
37-44: UsingLinkfor an external URL disables Next.js optimizations
next/linkis designed for internal navigation; for external resources it adds unnecessary overhead.
31-31:⚠️ Potential issue
showCreatecan erroneously expose create-UI to unauthenticated usersWhen
sessionis still loading (null/undefined),session?.user?.role !== "Read-Only"evaluates totrue, and the create button becomes visible before authentication is complete.-const showCreate = (pathname.includes("/graph") || pathname.includes("/schema")) && session?.user?.role !== "Read-Only" +const showCreate = + (pathname.includes("/graph") || pathname.includes("/schema")) && + Boolean(session?.user?.role) && + session.user.role !== "Read-Only"app/api/graph/[graph]/count/route.ts (1)
24-24: Improve error handling with specific error detailsThe current error message is too generic, making debugging difficult.
- if (!result.ok) throw new Error("Something went wrong") + if (!result.ok) { + const errorText = await result.text().catch(() => "Could not read error response"); + throw new Error(`Failed to fetch graph data: ${result.status} ${result.statusText}. ${errorText}`); + }app/api/schema/[schema]/count/route.ts (1)
25-25: Improve error handling with more specific messagingThe current error message is too generic and doesn't provide useful debugging information.
- if (!result.ok) throw new Error("Something went wrong") + if (!result.ok) { + const errorText = await result.text().catch(() => "Could not read error response"); + throw new Error(`Failed to fetch schema data: ${result.status} ${result.statusText}. ${errorText}`); + }
🧹 Nitpick comments (1)
app/components/Header.tsx (1)
138-138: Remove unnecessary template literal and braces in version displayThe version is displayed with superfluous braces that will show as "{2.0.0}" instead of just "2.0.0".
-<p>Version: {`{${pkg.version}}`}</p> +<p>Version: {pkg.version}</p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/api/graph/[graph]/count/route.ts(1 hunks)app/api/schema/[schema]/count/route.ts(1 hunks)app/components/EditorComponent.tsx(9 hunks)app/components/Header.tsx(1 hunks)app/settings/page.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/EditorComponent.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/settings/page.tsx (1)
lib/utils.ts (1)
cn(52-54)
🪛 GitHub Check: CodeQL
app/api/graph/[graph]/count/route.ts
[failure] 17-22: Server-side request forgery
The URL of this request depends on a user-provided value.
app/api/schema/[schema]/count/route.ts
[failure] 18-23: Server-side request forgery
The URL of this request depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run settings tests
🔇 Additional comments (3)
app/components/Header.tsx (2)
26-32: 🛠️ Refactor suggestionProps are now required but lack proper TypeScript validation
The component props have been changed from optional to required, but the interface definition doesn't reflect this properly.
interface Props { - onSetGraphName: (newGraphName: string) => void - graphNames: string[] + onSetGraphName: (newGraphName: string) => void; + graphNames: string[]; }Likely an incorrect or invalid review comment.
86-86:❓ Verification inconclusive
Verify if preventDefault() is necessary on the dropdown trigger
Using
preventDefault()on the dropdown trigger might interfere with the normal behavior of the dropdown menu.
🏁 Script executed:
#!/bin/bash # Check if the DropdownMenuTrigger has onClick handlers in other places rg -A 2 "DropdownMenuTrigger" --type tsx --type jsxLength of output: 79
Let’s search for all
<DropdownMenuTrigger>usages and inspect the surrounding code inHeader.tsxfor context:
🏁 Script executed:
#!/bin/bash # Search for all DropdownMenuTrigger occurrences in .tsx/.jsx files rg -n "<DropdownMenuTrigger" -g "*.tsx" -g "*.jsx" -A 2 || true # Show lines 60–120 of Header.tsx to get full context around the preventDefault call if [ -f app/components/Header.tsx ]; then echo "== app/components/Header.tsx (lines 60–120) ==" sed -n '60,120p' app/components/Header.tsx else echo "app/components/Header.tsx not found" fiLength of output: 4720
Remove unnecessary preventDefault on DropdownMenuTrigger
The
onClick={(e) => e.preventDefault()}on your Help menu trigger is not used in any otherDropdownMenuTrigger(seeselectGraph.tsxandtoolbar.tsx) and may actually block Radix’s built-in toggle behavior. Unless you have a specific edge-case you’re preventing here, please remove this handler and verify that the Help dropdown still opens and closes as expected:• Location:
app/components/Header.tsxline 86
• Compare with usages in:
–app/graph/selectGraph.tsx(no preventDefault)
–app/graph/toolbar.tsx(no preventDefault)app/settings/page.tsx (1)
53-53: Improved tab styling with active statesThe tab styling has been updated to use foreground background for active tabs and gray text for inactive tabs, which improves visual distinction.
Also applies to: 55-55, 61-61, 67-67
| useEffect(() => { | ||
| window.addEventListener("keydown", (e) => { | ||
| if (e.key === "Escape") { | ||
| router.back() | ||
| } | ||
| }) | ||
|
|
||
| return () => { | ||
| window.removeEventListener("keydown", (e) => { | ||
| if (e.key === "Escape") { | ||
| router.back() | ||
| } | ||
| }) | ||
| } | ||
| }, [router]) |
There was a problem hiding this comment.
Fix memory leak in event listener cleanup
The cleanup function creates a new function reference instead of removing the original listener, causing a memory leak.
useEffect(() => {
+ const handleEscapeKey = (e: KeyboardEvent) => {
+ if (e.key === "Escape") {
+ router.back()
+ }
+ }
- window.addEventListener("keydown", (e) => {
- if (e.key === "Escape") {
- router.back()
- }
- })
+ window.addEventListener("keydown", handleEscapeKey)
return () => {
- window.removeEventListener("keydown", (e) => {
- if (e.key === "Escape") {
- router.back()
- }
- })
+ window.removeEventListener("keydown", handleEscapeKey)
}
}, [router])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| window.addEventListener("keydown", (e) => { | |
| if (e.key === "Escape") { | |
| router.back() | |
| } | |
| }) | |
| return () => { | |
| window.removeEventListener("keydown", (e) => { | |
| if (e.key === "Escape") { | |
| router.back() | |
| } | |
| }) | |
| } | |
| }, [router]) | |
| useEffect(() => { | |
| const handleEscapeKey = (e: KeyboardEvent) => { | |
| if (e.key === "Escape") { | |
| router.back() | |
| } | |
| } | |
| window.addEventListener("keydown", handleEscapeKey) | |
| return () => { | |
| window.removeEventListener("keydown", handleEscapeKey) | |
| } | |
| }, [router]) |
🤖 Prompt for AI Agents
In app/settings/page.tsx around lines 18 to 32, the useEffect hook adds a
keydown event listener with an inline function but attempts to remove it with a
new inline function, which does not match the original reference and causes a
memory leak. To fix this, define the event handler function separately outside
the addEventListener and removeEventListener calls, then use that same function
reference in both to properly add and remove the listener.
|
|
||
| const json = await result.json() | ||
|
|
||
| const data = typeof json.result === "number" ? json.result : { data: [json.result.data[0] || { edges: 0, nodes: 0 }] } |
There was a problem hiding this comment.
Fix data transformation logic
The current data transformation is incorrect and inconsistent. The ternary operator creates different data structures based on the type of json.result, which can lead to unexpected behavior.
- const data = typeof json.result === "number" ? json.result : { data: [json.result.data[0] || { edges: 0, nodes: 0 }] }
+ let data;
+ if (typeof json.result === "number") {
+ data = { data: [{ edges: 0, nodes: json.result }] };
+ } else if (json.result?.data?.[0]) {
+ data = { data: [json.result.data[0]] };
+ } else {
+ data = { data: [{ edges: 0, nodes: 0 }] };
+ }This ensures a consistent data structure regardless of the input format, and includes proper validation of nested properties.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data = typeof json.result === "number" ? json.result : { data: [json.result.data[0] || { edges: 0, nodes: 0 }] } | |
| let data; | |
| if (typeof json.result === "number") { | |
| data = { data: [{ edges: 0, nodes: json.result }] }; | |
| } else if (json.result?.data?.[0]) { | |
| data = { data: [json.result.data[0]] }; | |
| } else { | |
| data = { data: [{ edges: 0, nodes: 0 }] }; | |
| } |
🤖 Prompt for AI Agents
In app/api/graph/[graph]/count/route.ts at line 28, the data transformation uses
a ternary operator that results in inconsistent data structures depending on
whether json.result is a number or an object. To fix this, refactor the logic to
always produce a consistent data structure by validating nested properties
properly and ensuring the output format is uniform regardless of input type.
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${graph}/?query=${encodeURIComponent(query)}`, { | ||
| method: "GET", | ||
| headers: { | ||
| cookie: request.headers.get('cookie') || '', | ||
| } | ||
| }) |
There was a problem hiding this comment.
Mitigate potential SSRF vulnerability by validating the graph parameter
The URL of the fetch request is constructed using a user-provided value (graph parameter). While this is an internal request within the same application, it's still important to validate this parameter to prevent path traversal or other injection attacks.
const { graph } = await params
+
+ // Validate graph parameter to prevent path traversal or injection
+ if (!graph || /[^a-zA-Z0-9_-]/.test(graph)) {
+ return NextResponse.json({ error: "Invalid graph name" }, { status: 400 })
+ }
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${graph}/?query=${encodeURIComponent(query)}`, { | |
| method: "GET", | |
| headers: { | |
| cookie: request.headers.get('cookie') || '', | |
| } | |
| }) | |
| const { graph } = await params | |
| // Validate graph parameter to prevent path traversal or injection | |
| if (!graph || /[^a-zA-Z0-9_-]/.test(graph)) { | |
| return NextResponse.json({ error: "Invalid graph name" }, { status: 400 }) | |
| } | |
| try { | |
| const result = await fetch( | |
| `${request.nextUrl.origin}/api/graph/${graph}/?query=${encodeURIComponent(query)}`, | |
| { | |
| method: "GET", | |
| headers: { | |
| cookie: request.headers.get('cookie') || '', | |
| }, | |
| } | |
| ) | |
| // … |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 17-22: Server-side request forgery
The URL of this request depends on a user-provided value.
🤖 Prompt for AI Agents
In app/api/graph/[graph]/count/route.ts around lines 17 to 22, the fetch URL
uses the user-provided graph parameter directly, which can lead to SSRF or path
traversal vulnerabilities. To fix this, validate the graph parameter against a
whitelist of allowed values or ensure it matches a strict pattern (e.g.,
alphanumeric and limited length) before using it in the URL. Reject or sanitize
any invalid input to prevent injection attacks.
|
|
||
| const result = data.length === 0 ? { nodes: 0, edges: 0 } : data[0] | ||
| const data = typeof json.result === "number" ? json.result : { data: [json.result.data[0] || { nodes: 0, edges: 0 }] } |
There was a problem hiding this comment.
Fix data transformation logic
The current data transformation is incorrect and inconsistent. The ternary operator creates different data structures based on the type of json.result, which can lead to unexpected behavior.
- const data = typeof json.result === "number" ? json.result : { data: [json.result.data[0] || { nodes: 0, edges: 0 }] }
+ let data;
+ if (typeof json.result === "number") {
+ data = { data: [{ nodes: json.result, edges: 0 }] };
+ } else if (json.result?.data?.[0]) {
+ data = { data: [json.result.data[0]] };
+ } else {
+ data = { data: [{ nodes: 0, edges: 0 }] };
+ }This ensures a consistent data structure regardless of the input format, and includes proper validation of nested properties.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const data = typeof json.result === "number" ? json.result : { data: [json.result.data[0] || { nodes: 0, edges: 0 }] } | |
| let data; | |
| if (typeof json.result === "number") { | |
| data = { data: [{ nodes: json.result, edges: 0 }] }; | |
| } else if (json.result?.data?.[0]) { | |
| data = { data: [json.result.data[0]] }; | |
| } else { | |
| data = { data: [{ nodes: 0, edges: 0 }] }; | |
| } |
🤖 Prompt for AI Agents
In app/api/schema/[schema]/count/route.ts at line 29, the data transformation
uses a ternary operator that produces inconsistent data structures depending on
whether json.result is a number or an object. To fix this, refactor the logic to
always produce a consistent data structure by validating nested properties
properly and ensuring the output format is uniform regardless of input type.
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, { | ||
| method: "GET", | ||
| headers: { | ||
| cookie: request.headers.get('cookie') || '', | ||
| } | ||
| }) |
There was a problem hiding this comment.
Mitigate potential SSRF vulnerability by validating the schema parameter
The URL of the fetch request is constructed using a user-provided value (schema parameter). While this is an internal request within the same application, it's still important to validate this parameter to prevent path traversal or other injection attacks.
const { schema } = await params
+
+ // Validate schema parameter to prevent path traversal or injection
+ if (!schema || /[^a-zA-Z0-9_-]/.test(schema)) {
+ return NextResponse.json({ error: "Invalid schema name" }, { status: 400 })
+ }
const schemaName = `${schema}_schema`
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await fetch(`${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, { | |
| method: "GET", | |
| headers: { | |
| cookie: request.headers.get('cookie') || '', | |
| } | |
| }) | |
| const { schema } = await params | |
| // Validate schema parameter to prevent path traversal or injection | |
| if (!schema || /[^a-zA-Z0-9_-]/.test(schema)) { | |
| return NextResponse.json({ error: "Invalid schema name" }, { status: 400 }) | |
| } | |
| const schemaName = `${schema}_schema` | |
| try { | |
| const result = await fetch( | |
| `${request.nextUrl.origin}/api/graph/${schemaName}/?query=${encodeURIComponent(query)}`, | |
| { | |
| method: "GET", | |
| headers: { | |
| cookie: request.headers.get('cookie') || '', | |
| }, | |
| } | |
| ) | |
| // … | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 18-23: Server-side request forgery
The URL of this request depends on a user-provided value.
🤖 Prompt for AI Agents
In app/api/schema/[schema]/count/route.ts around lines 18 to 23, the fetch URL
is constructed using the user-provided schema parameter without validation,
which risks SSRF or injection attacks. To fix this, validate the schemaName
parameter against a whitelist of allowed schema names or use strict pattern
matching to ensure it contains only safe characters before including it in the
URL. Reject or sanitize any invalid input to prevent malicious URL manipulation.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores