-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5529
bhamodi · open, request changes drafted · reviewed at 8d870ee3bb2 · view on GitHub
Verdict: request changes — six of seven assembly paths are closed, and the changeset says all of them are
The review asks for the prose rules to route through the same check, because generateProseRules interpolates 11 theme token values straight into rule text and joinDeclarations never reaches them — so the guarantee the changeset ships is not true yet. It did not merge because a typed public token still escapes its declaration and paints the page, and because the build is red on the PR's own new test.
generateThemeRules assembles CSS by string concatenation, so a theme value carrying ; or } closes its declaration and whatever follows it is parsed as CSS in the injected <style>. The person this reaches is an app that builds its theme from stored input — the PR names brand colours and white-labelling — where a tenant-supplied string becomes a rule that paints the whole page instead of one property.
This is the honest "defect nobody has hit yet" exception. The reachable state is named and it is real; nothing in the repo demonstrates a consumer doing it, and the PR does not claim one. That is fine for a hardening fix — but it means the guarantee is the deliverable, and a guarantee that is 90% true is the thing to check hardest.
(2 decisions · ~40 runtime lines of 132)
- Property names must match a name grammar and values must not contain
;,{,},/*or a control character — the fix. - A violation is dropped with a
console.warnrather than thrown — the failure mode.
Before, the theme system built its stylesheet by gluing strings together, and a value containing the characters CSS uses to end one rule and start another could smuggle extra rules in. Now, before a name: value pair is glued in, the pair is checked: the name has to look like a property name, and the value must not contain the characters that end a declaration or a rule. Anything that fails is thrown away with a console warning rather than written out. The responsibility sits in one small helper the assembly sites call, so the stylesheet only ever contains declarations somebody meant to emit — except that only six of the assembly sites call it.
Both decisions trace to the stated problem and nothing is piggybacked. Decision 2 has no sentence in the body saying why silence beats a hard failure; it is a defensible default for a guard, so it is not a finding, but it is unrecorded and it is what makes the false-positive case below invisible.
The unit is right and the placement is wrong at the radius that matters. joinDeclarations is exactly the shape this wants — one pure function, no public surface, cheap to extend — but the file has more assembly sites than it is called from. Driven, all output real from the generator running in the review worktree: the token block, the component base rule, the component pseudo rule and all three of their on-media equivalents are guarded, with the value dropped and warned; the prose element rules are not, and the token value is emitted raw.
Counted: 6 sites route through the helper; 11 val() interpolations in generateProseRules (:545-591) do not. The mechanism is identical and the input is the same input — val at :384 returns the theme's own token string, and generateProseRules(val, parts) at :400 drops it straight into a template literal at :564. Run with a type-scale token carrying a payload, the prose block emits it verbatim: :where(p) closes on the value's } and body { … } becomes a rule of its own, while the same theme object has its token block correctly dropped and warned in the same call. The guard fires and the escape still ships, in a different block of the same stylesheet.
The token used is not contrived. It typechecks against the public API — an ordinary member of the token union — and it plus the ten sibling type-scale tokens are exactly what a white-label config sets. Two weaker instances of the same shape sit on the selector side, also driven: the theme name reaches the @scope prelude and a component key reaches the class selector unescaped. Both need control of the theme's shape rather than one value, so they are the same parent rather than separate asks, and they are why the ask is "put the check where the CSS is emitted" rather than "add a seventh call site".
The next place the same failure occurs is 164 lines below the fix, in the same file, reached by the same input type — not an edge left over, but the original defect, unfixed. Where an unevadable fix lives is at the point a rule is emitted: one emitRule(selector, entries) every generator path goes through, so a new rule site cannot be added without the check. The cheap version that makes the changeset true today uses the fallback val already has, returning var(<key>) when the value fails the predicate.
End users of any app on a theme in this repo: nothing. No shipped theme renders differently, measured byte for byte.
Builders — two of them, and they get opposite things.
- The builder the PR is for, piping tenant input into
defineTheme, gets six of seven assembly paths closed and a changeset telling them the job is done. They stop looking. That is the impact that decides this review: an incorrect safety claim is worse than no claim, because it retires the question. Today nobody believes the generator is safe; after this lands, the changelog says it is, and the prose path is still open. - The theme author who writes a value containing
;— adata:URI is the realistic one — loses that declaration. Their brand mark stops rendering and only a console warning says why.
What landing it newly exposes: nothing in this repo. But it moves the prose gap from "one of many holes in an unguarded generator" to "the one hole in a generator advertised as guarded", which is a worse place for it to sit.
No API change. Nothing added to the barrel, no prop, no export, no type, no default moved; the helper and its two regex constants are module-local, confirmed against the barrel.
No new theme targets, no target renamed, no target's element moved, no token added or removed. The diff writes no CSS of its own — it validates CSS the theme wrote — and the style grep hits only the new docblock's prose and the pre-existing generator, with nothing added as styling. The one theming-shaped consequence is under Breaking: a theme whose value fails the new predicate loses that declaration. Zero across every theme in this repo, and unmeasurable outside it.
joinDeclarations is a new internal helper beside a generator that had none. Is there a sibling already doing this job? A grep for escaping and sanitising across core and the CLI returns 10 files, and the only structural one guards filesystem paths rather than CSS — no existing mechanism reaches this, so a new helper is right and the shape is right. Nothing is frozen either way: it can be renamed or absorbed into an emitter tomorrow at zero cost.
@astryxdesign/core is published and not private, and that governs every axis below — "how many themes do this?" is a question about a population outside this repo, so each answer says which population it is answering for.
- API — no, for everyone. Nothing exported changed signature or meaning; the three new identifiers are module-local.
-
Visual — no for any theme in this repo; yes for a consumer theme whose value contains a
;. Both halves measured. In-repo:generateThemeCSS()for all seven shipped themes at the PR head, then the generator reverted toorigin/mainin the same worktree, same install, same session, and regenerated — 130,285 bytes both times, 0 declarations dropped, and the text diff identical. The PR's byte-identical claim holds for everything that ships here, and this is also the honest answer for a new enforcement: it catches zero violations on this repo's themes today, and the only hits producible were constructed. A rule that fires zero times is normally unfalsifiable; here it is the correct result, because it guards future input rather than migrating existing themes. Consumer half: adata:URI carries;in its mime prefix, so a theme using one loses the declaration, driven end to end in both builds with a frame pair. -
Theme — no in this repo. Every existing target, token and override resolves identically; the byte diff is the proof. Outside it, the
data:case is the exception. -
Behaviour — a theme with zero tokens is unchanged, a passing value is emitted byte-identically, and a failing value or property name reaches a new state: the declaration is dropped, a warning is logged, and the rule may now be empty —
.astryx-button.primary { }, driven, that exact string. Every other state is unreachable, since the diff adds no prop, no default and no early return in a component. The new state is the only behaviour change, and it is the intended one.
The far side of the bound is a legitimate value that happens to contain a listed character. Counted: 0 of the 7 shipped themes carry a value containing ;, and 0 of the 8 data:image hits in this repo are in a theme — all 8 are CLI block templates and unit tests. Not counted: themes in consumers' own apps, a population that exists and cannot be measured from here. Not accepted on anyone's behalf — it is raised in the ask, marked non-blocking, so the author decides.
Zero effects: no useEffect, no useLayoutEffect, no hook, no component — a pure module-scope function, and nothing renders. No listeners or observers added; no getComputedStyle, offsetWidth or getBoundingClientRect anywhere in the diff, so nothing reads layout and nothing can force a reflow; and the diff changes no hook's return value, so nothing re-memoizes.
The cost of the check itself, measured rather than asserted: two regexes per declaration, run once at theme-CSS generation rather than per render or per component — 100,000 checks in 24.2 ms, which is 0.484 ms per 2,000-declaration theme. The worst realistic case in the repo is well under that: all seven shipped themes together generate 130,285 bytes, so the figure overstates the real cost, and it is paid once, off the render path. The bundle gains three module-local identifiers, two regex literals and one loop — tens of bytes, no new dependency.
Two frames, captured in real Chromium and opened with the read tool, plus a text comparison of the generated CSS. The frame pair shows a themed data: URI background: the pink brand mark renders behind the button on main and is absent at this head, with the computed background image going from the data URI to none. The text comparison covers all seven shipped themes, main against head, in the same worktree, install and session: identical, 130,285 bytes each. The Playwright probe was banked in the review kit, reusable for any "does this theme value survive generation?" claim, since the generator's output is CSS text and there is no Storybook story to drive.
| difference | verdict | source |
|---|---|---|
the brand mark renders on main and is absent at this head |
unintentional | the body says "No serialization change for anything legitimate: gradients, calc(), light-dark(), color-mix(), quoted font stacks, and every shipped theme emit byte-identical CSS." A data: URI is legitimate and is not on that list. Nothing in the PR asks for this difference |
| every shipped theme's generated CSS | no difference | byte-identical, both builds |
No rationale is being supplied that the PR did not offer: the body claims legitimate values are unaffected, and this is a legitimate value that is affected. That gap is the finding. Whoever posts publishes the frames to the assets branch on the fork, never to facebook/astryx, and embeds them beside the third finding, because the comment makes a visual claim and that claim owes a frame.
Structural only — this diff renders nothing, adds no element, no role, no ARIA, no user-facing string. Both greps return 0.
Nothing in the rendered-surface set is reachable: no interactive element, no focus, no announcement, no painted state, and CI's a11y job skips on this PR, correctly, because it touches no component.
One line is reachable and was checked: the new console.warn string is not routed through the translator hook, and that is correct rather than a violation. The rule governs user-visible and AT-visible strings, and a developer console warning is neither — the repo has 7 non-test console.warn call sites in core, including one already in this same file at :358, none translated, and the dev-warning hook that does exist is a component hook that does not apply to a pure generator. No finding. Direction: no logical or physical properties written, nothing to mirror.
request changes. The goal is partly met: six of seven assembly paths are genuinely closed, driven — the token block drops the payload and warns — and the seventh is not, driven — the same payload is emitted raw into the prose rules in the same call. The uncovered member is the 11 interpolations in generateProseRules, and it cannot follow separately, because the changeset ships the guarantee now: a follow-up closes the code gap a week later while the changelog has said for a week that it is already closed.
1. generateProseRules interpolates 11 theme token values straight into
rule text; joinDeclarations never reaches them
→ an app that feeds stored input into a type-scale token still gets a
rule that escapes :where(p) and paints the page, while the changeset
tells them the generator no longer allows it — so they stop checking
· generateThemeRules.ts:400, :564 (val at :384)
2. a token name used in the new test is not a member of the token union
→ the build is red; `test` passes because vitest does not typecheck,
so the PR's own test-plan numbers cannot see it
· generateThemeRules.test.ts:749
3. [not blocking] a data: URI value contains ';' and is dropped
→ a theme author's brand mark stops rendering and only a console line
says why; 0 themes in this repo do it, consumer themes unknown
· generateThemeRules.ts:341
The code gap and the false guarantee are the same defect from two ends, and each carries its own weight, so neither depends on the other to reach the verdict. Three notes do compound and they resolve to one thing — failing silently on a value we do not like — describing a theme author whose declaration disappears with only a console line. That is survivable, it is in the ask in its own words, and it does not add up to a block: the harm is a lost background image, not a lost exit.
The worst outcome is the prose escape itself: a stylesheet that still extends beyond its declarations, from a value that arrived through the typed public API, on a PR whose changeset says that cannot happen. That cannot coexist with approve. No design call is involved — this is a defect against the PR's own stated contract, verified by running it.
Each block was confirmed a second way. The first was found by reading the diff's six call sites against the file's assembly sites, and confirmed by running the generator with a typed token; the test is not that the six converted sites still work, but that the unconverted path emits the payload, and it does. The second was found in CI's log and confirmed by reproducing locally: one type error, at that line. The diff was grepped for both fixes and touches neither, so the author has not already done this. The ask costs no extra round trip: CI is already red — the storybook build fails on the type error while the test job passes, because vitest does not typecheck — so the PR was being re-pushed either way.
Two things found and not spent. The selector-side instances ride the "at the emitter" ask rather than being listed separately. And the author has nine other open PRs, all security hardening, several theme-adjacent, none touching this file — so there is no set to review together, and three of the six commits that landed on main during this run are his own PRs landing cleanly, which is worth saying because the block here is about this diff and not about the contributor.
Not verified: how many published consumers set a theme value containing ;, which is why the third finding is raised rather than accepted; whether any real consumer assembles a theme from stored input at all, which the PR asserts and this repo cannot demonstrate — the blocking finding does not depend on it either way; and whether the storybook build fails for a second reason after the type error, since the typechecker stops there.
Thanks for this — the hard part is picking which of the generator's many concatenation sites to guard, and six of them are right.
The prose rules still interpolate token values straight in, though, so the changeset's guarantee isn't true yet:
defineTheme({name: 'brand', tokens: {'--text-body-size': '1rem; } body { background: url(x) '}})The token block drops that and warns;
:where(p)emits it and the rule closes early.--text-body-sizeis an ordinary typed token, so a white-label config feeding the type scale gets there. Elevenval()calls have the same shape.
--color-heroisn't aTokenNameeither — that's the red build.Not blocking: a
data:URI value carries a;, so a themedbackground-imagegets dropped too.Can you route
valthrough the check as well, or is the prose block different for a reason I'm missing? If you'd rather talk it through with someone, we're in Discord.
Inline: packages/core/src/theme/generateThemeRules.ts:564 — Token value goes in raw here. joinDeclarations doesn't reach the prose block. · :384 — val hands back the theme's raw token string. Could route through the same check — the var() fallback is already here. · generateThemeRules.test.ts:749 — --color-hero isn't a TokenName. This is what's failing the build.
One review, three gate passes.
-
Gate 1 — failed on two, both in the private record while the comment was already clean. The impact slot's body described builders shipping a live escape because the changelog told them it was closed, and its own verdict line called that a note; the outcome only survived because another slot happened to carry the weight. And the
data:URI finding was accepted on a class count taken inside the repo, while the same draft had established two paragraphs above that the package is published and its consumers are outside it — the count was measured against the wrong population. - Gate 2 — two more. The blocking and non-blocking findings were flattened into one "two smaller things" clause in the comment, so the author could not tell which was required. And the visual breaking axis was still answered against the in-repo population while the evidence slot admitted one case renders differently — the same scoping error the draft had just fixed one paragraph up, and the reason the frames now exist at all.
- Gate 3 — clean. All four prior fixes confirmed in substance, with both frames opened by the critic and confirmed to show what the slot claims. One practice was adopted mid-run rather than argued: every verdict line now carries its own harm sentence, because round 1's mislabelled slot would not have survived being written next to the sentence its own body contained.
Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inlines, and the frames are unpublished. The presentation's merge trailer is a hold whose content is the first finding — the stated guarantee is not true for the prose path — with the red build beside it. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5529) line goes in before the attribution.
Two brief changes this run earned and did not write, because it was read-only: a harm sentence on every verdict line, and a check that reads the posted comment beside the numbered findings item for item, so a blocking finding cannot arrive in the comment looking optional.