Skip to content

Conversation

@seanjSO
Copy link
Contributor

@seanjSO seanjSO commented Dec 22, 2025

Description

Adds a filter stage to the SearchCurrentNodes component to filter out edges from the list of nodes.

Motivation and Context

Resolves BED-6940

Why is this change required? What problem does it solve?

How Has This Been Tested?

  • Added a graph edge to the test cases included with the component
  • Manually tested search scenarios to ensure edges are not included in search list

Screenshots (optional):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

Summary by CodeRabbit

  • New Features

    • Added support for graph edge relationships in the search component so it can handle mixed node-and-edge records.
  • Bug Fixes

    • Improved filtering so edge entries are excluded from node results, preventing edges from appearing as searchable nodes.

✏️ Tip: You can customize this high-level summary in your review settings.

@seanjSO seanjSO self-assigned this Dec 22, 2025
@seanjSO seanjSO added bug Something isn't working user interface A pull request containing changes affecting the UI code. labels Dec 22, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Walkthrough

Replaces the previous GraphNodes-only representation with a combined GraphRecords (nodes or edges) type, updates the SearchCurrentNodes component to accept GraphRecords and filter out edge-like entries when flattening nodes, and extends test fixtures to include an example edge record.

Changes

Cohort / File(s) Summary
Type definitions
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts
Removed GraphNodes; added GraphEdge (fields: id1, id2, optional end1/end2, color?, data?, label) and `GraphRecords = Record<string, GraphNode
Component filtering logic
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx
Changed prop typing from GraphNodes to GraphRecords; updated transformation to exclude records where both id1 and id2 exist (treat as edges) before mapping to FlatNode.
Test fixtures
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx
Updated imports/variable types from GraphNodesGraphRecords; added an edge-like test record (e.g., rel_2_MemberOf_3 with id1, id2, end2 arrow).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • maffkipp
  • mistahj67

Poem

🐰 I hopped through types and tests tonight,
Edges added, nodes kept in sight.
I filtered links with gentle cheer,
So search shows only nodes right here.
🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: filtering edges from graph search with a specific ticket reference.
Description check ✅ Passed The description provides a clear problem statement, testing details, and references the associated ticket BED-6940, though motivation details are minimal.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between de01743 and ef69254.

📒 Files selected for processing (3)
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.

Applied to files:

  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.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/SearchCurrentNodes/SearchCurrentNodes.tsx
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx
📚 Learning: 2025-08-27T15:10:29.731Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: cmd/ui/src/views/Explore/ExploreSearch/ExploreSearch.tsx:206-210
Timestamp: 2025-08-27T15:10:29.731Z
Learning: In the BloodHound codebase, the team prefers to keep JSX elements without keys in the TabPanels component's tabs array, as indicated by the existing ESLint disable comment and explicit decision to not add keys despite Biome linting warnings.

Applied to files:

  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.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/SearchCurrentNodes/SearchCurrentNodes.test.tsx
📚 Learning: 2025-07-17T16:26:22.455Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/components/SigmaChart/GraphEdgeEvents.tsx:145-152
Timestamp: 2025-07-17T16:26:22.455Z
Learning: In the BloodHound codebase GraphEdgeEvents.tsx, context menu positioning should use event.nativeEvent.offsetX/offsetY (canvas-relative coordinates) rather than event.clientX/clientY (page-relative coordinates). The horizontal offset adjustments are handled later in the processing chain.

Applied to files:

  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (2)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts (2)
  • GraphRecords (46-46)
  • FlatNode (17-17)
packages/javascript/js-client-library/src/types.ts (1)
  • FlatGraphResponse (445-445)
🔇 Additional comments (4)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx (2)

22-24: LGTM! Type updates correctly reflect the new GraphRecords structure.

The import and type annotation have been properly updated from GraphNodes to GraphRecords, which now represents a record that can contain both node and edge entries. This aligns with the component's new edge-filtering capability.


49-59: LGTM! Edge fixture is complete and correctly structured.

The edge test fixture includes all required fields (label.text, id1, id2, end2) and properly validates that the component filters out edge entries while still matching only the 3 node entries in search results.

packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (2)

25-34: LGTM! Type updates correctly support both nodes and edges.

The prop type has been updated to GraphRecords | FlatGraphResponse, which correctly handles the new structure where records can contain both GraphNode and GraphEdge (or their styled variants). This change enables the component to receive edge data and filter it out appropriately.


44-59: LGTM! Edge filtering and transformation logic is correct.

The implementation correctly:

  1. Filters out edges by checking for the presence of both id1 and id2 properties, which distinguishes edges (GraphEdge/StyledGraphEdge) from nodes (GraphNode/StyledGraphNode).
  2. Resolves the ESLint error by omitting the unused parameter with [, value] syntax instead of naming it _.
  3. Maintains existing mapping logic for both GraphNode entries (with objectId) and StyledGraphNode entries (with nested data structure).

The filter correctly handles both GraphRecords and FlatGraphResponse input types, as both can contain edge entries that need to be excluded from the node search results.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e2e3a8 and de01743.

📒 Files selected for processing (3)
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.

Applied to files:

  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx
  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx
📚 Learning: 2025-08-27T19:22:50.905Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx:71-74
Timestamp: 2025-08-27T19:22:50.905Z
Learning: In BloodHound's CommonSearches component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/CommonSearches.tsx), the useEffect that resets filteredList to queryList when userQueries.data changes is intentional behavior - filters should be cleared when the underlying saved queries data updates.

Applied to files:

  • packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.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/SearchCurrentNodes/SearchCurrentNodes.test.tsx
🧬 Code graph analysis (3)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (1)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts (1)
  • FlatNode (17-17)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts (1)
packages/javascript/js-client-library/src/types.ts (4)
  • GraphEdge (396-405)
  • GraphNodes (394-394)
  • GraphNode (381-390)
  • GraphEdges (407-407)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.test.tsx (2)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts (2)
  • GraphNodes (46-46)
  • GraphEdges (47-47)
packages/javascript/js-client-library/src/types.ts (2)
  • GraphNodes (394-394)
  • GraphEdges (407-407)
🪛 GitHub Actions: Build UI
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx

[error] 45-45: ESLint: '_' is defined but never used. (no-unused-vars)

⏰ 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). (3)
  • GitHub Check: Build BloodHound Container Image / Build and Package Container
  • GitHub Check: run-tests
  • GitHub Check: run-analysis
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/types.ts (2)

30-44: GraphEdge type definition looks correct for UI graph representation.

The structure with id1, id2, and arrow configurations is appropriate for UI graph rendering. Note that this differs from the GraphEdge in js-client-library/src/types.ts which uses source/target fields, but both serve different purposes (UI rendering vs API/domain model).


46-47: Remove the unused GraphEdges type definition or clarify its purpose.

The GraphEdges type exported from SearchCurrentNodes/types.ts (line 47) is never imported or used anywhere in the codebase. Either remove it as dead code or add documentation if it's intended for future use. Note: The GraphNodes pattern (Record-based) is used locally and differs from js-client-library's array-based design, but since GraphEdges isn't used, this is not a data flow issue.

Likely an incorrect or invalid review comment.

packages/javascript/bh-shared-ui/src/components/SearchCurrentNodes/SearchCurrentNodes.tsx (1)

49-59: Mapping logic correctly transforms node data to FlatNode.

The three-case transformation handles:

  1. Direct GraphNode objects with objectId
  2. Graph data with nested data and label structures
  3. Fallback for malformed entries

This approach properly accommodates the different node shapes that may appear in currentNodes.

@seanjSO seanjSO marked this pull request as draft December 22, 2025 18:34
@seanjSO seanjSO force-pushed the seanj/BED-6940 branch 2 times, most recently from 6de2843 to 5d201bd Compare December 22, 2025 22:34
@seanjSO seanjSO force-pushed the seanj/BED-6940 branch 2 times, most recently from 10b075f to a3b88db Compare January 6, 2026 18:54
@seanjSO seanjSO marked this pull request as ready for review January 6, 2026 20:01
Copy link
Contributor

@TheNando TheNando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@seanjSO seanjSO merged commit 52376d8 into main Jan 6, 2026
13 checks passed
@seanjSO seanjSO deleted the seanj/BED-6940 branch January 6, 2026 21:17
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working user interface A pull request containing changes affecting the UI code.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants