feat(agt): member filtering per environment in pz details page #1794
Conversation
urangel
added
enhancement
WalkthroughReplaces legacy DataSelector with a new SimpleEnvironmentSelector across UI, adds environment-aware query parameters and hooks (useEnvironmentIdList), updates API client methods to accept environments, adjusts Zone Management and Data Quality flows to use environment IDs/aggregation, updates tests/mocks, and pins sha.js in package.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as InfoHeader / DataQuality
participant Selector as SimpleEnvironmentSelector
participant Hook as useEnvironmentIdList
participant Query as useTagMembersInfiniteQuery
participant API as BHEAPIClient
User->>Selector: select environment/platform
Selector-->>UI: onSelect({ type, id })
UI->>Hook: request env IDs (route support)
Hook-->>UI: [envId...]
UI->>Query: fetch members(tagId, sortOrder, environments)
Query->>API: getAssetGroupTagMembers(..., environments)
API-->>Query: data
Query-->>UI: results
UI-->>User: render filtered members
sequenceDiagram
autonumber
participant UI as ZoneManagement Details
participant Hook as useSelectorMembersInfiniteQuery
participant API as BHEAPIClient
UI->>Hook: query(tagId, selectorId, sort, environments)
Hook->>API: getAssetGroupTagSelectorMembers(..., environments)
API-->>Hook: data
Hook-->>UI: result
note right of UI: Query key includes environments to scope caching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
106-110: Edit button is rendered enabled but marked disabled (inverted logic).
showEditButtonis computed as the condition to render the button; settingdisabled={showEditButton}disables it when it’s shown. Flip or remove thedisabledprop.Apply this diff:
- {showEditButton && ( - <Button asChild variant={'secondary'} disabled={showEditButton}> + {showEditButton && ( + <Button asChild variant={'secondary'} disabled={false}> <AppLink to={getSavePath(tierId, labelId, selectorId)}>Edit</AppLink> </Button> )}
🧹 Nitpick comments (28)
packages/javascript/bh-shared-ui/src/utils/datePicker.ts (1)
3-8: Nit: Align license header with common Apache-2.0 template (capitalize "You"; use https URL).Small polish to match typical Apache-2.0 headers and improve consistency.
-// Licensed under the Apache License, Version 2.0 -// you may not use this file except in compliance with the License. +// Licensed under the Apache License, Version 2.0 (the "License"); +// You may not use this file except in compliance with the License. -// http://www.apache.org/licenses/LICENSE-2.0 +// https://www.apache.org/licenses/LICENSE-2.0packages/javascript/bh-shared-ui/src/hooks/useInitialEnvironment/useInitialEnvironment.tsx (2)
41-47: Consider signaling "no initial environment" via the callbackWhen there are no environments or none are collected, the hook returns early without invoking
handleInitialEnvironment. If the consumer relies on this side-effect to initialize state, consider passingnullto make behavior explicit and symmetric.- if (!availableEnvironments?.length) return; + if (!availableEnvironments?.length) { + handleInitialEnvironment?.(null); + return; + } @@ - if (collectedEnvironments.length < 1) return; // We need to check after as well because OpenGraph allows us to create environments that are *not* collected + if (collectedEnvironments.length < 1) { + handleInitialEnvironment?.(null); + return; // We need to check after as well because OpenGraph allows us to create environments that are *not* collected + }
34-35: Minor: remove redundant defaulting of orderByYou already default
_orderBy = 'impactValue'in destructuring; the nullish coalesce on Line 49 is redundant.- const direction = (_orderBy ?? 'impactValue') === 'name' ? 'asc' : 'desc'; + const direction = _orderBy === 'name' ? 'asc' : 'desc';Also applies to: 49-51
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx (3)
30-39: Type the anchor element and click eventAvoid
anyand untyped state; improves safety and IDE hints.- const [anchorEl, setAnchorEl] = useState(null); + const [anchorEl, setAnchorEl] = useState<HTMLElement | null>(null); @@ - const handleClick = (e: any) => { + const handleClick = (e: React.MouseEvent<HTMLButtonElement>) => { setAnchorEl(e.currentTarget); };
43-50: Ensure smooth caret animation
transition-transformonly applies when open, so the rotation won’t animate on close. Always apply the transition; toggle only the rotation class.- <span className={cn({ 'rotate-180 transition-transform': open })}> + <span className={cn('transition-transform', { 'rotate-180': open })}> <AppIcon.CaretDown size={12} /> </span>
41-51: A11Y: add ARIA attributes for button/popoverProvide
aria-haspopup,aria-expanded, andaria-controlsfor better accessibility.- <Button className='w-full truncate uppercase' onClick={handleClick} data-testid='dropdown_context-selector'> + <Button + className='w-full truncate uppercase' + onClick={handleClick} + aria-haspopup='menu' + aria-expanded={open} + aria-controls='dropdown_context-selector-popover' + data-testid='dropdown_context-selector'> @@ - <Popover + <Popover open={open} anchorEl={anchorEl} onClose={handleClose} + id='dropdown_context-selector-popover'Also applies to: 63-101
packages/javascript/js-client-library/src/client.ts (1)
225-231: Nit: inconsistent skip type across related methods
skipisnumber | stringingetAssetGroupTagMembersbutnumberingetAssetGroupTagSelectorMembers. Consider aligning these to avoid unnecessary casts upstream.Also applies to: 247-251
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (1)
19-26: LGTM; small ergonomics improvement optionalThe constants and types look good and clarify platform vs environment types. Consider exporting a consolidated list for consumers that need to iterate platforms.
export const AD_PLATFORM = 'active-directory-platform' as const; export const AZ_PLATFORM = 'azure-platform' as const; +export const PLATFORM_VALUES = [AD_PLATFORM, AZ_PLATFORM] as const; +export type PlatformValue = typeof PLATFORM_VALUES[number];packages/javascript/bh-shared-ui/src/views/DataQuality/index.ts (1)
19-19: Consider a back-compat alias to honor the “non-breaking” claimThis replaces DataSelector with SimpleEnvironmentSelector on the public surface. If external consumers import DataSelector from this barrel, they’ll break. If you want to keep this non-breaking, add an alias export.
Proposed diff:
export { default as LoadContainer } from './LoadContainer'; -export * from './SimpleEnvironmentSelector'; +export * from './SimpleEnvironmentSelector'; +// Back-compat alias; remove in a future major +export { SimpleEnvironmentSelector as DataSelector } from './SimpleEnvironmentSelector'; export * from './DomainInfo'; export * from './TenantInfo';Follow-up: verify any docs or examples still referencing DataSelector and update accordingly.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx (1)
24-29: Enable the query only when environments are resolvedWhen environments is empty, the query will fire with an empty list, which may cause avoidable 400s or noisy errors. Gate the query with enabled to reduce churn and flicker.
const objectsCountQuery = useQuery({ - queryKey: ['asset-group-tags-count', tagId, ...environments], - queryFn: ({ signal }) => - apiClient.getAssetGroupTagMembersCount(tagId, environments, { signal }).then((res) => res.data.data), + queryKey: ['asset-group-tags-count', tagId, ...environments], + queryFn: ({ signal }) => + apiClient.getAssetGroupTagMembersCount(tagId, environments, { signal }).then((res) => res.data.data), + enabled: environments.length > 0, });packages/javascript/bh-shared-ui/src/hooks/useMatchingPaths.tsx (1)
17-17: Type-only import and minor readability nits
- Use a type-only import for PathPattern to avoid creating a runtime import.
- In the reducer, avoid shadowing the parameter name and simplify the boolean aggregation.
- Optional: for the single-string branch, you can pass the string directly to matchPath instead of wrapping with { path }.
Proposed diff:
-import { matchPath, PathPattern, useLocation } from 'react-router-dom'; +import { matchPath, useLocation } from 'react-router-dom'; +import type { PathPattern } from 'react-router-dom'; export const useMatchingPaths = (pattern: string | (string | PathPattern)[]) => { const { pathname } = useLocation(); if (typeof pattern === 'string') { - const match = matchPath({ path: pattern }, pathname); + const match = matchPath(pattern, pathname); return !!match?.pathname; } else { - return pattern.reduce( - (match: boolean, pattern) => (match ? match : !!matchPath(pattern, pathname)?.pathname), - false - ); + return pattern.reduce( + (found: boolean, entry) => found || !!matchPath(entry, pathname)?.pathname, + false + ); } };If you rely on partial matches, consider explicitly specifying end: false wherever you pass an object pattern to keep behavior consistent across call sites.
Also applies to: 19-19, 26-26
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
77-77: Confirm route pattern for aggregation support; consider matching all Zone Management routes.Passing
['zone-management']relies on react-router’s partial matching and may not cover all nested routes (/zone-management/details/*,/zone-management/save/*). Recommend using explicit patterns to avoid surprises.If desired, you can centralize and use a more explicit pattern:
- const environments = useEnvironmentIdList(['zone-management']); + const environments = useEnvironmentIdList(['/zone-management/*']);cmd/ui/src/views/ZoneManagement/InfoHeader.tsx (1)
52-60: Unnecessary empty errorMessage prop.
errorMessage={''}adds no value. Let the component decide default rendering when no error is present.- <SimpleEnvironmentSelector + <SimpleEnvironmentSelector selected={{ type: selectedEnvironment?.type ?? null, id: selectedEnvironment?.id ?? null, }} - errorMessage={''} buttonPrimary={false} onSelect={handleSelect} />packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (2)
40-55: Optional: expose a readiness flag to help query gating.Today, consumers fire queries regardless of environment availability and then refetch. Consider returning
[ids, ready]or a separateuseEnvironmentFilterthat includes a boolean, so queries canenabled: ready.
40-55: Ensure empty environment list is treated as “no filter” not “filter none”We audited all callers of useEnvironmentIdList and found two sites where an empty array ([]) is spread into query keys and passed to the API:
• packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx
– Line 24: const environments = useEnvironmentIdList(['zone-management'])
– Lines 26–28: queryKey includes ...environments, and apiClient.getAssetGroupTagMembersCount(tagId, environments)• packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx
– Line 77: const environments = useEnvironmentIdList(['zone-management'])Next steps:
• Confirm that calling getAssetGroupTagMembersCount(tagId, []) omits environment params (no filter) rather than filtering out all results.
• Either document this hook’s contract (“returns [] ⇒ no filter”) or change its signature to return undefined for “no filter” and normalize consumers to handle undefined.cmd/ui/src/views/QA/QA.tsx (1)
131-136: Minor consistency nit: avoid unnecessary object spread in onSelect.In the bottom selector you spread the selection, while above you pass it through. Prefer one style.
- onSelect={(selection) => setSelectedEnvironment({ ...selection })} + onSelect={(selection) => setSelectedEnvironment(selection)}packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (2)
156-164: SimpleEnvironmentSelector integration looks good; consider aligning button styling.Other usages pass
buttonPrimary={false}to standardize appearance. If that’s the intended design here, add it for visual consistency.- <SimpleEnvironmentSelector + <SimpleEnvironmentSelector selected={selectedEnvironment || globalEnvironment || { type: null, id: null }} errorMessage={domainSelectorErrorMessage} - onSelect={(selection: SelectedEnvironment) => + buttonPrimary={false} + onSelect={(selection: SelectedEnvironment) => setSelectedEnvironment({ ...selection }) } />
121-133: Effect dependencies include selectedAssetGroupId but don’t use it in the filter calculation.The filter built here depends only on the environment, not the asset group. Consider dropping
selectedAssetGroupIdfrom deps to avoid unnecessary recomputation.- }, [selectedEnvironment, globalEnvironment, selectedAssetGroupId]); + }, [selectedEnvironment, globalEnvironment]);packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (7)
81-86: Use the next open state from onOpenChange to avoid desync/flickerRadix-style popovers pass the next open value. Toggling based on previous state can lead to double-toggles or desync when closing via outside click/ESC.
Apply this diff:
- onOpenChange={() => { - setOpen((prev) => !prev); - }}> + onOpenChange={(nextOpen) => { + setOpen(nextOpen); + }}>
170-181: Disable "All ..." options based on collected environments, not mere presenceThe list only shows collected environments, but these buttons enable when there are any environments of that type (even uncollected). This can be misleading and produce empty results.
Apply this diff:
- disabled={!data?.filter((env) => env.type === 'active-directory').length} + disabled={!data?.some((env) => env.type === 'active-directory' && env.collected)}- disabled={!data?.filter((env) => env.type === 'azure').length} + disabled={!data?.some((env) => env.type === 'azure' && env.collected)}Also applies to: 182-194
126-131: Sort names case-insensitively for a better UXMakes sorting stable regardless of name casing.
Apply this diff:
- ?.sort((a: Environment, b: Environment) => { - return a.name.localeCompare(b.name); - }) + ?.sort((a: Environment, b: Environment) => { + return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' }); + })
133-137: Use cn consistently instead of mixing clsx and cnYou already import cn; using a single helper improves consistency.
Apply this diff:
- className={clsx( + className={cn( index === filteredEnvironments.length - 1 && 'border-b border-neutral-light-5 pb-2 mb-2' )}>
145-161: Avoid per-item TooltipProvider; wrap once to reduce providersCreating a TooltipProvider for every item is unnecessary overhead. Wrap the list with a single provider.
Apply this diff within the item:
- <TooltipProvider> - <TooltipRoot> - <TooltipTrigger> + <TooltipRoot> + <TooltipTrigger> <span className='uppercase max-w-96 truncate'> {environment.name} </span> - </TooltipTrigger> - <TooltipPortal> - <TooltipContent - side='left' - className='dark:bg-neutral-dark-5 border-0'> - <span className='uppercase'>{environment.name}</span> - </TooltipContent> - </TooltipPortal> - </TooltipRoot> - </TooltipProvider> + </TooltipTrigger> + <TooltipPortal> + <TooltipContent side='left' className='dark:bg-neutral-dark-5 border-0'> + <span className='uppercase'>{environment.name}</span> + </TooltipContent> + </TooltipPortal> + </TooltipRoot>And wrap the list once (outside the selected lines; example):
// Around the <ul> block <TooltipProvider> <ul> {/* items */} </ul> </TooltipProvider>
82-82: Align data-testid with component intent"data-selector" is generic and could collide with prior components. Consider a more specific id to keep tests unambiguous.
Apply this diff (verify tests before changing):
- data-testid='data-selector' + data-testid='environment-selector'
88-95: Consider using a proper Button variant instead of overriding primary stylesIf DoodleUI exposes variants (e.g., secondary/outline), prefer them over heavy class overrides for !buttonPrimary.
Would you like me to adjust the variant and classes once you confirm the intended visual spec for the non-primary state?
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
69-76: Stabilize query keys by sorting environments to avoid duplicate cachesSpreading environments directly into the key makes order matter. Sorting produces a canonical key for the same set.
Apply this diff:
- membersByTag: (tagId: string | number, sortOrder: SortOrder, environments: string[] = []) => - [...zoneManagementKeys.members(), 'tag', tagId, sortOrder, ...environments] as const, + membersByTag: (tagId: string | number, sortOrder: SortOrder, environments: string[] = []) => { + const envsKey = [...environments].sort(); + return [...zoneManagementKeys.members(), 'tag', tagId, sortOrder, ...envsKey] as const; + },- ) => [...zoneManagementKeys.members(), 'tag', tagId, 'selector', selectorId, sortOrder, ...environments] as const, + ) => { + const envsKey = [...environments].sort(); + return [...zoneManagementKeys.members(), 'tag', tagId, 'selector', selectorId, sortOrder, ...envsKey] as const; + },Also applies to: 71-77
141-147: Default sortOrder in hook signature for stable query keysAllowing undefined to flow into the key can fragment caches. Default to 'asc' at the hook boundary.
Apply this diff:
-export const useTagMembersInfiniteQuery = ( - tagId: number | string | undefined, - sortOrder: SortOrder, - environments?: string[] -) => +export const useTagMembersInfiniteQuery = ( + tagId: number | string | undefined, + sortOrder: SortOrder = 'asc', + environments?: string[] +) =>
182-187: Default sortOrder in selector-members hook as wellConsistent with the tag-members hook, ensure stable keys by defaulting.
Apply this diff:
-export const useSelectorMembersInfiniteQuery = ( - tagId: number | string | undefined, - selectorId: number | string | undefined, - sortOrder: SortOrder, - environments?: string[] -) => +export const useSelectorMembersInfiniteQuery = ( + tagId: number | string | undefined, + selectorId: number | string | undefined, + sortOrder: SortOrder = 'asc', + environments?: string[] +) =>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
cmd/ui/src/views/QA/QA.tsx(3 hunks)cmd/ui/src/views/ZoneManagement/InfoHeader.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/index.ts(0 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/types.ts(0 hunks)packages/javascript/bh-shared-ui/src/hooks/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(5 hunks)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useInitialEnvironment/useInitialEnvironment.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useMatchingPaths.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/datePicker.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/DataSelector/DataSelector.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/javascript/bh-shared-ui/src/components/GroupManagementContent/types.ts
- packages/javascript/bh-shared-ui/src/views/DataQuality/DataSelector/DataSelector.tsx
- packages/javascript/bh-shared-ui/src/components/GroupManagementContent/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (3)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentParams/useEnvironmentParams.tsx (1)
EnvironmentAggregation(23-23)packages/javascript/bh-shared-ui/src/hooks/useAvailableEnvironments/useAvailableEnvironments.tsx (2)
useAvailableEnvironments(33-41)useSelectedEnvironment(43-72)packages/javascript/bh-shared-ui/src/hooks/useMatchingPaths.tsx (1)
useMatchingPaths(19-30)
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx (2)
packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
cmd/ui/src/views/ZoneManagement/InfoHeader.tsx (5)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
useHighestPrivilegeTagId(359-364)packages/javascript/bh-shared-ui/src/hooks/useInitialEnvironment/useInitialEnvironment.tsx (1)
useInitialEnvironment(33-63)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (3)
SelectedEnvironment(25-25)AD_PLATFORM(19-19)AZ_PLATFORM(20-20)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentParams/useEnvironmentParams.tsx (1)
useEnvironmentParams(47-63)packages/javascript/bh-shared-ui/src/views/ZoneManagement/utils.tsx (1)
getTagUrlValue(20-22)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (4)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (2)
SelectedEnvironment(25-25)SelectorValueTypes(23-23)packages/javascript/bh-shared-ui/src/hooks/useAvailableEnvironments/useAvailableEnvironments.tsx (1)
useAvailableEnvironments(33-41)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(40-55)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useSelectorMembersInfiniteQuery(182-207)useTagMembersInfiniteQuery(141-157)
packages/javascript/js-client-library/src/client.ts (2)
packages/javascript/js-client-library/src/requests.ts (1)
RequestOptions(26-26)packages/javascript/js-client-library/src/responses.ts (2)
AssetGroupTagMembersResponse(190-190)AssetGroupMemberCountsResponse(232-232)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(40-55)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(177-179)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (1)
SelectedEnvironment(25-25)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (8)
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx (1)
25-30: No fullWidth prop usages detected—no updates needed
Search across.ts/.tsx/.js/.jsxfiles found zero instances of<DropdownSelector fullWidth=…>. The only import and usage inGroupManagementContent.tsxdoesn’t includefullWidth. Removal of this prop introduces no breaking changes.packages/javascript/js-client-library/src/client.ts (1)
225-232: Signature change: confirm all call sites updated for newenvironmentsparameterWe’ve inserted an optional
environments?: string[]argument beforeoptions?: RequestOptionsin three public methods. No internal usages were found in this repository—ensure any external callers are updated to pass (or omit) the newenvironmentsparameter to avoid misaligned arguments.Affected methods:
getAssetGroupTagMembers(lines 225–232)getAssetGroupTagSelectorMembers(lines 244–252)getAssetGroupTagMembersCount(lines 267–268)packages/javascript/bh-shared-ui/src/hooks/index.ts (1)
35-36: Re-export addition looks goodRe-exporting
useEnvironmentIdListfrom the hooks index improves discoverability and keeps imports consistent.packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/index.ts (1)
17-17: Re-export looks goodNamed re-export aligns with the new component and keeps types next to it via export *. No issues.
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx (1)
20-20: Tests updated to new API look correctImports and prop renames (selected/onSelect) align with the new component. The interactions still cover the essential UX paths.
Also applies to: 268-269, 293-294, 343-343, 373-379
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (1)
95-98: Prevent emptyenvironmentsfrom zeroing out resultsWhen no environment is selected, our hooks return
[], which the API may interpret as “filter to none” and return an empty list. It’s safer to omit theenvironmentsparameter (i.e. passundefined) until at least one environment is chosen.— Define once per component/hook:
const normalizedEnvs = environments.length ? environments : undefined;— In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(lines 95–98):- const selectorMembersQuery = useSelectorMembersInfiniteQuery(tagId, selectorId, membersListSortOrder, environments); - const tagMembersQuery = useTagMembersInfiniteQuery(tagId, membersListSortOrder, environments); + const selectorMembersQuery = useSelectorMembersInfiniteQuery( + tagId, + selectorId, + membersListSortOrder, + normalizedEnvs + ); + const tagMembersQuery = useTagMembersInfiniteQuery( + tagId, + membersListSortOrder, + normalizedEnvs + );— In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx:- queryKey: ['asset-group-tags-count', tagId, ...environments], - queryFn: ({ signal }) => - apiClient.getAssetGroupTagMembersCount(tagId, environments, { signal }), + queryKey: ['asset-group-tags-count', tagId, ...(normalizedEnvs ?? [])], + queryFn: ({ signal }) => + apiClient.getAssetGroupTagMembersCount(tagId, normalizedEnvs, { signal }),— In
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx: replaceenvironmentswithnormalizedEnvswhen calling
apiClient.getAssetGroupTagMembers(...)getAssetGroupSelectorMembers(...)To confirm how the generated API methods serialize an empty array vs.
undefined, you can inspect their signatures andqueryblocks:rg -n -C3 'getAssetGroup(TagMembers|SelectorMembers|TagMembersCount)' -t ts packages/javascript/bh-shared-ui/src/apiThis will show whether passing
undefinedomits theenvironmentsparam entirely, preventing unintended “empty” filters.packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
22-38: Aggregation helper looks good; covers 'all' and type-specific cases, and filters to collected.No functional concerns.
cmd/ui/src/views/QA/QA.tsx (1)
106-114: Migration to SimpleEnvironmentSelector looks correct.Prop mapping (selected/onSelect/buttonPrimary) and error handling align with the new component. No functional issues spotted.
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (1)
84-114: Make the test timezone-stable and drop redundant act wrapping
- The expectation '2024/07/25' depends on the local timezone, while updated_at is '2024-07-26T02:15:04.556Z'. This will be flaky across environments (UTC vs. local). Use a mid-day timestamp or assert against a timezone-agnostic format.
- The render call is already wrapped by RTL’s act; the explicit await act(...) around render is unnecessary for this case.
Suggested changes:
- Use a mid-day UTC timestamp to avoid date rollover.
- Remove the manual act wrapper.
- it('renders details for a selected tier', async () => { + it('renders details for a selected tier', () => { const testTag = { isLoading: false, isError: false, isSuccess: true, data: { requireCertify: true, created_at: '2024-09-08T03:38:22.791Z', created_by: '[email protected]', deleted_at: '2025-02-03T18:32:36.669Z', deleted_by: '[email protected]', description: 'pique International', id: 9, kind_id: 59514, name: 'Tier-8', - updated_at: '2024-07-26T02:15:04.556Z', + updated_at: '2024-07-26T12:15:04.556Z', updated_by: '[email protected]', position: 0, type: 1 as AssetGroupTagTypes, }, } as unknown as UseQueryResult<AssetGroupTag | undefined>; - await act(async () => { - render(<DynamicDetails queryResult={testTag} />); - }); + render(<DynamicDetails queryResult={testTag} />); expect(screen.getByText('Tier-8')).toBeInTheDocument(); expect(screen.getByText('pique International')).toBeInTheDocument(); expect(screen.getByText('[email protected]')).toBeInTheDocument(); - expect(screen.getByText('2024/07/25')).toBeInTheDocument(); + expect(screen.getByText('2024/07/26')).toBeInTheDocument(); });Optionally, if the component does async work post-mount, prefer awaiting a findBy* assertion instead of wrapping render in act:
// Example render(<DynamicDetails queryResult={testTag} />); await screen.findByText('Tier-8');
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts (2)
55-63: Fix misleading comment and consider a more realistic mock payload for available domains
- The comment says "GET Kinds" but the route is "/api/v2/available-domains".
- Optional: returning an empty array works, but seeding a minimal object (e.g., one domain/environment) tends to catch UI edge cases and better reflects the new environment filtering feature.
Apply this comment fix:
- // GET Kinds + // GET Available Domains
175-198: Mocks ignore environment filtering; consider reflecting environments[] query paramGiven the PR introduces environment-scoped filtering, these member endpoints should optionally honor the environments[] query param to better exercise the UI logic in tests. Right now they return the same dataset regardless of filters, which can mask issues.
If helpful, I can draft MSW handlers that:
- read url.searchParams.getAll('environments[]')
- tag mock members with an environment_id/domain_id
- filter before returning members/count
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (1)
27-27: Remove unnecessary act importReact Testing Library already wraps render in act; the manual import is typically redundant unless you’re explicitly testing synchronous batched state updates.
-import { act, render, screen } from '../../../test-utils'; +import { render, screen } from '../../../test-utils';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts (1)
69-76: Confirm the response shape for /api/v2/users/ matches consumer expectations*You switched this mock to return a JSON body. Please verify downstream code reads this shape (data.props) and doesn’t expect e.g. data.user or a richer payload. If needed, return a minimal user object to avoid undefined property access at runtime.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (1)
71-77: LGTM: adds MSW handler for available domainsThis aligns with the new environment selection flow and keeps the test self-contained.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx (5)
263-263: Suite title updated; align remaining suite names for consistencyNice rename. For consistency, consider renaming the other suites still titled “Context Selector” (Line 311 and Line 356) to “Simple Environment Selector”.
Example:
describe('Simple Environment Selector', () => { // ... }); describe('Simple Environment Selector', () => { // ... }); describe('Simple Environment Selector Error', () => { // ... });
269-288: Minor RTL assertion improvements and reduce brittleness in count check
- Prefer “not.toBeInTheDocument()” over “toBeNull()” for readability (Line 274).
- Compute the expected count from fixtures to avoid brittle hard-coding (Line 287). If the test fixture changes, the test won’t need updates.
- expect(screen.queryByLabelText('Search')).toBeNull(); + expect(screen.queryByLabelText('Search')).not.toBeInTheDocument(); @@ - //Both AD and Azure are available so 30 menu items are available plus both show All items for a count of 32 - expect(within(container).getAllByRole('button')).toHaveLength(32); + // Both AD and Azure are available; count collected items plus the two "All" options + const expectedCount = testDomains.filter((d) => d.collected).length + 2; + expect(within(container).getAllByRole('button')).toHaveLength(expectedCount);
290-308: Clarify test intent and slightly strengthen expectationsThe test name mentions “initiate data loading,” but the assertions focus on callback invocation. Either add a loading-state assertion or rename for clarity. Also, asserting the call count helps catch extra/unexpected invocations.
- it('should initiate data loading when an item is selected', async () => { + it('should invoke onSelect when an item is selected', async () => { @@ - expect(testOnChange).toHaveBeenLastCalledWith({ type: 'active-directory-platform', id: null }); + expect(testOnChange).toHaveBeenLastCalledWith({ type: 'active-directory-platform', id: null }); + expect(testOnChange).toHaveBeenCalledTimes(2);
344-353: Strengthen negative case by asserting absent, non-collected domainIn addition to checking the total, assert the non-collected domain isn’t rendered. This guards against regressions where uncollected items slip into the list.
const container = await screen.findByTestId('data-quality_context-selector-popover'); //Only one of the served domains is collected so only one list item plus the two options for all domains and all tenants are displayed. expect(within(container).getAllByRole('button')).toHaveLength(3); + expect(screen.queryByText('ryleigh.com')).not.toBeInTheDocument();
357-369: Avoid permanently stubbing console.error; use a spy and restoreDirectly assigning console.error can leak across tests if not restored. Use a spy with restoration to keep the environment clean.
Example:
describe('Context Selector Error', () => { let consoleErrorSpy: ReturnType<typeof vi.spyOn>; beforeEach(() => { consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); server.use( rest.get(`/api/v2/available-domains`, (req, res, ctx) => res(ctx.status(500), ctx.json({ errorMessage: 'Internal Server Error' })) ) ); }); afterEach(() => { consoleErrorSpy.mockRestore(); }); // ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (2)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx (2)
20-20: Use of public re-export looks goodImporting from the local index keeps the test aligned with the public API surface for this component. No action needed.
375-381: Passing errorMessage as a ReactNode is correctGood update to pass a ReactNode instead of a plain string, matching the new prop type and allowing richer content.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (5)
34-34: Memoize filter/sort and remove potential undefined reference inside map
- Avoid re-filtering/re-sorting every render.
- Ensure an always-array source for
.map(...)to prevent “object is possibly undefined” hazards and to stop referencingfilteredEnvironments.lengthwhile chaining optional operations.-import React, { ReactNode, useState } from 'react'; +import React, { ReactNode, useMemo, useState } from 'react';- const filteredEnvironments = data?.filter( - (environment: Environment) => - environment.name.toLowerCase().includes(searchInput.toLowerCase()) && environment.collected - ); + const filteredEnvironments = useMemo<Environment[]>(() => { + const list = + data?.filter( + (environment: Environment) => + environment.name.toLowerCase().includes(searchInput.toLowerCase()) && environment.collected + ) ?? []; + // Sort once, together with filtering + return list.sort((a: Environment, b: Environment) => a.name.localeCompare(b.name)); + }, [data, searchInput]);- {filteredEnvironments - ?.sort((a: Environment, b: Environment) => { - return a.name.localeCompare(b.name); - }) - .map((environment: Environment, index: number) => { + {filteredEnvironments.map((environment: Environment, index: number) => {Also applies to: 73-79, 125-129
38-38: Prefer exported constants over string literals for platform typesAvoid magic strings for platform discriminants; it reduces typo risk and centralizes changes.
-import { SelectedEnvironment, SelectorValueTypes } from './types'; +import { AD_PLATFORM, AZ_PLATFORM, SelectedEnvironment, SelectorValueTypes } from './types';- if (selected.type === 'active-directory-platform') { + if (selected.type === AD_PLATFORM) { return 'All Active Directory Domains'; - } else if (selected.type === 'azure-platform') { + } else if (selected.type === AZ_PLATFORM) { return 'All Azure Tenants';- onClick={() => { - onSelect({ type: 'active-directory-platform', id: null }); + onClick={() => { + onSelect({ type: AD_PLATFORM, id: null }); handleClose(); }}- onClick={() => { - onSelect({ type: 'azure-platform', id: null }); + onClick={() => { + onSelect({ type: AZ_PLATFORM, id: null }); handleClose(); }}Also applies to: 41-45, 172-175, 183-186
87-93: Variant should reflectbuttonPrimaryRight now the button uses
variant="primary"regardless ofbuttonPrimary. Let the variant match the prop to avoid relying on overriding classes.- <Button - variant={'primary'} + <Button + variant={buttonPrimary ? 'primary' : 'text'} className={cn({ 'bg-transparent rounded-md border uppercase shadow-outer-0 hover:bg-neutral-3 text-black dark:text-white hover:text-white truncate': !buttonPrimary, 'w-full': buttonPrimary, })}
133-136: Unifyclsx/cnusage
cnalready wraps class merging; using bothclsxandcnis inconsistent. Prefer one utility.- className={clsx( + className={cn( index === filteredEnvironments.length - 1 && 'border-b border-neutral-light-5 pb-2 mb-2' )}>If you remove
clsx, also drop its import:-import clsx from 'clsx';Also applies to: 32-32
175-176: Disable “All …” options only when no collected envs existThe list above filters by
collected. For consistency, gate the “All …” selections on the presence of at least one collected env of that type.- disabled={!data?.filter((env) => env.type === 'active-directory').length} + disabled={!data?.some((env) => env.type === 'active-directory' && env.collected)}- disabled={!data?.filter((env) => env.type === 'azure').length} + disabled={!data?.some((env) => env.type === 'azure' && env.collected)}Also applies to: 188-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cmd/ui/src/views/GroupManagement/GroupManagement.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx(3 hunks)packages/javascript/js-client-library/src/client.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/ui/src/views/GroupManagement/GroupManagement.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (1)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (1)
141-144: Type compatibility ofonSelectpayload is correct
TheSelectorValueTypesunion is defined asexport type SelectorValueTypes = Environment['type'] /* 'active-directory' | 'azure' */ | EnvironmentPlatform /* 'active-directory-platform' | 'azure-platform' */;Since
environment.typeis one of'active-directory' | 'azure', it satisfies theSelectorValueTypesrequirement.
...t/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
Outdated
Show resolved
Hide resolved
urangel
added
the
blocked
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
19-20: Align root constant across packages to avoid driftThere’s an existing ROUTE_ZONE_MANAGEMENT_ROOT in cmd/ui/src/routes/constants.ts built as ROUTE_ZONE_MANAGEMENT + '*'. Consider unifying or re-exporting from a single module to prevent divergence between packages, or documenting the intent if they’re meant to differ (pattern vs. literal base).
Would you like me to propose a small consolidation PR to share a single source-of-truth for these constants?
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (2)
78-81: Coalesce empty environment selection to undefined to avoid “filter-to-none”If no environment is selected, useEnvironmentIdList returns an empty array. Passing [] to the APIs can semantically mean “no environments” (often resulting in zero results) versus undefined meaning “no filtering.” To be safe, coalesce [] to undefined. Optionally, sort for key stability to avoid cache thrash if order is non-deterministic.
Please confirm backend semantics for environments: [] vs undefined. If [] means “filter to none,” apply the change below.
- const environments = useEnvironmentIdList([ - { path: ROUTE_ZONE_MANAGEMENT_ROOT + ROUTE_ZONE_MANAGEMENT_DETAILS, caseSensitive: false, end: false }, - ]); + const environmentIds = useEnvironmentIdList([ + { path: ROUTE_ZONE_MANAGEMENT_ROOT + ROUTE_ZONE_MANAGEMENT_DETAILS, caseSensitive: false, end: false }, + ]); + // Use undefined to indicate "no filtering" when nothing is selected; sort for stable query keys. + const environments = environmentIds.length ? [...environmentIds].sort() : undefined;
78-81: Optional: extract an absolute details PathPattern constant for reuseYou’re composing { path: ROUTE_ZONE_MANAGEMENT_ROOT + ROUTE_ZONE_MANAGEMENT_DETAILS, ... } here. Consider exporting a PathPattern (or absolute string) from routes to standardize usage across components and tests, reducing future string concat drift.
If you’d like, I can follow up with a small patch to add something like:
- export const ROUTE_ZONE_MANAGEMENT_DETAILS_ABS =
${ROUTE_ZONE_MANAGEMENT_ROOT}${ROUTE_ZONE_MANAGEMENT_DETAILS};- or export const ZONE_MANAGEMENT_DETAILS_PATTERN = { path: ROUTE_ZONE_MANAGEMENT_ROOT + ROUTE_ZONE_MANAGEMENT_DETAILS, end: false };
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx (2)
204-220: Fix typo in test name and optionally assert client is called with environmentsTypo: "envrionment" → "environment" (Line 204). Also consider asserting the last call includes the environments array for stronger coverage.
Apply this diff for the typo:
- test('selector members refetches on envrionment change', async () => { + test('selector members refetches on environment change', async () => {Optionally, after the second waitFor, add an expectation on the spy’s last call to verify the environments parameter (exact matcher depends on the method signature; see the verification script in my previous comment).
168-185: LGTM: refetch-on-environment-change behavior is covered; consider tightening assertions and making the initial environments explicitThe test correctly ensures that changing
environmentstriggers a refetch. To harden it further, you might:
- Assert that the API client is called with the correct
environmentsargument (5th parameter) to catch any regressions in plumbing. For example:await waitFor(() => expect(tagMembersSpy).toHaveBeenNthCalledWith( 2, 1, expect.any(Number), // skip expect.any(Number), // limit 'name', // sort_by ['1'], // environments undefined // options ) );- Initialize the hook with a concrete environments value (e.g.
[]) instead ofundefinedto make the refetch trigger explicit:const { rerender } = renderHook( (envs: string[]) => useTagMembersInfiniteQuery(1, 'asc', envs), { initialProps: [] } ); // … rerender(['1']);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/ui/src/views/ZoneManagement/InfoHeader.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/routes/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ui/src/views/ZoneManagement/InfoHeader.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
cmd/ui/src/routes/constants.ts (1)
ROUTE_ZONE_MANAGEMENT_ROOT(21-21)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (3)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(40-55)packages/javascript/bh-shared-ui/src/routes/index.tsx (2)
ROUTE_ZONE_MANAGEMENT_ROOT(19-19)ROUTE_ZONE_MANAGEMENT_DETAILS(21-21)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useSelectorMembersInfiniteQuery(182-207)useTagMembersInfiniteQuery(141-157)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (4)
packages/javascript/bh-shared-ui/src/routes/index.tsx (1)
19-20: Good addition: base route constant for Zone ManagementDefining ROUTE_ZONE_MANAGEMENT_ROOT centralizes the base path and avoids scattered string literals.
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx (3)
30-31: LGTM: imports for environment-aware routingImporting useEnvironmentIdList and the route constants is consistent with the new environment-scoped data flow.
98-98: LGTM: selector members are now environment-scopedWiring environments into useSelectorMembersInfiniteQuery matches the new hook signature and keeps queries aligned with route context.
100-100: LGTM: tag members are now environment-scopedPassing environments into useTagMembersInfiniteQuery is consistent with the API and hook changes.
urangel
removed
the
blocked
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
121-133: Guard against null environment to avoid sendingeq:undefined; also clear stale env keys and avoid resetting non-env filters.If both
selectedEnvironmentandglobalEnvironmentare null (or cleared), theelsebranch buildsenvironment_id = "eq:undefined". This will produce incorrect requests and may confuse the backend and caching. Additionally, when switching between a platform selection and a concrete environment, the previous key (e.g.,environment_idvsenvironment_kind) can linger. Lastly, includingselectedAssetGroupIdin the deps causes this effect to run on group changes and wipes other user-applied filters since you rebuild thefilterobject from scratch.Fix by:
- Guarding against missing env.
- Removing stale env keys before setting the new one.
- Preserving other filter fields via functional
setState.- Dropping
selectedAssetGroupIdfrom deps unless intentionally resetting filters on group change.Apply this diff:
- useEffect(() => { - const filterDomain = selectedEnvironment || globalEnvironment; - const filter: AssetGroupMemberParams = {}; - if (filterDomain?.type === 'active-directory-platform') { - filter.environment_kind = 'eq:Domain'; - } else if (filterDomain?.type === 'azure-platform') { - filter.environment_kind = 'eq:AZTenant'; - } else { - filter.environment_id = `eq:${filterDomain?.id}`; - } - setFilterParams(filter); - }, [selectedEnvironment, globalEnvironment, selectedAssetGroupId]); + useEffect(() => { + const filterDomain = selectedEnvironment || globalEnvironment; + setFilterParams((prev) => { + // preserve other filters but rebuild environment filters + const next: AssetGroupMemberParams = { ...prev }; + delete next.environment_id; + delete next.environment_kind; + + // No environment selected — leave env filters unset + if (!filterDomain) return next; + + if (filterDomain.type === 'active-directory-platform') { + next.environment_kind = 'eq:Domain'; + } else if (filterDomain.type === 'azure-platform') { + next.environment_kind = 'eq:AZTenant'; + } else if (filterDomain.id) { + next.environment_id = `eq:${filterDomain.id}`; + } + return next; + }); + }, [selectedEnvironment, globalEnvironment]);packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (1)
84-114: Action Required: Stabilize date fixture to prevent timezone-dependent test failuresIn packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (lines 84–114), the test currently uses:
updated_at: '2024-07-26T02:15:04.556Z',When rendered in a UTC‐based CI environment this formats as “2024/07/26” instead of the expected “2024/07/25,” leading to intermittent failures in timezones east of UTC. Update the fixture to a midday UTC timestamp so all environments render “2024/07/25” consistently:
- updated_at: '2024-07-26T02:15:04.556Z', + // Midday UTC to avoid TZ rollovers in CI + updated_at: '2024-07-25T12:15:04.556Z',• Additionally, please confirm that the displayed date field (“updated_at”) aligns with the intended label in the UI. If the design should show “created_at” or another field instead, adjust the label/value pairing accordingly.
♻️ Duplicate comments (3)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx (1)
26-27: Stabilize cache key: ensure environments order is deterministic and optionally label the segmentIf environments are produced with varying order, the queryKey will thrash and cause unnecessary refetches. Ensure the upstream hook returns a sorted array. Optionally, add a label before spreading to make the key more self-descriptive.
Apply this minimal key tweak (optional but clearer):
- queryKey: ['asset-group-tags-count', tagId, ...environments], + queryKey: ['asset-group-tags-count', tagId, 'environments', ...environments],And address sorting in getEnvironmentAggregationIds (see my comment in useEnvironmentIdList.tsx).
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
22-38: Return environments in a stable order to prevent cache thrashinggetEnvironmentAggregationIds currently preserves input order. If the upstream source yields variable ordering, react-query keys will change unnecessarily. Sort the IDs before returning.
Apply this diff within the selected lines:
- return aggregationIds; + // Sort IDs to guarantee a stable order for cache keys + return aggregationIds.sort((a, b) => String(a).localeCompare(String(b)));packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
69-77: Fix query key for membersByTagAndSelector to include the common prefixThis omits zoneManagementKeys.members(), making it inconsistent with membersByTag and prone to collisions/missed invalidations.
Apply this diff within the selected lines:
- ) => ['tag', tagId, 'selector', selectorId, sortOrder, ...environments] as const, + ) => [...zoneManagementKeys.members(), 'tag', tagId, 'selector', selectorId, sortOrder, ...environments] as const,
🧹 Nitpick comments (12)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (2)
79-95: Use a stable queryKey; avoid putting objects into react-query keys.
selectedAssetGroupis an object and will change identity across renders, defeating caching semantics and causing redundant refetches. UseselectedAssetGroupId(a primitive) instead.Apply this diff:
- const { data: memberCounts } = useQuery({ - queryKey: [ - 'getAssetGroupMembersCount', - filterParams.environment_id, - filterParams.environment_kind, - selectedAssetGroup, - ], + const { data: memberCounts } = useQuery({ + queryKey: [ + 'getAssetGroupMembersCount', + selectedAssetGroupId, + filterParams.environment_id, + filterParams.environment_kind, + ], enabled: !!selectedAssetGroupId, queryFn: ({ signal }) => apiClient .getAssetGroupMembersCount( - selectedAssetGroupId?.toString() ?? '', // This query will only execute if selectedAssetGroup is truthy. + selectedAssetGroupId?.toString() ?? '', // guarded by `enabled` { environment_id: filterParams.environment_id, environment_kind: filterParams.environment_kind }, { signal } ) .then((res) => res.data.data), });
156-163: Stabilizeselectedprop identity and avoid unnecessary object cloning inonSelect.Passing a fresh literal
{ type: null, id: null }on every render can cause needless re-renders downstream. Also, cloningselectionon set is unnecessary.Apply this diff:
- <SimpleEnvironmentSelector - selected={selectedEnvironment || globalEnvironment || { type: null, id: null }} + <SimpleEnvironmentSelector + selected={selectedEnvironment || globalEnvironment || EMPTY_ENV} errorMessage={domainSelectorErrorMessage} - onSelect={(selection: SelectedEnvironment) => - setSelectedEnvironment({ ...selection }) - } + onSelect={(selection: SelectedEnvironment) => setSelectedEnvironment(selection)} />Add this helper near other hooks:
// keep outside of JSX to avoid new-object each render const EMPTY_ENV = useMemo<SelectedEnvironment>(() => ({ type: null, id: null }), []);packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx (6)
27-27: Unnecessary act import; prefer consistent async handling (or none) across testsRTL’s render already wraps updates in act. Importing act here adds noise and is only used in one test. Either remove it and keep the test synchronous, or switch to findBy*/waitFor for async expectations consistently.
Apply this diff to remove the import (paired with the test change below):
-import { act, render, screen } from '../../../test-utils'; +import { render, screen } from '../../../test-utils';
106-108: Remove explicit act wrapper around renderGiven the data is injected via the queryResult prop and not fetched in this test, the explicit act is unnecessary. Keeping this test synchronous improves consistency with the two tests below.
Apply this diff (pairs with the import change above):
- await act(async () => { - render(<DynamicDetails queryResult={testTag} />); - }); + render(<DynamicDetails queryResult={testTag} />);If you expect async UI updates, prefer:
- render(<DynamicDetails queryResult={testTag} />); - expect(screen.getByText('Tier-8')).toBeInTheDocument(); + render(<DynamicDetails queryResult={testTag} />); + expect(await screen.findByText('Tier-8')).toBeInTheDocument();
116-138: Generic type mismatch for UseQueryResult in the “Cypher” selector testThe data shape is AssetGroupTagSelector, but the cast uses UseQueryResult<AssetGroupTag | undefined>. Correct the generic to reflect the actual payload type to avoid misleading types and future refactor breakage.
- } as unknown as UseQueryResult<AssetGroupTag | undefined>; + } as unknown as UseQueryResult<AssetGroupTagSelector | undefined>;
121-136: Nit: Prefer enum members over magic numbers for type fieldsUsing 1 as AssetGroupTagTypes obscures intent. If an enum member exists (e.g., AssetGroupTagTypes.Tier), use it for clarity and future-proofing.
Example:
- type: 1 as AssetGroupTagTypes, + type: AssetGroupTagTypes.Tier,Also applies to: 154-168
116-138: Reduce boilerplate with a typed test helper for query resultsCasting through unknown as UseQueryResult is noisy. Introduce a small helper in test-utils to create minimal query results with proper typing.
Helper (in test-utils):
export function makeSuccessQueryResult<T>(data: T): Pick<UseQueryResult<T>, 'isLoading'|'isError'|'isSuccess'|'data'> { return { isLoading: false, isError: false, isSuccess: true, data }; }Usage in tests:
-const testSelector = { isLoading: false, isError: false, isSuccess: true, data: { /* ... */ } } as unknown as UseQueryResult<AssetGroupTagSelector | undefined>; +const testSelector = makeSuccessQueryResult<AssetGroupTagSelector>({ /* ... */ });Also applies to: 149-169
70-77: Reuse shared MSW handler for/api/v2/available-domains
This test is manually stubbing the same/api/v2/available-domainsresponse thatzoneHandlersalready provides. To avoid divergence:• Import and spread the shared handlers instead of duplicating the stub.
• Remove the manualrest.get('/api/v2/available-domains', …)block.Example diff in
DynamicDetails.test.tsx:import { rest } from 'msw'; -import { setupServer } from 'msw/node'; +import { setupServer } from 'msw/node'; +import { zoneHandlers } from '../../../mocks/handlers'; describe('DynamicDetails', () => { - const server = setupServer( - rest.get('/api/v2/asset-group-tags/*', async (req, res, ctx) => { … }), - rest.get('/api/v2/available-domains', async (_req, res, ctx) => { - return res(ctx.json({ data: [] })); - }) - ); + const server = setupServer( + rest.get('/api/v2/asset-group-tags/*', async (req, res, ctx) => { … }), + ...zoneHandlers + );This way, any updates to the shared handler for
/api/v2/available-domainswill automatically apply here.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx (2)
168-185: Stabilize renderHook props and reset spies to avoid flaky counts
- Pass explicit initialProps ([]) so the first render uses an empty environments array instead of undefined. This removes ambiguity and makes intent clear.
- Consider clearing/restoring spies between tests to prevent cross-test leakage of call counts if global config doesn’t auto-restore.
Apply this diff within the selected lines:
- const { rerender } = renderHook((environments: string[]) => - agtHook.useTagMembersInfiniteQuery(1, 'asc', environments) - ); + const { rerender } = renderHook( + (environments: string[]) => agtHook.useTagMembersInfiniteQuery(1, 'asc', environments), + { initialProps: [] as string[] } + );Outside the selected range, update afterEach to also clear mocks (only if not already handled globally):
afterEach(() => { vi.clearAllMocks(); server.resetHandlers(); });
204-220: Fix typo in test name and make initial props explicit
- Typo: “envrionment” → “environment”.
- Mirror the initialProps pattern here as well for consistency.
Apply this diff within the selected lines:
- test('selector members refetches on envrionment change', async () => { + test('selector members refetches on environment change', async () => { @@ - const { rerender } = renderHook((environments: string[]) => - agtHook.useSelectorMembersInfiniteQuery(1, 1, 'asc', environments) - ); + const { rerender } = renderHook( + (environments: string[]) => agtHook.useSelectorMembersInfiniteQuery(1, 1, 'asc', environments), + { initialProps: [] as string[] } + );packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
40-55: Memoize the computed environment ID list to stabilize identity across rendersAlthough the queryKey uses array contents, memoizing the computed list avoids unnecessary downstream effects and improves readability. Do not call hooks conditionally—compute the result in a single useMemo.
Apply this diff within the selected lines:
-export const useEnvironmentIdList = ( - ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES: (string | PathPattern)[] -): Environment['id'][] => { - const { data: availableEnvironments } = useAvailableEnvironments(); - const { environment, environmentAggregation } = useSelectedEnvironment(); - const isEnvironmentAggregationSupportedPage = useMatchingPaths(ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES); - - if (isEnvironmentAggregationSupportedPage && environmentAggregation && availableEnvironments) { - const aggregatedEnvironmentIds = getEnvironmentAggregationIds(environmentAggregation, availableEnvironments); - return aggregatedEnvironmentIds; - } - - if (environment?.id) return [environment.id]; - - return []; -}; +export const useEnvironmentIdList = ( + ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES: (string | PathPattern)[] +): Environment['id'][] => { + const { data: availableEnvironments } = useAvailableEnvironments(); + const { environment, environmentAggregation } = useSelectedEnvironment(); + const isEnvironmentAggregationSupportedPage = useMatchingPaths(ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES); + + const ids = useMemo<Environment['id'][]>(() => { + if (isEnvironmentAggregationSupportedPage && environmentAggregation && availableEnvironments) { + return getEnvironmentAggregationIds(environmentAggregation, availableEnvironments); + } + if (environment?.id) return [environment.id]; + return []; + }, [isEnvironmentAggregationSupportedPage, environmentAggregation, availableEnvironments, environment?.id]); + + return ids; +};Outside the selected range, add the missing import:
import { useMemo } from 'react';packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (1)
126-139: Deduplicate sort mapping logicThe mapping sortOrder === 'asc' ? 'name' : '-name' appears in both member fetchers. Centralize it in a tiny helper for consistency and to reduce drift.
Apply these diffs within the selected lines:
- apiClient.getAssetGroupTagMembers(tagId, skip, limit, sortOrder === 'asc' ? 'name' : '-name', environments), + apiClient.getAssetGroupTagMembers(tagId, skip, limit, toNameSort(sortOrder), environments),- sortOrder === 'asc' ? 'name' : '-name', + toNameSort(sortOrder),Outside the selected ranges, add this helper near the top of the module:
const toNameSort = (order: SortOrder) => (order === 'asc' ? 'name' : '-name');Also applies to: 159-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
cmd/ui/src/views/GroupManagement/GroupManagement.test.tsx(1 hunks)cmd/ui/src/views/QA/QA.tsx(3 hunks)cmd/ui/src/views/ZoneManagement/InfoHeader.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/index.ts(0 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/types.ts(0 hunks)packages/javascript/bh-shared-ui/src/hooks/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.test.tsx(2 hunks)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx(5 hunks)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useInitialEnvironment/useInitialEnvironment.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/useMatchingPaths.tsx(1 hunks)packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts(1 hunks)packages/javascript/bh-shared-ui/src/routes/index.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/DataSelector/DataSelector.tsx(0 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/DynamicDetails.test.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx(1 hunks)packages/javascript/js-client-library/src/client.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/javascript/bh-shared-ui/src/components/GroupManagementContent/index.ts
- packages/javascript/bh-shared-ui/src/views/DataQuality/DataSelector/DataSelector.tsx
- packages/javascript/bh-shared-ui/src/components/GroupManagementContent/types.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/javascript/bh-shared-ui/src/hooks/index.ts
- packages/javascript/bh-shared-ui/src/routes/index.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/index.ts
- packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/index.ts
- packages/javascript/bh-shared-ui/src/mocks/handlers/zoneHandlers.ts
- cmd/ui/src/views/GroupManagement/GroupManagement.test.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts
- cmd/ui/src/views/ZoneManagement/InfoHeader.tsx
- packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/Details.tsx
- cmd/ui/src/views/QA/QA.tsx
- packages/javascript/bh-shared-ui/src/hooks/useMatchingPaths.tsx
- packages/javascript/js-client-library/src/client.ts
- packages/javascript/bh-shared-ui/src/hooks/useInitialEnvironment/useInitialEnvironment.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
- packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
useEnvironmentIdList(40-55)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (3)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentParams/useEnvironmentParams.tsx (1)
EnvironmentAggregation(23-23)packages/javascript/bh-shared-ui/src/hooks/useAvailableEnvironments/useAvailableEnvironments.tsx (2)
useAvailableEnvironments(33-41)useSelectedEnvironment(43-72)packages/javascript/bh-shared-ui/src/hooks/useMatchingPaths.tsx (1)
useMatchingPaths(19-30)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (1)
SelectedEnvironment(25-25)
packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (3)
packages/javascript/bh-shared-ui/src/types.ts (1)
SortOrder(27-27)packages/javascript/bh-shared-ui/src/utils/paginatedFetcher.ts (2)
createPaginatedFetcher(37-50)PageParam(17-20)packages/javascript/js-client-library/src/responses.ts (1)
AssetGroupTagMemberListItem(177-179)
⏰ 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 (4)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (2)
24-25: apiClient export verified and import path is correctI’ve confirmed that
apiClientis indeed a named export inpackages/javascript/bh-shared-ui/src/utils/api.ts(line 19), so the import from../../utils/apiwill resolve correctly. No further changes are needed here.
97-100: No action needed:DropdownOption.keyalready matchesAssetGroup.idI verified that in
packages/javascript/bh-shared-ui/src/components/DropdownSelector/types.ts,export type DropdownOption = { key: number; value: string; icon?: IconDefinition; };so
selectedAssetGroup.keyis a number and will strictly equalassetGroup.id(also a numeric ID). You don’t need to coerce or change anything here—this lookup is already type-safe.packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
141-157: LGTM: environments threaded through key and fetch; defaults handledThe environments parameter is correctly incorporated into both the query key and the fetcher call. Default handling in the key builder avoids spreading undefined.
182-207: LGTM: selector members hook guards inputs and propagates environmentsEnabled flag prevents premature requests; key includes selector dimension; environments are forwarded correctly.
| import { apiClient } from '../../../utils'; | ||
|
|
||
| const ObjectCountPanel: FC<{ tagId: string }> = ({ tagId }) => { | ||
| const environments = useEnvironmentIdList(['zone-management']); |
There was a problem hiding this comment.
Route pattern likely never matches; aggregation may be disabled inadvertently
Passing 'zone-management' (no leading slash/wildcard) to useEnvironmentIdList will not match typical React Router paths like '/zone-management/...', causing aggregation to be skipped. Use the route constant or a proper pattern.
Apply one of the following:
Option A (preferred, if available):
+import { ROUTE_ZONE_MANAGEMENT_ROOT } from '../../../routes';
@@
- const environments = useEnvironmentIdList(['zone-management']);
+ const environments = useEnvironmentIdList([ROUTE_ZONE_MANAGEMENT_ROOT]);Option B (fallback if the constant isn’t available):
- const environments = useEnvironmentIdList(['zone-management']);
+ const environments = useEnvironmentIdList(['/zone-management/*']);📝 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 environments = useEnvironmentIdList(['zone-management']); | |
| // at the top of packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx | |
| import React from 'react'; | |
| import { useEnvironmentIdList } from '../../../hooks/useEnvironmentIdList'; | |
| import { ROUTE_ZONE_MANAGEMENT_ROOT } from '../../../routes'; | |
| … | |
| // around line 24, replace the literal string with the route constant | |
| const environments = useEnvironmentIdList([ROUTE_ZONE_MANAGEMENT_ROOT]); |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx
around line 24, the call useEnvironmentIdList(['zone-management']) uses a route
string that likely won't match actual router paths (missing leading
slash/wildcard) so aggregation is skipped; replace the literal with the
canonical route constant (e.g. ROUTES.ZONE_MANAGEMENT or equivalent) if
available, otherwise use a proper path pattern such as '/zone-management/*' or
'/zone-management' to ensure it matches the app's route pattern; update imports
if you use the route constant and run type checks.
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Tested locally looks functional! Mostly nits and possible perf improvements for hooks
...ges/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx
Outdated
Show resolved
Hide resolved
...t/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
Show resolved
Hide resolved
...t/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
Outdated
Show resolved
Hide resolved
...t/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
Outdated
Show resolved
Hide resolved
...t/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.tsx
Show resolved
Hide resolved
…t.tsx Co-authored-by: mistahj67 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (1)
22-39: Aggregator implementation is good; matches prior suggestion.The forEach/push version shared in the earlier review is fine; the filter/map version above is equivalent and a bit tighter. Either approach works.
🧹 Nitpick comments (5)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx (5)
17-18: Use a type-only import for PathPattern.Prevents an unnecessary runtime import and aligns with TS best practices.
-import { PathPattern } from 'react-router-dom'; +import type { PathPattern } from 'react-router-dom';
49-52: Tiny simplification — return directly.Minor readability pass; no behavior change.
- if (isEnvironmentAggregationSupportedPage && environmentAggregation && availableEnvironments) { - const aggregatedEnvironmentIds = getEnvironmentAggregationIds(environmentAggregation, availableEnvironments); - return aggregatedEnvironmentIds; - } + if (isEnvironmentAggregationSupportedPage && environmentAggregation && availableEnvironments) { + return getEnvironmentAggregationIds(environmentAggregation, availableEnvironments); + }
43-44: Signal immutability in the API with a readonly param.Prevents accidental mutation of the routes array by consumers.
-export const useEnvironmentIdList = ( - ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES: (string | PathPattern)[] +export const useEnvironmentIdList = ( + ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES: readonly (string | PathPattern)[]
42-57: Optional: memoize the computed ID list to avoid unnecessary recomputation.If these IDs are used in query keys, react-query hashes keys structurally so new arrays are fine. If they’re passed to memoized components, a stable reference can help avoid needless renders.
+import { useMemo } from 'react'; @@ export const useEnvironmentIdList = ( ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES: readonly (string | PathPattern)[] ): Environment['id'][] => { const { data: availableEnvironments } = useAvailableEnvironments(); const { environment, environmentAggregation } = useSelectedEnvironment(); const isEnvironmentAggregationSupportedPage = useMatchingPaths(ENVIRONMENT_AGGREGATION_SUPPORTED_ROUTES); - if (isEnvironmentAggregationSupportedPage && environmentAggregation && availableEnvironments) { - return getEnvironmentAggregationIds(environmentAggregation, availableEnvironments); - } - - if (environment?.id) return [environment.id]; - - return []; + return useMemo(() => { + if (isEnvironmentAggregationSupportedPage && environmentAggregation && availableEnvironments) { + return getEnvironmentAggregationIds(environmentAggregation, availableEnvironments); + } + if (environment?.id) return [environment.id]; + return []; + }, [isEnvironmentAggregationSupportedPage, environmentAggregation, availableEnvironments, environment?.id]); };
1-58: Add unit tests for edge cases.You mentioned tests were added elsewhere. Please include targeted tests for this hook/function pair:
- getEnvironmentAggregationIds returns only collected IDs.
- Aggregation mode 'all' vs specific type.
- useEnvironmentIdList falls back to [environment.id] when aggregation not supported for the route.
- Empty/undefined availableEnvironments returns [].
I can draft tests using React Testing Library + a simple mock for useAvailableEnvironments/useSelectedEnvironment/useMatchingPaths if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx
[error] 25-39: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
🪛 GitHub Actions: Static Code Analysis
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx
[error] 23-23: ESLint: '@typescript-eslint/no-unused-vars' - 'environmentAggregation' is defined but never used. (no-unused-vars)
[error] 24-24: ESLint: '@typescript-eslint/no-unused-vars' - 'environments' is defined but never used. (no-unused-vars)
🪛 GitHub Actions: Build UI
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx
[error] 26-26: TS1184: Modifiers cannot appear here.
[error] 26-26: TS6133: 'getEnvironmentAggregationIds' is declared but its value is never read.
[error] 54-54: TS2322: Type 'void' is not assignable to type 'string[]'.
🪛 GitHub Actions: Run UI Tests
packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx
[error] 26-26: esbuild transform error: Unexpected 'export' at useEnvironmentIdList.tsx:26. 'export' must be at top-level. Move export statements to module scope.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (2)
124-135: Prevent sending invalid filter: avoideq:undefined/eq:nullwhen no environment is selectedIf both
selectedEnvironmentandglobalEnvironmentare null (or{ type: null, id: null }), the else branch buildsenvironment_id = "eq:undefined"(or"eq:null"), which will produce a bad server query and misleading cache keys. Guard early and clear the filter in that case. Also, the dependency onselectedAssetGroupIdhere is unnecessary because the filter does not depend on it.Apply this diff:
useEffect(() => { - const filterDomain = selectedEnvironment || globalEnvironment; - const filter: AssetGroupMemberParams = {}; - if (filterDomain?.type === 'active-directory-platform') { - filter.environment_kind = 'eq:Domain'; - } else if (filterDomain?.type === 'azure-platform') { - filter.environment_kind = 'eq:AZTenant'; - } else { - filter.environment_id = `eq:${filterDomain?.id}`; - } - setFilterParams(filter); -}, [selectedEnvironment, globalEnvironment, selectedAssetGroupId]); + const env = selectedEnvironment ?? globalEnvironment; + + // No environment selected — clear filters. + if (!env || (env.type === null && env.id == null)) { + setFilterParams({}); + return; + } + + const next: AssetGroupMemberParams = {}; + if (env.type === 'active-directory-platform') { + next.environment_kind = 'eq:Domain'; + } else if (env.type === 'azure-platform') { + next.environment_kind = 'eq:AZTenant'; + } else if (env.id) { + next.environment_id = `eq:${env.id}`; + } + setFilterParams(next); +}, [selectedEnvironment, globalEnvironment]);
79-95: Stabilize react-query key: avoid object identities and use IDs in the key
selectedAssetGroupis an object; using it in the key can cause unnecessary cache misses when its identity changes. Prefer the scalarselectedAssetGroupId.Apply this diff:
- const { data: memberCounts } = useQuery({ - queryKey: [ - 'getAssetGroupMembersCount', - filterParams.environment_id, - filterParams.environment_kind, - selectedAssetGroup, - ], + const { data: memberCounts } = useQuery({ + queryKey: [ + 'getAssetGroupMembersCount', + selectedAssetGroupId, + filterParams.environment_id, + filterParams.environment_kind, + ], enabled: !!selectedAssetGroupId, queryFn: ({ signal }) => apiClient .getAssetGroupMembersCount( - selectedAssetGroupId?.toString() ?? '', // This query will only execute if selectedAssetGroup is truthy. + selectedAssetGroupId?.toString() ?? '', // This query only executes when selectedAssetGroupId is truthy. { environment_id: filterParams.environment_id, environment_kind: filterParams.environment_kind }, { signal } ) .then((res) => res.data.data), });
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
121-122: MovehandleSelectalongside other handlers for consistencyKeep handlers co-located (with
handleAssetGroupSelectorChange,handleFilterChange) to improve scanability.
🧹 Nitpick comments (4)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (4)
62-62: Provide a useful default error messageIf consumers omit
errorMessage, the Alert renders empty on failure. Default to a meaningful fallback.Apply this diff:
-}> = ({ selected, errorMessage = '', buttonPrimary = false, onSelect = () => {} }) => { +}> = ({ selected, errorMessage = 'Failed to load environments.', buttonPrimary = false, onSelect = () => {} }) => {
88-106: Improve accessibility on the trigger buttonAdd ARIA attributes so screen readers understand the control and state.
Apply this diff:
<Button variant={'primary'} + aria-label='Select environment' + aria-haspopup='listbox' + aria-expanded={open} className={cn({ 'bg-transparent rounded-md border uppercase shadow-outer-0 hover:bg-neutral-3 text-black dark:text-white truncate': !buttonPrimary, 'w-full': buttonPrimary, })} data-testid='data-quality_context-selector'>
124-166: Hoist TooltipProvider to avoid per-item providersCreating a
TooltipProviderfor every row is unnecessary. Wrap once around the list and drop the inner providers.Apply this diff:
- <ul> + <TooltipProvider> + <ul> {filteredEnvironments @@ - <Button + <Button variant={'text'} className='flex justify-between items-center gap-2 w-full' onClick={() => { onSelect({ type: environment.type, id: environment.id }); handleClose(); }}> - <TooltipProvider> - <TooltipRoot> - <TooltipTrigger> - <span className='uppercase max-w-96 truncate'> - {environment.name} - </span> - </TooltipTrigger> - <TooltipPortal> - <TooltipContent - side='left' - className='dark:bg-neutral-dark-5 border-0'> - <span className='uppercase'>{environment.name}</span> - </TooltipContent> - </TooltipPortal> - </TooltipRoot> - </TooltipProvider> + <TooltipRoot> + <TooltipTrigger> + <span className='uppercase max-w-96 truncate'> + {environment.name} + </span> + </TooltipTrigger> + <TooltipPortal> + <TooltipContent side='left' className='dark:bg-neutral-dark-5 border-0'> + <span className='uppercase'>{environment.name}</span> + </TooltipContent> + </TooltipPortal> + </TooltipRoot> @@ - </ul> + </ul> + </TooltipProvider>
168-176: Use.some()for existence checks to avoid creating arraysMicro-optimization and a bit clearer intent.
Apply this diff:
- disabled={!data?.filter((env) => env.type === 'active-directory').length} + disabled={!data?.some((env) => env.type === 'active-directory')} @@ - disabled={!data?.filter((env) => env.type === 'azure').length} + disabled={!data?.some((env) => env.type === 'azure')}Also applies to: 181-188
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/ui/src/views/QA/QA.tsx(3 hunks)cmd/ui/src/views/ZoneManagement/InfoHeader.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx(3 hunks)packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/javascript/bh-shared-ui/src/hooks/useEnvironmentIdList.tsx
- cmd/ui/src/views/ZoneManagement/InfoHeader.tsx
- cmd/ui/src/views/QA/QA.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (4)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (2)
SelectedEnvironment(25-25)SelectorValueTypes(23-23)packages/javascript/bh-shared-ui/src/hooks/useAvailableEnvironments/useAvailableEnvironments.tsx (1)
useAvailableEnvironments(33-41)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/types.ts (1)
SelectedEnvironment(25-25)
⏰ 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). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (3)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (2)
158-164: Selector integration looks correctGood swap to
SimpleEnvironmentSelectorwith the newselected/onSelectAPI and sensible fallback{ type: null, id: null }. Passing through theerrorMessageand opting intobuttonPrimaryaligns with the new component’s contract.
127-133: Confirm server-side enum values forenvironment_kind
I ran the grep acrosspackages/javascript/bh-shared-uiand only found:
- Your two assignments in
GroupManagementContent.tsx- A single test in
AssetGroupFilters.test.tsxthat asserts"eq:Domain"- A mock in
mocks/factories/initial.tsusingprimary_kind: 'eq:Domain', notenvironment_kindNone of these prove what the server actually expects. A typo in the filter string here will silently break the UI. Please:
- Verify in your API’s specification (e.g. OpenAPI/Swagger, or backend enum definition) that
environment_kindsupports exactly:
"eq:Domain""eq:AZTenant"- If those differ (e.g. different casing or prefix), update these assignments and corresponding tests/mocks accordingly.
packages/javascript/bh-shared-ui/src/views/DataQuality/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (1)
73-87: LGTM: Popover open state now respectsonOpenChange(nextOpen)Nice fix using a setter-style handler; this avoids the toggle bug on outside clicks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
11-20: Double-check intentional “test run” arg propagation to workspace scriptsThe root script passes a trailing “run” argument to each workspace’s test script:
- If your workspaces define "test": "vitest" this yields "vitest run" (good for CI).
- If any workspace uses a different test runner or doesn’t accept that arg, it will fail.
If you want resilience across mixed test runners, consider adding a separate CI script and keep "test" argument-free:
"scripts": { - "test": "yarn workspaces foreach -ptv run test run", + "test": "yarn workspaces foreach -ptv run test", + "test:ci": "yarn workspaces foreach -ptv run test run", "check-types": "yarn workspaces foreach -ptv run check-types",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (10)
.yarn/cache/call-bind-npm-1.0.8-4145a20621-aa2899bce9.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/call-bound-npm-1.0.4-359cfa32c7-2f6399488d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/for-each-npm-0.3.5-b74c5c5ba7-3c986d7e11.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/is-typed-array-npm-1.1.15-33aa18e28f-ea7cfc46c2.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/sha.js-npm-2.4.11-14868df4ca-ebd3f59d4b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/sha.js-npm-2.4.12-bc0424125d-9ec0fe39cc.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/to-buffer-npm-1.2.1-d977d5fb59-a683dcf19b.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/typed-array-buffer-npm-1.0.3-bddcba0c25-3fb91f0735.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/which-typed-array-npm-1.1.19-e664d1e89c-162d2a07f6.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json(1 hunks)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-tests
🔇 Additional comments (4)
package.json (4)
28-34: Syntax confirmation: your use of “resolutions” keys is correct for Yarn 3 (no globs needed)The plain package names (e.g., "sha.js", "dompurify") are the recommended pattern under Yarn Berry; the older “**/pkg” glob syntax is no longer supported. No change needed here—just noting the correctness. (yarnpkg.com, github.com)
22-22: Ensure Corepack enforces Yarn 3.5.1 in CI/devYou’ve set packageManager to [email protected]. Make sure:
- Corepack is enabled on developer machines and CI.
- The project includes the Yarn release file in .yarn/releases (or that CI uses “corepack prepare”).
This prevents environment drift where “resolutions” behavior and foreach flags can differ across Yarn versions. (yarnpkg.com)
4-9: Workspace “bloodhound-ui” confirmed
Thecmd/ui/package.jsonfile defines"name": "bloodhound-ui", matching the workspace entry and ensuring all dev/debug/start/preview scripts will resolve correctly.
28-34: Manual Verification Required: Dependency ResolutionsI attempted to automate
yarn whyandyarn npm auditchecks but ran into internal errors in Yarn v3.6.1, so please manually confirm the following:
- In package.json (lines 28–34), the resolutions block should be:
- dompurify → “3.2.6”
- tar-fs → “2.1.3”
- sha.js → “2.4.12”
- Inspect your lockfile (
yarn.lockorpackage-lock.json) to ensure those exact versions are enforced and that no lower/vulnerable versions remain.- Run
npm audit --depth=0(oryarn npm audit --recursive) and confirm no advisories surface for dompurify, tar-fs, or sha.js.Suggested diff for package.json:
"resolutions": { - "dompurify": "3.1.3", + "dompurify": "3.2.6", "cross-spawn": "^7.0.5", - "tar-fs": "^2.1.2", + "tar-fs": "2.1.3", - "sha.js": "^2.4.12", + "sha.js": "2.4.12", "form-data": "^4.0.4" }
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Thanks for making the improvements and including the vuln fix 🚀
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
124-135: Avoid sending “eq:undefined” when no environment is selectedIf selectedEnvironment and globalEnvironment are both null (or type requires an id but id is missing), the else branch sets environment_id to eq:undefined. This yields incorrect queries and confusing results.
Apply this diff:
- useEffect(() => { - const filterDomain = selectedEnvironment || globalEnvironment; - const filter: AssetGroupMemberParams = {}; - if (filterDomain?.type === 'active-directory-platform') { - filter.environment_kind = 'eq:Domain'; - } else if (filterDomain?.type === 'azure-platform') { - filter.environment_kind = 'eq:AZTenant'; - } else { - filter.environment_id = `eq:${filterDomain?.id}`; - } - setFilterParams(filter); - }, [selectedEnvironment, globalEnvironment, selectedAssetGroupId]); + useEffect(() => { + const env = selectedEnvironment || globalEnvironment; + if (!env) { + setFilterParams({}); + return; + } + const filter: AssetGroupMemberParams = {}; + if (env.type === 'active-directory-platform') { + filter.environment_kind = 'eq:Domain'; + } else if (env.type === 'azure-platform') { + filter.environment_kind = 'eq:AZTenant'; + } else if (env.id) { + filter.environment_id = `eq:${env.id}`; + } + setFilterParams(filter); + }, [selectedEnvironment, globalEnvironment, selectedAssetGroupId]);
♻️ Duplicate comments (1)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
121-122: Hoist handleSelect alongside other handlers for readabilityMatches prior suggestion; keeps handlers grouped and easier to find.
🧹 Nitpick comments (21)
packages/javascript/bh-shared-ui/src/views/DataQuality/TenantInfo.tsx (2)
87-89: Avoid silent null UI on empty data; notify parent via onDataError.Current code returns
nullwhen data is empty without invokingonDataError. Consider signaling the parent to handle empty state the same way as errors.Apply:
- useEffect(() => { - if (isError) onDataError(); - }, [isError, onDataError]); + useEffect(() => { + if (isError || (tenantData && !tenantData.data.length)) onDataError(); + }, [isError, tenantData, onDataError]);Optionally, render a lightweight empty state instead of returning
null.
103-109: Consistent empty-state handling for platform stats.Mirror the same enhancement for the platform stats path to avoid a blank UI on empty arrays.
Apply:
- useEffect(() => { - if (isError) onDataError(); - }, [isError, onDataError]); + useEffect(() => { + if (isError || (platformData && !platformData.data.length)) onDataError(); + }, [isError, platformData, onDataError]);packages/javascript/bh-shared-ui/src/views/DataQuality/DomainInfo.tsx (2)
82-84: Propagate empty-data condition to onDataError to avoid silent null UI.Like TenantInfo, notify the parent if the dataset is empty.
Apply:
- useEffect(() => { - if (isError) onDataError(); - }, [isError, onDataError]); + useEffect(() => { + if (isError || (domainData && !domainData.data.length)) onDataError(); + }, [isError, domainData, onDataError]);
98-104: Consistent handling for platform stats empty data.Apply the same notification pattern on the AD platform stats component.
Apply:
- useEffect(() => { - if (isError) onDataError(); - }, [isError, onDataError]); + useEffect(() => { + if (isError || (adPlatformData && !adPlatformData.data.length)) onDataError(); + }, [isError, adPlatformData, onDataError]);packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (8)
68-75: Use Array.some for intent clarity and to avoid intermediate arrays.
someexpresses the check directly and saves an allocation.Apply:
- const disableADPlatform = useMemo(() => { - return !data?.filter((env) => env.type === 'active-directory').length; - }, [data]); + const disableADPlatform = useMemo(() => !data?.some((env) => env.type === 'active-directory'), [data]); - const disableAZPlatform = useMemo(() => { - return !data?.filter((env) => env.type === 'azure').length; - }, [data]); + const disableAZPlatform = useMemo(() => !data?.some((env) => env.type === 'azure'), [data]);
86-93: Tighten the click handler type to Environment.You pass an
EnvironmentintohandleEnvironmentClick, but the parameter is typed asSelectedEnvironment. Typing it asEnvironmentmakes intent explicit and removes the non-null assertion.Apply:
- const handleEnvironmentClick = useCallback( - (environment: SelectedEnvironment) => { - onSelect({ type: environment.type!, id: environment.id }); + const handleEnvironmentClick = useCallback( + (environment: Environment) => { + onSelect({ type: environment.type, id: environment.id }); handleClose(); }, [onSelect] );
98-108: Minor: normalize search once and reuse.Precompute a normalized query string to avoid repeated
.toLowerCase()calls and trim accidental whitespace.Apply:
- const handleChange: React.ChangeEventHandler<HTMLInputElement | HTMLTextAreaElement> = (e) => - setSearchInput(e.target.value); + const handleChange: React.ChangeEventHandler<HTMLInputElement | HTMLTextAreaElement> = (e) => + setSearchInput(e.target.value); - const filteredEnvironments = data?.filter( - (environment: Environment) => - environment.name.toLowerCase().includes(searchInput.toLowerCase()) && environment.collected - ); + const query = searchInput.trim().toLowerCase(); + const filteredEnvironments = data?.filter( + (environment: Environment) => environment.collected && environment.name.toLowerCase().includes(query) + );
94-94: Skeleton width should match button mode.When
buttonPrimaryis true, the trigger button spans the full width; mirror that in the loading state for less layout shift.Apply:
- if (isLoading) return <Skeleton className='rounded-md w-10' />; + if (isLoading) return <Skeleton className={cn('rounded-md', { 'w-full': buttonPrimary, 'w-10': !buttonPrimary })} />;
150-156: Add empty-state row when no environments match.Currently the list renders nothing when filtered to zero results and both platform options may be disabled, which is confusing.
Apply:
- <ul> - {filteredEnvironments + <ul role="listbox" aria-label="Environments"> + {(!filteredEnvironments || filteredEnvironments.length === 0) && ( + <li className="py-2 text-neutral-7" aria-disabled={true}>No collected environments found</li> + )} + {filteredEnvironments ?.sort((a: Environment, b: Environment) => { return a.name.localeCompare(b.name); }) .map((environment: Environment, index: number) => { return ( - <li + <li role="option" aria-selected={false} key={environment.id}
32-32: Unify class merging utility to a single helper.You import both
clsxandcn(which already wrapsclsx+twMerge). Prefercnconsistently.Apply:
-import clsx from 'clsx'; ... - className={clsx( + className={cn( index === filteredEnvironments.length - 1 && 'border-b border-neutral-light-5 pb-2 mb-2' )}>Remove the now-unused
clsximport.Also applies to: 159-162
115-121: Optional: align Button variant with visual intent.You override a lot of “primary” styling via classes when
buttonPrimaryis false. Consider switching the variant instead of fighting defaults.Apply:
- <Button - variant={'primary'} + <Button + variant={buttonPrimary ? 'primary' : 'text'} className={cn({ - 'bg-transparent rounded-md border uppercase shadow-outer-0 hover:bg-neutral-3 text-black dark:text-white truncate': - !buttonPrimary, + 'uppercase truncate': true, + 'rounded-md border shadow-outer-0 text-black dark:text-white': !buttonPrimary, 'w-full': buttonPrimary, })}If
textis not appropriate, consider asecondary/ghostvariant offered by@bloodhoundenterprise/doodleui.
193-212: Accessibility/naming polish for platform options.
- Add
aria-disabledthat mirrors thedisabledprop.- Consider consistent role usage with the list items above.
Apply:
- <li key='active-directory-platform'> + <li key='active-directory-platform' role="option" aria-selected={false}> <Button className='flex justify-between items-center gap-2 w-full' onClick={handleADPlatformClick} - disabled={disableADPlatform} + disabled={disableADPlatform} + aria-disabled={disableADPlatform} variant={'text'}> ... - <li key='azure-platform'> + <li key='azure-platform' role="option" aria-selected={false}> <Button onClick={handleAzurePlatformClick} variant={'text'} - disabled={disableAZPlatform} + disabled={disableAZPlatform} + aria-disabled={disableAZPlatform} className='flex justify-between items-center gap-2 w-full'>packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/types.ts (1)
19-25: Add TSDoc to clarify semantics (domain vs. platform aggregates)A brief TSDoc on AD_PLATFORM/AZ_PLATFORM and SelectedEnvironment would reduce ambiguity for consumers (especially around when id must be null vs. required).
Example:
+/** Aggregate across all Active Directory environments (used for "All Active Directory Domains"). */ export const AD_PLATFORM = 'active-directory-platform' as const; +/** Aggregate across all Azure environments (used for "All Azure Tenants"). */ export const AZ_PLATFORM = 'azure-platform' as const; +/** Platform-level aggregate selection discriminant. */ export type EnvironmentPlatform = typeof AD_PLATFORM | typeof AZ_PLATFORM; +/** Values allowed by the environment selector (environment type or platform aggregate). */ export type SelectorValueTypes = Environment['type'] | EnvironmentPlatform; - - export type SelectedEnvironment = { type: SelectorValueTypes | null; id: string | null }; +/** See discriminated union below for valid (type,id) combinations. */ +export type SelectedEnvironment = /* see suggested union in previous comment */;cmd/ui/src/views/Administration/Administration.tsx (1)
43-43: Rename lazy import from QA → DataQuality for clarityThe import now points to DataQuality; renaming the variable improves readability and avoids confusion with the removed/renamed QA module.
Apply this diff:
-const QA = React.lazy(() => import('src/views/DataQuality')); +const DataQuality = React.lazy(() => import('src/views/DataQuality')); @@ - component: QA, + component: DataQuality,Also applies to: 63-66
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx (4)
287-287: Use findAllByRole to avoid timing flakiness when the popover rendersgetAllByRole can race with async rendering of the popover content. Prefer findAllByRole for robustness.
Apply this diff:
- expect(within(container).getAllByRole('button')).toHaveLength(32); + expect(await within(container).findAllByRole('button')).toHaveLength(32);
294-308: Add symmetric assertion for “All Azure Tenants” selectionYou already verify the AD aggregate. Adding the Azure aggregate improves confidence in the platform literals and onSelect payloads.
Apply this diff:
await user.click(contextSelector); await user.click(screen.getByText('All Active Directory Domains')); expect(testOnChange).toHaveBeenLastCalledWith({ type: 'active-directory-platform', id: null }); + + // Verify Azure aggregate selection mirrors behavior + await user.click(contextSelector); + await user.click(screen.getByText('All Azure Tenants')); + expect(testOnChange).toHaveBeenLastCalledWith({ type: 'azure-platform', id: null });
344-345: Make the “filtered” case assertions more explicitComplement the button-count assertion by proving the uncollected domain is not rendered. This tightens the behavior guarantee.
Apply this diff:
- const container = await screen.findByTestId('data-quality_context-selector-popover'); - //Only one of the served domains is collected so only one list item plus the two options for all domains and all tenants are displayed. - expect(within(container).getAllByRole('button')).toHaveLength(3); + const container = await screen.findByTestId('data-quality_context-selector-popover'); + // Only one domain is collected, so we expect one item + two aggregate options. + expect(await within(container).findAllByRole('button')).toHaveLength(3); + // Ensure the uncollected domain is not present. + expect(screen.queryByText('ryleigh.com')).not.toBeInTheDocument();Also applies to: 352-352
356-384: Spy and restore console.error to prevent test spilloverDirectly assigning console.error can leak into other tests. Use vi.spyOn and restore after each test in this describe.
Apply this diff:
describe('Context Selector Error', () => { - beforeEach(() => { - console.error = vi.fn(); + beforeEach(() => { + vi.spyOn(console, 'error').mockImplementation(() => {}); server.use( @@ ); }); + afterEach(() => { + vi.restoreAllMocks(); + });cmd/ui/src/views/DataQuality/DataQuality.tsx (2)
127-135: Selector’s “selected” value should mirror the effective environmentWhen selectedEnvironment is null but initialEnvironment exists, the stats render for initialEnvironment, yet the selector appears empty. Bind the selector to environment for consistency.
Apply this diff:
- <SimpleEnvironmentSelector - align='end' - selected={{ - type: selectedEnvironment?.type ?? null, - id: selectedEnvironment?.id ?? null, - }} - errorMessage={environmentErrorMessage} - onSelect={handleSelect} - /> + <SimpleEnvironmentSelector + align='end' + selected={{ + type: environment?.type ?? null, + id: environment?.id ?? null, + }} + errorMessage={environmentErrorMessage} + onSelect={handleSelect} + />
66-69: Don’t overwrite a user’s selection when initialEnvironment updatesThe effect unconditionally sets selectedEnvironment whenever initialEnvironment changes, potentially clobbering a user’s manual selection on refresh. Guard it.
Apply this diff:
- useEffect(() => { - initialEnvironment && setSelectedEnvironment(initialEnvironment); - }, [initialEnvironment]); + useEffect(() => { + if (!selectedEnvironment && initialEnvironment) { + setSelectedEnvironment(initialEnvironment); + } + }, [initialEnvironment, selectedEnvironment]);packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
79-85: Prefer stable primitives in react-query keysUsing selectedAssetGroup (an object) in the key can cause unnecessary cache misses due to identity changes. Use the id instead.
Apply this diff:
- const { data: memberCounts } = useQuery({ - queryKey: [ - 'getAssetGroupMembersCount', - filterParams.environment_id, - filterParams.environment_kind, - selectedAssetGroup, - ], + const { data: memberCounts } = useQuery({ + queryKey: [ + 'getAssetGroupMembersCount', + selectedAssetGroupId, + filterParams.environment_id, + filterParams.environment_kind, + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
cmd/ui/src/views/Administration/Administration.tsx(1 hunks)cmd/ui/src/views/DataQuality/DataQuality.tsx(3 hunks)cmd/ui/src/views/DataQuality/index.tsx(1 hunks)cmd/ui/src/views/GroupManagement/GroupManagement.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx(3 hunks)packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx(5 hunks)packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/types.ts(1 hunks)packages/javascript/bh-shared-ui/src/components/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/DomainInfo.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/TenantInfo.tsx(4 hunks)packages/javascript/bh-shared-ui/src/views/DataQuality/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/views/DataQuality/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx (4)
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/types.ts (2)
SelectedEnvironment(25-25)SelectorValueTypes(23-23)packages/javascript/bh-shared-ui/src/hooks/useAvailableEnvironments/useAvailableEnvironments.tsx (1)
useAvailableEnvironments(33-41)packages/javascript/bh-shared-ui/src/utils/theme.ts (1)
cn(41-43)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/views/DataQuality/DomainInfo.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useDataQualityStats.tsx (2)
useActiveDirectoryDataQualityStatsQuery(34-41)useActiveDirectoryPlatformsDataQualityStatsQuery(78-85)
packages/javascript/bh-shared-ui/src/views/DataQuality/TenantInfo.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useDataQualityStats.tsx (2)
useAzureDataQualityStatsQuery(56-63)useAzurePlatformsDataQualityStatsQuery(105-112)
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (1)
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/types.ts (1)
SelectedEnvironment(25-25)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (15)
cmd/ui/src/views/GroupManagement/GroupManagement.tsx (2)
39-39: Import path update looks correct and matches the QA → DataQuality move.No functional changes; aligns with the refactor.
39-39: Exports and imports verified – no changes needed.The symbol
dataCollectionMessageis exported incmd/ui/src/views/DataQuality/utils.tsx(line 19), and there are no lingering imports from../QA/utilsanywhere undercmd/ui/src/views. Everything is correct and ready to merge.packages/javascript/bh-shared-ui/src/components/index.ts (1)
185-186: Clarify named vs. default re-export for SimpleEnvironmentSelectorIt looks like the named export
SimpleEnvironmentSelectoris already available via the existingexport * from './SimpleEnvironmentSelector';because
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/index.tsitself doesexport { default as SimpleEnvironmentSelector } from './SimpleEnvironmentSelector';So consumers can already do
import { SimpleEnvironmentSelector } from 'bh-shared-ui';If you also want to support a default import (
import SimpleEnvironmentSelector from 'bh-shared-ui'), you’ll need to add a true default re-export at the root barrel, e.g.:export * from './SimpleEnvironmentSelector'; +export { default } from './SimpleEnvironmentSelector';The originally suggested line
+export { default as SimpleEnvironmentSelector } from './SimpleEnvironmentSelector';only creates an identical named export (redundant in this case) and does not enable default-import syntax.
—Likely an incorrect or invalid review comment.
packages/javascript/bh-shared-ui/src/views/DataQuality/TenantInfo.tsx (2)
22-22: Import cleanup looks good.Switching away from local state is appropriate; keeping
useEffectfor error side-effects is fine.
77-81: Good shift to using query results directly.Directly consuming
{ data, isLoading, isError }avoids unnecessary state mirroring and re-renders.packages/javascript/bh-shared-ui/src/views/DataQuality/DomainInfo.tsx (1)
72-76: Good refactor to rely on query state.Eliminating mirrored local state improves correctness and reduces render churn.
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/types.ts (1)
19-24: LGTM on platform literals and unionsThe AD/AZ platform literals and the EnvironmentPlatform/SelectorValueTypes unions align with the selector behavior and tests. Using const string literals here is the right call to keep the public API narrow and stable.
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/index.ts (1)
17-18: Public export swap looks goodExporting SimpleEnvironmentSelector here (and re-exporting types) cleanly updates the package surface for downstream consumers.
cmd/ui/src/views/DataQuality/index.tsx (1)
17-20: Default export updated correctlyRe-pointing the default export to DataQuality matches the new import target from Administration and keeps lazy loading intact.
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.test.tsx (2)
20-22: Tests target the new public API correctlySwitch to importing { SimpleEnvironmentSelector } from '.' and updated test utilities is correct and mirrors the package exports.
263-270: Good coverage of new selected/onSelect propsThe test drives the new selected/onSelect contract through the UI and asserts key affordances (search input, “All …” entries).
cmd/ui/src/views/DataQuality/DataQuality.tsx (2)
32-49: Good abstraction for environment-scoped stats renderingCentralizing environment-type routing in getStatsComponent simplifies the main render path and reduces conditional noise. Props passed look correct.
95-119: Solid UX gating before stats renderEarly-returning to prompt for a Domain/Tenant when required IDs are missing prevents null-prop errors and clarifies the user flow.
packages/javascript/bh-shared-ui/src/components/GroupManagementContent/GroupManagementContent.tsx (2)
158-164: LGTM: SimpleEnvironmentSelector integrationProp mapping (selected/onSelect/buttonPrimary/errorMessage) looks correct; defaulting to { type: null, id: null } is safe.
24-24: LGTM: apiClient import path updateThe move to '../../utils/api' aligns with the new utils structure.
| <Link | ||
| target='_blank' | ||
| href={'https://bloodhound.specterops.io/collect-data/overview#bloodhound-ce-collection'}> | ||
| Data Collection |
There was a problem hiding this comment.
Fix reverse‑tabnabbing on external link
External links opened with target="_blank" should include rel="noopener noreferrer" to prevent the new page from gaining access to window.opener.
Apply this diff:
- <Link
- target='_blank'
- href={'https://bloodhound.specterops.io/collect-data/overview#bloodhound-ce-collection'}>
+ <Link
+ target='_blank'
+ rel='noopener noreferrer'
+ href={'https://bloodhound.specterops.io/collect-data/overview#bloodhound-ce-collection'}>📝 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.
| <Link | |
| target='_blank' | |
| href={'https://bloodhound.specterops.io/collect-data/overview#bloodhound-ce-collection'}> | |
| Data Collection | |
| <Link | |
| target='_blank' | |
| rel='noopener noreferrer' | |
| href={'https://bloodhound.specterops.io/collect-data/overview#bloodhound-ce-collection'}> | |
| Data Collection |
🤖 Prompt for AI Agents
In cmd/ui/src/views/DataQuality/DataQuality.tsx around lines 142 to 145, the
external Link uses target="_blank" without rel attributes which allows
reverse-tabnabbing; update the Link component to include rel="noopener
noreferrer" (i.e., add rel prop with value "noopener noreferrer") so the opened
page cannot access window.opener and the risk is mitigated.
| export type EnvironmentPlatform = typeof AD_PLATFORM | typeof AZ_PLATFORM; | ||
| export type SelectorValueTypes = Environment['type'] | EnvironmentPlatform; | ||
|
|
||
| export type SelectedEnvironment = { type: SelectorValueTypes | null; id: string | null }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten SelectedEnvironment to a discriminated union (prevents invalid states)
As written, { type: 'active-directory', id: null } is allowed even though domain selections should always carry a non-null id. Recommend a discriminated union that encodes the invariant: platform selections have id: null, specific environment selections have a concrete id, and an optional explicit “nothing selected”.
Apply this diff:
-export type SelectedEnvironment = { type: SelectorValueTypes | null; id: string | null };
+// Selected environment can be either:
+// - a specific environment with concrete id, or
+// - a platform-wide aggregate (id is null), or
+// - nothing selected (both null).
+export type SelectedEnvironment =
+ | { type: Environment['type']; id: string }
+ | { type: EnvironmentPlatform; id: null }
+ | { type: null; id: null };📝 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.
| export type SelectedEnvironment = { type: SelectorValueTypes | null; id: string | null }; | |
| // Selected environment can be either: | |
| // - a specific environment with concrete id, or | |
| // - a platform-wide aggregate (id is null), or | |
| // - nothing selected (both null). | |
| export type SelectedEnvironment = | |
| | { type: Environment['type']; id: string } | |
| | { type: EnvironmentPlatform; id: null } | |
| | { type: null; id: null }; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/types.ts
around line 25, the SelectedEnvironment type currently allows invalid states
(e.g. a platform selection with a null id). Replace the single loose object type
with a discriminated union that encodes three explicit cases: 1) "nothing
selected" as { type: null; id: null }, 2) platform-level selections (the
SelectorValueTypes members that represent platform/aggregate choices) with id
set to null, and 3) concrete environment selections (the SelectorValueTypes
members that represent specific environments) with id: string. Update any call
sites/tests to match the new union shape.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/utils/finishedJobs.test.ts (1)
178-186: Standardize console.error spy usage across all testsDirectly assigning
console.error = vi.fn()in individual tests leads to global side effects and can mask errors in other tests. Instead:
Replace each occurrence of
console.error = vi.fn();with
const consoleErrorSpy = vi .spyOn(console, 'error') .mockImplementation(() => {});…then, after your assertions, restore the spy:
consoleErrorSpy.mockRestore();Ensure spies are cleaned up after each test. You can either:
• Add to every test file’s bottom:afterEach(() => { server.resetHandlers(); vi.restoreAllMocks(); });• Or, better, put that
afterEachinto your global test setup (e.g.setupTests.ts) so it applies everywhere.To locate all offending tests, run:
rg -nP 'console\.error\s*=\s*vi\.fn' -g '!**/node_modules/**'Please update each test accordingly to prevent cross-test bleed and ensure clean spy restoration.
🧹 Nitpick comments (3)
packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.test.tsx (3)
32-38: Good addition: stub/api/v2/available-domains; consider aligning payload schema and centralizing handlerThis prevents incidental network calls from failing during render. Two small improvements to future‑proof it:
- Return a minimally valid domain item (if the real API returns objects) to catch shape regressions early, or reuse a shared fixture used elsewhere to avoid drift.
- If multiple suites need this endpoint, consider moving this handler into a shared MSW handler set (e.g., a zone handlers module) to reduce duplication.
48-54: Avoid mutatingconsole.error; use a spy and restore to prevent cross-test leakageDirectly assigning
console.error = vi.fn()can leak into subsequent tests if an assertion throws before teardown. Prefer a spy you restore.Apply this diff within the test body:
- console.error = vi.fn(); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); server.use( rest.get('/api/v2/asset-group-tags/1/members/counts', async (_, res, ctx) => { return res(ctx.status(403)); }) ); render(<ObjectCountPanel tagId='1' />); expect(await screen.findByText('There was an error fetching this data')).toBeInTheDocument(); + errorSpy.mockRestore();
61-72: Add assertion forenvironmentsquery parameter in ObjectCountPanel testThe test currently mocks the counts endpoint but doesn’t verify that the environment IDs are actually sent as query parameters. Since
getAssetGroupTagMembersCountuses theenvironmentskey (seejs-client-library/src/client.ts:271), capturing and assertingreq.url.searchParamsforenvironmentswill prevent regressions if the hook or API client changes.• File to update:
•packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.test.tsx(around lines 61–72)Suggested diff:
- server.use( - rest.get('/api/v2/asset-group-tags/1/members/counts', async (_, res, ctx) => { - return res( - ctx.json({ - data: { - total_count: 100, - counts: { 'Object A': 50, 'Object B': 30, 'Object C': 20 }, - }, - }) - ); - }) - ); + let capturedParams: URLSearchParams | undefined; + server.use( + rest.get('/api/v2/asset-group-tags/1/members/counts', async (req, res, ctx) => { + capturedParams = req.url.searchParams; + return res( + ctx.json({ + data: { + total_count: 100, + counts: { 'Object A': 50, 'Object B': 30, 'Object C': 20 }, + }, + }) + ); + }) + ); render(<ObjectCountPanel tagId='1' />); expect(screen.getByText('Total Count')).toBeInTheDocument(); expect(await screen.findByText('100')).toBeInTheDocument(); + // Assert that the `environments` query parameter is included + expect(capturedParams?.has('environments')).toBe(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/finishedJobs.test.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/ZoneManagement/Details/ObjectCountPanel.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsx
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-tests
- GitHub Check: run-analysis
Description
Enables filtering members per environment in pz details page.
Motivation and Context
Resolves BED-6150
Why is this change required? What problem does it solve?
How Has This Been Tested?
Unit tests updated. Unit tests added to useAssetGroupTags for testing rerender on environments change.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Enhancements
Tests
Chores