Repository navigation
css: preserve url() query/fragment suffix when rewriting asset references - #33685
Conversation
…nces A CSS url() with a fragment like url(sprites.svg#icon) lost the #icon part when the referenced asset was rewritten to a hashed path or inlined as a data: URL. The resolver strips the ?query/#fragment suffix to find the file on disk, but nothing re-appended it to the rewritten reference, so SVG sprite / mask / clip-path / paint-server references silently pointed at the whole document instead of the fragment. The original specifier (with suffix) is already preserved on the import record as original_path. When the CSS printer emits a rewritten url() reference (data: URL or file-loader unique key), it now re-appends any ?/# suffix found in original_path. External references are unchanged since they keep their original text.
|
Warning Review limit reached
Next review available in: 19 minutes 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 2:33 PM PT - Jul 7th, 2026
❌ @robobun, your commit 2ef395d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33685That installs a local version of the PR into your bun-33685 --bun |
|
No open issues were found that this PR would fix. 🤖 Generated with Claude Code |
Appending a ?query to a base64 data: URL places it in the body passed to forgiving-base64-decode (the Fetch data-URL processor excludes only the fragment when re-serializing), which fails on '?'. Keep only the #fragment on the inline path; the file-loader path still gets the full ?query#fragment since that becomes a real HTTP URL.
There was a problem hiding this comment.
LGTM — the data-URL ?query concern from my earlier review is addressed in 2ef395d, and the fix is now scoped correctly per path.
Extended reasoning...
Overview
This PR fixes CSS url() references losing their ?query/#fragment suffix when the bundler rewrites the asset path. The change is confined to get_import_record_url() in src/css/printer.rs (~30 lines): when an ImportKind::Url record has a valid source_index, the suffix is recovered from record.original_path and re-appended — the full ?query#fragment for the file-loader (unique-key) path, and only the #fragment for the inlined data-URL path. Concatenation goes through the printer's arena (alloc_slice_fill_copy), matching the file's existing allocation pattern; nothing outlives the arena. Three new itBundled cases in test/bundler/esbuild/css.test.ts cover file-loader, data-URL (including ?v=2 dropped and ?v=2#m1 → #m1), and minified output.
Security risks
None. This is output serialization of already-resolved import records; no new untrusted-input parsing, no filesystem/network access, no auth or crypto surface.
Level of scrutiny
Medium — bundler output correctness affects every CSS user, but the change is narrowly gated (record.kind == ImportKind::Url + source_index.is_valid()), so @import and external URLs are untouched, and the no-suffix fast path returns the original slice unchanged. The one subtle interaction (query strings on base64 data: URLs) was raised in my earlier review and fixed exactly as suggested; the thread is resolved.
Other factors
The tests exercise the full variant matrix (fragment / query+fragment / none / custom property / minified / data-URL query-drop) with exact-output assertions, and the PR description confirms they fail on main and the rest of the CSS bundler suite still passes. Behavior now matches esbuild. No open reviewer comments remain.
|
CI is green on every lane that ran tests (278 jobs passed, including all test-bun shards on debian x64-asan, windows x64/x64-baseline/aarch64). The only hard failures are two on agent This should be ready for a maintainer to look at. |
What does this PR do?
A CSS
url()with a fragment or query, e.g.url(sprites.svg#icon)orurl(file.svg?v=3#c), silently lost the#icon/?v=3#cpart when the referenced asset was rewritten during bundling. This happened on both rewrite paths:url(./sprites-<hash>.svg))url(data:image/svg+xml;base64,...))The fragment is load-bearing in CSS: SVG sprite /
<view>/ paint-server references andmask/clip-path/filterelement references are all writtenurl(file.svg#id). Afterbun buildthey pointed at the whole document instead of the fragment, so the mask/clip/paint silently resolved to nothing.Repro
Before:
After:
Cause
The resolver strips the
?#suffix to find the file on disk (src/resolver/resolver.rs:1464) but the stripped suffix is never stored or re-appended. The CSS printer'sget_import_record_urlthen emits the rewritten path or data URL keyed only bysource_index, which has no per-reference suffix.Fix
The original specifier (with suffix) is already preserved on the import record as
original_pathbefore resolution overwritespath. When the CSS printer emits a rewrittenurl()reference for anImportKind::Urlrecord, it now re-appends the suffix fromoriginal_path:?query#fragmentis kept.#fragmentis kept. A?queryappended to a base64data:URL lands in the body passed to forgiving-base64-decode (the Fetch data-URL processor excludes only the fragment when re-serializing) and fails on?, so the query is dropped there. A query on an inlined asset is meaningless anyway since there is no server to interpret it.External references are unchanged since they keep their original text.
How did you verify your code works?
Three new
itBundledcases intest/bundler/esbuild/css.test.ts:css/URLQueryFragmentPreservedFileLoader: file-loader path with#fragment,?query#fragment, no suffix (unchanged), and a custom-property--bg: url(...)casecss/URLQueryFragmentPreservedDataURL: data-URL inline path with#fragment,?query(dropped),?query#fragment(only#fragmentkept), and no suffixcss/URLQueryFragmentPreservedMinified: minified output pathAll three fail on
mainand pass with the fix. The rest oftest/bundler/esbuild/css.test.ts(56 tests) andtest/bundler/css/(168 tests) continue to pass.