GitDesktop / Tickets / #97 feat(settings,theme,hotkeys): add selectable application themes

Anonymous - 2026-07-21

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 8df9a39

Status: ✅ Deploy successful!

Preview URL: https://1e4667ba.gitdesktop.pages.dev

Branch Preview URL: https://feat-theme-picker-dark-dimme.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

Context for reviewers

What this does

Adds a theme picker (System / Light / Dark / Slate) in Settings → Appearance that overrides the OS color scheme, ships Slate — a softer cool blue-gray dark theme to reduce the eye strain of the default near-black + near-white Dark — plus a command-palette Cycle theme action. Default Dark and existing Light are unchanged.

Deliberate calls (please read before flagging):

Theme is apply-on-change, deliberately outside the settings draft/Save form — excluded from SettingsDraft, applied via saveSettings.mutate({…data, theme}) + commitTheme(), the exact pattern diffViewMode already uses. A visual theme wants instant feedback; CLAUDE.md sanctions apply-on-change for single discrete selects.

Flash-free boot uses a localStorage mirror (gd-theme) read synchronously in initTheme() before first paint. The tauri store's loadSettings() is async and resolves after first paint, so without the mirror a saved override flashes the wrong theme on every cold boot. The store stays source of truth; the mirror is reconciled via commitTheme(s.theme) on load.

useIsDark() now observes the .dark class (MutationObserver) instead of matchMedia — required so the diff viewer / code editor follow a manual override, not just the OS scheme. theme.ts is the sole writer of the class.

code-highlight.css converted from @media (prefers-color-scheme: dark) to .dark-scoped selectors — an intentional in-scope bug fix. The old media query bypassed the class, so blame/conflict syntax colors would ignore a manual override.

Slate is .dark.slate (extends .dark), overriding only the neutral ramp + desaturated accents; everything .dark-keyed inherits (mirrors Primer's dark.dimmed @extends dark.json). Cool tint is hue ~257, deliberately not the mint brand hue (175), so chrome never reads brand-tinted.

The next-themes import in components/ui/sonner.tsx is intentionally left as-is — inert scaffold (no ThemeProvider is mounted anywhere; it's the only reference), and Sonner themes via CSS vars through .dark.

Disclosures / deferred:

Diff viewer + syntax highlighting keep their own hardcoded GitHub palettes (per the design system — syntax themes are deliberately not tokenized), so the diff surface reads a touch darker than the Slate chrome. Deferred as an optional follow-up, not in scope.

Docs are synced in this PR — README Highlights, marketing capabilities.ts (both AI-native and Just-Git views), in-app guide (content.ts), and a changelog.d fragment.

Branch is still named feat/theme-picker-dark-dimmed (predates the Slate rename) — cosmetic only.

Verified locally: pnpm build, pnpm lint, pnpm changelog:check, cd site && pnpm build all green; all four themes exercised in the running dev app (Slate measured at [#1]c2128 / #d1d7e0, ~11:1 contrast).

Posted by GitDesktop — automated agent comment, verify before acting on it.

Related

Tickets: #1
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Summary

Adds a System/Light/Dark/Slate theme picker (Settings → Appearance + a command-palette "Cycle theme" action), with a localStorage mirror for flash-free boot and a switch from OS-media-query theming to .dark-class theming so manual overrides reach the diff viewer, code editor, and highlight surfaces. The change is sound and I found no blockers — the data flow is correct and the OS→class migration is thorough. One should-fix around native-control theming.

I traced the two paths most likely to break:

Bulk Save doesn't drop theme. theme is excluded from SettingsDraft, but SettingsScreen's onSubmit persists { ...current, ...value } with current = settings.data (full AppSettings) and value = the theme-less draft, so the saved theme is preserved. The dirty check uses toDraft on both sides, so theme changes never mark the bulk form dirty. Correct.

The OS-scheme migration is complete. The only app CSS keyed on @media (prefers-color-scheme: dark) was code-highlight.css (converted), and every JS consumer (shiki-highlighter, DiffSurface, DiffViewer, code-editor via useIsDark) now reads the .dark class, so a forced Light/Dark/Slate reaches all of them.

Correctness / Edge cases

should-fix — src/App.css / src/lib/theme.ts apply(): nothing sets the CSS color-scheme property (confirmed absent from all app CSS, index.html, and the imported shadcn/tailwind.css), so native controls still follow the OS scheme rather than the chosen theme. Concrete case: on a light-OS machine, choosing Dark or Slate darkens the CSS-variable-driven chrome but leaves native scrollbars, <input>/<textarea> rendering, and the webview's default canvas light — and the inverse (forcing Light on a dark-OS machine) yields a light app with dark scrollbars. Before this PR the app always tracked the OS, so these coincided; the whole point of the new override is that they now diverge, which makes the mismatch reachable. Fix: drive color-scheme from the same source — either root.style.colorScheme = dark ? "dark" : "light" inside apply(), or add :root { color-scheme: light } and .dark { color-scheme: dark } to App.css so it follows the class automatically.

Everything else — the boot mirror + store reconciliation in main.tsx/initTheme/commitTheme, the nextTheme wrap-around, the MutationObserver-based useIsDark (getSnapshot returns a stable boolean; duplicate .dark/.slate toggles dedupe via React), the command-palette registration, and the docs/changelog/capabilities sync — looks correct and consistent with the repo's established diffViewMode apply-on-change pattern.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No security issues in this diff. The theme value is validated by isTheme() on read from localStorage, and both apply() sinks (classList.toggle("dark", …) / classList.toggle("slate", …)) use literal class names — no attacker-controlled string reaches the DOM, and there's no injection, eval, network, or command sink anywhere in the change. Copilot's two flags (missing try/catch around localStorage.getItem in initTheme, and the non-optimistic select value) are robustness/UX, not exploitable security bugs.

Posted by GitDesktop — AI output, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

Round 1 — dispositions

All three findings accepted and fixed (in the next push):

should-fix (color-scheme, AI review): Added :root { color-scheme: light } and .dark { color-scheme: dark } to App.css — verified first that no color-scheme property was set anywhere. Native scrollbars, form controls, and the webview's default canvas now follow the theme class instead of the OS, closing the divergence the override introduced. Slate inherits .dark, so it correctly renders dark native controls.

initTheme() unguarded localStorage (Copilot): now try/catch-guarded with a "system" fallback, symmetric with commitTheme's existing setItem guard. (Replied + resolved on the thread.)

Non-optimistic theme <Select> (Copilot): now patches the settings cache optimistically at mutate time, so the picker reflects the choice immediately. (Replied + resolved on the thread.)

Agreed with the security audit — no exploitable issue in either Copilot flag.

Verified locally: tsc -b, pnpm lint, pnpm build, and cd site && pnpm build all green. Ready to push.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

This adds a System/Light/Dark/Slate theme picker (Settings → Appearance + a "Cycle theme" command), persisted in settings with a localStorage mirror for flash-free boot, and switches JS-themed surfaces plus native controls from OS-media-query theming to .dark-class theming so manual overrides take effect. I traced the data flow, the OS→class migration, and the changes made since the last review; the change is sound and I found no blockers.

Resolved since last review

color-scheme divergence (previous should-fix) — App.css now sets :root { color-scheme: light } and .dark { color-scheme: dark }. Specificity is equal (:root and .dark are both one class-level token), so source order decides and the later .dark rule wins when the class is present; .dark.slate doesn't redeclare it and correctly inherits dark. Native scrollbars/form controls now follow the forced theme. Confirmed fixed in the current diff.

initTheme() unguarded localStorage (Copilot) — the getItem read is now wrapped in try/catch with a "system" fallback, symmetric with commitTheme's setItem guard. Confirmed fixed.

Non-optimistic theme <Select> (Copilot) — selectTheme now calls queryClient.setQueryData(settingsKeys.settings, updated) before mutating; settingsKeys.settings (["settings"]) matches useSettings's query key, so the patch lands on the right cache entry. Confirmed fixed.

Nits

nit — src/App.tsx cycle-theme handler vs src/features/settings/AppearanceSection.tsx selectTheme: the two theme-change paths diverged. selectTheme optimistically patches the settings cache, but the cycle-theme hotkey handler only does saveSettings.mutate(...) + commitTheme(...). Concrete case: with the Settings → Appearance panel open, invoking "Cycle theme" from the command palette repaints the app immediately (via commitTheme) but the <Select> trigger, bound to settings.data.theme, keeps showing the previous label until the mutation's onSuccess invalidate refetches — the exact flicker the optimistic patch was added to avoid on the other path. Extracting a shared helper (applyThemeChange(queryClient, saveSettings, next)) used by both would keep them consistent.

nit — src/features/settings/AppearanceSection.tsx selectTheme: the optimistic setQueryData has no onError rollback, and useSaveSettings only invalidates on success. If the store write throws, the cache and DOM stay on the new theme while the persisted store keeps the old value (self-heals on next boot via commitTheme(s.theme)). Low severity given it's a local write, but the repo's optimistic-mutation convention pairs the patch with a snapshot/rollback; worth a getQueryData snapshot + restore in an onError.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

Round 2 — dispositions

Both nits accepted and fixed (next push) via a single shared hook, useApplyTheme(current, next) in queries.ts, now used by both the Appearance picker (selectTheme) and the cycle-theme command:

nit — diverged paths: the two entry points now go through the same helper, so the cycle-theme path is optimistic too — the <Select> no longer flickers when you cycle theme with the Appearance panel open. That inconsistency was introduced by the round-1 optimistic fix landing on only one path; consolidating removes it by construction.

nit — no rollback: the helper snapshots the cache and rolls back on a store-write error — it restores the previous cache entry and re-applies the previous theme (class + localStorage, via commitTheme), matching the repo's optimistic snapshot/rollback convention.

Net effect: one place owns theme application, so the two paths can't drift again. Verified green: tsc -b, pnpm lint, pnpm build. Ready to push.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

This adds a System/Light/Dark/Slate theme picker (Settings → Appearance + a "Cycle theme" command), persisted with a localStorage mirror for flash-free boot, and moves JS-themed surfaces and native controls from OS-media-query theming to .dark-class theming so manual overrides take effect. Having traced the data flow (initTheme → commitTheme/useApplyTheme → .dark/.slate classes → useIsDark), the change is sound and I found no blockers.

Resolved since last review

Diverged theme-change paths (previous nit) — both entry points now route through useApplyTheme in queries.ts: App.tsx's cycle-theme handler calls applyTheme(current, next) and AppearanceSection.selectTheme calls applyTheme(settings.data, next). Both patch the cache optimistically, so the <Select> no longer flickers when cycling with the panel open. Confirmed in the current diff.

Optimistic patch without rollback (previous nit) — useApplyTheme now passes an onError to saveSettings.mutate that restores the prior cache entry (setQueryData(settingsKeys.settings, current)) and re-applies the prior theme (commitTheme(current.theme)). Confirmed.

initTheme() unguarded localStorage read (Copilot, marked outdated) — the getItem in initTheme is now try/catch-wrapped with a "system" fallback, symmetric with commitTheme's setItem guard. Confirmed in theme.ts.

Non-optimistic theme <Select> (Copilot, marked outdated) — handled by the useApplyTheme setQueryData patch on the same ["settings"] key useSettings reads. Confirmed.

Nits

nit — src/lib/settings/queries.ts, useApplyTheme onError: the rollback closes over the current snapshot of its own call, so under rapid cycling an earlier failed write can revert past a later successful one. Concrete case: cycle-theme pressed twice quickly — call A (system→light) and call B (light→dark); B's store write succeeds and applies dark, but A's write then rejects and its onError restores cache+class to A's snapshot (system), diverging the DOM from the persisted dark until the next boot reconciles via commitTheme(s.theme). This requires an actual store-write failure (rare) so it's low-severity; if you want to close it, guard the rollback on the cache still holding updated before restoring (or skip rollback when a newer theme has superseded it).

Otherwise the OS→class migration is clean: code-highlight.css's .dark-scoped selectors gain specificity over the base rules (so dark wins only when the class is present, with no source-order fragility), useIsDark correctly reports dark for both dark and slate, and the :root/.dark color-scheme pair keeps native controls in sync with a forced theme.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

Round 3 — disposition

nit (rollback race in useApplyTheme): accepted and fixed (next push). The onError rollback now guards on the cache still holding this call's theme before reverting:

const latest = queryClient.getQueryData<AppSettings>(settingsKeys.settings); if (latest?.theme !== next) return;

So a late-failing earlier write can no longer stomp a newer successful one (the two-fast-cycles case you described). The residual "both writes fail" case rolls back to the later snapshot and self-heals on next boot via commitTheme(s.theme) — acceptable for a local store write that essentially never fails.

Verified green: tsc -b, pnpm lint, pnpm build. Ready to push.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Sound change. The theme picker (System/Light/Dark/Slate) with a synchronous localStorage mirror for flash-free boot and the OS→.dark-class migration all trace cleanly, and I found no blockers or should-fixes. I verified the one thing the diff didn't fully show — dropping theme from SettingsDraft is safe because the bulk Save reconstructs the full object as { ...settings.data, ...value } (SettingsScreen onSubmit, line 148), and value no longer carries theme, so the apply-on-change value is preserved rather than clobbered.

Resolved since last review

Rollback race in useApplyTheme onError (prior nit) — now guarded: onError reads back the cache and bails (if (latest?.theme !== next) return;) before restoring, so a late-failing earlier write can't stomp a newer successful one. Confirmed in queries.ts. (In practice the guard is belt-and-suspenders: the shared useSaveSettings observer drops the earlier call's per-mutate onError when a newer mutate supersedes it, so the earlier rollback wouldn't fire anyway — but the guard is correct and harmless.)

initTheme() unguarded localStorage read (Copilot) — the getItem is now try/catch-wrapped with a "system" fallback, symmetric with commitTheme's setItem guard. Confirmed in theme.ts.

Non-optimistic theme <Select> (Copilot) — useApplyTheme patches the ["settings"] cache via setQueryData before firing the mutation, on the same key useSettings reads, so both the picker and cycle-theme reflect the choice immediately. Confirmed.

Readability

nit — src/lib/use-is-dark.ts, useIsDark: the subscribe and getSnapshot args are inline closures recreated every render, so useSyncExternalStore tears down and re-subscribes on each render — now allocating/disconnecting a fresh MutationObserver each time rather than the old (cheaper) addEventListener. The re-subscribe frequency is unchanged from before this PR, so this isn't a regression in behavior, just a slightly heavier per-render churn in a hook the diff viewer/editor use. Optional: hoist a module-scope subscribe/getSnapshot pair (both are stateless) so the subscription is stable.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

Round 4 — disposition

nit (readability — useSyncExternalStore re-subscribe in useIsDark): accepted and fixed (next push). Hoisted subscribe and getSnapshot to module-scope functions (subscribeToThemeClass / isDarkSnapshot, both stateless), so useSyncExternalStore subscribes once per mount instead of tearing down and recreating the MutationObserver on every render. Good catch — the OS→class rewrite made the per-render churn heavier (observer alloc vs the old addEventListener); this removes it entirely.

Also — thanks for confirming the round-3 rollback guard is belt-and-suspenders (React Query drops the superseded per-mutate onError anyway). Agreed: correct and harmless.

Verified green: tsc -b, pnpm lint, pnpm build. Ready to push.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

This change adds a System/Light/Dark/Slate theme picker (Settings → Appearance + a cycle-theme palette command), persisted in settings with a synchronous localStorage mirror for flash-free boot, and drives native controls via color-scheme. I traced every path and it's sound — no blockers, no should-fixes; safe to merge.

I re-verified the one cross-file concern from the prior round: dropping theme from SettingsDraft doesn't lose it on bulk Save, because SettingsScreen.onSubmit reconstructs the payload as saveSettings.mutateAsync({ ...current, ...value }) where current = settings.data (which useApplyTheme keeps patched with the live theme via setQueryData) and value no longer carries theme — so the apply-on-change theme is preserved, not clobbered (confirmed at SettingsScreen.tsx:148). The .dark.slate layering, the sole-writer apply(), the OS-listener registration (now only in initTheme, no duplicate), and the rollback-guard are all correct.

Resolved since last review

useIsDark re-subscribe churn (prior nit) — resolved: subscribeToThemeClass/isDarkSnapshot are now module-scope, so useSyncExternalStore subscribes once per mount instead of recreating the MutationObserver each render. Confirmed in use-is-dark.ts.

initTheme() unguarded localStorage read (Copilot) — resolved: the getItem is try/catch-wrapped with a "system" fallback, symmetric with commitTheme's setItem guard. Confirmed at theme.ts:82-93.

Non-optimistic theme <Select> (Copilot) — resolved: useApplyTheme patches the ["settings"] cache via setQueryData before firing the mutation, on the same key useSettings reads, so both the picker and cycle-theme reflect the choice immediately. Confirmed at queries.ts:116-141.

Nothing else worth raising — the change is well-constructed and the docs surfaces (README, capabilities, help guide, changelog fragment) are all updated in the same PR.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-21

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

feat(settings,theme,hotkeys): add selectable application themes

Milestone

Searches

Help

#97 feat(settings,theme,hotkeys): add selectable application themes

Theme resolution and styling

Settings and hotkeys

Documentation

Discussion

Deploying gitdesktop with Cloudflare Pages

Context for reviewers

Related

Summary

Correctness / Edge cases

Round 1 — dispositions

Resolved since last review

Nits

Round 2 — dispositions

Resolved since last review

Nits

Round 3 — disposition

Resolved since last review

Readability

Round 4 — disposition

Resolved since last review

Latest commit:	`8df9a39`
Status:	✅ Deploy successful!
Preview URL:	https://1e4667ba.gitdesktop.pages.dev
Branch Preview URL:	https://feat-theme-picker-dark-dimme.gitdesktop.pages.dev