Repository navigation
Conversation
Merging an adjacent @media rule with the same query into the previous one re-minified the whole merged body after every rule, so a run of n such rules cost O(n^2) rule minifications (4000 rules: 3.6s in a release build). Inside a style rule whose nesting is compiled away, every pass also charged the accumulated body against the selector-expansion limit again, so a run of a few hundred rules failed with selector_expansion_limit_exceeded. Defer the re-minify until the run ends, the same way the style-rule declaration merge defers its re-minify: mark the merged rule pending and minify it once before anything is pushed after it or the list ends. Rules that minify away in between never ended a run before and still do not.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fixed; ready for a maintainer.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The deferred-flush pattern mirrors the existing StyleRuleMergeState::pending_minify from #31920 and the flush points look complete, but since the change acknowledges output differences for runs of ≥3 rules where re-minify is not idempotent (the color-scheme and lab() cases in the description), a human sign-off on that trade-off would be worthwhile.
What was reviewed:
- Traced every path that pushes to
rules(generic push, style-rule push, extras append, end-of-list) — each flushespending_media_minifyfirst, so thelast() is Mediainvariant holds. - Checked that dropped rules (
@media not all, empty style rules,parent_is_unused) correctly keep the run going by not pushing and not flushing. - Verified
pending_media_minifyandmerge_state.pending_minifyare mutually exclusive (last is Media vs. last is Style), so flush order at the generic push is immaterial. - Confirmed the
?error propagation inflush_pending_media_mergematches the old per-mergelast_rule.minify(...)?.
Extended reasoning...
Overview
This PR changes CssRuleList::minify in src/css/rules/mod.rs to defer re-minification of a merged @media rule body until the run of same-query rules ends, instead of re-minifying the accumulated body after every merge. It adds a pending_media_minify: bool flag, a flush_pending_media_merge helper, and calls that helper at every point where a rule is appended to the output list (the generic push, the style-rule push in minify_style_arm, the extras append, and end-of-list). Three new tests are added to test/js/bun/css/duplicate-declaration-merge-hang.test.ts: a 6000-rule timeout-guarded hang test, a selector-expansion-limit test, and an output test covering every way a run can end.
Security risks
None. This is pure CSS minifier optimization with no I/O, auth, network, or user-controlled path handling. The only user-facing effect is faster minification and the removal of a spurious selector_expansion_limit_exceeded error.
Level of scrutiny
Medium-high. The change is small (~60 lines of Rust) and follows an established pattern in the same file (#31920's StyleRuleMergeState::pending_minify for style-rule declaration merges), so the shape is well-understood. However, correctness depends on a state-machine invariant — pending_media_minify implies rules.last() is a Media rule — that must hold across every code path in a ~500-line function. I traced each push/append site and believe the invariant holds (and it is debug_asserted in the flush), but the PR also acknowledges that for runs of ≥3 rules, output can differ from before where re-minifying already-minified output is not a no-op. The PR description argues convincingly that the new behavior is more correct (output no longer depends on how many pieces the input was split into) and reports byte-identical differential results on 15 real-world stylesheets, but this is a deliberate semantic change to minifier output that a maintainer should acknowledge rather than have auto-approved.
Other factors
- The PR description is exceptionally thorough: root-cause analysis, correctness argument per flush point, timing tables, differential testing over real and generated stylesheets, and a probe demonstrating the old behavior's dependence on run length.
- Tests are well-designed per REVIEW.md: the hang test uses a spawn timeout (not
setTimeout), asserts exact stdout, and drains pipes concurrently; the output test covers every run-termination path including dropped intermediate rules and the extras-only style-rule case. - No CODEOWNER on
src/css/. - The
@supportsarm has a different shape (drops duplicates rather than merging bodies) so is not affected; the PR notes #33680 should adopt this pattern when it adds body merging there.
|
On the one open point from the review, the output difference for runs of three or more rules, two facts for whoever signs off:
The |
Fold the @media flag into the existing merge state and settle it together with the pending declaration merge in flush_pending_merges; every push into the output list goes through end_merge_run, so a push site cannot settle one kind of merge and miss the other. Output is unchanged.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Reshaped after review, in c1cae64: the |
|
Heads-up: #38589 (which carries #33680's change) turns the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The deferred-minify logic looks sound — every push into rules is preceded by end_merge_run, the two pending flags are mutually exclusive by construction, and error propagation through the new Result return type is consistent. Given the acknowledged output changes for runs of three or more rules (the probe table and the "single-pass leftovers" note), a human sign-off on that behavioral trade-off would still be worthwhile.
What was reviewed:
- Traced every
rules.push/append/extendinCssRuleList::minifyandminify_style_arm— each is preceded byend_merge_runor is behind one that already ran. - Verified
pending_at_rule_minifycannot be set whilerules.last()is notMedia: the flag is only set on a merge (last isMedia),break 'armskips the push, and the flag is cleared before any subsequent push. - Checked the style-rule-follows-
@mediapath:rules.last()won't matchCssRule::Style, somergedstays false and the flush happens at the push (or the extras'end_merge_runwhen the rule itself is empty — the chrome-60inset-inline-starttest case).
Extended reasoning...
Overview
This PR fixes an O(n²) hang in CSS minification when many adjacent @media rules share a query, and a spurious selector_expansion_limit_exceeded error from the same loop. It renames StyleRuleMergeState → MergeState, adds a pending_at_rule_minify flag alongside the existing style-rule pending_minify, and folds both settlement paths into one flush_pending_merges / end_merge_run pair. The @media arm now appends the body and sets the flag instead of re-minifying the whole accumulated body per merge; the flush runs once when the run ends (before the next push, or at end of list). Three new tests cover the timing regression, the expansion-limit false positive, and every way a run can end.
Security risks
None. The change is confined to CSS AST transformation and does not touch parsing, allocation sizing, or any trust boundary. The ? on the new Result return type propagates the same MinifyErr (selector-expansion limit) that the removed per-merge call already propagated.
Level of scrutiny
Medium-high. The mechanism (defer minify, flush before push) mirrors what #31920 already established for the style-rule side, and the diff is small. But the change reorders when a shared-path minify runs, and the PR itself documents that output for runs of ≥3 rules can differ from before in specific handlers (color-scheme fallback duplication, a lab() fallback left before a later color: red, a duplicate rule from selector partitioning). The author argues — convincingly — that these are cases where main's output already varied with how many pieces the same content was split into, and that this change makes the output stable at the two-rule result. That is a behavioral trade-off, not a pure refactor, and the author's own comment ("two facts for whoever signs off") anticipates a human decision on it.
Other factors
- The bug-hunting system found nothing. I traced every push site into the output list and each goes through
end_merge_runfirst (src/css/rules/mod.rs:608,:842,:868), so a pending@mediabody is always minified before it can be buried or observed. The mutual exclusion of the two flags holds becausepending_at_rule_minifyis only set whenrules.last()isMedia, which precludespending_minify(only set when last isStyle), and both are cleared before any push changesrules.last(). - Test coverage is thorough: the linear-time test spawns with a 60s kill so the quadratic version fails deterministically; the run-end test covers end-of-list, style rule, minified at-rule, non-minified at-rule (
@keyframes), different-query@media, style-rule-emitted-only-as-fallbacks, and dropped rules mid-run. The PR reports byte-identical output over 15 real-world stylesheets and 800 generated ones (modulo the documented handler quirks). - The comment-cop bot flagged several doc comments; the author trimmed most in 3dbd77d and left justifications for the four that remain (they document the deferral rationale, parallel to the existing
pending_minifydoc). Those responses look reasonable — they are struct/function doc comments, not workaround excuses — but a human may want to confirm the four remaining ones are acceptable per repo comment policy. - No CODEOWNERS for
src/css/. No prior human review on the thread.
|
The trade-off for runs of three or more rules is the one discussed in my earlier comment and in the description's probe table (main's output for such runs already depends on the run length; this makes it the two-rule result regardless), so that is the point for a maintainer to weigh in on. The comment threads from the comment linter are all replied to and resolved: three comments were removed or shortened, the remaining ones are the doc comments for the merge state and its two functions. |
|
Agreed on the ordering. If #38589 lands first I will rebase this and switch its |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The deferred-minify pattern mirrors #31920's style-rule fix, every push site into rules goes through end_merge_run, and the tests cover each way a run can end. Deferring for a maintainer to sign off on the one open point the author flagged: output for runs of 3+ rules changes where re-minifying is not idempotent (the probe table in the description).
What was reviewed:
- Traced every
rules.push/rules.append/rules.extendsite inCssRuleList::minifyandminify_style_arm— each flushespending_at_rule_minifyfirst (either viaend_merge_runor because the flag can't be set on that path). - Checked mutual exclusion of the two pending flags:
pending_at_rule_minifyrequiresrules.last()to beMedia, so the style-rule forward merge andhad_pendingpaths can't fire while it's set. - Checked
last_compathandling:end_merge_runclears it, and the style-rule push site correctly re-sets it tosty_compatafterward. - Verified error propagation: the old per-merge
last_rule.minify(...)?and the new deferred flush both propagateMinifyErrvia?; the discarded drop-decision matches the old behavior.
Extended reasoning...
Overview
This PR fixes an O(n²) performance regression in the CSS minifier when many adjacent @media rules share a query. The CssRule::Media arm of CssRuleList::minify used to re-minify the entire merged body after appending each rule; now it appends and sets pending_at_rule_minify, and one deferred minify runs when the run ends. The existing StyleRuleMergeState (from #31920, which fixed the same shape for style rules) is renamed to MergeState and gains the new flag; flush_pending_style_merge becomes flush_pending_merges (now fallible, now handles both flags), and a new end_merge_run helper (flush + clear last_compat) is called before every push into the output list. Two files touched: src/css/rules/mod.rs (~60 lines net) and three new tests in test/js/bun/css/duplicate-declaration-merge-hang.test.ts.
Security risks
None. This is pure CSS minifier control flow with no external input handling beyond what the parser already produces; no auth, crypto, filesystem, or network paths are touched. The change actually fixes a soft-DoS vector (quadratic minify time on adversarial input, and a spurious selector-expansion-limit error at 256 rules).
Level of scrutiny
Medium-high. The change is small in line count but sits in the core rule-merging loop of CssRuleList::minify, where the invariant ("a pending flag always describes rules.last()") must hold across every push site — including the several in minify_style_arm for logical properties, @supports extras, incompatible-selector partitioning, and nested-rule splits. I traced each push site and confirmed end_merge_run (or an equivalent guarantee that pending_at_rule_minify is unset) precedes it. The two flags are structurally mutually exclusive because one requires the last rule to be Style and the other Media, and this holds through the style-rule forward merge. The signature change (flush_pending_merges is now fallible and requires DeepClone) is threaded correctly through all callers.
Other factors
The PR description and follow-up comments are unusually thorough: differential testing over 15 real-world stylesheets across three target sets was byte-identical, and 800 generated stylesheets differ only in the known non-idempotent handlers. The tests cover the linear-time claim (spawn timeout), the fixed selector-expansion false positive, and every way a merge run can end. The comment-cop bot threads are all resolved (comments trimmed in 3dbd77d).
The reason I'm not approving outright is that the author explicitly flagged, in their last two comments, a behavioral change for runs of 3+ rules where re-minifying already-minified output is not a no-op (today: color-scheme fallback duplication, a leftover lab() fallback, a duplicate from selector partitioning). The author's argument — that main's output for such runs already depended on run length and this makes it consistent — is sound, but they asked for a maintainer to weigh in on that trade-off, and it's the right call for a change to minifier output semantics.
|
Updated 5:51 PM PT - Aug 14th, 2026
❌ @robobun, your commit dd6ce60 has 8 failures in
🧪 To try this PR locally: bunx bun-pr 38531That installs a local version of the PR into your bun-38531 --bun |
|
This branch conflicts with main since #42948, which added the A merge of this branch with main Checked on that merge:
|
### What does this PR do? Consolidates the open pull requests on the CSS parser (`src/css`: parsing, minifying, lowering, printing, CSS modules) into one. 179 open pull requests were in scope. Each was judged by one rule: **the output must mean to a browser what the input meant.** Every claim was run through a build of `main`, a build with the change, lightningcss 1.33 and Chromium 156 (CSSOM, computed values, painted pixels for colors and gradients), and counted against 10,116 real stylesheets: the published CSS of about 190 popular npm packages, plus stylesheets from open source repositories. 107 land here, whole or in part, most of them cut down. The source diff is net negative. Of the 10,116, 9,509 are stylesheets that someone ships. The rest are test fixtures of other tools (521), Tailwind v4 source that needs its plugin (59) and SCSS, PostCSS or template source (27). | 9,509 shipped stylesheets | `main` | this branch | |---|---|---| | fail to build | 71 | 35 | | ... of those, valid CSS that Bun rejects | | 0 | | minified output reads differently in Chromium than the input | 552 | 2 | | CSS modules with a name that is referenced but not defined, or exported but not defined | 166 | 0 | | all 10,116 | `main` | this branch | |---|---|---| | fail to build | 192 | 141 | | minified output reads differently in Chromium than the input | 593 | 15 | | build on `main`, fail here | | 0 | | right on `main`, wrong here (minified, and at the default targets) | | 0 | | size of the minified output, no targets | 162.32 MB | 162.47 MB (+0.093%) | | size of the output at the default targets | 224.1 MB | 210.2 MB (-6.2%) | A syntax error is a build error, with one exception: the IE star hack `*zoom: 1` is skipped, because tachyons and handsontable ship it. The 35 shipped stylesheets that fail are invalid CSS: 28 have a selector that Chromium drops (semantic-ui-css, swiper, @ionic/core, Adobe Spectrum, xp.css), 2 an `@import` after other rules, and one each `@supports not X and Y`, a nested `@keyframes`, a stray comma, a selector list in `::slotted()`, `@media screen\0` (govuk-frontend). The 2 that read differently fold a `color-mix()` inside a gradient (see the known gaps). The growth of the minified output is rules and declarations that `main` deleted by mistake: 87 KB of rules, 45 KB of prefixed declarations that only browser targets can rule out. #### Popular CSS that `main` breaks today - normalize.css, modern-normalize, sanitize.css: `font-family: monospace, monospace` is collapsed to `monospace`, which changes the font size from 16px to 13px (#46662). - CKEditor 5, Tabler, Carbon, VitePress, Ionic, PatternFly, Excalidraw: a rule of custom properties is deleted as a "duplicate" of a later rule with the same selector and other custom properties. 37 stylesheets lose 1,681 custom properties (#33684). - Material Components, daisyUI, DocSearch, plyr: a `var()` size loses to its own fallback (#42101). - nextra, daisyUI, CKEditor: declarations written after a nested rule are moved above it (#45462). - antd 4, purecss, govuk-frontend: browser hacks in selector lists start to apply everywhere (#42100). - foundation-sites, flatpickr, 98.css, blueprintjs, scalar, react-datepicker: the build fails on valid CSS (#46756, #38552, #38534). tachyons, handsontable: it fails on the IE star hack (#46751). - nodejs.org, Mantine: a CSS module exports classes that no rule defines, because the `@apply` or `@mixin` in them is moved out of the rule (#46759). - hint.css and 42 more: `z-index: 1000001` and `1000000` become the same number (#46715). - Adobe Spectrum: `--version: 5.0.1` prints as `5.1`. - A quarter of all stylesheets: minified output has a stray space in `align-items: center` (#41993). #### Behavior changes to look at first 1. **`light-dark()` is kept as written at the default browser targets** (#42009). The lowered form `var(--buncss-light, a) var(--buncss-dark, b)` is invalid unless a compiled `color-scheme` covers the element: 184 of the 215 real stylesheets that get it never define the variable, so every browser drops the declaration. The docs section on the polyfill is removed. 2. **`border` is not built from longhands** (#42072): `border-width:0;border-style:solid;border-color:red` stays three declarations, because `border` also resets `border-image`. 305 real stylesheets grow by 30 KB in total; `bootstrap.min.css` by 113 bytes, 5 gzipped. No real stylesheet was found where the old merge shows, so this is the one to drop if size matters more. 14 existing expectations change. 3. **A CSS module no longer exports names that only occur inside `:global(...)`** (#45487). 73 real modules lose 202 export keys, all of them names that no rule has. `import { g }` or `composes: g` of such a name becomes a build error. 4. **Container names and `@counter-style` names stay global in a CSS module** (#45247 by @mikamikasuki). `main` hashes the `@container` name but not `container-name`, so a named query never matches. postcss-modules (Vite, css-loader, Next.js) keeps them global; lightningcss and esbuild hash both. Global keeps a container that is queried from another file or from JS working. 5. **Syntax errors are build errors, also two that `main` lets through**: a media query that stops short (`@media screen and {`, `@media not {`, `@import "a.css" screen and;`) and `@media (width <) foo {`. No stylesheet of the 10,116 has either. A condition that is valid by the grammar though no browser knows it, `( ... )` or `fn( ... )`, is kept as written (#46756): `(min-width:0\0)`, `(not(hover))`. 6. **Declarations after a nested rule stay where they were written**, as a nested declarations rule (#45462). Changes the output of 199 real stylesheets; in a fuzz of 2,850 nested programs compared by computed style, `main` was wrong in 1,056 and the pull request's code, before it was trimmed here, in none. 7. **`Bun.color`**: `color()` inputs are converted instead of returned as a string (`Bun.color("color(srgb 1 0 0)", "hex")` was `"color(srgb 1 0 0)"`); `currentColor`, system colors and `light-dark()` give `null` in the converting formats instead of the keyword as a string; a relative color or `color-mix()` outside sRGB is clipped, not gamut-mapped (`hsl(from oklch(100% .399 336.3) h s l)` is `#ff5bff`, was `#fff`). 8. The `:-moz-any()` and `:-webkit-any()` copies of rules with `:is()` are gone at the default targets (#45945, #42029). The `:-webkit-any()` copy outranked the original rule: `.title{color:blue} :is(h1,h2,h3){color:red}` painted red. 5.8% of all default-target output was the `:-moz-any()` copies. 9. `Property` grows from 216 to 224 bytes: `Url` stores its text as a slice so that two `url()` values can be compared (#46895). The pull request packed a pointer and a `u32` behind `unsafe` to avoid that. #### Not in this PR, needs a decision - **Raise the default browser targets (#40369) and add a `cssTarget` option (#40368).** Today: Chrome 80, Edge 80, Firefox 78, Safari 14, Opera 67, and no public way to change them. At Vite 8's floor the output of the 9,924 stylesheets is 15.3% smaller (3.9% gzipped). About a dozen fixes here only matter at today's floor, and code for older targets is unreachable without the option. - Vendor prefixes for `user-select`, `appearance`, `hyphens` and five more (#45814): 1,116 real stylesheets lack a prefix they need at the default targets. And for `backdrop-filter` (#44648): 173. - Clip instead of gamut-map in the sRGB fallback of a plain wide-gamut color: deletes about 90 lines, changes every pinned fallback hex. If the mapping stays, #44647 by @oddharsh makes it closer to what browsers paint (colors off by 2/255 or more: 168 to 110 of 3,170 real ones). #### Changes **Rules: duplicates, merging, nesting, fallbacks** - The `:-moz-any()` copy of every rule with `:is()` or `:not(a, b)` is gone: Firefox 78 has `:is()` (#45945). 1,164 real stylesheets - Keep declarations written after a nested rule in place with a nested declarations rule (#45462). 379 real stylesheets - With nesting compiled away, an unknown at-rule inside a style rule was moved out of it: `.wrapper{@apply flex}` printed a bare `@apply flex;` and the rule was gone, so a CSS module exported a class that no rule defines. It now stays in a block under the compiled selector. 72 real modules with Tailwind's `@apply`, 25 with Mantine's `@mixin` (#46759). 316 real stylesheets - Keep !important in the rules generated for logical properties and color-scheme (#38597). 311 real stylesheets - With browser targets, remove an earlier duplicate rule only when its declaration block is equal (#46861). 171 real stylesheets - Compare custom property names in `PropertyId` equality and importance in duplicate-rule detection (#33684). 40 real stylesheets - Keep same-condition @supports rules and empty named @layer blocks (#46733). 31 real stylesheets - Keep target-added vendor prefix passes on cloned and re-minified rules, and never print a bare `:full-screen` (#43339). 12 real stylesheets - Keep the selectors on rules added for color fallbacks and logical properties (#43240). 3 real stylesheets - A later declaration of a property drops the `@supports` color copies of the earlier one, which printed after the rule and won (#45560). 2 real stylesheets - Do not merge style rules in a stylesheet that holds `revert-rule` (#46953). 1 real stylesheets - `@import ... supports(-webkit-mask: none)` kept its prefix (#38589) **Properties and shorthands** - Print align-items:center without a stray space when no safe/unsafe keyword is given (#41993). 3,317 real stylesheets - Emit the physical longhands instead of the inset shorthand for targets without it (#38583). 1,452 real stylesheets - Keep the stops of an upward gradient as written, and drop a `50%` hint only at the midpoint (#46790). 104 real stylesheets - Emit unparsed border-block-* declarations when compiling logical borders (#38582). 89 real stylesheets - Keep repeated font families and dedupe only the inserted system-ui fallbacks (#46662). 88 real stylesheets - A `width` / `height` / `min-*` / `max-*` with `var()` or `env()` was printed before the plain fallback written above it, so the fallback won (#42101). 26 real stylesheets - Keep box-orient and box-direction when flex-direction or flex-flow follows (#45672). 16 real stylesheets - Write held prefixed background declarations at every flush (#46882). 6 real stylesheets - Derive equality for Size2D and BorderRadius, and keep every prefix in the border-radius handler (#45665). 2 real stylesheets - Print `none` instead of an empty border-image, mask-border or mask-box-image value (#45143). 2 real stylesheets - Keep font/transition sub-properties on the side of the shorthand they were written on (#41990). 2 real stylesheets - CSS-wide keywords are not names: `font:12px Arial;font-family:inherit` printed `font:12px inherit`, `color-scheme:inherit` and `color-scheme:auto` printed `normal`, `transition-property:unset` was folded into `transition` (#42022). 2 real stylesheets - Only fold font longhands into `font` when the rule declared a `font` (#45459) - Keep a font family name quoted when its identifiers do not read back as the same name (#46606) - Stop rewriting valid positions into ones browsers reject (#45197) - `border` is no longer built from longhands unless the rule also sets what `border` resets (`border-image`), and an author's `border-image` reset is no longer dropped. `transition` is likewise only written where the rule wrote one, because it resets `transition-behavior` (#42072) - `border; border-image: <gradient>; border-image-slice: 1` lost the gradient in every default build (#42018) - A prefixed `linear-gradient` keeps its direction, written or generated: the prefixed syntax names the starting side and measures angles differently (#46697) **At-rules** - Print @font-face format() keywords as strings (#37979). 439 real stylesheets - Print @supports `not` conditions without a leading space (#38559). 88 real stylesheets - Reset after the number token of a unicode-range (#45835). 33 real stylesheets - Keep `<general-enclosed>` media and container conditions as written (#46756). 24 real stylesheets - A strict range such as `(width < 768px)` lowered for old targets still matched at exactly 768px. The step is now `.02px`. The `not all and` form for other units is not taken (#46806). 18 real stylesheets - Keep scanning an @supports condition after a nested block (#38552). 9 real stylesheets - Every `@counter-style name` printed as `@counter-stylename`, which browsers drop (also #46692). In a CSS module the name now stays global, like the `list-style` that refers to it (#33679). 9 real stylesheets - Keep a keyframe whose block has an invalid item (#46288). 2 real stylesheets - Read an @Property initial-value on the stylesheet's parser, keep an unmatched value as written (#45806). 2 real stylesheets - `@media {}` is `all`. A media query that stops after `and`, `or` or `not` (`@media screen and {`) is a build error; `main` printed `not all` or dropped the condition, silently. The `@layer` and type selector parts are not taken (#46869). 2 real stylesheets - Accept a name-only `@container` and a `style()` feature with no value (#46689). 1 real stylesheets - Parse `<timeline-range-name> <percentage>` keyframe selectors (#45652) - Allow `or` inside parentheses after a media type (#45088) - Print the `=` comparison in media and container queries as `=` (#45071) - Parse the @font-palette-values font-family descriptor as a list of family names (#45484) **Selectors** - Never wrap a selector list in :is() (#46787). 414 real stylesheets - Stop emitting :-webkit-any() copies of :is() that outrank the original rule (#42029). 373 real stylesheets - With browser targets, a selector list with a vendor-prefixed member was split, which revives hacks that rely on other engines dropping the whole list: `.s *::-ms-backdrop,.s{flex:auto}` applied everywhere. Rewritten; the list is printed as written (#42100). 42 real stylesheets - Print the vendor prefix on downleveled :any-link, :read-only, :read-write, :placeholder-shown and :autofill (#42131). 12 real stylesheets - Fix nesting lowering of a `&` that does not lead its selector (#43047). 8 real stylesheets - `@scope`: `&` and implicit selectors compile to `:where(:scope)`, relative selectors and bare declarations parse (#43165). 7 real stylesheets - Only reject bare :global / :local pseudo-classes in CSS modules (#38534). 5 real stylesheets - Stop truncating `@scope to (...)` and stop replacing `&` in the scope-end selector (#43146). 2 real stylesheets - `[xl|href$=".pdf"]` lost its namespace prefix (#45159). 2 real stylesheets - Match An+B keywords exactly and take only `+` as the leading sign (#42953). 1 real stylesheets - Parse ::details-content and ::picker() like ::part() (#45504). 1 real stylesheets - Parse :window-inactive as an ordinary pseudo-class, and accept it after ::selection (#45815) - Print :nth-col(1) and :nth-last-col(1) in functional form (#41575) - `li::before::marker` failed the build (#45810) - `&` inside `:nth-child(... of S)` is resolved when nesting is compiled away. The second selector serializer (326 lines) is deleted (#43024) **Colors and `Bun.color`** - Keep light-dark() as written under the default browser targets (#42009). 227 real stylesheets - Number and percentage channels parse in `lab()`, `lch()`, `oklab()`, `oklch()`, `hsl()` and `hwb()`; the hue of `lch` / `oklch` is no longer scaled by alpha (#45084). 130 real stylesheets - `Bun.color` returns `null` for `currentColor`, system colors and `light-dark()` in the converting formats, instead of the CSS keyword as a string (#33047). 130 real stylesheets - Relative colors: channel keywords are on the scale of their function (`rgb(from #369 calc(r + 10) g b)`), the origin's alpha is kept, a relative color kept as written gets no `@supports` copies, and a result outside sRGB is not folded (#45362). 31 real stylesheets - Keep rotate: 0deg out of none, and keep the origin alpha in relative colors (#38486). 13 real stylesheets - Compute the D50 reference white in f32 so greys convert to lab(l 0 0) like lightningcss (#38535). 7 real stylesheets - Return a missing hue for a grey from conversions into lch, oklch, hsl and hwb (#45201). 5 real stylesheets - Fix color(a98-rgb), srgb-linear relative colors and color-mix(in xyz-d50) (#38487). 4 real stylesheets - Match named colors without regard to ASCII case (#45092). 3 real stylesheets - Fold min(), max() and clamp() over plain numbers (#39515) - `calc(l - .08)` in a percentage channel of a relative color became NaN and printed `#000` (also #45970) (#45971) - Bun.color: convert color() inputs instead of returning them as a CSS string (#38488) - A `color-mix(in srgb, ...)` result outside sRGB prints `color(srgb ...)` instead of a gamut-mapped hex (#39269) **`calc()` and numbers** - Rounding to 6 significant digits ate into the integer part: `z-index:2147483647` became `2147480000`, `max-width:9999999px` became `10000000px`, `--x:1234567.5` became `1234570`. It now only cuts a fraction short: a value kept as tokens prints its source text, a typed value prints every integer digit and at least one digit of the fraction (#46715). 43 real stylesheets - A number without a unit is no longer repaired into `px` (`height:1.5715` is dropped by a standards-mode browser, so it prints as written), `calc(-1 * 0)` is not a length, and on a property of the range [0,∞] (sizes, border radii, padding, border and outline widths, gap, `flex-basis`, `font-size`) a `calc()` that folds below zero prints `0` instead of an invalid negative literal, and a negative literal no longer replaces the valid declaration before it (`width:10px;width:-5px` printed `width:-5px`) (#42104). 24 real stylesheets - Print numbers from the f32's shortest digits, not its f64 expansion (#38565). 17 real stylesheets - Print negative numbers below 1 with a single minus sign (#38516) - Keep math functions over two different length units as written (#38489) - Fold rem() with a truncated quotient, so it keeps the sign of the dividend (#32286) - A one-argument `min()` / `max()` around a sum printed without any function (`width:100% - 20px`) (#46145) - `round()` breaks ties upward and handles a negative step as browsers do (#45249) **CSS modules** - A CSS module exports its `@keyframes` names (`styles.spin`). Hashing is unchanged (#46720). 145 real stylesheets - A name inside `:global(...)` prints plain but was exported as `name_<hash>`, a class no rule has (also #38573) (#45487). 90 real stylesheets - Keep container names global in CSS modules (#45247). 6 real stylesheets - Tag the keyframes name when an `animation` value falls back to raw tokens (#46034). 6 real stylesheets - In a plain stylesheet `composes: a from "./b.css"` was treated as an import: the file was inlined and an absolute path printed (#45961). 4 real stylesheets - Drop a rejected composes instead of failing the file at print time (#38538). 1 real stylesheets - Keep every rule's custom properties for the composes conflict check (#38523) - Keep composes working after a nested rule in the same block (#38520) - `IdentOrRef::hash` read 2 of 16 bytes, so in release builds every class of a module hashed alike (#30784) **Tokens, `url()`, diagnostics** - The block skip stack no longer allocates on every block exit (#39533) - Report the diagnostic's line without its line breaks in position.lineText (#38342). 192 real stylesheets - Compare `url()` values by their text (#46895). 26 real stylesheets - Values kept as tokens keep the blanks they were written with instead of having them re-invented at print time: `attr(x type(<number>))` printed `type(< number> )`, which browsers drop. Minified output still drops the blanks that cannot matter (#45577). 19 real stylesheets - The IE star hack (`*zoom:1`) failed the whole build. The declaration is skipped, as every browser does. It is the one syntax error that builds: tachyons, handsontable and jasmine ship it (#46751). 8 real stylesheets - The path of a copied asset spliced into `url()` is always quoted, so a file name with a space or a parenthesis cannot end the `url()` (#41557). 7 real stylesheets - Move the skip loop of parse_until_before out of the generic body (#39520) - Remove dead code from bun_css: the unused `impl Default for Property` (#45618) - Default the at-rule and qualified-rule parser traits, drop the url_to_css shim (#31999) **Speed** Four inputs that took quadratic or exponential time, or produced output out of proportion, are now linear, with no new limit and no extra pass (#31639, #42243, #38531, #43421). **Tests only** - `css-fuzz.test.ts` never ran in CI and timed out on debug builds; it is a deterministic table now (#38500) - `nth-anplusb-ident.test.ts` counts heaps in a child process (#46331) - `color.test.ts` checks the ansi-16 sweep with one `expect()` (#42824) - Not CSS, but it failed this PR's CI and fails on `main` too: `ws-proxy.test.ts` on Windows, "a message sent during a TLS renegotiation arrives after it". The helper server now renegotiates from the callback of the frame's write (#46170) **No pull request covered these** - `--v: 5.0.1` printed as `5.1`, `--w: 1.0.0` as `10`, `--x: 1+2` as `12`, `2n+1` as `2n1`: a number token that touches another keeps its source text. - `color-scheme: auto` printed as `normal`, and `color-scheme: dark light` as `light dark`. - A bare `:global` or `:local` in a CSS module sets the scope of the names after it, up to the end of the selector, as in css-loader. It was a build error. - The scrollbar states (`a:horizontal`, `::-webkit-scrollbar{&:horizontal{}}`) are valid in a plain compound, as in WebKit and Blink. - Without browser targets, `-webkit-box-align`, `-ms-flex-align` and the like were dropped next to their standard form (flatpickr, foundation-sites, materialize-css: 28 stylesheets), and `-webkit-fill-available` before `-moz-available` (beercss). - `(-o-min-device-pixel-ratio: 2/1)` printed as `2`. - `sin()`, `cos()` and `tan()` are exact at multiples of 90deg: `calc(100px * cos(90deg))` printed `-.00000437114px`. - A `display-p3` color at targets that have P3 but not the other `color()` spaces (Safari 10.1 to 14) got itself as a fallback. - `width: rem(-18px, 5px)` folds to a negative literal once `rem()` is right; it prints `0`. - Consecutive nested declarations rules, which arise when same-query `@media` blocks in one rule merge, are folded into one and count against the existing selector expansion limit as the `&{}` rules they replace did. #### Known and not fixed here - Four longhands are still folded into a `transition` the author did not write where the targets add a prefixed copy of a property (`mask-image` at Chrome 80): only the shorthand gives the copy its own times. - A later `background`, `box-shadow` or `border-*` declaration still loses to the `@supports` copy of an earlier `var()` value of the same property with a wide-gamut color. Same on `main`; #45560 is taken for the properties of the fallback handler and for unparsed values only. - A merged rule prints the unprefixed name in the passes of the rule it absorbed: `.d::-webkit-input-placeholder{}.d::-moz-placeholder{}` prints `::placeholder` for the second (#42106, not taken: it breaks the merge of prefix-equivalent rules). - The `border`, `outline` and `font` shorthands, `line-height`, `scroll-padding`, `background-size` and a few more still print a `calc()` that folds below zero as a negative literal. No real stylesheet has one. - `all` is not a property Bun knows, so `a{margin-top:1px;all:unset}` prints `all` first. Same on `main`. - A selector kept as raw tokens loses the blank before `*` when minified: `--sel: .dark *` prints `.dark*`. Same on `main`. - `width:calc(0% + 12px)` folds to `12px`, which lays out differently in a table cell. Same on `main`. - The `:dir()` / `:lang()` rules of a lowered logical property print before the nested rules of their rule, so a logical declaration written after a nested rule of equal specificity still loses to it. Same on `main`. - A fraction is still rounded to 6 significant digits (`12345.678` prints `12345.7`, `33.3333333333%` prints `33.3333%`). Keeping every digit would add up to 282 KB over the corpus. - `@keyframes "quoted"` is hashed in a module but not exported. - In a module, a keyframes name inside a `var()` fallback is not hashed: `animation: var(--enter, fade 1s)`. Same on `main`. - A `color-mix()` or relative color folded to a hex inside a gradient changes the interpolation from Oklab to sRGB (VS Code, clawhub). Printing `color(srgb ..)` instead needs a fallback for `mask-image`, `list-style-image` and custom properties at the default targets, which have none. - A comma list in `@container` still fails the build. - After a bare `:global`, names that are hashed at print time (`::view-transition-old(foo)`) are still hashed; inside `:global(...)` they are not. #### Pull requests in scope that do not land here - Duplicates of a pull request that lands here: #36961 (of #37979), #38478 (of #46662), #38480 (of #46733), #38482 (of #46715), #38491 (of #38500), #38508 (of #45201), #38513 (of #45362), #38573 (of #45487), #38640 (of #38500), #45525 (of #45247), #45589 (of #46756), #45970 (of #45971) - Already fixed on `main`: #38674 - Right, but for input nobody writes (0 of 10,116 stylesheets, and not new syntax): #31916, #38526, #38592, #39349, #41139, #41801, #42042, #42484, #43078, #45604, #45908, #46086, #46221, #46224, #46273, #46694, #46707, #46737, #46755, #46816, #46870, #46934 - Right or harmless, but not worth their code, their bigger output or the behavior they change: #30905, #31924, #33332, #34426, #36225, #38006, #38512, #38547, #38551, #38561, #38572, #38576, #38607, #38623, #38667, #38692, #42106, #42912, #42918, #43407, #43434, #44700, #45442, #45546, #45747, #45759, #45825, #45946, #46061, #46127, #46206 - Need a decision (see above), stay open: #40368, #40369, #44647, #44648, #45814 - Stays open: #45787 Out of scope: CSS linking and chunking in the bundler, the dev server, docs-only pull requests, and repository-wide sweeps that touch `src/css` in passing. Thanks to @mikamikasuki (#45247), @alii (#39520, #31999) and @oddharsh, whose measurements on #40369 and #41990 were used. Fixes #16727. Fixes #24770. Fixes #30772. Fixes #44704. Fixes #45089. Fixes #45237. Fixes #46872. ### How did you verify your code works? - The 35 CSS test files pass on a debug build, also with `FORCE_COLOR=1`. 600+ of the tests in this branch fail on a build of `main`. An earlier head of this branch passed all 179 CI jobs that ran. - The reproduction of each issue listed above was run on `main` (wrong) and on this branch (right). - Differential run over the 10,116 stylesheets, `main` against this branch, minified without targets and through `Bun.build` at the default targets: no stylesheet that builds on `main` fails; Chromium 156 compares the output with the input rule by rule (CSSOM, then computed values on a sized element in a standards-mode document, colors as painted pixels in sRGB and P3). The comparison has 195 control pairs that must read equal or different. For stylesheets that read differently on both builds, mostly because of lowering, the sets of differences were compared; the three new ones are two minifier fixtures that are wrong on both and one comparison artifact. - CSS modules are checked for consistency instead, since hashed names defeat the comparison: every reference points at a name the output defines, every export exists. - `Bun.color`: 59 inputs in 13 formats on both builds; every changed cell was read.
Problem
@mediarules that share a query is quadratic in their number: 1000 rules take 0.19s, 2000 take 0.77s, 4000 take 3.6s, 8000 take 20s (release 1.4.0,cssInternals.minifyTest). The same rules inside one@mediablock take 6ms at 4000.Nested CSS rules expand to more than 65536 selectors ...from 256 rules on under a two-selector parent, although the output only has 512 selectors. Fewer rules are needed under parents with more selectors.CssRule::Mediaarm ofCssRuleList::minify(src/css/rules/mod.rs:517on main) appends the new rule's body to the previous rule and then re-minifies the whole previous rule, so the body merged so far is minified once more per further rule. Each of those passes also runsStyleRule::charge_selector_expansionfor every rule in the body again, which is where the spurious limit error comes from.StyleRuleMergeState::pending_minifyin the same file); the@mediaarm was not covered.Fix
@mediamerge now only appends the body and sets a flag. The flag lives in the merge state css: minify merged duplicate rules in linear time #31920 added (renamedStyleRuleMergeState->MergeState, new fieldpending_at_rule_minifynext to the style rules'pending_minify), and one function,flush_pending_merges, settles whichever of the two is set: re-minify the style rule's declarations as before, or minify the at-rule's merged body.end_merge_run(flush + forgetlast_compat) is what every push into the output list calls first: the generic push inCssRuleList::minify, the style rule push and the extra-rule appends inminify_style_arm. The merged body is therefore minified once per run instead of once per merged rule, and its rules are charged against the expansion limit once.@mediarule still merges intorules.last()when that is a@mediarule with an equal query. Rules that minify away in between (an empty rule,@media not all {...}, ...) never got pushed and so never ended a run; they still do not, which is also why settling is tied to pushes rather than to the loop iteration.rulesgoes throughend_merge_run. A pending flag therefore always refers torules.last()(debug_asserted), and the two flags are mutually exclusive because they describe the same rule. Settling both kinds through one function means a push site cannot settle one and miss the other;flush_pending_style_mergeno longer exists, so the push site css: implement@keyframesminification #36224 adds in its@keyframesarm picks this up when it rebases instead of silently skipping the@mediaflush.color-schemehandler (duplicates its fallback vars per pass; css: emit the color-scheme fallback vars once when a block is minified again #38512 fixes the handler, this change reduces the copies meanwhile) and a few single-pass leftovers that extra passes happened to clean up (alab()fallback left before a latercolor: red, a duplicate rule created by selector partitioning). The output of a run no longer depends on how many rules it was split into; see the probe below.test/js/bun/css/duplicate-declaration-merge-hang.test.ts: a 6000-rule run spawned with a timeout (the unfixed debug build is killed at 60s; 51s for 2000 rules), the nested 1000-rule run that used to throw the expansion error and now matches the single-block output, and an output test for each way a run can end (end of list, style rule, at-rule that is minified,@keyframeswhich is not, different query, a style rule emitted only as its compiled fallback rules) plus dropped rules in between. The first two fail on the unfixed debug build, all pass with the fix. Debug ASAN timings with the fix: 2000 rules 0.24s, 4000 0.48s, 8000 0.97s.bun bd test test/js/bun/css/andtest/bundler/css/+test/bundler/esbuild/css.test.ts: unchanged (the only local failures are the fuzz tests' and one 20k-rule test's debug-build timeouts, identical without this change).test/js/bun/css/files/with no targets, chrome 80 and chrome 60/firefox 60/safari 10 targets: byte-identical. 800 generated stylesheets mixing same-query runs with dropped rules, nested rules,@keyframes, duplicates and logical properties: identical once thecolor-schemedeclarations and theSizeHandlerstate leak below are excluded, apart from the single-pass leftovers described above. The restructuring from the first revision of this PR (separate flag and flush function) to the current one produced byte-identical output over the same corpus.SizeHandler(margin/padding/inset) keeps its logical/physicalcategoryacross declaration blocks, so after any logical property later rules keep redundant shorthands. It is independent of this change (.a{margin:1px}.a{margin:0}@media not all{div{margin-inline-start:2px}}already shows it through the style-rule merge on main), but it is what makes the order in which sibling rules are minified visible at all.@supportsrules instead of dropping the duplicate) currently re-minifies per merge, i.e. would add this shape back one arm over; on top of this PR it becomesappend+pending_at_rule_minify = trueplus aCssRule::Supportsarm inflush_pending_merges(SupportsRule::minifyhas the same signature).Background
CssRuleList::minifywalks a rule list and builds a newrulesvector. Style rules try to merge intorules.last();@mediarules with the same query asrules.last()are merged into it; rules that minify to nothing are simply not pushed. Because merging always targets the last pushed rule, a rule that is dropped between two mergeable rules does not separate them.@mediabody has to be minified as a whole after its parts were appended; the question is only how often.StyleRule::minifyaddsparent selectors x own selectorsto a running total inMinifyContextand fails past 65536. The total is never decremented, so minifying the same rule twice charges it twice.Probe: output depends on the run length before, not after
chrome 80 targets,
cssInternals.minifyTest:...{color:#b32323;color:red}...{color:#b32323;color:red}...{color:red}...{color:#b32323;color:red}...{color:red}...{color:#b32323;color:red}On main, appending an empty same-query block changes the output because it triggers one more pass over the body.
Timings
nadjacent@media (min-width:1px){.aN{margin:Npx}}rules:First revision of this PR
The first push kept the
@mediaflag as a separatepending_media_minifybool threaded intominify_style_arm, with its ownflush_pending_media_mergecalled next toflush_pending_style_mergeat each push site. Review pointed out that this leaves every present and future push site with two separate obligations (and #36224's new push site would have met only one of them), so the flag was folded into the shared merge state and both kinds are settled throughflush_pending_merges/end_merge_run. Output is identical between the two revisions.[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file