Skip to content

feat(language-service): add installed package definitions#138

Open
hyoban wants to merge 1 commit into
npmx-dev:mainfrom
hyoban:codex/2026-07-03-installed-package-definitions
Open

feat(language-service): add installed package definitions#138
hyoban wants to merge 1 commit into
npmx-dev:mainfrom
hyoban:codex/2026-07-03-installed-package-definitions

Conversation

@hyoban

@hyoban hyoban commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

CleanShot.2026-07-03.at.22.45.04.mp4
  • add a package-name definition provider that jumps from dependency names in package.json to installed node_modules package manifests
  • keep version/spec interactions separate: catalog specs resolve to workspace catalog definitions and npm specs remain npmx document links
  • support package.json files opened from node_modules by resolving pnpm symlink realpaths inside the workspace
  • add focused tests for installed package lookups, range-specific dependency helpers, catalog definitions, and document links

Validation

  • pnpm vitest run packages/language-core/src/workspace.test.ts packages/language-service/src/plugins/document-link.test.ts packages/language-service/src/plugins/catalog.test.ts packages/language-service/src/plugins/installed-package-definition.test.ts packages/language-service/src/utils/document.test.ts
  • pnpm typecheck
  • pnpm lint
  • pnpm build
  • pnpm test
  • git diff --check

@hyoban hyoban marked this pull request as ready for review July 3, 2026 14:56
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

This change adds package-name path-safety helpers and a new findInstalledPackageManifestPath method to workspace context in language-core, along with an optional realpath adapter capability implemented in language-server. A new language-service plugin, installed-package-definition, resolves "go to definition" for installed dependencies. Offset-resolution helpers in document.ts are split into name/spec variants, catalog.ts and document-link.ts are refactored to use these helpers with catalog-protocol filtering, and a shared test fixture utility is introduced. Configuration and documentation text for npmx.packageLinks is updated to reference package version spec links. Extensive new tests accompany all changes.

Related PRs: None indicated.

Suggested labels: enhancement, language-service, language-core

Suggested reviewers: None indicated.

🐰 A rabbit hops through node_modules deep,
Chasing realpaths where symlinks sleep,
Definitions found for packages installed,
Catalog specs politely halted,
Hop, hop, hooray — new links appear! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description matches the implemented changes: installed package definitions, separated catalog/npm handling, pnpm realpath support, and added tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

🧹 Nitpick comments (4)
packages/language-service/src/utils/document.ts (1)

23-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication between the two offset helpers.

getResolvedDependencyNameAtOffset and getResolvedDependencySpecAtOffset are identical apart from the range field used. Could be reduced to a single parametrized helper, but the duplication is small and keeping them separate/explicit aids readability given each is a public API. Optional.

packages/language-service/src/plugins/catalog.ts (1)

8-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent catalog-detection signal vs. document-link.ts.

This helper detects catalog dependencies via rawSpec.startsWith('catalog:'), whereas providePackageDocumentLinks in document-link.ts uses dep.protocol === 'catalog' for the same purpose. Both work today, but relying on two different signals for the same classification is fragile if they ever diverge. Consider standardising on dep.protocol === 'catalog' here as well.

packages/language-service/src/plugins/document-link.ts (1)

27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Implicit handling of the 'latest' link mode.

targetVersion is only set for 'declared'/'resolved'; the 'latest' case is handled implicitly by falling through both branches. This currently produces the intended result, but it's not exhaustively checked — a future addition to EnabledPackageLinkMode without an explicit branch here would silently fall through the same way. Consider making the 'latest' case explicit for clarity and safety.

♻️ Proposed explicit handling
     let targetVersion: string | undefined

     if (linkMode === 'declared') {
       targetVersion = resolvedSpec
     } else if (linkMode === 'resolved') {
       targetVersion = await dep.resolvedVersion() ?? resolvedSpec
+    } else if (linkMode === 'latest') {
+      targetVersion = undefined
     }
packages/language-core/src/workspace.ts (1)

200-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated directory-walk logic between the two manifest lookups.

findNearestPackageManifestPath (Lines 200-216) and findInstalledPackageManifestPath (Lines 218-250) share the same upward directory-walk skeleton (loop condition, root-break, parent-advance-break), differing only in what candidate path is tested per directory. Extracting the shared walk into a helper that accepts a per-directory candidate-path callback would remove the duplication and reduce the risk of the two loops drifting apart on future fixes.

♻️ Possible refactor sketch
+  async `#walkUpToRoot`(
+    startDir: string,
+    getCandidate: (dir: string) => string | undefined,
+  ): Promise<string | undefined> {
+    let dir = startDir
+    while (isPathInsideOrEqual(dir, this.rootPath)) {
+      const candidate = getCandidate(dir)
+      if (candidate && await this.adapter.fileExists(candidate))
+        return candidate
+
+      if (dir === this.rootPath)
+        break
+
+      const parent = dirname(dir)
+      if (parent === dir)
+        break
+      dir = parent
+    }
+  }

Also applies to: 218-250


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e9d63ad0-d37f-4b8d-881b-722ae84eba02

📥 Commits

Reviewing files that changed from the base of the PR and between a6691a3 and 294426b.

📒 Files selected for processing (16)
  • extensions/vscode/README.md
  • extensions/vscode/package.json
  • packages/language-core/src/constants.ts
  • packages/language-core/src/workspace.test.ts
  • packages/language-core/src/workspace.ts
  • packages/language-server/src/workspace.ts
  • packages/language-service/src/index.ts
  • packages/language-service/src/plugins/catalog.test.ts
  • packages/language-service/src/plugins/catalog.ts
  • packages/language-service/src/plugins/document-link.test.ts
  • packages/language-service/src/plugins/document-link.ts
  • packages/language-service/src/plugins/installed-package-definition.test.ts
  • packages/language-service/src/plugins/installed-package-definition.ts
  • packages/language-service/src/test-utils/dependency.ts
  • packages/language-service/src/utils/document.test.ts
  • packages/language-service/src/utils/document.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.

1 participant