Merged
Conversation
Design spec for the setTheme() API plus the matchMedia fix for auto mode. Captures the decisions (void return, console.warn on bad input, no events, bgColor re-derivation), the architecture (new theme.ts module), the three runtime paths (init / setTheme / OS change), edge cases, and the unit + E2E test plan.
17-task TDD-ordered plan covering the new theme.ts module, the injectStyles refactor, the setTheme API wiring, the matchMedia listener, vitest unit coverage, the 6-case Playwright E2E, the pre-PR review gate, and PR open. Each task is bite-sized with explicit verification commands. Drives the implementation session.
Scaffold e2e/theme.spec.ts with three cases covering the runtime theme API:
setTheme("dark") adds bd-dark, setTheme("light") removes it, and an invalid
input logs a warning without changing the class list.
Add a tiny inline harness to public/test/index.html that reads ?theme= and
?bg= query params and overrides the corresponding data-* attributes on the
BugDrop script tag. The widget script is marked `defer` so the harness —
which runs synchronously during parsing, after the widget tag is already
in the DOM — can set the attributes before the widget reads them.
Addresses feedback from pre-PR review gate:
- attachSystemThemeListener now warns when window.matchMedia is unavailable,
so integrators in sandboxed iframes or restrictive CSP environments can
debug why data-theme="auto" stops tracking OS changes.
- The matchMedia change handler wraps its callback in try/catch so a throw
(e.g. from a detached DOM root) doesn't propagate into the browser's event
loop and stop subsequent events from being processed.
- applyCustomStyles adds a Number.isFinite guard on the parsed borderWidth
so invalid input no longer produces 'NaNpx' in the shadow offset.
- setTheme's invalid-input warn uses String(mode) instead of JSON.stringify
so circular objects don't turn a bad-input warning into a runtime error.
- Module-level _currentMode and _detachSystemListener are gone. currentMode
is now closure-captured inside exposeBugDropAPI, giving per-widget-instance
isolation and removing the far-away _currentMode = config.theme assignment.
Adds two E2E tests covering gaps the test analyzer flagged:
- explicit setTheme('light') resists a subsequent OS flip to dark (proves
the currentMode !== 'auto' gate in the matchMedia callback works).
- auto mode + bgColor re-derives --bd-bg-secondary on OS theme flip (proves
the full matchMedia callback path, both applyThemeClass and applyCustomStyles,
runs end-to-end for users combining data-theme="auto" with data-bg="...").
Knip in CI flagged both as unused exported types because nothing outside src/widget/theme.ts imports them — they exist purely for the module's internal type safety (function signatures and interface shape). Dropping the `export` keyword keeps the types in place for internal type checking and satisfies knip. ThemeMode stays exported because src/widget/index.ts imports it to type the setTheme method signature.
|
🎉 This PR is included in version 1.28.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Closes #104
Summary
window.BugDrop.setTheme('light' | 'dark' | 'auto')so host apps can sync the widget theme whenever their own theme toggle changes (concrete use case: Seatify).data-theme="auto"never followed OS theme changes after init — the widget now installs amatchMedia('(prefers-color-scheme: dark)')listener at init, gated bycurrentMode === 'auto', so widgets inautomode track OS theme changes from then on.injectStylesinto a new focusedsrc/widget/theme.tsmodule (~150 lines).src/widget/ui.tsshrinks by ~80 lines.Design
Full design spec:
docs/superpowers/specs/2026-04-15-runtime-theme-api-design.md.Full implementation plan:
docs/superpowers/plans/2026-04-15-runtime-theme-api.md.Locked-in decisions:
voidreturn,console.warnon invalid input, no events (YAGNI), always-on matchMedia listener gated by mode check,bgColor-derived inline styles re-applied on every theme change via a consolidatedapplyCustomStyleshelper. See the spec for rationale.Module boundary
New
src/widget/theme.tsexports:ThemeMode = 'light' | 'dark' | 'auto'andResolvedTheme = 'light' | 'dark'— the split keeps "user intent" and "resolved DOM state" as distinct types soapplyThemeClasscan never receive'auto'.ThemeConfigSlice— minimal structural subset ofWidgetConfig(6 optional fields) used byapplyCustomStyles, declared locally to avoid an import cycle.resolveTheme,isValidTheme,getSystemTheme,applyThemeClass,applyCustomStyles,attachSystemThemeListener— all 6 exports fully tested.Hardening (from pre-PR review gate)
The pre-merge review gate (5
pr-review-toolkitagents in parallel — code-reviewer, pr-test-analyzer, code-simplifier, silent-failure-hunter, type-design-analyzer) surfaced several defense-in-depth items, all addressed in the final commit:attachSystemThemeListenernowconsole.warns whenwindow.matchMediais unavailable, so integrators in sandboxed iframes / restrictive CSP environments can debug whyautomode stops tracking.applyCustomStylesadds aNumber.isFiniteguard on the parsed border width — protects against'NaNpx'leaking into the shadow offset when a user passesdata-border-width="invalid".setTheme's invalid-input warn usesString(mode)instead ofJSON.stringify(mode)— avoids throwing on circular objects._currentModeand_detachSystemListenerwere removed.currentModeis now closure-captured insideexposeBugDropAPI, giving per-widget-instance isolation and removing the far-away assignment.Deferred (not addressed in this PR)
shadow?: stringto a literal union (type-design-analyzer suggestion) — real win but touchesWidgetConfigin multiple files. Follow-up PR.data-themescript-attribute validation (code-reviewer observation) — the init path accepts any string indata-theme, which is pre-existing behavior from before this PR. Follow-up PR.destroy()API — out of scope.bugdrop:themechange) — YAGNI per spec; no concrete consumer.Test plan
Unit (vitest,
test/theme.test.ts, 43 tests, jsdom environment):resolveTheme— light/dark passthrough, auto via injected probe, auto via default-parameter fallbackisValidTheme— 3 accepts + 8 rejectsgetSystemTheme— returns light/dark based on mockedmatchMedia, falls back to light when unavailableapplyThemeClass— adds/removesbd-dark, idempotent in both directionsapplyCustomStyles— 14 cases covering the no-op branch + accent/bg/text/border/shadow blocks, including theme-dependent fallbacks (bgBase,shadowColor) and re-run overwrite semanticsattachSystemThemeListener— no-op cleanup whenmatchMediamissing, dark/light callback dispatch, cleanup stops subsequent callsE2E (Playwright,
e2e/theme.spec.ts, 8 tests):setTheme('dark')flips the root classsetTheme('light')removesbd-darkpage.emulateMedia(the issue Support runtime theme switching via JavaScript API #104 proof-of-life)setTheme('auto')resolves to the current emulated OS themebgColor+setThemere-derives--bd-bg-secondaryviacolor-mix(imperative path)setTheme('light')resists subsequent OS flip to dark (gate-behavior proof — added from review gate)bgColorre-derives on OS flip (full callback integration — added from review gate)Full local verification just before PR open:
npm run lint— 0 errors, 20 warnings (pre-existing)npm run typecheck— cleannpm test— 109 passingnpm run build:widget— widget.js at 67.2 kbnpx playwright test e2e/theme.spec.ts— 8 passing in 2.0snpx playwright test e2e/widget.spec.ts— 91 passing (no regression)Release validation
This PR is a
feat:(minor version bump). Beyond shipping the feature, it serves as the first real-world validation of the newdeploy.ymlwith-release path from #111 — after merge:.github/workflows/deploy.ymlfires onpush: mainreleasejob publishes a new minor tag (e.g.v1.15.0)deployjob rebuilds with the newVERSIONand ships to Cloudflare WorkersWe'll capture wall-clock for the post-merge phase as the first datapoint for the "with-release" baseline (the
fc8749emerge from #111 gave us the "no-release" baseline of 57s; this one will be measurably higher because it actually publishes).Commit log (15 commits)
8e9ac4e feat(theme): scaffold theme module and test file (#104)bd340ce feat(theme): add isValidTheme predicate (#104)6e74ae9 feat(theme): move getSystemTheme into theme module (#104)6de157b feat(theme): add resolveTheme helper (#104)d58558a feat(theme): add applyThemeClass helper (#104)1dacdd8 feat(theme): add applyCustomStyles helper (#104)9e4c28c feat(theme): add attachSystemThemeListener wiring (#104)db21b24 refactor(widget): delegate theme resolution and custom styles to theme module (#104)110c260 feat(widget): add window.BugDrop.setTheme runtime API (#104)b04ef27 feat(widget): auto-follow OS theme changes via matchMedia listener (#104)97b456b test(e2e): add setTheme happy-path and invalid-input cases (#104)11c209c test(e2e): verify auto mode follows OS theme changes (#104)bec7106 test(e2e): verify bgColor re-derives on runtime theme change (#104)446f601 fix(theme): harden listener + closure-scope mode state (#104)