feat(agt): zone glyph selector #1836
Conversation
urangel
added
the
user interface
WalkthroughAdds glyph selection/display to Privilege Zones: introduces a GlyphSelectDialog with virtualized icon grid and search; integrates glyph into TagForm state, payloads, and diffing; displays glyphs in DynamicDetails. Adds a free icon list utility and re-export. Adjusts dialog styling classes and minor DOM changes. Updates tests and request types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant TF as TagForm
participant GSD as GlyphSelectDialog
participant VIL as VirtualizedIconList
participant FI as freeIconsList
participant API as API Client
U->>TF: Open Edit/Create Tag
TF->>GSD: Open dialog (selected = current glyph)
GSD->>FI: Load icon names
U->>GSD: Type search query
GSD->>VIL: Render filtered list
U->>VIL: Click icon
VIL-->>GSD: onClick(iconName)
GSD-->>TF: onSelect(iconName)
TF->>API: Create/Update Tag ({ glyph, ... })
API-->>TF: Success/Failure
TF-->>U: Close dialog, update preview / show error
sequenceDiagram
autonumber
participant DD as DynamicDetails
participant Tag as AssetGroupTag
DD->>Tag: Receive tagData
alt tagData.glyph present
DD-->>DD: Render FontAwesomeIcon(glyph)
else
DD-->>DD: Render without icon
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
urangel
force-pushed
the
BED-6151
branch
2 times, most recently
from
e0bd041 to
dd8b297
Compare
urangel
force-pushed
the
BED-6151
branch
5 times, most recently
from
7c5cd42 to
916ff0b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx (2)
74-86: Accessibility: input is inside DialogDescription; add name/associations and focus.Good use of
asChildand consistent nested Dialog structure per previous learnings. To improve SR UX, give the input an accessible name and tie it to the helper text.- <DialogDescription asChild className='text-sm'> - <div> + <DialogDescription asChild className='text-sm'> + <div id='confirmation-challenge-help'> Please input "{challengeTxt}" prior to clicking confirm. - <Input - placeholder={challengeTxt} + <Input + aria-label='Confirmation challenge' + aria-describedby='confirmation-challenge-help' + autoFocus + placeholder={challengeTxt} className='border-t-0 border-l-0 border-r-0 rounded-none border-black dark:border-white bg-transparent dark:bg-transparent placeholder-neutral-dark-10 dark:placeholder-neutral-light-10 focus-visible:ring-0 focus-visible:ring-offset-0 pl-2' onChange={(e) => setChallengeTxtReply(e.target.value)} value={challengeTxtReply} data-testid='confirmation-dialog_challenge-text' /> </div> </DialogDescription>
88-88: Error message is not announced; add alert semantics.Ensure screen readers announce errors immediately.
- {error && <p className='content-center text-error text-xs mt-[3px]'>{error}</p>} + {error && ( + <p role='alert' aria-live='polite' className='content-center text-error text-xs mt-[3px]'> + {error} + </p> + )}Optionally also set
aria-invalid={Boolean(error)}on the Input and include the error element’s id inaria-describedby.packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx (1)
36-36: Prefer semantic over for the warning.Improves accessibility and intent with no visual change.
- <span className='font-bold text-error'>Warning: This change is irreversible.</span> + <strong className='text-error'>Warning: This change is irreversible.</strong>packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts (1)
3-1959: Large static list: stabilize and guard usage
- Mark as readonly to prevent mutation.
- Consider de‑duping aliases to avoid duplicate entries in the picker.
- Keep this module import‑scoped (avoid wildcard barrel imports) to minimize accidental bundle bloat.
Apply:
-export const freeIconsList: IconName[] = [ +export const freeIconsList: readonly IconName[] = [packages/javascript/bh-shared-ui/src/utils/index.ts (1)
27-27: Avoid barrel‑exporting heavy dataRe‑exporting freeIconsList from the top‑level utils increases the chance it’s bundled where not needed (especially with namespace imports). Prefer importing it directly from './freeIconsList' at call sites.
If you proceed, also update consumers:
- import { freeIconsList } from '../../../../utils'; + import { freeIconsList } from '../../../../utils/freeIconsList';packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx (1)
195-198: Strengthen glyph behavior testsAdd a flow that opens the dialog, selects an icon, confirms, then clears it and confirms, asserting form value changes and payload diffs.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (3)
41-56: Don’t render clickable placeholders for empty cells; add accessible labelsEmpty grid slots are still buttons and can emit undefined selections. Render a non‑interactive placeholder and label icon buttons for a11y.
-const IconCard: FC<{ iconName: IconName; onClick: (iconName: IconName) => void }> = ({ iconName, onClick }) => { - return ( - <Button - variant={'text'} - className={clsx(['relative', !iconName && 'invisible'])} - onClick={() => { - onClick(iconName); - }}> +const IconCard: FC<{ iconName?: IconName; onClick: (iconName: IconName) => void }> = ({ iconName, onClick }) => { + if (!iconName) return <div className='h-24 w-24' aria-hidden />; + return ( + <Button + aria-label={`Select "${iconName}" icon`} + variant='text' + className='relative' + onClick={() => onClick(iconName)}> <Card className='flex items-center justify-center h-24 w-24'> <CardContent className='first:pt-0'> - {iconName && <FontAwesomeIcon icon={iconName} size='2xl' />} + <FontAwesomeIcon icon={iconName} size='2xl' /> </CardContent> </Card> <p className='absolute -bottom-16'>{iconName}</p> </Button> ); };Also update Row:
-<IconCard key={row * 5 + index} iconName={filteredList[row * 5 + index]} onClick={onClick} /> +<IconCard key={row * 5 + index} iconName={filteredList[row * 5 + index]} onClick={onClick} />
142-159: Case‑insensitive search and memoize filteringNormalize and trim the query; memoize the filtered list to avoid unnecessary recomputes while typing.
-import { AppIcon } from '../../../../components'; +import { AppIcon } from '../../../../components'; +import { useMemo } from 'react'; @@ - const [query, setQuery] = useState(''); + const [query, setQuery] = useState(''); + const filteredList = useMemo( + () => freeIconsList.filter((n) => n.toLowerCase().includes(query.trim().toLowerCase())), + [query] + ); @@ - <Input + <Input placeholder='Search' onChange={(e) => setQuery(e.target.value)} className='pl-8' /> @@ - <VirtualizedIconList - filteredList={freeIconsList.filter((iconName) => iconName.includes(query))} + <VirtualizedIconList + filteredList={filteredList} onClick={(iconName) => { setSelectedIcon(iconName); }} />
115-169: Rename data‑testids to avoid collisions with DeleteConfirmationDialogUsing 'confirmation-dialog*' here collides with the delete dialog test ids and can create flaky tests when both are present. Use glyph‑specific ids.
- <Dialog open={open} data-testid='confirmation-dialog'> + <Dialog open={open} data-testid='glyph-dialog'> @@ - <Button variant='tertiary' onClick={handleCancel} data-testid='confirmation-dialog_button-no'> + <Button variant='tertiary' onClick={handleCancel} data-testid='glyph-dialog_button-cancel'> Cancel </Button> - <Button onClick={handleConfirm} data-testid='confirmation-dialog_button-yes'> + <Button onClick={handleConfirm} data-testid='glyph-dialog_button-confirm'> Confirm </Button>packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (2)
107-115: Use strict equality for glyph diffAvoid loose inequality on strings.
- if (data.glyph != workingCopy.glyph) diffed.glyph = workingCopy.glyph; + if (data.glyph !== workingCopy.glyph) diffed.glyph = workingCopy.glyph;
136-144: Guard create payload to omit undefined/empty glyph cleanlyCurrent check adds glyph when it’s undefined (relies on JSON.stringify dropping it). Make intent explicit.
- if (formData.glyph !== '') requestValues.glyph = formData.glyph; + if (formData.glyph && formData.glyph !== '') { + requestValues.glyph = formData.glyph; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx(7 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx(13 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts(1 hunks)packages/javascript/js-client-library/src/requests.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsxpackages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
PR: SpecterOps/BloodHound#1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
PR: SpecterOps/BloodHound#1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/js-client-library/src/requests.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
🧬 Code graph analysis (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (4)
packages/javascript/js-client-library/src/requests.ts (2)
CreateAssetGroupTagRequest(37-44)UpdateAssetGroupTagRequest(46-48)packages/javascript/bh-shared-ui/src/providers/NotificationProvider/actions.ts (1)
addNotification(31-44)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
handleError(20-42)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (2)
packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts (1)
freeIconsList(3-1958)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
TagForm(67-592)
⏰ 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 (6)
packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx (1)
36-36: Verify 'text-error' token exists and remove legacy 'text-red' usagesScript output: no matches for "text-red" or "text-error"; tailwind config search returned "unrecognized file type: tsx" — repo-level verification inconclusive.
- Confirm tailwind/daisyUI theme defines an "error" color token in tailwind.config.js/ts so "text-error" resolves.
- Re-run a full repo search (all filetypes) and inspect tailwind config with these commands:
rg -n -S '\btext-red' || true
rg -n -S '\btext-error\b' || true
rg -n 'error|daisyui' tailwind.config.* || true- If "text-error" is not defined, add it to the theme or revert to an explicit utility token.
Location: packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx (uses "text-error" at ~line 36).
packages/javascript/js-client-library/src/requests.ts (1)
37-44: Add glyph field: looks good; verify API parityOptional field keeps this non‑breaking. Please confirm the server/OpenAPI accepts glyph on create/update for zones (and rejects/ignores it for labels if that’s the contract).
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts (1)
62-69: Expose tagId from hook: OK; check consumer typesChange is fine. Ensure all consumers (e.g., useAssetGroupTagInfo/usePatchAssetGroupTag) accept a string tagId as provided by usePZPathParams.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx (1)
329-370: Config‑driven analysis toggle: nice coverageThe two tests accurately gate the toggle by config and edit path. No action needed.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (2)
450-507: Glyph field UX: solidHidden form field + visible summary + modal selector is clear and testable. Nice use of Tooltip and icon preview.
575-581: Dialog integration: OK; consider unique testidsIntegration is correct. If you adopt the glyph dialog testid rename, reflect it here as well. No behavior change needed.
...ges/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx
Show resolved
Hide resolved
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Code LGTM, tested locally and selection works. I sent some suggestions for other areas that could use this info, such as the details panel for zones and perhaps next to the zone display name
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
Outdated
Show resolved
Hide resolved
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
Outdated
Show resolved
Hide resolved
| <ul style={{ ...style, overflowX: 'hidden', marginTop: 0, overflowY: 'auto' }} {...rest}></ul> | ||
| ); | ||
|
|
||
| const IconCard: FC<{ iconName: IconName; onClick: (iconName: IconName) => void }> = ({ iconName, onClick }) => { |
There was a problem hiding this comment.
Same feels here, only used in the Row component, why not inline this as well?
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 (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
99-116: Normalize glyph (treat null and empty-string equivalent) and align client typesOpenAPI/schema accepts glyph:null (model.asset-group-tag-request.yaml → null.string) but js-client-library request types only allow string; the current check (data.glyph != workingCopy.glyph) will treat null vs '' as changed and send glyph: ''. Normalize the comparison and only include glyph when it has a meaningful value; if the API requires an explicit “clear”, widen the request types to allow null and send glyph: null.
File: packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (lines 99–116). Also consider updating packages/javascript/js-client-library/src/requests.ts (Create/UpdateAssetGroupTagRequest: glyph?: string | null) if you choose to send explicit null to clear.
- if (data.glyph != workingCopy.glyph) diffed.glyph = workingCopy.glyph; + // Treat null and empty-string as equivalent “no glyph” + const norm = (v?: string | null) => (v ?? '').trim(); + if (norm(data.glyph) !== norm(workingCopy.glyph)) { + // Only send when there is a meaningful value; leave unset to avoid pushing '' unnecessarily + if (norm(workingCopy.glyph) !== '') { + diffed.glyph = workingCopy.glyph; + } else { + // Intentionally omit; confirm API semantics for “clear glyph” + // If backend requires explicit clear, consider sending a separate flag or allow null. + } + }
🧹 Nitpick comments (12)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (5)
139-156: Fix ARIA misuse on “Clear selection” button.
aria-describedbymust reference an element id, not a string. Usearia-label.-<Button - variant={'text'} - onClick={handleClear} - aria-describedby='Clear selection'> +<Button + variant={'text'} + onClick={handleClear} + aria-label='Clear selection'>
159-163: Add accessible name to search input.Provide an aria-label (and input type) for screen readers.
-<Input placeholder='Search' onChange={handleChange} autoFocus className='pl-8' /> +<Input + type='search' + placeholder='Search' + aria-label='Search glyphs' + onChange={handleChange} + autoFocus + className='pl-8' +/>
82-90: Name “magic numbers” for list layout.Extract row width/height and columns to constants for readability and single‑source‑of‑truth.
+const COLS = 5; +const CARD_HEIGHT = 128; +const LABEL_HEIGHT = 64; +const ROW_HEIGHT = CARD_HEIGHT + LABEL_HEIGHT; ... - <FixedSizeList - height={64 * 9} - itemCount={Math.ceil(filteredList.length / 5)} + <FixedSizeList + height={LABEL_HEIGHT * 9} + itemCount={Math.ceil(filteredList.length / COLS)} itemData={{ filteredList, onClick }} - itemSize={128 + 64} + itemSize={ROW_HEIGHT}
66-70: Optional: ensure placeholders are non-interactive.You rely on
invisibleto prevent clicks. Consider alsoaria-hiddenanddisabledon the Button when!iconNameto harden against CSS regressions.-<IconCard key={row * 5 + index} iconName={filteredList[row * 5 + index]} onClick={onClick} />; +<IconCard + key={row * 5 + index} + iconName={filteredList[row * 5 + index]} + onClick={onClick} />;Inside IconCard:
-<Button +<Button variant={'text'} - className={clsx(['relative', !iconName && 'invisible'])} + className={clsx(['relative', !iconName && 'invisible'])} + aria-hidden={!iconName} + disabled={!iconName}
51-55: Label text may overflow off-card.Absolute
-bottom-16could clip in small viewports. Consider truncating with tooltip or using a caption area with measured height.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (6)
118-127: Initialize form with undefined for optional glyph.Using
''forces downstream equality checks and leaks into payloads. Preferundefined.- defaultValues: { + defaultValues: { name: '', description: '', position: -1, require_certify: false, analysis_enabled: false, - glyph: '', + glyph: undefined, },
251-261: Normalize incoming glyph to undefined.Avoid converting
nullto''; keep “no glyph” asundefinedfor consistent diffing.- form.reset({ + form.reset({ name: tagQuery.data.name, description: tagQuery.data.description, position: tagQuery.data.position, require_certify: tagQuery.data.require_certify || false, analysis_enabled: tagQuery.data.analysis_enabled || false, - glyph: tagQuery.data.glyph === null ? '' : tagQuery.data.glyph, + glyph: tagQuery.data.glyph ?? undefined, });
136-144: Trim glyph before including in create payload.Prevents accidental whitespace-only values.
- if (formData.glyph !== '') requestValues.glyph = formData.glyph; + const glyph = formData.glyph?.trim(); + if (glyph) requestValues.glyph = glyph;
241-249: Tighten glyph selection handler.Single call with normalized value; mark as touched for validation UX.
- (iconName?: IconName) => { - if (iconName) form.setValue('glyph', iconName, { shouldDirty: true }); - else form.setValue('glyph', '', { shouldDirty: true }); - - setGlyphDialogOpen(false); - }, + (iconName?: IconName) => { + form.setValue('glyph', iconName ?? undefined, { shouldDirty: true, shouldTouch: true }); + setGlyphDialogOpen(false); + },
342-343: Watch glyph for reactive UI updates.
getValueswon’t re-render on change unless another state flips. Preferwatch.- const glyph = form.getValues('glyph'); + const glyph = form.watch('glyph');
583-588: Don’t cast empty string to IconName.Pass
undefinedwhen no glyph is selected to align with dialog’s prop type.- <GlyphSelectDialog - open={glyphDialogOpen} - selected={glyph as IconName} + <GlyphSelectDialog + open={glyphDialogOpen} + selected={(glyph || undefined) as IconName | undefined} onCancel={handleGlyphCancel} onSelect={handleGlyphSelect} />packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
162-163: Guard against empty seeds when showing Cypher.If a selector has no seeds,
seeds[0]will throw even whenseedType === SeedTypeCypherdue to stale data. Add a null-safe check.- {seedType === SeedTypeCypher && <Cypher preview initialInput={seeds[0].value} />} + {seedType === SeedTypeCypher && seeds?.[0]?.value && ( + <Cypher preview initialInput={seeds[0].value} /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx(13 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx
🧬 Code graph analysis (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (2)
packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts (1)
freeIconsList(3-1958)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (5)
packages/javascript/js-client-library/src/types.ts (6)
AssetGroupTag(76-87)AssetGroupTagTypeZone(56-56)AssetGroupTagSelector(127-137)AssetGroupTagSelectorAutoCertifyMap(118-122)SeedTypesMap(104-107)SeedTypeCypher(90-90)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useHighestPrivilegeTagId(454-459)useOwnedTagId(470-473)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (2)
getSelectorSeedType(45-49)isSelector(41-43)packages/javascript/bh-shared-ui/src/hooks/useZoneParams/usePZPathParams.tsx (1)
usePZPathParams(20-51)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx (1)
Cypher(30-156)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (2)
packages/javascript/js-client-library/src/requests.ts (2)
CreateAssetGroupTagRequest(37-44)UpdateAssetGroupTagRequest(46-48)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
⏰ 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-analysis
- GitHub Check: run-tests
🔇 Additional comments (6)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (3)
105-107: Nice: case‑insensitive search implemented.Lowercasing the query aligns with prior feedback and improves findability.
124-131: Dialog composition matches established pattern.Keeping Title/Actions inside Content is consistent with prior intentional design.
31-36: Icons registered at runtime — no changes needed.library.add(fas) is present in packages/javascript/bh-shared-ui/src/utils/icons.ts (line 53). freeIconsList is exported from packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts and imported/used in packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (import at line 36, usage at line 167).
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
174-181: Update payload shaping looks good.Analysis/require_certify stripping matches page type; notifications and navigation are consistent.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (2)
86-93: Glyph rendering next to name looks good.Conditional icon render is safe; small and unobtrusive placement.
60-75: Date formatting and field extraction LGTM.Consistent with Luxon/format usage elsewhere; no issues spotted.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (9)
34-39: Import IconName from svg-core, not free-solidThe canonical IconName type lives in @fortawesome/fontawesome-svg-core. Importing it from free-solid can break typings or tooling.
Apply this diff:
-import { IconName, faTrashCan } from '@fortawesome/free-solid-svg-icons'; +import { IconName } from '@fortawesome/fontawesome-svg-core'; +import { faTrashCan } from '@fortawesome/free-solid-svg-icons';Also applies to: 49-49
110-111: Use strict equality and normalize for glyph/booleans in diffLoose != can hide type/whitespace issues. Prefer strict comparison with nullish coalescing or simple normalization.
Apply this diff:
- if (data.glyph != workingCopy.glyph) diffed.glyph = workingCopy.glyph; - if (data.require_certify != workingCopy.require_certify) diffed.require_certify = workingCopy.require_certify; + if ((data.glyph ?? '') !== (workingCopy.glyph ?? '')) { + diffed.glyph = (workingCopy.glyph ?? '').trim(); + } + if ((data.require_certify ?? null) !== (workingCopy.require_certify ?? null)) { + diffed.require_certify = workingCopy.require_certify; + }
122-126: Default sentinels: verify -1 for position and '' for glyph are intentionalIf -1 and '' leak into update payloads, APIs may reject or persist invalid values. Consider leaving these undefined internally and only setting explicit values when needed.
Would you like me to add a guard to strip position === -1 and glyph === '' from update payloads?
136-144: Trim glyph before including in create payloadPrevents sending whitespace-only glyphs.
Apply this diff:
- if (formData.glyph !== '') requestValues.glyph = formData.glyph; + if (formData.glyph && formData.glyph.trim() !== '') { + requestValues.glyph = formData.glyph.trim(); + }
240-249: Type source and clearing behavior for handleGlyphSelectWith IconName from svg-core, the signature is good. When clearing, consider setting undefined to better represent "no glyph" internally (paired with update guard).
Apply this diff:
- (iconName?: IconName) => { - if (iconName) form.setValue('glyph', iconName, { shouldDirty: true }); - else form.setValue('glyph', '', { shouldDirty: true }); + (iconName?: IconName) => { + form.setValue('glyph', iconName ?? '', { shouldDirty: true }); },
259-260: Prefer nullish coalescing and trim on resetMinor robustness improvement.
Apply this diff:
- glyph: tagQuery.data.glyph === null ? '' : tagQuery.data.glyph, + glyph: (tagQuery.data.glyph ?? '').trim(),
342-343: Use watch for reactive glyph valuewatch('glyph') avoids stale reads and re-renders the preview consistently.
Apply this diff:
- const glyph = form.getValues('glyph'); + const glyph = form.watch('glyph');
506-513: Ensure “Select Glyph” button doesn’t submit the formInside a form, button defaults can be submit. Set type='button' to avoid accidental submissions.
Apply this diff:
- <Button + <Button + type='button' onClick={() => { setGlyphDialogOpen(true); }} className='w-48' variant={'secondary'}> <span>Select Glyph</span> </Button>
585-591: Don’t cast '' to IconName; pass undefined when no selectionPrevents passing an invalid IconName and aligns with dialog APIs that expect optional selection.
Apply this diff:
- <GlyphSelectDialog + <GlyphSelectDialog open={glyphDialogOpen} - selected={glyph as IconName} + selected={glyph || undefined} onCancel={handleGlyphCancel} onSelect={handleGlyphSelect} />If needed, update GlyphSelectDialog props to
selected?: IconName.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx(13 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx
🧬 Code graph analysis (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (4)
packages/javascript/bh-shared-ui/src/providers/NotificationProvider/hooks.ts (1)
useNotifications(43-60)packages/javascript/js-client-library/src/requests.ts (2)
CreateAssetGroupTagRequest(37-44)UpdateAssetGroupTagRequest(46-48)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
handleError(20-42)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
⏰ 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 (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
40-45: Type source for AssetGroupTag: confirm correctness per prior learningWe previously learned not to rely on js-client-library’s AssetGroupTag type for UI tag shapes in some contexts. Verify that useAssetGroupTagInfo/useAssetGroupTags return this exact type. If not, introduce a local UI type or pick required fields for diffing.
From prior learning in another component, this mismatch caused subtle typing issues. Do you want a quick PR to extract a UI Tag type and adapt these signatures?
Also applies to: 99-106
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx
Show resolved
Hide resolved
mistahj67
left a comment
mistahj67
left a comment
There was a problem hiding this comment.
Looks good. Locally working as intended as well.
I had one suggestion for abstraction. Definitely worth waiting to merge until you get confirmation on the main list for supported icons.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (2)
42-59: Improve prop types and accessibility for IconCard.The component handles undefined gracefully but could benefit from more precise typing and better accessibility.
Apply this diff to improve the component:
-const IconCard: FC<{ iconName: IconName | undefined; onClick: (iconName: IconName) => void }> = ({ +const IconCard: FC<{ iconName: IconName | undefined; onClick: (iconName: IconName) => void }> = ({ iconName, onClick, }) => ( <Button variant={'text'} className={clsx(['relative', !iconName && 'invisible'])} + aria-label={iconName ? `Select ${iconName} icon` : undefined} onClick={() => { iconName && onClick(iconName); }}>
70-73: Hardcoded grid width should be consistent.The hardcoded value
5appears both here and in theitemCountcalculation. Consider using a constant for maintainability.Apply this diff to use a consistent constant:
+const ICONS_PER_ROW = 5; + const Row = ({ data, index: row, style, }: ListChildComponentProps<{ filteredList: IconList; onClick: (iconName: IconName) => void }>) => { const { filteredList, onClick } = data; return ( <li className={'flex justify-evenly items-center'} style={{ ...style }}> - {Array.from({ length: 5 }, (_, index) => { - const icon = filteredList[row * 5 + index]; - return <IconCard key={row * 5 + index} iconName={icon ? icon.id : undefined} onClick={onClick} />; + {Array.from({ length: ICONS_PER_ROW }, (_, index) => { + const icon = filteredList[row * ICONS_PER_ROW + index]; + return <IconCard key={row * ICONS_PER_ROW + index} iconName={icon ? icon.id : undefined} onClick={onClick} />; })} </li> ); };Also update the
itemCountcalculation in VirtualizedIconList:<FixedSizeList height={64 * 9} - itemCount={Math.ceil(filteredList.length / 5)} + itemCount={Math.ceil(filteredList.length / ICONS_PER_ROW)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
.yarn/cache/@fortawesome-fontawesome-common-types-npm-6.4.2-1f8b184e1e-4a22932bd0.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-fontawesome-free-npm-6.4.2-49cdde900e-14be8fbb8f.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-fontawesome-free-npm-6.7.2-d3f6d6ec6d-2ceb384ada.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-fontawesome-svg-core-npm-6.4.2-530d31922b-0c0ecd9058.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-fontawesome-svg-core-npm-6.7.2-e22b101297-b3c269545d.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-free-solid-svg-icons-npm-6.4.2-c582f5c032-4a36500499.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-free-solid-svg-icons-npm-6.7.2-35f32a3213-457cc18039.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-react-fontawesome-npm-0.2.0-a36215138f-f652a0c217.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-react-fontawesome-npm-0.2.2-e1863961b2-e4bed35bfb.zipis excluded by!**/.yarn/**,!**/*.zip.yarn/cache/@fortawesome-react-fontawesome-npm-3.0.2-8506c26a1f-86df127661.zipis excluded by!**/.yarn/**,!**/*.zipyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
cmd/ui/package.json(1 hunks)packages/javascript/bh-shared-ui/package.json(1 hunks)packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx(10 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx(13 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts(1 hunks)packages/javascript/js-client-library/src/requests.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx
- packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts
- packages/javascript/js-client-library/src/requests.ts
- packages/javascript/bh-shared-ui/src/utils/index.ts
- packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
PR: SpecterOps/BloodHound#1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx
🧬 Code graph analysis (4)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (2)
packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts (2)
IconList(4-4)freeIconsList(6-5563)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (3)
packages/javascript/js-client-library/src/requests.ts (2)
CreateAssetGroupTagRequest(37-44)UpdateAssetGroupTagRequest(46-48)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
handleError(20-42)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx (2)
packages/javascript/bh-shared-ui/src/routes/index.tsx (3)
privilegeZonesPath(22-22)zonesPath(17-17)savePath(24-24)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
TagForm(67-602)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (5)
packages/javascript/js-client-library/src/types.ts (6)
AssetGroupTag(76-87)AssetGroupTagTypeZone(56-56)AssetGroupTagSelector(127-137)AssetGroupTagSelectorAutoCertifyMap(118-122)SeedTypesMap(104-107)SeedTypeCypher(90-90)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (1)
PrivilegeZonesContext(51-51)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useHighestPrivilegeTagId(454-459)useOwnedTagId(470-473)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (2)
getSelectorSeedType(45-49)isSelector(41-43)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(20-51)
⏰ 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: run-tests
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (27)
cmd/ui/package.json (1)
25-28: LGTM! Consistent FontAwesome upgrades across packages.The upgrades match those in the
bh-shared-uipackage, maintaining consistency across the monorepo. The version bumps align with the broader FontAwesome enhancement for glyph support.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (5)
38-40: Forward ref is required for react-window innerElement.react-window passes a ref to
innerElementType; not forwarding it can break keyboard scrolling/virtualization in edge cases.Apply this diff to forward the ref:
-const InnerElement = ({ style, ...rest }: any) => ( - <ul style={{ ...style, overflowX: 'hidden', marginTop: 0, overflowY: 'auto' }} {...rest}></ul> -); +import { forwardRef } from 'react'; +const InnerElement = forwardRef<HTMLUListElement, any>(({ style, ...rest }, ref) => ( + <ul ref={ref} style={{ ...style, overflowX: 'hidden', marginTop: 0, overflowY: 'auto' }} {...rest} /> +));
86-97: LGTM! Well-structured virtualized list component.The VirtualizedIconList component is properly structured with appropriate height calculations and virtualization settings for performance with large icon lists.
109-110: Good implementation of lowercase search.The toLowerCase() call addresses the previous review feedback and makes the search more user-friendly by being case-insensitive.
100-192: LGTM! Well-implemented glyph selection dialog.The component provides excellent UX with proper state management, search functionality, current selection display with clear button, and proper dialog accessibility.
171-173: Normalize the search query before filtering (make label/id matching case-insensitive)File: packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (around lines 171-173) — compute a normalized query first (e.g.
const q = query.trim().toLowerCase()) and use it in the filter:filteredList={freeIconsList.filter(icon => icon.id.toLowerCase().includes(q) || icon.label.toLowerCase().includes(q))}
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx (6)
38-38: LGTM! Test fixtures include glyph data appropriately.The test fixtures correctly include glyph data:
testTierZero.glyph: 'lightbulb'andtestOwned.glyph: null, which aligns with the new glyph functionality being tested.Also applies to: 56-56
96-102: Good test coverage for additional zone data.The new test route for zone 4 provides coverage for non-highest-privileged zones, which is essential for testing glyph functionality that's conditionally rendered.
165-166: LGTM! Test paths properly distinguish zone privilege levels.The updated test paths (
editHighestPrivilegeZonePathandeditOtherZonePath) clearly distinguish between different zone types for better test coverage of glyph-related behavior.Also applies to: 336-350
203-206: Excellent test coverage for conditional glyph rendering.The tests properly verify that:
- Glyph input appears for zone creation
- Glyph input is absent for label creation
- Glyph input is absent for highest privilege zones (Tier Zero)
- Glyph input appears for non-highest privilege zones
This thoroughly covers the conditional rendering logic
isZonePage && form.getValues('position') !== 1.Also applies to: 237-239, 283-285, 324-326
352-393: LGTM! Comprehensive analysis toggle test coverage.The new tests properly verify the analysis toggle behavior based on configuration settings, replacing the previous tests while maintaining comprehensive coverage.
348-350: Glyph count is correct — test expectation is valid.TagForm renders the glyph name in the "Selected Glyph" paragraph and the FontAwesomeIcon yields a second accessible text node (icon title), so findAllByText('lightbulb') matching length 2 is expected.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (9)
110-110: LGTM! Correct glyph diff comparison.The loose equality comparison (
!=) correctly handles bothnullandundefinedcases for glyph comparison in the diff logic.
125-125: LGTM! Proper glyph initialization in form defaults.Initializing
glyph: ''provides a consistent empty state for the form field and aligns with the UI's handling of glyph selection.
136-143: LGTM! Conditional glyph inclusion in create request.The logic correctly includes glyph in the create request only when it's not empty, avoiding unnecessary empty string payloads.
161-206: Do not send glyph: '' — delete the glyph key when emptyThe requests.ts defines
glyph?: stringwhile response types allowglyph: string | null, and TagForm initializesglyph: ''— delete empty-string glyph from the update payload to avoid sending "" (or set to null only if the API explicitly accepts null).Apply this diff:
const updatedValues = { ...diffedValues }; + // Avoid sending empty strings; treat as "no value" + if ('glyph' in updatedValues && (updatedValues.glyph ?? '').trim() === '') { + // requests.ts defines glyph?: string, so prefer omitting the key rather than sending '' + delete (updatedValues as any).glyph; + }
240-249: LGTM! Well-structured glyph dialog handlers.The glyph selection handlers properly manage form state with
shouldDirty: trueand correctly handle both icon selection and clearing scenarios.
259-259: LGTM! Proper null-to-empty-string normalization.The normalization
tagQuery.data.glyph === null ? '' : tagQuery.data.glyphensures consistent handling between API responses (which can be null) and form state (which uses empty string).
298-303: LGTM! Proper skeleton for glyph field during loading.The loading skeleton appropriately includes the glyph field placeholder when in zone context, providing good UX during data fetching.
451-518: Excellent glyph UI implementation with proper conditional rendering.The glyph form field implementation is comprehensive:
- Correctly conditioned on
isZonePage && form.getValues('position') !== 1- Includes helpful tooltip explaining glyph purpose
- Hidden actual input with visible preview display
- Clear glyph name display and icon preview
- Well-structured selection button
585-590: LGTM! Proper dialog integration.The GlyphSelectDialog is properly integrated with state management and callback handlers for a complete glyph selection workflow.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (5)
60-72: LGTM! Comprehensive props destructuring for TagDetails.The expanded destructuring covers all necessary fields from the AssetGroupTag, including the new glyph field, and the prop rename from
datatotagDataimproves code clarity.
87-92: LGTM! Clean glyph icon rendering with proper spacing.The conditional glyph rendering is well-implemented:
- Properly typed with
as IconName- Good visual spacing with explicit space character
- Clean conditional rendering structure
109-113: LGTM! Proper zone-type conditional rendering for certification field.Using
AssetGroupTagTypeZoneconstant for the type comparison is the correct approach and aligns with the learned pattern for BloodHound components.
121-122: LGTM! Consistent props destructuring for SelectorDetails.The destructuring pattern matches the TagDetails approach and covers all necessary selector fields for proper rendering.
181-183: LGTM! Consistent prop passing with renamed parameters.The updated prop names (
tagDataandselectorData) improve code readability and make the component interfaces clearer.packages/javascript/bh-shared-ui/package.json (1)
24-28: Approve — FontAwesome upgrade verified (6.7.2 + react-fontawesome 3.0.2)package.json and src imports/usages are consistent (FontAwesomeIcon, fa* imports, IconName/IconProp, findIconDefinition); no breaking changes found from this static inspection.
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)
.github/workflows/vuln-scan.yml (2)
32-47: Pin the action to a commit SHA and restrict GITHUB_TOKEN permissions.Improve supply‑chain posture by pinning to a full commit SHA and setting least‑privilege permissions.
Apply:
name: Vulnerability Scan +permissions: + contents: read ... - - name: Run vulnerability scanner - uses: aquasecurity/[email protected] + - name: Run vulnerability scanner + # Replace <COMMIT_SHA_FOR_v0.33.1> with the release's commit SHA + uses: aquasecurity/trivy-action@<COMMIT_SHA_FOR_v0.33.1>Optionally add a comment with the release URL next to the SHA for traceability.
39-41: Use scan-type: "fs" with scan-ref: "." for local workspace scanningTrivy-action docs/examples use scan-type: "fs" and scan-ref: '.' for the checked-out workspace; "repo" is for remote/repo scanning — update .github/workflows/vuln-scan.yml (lines 39–41).
🧹 Nitpick comments (1)
package.json (1)
28-36: Consider automating override lifecycle via Renovate/Dependabot rules.Add rules to alert/remove these overrides once upstreams are fixed to avoid long‑term pin drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
.github/workflows/vuln-scan.yml(1 hunks)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). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (2)
.github/workflows/vuln-scan.yml (1)
37-37: Upgrade to [email protected] LGTM.package.json (1)
28-36: Confirm root resolutions are applied and no workspace conflictsRoot package.json contains resolutions for axios, braces, cross-spawn, dompurify, form-data, sha.js, tar-fs.
yarn.lock exists but has no entries matching braces, dompurify, sha.js, or tar-fs; axios is declared in packages/javascript/js-client-library/package.json (axios: ^1.12.0).
Action: run yarn install / update the lockfile so yarn.lock reflects the forced versions and check all workspace package.json files for conflicting explicit ranges.
- Add glyph icon to details info pane on details page - Change display of selected icon on tag form - Add tooltip to clear selection button in glyph select dialog
- upgrades dependencies to latest v6 fa version - deduplicates icons included via aliases - unit test updates - search match on id or label text
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 (1)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (1)
36-36: Selectors: unsafe seed access can crash when seeds are emptyAccessing
seeds[0](directly and viagetSelectorSeedType) will throw if a selector has no seeds. Guard for empties and render safe fallbacks.Apply these diffs:
Remove the brittle util import:
-import { getSelectorSeedType, isSelector, isTag } from './utils'; +import { isSelector, isTag } from './utils';Guard seed extraction:
- const seedType = getSelectorSeedType(selectorData); + const firstSeed = seeds[0]; + const seedType = firstSeed?.type;Render type with fallback:
- <DetailField label='Type' value={SeedTypesMap[seedType]} /> + <DetailField label='Type' value={seedType !== undefined ? SeedTypesMap[seedType] : 'Unknown'} />Only render Cypher preview when present:
-{seedType === SeedTypeCypher && <Cypher preview initialInput={seeds[0].value} />} +{seedType === SeedTypeCypher && firstSeed?.value && <Cypher preview initialInput={firstSeed.value} />}Also applies to: 121-127, 157-160, 162-163
🧹 Nitpick comments (9)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (2)
115-115: Avoid showing SalesMessage while IDs are unresolvedGuard on
topTagIdandownedIdto prevent a brief, incorrect render while hooks load.Apply this diff:
-{tagId !== topTagId && tagId !== ownedId && SalesMessage && <SalesMessage />} +{topTagId !== undefined && + ownedId !== undefined && + tagId !== topTagId && + tagId !== ownedId && + SalesMessage && <SalesMessage />}
86-93: Harden glyph rendering and simplify markupReplace the conditional wrapper and empty spacer with a single icon element:
- {glyph && ( - <span> - <FontAwesomeIcon icon={glyph as IconName} size="sm" /> <span> </span> - </span> - )} + {glyph ? <FontAwesomeIcon icon={['fas', glyph as IconName]} size="sm" className="mr-1" /> : null}Verify
utils/icons.tscallslibrary.add(fas)and that all storedglyphstrings match theIconNameentries infreeIconsList.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (3)
44-61: Disable non-icons to avoid invisible but focusable/clickable buttonsWhen iconName is undefined, the button is invisible but still focusable/clickable. Disable it and hide from the a11y tree.
-const IconCard: FC<{ iconName: IconName | undefined; onClick: (iconName: IconName) => void }> = ({ +const IconCard: FC<{ iconName: IconName | undefined; onClick: (iconName: IconName) => void }> = ({ iconName, onClick, }) => ( <Button variant={'text'} - className={clsx(['relative', !iconName && 'invisible'])} + className={clsx(['relative', !iconName && 'invisible'])} + disabled={!iconName} + aria-hidden={!iconName} onClick={() => { iconName && onClick(iconName); }}>
151-155: A11y: use aria-label instead of aria-describedbyaria-describedby must reference an element ID. Use aria-label for the clear action.
- <Button - variant={'text'} - onClick={handleClear} - aria-describedby='Clear selection'> + <Button + variant={'text'} + onClick={handleClear} + aria-label='Clear selection'>
33-33: Control the search input and memoize filtering
- Controlled input guarantees UI clears when query resets.
- Memoizing avoids O(n) filtering on every render and reduces react-window churn.
-import React, { FC, forwardRef, useEffect, useState } from 'react'; +import React, { FC, forwardRef, useEffect, useMemo, useState } from 'react';- <AppIcon.MagnifyingGlass className='-mr-4' /> - <Input placeholder='Search' onChange={handleChange} autoFocus className='pl-8' /> + <AppIcon.MagnifyingGlass className='-mr-4' /> + <Input + placeholder='Search' + onChange={handleChange} + value={query} + autoFocus + className='pl-8' + />- <VirtualizedIconList - filteredList={freeIconsList.filter( - (icon) => icon.id.includes(query) || icon.label.toLowerCase().includes(query) - )} - onClick={(iconName) => { - setSelectedIcon(iconName); - }} - /> + {(() => { + const filteredList = useMemo( + () => + freeIconsList.filter( + (icon) => + icon.id.includes(query) || + icon.label.toLowerCase().includes(query) + ), + [query] + ); + return ( + <VirtualizedIconList + filteredList={filteredList} + onClick={(iconName) => { + setSelectedIcon(iconName); + }} + /> + ); + })()}Also applies to: 166-169, 172-180
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx (2)
47-49: Avoid brittle assertion on icon renderingVirtualization may not render the selected icon label in the visible region. Assert presence once instead of count.
- expect(screen.getAllByText('lightbulb')).toHaveLength(2); + expect(screen.getByText('lightbulb')).toBeInTheDocument();
62-65: Assert selected value is passed throughStrengthen the test to validate the argument.
- expect(onSelect).toHaveBeenCalled(); + expect(onSelect).toHaveBeenCalledWith('lightbulb');packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (2)
591-596: Type-safe selected prop for GlyphSelectDialogPass undefined when no glyph instead of casting '' to IconName.
- selected={glyph as IconName} + selected={glyph || undefined}
118-119: Loose inequality can mis-diff null vs ''data.glyph (string | null) compared to workingCopy.glyph (string) with != will treat null vs '' as change, producing glyph: ''. Combined with the update fix, this is OK but still noisy. Prefer strict comparison after normalizing.
- if (data.glyph != workingCopy.glyph) diffed.glyph = workingCopy.glyph; + const normalizedGlyph = (workingCopy.glyph ?? '').trim(); + const currentGlyph = data.glyph ?? ''; + if (currentGlyph !== normalizedGlyph) diffed.glyph = normalizedGlyph;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx(2 hunks)packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/utils/index.ts(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx(3 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx(1 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx(9 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx(13 hunks)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts(1 hunks)packages/javascript/js-client-library/src/requests.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/javascript/bh-shared-ui/src/utils/index.ts
- packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsx
- packages/javascript/js-client-library/src/requests.ts
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.test.tsx
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/utils.ts
- packages/javascript/bh-shared-ui/src/components/DeleteConfirmationDialog.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
PR: SpecterOps/BloodHound#1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx
🧬 Code graph analysis (3)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (6)
packages/javascript/js-client-library/src/types.ts (6)
AssetGroupTag(85-96)AssetGroupTagTypeZone(65-65)AssetGroupTagSelector(136-146)AssetGroupTagSelectorAutoCertifyMap(127-131)SeedTypesMap(113-116)SeedTypeCypher(99-99)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/PrivilegeZonesContext.tsx (1)
PrivilegeZonesContext(51-51)packages/javascript/bh-shared-ui/src/hooks/useAssetGroupTags/useAssetGroupTags.tsx (2)
useHighestPrivilegeTagId(454-459)useOwnedTagId(470-473)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/utils.tsx (2)
getSelectorSeedType(45-49)isSelector(41-43)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
usePZPathParams(20-51)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Cypher/Cypher.tsx (1)
Cypher(30-156)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (3)
packages/javascript/js-client-library/src/requests.ts (2)
UpdateAssetGroupTagRequest(46-48)CreateAssetGroupTagRequest(37-44)packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/utils.ts (1)
handleError(20-42)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (2)
packages/javascript/bh-shared-ui/src/utils/freeIconsList.ts (2)
IconList(7-7)freeIconsList(12-5569)packages/javascript/bh-shared-ui/src/components/AppIcon/AppIcon.tsx (1)
AppIcon(28-28)
⏰ 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: run-tests
- GitHub Check: build-ui
🔇 Additional comments (5)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx (2)
150-153: LGTM: correct human-readable mapping for auto-certifyUsing
AssetGroupTagSelectorAutoCertifyMapwith an'Off'fallback reads well.
171-186: LGTM: discriminated rendering via type guardsClean split between TagDetails and SelectorDetails using
isTag/isSelectorlooks good.packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.tsx (1)
38-42: Forward ref for react-window inner element looks goodThis fixes the ref requirement for react-window’s innerElementType.
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/GlyphSelectDialog.test.tsx (1)
24-26: Reset mocks between testsWithout clearing, call counts can leak across tests.
describe('Glyph Select Dialog', () => { - const user = userEvent.setup(); + const user = userEvent.setup(); + beforeEach(() => { + vi.clearAllMocks(); + });packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Save/TagForm/TagForm.tsx (1)
179-182: Don’t send glyph: '' in update payloadsAPI types define glyph?: string. Sending an empty string can cause validation issues. Omit the key when empty.
- const updatedValues = { ...diffedValues }; + const updatedValues = { ...diffedValues }; + // Avoid sending empty strings; omit glyph when not set + if ('glyph' in updatedValues && (updatedValues.glyph ?? '').trim() === '') { + delete (updatedValues as any).glyph; + }
| import { IconName } from '@fortawesome/free-solid-svg-icons'; | ||
| import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; |
There was a problem hiding this comment.
Incorrect FontAwesome type import — will break TypeScript build
Import IconName from @fortawesome/fontawesome-svg-core, not from @fortawesome/free-solid-svg-icons.
Apply this diff:
-import { IconName } from '@fortawesome/free-solid-svg-icons';
+import { IconName } from '@fortawesome/fontawesome-svg-core';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';📝 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.
| import { IconName } from '@fortawesome/free-solid-svg-icons'; | |
| import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; | |
| import { IconName } from '@fortawesome/fontawesome-svg-core'; | |
| import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; |
🤖 Prompt for AI Agents
In
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/DynamicDetails.tsx
around lines 18 to 19, the code imports IconName from the wrong package which
will break the TypeScript build; change the import so IconName is imported from
'@fortawesome/fontawesome-svg-core' instead of
'@fortawesome/free-solid-svg-icons', keeping FontAwesomeIcon import from
'@fortawesome/react-fontawesome' unchanged; update the import line accordingly
and run typecheck to verify the build succeeds.
Description
Adds glyph selection to zone create and update forms.
Motivation and Context
Resolves BED-6151
How Has This Been Tested?
Unit tests were added.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit