feat: detect and warn when step overrides shadow generated scales - #663
Conversation
Every ClampField that carries the modular-scale ratio block also owns the base min/max pair, but its `overridden` flag and its reset only ever looked at the base tokens. Two consequences, in the spacing card and both typography cards: - A ratio-only change produced no override marker and no reset affordance at all, so the card presented itself as untouched and the ratio could not be reverted from where it was set — only via the All-tokens tab or Reset all. - After changing base and ratio, the card's reset cleared the base and silently left the ratio override in place, so the card read as pristine while the generated scale stayed shifted. Include the ratio tokens in both. --sf-text-ratio-* is deliberately shared between the text and display generators, so resetting from either typography card clears it for both — the mirror image of editing from either card setting it for both, which is already the documented behaviour. Verified by driving the built configurator in a browser: on a ratio-only change the card's reset link goes from absent (before) to present (after), and clicking it clears --sf-space-ratio-min back to the framework default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
A concrete per-step value in the override map (--sf-space-m, --sf-text-l, …) beats the generated one on purpose: a fine-tuned rung should survive the knob that would otherwise produce it, and the live preview and the WordPress plugin's PHP emitter implement the same precedence. But when a whole ladder is stored — from an older settings page, an imported theme, or hand edits in the All-tokens tab — every control in the scale section goes inert while still reading back its own values. The panel shows the knobs, the page reports them at :root, the preview bars redraw, and nothing moves. Nothing said why. That state was diagnosed in the field only by fitting the framework's formula to the measured ladder and finding that no viewport could explain it. Surface it in the panel instead: report how many fixed values are shadowing the scale, say that fixed values win, list them on demand, and offer to clear them. The notice sits above the fold rather than inside the scale section, which is collapsed by default — a warning explaining why those controls do nothing is useless behind the disclosure the user has not opened. tests/scale-shadow-steps.test.js pins the step lists to core/tokens.css by deriving them from the generator inputs each token's value reads, so a new rung cannot ship with the guard blind to it. Verified by driving the built configurator against the reported override map: the notice appears for both scales, clearing restores --sf-space-m to the generated clamp() — now honouring the base-max the fixed ladder had been suppressing — and leaves the knobs themselves untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configurator/src/components/inputs/ScaleShadowNotice.svelte`:
- Around line 35-40: Update the disclosure button in ScaleShadowNotice so it has
an accessible name identifying the content it controls and exposes the current
expanded state to assistive technology, while preserving the existing expanded
toggle and visible “show”/“hide” labels.
- Around line 22-26: Update the plural wording in ScaleShadowNotice to state
that the controls do not affect the shadowed steps, while preserving the
singular wording for one overridden token. Ensure the message does not imply the
scale controls have no effect on unshadowed steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa91a5c0-851f-4f8c-ae34-a19e200b54f1
📒 Files selected for processing (5)
configurator/src/components/inputs/ScaleShadowNotice.svelteconfigurator/src/components/panels/SpacingPanel.svelteconfigurator/src/components/panels/TypographyPanel.svelteconfigurator/src/lib/scaleShadow.tstests/scale-shadow-steps.test.js
…sure Review feedback on #663. The plural copy claimed the controls "have no effect", but a partial ladder only shadows the steps it covers — the knobs still drive every unshadowed rung. Overstating this invites clearing fixed values that were deliberate, so say the controls do not affect *those steps*. The show/hide toggle had no accessible name beyond its visible word, which does not identify what is being disclosed, and did not expose its state. Add aria-expanded, a descriptive aria-label naming the scale, and aria-controls pointing at the token list. The id is per-instance because the spacing and type notices can be mounted at the same time. Verified in a browser: the disclosure is reachable by its accessible name, aria-expanded flips on toggle, aria-controls resolves to the list element, and ids stay unique across mounted notices. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
2 participants
Summary
Adds detection and UI warnings for a silent failure mode in the scale panels: when concrete per-step values are stored in the override map (e.g., from an older settings page, imported theme, or hand edits), they completely shadow the generated scale, making all the knobs inert while still reading back their own values everywhere. The user sees no indication that the controls have stopped working.
This PR introduces:
scaleShadow.ts— Core library with:SPACE_STEP_TOKENS,TEXT_STEP_TOKENS,DISPLAY_STEP_TOKENS: pinned lists of every generated step token, verified by contract test to matchcore/tokens.cssshadowingSteps(): filters overrides to find which steps are shadowing the scaleScaleShadowNotice.svelte— Reusable warning component that:Contract test (
scale-shadow-steps.test.js) — Ensures the step lists stay in sync with the source of truth (core/tokens.css). Any new scale rung (e.g.,5xl) will fail the test until the lists are updated, preventing silent misses.Panel integration — Both
SpacingPanelandTypographyPanelnow:Type
Checklist
feat:,fix:,docs:, …) — enforced by commitlintnpm run lint:csspasses (stylelint)npm run buildrebuildsdist/(bundles are git-ignored; CI rebuilds and stamps headers)npm testpasses (unit + Playwright e2e)npm run check:version)core/*.css,optional/*.css, ortoken-registry.jsonchanged (npm run check:llm-guide)npm run check:macros,check:registry,audit:check)CHANGELOG.mdupdated under## [Unreleased](for user-facing changes)Notes
The contract test (
tests/scale-shadow-steps.test.js) runs in the root unit suite vianode --test, so it checks TypeScript source without requiring a TS toolchain — the step lists are plain string-literal arrays read as text.The warning placement is intentional: it sits above collapsed sections so users see it immediately, not buried where they'd have to expand a section to discover why the controls aren't working.
Both text and display scales are reported together in the typography panel because both generators live in the same "Fluid scale" section and share the
--sf-text-ratio-*inputs.https://claude.ai/code/session_01DftXg4tjRkey2c3ybxAWjN
Summary by CodeRabbit
New Features
Tests