mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
Zachary Lowden
36e28b7792
* fix(test): give the component harness the app's :root custom properties, so components stop laying out against undefined vars The component-test harness loads no app stylesheet, so every var(--header-height) in a component under test resolved to "". That is not a missing nicety: an unresolvable var() makes the whole declaration invalid at computed-value time, so calc(100dvh - var(--header-height)) silently computed to 0px and the component laid out differently under test than in production. 33 TS/TSX files and 7 stylesheets read that property today, so the divergence was repo-wide and nothing reported it -- a layout assertion could be green against geometry no user sees. Measured end to end in real Chromium, innerHeight 896: without the injection: --header-height = "" calc(...) = "0px" with it: --header-height = "60px" calc(...) = "836px" EXTRACTED, NOT IMPORTED. Importing globals.css pulls the real cascade -- Tailwind preflight, @layer ordering, element defaults -- which changes the rendered geometry of every existing test (Mantine Group starts centring its items, so same-line assertions written against `top` begin failing against correct code; that hazard is already documented in the harvest-component-tests skill). Taking only the :root custom properties gives components the values they read and leaves the cascade exactly as the suite has always had it. The A/B says it worked: 189 files / 2078 tests before, 190 / 2079 after -- exactly the one file and one test this adds, zero regressions. PARSED, NOT RESTATED. Hardcoding --header-height: 60px in the setup would be a fourth copy of the constant civitai#4379 existed to consolidate, and it would drift the first time the header is resized. The values come from globals.css itself. The extraction throws loudly if the :root block stops matching or declares nothing, because a silent revert here puts every component test back to laying out against undefined properties with nothing to say so. src/components/AppLayout/rootCustomProperties.browser.test.tsx is the guard, and it is the only thing that proves the injection reaches the document rather than merely that the parse succeeded. Its expected value is derived from window.innerHeight and from the property it just read, never hardcoded, so it cannot pass by coincidence on a differently-sized runner. Negative control run: with the setup reverted it fails with v: "" / computed: "0px". Retires the prohibition #4379 had to add. Both the constant's comment and PageBlockHost's now record that the var is safe again and that keeping the interpolation is a deliberate non-change -- switching would leave AppHeader as the constant's only consumer and need the CSS/TS binding guard re-pointed, which is a refactor with its own risk and no user-visible benefit. `*.css?raw` needed an ambient declaration; added beside the CSS-modules ones rather than pulling in vite/client, which would add unrelated globals to the Next app's program. Verified: full unit project 22164 passed / 0 failed; full component project 190 files / 2079 tests, 0 failed; typecheck 0; eslint --max-warnings=0; prettier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test): close the two silent-divergence paths the audit found in the :root extraction Both reintroduced the exact failure this PR exists to remove -- a component harness laying out against custom properties that are wrong or missing, with the whole suite green -- so they land here rather than as a follow-up. WRONG-BLOCK CAPTURE, AND A GUARD THAT COULD NOT SEE IT. The extraction takes the FIRST `:root` match anywhere in globals.css, so a second one earlier in the file (inside an @media, say) was injected UNCONDITIONALLY while the real block's other properties were dropped entirely. The guard could not catch it because its expectation was derived from the property it had just read -- it graded the injection against itself. That is the self-referential-assertion trap, and it is the second time this arc has produced one. Two fixes, because they close different halves: - the setup now refuses more than one `:root {` outright, rather than guessing which was meant. Measured: with a conditional :root added above the real one, the setup THROWS before any test runs (loud, 0 executed). - the guard now pins the injected value to HEADER_HEIGHT_PX. Not circular: pageRunScrollContract.test.ts (GATING tier) already binds that constant to globals.css independently, so this compares the injected value against a number a different guard ties to the stylesheet. Measured: changing the CSS value alone now fails the guard, where before it passed. A CSS COMMENT INSIDE :root SILENTLY DROPPED THE DECLARATION AFTER IT. split(';') leaves the comment glued to the next declaration, which then fails startsWith('--') and vanishes -- and no count reached zero, so nothing threw. Measured on the real file: adding `/* keep in sync with AppFooter */` above --footer-height parsed 3 properties down to 2, with all 190 files green. --footer-height is read inside calc() by the auctions page and CollectionsLayout, so documenting a variable -- the most ordinary edit imaginable -- silently reintroduced the divergence. Comments are now stripped before splitting, and the parse must account for every property the block declares. Positive control for that count assertion, so it is not inert: a nested rule inside :root (legal CSS, and the `[^}]*` capture truncates at its brace) gives parsed=3 vs declared=4 and throws. Also restores the tier caveat the previous commit deleted: this guard lives in preview/component-tests, which is REPORT-ONLY -- it tells you, it does not stop you. The CSS-to-TS binding it leans on is in the gating tier. And widens the calc assertion from an exact px string to a 1px tolerance. 100dvh resolves to the layout viewport, which is fractional on a non-integer device-scale factor, so an exact match would flake on a differently-configured runner and read as a real break. The failure it must catch is 0px, not a rounding difference. Sweep: wrong-block KILLED (setup throws), value-diverges KILLED, injection removed KILLED, --header-height filtered out KILLED, a NEW property added to :root stays GREEN, baseline and post-restore GREEN. Full component project 190 files / 2079 tests, 0 failed; typecheck 0; prettier; eslint clean on the changed files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test): strip CSS comments from the WHOLE file first, and derive the coverage check from the file rather than the captured block The delta re-audit found the previous round shipped a guard that could not detect the defect it was written for, one that graded a truncated string against itself, and a new false-throw class with a 2079-test blast radius. All three had one root cause and one edit. THE COUNT ASSERTION WAS INERT AGAINST ITS OWN DEFECT. A comment sitting between `;` and the next `--name` removes a member from BOTH sides symmetrically: it kills the split(';') fragment AND destroys the `[;{]\s*--` anchor the declared-count regex needs. Counts stay equal, nothing throws, the property is silently gone. Measured in the real harness with the comment-strip disabled: setup did not throw, the guard stayed green, --footer-height resolved to "". AND IT WAS SELF-REFERENTIAL. `declared` was computed from the same captured substring the parse used, so it structurally could not see a truncation of the block itself — a `}` inside a comment inside :root (comments were stripped AFTER extraction, so they still truncated it) dropped --buzz-color with counts equal. That is the same trap this PR's previous round removed from the browser test, reintroduced one file over. THE ONE-`:root` REFUSAL WAS ORDER-BLIND AND RAN ON RAW TEXT. Its own comment named the hazard as a second block EARLIER in the file; the code counted every `:root {` anywhere, comments included. So an ordinary dark-mode override AFTER the base block, a commented-out block, or prose merely mentioning ":root {" each threw and took all 190 files to "Tests no tests" — in a report-only tier. globals.css already contains commented-out CSS after :root, so that was live. ONE EDIT FIXES ALL THREE, and it is simpler than what it replaces: - strip comments ONCE, from the whole file, before anything looks at it; - drop the >1-`:root` refusal entirely; - check COVERAGE BY NAME against every `:root` block in the file — every custom property declared anywhere on :root must survive extraction. That needs no brace-counting and no block-ordering logic, and it is strictly stronger: a conditional first block is caught because the real block's other properties go missing; a truncated capture is caught the same way; a legitimate later override declares the same NAMES, so it is not a false failure. Verified, each case the audit named: comment glued to the next property -> property SURVIVES (probe asserts it) comment containing `{ }` before a property -> SURVIVES dark-mode :root override after the base -> GREEN (was a false throw) commented-out :root block after -> GREEN (was a false throw) prose mentioning ':root {' after -> GREEN (was a false throw) conditional :root FIRST -> setup THROWS, loudly --header-height removed from :root -> guard FAILS css value != HEADER_HEIGHT_PX -> guard FAILS a NEW property added to :root -> GREEN baseline / post-restore -> GREEN Also corrects the non-circularity claim on the value pin, which was overstated: the ledger binds CSS<->TS and this binds INJECTED<->TS, so together they bind INJECTED<->CSS. That is a composition, not two independent paths, and given a green ledger the pin is close to tautological — its residual live coverage is --header-height's own value being mangled inside the block. Worth keeping, and worth describing accurately. Full component project 190 files / 2079 tests, 0 failed (run alone); gating pageRunScrollContract ledger 10/10; typecheck 0; prettier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(test): let the BROWSER parse globals.css instead of three regexes that cannot agree where a CSS block ends Four audit rounds, four defects, one cause. Each round fixed what it aimed at and shipped the next one, and every one came from asking a regex to decide the extent of a CSS block: round 1 a comment glued to the next property dropped it, silently round 2 the count check was inert against exactly that, and graded a truncated string against itself round 3 "refuse >1 :root" blacked out all 2079 tests on an ordinary dark-mode override; and `[^{]*` attributed an unrelated rule's properties to :root because the string ":root" appeared in a content: value round 4 a `}` or `;` inside a STRING value still truncated the capture -- --footer-height and --buzz-color silently undefined across all 190 component files, verified in Chromium, everything green The regexes are gone. `CSSStyleSheet.replaceSync` hands the whole problem to the engine that will actually evaluate these properties, so comments, strings, nesting and at-rules stop being our concern. The distinction that mattered most falls out of the rule types for free: a :root inside @media/@supports/@container is a CSSMediaRule and is skipped on purpose -- such a property is undefined outside its condition, so injecting it unconditionally would be a lie -- while @layer only orders the cascade and is descended into. A grouped selector (`:root, html`) is matched on its parts. The guard now sweeps EVERY injected property rather than the one it is named after. That is what round 4 found: it asserted only --header-height while two other properties were undefined everywhere, and --footer-height is read inside calc() by 8 files, --buzz-color by 20+ SCSS modules. Checking only the property you happened to think of is how a guard reads as coverage while providing almost none. It also asserts the injected list is non-empty first, so the sweep cannot pass vacuously. Full case matrix -- all 16 correct, first clean run of it: comment glued to a property property SURVIVES comment containing `{ }` SURVIVES `}` inside a string value SURVIVES (round-4 defect) `;` inside a string value SURVIVES dark-mode override ADDING a variable GREEN (was a blackout) `content: ":root"` + a later rule GREEN (was a blackout) @supports selector(:root) GREEN (was a blackout) :root.theme-dark GREEN (was a blackout) unterminated /* inside a string GREEN (was a blackout) commented-out :root block GREEN ALL properties moved into @media setup THROWS, loudly css value != HEADER_HEIGHT_PX guard FAILS a NEW property added to :root GREEN grouped selector `:root, html {` GREEN baseline / post-restore GREEN Checked green-for-the-right-reason, not just green: under the `}`-in-a-string case the extracted names are exactly --buzz-color, --footer-height, --header-height, --quote. The regex version dropped two of those. Full component project 190 files / 2080 tests, 0 failed (run alone); gating pageRunScrollContract ledger 10/10; typecheck 0; eslint; prettier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>