Additional design revisions to dark mode theme - #43634
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43634 +/- ##
==========================================
- Coverage 66.91% 66.90% -0.01%
==========================================
Files 2600 2601 +1
Lines 208361 208423 +62
Branches 9283 9315 +32
==========================================
+ Hits 139415 139454 +39
- Misses 56273 56296 +23
Partials 12673 12673
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lukeheath
changed the title
lukeheath
commented
Apr 21, 2026
|
|
||
| body.dark-mode .default-fleet-logo, | ||
| body.dark-mode .org-logo-icon--invert-dark { | ||
| filter: brightness(0) invert(1); |
Member
Author
There was a problem hiding this comment.
No longer forcing logo white in dark mode.
lukeheath
commented
Apr 21, 2026
| // should wrap their routed content in this class with `key={location.pathname}` | ||
| // so the element remounts on tab change and re-triggers the animation. | ||
| .tab-nav-routed-content { | ||
| animation: fade-in 250ms ease-out; |
Member
Author
There was a problem hiding this comment.
This applies the same page load transition to tabbed nav.
lukeheath
commented
Apr 21, 2026
|
|
||
| &__card-icon { | ||
| body.dark-mode & { | ||
| opacity: 0.75; |
Member
Author
There was a problem hiding this comment.
Don't fade icons in dark mode.
lukeheath
commented
Apr 21, 2026
| vulnFilters: softwareVulnFilters, | ||
| onAddFiltersClick: toggleSoftwareFiltersModal, | ||
| })} | ||
| <div key={location?.pathname} className="tab-nav-routed-content"> |
Member
Author
There was a problem hiding this comment.
Wrapping so we can target the transition.
lukeheath
commented
Apr 21, 2026
| background-color: #111214; | ||
| } | ||
|
|
||
| body.dark-mode .data-table-block thead, |
Member
Author
There was a problem hiding this comment.
These are the targeted dark mode-specific overrides that were surfaced by Mike and in the issue.
lukeheath
commented
Apr 21, 2026
|
|
||
| export const isDarkMode = (): boolean => { | ||
| return localStorage.getItem(THEME_KEY) === "true"; | ||
| const systemPrefersDark = (): boolean => { |
Member
Author
There was a problem hiding this comment.
The logic in place in this file is:
- On first load, set the Fleet UI theme to whatever the system default is.
- If the system default changes, change the Fleet theme.
- Unless they go to the "My profile" page and manually toggle the theme. In that case, we store the preference in localStorage and will always default to their selection.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the frontend dark mode experience by improving theming behavior (including OS preference following), polishing dark-mode styling across common UI elements, and reducing spinner “flash” by introducing a built-in spinner delay (with corresponding test updates).
Changes:
- Update theme initialization/toggling to respect system
prefers-color-schemewhen no explicit user preference is stored and emit a global theme-change event. - Introduce anti-flash spinner delay behavior (default 250ms) and update tests/components to account for delayed rendering.
- Apply dark-mode styling/token improvements across nav, tables, dropdowns, flash toasts, and multiple icons/components.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/utilities/theme.ts | System preference fallback + live OS theme change listener + centralized DOM apply helper. |
| frontend/styles/var/colors.ts | Add static (un-themed) color tokens for always-colored surfaces. |
| frontend/styles/var/colors.scss | Add nav underline token; refine dark-mode palette; add new SCSS aliases/tokens. |
| frontend/styles/global/_global.scss | Dark-mode global overrides for tables, nav hover, react-select, checkboxes, etc. |
| frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tests.tsx | Make assertions resilient to async rendering. |
| frontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tests.tsx | Adjust spinner test for delayed spinner rendering. |
| frontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdatesCard.tests.tsx | Adjust spinner test for delayed spinner rendering. |
| frontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdateSoftwareItem/UpdateSoftwareItem.tsx | Ensure “pending/updating” status spinners show immediately (delay={0}). |
| frontend/pages/hosts/details/cards/Software/SelfService/components/TileActionStatus/TileActionStatus.tsx | Ensure action status spinner shows immediately (delay={0}). |
| frontend/pages/hosts/details/cards/Software/SelfService/SelfServiceCard/SelfServiceCard.tests.tsx | Adjust spinner test for delayed spinner rendering. |
| frontend/pages/hosts/details/cards/Software/InstallStatusCell/InstallStatusCell.tsx | Ensure pending status spinner shows immediately (delay={0}). |
| frontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/TeamDetailsWrapper.tsx | Wrap routed tab content to re-trigger fade-in animation on route change. |
| frontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tests.tsx | Adjust spinner test for delayed spinner rendering. |
| frontend/pages/admin/AdminWrapper.tsx | Wrap routed tab content to re-trigger fade-in animation on route change. |
| frontend/pages/SoftwarePage/SoftwarePage.tsx | Wrap routed tab content (and cloned children) to re-trigger fade-in animation on route change. |
| frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareAddPage.tsx | Wrap routed tab content (and cloned children) to re-trigger fade-in animation on route change. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/RunScript/RunScript.tests.tsx | Update expectations for spinner anti-flash behavior + async rendering. |
| frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx | Use async queries for inputs that may render after loading completes. |
| frontend/pages/ManageControlsPage/ManageControlsPage.tsx | Wrap routed tab content (and cloned children) to re-trigger fade-in animation on route change. |
| frontend/pages/DashboardPage/cards/HostCountCard/_styles.scss | Remove dark-mode icon opacity tweak. |
| frontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsx | Subscribe to fleet-theme-change event to keep UI toggle state in sync. |
| frontend/components/top_nav/SiteTopNav/_styles.scss | Use themed nav-active-underline token. |
| frontend/components/top_nav/SiteTopNav/SiteTopNav.tsx | Remove logo inversion prop usage. |
| frontend/components/icons/TotalHosts.tsx | Make icon color themeable via COLORS and optional color prop. |
| frontend/components/icons/OrgLogoIcon/_styles.scss | Remove dark-mode inversion filter styles. |
| frontend/components/icons/OrgLogoIcon/OrgLogoIcon.jsx | Remove invertDark prop and related class logic. |
| frontend/components/icons/MissingHosts.tsx | Make icon color themeable via COLORS and optional color prop. |
| frontend/components/icons/LowDiskSpaceHosts.tsx | Make icon color themeable via COLORS and optional color prop. |
| frontend/components/icons/Calendar.tsx | Add optional size/color props and use shared sizing/tokens. |
| frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx | Use themed colors for active/hover/focus styling; tweak open-state styling logic. |
| frontend/components/forms/fields/Dropdown/_styles.scss | Override react-select hardcoded colors to keep dropdown themed in dark mode. |
| frontend/components/buttons/Button/Button.tsx | Ensure loading spinner on buttons shows immediately (delay={0}). |
| frontend/components/TargetsInput/_styles.scss | Use themed loading overlay variable. |
| frontend/components/TabNav/_styles.scss | Add routed-content fade-in helper class + themed underline color token. |
| frontend/components/Spinner/Spinner.tsx | Add anti-flash delay behavior (render null until delay elapsed). |
| frontend/components/FlashMessage/_styles.scss | Use static (un-themed) foreground colors for always-colored toast surfaces. |
| frontend/components/FlashMessage/FlashMessage.tsx | Use static icon colors for toast surfaces (esp. warning-filled). |
| frontend/components/AddHostsModal/AddHostsModal.tests.tsx | Adjust spinner test for delayed spinner rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR adds an optional anti-flash Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
frontend/components/Spinner/Spinner.tsx (1)
28-42:⚠️ Potential issue | 🟠 MajorDefault
delay={250}is a behavior change for every existing call site that didn't explicitly opt in.The only site updated to use the new
delayprop isButtoncomponent (withdelay={0}). This means approximately 90+ other<Spinner />usages across the codebase now have a 250ms delay applied without modification. While this prevents flash during quick loads, the component documentation explicitly acknowledges that scenarios representing ongoing work (like pending install/uninstall states) should usedelay={0}for immediate feedback.Either confirm that all remaining call sites are genuinely short-load scenarios where 250ms delay improves UX, or reconsider defaulting
delayto0and explicitly opting into delay where the anti-flash behavior is desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Spinner/Spinner.tsx` around lines 28 - 42, The default delay=250 on the Spinner component (ISpinnerProps / Spinner) changes behavior for all existing call sites; revert the Spinner default to delay = 0 and instead opt individual callers into the delayed anti-flash behavior by passing delay={250} where desired (e.g., update the Button usage to explicitly pass delay={250} or other components that should defer showing the spinner); ensure ISpinnerProps still declares delay?: number and adjust any docs/comments to reflect the new default.
🧹 Nitpick comments (1)
frontend/components/top_nav/SiteTopNav/SiteTopNav.tsx (1)
153-180: Optional: hoist logo URL computation out ofrenderNavItem.
darkLogoURL,lightLogoURL,hasDarkLogo, andorgLogoURLare computed on everyrenderNavItemcall, but only consumed wheniconName === "logo". Consider moving these into auseMemokeyed on[config.org_info, darkMode](or computing inline only within the logo branch) to avoid unnecessary work per nav item and to make the data dependency clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/top_nav/SiteTopNav/SiteTopNav.tsx` around lines 153 - 180, The logo URL computation is being repeated on every renderNavItem call; hoist the logic out of renderNavItem by using a useMemo at the component level (keyed on [config.org_info, darkMode]) to compute darkLogoURL, lightLogoURL, hasDarkLogo and orgLogoURL once, or alternatively move those variable assignments inside the iconName === "logo" branch so they are only computed when needed; update renderNavItem to use the memoized orgLogoURL (or the inline values) and keep the rendering of OrgLogoIcon unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/FlashMessage/_styles.scss`:
- Around line 20-22: Add a blank line before the double-slash comment blocks
that precede the color declaration using $static-white (the comment starting
"Use static (un-themed) white: the flash toast...") and the later comment block
around lines 43–45 so the scss/double-slash-comment-empty-line-before rule is
satisfied; edit the _styles.scss where the color: $static-white; declaration and
the second comment occur and insert one empty line immediately above each `//`
comment.
In `@frontend/components/forms/fields/Dropdown/_styles.scss`:
- Around line 11-13: Add a blank line before each `//` comment block in the
Dropdown styles to satisfy Stylelint's
scss/double-slash-comment-empty-line-before rule; specifically, insert an empty
line immediately above the comment that begins with "// Override react-select's
hardcoded `.Select-value { color: `#aaa`; }`..." (the block that sets color:
$core-fleet-black) and likewise add an empty line before the later comment
around line ~176-177 so each double-slash comment is preceded by a blank line.
In `@frontend/components/icons/TotalHosts.tsx`:
- Around line 14-15: The SVG currently uses static hex values via COLORS[color]
and COLORS["core-fleet-white"] so it won’t respond to dark-mode overrides;
update the TotalHosts component to use CSS custom properties instead (e.g.,
derive fill and background as CSS variable strings rather than hex), replace
usages of fillColor and bgColor with those CSS variable references, and ensure
the SVG elements use those variables (so dark-mode values from colors.scss
apply).
In `@frontend/components/Spinner/Spinner.tsx`:
- Around line 41-51: The spinner can remain hidden if delay changes from >0 to 0
because the useEffect early-returns and visible stays false; in the Spinner
component update the effect (and/or state init) so that when delay === 0 you
call setVisible(true) immediately instead of returning early, and otherwise
always schedule a timeout (const id = setTimeout(() => setVisible(true), delay))
and clear it in the cleanup (clearTimeout(id)); reference useState(visible,
setVisible), useEffect, and the delay prop/ISpinnerProps when applying this
change.
In `@frontend/styles/var/colors.scss`:
- Around line 105-113: The SCSS variable --ui-fleet-black-25 is empty and must
be assigned a valid color to avoid broken dark-mode styles; update the
declaration for --ui-fleet-black-25 in frontend/styles/var/colors.scss (the same
token referenced by $ui-fleet-black-25/var(--ui-fleet-black-25)) to a sensible
mid-dark hex between --ui-fleet-black-33 and --ui-fleet-black-10 (for example,
set it to a hex like `#565A66`) so components using that token render correctly in
dark mode.
In `@frontend/styles/var/colors.ts`:
- Around line 50-53: The color token "static-white" is incorrectly set to
"#e8eaf0" (a light grey-blue) which mismatches its name and reduces contrast for
components like FlashMessage that expect pure white; either rename the token
(e.g., "static-off-white" or "static-foreground-light") and update all callers
(references to "static-white" in FlashMessage/_styles.scss) to the new name, or
change the value of "static-white" to "#FFFFFF" and then re-check WCAG contrast
for toast backgrounds ("$core-vibrant-blue", "$ui-error", "$ui-success"); also
ensure any documentation or usage comments near "static-black" and
"static-white" are updated to reflect the chosen fix.
In `@frontend/utilities/theme.ts`:
- Around line 52-57: The MediaQueryList returned by window.matchMedia may not
support addEventListener, so update the listener registration in initTheme (the
block where you call window.matchMedia("(prefers-color-scheme: dark)") and
reference media) to first check for media.addEventListener and use it if
available, otherwise fall back to media.addListener; ensure the handler still
checks localStorage.getItem(THEME_KEY) and calls applyDarkMode(e.matches, true)
(or the legacy event signature) so behavior remains identical across
environments.
- Around line 12-20: Guard all direct localStorage accesses with try-catch and
fall back to systemPrefersDark() (or no-op on writes). Specifically, wrap the
localStorage.getItem(THEME_KEY) call inside isDarkMode() in a try-catch and
return systemPrefersDark() on any exception; likewise wrap any
localStorage.setItem/removeItem calls used by initTheme and the theme
persistence functions (e.g., setTheme/toggleTheme) so they silently fail (no-op)
when storage is unavailable. Ensure you reference THEME_KEY consistently and do
not change the external behavior except for the safe fallback when storage
throws.
---
Outside diff comments:
In `@frontend/components/Spinner/Spinner.tsx`:
- Around line 28-42: The default delay=250 on the Spinner component
(ISpinnerProps / Spinner) changes behavior for all existing call sites; revert
the Spinner default to delay = 0 and instead opt individual callers into the
delayed anti-flash behavior by passing delay={250} where desired (e.g., update
the Button usage to explicitly pass delay={250} or other components that should
defer showing the spinner); ensure ISpinnerProps still declares delay?: number
and adjust any docs/comments to reflect the new default.
---
Nitpick comments:
In `@frontend/components/top_nav/SiteTopNav/SiteTopNav.tsx`:
- Around line 153-180: The logo URL computation is being repeated on every
renderNavItem call; hoist the logic out of renderNavItem by using a useMemo at
the component level (keyed on [config.org_info, darkMode]) to compute
darkLogoURL, lightLogoURL, hasDarkLogo and orgLogoURL once, or alternatively
move those variable assignments inside the iconName === "logo" branch so they
are only computed when needed; update renderNavItem to use the memoized
orgLogoURL (or the inline values) and keep the rendering of OrgLogoIcon
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20b7077e-a19d-481b-b120-1818fc62b490
📒 Files selected for processing (38)
frontend/components/AddHostsModal/AddHostsModal.tests.tsxfrontend/components/FlashMessage/FlashMessage.tsxfrontend/components/FlashMessage/_styles.scssfrontend/components/Spinner/Spinner.tsxfrontend/components/TabNav/_styles.scssfrontend/components/TargetsInput/_styles.scssfrontend/components/buttons/Button/Button.tsxfrontend/components/forms/fields/Dropdown/_styles.scssfrontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsxfrontend/components/icons/Calendar.tsxfrontend/components/icons/LowDiskSpaceHosts.tsxfrontend/components/icons/MissingHosts.tsxfrontend/components/icons/OrgLogoIcon/OrgLogoIcon.jsxfrontend/components/icons/OrgLogoIcon/_styles.scssfrontend/components/icons/TotalHosts.tsxfrontend/components/top_nav/SiteTopNav/SiteTopNav.tsxfrontend/components/top_nav/SiteTopNav/_styles.scssfrontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsxfrontend/pages/DashboardPage/cards/HostCountCard/_styles.scssfrontend/pages/ManageControlsPage/ManageControlsPage.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/RunScript/RunScript.tests.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareAddPage.tsxfrontend/pages/SoftwarePage/SoftwarePage.tsxfrontend/pages/admin/AdminWrapper.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tests.tsxfrontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/TeamDetailsWrapper.tsxfrontend/pages/hosts/details/cards/Software/InstallStatusCell/InstallStatusCell.tsxfrontend/pages/hosts/details/cards/Software/SelfService/SelfServiceCard/SelfServiceCard.tests.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/TileActionStatus/TileActionStatus.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdateSoftwareItem/UpdateSoftwareItem.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdatesCard.tests.tsxfrontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tests.tsxfrontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tests.tsxfrontend/styles/global/_global.scssfrontend/styles/var/colors.scssfrontend/styles/var/colors.tsfrontend/utilities/theme.ts
💤 Files with no reviewable changes (2)
- frontend/components/icons/OrgLogoIcon/_styles.scss
- frontend/pages/DashboardPage/cards/HostCountCard/_styles.scss
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx (2)
269-276:⚠️ Potential issue | 🟡 MinorBehavior change: placeholder/indicator overrides now gated on
menuIsOpeninstead ofisFocused.Per the PR summary, this block previously applied under
state.isFocusedand a duplicatedstate.menuIsOpenblock was removed. As a result, for non-buttondropdowns, when the control is tab-focused but the menu is closed, the placeholder and indicator no longer receive theui-fleet-black-75override — they'll fall back to whatever color the baseplaceholder/dropdownIndicatorstyles define. Please confirm this is intentional (e.g., matches updated Figma for focus vs. open states); otherwise keep it underisFocusedor add both conditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx` around lines 269 - 276, The recent change moved the placeholder/indicator color override from being applied when state.isFocused to only when state.menuIsOpen, causing tab-focused (but closed) non-button dropdowns to lose the ui-fleet-black-75 styling; update DropdownWrapper.tsx to apply the override when either state.isFocused or state.menuIsOpen is true (i.e., use a combined condition) so the ".dropdown-wrapper__placeholder" and ".dropdown-wrapper__indicator path" rules still get the COLORS["ui-fleet-black-75"] styling on focus as well as when the menu is open.
202-224:⚠️ Potential issue | 🟡 MinorAdd
&prefix to the selector on line 202 to match the correct CSS-in-JS pattern.The selector
.react-select__control--is-focused(line 202) is missing the&prefix and should be&.react-select__control--is-focused. Without it, the styles target descendant elements rather than the control itself, so thebackgroundColorand color overrides won't apply when the control is focused.This is inconsistent with the non-button variant at line 258, which correctly uses
&.react-select__control--is-focused. The bug is partially masked in the button variant because the identical styles are applied viastate.menuIsOpen(line 212), which covers the "menu open" case but not "tab-focused with closed menu."Proposed fix
- ".react-select__control--is-focused": { + "&.react-select__control--is-focused": {Additionally, note that the button variant applies focus-state overrides only when
state.menuIsOpen, whereas the non-button variant applies them whenstate.isFocused. This semantic difference may cause the button dropdown to lack focus-state styling when navigated to via Tab but not yet opened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx` around lines 202 - 224, Update the focused-control selector in DropdownWrapper to use the ampersand prefix so the styles apply to the control itself: change the selector `.react-select__control--is-focused` to `&.react-select__control--is-focused` inside the style object that currently sets backgroundColor, boxShadow, placeholder and indicator colors; also align the focus condition with the non-button variant by using state.isFocused (instead of only state.menuIsOpen) for the button variant so keyboard tab focus receives the same overrides as the open-menu state (adjust the block that currently spreads ...(state.menuIsOpen && { ... }) or add a similar ...(state.isFocused && { ... }) branch as appropriate).
♻️ Duplicate comments (6)
frontend/styles/var/colors.scss (1)
111-111:⚠️ Potential issue | 🟠 MajorFill in the dark-mode
--ui-fleet-black-25token.An empty custom property resolves to an empty
var(...)substitution, so any dark-mode declaration using$ui-fleet-black-25can become invalid at computed-value time.🎨 Proposed fix
- --ui-fleet-black-25: ; + --ui-fleet-black-25: `#555a66`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/styles/var/colors.scss` at line 111, The dark-mode custom property --ui-fleet-black-25 in frontend/styles/var/colors.scss is empty and must be assigned a valid color value; update the declaration to a concrete color (for example rgba(0,0,0,0.25) or a corresponding hex/rgba from your design tokens) so any references to var(--ui-fleet-black-25) resolve correctly and don't produce invalid computed values.frontend/styles/var/colors.ts (1)
50-53:⚠️ Potential issue | 🟡 MinorClarify whether
static-whiteshould be true white or an off-white token.
static-whiteis currently#e8eaf0, which is not white and can subtly reduce foreground contrast for callers expecting a theme-independent white. Either rename it to an off-white/foreground token or set it to#FFFFFFand re-check toast contrast.Possible fixes
- "static-white": "#e8eaf0", + "static-white": "#FFFFFF", "static-black": "#192147",or rename the token and update call sites:
- "static-white": "#e8eaf0", + "static-foreground-light": "#e8eaf0", "static-black": "#192147",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/styles/var/colors.ts` around lines 50 - 53, The token "static-white" currently uses `#e8eaf0` but is misleadingly named; either change the value of the "static-white" token to true white (`#FFFFFF`) and then re-check contrast in toast/tooltip components (update any visual tests), or rename the token to a clear off-white name like "static-offwhite" (or "static-gray-100") and then update all call sites that reference "static-white" to the new token name (search for the "static-white" identifier in the codebase and replace usages in toast/tooltip/foreground styles). Ensure whichever path you choose leaves a semantically correct token name/value and that contrast checks for toasts/tooltips are validated.frontend/components/forms/fields/Dropdown/_styles.scss (1)
11-13:⚠️ Potential issue | 🟡 MinorAdd blank lines before these
//comment blocks.Stylelint still reports
scss/double-slash-comment-empty-line-beforehere. Insert an empty line before the comments on Line 11 and Line 176.Proposed fix
&-label { line-height: 38px; font-size: $x-small; + // Override react-select's hardcoded `.Select-value { color: `#aaa`; }` so // the selected value uses themed text color (esp. needed for dark mode). color: $core-fleet-black; @@ > .Select-control { border-radius: $border-radius; + // Override react-select's hardcoded white background when open. background-color: $core-fleet-white; }Also applies to: 174-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/forms/fields/Dropdown/_styles.scss` around lines 11 - 13, Add a blank line before the double-slash comment blocks that start with "// Override react-select's hardcoded `.Select-value { color: `#aaa`; }`..." and the other similar comment near the bottom (the two occurrences flagged by scss/double-slash-comment-empty-line-before) so Stylelint no longer reports the rule; simply insert an empty line immediately above each of those // comment lines in _styles.scss.frontend/components/FlashMessage/_styles.scss (1)
20-22:⚠️ Potential issue | 🟡 MinorAdd blank lines before these
//comment blocks.Stylelint still reports
scss/double-slash-comment-empty-line-beforehere. Insert an empty line before the comments on Line 20 and Line 43.Proposed fix
.flash-message { display: flex; align-items: center; justify-content: center; + // Use static (un-themed) white: the flash toast is always a colored // surface, so foreground should stay light regardless of dark mode. color: $static-white; @@ &--warning-filled { background-color: $ui-warning; + // Yellow is light enough that foreground should be dark in BOTH modes. // Use static (un-themed) tokens so dark mode doesn't flip to light text. color: $static-black;Also applies to: 41-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/FlashMessage/_styles.scss` around lines 20 - 22, Add a blank line before each double-slash comment block to satisfy scss/double-slash-comment-empty-line-before: insert an empty line immediately above the comment that precedes the color: $static-white; declaration (the "// Use static (un-themed) white..." comment) and likewise add a blank line before the comment block around the other flash toast comments referenced (the block covering the lines near the second occurrence). No other changes—just ensure there is an empty line above each "// ..." comment so stylelint stops reporting the rule.frontend/utilities/theme.ts (2)
52-57:⚠️ Potential issue | 🟠 MajorCheck
MediaQueryList.addEventListenerbefore calling it.
window.matchMediacan exist while the returnedMediaQueryListlacksaddEventListener(older Safari / some test environments using jsdom mocks), which would throw duringinitTheme(). The codebase already uses theaddEventListener/addListenerfallback pattern inuseIsMobileWidth.tsx— mirror it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/utilities/theme.ts` around lines 52 - 57, The MediaQueryList returned from window.matchMedia may not support addEventListener; in the initTheme logic where you create const media = window.matchMedia("(prefers-color-scheme: dark)"), guard the listener registration by checking for media.addEventListener and falling back to media.addListener if absent (and pass the same handler that calls applyDarkMode(e.matches, true) only when localStorage.getItem(THEME_KEY) is null). Update the listener registration around the media variable to use this addEventListener/addListener fallback pattern so older Safari/jsdom environments don't throw.
15-15:⚠️ Potential issue | 🟠 MajorGuard
localStorageaccess to prevent startup crashes.
initTheme()runs before React mounts, andlocalStorage.getItem/setItemcan throw when storage is disabled or unavailable (e.g., strict privacy modes, sandboxed iframes). The unguarded calls at lines 15, 39, and 55 will crash app initialization. Wrap access in try/catch and fall back tosystemPrefersDark()on read failure; no-op on write failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/utilities/theme.ts` at line 15, The unguarded localStorage access in initTheme (e.g., the stored = localStorage.getItem(THEME_KEY) read and subsequent localStorage.setItem writes) can throw and crash startup; wrap reads and writes in try/catch: for reads (getItem) catch errors and treat as "no stored value" so you fall back to calling systemPrefersDark(); for writes (setItem) catch and swallow the error (no-op) so failures to persist do not break initTheme. Update all localStorage touches in initTheme (and any helpers that reference THEME_KEY) to follow this pattern.
🧹 Nitpick comments (2)
frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tests.tsx (1)
737-737: Minor:findByTextis redundant afterwaitForon spinner removal, but harmless.Since the preceding
waitForalready ensures the spinner is gone and the content has rendered,screen.getByTextwould also work. UsingfindByTextis fine and arguably more resilient — no change needed.Also applies to: 782-784
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tests.tsx` at line 737, The test uses await screen.findByText(/Creating a new report for/i) after a waitFor that ensures the spinner is gone, which is redundant; change these assertions to synchronous lookups (screen.getByText(/Creating a new report for/i)) to remove the unnecessary async call — update the specific occurrences of await screen.findByText in EditQueryForm.tests.tsx (including the shown call and the similar instances around the later block) to use screen.getByText (or keep findByText if you prefer the more resilient async form).frontend/utilities/theme.ts (1)
24-35: Overlapping toggles can prematurely strip the transition class.Each
applyDarkMode(_, true)call schedules asetTimeoutthat unconditionally removestheme-transitionafter 300ms. If the OS fires achangeevent (or the user toggles) while an earlier transition is still in flight, the first timer will remove the class mid-transition on the second change. Consider tracking the pending timeout and clearing it before scheduling a new one.♻️ Proposed fix
+let transitionTimeout: ReturnType<typeof setTimeout> | null = null; + const applyDarkMode = (dark: boolean, animate: boolean): void => { if (animate) { document.body.classList.add("theme-transition"); - setTimeout(() => { + if (transitionTimeout !== null) { + clearTimeout(transitionTimeout); + } + transitionTimeout = setTimeout(() => { document.body.classList.remove("theme-transition"); + transitionTimeout = null; }, TRANSITION_MS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/utilities/theme.ts` around lines 24 - 35, applyDarkMode currently schedules a setTimeout that unconditionally removes the "theme-transition" class (using TRANSITION_MS), which allows overlapping calls to clear the class mid-transition; modify applyDarkMode to track the pending timeout (e.g., a module-scoped variable like pendingTransitionTimeout) and clearTimeout(pendingTransitionTimeout) before scheduling a new one so only the most recent timer removes "theme-transition"; keep the existing behavior of adding the class when animate is true, toggling "dark-mode", and dispatching the "fleet-theme-change" CustomEvent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/components/Spinner/Spinner.tsx`:
- Line 42: The Spinner component's signature now returns JSX.Element | null
which breaks callers expecting JSX.Element (e.g., the PolicyDetailsPage
function). Fix by either updating the return type of affected components (e.g.,
change PolicyDetailsPage's declared return from JSX.Element to JSX.Element |
null) or update call sites that directly return <Spinner /> to handle the
nullable case (wrap the call in a null-coalescing fallback or conditional render
so the returned expression is always a JSX.Element). Locate the Spinner
component (ISpinnerProps -> Spinner) and the PolicyDetailsPage function and
apply one consistent approach across callers.
In `@frontend/styles/global/_global.scss`:
- Around line 285-290: In the dark-mode card button hover rules, replace the
themed color variable $core-fleet-white with the unthemed $static-white so hover
backgrounds remain visible; update the selector block that targets
body.dark-mode .card .button--inverse:hover, .button--inverse-alert:hover,
.button--text-icon:hover, and .button--icon:hover to use $static-white for
background-color instead of $core-fleet-white.
---
Outside diff comments:
In `@frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx`:
- Around line 269-276: The recent change moved the placeholder/indicator color
override from being applied when state.isFocused to only when state.menuIsOpen,
causing tab-focused (but closed) non-button dropdowns to lose the
ui-fleet-black-75 styling; update DropdownWrapper.tsx to apply the override when
either state.isFocused or state.menuIsOpen is true (i.e., use a combined
condition) so the ".dropdown-wrapper__placeholder" and
".dropdown-wrapper__indicator path" rules still get the
COLORS["ui-fleet-black-75"] styling on focus as well as when the menu is open.
- Around line 202-224: Update the focused-control selector in DropdownWrapper to
use the ampersand prefix so the styles apply to the control itself: change the
selector `.react-select__control--is-focused` to
`&.react-select__control--is-focused` inside the style object that currently
sets backgroundColor, boxShadow, placeholder and indicator colors; also align
the focus condition with the non-button variant by using state.isFocused
(instead of only state.menuIsOpen) for the button variant so keyboard tab focus
receives the same overrides as the open-menu state (adjust the block that
currently spreads ...(state.menuIsOpen && { ... }) or add a similar
...(state.isFocused && { ... }) branch as appropriate).
---
Duplicate comments:
In `@frontend/components/FlashMessage/_styles.scss`:
- Around line 20-22: Add a blank line before each double-slash comment block to
satisfy scss/double-slash-comment-empty-line-before: insert an empty line
immediately above the comment that precedes the color: $static-white;
declaration (the "// Use static (un-themed) white..." comment) and likewise add
a blank line before the comment block around the other flash toast comments
referenced (the block covering the lines near the second occurrence). No other
changes—just ensure there is an empty line above each "// ..." comment so
stylelint stops reporting the rule.
In `@frontend/components/forms/fields/Dropdown/_styles.scss`:
- Around line 11-13: Add a blank line before the double-slash comment blocks
that start with "// Override react-select's hardcoded `.Select-value { color:
`#aaa`; }`..." and the other similar comment near the bottom (the two occurrences
flagged by scss/double-slash-comment-empty-line-before) so Stylelint no longer
reports the rule; simply insert an empty line immediately above each of those //
comment lines in _styles.scss.
In `@frontend/styles/var/colors.scss`:
- Line 111: The dark-mode custom property --ui-fleet-black-25 in
frontend/styles/var/colors.scss is empty and must be assigned a valid color
value; update the declaration to a concrete color (for example rgba(0,0,0,0.25)
or a corresponding hex/rgba from your design tokens) so any references to
var(--ui-fleet-black-25) resolve correctly and don't produce invalid computed
values.
In `@frontend/styles/var/colors.ts`:
- Around line 50-53: The token "static-white" currently uses `#e8eaf0` but is
misleadingly named; either change the value of the "static-white" token to true
white (`#FFFFFF`) and then re-check contrast in toast/tooltip components (update
any visual tests), or rename the token to a clear off-white name like
"static-offwhite" (or "static-gray-100") and then update all call sites that
reference "static-white" to the new token name (search for the "static-white"
identifier in the codebase and replace usages in toast/tooltip/foreground
styles). Ensure whichever path you choose leaves a semantically correct token
name/value and that contrast checks for toasts/tooltips are validated.
In `@frontend/utilities/theme.ts`:
- Around line 52-57: The MediaQueryList returned from window.matchMedia may not
support addEventListener; in the initTheme logic where you create const media =
window.matchMedia("(prefers-color-scheme: dark)"), guard the listener
registration by checking for media.addEventListener and falling back to
media.addListener if absent (and pass the same handler that calls
applyDarkMode(e.matches, true) only when localStorage.getItem(THEME_KEY) is
null). Update the listener registration around the media variable to use this
addEventListener/addListener fallback pattern so older Safari/jsdom environments
don't throw.
- Line 15: The unguarded localStorage access in initTheme (e.g., the stored =
localStorage.getItem(THEME_KEY) read and subsequent localStorage.setItem writes)
can throw and crash startup; wrap reads and writes in try/catch: for reads
(getItem) catch errors and treat as "no stored value" so you fall back to
calling systemPrefersDark(); for writes (setItem) catch and swallow the error
(no-op) so failures to persist do not break initTheme. Update all localStorage
touches in initTheme (and any helpers that reference THEME_KEY) to follow this
pattern.
---
Nitpick comments:
In
`@frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tests.tsx`:
- Line 737: The test uses await screen.findByText(/Creating a new report for/i)
after a waitFor that ensures the spinner is gone, which is redundant; change
these assertions to synchronous lookups (screen.getByText(/Creating a new report
for/i)) to remove the unnecessary async call — update the specific occurrences
of await screen.findByText in EditQueryForm.tests.tsx (including the shown call
and the similar instances around the later block) to use screen.getByText (or
keep findByText if you prefer the more resilient async form).
In `@frontend/utilities/theme.ts`:
- Around line 24-35: applyDarkMode currently schedules a setTimeout that
unconditionally removes the "theme-transition" class (using TRANSITION_MS),
which allows overlapping calls to clear the class mid-transition; modify
applyDarkMode to track the pending timeout (e.g., a module-scoped variable like
pendingTransitionTimeout) and clearTimeout(pendingTransitionTimeout) before
scheduling a new one so only the most recent timer removes "theme-transition";
keep the existing behavior of adding the class when animate is true, toggling
"dark-mode", and dispatching the "fleet-theme-change" CustomEvent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d027842-5410-4013-ae4e-749940e3c726
📒 Files selected for processing (38)
frontend/components/AddHostsModal/AddHostsModal.tests.tsxfrontend/components/FlashMessage/FlashMessage.tsxfrontend/components/FlashMessage/_styles.scssfrontend/components/Spinner/Spinner.tsxfrontend/components/TabNav/_styles.scssfrontend/components/TargetsInput/_styles.scssfrontend/components/buttons/Button/Button.tsxfrontend/components/forms/fields/Dropdown/_styles.scssfrontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsxfrontend/components/icons/Calendar.tsxfrontend/components/icons/LowDiskSpaceHosts.tsxfrontend/components/icons/MissingHosts.tsxfrontend/components/icons/OrgLogoIcon/OrgLogoIcon.jsxfrontend/components/icons/OrgLogoIcon/_styles.scssfrontend/components/icons/TotalHosts.tsxfrontend/components/top_nav/SiteTopNav/SiteTopNav.tsxfrontend/components/top_nav/SiteTopNav/_styles.scssfrontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsxfrontend/pages/DashboardPage/cards/HostCountCard/_styles.scssfrontend/pages/ManageControlsPage/ManageControlsPage.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/RunScript/RunScript.tests.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareAddPage.tsxfrontend/pages/SoftwarePage/SoftwarePage.tsxfrontend/pages/admin/AdminWrapper.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tests.tsxfrontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/TeamDetailsWrapper.tsxfrontend/pages/hosts/details/cards/Software/InstallStatusCell/InstallStatusCell.tsxfrontend/pages/hosts/details/cards/Software/SelfService/SelfServiceCard/SelfServiceCard.tests.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/TileActionStatus/TileActionStatus.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdateSoftwareItem/UpdateSoftwareItem.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdatesCard.tests.tsxfrontend/pages/hosts/details/modals/MDMStatusModal/MDMStatusModal.tests.tsxfrontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tests.tsxfrontend/styles/global/_global.scssfrontend/styles/var/colors.scssfrontend/styles/var/colors.tsfrontend/utilities/theme.ts
💤 Files with no reviewable changes (2)
- frontend/components/icons/OrgLogoIcon/_styles.scss
- frontend/pages/DashboardPage/cards/HostCountCard/_styles.scss
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx (1)
202-224:⚠️ Potential issue | 🟡 MinorReserve the “down” colors for the open state.
Because
state.isFocusedcan remain true after the menu closes, Line 202 can leave the closed button styled with the active/down placeholder and chevron colors until blur. Keep the focus background, but let the existing placeholder/indicator focus styling use the “over” color and only apply “down” whenmenuIsOpen.🎨 Proposed fix
...(state.isFocused && { backgroundColor: COLORS["ui-fleet-black-5"], boxShadow: "none", - ".dropdown-wrapper__placeholder": { - color: COLORS["ui-fleet-black-75-down"], - }, - ".dropdown-wrapper__indicator path": { - stroke: COLORS["ui-fleet-black-75-down"], - }, }), ...(state.menuIsOpen && { backgroundColor: COLORS["ui-fleet-black-5"], ".dropdown-wrapper__placeholder": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx` around lines 202 - 224, The focus block (state.isFocused) currently applies the “down” colors to .dropdown-wrapper__placeholder and .dropdown-wrapper__indicator path which can persist after the menu closes; change the state.isFocused style to only set the focus backgroundColor (keep boxShadow: "none") and set placeholder/indicator colors to the “over” variant instead, and reserve the “down” color overrides for the state.menuIsOpen block (also keep the .dropdown-wrapper__indicator svg rotation/transition inside menuIsOpen). Target the style objects referenced (state.isFocused, state.menuIsOpen, ".dropdown-wrapper__placeholder", ".dropdown-wrapper__indicator path", ".dropdown-wrapper__indicator svg") in DropdownWrapper.tsx to implement this swap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx`:
- Around line 202-224: The focus block (state.isFocused) currently applies the
“down” colors to .dropdown-wrapper__placeholder and .dropdown-wrapper__indicator
path which can persist after the menu closes; change the state.isFocused style
to only set the focus backgroundColor (keep boxShadow: "none") and set
placeholder/indicator colors to the “over” variant instead, and reserve the
“down” color overrides for the state.menuIsOpen block (also keep the
.dropdown-wrapper__indicator svg rotation/transition inside menuIsOpen). Target
the style objects referenced (state.isFocused, state.menuIsOpen,
".dropdown-wrapper__placeholder", ".dropdown-wrapper__indicator path",
".dropdown-wrapper__indicator svg") in DropdownWrapper.tsx to implement this
swap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29680cab-76e7-4f54-8a80-7438aac03355
📒 Files selected for processing (2)
frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsxfrontend/styles/var/colors.scss
Member
Author
|
@sgress454 Also tagging you if you want to get your review on. |
RachelElysia
approved these changes
Apr 23, 2026
RachelElysia
left a comment
RachelElysia
left a comment
Member
There was a problem hiding this comment.
Thanks for all this! Sorry empty states will not go in 4.85.0. I've had to prioritize on-call + current sprint work this week <3
This was referenced Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design revisions for #42977
Summary by CodeRabbit
New Features
Style
Refactor
Tests