Skip to content

fix(configurator): align Auto dark derivation with CSS formula - #428

Merged
jackgranatowski merged 2 commits into
mainfrom
claude/primary-source-light-color-t9vi98
Jun 27, 2026
Merged

fix(configurator): align Auto dark derivation with CSS formula#428
jackgranatowski merged 2 commits into
mainfrom
claude/primary-source-light-color-t9vi98

Conversation

@jackgranatowski

@jackgranatowski jackgranatowski commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

The JS deriveDarkFromLight formula (l + 0.27) didn't match the CSS
formula in themes.css (clamp(0.65, 0.95 - l * 0.5, 0.88)), and the
base color used a completely different formula that wasn't handled at all.

In Auto dark mode the configurator was also baking the derived dark value
into the exported CSS override, bypassing the runtime CSS formula entirely.

  • Fix deriveDarkFromLight to mirror the exact CSS formulas (including the
    base-specific clamp(0.16, 1.18 - l, 0.24) / c * 0.5 variant)
  • In Auto dark mode, only write the light token to overrides — the CSS
    formula in themes.css handles source-dark at runtime
  • Switching Auto → Manual now removes the dark override; switching
    Manual → Auto populates dark as a pre-filled starting point

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Gjdz9WEvMA8M1ve3Z1Xsoc

Summary by CodeRabbit

  • New Features

    • Improved automatic dark-mode color handling for brand colors, with more accurate derived values based on the selected color type.
    • The dark-mode toggle now preserves a sensible starting value when switching from automatic to manual mode.
  • Bug Fixes

    • Resetting a brand color now only clears the light value, avoiding unexpected changes to the dark value.
    • The “Dark: auto-derived” preview now reflects the current light color more accurately.

The JS deriveDarkFromLight formula (l + 0.27) didn't match the CSS
formula in themes.css (clamp(0.65, 0.95 - l * 0.5, 0.88)), and the
base color used a completely different formula that wasn't handled at all.

In Auto dark mode the configurator was also baking the derived dark value
into the exported CSS override, bypassing the runtime CSS formula entirely.

- Fix deriveDarkFromLight to mirror the exact CSS formulas (including the
  base-specific clamp(0.16, 1.18 - l, 0.24) / c * 0.5 variant)
- In Auto dark mode, only write the light token to overrides — the CSS
  formula in themes.css handles source-dark at runtime
- Switching Auto → Manual now removes the dark override; switching
  Manual → Auto populates dark as a pre-filled starting point

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

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

More reviews will be available in 53 minutes and 40 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

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 credits.

🚦 How do rate 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 see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a05234eb-e881-4f6c-b176-427826268b6f

📥 Commits

Reviewing files that changed from the base of the PR and between 0065715 and 5b3d22d.

📒 Files selected for processing (1)
  • configurator/src/components/panels/ColorsPanel.svelte
📝 Walkthrough

Walkthrough

ColorsPanel now derives auto-dark values with colorKey-specific logic, applies light-token edits without auto-dark side effects, changes auto/manual dark toggling to seed or clear overrides, and resets only the light brand token.

Changes

ColorsPanel auto-dark updates

Layer / File(s) Summary
Color-key dark derivation
configurator/src/components/panels/ColorsPanel.svelte
deriveDarkFromLight now branches on colorKey, and the auto-derived dark label uses the current light value plus colorKey.
Light updates and reset
configurator/src/components/panels/ColorsPanel.svelte
Brand light changes now always use onSet, and resetting a brand light token clears only that light token.
Dark mode toggle flow
configurator/src/components/panels/ColorsPanel.svelte
Auto/manual dark toggling now seeds a derived dark value when leaving auto, removes the dark override when returning to auto, and updates autoDarkSet.

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 The title clearly matches the main change: aligning Auto dark derivation with the CSS formula in the configurator.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/primary-source-light-color-t9vi98

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.

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

Copy link
Copy Markdown

PR Summary by Qodo

Fix configurator Auto dark to match themes.css OKLCH derivation
🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

Grey Divider

Description

• Match configurator dark-derivation logic to the OKLCH clamp formulas in core/themes.css.
• In Auto dark mode, stop exporting dark overrides and let CSS derive them at runtime.
• Make Auto/Manual switching add/remove dark overrides predictably (derived seed on Manual).
Diagram

graph TD
  A["ColorsPanel.svelte"] --> B[("Token overrides")] --> C["Exported CSS overrides"] --> D["Runtime theme resolution"] --> E["Rendered dark colors"]
  F["core/themes.css"] --> D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Single source of truth for derivation formulas
  • ➕ Eliminates drift between JS preview logic and CSS runtime logic
  • ➕ Makes future formula tweaks safer (change once)
  • ➖ Requires build-time generation or shared config plumbing across CSS and configurator
2. Preview by reading computed CSS values
  • ➕ No duplicated math in JS; preview always matches runtime
  • ➕ Naturally supports future CSS-only changes
  • ➖ Harder to implement (requires DOM/rendered styles), less deterministic in non-browser contexts
  • ➖ Potential perf/complexity tradeoffs for live editing

Recommendation: The PR’s approach is the right near-term fix: it removes the incorrect persistence of derived dark values and aligns preview math with themes.css, restoring a consistent mental model (Auto mode delegates to CSS). Longer-term, consider centralizing the derivation formulas (generation or computed-style sampling) to avoid future JS/CSS drift.

Files changed (1) +17 / -20

Bug fix (1) +17 / -20
ColorsPanel.svelteAlign auto-dark derivation with themes.css and stop persisting dark overrides +17/-20

Align auto-dark derivation with themes.css and stop persisting dark overrides

• Updates deriveDarkFromLight to mirror the exact themes.css OKLCH clamp formulas, including a base-specific variant. Changes Auto dark behavior to only persist the light token (dark is derived at runtime), and adjusts Auto/Manual toggling so Manual seeds a derived dark override while Auto removes any dark override.

configurator/src/components/panels/ColorsPanel.svelte

@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


Action required

1. Auto dark state can desync ✓ Resolved 🐞 Bug ≡ Correctness
Description
In Auto dark, light edits/resets no longer clear or update the dark-side override, so if a
--*-source-dark override is introduced via import/undo while ColorsPanel is mounted, the UI can
still show “Auto dark” while runtime CSS uses the hidden dark override instead of the CSS derivation
formula.
Code

configurator/src/components/panels/ColorsPanel.svelte[R163-177]

  function handleLightChange(light: ColorSource, dark: ColorSource | undefined, newVal: string) {
-    if (dark && autoDarkSet.has(light.colorKey)) {
-      onBulkChange({ [light.name]: newVal, [dark.name]: deriveDarkFromLight(newVal) });
-    } else {
-      onSet(light.name, newVal);
-    }
+    onSet(light.name, newVal);
  }

  function toggleDarkMode(colorKey: string, dark: ColorSource | undefined, lightName: string) {
    if (autoDarkSet.has(colorKey)) {
-      // Switch to manual — just remove from auto set, keep current dark value
+      // Switch to manual — populate dark with the derived value as a starting point
+      const lightVal = overrides[lightName] ?? BRAND_SOURCES.find(s => s.name === lightName)?.default ?? "";
+      if (dark) onSet(dark.name, deriveDarkFromLight(lightVal, colorKey));
      autoDarkSet = new Set([...autoDarkSet].filter(k => k !== colorKey));
    } else {
-      // Switch to auto — derive dark from current light and apply
-      const lightVal = overrides[lightName] ?? BRAND_SOURCES.find(s => s.name === lightName)?.default ?? "";
-      if (dark) onSet(dark.name, deriveDarkFromLight(lightVal));
+      // Switch to auto — remove dark override so the CSS formula handles it at runtime
+      if (dark) onBulkChange({ [dark.name]: null });
      autoDarkSet = new Set([...autoDarkSet, colorKey]);
    }
Relevance

⭐⭐⭐ High

PR #423 accepted keeping autoDarkSet synced with overrides to prevent hidden/overwritten dark
overrides; same desync issue.

PR-#423

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
After this PR, auto-mode no longer maintains/clears dark overrides during light edits or resets, but
overrides can still be introduced externally (Import/Undo/Redo). If a dark override exists, core CSS
uses it (via var fallback), so runtime behavior can contradict the Auto-dark UI state and can even
lead to overwriting an imported dark value when toggling.

configurator/src/components/panels/ColorsPanel.svelte[149-178]
configurator/src/components/panels/ColorsPanel.svelte[221-254]
configurator/src/components/panels/ColorsPanel.svelte[109-116]
configurator/src/App.svelte[111-120]
configurator/src/App.svelte[134-150]
configurator/src/App.svelte[152-176]
configurator/src/components/shell/StudioHeader.svelte[59-107]
core/tokens.css[370-376]
core/themes.css[64-85]
PR-#423

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

### Issue description
`autoDarkSet` is a local UI state, but `overrides` can change externally (Import, Undo/Redo, bulk patches) while `ColorsPanel` is mounted. After this PR, auto-mode no longer writes/clears dark overrides on light edits and light resets, so a dark override can remain present while the row still behaves as Auto dark.

This yields a correctness issue: CSS prefers an explicit `--sf-color-*-source-dark` override over the runtime formula fallback, so the preview/exported overrides can diverge from what the Auto-dark UI implies.

### Issue Context
- `autoDarkSet` is only initialized once from `overrides` and then only mutated via `toggleDarkMode`.
- `overrides` can be modified at any time via Import and Undo/Redo.
- Core CSS uses `var(--sf-color-*-source-dark, <derived>)` / `--sf-color-*` assignments, so the presence of a dark override changes runtime behavior.

### Fix Focus Areas
- configurator/src/components/panels/ColorsPanel.svelte[109-178]
- configurator/src/components/panels/ColorsPanel.svelte[205-254]

### Proposed fix
Implement an invariant: **if a dark token is overridden, treat the row as Manual dark** (and do not run the Auto→Manual branch that overwrites the imported value).

Concretely (any one approach is acceptable, but must also make the toggle logic consistent with what the UI displays):
1) Compute `isAutoMode` using both `autoDarkSet` and the presence of a dark override:
  - `const darkOverridden = dark?.name && (dark.name in overrides)`
  - `const isAutoMode = autoDarkSet.has(light.colorKey) && !darkOverridden`
2) Update `toggleDarkMode` to use the same `darkOverridden` guard:
  - Treat `darkOverridden === true` as Manual mode regardless of `autoDarkSet`, so clicking toggles to Auto by clearing the override.
  - Only run the Auto→Manual “populate derived dark” branch when `autoDarkSet.has(colorKey)` **and** `!darkOverridden`.
3) Optionally add a small `$effect` that removes keys from `autoDarkSet` whenever their dark token appears in `overrides` (prevents stale set values from lingering after import/undo).
4) (Optional hardening) In `handleLightChange` / light `onReset`, if `isAutoMode` is true but a dark override exists, clear it via `onBulkChange({ [dark.name]: null })` before/alongside applying the light change.

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


Grey Divider

Qodo Logo

Comment thread configurator/src/components/panels/ColorsPanel.svelte
autoDarkSet is only updated via toggleDarkMode, but overrides can change
externally (import, undo/redo) while ColorsPanel is mounted. A dark
override introduced that way would cause the row to display "Auto dark"
while the CSS formula is actually bypassed by the hidden override.

Derive isAutoMode from both autoDarkSet and the live absence of a dark
override in overrides, and apply the same guard in toggleDarkMode so
clicking the toggle on an externally-overridden row correctly switches
to Auto (clearing the stale override) rather than Manual.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gjdz9WEvMA8M1ve3Z1Xsoc
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