Repository navigation
Conversation
…browser targets A --target=bun or --target=node build that contains an HTML file (imported from server code, or passed as an entry point) parses and minifies the stylesheets that HTML links with the client transpiler, so they are minified for the default browser targets. The linker then printed them with the build-wide target, which maps to the runtime CSS targets, so the transforms that happen at print time (compiling nesting, lowering media range syntax, rewriting light-dark() references) were skipped and the light-dark() polyfill was emitted half-way: definitions injected at minify time, references left as written. Pick the print targets per CSS chunk instead: a chunk whose entry point was parsed for the browser is printed with browser targets, every other chunk keeps the build's target as before. All four places the linker prints CSS (the stylesheets themselves, preserved external imports, the data: URL wrapper for nested external import conditions, and bare layer statements) go through the same helper.
|
Warning Review limit reached
Next review available in: 6 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughCSS compilation targets now persist on stylesheets, propagate through generated CSS chunk stylesheets, and control CSS printing. Tests cover browser-target transformations across development-server, HTML, Bun, and Node build paths. ChangesCSS target propagation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reworked after review (head a8542a0), waiting on CI. The automated reviews of the rework found nothing to change; the comment-length threads are addressed and resolved. Ready for a maintainer look. Reproduced on bun 1.4.0 with The current revision records the minify targets on the stylesheet and prints with those, instead of deriving the print targets from the chunk's entry point in the linker as the first push did (details in the PR description). Tests: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix routing CSS print targets through the chunk's entry-point target instead of the build-wide one.
What was reviewed:
css_targets_for_chunkreadschunk.entry_point.source_index()and indexesitems_target()— same pattern aspostProcessJSChunk.rs:352; both CSS chunk creation sites incomputeChunks.rssetentry_pointexplicitly, so the index is always valid.- All four
PrinterOptionssites in the linker now use the helper; no strayfor_bundler_target(c.options.target)remains underlinker_context/.TargetsisCopy, so hoisting to a local is fine. - Concurrency: the helper only reads SoA columns already accessed read-only from the same worker callbacks; the
&Chunkreborrow inprepareCssAstsForChunkhappens before the&mut chunk.contentdestructure. - Tests exercise all four print arms plus a negative case pinning that server-graph CSS keeps runtime targets.
Extended reasoning...
Overview
Adds LinkerContext::css_targets_for_chunk (~15 lines + doc comment) and points the four CSS print sites in generateCompileResultForCssChunk.rs (three match arms) and prepareCssAstsForChunk.rs (data-URL nested import) at it. The helper returns browser CSS targets when the chunk's entry point was parsed for Target::Browser, and falls back to c.options.target otherwise — so behavior is unchanged for every existing case except HTML-originated CSS chunks in a bun/node build, which now print with the same targets they were minified with. Unused Targets imports are dropped. Five itBundled tests are added covering a browser-build reference, HTML imported from bun/node server code, an HTML entry of a bun build, and a negative test that server-imported CSS still prints with runtime targets.
Security risks
None. This is bundler output selection (which bun_css::Targets to pass to the CSS printer). No untrusted input parsing, no auth/crypto/permissions, no FFI or memory-ownership changes.
Level of scrutiny
Low-to-medium. ~20 lines of production Rust that swap one Targets computation for another at existing call sites, plus a small read-only helper. The helper's index pattern (items_target()[chunk.entry_point.source_index() as usize]) is identical to an existing use in postProcessJSChunk.rs, and both CSS-chunk constructors in computeChunks.rs populate entry_point. Targets derives Copy. The concurrency contracts of both callbacks are unchanged (read-only over c).
Other factors
- The fix is placed at the right layer: one shared helper on
LinkerContextused by all four sites, rather than duplicating the branch. - The
Browser-only trust with fallback to the build target is deliberately conservative and documented (hashbang-marked-Bunin a browser build, SSR graph); neither edge case changes behavior here. - Tests are thorough: one fixture reaches all four print arms (SourceIndex body, ExternalPath
@import, Layers@layer, and theprepareCssAstsForChunkdata-URL path), with both positive assertions on lowered output andnot.toContainguards on the un-lowered forms. The negative test pins the intentional non-change for server-graph CSS. - The PR description records a broad set of adjacent test files run against a debug build.
Record the minify targets on the StyleSheet (and on the empty sheet the bundler creates for an empty CSS file without minifying it) and print every stylesheet with the targets it carries; the layer statements, external imports and data: URL wrappers synthesized for a chunk take the targets of the chunk's stylesheets. This replaces deriving the print targets from the chunk's entry point in the linker, so print and minify agree by construction rather than by re-deriving the transpiler choice. The shared-stylesheet test now checks that each emitted copy is printed for the targets it was minified for instead of pinning the server build's targets, the fixture also covers an empty imported stylesheet, and a dev server test covers stylesheets linked from HTML there, whose printing changes in the same way.
robobun
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how CSS targets flow between minify and print across the bundler linker (a design change that also interacts with #37051), and the reworked approach hasn't had a human look yet, another pair of eyes would be worthwhile.
What was reviewed:
TargetsisCopy, so the new field survives theptr::readshallow copies inprepareCssAstsForChunkand adds no ownership hazard.chunk_targetsreads the graph ASTs (not the placeholdercss_chunk.astsslots fromcomputeChunks), and every chunk has at least oneSourceIndexentry, so the.unwrap_or_default()is a dead fallback.- The one remaining
for_bundler_targetprint site (transpiler.rs:3068) is the standalone transpiler path, which doesn't readsheet.targetsand is unaffected. - The comment-cop bot feedback was addressed in a8542a0.
Extended reasoning...
Overview
The PR fixes a mismatch between the targets CSS is minified for and the targets it is printed for when an HTML file (and its linked stylesheets) is built by the client transpiler inside a --target=bun/node build. It adds a targets: Targets field to StyleSheet, has StyleSheet::minify record the targets it ran with, and rewires the four linker print sites (generateCompileResultForCssChunk.rs × 3 arms, prepareCssAstsForChunk.rs) to read css.targets instead of Targets::for_bundler_target(c.options.target). The empty-CSS path in ParseTask.rs sets targets from the transpiler that created it. Synthesized chunk entries (layers, external @import, data-URL wrappers) take the chunk's targets via a new chunk_targets helper. Tests cover browser/bun/node builds, an HTML entry of a bun build, a stylesheet shared between server and client graphs, and the dev server.
Security risks
None. This is a build-output correctness change to which CSS lowering transforms run at print time; no user input parsing, auth, or filesystem behavior is touched.
Level of scrutiny
Moderate-to-high. The mechanical edits are small and Targets is Copy, so there is no new memory-safety surface. But the change encodes a design decision — carrying minify targets on the stylesheet and asserting that every stylesheet in a CSS chunk comes from one module graph — that affects all CSS output for server builds and the dev server, and the PR description explicitly calls out interaction with three other open PRs (#37051, #39245, #38594). The approach was already reworked once after review; the current revision has only had bot feedback (comment length) since. A maintainer should confirm the invariant and the interaction with #37051's plan.
Other factors
- Test coverage is thorough: the fixture deliberately reaches every linker CSS print site (source, layers, external path, nested data-URL, empty sheet with import conditions), and the four server-build cases are documented as failing on the release build and passing here. The shared-stylesheet test checks minify/print agreement rather than pinning specific targets, which is the right invariant.
- I checked that the placeholder
BundlerStyleSheet::empty()slots incomputeChunks.rsandbundle_v2.rsare always overwritten byprepareCssAstsForChunkbefore printing, so their defaulttargetsnever reach a printer. - The standalone transpiler print at
transpiler.rs:3068still buildsPrinterOptions.targetsinline and does not read the recorded field, so that path is unchanged. - The github-actions comment-cop flags were addressed in a8542a0 (comments shortened to one line each).
|
Updated 11:39 PM PT - Aug 15th, 2026
✅ @robobun, your commit a8542a09265fece69892e34deea8f76862b5395e passed in 🧪 To try this PR locally: bunx bun-pr 39251That installs a local version of the PR into your bun-39251 --bun |
This branch has not been deployed
1 participant
Problem
bun build --target=bun ./server.tswhereserver.tsimportsindex.html(theBun.servefullstack pattern) emits the stylesheet linked from the HTML with CSS nesting, media range syntax ((width >= 600px)) andlight-dark()left as written.bun build --target=browser ./index.htmllowers all three for the default browser targets (.a .b {,(min-width: 600px),var(--buncss-light, ...)). Same with--target=node, with the HTML passed directly as an entry point of a--target=bunbuild, and for stylesheets linked from HTML served by the dev server.:-webkit-full-screenare added, and the--buncss-light/--buncss-darkdefinitions of thelight-dark()polyfill are injected), but thelight-dark()references that printing is supposed to rewrite are left as is, so the polyfill definitions are dead weight.Target::Browser; minify targets come from that transpiler's target insrc/bundler/ParseTask.rs), but the four places the linker prints CSS built theirPrinterOptionsfromTargets::for_bundler_target(c.options.target), the build-wide target copied from the server transpiler (src/bundler/linker_context/generateCompileResultForCssChunk.rs, three arms, andsrc/bundler/linker_context/prepareCssAstsForChunk.rs).for_bundler_target(Bun | Node)has no browser list, under which every print-gated transform counts as supported.Fix
StyleSheetgets atargetsfield thatStyleSheet::minifyfills in with the targets it ran with (src/css/css_parser.rs). The bundler's one unminified stylesheet, the empty sheet it creates for an empty CSS file (get_empty_css_ast), records its transpiler's targets the same way, because its@importconditions still get printed around it.prepareCssAstsForChunksynthesizes for a chunk (bare@layerstatements, preserved external@imports and thedata:URL wrapper for nested import conditions) take the targets of the chunk's first stylesheet; all stylesheets in a chunk come from the same module graph, so they all carry the same ones. The linker no longer consults the build target for CSS printing at all.light-dark()references) complete what minify did for the same targets, and thelight-dark()polyfill is only right when both halves agree. Printing with what minify recorded makes that hold by construction for every sheet, whichever transpiler minified it. A stylesheet imported by the server code itself therefore keeps printing with the runtime targets it is minified with today; printing it for browsers would produce the opposite half-polyfill (references rewritten to variables that were never declared, which drops the declaration). Browser builds are unchanged since minify and print already agreed there.css_target; it deliberately leaves plain--target=bunbuilds alone, which is the case fixed here. With this change its minify half flows through to printing on its own, so on rebase it can drop its print-side hunks (LinkerOptions.css_targetand the four print sites); keeping them would reintroduce a build-wide print target. bun build --compile --target=browser: bundle standalone HTML as a browser build #39245 (CLI--compile --target=browserstandalone HTML) and bundler: bundle HTML entry points for the browser on every entry path of a server build #38594 (HTML entries registered through plugins orfiles:) fix which transpiler the HTML goes through; this PR fixes how the resulting stylesheets are printed, and applies to the entries those PRs route to the browser graph as well.test/bundler/bundler_html.test.ts(html/css-browser-targets/*). One fixture reaches every print site: the stylesheet itself (nesting, media range,light-dark()), a preserved external@importwith a media condition, an external@importbehind a conditional internal@import(printed into adata:URL while preparing the chunk), the bare@layer foo;left by a deduplicated layer import, and an empty imported stylesheet whoselayer(bar)and media conditions are printed around it (the unminified path). The same assertions run against a browser build (reference, passes before and after), an HTML import from a--target=bunentry, the same from--target=node, and an HTML entry point of a--target=bunbuild; a fifth build imports the HTML and the stylesheet itself fromserver.tsand checks that the HTML copy is printed for browsers and that the server copy is printed for whatever targets it was minified for (the polyfill's definitions and references must come together). The four server cases fail on the current release and pass with this change.test/bake/dev/css.test.tsgets a test that a stylesheet linked from an HTML route is served flattened and lowered with the full polyfill, since the dev server's printing changes in the same way (fails on the current release: it served the half-polyfill output shown above).bundler_html(27),html-import-manifest,bundler_html_server,bun-build-api,standalone,metafile,bundler_edgecase,bundler/css/*,js/bun/css/*(the local-onlycss-fuzzfile and a few bound tests hit their 5s/10s timeouts under the debug build in this environment and pass alone or are skipped in CI; reported separately),js/bun/http/bun-serve-html,bun-serve-html-manifest, andbake/dev/css.test.ts(16). Inbake/dev/production.test.tsthree React server component tests that do not involve CSS hit the 5s default timeout under the debug build here and pass on rerun; the other nine pass.Background
--target=bun/node) that contains HTML keeps two module graphs, one per target, each with its own transpiler. The HTML file and every script and stylesheet it links are registered in the browser graph and parsed by a client transpiler cloned from the server one withtarget = browser; server modules importing the HTML get an import manifest instead. The dev server works the same way for its HTML routes.bun_css::Targets) are a browser version list. The bundler uses the default list (chrome 87, safari 14, firefox 78, edge 88) for the browser target and an empty list for bun and node; with an empty list every feature counts as supported and nothing is lowered.StyleSheet::minifyruns once per file while parsing and decides vendor prefixes and selector fallbacks and injects thelight-dark()polyfill definitions; printing runs in the linker once per chunk entry and compiles nesting, lowers media interval syntax, and rewriteslight-dark(a, b)tovar(--buncss-light, a) var(--buncss-dark, b). Before this change the two took their targets from unrelated places.@imports that have to be hoisted to the top and for the@layerstatements a deduplicated import leaves behind.prepareCssAstsForChunkbuilds aStyleSheetfor each entry (shallow copies of the real ones, fresh ones for the synthesized entries), thengenerateCompileResultForCssChunkprints each of them.Earlier revision of this PR
The first push added
LinkerContext::css_targets_for_chunk, which re-derived the print targets in the linker: browser targets when the chunk's entry point had been parsed forTarget::Browser, the build target otherwise. Review pointed out that this recomputes the decision minify already made (with caveats for entries whose AST target differs from the transpiler that minified them, such as a#!/usr/bin/env bunhashbang or the separate SSR graph) and that recording the minify targets on the stylesheet makes print and minify agree by construction, also for the bake builds #37051 changes. The current revision does that; the test that pinned the server copy's runtime targets was reframed to check minify/print agreement, and the empty-stylesheet case was added after it turned up while moving to the recorded targets.