BED-6709 chore: default tags glyph migration #2022
Conversation
urangel
changed the title
WalkthroughThis PR removes glyph-specific types/exports and consolidates glyph handling to TagGlyphs across shared-ui and UI code, renames Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as GraphView
participant Hook as useTagGlyphs
participant Utils as createGlyphMapFromTags
participant Graph as GraphRenderer
UI->>Hook: useTagGlyphs(glyphUtils, darkMode)
Hook->>Utils: createGlyphMapFromTags(tags)
Utils-->>Hook: TagGlyphs (ownedGlyph, tag mappings...)
Hook-->>UI: tagGlyphs
UI->>Graph: getGlyphFromKinds(node.kinds, tagGlyphs)
Graph-->>UI: glyphImage or null
Note right of Graph: place glyph at BOTTOM_RIGHT if owned else TOP_RIGHT
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
specter-flq
left a comment
specter-flq
left a comment
There was a problem hiding this comment.
Had some questions on functionality when node is T0 and Owned. Changes related to that coming so I will continue review after that
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.tsx (1)
50-59: LGTM! Clean refactor to global screen usage.The setup function correctly removes the local
screenfrom its return value in favor of the globalscreenfrom test-utils. This standardizes test patterns across the codebase.Optionally remove the
returnkeyword on line 51 since the render result is no longer used:await act(async () => { - return render( + render( <AssetGroupFilters filterParams={options?.filterParams ?? {}} handleFilterChange={handleFilterChange} memberCounts={memberCounts} /> ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/api/src/database/migration/migrations/v8.4.0.sql(2 hunks)cmd/ui/src/utils.ts(1 hunks)cmd/ui/src/views/Explore/GraphView.tsx(2 hunks)cmd/ui/src/views/Explore/svgIcons.ts(1 hunks)cmd/ui/src/views/Explore/utils.test.ts(1 hunks)cmd/ui/src/views/Explore/utils.ts(4 hunks)packages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsx(4 hunks)packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.tsx(11 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(3 hunks)packages/javascript/bh-shared-ui/src/utils/icons.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/utils/icons.ts
✅ Files skipped from review due to trivial changes (1)
- cmd/ui/src/views/Explore/GraphView.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/ui/src/utils.ts
- cmd/ui/src/views/Explore/utils.ts
- cmd/api/src/database/migration/migrations/v8.4.0.sql
🧰 Additional context used
🧠 Learnings (8)
📚 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:
cmd/ui/src/views/Explore/utils.test.tspackages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsxpackages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.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:
cmd/ui/src/views/Explore/utils.test.ts
📚 Learning: 2025-11-06T21:23:30.689Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 2010
File: packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx:0-0
Timestamp: 2025-11-06T21:23:30.689Z
Learning: In ExploreSearchCombobox component (packages/javascript/bh-shared-ui/src/components/ExploreSearchCombobox/ExploreSearchCombobox.tsx), setting aria-autocomplete={null} on the TextField is intentional to override downshift's default aria-autocomplete attribute, which was flagged by Axe DevTools. This approach satisfies Axe accessibility requirements by removing the conflicting attribute.
Applied to files:
packages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsx
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsxpackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.tsxpackages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.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/AssetGroupFilters/AssetGroupFilters.test.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
cmd/ui/src/views/Explore/svgIcons.ts
🧬 Code graph analysis (3)
cmd/ui/src/views/Explore/utils.test.ts (1)
cmd/ui/src/views/Explore/utils.ts (1)
initGraph(60-91)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTagTypeOwned(118-118)cmd/api/src/model/assetgrouptags.go (1)
AssetGroupTagTypeOwned(51-51)
cmd/ui/src/views/Explore/svgIcons.ts (1)
packages/javascript/bh-shared-ui/src/utils/icons.ts (4)
NODE_ICONS(72-230)UNKNOWN_ICON(232-235)getModifiedSvgUrlFromIcon(257-271)NODE_SCALE(63-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (12)
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx (1)
33-33: LGTM! Clean refactor to use a specialized component.The import path change correctly switches to a co-located, specialized
AssetGroupMenuItemPrivilegeZonesEnabledcomponent. This aligns well with the PR's objective of simplifying glyph handling and improving code organization.cmd/ui/src/views/Explore/svgIcons.ts (2)
44-44: LGTM! Comment accurately describes the remaining icon URL generation.The updated comment clearly reflects that only icon URLs (nodes and utility icons like UNKNOWN_ICON) are being appended after the removal of glyph-related code.
48-48: LGTM! Clean export statement focused on icon utilities.The exports are now streamlined to include only icon-related utilities, successfully removing glyph-specific exports as intended by the PR objectives.
packages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.test.tsx (1)
18-67: LGTM! Test refactoring aligns with codebase patterns.The refactoring successfully migrates to the global
screenpattern while maintaining correct test structure. The import from custom test-utils follows established conventions, and the setup function appropriately returns only what's needed by each test.Based on learnings
packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.tsx (2)
23-23: LGTM! Correct test-utils usage.Importing
screenandwaitForfrom custom test-utils follows the established pattern in the BloodHound codebase.Based on learnings
63-182: LGTM! Consistent test updates across all test cases.All test functions have been correctly updated to use the global
screenfrom test-utils instead of destructuring it from thesetup()return value. The refactor is thorough and consistent across all 12 test cases.packages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsx (1)
78-78: Good refactor to Testing Library best practices.The shift from checking
input.valuedirectly to usingtoHaveAttribute('value', 'test')aligns with Testing Library's philosophy of testing the DOM as users experience it. This approach verifies the attribute rather than the JavaScript property, which is more robust and consistent with how the browser presents the element.Also applies to: 91-91, 109-109
cmd/ui/src/views/Explore/utils.test.ts (1)
87-93: LGTM! Property rename aligns with the refactor.The update from
tagGlyphMaptotagGlyphsis consistent with the broader glyph system refactor. The tests correctly verify graph initialization and edge deduplication behavior with an empty glyph map.Also applies to: 101-107
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (4)
133-133: LGTM! Extracting the tag prefix constant improves maintainability.The
TagLabelPrefixconstant eliminates magic strings and ensures consistent tag labeling across the codebase.
146-158: Verify the owned tag empty glyph handling is intentional.The logic adds owned tags to the glyph map even when
glyphValueis empty (lines 151-155), but excludes regular tags with empty glyph values (line 157). This meansownedGlyphcould be set to an empty string, while regular tags with empty glyphs are omitted entirely.Is this intentional? If the owned tag needs to be tracked even without a valid glyph, consider documenting this behavior. Otherwise, apply the same empty-check for owned tags:
if (tag.type === AssetGroupTagTypeOwned) { + if (glyphValue === '') return; glyphMap.owned = `${TagLabelPrefix}${underscoredTagName}`; glyphMap.ownedGlyph = glyphValue; return; }
163-172: LGTM! Using the constant ensures consistency.The update to use
TagLabelPrefixeliminates the hardcoded string and ensures consistent tag label matching.
174-176: LGTM! Explicit type exports improve API clarity.Adding the
TagGlyphstype export and explicit return type annotation enhances type safety and makes the hook's contract clearer to consumers.
| it('should display a total count of asset group members', async () => { | ||
| const { screen } = await setup(); | ||
| const count = screen.getByText('Total Count').nextSibling.textContent; | ||
| const count = screen.getByText('Total Count').nextSibling?.textContent; |
There was a problem hiding this comment.
Optional chaining may reduce test failure clarity.
The optional chaining on nextSibling?.textContent prevents an immediate null reference error if the DOM structure is unexpected. While safer, this means count will be undefined rather than throwing at the access point, making test failures less clear about whether the issue is a missing element or an incorrect value.
Consider one of these approaches for clearer test failures:
-const count = screen.getByText('Total Count').nextSibling?.textContent;
+const count = screen.getByText('Total Count').nextSibling.textContent;Or explicitly assert the structure:
-const count = screen.getByText('Total Count').nextSibling?.textContent;
+const totalCountElement = screen.getByText('Total Count');
+expect(totalCountElement.nextSibling).toBeTruthy();
+const count = totalCountElement.nextSibling.textContent;📝 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 count = screen.getByText('Total Count').nextSibling?.textContent; | |
| const count = screen.getByText('Total Count').nextSibling.textContent; |
| const count = screen.getByText('Total Count').nextSibling?.textContent; | |
| const totalCountElement = screen.getByText('Total Count'); | |
| expect(totalCountElement.nextSibling).toBeTruthy(); | |
| const count = totalCountElement.nextSibling.textContent; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsx
around line 69, the test uses optional chaining on nextSibling?.textContent
which hides a missing/changed DOM structure by returning undefined instead of
failing fast; remove the optional chaining and explicitly assert the DOM
relationship before reading textContent (for example, get the "Total Count"
node, assert its nextSibling is not null/undefined with
expect(...).not.toBeNull(), then read nextSibling.textContent), or query the
exact element that contains the count so you can directly assert its value.
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Tested locally, thanks for bringing these all in line 🙇
🚀
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.4.0.sql (1)
46-51: Minor redundancy in conditional CASE logic.Lines 48-50 use CASE statements to conditionally update columns only when 'SYSTEM' is present. However, the WHERE clause (line 51) already filters the entire UPDATE to rows containing 'SYSTEM'. The CASE statements are harmless but could be simplified for clarity:
UPDATE asset_group_tags -SET created_by = CASE WHEN created_by = 'SYSTEM' THEN 'BloodHound' ELSE created_by END, - updated_by = CASE WHEN updated_by = 'SYSTEM' THEN 'BloodHound' ELSE updated_by END, - deleted_by = CASE WHEN deleted_by = 'SYSTEM' THEN 'BloodHound' ELSE deleted_by END -WHERE created_by = 'SYSTEM' OR updated_by = 'SYSTEM' OR deleted_by = 'SYSTEM'; +SET created_by = 'BloodHound' +WHERE created_by = 'SYSTEM';Alternatively, keep the current approach if intentionally preserving non-SYSTEM values during a partial migration (e.g., if some rows have mixed SYSTEM and non-SYSTEM authors). The current logic is correct but less obvious.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/api/src/database/migration/migrations/v8.4.0.sql(3 hunks)cmd/ui/src/utils.ts(1 hunks)cmd/ui/src/views/Explore/GraphView.tsx(2 hunks)cmd/ui/src/views/Explore/svgIcons.ts(1 hunks)cmd/ui/src/views/Explore/utils.test.ts(1 hunks)cmd/ui/src/views/Explore/utils.ts(4 hunks)packages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsx(4 hunks)packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.tsx(11 hunks)packages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(3 hunks)packages/javascript/bh-shared-ui/src/utils/icons.ts(0 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/utils/icons.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/javascript/bh-shared-ui/src/components/AssetGroupEdit/AssetGroupEdit.test.tsx
- packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx
- cmd/ui/src/views/Explore/GraphView.tsx
- cmd/ui/src/views/Explore/utils.ts
- packages/javascript/bh-shared-ui/src/components/HelpTexts/CodeController/CodeController.test.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
cmd/ui/src/views/Explore/svgIcons.tscmd/ui/src/utils.ts
📚 Learning: 2025-09-08T19:24:33.396Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/components/PrebuiltSearchList/PrebuiltSearchList.test.tsx:17-20
Timestamp: 2025-09-08T19:24:33.396Z
Learning: In BloodHound codebase, prefer importing `render` and `screen` from custom test-utils (e.g., `import { render, screen } from '../../test-utils';`) instead of directly from `testing-library/react`.
Applied to files:
packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.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/AssetGroupFilters/AssetGroupFilters.test.tsxcmd/ui/src/views/Explore/utils.test.tspackages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.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/AssetGroupFilters/AssetGroupFilters.test.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:
cmd/ui/src/views/Explore/utils.test.ts
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.4.0.sql
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.4.0.sql
📚 Learning: 2025-09-02T16:46:30.895Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
Applied to files:
cmd/api/src/database/migration/migrations/v8.4.0.sql
🧬 Code graph analysis (3)
cmd/ui/src/views/Explore/svgIcons.ts (1)
packages/javascript/bh-shared-ui/src/utils/icons.ts (4)
NODE_ICONS(72-230)UNKNOWN_ICON(232-235)getModifiedSvgUrlFromIcon(257-271)NODE_SCALE(63-63)
cmd/ui/src/views/Explore/utils.test.ts (1)
cmd/ui/src/views/Explore/utils.ts (1)
initGraph(60-91)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
cmd/api/src/model/assetgrouptags.go (1)
AssetGroupTagTypeOwned(51-51)packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTagTypeOwned(118-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (12)
packages/javascript/bh-shared-ui/src/components/AssetGroupFilters/AssetGroupFilters.test.tsx (2)
23-23: LGTM! Test utilities refactoring aligns with codebase patterns.The changes properly migrate to using global
screenfrom test-utils and remove it from the setup return value. Theactwrapping around render ensures any immediate async operations (like the mocked/api/v2/custom-nodescall) settle before tests proceed, which is appropriate for this component.Based on learnings
Also applies to: 50-59
63-63: LGTM! All test cases updated consistently.All 12 test cases have been correctly updated to use the global
screenand only destructure the values they need from the setup function. The changes are uniform and maintain all existing test coverage.Also applies to: 78-78, 86-86, 95-95, 104-104, 119-119, 134-134, 141-141, 151-151, 163-163, 170-170, 182-182
cmd/api/src/database/migration/migrations/v8.4.0.sql (2)
17-44: Migration setup and permissions follow established patterns.The permission and role insertion, along with the role-permissions join logic, correctly follow the codebase pattern of using
current_timestampfor timestamp fields. The addition of audit_log permission and Auditor role is clear and properly integrated into the existing permission structure.
68-72: Original review comment contains overstated concerns; glyph assignments are correct.The migration targets two distinct default asset_group_tags: one identified by position=1 (Tier Zero → 'gem') and another by type=3 (Owned → 'skull'). Based on the schema constraint (UNIQUE on glyph, added in v8.1.0.sql) and usage patterns throughout v8.3.0.sql, these are separate records with no overlap. The two sequential UPDATE statements are intentional and safe—they will not conflict or violate constraints.
Non-default tags retaining NULL glyphs is by design, as the comments indicate this migration only assigns glyphs to the two default asset_group_tags.
cmd/ui/src/views/Explore/utils.test.ts (1)
87-93: LGTM! Property rename aligns with interface changes.The tests correctly update the property name from
tagGlyphMaptotagGlyphsto match the updatedGraphOptionsinterface. The empty object initialization is appropriate for these test cases.Also applies to: 101-107
cmd/ui/src/utils.ts (1)
17-17: Removal of ThemedGlyph fields verified — no remaining references found.The verification confirms that
tierZeroGlyphandownedObjectGlyphhave been completely removed from the codebase with no orphaned references remaining. The removal ofGlyphIconInfofrom imports is clean and complete.cmd/ui/src/views/Explore/svgIcons.ts (1)
44-48: No remaining imports of removed glyph exports from svgIcons.ts detected.Verification confirms all five removed exports (GLYPHS, GlyphDictionary, GLYPH_SCALE, DEFAULT_GLYPH_COLOR, appendGlyphSvgUrls) have no lingering imports from svgIcons.ts. The glyph-related exports that remain in packages/javascript/bh-shared-ui/src/utils/icons.ts are unaffected and continue to be imported from their original location. The consolidation is complete with no breaking changes.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (5)
133-133: LGTM! Good extraction of magic string.The
TagLabelPrefixconstant improves maintainability by replacing inline'Tag_'strings throughout the code. Theas constassertion ensures proper type narrowing.
163-172: LGTM! Consistent use of TagLabelPrefix.The refactoring to use
TagLabelPrefixinstead of the hardcoded'Tag_'string improves consistency and maintainability.
174-174: LGTM! Type export improves reusability.Exporting the
TagGlyphstype allows consistent typing across modules that consume glyph maps.
176-176: LGTM! Explicit return type improves type safety.Adding the explicit
TagGlyphsreturn type annotation makes the hook's contract clearer and provides better IDE support.
151-155: Verify whether the API enforces a single owned tag per asset group.Based on code analysis, the system design assumes exactly one owned tag exists per asset group:
useOwnedTagId()(line 501) uses.find()rather than filtering to get a single tag, and the backend prevents deletion and renaming of the owned tag type. However, if the API allows creating multiple owned tags, the forEach loop's last-wins behavior could silently overwrite earlier owned tag data.Confirm either:
- The asset group tag creation API validates that only one owned tag can exist per asset group, or
- Document the intentional last-wins behavior if multiple owned tags are possible
| if (tag.glyph === null) return; | ||
| if (!qualifier(tag.glyph)) return; | ||
|
|
||
| const glyphValue = transformer(tag.glyph, darkMode); | ||
|
|
||
| if (glyphValue !== '') glyphMap[`Tag_${underscoredTagName}`] = glyphValue; | ||
| if (tag.type === AssetGroupTagTypeOwned) { | ||
| glyphMap.owned = `${TagLabelPrefix}${underscoredTagName}`; | ||
| glyphMap.ownedGlyph = glyphValue; | ||
| return; | ||
| } | ||
|
|
||
| if (glyphValue !== '') glyphMap[`${TagLabelPrefix}${underscoredTagName}`] = glyphValue; | ||
| }); |
There was a problem hiding this comment.
Inconsistent empty string validation for owned tags.
Non-owned tags validate that glyphValue !== '' before adding to the map (line 157), but owned tags (lines 151-154) set ownedGlyph without checking if the transformed value is empty. If an owned tag has an invalid glyph that transforms to an empty string, the map will contain ownedGlyph: '', which may cause rendering issues downstream.
Apply this diff to add consistent validation:
if (tag.type === AssetGroupTagTypeOwned) {
+ const glyphValue = transformer(tag.glyph, darkMode);
+ if (glyphValue === '') return;
glyphMap.owned = `${TagLabelPrefix}${underscoredTagName}`;
- glyphMap.ownedGlyph = glyphValue;
+ glyphMap.ownedGlyph = glyphValue;
return;
}
-
-if (glyphValue !== '') glyphMap[`${TagLabelPrefix}${underscoredTagName}`] = glyphValue;
+
+const glyphValue = transformer(tag.glyph, darkMode);
+if (glyphValue !== '') glyphMap[`${TagLabelPrefix}${underscoredTagName}`] = glyphValue;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
around lines 146 to 158, the owned-tag branch sets glyphMap.ownedGlyph without
checking for an empty transformed value while non-owned tags require glyphValue
!== ''; update the owned-tag handling to only set glyphMap.owned and
glyphMap.ownedGlyph when glyphValue is non-empty (i.e., mirror the same
glyphValue !== '' validation used for non-owned tags) so ownedGlyph is never
assigned an empty string.
Description
Explicit glyph values are set for default tags and some implicit glyph rendering logic is cleaned up.
Motivation and Context
Resolves BED-6709
How Has This Been Tested?
Tests have been updated and continue to pass. Glyphs verified to render as expected with sigmajs.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Refactor
Bug Fixes
Chores