feat(studio): fail the suite on a class that resolves to nothing - #3623
Conversation
a6424cd to
4b0ef5a
Compare
2e4c3b9 to
af22c5a
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
af22c5a to
23cef6b
Compare
Tailwind silently drops a class it cannot compile, so `rounded-button` renders as no radius at all and nothing goes red. Two tests close that: - the token gate compiles Studio's entry stylesheet with every class the source claims and reports `file: class` for anything that produces no selector. Tailwind is the judge, so there is no allowlist. - the hex ratchet counts colour literals per file against a committed baseline. It fails on a rise, and on a fall it prints the command that banks the lower number. The baseline is only written under a named flag, never as a side effect of a run. The gate's first run found seven names the markup has always asked for and no config has ever defined: rounded-button, shadow-btn-primary, bg-surface-hover, bg-accent-red, ease-standard, text-2xs and two panel backgrounds. Each is added to theme.css, the semantic ones by role and the rest as deprecated aliases for the sweep to remove. It also found a dead state class on the timeline clip, styled nowhere, which is deleted.
The extractor treated a string as a class candidate only when its binding matched Styles/Classes/ClassName(s), so the same twelve classes were gated in `sizeStyles` and invisible in `buttonSizes`. A name is not a contract. Candidate-ness now follows one file's data flow: an identifier used inside a className attribute or a class-building call marks whatever is bound to it as classes, under any name. A subscript is excluded, so the key of `variantStyles[variant]` is not mistaken for a class list. The name-based anchor stays for the map whose only consumer is another module. The sweep this opened found one class no config has ever defined: the automation menu's hover row asked for a background that compiled to nothing, and now uses the token the rest of that row already uses.
…urs instead of literals
…ts are not counted
23cef6b to
c92a90c
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at c92a90c7 off the merge-base diff (+849/-22, 12 files). Approving. The ratchet is well built — it fails in both directions, which is the part most ratchets get wrong.
The scope did not shrink, despite the shorter one-line summary. All four colour-ratchet commits are still here (f12321b7, b863e88c, 182f6748, c92a90c7), so "7 commits identical by range-diff" holds and the blurb is just abbreviated. Worth knowing for whoever reads the summary rather than the commits, because the re-banked baseline lives in the half the summary now omits.
theme.test.ts at −15/+2 is a clean extraction, not a trim. loadStylesheet, STYLES_DIR and TAILWIND_DIR moved verbatim into styleSources.ts; no test case was lost, and the call site still exercises them.
The head commit fixes two real undercounts, and I traced both against the old regex. The previous stripper ran /\*[\s\S]*?\*\//g over raw text, so accept="image/*" opened a block comment that ate everything up to the next */ — swallowing any real #161618 in between. And the line-comment rule (^|[^:"'])//.*$inspected only **one** character before//, so `` x//y`` tripped it and ate the rest of the line. Both were silent undercounts, i.e. the ratchet was quietly generous. Matching strings first and keeping them fixes both, and passinglineComments=falsefor.cssis right — CSS has no//, so url(//cdn…)` must survive.
These gates do execute in CI. I checked rather than assumed, since a ratchet that only runs on a developer's laptop is not a ratchet: Test and Tests on windows-latest: studio-core are both running the Studio suite at this head.
Should-fix: the baseline was banked before the counting rule changed
hex-baseline.json is written by exactly two commits — 2392c8b5 and 182f6748 ("bank the lower baseline now that comments are not counted"). The counting rule then changed again at c92a90c7, and the baseline was not re-banked.
Because c92a90c7 only ever un-eats text, counts can move up or stay equal, never down — and a rise fails the ratchet. So this is correct only if no Studio file happens to contain a string holding /*, */ or // ahead of a colour literal. Your body says the styles suite passes at this head, so empirically it holds today.
The fragile part is the ordering, not the outcome. This file's own header promises "the number in git is one a human accepted" — but 423 across 80 files was accepted under the previous rule. A HEX_BASELINE_WRITE=1 re-bank as the last commit would make the committed number one that was accepted under the rule that actually ships, and would turn "no file happens to hit this" from luck into a recorded fact.
Minor: listSourceFiles's from sets the key base, not the traversal root
walk(SRC_DIR) is hardcoded, so from only decides how keys are named. hexRatchet passes REPO_ROOT and gets repo-relative keys over Studio-only content, which is exactly what it wants — every baseline key is packages/studio/…, so it's correct today. But the name reads like a root to scan from, and compare() treats an unknown key as baseline 0. A later caller who changes from hoping to widen coverage gets 80 re-keyed entries instead. It fails loudly rather than silently, so this is an API-shape note, not a defect — the doc comment already says "keyed by path relative to from", and naming the parameter keyBase would close it.
Gate mechanic, not a code finding
Studio and player captures is red because the body has no ## Before / ## After section, which any packages/studio change requires. That is a body edit, not a code fix, and the ## No visible change escape doesn't apply here since .tsx and .css both changed. Flagging it only so whoever presses merge isn't looking for a defect that isn't there.
— Rames
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed the delta only: c92a90c7..384b6239, 2 commits, 3 files, +10/−2. c92a90c7 is still position 7 of 9 in the commit list, so there was no rebase and the incremental compare is the honest diff.
Approving at this head.
The CodeQL fix is right, and it changes no behaviour
Worth stating plainly, because the two commits are easy to read as one fix: the $cls miss was not caused by the escaping.
boundStrings only ever receives names produced by REFERENCE = /(?<![.\w$])[A-Za-z_$][\w$]*/g, so a name is drawn from [A-Za-z0-9_$] and $ is the only regex metacharacter it can contain. I ran both escapes over the names that grammar admits — $cls, cls, _x, a$b, x$, $, $$, Styles, btnStyles, $Styles — and escapeRegex(name) is byte-identical to the old name.replace(/\$/g, "\\$") on every one. So nothing was injectable, and nothing about the output moves.
That is not an argument against the commit. A hand-rolled partial escape next to a new RegExp is exactly the shape the alert is for, it is correct only because of a grammar two functions away, and reusing the repo's own escapeRegex removes that dependency. I am flagging it only so the $cls fix does not get attributed to the escape later — the behaviour change is entirely the \b → lookaround swap.
The new test discriminates
I ran the old and new anchors against the exact source in follows an identifier that contains a dollar sign:
const $cls = "flex-none";
const a = <div className={$cls} />;
old \b\$cls\b… -> no match
new (?<![\w$])\$cls(?![\w$])… -> match
\b asserts a boundary between a word and a non-word character. $ is not a word character, so in const $cls the space-then-$ pair has no boundary at all and the anchor could never fire. The test fails on the old code and passes on the new one, which is what makes it a test rather than a record.
The swap loses nothing it should keep
Across the shapes I tried, the only match the new anchor gives up is the name cls binding to const $cls = "a" — \b matched cls inside $cls, which was a false positive, and (?<![\w$]) correctly refuses it. It gains $cls, a$ and $Styles. Ordinary names (const cls =, let cls=, const cls: string =) and the existing negative case ({ cls: "a" }) are unchanged.
Two things I checked rather than assumed:
- Lookbehind is not a new platform floor here.
REFERENCEandSUBSCRIPTin this same file already use(?<!…)/(?<=…), so the requirement predates this commit. - The new import drags in nothing.
sourcePatcher.tshas no imports of its own, soclassCandidates.tsstays the pure text-in/candidates-out module its header promises, andclassCandidates.tsis added by this PR, so no existing consumer is affected by the direction of the dependency.
Minor, non-blocking: the same $-blindness survives one function away
The *Styles/*Classes binding anchor still leads with \b\w+:
/\b\w+(?:Styles|Classes|ClassNames?)\b(?:\s*:[^=;{}()[\]]*)?\s*=(?!=)\s*/g
Measured: const $buttonStyles = "a" anchors fine (the \b sits between $ and b), but const $Styles = "a" and const $Classes = "a" do not, because \w+ needs at least one word character ahead of the suffix. It needs a binding named literally $Styles/$Classes that is never referenced elsewhere in its file, so it is narrow enough to leave — noting it only because it is the same class of bug this head just fixed.
Still open from c92a90c7, unchanged by this delta
The baseline in hex-baseline.json is still the one banked at 182f6748, before c92a90c7 changed the counting rule. My reasoning there holds as written: c92a90c7 only un-eats text, so counts can rise or hold but never fall, and a rise fails the ratchet — correct today, but the committed number was accepted under the previous rule. A HEX_BASELINE_WRITE=1 re-bank as the final commit would still be the cheap way to close it. Non-blocking, same as last time.
One direction note: this delta makes the extractor see strictly more class lists, so a green Studio suite at this head is a stronger statement than it was at the last one.
Gate mechanics, not code findings
Studio and player capturesis green at this head — the## Before/## Aftergap I flagged atc92a90c7is resolved.- The red
Testat this head never ran a test. It belongs to run35450764673, which was cancelled by the newer run in the same concurrency group. Its first step isRequire producer source tests, guarded byif: needs.producer-source-tests.result != 'success'— a cancelled upstream is not'success', so the guardexit 1s beforeactions/checkout, and every build and test step in that job reportsskipped. The liveTest(run35450793316) has that guard skipped, checkout green, and is executingbun run --filter '!@hyperframes/producer' testright now. Whoever — or whatever — reads a singleTestrow at this head is reading a job that failed a policy guard on an empty workspace, not a suite result. - CI is triggered by
pull_requestandpushonly, with nopull_request_reviewentry, so submitting this review neither starts nor cancels a run at this head.
— Rames
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-stamp at 8f3621c3. 384b6239 — the head I approved last — is still commit 9 of 10 on this branch, so there was no rebase and compare/384b6239...8f3621c3 is the honest incremental delta: one commit, two files, +23/−2, all of it in packages/studio/src/styles/styleSources.ts and its new test.
The fix is in the right place, and it closes more than the body claims
toPosixPath is applied once inside listSourceFiles, and the same normalized string is handed to both keep(...) and files.set(...). That placement is what makes it a one-line fix, because the Windows breakage was three compounding things, not one:
- Every scanned file keyed
packages\studio\src\...never matches ahex-baseline.jsonkey, sobaseline.files[file] ?? 0gives every file an allowed count of 0 — any file with a colour literal reads as risen. - Symmetrically, every one of the 80-odd baseline entries is absent from
counts, so the second loop incompare()reports them all as fallen. - The one the body doesn't mention, and the sharpest:
isTokenSourcetests/styles\/(theme\.css|tailwind-preset\.shared\.js|tailwind-preset\.ts)$/. With backslashes that never matches, sotheme.css— the file whose entire job is to hold colour values — gets scanned and counted.
Normalizing at the lister fixes the predicate and the key together, which is exactly the coupling (3) needs; normalizing at the call sites would have fixed (1) and (2) and left (3) live. Worth stating in the body, since "the baseline matches on every OS" undersells it.
The extension predicates (/\.tsx?$/, /\.test\.tsx?$/) are separator-agnostic, so nothing else in isScanned or in tokenGate.test.ts's studioSources() was affected either way.
The change is provably inert off Windows
path.relative only emits backslashes on Win32, so on Linux and macOS toPosixPath is the identity on every input listSourceFiles can produce. The whole behaviour change is Windows-only — nothing on the platforms CI already had green can move, which is the right shape for a fix landing this late in the branch.
Where the guard actually lives
Worth being precise about, because it decides whether this can be silently reverted:
- Tests 1 and 2 call
toPosixPathdirectly with a literal backslash string, so they discriminate on every platform — but they pin the helper, not its wiring. - Test 3's two assertions —
seen.every(file => !file.includes("\\"))andkeys() === ["styles/styleSources.ts"]— are both satisfied on POSIX with or without thetoPosixPathcall, sincepath.relativealready returns forward slashes there.
So deleting the toPosixPath(...) from listSourceFiles leaves the entire Linux suite green. The wiring is pinned only by Tests on windows-latest, and I checked that lane does carry it: both matrix lanes run bun run --cwd packages/studio test -- --shard=1/2 | 2/2, so the styles suite including hexRatchet.test.ts executes on Windows. The guard is real — it just lives on one runner, which is worth knowing before anyone trims that job.
Nit, non-blocking: toPosixPath rewrites unconditionally, so a POSIX filename containing a literal backslash would be mangled. Pathological in a TS package; not worth code.
Standing should-fix — this one retires itself
I've carried a note since c92a90c7 that hex-baseline.json was banked at 182f6748, before the counting rule changed again. That's still literally true (the baseline's last touch is 182f6748; hexRatchet.test.ts changed after it at c92a90c7), but it does not need action, and I was wrong to leave it open-ended:
verdict() fails on both directions — risen returns a message, and so does fallen, with the final assertion demanding []. A passing ratchet therefore proves the banked numbers equal today's counts exactly. So a green styles suite is itself the re-bank check, and it also proves c92a90c7 changed no file's count in the scanned corpus. Consider this one closed on green rather than needing a chore commit.
Gate mechanic, out of scope, but it has now misled the loop three times today
regression is red at this head with a 2-second duration: Set up job → Check results failure → Complete job. No shard ran; the matrix was cancelled by cancel-in-progress and Check results prints "One or more regression shards failed" for any non-success result, including cancelled. The message is false — nothing failed.
The repo already contains the correct version of this, one workflow file over. windows-render.yml's Require all Windows test lanes handles the same situation by asking the API whether a newer run of the same workflow exists for the branch and head repo, passing only then, and still failing on push runs, a manual cancel and any API error. That is why Tests on windows-latest is legitimately green in 5 seconds off cancelled lanes at this same head, while regression is red off the same cause. Lifting that step into regression.yml would stop the automated reader from seeing a defect that is not in anyone's diff.
Approving — the delta is correct, minimal, and lands where the coupling is.
— Rames
3 participants
Lands unit U2 (token gate and hex ratchet) of the Studio design-system foundation, on main now that the theme PR (#3621) has merged. Ratchet baseline: 423 colour literals, down from 475 because comments are no longer counted; this PR raises no file.
What
Two tests in
packages/studio/src/stylesthat fail the Studio suite when a design value stops resolving.tokenGate.test.ts). Compiles Studio's real entry stylesheet through Tailwind v4'scompile()with every class the source claims as the candidate list, then reportsfile: classfor any candidate that produces no selector.hexRatchet.test.ts). Counts colour literals (#hex,rgb(),rgba(),hsl(),hsla()) per file against a committed baseline. A rise fails; a fall passes and prints the command that banks the lower number.classCandidates.ts, the pure extractor both the gate and its own tests use, plusstyleSources.ts, the small shared reader the three style tests now share.Why
Tailwind has no strict mode for the classes it finds in markup. A class it cannot compile is silently dropped, so a button asking for
rounded-buttonrenders with no radius, review reads it as real, and nothing anywhere goes red. Colour literals have the same shape of problem in reverse: they always work, so they accumulate.Both checks are vitest tests inside
packages/studioso they run in the required Test job. A lint-only rule would not block.How
Tailwind is the judge, so there is no allowlist. Static utilities, arbitrary values, theme tokens and Studio's own hand-written CSS rules all appear in the emitted sheet; a name nothing defines does not. Two structural exceptions, both namespaces rather than lists: Tailwind's
groupandpeervariant markers, which by design emit no rule, and thehf-prefix Studio reserves for its own semantic hooks, which are not utilities and carry no design value.The extractor is deliberately narrow. It reads
className/classattributes, arguments to class-building calls,*class/*classNameproperties, and bindings named*Styles/*Classes/*ClassName(s)(the lookup-table shape the primitives use, which never reaches aclassNameliteral). Anchors are matched against a masked copy of the file with comment and string bodies blanked, so prose in a doc comment and generated markup inside a template literal are both invisible to it. A template chunk that touches an interpolation is dropped at that edge.The ratchet's baseline is per file, not per repository. Two sweep PRs touching the same file conflict on that file's line, which is exactly when a recount is wanted. A file absent from the baseline has a baseline of zero, so a rename cannot smuggle colours past it. The baseline is only written under
HEX_BASELINE_WRITE=1, never as a side effect of a normal run, and a missing baseline fails with that flag named.First-run false-positive list. The gate was run over the tree before it was allowed to fail the suite. Every entry is accounted for; none is allowlisted.
activeTool === "razor"), lookup keys, inline CSS texthf-fx-*,hf-automation-*,hf-volume-rowrounded-button,shadow-btn-primary,bg-surface-hover,bg-accent-red,ease-standard,text-2xs,bg-panel-bg-soft,bg-panel-bg-2theme.cssis-microon the timeline clipstudio.css, this one has none anywhere. Dead class removedThe new tokens:
--radius-buttonand--ease-standardare named by role and alias--radius-mdand--ease-out-quint;--shadow-btn-primaryis a new value, a contact shadow plus an ambient one plus an inner top highlight for the one light surface in a dark UI;--color-surface-hover,--color-accent-red,--text-2xs,--color-panel-bg-softand--color-panel-bg-2go in the deprecated block as aliases for the sweep to remove, so there is still one source per decision.Adding them changes what renders: those classes previously compiled to nothing, so the affected surfaces had no background, no radius and no shadow at all. The alias targets are a judgement call and worth a look in review.
Baseline total: 475 colour literals across 104 files. Higher than earlier hand counts because this regex also counts
rgb(),rgba(),hsl()andhsla()alongside hex, as the plan requires. Whatever this rule sees is the number.Test plan
bunx vitest run --poolOptions.forks.maxForks=4inpackages/studio: 431 files, 4778 tests, all passing.rounded-nonesuchand a hex toui/Button.tsxfails both, each naming the file and the offending string.cn()arguments, variant stripping, arbitrary-value counting, interpolation edges, unterminated regions and non-class strings.bun run typecheck,bun run build,bunx oxlint,bunx oxfmt --checkall clean.bunx fallow audit --base origin/main --fail-on-issuespasses, at the same counts as the branch point.Scoped out, with owners
hf-hook classes are exempt, not fixed (owner: the panel sweep PRs; feat(studio): header and inspector tabs on the shared primitives #3624 for the header and inspector tabs, the later sweep PRs for the other panels). 81 names are referenced in markup and styled nowhere in the repo. Deciding which are intentional hooks and which are leftovers needs a look at each panel.hover:resolves asbg-xand a misspelled variant is not caught.classCandidates.test.ts"strips variant prefixes, including bracketed ones" (line 68) pins the stripping; no test pins the miss itself.hexRatchet.test.ts, the comment and string cases); the count only has to be stable and monotone.hf-color-grading-*andtimeline-cliprules instudio.cssare untouched.Second commit in this head: gate a class by where it is used
fix(studio): gate a class by where it is used, not what it is called. The first version decided candidate-ness by the name of the binding: onlyStyles/Classes/ClassName(s)counted. A primitive that keeps its size classes in a record namedbuttonSizeswas therefore invisible to the gate, sorounded-holograminside that record stayed green while the identical string in aclassNameattribute went red. A name is not a contract.Candidate-ness now follows one file's data flow. Every identifier used inside a
className/classattribute or a class-building call marks whatever that identifier is bound to elsewhere in the file as a class list, under any name. The four masking rules that kept the first run's false positives out (comments, generated code inside template strings, comparison operands, non-class call arguments) are unchanged, and one more joins them: a subscript is excluded, so the key ofvariantStyles[variant]is not read as a class and the parameter defaultvariant = "ghost"is not read as a class list. The name-based anchor is kept as a second source rather than the only one, because it is the only thing that can see a class map whose only consumer lives in another module, which single-file data flow cannot follow.Running the widened gate over the whole tree produced two new hits, one bug in the extractor and one real:
ui/Button.tsx(md,ghost,secondary) were lookup-key defaults, not classes. That is the subscript rule above, and a unit test pins it.bg-panel-bg-3in the automation selection menu is a real one: no config has ever defined that token, so the menu row's hover background compiled to nothing. It now uses the hover token the rest of that row already uses.Both the extractor test and the gate test are proven non-vacuous: with the old extractor restored, the
buttonSizesfixture and the subscript fixture fail and the gate reports nothing.Test plan for this commit:
bunx vitest run src/styles --poolOptions.forks.maxForks=4(38 passing, 4 files), the full Studio suite once (431 files, 4781 tests, 1 skipped, all passing),bun run typecheck,bunx oxlintandbunx oxfmt --checkon the changed files, andbunx fallow audit --base origin/main --fail-on-issuesclean.classCandidates.tsis 337 lines.Still not covered: the data flow is one file deep and one hop long. A class list assembled through two intermediate variables, or imported from another module under a name that does not end in
Styles, is still only reached by the name-based anchor.Restack and root fixes (this head)
git range-diffof old tip against the replay shows all six as identical before the fixes below.#2291that read as hex) are stripped, with strings matched first so"image/*"or"a//b"never opens a comment. Its own commit, with a test for comment-shaped text inside a string.TimelineCanvas.tsxandPreviewGuides.tsxare now theme colours (color-mixon--color-accent), not baseline growth. Its own commit.Conflict hunks against both parents
theme.css: kept main's trimmed comment plus this PR's--shadow-btn-primaryblock, oxfmt-formatted.theme.test.ts: helpers now come from./styleSources; bothbuildSourceandbuildkept.Checks
Full Studio suite on miga at c92a90c (the head before the last commit): 464 files passed, 5081 tests passed; tsc 0; format clean; comment-check clean.
The last commit (
fix(studio): escape every regex metacharacter when following a bound class list) answers the CodeQLjs/incomplete-sanitizationalert:boundStringsnow builds its pattern through the repo's existingescapeRegex(exported fromutils/sourcePatcher.ts) instead of escaping only$. A second commit fixes a defect the review found in the same function: the\banchor never matched a name starting with$, so a class list bound to$clswas silently missed; it is now(?<![\w$])and(?![\w$]), and the new test fails without it (reproduced in node: old anchor false, new true). Suite ran in CI only for this commit, remote box awaiting re-auth; the pre-commit typecheck was skipped locally because this worktree lacks the generated core files, CI typechecks it.Before
Origin main, the four spots this PR touches, rendered in Chromium on the shared test box with the exact CSS each side ships (computed values printed beside each swatch).
After
Per spot:
rgba(60, 230, 172, 0.55)border andrgba(60, 230, 172, 0.12)fill; aftercolor-mixon--color-accent(#3ce6ac), computed ascolor(srgb 0.235294 0.901961 0.67451 / 0.55)and/ 0.12, which is 60, 230, 172 over 255.rgba(255, 255, 255, 0.7)before,color(srgb 1 1 1 / 0.7)after.hover:bg-panel-bg-3names a token that nothing defines (no match in the Studio tree), so it compiled to nothing and the row had no hover background. It now uses--color-panel-hover, #27272a (rgb(39, 39, 42)).is-microremoved from the timeline clip: no visible change. No stylesheet or script in Studio has a rule or reader for that class.Rendered from the shipped CSS values, not a live session.
Windows path fix (last commit)
The ratchet failed on windows-latest because
listSourceFileskeyed files bypath.relative, which yields backslashes there, so every file read as absent fromhex-baseline.json(baseline 0). The keys and thekeepfilter now go through onetoPosixPathinstyleSources.ts. Styles suite ran on a Linux box (5 files, 45 tests passed); the Windows run in CI is what confirms it on Windows. Of the new tests, the twotoPosixPathcases witness the normaliser anywhere, and the keep-filter/keys case only bites on Windows, since POSIX already returns forward slashes.