Conversation
|
Updated 6:44 PM PT - Sep 24th, 2026
✅ @robobun, your commit 1792fc10f27455a077afc5a72d005b88250f9ff2 passed in 🧪 To try this PR locally: bunx bun-pr 43937That installs a local version of the PR into your bun-43937 --bun |
|
Status: reproduced on 1.4.3-canary.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughResolution-failure tracking now passes each failed import’s kind to directory watches and stores it with the dependency. Directory-watch retries use the stored kind. CSS and HTML dev-server tests cover unresolved references and file-change behavior. ChangesResolution failure retries
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The retry change preserves each import’s resolution kind. No issue identified here requires resolution before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/bake/dev_server/mod.rs— pre-existing: a CSS or HTML reference whose?queryor#fragmentcontains a/still never reloads after the missing file is created. The watched dir at src/runtime/bake/dev_server/mod.rs:1274 isdirnameof the joined specifier, sourl(./a.svg#icons/home)watches<root>/a.svg#icons, which does not exist. The open at mod.rs:1410 gets ENOENT and insert() returns Ignore, so no watch is registered at all. Fix: for a kind wherekind.is_from_css()holds, strip the specifier at its first?or#(index >= 1, the same rule as resolver.rs:1388) before joining to computedir, so every suffixed CSS/HTML reference watches the real parent directory. The PR text lists this case as a remaining downside.Why this was flagged
A stylesheet contains
background: url(./a.svg#icons/home)orurl(./img.png?path=/x)and the file does not exist yet. resolve_import_records at src/bundler/bundle_v2.rs:6797 calls track_resolution_failure with the raw specifier. mod.rs:1269-1274 joins it onto the importer's dirname and takesdirname, giving<root>/a.svg#iconsinstead of<root>. insert() at mod.rs:1410 opens that path with O_DIRECTORY, gets ENOENT at mod.rs:1420 and returns DirectoryWatchInsertError::Ignore, which track_resolution_failure maps to Ok(()) at mod.rs:1307. No Dep is stored, so the new import_kind retry never runs; creatinga.svgin the root does not send a reload and the page keepsCould not resolve. The base branch fails the same way (it also watched the wrong directory and additionally used ImportKind::Stmt). The resolver itself resolves this specifier fine at bundle time by stripping at the first?/#(resolver.rs:1388), so the only place where the/inside the suffix matters is this dir computation.Verification: pre-existing; acknowledged in diff: the PR description's "Downsides" and "Still kind-blind, not changed here" sections state "A suffix that contains
/(url(./a.svg#icons/home)) still does not reload ... the watched directory is<root>/a.svg#icons, which does not exist" — the note is accurate. Trigger: a CSSurl()/@ import/composes(or HTML asset) reference to a not-yet-existing…
c5b562d to
2f6c4fb
Compare
|
Reply to the review finding on a suffix that contains This PR does not change that case. To watch the correct directory, the code that computes it must remove the suffix first. That is the resolver's rule for a CSS kind, written a second time. I prefer to keep that rule in the resolver only, so this needs a change there, and that is a separate PR. The body lists the case under Downsides. Three of the four inline findings are fixed in 2f6c4fb. The fourth has a reply in its thread. |
When an import does not resolve, the dev server watches its directory and resolves the import again on each change. The retry used ImportKind::Stmt for every record. The resolver removes a ?query or #fragment, and uses the CSS extension order, only for a CSS import kind. So the page did not reload when the missing file of a CSS url() or @import, or of an HTML asset or stylesheet link, with a ?query or #fragment was created. An @import "./other" beside an other.ts resolved in the retry and rebuilt the stylesheet on each change. track_resolution_failure now takes the kind that the bundler resolved with. DirectoryWatchStore stores it in Dep.import_kind, and the retry passes it to the resolver. A bare specifier keeps the ./ prefix, so the retry stays on the relative path. A CSS specifier that starts with ? is stored as written, because ./?v=2 would resolve to the directory.
… is created css: url() with a query, a fragment, or both, bare and after a plugin declines it, @import with a suffix, with a condition, and without an extension beside a file that a JS import finds, and composes with a query. One test has url(?v=2) and url(bun) beside a reference that recovers: an unrelated change must send nothing. One test uses a framework route. html: img src with a query or a fragment, project-relative, a bare stylesheet href, and one page with an asset and a script.
With the "./" prefix the resolver removes the query of "?v=2" and resolves the directory, so the retry rebuilt the stylesheet on each change. track_resolution_failure now returns early for such a specifier, and insert is the same as before. The retry takes the specifier reference inside a block that ends before the dep is freed. Each unsafe block has the comment for its own invariant.
2f6c4fb to
1792fc1
Compare
This branch has not been deployed
1 participant
Problem
?queryor#fragmentnames a missing file. When the file is created, the dev server page does not reload.Could not resolve: "./logo.png?v=2"stays on the open page.@import "./other"beside another.tsrebuilds the stylesheet on each directory change.src/runtime/bake/dev_server/mod.rs:461) resolves each failed import asImportKind::Stmt. The resolver removes a suffix only for a CSS import kind (src/resolver/resolver.rs:1385).Fix
track_resolution_failuretakes theImportKindof the record.Dep.import_kindstores it, and the retry passes it to the resolver.?is not tracked. With the./prefix,./?v=2resolves to the directory.test/bake/dev/{css,html}.test.ts, all fail on 1.4.3-canary. Alsobundle,plugins,hot.Background
DirectoryWatchStorelists the imports that did not resolve. On a directory change the dev server resolves each entry (Dep) again, and rebuilds the importer on success.ImportKindis the origin of an import:Stmt(JS),At(CSS@import),Url(CSSurl(), HTML asset).StmtandUrlrecords.Downsides
Depgrows from 40 to 48 bytes per pending failure. Release.textdoes not grow./(url(./a.svg#icons/home)) still does not reload.Notes
There is no user report for this. It was found by a read of the code during #43854, which names it.
Scope. The references are a CSS
url(),@importandcomposes, and an HTML asset or stylesheet link. A CSS@import "./theme"with no extension has the same fault, because the retry used the JS extensions.Severity. On 1.4.3-canary, for an HTML route,
GET /is 500 before the file is created and 200 after it. The rebuild on request (check_route_failures) recovers it. The open page keeps its error, because no frame is sent. For a framework route, the stylesheet ofmeta.stylesstaysNot Foundon new requests.The
./prefix. The retry runs on the resolver of the server. A bare specifier as written reaches its package and builtin rules:url(bun)and@import "ws"resolved there, and the stylesheet was rebuilt on each change. With the prefix the retry stays on the relative path, which is all that a directory watch can see.insertis the same as on main.A specifier that starts with
?. The resolver removes a?queryunless it starts the specifier. With the prefix,./?v=2loses its query and resolves to the directory, so the stylesheet was rebuilt on each change.track_resolution_failurereturns early for a CSS kind with such a specifier. In the bundlerurl(?v=2)resolves only to a file with that literal name. That case no longer reloads by itself.One failing retry, in a project with a
node_modulesdirectory. Resolver passes / extension probes / package lookups, from temporary counters in a debug build:url(./bun.png?v=2)url(bun.png?v=2)url(bun.png)url(bun)url(?v=2)import "./missing""main's call" is this build with main's arguments at the retry (the
./prefix andImportKind::Stmt). It is not a build of main.Retries for each directory change, one pending
Dep(BUN_DEBUG_DevServer=1, the newDirectoryWatchStore retrylog line): 2 while the file is missing (main's call: 2), 1 on the change that creates it (main's call: 2, fails), 0 after that (main's call: 2).Size, release builds of 4227e46 and of this PR on it:
size: text 80,829,819, data 110,372, bss 1,823,056 on both.nm -S:DirectoryWatchStore::track_resolution_failure2763 to 2913 bytes,resolve_import_records14359 to 14362,handle_parse_task_failure3420 to 3424,process_file_list5348 to 5348 (the log line is not in a release build),on_resolve12160 to 12147.resolve_import_records: 2850 to 2851 instructions. The 5 lines that differ are the argument setup before the call oftrack_resolution_failure. Nothing differs outside the arm of a failed resolve.on_resolveholds the inlinedrun_resolver: 2263 instructions on both. The compiler changed its register and stack slot allocation, so 118 lines differ after the stack slots are normalized. It runs only for an import that anonResolveplugin handled.Dep:size_ofconst assert on both sources, with a failing control in each direction.Tests. Each new test fails on 1.4.3-canary and passes on the debug build, in repeated runs.
@import "./other"besideother.tsfails on main inside its no-traffic check. The other rows fail on main at the reload.css retry does not rebuild on an unrelated changehasurl(?v=2)andurl(bun)beside a reference that recovers. It fails at its no-traffic check when the early return for?is removed, and when a bare CSS specifier is stored as written.html asset and script before createfails when the kind is derived from the loader.run_resolver: withImportKind::Stmtpassed at that call only, exactly that row fails.@importand forcomposesread the stylesheet that the server sends. The test client drops an@layerblock and hashes class names.Not checked.
straceis not installed.Still kind-blind, not changed here.
Resolver::bust_dir_cache_from_specifierreturns false for a bare specifier (Resolver: re-check what a failed lookup did not find, then drop only the stale directories before retrying #40279)./: the watched directory is<root>/a.svg#icons, which does not exist.ImportKind::Stmt, as on main. A siblingindex.html.tssatisfies that retry.<link>beside the bundled one (Dev server keeps the source <link rel="stylesheet"> tag after a failed CSS root recovers #40081, bake: strip the source stylesheet tag from HTML bundled during a failed CSS build #40092).Open PRs on the same lines. None changes the kind of the retry.
pathsalias never resolves a file created after the server starts #43391) moves the./prefix intotrack_resolution_failure. The PR that lands second must keep the early return for?.css retry does not rebuild on an unrelated changefails if it is lost.Dep.import_kindfield, for symlinks.renderertotargetin the same signature. Take both sides.ImportKind::Stmt.[human-review] gate passed · iteration 2 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 2
evidence per changed file