Repository navigation
Conversation
…osition
Declarations that followed a nested rule were parsed into the enclosing
style rule's own declaration block, which hoisted them above the nested
rules they were written after. `a { color: red; & { color: blue } color:
green }` therefore printed the `green` block before the `blue` one and
rendered blue. Declarations directly inside a nested conditional at-rule
were wrapped in a `& { }` rule, which also changed their specificity for
selector lists.
Parse both into a new CssRule::NestedDeclarations rule that keeps its
position among the nested rules (https://drafts.csswg.org/css-nesting/#nested-declarations-rule).
When nesting is compiled away it prints as a rule under the enclosing
rule's selectors (repeated as written, not wrapped in :is()); when nesting
is preserved it prints its declarations in place, keeping the trailing `;`
whenever more output follows it in the block, including through an
unwrapped `@media all`. Minifying it runs the declarations through the
property handlers and inserts the logical property, @supports and dark
color scheme fallback rules they generate right after it, nested under
`&`, so those fallbacks keep their position too. The declarations still
count towards the CSS modules `composes` property usage of the rule.
|
Warning Review limit reached
Next review available in: 10 seconds 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 (10)
Comment |
|
Status: ready for review; the diff is green on every lane that ran. The red build status is the macOS 14 arm64 test lane never getting an agent (details below), not a test failure. Reproduced on the released build with Tests: test/js/bun/css/nested-declarations.test.ts (27 of its 30 cases fail on the released build, all pass here) and the new CI on the current head (e120c58, build 96184): 177 of 179 jobs passed, with only retried flakes in unrelated tests (child_process IPC, crypto/vm/inspect leak tests, watch mode, install registry, terminal, napi hello world). The build is marked failed because its two Review follow-ups: the undrained stdout pipe in the Two adjacent pre-existing bugs found on the way are tracked separately and are not changed here: #38520 ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — the follow-up commits address both the earlier stdout-drain nit (2c66fff now asserts the bundled stylesheet on stdout) and the comment-cop lints (e120c58 trims the added comments to one line and names write_declarations' bool flags). I re-reviewed and didn't find any bugs. Because this threads a new CssRule variant through the parser, minifier, and printer, a human look would still be worthwhile.
What was reviewed:
parse_value's pop/push of the trailingNestedDeclarationsrule — checked that a failed declaration retried as a nested rule leaves no empty rule behind, and that consecutive declarations coalesce into one rule.write_declarationsrefactor vs. the old inline loop — thecomposeshandling, trailing-;condition, andicounter (still skipped for consumedcomposes) match the previous behaviour;handle_composeslosing its unused params is safe (it never read them).more_rules_follow/to_css_unwrapped— verified the@media allunwrap is the only caller and the trailing-;tests cover the case lightningcss gets wrong.charge_selector_expansionmove toMinifyContext— body is byte-identical to the oldStyleRulemethod.
Extended reasoning...
Overview
This PR fixes a cascade-order bug in Bun's CSS bundler: declarations written after a nested rule were being hoisted into the enclosing rule's leading declaration block, so they printed before the nested rule instead of after it. The fix adds a new CssRule::NestedDeclarations variant (mirroring lightningcss's nested-declarations rule) and threads it through parsing (NestedRuleParser::parse_value, parse_style_block), minifying (minify_nested_declarations_arm, NestedDeclarationsRule::minify), and printing (NestedDeclarationsRule::to_css, a new Printer::more_rules_follow flag, and a shared write_declarations helper factored out of StyleRule::to_css_base). charge_selector_expansion moves from StyleRule to MinifyContext so both callers share it, and CssModule::handle_composes drops three parameters it never read. 30 new test cases plus one itBundled case cover both nesting-preserved and nesting-compiled modes, pretty printing, fallback-rule placement, vendor-prefix passes, and bun build end to end.
Security risks
None. This is CSS AST transformation and serialization; no untrusted-length arithmetic, no new syscalls, no auth/crypto/permissions surface. The one unsafe addition (detach_lifetime_ref on the parser arena in parse_value) is the same pattern already used two lines above in the pre-existing code and is covered by the existing SAFETY comment.
Level of scrutiny
Medium-high. The CSS bundler runs on every bun build of a stylesheet, and this touches the parser, the minify dispatch, the printer, and the CssRule variant table simultaneously — a mistake in any one of those paths would corrupt output for any stylesheet using CSS nesting. The change is well-scoped (each piece is small and follows the existing sibling-variant patterns closely), thoroughly tested (27 of 30 new cases fail on the released build and pass here; a 40-input differential run against lightningcss 1.30.2 matched), and the PR description explains the mechanism precisely. But a new enum variant plus printer state (more_rules_follow) is the kind of cross-cutting change a maintainer familiar with the lightningcss port should sanity-check.
Other factors
- My prior review's only finding (undrained
stdoutpipe in thebun buildtest) was addressed in 2c66fff, which now also asserts the bundled stylesheet on stdout — a stronger test than before. - The comment-cop bot flagged multi-line comments across seven files; e120c58 trimmed every added comment to one line and replaced the two positional bools on
write_declarationswith named args (newline_before_first,trailing_semicolon), so those lints no longer apply to the current diff even though the inline threads aren't marked resolved. - I spot-checked the
write_declarationsextraction against the old inline loop: theicounter is still only incremented for printed declarations (so a consumedcomposesdoesn't throw off the last-declaration;check), and thehandle_composessimplification is safe because the removed params were genuinely unused. - The
charge_selector_expansionmove is a pure hoist — the body is identical, and both call sites pass what the old inline code read. - Two adjacent pre-existing bugs (#38520, #38531, and the
composes_refsclearing) are explicitly called out as tracked separately and left unchanged here.
|
Both review items are in: 2c66fff fixes the undrained stdout pipe in the |
|
This PR also fixes a second report: declarations directly inside a nested at-rule, under a selector list that has a pseudo-element. *, ::before, ::after { @media (prefers-reduced-motion: reduce) { animation: none } }
I did not open a second PR for that report. One gap of the same kind is left here. .a::before, .b { @media screen { color: var(--x, lab(50% 20 30)) } }With Chrome 95 targets this prints Branch
|
|
Closing: superseded by #45462. It has the same nested declarations rule on current main. It builds the fallback rules on the selectors of the enclosing rule, and it lists what it gives up against this PR. |
Problem
Declarations written after a nested rule are hoisted above it, changing the cascade.
bun build in.css --minifyonprints
a{color:green}a{color:#00f}.c{color:#000}@media (min-width:600px){.c{color:pink}}, soarenders blue and.cpink on wide viewports; the source means green and always black.--target=bun(nesting preserved) has the same problem:a{color:green;&{color:#00f}}.Cause:
NestedRuleParser::parse_value(src/css/css_parser.rs) appends every declaration to the block's leading declaration list regardless of what was parsed before it, andStyleRuleprints that list before its nested rules. The port predates lightningcss's nested declarations rule.parse_style_blockseparately wraps declarations found directly inside a nested@media/@supports/... in a& { }rule, which for a selector list compiles to:is(a, b)and so also changes their specificity.Fix
CssRule::NestedDeclarations(NestedDeclarationsRule)(src/css/rules/nesting.rs).parse_valueparses a declaration that follows a nested rule into the nested declarations rule at the end of the block's rule list (creating one unless the previous declaration already did, and not leaving an empty one behind when the declaration fails to parse andRuleBodyParserretries it as a nested rule such asdiv:hover {).parse_style_blockuses the same rule for declarations directly inside a nested conditional at-rule instead of the& { }wrapper.:is()so each selector keeps its specificity). When nesting is preserved, it prints its declarations in place among the nested rules.CssRuleList::to_cssnow records inPrinter::more_rules_followwhether anything follows the rule being printed, so the last declaration keeps its;exactly when needed, including when an always-true@media allis unwrapped into the enclosing block (lightningcss 1.30 emitscolor:redcolor:#00fthere; see the "trailing semicolon" tests). The declaration printing loop ofStyleRule::to_css_baseis factored intowrite_declarationsand shared, socomposesis consumed in CSS modules the same way in both places;CssModule::handle_composesloses three parameters it never read.minify_nested_declarations_armin src/css/rules/mod.rs) runs the declarations through the property handlers and inserts the fallback rules they stage (logical properties,@supportscolor fallbacks,prefers-color-schemeforcolor-scheme) right after the rule, built under a&selector the same wayminify_style_armbuilds them under a style rule's own selectors, so e.g. a trailinginset-inline-endkeeps working with the default browser targets. Otherwise the rule gets the same bookkeeping as the other non-style rules (ends the merge run, dedup barrier, dropped when the enclosing rule is unused), and is charged one selector against the expansion budget, which moved fromStyleRuletoMinifyContext::charge_selector_expansionto be shared.parse_blockstill counts the trailing declarations towards the rule's property usage for the CSS modulescomposesconflict check. (That check is currently defeated for any rule containing a nested rule by a pre-existing bug in how nested rules clearcomposes_refs; handed off separately, with css modules: keep composes working after a nested rule in the same block #38520 covering the relatedcomposes_stateclobbering.)bun buildfor both targets: 27 fail on the released build, all pass with this change) and theDeclarationsAfterNestedRulecase in test/bundler/css/css-modules.test.ts (fails before, passes after).--target=bunoutput, exercising re-parsing of the in-place form), test/bundler/esbuild/css.test.ts, test/bundler/css/, test/regression/issue/25794.test.ts,cargo clippy -p bun_css; and a differential run of 40 nesting inputs against lightningcss 1.30.2 in both modes: identical except for pre-existing unrelated printing differences and the two@media allcases above where Bun's output is the valid one.Background
@mediado the same. Browsers ship this (Chrome 130, Firefox 132, Safari 18.2), so source order is meaningful and a build tool must preserve it.--target=browserthe default targets predate nesting, so nesting is compiled away: nested rules are printed after their parent with&replaced by the parent selectors, which the printer tracks throughPrinter::ctx/StyleContext(set only in that mode, which is howNestedDeclarationsRule::to_cssand the existing@nestprinter tell the modes apart). With--target=bun/nodenesting is printed as written.DeclarationBlock::minify) may consume a declaration and stage replacement rules inPropertyHandlerContext(e.g.inset-inline-endbecomes:dir(ltr)/:dir(rtl)rules); the caller that minified the block has to turn those into rules and place them after it, which previously onlyminify_style_armdid.Output before / after for the example above
lightningcss 1.30.2 produces the "after" output in both modes.
[review] gate passed · iteration 0 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file