fix: detect Claude CLI installed via npm on non-C drive Windows#2004
fix: detect Claude CLI installed via npm on non-C drive Windows#2004octo-patch wants to merge 1 commit intoAndyMik90:developfrom
Conversation
|
octo-patch seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughUpdated npm global prefix detection in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/env-utils.ts`:
- Around line 142-147: The prefix extracted from the .npmrc lines (via the regex
match in the loop that assigns const prefix = match[1].trim()) can include
surrounding quotes (e.g., "E:\npm"); update the extraction logic to strip any
surrounding single or double quotes from match[1] before trimming/returning so
callers like fs.existsSync receive a raw path; ensure you still return null for
empty values. Target the block that computes prefix from
line.match(/^prefix\s*=\s*(.+)$/) and apply a small sanitization step to remove
leading/trailing quotes.
- Around line 428-436: readNpmrcPrefix currently performs sync I/O which blocks
the event loop; add an async variant (e.g., readNpmrcPrefixAsync) that uses
non-blocking fs.promises APIs (or existsAsync + fs.promises.readFile) to read
and parse the .npmrc prefix, then replace the synchronous call in the async flow
with await readNpmrcPrefixAsync() in the same block that sets
npmGlobalPrefixCache and logs the prefix; ensure the new function preserves the
same parsing semantics and returns null/undefined when no prefix is found so the
existing existsAsync and binPath handling remains valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9024af8-6355-4307-bad6-495ab4ca7658
📒 Files selected for processing (1)
apps/desktop/src/main/env-utils.ts
| for (const line of content.split(/\r?\n/)) { | ||
| const match = line.match(/^prefix\s*=\s*(.+)$/); | ||
| if (match) { | ||
| const prefix = match[1].trim(); | ||
| return prefix || null; | ||
| } |
There was a problem hiding this comment.
Minor: Quoted prefix values not handled.
If a user's .npmrc contains a quoted value like prefix="E:\npm", the regex captures the quotes, causing fs.existsSync to fail. This is an uncommon edge case but worth considering.
🔧 Proposed fix to strip quotes
const match = line.match(/^prefix\s*=\s*(.+)$/);
if (match) {
- const prefix = match[1].trim();
+ let prefix = match[1].trim();
+ // Strip surrounding quotes if present (e.g., prefix="E:\npm")
+ if ((prefix.startsWith('"') && prefix.endsWith('"')) ||
+ (prefix.startsWith("'") && prefix.endsWith("'"))) {
+ prefix = prefix.slice(1, -1);
+ }
return prefix || null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/env-utils.ts` around lines 142 - 147, The prefix
extracted from the .npmrc lines (via the regex match in the loop that assigns
const prefix = match[1].trim()) can include surrounding quotes (e.g., "E:\npm");
update the extraction logic to strip any surrounding single or double quotes
from match[1] before trimming/returning so callers like fs.existsSync receive a
raw path; ensure you still return null for empty values. Target the block that
computes prefix from line.match(/^prefix\s*=\s*(.+)$/) and apply a small
sanitization step to remove leading/trailing quotes.
| const npmrcPrefix = readNpmrcPrefix(os.homedir()); | ||
| if (npmrcPrefix) { | ||
| const binPath = path.normalize(npmrcPrefix); | ||
| if (await existsAsync(binPath)) { | ||
| console.warn('[env-utils] Using npm prefix from .npmrc:', binPath); | ||
| npmGlobalPrefixCache = binPath; | ||
| return binPath; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sync I/O in async function contradicts non-blocking design.
readNpmrcPrefix() uses fs.existsSync and fs.readFileSync, blocking the event loop. This contradicts the docstring stating this is "non-blocking" and "Safe to call from Electron main process without blocking the event loop."
While .npmrc files are typically small, consider adding an async variant for consistency.
♻️ Proposed async variant
+/**
+ * Read npm global prefix from .npmrc file asynchronously
+ */
+async function readNpmrcPrefixAsync(homedir: string): Promise<string | null> {
+ const npmrcPath = path.join(homedir, '.npmrc');
+ if (!await existsAsync(npmrcPath)) {
+ return null;
+ }
+
+ try {
+ const content = await fsPromises.readFile(npmrcPath, 'utf-8');
+ for (const line of content.split(/\r?\n/)) {
+ const match = line.match(/^prefix\s*=\s*(.+)$/);
+ if (match) {
+ const prefix = match[1].trim();
+ return prefix || null;
+ }
+ }
+ } catch {
+ // Ignore read errors (permission issues, etc.)
+ }
+
+ return null;
+}Then update the async fallback:
- const npmrcPrefix = readNpmrcPrefix(os.homedir());
+ const npmrcPrefix = await readNpmrcPrefixAsync(os.homedir());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/env-utils.ts` around lines 428 - 436, readNpmrcPrefix
currently performs sync I/O which blocks the event loop; add an async variant
(e.g., readNpmrcPrefixAsync) that uses non-blocking fs.promises APIs (or
existsAsync + fs.promises.readFile) to read and parse the .npmrc prefix, then
replace the synchronous call in the async flow with await readNpmrcPrefixAsync()
in the same block that sets npmGlobalPrefixCache and logs the prefix; ensure the
new function preserves the same parsing semantics and returns null/undefined
when no prefix is found so the existing existsAsync and binPath handling remains
valid.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism for Windows to determine the npm global prefix by reading the .npmrc file, specifically for cases where npm.cmd is not available in the PATH. The review feedback highlights two main areas for improvement: the use of synchronous file reading within an asynchronous function which could block the event loop, and the need for more robust parsing of the .npmrc file to correctly handle quotes, comments, and multiple prefix entries.
| } | ||
|
|
||
| try { | ||
| const content = fs.readFileSync(npmrcPath, 'utf-8'); |
There was a problem hiding this comment.
The readNpmrcPrefix function uses fs.readFileSync, which is a blocking operation. While this is acceptable for the synchronous getNpmGlobalPrefix, it is also called by getNpmGlobalPrefixAsync (line 428). Since getNpmGlobalPrefixAsync is intended to be a non-blocking alternative for the Electron main process, consider using fs.promises.readFile or providing an asynchronous version of this helper to avoid blocking the event loop during app startup.
| for (const line of content.split(/\r?\n/)) { | ||
| const match = line.match(/^prefix\s*=\s*(.+)$/); | ||
| if (match) { | ||
| const prefix = match[1].trim(); | ||
| return prefix || null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current .npmrc parsing logic is fragile and doesn't follow standard npm configuration rules:
- Quotes: npm allows values to be quoted (e.g.,
prefix="E:\npm").fs.existsSyncwill fail if these quotes are not stripped. - Comments: It doesn't ignore lines starting with
#or;, nor does it handle trailing comments on the same line (e.g.,prefix=E:\npm # comment). - Precedence: If multiple
prefixentries exist in the same file, npm uses the last one. The current code returns the first one found.
let lastPrefix: string | null = null;
for (const line of content.split(/\r?\n/)) {
const trimmedLine = line.trim();
if (!trimmedLine || trimmedLine.startsWith('#') || trimmedLine.startsWith(';')) {
continue;
}
const match = trimmedLine.match(/^prefix\s*=\s*(.+)$/);
if (match) {
let prefix = match[1].split(/[#;]/)[0].trim();
if ((prefix.startsWith('"') && prefix.endsWith('"')) || (prefix.startsWith("'") && prefix.endsWith("'"))) {
prefix = prefix.slice(1, -1);
}
if (prefix) {
lastPrefix = prefix;
}
}
}
return lastPrefix;2 participants
Fixes #1922
Problem
When Node.js or npm is installed on a non-C drive on Windows (e.g.,
E:\nodejs),npm.cmdmay not appear in the PATH that the Electron app inherits when launched from the GUI. In this scenario,getNpmGlobalPrefix()throws when trying to runnpm config get prefix, and falls back only to the hardcoded default%APPDATA%\npmpath — missing any custom npm global prefix the user may have configured (e.g.,npm config set prefix E:\npm).This causes Claude CLI (and other tools like GitHub CLI) installed via
npm install -gon a non-standard drive to go undetected.Solution
Add a
readNpmrcPrefix()helper that parses the user's~/.npmrcfile for aprefix=line. This is used as a new fallback in both the sync and async variants ofgetNpmGlobalPrefix()— tried before the hardcoded%APPDATA%\npmdefault when the npm command itself is unavailable.The
.npmrcfile is a simple key-value format that npm writes when users runnpm config set prefix <path>. Reading it directly avoids the dependency on npm being in PATH, making detection reliable even when Node.js is installed on a non-standard drive.Testing
readNpmrcPrefix()function correctly parsesprefix=E:\npmfrom a.npmrcfile%APPDATA%\npmis preserved when.npmrchas no prefix lineSummary by CodeRabbit