fix(plugin_css): handle empty 'url()' and improve performance#2265
fix(plugin_css): handle empty 'url()' and improve performance#2265Nirvana-Jie wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 13ac8f0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
Nirvana-Jie
requested review from
Copilot and
wre232114
and removed request for
Copilot
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralized CSS URL resolution by adding a computed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CSS_AST as CSS AST (visitor)
participant SourceReplacer as SourceReplacer
participant ModuleGraph as ModuleGraph
participant Resources as ResourcesMap
participant Public as NormalizedPublicPath
CSS_AST->>SourceReplacer: visit url(...) (Str/Raw)
SourceReplacer->>SourceReplacer: normalize source & early-ignore checks
SourceReplacer->>ModuleGraph: resolve module (ResolveKind::CssUrl)
ModuleGraph-->>SourceReplacer: dependency module id
SourceReplacer->>Resources: filter resources by origin == module id
alt multiple resources
Resources-->>SourceReplacer: select primary if RESOURCE_META_PRIMARY_KEY=="true" else first
else single resource
Resources-->>SourceReplacer: resource
end
SourceReplacer->>Public: prepend normalized_public_path
SourceReplacer-->>CSS_AST: replace url(...) with computed public path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/css-url/src/main.css (1)
3-3: Consider adding a comment explaining this test case.An empty
url()in an example file might confuse developers browsing examples. A brief comment indicating this tests empty URL handling would improve clarity..main { color: red; + /* Test case: empty url() should be handled gracefully without panics */ background: url();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/plugin_css/src/source_replacer.rs(3 hunks)examples/css-url/src/main.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/plugin_css/src/source_replacer.rs (2)
crates/plugin_css/src/dep_analyzer.rs (2)
is_source_ignored(75-81)new(14-19)crates/toolkit/src/resolve/path_start_with_alias.rs (1)
is_start_with_alias(9-31)
🔇 Additional comments (4)
crates/plugin_css/src/source_replacer.rs (4)
17-23: LGTM!The struct refactoring with
normalized_public_pathandreverse_resource_mapcleanly supports the performance optimization (O(1) lookups) and precomputation goals.
33-53: LGTM!The path normalization correctly handles edge cases (empty string, multiple slashes) and ensures a consistent trailing slash for URL construction. The
reverse_resource_mapconstruction efficiently filters to module-origin resources.
64-72: Good fix for empty URL handling.The early return for empty strings on lines 66-68 correctly addresses the empty
url()issue described in the PR. This prevents invalid URLs from being processed further.
88-109: LGTM!The refactored
visit_mut_urlcleanly delegates toresolve_urland only updates the URL value when a valid resource is found (lines 99-105). This prevents the previous panic behavior and leaves unresolved URLs unchanged.
There was a problem hiding this comment.
Pull request overview
This PR refactors the CSS URL resolver in plugin_css to improve performance and handle edge cases, specifically empty url() declarations. The changes replace O(n) linear searches with O(1) HashMap lookups and pre-compute the normalized public path.
Key Changes:
- Introduced early return for empty URL strings to prevent processing errors
- Pre-built a reverse resource map (
ModuleId→resource_name) for O(1) lookups instead of iterating through all resources - Pre-computed
normalized_public_pathin the constructor to avoid repeated calculations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
examples/css-url/src/main.css |
Changed to test empty url() handling by replacing valid URL with url() |
crates/plugin_css/src/source_replacer.rs |
Refactored URL resolution with performance optimizations and empty URL handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if is_source_ignored(source) && !is_start_with_alias(&self.alias, source) { | ||
| return Some(source.to_string()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on line 73 after the closing brace. This should be removed to maintain consistent formatting.
| fn resolve_url(&self, source: &str) -> Option<String> { | ||
| // Ignore empty strings | ||
| if source.is_empty() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
The new empty URL handling logic (returning None when source.is_empty()) lacks test coverage. While there are existing tests in crates/plugin_css/tests/analyze_deps.rs for various URL scenarios, there's no test case for empty url() declarations.
Consider adding a test case that verifies:
- Empty
url()is handled gracefully without panicking - The CSS output remains unchanged or handles the empty URL appropriately
- No dependencies are added for empty URLs
| let dep_module = self.module_graph.get_dep_by_source( | ||
| &self.module_id, | ||
| source, | ||
| Some(ResolveKind::CssUrl), | ||
| ); | ||
|
|
||
| if let Some(resource_name) = self.reverse_resource_map.get(&dep_module) { | ||
| return Some(format!("{}{}", self.normalized_public_path, resource_name)); |
There was a problem hiding this comment.
The call to get_dep_by_source is missing error handling. This method panics when the dependency is not found (as seen in its implementation). The old code wrapped this in a loop that would panic with a more descriptive message if no resource was found, but the new code silently returns None, which could mask legitimate errors.
For URLs that should resolve but are missing from the module graph, the old code would panic with: "can not find resource: resolving {source} for {module_id}. dep: {dep_module}". The new code will silently return None, causing the URL to remain unchanged in the CSS output. This could lead to broken references that are difficult to debug.
Consider either:
- Adding a panic/error when
reverse_resource_map.get(&dep_module)returnsNonefor non-ignored sources, or - Using
get_dep_by_source_optionalinstead and handling theNonecase appropriately.
| let dep_module = self.module_graph.get_dep_by_source( | |
| &self.module_id, | |
| source, | |
| Some(ResolveKind::CssUrl), | |
| ); | |
| if let Some(resource_name) = self.reverse_resource_map.get(&dep_module) { | |
| return Some(format!("{}{}", self.normalized_public_path, resource_name)); | |
| let dep_module = match self.module_graph.get_dep_by_source( | |
| &self.module_id, | |
| source, | |
| Some(ResolveKind::CssUrl), | |
| ) { | |
| dep_module => dep_module, | |
| }; | |
| if let Some(resource_name) = self.reverse_resource_map.get(&dep_module) { | |
| return Some(format!("{}{}", self.normalized_public_path, resource_name)); | |
| } else { | |
| // Only panic for non-ignored sources | |
| if !is_source_ignored(source) { | |
| panic!( | |
| "can not find resource: resolving {} for {}. dep: {}", | |
| source, self.module_id, dep_module | |
| ); | |
| } |
examples/css-url/src/main.css
Outdated
| .main { | ||
| color: red; | ||
| background: url(/@/logo.png); | ||
| background: url(); |
There was a problem hiding this comment.
[nitpick] The example file change from url(/@/logo.png) to url() appears to be for testing the empty URL handling. However, this makes the example less useful as a demonstration. The empty url() will now be silently ignored rather than processed, which means this example no longer demonstrates actual URL resolution.
Consider:
- Keeping the original valid URL in the main example, or
- Adding a separate test file specifically for edge cases like empty URLs rather than changing the primary example.
| background: url(); | |
| background: url(/@/logo.png); |
| public_path: String, | ||
| alias: Vec<AliasItem>, | ||
| normalized_public_path: String, | ||
| reverse_resource_map: HashMap<ModuleId, String>, |
There was a problem hiding this comment.
A Module id may be mapped to multiple resources, we should use HashMap<ModuleId, Vec> here. BTW, we should only map css related module resource map here.
Nirvana-Jie
force-pushed
the
fix/css-url-resolver-improvements
branch
from
ea98a58 to
8224f66
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/plugin_css/src/source_replacer.rs (1)
53-57: Add an explicit test for emptyurl()handlingThe new early return for
source.is_empty()fixes the panic for emptyurl(), but this path doesn’t appear to be covered by tests yet. Adding a small fixture that exercisesurl("")/url('')/url()(whatever SWC parses as empty) would help prevent regressions.
🧹 Nitpick comments (3)
examples/css-url/src/main.tsx (1)
19-25: Minor indentation inconsistency.Lines 20 and 23 have an extra leading space compared to the existing
<div>blocks above (e.g., line 10, 14, 17). This is purely cosmetic but worth aligning for consistency.- <div className='main-4'> + <div className='main-4'> main-4 </div> - <div className='main-5'> + <div className='main-5'> main-5 </div>crates/plugin_css/src/source_replacer.rs (2)
53-95: Good defensive URL resolution; consider tightening behavior for ignored sources and optimizing resource lookupThe new
resolve_urlfixes the emptyurl()panic and replacesget_dep_by_sourcewith the optional variant, which is a solid safety improvement. Two follow‑ups to consider:
Ignored sources rewriting AST
Foris_source_ignored(source) && !is_start_with_alias(...)you returnSome(source.to_string()), which causesvisit_mut_urlto rewrite the node toUrlValue::Strwithraw: None. That slightly changes how ignored URLs (e.g.data:or external URLs) are serialized because the original raw lexeme is lost. If preserving exact output for ignored URLs matters, you could instead returnNonehere and let the caller leave the AST untouched.Performance of
matching_resourceslookup
matching_resourcesis built by scanningself.resources_map.values()on everyresolve_urlcall, which is O(N_resources) per URL. For stylesheets with many URLs and a largeresources_map, this can dominate runtime. Since the access pattern isModuleId -> [Resource], consider precomputing a reverse map once innew(e.g.HashMap<ModuleId, Vec<&Resource>>or similar, possibly CSS‑only) soresolve_urldoes a cheap hash lookup plus a small linear scan over that vector.Both items are non‑blocking but would align the implementation more closely with the stated perf and behavior goals.
98-115: URL visitor correctly delegates toresolve_url, with a minor behavioral change for ignored URLsThe visitor now:
- Delegates resolution for both
UrlValue::StrandUrlValue::Rawtoresolve_url.- Only rewrites the node when a concrete string is returned, avoiding accidental panics or overwriting on failures.
One side effect (due to the
resolve_urlbranch for ignored sources) is that ignored URLs will always be rewritten toUrlValue::Strwithraw: None, even if they started asRaw. If you adopt the suggestion to returnNonefor ignored sources, this visitor will automatically preserve the original AST shape for those cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/mighty-pears-carry.md(1 hunks)crates/core/src/context/mod.rs(4 hunks)crates/core/src/resource/mod.rs(1 hunks)crates/plugin_css/src/source_replacer.rs(4 hunks)crates/plugin_static_assets/src/lib.rs(1 hunks)examples/css-url/src/main.css(1 hunks)examples/css-url/src/main.tsx(1 hunks)rust-plugins/dts/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
rust-plugins/dts/src/lib.rs (1)
crates/core/src/context/mod.rs (6)
new(72-101)new(301-313)new(433-435)new(447-449)new(461-463)new(491-504)
crates/core/src/context/mod.rs (1)
crates/core/src/resource/mod.rs (1)
default(147-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Spell
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm-eabi
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm-gnueabihf
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Type Check
- GitHub Check: Test - windows-latest
- GitHub Check: Test - macos-latest
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Check
- GitHub Check: Rust Code Lint
🔇 Additional comments (10)
.changeset/mighty-pears-carry.md (1)
1-6: LGTM!The changeset correctly documents the patches with an accurate description of the fixes: empty URL handling and multi-resource primary field support.
crates/core/src/resource/mod.rs (1)
14-16: LGTM!The constant is well-named, appropriately documented, and follows the existing naming conventions. The
farm_prefix helps prevent collisions with user-defined metadata keys.examples/css-url/src/main.css (1)
28-46: LGTM!Good addition of test cases for both empty URL variants (
url()andurl('')). This properly addresses the prior review feedback by adding new test cases instead of modifying existing ones, ensuring the alias URL test remains intact.rust-plugins/dts/src/lib.rs (1)
113-118: LGTM!Clean migration to the new
EmitFileParams::new()constructor. The defaultis_primary: Some(true)is appropriate for DTS files since they represent the primary output for their source module.crates/plugin_static_assets/src/lib.rs (1)
271-276: LGTM!Consistent migration to
EmitFileParams::new(). Static assets are correctly marked as primary by default, which aligns with expected URL resolution behavior.crates/core/src/context/mod.rs (3)
165-181: Verify lock scope and ordering.The current implementation acquires the lock to clear existing primary markers (lines 170-176), then releases it before acquiring it again to insert the new resource (lines 203-206). This is correct for avoiding deadlocks, but worth noting:
- If another thread emits a resource for the same module between the two lock acquisitions, both could end up marked as primary until the second thread's clearing pass runs.
- This is likely acceptable given typical single-threaded emit patterns during transform.
If strict atomicity is required, consider holding a single lock scope for both operations. Otherwise, the current approach is fine for the expected use case.
472-474: LGTM!The
default_is_primaryhelper function correctly provides the serde default value.
476-510: Well-designed API additions.The
EmitFileParamsenhancements are clean:
is_primaryfield with appropriate serde handling (defaulting totrue, skipping serialization whenNone)new()constructor that sets sensible defaultswith_primary()builder for explicit control when neededThis provides a good balance between convenience (most callers use
new()with default primary=true) and flexibility (explicit override via builder).crates/plugin_css/src/source_replacer.rs (2)
26-52: Normalized public path construction looks correct and robustThe
normalized_public_pathlogic correctly handles"","/"and multiple trailing slashes, and centralizing this innewavoids repeated work in hot paths. No correctness issues spotted here.
121-145: Usingget_dep_by_source_optionalin stylesheet import handling removes panic riskSwitching
visit_mut_stylesheettoget_dep_by_source_optional(...CssAtImport)and gating rule removal on.is_some()ensures that missing@importdependencies no longer panic the build; they simply leave the rule in place. This is the right trade‑off for robustness, and the existing!is_source_ignored(&source)guard keeps behavior consistent with URL resolution.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
crates/core/src/context/mod.rs:206
- The
resources_maplock is acquired twice in this function: once at line 170 to clear primary markers, and again at line 204 to insert the new resource. This creates a window where other threads could modify the resources_map between releases. Consider holding the lock for the entire operation or restructuring to acquire it once:
let mut resources = self.resources_map.lock();
// Clear previous primary markers
if should_mark_primary {
for resource in resources.values_mut() {
if matches!(&resource.origin, ResourceOrigin::Module(m_id) if m_id == &module_id) {
resource.meta.remove(RESOURCE_META_PRIMARY_KEY);
}
}
}
// Insert new resource
resources.insert(params.name.clone(), resource); if should_mark_primary {
let mut resources = self.resources_map.lock();
for resource in resources.values_mut() {
if matches!(&resource.origin, ResourceOrigin::Module(m_id) if m_id == &module_id) {
resource.meta.remove(RESOURCE_META_PRIMARY_KEY);
}
}
}
let mut meta = HashMap::default();
if should_mark_primary {
meta.insert(RESOURCE_META_PRIMARY_KEY.to_string(), "true".to_string());
}
let resource = Resource {
name: params.name.clone(),
name_hash: "".to_string(),
bytes: params.content,
emitted: false,
should_transform_output_filename: true,
resource_type: params.resource_type,
origin: ResourceOrigin::Module(module_id.clone()),
meta,
special_placeholders: Default::default(),
};
if self.cache_manager.enable() {
self.write_metadata(
"builtin:emit_file",
resource.clone(),
Some(MetadataOption::default().refer(vec![module_id])),
);
}
self
.resources_map
.lock()
.insert(params.name.clone(), resource);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Mark this resource as the primary one when multiple resources exist for the same module. | ||
| /// Defaults to true if not specified. | ||
| /// When a module generates multiple resources, the primary one is used for URL resolution. | ||
| #[serde(default = "default_is_primary", skip_serializing_if = "Option::is_none")] | ||
| pub is_primary: Option<bool>, |
There was a problem hiding this comment.
The documentation states "Mark this resource as the primary one when multiple resources exist for the same module" and "Defaults to true if not specified." However, when is_primary is None or not provided, the behavior defaults to true (line 166 with unwrap_or(true)), which means every resource emitted will be marked as primary and will clear previous primary markers. This could be confusing.
Consider either:
- Making the default
falseso resources aren't automatically marked as primary unless explicitly requested, or - Clarifying in the documentation that the first resource emitted for a module will be primary by default, and subsequent resources need to explicitly set
is_primary: trueto change which one is primary.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/css-url/src/main.tsx (1)
20-25: Newmain-4/main-5blocks correctly extend the example; consider a brief intent commentThe additional divs are structurally consistent with the rest of the component and give you concrete elements to exercise the
url()andurl('')cases the plugin now handles.Optionally, add a short comment here or in
main.cssnoting thatmain-4andmain-5are intentionally using emptyurl()values, so future readers understand these are fixtures for the CSS URL resolver behavior rather than accidental broken styles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/css-url/src/main.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm-gnueabihf
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm-eabi
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Type Check
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Test - windows-latest
- GitHub Check: Test - macos-latest
Nirvana-Jie
force-pushed
the
fix/css-url-resolver-improvements
branch
from
eba938d to
64706a2
Compare
Nirvana-Jie
force-pushed
the
fix/css-url-resolver-improvements
branch
from
64706a2 to
eb75931
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/context/mod.rs (1)
162-207: Primary resource marking is not atomic across concurrentemit_filecallsThe clearing of previous primary markers (lines 169‑175) and insertion of the new
Resource(lines 203‑206) use two separateresources_maplocks. Two threads callingemit_fileconcurrently for the samemodule_idwithis_primary = truecan each clear existing markers and then insert their own resource, leaving multiple resources marked primary for that module.To keep the “single primary per module” invariant that
SourceReplacerrelies on, consider making the clear‑and‑insert sequence atomic under one lock, e.g.:let module_id = self.str_to_module_id(¶ms.resolved_path); - // Determine if this resource should be marked as primary (defaults to true) - let should_mark_primary = params.is_primary.unwrap_or(true); - - // If user marks this resource as primary, clear previous primary markers for this module - if should_mark_primary { - let mut resources = self.resources_map.lock(); - for resource in resources.values_mut() { - if matches!(&resource.origin, ResourceOrigin::Module(m_id) if m_id == &module_id) { - resource.meta.remove(RESOURCE_META_PRIMARY_KEY); - } - } - } - - let mut meta = HashMap::default(); - if should_mark_primary { - meta.insert(RESOURCE_META_PRIMARY_KEY.to_string(), "true".to_string()); - } - - let resource = Resource { + let should_mark_primary = params.is_primary.unwrap_or(true); + + let mut meta = HashMap::default(); + if should_mark_primary { + meta.insert(RESOURCE_META_PRIMARY_KEY.to_string(), "true".to_string()); + } + + let resource = Resource { name: params.name.clone(), @@ - origin: ResourceOrigin::Module(module_id.clone()), - meta, + origin: ResourceOrigin::Module(module_id.clone()), + meta, special_placeholders: Default::default(), }; @@ - self - .resources_map - .lock() - .insert(params.name.clone(), resource); + { + let mut resources = self.resources_map.lock(); + if should_mark_primary { + for existing in resources.values_mut() { + if matches!(&existing.origin, ResourceOrigin::Module(m_id) if m_id == &module_id) { + existing.meta.remove(RESOURCE_META_PRIMARY_KEY); + } + } + } + resources.insert(params.name.clone(), resource); + }This way, at most one resource per module can end up marked primary, even under concurrent emission.
♻️ Duplicate comments (1)
crates/plugin_css/src/source_replacer.rs (1)
53-94: URL resolution behavior is sound, but per‑URL scan over all resources is still O(n)The new
resolve_urlnicely:
- skips empty/whitespace
url()values,- leaves ignored sources untouched (unless matched by an alias),
- avoids panics by using
get_dep_by_source_optional, and- prefers resources marked with
RESOURCE_META_PRIMARY_KEYwhen multiple resources share the same origin.However, for every CSS
url(...)you still do:self .resources_map .values() .filter(|r| matches!(&r.origin, ResourceOrigin::Module(m_id) if m_id == &dep_module)) .collect();which is O(total_resources) per URL. If there are many resources and many CSS URLs, this remains a potential hot path.
Consider pre‑indexing once into a reverse map, e.g.
HashMap<ModuleId, Vec<&Resource>>(or resource names), possibly filtered to the CSS‑relevant resources, and then doing O(1) lookups inresolve_url. That would align the implementation with the stated performance goals.
🧹 Nitpick comments (1)
crates/core/src/context/mod.rs (1)
472-509:EmitFileParams::is_primaryAPI semantics may be more complex than neededThe combination of:
fn default_is_primary() -> Option<bool> { Some(true) }(lines 472‑474),EmitFileParams::newsettingis_primary: Some(true)(lines 497‑503), andparams.is_primary.unwrap_or(true)inemit_filemeans resources are primary by default everywhere, and
Noneis effectively treated the same asSome(true). In practice, onlySome(false)ever yields a non‑primary resource.Given that behavior, you might simplify the API to a plain
bool:#[serde(default)] pub is_primary: bool;with
EmitFileParams::newusingis_primary: trueandwith_primarytoggling it, and adjustemit_fileto read thebooldirectly. This removes an unnecessaryOptionlayer and makes the default behavior clearer to callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/mighty-pears-carry.md(1 hunks)crates/core/src/context/mod.rs(4 hunks)crates/core/src/resource/mod.rs(1 hunks)crates/plugin_css/src/dep_analyzer.rs(1 hunks)crates/plugin_css/src/source_replacer.rs(4 hunks)crates/plugin_static_assets/src/lib.rs(1 hunks)examples/css-url/src/main.css(2 hunks)examples/css-url/src/main.tsx(1 hunks)examples/css-url/src/test.css(1 hunks)rust-plugins/dts/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/plugin_static_assets/src/lib.rs
- examples/css-url/src/main.tsx
- examples/css-url/src/main.css
- crates/plugin_css/src/dep_analyzer.rs
- .changeset/mighty-pears-carry.md
- rust-plugins/dts/src/lib.rs
- crates/core/src/resource/mod.rs
- examples/css-url/src/test.css
🧰 Additional context used
🧬 Code graph analysis (1)
crates/plugin_css/src/source_replacer.rs (3)
crates/plugin_css/src/dep_analyzer.rs (2)
is_source_ignored(75-81)new(14-19)crates/toolkit/src/resolve/path_start_with_alias.rs (1)
is_start_with_alias(9-31)packages/core/src/types/binding.ts (1)
ResolveKind(15-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm-gnueabihf
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm-eabi
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Type Check
- GitHub Check: Check
- GitHub Check: Test - macos-latest
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Test - windows-latest
- GitHub Check: Spell
- GitHub Check: Rust Code Lint
🔇 Additional comments (3)
crates/plugin_css/src/source_replacer.rs (3)
26-51:normalized_public_pathconstruction looks robustThe normalization in
SourceReplacer::newcorrectly handles:
- empty
public_pathand slash-only values by normalizing to"/", and- non-empty values by trimming trailing slashes and appending a single
/.This should avoid subtle double‑slash or empty‑path issues in URL generation while keeping behavior predictable.
98-115:visit_mut_urlrefactor correctly centralizes URL handling and skips invalid URLsDelegating both
StrandRawcases toresolve_urland only rewriting when it returnsSome(...)improves clarity and ensures:
- empty
url()s and unresolved URLs leave the AST untouched, and- all replacement logic (including ignore/alias rules and primary selection) lives in a single place.
This looks consistent with the intended behavior change to avoid panics while preserving valid URLs.
134-143: Usingget_dep_by_source_optionalfor@importavoids hard panicsSwitching
visit_mut_stylesheettoget_dep_by_source_optional(...).is_some()forCssAtImportresolution removes the previous panic risk when an@importcannot be resolved, while still removing rules only when there is an actual dependency edge.This aligns with the more defensive handling you added for
url()resolution and should make CSS processing more robust.
Nirvana-Jie
force-pushed
the
fix/css-url-resolver-improvements
branch
from
eb75931 to
13ac8f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/plugin_css/src/source_replacer.rs (1)
68-72: O(n) linear search contradicts PR's claimed O(1) HashMap lookup.The PR description states: "Replaced O(n) linear search through all resources for each
url()with O(1) HashMap lookups using a pre-built reverse resource map." However, this code still performs an O(n).filter()iteration through all resources for eachurl()encountered in CSS files.For projects with many resources and CSS files containing multiple
url()references, this could significantly impact build performance.🔎 Suggested fix: Pre-build a reverse map for O(1) lookups
Modify the
SourceReplacerstruct to include a reverse map:pub struct SourceReplacer<'a> { module_id: ModuleId, module_graph: &'a ModuleGraph, alias: Vec<AliasItem>, normalized_public_path: String, - resources_map: &'a HashMap<String, Resource>, + reverse_resource_map: HashMap<ModuleId, Vec<&'a Resource>>, }In the constructor, build the reverse map once:
) -> Self { let normalized_public_path = if public_path.is_empty() { "/".to_string() } else { let trimmed = public_path.trim_end_matches('/'); if trimmed.is_empty() { "/".to_string() } else { format!("{}/", trimmed) } }; + + // Build reverse map: ModuleId -> Vec<Resource> + let mut reverse_resource_map: HashMap<ModuleId, Vec<&Resource>> = HashMap::default(); + for resource in resources_map.values() { + if let ResourceOrigin::Module(m_id) = &resource.origin { + reverse_resource_map.entry(m_id.clone()).or_default().push(resource); + } + } Self { module_id, module_graph, alias, normalized_public_path, - resources_map, + reverse_resource_map, } }Then in
resolve_url, use O(1) lookup:- let matching_resources: Vec<&Resource> = self - .resources_map - .values() - .filter(|r| matches!(&r.origin, ResourceOrigin::Module(m_id) if m_id == &dep_module)) - .collect(); + let matching_resources = self + .reverse_resource_map + .get(&dep_module)?; - if matching_resources.is_empty() { - return None; - } - - let resource = if matching_resources.len() > 1 { + let resource = if matching_resources.len() > 1 { matching_resources .iter() - .find(|res| { + .find(|&res| { res .meta .get(RESOURCE_META_PRIMARY_KEY) .map(|v| v == "true") .unwrap_or(false) }) - .unwrap_or(&matching_resources[0]) + .copied() + .unwrap_or(matching_resources[0]) } else { - &matching_resources[0] + matching_resources[0] };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/mighty-pears-carry.mdcrates/core/src/context/mod.rscrates/core/src/resource/mod.rscrates/plugin_css/src/dep_analyzer.rscrates/plugin_css/src/source_replacer.rscrates/plugin_static_assets/src/lib.rsexamples/css-url/src/main.cssexamples/css-url/src/main.tsxexamples/css-url/src/test.cssrust-plugins/dts/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/core/src/resource/mod.rs
- examples/css-url/src/test.css
- crates/plugin_static_assets/src/lib.rs
- rust-plugins/dts/src/lib.rs
- .changeset/mighty-pears-carry.md
- examples/css-url/src/main.tsx
- crates/plugin_css/src/dep_analyzer.rs
- examples/css-url/src/main.css
🧰 Additional context used
🧬 Code graph analysis (2)
crates/core/src/context/mod.rs (1)
crates/core/src/resource/mod.rs (1)
default(147-159)
crates/plugin_css/src/source_replacer.rs (3)
crates/plugin_css/src/dep_analyzer.rs (2)
is_source_ignored(75-81)new(14-19)crates/toolkit/src/resolve/path_start_with_alias.rs (1)
is_start_with_alias(9-31)packages/core/src/types/binding.ts (1)
ResolveKind(15-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm-gnueabihf
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm-eabi
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Type Check
- GitHub Check: Check
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Test - macos-latest
- GitHub Check: Test - windows-latest
- GitHub Check: Rust Code Lint
🔇 Additional comments (3)
crates/core/src/context/mod.rs (1)
490-509: LGTM! Well-structured constructor and builder pattern.The
new()constructor correctly initializesis_primarytoSome(true), aligning with the default behavior. Thewith_primary()builder method provides a clean API for callers who need to override the default.crates/plugin_css/src/source_replacer.rs (2)
33-42: LGTM! Robust public path normalization.The logic correctly handles empty paths, ensures a trailing slash, and covers the edge case where the path contains only slashes. Pre-computing this value in the constructor avoids repeated normalization on every URL resolution.
103-114: LGTM! Clean refactoring that eliminates code duplication.The centralized
resolve_urlmethod handles bothStrandRawURL variants consistently, and the conditional update ensures URLs that can't be resolved remain unchanged. This improves maintainability without altering behavior.
| if should_mark_primary { | ||
| let mut resources = self.resources_map.lock(); | ||
| for resource in resources.values_mut() { | ||
| if matches!(&resource.origin, ResourceOrigin::Module(m_id) if m_id == &module_id) { | ||
| resource.meta.remove(RESOURCE_META_PRIMARY_KEY); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
O(n) iteration through all resources to clear primary markers.
On each emit_file call with is_primary=true (the default), this code locks the resources map and iterates through all resources to find those originating from the same module. For large projects with thousands of resources, this O(n) scan on every emission could become a performance bottleneck.
Consider maintaining a reverse index (HashMap<ModuleId, Vec<String>>) mapping module IDs to their resource names to enable O(1) lookups.
Would you like me to generate a refactored implementation using a reverse index for O(1) primary marker management?
| let resource = if matching_resources.len() > 1 { | ||
| matching_resources | ||
| .iter() | ||
| .find(|res| { | ||
| res | ||
| .meta | ||
| .get(RESOURCE_META_PRIMARY_KEY) | ||
| .map(|v| v == "true") | ||
| .unwrap_or(false) | ||
| }) | ||
| .unwrap_or(&matching_resources[0]) | ||
| } else { | ||
| &matching_resources[0] | ||
| }; |
There was a problem hiding this comment.
Non-deterministic fallback when no primary resource is marked.
When multiple resources match but none has RESOURCE_META_PRIMARY_KEY set, the code falls back to matching_resources[0]. Since HashMap iteration order is not guaranteed, this fallback could be non-deterministic across runs.
While the new primary-marking logic in emit_file should ensure at most one primary resource per module, edge cases (e.g., resources emitted before this PR, or external resources) might still trigger this path.
🔎 Suggested improvement for deterministic fallback
Consider sorting by resource name or timestamp before selecting the fallback:
let resource = if matching_resources.len() > 1 {
- matching_resources
+ let primary_resource = matching_resources
.iter()
.find(|res| {
res
.meta
.get(RESOURCE_META_PRIMARY_KEY)
.map(|v| v == "true")
.unwrap_or(false)
- })
- .unwrap_or(&matching_resources[0])
+ });
+
+ if let Some(res) = primary_resource {
+ res
+ } else {
+ // Fallback: use lexicographically first resource name for determinism
+ matching_resources.iter().min_by_key(|r| &r.name).unwrap()
+ }
} else {
&matching_resources[0]
};🤖 Prompt for AI Agents
In crates/plugin_css/src/source_replacer.rs around lines 78 to 91, the fallback
selection of a resource when multiple matches exist but none is marked as
primary is non-deterministic (uses matching_resources[0]); change this to a
deterministic choice by selecting the resource with a stable key (for example,
sort or use min_by on a stable field such as resource.name, resource.path, or a
timestamp) and use that as the fallback; ensure the comparison is
locale/OS-independent (e.g., use raw string ordering) and preserve the existing
primary-selection branch when a primary meta flag is present.
📋 Description
This PR improves the CSS URL resolver in
plugin_cssby fixing empty URL handling and optimizing resource lookup performance.🐛 Bug Fixes
url()gracefully: Previously, empty URLs could cause processing errors. Now returnsNoneearly to skip invalid URLs.unwrap()calls with safeOptionhandling. Missing resources now returnNoneinstead of panicking.⚡ Performance Improvements
Before
url()referencenormalized_public_pathfor every URLAfter
ModuleId→resource_name)normalized_public_pathin constructorSummary by CodeRabbit
Refactor
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.