Remove personal access token #1264
Conversation
…readability. Added GET method for schema elements, updated controls layout, and removed unused effects in Selector. Adjusted Configurations component for better styling and commented out Tokens section in Settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR disables the run-token-tests workflow job, adds route handlers for graph elements and schema attributes with session-based access control, refactors graph UI (Selector and controls) to use node/edge add-state and layout tweaks, updates Settings UI and API payloads, and reorganizes e2e API response types and token test utilities with temporary-token cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Route Handler
participant Auth as getClient
participant DB as Query Engine
participant Logger as Console
Client->>Route: HTTP {GET/POST/PATCH/DELETE} /api/...
Route->>Auth: getClient(request)
Auth-->>Route: client (session, role)
Route->>Route: validateBody(request) (if applicable)
alt validation fails
Route->>Logger: log validation error
Route-->>Client: 400 + { error }
else validation succeeds
alt role == full
Route->>DB: query(...)
else
Route->>DB: roQuery(...)
end
DB-->>Route: result / affected rows
Route-->>Client: 200 + JSON result
end
alt server error
Route->>Logger: log error
Route-->>Client: 500 + { error }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Suggested PR titles:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
e2e/infra/utils.ts (1)
21-29: ChangerefreshIntervale2e default from "60" to "10" to match app initializationThe e2e default of
"60"is the maximum slider value and does not match the app's actual defaults. The app initializesrefreshIntervalto10seconds (app/providers.tsx:70) with a localStorage fallback of30seconds (app/providers.tsx:420). Using the maximum value in e2e tests could mask bugs and create inconsistency. Change the e2e default to"10"to match the app's primary initialization.app/components/graph/UploadGraph.tsx (1)
58-58: Form submission doesn't prevent default.The
onSubmithandler should prevent the default form submission behavior to avoid page reload.- <form onSubmit={onUploadData} className="grow p-4 flex flex-col gap-6"> + <form onSubmit={(e) => { e.preventDefault(); onUploadData(); }} className="grow p-4 flex flex-col gap-6">app/settings/users/Users.tsx (1)
159-161: Non-null assertion onnewUsercould throw.The
handleSetRole(newUser!)assumesnewUseris never null when clicked, but there's no guard. Consider disabling the button whennewUseris null.- <Button onClick={() => handleSetRole(newUser!)}>Set User</Button> + <Button + onClick={() => newUser && handleSetRole(newUser)} + disabled={!newUser} + > + Set User + </Button>app/components/TableComponent.tsx (3)
183-199: Duplicate useEffect for focusing search input.Lines 183-187 and 195-199 both focus
searchRef.currenton mount. Remove one of these duplicate effects.- useEffect(() => { - if (searchRef.current) { - searchRef.current.focus() - } - }, []) - useEffect(() => { if (inputRef && inputRef.current && editable) { inputRef.current.focus() } }, [inputRef, editable]) useEffect(() => { if (searchRef.current) { searchRef.current.focus() } }, [])
493-499: Avoid mutating row objects directly.The checkbox change handler mutates
r.checkeddirectly instead of returning a new object. This can cause React reconciliation issues since the object reference doesn't change.onCheckedChange={() => { - setRows(rows.map((r) => { - if (r.name === row.name) { - r.checked = !r.checked - } - return r - })) + setRows(prevRows => prevRows.map((r) => + r.name === row.name + ? { ...r, checked: !r.checked } + : r + )) }}
377-392: Same mutation issue in header checkbox.The "select all" checkbox handler also mutates row objects directly.
onCheckedChange={() => { const checked = rows.every(row => row.checked) - setRows(rows.map((row) => { - row.checked = !checked - return row - })) + setRows(rows.map((row) => ({ + ...row, + checked: !checked + }))) }}app/api/schema/[schema]/[element]/label/_route.ts (1)
2-56: Sanitizelabeland validateelementIdto avoid Cypher injection and invalid IDsThe switch to
validateBodyand parameterized$idis a solid step forward, but two things are still worth tightening up here:
Label safety:
labelcomes from the request body and is interpolated directly into the Cypher in both handlers:const query = `MATCH (n) WHERE ID(n) = $id SET n:${label}`; // and const query = `MATCH (n) WHERE ID(n) = $id REMOVE n:${label}`;Since the Zod schemas for
addSchemaElementLabel/removeSchemaElementLabelonly enforce “non‑empty string”, a crafted label could break the query or be used for injection. At minimum, enforce an allowed character set (e.g., labels starting with a letter/underscore and containing only letters, digits, and underscores) before building the query.For example:
const { label } = validation.data;
const { label } = validation.data;// Ensure label is in a safe format (alphanumeric + underscore) to avoid Cypher injectionif (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(label)) {return NextResponse.json({ message: "Invalid label format" },{ status: 400 });}Apply the same check in both POST and DELETE.
Element ID validation:
const elementId = Number(node);will yieldNaNfor bad input, which then goes into{ params: { id: elementId } }. You could fail fast instead:
- const elementId = Number(node);
- const elementId = Number(node);
- if (!Number.isInteger(elementId) || elementId < 0) {
return NextResponse.json({ message: "Invalid element id" },{ status: 400 });- }
These changes would close the remaining gaps while keeping the current route structure intact. Also applies to: 59-112 </blockquote></details> <details> <summary>app/api/schema/[schema]/[element]/_route.ts (1)</summary><blockquote> `36-41`: **Incorrect empty array check for `label`.** `label` is `z.array(z.string())`, so `!label` will always be `false` (empty arrays are truthy). This validation never triggers. ```diff if (!type) { - if (!label) throw new Error("Label is required"); + if (!label || label.length === 0) throw new Error("Label is required"); if (!selectedNodes || selectedNodes.length !== 2) throw new Error("Selected nodes are required"); }app/api/auth/[...nextauth]/options.ts (1)
10-20:isValidJWTPayloadshould validatejtipresence since it's required inCustomJWTPayloadand directly used at line 279.The type guard at line 189 only validates
sub,host, andportbut doesn't check forjti, despite it being marked as required in the interface. WhengetPasswordFromTokenDB(payload.jti)is called at line 279, a missingjtiwould pass undefined to a function expecting a string. Addjtivalidation to the type guard:return Boolean(p.sub && p.host && p.port && p.jti);
🟡 Minor comments (20)
app/components/TableComponent.tsx-92-169 (1)
92-169: Stale closure risk in lazy loading check.The
handleLoadLazyCellcallback capturesloadingCellsandloadedCellsin its closure, but these checks happen synchronously at call time. Due to React's batched state updates, multiple cells could pass the guard simultaneously before state updates propagate.Consider using refs for the tracking sets or moving the check inside the state setter:
- // If already loading or loaded, don't load again - if (loadingCells.has(cellKey) || loadedCells.has(cellKey)) { - return; - } + // Use functional update to check current state atomically + setLoadingCells(prev => { + if (prev.has(cellKey) || loadedCells.has(cellKey)) { + return prev; // No change, already loading/loaded + } + // Mark as loading and proceed + setTimeout(() => loadFn().then(...), 0); + return new Set(prev).add(cellKey); + });Alternatively, since
loadAttemptedRefalready tracks attempts synchronously, the current implementation may work in practice.Committable suggestion skipped: line range outside the PR's diff.
app/api/chat/route.tsx-66-76 (1)
66-76: Non-standard SSE format for validation errors.The error event on line 67 uses
event: error status: ${400} data: ...which doesn't follow the SSE specification. Standard format uses separate lines for event type and data.Consider aligning with the format used elsewhere in this file (e.g., lines 137, 142):
-writer.write(encoder.encode(`event: error status: ${400} data: ${JSON.stringify(validation.error)}\n\n`)) +writer.write(encoder.encode(`event: error\ndata: ${JSON.stringify({ message: validation.error, status: 400 })}\n\n`))app/api/chat/route.tsx-5-5 (1)
5-5: Trailing slash inconsistency in URL construction.The default value includes a trailing slash, but if
CHAT_URLis set without one, the URL concatenation on lines 16 and 91 will produce malformed URLs (e.g.,http://example.comconfigured-model).Consider normalizing the URL:
-const CHAT_URL = process.env.CHAT_URL || "http://localhost:8000/" +const CHAT_URL = (process.env.CHAT_URL || "http://localhost:8000").replace(/\/$/, "");Then update the fetch calls:
-const response = await fetch(`${CHAT_URL}configured-model`, { +const response = await fetch(`${CHAT_URL}/configured-model`, {-const response = await fetch(`${CHAT_URL}text_to_cypher`, { +const response = await fetch(`${CHAT_URL}/text_to_cypher`, {Committable suggestion skipped: line range outside the PR's diff.
app/api/graph/[graph]/[element]/[key]/route.ts-24-24 (1)
24-24: Validateelementparam parses to a valid number.
Number(element)will produceNaNfor invalid strings, which may cause unexpected behavior in the query.const elementId = Number(element); + +if (Number.isNaN(elementId)) { + return NextResponse.json( + { message: "Invalid element ID" }, + { status: 400 } + ); +}app/schema/DataPanel.tsx-516-516 (1)
516-516: Same optional chaining inconsistency.Apply the same fix here for consistency with line 353.
- session?.user.role !== "Read-Only" && ( + session?.user?.role !== "Read-Only" && (app/schema/DataPanel.tsx-337-337 (1)
337-337: Inconsistent optional chaining onuser.roleLine 337 uses
session?.user.rolebut line 353 usessession?.user?.role. Ifsession?.usercould be undefined, direct property access to.rolewould throw. Align the optional chaining pattern across all occurrences (lines 337, 516, 715 vs 353).- type && session?.user.role !== "Read-Only" && + type && session?.user?.role !== "Read-Only" &&app/schema/DataPanel.tsx-715-715 (1)
715-715: Same optional chaining inconsistency.Apply the same fix here for consistency with line 353.
- session?.user.role !== "Read-Only" && + session?.user?.role !== "Read-Only" &&app/api/graph/[graph]/count/edges/route.ts-65-68 (1)
65-68: Potential double-close on abort handler.The abort listener is registered after the stream may already be closed (lines 39, 50, 62). Calling
writer.close()on an already-closed writer throws. Wrap in try-catch or guard with a flag.+ let closed = false; + const safeClose = () => { + if (!closed) { + closed = true; + writer.close(); + } + }; // Clean up if the client disconnects early request.signal.addEventListener("abort", () => { - writer.close(); + safeClose(); });Then replace other
writer.close()calls withsafeClose().Committable suggestion skipped: line range outside the PR's diff.
app/api/graph/[graph]/[element]/route.ts-152-152 (1)
152-152: Missing NaN validation for elementId in DELETE.Same issue as GET - validate the parsed element ID.
app/api/graph/[graph]/[element]/route.ts-22-22 (1)
22-22: Missing NaN validation for elementId.
Number(element)will produceNaNfor non-numeric strings, which could cause unexpected query behavior. Validate before use.const { graph: graphId, element } = await params; const elementId = Number(element); + + if (Number.isNaN(elementId)) { + return NextResponse.json( + { message: "Invalid element ID" }, + { status: 400 } + ); + }app/api/auth/tokens/[tokenId]/route.ts-226-232 (1)
226-232: Revoker user lookup may fail silently.The query assumes the revoker user exists. If
authenticatedUser.usernamedoesn't match anyUsernode (e.g., JWT-only auth without a stored user), theMATCH (revoker:User ...)will yield no results, and the revocation fails with a generic error.Consider using
OPTIONAL MATCHfor the revoker or creating the relationship conditionally:const revokeQuery = ` MATCH (t:Token {token_id: '${escapeString(tokenId)}'})-[:BELONGS_TO]->(u:User) - MATCH (revoker:User {username: '${escapeString(revokerUsername)}'}) + OPTIONAL MATCH (revoker:User {username: '${escapeString(revokerUsername)}'}) SET t.is_active = false - CREATE (t)-[:REVOKED_BY {at: ${nowUnix}}]->(revoker) + FOREACH (_ IN CASE WHEN revoker IS NOT NULL THEN [1] ELSE [] END | + CREATE (t)-[:REVOKED_BY {at: ${nowUnix}}]->(revoker) + ) RETURN t.token_id as token_id `;app/graph/CreateElementPanel.tsx-144-149 (1)
144-149: Global Escape listener may conflict with nested dialogs.The component registers a window-level
keydownlistener that closes the panel on Escape. This could interfere with nestedDialogComponentinstances (e.g., delete confirmation) where Escape should only close the inner dialog.Consider stopping propagation in nested dialogs or checking if the panel itself should handle the escape:
useEffect(() => { + const handler = (e: KeyboardEvent) => { + // Only close if no dialog is open + if (e.key === "Escape" && !document.querySelector('[role="dialog"][data-state="open"]')) { + handleClose(e) + } + } - window.addEventListener("keydown", handleClose) + window.addEventListener("keydown", handler) return () => { - window.removeEventListener("keydown", handleClose) + window.removeEventListener("keydown", handler) } }, [handleClose])app/graph/DataPanel.tsx-162-162 (1)
162-162: Optional chaining removed onuser.roleaccess.Changed from
session?.user?.roletosession?.user.role. Ifsession.userisundefined, this will throw a runtime error. The original optional chaining was safer.- type && l && session?.user.role !== "Read-Only" && + type && l && session?.user?.role !== "Read-Only" &&app/graph/DataPanel.tsx-181-181 (1)
181-181: Same optional chaining issue.Apply consistent optional chaining for role access.
- type && (labelsHover || label.length === 0) && session?.user?.role !== "Read-Only" && + type && (labelsHover || label.length === 0) && session?.user?.role !== "Read-Only" &&Committable suggestion skipped: line range outside the PR's diff.
app/api/auth/tokens/credentials/route.ts-205-209 (1)
205-209: Silent storage failure allows unmanageable tokens.When token storage fails, the token is still returned to the user. This creates "orphan" tokens that work but can't be revoked via the UI. Consider returning the storage failure to the client or at least including a warning in the response.
} catch (storageError) { // eslint-disable-next-line no-console console.error('Failed to store token in FalkorDB for user:', authenticatedUser.username, storageError); - // Continue - token will still work but can't be managed via UI + return NextResponse.json( + { message: "Token created but storage failed - token cannot be managed via UI", token }, + { status: 201 } + ); }Committable suggestion skipped: line range outside the PR's diff.
e2e/logic/POM/settingsTokensPage.ts-295-302 (1)
295-302: Unused return value inverifyTokenExists.
isTokenRowVisiblereturns a boolean, but it's not used. IfinteractWhenVisiblethrows when the element isn't visible, this logic works. However, if it returnsfalse, the function incorrectly returnstrue. Consider clarifying the intent.async verifyTokenExists(name: string): Promise<boolean> { try { - await this.isTokenRowVisible(name); - return true; + return await this.isTokenRowVisible(name); } catch { return false; } }app/providers.tsx-175-175 (1)
175-175: Duplicate dependency in useMemo array.
navigateToSettingsappears twice in the dependency array.- }), [displayChat, navigateToSettings, contentPersistence, defaultQuery, hasChanges, lastLimit, limit, model, navigateToSettings, newContentPersistence, newDefaultQuery, newLimit, newModel, newRefreshInterval, newRunDefaultQuery, newSecretKey, newTimeout, refreshInterval, runDefaultQuery, secretKey, timeout, displayTextPriority, newDisplayTextPriority, replayTutorial, tutorialOpen]) + }), [displayChat, navigateToSettings, contentPersistence, defaultQuery, hasChanges, lastLimit, limit, model, newContentPersistence, newDefaultQuery, newLimit, newModel, newRefreshInterval, newRunDefaultQuery, newSecretKey, newTimeout, refreshInterval, runDefaultQuery, secretKey, timeout, displayTextPriority, newDisplayTextPriority, replayTutorial, tutorialOpen, showMemoryUsage])Note: Also added
showMemoryUsagesince it's used insettings.graphInfo.app/api/swagger/swagger-spec.ts-94-98 (1)
94-98: Port should be typeintegerper OpenAPI best practices.The
portfield is defined as typestringin both the Swagger spec and Zod validation schema, but the implementation inapp/api/auth/[...nextauth]/options.ts(line 63) explicitly parses it to an integer:parseInt(credentials.port, 10). According to OpenAPI specifications, port numbers should be represented astype: "integer"withformat: "int32"and validation constraints (minimum: 1, maximum: 65535). Update the Swagger schema to reflect the semantic type and consider using Zod's.coerce.number()for validation consistency.app/graph/page.tsx-192-202 (1)
192-202: Missing leading slash in API URLLine 194 uses
api/graph/...but other API calls in this file (e.g., line 104) use/api/graph/.... This inconsistency could cause routing issues depending on the current browser path.- const result = await securedFetch(`api/graph/${prepareArg(graphName)}/${fakeId}`, { + const result = await securedFetch(`/api/graph/${prepareArg(graphName)}/${fakeId}`, {app/schema/CreateElementPanel.tsx-635-638 (1)
635-638: Type assertion may be unsafeThe filter
prev.filter((e): e is Node => !!e.labels)doesn't guarantee exactly two nodes. If the filtered result has more or fewer elements, theas [Node, Node]assertion could lead to runtime issues.Consider adding a guard or using the existing
isTwoNodesutility:- onClick={() => setSelectedNodes && setSelectedNodes(prev => { - const nodes = prev.filter((e): e is Node => !!e.labels) as [Node, Node] - return [nodes[1], nodes[0]] - })} + onClick={() => setSelectedNodes && setSelectedNodes(prev => { + const nodes = prev.filter((e): e is Node => !!e.labels) + if (nodes.length !== 2) return prev + return [nodes[1], nodes[0]] + })}
🧹 Nitpick comments (56)
app/globals.css (1)
52-57: Verify visual impact of lighter font weight.The SofiaSans font weight reduced from 400 to 200 is significantly lighter. Combined with the added word-spacing, this could affect readability—especially for body text or longer content. Please verify the visual appearance in the UI matches design intent.
app/api/monitor/route.ts (1)
48-48: Good addition for debugging visibility.Adding
console.error(err)before the 500 response improves observability and aligns with the inner catch pattern at line 41.Optional enhancement: Consider structured logging (e.g., using a logging library) with error sanitization to prevent potential exposure of sensitive data in production logs.
app/graph/TableView.tsx (1)
17-22: Consider using Set for more efficient headers computation.The current
reduce+includes()approach has O(n×m×h) complexity. Using a Set would reduce it to O(n×m):- const headers = graph.Data.reduce<string[]>((acc, row) => { - Object.keys(row).forEach((key) => { - if (!acc.includes(key)) acc.push(key); - }); - return acc; - }, []); + const headersSet = new Set<string>(); + graph.Data.forEach((row) => { + Object.keys(row).forEach((key) => headersSet.add(key)); + }); + const headers = Array.from(headersSet);app/graph/controls.tsx (1)
52-59: Consider adding an aria-label to the Switch for screen reader users.The Switch is wrapped in a Tooltip which provides visual context ("Animation Control") on hover, but screen readers may not automatically associate the tooltip content with the Switch control.
Apply this diff to improve accessibility:
<Switch data-testid="animationControl" + aria-label="Animation Control" className="pointer-events-auto data-[state=unchecked]:bg-border" checked={cooldownTicks === undefined} onCheckedChange={() => { handleCooldown(cooldownTicks === undefined ? 0 : undefined, false) }} />e2e/infra/ui/browserWrapper.ts (1)
71-85: Teardown order looks good; consider clearing browser/context references after closeCalling
await this.closePage()before closing the browser is a solid improvement and matches the comment about clearing intervals first. If aBrowserWrapperinstance is ever reused aftercloseBrowserruns, though,createNewPagewill see non‑nullthis.browser/this.contextand try to use already‑closed Playwright objects.To make reuse safe and avoid subtle “browser/context already closed” errors, consider explicitly nulling these references after a successful close:
async closeBrowser() { // Close the page first (which clears intervals) before closing the browser await this.closePage(); - - if (this.browser) { - await this.browser.close(); - } + + if (this.browser) { + await this.browser.close(); + this.browser = null; + } + + // Context becomes invalid once the browser is closed + this.context = null; }Possible conventional‑commit style PR titles that fit this change set:
test(e2e): close page before browser in BrowserWrapperchore(e2e): harden Playwright browser teardown.github/workflows/release-image.yml (1)
20-28: Add error handling and timeout for GitHub API call.The curl command fetching the latest release has no timeout, error handling, or response validation. If the API is unavailable or rate-limited,
CYPHER_VERSIONwill be empty, causing a downstream build failure that's hard to debug.Consider:
- Adding
--max-timeflag for timeout- Checking curl exit code
- Validating that
LATEST_VERSIONis not empty before using it- Adding a fallback version as safety net
- name: Set up variables run: | if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then echo "CYPHER_VERSION=${{ github.event.inputs.cypher_version }}" >> $GITHUB_ENV else # Get the latest released version from GitHub releases - LATEST_VERSION=$(curl -s https://api.github.com/repos/falkordb/text-to-cypher/releases/latest | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/') + LATEST_VERSION=$(curl -s --max-time 10 https://api.github.com/repos/falkordb/text-to-cypher/releases/latest | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/' || echo "") + if [ -z "$LATEST_VERSION" ]; then + echo "Failed to fetch latest version, using fallback: v0.1.5" + LATEST_VERSION="v0.1.5" + fi echo "CYPHER_VERSION=$LATEST_VERSION" >> $GITHUB_ENV fiDockerfile (1)
1-5: Dockerfile CYPHER_VERSION default differs from workflow.The Dockerfile defaults to
latestwhile the workflow defaults tov0.1.5. For build reproducibility, consider aligning these defaults.-ARG CYPHER_VERSION=latest +ARG CYPHER_VERSION=v0.1.5app/components/EditorComponent.tsx (1)
60-62: Centralized sizing constants are good; double-check lineHeight vs fontSize, especially in dialogCentralizing
MAX_HEIGHT,LINE_HEIGHT, andPLACEHOLDER, and wiringLINE_HEIGHTthroughmonacoOptions,editorHeight, and the toolbar container improves consistency.One thing to verify: in the dialog editor you now use
lineHeight: LINE_HEIGHT(22) withfontSize: 25. Depending on Monaco’s font metrics, this can make lines feel cramped or even clip ascenders/descenders. In the main editorfontSize(20) is closer toLINE_HEIGHT(22), so it’s less of a concern there.Consider either:
- Using a slightly larger line height for the dialog (e.g. a separate
DIALOG_LINE_HEIGHT), or- Deriving
LINE_HEIGHTfrom the chosenfontSize(e.g.Math.round(fontSize * 1.3)), at least for the larger dialog editor.Also applies to: 79-82, 677-677, 765-767
e2e/tests/cluster.spec.ts (2)
75-86: Prefer a simplefor...ofloop overreducefor sequential async stepsFunctionally this works, but the Promise
reducepattern makes the test harder to read and reason about than a straightforward loop in anasynctest body. A plainfor...ofwithawaitkeeps the same sequential semantics and matches common Playwright style.You could simplify as:
- await CLUSTER_NODES.reduce( - (chain, node) => - chain.then(async () => { - await loginPage.fillHost(node.host); - await loginPage.fillPort(node.port); - expect(await loginPage.getHost()).toBe(node.host); - expect(await loginPage.getPort()).toBe(node.port); - await loginPage.fillHost(""); - await loginPage.fillPort(""); - }), - Promise.resolve() - ); + for (const node of CLUSTER_NODES) { + await loginPage.fillHost(node.host); + await loginPage.fillPort(node.port); + expect(await loginPage.getHost()).toBe(node.host); + expect(await loginPage.getPort()).toBe(node.port); + await loginPage.fillHost(""); + await loginPage.fillPort(""); + }If you want slightly stronger coverage, you could also assert that the fields are empty after the reset step.
115-117: Tidy up or replace commented-out schema navigation assertionsThe comment explains why schema navigation is disabled, but leaving assertions commented-out tends to accumulate dead code and can confuse future readers about the intended behavior.
Two options to consider:
- If schema navigation is permanently removed: delete these lines entirely so the test reflects the current feature set.
- If the behavior changed (e.g., no schema tab anymore): replace with an explicit assertion for the new behavior (e.g., schema button not visible, or alternative navigation path).
Either way, having active expectations rather than commented code will keep this test more maintainable.
app/components/PaginationList.tsx (1)
304-336: Next 5 pages button reorder is fine; consider aligning multi-page navigation behaviorThe layout change to place
">>"after">"looks good and is more conventional. Functionally, the logic for the "Next 5 pages" button remains consistent.Two small, optional cleanups you might consider while touching this area:
- Consistent focus behavior for multi-page jumps
- Line 305 (
"Previous 5 pages") usessetStepCounterdirectly, while line 335 ("Next 5 pages") useshandleSetStepCounter, which also refocuses the search input.- For consistent keyboard/focus behavior when jumping by 5 pages, consider routing the previous-5 button through
handleSetStepCounteras well:- <Button disabled={stepCounter < 4} label="<<" title="Previous 5 pages" onClick={() => setStepCounter(prev => prev > 4 ? prev - 5 : prev)} /> + <Button + disabled={stepCounter < 4} + label="<<" + title="Previous 5 pages" + onClick={() => handleSetStepCounter(prev => prev > 4 ? prev - 5 : prev)} + />
- Avoid enabled-but-no-op state on "Previous 5 pages"
- With
disabled={stepCounter < 4}and the guardprev > 4 ? prev - 5 : prev, the button is enabled atstepCounter === 4but the click is a no-op.- If you want the button disabled whenever a 5-page jump isn’t possible, adjust the disabled condition:
- <Button - disabled={stepCounter < 4} + <Button + disabled={stepCounter <= 4} label="<<" title="Previous 5 pages" onClick={() => handleSetStepCounter(prev => prev > 4 ? prev - 5 : prev)} />These are minor UX consistency tweaks and not blockers.
Suggested conventional-commit style PR title:
fix: improve pagination navigation order.components/ui/drawer.tsx (1)
58-60: Consider simplifying handle markup and marking it as presentationalThe handle’s
classNameexpression is quite dense, and since it’s a purely visual affordance (the drawer itself handles interaction), you can improve readability and clarify accessibility intent with a small refactor.For example:
- <div className={cn("mx-auto mt-2 h-2 w-[100px] rounded-full bg-muted cursor-grab active:cursor-grabbing", (side === "left" || side === "right") && "w-2 h-[100px] my-auto", side === "left" && "mr-2", side === "right" && "ml-2")} /> + <div + aria-hidden="true" + className={cn( + "mx-auto mt-2 h-2 w-[100px] rounded-full bg-muted cursor-grab active:cursor-grabbing", + (side === "left" || side === "right") && "w-2 h-[100px] my-auto", + side === "left" && "mr-2", + side === "right" && "ml-2", + )} + />This keeps behavior the same, but makes the intent clearer for future readers and assistive tech.
app/login/LoginForm.tsx (1)
152-158: URL validation regex and message look sound; consider extraction + testsThe new regex correctly enforces
falkor[s]://andredis[s]://with optionalusername:password@, host, port, and/db-number, and the error message matches that contract. For readability and future updates, consider extracting the pattern (and perhaps a small validator helper) to a shared constant and adding unit tests for typical positive/negative cases.app/graph/DeleteElement.tsx (1)
31-44: RedundantsetIsLoading(false)in finally block.When
setOpen(false)is called on Line 41, theuseEffect(Lines 31-35) will already resetisLoadingtofalse. Thefinallyblock on Line 43 then sets it again.This is harmless but slightly redundant. Consider removing the finally block:
const handleDelete = async () => { - try { - setIsLoading(true) - await onDeleteElement() - setOpen(false) - } finally { - setIsLoading(false) - } + setIsLoading(true) + await onDeleteElement() + setOpen(false) }Alternatively, keep the
finallyfor explicit cleanup ifonDeleteElementthrows beforesetOpen(false)is reached.app/components/graph/UploadGraph.tsx (1)
17-20: SimplifydialogOpencomputation.The
useMemoadds overhead for a simple ternary. SinceopenandinternalOpenare both primitives, React's normal rendering would handle this efficiently.- const dialogOpen = useMemo( - () => (isControlled ? (open as boolean) : internalOpen), - [isControlled, open, internalOpen] - ) + const dialogOpen = isControlled ? open : internalOpenapp/graph/DataTable.tsx (2)
63-94: Consider debouncing resize measurements.The
measureValueOverflowfunction runs on every resize event and ResizeObserver callback. For components with many attributes, this could cause performance issues.+import { useMemo, useCallback, useLayoutEffect, useRef, useState, Fragment, useEffect, useContext } from "react" + +// Add a simple debounce helper or use lodash +const debounce = <T extends (...args: any[]) => void>(fn: T, ms: number) => { + let timeoutId: ReturnType<typeof setTimeout> + return (...args: Parameters<T>) => { + clearTimeout(timeoutId) + timeoutId = setTimeout(() => fn(...args), ms) + } +} // Then wrap measureValueOverflow: +const debouncedMeasure = useMemo( + () => debounce(measureValueOverflow, 100), + [measureValueOverflow] +)
96-104: Missing cleanup for debounced handler if implemented.If debouncing is added, ensure the debounced function is cancelled on unmount and the timeout is cleared.
.github/workflows/playwright.yml (1)
272-283: Consider adding a failure exit if app doesn't start.The startup loop doesn't exit with an error if the app fails to start within 30 seconds. Tests will run against an unavailable app, potentially causing confusing failures.
for i in {1..30}; do if curl -f http://localhost:3000 > /dev/null 2>&1; then echo "App is ready" break fi + if [ $i -eq 30 ]; then + echo "App failed to start within 30 seconds" + exit 1 + fi sleep 1 doneapp/api/validate-body.ts (2)
201-207: Consider stricter typing for messages array.Using
z.any()for the messages array bypasses type validation. If the message structure is known (e.g., OpenAI chat format), consider defining it explicitly for better runtime validation.+ const chatMessage = z.object({ + role: z.enum(["user", "assistant", "system"]), + content: z.string(), + }); + export const chatRequest = z.object({ - messages: z - .array(z.any(), { + messages: z + .array(chatMessage, { required_error: "Messages are required", invalid_type_error: "Invalid Messages", }) .min(1, "Messages are required"),
229-282: Login schema mixes creation and connection concerns.The login schema combines connection params (
host,port,tls,ca) with token creation params (name,expiresAt,ttlSeconds). Consider splitting into separate schemas for clarity:This is a nitpick - the current approach works if the API endpoint handles both concerns.
app/api/user/route.ts (1)
138-155: Consider handling partial deletion failures.
Promise.allwill reject on the first failure, potentially leaving some users deleted and others not. If atomic behavior isn't required,Promise.allSettledcould provide better visibility into which deletions succeeded or failed.-await Promise.all( - users.map(async (user) => { - await connection.aclDelUser(user.username); - }) -); +const results = await Promise.allSettled( + users.map((user) => connection.aclDelUser(user.username)) +); +const failures = results.filter((r) => r.status === "rejected"); +if (failures.length > 0) { + throw new Error(`Failed to delete ${failures.length} user(s)`); +}app/schema/_page.tsx (1)
256-291: Consider memoizinggetCurrentPanelto prevent unnecessary re-renders.
getCurrentPanelis recreated on every render. Since it depends on multiple state values and handlers, consider usinguseMemoor converting to inline JSX with proper memoization of child components.-const getCurrentPanel = () => { +const currentPanel = useMemo(() => { if (selectedElement) { return ( <DataPanel object={selectedElement} setObject={handleSetSelectedElement} schema={schema} setLabels={setLabels} /> ) } // ... rest of the logic return null -} +}, [selectedElement, isAddNode, selectedElements, handleCreateElement, handleSetIsAddNode, handleSetIsAddEdge, schema, setLabels])Then use
{currentPanel}instead of{getCurrentPanel()}at line 356.app/graph/Selector.tsx (1)
427-449: Fix formatting inconsistency in JSX.There's irregular whitespace at lines 428 and 438 which appears to be a merge artifact.
<div className="z-10 absolute bottom-4 right-8 flex gap-2"> - { - historyQuery.counter ? + {historyQuery.counter && ( <Button variant="Delete" data-testid="queryHistoryDelete" label="Delete" title="Remove selected query from history" onClick={handleDeleteQuery} /> - : undefined -} <Button + )} + <Button ref={submitQuery}e2e/tests/graph.spec.ts (1)
298-312: Consider adding animation wait to this similar test.The
@readonly Validate success when RO user attempts to execute ro-querytest at lines 298-312 also callsselectGraphByNamebut doesn't havewaitForCanvasAnimationToEnd(). If consistency is desired, consider adding it here as well.await graph.selectGraphByName(graphName); +await graph.waitForCanvasAnimationToEnd(); await graph.insertQuery("MATCH (n)-[r]->(m) RETURN n, r, m LIMIT 10");app/api/graph/[graph]/memory/route.ts (4)
7-9: Unusedrequestparameter.The
requestparameter is not used in this handler. Consider removing it or prefixing with underscore if the signature is required by Next.js.export async function GET( - request: NextRequest, + _request: NextRequest, { params }: { params: Promise<{ graph: string }> } ) {
18-27: Consider extracting shared version-check logic.Calling
getDBVersion()route handler directly works but is unconventional. If this version check is needed in multiple places, consider extracting the core logic to a shared utility function.
47-52: Add error logging for consistency.The inner catch block doesn't log the error, unlike the pattern established in other routes (e.g.,
explain/route.ts). Addingconsole.error(err)would improve debugging.} catch (err) { + console.error(err); return NextResponse.json( { message: (err as Error).message }, { status: 400 } ); }
53-58: Outer catch missing error logging.For consistency with the updated pattern in
explain/route.ts, addconsole.error(err)here as well.} catch (err) { + console.error(err); return NextResponse.json( { message: (err as Error).message }, { status: 500 } ); }.env.local.template (2)
8-8: Clarify ENCRYPTION_KEY requirements.Consider adding a comment indicating the expected format/length for
ENCRYPTION_KEY(e.g., 64-character hex string for AES-256) to help developers configure it correctly.
16-16: Add trailing newline.Per static analysis, the file is missing a trailing newline. This is a POSIX convention and can prevent issues with some tools.
PAT_FALKORDB_GRAPH_NAME=token_management +app/api/auth/DBVersion/route.ts (1)
4-31: DBVersion handler looks solid; consider documentingmoduleListshape and NOPERM semanticsThe overall pattern (getClient, NextResponse short‑circuit, inner/outer try/catch, and status mapping for
"NOPERM") is consistent and technically sound. To make this more maintainable, consider adding a brief comment or small helper to clarify whyresult[0][1]andresult[0][3]are the relevant indices and why"NOPERM"should map to HTTP 200, since both are a bit “magic” today.app/api/schema/[schema]/duplicate/_route.ts (1)
3-52: Good move to centralized validation for schema duplicationSwitching to
validateBody(duplicateSchema, body)and returning a structured 400 on validation failure makes this endpoint much safer and easier to reason about. The rest of the flow (session check, source/target naming, 500 on outer failures) is coherent and in line with the other API routes.app/api/graph/[graph]/duplicate/route.ts (1)
3-50: Graph duplication endpoint correctly migrated to validated request bodyUsing
validateBody(duplicateGraph, body)and returning a 400 with a clear message on validation failure is a solid upgrade over ad‑hoc parsing. The rest of the handler (session handling,sourceName→copy(graphId)call, and 500 fallback) is consistent with the surrounding API design.app/api/graph/[graph]/count/nodes/route.ts (1)
1-77: SSE nodes count endpoint is consistent with edges count implementationThe SSE setup (TransformStream, encoder, abort handling, headers) and the
runQuery‑basedMATCH (n) RETURN count(n) as nodesflow are aligned with the existing edges count route and look correct. If you find yourself adding more SSE endpoints, consider extracting the common “count & stream result/error” pattern into a small helper to reduce duplication.app/graph/Chat.tsx (1)
3-4: Chat request reshaping looks correct; consider typing the request bodyThe new flow that:
- reads
secretKey/modelfromBrowserSettingsContext,- constructs a JSON payload with
messages,graphName, and optionalmodel/key,- and posts it via
fetch("/api/chat", …),is coherent and aligns with a session‑based auth model instead of
securedFetch. To keep things type‑safe, you could replace theanybody with a smallChatRequestinterface or inferred type, which would also help catch future changes to/api/chat’s contract at compile time. As per coding guidelines, this would improve TS correctness without changing runtime behavior.Also applies to: 21-22, 89-113
app/api/schema/[schema]/_route.ts (1)
73-80: Schema creation via lazy initialization.The
RETURN 1query triggers schema creation if it doesn't exist. This is an implicit side effect—consider adding a brief inline comment clarifying intent.const graph = client.selectGraph(schemaName); + // Trigger graph creation by executing a minimal query await graph.query("RETURN 1");app/api/auth/encryption.ts (2)
7-19: Consider validating hex characters in ENCRYPTION_KEY.The key length is validated, but if the key contains non-hex characters,
Buffer.from(key, "hex")will silently produce a shorter or malformed buffer. This could lead to cryptographic errors at runtime.function getEncryptionKey(): Buffer { const key = process.env.ENCRYPTION_KEY; if (!key) { throw new Error("ENCRYPTION_KEY environment variable is required"); } if (key.length !== 64) { throw new Error("ENCRYPTION_KEY must be 64 hexadecimal characters (32 bytes)"); } + + if (!/^[0-9a-fA-F]+$/.test(key)) { + throw new Error("ENCRYPTION_KEY must contain only hexadecimal characters"); + } return Buffer.from(key, "hex"); }
44-63: Consider adding validation for IV and authTag buffer lengths.If the encrypted text is malformed (e.g., truncated IV or authTag), the decryption will fail with a less informative error. Validating expected lengths (12 bytes for IV, 16 bytes for authTag) would improve error messages.
const iv = Buffer.from(parts[0], "hex"); const authTag = Buffer.from(parts[1], "hex"); const encrypted = parts[2]; + + if (iv.length !== 12) { + throw new Error("Invalid IV length in encrypted text"); + } + if (authTag.length !== 16) { + throw new Error("Invalid auth tag length in encrypted text"); + } const decipher = crypto.createDecipheriv("aes-256-gcm", key, iv);app/settings/tokens/PersonalAccessTokens.tsx (1)
214-220: Handle clipboard API errors.
navigator.clipboard.writeTextcan throw if clipboard access is denied. Wrap in try-catch for better UX.- const copyToClipboard = (text: string) => { - navigator.clipboard.writeText(text); - toast({ - title: "Copied", - description: "Token copied to clipboard", - }); - }; + const copyToClipboard = async (text: string) => { + try { + await navigator.clipboard.writeText(text); + toast({ + title: "Copied", + description: "Token copied to clipboard", + }); + } catch { + toast({ + title: "Error", + description: "Failed to copy to clipboard", + variant: "destructive", + }); + } + };app/graph/graphInfo.tsx (1)
34-53: Consider edge case when memory value is exactly 0.Line 43 uses
MemoryUsage.get("total_graph_sz_mb") || "<1"which will show "<1" when the value is 0 (falsy). If 0 MB is a valid value you want to display, use nullish coalescing instead.- <p className="truncate pointer-events-auto SofiaSans">{MemoryUsage.get("total_graph_sz_mb") || "<1"} MB</p> + <p className="truncate pointer-events-auto SofiaSans">{MemoryUsage.get("total_graph_sz_mb") ?? "<1"} MB</p>app/graph/CreateElementPanel.tsx (1)
183-187: Type coercion could yield"object"for arrays.
typeof valuereturns"object"for arrays andnull. SettingeditTypeto"object"when theValueTypeunion is"string" | "number" | "boolean"may cause issues.const handleSetEditable = (key: string, value?: Value) => { setEditable(key) setEditVal(value || "") - setEditType(typeof value === "undefined" ? "string" : typeof value as ValueType) + const valueType = typeof value + const validType = valueType === "number" || valueType === "boolean" ? valueType : "string" + setEditType(typeof value === "undefined" ? "string" : validType) }app/graph/selectGraph.tsx (1)
100-106: Handle potential undefined fromgetMemoryUsage.If
getMemoryUsagefails or returns an empty map,memoryMap.get("total_graph_sz_mb")returnsundefined, which the fallback handles. However, ifgetMemoryUsagethrows, the error isn't caught here.const loadMemory = useCallback((opt: string) => async () => { - const memoryMap = await getMemoryUsage(opt, toast, setIndicator); - const memoryValue = memoryMap.get("total_graph_sz_mb") || '<1'; - - return `${memoryValue} MB`; + try { + const memoryMap = await getMemoryUsage(opt, toast, setIndicator); + const memoryValue = memoryMap.get("total_graph_sz_mb") || '<1'; + return `${memoryValue} MB`; + } catch { + return "N/A"; + } }, [toast, setIndicator])app/api/auth/tokens/route.ts (3)
108-108: Unsafe non-null assertion.Using
fetchResult.tokens!.lengthassumes tokens is always defined when there's no error. Use nullish coalescing for safety.- count: fetchResult.tokens!.length, + count: fetchResult.tokens?.length ?? 0,
216-218: Type-unsafe password access.Casting to
anyto accesspasswordbypasses TypeScript's type safety. The user type fromgetClientshould be extended to includepasswordwhen available.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const password = (user as any).password || ''; + const password = 'password' in user ? (user as { password?: string }).password || '' : '';Alternatively, update the
AuthenticatedUserinterface to includepassword?: string.
230-249: Duplicate Cypher query construction pattern.This is nearly identical to the query in
credentials/route.ts. Consider extracting to a shared utility to reduce duplication and ensure consistent security handling.app/graph/toolbar.tsx (2)
283-294: Semantic mismatch:variant="Delete"for Info button.Using
variant="Delete"for an informational button is confusing. Consider introducing a neutral or "Info" variant, or use a different existing variant.<Button data-testid={`elementCanvasInfo${label}`} className="pointer-events-auto bg-background cursor-default border-primary" - variant="Delete" + variant="Secondary" tooltipVariant="Primary"
295-319: Same variant concern for Add Node/Edge buttons.The Add Node and Add Edge buttons also use
variant="Delete", which is semantically incorrect for creation actions.<Button data-testid={`elementCanvasAddNode${label}`} className="pointer-events-auto bg-background border-green-900" - variant="Delete" + variant="Secondary"app/api/auth/[...nextauth]/options.ts (1)
274-301: Dynamic import adds latency; consider top-level import.The
getPasswordFromTokenDBis dynamically imported on each connection recreation. Since this is a standard code path, a top-level import would be cleaner and slightly more performant.import { isTokenActive } from "../tokenUtils"; +import { getPasswordFromTokenDB } from "../tokenUtils";Then remove the dynamic import:
- const { getPasswordFromTokenDB } = await import('../tokenUtils'); - const password = await getPasswordFromTokenDB(payload.jti); + const password = await getPasswordFromTokenDB(payload.jti);e2e/logic/POM/graphInfoPage.ts (2)
42-45: Avoid hardcodedwaitForTimeout.The 1000ms hardcoded timeout is a test smell. Consider waiting for a specific condition (e.g., element state change) rather than an arbitrary delay, which can lead to flaky tests.
async isGraphInfoPanelContainerVisible(): Promise<boolean> { - await this.page.waitForTimeout(1000); return waitForElementToBeVisible(this.graphInfoNodesCount); }
123-133: Consider addingisGraphInfoEdgeButtonNotVisiblefor symmetry.You have
isGraphInfoNodeButtonNotVisiblebut no equivalent for edges. If edge visibility assertions are needed in tests, adding a symmetric method would maintain API consistency.async isGraphInfoEdgeButtonVisible(relationship: string): Promise<boolean> { return waitForElementToBeVisible(this.graphInfoEdgeButton(relationship)); } + + async isGraphInfoEdgeButtonNotVisible(relationship: string): Promise<boolean> { + return waitForElementToNotBeVisible(this.graphInfoEdgeButton(relationship)); + } }app/api/auth/tokenUtils.ts (2)
50-51: Extract duplicatedescapeStringhelper.The
escapeStringfunction is defined twice (lines 51 and 115). Extract it as a module-level utility to avoid duplication.+// Helper to escape strings for Cypher (FalkorDB doesn't support parameterized queries for all cases) +const escapeString = (str: string) => str.replace(/'/g, "''"); + // Helper function to check if a token is active using FalkorDB PAT instance export async function isTokenActive( token: string ): Promise<boolean> { try { ... - // Helper to escape strings for Cypher (FalkorDB doesn't support parameterized queries) - const escapeString = (str: string) => str.replace(/'/g, "''"); // Check if token exists in FalkorDB and is activeAlso applies to: 114-115
126-128: Avoid logging sensitive identifiers in error messages.The error messages include
tokenIdwhich could expose sensitive information in logs. Consider using a generic message or truncating/hashing the identifier for debugging purposes.if (!result || !result.data || result.data.length === 0) { - throw new Error(`Token not found or inactive: ${tokenId}`); + throw new Error("Token not found or inactive"); } ... console.error("Error fetching password from Token DB:", error); - throw new Error(`Failed to retrieve password for token: ${tokenId}`); + throw new Error("Failed to retrieve password for token"); }Also applies to: 144-145
e2e/logic/POM/settingsTokensPage.ts (1)
82-84: Reduce reliance on hardcoded timeouts.The
waitForhelper wrapspage.waitForTimeout, which is a test anti-pattern. While some delays may be unavoidable, consider using Playwright's built-in auto-waiting or explicit conditions likewaitForSelector,waitForResponse, orexpect.pollfor more reliable tests.app/graph/GraphView.tsx (1)
78-82: Complex condition for Metadata tab enablement.The Metadata tab condition is quite verbose. Consider extracting to a named variable for clarity.
const isTabEnabled = useCallback((tab: Tab) => { if (tab === "Table") return graph.Data.length !== 0 - if (tab === "Metadata") return historyQuery.currentQuery && historyQuery.currentQuery.metadata.length > 0 && graph.Metadata.length > 0 && historyQuery.currentQuery.explain.length > 0 + if (tab === "Metadata") { + const { currentQuery } = historyQuery + return currentQuery && currentQuery.metadata.length > 0 && graph.Metadata.length > 0 && currentQuery.explain.length > 0 + } return true }, [graph, historyQuery.currentQuery])e2e/logic/api/apiCalls.ts (1)
512-529: Generates new token for each listTokensAsUser call.This method creates a new token via
generateTokenAsUserfor every list operation. For e2e tests this is acceptable, but be aware each call creates a new PAT that should ideally be cleaned up.Consider adding a comment documenting this behavior:
async listTokensAsUser(username: string, password: string): Promise<any> { try { - // First get a session/token by logging in + // Note: Creates a new PAT for each call - acceptable for e2e tests + // The created token should be cleaned up after test completion const loginResponse = await this.generateTokenAsUser(username, password);app/graph/page.tsx (1)
241-244: Avoid mutating state array directly
selectedElements.push(selectedElement)mutates the state array directly. Consider creating a local copy to avoid potential React state management issues.const handleDeleteElement = useCallback(async () => { - if (selectedElements.length === 0 && selectedElement) { - selectedElements.push(selectedElement) - } + const elementsToDelete = selectedElements.length === 0 && selectedElement + ? [selectedElement] + : selectedElements; - await Promise.all(selectedElements.map(async (element) => { + await Promise.all(elementsToDelete.map(async (element) => {Also update subsequent references from
selectedElementstoelementsToDeletewithin this callback.
…e functionality for refactoring and debugging purposes.
|
@Anchel123 Too much unwanted changes, only require to disable the tab, and skip the tests. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.github/workflows/playwright.yml (1)
212-290: Remove the commented-out job instead of leaving it commented; this also resolves the Gitleaks alert.Commenting out an entire job leaves dead code in the repository and triggers security tooling (Gitleaks flagged the
ENCRYPTION_KEYon line 270). While the learnings confirm the key is test-only, having it visible in source — even commented — is a code smell.Two better options:
Option 1 (preferred if this is permanent): Delete the entire job and commit history. If token tests need to be re-added later, restore them from git history with proper context.
Option 2 (if temporary): Use GitHub Actions' conditional workflow syntax to disable the job without leaving commented code:
run-token-tests: name: Run token tests if: false # Disabled until token testing is re-enabled needs: setup-dependencies # ... rest of job configuration ...This approach keeps the configuration clear, allows easy re-enabling, and doesn't confuse static analysis tools.
Which approach aligns with your intent—permanent removal or temporary disabling pending re-enablement?
e2e/logic/api/responses/tokenResponse.ts (1)
1-19: Avoid duplicating token shape across interfaces
TokenDetailsResponseduplicates the exact shape ofToken. To keep things in sync as the API evolves, consider aliasing instead of repeating fields.-export interface TokenDetailsResponse { - token_id: string; - name: string; - createdAt: string; - expiresAt: string | null; - lastUsed: string | null; -} +export type TokenDetailsResponse = Token;e2e/logic/api/apiCalls.ts (4)
434-444: Tighten typing forlistTokensJSON parsing (optional)The
Promise<ListTokensResponse>return type is good. To avoid untypedjson()results flowing through, you could explicitly assert the type at the call site.async listTokens(): Promise<ListTokensResponse> { try { const headers = await getAdminToken(); const result = await getRequest(`${urls.api.tokenUrl}tokens`, headers); - return await result.json(); + return (await result.json()) as ListTokensResponse; } catch (error) { throw new Error( `Failed to list tokens. \n Error: ${(error as Error).message}` ); } }
458-468: AligngetTokenDetailswith its response type (optional)Same idea here: you already declare
Promise<TokenDetailsResponse>, so explicitly asserting the parsed JSON helps keep call sites strongly typed.async getTokenDetails(tokenId: string): Promise<TokenDetailsResponse> { try { const headers = await getAdminToken(); const result = await getRequest(`${urls.api.tokenUrl}tokens/${tokenId}`, headers); - return await result.json(); + return (await result.json()) as TokenDetailsResponse; } catch (error) { throw new Error( `Failed to get token details. \n Error: ${(error as Error).message}` ); } }
519-541: Ensure temporary user tokens are cleaned up even when listing failsRight now the cleanup (
revokeToken(token_id)) runs only if the list request andjson()both succeed. If either fails, the temporary token created bygenerateTokenAsUsercan be left behind, which is undesirable even in tests.You can make cleanup best-effort by tracking the temporary token id and revoking it in a
finallyblock.async listTokensAsUser(username: string, password: string): Promise<any> { - try { - // First get a session/token by logging in - const loginResponse = await this.generateTokenAsUser(username, password); - const { token, token_id } = loginResponse; - - // Use that token to list tokens - const result = await getRequest( - `${urls.api.tokenUrl}tokens`, - { Authorization: `Bearer ${token}` } - ); - const response = await result.json(); - - // Clean up the temporary token - await this.revokeToken(token_id); - - return response; - } catch (error) { + let temporaryTokenId: string | undefined; + try { + // First get a session/token by logging in + const loginResponse = await this.generateTokenAsUser(username, password); + const { token, token_id } = loginResponse; + temporaryTokenId = token_id; + + // Use that token to list tokens + const result = await getRequest( + `${urls.api.tokenUrl}tokens`, + { Authorization: `Bearer ${token}` } + ); + return await result.json(); + } catch (error) { throw new Error( `Failed to list tokens as user ${username}. \n Error: ${(error as Error).message}` ); + } finally { + if (temporaryTokenId) { + try { + await this.revokeToken(temporaryTokenId); + } catch { + // best-effort cleanup; ignore failures + } + } } }Also, given prior feedback on this PR (“only require to disable the tab, and skip the tests”), consider whether you still need this multi-user helper now that token-based tests are disabled, or if it can be removed to keep the diff smaller.
543-569: Also clean up temporary tokens inrevokeTokenAsUseron failure
revokeTokenAsUserhas the same pattern: if the DELETE orjson()call fails, the temporary token returned fromgenerateTokenAsUseris never revoked.A similar
try/finallystructure keeps test-generated tokens from lingering.async revokeTokenAsUser( username: string, password: string, tokenId: string ): Promise<any> { - try { - // First get a session/token by logging in - const loginResponse = await this.generateTokenAsUser(username, password); - const { token, token_id } = loginResponse; - - // Use that token to revoke via DELETE - const result = await deleteRequest( - `${urls.api.tokenUrl}tokens/${tokenId}`, - { Authorization: `Bearer ${token}` } - ); - const response = await result.json(); - - // Clean up the temporary token - await this.revokeToken(token_id); - - return response; - } catch (error) { + let temporaryTokenId: string | undefined; + try { + // First get a session/token by logging in + const loginResponse = await this.generateTokenAsUser(username, password); + const { token, token_id } = loginResponse; + temporaryTokenId = token_id; + + // Use that token to revoke via DELETE + const result = await deleteRequest( + `${urls.api.tokenUrl}tokens/${tokenId}`, + { Authorization: `Bearer ${token}` } + ); + return await result.json(); + } catch (error) { throw new Error( `Failed to revoke token as user ${username}. \n Error: ${(error as Error).message}` ); + } finally { + if (temporaryTokenId) { + try { + await this.revokeToken(temporaryTokenId); + } catch { + // best-effort cleanup; ignore failures + } + } } }Same as above, if token-based UI/tests are now disabled for this release, you may not need to keep this helper at all and could drop it to reduce PR scope.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/playwright.yml(1 hunks)e2e/logic/api/apiCalls.ts(6 hunks)e2e/logic/api/responses/tokenResponse.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
e2e/logic/api/responses/tokenResponse.tse2e/logic/api/apiCalls.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/logic/api/responses/tokenResponse.tse2e/logic/api/apiCalls.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
e2e/logic/api/responses/tokenResponse.tse2e/logic/api/apiCalls.ts
🧠 Learnings (1)
📚 Learning: 2025-11-13T09:41:15.717Z
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: .github/workflows/playwright.yml:264-264
Timestamp: 2025-11-13T09:41:15.717Z
Learning: The hardcoded ENCRYPTION_KEY value "a3f5d8c2b7e4f1a0c9d6e8b2f5a7c3d1e9f2a4b8c6d3e7f1a5b9c2d8e4f0a6b3" in .github/workflows/playwright.yml at the run-token-tests job is a test-only encryption key used for CI token tests and is not a production secret.
Applied to files:
.github/workflows/playwright.yml
🧬 Code graph analysis (1)
e2e/logic/api/apiCalls.ts (1)
e2e/logic/api/responses/tokenResponse.ts (2)
ListTokensResponse(9-11)TokenDetailsResponse(13-19)
🪛 Gitleaks (8.29.1)
.github/workflows/playwright.yml
[high] 270-270: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup and cache dependencies
🔇 Additional comments (2)
e2e/logic/api/apiCalls.ts (2)
33-33: Import of token response types looks correctThe new
tokenResponseimport is consistent with the existing response imports and keeps token-related typings centralized.
470-481: UsingPromise.allSettledfor bulk revoke is appropriateSwitching
revokeAllUserTokenstoPromise.allSettledis a good fit: you attempt to revoke all tokens in parallel and then fail only if any revocation is rejected, with a clear aggregated error message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
e2e/logic/api/responses/settingsResponses.ts (1)
1-7: Settings response typings are consistent with usageThe interfaces are straightforward and match how
modifySettingsRoleandgetSettingsRoleValueconsume them. If these are used beyond tests later, consider more descriptive property naming than a genericconfigfield for long-term clarity.e2e/logic/api/responses/userResponses.ts (1)
1-15: User response types look good; consider reusing common shapesThe user-related response interfaces are clear and type-safe. Since
CreateUsersResponseandDeleteUsersResponseshare the same{ message: string }structure, you could optionally introduce a sharedMessageResponseto DRY things up if you add more similar endpoints later.e2e/logic/api/responses/schemaResponses.ts (1)
1-14: Schema response interfaces are coherent; align removeSchema return typeThe schema response types are coherent and match their likely payloads. In
ApiCalls.removeSchemayou currently usePromise<RemoveGraphResponse>even thoughRemoveSchemaResponseexists with the same shape; switching that method toPromise<RemoveSchemaResponse>would avoid semantic confusion and better reflect intent.e2e/logic/api/responses/graphResponses.ts (1)
1-54: Graph response types are reasonable; tighten naming and any-usage if possibleThe graph response interfaces look structurally sound for the e2e layer. Two optional cleanups to consider:
- Rename
DuplicateGraphresponse→DuplicateGraphResponseto match the other interface names and typical PascalCase conventions.- Instead of a file-level
@typescript-eslint/no-explicit-anydisable, you could scope it toRunQueryResponse.dataor replaceanywith a more precise generic/union once the query result shape is better understood.Neither is blocking, but they’ll improve consistency and lint hygiene.
e2e/logic/api/apiCalls.ts (4)
444-454: Strongly typing token list/detail responses is a good step; consider finishing the setUpdating
listTokensandgetTokenDetailsto returnListTokensResponse/TokenDetailsResponseremoves a lot ofanyfrom the token surface and will make tests safer to refactor. For consistency, you may also want to eventually introduce concrete response types forgenerateTokenandrevokeTokenso all token helpers have well-defined shapes.Also applies to: 468-478
482-490: Parallel token revocation is fine; error detail could be richerUsing a parallel “all-settled” style revocation ensures you attempt to revoke every token even if some calls fail, which is appropriate for cleanup. If you find these errors hard to debug in CI, you could enhance the thrown message to include which token IDs failed by zipping
tokenswith the settled results.
533-545: User token helpers now clean up temporary login tokens; add minimal guardsThe new pattern of:
- generating a short-lived token as the user,
- using it to list or revoke tokens,
- then revoking the temporary login token via its
token_id,is a solid way to avoid leaking credentials during e2e runs. To make failures easier to diagnose if the API shape ever changes, you might add a quick runtime guard that
tokenandtoken_idare present before using them; otherwise you end up issuingDELETErequests with an undefined ID and get a less clear failure.Also, given the scope of this PR around token handling, a concise title like:
test: refactor e2e token helpers and consolidate API response typeswould capture the main impact.
Also applies to: 561-573
592-601: Align removeSchema return type with schema-specific interface
removeSchemacurrently returnsPromise<RemoveGraphResponse>, even thoughRemoveSchemaResponseexists and has the same{ message: string }payload. Switching toRemoveSchemaResponsehere would better match the schema-focused behavior and the newschemaResponses.tsinterfaces, and avoids coupling schema operations to graph response types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
e2e/logic/api/apiCalls.ts(6 hunks)e2e/logic/api/responses/LoginResponse.ts(0 hunks)e2e/logic/api/responses/addGraphResponse.ts(0 hunks)e2e/logic/api/responses/addSchemaResponse.ts(0 hunks)e2e/logic/api/responses/authResponses.ts(1 hunks)e2e/logic/api/responses/changeGraphNameResponse.ts(0 hunks)e2e/logic/api/responses/createUsersResponse.ts(0 hunks)e2e/logic/api/responses/deleteUsersResponse.ts(0 hunks)e2e/logic/api/responses/duplicateGraph.ts(0 hunks)e2e/logic/api/responses/getGraphsResponse.ts(0 hunks)e2e/logic/api/responses/getSchemaResponse.ts(0 hunks)e2e/logic/api/responses/getSettingsRoleValue.ts(0 hunks)e2e/logic/api/responses/getUsersResponse.ts(0 hunks)e2e/logic/api/responses/graphAttributeResponse.ts(0 hunks)e2e/logic/api/responses/graphCountResponse.ts(0 hunks)e2e/logic/api/responses/graphNodeResponse.ts(0 hunks)e2e/logic/api/responses/graphResponses.ts(1 hunks)e2e/logic/api/responses/logoutResponse.ts(0 hunks)e2e/logic/api/responses/modifySettingsRoleResponse.ts(0 hunks)e2e/logic/api/responses/removeGraphResponse.ts(0 hunks)e2e/logic/api/responses/removeSchemaResponse.ts(0 hunks)e2e/logic/api/responses/runQueryResponse.ts(0 hunks)e2e/logic/api/responses/schemaResponses.ts(1 hunks)e2e/logic/api/responses/settingsResponses.ts(1 hunks)e2e/logic/api/responses/userResponses.ts(1 hunks)
💤 Files with no reviewable changes (19)
- e2e/logic/api/responses/addSchemaResponse.ts
- e2e/logic/api/responses/graphNodeResponse.ts
- e2e/logic/api/responses/removeSchemaResponse.ts
- e2e/logic/api/responses/createUsersResponse.ts
- e2e/logic/api/responses/removeGraphResponse.ts
- e2e/logic/api/responses/getUsersResponse.ts
- e2e/logic/api/responses/graphCountResponse.ts
- e2e/logic/api/responses/getSettingsRoleValue.ts
- e2e/logic/api/responses/duplicateGraph.ts
- e2e/logic/api/responses/getGraphsResponse.ts
- e2e/logic/api/responses/changeGraphNameResponse.ts
- e2e/logic/api/responses/getSchemaResponse.ts
- e2e/logic/api/responses/runQueryResponse.ts
- e2e/logic/api/responses/logoutResponse.ts
- e2e/logic/api/responses/LoginResponse.ts
- e2e/logic/api/responses/graphAttributeResponse.ts
- e2e/logic/api/responses/deleteUsersResponse.ts
- e2e/logic/api/responses/modifySettingsRoleResponse.ts
- e2e/logic/api/responses/addGraphResponse.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
e2e/logic/api/responses/authResponses.tse2e/logic/api/responses/schemaResponses.tse2e/logic/api/responses/settingsResponses.tse2e/logic/api/responses/userResponses.tse2e/logic/api/responses/graphResponses.tse2e/logic/api/apiCalls.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/logic/api/responses/authResponses.tse2e/logic/api/responses/schemaResponses.tse2e/logic/api/responses/settingsResponses.tse2e/logic/api/responses/userResponses.tse2e/logic/api/responses/graphResponses.tse2e/logic/api/apiCalls.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
e2e/logic/api/responses/authResponses.tse2e/logic/api/responses/schemaResponses.tse2e/logic/api/responses/settingsResponses.tse2e/logic/api/responses/userResponses.tse2e/logic/api/responses/graphResponses.tse2e/logic/api/apiCalls.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup and cache dependencies
🔇 Additional comments (2)
e2e/logic/api/responses/authResponses.ts (1)
1-13: Auth response typings are straightforward and consistentThe auth-related interfaces are minimal and consistent. They should integrate cleanly with the
loginandlogouthelpers inapiCalls.tsas long as the backend preserves this shape.e2e/logic/api/apiCalls.ts (1)
15-43: Grouped response-type imports improve clarityCentralizing the various response interfaces (
graphResponses,userResponses,schemaResponses,settingsResponses,authResponses,tokenResponse) keeps this helper focused and self-documenting. This makes the return types of each method much clearer when scanning the class.
Summary by CodeRabbit
New Features
Improvements
Tests / Chores
✏️ Tip: You can customize this high-level summary in your review settings.