Uh oh!
There was an error while loading. Please reload this page.
fix: correctly parse package name for scoped packages - #2135
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDecodes the route Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/api/registry/analysis/[...pkg].get.ts (1)
38-46: Consider extracting route package-param parsing into a shared helper.This block is now duplicated across endpoints; centralising it would lower future drift risk when parsing/decoding rules change.
test/unit/server/utils/parse-package-params.spec.ts (1)
4-53: Useful parser coverage; add one regression test for encoded route input.These tests validate segment parsing well, but the bug fix is in handler decoding. Please add a handler-level test with
%40scope%2Fnameso the original regression is fully guarded.As per coding guidelines
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality using vitest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a276c004-28ce-491f-b50c-2f60a4d5f279
📒 Files selected for processing (3)
server/api/registry/analysis/[...pkg].get.tsserver/api/registry/install-size/[...pkg].get.tstest/unit/server/utils/parse-package-params.spec.ts
MatteoGabriele
commented
Mar 18, 2026
@gameroman, could you provide a more detailed description of what this PR aims to fix? The issue you mentioned links to a ticket on your fork, and there are only two links. It's a bit cryptic, even for future reference, and it would be better documented/tracked in this repository. |
gameroman
commented
Mar 18, 2026
Updated |
MatteoGabriele
commented
Mar 18, 2026
Here is what it's like for me ![]() Before: https://main.npmx.dev/compare?packages=@angular/core,vue After: https://npmx-qljid94b8-npmx.vercel.app/compare?packages=@angular/core,vue |
MatteoGabriele
commented
Mar 19, 2026
@gameroman, where did you find this link? 🤔 https://npmx-qljid94b8-npmx.vercel.app/compare?packages=@angular/core,vue |
MatteoGabriele
commented
Mar 19, 2026
btw I like the addition. I was wondering if it wouldn't make sense to include it directly within the parsePackageParams utility and add tests for when the string is actually encoded? What do you think? |
![]()
Yes, I think that makes sense, will check where this util is used at the moment |
MatteoGabriele
commented
Mar 19, 2026
@gameroman Oh wow, that's where that URL is from. I always click on the preview link in the Vercel bot comment. It's strange that those are two different links and give two different results. I wonder why 🤔
Thanks a lot 🙏 |
gameroman
commented
Mar 19, 2026
@coderabbitai what test should I add to make sure there is no regressions in the future? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me look at the relevant files to give you precise test recommendations. 🏁 Script executed: Length of output: 140 🏁 Script executed: Length of output: 12244
The core regression risk is a scoped package URL where the describe('encoded package names',()=>{it('parses fully encoded scoped package name without version',()=>{// e.g. URL: /api/registry/install-size/%40angular%2Fcore// Nitro does not split on %2F, so the whole thing arrives as one segmentconstsegments=['%40angular%2Fcore']constresult=parsePackageParams(segments)expect(result).toEqual({rawPackageName: '@angular/core',rawVersion: undefined,})})it('parses fully encoded scoped package name with version',()=>{// e.g. URL: /api/registry/install-size/%40angular%2Fcore/v/17.0.0constsegments=['%40angular%2Fcore','v','17.0.0']constresult=parsePackageParams(segments)expect(result).toEqual({rawPackageName: '@angular/core',rawVersion: '17.0.0',})})it('parses partially encoded scoped package name (only slash encoded)',()=>{// e.g. URL: /api/registry/install-size/@angular%2Fcore// @ is not encoded, but the inner slash is, so still one segmentconstsegments=['@angular%2Fcore']constresult=parsePackageParams(segments)expect(result).toEqual({rawPackageName: '@angular/core',rawVersion: undefined,})})it('parses partially encoded scoped package name with version',()=>{constsegments=['@angular%2Fcore','v','17.0.0']constresult=parsePackageParams(segments)expect(result).toEqual({rawPackageName: '@angular/core',rawVersion: '17.0.0',})})})These tests will only pass once |
gameroman
commented
Mar 19, 2026
@coderabbitai I am thinking more of an e2e test since I currently modified |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the existing E2E test structure to give you precise recommendations. 🏁 Script executed: Length of output: 18034 Let me check the existing E2E test patterns and mock setup: 🏁 Script executed: Length of output: 49280
Here's the approach that fits best with the existing test patterns in this project: Strategy: Use
Add these tests to test.describe('Compare Page',()=>{// ... existing tests ...test('loads install-size data for a scoped package',async({ page, goto })=>{// Intercept the internal API call the browser makes for install-size.// The browser will request /api/registry/install-size/@nuxt%2Fkit (encoded slash).// Before the fix this would fail to parse and return an error; after it returns 200.constinstallSizeResponse=page.waitForResponse(res=>res.url().includes('/api/registry/install-size/')&&res.url().includes('nuxt')&&res.request().method()==='GET',{timeout: 20_000},)awaitgoto('/compare?packages=@nuxt/kit,vue',{waitUntil: 'hydration'})constresponse=awaitinstallSizeResponseexpect(response.status()).toBe(200)constbody=awaitresponse.json()// The API should return a valid install size object, not an errorexpect(body).toHaveProperty('installSize')})test('loads analysis data for a scoped package',async({ page, goto })=>{constanalysisResponse=page.waitForResponse(res=>res.url().includes('/api/registry/analysis/')&&res.url().includes('nuxt')&&res.request().method()==='GET',{timeout: 20_000},)awaitgoto('/compare?packages=@nuxt/kit,vue',{waitUntil: 'hydration'})constresponse=awaitanalysisResponseexpect(response.status()).toBe(200)constbody=awaitresponse.json()expect(body).toHaveProperty('package','@nuxt/kit')})test('compare grid shows data (not all dashes) for a scoped package',async({
page,
goto,})=>{awaitgoto('/compare?packages=@nuxt/kit,vue',{waitUntil: 'hydration'})constgrid=page.locator('.comparison-grid')awaitexpect(grid).toBeVisible({timeout: 20_000})// Package size row should have a value for the scoped package column,// not a dash, which would indicate the API call failed before the fix.constpackageSizeRow=grid.locator('[data-facet="packageSize"]')awaitexpect(packageSizeRow).toBeVisible({timeout: 15_000})awaitexpect(packageSizeRow.locator('.comparison-cell').first()).not.toContainText('-')})})A few notes:
|
gameroman
commented
Mar 19, 2026
@MatteoGabriele I've checked and the util is used several places and some might not need the decoding logic so I'm thinking I'll do that in a follow-up PR, if that's okay? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/interactions.spec.ts (1)
68-71: Consider asserting thepackageproperty for consistency.The analysis test (lines 87-88) asserts
expect(body).toHaveProperty('package', '@nuxt/kit')to verify the scoped package name was correctly parsed. Adding the same assertion here would strengthen the test and maintain consistency between the two API tests.💡 Suggested improvement
const body = await response.json() // The API should return a valid install size object, not an error expect(body).toHaveProperty('installSize') + expect(body).toHaveProperty('package', '@nuxt/kit') })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1074fe2-be46-4ba6-bb41-4e829387b301
📒 Files selected for processing (1)
test/e2e/interactions.spec.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gameroman
commented
Mar 19, 2026
Ready to be merged |
MatteoGabriele
left a comment
There was a problem hiding this comment.
LGTM ✨
Thanks for the great work and for being so responsive to feedback!



🔗 Linked issue
Fixes#2141
📚 Description
Fixes fetching data on compare page with scoped packages
Before
After