fix: Revert focus state - BED-8864 - #2952
Conversation
📝 WalkthroughWalkthroughThis PR removes focus-visible ring/outline styling and simplifies className/icon-style props across doodle-ui and bh-shared-ui component libraries, trims legacy Tailwind CSS variables and MUI theme overrides, refactors the DropdownSelector trigger implementation, and refactors branching logic in a Go integration test. ChangesUI focus styling and theme cleanup
Go integration test
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/doodle-ui/src/components/DataTable/DataTable.tsx (1)
574-597: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRow click is no longer keyboard-accessible.
Rows conditionally show
cursor-pointerwhenonRowClickis set (Line 591), but theonKeyDownEnter/Space handling androle/tabIndexforwarding for the row were removed along with the focus-ring styling. Keyboard-only users can no longer activateonRowClickon a row.♿ Proposed fix to restore keyboard activation
<TableRow key={row.id} + role={onRowClick ? 'button' : undefined} + tabIndex={onRowClick ? 0 : undefined} onClick={(e) => { e.stopPropagation(); handleRowClick(row); }} + onKeyDown={(e) => { + if (onRowClick && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + handleRowClick(row); + } + }} data-state={row.getIsSelected() && 'selected'}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/doodle-ui/src/components/DataTable/DataTable.tsx` around lines 574 - 597, Restore keyboard accessibility in DataTable by re-adding the row interaction wiring around TableRow in the row render path: when onRowClick is present, forward role/tabIndex and handle onKeyDown for Enter/Space so handleRowClick can be triggered from the keyboard. Also restore the focus-visible styling that was removed so keyboard users can see which row is focused.
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTrigger.tsx (1)
21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated prop-type literal drifts from
DropdownTriggerContentsProps.This FC prop object type duplicates
DropdownTriggerContentsProps(DropdownTriggerContents.tsx lines 21-30) almost verbatim, but is already missingreadOnlyand — per the linked fix — should also gain the widerButtonPropspass-through. Importing the type fromDropdownTriggerContents.tsx(or vice versa) would keep both in sync automatically.♻️ Suggested consolidation
-import { ButtonProps, PopoverTrigger } from 'doodle-ui'; -import { FC } from 'react'; -import DropdownTriggerContents from './DropdownTriggerContents'; +import { PopoverTrigger } from 'doodle-ui'; +import { FC } from 'react'; +import DropdownTriggerContents, { DropdownTriggerContentsProps } from './DropdownTriggerContents'; -const DropdownTrigger: FC<{ - open: boolean; - selectedText: JSX.Element | string; - buttonProps?: ButtonProps; - StartAdornment?: React.FC; - EndAdornment?: React.FC; - testId?: string; - variant?: ButtonProps['variant']; -}> = (props) => { +const DropdownTrigger: FC<DropdownTriggerContentsProps> = (props) => {(requires exporting
DropdownTriggerContentsPropsfromDropdownTriggerContents.tsx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTrigger.tsx` around lines 21 - 34, The prop type on DropdownTrigger is drifting from DropdownTriggerContentsProps and is missing shared fields like readOnly and the wider ButtonProps pass-through. Update DropdownTrigger to reuse the existing DropdownTriggerContentsProps type from DropdownTriggerContents and make sure that type is exported from DropdownTriggerContents.tsx so both components stay in sync. Keep the DropdownTrigger and DropdownTriggerContents symbols aligned by consolidating the duplicated FC prop shape rather than maintaining two separate literals.packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsx (1)
30-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared button styling here
These trigger classes duplicatedoodle-ui’s exportedButtonVariantsbase styling. Pulling from that shared source — or wrapping aButtonwithPopoverTrigger asChild— would keep the menu trigger aligned with future Button changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsx` around lines 30 - 39, The PopoverTrigger in SaveQueryActionMenu duplicates the shared Button styling instead of reusing the existing button variants. Update the trigger to use the shared Button/ButtonVariants styling from doodle-ui, ideally via PopoverTrigger with asChild wrapping a Button, so the menu trigger stays aligned with future Button changes and avoids duplicated class strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsx`:
- Around line 21-55: DropdownTriggerContents is dropping standard button/ARIA
props such as aria-describedby, which breaks the programmatic link from
DropdownSelector to the trigger. Update DropdownTriggerContents (and the
DropdownTrigger path that feeds it) so non-styling props are accepted and
forwarded to Button, or narrow buttonProps to className only if that is the
intended contract. Use the existing DropdownTriggerContentsProps and the Button
render in DropdownTriggerContents to locate the change and ensure aria and other
native button attributes pass through.
In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx`:
- Around line 90-94: The FontAwesome branch in ZoneIcon is not receiving the
shared size prop, so glyph-backed zones can render at the default size instead
of matching the other icon variants. Update the iconDefinition/FontAwesomeIcon
path in ZoneIcon to pass through the same size value used by AppIcon.Zones so
both branches stay visually consistent.
In `@packages/javascript/doodle-ui/src/components/Button/Button.tsx`:
- Line 22: The Button and Link focus-ring utilities still depend on the missing
ring/background theme tokens, so restore those color definitions in the Tailwind
preset. Update the preset used by `Button.tsx` and `Link.tsx` in the tailwind
configuration so `focus-visible:ring-ring` and `ring-offset-background` resolve
again, keeping the existing utility classes unchanged.
In `@packages/javascript/doodle-ui/src/components/Pagination/Pagination.tsx`:
- Around line 50-54: PaginationLink is no longer applying its built-in button
baseline styles, so exported consumers render unstyled unless they happen to
come through PaginationPrevious or PaginationNext. Restore the default
sizing/layout/disabled styling inside PaginationLink (the component that returns
Slot or button), and keep className as an additive override instead of replacing
the base styles. Make sure the wrapper components still layer on their spacing
without depending on PaginationLink to provide everything.
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx`:
- Around line 27-31: The Switch component’s focus styles remove the
keyboard-focus indicator, so restore a visible focus state in both
switchRootStyles and switchThumbStyles in Switch.tsx. Remove the focus-reset
classes that suppress the ring and add a focus-visible indicator consistent with
the other primitives in this revert, while keeping the disabled styling intact.
In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts`:
- Around line 46-51: The dark theme block in plugin.ts is redefining the wrong
CSS custom property, so the Tailwind `secondary` token from preset.ts still
resolves to the light value. Update the `.dark` theme variables to keep the same
`--secondary-main` symbol used in `:root` and `preset.ts`, and assign it the
dark secondary value instead of introducing `--secondary`; check the dark theme
variable list around the `light`/`dark` theme definitions and ensure the
`secondary` token name stays consistent.
- Line 51: The dark theme legacy variables are missing a `--tertiary` override,
so `preset.ts`’s `tertiary: 'var(--tertiary)'` always falls back to the light
hardcoded value from the `:root` legacy block. Update the `.dark` legacy block
in `plugin.ts` to define `--tertiary` with the dark theme’s tertiary color,
matching the existing pattern used for other theme variables and keeping the
`MAIN` section comments consistent.
In `@packages/javascript/doodle-ui/src/tailwind/preset.ts`:
- Around line 33-40: The secondary color token is mapped to a CSS variable name
that does not match the dark theme definition, so it can resolve incorrectly
when themes switch. Update the token in the Tailwind preset’s color map (the
secondary entry in preset.ts) to use the same variable name defined by the theme
plugin’s `.dark` block, and make sure the naming is consistent across preset.ts
and plugin.ts for the secondary token.
---
Outside diff comments:
In `@packages/javascript/doodle-ui/src/components/DataTable/DataTable.tsx`:
- Around line 574-597: Restore keyboard accessibility in DataTable by re-adding
the row interaction wiring around TableRow in the row render path: when
onRowClick is present, forward role/tabIndex and handle onKeyDown for
Enter/Space so handleRowClick can be triggered from the keyboard. Also restore
the focus-visible styling that was removed so keyboard users can see which row
is focused.
---
Nitpick comments:
In
`@packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTrigger.tsx`:
- Around line 21-34: The prop type on DropdownTrigger is drifting from
DropdownTriggerContentsProps and is missing shared fields like readOnly and the
wider ButtonProps pass-through. Update DropdownTrigger to reuse the existing
DropdownTriggerContentsProps type from DropdownTriggerContents and make sure
that type is exported from DropdownTriggerContents.tsx so both components stay
in sync. Keep the DropdownTrigger and DropdownTriggerContents symbols aligned by
consolidating the duplicated FC prop shape rather than maintaining two separate
literals.
In
`@packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsx`:
- Around line 30-39: The PopoverTrigger in SaveQueryActionMenu duplicates the
shared Button styling instead of reusing the existing button variants. Update
the trigger to use the shared Button/ButtonVariants styling from doodle-ui,
ideally via PopoverTrigger with asChild wrapping a Button, so the menu trigger
stays aligned with future Button changes and avoids duplicated class strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 17cef849-24ae-4339-94e1-8a55eaf48857
📒 Files selected for processing (38)
packages/javascript/bh-shared-ui/src/components/ConfirmationDialog.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTrigger.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsxpackages/javascript/bh-shared-ui/src/components/DropdownSelector/constants.tspackages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsxpackages/javascript/bh-shared-ui/src/components/NodeIcon/NodeIcon.tsxpackages/javascript/bh-shared-ui/src/components/SearchResultItem/SearchResultItem.tsxpackages/javascript/bh-shared-ui/src/components/SimpleEnvironmentSelector/SimpleEnvironmentSelector.tsxpackages/javascript/bh-shared-ui/src/constants.tspackages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/SaveQueryActionMenu.tsxpackages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxpackages/javascript/doodle-ui/src/components/Accordion/Accordion.tsxpackages/javascript/doodle-ui/src/components/Alert/Alert.tsxpackages/javascript/doodle-ui/src/components/Breadcrumbs/Breadcrumbs.tsxpackages/javascript/doodle-ui/src/components/Button/Button.tsxpackages/javascript/doodle-ui/src/components/Carousel/Carousel.tsxpackages/javascript/doodle-ui/src/components/Checkbox/Checkbox.tsxpackages/javascript/doodle-ui/src/components/DataTable/DataTable.tsxpackages/javascript/doodle-ui/src/components/DataTable/StorybookExample/utils.tsxpackages/javascript/doodle-ui/src/components/Dialog/Dialog.tsxpackages/javascript/doodle-ui/src/components/Input/Input.tsxpackages/javascript/doodle-ui/src/components/Link/Link.tsxpackages/javascript/doodle-ui/src/components/Menu/Menu.tsxpackages/javascript/doodle-ui/src/components/Pagination/Pagination.tsxpackages/javascript/doodle-ui/src/components/Popover/Popover.tsxpackages/javascript/doodle-ui/src/components/RadioGroup/RadioGroup.tsxpackages/javascript/doodle-ui/src/components/Select/Select.tsxpackages/javascript/doodle-ui/src/components/Switch/Switch.tsxpackages/javascript/doodle-ui/src/components/Table/Table.tsxpackages/javascript/doodle-ui/src/components/Tabs/Tabs.tsxpackages/javascript/doodle-ui/src/components/Textarea/Textarea.tsxpackages/javascript/doodle-ui/src/components/Toggle/Toggle.tsxpackages/javascript/doodle-ui/src/components/ToggleGroup/ToggleGroup.tsxpackages/javascript/doodle-ui/src/components/Tooltip/Tooltip.tsxpackages/javascript/doodle-ui/src/tailwind/plugin.tspackages/javascript/doodle-ui/src/tailwind/preset.tsserver/graphdb/internal/appdb/node_integration_test.go
💤 Files with no reviewable changes (1)
- packages/javascript/bh-shared-ui/src/constants.ts
| type DropdownTriggerContentsProps = { | ||
| open: boolean; | ||
| selectedText: JSX.Element | string; | ||
| buttonProps?: ButtonProps; | ||
| StartAdornment?: FC; | ||
| EndAdornment?: FC; | ||
| StartAdornment?: React.FC; | ||
| EndAdornment?: React.FC; | ||
| testId?: string; | ||
| variant?: ButtonProps['variant']; | ||
| readOnly?: boolean; | ||
| }; | ||
|
|
||
| const DropdownTriggerContents = forwardRef<HTMLButtonElement, DropdownTriggerContentsProps>( | ||
| ( | ||
| { | ||
| open, | ||
| selectedText, | ||
| buttonProps, | ||
| StartAdornment, | ||
| EndAdornment, | ||
| testId, | ||
| variant, | ||
| readOnly, | ||
| className, | ||
| ...props | ||
| }, | ||
| ref | ||
| ) => { | ||
| const buttonPrimary = variant === 'primary'; | ||
| const DropdownTriggerContents = ({ | ||
| open, | ||
| selectedText, | ||
| buttonProps, | ||
| StartAdornment, | ||
| EndAdornment, | ||
| testId, | ||
| variant, | ||
| readOnly, | ||
| }: DropdownTriggerContentsProps) => { | ||
| const buttonPrimary = variant === 'primary'; | ||
|
|
||
| return ( | ||
| <Button | ||
| ref={ref} | ||
| {...props} | ||
| variant={variant ?? 'transparent'} | ||
| className={cn( | ||
| 'uppercase group', | ||
| buttonPrimary && `w-full text-sm ${dropdownIconStateStyles}`, | ||
| { | ||
| [triggerStyles]: !buttonPrimary, | ||
| 'bg-primary text-white dark:text-neutral-dark-1 border-transparent [&_svg]:text-white dark:[&_svg]:text-neutral-dark-1 [&_svg]:fill-current [&_svg_*]:text-white dark:[&_svg_*]:text-neutral-dark-1 [&_svg_*]:fill-current': | ||
| open, | ||
| }, | ||
| className, | ||
| buttonProps?.className | ||
| return ( | ||
| <Button | ||
| variant={variant} | ||
| className={cn( | ||
| 'uppercase', | ||
| { | ||
| 'w-full text-sm': buttonPrimary, | ||
| [triggerStyles]: !buttonPrimary, | ||
| 'bg-primary text-white border-transparent': open, | ||
| }, | ||
| buttonProps?.className | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant components and their prop types/usage.
git ls-files 'packages/javascript/bh-shared-ui/src/components/DropdownSelector/*' \
'packages/javascript/bh-shared-ui/src/components/*DropdownTrigger*' \
'packages/javascript/bh-shared-ui/src/components/*Button*' |
sed 's#^`#FILE`: #'
echo
echo '--- DropdownTriggerContents.tsx ---'
sed -n '1,220p' packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsx
echo
echo '--- DropdownTrigger.tsx ---'
sed -n '1,220p' packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTrigger.tsx
echo
echo '--- DropdownSelector.tsx ---'
sed -n '1,220p' packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.tsx
echo
echo '--- ButtonProps definition search ---'
rg -n "export type ButtonProps|type ButtonProps|interface ButtonProps|ButtonProps" packages/javascript/bh-shared-ui/src/components -g '!**/*.stories.*'Repository: SpecterOps/BloodHound
Length of output: 11772
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- usages of DropdownTrigger / DropdownTriggerContents ---'
rg -n "DropdownTriggerContents|<DropdownTrigger\b|<DropdownTriggerContents\b" packages/javascript/bh-shared-ui/src -g '!**/*.stories.*'
echo
echo '--- buttonProps occurrences in DropdownSelector area ---'
rg -n "buttonProps" packages/javascript/bh-shared-ui/src/components/DropdownSelector -g '!**/*.stories.*'
echo
echo '--- DropdownSelector test file ---'
sed -n '1,260p' packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownSelector.test.tsxRepository: SpecterOps/BloodHound
Length of output: 5983
🌐 Web query:
TypeScript JSX excess property checking custom function components unknown props custom components documentation
💡 Result:
In TypeScript, JSX attributes undergo strict type checking, including excess property checks, to ensure that only props defined in a component's type interface are passed [1][2]. When you use a custom function component in JSX, TypeScript determines the valid props by examining the type of the component's first argument [1][3]. If you pass a property to a component that is not defined in its prop interface, TypeScript will report an error (e.g., "Property '...' does not exist on type '...'") [2][3]. This mechanism serves as a safeguard against typos and passing unnecessary or unsupported props [3]. Key behaviors regarding JSX property checking include: 1. Freshness and Excess Properties: Similar to object literal assignment, JSX attributes are treated with "freshness" [2]. If you pass attributes directly (e.g., ), TypeScript performs an excess property check, and unknown props will trigger a compiler error [2]. 2. Spread Attributes: Excess property checking behavior changes when using the spread operator [2]. If you spread a props object into a component (e.g., <MyComponent {...props} />), TypeScript may allow excess properties because the "freshness" flag is effectively removed, focusing instead on whether the spread object is assignable to the required prop type [2]. 3. Intrinsic vs. Value-based Elements: - Intrinsic elements (e.g., div, span) are validated against the global JSX.IntrinsicElements interface [1][3]. - Value-based elements (custom components) are validated against the prop interface defined in the component's function signature [1][3]. 4. Global Attributes: If you need to allow specific extra properties across all components (for instance, framework-level attributes like key or ref), you can use the JSX.IntrinsicAttributes interface, which allows you to define properties that are not part of the standard component props [1][3]. If you specifically want to allow any arbitrary props on a component, you would need to define an index signature in your component's prop interface (e.g., [key: string]: any;), though this is generally discouraged as it bypasses the type-safety that TypeScript is intended to provide [3].
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/jsx
- 2: Can't pass props with excess properties microsoft/TypeScript#15463
- 3: https://www.typescriptlang.org/docs/handbook/jsx.html
Restore prop passthrough on the dropdown trigger. DropdownSelector still passes aria-describedby, but DropdownTrigger/DropdownTriggerContents neither accept nor forward it, so the caption loses its programmatic association with the button. If buttonProps is only for styling, narrow it to className; otherwise spread the remaining props onto <Button> so standard button/ARIA attributes can flow through.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/javascript/bh-shared-ui/src/components/DropdownSelector/DropdownTriggerContents.tsx`
around lines 21 - 55, DropdownTriggerContents is dropping standard button/ARIA
props such as aria-describedby, which breaks the programmatic link from
DropdownSelector to the trigger. Update DropdownTriggerContents (and the
DropdownTrigger path that feeds it) so non-styling props are accepted and
forwarded to Button, or narrow buttonProps to className only if that is the
intended contract. Use the existing DropdownTriggerContentsProps and the Button
render in DropdownTriggerContents to locate the change and ensure aria and other
native button attributes pass through.
| {iconDefinition ? ( | ||
| <FontAwesomeIcon icon={iconDefinition} className={iconClasses} /> | ||
| <FontAwesomeIcon icon={iconDefinition} /> | ||
| ) : ( | ||
| <AppIcon.Zones size={size} className={iconClasses} /> | ||
| <AppIcon.Zones size={size} /> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for callers passing iconClasses to ZoneIcon
rg -nP -A2 -B2 '<ZoneIcon\b' packages/javascript/bh-shared-ui/src | rg -n "iconClasses"Repository: SpecterOps/BloodHound
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file, then read the relevant section with line numbers.
ast-grep outline packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx --view expanded || true
printf '\n--- FILE (relevant section) ---\n'
sed -n '1,220p' packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsxRepository: SpecterOps/BloodHound
Length of output: 5508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all ZoneIcon call sites and any iconClasses usage.
rg -n --glob 'packages/javascript/bh-shared-ui/src/**/*.{ts,tsx,js,jsx}' '\b<ZoneIcon\b|\bZoneIcon\(' packages/javascript/bh-shared-ui/src || true
printf '\n--- iconClasses references in target area ---\n'
rg -n --glob 'packages/javascript/bh-shared-ui/src/**/*.{ts,tsx,js,jsx}' 'iconClasses' packages/javascript/bh-shared-ui/src || trueRepository: SpecterOps/BloodHound
Length of output: 838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target component and nearby type definitions.
sed -n '1,240p' packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx
printf '\n--- nearby exports/usages ---\n'
rg -n --glob 'packages/javascript/bh-shared-ui/src/**/*.{ts,tsx}' 'export type ZoneIconProps|interface ZoneIconProps|type ZoneIconProps|iconClasses|FontAwesomeIcon' packages/javascript/bh-shared-ui/src/views/PrivilegeZones packages/javascript/bh-shared-ui/src | head -n 200Repository: SpecterOps/BloodHound
Length of output: 36837
Pass size through the FontAwesome branch. Glyph-backed zones ignore the shared size prop here, so they can render at FontAwesome’s default size instead of matching the other icon variants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/bh-shared-ui/src/views/PrivilegeZones/ZoneIcon.tsx`
around lines 90 - 94, The FontAwesome branch in ZoneIcon is not receiving the
shared size prop, so glyph-backed zones can render at the default size instead
of matching the other icon variants. Update the iconDefinition/FontAwesomeIcon
path in ZoneIcon to pass through the same size value used by AppIcon.Zones so
both branches stay visually consistent.
|
|
||
| export const ButtonVariants = cva( | ||
| 'inline-flex items-center justify-center whitespace-nowrap h-10 px-6 py-2 rounded-3xl text-sm transition-colors hover:underline focus:outline-none focus-visible:focus-ring disabled:pointer-events-none disabled:opacity-50 active:no-underline', | ||
| 'inline-flex items-center justify-center whitespace-nowrap h-10 px-6 py-2 rounded-3xl text-sm ring-offset-background transition-colors hover:underline focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:pointer-events-none disabled:opacity-50 active:no-underline', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check that ring / ring-offset-background tokens are still defined in the tailwind preset/theme
rg -n "ring-offset-background|'ring'|\"ring\"|--ring" packages/javascript/doodle-ui/src/tailwind/preset.ts packages/javascript/doodle-ui/src/tailwind/plugin.tsRepository: SpecterOps/BloodHound
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the doodle-ui tailwind/theme files and inspect where ring tokens are defined.
git ls-files 'packages/javascript/doodle-ui/**' | rg 'tailwind|preset|plugin|constants|Button\.tsx|Link\.tsx|theme|tailwind\.config'
printf '\n## Matches for ring-related tokens in doodle-ui\n'
rg -n --hidden --glob 'packages/javascript/doodle-ui/**' \
'ring-offset-background|focus-visible:ring-ring|--ring|--tw-ring|--tw-ring-offset|ring-ring' \
packages/javascript/doodle-ui
printf '\n## Key file contents\n'
for f in \
packages/javascript/doodle-ui/src/tailwind/preset.ts \
packages/javascript/doodle-ui/src/tailwind/plugin.ts \
packages/javascript/doodle-ui/src/components/Link/Link.tsx \
packages/javascript/doodle-ui/src/components/Button/Button.tsx \
packages/javascript/doodle-ui/src/constants.ts
do
if [ -f "$f" ]; then
echo "\n### $f"
sed -n '1,240p' "$f"
fi
doneRepository: SpecterOps/BloodHound
Length of output: 29335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the tokens used by focus-visible:ring-ring and ring-offset-background are defined anywhere in doodle-ui.
rg -n --hidden --glob 'packages/javascript/doodle-ui/**' \
"['\"]ring['\"]|ring:\s*['\"]|--ring|['\"]background['\"]|background:\s*['\"]|--background|ring-offset-background" \
packages/javascript/doodle-uiRepository: SpecterOps/BloodHound
Length of output: 1539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the doodle-ui Tailwind config and any shared color definitions for ring/background tokens.
sed -n '1,220p' packages/javascript/doodle-ui/tailwind.config.js
printf '\n## tailwind colors\n'
sed -n '1,240p' packages/javascript/doodle-ui/src/tailwind/colors.ts
printf '\n## direct token searches\n'
rg -n --hidden --glob 'packages/javascript/doodle-ui/**' \
"['\"]ring['\"]|['\"]background['\"]|ring:\s|background:\s|--ring|--background|ringOffset|ring-offset" \
packages/javascript/doodle-ui/src packages/javascript/doodle-ui/tailwind.config.jsRepository: SpecterOps/BloodHound
Length of output: 13264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the whole doodle-ui package for theme color definitions of ring/background.
rg -n --hidden --glob 'packages/javascript/doodle-ui/**' \
"ring:\s*['\"]|background:\s*['\"]|--ring|--background|theme:\s*{|extend:\s*{[^}]*colors" \
packages/javascript/doodle-ui/src packages/javascript/doodle-ui/tailwind.config.js packages/javascript/doodle-uiRepository: SpecterOps/BloodHound
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant Tailwind preset section around the token-regex and color/theme definitions.
wc -l packages/javascript/doodle-ui/src/tailwind/preset.ts
printf '\n## preset.ts outline\n'
ast-grep outline packages/javascript/doodle-ui/src/tailwind/preset.ts --view expanded
printf '\n## preset.ts around the regex and any ring/background definitions\n'
sed -n '220,320p' packages/javascript/doodle-ui/src/tailwind/preset.ts
printf '\n## explicit ring/background searches\n'
rg -n --hidden --glob 'packages/javascript/doodle-ui/**' \
'\b(ring|background)\b|--ring|--background|ringOffsetColor|ringColor|backgroundColor' \
packages/javascript/doodle-ui/src/tailwind packages/javascript/doodle-ui/tailwind.config.js packages/javascript/doodle-ui/src/components/Button/Button.tsx packages/javascript/doodle-ui/src/components/Link/Link.tsxRepository: SpecterOps/BloodHound
Length of output: 3073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any actual definitions for the CSS vars / Tailwind color keys used by these classes.
rg -n --hidden --glob '!**/node_modules/**' --glob 'packages/javascript/doodle-ui/**' \
--fixed-strings \
-- '--ring' '--background' 'ring:' 'background:' 'ring-ring' 'ring-offset-background' 'ring-offset-' 'focus-visible:ring-ring'Repository: SpecterOps/BloodHound
Length of output: 540
Restore the ring and background color tokens in the preset. Button.tsx and Link.tsx still use focus-visible:ring-ring / ring-offset-background, but packages/javascript/doodle-ui/src/tailwind/preset.ts no longer defines those colors, so the focus-ring utilities won’t generate correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/doodle-ui/src/components/Button/Button.tsx` at line 22,
The Button and Link focus-ring utilities still depend on the missing
ring/background theme tokens, so restore those color definitions in the Tailwind
preset. Update the preset used by `Button.tsx` and `Link.tsx` in the tailwind
configuration so `focus-visible:ring-ring` and `ring-offset-background` resolve
again, keeping the existing utility classes unchanged.
| const PaginationLink = ({ className, isActive, asChild, ...props }: PaginationLinkProps) => { | ||
| const Comp = asChild ? Slot : 'button'; | ||
|
|
||
| return ( | ||
| <Comp | ||
| aria-current={isActive ? 'page' : undefined} | ||
| className={cn( | ||
| 'inline-flex min-h-9 min-w-9 items-center justify-center rounded-md focus:outline-none focus-visible:focus-ring disabled:cursor-not-allowed disabled:opacity-50', | ||
| className | ||
| )} | ||
| {...props} | ||
| /> | ||
| ); | ||
| return <Comp aria-current={isActive ? 'page' : undefined} className={cn(className)} {...props} />; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A10 'PaginationLink|PaginationPrevious|PaginationNext' packages/javascript/doodle-ui/src/components/Pagination/Pagination.tsxRepository: SpecterOps/BloodHound
Length of output: 3050
Restore PaginationLink’s base styles or keep defaults in the wrapper
PaginationLink now drops all built-in sizing/layout/disabled styling and only forwards className; PaginationPrevious/PaginationNext add spacing but don’t replace the missing baseline styles, so other callers of this exported component will render unstyled buttons.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/doodle-ui/src/components/Pagination/Pagination.tsx`
around lines 50 - 54, PaginationLink is no longer applying its built-in button
baseline styles, so exported consumers render unstyled unless they happen to
come through PaginationPrevious or PaginationNext. Restore the default
sizing/layout/disabled styling inside PaginationLink (the component that returns
Slot or button), and keep className as an additive override instead of replacing
the base styles. Make sure the wrapper components still layer on their spacing
without depending on PaginationLink to provide everything.
| 'group flex h-3 w-6 items-center rounded-3xl transition-all ease-in-out bg-switch-fill data-[state=checked]:bg-primary disabled:data-[state=checked]:bg-disabled disabled:cursor-not-allowed disabled:bg-disabled outline-none focus:outline-none focus-visible:outline-none focus:ring-0 focus-visible:ring-0' | ||
| ); | ||
|
|
||
| const switchThumbStyles = cva( | ||
| 'h-2.5 w-2.5 translate-x-px rounded-full shadow-outer-1 transition-all ease-in-out bg-neutral-50 data-[state=checked]:translate-x-[13px] disabled:bg-neutral-400 group-data-[disabled]:bg-neutral-400 group-data-[disabled]:data-[state=checked]:bg-neutral-400' | ||
| 'h-2.5 w-2.5 translate-x-px rounded-full shadow-outer-1 transition-all ease-in-out bg-neutral-50 data-[state=checked]:translate-x-[13px] disabled:bg-neutral-400 group-data-[disabled]:bg-neutral-400 group-data-[disabled]:data-[state=checked]:bg-neutral-400 focus-visible:outline-none focus-visible:ring-0' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Switch has no visible keyboard-focus indicator.
Both switchRootStyles and switchThumbStyles explicitly zero out focus styling (outline-none focus:outline-none focus-visible:outline-none focus:ring-0 focus-visible:ring-0). Unlike every other primitive touched in this revert (Button, Input, Link, Select, Textarea, Toggle), which retain a focus-visible:ring-* indicator, Switch keyboard users get no visual cue when the control is focused.
♿ Proposed fix to restore a visible focus indicator
const switchRootStyles = cva(
- 'group flex h-3 w-6 items-center rounded-3xl transition-all ease-in-out bg-switch-fill data-[state=checked]:bg-primary disabled:data-[state=checked]:bg-disabled disabled:cursor-not-allowed disabled:bg-disabled outline-none focus:outline-none focus-visible:outline-none focus:ring-0 focus-visible:ring-0'
+ 'group flex h-3 w-6 items-center rounded-3xl transition-all ease-in-out bg-switch-fill data-[state=checked]:bg-primary disabled:data-[state=checked]:bg-disabled disabled:cursor-not-allowed disabled:bg-disabled outline-none focus-visible:ring-2 focus-visible:ring-secondary focus-visible:ring-offset-2'
);📝 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.
| 'group flex h-3 w-6 items-center rounded-3xl transition-all ease-in-out bg-switch-fill data-[state=checked]:bg-primary disabled:data-[state=checked]:bg-disabled disabled:cursor-not-allowed disabled:bg-disabled outline-none focus:outline-none focus-visible:outline-none focus:ring-0 focus-visible:ring-0' | |
| ); | |
| const switchThumbStyles = cva( | |
| 'h-2.5 w-2.5 translate-x-px rounded-full shadow-outer-1 transition-all ease-in-out bg-neutral-50 data-[state=checked]:translate-x-[13px] disabled:bg-neutral-400 group-data-[disabled]:bg-neutral-400 group-data-[disabled]:data-[state=checked]:bg-neutral-400' | |
| 'h-2.5 w-2.5 translate-x-px rounded-full shadow-outer-1 transition-all ease-in-out bg-neutral-50 data-[state=checked]:translate-x-[13px] disabled:bg-neutral-400 group-data-[disabled]:bg-neutral-400 group-data-[disabled]:data-[state=checked]:bg-neutral-400 focus-visible:outline-none focus-visible:ring-0' | |
| 'group flex h-3 w-6 items-center rounded-3xl transition-all ease-in-out bg-switch-fill data-[state=checked]:bg-primary disabled:data-[state=checked]:bg-disabled disabled:cursor-not-allowed disabled:bg-disabled outline-none focus-visible:ring-2 focus-visible:ring-secondary focus-visible:ring-offset-2' | |
| ); | |
| const switchThumbStyles = cva( | |
| 'h-2.5 w-2.5 translate-x-px rounded-full shadow-outer-1 transition-all ease-in-out bg-neutral-50 data-[state=checked]:translate-x-[13px] disabled:bg-neutral-400 group-data-[disabled]:bg-neutral-400 group-data-[disabled]:data-[state=checked]:bg-neutral-400 focus-visible:outline-none focus-visible:ring-0' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/doodle-ui/src/components/Switch/Switch.tsx` around lines
27 - 31, The Switch component’s focus styles remove the keyboard-focus
indicator, so restore a visible focus state in both switchRootStyles and
switchThumbStyles in Switch.tsx. Remove the focus-reset classes that suppress
the ring and add a focus-visible indicator consistent with the other primitives
in this revert, while keeping the disabled styling intact.
| // MAIN | ||
| '--primary': light.primary.main, | ||
| '--primary-variant': light.primary.variant, | ||
| '--secondary': light.secondary.main /* failsafe until useTheme is removed.*/, | ||
| '--secondary-main': light.secondary.main, | ||
| '--secondary-variant': light.secondary.variant, | ||
| // '--tertiary': light.tertiary.main, /* failsafe until useTheme is removed.*/ | ||
| // '--tertiary-main': light.tertiary.main, | ||
| // '--tertiary': light.tertiary.main, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Dark theme drops --secondary-main, breaking secondary color token in dark mode.
Light :root defines '--secondary-main': light.secondary.main (Line 49), but the .dark block replaces it with '--secondary': dark.secondary.main (Line 234) — the variable name changed instead of just the value. Since preset.ts consumes secondary: 'var(--secondary-main)' (see packages/javascript/doodle-ui/src/tailwind/preset.ts Line 36), and CSS custom properties not redefined in a scope inherit from their ancestor (:root), --secondary-main under .dark will fall back to the light-theme value instead of dark.secondary.main. This means dark mode will render the light theme's secondary color for any element using the secondary Tailwind color token.
🐛 Proposed fix
'.dark': {
// MAIN
'--primary': dark.primary.main,
'--primary-variant': dark.primary.variant,
- '--secondary': dark.secondary.main,
+ '--secondary-main': dark.secondary.main,
'--secondary-variant': dark.secondary.variant,Also applies to: 231-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts` around lines 46 - 51,
The dark theme block in plugin.ts is redefining the wrong CSS custom property,
so the Tailwind `secondary` token from preset.ts still resolves to the light
value. Update the `.dark` theme variables to keep the same `--secondary-main`
symbol used in `:root` and `preset.ts`, and assign it the dark secondary value
instead of introducing `--secondary`; check the dark theme variable list around
the `light`/`dark` theme definitions and ensure the `secondary` token name stays
consistent.
| '--secondary-variant': light.secondary.variant, | ||
| // '--tertiary': light.tertiary.main, /* failsafe until useTheme is removed.*/ | ||
| // '--tertiary-main': light.tertiary.main, | ||
| // '--tertiary': light.tertiary.main, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Dark theme has no --tertiary fallback.
Both light (Line 51) and dark (Line 236) comment out --tertiary in the MAIN section. Light's :root legacy block later redefines --tertiary: '#02c577' unconditionally, but the .dark legacy block (Lines 377-390) never redefines --tertiary. Since preset.ts maps tertiary: 'var(--tertiary)', dark mode will inherit the light-theme hardcoded value from :root rather than a dark-specific tertiary color. This currently produces the same value in both themes only because the light legacy value is hardcoded, but it's fragile — any future change to that legacy value silently leaks into dark mode too.
Also applies to: 236-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/doodle-ui/src/tailwind/plugin.ts` at line 51, The dark
theme legacy variables are missing a `--tertiary` override, so `preset.ts`’s
`tertiary: 'var(--tertiary)'` always falls back to the light hardcoded value
from the `:root` legacy block. Update the `.dark` legacy block in `plugin.ts` to
define `--tertiary` with the dark theme’s tertiary color, matching the existing
pattern used for other theme variables and keeping the `MAIN` section comments
consistent.
| primary: 'var(--primary)', | ||
| 'primary-variant': 'var(--primary-variant)', | ||
|
|
||
| secondary: 'var(--secondary-main)', | ||
| 'secondary-main': | ||
| 'var(--secondary-main)' /* failsafe for any classes possibly using the optional '-main' suffix*/, | ||
| 'secondary-variant': 'var(--secondary-variant)', | ||
| 'secondary-variant-2': 'var(--secondary-variant-2)', | ||
|
|
||
| tertiary: 'var(--tertiary-main)', | ||
| 'tertiary-main': | ||
| 'var(--tertiary-main)' /* failsafe for any classes possibly using the optional '-main' suffix*/, | ||
| tertiary: 'var(--tertiary)', |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
secondary token references a variable that's inconsistently named across themes.
This relies on var(--secondary-main), but as flagged in plugin.ts, the .dark block defines --secondary instead of --secondary-main, so this token will resolve to the light-theme color when the dark theme is active. See the corresponding comment on packages/javascript/doodle-ui/src/tailwind/plugin.ts Lines 231-236 for the root cause.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/javascript/doodle-ui/src/tailwind/preset.ts` around lines 33 - 40,
The secondary color token is mapped to a CSS variable name that does not match
the dark theme definition, so it can resolve incorrectly when themes switch.
Update the token in the Tailwind preset’s color map (the secondary entry in
preset.ts) to use the same variable name defined by the theme plugin’s `.dark`
block, and make sure the naming is consistent across preset.ts and plugin.ts for
the secondary token.
Description
Describe your changes in detail
Focus state contained regressions - BED-7215
✅ 113c23e
✅ 5284e43
(Skipped - Merge of all changes) e62f862
✅ 4e6deb0
(Skipped - plugin) 140b690
✅ ac5f73d
✅ 7b36915
✅ 2949c9c
✅ b4fd43b
(Skipped - test fix) fce6f91
✅ b26ffd4
Motivation and Context
Resolves BED-8864
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Style