feat: Objects Accordion for Details redesign BED-6844 #2160
Conversation
WalkthroughThreads a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ObjectsAccordion
participant Query as InfiniteQuery (useTag/useRuleMembersInfiniteQuery)
participant APIClient as BHEAPIClient
participant Backend as Backend API
participant Nav as useAppNavigate
User->>ObjectsAccordion: expand kind panel
ObjectsAccordion->>Query: start infinite query (tagId, ruleId?, sortOrder, primary_kind, environments)
Query->>APIClient: getAssetGroupTagMembers / getAssetGroupTagSelectorMembers (includes primary_kind)
APIClient->>Backend: HTTP GET /members with query params (primary_kind, environments, skip, limit, sort_by)
Backend-->>APIClient: paginated members
APIClient-->>Query: return data
Query-->>ObjectsAccordion: render list
User->>ObjectsAccordion: click sort header
ObjectsAccordion->>Query: refetch with toggled sortOrder (query key includes primary_kind)
Query->>APIClient: new GET with updated sort_by
Backend-->>APIClient: sorted results
APIClient-->>Query: return data
Query-->>ObjectsAccordion: re-render list
User->>ObjectsAccordion: click object row
ObjectsAccordion->>Nav: navigate(objectDetailsLink(tagId, objectId))
Nav->>Nav: update route
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)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
urangel
added
the
user interface
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (2)
30-34:getRuleSeedTypeassumes a non-emptyseedsarray
getRuleSeedTypeindexesrule.seeds[0]and immediately reads.type. If a selector is ever created without seeds (or seeds are temporarily empty), this will throw. If the API contract guarantees at least one seed per selector you’re fine, otherwise consider making this defensive (e.g., returningundefinedor a default whenseeds.length === 0) and handling that case at the call sites.
36-41:getListHeightworks but can be simplifiedThe conditions cover all possible heights, making the final
return 436;unreachable. You can simplify and avoid redundant comparisons:-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) => { + if (windowHeight > 1080) return 760; + if (windowHeight > 900) return 640; + return 436; +};packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
80-88:kind-aware member keys and fetchers are consistent; double-check key namespace for rule membersThe additions of the optional
kindargument togetAssetGroupTagMembers/getAssetGroupTagRuleMembersand to the corresponding infinite-query hooks look coherent: the value is included in the React Query keys and correctly threaded down to the js-clientgetAssetGroupTagMembers/getAssetGroupTagSelectorMemberscalls, while the default'all'is used only at the key level so “no-kind-filter” requests still omit thekindparam.One thing to confirm:
privilegeZonesKeys.membersByTagAndRulenow returns['tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments]instead of being prefixed withprivilegeZonesKeys.members(). If you rely anywhere on broad invalidation viaprivilegeZonesKeys.members()(or similar prefixes), those calls will no longer catch rule-member queries. If that pattern isn’t used elsewhere, this is fine as-is.Also applies to: 241-263, 264-335
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (2)
147-152: Height calculation is not reactive to window resize.
getListHeight(window.innerHeight)is evaluated once at render time. If the user resizes the browser, the height won't update until a re-render is triggered by other state changes.Consider using a resize hook or CSS-based solution for responsive height:
// Option 1: Use a resize hook const [listHeight, setListHeight] = useState(() => getListHeight(window.innerHeight)); useEffect(() => { const handleResize = () => setListHeight(getListHeight(window.innerHeight)); window.addEventListener('resize', handleResize); return () => window.removeEventListener('resize', handleResize); }, []);Or use CSS
calc()with viewport units if the breakpoints align.
125-127: Accordion toggle behavior may be unintuitive.The current toggle logic
onOpen((prev) => (prev === '' ? kind : ''))will close any open accordion when clicked, rather than switching to the clicked one. If the user clicks "Group" while "User" is open, it closes "User" instead of opening "Group".If the intent is to allow switching between accordions:
-onOpen((prev) => (prev === '' ? kind : '')); +onOpen((prev) => (prev === kind ? '' : kind));This closes the accordion if clicking the same one, or opens the clicked accordion otherwise.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
75-75:totalCountis inconsistent withkindCounts.The hardcoded
totalCount={777}doesn't match the sum of values inkindCounts. While both are placeholders, they should be consistent to avoid confusion.Calculate
totalCountfromkindCounts:+const totalCount = Object.values(kindCounts).reduce((sum, count) => sum + count, 0); + return ( // ... - <ObjectsAccordion kindCounts={kindCounts} totalCount={777} /> + <ObjectsAccordion kindCounts={kindCounts} totalCount={totalCount} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 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.
📚 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/Details/SearchBar.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/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/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/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/js-client-library/src/client.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.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/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:23:30.689Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.689Z
Learning: In ExploreSearchCombobox component (packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx), setting aria-autocomplete={null} on the TextField is intentional to override downshift's default aria-autocomplete attribute, which was flagged by Axe DevTools. This approach satisfies Axe accessibility requirements by removing the conflicting attribute.
Applied to files:
packages/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/Details/SearchBar.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/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/Details/TagList.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
🧬 Code graph analysis (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
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(198-198)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (7)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(41-56)packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
privilegeZonesPath(22-22)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useRuleMembersInfiniteQuery(308-335)useTagMembersInfiniteQuery(264-281)packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/InfiniteQueryFixedList.tsx (2)
InfiniteQueryFixedListProps(23-32)InfiniteQueryFixedList(34-98)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
getListHeight(36-41)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTag(138-149)AssetGroupTagSelector(189-199)SeedTypes(164-164)
⏰ 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). (2)
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (13)
packages/javascript/js-client-library/src/client.ts (1)
361-381: Kind parameter wiring in AGT member client methods looks correctThe new optional
kindargument is added without breaking the existing call signature and is correctly forwarded into the queryparamsfor both tag members and selector members; the merge order withoptions?.paramsis sound.Also applies to: 383-407
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
35-35: Shared utils import for type guards and seed helper looks goodSwitching
getRuleSeedType,isRule, andisTagto the centralized../utilskeeps the Details component thin and aligns with the new shared PrivilegeZones utilities.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (1)
24-24: ImportinggetListHeightfrom shared PrivilegeZones utils is appropriateThe updated import path to
../utilsaligns this component with the centralized PrivilegeZones utilities without changing runtime behavior.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx (1)
25-25: SharedgetListHeightimport for Rules list is consistentThe Rules list now consumes
getListHeightfrom the shared PrivilegeZones utils module, matching the Members list and keeping layout logic centralized.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx (1)
1-3: Index re-export for DetailsV2 is idiomaticRe-exporting
Detailsas the default from thisindex.tsxprovides a clean, conventional entrypoint for the DetailsV2 module.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
30-30: SearchBar now using shared type guards from PrivilegeZones utilsRouting
isTag/isRulethrough the shared../utilsmodule keeps the SearchBar implementation simple and reuses the same tag/rule detection logic as other PrivilegeZones views.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
49-51: Clear TODO for future swap.The TODO comment effectively documents the planned transition to DetailsV2, and keeping the commented import alongside it provides useful context for the eventual changeover.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
20-58: Good test coverage for ObjectsAccordion.The test correctly validates rendering of static elements, kind-specific counts, and alphabetical ordering. The childNodes index approach for order verification is acceptable given the comments explaining the intent.
Consider adding edge case tests (empty
kindCounts, single entry) in a follow-up to improve coverage.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
26-26: Import path update aligns with utility consolidation.The change to import
isTagfrom../utilsis consistent with the PR's refactoring of utility functions to a centralized location.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (1)
40-50: Well-structured context interface.The
PrivilegeZonesContextValueinterface cleanly separates required properties (defaultPath,savePaths,InfoHeader) from optional component slots. The use ofReact.LazyExoticComponentforSummaryandCertificationappropriately supports code-splitting patterns.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (2)
88-89: Both queries are always instantiated.Both
ruleMembersQueryandtagMembersQueryare created unconditionally, though only one is used based onruleIdat line 155. WhileuseInfiniteQuerywithenabled: falsewon't fire network requests, this could be optimized by conditionally calling hooks or restructuring the logic.However, since both hooks check their
enabledcondition internally (tagId/ruleId must be defined), this is acceptable and avoids conditional hook violations.
38-63: ObjectsAccordion implementation looks solid.The component correctly renders a header with total count and maps sorted kind entries to ObjectList panels. Alphabetical sorting via
localeCompareensures deterministic order.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
28-39: Context integration and navigation logic are well-structured.The component correctly consumes
ZoneSelectorfromPrivilegeZonesContext, handles the case where context is missing with an appropriate error, and wires up navigation viatagDetailsLink. The fallback tozoneIdwhendefaultTagIdis undefined ensures robust tag resolution.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx (1)
25-75: Import path change looks correct; optional cleanup for repeated height callsSwitching
getListHeightto import from../utilsmatches the shared PrivilegeZones utils location and should be a no-op behaviorally, assumingutils.tsxexposes the same helper. As an optional cleanup, you could compute the height once (e.g.,const listHeight = getListHeight(window.innerHeight);) and drive the class conditions from that value instead of calling it three times, but this is purely a readability/micro-optimization tweak.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
27-28: Avoid brittlechildNodesindexing; consider Testing Library queries insteadIndexing into
accordion.childNodes[0..2]assumes a very specific DOM structure (no extra wrappers or text nodes), which makes the test fragile to internal layout changes. Also, theasyncon the test function isn’t used.Consider something along these lines to make the ordering check more resilient:
- it('renders', async () => { + it('renders', () => { render(<ObjectsAccordion kindCounts={testKindCounts} totalCount={15} />); @@ - const accordion = screen.getByTestId('privilege-zones_details_objects-accordion'); - const computerAccordionItem = accordion.childNodes[0]; - const groupAccordionItem = accordion.childNodes[1]; - const userAccordionItem = accordion.childNodes[2]; + const accordion = screen.getByTestId('privilege-zones_details_objects-accordion'); + const items = within(accordion).getAllByText(/(Computer|Group|User)/); + const [computerAccordionItem, groupAccordionItem, userAccordionItem] = items;(or, alternatively, exposing a per-item test id or using a more specific query such as
getAllByRoleif your Accordion implementation supports it).This keeps the intent (alphabetical order) while decoupling the test from exact node indexing.
Also applies to: 43-58
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
264-281: Consider using a more specific type forkind.The
kindparameter is currently typed asstring, which is permissive. If the valid values are known and finite (e.g.,'all' | 'User' | 'Group' | 'Computer'), consider using a union type to improve type safety and developer experience with autocomplete.Also applies to: 308-335
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 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/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.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/Details/TagList.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.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/Details/TagList.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.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
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (4)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(179-181)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/utils.ts (1)
PAGE_SIZE(21-21)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
⏰ 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: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (6)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
35-35: LGTM! Import path correctly updated.The import path update from
./utilsto../utilscorrectly reflects the consolidation of PrivilegeZones utilities to a shared parent module.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx (1)
110-110: LGTM! Test ID updated to align with component refactor.The test ID change from
labels-listtotags-listcorrectly reflects the refactored TagList component's generic loading skeleton approach, as noted in the AI summary.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx (1)
1-3: Default re-export wiring looks goodSimple barrel re-export of
Detailsas the default export is clear and matches typical view index patterns; any mismatch with./Details’ export would be caught by TypeScript/build, so no further changes needed here.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
21-42: Test assertions align with component behaviorThe fixture data and assertions for header text, total count (
15), and per-kind labels/counts (User 4, Computer 5, Group 6) match theObjectsAccordionimplementation and its alphabetical sorting intent. This gives good basic coverage for the new UI contract.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
80-88: LGTM! Good backward-compatible design.The query key updates correctly incorporate the
kinddiscriminator for cache separation. The design choice to addkindat the end of function signatures (for backward compatibility with positional arguments) while placing it logically in the query key structure (beforesortOrder) is well thought out.
241-262: The API client correctly accepts and passes thekindparameter through to the backend. BothgetAssetGroupTagMembersandgetAssetGroupTagSelectorMembersmethods properly includekindin their HTTP request params, and the backend is designed to handle both defined and undefined values appropriately. No changes needed.
urangel
force-pushed
the
BED-6844
branch
2 times, most recently
from
5253fac to
be32771
Compare
|
|
||
| const Details = React.lazy(() => import('./Details')); | ||
| // TODO: these will be swapped when all work is ready for the details redesign | ||
| const Details = React.lazy(() => import('./DetailsV2')); |
There was a problem hiding this comment.
Note to self: swap this back before merging to main
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
17-41: Loading skeleton behavior is correct; consider simplifying skeleton helperThe added loading branch correctly renders three
<li>skeletons withprivilege-zones_tags-list_loading-skeleton, matching the updated test and keeping the list markup consistent. ImportingisTagfrom../utilsalso aligns with the new shared utilities.If you want to simplify the skeleton logic, you could convert
ItemSkeletoninto a small component and map over a plain length array instead of an array of functions, e.g.:-const ItemSkeleton = (key: number) => { - return ( - <li - key={key} - data-testid={`privilege-zones_tags-list_loading-skeleton`} - className='border-y border-neutral-light-3 dark:border-neutral-dark-3 relative w-full'> - <Skeleton className='h-10 rounded-none' /> - </li> - ); -}; - -const itemSkeletons = [ItemSkeleton, ItemSkeleton, ItemSkeleton]; +const ItemSkeleton: React.FC = () => ( + <li + data-testid='privilege-zones_tags-list_loading-skeleton' + className='border-y border-neutral-light-3 dark:border-neutral-dark-3 relative w-full'> + <Skeleton className='h-10 rounded-none' /> + </li> +); ... - {listQuery.isLoading ? ( - itemSkeletons.map((skeleton, index) => { - return skeleton(index); - }) + {listQuery.isLoading ? ( + Array.from({ length: 3 }).map((_, index) => <ItemSkeleton key={index} />)Purely optional, but a bit more idiomatic.
Also applies to: 86-89
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
41-45: Random kindCounts per render will cause non-deterministic UI and tests
kindCountsis rebuilt withMath.random()on every render, andtotalCountis hardcoded to777. This means:
- The per-kind counts change whenever the component re-renders.
- The displayed total is inconsistent with the per-kind values.
- Any future visual/snapshot tests will be flaky.
Even as a TODO placeholder, it’s safer to use deterministic stub data until the API is wired, for example:
-// TODO: these counts should be coming from the API once those updates are ready -const kindCounts: Record<string, number> = {}; -Object.keys(ActiveDirectoryNodeKind).forEach((kind) => { - kindCounts[kind] = Math.ceil(Math.random() * 25); -}); +// TODO: these counts should be coming from the API once those updates are ready +const kindCounts: Record<string, number> = Object.fromEntries( + Object.keys(ActiveDirectoryNodeKind).map((kind, index) => [kind, (index + 1) * 5]) +);Or compute a fixed map outside the component so values are stable across renders.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
22-28: Harden type guards to avoid runtime errors on non-object inputsBoth
isTagandisRulecurrently do:export const isTag = (data: any): data is AssetGroupTag => { return 'kind_id' in data; }; export const isRule = (data: any): data is AssetGroupTagSelector => { return 'is_default' in data; };If
datais everundefined,null, or a primitive (some call sites pass loosely-typed values), this will throw aTypeError.Guard the input before using the
inoperator, e.g.:-export const isTag = (data: any): data is AssetGroupTag => { - return 'kind_id' in data; -}; - -export const isRule = (data: any): data is AssetGroupTagSelector => { - return 'is_default' in data; -}; +export const isTag = (data: unknown): data is AssetGroupTag => + typeof data === 'object' && data !== null && 'kind_id' in data; + +export const isRule = (data: unknown): data is AssetGroupTagSelector => + typeof data === 'object' && data !== null && 'is_default' in data;This keeps the guards total and safe for all call sites.
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
20-58: Render and ordering test is useful butchildNodesaccess is brittleThe test does a nice job asserting labels, counts, and alphabetical ordering. The direct
accordion.childNodes[0/1/2]access is a bit fragile, since DOM structure or extra text nodes could break this even if behavior is unchanged.Consider querying elements more semantically, for example:
- Use a shared test id pattern (e.g.,
privilege-zones_details_object-accordion-item) andwithin(accordion).getAllByTestId(...)to check order, or- Query header text nodes (e.g., all buttons or headings within the accordion) and assert their order from the returned array.
This keeps the ordering assertion while decoupling the test from exact DOM node structure.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
148-152: RepeatedgetListHeight(window.innerHeight)and direct window accessThe height class is currently derived by calling
getListHeight(window.innerHeight)three times:className={cn(`border-neutral-5`, { 'h-[760px]': getListHeight(window.innerHeight) === 760, 'h-[640px]': getListHeight(window.innerHeight) === 640, 'h-[436px]': getListHeight(window.innerHeight) === 436, })}This works, but you can make it a bit cleaner and friendlier to non-browser environments by computing once and guarding window access, e.g.:
const height = typeof window !== 'undefined' ? getListHeight(window.innerHeight) : 436; <div className={cn('border-neutral-5', { 'h-[760px]': height === 760, 'h-[640px]': height === 640, 'h-[436px]': height === 436, })} >Same pattern could be reused in MembersList.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
80-88:membersByTag/membersByTagAndRulekeys:kinddimension looks good; consider prefix consistencyIncluding
kindin both member-related keys (beforesortOrder) is a good way to separate caches per kind. One nit:membersByTagis prefixed withprivilegeZonesKeys.members(), butmembersByTagAndRuleis not, which makes it harder to bulk‑invalidate or group all “members” queries by prefix. Consider:- membersByTagAndRule: ( - tagId: string | number, - ruleId: string | number | undefined, - sortOrder: SortOrder, - environments: string[] = [], - kind: string = 'all' - ) => ['tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments] as const, + membersByTagAndRule: ( + tagId: string | number, + ruleId: string | number | undefined, + sortOrder: SortOrder, + environments: string[] = [], + kind: string = 'all' + ) => + [...privilegeZonesKeys.members(), 'tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments] as const,This keeps the key hierarchy parallel to
rules/rulesByTagand makes anyinvalidateQueries(privilegeZonesKeys.members())pattern future‑proof.
241-258:getAssetGroupTagMembers:kindforwarding is correct; explicit defaults would better match query key semanticsThreading
kindandenvironmentsthrough toapiClient.getAssetGroupTagMemberslooks correct. To avoid any behavior change where the backend might seeundefinedinstead of an empty list or'all', and to keep the request semantics aligned with the query key (which defaultskindto'all'), consider giving these parameters explicit defaults:-export const getAssetGroupTagMembers = ( - tagId: number | string, - skip = 0, - limit = PAGE_SIZE, - sortOrder: SortOrder = 'asc', - environments?: string[], - kind?: string -) => +export const getAssetGroupTagMembers = ( + tagId: number | string, + skip = 0, + limit = PAGE_SIZE, + sortOrder: SortOrder = 'asc', + environments: string[] = [], + kind: string = 'all' +) => createPaginatedFetcher<AssetGroupTagMemberListItem>( () => apiClient.getAssetGroupTagMembers( tagId, skip, limit, sortOrder === 'asc' ? 'name' : '-name', environments, - kind + kind ),Based on learnings, this helps keep the new
kindfilter consistent with how backend “kinds” are used to drive graph query filters.Please double‑check the js‑client and API contracts to confirm that
kind: 'all'is the intended default and thatenvironmentsshould default to an empty array rather thanundefined.
283-302:getAssetGroupTagRuleMembers: mirrorkind/environmentsdefaults for rule‑scoped membersSame comments here as for
getAssetGroupTagMembers: thekindandenvironmentsplumbing intogetAssetGroupTagSelectorMemberslooks correct, but explicit defaults would make behavior clearer and align with key defaults and any existing expectations that “no filter” means all environments andkind='all':-export const getAssetGroupTagRuleMembers = ( - tagId: number | string, - ruleId: number | string, - skip: number = 0, - limit: number = PAGE_SIZE, - sortOrder: SortOrder = 'asc', - environments?: string[], - kind?: string -) => +export const getAssetGroupTagRuleMembers = ( + tagId: number | string, + ruleId: number | string, + skip: number = 0, + limit: number = PAGE_SIZE, + sortOrder: SortOrder = 'asc', + environments: string[] = [], + kind: string = 'all' +) => createPaginatedFetcher( () => apiClient.getAssetGroupTagSelectorMembers( tagId, ruleId, skip, limit, sortOrder === 'asc' ? 'name' : '-name', - environments, - kind + environments, + kind ),Please verify that the selector‑members endpoint expects the same default semantics as the tag‑members endpoint (
kind='all', empty environments list).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
🧰 Additional context used
🧠 Learnings (13)
📚 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/views/PrivilegeZones/Details/MembersList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.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/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.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.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/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.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.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 version of react-query used in this codebase doesn't have typeguard support for itemId when used in the enabled field, so type assertions may still be necessary even with enabled checks in place.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/PrivilegeZones/utils.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-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/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/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:23:30.689Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.689Z
Learning: In ExploreSearchCombobox component (packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx), setting aria-autocomplete={null} on the TextField is intentional to override downshift's default aria-autocomplete attribute, which was flagged by Axe DevTools. This approach satisfies Axe accessibility requirements by removing the conflicting attribute.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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/Details/TagList.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
🧬 Code graph analysis (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (5)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(498-503)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(58-58)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTag(138-149)AssetGroupTagSelector(189-199)SeedTypes(164-164)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (4)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(179-181)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/utils.ts (1)
PAGE_SIZE(21-21)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: run-go-unit-tests
🔇 Additional comments (7)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx (1)
103-111: Loading skeleton test ID matches component implementationThe updated test ID aligns with
TagList’s loading skeleton (privilege-zones_tags-list_loading-skeleton) and keeps the test accurate.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (1)
17-25: Import realigned to shared PrivilegeZones utilsUsing
getListHeightfrom../utilsis consistent with the new centralized utilities and keeps MembersList aligned with other Details components.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
16-31: Type-guard imports correctly point to shared utilsSwitching
isRule/isTagimports to../utilsmatches the new shared PrivilegeZones utilities and keeps SearchBar’s behavior unchanged.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
17-23: > Likely an incorrect or invalid review comment.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
27-30: Remove unusedtopTagIdvariable or implement proper fallback logic
usePZPathParams()returnszoneIdas a string (defaulting to''when not present in route params), so the destructuring default on line 28 will never activate. ThetopTagIdvariable is imported but never used, making it dead code.Either remove the import and variable, or refactor to handle the fallback case:
const { zoneId: rawZoneId, tagTypeDisplay, tagId: defaultTagId, tagDetailsLink } = usePZPathParams(); const zoneId = rawZoneId || topTagId?.toString() || ''; const tagId = defaultTagId || zoneId;⛔ Skipped due to learnings
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.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/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
264-278:useTagMembersInfiniteQuery:kindandenvironmentsare correctly threaded into key and fetcherThe hook cleanly propagates
environmentsandkindboth into the query key and intogetAssetGroupTagMembers, so each distinct combination gets its own cache entry and fetch behavior. The non‑null assertion ontagIdfor the key is guarded byenabled: tagId !== undefined, matching the existing pattern in this file.
308-331:useRuleMembersInfiniteQuery:kinddimension is consistently included in key and fetch logicThe rule‑scoped infinite query correctly adds
kindandenvironmentsto the query key and forwards them togetAssetGroupTagRuleMembers, so rule member lists are independently cached per(tagId, ruleId, kind, environments, sortOrder). The error handling for missingtagId/ruleIdis unchanged and still appropriate for this hook.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
Show resolved
Hide resolved
| export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => { | ||
| const firstSeed = rule.seeds[0]; | ||
|
|
||
| return firstSeed.type; | ||
| }; | ||
|
|
||
| export const getListHeight = (windoHeight: number) => { | ||
| if (windoHeight > 1080) return 760; | ||
| if (1080 >= windoHeight && windoHeight > 900) return 640; | ||
| if (900 >= windoHeight) return 436; | ||
| return 436; | ||
| }; |
There was a problem hiding this comment.
getRuleSeedType assumes a non-empty seeds array
getRuleSeedType currently reads:
export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => {
const firstSeed = rule.seeds[0];
return firstSeed.type;
};If rule.seeds is ever empty, this will throw. If the domain truly guarantees at least one seed, a defensive assertion (or explicit error) would document that; otherwise, consider a safe fallback:
export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => {
- const firstSeed = rule.seeds[0];
- return firstSeed.type;
+ const firstSeed = rule.seeds[0];
+ if (!firstSeed) {
+ // Fallback or throw; adjust to match domain expectations
+ throw new Error('AssetGroupTagSelector has no seeds');
+ }
+ return firstSeed.type;
};Optionally, you could also tidy getListHeight while you’re here:
-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) => {
+ if (windowHeight > 1080) return 760;
+ if (windowHeight > 900) return 640;
+ return 436;
+};🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx around
lines 30–41, getRuleSeedType blindly accesses rule.seeds[0].type which will
throw if seeds is empty; add a defensive check: validate that rule.seeds exists
and has length > 0, and either throw a clear Error/assertion documenting the
invariant or return a safe fallback SeedTypes value (or undefined) depending on
domain expectations; update the function signature/return type if returning a
fallback/undefined. Also tidy getListHeight by fixing the parameter name
(windowHeight), simplifying the comparisons to a clear if/else chain or using
min/max logic, and keep a final default return value.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
29-40: Unresolved: Incorrect use ofkeyprop insideItemSkeleton.This issue was previously flagged but remains unaddressed. The
keyprop should be applied at the call site (line 87-89) when mapping, not passed as a parameter and used inside the component's JSX. React ignoreskeywhen set inside the returned element.Please refer to the previous review comment for the suggested fix.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
41-45: ReplaceMath.random()placeholder counts with deterministic values.The placeholder
kindCountsbuilt withMath.random()inside render still causes non-deterministic UI and test output on every render (previously flagged in this file). That makes debugging and snapshots flaky and can mask regressions.You can keep it simple and deterministic while remaining obviously fake:
- // TODO: these counts should be coming from the API once those updates are ready - const kindCounts: Record<string, number> = {}; - Object.keys(ActiveDirectoryNodeKind).forEach((kind) => { - kindCounts[kind] = Math.ceil(Math.random() * 25); - }); + // TODO: these counts should be coming from the API once those updates are ready + const kindCounts: Record<string, number> = Object.fromEntries( + Object.keys(ActiveDirectoryNodeKind).map((kind, i) => [kind, (i + 1) * 5]) + );This preserves the “obviously mock” feel but is stable across renders.
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
27-58: Tighten DOM queries to reduce brittleness and drop unnecessaryasync.The order check via
accordion.childNodes[0/1/2]and baregetByText('4' | '5' | '6')couples the test to the Accordion’s exact DOM tree and text distribution. A small internal markup change (extra wrapper, additional numeric text) will break this test without a real regression.Consider:
- Querying items via roles or test-ids (e.g.,
getAllByRole/getAllByTestId) and asserting on their text content in order, instead of indexingchildNodes.- Scoping number assertions with
withinto the corresponding accordion item.- Removing
asyncfrom the test signature since nothing is awaited.These are behavioral no-ops but will make the test more resilient.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
17-18: Use a type-only import forAssetGroupTag(optional).
AssetGroupTagis only used for typing thezoneparameter inhandleZoneClick. To avoid introducing an unnecessary runtime dependency on'js-client-library'from this component, consider switching to a type-only import:-import { AssetGroupTag } from 'js-client-library'; +import type { AssetGroupTag } from 'js-client-library';This is a small tree‑shaking and clarity win without changing behavior.
Also applies to: 47-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 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/Details/TagList.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.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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.tsxpackages/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/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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/Details/TagList.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
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (5)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(498-503)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(58-58)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
⏰ 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). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (7)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (2)
17-17: LGTM!The Skeleton import is appropriately added to support the new loading state UI.
26-26: LGTM!The import path adjustment aligns with the PR's consolidation of PrivilegeZones utilities.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (5)
80-88: Kind-aware query keys look correct and backward compatibleIncluding
kindinmembersByTag/membersByTagAndRulekeys (with a default of'all') cleanly separates caches by object kind while keeping existing call sites working, sincekindis added as a trailing param with a safe default. The ordering (tagId,kind,sortOrder,...environments) is stable and consistent with how the hooks construct these keys.
246-258: getAssetGroupTagMembers: threadingkindthrough to the client is soundExtending
getAssetGroupTagMemberswith optionalenvironmentsandkindand passing them through toapiClient.getAssetGroupTagMembersis straightforward and source-compatible (new params are trailing and optional). This keeps the pagination wrapper intact while enabling kind-level filtering at the API layer.
267-278: useTagMembersInfiniteQuery correctly propagateskindinto key and fetcherThe hook now:
- Includes
kindin the query key viaprivilegeZonesKeys.membersByTag.- Passes
kinddown togetAssetGroupTagMembers.With
enabled: tagId !== undefined, the non-null assertion ontagId!remains safe. Overall, the change is consistent and should give per-kind caches without breaking existing callers that omitkind.
289-302: getAssetGroupTagRuleMembers: kind-aware extension is consistent with tag membersAdding optional
environmentsandkindand forwarding them toapiClient.getAssetGroupTagSelectorMembersmirrors the tag-members helper and preserves the existing signature shape. Pagination and sort-by-name behavior are unchanged, so the new filter is additive only.
312-331: useRuleMembersInfiniteQuery: kind threaded end-to-end with no regressionsThe rule-members infinite query now:
- Uses
privilegeZonesKeys.membersByTagAndRule(tagId!, ruleId, sortOrder, environments, kind)so keys are kind-aware.- Forwards
environmentsandkindintogetAssetGroupTagRuleMembers.The guard
enabled: tagId !== undefined && ruleId !== undefinedstill protects the non-null assertions. This keeps behavior identical for callers that don’t passkind, while enabling the new DetailsV2/ObjectsAccordion use cases.
| const { tagId: topTagId } = useHighestPrivilegeTagId(); | ||
| const { zoneId = topTagId?.toString(), tagTypeDisplay, tagId: defaultTagId } = usePZPathParams(); | ||
| const { zoneId = topTagId?.toString(), tagTypeDisplay, tagId: defaultTagId, tagDetailsLink } = usePZPathParams(); | ||
| const tagId = !defaultTagId ? zoneId : defaultTagId; | ||
|
|
||
| const navigate = useAppNavigate(); | ||
|
|
||
| const context = useContext(PrivilegeZonesContext); | ||
| if (!context) { | ||
| throw new Error('Details must be used within a PrivilegeZonesContext.Provider'); | ||
| } | ||
| const { InfoHeader } = context; | ||
| const { InfoHeader, ZoneSelector } = context; | ||
|
|
||
| if (!tagId) return null; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find usePZPathParams definition
fd -type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "usePZPathParams" | head -20Repository: SpecterOps/BloodHound
Length of output: 1848
🏁 Script executed:
#!/bin/bash
# Search for usePZPathParams export and definition
rg "export.*usePZPathParams|function usePZPathParams" -A 20 -B 2Repository: SpecterOps/BloodHound
Length of output: 3324
🏁 Script executed:
#!/bin/bash
# Read the Details.tsx file around lines 27-40
sed -n '1,50p' packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx | cat -nRepository: SpecterOps/BloodHound
Length of output: 2420
🏁 Script executed:
#!/bin/bash
# Get the import statements to confirm what's being used
head -35 packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxRepository: SpecterOps/BloodHound
Length of output: 1613
Fix tagId / zoneId fallback so highest-privilege tag is actually used.
usePZPathParams() always returns zoneId: '' (never undefined), so the destructuring default:
const { zoneId = topTagId?.toString(), tagTypeDisplay, tagId: defaultTagId, tagDetailsLink } = usePZPathParams();never executes. The = topTagId?.toString() default only triggers on undefined, not empty strings. Combined with line 29's logic, this results in tagId = '', causing if (!tagId) return null; to exit the component instead of falling back to the highest-privilege tag.
Separate path params from the fallback logic:
- const { zoneId = topTagId?.toString(), tagTypeDisplay, tagId: defaultTagId, tagDetailsLink } = usePZPathParams();
- const tagId = !defaultTagId ? zoneId : defaultTagId;
+ const { zoneId, tagTypeDisplay, tagId: pathTagId, tagDetailsLink } = usePZPathParams();
+ const effectiveZoneId = zoneId || topTagId?.toString() || '';
+ const tagId = pathTagId || effectiveZoneId;This ensures URL tagId (or labelId) wins when present, otherwise falls back to route zoneId, then to topTagId if both are missing.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (1)
38-51: Fix TS compile error:Reactnamespace is referenced but not imported.
EmptyHeader: React.FCandReact.LazyExoticComponent<React.FC>referenceReact, but this file only imports{ createContext, FC }from'react'.-import { createContext, FC } from 'react'; +import type { FC, LazyExoticComponent } from 'react'; +import { createContext } from 'react'; -const EmptyHeader: React.FC = () => <></>; +const EmptyHeader: FC = () => null; export interface PrivilegeZonesContextValue { defaultPath: string; savePaths: string[]; InfoHeader: FC; ZoneSelector?: FC<{ onZoneClick?: (zone: AssetGroupTag) => void }>; LabelSelector?: FC<{ onLabelClick?: (label: AssetGroupTag) => void }>; SupportLink?: FC; - Summary?: React.LazyExoticComponent<React.FC>; - Certification?: React.LazyExoticComponent<React.FC>; + Summary?: LazyExoticComponent<FC>; + Certification?: LazyExoticComponent<FC>; SalesMessage?: FC; ZoneList?: FC<{ zones: AssetGroupTag[]; setPosition: (position: number) => void; name: string }>; }packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
70-94: Fix react-query key bug:membersByTagAndRuleis missing themembers()prefix.Right now,
membersByTagAndRulereturns['tag', ...]whilemembersByTagreturns[..., members(), ...]. This will fragment caching and likely break invalidation.membersByTagAndRule: ( tagId: string | number, ruleId: string | number | undefined, sortOrder: SortOrder, environments: string[] = [], kind: string = 'all' - ) => ['tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments] as const, + ) => [...privilegeZonesKeys.members(), 'tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments] as const,
223-240: Alignenabledwith the runtime guards to avoid queries that immediately reject.These queries are enabled on
tagId !== undefined/ruleId !== undefined, but thequeryFnrejects on falsy values. IftagId/ruleIdis''(common from URL params), the query will run and immediately reject.- enabled: tagId !== undefined, + enabled: Boolean(tagId), ... - enabled: tagId !== undefined, + enabled: Boolean(tagId), ... - enabled: tagId !== undefined && ruleId !== undefined, + enabled: Boolean(tagId) && Boolean(ruleId),Also applies to: 264-282, 308-336
♻️ Duplicate comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (2)
27-30: FixzoneIdfallback whenusePZPathParams()returns empty strings (notundefined).The default
zoneId = topTagId?.toString()won’t run ifzoneIdis'', which can leavetagIdempty and short-circuit the view. Please apply the fallback via||(or normalize in the hook) and confirm with a quick route-path smoke test.
41-45: RemoveMath.random()from render; make placeholder counts deterministic/stable.This makes renders nondeterministic and can cause flaky snapshots/visual diffs. Move deterministic placeholder data outside the component or compute once (e.g.,
useMemo([])), then remove before shipping when API-backed.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
85-92: Preserve rule context in object navigation (includeruleIdwhen present).Currently, clicking a row always navigates via the tag-level route. If
objectDetailsLinksupportsruleId, pass it through (or use the rule-scoped helper) so rule-level pages don’t lose context.- const handleClick = (id: number) => navigate(objectDetailsLink(tagId, id)); + const handleClick = (id: number) => + navigate(ruleId ? objectDetailsLink(tagId, id, ruleId) : objectDetailsLink(tagId, id));(Please confirm the exact
objectDetailsLinksignature via TS types.)
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (2)
22-32: Prefer hooks barrel exports over deep imports forusePZPathParams.Based on learnings, this hook is available via existing barrels; a barrel import reduces path fragility during refactors.
146-153: Avoid directwindow.innerHeightreads during render (and compute listHeight once).At minimum, compute
const listHeight = getListHeight(window.innerHeight)once and reuse; ideally drive this from a resize-aware hook to keep behavior consistent in tests and on resize.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 Learning: 2025-07-17T16:28:00.770Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/views/Explore/GraphView.tsx:155-159
Timestamp: 2025-07-17T16:28:00.770Z
Learning: In the BloodHound codebase GraphView.tsx, the context menu toggle logic that switches between null and new coordinates (closing on different item clicks) is intentional, expected behavior as of the current implementation, though this may change in the future.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
📚 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/views/PrivilegeZones/PrivilegeZonesContext.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/views/PrivilegeZones/Details/TagList.test.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/views/PrivilegeZones/Details/TagList.test.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.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/PrivilegeZones/DetailsV2/Details.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-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
⏰ 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: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx (1)
110-110: LGTM! Test ID correctly updated.The test ID change from
privilege-zones_labels-list_loading-skeletontoprivilege-zones_tags-list_loading-skeletonappropriately reflects the component's updated terminology, making it more generic for both labels and zones while maintaining the same test coverage.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
241-262: Verify apiClient/js-client-library signatures for the new(environments?, kind?)threading.These functions now pass
environmentsandkindthrough toapiClient.getAssetGroupTagMembers/apiClient.getAssetGroupTagSelectorMembers. Please confirm the parameter order and “kind = 'all'” semantics match the js-client-library typings + backend expectations (a TS typecheck should catch mismatches).Also applies to: 283-306
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
49-60: Verify Accordion a11y/expected structure with doodleui (Trigger semantics).This uses a custom
Buttonto toggle the controlledAccordionvalue; if doodleui expects anAccordionTrigger(for aria-expanded/keyboard behavior), consider switching to the library’s trigger component or ensuring equivalent aria attributes are present.Also applies to: 114-161
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (2)
36-41: TidygetListHeightand fix the parameter name typoThe logic is correct but redundant, and
windoHeightis a typo. You can simplify and clarify:-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) => { + if (windowHeight > 1080) return 760; + if (windowHeight > 900) return 640; + return 436; +};
22-34: HardenisTag/isRuleand seed access to avoid runtime throwsUsing the
inoperator directly ondata: anywill throw ifdataisnull,undefined, or a primitive, andgetRuleSeedTypewill throw whenseedsis empty. Safer guards keep these helpers from crashing callers.Consider:
-export const isTag = (data: any): data is AssetGroupTag => { - return 'kind_id' in data; -}; - -export const isRule = (data: any): data is AssetGroupTagSelector => { - return 'is_default' in data; -}; - -export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => { - const firstSeed = rule.seeds[0]; - - return firstSeed.type; -}; +export const isTag = (data: unknown): data is AssetGroupTag => + typeof data === 'object' && data !== null && 'kind_id' in data; + +export const isRule = (data: unknown): data is AssetGroupTagSelector => + typeof data === 'object' && data !== null && 'is_default' in data; + +export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => { + const firstSeed = rule.seeds[0]; + if (!firstSeed) { + throw new Error('AssetGroupTagSelector has no seeds'); + } + return firstSeed.type; +};packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (2)
27-30: FixtagId/zoneIdfallback so highest‑privilege tag is actually used
usePZPathParamsalready defaultszoneIdto'', so the destructuring default (zoneId = topTagId?.toString()) never runs. When the URL lacks bothzoneIdandlabelId,tagIdends up as''and the component returnsnullinstead of falling back totopTagId.You can separate raw path params from the fallback:
- const { zoneId = topTagId?.toString(), tagTypeDisplay, tagId: defaultTagId, tagDetailsLink } = - usePZPathParams(); - const tagId = !defaultTagId ? zoneId : defaultTagId; + const { zoneId, tagTypeDisplay, tagId: pathTagId, tagDetailsLink } = usePZPathParams(); + const effectiveZoneId = zoneId || topTagId?.toString() || ''; + const tagId = pathTagId || effectiveZoneId;This preserves path
tagId/labelIdwhen present, otherwise falls back tozoneId, then to the highest‑privilege tag.
41-46: AvoidMath.random()forkindCountsplaceholder dataBuilding
kindCountswithMath.random()inside render means counts change on every render, which can confuse users and make visual/snapshot tests flaky. The hard‑coded777further disconnects the header from the list.Prefer a deterministic placeholder (or even zeroed counts) until the API is wired, e.g.:
- // TODO: these counts should be coming from the API once those updates are ready - const kindCounts: Record<string, number> = {}; - Object.keys(ActiveDirectoryNodeKind).forEach((kind) => { - kindCounts[kind] = Math.ceil(Math.random() * 25); - }); + // TODO: replace with real counts from the API once available + const placeholderKindCounts: Record<string, number> = Object.fromEntries( + Object.keys(ActiveDirectoryNodeKind).map((kind, index) => [kind, (index + 1) * 5]) + ); + const kindCounts = placeholderKindCounts; + const totalCount = Object.values(kindCounts).reduce((sum, v) => sum + v, 0); ... - <ObjectsAccordion kindCounts={kindCounts} totalCount={777} /> + <ObjectsAccordion kindCounts={kindCounts} totalCount={totalCount} />Also applies to: 75-76
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
85-92: Preserve rule context when navigating to object details
objectDetailsLinkaccepts an optionalruleId, buthandleClickcurrently calls it with only(tagId, id), so rule-level pages navigate to tag-level object details and drop theruleIdsegment.You can preserve the current rule context like this:
- const { ruleId, memberId, tagId, objectDetailsLink } = usePZPathParams(); + const { ruleId, memberId, tagId, objectDetailsLink } = usePZPathParams(); ... - const handleClick = (id: number) => navigate(objectDetailsLink(tagId, id)); + const handleClick = (id: number) => navigate(objectDetailsLink(tagId, id, ruleId));This still falls back to the tag‑level route when
ruleIdis undefined.
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (2)
148-152: Compute list height once instead of callinggetListHeightthree timesMinor readability/efficiency nit:
getListHeight(window.innerHeight)is called three times with the same argument. Computing it once makes the mapping clearer:- <div - className={cn(`border-neutral-5`, { - 'h-[760px]': getListHeight(window.innerHeight) === 760, - 'h-[640px]': getListHeight(window.innerHeight) === 640, - 'h-[436px]': getListHeight(window.innerHeight) === 436, - })}> + {(() => { + const listHeight = getListHeight(window.innerHeight); + return ( + <div + className={cn('border-neutral-5', { + 'h-[760px]': listHeight === 760, + 'h-[640px]': listHeight === 640, + 'h-[436px]': listHeight === 436, + })}> + <InfiniteQueryFixedList<AssetGroupTagMemberListItem> + itemSize={40} + queryResult={ruleId ? ruleMembersQuery : tagMembersQuery} + renderRow={Row} + renderLoadingRow={LoadingRow} + /> + </div> + ); + })()}(or compute
listHeightabove the return and reuse it here).
22-22: AligngetListHeightimport with established pattern in the codebaseThe import
import { getListHeight } from '..';works due to the index re-export, but it's inconsistent with how other components in this module import it. BothDetails/MembersList.tsxandDetails/RulesList.tsximport directly from../utils, andDetailsV2should follow the same pattern:-import { getListHeight } from '..'; +import { getListHeight } from '../utils';packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
80-88: AlignmembersByTagAndRulequery key with other privilege zones keys
membersByTagAndRuleno longer includes theprivilegeZonesKeys.members()prefix, so its keys are shaped like['tag', tagId, 'rule', ruleId, kind, sortOrder, ...]whilemembersByTagstarts with['privilege-zones', 'members', ...]. That breaks the previous consistency and may interfere with anyinvalidateQueries(privilegeZonesKeys.members())patterns.Consider restoring the common prefix:
- membersByTagAndRule: ( - tagId: string | number, - ruleId: string | number | undefined, - sortOrder: SortOrder, - environments: string[] = [], - kind: string = 'all' - ) => ['tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments] as const, + membersByTagAndRule: ( + tagId: string | number, + ruleId: string | number | undefined, + sortOrder: SortOrder, + environments: string[] = [], + kind: string = 'all' + ) => [...privilegeZonesKeys.members(), 'tag', tagId, 'rule', ruleId, kind, sortOrder, ...environments] as const,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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.
📚 Learning: 2025-07-17T16:28:00.770Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/views/Explore/GraphView.tsx:155-159
Timestamp: 2025-07-17T16:28:00.770Z
Learning: In the BloodHound codebase GraphView.tsx, the context menu toggle logic that switches between null and new coordinates (closing on different item clicks) is intentional, expected behavior as of the current implementation, though this may change in the future.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.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/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/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/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
📚 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/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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/Details/TagList.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/hooks/useAssetGroupTags/useAssetGroupTags.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 version of react-query used in this codebase doesn't have typeguard support for itemId when used in the enabled field, so type assertions may still be necessary even with enabled checks in place.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/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/PrivilegeZones/utils.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/PrivilegeZones/DetailsV2/Details.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
🧬 Code graph analysis (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (11)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/searchParams/useAppNavigate.ts (1)
useAppNavigate(27-42)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(41-56)packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
privilegeZonesPath(22-22)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useRuleMembersInfiniteQuery(308-335)useTagMembersInfiniteQuery(264-281)packages/javascript/bh-shared-ui/src/components/InfiniteQueryFixedList/InfiniteQueryFixedList.tsx (2)
InfiniteQueryFixedListProps(23-32)InfiniteQueryFixedList(34-98)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(179-181)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/components/ColumnHeaders/ColumnHeaders.tsx (1)
SortableHeader(60-106)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
getListHeight(36-41)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (4)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(179-181)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/utils.ts (1)
PAGE_SIZE(21-21)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTag(138-149)AssetGroupTagSelector(189-199)SeedTypes(164-164)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (5)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(498-503)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/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
⏰ 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 (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
84-88: Loading skeleton usage looks goodInline
ItemSkeletonwith three explicit instances under the loading branch is simple and avoids the earlier key misuse; no issues here.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
241-281:kindandenvironmentsplumbing through member queries looks correctThe new optional
environments/kindparameters are consistently threaded from:
useTagMembersInfiniteQuery/useRuleMembersInfiniteQuery- through the updated query keys
- into
getAssetGroupTagMembers/getAssetGroupTagRuleMembers- and finally to the
apiClientcalls.This keeps existing call sites backward‑compatible while enabling kind‑scoped member fetching.
Also applies to: 283-335
There was a problem hiding this comment.
Actionable comments posted: 0
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/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
314-337: Critical: Same query key and API call parameter mismatch.This function has the same issue as
getAssetGroupTagMembers: the optionalprimary_kindparameter will beundefinedwhen not provided, but the query key functionmembersByTagAndRule(line 94) defaults it to'all', creating a cache mismatch.🔎 Apply this diff to align the default values:
export const getAssetGroupTagRuleMembers = ( tagId: number | string, ruleId: number | string, skip: number = 0, limit: number = PAGE_SIZE, sortOrder: SortOrder = 'asc', environments?: string[], - primary_kind?: string + primary_kind: string = 'all' ) =>
265-286: Alignprimary_kinddefault between query key and fetcher to prevent cache mismatch.The query key functions (
membersByTagandmembersByTagAndRule) defaultprimary_kindto'all', but the fetcher functions (getAssetGroupTagMembersandgetAssetGroupTagRuleMembers) have no default, passingundefinedto the API. This creates a cache mismatch when the hooks are called withoutprimary_kind.Apply this diff:
export const getAssetGroupTagMembers = ( tagId: number | string, skip = 0, limit = PAGE_SIZE, sortOrder: SortOrder = 'asc', environments?: string[], - primary_kind?: string + primary_kind: string = 'all' ) =>and similarly for
getAssetGroupTagRuleMembersat line 321:export const getAssetGroupTagRuleMembers = ( tagId: number | string, ruleId: number | string, skip: number = 0, limit: number = PAGE_SIZE, sortOrder: SortOrder = 'asc', environments?: string[], - primary_kind?: string + primary_kind: string = 'all' ) =>
♻️ Duplicate comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (3)
36-41: Fix parameter typo and simplify logic.The parameter name has a typo (
windoHeightshould bewindowHeight), and the conditional logic can be simplified:-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) => { + if (windowHeight > 1080) return 760; + if (windowHeight > 900) return 640; + return 436; +};
22-28: Type guards will throw on undefined or primitive inputs.Both
isTagandisRuleuse theinoperator without first validating thatdatais a non-null object. This will throw aTypeErrorifdataisundefined,null, or a primitive value.Add proper type narrowing before using the
inoperator:-export const isTag = (data: any): data is AssetGroupTag => { - return 'kind_id' in data; -}; - -export const isRule = (data: any): data is AssetGroupTagSelector => { - return 'is_default' in data; -}; +export const isTag = (data: unknown): data is AssetGroupTag => + typeof data === 'object' && data !== null && 'kind_id' in data; + +export const isRule = (data: unknown): data is AssetGroupTagSelector => + typeof data === 'object' && data !== null && 'is_default' in data;
30-34: Array access without bounds checking.
getRuleSeedTypeaccessesrule.seeds[0]without verifying the array is non-empty, which will throw ifseedsis empty.Add a defensive check:
export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => { const firstSeed = rule.seeds[0]; + if (!firstSeed) { + throw new Error('AssetGroupTagSelector has no seeds'); + } return firstSeed.type; };
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx (3)
43-58: Refactor to use resilient queries instead of direct childNodes access.Accessing
accordion.childNodes[0],[1],[2]directly couples the test to the exact DOM structure. If the component adds wrapper elements or conditional fragments, the indices will shift and the test will break.Consider using semantic queries that verify order without relying on DOM implementation details:
🔎 Suggested approach using getAllByRole or getAllByText
- // Accessing these child nodes at 0, 1, and 2 indices tests the render order of the elements - // The elements should be rendered in alphabetical order even though the testKindCounts is not ordered that way - const accordion = screen.getByTestId('privilege-zones_details_objects-accordion'); - const computerAccordionItem = accordion.childNodes[0]; - const groupAccordionItem = accordion.childNodes[1]; - const userAccordionItem = accordion.childNodes[2]; - - expect(computerAccordionItem).not.toBeUndefined(); - expect(within(computerAccordionItem as HTMLElement).getByText('Computer')).toBeInTheDocument(); - - expect(groupAccordionItem).not.toBeUndefined(); - expect(within(groupAccordionItem as HTMLElement).getByText('Group')).toBeInTheDocument(); - - expect(userAccordionItem).not.toBeUndefined(); - expect(within(userAccordionItem as HTMLElement).getByText('User')).toBeInTheDocument(); + // Verify alphabetical order by checking the order of accordion item labels + const accordion = screen.getByTestId('privilege-zones_details_objects-accordion'); + const accordionItems = within(accordion).getAllByRole('button'); // or getAllByText if buttons aren't used + + expect(accordionItems[0]).toHaveTextContent('Computer'); + expect(accordionItems[1]).toHaveTextContent('Group'); + expect(accordionItems[2]).toHaveTextContent('User');
27-27: Remove unnecessaryasynckeyword.The test function is marked
asyncbut contains noawaitstatements or asynchronous operations. Theasynckeyword is unnecessary here.🔎 Apply this diff
- it('renders', async () => { + it('renders', () => {
20-59: Consider adding tests for user interactions and edge cases.The current test provides good baseline coverage for rendering and alphabetical ordering. To improve test robustness, consider adding test cases for:
- Accordion expansion/collapse behavior
- Empty state (
kindCounts={},totalCount=0)- Loading states (if applicable)
- Click interactions and navigation to object details (mentioned in AI summary)
- Edge cases like single object kind or very large counts
These additions would help catch regressions in interactive and boundary behaviors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(6 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
📚 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/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/Details/SearchBar.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/PrivilegeZones/Details/SearchBar.tsx
📚 Learning: 2025-11-06T21:23:30.689Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.689Z
Learning: In ExploreSearchCombobox component (packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx), setting aria-autocomplete={null} on the TextField is intentional to override downshift's default aria-autocomplete attribute, which was flagged by Axe DevTools. This approach satisfies Axe accessibility requirements by removing the conflicting attribute.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.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-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 version of react-query used in this codebase doesn't have typeguard support for itemId when used in the enabled field, so type assertions may still be necessary even with enabled checks in place.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/PrivilegeZones/utils.tsx
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (4)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(179-181)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/utils.ts (1)
PAGE_SIZE(21-21)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTag(140-151)AssetGroupTagSelector(191-201)SeedTypes(166-166)
⏰ 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: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (7)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx (1)
30-30: LGTM! Import path correctly updated.The import path change from
./utilsto../utilscorrectly reflects the consolidation of utilities into the parent PrivilegeZones directory.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (3)
38-39: LGTM: Clean placeholder implementation.The
EmptyHeadercomponent correctly serves as a no-op default for theInfoHeaderfield.
53-59: LGTM: Default value and context creation are correct.The default value properly implements only the required fields from
PrivilegeZonesContextValue, and the context is created using standard React patterns.
40-51: Type-usage mismatch forCertification(and potentiallySummary).The interface types
CertificationandSummaryasReact.LazyExoticComponent<React.FC>, but in DynamicDetails.tsx they are used only as existence checks (lines 93, 115, 135, 155), not as rendered components. If these should be lazy-loaded components, they need to be invoked as<Certification />. If they're intended to be feature flags, the type should reflect that (e.g.,booleanorundefined). Verify the intended usage pattern and align the types accordingly.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
83-95: Query key structure looks good, but verify default value handling.The addition of
primary_kindto the query keys is properly structured and will ensure correct cache separation. However, the default value of'all'in these query key functions may cause a mismatch with the API calls if the fetcher functions passundefinedinstead. See the comment on lines 265-286 for details.
288-312: Hook implementation is well-structured.The
useTagMembersInfiniteQueryhook correctly propagates theprimary_kindparameter through both the query key and the query function. Once the underlying fetcher function (getAssetGroupTagMembers) is updated with a matching default value, this implementation will work correctly.
339-366: Hook implementation is well-structured.The
useRuleMembersInfiniteQueryhook correctly propagates theprimary_kindparameter through both the query key and the query function. Once the underlying fetcher function (getAssetGroupTagRuleMembers) is updated with a matching default value, this implementation will work correctly.
|
Ticket shows icons next to each user/node - but they do not render locally. |
That's true the list items themselves are not rendering the icons. The mock is out of date but ticket specifies that the icons are only needed in the headers in the text of the requirements. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (2)
22-28: Type guards can throw on undefined or non-object inputs.Both
isTagandisRuleuse theinoperator without first ensuringdatais a non-null object. This will throw aTypeErrorifdataisundefined,null, or a primitive value.🔎 Harden the type guards
-export const isTag = (data: any): data is AssetGroupTag => { - return 'kind_id' in data; -}; - -export const isRule = (data: any): data is AssetGroupTagSelector => { - return 'is_default' in data; -}; +export const isTag = (data: unknown): data is AssetGroupTag => + typeof data === 'object' && data !== null && 'kind_id' in data; + +export const isRule = (data: unknown): data is AssetGroupTagSelector => + typeof data === 'object' && data !== null && 'is_default' in data;
30-41:getRuleSeedTypeassumes non-empty seeds array andgetListHeighthas a typo.Line 31 accesses
rule.seeds[0]without checking if the array is empty, which will throw ifseedsis empty. Additionally, the parameter namewindoHeighton Line 36 has a typo.🔎 Add defensive check and fix typo
export const getRuleSeedType = (rule: AssetGroupTagSelector): SeedTypes => { const firstSeed = rule.seeds[0]; + if (!firstSeed) { + throw new Error('AssetGroupTagSelector has no seeds'); + } return firstSeed.type; }; -export const getListHeight = (windoHeight: number) => { - if (windoHeight > 1080) return 760; - if (1080 >= windoHeight && windoHeight > 900) return 640; - if (900 >= windoHeight) return 436; +export const getListHeight = (windowHeight: number) => { + if (windowHeight > 1080) return 760; + if (windowHeight > 900) return 640; return 436; };packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (2)
27-29: Previously flagged: Fix tagId/zoneId fallback logic.This issue was already identified in a previous review. The destructuring default
zoneId = topTagId?.toString()won't execute ifusePZPathParams()returns an empty string (instead ofundefined), preventing the fallback to the highest-privilege tag.
41-45: Previously flagged: Non-deterministic placeholder data.This issue was already identified in a previous review. Using
Math.random()in the render body generates new values on every render, causing inconsistent UI and potential testing issues.
🧹 Nitpick comments (6)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
270-271: Consider using a more specific type forprimary_kind.Currently
primary_kindis typed asstring, which allows any value. If the valid values are known (e.g.,'all' | 'user' | 'computer' | 'group'), using a union type would provide better type safety and IDE autocomplete.Example:
type PrimaryKind = 'all' | 'user' | 'computer' | 'group'; // Then update signatures like: export const getAssetGroupTagMembers = ( tagId: number | string, skip = 0, limit = PAGE_SIZE, sortOrder: SortOrder = 'asc', environments?: string[], primary_kind: PrimaryKind = 'all' ) => // ...Also applies to: 291-292, 320-321, 343-344
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
65-65: Ensure placeholder data is replaced with API values.The hardcoded domain
"TITANCORP.LOCAL"(line 65) andtotalCount={777}(line 75) are placeholder values. Similar to thekindCountsTODO on line 41, these should be replaced with actual data from the API in the final implementation.Also applies to: 75-75
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tag.test.tsx (2)
45-45: Consider improving type safety.Using
anybypasses TypeScript's type checking. If the actual utils module has exported types, consider using them for better type safety.🔎 Suggested refactor
- const actual = await vi.importActual<any>('../../../utils'); + const actual = await vi.importActual('../../../utils');Note: TypeScript can often infer the type automatically. If explicit typing is needed, consider importing and using the actual module type.
1-130: Consider consolidating test setup across tag and rule test files.Both
ObjectsAccordion.tag.test.tsxandObjectsAccordion.rule.test.tsxshare approximately 90% of their code (imports, mocks, server setup). The primary differences are:
- The
ruleIdvalue in path params (undefined vs 777)- The spy target (
useTagMembersInfiniteQueryvsuseRuleMembersInfiniteQuery)- The
objectDetailsLinksignatureConsider extracting common test utilities or creating a parameterized test helper to reduce duplication and improve maintainability.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.rule.test.tsx (2)
45-45: Consider improving type safety.Using
anybypasses TypeScript's type checking. If the actual utils module has exported types, consider using them for better type safety.🔎 Suggested refactor
- const actual = await vi.importActual<any>('../../../utils'); + const actual = await vi.importActual('../../../utils');Note: TypeScript can often infer the type automatically. If explicit typing is needed, consider importing and using the actual module type.
62-98: Add comprehensive rendering test for consistency.The tag test file (
ObjectsAccordion.tag.test.tsx) includes a detailed rendering test (lines 68-99) that verifies:
- All text elements render correctly
- Accordion items are sorted alphabetically
- DOM structure matches expectations
Consider adding a similar rendering test to this rule test file to maintain test coverage consistency across both scenarios.
Example test to add
it('renders', async () => { render(<ObjectsAccordion kindCounts={testKindCounts} totalCount={15} />); expect(screen.getByText('Objects')).toBeInTheDocument(); expect(screen.getByText('Total Objects:')).toBeInTheDocument(); expect(screen.getByText('15')).toBeInTheDocument(); expect(screen.getByText('User')).toBeInTheDocument(); expect(screen.getByText('4')).toBeInTheDocument(); expect(screen.getByText('Computer')).toBeInTheDocument(); expect(screen.getByText('5')).toBeInTheDocument(); expect(screen.getByText('Group')).toBeInTheDocument(); expect(screen.getByText('6')).toBeInTheDocument(); // Verify alphabetical ordering const accordion = screen.getByTestId('privilege-zones_details_objects-accordion'); const computerAccordionItem = accordion.childNodes[0]; const groupAccordionItem = accordion.childNodes[1]; const userAccordionItem = accordion.childNodes[2]; expect(computerAccordionItem).not.toBeUndefined(); expect(within(computerAccordionItem as HTMLElement).getByText('Computer')).toBeInTheDocument(); expect(groupAccordionItem).not.toBeUndefined(); expect(within(groupAccordionItem as HTMLElement).getByText('Group')).toBeInTheDocument(); expect(userAccordionItem).not.toBeUndefined(); expect(within(userAccordionItem as HTMLElement).getByText('User')).toBeInTheDocument(); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/javascript/bh-shared-ui/src/components/ExploreTable/ExploreTable.test.tsxpackages/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/Details/Details.test.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/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/ObjectsAccordion.rule.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tag.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.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/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/javascript/js-client-library/src/client.ts
💤 Files with no reviewable changes (3)
- 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
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SearchBar.tsx
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/RulesList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
🧰 Additional context used
🧠 Learnings (13)
📚 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/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.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/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.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/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.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/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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.ts
📚 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 version of react-query used in this codebase doesn't have typeguard support for itemId when used in the enabled field, so type assertions may still be necessary even with enabled checks in place.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsxpackages/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/PrivilegeZones/utils.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/PrivilegeZones/DetailsV2/Details.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-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.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/Details/TagList.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-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 (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tag.test.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useAssetGroupTags(494-504)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(37-62)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/utils.tsx (1)
packages/javascript/js-client-library/src/types.ts (3)
AssetGroupTag(140-151)AssetGroupTagSelector(191-201)SeedTypes(166-166)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (4)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(186-188)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/History/utils.ts (1)
PAGE_SIZE(21-21)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.rule.test.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useAssetGroupTags(494-504)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(37-62)
⏰ 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: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (20)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx (1)
27-29: Import paths correctly refactored and validated.The changes update import paths to more specific locations:
AppIconto'../../components/AppIcon'andcnto'../../utils'. Both modules exist at the new locations and are properly exported. No old import patterns remain in the PrivilegeZones directory. The refactoring is clean and consistent.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
17-17: LGTM! Skeleton rendering and import paths updated correctly.The changes successfully address the previous feedback:
ItemSkeletonis now a proper React component without the key parameter issue- Hard-coded rendering of three skeleton items is cleaner than the array approach
- Import path consolidation to
../utilsaligns with the broader refactorAlso applies to: 26-26, 29-37, 85-89
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
34-34: LGTM! Import path updated to reflect utils consolidation.The change from
'./utils'to'../utils'aligns with the broader refactor that consolidates PrivilegeZones utilities.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx (1)
110-110: LGTM! Test id updated to match component changes.The data-testid correctly reflects the new
ItemSkeletoncomponent rendering inTagList.tsx.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
83-95: LGTM! Query keys properly updated to includeprimary_kind.The
primary_kindparameter is correctly integrated into bothmembersByTagandmembersByTagAndRulequery key factories with a sensible default of'all'. The placement between the identifier andsortOrderensures proper cache segregation.
265-286: LGTM! Tag members fetching correctly threadsprimary_kind.The
primary_kindparameter is consistently:
- Accepted with optional typing and
'all'default- Included in the query key for proper caching
- Passed through to the API client call
Also applies to: 288-312
314-337: LGTM! Rule members fetching correctly threadsprimary_kind.The implementation mirrors the tag members pattern, ensuring consistency across the codebase. The parameter flows correctly from hook → query key → fetcher → API client.
Also applies to: 339-366
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/index.tsx (1)
16-18: LGTM!Standard barrel export pattern for the DetailsV2 module. This enables cleaner imports from parent components.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (3)
17-24: LGTM!Imports are appropriate for the component's functionality. The addition of
AssetGroupTag,ActiveDirectoryNodeKind, anduseAppNavigatesupports the new navigation and ObjectsAccordion integration.
47-49: LGTM!The
handleZoneClickhandler is properly typed and uses thetagDetailsLinkbuilder from path params for consistent navigation.
63-63: LGTM!The conditional rendering of
ZoneSelectorwith theonZoneClickhandler follows React best practices for optional context-provided components.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
49-51: Verify the import swap is reverted or finalized before merging.The TODO comment and your prior note indicate this is temporary development code. Ensure the final decision (keep DetailsV2 or revert to Details) is implemented before merging to main.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (1)
118-118: Test expectations correctly updated to match new labeling convention.Lines 118 and 137 correctly expect the new
tag-0-*label format. The mock data factory inprivilegeZones.tsgenerates object and rule names using thetag-${tagId - 1}-*pattern, confirming these test expectations align with the component's data.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tag.test.tsx (3)
68-99: Comprehensive rendering test with alphabetical order verification.The test thoroughly validates:
- All expected text elements render correctly
- Accordion items are sorted alphabetically (Computer, Group, User) regardless of input order
- Uses DOM structure assertions to verify render order
This is a well-structured test that covers both content and ordering behavior.
101-115: Effective sorting interaction test.The test correctly verifies that:
- Clicking the sort button toggles between 'desc' and 'asc' orders
- The spy confirms correct parameters are passed to
useTagMembersInfiniteQuery
117-129: Navigation test validates user interaction flow.The test properly verifies the complete interaction: expanding the accordion, waiting for list items to load, clicking an object row, and asserting the correct navigation URL is called.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.rule.test.tsx (2)
69-83: Sorting interaction test with rule context.The test correctly verifies sorting behavior specific to rules by:
- Validating
useRuleMembersInfiniteQueryis called with proper parameters includingtagId: 42andruleId: 777- Testing both 'desc' and 'asc' sort orders
85-97: Navigation test validates rule-specific URL structure.The test properly verifies navigation to the rule-specific object details URL (
/tags/42/rules/777/objects/1), confirming the component correctly handles the rule context.packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts (2)
140-141: Consistent terminology update across conditional branches.Both branches of the conditional correctly use the "tag-" prefix, maintaining consistency with the change at line 76.
76-76: Mock factory naming update properly synchronized with tests.The change from "tier" to "tag" prefix in mock data generation is correctly reflected in dependent tests. Test files including
Details.test.tsx,ObjectsAccordion.rule.test.tsx, andObjectsAccordion.tag.test.tsxall expect the new "tag-" naming patterns, confirming the factory and tests have been updated together.
|
|
||
| import userEvent from '@testing-library/user-event'; | ||
| import { setupServer } from 'msw/node'; | ||
| import { beforeEach, describe, expect, it } from 'vitest'; |
There was a problem hiding this comment.
Add missing Vitest lifecycle imports.
The test file uses beforeAll, afterEach, and afterAll on lines 55-57 but only imports beforeEach, describe, expect, it from vitest. This will cause runtime errors.
🔎 Proposed fix
-import { beforeEach, describe, expect, it } from 'vitest';
+import { beforeAll, beforeEach, afterEach, afterAll, describe, expect, it } from 'vitest';📝 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.
| import { beforeEach, describe, expect, it } from 'vitest'; | |
| import { beforeAll, beforeEach, afterEach, afterAll, describe, expect, it } from 'vitest'; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.rule.test.tsx
around line 19, the test file calls beforeAll, afterEach, and afterAll but only
imports beforeEach, describe, expect, it from vitest; update the import
statement to also include beforeAll, afterEach, and afterAll so the lifecycle
hooks are available at runtime.
|
|
||
| import userEvent from '@testing-library/user-event'; | ||
| import { setupServer } from 'msw/node'; | ||
| import { beforeEach, describe, expect, it } from 'vitest'; |
There was a problem hiding this comment.
Add missing Vitest lifecycle imports.
The test file uses beforeAll, afterEach, and afterAll on lines 55-57 but only imports beforeEach, describe, expect, it from vitest. This will cause runtime errors.
🔎 Proposed fix
-import { beforeEach, describe, expect, it } from 'vitest';
+import { beforeAll, beforeEach, afterEach, afterAll, describe, expect, it } from 'vitest';📝 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.
| import { beforeEach, describe, expect, it } from 'vitest'; | |
| import { beforeAll, beforeEach, afterEach, afterAll, describe, expect, it } from 'vitest'; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tag.test.tsx
around line 19, the test imports only beforeEach, describe, expect, it from
vitest but uses beforeAll, afterEach, and afterAll later; update the import
statement to also include beforeAll, afterEach, and afterAll so the lifecycle
hooks used on lines ~55-57 are available at runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (1)
67-68: ThezoneIdfallback totopTagIdstill does not work as intended.Per the previous review comment,
usePZPathParams()returnszoneId: ''(empty string) by default, notundefined. Destructuring defaults only trigger onundefined, sozoneId = topTagId?.toString()never executes when the URL has no zone ID.🔎 Recommended fix
- const { zoneId = topTagId?.toString(), tagTypeDisplay, tagId: defaultTagId, tagDetailsLink } = usePZPathParams(); - const tagId = !defaultTagId ? zoneId : defaultTagId; + const { zoneId, tagTypeDisplay, tagId: pathTagId, tagDetailsLink } = usePZPathParams(); + const effectiveZoneId = zoneId || topTagId?.toString() || ''; + const tagId = pathTagId || effectiveZoneId;
🧹 Nitpick comments (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (4)
28-37: Consider rejecting with Error objects for better debugging.Rejecting with plain strings loses stack trace information. Using
Errorobjects provides better debugging context when errors propagate.🔎 Suggested improvement
queryFn: async ({ signal }) => { - if (!tagId) return Promise.reject('No Tag ID available for tag counts request'); + if (!tagId) return Promise.reject(new Error('No Tag ID available for tag counts request')); return apiClient.getAssetGroupTagMembersCount(tagId, environments, { signal }).then((res) => res.data.data); },
39-51: Placeholder endpoint noted; offer to track.The TODO at line 46-47 indicates this is using a placeholder endpoint until the actual rule counts API is available. The same Error object suggestion from the previous hook applies here.
Would you like me to open an issue to track the replacement of this placeholder with the actual
getAssetGroupTagRuleMembersCountendpoint?
110-114: Consider handling loading state for better UX.When
objectCountsQueryis loading, the component renders withkindCounts: {}andtotalCount: 0, displaying "Total Objects: 0" which may mislead users. Consider rendering a skeleton or loading indicator.🔎 Suggested approach
+ {objectCountsQuery.isLoading ? ( + <div className='p-4'>Loading...</div> + ) : ( <ObjectsAccordion kindCounts={objectCountsQuery.data?.counts || {}} totalCount={objectCountsQuery.data?.total_count || 0} /> + )}Or pass the loading state to
ObjectsAccordionto render appropriate skeleton UI internally.
106-108: Empty placeholder list noted.The empty
<ul>withh-dvh overflow-y-scrollis presumably a placeholder for future content. Theh-dvh(dynamic viewport height) may cause unexpected layout when the list is empty.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsxpackages/javascript/js-client-library/src/client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 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.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.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.tsx
📚 Learning: 2025-09-08T20:04:09.070Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/QuerySearchFilter.tsx:110-116
Timestamp: 2025-09-08T20:04:09.070Z
Learning: In BloodHound's TypeScript codebase, jvacca-specterops considers runtime guards for type assertions unnecessary when the UI state already prevents the problematic scenario (e.g., disabled buttons when required data is unavailable), preferring to rely on the existing disabled state logic rather than adding additional runtime checks.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.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/PrivilegeZones/DetailsV2/Details.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
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx (7)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(41-56)packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
privilegeZonesPath(22-22)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(527-532)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/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/ObjectsAccordion.tsx (1)
ObjectsAccordion(38-63)
⏰ 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/DetailsV2/Details.tsx (3)
17-26: LGTM!Imports are well-organized and all appear to be utilized in the component.
53-63: LGTM!The hook cleanly encapsulates the conditional logic for choosing between tag and rule counts based on the current route.
99-101: Verify if hardcoded domain is intentional.
TITANCORP.LOCALappears to be a hardcoded placeholder. Is this meant to be replaced with dynamic environment/domain data?
| const context = useContext(PrivilegeZonesContext); | ||
| if (!context) { | ||
| throw new Error('Details must be used within a PrivilegeZonesContext.Provider'); | ||
| } | ||
| const { InfoHeader, ZoneSelector } = context; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the PrivilegeZonesContext file
fd "PrivilegeZonesContext" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ --type fRepository: SpecterOps/BloodHound
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Examine the PrivilegeZonesContext file
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsxRepository: SpecterOps/BloodHound
Length of output: 2450
🏁 Script executed:
#!/bin/bash
# Check how the Details component is used and if it's always wrapped by a Provider
rg -B 5 -A 5 "Details must be used within a PrivilegeZonesContext.Provider" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Repository: SpecterOps/BloodHound
Length of output: 2569
🏁 Script executed:
#!/bin/bash
# Check where PrivilegeZonesContext.Provider is used
rg "PrivilegeZonesContext\.Provider" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ -B 3 -A 3Repository: SpecterOps/BloodHound
Length of output: 2383
🏁 Script executed:
#!/bin/bash
# Search for where PrivilegeZonesContext is being provided (value prop)
rg "PrivilegeZonesContext" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ -t tsx -t ts | grep -i "value\|provider"Repository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for where PrivilegeZonesContext.Provider is being used with a value prop
rg "value=" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ -A 2 -B 2 | grep -i "privilege\|context" | head -50Repository: SpecterOps/BloodHound
Length of output: 7502
🏁 Script executed:
#!/bin/bash
# Search for the actual Provider element rendering
rg "PrivilegeZonesContext.Provider" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ -B 5 -A 5Repository: SpecterOps/BloodHound
Length of output: 3880
🏁 Script executed:
#!/bin/bash
# Search for files that import PrivilegeZonesContext and use it as a Provider
rg "import.*PrivilegeZonesContext" packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Repository: SpecterOps/BloodHound
Length of output: 1091
🏁 Script executed:
#!/bin/bash
# Check fragments.tsx which might have the Provider
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/fragments.tsxRepository: SpecterOps/BloodHound
Length of output: 4951
🏁 Script executed:
#!/bin/bash
# Search in parent directory for Provider implementation or entry point
fd -e "tsx" -e "ts" packages/javascript/bh-shared-ui/src/ | xargs rg "PrivilegeZonesContext" | grep -i "value\|provider" | head -20Repository: SpecterOps/BloodHound
Length of output: 1926
🏁 Script executed:
#!/bin/bash
# Check the PrivilegeZonesProvider implementation
cat -n cmd/ui/src/views/PrivilegeZones/PrivilegeZonesProvider.tsxRepository: SpecterOps/BloodHound
Length of output: 1364
Context check is ineffective at detecting missing Provider.
useContext returns the default value (defaultPrivilegeZoneCtxValue, a truthy object) when no Provider exists, not undefined. The check if (!context) will never trigger this error. The component will silently use default values (e.g., InfoHeader: EmptyHeader) instead of requiring a Provider.
If enforcing a Provider is necessary, either:
- Set the default context value to
nulland update the type toPrivilegeZonesContextValue | null - Add a sentinel property to the default that indicates "not provided"
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/DetailsV2/Details.tsx
around lines 72–76, the current check if (!context) is ineffective because
useContext returns the default object when no Provider exists; change the
default context to null and the context type to PrivilegeZonesContextValue |
null so useContext returns null when no Provider is present, then update the
runtime check to if (context === null) throw new Error('Details must be used
within a PrivilegeZonesContext.Provider'); alternatively (if you prefer not to
use null) add a clear sentinel property (e.g., __isProvided = true/false) to the
default value and change the check to if (!context.__isProvided) throw, and
update the context creation and types accordingly.
Description
A redesigned Objects display is implemented for the Details page redesign.
Motivation and Context
Resolves BED-6844
Why is this change required? What problem does it solve?
How Has This Been Tested?
Render unit test added to the new component added.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.