fix: don't flag a license change when there is no previous version#2792
Draft
jp-knj wants to merge 1 commit into
Draft
fix: don't flag a license change when there is no previous version#2792jp-knj wants to merge 1 commit into
jp-knj wants to merge 1 commit into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
📝 WalkthroughWalkthroughThis PR fixes the license-change endpoint to only report license changes when a valid prior version exists, addressing a bug where single-version packages incorrectly showed change warnings. The handler is guarded with a version-index check, and comprehensive tests validate the corrected behaviour across multiple scenarios. ChangesLicense-change handler fix and validation
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Hello! Thank you for opening your first PR to npmx, @jp-knj! 🚀 Here’s what will happen next:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/api/registry/license-change/`[...pkg].get.ts:
- Around line 50-51: Normalize license values the same way as the timeline
handler: instead of using String(...) for currentLicense and previousLicense,
detect if versions[currentVersionIndex]?.license or
versions[previousVersionIndex]?.license is an object and, if so, use its .type
property (fallback to 'UNKNOWN' if missing); otherwise use the string value.
Update the assignments that set currentLicense and previousLicense (referencing
versions, currentVersionIndex, previousVersionIndex) to perform this object
check and extraction so change detection reports the actual license.type rather
than "[object Object]".
In `@test/unit/server/api/registry/license-change/pkg.get.spec.ts`:
- Around line 37-190: Add a new test in this spec to exercise object-shaped
licenses: create a case (similar to the suggested snippet) that sets routerParam
= 'my-pkg', mocks fetchNpmPackageMock via makePackument to return versions where
license values are objects (e.g. { type: 'MIT' } and { type: 'Apache-2.0', url:
'...' }), call handler(fakeEvent), and assert the returned change equals { from:
'MIT', to: 'Apache-2.0' }; this ensures the handler's license normalization
logic (used when reading packument versions) correctly extracts the type field
from object licenses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0f1dfca-89e7-461a-88fa-6539b5ef9d3d
📒 Files selected for processing (2)
server/api/registry/license-change/[...pkg].get.tstest/unit/server/api/registry/license-change/pkg.get.spec.ts
Comment on lines
+50
to
+51
| const currentLicense = String(versions[currentVersionIndex]?.license || 'UNKNOWN') | ||
| const previousLicense = String(versions[previousVersionIndex]?.license || 'UNKNOWN') |
Contributor
There was a problem hiding this comment.
Handle license objects consistently with the timeline handler.
The String() conversion doesn't properly handle licenses that are objects (e.g., { type: 'MIT', url: '...' }). When license is an object, String(license) produces "[object Object]" rather than extracting the type field. This could result in incorrect change detection or misleading from/to values.
The timeline handler (server/api/registry/timeline/[...pkg].get.ts:66-69) already implements the correct normalization pattern: check if the license is an object and extract the .type property.
🔧 Proposed fix to normalize license objects
if (currentVersionIndex > 0) {
- const currentLicense = String(versions[currentVersionIndex]?.license || 'UNKNOWN')
- const previousLicense = String(versions[previousVersionIndex]?.license || 'UNKNOWN')
+ let currentLicense = versions[currentVersionIndex]?.license
+ if (currentLicense && typeof currentLicense === 'object' && 'type' in currentLicense) {
+ currentLicense = currentLicense.type
+ }
+ const currentLicenseStr = typeof currentLicense === 'string' ? currentLicense : 'UNKNOWN'
+
+ let previousLicense = versions[previousVersionIndex]?.license
+ if (previousLicense && typeof previousLicense === 'object' && 'type' in previousLicense) {
+ previousLicense = previousLicense.type
+ }
+ const previousLicenseStr = typeof previousLicense === 'string' ? previousLicense : 'UNKNOWN'
- if (currentLicense !== previousLicense) {
+ if (currentLicenseStr !== previousLicenseStr) {
change = {
- from: previousLicense,
- to: currentLicense,
+ from: previousLicenseStr,
+ to: currentLicenseStr,
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const currentLicense = String(versions[currentVersionIndex]?.license || 'UNKNOWN') | |
| const previousLicense = String(versions[previousVersionIndex]?.license || 'UNKNOWN') | |
| let currentLicense = versions[currentVersionIndex]?.license | |
| if (currentLicense && typeof currentLicense === 'object' && 'type' in currentLicense) { | |
| currentLicense = currentLicense.type | |
| } | |
| const currentLicenseStr = typeof currentLicense === 'string' ? currentLicense : 'UNKNOWN' | |
| let previousLicense = versions[previousVersionIndex]?.license | |
| if (previousLicense && typeof previousLicense === 'object' && 'type' in previousLicense) { | |
| previousLicense = previousLicense.type | |
| } | |
| const previousLicenseStr = typeof previousLicense === 'string' ? previousLicense : 'UNKNOWN' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/api/registry/license-change/`[...pkg].get.ts around lines 50 - 51,
Normalize license values the same way as the timeline handler: instead of using
String(...) for currentLicense and previousLicense, detect if
versions[currentVersionIndex]?.license or
versions[previousVersionIndex]?.license is an object and, if so, use its .type
property (fallback to 'UNKNOWN' if missing); otherwise use the string value.
Update the assignments that set currentLicense and previousLicense (referencing
versions, currentVersionIndex, previousVersionIndex) to perform this object
check and extraction so change detection reports the actual license.type rather
than "[object Object]".
Contributor
There was a problem hiding this comment.
I think you can use normalizeLicense from
Contributor
There was a problem hiding this comment.
@gameroman Here's the confirmed plan. A couple of findings from the investigation:
-
PackumentLicenseinusePackage.tsis only used by the localnormalizeLicense— so that import can be removed along with the function. -
Nuxt auto-imports only covers
~/composables/**(notshared/utils/), so the handler will need an explicit import — consistent with what other server files already do (e.g.import { ... } from '#shared/utils/constants'). -
Export
normalizeLicensefromshared/utils/npm.ts -
Remove the local definition (and now-unused
PackumentLicenseimport) fromapp/composables/npm/usePackage.ts -
Add an explicit import and use
normalizeLicensein the license-change handler
🧠 Learnings used
Learnt from: Moshyfawn
Repo: npmx-dev/npmx.dev PR: 1796
File: server/api/registry/badge/[type]/[...pkg].get.ts:419-431
Timestamp: 2026-03-01T19:21:58.834Z
Learning: In the npmx.dev codebase, handleApiError is defined with a never return type and always throws. When calling this function, do not add an explicit throw or return after the call; TypeScript's control-flow analysis recognizes it as non-returning. This guidance applies to all TS files that call handleApiError and should be checked wherever the function is used (e.g., places importing and invoking it).
Learnt from: serhalp
Repo: npmx-dev/npmx.dev PR: 2459
File: test/unit/server/utils/likes-leaderboard.spec.ts:137-165
Timestamp: 2026-04-26T00:02:20.876Z
Learning: In this Nuxt project (npmx-dev/npmx.dev), the `Packument` type is globally available via Nuxt auto-imports from `shared/types/` (exported from `shared/types/npm-registry.ts`). Therefore, do not raise or require missing `import type { Packument } from '`#shared/types`'` (or any equivalent) when `Packument` is referenced, including in unit test files.
Learnt from: WilcoSp
Repo: npmx-dev/npmx.dev PR: 2717
File: server/api/changelog/md/[provider]/[owner]/[repo]/[...path].get.ts:34-34
Timestamp: 2026-05-15T07:45:36.306Z
Learning: HTTP 5xx status-code convention in the npmx-dev/npmx.dev codebase: use 502 exclusively when the upstream API is exhausted due to upstream API key/rate limits (e.g., throw/return ERROR_UNGH_API_KEY_EXHAUSTED). For all other server-side errors, use 500. During code review, treat existing 500 usage in changelog and other API endpoints as intentional—do not flag it as incorrect unless the error specifically corresponds to the upstream key/rate-limit exhaustion case that should map to 502.
Failed to handle agent chat message. Please try again.
Comment on lines
+37
to
+190
| describe('license-change API', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| routerParam = undefined | ||
| queryParams = {} | ||
| }) | ||
|
|
||
| it('throws 400 when package name param is missing', async () => { | ||
| routerParam = undefined | ||
| await expect(handler(fakeEvent)).rejects.toMatchObject({ statusCode: 400 }) | ||
| }) | ||
|
|
||
| it('reports no change for a new package with a single version (issue #2720)', async () => { | ||
| routerParam = 'vsxtools' | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { '0.0.1': { license: 'MIT' } }, | ||
| time: { '0.0.1': '2024-01-01T00:00:00Z' }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toBeNull() | ||
| }) | ||
|
|
||
| it('reports a change when the license changed between versions', async () => { | ||
| routerParam = 'my-pkg' | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { | ||
| '1.0.0': { license: 'MIT' }, | ||
| '2.0.0': { license: 'ISC' }, | ||
| }, | ||
| time: { | ||
| '1.0.0': '2024-01-01T00:00:00Z', | ||
| '2.0.0': '2024-06-01T00:00:00Z', | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toEqual({ from: 'MIT', to: 'ISC' }) | ||
| }) | ||
|
|
||
| it('reports no change when the license is unchanged between versions', async () => { | ||
| routerParam = 'my-pkg' | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { | ||
| '1.0.0': { license: 'MIT' }, | ||
| '2.0.0': { license: 'MIT' }, | ||
| }, | ||
| time: { | ||
| '1.0.0': '2024-01-01T00:00:00Z', | ||
| '2.0.0': '2024-06-01T00:00:00Z', | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toBeNull() | ||
| }) | ||
|
|
||
| it('defaults to the latest (chronologically newest) version', async () => { | ||
| routerParam = 'my-pkg' | ||
| queryParams = {} | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { | ||
| '1.0.0': { license: 'MIT' }, | ||
| '2.0.0': { license: 'MIT' }, | ||
| '3.0.0': { license: 'Apache-2.0' }, | ||
| }, | ||
| time: { | ||
| '1.0.0': '2024-01-01T00:00:00Z', | ||
| '2.0.0': '2024-06-01T00:00:00Z', | ||
| '3.0.0': '2025-01-01T00:00:00Z', | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toEqual({ from: 'MIT', to: 'Apache-2.0' }) | ||
| }) | ||
|
|
||
| it('compares the requested version against its predecessor', async () => { | ||
| routerParam = 'my-pkg' | ||
| queryParams = { version: '2.0.0' } | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { | ||
| '1.0.0': { license: 'MIT' }, | ||
| '2.0.0': { license: 'ISC' }, | ||
| '3.0.0': { license: 'Apache-2.0' }, | ||
| }, | ||
| time: { | ||
| '1.0.0': '2024-01-01T00:00:00Z', | ||
| '2.0.0': '2024-06-01T00:00:00Z', | ||
| '3.0.0': '2025-01-01T00:00:00Z', | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toEqual({ from: 'MIT', to: 'ISC' }) | ||
| }) | ||
|
|
||
| it('reports no change when the requested version is the oldest', async () => { | ||
| routerParam = 'my-pkg' | ||
| queryParams = { version: '1.0.0' } | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { | ||
| '1.0.0': { license: 'MIT' }, | ||
| '2.0.0': { license: 'ISC' }, | ||
| }, | ||
| time: { | ||
| '1.0.0': '2024-01-01T00:00:00Z', | ||
| '2.0.0': '2024-06-01T00:00:00Z', | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toBeNull() | ||
| }) | ||
|
|
||
| it('reports no change when the requested version is not found', async () => { | ||
| routerParam = 'my-pkg' | ||
| queryParams = { version: '9.9.9' } | ||
|
|
||
| fetchNpmPackageMock.mockResolvedValue( | ||
| makePackument({ | ||
| versions: { | ||
| '1.0.0': { license: 'MIT' }, | ||
| '2.0.0': { license: 'ISC' }, | ||
| }, | ||
| time: { | ||
| '1.0.0': '2024-01-01T00:00:00Z', | ||
| '2.0.0': '2024-06-01T00:00:00Z', | ||
| }, | ||
| }), | ||
| ) | ||
|
|
||
| const result = await handler(fakeEvent) | ||
| expect(result.change).toBeNull() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for license objects.
The test suite thoroughly validates the guard logic for single-version, multi-version, and version-not-found cases. However, it doesn't cover the scenario where a license is an object (e.g., { type: 'MIT' }) rather than a string. According to shared/types/npm-registry.ts, PackumentLicense can be string | { type: string; url?: string }.
Adding a test case for object licenses would ensure the handler normalizes them correctly (extracting the type field) and prevent regressions.
📋 Suggested test case for license objects
it('handles license objects by extracting the type field', async () => {
routerParam = 'my-pkg'
fetchNpmPackageMock.mockResolvedValue(
makePackument({
versions: {
'1.0.0': { license: { type: 'MIT' } },
'2.0.0': { license: { type: 'Apache-2.0', url: 'https://...' } },
},
time: {
'1.0.0': '2024-01-01T00:00:00Z',
'2.0.0': '2024-06-01T00:00:00Z',
},
}),
)
const result = await handler(fakeEvent)
expect(result.change).toEqual({ from: 'MIT', to: 'Apache-2.0' })
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/server/api/registry/license-change/pkg.get.spec.ts` around lines 37
- 190, Add a new test in this spec to exercise object-shaped licenses: create a
case (similar to the suggested snippet) that sets routerParam = 'my-pkg', mocks
fetchNpmPackageMock via makePackument to return versions where license values
are objects (e.g. { type: 'MIT' } and { type: 'Apache-2.0', url: '...' }), call
handler(fakeEvent), and assert the returned change equals { from: 'MIT', to:
'Apache-2.0' }; this ensures the handler's license normalization logic (used
when reading packument versions) correctly extracts the type field from object
licenses.
jp-knj
added a commit
to jp-knj/npmx.dev
that referenced
this pull request
May 25, 2026
The license field can be an object ({ type, url }), where String() would
yield "[object Object]". Reuse normalizeLicense (moved from usePackage to
shared/utils/npm) to extract the type, so change detection compares real
license values. Adds an object-license test case.
Addresses review feedback on npmx-dev#2792.
jp-knj
force-pushed
the
fix/2720-license-change-no-previous-version
branch
from
2d1cf32 to
8181098
Compare
…rsion
Don't flag a license change when there's no real previous version (new
single-version packages were diffing against a phantom 'UNKNOWN'), and
normalize object-shaped licenses ({ type, url }) so comparisons use the
real value instead of "[object Object]". Adds unit tests.
Closes npmx-dev#2720
jp-knj
force-pushed
the
fix/2720-license-change-no-previous-version
branch
from
8181098 to
b0e32ce
Compare
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
2 participants
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.
Fixes #2720.
Summary
Fix false-positive license change warnings for packages that have no previous version.
Added unit tests for the new-package case and normal license-change comparisons.
Test plan
test/unit/server/api/registry/license-change/pkg.get.spec.tspnpm lintcleanpnpm test:typespasses/package/vsxtools/v/0.0.1no longer shows a license-change warning; a multi-version package with an actual license change still does