Add appearance settings with themes and accent colors#1550
Add appearance settings with themes and accent colors#1550juliusmarminge wants to merge 3 commits intomainfrom
Conversation
- Move theme control into a dedicated appearance settings panel - Add built-in themes, accent overrides, and desktop IPC sync - Migrate settings and update components to use unified appearance state
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| return cachedSystemDark; | ||
| } | ||
|
|
||
| function subscribeSystemDark(listener: () => void): () => void { |
There was a problem hiding this comment.
🟡 Medium hooks/useAppearance.ts:93
Each call to subscribeSystemDark registers a new handler on the MediaQueryList, and each handler iterates over the full systemDarkListeners array. With N concurrent subscriptions, a single media-query change triggers N² listener invocations instead of N, causing quadratic performance degradation as the number of subscribing components grows. The handler should be registered once globally rather than per-subscription.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/hooks/useAppearance.ts around line 93:
Each call to `subscribeSystemDark` registers a new `handler` on the MediaQueryList, and each handler iterates over the full `systemDarkListeners` array. With N concurrent subscriptions, a single media-query change triggers N² listener invocations instead of N, causing quadratic performance degradation as the number of subscribing components grows. The handler should be registered once globally rather than per-subscription.
Evidence trail:
apps/web/src/hooks/useAppearance.ts lines 93-103 at REVIEWED_COMMIT: The `subscribeSystemDark` function registers a new handler via `mq.addEventListener('change', handler)` on every call (line 99), and each handler iterates `for (const l of systemDarkListeners) l()` (line 97-98). With N subscribers, N handlers are attached, each iterating N listeners = N² total calls.
| function suppressTransitions(fn: () => void) { | ||
| document.documentElement.classList.add("no-transitions"); | ||
| fn(); | ||
| // Force reflow so the no-transitions class takes effect before removal | ||
| // oxlint-disable-next-line no-unused-expressions | ||
| document.documentElement.offsetHeight; | ||
| requestAnimationFrame(() => { | ||
| document.documentElement.classList.remove("no-transitions"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟢 Low hooks/useAppearance.ts:29
In suppressTransitions, if fn() throws an exception, the requestAnimationFrame callback that removes the no-transitions class is never scheduled, leaving the class on document.documentElement permanently. This disables all CSS transitions for the rest of the session. Consider wrapping fn() in a try/finally to ensure the cleanup always runs.
function suppressTransitions(fn: () => void) {
document.documentElement.classList.add("no-transitions");
- fn();
- // Force reflow so the no-transitions class takes effect before removal
- // oxlint-disable-next-line no-unused-expressions
- document.documentElement.offsetHeight;
- requestAnimationFrame(() => {
- document.documentElement.classList.remove("no-transitions");
- });
+ try {
+ fn();
+ // Force reflow so the no-transitions class takes effect before removal
+ // oxlint-disable-next-line no-unused-expressions
+ document.documentElement.offsetHeight;
+ } finally {
+ requestAnimationFrame(() => {
+ document.documentElement.classList.remove("no-transitions");
+ });
+ }
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/hooks/useAppearance.ts around lines 29-38:
In `suppressTransitions`, if `fn()` throws an exception, the `requestAnimationFrame` callback that removes the `no-transitions` class is never scheduled, leaving the class on `document.documentElement` permanently. This disables all CSS transitions for the rest of the session. Consider wrapping `fn()` in a try/finally to ensure the cleanup always runs.
Evidence trail:
apps/web/src/hooks/useAppearance.ts lines 27-35 at REVIEWED_COMMIT: The `suppressTransitions` function adds `no-transitions` class, calls `fn()`, then schedules cleanup via `requestAnimationFrame`. There is no try/finally block, so if `fn()` throws, the cleanup code never executes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Effect cleanup removes shared global style element
- Removed the useEffect cleanup that called removeThemeTokens(), since the shared global <style> element should persist across component unmounts and the FOUC-prevention code plus remaining mounted components' effects handle reapplication.
- ✅ Fixed: Theme migration unreachable without old settings key
- Moved the t3code:theme localStorage migration block before the OLD_SETTINGS_KEY early-return guard so it runs independently regardless of whether old-format settings exist.
- ✅ Fixed: Redundant appearance update before full settings reset
- Removed the redundant updateSettings() call that set appearance defaults immediately before resetSettings(), which already applies all defaults including the same appearance values.
Or push these changes by commenting:
@cursor push 57ebd82da6
Preview (57ebd82da6)
diff --git a/apps/web/src/components/settings/SettingsPanels.tsx b/apps/web/src/components/settings/SettingsPanels.tsx
--- a/apps/web/src/components/settings/SettingsPanels.tsx
+++ b/apps/web/src/components/settings/SettingsPanels.tsx
@@ -424,7 +424,7 @@
export function useSettingsRestore(onRestored?: () => void) {
const settings = useSettings();
- const { updateSettings, resetSettings } = useUpdateSettings();
+ const { resetSettings } = useUpdateSettings();
const isGitWritingModelDirty = !Equal.equals(
settings.textGenerationModelSelection ?? null,
@@ -487,10 +487,9 @@
);
if (!confirmed) return;
- updateSettings({ colorMode: "system", activeThemeId: "t3code", accentHue: null });
resetSettings();
onRestored?.();
- }, [changedSettingLabels, onRestored, resetSettings, updateSettings]);
+ }, [changedSettingLabels, onRestored, resetSettings]);
return {
changedSettingLabels,
diff --git a/apps/web/src/hooks/useAppearance.ts b/apps/web/src/hooks/useAppearance.ts
--- a/apps/web/src/hooks/useAppearance.ts
+++ b/apps/web/src/hooks/useAppearance.ts
@@ -12,7 +12,7 @@
import { useCallback, useEffect, useMemo, useSyncExternalStore } from "react";
import type { ColorMode } from "@t3tools/contracts/settings";
import type { DesktopAppearance } from "@t3tools/contracts";
-import { applyThemeTokens, findThemeById, removeThemeTokens, BUILT_IN_THEMES } from "~/lib/themes";
+import { applyThemeTokens, findThemeById, BUILT_IN_THEMES } from "~/lib/themes";
import { useSettings, useUpdateSettings } from "./useSettings";
// ── Constants ────────────────────────────────────────────────────
@@ -138,8 +138,6 @@
// Sync to Electron
syncDesktopAppearance({ mode: colorMode, themeId: activeThemeId, accentHue });
-
- return () => removeThemeTokens();
}, [resolvedTheme, activeTheme, accentHue, colorMode, activeThemeId]);
const setColorMode = useCallback(
diff --git a/apps/web/src/hooks/useSettings.ts b/apps/web/src/hooks/useSettings.ts
--- a/apps/web/src/hooks/useSettings.ts
+++ b/apps/web/src/hooks/useSettings.ts
@@ -235,10 +235,9 @@
export function migrateLocalSettingsToServer(): void {
if (typeof window === "undefined") return;
- const raw = localStorage.getItem(OLD_SETTINGS_KEY);
- if (!raw) return;
-
- // Migrate old t3code:theme localStorage key to server colorMode
+ // Migrate old t3code:theme localStorage key to server colorMode.
+ // This runs independently of OLD_SETTINGS_KEY since the old useTheme hook
+ // wrote t3code:theme separately.
try {
const oldTheme = localStorage.getItem("t3code:theme");
if (oldTheme === "light" || oldTheme === "dark" || oldTheme === "system") {
@@ -250,6 +249,9 @@
// Best-effort — don't block startup
}
+ const raw = localStorage.getItem(OLD_SETTINGS_KEY);
+ if (!raw) return;
+
try {
const old = JSON.parse(raw);
if (!Predicate.isObject(old)) return;| // Sync to Electron | ||
| syncDesktopAppearance({ mode: colorMode, themeId: activeThemeId, accentHue }); | ||
|
|
||
| return () => removeThemeTokens(); |
There was a problem hiding this comment.
Effect cleanup removes shared global style element
High Severity
The useEffect cleanup calls removeThemeTokens(), which removes a shared global <style> element by ID. Since multiple components (ChatView, DiffPanel, DiffWorkerPoolProvider, ChatMarkdown, AppearanceSettingsPanel) each call useAppearance(), each gets its own effect cleanup. When any of these components unmounts (e.g., navigating from chat to settings), the cleanup deletes the shared style element. The root component's effect doesn't re-run because its dependencies haven't changed, so theme token CSS overrides are permanently lost until a dependency changes. Non-default themes and accent colors break on navigation.
Additional Locations (1)
| } | ||
| } catch { | ||
| // Best-effort — don't block startup | ||
| } |
There was a problem hiding this comment.
Theme migration unreachable without old settings key
Medium Severity
The t3code:theme localStorage migration is placed after if (!raw) return; on line 239, where raw reads from OLD_SETTINGS_KEY. Since t3code:theme was written independently by the now-deleted useTheme hook, users whose old settings were already migrated (removing OLD_SETTINGS_KEY) or who never had old-format settings will hit the early return. Their theme preference in t3code:theme is never migrated to the server's colorMode setting, silently losing the preference.
| if (!confirmed) return; | ||
|
|
||
| setTheme("system"); | ||
| updateSettings({ colorMode: "system", activeThemeId: "t3code", accentHue: null }); |
There was a problem hiding this comment.
Redundant appearance update before full settings reset
Low Severity
updateSettings({ colorMode: "system", activeThemeId: "t3code", accentHue: null }) is called immediately before resetSettings(), which internally calls updateSettings(DEFAULT_UNIFIED_SETTINGS) — containing the same appearance defaults. This fires two separate server RPCs with overlapping content when one suffices.
- Write theme CSS variables at the intermediate token level so Tailwind picks them up - Call `useAppearance()` before the root route can return early to satisfy Hooks rules
- Apply theme overrides as inline CSS variables - Add diff-specific tokens and color-blind diff palette - Update swatches and diff stats to use new semantic colors
github-actions
bot
added
size:XXL
| border: "--alpha(var(--color-black) / 45%)", | ||
| input: "--alpha(var(--color-black) / 50%)", |
There was a problem hiding this comment.
🟠 High lib/themes.ts:68
--alpha() is not valid CSS syntax, so the border and input token values for the High Contrast theme produce invalid CSS when consumed. The browser ignores these declarations and the overrides silently fail to apply. Lines 68–69 and 73–74 should use color-mix(in srgb, var(--color-black) 45%, transparent) (or equivalent modern syntax) instead of --alpha(...).
- border: "--alpha(var(--color-black) / 45%)",
- input: "--alpha(var(--color-black) / 50%)",
+ border: "color-mix(in srgb, var(--color-black) 45%, transparent)",
+ input: "color-mix(in srgb, var(--color-black) 50%, transparent)",🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/lib/themes.ts around lines 68-69:
`--alpha()` is not valid CSS syntax, so the `border` and `input` token values for the High Contrast theme produce invalid CSS when consumed. The browser ignores these declarations and the overrides silently fail to apply. Lines 68–69 and 73–74 should use `color-mix(in srgb, var(--color-black) 45%, transparent)` (or equivalent modern syntax) instead of `--alpha(...)`.
Evidence trail:
apps/web/src/lib/themes.ts lines 68-69, 73-74 (--alpha usage); apps/web/src/lib/themes.ts lines 180-182 (el.style.setProperty applying raw values); apps/web/src/index.css lines 77-85, 107-115 (--alpha usage in build-time CSS); GitHub discussion https://github.com/tailwindlabs/tailwindcss/discussions/18124 (collaborator @wongjn: "`--alpha()` is syntactic sugar for `color-mix()`" - confirms it's a build-time transformation)
1 participant
Closes #1284
Closes #1535
Closes #1545
Closes #418
Closes #464
Closes #254
Closes #1279
Closes #233
Closes #400
Closes #1183
Summary
useAppearanceflow that syncs theme state, applies tokens, and prevents FOUC.Testing
bun fmtbun lintbun typecheckbun run testHigh Contrast
Color Blind
Configurable Accent
Settings (NEEDS A LOT OF WORK STILL)
Note
Medium Risk
Moves theme control into server-authoritative settings and adds cross-surface token injection plus new Electron IPC, which could cause regressions in initial render (FOUC) and dark-mode synchronization across web/desktop.
Overview
Adds a dedicated Appearance settings page to manage
colorMode(system/light/dark), select a built-in theme, and override the accent hue, and wires it into the settings navigation/routes.Replaces the legacy
useThemeflow with a unifieduseAppearancehook that applies the.darkclass and injects theme CSS tokens (with a localStorage cache to reduce FOUC), updates consumers (chat markdown, diff panel, diff worker pool) to readresolvedThemefrom appearance, and updates “restore defaults” to reset the new appearance fields.Extends contracts and Electron bridging by adding
DesktopAppearanceplus a newdesktop:set-appearanceIPC (setThemedeprecated) so the web UI can sync richer appearance state to the desktop shell (currently used to setnativeTheme.themeSource).Written by Cursor Bugbot for commit f4ab7eb. This will update automatically on new commits. Configure here.
Note
Add appearance settings with theme and accent color controls
/settings/appearanceroute with a settings panel for color mode (light/dark/system), built-in theme selection, and accent color customization (presets + hex picker).useAppearanceto replace the deleteduseThemehook; it drives.darkclass and CSS custom property injection, caches values in localStorage to prevent FOUC, and syncs to Electron viadesktopBridge.setAppearance.themes.tsmodule defining token types, three built-in themes (t3code,high-contrast,color-blind), accent presets, and helpers to inject/remove CSS variables.ServerSettingswithcolorMode,activeThemeId, andaccentHuefields; migrates the legacyt3code:themelocalStorage key tocolorModeon startup.Macroscope summarized f4ab7eb.