fix: resolve dir index with dot specifier correctly #123
Conversation
WalkthroughThe changes update the resolution logic for relative specifiers Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Resolver
participant Filesystem
Caller->>Resolver: require_relative(specifier, cached_path, options)
alt specifier is "." or "./" and !fully_specified
Resolver->>Filesystem: check if cached_path is directory
alt cached_path is directory
Resolver->>Filesystem: normalize with "./index.js"
else cached_path is file
Resolver->>Filesystem: normalize with "../index.js"
end
Filesystem-->>Resolver: return file existence
alt index.js exists
Resolver-->>Caller: return index.js path
else
Resolver->>Filesystem: continue with original logic
end
else
Resolver->>Filesystem: continue with original logic
end
Filesystem-->>Resolver: result
Resolver-->>Caller: resolved path
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (2)
✨ 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.
Pull Request Overview
This PR fixes the resolution of directory indexes when using the dot specifier, ensuring that the correct index file is selected based on whether the target is a directory or a file.
- Test cases in src/tests/resolve.rs were updated to verify resolution behavior using a "foo" directory.
- The resolution logic in src/lib.rs now considers the fully_specified option and conditionally applies sub-path normalization.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/resolve.rs | Updated tests to validate proper index resolution in a "foo" folder |
| src/lib.rs | Modified resolution logic to conditionally use "./index.js" or "../index.js" based on file type and the fully_specified flag |
Comments suppressed due to low confidence (1)
src/tests/resolve.rs:139
- [nitpick] The variable name 'foo_dir_foo' may be unclear. Consider renaming it to 'foo_file' or a similarly concise name to better indicate that it contains a file path.
let foo_dir_foo = foo_dir.join("foo.js");
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to bcf1978 in 57 seconds. Click for details.
- Reviewed
52lines 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:435
- Draft comment:
Good fix in require_relative: using "./index.js" if the base path is a directory and "../index.js" if it's a file. Consider adding a brief comment explaining why the file case uses '../index.js'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/tests/resolve.rs:39
- Draft comment:
Duplicate test case for 'file in module with query and fragment' found (lines 39–40). Consider removing one to avoid redundancy. - 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. src/tests/resolve.rs:136
- Draft comment:
The 'resolve_dot' test clearly verifies dot specifier resolution in both file and directory contexts. Adding a brief inline comment to explain the expected behavior could improve clarity for future maintainers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_QG6OZQh5fWiheiKf
You can customize 
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
=======================================
Coverage 93.42% 93.43%
=======================================
Files 13 13
Lines 2891 2892 +1
=======================================
+ Hits 2701 2702 +1
Misses 190 190 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #123 will not alter performanceComparing Summary
|
Important
Fixes directory index resolution with dot specifier in
ResolverGeneric::require_relative()and updates tests and fixtures accordingly.ResolverGeneric::require_relative()insrc/lib.rs../index.js; otherwise, resolves to../index.js.resolve_dot()insrc/tests/resolve.rsto verify correct resolution of dot specifiers.dot.jstofoo.jsandindex.jstofoo/foo.jsinfixtures/dotto align with test cases.This description was created by
for bcf1978. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests