css: keep !important in the rules generated for logical properties and color-scheme

[robobun]

Problem

• When a logical inline property is declared with !important and the targets require compiling logical properties away, the generated direction rules drop the !important. .foo { margin-inline-start: 2px !important } with safari: 12 comes out as .foo:not(:lang(...)) { margin-left: 2px } and .foo:lang(...) { margin-right: 2px }.
• The compiled output therefore loses to any rule setting the physical property with !important, and the rtl rule also loses to a later plain rule of higher specificity, while the source declaration would have won. Block-axis properties are unaffected because they are compiled into the original rule, which keeps the importance.
• Same class: color-scheme: light dark !important emits the light variables with !important in the rule itself, but the @media (prefers-color-scheme: dark) override without it, so the dark override can never apply.
• The @supports fallbacks split out of the generated rules inherit the same loss, since they are derived from declarations that were already staged as normal ones.
• Cause: PropertyHandlerContext (src/css/context.rs) stages the ltr / rtl / dark declarations as a single Vec<Property> each and builds the generated rules with an empty important_declarations list. The @supports entries next to them already keep two lists and honor is_important.
• Second half of the cause: the handlers stage most of these rules while flushing in finalize(), and DeclarationBlock::minify (src/css/declaration.rs) left context.is_important at whatever the last handled list set, so the important handler was finalized with the flag describing the normal declarations whenever the rule had any.
• Upstream lightningcss 1.30.2 produces the same output for these inputs, so this is inherited rather than a porting error; the expected output follows from the cascade.

Fix

• StagedDeclarations holds a normal and an important list; ltr, rtl, dark and the @supports entries all use it, add_logical_rule / add_dark_rule / add_conditional_property route on is_important, and one clone_block helper builds every generated rule's DeclarationBlock from both lists.
• DeclarationBlock::minify sets is_important explicitly before finalizing each handler, so declarations staged during finalize() are attributed to the handler that staged them.
• Correct because each generated rule is a rewrite of the source declarations under a narrower selector (or media query); keeping each declaration's importance is what makes it rank the same way in the cascade as the declaration it replaces. The generated rules are minified again after this, and DeclarationBlock::minify already keeps the two lists apart, so !important flows through to their @supports fallbacks as well.
• No change for declarations that are not important: they still land in declarations, and reset() and the pending-merge check in rules/mod.rs (which calls is_empty() on the staged lists) see both lists.
• Tests: test/js/bun/css/css.test.ts, describe("!important in generated fallback rules"), one case per staging site (margin/padding/inset handler, border, border-radius, transition, color-scheme), plus a mixed important/normal rule, an important declaration followed by a normal one (the stale flag case), an unparsed var() + lab() value whose @supports fallback must stay important, and a block-axis case as the unchanged contrast. 9 of the 10 fail on the current build, all pass with the fix.
• bun bd test test/js/bun/css/ passes apart from six pre-existing timeouts (css-fuzz and two subprocess tests) that fail identically on a build without this diff in the same container; test/bundler/css/ and the css regression tests pass; cargo clippy -p bun_css is clean.

Background

• Compiling logical properties: for targets that lack e.g. margin-inline-start, the minifier rewrites the declaration into physical properties. Block-axis properties map to a fixed physical side and stay in the rule. Inline-axis properties depend on the text direction, so the minifier removes them from the rule and emits two extra rules, one with :dir(ltr) and one with :dir(rtl) appended to the selectors (later compiled to :lang() lists), each holding the physical declaration for that direction. color-scheme works the same way, emitting a @media (prefers-color-scheme: dark) rule that overrides the variables set in the main rule.
• PropertyHandlerContext is where the property handlers stage these extra declarations while a rule's declarations are minified; minify_style_arm in rules/mod.rs turns them into rules after the rule is done and then calls reset().
• DeclarationBlock stores normal and !important declarations as two separate lists instead of a flag per declaration, and minify runs two independent sets of handlers over them (handler and important_handler). context.is_important is the only thing telling the context which set is currently running.
• finalize() is the end-of-block flush of a handler: handlers such as margin buffer the sides they have seen and only decide what to emit (and what to stage) once the whole block has been handled.

Output before and after for the repro

.foo { margin-inline-start: 2px !important; margin-inline-end: 4px; }   (targets: safari 12)

before:
.foo:not(:lang(...)) { margin-left: 2px; margin-right: 4px; }
.foo:lang(...)       { margin-left: 4px; margin-right: 2px; }

after:
.foo:not(:lang(...)) { margin-right: 4px; margin-left: 2px !important; }
.foo:lang(...)       { margin-left: 4px; margin-right: 2px !important; }

.foo { color-scheme: light dark !important; }   (targets: chrome 90)

before:
@media (prefers-color-scheme: dark) { .foo { --buncss-light: ; --buncss-dark: initial; } }

after:
@media (prefers-color-scheme: dark) { .foo { --buncss-light: !important; --buncss-dark: initial !important; } }

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0a07cdce-a478-44d8-a219-3f7206156e7d

📥 Commits

Reviewing files that changed from the base of the PR and between eabb96d and 4a8479f.

📒 Files selected for processing (3)

• src/css/context.rs
• src/css/declaration.rs
• test/js/bun/css/css.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: diff is ready; CI build #96261 is green on every lane that ran.

• 177 jobs passed, including test/js/bun/css/css.test.ts on all platforms. The build is marked failed only because the two darwin 14 aarch64 - test-bun shards never got an agent and expired three times without starting; every build on this pipeline since about 14:35 UTC has the same expired darwin jobs, so retrying those two jobs once the pool catches up is all that is needed.
• Reproduced on the released build and on a debug build of main with css.test.ts -t "generated fallback" (9 of the 10 new cases fail there; all pass with this branch). Upstream lightningcss 1.30.2 emits the same non-important output for these inputs, so the expectation is taken from the cascade rather than from upstream.

[claude]

LGTM — focused fix that routes staged logical/color-scheme fallback declarations into the right importance list, with the finalize-time is_important flag now set per handler.

What was reviewed:

• Confirmed is_important is only read inside context.rs (staging into ltr/rtl/dark/supports), so setting it before each finalize() cannot affect anything else.
• The one external consumer of the retyped fields — the pending-merge check at src/css/rules/mod.rs:767-769 — now dispatches to StagedDeclarations::is_empty(), which checks both lists.
• reset() clears both lists; SupportsEntry refactor is behavior-preserving (it already split by importance).
• Tests cover every add_logical_rule/add_dark_rule call site (margin/padding/inset, border, border-radius, transition, color-scheme) plus the mixed and stale-flag cases.

Extended reasoning...

Overview

The PR fixes a CSS minifier bug where !important was dropped from the extra rules generated when compiling away logical inline properties (margin-inline-start, border-inline-*, inset-inline-*, border-*-*-radius, transition-property referencing them) and color-scheme. Two changes:

1. src/css/context.rs: introduce StagedDeclarations { declarations, important_declarations } and use it for ltr, rtl, dark, and SupportsEntry. add_logical_rule / add_dark_rule now route on self.is_important, and a shared clone_block builds each generated rule's DeclarationBlock from both lists (previously important_declarations was left empty).
2. src/css/declaration.rs: set context.is_important explicitly before each finalize() call, so declarations staged during flush inherit the importance of the handler that owns them rather than whatever the last-processed list left behind.

Tests add a describe("!important in generated fallback rules") block with 10 prefix_test cases, one per staging site plus the mixed, stale-flag, @supports-fallback, and unchanged block-axis cases.

Security risks

None. Pure CSS-output correctness in the minifier; no untrusted-input parsing changes, no allocation-size logic, no FFI.

Level of scrutiny

Medium. It changes shared minifier plumbing (PropertyHandlerContext), so I traced every reader of the retyped fields and every reader of is_important:

• is_important is set only in declaration.rs and read only in context.rs (for staging), so the extra assignments around finalize() cannot leak into any other behavior.
• ltr/rtl/dark are read only inside context.rs and at rules/mod.rs:767-769 via .is_empty(); StagedDeclarations::is_empty() is pub(crate) and covers both lists, so the pending-merge check is unchanged for non-important input and correctly non-empty for important-only input.
• SupportsEntry already tracked both lists; the refactor just unifies the shape and clone_block reproduces the exact prior mapping.
• Hoisting ctx.is_important = important out of the per-declaration loop is a no-op because no property handler mutates the flag (grep-verified), and the value is overwritten before both finalize() calls anyway.

Other factors

• The fix is at the right layer — the shared staging context — rather than in each of the six handlers, matching the "fix the whole class" guidance.
• Test coverage maps directly onto every add_logical_rule / add_dark_rule call site found by grep (margin_padding, border, border_radius, transition, ui), plus the interaction with add_conditional_property via the var() + lab() case.
• Non-important declarations are unaffected: they land in .declarations as before, and reset()/is_empty() see both lists.
• The PR description confirms 9/10 new tests fail on main and all pass with the fix, and that upstream lightningcss shares the bug (so the expected output is derived from cascade semantics, not upstream behavior).

[robobun]

This also reproduces in a plain bun build, with the default browser targets. An old target is not necessary.

printf '.a { border-start-start-radius: 3px !important }' > a.css
bun build --minify a.css

Bun 1.4.3 prints six rules. None of them has !important:

.a:not(:-webkit-any(:lang(ae),...,:lang(yi))){border-top-left-radius:3px}
.a:not(:-moz-any(...)){border-top-left-radius:3px}
.a:not(:is(...)){border-top-left-radius:3px}
.a:-webkit-any(...){border-top-right-radius:3px}
.a:-moz-any(...){border-top-right-radius:3px}
.a:is(...){border-top-right-radius:3px}

The default floor is chrome 80, edge 80, firefox 78, safari 14 (BROWSER_DEFAULT in src/css/targets.rs). At that floor the minifier generates direction rules for these properties, and each one loses !important:

• The four logical corner radii (LogicalBorderRadius needs chrome 89 and safari 15).
• inset-inline, inset-inline-start and inset-inline-end (LogicalInset needs chrome 87 and safari 14.1).

margin-inline-start and the margin-inline, padding-inline and border-inline shorthands stay logical at that floor, so they keep !important.

#40369 raises the default floor. At its floor the minifier compiles no logical property away, so a default build no longer reaches this code. Explicit lower targets (#40368) still reach it.