Skip to content

Add light mode toggle to Studio shell chrome (1/3) - #506

Merged
jackgranatowski merged 1 commit into
mainfrom
claude/studio-light-mode-1-infra
Jul 3, 2026
Merged

Add light mode toggle to Studio shell chrome (1/3)#506
jackgranatowski merged 1 commit into
mainfrom
claude/studio-light-mode-1-infra

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

First of a 3-part light-mode rollout for the SLASHED Studio configurator, split from #500 for reviewability. Stack: 1/3 (this PR) → #TBD (2/3) → #TBD (3/3).

Summary

Adds a light/dark theme toggle to the Studio chrome and applies it to the app shell. Dark remains the default appearance until the user toggles or their OS is already light; panels/inputs get their light-mode pass in the two follow-up PRs in this stack (currently unstyled for light mode until those land — expected/acceptable interim state since dark stays the default).

  • New configurator/src/lib/theme.svelte.ts: reactive theme state that defaults to (and live-follows) prefers-color-scheme until the user explicitly toggles, then persists the choice to localStorage.
  • main.ts binds the theme to the mount root before Svelte mounts, so the first paint is never wrong-themed (no flash).
  • Tailwind v4 class-based dark: variant (@custom-variant dark) added in app.css, kept independent of the framework's own [data-theme] (which only governs the dogfooded --sf-* tokens loaded by main.ts).
  • Sun/Moon toggle button added to StudioHeader.svelte.
  • Shell components retrofitted for both themes: App.svelte, StudioHeader, SidebarNav, StatusBar, PreviewPanel, CommandPalette, DomainPanel.

Review-bot fixes folded in

Several bots reviewed the original combined PR (#500) before this split. The following real findings are fixed here:

  • StudioHeader: theme toggle now has aria-label/aria-pressed; save-state error/saved chips get a light-mode background (were dark-only, producing unreadable text in light mode).
  • theme.svelte.ts: feature-detects MediaQueryList.addEventListener with a fallback to the deprecated addListener (Safari < 14), so an unusual environment can't throw at module-import time.
  • CommandPalette: a domain badge was missing its dark: text-color pairing.
  • StatusBar: the domain label had a contrast bug in light mode (text-slate-300 on bg-slate-100), fixed to match the file's existing muted-text convention.

Not actioned (raised, considered, deliberately skipped): a suggestion to scope the dark: variant to a component-specific attribute instead of a .dark class, to guard against a coincidental .dark class from an unrelated embedding host. The bot's own alternatives analysis recommended keeping the current approach; flagging here for visibility rather than adding speculative complexity.

Testing

  • npx svelte-check — 0 errors
  • npx vitest run — 113/113 passing
  • Playwright: verified toggle mechanics (persistence, OS-follow, first-paint no-flash) and visual pass across Home/Colors/Typography/Borders/Effects panels and the command palette in both themes.

Generated by Claude Code

First of a 3-part light-mode rollout (split for reviewability). Adds
the theme system and applies it to the app shell: header (with the new
Sun/Moon toggle), sidebar nav, status bar, preview panel toolbar,
command palette, and domain panel tab bar. Dark remains the default
appearance; panels/inputs get their light-mode pass in the follow-up
PRs in this stack.

- New `lib/theme.svelte.ts`: reactive theme state, defaults to (and
  live-follows) `prefers-color-scheme` until the user explicitly
  toggles, then persists to localStorage.
- `main.ts` binds the theme to the mount root before Svelte mounts, so
  the first paint is never wrong-themed.
- Tailwind v4 class-based `dark:` variant (`@custom-variant dark`) in
  `app.css`, kept separate from the framework's own `[data-theme]`
  (which only governs the dogfooded `--sf-*` tokens).
- Includes a couple of small hardening/accessibility fixes surfaced by
  review bots on the original combined PR: aria-label/aria-pressed on
  the toggle, light-mode backgrounds for the save-state error/saved
  chips, a feature-detect fallback for the deprecated
  MediaQueryList.addListener API, and a missing dark: pairing on a
  command-palette badge.
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jackgranatowski, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ca9822b5-1f7c-43db-805f-d84c1e2f25c1

📥 Commits

Reviewing files that changed from the base of the PR and between 5f34a8a and 57dbcec.

📒 Files selected for processing (10)
  • configurator/src/App.svelte
  • configurator/src/app.css
  • configurator/src/components/CommandPalette.svelte
  • configurator/src/components/DomainPanel.svelte
  • configurator/src/components/shell/PreviewPanel.svelte
  • configurator/src/components/shell/SidebarNav.svelte
  • configurator/src/components/shell/StatusBar.svelte
  • configurator/src/components/shell/StudioHeader.svelte
  • configurator/src/lib/theme.svelte.ts
  • configurator/src/main.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/studio-light-mode-1-infra

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add light/dark theme toggle for Studio shell chrome

✨ Enhancement ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Add reactive theme state with OS-follow + localStorage persistence
• Bind .dark to the mount root before Svelte mounts to avoid first-paint flash
• Retrofit Studio shell chrome components for light-mode-safe colors and contrast
Diagram

graph TD
  A["main.ts"] --> B["theme.svelte.ts"] --> C["Mount root (.dark class)"] --> D["Tailwind dark: variant"]
  E["StudioHeader toggle"] --> B
  B --> F["localStorage"]
  B --> G["prefers-color-scheme"]
  D --> H["Shell chrome components"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use a component-scoped attribute for dark mode (e.g. [data-studio-theme])
  • ➕ Avoids potential collisions if an embedding host also uses a .dark class
  • ➕ Makes the dark-mode scope explicit in the DOM
  • ➖ More bespoke Tailwind wiring and mental overhead vs the conventional .dark approach
  • ➖ Harder interoperability with existing dark: expectations and tooling
2. Rely solely on prefers-color-scheme (no persistence / toggle)
  • ➕ Simpler state management; no storage concerns
  • ➕ Always matches OS preference
  • ➖ No user override when OS preference is undesirable
  • ➖ Does not meet the stated requirement for an in-app toggle
3. Drive all theming via framework token layer ([data-theme] + CSS variables)
  • ➕ Single source of truth for theme across chrome and components
  • ➕ Can reduce duplicated light/dark Tailwind classes long-term
  • ➖ Larger refactor; higher risk and broader blast radius
  • ➖ Not aligned with the incremental 3-PR rollout plan

Recommendation: Keep the current .dark class approach: it is conventional for Tailwind, easy to reason about, and the early bindThemeRoot() call prevents first-paint flash. The attribute-scoping alternative is viable if embedding collisions become a real issue, but adds complexity prematurely.

Files changed (10) +176 / -76

Enhancement (8) +167 / -72
App.svelteApply light-mode-safe base chrome colors and mobile tab styles +7/-7

Apply light-mode-safe base chrome colors and mobile tab styles

• Updates the app shell background/text colors to support light mode with 'dark:' fallbacks. Adjusts mobile fold toggle and domain panel header styles for light-mode contrast.

configurator/src/App.svelte

CommandPalette.svelteTheme the command palette container, text, and borders +16/-16

Theme the command palette container, text, and borders

• Adds light-mode backgrounds, border colors, and improved hover/selection styling with 'dark:' counterparts. Fixes a missing dark-mode pairing on a domain badge and improves placeholder/aux text contrast.

configurator/src/components/CommandPalette.svelte

DomainPanel.svelteTheme the mobile tab bar for light/dark +6/-6

Theme the mobile tab bar for light/dark

• Updates the bottom tab bar background/border colors and active/hover states so tabs remain readable in light mode while preserving existing dark styling.

configurator/src/components/DomainPanel.svelte

PreviewPanel.svelteTheme preview toolbar controls and split-view headers +20/-20

Theme preview toolbar controls and split-view headers

• Retrofitted toolbar groups, buttons, separators, and select styling for light mode with 'dark:' fallbacks. Also updates split-view header bars and constrained preview background to avoid low-contrast combinations.

configurator/src/components/shell/PreviewPanel.svelte

SidebarNav.svelteTheme sidebar nav background, separators, and hover states +4/-4

Theme sidebar nav background, separators, and hover states

• Adds light-mode background/border colors and updates hover feedback to remain visible in both themes while preserving active item styling.

configurator/src/components/shell/SidebarNav.svelte

StudioHeader.svelteAdd theme toggle button and light-mode header styling +36/-19

Add theme toggle button and light-mode header styling

• Adds a Sun/Moon toggle wired to the shared theme state with 'aria-label' and 'aria-pressed'. Updates header chrome colors and fixes light-mode readability for saved/error state chips and dialog styling.

configurator/src/components/shell/StudioHeader.svelte

theme.svelte.tsIntroduce theme state with OS-follow and persisted user override +74/-0

Introduce theme state with OS-follow and persisted user override

• Implements a reactive light/dark theme state that defaults to 'prefers-color-scheme' until the user toggles, then persists to localStorage. Binds theme application to a root element and listens for OS changes with a Safari <14 'addListener' fallback.

configurator/src/lib/theme.svelte.ts

main.tsBind theme root pre-mount to prevent wrong-themed first paint +4/-0

Bind theme root pre-mount to prevent wrong-themed first paint

• Calls 'bindThemeRoot(target)' before mounting the Svelte app so the '.dark' class is applied based on stored/OS theme ahead of the first render.

configurator/src/main.ts

Bug fix (1) +4 / -4
StatusBar.svelteFix light-mode contrast for status bar text +4/-4

Fix light-mode contrast for status bar text

• Introduces light-mode background/border colors and adjusts muted text colors so the domain/override status remains legible in light mode.

configurator/src/components/shell/StatusBar.svelte

Other (1) +5 / -0
app.cssAdd Tailwind v4 custom 'dark:' variant keyed off '.dark' +5/-0

Add Tailwind v4 custom 'dark:' variant keyed off '.dark'

• Defines '@custom-variant dark (&:where(.dark, .dark *))' so the Studio chrome can be themed via a '.dark' class on the mount root, independent of any framework token theming.

configurator/src/app.css

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 11 rules

Grey Divider


Informational

1. No scheme listener teardown 🐞 Bug ☼ Reliability
Description
The theme module registers a matchMedia('(prefers-color-scheme: dark)') change listener at module
load and never removes it, so HMR or repeated module evaluation can accumulate duplicate listeners.
This can cause redundant theme updates and minor memory growth in dev/remount scenarios.
Code

configurator/src/lib/theme.svelte.ts[R61-73]

+if (typeof matchMedia !== "undefined") {
+  const mql = matchMedia("(prefers-color-scheme: dark)");
+  const onChange = (e: MediaQueryListEvent) => {
+    if (!followSystem) return;
+    themeState.value = e.matches ? "dark" : "light";
+    applyToRoot();
+  };
+  // Safari < 14 only exposes the older addListener/removeListener pair.
+  if (typeof mql.addEventListener === "function") {
+    mql.addEventListener("change", onChange);
+  } else if (typeof mql.addListener === "function") {
+    mql.addListener(onChange);
+  }
Relevance

⭐⭐⭐ High

Prior PRs accepted matchMedia listeners with cleanup/teardown patterns to avoid duplicate listeners
on re-eval.

PR-#312
PR-#316

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The listener is added at import time in theme.svelte.ts, but there is no corresponding removal
path in that module or the boot code.

configurator/src/lib/theme.svelte.ts[61-74]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A `matchMedia` change listener is attached at module scope and is never removed. In production this typically lives for the page lifetime, but under HMR or repeated module evaluation it can create duplicate listeners.

## Issue Context
The callback is guarded by `followSystem`, but the listener remains registered regardless, and there is no exported teardown API.

## Fix Focus Areas
- configurator/src/lib/theme.svelte.ts[61-74]
- configurator/src/main.ts[19-30]

## Suggested fix
Option A (simple guard):
- Add a module-level boolean like `let listenerBound = false;` and skip binding if already bound.

Option B (proper teardown):
- Store `mql` + handler references, export `disposeThemeListener()` that calls `removeEventListener('change', handler)` (and `removeListener` fallback).
- In dev/HMR, call disposal from an HMR hook (or if you ever add an explicit unmount path, call it there).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Unscoped dark variant leakage 🐞 Bug ≡ Correctness
Description
The dark: variant is defined as &:where(.dark, .dark *), so any ancestor .dark forces dark
styling even if Studio removes .dark from its own mount root. In an embedded host that already
uses a .dark class strategy, the new light-mode toggle cannot visually switch to light mode.
Code

configurator/src/app.css[R3-6]

+/* Studio chrome theme — toggled by lib/theme.svelte.ts via a `.dark` class
+   on the mount root, independent of the framework's own [data-theme]
+   (which only governs the dogfooded --sf-* tokens loaded below). */
+@custom-variant dark (&:where(.dark, .dark *));
Relevance

⭐ Low

PR description says scoping dark variant to avoid host .dark was deliberately skipped.

PR-#323

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
app.css defines dark: as matching any .dark ancestor (.dark *), while the theme system only
toggles .dark on the mount root. Because .dark * can match due to an outer ancestor, Studio
cannot reliably disable dark styling by removing .dark from its own root.

configurator/src/app.css[1-6]
configurator/src/lib/theme.svelte.ts[34-44]
configurator/src/main.ts[19-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tailwind’s `dark:` variant is currently activated by any `.dark` ancestor in the document (`.dark *`). That means Studio’s theme toggle (which only toggles `.dark` on the mount root) cannot guarantee light mode if the embedding page already has `.dark` applied higher up (e.g., `<body class="dark">`).

## Issue Context
Studio is sometimes embedded (see `#slashed-admin-app` mount path). The current variant selector is not scoped to the Studio root, so external `.dark` can override Studio’s intended theme.

## Fix Focus Areas
- configurator/src/app.css[3-6]
- configurator/src/lib/theme.svelte.ts[34-44]
- configurator/src/main.ts[19-29]

## Suggested fix
1. Change the custom variant to key off a Studio-specific selector (recommended: a unique data-attribute or class).
  - Example (attribute-based):
    - `@custom-variant dark (&:where([data-studio-theme='dark'], [data-studio-theme='dark'] *));`
2. Update `bindThemeRoot/applyToRoot` to set that attribute/class on the mount root instead of toggling the generic `.dark`.
  - Example:
    - `root?.setAttribute('data-studio-theme', themeState.value);`
3. Ensure all existing `dark:` usages still work (they will, but now only within the Studio root).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +61 to +73
if (typeof matchMedia !== "undefined") {
const mql = matchMedia("(prefers-color-scheme: dark)");
const onChange = (e: MediaQueryListEvent) => {
if (!followSystem) return;
themeState.value = e.matches ? "dark" : "light";
applyToRoot();
};
// Safari < 14 only exposes the older addListener/removeListener pair.
if (typeof mql.addEventListener === "function") {
mql.addEventListener("change", onChange);
} else if (typeof mql.addListener === "function") {
mql.addListener(onChange);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

2. No scheme listener teardown 🐞 Bug ☼ Reliability

The theme module registers a matchMedia('(prefers-color-scheme: dark)') change listener at module
load and never removes it, so HMR or repeated module evaluation can accumulate duplicate listeners.
This can cause redundant theme updates and minor memory growth in dev/remount scenarios.
Agent Prompt
## Issue description
A `matchMedia` change listener is attached at module scope and is never removed. In production this typically lives for the page lifetime, but under HMR or repeated module evaluation it can create duplicate listeners.

## Issue Context
The callback is guarded by `followSystem`, but the listener remains registered regardless, and there is no exported teardown API.

## Fix Focus Areas
- configurator/src/lib/theme.svelte.ts[61-74]
- configurator/src/main.ts[19-30]

## Suggested fix
Option A (simple guard):
- Add a module-level boolean like `let listenerBound = false;` and skip binding if already bound.

Option B (proper teardown):
- Store `mql` + handler references, export `disposeThemeListener()` that calls `removeEventListener('change', handler)` (and `removeListener` fallback).
- In dev/HMR, call disposal from an HMR hook (or if you ever add an explicit unmount path, call it there).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Contributor Author

Triaged both findings from the last review pass — no code changes for either:

  1. matchMedia listener teardown: real observation, but low-impact in practice — the module loads once per page in production (no unmount path exists for the Studio app to call a disposer from), so the only realistic accumulation path is dev-mode HMR re-evaluation, which doesn't affect end users and would only add a handful of duplicate no-op-guarded callbacks during active development. Not adding a teardown API for a listener that's never actually removed by any caller.
  2. Unscoped dark: variant selector (.dark *): same trade-off already called out in this PR's description — scoping to a Studio-specific attribute instead of the conventional .dark class is a real alternative, but adds indirection for a theoretical collision (an embedding host applying .dark somewhere in the ancestor chain of #slashed-admin-app/#app for unrelated reasons) that hasn't been observed. Keeping the conventional approach per the bot's own earlier recommendation on the original PR. Flagging again here for visibility if it ever becomes a real bug report.

Generated by Claude Code

@jackgranatowski
jackgranatowski merged commit 3e3bfa5 into main Jul 3, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants