BED-5897: Fix Zone Management Back Button in Panel #1599
Conversation
WalkthroughThis update refactors the Entity Info panel system, consolidating and relocating components from a zone management-specific directory to a shared component structure. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectedDetails
participant EntityInfoPanel
participant EntityInfoContent
participant EntityInfoDataTableList
participant EntitySelectorsInformation
User->>SelectedDetails: Selects a member (with additionalSections)
SelectedDetails->>EntityInfoPanel: Passes selectedNode and additionalSections
EntityInfoPanel->>EntityInfoContent: Passes node info and additionalSections
EntityInfoContent->>EntityInfoDataTableList: Passes props including additionalSections
EntityInfoDataTableList-->>EntitySelectorsInformation: Renders if additionalSections is true
EntitySelectorsInformation->>API: Fetches selector info
EntitySelectorsInformation-->>User: Displays selectors with actions
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/SelectorsInfoPanelProvider/SelectorsInfoPanelContextProvider.tsx (1)
24-34: Optional: removecollapseAllSectionsinline allocationEach render recreates the arrow function, defeating some memoization benefit. Minor, but you can lift it into
useCallback:- const value = useMemo( - () => ({ - isSelectorsInfoPanelOpen, - setIsSelectorsInfoPanelOpen, - collapseAllSections: () => { - setIsSelectorsInfoPanelOpen(false); - }, - }), - [isSelectorsInfoPanelOpen] - ); + const collapseAllSections = useCallback(() => setIsSelectorsInfoPanelOpen(false), []); + + const value = useMemo( + () => ({ isSelectorsInfoPanelOpen, setIsSelectorsInfoPanelOpen, collapseAllSections }), + [isSelectorsInfoPanelOpen, collapseAllSections] + );packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx (1)
29-34:zoneManagementprop is currently unused
zoneManagementis destructured but never referenced, which will trigger eslint / tsc “unused variable” warnings and can be confusing to readers.If it’s planned for future logic, prefix with
_or leave a TODO; otherwise remove it for now.packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx (1)
68-73: Index-based menu state is brittle
indexchanges when the array is resorted/filtered, which can flip open popovers onto the wrong row.
Key off a stable identifier (selector.id) instead.-const [menuOpen, setMenuOpen] = useState<{ [key: number]: boolean }>({}); -... -const handleMenuClick = (index: number) => { +const [menuOpen, setMenuOpen] = useState<Record<number, boolean>>({}); +... +const handleMenuClick = (selectorId: number) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSction.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSection.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityObjectInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.test.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoContent.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoHeader.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanel.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContextProvider.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityObjectInformation.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/SelectorList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx(2 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/SelectorsInfoPanelProvider/SelectorsInfoPanelContext.ts(2 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/SelectorsInfoPanelProvider/SelectorsInfoPanelContextProvider.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/SelectorsInfoPanelProvider/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/index.ts(1 hunks)
💤 Files with no reviewable changes (8)
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityObjectInformation.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.test.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContextProvider.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/SelectorList.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoContent.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanel.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoHeader.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (15)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSction.test.tsx (1)
17-17: Import Path Simplification ApprovedThe updated
../../test-utilsimport correctly matches the new directory layout and is consistent with other EntityInfo tests.packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityObjectInformation.tsx (3)
18-18: Approve centralized hooks import
Consolidating hooks into a single import from../../hooksimproves maintainability and aligns with the new module structure.
19-19: Approve utilities import consolidation
GroupingEntityFieldandformatObjectInfoFieldsunder../../utilsis consistent with the shared utilities pattern.
20-26: Approve grouping of view components and context
Importing all related view entities from../../viewscentralizes component exports and reduces deep-relative paths.packages/javascript/bh-shared-ui/src/views/Explore/index.ts (1)
19-19: ```shell
#!/bin/bashList files in the Explore view directory
echo "=== Files in packages/javascript/bh-shared-ui/src/views/Explore ==="
ls -1 packages/javascript/bh-shared-ui/src/views/ExploreShow the first 200 lines of the Explore index to inspect exports
echo -e "\n=== Content of packages/javascript/bh-shared-ui/src/views/Explore/index.ts ==="
sed -n '1,200p' packages/javascript/bh-shared-ui/src/views/Explore/index.tsFind any other local imports from './EntityInfo'
echo -e "\n=== Local imports from './EntityInfo' ==="
rg "from ['"]./EntityInfo['"]" -n --glob ".ts" --glob ".tsx"</details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.test.tsx (1)</summary> `19-21`: **Approve shortened import paths in tests** Updating test imports to use `../../` instead of `../../../` keeps paths shallow and consistent with the new structure. </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSection.tsx (1)</summary> `20-20`: **Approve consolidated view imports** Merging `SubHeader` and `useCollapsibleSectionStyles` imports from `../../views` supports the centralized component library approach. </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx (1)</summary> `17-21`: **Approve import path simplifications** Switching to `../../` imports for tables, constants, hooks, types, and utils aligns with the flattened project structure and improves readability. </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/index.ts (1)</summary> `17-17`: **Ensure consumers are updated to use default export** Switching from a named export `EntityInfoPanel` to a default export may break existing import sites. Verify all modules import the component via default import (e.g. `import EntityInfoPanel from '.../EntityInfo'`) and remove any leftover named imports. </details> <details> <summary>packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/index.ts (1)</summary> `17-17`: **Approve new providers index barrel** The added `index.ts` re-exporting `SelectorsInfoPanelProvider` correctly centralizes the context API for zone management. This aligns with the refactor and simplifies imports. </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/index.ts (1)</summary> `82-84`: **Expose EntityInfo in central components index** Adding `export * from './EntityInfo'` and aliasing the default as `EntityInfoPanel` follows the existing export pattern and ensures the new component is discoverable. </details> <details> <summary>packages/javascript/bh-shared-ui/src/views/ZoneManagement/providers/SelectorsInfoPanelProvider/index.ts (1)</summary> `17-18`: **Barrel looks good** Straightforward re-export; keeps import paths tidy and avoids deep relative paths. 👍 </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx (1)</summary> `40-43`: **Ensure the provider always wraps this component** `handleCollapseAll` calls `setIsSelectorsInfoPanelOpen(false)`. If `Header` is ever rendered outside `SelectorsInfoPanelContextProvider`, React will throw `useContext(...) is undefined`. Please double-check every code-path (stories/tests included) to guarantee the provider is present. </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx (1)</summary> `40-44`: **Conditional uses route params safely, but relies on the broken flag** After fixing the signature, the `zoneManagement` gate will work. No change needed apart from the previous fix, but please verify that callers actually set `zoneManagement` when required. </details> <details> <summary>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (1)</summary> `66-72`: **Missing `zoneManagement` prop propagation** `EntityInfoContent` now supports a `zoneManagement` flag, but the call here omits it, meaning the selectors panel cannot appear from the main Explore pane. If this panel is meant to display selectors in Zone-Management mode, pass the prop explicitly; otherwise, consider removing the flag from `EntityInfoContent` to avoid confusion. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| import { act, render } from '../../test-utils'; | ||
|
|
||
| import userEvent from '@testing-library/user-event'; | ||
| import { AzureNodeKind } from '../../../graphSchema'; | ||
| import { ObjectInfoPanelContext } from '../providers/ObjectInfoPanelProvider'; | ||
| import { AzureNodeKind } from '../../graphSchema'; | ||
| import { ObjectInfoPanelContext } from '../../views'; |
There was a problem hiding this comment.
Context import mismatch in EntityInfoHeader test
The test imports and wraps ObjectInfoPanelContext, but the implementation now uses a SelectorsInfoPanelContext. This will cause the context value to be undefined. Update the test to import and provide the correct SelectorsInfoPanelContext (and its provider) from the new ZoneManagement/providers module.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx
around lines 18 to 22, the test imports and uses ObjectInfoPanelContext, but the
component now uses SelectorsInfoPanelContext. Replace the import of
ObjectInfoPanelContext with SelectorsInfoPanelContext from the
ZoneManagement/providers module, and update the test wrapper to provide the
correct SelectorsInfoPanelContext provider to ensure the context value is
properly defined during testing.
| import { ActiveDirectoryNodeKind, AzureNodeKind } from '../../graphSchema'; | ||
| import { render, screen, waitForElementToBeRemoved } from '../../test-utils'; | ||
| import { EntityKinds } from '../../utils'; | ||
| import { ObjectInfoPanelContextProvider } from '../../views'; |
There was a problem hiding this comment.
Context provider import mismatch in EntityInfoContent test
Tests still use ObjectInfoPanelContextProvider, but the refactor replaced it with SelectorsInfoPanelContextProvider. Adjust imports and wrapper component to use the new selectors info panel provider so the context is correctly applied.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.test.tsx
around lines 18 to 21, the test imports and uses ObjectInfoPanelContextProvider,
but the refactor replaced it with SelectorsInfoPanelContextProvider. Update the
import statement to import SelectorsInfoPanelContextProvider instead of
ObjectInfoPanelContextProvider, and replace all usages of
ObjectInfoPanelContextProvider in the test file with
SelectorsInfoPanelContextProvider to ensure the correct context provider is
applied.
...ws/ZoneManagement/providers/SelectorsInfoPanelProvider/SelectorsInfoPanelContextProvider.tsx
Outdated
Show resolved
Hide resolved
| if (memberQuery.data) { | ||
| const selectedNode = { | ||
| id: memberQuery.data.object_id, | ||
| name: memberQuery.data.name, | ||
| type: memberQuery.data.primary_kind as EntityKinds, | ||
| }; | ||
| return <EntityInfoPanel selectedNode={selectedNode} />; | ||
| } else if (selectorId !== undefined) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type safety for constructed selectedNode
memberQuery.data.primary_kind is cast to EntityKinds without runtime validation. If the API ever returns an unknown kind, the panel may crash downstream.
Minimal safeguard:
- type: memberQuery.data.primary_kind as EntityKinds,
+ type: (memberQuery.data.primary_kind as EntityKinds) ?? EntityKinds.Entity,or validate against Object.values(EntityKinds).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx
around lines 67 to 74, the selectedNode object casts
memberQuery.data.primary_kind to EntityKinds without runtime validation, risking
crashes if the API returns an unexpected kind. To fix this, add a runtime check
to verify that memberQuery.data.primary_kind is a valid EntityKinds value before
casting. If it is invalid, handle the case gracefully, such as by returning null
or a fallback UI, to ensure type safety and prevent downstream errors.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
Outdated
Show resolved
Hide resolved
| import { Popover, PopoverContent, PopoverTrigger } from '@bloodhoundenterprise/doodleui'; | ||
| import { useCallback, useState } from 'react'; | ||
| import { useQuery } from 'react-query'; | ||
| import { useParams } from 'react-router-dom'; | ||
| import { AppIcon } from '../../components/AppIcon'; | ||
| import { apiClient, cn, useAppNavigate } from '../../utils'; | ||
| import { getTagUrlValue, itemSkeletons } from '../../views'; |
There was a problem hiding this comment.
Import lacks React default – React.FC will not resolve
React.FC is a type that lives on the default export of react; bringing in only named hooks won’t put the React namespace in scope, so the file will not compile.
-import { useCallback, useState } from 'react';
+import React, { useCallback, useState } from 'react';📝 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.
| import { Popover, PopoverContent, PopoverTrigger } from '@bloodhoundenterprise/doodleui'; | |
| import { useCallback, useState } from 'react'; | |
| import { useQuery } from 'react-query'; | |
| import { useParams } from 'react-router-dom'; | |
| import { AppIcon } from '../../components/AppIcon'; | |
| import { apiClient, cn, useAppNavigate } from '../../utils'; | |
| import { getTagUrlValue, itemSkeletons } from '../../views'; | |
| import { Popover, PopoverContent, PopoverTrigger } from '@bloodhoundenterprise/doodleui'; | |
| import React, { useCallback, useState } from 'react'; | |
| import { useQuery } from 'react-query'; | |
| import { useParams } from 'react-router-dom'; | |
| import { AppIcon } from '../../components/AppIcon'; | |
| import { apiClient, cn, useAppNavigate } from '../../utils'; | |
| import { getTagUrlValue, itemSkeletons } from '../../views'; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
around lines 17 to 23, the import statement only imports named exports from
'react' but does not import the default React export. To fix the issue, add an
import for the default React export (e.g., import React from 'react') alongside
the named imports so that React.FC and other React namespace types are available
and the file compiles correctly.
| const memberInfoQuery = useQuery(['asset-group-member-info'], () => { | ||
| return apiClient.getAssetGroupTagMemberInfo(tagId, memberId).then((res) => { | ||
| return res.data.data['member']; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
react-query key is not parameterised – stale-data hazard
Using the static key ['asset-group-member-info'] causes every selector panel instance (different tag/member) to share one cache entry, leading to incorrect data and race conditions.
-const memberInfoQuery = useQuery(['asset-group-member-info'], () => {
+const memberInfoQuery = useQuery(
+ ['asset-group-member-info', tagId, memberId],
+ () => apiClient.getAssetGroupTagMemberInfo(tagId, memberId).then((res) => res.data.data.member)
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const memberInfoQuery = useQuery(['asset-group-member-info'], () => { | |
| return apiClient.getAssetGroupTagMemberInfo(tagId, memberId).then((res) => { | |
| return res.data.data['member']; | |
| }); | |
| }); | |
| const memberInfoQuery = useQuery( | |
| ['asset-group-member-info', tagId, memberId], | |
| () => apiClient.getAssetGroupTagMemberInfo(tagId, memberId).then((res) => res.data.data.member) | |
| ); |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
around lines 62 to 66, the react-query key is static and not parameterized,
causing all queries to share the same cache entry. Fix this by including tagId
and memberId in the query key array to uniquely identify each query instance and
prevent stale or incorrect data from being shared.
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx
Outdated
Show resolved
Hide resolved
...i/src/views/ZoneManagement/providers/SelectorsInfoPanelProvider/SelectorsInfoPanelContext.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const handleCollapseAll = () => { | ||
| setIsObjectInfoPanelOpen(false); | ||
| setIsSelectorsInfoPanelOpen(false); |
There was a problem hiding this comment.
this is not working when Collapse All button is clicked for the Selectors section (but works for the Object Information section)
| () => ({ | ||
| isSelectorsInfoPanelOpen, | ||
| setIsSelectorsInfoPanelOpen, | ||
| collapseAllSections: () => { |
There was a problem hiding this comment.
tried this inside the Context Provider (collapseAllSections) but no luck in collapsing the Selectors section
catsiller
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx (1)
51-55: Critical: Fix react-query cache key to prevent data contamination.The static cache key
['asset-group-member-info']causes all component instances to share the same cached data, leading to incorrect information being displayed when switching between different tags/members.Apply this diff to parameterize the cache key:
-const memberInfoQuery = useQuery(['asset-group-member-info'], () => { +const memberInfoQuery = useQuery(['asset-group-member-info', tagId, memberId], () => { return apiClient.getAssetGroupTagMemberInfo(tagId!!, memberId!!).then((res) => { return res.data.data['member']; }); });
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx (2)
84-89: Improve error state styling and accessibility.The error message lacks proper semantic structure and could benefit from better styling consistency.
Consider using a more semantic error component:
-return ( - <li className='border-y-[1px] border-neutral-light-3 dark:border-neutral-dark-3 relative h-10 pl-2'> - <span className='text-base'>There was an error fetching this data</span> - </li> -); +return ( + <div role="alert" className='p-4 text-center text-red-600 dark:text-red-400'> + <span>Unable to load selector information. Please try again.</span> + </div> +);
117-118: Optimize popover close handlers.The current approach clears all menu states when any popover closes, which could interfere with user interactions if multiple popovers are open.
Consider closing only the specific popover:
-onInteractOutside={() => setMenuOpen({})} -onEscapeKeyDown={() => setMenuOpen({})}> +onInteractOutside={() => setMenuOpen(prev => ({ ...prev, [index]: false }))} +onEscapeKeyDown={() => setMenuOpen(prev => ({ ...prev, [index]: false }))}>packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx (1)
67-67: Remove debug statement from test.The
screen.debug()call should be removed as it's not needed in production tests and can clutter test output.-screen.debug(undefined, Infinity);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx(4 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/InfoStyles/Pane.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/javascript/bh-shared-ui/src/views/Explore/InfoStyles/Pane.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
[error] 52-52: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
[error] 52-52: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx (2)
72-90: Address skipped tests to ensure proper coverage.Two important tests for the zoneManagement functionality are currently skipped. This reduces test coverage for critical conditional rendering logic.
These tests verify the core functionality introduced in this PR. Please unskip and fix these tests to ensure the zoneManagement prop works correctly.
If there are specific issues preventing these tests from running, please document them or fix the underlying problems. The conditional rendering of the Selectors section is a key feature that should be properly tested.
22-55: Well-structured API mocking setup.The MSW server configuration properly mocks all the required endpoints for testing the component. The mock responses are appropriate for the test scenarios.
| }; | ||
|
|
||
| const memberInfoQuery = useQuery(['asset-group-member-info'], () => { | ||
| return apiClient.getAssetGroupTagMemberInfo(tagId!!, memberId!!).then((res) => { |
There was a problem hiding this comment.
Remove double non-null assertions for safer code.
The double non-null assertions (tagId!!, memberId!!) are flagged by the linter and could cause runtime issues if these values are actually null/undefined.
Replace with proper null checks or single assertions:
-return apiClient.getAssetGroupTagMemberInfo(tagId!!, memberId!!).then((res) => {
+return apiClient.getAssetGroupTagMemberInfo(tagId!, memberId!).then((res) => {Or better yet, add proper validation:
-const memberInfoQuery = useQuery(['asset-group-member-info', tagId, memberId], () => {
+const memberInfoQuery = useQuery(
+ ['asset-group-member-info', tagId, memberId],
+ () => {
+ if (!tagId || !memberId) {
+ throw new Error('Missing required parameters');
+ }
return apiClient.getAssetGroupTagMemberInfo(tagId, memberId).then((res) => {
return res.data.data['member'];
});
- });
+ },
+ { enabled: !!(tagId && memberId) }
+);📝 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.
| return apiClient.getAssetGroupTagMemberInfo(tagId!!, memberId!!).then((res) => { | |
| -const memberInfoQuery = useQuery(['asset-group-member-info', tagId, memberId], () => { | |
| - return apiClient.getAssetGroupTagMemberInfo(tagId!!, memberId!!).then((res) => { | |
| - return res.data.data['member']; | |
| - }); | |
| -}); | |
| +const memberInfoQuery = useQuery( | |
| + ['asset-group-member-info', tagId, memberId], | |
| + () => { | |
| + if (!tagId || !memberId) { | |
| + throw new Error('Missing required parameters'); | |
| + } | |
| + return apiClient.getAssetGroupTagMemberInfo(tagId, memberId).then((res) => { | |
| + return res.data.data['member']; | |
| + }); | |
| + }, | |
| + { enabled: !!(tagId && memberId) } | |
| +); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
[error] 52-52: Forbidden extra non-null assertion.
Safe fix: Remove extra non-null assertion.
(lint/suspicious/noExtraNonNullAssertion)
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
at line 52, remove the double non-null assertions (!!) on tagId and memberId as
they can cause runtime errors if the values are null or undefined. Instead, add
proper null or undefined checks before calling
apiClient.getAssetGroupTagMemberInfo, or use single non-null assertions if you
are certain the values are not null. Ideally, validate these variables earlier
in the code to ensure they are defined before this call.
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 (2)
packages/go/analysis/azure/post.go(1 hunks)packages/go/analysis/azure/role_approver.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (1)
packages/go/analysis/azure/role_approver.go (1)
96-107: Consider handling potential type mismatches gracefully.If the approver properties exist but are not string slices (e.g., due to data corruption or schema changes), the current implementation will fail processing for the entire role. Consider adding more robust error handling or type checking.
#!/bin/bash # Description: Check if there are any existing patterns for handling property type mismatches in the codebase # Search for similar property retrieval patterns with error handling rg -A 5 "Properties\.Get.*StringSlice\(\)" --type go # Look for any custom error handling for property type mismatches ast-grep --pattern 'if $_, err := $_.Properties.Get($_).StringSlice(); err != nil { $$$ }'
catsiller
added
the
user interface
catsiller
force-pushed
the
BED-5897-back-button-fix-entity-panel
branch
2 times, most recently
from
60c979a to
8f3e380
Compare
| sx?: SxProps; | ||
| zoneManagement?: boolean; |
There was a problem hiding this comment.
I dislike this prop - as a developer it is not immediately obvious what effect setting this prop to true or false will have on the component.
From reviewing the PR I am gathering that we have different use cases for what should happen when expanding/collapsing the various sections within an EntityInfoPanel. And we have different sections to display depending on where this component is being rendered.
If we are going to maintain a single set of EntityInfo* components we need to come up with a set of props that gives us the flexibility we need for all of our use cases.
| @@ -33,6 +33,7 @@ const EntityInfoDataTable: React.FC<EntityInfoDataTableProps> = ({ | |||
|
|
|||
| const endpoint = queryType ? entityRelationshipEndpoints[queryType] : undefined; | |||
| const isExpandedPanelSection = (expandedPanelSections as string[]).includes(label); | |||
| const zoneManagementPage = location.pathname.includes('zone-management'); | |||
There was a problem hiding this comment.
We should avoid this type of coupling. This EntityInfoDataTable component should not behave differently based on the current location. Instead we should explicitly control the behavior of the component via props.
There was a problem hiding this comment.
I appreciate that we are consolidating components but I have concerns about the approach. I suggest we consider what use cases we need to satisfy with this EntityInfo component and come up with an appropriate array of props required to implement those use cases.
If we aren't willing to handle that deep of a refactor then I'd suggest keeping the Zone Management page specific implementation of EntityInfo separate for now.
catsiller
force-pushed
the
BED-5897-back-button-fix-entity-panel
branch
from
8f3e380 to
eb5955a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx (1)
38-38: Improve readability of conditional array push.Using the
&&operator for side effects like array mutation is unconventional and can be confusing.- additionalSections && tables.push({ id, label: 'Selectors' }); + if (additionalSections) { + tables.push({ id, label: 'Selectors' }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSction.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSection.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityObjectInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/utils/content.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/EntityInfo/EntityInfoDataTableList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/Explore/InfoStyles/Pane.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/index.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.test.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoContent.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoHeader.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanel.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContext.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContextProvider.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityObjectInformation.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/SelectorList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx(2 hunks)
💤 Files with no reviewable changes (11)
- packages/javascript/bh-shared-ui/src/views/Explore/index.ts
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.test.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContextProvider.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContext.ts
- packages/javascript/bh-shared-ui/src/views/Explore/EntityInfo/EntityInfoDataTableList.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityObjectInformation.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoHeader.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/SelectorList.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoContent.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanel.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityObjectInformation.tsx
- packages/javascript/bh-shared-ui/src/utils/content.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSction.test.tsx
- packages/javascript/bh-shared-ui/src/views/Explore/InfoStyles/Pane.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSection.tsx
- packages/javascript/bh-shared-ui/src/components/index.ts
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.test.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/index.ts
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx (2)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx (1)
EntityInfoContentProps(22-27)packages/javascript/bh-shared-ui/src/utils/content.ts (2)
EntityKinds(48-48)allSections(158-1038)
🪛 Biome (1.9.4)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx
[error] 112-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Process CLA
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: CLAssistant
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx (1)
61-61: Good improvement to component flexibility!Making the
onClickprop optional and conditionally rendering interactive vs non-interactive list items is a cleaner approach than having a default no-op function. This allows the component to be used for display-only tables without unnecessary click handlers.Also applies to: 93-131
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.test.tsx (1)
1-208: Comprehensive test coverage!The test suite provides thorough coverage including:
- Count aggregation for nested sections
- Error handling with proper error icon display
- Edge cases like empty/undefined counts
- Specific business logic verification (Vault Readers counts)
Well-structured with proper mock server setup and teardown.
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx
Outdated
Show resolved
Hide resolved
| key={nestedIndex} | ||
| parentLabels={[...(parentLabels as string[]), label]} | ||
| {...nestedSection} | ||
| data-testid='entity-info-data-table-graphed' |
There was a problem hiding this comment.
Remove ineffective data-testid attribute.
The data-testid attribute is being passed as a prop to the EntityInfoDataTableGraphed component, but this component doesn't use or forward this prop, making it ineffective for testing.
- data-testid='entity-info-data-table-graphed'If you need this for testing, consider adding proper testid support within the component implementation.
📝 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.
| data-testid='entity-info-data-table-graphed' |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx
at line 152, the data-testid attribute is passed as a prop but not used or
forwarded inside the component, rendering it ineffective for testing. To fix
this, either remove the data-testid prop from where the component is used or
update the EntityInfoDataTableGraphed component to accept and apply the
data-testid attribute to a relevant DOM element within its render method to
support testing.
| <> | ||
| <EntityInfoDataTable | ||
| {...nestedSection} | ||
| additionalSections | ||
| data-testid='entity-info-data-table' | ||
| key={nestedIndex} | ||
| parentLabels={[...(parentLabels as string[]), label]} | ||
| /> | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary Fragment wrapper and fix data-testid.
The Fragment wrapper serves no purpose here, and the data-testid attribute won't work as a prop to the component.
- sections.map((nestedSection: EntityInfoDataTableProps, nestedIndex: number) => (
- <>
- <EntityInfoDataTable
- {...nestedSection}
- additionalSections
- data-testid='entity-info-data-table'
- key={nestedIndex}
- parentLabels={[...(parentLabels as string[]), label]}
- />
- </>
- ))}
+ sections.map((nestedSection: EntityInfoDataTableProps, nestedIndex: number) => (
+ <EntityInfoDataTable
+ {...nestedSection}
+ additionalSections
+ key={nestedIndex}
+ parentLabels={[...(parentLabels as string[]), label]}
+ />
+ ))}📝 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.
| <> | |
| <EntityInfoDataTable | |
| {...nestedSection} | |
| additionalSections | |
| data-testid='entity-info-data-table' | |
| key={nestedIndex} | |
| parentLabels={[...(parentLabels as string[]), label]} | |
| /> | |
| </> | |
| ))} | |
| sections.map((nestedSection: EntityInfoDataTableProps, nestedIndex: number) => ( | |
| <EntityInfoDataTable | |
| {...nestedSection} | |
| additionalSections | |
| key={nestedIndex} | |
| parentLabels={[...(parentLabels as string[]), label]} | |
| /> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 112-123: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx
around lines 114 to 123, remove the unnecessary React Fragment wrapper
surrounding the EntityInfoDataTable component as it is redundant. Also, remove
the data-testid attribute from the component props since it does not work as a
prop; if needed for testing, place the data-testid on a valid DOM element inside
the component instead.
d91a1b9 to
f6e0748
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx (1)
150-150: Remove ineffective data-testid attribute.The
data-testidattribute is being passed as a prop but this component doesn't accept or forward it.
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx (1)
146-154: Use optional chaining for cleaner code.Consider using optional chaining for more concise code.
- {sections && - sections.map((nestedSection: EntityInfoDataTableProps, nestedIndex: number) => ( + {sections?.map((nestedSection: EntityInfoDataTableProps, nestedIndex: number) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSction.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSection.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx(4 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityObjectInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/utils/content.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/EntityInfo/EntityInfoDataTableList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/Explore/InfoStyles/Pane.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/index.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.test.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoContent.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoHeader.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanel.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContext.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContextProvider.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityObjectInformation.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/SelectorList.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx(2 hunks)
💤 Files with no reviewable changes (11)
- packages/javascript/bh-shared-ui/src/views/Explore/index.ts
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.test.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityObjectInformation.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContextProvider.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanelContext.ts
- packages/javascript/bh-shared-ui/src/views/Explore/EntityInfo/EntityInfoDataTableList.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/SelectorList.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoCollapsibleSection.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoPanel.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoHeader.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/EntityInfo/EntityInfoContent.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSection.tsx
- packages/javascript/bh-shared-ui/src/utils/content.ts
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoCollapsibleSction.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/index.ts
- packages/javascript/bh-shared-ui/src/views/Explore/InfoStyles/Pane.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.tsx
- packages/javascript/bh-shared-ui/src/components/index.ts
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoHeader.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityObjectInformation.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/SelectedDetails.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoContent.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
- packages/javascript/bh-shared-ui/src/components/InfiniteScrollingTable/InfiniteScrollingTable.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTable.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.test.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx
- packages/javascript/bh-shared-ui/src/components/EntityInfo/EntitySelectorsInformation.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx
[error] 146-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (1)
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx (1)
24-157: Well-structured component implementation!The component effectively consolidates the entity info panel functionality and properly manages URL parameters for expanded sections, which aligns with the PR objectives of fixing the back button functionality in the Zone Management panel.
| countQuery.data.forEach((sectionData: any) => { | ||
| if (sectionData.countLabel === countLabel) count = sectionData.count; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with proper type definition.
Avoid using any type as it defeats TypeScript's type safety.
Define a proper type for the section data:
- countQuery.data.forEach((sectionData: any) => {
+ countQuery.data.forEach((sectionData: { countLabel?: string; count?: number }) => {📝 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.
| countQuery.data.forEach((sectionData: any) => { | |
| if (sectionData.countLabel === countLabel) count = sectionData.count; | |
| }); | |
| countQuery.data.forEach((sectionData: { countLabel?: string; count?: number }) => { | |
| if (sectionData.countLabel === countLabel) count = sectionData.count; | |
| }); |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx
around lines 117 to 119, the variable sectionData is typed as any, which
bypasses TypeScript's type safety. Define a proper interface or type for
sectionData that includes at least the properties countLabel and count with
their correct types, then replace the any type annotation with this new type to
ensure type safety and better code clarity.
| const isExpandedPanelSection = (expandedPanelSections as string[]).includes(label); | ||
|
|
||
| const countQuery = useQuery( | ||
| ['relatedCount', label, id], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and query key dependencies.
The type assertion on line 36 could be unsafe. Additionally, the query key should include all dependencies to ensure proper cache invalidation.
- const isExpandedPanelSection = (expandedPanelSections as string[]).includes(label);
+ const isExpandedPanelSection = Array.isArray(expandedPanelSections) && expandedPanelSections.includes(label);
const countQuery = useQuery(
- ['relatedCount', label, id],
+ ['relatedCount', label, id, queryType, sections],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isExpandedPanelSection = (expandedPanelSections as string[]).includes(label); | |
| const countQuery = useQuery( | |
| ['relatedCount', label, id], | |
| const isExpandedPanelSection = Array.isArray(expandedPanelSections) && expandedPanelSections.includes(label); | |
| const countQuery = useQuery( | |
| ['relatedCount', label, id, queryType, sections], |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableGraphed.tsx
around lines 36 to 39, avoid using an unsafe type assertion for
expandedPanelSections by properly typing it or adding a runtime check to ensure
it is an array of strings. Also, update the useQuery key to include all relevant
dependencies, such as id and label, to guarantee correct cache invalidation and
data fetching behavior.
f6e0748 to
0c6eeb6
Compare
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoDataTableList.tsx
Outdated
Show resolved
Hide resolved
catsiller
force-pushed
the
BED-5897-back-button-fix-entity-panel
branch
from
0c6eeb6 to
d240739
Compare
catsiller
force-pushed
the
BED-5897-back-button-fix-entity-panel
branch
from
d240739 to
c8809ea
Compare
| additionalTables?: [ | ||
| { | ||
| sectionProps: EntityInfoDataTableProps; | ||
| TableComponent: React.FC<EntityInfoDataTableProps>; | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
| additionalTables?: [ | |
| { | |
| sectionProps: EntityInfoDataTableProps; | |
| TableComponent: React.FC<EntityInfoDataTableProps>; | |
| }, | |
| ]; | |
| additionalTables?: | |
| { | |
| sectionProps: EntityInfoDataTableProps; | |
| TableComponent: React.FC<EntityInfoDataTableProps>; | |
| }[] |
I believe what you have typed is an array with a single element but what I think you want is an array with multiple elements
| additionalTables?: [ | ||
| { | ||
| sectionProps: EntityInfoDataTableProps; | ||
| TableComponent: React.FC<EntityInfoDataTableProps>; | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
| additionalTables?: [ | |
| { | |
| sectionProps: EntityInfoDataTableProps; | |
| TableComponent: React.FC<EntityInfoDataTableProps>; | |
| }, | |
| ]; | |
| additionalTables?: | |
| { | |
| sectionProps: EntityInfoDataTableProps; | |
| TableComponent: React.FC<EntityInfoDataTableProps>; | |
| }[] |
There was a problem hiding this comment.
But maybe it makes sense to import this type from bh-shared-ui?
There was a problem hiding this comment.
I agree with importing EntityInfoContentProps from bh-shared-ui and have updated!
|
At a glance it is not too obvious to me what the difference between EntityInfoDataTableGraphed and EntityInfoDataTable is. Would you mind breaking it down? |
Excellent point @elikmiller ! EntityInfoDataTableGraphed is on the Explore page which visually displays a graph and EntityInfoDataTable is on the Privilege Zones page which is displayed in the table on click (no graph connected) One main goal with dividing these was that the EntityInfoDataTable only needs one search param vs EntityInfoDataTableGraphed requires more search params Thoughts on dividing these vs one component? Better names if keeping divided? Thank you! |
Description
Original issue:
On the Zone Management page, in panel on the furthest right on the page, if section (like Selectors) was opened and then the collapse all button was pressed (and collapsed all sections), the back button would not take user back to the open Selectors section
Motivation and Context
Resolves BED-5897
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
Enhancements
Bug Fixes
Tests
Refactor
Chores