Uh oh!
There was an error while loading. Please reload this page.
fix(design-system): stop the radius scanner from skipping wrapped values - #1520
Merged
Conversation
The CSS governance scanner matched `border-radius` values with the class `[^;}\n]+`. Excluding `\n` meant a declaration whose VALUE wrapped onto a second line matched nothing at all — the scanner skipped it in silence rather than reporting it, so any radius could escape the token contract just by being formatted across two lines: border-radius: 10px 12px; /* never scanned */ Neither prettier nor biome reflows such a value back onto one line, so a long four-corner shorthand or calc() reaches this state on its own. (A newline directly after the colon was always fine — the `\s*` there already spans it. Only a newline *inside* the value was affected, which is narrower than it first looks.) Widening the class to `[^;}]+` cannot make a match outrun its own declaration: `;` and `}` are still excluded, so it stops at the first declaration terminator or the end of the rule body either way. The regression test covering that is asserted alongside the fix. Found while reviewing #1514, which fixed a separate bug in the same contract (it accepted only invalid unspaced calc()). This is the remaining scanner gap.
`main` has been red since 17362dc (#1515); 0e80fe1 (#1359) inherited it. All three jobs failed, none of it visible in that PR's own checks because the earliest failure short-circuits the rest of the job. typecheck job - `format:check`: scripts/build-cursor-overlay.mjs was committed unformatted. Reapplied `biome format`; purely mechanical. - `knip`: #1515 added a second overlay entry point pair (permission-overlay{,-preload}.ts) but registered only the cursor overlay in knip.json, so both files read as unused. Registered them beside their cursor-overlay twins — they are loaded by path at runtime (loadFile / preload), never imported. - `knip`: asOsPermissionId was exported and referenced nowhere in the repo. It is an identity function (`return id`), so nothing is lost by dropping it — git history holds it if Stage 2 wants it back. Its sole import (OsPermissionId) went with it. test job - `check-console`: three console.warn sites in permission-overlay-main.ts (locale fallback, missing .app bundle, the controller log sink). These are main-process diagnostics that expose paths, not secrets — the same category as the daily-review and settings-runtime entries — so they are allow-listed with a reason per the script's own documented convention. e2e job - `settings.spec.ts:141` asserted a permission row draws exactly two grant buttons. #1515 added a third (拖拽授权) to precisely the row the fixture aims at — screen_recording is requestable, openable AND drag-grantable — so the `=== 2` filter matched nothing and the guard silently measured no rows. Relaxed to `>= 2`, which is what the contract was always about: a row whose actions track competes with the body. A wider actions track only makes the squeeze worse, so the three-button row is strictly the better subject. Verified locally: lint, format:check, build, typecheck, knip (both workspaces), check-console, and the full test:dist suite.
jackwenerforce-pushed
the
claude/modest-spence-135ff7
branch
from
July 27, 2026 08:52
717931f to
6e97744CompareUh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 27, 2026
jackwener added a commit
that referenced
this pull request
Jul 27, 2026
…name (#1537) The CSS scanner matched the property name as a bare substring, so `--os-track-border-radius: …` in base.css was scanned as though it were a plain `border-radius` declaration. Both such properties currently hold whitelisted tokens, so nothing failed — but the moment one held a raw px the offender would have read renderer CSS: border-radius: 4px; naming a declaration that does not exist anywhere in the stylesheet, and sending whoever has to fix it looking for the wrong thing. Keeping these in scope is deliberate, not merely tolerated. `.os-theme-maka` is OverlayScrollbars' theming API: the library's own CSS consumes those properties and renders a real scrollbar corner. A radius that reaches the screen is governed no matter which declaration spells it. So the fix is to capture the whole property name rather than to anchor the match and drop the coverage. The lookbehind is load-bearing: without it a prefixed property also matches at its inner `border-radius` and is reported twice. `\b` cannot do this job — `-` is a non-word character, so a word boundary already exists between `-` and `border`. Found while reviewing #1520; deliberately left out of that PR and #1535 as pre-existing and orthogonal. Verified: lint, format:check, typecheck, check-dead-css, and 2891 main-process tests. The new assertion fails against the old regex.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The gap
radius-converge-contract.test.tsscans renderer CSS forborder-radiusdeclarations that violate the radius token contract. Both scanner regexes matched the value with[^;}\n]+.Excluding
\nmeant a declaration whose value wraps onto a second line matched nothing at all — the scanner skipped it silently instead of reporting it. Any radius could escape governance just by being formatted across two lines:Prettier/biome will not reflow a long value back onto one line, so a four-corner shorthand or a long
calc()reaches this state on its own.One nuance worth recording
A newline directly after the colon was always fine — the
\s*there already spans it. Only a newline inside the value, after the first token, was affected:border-radius:⏎ 10px;border-radius: 10px⏎ 12px;border-radius: 10px 12px⏎ 14px 16px;border-radius: calc(var(--radius-modal)⏎ - 8px);The obvious minimal repro (
border-radius:⏎ 10px;) is not a repro. A regression test written around it would have been green against the buggy regex and guarded nothing.The fix
[^;}\n]+→[^;}]+onRADIUS_DECL_REandRADIUS_LONGHAND_RE.Greediness stays bounded regardless:
;and}are still excluded, so a match cannot outrun its own declaration or rule body.splitCorners()already tokenizes on any whitespace at paren depth 0, newlines included, so wrapped values tokenize correctly once matched at all.Tests
Two cases added next to the existing scanner tests:
reads values that wrap onto a second line— five offenders that must be flagged (incl. a longhand, and a wrapped+ 8pxproving the shrink-only rule still applies across the break), plus three valid multi-line values that must pass.stops each match at its own declaration boundary— three declarations across three rules must capture separately and not bleed, i.e. the widened class did not become a false-positive machine.Verified the first test fails against the old regex and passes with the fix. The second passes both ways by design — it is an over-reach guard, not a bug repro.
Gate
npm run build:main— cleannode --test "dist/main/**/*.test.js"— 2878 pass, 0 fail (2876 baseline + 2)node scripts/check-dead-css.mjs --check— no dead classes ✓biome checkon the changed file — cleanContext
Found while reviewing #1514, which fixed a separate bug in this same contract (it mandated invalid unspaced
calc()). That fix already landed; this is the remaining scanner gap.