fix(transforms): keep the subpath of scoped esm.sh specifiers - #4150
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds shared esm.sh import-map parsing and resolution. It handles scoped packages, subpaths, build channels, reserved names, CDN mappings, URL boundaries, and mapping precedence. Existing resolver paths now delegate esm.sh handling to the shared module. Changesesm.sh import-map resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The PR is merge-ready after normal review and checks; the remaining concern is limited to relocating documentation comments so they describe the correct declarations. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ImportMapResolver
participant resolveEsmShThroughImportMap
participant ImportMap
ImportMapResolver->>resolveEsmShThroughImportMap: resolve esm.sh specifier
resolveEsmShThroughImportMap->>ImportMap: check scoped mapping
ImportMap-->>resolveEsmShThroughImportMap: mapped URL or null
resolveEsmShThroughImportMap->>ImportMap: check global mapping when needed
ImportMap-->>resolveEsmShThroughImportMap: mapped URL or null
resolveEsmShThroughImportMap-->>ImportMapResolver: resolved import or null
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies Full details: Out of Scope Changes checkExplanation The PR includes changes beyond ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac791ac2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00321cdf74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 065b9fb4bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7902972a6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897fb5c327
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0d5cc64ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5cf28350
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review the exact current head . All existing review threads are resolved. Please report any remaining findings against this SHA. |
1 similar comment
|
@codex review Please review the exact current head . All existing review threads are resolved. Please report any remaining findings against this SHA. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5cf28350
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c427807b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ensions Two review findings, both my heuristic reaching further than its evidence. - The route search scanned every path segment for npm or jsr, so any site with a directory of that name looked like a package CDN and https://example.com/npm/some.js stopped being a file. The route is now recognised only at the front of a known CDN's path, which is the same restriction the package-root form already had through its host list. - A version marked a coordinate unconditionally, so a stamped filename such as https://cdn.example/pkg@2.0.0.js was read as one. A version marks a coordinate only when the name carries no extension. The extension pattern now requires a letter after the dot. That is what keeps lodash@4.17.21 a coordinate, since .21 is the tail of a version rather than an extension, while pkg@2.0.0.js is a file. There is a test for each side. Checked thirteen mapping shapes covering every case fixed earlier in this review. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
… a root npm:react@19/ and jsr:@std/path@1/ name package roots, but coordinateSelectsExport counted the trailing separator as an export and dropped the subpath. The remote branch already reads a trailing separator as a directory; this applies the same reading to the two schemes. main produced npm:react@19//sub for that mapping, so neither behaviour was right. Appending now goes through the same join that collapses the doubled separator, giving npm:react@19/sub. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
- A scope marker may arrive percent-encoded, and pathname does not decode it, so https://unpkg.com/%40scope/pkg@2 counted one coordinate segment instead of two and was read as an export rather than a root. - A build channel disambiguates a reserved name the way a version does. Round-tripping the produced URL shows it: v135/v8/sub reads back as the package v8, while a bare stable/sub reads back as the package sub. The exactness rule now applies only when neither a version nor a channel is present, which is where the collision actually is. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
…separator stripTrailingSlash read the raw string, so a dot segment that URL normalisation turns into a separator was invisible to it. https://esm.sh/react@18/. reached the parser as the pathname /react@18/, whose empty final segment it rejects, and the react mapping was bypassed. main resolved all of these, since it normalised with new URL() before extracting the package. Normalise first, then detect and strip. That covers /., /./ and the percent-encoded /%2e, and keeps the separator attached to a subpath when one precedes it, so react@18/sub/. still resolves to .../sub/. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
https://esm.sh/v135/stable/sub stopped resolving. Stripping the channel left `stable/sub`, and the reserved-word rule only accepted a lone segment, so the parser rejected it. main resolved these as the package stable. esm.sh does not nest channels, so once one has been stripped a leading reserved word is a package name whatever follows it. The rule now allows a subpath in that position only. Without a channel ahead of it the ambiguity stands and is unchanged: stable/sub still reads as the package sub, because stable is itself the channel there. Claude-Session: https://claude.ai/code/session_01TNbcqUy64goaeCShfjbRmf
2053774 to
52dedff
Compare
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52dedffd04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
2 participants
Description
extractEsmShSubpathreturned""for every scoped package. The scoped branch took the first two path segments as the package name, which already includes the version, and then looked for the version again in the remainder:https://esm.sh/@scope/pkg@1.0/subpathname@scope/pkg@1.0/subpackageParts@scope/pkg@1.0— already contains the versionafterPackage/subversionMatchnull, because/subdoes not start with@""An empty subpath skips the
esmShPackage + subpathlookup inresolveImportWithMap, so an import map entry keyed on@scope/pkg/subnever matched. The specifier resolved to the package root instead, or tonullwhen only a package-plus-subpath key existed — silent wrong resolution rather than an error. Unversioned scoped specifiers (@scope/pkg/sub) were broken the same way.Fix
Parse both specifier shapes with one expression:
The version is optional and never contains a slash, so splitting it off first leaves the subpath as the remainder for
pkg[@version][/subpath]and@scope/pkg[@version][/subpath]alike. Having a single parse also removes the drift that caused this in the first place: the package name and the subpath now come from the same match rather than from two independent rules that disagreed.One extra defect fixed by the same change
extractEsmShPackagestripped the version with/@[\d.]+.*$/, which only matches numeric versions.@scope/pkg@betakept its tag in the package name and matched nothing in the import map. Sharing the parse fixes that too, and there is a test for it. Flagging it explicitly since it is beyond the literal text of the issue — it is the same scoped-parsing defect and separating the two parsers again would just reintroduce the drift.Related Issue(s)
Fixes #4098
Type of Change
Checklist
Testing
Six cases added, five of which were verified red before the fix: exact package+subpath preference, subpath appended to a URL mapping, unversioned scoped specifiers, multi-segment esm.sh build targets (
@scope/pkg@1.0/es2022/pkg.mjs), and non-numeric version tags. The sixth ("returns null when only a scoped package+subpath key is mapped") passes both before and after and is kept as a regression guard, so the fix cannot start silently falling back to the package root.Summary by CodeRabbit
Bug Fixes
Tests