Skip to content

Comments

feat: show warning on pkg fetch error#28

Merged
9romise merged 4 commits intonpmx-dev:mainfrom
nitodeco:feat/ui-feedback-on-lookup-failure
Feb 5, 2026
Merged

feat: show warning on pkg fetch error#28
9romise merged 4 commits intonpmx-dev:mainfrom
nitodeco:feat/ui-feedback-on-lookup-failure

Conversation

@nitodeco
Copy link
Collaborator

@nitodeco nitodeco commented Feb 5, 2026

Resolves #19.

Shows warning when package can't be found:
datprotalav

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

The pull request refactors the hover provider for npmx packages by fixing a typo in the package URL function name (npmPacakgeUrl to npmPackageUrl), adding support for the JSR (JavaScript Registry) protocol, and reorganising the protocol validation flow. New logic explicitly handles JSR packages with a warning that they are not available on npmx, and includes fallback messaging when package information retrieval fails. A new jsrPackageUrl utility function is introduced to generate JSR package links.

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The PR description directly references issue #19 and describes the implemented feature: showing a warning when package fetch fails, with supporting visual evidence.
Linked Issues check ✅ Passed The code changes successfully implement the primary requirement from #19: providing user-facing feedback when package lookup fails by returning a warning hover instead of undefined.
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving #19. The modifications to npmx.ts handle JSR protocol support and error feedback, whilst updates to links.ts provide required URL generation utilities.

✏️ 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

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

Copy link
Member

@9romise 9romise left a comment

Choose a reason for hiding this comment

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

Good job! I think we could also prompt users to visit https://jsr.io when a jsr:‑style specifier is detected.

@nitodeco
Copy link
Collaborator Author

nitodeco commented Feb 5, 2026

@9romise wdyt?

Pasted Graphic 1

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.

Actionable comments posted: 1

Copy link
Member

@9romise 9romise left a comment

Choose a reason for hiding this comment

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

Nice! Thank you! ❤️

@nitodeco nitodeco force-pushed the feat/ui-feedback-on-lookup-failure branch from a053267 to 9d877e0 Compare February 5, 2026 13:23
@nitodeco nitodeco force-pushed the feat/ui-feedback-on-lookup-failure branch from 40cf049 to 8f5fdd9 Compare February 5, 2026 13:24
@nitodeco nitodeco requested a review from 9romise February 5, 2026 13:33
# Conflicts:
#	src/providers/hover/npmx.ts
@9romise 9romise merged commit 7857416 into npmx-dev:main Feb 5, 2026
6 checks passed
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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/providers/hover/npmx.ts (1)

46-47: Consider showing feedback for other unsupported protocols.

The PR objectives mention aligning UX with tools like Version Lens that display "not supported" for unsupported registries. Currently, JSR gets explicit feedback, but other unsupported protocols result in no hover at all. For consistency, you might want to show a similar warning for other unsupported protocols.

💡 Optional: Add feedback for unsupported protocols
-    if (!isSupportedProtocol(protocol))
-      return
+    if (!isSupportedProtocol(protocol)) {
+      const unsupportedMd = new MarkdownString('', true)
+      unsupportedMd.isTrusted = true
+      unsupportedMd.appendMarkdown(`$(warning) Protocol "${protocol}" is not supported`)
+      return new Hover(unsupportedMd)
+    }

Comment on lines +49 to +57
const pkg = await getPackageInfo(name)
if (!pkg) {
const errorMd = new MarkdownString('', true)

errorMd.isTrusted = true
errorMd.appendMarkdown('$(warning) Unable to fetch package information')

return new Hover(errorMd)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Good error handling for fetch failures, but consider wrapping in try-catch.

This correctly addresses the PR objective of showing feedback when package lookup fails. However, looking at getPackageInfo in the relevant snippets, it returns null for 404 errors but throws for other error types. If a non-404 error occurs (e.g., network timeout, 500), this could result in an unhandled promise rejection.

🛡️ Proposed fix to handle all error cases
-    const pkg = await getPackageInfo(name)
-    if (!pkg) {
+    let pkg: Awaited<ReturnType<typeof getPackageInfo>> = null
+    try {
+      pkg = await getPackageInfo(name)
+    }
+    catch {
+      // Fall through to show error message
+    }
+
+    if (!pkg) {
       const errorMd = new MarkdownString('', true)
 
       errorMd.isTrusted = true
       errorMd.appendMarkdown('$(warning) Unable to fetch package information')
 
       return new Hover(errorMd)
     }
📝 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 pkg = await getPackageInfo(name)
if (!pkg) {
const errorMd = new MarkdownString('', true)
errorMd.isTrusted = true
errorMd.appendMarkdown('$(warning) Unable to fetch package information')
return new Hover(errorMd)
}
let pkg: Awaited<ReturnType<typeof getPackageInfo>> = null
try {
pkg = await getPackageInfo(name)
}
catch {
// Fall through to show error message
}
if (!pkg) {
const errorMd = new MarkdownString('', true)
errorMd.isTrusted = true
errorMd.appendMarkdown('$(warning) Unable to fetch package information')
return new Hover(errorMd)
}

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.

No hover feedback when package lookup fails (e.g. @deno/doc)

2 participants