Staging #1230
Staging #1230
Conversation
- Updated the `fetchInfo` function to accept a `name` parameter for more flexible API calls. - Introduced a new `getMemoryUsage` function to fetch and process memory usage data for the specified graph. - Modified the `GraphInfo` class to include memory usage as part of its state management. - Updated the `runQuery` function to incorporate memory usage when creating graph information. - Adjusted the `GraphInfoPanel` component to display labels, relationships, and property keys directly from the context.
- Bumped FalkorDB version in package.json and package-lock.json to 6.3.1. - Refactored memory usage handling in the GraphInfo class to support nested memory values. - Introduced new API routes for counting nodes and edges in the graph. - Updated the GraphInfoPanel to display memory usage alongside nodes and edges count. - Enhanced TableComponent to support lazy loading of cell values for improved performance.
- Added `Tab` import to the `providers.tsx` file for enhanced functionality. - Adjusted z-index in the `DropdownMenuContent` component to improve layering and visibility of dropdown menus.
- Included the `password` field in the user object within the authentication options to enhance user data handling.
- Added "+graph.memory" permission to both "Read-Write" and "Read-Only" roles to improve memory management capabilities.
- Improved the structure of user role permissions by defining a separate READ_ONLY_ROLE array for better maintainability. - Updated the ROLE map to include additional permissions for "Read-Write" and "Read-Only" roles, enhancing user access control.
- Added a semicolon to the `loadCell` function type definition for consistency. - Updated the API endpoint in the `getMemoryUsage` function to use `prepareArg(name)` for improved argument handling.
- Moved the calculation of `query` and `url` to a different position in `ProvidersWithSession` for better readability. - Updated the default refresh interval from 10 to 30 in `ProvidersWithSession`. - Increased the maximum allowed value for the refresh interval from 60 to 600 in `BrowserSettings`.
- Introduced a new `GraphInfoPage` class for managing interactions with the graph info panel in end-to-end tests. - Added methods to retrieve node and edge counts, property keys count, and handle button interactions in the graph info panel. - Enhanced the `EditorComponent` to include a `data-value` attribute for the editor container and added a clear editor button. - Updated the `GraphPage` class to include a method for clicking the clear editor input. - Created comprehensive tests for the graph info panel, validating its functionality and real-time updates after graph modifications.
- Renamed the `clearEditor` locator in `GraphPage` for clarity. - Uncommented and organized test cases in `graphInfo.spec.ts` to improve readability and maintainability. - Added functionality to clear the editor input in the graph info panel tests to ensure accurate query validation. - Enhanced tests to validate real-time updates in the graph info panel after node and edge deletions.
- Redirected output of the `npm start` command to `nextjs.log` for better debugging during Playwright tests. - This change applies to multiple test shards and configurations to ensure consistent logging across all test runs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
app/components/TableComponent.tsx (2)
152-168: Allow retry after load failuresWhen
loadFnrejects, the code removes the spinner but leaves the cell key insideloadAttemptedRef.current(line 518 adds it, but catch never removes it). Subsequent renders treat the cell as "already attempted", so it will never retry even if the backend recovers or the user revisits the row.Apply this diff to enable retries after failures:
}).catch((error) => { // Only log error if not aborted if (!abortController.signal.aborted) { // eslint-disable-next-line no-console console.error(`Failed to load cell ${cellKey}:`, error); } // Remove from loading set on error setLoadingCells(prev => { const newSet = new Set(prev); newSet.delete(cellKey); return newSet; }); + + // Allow retry by removing from load attempted set + loadAttemptedRef.current.delete(cellKey); // Clean up abort controller abortControllersRef.current.delete(cellKey); });
517-520: Fix: avoid state updates during render
handleLoadLazyCellis invoked at line 519 while rendering each cell (inside therow.cells.mapat line 509), but it immediately callssetLoadingCellsat line 112. React forbidssetStateduring render; the first lazy cell encountered will throw "Cannot update a component while rendering" and the table will crash.Defer the load to an effect: during render, collect pending cells into a ref, then trigger
handleLoadLazyCellfromuseEffect:+ const pendingLoadsRef = useRef<Array<{rowName: string, cellIndex: number, loadFn: () => Promise<any>}>>([]); + + useEffect(() => { + if (pendingLoadsRef.current.length > 0) { + const loads = [...pendingLoadsRef.current]; + pendingLoadsRef.current = []; + loads.forEach(({rowName, cellIndex, loadFn}) => { + handleLoadLazyCell(rowName, cellIndex, loadFn); + }); + } + }, [visibleRows, handleLoadLazyCell]);Then in the render path (around line 517-520), replace the direct call:
- if (isLazyCell && !cell.value && !loadingCells.has(cellKey) && !loadAttemptedRef.current.has(cellKey)) { - loadAttemptedRef.current.add(cellKey); - handleLoadLazyCell(row.name, j, cell.loadCell); - } + if (isLazyCell && !cell.value && !loadingCells.has(cellKey) && !loadAttemptedRef.current.has(cellKey)) { + loadAttemptedRef.current.add(cellKey); + pendingLoadsRef.current.push({rowName: row.name, cellIndex: j, loadFn: cell.loadCell}); + }app/api/graph/model.ts (1)
220-232: Unresolved:GraphInfo.empty()throws whenmemoryUsageis undefined.Line 229 calls
new Map(memoryUsage)butmemoryUsageis optional. Whenundefined,new Map(undefined)throwsTypeError: undefined is not iterable. This was flagged in a previous review and appears unaddressed.app/components/Header.tsx (3)
115-122: Unresolved: Remove commented-out SCHEMAS code.This was flagged in a previous review. Commented-out code creates technical debt. Either delete it or use a feature flag.
144-146: Unresolved: Multi-line template literal preserves whitespace in tooltip.This was flagged in a previous review. The title will display with unintended indentation. Use a single-line string.
148-158: Unresolved: Chat opens without settings validation whennavigateToSettingsis false.This was flagged in a previous review. When
navigateToSettingsis false butmodelorsecretKeyis missing, the chat panel opens without validation.app/components/ForceGraph.tsx (1)
341-347: Unresolved: Schema route missing GET handler for node neighbors.The URL changed from a fixed path to
/api/${type}/${graph.Id}/${node.id}. Fortype="schema", the backend route atapp/api/schema/[schema]/[element]/_route.tsonly has POST and DELETE handlers, not GET. This will cause runtime failures.
🧹 Nitpick comments (5)
app/components/TableComponent.tsx (1)
528-528: Consider aligning editable check with cellKey approachLine 528 still checks
row.cells[0]?.value === editable, but elsewhere (lines 582, 677)editableis compared tocellKey. This inconsistency is a leftover from the index-based approach. Since this only affects padding in loading cells, the impact is minimal, but for consistency you might remove or update this condition.Apply this diff if you want to remove the outdated check:
<TableCell className={cn( j + 1 !== row.cells.length && "border-r", - row.cells[0]?.value === editable && (cell.type !== "readonly" && cell.type !== "object") && "p-2", cell.type === "object" && "p-1", "border-border" )}lib/utils.ts (1)
316-347: Reduce redundant key lookups ingetNodeDisplayText.The key lookup logic is duplicated in both the
findcallback and the subsequent block. Extract the key resolution into a reusable helper or cache the result.export const getNodeDisplayText = (node: Node, displayTextPriority: TextPriority[]) => { const { data: nodeData } = node; - const displayText = displayTextPriority.find(({ name, ignore }) => { - const key = ignore + const resolveKey = (name: string, ignore: boolean) => + ignore ? Object.keys(nodeData).find( (k) => k.toLowerCase() === name.toLowerCase() ) : name; + const displayText = displayTextPriority.find(({ name, ignore }) => { + const key = resolveKey(name, ignore); return ( key && nodeData[key] && typeof nodeData[key] === "string" && nodeData[key].trim().length > 0 ); }); if (displayText) { - const key = displayText.ignore - ? Object.keys(nodeData).find( - (k) => k.toLowerCase() === displayText.name.toLowerCase() - ) - : displayText.name; - + const key = resolveKey(displayText.name, displayText.ignore); if (key) { return String(nodeData[key]); } } return String(node.id); }app/components/ForceGraph.tsx (1)
615-622: Early return guard is redundant.The guard at lines 615-622 checks the same variables that were just assigned in lines 600-603 within the
ifblock. This guard will only be reached if theif (relationship)block was skipped, meaningtextWidthetc. were just set. Consider restructuring for clarity.- if ( - textWidth === undefined || - textHeight === undefined || - textAscent === undefined || - textDescent === undefined - ) { - return - } + // At this point, all text metrics are guaranteed to be definedapp/graph/toolbar.tsx (1)
283-294: Info button usescursor-defaultbut has no interactive purpose.The Info button displays a tooltip via the
titleattribute but setscursor-default, suggesting it's non-interactive. Consider using a<span>or purely informational element instead of a<Button>if no click action is intended. Alternatively, if keeping it as a button for consistency, ensure keyboard accessibility is maintained.Also, the multi-line
titleattribute may not render well across browsers. Consider using a properTooltipcomponent for better control over formatting.app/graph/Selector.tsx (1)
144-166: IncludesetHistoryQueryonly if it changes; consider whether it should be in the dependency array.The dependency chain identified is accurate: when
historyQuerychanges,afterSearchCallbackis recreated, triggeringresetHistoryFiltersto be recreated, which then fires theuseEffectat lines 192-194. However,setHistoryQueryis a stable state setter function (type:Dispatch<SetStateAction<HistoryQuery>>) that never changes across renders, so its inclusion in theafterSearchCallbackdependency array at line 142 is unnecessary. While the code follows exhaustive-deps rules, consider whether this is intentional or if it could be safely removed or handled differently. The real performance concern depends on how frequentlyhistoryQueryis updated by the parent component inapp/providers.tsx(line 49). If updates are unavoidable, the current memoization pattern is already appropriate and further optimization may not be needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/api/graph/model.ts(10 hunks)app/components/ForceGraph.tsx(9 hunks)app/components/Header.tsx(8 hunks)app/components/TableComponent.tsx(15 hunks)app/components/ui/Button.tsx(3 hunks)app/graph/DeleteElement.tsx(3 hunks)app/graph/GraphView.tsx(8 hunks)app/graph/Selector.tsx(12 hunks)app/graph/toolbar.tsx(5 hunks)lib/utils.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/graph/DeleteElement.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/components/Header.tsxapp/components/ui/Button.tsxapp/components/TableComponent.tsxapp/components/ForceGraph.tsxapp/graph/toolbar.tsxapp/graph/GraphView.tsxapp/graph/Selector.tsx
**/*.ts
⚙️ CodeRabbit configuration file
Review TypeScript code for type safety, proper error handling, and Next.js best practices.
Files:
lib/utils.tsapp/api/graph/model.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/graph/model.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
🧬 Code graph analysis (6)
app/components/Header.tsx (1)
app/components/provider.ts (1)
BrowserSettingsContext(155-214)
app/components/TableComponent.tsx (1)
lib/utils.ts (2)
Row(85-89)cn(91-93)
app/components/ForceGraph.tsx (2)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)lib/utils.ts (2)
getNodeDisplayText(316-347)securedFetch(140-162)
app/graph/toolbar.tsx (2)
lib/utils.ts (1)
cn(91-93)app/components/ui/Button.tsx (1)
Props(11-21)
lib/utils.ts (1)
app/api/graph/model.ts (3)
MemoryValue(117-117)Node(55-66)Link(68-84)
app/graph/Selector.tsx (1)
app/graph/toolbar.tsx (1)
Toolbar(32-335)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: Setup and cache dependencies
- GitHub Check: Setup and cache dependencies
- GitHub Check: build
🔇 Additional comments (53)
app/components/ui/Button.tsx (3)
20-20: LGTM: Clean tooltip positioning prop.The optional
tooltipSideprop provides standard tooltip placement control without breaking existing usage.
55-55: LGTM: Proper prop destructuring.The
tooltipSideprop is correctly integrated into the component signature.
82-82: LGTM: Tooltip positioning and formatting enhancements.The
sideprop enables flexible tooltip placement, andwhitespace-pre-lineallows multi-line tooltip content. The whitespace class applies to all Button tooltips globally (not just Delete variants), enabling consistent newline handling across the UI. Verification confirms no existing Button tooltips contain newline characters, so this change is backwards-compatible and safe.lib/utils.ts (7)
10-12: LGTM on imports and constant.The new
MemoryValueimport andMEMORY_USAGE_VERSION_THRESHOLDconstant are appropriately added.
25-25: LGTM on Panel type extension.Adding
"add"to the Panel union type cleanly supports the new add-element functionality.
58-62: LGTM on LazyCell and Cell union extension.The
LazyCelltype with optionalvalueand asyncloadCellis well-structured for deferred loading.Also applies to: 77-78
85-89: LGTM on Row type extension.Adding the
namefield toRowis straightforward.
106-111: LGTM on indicator updates in SSE and fetch helpers.Setting the indicator to
"online"on successful results improves UI feedback consistency.Also applies to: 140-162
256-275: LGTM ongetMemoryUsageimplementation.The function properly handles errors by returning an empty Map when
result.okis false.
402-406: Type guard useslabelsproperty which exists on both Node and Link types.The
isTwoNodesguard checks for!!e.labelsto identify nodes. However, per theLinktype inapp/api/graph/model.ts, links don't have alabelsproperty, so this works correctly. Consider adding a comment for clarity.app/api/graph/model.ts (7)
8-8: LGTM on Value type simplification.Removing recursive
Value[]simplifies the type while maintaining needed flexibility.
117-118: LGTM on MemoryValue type.Recursive type definition using
Map<string, MemoryValue>correctly models nested memory data.
148-150: LGTM on Relationship extension.Adding
textAscentandtextDescentfor text metrics caching is appropriate.
166-167: LGTM on GraphInfo memoryUsage integration.The new
memoryUsagefield, constructor parameter, getter, and clone logic are correctly implemented.Also applies to: 176-177, 182-183, 202-204, 210-217
234-248: LGTM onGraphInfo.create()with required memoryUsage.The factory properly passes
memoryUsagethrough toempty().
464-495: LGTM oncalculateLinkCurveimplementation.The method correctly computes curvature based on parallel links and self-loops with appropriate spacing.
745-747: LGTM on refactored curve assignment.Centralizing curvature calculation via
calculateLinkCurveimproves maintainability.app/components/Header.tsx (5)
24-30: LGTM on prop rename and signature update.Renaming
displayChattonavigateToSettingsin Props and updating the function signature is consistent.Also applies to: 40-40
44-44: LGTM on context destructuring.Extracting
displayChatfromchatSettingsalongsidemodelandsecretKeyis correct.
56-56: LGTM on showCreate logic.The role check is appropriate for controlling create button visibility.
84-84: LGTM on gap reduction.Reducing
gap-6togap-4is a minor layout adjustment.Also applies to: 164-164
180-180: LGTM on dropdown padding reduction.Reducing
p-4top-2is a minor styling adjustment.app/components/ForceGraph.tsx (11)
7-9: LGTM on imports update.Adding
useCallback,useMemo, andgetNodeDisplayTextsupports the new degree-aware and display text logic.
104-114: LGTM on force configuration constants.Well-documented constants for link distance, strength, collision, and degree thresholds improve readability and tunability.
116-123: LGTM ongetEndpointIdhelper.Robust handling of Node objects, numbers, and string representations with proper NaN checking.
161-179: LGTM onnodeDegreeMapcomputation.The useMemo correctly computes node degrees from links, handling various endpoint representations.
258-298: LGTM on degree-aware link force.The distance and strength functions properly scale based on node degrees with appropriate thresholds and decay formulas.
301-305: LGTM on degree-aware collision force.Using
Math.sqrt(degree)for radius scaling provides reasonable growth for high-degree nodes.
318-321: LGTM on charge force tuning and dependency update.Capping
distanceMaxand includingnodeDegreeMapin dependencies ensures proper reactivity.Also applies to: 326-326
339-339: LGTM onhandleGetNodeDisplayTextcallback.Memoizing with
displayTextPrioritydependency is correct.
393-395: LGTM on consistent use ofhandleGetNodeDisplayText.Node display text is consistently derived via the memoized callback throughout click handling and rendering.
Also applies to: 447-447, 494-494
574-612: LGTM on text metrics caching with ascent/descent.Extending the cache to include
textAscentandtextDescentreduces repeated measurements.
631-638: LGTM on background dimension calculation.Using 0.7 multiplier for background width/height provides appropriate padding.
app/graph/GraphView.tsx (9)
6-6: LGTM on ScrollText import.Adding
ScrollTexticon for Metadata tab is appropriate.
37-41: LGTM on new add-element props.The
setIsAddNode,setIsAddEdge,isAddNode, andisAddEdgeprops are properly typed and destructured.Also applies to: 60-64
66-66: LGTM on graphName destructuring.Extracting
graphNamefromGraphContextfor passing to Toolbar.
78-82: Verify Metadata tab enablement logic.The condition
historyQuery.currentQuery && historyQuery.currentQuery.metadata.length > 0 && graph.Metadata.length > 0 && historyQuery.currentQuery.explain.length > 0is complex. Ensure this matches the intended behavior—metadata tab should only enable when all four conditions are true.
84-91: LGTM on default tab selection simplification.The logic clearly defaults to "Table" only when there are no elements but data exists.
125-126: LGTM on layout class adjustments.Minor styling tweaks for gap and flex layout.
Also applies to: 146-146, 155-155, 161-161
130-143: LGTM on Toolbar props wiring.The conditional
setIsAddEdgeensures it's only passed when exactly two nodes are selected, using theisTwoNodespattern (checking!!e.labels).
167-199: LGTM on tab button enablement and titles.Dynamic
disabledandtitleprops based onisTabEnabledprovide clear user feedback.
197-197: LGTM on Metadata icon change.
ScrollTexticon is more semantically appropriate for metadata than the previous icon.app/graph/toolbar.tsx (6)
1-5: LGTM!Imports are clean and appropriate for the new functionality.
useSessionfrom next-auth/react aligns with the session-based UI gating pattern used elsewhere in the codebase.
12-26: LGTM!Props interface is well-structured with the renamed props (
setIsAddNode,setIsAddEdge,isAddNode,isAddEdge) and the addition ofgraphName. The optionalsetIsAddEdgeprop allows conditional rendering of the Add Edge button.
295-305: Add Node button toggles state correctly.The implementation correctly toggles
isAddNodeviasetIsAddNode(!isAddNode). Thedata-testidis properly set for E2E testing.
306-319: Add Edge button conditional rendering is correct.The button only renders when
setIsAddEdgeis defined, which aligns with the prop contract. Toggle logic is consistent with Add Node.
279-281: Consider null-safety forsession?.user.role.The condition
session?.user.role !== "Read-Only"will evaluate totruewhensessionisnullorsession.useris undefined. If the intent is to hide these buttons for unauthenticated users, consider an explicit check:-graphName && session?.user.role !== "Read-Only" && +graphName && session?.user && session.user.role !== "Read-Only" &&
337-340: LGTM!Default props are correctly updated to reflect the new prop structure.
app/graph/Selector.tsx (5)
121-127: LGTM!Good use of
useMemoto avoid recomputing filters on every render. The conditional logic efficiently handles both cases: usinggraphNameswhen the list is large enough, or deriving unique names from queries otherwise.
130-141: Functional state update prevents unnecessary re-renders.Good refactor using
setHistoryQuery(prev => ...)to avoid stale state and only update whenprev.counter !== 0.
293-315:handleDeleteQuerycorrectly handles edge cases.The function properly:
- Guards against missing state
- Updates localStorage
- Calculates next counter position
- Filters the query from filtered list using timestamp
The use of
timestampas a unique identifier for filtering is appropriate.
467-470: Editor options adjusted for horizontal scrolling.Setting
horizontal: "auto"andwordWrap: "off"enables horizontal scrolling for long queries. This is a reasonable UX choice for query editors.
560-575: ConditionalsetIsAddEdgelogic is well-implemented.The condition
selectedElements.length === 2 && selectedElements.every(e => !!e.labels)ensures the Add Edge action is only available when exactly two nodes (elements with labels) are selected, which is semantically correct for creating an edge between nodes.
…versions Fix #1251 Remove memory usage for lower versions
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/providers.tsx (2)
70-70:refreshIntervaldefault mismatch:useState(10)vs localStorage fallback30.Initial state is
10(line 70) but hydration defaults to30(line 420). First-time users see10initially, then30after reload.- const [refreshInterval, setRefreshInterval] = useState(10) + const [refreshInterval, setRefreshInterval] = useState(30)Also applies to: 420-420
258-271:fetchInfochecksgraphNamebut receivesnameparam—use passed param.The function accepts
nameas a parameter but the early return at line 259 checksgraphName(component state). This could cause issues if called with a different graph name.- const fetchInfo = useCallback(async (type: string, name: string) => { - if (!graphName) return [] + const fetchInfo = useCallback(async (type: string, name: string) => { + if (!name) return [] - const result = await securedFetch(`/api/graph/${name}/info?type=${type}`, { + const result = await securedFetch(`/api/graph/${prepareArg(name)}/info?type=${type}`, {Also note:
nameshould be encoded withprepareArgfor URL safety.app/graph/page.tsx (1)
101-113:fetchInfoshould encode graph name for URL safety.Unlike the version in providers.tsx, this
fetchInfodoesn't useprepareArgto encode the graph name.- const result = await securedFetch(`/api/graph/${graphName}/info?type=${type}`, { + const result = await securedFetch(`/api/graph/${prepareArg(graphName)}/info?type=${type}`, {
♻️ Duplicate comments (4)
app/graph/selectGraph.tsx (1)
279-279: Inconsistent role check: usesessionRolefor consistency.Line 279 uses
session?.user.role !== "Read-Only"while the rest of the file usessessionRole. When session isundefined, this evaluates totrue, briefly showing admin UI.- session?.user.role !== "Read-Only" && + sessionRole && sessionRole !== "Read-Only" &&app/providers.tsx (2)
175-175: Duplicate dependency:navigateToSettingsappears twice.- }), [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 add
showMemoryUsagesince it's now part of the context value (line 124).
139-140: PlaintextsecretKeyin localStorage remains a security consideration.As noted in prior review, storing
secretKeyunencrypted in localStorage poses security risks if it's an API key or sensitive token. Consider server-side session storage or client-side encryption if this is sensitive data.Also applies to: 422-423
app/graph/page.tsx (1)
192-229: GuardgraphNamebefore API call inhandleCreateElement.As noted in prior review, if
graphNameis falsy, the API URL becomesapi/graph/undefined/-1. Add an early return.const handleCreateElement = useCallback(async (attributes: [string, Value][], label: string[]) => { + if (!graphName) { + return false + } const fakeId = "-1"
🧹 Nitpick comments (7)
app/graph/selectGraph.tsx (1)
100-106: Memory loader returns<1for undefined values—verify intent.
getMemoryUsagereturns an empty Map on failure (per lib/utils.ts). Iftotal_graph_sz_mbis missing,memoryMap.get("total_graph_sz_mb")returnsundefined, and the fallback'<1'displays. If the API call failed entirely, showing<1 MBmay mislead users. Consider returning an error indicator or empty string on failure.app/api/graph/[graph]/memory/route.ts (2)
12-27: Double authentication:getDBVersion()callsgetClient()internally.Both this handler and
getDBVersion()callgetClient(), resulting in two auth checks per request. Consider passing the already-authenticated client to avoid redundant session lookups.- const res = await getDBVersion(); - - if (!res.ok) { - return NextResponse.json( - { - message: `Failed to retrieve database version: ${await res.text()}`, - }, - { status: 400 } - ); - } - - const [name, version] = (await res.json()).result || ["", 0]; + const { client } = session; + + let name: string; + let version: number; + try { + const moduleInfo = await (await client.connection).moduleList(); + [name, version] = [moduleInfo[0][1], moduleInfo[0][3]]; + } catch (err) { + return NextResponse.json( + { message: `Failed to retrieve database version: ${(err as Error).message}` }, + { status: 400 } + ); + }
40-52: Graph name should be validated before use.The
graphparameter is used directly inclient.selectGraph(graph). While FalkorDB likely handles invalid graph names, consider validating or sanitizing the input to provide clearer error messages.app/graph/graphInfo.tsx (2)
40-50: Memory usage tooltip may showundefined.Line 46 displays
MemoryUsage.get("total_graph_sz_mb")in the tooltip without a fallback. If the value is0(falsy but defined), the tooltip would show0 MBwhich is correct, but the outer|| "<1"on line 43 would show<1. Consider using nullish coalescing for consistency.- <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> </TooltipTrigger> <TooltipContent> - {MemoryUsage.get("total_graph_sz_mb")} MB + {MemoryUsage.get("total_graph_sz_mb") ?? "<1"} MB </TooltipContent>
133-144: Consider filtering Relationships for empty names like Labels.Labels are filtered with
.filter(({ name }) => !!name)but Relationships are not. If empty relationship names are possible, this could render broken buttons.- {Array.from(Relationships.values()).map(({ name, color }) => ( + {Array.from(Relationships.values()).filter(({ name }) => !!name).map(({ name, color }) => (app/api/auth/DBVersion/route.ts (2)
1-4: Tighten the ESLint disable scope and suggest PR titleYou only need to suppress
import/prefer-default-exporthere; disabling@typescript-eslint/no-unused-varson the handler can hide real unused-parameter issues in the future.-// eslint-disable-next-line import/prefer-default-export, @typescript-eslint/no-unused-vars +// eslint-disable-next-line import/prefer-default-exportSuggested PR title (conventional commit):
feat(auth): add DB version inspection endpoint
15-31: Harden moduleList parsing and refine error/status handlingTwo things here could be more robust:
- Result indexing without shape checks
result[0][1]andresult[0][3]assumemoduleList()always returns at least one module with a fixed tuple shape. If the DB has no modules, or the response shape changes, this will throw and be treated as a generic 400.Consider validating the structure and making the intent (name/version) explicit:
- try { - const result = await (await client.connection).moduleList(); - return NextResponse.json({ result: [result[0][1], result[0][3]] }, { status: 200 }); - } catch (error) { - console.error(error); - return NextResponse.json( - { message: (error as Error).message }, - { status: (error as Error).message.includes("NOPERM") ? 200 : 400 } - ); - } + try { + const conn = await client.connection; + const result = await conn.moduleList(); + + if (!Array.isArray(result) || !Array.isArray(result[0]) || result[0].length < 4) { + console.error("Unexpected moduleList response:", result); + return NextResponse.json( + { message: "Unexpected moduleList response from FalkorDB" }, + { status: 500 } + ); + } + + const [, moduleName, , moduleVersion] = result[0]; + + return NextResponse.json({ result: [moduleName, moduleVersion] }, { status: 200 }); + } catch (error: unknown) { + console.error(error); + + const message = error instanceof Error ? error.message : String(error); + const noPerm = message.includes("NOPERM"); + + return NextResponse.json( + { + message: noPerm + ? "Missing permissions to list modules" + : "Failed to read DB version", + }, + { status: noPerm ? 200 : 500 } + ); + }
- Status codes and message exposure
- Treating all non‑
NOPERMerrors as400blurs client vs server responsibility; most failures frommoduleList()(connection issues, server errors, unexpected shapes) are better surfaced as5xx.- Returning raw
error.messageto clients (both inner and outer catch) may leak internal details; a short, generic message is usually safer while you keep the full error in logs.Please verify the exact
moduleList()response shape in your client library and adjust the guards/destructuring accordingly, and double‑check that returning200on"NOPERM"is intentional from an API‑contract perspective.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/api/auth/DBVersion/route.ts(1 hunks)app/api/graph/[graph]/memory/route.ts(1 hunks)app/components/provider.ts(2 hunks)app/graph/graphInfo.tsx(7 hunks)app/graph/page.tsx(10 hunks)app/graph/selectGraph.tsx(10 hunks)app/providers.tsx(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/provider.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:
app/api/auth/DBVersion/route.tsapp/api/graph/[graph]/memory/route.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/auth/DBVersion/route.tsapp/api/graph/[graph]/memory/route.ts
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/graph/selectGraph.tsxapp/graph/page.tsxapp/providers.tsxapp/graph/graphInfo.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
🧬 Code graph analysis (5)
app/api/auth/DBVersion/route.ts (1)
app/api/auth/[...nextauth]/options.ts (2)
session(381-399)getClient(403-480)
app/graph/selectGraph.tsx (2)
app/api/auth/[...nextauth]/options.ts (1)
session(381-399)lib/utils.ts (4)
prepareArg(164-166)getMemoryUsage(256-275)getSSEGraphResult(95-138)Row(85-89)
app/api/graph/[graph]/memory/route.ts (3)
app/api/auth/DBVersion/route.ts (1)
GET(5-32)app/api/auth/[...nextauth]/options.ts (2)
session(381-399)getClient(403-480)lib/utils.ts (1)
MEMORY_USAGE_VERSION_THRESHOLD(12-12)
app/providers.tsx (2)
lib/utils.ts (6)
securedFetch(140-162)getQueryWithLimit(284-314)prepareArg(164-166)getSSEGraphResult(95-138)getMemoryUsage(256-275)MEMORY_USAGE_VERSION_THRESHOLD(12-12)app/api/graph/model.ts (4)
MemoryValue(117-117)GraphInfo(159-299)GraphInfo(419-421)GraphInfo(423-425)
app/graph/graphInfo.tsx (2)
app/components/provider.ts (2)
GraphContext(217-237)BrowserSettingsContext(156-215)app/api/graph/model.ts (9)
MemoryUsage(202-204)Labels(194-196)Labels(363-365)Labels(367-369)Relationships(198-200)Relationships(375-377)Relationships(379-381)PropertyKeys(186-188)PropertyKeys(190-192)
⏰ 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). (4)
- GitHub Check: Setup and cache dependencies
- GitHub Check: Setup and cache dependencies
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (11)
app/graph/selectGraph.tsx (2)
126-149: LGTM on row building logic.The
handleSetRowscallback correctly:
- Uses
sessionRoleto determine cell editability- Conditionally adds memory usage column based on
showMemoryUsage- Includes appropriate dependencies
151-160: Dependency arrays now includehandleSetRows—addresses prior feedback.Both
useEffecthooks at lines 151-156 and 158-160 correctly depend onhandleSetRows, which transitively depends onsessionRole. This ensures rows rebuild when the session role becomes available.app/api/graph/[graph]/memory/route.ts (1)
7-59: Overall structure is sound.The handler properly:
- Authenticates requests
- Feature-gates based on DB version
- Uses appropriate HTTP status codes (400 for client errors, 500 for server errors)
- Follows Next.js 15 async params pattern
app/graph/graphInfo.tsx (2)
86-97: Labels filtered for empty names—good defensive coding.The filter
({ name }) => !!nameensures empty label names don't render broken buttons.
8-11: Clean context consumption with destructuring.The destructuring pattern for extracting Labels, Relationships, PropertyKeys, and MemoryUsage from graphInfo is clean and readable.
app/providers.tsx (1)
392-406: DB version check effect is well-structured.The async IIFE pattern properly checks authentication status before fetching, and the version threshold check correctly enables memory usage features.
app/graph/page.tsx (4)
115-143: Graph info fetch with memory usage integration looks correct.The effect properly:
- Guards on
graphName- Conditionally fetches memory usage based on
showMemoryUsage- Creates GraphInfo with all required params
- Sets up interval for periodic refresh
- Returns cleanup function
180-190:handleSetIsAddcurrying pattern is clean.The helper creates toggled setters that manage mutual exclusivity between add-node and add-edge modes while updating panel state appropriately.
320-344: Add panel rendering handles both node and edge creation.The logic correctly:
- Provides close handler that resets all add states
- Renders node creation panel when
isAddNodeis true- Guards edge creation with
isTwoNodescheck- Passes selected nodes to edge creation panel
196-201: API correctly interpretstype: booleanfor node/edge distinction.The endpoint validates and uses the boolean
typefield as expected:type: truecreates nodes,type: falsecreates edges. The Zod schema atcreateSchemaElementexplicitly definestype: z.boolean(), and the POST handler atapp/api/graph/[graph]/[element]/route.ts(lines 91-105) correctly branches on this boolean to execute the appropriate CREATE query.app/api/auth/DBVersion/route.ts (1)
5-14: GET handler + getClient usage look consistent and correctThe
getClient()pattern with theinstanceof NextResponseguard matches the shared auth flow and cleanly reuses the central connection management, so the control flow here looks good and maintainable. Based on learnings, this also keeps the session-basedpasswordfield separate from PATpwdusage viagetClient, which is what we want for this route.
Update version to 1.6.0 in package.json and package-lock.json
…-new-text-to-cypher-version Feature/update pipeline to get new text to cypher version
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release-image.yml (1)
5-9: Review the default cypher_version value.The hardcoded default
v0.1.5may drift from the actual latest stable release. Consider either omitting a default (to force explicit input) or dynamically determining it during workflow initialization.Confirm whether
v0.1.5is actively maintained and remains compatible with the current browser application.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release-image.yml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup and cache dependencies
- GitHub Check: Setup and cache dependencies
| - 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/') | ||
| echo "CYPHER_VERSION=$LATEST_VERSION" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files .github/workflows/ | head -20Repository: FalkorDB/falkordb-browser
Length of output: 166
🏁 Script executed:
cat .github/workflows/release-image.ymlRepository: FalkorDB/falkordb-browser
Length of output: 1778
🌐 Web query:
GitHub API rate limit unauthenticated requests per hour documentation
💡 Result:
Unauthenticated REST API requests are limited to 60 requests per hour. (docs.github.com)
Citations:
🌐 Web query:
gh cli pre-installed ubuntu-latest GitHub Actions
💡 Result:
Short answer: Yes — the GitHub-hosted ubuntu-latest runner includes the GitHub CLI (gh) by default. [1][2]
Notes:
- If you run inside a container image or on a self‑hosted runner that doesn't include it, gh may be missing and you must install it yourself (or use a setup action). [2][3]
Sources:
- actions/runner-images (list of included software for ubuntu-latest). [GitHub].
- setup-gh / marketplace README (states hosted runners include gh by default; needed only for containers/self‑hosted). [GitHub Marketplace].
- GitHub Docs — customizing GitHub‑hosted runners (how to install software in jobs). [GitHub Docs].
🏁 Script executed:
# Verify the syntax of the proposed solution in the review comment
# The review shows a diff with what appears to be misplaced validation logic
python3 << 'EOF'
# Check if the proposed diff syntax is correct
proposed_code = """
- 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=$(gh release list --repo falkordb/text-to-cypher --limit 1 --json tagName -q '.[0].tagName')
echo "CYPHER_VERSION=$LATEST_VERSION" >> $GITHUB_ENV
fi
# Validate that version was set
if [ -z "$CYPHER_VERSION" ]; then
echo "ERROR: CYPHER_VERSION is empty"
exit 1
fi
"""
print("Proposed code structure:")
print(proposed_code)
print("\nValidation: The code looks syntactically correct.")
print("- Uses gh CLI with --json flag")
print("- gh CLI outputs JSON and uses -q to query tagName")
print("- Validation happens after fi (correct placement)")
EOFRepository: FalkorDB/falkordb-browser
Length of output: 935
🏁 Script executed:
# Check if GITHUB_TOKEN would be available in this workflow
grep -n "env:" .github/workflows/release-image.yml || echo "No explicit env section found at top level"Repository: FalkorDB/falkordb-browser
Length of output: 111
🌐 Web query:
gh release list --json tagName query syntax GitHub CLI
💡 Result:
You can output release tag names with gh like this:
-
To list tagName for all releases:
gh release list --limit 30 --json tagName --jq '.[].tagName'. [1] -
To get the single latest release tag:
gh release list --limit 1 --json tagName --jq '.[].tagName'. [1][2]
Notes:
- --json accepts a comma-separated list of fields (createdAt,isDraft,isPrerelease,name,publishedAt,tagName, etc.). Use --jq with a jq expression to extract values. [1]
Sources: GitHub CLI manual (gh release list --json fields). [1] Example usage on StackOverflow. [2]
🌐 Web query:
GitHub Actions default GITHUB_TOKEN available gh cli authentication
💡 Result:
Short answer: Yes — the Actions-generated token can be used by gh. Set the token in the job step environment (prefer GH_TOKEN) and ensure the job permissions you need.
How:
- gh will read GH_TOKEN (preferred) or GITHUB_TOKEN from the environment (GH_TOKEN takes precedence). [1][2]
- Example step:
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: gh
(Also set required permissions under jobs.*.permissions in the workflow.) [1][2][3]
Notes:
- Some older gh versions had issues when trying to run gh auth login with GITHUB_TOKEN present — the recommended pattern is to export GH_TOKEN (or pass token to gh via stdin with --with-token) rather than attempting interactive login. [4][2]
Sources:
[1] GitHub Docs — Use GITHUB_TOKEN for authentication in a workflow.
[2] GitHub CLI manual — gh auth login / environment (GH_TOKEN/GITHUB_TOKEN precedence).
[3] GitHub Blog — examples of using GITHUB_TOKEN with gh in Actions.
[4] cli/cli issue about GITHUB_TOKEN and gh auth login.
Replace fragile grep/sed parsing with jq or gh CLI, but use correct gh syntax.
The curl + grep/sed approach to parse the GitHub API response is error-prone and will break if the JSON structure changes. Additionally, unauthenticated GitHub API requests are rate-limited to 60/hour, which risks workflow failures on subsequent pushes to main.
Apply this diff to use the GitHub CLI with automatic authentication:
- 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=$(gh release list --repo falkordb/text-to-cypher --limit 1 --json tagName --jq '.[0].tagName')
echo "CYPHER_VERSION=$LATEST_VERSION" >> $GITHUB_ENV
+ fi
+ # Validate that version was set
+ if [ -z "$CYPHER_VERSION" ]; then
+ echo "ERROR: CYPHER_VERSION is empty"
+ exit 1
fiThe gh CLI is pre-installed on ubuntu-latest and automatically uses the GITHUB_TOKEN from the job context, eliminating rate-limit issues and providing robust error handling. The validation step ensures the workflow fails loudly if the version cannot be resolved.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/release-image.yml lines 20-28: replace the fragile
curl+grep/sed GitHub API parsing with the gh CLI to fetch the latest release
tag, write it to $GITHUB_ENV as CYPHER_VERSION, and validate that a non-empty
value was obtained; ensure the workflow uses the preinstalled gh (which
leverages GITHUB_TOKEN), run gh api repos/:owner/:repo/releases/latest or
equivalent gh command to extract tag_name, set CYPHER_VERSION in the
environment, and explicitly fail the step if the resolved version is empty so
the workflow fails loudly on resolution errors.
…ts array instead of individual selectedElement. Update related functions and props across ForceGraph, GraphView, PaginationList, Selector, and Toolbar components for improved state management.
…eleteSelected prop and remove selectedElement state. Update related logic to manage selectedElements array for improved selection handling.
…s across components for improved selection handling. Update related functions in ForceGraph, GraphView, DataPanel, and SchemaView to align with new prop type.
- Add storage abstraction with file and FalkorDB backends - Support API_TOKEN_FALKORDB_URL for production storage - Default to .data/api_tokens.json for local development - Consolidate token storage logic in lib/token-storage
…ent for handleSetSelectedElements, improving clarity in selection logic.
Fix #1262 Refactor selection handling in graph components to use selectedElemen…
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/PaginationList.tsx (1)
146-150: Guard Enter key handler when there is no valid itemOn Enter, you directly index
items[hoverIndex]. When the current page has no items (e.g., emptyfilteredList) orhoverIndexis out of range, this becomesundefinedand will throw at.text. This is a correctness issue and will surface as a runtime error on keyboard use. As per coding guidelines, keyboard flows should be robust and not crash on edge cases.Consider tightening the handler like this:
- if (e.key === "Enter") { - e.preventDefault() - onClick(typeof items[hoverIndex] === "string" ? items[hoverIndex] : items[hoverIndex].text, e) - } + if (e.key === "Enter") { + e.preventDefault() + if (hoverIndex < 0 || hoverIndex >= items.length) return + const currentItem = items[hoverIndex] + onClick( + typeof currentItem === "string" ? currentItem : currentItem.text, + e + ) + }Also applies to: 217-239, 250-257
♻️ Duplicate comments (6)
app/components/ForceGraph.tsx (1)
337-343: Verify backend supports GET endpoint for schema.The API path uses
/api/${type}/${graph.Id}/${node.id}wheretypecan be "schema". Per previous review, the schema route handler (app/api/schema/[schema]/[element]/_route.ts) may be missing a GET handler.#!/bin/bash # Check if schema route has GET handler rg -n "export async function GET" app/api/schema/app/graph/Selector.tsx (1)
190-201: Overlapping useEffect hooks callresetHistoryFilterstwice.The effect at lines 190-192 runs when
resetHistoryFilterschanges, and the effect at lines 194-201 also callsresetHistoryFilterswhenqueriesOpenchanges. This can causeresetHistoryFiltersto execute twice in succession.Consider consolidating into a single effect:
- useEffect(() => { - resetHistoryFilters() - }, [resetHistoryFilters]) - useEffect(() => { if (!queriesOpen) { setIsLoading(false) setTab("text") searchQueryRef.current?.blur() } resetHistoryFilters() }, [queriesOpen, resetHistoryFilters])app/schema/DataPanel.tsx (3)
337-337: Inconsistent optional chaining - potential runtime error.This line uses
session?.user.rolewhile line 353 usessession?.user?.role. Ifsession.useris undefined during auth transitions, accessing.roledirectly will throw.- type && session?.user.role !== "Read-Only" && + type && session?.user?.role !== "Read-Only" &&
516-516: Same optional chaining concern.- session?.user.role !== "Read-Only" && ( + session?.user?.role !== "Read-Only" && (
715-715: Same optional chaining concern.- session?.user.role !== "Read-Only" && + session?.user?.role !== "Read-Only" &&app/graph/page.tsx (1)
191-201: Guard against missinggraphNamebefore API call.The
handleCreateElementfunction constructs a URL withgraphNamebut doesn't early-return if it's falsy, risking requests toapi/graph/undefined/....const handleCreateElement = useCallback(async (attributes: [string, Value][], label: string[]) => { + if (!graphName) { + return false + } const fakeId = "-1" const result = await securedFetch(`api/graph/${prepareArg(graphName)}/${fakeId}`, {
🧹 Nitpick comments (3)
app/graph/page.tsx (1)
286-289: RedundantsetSelectedElementscalls.Line 288 calls
handleSetSelectedElements()which internally callssetSelectedElements([])and sets panel. Line 289 then callssetSelectedElements([])again unconditionally. This is redundant.if (panel === "data") handleSetSelectedElements() - else setSelectedElements([])Or consolidate to a single call regardless of panel state.
app/components/PaginationList.tsx (2)
121-129:isDeleteSelectedintegration is coherent (minor style tweak possible)The new
isDeleteSelected?: (item: T) => booleanhook-up, includingdeleteSelectedderivation anddefaultProps, is consistent with the existingisSelectedpattern and generics.If you want to shave a bit of boilerplate, you could simplify the per-item check using optional chaining:
-const deleteSelected = isDeleteSelected ? isDeleteSelected(item) : false +const deleteSelected = isDeleteSelected?.(item) ?? falseAlso applies to: 135-138, 253-254, 351-352
2-2: Avoid manufacturing a custom “rightclick” event objectPassing the actual React event objects through
onClickis good, but theonContextMenupath currently creates a plain object with a non‑standardtype: "rightclick"by spreadinge. That:
- Deviates from the standard event
type("contextmenu"), which callers might reasonably key off.- Loses the original prototype, which could surprise code that relies on methods/properties behaving exactly like a React
MouseEvent.You can keep the API and behavior while simplifying to just forward the original event:
- onClick={(e) => { - onClick(text, e) - }} - onContextMenu={(e) => { - e.preventDefault() - const syntheticEvent = { - ...e, - type: "rightclick" as const - } as typeof e & { type: "rightclick" } - onClick(text, syntheticEvent) - }} + onClick={(e) => { + onClick(text, e) + }} + onContextMenu={(e) => { + e.preventDefault() + onClick(text, e) + }}Callers that care about right‑clicks can distinguish using
evt.type === "contextmenu"orevt.button === 2.Also applies to: 121-124, 237-238, 286-296
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/components/ForceGraph.tsx(12 hunks)app/components/PaginationList.tsx(12 hunks)app/graph/GraphView.tsx(9 hunks)app/graph/Selector.tsx(12 hunks)app/graph/page.tsx(10 hunks)app/graph/toolbar.tsx(6 hunks)app/schema/DataPanel.tsx(10 hunks)app/schema/SchemaView.tsx(2 hunks)app/schema/_page.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/schema/_page.tsxapp/components/ForceGraph.tsxapp/graph/toolbar.tsxapp/components/PaginationList.tsxapp/graph/GraphView.tsxapp/graph/page.tsxapp/graph/Selector.tsxapp/schema/SchemaView.tsxapp/schema/DataPanel.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
🧬 Code graph analysis (7)
app/schema/_page.tsx (2)
lib/utils.ts (4)
securedFetch(140-162)prepareArg(164-166)isTwoNodes(403-406)cn(91-93)app/schema/DataPanel.tsx (1)
DataPanel(30-731)
app/components/ForceGraph.tsx (2)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)lib/utils.ts (2)
getNodeDisplayText(316-347)securedFetch(140-162)
app/graph/toolbar.tsx (4)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)lib/utils.ts (2)
GraphRef(21-23)cn(91-93)app/components/ui/Button.tsx (1)
Props(11-21)app/components/provider.ts (1)
GraphContext(217-237)
app/components/PaginationList.tsx (3)
lib/utils.ts (1)
cn(91-93)app/api/graph/model.ts (1)
Query(17-26)app/components/ui/Button.tsx (1)
Props(11-21)
app/graph/GraphView.tsx (4)
app/components/provider.ts (1)
GraphContext(217-237)lib/utils.ts (2)
Tab(51-51)cn(91-93)app/graph/toolbar.tsx (1)
Toolbar(31-333)app/graph/GraphDetails.tsx (1)
GraphDetails(8-106)
app/schema/SchemaView.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/schema/DataPanel.tsx (5)
app/api/graph/model.ts (3)
Node(55-66)Link(68-84)Graph(301-1101)app/graph/DataPanel.tsx (1)
DataPanel(25-207)app/components/ui/Button.tsx (1)
Props(11-21)app/components/provider.ts (1)
IndicatorContext(267-270)app/api/auth/[...nextauth]/options.ts (1)
session(381-399)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup and cache dependencies
- GitHub Check: Setup and cache dependencies
🔇 Additional comments (23)
app/schema/DataPanel.tsx (2)
14-14: LGTM - Import and signature updates align with the multi-element selection refactor.The import source change,
setObjectsignature update to accept optional array, and destructuringsetIndicatorfrom context are consistent with the broader PR changes.Also applies to: 25-25, 30-32
53-59: LGTM - Escape handler updated correctly.The change from
setObject(undefined)tosetObject()aligns with the new signature(el?: (Node | Link)[]) => void.app/schema/_page.tsx (2)
59-63: LGTM - Panel size calculation updated for new states.The memoized
panelSizecorrectly derives fromselectedElements,isAddNode, andisAddEdge.
65-79: LGTM - Selection handler with mutual exclusivity.The
handleSetSelectedElementscorrectly resets add-mode flags when elements are selected, maintaining mutually exclusive states.app/schema/SchemaView.tsx (2)
14-29: LGTM - Props interface updated for multi-element selection.The
setSelectedElementssignature change to accept optional array aligns with the broader refactor.
57-59: LGTM - Effect dependency cleanup.Dependency array correctly reflects the removal of
setSelectedElementprop.app/components/ForceGraph.tsx (4)
114-121: LGTM - Endpoint ID normalization helper.The
getEndpointIdhelper correctly handles all possible endpoint types (Node object, number, string) with proper null safety.
157-175: LGTM - Degree map computation is well-structured.The
nodeDegreeMapuseMemo correctly initializes nodes with 0 degree and increments for each link endpoint. Dependencies are appropriate.
254-294: LGTM - Degree-aware link forces.The distance and strength calculations based on node degree are well-implemented with clear thresholds and decay formulas.
408-418: LGTM - Right-click handler simplified.The removal of
isAddElement/setSelectedNodeslogic and consolidation to single/multi-select via ctrl-key is cleaner.app/graph/GraphView.tsx (3)
20-39: LGTM - Props interface extended for add-mode controls.The new
setIsAddNode,setIsAddEdge,isAddNode, andisAddEdgeprops are properly typed and thesetSelectedElementssignature update is consistent with the refactor.
74-78: LGTM - Tab enablement logic simplified.The Graph tab is now always enabled, with Metadata requiring both metadata content and explain data. This is a reasonable UX improvement.
125-137: LGTM - Toolbar integration with add-mode controls.The conditional
setIsAddEdgebased on having exactly 2 nodes selected correctly enables edge creation only when valid.app/graph/toolbar.tsx (2)
12-25: LGTM - Props interface updated for new add-mode controls.The interface correctly declares
graphName, updatedsetSelectedElementssignature, and the newisAddNode/isAddEdge/setIsAddNode/setIsAddEdgeprops withsetIsAddEdgebeing optional.
277-329: LGTM - Action buttons replace dropdown menu.The direct Info, Add Node, and Add Edge buttons provide clearer UX than the previous dropdown approach. The conditional rendering of the Add Edge button based on
setIsAddEdgeavailability is correct.app/graph/page.tsx (2)
179-189: LGTM - Curried add-mode handler.The
handleSetIsAddpattern cleanly manages mutual exclusivity between node/edge add modes while coordinating panel state.
117-142: LGTM - Memory usage integration.The
GraphInfo.createcall now properly includes memory usage data whenshowMemoryUsageis enabled, with appropriate fallback to empty Map.app/graph/Selector.tsx (4)
119-125: LGTM - Memoized filters computation.The filters memoization with conditional logic based on query count vs graphNames length is a reasonable optimization.
142-164: LGTM - Centralized history filter reset.The
resetHistoryFilterscallback properly handles graph-scoped filtering when a matching graphName exists, with fallback to all queries.
291-311: LGTM - Multi-select deletion handler.The
handleDeleteQuerycorrectly filters out selected indices, updates localStorage, and recalculates counter/query state.
583-598: LGTM - Schema selector toolbar integration.The conditional rendering properly gates toolbar display and passes the new add-mode props when in Schema mode.
app/components/PaginationList.tsx (2)
32-37: Centralized selection/delete/hover styling looks solidExtracting
getItemClassName, threadingselected/deleteSelected/hoverthroughgetSeparatorandgetQueryElement, and reusing it on the<li>keeps the visual states consistent for both metadata chips and row borders/text. I don’t see issues with this refactor.Also applies to: 39-42, 57-119, 262-263, 270-273
338-341:>>pagination button math is consistentThe updated disabled condition
stepCounter > pageCount - 6lines up with the click handler guardprev < pageCount - 5, so you can’t jump past the last page, and the control naturally disables once you’re within the last (up to) 5 pages. Behavior looks correct even for smallpageCountvalues.
| <div className={cn("flex flex-row-reverse gap-2", label === "Schema" && "h-full")}> | ||
| { | ||
| graph.Id && | ||
| graphName && session?.user.role !== "Read-Only" && |
There was a problem hiding this comment.
Optional chaining concern - potential runtime error.
Using session?.user.role can throw if session exists but session.user is undefined during auth transitions.
- graphName && session?.user.role !== "Read-Only" &&
+ graphName && session?.user?.role !== "Read-Only" &&🤖 Prompt for AI Agents
In app/graph/toolbar.tsx around line 279, the expression uses session?.user.role
which can throw if session exists but session.user is undefined during auth
transitions; update the condition to safely access role (e.g., use
session?.user?.role or guard session.user before accessing role) so the check
becomes null-safe and avoids runtime errors during transient auth states.
| const getCurrentPanel = () => { | ||
| if (selectedElements.length !== 0) { | ||
| return ( | ||
| <DataPanel | ||
| object={selectedElements[selectedElements.length - 1]} | ||
| setObject={handleSetSelectedElements} | ||
| schema={schema} | ||
| setLabels={setLabels} | ||
| /> | ||
| ) | ||
| } | ||
|
|
||
| if (isAddNode) { | ||
| return ( | ||
| <SchemaCreateElement | ||
| onCreate={handleCreateElement} | ||
| setIsAdd={handleSetIsAddNode} | ||
| type | ||
| /> | ||
| ) | ||
| } | ||
|
|
||
| if (isTwoNodes(selectedElements)) { | ||
| return ( | ||
| <SchemaCreateElement | ||
| onCreate={handleCreateElement} | ||
| setIsAdd={handleSetIsAddEdge} | ||
| selectedNodes={selectedElements} | ||
| setSelectedNodes={setSelectedElements} | ||
| type={false} | ||
| /> | ||
| ) | ||
| } | ||
|
|
||
| return null | ||
| } |
There was a problem hiding this comment.
Logic issue: Edge creation panel condition is unreachable.
The isTwoNodes(selectedElements) check at line 272 will never be reached because selectedElements.length !== 0 at line 251 returns early with DataPanel. When two nodes are selected for edge creation, DataPanel will render instead of CreateElementPanel.
Consider restructuring to check isAddEdge state:
const getCurrentPanel = () => {
- if (selectedElements.length !== 0) {
+ if (selectedElements.length !== 0 && !isAddEdge) {
return (
<DataPanel
object={selectedElements[selectedElements.length - 1]}
setObject={handleSetSelectedElements}
schema={schema}
setLabels={setLabels}
/>
)
}
if (isAddNode) {
return (
<SchemaCreateElement
onCreate={handleCreateElement}
setIsAdd={handleSetIsAddNode}
type
/>
)
}
- if (isTwoNodes(selectedElements)) {
+ if (isAddEdge && isTwoNodes(selectedElements)) {
return (
<SchemaCreateElement
onCreate={handleCreateElement}
setIsAdd={handleSetIsAddEdge}
selectedNodes={selectedElements}
setSelectedNodes={setSelectedElements}
type={false}
/>
)
}📝 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 getCurrentPanel = () => { | |
| if (selectedElements.length !== 0) { | |
| return ( | |
| <DataPanel | |
| object={selectedElements[selectedElements.length - 1]} | |
| setObject={handleSetSelectedElements} | |
| schema={schema} | |
| setLabels={setLabels} | |
| /> | |
| ) | |
| } | |
| if (isAddNode) { | |
| return ( | |
| <SchemaCreateElement | |
| onCreate={handleCreateElement} | |
| setIsAdd={handleSetIsAddNode} | |
| type | |
| /> | |
| ) | |
| } | |
| if (isTwoNodes(selectedElements)) { | |
| return ( | |
| <SchemaCreateElement | |
| onCreate={handleCreateElement} | |
| setIsAdd={handleSetIsAddEdge} | |
| selectedNodes={selectedElements} | |
| setSelectedNodes={setSelectedElements} | |
| type={false} | |
| /> | |
| ) | |
| } | |
| return null | |
| } | |
| const getCurrentPanel = () => { | |
| if (selectedElements.length !== 0 && !isAddEdge) { | |
| return ( | |
| <DataPanel | |
| object={selectedElements[selectedElements.length - 1]} | |
| setObject={handleSetSelectedElements} | |
| schema={schema} | |
| setLabels={setLabels} | |
| /> | |
| ) | |
| } | |
| if (isAddNode) { | |
| return ( | |
| <SchemaCreateElement | |
| onCreate={handleCreateElement} | |
| setIsAdd={handleSetIsAddNode} | |
| type | |
| /> | |
| ) | |
| } | |
| if (isAddEdge && isTwoNodes(selectedElements)) { | |
| return ( | |
| <SchemaCreateElement | |
| onCreate={handleCreateElement} | |
| setIsAdd={handleSetIsAddEdge} | |
| selectedNodes={selectedElements} | |
| setSelectedNodes={setSelectedElements} | |
| type={false} | |
| /> | |
| ) | |
| } | |
| return null | |
| } |
🤖 Prompt for AI Agents
In app/schema/_page.tsx around lines 250 to 285, the current early return for
selectedElements (lines ~251) causes the isTwoNodes(selectedElements) branch
(line ~272) to never run, so the edge-creation panel is unreachable; reorder the
checks so edge creation is handled before the generic selectedElements
DataPanel: specifically, check the isAddEdge (or isTwoNodes(selectedElements)
combined with an isAddEdge flag) case first and render the SchemaCreateElement
for edges when that state is set, then fall back to rendering DataPanel for
single selection and other cases; update the props passed accordingly (keep
type={false} for edges) and ensure state setters (setIsAddEdge /
setSelectedElements) are used in that branch.
feat: add configurable token storage
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/api/auth/tokens/route.ts (1)
145-161: Expiration parameter validation needs hardening.The validation for
expiresAtandttlSecondsdoesn't guard againstInvalid Dateor string coercion issues, as previously noted.
🧹 Nitpick comments (19)
app/api/auth/tokens/route.ts (3)
45-47: Consider defensive checks for timestamp conversion.If storage returns invalid numeric values (NaN, undefined, or non-numeric),
new Date(token.created_at * 1000)yields an Invalid Date, andtoISOString()will throw aRangeError. This would bubble up as a generic 500 error rather than graceful handling.- created_at: new Date(token.created_at * 1000).toISOString(), - expires_at: token.expires_at > 0 ? new Date(token.expires_at * 1000).toISOString() : null, - last_used: token.last_used > 0 ? new Date(token.last_used * 1000).toISOString() : null, + created_at: Number.isFinite(token.created_at) ? new Date(token.created_at * 1000).toISOString() : null, + expires_at: token.expires_at > 0 && Number.isFinite(token.expires_at) ? new Date(token.expires_at * 1000).toISOString() : null, + last_used: token.last_used > 0 && Number.isFinite(token.last_used) ? new Date(token.last_used * 1000).toISOString() : null,
236-242: Use 201 Created for resource creation.REST convention is to return
201 Createdwhen a new resource is successfully created.return NextResponse.json( { message: "Token created successfully", token }, - { status: 200 } + { status: 201 } );
201-203: Type-unsafe password access.The
(user as any).passwordcast bypasses type safety. If the user object doesn't have a password (e.g., different auth flow), this silently encrypts an empty string. Consider adding explicit type handling or a guard.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - const password = (user as any).password || ''; + const password = 'password' in user && typeof user.password === 'string' + ? user.password + : '';Based on learnings, session tokens store
passwordwhile PATs usepwd. Verify this access pattern aligns with the session token flow..env.local.template (1)
2-7: Env variables look coherent; consider satisfying dotenv‑linter hintsThe added variables (CHAT_URL, NEXTAUTH_URL, PORT, API_TOKEN_STORAGE_PATH) line up with the new token/chat flows and file‑based storage path. Functionally this is fine.
If you’re running
dotenv-linterin CI, you may want to:
- Reorder keys so
NEXTAUTH_SECRETcomes beforeNEXTAUTH_URL, andAPI_TOKEN_STORAGE_PATHcomes beforeCHAT_URL.- Add a trailing newline at the end of the file.
These are purely stylistic and not required for correctness.
lib/token-storage/FileTokenStorage.ts (1)
21-55: Behavior on corrupted JSON gracefully degrades but drops all tokensIf the JSON file becomes unreadable,
readTokenslogs the error and returns an empty array. That’s a reasonable “fail closed” approach (no tokens are treated as valid), but it does mean all existing tokens become effectively invisible until the file is fixed.If you want stronger safety around operational incidents, consider:
- Surfacing a more explicit error in admin UIs, or
- Backing up the corrupted file before overwriting it on the next write.
Not urgent, just something to be aware of.
lib/token-storage/falkordb-client.ts (1)
53-94: PAT FalkorDB client behavior looks robust for typical usageThe PAT‑specific client does a good job of:
- Reusing an existing connection when healthy (via
ping),- Falling back from
API_TOKEN_FALKORDB_URLtoPAT_FALKORDB_*envs, and- Verifying connectivity before returning the client.
For most workloads this should be sufficient. If you ever expect extremely high concurrency at startup, you might later consider guarding against multiple simultaneous
connectcalls by serializing the initial connection, but that’s an optimization rather than a blocker.e2e/logic/POM/settingsTokensPage.ts (5)
1-1: Unnecessary global eslint disableThere are no
any-typed values in this file, so/* eslint-disable @typescript-eslint/no-explicit-any */is redundant and slightly weakens linting. Consider removing it or narrowing the disable to where it’s actually needed.
55-61: Scope row locators to the token table to avoid accidental matchesHelpers like
tokenRowByName,getRowByText,isTokenRowVisible,getTokenCount, andgetAllTokenNamesall usethis.page.getByRole("row"), which can pick up unrelated rows if other tables or row-like components exist on the page. This can cause subtle test failures as the Settings page evolves.You can make these more robust by scoping them to
this.tokenTable, for example:- private tokenRowByName(name: string): Locator { - return this.page.getByRole("row").filter({ hasText: name }); - } + private tokenRowByName(name: string): Locator { + return this.tokenTable.getByRole("row").filter({ hasText: name }); + }and similarly for
getRowByText,getTokenCount, andgetAllTokenNames.Also applies to: 86-88, 196-203, 304-311, 353-371
33-45: Reduce reliance on fixed timeouts; favor condition-based waitsMethods like
waitFor,navigateToTokensTab,clickGenerateToken,confirmGenerateToken,generateToken,copyToken,dismissTokenDisplay, andrevokeTokenuse hard-codedwaitForTimeoutvalues (300–1000 ms). These can be flaky across environments and CI load.Where possible, prefer condition-based waits (e.g.,
expect(locator).toBeVisible(), waiting for specific rows/toasts to appear/disappear, or waiting for network responses) instead of arbitrary sleeps. You already haveinteractWhenVisibleandtokenTable.waitFor, so extending that pattern here would make the POM more resilient.Also applies to: 47-49, 81-85, 223-227, 229-232, 252-255, 262-270, 283-287, 290-293, 314-318
34-41: Consider making token-specific locators more targetedLocators like
generatedTokenInput(input[readonly].first()),copyTokenButton(name: /Copy/i), anddismissTokenButton(name: /I've saved my token/i) may accidentally match other controls if the UI grows.If the UI exposes data attributes or test IDs for the token display (e.g.,
data-testid="generated-token",data-testid="copy-token"), consider switching to those for long-term stability. The current approach is fine for now but somewhat brittle.Also applies to: 73-79, 147-154, 275-280
353-371: Align cell text handling betweengetAllTokenNamesandgetTokenDetails
getAllTokenNamestrims cell text (name.trim()), butgetTokenDetailsreturns rawtextContent()forname,created,lastUsed, andexpires. If the table markup introduces extra whitespace or line breaks, assertions likeexpect(details?.name).toBe(tokenName)may fail whilegetAllTokenNamesbehaves differently.To keep behavior consistent and avoid whitespace-related flakes, consider trimming all cell texts in
getTokenDetailsas well:- name: nameIdx >= 0 ? (await cells[nameIdx]?.textContent() || "") : "", - created: createdIdx >= 0 ? (await cells[createdIdx]?.textContent() || null) : null, - lastUsed: lastUsedIdx >= 0 ? (await cells[lastUsedIdx]?.textContent() || null) : null, - expires: expiresIdx >= 0 ? (await cells[expiresIdx]?.textContent() || null) : null, + name: nameIdx >= 0 ? ((await cells[nameIdx]?.textContent())?.trim() || "") : "", + created: createdIdx >= 0 ? ((await cells[createdIdx]?.textContent())?.trim() || null) : null, + lastUsed: lastUsedIdx >= 0 ? ((await cells[lastUsedIdx]?.textContent())?.trim() || null) : null, + expires: expiresIdx >= 0 ? ((await cells[expiresIdx]?.textContent())?.trim() || null) : null,Also applies to: 377-401
e2e/tests/settingsTokens.spec.ts (8)
57-91: Simplify asyncreduceusage for sequential token generationUsing
tokenNames.reduce(async (prevPromise, tokenName) => { ... })works but is harder to read than a simplefor...ofloop and doesn’t actually take advantage ofreduce.You can simplify without changing behavior:
- await tokenNames.reduce(async (previousPromise, tokenName) => { - await previousPromise; - const token = await settingsTokensPage.generateToken(tokenName); - expect(token).not.toBeNull(); - await settingsTokensPage.dismissTokenDisplay(); - return Promise.resolve(); - }, Promise.resolve()); + for (const tokenName of tokenNames) { + const token = await settingsTokensPage.generateToken(tokenName); + expect(token).not.toBeNull(); + await settingsTokensPage.dismissTokenDisplay(); + }Same for the cleanup reduce below.
131-144: Reducing flakiness in “no name” validation testThe test correctly verifies that the generate-confirm button stays disabled when the token name is empty. The explicit
waitFor(300)introduces timing sensitivity though.Because
isGenerateTokenConfirmButtonDisabled()already uses locator state, you can usually rely on Playwright’s auto-waiting and drop or increase the arbitrary delay, or assert directly withawait expect(locator).toBeDisabled()if you expose the button locator from the POM.
164-194: Reuse POM helper instead of direct row/button interactionIn the “Cancel token revocation” test, you reach into the row and click the first button directly:
const row = settingsTokensPage.getRowByText(tokenName); await row.getByRole("button").click();Since
SettingsTokensPagealready exposesclickRevokeButtonByName, using it here would keep tests aligned with the POM abstraction and reduce duplication of locator logic:await settingsTokensPage.clickRevokeButtonByName(tokenName); await settingsTokensPage.cancelRevokeToken();Functionally equivalent, but easier to maintain if the revoke button changes.
220-270: Token count-based assertions may be sensitive to external stateThe revocation tests use
getTokenCount()and expect the count to decrease by 1 or 2. This is fine as long as:
- No other tests mutate tokens concurrently for the same user, and
getTokenCount()only reflects the PAT table rows.If in the future tests are run fully parallel or other tables appear on the Settings page, these count-based checks could become flaky. If that happens, consider asserting more directly on the presence/absence of rows by name (as you already do elsewhere) instead of relative counts.
315-337: Test name vs. assertion gap for “dialog dismisses after refresh”This test is named and commented as validating that the token generation success dialog disappears after a page refresh, but the only assertion is that the token appears in the list.
To enforce the intended behavior, you might also assert that the success dialog (or its dismiss button) is no longer interactable after refresh — for example, by:
- Adding a POM helper that checks the visibility of the success dialog/dismiss button, and
- Asserting it’s not visible or catching an expected failure when trying to interact with it.
That would align the assertion with the test’s stated purpose.
425-455: Assertions on exact API messages may be brittleThese API tests assert exact response messages:
expect(response.message).toBe("Token created successfully"); ... expect(revokeResponse.message).toContain("revoked successfully");If backend wording changes slightly (punctuation, capitalization, additional context), these will fail even though semantics remain correct. If you want the tests to be more future-proof, consider:
- Asserting on HTTP status codes and core fields (
token,tokens,count, etc.), and/or- Using
toContainor regex for messages where exact phrasing isn’t critical.This trades a bit of strictness for better long-term stability.
628-734: Nice role-based coverage; consider small DRY helpersThe “Basic Functionality - All User Types” block verifies that:
- ReadOnly and ReadWrite users can generate tokens via UI.
- Each can revoke their own tokens, with counts decreasing as expected.
There’s some repetition across these tests (generate token name, create via API or UI, open SettingsTokensPage, navigate, verify existence, revoke, optionally check counts). If this suite grows further, extracting tiny helpers (e.g.,
openTokensAsRole,createUserTokenViaApi) could make the tests shorter and easier to extend, but the current structure is perfectly serviceable.
1-735: Suggested conventional-commit style PR titlesNot code-related, but for this change set you might consider PR titles like:
test: add e2e coverage for PAT settings tokens UI and APIfeat: introduce PAT settings tokens page object and end-to-end testsEither would fit conventional-commit conventions while reflecting the scope here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.env.local.template(1 hunks).gitignore(1 hunks)app/api/auth/tokenUtils.ts(3 hunks)app/api/auth/tokens/[tokenId]/route.ts(1 hunks)app/api/auth/tokens/credentials/route.ts(1 hunks)app/api/auth/tokens/route.ts(2 hunks)e2e/logic/POM/settingsTokensPage.ts(1 hunks)e2e/tests/settingsTokens.spec.ts(1 hunks)lib/token-storage/FalkorDBTokenStorage.ts(1 hunks)lib/token-storage/FileTokenStorage.ts(1 hunks)lib/token-storage/ITokenStorage.ts(1 hunks)lib/token-storage/StorageFactory.ts(1 hunks)lib/token-storage/falkordb-client.ts(1 hunks)lib/token-storage/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/api/auth/tokens/[tokenId]/route.ts
- app/api/auth/tokenUtils.ts
- app/api/auth/tokens/credentials/route.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:
lib/token-storage/StorageFactory.tslib/token-storage/FileTokenStorage.tslib/token-storage/falkordb-client.tse2e/logic/POM/settingsTokensPage.tse2e/tests/settingsTokens.spec.tslib/token-storage/index.tslib/token-storage/FalkorDBTokenStorage.tsapp/api/auth/tokens/route.tslib/token-storage/ITokenStorage.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/logic/POM/settingsTokensPage.tse2e/tests/settingsTokens.spec.ts
**/api/**/*
⚙️ CodeRabbit configuration file
Review API routes for security, error handling, and proper HTTP status codes.
Files:
app/api/auth/tokens/route.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
📚 Learning: 2025-11-11T13:42:45.194Z
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
Applied to files:
lib/token-storage/falkordb-client.tslib/token-storage/FalkorDBTokenStorage.tsapp/api/auth/tokens/route.ts
🧬 Code graph analysis (5)
lib/token-storage/StorageFactory.ts (2)
lib/token-storage/ITokenStorage.ts (1)
ITokenStorage(34-78)lib/token-storage/index.ts (1)
ITokenStorage(2-2)
lib/token-storage/FileTokenStorage.ts (1)
lib/token-storage/ITokenStorage.ts (3)
ITokenStorage(34-78)TokenData(4-18)TokenFetchOptions(23-28)
e2e/logic/POM/settingsTokensPage.ts (2)
e2e/logic/POM/headerComponent.ts (1)
HeaderComponent(6-152)e2e/infra/utils.ts (1)
interactWhenVisible(192-200)
e2e/tests/settingsTokens.spec.ts (4)
e2e/infra/ui/browserWrapper.ts (1)
BrowserWrapper(14-87)e2e/logic/api/apiCalls.ts (1)
ApiCalls(88-602)e2e/logic/POM/settingsTokensPage.ts (1)
SettingsTokensPage(6-436)e2e/infra/utils.ts (1)
getRandomString(186-190)
lib/token-storage/FalkorDBTokenStorage.ts (2)
lib/token-storage/ITokenStorage.ts (3)
ITokenStorage(34-78)TokenData(4-18)TokenFetchOptions(23-28)lib/token-storage/falkordb-client.ts (1)
executePATQuery(106-114)
🪛 dotenv-linter (4.0.0)
.env.local.template
[warning] 4-4: [UnorderedKey] The NEXTAUTH_SECRET key should go before the NEXTAUTH_URL key
(UnorderedKey)
[warning] 7-7: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 7-7: [UnorderedKey] The API_TOKEN_STORAGE_PATH key should go before the CHAT_URL key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup and cache dependencies
- GitHub Check: Setup and cache dependencies
🔇 Additional comments (12)
app/api/auth/tokens/route.ts (1)
63-104: LGTM!The GET handler has clean authentication flow, proper error propagation, and sensible fallback defaults. The non-null assertion on line 91 is safe given the preceding error check.
.gitignore (1)
33-35: Ignore.data/for local token storage is appropriateIgnoring the
.data/directory aligns with the new file-based token storage and helps avoid committing local PAT/token data. Looks good as-is.lib/token-storage/ITokenStorage.ts (1)
4-78: Token storage interfaces are clear and aligned with implementations
TokenData,TokenFetchOptions, andITokenStorageline up well with both file‑based and FalkorDB implementations (including timestamp semantics andis_active/expiry handling). The comments about Unix timestamps and-1sentinels make the contract explicit. No issues from a typing/abstraction standpoint.lib/token-storage/StorageFactory.ts (1)
1-71: StorageFactory selection & defaults look solidThe singleton factory cleanly abstracts storage selection (FalkorDB vs file) based on
API_TOKEN_FALKORDB_URL, and the default path logic matches the file‑based implementation.reset()plus theisFalkorDBStorage/isFileStoragehelpers are handy for tests and diagnostics. No changes needed here.lib/token-storage/falkordb-client.ts (1)
1-3: Deep import ofFalkorDBOptionsmay be brittle across library upgradesYou’re importing
FalkorDBOptionsfromfalkordb/dist/src/falkordb, which reaches into the package’s internal structure. That can break on library updates if the internal path changes.If the library exposes this type from its public entrypoint, it would be safer to import from
"falkordb"instead (or use a localtypealias based on the connection options you actually pass).Please double‑check the current
falkordbdocs to see whetherFalkorDBOptions(or an equivalent type) is publicly exported and update the import accordingly if possible.lib/token-storage/index.ts (1)
1-14: Centralized token-storage exports are well structuredBringing all token storage types, implementations, the factory, and FalkorDB helpers under a single
lib/token-storageentrypoint is a nice cleanup and makes consumers’ imports much simpler. No issues here.Suggested PR title (conventional commit style):
feat: add token storage backends and PAT FalkorDB integrationlib/token-storage/FalkorDBTokenStorage.ts (1)
39-82: Token listing and filtering logic is consistent with the file backendThe
fetchTokensimplementation:
- Returns only
t.is_active = truetokens,- Applies username/host/port filtering for non‑admin callers, and
- Sorts by
created_at DESCon the DB side.This matches the behavior in
FileTokenStorage.fetchTokensand should give consistent results across backends. No changes needed here.e2e/logic/POM/settingsTokensPage.ts (1)
407-435: Nice dynamic header handling for token details
getTableColumnHeaders+hasColumnHeader+getTokenDetailsmake the POM robust against column order changes and optional columns (Username/Role for admins only). This is a solid pattern for UI tests where table layouts may evolve.e2e/tests/settingsTokens.spec.ts (4)
13-21: Setup/teardown pattern is clear and isolatedCreating a fresh
BrowserWrapperandApiCallsinbeforeEachand closing the browser inafterEachkeeps tests isolated and makes lifecycle explicit. This works well with the POM and API helper abstractions you’ve introduced.
22-55: Good coverage of token expirations; UI coupling noteThe parametrized
expirationOptionssuite nicely covers “never” and all TTL variants and then validates both the generated token and presence in the list. The extragetTokenDetailscheck onexpiresadds confidence.Just keep in mind these tests assume the UI string for non-expiring tokens is exactly
"Never". If that label ever changes (e.g., to"No expiration"), these assertions will start failing; if you want them less brittle, you could switch to a more tolerant check (contains/regex) or assert on structure rather than exact text.
95-127: Admin details test is strong; be aware of UI text assumptionsThe “admin-specific columns” test effectively validates:
- API-created token appears in the UI.
- Admin-only columns (
Username,Role) are present.getTokenDetailsreturns expected values (name,created,lastUsed).Like the earlier suite, this assumes
lastUseddisplays exactly"Never"for new tokens. If that label ever changes (capitalization, wording), this assertion will fail even though behavior is correct, so consider whether strict equality is desired here.
516-626: Multi-user isolation scenarios are well coveredThe four isolation tests comprehensively validate that:
- ReadWrite cannot see ReadOnly or Admin tokens.
- ReadOnly cannot see ReadWrite or Admin tokens.
- Each scenario both creates and cleans up tokens via the appropriate user-scoped APIs.
The structure is consistent and the cleanup logic ensures tests don’t leak state across runs.
tests maintenance
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
e2e/logic/api/apiCalls.ts (3)
427-478: Tighten response types for admin token helpersNice job wiring admin token helpers and introducing
ListTokensResponse/TokenDetailsResponse. A few methods still usePromise<any>, which weakens type safety:
generateToken(...): Promise<any>revokeToken(tokenId: string): Promise<any>Given you already have token response typings, consider introducing concrete types here as well (e.g.,
GenerateTokenResponse,RevokeTokenResponse) and using them both in the function signatures and casts:- async revokeToken(tokenId: string): Promise<any> { + async revokeToken(tokenId: string): Promise<RevokeTokenResponse> { try { const headers = await getAdminToken(); const result = await deleteRequest( `${urls.api.tokenUrl}tokens/${tokenId}`, headers ); - return await result.json(); + return (await result.json()) as RevokeTokenResponse; } catch (error) { throw new Error( `Failed to revoke token. \n Error: ${(error as Error).message}` ); } }Same idea for
generateToken. This will make the e2e helpers more self-documenting and reduce accidental misuse of the parsed JSON.
480-496: Revoke-all token logic now correctly attempts all revocationsSwitching
revokeAllUserTokenstoPromise.allSettledand summarizing failures ensures every token is attempted for revocation, and partial failures are surfaced with a clear error message:const results = await Promise.allSettled( tokens.map((tokenId) => this.revokeToken(tokenId)) );This addresses the earlier fail‑fast behavior from
Promise.all; the implementation looks good.
498-579: Improve cleanup robustness and typing in multi-user token helpersThe multi-user helpers are much improved—
generateTokenAsUserencapsulates credential‑based token creation, and bothlistTokensAsUserandrevokeTokenAsUsernow revoke their temporary token viathis.revokeToken(token_id).Two follow-ups would make these safer and clearer:
Ensure cleanup runs even on failures
Right now, the temporary token is revoked only after a successful list/revoke call. If
generateTokenAsUseror the main request fails, that token is never cleaned up. Wrapping the cleanup in afinally(and guarding against missingtoken_id) avoids leaks:async listTokensAsUser(username: string, password: string): Promise<ListTokensResponse> { let token_id: string | undefined; let token: string | undefined; try { const loginResponse = await this.generateTokenAsUser(username, password); ({ token, token_id } = loginResponse); const result = await getRequest( `${urls.api.tokenUrl}tokens`, { Authorization: `Bearer ${token}` } ); return (await result.json()) as ListTokensResponse; } catch (error) { throw new Error( `Failed to list tokens as user ${username}. \n Error: ${(error as Error).message}` ); } finally { if (token_id) { try { await this.revokeToken(token_id); } catch { // Optionally log cleanup failure; avoid masking the main error. } } } }A similar pattern can be applied to
revokeTokenAsUser.Avoid
Promise<any>on these helpers
listTokensAsUsercan likely returnPromise<ListTokensResponse>.revokeTokenAsUsercan use the same revoke response type you choose forrevokeToken.This keeps multi-user flows aligned with the admin helpers and improves type safety in tests.
🧹 Nitpick comments (7)
e2e/logic/api/responses/settingsResponses.ts (1)
1-7: Interfaces look fine; consider clarifyingconfigstructureThe response interfaces are syntactically correct and usable as e2e typings. If you want to make future maintenance easier, consider either documenting what
configrepresents (especially the tuple[string, number]) or replacing the tuple with an object with named fields to make call sites more self-explanatory.e2e/logic/api/responses/userResponses.ts (2)
1-7: LGTM with an optional type safety enhancement.The interface structure is correct and well-defined. Consider typing
roleas a union type if the set of roles is known (e.g.,'admin' | 'user' | 'viewer') for stricter type safety in your e2e tests.
9-15: Consider consolidating duplicate interfaces.
CreateUsersResponseandDeleteUsersResponsehave identical structures. You could reduce duplication by defining a shared type:export interface MessageResponse { message: string; } export type CreateUsersResponse = MessageResponse; export type DeleteUsersResponse = MessageResponse;Or use the single interface for both operations if that better reflects your API design.
e2e/logic/api/responses/tokenResponse.ts (1)
1-19: Avoid duplicating the Token shape in TokenDetailsResponse
TokenDetailsResponserepeats the exact fields ofToken. To keep types DRY and easier to evolve, you can alias the details type toToken:-export interface TokenDetailsResponse { - token_id: string; - name: string; - createdAt: string; - expiresAt: string | null; - lastUsed: string | null; -} +export type TokenDetailsResponse = Token;This way, any future changes to the token shape only need to be made in one place.
e2e/logic/api/responses/graphResponses.ts (1)
1-54: Minor naming and typing tweaks for graph responsesTwo small nits here:
- The interface name
DuplicateGraphresponsebreaks the usualPascalCaseconvention used by the other types. Renaming it toDuplicateGraphResponse(and updating imports/usages) would improve consistency.RunQueryResponse.datais typed asArray<Record<string, any>>under a file-levelno-explicit-anydisable. For e2e this is acceptable, but if you find yourself consuming this data more heavily, consider tightening it tounknownor a narrower shape and casting at the call sites.These are non-blocking and can be addressed later if you prefer.
e2e/logic/api/apiCalls.ts (2)
45-97: Harden SSE error handling for unexpected error payloadsIn
getSSEGraphResult, the"error"listener assumesevent.datais JSON with a{ message }shape:const { message } = JSON.parse(event.data);If the server ever emits a non‑JSON error or omits
message, this will throw inside the event handler and bypass your intendedreject, making failures harder to debug.You can make this more robust and also standardize on rejecting with an
Error:- evtSource.addEventListener("error", (event: MessageEvent) => { - handled = true; - const { message } = JSON.parse(event.data); - evtSource.close(); - reject(message); - }); + evtSource.addEventListener("error", (event: MessageEvent) => { + handled = true; + let message = "Unknown error"; + try { + const parsed = JSON.parse(event.data); + if (parsed && typeof parsed.message === "string") { + message = parsed.message; + } + } catch { + // Fallback to generic message if payload is not JSON + } + evtSource.close(); + reject(new Error(message)); + });Callers that use
(error as Error).messagewill then behave more predictably.
592-601: Use schema-specific response type for removeSchema
removeSchemacurrently returnsPromise<RemoveGraphResponse>:async removeSchema(schemaName: string): Promise<RemoveGraphResponse> {Given you have a dedicated
RemoveSchemaResponsetype ine2e/logic/api/responses/schemaResponses.ts, it would be clearer to use that here:-import { AddSchemaResponse, SchemaListResponse } from "./responses/schemaResponses"; +import { + AddSchemaResponse, + SchemaListResponse, + RemoveSchemaResponse, +} from "./responses/schemaResponses"; ... - async removeSchema(schemaName: string): Promise<RemoveGraphResponse> { + async removeSchema(schemaName: string): Promise<RemoveSchemaResponse> {Even if both shapes are currently
{ message: string }, this makes the intent explicit and helps future readers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
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/tokenResponse.ts(1 hunks)e2e/logic/api/responses/userResponses.ts(1 hunks)
💤 Files with no reviewable changes (19)
- e2e/logic/api/responses/removeGraphResponse.ts
- e2e/logic/api/responses/getUsersResponse.ts
- e2e/logic/api/responses/getSettingsRoleValue.ts
- e2e/logic/api/responses/LoginResponse.ts
- e2e/logic/api/responses/modifySettingsRoleResponse.ts
- e2e/logic/api/responses/deleteUsersResponse.ts
- e2e/logic/api/responses/getGraphsResponse.ts
- e2e/logic/api/responses/addGraphResponse.ts
- e2e/logic/api/responses/addSchemaResponse.ts
- e2e/logic/api/responses/duplicateGraph.ts
- e2e/logic/api/responses/logoutResponse.ts
- e2e/logic/api/responses/removeSchemaResponse.ts
- e2e/logic/api/responses/changeGraphNameResponse.ts
- e2e/logic/api/responses/runQueryResponse.ts
- e2e/logic/api/responses/graphNodeResponse.ts
- e2e/logic/api/responses/graphAttributeResponse.ts
- e2e/logic/api/responses/createUsersResponse.ts
- e2e/logic/api/responses/getSchemaResponse.ts
- e2e/logic/api/responses/graphCountResponse.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/userResponses.tse2e/logic/api/responses/authResponses.tse2e/logic/api/responses/graphResponses.tse2e/logic/api/responses/settingsResponses.tse2e/logic/api/responses/tokenResponse.tse2e/logic/api/responses/schemaResponses.tse2e/logic/api/apiCalls.ts
e2e/**/*
⚙️ CodeRabbit configuration file
Review test code for proper assertions, test coverage, and maintainability.
Files:
e2e/logic/api/responses/userResponses.tse2e/logic/api/responses/authResponses.tse2e/logic/api/responses/graphResponses.tse2e/logic/api/responses/settingsResponses.tse2e/logic/api/responses/tokenResponse.tse2e/logic/api/responses/schemaResponses.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/userResponses.tse2e/logic/api/responses/authResponses.tse2e/logic/api/responses/graphResponses.tse2e/logic/api/responses/settingsResponses.tse2e/logic/api/responses/tokenResponse.tse2e/logic/api/responses/schemaResponses.tse2e/logic/api/apiCalls.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: Naseem77
Repo: FalkorDB/falkordb-browser PR: 1208
File: app/api/auth/[...nextauth]/options.ts:9-10
Timestamp: 2025-11-11T13:42:45.194Z
Learning: In app/api/auth/[...nextauth]/options.ts for the FalkorDB browser application: The jwt() callback stores 'password' field for NextAuth session tokens (cookie-based auth), while Personal Access Tokens (PATs) created by /api/auth/login store 'pwd' field (application-encrypted). These are two separate JWT flows: session tokens are never used with Bearer auth and PATs are never used with session auth. The jwt() callback must keep 'password' field for the session() callback to work correctly.
🧬 Code graph analysis (1)
e2e/logic/api/apiCalls.ts (2)
e2e/infra/utils.ts (1)
getAdminToken(146-184)e2e/logic/api/responses/tokenResponse.ts (2)
ListTokensResponse(9-11)TokenDetailsResponse(13-19)
⏰ 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). (3)
- GitHub Check: Setup and cache dependencies
- GitHub Check: build
- GitHub Check: Setup and cache dependencies
🔇 Additional comments (3)
e2e/logic/api/responses/authResponses.ts (1)
1-13: Auth response typings look correct
AuthCredentialsResponseandLogoutResponseare narrowly scoped, stringly-typed shapes appropriate for these e2e auth calls; no issues spotted.e2e/logic/api/responses/schemaResponses.ts (1)
1-14: Schema response interfaces look appropriateThe
AddSchemaResponse,SchemaListResponse, andRemoveSchemaResponseshapes are consistent with how schema APIs are usually exercised in tests; no issues from a typing perspective.e2e/logic/api/apiCalls.ts (1)
176-191: Graph and settings API payload changes look consistentThe following adjustments all look coherent with a body‑centric API surface:
changeGraphNamenow PATCHes the destination graph with{ sourceName }in the body instead of relying on query parameters.duplicateGraphuses a PATCH to/duplicatewith{ sourceName, ...data }, which is easier to extend with extra options.deleteGraphNodebroadens itsdatapayload toRecord<string, string | boolean>, which is suitable for boolean flags alongside string fields.modifySettingsRolenow posts{ value: roleValue }tosettingsConfig + roleName, matching the other body-based configuration endpoints.No issues here from a correctness or type-safety standpoint.
Also applies to: 206-222, 295-305, 357-365
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.