[robobun]

Problem

• When logical properties are compiled away for the targets, a margin/padding shorthand that overrides an earlier inline logical longhand in the same rule still gets that dead longhand compiled into :lang() fallback rules. Those rules are emitted after the rule, so they win the cascade and the output no longer matches the source:

  .a{margin-inline-start:3px;margin:0}
  -> .a{margin:0} .a:not(:lang(ae), ...){margin-left:3px} ... .a:lang(ae), ...{margin-right:3px}
  

  The source gives every side 0; the output gives LTR pages margin-left: 3px. Same for margin-inline-end, margin-inline, unparsed margin-inline-start: var(--x), and the padding family.
• With logical properties supported (the bundler's fixed browser targets, or --target bun), the same inputs are only under-minified: bun build --minify keeps .a{margin-inline-start:3px;margin:0} as written for all four families (margin, padding, inset, scroll-margin). Targets old enough to compile logical properties are not reachable from the bun build CLI today (Targets::for_bundler_target is fixed), so the wrong-cascade output above shows up through the internal test API that test/js/bun/css uses; the under-minification is what users see.
• Cause: SizeHandler::handle_property, S::SHORTHAND arm (src/css/properties/margin_padding.rs:677). It called flush_helper_physical for the four sides, and that helper flushes the whole buffer whenever the category changes (margin_padding.rs:747). After a logical longhand the buffer is Logical and the shorthand is Physical, so the buffered longhands were flushed, and for targets without logical properties flush() turns inline ones into context.add_logical_rule(...) rules (margin_padding.rs:893). Right after that the arm cleared the four logical slots anyway: the handler already knew they were overridden, it had just emitted them first.
• Upstream lightningcss 1.30.2 produces the same output for these inputs (its side_handler! shorthand arm has the same shape), so this is inherited rather than a porting error. Its inset handler, whose shorthand shares the logical category, already drops the overridden longhands (inset-inline-start:3px;inset:0 -> inset:0), which is the behavior this change gives the other families.

Fix

• The shorthand arm no longer flushes on a category change. It flushes in exactly one case: when one of the shorthand's four values is not supported by the targets (is_compatible), because such a target rejects the whole declaration and everything buffered before it, physical or logical, still applies there. Otherwise the shorthand replaces whatever is buffered, and the arm records the shorthand's category like every other buffering arm does.
• Why this is correct: the 4-side shorthand sets all four physical longhands, and each logical longhand resolves to one of those four in every writing mode, so nothing declared before the shorthand in the same block can take effect in a browser that applies the shorthand. The only browser where it still matters is one that rejects the shorthand's value, which is the fallback condition the arm keeps. A 2-side logical shorthand (margin-inline) is not treated this way because the sides it covers depend on the writing mode; only the 4-side shorthand arm changes.
• One physical-side detail changes along the way (raised in review): the old per-side check only kept a buffered margin-top if the shorthand's own top value was unsupported, so margin-top:10px;margin:5px max(2cqw,22px) lost its fallback for a target without container query units even though that target rejects the whole margin declaration. Checking the four values once keeps it; this is the only output change for physical-only input, and it only affects shorthands with a value some target rejects.
• Verified with test/js/bun/css/css.test.ts, new "logical longhands overridden by a 4-side shorthand" block: margin and padding with the longhand compiled (safari 8) and supported (safari 15) for inline-start, inline-end, *-inline, block-start, an unparsed longhand, and a mixed pair; the fallback cases (a logical longhand and a physical one before 5px max(2cqw, 22px), kept with safari 14, dropped with safari 16); a longhand declared after the shorthand staying; and minify cases for all four families, including !important, a longhand folding into the shorthand afterwards, and the three inputs that must stay as written. 44 of the 62 cases fail on bun 1.4.0 (USE_SYSTEM_BUN=1), all 62 pass with the fix.
• bun bd test test/js/bun/css/css.test.ts: 1204 pass; test/bundler/css/, test/bundler/esbuild/css.test.ts, doesnt_crash, css-loader: 288 pass. bun build --minify on the first snippet now prints .a{margin:0}.
• Overlap with css: record the category when a margin, padding, inset or scroll-margin shorthand replaces the buffered values #38551: that PR adds the same self.category = S::SHORTHAND_CATEGORY line to this arm and pins three intra-block outputs that this change improves (.a{margin-inline-start:1px;margin:0} becomes .a{margin:0}, ...;margin:0;margin-top:5px} becomes .a{margin:5px 0 0}, ...;margin:0;margin-inline-end:2px} becomes .a{margin:0;margin-inline-end:2px}). Whichever lands second needs those three expectations updated.
• Out of scope, reported separately: border-width/border-style/border-color after a logical border longhand have the same shape in border.rs, and a physical margin-left after a compiled margin-inline-start in the same rule also loses to the appended rules (that one needs the flush to emit only one of the two :lang() halves).

Background

• The minifier runs each declaration block through property handlers. SizeHandler (one instantiation each for margin, padding, inset and scroll-margin) buffers the block's declarations of its family and writes them out in flush(), merged into a shorthand when possible. A declaration for a slot that is already buffered simply replaces it; that is how duplicates disappear and how margin:0;margin-top:5px becomes margin:5px 0 0.
• Logical properties (margin-inline-start, ...) resolve to a physical side based on the element's writing mode. The handler cannot merge them with physical values, so it buffers one category at a time and flushes the buffered category when the other one arrives, which preserves source order between two groups that are both live. The 4-side shorthand is the one declaration that overrides both groups at once, which is why that flush is unnecessary for it.
• Flushing is also used for fallbacks: if a new value uses syntax some target does not support (max(), container query units), the value it replaces is flushed first so that browser still gets the old declaration. is_compatible against the targets is that check, and it is the only flush condition the shorthand arm keeps.
• When the targets lack logical properties, flush() compiles block longhands into margin-top/margin-bottom inside the same rule, but inline ones depend on text direction, so context.add_logical_rule() emits them as separate :lang()-selected rules placed after the current rule. Anything emitted that way outranks the rule itself, which is why flushing a dead inline longhand changes the result while flushing a dead block longhand does not.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 36 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: 956e52e5-e660-4cb2-8003-bc4e2882f2f6

📥 Commits

Reviewing files that changed from the base of the PR and between eabb96d and 55ffa5d.

📒 Files selected for processing (2)

• src/css/properties/margin_padding.rs
• test/js/bun/css/css.test.ts

---

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

[robobun]

Status: reproduced and fixed, waiting for CI.

• Reproduced on bun 1.4.0 and main (eabb96de72) with the internal CSS test API: .a{margin-inline-start:3px;margin:0} compiled for targets without logical properties emits :lang() rules carrying margin-left: 3px after the rule, and bun build --minify keeps the dead margin-inline-start declaration. Upstream lightningcss 1.30.2 gives the same output.
• Fix: src/css/properties/margin_padding.rs, SizeHandler shorthand arm. Tests: test/js/bun/css/css.test.ts, "logical longhands overridden by a 4-side shorthand" (44 of 62 cases fail without the fix).
• Review follow-ups: the fallback check now looks at all four values of the shorthand (af75afd, with tests); the explanatory comment in the arm was cut down to single lines (55ffa5d), the reasoning lives in the PR description.
• Related, not covered here: css: record the category when a margin, padding, inset or scroll-margin shorthand replaces the buffered values #38551 (records the category only; three of its pinned expectations change with this PR, noted in the description).

[claude]

LGTM. The inline note is a pre-existing upstream-inherited edge case the PR preserves verbatim, not something it introduces — fine as a follow-up.

Extended reasoning...

Overview

The PR rewrites one arm of SizeHandler::handle_property in src/css/properties/margin_padding.rs — the S::SHORTHAND arm that handles 4-side margin/padding/inset/scroll-margin shorthands. Previously it called flush_helper_physical per side, which flushed the whole buffer on any Logical→Physical category change; the rewrite drops the category-change flush and keeps only the per-target compatibility fallback flush, then records self.category = S::SHORTHAND_CATEGORY. 58 new test cases in test/js/bun/css/css.test.ts cover both families × compiled/supported targets × six longhand shapes, the fallback boundary (safari 14 vs 16 with max(2cqw, ...)), longhand-after-shorthand, and minify-only cases for all four families including the negative contracts.

Security risks

None. Pure CSS minifier output logic; no untrusted input parsing changed, no allocation/lifetime changes, no FFI.

Level of scrutiny

Medium. It changes minifier output semantics and intentionally diverges from upstream lightningcss, but the correctness argument is a one-liner of CSS spec: a 4-side shorthand sets all four physical longhands, and every logical margin/padding longhand resolves to one of those four in every writing mode, so an earlier logical longhand in the same block is dead in any browser that applies the shorthand. The only case where it isn't dead — a target that rejects the shorthand's value — is the retained fallback condition. The change is ~30 lines localized to one match arm.

Other factors

• flush_helper_physical remains live via property_helper; nothing became dead.
• The new self.category = S::SHORTHAND_CATEGORY line is what makes {inline-start:2px; margin:0; margin-top:5px} fold into margin:5px 0 0 (covered by a test), and the PR description flags the overlap with #38551.
• The one finding — per-side physical fallback missing shorthand parse atomicity — is the exact behavior flush_helper_physical had before this change (and upstream lightningcss still has); the PR neither introduced nor worsened it.
• No CODEOWNERS entry for src/css/. Test coverage is thorough on the variant matrix and includes the negative cases (2-side logical shorthand does not override physical, unparsed var() shorthand keeps the fallback).

[claude]

LGTM — the follow-up in af75afd addresses the per-side fallback gap I raised, and the final shorthand arm is simpler than what I suggested (unconditional flush() when any side is incompatible; has_any already makes that a no-op with an empty buffer).

Checked: the 4-side-overrides-all-logical reasoning holds for every writing mode; context.targets.browsers = None skips the check as before; flush_helper_physical is still live via property_helper; self.category is now recorded so a following physical longhand doesn't spuriously flush. The two in-source comments are single-line, so the comment-cop flags are on stale commits.

Extended reasoning...

Overview

Rewrites the S::SHORTHAND arm of SizeHandler::handle_property in src/css/properties/margin_padding.rs (~20 lines). Instead of calling flush_helper_physical per side — which flushed the whole buffer on any Logical→Physical category change and emitted dead inline logical longhands as post-rule :lang() fallbacks that then won the cascade — the arm now flushes only when one of the shorthand's four values is incompatible with a target, then unconditionally overwrites the physical slots, clears the logical slots, and records the shorthand's category. Adds a 62-case test block in test/js/bun/css/css.test.ts covering margin/padding × compiled/supported targets × six longhand shapes, the Safari 14/16 fallback split (both logical and the physical case I raised), a post-shorthand longhand, and minify cases across all four families.

Security risks

None. Pure CSS minifier output logic; no untrusted input parsing changes, no memory-unsafe code, no FFI.

Level of scrutiny

Medium. It's a small, localized change to one match arm, but it deliberately diverges from upstream lightningcss (which has the same bug). The PR description lays out the CSS-spec justification — a 4-side shorthand sets all four physical longhands, and every logical longhand resolves to one of those four in every writing mode, so anything buffered before it is dead unless a target rejects the shorthand's value — and that reasoning is correct. The wrong-cascade output is only reachable via the internal test API today (bundler targets are fixed), so user-facing risk is limited to under-minification.

Other factors

• My earlier inline comment (per-side physical fallback check drops margin-top when the incompatible value lands on a different side) was taken in af75afd and is now covered by the ${top}: 10px Safari 14/16 test pair; the thread is resolved.
• The two comment-cop bot flags are on the pre-55ffa5dd revisions; the final diff's two comments are single-line.
• flush_helper_physical is not dead — property_helper still calls it for individual physical longhands.
• The if let Some(browsers) gate preserves the old no-targets behavior (no compatibility check, just overwrite).
• Full CSS suite (1204) and bundler CSS suites (288) reported passing; 44/62 new cases fail on system bun.
• Known overlap with #38551 is called out with the three expectations that will need updating in whichever lands second.

[Jarred-Sumner]

Not taken in #47025 (7942008), which consolidated the open CSS parser pull requests. It is right or harmless, but not worth its code, the bigger output or the behavior it changes.