feat: detect package manager automatically with the vscode command#76
feat: detect package manager automatically with the vscode command#76
Conversation
📝 WalkthroughWalkthroughAdds a mapping Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/workspace.ts (1)
54-69:⚠️ Potential issue | 🔴 CriticalAlways initialise
#catalogsbefore loading manifests.For
npmworkspaces this branch leaves#catalogsunset, but Line 128 later doesthis.#catalogs!.promiseunconditionally. Opening anypackage.jsonin annpmrepo will therefore throw before dependency resolution runs.Possible fix
async loadWorkspace() { - this.#catalogs = undefined + this.#catalogs = Promise.withResolvers<CatalogsInfo | undefined>() this.packageManager = await getPackageManager(this.folder.uri) logger.info(`[workspace-context] detect package manager: ${this.packageManager}`) if (this.packageManager !== 'npm') { - this.#catalogs = Promise.withResolvers() const workspaceFilename = workspaceFileMapping[this.packageManager] const workspaceFile = Uri.joinPath(this.folder.uri, workspaceFilename) this.#catalogs.resolve( await accessOk(workspaceFile) ? (await this.loadWorkspaceCatalogInfo(workspaceFile))?.catalogs : undefined, ) + } else { + this.#catalogs.resolve(undefined) } }
🧹 Nitpick comments (1)
src/core/workspace.ts (1)
22-25: Keep workspace filename rules in one place.
workspaceFileMappingis now the obvious canonical source, butsrc/utils/file.tsLines 27-30 still hardcode the samepnpm/yarnbasenames inisWorkspaceFilePath(). That duplication will drift the next time a manager or filename changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5e19b11-35cc-46ec-a8c3-e2ad6b3ff2e4
📒 Files selected for processing (17)
src/core/workspace.tssrc/utils/package-manager.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/package-manager.test.ts
💤 Files with no reviewable changes (16)
- tests/utils/package-manager.test.ts
- tests/fixtures/workspace/dirty-doc/package.json
- src/utils/package-manager.ts
- tests/fixtures/workspace/package-manager-npm/package.json
- tests/fixtures/workspace/package-manager-yarn/package.json
- tests/fixtures/workspace/package-manager-npm/.yarnrc.yml
- tests/fixtures/workspace/package-manager-pnpm/package.json
- tests/fixtures/workspace/minimal/package.json
- tests/fixtures/workspace/dirty-doc/packages/app/package.json
- tests/fixtures/workspace/pnpm-workspace/packages/core/package.json
- tests/fixtures/workspace/pnpm-workspace/package.json
- tests/fixtures/workspace/pnpm-workspace/packages/app/package.json
- tests/fixtures/workspace/package-manager-pnpm/pnpm-workspace.yaml
- tests/fixtures/workspace/package-manager-yarn/.yarnrc.yml
- tests/fixtures/workspace/pnpm-workspace/pnpm-workspace.yaml
- tests/fixtures/workspace/package-manager-npm/pnpm-workspace.yaml
9romise
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/workspace.ts (1)
55-70:⚠️ Potential issue | 🔴 CriticalCritical:
#catalogsis never initialised for npm, causing runtime crash.When
packageManageris'npm',#catalogsremainsundefined(set at line 56, never reassigned). However,loadPackageManifestInfoat line 129 accessesthis.#catalogs!.promise, which will throwTypeError: Cannot read properties of undefined.🐛 Proposed fix: always initialise `#catalogs`
async loadWorkspace() { - this.#catalogs = undefined - this.packageManager = await getPackageManager(this.folder.uri) - - logger.info(`[workspace-context] detect package manager: ${this.packageManager}`) - - if (this.packageManager !== 'npm') { - this.#catalogs = Promise.withResolvers() - const workspaceFilename = workspaceFileMapping[this.packageManager] - const workspaceFile = Uri.joinPath(this.folder.uri, workspaceFilename) - this.#catalogs.resolve( - await accessOk(workspaceFile) - ? (await this.loadWorkspaceCatalogInfo(workspaceFile))?.catalogs - : undefined, - ) - } + this.#catalogs = Promise.withResolvers() + this.packageManager = await getPackageManager(this.folder.uri) + + logger.info(`[workspace-context] detect package manager: ${this.packageManager}`) + + if (this.packageManager === 'npm') { + this.#catalogs.resolve(undefined) + } else { + const workspaceFilename = workspaceFileMapping[this.packageManager] + const workspaceFile = Uri.joinPath(this.folder.uri, workspaceFilename) + this.#catalogs.resolve( + await accessOk(workspaceFile) + ? (await this.loadWorkspaceCatalogInfo(workspaceFile))?.catalogs + : undefined, + ) + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3236b8a-ba1e-4a24-a6e6-d69c8e03d47c
📒 Files selected for processing (1)
src/core/workspace.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/workspace.ts (1)
27-36: Consider handling falsy results more explicitly.Line 30 silently falls back to
'npm'when the command returns a falsy value (e.g.,undefinedornull), whereas line 33 only notifies the user when the command throws. If the command succeeds but returns nothing, the user won't be informed of the fallback.💡 Optional: Add logging for falsy result fallback
async function getPackageManager(uri: Uri): Promise<PackageManager> { try { const result = await commands.executeCommand<PackageManager>('npm.packageManager', uri) + if (!result) { + logger.info('[workspace-context] npm.packageManager returned no result, defaulting to npm') + } return result || 'npm' } catch (error) { console.error('Error getting package manager:', error) window.showErrorMessage('Failed to detect package manager. Defaulting to npm.') return 'npm' } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2881ec6f-f795-4e67-be70-e1c219ea7ed5
📒 Files selected for processing (1)
src/core/workspace.ts
1 participant
No description provided.