Review 5260

#5260 — feat(cli): doctor detects CSS theming escapes and swizzled components

josephfarina (internal) · OPEN · view on GitHub

Verdict: request changes — two builder footguns (R1i), both in hand-maintained tables (R7b).

Head reviewed: b3d0c44ca0b300a5d1ebe4a5563cbe2f129078b1. Stacked on #5226, still open. 229 commits behind main. No prior reviews.

Problem

A builder themes their app and the theme does not take, because their own CSS out-ranks it — a :root token definition sits outside the theme's @scope. Or a swizzled component renders with no styles at all, because its StyleX source never compiled: no build error, no warning. Today doctor says nothing.

Solution

Two new doctor checks: a CSS scan for theming escapes, and a swizzled-component drift check that looks for a StyleX compiler in the project's build config.

Impact

Anyone running astryx doctor. New findings appear on projects that were previously clean — including, per the findings below, false ones.

API

Two new check ids in doctor's output. No new config keys.

Theme targets

No new theme targets.

Breaking

• API — no. Visual — no, CLI output. Theme — no.

Performance & resources

MAX_FILES bounds files, not the walk: 68ms on docsite, 1.8s on the repo root, 4.2s cold on a 1,550-directory project with nothing to find — against the body's claimed "~250ms".

Risk class

Needs judgement — new detection that can fail a build.

Judgement

request changes

1. `theme-drift.mjs:334` hardcodes `data-variant` in the fix text, but components
   reflect the prop KEY
   → docsite's own `.astryx-pagination-dot.active` is `[data-active="active"]`;
     `[data-variant="active"]` matches 0 elements, so the one true positive in the
     repo ships advice that silently drops the rule    · theme-drift.mjs:334

2. `STYLEX_COMPILERS` omits `@stylexjs/webpack-plugin` and `@stylexjs/rollup-plugin`
   → a Webpack user who swizzles gets `[fail]`, exit 1 in CI, and advice to add a
     compiler they already have. Our own styling doc tells them to install exactly
     those two                        · theme-drift.mjs:188 · styling.doc.mjs:353

R14b counts: docsite 1 finding (true positive, the author's claim holds); three other apps clean; a plausible app stylesheet → 4 findings, 0 real (html matches inside .markdown-html-body; token families are prefixes).

R7b: main already ships the derivation this needs — foundation/discovery/theming-targets.mjs carries each target's className, props and states, and says "nothing here is a second registry". This head predates it.

The review, as posted

Thanks for this — the negative controls and the line-number care are the parts most detectors skip.

Two to fix. The bare-class fix always names data-variant, but a component reflects the prop key: docsite's own .astryx-pagination-dot.active is [data-active="active"], and [data-variant="active"] matches nothing — so following doctor drops the rule (theme-drift.mjs:334).

STYLEX_COMPILERS is missing @stylexjs/webpack-plugin and @stylexjs/rollup-plugin, the two our own styling doc tells people to install (styling.doc.mjs:353). A Webpack user who swizzles gets [fail], exit 1 in CI, and advice to add a compiler they already have.

Same shape both times, and main now has the answer: foundation/discovery/theming-targets.mjs carries each target's className, props and states. Merge main and derive from it, loaded only when there's a hit to name so the 700ms stays off the clean path.

Any reason to keep the lists local?

What changed before posting

Posted as drafted.

Open, for the maintainer

doctor gains its first check that fails on hand-written consumer CSS, and there is no per-check suppression anywhere in astryx.config.mjs — a deliberate --_* write becomes an unexitable CI failure. That is a decision, not the author's to make.