Stage/v7.6.1: backmerge #1675
Stage/v7.6.1: backmerge #1675
Conversation
WalkthroughThe changes update the asset group tag ID handling and associated hooks, shifting from returning primitive values to structured objects with status flags. Feature flag checks for "tier_management_engine" are introduced throughout the codebase, affecting hooks, components, and tests. The Go backend function for parsing asset group tag IDs now returns slices, not single integers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI_Component
participant useHighestPrivilegeTagId
participant useFeatureFlag
participant API
User->>UI_Component: Interacts with Zone Management UI
UI_Component->>useHighestPrivilegeTagId: Request highest privilege tag ID
useHighestPrivilegeTagId->>useFeatureFlag: Check "tier_management_engine" enabled
useFeatureFlag->>API: GET /api/v2/features
API-->>useFeatureFlag: Respond with feature flags
useFeatureFlag-->>useHighestPrivilegeTagId: Return enabled/disabled state
useHighestPrivilegeTagId-->>UI_Component: Return { tagId, isLoading, isError }
UI_Component->>UI_Component: Use tagId if available, else handle loading/error
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…back (#1678) * fix(AGT): add hygiene placeholder id to agt query param fallback * fix: update zone query params and fix tests --------- Co-authored-by: Ulises Rangel <[email protected]>
* fix: update tag id usage for object return value * chore: add unit test checking url based on top tag id
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
79-80: Good use of optional chaining, but consider handling hook states.The optional chaining (
topTagId?.toString()) correctly handles the potentially undefinedtagId. However, theisLoadingandisErrorstates returned by the hook aren't being handled, which could lead to suboptimal UX during loading states.Consider extracting and handling these states:
-const { tagId: topTagId } = useHighestPrivilegeTagId(); +const { tagId: topTagId, isLoading, isError } = useHighestPrivilegeTagId();This would allow you to show loading indicators or handle error states appropriately.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx (1)
40-41: Consider handling loading and error states from the hook.The destructuring and optional chaining are implemented correctly. However, the
isLoadingandisErrorstates fromuseHighestPrivilegeTagId()aren't being handled, which could result in suboptimal UX during loading states.Consider extracting these states for better user experience:
-const { tagId: topTagId } = useHighestPrivilegeTagId(); +const { tagId: topTagId, isLoading, isError } = useHighestPrivilegeTagId();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/api/src/api/attackpaths.go(1 hunks)cmd/ui/src/views/ZoneManagement/InfoHeader.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useZoneParams/useZoneQueryParams.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/hooks/useZoneParams/useZoneQueryParams.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/DetailRoot.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/DetailsRoot.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/Save.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/Save.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
cmd/api/src/api/attackpaths.go (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
🧬 Code Graph Analysis (9)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/Save.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/DetailsRoot.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/Summary.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
cmd/ui/src/views/ZoneManagement/InfoHeader.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(64-69)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useFeatureFlags.tsx (1)
useFeatureFlag(52-54)
🔇 Additional comments (29)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.test.tsx (1)
96-107: LGTM: Feature flag mock properly configuredThe mock API handler for
/api/v2/featuresis correctly structured and enables thetier_management_enginefeature flag for testing. This aligns with the broader refactoring to introduce feature flag gating in the hooks and components.packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.test.tsx (1)
63-75: LGTM: Consistent feature flag mock implementationThe mock handler follows the same pattern as other test files, ensuring consistent testing behavior across the ZoneManagement components with the
tier_management_enginefeature flag enabled.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (1)
58-70: LGTM: Consistent feature flag mock patternThe mock handler implementation is identical to other test files, maintaining consistency in how the
tier_management_enginefeature flag is tested across the ZoneManagement components.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/TagForm/TagForm.tsx (1)
111-111: LGTM: Correctly updated hook usageThe change properly adapts to the new
useHighestPrivilegeTagIdhook return type, which now returns an object with{ isLoading, isError, tagId }instead of a primitive value. The destructuring maintains the existingtopTagIdvariable name for consistency.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.tsx (1)
57-57: LGTM: Consistent hook usage updateThe change correctly updates the
useHighestPrivilegeTagIdhook usage to destructure thetagIdfrom the returned object, maintaining consistency with the pattern established in other ZoneManagement components.packages/javascript/bh-shared-ui/src/hooks/useZoneParams/useZoneQueryParams.test.tsx (1)
44-55: Feature flag mock handler looks correct.The mock API handler for the
/api/v2/featuresendpoint properly simulates thetier_management_enginefeature flag being enabled, which aligns with the test requirements.cmd/ui/src/views/ZoneManagement/InfoHeader.tsx (1)
22-23: LGTM: Correct adaptation to new hook return structureThe destructuring of
tagIdfromuseHighestPrivilegeTagId()and the use of optional chaining fortopTagId?.toString()properly handles the hook's new return structure wheretagIdcan be undefined during loading or error states.packages/javascript/bh-shared-ui/src/views/ZoneManagement/DetailsRoot.tsx (1)
22-24: LGTM: Proper handling of updated hook return structureThe destructuring of
tagIdfromuseHighestPrivilegeTagId()and the conditional check correctly handle the hook's new return structure wheretagIdcan be undefined during loading or error states.packages/javascript/bh-shared-ui/src/views/ZoneManagement/DetailRoot.test.tsx (1)
22-41: LGTM: Proper mock server setup for feature flag testingThe mock server setup with the feature flag endpoint is correctly implemented and aligns with the broader changes to support the
tier_management_enginefeature flag.packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/Save.test.tsx (1)
24-58: LGTM: Comprehensive mock server setupThe mock handlers for asset group tags, features, and configuration endpoints are well-structured and appropriate for testing the Save component.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx (3)
43-54: LGTM: Proper feature flag mock setupThe feature flag mock handler is correctly implemented and aligns with the broader changes to support the
tier_management_enginefeature flag testing.
82-89: LGTM: Correct test assertion updatesThe test assertions have been properly updated to work with the new hook return structure where
useOrderedTagsreturns an object withorderedTagsproperty instead of a direct array.
113-113: LGTM: Correct assertion update for object returnThe test assertion correctly accesses
result.current.tag.positionto work with the new hook return structure.packages/javascript/bh-shared-ui/src/hooks/useZoneParams/useZoneQueryParams.tsx (6)
20-20: LGTM! Feature flag import added correctly.The import for
useFeatureFlagis properly added to integrate tier management engine feature gating.
36-36: LGTM! Feature flag check implementation is correct.The feature flag check for
tier_management_enginefollows the established pattern and will properly gate the tier management functionality.
38-38: LGTM! Updated hook usage aligns with new return structure.The destructuring of
tagId,isLoading, andisErrorfromuseHighestPrivilegeTagIdcorrectly handles the updated hook return structure that now includes loading and error states.
40-44: LGTM! Memoized callback prevents unnecessary re-renders.The
setZoneQueryParamscallback is properly memoized withuseCallbackand the correct dependency array, which will prevent unnecessary re-renders when the hook's return value is used in React components.
48-54: LGTM! Early return logic properly handles loading and error states.The early return when feature flag or tag ID is loading/errored prevents unnecessary processing and ensures consistent behavior. Returning
undefinedforassetGroupTagIdand emptyparamsis the correct fallback behavior.
56-56: LGTM! Correctly uses destructuredtagIdvalue.The
parseAssetGroupTagIdfunction now correctly receives the destructuredtagIdfrom the updated hook return structure instead of a direct primitive value.cmd/api/src/api/attackpaths.go (4)
28-29: LGTM! Function signature change to return slice is well-implemented.The change from returning a single
intto[]intallows the function to return multiple relevant tag IDs, which is more flexible and aligns with the broader codebase changes for handling multiple asset group tag IDs.
31-43: LGTM! Logic correctly handles specific tag ID cases.The updated logic properly handles:
- Parsing errors by returning the slice
- Hygiene placeholder (ID 0) by appending the constant
- Valid tag IDs by appending the parsed value
- Database lookup errors appropriately
All return paths consistently return slices.
45-54: LGTM! Fallback logic returns both hygiene and tier zero IDs.The fallback behavior correctly returns both the hygiene placeholder ID and the tier zero asset group tag ID when no specific tag ID is provided. This makes sense as it provides comprehensive coverage for default scenarios.
28-54: No callers found for ParseAssetGroupTagIdWithFallback
A search across the repository for usages ofParseAssetGroupTagIdWithFallbackreturned only its own definition and no call sites. There are currently no callers that need to be updated to handle the new[]intreturn type.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (6)
24-24: LGTM! Feature flag import added correctly.The import for
useFeatureFlagis properly added to enable tier management engine feature gating.
26-39: LGTM! Feature flag gating prevents unnecessary queries.The feature flag check and query enablement logic correctly:
- Checks if the feature flag is not loading, not errored, and enabled
- Only enables the query when the feature flag conditions are met
- Prevents unnecessary API calls when tier management is disabled
This follows good practices for feature flag implementation.
41-53: LGTM! Updated return structure provides better state management.The
useOrderedTagshook now:
- Properly destructures loading and error states from
useAssetGroupTags- Returns an object with
orderedTags,isLoading, andisError- Maintains the same filtering and sorting logic
This provides consumers with better visibility into the loading and error states.
57-62: LGTM! Consistent return structure foruseHighestPrivilegeTag.The hook correctly:
- Destructures from the updated
useOrderedTagsreturn structure- Returns an object with loading/error states alongside the tag data
- Maintains the same logic for finding the highest privilege tag
This provides consistent state management across all hooks.
64-69: LGTM! Consistent return structure foruseHighestPrivilegeTagId.The hook correctly:
- Destructures from the updated
useOrderedTagsreturn structure- Returns an object with loading/error states alongside the tag ID
- Maintains the same logic for finding the highest privilege tag ID
This aligns with the pattern established by other hooks in the file.
26-69: Verify all consumers of these hooks are updated
The return shapes ofuseOrderedTags,useHighestPrivilegeTag, anduseHighestPrivilegeTagIdhave changed from primitives to objects. Please search for every usage and update them to destructure the new properties:Example updates:
- const tags = useOrderedTags(); + const { orderedTags, isLoading, isError } = useOrderedTags();- const highestTag = useHighestPrivilegeTag(); + const { tag: highestTag, isLoading, isError } = useHighestPrivilegeTag();- const highestTagId = useHighestPrivilegeTagId(); + const { tagId: highestTagId, isLoading, isError } = useHighestPrivilegeTagId();
packages/javascript/bh-shared-ui/src/hooks/useZoneParams/useZoneQueryParams.test.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneManagement.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/DetailRoot.test.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/Save.test.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Save/Save.test.tsx
Show resolved
Hide resolved
Description
Back merges the stage/v7.6.1 branch into mainline
Motivation and Context
Resolves BED-6179
Why is this change required? What problem does it solve?
How Has This Been Tested?
Builds successfully, tests pass, and smoke test performed
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests