feat: add theme mode dropdown with sync and single modes#24680
feat: add theme mode dropdown with sync and single modes#24680jaaydenh wants to merge 36 commits into
Conversation
Docs preview |
jaaydenh
left a comment
jaaydenh
left a comment
There was a problem hiding this comment.
Nice cleanup of the flat grid into a two-mode flow. The state machine in themeMode.ts is well-factored and the legacy migration path is carefully covered by tests. The readOptionalUserConfig helper is a great DRY win.
3 findings + 2 observations across 5 inline comments. No blockers, but the non-transactional multi-field write deserves attention.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 165a72c4bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Add four colorblind-friendly theme variants alongside the existing light and dark themes: - `dark-protan-deuter` and `light-protan-deuter`: high contrast for users with red-green colorblindness (protanopia and deuteranopia). - `dark-tritan` and `light-tritan`: high contrast for users with blue-yellow colorblindness (tritanopia). Each variant ships a full theme module (`branding`, `experimental`, `monaco`, `mui`, `roles`) and a CSS class block in `site/src/index.css` that overrides the semantic color variables most affected by colorblindness (diff highlights, content accents, surface, border, and highlight colors). Extract theme resolution into `site/src/theme/colorblind.ts`, which owns the concrete theme registry, the `resolveThemeName` helper used by `ThemeProvider`, and the `isConcreteThemeName` validator used by `AgentEmbedPage`'s postMessage handler. The embed page now accepts every concrete theme and clears every possible theme class on unmount so switching between a colorblind variant and the base theme does not leave a stale class on the root. Add `cssVariables.test.ts`, which asserts that every theme class block in `site/src/index.css` declares the full set of required CSS variables so switching themes cannot leave stale values leaking through from a previous theme. This PR ships the palettes and the resolution machinery but does not yet expose the new themes in the Appearance settings page; the follow-up PR adds the picker UI that lets users select them. Produced with Coder Agents assistance.
Replace the flat 9-theme grid on the Appearance settings page with a
"Theme mode" dropdown that exposes two modes:
- Sync with system: pick a light theme and a dark theme, each as a
row of three circular swatches (default, protanopia/deuteranopia,
tritanopia). The card matching the active OS color scheme gets an
"Active" badge.
- Single theme: pick one of six concrete themes as a large tile
(Light default, Light protanopia/deuteranopia, Light tritanopia,
and the three dark equivalents). Colorblind-friendly variants
carry a Beta badge.
The persisted schema gains three typed fields on
UserAppearanceSettings: theme_mode ("sync" | "single" | ""),
theme_light, and theme_dark. The legacy theme_preference remains
the authoritative value read by the runtime theme provider and
continues to round-trip through the SDK. In sync mode the
frontend rewrites theme_preference to match the slot that is
currently active; in single mode it mirrors the chosen theme.
Legacy auto-* preferences ("auto", "auto-protan-deuter",
"auto-tritan") are no longer selectable in the UI. Users whose
stored preference still points at an auto-* value are migrated on
read to the equivalent sync-mode triple, preserving their
colorblind family choice. The auto-* names continue to resolve at
runtime so older clients keep working until the user next saves
their preferences.
Backend: six new sqlc queries (GetUserThemeMode, UpdateUserThemeMode,
and matching Light/Dark pairs) mirror the existing
GetUserTerminalFont/UpdateUserTerminalFont shape using the
user_configs key-value table. No SQL migration is required because
user_configs is schema-free. Empty theme_mode is accepted on write
and defaults to "single" for backward compatibility with the older
CLI client that only sends theme_preference.
Frontend: a new theme/themeMode.ts module owns the draft state
machine, legacy migration, and the draft-to-API payload conversion.
The AppearanceForm rewrite is covered by Storybook variants,
vitest coverage for the pure helpers, and the AppearancePage RTL
tests. Playwright e2e updated to drive the new dropdown flow.
jaaydenh
force-pushed
the
feat/colorblind-themes
branch
from
2c1973e to
cf99301
Compare
jaaydenh
force-pushed
the
feat/theme-mode-dropdown
branch
from
165a72c to
6e5c472
Compare
- Apply base mode class (`dark`/`light`) alongside concrete theme class
so Tailwind `dark:` utilities and selector-based theming (e.g.
`Chart.tsx`, `ToolIcon.tsx`) keep matching when a colorblind variant
is active. Extract a shared `baseModeFor` helper and cover every
concrete theme in `colorblind.test.ts`.
- Remove the overclaimed Okabe-Ito / WCAG AA language from the
protan-deuter role comments; describe the palette as inspired by the
CVD-safe scheme instead.
- Update the stale `?theme=light|dark` comment in `AgentEmbedPage` to
reflect that any concrete theme (including colorblind variants) is
accepted.
- Replace the `.toContain("light")` assertion in the user-settings e2e
test with `classList.contains("light")` so a hyphenated colorblind
light variant cannot satisfy the check.
- Drop the redundant `--avatar-*` and `--radius` declarations from
`.light-protan-deuter` and `.light-tritan` (they already inherit from
`:root`), matching the `.dark` convention.
- Fill in the previously empty ThemeProvider doc comment.
Co-authored with Coder Agents.
Addresses review comments on #24680: - Wrap the five appearance writes in putUserAppearanceSettings in a single InTx so a mid-batch failure cannot leave the user's theme_mode, theme_light, theme_dark, and theme_preference fields half-applied (the sync-mode invariant is that mode plus both slots move together). - Document the write-path sanitization contract on UpdateUserAppearanceSettingsRequest: theme names are not validated against a server-side allowlist by design; migrateLegacyPreference and resolveThemeName sanitize unknown or stale values on read. - Preserve persisted theme_light and theme_dark when the decoded state is single mode by threading the raw slots through draftFromState. Previously they were discarded, so a user who customized sync slots, switched to single mode, and reopened settings would lose their sync selections. - Rewrite onChangeMode so toggling sync <-> single is a lossless UI choice: the current light and dark slots always survive the detour through single mode, and vice versa. Single -> sync no longer recomputes slots from FAMILY_PAIR. - Add direct unit tests for draftFromState covering sync round-trip, persisted slot reuse, garbage-slot fallback, and every concrete theme as a single-mode seed. Coder Agents generated.
Review fixesAddressed the five findings/observations from jaaydenh and the two P2 codex-bot comments in commit Comment-by-comment response
Verified locally with Coder Agents generated. |
The previous `classList.contains(\"light\")` check passes for any theme whose base mode is light, including colorblind variants such as `light-tritan` (which now set both `light-tritan` and `light` classes since ThemeProvider applies the base mode class alongside the concrete theme class). Assert against the full class-list tokens so only the plain `light` theme satisfies the check. Co-authored with Coder Agents.
- Restore mobile full-width dropdown component rules removed from index.css. - Mirror ThemeProvider class behavior in Storybook so dark colorblind themes also carry the base dark class there. - Move protan-deuter warning tokens to fuchsia and keep links on the base blue axis so warning/destructive and link/success remain distinct. - Keep protan-deuter role error distinct from danger, and keep tritan danger on the base orange role instead of rose. - Extend CSS-variable tests to cover syntax tokens and add regression checks for the reviewed semantic color collisions. - Update PR-body palette provenance to avoid unsupported Okabe-Ito/WCAG AA claims. Co-authored with Coder Agents.
Address remaining PR review feedback on #24680. - Add scheme-specific validation for theme_light and theme_dark using oneof validators, with required_if so sync mode cannot persist empty slots. - Generate updated API docs and types so external consumers see the new enum constraints. - Add backend tests for arbitrary slot values, cross-scheme slot values, and empty sync slots. - Switch FAMILY_PAIR to a satisfies Record<ConcreteThemeName, ThemeFamilyPair> assertion for explicit compile-time exhaustiveness. Coder Agents generated.
|
Follow-up: pushed Changes added on top of
Validation run:
Coder Agents generated. |
…e appearance submits
|
Update after latest merge-conflict pass:
PR is now reporting Generated by Coder Agents. |
|
/coder-agents-review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c31265b875
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The theme state machine in themeMode.ts is well-factored: clean discriminated unions, exhaustive migration paths, 26 unit tests that exercise every transition. The InTx wrapping on the write path, the module-load assertions in SingleModeSection, and the satisfies Record on FAMILY_PAIR all show careful attention to invariant enforcement. The review feedback loop across the three follow-up commits was disciplined.
1 P1, 1 P2, 12 P3, 2 Nit. The P1 is a session-long wrong theme for every sync-mode user.
The P1 (SSR metadata) is the only blocker. The P2 (old-CLI silent data loss) is worth a defensive guard before this ships. The 12 P3s cluster around three themes: dead code that tests describe but production never calls (resolveThemeName, switchToSync), test assertions that check the happy path but not the contract (require.Error without status, objectContaining without full payload), and the inherited per-key query pattern that this PR stretches from 2 keys to 5 (10 DB round-trips per GET).
Notes not posted inline: the PR description says sync mode mirrors "the currently active slot" but the code always mirrors the dark slot (the code is correct; the description is inaccurate). The writeUserSettingsReadError helper returns an identical message for all 5 settings, making operator triage harder. The ChatModelAdminPanel diff (~800 lines) is noise from merging main.
"The question is never 'is this worth fixing now vs. later?' The question is 'is this worth fixing vs. never?'" Pariston tried to build a case against this design and couldn't: "The problem is correctly understood, the solution is proportional, the fix is at the right causal level, and the edge cases are handled."
site/site.go:478-481
P1 [DEREM-4] SSR metadata omits theme_mode, theme_light, and theme_dark. The struct literal here only sets ThemePreference and TerminalFont; the three new fields zero-value to "".
cachedQuery applies staleTime: Infinity and refetchOnMount: false when SSR metadata is available, so React Query never refetches. For a sync-mode user whose OS is light:
- SSR delivers
theme_mode: "",theme_light: "",theme_dark: "". migrateLegacyPreferencesees empty mode, falls to the legacy path, returns single-mode with whatevertheme_preferenceholds (the dark-slot mirror).- User sees the dark theme. Correct answer: the light sync slot.
- Wrong theme persists for the entire session. No refetch. No self-healing.
"A sync-mode user whose OS is in light mode has theme_preference: 'dark-tritan' (the mirror value). First paint resolves to single-mode dark-tritan. The user sees a dark-to-light flash on every page load." (Hisoka)
"React Query never refetches because of the infinite stale time. The wrong theme persists until the user visits the Appearance page and saves." (Kite)
Fix: add eg.Go blocks around line 410 to fetch theme_mode, theme_light, and theme_dark (same pattern as the existing themePreference/terminalFont fetches), pass them through populateHTMLState, and include them in this struct literal. (Hisoka P2, Mafuuu P1, Kite P1)
🤖
site/src/api/queries/users.ts:287-290
P3 [DEREM-6] onMutate writes the optimistic payload into the query cache but there is no onError handler to restore the previous data. On a failed mutation (network error, validation rejection), ThemeProvider renders the unsaved theme for the rest of the session until something else triggers cache invalidation.
"The user sees 'something went wrong' next to a UI that looks like nothing went wrong." (Nami)
The standard React Query pattern: save previous data in onMutate, restore it in onError. Or move invalidation to onSettled so it fires on both success and failure. The pattern is pre-existing (the old code did the same with fewer fields), but this PR widens it to 5 coupled fields. (Nami P2, Knov P3, Ryosuke P3, Meruem P3)
🤖
🤖 This review was automatically generated with Coder Agents.
Refresh PR #24680 against its current base (feat/colorblind-themes) to clear mergeStateStatus: DIRTY. Conflicts resolved: - site/src/theme/colorblind.ts: deleted on base, modified on PR head. Honored the base branch deletion and migrated theme-mode helpers (CONCRETE_THEMES, isConcreteThemeName, LEGACY_AUTO_SYNC, etc.) into site/src/theme/index.ts to match the consolidated registry design. - site/src/theme/colorblind.test.ts: combined coverage from both branches (registry behavior, resolveThemeName, concrete-theme validation, legacy auto-family migration, baseModeFor, palette checks). - site/src/contexts/ThemeProvider.tsx: integrated the consolidated registry from #/theme while preserving theme-mode dropdown behavior via migrateLegacyPreference, resolveActiveThemeName, and usePreferredColorScheme. Also added narrow .gitattributes rules so merge-introduced SMTP and Helm goldens preserve their intentional whitespace through format-patch / git am. Validated locally: make gen, targeted frontend tests for src/theme, src/contexts/ThemeProvider, and src/pages/UserSettingsPage/AppearancePage, backend tests TestUserThemeMode and TestUserTerminalFont, make fmt, make lint.
…an (#24672) This is part 1 to lay the foundation for the theme changes. Part 2 which adds the UI implementation is in this PR, #24680 ----------------- Adds four sitewide colorblind-friendly theme palettes alongside the existing light and dark themes. The palettes retune the red/green and blue/yellow semantic axes so success/error, warning, and diff additions/deletions remain distinguishable under the most common color vision deficiencies. | Preference ID | Purpose | |---|---| | `dark-protan-deuter` / `light-protan-deuter` | Protanopia & deuteranopia (red/green). Success and additions shift to sky-blue; destructive and deletions shift to vermilion/orange; warning shifts to fuchsia so warning and destructive states do not collapse onto the same hue. | | `dark-tritan` / `light-tritan` | Tritanopia (blue/yellow). Warning shifts from amber to fuchsia; red/green semantic pair is preserved. | The diff panel and every semantic role (`success`, `error`, `warning`, `notice`, `danger`) pick up the new palette automatically because they consume the sitewide CSS variables in `site/src/index.css`. No backend or database change is required: `theme_preference` is already a free-form `text` column. The existing `"dark"`, `"light"`, and `"auto"` preferences are unchanged. This PR ships the palettes and the resolution machinery (`CONCRETE_THEMES`, `resolveThemeName`, `isConcreteThemeName`, and the ThemeProvider/AgentEmbedPage plumbing). It intentionally does **not** add UI to select the new themes; the follow-up PR #24680 adds a Theme mode dropdown (Sync with system / Single theme) that exposes every concrete theme, including the four added here. Produced with Coder Agents assistance. <details> <summary>Implementation plan and decision log</summary> Full plan (investigation, file layout, TDD phases, open risks) is attached to the chat that produced this PR. Key decisions: - **Sitewide, not agent-scoped.** The diff panel already consumes the sitewide theme via CSS variables. Keeping the change at the user appearance layer also fixes red/green accents in alerts, badges, and build states in one change, and matches how comparable products (e.g. GitHub) ship this feature. - **No backend change.** `codersdk/users.go` already accepts any `theme_preference` string, and `ThemeProvider` now has a shared resolver (`resolveThemeName`) that maps any persisted value to a concrete theme, tolerating unknowns and the legacy `"auto"` value. - **Palette provenance.** The protan/deuter palette is inspired by CVD-safe blue/orange palettes and tuned within the existing Tailwind color scales; the tritan palette keeps red/green semantics intact and shifts warning to fuchsia. This PR does not claim exact Okabe-Ito or WCAG AA derivation without recorded contrast data. - **UI deferred.** Selecting the new themes is gated on the follow-up PR #24680 which replaces the flat theme grid with a Theme mode dropdown. </details>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 886e65205e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d614912a82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All 28 addressed findings from rounds 1-4 verified by the panel. Kite and Mafuuu independently confirmed all R4 fixes. CI is green. Netero found no mechanical issues.
3 P3 new this round, all in the test/edge-case layer. No blockers, no P0-P2.
The PR description still says "Colorblind variants carry a Beta badge" but the beta field was removed (DEREM-27 fix) rather than a badge being implemented. The description should be updated before merge to match the shipped behavior.
Across 5 rounds: 39 findings raised, 28 addressed in code, 8 dropped by orchestrator, 3 open (all P3), 0 contested, 0 deferred. Every fix was verified by at least two panel members. The author-agent's response quality has been consistently strong, addressing findings with targeted commits, regression tests, and clear commit messages.
Kite: "All 36 prior findings (DEREM-1 through DEREM-36) are closed and verified. No new findings."
🤖 This review was automatically generated with Coder Agents.
…9125408c # Conflicts: # site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx
1 participant
Replaces the flat theme grid on the Appearance settings page with a Theme mode dropdown that exposes two modes:
The persisted schema gains three typed fields on
UserAppearanceSettings:theme_mode("sync" | "single" | ""),theme_light, andtheme_dark. The pre-existingtheme_preferencecontinues to drive the runtime theme class so older clients keep working; insyncmode the new client rewritestheme_preferenceto match the currently active slot.The pre-existing
"auto"preference (from main) is migrated on read to the equivalent sync-mode pair (theme_light=light, theme_dark=dark). Users who had"auto"selected continue to see their theme follow the OS color scheme without any data migration.Depends on #24672.
Produced with Coder Agents assistance.
Storage migration
theme_preferenceautomode=sync, light=light, dark=darkdark)mode=single, theme=<name>mode=single, theme=DEFAULT_THEMEauto-protan-deuterandauto-tritanare handled defensively even though they were never exposed to users on main.The backend performs no migration. Six new sqlc queries mirror
GetUserTerminalFont/UpdateUserTerminalFontand use the existinguser_configskey-value table (no SQL migration, no audit entry). Emptytheme_modeon the write path defaults to"single"so older CLI clients that only sendtheme_preferencecontinue to work.