feat(site): add colorblind-friendly themes for protan/deuter and tritan#24672
Conversation
10c1db5 to
01df266
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Solid architecture. The resolver/registry/CSS-variable test triangle is well-constructed: resolveThemeName always returns a key that exists in the themes registry (proven by the cartesian-product test), and every CSS class block declares the full semantic variable set (proven by cssVariables.test.ts). The ThemePreview component scoping via nested <div className={theme}> is a clean way to give each tile accurate CSS variable colors. The radiogroup wrapper is a real a11y improvement.
1 P1, 2 P2, 8 P3, 4 Nit. 4 P4 findings in the inventory but below the inline threshold.
The P1 is a functional breakage: dark colorblind themes do not activate Tailwind's dark: variant, so tool icons in the Agents chat are invisible (near-black on dark background). The fix is small (add dark class alongside the concrete theme class for dark variants).
The P2 findings are a stale embed comment and an unverified Okabe-Ito palette claim in the code comments.
"If a bug routed to the wrong variant, the assertion would silently accept it." (Hisoka, on the
\blight\bregex)
Non-posted findings in inventory: DEREM-12 (P4, extractBlock fragility), DEREM-13 (P4, REQUIRED_VARIABLES coverage gap), DEREM-14 (P4, no embed page test), DEREM-15 (P4, CSS/JS dual representation drift risk, pre-existing).
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c1973eda3
ℹ️ 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.
2c1973e to
cf99301
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.
|
Coder Agents here. Addressed the review feedback in 3c361f0. Summary by comment: P1 / DEREM-1 (Codex + coder-agents-review) — fixed. P2 / DEREM-2 — fixed. P2 / DEREM-3 — fixed. P3 / DEREM-4 (+ Codex P3) — fixed. Nit / DEREM-18 — fixed. Nit / DEREM-19 — fixed. Out of scope on this PR — not changed.
Intentionally not acted on (architectural choice).
Produced with Coder Agents assistance. |
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.
|
Coder Agents follow-up on the two Codex review items: Codex P1 ( Codex P3 ( Produced with Coder Agents assistance. |
|
/coder-agents-review |
There was a problem hiding this comment.
Re-review after the author addressed 7 of 16 R1 findings (DEREM-1 through -4, -18, -19 fixed; -10 and -11 contested and closed by panel). Seven findings were dropped because the AppearanceForm code was removed from this PR and deferred to PR #24680.
The R1 fixes look correct. The baseModeFor helper, the classList.contains assertion tightening, the comment updates, and the CSS variable cleanup all land cleanly.
This round surfaced 1 P0, 1 P1, 2 P2, 4 P3.
P0 [DEREM-20] (site/src/index.css): The @layer components { ... } block was deleted from index.css, removing the mobile-full-width-dropdown CSS rules. Nine live components still reference these classes: WorkspacePill.tsx:124, ContextUsageIndicator.tsx:267, AgentsSidebar.tsx:994, ChatTopBar.tsx:191, AgentPageHeader.tsx:167, ModelSelector.tsx:132, CompactOrgSelector.tsx:86, AgentChatInput.tsx:851,1148. Every mobile dropdown on the agents page breaks. The deletion rode in with the navbar-stripe cleanup (also unrelated to colorblind themes). Fix: restore the @layer components { @media (max-width: 767px) { ... } } rules. (Netero)
The P1 is a palette-design problem: in the protan-deuter CSS blocks, --content-warning and --content-destructive are 3 degrees apart on the same orange axis. The palette designed to help CVD users makes warning and error indistinguishable for them in banners, badges, and alerts. The PR description says "warning and diff additions/deletions remain distinguishable"; the CSS variables say otherwise.
"Moving destructive onto orange puts it on top of warning." (Pariston, tracing the in-block hue collision)
DEREM-10 and DEREM-11 (contested directory structure and roles duplication): Pariston evaluated both and accepted the author's defense. Panel closed.
site/.storybook/preview.tsx:97
P2 [DEREM-27] The Storybook decorator at this line sets document.querySelector("html")?.setAttribute("class", selected) with a single token. The production ThemeProvider (fixed in 3c361f0 for DEREM-1) now adds both concreteName and baseModeFor(concreteName). This sibling was not updated.
Consequence: selecting dark-tritan in Storybook leaves .dark off the root, so dark: Tailwind utilities and .dark selector-based theming break, the same class of bug DEREM-1 fixed in production. Fix: import baseModeFor and set both classes. (Mafuuu)
🤖
🤖 This review was automatically generated 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.
|
Coder Agents follow-up: addressed the remaining active PR comments in da2f585. Fixed:
Validation:
Produced with Coder Agents assistance. |
|
Coder Agents follow-up after the required deep-review pass:
Validated:
|
|
/coder-agents-review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
There was a problem hiding this comment.
Re-review after the author addressed all 8 R2 findings in da2f585 and follow-up hardening commits. Pariston verified every R2 fix against the current code and confirmed they address root causes. The palette hue separations, the baseModeFor sibling in Storybook, the restored mobile-dropdown rules, the syntax token coverage, and the tritan danger/error restoration all look correct.
1 P2, 2 P3 this round. All are test-infrastructure gaps, not production breakage.
The P2 is the sibling-miss pattern recurring for the third time: commit 0716dc0 fixed the manually-synced theme list in the e2e test by importing CONCRETE_THEMES, but did not search for other instances. cssVariables.test.ts has the same hardcoded list at line 58.
Process observation: the e2e theme assertion took 5 iterations across 5 commits to stabilize (toContain → classList.contains → toEqual → toContain+exclusion → import CONCRETE_THEMES). The final result is correct, but each step was a reaction to the previous step breaking rather than reasoning about the invariant upfront.
"I tried to build a case against this change and could not. The problem is correctly understood, the solution intervenes at the right level, and the palette choices are defensible." (Pariston)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 4. All three R3 findings addressed in 9dd284f. Mafuuu verified the contract chain from resolver through registry to cleanup. Pariston verified CSS specificity ordering, variable completeness, type safety bidirectionality, and tried four alternative framings of the problem; none held better than the author's approach.
1 P2 this round. The tritan palette shifts warning text, borders, and highlights to fuchsia but leaves --surface-orange on amber. Components like Badge, Alert, and LicenseBanner pair bg-surface-orange with fuchsia warning foregrounds, producing an amber background with fuchsia text. Amber is the exact hue tritanopia desaturates.
"The surface on which the fuchsia text sits is the one amber token that was left behind." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 5. DEREM-34 (tritan --surface-orange) addressed in 0ad35a2. Netero verified the fix: both tritan blocks now shift --surface-orange to fuchsia, matching the rest of the warning family, with a new test assertion guarding the invariant.
34 findings across 5 rounds, all resolved. 19 fixed by author, 9 dropped (code removed from PR/deferred to #24680), 2 contested and panel-closed, 4 dropped below threshold. Zero open.
The resolver/registry/CSS-variable test triangle is sound. The palette hue separations hold for both CVD types. The baseModeFor helper, the Storybook decorator, and the embed page all correctly apply dual classes. The satisfies Record<ConcreteThemeName, Theme> constraint plus the bidirectional registry test enforce compile-time and test-time sync. The semantic separation tests cover all four colorblind variants for warning/destructive and link/success distinctness.
🤖 This review was automatically generated with Coder Agents.
aslilac
left a comment
There was a problem hiding this comment.
I think there are bits that could be simplified a bit, and I think we'd get a lot more out of snapshots of these colorblind themes than a unit test attempting to parse using regular expressions.
| * | ||
| * The four colorblind variants (`dark-protan-deuter`, `light-protan-deuter`, | ||
| * `dark-tritan`, `light-tritan`) sit alongside the standard `dark` and | ||
| * `light` themes. The legacy `auto` preference is virtual: it resolves to |
There was a problem hiding this comment.
auto is not legacy, it is an important option
if anything, we should also have an auto-protan-deuter and an auto-tritan
There was a problem hiding this comment.
removed the comment, I don't think of auto as a theme but a way of arriving at a theme.
There was a problem hiding this comment.
yeah, that's true. I usually referred to the selection here as a "preference" rather than a specific "theme".
| import type { Decorator, Loader, Parameters } from "@storybook/react-vite"; | ||
| import isChromatic from "chromatic/isChromatic"; | ||
| import { StrictMode } from "react"; | ||
| import React from "react"; |
There was a problem hiding this comment.
I'd have to go digging but I remember the React team saying that one day there were gonna remove the default export. curious about this change, I'd rather we leave it as it was
| import React from "react"; | |
| import { StrictMode } from "react"; |
There was a problem hiding this comment.
this error shows up in the jsx of preview.tsx, reverted for now and we can handle this separately.
'React' refers to a UMD global, but the current file is a module. Consider adding an import instead.
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.
dark-protan-deuter/light-protan-deuterdark-tritan/light-tritanThe diff panel and every semantic role (
success,error,warning,notice,danger) pick up the new palette automatically because they consume the sitewide CSS variables insite/src/index.css. No backend or database change is required:theme_preferenceis already a free-formtextcolumn.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.
Implementation plan and decision log
Full plan (investigation, file layout, TDD phases, open risks) is attached to the chat that produced this PR.
Key decisions:
codersdk/users.goalready accepts anytheme_preferencestring, andThemeProvidernow has a shared resolver (resolveThemeName) that maps any persisted value to a concrete theme, tolerating unknowns and the legacy"auto"value.