BED-6625 make the active border only on active selection #2017
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Holocraft
added
enhancement
Contributor
WalkthroughRefactors SelectedHighlight into its own component that derives active state from route params; updates consuming PrivilegeZones Details components and tests to use itemId/type-based API; removes SelectedHighlight from utils; and adds a new string field Changes
Sequence Diagram(s)sequenceDiagram
participant Route as Route / URL
participant Hook as usePZPathParams
participant Component as Details Component
participant Highlight as SelectedHighlight
Note over Route,Hook: URL params represent active tag/selector/member
Route->>Hook: params change (tagId/selectorId/memberId)
Hook-->>Highlight: exposes active ids
Component->>Highlight: render with itemId + type
Highlight->>Highlight: compare provided type+itemId to active ids
alt match
Highlight-->>Component: render highlight (data-testid includes type + itemId)
else no match
Highlight-->>Component: render nothing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
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: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-08-28T19:26:03.304ZApplied to files:
📚 Learning: 2025-08-25T20:12:35.629ZApplied to files:
📚 Learning: 2025-09-08T19:22:49.284ZApplied to files:
📚 Learning: 2025-08-11T23:51:32.709ZApplied to files:
📚 Learning: 2025-08-11T23:51:32.709ZApplied to files:
📚 Learning: 2025-09-08T19:24:33.396ZApplied to files:
📚 Learning: 2025-08-27T21:15:32.207ZApplied to files:
🧬 Code graph analysis (4)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (2)
⏰ 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)
🔇 Additional comments (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx (1)
19-204: Active highlight desync on refresh/deep linkWith this state initialised from
tagIdonly, any deep link, refresh, or browser back that lands on a selector or member route leaves the blue “active” border stuck on the tag list. That breaks the PR intent of showing the border only on the current item. Seed and keepactiveItemin sync with the route params so the highlight always follows the displayed detail.-import { FC, useContext, useState } from 'react'; +import { FC, useContext, useEffect, useState } from 'react'; @@ - const [activeItem, setActiveItem] = useState<{ id: string; type: 'tag' | 'selector' | 'member' } | undefined>( - tagId ? { id: tagId, type: 'tag' } : undefined - ); + const getActiveItemFromParams = () => { + if (memberId) return { id: String(memberId), type: 'member' as const }; + if (selectorId) return { id: String(selectorId), type: 'selector' as const }; + if (tagId) return { id: String(tagId), type: 'tag' as const }; + return undefined; + }; + + const [activeItem, setActiveItem] = useState< + { id: string; type: 'tag' | 'selector' | 'member' } | undefined + >(getActiveItemFromParams); + + useEffect(() => { + setActiveItem(getActiveItemFromParams()); + }, [memberId, selectorId, tagId]);
🧹 Nitpick comments (1)
packages/go/openapi/doc/openapi.json (1)
3365-3370: Add description/enum and an example for product_edition to tighten the contract.The new field is good, but it lacks allowed values and a sample. Document it as Community/Enterprise and add a 200 example for quick discovery.
Apply this diff:
@@ "server_version": { "type": "string" - }, + }, "product_edition": { - "type": "string" + "type": "string", + "description": "Edition of the running product.", + "enum": ["Community", "Enterprise"] } @@ "responses": { "200": { "description": "OK", "content": { "application/json": { "schema": { "type": "object", "properties": { "data": { "type": "object", "properties": { "API": { "type": "object", "properties": { "current_version": { "type": "string" }, "deprecated_version": { "type": "string" } } }, "server_version": { "type": "string" }, "product_edition": { "type": "string", "description": "Edition of the running product.", "enum": ["Community", "Enterprise"] } } } } - } + }, + "example": { + "data": { + "API": { + "current_version": "v2", + "deprecated_version": "v1" + }, + "server_version": "5.3.1", + "product_edition": "Enterprise" + } + } } } },Please confirm the backend for GET /api/version already returns product_edition so the spec and implementation stay in sync. If helpful, I can draft an e2e check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/api/src/test/integration/harnesses/citrixRDPHarness.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
packages/go/openapi/doc/openapi.json(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
📚 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/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
PR: SpecterOps/BloodHound#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/utils.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
PR: SpecterOps/BloodHound#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/utils.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
PR: SpecterOps/BloodHound#1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
🧬 Code graph analysis (5)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (2)
packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
SelectedHighlight(20-31)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
SelectedHighlight(20-31)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx (4)
packages/javascript/bh-shared-ui/src/routes/index.tsx (3)
privilegeZonesPath(22-22)detailsPath(23-23)selectorsPath(19-19)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
TagList(46-137)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx (1)
SelectorsList(91-151)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx (1)
MembersList(55-116)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
SelectedHighlight(20-31)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx (1)
TagList(46-137)packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(99-110)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). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
Outdated
Show resolved
Hide resolved
mistahj67
requested changes
Oct 30, 2025
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
Outdated
Show resolved
Hide resolved
specter-flq
reviewed
Oct 30, 2025
Contributor
There was a problem hiding this comment.
LGTM! Just that change that @mistahj67 caught and good to go
Contributor
|
(Out of scope but worth mentioning) I saw hardcoded color variables in some of the changed files ( although not in actual changes), also saw them used in other areas related to PZ. Would be good to have those as variables in our theme. If maybe they are close to an existing color then maybe change to that color for consistency if design approves. Maybe a backlog ticket. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.text.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/TagList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/Details.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/MembersList.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectorsList.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
PR: SpecterOps/BloodHound#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/SelectedHighlight.text.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
📚 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/SelectedHighlight.text.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.text.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
PR: SpecterOps/BloodHound#1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.text.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
SelectedHighlight(21-44)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(20-53)
⏰ 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 (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
21-44: Active highlight now keys off deepest path paramThe
memberId ? 'member' : selectorId ? 'selector' : 'tag'gating keeps the blue border on just the current leaf while parents stay gray, exactly what BED-6625 calls for. Nicely done.
Comment on lines
1
to
74
| import { vi } from 'vitest'; | ||
| import * as hooks from '../../../hooks'; | ||
| import { render, screen } from '../../../test-utils'; | ||
| import { SelectedHighlight } from './utils'; | ||
|
|
||
| vi.mock('../../../hooks', () => ({ | ||
| usePZPathParams: vi.fn(), | ||
| })); | ||
|
|
||
| const mockUsePZPathParams = hooks.usePZPathParams as unknown as ReturnType<typeof vi.fn>; | ||
|
|
||
| describe('SelectedHighlight', () => { | ||
| afterEach(() => { | ||
| vi.resetAllMocks(); | ||
| }); | ||
|
|
||
| it('renders highlight for the active tag', () => { | ||
| mockUsePZPathParams.mockReturnValue({ | ||
| tagId: '1', | ||
| selectorId: undefined, | ||
| memberId: undefined, | ||
| }); | ||
|
|
||
| render(<SelectedHighlight itemId='1' type='tag' />); | ||
|
|
||
| expect(screen.getByTestId('privilege-zones_details_tags-list_active-tags-item-1')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not render highlight for non-active tag', () => { | ||
| mockUsePZPathParams.mockReturnValue({ | ||
| tagId: '2', | ||
| selectorId: undefined, | ||
| memberId: undefined, | ||
| }); | ||
|
|
||
| render(<SelectedHighlight itemId='1' type='tag' />); | ||
|
|
||
| expect(screen.queryByTestId('privilege-zones_details_tags-list_active-tags-item-1')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders highlight only for active selector when selectorId is present', () => { | ||
| mockUsePZPathParams.mockReturnValue({ | ||
| tagId: '1', | ||
| selectorId: '5', | ||
| memberId: undefined, | ||
| }); | ||
|
|
||
| const { rerender } = render(<SelectedHighlight itemId='5' type='selector' />); | ||
| expect( | ||
| screen.getByTestId('privilege-zones_details_selectors-list_active-selectors-item-5') | ||
| ).toBeInTheDocument(); | ||
|
|
||
| rerender(<SelectedHighlight itemId='1' type='tag' />); | ||
| expect(screen.queryByTestId('privilege-zones_details_tags-list_active-tags-item-1')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders highlight only for active member when memberId is present', () => { | ||
| mockUsePZPathParams.mockReturnValue({ | ||
| tagId: '1', | ||
| selectorId: '2', | ||
| memberId: '3', | ||
| }); | ||
|
|
||
| const { rerender } = render(<SelectedHighlight itemId='3' type='member' />); | ||
| expect(screen.getByTestId('privilege-zones_details_members-list_active-members-item-3')).toBeInTheDocument(); | ||
|
|
||
| rerender(<SelectedHighlight itemId='2' type='selector' />); | ||
| expect( | ||
| screen.queryByTestId('privilege-zones_details_selectors-list_active-selectors-item-2') | ||
| ).not.toBeInTheDocument(); | ||
|
|
||
| rerender(<SelectedHighlight itemId='1' type='tag' />); | ||
| expect(screen.queryByTestId('privilege-zones_details_tags-list_active-tags-item-1')).not.toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Rename to SelectedHighlight.test.tsx so Vitest runs these specs
Because this file is named .text.tsx, our Vitest config (which only matches *.test.tsx/*.spec.tsx) will skip it, so the new coverage never executes. Please rename the file to .test.tsx before merging so these assertions actually run.
-// packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.text.tsx
+// packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsxCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.text.tsx
(lines 1-74): the test file is misnamed with a .text.tsx extension so Vitest
won't pick it up; rename the file to SelectedHighlight.test.tsx (update any
import paths or references if they exist) so the test runner includes and
executes these specs.
Contributor
There was a problem hiding this comment.
@Holocraft, thank you for addressing this! Once the file is renamed to .test.tsx, Vitest will discover and run these tests automatically. 🎯
/)/)
(-.-)
uu--uu
🧠 Learnings used
Learnt from: mistahj67
PR: SpecterOps/BloodHound#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.
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.
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx (2)
1-15: LGTM! Clean test setup with proper imports and mocking.The mock setup follows vitest patterns correctly and uses custom test-utils as recommended. The
afterEachcleanup ensures test isolation.Optional: Consider simpler type casting for the mock.
The type casting on line 10 works but is verbose. If desired, you could simplify to:
-const mockUsePZPathParams = hooks.usePZPathParams as unknown as ReturnType<typeof vi.fn>; +const mockUsePZPathParams = vi.mocked(hooks.usePZPathParams);This assumes
vi.mockedis available in your vitest version and provides better type inference.
41-74: LGTM! Excellent coverage of the activation priority hierarchy.These tests correctly verify that:
- When
selectorIdis present, only selectors are highlighted (not tags)- When
memberIdis present, only members are highlighted (not selectors or tags)The use of
rerenderefficiently tests multiple scenarios while maintaining clear assertions.Recommended: Add test coverage for the "nothing selected" edge case.
Consider adding a test case where all path params are
undefinedto explicitly verify the component returnsnullwhen nothing is selected:it('does not render highlight when no path params are present', () => { mockUsePZPathParams.mockReturnValue({ tagId: undefined, selectorId: undefined, memberId: undefined, }); render(<SelectedHighlight itemId='1' type='tag' />); expect(screen.queryByTestId('privilege-zones_details_tags-list_active-tags-item-1')).not.toBeInTheDocument(); });This makes the edge case behavior explicit and improves test documentation.
Optional: Consider testing with numeric itemId values.
The component accepts
itemId: string | number, but all tests use string IDs. While the component converts to string internally, testing with a numeric ID would verify the type conversion works correctly:it('renders highlight for active tag with numeric itemId', () => { mockUsePZPathParams.mockReturnValue({ tagId: '42', selectorId: undefined, memberId: undefined, }); render(<SelectedHighlight itemId={42} type='tag' />); expect(screen.getByTestId('privilege-zones_details_tags-list_active-tags-item-42')).toBeInTheDocument(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
PR: SpecterOps/BloodHound#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/SelectedHighlight.test.tsx
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (1)
SelectedHighlight(21-44)
⏰ 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-analysis
- GitHub Check: run-go-unit-tests
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedHighlight.test.tsx (1)
17-39: LGTM! Clear positive and negative test cases for tag activation.These tests properly verify that the highlight appears only when the
tagIdmatches theitemId, which is the core behavior for tag selection.
mistahj67
requested changes
Oct 30, 2025
Contributor
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Thanks for adding those tests! Code is looking solid! I had just 2 small polish requests and I think this is ready to rock!
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx
Outdated
Show resolved
Hide resolved
mistahj67
approved these changes
Oct 30, 2025
Contributor
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Code LGTM and works locally, thanks for your patience with my feedback. 🚀
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change makes it so the active selection in Privilege Zones details page has the active blue border for only the currently selected item.
Motivation and Context
Resolves <BED-6625>
To be more clear of what is currently selected, we limit the active blue border for only the currently selected item instead of giving that border to every selection in the tree. The gray selected background will still be there for prior selected items so that the user can see where they are in the tree.
How Has This Been Tested?
Fixed current unit tests to adhere to the new structure. Also manually tested.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Style