-
Notifications
You must be signed in to change notification settings - Fork 109
implement custom folders tree for expanded options page #4396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a new folder-based navigation and empty state experience to the Test Case Library page. New React components, mock data, and SCSS styles were added for folder trees, empty states, and expanded options. The main page now conditionally renders either an empty state or the expanded folder view, controlled by a toggle. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestCaseLibraryPage
participant ExpandedOptions
participant Folder
participant MainPageEmptyState
participant FolderEmptyState
User->>TestCaseLibraryPage: Load page
TestCaseLibraryPage->>User: Show Toggle (Empty/Expanded)
User->>TestCaseLibraryPage: Toggle Expanded View
alt Expanded View
TestCaseLibraryPage->>ExpandedOptions: Render
ExpandedOptions->>User: Show Folder Sidebar and Content
User->>ExpandedOptions: Select Folder
ExpandedOptions->>Folder: Set Active Folder
alt Folder is Empty
ExpandedOptions->>FolderEmptyState: Render Empty Folder UI
else Folder has Tests
ExpandedOptions->>User: Show Test Cases Table (placeholder)
end
else Empty State
TestCaseLibraryPage->>MainPageEmptyState: Render
MainPageEmptyState->>User: Show Empty State UI
end
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@OleksandrDragun should current selection in tree be reflected in address bar? |
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/emptyState/emptyState.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/emptyState/messages.js
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
app/src/pages/inside/testCaseLibraryPage/emptyState/emptyState.jsx (1)
54-59
:⚠️ Potential issueEnsure unique dataAutomationId for import button.
TheimportTestCases
button reuses thecreateTestCaseButton
ID, which may break automation scripts. It should be distinct.Apply this diff:
- { - name: formatMessage(parentMessages.importTestCases), - dataAutomationId: 'createTestCaseButton', + { + name: formatMessage(parentMessages.importTestCases), + dataAutomationId: 'importTestCasesButton', variant: 'ghost', icon: ImportIcon, isCompact: true, },
♻️ Duplicate comments (4)
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss (1)
17-17
: Address previous review feedback on CSS structure.The BEM naming and deep nesting issues raised in previous reviews are still present. Consider flattening the class structure and following proper BEM conventions for better maintainability.
Also applies to: 27-27
app/src/pages/inside/testCaseLibraryPage/expandedOptions/mockData.js (1)
1-305
: Consider adding unique identifiers for better tree navigation.The current implementation uses folder names as identifiers, but this approach has limitations for URL synchronization and state management as mentioned in previous reviews. Consider adding unique IDs to support better navigation and selection tracking.
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx (1)
39-40
: Implement path-based state management for better tree navigation.As suggested in previous reviews, storing active path instead of folder name would eliminate sync risks and enable better URL integration. The current approach using folder names could lead to issues with duplicate folder names across different tree levels.
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx (1)
28-77
: 🛠️ Refactor suggestionAddress accessibility and keyboard navigation concerns.
As raised in previous reviews, the recursive rendering approach may create challenges with keyboard interactions and accessibility. The current
aria-selected
logic usesisOpen
instead of active state, which is semantically incorrect.Consider implementing:
- Proper keyboard navigation (arrow keys, Enter, Space)
- Correct
aria-selected
based on active folder, not open state- Focus management for tree navigation
🧰 Tools
🪛 GitHub Actions: Build
[warning] 37-37: React Hook useCallback has missing dependencies: 'setActiveFolder' and 'setIsEmptyFolder'. Either include them or remove the dependency array. If 'setActiveFolder' changes too often, find the parent component that defines it and wrap that definition in useCallback (react-hooks/exhaustive-deps)
🧹 Nitpick comments (2)
app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.jsx (2)
62-62
: Add meaningful automation IDs for testing.Empty
data-automation-id
attributes reduce testability. Consider adding descriptive IDs for these interactive elements.- data-automation-id="" + data-automation-id="toggle-empty-state"- <Button variant="ghost" data-automation-id=""> + <Button variant="ghost" data-automation-id="create-test-case-btn">Also applies to: 83-83
71-71
: Optimize SVG parsing performance.Parsing SVGs in the render method creates new objects on every render. Consider moving the parsed SVGs to constants or using direct imports.
+const EXPORT_ICON = Parser(ExportIcon); +const IMPORT_ICON = Parser(ImportIcon); + export const TestCaseLibraryPage = () => { // ... rest of component - icon={Parser(ExportIcon)} + icon={EXPORT_ICON} - icon={Parser(ImportIcon)} + icon={IMPORT_ICON}Also applies to: 78-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/src/common/constants/eventsObjectTypes.js
(1 hunks)app/src/common/constants/localization/eventsLocalization.js
(2 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/emptyState.jsx
(3 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/messages.js
(0 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/index.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/index.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/mockData.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/messages.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.jsx
(2 hunks)app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.scss
(1 hunks)
💤 Files with no reviewable changes (1)
- app/src/pages/inside/testCaseLibraryPage/emptyState/messages.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/common/constants/localization/eventsLocalization.js (1)
app/src/common/constants/eventsObjectTypes.js (2)
EXPORT
(22-22)EXPORT
(22-22)
🪛 GitHub Actions: Build
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss
[error] 29-29: Unexpected hex color "#d9d9d9" (color-no-hex)
[error] 44-44: Unexpected hex color "#d9d9d9" (color-no-hex)
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
[warning] 37-37: React Hook useCallback has missing dependencies: 'setActiveFolder' and 'setIsEmptyFolder'. Either include them or remove the dependency array. If 'setActiveFolder' changes too often, find the parent component that defines it and wrap that definition in useCallback (react-hooks/exhaustive-deps)
🔇 Additional comments (11)
app/src/common/constants/eventsObjectTypes.js (1)
22-22
: Approve addition of EXPORT constant.
The newEXPORT
event object type aligns with the existing pattern and supports the export functionality.app/src/common/constants/localization/eventsLocalization.js (1)
24-24
: Approve import of EXPORT constant.
ImportingEXPORT
fromeventsObjectTypes
ensures the new action message can reference the correct event type.app/src/pages/inside/testCaseLibraryPage/expandedOptions/index.js (1)
17-17
: Approve re-export of ExpandedOptions.
This centralized export file simplifies imports and aligns with the modular folder structure.app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/index.js (1)
17-17
: Approve re-export of Folder component.
Providing a single entry point for theFolder
component improves maintainability.app/src/pages/inside/testCaseLibraryPage/emptyState/emptyState.jsx (2)
27-27
: Approve import of parent messages.
Pulling inparentMessages
centralizes localization strings and ensures consistency across the TestCaseLibrary page.
43-44
:✅ Verification successful
Verify use of parentMessages.emptyPageTitle.
Ensure thatemptyPageTitle
is defined in the parentmessages.js
and matches your UI requirements.You can grep for its definition:
🏁 Script executed:
rg 'emptyPageTitle' -n app/src/pages/inside/testCaseLibraryPage/messages.js
Length of output: 98
Confirmed definition of
emptyPageTitle
emptyPageTitle
is defined inapp/src/pages/inside/testCaseLibraryPage/messages.js
at line 24. No further action required.app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.scss (1)
54-72
: LGTM! Clean CSS structure following established patterns.The new modifier class and positioned elements follow the existing code style and use appropriate CSS variables for theming consistency.
app/src/pages/inside/testCaseLibraryPage/messages.js (1)
24-56
: LGTM! Proper internationalization structure.All new message entries follow the correct react-intl pattern with unique IDs and appropriate default messages.
app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.jsx (1)
42-45
: Address temporary code before merging.The comment indicates this toggle is temporary for BA and designer review. Ensure this temporary code is removed or properly implemented before the final merge.
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss (1)
17-97
: LGTM! Well-structured folder tree styles.The CSS effectively creates a hierarchical folder tree with proper visual connectors and interactive states. The structure supports the folder navigation functionality well.
🧰 Tools
🪛 GitHub Actions: Build
[error] 29-29: Unexpected hex color "#d9d9d9" (color-no-hex)
[error] 44-44: Unexpected hex color "#d9d9d9" (color-no-hex)
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss (1)
18-129
: Well-structured layout for expanded options UI.The flex-based layout effectively creates a sidebar-content structure with appropriate spacing and visual hierarchy for the test case library interface.
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/emptyState/emptyState.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.jsx
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/test-library #4396 +/- ##
=====================================================
Coverage 66.73% 66.73%
=====================================================
Files 85 85
Lines 995 995
Branches 142 142
=====================================================
Hits 664 664
Misses 300 300
Partials 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3c25f01
to
70e16fd
Compare
@ViktorSoroka07 I've reviewed sonar and suggest to ignoret it:
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (5)
app/.eslintrc (1)
145-146
: Redundant rule override – keep the config DRY
jsx-a11y/click-events-have-key-events
andjsx-a11y/no-static-element-interactions
are already disabled globally at lines 36 and 81. Re-declaring them here adds noise and increases the chance the two locations diverge later.- "jsx-a11y/click-events-have-key-events": "off", - "jsx-a11y/no-static-element-interactions": "off",app/src/pages/inside/testCaseLibraryPage/emptyState/messages.ts (1)
29-45
: Message-ID/label naming driftYou now have both createTestCase and addTestCase strings that map to practically the same user action. Consider consolidating to a single wording (and ID) to avoid future “which one do I use?” confusion and to keep translation files lean.
Example:- addTestCase: { … defaultMessage: 'Add Test Case' }, + createTestCase: { … defaultMessage: 'Create Test Case' },(or vice-versa, but pick one).
Also double-check the newly introduced IDs don’t collide with existing ones in other modules.
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.tsx (1)
47-56
: Type safety & empty-folder logic
- The callback parameter is
any
; typing it eliminates cast errors:interface HandleOpenArgs { event: React.MouseEvent<HTMLDivElement>; name: string; count?: number; }
setIsEmptyFolder(!count)
relies on JS truthiness.0
,undefined
, ornull
all flip totrue
. This hides intent and can mis-flag negative counts. Use an explicit check:- setIsEmptyFolder(!count); + setIsEmptyFolder(!count || count === 0);app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.tsx (2)
86-95
: Prefer a stable unique key overfolder.name
.
name
may collide or change; React reconciliation suffers.
If the mock data lacks anid
, use the array index only as a last resort or extendFOLDERS
with anid
field.-key={folder.name} +key={folder.id}
101-105
: Internationalise the fallback copy.
'Here will be the table with test cases'
is hard-coded English and will bypassreact-intl
.-{isEmptyFolder ? <FolderEmptyState /> : 'Here will be the table with test cases'} +{isEmptyFolder + ? <FolderEmptyState /> + : formatMessage(commonMessages.testCasesTablePlaceholder)}Add the message to
commonMessages
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/.eslintrc
(2 hunks)app/src/common/constants/localization.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/commonMessages.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/folder/folderEmptyState.tsx
(1 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/folder/index.ts
(1 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/mainPage/index.ts
(1 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/mainPage/mainPageEmptyState.tsx
(1 hunks)app/src/pages/inside/testCaseLibraryPage/emptyState/messages.ts
(2 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.tsx
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.tsx
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/index.ts
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/index.ts
(1 hunks)app/src/pages/inside/testCaseLibraryPage/expandedOptions/mockData.js
(1 hunks)app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.jsx
(2 hunks)app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.scss
(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- app/src/pages/inside/testCaseLibraryPage/emptyState/folder/index.ts
- app/src/common/constants/localization.js
- app/src/pages/inside/testCaseLibraryPage/expandedOptions/index.ts
- app/src/pages/inside/testCaseLibraryPage/commonMessages.js
- app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/index.ts
- app/src/pages/inside/testCaseLibraryPage/emptyState/mainPage/index.ts
- app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.scss
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.scss
- app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.scss
- app/src/pages/inside/testCaseLibraryPage/expandedOptions/mockData.js
- app/src/pages/inside/testCaseLibraryPage/testCaseLibraryPage.jsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/pages/inside/testCaseLibraryPage/emptyState/folder/folderEmptyState.tsx (1)
app/src/pages/inside/testCaseLibraryPage/emptyState/messages.ts (1)
messages
(19-65)
app/src/pages/inside/testCaseLibraryPage/emptyState/mainPage/mainPageEmptyState.tsx (3)
app/src/pages/inside/testCaseLibraryPage/emptyState/mainPage/index.ts (1)
MainPageEmptyState
(17-17)app/src/pages/inside/testCaseLibraryPage/emptyState/messages.ts (1)
messages
(19-65)app/src/pages/inside/testCaseLibraryPage/commonMessages.js (2)
commonMessages
(19-40)commonMessages
(19-40)
🔇 Additional comments (2)
app/src/pages/inside/testCaseLibraryPage/emptyState/folder/folderEmptyState.tsx (1)
31-33
: HTML parsing – validate XSS surface
Parser(formatMessage(messages.folderEmptyPageDescription))
renders raw HTML coming from translations. While these strings are currently static, they are still externalised. Make sure your l10n pipeline sanitises/escapes untrusted content or limit allowed tags.app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.tsx (1)
24-28
: Verify relative import paths foremptyState
andcommonMessages
.The component lives in
expandedOptions/
.
'../emptyState/folder'
and'../commonMessages'
resolve one level up, i.e.testCaseLibraryPage/emptyState/*
andtestCaseLibraryPage/commonMessages.*
.
If those modules actually sit next toexpandedOptions/
(e.g.expandedOptions/emptyState/…
), the import will break at runtime/compile-time.Please double-check folder layout and adjust the paths or move the files accordingly.
app/src/pages/inside/testCaseLibraryPage/emptyState/folder/folderEmptyState.tsx
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/emptyState/mainPage/mainPageEmptyState.tsx
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/folder/folder.tsx
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.tsx
Outdated
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.tsx
Show resolved
Hide resolved
app/src/pages/inside/testCaseLibraryPage/expandedOptions/expandedOptions.tsx
Show resolved
Hide resolved
|
PR Checklist
develop
for features/bugfixes, other if mentioned in the task)npm run lint
) prior to submission? Enable the git hook on commit in your IDE to run it and format the code automatically.npm run build
)?dev
npm script)manage:translations
script?Visuals
Empty state
Summary by CodeRabbit
New Features
Style
Chores