[robobun]

Problem

• The CSS minifier (bun build --minify, Bun.build) leaves scroll-padding declarations exactly as written (release 1.4.0 and main): duplicates stay and longhands are never merged, while every sibling family is minified.

  .a{scroll-padding:1px;scroll-padding:0}                                  ->  unchanged   (expected .a{scroll-padding:0})
  .a{scroll-padding-top:1px;scroll-padding-right:1px;
     scroll-padding-bottom:1px;scroll-padding-left:1px}                    ->  unchanged   (expected .a{scroll-padding:1px})
  .a{scroll-margin:1px;scroll-margin:0}                                    ->  .a{scroll-margin:0}
  

• Cause: merging for this kind of property is done by SizeHandler in src/css/properties/margin_padding.rs, which is instantiated for margin, padding, scroll-margin and inset (margin_padding.rs:267) but not for scroll-padding; DeclarationHandler in src/css/declaration.rs:390 accordingly has no scroll_padding handler, so the declarations fall through every handler and are copied to the output. The ScrollPadding, ScrollPaddingBlock and ScrollPaddingInline value types and all the Property variants already exist (the properties parse and print fine); only the handler is missing. The removed .zig sources had the same four handlers, so this was never implemented rather than lost in the port.

Fix

• Adds ScrollPaddingSpec and pub type ScrollPaddingHandler = SizeHandler<ScrollPaddingSpec>, and runs it from DeclarationHandler (field, handle_property chain, finalize, new) directly after scroll_margin.
• The spec is ScrollMarginSpec with the scroll-padding variants substituted: physical shorthand category, and no logical compat feature (FEATURE / SHORTHAND_FEATURE = None), so the logical scroll-padding properties are never rewritten to physical ones for old targets. This is the configuration upstream lightningcss uses for its ScrollPaddingHandler. The handler also sits where upstream puts it (right after scroll-margin), so a block mixing several families emits its merged declarations in the same order as upstream.
• Checked against the lightningcss 1.30.2 binary in the repo's node_modules: 67 scroll-padding inputs (duplicates, every merge shape, physical/logical mixes, var() values, !important, multiple rules, interleaving with other families), each run with no targets, safari 8 and chrome 90, produce byte-identical output from this branch and upstream. 34 of them differ on the released build.
• Verified: test/js/bun/css/css.test.ts, "scroll-padding" block (the old scroll-paddding block, which held one prefix test): 18 of the 25 cases fail on the released build and all pass with the fix; the remaining 7 pin behaviour that must stay the same with the handler in place (a var() value keeps the earlier declaration as a fallback, switching between physical and logical properties flushes instead of merging, logical properties survive old targets). The whole file (1166 tests) and doesnt_crash, duplicate-declaration-merge-hang and css-loader pass; cargo clippy -p bun_css is clean.
• css: record the category when a margin, padding, inset or scroll-margin shorthand replaces the buffered values #38551 fixes the shorthand arm of the shared SizeHandler body; the new instantiation picks that up automatically, and nothing here depends on it.
• Known limitation shared with every buffered family: DeclarationHandler has no arm for the all shorthand, so buffered declarations are written out after a later all: (.a{margin-top:1px;all:unset} already minifies to .a{all:unset;margin-top:1px} on the released build, where upstream emits .a{all:unset}). With this change scroll-padding behaves like margin here, where before it was copied through in source order. The comparison above therefore excludes inputs containing all; the missing all arm is a separate, pre-existing bug in declaration.rs and is filed on its own.

Background

• The minifier pushes each declaration of a block through a chain of per-family handlers (DeclarationHandler::handle_property); the first one that accepts a declaration buffers it, and a declaration nobody accepts is copied to the output unchanged. At the end of the block finalize makes every handler flush its buffer, emitting one shorthand when all four sides are buffered and the individual longhands otherwise. A later declaration for an already buffered side replaces it, which is what removes duplicates.
• SizeHandler<S> is the handler for the four-sided families. S: SizeHandlerSpec only supplies the property ids, value types and two options: the category of the shorthand, and which compat Feature (if any) says a target lacks the logical properties, in which case they are compiled down to physical ones. scroll-padding, like scroll-margin, has no such feature: every browser that supports scroll-padding supports its logical variants.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 11 seconds

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: 0566b86a-2500-4e7c-a2c9-ee6a29728bd8

📥 Commits

Reviewing files that changed from the base of the PR and between eabb96d and 7a2ae0b.

📒 Files selected for processing (3)

• src/css/declaration.rs
• 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 on the released build (1.4.0) with cssInternals.minifyTest from bun:internal-for-testing: .a{scroll-padding:1px;scroll-padding:0} and four equal scroll-padding-* longhands come back unchanged, while the same inputs spelled with scroll-margin collapse to scroll-margin:0 / scroll-margin:1px.
• Fix: ScrollPaddingSpec / ScrollPaddingHandler in src/css/properties/margin_padding.rs, wired into DeclarationHandler in src/css/declaration.rs after scroll_margin.
• Tests: test/js/bun/css/css.test.ts, "scroll-padding" block. 18 of the 25 cases fail on the released build, all 25 pass with the fix; the full file passes.
• Output for 67 scroll-padding inputs, each with no targets, safari 8 and chrome 90, is identical to lightningcss 1.30.2 (inputs containing all excluded, see the PR description: the missing all reset in DeclarationHandler is pre-existing, affects every handled family, and is filed separately).
• Ready for review.

[claude]

LGTM — mechanical instantiation of the existing SizeHandler for scroll-padding, mirroring ScrollMarginSpec exactly.

What was reviewed:

• ScrollPaddingSpec tags/types line up with the generated Property/PropertyId variants (all payload-free ids, LengthPercentageOrAuto longhands) so the macro projections are well-formed.
• DeclarationHandler wiring is complete and consistent across field / handle_property / finalize / new, placed after scroll_margin to match upstream emit order.
• FEATURE/SHORTHAND_FEATURE = None matches scroll-margin and upstream lightningcss, so logical→physical lowering stays off; the new prefix_tests pin that.
• Test block covers dedup, merge shapes, var() fallback, !important split, physical/logical flush, and cross-rule isolation.

Extended reasoning...

Overview

Adds the missing ScrollPaddingHandler to the CSS minifier so scroll-padding* declarations are deduplicated and merged into shorthands like every sibling family. Three files: a new ScrollPaddingSpec: SizeHandlerSpec impl + type alias in src/css/properties/margin_padding.rs (a direct copy of the adjacent ScrollMarginSpec with the scroll-padding variants substituted), four one-line insertions in src/css/declaration.rs to wire the handler into DeclarationHandler, and ~25 new test cases in the existing css.test.ts (also fixes the scroll-paddding describe-name typo). The stale NOTE: comment about no ScrollPaddingHandler existing is deleted, and the "four"→"five" comment is updated.

Security risks

None. This is a pure CSS-minifier optimization pass with no I/O, no user-controlled paths, no unsafe blocks, and no changes to parsing of untrusted input — the parser already handled these properties.

Level of scrutiny

Low-to-medium. All behavior lives in the shared, already-tested SizeHandler<S> body; this PR only supplies a compile-time spec table. I verified in properties_generated.rs that every referenced PropertyIdTag/PropertyId/Property variant exists with the expected payload type (LengthPercentageOrAuto for the eight longhands, ScrollPadding{,Block,Inline} for the shorthands), and that the PropertyId variants are payload-free so the macro's const TOP_ID = PropertyId::$Top is valid. The FEATURE = None choice mirrors ScrollMarginSpec and, per the PR, upstream lightningcss — so flush() always takes the logical_supported = true path and never lowers logical scroll-padding to physical, which the new prefix_test cases pin.

Other factors

The wiring in DeclarationHandler touches all four required sites in the same relative position, so there's no partial-registration risk. Test coverage is thorough (physical/logical merge, dedup, auto, !important separation, var() fallback preservation, physical↔logical flush, per-rule state reset, interleaving with scroll-margin) and the PR reports byte-identical output vs. lightningcss 1.30.2 across 67 inputs × 3 target sets. No outstanding reviewer comments; no prior claude review on this PR.

[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.