From cdb2ee3a74b366499c8095de6d8df95bbe4ae5fa Mon Sep 17 00:00:00 2001 From: Tomas Madariaga Date: Fri, 18 Sep 2026 13:42:34 +0200 Subject: [PATCH 1/4] fix(packages/sui-critical-css): preserve at-rule context when rebuilding covered CSS Chrome's CSS coverage reports ranges that cover the style rule only, never the `@media`, `@layer`, `@supports` or `@container` block it lives in. Concatenating the raw byte slices therefore produced completely flat CSS: measured on a production page, 0 `@layer` and 0 `@media` in 51841 chars of critical CSS. Two consequences. Nothing is layered any more, so ties that the layer order used to decide fall back to source order and a `@layer base` reset extracted after a `@layer utilities` rule of the same specificity silently starts winning. And media queries are flattened, so rules extracted at the 360px extraction viewport apply at every width. Parse the stylesheet with postcss instead of slicing bytes, and keep every declaration-holding node whose source range intersects a covered range together with its whole chain of ancestor at-rules. `@layer a, b, c;` statements are never covered (they are not style rules) yet they are the only thing that fixes the layer order, so they are kept unconditionally and hoisted to the top: coverage entries are not guaranteed to be in document order. A stylesheet postcss cannot parse falls back to the old behaviour and logs why. Adds the package's first test suite. Co-Authored-By: Claude Opus 5 --- packages/sui-critical-css/package.json | 3 +- packages/sui-critical-css/src/covered-css.js | 93 +++++++++++++++ .../sui-critical-css/src/extract-from-url.js | 5 +- .../test/server/covered-cssSpec.js | 108 ++++++++++++++++++ 4 files changed, 205 insertions(+), 4 deletions(-) create mode 100644 packages/sui-critical-css/src/covered-css.js create mode 100644 packages/sui-critical-css/test/server/covered-cssSpec.js diff --git a/packages/sui-critical-css/package.json b/packages/sui-critical-css/package.json index dd27e8010..9409384e8 100644 --- a/packages/sui-critical-css/package.json +++ b/packages/sui-critical-css/package.json @@ -5,13 +5,14 @@ "type": "module", "main": "src/index.js", "scripts": { - "test": "echo \"Error: no test specified\" && exit 1" + "test": "sui-test server" }, "dependencies": { "clean-css": "5.3.3", "node-fetch": "2.7.0", "path-to-regexp": "0.1.10", "playwright-chromium": "1.25.0", + "postcss": "8.4.31", "@s-ui/critical-css-middleware": "1" }, "keywords": [], diff --git a/packages/sui-critical-css/src/covered-css.js b/packages/sui-critical-css/src/covered-css.js new file mode 100644 index 000000000..489b80b06 --- /dev/null +++ b/packages/sui-critical-css/src/covered-css.js @@ -0,0 +1,93 @@ +import postcss from 'postcss' + +// Chrome's CSS coverage reports ranges that cover the style rule ONLY. The `@media`, +// `@layer`, `@supports` or `@container` block the rule lives in is never part of the +// range, so slicing the raw bytes out of `entry.text` and concatenating them produces +// CSS that has lost every at-rule wrapper. Two things break as a result: +// +// 1. Nothing is layered any more, so ties that the layer order used to decide now +// fall back to source order. A `@layer base` reset extracted after a +// `@layer utilities` rule of the same specificity silently starts winning. +// 2. Media queries are flattened, so rules only meant for the extraction viewport +// apply at every width. +// +// So instead of slicing bytes, parse the stylesheet and rebuild it: keep every +// declaration-holding node whose source range intersects a covered range, together +// with its whole chain of ancestor at-rules. +const KEEP_AT_RULE = 'layer' + +const holdsDeclarations = node => + node.type === 'rule' || (node.type === 'atrule' && node.nodes?.some(child => child.type === 'decl')) + +// `@layer a, b, c;` is a statement, not a style rule, so coverage never marks it as +// used — yet it is the only thing that fixes the order of the layers. Always keep it. +const isLayerStatement = node => + node.type === 'atrule' && node.nodes === undefined && node.name.toLowerCase() === KEEP_AT_RULE + +const intersectsCoveredRange = (node, ranges) => { + const start = node.source?.start?.offset + const end = node.source?.end?.offset + + if (start === undefined || end === undefined) return false + + return ranges.some(range => range.start < end && range.end > start) +} + +/** + * Rebuild the used part of a stylesheet from its Chrome CSS coverage ranges, keeping + * the at-rule context of every rule it keeps. + * + * @param {object} params + * @param {string} params.text Full source of the stylesheet, as reported by coverage. + * @param {Array<{start: number, end: number}>} params.ranges Covered byte ranges. + * @returns {string} CSS containing only the covered rules, still wrapped in their + * original at-rules, with every `@layer` statement hoisted to the top. + */ +export const rebuildCoveredCSS = ({text, ranges}) => { + const root = postcss.parse(text) + const keep = new Set() + const layerStatements = [] + + const keepWithAncestors = node => { + for (let current = node; current && current.type !== 'root'; current = current.parent) keep.add(current) + } + + root.walk(node => { + // postcss leaves the terminating `;` of a bodyless at-rule to the parent's + // stringifier, so it has to be added back when lifting the node out on its own. + if (isLayerStatement(node)) return layerStatements.push(`${node.toString()};`) + if (holdsDeclarations(node) && intersectsCoveredRange(node, ranges)) keepWithAncestors(node) + }) + + const discarded = [] + root.walk(node => { + if ((node.type === 'rule' || node.type === 'atrule') && keep.has(node) === false) discarded.push(node) + }) + discarded.forEach(node => node.remove()) + + // Coverage entries are not guaranteed to be in document order, so a `@layer` + // statement could otherwise end up after a rule that already registered the layers + // in the wrong order. Hoisting makes the result independent of entry order. + return [...layerStatements, root.toString()].filter(Boolean).join('\n') +} + +/** + * Same as `rebuildCoveredCSS`, but never throws: a stylesheet postcss cannot parse + * falls back to the old byte-slicing behaviour instead of failing the whole + * extraction. The fallback loses at-rule context, so it is logged loudly. + * + * @param {object} entry A single Chrome CSS coverage entry. + * @returns {string} + */ +export const rebuildCoveredCSSFromEntry = entry => { + try { + return rebuildCoveredCSS(entry) + } catch (error) { + console.warn( + `[ko] Could not parse ${entry.url ?? 'a stylesheet'} (${error.message}).`, + 'Falling back to raw coverage ranges, so its @media and @layer context is lost.' + ) + + return entry.ranges.reduce((css, range) => css + entry.text.slice(range.start, range.end), '') + } +} diff --git a/packages/sui-critical-css/src/extract-from-url.js b/packages/sui-critical-css/src/extract-from-url.js index 30c7c3e48..a348d5685 100644 --- a/packages/sui-critical-css/src/extract-from-url.js +++ b/packages/sui-critical-css/src/extract-from-url.js @@ -2,6 +2,7 @@ import CleanCSS from 'clean-css' import {chromium} from 'playwright-chromium' import {blockedResourceTypes, skippedResources} from './config.js' +import {rebuildCoveredCSSFromEntry} from './covered-css.js' let browser @@ -84,9 +85,7 @@ export async function extractCSSFromUrl({customHeaders, height, url, userAgent, let coveredCSS = '' for (const entry of coverage) { - for (const range of entry.ranges) { - coveredCSS += entry.text.slice(range.start, range.end) - } + coveredCSS += rebuildCoveredCSSFromEntry(entry) } // Close the browser to close the connection and free up resources diff --git a/packages/sui-critical-css/test/server/covered-cssSpec.js b/packages/sui-critical-css/test/server/covered-cssSpec.js new file mode 100644 index 000000000..05b9cc589 --- /dev/null +++ b/packages/sui-critical-css/test/server/covered-cssSpec.js @@ -0,0 +1,108 @@ +import {expect} from 'chai' + +import {rebuildCoveredCSS, rebuildCoveredCSSFromEntry} from '../../src/covered-css.js' + +// Chrome reports coverage as byte ranges into the stylesheet source, and the range of a +// used rule covers that rule ONLY — never the at-rule it is nested in. These helpers +// build such a range from a substring so the specs read like the real input. +const rangeOf = (text, snippet) => { + const start = text.indexOf(snippet) + + if (start === -1) throw new Error(`Fixture does not contain "${snippet}"`) + + return {start, end: start + snippet.length} +} + +const rebuild = (text, ...snippets) => + rebuildCoveredCSS({text, ranges: snippets.map(snippet => rangeOf(text, snippet))}) + +describe('@s-ui/critical-css covered-css', () => { + it('keeps the @media a covered rule lives in', () => { + const text = '@media (min-width:600px){.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps the @layer a covered rule lives in', () => { + const text = '@layer utilities{.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('keeps the whole chain of nested at-rules', () => { + const text = '@supports (display:grid){@media (min-width:600px){@layer base{.used{color:red}}}}' + + expect(rebuild(text, '.used{color:red}')).to.equal(text) + }) + + it('drops uncovered siblings but keeps the at-rule of the covered one', () => { + const text = '@media print{.used{color:red}.unused{color:blue}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('@media print{.used{color:red}}') + }) + + it('drops an at-rule with no covered rule at all', () => { + const text = '.used{color:red}@media print{.unused{color:blue}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}') + }) + + it('keeps unlayered covered rules as they are', () => { + const text = '.used{color:red}.unused{color:blue}' + + expect(rebuild(text, '.used{color:red}')).to.equal('.used{color:red}') + }) + + it('keeps a @layer statement even though coverage never marks it as used', () => { + const text = '@layer base,utilities;@layer utilities{.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('@layer base,utilities;\n@layer utilities{.used{color:red}}') + }) + + it('keeps a @layer statement even when nothing else is covered', () => { + expect(rebuildCoveredCSS({text: '@layer base,utilities;.unused{color:blue}', ranges: []})).to.equal( + '@layer base,utilities;' + ) + }) + + it('hoists @layer statements above the rules, whatever their original position', () => { + const text = '@layer utilities{.used{color:red}}@layer base,utilities;' + + expect(rebuild(text, '.used{color:red}')).to.equal('@layer base,utilities;\n@layer utilities{.used{color:red}}') + }) + + it('keeps a covered at-rule that holds declarations directly, such as @font-face', () => { + const text = '@font-face{font-family:F;src:url(f.woff2)}.unused{color:blue}' + + expect(rebuild(text, '@font-face{font-family:F;src:url(f.woff2)}')).to.equal( + '@font-face{font-family:F;src:url(f.woff2)}' + ) + }) + + it('returns an empty string when nothing is covered', () => { + expect(rebuildCoveredCSS({text: '@media print{.unused{color:blue}}', ranges: []})).to.equal('') + }) + + it('does not lose a rule whose range only partially overlaps it', () => { + const text = '@layer base{.used{color:red}}' + const rule = rangeOf(text, '.used{color:red}') + + expect(rebuildCoveredCSS({text, ranges: [{start: rule.start, end: rule.start + 3}]})).to.equal(text) + }) + + describe('rebuildCoveredCSSFromEntry', () => { + it('falls back to the raw coverage ranges when the stylesheet cannot be parsed', () => { + const text = '.used{color:red}@media{' + const entry = {url: 'https://example.com/broken.css', text, ranges: [rangeOf(text, '.used{color:red}')]} + + expect(rebuildCoveredCSSFromEntry(entry)).to.equal('.used{color:red}') + }) + + it('rebuilds a parseable stylesheet with its at-rule context', () => { + const text = '@layer utilities{.used{color:red}}' + const entry = {url: 'https://example.com/ok.css', text, ranges: [rangeOf(text, '.used{color:red}')]} + + expect(rebuildCoveredCSSFromEntry(entry)).to.equal(text) + }) + }) +}) From 2e50da6c68cfb8339b5af0babe3c44d5f6eb7d36 Mon Sep 17 00:00:00 2001 From: Tomas Madariaga Date: Mon, 21 Sep 2026 13:39:18 +0200 Subject: [PATCH 2/4] fix(packages/sui-critical-css): split multi-name @layer statements for clean-css A bodyless `@layer` naming more than one layer is mis-parsed by clean-css 5.3.3: it loses the statement AND every declaration of the rule that follows it, so `@layer a,b;.x{color:red}` minifies to nothing at all. That is exactly how a Tailwind sheet opens, and measured against a real app it silently dropped the 320 custom properties of the `:root,[data-theme=default]` token block sitting right after the statement. Emitting one statement per layer name establishes the same order and parses correctly. Co-Authored-By: Claude Opus 5 --- packages/sui-critical-css/src/covered-css.js | 10 +++++++--- .../test/server/covered-cssSpec.js | 20 ++++++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/packages/sui-critical-css/src/covered-css.js b/packages/sui-critical-css/src/covered-css.js index 489b80b06..5288b282e 100644 --- a/packages/sui-critical-css/src/covered-css.js +++ b/packages/sui-critical-css/src/covered-css.js @@ -24,6 +24,12 @@ const holdsDeclarations = node => const isLayerStatement = node => node.type === 'atrule' && node.nodes === undefined && node.name.toLowerCase() === KEEP_AT_RULE +// One statement per layer name, because clean-css 5.3.3 mis-parses a bodyless `@layer` +// that names more than one layer: it drops the statement AND every declaration of the +// rule that follows it (`@layer a,b;.x{color:red}` minifies to nothing at all). A +// sequence of single-name statements establishes exactly the same order. +const splitLayerStatement = node => node.params.split(',').map(name => `@${node.name} ${name.trim()};`) + const intersectsCoveredRange = (node, ranges) => { const start = node.source?.start?.offset const end = node.source?.end?.offset @@ -53,9 +59,7 @@ export const rebuildCoveredCSS = ({text, ranges}) => { } root.walk(node => { - // postcss leaves the terminating `;` of a bodyless at-rule to the parent's - // stringifier, so it has to be added back when lifting the node out on its own. - if (isLayerStatement(node)) return layerStatements.push(`${node.toString()};`) + if (isLayerStatement(node)) return layerStatements.push(...splitLayerStatement(node)) if (holdsDeclarations(node) && intersectsCoveredRange(node, ranges)) keepWithAncestors(node) }) diff --git a/packages/sui-critical-css/test/server/covered-cssSpec.js b/packages/sui-critical-css/test/server/covered-cssSpec.js index 05b9cc589..838df73c9 100644 --- a/packages/sui-critical-css/test/server/covered-cssSpec.js +++ b/packages/sui-critical-css/test/server/covered-cssSpec.js @@ -1,4 +1,5 @@ import {expect} from 'chai' +import CleanCSS from 'clean-css' import {rebuildCoveredCSS, rebuildCoveredCSSFromEntry} from '../../src/covered-css.js' @@ -56,19 +57,32 @@ describe('@s-ui/critical-css covered-css', () => { it('keeps a @layer statement even though coverage never marks it as used', () => { const text = '@layer base,utilities;@layer utilities{.used{color:red}}' - expect(rebuild(text, '.used{color:red}')).to.equal('@layer base,utilities;\n@layer utilities{.used{color:red}}') + expect(rebuild(text, '.used{color:red}')).to.equal( + '@layer base;\n@layer utilities;\n@layer utilities{.used{color:red}}' + ) }) it('keeps a @layer statement even when nothing else is covered', () => { expect(rebuildCoveredCSS({text: '@layer base,utilities;.unused{color:blue}', ranges: []})).to.equal( - '@layer base,utilities;' + '@layer base;\n@layer utilities;' ) }) it('hoists @layer statements above the rules, whatever their original position', () => { const text = '@layer utilities{.used{color:red}}@layer base,utilities;' - expect(rebuild(text, '.used{color:red}')).to.equal('@layer base,utilities;\n@layer utilities{.used{color:red}}') + expect(rebuild(text, '.used{color:red}')).to.equal( + '@layer base;\n@layer utilities;\n@layer utilities{.used{color:red}}' + ) + }) + + it('splits a multi-name @layer statement so clean-css does not swallow the next rule', () => { + const text = '@layer base,utilities;:root{--brand:red}' + const rebuilt = rebuildCoveredCSS({text, ranges: [rangeOf(text, ':root{--brand:red}')]}) + + // `@layer base,utilities;:root{--brand:red}` minifies to `:root{}` on clean-css + // 5.3.3: the statement is dropped and the rule after it loses every declaration. + expect(new CleanCSS({level: 2}).minify(rebuilt).styles).to.contain('--brand:red') }) it('keeps a covered at-rule that holds declarations directly, such as @font-face', () => { From 828af04b00d1c1ea3a23604a9f40415b21b6e881 Mon Sep 17 00:00:00 2001 From: Tomas Madariaga Date: Tue, 22 Sep 2026 10:59:43 +0200 Subject: [PATCH 3/4] fix(packages/sui-critical-css): keep nested @layer statements inside the at-rule that guards them Hoisting every `@layer` statement to the top makes the rebuild independent of the order Chrome reports coverage entries in, but a statement is not always safe to move: inside `@supports` or `@media` it only registers its layers while that condition holds, and inside another `@layer` it names sublayers of it. Hoisting those dropped the guard, which is the same loss of at-rule context this file exists to prevent, just for statements instead of style rules. Only top-level statements are hoisted now. A nested one stays where it is and is merely split, so `@supports (color:lab(0 0 0)){@layer a,b;}` rebuilds as `@supports (color:lab(0 0 0)){@layer a;@layer b;}` instead of registering the order unconditionally. Co-Authored-By: Claude Opus 5 --- packages/sui-critical-css/src/covered-css.js | 19 ++++++++++++++++++- .../test/server/covered-cssSpec.js | 15 +++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/sui-critical-css/src/covered-css.js b/packages/sui-critical-css/src/covered-css.js index 5288b282e..da85a073f 100644 --- a/packages/sui-critical-css/src/covered-css.js +++ b/packages/sui-critical-css/src/covered-css.js @@ -53,16 +53,33 @@ export const rebuildCoveredCSS = ({text, ranges}) => { const root = postcss.parse(text) const keep = new Set() const layerStatements = [] + const nestedLayerStatements = [] const keepWithAncestors = node => { for (let current = node; current && current.type !== 'root'; current = current.parent) keep.add(current) } root.walk(node => { - if (isLayerStatement(node)) return layerStatements.push(...splitLayerStatement(node)) + if (isLayerStatement(node)) { + if (node.parent.type === 'root') return layerStatements.push(...splitLayerStatement(node)) + + // A statement inside `@supports` or `@media` only registers its layers while that + // condition holds, and one inside another `@layer` names sublayers of it, so it + // cannot be hoisted out of its parent the way a top-level one can. Keep it in place. + return nestedLayerStatements.push(node) + } if (holdsDeclarations(node) && intersectsCoveredRange(node, ranges)) keepWithAncestors(node) }) + // Replacing them during the walk above would make postcss visit the statements the split + // produces, so they are only rewritten once the walk is over. + nestedLayerStatements.forEach(node => { + const replacements = splitLayerStatement(node).map(statement => postcss.parse(statement).first) + + node.replaceWith(...replacements) + replacements.forEach(keepWithAncestors) + }) + const discarded = [] root.walk(node => { if ((node.type === 'rule' || node.type === 'atrule') && keep.has(node) === false) discarded.push(node) diff --git a/packages/sui-critical-css/test/server/covered-cssSpec.js b/packages/sui-critical-css/test/server/covered-cssSpec.js index 838df73c9..56a58943e 100644 --- a/packages/sui-critical-css/test/server/covered-cssSpec.js +++ b/packages/sui-critical-css/test/server/covered-cssSpec.js @@ -76,6 +76,21 @@ describe('@s-ui/critical-css covered-css', () => { ) }) + it('does not hoist a @layer statement out of the at-rule that guards it', () => { + const text = '@supports (color:lab(0 0 0)){@layer a,b;}@layer a{.used{color:red}}' + + // Hoisting it would register the order even where the condition does not hold. + expect(rebuild(text, '.used{color:red}')).to.equal( + '@supports (color:lab(0 0 0)){@layer a;@layer b;}@layer a{.used{color:red}}' + ) + }) + + it('does not hoist a @layer statement out of the layer it names sublayers of', () => { + const text = '@layer a{@layer b,c;}@layer a{.used{color:red}}' + + expect(rebuild(text, '.used{color:red}')).to.equal('@layer a{@layer b;@layer c;}@layer a{.used{color:red}}') + }) + it('splits a multi-name @layer statement so clean-css does not swallow the next rule', () => { const text = '@layer base,utilities;:root{--brand:red}' const rebuilt = rebuildCoveredCSS({text, ranges: [rangeOf(text, ':root{--brand:red}')]}) From ef7f1d2b70738f1474a848a1d1fa7039e006373c Mon Sep 17 00:00:00 2001 From: Tomas Madariaga Date: Tue, 22 Sep 2026 10:59:44 +0200 Subject: [PATCH 4/4] chore(Root): run the sui-critical-css server specs in CI and sync the lockfile `test:server` is what the Tests step of `.github/workflows/main.yml` runs, and its glob only listed `sui-test-contract` and `sui-js-compiler`, so the 17 specs added with the at-rule-context fix never executed in CI. A regression in `covered-css.js` would have gone unnoticed. Adding the package to the brace list takes the run from 18 to 35 specs. `package-lock.json` also never got the `postcss` dependency the same fix added to `packages/sui-critical-css/package.json`. `npm ci` tolerates it because postcss 8.4.31 already resolves at the root, but every other dependency change in this repo keeps the lockfile in step. Only that one entry is touched: regenerating the whole file would drag in nine unrelated version bumps that the release commits left behind, since they bypass CI and never sync the lockfile. Co-Authored-By: Claude Opus 5 --- package-lock.json | 3 ++- package.json | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index d1eab1489..9bddfa443 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27023,7 +27023,8 @@ "clean-css": "5.3.3", "node-fetch": "2.7.0", "path-to-regexp": "0.1.10", - "playwright-chromium": "1.25.0" + "playwright-chromium": "1.25.0", + "postcss": "8.4.31" } }, "packages/sui-critical-css-middleware": { diff --git a/package.json b/package.json index e677b7c21..3d6ff4ed0 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "test:server:ci": "npm run test:server", "test:client": "npx @s-ui/test browser -P 'packages/**/!(server)/*Spec.js' -H", "test:client:watch": "npm run test:client -- --watch", - "test:server": "npx @s-ui/test server -P 'packages/{sui-test-contract,sui-js-compiler}/**/server/*Spec.js'", + "test:server": "npx @s-ui/test server -P 'packages/{sui-test-contract,sui-js-compiler,sui-critical-css}/**/server/*Spec.js'", "test:server:watch": "npm run test:server -- --watch", "test:e2e": "node ./packages/sui-studio/test/server/integration/static-server.js ./packages/sui-studio/test/server/integration/sample-studio/public && npx @s-ui/test-e2e --baseUrl=http://localhost:1234", "pre-commit": "sui-lint js --staged && sui-lint sass --staged",