feat: follow system light/dark appearance with per-mode theme selection - #650
feat: follow system light/dark appearance with per-mode theme selection#650iamthenuggetman wants to merge 7 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| // If the deleted theme was active, switch to default | ||
| if (currentTheme.id === themeId) { | ||
| const defaultTheme = themeRegistry.getDefault(); |
There was a problem hiding this comment.
WARNING: Deleting the active per-mode theme in follow-system mode switches to getDefault() (always dark), ignoring the current OS mode.
When follow-system is enabled and the deleted custom theme was the active system-matched pick, the reset block above correctly restores lightThemeId/darkThemeId to the mode-correct preset, but this branch then falls back to themeRegistry.getDefault() regardless of OS. If the OS is currently in light mode, the app ends up showing the dark default and stays there until the OS appearance changes (the matchMedia listener only re-resolves on an OS change, not on this settings/currentTheme update). Consider resolving the replacement through the current OS mode and the just-reset picks (e.g. via getSystemThemeId(window.matchMedia("(prefers-color-scheme: dark)").matches, { lightThemeId, darkThemeId, ... })) instead of getDefault().
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of commit Files Reviewed (1 file)
Previous Review Summaries (2 snapshots, latest commit c721476)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c721476)Status: No Issues Found | Recommendation: Merge The previously reported WARNING (deleting an active per-mode theme in follow-system mode falling back to Files Reviewed (2 files)
Previous review (commit b9809a7)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 substantive files)
i18n locale additions (11 files) and test files (3 files) reviewed for consistency; no issues. Reviewed by glm-5.2 · Input: 67.1K · Output: 4.9K · Cached: 216.1K |
|
To use Kilo from GitHub you first need to link your GitHub account to Kilo. Link your Kilo account to continue. After linking, mention me again in this issue or pull request. |
…low-system mode Kilo Code review (TabularisDB#650): in follow-system mode, deleting the active per-mode pick left the app stuck on themeRegistry.getDefault() (always dark) until the OS appearance changed. Resolve the replacement through the current OS mode and the just-reset per-mode picks, mirroring the listener and load path. Static mode keeps the existing getDefault() fallback. - ThemeProvider.tsx deleteCustomTheme active branch now branches on settings.followSystemTheme; follow-system path uses getSystemThemeId + preset fallback by current OS mode. - New test in system theme sync suite: deletes the active per-mode pick in follow-system mode (OS light) and asserts the replacement is the light preset, not getDefault().
|
Addressed the warning at In follow-system mode, deleting the active per-mode pick now resolves the replacement through the current OS mode and the just-reset per-mode picks — same fallback chain the listener and load path already use. Static mode keeps New test in the system-theme-sync suite: deletes the active per-mode pick in follow-system mode (OS light, |
CI build failed: themeRegistry.getPreset returns Theme | undefined, and the find() || getPreset() chain can resolve to undefined. tsc -b rejects assigning to a Theme-typed binding. Add themeRegistry.getDefault() as the final fallback so the assignment is non-undefined, matching the load-path fallback chain.
1 participant
Closes #649
Summary
Users can now pick separate light and dark themes; in Follow System mode the app watches the OS appearance (
prefers-color-scheme) and live-applies the theme for the current mode, including the native window chrome via TaurisetTheme. Static mode keeps a single fixed theme (previous behavior, still the default).Changes
src-tauri/src/config.rs): three new optionalAppConfigfields —followSystemTheme,lightThemeId,darkThemeId— persisted toconfig.jsonvia the existing field-mergesave_config. Zero migration: absent fields = static mode, existing users unaffected. Serde round-trip + defaults tests added.src/utils/themeManagement.ts"): new pureresolveActiveThemeId(settings, systemIsDark)helper; per-mode picks resolve through the previously deadgetSystemThemeId`.src/contexts/ThemeProvider.tsx):get_configand persists them viaupdateSettings(was a no-op);matchMediachange listener now uses the user's per-mode picks (previously hardcodedtabularis-dark/tabularis-lightbehind a flag nothing could set) with mode-correct preset fallback if a picked theme no longer resolves (e.g. deleted custom theme);getCurrentWindow().setTheme) on every theme application — logged and non-fatal on failure;src/components/settings/AppearanceTab.tsx): Static / Follow SystemSettingButtonGroup; in follow mode two pickers filtered by theme classification (monacoTheme.base, covers custom themes); static mode picker unchanged.themeMode,themeModeDesc,themeModeStatic,themeModeSystem,lightTheme,darkTheme) in all 11 locales.currentThemeand follows system switches automatically; explicit editor overrides stay fixed.Testing
pnpm vitest run: 3789 tests pass (new: resolution helper unit tests; provider tests for load hydration, persistence, immediate apply on toggle, bidirectional OS switching, unresolvable-pick fallback, static-mode immunity; AppearanceTab UI tests).pnpm test:rust: 1139/1143 — 4askpassfailures are environmental (socket path under the default macOS TMPDIR; pass withTMPDIR=/tmp), pre-existing and unrelated.pnpm typecheck,pnpm lint: clean.