Switch to radio buttons for theme selection#44518
Conversation
![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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughReplaces the binary dark-mode toggle with a tri-state theme picker supporting 🚥 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. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 23 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44518 +/- ##
=======================================
Coverage 66.75% 66.75%
=======================================
Files 2636 2636
Lines 211725 211731 +6
Branches 9556 9559 +3
=======================================
+ Hits 141332 141337 +5
- Misses 57549 57550 +1
Partials 12844 12844
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:
|
There was a problem hiding this comment.
Pull request overview
Updates the frontend theme selection UX from a binary dark-mode toggle to a tri-state theme mode (system/light/dark) backed by a new localStorage key, enabling “follow system” behavior while keeping explicit user selections sticky.
Changes:
- Replace
fleet-dark-modeboolean storage withfleet-thememode storage (system/light/dark) and add legacy-key migration. - Update Account side panel UI to use radio buttons for theme selection.
- Adjust Account side panel styling to support the new radio-based theme picker layout.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/utilities/theme.ts | Introduces tri-state theme mode storage + legacy migration and resolves effective dark mode from mode/system preference. |
| frontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsx | Replaces switch toggle with a radiogroup using shared Radio component, persisting the selected mode. |
| frontend/pages/AccountPage/AccountSidePanel/_styles.scss | Removes toggle styling and adds styles for the new theme picker and label. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const onThemeSelect = (value: string) => { | ||
| const mode = value as ThemeMode; | ||
| setThemeModeState(mode); | ||
| setThemeMode(mode); |
There was a problem hiding this comment.
onThemeSelect uses a type assertion (value as ThemeMode) and then persists that value. Because value is just string, this can silently allow invalid values into component state/localStorage if the handler is ever reused or called with an unexpected value. Consider validating against the allowed set (e.g., value === "system" || value === "light" || value === "dark") before calling setThemeModeState/setThemeMode, or narrowing the handler so it only accepts ThemeMode values.
| const onThemeSelect = (value: string) => { | |
| const mode = value as ThemeMode; | |
| setThemeModeState(mode); | |
| setThemeMode(mode); | |
| const isThemeMode = (value: string): value is ThemeMode => { | |
| return value === "system" || value === "light" || value === "dark"; | |
| }; | |
| const onThemeSelect = (value: string) => { | |
| if (!isThemeMode(value)) { | |
| return; | |
| } | |
| setThemeModeState(value); | |
| setThemeMode(value); |
There was a problem hiding this comment.
We already have isThemeMode in frontend/utilities/theme.ts, we can just export it and use it here. But honestly if this is passing TypeScript checking already I don't think this would add a lot of value unless we plan on throwing an error if we receive an invalid value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utilities/theme.ts`:
- Around line 17-19: readStoredMode currently calls localStorage.getItem
directly which can throw in non-browser or restricted environments; wrap the
localStorage access in a try/catch and/or guard with a runtime check (e.g.,
typeof window !== "undefined" && window.localStorage) so that readStoredMode
returns the safe default "system" on any error or when storage is unavailable;
apply the same pattern to the other functions touching storage (the
setter/remover around THEME_KEY, e.g., setStoredMode and clearStoredMode) so all
localStorage.getItem/setItem/removeItem calls are protected and fall back
gracefully.
🪄 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: d901c316-f452-45ce-b262-e7d6227b1ffb
📒 Files selected for processing (3)
frontend/pages/AccountPage/AccountSidePanel/AccountSidePanel.tsxfrontend/pages/AccountPage/AccountSidePanel/_styles.scssfrontend/utilities/theme.ts
| const readStoredMode = (): ThemeMode => { | ||
| const stored = localStorage.getItem(THEME_KEY); | ||
| if (stored !== null) { | ||
| return stored === "true"; | ||
| } | ||
| return systemPrefersDark(); | ||
| return isThemeMode(stored) ? stored : "system"; |
There was a problem hiding this comment.
Guard localStorage access to prevent runtime crashes.
localStorage.getItem/setItem/removeItem can throw in restricted environments (or non-browser execution), which would break both initialization and theme selection.
Proposed fix
+const getStorage = (): Storage | null => {
+ if (typeof window === "undefined") return null;
+ try {
+ return window.localStorage;
+ } catch {
+ return null;
+ }
+};
+
const readStoredMode = (): ThemeMode => {
- const stored = localStorage.getItem(THEME_KEY);
+ const stored = getStorage()?.getItem(THEME_KEY);
return isThemeMode(stored) ? stored : "system";
};
export const setThemeMode = (mode: ThemeMode): void => {
- if (mode === "system") {
- localStorage.removeItem(THEME_KEY);
- } else {
- localStorage.setItem(THEME_KEY, mode);
+ const storage = getStorage();
+ if (storage) {
+ if (mode === "system") {
+ storage.removeItem(THEME_KEY);
+ } else {
+ storage.setItem(THEME_KEY, mode);
+ }
}
applyDarkMode(resolveDark(mode), true);
};Also applies to: 45-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/utilities/theme.ts` around lines 17 - 19, readStoredMode currently
calls localStorage.getItem directly which can throw in non-browser or restricted
environments; wrap the localStorage access in a try/catch and/or guard with a
runtime check (e.g., typeof window !== "undefined" && window.localStorage) so
that readStoredMode returns the safe default "system" on any error or when
storage is unavailable; apply the same pattern to the other functions touching
storage (the setter/remover around THEME_KEY, e.g., setStoredMode and
clearStoredMode) so all localStorage.getItem/setItem/removeItem calls are
protected and fall back gracefully.
There was a problem hiding this comment.
I don't think this is very high risk, especially "non-browser execution", but it's also cheap so sure
| return isThemeMode(stored) ? stored : "system"; | ||
| }; | ||
|
|
||
| export const getThemeMode = (): ThemeMode => readStoredMode(); |
There was a problem hiding this comment.
I don't follow why we need both the internal readStoredMode() and the exported getThemeMode() that wraps it
| const onThemeSelect = (value: string) => { | ||
| const mode = value as ThemeMode; | ||
| setThemeModeState(mode); | ||
| setThemeMode(mode); |
There was a problem hiding this comment.
We already have isThemeMode in frontend/utilities/theme.ts, we can just export it and use it here. But honestly if this is passing TypeScript checking already I don't think this would add a lot of value unless we plan on throwing an error if we receive an invalid value.
| const readStoredMode = (): ThemeMode => { | ||
| const stored = localStorage.getItem(THEME_KEY); | ||
| if (stored !== null) { | ||
| return stored === "true"; | ||
| } | ||
| return systemPrefersDark(); | ||
| return isThemeMode(stored) ? stored : "system"; |
There was a problem hiding this comment.
I don't think this is very high risk, especially "non-browser execution", but it's also cheap so sure
Summary by CodeRabbit
New Features
Style