feat(SigmaChart): add pathfinding edge filter context menu option #1631
Conversation
WalkthroughThis change introduces right-click context menu support for graph edges in the UI, modularizes context menu rendering for nodes and edges, and refactors pathfinding filter logic for better state management and usability. Several utility functions are added or moved, and related hooks, types, and tests are updated to reflect the new event handling and filtering capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GraphEdgeEvents
participant SigmaChart
participant ContextMenu
User->>GraphEdgeEvents: Right-click on edge
GraphEdgeEvents->>SigmaChart: onRightClickEdge(payload)
SigmaChart->>ContextMenu: handleContextMenu(payload)
ContextMenu->>ContextMenu: Render edge-specific menu items
sequenceDiagram
participant User
participant ContextMenu
participant PathfindingFilters
User->>ContextMenu: Select "Filter out Edge"
ContextMenu->>PathfindingFilters: handleRemoveEdgeType(edgeType)
PathfindingFilters->>PathfindingFilters: Update selectedFilters and apply
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
✨ 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 (
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -68,34 +61,3 @@ describe('getNodeRadius', () => { | |||
| expect(getNodeRadius(false, 1, nodeSize)).not.toEqual(getNodeRadius(false, 0.5, nodeSize)); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('preventAllDefaults', () => { | |||
There was a problem hiding this comment.
I am not sure why I originally put this in the rendering utils. Moving it to a more appropriate module.
| }; | ||
|
|
||
| const ContextMenu: FC<{ | ||
| contextMenu: Coordinates | null; |
There was a problem hiding this comment.
Switched from the inline { mouseX, mouseY } to the Coordinates interface, which is already compatible with event mouse data (the source of this prop).
| </> | ||
| }> | ||
| <MenuItem sx={{ justifyContent: 'space-between' }} => e.stopPropagation()}> | ||
| <MenuItem | ||
| sx={{ justifyContent: 'space-between', minWidth: '8rem' }} |
There was a problem hiding this comment.
min-width ensures that the submenu arrow isn't right up against the Copy when no other options are shown.
packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/utils.ts
Outdated
Show resolved
Hide resolved
d146343 to
d4726fa
Compare
| selectedItemType: 'node', | ||
| } as any; | ||
|
|
||
| const fakeSelectedEdge = { |
There was a problem hiding this comment.
Many new variables. Caching mocks to keep test implementation less noisey.
| @@ -66,7 +83,14 @@ beforeAll(() => server.listen()); | |||
| afterEach(() => server.resetHandlers()); | |||
| afterAll(() => server.close()); | |||
|
|
|||
| const setup = async (permissions?: Permission[], primarySearch?: string, secondarySearch?: string) => { | |||
| interface SetupOptions { | |||
There was a problem hiding this comment.
Options object is a little more flexible and scales better, since additional params are needed now.
| } as any); | ||
|
|
||
| const mockPathfindingFilters: PathfindingFilters = { |
There was a problem hiding this comment.
This is now a required prop for the ContextMenu.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }; | ||
|
|
||
| describe('ContextMenu', async () => { | ||
| describe('ContextMenu - Nodes', async () => { | ||
| beforeEach(() => mockSelectedItemQuery.mockReturnValue(fakeSelectedNode)); |
There was a problem hiding this comment.
Now that there are scenarios where the return varies (for 'edge' tests), this mock is selectively applied.
| import { selectOwnedAssetGroupId, selectTierZeroAssetGroupId } from 'src/ducks/assetgroups/reducer'; | ||
| import { useAppSelector } from 'src/store'; | ||
| import AssetGroupMenuItem from './AssetGroupMenuItem'; | ||
| import CopyMenuItem from './CopyMenuItem'; | ||
|
|
||
| const ContextMenu: FC<{ |
There was a problem hiding this comment.
This component has been modified to show the ContextMenu when right clicking a node in any tab or an edge if in the pathfiltering tab. I also refactored for modularity, moving many of the menu items into the shared lib. Not all of it can be moved since it relies on Redux state, but this is a start.
| @@ -93,6 +94,7 @@ const GraphView: FC = () => { | |||
| const sigmaChartRef = useRef<any>(null); | |||
| 6302 |
|
||
| const customIcons = useCustomNodeKinds({ select: transformIconDictionary }); | |||
| const pathfindingFilters = usePathfindingFilters(); | |||
There was a problem hiding this comment.
This was lifted from a child so that it could be shared by the ExploreSearch and the ContextMenu. We should get an alternative to Redux for sharable state. React Contexts are ok, but there are some nice, simple options out there.
| @@ -27,13 +27,6 @@ export const mapParamsToFilters = (params: string[], initial: EdgeCheckboxType[] | |||
| })); | |||
| }; | |||
|
|
|||
| export const compareEdgeTypes = (initial: string[], comparison: string[]): boolean => { | |||
There was a problem hiding this comment.
This was generalized into areArraysSimilar and moved to the utils folder.
d4726fa to
366bba0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
366bba0 to
0f6bb6f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1a05112 to
94cc2ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/ui/src/views/Explore/ContextMenu/ContextMenu.test.tsx (2)
31-54: Consider using factory functions for better maintainability.The approach of using
JSON.parse(JSON.stringify())for creating mock variations works but is fragile and not type-safe. Consider using factory functions for better maintainability and type safety.-const fakeSelectedNonFilterable = JSON.parse(JSON.stringify(fakeSelectedEdge)); -fakeSelectedNonFilterable.selectedItemQuery.data.id = '123_CrossForestTrust_456'; - -const fakeSelectedNonEdge = JSON.parse(JSON.stringify(fakeSelectedEdge)); -fakeSelectedNonEdge.selectedItemQuery.data.id = '345'; +const createFakeSelectedEdge = (id: string, objectId: string = fakeSelectedItemId) => ({ + selectedItemQuery: { + data: { + objectId, + id, + }, + }, + selectedItemType: 'edge', +} as any); + +const fakeSelectedEdge = createFakeSelectedEdge('123_MemberOf_456'); +const fakeSelectedNonFilterable = createFakeSelectedEdge('123_CrossForestTrust_456'); +const fakeSelectedNonEdge = createFakeSelectedEdge('345');
296-296: Consider verifying the exact parameters passed to the handler.The test verifies that
handleRemoveEdgeTypeis called, but doesn't verify the parameters. Consider checking that the correct edge type is passed to ensure the filtering logic works as expected.- expect(mockPathfindingFilters.handleRemoveEdgeType).toBeCalled(); + expect(mockPathfindingFilters.handleRemoveEdgeType).toBeCalledWith('MemberOf');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx(5 hunks)cmd/ui/src/components/SigmaChart/GraphEvents.tsx(1 hunks)cmd/ui/src/components/SigmaChart/SigmaChart.tsx(3 hunks)cmd/ui/src/rendering/utils/utils.test.ts(1 hunks)cmd/ui/src/rendering/utils/utils.ts(1 hunks)cmd/ui/src/utils.test.ts(1 hunks)cmd/ui/src/utils.ts(2 hunks)cmd/ui/src/views/Explore/ContextMenu/ContextMenu.test.tsx(11 hunks)cmd/ui/src/views/Explore/ContextMenu/ContextMenu.tsx(1 hunks)cmd/ui/src/views/Explore/ContextMenu/ContextMenuZoneManagementEnabled.test.tsx(7 hunks)cmd/ui/src/views/Explore/ContextMenu/ContextMenuZoneManagementEnabled.tsx(4 hunks)cmd/ui/src/views/Explore/ContextMenu/CopyMenuItem.tsx(2 hunks)cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.test.tsx(2 hunks)cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx(3 hunks)cmd/ui/src/views/Explore/GraphView.tsx(6 hunks)packages/javascript/bh-shared-ui/src/components/ContextMenuItems/EdgeMenuItems.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ContextMenuItems/NodeMenuItems.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/ContextMenuItems/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/edgeTypes.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/usePathfindingFilters.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/usePathfindingFilters.ts(2 hunks)packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/utils.ts(0 hunks)packages/javascript/bh-shared-ui/src/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/utils/array.test.ts(1 hunks)packages/javascript/bh-shared-ui/src/utils/array.ts(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/utils.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/javascript/bh-shared-ui/src/types.ts
- packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/usePathfindingFilters.test.tsx
- cmd/ui/src/components/SigmaChart/GraphEvents.tsx
- packages/javascript/bh-shared-ui/src/components/ContextMenuItems/index.ts
- packages/javascript/bh-shared-ui/src/utils/index.ts
- packages/javascript/bh-shared-ui/src/components/index.ts
- cmd/ui/src/views/Explore/ContextMenu/ContextMenuZoneManagementEnabled.test.tsx
- packages/javascript/bh-shared-ui/src/utils/array.ts
- cmd/ui/src/views/Explore/ContextMenu/CopyMenuItem.tsx
- cmd/ui/src/rendering/utils/utils.test.ts
- packages/javascript/bh-shared-ui/src/utils/array.test.ts
- cmd/ui/src/rendering/utils/utils.ts
- cmd/ui/src/utils.test.ts
- cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx
- cmd/ui/src/views/Explore/ContextMenu/ContextMenuZoneManagementEnabled.tsx
- cmd/ui/src/views/Explore/GraphView.tsx
- packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/EdgeFilter.tsx
- packages/javascript/bh-shared-ui/src/components/ContextMenuItems/EdgeMenuItems.tsx
- packages/javascript/bh-shared-ui/src/edgeTypes.tsx
- cmd/ui/src/components/SigmaChart/SigmaChart.tsx
- cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.test.tsx
- cmd/ui/src/utils.ts
- packages/javascript/bh-shared-ui/src/components/ContextMenuItems/NodeMenuItems.tsx
- cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx
- cmd/ui/src/views/Explore/ContextMenu/ContextMenu.tsx
- packages/javascript/bh-shared-ui/src/hooks/useExploreGraph/usePathfindingFilters.ts
⏰ 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 (6)
cmd/ui/src/views/Explore/ContextMenu/ContextMenu.test.tsx (6)
19-19: LGTM: Import addition supports new functionality.The PathfindingFilters import is correctly added to support the new edge filtering functionality being tested.
86-92: LGTM: Options object pattern improves flexibility.The refactoring to use an options object pattern makes the setup function more maintainable and flexible as new parameters can be added without breaking existing calls.
115-122: LGTM: Comprehensive pathfinding filters mock.The PathfindingFilters mock includes all necessary methods for testing edge filtering functionality. The mock is well-structured and supports the new edge context menu features.
126-126: LGTM: Coordinate property names updated correctly.The coordinate properties have been correctly updated from
mouseX/mouseYtox/yto align with the newCoordinatestype.
140-141: LGTM: Well-organized test structure.The addition of a describe block for node tests and the
beforeEachhook provides good separation and ensures consistent test state setup.
254-298: LGTM: Comprehensive edge test coverage.The edge tests provide excellent coverage of the new functionality:
- Tests filtering options appear only on pathfinding tab
- Tests behavior with invalid edge IDs
- Tests non-filterable edge scenarios
- Tests the actual filtering behavior
The test cases are well-structured and cover both positive and negative scenarios effectively.
| contextMenu={isNode(selectedItemQuery.data) ? contextMenu : null} | ||
| contextMenu={contextMenu} |
There was a problem hiding this comment.
We should have this edge context capability work for the ContextMenuZoneManagementEnabled component as well as it may look like a bug if the edge menu doesn't show when the tier_management_engine feature flag is enabled.
| @@ -44,7 +48,7 @@ export const GraphEdgeEvents: FC<GraphEdgeEventProps> = ({ onClickEdge }) => { | |||
| (event: any) => { | |||
| const context = edgeLabelsCanvas.getContext('2d'); | |||
| if (!context) return; | |||
| if (event.type === 'click' || event.type === 'mousemove') { | |||
| if (EDGE_EVENTS.includes(event.type)) { | |||
There was a problem hiding this comment.
I appreciate the EDGE_EVENTS declaration for this condition checking here. Reads nicer than two ||
| // Used to quickly determine if a given string is an edge type | ||
| const EDGE_TYPE_SET = new Set( | ||
| AllEdgeTypes.flatMap((category) => category.subcategories.flatMap((sub) => sub.edgeTypes)) | ||
| ); |
| pathfindingFilters: PathfindingFilters; | ||
| }; | ||
|
|
||
| const RX_EDGE_TYPE = /^[^_]+_(.+)_[^_]+$/; |
There was a problem hiding this comment.
This works for the use case today as no pathfinding edges contain underscores in the name but I don't think there are gates prohibiting the use of them for new edges. There are actually some edges in our schema that do have underscores in the name, AZMGAppRoleAssignment_ReadWrite_All for example. These edges happen to not be pathfinding edges though so do not conflict with the expected behavior.
I think it would be good to add a test that pulls the list of pathfinding edges and looks for the presence of underscores and asserts that there are none. This way if one does get added in the future the failing test would be a good reminder to update the logic to account for that case.
Alternatively we could update the useGraphItem hook to additionally return the edge name without the source and target node appended at the ends so that we don't have to parse it out again here.
Description
When in Explore -> Pathfinding tab, right-clicking on an edge will show a new context menu allowing to easily update pathfinding filters.
Motivation and Context
Resolves BED-5180
This will allow users to remove complexity from the graph while being able to identify and simulate changes in the graph.
How Has This Been Tested?
Manual testing and new/updated unit tests
Screenshots:
Types of changes
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Refactor
Tests