Skip to content

feat: add vulnerability quick-fix + hint#39

Open
nitodeco wants to merge 8 commits intonpmx-dev:mainfrom
nitodeco:vuln-quick-fix
Open

feat: add vulnerability quick-fix + hint#39
nitodeco wants to merge 8 commits intonpmx-dev:mainfrom
nitodeco:vuln-quick-fix

Conversation

@nitodeco
Copy link
Collaborator

@nitodeco nitodeco commented Feb 9, 2026

Closes #24

marked 16 1 1, This version has 1 high, 2 moderate vulnerabilities  Upgrade to 16 1 5 to

@nitodeco nitodeco requested a review from 9romise February 9, 2026 23:24
@coderabbitai
Copy link

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

Adds a VulnerabilityCodeActionProvider that offers QuickFix actions to replace vulnerable dependency versions with a parsed "fixed in" version; registers the provider conditionally when diagnostics.vulnerability is enabled; extends vulnerability diagnostics to compute and include a best fixedIn version and to append an upgrade hint to diagnostic messages; adds an optional fixedIn?: string field to the vulnerability API type; includes tests for the code actions and a test alias for #utils.

Possibly related PRs

  • npmx-dev/vscode-npmx PR 43: Touches provider registration and diagnostics wiring in src/index.ts, related to how the new code action provider is registered.
  • npmx-dev/vscode-npmx PR 29: Modifies src/providers/diagnostics/rules/vulnerability.ts around version parsing/formatting, directly related to the fixed-in/version selection logic.
  • npmx-dev/vscode-npmx PR 30: Adds fixedIn handling and selection logic in src/providers/diagnostics/rules/vulnerability.ts, overlapping the changes that compute the best fixed-in version.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The PR description references issue #24 and includes visual examples of the implemented quick-fix and vulnerability upgrade hint features.
Linked Issues check ✅ Passed All coding requirements from issue #24 are met: the PR adds vulnerability quick-fix actions, displays the earliest safe version in diagnostic messages, provides upgrade hints, and implements server-side support integration.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing issue #24 requirements: code action provider, diagnostic enhancements, version parsing logic, mock updates, and comprehensive test coverage.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

Copy link

@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

🧹 Nitpick comments (1)
src/providers/code-actions/vulnerability.ts (1)

47-51: Minor: Redundant parseVersion call on fixedInVersion.

The fixedInVersion extracted from the diagnostic code is already a raw semver string (e.g., "16.1.5"), so parseVersion(fixedInVersion)?.semver will return the same value. While this works correctly, it's slightly redundant.

♻️ Optional simplification
       const currentVersion = document.getText(diagnostic.range)
       const currentSemver = parseVersion(currentVersion)?.semver
-      const fixedSemver = parseVersion(fixedInVersion)?.semver ?? fixedInVersion
-      if (currentSemver && currentSemver === fixedSemver)
+      if (currentSemver && currentSemver === fixedInVersion)
         return []

Copy link

@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: 2

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/vulnerability.ts (1)

75-103: ⚠️ Potential issue | 🟠 Major

Guard vulnerablePackages to avoid runtime crashes.
If the API omits vulnerablePackages, Line 94 will throw, breaking diagnostics for the file. Default to an empty list before filtering.

🔧 Suggested fix
-  const { totalCounts, vulnerablePackages } = result
+  const { totalCounts, vulnerablePackages = [] } = result
@@
-  const rootVulnerabilitiesFixedIn = vulnerablePackages
+  const rootVulnerabilitiesFixedIn = vulnerablePackages
     .filter((vulnerablePackage) => vulnerablePackage.depth === 'root')

Copy link

@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.

🧹 Nitpick comments (2)
src/providers/diagnostics/rules/vulnerability.ts (2)

15-20: Clarify function intent: this finds the maximum (not earliest) fixedIn version.

The reduce logic returns the maximum version from the list (since lt(best, current) keeps the larger value). This is correct when you need to fix all vulnerabilities simultaneously—the user must upgrade to at least the highest fixedIn version. However, the PR objective references "earliest safe version," which could cause confusion.

Consider renaming to getMinimumRequiredFixVersion or adding a brief comment explaining why the maximum is chosen.

📝 Suggested clarification
+/**
+ * Returns the highest fixedIn version, ensuring all vulnerabilities are addressed.
+ */
 function getBestFixedInVersion(fixedInVersions: string[]): string | undefined {
   if (!fixedInVersions.length)
     return

-  return fixedInVersions.reduce((best, current) => lt(best, current) ? current : best)
+  return fixedInVersions.reduce((best, current) => lt(best, current) ? current : best)
 }

62-64: Reconsider preserving the version prefix in the upgrade message.

Using parsed.prefix (e.g., ^ or ~) in the message produces output like "Upgrade to ^16.1.5 to fix." This could be confusing because:

  1. The prefix implies a range, not a specific version.
  2. If the user's current range (e.g., ^16.0.0) already technically includes 16.1.5, the message may seem contradictory.

Consider showing the exact version without a prefix in the message, or clarifying that the version number is the minimum required.

💡 Suggested fix
   const fixedInVersion = getBestFixedInVersion(rootVulnerabilitiesFixedIn)
   const messageSuffix = fixedInVersion
-    ? ` Upgrade to ${parsed.prefix}${fixedInVersion} to fix.`
+    ? ` Upgrade to ${fixedInVersion} to fix.`
     : ''

Copy link

@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

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@nitodeco nitodeco requested a review from 9romise February 16, 2026 14:16
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.

Display earliest safe version for vulnurability check

2 participants