fix: correctly parse package name for scoped packages#2135
fix: correctly parse package name for scoped packages#2135MatteoGabriele merged 6 commits intonpmx-dev:mainfrom
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
|
gameroman
changed the title
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
@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. |
Updated |
|
Judging by the title, I expected to see some artifacts in the stats, but they look the same unless I'm missing something. 
|
|
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 |
|
@gameroman, where did you find this link? 🤔 https://npmx-qljid94b8-npmx.vercel.app/compare?packages=@angular/core,vue |
|
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 |
|
@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 🙏 |
|
@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 segment
const segments = ['%40angular%2Fcore']
const result = 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.0
const segments = ['%40angular%2Fcore', 'v', '17.0.0']
const result = 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 segment
const segments = ['@angular%2Fcore']
const result = parsePackageParams(segments)
expect(result).toEqual({
rawPackageName: '@angular/core',
rawVersion: undefined,
})
})
it('parses partially encoded scoped package name with version', () => {
const segments = ['@angular%2Fcore', 'v', '17.0.0']
const result = parsePackageParams(segments)
expect(result).toEqual({
rawPackageName: '@angular/core',
rawVersion: '17.0.0',
})
})
})These tests will only pass once |
|
@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.
const installSizeResponse = page.waitForResponse(
res =>
res.url().includes('/api/registry/install-size/') &&
res.url().includes('nuxt') &&
res.request().method() === 'GET',
{ timeout: 20_000 },
)
await goto('/compare?packages=@nuxt/kit,vue', { waitUntil: 'hydration' })
const response = await installSizeResponse
expect(response.status()).toBe(200)
const body = await response.json()
// The API should return a valid install size object, not an error
expect(body).toHaveProperty('installSize')
})
test('loads analysis data for a scoped package', async ({ page, goto }) => {
const analysisResponse = page.waitForResponse(
res =>
res.url().includes('/api/registry/analysis/') &&
res.url().includes('nuxt') &&
res.request().method() === 'GET',
{ timeout: 20_000 },
)
await goto('/compare?packages=@nuxt/kit,vue', { waitUntil: 'hydration' })
const response = await analysisResponse
expect(response.status()).toBe(200)
const body = await response.json()
expect(body).toHaveProperty('package', '@nuxt/kit')
})
test('compare grid shows data (not all dashes) for a scoped package', async ({
page,
goto,
}) => {
await goto('/compare?packages=@nuxt/kit,vue', { waitUntil: 'hydration' })
const grid = page.locator('.comparison-grid')
await expect(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.
const packageSizeRow = grid.locator('[data-facet="packageSize"]')
await expect(packageSizeRow).toBeVisible({ timeout: 15_000 })
await expect(packageSizeRow.locator('.comparison-cell').first()).not.toContainText('-')
})
})A few notes:
|
@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
|
Ready to be merged |
MatteoGabriele
left a comment
MatteoGabriele
left a comment
There was a problem hiding this comment.
LGTM ✨
Thanks for the great work and for being so responsive to feedback!
2 participants
🔗 Linked issue
Fixes #2141
📚 Description
Fixes fetching data on compare page with scoped packages
Before
After