Staging #817
Staging #817
Conversation
…thub.com/FalkorDB/falkordb-browser into add-indicator-on-the-db-connecton-status
Fix #790 Fix profile button
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
…tatus. Updated button states across various components to reflect connection status, enhancing user experience by preventing actions when offline.
Fix #814 Update tooltip text for the Run button in EditorComponent for improve…
Replace userid with UUID
…on-status Fix #669 Fix Add indicator on the db connection status
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/components/ui/combobox.tsx (1)
91-91: Appropriate UI feedback for offline stateThe Select component now appropriately disables when the server is offline, and the tooltip provides clear feedback to users about the connectivity status. This is a good user experience enhancement.
However, for improved readability, consider extracting the complex disabled condition to a variable:
+ const isDisabled = disabled || options.length === 0 || (indicator === "offline" && label === type); - <Select disabled={disabled || options.length === 0 || (indicator === "offline" && label === type)} value={selectedValue} onValueChange={setSelectedValue} open={open} onOpenChange={(o) => { + <Select disabled={isDisabled} value={selectedValue} onValueChange={setSelectedValue} open={open} onOpenChange={(o) => {Also applies to: 102-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
app/components/CreateGraph.tsx(5 hunks)app/components/EditorComponent.tsx(7 hunks)app/components/ExportGraph.tsx(3 hunks)app/components/ForceGraph.tsx(4 hunks)app/components/Header.tsx(5 hunks)app/components/TableComponent.tsx(4 hunks)app/components/graph/DeleteGraph.tsx(1 hunks)app/components/provider.ts(1 hunks)app/components/ui/Button.tsx(3 hunks)app/components/ui/combobox.tsx(5 hunks)app/graph/DeleteElement.tsx(3 hunks)app/graph/Duplicate.tsx(5 hunks)app/graph/GraphDataPanel.tsx(12 hunks)app/graph/GraphView.tsx(6 hunks)app/graph/MetadataView.tsx(2 hunks)app/graph/Selector.tsx(7 hunks)app/graph/page.tsx(6 hunks)app/graph/toolbar.tsx(5 hunks)app/loginVerification.tsx(2 hunks)app/monitor/page.tsx(1 hunks)app/schema/SchemaCreateElement.tsx(4 hunks)app/schema/SchemaDataPanel.tsx(8 hunks)app/schema/SchemaView.tsx(2 hunks)app/schema/page.tsx(4 hunks)app/settings/Configurations.tsx(4 hunks)app/settings/users/Users.tsx(6 hunks)lib/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/components/EditorComponent.tsx
- app/graph/Selector.tsx
🧰 Additional context used
🧬 Code Definitions (16)
app/graph/toolbar.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/settings/users/Users.tsx (2)
app/components/provider.ts (1)
IndicatorContext(5-5)lib/utils.ts (1)
securedFetch(42-66)
app/components/graph/DeleteGraph.tsx (3)
lib/utils.ts (2)
Row(33-36)securedFetch(42-66)app/components/provider.ts (1)
IndicatorContext(5-5)app/components/DialogComponent.tsx (1)
DialogComponent(20-52)
app/graph/Duplicate.tsx (2)
app/components/provider.ts (1)
IndicatorContext(5-5)lib/utils.ts (2)
securedFetch(42-66)prepareArg(68-70)
app/graph/page.tsx (2)
app/components/provider.ts (1)
IndicatorContext(5-5)lib/utils.ts (2)
securedFetch(42-66)prepareArg(68-70)
app/components/ForceGraph.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/monitor/page.tsx (2)
app/components/provider.ts (1)
IndicatorContext(5-5)lib/utils.ts (1)
securedFetch(42-66)
app/loginVerification.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/components/Header.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/components/CreateGraph.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/graph/GraphView.tsx (2)
app/components/provider.ts (1)
IndicatorContext(5-5)lib/utils.ts (1)
cn(38-40)
app/graph/MetadataView.tsx (2)
lib/utils.ts (3)
Query(21-25)securedFetch(42-66)prepareArg(68-70)app/components/provider.ts (1)
IndicatorContext(5-5)
app/components/TableComponent.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/settings/Configurations.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/graph/DeleteElement.tsx (1)
app/components/provider.ts (1)
IndicatorContext(5-5)
app/schema/SchemaView.tsx (1)
lib/utils.ts (1)
cn(38-40)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (97)
app/components/provider.ts (1)
1-5: Well-structured context implementation for online/offline state management.The implementation of
IndicatorContextfollows React's context API best practices, providing a strongly typed context with appropriate default values. The context correctly defines the indicator state with union types "online" | "offline" and includes a setter function.app/settings/users/Users.tsx (5)
5-5: Appropriate imports added for IndicatorContext integration.The imports have been properly updated to include React's context functionality and the IndicatorContext.
Also applies to: 14-14, 38-38
42-44: Successful integration of indicator state in API fetch call.The
securedFetchfunction has been updated to include thesetIndicatorcallback, which will update the indicator state based on API response status.
61-66: Consistent implementation of setIndicator in fetch calls.The fetch call for retrieving users properly includes the
setIndicatorparameter, maintaining consistency with other API calls in the component.
87-87: Updated dependency array to include setIndicator.The useEffect dependency array has been correctly updated to include
setIndicator, ensuring the effect reruns if the indicator setter changes.
90-96: Complete integration of setIndicator in all API calls.The fetch call for adding a user also includes the
setIndicatorparameter, completing the integration of the indicator context throughout all API calls in this component.app/monitor/page.tsx (2)
4-4: Properly imported and accessed IndicatorContext.The imports and context access are correctly implemented, allowing the component to utilize the indicator state management functionality.
Also applies to: 7-7, 14-14
16-21: Enhanced fetcher with indicator state management.The fetcher function now includes the
setIndicatorcallback, enabling proper indicator state updates based on API response status. This is consistent with the implementation in other components.app/components/TableComponent.tsx (2)
13-13: Successfully imported and accessed IndicatorContext.The component properly imports and uses the IndicatorContext, integrating it with the existing state management.
Also applies to: 19-19, 38-38
261-261: Enhanced button with indicator state.The button component now receives the
indicatorprop, allowing it to reflect the current online/offline state. This provides better visual feedback to users about the system's connectivity status.app/graph/toolbar.tsx (4)
6-6: Properly updated imports to include useContextThe import statement now includes useContext which is used to access the IndicatorContext.
11-11: Added necessary IndicatorContext importThe IndicatorContext is properly imported from the provider component.
37-37: Correctly implemented context usageThe indicator state is properly extracted from the IndicatorContext using useContext.
113-113: Consistent propagation of indicator prop to Button componentsThe indicator prop is now consistently passed to all zoom-related buttons, allowing them to reflect the application's connectivity status.
Also applies to: 124-124, 135-135
app/schema/page.tsx (7)
3-3: Updated import to include useContextThe import statement now correctly includes useContext which is needed to access the IndicatorContext.
11-11: Added IndicatorContext importThe IndicatorContext is properly imported from the provider component.
24-24: Added context usage for online/offline state managementBoth indicator and setIndicator are properly retrieved from the IndicatorContext.
33-33: Enhanced securedFetch calls with indicator state managementThe securedFetch calls now include setIndicator as a parameter, which will allow updating the indicator state during fetch operations.
Also applies to: 37-37
43-43: Updated dependencies to include setIndicatorThe fetchCount useCallback dependencies now include setIndicator to ensure the callback is recreated when setIndicator changes.
46-46: Added defensive check for offline statusThe useEffect now checks if the application is offline before attempting to fetch data, which prevents unnecessary network requests.
60-60: Updated useEffect dependencies to include indicator and setIndicatorThe useEffect dependencies now include indicator and setIndicator, ensuring the effect runs when these values change.
app/graph/DeleteElement.tsx (4)
5-5: Updated import to include useContextThe import statement now correctly includes useContext which is needed to access the IndicatorContext.
9-9: Added IndicatorContext importThe IndicatorContext is properly imported from the provider component.
27-27: Added context usage for online/offline stateThe indicator state is properly extracted from the IndicatorContext using useContext.
49-49: Enhanced Button with indicator propThe Button component now receives the indicator prop, allowing it to reflect the application's connectivity status.
app/schema/SchemaDataPanel.tsx (5)
7-7: Updated import to include useContextThe import statement now correctly includes useContext which is needed to access the IndicatorContext.
21-21: Added IndicatorContext importThe IndicatorContext is properly imported from the provider component.
53-53: Added context usage for online/offline stateThe indicator state is properly extracted from the IndicatorContext.
148-148: Added defensive checks for offline status in event handlersEvent handlers now check if the application is offline before executing certain operations, preventing actions when connectivity is lost. This improves the user experience by avoiding unsuccessful operations attempts.
Also applies to: 160-160, 259-259
220-220: Enhanced Button components with indicator propButton components now receive the indicator prop, allowing them to reflect the application's connectivity status. This is consistent with the pattern used across the application.
Also applies to: 266-266, 542-542
app/loginVerification.tsx (5)
5-6: Good addition of necessary imports for the indicator functionality.The imports of
useMemoanduseStateare necessary for the implementation of the indicator context, and theIndicatorContextimport connects this component to the provider pattern.
13-13: Well-implemented state initialization for online/offline indicator.The indicator state is appropriately initialized with "online" as the default value, which is a reasonable starting assumption.
29-48: Good implementation of periodic status checking.This effect properly sets up interval-based checking of the application's connection status by making API calls to
/api/graph. The implementation includes:
- Initial check on component mount
- 30-second interval for continuous monitoring
- Proper cleanup of the interval when the component unmounts
- Status update based on API response
50-50: Effective use of useMemo for context value optimization.Using
useMemoto create the context value prevents unnecessary re-renders of child components when the component re-renders but the indicator state hasn't changed.
52-56: Clean implementation of context provider.The component now properly wraps its children with the
IndicatorContext.Provider, making the indicator state and setter available to all descendant components.app/graph/page.tsx (5)
3-3: Good integration of context usage in imports and component.The component correctly imports
useContextand uses it to access theIndicatorContext, ensuring that it can participate in the application's online/offline status tracking.Also applies to: 12-12
32-32: Proper extraction of setIndicator from context.The component correctly extracts the
setIndicatorfunction from theIndicatorContext, which will be used for updating the online/offline status during API calls.
48-51: Correctly enhanced API calls with indicator updates.The
securedFetchcalls now includesetIndicator, allowing the application to update the online/offline status based on API responses, improving error handling and user feedback.Also applies to: 52-55
60-60: Appropriate dependency array update.The
fetchCountdependency array now includessetIndicator, ensuring the callback is recreated if the setter function changes, maintaining the accuracy of the React dependency system.
80-83: Consistently enhanced API calls throughout the component.All
securedFetchcalls throughout the component have been updated to include thesetIndicatorparameter, ensuring consistent behavior across all API operations.Also applies to: 96-99
app/components/graph/DeleteGraph.tsx (4)
3-3: Good integration of context in imports.The component correctly imports
useContextfrom React andIndicatorContextfrom the provider, enabling access to the application's online/offline status.Also applies to: 7-7
24-24: Proper extraction of indicator state and setter.The component correctly extracts both the
indicatorstate andsetIndicatorfunction from the context, allowing it to both read and update the online/offline status.
32-35: Enhanced API call with indicator updates.The
securedFetchcall now includessetIndicator, allowing the application to update the online/offline status based on the API response during graph deletion operations.
69-70: Proper use of indicator in Button component.The
Buttoncomponent now receives theindicatorprop, enabling it to adjust its appearance or behavior based on the online/offline status of the application.app/schema/SchemaCreateElement.tsx (3)
6-6: Good integration of context in imports.The component correctly imports
useContextfrom React andIndicatorContextfrom the provider, enabling access to the application's online/offline status.Also applies to: 15-15
44-44: Proper extraction of indicator state.The component correctly extracts the
indicatorstate from the context, allowing the form submission button to react to the application's online/offline status.
573-573: Appropriate use of indicator in form submission button.The form submission button now receives the
indicatorprop, enabling it to adjust its appearance or behavior based on the online/offline status of the application, providing better user feedback during form submissions.app/components/ui/Button.tsx (4)
18-18: LGTM: New indicator property enhances button functionalityThe added
indicatorproperty allows the Button component to display connectivity state, which is a good enhancement.
50-51: LGTM: Updated component signature and render logicThe parameter extraction and conditional rendering logic have been properly updated to include the new indicator state.
57-57: LGTM: Appropriate button disabled state handlingDisabling the button when the indicator is "offline" prevents user actions when the server is unavailable, which is a good UX practice.
73-82: LGTM: Enhanced tooltip content handlingThe conditional tooltip content now properly displays a message when the server is offline, providing useful feedback to users about system status.
app/components/ExportGraph.tsx (4)
1-1: LGTM: Added necessary imports for context usageThe imports for
useContextandIndicatorContextare correctly added to support the connectivity state feature.Also applies to: 7-7
20-20: LGTM: Properly destructured context valuesThe context is correctly accessed to obtain both the current indicator state and the function to update it.
29-29: LGTM: Updated securedFetch with indicator state handlingThe
securedFetchfunction now receives thesetIndicatorfunction, allowing it to update the connectivity state during API operations.
69-69: LGTM: Button component receives indicator stateThe indicator prop is correctly passed to the Button component, ensuring the UI reflects the current connectivity state.
app/graph/GraphView.tsx (5)
6-6: LGTM: Added necessary imports for context usageThe imports for
useContextandIndicatorContextare correctly added to support the connectivity state feature.Also applies to: 17-17
51-51: LGTM: Properly destructured context valuesThe
setIndicatorfunction is correctly extracted from the context for use in API operations.
174-174: LGTM: Updated securedFetch with indicator state handlingThe
securedFetchfunction now receives thesetIndicatorfunction, allowing it to update the connectivity state during API operations.
339-339: LGTM: Simplified conditional logicThe condition for disabling the delete operation has been simplified while maintaining the same functionality, which improves readability.
442-442: LGTM: Improved resizable handle behaviorThe handle is now properly disabled when no element is selected, providing better user interaction feedback.
app/components/ForceGraph.tsx (4)
7-7: LGTM: Added necessary imports for context usageThe imports for
useContextandIndicatorContextare correctly added to support the connectivity state feature.Also applies to: 13-13
58-58: LGTM: Properly destructured context valuesThe indicator state is correctly extracted from the context to control node click interactions.
198-198: Fixed function argument in handleRightClickThe
onExpandfunction now correctly uses the currently clicked element rather than the previously selected one, which fixes a logical issue.
339-339: LGTM: Disabled node clicks when offlineNode click interactions are now disabled when the system is offline, which prevents users from attempting operations that would fail.
lib/utils.ts (2)
46-46: Good addition of the optional setIndicator parameterThe change adds flexibility to the
securedFetchfunction, allowing components to update online/offline status indicators when API calls fail with specific status codes.
60-62: Well implemented conditional indicator state updateThe condition properly checks for the presence of the
setIndicatorcallback before calling it, and appropriately sets the status to "offline" for authentication failures (401) or server errors (500+).app/graph/Duplicate.tsx (3)
1-1: Good integration of IndicatorContextThe component now correctly imports and utilizes the IndicatorContext, making it aware of the application's online/offline state.
Also applies to: 7-7, 21-21
31-33: Properly updated securedFetch call with indicator handlingThe securedFetch call now includes the toast and setIndicator parameters, allowing it to update the application's online/offline state when API calls fail.
70-70: Good UI feedback integrationThe Button component now receives the indicator state, enabling it to visually reflect the application's online/offline status to the user.
app/graph/GraphDataPanel.tsx (5)
8-8: Proper context integrationThe GraphDataPanel component now correctly imports and integrates the IndicatorContext to access the application's online/offline state.
Also applies to: 20-20, 52-52
88-91: Updated API call with proper error handlingThe securedFetch call now includes setIndicator to update the online/offline state when API calls fail, and the subsequent code correctly checks result.ok instead of the previous boolean success value.
158-160: Consistent update to securedFetch callThis second instance of securedFetch is properly updated to include the setIndicator parameter, ensuring consistent behavior across all API calls.
194-194: Good defensive programming for offline stateThe keyDown handlers now check if indicator === "offline" to prevent actions when the application is offline, which prevents potential errors and improves user experience.
Also applies to: 205-205
269-269: Consistent UI feedback across multiple componentsAll relevant Button components now receive the indicator prop, ensuring consistent visual feedback about the application's online/offline state throughout the interface.
Also applies to: 318-318, 375-375, 460-460
app/settings/Configurations.tsx (3)
8-8: Good context integrationThe Configurations component correctly imports and uses the IndicatorContext to access the setIndicator function for managing online/offline state during API operations.
Also applies to: 14-14, 132-132
158-159: Well structured API call with indicator handlingThe handleSetConfig function now properly passes setIndicator to securedFetch, enabling online/offline state updates when the configuration API calls fail.
195-195: Consistent API call patternThe fetchConfigs function follows the same pattern of providing setIndicator to securedFetch, ensuring consistent behavior across all API calls in the component.
app/graph/MetadataView.tsx (7)
1-1: Good enhancement of imports with util functions.Adding
prepareArgandsecuredFetchutilities improves API security by properly encoding query parameters and handling fetch responses consistently.
3-3: Good addition of useContext.This import enables the component to access the IndicatorContext, aligning with application-wide connectivity status management pattern.
5-5: Appropriate additions for better UX and connectivity.Importing
useToastandIndicatorContextenables better error handling and connectivity state management, providing useful feedback to users.Also applies to: 7-7
15-16: Good context and toast integration.Accessing the indicator state and toast functionality improves the component's ability to show connectivity status and provide user feedback.
19-21: Security improvement with securedFetch.Replacing direct fetch with
securedFetchand encoding query parameters withprepareArgsignificantly enhances API call security. Passing the toast function allows for consistent error handling across the application.
23-23: Improved error handling.Adding early return when the fetch result is not ok prevents attempting to parse JSON from failed responses, which could cause unhandled exceptions.
35-35: Proper connectivity indicator.Adding the indicator prop to the Button component appropriately disables it when offline, preventing actions that would fail.
app/schema/SchemaView.tsx (2)
111-127: Enhanced flexibility of onExpand function.The function now accepts an optional
expandparameter, allowing for more explicit control over panel expansion/collapse behavior. This improvement makes the function more versatile, supporting both toggle and directed expansion modes.
491-491: Improved resizable handle behavior.Changing the
disabledproperty to depend onselectedElementinstead of panel collapse state improves UI consistency by only enabling resizing when there's content to display.app/components/Header.tsx (4)
6-6: Good import additions for connectivity features.Adding imports for
useContext, tooltip components, andIndicatorContextenables the header to display connectivity status information to users.Also applies to: 15-15, 18-18
31-31: Appropriate context usage.Using the IndicatorContext to access the current connectivity state aligns with the application-wide pattern for managing online/offline status.
68-68: Consistent button indicator prop.Adding the indicator prop to the Settings button ensures consistency with other components and prevents actions when offline.
114-126: Excellent offline status indicator.Adding a visual indicator with explanatory tooltip when the application is offline provides crucial feedback to users about connectivity issues, improving overall user experience.
app/components/CreateGraph.tsx (4)
5-5: Good import additions for connectivity features.Adding imports for
useContextandIndicatorContextenables the component to manage and respond to connectivity state changes.Also applies to: 14-14
38-38: Proper context usage with destructuring.Destructuring both
indicatorandsetIndicatorfrom IndicatorContext enables both reading and updating the connectivity state.
68-68: Security enhancement with connectivity update.Passing
setIndicatorto thesecuredFetchcall enables updating the connectivity status based on API responses, maintaining accurate application state.
111-111: Consistent button indicator prop.Adding the indicator prop to the button aligns with the pattern used across the application and prevents actions when offline.
app/components/ui/combobox.tsx (3)
9-9: Good integration of IndicatorContext for offline status handlingThe addition of
useContextandIndicatorContextis well implemented. This allows the component to be aware of the application's connectivity status, which is essential for providing appropriate user feedback.Also applies to: 21-21, 49-49
39-39: Smart default for label propSetting
label = typeas a default value is a good improvement. This reduces redundancy for consumers while maintaining backward compatibility.
108-108: Improved input placeholderUsing the
labelvalue in the input placeholder makes the UI more context-aware and user-friendly.
Update package.json
Fix #835 Update placeholder text in Combobox component for clarity
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e/logic/POM/graphPage.ts (1)
233-235: Typo in property nameattriubutesStatsInDataPanelThere's a typo in the property name which should be "attributesStatsInDataPanel".
Note: This issue was already identified and fixed in PR #877, but that fix appears not to be included in this PR yet.
Also applies to: 468-471
e2e/logic/POM/schemaPage.ts (2)
17-19: Multiple typos in property and method namesThere are several typos in the property names and related methods:
schemahNameInput(line 17) →schemaNameInputuniqueAtiveRadioBtn(line 69) →uniqueActiveRadioBtnrequiredAtiveRadioBtn(line 73) →requiredActiveRadioBtnclickEditeValueBtnInDataPanel(line 217) →clickEditValueBtnInDataPanelNote: These issues were already identified and fixed in PR #877, but those fixes appear not to be included in this PR yet.
Also applies to: 69-75, 133-135, 185-191, 217-219
332-334:⚠️ Potential issueImplement the empty
deleteAttributemethodThis method has a parameter but no implementation. Based on similar methods in the class and the function signature, it should implement deletion of an attribute by key.
async deleteAttribute(key: string): Promise<void>{ - + // Find the attribute row by key + const attributeRows = await this.attributeRows.all(); + for (let i = 0; i < attributeRows.length; i++) { + const rowKey = await this.getKeyInDataPanelAttr(`${i + 1}`); + if (rowKey === key) { + // Use the index to locate and click the delete button for this attribute + const deleteButton = this.page.locator(`//div[contains(@class, "DataPanel")]//tr[${i + 1}]//td//button[2]`); + await interactWhenVisible(deleteButton, el => el.click(), `delete button for attribute '${key}'`); + await this.clickConfirmDeleteNodeInDataPanel(); + return; + } + } + throw new Error(`Attribute with key '${key}' not found`); }🧰 Tools
🪛 ESLint
[error] 332-332: Expected 'this' to be used by class async method 'deleteAttribute'.
(class-methods-use-this)
[error] 332-332: 'key' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 332-334: Unexpected empty async method 'deleteAttribute'.
(@typescript-eslint/no-empty-function)
🧹 Nitpick comments (1)
e2e/logic/POM/graphPage.ts (1)
586-589: MakeinferLabelFromGrapha static methodThis method doesn't use any instance properties or methods, and operates solely on its parameters. Consider making it a static method to better indicate its stateless nature.
- private inferLabelFromGraph(graph: string): string { + private static inferLabelFromGraph(graph: string): string { if (graph.toLowerCase().includes('schema')) return 'Schema'; return 'Graph'; }To use this static method, update line 592:
- const resolvedLabel = this.inferLabelFromGraph(graph); + const resolvedLabel = SchemaPage.inferLabelFromGraph(graph);🧰 Tools
🪛 ESLint
[error] 586-586: Expected 'this' to be used by class method 'inferLabelFromGraph'.
(class-methods-use-this)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/ui/combobox.tsx(8 hunks)e2e/logic/POM/graphPage.ts(8 hunks)e2e/logic/POM/schemaPage.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/ui/combobox.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
e2e/logic/POM/schemaPage.ts (2)
e2e/logic/POM/graphPage.ts (1)
GraphPage(11-858)e2e/infra/utils.ts (1)
interactWhenVisible(142-146)
e2e/logic/POM/graphPage.ts (1)
e2e/infra/utils.ts (3)
interactWhenVisible(142-146)waitForTimeOut(58-60)waitForElementToBeVisible(13-25)
🪛 ESLint
e2e/logic/POM/schemaPage.ts
[error] 332-332: Expected 'this' to be used by class async method 'deleteAttribute'.
(class-methods-use-this)
[error] 332-332: 'key' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 332-334: Unexpected empty async method 'deleteAttribute'.
(@typescript-eslint/no-empty-function)
e2e/logic/POM/graphPage.ts
[error] 586-586: Expected 'this' to be used by class method 'inferLabelFromGraph'.
(class-methods-use-this)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run settings tests
- GitHub Check: Run all tests except settings
- GitHub Check: Run all tests except settings
- GitHub Check: Run settings tests
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
e2e/logic/api/apiCalls.ts (1)
29-36:⚠️ Potential issue
loginno longer works without arequest– consider overload or default pathway.All previous callers that relied on the old signature (
login(username, password)) will now break.
Either (a) provide a convenience overload that builds anAPIRequestContextinternally, or (b) update every call‑site in the same PR.A small shim keeps backward compatibility:
-async login(request: APIRequestContext, username?: string, password?: string) +async login(requestOrUsername: APIRequestContext | string, username?: string, password?: string)…and branch on
instanceof.
Failing to do so will surface as undefined is not an object at runtime.
♻️ Duplicate comments (5)
e2e/logic/POM/dataPanelComponent.ts (1)
97-100:⚠️ Potential issueFix potential logic issue in getAttributeValueInGraphDataPanel method
The method is checking visibility of the save button but returns its text content, which seems incorrect. You should probably check visibility of the element whose text you're retrieving.
async getAttributeValueInGraphDataPanel(): Promise<string | null> { - await interactWhenVisible(this.saveAttributeButtonInGraphDataPanel, async () => {}, "save attribute button in graph data panel"); - return await this.saveAttributeButtonInGraphDataPanel.textContent(); + // Assuming we should be getting the value from a different element + const valueCell = this.page.locator('#graphDataPanel tbody tr td').nth(1); + await interactWhenVisible(valueCell, async () => {}, "attribute value cell in graph data panel"); + return await valueCell.textContent(); }e2e/tests/canvas.spec.ts (1)
26-40:⚠️ Potential issueFix function declared in a loop with referenced variables
The function declaration inside a loop references variables from the outer scope, which can lead to unexpected behavior due to closures.
for (const node of testNodes) { - test(`@admin Validate search for Person ${node} in the canvas and ensure focus`, async () => { + test(`@admin Validate search for Person ${node} in the canvas and ensure focus`, async () => { + // Capture the node value in a local constant to avoid closure issues + const testNode = node; const graph = await browser.createNewPage(GraphPage, urls.graphUrl); await browser.setPageToFullScreen(); const graphName = getRandomString('canvas'); await graph.addGraph(graphName); await graph.insertQuery(BATCH_CREATE_PERSONS); await graph.clickRunQuery(); - const searchQuery = `Person ${node}`; + const searchQuery = `Person ${testNode}`; await graph.searchForElementInCanvas(searchQuery); await graph.hoverAtCanvasCenter(); expect(await graph.getNodeCanvasToolTip()).toBe(searchQuery); await apicalls.removeGraph(graphName); }); }🧰 Tools
🪛 ESLint
[error] 27-39: Function declared in a loop contains unsafe references to variable(s) 'browser', 'browser', 'apicalls'.
(@typescript-eslint/no-loop-func)
app/graph/GraphView.tsx (2)
168-171:⚠️ Potential issueTrailing space corrupts the DELETE URL
The template literal contains a space after
${element.id}, resulting in a path like
/api/graph/foo/123␠→ encoded as%20and likely returning 404.-const result = await securedFetch(`api/graph/${prepareArg(graph.Id)}/${element.id} `, { +const result = await securedFetch( + `api/graph/${prepareArg(graph.Id)}/${prepareArg(element.id.toString())}`, + { method: "DELETE", body: JSON.stringify({ type }) }, toast, setIndicator)
180-183:⚠️ Potential issueWrong array referenced when removing empty category
You splice
graph.Categoriesusing the index fromgraph.Labels, which can be‑1
and will remove the last category by accident.-graph.Categories.splice(graph.Labels.findIndex(l => l.name === cat.name), 1) +graph.Categories.splice( + graph.Categories.findIndex(c => c.name === cat.name), + 1 +)e2e/logic/POM/graphPage.ts (1)
615-619:⚠️ Potential issueAdd a timeout or max‑attempt safeguard to prevent an infinite wait loop
waitForRunQueryToBeEnabledbusy‑waits until the Run button becomes enabled with no upper bound.
If the button never flips to the enabled state (e.g. network failure, selector mismatch), the whole E2E suite will hang indefinitely.- async waitForRunQueryToBeEnabled(): Promise<void> { - while (await this.queryRunBtn.isDisabled()) { - await this.page.waitForTimeout(1000); - } - } + async waitForRunQueryToBeEnabled(maxRetries = 30): Promise<void> { + let retries = 0; + while (await this.queryRunBtn.isDisabled()) { + if (retries++ >= maxRetries) { + throw new Error("Timed out waiting for Run Query button to become enabled"); + } + await this.page.waitForTimeout(1000); + } + }This exact issue was flagged earlier in a previous commit; the safeguard seems to have been lost during the latest refactor.
Restoring it will protect the test runner from dead‑locking.
🧹 Nitpick comments (15)
e2e/logic/POM/dataPanelComponent.ts (2)
1-8: Consider consolidating ESLint disables and adding explanatory commentsThe file starts with multiple ESLint disable comments. Consider consolidating these into a single disable comment with explanations for why each rule needs to be disabled, or better yet, address the underlying issues where possible.
-/* eslint-disable prefer-destructuring */ -/* eslint-disable @typescript-eslint/no-shadow */ -/* eslint-disable no-plusplus */ -/* eslint-disable no-await-in-loop */ -/* eslint-disable arrow-body-style */ -/* eslint-disable @typescript-eslint/no-explicit-any */ +/* eslint-disable prefer-destructuring, @typescript-eslint/no-shadow, no-plusplus, no-await-in-loop, arrow-body-style, @typescript-eslint/no-explicit-any */ +/* Disabled rules explanation: + * - prefer-destructuring: POM methods often need direct property access + * - @typescript-eslint/no-shadow: Variable shadowing needed for Playwright patterns + * - no-plusplus, no-await-in-loop: Required for sequential test operations + * - arrow-body-style, @typescript-eslint/no-explicit-any: Simplifies Playwright interaction patterns + */
106-108: Ensure consistent method naming for hover operationsThis method name uses "hoverLastAttributeRowInGraphDataPanel" while similar methods in other parts of the codebase might use different patterns. Ensure naming consistency across all hover-related methods.
-async hoverLastAttributeRowInGraphDataPanel(): Promise<void> { +async hoverLastAttributeRowInGraphDataPanel(): Promise<void> { await interactWhenVisible(this.lastAttributeRowInGraphDataPanel, el => el.hover(), "last attribute row in graph data panel"); }e2e/tests/schema.spec.ts (2)
113-113: Fix space after commentAdd a space after the
//in the comment for proper formatting according to ESLint rules.-test(`@admin Validate that deleting a relationship removes it from the canvas and updates the relationship types panel`, async () => { //#849 +test(`@admin Validate that deleting a relationship removes it from the canvas and updates the relationship types panel`, async () => { // #849🧰 Tools
🪛 ESLint
[error] 113-113: Expected exception block, space or tab after '//' in comment.
(spaced-comment)
24-336: Consider adding a test for schema deletion reloadThe current tests validate reloading schema list after adding a schema, but there's no test for validating the list updates after schema deletion.
Consider adding a test that:
- Creates a schema
- Verifies it exists in the UI
- Deletes it via API
- Verifies the UI updates correctly after reload
test(`@admin Validate that the reload schema list function works by removing a schema via API and testing the reload button`, async () => { const schemaName = getRandomString('schema'); await apicalls.addSchema(schemaName); await waitForApiSuccess(() => apicalls.getSchemas(), res => res.opts.includes(schemaName)); const schema = await browser.createNewPage(SchemaPage, urls.schemaUrl); await browser.setPageToFullScreen(); expect(await schema.verifyGraphExists(schemaName)).toBe(true); await apicalls.removeGraph(schemaName); await schema.reloadGraphList(); expect(await schema.verifyGraphExists(schemaName)).toBe(false); });🧰 Tools
🪛 ESLint
[error] 113-113: Expected exception block, space or tab after '//' in comment.
(spaced-comment)
e2e/tests/canvas.spec.ts (2)
114-114: Remove unnecessary trailing commaThere's a trailing comma in the
addGraphmethod call that should be removed.- await graph.addGraph(graphName,); + await graph.addGraph(graphName);
58-72: Test title doesn't match implementationThe test title mentions "zoom in button" but the test is actually testing zoom out functionality.
-test(`@admin Validate zoom-out functionality upon clicking the zoom in button`, async () => { +test(`@admin Validate zoom-out functionality upon clicking the zoom out button`, async () => {e2e/logic/api/apiCalls.ts (2)
21-26: Re‑order imports to satisfy ESLintimport/order.
playwrightshould be imported before the long list of local response interfaces.
Keeping imports ordered prevents noisy lint failures in CI and improves readability.-import { deleteRequest, getRequest, patchRequest, postRequest } from "../../infra/api/apiRequests"; -import urls from '../../config/urls.json' +import { APIRequestContext } from "playwright"; +import { deleteRequest, getRequest, patchRequest, postRequest } from "../../infra/api/apiRequests"; +import urls from '../../config/urls.json'🧰 Tools
🪛 ESLint
[error] 21-21:
playwrightimport should occur before import of../../infra/api/apiRequests(import/order)
117-118:await‑inside‑loop ESLint warnings – restructure for clarity.Once a bounded loop/back‑off is introduced (see previous comment) the
no‑await‑in‑looprule can be disabled for that block with// eslint-disable-next-line no-await-in-loop, or convert toPromise.allSettledif parallelism is safe.
Currently the warnings will fail the linter.Also applies to: 118-119
🧰 Tools
🪛 ESLint
[error] 117-117: Unexpected
awaitinside a loop.(no-await-in-loop)
[error] 118-118: Unexpected
awaitinside a loop.(no-await-in-loop)
e2e/logic/POM/schemaPage.ts (2)
318-330: Method parameter list is growing – switch to a descriptive object.Seven positional params (
key, type, desc, unique, required…) make call‑sites error‑prone and unreadable.-async addAttribute(key: string, type: string, desc: string, unique: boolean, required: boolean) +interface AttributeOptions { key: string; type: string; desc?: string; unique?: boolean; required?: boolean; } +async addAttribute(opts: AttributeOptions)This also offers default values and forward compatibility.
65-76: Duplicated XPath fragments – consider extracting a constant.Selectors repeat the long prefix
//div[contains(@class, "DataPanel")]dozens of times, increasing maintenance cost.const DATA_PANEL = '//div[contains(@class, "DataPanel")]'; return this.page.locator(`${DATA_PANEL}//tr//td[4]//button[not(@disabled)]`);A single typo fix then propagates everywhere.
e2e/tests/graph.spec.ts (1)
541-553: Race between UI completion and API verification.Right after
clickRunQuery()the code callsapiCall.getGraphCount().
Depending on backend processing speed, the count might not reflect the newly‑run UI mutation.Insert an explicit wait (e.g.,
await expect.poll(() => apiCall.getGraphCount(...)).toHaveProperty('result.nodes', 2)), or poll insidegetGraphCountuntil the expected result stabilises.e2e/logic/POM/graphPage.ts (4)
783-792: Leverage the existing canvas locator instead of hard‑coding the selector
waitForCanvasAnimationToEndhard‑codes.force-graph-container canvas, duplicating the selector already encapsulated bythis.canvasElement.
This creates maintenance drift if the DOM changes.- (selector) => { - const canvas = document.querySelector(selector); + () => { + const canvas = document.querySelector('.force-graph-container canvas'); return canvas?.getAttribute("data-engine-status") === "stop"; - }, - '.force-graph-container canvas', + }Or even better:
await this.page.waitForFunction( (el) => el.getAttribute("data-engine-status") === "stop", await this.canvasElement.elementHandle(), { timeout } );This keeps the selector in a single place and avoids string duplication.
694-701: Replace magic pixel offsets with a calculated valueThe hard‑coded offsets
{ graph: { x: ‑105, y: ‑380 }, schema: { x: ‑40, y: ‑370 } }are brittle:
• They depend on viewport size and UI changes.
• They obscure where the numbers come from.Consider deriving the offset from the canvas bounding box or container padding instead:
const { left, top, width, height } = await this.canvasElement.boundingBox(); const offsetX = width * 0.5; // example: center offset const offsetY = height * 0.5;Doing so will make
getNodeScreenPositions/getLinksScreenPositionsresolution resilient to layout changes.Also applies to: 719-726
632-635:inferLabelFromGraphdoes not usethis; mark itstaticESLint rightly warns that the method doesn’t access instance state.
Making it static documents that fact and removes the lint suppression.- private inferLabelFromGraph(graph: string): string { + private static inferLabelFromGraph(graph: string): string {Remember to call it via
GraphPage.inferLabelFromGraph(...)orthis.constructor['inferLabelFromGraph'](...).🧰 Tools
🪛 ESLint
[error] 632-632: Expected 'this' to be used by class method 'inferLabelFromGraph'.
(class-methods-use-this)
662-682: SerialisingDOMMatrixacross the page‑browser boundary may fail in some browsers
ctx.getTransform()returns aDOMMatrix, which isn’t guaranteed to be serialisable; Playwright serialises plain objects, but some browser versions stringify the matrix causingtransformData.transformto benull.Safest option: extract only the needed numeric fields inside the browser context:
transform: ctx ? { a: ctx.getTransform().a, d: ctx.getTransform().d, e: ctx.getTransform().e, f: ctx.getTransform().f } : null,This ensures the returned object is JSON‑serialisable and avoids the retry loop silently failing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/playwright.yml(2 hunks)app/graph/GraphView.tsx(9 hunks)e2e/logic/POM/dataPanelComponent.ts(1 hunks)e2e/logic/POM/graphPage.ts(8 hunks)e2e/logic/POM/schemaPage.ts(1 hunks)e2e/logic/api/apiCalls.ts(6 hunks)e2e/logic/api/responses/graphAttributeResponse.ts(1 hunks)e2e/logic/api/responses/graphCountResponse.ts(1 hunks)e2e/logic/api/responses/graphNodeResponse.ts(1 hunks)e2e/tests/canvas.spec.ts(1 hunks)e2e/tests/graph.spec.ts(2 hunks)e2e/tests/schema.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- e2e/logic/api/responses/graphNodeResponse.ts
- e2e/logic/api/responses/graphCountResponse.ts
- e2e/logic/api/responses/graphAttributeResponse.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/playwright.yml
🧰 Additional context used
🧬 Code Graph Analysis (5)
e2e/logic/POM/dataPanelComponent.ts (2)
e2e/logic/POM/graphPage.ts (1)
GraphPage(11-903)e2e/infra/utils.ts (1)
interactWhenVisible(142-146)
app/graph/GraphView.tsx (3)
app/components/provider.ts (1)
IndicatorContext(5-5)lib/utils.ts (3)
securedFetch(42-66)prepareArg(68-70)cn(38-40)app/api/graph/model.ts (2)
Node(31-42)Link(44-57)
e2e/logic/api/apiCalls.ts (8)
e2e/logic/api/responses/LoginResponse.ts (1)
AuthCredentialsResponse(1-9)e2e/infra/api/apiRequests.ts (3)
getRequest(50-50)postRequest(50-50)deleteRequest(50-50)e2e/logic/api/responses/graphCountResponse.ts (1)
GraphCountResponse(1-6)e2e/logic/api/responses/graphNodeResponse.ts (1)
GraphNodeResponse(1-3)e2e/logic/api/responses/graphAttributeResponse.ts (1)
GraphAttributeResponse(1-5)e2e/logic/api/responses/createUsersResponse.ts (1)
CreateUsersResponse(1-3)e2e/logic/api/responses/addSchemaResponse.ts (1)
AddSchemaResponse(1-6)e2e/logic/api/responses/getSchemaResponse.ts (1)
SchemaListResponse(1-3)
e2e/logic/POM/graphPage.ts (1)
e2e/infra/utils.ts (3)
interactWhenVisible(142-146)waitForElementToBeVisible(13-25)waitForTimeOut(58-60)
e2e/tests/graph.spec.ts (4)
e2e/infra/ui/browserWrapper.ts (1)
BrowserWrapper(4-70)e2e/logic/api/apiCalls.ts (1)
ApiCalls(27-265)e2e/infra/utils.ts (1)
getRandomString(112-116)e2e/logic/POM/graphPage.ts (1)
GraphPage(11-903)
🪛 ESLint
e2e/tests/schema.spec.ts
[error] 113-113: Expected exception block, space or tab after '//' in comment.
(spaced-comment)
e2e/logic/api/apiCalls.ts
[error] 21-21: playwright import should occur before import of ../../infra/api/apiRequests
(import/order)
[error] 117-117: Unexpected await inside a loop.
(no-await-in-loop)
[error] 118-118: Unexpected await inside a loop.
(no-await-in-loop)
e2e/logic/POM/schemaPage.ts
[error] 332-332: Expected 'this' to be used by class async method 'deleteAttribute'.
(class-methods-use-this)
[error] 332-332: 'key' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 332-334: Unexpected empty async method 'deleteAttribute'.
(@typescript-eslint/no-empty-function)
e2e/logic/POM/graphPage.ts
[error] 632-632: Expected 'this' to be used by class method 'inferLabelFromGraph'.
(class-methods-use-this)
e2e/tests/canvas.spec.ts
[error] 27-39: Function declared in a loop contains unsafe references to variable(s) 'browser', 'browser', 'apicalls'.
(@typescript-eslint/no-loop-func)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run tests part 2 (except settings)
🔇 Additional comments (5)
app/graph/GraphView.tsx (3)
321-321: Good addition of id attribute for test automationAdding the
id="canvasPanel"attribute to the animation controls container enhances test automation capabilities by providing a reliable selector.
83-90: Excellent addition of data-engine-status attributeThe addition of the
data-engine-statusattribute on the canvas element is a great improvement for test automation, making animation state more observable.
51-51: Good integration with IndicatorContextThe integration with
IndicatorContextproperly implements the application-wide pattern for managing online/offline states.e2e/logic/api/apiCalls.ts (1)
130-137: HTTP 4xx/5xx paths return plain text –result.json()will throw.Several helper methods (
getGraphCount,addGraphNodeLabel, …) blindly assume JSON.
Wrap parsing in try/catch and surface API error bodies to aid debugging:const text = await result.text(); let payload: unknown; try { payload = JSON.parse(text); } catch { throw new Error(`Unexpected response: ${text}`); } return payload as GraphCountResponse;e2e/tests/graph.spec.ts (1)
56-66:failedQueriesloop lacksawait expect.poll(); test may assert before toast is rendered.
clickRunQuery(false)bypasses internal wait logic, yet the next line immediately checksgetErrorNotification().
Intermittent failures are likely on CI.await expect.poll(async () => await graph.getErrorNotification()).toBe(true);(or use Playwright
toBeVisible()on the toast locator).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
e2e/tests/login.spec.ts (2)
48-48:⚠️ Potential issueFix Promise executor function to follow best practices.
Similar to line 31, this returns a value from the Promise executor function.
- await new Promise((res) => setTimeout(res, 500)); + await new Promise(res => { setTimeout(res, 500); });🧰 Tools
🪛 ESLint
[error] 48-48: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
31-31:⚠️ Potential issueFix Promise executor function to follow best practices.
The current implementation returns a value from the Promise executor function, which is an anti-pattern and flagged by ESLint.
- await new Promise((res) => setTimeout(res, 500)); + await new Promise(res => { setTimeout(res, 500); });🧰 Tools
🪛 ESLint
[error] 31-31: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
e2e/tests/schema.spec.ts (1)
282-282:⚠️ Potential issueAdd missing await for API call.
The call to
apicalls.addSchemais missing anawait, which could lead to race conditions. This is inconsistent with other test cases in the file.- await apicalls.addSchema(schemaName) + await apicalls.addSchema(schemaName)e2e/tests/canvas.spec.ts (1)
25-40:⚠️ Potential issueFix function declared in a loop with referenced variables
The test function inside the loop references variables from the outer scope (
browser,apicalls), which can lead to unexpected behavior due to JavaScript closures.Apply this fix:
for (const node of testNodes) { - test(`@admin Validate search for Person ${node} in the canvas and ensure focus`, async () => { + test(`@admin Validate search for Person ${node} in the canvas and ensure focus`, async ({ }) => { + // Capture the node value in a local constant to avoid closure issues + const testNode = node; const graph = await browser.createNewPage(GraphPage, urls.graphUrl); await browser.setPageToFullScreen(); const graphName = getRandomString('canvas'); await graph.addGraph(graphName); await graph.insertQuery(BATCH_CREATE_PERSONS); await graph.clickRunQuery(); - const searchQuery = `Person ${node}`; + const searchQuery = `Person ${testNode}`; await graph.searchForElementInCanvas(searchQuery); await graph.hoverAtCanvasCenter(); expect(await graph.getNodeCanvasToolTip()).toBe(searchQuery); await apicalls.removeGraph(graphName); }); }🧰 Tools
🪛 ESLint
[error] 27-39: Function declared in a loop contains unsafe references to variable(s) 'browser', 'browser', 'apicalls'.
(@typescript-eslint/no-loop-func)
e2e/tests/dataPanel.spec.ts (1)
188-188: 🛠️ Refactor suggestionAdd radix parameter to parseInt
When using parseInt, you should specify the radix to avoid unexpected behavior.
- expect(parseInt(nodes ?? "")).toBe(1); + expect(parseInt(nodes ?? "", 10)).toBe(1);🧰 Tools
🪛 ESLint
[error] 188-188: Missing radix parameter.
(radix)
🧹 Nitpick comments (9)
e2e/infra/ui/browserWrapper.ts (2)
1-3: Fix import orderThe import order should be corrected to follow the project's convention.
-import { chromium, Browser, BrowserContext, Page, firefox } from 'playwright'; -import BasePage from './basePage'; -import { test } from '@playwright/test'; +import { test } from '@playwright/test'; +import { chromium, Browser, BrowserContext, Page, firefox } from 'playwright'; +import BasePage from './basePage';🧰 Tools
🪛 ESLint
[error] 3-3:
@playwright/testimport should occur before import of./basePage(import/order)
32-38: Optimize the launchBrowser methodThe
launchBrowsermethod can be improved in two ways:
- It doesn't use
this, so it could be declared as static- The else block after return is unnecessary
- private async launchBrowser(projectName: string): Promise<Browser> { + private static async launchBrowser(projectName: string): Promise<Browser> { if (projectName.toLowerCase().includes('firefox')) { return await firefox.launch(); - } else { - return await chromium.launch(); } + return await chromium.launch(); }Additionally, update the method call on line 16:
- this.browser = await this.launchBrowser(projectName); + this.browser = await BrowserWrapper.launchBrowser(projectName);🧰 Tools
🪛 ESLint
[error] 32-32: Expected 'this' to be used by class async method 'launchBrowser'.
(class-methods-use-this)
[error] 35-37: Unnecessary 'else' after 'return'.
(no-else-return)
e2e/tests/login.spec.ts (1)
42-42: Remove unused parameter.The 'index' parameter is defined but never used in this function.
- invalidInputs.forEach(({ description, host, port, username, password }, index) => { + invalidInputs.forEach(({ description, host, port, username, password }) => {🧰 Tools
🪛 ESLint
[error] 42-42: 'index' is defined but never used.
(@typescript-eslint/no-unused-vars)
e2e/tests/schema.spec.ts (1)
1-4: Consider using TypeScript's built-in ESLint ignores.Instead of using multiple ESLint disable comments at the file level, consider consolidating them or using a more targeted approach for specific lines where needed.
-/* eslint-disable no-restricted-syntax */ -/* eslint-disable no-plusplus */ -/* eslint-disable no-await-in-loop */ +/* eslint-disable no-restricted-syntax, no-plusplus, no-await-in-loop */lib/utils.ts (1)
139-142: Use optional chaining in getMainReturnLimit.The current implementation can be improved by using optional chaining to handle the potential null match result.
export function getMainReturnLimit(query: string): number { const match = query.match(/.*\bRETURN\b.*?(?:\bLIMIT\b\s*(\d+))?(?:\s*;?\s*$|\s*$)/i); - return match && match[1] ? parseInt(match[1], 10) : -1; + return match?.[1] ? parseInt(match[1], 10) : -1; }🧰 Tools
🪛 Biome (1.9.4)
[error] 141-141: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
e2e/tests/canvas.spec.ts (1)
116-116: Remove unnecessary trailing commaThere's an unnecessary trailing comma after
graphNameparameter.- await graph.addGraph(graphName,); + await graph.addGraph(graphName);e2e/tests/queryHistory.spec.ts (1)
1-8: Fix import order and empty first lineThe imports should follow a consistent order and there should be no empty line at the beginning of the file.
- import { expect, test } from "@playwright/test"; import BrowserWrapper from "../infra/ui/browserWrapper"; import ApiCalls from "../logic/api/apiCalls"; import urls from '../config/urls.json' import QueryHistory from "../logic/POM/queryHistoryComponent"; -import { BATCH_CREATE_PERSONS } from "@/e2e/config/constants"; import { getRandomString } from "../infra/utils"; +import { BATCH_CREATE_PERSONS } from "../config/constants";🧰 Tools
🪛 ESLint
[error] 7-7:
@/e2e/config/constantsimport should occur before import of../infra/ui/browserWrapper(import/order)
e2e/logic/POM/graphPage.ts (2)
630-639: Replace custom retry loop with Playwright’s built‑in wait helperThe manual loop caps the wait at ~5 s (
5 × 1 s). In slow CI runs the “Run” button can take longer, causing flaky tests.- const maxRetries = 5; - let retries = 0; - while (await this.queryRunBtn.isDisabled()) { - if (retries++ >= maxRetries) { - throw new Error("Timed out waiting for Run Query button to be enabled."); - } - await this.page.waitForTimeout(1000); - } + await this.queryRunBtn.waitFor({ state: "enabled", timeout: 15000 });This leverages Playwright’s native waiting, removes the ESLint
no-plusplus/no-await-in-loopsuppressions, and avoids hard‑coded retry maths.
652-655: ConvertinferLabelFromGraphtostaticto silence ESLint & clarify intentThe method doesn’t use
this; declaring itstaticavoids theclass-methods-use-thiswarning and communicates that it’s a pure helper.- private inferLabelFromGraph(graph: string): string { + private static inferLabelFromGraph(graph: string): string {Call‑sites (
this.inferLabelFromGraph) will continue to work.🧰 Tools
🪛 ESLint
[error] 652-652: Expected 'this' to be used by class method 'inferLabelFromGraph'.
(class-methods-use-this)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/api/schema/[schema]/route.ts(1 hunks)app/graph/GraphView.tsx(9 hunks)app/graph/Selector.tsx(6 hunks)e2e/infra/ui/browserWrapper.ts(3 hunks)e2e/infra/utils.ts(3 hunks)e2e/logic/POM/dataPanelComponent.ts(1 hunks)e2e/logic/POM/graphPage.ts(7 hunks)e2e/logic/POM/schemaPage.ts(1 hunks)e2e/logic/api/apiCalls.ts(6 hunks)e2e/logic/api/responses/getGraphsResponse.ts(1 hunks)e2e/tests/canvas.spec.ts(1 hunks)e2e/tests/dataPanel.spec.ts(1 hunks)e2e/tests/graph.spec.ts(1 hunks)e2e/tests/login.spec.ts(1 hunks)e2e/tests/metadata.spec.ts(1 hunks)e2e/tests/navBar.spec.ts(1 hunks)e2e/tests/preferences.spec.ts(2 hunks)e2e/tests/queryHistory.spec.ts(1 hunks)e2e/tests/schema.spec.ts(1 hunks)e2e/tests/settingsConfig.spec.ts(11 hunks)e2e/tests/settingsQuery.spec.ts(1 hunks)e2e/tests/settingsUsers.spec.ts(9 hunks)e2e/tests/signOut.spec.ts(1 hunks)lib/utils.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e/logic/POM/dataPanelComponent.ts
- e2e/tests/preferences.spec.ts
- e2e/tests/settingsConfig.spec.ts
- e2e/infra/utils.ts
- e2e/tests/settingsUsers.spec.ts
- app/api/schema/[schema]/route.ts
- app/graph/GraphView.tsx
- e2e/tests/settingsQuery.spec.ts
- e2e/tests/graph.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
e2e/tests/navBar.spec.ts (1)
e2e/infra/ui/browserWrapper.ts (1)
BrowserWrapper(5-80)
e2e/tests/signOut.spec.ts (1)
e2e/infra/ui/browserWrapper.ts (1)
BrowserWrapper(5-80)
e2e/tests/metadata.spec.ts (3)
e2e/infra/ui/browserWrapper.ts (1)
BrowserWrapper(5-80)e2e/infra/utils.ts (1)
getRandomString(112-116)e2e/logic/POM/metadataView.ts (1)
MetadataView(5-37)
e2e/tests/schema.spec.ts (4)
e2e/infra/ui/browserWrapper.ts (1)
BrowserWrapper(5-80)e2e/logic/api/apiCalls.ts (1)
ApiCalls(27-287)e2e/infra/utils.ts (1)
getRandomString(112-116)e2e/logic/POM/schemaPage.ts (1)
SchemaPage(11-326)
e2e/logic/api/apiCalls.ts (8)
e2e/logic/api/responses/LoginResponse.ts (1)
AuthCredentialsResponse(1-9)e2e/infra/api/apiRequests.ts (3)
getRequest(50-50)postRequest(50-50)deleteRequest(50-50)e2e/logic/api/responses/graphCountResponse.ts (1)
GraphCountResponse(1-6)e2e/logic/api/responses/graphNodeResponse.ts (1)
GraphNodeResponse(1-3)e2e/logic/api/responses/graphAttributeResponse.ts (1)
GraphAttributeResponse(1-5)e2e/logic/api/responses/createUsersResponse.ts (1)
CreateUsersResponse(1-3)e2e/logic/api/responses/addSchemaResponse.ts (1)
AddSchemaResponse(1-6)e2e/logic/api/responses/getSchemaResponse.ts (1)
SchemaListResponse(1-3)
e2e/logic/POM/graphPage.ts (1)
e2e/infra/utils.ts (3)
interactWhenVisible(118-122)waitForElementToBeVisible(13-25)waitForTimeOut(58-60)
🪛 ESLint
e2e/infra/ui/browserWrapper.ts
[error] 3-3: @playwright/test import should occur before import of ./basePage
(import/order)
[error] 32-32: Expected 'this' to be used by class async method 'launchBrowser'.
(class-methods-use-this)
[error] 35-37: Unnecessary 'else' after 'return'.
(no-else-return)
e2e/tests/dataPanel.spec.ts
[error] 188-188: Missing radix parameter.
(radix)
e2e/tests/canvas.spec.ts
[error] 27-39: Function declared in a loop contains unsafe references to variable(s) 'browser', 'browser', 'apicalls'.
(@typescript-eslint/no-loop-func)
e2e/logic/api/apiCalls.ts
[error] 21-21: playwright import should occur before import of ../../infra/api/apiRequests
(import/order)
[error] 118-118: Unary operator '++' used.
(no-plusplus)
[error] 122-122: Unexpected await inside a loop.
(no-await-in-loop)
[error] 123-123: Unexpected await inside a loop.
(no-await-in-loop)
[error] 261-261: Unary operator '++' used.
(no-plusplus)
[error] 265-265: Unexpected await inside a loop.
(no-await-in-loop)
[error] 265-265: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 266-266: Unexpected await inside a loop.
(no-await-in-loop)
[error] 267-267: Unexpected await inside a loop.
(no-await-in-loop)
e2e/logic/POM/graphPage.ts
[error] 652-652: Expected 'this' to be used by class method 'inferLabelFromGraph'.
(class-methods-use-this)
e2e/tests/login.spec.ts
[error] 31-31: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 42-42: 'index' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 48-48: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
e2e/tests/queryHistory.spec.ts
[error] 7-7: @/e2e/config/constants import should occur before import of ../infra/ui/browserWrapper
(import/order)
🪛 Biome (1.9.4)
lib/utils.ts
[error] 141-141: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run settings tests
🔇 Additional comments (15)
e2e/logic/api/responses/getGraphsResponse.ts (1)
2-2: Property name change improves API response consistencyChanging from
resulttooptsaligns this interface with other response interfaces in the project, creating a consistent pattern for API response payloads across the codebase.e2e/tests/signOut.spec.ts (1)
11-13: Test isolation improvement with per-test browser instancesChanging from
beforeAll/afterAlltobeforeEach/afterEachensures each test runs with a fresh browser instance. This prevents state from one test affecting others, improving test reliability and isolation.Also applies to: 15-17
e2e/tests/navBar.spec.ts (1)
11-13: Improved test isolation with per-test browser instancesChanging from
beforeAll/afterAlltobeforeEach/afterEachensures each test runs with a fresh browser instance. This enhances test reliability by preventing state leakage between tests.Also applies to: 15-17
e2e/infra/ui/browserWrapper.ts (1)
15-17: Cross-browser testing support addedGood enhancement to dynamically select the browser based on the test project name, enabling cross-browser testing capabilities.
e2e/tests/metadata.spec.ts (1)
1-48: Well-structured test suite for metadata functionality.This looks like a well-implemented test suite that verifies the behavior of the metadata view tab under different conditions. The tests appropriately:
- Set up and tear down test data
- Test the disabled state when no metadata exists
- Test the enabled state when metadata is available
- Test the default selection behavior
The use of the Page Object Model pattern makes these tests clear and maintainable.
lib/utils.ts (5)
42-47: Good enhancement to securedFetch with indicator support.Adding the optional
setIndicatorparameter is a positive improvement that allows components to update their online/offline state based on API responses.
59-63: Improved error handling with indicator state.The implementation now properly sets the indicator state to "offline" when a 401 or 500+ error is encountered, which improves user feedback for connectivity issues.
72-72: Improved default query to return all related data.The updated default query now returns all matched nodes and edges using the
return *syntax, which provides more comprehensive data for visualization.
144-146: Ensure RegExp replacement pattern is correct.The current implementation refers to a capture group (
$1) and it appears this fix was already applied from a previous review. It now correctly captures and preserves the content before the LIMIT clause.
148-162: Well-implemented query limit management.The
getQueryWithLimitfunction provides a robust way to manage query limits by:
- Preserving the query if limit is 0 (no limit)
- Checking if an existing limit is already higher than the requested limit
- Properly modifying or appending the LIMIT clause as needed
This will ensure consistent behavior when applying user-configured limits to queries.
app/graph/Selector.tsx (5)
334-334: Improved MetadataView conditional renderingThe conditional rendering of MetadataView has been improved to use a ternary operator, which enhances readability and clarity about when the component should render.
This change correctly displays the MetadataView only when there are queries in the history and a selected query (counter > 0). The improved logic helps prevent potential rendering issues when history data is incomplete.
55-55: Integrate IndicatorContext to handle online/offline stateThe component now properly integrates with the IndicatorContext to manage connectivity state for API operations, allowing UI components to respond to connectivity changes.
The implementation correctly:
- Consumes the indicator state and setIndicator function from context
- Uses the indicator state to conditionally execute API calls
- Updates the indicator state during API operations via securedFetch
Also applies to: 75-86
57-73: Improved performance with memoized callbacksThe component has been optimized by using useCallback to memoize handler functions, preventing unnecessary re-renders.
The memoized handleOnChange function properly:
- Handles empty graph selection cases
- Makes schema API calls with proper error handling
- Updates the schema state and selected value
- Only re-creates when dependencies change
188-188: Add indicator prop to Button componentsThe Button components now receive the indicator prop to handle UI state based on connectivity status.
This integration allows buttons to:
- Visually indicate connectivity status
- Disable functionality when offline
- Provide appropriate feedback to users about system state
Also applies to: 342-342
88-94: Enhanced effect dependencies with online state detectionThe component now correctly fetches options on mount and when connectivity is restored.
This enhancement:
- Uses the memoized getOptions function in effect dependencies
- Adds an effect to detect when indicator changes to "online"
- Refreshes options when connectivity is restored
There was a problem hiding this comment.
Pull Request Overview
This PR enhances API endpoints for managing schema and graph data, improves error handling, readability, and introduces caching and query timeout mechanisms. Key changes include:
- New API endpoints for schema operations and attribute formatting with improved error notifications.
- Refactoring of graph API endpoints to include caching, unique id generation, and timeout handling.
- Updates to CI/CD workflows for Playwright test sharding and artifact upload.
Reviewed Changes
Copilot reviewed 92 out of 93 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/api/schema/* | Enhanced secure API calls and error reporting for schema endpoints. |
| app/api/graph/* | Updated graph-related endpoints with caching, query timeout logic, and refined query building. |
| app/api/auth/[...nextauth]/options.ts | Improved client connection caching and cache integration. |
| .github/workflows/playwright.yml | Split Playwright tests into sharded jobs and added artifact upload steps. |
Files not reviewed (1)
- .env.local.template: Language not supported
Comments suppressed due to low confidence (1)
app/api/schema/[schema]/[node]/route.ts:27
- The variable 'formateAttributes' appears to be a typo; consider renaming it to 'formattedAttributes' for clarity.
const formateAttributes = formatAttributes(attributes)
There was a problem hiding this comment.
Pull Request Overview
This PR enhances API endpoints for schema and graph management by improving secure API calls, caching, and error handling while also refining test workflows. Key changes include:
- Integration of secured API call methods and toast notifications for error handling in MetadataView.
- Introduction of a caching mechanism with a unique id generator for handling timeouts and asynchronous query execution in graph endpoints.
- Updates to various API routes for schema, graph, and node management as well as improvements to CI workflow with test sharding.
Reviewed Changes
Copilot reviewed 92 out of 93 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/api/schema/route.ts | New GET endpoint for listing schema names with basic error handling. |
| app/api/schema/[schema]/route.ts | Added GET and POST endpoints for handling schema queries and creation with basic error messaging. |
| app/api/schema/[schema]/count/route.ts | New endpoint for returning node/edge counts in a schema. |
| app/api/schema/[schema]/[node]/route.ts | Added POST and DELETE endpoints for node creation and deletion, with attribute formatting. |
| app/api/schema/[schema]/[node]/label/route.ts | Added endpoints for adding and removing node labels. |
| app/api/schema/[schema]/[node]/[key]/route.ts | Endpoints added for updating and deleting node attribute keys. |
| app/api/graph/route.ts | Refactored GET endpoint to filter out schema names from graph listings. |
| app/api/graph/model.ts | Adjustments for consistent color assignment using category indices. |
| app/api/graph/config/route.ts | Updated error response status and message for configuration retrieval. |
| app/api/graph/[graph]/route.ts | Revamped POST and GET endpoints with a new caching mechanism and id generator for queries. |
| app/api/graph/[graph]/query/route.ts | New endpoint to retrieve cached query results with a timeout mechanism. |
| app/api/graph/[graph]/count/route.ts | New endpoint to count nodes and relationships in a graph. |
| app/api/graph/[graph]/[node]/route.ts | Added DELETE endpoint for node deletion based on type (node or edge). |
| app/api/graph/[graph]/[node]/label/route.ts | Endpoints for adding and removing labels for nodes within a graph. |
| app/api/graph/[graph]/[node]/[key]/route.ts | Endpoints for updating and deleting node property keys. |
| app/api/auth/[...nextauth]/options.ts | Updates to client creation and caching logic for consistent API authentication. |
| .github/workflows/playwright.yml | Updated CI workflow to include test sharding and artifact uploads for improved reporting. |
Files not reviewed (1)
- .env.local.template: Language not supported
Comments suppressed due to low confidence (1)
app/api/schema/[schema]/[node]/route.ts:27
- The variable 'formateAttributes' appears to be a typo. Consider renaming it to 'formattedAttributes' for clarity.
const formateAttributes = formatAttributes(attributes)
| resolve(id) | ||
| }, INITIAL) | ||
|
|
||
| const timeoutNumber = timeout === "undefined" ? 0 : Number(timeout) * 1000 |
There was a problem hiding this comment.
Comparing the timeout parameter to the string 'undefined' may lead to unintended behavior. Consider checking if the timeout parameter is absent or falsy and then providing a default numeric value.
| const timeoutNumber = timeout === "undefined" ? 0 : Number(timeout) * 1000 | |
| const timeoutNumber = timeout ? Number(timeout) * 1000 : 0 |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/logic/POM/schemaPage.ts (2)
97-103: Consider using more robust selectors for UI elements.The deleteValueBtnInDataPanel and editValueBtnInDataPanel locators use positional XPath selectors (e.g.,
(//div[contains(@class, "DataPanel")]//tr//td//button)[1]). These can be brittle if the order of buttons changes in the UI. Consider using more specific attributes, text content, or data-testid attributes for more robust selection.- private get deleteValueBtnInDataPanel(): Locator { - return this.page.locator('(//div[contains(@class, "DataPanel")]//tr//td//button)[1]'); - } + private get deleteValueBtnInDataPanel(): Locator { + return this.page.locator('//div[contains(@class, "DataPanel")]//tr//td//button[contains(@aria-label, "Delete") or contains(text(), "Delete")]'); + } - private get editValueBtnInDataPanel(): Locator { - return this.page.locator('(//div[contains(@class, "DataPanel")]//tr//td//button)[2]'); - } + private get editValueBtnInDataPanel(): Locator { + return this.page.locator('//div[contains(@class, "DataPanel")]//tr//td//button[contains(@aria-label, "Edit") or contains(text(), "Edit")]'); + }
1-6: Consider reducing ESLint disable directives.The file starts with multiple ESLint disable directives at the global level. This can hide potential issues in the code. Consider addressing the underlying issues or applying the directives more selectively at the specific lines where they're needed rather than disabling rules for the entire file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/logic/POM/schemaPage.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
- GitHub Check: Run settings tests
- GitHub Check: Run tests part 2 (except settings)
- GitHub Check: Run tests part 1 (except settings)
🔇 Additional comments (3)
e2e/logic/POM/schemaPage.ts (3)
221-226: Improved implementation ofhasAttributeRowsmethod.This implementation correctly handles the case when attribute rows are not visible by first checking visibility and returning false when not visible. The previous version had issues where it would throw an error when no rows were present, as noted in past reviews.
266-273: Well-structured workflow method for adding nodes.The addNode method effectively combines multiple lower-level interactions into a cohesive workflow, making tests more readable and maintainable. This pattern is well-applied throughout the class for other workflows like addSchema, prepareRelation, etc.
311-323: Good implementation of theaddAttributemethod.This method handles all aspects of adding an attribute, including conditional logic for unique and required flags. It's a good example of encapsulating a complex UI interaction into a single method, making tests more concise and readable.
PR Type
Enhancement, Bug fix
Description
Enhanced
MetadataViewwithsecuredFetchandprepareArgfor API calls.Added toast notifications for error handling in
MetadataView.Updated
Selectorto conditionally renderMetadataViewmore robustly.Improved code readability and maintainability in
MetadataViewandSelector.Changes walkthrough 📝
MetadataView.tsx
Enhance API handling and error notifications in MetadataViewapp/graph/MetadataView.tsx
securedFetchandprepareArgfor secure API calls.Selector.tsx
Refine conditional rendering of MetadataView in Selectorapp/graph/Selector.tsx
MetadataView.Summary by CodeRabbit
New Features
INITIALfor query timeout configuration.Bug Fixes