Skip to content

fix: ignore license change for initial version#2816

Open
charpeni wants to merge 4 commits into
npmx-dev:mainfrom
charpeni:fix/license-change-first-version
Open

fix: ignore license change for initial version#2816
charpeni wants to merge 4 commits into
npmx-dev:mainfrom
charpeni:fix/license-change-first-version

Conversation

@charpeni
Copy link
Copy Markdown

🔗 Linked issue

Resolves #2720

🧭 Context

The license-change API reported a license change for a package’s first published version because it compared that version against a missing previous version, which was normalized to UNKNOWN.

📚 Description

This updates the license-change endpoint to return no change when the selected version has no previous version to compare against.

It also adds focused unit coverage for:

  • first package version does not report a license change
  • later package version still reports an actual license change

Tested with:

pnpm test --project unit test/unit/server/api/registry/license-change/pkg.get.spec.ts

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 29, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment May 30, 2026 3:49pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview May 30, 2026 3:49pm
npmx-lunaria Ignored Ignored May 30, 2026 3:49pm

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2515567a-dab9-46c6-a782-3f6976b27ceb

📥 Commits

Reviewing files that changed from the base of the PR and between db425d8 and f78042b.

📒 Files selected for processing (2)
  • shared/utils/npm.ts
  • test/unit/shared/utils/npm.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • test/unit/shared/utils/npm.spec.ts
  • shared/utils/npm.ts

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Licence-change detection correctly handles initial releases and object-shaped licence fields, avoiding false positives.
    • Licence values shown in package metadata, timelines and badges are consistently normalised for clearer, predictable display.
  • Tests

    • Added unit tests for licence normalisation and badge rendering to ensure accurate detection and consistent display across scenarios.

Walkthrough

Adds a shared normalizePackageLicense util, replaces ad-hoc license extraction across composables and server endpoints with it, adds an early-return guard in the license-change handler for packages without a previous version, and extends unit tests for badge rendering and license-change comparisons (including object-shaped licences).

Changes

License normalisation rollout

Layer / File(s) Summary
Utility and tests
shared/utils/npm.ts, test/unit/shared/utils/npm.spec.ts
Adds exported normalizePackageLicense(license) and unit tests covering string, legacy object { type }, and invalid/empty inputs.
Client composables
app/composables/npm/usePackage.ts, app/composables/usePackageComparison.ts
Replaces inline license handling with normalizePackageLicense(...) when transforming packument data and populating metadata.license.
Server endpoints: package-meta, badge, timeline
server/api/registry/package-meta/[...pkg].get.ts, server/api/registry/badge/[type]/[...pkg].get.ts, server/api/registry/timeline/[...pkg].get.ts
Use normalizePackageLicense(...) for package-level license, badge license value, and timeline entry license fields.
Handler: license-change
server/api/registry/license-change/[...pkg].get.ts
Import and use normalizePackageLicense for current/previous comparisons; add early return when previousVersionIndex < 0 to avoid accessing a non-existent prior version.
Tests: badge and license-change
test/unit/server/api/registry/badge/pkg.get.spec.ts, test/unit/server/api/registry/license-change/pkg.get.spec.ts
Adds/updates Vitest suites to assert badge SVG contains normalised license (not [object Object]) and that license-change comparisons treat object-shaped license { type: 'X' } as equivalent to string licenses.

Possibly related PRs

  • npmx-dev/npmx.dev#2662: Also modified app/composables/npm/usePackage.ts to change how packument license is normalised; likely overlaps in client-side license handling.

Suggested reviewers

  • ghostdevv
  • 43081j
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ❓ Inconclusive Additional changes refactoring license normalization to a shared utility function are related improvements supporting the core fix but go beyond the minimal scope. Clarify whether the widespread refactoring of license normalization across multiple files is intentional scope expansion or necessary to support the primary fix.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: ignoring license changes for initial versions.
Description check ✅ Passed The description provides relevant context about the bug fix, including the linked issue, the problem, and the solution implemented.
Linked Issues check ✅ Passed The PR fully addresses issue #2720 by preventing false-positive license change reports for initial package versions through early return logic.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

Hello! Thank you for opening your first PR to npmx, @charpeni! 🚀

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any issues you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Vercel

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
shared/utils/npm.ts 50.00% 0 Missing and 2 partials ⚠️
server/api/registry/license-change/[...pkg].get.ts 75.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
server/api/registry/license-change/[...pkg].get.ts (1)

41-49: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate currentVersionIndex before array access.

The currentVersionIndex can be -1 in two scenarios: (1) when version === 'latest' and the versions array is empty, or (2) when findIndex fails to locate the requested version. Accessing versions[currentVersionIndex] on line 49 with a negative index violates type-safety. As per coding guidelines, always check when accessing an array value by index.

🛡️ Proposed fix to validate currentVersionIndex
 const currentVersionIndex =
   version === 'latest' ? versions.length - 1 : versions.findIndex(v => v.version === version)

+if (currentVersionIndex < 0) {
+  throw createError({
+    statusCode: 404,
+    statusMessage: 'Version not found',
+  })
+}
+
 const previousVersionIndex = currentVersionIndex - 1
 if (previousVersionIndex < 0) {
   return { change }
 }
🤖 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 41 - 49,
currentVersionIndex can be -1 (empty versions or findIndex miss), so before
accessing versions[currentVersionIndex] in the assignment to currentLicense,
validate currentVersionIndex is within [0, versions.length-1]; if it's -1 or out
of range, set currentLicense to 'UNKNOWN' (or the existing fallback) and
proceed—update the logic around the currentVersionIndex calculation and the
currentLicense assignment to guard against negative indices and avoid unsafe
array access.
🤖 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.

Outside diff comments:
In `@server/api/registry/license-change/`[...pkg].get.ts:
- Around line 41-49: currentVersionIndex can be -1 (empty versions or findIndex
miss), so before accessing versions[currentVersionIndex] in the assignment to
currentLicense, validate currentVersionIndex is within [0, versions.length-1];
if it's -1 or out of range, set currentLicense to 'UNKNOWN' (or the existing
fallback) and proceed—update the logic around the currentVersionIndex
calculation and the currentLicense assignment to guard against negative indices
and avoid unsafe array access.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 250d627f-c1e0-495f-b578-54f044d06a98

📥 Commits

Reviewing files that changed from the base of the PR and between b3ba515 and e2a9cc5.

📒 Files selected for processing (2)
  • server/api/registry/license-change/[...pkg].get.ts
  • test/unit/server/api/registry/license-change/pkg.get.spec.ts

Copy link
Copy Markdown
Member

@serhalp serhalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

Comment thread test/unit/server/api/registry/license-change/pkg.get.spec.ts
Comment thread server/api/registry/license-change/[...pkg].get.ts Outdated
Comment thread shared/utils/npm.ts Outdated
@gameroman gameroman added the needs review This PR is waiting for a review from a maintainer label May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review This PR is waiting for a review from a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package page shows license changed warning for new package

3 participants