feat: BED-5225 #1576
feat: BED-5225 #1576
Conversation
WalkthroughThis update introduces new summary routes and corresponding UI components for zone management, including a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant ZoneManagement
participant Summary
participant DataAPI
User->>Router: Navigate to /summary or /summary/tier/:tierId
Router->>ZoneManagement: Render ZoneManagement
ZoneManagement->>Summary: Render Summary component
Summary->>DataAPI: Fetch tags and selectors data
DataAPI-->>Summary: Return data/results
Summary->>User: Display InfoHeader, SummaryList, SelectedDetails, Edit button
User->>Summary: Select item in SummaryList
Summary->>ZoneManagement: Navigate to selected item's summary/details
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Holocraft
added
enhancement
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.test.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/fragments.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/utils.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx
Show resolved
Hide resolved
| .sort((a, b) => { | ||
| return b.name.localeCompare(a.name); | ||
| }) |
There was a problem hiding this comment.
Tiers should be sorted by position with tier zero (position 1) at the top instead of by name
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx
Outdated
Show resolved
Hide resolved
| <div className='flex flex-col w-full h-full space-y-4'> | ||
| <div className='flex flex-col flex-1 space-y-4'> | ||
| <ul> |
There was a problem hiding this comment.
Will this contain the list of items within the height of the page and scroll the list within the container or extend past the page height and scroll the whole page?
There was a problem hiding this comment.
Your attention to detail is unmatched!
There was a problem hiding this comment.
I feel bad for the nits especially since these things get discussed but sometimes don't end up in the ticket 😓 I really appreciate your steadfastness 🙇
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx
Show resolved
Hide resolved
|
Do we need the summary view in CE? |
|
@urangel summary view is BHE only |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.test.tsx (1)
82-82: Fix test ID inconsistency.The test ID should use
'zone-management_summary-list_loading-skeleton'to be consistent with other test IDs in the file and match the mocked component.Apply this diff to fix the inconsistency:
- expect(screen.getAllByTestId('zone-management_tiers-list_loading-skeleton')).toHaveLength(3); + expect(screen.getAllByTestId('zone-management_summary-list_loading-skeleton')).toHaveLength(3);
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx (1)
45-45: Consider making the fixed height responsive.The hardcoded height
h-[445px]might not work well across different screen sizes or when the component is used in different contexts.Consider using a more flexible approach:
- <ul className='overflow-y-auto h-[445px]'> + <ul className='overflow-y-auto max-h-[445px] h-full'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/javascript/bh-shared-ui/src/routes/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagementContext.tsx(1 hunks)
🔇 Additional comments (12)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagementContext.tsx (1)
39-49: LGTM! Clean solution for making InfoHeader required.The addition of
EmptyHeaderas a default fallback and changingInfoHeaderfrom optional to required provides a robust solution. This ensures that components consuming the context can rely onInfoHeaderbeing present without additional null checks.packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
23-25: LGTM! Routes follow established patterns.The new summary routes are well-structured and maintain consistency with the existing details routes pattern. The parallel structure enables clean navigation between summary and detail views.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (3)
29-43: LGTM! Good refactoring for reusability.Exporting
getSavePathwith explicit multiline parameter formatting improves code reusability and readability. This enables sharing the utility function with the new Summary component.
66-72: LGTM! Improved function robustness.Making all parameters optional and using optional chaining enhances the function's robustness and consistency with its usage in the Summary component.
108-155: LGTM! Wrapper change for consistency.The change from fragment to div wrapper maintains consistency with other Zone Management views and may support additional styling requirements.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.tsx (3)
42-42: LGTM! Proper lazy loading implementation.Adding lazy loading for the Summary component follows the established pattern and optimizes bundle size.
55-81: LGTM! Clean routing integration.The addition of
summaryPathsand refactoring ofchildRoutesusing spread operators creates a maintainable and scalable routing structure that properly integrates the new summary functionality.
98-101: LGTM! Smart navigation logic.The dynamic path detection for summary vs details views in the tab navigation logic elegantly handles the parallel route structure without code duplication.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.test.tsx (1)
73-134: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of all component states and behaviors:
- Loading and error states
- Data sorting and rendering
- Conditional rendering based on item type
- User interaction handling
The mock data is realistic and the test structure follows testing best practices.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx (1)
38-96: LGTM! Well-structured component with proper error handling.The component properly handles loading and error states, implements filtering and sorting logic, and provides good user interaction patterns with selection highlighting and callback handling.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx (2)
42-46: LGTM! Proper context validation.Good defensive programming practice to validate context availability and provide a clear error message when the component is used incorrectly.
48-65: LGTM! Well-structured data fetching.The React Query implementation follows best practices with proper query keys, error handling, and conditional execution for the selectors query.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx
Show resolved
Hide resolved
| </li> | ||
| {listItem.type === AssetGroupTagTypeTier ? ( | ||
| <div | ||
| key={listItem.id} |
There was a problem hiding this comment.
Avoid duplicate key usage.
The key prop is used twice in the same React.Fragment - once for the div and once for the parent li element. React requires unique keys at the same level.
- <div
- key={listItem.id}
- className='flex justify-center my-2 pl-6 last:hidden'>
+ <div className='flex justify-center my-2 pl-6 last:hidden'>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| key={listItem.id} | |
| - <div | |
| - key={listItem.id} | |
| - className='flex justify-center my-2 pl-6 last:hidden'> | |
| + <div className='flex justify-center my-2 pl-6 last:hidden'> |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryList.tsx
at line 84, the key prop is assigned to both the div and the parent li element
within the same React.Fragment, causing duplicate keys at the same level. Remove
the key from the inner div or the parent li so that only one unique key is used
per sibling element to satisfy React's requirement for unique keys.
| }, | ||
| }); | ||
|
|
||
| const showEditButton = !getEditButtonState(memberId, selectorsQuery, tagsQuery); |
There was a problem hiding this comment.
Fix button logic to eliminate double negation.
The current logic creates confusion with double negation. The button is only shown when showEditButton is true, but then immediately disabled.
- const showEditButton = !getEditButtonState(memberId, selectorsQuery, tagsQuery);
+ const isEditButtonDisabled = shouldDisableEditButton(memberId, selectorsQuery, tagsQuery);
<div className='basis-1/3'>
- {showEditButton && (
- <Button asChild variant={'secondary'} disabled={showEditButton}>
+ <Button asChild variant={'secondary'} disabled={isEditButtonDisabled}>
<Link to={getSavePath(tierId, labelId, selectorId)}>Edit</Link>
</Button>
- )}
</div>Also applies to: 74-78
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx
around lines 67 and 74-78, the button logic uses double negation which causes
confusion and incorrect button state. Refactor the logic to remove the negation
by directly assigning the correct boolean value from getEditButtonState to
showEditButton or a similarly named variable, and adjust the button's disabled
property accordingly to ensure the button is shown and enabled only when
appropriate.
| export const getEditButtonState = (memberId?: string, selectorsQuery?: UseQueryResult, tagsQuery?: UseQueryResult) => { | ||
| return ( | ||
| !!memberId || | ||
| (selectorsQuery?.isLoading && tagsQuery?.isLoading) || | ||
| (selectorsQuery?.isError && tagsQuery?.isError) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix confusing function naming and logic.
The function name getEditButtonState suggests it returns the button's state, but it actually returns whether the button should be disabled. This creates confusion when used with double negation.
-export const getEditButtonState = (memberId?: string, selectorsQuery?: UseQueryResult, tagsQuery?: UseQueryResult) => {
+export const shouldDisableEditButton = (memberId?: string, selectorsQuery?: UseQueryResult, tagsQuery?: UseQueryResult) => {
return (
!!memberId ||
(selectorsQuery?.isLoading && tagsQuery?.isLoading) ||
(selectorsQuery?.isError && tagsQuery?.isError)
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getEditButtonState = (memberId?: string, selectorsQuery?: UseQueryResult, tagsQuery?: UseQueryResult) => { | |
| return ( | |
| !!memberId || | |
| (selectorsQuery?.isLoading && tagsQuery?.isLoading) || | |
| (selectorsQuery?.isError && tagsQuery?.isError) | |
| ); | |
| }; | |
| export const shouldDisableEditButton = ( | |
| memberId?: string, | |
| selectorsQuery?: UseQueryResult, | |
| tagsQuery?: UseQueryResult | |
| ) => { | |
| return ( | |
| !!memberId || | |
| (selectorsQuery?.isLoading && tagsQuery?.isLoading) || | |
| (selectorsQuery?.isError && tagsQuery?.isError) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx
around lines 29 to 35, rename the function from getEditButtonState to a name
that clearly indicates it returns whether the edit button should be disabled,
such as isEditButtonDisabled. Also, review and adjust the logic if needed to
ensure it directly reflects the disabled state without requiring double negation
when used.
urangel
left a comment
urangel
left a comment
There was a problem hiding this comment.
Nice work! 🚀
Thanks for updating with the feedback 🙇
Description
This changeset adds the main summary view and summary tier list.
Motivation and Context
Resolves BED-5225, BED-5229, and BED-5261
Also resolves BED-5997
How Has This Been Tested?
Unit tests were added.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests