feat(desktop): configurable light/dark themes for system mode#2557
Merged
Kitenite merged 4 commits intoMar 26, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-mode system theme preferences: two new fields ( Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as SystemThemeCard
participant Section as ThemeSection
participant Store as ThemeStore
participant Persist as Persistence
User->>UI: select system light/dark mapping
UI->>Section: onSystemThemePreferenceChange(mode, themeId)
Section->>Store: setSystemThemePreference(mode, themeId)
Store->>Store: update systemLightThemeId/systemDarkThemeId
Store->>Store: re-resolve active theme (OS mode + system IDs)
Store->>Persist: persist updated theme state
Persist-->>Store: persisted
Store-->>Section: state change via hook
Section-->>UI: re-render with resolved themes
UI-->>User: updated preview/controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsx:165">
P2: System theme selectors include themes with optional/missing `terminal` colors, but the card returns `null` if either selected theme lacks `terminal`, causing the entire System card to disappear.</violation>
</file>
<file name="apps/desktop/src/renderer/stores/theme/store.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/theme/store.ts:275">
P2: Deleting a custom theme used by system mode does not immediately re-resolve and apply the fallback theme, leaving stale active theme state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/theme/store.ts (1)
312-325: Consider cleaning up the media query listener on store destruction.The
addEventListenerfor OS theme preference changes is set up but never removed. While this is typically fine for a singleton store that lives for the app's lifetime, adding a cleanup mechanism would be more robust, especially for testing scenarios.♻️ Optional: Store cleanup reference
+ // Store reference for potential cleanup + let mediaQueryCleanup: (() => void) | null = null; + // Set up listener for OS theme preference changes if (typeof window !== "undefined") { const mediaQuery = window.matchMedia( "(prefers-color-scheme: dark)", ); const handleChange = () => { const currentState = get(); // Only update if system theme is selected if (currentState.activeThemeId === SYSTEM_THEME_ID) { currentState.setTheme(SYSTEM_THEME_ID); } }; mediaQuery.addEventListener("change", handleChange); + mediaQueryCleanup = () => mediaQuery.removeEventListener("change", handleChange); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/theme/store.ts` around lines 312 - 325, The mediaQuery change listener added in the store (window.matchMedia("(prefers-color-scheme: dark)") with addEventListener and handleChange) is never removed; modify the store to keep references to mediaQuery and handleChange and remove the listener on store teardown (e.g., implement an unsubscribe/cleanup method returned from the store factory or a destroy method that calls mediaQuery.removeEventListener("change", handleChange)), ensuring get(), SYSTEM_THEME_ID, and setTheme logic remain 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
`@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeSection/ThemeSection.tsx`:
- Around line 46-49: The fallbacks for systemLightTheme and systemDarkTheme
currently use defaultLightTheme/defaultDarkTheme which lack terminal colors and
make SystemThemeCard return null; update the fallback logic in the ThemeSection
where systemLightTheme and systemDarkTheme are computed to instead (1) try to
find the theme by id in allThemes, (2) if not found, select the first theme in
allThemes with the matching type (light/dark), and (3) if still not found, fall
back to a builtInThemes entry that includes terminal colors (e.g., from
builtInThemes filtered by id or type) so SystemThemeCard always receives a theme
object with terminal properties. Ensure you reference and adjust the variables
systemLightTheme, systemDarkTheme, allThemes,
defaultLightTheme/defaultDarkTheme, and builtInThemes when implementing this
change.
---
Nitpick comments:
In `@apps/desktop/src/renderer/stores/theme/store.ts`:
- Around line 312-325: The mediaQuery change listener added in the store
(window.matchMedia("(prefers-color-scheme: dark)") with addEventListener and
handleChange) is never removed; modify the store to keep references to
mediaQuery and handleChange and remove the listener on store teardown (e.g.,
implement an unsubscribe/cleanup method returned from the store factory or a
destroy method that calls mediaQuery.removeEventListener("change",
handleChange)), ensuring get(), SYSTEM_THEME_ID, and setTheme logic remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d071061-5d27-4e1e-85b0-f7e448a3f4ac
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/main/lib/app-state/schemas.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeSection/ThemeSection.tsxapps/desktop/src/renderer/stores/theme/index.tsapps/desktop/src/renderer/stores/theme/store.ts
Roshvan
force-pushed
the
Roshvan/light-mode-dark-mode-choice
branch
from
6d3b3f0 to
e6779ce
Compare
Contributor
Author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/theme/store.ts (1)
173-199: Add regression coverage for the new system-theme branches.These paths now own the core behaviors in this feature: re-resolving the active theme for the current OS mode, resetting deleted custom preferences, and restoring those preferences after hydration. A small co-located store test around those cases would make this much safer to evolve.
As per coding guidelines, "
**/*.{ts,tsx}: Co-locate tests with their implementation files (e.g.,Component.test.tsxnext toComponent.tsx)."Also applies to: 257-298, 352-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/theme/store.ts` around lines 173 - 199, Add co-located unit tests for the new system-theme branches in the store (tests next to store.ts) that exercise setSystemThemePreference and the re-resolve logic when activeThemeId === SYSTEM_THEME_ID: mock/construct store state with systemLightThemeId/systemDarkThemeId and customThemes, change light and dark preferences, verify resolveThemeId is invoked (via expected resolvedId), that findTheme returns the theme and applyTheme produces terminalTheme, and that set updates activeTheme and terminalTheme; include cases for resetting deleted custom preferences and restoring preferences after hydration to cover the other branches mentioned (also add tests around the other ranges referenced: 257-298 and 352-360).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/stores/theme/store.ts`:
- Around line 76-84: resolveThemeId currently trusts
systemLightThemeId/systemDarkThemeId verbatim which can reference deleted or
missing themes; validate those IDs against the store's available themes (e.g.,
the themes list/map used by initializeTheme) and if an ID is not found replace
it with the known built-in light/dark fallback IDs before using them; apply the
same validation/fallback logic in initializeTheme (and the other block noted
around lines 316-330) so the store never silently drops System mode — use
resolveThemeId and initializeTheme to perform the check against the themes
registry and substitute the built-in light/dark IDs when necessary.
---
Nitpick comments:
In `@apps/desktop/src/renderer/stores/theme/store.ts`:
- Around line 173-199: Add co-located unit tests for the new system-theme
branches in the store (tests next to store.ts) that exercise
setSystemThemePreference and the re-resolve logic when activeThemeId ===
SYSTEM_THEME_ID: mock/construct store state with
systemLightThemeId/systemDarkThemeId and customThemes, change light and dark
preferences, verify resolveThemeId is invoked (via expected resolvedId), that
findTheme returns the theme and applyTheme produces terminalTheme, and that set
updates activeTheme and terminalTheme; include cases for resetting deleted
custom preferences and restoring preferences after hydration to cover the other
branches mentioned (also add tests around the other ranges referenced: 257-298
and 352-360).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f895d46a-b743-489f-82d1-dbdf133cd8f7
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/main/lib/app-state/schemas.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeSection/ThemeSection.tsxapps/desktop/src/renderer/stores/theme/index.tsapps/desktop/src/renderer/stores/theme/store.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/lib/trpc/routers/ui-state/index.ts
- apps/desktop/src/renderer/stores/theme/index.ts
- apps/desktop/src/main/lib/app-state/schemas.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsx
Contributor
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 `@apps/desktop/src/renderer/stores/theme/store.ts`:
- Around line 182-207: setSystemThemePreference currently accepts and persists
any themeId (risking invalid state); validate the incoming themeId against the
store's available/custom themes before calling set. Inside
setSystemThemePreference, use get() to access state.customThemes (or whichever
canonical list you maintain), check that the provided themeId exists (e.g.,
findTheme(themeId, state.customThemes) or equivalent) and only call set(updates)
when valid; if invalid, skip persisting (or return/error) and do not attempt to
re-resolve SYSTEM_THEME_ID or call resolveThemeId/findTheme/applyTheme with
invalid IDs. Ensure you still re-resolve and apply the theme only using
validated newLightId/newDarkId values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6849e173-34cf-45f4-8c6b-6c0e3b70eac2
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsxapps/desktop/src/renderer/stores/theme/store.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/theme/store.ts (2)
182-215: Please add co-located tests for the new system preference flows.This change adds multiple critical branches (validation, stale normalization, deletion resets, runtime re-resolution). A co-located store test file would lock behavior and prevent regressions.
As per coding guidelines:
**/*.{ts,tsx}: Co-locate tests with their implementation files (e.g.,Component.test.tsxnext toComponent.tsx).Also applies to: 274-317, 332-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/theme/store.ts` around lines 182 - 215, Add co-located unit tests next to store.ts that exercise setSystemThemePreference and the related flows: validation (rejecting SYSTEM_THEME_ID or missing findTheme), updating systemLightThemeId/systemDarkThemeId, behavior when activeThemeId === SYSTEM_THEME_ID including stale normalization and re-resolution via resolveThemeId, and confirming applyTheme and set are called to update activeTheme/terminalTheme; write tests that mock or provide customThemes and assert state changes for all branches (validation early return, light update, dark update, and runtime re-resolution using findTheme, resolveThemeId, and applyTheme) to prevent regressions.
86-87: Extract built-in system fallback IDs into shared constants.
"light"/"dark"are repeated in several branches; centralizing them (e.g.,SYSTEM_LIGHT_FALLBACK_ID,SYSTEM_DARK_FALLBACK_ID) would reduce drift risk during future refactors.Also applies to: 153-154, 285-290, 346-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/theme/store.ts` around lines 86 - 87, The code repeats literal theme IDs "light" and "dark" (e.g., where fallbackId is set and other branches in store.ts) — extract these into shared constants (for example SYSTEM_LIGHT_FALLBACK_ID and SYSTEM_DARK_FALLBACK_ID) and replace all literal occurrences with those constants (update places referenced around the fallbackId assignment and the other branches noted at lines ~153-154, ~285-290, ~346-349). Ensure the constants are exported from a central module or the same file if intended for local use, update imports/usages (e.g., where fallbackId, system preference handling, and any map/lookup use the literals), and run tests/build to confirm no missing references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/stores/theme/store.ts`:
- Around line 182-215: Add co-located unit tests next to store.ts that exercise
setSystemThemePreference and the related flows: validation (rejecting
SYSTEM_THEME_ID or missing findTheme), updating
systemLightThemeId/systemDarkThemeId, behavior when activeThemeId ===
SYSTEM_THEME_ID including stale normalization and re-resolution via
resolveThemeId, and confirming applyTheme and set are called to update
activeTheme/terminalTheme; write tests that mock or provide customThemes and
assert state changes for all branches (validation early return, light update,
dark update, and runtime re-resolution using findTheme, resolveThemeId, and
applyTheme) to prevent regressions.
- Around line 86-87: The code repeats literal theme IDs "light" and "dark"
(e.g., where fallbackId is set and other branches in store.ts) — extract these
into shared constants (for example SYSTEM_LIGHT_FALLBACK_ID and
SYSTEM_DARK_FALLBACK_ID) and replace all literal occurrences with those
constants (update places referenced around the fallbackId assignment and the
other branches noted at lines ~153-154, ~285-290, ~346-349). Ensure the
constants are exported from a central module or the same file if intended for
local use, update imports/usages (e.g., where fallbackId, system preference
handling, and any map/lookup use the literals), and run tests/build to confirm
no missing references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4682119a-51b6-4951-9ebf-97f73eb00645
📒 Files selected for processing (1)
apps/desktop/src/renderer/stores/theme/store.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/theme/store.ts (1)
188-221: Consider batching store updates insetSystemThemePreference.When system mode is active, this path can call
set(...)twice. Combining into one write avoids extra subscriber notifications/rerenders.♻️ Suggested batching refactor
setSystemThemePreference: (mode: "light" | "dark", themeId: string) => { const state = get(); if ( themeId === SYSTEM_THEME_ID || !findTheme(themeId, state.customThemes) ) { return; } const updates = mode === "light" ? { systemLightThemeId: themeId } : { systemDarkThemeId: themeId }; - set(updates); - - // Re-resolve if system theme is currently active - if (state.activeThemeId === SYSTEM_THEME_ID) { + // Re-resolve if system theme is currently active + if (state.activeThemeId === SYSTEM_THEME_ID) { const newLightId = mode === "light" ? themeId : state.systemLightThemeId; const newDarkId = mode === "dark" ? themeId : state.systemDarkThemeId; const resolvedId = resolveThemeId( SYSTEM_THEME_ID, newLightId, newDarkId, state.customThemes, ); const theme = findTheme(resolvedId, state.customThemes); if (theme) { const { terminalTheme } = applyTheme(theme); - set({ activeTheme: theme, terminalTheme }); + set({ ...updates, activeTheme: theme, terminalTheme }); + return; } } + set(updates); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/theme/store.ts` around lines 188 - 221, setSystemThemePreference currently calls set(...) twice when the system theme is active (once to update systemLightThemeId/systemDarkThemeId and again to set activeTheme/terminalTheme), causing extra subscriber notifications; change it to compute the new state first (derive newLightId/newDarkId, call resolveThemeId and applyTheme) and then call set once with a combined update object that includes the systemLightThemeId/systemDarkThemeId fields plus activeTheme and terminalTheme as needed; update references inside setSystemThemePreference (get, set, SYSTEM_THEME_ID, systemLightThemeId, systemDarkThemeId, resolveThemeId, applyTheme, findTheme) so the single set merges both the preference change and any re-resolved activeTheme in one write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/stores/theme/store.ts`:
- Around line 188-221: setSystemThemePreference currently calls set(...) twice
when the system theme is active (once to update
systemLightThemeId/systemDarkThemeId and again to set
activeTheme/terminalTheme), causing extra subscriber notifications; change it to
compute the new state first (derive newLightId/newDarkId, call resolveThemeId
and applyTheme) and then call set once with a combined update object that
includes the systemLightThemeId/systemDarkThemeId fields plus activeTheme and
terminalTheme as needed; update references inside setSystemThemePreference (get,
set, SYSTEM_THEME_ID, systemLightThemeId, systemDarkThemeId, resolveThemeId,
applyTheme, findTheme) so the single set merges both the preference change and
any re-resolved activeTheme in one write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40ea7a98-7f1b-434d-975a-f0880bab55ec
📒 Files selected for processing (1)
apps/desktop/src/renderer/stores/theme/store.ts
Roshvan
force-pushed
the
Roshvan/light-mode-dark-mode-choice
branch
2 times, most recently
from
0bb0652 to
f5e77d8
Compare
Contributor
Author
|
@Kitenite what did we want to do with this plan to merge at some point or should I close? |
Collaborator
|
I think we could probably merge it. Is there a preference between this and the other implementation? |
Contributor
Author
Seem similar implementations I have not pulled down and tested or seen what the ui looks like on his implementation I don't mind either |
Contributor
Author
|
@AviPeltz want to take a look here? |
When "System" theme is selected, users can now choose which specific theme to use for light and dark modes via dropdowns on the System card, instead of always resolving to the built-in light/dark themes.
…or fallbacks resolveThemeId now validates that persisted system theme IDs reference existing themes, falling back to built-in light/dark when stale. initializeTheme normalizes stale IDs on hydration so system mode is never silently dropped. SystemThemeCard uses getTerminalColors() for guaranteed terminal colors instead of accessing the optional .terminal property directly.
…sisting Reject invalid or recursive "system" IDs to prevent inconsistent persisted state from non-UI callers.
Replace repeated "light"/"dark" literals with DEFAULT_LIGHT_THEME_ID and DEFAULT_DARK_THEME_ID to reduce drift risk during future refactors.
Roshvan
force-pushed
the
Roshvan/light-mode-dark-mode-choice
branch
from
f5e77d8 to
6bfcd3c
Compare
Collaborator
|
Let's ball! |
Contributor
Author
Love it LOL |
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
2 participants
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.
Summary
When "System" theme is selected, users can now choose which specific theme to use for light and dark OS modes via dropdowns on the System card, instead of always resolving to the built-in light/dark themes.
Changes
systemLightThemeId/systemDarkThemeIdto theme state (persistence schema, tRPC schema, zustand store)resolveThemeIdto use user-configured preferences instead of hardcoded "light"/"dark"<Select>dropdowns (all themes listed) when selectedTest plan
Summary by cubic
Adds configurable theme mapping for System mode so users can pick which themes apply in OS light and dark modes. Includes validation and fallbacks to keep System stable across restarts, deletions, and bad inputs.
New Features
systemLightThemeIdandsystemDarkThemeIdto app state, tRPC schema, and persistence; defaults tolight/dark.systemnow resolves to these IDs based on OS preference and re-resolves on OS changes, preference updates, deletions, and app start.@superset/ui/selectdropdowns for Light and Dark; previews usegetTerminalColors. Hooks:useSystemLightThemeId,useSystemDarkThemeId,useSetSystemThemePreference.Bug Fixes
light/dark; normalize on hydration so System stays active.systemIDs insetSystemThemePreference; ignore bad inputs from non-UI callers.DEFAULT_LIGHT_THEME_ID/DEFAULT_DARK_THEME_IDconstants.Written for commit 6bfcd3c. Summary will update on new commits.
Summary by CodeRabbit