CORE-2731: Generate a global CSS token file from the theme - #3133
Open
OpenStaxClaude wants to merge 12 commits into
Open
CORE-2731: Generate a global CSS token file from the theme#3133OpenStaxClaude wants to merge 12 commits into
OpenStaxClaude wants to merge 12 commits into
Conversation
Splits the pure theme data out of theme.ts into themeData.ts (no styled-components import, so a build-time generator can read it), projects it to CSS custom properties in themeCss.ts, and generates src/app/theme.css from that. Replaces the 21-token :root block that was hand-copied into index.css. Link colours and mainContentBackground were separate sources of truth; they now come from the theme, so #027EB5 and its 17 lowercase CSS copies cannot diverge. theme.spec.ts enforces it: the committed theme.css must equal the generator's output, no stylesheet may read a --color-*/--z-index-*/--padding-* token that does not exist, and no breakpoint may sit within 1em of a theme breakpoint without being one. The two colour checks are locked to a committed baseline (184 duplicated literals, 37 unrecognised) so new violations fail CI now while the sweep works through the existing ones. The audit is shared with the baseline generator so they cannot drift, and has 46 tests of its own covering hex/rgba/hsl/named-colour syntaxes, gradient stops, comments and selectors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Generator shortens hex to the 3-digit form where equivalent, so theme.css satisfies stylelint's color-hex-length as well as color-hex-case. - flatten()'s default prefix was an unreachable branch; jest enforces 100% branch coverage on src/app. - BookBanner snapshots: the two static z-index bindings are gone. The diff is style-attribute-only, and the removed values (69, 68) are what the new calc(var(--z-index-navbar) - 1 / - 2) resolves to. - PLAIN_CSS_MIGRATION_GUIDE.md: Pattern 2.5 described the index.css :root block as hand-copied and needing manual syncing, which is what this ticket removes. Rewritten around the generated file, plus what CI now enforces, the baseline ratchet and the breakpoint gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot's review found four real defects in the audit and two documentation
claims that were not true. All six are fixed here; no rendered output changes.
cssColors.ts:
- Percentage channels scaled by the decimal 2.55, so `rgb(50%, 50%, 50%)` keyed
as 127,127,127 while `#808080` keyed as 128,128,128 and the two spellings of
one grey could never match. Now `(value / 100) * 255`.
- fromHex validated the expanded length and not the digits, so `#ggg` came back
as an Rgba of NaNs rather than null -- and a malformed *theme* value would
then have passed the "every colour token resolves" spec while generating
invalid CSS. The hex grammar is checked before parsing.
- The declaration walk discarded the property name, so any identifier that
happens to be a named colour was read as one: `animation-name: red` and
`font-family: black` were palette violations. It now returns
{context, property, value}, and a bare identifier is only a colour where the
property can hold one. Hex and the colour functions are unambiguous and stay
in scope everywhere.
- Baseline entries were file + literal, so removing one `#fff` and adding
another elsewhere in the same file left the sorted array unchanged and the
ratchet passed. An occurrence is now identified by the declaration it was
written in -- selectors, at-rules and property. Declaration context rather
than line/column on purpose: a line number churns the baseline whenever an
unrelated rule is inserted above one, and a baseline regenerated for an
unrelated reason is where a new colour would hide.
theme.spec.ts: the breakpoint proximity bound was `< 1`, so 74em -- the example
in its own comment and in the migration guide -- was exactly on the boundary and
not caught. Now `<= 1`; no width currently in use lands there.
themeData.ts said every value is projected into a token, but the breakpoints
deliberately are not, since a media query cannot read a custom property. index.css
pointed at a README "Styling" section that does not exist; it points at
PLAIN_CSS_MIGRATION_GUIDE.md Pattern 2.5 now.
The baseline is regenerated for the new entry format. The counts are unchanged
at 184 duplicates and 37 unrecognised, including all 14 bare `white` keywords,
so the named-colour narrowing lost no real coverage. cssColors.spec.ts grows
from 46 tests to 80, covering each of the above in both directions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
The three defects named on ui-components#143 -- hex grammar, the named-colour property gate and the 2.55 percentage scaling -- were already fixed here in 6bfa139, and each is pinned by a test that fails when reverted. That note was written from #143's state rather than this branch's; nothing in cssColors.ts changes here. Diffing the two checkers properly did turn up one real gap, in the tests: findColors recurses into var(), color-mix() and the gradients, threading the `named` flag down to itself. Nothing asserted that it does. Passing `true` instead of `named` in the recursive call left the whole suite green, so the property gate was only actually guarded at the top level of a declaration value. Covered now from both sides -- `animation-name: var(--enter, red)` finds nothing, `background: linear-gradient(to top, red, transparent)` still finds red -- and the first was confirmed load-bearing by leaking the flag and watching it fail alone. Also brought across three cases #143 has and this did not: a named colour in box-shadow, a named colour in a vendor-prefixed property, and the #1234567 / #123456789 malformed hex lengths. The colour-bearing property lists still differ between the two repos and are deliberately left alone -- this one enumerates the eleven border shorthands that can hold a colour where #143 uses startsWith('border'), and additionally covers mask-image, border-image and text-stroke. Converging them is CORE-2736. 80 -> 86 cases in cssColors.spec.ts. theme.baseline.json regenerates byte-identical at 184/37. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
stripNoise blanked string contents before `declarations` recorded `context`, so two rules differing only inside a selector string reduced to one identity. That was live in the committed baseline -- five entries carried `input[type=""]` or `[data-active=""]` -- and reachable, not just latent: the tree already contains [data-loading="true"]/[data-loading="false"], [data-fading-in="true"]/[data-fading-in="false"], [data-type="page"]/[data-type="equation"]/[data-type="composite-page"] and [data-testid="chapter-title"]/[data-testid="section-title"]. Moving a literal between either half of any of those left the sorted baseline unchanged, which is precisely the hole the declaration identity exists to close. A string is noise in a declaration value -- `content: "#fff"` is not a colour -- and meaning in a selector, so one blanked copy cannot serve both. stripNoise now blanks to spaces rather than deleting, making its output the same length as its input, and `declarations` walks two aligned copies: structure and values from the copy with strings blanked, `context` sliced from the copy that keeps them. url() parentheses are kept either way, since the walk balances them to know a `;` inside url() is not a separator. Three cases pin it, each confirmed by making the specific mistake and watching only it fail: context from the blanked copy, values from the preserving copy, and a brace inside a selector string opening a block. The length invariant is covered separately over unterminated strings, url() and comments -- it is load-bearing now, because a length change misaligns context against structure and would surface only as a wrong selector in a baseline entry. Also: the mainContentBackground comment claimed the value is referenced from CSS as var(--color-neutral-base). It is not -- ContentPane binds it as --main-content-background and ContentPane.css reads that -- so the comment would have sent the sweep looking for a reference that does not exist. Baseline regenerates at 184/37, unchanged; the five entries gain their real selectors. cssColors.spec.ts 86 -> 96 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RoyEJohnson
force-pushed
the
CORE-2731-global-css-theme-tokens
branch
from
September 1, 2026 21:41
8ced57e to
8a0c631
Compare
RoyEJohnson
force-pushed
the
CORE-2731-global-css-theme-tokens
branch
from
September 1, 2026 21:58
8a0c631 to
6703f43
Compare
stylesheetColors built its result by spreading into a new array per declaration, which is quadratic in the declarations of a single stylesheet. Replaced with push, along with the same shape in the three other places on the whole-tree scan path: themeColorIndex's object spread per token, the stylesheetFiles directory walk, and the two per-file reduces in theme.spec.ts. Measured over the tree it audits -- 102 files, 3193 declarations -- this is not a speedup: 26.5ms median before, 26.1ms after. The quadratic term is bounded per file rather than across the tree, because colorViolations already accumulated with push and stylesheetColors runs once per stylesheet; the largest file has 210 declarations. Worth doing anyway since it is strictly less work for the same number of lines, and the bound only holds until someone writes a bigger stylesheet, but the comment on stylesheetColors says why rather than implying a win that is not there. theme.baseline.json regenerates byte-identical at 184/37, which is what makes this a pure refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces new build-time generation plus repo-wide stylesheet enforcement logic that can have broad CI/runtime impact and merits final human verification before merge.
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
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 free
to 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.
Jira: CORE-2731 (sub-task of CORE-1685)
Follows ui-components#143 (CORE-2720), which did the same thing there.
Why
The styled-components migration has been copying theme values into CSS by hand. Measured on
mainat f00f987:theme.ts—#42424234× across 26 files,#fff26× across 16,#d5d5d518×. (The audit added here finds 184, because it also catches barewhitekeywords and colors inside shorthands that a hex grep misses.):rootblock intosrc/index.css, referenced 63 times against those 154 literals. Colors only — no z-index or padding tokens — and nothing kept it in step withtheme.ts.Links.constants.tsdeclareslinkColor = '#027EB5'andlinkHover = '#0064A0'— the two most common "off-theme" colors in the CSS at 17 occurrences each, every CSS copy lowercase against the uppercase TS original.content/components/constants.tswas a third (mainContentBackground = '#fff').What this does
Splits the theme data out.
theme.tsimportscssfromstyled-components/macrofor thebreakpoints.mobile()helpers, so a build-time generator importing it would drag styled-components into the build. The pure data —color,padding,zIndex, the breakpoint sizes — moves tosrc/app/themeData.tswith no imports at all.theme.tsspreads it back into its default export, so everytheme.color.xpath and all ~200 consumers are untouched. This is a step the epic needs regardless;theme.tsis one of the 26 files still on styled-components.Generates the token file.
src/app/themeCss.tsowns the projection;src/app/theme.cssis generated from it, never hand-edited. 80 tokens:--color-*(kebab-case of the JS path, so all 21 existing names are preserved exactly),--z-index-*(the 15 computed keys),--padding-page-*. Hex is lowercased on the way out, so a token's value has one spelling andcolor-hex-caseis satisfied without touching the published JS values.script/generate-theme-css.tsruns vianode ./script/entry generate-theme-css— the repo's existing TS-script idiom, so unlike ui-components no tsc-into-a-temp-dir dance was needed. Wired intobuildandprestart. Deliberately not intobuild:css:pretestrunsbuild:css, so generating there would regenerate the file immediately before the suite and make the freshness check pass vacuously.Absorbs the other sources. Link colors live in
themeData.tsand are re-exported byLinks.constants.ts, so its 8 JS importers are unchanged but there is now one source and three tokens.mainContentBackgroundpoints at the theme's white.Enforces it.
src/app/theme.spec.ts:theme.cssequals the generator's output — one equality, so a missing token, an orphan token and a stale value all fail the same way;--color-*/--z-index-*/--padding-*token that does not exist;(5) found two real cases:
BookBanner.cssread--z-index-bigand--z-index-mini, component-local variables sitting in the global family's namespace. They are derived from--z-index-navbar, so CSS now computes them withcalc()and the two JS bindings are gone.(6) is the cheap mitigation for the breakpoint gap.
@media (min-width: var(--x))is not valid CSS, so75emstays duplicated 144 times. Banning component-specific breakpoints would be wrong — Footer legitimately uses 37.5em, 60.1em and 90em — so instead it catches the failure the duplication actually causes: a value meant to be a theme breakpoint but mistyped as74em, which silently stops matching.The baseline is a ratchet, not a skip. Checks (3) and (4) are locked to
src/app/theme.baseline.json— 184 duplicated literals and 37 unrecognised colors as of this commit. So enforcement starts now: a new hardcoded#424242fails CI today, and the list can only shrink as the sweep PRs land.yarn generate:theme-baselineregenerates it and prints the counts, which should only go down. This is why the sweep is not in this PR: it is ~50 files of values-only edits and wants reviewing against screenshots separately.Verification
src/test/cssColors.tsparses declarations rather than grepping for hex, sorgba(),hsl(), named colors in shorthands and colors in gradient stops are all covered. It descends intovar()/color-mix()/gradients, passes a translucent color whose opaque channels are a theme value (rgba(0, 0, 0, 0.2)is black at 20% and has no token form) while still refusing a new hue throughrgba(), and fails rather than passing silently on anything it cannot resolve to channels. 96 tests cover exactly this — without them "CI enforces the palette" would be an assertion rather than a tested guarantee. There is also a guard that the file walk found something, so the suite cannot pass vacuously.palette.blackwithout regenerating fails the spec; regenerating passes it.calc()z-indexes, which resolve to the same 69 and 68 as before.Status
Ready for review. CI green. The sweep follows in separate PRs under the same ticket.
Not yet done: the screenshot suite. No visual change is intended and the only rendered-output change is BookBanner's two
calc()z-indexes, which I have now verified in Chrome resolve to the same 69 and 68 as the bindings they replaced — but that is reasoning plus a unit check, not a pixel diff, so the suite is still worth running before merge.Review rounds
Copilot found nine issues across four rounds; all are fixed. The ones worth knowing about:
rgb(50%, 50%, 50%)scaled by the decimal2.55keys as127,127,127while#808080keys as128,128,128, so the two spellings of one grey never matched.#gggparsed as a color. Only the expanded length was checked, not the digits, so a malformed theme value would have passed the "every token resolves" guard while generating invalid CSS.animation-name: redandfont-family: blackwere palette violations. Bare identifiers are now gated on the property; hex and the color functions stay in scope everywhere.#fffand adding another elsewhere in the same file left the sorted baseline unchanged. They are now keyed by the declaration — selector, at-rule and property. Then a second hole in that fix:stripNoiseblanked string contents before the context was recorded, so[data-loading="true"]and[data-loading="false"]collapsed to one identity. Blanking is now length-preserving and context is sliced from a copy that keeps strings.🤖 Generated with Claude Code