feat(UI): group management page badge and remove gm page entity badge etac disabled users - BED-7123 #2251
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR refactors EntityInfoPanel to accept explicit boolean props ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
catsiller
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx`:
- Around line 142-149: The user-facing label string in the Role-Based Filtering
block rendered when isRoleBasedFiltering is true contains a typo ("and
administrator"); update the label prop on the Badge component (inside the
isRoleBasedFiltering conditional) to read "an administrator" instead of "and
administrator" so the message becomes: "This account does not have access to
this page. Please contact an administrator if this message is in error."
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
144-144: Consider using a distinctdata-testidfor the group management badge.The current test ID
explore_entity-information-panel-badge-etac-filteringis copied fromEntityInfoPanel.tsxbut doesn't accurately describe this component's context. A page-specific ID likegroup-management-badge-etac-filteringwould improve test clarity and maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsxpackages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsxpackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoContent.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsxpackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoContent.test.tsxpackages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.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/components/GroupManagementContent/GroupManagementContent.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/components/GroupManagementContent/GroupManagementContent.tsx
📚 Learning: 2025-11-06T21:35:45.118Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:86-90
Timestamp: 2025-11-06T21:35:45.118Z
Learning: In CypherSearch.tsx (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the useLayoutEffect that directly sets aria-label on cypherEditorRef.current.cypherEditor.codemirror.contentDOM is necessary because the aria-label prop passed to the CypherEditor component (neo4j-cypher/react-codemirror) does not properly reach the correct DOM element for accessibility. The direct DOM manipulation is required to satisfy Axe DevTools requirements.
Applied to files:
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoContent.test.tsx (2)
packages/javascript/bh-shared-ui/src/store/edgeinfo/edgeSlice.ts (1)
SelectedEdge(25-31)packages/javascript/bh-shared-ui/src/views/Explore/providers/ObjectInfoPanelProvider/ObjectInfoPanelContextProvider.tsx (1)
ObjectInfoPanelContextProvider(22-32)
⏰ 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 (3)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (1)
48-50: LGTM!The addition of
isGroupManagementPagefollows the same pattern as the existingisPrivilegeZonesPagecheck. This correctly ensures the badge is hidden in the entity panel when on the group management page (where the badge is now displayed at the page level instead).packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoContent.test.tsx (2)
239-249: LGTM on the wrapper signature update.The updated signature correctly accepts the optional
hiddenEdgeparameter and forwards it toEdgeInfoContent.Minor note: The non-null assertion
selectedEdge!on line 247 is unnecessary sinceselectedEdgeis a required parameter in the destructured props.
351-388: Verify that the hidden edge test correctly exercises thehiddenEdgeprop.The test description states "displays contact admin message when hidden edge is true", but the
hiddenEdgeprop is not passed to the component. If thehiddenEdgeprop is meant to control this behavior, the test setup should includehiddenEdge={true}:- const screen = render(<EdgeInfoContentWithProvider selectedEdge={selectedEdgeHidden} />); + const screen = render(<EdgeInfoContentWithProvider selectedEdge={selectedEdgeHidden} hiddenEdge={true} />);If the hidden behavior is determined internally by the
selectedEdgeHiddenfixture data instead, please disregard this comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
...ges/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsx`:
- Line 34: The code uses location.pathname in the showPlaceholderMessage
computation but location is not defined; replace the global reference by
importing and calling React Router's useLocation and use that hook's return
value (e.g., const location = useLocation()) so showPlaceholderMessage uses
location.pathname reactively; update the EdgeInfoPane component to import
useLocation from 'react-router-dom' and reference that location instead of the
global, matching how usePZPathParams behaves and ensuring route changes
re-render correctly (keep the existing privilegeZonesPath constant usage).
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (1)
60-67: Consider clarifying theshowFilteringBannerprop semantics.The condition
!showFilteringBannerhides the badge whenshowFilteringBanneristrue, which reads counterintuitively. Based on the PR objectives, whenshowFilteringBanner={true}is passed (e.g., from Group Management), the badge is shown at the page level instead of within this panel.Consider renaming to
filteringBannerShownElsewhereorhideFilteringBannerInPanelto make the intention clearer, or add a brief doc comment explaining the logic.packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (1)
40-61: Consider the semantic clarity ofshowPlaceholderMessage.The variable
showPlaceholderMessageis derived fromlocation.pathname.includes(privilegeZonesPath)inusePZPathParams. The current name describes what happens (show placeholder) rather than why (being on privilege zones page), which can make the logic less clear to future readers.If this naming is intentional to decouple the component from page-specific awareness, it would benefit from a brief inline comment explaining the relationship.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/ui/src/views/Explore/GraphItemInformationPanel.tsxcmd/ui/src/views/GroupManagement/GroupManagement.tsxpackages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsxpackages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsxpackages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.tspackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabContent.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsxpackages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsxpackages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsxpackages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsxpackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts
📚 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/hooks/usePZParams/usePZPathParams.tsxpackages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsxpackages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsxpackages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts
📚 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:
cmd/ui/src/views/GroupManagement/GroupManagement.tsxcmd/ui/src/views/Explore/GraphItemInformationPanel.tsx
📚 Learning: 2025-11-06T21:35:45.118Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:86-90
Timestamp: 2025-11-06T21:35:45.118Z
Learning: In CypherSearch.tsx (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the useLayoutEffect that directly sets aria-label on cypherEditorRef.current.cypherEditor.codemirror.contentDOM is necessary because the aria-label prop passed to the CypherEditor component (neo4j-cypher/react-codemirror) does not properly reach the correct DOM element for accessibility. The direct DOM manipulation is required to satisfy Axe DevTools requirements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsx
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
cmd/ui/src/views/Explore/GraphItemInformationPanel.tsx
🧬 Code graph analysis (3)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
privilegeZonesPath(24-24)
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-103)
packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsx (1)
packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
privilegeZonesPath(24-24)
🔇 Additional comments (10)
packages/javascript/bh-shared-ui/src/mocks/factories/privilegeZones.ts (1)
208-208: LGTM!The mock property rename from
isPrivilegeZonesPagetoshowPlaceholderMessagecorrectly aligns with the updatedusePZPathParamshook return value.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetails.tsx (1)
41-52: LGTM!The explicit prop values correctly configure the EntityInfoPanel for the Privilege Zones context:
showPlaceholderMessage={true}displays the appropriate placeholder textshowFilteringBanner={false}hides the ETAC filtering badge as intendedThis approach with explicit props is cleaner than the previous URL-based detection.
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (2)
38-39: Good API design - explicit props instead of URL detection.Adding
showPlaceholderMessageandshowFilteringBanneras explicit props addresses the previous review feedback about avoiding URL-dependent logic. This makes the component more reusable and the behavior more predictable.
85-89: LGTM!The conditional placeholder text logic is clear and provides appropriate messaging based on context.
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
39-39: Semantic rename properly applied with all consumers updated.The rename from
isPrivilegeZonesPagetoshowPlaceholderMessageis correctly implemented. No stale references to the old name exist, and all consumers across the codebase—includingSimpleEnvironmentSelector,EntityInfoPanel, and components in the Privilege Zones views—have been updated to use the new name. The rename effectively describes the UI behavior rather than the page location, improving clarity.cmd/ui/src/views/GroupManagement/GroupManagement.tsx (1)
96-103: LGTM!The explicit prop values align well with the PR objectives:
showFilteringBanner={true}enables the ETAC filtering banner on the Group Management page, whileshowPlaceholderMessage={false}correctly disables the placeholder since this page has its own member selection context.packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (1)
73-73: LGTM!The usage of
showPlaceholderMessageis consistent throughout the component - correctly destructured from the hook, passed toselectedText, and used to conditionally render the "All Environments" option.Also applies to: 122-122, 163-172
cmd/ui/src/views/Explore/GraphItemInformationPanel.tsx (3)
35-43: LGTM!Explicitly setting
showPlaceholderMessage={false}andshowFilteringBanner={false}for the loading/empty state is appropriate for the Explore page, which should not display the ETAC filtering banner or privilege zones placeholder.
46-56: LGTM!Consistent prop usage for the error state path.
86-95: LGTM!Consistent prop usage for the node display path, completing the explicit configuration across all rendering branches.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/javascript/bh-shared-ui/src/views/Explore/EdgeInfo/EdgeInfoPane.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.test.tsx`:
- Line 69: Remove the leftover test debug statement screen.debug(undefined,
Infinity) from the EntityInfoPanel.test.tsx test; locate the call in the
EntityInfoPanel tests and delete that line so tests no longer print debug
output, then run the test suite to confirm no output or regressions.
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.test.tsx (3)
59-59: Removeasyncfrom describe block.The
asynckeyword ondescribeis not standard practice and is typically unnecessary. Test runners like Vitest don't expectdescribecallbacks to be async.Suggested fix
-describe('EntityInfoPanel', async () => { +describe('EntityInfoPanel', () => {
32-34: DataTable prop type mismatch.
DataTableexpectsReact.FC<EntityInfoDataTableProps>, but the test spreads an object fromazKeyVaultSections[0]. This works in the current tests only becauseselectedNodeis alwaysnull, soEntityInfoContent(and thusDataTable) is never rendered. Consider using a proper mock component for more robust tests.Suggested improvement
+const MockDataTable: React.FC<EntityInfoDataTableProps> = () => <div data-testid="mock-data-table" />; + const testProps: EntityInfoPanelProps = { - DataTable: { ...azKeyVaultSections[0] }, + DataTable: MockDataTable, };
60-60: Minor: Fix test description grammar and trailing spaces.The test descriptions have grammatical issues and trailing spaces:
- Line 60: awkward phrasing
- Line 76: trailing space
- Line 90: trailing space
Suggested improvements
- it('should not display a badge that role based filtering is applied but filtering banner is true', async () => { + it('should not display role-based filtering badge when filtering banner is enabled', async () => {- it('should display a message to select an object ', async () => { + it('should display a message to select an object', async () => {- it('should display a badge that role based filtering is applied on top of section ', async () => { + it('should display role-based filtering badge when filtering is active', async () => {Also applies to: 76-76, 90-90
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.test.tsxpackages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.test.tsxpackages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (2)
packages/javascript/bh-shared-ui/src/utils/content.ts (1)
EntityInfoDataTableProps(32-39)packages/javascript/bh-shared-ui/src/types.ts (1)
SelectedNode(103-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (4)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (3)
32-40: Good refactor: Props-based behavior control.The introduction of
showPlaceholderMessageandshowFilteringBanneras explicit props addresses the previous feedback about avoiding URL-dependent logic. This approach is cleaner and more reusable across different pages.Exporting
EntityInfoPanelPropsaligns with the package's API surface requirements. Based on learnings, public types should be exported for downstream consumers.
60-67: LGTM: Badge visibility logic is now prop-driven.The condition
!showPlaceholderMessage && !showFilteringBanner && isRoleBasedFilteringcorrectly hides the badge on pages that handle filtering differently (Group Management) while still showing it where appropriate.
85-89: LGTM: Placeholder message selection.The ternary correctly differentiates between the Privilege Zones placeholder text and the default
NoEntitySelectedMessagebased on theshowPlaceholderMessageprop.packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.test.tsx (1)
76-88: Good test coverage for the new props.The tests adequately cover the key scenarios:
- Badge hidden when
showFilteringBanner=true- Placeholder message shown when
showPlaceholderMessage=true- Badge shown when both flags are false and role-based filtering is active
This validates the core behavior changes in
EntityInfoPanel.Also applies to: 90-103
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.test.tsx
Outdated
Show resolved
Hide resolved
| priorityTables, | ||
| DataTable, | ||
| showPlaceholderMessage, | ||
| showFilteringBanner, |
There was a problem hiding this comment.
it is probably best practice to give these two props a default value of false so that we are always working with a boolean
| )} | ||
| data-testid='explore_entity-information-panel'> | ||
| {!isPrivilegeZonesPage && isRoleBasedFiltering && ( | ||
| {!showPlaceholderMessage && !showFilteringBanner && isRoleBasedFiltering && ( |
There was a problem hiding this comment.
To match with the new names of these props, the logic could be simplified here by just checking showFilteringBanner && isRoleBasedFiltering. This means if that prop is true, it will show the banner when filtering is applied, no matter the value of showPlaceholderMessage.
There was a problem hiding this comment.
tried removing !showPlaceholderMessage from line 60 but since the !showFilteringBanner && isRoleBasedFiltering check is returning back true, it is displaying on the Privilege Zones Page (which we do not want it to)
| className={defaultClasses} | ||
| selectedNode={null} | ||
| DataTable={EntityInfoDataTable} | ||
| showPlaceholderMessage={false} |
There was a problem hiding this comment.
if we set these to a default value of false, we can remove all cases where either is set to false
specter-flq
added
the
user interface
catsiller
force-pushed
the
BED-7123--etac-badge-group-management
branch
from
cfe947e to
c501b79
Compare
Description
Describe your changes in detail
Motivation and Context
Resolves https://specterops.atlassian.net/browse/BED-7123
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.