Repository navigation
Conversation
…o bundle In the dev server, finish_from_bake_dev_server invalidated the import record pointing at a CSS file that failed to bundle so that the file would not become a CSS chunk. The HTML rewriter and the HMR import conversion identify stylesheet imports through that source index, so the importer (re-bundled alongside the broken stylesheet) was compiled as if it imported a plain asset: HTML routes kept their original <link> tag next to the injected one, and JavaScript importers listed the stylesheet as a module dependency and failed to boot. Both outputs stay cached after the stylesheet itself recovers, since only the stylesheet is rebuilt then. Only skip the CSS chunk and leave the record alone. Nothing in the loop mutates the AST columns anymore, so the raw column pointers are replaced with plain slices.
|
Warning Review limit reached
Next review available in: 1 minute 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 (2)
Comment |
|
Updated 3:19 PM PT - Aug 12th, 2026
❌ @robobun, your commit 4b2b14c has 4 failures in
🧪 To try this PR locally: bunx bun-pr 37844That installs a local version of the PR into your bun-37844 --bun |
|
Status: ready for review. Reproduced on the current release build (1.4.0-canary.1, da3851e) with the Fix and regression tests are in this PR ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted one-line behavior fix with a nice unsafe-code cleanup as a side effect.
What was reviewed
- Verified downstream consumers handle a valid
source_indexon a failed CSS file correctly:generateCompileResultForHtmlChunk.rs:143-167(loader resolves to CSS →element.remove()),convertStmtsForChunkForDevServer.rs:64-68(disables the record),generate_javascript_code_for_html_fileatDevServer.rs:2859-2862(skips non-JS). - Confirmed
process_edge_attachment(incremental_graph.rs:979-1005) falls through to the path lookup whenget_cached_indexreturnsNoneforJsOrHtmlmode, so edge tracking for the failure node is unchanged. - Checked that no remaining code in the loop needs mutable column access — the raw-pointer
split_raw()machinery was only there for the removed mutation, so the plain-slice replacement is sound.
Extended reasoning...
Overview
This PR fixes a dev-server bug where an HTML route or JS module that imports a CSS file gets bundled with incorrect output when the CSS file fails in the same build, and that incorrect output persists after the CSS is fixed. The fix is a single behavioral change in finish_from_bake_dev_server (src/bundler/bundle_v2.rs): stop setting record.source_index = Index::INVALID for import records pointing at failed CSS files. Because that was the only mutation in the loop, the surrounding split_raw() raw-pointer column access is replaced with plain shared slices. Two new tests in test/bake/dev/css.test.ts cover the HTML <link> and JS import variants across first-load failure, syntax-error recovery, and url() resolution-error recovery.
Security risks
None. This is dev-server-only bundler bookkeeping with no bearing on auth, crypto, permissions, or untrusted-input parsing.
Level of scrutiny
Moderate. The bake dev server's incremental graph is subtle, and removing a defensive invalidation requires confirming every consumer of the now-preserved source_index behaves correctly for a failed CSS file. I traced each consumer named in the PR description against the source:
generateCompileResultForHtmlChunk.rs:143-170: with a valid index the loader resolves toLoader::Css, so the dev-server branch callselement.remove()instead of falling through to the pretty-path rewrite.unique_key_for_additional_filesis empty for a failed parse, so the first branch is not taken.convertStmtsForChunkForDevServer.rs:64-68: valid index + CSS loader →record.path.is_disabled = true, so the import is not emitted as a JS dependency.DevServer.rs:2858-2862(generate_javascript_code_for_html_file): valid index →is_javascript_like()is false for CSS →continue.incremental_graph.rs:979-1005(process_edge_attachment): valid index with no cached entry (failed files never reach the cache-populating line at :587) falls through to theJsOrHtmlpath lookup at :1001, reaching the same failure node as an invalid index did. This is the original reason the invalidation existed, and it is now handled by the fallback.
The raw-pointer removal is mechanically correct: parts and import_records are now only read (.len(), .as_slice()), and scan_css_imports already took an immutable slice.
Other factors
The tests follow harness conventions (await using for clients, dev.fetch/dev.write, no sleeps), assert specific observable output (exact link-tag count and pattern, fetched CSS content, computed style), and cover the variant matrix the review guidelines ask for (both importer kinds × both failure modes × first-load and mid-session). The PR description states both tests fail on the release build and pass on the debug build, and that the surrounding test/bake/dev/*.test.ts suites still pass. The bug-hunting system found no issues. The net diff removes unsafe code and adds a clarifying comment where the behavior changed.
Editing the HTML file or the script that imports the broken stylesheet rebundles it together with the stylesheet as well, so each test now exercises that trigger in addition to first load and a request while broken.
![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. The one-line behavioral change (dropping the Index::INVALID assignment) checks out against the downstream consumers named in the description, and the raw-pointer → slice simplification follows naturally now that the loop no longer mutates. Because the safety argument hinges on how process_edge_attachment and the HTML/HMR emitters treat a valid-but-uncached source index, a look from someone who owns the bake incremental graph would still be worthwhile.
What was reviewed:
- Traced
generateCompileResultForHtmlChunk.rs:143-170andconvertStmtsForChunkForDevServer.rs:60-68— both key the CSS branch offrecord.source_index, so keeping it valid restores the correct path. - Checked
process_edge_attachment(incremental_graph.rs:978-1005) — a valid index with no cached entry falls through to theJsOrHtmlpath lookup, matching the description's safety claim. - The two new tests cover HTML and JS importers across first-load, request-while-broken, and edit-while-broken, for both syntax and
url()resolution errors.
Extended reasoning...
Overview
The PR removes one line of behavior from finish_from_bake_dev_server in src/bundler/bundle_v2.rs: when walking import records to collect CSS roots, it no longer sets record.source_index = Index::INVALID for a CSS target that failed to bundle (zero parts). Because that was the only mutation in the loop, the surrounding split_raw() raw-pointer scaffolding is replaced with plain immutable slice reads. Two regression tests are added to test/bake/dev/css.test.ts.
Security risks
None. This is dev-server-only bundling logic (guarded by the bake path), affects how a failed stylesheet's import record is classified during HTML/JS code generation, and touches no auth, crypto, network, or untrusted-input parsing.
Level of scrutiny
Moderate. The diff is small, but it removes an assignment that was deliberately added in the past, and the correctness argument spans several files: the HTML chunk emitter derives loader from record.source_index (falling back to Loader::File when invalid, which is what left the original <link> in place); the HMR statement converter disables CSS records only when the index is valid; and process_edge_attachment must not misbehave when it now sees a valid index for a file that never got a cached graph entry. I verified each of these against the current source and they match the PR description — in particular, process_edge_attachment falls through to the bundled_files.get_index(key) path lookup for JsOrHtml mode when get_cached_index returns None, so the failed file still resolves to its existing failure node. Still, the incremental graph's edge bookkeeping is subtle enough that a maintainer who owns it should confirm nothing else relied on the invalidated index as a sentinel.
Other factors
The tests are thorough: they exercise the three ways the importer ends up bundled alongside the broken stylesheet (broken on first load, route requested while broken, importer edited while broken) and both failure kinds (parse error and url() resolve error), for both the HTML-link and JS-import entry points. The fetchRouteHtml helper masks the generation-numbered route script URL so the second and third recoveries can be compared byte-for-byte against the first, which is a strong invariant. The comment-cop bot findings were addressed (the explanatory comment was removed in 4b2b14c). The CI failures reported so far (FreeBSD scripts/build/ci.ts build failure, Windows winver.h clang warnings) are unrelated infrastructure noise. The bug-hunting pass raised nothing.
1 participant
Problem
<link href="styles.css">(a 404 in the browser) and the injected/_bun/asset/<hash>.csslink.import "./styles.css"fails to boot withError: Failed to load bundled module 'styles.css'. This is not a dynamic import, and therefore is a bug in Bun's bundler.Fix
url(), broken on first load and after a rebundle) fail on the current release build with the symptoms above and pass with this change on a debug build.Background
import,<link>or<script>in a file. Itssource_indexpoints at the resolved file, and that file's loader (CSS here) is how later stages decide whether to strip the tag, disable the import, or rewrite it as an asset. An invalid index means unresolved.css_entry_points) are the stylesheets reached from JS or HTML that get a CSS chunk of their own; the loop this PR touches builds that set.Original description
What does this PR do?
In the dev server, an importer that gets bundled in the same build as a stylesheet that fails to bundle is compiled as if the stylesheet were a plain asset, and that output is what keeps being served after the stylesheet is fixed.
Repro (
test/bakeharness, also reproduces withBUN_ASSUME_PERFECT_INCREMENTAL=1through the first-load variant):Observed after recovery:
<link rel="stylesheet" href="styles.css">(a 404 in the browser) and the injected<link rel="stylesheet" href="/_bun/asset/<hash>.css">. A route that was never broken serves only the injected link."index.ts": [ [], ...to"index.ts": [ [ "styles.css", 0 ], ..., and the page fails to boot withError: Failed to load bundled module 'styles.css'. This is not a dynamic import, and therefore is a bug in Bun's bundler.Both persist across further successful edits of the stylesheet and also happen when the stylesheet is broken on first load and fixed afterwards (the existing "initial syntax error" and "css import before create" tests go through this path but only check the status code or the body text).
Cause
finish_from_bake_dev_server(src/bundler/bundle_v2.rs, "Discover all CSS roots") walks the import records of every JS/HTML file to collect CSS roots. For a CSS file that failed (no parts), it skipped the file and also set the record'ssource_indexto invalid. That invalidation dates from when the dev server identified CSS records through a.csstag; today the resolvedsource_indexis the only thing that marks a record as a stylesheet import for the code that runs afterwards in the same build:generateCompileResultForHtmlChunk.rsremoves<link>/<script>tags whose target loader is CSS/JS. With an invalid index it falls through to the "asset" branch and rewriteshrefto the pretty path instead.convertStmtsForChunkForDevServer.rsdisables records whose target is a CSS file. With an invalid index the import is emitted as a regular module dependency.The importer is bundled together with the broken stylesheet on first load, or when a route with reachable failures is requested (
check_route_failuresmarks everything reachable stale and rebundles the route). When the stylesheet is fixed, only the stylesheet is rebuilt, so the HTML text stored on the route bundle and the importer's module code in the incremental graph keep the wrong output.Fix
Leave the record alone and only skip adding the failed file to
css_entry_points. The failed stylesheet is then handled exactly like a healthy one by the rewriter and the HMR conversion, and it still does not get a CSS chunk.Why this is safe: the original reason for clearing the index was the dev server's edge processing, which at the time indexed the resolved-index cache unconditionally for valid indices.
process_edge_attachmentnow falls back to the path lookup when a source index has no cached entry (failed files never get one), so it reaches the same failure node as before, and the route keeps tracking the failure and picks the stylesheet up again when it recovers. The other consumers (generate_javascript_code_for_html_file, the printer'srequire()/import()path) already see valid indices for healthy CSS files and treat a failed one the same way.Since nothing in that loop mutates the AST columns anymore, the raw column pointers it used are replaced with plain slices.
Verification
Two tests added to
test/bake/dev/css.test.ts:html route strips its link tag when the stylesheet is bundled alongside a failing version of itself: broken on first load, then a syntax error and aurl()resolution error after a request-triggered rebundle. Asserts the served document after each recovery has exactly the injected link (and that it serves the fixed CSS), and is otherwise identical to the first recovered document.script importing a stylesheet still boots after the stylesheet fails and recovers: same two flows for a JS importer, asserting a fresh client boots and the recovered styles apply.Both fail on the current release build (the first with both
<link>tags present, the second with theFailed to load bundled module 'styles.css'boot failure) and pass with this change on a debug build.test/bake/dev/{css,html,hot,esm,bundle}.test.tspass on the debug build.