feat(PZ): Added Selected Details Tabs BED-6836 #2189
Conversation
Co-authored-by: Nicole Martin <[email protected]>
WalkthroughAdds a tabbed details UI for Privilege Zones with context-based tab state, new tab components and provider, conditional rendering of tag/object/rule content, updated DynamicDetails to accept a hasObjectCountPanel prop and use ZoneIcon, and extends mocks with a cypher-ready rule factory. Tests added/updated for tabs and conditional rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as PrivilegeZones (with Provider)
participant TabsCtx as SelectedDetailsTabsContext
participant TabsUI as SelectedDetailsTabs (Tab UI)
participant Content as SelectedDetailsTabContent
participant Dynamic as DynamicDetails
participant Entity as EntityInfoPanel
User->>App: Open Privilege Zone details
App->>TabsCtx: Provider initializes (default: tag)
TabsUI->>TabsCtx: Read selectedDetailsTab
TabsUI->>User: Render tabs (Tag, Rule, Object)
User->>TabsUI: Click "Rule" tab
TabsUI->>TabsCtx: setSelectedDetailsTab("rule")
TabsCtx-->>Content: notify selectedDetailsTab change
Content->>Content: determine active tab and ids
alt Rule tab active
Content->>Dynamic: fetch and render rule details (ruleId)
Dynamic-->>User: show rule details (cypher preview if present)
else Object tab active
Content->>Entity: fetch member and render EntityInfoPanel
Entity-->>User: show member info + rules table
else Tag tab active
Content->>Dynamic: fetch and render tag details (tagId, hasObjectCountPanel)
Dynamic-->>User: show tag details (conditionally show object count)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsProvider.tsx (1)
21-30: Consider memoizing the context value.The
contextValueobject is recreated on every render. For a provider that may wrap a large subtree, consider usinguseMemoto prevent unnecessary re-renders of consumers when the state hasn't changed.🔎 View suggested optimization
-import { useState } from 'react'; +import { useMemo, useState } from 'react'; import { DetailsTabOption, TabTagOption } from '../utils'; import { SelectedDetailsTabsContext } from './SelectedDetailsTabsContext'; const SelectedDetailsTabsProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const [selectedDetailsTab, setSelectedDetailsTab] = useState<DetailsTabOption>(TabTagOption); - const contextValue = { + const contextValue = useMemo(() => ({ selectedDetailsTab, setSelectedDetailsTab, - }; + }), [selectedDetailsTab]); return <SelectedDetailsTabsContext.Provider value={contextValue}>{children}</SelectedDetailsTabsContext.Provider>; };packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsx (2)
29-30: Hardcoded dimensions acknowledged but should be addressed.The TODO comment indicates awareness of the hardcoded
w-[400px] h-[625px]dimensions. Consider using a more flexible approach likeflexproperties or CSS variables that can adapt to different viewport sizes or content needs.
47-59: Consider adding an error boundary around Suspense.While the Suspense component handles loading states, it doesn't catch errors from the async content. Consider wrapping this in an ErrorBoundary to gracefully handle query failures from
SelectedDetailsTabContent.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx (1)
38-42: Consider conditional query execution for better performance.All three queries execute on every render regardless of which tab is active. While React Query handles inactive queries efficiently, you could improve initial render performance by conditionally calling these hooks or using the
enabledoption.🔎 View suggested optimization
- const tagQuery = useAssetGroupTagInfo(tagId); + const tagQuery = useAssetGroupTagInfo(tagId, { enabled: currentDetailsTab === TabTagOption }); - const ruleQuery = useRuleInfo(tagId, ruleId); + const ruleQuery = useRuleInfo(tagId, ruleId, { enabled: currentDetailsTab === TabRuleOption && !!ruleId }); - const memberQuery = useMemberInfo(tagId, memberId); + const memberQuery = useMemberInfo(tagId, memberId, { enabled: currentDetailsTab === TabObjectOption && !!memberId });Note: Verify that these hooks support the
enabledoption.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx (1)
1-2: Minor formatting inconsistency in copyright header.Line 2 has the copyright text without the comment prefix on a separate line. While this doesn't affect functionality, it's inconsistent with other files in the PR.
🔎 Apply this diff for consistency
// Copyright 2025 Specter Ops, Inc. - +// // Licensed under the Apache License, Version 2.0
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/ui/src/views/PrivilegeZones/PrivilegeZones.tsx(1 hunks)packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/NewDetails.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsProvider.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/SelectedDetailsTabs/index.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsProvider.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/NewDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxcmd/ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts
📚 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/SelectedDetailsTabs/index.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxcmd/ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts
📚 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/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts
🧬 Code graph analysis (7)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.test.tsx (1)
packages/javascript/js-client-library/src/types.ts (2)
AssetGroupTagType(122-125)AssetGroupTag(140-151)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/NewDetails.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(495-500)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (2)
DetailsTabOption(37-37)TabTagOption(33-33)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.test.tsx (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx (1)
SelectedDetailsTabContent(32-71)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (3)
TabTagOption(33-33)TabRuleOption(34-34)TabObjectOption(35-35)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(140-151)
packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts (2)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTagSelector(191-201)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
createRule(347-353)
⏰ 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 (18)
packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts (1)
102-109: Potential inconsistency: hardcodedselector_iddoesn't matchruleIdparameter.The hardcoded seed has
selector_id: 2, but the function accepts aruleIdparameter that becomes the selector'sid. This creates a mismatch between the selector's ID and the seed'sselector_id.Looking at
createRuleSeeds(line 127), seeds typically useselector_id: ruleIdto maintain referential consistency. If this hardcoded value is intentional for specific test scenarios, please verify that it won't cause issues with any validation logic that expects seeds to reference their parent selector.🔎 Consider using `ruleId` for consistency:
export const createRuleWithCypher = (tagId: number = 0, ruleId: number = 0) => { const data: AssetGroupTagSelector = { ...createRule(tagId, ruleId), - seeds: [{ selector_id: 2, type: 2, value: '9a092ad2-3114-40e7-9bb6-6e47944ad83c' }], + seeds: [{ selector_id: ruleId, type: 2, value: '9a092ad2-3114-40e7-9bb6-6e47944ad83c' }], }; return data; };packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
33-37: LGTM! Clean tab option definitions.The use of
as constassertions for the string literals and the union type definition follow TypeScript best practices. This provides type safety for the tab selection system.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.test.tsx (2)
32-51: LGTM! Good refactor for test data reusability.Moving
testTagto module scope eliminates duplication and makes the test data available for multiple test cases.
82-95: LGTM! Proper test coverage for the new prop.The new tests correctly verify both the presence and absence of the ObjectCountPanel based on the
hasObjectCountPanelprop. The use offindByTestIdfor async presence andqueryByTestIdfor absence checks is appropriate.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
60-60: LGTM! Clean implementation of conditional ObjectCountPanel rendering.The changes correctly introduce an optional
hasObjectCountPanelprop that:
- Defaults to
falsefor backward compatibility- Is threaded from
DynamicDetailsthrough toTagDetails- Controls conditional rendering of
ObjectCountPanelat line 122The implementation is consistent and maintains the existing behavior for cases where the prop is not provided.
Also applies to: 122-122, 176-176, 179-182, 192-192
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsx (1)
56-56: LGTM! Appropriate use of the hasObjectCountPanel prop.The
hasObjectCountPanelprop is correctly passed toDynamicDetailsonly for the tag rendering path, which aligns with the requirement to show object counts for zone/label details.cmd/ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
16-16: LGTM! Proper provider setup for BHCE.The
SelectedDetailsTabsProvideris correctly imported and wrapped around thePrivilegeZonescomponent, establishing the context needed for the tab management functionality. The aliasPZDetailsTabsProvideris clear and avoids potential naming conflicts.Also applies to: 24-26
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/index.ts (1)
1-19: LGTM! Standard barrel export.This follows the established pattern for index exports in the codebase, making the
SelectedDetailsTabsmodule easier to import.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx (1)
19-19: LGTM! Public API extended with SelectedDetailsTabsProvider.The addition of
SelectedDetailsTabsProviderto the public exports enables consumers (likecmd/ui) to properly wrap thePrivilegeZonescomponent with the required context provider. This is a non-breaking additive change to the API.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (1)
47-48: Error handling for SelectedDetailsTabsContext is properly implemented.The
useSelectedDetailsTabsContexthook already throws a descriptive error when used outside its provider:"useSelectedDetailsTabsContext is outside of SelectedDetailsTabsContext". This implementation follows React context best practices and is actually more robust than the pattern used in PrivilegeZonesContext, as the error handling is encapsulated within the hook itself rather than requiring manual checks in consuming components.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/NewDetails.tsx (2)
21-24: LGTM - Planned integration is well-documented.The commented imports are properly documented with the ticket reference (BED-6836) and clear instructions for when to uncomment. This is appropriate scaffolding for the planned list interaction feature.
71-71: LGTM - Component integration is correct.The replacement of the old component with
SelectedDetailsTabsis cleanly implemented and aligns with the PR objectives.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx (1)
20-22: LGTM - Public API extension is correct.The import and export of
SelectedDetailsTabsProvidercorrectly extends the public API surface, making the provider available for use in higher-level components.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.test.tsx (1)
44-73: LGTM - Test coverage is comprehensive.The test suite properly covers all tab options (Tag, Rule, Object) and verifies correct rendering of content components. The use of MSW for API mocking and async queries is appropriate.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsx (1)
24-62: LGTM - Tab logic and accessibility are well-implemented.The component correctly manages tab state, properly disables tabs based on available data, and uses accessible ARIA attributes through the DoodleUI components.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx (1)
44-176: LGTM - Comprehensive test coverage for tab behavior.The test suite thoroughly covers:
- Tab selection and interaction via user events
- Tab enablement/disablement based on route params
- ARIA attributes and accessibility states
- Both Zone and Label view variations
The use of mock hooks and proper test wrappers ensures isolated, reliable tests.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx (1)
32-71: LGTM - Conditional rendering logic is clear and correct.The component appropriately handles three different tab states with proper fallback to
null. The use of EntityInfoPanel for object details and DynamicDetails for rule/tag details follows the established patterns.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx (1)
19-39: LGTM - Well-implemented context pattern with proper error handling.The context implementation follows React best practices:
- Typed interface for context value
- Sensible initial/default values
- Custom hook with runtime check for proper usage
- Clear error message when used outside provider
...d-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.test.tsx
Outdated
Show resolved
Hide resolved
| if (memberQuery?.data && currentDetailsTab === TabObjectOption) { | ||
| const selectedNode = { | ||
| id: memberQuery.data.object_id, | ||
| name: memberQuery.data.name, | ||
| type: memberQuery.data.primary_kind as EntityKinds, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find EntityKinds type definition
echo "=== Searching for EntityKinds definition ==="
rg -nP --type=ts 'export.*type EntityKinds|export.*enum EntityKinds' | head -20
echo -e "\n=== Searching for EntityKinds usage ==="
rg -nP --type=ts 'EntityKinds' | grep -E '(type|enum) EntityKinds' | head -10
echo -e "\n=== Searching for primary_kind field ==="
rg -nP --type=ts 'primary_kind' -C2 | head -40Repository: SpecterOps/BloodHound
Length of output: 3410
🏁 Script executed:
#!/bin/bash
# Find the definitions of the individual node kinds
echo "=== Searching for ActiveDirectoryNodeKind, AzureNodeKind, MetaNodeKind ==="
rg -nP --type=ts 'export.*type (ActiveDirectoryNodeKind|AzureNodeKind|MetaNodeKind)' | head -20
echo -e "\n=== Detailed search for their definitions ==="
rg -nP --type=ts '(ActiveDirectoryNodeKind|AzureNodeKind|MetaNodeKind)\s*=' | head -30
echo -e "\n=== Check the actual file context around line 44-49 ==="
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx | sed -n '35,60p'Repository: SpecterOps/BloodHound
Length of output: 1445
🏁 Script executed:
#!/bin/bash
# View the content.ts file around EntityKinds definition
echo "=== Viewing content.ts around EntityKinds definition ==="
cat -n packages/javascript/bh-shared-ui/src/utils/content.ts | sed -n '1,100p'
echo -e "\n=== Search for ActiveDirectoryNodeKind import/definition ==="
rg -nP --type=ts 'ActiveDirectoryNodeKind' packages/javascript/bh-shared-ui/src/utils/content.ts -B2 -A2
echo -e "\n=== Search for AzureNodeKind import/definition ==="
rg -nP --type=ts 'AzureNodeKind' packages/javascript/bh-shared-ui/src/utils/content.ts -B2 -A2Repository: SpecterOps/BloodHound
Length of output: 16725
🏁 Script executed:
#!/bin/bash
# Find graphSchema file and check ActiveDirectoryNodeKind and AzureNodeKind
echo "=== Searching for graphSchema file ==="
find packages/javascript/bh-shared-ui/src -name '*graphSchema*' -type f
echo -e "\n=== Check graphSchema definitions ==="
cat -n packages/javascript/bh-shared-ui/src/graphSchema.ts | head -150Repository: SpecterOps/BloodHound
Length of output: 6688
🏁 Script executed:
#!/bin/bash
# Find AzureNodeKind definition
echo "=== Check AzureNodeKind definition ==="
cat -n packages/javascript/bh-shared-ui/src/graphSchema.ts | sed -n '150,250p'
echo -e "\n=== Search for useMemberInfo hook ==="
rg -nP --type=ts 'useMemberInfo' packages/javascript/bh-shared-ui/src -A5 | head -50
echo -e "\n=== Check if there's validation for primary_kind anywhere ==="
rg -nP --type=ts 'primary_kind' packages/javascript/bh-shared-ui/src -B3 -A3 | grep -E '(validate|check|guard|filter|includes|Object\.values)' -A2 -B2Repository: SpecterOps/BloodHound
Length of output: 13768
🏁 Script executed:
#!/bin/bash
# Find AzureNodeKind definition
echo "=== Search for AzureNodeKind enum ==="
grep -n "export enum AzureNodeKind" packages/javascript/bh-shared-ui/src/graphSchema.ts -A50 | head -60
echo -e "\n=== Check entityInformationEndpoints usage in EntityInfoPanel ==="
rg -nP --type=ts 'entityInformationEndpoints\[' packages/javascript/bh-shared-ui/src -B2 -A2
echo -e "\n=== Verify if EntityInfoPanel handles invalid node types ==="
rg -nP --type=ts 'EntityInfoPanel' packages/javascript/bh-shared-ui/src -l | head -5Repository: SpecterOps/BloodHound
Length of output: 3211
🏁 Script executed:
#!/bin/bash
# Check how validatedKind is determined in useFetchEntityProperties
echo "=== Check useFetchEntityProperties hook for validation ==="
cat -n packages/javascript/bh-shared-ui/src/hooks/useFetchEntityProperties/useFetchEntityProperties.tsx | head -100
echo -e "\n=== Search for validatedKind definition ==="
rg -nP --type=ts 'validatedKind' packages/javascript/bh-shared-ui/src/hooks/useFetchEntityProperties/useFetchEntityProperties.tsx -B5 -A5Repository: SpecterOps/BloodHound
Length of output: 5099
🏁 Script executed:
#!/bin/bash
# Find validateNodeType function
echo "=== Find validateNodeType function ==="
rg -nP --type=ts 'export.*validateNodeType|function validateNodeType' packages/javascript/bh-shared-ui/src -B2 -A15
echo -e "\n=== Search in useSearch file ==="
rg -nP --type=ts 'validateNodeType' packages/javascript/bh-shared-ui/src/hooks/useSearch/useSearch.ts -B5 -A15Repository: SpecterOps/BloodHound
Length of output: 2121
🏁 Script executed:
#!/bin/bash
# Final verification: check EntityInfoPanel to see if it handles invalid types
echo "=== Check EntityInfoPanel usage ==="
cat -n packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx | head -80
echo -e "\n=== Verify the complete flow in SelectedDetailsTabContent ==="
cat -n packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx | sed -n '40,70p'
echo -e "\n=== Check if there are any type guards after the cast ==="
rg -nP --type=tsx 'type: memberQuery.data.primary_kind' packages/javascript/bh-shared-ui/src -A10 -B2Repository: SpecterOps/BloodHound
Length of output: 5416
🏁 Script executed:
#!/bin/bash
# Final verification with correct file type
echo "=== Check if there are any type guards after the cast ==="
rg -nP --type=ts 'type: memberQuery.data.primary_kind' packages/javascript/bh-shared-ui/src -A10 -B2
echo -e "\n=== Summary: Check EntityInfoContent to see validation ==="
cat -n packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx | head -100Repository: SpecterOps/BloodHound
Length of output: 5575
Add runtime validation before casting primary_kind to EntityKinds.
The cast on line 48 (memberQuery.data.primary_kind as EntityKinds) bypasses type safety without upfront validation. While validateNodeType() exists and performs validation downstream in useFetchEntityProperties, this creates a maintenance risk: the cast assumes the API returns valid enum values, but the validation is hidden and decoupled from where the assertion occurs. If validation logic changes or the component flow is refactored, invalid values could propagate silently.
Use the existing validateNodeType() function before the cast:
const validatedKind = validateNodeType(memberQuery.data.primary_kind);
const selectedNode = {
id: memberQuery.data.object_id,
name: memberQuery.data.name,
type: validatedKind || (memberQuery.data.primary_kind as EntityKinds),
};This couples validation to the assertion point, making the requirement explicit and safer.
Note: The same issue exists in SelectedDetails.tsx at line 37.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx
around lines 44 to 49, the code casts memberQuery.data.primary_kind directly to
EntityKinds without validating it; call
validateNodeType(memberQuery.data.primary_kind) first, store the result in a
variable (e.g., validatedKind) and use that for the selectedNode.type, falling
back to the original cast only if validation returns falsy; apply the same
change in
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsx
at line 37.
...-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
Outdated
Show resolved
Hide resolved
| return ( | ||
| <PrivilegeZonesProvider> | ||
| <PrivilegeZones /> | ||
| <PZDetailsTabsProvider> |
There was a problem hiding this comment.
Suggest doing this provider wrapping in the shared-ui so that we don't need to do duplicate it in bhe
There was a problem hiding this comment.
Agreed! Good catch and feedback much appreciated! updated!
| import { rest } from 'msw'; | ||
| import { setupServer } from 'msw/node'; | ||
| import { zoneHandlers } from '../../../../mocks'; | ||
| import * as tierMocks from '../../../../mocks/factories/privilegeZones'; |
There was a problem hiding this comment.
| import * as tierMocks from '../../../../mocks/factories/privilegeZones'; | |
| import * as zoneMocks from '../../../../mocks/factories/privilegeZones'; |
There was a problem hiding this comment.
Updated by @nicole-m-martin ! Thank you both!
| export const TabTagOption = 'tag' as const; | ||
| export const TabRuleOption = 'rule' as const; | ||
| export const TabObjectOption = 'object' as const; |
There was a problem hiding this comment.
These may be easier to distinguish between each other when looking at them if we start with the entity name instead of Tab
| export const TabTagOption = 'tag' as const; | |
| export const TabRuleOption = 'rule' as const; | |
| export const TabObjectOption = 'object' as const; | |
| export const TagTabValue = 'tag' as const; | |
| export const RuleTabValue = 'rule' as const; | |
| export const ObjectTabValue = 'object' as const; |
There was a problem hiding this comment.
Updated by @nicole-m-martin ! Thank you both!
|
|
||
| return ( | ||
| <div className='flex flex-col w-[400px] h-[625px]'> | ||
| {/* To Do: Address hardcoded height once main content is finished */} |
There was a problem hiding this comment.
The main content area height should already be set. The child accordions will not affect the height once they merge
There was a problem hiding this comment.
Updated thank you!
There was a problem hiding this comment.
Updated in SelectedTabContent component to make sure that tabs stay in place and the content is the one that is restricted/scrolled
| @@ -0,0 +1,176 @@ | |||
| // Copyright 2025 Specter Ops, Inc. | |||
* chore: Zone Icon consistency BED-6924 (#2171) * chore:BED-6924 aligned/sized with notes * chore:BED-6924 remove stroke in DataAlert, remove height/width in Disabled, cleanup Taglist/ZoneIcon * chore:BED-6924 add license to ZoneIcon / remove ZoneAnalysisIcon * chore:BED-6924 small style adjustment * chore:BED-6924 remove comments * chore:BED-6924 only display disabled or upgrade zone Icon for TagList * chore:BED-6924 add persistGlyph prop to ZoneIcon for use in DynamicDetails, add case for hygiene,update styles * chore:BED-6924 clean up/add pick to ZoneIcon type * chore:BED-6924 add hygiene id to ZoneIcon, add size prop to DynammicDetails * chore:BED-6924 small logic change in ZoneIcon * chore:BED-6924 remove isZonePage, clean up ZoneIcon * chore:BED-6924 update styling * chore: Removes unnecessary async from describe blocks on tests * feat: Source Kind Deactivation instead of Deletion + Source Kind Registration / Re-activation BED-6939 BED-6913 (#2165) * feat: modified delete source kinds to deactivate source kinds by adding an active field on the source_kinds table, also modified the methods needed to register, retrieve, and deactivate a source kind * just prepare * added a couple more integration tests * woopsie poopsie * annoying lil line snuck in * addressed some things I overlooked with tests * wow coderabbit caught me again * feat: modified DeactivateSourceKindsByName to no longer allow user to deactivate source kinds that are AZBase or Base, added more integration tests to reflect this change * one more flaw in existing tests fixed, ready for re-review * final address coderabbit * source kinds returned in deterministic order + updated tests to reflect the change * feat(OpenGraph): Add API endpoint to get OpenGraph edge kinds BED-6708 (#2179) * add api to get edge kinds * add tests * generate mocks * fix pointer dereference * rabbit suggestions * rabbit suggestions * fix failing test * fix test * fix verb in test string * better test case for empty response * move feature flag check to the router * translate query filters in new struct * chore:updates variable names for Tabs options, updates mock imports variable name * chore: updates variable name for TabTagValue in PZ.tsx file --------- Co-authored-by: Leah Miller <[email protected]> Co-authored-by: Katherine Powderly <[email protected]> Co-authored-by: Siri <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx (1)
44-49: Type assertion bypasses validation.Line 48 casts
primary_kindtoEntityKindswithout runtime validation. As noted in previous reviews, consider usingvalidateNodeType()to validate before asserting.
🧹 Nitpick comments (9)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
166-167: Consider removing the redundant UPDATE statement.Since the
activecolumn is added withDEFAULT true NOT NULLon lines 163-164, PostgreSQL automatically populates existing rows withtrueand prevents NULL values. This UPDATE statement should match zero rows if the column was just created.However, if this is intentional defensive programming to handle cases where the column already exists from a failed prior migration attempt, consider adding a comment explaining the reasoning.
💡 If the defensive UPDATE is unnecessary, consider removing it
ALTER TABLE source_kinds ADD COLUMN IF NOT EXISTS active BOOLEAN DEFAULT true NOT NULL; - -UPDATE source_kinds SET active = true WHERE active is NULL; -packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsProvider.tsx (1)
21-30: Memoize contextValue to prevent unnecessary re-renders.The
contextValueobject is recreated on every render (lines 24-27), causing all context consumers to re-render even whenselectedDetailsTabhasn't changed.🔎 Wrap contextValue in useMemo
-import { useState } from 'react'; +import { useMemo, useState } from 'react'; import { DetailsTabOption, TagTabValue } from '../utils'; import { SelectedDetailsTabsContext } from './SelectedDetailsTabsContext'; const SelectedDetailsTabsProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const [selectedDetailsTab, setSelectedDetailsTab] = useState<DetailsTabOption>(TagTabValue); - const contextValue = { + const contextValue = useMemo(() => ({ selectedDetailsTab, setSelectedDetailsTab, - }; + }), [selectedDetailsTab]); return <SelectedDetailsTabsContext.Provider value={contextValue}>{children}</SelectedDetailsTabsContext.Provider>; };packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx (1)
38-42: Consider conditional hook calls for efficiency.All three data-fetching hooks execute unconditionally regardless of
currentDetailsTab. If these hooks make API calls, you're fetching data for tabs that aren't active.If React Query caching handles this efficiently and prefetching is intentional, this is acceptable. Otherwise, consider conditional invocation:
🔎 Example conditional hook pattern
- const tagQuery = useAssetGroupTagInfo(tagId); - const ruleQuery = useRuleInfo(tagId, ruleId); - const memberQuery = useMemberInfo(tagId, memberId); + const tagQuery = useAssetGroupTagInfo(currentDetailsTab === TagTabValue ? tagId : undefined); + const ruleQuery = useRuleInfo(currentDetailsTab === RuleTabValue ? tagId : undefined, ruleId); + const memberQuery = useMemberInfo(currentDetailsTab === ObjectTabValue ? tagId : undefined, memberId);Note: Only apply if hooks support conditional execution via undefined params and prefetching isn't desired.
packages/go/openapi/src/openapi.yaml (1)
381-384: Minor: Trailing whitespace detected on added lines.Lines 381-384 appear to have trailing whitespace characters. While this doesn't affect functionality, it's good practice to remove trailing whitespace for cleaner diffs and consistency.
The endpoint registration itself looks correct and follows the existing pattern for referencing external path definitions.
cmd/api/src/database/graphschema.go (1)
60-65: Consider makingFilterAndPaginationunexported.The
FilterAndPaginationstruct is exported (starts with uppercase) but appears to be used only internally within this package. If it's not intended for external consumption, consider making it unexported (filterAndPagination) to keep the public API surface minimal.🔎 Proposed change
-type FilterAndPagination struct { - Filter sqlFilter - SkipLimit string - WhereClause string - OrderSql string +type filterAndPagination struct { + filter sqlFilter + skipLimit string + whereClause string + orderSql string }packages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yaml (1)
17-29: Consider addingrequiredfield list for mandatory properties.The schema defines all properties but doesn't specify which are required. If certain fields like
idandnameare always present in responses, adding arequiredarray would improve API documentation and client code generation.🔎 Suggested enhancement
type: object +required: + - id + - name + - is_traversable + - schema_name properties: id: type: integerpackages/go/openapi/doc/openapi.json (1)
20081-20101: Consider specifying required fields in the schema.The
model.graph-schema-edge-kindsschema doesn't include arequiredarray, making it unclear which fields are mandatory. At minimum,idandnameare likely required for an edge kind definition.💡 Suggested enhancement for the schema
Consider adding a
requiredarray to the YAML source for this schema (since this JSON is generated):"model.graph-schema-edge-kinds": { "type": "object", + "required": ["id", "name", "schema_name"], "properties": { "id": {Adjust the required fields list based on your business logic requirements.
cmd/api/src/api/v2/graphschema.go (1)
68-68: Minor: Use camelCase for Go variable names.Go convention prefers
schemaNameFilteroverschema_name_filter.🔎 Suggested fix
- for i, schema_name_filter := range queryFilters["schema_names"] { - schema_name_filter.SetOperator = model.FilterOr - translatedFilters["schema.name"][i] = schema_name_filter + for i, schemaNameFilter := range queryFilters["schema_names"] { + schemaNameFilter.SetOperator = model.FilterOr + translatedFilters["schema.name"][i] = schemaNameFilterpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx (1)
114-129: Consider explicit handling for undefined zone.When
zoneisundefined, the component falls through to render upgrade/disabled icons. While this may be acceptable for the current use case, it could be confusing since these icons suggest an issue with privilege zone analysis rather than missing data.🔎 Optional improvement
+ if (!zone) return null; + return ( <TooltipProvider>This would make the component's behavior more explicit when zone data is unavailable. Alternatively, document this fallback behavior if it's intentional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
cmd/api/src/api/constant.gocmd/api/src/api/registration/v2.gocmd/api/src/api/v2/graphschema.gocmd/api/src/api/v2/graphschema_test.gocmd/api/src/daemons/datapipe/pipeline.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/sourcekinds.gocmd/api/src/database/sourcekinds_integration_test.gocmd/api/src/model/graphschema.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/openapi.yamlpackages/go/openapi/src/paths/graph-schema.edge-kinds.yamlpackages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yamlpackages/javascript/bh-shared-ui/src/components/AppIcon/Icons/DataAlert.tsxpackages/javascript/bh-shared-ui/src/components/AppIcon/Icons/Disabled.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsProvider.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneAnalysisIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneAnalysisIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx
🧰 Additional context used
🧠 Learnings (17)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/model/graphschema.gocmd/api/src/api/v2/graphschema_test.gopackages/go/openapi/src/openapi.yamlcmd/api/src/database/graphschema.gocmd/api/src/api/v2/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/api/registration/v2.gopackages/go/openapi/src/paths/graph-schema.edge-kinds.yamlpackages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yamlpackages/go/openapi/doc/openapi.jsoncmd/api/src/database/mocks/db.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/api/v2/graphschema_test.gocmd/api/src/database/graphschema_integration_test.go
📚 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/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.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.tsxpackages/javascript/bh-shared-ui/src/components/AppIcon/Icons/DataAlert.tsxpackages/javascript/bh-shared-ui/src/components/AppIcon/Icons/Disabled.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/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.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/ZoneIcon.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
packages/go/openapi/src/openapi.yamlpackages/go/openapi/src/paths/graph-schema.edge-kinds.yamlpackages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yamlpackages/go/openapi/doc/openapi.json
📚 Learning: 2025-11-06T21:35:02.751Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx:30-30
Timestamp: 2025-11-06T21:35:02.751Z
Learning: In AppLink component (packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx), the aria-label="Navigate to ${path}" is added as a fallback for cases where children may be non-string content (icons, JSX elements) rather than readable text.
Applied to files:
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/DataAlert.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 BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/DataAlert.tsxpackages/javascript/bh-shared-ui/src/components/AppIcon/Icons/Disabled.tsx
📚 Learning: 2025-12-18T21:50:43.837Z
Learnt from: brandonshearin
Repo: SpecterOps/BloodHound PR: 2165
File: cmd/api/src/database/sourcekinds.go:64-69
Timestamp: 2025-12-18T21:50:43.837Z
Learning: In cmd/api/src/database/sourcekinds.go, the GetSourceKinds query should sort results by name ASC at the database layer to match the UI's alphanumeric sorting behavior and maintain consistency.
Applied to files:
cmd/api/src/database/graphschema.gocmd/api/src/daemons/datapipe/pipeline.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/sourcekinds.gocmd/api/src/database/sourcekinds_integration_test.gocmd/api/src/database/mocks/db.go
📚 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/Details/TagList.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/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx
📚 Learning: 2025-08-07T19:30:58.455Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx:107-117
Timestamp: 2025-08-07T19:30:58.455Z
Learning: In the BloodHound codebase Field component in packages/javascript/bh-shared-ui/src/views/Explore/fragments.tsx, null values are intentionally NOT explicitly filtered out in the value validation check. The EntityField type definition does not include null as a supported value type, and the try-catch block is used to safely handle edge cases where Object.keys() might be called on null values while preserving the intended null value handling behavior.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
packages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yaml
📚 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: Icon components in the BloodHound codebase use BaseSVG which automatically includes VisuallyHidden labels with accessible names from the name prop, so additional aria-label/title attributes are not needed on parent elements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/Disabled.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/DynamicDetails.tsx
📚 Learning: 2025-12-10T20:16:54.652Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 2105
File: cmd/api/src/database/migration/migrations/v8.5.0.sql:139-143
Timestamp: 2025-12-10T20:16:54.652Z
Learning: In the BloodHound schema_environments_principal_kinds table (cmd/api/src/database/migration/migrations/v8.5.0.sql), the PRIMARY KEY(principal_kind) design is intentional and enforces that each principal kind is globally unique and exclusively owned by a single environment. This creates a one-to-many relationship from environments to principal kinds where an environment can have multiple principal kinds, but a principal kind can never be shared between environments.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
🧬 Code graph analysis (13)
cmd/api/src/model/graphschema.go (1)
cmd/api/src/model/filter.go (4)
ValidFilters(486-486)FilterOperator(38-38)NotEquals(46-46)ApproximatelyEquals(47-47)
cmd/api/src/api/v2/graphschema_test.go (6)
cmd/api/src/database/mocks/db.go (2)
MockDatabase(43-47)NewMockDatabase(55-59)cmd/api/src/model/filter.go (3)
Sort(507-507)Filter(479-483)FilterOr(499-499)cmd/api/src/model/graphschema.go (1)
GraphSchemaEdgeKindWithNamedSchema(121-127)cmd/api/src/api/v2/model.go (1)
Resources(105-118)cmd/api/src/auth/model.go (2)
Authorizer(95-98)NewAuthorizer(107-109)cmd/api/src/utils/test/response.go (1)
ProcessResponse(34-56)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx (5)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)packages/javascript/bh-shared-ui/src/hooks/useConfiguration.ts (1)
usePrivilegeZoneAnalysis(35-41)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(495-500)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
cmd/api/src/database/graphschema.go (4)
cmd/api/src/model/filter.go (5)
Filters(485-485)Sort(507-507)Filter(479-483)SortItem(502-505)AscendingSortDirection(491-491)cmd/api/src/model/graphschema.go (6)
GraphSchemaEdgeKindsWithNamedSchema(129-129)GraphSchemaEdgeKinds(84-84)GraphSchemaEdgeKind(87-93)GraphSchemaEdgeKind(95-97)GraphSchemaEdgeKind(110-115)GraphSchemaEdgeKind(117-119)cmd/api/src/database/helper.go (1)
CheckError(39-45)cmd/api/src/database/db.go (1)
BloodhoundDB(192-195)
cmd/api/src/database/graphschema_integration_test.go (3)
cmd/api/src/model/graphschema.go (2)
GraphSchemaEdgeKindWithNamedSchema(121-127)GraphSchemaEdgeKindsWithNamedSchema(129-129)cmd/api/src/model/filter.go (9)
Sort(507-507)Filter(479-483)FilterOr(499-499)ApproximatelyEquals(47-47)FilterAnd(498-498)NotEquals(46-46)SortItem(502-505)DescendingSortDirection(492-492)InvalidSortDirection(490-490)cmd/api/src/database/helper.go (1)
ErrInvalidSortDirection(37-37)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (3)
packages/javascript/bh-shared-ui/src/hooks/useConfiguration.ts (1)
usePrivilegeZoneAnalysis(35-41)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx (1)
ZoneIcon(40-130)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)
cmd/api/src/database/sourcekinds.go (2)
packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)cmd/api/src/database/db.go (1)
BloodhoundDB(192-195)
cmd/api/src/database/sourcekinds_integration_test.go (2)
cmd/api/src/database/sourcekinds.go (1)
SourceKind(58-62)cmd/api/src/daemons/datapipe/datapipe_integration_test.go (1)
IntegrationTestSuite(50-57)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsProvider.tsx (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx (1)
SelectedDetailsTabsProvider(19-19)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx (1)
SelectedDetailsTabsProvider(22-22)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (2)
DetailsTabOption(37-37)TagTabValue(33-33)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx (2)
SelectedDetailsTabsContext(19-22)SelectedDetailsTabsContext(29-29)
cmd/api/src/api/registration/v2.go (1)
cmd/api/src/model/appcfg/flag.go (1)
FeatureOpenGraphSearch(44-44)
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/Disabled.tsx (1)
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx (1)
BaseSVG(24-40)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (2)
DetailsTabOption(37-37)TagTabValue(33-33)
cmd/api/src/database/mocks/db.go (2)
cmd/api/src/model/filter.go (2)
Filters(485-485)Sort(507-507)cmd/api/src/model/graphschema.go (1)
GraphSchemaEdgeKindsWithNamedSchema(129-129)
⏰ 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 (29)
cmd/api/src/database/migration/migrations/v8.5.0.sql (2)
163-164: LGTM! Column addition looks correct.The column definition is valid and the
IF NOT EXISTSclause ensures idempotency. TheDEFAULT true NOT NULLconstraint will automatically populate the column for existing rows.
168-172: LGTM! Parameter update is correct.The UPDATE statement correctly enables Citrix RDP support for the analysis parameter.
packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/Disabled.tsx (1)
21-21: LGTM! Improved flexibility while maintaining defaults.Removing the explicit
widthandheightprops allows the icon size to be controlled via thesizeprop (whichBaseSVGhandles internally with a default of 16). TheviewBoxensures proper scaling, and the change maintains backward compatibility while making the component more flexible.packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/DataAlert.tsx (1)
27-28: No changes needed—DataAlert icon correctly uses inherited color via fill property.The icon uses
fill='currentColor'from theBasePathcomponent, which inherits color from parent text-color classes liketext-[#8E8C95]. ThestrokeWidthproperties control stroke thickness, not color. The implementation is correct and safe.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
33-37: LGTM! Clean TypeScript constant and type definitions.The tab value constants are properly typed with
as constfor literal type inference, and the union typeDetailsTabOptionfollows TypeScript best practices. The naming convention<Entity>TabValueprovides clear semantic meaning and aligns with the previous review suggestion to distinguish entities.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsx (1)
28-61: Verify Suspense fallback positioning.The Suspense fallback uses
absolutepositioning (line 49) to center the loading spinner, but the parent container (line 29) doesn't haverelativepositioning. The spinner will position itself relative to the nearest positioned ancestor, which may not be this container.🔎 Suggested fix
- <div className='flex flex-col w-[400px] h-[625px]'> + <div className='relative flex flex-col w-[400px] h-[625px]'> {/* To Do: Address hardcoded height once main content is finished */}cmd/api/src/database/graphschema.go (1)
527-558: Good refactoring to extract common filter/pagination logic.The
parseFiltersAndPaginationhelper effectively consolidates repeated logic from multiple Get methods, improving maintainability. The default sort byidwhen no sort is specified ensures consistent pagination behavior.cmd/api/src/api/constant.go (1)
50-50: LGTM!The new
QueryParameterSchemasconstant follows the established naming convention and is properly placed within the query parameters block.cmd/api/src/model/graphschema.go (2)
121-129: LGTM!The
GraphSchemaEdgeKindWithNamedSchemastruct is well-defined with appropriate JSON tags that align with the OpenAPI schema definition. The slice type alias provides good readability.
110-119: Verify filter name consistency:schema_namesvsschema.name.The
ValidFilters()method defines"schema_names"as a valid filter key, but the integration tests use"schema.name"when callingGetGraphSchemaEdgeKindsWithSchemaName. This inconsistency could cause issues if the API layer validates incoming filter parameters againstValidFilters()before passing them to the database layer.Please ensure the filter key names are consistent across:
- The OpenAPI spec parameter name
- The
ValidFilters()method- The database query column reference
#!/bin/bash # Check for schema filter name usage across the codebase rg -n 'schema.name|schema_names' cmd/api/src/cmd/api/src/api/registration/v2.go (1)
212-214: LGTM!The endpoint registration is correctly implemented with:
- Appropriate feature flag check (
FeatureOpenGraphSearch)- Proper permission requirement (
GraphDBRead)- Logical placement under the Graph Schema API section
cmd/api/src/database/graphschema_integration_test.go (2)
999-1048: Good test coverage for the new method.The test setup is thorough with two extensions and multiple edge kinds, enabling comprehensive testing of filtering by schema, traversability, and combinations. The test data structure with
want1throughwant4makes assertions clear.
1134-1149: LGTM!The comparison helper functions follow the established patterns in this file. The approach of comparing expected vs actual fields without strict ID matching is appropriate for parallel test execution where IDs are non-deterministic.
cmd/api/src/daemons/datapipe/pipeline.go (1)
120-122: VerifyDeactivateSourceKindsByNameis implemented in the database layer.Changing from
DeleteSourceKindsByNametoDeactivateSourceKindsByNameis a safer pattern that preserves data integrity and enables potential recovery. The error message is appropriately updated to reflect the new behavior. Confirm that theDeactivateSourceKindsByNamemethod is properly implemented in theSourceKindsDatainterface and database layer.packages/go/openapi/doc/openapi.json (1)
7385-7457: No changes needed.The YAML source files have been properly updated in
packages/go/openapi/src/paths/graph-schema.edge-kinds.yamlandpackages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yaml. The ListEdgeKinds endpoint intentionally omits pagination parameters because it returns a reference metadata listing (edge kinds across schemas), not a large dataset. Other metadata or small reference endpoints follow the same pattern, while data-heavy list operations include skip/limit parameters.cmd/api/src/api/v2/graphschema_test.go (1)
35-213: Well-structured test suite with good coverage.The test file follows Go testing best practices:
- Table-driven tests with clear naming
- Parallel execution with isolated mock controllers per subtest (as per retrieved learnings, this is acceptable)
- Coverage of error paths (bad query params, non-filterable columns, unsupported predicates, DB errors) and success path
One minor observation: the header comparison on line 208 (
assert.Equal(t, testCase.expected.responseHeader, header)) compares the full header map. If the response writer adds any additional default headers in the future, this could cause test failures. However, this is a common pattern and acceptable for now.cmd/api/src/database/sourcekinds_integration_test.go (2)
30-157: Good integration test coverage for source kind registration.The tests properly cover:
- Empty kind handling (no-op)
- New kind registration
- Re-activation of deactivated kinds
The test structure with table-driven tests and explicit setup/teardown is clean.
209-388: Comprehensive deactivation test coverage.Tests correctly verify:
- No-op when empty kinds list
- Base and AZBase are protected from deactivation
- Single and multiple kind deactivation
Note: The tests rely on hardcoded IDs (1, 2, 3, 4) based on migration seeding order. This is acceptable for integration tests that run against a controlled database state, but be aware that changes to the migration or seeding order could break these tests.
cmd/api/src/database/sourcekinds.go (3)
37-56: Clean upsert implementation for source kind registration.The SQL upsert pattern correctly handles both new registrations and re-activation of previously deactivated kinds. The early return for nil/empty kinds prevents unnecessary database calls.
64-94: GetSourceKinds implementation correctly filters and orders results.The query properly filters by
active = trueand orders byname ASC, which aligns with the UI's alphanumeric sorting behavior as noted in the retrieved learnings.
96-118: DeactivateSourceKindsByName correctly protects core source kinds.The SQL query properly excludes 'Base' and 'AZBase' from deactivation, ensuring these core source kinds remain active. The use of
pq.Arrayfor the PostgreSQL array parameter is correct.cmd/api/src/database/mocks/db.go (2)
603-615: Generated mock for DeactivateSourceKindsByName looks correct.The mock method signature properly matches the interface definition with
(ctx context.Context, kinds graph.Kinds) error.
1680-1694: Generated mock for GetGraphSchemaEdgeKindsWithSchemaName looks correct.The mock method properly handles the return type
(model.GraphSchemaEdgeKindsWithNamedSchema, int, error)with appropriate type assertions.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (3)
80-117: LGTM: TagDetails refactored to use ZoneIcon.The integration of ZoneIcon with
persistGlyph={true}ensures the glyph always renders in the details header, and the conditional rendering of ObjectCountPanel based on the new prop is well-implemented.
167-175: LGTM: Backwards-compatible prop addition.Making
hasObjectCountPaneloptional with a sensible default (false) maintains backwards compatibility while enabling the new feature when needed.
59-59: No action needed.All TagDetails call sites have been updated with the new
hasObjectCountPanelprop. The only usage at line 185 correctly passes both required props, so the breaking change has been properly handled across the codebase.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx (3)
31-38: LGTM: Flexible type definition.The
ZoneIconPropstype appropriately usesPickto extract specific fields fromAssetGroupTagwhile makinganalysis_enabledandidoptional, allowing the component to work with partial zone data.
79-87: LGTM: Clear special-case handling.The dedicated rendering paths for the top privilege tag and hygiene tag are well-structured and use appropriate icons (
TierZeroandShield).
89-111: LGTM: Well-designed glyph rendering with fallbacks.The
persistGlyphprop provides a useful escape hatch, and the component gracefully falls back toAppIcon.Zoneswhen no valid glyph definition exists. The tooltip messaging is contextual and informative.
| if len(queryFilters["schema_names"]) > 0 { | ||
| translatedFilters["schema.name"] = make([]model.QueryParameterFilter, len(queryFilters["schema_names"])) | ||
| for i, schema_name_filter := range queryFilters["schema_names"] { | ||
| schema_name_filter.SetOperator = model.FilterOr | ||
| translatedFilters["schema.name"][i] = schema_name_filter | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the OpenAPI spec for the parameter name
cat packages/go/openapi/src/paths/graph-schema.edge-kinds.yaml | head -50Repository: SpecterOps/BloodHound
Length of output: 1459
🏁 Script executed:
# Also check the handler file itself around the translateQueryFilters function
cat -n cmd/api/src/api/v2/graphschema.go | sed -n '59,75p'Repository: SpecterOps/BloodHound
Length of output: 1054
🏁 Script executed:
# Search for how schema filters are defined in the codebase
rg -n "schema_names|schemas" cmd/api/src/api/v2/ -A 2 -B 2Repository: SpecterOps/BloodHound
Length of output: 2977
🏁 Script executed:
# Check if there's a mapping or translation happening elsewhere
rg -n "queryFilters\[" cmd/api/src/api/v2/graphschema.goRepository: SpecterOps/BloodHound
Length of output: 516
🏁 Script executed:
# Check the ParseQueryParameterFilters function and how it populates queryFilters
rg -n "ParseQueryParameterFilters" cmd/api/src/api/v2/graphschema.go -A 5 -B 5Repository: SpecterOps/BloodHound
Length of output: 717
🏁 Script executed:
# Look at the entire ListEdgeTypes handler to see the full flow
cat -n cmd/api/src/api/v2/graphschema.go | sed -n '27,57p'Repository: SpecterOps/BloodHound
Length of output: 1816
🏁 Script executed:
# Search for the ParseQueryParameterFilters definition
fd -t f -n "*.go" cmd/api/src/ | xargs rg -l "func ParseQueryParameterFilters" | head -5Repository: SpecterOps/BloodHound
Length of output: 295
🏁 Script executed:
# Search for the ParseQueryParameterFilters definition
fd -t f "*.go" cmd/api/src/ | xargs rg -l "func ParseQueryParameterFilters" | head -5Repository: SpecterOps/BloodHound
Length of output: 348
🏁 Script executed:
# Check what this function does - look for QueryParameterFilterParser
rg -n "type QueryParameterFilterParser" cmd/api/src/ -A 20Repository: SpecterOps/BloodHound
Length of output: 1409
🏁 Script executed:
# Also check if there's any parameter name mapping elsewhere
rg -n "schemas" cmd/api/src/api/v2/ -B 3 -A 3Repository: SpecterOps/BloodHound
Length of output: 2662
🏁 Script executed:
# Get more of the ParseQueryParameterFilters function to see the full implementation
cat -n cmd/api/src/model/filter.go | sed -n '446,480p'Repository: SpecterOps/BloodHound
Length of output: 1163
🏁 Script executed:
# Also check the Query method to see if there's any parameter name mapping
rg -n "ParseQueryParameterFilters" cmd/api/src/model/filter.go -A 30Repository: SpecterOps/BloodHound
Length of output: 990
🏁 Script executed:
# Let me verify the exact flow - check if there's any constant mapping or translation I missed
rg -n "\"schemas\"|\"schema_names\"" cmd/api/src/api/v2/graphschema.goRepository: SpecterOps/BloodHound
Length of output: 292
🏁 Script executed:
# Check the test to confirm what parameter it actually uses
cat -n cmd/api/src/api/v2/graphschema_test.go | sed -n '125,145p'Repository: SpecterOps/BloodHound
Length of output: 878
Fix parameter name mismatch with OpenAPI spec.
The OpenAPI spec defines the query parameter as schemas, but the handler checks for schema_names. This breaks the API contract—clients following the documented spec will have their filter silently ignored because queryFilters["schema_names"] will be empty. Either update the handler to check for schemas (recommended, since that's the public API contract) or update the OpenAPI spec and test to use schema_names.
🤖 Prompt for AI Agents
In cmd/api/src/api/v2/graphschema.go around lines 66 to 72, the handler is
checking queryFilters["schema_names"] but the OpenAPI spec defines the parameter
as "schemas", so filters are ignored; update the handler to use
queryFilters["schemas"] (replace all occurrences of "schema_names" in this
block) and keep the rest of the logic (set operator to FilterOr and assign into
translatedFilters["schema.name"]) so the public API contract is honored; also
run/update related tests to use "schemas" if any test or spec references
"schema_names".
| parameters: | ||
| - name: schemas | ||
| description: Schema names to filter response by | ||
| in: query | ||
| required: false | ||
| schema: | ||
| type: array | ||
| items: | ||
| type: string |
There was a problem hiding this comment.
Parameter name mismatch with handler implementation.
The parameter is named schemas here, but the handler in cmd/api/src/api/v2/graphschema.go expects schema_names in the translateQueryFilters function. One of these needs to be updated for consistency.
Either:
- Update this OpenAPI spec to use
schema_names, or - Update the handler's
translateQueryFiltersto check forschemasinstead ofschema_names
Based on the description "Schema names to filter response by", schema_names seems more semantically accurate.
🔎 Suggested fix (if keeping handler as-is)
parameters:
- - name: schemas
+ - name: schema_names
description: Schema names to filter response by
in: query
required: false 📝 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.
| parameters: | |
| - name: schemas | |
| description: Schema names to filter response by | |
| in: query | |
| required: false | |
| schema: | |
| type: array | |
| items: | |
| type: string | |
| parameters: | |
| - name: schema_names | |
| description: Schema names to filter response by | |
| in: query | |
| required: false | |
| schema: | |
| type: array | |
| items: | |
| type: string |
🤖 Prompt for AI Agents
In packages/go/openapi/src/paths/graph-schema.edge-kinds.yaml around lines 29 to
37, the query parameter is named `schemas` but the handler expects
`schema_names`; update the OpenAPI spec to rename the parameter to
`schema_names` to match the handler. Change the `- name: schemas` entry to `-
name: schema_names` and keep the rest of the parameter definition (description,
in: query, required: false, schema: type: array of string) so the generated
client and documentation align with translateQueryFilters in
cmd/api/src/api/v2/graphschema.go.
| export const useSelectedDetailsTabsContext = () => { | ||
| const context = useContext(SelectedDetailsTabsContext); | ||
|
|
||
| if (!context) { | ||
| throw new Error('useSelectedDetailsTabsContext is outside of SelectedDetailsTabsContext'); | ||
| } | ||
|
|
||
| return context; | ||
| }; |
There was a problem hiding this comment.
Ineffective context validation check.
The condition if (!context) on line 34 will never be true. useContext always returns a value—either from the nearest provider or the initial default (lines 24-27). The check cannot detect usage outside a provider because the initial value is a valid object, not undefined.
While the current implementation functions correctly, the error message suggests developer intent to catch misuse. If protection is desired, consider using a sentinel value pattern:
🔎 Alternative implementation with effective validation
+const UNINITIALIZED = Symbol('UNINITIALIZED');
+
const initialSelectedDetailsTabsValue = {
selectedDetailsTab: TagTabValue,
- setSelectedDetailsTab: () => {},
+ setSelectedDetailsTab: UNINITIALIZED as any,
};
export const SelectedDetailsTabsContext = createContext<SelectedDetailsTabsContext>(initialSelectedDetailsTabsValue);
export const useSelectedDetailsTabsContext = () => {
const context = useContext(SelectedDetailsTabsContext);
- if (!context) {
+ if (context.setSelectedDetailsTab === UNINITIALIZED) {
throw new Error('useSelectedDetailsTabsContext is outside of SelectedDetailsTabsContext');
}
return context;
};Alternatively, if the error check isn't essential, remove it to avoid misleading future maintainers.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx
around lines 31 to 39, the runtime check `if (!context)` is ineffective because
useContext always returns the default value; replace the default context value
with a distinct sentinel (e.g., Symbol or null) and update the provider to
supply the real context so the hook can reliably detect missing provider, or
simply remove the erroneous check and its error throw if you prefer no runtime
guard; ensure whichever approach you choose keeps types consistent and update
any related types/initial values accordingly.
| } | ||
| }) | ||
| .map((listItem) => { | ||
| const displayIcon = !listItem.analysis_enabled || !privilegeZoneAnalysisEnabled; |
There was a problem hiding this comment.
Critical logic error: Glyphs won't render for enabled zones.
The displayIcon calculation inverts the intended behavior. Currently:
displayIcon = !listItem.analysis_enabled || !privilegeZoneAnalysisEnabledistruewhen analysis is disabled- Line 115 only renders
ZoneIconwhendisplayIconistrue - Result: Glyphs never display for zones with enabled analysis
The ZoneIcon component internally handles what to display based on privilegeZoneAnalysisEnabled and analysis_enabled. The conditional on line 115 should either:
- Always render
ZoneIcon(remove thedisplayIconcondition entirely), OR - Invert the logic:
const displayIcon = listItem.analysis_enabled && privilegeZoneAnalysisEnabled;
Recommendation: Remove the displayIcon condition and always render ZoneIcon for non-top tags, letting ZoneIcon handle the display logic internally.
🔎 Proposed fix
- const displayIcon = !listItem.analysis_enabled || !privilegeZoneAnalysisEnabled;
-
return (
<li
key={listItem.id}
data-testid={`privilege-zones_details_${title.toLowerCase()}-list_item-${listItem.id}`}
className={cn('border-y border-neutral-3 relative h-10', {
'bg-neutral-4': selected === listItem.id.toString(),
})}>
<SelectedHighlight itemId={listItem.id} type='tag' />
<Button
variant='text'
className='flex justify-between w-full'
onClick={() => onSelect(listItem.id)}>
<div className='flex overflow-hidden'>
- {listItem.id !== topTagId && displayIcon && (
+ {listItem.id !== topTagId && (
<ZoneIcon
size={18}
zone={listItem}📝 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.
| const displayIcon = !listItem.analysis_enabled || !privilegeZoneAnalysisEnabled; | |
| return ( | |
| <li | |
| key={listItem.id} | |
| data-testid={`privilege-zones_details_${title.toLowerCase()}-list_item-${listItem.id}`} | |
| className={cn('border-y border-neutral-3 relative h-10', { | |
| 'bg-neutral-4': selected === listItem.id.toString(), | |
| })}> | |
| <SelectedHighlight itemId={listItem.id} type='tag' /> | |
| <Button | |
| variant='text' | |
| className='flex justify-between w-full' | |
| onClick={() => onSelect(listItem.id)}> | |
| <div className='flex overflow-hidden'> | |
| {listItem.id !== topTagId && ( | |
| <ZoneIcon | |
| size={18} | |
| zone={listItem} | |
| iconClasses={iconClasses} | |
| wrapperClasses={wrapperClasses} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
around line 100, the displayIcon boolean is inverted causing glyphs to be hidden
for enabled zones; remove the displayIcon check and always render <ZoneIcon> for
non-top tags (delete the displayIcon variable and any conditional that skips
ZoneIcon at line ~115) so ZoneIcon can internally decide what to show, ensuring
glyphs appear correctly when analysis is enabled.
| /> | ||
| ); | ||
| const hygieneIcon = <AppIcon.Shield {...iconProps} className={cn('ml-0 text-contrast', iconClasses)} />; | ||
| const iconDefinition = findIconDefinition({ prefix: 'fas', iconName: glyph as IconName }); |
There was a problem hiding this comment.
Unsafe type assertion on glyph.
The glyph as IconName cast assumes glyph is always a valid FontAwesome icon name. If an invalid glyph value exists in the data, this could lead to runtime issues or silent failures.
While findIconDefinition returns undefined for invalid icon names (which is handled later), the type assertion bypasses TypeScript's type safety.
🔎 Safer alternative
- const iconDefinition = findIconDefinition({ prefix: 'fas', iconName: glyph as IconName });
+ const iconDefinition = glyph ? findIconDefinition({ prefix: 'fas', iconName: glyph as IconName }) : undefined;Consider adding runtime validation or documenting that glyph values must be validated before being stored in AssetGroupTag.
📝 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.
| const iconDefinition = findIconDefinition({ prefix: 'fas', iconName: glyph as IconName }); | |
| const iconDefinition = glyph ? findIconDefinition({ prefix: 'fas', iconName: glyph as IconName }) : undefined; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsx (1)
29-29: Verify the necessity of the fixed height constraint.The fixed
h-[625px]may be unnecessary if the parent container already manages height, and could reduce flexibility across different viewport sizes or layouts. Based on past review feedback, the main content area height is expected to be set by the parent.Consider whether this component should inherit its height from the parent layout instead of using a fixed value, or verify that this specific dimension is required for the design.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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/SelectedDetailsTabs/SelectedDetailsTabs.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/PrivilegeZones.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabsContext.tsx (1)
useSelectedDetailsTabsContext(31-39)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
TagTabValue(33-33)
⏰ 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: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZones.tsx (2)
135-139: LGTM! Tab reset on navigation is well-implemented.Resetting the selected details tab to
TagTabValuewhen navigating between zones and labels ensures a consistent starting state. The placement after thenavigatecall is appropriate.
188-194: LGTM! Provider wrapper follows best practices.The provider wrapper pattern cleanly integrates the
PZDetailsTabsProviderwhile maintaining the existing public API through the default export. This separation allows the inner component to consume the context without prop drilling.
| <Suspense | ||
| fallback={ | ||
| <div className='absolute inset-0 flex items-center justify-center'> | ||
| <CircularProgress color='primary' size={80} /> | ||
| </div> | ||
| }> |
There was a problem hiding this comment.
Fix the Suspense fallback positioning.
The fallback uses absolute inset-0 positioning, but the parent container (line 29) lacks position: relative. This causes the loading spinner to position relative to a higher ancestor rather than centering within this component's bounds.
🔎 Suggested fix
- <div className='flex flex-col w-[400px] h-[625px]'>
+ <div className='relative flex flex-col w-[400px] h-[625px]'>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.tsx
around lines 46 to 51 (and the parent container at line 29), the Suspense
fallback uses absolute positioning but the parent lacks position: relative,
causing the spinner to be positioned against an ancestor; add position: relative
to the parent container (e.g., add the 'relative' class to the parent element’s
className at line 29) so the fallback’s absolute inset-0 centers correctly
within this component, or alternatively change the fallback to a non-absolute
centered layout if you prefer not to modify the parent.
Description
Added selected details tabs to PZ. We added the following
Note:
To actually see the changes you need to uncomment NewDetails in the Details index file for PZ in shared ui
Functionality will not work in current NewDetails until lists are added
If you want to test you need to add SelectedDetailsTabs in Details
Motivation and Context
Resolves: BED-6836
How Has This Been Tested?
Manually tested, added new tests and existing ones passed.
Screenshots (optional):
PZDetailsTabDemo.mp4
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.