feat: add three-state dark mode - #1
Open
hitpopdimestop wants to merge 23 commits into
Open
Conversation
Adds a --color-scrim token: the modal backdrop is a third meaning of slate-950 that must stay dark in both themes, so it cannot reuse surface-inverted.
React reconciles the server-rendered data-theme onto <html>, discarding what the blocking inline script wrote; suppressHydrationWarning does not prevent the attribute patch.
The inline script traded two defects for the flash it prevented: React 19 errors on any <script> rendered in a component, and suppressHydrationWarning does not cascade, so the toggle's lucide icon still mismatched between server and client. Both appeared on every load with an explicit preference, and both were invisible to the previous verification because it ran a production build. Theming now runs on color-scheme. :root declares `light dark` and every token is defined once with light-dark(), so the OS setting is honoured during initial parse with no JavaScript at all. An explicit choice sets data-theme, which narrows color-scheme to one value; `system` is the absence of the attribute. This also lets the browser theme its own canvas, scrollbars and form controls. useSyncExternalStore reports `system` for the hydrating render and the stored value afterwards, which removes the hydration mismatch by construction rather than silencing it, and replaces both the mounted flag and the manual storage listener. resolveTheme is gone; CSS owns resolution now. Visitors who override their OS setting see it applied after hydration, with a transition. Every application animates, so there is no special case. Adds an e2e guard asserting zero console errors in all three preferences — the coverage whose absence let both defects through.
The inline script traded two defects for the flash it prevented: React 19 errors on any <script> rendered in a component, and suppressHydrationWarning does not cascade, so the toggle's lucide icon still mismatched between server and client. Both appeared on every load with an explicit preference, and both were invisible to the previous verification because it ran a production build. Theming now runs on color-scheme. :root declares `light dark` and every token is defined once with light-dark(), so the OS setting is honoured during initial parse with no JavaScript at all. An explicit choice sets data-theme, which narrows color-scheme to one value; `system` is the absence of the attribute. This also lets the browser theme its own canvas, scrollbars and form controls. useSyncExternalStore reports `system` for the hydrating render and the stored value afterwards, which removes the hydration mismatch by construction rather than silencing it, and replaces both the mounted flag and the manual storage listener. resolveTheme is gone; CSS owns resolution now. The switch is instant. A CSS transition cannot animate it — light-dark() follows color-scheme, which is not animatable, so the colours change discretely and nothing interpolates. A view transition did animate it but brought aborted-transition errors and unhandled rejections from its skipped promises, and the result was not perceptible in practice. next-themes ships disableTransitionOnChange for much the same reasons. Adds an e2e guard asserting zero console errors in all three preferences — the coverage whose absence let the original defects through.
The README still advertised a pre-paint inline script, which no longer exists. Replace it with what the module actually does, and note that resolution belongs to CSS rather than this code. architecture-overview.md enumerates the codebase by runtime ownership and was missing src/features/theme/ entirely. Record it, and why it sits outside the dashboard's client-state layers. testing-strategy.md specifies required coverage, so add the theme cases the implementation now carries — notably the console-error assertion, which is the only place a hydration mismatch is visible.
The theme module was written in a far more annotated register than the rest of the code, which carries almost no comments. Keep only the notes covering genuinely non-obvious contracts — the server snapshot pinning hydration to `system`, `system` meaning the attribute's absence, `storage` firing only in other tabs, and why a console-error test exists — and drop the rest.
The subtle-text contrast fix left --content-subtle and --content-muted resolving to the same value in both themes, so the vocabulary claimed a distinction the UI no longer made. Merge them: the three usage sites read as de-emphasised through size, casing and letterspacing rather than colour, which the rendered output confirms. Split e2e/contrast.ts by dependency. The WCAG maths is pure and now lives in test-utils/ with a vitest suite covering non-sRGB rejection, alpha handling and symmetry; renderedColors keeps the Playwright Locator work. Previously its unit test sat in the Playwright suite, so asserting a pure function threw required a full production build first. Drop the committed superpowers plan: this project predates that workflow and should not mix conventions.
The strategy specifies required coverage, so the cases the theme work added belong in it: the session fallback when storage writes fail, a cross-tab change applied when reads fail, and AA contrast for helper text on normal and inverted surfaces. Also record two conventions the contrast work established — normalizing to sRGB before measuring, since getComputedStyle now returns lab()/oklch() and digit-scraping a non-sRGB string yields a confident wrong ratio, and keeping test tooling split so a pure assertion never requires a production build.
The rule read as an unexplained prohibition, which made it look obvious while hiding the reason it needed stating. Cross-tab sync everywhere else in this app goes through SSE, so reusing that machinery for theme is the tempting move — and it would be wrong, because the store is a single process-wide singleton whose broadcast reaches every connected visitor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a three-state (system / light / dark) theme to the Subscription Billing Runner, with no flash before hydration and cross-tab synchronization.
What changed
docs/frontend.mdgains a## Themesection defining the preference values, thesubscriptions:themestorage key, the always-concretedata-themeattribute, the pre-paint requirement, cross-tab propagation, and the explicit non-goal that theme never touches SSE, the store, or any server route.src/features/theme/(new):theme.ts(pure logic — parse/resolve/cycle),theme-script.tsx(blocking inline<head>script),use-theme.ts(client hook),theme-toggle.tsx(three-state control in the dashboard header).src/app/globals.css: light values on:root, dark on[data-theme="dark"], exposed as Tailwind utilities via@theme inline. Intermediate names (--border-subtle) avoid colliding with Tailwind's namespace.constants.ts,formatters.ts, the 9 dashboard components, anddashboard.tsx. Thetransaction-cardenter-flash keyframe moved off raw hex and unified onemerald.Design decisions
The token collapse is intentionally lossy.
text-slate-500/600/700all becamecontent-muted;bg-amber-50andbg-amber-100both becamewarn-surface. Some elements shift by one shade. That is the point of a token system — no extra tokens were invented to preserve every original shade.slate-950was disambiguated by role, not find-and-replace.focus:border-slate-950on inputs is a focus affordance →border-strong.border-slate-950 bg-slate-950 text-whiteon selected/primary controls is an inverted surface →surface-inverted/content-inverted.hover:bg-slate-800on the primary button becamehover:opacity-90, since a single darker-slate hover does not invert correctly across both themes.Deviations from the plan — please read
1. The plan's theme script alone did not survive hydration. Added a fix (
aa61c3f).The plan assumed
suppressHydrationWarningon<html>would let the inline script'sdata-themestand. It does not: React reconciles the server-rendereddata-theme="light"back onto<html>during hydration, andsuppressHydrationWarningonly suppresses the console warning, not the attribute patch. Becauseuse-theme.tsas specified calledapplyPreferenceonly when the preference wassystem(or on astorageevent), an explicitlight/darkpreference was silently reverted on every load.Measured with Playwright against a production build, stored preference
dark:Fix: the mount effect in
use-theme.tsnow re-assertsapplyPreference(preference)for every preference before deciding whether to subscribe to the media query. This is the same approachnext-themesuses. The pre-paint guarantee is unaffected — the inline script still wins the initial paint, which thewaitUntil: "commit"e2e assertion covers.2. Added one token the plan's table omitted:
--color-scrim.The pause dialog backdrop is
bg-slate-950/45— a third meaning ofslate-950that the Migration Hazards section did not list. Applying the table literally (surface-inverted) turned the modal scrim into a light wash over the dark page: the backdrop brightened instead of dimming. A scrim must darken in both themes, so it cannot invert.--color-scrimis defined once on:rootasvar(--color-slate-950)with no dark override. This is a distinct semantic role, not a shade-preservation token.3.
vitest.setup.tsneeded amatchMediastub.jsdom does not implement
window.matchMedia, so mounting the header indashboard.test.tsxthrew onceThemeTogglewas added. Added a guarded light-mode stub (only applied whenwindowexists and the API is missing).4. Updated an existing assertion in
dashboard.test.tsx.It asserted the literal classes
bg-amber-50/bg-slate-50on transaction cards; retargeted tobg-warn-surface/bg-surface-muted.5. Collapsed a now-redundant ternary on the pause dialog's custom-seconds input, whose two branches became character-identical after migration.
Contrast pass (Task 9)
Computed WCAG ratios from the actual Tailwind oklch palette rather than estimating:
content-mutedonsurface(slate-400/slate-900)content-subtleonsurface(slate-500/slate-900)contentonsurfacewarn/success/dangercontent on their surfacesNo token values were changed. The plan's conditional remedy (lightening
content-subtleand shiftingcontent-mutedto slate-300) was not triggered:content-subtleis used only for secondary text and a decorative dot separator, and 3.74:1 clears the 3:1 bar the plan set for secondary text.content-mutedat 6.79:1 clears 4.5:1 comfortably.Pre-existing issue, not introduced here and not fixed here: in light mode,
content-subtleonsurface(slate-400 on white) is 2.63:1, below even the 3:1 secondary bar. This is the originaltext-slate-400intransactions-panel.tsx, carried over unchanged so light mode keeps its current appearance. Worth a follow-up.Verification
yarn test— 62 passed, including 8 new intheme.test.tsyarn test:e2e— 7 passed, including 3 new theme specs (persistence across reload, pre-hydration application, cross-tab sync)yarn buildandyarn lintcleansrc/returns nothing; no raw hex insrc/tailwind.config.jsgrep -i themeoversrc/server/,src/shared/,src/app/api/returns nothing — theme never reaches SSE, the store, or a server route🤖 Generated with Claude Code