Conversation
📝 WalkthroughWalkthroughThis pull request implements support for the "catalog" protocol in package version management. The changes refactor parsing utilities across extractors (package-json and pnpm-workspace-yaml) to use new document-caching parsers, introduce a new catalog resolution system that reads workspace YAML catalogs, and update version providers and diagnostics to handle catalog-based version references. The version utility is updated to treat 'catalog' as a supported protocol. New utilities in 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/resolve.ts (1)
54-55:⚠️ Potential issue | 🟡 MinorHarden manifest field validation to match the declared return type.
Line 54 currently checks only truthiness, so non-string
name/versionvalues can slip through as valid manifests.💡 Suggested fix
- if (!manifest || !manifest.name || !manifest.version) + if ( + !manifest + || typeof manifest.name !== 'string' + || manifest.name.length === 0 + || typeof manifest.version !== 'string' + || manifest.version.length === 0 + ) return
🧹 Nitpick comments (1)
src/utils/catalog/yaml.ts (1)
47-48: Avoid filtering catalog values by AST range metadata.For extraction, only key/value content is needed. The guard at Line 47 can discard otherwise valid catalog entries.
♻️ Proposed simplification
- if (!entry.value?.range) - return - const name = String(entry.key.value) const version = String(entry.value.value)
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/extractors/package-json.tssrc/extractors/pnpm-workspace-yaml.tssrc/providers/completion-item/version.tssrc/providers/diagnostics/index.tssrc/providers/hover/npmx.tssrc/utils/catalog.tssrc/utils/catalog/yaml.tssrc/utils/memoize.tssrc/utils/parse.tssrc/utils/resolve.tssrc/utils/version.ts
| let parsed = parseVersion(dep.version) | ||
|
|
||
| if (parsed?.protocol === 'catalog') { | ||
| const resolved = await resolveCatalogVersion(document.uri, dep.name, parsed.version) | ||
| if (isDocumentChanged(document, targetUri, targetVersion)) | ||
| return | ||
| parsed = resolved ? { protocol: 'catalog', version: resolved } : null | ||
| } | ||
|
|
||
| const exactVersion = parsed && isSupportedProtocol(parsed.protocol) | ||
| ? resolveExactVersion(pkg, parsed.version) | ||
| : null |
There was a problem hiding this comment.
Do not overwrite parsed.version with resolved catalog value.
Line 112 changes parsed.version from the raw catalog selector to a resolved semver string while keeping protocol: 'catalog'. That mixes two different meanings and can lead to invalid downstream edit formatting (for example, catalog:^x.y.z).
💡 Suggested fix (preserve raw parsed data, resolve using a separate variable)
- let parsed = parseVersion(dep.version)
+ let parsed = parseVersion(dep.version)
+ let versionForResolution = parsed?.version
if (parsed?.protocol === 'catalog') {
const resolved = await resolveCatalogVersion(document.uri, dep.name, parsed.version)
if (isDocumentChanged(document, targetUri, targetVersion))
return
- parsed = resolved ? { protocol: 'catalog', version: resolved } : null
+ if (!resolved)
+ parsed = null
+ else
+ versionForResolution = resolved
}
const exactVersion = parsed && isSupportedProtocol(parsed.protocol)
- ? resolveExactVersion(pkg, parsed.version)
+ ? resolveExactVersion(pkg, versionForResolution!)
: null📝 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.
| let parsed = parseVersion(dep.version) | |
| if (parsed?.protocol === 'catalog') { | |
| const resolved = await resolveCatalogVersion(document.uri, dep.name, parsed.version) | |
| if (isDocumentChanged(document, targetUri, targetVersion)) | |
| return | |
| parsed = resolved ? { protocol: 'catalog', version: resolved } : null | |
| } | |
| const exactVersion = parsed && isSupportedProtocol(parsed.protocol) | |
| ? resolveExactVersion(pkg, parsed.version) | |
| : null | |
| let parsed = parseVersion(dep.version) | |
| let versionForResolution = parsed?.version | |
| if (parsed?.protocol === 'catalog') { | |
| const resolved = await resolveCatalogVersion(document.uri, dep.name, parsed.version) | |
| if (isDocumentChanged(document, targetUri, targetVersion)) | |
| return | |
| if (!resolved) | |
| parsed = null | |
| else | |
| versionForResolution = resolved | |
| } | |
| const exactVersion = parsed && isSupportedProtocol(parsed.protocol) | |
| ? resolveExactVersion(pkg, versionForResolution!) | |
| : null |
| const cache = new Map<string, T | null>() | ||
|
|
||
| return (doc: TextDocument): T | null => { | ||
| const key = doc.uri.toString() | ||
| if (cache.has(key)) | ||
| return cache.get(key)! | ||
| const result = parse(doc.getText()) | ||
| cache.set(key, result) | ||
| const disposable = workspace.onDidChangeTextDocument((e) => { | ||
| if (e.document.uri.toString() === key) { | ||
| cache.delete(key) | ||
| disposable.dispose() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/utils/parse.ts | head -30Repository: npmx-dev/vscode-npmx
Length of output: 1303
🏁 Script executed:
# Check if the file exists and get its size
wc -l src/utils/parse.tsRepository: npmx-dev/vscode-npmx
Length of output: 85
🏁 Script executed:
rg -n 'parseJson|parseYaml' src --type ts --type tsx -B2 -A2Repository: npmx-dev/vscode-npmx
Length of output: 91
🏁 Script executed:
# Check if there are other listener management patterns in the codebase
rg -n 'workspace\.on|onDid' src --type ts --type tsx -B1 -A1 | head -50Repository: npmx-dev/vscode-npmx
Length of output: 91
🏁 Script executed:
rg -n 'parseJson|parseYaml' src --type ts -B2 -A2Repository: npmx-dev/vscode-npmx
Length of output: 2409
🏁 Script executed:
# Check other listener patterns
rg -n 'workspace\.on|onDid' src --type ts -B1 -A1Repository: npmx-dev/vscode-npmx
Length of output: 269
🏁 Script executed:
rg -n 'PackageJsonExtractor|PnpmWorkspaceYamlExtractor' src --type tsRepository: npmx-dev/vscode-npmx
Length of output: 792
🏁 Script executed:
# Check the extractor interface and how parse is called
rg -n 'interface Extractor|\.parse\(' src --type ts -B2 -A2 | head -60Repository: npmx-dev/vscode-npmx
Length of output: 3487
🏁 Script executed:
# Check if TextDocument.version is used elsewhere in codebase
rg -n 'doc\.version|document\.version' src --type tsRepository: npmx-dev/vscode-npmx
Length of output: 269
🏁 Script executed:
# Check TypeScript definitions to confirm TextDocument structure
rg -n 'TextDocument' src --type ts | head -20Repository: npmx-dev/vscode-npmx
Length of output: 2051
Refactor parser cache to use document version instead of event subscriptions.
Lines 18–23 register a new workspace.onDidChangeTextDocument listener per parsed document. These listeners persist until that document changes, creating accumulating memory overhead in sessions that parse many files. The codebase already uses document.version for cache invalidation elsewhere (diagnostics provider); apply the same pattern here.
Suggested fix (cache by URI + document version, no per-entry listener)
import type { ValidNode } from '#types/extractor'
import type { Node as JsonNode } from 'jsonc-parser'
import type { TextDocument } from 'vscode'
import type { Node as YamlNode } from 'yaml'
import { parseTree } from 'jsonc-parser'
-import { workspace } from 'vscode'
import { parseDocument } from 'yaml'
function createDocumentParse<T extends ValidNode>(parse: (text: string) => T | null) {
- const cache = new Map<string, T | null>()
+ const cache = new Map<string, { version: number, value: T | null }>()
return (doc: TextDocument): T | null => {
const key = doc.uri.toString()
- if (cache.has(key))
- return cache.get(key)!
+ const hit = cache.get(key)
+ if (hit && hit.version === doc.version)
+ return hit.value
+
const result = parse(doc.getText())
- cache.set(key, result)
- const disposable = workspace.onDidChangeTextDocument((e) => {
- if (e.document.uri.toString() === key) {
- cache.delete(key)
- disposable.dispose()
- }
- })
+ cache.set(key, { version: doc.version, value: result })
return result
}
}📝 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 cache = new Map<string, T | null>() | |
| return (doc: TextDocument): T | null => { | |
| const key = doc.uri.toString() | |
| if (cache.has(key)) | |
| return cache.get(key)! | |
| const result = parse(doc.getText()) | |
| cache.set(key, result) | |
| const disposable = workspace.onDidChangeTextDocument((e) => { | |
| if (e.document.uri.toString() === key) { | |
| cache.delete(key) | |
| disposable.dispose() | |
| } | |
| }) | |
| const cache = new Map<string, { version: number, value: T | null }>() | |
| return (doc: TextDocument): T | null => { | |
| const key = doc.uri.toString() | |
| const hit = cache.get(key) | |
| if (hit && hit.version === doc.version) | |
| return hit.value | |
| const result = parse(doc.getText()) | |
| cache.set(key, { version: doc.version, value: result }) | |
| return result | |
| } |
1 participant
Resolves #12