Skip to content

fix: don't show license changed warning for first package versions#2723

Closed
ProCode-Software wants to merge 9 commits into
npmx-dev:mainfrom
ProCode-Software:patch-1
Closed

fix: don't show license changed warning for first package versions#2723
ProCode-Software wants to merge 9 commits into
npmx-dev:mainfrom
ProCode-Software:patch-1

Conversation

@ProCode-Software
Copy link
Copy Markdown
Contributor

Add check for previous version index to avoid displaying 'UNKNOWN' as the last version when there were no previous versions.

🔗 Linked issue

Resolves #2720.

🧭 Context

The previous functionality would index the package versions before the current version, even if it is 0. This would result in the previous version being -1 and the default value of "UNKNOWN" being returned as the previous version license.

Added a return if previousVersionIndex < 0

📚 Description

For new packages, a license changed warning would be displayed:

The license of this package changed from "UNKNOWN" to "MIT".

There were no prior versions, however, the current functionality claims its license was UNKNOWN.

Add check for previous version index to avoid displaying 'UNKNOWN' as the last version when there were no previous versions.
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 11, 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 8:26pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview May 30, 2026 8:26pm
npmx-lunaria Ignored Ignored May 30, 2026 8:26pm

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 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: e1af3f58-3e77-4cb1-b8ba-e430a48921ce

📥 Commits

Reviewing files that changed from the base of the PR and between 6f6fb7a and a8bd34b.

📒 Files selected for processing (3)
  • app/composables/npm/usePackage.ts
  • server/api/registry/license-change/[...pkg].get.ts
  • test/unit/server/api/registry/license-change.spec.ts
💤 Files with no reviewable changes (1)
  • test/unit/server/api/registry/license-change.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/api/registry/license-change/[...pkg].get.ts

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling for requests targeting unavailable package versions (now returns a clear 400).
    • More robust license-change detection with consistent normalisation of licence values, correct handling when there is no prior version, and better single-version behaviour.
  • Tests

    • Added a comprehensive unit test suite covering licence-change scenarios, edge cases and error paths.

Walkthrough

Exports normalizeLicense, adds version-index validation and license normalisation to the license-change handler (400 for unknown version, early return when no previous version), and introduces Vitest tests covering errors and edge cases.

Changes

License change export, handler guards and tests

Layer / File(s) Summary
Export normalizeLicense
app/composables/npm/usePackage.ts
Makes normalizeLicense exported so other modules can import it.
Handler: import normalize + version/index guards
server/api/registry/license-change/[...pkg].get.ts
Imports normalizeLicense; throws 400 when requested version isn't found; returns early when no previous version exists; normalises current and previous license values before comparing.
Unit tests for license-change handler
test/unit/server/api/registry/license-change.spec.ts
Adds Vitest tests and stubs (bootstrapping, makePackument, fake H3Event). Covers missing params (400), missing upstream metadata (404), fetch errors (500), license-change detection, unchanged -> null, single-version regression, chronological previous-version selection, default-to-latest behaviour, and UNKNOWN normalisation.

Possibly related PRs

  • npmx-dev/npmx.dev#2662: Changes touching normalizeLicense and related license derivation/normalisation logic.

Suggested reviewers

  • ghostdevv
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: preventing misleading license change warnings for packages with no prior versions.
Description check ✅ Passed The description is relevant to the changeset, explaining the bug, context, and the fix applied to prevent showing 'UNKNOWN' as a prior license.
Linked Issues check ✅ Passed The PR successfully addresses issue #2720 by adding a guard to return early when previousVersionIndex < 0, preventing false license-change warnings for first-version packages.
Out of Scope Changes check ✅ Passed All changes are scoped to the bug fix: handler logic improvements, comprehensive unit tests, and exporting a normalisation helper already used in the codebase.

✏️ 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, @ProCode-Software! 🚀

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.

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.

Actionable comments posted: 1

🤖 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 45-47: The current guard returns { change } when
previousVersionIndex < 0 which lets a missing requested version
(currentVersionIndex === -1) slip through; update the logic in the request
handler to first check if currentVersionIndex < 0 and return a 404/appropriate
error (or throw) for unknown version before checking previousVersionIndex, then
only proceed to access the versions array and compute change if
currentVersionIndex and previousVersionIndex are valid; ensure you reference and
protect accesses to versions[currentVersionIndex] and
versions[previousVersionIndex] to satisfy type-safety.
🪄 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: c6736d7d-c7bc-478a-ac3a-def91955178d

📥 Commits

Reviewing files that changed from the base of the PR and between 92cbee7 and d6b5762.

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

Comment thread server/api/registry/license-change/[...pkg].get.ts
@ProCode-Software ProCode-Software changed the title fix(api): don't show license changed warning for first package versions fix: don't show license changed warning for first package versions May 11, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

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

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

📢 Thoughts on this report? Let us know!

Add error handling for invalid version requests
Copy link
Copy Markdown
Contributor Author

@ProCode-Software ProCode-Software left a comment

Choose a reason for hiding this comment

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

Added logic for checking if a version wasn't found for a package

Copy link
Copy Markdown
Contributor

@gameroman gameroman left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Could you add a regression test please?

ProCode-Software added a commit to ProCode-Software/npmx.dev that referenced this pull request May 12, 2026
@ProCode-Software
Copy link
Copy Markdown
Contributor Author

Done!

@ghostdevv
Copy link
Copy Markdown
Member

Done!

I think you pushed it to the wrong branch 😄

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.

Actionable comments posted: 1

🤖 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 `@test/unit/server/api/registry/license-change.spec.ts`:
- Around line 175-197: The test expects the handler to extract the "type" from
an object-format license but currently asserts the incorrect string '[object
Object]'; update the expectation in the test (the handler call in this spec) so
that result.change.from equals 'Apache-2.0' (matching the license.type provided
by makePackument) while keeping result.change.to as 'ISC' — this will validate
the normalization behavior for object-format licenses handled by the handler
used in this spec.
🪄 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: 082dc3fb-2e44-427a-b065-6fdf39b12bbb

📥 Commits

Reviewing files that changed from the base of the PR and between 31ef8fd and 5019f87.

📒 Files selected for processing (1)
  • test/unit/server/api/registry/license-change.spec.ts

Comment thread test/unit/server/api/registry/license-change.spec.ts Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@ProCode-Software
Copy link
Copy Markdown
Contributor Author

ProCode-Software commented May 12, 2026

Oops, I fixed it now.

@ProCode-Software
Copy link
Copy Markdown
Contributor Author

npm's website says the license field specified as an object is deprecated, and present only in old packages. The code currently doesn't handle that, so I removed the test case for now.

@gameroman
Copy link
Copy Markdown
Contributor

npm's website says the license field specified as an object is deprecated, and present only in old packages. The code currently doesn't handle that, so I removed the test case for now.

I think you can use normalizeLicense from

function normalizeLicense(license?: PackumentLicense): string | undefined {
to handle that part. You could probably put it in some shared utils file

Copy link
Copy Markdown
Member

@ghostdevv ghostdevv left a comment

Choose a reason for hiding this comment

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

Could you take a look at Roman's comment (no rush)?

@ProCode-Software
Copy link
Copy Markdown
Contributor Author

I exported normalizeLicense in usePackage.ts (the same path you mentioned earlier), and I'm using that now.

@gameroman
Copy link
Copy Markdown
Contributor

I exported normalizeLicense in usePackage.ts (the same path you mentioned earlier), and I'm using that now.

could you also make sure the ci passes, thank you ❤️

@ProCode-Software
Copy link
Copy Markdown
Contributor Author

I exported normalizeLicense in usePackage.ts (the same path you mentioned earlier), and I'm using that now.

could you also make sure the ci passes, thank you ❤️

Why is type check failing? I synced usePackage.ts with main?

@ghostdevv
Copy link
Copy Markdown
Member

Hey, thanks for the PR! We got like four for this so we're gonna close this in favor of #2792 and copy over any changes we can from here to there - we'll make sure you get credit dw 😄

@ghostdevv ghostdevv closed this May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package page shows license changed warning for new package

3 participants