feat: supports clicking on dependency names to jump#60
feat: supports clicking on dependency names to jump#60RYGRIT wants to merge 9 commits intonpmx-dev:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new configuration option Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
53-53: Consider documenting the allowed values inline.The table entry is correct, but adding
off | latest | versionin the description would reduce context switching for users.Suggested README tweak
-| `npmx.documentLinks` | Enable clickable links for package names | `string` | `"version"` | +| `npmx.documentLinks` | Enable clickable links for package names (`off`, `latest`, `version`) | `string` | `"version"` |
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
README.mdpackage.jsonpnpm-workspace.yamlsrc/index.tssrc/providers/document-link/npmx.ts
src/providers/document-link/npmx.ts
Outdated
| const nameRange = this.extractor.getNodeRange(document, nameNode) | ||
| links.push(new VscodeDocumentLink(nameRange, Uri.parse(url))) |
There was a problem hiding this comment.
Add a defensive guard before range/link creation.
For transient malformed edits, skipping entries with missing/invalid nameNode would make this provider more robust.
Defensive tweak
- const nameRange = this.extractor.getNodeRange(document, nameNode)
- links.push(new VscodeDocumentLink(nameRange, Uri.parse(url)))
+ if (!nameNode)
+ continue
+ const nameRange = this.extractor.getNodeRange(document, nameNode)
+ links.push(new VscodeDocumentLink(nameRange, Uri.parse(url)))📝 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.
| const nameRange = this.extractor.getNodeRange(document, nameNode) | |
| links.push(new VscodeDocumentLink(nameRange, Uri.parse(url))) | |
| if (!nameNode) | |
| continue | |
| const nameRange = this.extractor.getNodeRange(document, nameNode) | |
| links.push(new VscodeDocumentLink(nameRange, Uri.parse(url))) |
This reverts commit ce28803.
RYGRIT
changed the title
9romise
left a comment
9romise
left a comment
There was a problem hiding this comment.
I'd like to something like this:
{
"npmx.clickableLinks.enabled": true,
"npmx.clickableLinks.versionMode": "resolved", // "declared" | "resolved" | "latest"
"npmx.clickableLinks.target": "package" // "docs", "code".
}documentLinks is a bit technical and not very intuitive for users, maybe we could rename it to clickableLinks?
|
Thanks for the detailed suggestions! I've carefully considered the proposed configuration structure, and here's my thinking: Regarding versionMode (declared/resolved/latest): I agree that having a resolved option would be valuable. My initial concern was the complexity of parsing lock files (package-lock.json, pnpm-lock.yaml, yarn.lock), which have different formats and can be quite large. However, I've been thinking about an alternative approach: reading the installed version from node_modules//package.json. This would be much simpler to implement:
Pros:
Cons:
What do you think of this method? Does it meet your needs for a parsed versioned pattern? If you have any ideas or better solutions, please feel free to share them! Regarding target (package/docs/code): I don't think we need separate docs and code targets because:
I'm open to adding this if you think there's a strong use case I'm missing. Looking forward to your feedback! |
|
Thank you for the detailed explanation. |
|
Got it. So declared refers to the raw string defined in package.json (preserving ranges like ^1.0.0 or dist-tags like latest), while resolved call getPackageInfo() + resolveExactVersion() to get the actual latest matching version (e.g., ^18.0.0 -> 18.2.0). I fully agree with this definition and the plan to drop target. Let's proceed with this! |
|
@9romise What do you think of calling it |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
53-53: Document the acceptednpmx.packageLinksvalues in the README.Line 53 is accurate but too generic. Please include the enum values (
off,latest,declared,resolved) in this table row so users do not need to cross-checkpackage.json.src/providers/document-link/npmx.ts (1)
11-14:extractorcan be immutable.
extractoris only set in the constructor, so marking itreadonlywould tighten the class contract and reduce accidental mutation risk.Small tidy-up
-export class NpmxDocumentLinkProvider<T extends Extractor> implements DocumentLinkProvider { - extractor: T +export class NpmxDocumentLinkProvider<T extends Extractor> implements DocumentLinkProvider { + readonly extractor: T
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 037a5e47-e036-4a4c-8659-3557e254a8c5
📒 Files selected for processing (4)
README.mdpackage.jsonsrc/index.tssrc/providers/document-link/npmx.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a62f7eb1-f0d3-44b1-963f-7202a15ab7e2
📒 Files selected for processing (1)
src/providers/document-link/npmx.ts
| const parsed = parseVersion(dep.version) | ||
| if (!parsed) | ||
| continue | ||
|
|
||
| // Skip unsupported protocols (workspace:, file:, git:, link:, jsr:, etc.) | ||
| if (!isSupportedProtocol(parsed.protocol)) | ||
| continue | ||
|
|
||
| parsedDeps.push({ dep, parsed }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether npm alias syntax appears in repository package manifests
rg -nP '"[^"]+"\s*:\s*"npm:[^"]+"' -g '**/package.json' -g '!**/node_modules/**'Repository: npmx-dev/vscode-npmx
Length of output: 113
🏁 Script executed:
# Get context around the relevant lines in npmx.ts
head -100 src/providers/document-link/npmx.ts | tail -50Repository: npmx-dev/vscode-npmx
Length of output: 1864
🏁 Script executed:
# Look for the parseVersion function to understand protocol extraction
rg -A 10 'function parseVersion|const parseVersion' src/providers/document-link/npmx.tsRepository: npmx-dev/vscode-npmx
Length of output: 46
🏁 Script executed:
# Check how the URL is actually built - look for the complete logic after parsedDeps.push
head -150 src/providers/document-link/npmx.ts | tail -100Repository: npmx-dev/vscode-npmx
Length of output: 3639
🏁 Script executed:
# Find and read the version utilities
fd 'version' src/utils --type fRepository: npmx-dev/vscode-npmx
Length of output: 84
🏁 Script executed:
# Search for parseVersion implementation
rg -n 'parseVersion|isSupportedProtocol' src/utils/version.ts -A 15Repository: npmx-dev/vscode-npmx
Length of output: 842
🏁 Script executed:
# Get complete context of the version utility file
wc -l src/utils/version.ts && cat -n src/utils/version.tsRepository: npmx-dev/vscode-npmx
Length of output: 2685
npm alias dependencies generate incorrect link targets.
When a dependency uses npm alias syntax (e.g., "nuxt": "npm:4.3.0"), the current code generates links using the alias key as the package name. The parseVersion function extracts only the protocol and version, not the target package name. This causes links to point to the wrong package.
A safe mitigation is to skip npm protocol entries until full alias parsing is implemented.
Suggested mitigation
- if (!isSupportedProtocol(parsed.protocol))
+ if (!isSupportedProtocol(parsed.protocol) || parsed.protocol === 'npm')
continueAlso applies to: 75–86
| const url = targetVersion | ||
| ? npmxPackageUrl(name, targetVersion) | ||
| : npmxPackageUrl(name) | ||
| // Create link for package name | ||
| const nameRange = this.extractor.getNodeRange(document, nameNode) | ||
| const link = new VscodeDocumentLink(nameRange, Uri.parse(url)) | ||
| link.tooltip = `Open ${name}@${targetVersion ?? parsed.version} on npmx` |
There was a problem hiding this comment.
Encode package/version path segments before URL generation.
Raw values in path segments can break links for scoped packages (e.g. @scope/pkg) and special version strings.
Suggested fix
- const url = targetVersion
- ? npmxPackageUrl(name, targetVersion)
- : npmxPackageUrl(name)
+ const encodedName = encodeURIComponent(name)
+ const encodedVersion = targetVersion ? encodeURIComponent(targetVersion) : undefined
+ const url = encodedVersion
+ ? npmxPackageUrl(encodedName, encodedVersion)
+ : npmxPackageUrl(encodedName)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/providers/document-link/npmx.ts (3)
58-67:⚠️ Potential issue | 🟠 MajorHandle
npm:alias specs before link generation.Line 59 parses protocol, but link construction later still uses the alias key (
dep.name) as the package path. For alias specs, this points to the wrong package page.Mitigation (safe until full alias parsing is added)
- if (!isSupportedProtocol(parsed.protocol)) + if (!isSupportedProtocol(parsed.protocol) || parsed.protocol === 'npm') continue
90-92:⚠️ Potential issue | 🟠 MajorEncode URL path segments before calling
npmxPackageUrl.Lines 90-92 pass raw
name/targetVersioninto path segments. Scoped package names and special version strings can produce invalid routes.Suggested fix
- const url = targetVersion - ? npmxPackageUrl(name, targetVersion) - : npmxPackageUrl(name) + const encodedName = encodeURIComponent(name) + const encodedVersion = targetVersion ? encodeURIComponent(targetVersion) : undefined + const url = encodedVersion + ? npmxPackageUrl(encodedName, encodedVersion) + : npmxPackageUrl(encodedName)
94-95:⚠️ Potential issue | 🟡 MinorAdd a defensive
nameNodeguard beforegetNodeRange.Line 94 assumes
nameNodeis always present. During transient malformed edits, this can throw and abort link generation for the whole document.Suggested fix
- const nameRange = this.extractor.getNodeRange(document, nameNode) + if (!nameNode) + continue + const nameRange = this.extractor.getNodeRange(document, nameNode)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8192cf9-86db-480a-900f-8dd5265b93c6
📒 Files selected for processing (1)
src/providers/document-link/npmx.ts
| // Create link for package name | ||
| const nameRange = this.extractor.getNodeRange(document, nameNode) | ||
| const link = new VscodeDocumentLink(nameRange, Uri.parse(url)) | ||
| link.tooltip = `Open ${name}@${targetVersion ?? parsed.version} on npmx` |
There was a problem hiding this comment.
Tooltip text is incorrect in latest mode.
At Line 96, targetVersion ?? parsed.version shows the declared spec even when linkMode === 'latest', so the tooltip can claim a version that is not the link target.
Suggested fix
- link.tooltip = `Open ${name}@${targetVersion ?? parsed.version} on npmx`
+ const tooltipVersion = linkMode === 'latest'
+ ? 'latest'
+ : (targetVersion ?? parsed.version ?? 'latest')
+ link.tooltip = `Open ${name}@${tooltipVersion} on npmx`2 participants
npmx.documentLinksconfig, defaultversion