Refactor: BED-6624 dynamic edit button #2012
Conversation
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (1)
209-219: Test coverage looks good, but consider adding a test for the Rule case.The test correctly verifies the "Edit Zone" button on the Zones page. However, there's no test coverage for the "Edit Rule" case when a
selectorIdis present.Would you like me to generate a test case for the "Edit Rule" scenario?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (1)
packages/javascript/bh-shared-ui/src/routes/index.tsx (3)
privilegeZonesPath(22-22)zonesPath(17-17)detailsPath(23-23)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx (1)
packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx (1)
AppLink(25-42)
⏰ 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 (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx (2)
81-81: LGTM!The extraction of
isZonePageis appropriate for determining the contextual button label.
118-118: LGTM!The ternary logic clearly handles the three contexts (Rule/Zone/Label) for the edit button title.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
Outdated
Show resolved
Hide resolved
LucasParraF
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
PR: SpecterOps/BloodHound#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/Details.test.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (1)
packages/javascript/bh-shared-ui/src/routes/index.tsx (3)
privilegeZonesPath(22-22)zonesPath(17-17)detailsPath(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx (2)
34-37: LGTM!The
SetupPropstype appropriately models the optional routing parameters for both Zones and Labels page tests.
206-210: LGTM!The test correctly validates the dynamic "Edit Zone" button text when on the Zones page.
| const setupComponent = ({ zoneId, labelId }: SetupProps) => { | ||
| const route = `/${privilegeZonesPath}/${zonesPath}/${zoneId}/${detailsPath}`; | ||
| vi.mocked(useParams).mockReturnValue({ zoneId, labelId }); | ||
| render( | ||
| <Routes> | ||
| <Route path={`/${privilegeZonesPath}/${zonesPath}/1/${detailsPath}`} element={<Details />} /> | ||
| <Route path={route} element={<Details />} /> | ||
| </Routes>, | ||
| { route: `/${privilegeZonesPath}/${zonesPath}/1/${detailsPath}` } | ||
| { route } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Fix route construction to support both Zones and Labels pages.
The setupComponent function always constructs a route using zonesPath and zoneId, even when testing the Labels page. When called with { labelId: '2' }, the route becomes /privilege-zones/zones/undefined/details, which is incorrect.
The route construction should be conditional based on whether you're testing Zones or Labels:
Apply this diff to fix the route construction:
const setupComponent = ({ zoneId, labelId }: SetupProps) => {
- const route = `/${privilegeZonesPath}/${zonesPath}/${zoneId}/${detailsPath}`;
+ const pathSegment = labelId ? 'labels' : zonesPath;
+ const id = labelId ?? zoneId;
+ const route = `/${privilegeZonesPath}/${pathSegment}/${id}/${detailsPath}`;
vi.mocked(useParams).mockReturnValue({ zoneId, labelId });Note: Verify that labelsPath is exported from routes/index.tsx and import it if available, then use it instead of the hardcoded string 'labels'.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.test.tsx
Outdated
Show resolved
Hide resolved
| const saveLink = getSavePath(zoneId, labelId, selectorId); | ||
|
|
||
| // Edit button title changes depending on tab/context | ||
| const editButtonTitle = selectorId ? 'Rule' : isZonePage ? 'Zone' : 'Label'; |
There was a problem hiding this comment.
(Partially copied from BHE) Although I feel this is simple enough to have as a nested ternary, and that it works, I do have something that pops to mind.
I feel that isZonePage is not the ultimate truth in determining if it should be Zone or Label. It happens to work in this case but its not like a isDisabled to determine if to set a class or not. In this case isLabelPage would be the ultimate truth. So feel like this maybe not as descriptive or as scalable as it could be. Although again it works and its simple.
Maybe a solution can be to incorporate all into a function that has each variable that decides the value and returns "Edit {{value}}"
Finally maybe another level of abstraction, considering we do the same in BHE ( an Edit is repeated ), is to maybe have a hook there that determines de Edit Title and is used in both places. ( this could be overkill but figured I would mention it )
There was a problem hiding this comment.
I like the thought in your final remark especially since like you mention the same is done in BHE. A small hook or component would be nice to encapsulate and reuse this logic.
| it('renders edit label button when on Labels page', async () => { | ||
| setupComponent({ labelId: '2' }); | ||
|
|
||
| longWait(async () => { |
There was a problem hiding this comment.
I need to remove this longWait utility function as it is not good practice. Let's refactor this test so that we reach for something else instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsx (2)
43-51: Consider simplifying the button state logic.The interaction between
asChild,disabled,showEditButton, andsaveLinkcreates complex conditional behavior that's difficult to understand:
asChild={showEditButton || !saveLink}disabled={!!saveLink}to={saveLink || ''}When
saveLinkis truthy, the button is disabled but still contains a link, andasChilddepends onshowEditButton. WhensaveLinkis falsy, navigation goes to an empty string (current page). Consider adding inline comments explaining the intended states or refactoring to make the logic more explicit.
44-44: Consider extracting layout styles to parent component.The wrapper div includes layout-specific classes (
basis-1/3,flex-col,gap-4) that couple this component to a specific usage context. For better reusability, consider letting the parent component handle layout concerns and keeping this component focused on rendering the edit button.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsx
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/index.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/EditInfoPanelButton/index.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsx (1)
packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx (1)
AppLink(25-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-go-unit-tests
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/index.tsx (1)
17-19: LGTM!Standard barrel export pattern correctly re-exports the EditInfoButton component.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsx (1)
20-24: LGTM!The enum values correctly map to the dynamic button labels described in the PR objectives.
| let titleAppend: TitleAppend | undefined; | ||
|
|
||
| if (selectorId) { | ||
| titleAppend = TitleAppend.Selector; | ||
| } else if (zoneId) { | ||
| titleAppend = TitleAppend.Zone; | ||
| } else if (labelId) { | ||
| titleAppend = TitleAppend.Label; | ||
| } |
There was a problem hiding this comment.
Handle the case where all IDs are undefined.
If selectorId, zoneId, and labelId are all undefined, titleAppend remains undefined, causing the button to display "Edit undefined" (line 47). While the component is likely always provided with at least one ID based on the current view context, defensive programming suggests handling this edge case gracefully.
Consider adding a fallback:
let titleAppend: TitleAppend | undefined;
if (selectorId) {
titleAppend = TitleAppend.Selector;
} else if (zoneId) {
titleAppend = TitleAppend.Zone;
} else if (labelId) {
titleAppend = TitleAppend.Label;
+} else {
+ titleAppend = TitleAppend.Zone; // or throw an error, or return null to hide the button
}📝 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.
| let titleAppend: TitleAppend | undefined; | |
| if (selectorId) { | |
| titleAppend = TitleAppend.Selector; | |
| } else if (zoneId) { | |
| titleAppend = TitleAppend.Zone; | |
| } else if (labelId) { | |
| titleAppend = TitleAppend.Label; | |
| } | |
| let titleAppend: TitleAppend | undefined; | |
| if (selectorId) { | |
| titleAppend = TitleAppend.Selector; | |
| } else if (zoneId) { | |
| titleAppend = TitleAppend.Zone; | |
| } else if (labelId) { | |
| titleAppend = TitleAppend.Label; | |
| } else { | |
| titleAppend = TitleAppend.Zone; // or throw an error, or return null to hide the button | |
| } |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/EditInfoPanelButton/EditInfoButton.tsx
around lines 33 to 41, titleAppend can remain undefined when selectorId, zoneId,
and labelId are all falsy, causing the button to render "Edit undefined"; add a
defensive fallback by setting titleAppend to a sensible default (e.g.,
TitleAppend.Unknown or TitleAppend.Selector) when none of the IDs are present,
or derive the displayed title via a conditional that falls back to a static
label like "Edit" / "Edit Item" to avoid showing "undefined".
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Solid clean up, I think there's opportunity for even more! 🙌
| zoneId?: string | undefined; | ||
| showEditButton: boolean; | ||
| saveLink?: string | undefined; | ||
| }> = ({ labelId, selectorId, zoneId, showEditButton, saveLink }) => { |
There was a problem hiding this comment.
Thanks for moving this into its own component. Really clean. If you used the PZPathParams hook, then passing the ids can be cleaned up as well as pull the saveLink function from Details.tsx as it's only used in the one spot. Then you only have a single prop showEditButton 🧼
| showEditButton: boolean; | ||
| saveLink?: string | undefined; | ||
| }> = ({ labelId, selectorId, zoneId, showEditButton, saveLink }) => { | ||
| let titleAppend: TitleAppend | undefined; |
There was a problem hiding this comment.
nit: "append" feels weird here for this variable. It's a verb used as a noun. What do you think of using TitleSuffix or title = 'Edit' and then in the if / else block just actually append it with +=
| labelId={labelId} | ||
| selectorId={selectorId} | ||
| zoneId={zoneId} | ||
| showEditButton={showEditButton} |
There was a problem hiding this comment.
You can call the getEditButtonState directly and eliminate another variable 🤷
| showEditButton={showEditButton} | |
| showEditButton={!getEditButtonState(memberId, selectorsQuery, zonesQuery, labelsQuery)} |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/EditInfoPanelButton.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/EditInfoPanelButton.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/index.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/index.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/index.tsx
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (3)
packages/javascript/bh-shared-ui/src/routes/index.tsx (5)
zonesPath(17-17)labelsPath(18-18)selectorsPath(19-19)savePath(24-24)privilegeZonesPath(22-22)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(20-55)packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx (1)
AppLink(25-42)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/EditInfoPanelButton.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (1)
PZEditButton(54-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx (1)
17-21: LGTM! Clean public API refinement.The removal of
EntitySelectorsInformationandgetSavePathexports aligns with the refactoring that moves this logic to the newPZEditButtonmodule.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/index.tsx (1)
17-17: LGTM! Standard barrel export.Clean re-export pattern following project conventions.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/index.tsx (1)
19-20: LGTM! Public API properly updated.The export changes correctly expose the new
PZEditButtonmodule and maintainSelectedDetailsfrom Details, aligning with the refactoring objectives.
...s/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/EditInfoPanelButton.test.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx
Outdated
Show resolved
Hide resolved
|
Hey guys! Thanks for all the feedback! I implemented the suggested changes. Please let me know if you have any other suggestions! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (2)
64-68: Consider handling the disabled state more explicitly.When
saveLinkis undefined, the component passes an empty string toAppLink'stoprop. While the button is disabled in this state, passing an invalid route to a Link component may trigger console warnings from React Router.Consider one of these approaches:
Option 1: Render a non-link button when disabled:
-<Button asChild={!!saveLink} variant={'secondary'} disabled={!saveLink}> - <AppLink data-testid='privilege-zones_edit-button' to={saveLink || ''}> - Edit {titleSuffix} - </AppLink> -</Button> +{saveLink ? ( + <Button asChild variant={'secondary'}> + <AppLink data-testid='privilege-zones_edit-button' to={saveLink}> + Edit {titleSuffix} + </AppLink> + </Button> +) : ( + <Button variant={'secondary'} disabled data-testid='privilege-zones_edit-button'> + Edit {titleSuffix} + </Button> +)}Option 2: Use a non-empty fallback (if your routing supports it):
-<AppLink data-testid='privilege-zones_edit-button' to={saveLink || ''}> +<AppLink data-testid='privilege-zones_edit-button' to={saveLink || '#'}>
62-62: Fixed width may not accommodate internationalized text.The wrapper div uses a fixed width class
w-[6.75rem](108px). Button text like "Edit Selector" or "Edit Label" may overflow in languages with longer translations.Consider using
min-w-[6.75rem]or removing the fixed width constraint to allow the button to grow with content:-<div className='flex flex-col gap-4 basis-1/3 w-[6.75rem]'> +<div className='flex flex-col gap-4 basis-1/3 min-w-fit'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/EditInfoPanelButton.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/EditInfoPanelButton.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 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/PZEditButton/PZEditButton.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (3)
packages/javascript/bh-shared-ui/src/routes/index.tsx (5)
zonesPath(17-17)labelsPath(18-18)selectorsPath(19-19)savePath(24-24)privilegeZonesPath(22-22)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(20-55)packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx (1)
AppLink(25-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (3)
23-27: LGTM!The enum clearly defines the context-specific button labels and appropriately maps "Selector" to the user-facing term "Rule".
29-42: LGTM!The path construction logic correctly prioritizes labelId over zoneId and handles the selector scope appropriately. The early return when no valid path exists prevents invalid navigation.
44-52: LGTM!The precedence logic (selectorId > labelId > zoneId) now aligns with
getSavePathand provides sensible context-aware labels.
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Code LGTM, please make sure BHE is approved and passing before merging this though!
| zoneId: string | undefined, | ||
| labelId: string | undefined, | ||
| selectorId: string | undefined |
There was a problem hiding this comment.
Seems like zoneId,labelId and selectorId here and below as args in line 44 have string | undefined types, maybe we can create a type PZId = string | undefined and reuse it.
Even better, but maybe out of scope, would be perfect if we can abstract this even more to maybe the usePZPathParams hook and type it through out to maybe something more specific like the actual values as types. I see alot of 'labels' and 'zones' strings that can maybe be organized into types and referenced everywhere else including here.
There was a problem hiding this comment.
I like the idea but I think this PR has had enough fun for one day, might be worth tossing a service continuity ticket up to research!
There was a problem hiding this comment.
Sounds fair, I will go ahead and write up the ticket.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (2)
29-42: Path construction logic is correct.The function correctly prioritizes
labelIdoverzoneId(line 34) and properly validates path segments (line 37). The conditional selector path logic (line 39) is also sound.Optional: Use constant instead of string literal.
Line 35 compares
tagTypewith the string literal'zones'. For consistency and maintainability, consider using the imported constantzonesPathinstead.Apply this diff to use the constant:
- const tagPathId = tagType === 'zones' ? zoneId ?? '' : labelId ?? ''; + const tagPathId = tagType === zonesPath ? zoneId ?? '' : labelId ?? '';
54-71: Component implementation is correct and handles edge cases well.The component correctly:
- Uses
asChild={!!saveLink}for proper Radix UI Button behavior (line 64)- Controls visibility with
showEditButton(line 63)- Disables the button when no valid path exists (line 64)
- Renders dynamic button text using the
suffixhelper (line 66)Nitpick: Consider the fixed width.
Line 62 sets a fixed width of
w-[6.75rem]. If this is intentional for layout alignment, consider adding a comment explaining why. Otherwise, a more flexible width (e.g.,w-autoormin-w-[...]) might be more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/PZEditButton/PZEditButton.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/PZEditButton/PZEditButton.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (3)
packages/javascript/bh-shared-ui/src/routes/index.tsx (5)
zonesPath(17-17)labelsPath(18-18)selectorsPath(19-19)savePath(24-24)privilegeZonesPath(22-22)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(20-55)packages/javascript/bh-shared-ui/src/components/Navigation/AppLink.tsx (1)
AppLink(25-42)
⏰ 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 (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PZEditButton/PZEditButton.tsx (2)
23-27: Clean enum definition for dynamic button text.The
TitleSuffixenum clearly maps the context to user-facing labels, aligning perfectly with the PR objective of making the Edit button context-aware.
44-52: Suffix logic correctly determines button context.The precedence order (selector → label → zone) now correctly matches
getSavePath, and the function appropriately returns the corresponding user-facing label for each context.
Description
On Privilege Zones, the Edit button located at the right side of the screen should dynamically change depending on the view the customer is. If the customer is in "Zones", the button should read "Edit Zone". If the customer is in "Label", the button should read "Edit Label". If a "selector" is selected, the button should read "Edit Rule".
Motivation and Context
Resolves <TICKET_OR_ISSUE_NUMBER>
Solves BED-6624 https://specterops.atlassian.net/browse/BED-6624
How Has This Been Tested?
Tested locally and updated current test.
Screenshots (optional):
Edit.Button.BHCE.mov
Types of changes
Checklist:
Summary by CodeRabbit
Refactor
Tests