Skip to content

Comments

refactor: use semver instead of manual implementations#48

Merged
9romise merged 2 commits intomainfrom
refactor/semver
Feb 25, 2026
Merged

refactor: use semver instead of manual implementations#48
9romise merged 2 commits intomainfrom
refactor/semver

Conversation

@9romise
Copy link
Member

@9romise 9romise commented Feb 25, 2026

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37882cb and 4275192.

📒 Files selected for processing (1)
  • src/providers/diagnostics/rules/upgrade.ts

📝 Walkthrough

Walkthrough

This pull request adds the semver runtime package and its TypeScript types, removes three custom version helpers (getPrereleaseId, comparePrereleasePrecedence, lt) from src/utils/version.ts, updates diagnostics rules to use semver functions (prerelease, gtr, ltr and semver/functions/lt), updates tests to remove coverage for the removed helpers, and configures semver to be inlined during bundling.

Possibly related PRs

  • npmx-dev/vscode-npmx PR 36: Refactors the upgrade diagnostic to replace local version helpers with semver functions, touching the same upgrade diagnostic code.
  • npmx-dev/vscode-npmx PR 30: Modifies the vulnerability diagnostics rule, replacing local utilities with semver-based logic in the same file updated here.
  • npmx-dev/vscode-npmx PR 29: Changes version-handling in provider code and aligns with replacing local utilities by semver-based parsing used in this PR.
🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request lacks any author-provided description, making it impossible to assess whether the intent is clearly communicated to reviewers. Add a pull request description that explains the refactoring goals, benefits of using the semver package, and any breaking changes or migration notes for maintainers.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/semver

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.

Copy link
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)
src/providers/diagnostics/rules/upgrade.ts (1)

30-42: ⚠️ Potential issue | 🔴 Critical

Array reference comparison silently disables all prerelease upgrade hints.

semver.prerelease() returns an array of prerelease components (e.g. prerelease('1.2.3-alpha.1') -> ['alpha', 1]), or null if none exist.

On Line 37, prerelease(tagVersion) !== currentPreId compares two separate array instances with reference equality. These are always distinct object references, so the condition is always true, causing the loop to continue on every iteration — meaning createUpgradeDiagnostic is never reached for any dist-tag. All prerelease upgrade diagnostics are silently suppressed.

The prior getPrereleaseId returned the first prerelease component as a string, which supported scalar ===/!== comparison. The fix is to compare only the first element (the channel identifier, e.g. 'beta'), preserving the original semantics:

🐛 Proposed fix
-  const currentPreId = prerelease(semver)
+  const currentPreId = prerelease(semver)?.[0]
   if (!currentPreId)
     return

   for (const [tag, tagVersion] of Object.entries(pkg.distTags)) {
     if (tag === 'latest')
       continue
-    if (prerelease(tagVersion) !== currentPreId)
+    if (prerelease(tagVersion)?.[0] !== currentPreId)
       continue
     if (!lt(semver, tagVersion))
       continue

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31ac4b6 and 37882cb.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • package.json
  • pnpm-workspace.yaml
  • src/providers/diagnostics/rules/upgrade.ts
  • src/providers/diagnostics/rules/vulnerability.ts
  • src/utils/version.ts
  • tests/utils/version.test.ts
  • tsdown.config.ts
💤 Files with no reviewable changes (1)
  • src/utils/version.ts

@9romise 9romise merged commit 3778859 into main Feb 25, 2026
9 checks passed
@9romise 9romise deleted the refactor/semver branch February 25, 2026 09:07
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.

1 participant