Bed 5985 #1664
Bed 5985 #1664
Conversation
WalkthroughThis update introduces a new virtualized infinite list component and supporting hook/utilities, refactors zone management details and member list components to use infinite queries, and consolidates all zone management-related React Query hooks into a single module. It removes now-redundant hooks files, updates API response types for pagination, and adapts tests and UI logic accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ZoneManagement UI
participant List as InfiniteQueryFixedList
participant Query as React Query Infinite Query
participant API as Backend API
UI->>Query: Initiate infinite query for items (selectors/members)
Query->>API: Fetch first page (skip, limit)
API-->>Query: Return items, count, nextPageParam
Query-->>UI: Provide items and loading state
UI->>List: Render InfiniteQueryFixedList with query result
List->>UI: Request renderRow for visible items
List->>Query: When near end, trigger fetchNextPage
Query->>API: Fetch next page
API-->>Query: Return more items
Query-->>List: Update items, trigger re-render
Suggested labels
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:
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/hooks/useMeasure.tsx (1)
19-41: Consider explicitly typing the return value as a tuple.The hook returns
[width, height]but the return type isn't explicitly defined as a tuple. This could lead to type inference issues where TypeScript might infer the return type asnumber[]instead of[number, number].-export function useMeasure(ref: RefObject<HTMLElement>) { +export function useMeasure(ref: RefObject<HTMLElement>): [number, number] { const [width, setWidth] = useState(0); const [height, setHeight] = useState(0);packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectorsList.tsx (1)
54-61: Consider optimizing window height calculation.The
getListHeight(window.innerHeight)is called multiple times on every render. Consider memoizing this value or using a resize observer to avoid unnecessary calculations.+import { useMemo } from 'react'; const SelectorsListWrapper = ({ children }: { children: React.ReactNode }) => { + const listHeight = useMemo(() => getListHeight(window.innerHeight), []); + return ( <div data-testid={`zone-management_details_selectors-list`}> <div data-testid={`zone-management_details_selectors-list_static-order`} className='p-0 relative w-full border-b-2 border-neutral-light-5 dark:border-neutral-dark-5'> <div className='inline-flex items-center justify-center h-10 transition-colors text-neutral-dark-5 dark:text-neutral-light-5 pl-6 font-bold text-xl'> Selectors </div> </div> <div className={cn(`overflow-y-auto border-x-2 border-neutral-light-5 dark:border-neutral-dark-5`, { - 'h-[762px]': getListHeight(window.innerHeight) === 762, - 'h-[642px]': getListHeight(window.innerHeight) === 642, - 'h-[438px]': getListHeight(window.innerHeight) === 438, + 'h-[762px]': listHeight === 762, + 'h-[642px]': listHeight === 642, + 'h-[438px]': listHeight === 438, })}> {children} </div> </div> ); };packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/InfiniteQueryFixedList.tsx (1)
23-30: Consider making the loading text customizable.The loading placeholder is hardcoded as "Loading..." which might not be appropriate for all use cases. Consider adding it as an optional prop.
export type InfiniteQueryFixedListProps<T> = { itemSize: number; queryResult: UseInfiniteQueryResult<PaginatedResult<T>>; renderRow: (item: T, index: number, style: React.CSSProperties, isScrolling?: boolean) => React.ReactNode; overscanCount?: number; thresholdCount?: number; listRef?: ForwardedRef<FixedSizeList<T[]>>; + loadingText?: string; };And update the component:
export const InfiniteQueryFixedList = <T,>({ itemSize, queryResult, renderRow, overscanCount = 5, thresholdCount = 5, listRef, + loadingText = 'Loading...', }: InfiniteQueryFixedListProps<T>) => {if (!isItemLoaded(index)) { - return <div style={style}>Loading...</div>; + return <div style={style}>{loadingText}</div>; }Also applies to: 56-58
packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts (2)
39-39: Consider making PAGE_SIZE configurableThe hardcoded PAGE_SIZE of 25 might not be optimal for all use cases. Different views might benefit from different page sizes.
Consider accepting page size as a parameter in the hooks or making it configurable through a context/environment variable:
-const PAGE_SIZE = 25; +const DEFAULT_PAGE_SIZE = 25; // Then in hooks: -export const useSelectorsInfiniteQuery = (tagId: string | number) => +export const useSelectorsInfiniteQuery = (tagId: string | number, pageSize = DEFAULT_PAGE_SIZE) => useInfiniteQuery<{ items: AssetGroupTagSelector[]; nextPageParam?: PageParam; }>({ queryKey: zoneManagementKeys.selectorsByTag(tagId), - queryFn: ({ pageParam = { skip: 0, limit: PAGE_SIZE } }) => + queryFn: ({ pageParam = { skip: 0, limit: pageSize } }) => getAssetGroupTagSelectors(tagId, pageParam.skip, pageParam.limit), getNextPageParam: (lastPage) => lastPage.nextPageParam, });
99-110: Document sorting behavior for undefined sortOrderThe function maps
undefinedsortOrder to descending order ('-name'). This implicit behavior should be documented.Add a comment to clarify the default behavior:
export const getAssetGroupTagMembers = ( tagId: number | string, skip = 0, limit = PAGE_SIZE, sortOrder: SortOrder = 'asc' ) => createPaginatedFetcher<AssetGroupTagMemberListItem>( + // Note: undefined or any non-'asc' value defaults to descending order () => apiClient.getAssetGroupTagMembers(tagId, skip, limit, sortOrder === 'asc' ? 'name' : '-name'), 'members', skip, limit );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/InfiniteQueryFixedList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useMeasure.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DetailsList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DetailsList.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/MembersList.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectorsList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/SeedSelection.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/SelectorForm.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/hooks.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/hooks.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts(1 hunks)packages/javascript/js-client-library/src/responses.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/hooks.ts
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/hooks.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: zinic
PR: SpecterOps/BloodHound#1573
File: packages/go/dawgs/drivers/pg/query/sql/schema_up.sql:514-581
Timestamp: 2025-06-12T15:42:11.258Z
Learning: Contributor "zinic" prefers not to receive automated refactor suggestions/comments on their pull requests.
packages/javascript/js-client-library/src/responses.ts (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
🧬 Code Graph Analysis (6)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.tsx (1)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts (1)
useTagsQuery(66-71)
packages/javascript/js-client-library/src/responses.ts (1)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTagSelector(100-110)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/MembersList.tsx (6)
packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(177-179)packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/InfiniteQueryFixedList.tsx (2)
InfiniteQueryFixedListProps(23-30)InfiniteQueryFixedList(32-85)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/utils.tsx (2)
SelectedHighlight(25-35)getListHeight(51-56)packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (1)
SortableHeader(60-100)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (9)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx (1)
getEditButtonState(29-35)packages/javascript/bh-shared-ui/src/utils/searchParams/useAppNavigate.ts (1)
useAppNavigate(25-40)packages/javascript/bh-shared-ui/src/views/ZoneManagement/utils.tsx (2)
TIER_ZERO_ID(20-20)getTagUrlValue(23-25)packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts (4)
useTagsQuery(66-71)useSelectorsInfiniteQuery(88-97)useSelectorMembersInfiniteQuery(144-158)useTagMembersInfiniteQuery(112-121)packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTagTypeTier(56-56)AssetGroupTagTypeLabel(57-57)AssetGroupTagTypeOwned(58-58)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DetailsList.tsx (1)
DetailsList(57-167)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectorsList.tsx (1)
SelectorsList(74-139)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/MembersList.tsx (1)
MembersList(44-101)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DetailsList.tsx (2)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(76-85)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/utils.tsx (1)
isTag(37-39)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts (6)
packages/javascript/js-client-library/src/requests.ts (3)
CreateAssetGroupTagRequest(35-41)UpdateAssetGroupTagRequest(43-43)RequestOptions(26-26)packages/javascript/js-client-library/src/types.ts (2)
AssetGroupTag(76-85)AssetGroupTagSelector(100-110)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPagi 8000 natedFetcher(37-50)PageParam(17-20)packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(177-179)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/types.ts (3)
CreateSelectorParams(26-29)PatchSelectorParams(35-37)DeleteSelectorParams(30-33)
🔇 Additional comments (16)
packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (1)
17-50: LGTM! Well-structured pagination utility.The implementation is clean and follows React Query conventions:
- Clear type definitions for pagination parameters and responses
- Correct pagination logic for determining if more pages exist
- Proper Promise-based API that integrates well with React Query infinite queries
- Generic typing allows for reusable implementation across different data types
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/SeedSelection.tsx (1)
29-29: LGTM! Import path updated to use centralized hooks.The import path change correctly reflects the consolidation of hooks from the local
./hooksfile to the centralized../../hooksmodule, improving code organization and maintainability.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/types.ts (1)
17-17: LGTM! Cleaner import path.The import path change from
'js-client-library/dist/requests'to'js-client-library'is an improvement that uses a more direct and maintainable import path from the package root.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/SelectorForm/SelectorForm.tsx (1)
28-28: LGTM! Import path updated to use centralized hooks.The import path change correctly reflects the consolidation of hooks from the local
./hooksfile to the centralized../../hooksmodule, maintaining functionality while improving code organization.packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/index.ts (1)
17-17: LGTM! Clean component entry point.The re-export pattern follows standard practices for component organization, allowing clean imports from the directory level.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.tsx (2)
44-50: Good refactoring to improve modularity.The replacement of the single
useAssetGroupTagshook with multiple specific hooks (useAssetGroupTagInfo,useCreateAssetGroupTag,useDeleteAssetGroupTag,usePatchAssetGroupTag,useTagsQuery) improves separation of concerns and makes the API more granular and reusable.
112-112: LGTM! Consistent with the new hook architecture.The change from
useAssetGroupTags()touseTagsQuery()maintains the same functionality while aligning with the refactored hook structure.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DetailsList.test.tsx (1)
73-73: Test updates correctly reflect component scope changes.The change from
'Selectors'to'Labels'in the test props aligns with the removal of selector support from theDetailsListcomponent, ensuring tests remain valid for the updated component interface.Also applies to: 81-81
packages/javascript/js-client-library/src/responses.ts (1)
189-189: Appropriate type change to support pagination.Changing from
BasicResponsetoPaginatedResponsecorrectly provides the necessary pagination metadata (count,limit,skip) required by the infinite query hooks and pagination components.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectorsList.tsx (1)
74-139: Well-structured component implementation.The component properly handles loading, error, and success states, integrates well with the InfiniteQueryFixedList, and provides appropriate visual feedback for disabled selectors.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DetailsList.tsx (2)
42-47: Clean refactoring to narrow component scope.The type restrictions properly limit this component to only handle 'Tiers' and 'Labels', making the component's purpose clearer and the API more type-safe.
148-156: Consistent refactoring removing selector-specific logic.The removal of disabled state styling and tooltip handling is appropriate since this component no longer handles selectors (which had the
disabled_atfield).packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
79-90: Excellent use of custom React Query hooks.The implementation properly uses the new infinite query hooks with appropriate filtering for tags and sort order support for members. This provides a clean separation of data fetching logic from the UI component.
packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/InfiniteQueryFixedList.tsx (1)
32-85: Well-implemented generic infinite scrolling component.The component demonstrates excellent use of React patterns:
- Proper memoization of callbacks and computed values
- Clean integration with react-window
- Good handling of container measurements
- Appropriate use of TypeScript generics
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/MembersList.tsx (1)
26-35: Excellent refactoring to use declarative data fetching.The refactoring successfully:
- Removes internal state management complexity
- Delegates infinite loading to the reusable component
- Improves separation of concerns by accepting query results as props
- Maintains all the original functionality in a cleaner implementation
Also applies to: 44-50
packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts (1)
262-270: Fix enabled condition to handle numerictagIdcorrectlyThe enabled condition only checks for empty string, but
tagIdcan be a number. A validtagIdof0would pass this check incorrectly.Update the condition to handle both types:
export const useAssetGroupTagInfo = (tagId: string) => useQuery({ queryKey: zoneManagementKeys.tagDetail(tagId), queryFn: async ({ signal }) => { const response = await apiClient.getAssetGroupTag(tagId, { signal }); return response.data.data.tag; }, - enabled: tagId !== '', + enabled: tagId !== '' && tagId !== undefined, });⛔ Skipped due to learnings
Learnt from: mistahj67 PR: SpecterOps/BloodHound#1648 File: cmd/api/src/api/v2/assetgrouptags.go:763-766 Timestamp: 2025-07-09T00:36:54.112Z Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
| import { render, screen, waitFor } from '../../../test-utils'; | ||
| import { apiClient } from '../../../utils'; | ||
| import { MembersList } from './MembersList'; | ||
| // import { apiClient } from '../../../utils'; |
There was a problem hiding this comment.
Update tests to match the refactored component implementation.
The tests have been commented out following the MembersList component refactor, leaving the component without test coverage. This creates a risk of undetected regressions.
The tests should be updated to work with the new implementation that uses UseInfiniteQueryResult and the InfiniteQueryFixedList component. Would you like me to help generate updated test cases that match the new architecture?
Also applies to: 39-39, 42-61
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/MembersList.test.tsx
around lines 20, 39, and 42 to 61, the existing tests are commented out and
outdated due to the MembersList component refactor using UseInfiniteQueryResult
and InfiniteQueryFixedList. Update the test cases to mock and assert behavior
based on the new infinite query data fetching pattern and the
InfiniteQueryFixedList rendering, ensuring coverage of loading states, data
fetching, and list rendering according to the new component structure.
| * @description This component is meant to display the lists for either Tiers, Labels, or Selectors but not the Members list since that is a paginated list that loads more data as a user scrolls. | ||
| * @param {object} props | ||
| * @param {UseQueryResult} props.listQuery The endpoint call result wrapper from react query that allows us to hook into different states that the fetched data could be in | ||
| * @param {selected} props.selected The id of the particular entity that is selected for the list. It is used for selected item rendering | ||
| * @param {(id:number) => void} props.onSelect The click handler that should be called when an item from this list is selected. This is primarily being used to set the selected id state in the parent Details component | ||
| * @returns The component that displays a list of entities for the zone management page | ||
| */ |
There was a problem hiding this comment.
Fix incorrect JSDoc comment.
The JSDoc comment states this component displays "Tiers, Labels, or Selectors" but this component is specifically designed only for Selectors. This appears to be copy-pasted from the DetailsList component.
/**
- * @description This component is meant to display the lists for either Tiers, Labels, or Selectors but not the Members list since that is a paginated list that loads more data as a user scrolls.
+ * @description This component displays a list of Selectors with infinite scrolling support.
* @param {object} props
* @param {UseQueryResult} props.listQuery The endpoint call result wrapper from react query that allows us to hook into different states that the fetched data could be in
* @param {selected} props.selected The id of the particular entity that is selected for the list. It is used for selected item rendering
* @param {(id:number) => void} props.onSelect The click handler that should be called when an item from this list is selected. This is primarily being used to set the selected id state in the parent Details component
- * @returns The component that displays a list of entities for the zone management page
+ * @returns The component that displays a list of selectors for the zone management page
*/📝 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.
| * @description This component is meant to display the lists for either Tiers, Labels, or Selectors but not the Members list since that is a paginated list that loads more data as a user scrolls. | |
| * @param {object} props | |
| * @param {UseQueryResult} props.listQuery The endpoint call result wrapper from react query that allows us to hook into different states that the fetched data could be in | |
| * @param {selected} props.selected The id of the particular entity that is selected for the list. It is used for selected item rendering | |
| * @param {(id:number) => void} props.onSelect The click handler that should be called when an item from this list is selected. This is primarily being used to set the selected id state in the parent Details component | |
| * @returns The component that displays a list of entities for the zone management page | |
| */ | |
| /** | |
| * @description This component displays a list of Selectors with infinite scrolling support. | |
| * @param {object} props | |
| * @param {UseQueryResult} props.listQuery The endpoint call result wrapper from react query that allows us to hook into different states that the fetched data could be in | |
| * @param {selected} props.selected The id of the particular entity that is selected for the list. It is used for selected item rendering | |
| * @param {(id:number) => void} props.onSelect The click handler that should be called when an item from this list is selected. This is primarily being used to set the selected id state in the parent Details component | |
| * @returns The component that displays a list of selectors for the zone management page | |
| */ |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectorsList.tsx
around lines 67 to 73, the JSDoc comment incorrectly states that the component
displays "Tiers, Labels, or Selectors," which is inaccurate as this component is
specifically for Selectors only. Update the JSDoc comment to reflect that this
component displays only the Selectors list, removing references to Tiers and
Labels to accurately describe its purpose.
| export const getEditButtonState = ( | ||
| memberId?: string, | ||
| selectorsQuery?: UseQueryResult, | ||
| tiersQuery?: UseQueryResult, | ||
| labelsQuery?: UseQueryResult | ||
| ) => { | ||
| if (selectorId !== undefined) { | ||
| const selectedSelector = selectorsQuery.data?.find((selector) => { | ||
| return selectorId === selector.id.toString(); | ||
| }); | ||
| return selectedSelector?.counts?.members || 0; | ||
| } else if (tagId !== undefined) { | ||
| const selectedTag = tagsQuery.data?.find((tag) => { | ||
| return tagId === tag.id.toString(); | ||
| }); | ||
| return selectedTag?.counts?.members || 0; | ||
| } else { | ||
| return 0; | ||
| } | ||
| }; | ||
|
|
||
| export const getEditButtonState = (memberId?: string, selectorsQuery?: UseQueryResult, tagsQuery?: UseQueryResult) => { | ||
| return ( | ||
| !!memberId || | ||
| (selectorsQuery?.isLoading && tagsQuery?.isLoading) || | ||
| (selectorsQuery?.isError && tagsQuery?.isError) | ||
| (selectorsQuery?.isLoading && tiersQuery?.isLoading && labelsQuery?.isLoading) || | ||
| (selectorsQuery?.isError && tiersQuery?.isError && labelsQuery?.isError) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Review the edit button state logic.
The current implementation only disables the edit button when ALL queries are loading or ALL queries have errors. This might not be the intended behavior - typically you'd want to disable the button if ANY query is loading or has an error.
export const getEditButtonState = (
memberId?: string,
selectorsQuery?: UseQueryResult,
tiersQuery?: UseQueryResult,
labelsQuery?: UseQueryResult
) => {
return (
!!memberId ||
- (selectorsQuery?.isLoading && tiersQuery?.isLoading && labelsQuery?.isLoading) ||
- (selectorsQuery?.isError && tiersQuery?.isError && labelsQuery?.isError)
+ (selectorsQuery?.isLoading || tiersQuery?.isLoading || labelsQuery?.isLoading) ||
+ (selectorsQuery?.isError || tiersQuery?.isError || labelsQuery?.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, | |
| tiersQuery?: UseQueryResult, | |
| labelsQuery?: UseQueryResult | |
| ) => { | |
| if (selectorId !== undefined) { | |
| const selectedSelector = selectorsQuery.data?.find((selector) => { | |
| return selectorId === selector.id.toString(); | |
| }); | |
| return selectedSelector?.counts?.members || 0; | |
| } else if (tagId !== undefined) { | |
| const selectedTag = tagsQuery.data?.find((tag) => { | |
| return tagId === tag.id.toString(); | |
| }); | |
| return selectedTag?.counts?.members || 0; | |
| } else { | |
| return 0; | |
| } | |
| }; | |
| export const getEditButtonState = (memberId?: string, selectorsQuery?: UseQueryResult, tagsQuery?: UseQueryResult) => { | |
| return ( | |
| !!memberId || | |
| (selectorsQuery?.isLoading && tagsQuery?.isLoading) || | |
| (selectorsQuery?.isError && tagsQuery?.isError) | |
| (selectorsQuery?.isLoading && tiersQuery?.isLoading && labelsQuery?.isLoading) || | |
| (selectorsQuery?.isError && tiersQuery?.isError && labelsQuery?.isError) | |
| ); | |
| }; | |
| export const getEditButtonState = ( | |
| memberId?: string, | |
| selectorsQuery?: UseQueryResult, | |
| tiersQuery?: UseQueryResult, | |
| labelsQuery?: UseQueryResult | |
| ) => { | |
| return ( | |
| !!memberId || | |
| (selectorsQuery?.isLoading || tiersQuery?.isLoading || labelsQuery?.isLoading) || | |
| (selectorsQuery?.isError || tiersQuery?.isError || labelsQuery?.isError) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx
around lines 53 to 64, the getEditButtonState function disables the edit button
only when all queries are loading or all have errors, but it should disable the
button if any query is loading or has an error. Update the logic to use OR
conditions for loading and error checks across selectorsQuery, tiersQuery, and
labelsQuery, so the button is disabled if any one of these queries is loading or
has an error.
| export const usePatchSelector = (tagId: string | number | undefined) => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation(patchSelector, { | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId!)); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add validation for undefined tagId in patch mutation hook
Same issue as useCreateSelector - the hook accepts optional tagId but uses non-null assertion.
Apply the same fix:
export const usePatchSelector = (tagId: string | number | undefined) => {
const queryClient = useQueryClient();
return useMutation(patchSelector, {
onSettled: () => {
+ if (tagId === undefined) return;
- queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId!));
+ queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId));
},
});
};📝 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 usePatchSelector = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(patchSelector, { | |
| onSettled: () => { | |
| queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId!)); | |
| }, | |
| }); | |
| }; | |
| export const usePatchSelector = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(patchSelector, { | |
| onSettled: () => { | |
| if (tagId === undefined) return; | |
| queryClient.invalidateQueries( | |
| zoneManagementKeys.selectorsByTag(tagId) | |
| ); | |
| }, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts around
lines 185 to 192, the usePatchSelector hook uses a non-null assertion on tagId
without validating if tagId is undefined, which can cause runtime errors. Fix
this by adding a validation check for tagId before calling
queryClient.invalidateQueries, ensuring the function only runs if tagId is
defined, similar to the fix applied in useCreateSelector.
| export const getAssetGroupSelectorMembers = ( | ||
| tagId: number | string, | ||
| selectorId: number | string | undefined = undefined, | ||
| skip: number = 0, | ||
| limit: number = PAGE_SIZE, | ||
| sortOrder: SortOrder = 'asc' | ||
| ) => | ||
| createPaginatedFetcher( | ||
| () => | ||
| apiClient.getAssetGroupTagSelectorMembers( | ||
| tagId, | ||
| selectorId!, | ||
| skip, | ||
| limit, | ||
| sortOrder === 'asc' ? 'name' : '-name' | ||
| ), | ||
| 'members', | ||
| skip, | ||
| limit | ||
| ); |
There was a problem hiding this comment.
Critical: Non-null assertion on potentially undefined selectorId
The function accepts selectorId as an optional parameter that defaults to undefined, but uses a non-null assertion operator (!) on line 134. This will cause a runtime error when selectorId is undefined.
Either make selectorId required or add proper handling:
export const getAssetGroupSelectorMembers = (
tagId: number | string,
- selectorId: number | string | undefined = undefined,
+ selectorId: number | string,
skip: number = 0,
limit: number = PAGE_SIZE,
sortOrder: SortOrder = 'asc'
) =>
createPaginatedFetcher(
() =>
apiClient.getAssetGroupTagSelectorMembers(
tagId,
- selectorId!,
+ selectorId,
skip,
limit,
sortOrder === 'asc' ? 'name' : '-name'
),
'members',
skip,
limit
);📝 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 getAssetGroupSelectorMembers = ( | |
| tagId: number | string, | |
| selectorId: number | string | undefined = undefined, | |
| skip: number = 0, | |
| limit: number = PAGE_SIZE, | |
| sortOrder: SortOrder = 'asc' | |
| ) => | |
| createPaginatedFetcher( | |
| () => | |
| apiClient.getAssetGroupTagSelectorMembers( | |
| tagId, | |
| selectorId!, | |
| skip, | |
| limit, | |
| sortOrder === 'asc' ? 'name' : '-name' | |
| ), | |
| 'members', | |
| skip, | |
| limit | |
| ); | |
| export const getAssetGroupSelectorMembers = ( | |
| tagId: number | string, | |
| selectorId: number | string, | |
| skip: number = 0, | |
| limit: number = PAGE_SIZE, | |
| sortOrder: SortOrder = 'asc' | |
| ) => | |
| createPaginatedFetcher( | |
| () => | |
| apiClient.getAssetGroupTagSelectorMembers( | |
| tagId, | |
| selectorId, | |
| skip, | |
| limit, | |
| sortOrder === 'asc' ? 'name' : '-name' | |
| ), | |
| 'members', | |
| skip, | |
| limit | |
| ); |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts around
lines 123 to 142, the function getAssetGroupSelectorMembers uses a non-null
assertion on the optional parameter selectorId, which can be undefined and cause
runtime errors. To fix this, either make selectorId a required parameter by
removing the default undefined and non-null assertion, or add conditional logic
to handle the case when selectorId is undefined before passing it to
apiClient.getAssetGroupTagSelectorMembers.
| export const useCreateSelector = (tagId: string | number | undefined) => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation(createSelector, { | ||
| onSettled: () => { | ||
| queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId!)); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add validation for undefined tagId in mutation hook
The hook accepts optional tagId but uses non-null assertion when invalidating queries. This could cause runtime errors.
Add validation:
export const useCreateSelector = (tagId: string | number | undefined) => {
const queryClient = useQueryClient();
return useMutation(createSelector, {
onSettled: () => {
+ if (tagId === undefined) return;
- queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId!));
+ queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId));
},
});
};📝 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 useCreateSelector = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(createSelector, { | |
| onSettled: () => { | |
| queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId!)); | |
| }, | |
| }); | |
| }; | |
| export const useCreateSelector = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(createSelector, { | |
| onSettled: () => { | |
| if (tagId === undefined) return; | |
| queryClient.invalidateQueries(zoneManagementKeys.selectorsByTag(tagId)); | |
| }, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts around
lines 168 to 175, the useCreateSelector hook uses a non-null assertion on tagId
when invalidating queries, which can cause runtime errors if tagId is undefined.
Add a validation check to ensure tagId is defined before calling
queryClient.invalidateQueries, and handle the case when tagId is undefined
appropriately to avoid runtime exceptions.
| export const useSelectorMembersInfiniteQuery = ( | ||
| tagId: number | string, | ||
| selectorId: number | string | undefined, | ||
| sortOrder: SortOrder | ||
| ) => | ||
| useInfiniteQuery<{ | ||
| items: AssetGroupTagMemberListItem[]; | ||
| nextPageParam?: PageParam; | ||
| }>({ | ||
| queryKey: zoneManagementKeys.membersByTagAndSelector(tagId, selectorId), | ||
| queryFn: ({ pageParam = { skip: 0, limit: PAGE_SIZE } }) => | ||
| getAssetGroupSelectorMembers(tagId, selectorId, pageParam.skip, pageParam.limit, sortOrder), | ||
| getNextPageParam: (lastPage) => lastPage.nextPageParam, | ||
| enabled: selectorId !== undefined, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for selector members query
The hook correctly uses enabled: selectorId !== undefined to prevent execution, but still passes potentially undefined selectorId to getAssetGroupSelectorMembers. This creates a fragile dependency on the execution order.
Consider this safer approach:
export const useSelectorMembersInfiniteQuery = (
tagId: number | string
10000
,
selectorId: number | string | undefined,
sortOrder: SortOrder
) =>
useInfiniteQuery<{
items: AssetGroupTagMemberListItem[];
nextPageParam?: PageParam;
}>({
queryKey: zoneManagementKeys.membersByTagAndSelector(tagId, selectorId),
- queryFn: ({ pageParam = { skip: 0, limit: PAGE_SIZE } }) =>
- getAssetGroupSelectorMembers(tagId, selectorId, pageParam.skip, pageParam.limit, sortOrder),
+ queryFn: ({ pageParam = { skip: 0, limit: PAGE_SIZE } }) => {
+ if (selectorId === undefined) {
+ throw new Error('selectorId is required');
+ }
+ return getAssetGroupSelectorMembers(tagId, selectorId, pageParam.skip, pageParam.limit, sortOrder);
+ },
getNextPageParam: (lastPage) => lastPage.nextPageParam,
enabled: selectorId !== undefined,
});📝 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 useSelectorMembersInfiniteQuery = ( | |
| tagId: number | string, | |
| selectorId: number | string | undefined, | |
| sortOrder: SortOrder | |
| ) => | |
| useInfiniteQuery<{ | |
| items: AssetGroupTagMemberListItem[]; | |
| nextPageParam?: PageParam; | |
| }>({ | |
| queryKey: zoneManagementKeys.membersByTagAndSelector(tagId, selectorId), | |
| queryFn: ({ pageParam = { skip: 0, limit: PAGE_SIZE } }) => | |
| getAssetGroupSelectorMembers(tagId, selectorId, pageParam.skip, pageParam.limit, sortOrder), | |
| getNextPageParam: (lastPage) => lastPage.nextPageParam, | |
| enabled: selectorId !== undefined, | |
| }); | |
| export const useSelectorMembersInfiniteQuery = ( | |
| tagId: number | string, | |
| selectorId: number | string | undefined, | |
| sortOrder: SortOrder | |
| ) => | |
| useInfiniteQuery<{ | |
| items: AssetGroupTagMemberListItem[]; | |
| nextPageParam?: PageParam; | |
| }>({ | |
| queryKey: zoneManagementKeys.membersByTagAndSelector(tagId, selectorId), | |
| queryFn: ({ pageParam = { skip: 0, limit: PAGE_SIZE } }) => { | |
| if (selectorId === undefined) { | |
| throw new Error('selectorId is required'); | |
| } | |
| return getAssetGroupSelectorMembers(tagId, selectorId, pageParam.skip, pageParam.limit, sortOrder); | |
| }, | |
| getNextPageParam: (lastPage) => lastPage.nextPageParam, | |
| enabled: selectorId !== undefined, | |
| }); |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/ZoneManagement/hooks.ts around
lines 144 to 158, the hook passes selectorId directly to
getAssetGroupSelectorMembers even when it might be undefined, risking runtime
errors. To fix this, add a runtime check to ensure selectorId is defined before
calling getAssetGroupSelectorMembers, or adjust the query function to only
execute when selectorId is non-undefined, preventing unsafe calls and improving
type safety.
|
This is about as far as I am gonna get on this pagination story before heading out on PTO for a few days
I'd be happy to pick this back up when I return or let someone else chew on it if it needs to move forward - I will defer to @mistahj67 or @urangel to decide |
|
One small thing I wanted to add back was using the DoodleUI Skeleton component in the new InfiniteQueryFixedList instead of the "Loading..." string |
| export const zoneManagementKeys = { | ||
| all: ['zone-management'] as const, | ||
| tags: () => [...zoneManagementKeys.all, 'tags'] as const, | ||
| tagDetail: (tagId: string | number) => [...zoneManagementKeys.tags(), 'tagId', tagId] as const, | ||
| selectors: () => [...zoneManagementKeys.all, 'selectors'] as const, | ||
| selectorsByTag: (tagId: string | number) => [...zoneManagementKeys.selectors(), 'tag', tagId] as const, | ||
| selectorDetail: (tagId: string | number, selectorId: string | number) => | ||
| [...zoneManagementKeys.selectorsByTag(tagId), 'selectorId', selectorId] as const, | ||
| members: () => [...zoneManagementKeys.all, 'members'] as const, | ||
| membersByTag: (tagId: string | number) => [...zoneManagementKeys.members(), 'tag', tagId] as const, | ||
| membersByTagAndSelector: (tagId: string | number, selectorId: string | number | undefined) => | ||
| [...zoneManagementKeys.membersByTag(tagId), 'selector', selectorId] as const, | ||
| }; |
Description
Describe your changes in detail
Motivation and Context
Resolves <TICKET_OR_ISSUE_NUMBER>
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores