feat(ui): pair a light theme and a dark theme, switched by mode - #1217
Merged
Conversation
ThemeProvider stored one palette plus a mode, so picking a dark-only
palette pinned the mode and greyed out the Light/System buttons. Store a
pair instead: { mode, light, dark }, resolved as pair[preferredMode], so
System flips between the two choices as the OS scheme changes.
The Settings Theme tab now assigns one half at a time. A Light/Dark
switch decides which half the grid is filling, the grid lists only the
palettes that can render that half (from the registry's modeSupport), and
a summary line names both halves with each side clickable. Every mode
button is permanently enabled: a dark-only palette simply never occupies
the light slot, so no mode coercion is left to do.
The pair round-trips through the SETTINGS registry to the `theme` key in
~/.plannotator/config.json the way diffOptions does. A user upgrading
seeds both halves from their stored single palette, and the legacy
plannotator-color-theme key keeps tracking the active palette so a
downgrade never lands on an unstyled first frame.
Addresses part 1 of #1211.
…destructive Review of #1217 found a data-loss path and three published-API regressions. Seeding: ThemeProvider handed its resolved pair to the config store through set(), which queues a debounced POST. configStore.init() applies the server config but never cancelled that queued write, so a single cookie-less visit (fresh profile, incognito, cleared cookies) flushed a default pair to ~/.plannotator/config.json AFTER the real one had arrived, and the next session restored those defaults over the user's cookies. The provider now uses a new configStore.seed(): memory plus cookie, never the server, and never over a value init() already applied. init() additionally retracts queued writes for the leaves the server just spoke for, which closes the same race for every server-synced setting rather than this one key. Deprecated APIs: isThemeModeAvailable() and normalizeThemeMode() are back as one-line wrappers with @deprecated notes, since packages/ui exports utils/*. setColorTheme: assigns exactly one half and nothing else. A both-mode palette goes to the half on screen instead of clobbering both; a mode-restricted one goes to its half without yanking a System user to an explicit mode (render time already resolves that). It persists through configStore.setLocal(), so it stays cookie-only as it was before the pair, unless a host installed its own serverSync transport. storageKey / colorThemeStorageKey are honored on the read path, so a host's stored pre-pair preference is migrated rather than discarded. The two halves have no pre-pair equivalent and stay on fixed keys, documented on the props. Tests: a fresh-mount case that pins zero POSTs (the previous helper pre-seeded cookies, which is why this was invisible), a case that pins a real choice still reaching config.json, direct setColorTheme cases for all three semantics, a host-storage-keys migration case, and configStore seed/retract unit tests. All of them fail against the code they replace.
1 task
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
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.
TLDR: You can now pick which palette is your light theme and which is your dark theme, and System mode flips between the two as your OS scheme changes (Kanagawa Lotus by day, Kanagawa Wave at night).
ThemeProviderstores{ mode, light, dark }instead of one palette plus a mode, the Settings Theme tab assigns one half at a time, and the Light/Dark/System buttons are permanently enabled because a dark-only palette simply never occupies the light slot. The pair persists to~/.plannotator/config.jsonundertheme, so it survives the random port each hook invocation runs on.This implements the design @kunaaal13 wrote in #1211, adopted nearly as written.
What changed
packages/ui/utils/themeRegistry.ts: pair helpers (ThemePair,themesForHalf,themeSupportsHalf,seedThemePair,normalizeThemePair,resolvePairTheme).isThemeModeAvailable()andnormalizeThemeMode()are gone: there is nothing left to coerce.packages/ui/components/ThemeProvider.tsx: resolvespair[preferredMode](preferredModealready resolvedsystemagainstprefers-color-scheme) and exposeslightTheme/darkTheme/setHalfThemealongside the existingcolorTheme/setColorTheme.packages/ui/components/ThemeTab.tsx: a Light/Dark switch decides which half the grid is filling, the grid lists only the palettes that can render that half (from the registry's existingmodeSupport, withbothpalettes appearing in each half using that mode's swatches), and a summary line names both halves with each side clickable to jump the switch. Same tab in both apps, plus the compact preview strip.ThemeTab,ModeToggle,PlanHeaderMenu, andReviewHeaderMenu.bothpalette, so it is assignable to either or both halves.Someone who never opens the Theme tab sees no change at all: the default pair is today's default palette in both halves.
Config round-trip
The pair is one entry in the
SETTINGSregistry (themePair) withserverKey: 'theme', soconfigStorehandles cookie write, debouncedPOST /api/config, and the server override oninit()exactly as it does fordiffOptions.Server changes were needed, and landed in both runtimes:
getServerConfig()builds its payload from an explicit key list, sothemehad to be added (packages/shared/config.ts, vendored to Pi byvendor.sh).saveConfig()also shallow-mergesthemethe way it mergesdiffOptions.POST /api/configallowlists each key it will persist, sothemehad to be added to all seven handlers:packages/server/{index,review,annotate,goal-setup}.ts(Bun) andapps/pi-extension/server/server{Plan,Review,Annotate}.ts(Pi).Verified live across two runtimes of the same binary: a pair chosen in an annotate session was written to
config.jsonand picked up by a fresh code review session on a different port.Migration
plannotator-theme(mode) keeps its meaning and gainsplannotator-light-theme/plannotator-dark-theme.plannotator-color-themepalette: abothpalette fills the whole pair, a mode-restricted one fills the half it supports while the other half falls back toplannotator.ThemeProviderkeeps writingplannotator-color-themewith the palette actually on screen, so a version downgrade still finds a palette and never renders an unstyled first frame. The migrated pair is persisted before that mirror runs, since the mirror overwrites the key the migration was derived from.config.json(an unknown id, a dark-only palette in the light half,mode: "sepia") is repaired rather than trusted.Syntax highlighting
No work needed, as the issue predicted, and confirmed live:
resolveSyntaxTheme()/SHIKI_THEME_MAPare keyed on(colorTheme, mode), and resolving the pair earlier hands them the right pair member. Flipping the OS scheme in a review swaps both the UI palette and the diff's matched syntax colors.Review findings addressed
An adversarial review of the first push returned BLOCK. All five findings are fixed in
ca80f44f:ThemeProviderhanded its resolved pair to the store throughconfigStore.set(), which queues a debouncedPOST /api/config.configStore.init()applied the server config but never cancelled that queued write, so one cookie-less visit (fresh profile, incognito, cleared cookies) flushed a default pair toconfig.jsonafter the real one had loaded, and the next session then restored those defaults over the user's cookies. Two fixes: a newconfigStore.seed()writes memory plus cookie and never the server (and never over a valueinit()already applied), andinit()now retracts queued writes for the leaves the server just spoke for, which closes the same race for every server-synced setting instead of this one key. Audited the otherserverKeysettings:displayNamegeneration is cookie-only, every other write is a user-action handler, and the review editor's panel-pair self-heal deliberately runs afterinit()on a genuinely conflicted persisted pair.isThemeModeAvailable()andnormalizeThemeMode()are restored as one-line wrappers carrying@deprecatednotes that point atthemeSupportsHalf/resolveThemeMode, sincepackages/uiexports./utils/*.setColorThemesemantics. It now assigns exactly one half and changes nothing else: a both-mode palette goes to the half currently on screen (no longer clobbering the other half's assignment), a mode-restricted palette goes to the half it supports without pinning the mode (render-time resolution already keeps a System user on a drawable palette), and it persists through a newconfigStore.setLocal()so it stays cookie-only exactly as it was before the pair, unless a host installed its ownserverSynctransport.storageKey/colorThemeStorageKeyignored on the read path. Both props are now honored when resolving the initial pair, so a host's stored pre-pair preference is migrated instead of discarded, and the mirror writes the host's keys rather than Plannotator's. The two halves have no pre-pair equivalent and stay on fixed keys; that limitation is documented on the props.config.json. The pre-seeding helper is kept for the other tests. Also added directsetColorThemecases for all three semantics, a host-storage-keys migration case, andconfigStoreseed/retract unit tests. Every new test fails against the code it replaces (verified by reverting each fix in turn).Re-verified live afterwards: with
config.jsonholding{"theme":{"mode":"system","light":"rose-pine","dark":"kanagawa-wave"}}, a brand new browser context made zeroPOST /api/configcalls, left the file byte-identical, and still rendered the server-configured palette; the returning profile kept flipping Rosé Pine and Kanagawa Wave with the OS scheme, and assigning a half from Settings still round-trips to the file.Tests
bun test(3167 tests) andDOM_TESTS=1 bun test packages/ui packages/review-editor packages/editor(1013 tests) are green, as isbun run typecheck. New coverage:packages/ui/components/ThemeProvider.test.tsx: half filtering, migration seeding, pair repair, the pair flipping on a liveprefers-color-schemechange, every mode staying selectable while a dark-only palette owns the dark half, and aThemeTabwalk that assigns each half from its own grid.packages/ui/config/themePairSetting.test.ts: cookie round-trip, legacy seeding, half rejection, thethemekey round-trip in both directions, and theconfigStoreseed / queued-write-retraction contracts.Verified live
Built the real bundles (
bun run --cwd apps/review build && bun run build:hook) and drove the built app against a scratch git repo with Playwright, screenshotting the Theme tab assigning the light half, assigning the dark half, and the summary with System active, plus the app flipping between Kanagawa Lotus and Kanagawa Wave asprefers-color-schemeis emulated in both the plan/annotate surface and the review surface. The first-run dialog chain still appears one dialog at a time with no stacking, and the theme announcement dialogs are untouched. Bundle delta: +2.9 kB raw, +1.4 kB gzip on each of the two single-file bundles.Addresses part 1 of #1211. Parts 2 (custom user theme files) and 3 (font settings) stay open; the pair format part 2 depends on is what this PR establishes.
AI-assisted.