Skip to content

feat(configurator): show both light and dark palette previews in color controls - #430

Merged
jackgranatowski merged 2 commits into
mainfrom
claude/configurator-dark-theme-colors-fi2zvj
Jun 27, 2026
Merged

feat(configurator): show both light and dark palette previews in color controls#430
jackgranatowski merged 2 commits into
mainfrom
claude/configurator-dark-theme-colors-fi2zvj

Conversation

@jackgranatowski

@jackgranatowski jackgranatowski commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

https://claude.ai/code/session_016K8W2WSJxe9Fc6jmMe3qxD

Summary by CodeRabbit

  • New Features
    • Brand color swatches now show separate light and dark theme rows for easier comparison.
    • Swatch labels and tooltips now display theme-specific color step information.
    • Theme-aware color previews now resolve correctly for both light and dark modes.
  • Bug Fixes / Improvements
    • Improved reliability and speed of theme color resolution by caching results within a preview session.

…r controls

Add resolveColorForTheme() to previewResolver using theme-scoped probe
elements ([data-theme] wrappers), enabling color resolution independent
of the preview's active theme. ColorsPanel now renders two swatch rows
per brand color — L (light) and D (dark) — so both palettes are always
visible regardless of which theme the preview is showing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016K8W2WSJxe9Fc6jmMe3qxD
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ffe18d11-9034-4f22-bb40-0a00083402fd

📥 Commits

Reviewing files that changed from the base of the PR and between 0dd8388 and 6b2ed2a.

📒 Files selected for processing (1)
  • configurator/src/lib/previewResolver.svelte.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • configurator/src/lib/previewResolver.svelte.ts

📝 Walkthrough

Walkthrough

Adds theme-specific color resolution in the preview resolver and updates the brand palette in ColorsPanel.svelte to render separate light and dark swatch rows with theme-aware colors and labels.

Changes

Theme-aware color preview

Layer / File(s) Summary
Themed probe resolution
configurator/src/lib/previewResolver.svelte.ts
Adds light and dark probe wrappers, clears themed probes and caches on preview document or version changes, and exports resolveColorForTheme(cssExpr, theme) for theme-scoped color lookup.
Theme-specific brand swatches
configurator/src/components/panels/ColorsPanel.svelte
Imports theme-scoped color resolution, adds paintTheme, and renders BRAND_COLOR_KEYS as light and dark rows with theme-specific swatch backgrounds and titles.

Sequence Diagram(s)

sequenceDiagram
  participant ColorsPanel.svelte
  participant previewResolver.svelte.ts
  participant preview document

  ColorsPanel.svelte->>previewResolver.svelte.ts: resolveColorForTheme(expr, theme)
  previewResolver.svelte.ts->>preview document: getThemedProbe(theme)
  previewResolver.svelte.ts->>preview document: set probe.color = expr
  previewResolver.svelte.ts->>preview document: getComputedStyle(probe).color
  previewResolver.svelte.ts-->>ColorsPanel.svelte: themed color string
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

codex

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly summarizes the main change: configurator color controls now show both light and dark palette previews.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/configurator-dark-theme-colors-fi2zvj

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

Show light + dark palette swatches in configurator color controls
✨ Enhancement 🕐 20-40 Minutes

Grey Divider

Description

• Add theme-scoped color resolution in the preview resolver using hidden themed probes.
• Render separate Light and Dark swatch strips for each brand color row.
• Keep swatch previews accurate regardless of the preview iframe’s active theme.
Diagram

graph TD
  A["ColorsPanel.svelte"] --> B["previewResolver.svelte.ts"] --> C[("Preview iframe document")]
  B --> D["Themed probe wrappers\n[data-theme=light/dark]"] --> C
  B --> E["getComputedStyle(color)"] --> A
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Temporarily toggle the preview’s active theme to resolve swatches
  • ➕ No extra probe wrapper elements to manage
  • ➕ Leverages existing resolveColor() path
  • ➖ Risk of visible flicker and side effects in the live preview
  • ➖ Harder to make reactive without triggering extra renders/layout
  • ➖ Can break if other preview logic depends on current theme during resolution
2. Maintain two hidden preview iframes (one per theme) for resolution
  • ➕ True isolation between light/dark contexts
  • ➕ Simplifies theme-specific resolution logic (each iframe has a fixed theme)
  • ➖ Higher memory/CPU cost and more complex lifecycle management
  • ➖ More moving parts (iframe load timing, override syncing, failure modes)

Recommendation: The chosen approach (theme-scoped probes via [data-theme] wrappers) is the best tradeoff: it avoids mutating the active preview theme (no flicker/side effects) while keeping resolution faithful to real computed styles. The added wrapper lifecycle management is modest and is already cleaned up on preview doc re-registration.

Files changed (2) +85 / -12

Enhancement (2) +85 / -12
ColorsPanel.svelteRender light+dark palette swatch rows for brand colors +36/-12

Render light+dark palette swatch rows for brand colors

• Adds a theme-aware paintTheme() helper and updates the brand color UI to show two swatch strips per color: an L row resolved in light theme and a D row resolved in dark theme. Swatch styling is split into top/bottom halves to visually pair the two theme rows.

configurator/src/components/panels/ColorsPanel.svelte

previewResolver.svelte.tsAdd resolveColorForTheme() using themed hidden probes +49/-0

Add resolveColorForTheme() using themed hidden probes

• Introduces theme-specific hidden probe wrappers scoped via data-theme="light"/"dark" so callers can resolve computed colors for either theme without changing the preview’s active theme. Updates registerPreviewDoc() cleanup to remove and reset themed probe elements when the preview document changes.

configurator/src/lib/previewResolver.svelte.ts

@coderabbitai coderabbitai Bot added the codex label Jun 27, 2026
@qodo-code-review

qodo-code-review Bot commented Jun 27, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 6 rules

Grey Divider


Remediation recommended

1. Excess computedStyle in swatches ✓ Resolved 🐞 Bug ➹ Performance
Description
ColorsPanel now renders light+dark palette swatch rows, and each swatch calls
resolveColorForTheme(), which performs a getComputedStyle() read; this multiplies synchronous
style resolution work during frequent updates (e.g., dragging/typing overrides). This can degrade
configurator responsiveness on slower devices or when the preview document is complex.
Code

configurator/src/components/panels/ColorsPanel.svelte[R265-295]

+          <!-- Palette swatch strips — light row then dark row -->
          {#if BRAND_COLOR_KEYS.includes(light.colorKey)}
-            <div class="flex gap-1 mt-1 pl-1">
-              {#each SWATCH_STEPS as step (step)}
-                {@const expr = `var(--sf-color-${light.colorKey}-${step})`}
-                {@const resolved = paint(expr, expr)}
-                <div
-                  class="w-5 h-5 rounded border border-white/10"
-                  style={`background: ${resolved}`}
-                  title={`${light.colorKey}-${step} — ${resolved}`}
-                ></div>
-              {/each}
+            <div class="mt-1 pl-1 space-y-px">
+              <div class="flex items-center gap-1">
+                <span class="text-[7px] text-slate-600 w-2.5 shrink-0 text-right select-none">L</span>
+                <div class="flex gap-0.5">
+                  {#each SWATCH_STEPS as step (step)}
+                    {@const expr = `var(--sf-color-${light.colorKey}-${step})`}
+                    {@const resolved = paintTheme(expr, "light", expr)}
+                    <div
+                      class="w-5 h-3 rounded-t border-x border-t border-white/10"
+                      style={`background: ${resolved}`}
+                      title={`${light.colorKey}-${step} (light) — ${resolved}`}
+                    ></div>
+                  {/each}
+                </div>
+              </div>
+              <div class="flex items-center gap-1">
+                <span class="text-[7px] text-slate-600 w-2.5 shrink-0 text-right select-none">D</span>
+                <div class="flex gap-0.5">
+                  {#each SWATCH_STEPS as step (step)}
+                    {@const expr = `var(--sf-color-${light.colorKey}-${step})`}
+                    {@const resolved = paintTheme(expr, "dark", expr)}
+                    <div
+                      class="w-5 h-3 rounded-b border-x border-b border-white/10"
+                      style={`background: ${resolved}`}
+                      title={`${light.colorKey}-${step} (dark) — ${resolved}`}
+                    ></div>
+                  {/each}
+                </div>
+              </div>
Relevance

⭐⭐ Medium

No prior accepted reviews about caching/avoiding getComputedStyle in swatch loops;
getComputedStyle-based resolver added in PR 429.

PR-#429

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The panel now resolves each step for both themes via paintTheme(), and paintTheme() calls
resolveColorForTheme(). resolveColorForTheme() performs a getComputedStyle() read, so the
template loop translates directly into many synchronous style computations per render/update.

configurator/src/components/panels/ColorsPanel.svelte[31-35]
configurator/src/components/panels/ColorsPanel.svelte[265-295]
configurator/src/lib/previewResolver.svelte.ts[102-139]

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

### Issue description
The new dual-row swatch rendering calls `resolveColorForTheme()` for every palette step in both light and dark, and `resolveColorForTheme()` performs `getComputedStyle()` each time. This can become a noticeable performance hotspot during rapid override changes.

### Issue Context
- Swatches are rendered for multiple `BRAND_COLOR_KEYS` and multiple `SWATCH_STEPS`, and now do this twice (L + D).
- `resolveColorForTheme()` uses a hidden probe element and calls `getComputedStyle()`, which is synchronous and can force style recalculation.

### Fix Focus Areas
- configurator/src/lib/previewResolver.svelte.ts[67-70]
- configurator/src/lib/previewResolver.svelte.ts[102-139]
- configurator/src/components/panels/ColorsPanel.svelte[31-35]
- configurator/src/components/panels/ColorsPanel.svelte[265-295]

### Suggested fix
1. Add a small in-memory cache in `previewResolver.svelte.ts` keyed by `(theme, cssExpr)` (and for `resolveColor`, also include the active theme) and return cached results when available.
2. Clear the cache inside `bumpPreviewVersion()` (safe because it already runs whenever overrides/theme/doc changes), without reading `previewVersion.value` (to avoid the effect dependency loop noted in comments).
3. Optionally, in `ColorsPanel.svelte`, precompute swatch arrays in a `$derived` that depends on `previewVersion.value` and reuse the resolved strings in the template to avoid repeated calls from the markup loop.

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


Grey Divider

Qodo Logo

Add a resolveCache Map cleared in bumpPreviewVersion() so repeated
resolveColor / resolveColorForTheme calls for the same expression within
one render cycle hit the cache instead of forcing style recalculation.
Eliminates 110 redundant getComputedStyle() calls per update caused by
the dual light/dark swatch rows.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016K8W2WSJxe9Fc6jmMe3qxD
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants