Repository navigation
feat(cli): doctor detects CSS theming escapes and swizzled components - #5260
Conversation
Two read-only, text-only checks for the cases where an app provably steps outside its theme. CSS escapes covers three: a write to a private --_* var, which theme build already rejects and which is no more valid in raw CSS; a system token redefined in :root/html/:host, which sits outside the theme's @scope and so beats every theme at once; and the deprecated bare prop classes (.astryx-button.primary) in place of the reflected data attributes. Swizzled components fails on exactly one combination — ejected source that imports StyleX with no StyleX compiler configured. That is not a build error. The component renders completely unstyled, with no warning, which makes it the most expensive thing here to discover by hand. Three details are load-bearing, each of which produced a false positive before it was handled. Generated theme CSS is skipped, because the pipeline emits private vars and bare classes itself and judging its own output as consumer error dominated the early findings. Comments are blanked while preserving newlines, because stripping them collapses lines and shifts every reported line number. And bare-class detection matches only the enumerated prop and state values, so a consumer's own class chained onto an Astryx class is left alone. Half the tests are negative controls. A detector that only proves it finds things says nothing about whether it will bury the reader in noise, and every false positive here is one an agent would act on. Co-authored-by: Cursor <cursoragent@cursor.com>
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
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?
[Reviewed by Robohands]
| message: | ||
| `${bareClasses.length} selector(s) use deprecated bare prop classes, e.g. ${first.detail} at ` + | ||
| `${at(first, ctx.cwd)}.`, | ||
| fix: `Target the reflected data attribute instead: .${base}[data-variant="${value}"].`, |
There was a problem hiding this comment.
Components reflect the prop key — [data-variant="active"] matches nothing for .active.
| } | ||
|
|
||
| /** StyleX compiler plugins. Swizzled StyleX source is inert without one. */ | ||
| const STYLEX_COMPILERS = [ |
There was a problem hiding this comment.
Missing @stylexjs/webpack-plugin and @stylexjs/rollup-plugin — both in our own styling doc.
Review found two bugs, both from hardcoding a list.
The bare-class fix always named data-variant, but a component reflects the
prop *key*: docsite's own .astryx-pagination-dot.active is
[data-active="active"], because Pagination renders
themeProps('pagination-dot', {active: 'active'}). Following the advice
replaced a working rule with a selector that matches nothing — the exact
class of confidently-wrong, mechanically-applicable fix this check exists
to avoid.
STYLEX_COMPILERS omitted @stylexjs/webpack-plugin and
@stylexjs/rollup-plugin, the two `astryx docs styling` tells people to
install. A Webpack user who swizzled got [fail], exit 1 in CI, and advice
to add a compiler they already had.
Both lists are gone. collectThemingTargets and the component docs now
supply the states, the prop keys and their enumerated values, so the
replacement selector is derived rather than guessed and a token that is
neither a state nor a documented prop value is recognised as the
consumer's own class and dropped. That also removes the false positive the
curated list only avoided by omission.
The enumeration costs ~300ms, so it is loaded lazily and keyed by core
directory: a project with no bare classes to name resolves in 3ms.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both correct, and both were the same mistake — I hardcoded a list rather than reading the one that exists. No reason to keep them local; merged main and derived from The
A useful side effect: a token that is neither a state nor a documented prop value is now recognised as the consumer's own class and dropped.
On the 700ms: the enumeration measures ~300ms here and is loaded lazily, keyed by core directory. A project with no bare class to name resolves in 3ms — the clean path never pays it. When core cannot be resolved at all the findings are dropped rather than reported without a usable fix, since guessing the attribute is what caused this. 48 tests, including one per resolution path and one pinning the drop-when-docs-unreachable behaviour. |
cixzhang
left a comment
cixzhang
left a comment
There was a problem hiding this comment.
Thanks — deriving both from theming-targets is the right fix. Verified: docsite now gets [data-active="active"], and a webpack or rollup swizzle project exits 0.
[Reviewed by Robohands]
…#5260) * feat(cli): doctor detects CSS theming escapes and swizzled components Two read-only, text-only checks for the cases where an app provably steps outside its theme. CSS escapes covers three: a write to a private --_* var, which theme build already rejects and which is no more valid in raw CSS; a system token redefined in :root/html/:host, which sits outside the theme's @scope and so beats every theme at once; and the deprecated bare prop classes (.astryx-button.primary) in place of the reflected data attributes. Swizzled components fails on exactly one combination — ejected source that imports StyleX with no StyleX compiler configured. That is not a build error. The component renders completely unstyled, with no warning, which makes it the most expensive thing here to discover by hand. Three details are load-bearing, each of which produced a false positive before it was handled. Generated theme CSS is skipped, because the pipeline emits private vars and bare classes itself and judging its own output as consumer error dominated the early findings. Comments are blanked while preserving newlines, because stripping them collapses lines and shifts every reported line number. And bare-class detection matches only the enumerated prop and state values, so a consumer's own class chained onto an Astryx class is left alone. Half the tests are negative controls. A detector that only proves it finds things says nothing about whether it will bury the reader in noise, and every false positive here is one an agent would act on. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): derive bare-class advice and the compiler list from real data Review found two bugs, both from hardcoding a list. The bare-class fix always named data-variant, but a component reflects the prop *key*: docsite's own .astryx-pagination-dot.active is [data-active="active"], because Pagination renders themeProps('pagination-dot', {active: 'active'}). Following the advice replaced a working rule with a selector that matches nothing — the exact class of confidently-wrong, mechanically-applicable fix this check exists to avoid. STYLEX_COMPILERS omitted @stylexjs/webpack-plugin and @stylexjs/rollup-plugin, the two `astryx docs styling` tells people to install. A Webpack user who swizzled got [fail], exit 1 in CI, and advice to add a compiler they already had. Both lists are gone. collectThemingTargets and the component docs now supply the states, the prop keys and their enumerated values, so the replacement selector is derived rather than guessed and a token that is neither a state nor a documented prop value is recognised as the consumer's own class and dropped. That also removes the false positive the curated list only avoided by omission. The enumeration costs ~300ms, so it is loaded lazily and keyed by core directory: a project with no bare classes to name resolves in 3ms. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This branch was successfully deployed
2 participants
Summary
Two read-only, text-only
astryx doctorchecks for the cases where an app provably steps outside its theme. No modules are imported or evaluated and the scan is bounded, so both are cheap enough for the default run (~250ms added) and safe in CI.CSS theming escapes covers three cases:
--_*var.astryx theme buildalready rejects these in a theme; they are no more valid in raw CSS, so this is afailto match.:root/html/:host. A theme applies inside@scope ([data-astryx-theme=...]), so a global definition sits outside it and overrides every theme at once..astryx-button.primary) in place of the reflected data attributes. The fix names the exact replacement selector.Swizzled components fails on exactly one combination: ejected source that imports StyleX while no StyleX compiler is configured. That is not a build error — the component renders completely unstyled, with no warning — which makes it the most expensive thing here to find by hand. Otherwise it is informational, noting that swizzled copies stop tracking upstream fixes and no longer respond to theme component overrides.
Three details that are load-bearing
Each of these produced a false positive before it was handled, found while testing the approach against this repo:
apps/docsite..astryx-card.my-highlight— a consumer's own class — trips it.Against this repo the pair reports exactly one finding, and it is a true positive: a hand-written
.astryx-pagination-dot.activeinapps/docsite/src/app/globals.css:174.Test plan
node_modules/dist, a project with nothing swizzled, and an unrelated directory namedastryxcheck:cli-structure,readme:check,check:changesets, golden suiteassets/templates/blocks/apps/docsite— 1 finding, manually confirmed genuineMade with Cursor