chore: env severity updates BED-6602 #2142
Conversation
WalkthroughRemoves highest-privilege fallback from tag selection, adds hygiene tag constants and a top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(1 hunks)packages/javascript/js-client-library/src/responses.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
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.
📚 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/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.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/views/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 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/hooks/useSelectedTag.ts
🧬 Code graph analysis (1)
packages/javascript/js-client-library/src/responses.ts (1)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(138-149)
⏰ 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 (2)
packages/javascript/js-client-library/src/responses.ts (1)
54-57: Environment exposure typings look consistentThe new
EnvironmentExposuretype and thehygiene_attack_paths/exposuresadditions toEnvironmentare coherent with existing response shapes and naming conventions; no issues from a typing/contract perspective based on this file alone.Also applies to: 65-66
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx (1)
22-23: Default export wiring forDetailslooks goodRe-exporting
Detailsas the default while keeping the existing named exports is clean and preserves current API surface; no issues here.
| export const useSelectedTag = (fromPathParams = false) => { | ||
| const pathTag = useSelectedTagPathParams(); | ||
| const queryTag = useSelectedTagQueryParams(); | ||
| const pathTag = useSelectedTagPathParams; | ||
| const queryTag = useSelectedTagQueryParams; | ||
|
|
||
| return fromPathParams ? pathTag : queryTag; |
There was a problem hiding this comment.
Curious as to why not something like:
return fromPathParams ? useSelectedTagPathParams() : useSelectedTagQueryParams();
That way we don't have to change all the other files to have the hook be a Self Invoking function and be in tune with what code rabbit is suggestion which is that it returns the values and not just another function, which is what is maybe expected.
There was a problem hiding this comment.
plus was getting a TS error with the Self Invoking functions
There was a problem hiding this comment.
The motivation was to avoid running both hooks. With the updates we return the callback that is needed and only run that one.
There was a problem hiding this comment.
Yup i get that part and with running the below it only runs one hook. I console logged to confirm. Works exactly the same as the callback but with less changes and without the need of self invoking functions off a hook.
return fromPathParams ? useSelectedTagPathParams() : useSelectedTagQueryParams();
There was a problem hiding this comment.
This suggestion breaks the rules of hooks
React Hook "useSelectedTagQueryParams" is called conditionally. React Hooks must be called in the exact same order in every component render. (eslint react-hooks/rules-of-hooks)
There was a problem hiding this comment.
I think the current change also technically violate the rule of hooks, since React can't guarantee anymore which hook will run on each render; I think in this case its just obfuscating that from the linter.
The way I have done it in the past has been the original pattern, where you just invoke both and then return the implementation you want to use. If you're trying to correct for some performance issues from running both, maybe two possible solutions would be:
- Combine these two hooks into a single hook that only conditionally computes the expensive operation
- In the components where this hook is called conditionally, instead conditionally render one of two wrapper components that each call one of these hooks and pass along the data to their children
There was a problem hiding this comment.
noooo Wes get outta here with your reason. It was cumbersome mocking both for tests -__-
There was a problem hiding this comment.
@urangel true true sorry about that the linter played a dirty one on me. Should have picked up the conditional restriction on hooks.
Good suggestions @maffkipp, I agree and feel that the solution should come from/or in the logic of the hook itself and the invocation of the hook should stay as commonly used and maybe stay away from the conditionals and self invoking as we dont really see a lot of that.
There was a problem hiding this comment.
I've put this back to how it was originally so that we are calling both hooks
ae2c6d6 to
7efeb5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (1)
16-37: Consider type‑only import (and confirmAssetGroupTagis the right domain type here)
AssetGroupTagis only used as a type in this hook. You can avoid a runtime dependency onjs-client-libraryby switching to a type‑only import, and it’s worth double‑checking that this hook’s tags are truly the APIAssetGroupTagshape (prior discussions discouraged reusing that type for unrelated tag shapes in some UI code).Example:
-import { AssetGroupTag } from 'js-client-library'; +import type { AssetGroupTag } from 'js-client-library';If these tags diverge from the API model, consider defining a local
SelectedTagUI type instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts(2 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx(1 hunks)packages/javascript/js-client-library/src/responses.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/PrivilegeZones/Details/index.tsxpackages/javascript/bh-shared-ui/src/hooks/useSelectedTag.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/useSelectedTag.ts
📚 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/hooks/useSelectedTag.ts
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-18T22:09:32.551Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1793
File: packages/javascript/bh-shared-ui/src/hooks/useFinishedJobs/useFinishedJobs.test.ts:45-53
Timestamp: 2025-08-18T22:09:32.551Z
Learning: When suggesting changes to hook dependencies (like adding permissionsLoaded/isSuccess checks), always mention that existing tests mocking those hooks will likely need updates to include the new dependencies, as omitting them can cause unexpected test behavior or failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-27T19:58:12.996Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx:89-97
Timestamp: 2025-08-27T19:58:12.996Z
Learning: In BloodHound's SaveQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx), the useEffect that sets isNew based on selectedQuery existence (rather than id presence) is intentional behavior - the logic for determining whether a query is new vs existing should not be changed to check for id presence.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
🧬 Code graph analysis (2)
packages/javascript/js-client-library/src/responses.ts (1)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(138-149)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useAssetGroupTags(442-452)
⏰ 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 (4)
packages/javascript/js-client-library/src/responses.ts (1)
54-57: EnvironmentExposure and new Environment fields look consistent; confirm API contractThe new
EnvironmentExposureshape and the addedhygiene_attack_paths/exposuresfields onEnvironmentfit the existing response-style patterns and AssetGroupTag usage. Please just confirm that the backing API always returns these fields (or that callers defensively handleundefinedat runtime) so we don’t introduce silent assumptions in consumers.Also applies to: 65-66
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx (1)
17-23: Default export for Details is straightforward and non‑breakingKeeping the named exports while adding
export default Detailsis safe and matches common index patterns; no issues from this change.packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (2)
35-49: Query/path param selection logic is safe and handles hygiene/placeholder casesUsing
data: tags = []fromuseAssetGroupTagsavoids null/undefined issues, the hygiene sentinel (HYGIENE_AGT_ID) is handled explicitly in the query‑param path, and both query and path variants fall back cleanly toplaceholderTagwhen no match is found. This is a reasonable and robust defaulting strategy.
51-55: useSelectedTag correctly composes hooks and returns tag valuesThe wrapper now unconditionally calls both
useSelectedTagPathParamsanduseSelectedTagQueryParamsand returns the appropriate tag value, which keeps consumers’ usage simple and adheres to React’s Rules of Hooks (the hook order is stable on every render). Any extra cost should be minimal since the underlying tag query is shared.
7efeb5c to
7a4c079
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (1)
48-48: Remove unnecessary optional chaining for consistency.Line 48 uses
tags?.findwith optional chaining, but line 41 usestags.findwithout it. Since both default to[]on lines 46 and 37, the optional chaining is redundant and creates inconsistency.🔎 Proposed fix
- return tags?.find((zone: AssetGroupTag) => zone.id.toString() === tagId) || placeholderTag; + return tags.find((tag: AssetGroupTag) => tag.id.toString() === tagId) || placeholderTag;Also renamed the callback parameter from
zonetotagfor clarity, since the type isAssetGroupTag.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsxpackages/javascript/js-client-library/src/responses.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/js-client-library/src/responses.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/useSelectedTag.ts
📚 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/hooks/useSelectedTag.ts
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-18T22:09:32.551Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1793
File: packages/javascript/bh-shared-ui/src/hooks/useFinishedJobs/useFinishedJobs.test.ts:45-53
Timestamp: 2025-08-18T22:09:32.551Z
Learning: When suggesting changes to hook dependencies (like adding permissionsLoaded/isSuccess checks), always mention that existing tests mocking those hooks will likely need updates to include the new dependencies, as omitting them can cause unexpected test behavior or failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-27T19:58:12.996Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx:89-97
Timestamp: 2025-08-27T19:58:12.996Z
Learning: In BloodHound's SaveQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx), the useEffect that sets isNew based on selectedQuery existence (rather than id presence) is intentional behavior - the logic for determining whether a query is new vs existing should not be changed to check for id presence.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (1)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useAssetGroupTags(462-472)
⏰ 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 (4)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (4)
16-18: LGTM: Import updates align with the refactored fallback behavior.The removal of
useHighestPrivilegeTagimport correctly reflects the change from highest-privilege fallback to placeholder fallback.
35-42: LGTM: Simplified fallback logic removes highest-privilege assumption.The change from
highestPrivilegeTagtoplaceholderTagfallback is a clear behavioral shift. The default empty array on line 37 ensurestags.find()is safe without optional chaining.
51-56: LGTM: Hook follows React's Rules of Hooks correctly.Both hooks are invoked unconditionally at the top level, which is the proper pattern to comply with React's Rules of Hooks. This design was confirmed in the past review discussion as preferable to conditional hook calls.
20-27: Fix return type to reflect actual shape differences between tag objects.The
useSelectedTagQueryParamsreturn type declaresAssetGroupTagbut returnsHygieneTagandplaceholderTag, which have incomplete shapes.HygieneTagis missing required fields fromAssetGroupTag(kind_id, type, position, require_certify, description, analysis_enabled, counts). Create a shared tag interface or union type that accurately represents all three possible returns instead of usingAssetGroupTagfor objects that don't implement its full contract.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
⏰ 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
| // Exit early in case number in response is larger than the max safe integer to avoid doing math on it and getting erronious numbers | ||
| if (!Number.isSafeInteger(num)) return '>9Q'; |
There was a problem hiding this comment.
Critical: Guard incorrectly rejects decimal numbers.
Number.isSafeInteger(num) returns false for non-integer values, causing the function to return '>9Q' for valid decimal inputs like 1500.5 instead of '1.5K'. This breaks the function's intended behavior.
Additionally, the comment contains a typo: "erronious" should be "erroneous".
🔎 Proposed fix
- // Exit early in case number in response is larger than the max safe integer to avoid doing math on it and getting erronious numbers
- if (!Number.isSafeInteger(num)) return '>9Q';
+ // Exit early for invalid numbers or numbers too large to abbreviate accurately
+ if (!isFinite(num)) return 'N/A';
+ if (Math.abs(num) > Number.MAX_SAFE_INTEGER) return '>9Q';This approach:
- Handles
NaNandInfinityexplicitly with'N/A' - Allows decimal numbers to pass through correctly
- Only guards against integers that exceed safe representation
🤖 Prompt for AI Agents
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts around lines
18-19: the current guard uses Number.isSafeInteger(num) which wrongly rejects
valid decimals; change the checks to: first handle NaN or non-finite values by
returning 'N/A', then only reject numbers that are integers and exceed
Number.MAX_SAFE_INTEGER (return '>9Q'), allowing decimal values like 1500.5 to
proceed to formatting; also fix the comment typo from "erronious" to
"erroneous".
bebfaa6 to
b3c3013
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (1)
48-48: Remove unnecessary optional chaining for consistency.Since
tagsdefaults to[]on line 46, the optional chaining on line 48 (tags?.find()) is unnecessary. For consistency with line 41 which usestags.find()directly, consider removing the optional chaining.🔎 Proposed fix
- return tags?.find((zone: AssetGroupTag) => zone.id.toString() === tagId) || placeholderTag; + return tags.find((zone: AssetGroupTag) => zone.id.toString() === tagId) || placeholderTag;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.tspackages/javascript/bh-shared-ui/src/utils/numberFormatting.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsxpackages/javascript/js-client-library/src/responses.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/index.tsx
- packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/useSelectedTag.ts
📚 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/hooks/useSelectedTag.ts
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-27T15:12:49.129Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts:98-105
Timestamp: 2025-08-27T15:12:49.129Z
Learning: In the BloodHound codebase, for the `useDeleteQueryPermissions` hook in `packages/javascript/bh-shared-ui/src/hooks/useSavedQueries.ts`, the team prefers to use broad cache invalidation with `queryClient.invalidateQueries(savedQueryKeys.permissions)` rather than targeted invalidation for specific permission entries.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-18T22:09:32.551Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1793
File: packages/javascript/bh-shared-ui/src/hooks/useFinishedJobs/useFinishedJobs.test.ts:45-53
Timestamp: 2025-08-18T22:09:32.551Z
Learning: When suggesting changes to hook dependencies (like adding permissionsLoaded/isSuccess checks), always mention that existing tests mocking those hooks will likely need updates to include the new dependencies, as omitting them can cause unexpected test behavior or failures.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
📚 Learning: 2025-08-27T19:58:12.996Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx:89-97
Timestamp: 2025-08-27T19:58:12.996Z
Learning: In BloodHound's SaveQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryDialog.tsx), the useEffect that sets isNew based on selectedQuery existence (rather than id presence) is intentional behavior - the logic for determining whether a query is new vs existing should not be changed to check for id presence.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (2)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useAssetGroupTags(462-472)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(31-101)
packages/javascript/js-client-library/src/responses.ts (1)
packages/javascript/js-client-library/src/types.ts (1)
AssetGroupTag(140-151)
⏰ 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 (2)
packages/javascript/js-client-library/src/responses.ts (1)
54-57: LGTM! Clean type definition.The
EnvironmentExposuretype is well-defined with appropriate field types and naming conventions consistent with the backend API patterns in this file.packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts (1)
51-56: LGTM! Correctly follows React's Rules of Hooks.The implementation properly invokes both
useSelectedTagPathParams()anduseSelectedTagQueryParams()unconditionally at the top level, then returns the appropriate value based on thefromPathParamsparameter. This correctly adheres to React's Rules of Hooks.Note: While both hooks internally call
useAssetGroupTags(), React Query will deduplicate these requests since they share the same query key, so there's no performance penalty.Based on learnings, this resolution aligns with the team's discussion in past review comments where conditional hook calls and function reference returns were both deemed incorrect.
| export const useSelectedTagQueryParams = (): AssetGroupTag | typeof HygieneTag | typeof placeholderTag => { | ||
| const tags = useAssetGroupTags().data ?? []; | ||
| const { assetGroupTagId } = usePZQueryParams(); | ||
| const { tag: highestPrivilegeTag } = useHighestPrivilegeTag(); | ||
| const { data: tags = [] } = useAssetGroupTags(); | ||
|
|
||
| if (assetGroupTagId === HYGIENE_AGT_ID) return HygieneTag; | ||
|
|
||
| return tags.find((tag) => tag.id === assetGroupTagId) || highestPrivilegeTag || placeholderTag; | ||
| return tags.find((tag) => tag.id === assetGroupTagId) || placeholderTag; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 10 -B 5 "useSelectedTagQueryParams|useSelectedTagPathParams" packages/javascript/bh-shared-ui/src --type ts --type tsxRepository: SpecterOps/BloodHound
Length of output: 92
🏁 Script executed:
rg -n -A 10 -B 5 "useSelectedTagQueryParams|useSelectedTagPathParams" packages/javascript/bh-shared-ui/src --glob "*.ts" --glob "*.tsx"Repository: SpecterOps/BloodHound
Length of output: 2676
🏁 Script executed:
rg -n "import.*useSelectedTag" packages/javascript/bh-shared-ui/src --glob "*.ts" --glob "*.tsx" -A 3Repository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
rg -n "useSelectedTag(|useSelectedTagQueryParams(|useSelectedTagPathParams(" packages/javascript/bh-shared-ui/src --glob "*.ts" --glob "*.tsx" -B 5 -A 8Repository: SpecterOps/BloodHound
Length of output: 251
🏁 Script executed:
rg -n "useSelectedTag\(|useSelectedTagQueryParams\(|useSelectedTagPathParams\(" packages/javascript/bh-shared-ui/src --glob "*.ts" --glob "*.tsx" -B 3 -A 8Repository: SpecterOps/BloodHound
Length of output: 790
🏁 Script executed:
# Search for exports from this hook file and check if it's in index.ts (barrel export)
rg -n "useSelectedTag" packages/javascript/bh-shared-ui/src/hooks/index.tsRepository: SpecterOps/BloodHound
Length of output: 101
🏁 Script executed:
# Search for usage of these hooks outside the hooks directory
rg -n "useSelectedTag" packages/javascript/bh-shared-ui/src --glob "*.ts" --glob "*.tsx" | grep -v "hooks/useSelectedTag.ts"Repository: SpecterOps/BloodHound
Length of output: 153
🏁 Script executed:
# Search more broadly for files that might import from hooks
find packages/javascript/bh-shared-ui/src -type f \( -name "*.ts" -o -name "*.tsx" \) | head -20Repository: SpecterOps/BloodHound
Length of output: 1620
🏁 Script executed:
# Search across entire src for useSelectedTag usage (import or function call)
rg "useSelectedTag" packages/javascript/bh-shared-ui/src -l --glob "*.ts" --glob "*.tsx" | grep -v node_modulesRepository: SpecterOps/BloodHound
Length of output: 177
🏁 Script executed:
# Search across entire repository packages for useSelectedTag usage
rg "useSelectedTag" packages/javascript --glob "*.ts" --glob "*.tsx" | grep -v node_modules | head -30Repository: SpecterOps/BloodHound
Length of output: 769
🏁 Script executed:
# Look at useAssetGroupTags hook to understand what data it provides
rg -n -A 20 "export const useAssetGroupTags|export function useAssetGroupTags" packages/javascript/bh-shared-ui/src/hooks --glob "*.ts" --glob "*.tsx"Repository: SpecterOps/BloodHound
Length of output: 2571
🏁 Script executed:
# Check for any tests or usage hints
cat -n packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts | head -60Repository: SpecterOps/BloodHound
Length of output: 2347
🏁 Script executed:
# Check if there are similar hooks that DO expose loading states to understand the pattern
rg -n "isLoading|isError" packages/javascript/bh-shared-ui/src/hooks --glob "*.ts" --glob "*.tsx" | head -40Repository: SpecterOps/BloodHound
Length of output: 5776
🏁 Script executed:
# Check useOrderedTags return type to see the pattern for exposing loading states
rg -n -A 15 "export const useOrderedTags" packages/javascript/bh-shared-ui/src/hooks --glob "*.ts" --glob "*.tsx"Repository: SpecterOps/BloodHound
Length of output: 1975
Ensure return type includes loading and error states if consumers need to distinguish loading states from missing data.
The return types for useSelectedTagQueryParams and useSelectedTagPathParams (lines 35-49) hide loading and error states by returning placeholderTag when data is unavailable. While this works when data is undefined during loading, it makes placeholderTag indistinguishable from the "no match found" case. Other hooks in the codebase (e.g., useOrderedTags) follow the pattern of explicitly exposing isLoading and isError. Clarify whether the current design is intentional or if the return type should follow the established pattern.
🤖 Prompt for AI Agents
In packages/javascript/bh-shared-ui/src/hooks/useSelectedTag.ts around lines 35
to 42, the hook currently returns a Tag or placeholderTag which hides
loading/error states; change the hook to return an object { tag: AssetGroupTag |
typeof HygieneTag | typeof placeholderTag | null, isLoading: boolean, isError:
boolean } (or similar shape consistent with useOrderedTags), derive
isLoading/isError from useAssetGroupTags(), return null (or placeholderTag only
for “no match”) when data is loaded but no match exists, and update the function
signature and all consumers to destructure the new properties so callers can
distinguish loading/error from “no match.”
| hygiene_attack_paths: number; // While improbable this number could possibly be higher than the JavaScript max safe integer in the response | ||
| exposures: EnvironmentExposure[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and examine lines 65-66
echo "=== Finding responses.ts file ==="
find . -name "responses.ts" -type f 2>/dev/null | head -20
echo -e "\n=== Checking for file in expected location ==="
ls -la "packages/javascript/js-client-library/src/responses.ts" 2>/dev/null || echo "File not found at expected path"Repository: SpecterOps/BloodHound
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Read the file to see lines 65-66 and surrounding context
echo "=== Reading lines around 65-66 ==="
sed -n '50,80p' packages/javascript/js-client-library/src/responses.ts | cat -n
echo -e "\n=== Searching for EnvironmentExposure type definition ==="
rg -n "EnvironmentExposure" packages/javascript/js-client-library/src/responses.ts -A 5 -B 2
echo -e "\n=== Searching for MAX_SAFE_INTEGER usage anywhere in codebase ==="
rg -n "MAX_SAFE_INTEGER" --type ts --type tsx
echo -e "\n=== Searching for hygiene_attack_paths usage ==="
rg -n "hygiene_attack_paths" --type ts --type tsx -B 2 -A 2Repository: SpecterOps/BloodHound
Length of output: 1887
🏁 Script executed:
#!/bin/bash
# Search for abbreviatedNumber utility and number handling patterns
echo "=== Searching for abbreviatedNumber utility ==="
rg -n "abbreviatedNumber" --type ts -B 2 -A 5
echo -e "\n=== Searching for Environment type usage/consumption ==="
rg -n "Environment" packages/javascript/js-client-library/src/responses.ts
echo -e "\n=== Looking for JSON parsing or response deserialization patterns ==="
rg -n "JSON|parse|deserialize" packages/javascript/js-client-library/src/ --type ts -l | head -10
echo -e "\n=== Checking if there's a utility directory for number handling ==="
find packages/javascript/js-client-library/src -name "*util*" -o -name "*helper*" | head -20Repository: SpecterOps/BloodHound
Length of output: 4476
🏁 Script executed:
#!/bin/bash
# Check how hygiene_attack_paths is actually used in the UI
echo "=== Searching for hygiene_attack_paths usage in components ==="
rg -n "hygiene_attack_paths" --type ts -B 3 -A 3
echo -e "\n=== Searching for abbreviatedNumber usage with Environment/exposures context ==="
rg -n "abbreviatedNumber" --type ts -B 5 -A 2 | grep -A 10 -B 10 "hygiene\|Environment\|exposure" || echo "No direct context found"
echo -e "\n=== Checking how Environment response is consumed/parsed ==="
rg -n "Environment" packages/javascript -B 2 -A 2 --type ts | head -50Repository: SpecterOps/BloodHound
Length of output: 4587
🏁 Script executed:
#!/bin/bash
# Final check: see if there's custom JSON parsing/deserialization for Environment type
echo "=== Checking for custom JSON parsing or ReviveJSON patterns ==="
rg -n "revive|custom.*parse|JSON.parse" packages/javascript/js-client-library/src --type ts -B 2 -A 2 | head -30
echo -e "\n=== Check if exposures field has any safeguards ==="
rg -n "exposure" --type ts | grep -i "number\|safe\|bigint"Repository: SpecterOps/BloodHound
Length of output: 356
Use string type or add runtime validation for hygiene_attack_paths.
The field can exceed JavaScript's MAX_SAFE_INTEGER (2^53 - 1 = 9,007,199,254,740,991), causing precision loss and incorrect comparisons. While an abbreviatedNumber utility exists that checks Number.isSafeInteger() for display purposes, standard JSON.parse() silently corrupts large integers before safeguards run, potentially causing wrong data or financial mismatches.
If this field is used beyond display (calculations, comparisons, storage), the number type provides no protection. Either:
- Change type to
stringand parse when needed - Add custom JSON deserialization to handle values beyond the safe range
- Confirm with the backend that values will never exceed MAX_SAFE_INTEGER and document this constraint
The same consideration applies to exposure_percent in EnvironmentExposure.
🤖 Prompt for AI Agents
In packages/javascript/js-client-library/src/responses.ts around lines 65-66,
the hygiene_attack_paths field is currently typed as number which risks
precision loss for values above Number.MAX_SAFE_INTEGER; change
hygiene_attack_paths to string (and parse/validate where consumers need numeric
operations) or add JSON deserialization/runtime validation that preserves big
integers (e.g., read as string and validate range/convert with BigInt) to
prevent silent corruption; apply the same treatment to exposure_percent in
EnvironmentExposure (either string or validated parse) and update
types/comments/tests/docs to reflect the chosen approach.
Description
Downstream type updates to
Environment.Refactor
useSelectedTagto return a callback to avoid always calling both hooks.Motivation and Context
Resolves BED-6602
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
Bug Fixes
New Features
Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.