feat: Details Rules display redesigned BED-6841 #2187
Conversation
urangel
added
the
user interface
WalkthroughThis PR refactors asset group tag management by introducing a new query/mutation hook architecture (useTagsQuery, useTagGlyphs, useTagMutations, useRulesInfiniteQuery) to replace the monolithic useAssetGroupTags pattern. It reorganizes hook exports within a dedicated sub-index, updates the client API to use typed key constants (RulesKey, ObjectsKey, CustomRulesKey, DefaultRulesKey, DisabledRulesKey), and restructures PrivilegeZones UI components to support rule-based views with a new RulesAccordion component. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsx (1)
82-138: Inconsistent data access: mix of RulesKey and hardcoded 'selectors'.Line 86 correctly uses
memberInfoQuery.data[RulesKey]?.lengthfor the count, but line 89 still accessesmemberInfoQuery.data.selectors?.map(...)directly. This inconsistency could cause runtime errors if:
- The data shape changes and only uses RulesKey
- RulesKey has a different value than 'selectors'
Both accesses should use the same key for consistency and correctness.
🔎 Proposed fix
<EntityInfoCollapsibleSection label='Rules' count={memberInfoQuery.data[RulesKey]?.length} isExpanded={!!isExpandedPanelSection} onChange={handleOnChange}> - {memberInfoQuery.data.selectors?.map((rule, index) => { + {memberInfoQuery.data[RulesKey]?.map((rule, index) => { return (packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx (1)
64-75: Duplicate MSW handler should be removed.This
/api/v2/featureshandler is identical to the one at lines 30-41. MSW will use the first matching handler, making this one unreachable and redundant.🔎 Proposed fix
rest.get('/api/v2/asset-group-tags/:tagId/selectors/:selectorId/members', async (_, res, ctx) => { return res(ctx.status(200)); }), - rest.get('/api/v2/features', async (_req, res, ctx) => { - return res( - ctx.json({ - data: [ - { - key: 'tier_management_engine', - enabled: true, - }, - ], - }) - ); - }), ];
🧹 Nitpick comments (9)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsx (2)
199-201: Fragile string manipulation for label extraction.The expression
filterKey.split('_')[0]assumes the key format will always beprefix_rules. If the keys fromjs-client-librarychange, this will silently produce incorrect labels.Consider using the
filterLabelsmap which already has human-readable labels, or extracting the count label more explicitly:🔎 Proposed fix
- <span className='pr-12 max-xl:pr-4 max-lg:pr-12 flex-none'> - <span className='font-bold'>Total {filterKey.split('_')[0]}: </span> - {count.toLocaleString()} - </span> + <span className='pr-12 max-xl:pr-4 max-lg:pr-12 flex-none'> + <span className='font-bold'>Total: </span> + {count.toLocaleString()} + </span>Or use a dedicated count labels map similar to
filterLabels.
143-163: Consider memoizing the Row renderer.The
Rowfunction is recreated on every render. For virtualized lists with many items, this can cause unnecessary re-renders. Since it depends on closure variables (isSelected,handleClick,ruleId), consider usinguseCallbackwith appropriate dependencies.🔎 Suggested approach
const Row = useCallback<InfiniteQueryFixedListProps<AssetGroupTagSelector>['renderRow']>( (item, index, style) => { return ( <div key={item.id} role='listitem' className={cn('border-y border-neutral-3 relative', { 'bg-neutral-4': ruleId === item.id.toString(), })} style={style}> <SelectedHighlight itemId={item.id} type='rule' /> <Button variant='text' className='w-full block text-left' title={`Name: ${item.name}`} onClick={() => navigate(ruleDetailsLink(tagId, item.id))}> <span className='pl-6 text-base text-contrast ml-2 truncate'>{item.name}</span> </Button> {ruleId === item.id.toString() && <SelectedCaretRight />} </div> ); }, [ruleId, navigate, ruleDetailsLink, tagId] );packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
36-41: Redundant return statement and typo in parameter name.The function covers all branches, making line 40 unreachable. Also,
windoHeightappears to be a typo forwindowHeight.🔎 Proposed fix
-export const getListHeight = (windoHeight: number) => { - if (windoHeight > 1080) return 760; - if (1080 >= windoHeight && windoHeight > 900) return 640; - if (900 >= windoHeight) return 436; - return 436; -}; +export const getListHeight = (windowHeight: number): number => { + if (windowHeight > 1080) return 760; + if (windowHeight > 900) return 640; + return 436; +};packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagGlyphs.ts (1)
103-115: Consider usinguseMemoinstead ofuseState+useEffect.The current pattern derives state from query data using
useEffect, which causes an extra render cycle. UsinguseMemowould be more efficient and idiomatic for derived state.🔎 Proposed refactor
export const useTagGlyphs = (glyphUtils: GlyphUtils, darkMode?: boolean): TagGlyphs => { - const [glyphMap, setGlyphMap] = useState<Record<string, string>>({}); const tagsQuery = useTagsQuery(); - useEffect(() => { - if (!tagsQuery.data) return; - - const newMap = createGlyphMapFromTags(tagsQuery.data, glyphUtils, darkMode); - setGlyphMap(newMap); - }, [tagsQuery.data, glyphUtils, darkMode]); + const glyphMap = useMemo(() => { + if (!tagsQuery.data) return {}; + return createGlyphMapFromTags(tagsQuery.data, glyphUtils, darkMode); + }, [tagsQuery.data, glyphUtils, darkMode]); return glyphMap; };packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
105-116: Address type safety and consider feature flag error handling.Line 111: The type cast
as unknown as string[]suggests a type incompatibility between the query key structure and whatuseQueryexpects. This workaround masks potential type issues.Line 108: If the feature flag fetch fails (
isErroris true), theenabledflag becomesfalse, silently disabling tag queries. Consider whether this is the desired behavior or if a fallback/error state should be exposed.Potential improvements
- Type safety: Investigate whether the query key type can be corrected without casting:
queryKey: privilegeZonesKeys.tags(),
- Feature flag handling: Consider exposing error state or using a default:
const enabled = !isLoading && (isError ? false : data?.enabled ?? false);
182-190: Reconsider defaultcountsbehavior.Line 189: The logic
counts: params.counts ? params.counts : truedefaultscountstotruewhenparams.countsisundefinedorfalse. This means:
- Passing
counts: falseexplicitly will still result incounts: true- Counts will always be fetched unless explicitly opted out
This behavior seems unintended. Consider using nullish coalescing or documenting this as intentional:
Proposed fix
- counts: params.counts ? params.counts : true, + counts: params.counts ?? true,Or if false should be respected:
- counts: params.counts ? params.counts : true, + counts: params.counts !== undefined ? params.counts : true,packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
49-51: Track the TODO for DetailsV2 swap.The commented import and TODO indicate that DetailsV2 will replace the current Details component once the redesign work is complete. Consider creating a tracking issue if one doesn't already exist to ensure this swap happens.
Do you want me to open a tracking issue for the DetailsV2 component swap mentioned in this TODO?
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
70-78: Consider loading and error states for rule query.The current conditional
ruleQuery.data && ruleQuery.data.disabled_at !== nullshows the placeholder text while the query is loading or if no rule is selected. This may be intentional, but consider whether a loading indicator is needed when fetching rule data.Also, the commented-out
ObjectsAccordionon line 77 should be removed if it's not immediately needed.🔎 Suggested cleanup
{ruleQuery.data && ruleQuery.data.disabled_at !== null ? ( <div className='flex justify-center items-center gap-2'> <FontAwesomeIcon icon={faWarning} className='text-orange-500' /> <span>Enable this Rule to see Objects</span> </div> ) : ( 'Objects Accordion' - // <ObjectsAccordion kindCounts={kindCounts} totalCount={777} /> )}packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.test.tsx (1)
22-30: Mock returns number for tagId, but real hook returns string.The mock for
usePZPathParamsreturnstagId: 1as a number, but the actual hook derivestagIdfromuseParams()which returns strings. Consider using a string value for consistency:🔎 Suggested fix
vi.mock('../../../hooks/usePZParams/usePZPathParams', () => ({ usePZPathParams: () => ({ - tagId: 1, + tagId: '1', ruleId: undefined, isZonePage: true, tagDetailsLink: (id: number) => `/tags/${id}`, ruleDetailsLink: (tagId: number, ruleId: number) => `/tags/${tagId}/rules/${ruleId}`, }), }));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.test.tsxpackages/javascript/bh-shared-ui/src/hooks/index.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/index.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagGlyphs.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.test.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.tspackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/types.tspackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/js-client-library/src/client.tspackages/javascript/js-client-library/src/types.ts
💤 Files with no reviewable changes (4)
- packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
- packages/javascript/bh-shared-ui/src/hooks/index.ts
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 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.
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.test.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/index.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.test.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/index.tspackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsxpackages/javascript/js-client-library/src/types.tspackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagGlyphs.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.tspackages/javascript/js-client-library/src/client.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.test.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.test.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/index.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsxpackages/javascript/js-client-library/src/types.tspackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagGlyphs.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-08-27T19:58:12.996Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx:89-97
Timestamp: 2025-08-27T19:58:12.996Z
Learning: In BloodHound's SaveQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx), the useEffect that sets isNew based on selectedQuery existence (rather than id presence) is intentional behavior - the logic for determining whether a query is new vs existing should not be changed to check for id presence.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsx
📚 Learning: 2025-08-27T19:22:50.905Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx:71-74
Timestamp: 2025-08-27T19:22:50.905Z
Learning: In BloodHound's CommonSearches component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx), the useEffect that resets filteredList to queryList when userQueries.data changes is intentional behavior - filters should be cleared when the underlying saved queries data updates.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsx
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:35:45.118Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:86-90
Timestamp: 2025-11-06T21:35:45.118Z
Learning: In CypherSearch.tsx (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the useLayoutEffect that directly sets aria-label on cypherEditorRef.current.cypherEditor.codemirror.contentDOM is necessary because the aria-label prop passed to the CypherEditor component (neo4j-cypher/react-codemirror) does not properly reach the correct DOM element for accessibility. The direct DOM manipulation is required to satisfy Axe DevTools requirements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsx
📚 Learning: 2025-08-18T22:09:32.551Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1793
File: packages/javascript/bh-shared-ui/src/hooks/useFinishedJobs/useFinishedJobs.test.ts:45-53
Timestamp: 2025-08-18T22:09:32.551Z
Learning: When suggesting changes to hook dependencies (like adding permissionsLoaded/isSuccess checks), always mention that existing tests mocking those hooks will likely need updates to include the new dependencies, as omitting them can cause unexpected test behavior or failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx
📚 Learning: 2025-11-07T22:29:44.150Z
Learnt from: sirisjo
Repo: SpecterOps/BloodHound PR: 2035
File: cmd/ui/src/views/Explore/ContextMenu/AssetGroupMenuItem.test.tsx:75-77
Timestamp: 2025-11-07T22:29:44.150Z
Learning: In the BloodHound codebase (SpecterOps/BloodHound), when renaming UI terminology from "Selectors" to "Rules", the backend API endpoints (e.g., `/api/v2/asset-groups/:assetGroupId/selectors`), API client method names (e.g., `updateAssetGroupSelector`), and payload field names (e.g., `selector_name`) should remain unchanged. Only user-facing labels and text in the UI should be updated to use "Rules" terminology.
Applied to files:
packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-08-11T18:25:23.685Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1737
File: packages/javascript/bh-shared-ui/src/components/StatusIndicator.tsx:23-35
Timestamp: 2025-08-11T18:25:23.685Z
Learning: In the BloodHound bh-shared-ui package, React is automatically injected into scope by the framework, so explicit React imports are not required when using React.FC or other React types in component files.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/js-client-library/src/client.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx
🧬 Code graph analysis (14)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.test.tsx (2)
packages/javascript/js-client-library/src/types.ts (4)
RulesKey(136-136)CustomRulesKey(137-137)DefaultRulesKey(138-138)DisabledRulesKey(139-139)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsx (1)
RulesAccordion(55-110)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (6)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useRuleInfo(319-327)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)packages/javascript/bh-shared-ui/src/utils/searchParams/useAppNavigate.ts (1)
useAppNavigate(27-42)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (1)
PrivilegeZonesContext(59-59)packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(152-163)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsx (1)
RulesAccordion(55-110)
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useTagsQuery(105-116)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useTagsQuery(105-116)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx (1)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(152-163)
packages/javascript/js-client-library/src/types.ts (1)
cmd/api/src/model/assetgrouptags.go (3)
AssetGroupTagTypeLabel(50-50)AssetGroupTagTypeOwned(51-51)AssetGroupTagCounts(93-99)
packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts (1)
packages/javascript/js-client-library/src/types.ts (3)
CustomRulesKey(137-137)DefaultRulesKey(138-138)DisabledRulesKey(139-139)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/EntityRulesInformation.tsx (1)
packages/javascript/js-client-library/src/types.ts (1)
RulesKey(136-136)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTag(152-163)AssetGroupTagSelector(203-213)SeedTypes(178-178)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
useRulesInfiniteQuery(166-196)useRuleMembersInfiniteQuery(276-306)useTagMembersInfiniteQuery(221-249)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (3)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(152-163)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZQueryParams.tsx (1)
usePZQueryParams(34-64)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useTagsQuery(105-116)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.ts (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useTagsQuery(105-116)
packages/javascript/js-client-library/src/client.ts (2)
packages/javascript/js-client-library/src/requests.ts (1)
RequestOptions(30-30)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMembersResponse(205-205)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (5)
packages/javascript/bh-shared-ui/src/types.ts (2)
SortOrder(29-29)SortOrderAscending(27-27)packages/javascript/bh-shared-ui/src/utils/queries/queries.ts (1)
GenericQueryOptions(19-19)packages/javascript/js-client-library/src/types.ts (6)
AssetGroupTag(152-163)RulesKey(136-136)ObjectsKey(142-142)ObjectKey(141-141)RuleKey(135-135)AssetGroupTagType(124-127)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)cmd/ui/public/mockServiceWorker.js (1)
response(157-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (52)
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx (1)
17-37: Import organization looks clean.The reordering follows a clear pattern (external libraries → hooks → local components) and all imports are properly used throughout the component.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx (1)
27-27: LGTM! Import paths refactored for better specificity.The import paths have been updated to be more explicit—AppIcon now imports from the specific component directory and cn from the utils directory. This improves code clarity and can benefit build performance by avoiding barrel exports.
Also applies to: 29-29
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/AssetGroupMenuItemPrivilegeZonesEnabled.tsx (1)
32-32: Migration touseTagsQueryis correct.The hook replacement properly updates to the standard React Query shape (
dataaliased astags), and the usage on line 75 correctly passestagstotagIdentifierFn. The return type ofgetAssetGroupTagsis properly typed asAssetGroupTag[], matching the function signature expectations. Error handling and null checks are in place.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (1)
24-24: LGTM!The import path update aligns with the centralization of utilities to the parent
PrivilegeZones/utils.tsxmodule.packages/javascript/bh-shared-ui/src/types.ts (1)
27-29: LGTM!Good refactor—exporting constants alongside the type prevents magic strings and enables IDE autocomplete. The derived union type ensures type safety is preserved.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsx (1)
55-110: Overall structure looks good.The component correctly manages accordion state, guards against missing data (Line 61), and conditionally renders sections based on page type. The integration with
useRulesInfiniteQueryand path-based navigation is well structured.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
22-28: LGTM!The type guards using the
inoperator provide effective duck typing for distinguishing between tags and rules.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
34-34: LGTM!The import path update correctly references the centralized utilities module.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx (1)
25-25: LGTM!The import path update correctly references the centralized utilities module.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagGlyphs.ts (1)
62-88: LGTM!The glyph map creation logic correctly handles edge cases (null glyphs, failed qualifiers, empty transformer results) and properly differentiates the "owned" tag type.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.ts (1)
96-111: LGTM!The
createAssetGroupTaganduseCreateAssetGroupTagimplementations correctly await the query invalidation.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
308-327: LGTM: Typed keys improve maintainability.The migration from hardcoded strings (
'member','selector') to typed constants (ObjectKey,RuleKey) at lines 313 and 324 improves type safety and maintainability. This aligns well with the broader refactor introducing generalized data access patterns.
339-376: LGTM: Cleaner implementation with select callbacks.The refactoring of
useOrderedTags,useHighestPrivilegeTag,useHighestPrivilegeTagId,useLabels, anduseOwnedTagIdto useuseTagsQuerywithselectcallbacks is cleaner and more composable than the previous approach.The data flow is now more explicit and leverages React Query's built-in selector optimization.
44-66: No changes needed. Query invalidation is working as intended. The calls toinvalidateQueries(privilegeZonesKeys.rulesByTag(tagId))use React Query v3's default prefix-matching behavior, which correctly invalidates all cached queries for that tag regardless of their filter parameters (sortOrder, environments, disabled, isDefault). This broad invalidation approach is appropriate for mutations that modify server state and aligns with the team's established cache invalidation patterns.Likely an incorrect or invalid review comment.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.test.ts (1)
22-22: LGTM: Import path updated for module reorganization.The import path change from
'../useAssetGroupTags'to'./useAssetGroupTags'reflects the module reorganization where tag-related utilities (useTagLimits, useTagGlyphs, useTagMutations) are now co-located with useAssetGroupTags.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
117-117: Height calculation change may affect layout.The height changed from
h-[75vh](75% of viewport height) toh-[calc(100%-12rem)](100% minus 12rem). This makes the component height relative to its parent container rather than the viewport.Verify that:
- The parent container has the expected height
- The 12rem deduction accounts for all fixed-height siblings (headers, tabs, etc.)
- This change works correctly across different screen sizes and zoom levels
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (2)
105-124: LGTM: Test data updated to match factory changes.The test expectations correctly reflect the naming convention change from
'tier-0-object-5'to'tag-0-object-5'in the mock factories, maintaining consistency between test data and assertions.
126-143: LGTM: Consistent test data update.The rule name expectation correctly updated from
'tier-0-rule-7'to'tag-0-rule-7', matching the factory changes.packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts (2)
27-29: LGTM: Extended mock data with rule count keys.The addition of
CustomRulesKey,DefaultRulesKey, andDisabledRulesKeyto the mockcountsobject (lines 60-62) aligns with the expanded filtering capabilities introduced in the query system. This ensures test data accurately reflects the API response shape.Also applies to: 54-65
78-97: LGTM: Consistent naming convention update from 'tier' to 'tag'.The mock factories now use
'tag-X-rule-Y'and'tag-X-object-Y'naming patterns instead of'tier-X-...'. This improves clarity and aligns with the broader terminology standardization across the codebase.Also applies to: 133-160
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/index.ts (1)
17-19: LGTM: Barrel exports extended with new tag utilities.The addition of exports for
useTagGlyphs,useTagLimits, anduseTagMutationsproperly exposes the new tag-related utility modules. This follows the established barrel export pattern and maintains a clean public API surface.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx (1)
103-111: TagList component testId verified as correct.The component at line 32 uses
'privilege-zones_tags-list_loading-skeleton', which matches the test expectation at line 111. No action needed.packages/javascript/js-client-library/src/types.ts (4)
118-119: LGTM! New constant for highest privilege position.This constant centralizes the magic number
1for tier zero/highest privilege position checks.
129-133: Good refactor to computed property keys.Using
[AssetGroupTagTypeZone]instead of literal1improves maintainability by co-locating the key definition with its usage.
135-142: Semantic key constants align API field names with domain terminology.The mapping (e.g.,
RulesKey = 'selectors',ObjectsKey = 'members') provides a translation layer between UI terminology ("Rules", "Objects") and the API's terminology ("selectors", "members"). This is a good abstraction.
144-150: Interface field names correctly match backend JSON tags.The TypeScript constants resolve to
selectors,custom_selectors,default_selectors,disabled_selectors, andmembers, which align perfectly with the backend struct's JSON tags incmd/api/src/model/assetgrouptags.go.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (2)
60-61: LGTM! Import updated to use new useTagsQuery hook.The change aligns with the broader refactor to generalize tag queries.
97-97: LGTM! Hook usage updated consistently.The
useTagsQuery()hook returns the same data shape (AssetGroupTag[]), so the consumption at line 682 (tagsQuery.data?.filter(...)) works correctly with optional chaining handling the undefined case.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx (2)
91-91: LGTM! Updated to use params object pattern.The call now matches the refactored
useRulesInfiniteQuery(tagId, params: GetRulesParams)signature. The hook handles optional params (disabled,isDefault,counts) with defaults internally.
97-97: Consider the UX impact of fixed viewport height constraint.The
max-h-[75vh]limits the details panel to 75% of viewport height. On smaller screens or when the viewport is resized, this may truncate content unexpectedly. Ensure this works well across typical screen sizes and consider whether a minimum height is also needed.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx (1)
16-18: LGTM! Clean module re-export pattern.The barrel file now exports the
Detailscomponent as the primary entry point for the DetailsV2 module. This is a cleaner public API compared to re-exporting internal utilities.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagLimits.ts (2)
19-19: LGTM! Import path corrected for same-directory import.Using
./useAssetGroupTagsis the correct relative path since both files are in the sameuseAssetGroupTagsdirectory.
22-22: LGTM! Hook updated to use useTagsQuery.The change aligns with the broader refactor, and
tagsQueryis used consistently throughout the function with the same data shape expectations.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (4)
17-17: LGTM! Skeleton import added for loading states.
26-26: LGTM! Import path updated to reflect utility relocation.The
isTagutility was moved to the parentutilsdirectory as part of this refactor.
29-37: LGTM! Clean inline skeleton component.The
ItemSkeletoncomponent is appropriately scoped to this file since it's only used here. The test ID follows the established naming convention.
85-89: LGTM! Fixed skeleton count for loading state.Rendering exactly 3 skeleton items provides a consistent loading experience. This is simpler than dynamically calculating the count and works well for a tag list that typically has a small number of items.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx (4)
30-41: LGTM! Feature flag mock added for tier_management_engine.This enables
useTagsQueryto function in tests since it checks the feature flag before fetching.
86-97: LGTM! Test updated to use new useTagsQuery hook and data shape.The test correctly validates that
useTagsQueryreturns tags with their original positions intact (not sorted).
99-111: LGTM! Test for useOrderedTags updated correctly.The assertions now access
result.current.dataand verify that tags are sorted by position in ascending order.
155-162: LGTM! Labels filter test updated to new data access pattern.The test correctly validates filtering for label and owned tag types.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (2)
40-51: LGTM! Well-structured context interface.The
PrivilegeZonesContextValueinterface properly documents the context shape:
- Required fields (
defaultPath,savePaths,InfoHeader) are always present- Optional enterprise features (
Certification,SalesMessage,ZoneList) are marked with?- Using
React.LazyExoticComponentfor lazy-loaded components is appropriate
53-57: LGTM! Default context value satisfies the interface.The
defaultPrivilegeZoneCtxValueprovides only the required fields, which is correct for the base/community edition.packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (2)
17-17: Consistent migration to useTagsQuery looks good.The refactor from
useAssetGroupTagstouseTagsQueryis applied consistently across both hook implementations. This aligns with the broader consolidation of tag data retrieval in this PR.Also applies to: 39-39, 48-48
53-58: New useSelectedTag hook provides clean abstraction.The hook cleanly delegates to the appropriate implementation based on the
fromPathParamsflag. However, note that both hooks are always called regardless of which result is used—this is necessary to comply with React's rules of hooks but may trigger unnecessary queries.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (2)
59-61: Hardcoded domain name appears to be a placeholder.The hardcoded "TITANCORP.LOCAL" appears to be placeholder text. Ensure this is replaced with dynamic content or confirm it's intentional for this iteration.
29-32: Fix type mismatch between string and numbertagIdparameters.
pathTagIdfromusePZPathParams()is a string, whiletopTagIdfromuseHighestPrivilegeTagId()is a number. WhenpathTagIdis falsy (empty string),topTagIdis selected via the??operator and passed touseRuleInfo, which expects string parameters. Ensure type consistency by convertingtopTagIdto string or adjust the parameter type.⛔ Skipped due to learnings
Learnt from: jvacca-specterops Repo: SpecterOps/BloodHound PR: 1823 File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35 Timestamp: 2025-09-08T19:22:49.284Z Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.Learnt from: mistahj67 Repo: SpecterOps/BloodHound PR: 1803 File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24 Timestamp: 2025-08-25T20:12:35.629Z Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.test.tsx (2)
61-72: Good test coverage for accordion rendering.Tests verify the presence of key UI elements including the "Rules" header, "Total Rules:" label, and all three accordion sections (Custom, Default, Disabled). This provides good coverage for the component's initial render state.
82-101: Sorting test validates query parameters correctly.The test appropriately verifies that toggling the sort button triggers
useRulesInfiniteQuerywith the expected parameters includingsortOrder,environments,isDefault, anddisabledflags.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
34-36: No changes needed. TheSectorMaptype definition and runtime assignment are properly aligned.ObjectsKeyis'members'andRulesKeyis'selectors', which match the literal values used in the runtimesectorMapobject at lines 95-97.packages/javascript/js-client-library/src/client.ts (2)
369-393: Same breaking change pattern for getAssetGroupTagSelectorMembers.The
primary_kindparameter is added beforeoptions, consistent withgetAssetGroupTagMembers. Theeq:prefix format aligns with existing filter patterns in the codebase (e.g., line 660).
347-367: Parameter order change: primary_kind added as 6th parameter, shifting options to 7th.The parameter shift is accurate. However, all call sites in the codebase (in
useAssetGroupTags.tsx) have been updated to passprimary_kindas the 6th argument, and the wrapper function doesn't exposeoptionsas a parameter. No broken call sites exist.
| const glyphQualifier = (glyph: string | null) => !glyph?.includes('http'); | ||
|
|
||
| const glyphTransformer = (glyph: string, darkMode?: boolean): string => { | ||
| const iconDefiniton = findIconDefinition({ prefix: 'fas', iconName: glyph as IconName }); |
There was a problem hiding this comment.
Typo: iconDefiniton should be iconDefinition.
🔎 Proposed fix
- const iconDefiniton = findIconDefinition({ prefix: 'fas', iconName: glyph as IconName });
+ const iconDefinition = findIconDefinition({ prefix: 'fas', iconName: glyph as IconName });
- if (!iconDefiniton) return '';
+ if (!iconDefinition) return '';
- const glyphIconUrl = getModifiedSvgUrlFromIcon(iconDefiniton, {
+ const glyphIconUrl = getModifiedSvgUrlFromIcon(iconDefinition, {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagGlyphs.ts
around line 31, there's a typo: the variable is named "iconDefiniton" instead of
"iconDefinition"; rename the variable to "iconDefinition" everywhere it's
declared and referenced on that line (and elsewhere in the file if present) to
correct the spelling and avoid undefined identifier issues.
| export const useCreateRule = (tagId: string | number | undefined) => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation(createRule, { | ||
| onSettled: async () => { | ||
| await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId!)); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Non-null assertion on potentially undefined tagId.
useCreateRule accepts tagId: string | number | undefined but uses tagId! in the onSettled callback. If called with undefined, this will create an invalid query key.
Consider requiring a defined tagId or guarding the invalidation:
🔎 Proposed fix (option 1: require defined tagId)
-export const useCreateRule = (tagId: string | number | undefined) => {
+export const useCreateRule = (tagId: string | number) => {
const queryClient = useQueryClient();
return useMutation(createRule, {
onSettled: async () => {
- await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId!));
+ await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId));
},
});
};🔎 Proposed fix (option 2: guard the invalidation)
export const useCreateRule = (tagId: string | number | undefined) => {
const queryClient = useQueryClient();
return useMutation(createRule, {
onSettled: async () => {
- await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId!));
+ if (tagId !== undefined) {
+ await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(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 useCreateRule = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(createRule, { | |
| onSettled: async () => { | |
| await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId!)); | |
| }, | |
| }); | |
| }; | |
| export const useCreateRule = (tagId: string | number) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(createRule, { | |
| onSettled: async () => { | |
| await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId)); | |
| }, | |
| }); | |
| }; |
| export const useCreateRule = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(createRule, { | |
| onSettled: async () => { | |
| await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId!)); | |
| }, | |
| }); | |
| }; | |
| export const useCreateRule = (tagId: string | number | undefined) => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(createRule, { | |
| onSettled: async () => { | |
| if (tagId !== undefined) { | |
| await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId)); | |
| } | |
| }, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.ts
around lines 57–64, the callback currently uses a non-null assertion tagId!
which can produce an invalid query key if tagId is undefined; either change the
function signature to require tagId (remove | undefined) so callers must supply
it, or keep tagId optional and guard the invalidation by checking if tagId is
null/undefined before calling
queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(tagId)); ensure the
chosen approach is reflected in the types and callers (update call sites if you
make tagId required) so no non-null assertions are needed.
| export const useDeleteRule = () => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation(deleteRule, { | ||
| onSettled: async (_data, _error, variables) => { | ||
| queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(variables.tagId)); | ||
| queryClient.invalidateQueries(privilegeZonesKeys.ruleDetail(variables.tagId, variables.ruleId)); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing await on query invalidations creates potential race conditions.
The onSettled callback is async but doesn't await the invalidateQueries calls, meaning the mutation could resolve before cache invalidation completes. This is inconsistent with other hooks in this file that do use await.
🔎 Proposed fix
export const useDeleteRule = () => {
const queryClient = useQueryClient();
return useMutation(deleteRule, {
onSettled: async (_data, _error, variables) => {
- queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(variables.tagId));
- queryClient.invalidateQueries(privilegeZonesKeys.ruleDetail(variables.tagId, variables.ruleId));
+ await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(variables.tagId));
+ await queryClient.invalidateQueries(privilegeZonesKeys.ruleDetail(variables.tagId, variables.ruleId));
},
});
};📝 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 useDeleteRule = () => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(deleteRule, { | |
| onSettled: async (_data, _error, variables) => { | |
| queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(variables.tagId)); | |
| queryClient.invalidateQueries(privilegeZonesKeys.ruleDetail(variables.tagId, variables.ruleId)); | |
| }, | |
| }); | |
| }; | |
| export const useDeleteRule = () => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(deleteRule, { | |
| onSettled: async (_data, _error, variables) => { | |
| await queryClient.invalidateQueries(privilegeZonesKeys.rulesByTag(variables.tagId)); | |
| await queryClient.invalidateQueries(privilegeZonesKeys.ruleDetail(variables.tagId, variables.ruleId)); | |
| }, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.ts
around lines 86 to 94, the async onSettled handler calls
queryClient.invalidateQueries without awaiting them which can cause race
conditions; update the handler to await the invalidations (either await each
queryClient.invalidateQueries(...) or await Promise.all([...]) ) so cache
invalidation completes before the mutation settles, matching the pattern used in
other hooks in this file.
| export const useDeleteAssetGroupTag = () => { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation(deleteAssetGroupTag, { | ||
| onSettled: async (_data, _error, tagId) => { | ||
| queryClient.invalidateQueries(privilegeZonesKeys.tags()); | ||
| queryClient.invalidateQueries(privilegeZonesKeys.tagDetail(tagId)); | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same missing await issue on useDeleteAssetGroupTag.
🔎 Proposed fix
export const useDeleteAssetGroupTag = () => {
const queryClient = useQueryClient();
return useMutation(deleteAssetGroupTag, {
onSettled: async (_data, _error, tagId) => {
- queryClient.invalidateQueries(privilegeZonesKeys.tags());
- queryClient.invalidateQueries(privilegeZonesKeys.tagDetail(tagId));
+ await queryClient.invalidateQueries(privilegeZonesKeys.tags());
+ await queryClient.invalidateQueries(privilegeZonesKeys.tagDetail(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 useDeleteAssetGroupTag = () => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(deleteAssetGroupTag, { | |
| onSettled: async (_data, _error, tagId) => { | |
| queryClient.invalidateQueries(privilegeZonesKeys.tags()); | |
| queryClient.invalidateQueries(privilegeZonesKeys.tagDetail(tagId)); | |
| }, | |
| }); | |
| }; | |
| export const useDeleteAssetGroupTag = () => { | |
| const queryClient = useQueryClient(); | |
| return useMutation(deleteAssetGroupTag, { | |
| onSettled: async (_data, _error, tagId) => { | |
| await queryClient.invalidateQueries(privilegeZonesKeys.tags()); | |
| await queryClient.invalidateQueries(privilegeZonesKeys.tagDetail(tagId)); | |
| }, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useTagMutations.ts
around lines 134 to 142, the onSettled handler is declared async but does not
await the queryClient.invalidateQueries calls; update the handler to await the
invalidations (preferably using await
Promise.all([queryClient.invalidateQueries(privilegeZonesKeys.tags()),
queryClient.invalidateQueries(privilegeZonesKeys.tagDetail(tagId))])) so both
cache invalidations complete before onSettled resolves.
| rest.get('/api/v2/features', async (_req, res, ctx) => { | ||
| return res( | ||
| ctx.json({ | ||
| data: [ | ||
| { | ||
| key: 'tier_management_engine', | ||
| enabled: true, | ||
| }, | ||
| ], | ||
| }) | ||
| ); | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the test file to understand context around lines 83-94
wc -l "packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsx"Repository: SpecterOps/BloodHound
Length of output: 176
🏁 Script executed:
# Read the test file around the flagged lines
sed -n '70,110p' "packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsx"Repository: SpecterOps/BloodHound
Length of output: 1097
🏁 Script executed:
# Search for tier_management_engine usage in non-test files
rg -nC5 'tier_management_engine' --type=ts --type=tsx -g '!*.test.*' -g '!*.spec.*'Repository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
# Search for /api/v2/features endpoint usage patterns in non-test files
rg -nC3 'api/v2/features' --type=ts --type=tsx -g '!*.test.*' -g '!*.spec.*'Repository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
# Check for other MSW handlers in the same file to see if async pattern is used elsewhere
rg -nC2 'rest\.(get|post|put|delete)\(' "packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsx"Repository: SpecterOps/BloodHound
Length of output: 483
🏁 Script executed:
# Search for tier_management_engine in TypeScript/TSX files (broader search)
rg 'tier_management_engine' --type ts --glob '!*.test.*' --glob '!*.spec.*'Repository: SpecterOps/BloodHound
Length of output: 1149
🏁 Script executed:
# Search for api/v2/features in TypeScript/TSX files
rg 'api/v2/features' --type ts --glob '!*.test.*' --glob '!*.spec.*'Repository: SpecterOps/BloodHound
Length of output: 577
🏁 Script executed:
# Search for tier_management or feature flag queries in the TagToZoneLabelDialog component specifically
fd 'TagToZoneLabelDialog' -e tsx -e ts | grep -v test | grep -v specRepository: SpecterOps/BloodHound
Length of output: 167
🏁 Script executed:
# Look for any feature flag or features API usage patterns
rg 'features' "packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/" --type ts -A 2 -B 2 | head -100Repository: SpecterOps/BloodHound
Length of output: 2659
🏁 Script executed:
# Read the TagToZoneLabelDialog component
cat -n "packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx"Repository: SpecterOps/BloodHound
Length of output: 6028
🏁 Script executed:
# Check if zoneHandlers.ts is a shared file with the same pattern
cat -n "packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts" | head -150Repository: SpecterOps/BloodHound
Length of output: 6080
Remove unnecessary async keyword.
The handler function is declared as async but contains no await statements. Other handlers in the same test file (lines 69, 76) omit the async keyword for the same pattern.
- rest.get('/api/v2/features', async (_req, res, ctx) => {
+ rest.get('/api/v2/features', (_req, res, ctx) => {
return res(
ctx.json({
data: [Verify whether this feature flag mock is needed for TagToZoneLabelDialog tests.
The tier_management_engine feature flag is used elsewhere in the codebase, and the mock structure is correct. However, the TagToZoneLabelDialog component itself does not directly reference this feature flag. Confirm that this mock is necessary for the test context, or consider removing it if it's not required.
📝 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.
| rest.get('/api/v2/features', async (_req, res, ctx) => { | |
| return res( | |
| ctx.json({ | |
| data: [ | |
| { | |
| key: 'tier_management_engine', | |
| enabled: true, | |
| }, | |
| ], | |
| }) | |
| ); | |
| }), | |
| rest.get('/api/v2/features', (_req, res, ctx) => { | |
| return res( | |
| ctx.json({ | |
| data: [ | |
| { | |
| key: 'tier_management_engine', | |
| enabled: true, | |
| }, | |
| ], | |
| }) | |
| ); | |
| }), |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.test.tsx
around lines 83 to 94, the MSW handler is declared async but uses no await —
remove the unnecessary async keyword; additionally verify whether the
tier_management_engine feature-flag mock is required for these tests and if not,
delete the entire handler to avoid unnecessary mocks (if it is required keep the
handler but without async).
| rest.get('/api/v2/features', async (_req, res, ctx) => { | ||
| return res( | ||
| ctx.json({ | ||
| data: [ | ||
| { | ||
| key: 'tier_management_engine', | ||
| enabled: true, | ||
| }, | ||
| ], | ||
| }) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tier_management_engine feature flag usage and /api/v2/features endpoint
echo "=== Finding FilterDialog implementation ==="
fd -e tsx "FilterDialog" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog --type f -x basename {}
echo -e "\n=== Checking FilterDialog.tsx content (first 100 lines) ==="
find packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog -name "FilterDialog.tsx" -type f -exec head -100 {} \;
echo -e "\n=== Searching for tier_management_engine references ==="
rg -n "tier_management_engine" --type ts --type tsx
echo -e "\n=== Searching for /api/v2/features endpoint references ==="
rg -n "api/v2/features" --type ts --type tsx
echo -e "\n=== Searching for useFeature or feature flag patterns in FilterDialog directory ==="
rg -n "useFeature|useFeatureFlag|getFeature" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog --type ts --type tsxRepository: SpecterOps/BloodHound
Length of output: 4288
🏁 Script executed:
#!/bin/bash
# More comprehensive search without file type restrictions
echo "=== All references to tier_management_engine ==="
grep -r "tier_management_engine" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ 2>/dev/null || echo "No results found"
echo -e "\n=== All references to /api/v2/features ==="
grep -r "api/v2/features" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ 2>/dev/null || echo "No results found"
echo -e "\n=== All references to useFeature ==="
grep -r "useFeature" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ 2>/dev/null || echo "No results found"
echo -e "\n=== Check full FilterDialog.tsx for feature flag usage ==="
wc -l packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.tsx
echo -e "\n=== Check what mocks are actually used in test file ==="
grep -n "rest\|rest.get" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.test.tsx | head -20Repository: SpecterOps/BloodHound
Length of output: 1747
🏁 Script executed:
#!/bin/bash
# Check if parent components actually use the feature flag
echo "=== Check DefaultRoot.tsx for feature flag usage ==="
grep -n "features\|useFeature\|tier_management" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DefaultRoot.tsx | head -20
echo -e "\n=== Check Save.tsx for feature flag usage ==="
grep -n "features\|useFeature\|tier_management" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/Save.tsx | head -20
echo -e "\n=== Check if there's any API call to /api/v2/features in implementations ==="
grep -r "features" packages/javascript/bh-shared-ui/src/views/PrivilegeZones --include="*.tsx" --exclude-dir=".test" | grep -v "test.tsx" | head -20
echo -e "\n=== Check what tests in FilterDialog.test.tsx actually do ==="
sed -n '1,100p' packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.test.tsxRepository: SpecterOps/BloodHound
Length of output: 4097
Remove the unused feature flag mock.
The /api/v2/features mock for tier_management_engine appears unused—the FilterDialog component and its implementation do not query the features endpoint or check this flag. The mock is not required by any test assertions and should be removed.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/FilterDialog/FilterDialog.test.tsx
around lines 29 to 40, remove the unused MSW handler that mocks GET
/api/v2/features returning the tier_management_engine flag; delete that entire
mock block and any related import or setup only tied to this handler if not used
elsewhere, then run the tests to confirm nothing else depends on this mock.
| export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => { | ||
| const firstSeed = rule.seeds[0]; | ||
|
|
||
| return firstSeed.type; | ||
| }; |
There was a problem hiding this comment.
Potential runtime error if seeds array is empty.
getRuleSeedType accesses rule.seeds[0] without checking if the array has elements. If a rule has no seeds, this will return undefined, and accessing firstSeed.type would throw.
🔎 Proposed fix
export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => {
const firstSeed = rule.seeds[0];
+ if (!firstSeed) {
+ throw new Error('Rule has no seeds');
+ }
+
return firstSeed.type;
};Or return a default/undefined with an updated return type if empty seeds are valid.
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx around
lines 30 to 34, getRuleSeedType reads rule.seeds[0] without checking for an
empty array which can cause a runtime error; add a guard that checks if
rule.seeds exists and has at least one element and then return the first seed's
type, otherwise return a safe default (or undefined) and update the function
return type to SeedTypes | undefined (or a concrete default SeedTypes) so
callers handle the empty-seeds case.
| getAssetGroupTagSelectors = (tagId: number | string, options?: RequestOptions) => | ||
| this.baseClient.get<AssetGroupTagSelectorsResponse>(`/api/v2/asset-group-tags/${tagId}/selectors`, { | ||
| ...options, | ||
| params: { | ||
| ...options?.params, | ||
| skip, | ||
| limit, | ||
| environments, | ||
| sort_by, | ||
| }, | ||
| paramsSerializer: { indexes: null }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "getAssetGroupTagSelectors\(" --type ts -A 3Repository: SpecterOps/BloodHound
Length of output: 186
🏁 Script executed:
# Also search in test files and check for any references to the old signature
rg "getAssetGroupTagSelectors" --type ts --type tsx -B 2 -A 2Repository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
# Check if there's any TypeScript definition or interface for this method
rg "getAssetGroupTagSelectors.*=" --type ts -A 5Repository: SpecterOps/BloodHound
Length of output: 657
🏁 Script executed:
rg "getAssetGroupTagSelectors" --type ts -B 2 -A 5Repository: SpecterOps/BloodHound
Length of output: 1626
🏁 Script executed:
# Search for any remaining usages in JavaScript/TypeScript files generally
rg "getAssetGroupTagSelectors\(" -B 1 -A 3Repository: SpecterOps/BloodHound
Length of output: 654
🏁 Script executed:
# Check for any imports or references to understand the usage pattern
rg "getAssetGroupTagSelectors" packages/javascript/ -A 2 -B 2Repository: SpecterOps/BloodHound
Length of output: 1123
🏁 Script executed:
cat -n packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxRepository: SpecterOps/BloodHound
Length of output: 15369
🏁 Script executed:
# Check the RequestOptions type to understand what parameters it supports
rg "RequestOptions" packages/javascript/js-client-library/src/ -A 10 | head -50Repository: SpecterOps/BloodHound
Length of output: 4276
🏁 Script executed:
# Search for all getAssetGroupTagSelectors calls with context
rg "getAssetGroupTagSelectors" --type ts -B 3 -A 3Repository: SpecterOps/BloodHound
Length of output: 1542
🏁 Script executed:
# Check if there's any documentation or changelog that mentions this change
fd -type f -name "*.md" packages/javascript/js-client-library/ | head -5 | xargs grep -l "getAssetGroupTagSelectors" 2>/dev/null || echo "No matches in markdown files"Repository: SpecterOps/BloodHound
Length of output: 281
🏁 Script executed:
# Check the type definition or export of RequestOptions to understand what can be passed
rg "interface RequestOptions|type RequestOptions" packages/javascript/js-client-library/src/ -A 10Repository: SpecterOps/BloodHound
Length of output: 902
Breaking change: getAssetGroupTagSelectors signature simplified.
The method signature was simplified by removing skip, limit, sort_by, and environments parameters. These are now passed through the options.params object. The call site in useAssetGroupTags.tsx has been updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
126-136: Consider handling loading and error states for the rule query.The conditional rendering only checks
ruleQuery.dataexistence anddisabled_atstatus, but doesn't handle loading or error states explicitly. This means:
- While
ruleQueryis loading,ruleQuery.datais undefined, soObjectsAccordionwill briefly render, potentially causing a visual flash when the disabled warning appears.- If
ruleQueryencounters an error, it will fall back to showingObjectsAccordion, which may not be the desired UX.Consider adding explicit loading and error state handling:
🔎 Suggested enhancement for loading/error states
+ {ruleQuery.isLoading ? ( + <div className='flex justify-center items-center'> + <span>Loading rule information...</span> + </div> + ) : ruleQuery.isError ? ( + <div className='flex justify-center items-center gap-2'> + <FontAwesomeIcon icon={faWarning} className='text-orange-500' /> + <span>Error loading rule information</span> + </div> + ) : ruleQuery.data && ruleQuery.data.disabled_at !== null ? ( <div className='flex justify-center items-center gap-2'> <FontAwesomeIcon icon={faWarning} className='text-orange-500' /> <span>Enable this Rule to see Objects</span> </div> ) : ( <ObjectsAccordion kindCounts={objectCountsQuery.data?.counts || {}} totalCount={objectCountsQuery.data?.total_count || 0} /> )}packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
111-111: Type cast workaround could be avoided with proper typing.The query key is cast to
as unknown as string[], which bypasses TypeScript's type checking. This is a minor code smell that reduces type safety.Consider defining the query key with a more specific type that react-query can accept, or ensure that
privilegeZonesKeys.tags()returns a type compatible with react-query's expectations. This would eliminate the need for the double cast.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/js-client-library/src/client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts
- packages/javascript/js-client-library/src/client.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 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.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-11-07T22:29:44.150Z
Learnt from: sirisjo
Repo: SpecterOps/BloodHound PR: 2035
File: cmd/ui/src/views/Explore/ContextMenu/AssetGroupMenuItem.test.tsx:75-77
Timestamp: 2025-11-07T22:29:44.150Z
Learning: In the BloodHound codebase (SpecterOps/BloodHound), when renaming UI terminology from "Selectors" to "Rules", the backend API endpoints (e.g., `/api/v2/asset-groups/:assetGroupId/selectors`), API client method names (e.g., `updateAssetGroupSelector`), and payload field names (e.g., `selector_name`) should remain unchanged. Only user-facing labels and text in the UI should be updated to use "Rules" terminology.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (4)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useRuleInfo(321-329)packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(152-163)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/RulesAccordion.tsx (1)
RulesAccordion(55-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
120-120: Verify the height calculation accounts for all UI elements.The change from
h-[75vh]toh-[calc(100%-12rem)]improves responsiveness by sizing relative to the parent container. However, ensure the 12rem offset correctly accounts for all surrounding elements (header withpt-8, tabs withmt-4, parent padding, etc.). An incorrect value could cause overflow or unexpected layout behavior.Please verify this calculation visually across different viewport sizes and zoom levels to confirm the layout renders correctly without overflow or spacing issues.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
341-378: Refactoring to useTagsQuery improves architectural consistency.The migration from the monolithic
useAssetGroupTagspattern to granular, selector-based hooks usinguseTagsQueryis a solid architectural improvement. The use of selector functions (select) allows for efficient data transformation while maintaining a single source of truth for tag data.Benefits:
- Better separation of concerns
- Improved reusability
- Consistent caching behavior via react-query
- Type-safe data extraction with key constants
44-50: Fix cache invalidation key mismatch in rule mutations.The mutation callbacks (useCreateRule, usePatchRule, useDeleteRule) invalidate queries using
rulesByTag(tagId)with default parameters, but the actual queries are cached with specific filter parameters (sortOrder, environments, disabled, isDefault). In react-query v3's prefix matching, this creates a key mismatch: invalidation keys like['privilege-zones', 'rules', 'tag', 1, undefined, [], undefined, undefined]don't match cached keys like['privilege-zones', 'rules', 'tag', 1, 'asc', ['env-1'], false, false], causing mutations to fail to invalidate the relevant queries.Pass the complete set of filter parameters to
rulesByTag()in mutation callbacks, or use a shared context to ensure mutations invalidate with matching cache keys.⛔ Skipped due to learnings
Learnt from: jvacca-specterops Repo: SpecterOps/BloodHound PR: 1823 File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105 Timestamp: 2025-08-27T15:12:49.129Z Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.Learnt from: jvacca-specterops Repo: SpecterOps/BloodHound PR: 1823 File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:96-105 Timestamp: 2025-09-08T18:47:36.790Z Learning: In the BloodHound codebase, the useUpdateQueryPermissions hook's cache invalidation using `data.query_id` in the onSuccess callback is the preferred implementation, even though the mutation returns a BasicResponse structure.
Description
Details page Rules display redesign
Motivation and Context
Resolves BED-6841
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
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.