Add #582 source-CSS audit gates (D1–D6) - #584
Merged
Merged
Conversation
Address every verified finding from the source-CSS audit in one pass: wire the dead knobs render-safely, correct annotation/doc drift, document the intentional architecture, and add five CI gates that stop the whole class of defects from recurring. Workstream A — wire dead knobs (zero render change; every var() fallback equals today's literal): - core/print.css @page now reads --sf-print-page-size / -margin (verified in Chromium that var() is honoured inside @page descriptors; a new Chromium-only e2e asserts an override reaches the page box). - optional/forms.css field metrics resolve through --sf-field-padding-block / -inline / --sf-field-radius, then the core --sf-field-block rhythm knob, then the literal. --sf-field-block's :root default is unified to space-xs to match the shipped field padding so wiring changes nothing. Workstream B/C — docs & comments: - Fix annotation drift caught by the new value gate: the whole duration scale (fast 150ms, normal 250ms, slow 400ms), size-s (32px), size-m (~40px, not "default for buttons/inputs"), size-xl (56px), instant 100ms. - architecture.md --sf-blur-* → --sf-blur; motion.md → --sf-color-primary- source-light; llm-guide.md --sf-field-block default. - Comment the @Property initial-value ≠ :root default (and its effect on fallbacks), the deliberate .sf-btn min-height ladder offset, and the .sf-h* / core heading mirror (adds overflow-wrap; margins intentionally not reset). Workstream D — new CI gates (+ negative tests): - check:dead-knobs — an annotation that promises wiring ("Maps to …") must have a real consumer. - check:annotations — ~Nms/~Npx prose must match the resolved token value. - check:hook-tokens — the fallback-only hook tokens (new scripts/hook-tokens.js) are undeclared, consumed with a fallback, and documented; no new hook escapes. - check:mirrors — @Property↔:root char mirrors, 21 container-query re-derivations, and 10 SL-001 dark derivations stay in sync. - check:bundle-defs — every no-fallback token consumption ships its definition in the same bundle. - check-llm-guide now counts only declarations (not consumption) and allows the documented hook tokens (D3/D5). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012i9DDskbZqmuMeDwQMjPjF
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 31 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 (3)
📝 WalkthroughWalkthroughThis PR decouples ChangesToken Decoupling
Estimated code review effort: 3 (Moderate) | ~25 minutes New CI Audit Gates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
# Conflicts: # configurator/src/data/api-index.generated.json
…-* rungs (#582 E1) The button size scale now maps 1:1 onto the --sf-size-* rungs (--xs→xs, --s→s, m→m, --l→l, --xl→xl) instead of the one-rung offset that pinned the default button to the 44px --sf-touch-target floor. Every non-xs/s button becomes shorter; the default drops from 44px (--sf-size-l, via --sf-touch-target) to 40px (--sf-size-m). BREAKING CHANGE: default/.sf-btn--l/.sf-btn--xl min-block-size shrink by one rung. The default control still clears the WCAG 2.2 AA 24px target but no longer meets the 44px AAA target by default — restore it globally with `:root { --sf-btn-min-height: var(--sf-touch-target) }` or per-button with `.sf-btn--l`. See docs/migration.md (0.7.8 → 0.8.0). - optional/components.css: base min-height--size → --sf-size-m; --l → --sf-size-l; --xl → --sf-size-xl; rewrite the ladder comment (the offset is gone). - docs/migration.md: add the 0.7.8 → 0.8.0 breaking entry with a before/after table and the opt-back-in recipe. - docs/components.md, docs/llm-guide.md: update the --sf-btn-min-height default. - tests/button.spec.js: assert the 1:1 min-height↔--sf-size-* mapping (both themes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012i9DDskbZqmuMeDwQMjPjF
…E1 follow-up) After the ladder remap the default button reads --sf-size-m, so the annotation's "not currently consumed by any shipped rule" is stale. Point it at the real consumer instead and regenerate the derived indexes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012i9DDskbZqmuMeDwQMjPjF
… the size scale (#582) An accessibility constraint must own its value, not borrow it from a configurable design scale. --sf-touch-target was `var(--sf-size-l)`, so retuning the (fully configurable) --sf-size-* scale could silently drag the WCAG 2.5.5 floor below spec. It now holds a fixed 2.75rem (44px) literal, independent of the scale. Freed from carrying the 44px anchor, --sf-size-l returns to its natural rung: the UI size ladder is now a clean +8px geometric scale (24 · 32 · 40 · 48 · 56) instead of 24 · 32 · 40 · 44 · 56. Audited the whole token set for the same anti-pattern (a spec/physical constraint aliased to a configurable scale rung); --sf-touch-target was the only instance — focus-ring dims, header/sticky heights, icon sizes and measure max-widths all already own literal values. BREAKING CHANGE: --sf-size-l is now 3rem (48px), was 2.75rem (44px); the WCAG 44px guarantee now lives only on --sf-touch-target (read that, not --sf-size-l, for a target-size floor). .sf-btn--l follows --sf-size-l to 48px. The a11y min-target helper is unchanged (still 44px). See docs/migration.md. - core/tokens.css: --sf-size-l 2.75rem→3rem; --sf-touch-target literal 2.75rem; comments on the geometric scale and the independent a11y anchor. - tests/a11y-patterns.spec.js: new test proving --sf-touch-target survives a --sf-size-* override. - docs/migration.md, llm-guide.md, token-annotations.json + regenerated indexes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012i9DDskbZqmuMeDwQMjPjF
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api-index.json (1)
1728-1751: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate
--sf-btn-min-heightcopy The description still says it defaults to--sf-touch-target, but the current fallback is--sf-size-munless the knob is overridden.🤖 Prompt for 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. In `@docs/api-index.json` around lines 1728 - 1751, The `--sf-btn-min-height` token description is stale and still mentions `--sf-touch-target` as the default. Update the `description` for this token in the API index to reflect the current behavior that it falls back to `--sf-size-m` unless overridden, keeping the wording aligned with the `--sf-btn-min-height` entry and related button token metadata.
🧹 Nitpick comments (3)
tests/check-source-audit-gates.test.js (2)
40-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to
spawnSyncto prevent CI hangs.
runGatehas no timeout option. If a gate script ever hangs (e.g., an infinite loop introduced by a future bug), the test suite will hang indefinitely in CI with no output.⏱️ Proposed fix
function runGate(gate, dir) { return spawnSync(process.execPath, [path.join(ROOT, 'scripts', gate)], { encoding: 'utf8', env: { ...process.env, SLASHED_ROOT: dir }, + timeout: 30000, }); }🤖 Prompt for 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. In `@tests/check-source-audit-gates.test.js` around lines 40 - 45, The `runGate` helper currently calls `spawnSync` without a timeout, so a hanging gate script can block `check-source-audit-gates.test.js` indefinitely. Update `runGate(gate, dir)` to pass a reasonable timeout option to `spawnSync`, and make sure the test handles the timeout case cleanly when the spawned process exceeds that limit.
53-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that string mutations actually took effect to avoid confusing test failures.
The
.replace()calls at lines 60, 91–92, and 108–109 use exact string matching against current CSS values. If those values change in the future, the replace silently no-ops, the mutation never happens, the gate passes (status 0), and theassert.equal(r.status, 1)failure message becomes confusing — it says "expected failure" but the gate actually passed because nothing was mutated.Adding a guard that the mutated string differs from the original makes the failure mode clear: "mutation did not apply, test fixture is stale."
This applies to all three mutation-by-replace tests (D1 line 60, D4 lines 91–92, D5 lines 108–109).
🛡️ Example fix for the D1 test (apply similarly to D4 and D5)
const original = readCss(dir, 'core/print.css'); const css = original .replace('var(--sf-print-page-margin, 2cm)', '2cm'); + assert.notEqual(original, css, 'mutation did not apply — fixture string is stale'); writeCss(dir, 'core/print.css', css);🤖 Prompt for 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. In `@tests/check-source-audit-gates.test.js` around lines 53 - 66, The mutation-by-replace tests in this suite can silently no-op if the expected CSS text changes, which makes the later gate assertion misleading. Add an explicit guard in each affected test around the existing readCss/writeCss flow in check-dead-knobs.js tests so the test fails immediately when the replacement does not change the fixture, and apply the same safeguard to the other replace-based cases in the same file (including the D4 and D5 scenarios) using the same helper/readCss/writeCss pattern.configurator/src/data/api-index.generated.json (1)
12790-12809: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "touch-friendly" phrasing after touch-target decoupling.
The note still reads "Large UI component height (~48px). Touch-friendly interactive elements." Since
--sf-touch-targetis now explicitly decoupled from--sf-size-lin this PR (fixed 2.75rem regardless of size scale), keeping "touch-friendly" framing on--sf-size-lis a minor holdover from the old aliasing relationship and could confuse consumers into thinking overriding--sf-size-lstill affects touch-target compliance.🤖 Prompt for 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. In `@configurator/src/data/api-index.generated.json` around lines 12790 - 12809, The note for the `--sf-size-l` token still uses “touch-friendly” language that implies size drives touch-target behavior. Update the generated token metadata so the `note` for `--sf-size-l` no longer ties this size token to touch-target compliance, and instead reflects only the component height description; use the `api-index.generated.json` entry for `--sf-size-l` as the source to align wording with the new decoupled `--sf-touch-target` behavior.
🤖 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 `@core/tokens.css`:
- Line 1629: Document the breaking default change for the `--sf-field-block`
token in `docs/migration.md`, since `core/tokens.css` now sets it to
`var(--sf-space-xs)` instead of the previous `var(--sf-space-l)`. Add this to
the existing token migration notes alongside the other token shifts, and make
sure the migration guide clearly calls out the new default and any affected form
spacing behavior.
In `@scripts/check-annotations.js`:
- Around line 82-86: `resolveMs` is matching `ms` anywhere in the string and can
misread calc()-based values as literals; update the `resolveMs` helper in
`check-annotations.js` to behave like `resolvePx` by only accepting fully
literal duration strings. Use the `baseValue(name)` result and tighten the regex
with start/end anchors so `calc(...)` or other compound expressions return
`null` instead of a partial number.
In `@scripts/check-dead-knobs.js`:
- Line 37: The WIRING_CLAIM pattern in check-dead-knobs.js is too broad because
it will still match wiring phrases like “applied to” or “wired to” even when
they are negated. Update the regex/validation around WIRING_CLAIM and the
token-scanning loop so negated annotations are ignored before treating them as
evidence of wiring, using the existing dead-knob check logic to guard against
false CI failures.
---
Outside diff comments:
In `@docs/api-index.json`:
- Around line 1728-1751: The `--sf-btn-min-height` token description is stale
and still mentions `--sf-touch-target` as the default. Update the `description`
for this token in the API index to reflect the current behavior that it falls
back to `--sf-size-m` unless overridden, keeping the wording aligned with the
`--sf-btn-min-height` entry and related button token metadata.
---
Nitpick comments:
In `@configurator/src/data/api-index.generated.json`:
- Around line 12790-12809: The note for the `--sf-size-l` token still uses
“touch-friendly” language that implies size drives touch-target behavior. Update
the generated token metadata so the `note` for `--sf-size-l` no longer ties this
size token to touch-target compliance, and instead reflects only the component
height description; use the `api-index.generated.json` entry for `--sf-size-l`
as the source to align wording with the new decoupled `--sf-touch-target`
behavior.
In `@tests/check-source-audit-gates.test.js`:
- Around line 40-45: The `runGate` helper currently calls `spawnSync` without a
timeout, so a hanging gate script can block `check-source-audit-gates.test.js`
indefinitely. Update `runGate(gate, dir)` to pass a reasonable timeout option to
`spawnSync`, and make sure the test handles the timeout case cleanly when the
spawned process exceeds that limit.
- Around line 53-66: The mutation-by-replace tests in this suite can silently
no-op if the expected CSS text changes, which makes the later gate assertion
misleading. Add an explicit guard in each affected test around the existing
readCss/writeCss flow in check-dead-knobs.js tests so the test fails immediately
when the replacement does not change the fixture, and apply the same safeguard
to the other replace-based cases in the same file (including the D4 and D5
scenarios) using the same helper/readCss/writeCss pattern.
🪄 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: 1f3ae1b8-a42e-42dd-af7b-c2aac84cd7bd
⛔ Files ignored due to path filters (1)
dist/css-custom-data.jsonis excluded by!**/dist/**
📒 Files selected for processing (35)
.github/workflows/ci.ymlconfigurator/src/data/api-index.generated.jsoncore/print.csscore/tokens.cssdemos/full-api-demo-with-overrides.htmldemos/full-api-demo.htmldemos/ultimate-override.cssdocs/api-index.jsondocs/api-index.mddocs/architecture.mddocs/components.mddocs/llm-guide.mddocs/migration.mddocs/motion.mddocs/token-annotations.jsondocs/token-index.jsondocs/token-index.mddocs/tokens.mdoptional/components.cssoptional/forms.cssoptional/tokens.components.cssoptional/utilities.csspackage.jsonscripts/check-annotations.jsscripts/check-bundle-defs.jsscripts/check-dead-knobs.jsscripts/check-hook-tokens.jsscripts/check-llm-guide.jsscripts/check-mirrors.jsscripts/hook-tokens.jstests/a11y-patterns.spec.jstests/button.spec.jstests/check-llm-guide.test.jstests/check-source-audit-gates.test.jstests/print.spec.js
- check-annotations: anchor resolveMs so compound calc() expressions
resolve to null instead of yielding the first ms literal, while still
resolving the framework's scaled calc(Nms * var(--sf-motion-scale)) form
- check-dead-knobs: skip negated wiring disclaimers ("not wired to any
rule") so a truthful annotation can't trip the gate
- migration: document the breaking --sf-field-block default change from
var(--sf-space-l) to var(--sf-space-xs)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0114fqDEQ6xsTpmowzLiQWVp
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
2 participants
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.
Implements the six CI gates from #582 to enforce consistency between token declarations, annotations, and framework consumption. These gates prevent silent failures and documentation drift by mechanically verifying:
New audit scripts:
check-dead-knobs.js(D1): Fails if an annotation claims a token is "wired" (e.g. "Maps to @page margin") but no rule actually consumes itcheck-annotations.js(D2): Fails if a~Nmsor~Npxfigure in token annotations disagrees with the token's resolved valuecheck-mirrors.js(D4): Verifies three families of hand-maintained mirrors stay in sync:.sf-fluid-cq > *match their base formulascheck-hook-tokens.js(D5): Enforces the fallback-only hook-token policy — undeclared tokens consumed only viavar(--sf-hook, <default>)must be listed inscripts/hook-tokens.jsand documented indocs/llm-guide.mdcheck-bundle-defs.js(D6): Fails if a bundle consumes a token (without fallback) that it never declaresSupporting changes:
scripts/hook-tokens.js: Canonical list of fallback-only hook tokens (--sf-color-code-block-bg, --sf-color-code-block-text, --sf-overlap-host-pad) with documentationscripts/lib/parse.js: Enhanced withstripComments()helper for CSS parsingcheck-llm-guide.js: Updated to recognize hook tokens as live (D5) and tighten declaration detection to exclude bare consumption (D3)tests/check-source-audit-gates.test.js: Comprehensive negative tests confirming each gate still fails on its specific defectDocumentation & annotation fixes:
--sf-duration-instantannotation: ~50ms → ~100ms (matches actualcalc(100ms * var(--sf-motion-scale)))--sf-duration-fastannotation: ~100ms → ~150ms (matches actualcalc(150ms * var(--sf-motion-scale)))--sf-duration-normalannotation: ~200ms → ~300ms--sf-duration-slowannotation: ~350ms → ~500ms--sf-field-padding-inlinedefault fromvar(--sf-space-l)tovar(--sf-space-xs)in token-indexcore/tokens.css,core/print.css,optional/forms.css,optional/components.css, andoptional/tokens.components.cssexplaining fallback patterns and override hooksCI integration:
npm run check:*commands topackage.json.github/workflows/ci.ymlafter the existing token checksAll gates are designed to be conservative (hard-fail only on unambiguous violations, warn on edge cases) and support
SLASHED_ROOTenvironment variable for testing against fixture trees.https://claude.ai/code/session_012i9DDskbZqmuMeDwQMjPjF
Summary by CodeRabbit
New Features
Bug Fixes
Documentation