Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a "version lens" CodeLens feature and supporting plumbing. Two new VS Code configuration keys ( Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (3)
packages/language-service/src/utils/version.ts (1)
59-71: Consider handling invalid semver input gracefully.
new SemVer(resolvedVersion)on line 60 will throw aTypeErrorifresolvedVersionis not a valid semver string. Whilst the caller should ideally provide valid input, defensive handling would prevent runtime crashes when encountering malformed version specs.♻️ Suggested defensive handling
export function resolveUpgradeTiers(pkg: PackageInfo, resolvedVersion: string): UpgradeTier[] { + let current: SemVer + try { + current = new SemVer(resolvedVersion) + } + catch { + return [] + } - const current = new SemVer(resolvedVersion) const currentMajor = current.major const currentMinor = current.minorpackages/language-service/src/utils/version.test.ts (1)
28-33: Test helper usesascast; consider a type-safe alternative.The
as PackageInfocast on line 32 bypasses type checking. Whilst acceptable in test helpers, a more type-safe approach would usesatisfiesor provide complete mock data.♻️ Type-safe alternative using Partial
-function createPkg(versions: string[]): PackageInfo { +function createPkg(versions: string[]): Partial<PackageInfo> & Pick<PackageInfo, 'versionsMeta' | 'distTags'> { const versionsMeta: Record<string, object> = {} for (const v of versions) versionsMeta[v] = {} - return { versionsMeta, distTags: { latest: versions.at(-1)! } } as PackageInfo + return { versionsMeta, distTags: { latest: versions.at(-1)! } } }As per coding guidelines: "Avoid
astype casts—validate instead in TypeScript".extensions/vscode/src/commands/replace-text.ts (1)
4-15: Consider handlingapplyEditfailure.
workspace.applyEdit()returnsPromise<boolean>indicating whether the edit was applied successfully. Silently ignoring failures could lead to confusing user experiences when edits don't apply (e.g., file changed externally, read-only file).♻️ Suggested error handling
export async function replaceText(uri: string, range: LspRange, newText: string) { const edit = new WorkspaceEdit() edit.replace( Uri.parse(uri), new Range( new Position(range.start.line, range.start.character), new Position(range.end.line, range.end.character), ), newText, ) - await workspace.applyEdit(edit) + const success = await workspace.applyEdit(edit) + if (!success) { + throw new Error(`Failed to apply edit to ${uri}`) + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cf001e9-a8a6-4365-a514-09e85dd1d164
📒 Files selected for processing (9)
extensions/vscode/README.mdextensions/vscode/package.jsonextensions/vscode/src/commands/replace-text.tsextensions/vscode/src/index.tspackages/language-service/src/index.tspackages/language-service/src/plugins/version-lens.tspackages/language-service/src/utils/version.test.tspackages/language-service/src/utils/version.tspackages/shared/src/commands.ts
| }, | ||
| }, | ||
| create(context): LanguageServicePluginInstance { | ||
| async function resolveVersionLensCommand({ uri, specRange, tier }: LenData, range: CodeLens['range']): Promise<CodeLens['command']> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/language-service/src/plugins/version-lens.ts | head -150Repository: npmx-dev/vscode-npmx
Length of output: 5201
Remove the as LenData cast and validate lens.data before destructuring.
Line 118 uses an unchecked type cast that bypasses validation. If lens.data is missing or malformed, the destructuring at line 29 will throw instead of degrading gracefully. Implement a validation function and guard the call:
Suggested implementation
async resolveCodeLens(lens): Promise<CodeLens> {
- const command = await resolveVersionLensCommand(lens.data as LenData, lens.range)
+ if (!isLenData(lens.data))
+ return { ...lens, command: UNKNOWN_COMMAND }
+ const command = await resolveVersionLensCommand(lens.data, lens.range)
return { ...lens, command }
},function isLenData(value: unknown): value is LenData {
if (typeof value !== 'object' || value === null)
return false
if (!('uri' in value) || typeof value.uri !== 'string')
return false
if (!('specRange' in value) || !Array.isArray(value.specRange) || value.specRange.length !== 2)
return false
if (typeof value.specRange[0] !== 'number' || typeof value.specRange[1] !== 'number')
return false
if (!('tier' in value) || value.tier === undefined)
return true
return typeof value.tier === 'object'
&& value.tier !== null
&& 'type' in value.tier
&& typeof value.tier.type === 'string'
&& 'version' in value.tier
&& typeof value.tier.version === 'string'
}This aligns with the coding guideline: "Avoid as type casts—validate instead in TypeScript".
| const ignoreList = await getConfig(context, 'npmx.ignore.upgrade') | ||
| const targetVersion = resolveUpgrade(dep, pkg, resolvedVersion, ignoreList) | ||
| if (!targetVersion) | ||
| return { title: '$(check) latest', command: '' } | ||
|
|
||
| return { | ||
| title: `$(arrow-up) ${targetVersion}`, | ||
| command: REPLACE_TEXT_COMMAND, | ||
| arguments: [uri, range, targetVersion], | ||
| } |
There was a problem hiding this comment.
Don't short-circuit the fallback path with hideWhenLatest.
Line 107 hides every dependency that reaches the non-tiered branch, so the fallback logic on Lines 54-63 never runs for those cases. That suppresses the fallback lens entirely and also hides the unknown state when metadata is still missing; hideWhenLatest should only hide confirmed latest cases.
Suggested fix
const dependencies = await workspaceState.getResolvedDependencies(document.uri)
if (!dependencies)
return []
const lenses: CodeLens[] = []
+ const hideWhenLatest = await getConfig(context, 'npmx.versionLens.hideWhenLatest')
+ const ignoreList = await getConfig(context, 'npmx.ignore.upgrade')
for (const dep of dependencies) {
if (dep.resolvedProtocol !== 'npm' || dep.category === 'peerDependencies')
continue
@@
- const hideWhenLatest = await getConfig(context, 'npmx.versionLens.hideWhenLatest')
- if (hideWhenLatest)
+ if (pkg && resolvedVersion && hideWhenLatest && !resolveUpgrade(dep, pkg, resolvedVersion, ignoreList))
continue
lenses.push({ range, data: baseData })
}Also applies to: 79-109
1 participant
close #2, close #32