Skip to content

feat: build workspace context then resolve dependencies#68

Merged
9romise merged 58 commits intomainfrom
resolve
Mar 10, 2026
Merged

feat: build workspace context then resolve dependencies#68
9romise merged 58 commits intomainfrom
resolve

Conversation

@9romise
Copy link
Copy Markdown
Member

@9romise 9romise commented Mar 8, 2026

  • Introduces WorkspaceContext, a per workspace-folder context that manages package manager detection, catalog loading, and dependency resolution
  • Rename extractors to JsonExtractor / YamlExtractor, selected by file extension; extractors only parse raw dependency info.
  • Unified resolution for all dependency protocols, including catalog resolution, close Support catalog #12, close feat: add catalogs support #56
  • All providers refactored to consume ResolvedDependencyInfo from WorkspaceContext now.

@9romise 9romise changed the title Resolve refactor: build workspace context then resolve dependencies Mar 8, 2026
Copy link
Copy Markdown
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: 2

♻️ Duplicate comments (2)
src/providers/diagnostics/rules/deprecation.ts (1)

25-29: ⚠️ Potential issue | 🟡 Minor

Link the deprecated version you actually reported.

The message is based on resolvedVersion, but the target URL is still built from resolvedSpec. 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 | 🟡 Minor

Link provenance to the version you verified.

The provenance check is performed against resolvedVersion, but the hover link still points at resolvedSpec. 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

📥 Commits

Reviewing files that changed from the base of the PR and between a8fe3e5 and b886fc3.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • package.json
  • pnpm-workspace.yaml
  • src/composables/workspace-context.ts
  • src/extractors/json.ts
  • src/providers/diagnostics/rules/deprecation.ts
  • src/providers/hover/npmx.ts
  • src/utils/ast.ts
  • src/utils/memoize.ts
  • src/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

Copy link
Copy Markdown
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: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/utils/file.ts (1)

51-59: ⚠️ Potential issue | 🟠 Major

readPackageManifest() no longer matches its declared contract.

Line 54 casts raw JSON.parse() output to PackageManifestInfo, even though that type now expects parsed DependencyInfo[] entries rather than the raw dependency object map from package.json. Lines 56-57 also still reject manifests without name/version, so this helper can now return either a structurally invalid PackageManifestInfo or undefined for manifests the new flow is meant to support; please either normalise through the JSON extractor or keep a separate raw-manifest type here.

src/providers/diagnostics/index.ts (1)

55-57: ⚠️ Potential issue | 🟠 Major

document.version is not a sufficient collection token.

The old collect()/runRule() promises keep running in the background. When recollection is triggered by rule toggles or file-system events, the version stays unchanged, so an older pass can still flush diagnostics after a newer pass has cleared them. Please use a per-run generation id or AbortController instead of relying on document.version alone.

Also applies to: 75-99, 105-123, 125-149

♻️ Duplicate comments (1)
src/providers/hover/npmx.ts (1)

39-42: ⚠️ Potential issue | 🟡 Minor

Link the exact version whose provenance was checked.

Line 40 verifies provenance on resolvedVersion, but Line 42 still links to resolvedSpec. For ranges and tags, the hover can report one release as verified and open another.

🧹 Nitpick comments (1)
src/commands/open-file-in-npmx.ts (1)

44-45: Consider narrowing the return type of readPackageManifest() to eliminate the need for non-null assertions.

Lines 44–45 are safe today because readPackageManifest() guards against manifests without name/version, but the non-null assertions hide that contract from the type system. Returning a narrowed manifest type from readPackageManifest() (or adding a local type guard) would make this call site self-documenting and prevent future regressions if the helper changes.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 663aa823-135a-46fa-b882-5377e344d1a1

📥 Commits

Reviewing files that changed from the base of the PR and between b886fc3 and 551d08e.

📒 Files selected for processing (14)
  • src/commands/open-file-in-npmx.ts
  • src/composables/workspace-context.ts
  • src/data/extractors/index.ts
  • src/data/extractors/json.ts
  • src/data/extractors/yaml.ts
  • src/data/workspace.ts
  • src/providers/completion-item/version.ts
  • src/providers/diagnostics/index.ts
  • src/providers/diagnostics/rules/engine-mismatch.ts
  • src/providers/document-link/npmx.ts
  • src/providers/hover/npmx.ts
  • src/types/extractor.ts
  • src/utils/file.ts
  • tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
  • tsconfig.json

Comment on lines +18 to 19
if (info.resolvedProtocol !== 'npm')
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep catalog: and protocol aliases intact when inserting a version.

Line 18 now admits any dependency whose resolved protocol is npm, but the formatter in src/utils/version.ts:32-47 only re-attaches the declared protocol when resolvedName !== rawName. On catalog: or same-name npm: specs, accepting a completion here can overwrite the whole value with a bare range and silently break the manifest.

Also applies to: 39-42

Copy link
Copy Markdown
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: 3

🧹 Nitpick comments (1)
src/data/extractors/yaml.ts (1)

21-24: Consider defensive handling for missing range property.

The non-null assertion node.range! assumes the range is always present. While parseDocument typically includes range information, this could fail if the YAML library's behaviour changes or if the node is synthetically created.

🛡️ Optional defensive check
 `#getScalarRange`(node: YamlNode): OffsetRange {
-  const [start, end] = node.range!
-  return [start, end]
+  const range = node.range
+  if (!range)
+    return [0, 0]
+  return [range[0], range[1]]
 }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 81324241-571f-4f36-9ba4-200694a6a3ee

📥 Commits

Reviewing files that changed from the base of the PR and between 551d08e and 8c31528.

📒 Files selected for processing (3)
  • src/composables/workspace-context.ts
  • src/data/extractors/yaml.ts
  • src/data/workspace.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/composables/workspace-context.ts

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.

Support catalog

1 participant