Conversation
|
""" WalkthroughThis change adds support for resolving specifiers with the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 13af688 in 2 minutes and 14 seconds. Click for details.
- Reviewed
448lines of code in4files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/resolve.rs:16
- Draft comment:
Good test coverage for file protocol support. The test cases verify both successful resolution with absolute file URLs and correct error handling for invalid file URL paths. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_B95JuDzgoGY1xtj2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 92.17% 92.20% +0.03%
==========================================
Files 13 13
Lines 3053 3068 +15
==========================================
+ Hits 2814 2829 +15
Misses 239 239 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #151 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib.rs (1)
389-399: Minor: return early instead of labelled blockThe labelled
'check_file_protocolblock +breakis hard to read.
After applying the leak fix you can simplify:if let Some(local) = try_convert_file_url(specifier)? { return self.require_without_parse(cached_path, &local, ctx); }…or keep
specifiermutable but drop the label.src/tests/resolve.rs (1)
108-112: Duplicate test case – remove one instanceThe tuple
"file in module with query and fragment"appears twice.
It’s harmless but clutters the test array.- ( - "file in module with query and fragment", - f.clone(), - "m1/a?#fragment", - f.join("node_modules/m1/a.js?#fragment"), - ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/lib.rs(2 hunks)src/tests/resolve.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (windows-latest)
- GitHub Check: Benchmark
🔇 Additional comments (3)
Cargo.toml (1)
95-96: Consider disabling default features forurlcrate
urlpulls in a noticeable dependency tree (idna,percent-encoding, etc.).
If you only needUrl::parse+Url::to_file_path, the default features (especiallyserde&rustls) are not required.-url = "2.5.4" +url = { version = "2.5.4", default-features = false }Trims compile time and binary size while keeping the new functionality intact.
Check with
cargo tree -e featuresbefore/after to quantify the win.src/lib.rs (1)
84-85: 👍 Import looks goodAdding the
urlcrate is the right call for robustfile://parsing.src/tests/resolve.rs (1)
177-179: Nice negative testThe explicit
PathNotSupportedassertion documents the new behaviour well.
f3aeaf7 to
5679bcb
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 5679bcb in 2 minutes and 27 seconds. Click for details.
- Reviewed
453lines of code in4files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/tests/resolve.rs:72
- Draft comment:
The tests for file protocol resolution look solid. Consider adding tests for variations such as uppercase 'FILE://' and cases with query/fragment parameters in file URLs to ensure comprehensive coverage. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
2. src/lib.rs:389
- Draft comment:
The error mapping for file:// URLs returns a generic 'PathNotSupported'. Consider refining the error message to distinguish between URL parse failures and unsupported (e.g. relative) file URLs. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_tbSEQj26pNmBQNOM
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
93b7f1d to
d797656
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f2d2711 in 2 minutes and 3 seconds. Click for details.
- Reviewed
473lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib.rs:385
- Draft comment:
In the block handlingfile://protocol (lines ~385–400), the URL is parsed and converted into a local file path. This implementation correctly separates absolute paths from relative ones and intentionally rejects relativefile://URLs. Consider adding a comment to document the platform‐specific behavior (e.g. on non‑Windows, relative URLs yield a 'PathNotSupported' error). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lib.rs:386
- Draft comment:
The check usesspecifier.starts_with("file://")which is case-sensitive. Since URL schemes are case-insensitive, consider normalizing the scheme (e.g. to lowercase) if supporting variants like "FILE://" is desired. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. src/tests/resolve.rs:71
- Draft comment:
The test for file protocol resolution (lines 71–79) correctly distinguishes platform-specific errors (usingNotFoundon Windows andPathNotSupportedelsewhere). This confirms the intended behavior; ensure such platform differences are documented in the API docs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. .github/workflows/ci.yml:80
- Draft comment:
Typographical error: The target name "wasm32-wasip1" looks suspicious. Did you mean "wasm32-wasi"? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% wasip1 appears to be a legitimate target, not a typo. It's used consistently in the file and appears alongside wasip1-threads variant. WASI Preview 1 (wasip1) is a real WebAssembly target that succeeded wasm32-wasi. The comment appears to be incorrect in suggesting this is a typo. I could be wrong about the legitimacy of wasip1 if it's a very new or experimental target. Maybe there's a reason to stick with wasm32-wasi. wasip1 is actually the newer, standardized name for the WASI target, and its consistent use in the file along with wasip1-threads variant strongly suggests this is intentional and correct. The comment should be deleted as it incorrectly flags a legitimate target name as a typo.
5. .github/workflows/ci.yml:81
- Draft comment:
Typographical error: The target name "wasm32-wasip1" looks like it might be a typo and perhaps should be "wasm32-wasi". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% 1. wasm32-wasip1 appears to be intentionally used throughout the file. 2. The fact it's used in multiple places suggests this is not a typo. 3. wasip1 appears to be a legitimate WASM target - it's the WASI Preview 1 target. 4. The change from wasm32-unknown-unknown to wasm32-wasip1 seems deliberate and coordinated with other parts of the build. I could be wrong about wasip1 being a legitimate target - maybe it really is a typo that was made consistently throughout the file. A quick verification shows that wasm32-wasip1 is indeed a valid Rust target - it's the WASI Preview 1 target that replaced wasm32-wasi in newer versions. The comment should be deleted as wasm32-wasip1 is not a typo but rather a legitimate and intentionally used WASM target.
Workflow ID: wflow_YixyxOvikfewPbLv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d797656 in 1 minute and 59 seconds. Click for details.
- Reviewed
470lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib.rs:386
- Draft comment:
Consider extracting the file:// protocol conversion into a helper for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The code block is moderately complex with error handling and string manipulation. A helper function could improve readability. However, this is a relatively self-contained piece of functionality that only appears once in the codebase. The current implementation is not particularly hard to understand. The suggestion is subjective - reasonable developers could disagree on whether this refactoring would meaningfully improve the code. The current implementation is already fairly clear. While the refactoring could help slightly, the benefit seems minimal given this appears to be a one-off piece of code. The current implementation is adequately clear and self-contained. The comment should be deleted as the suggested refactoring would provide minimal benefit and the current code is sufficiently clear.
2. src/tests/resolve.rs:44
- Draft comment:
Duplicate test case for 'file in module with query and fragment'; consider removing one. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. .github/workflows/ci.yml:80
- Draft comment:
Typo alert: The target string 'wasm32-wasip1' on line 80 might be a typo. It is likely intended to be 'wasm32-wasi' (consistent with the 'wasi' target) unless this naming is deliberate. Please double-check. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% 1. The comment assumeswasm32-wasip1is a typo, but this appears to be incorrect. 2.wasip1is actually a valid and newer WASI preview 1 target. 3. The consistent use ofwasip1in multiple places in the file suggests this is intentional. 4. The comment is speculative and asks for verification, which violates our rules. Could there be some technical reason whywasm32-wasiwould be preferred overwasm32-wasip1that I'm not aware of? Even if there were technical tradeoffs, the consistent use ofwasip1throughout the file and the existence ofwasip1-threadsstrongly indicates this is a deliberate choice, not a typo. Delete the comment. It incorrectly assumes a valid target name is a typo, and it violates our rules by being speculative and asking for verification.
Workflow ID: wflow_Cr1FORvQEqRdk5w3
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
d797656 to
ec417eb
Compare
ec417eb to
0404b2a
Compare
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0404b2a in 2 minutes and 36 seconds. Click for details.
- Reviewed
470lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib.rs:388
- Draft comment:
In the file:// branch, the URL is parsed and converted to a file path. Consider adding documentation explaining that only absolute file URLs are supported and that on Windows, a relative file URL (e.g. "file://./main.js") results in a PathNotSupported error, which must match platform expectations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment points out a potential platform-specific behavior that could be surprising to users. However, the behavior is determined by the url::Url crate's implementation, not by this code directly. The code simply delegates to url::Url for parsing and doesn't make any guarantees about relative vs absolute URLs. Adding documentation about platform-specific behaviors that we don't control could be misleading if the underlying library changes. I could be wrong about whether this is a limitation we should document - if this is a known limitation of url::Url that won't change, documenting it could help users avoid issues. Even if it's a stable limitation of url::Url, documenting external library behaviors in our code is risky. Users should refer to url::Url docs for its limitations. Our code simply delegates to that library. The comment should be deleted. While it points out a potentially useful detail, it asks us to document behavior that is controlled by an external dependency rather than our own code.
2. src/tests/resolve.rs:71
- Draft comment:
The tests for file:// resolution are comprehensive; ensure that the expected error (NotFound on Windows vs PathNotSupported on others) is consistent with the implementation. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. .github/workflows/ci.yml:80
- Draft comment:
It appears that the target name "wasm32-wasip1" may be a typographical error. If the intention was to support WASI, it might be meant to be "wasm32-wasi". Please confirm if "wasip1" is correct. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests a possible typo, but the consistent use of 'wasip1' in multiple places in the file strongly suggests this is intentional. WASIP1 appears to be a real target - it's the WASI Preview 1 target. The comment is questioning something that appears to be correct and intentional. I might be missing some context about whether wasip1 is a valid Rust target. Maybe there's some specific version requirement or compatibility issue. The consistent usage in the file and the existence of related targets like wasm32-wasip1-threads strongly suggests this is a valid target. Even if there were version requirements, that would be caught by the build system. The comment should be deleted as it's questioning something that appears to be correct and intentional, and any actual issues would be caught by the build.
Workflow ID: wflow_qqAyWGDcQSCvPKw1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #127
Important
Add support for
file://protocol in path resolution and improve error handling for unsupported cases.file://protocol inrequire()inlib.rs, allowing file URL resolution.file://URLs with relative paths inlib.rs.urlcrate toCargo.tomlfor URL parsing.Cargo.lockwith new dependencies:url,form_urlencoded,idna,percent-encoding.resolve.rsforfile://URL resolution and error scenarios.This description was created by
for 0404b2a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests