Conversation
9romise
changed the title
# Conflicts: # src/providers/diagnostics/rules/upgrade.ts
# Conflicts: # src/commands/open-file-in-npmx.ts # src/constants.ts # src/providers/completion-item/index.ts # src/providers/completion-item/version.ts # src/providers/diagnostics/index.ts # src/providers/diagnostics/rules/engine-mismatch.ts # src/utils/document.ts # src/utils/file.ts # src/utils/resolve.ts # tests/diagnostics/context.ts
9romise
changed the title
📝 WalkthroughWalkthroughThis PR adds workspace-aware dependency resolution for package.json, pnpm-workspace.yaml and .yarnrc.yml by introducing SUPPORTED_DOCUMENT_PATTERN, new JsonExtractor and YamlExtractor, ResolvedDependencyInfo types, and workspace context utilities (getWorkspaceContext, getResolvedDependencies, getResolvedDependencyByOffset, useWorkspaceContext). It consolidates provider registrations to a single pattern, replaces extractor-driven parsing with resolved dependency flows, adds resolveDependencySpec and workspace utilities, removes legacy extractors/parsers, and updates diagnostics, hover, completion, document-links and tests to operate on resolved dependency information. 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/providers/diagnostics/rules/upgrade.ts (1)
34-46:⚠️ Potential issue | 🟡 MinorPick the highest matching pre-release before returning.
This loop returns the first matching dist-tag, but dist-tag iteration order is not a version sort. If two tags point at different
-beta/-rcbuilds, the diagnostic can suggest an older pre-release even when a newer one is available.Suggested fix
- for (const [tag, tagVersion] of Object.entries(distTags)) { + let bestTagVersion: string | undefined + let bestTargetVersion: string | undefined + for (const [tag, tagVersion] of Object.entries(distTags)) { if (tag === 'latest') continue if (prerelease(tagVersion)?.[0] !== currentPreId) continue if (lte(tagVersion, resolvedVersion)) continue const targetVersion = formatUpgradeVersion(dep, tagVersion) if (checkIgnored({ ignoreList, name: resolvedName, version: targetVersion })) continue - - return targetVersion + if (!bestTagVersion || gt(tagVersion, bestTagVersion)) { + bestTagVersion = tagVersion + bestTargetVersion = targetVersion + } } + + return bestTargetVersionsrc/providers/diagnostics/index.ts (1)
59-123:⚠️ Potential issue | 🟠 MajorMake each collection run cancellable per document.
isStale()only keys offdocument.version, but this function is also re-entered whenenabledRulesor watched files change without editing the document. Becausecollect()andrunRule()are launched fire-and-forget, an older run can still flush after a newer run has started and reintroduce stale diagnostics.Suggested fix
export function useDiagnostics() { const diagnosticCollection = useDisposable(languages.createDiagnosticCollection(displayName)) + const collectRunIds = new WeakMap<TextDocument, number>() const activeEditor = useActiveTextEditor() const activeDocumentText = useDocumentText(() => activeEditor.value?.document) @@ async function collectDiagnostics(document: TextDocument) { + const runId = (collectRunIds.get(document) ?? 0) + 1 + collectRunIds.set(document, runId) await nextTick() logger.info(`[diagnostics] collect: ${document.uri.path}`) diagnosticCollection.set(document.uri, []) @@ const targetVersion = document.version + const isSuperseded = () => collectRunIds.get(document) !== runId || isStale(document, targetVersion) + if (isSuperseded()) + return + const dependencies = await getResolvedDependencies(document.uri) - if (!dependencies) + if (!dependencies || isSuperseded()) return const diagnostics: Diagnostic[] = [] const flush = debounce(() => { - if (isStale(document, targetVersion)) + if (isSuperseded()) return diagnosticCollection.set(document.uri, [...diagnostics]) @@ const runRule = async (rule: DiagnosticRule, ctx: DiagnosticContext) => { try { const diagnostic = await rule(ctx) - if (isStale(document, targetVersion)) + if (isSuperseded()) return if (!diagnostic) return @@ const collect = async (dep: ResolvedDependencyInfo) => { try { const pkg = await dep.packageInfo() - if (!pkg || isStale(document, targetVersion)) + if (!pkg || isSuperseded()) return for (const rule of rules) { - runRule(rule, { uri: document.uri, dep, pkg }) + if (isSuperseded()) + return + void runRule(rule, { uri: document.uri, dep, pkg }) } } catch (err) { logger.warn(`[diagnostics] fail to check ${dep.rawName}: ${err}`) @@ // fire-and-forget to progressively display diagnostics as each dep resolves, rather than awaiting all for (const dep of dependencies) { - collect(dep) + if (isSuperseded()) + break + void collect(dep) } }Also applies to: 125-143
🧹 Nitpick comments (6)
tests/utils/memoize.test.ts (1)
110-115: Use the explicit no-TTL mode in this new test.Line 115 is exercising invalidation, not expiry. Switching this case to
ttl: falsemakes the test depend on the public contract instead of the current falsy-0implementation detail.Suggested tweak
- const memoized = memoize(fn, { ttl: 0 }) + const memoized = memoize(fn, { ttl: false })src/extractors/yaml.ts (1)
21-24: Consider defensive handling for missingrange.The
node.range!non-null assertion assumes the YAML node always has a range. Whilst this is true for nodes parsed from text, it could be undefined for programmatically created nodes. Since this extractor is specifically for parsing text (viaparseDocument), this is likely safe in practice, but a defensive check could prevent unexpected errors if the API is used differently in future.♻️ Optional defensive fix
`#getScalarRange`(node: YamlNode): OffsetRange { - const [start, end] = node.range! + const range = node.range ?? [0, 0] + const [start, end] = range return [start, end] }src/constants.ts (1)
5-5: Add a regression test for the shared document glob.This selector now gates hover, completion, and code actions for every supported manifest. A small matcher-level test covering
package.json,pnpm-workspace.yaml, and.yarnrc.ymlwould catch a silent break here before three features go dark.tests/utils/version.test.ts (1)
6-23: Cover the protocol-aware branches here.The PR’s main behaviour change is resolution across protocols, but this table only exercises plain specs and
npm:aliases. Please add cases forworkspace:,catalog:, andfile:/link:inputs so regressions in the new paths are caught.tests/utils/package-manager.test.ts (1)
34-45: Please add the default-to-npmcase as well.This matrix verifies explicit detection signals, but not the fallback branch when neither
packageManagernor workspace files are present. That default is part of the new detection contract and is easy to regress during future refactors.tests/utils/dependency.test.ts (1)
4-80: Addworkspace:coverage to this suite.The new resolution path is now workspace-aware, but these tests still skip
workspace:specifiers entirely. A regression inworkspace:*,workspace:^, or aliased workspace refs would slip through even though that is a core branch of this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 967a24a0-f338-4757-a00e-5ee8af0692bd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (62)
README.mdeslint.config.jspackage.jsonpnpm-workspace.yamlsrc/composables/workspace-context.tssrc/constants.tssrc/extractors/index.tssrc/extractors/json.tssrc/extractors/package-json.tssrc/extractors/pnpm-workspace-yaml.tssrc/extractors/yaml.tssrc/index.tssrc/providers/code-actions/index.tssrc/providers/completion-item/index.tssrc/providers/completion-item/version.tssrc/providers/diagnostics/index.tssrc/providers/diagnostics/rules/deprecation.tssrc/providers/diagnostics/rules/dist-tag.tssrc/providers/diagnostics/rules/engine-mismatch.tssrc/providers/diagnostics/rules/replacement.tssrc/providers/diagnostics/rules/upgrade.tssrc/providers/diagnostics/rules/vulnerability.tssrc/providers/document-link/index.tssrc/providers/document-link/npmx.tssrc/providers/hover/index.tssrc/providers/hover/npmx.tssrc/types/context.tssrc/types/extractor.tssrc/utils/ast.tssrc/utils/dependency.tssrc/utils/file.tssrc/utils/memoize.tssrc/utils/package-manager.tssrc/utils/package.tssrc/utils/shared.tssrc/utils/version.tssrc/utils/workspace.tstests/__setup__/index.tstests/diagnostics/context.tstests/diagnostics/engine-mismatch.test.tstests/diagnostics/upgrade.test.tstests/fixtures/workspace/dirty-doc/package.jsontests/fixtures/workspace/dirty-doc/packages/app/package.jsontests/fixtures/workspace/minimal/package.jsontests/fixtures/workspace/package-manager-npm/.yarnrc.ymltests/fixtures/workspace/package-manager-npm/package.jsontests/fixtures/workspace/package-manager-npm/pnpm-workspace.yamltests/fixtures/workspace/package-manager-pnpm/package.jsontests/fixtures/workspace/package-manager-pnpm/pnpm-workspace.yamltests/fixtures/workspace/package-manager-yarn/.yarnrc.ymltests/fixtures/workspace/package-manager-yarn/package.jsontests/fixtures/workspace/pnpm-workspace/package.jsontests/fixtures/workspace/pnpm-workspace/packages/app/package.jsontests/fixtures/workspace/pnpm-workspace/packages/core/package.jsontests/fixtures/workspace/pnpm-workspace/pnpm-workspace.yamltests/utils/dependency.test.tstests/utils/memoize.test.tstests/utils/package-manager.test.tstests/utils/version.test.tstsconfig.jsontsdown.config.tsvitest.config.ts
💤 Files with no reviewable changes (3)
- src/utils/package.ts
- src/extractors/package-json.ts
- src/extractors/pnpm-workspace-yaml.ts
| export const checkReplacement: DiagnosticRule = async ({ dep: { nameRange, resolvedName } }) => { | ||
| if (checkIgnored({ ignoreList: config.ignore.replacement, name: resolvedName })) | ||
| return | ||
|
|
||
| const replacement = await getReplacement(name) | ||
| const replacement = await getReplacement(resolvedName) | ||
| if (!replacement) | ||
| return | ||
|
|
||
| const { message, link } = getReplacementInfo(replacement) | ||
|
|
||
| return { | ||
| node: dep.nameNode, | ||
| message: `"${name}" ${message}`, | ||
| range: nameRange, | ||
| message: `"${resolvedName}" ${message}`, |
There was a problem hiding this comment.
Do not lose the raw manifest name for aliases.
Lines 44-56 now ignore and report against resolvedName only, while nameRange still points at the raw key. For an aliased dependency such as foo: npm:bar@..., an ignore entry for foo will stop working and the warning text will no longer match the underlined token.
Suggested fix
-export const checkReplacement: DiagnosticRule = async ({ dep: { nameRange, resolvedName } }) => {
- if (checkIgnored({ ignoreList: config.ignore.replacement, name: resolvedName }))
+export const checkReplacement: DiagnosticRule = async ({ dep: { nameRange, rawName, resolvedName } }) => {
+ if (
+ checkIgnored({ ignoreList: config.ignore.replacement, name: rawName })
+ || checkIgnored({ ignoreList: config.ignore.replacement, name: resolvedName })
+ )
return
@@
return {
range: nameRange,
- message: `"${resolvedName}" ${message}`,
+ message: rawName === resolvedName
+ ? `"${rawName}" ${message}`
+ : `"${rawName}" (alias of "${resolvedName}") ${message}`,
severity: DiagnosticSeverity.Warning,
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/composables/workspace-context.ts (2)
10-15:⚠️ Potential issue | 🟡 MinorDispose the workspace folder listener.
The
workspace.onDidChangeWorkspaceFolderssubscription is not wrapped inuseDisposable(), unlike the other subscriptions in this file. IfuseWorkspaceContext()is initialised multiple times, duplicate handlers will accumulate.Proposed fix
export function useWorkspaceContext() { - workspace.onDidChangeWorkspaceFolders(({ removed }) => { + useDisposable(workspace.onDidChangeWorkspaceFolders(({ removed }) => { removed.forEach((folder) => { getWorkspaceContext.delete(folder.uri) logger.info(`[workspace-context] delete workspace folder cache: ${folder.uri.path}`) }) - }) + })),
17-28:⚠️ Potential issue | 🟠 MajorWorkspace-level metadata changes require broader cache invalidation.
When workspace configuration files (
pnpm-workspace.yaml,.yarnrc.yml, rootpackage.json) change, only the individual URI cache entry is deleted. However, data from these files (package manager detection, catalog definitions) feeds the entireWorkspaceContextand affects resolution of all dependencies in the workspace.Deleting only
loadPackageManifestInfo.delete(uri)/loadWorkspaceCatalogInfo.delete(uri)leaves sibling manifests resolved against stale package-manager or catalog data until each file is individually touched.Consider invalidating the entire
WorkspaceContextwhen workspace-level configuration files change, or at minimum clearing all cached manifest info within that workspace.,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a5b07a6-0d00-40a0-844d-440fe6153eda
📒 Files selected for processing (1)
src/composables/workspace-context.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/providers/diagnostics/rules/deprecation.ts (1)
25-29:⚠️ Potential issue | 🟡 MinorLink the deprecated version you actually reported.
The message is based on
resolvedVersion, but the target URL is still built fromresolvedSpec. For tags and ranges that can open a different release from the one marked as deprecated.💡 Suggested fix
code: { value: 'deprecation', - target: Uri.parse(npmxPackageUrl(resolvedName, resolvedSpec)), + target: Uri.parse(npmxPackageUrl(resolvedName, resolvedVersion)), },src/providers/hover/npmx.ts (1)
39-42:⚠️ Potential issue | 🟡 MinorLink provenance to the version you verified.
The provenance check is performed against
resolvedVersion, but the hover link still points atresolvedSpec. For tags and ranges that can open a different release from the one just marked as verified.💡 Suggested fix
const resolvedVersion = await dep.resolvedVersion() if (resolvedVersion && pkg.versionsMeta[resolvedVersion]?.provenance) // npmx.dev can resolve ranges and tags version specifier - md.appendMarkdown(`[$(verified)${SPACER}Verified provenance](${npmxPackageUrl(resolvedName, resolvedSpec)}#provenance)\n\n`) + md.appendMarkdown(`[$(verified)${SPACER}Verified provenance](${npmxPackageUrl(resolvedName, resolvedVersion)}#provenance)\n\n`)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84201ddf-328e-411e-8c6d-479ab77f3841
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonpnpm-workspace.yamlsrc/composables/workspace-context.tssrc/extractors/json.tssrc/providers/diagnostics/rules/deprecation.tssrc/providers/hover/npmx.tssrc/utils/ast.tssrc/utils/memoize.tssrc/utils/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/composables/workspace-context.ts
- src/utils/ast.ts
- package.json
- pnpm-workspace.yaml
| const name = this.#getStringValue(root, 'name') | ||
| const version = this.#getStringValue(root, 'version') | ||
| if (!name || !version) | ||
| return | ||
|
|
||
| return { | ||
| name, | ||
| version, | ||
| packageManager: this.#getStringValue(root, 'packageManager'), | ||
| engines: this.#getEngines(root), | ||
| dependencies: this.getDependenciesInfo(root), | ||
| } |
There was a problem hiding this comment.
Do not short-circuit dependency extraction on missing manifest metadata.
Many private apps and workspace roots omit version, and some also omit name. Returning undefined here means WorkspaceContext.loadPackageManifestInfo() never exposes their dependency list, so hover/diagnostics/completions disappear for otherwise valid dependency blocks. If PackageManifestInfo needs required name/version, this likely wants a separate dependency-only result instead of an early return.
| static async create(folder: WorkspaceFolder): Promise<WorkspaceContext> { | ||
| const ctx = new WorkspaceContext(folder) | ||
| ctx.packageManager = await detectPackageManager(folder) | ||
|
|
||
| if (ctx.packageManager !== 'npm') { | ||
| const workspaceFilename = workspaceFileMapping[ctx.packageManager] | ||
| const workspaceFile = Uri.joinPath(folder.uri, workspaceFilename) | ||
| if (await accessOk(workspaceFile)) | ||
| ctx.catalogs = (await ctx.loadWorkspaceCatalogInfo(workspaceFile))?.catalogs | ||
| } |
There was a problem hiding this comment.
Invalidate the folder context when workspace metadata changes.
WorkspaceContext.create() snapshots packageManager and catalogs, and getWorkspaceContextByFolder() caches that instance indefinitely. With the current watcher wiring in src/composables/workspace-context.ts:17-41, file changes only clear the per-URI extractor caches, so edits to package.json, pnpm-workspace.yaml, or .yarnrc.yml keep resolving against stale workspace state until the window is reloaded.
Also applies to: 130-137
1 participant
WorkspaceContext, a per workspace-folder context that manages package manager detection, catalog loading, and dependency resolutionJsonExtractor/YamlExtractor, selected by file extension; extractors only parse raw dependency info.catalogssupport #56ResolvedDependencyInfofromWorkspaceContextnow.