fix(windows): improve Claude CLI scan for paths with spaces #1156
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces Windows Claude CLI invocation to force UTF‑8 (prefixing with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (Renderer)
participant Main as Main Process
participant Scanner as claude-code-handlers
participant PathUtil as windows-paths
participant Cmd as cmd.exe
UI->>Main: request Claude CLI scan
Main->>Scanner: scanClaudeInstallations()
Scanner->>PathUtil: collect where()/PATH candidates
PathUtil-->>Scanner: filtered list (.cmd/.bat/.exe)
Scanner->>Scanner: skip extensionless/non-executable entries
Scanner->>Main: validateClaudeCliAsync(selectedExecutable)
Main->>Cmd: run "chcp 65001 >nul && \"<selectedExecutable>\" --version"
Cmd-->>Main: stdout/stderr (UTF-8)
Main-->>Scanner: validation result
Scanner-->>UI: installations list / status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @youngmrz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two critical issues affecting the Claude CLI installation scan on Windows. It enhances the robustness of path detection by ignoring non-executable, extensionless files and ensures that any error messages displayed are correctly rendered by enforcing UTF-8 encoding, preventing garbled text. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 provides a clear and effective fix for issues with Claude CLI scanning on Windows, specifically handling paths with spaces and fixing garbled error messages. The changes are well-implemented and directly address the problems described. I've added one comment pointing out a similar potential issue in another part of the codebase to ensure the fix is applied consistently. Overall, great work on improving the Windows user experience.
| const trimmed = p.trim(); | ||
| // On Windows, skip extensionless paths - they're not directly executable. | ||
| // Only consider .cmd, .exe, .bat files which are the actual executables. | ||
| if (!/\.(cmd|exe|bat)$/i.test(trimmed)) { | ||
| console.log('[Claude Code] Skipping non-executable path:', trimmed); | ||
| continue; | ||
| } | ||
| await addInstallation(trimmed, 'system-path'); |
There was a problem hiding this comment.
This is a great fix for handling extensionless paths from the where command on Windows. I've noticed that similar logic for finding the Claude CLI exists in apps/frontend/src/main/cli-tool-manager.ts within the detectClaude function, which in turn uses findWindowsExecutableViaWhere from apps/frontend/src/main/utils/windows-paths.ts.
The findWindowsExecutableViaWhere function appears to have a related issue where it might fall back to an extensionless path if no executable with a .cmd, .exe, or .bat extension is found first in the list from where. To ensure consistency and fix this potential bug across the application, consider making findWindowsExecutableViaWhere stricter to only return paths with valid executable extensions, similar to the logic you've added here.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 structural/other issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 1 non-blocking suggestion(s) to consider.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (1 selected of 1 total)
🔵 [d9aeefe02fce] [LOW] Duplicated Windows executable filtering regex
📁 apps/frontend/src/main/utils/windows-paths.ts:159
The regex pattern /\.(cmd|bat|exe)$/i for filtering Windows executables is duplicated in three locations: scanClaudeInstallations in claude-code-handlers.ts, and both sync/async versions of findWindowsExecutableViaWhere in windows-paths.ts. While functional, this could be extracted to a shared utility if the pattern needs to change in the future.
Suggested fix:
Consider extracting to a shared utility function like `isWindowsExecutable(path: string): boolean` if this pattern spreads to more locations
This review was generated by Auto Claude.
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
from
d7f5845 to
7786ffd
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const cmdLine = `""${cliPath}" --version"`; | ||
| // Use chcp 65001 to set UTF-8 code page, then run the command with proper quoting. | ||
| // This prevents garbled error messages when paths contain special characters. | ||
| const cmdLine = `chcp 65001 >nul && ""${cliPath}" --version"`; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
from
464a65d to
bc5e36a
Compare
| const cmdLine = `""${unquotedCmd}" --version"`; | ||
| // Use chcp 65001 to set UTF-8 code page, then run the command with proper quoting. | ||
| // This prevents garbled error messages when paths contain special characters. | ||
| const cmdLine = `chcp 65001 >nul && ""${unquotedCmd}" --version"`; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const cmdLine = `""${cliPath}" --version"`; | ||
| // Use chcp 65001 to set UTF-8 code page, then run the command with proper quoting. | ||
| // This prevents garbled error messages when paths contain special characters. | ||
| const cmdLine = `chcp 65001 >nul && ""${cliPath}" --version"`; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
from
edac03f to
fbf0e70
Compare
| // This prevents garbled error messages when paths contain special characters. | ||
| // Note: We use simple double-quote wrapping since the command doesn't start with | ||
| // a quote (chcp prefix), so cmd.exe /s won't strip outer quotes. | ||
| const cmdLine = `chcp 65001 >nul && "${unquotedCmd}" --version`; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
from
fbf0e70 to
89e1bb7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts (1)
39-39: Consider using platform abstraction functions.Per coding guidelines, prefer
isWindows()from../platformmodule instead of checkingprocess.platformdirectly. This applies to lines 39, 108, and 173.This is pre-existing code, so it can be addressed in a future cleanup pass.
♻️ Suggested refactor
+import { isWindows, isMacOS } from '../platform'; import { getToolInfo, configureTools, sortNvmVersionDirs, getClaudeDetectionPaths, type ExecFileAsyncOptionsWithVerbatim } from '../cli-tool-manager'; // Then replace: -const isWindows = process.platform === 'win32'; +const isWin = isWindows(); -if (process.platform === 'darwin') +if (isMacOS())
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
279-366: Add macOS test case for complete platform coverage.These tests only cover Win32 and Linux; macOS coverage is missing. Add a test case that mocks the macOS platform (using
'Mac'or'Darwin'value) so all three platforms are consistently tested. Per the test guidelines, platform-specific behavior should be verified across Windows, macOS, and Linux.Suggested addition (after the existing Linux test):
it('should paste on macOS', async () => { const { useXterm } = await import('../../renderer/components/terminal/useXterm'); // Mock macOS platform Object.defineProperty(navigator, 'platform', { value: 'Mac', writable: true }); // ... rest of test following same pattern as Windows/Linux tests });
🤖 Fix all issues with AI agents
In `@apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts`:
- Around line 72-78: The tests override global.requestAnimationFrame and
global.cancelAnimationFrame but never restore them; save the original globals
(e.g., const origRaf = global.requestAnimationFrame, const origCancel =
global.cancelAnimationFrame) before mocking in the setup and restore them in an
afterEach or afterAll teardown to avoid leaking into other tests (apply the same
pattern for the mock at the other location referenced around lines 106-109);
ensure you reference the mocked symbols requestAnimationFrame and
cancelAnimationFrame when locating where to save and restore.
- Around line 68-79: The beforeEach replaces global.requestAnimationFrame and
global.cancelAnimationFrame but doesn't restore them and only covers
Win32/Linux; to fix, capture the originals at top of the test file (e.g., const
_origRequestAnimationFrame = global.requestAnimationFrame; const
_origCancelAnimationFrame = global.cancelAnimationFrame), then in afterEach
restore them (global.requestAnimationFrame = _origRequestAnimationFrame;
global.cancelAnimationFrame = _origCancelAnimationFrame) and call
vi.restoreAllMocks()/vi.useRealTimers()/vi.clearAllMocks() as appropriate; also
add a macOS platform test case (platform === 'darwin') mirroring the existing
Windows and Linux tests to ensure coverage of all three platforms.
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| vi.clearAllMocks(); | ||
|
|
||
| // Mock requestAnimationFrame to prevent async operations after test teardown | ||
| global.requestAnimationFrame = vi.fn((callback) => { | ||
| return setTimeout(callback, 0) as unknown as number; | ||
| }); | ||
| global.cancelAnimationFrame = vi.fn((id) => { | ||
| clearTimeout(id); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the structure and find the file
fd terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 132
🏁 Script executed:
# Read the full test file to understand context
cat -n apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 28563
🏁 Script executed:
# Let's specifically check the beforeEach and afterEach sections
sed -n '68,110p' apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1183
🏁 Script executed:
# Check for setTimeout patterns in the file
rg -n "await new Promise.*setTimeout" apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 472
🏁 Script executed:
# Check for platform.navigator mocks
rg -n "navigator\.platform\|process\.platform" apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Let's also check what Vitest version is being used to understand useFakeTimers behavior
fd -e json -e yaml -e lock | head -20Repository: AndyMik90/Auto-Claude
Length of output: 1042
🏁 Script executed:
cd apps/frontend && cat package.json | grep -A 5 '"vitest"\|"@testing-library'Repository: AndyMik90/Auto-Claude
Length of output: 725
🏁 Script executed:
# Check if there's a vitest config that might affect timer behavior
fd vitest.config -o vite.config 2>/dev/null | head -5Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Look for any timer advancement calls in the tests
rg "advanceTimer|runAll|runOnly" apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 95
🏁 Script executed:
# Check how requestAnimationFrame is actually used in the implementation
rg -n "requestAnimationFrame\|cancelAnimationFrame" apps/frontend/src/renderer --type ts --type tsx -A 2 | head -30Repository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
# Search for RAF/CAF usage in the implementation
rg "requestAnimationFrame|cancelAnimationFrame" apps/frontend/src/renderer -A 2 | head -40Repository: AndyMik90/Auto-Claude
Length of output: 3587
🏁 Script executed:
# Check the vitest version and review fake timers behavior in context
cat apps/frontend/package.json | grep '"vitest"'Repository: AndyMik90/Auto-Claude
Length of output: 116
🏁 Script executed:
# Let's examine how useXterm handles the clipboard and RAF
fd useXterm apps/frontend/src/renderer --type fRepository: AndyMik90/Auto-Claude
Length of output: 197
🏁 Script executed:
# Check the useXterm.test.ts to see the proper pattern for RAF restoration
cat -n apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.ts | head -100Repository: AndyMik90/Auto-Claude
Length of output: 3563
🏁 Script executed:
# Let's see more of the useXterm.test.ts file to understand the complete pattern
wc -l apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 142
🏁 Script executed:
# Check if there are any test configurations for fake timers behavior
fd vitest.config.ts -o vite.config.ts | xargs cat 2>/dev/nullRepository: AndyMik90/Auto-Claude
Length of output: 212
🏁 Script executed:
# Find vitest config file properly
find apps/frontend -name "vitest.config.*" -o -name "vite.config.*" | head -3Repository: AndyMik90/Auto-Claude
Length of output: 95
🏁 Script executed:
# Check how the other test file (useXterm.test.ts) handles RAF and timers - it seems to have the right pattern
sed -n '100,150p' apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1514
🏁 Script executed:
# Check beforeAll and afterAll in useXterm.test.ts
sed -n '100,200p' apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 3064
🏁 Script executed:
# Check the complete beforeEach/afterEach pattern in useXterm.test.ts
sed -n '176,210p' apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 1123
🏁 Script executed:
# Look for all platform mocking patterns in both test files
rg -n "navigator\.platform\|process\.platform" apps/frontend/src/__tests__/integration/terminal-copy-paste.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Check if there are any platform abstraction modules being used
fd "platform" apps/frontend/src --type f | grep -v node_modules | grep -v ".test"Repository: AndyMik90/Auto-Claude
Length of output: 101
🏁 Script executed:
# Review the vitest config to understand timer behavior settings
cat -n apps/frontend/vitest.config.ts | head -50Repository: AndyMik90/Auto-Claude
Length of output: 1489
🏁 Script executed:
# Check the platform.ts file to see if there's a platform abstraction
cat -n apps/frontend/src/shared/platform.tsRepository: AndyMik90/Auto-Claude
Length of output: 2080
🏁 Script executed:
# Look for how platform detection is used in the implementation
rg "platform\." apps/frontend/src/renderer --type ts --type tsx | head -20Repository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
# Check if Darwin or MacIntel platform variants are used elsewhere in tests
rg "Darwin\|MacIntel\|navigator\.platform" apps/frontend/src --type ts --type tsx | grep -v node_modulesRepository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
# Check how platform detection is used in useXterm implementation
rg "platform\." apps/frontend/src/renderer/components/terminal/useXterm.ts -A 2Repository: AndyMik90/Auto-Claude
Length of output: 447
🏁 Script executed:
# Check how the implementation differentiates between platforms for copy/paste
rg "Win32\|Darwin\|Linux\|navigator" apps/frontend/src/renderer/components/terminal/useXterm.ts -B 2 -A 2Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Check the setup file to understand test environment configuration
cat apps/frontend/src/__tests__/setup.tsRepository: AndyMik90/Auto-Claude
Length of output: 3987
🏁 Script executed:
# Check if there's any test output or CI logs showing if these tests pass/hang
git log --oneline -20 2>/dev/null || echo "no git history"Repository: AndyMik90/Auto-Claude
Length of output: 153
🏁 Script executed:
# Look at the exact implementation to see if it actually creates pending timers with RAF
sed -n '1,150p' apps/frontend/src/renderer/components/terminal/useXterm.tsRepository: AndyMik90/Auto-Claude
Length of output: 5601
🏁 Script executed:
# Check if the tests actually run successfully - look for any recent test runs or CI
find . -name "*.log" -o -name ".github" -type d 2>/dev/null | head -5Repository: AndyMik90/Auto-Claude
Length of output: 74
🏁 Script executed:
# Check the git commit that mentions RAF mock issues
git show 037bf94 --stat 2>/dev/null || echo "can't show commit"Repository: AndyMik90/Auto-Claude
Length of output: 50377
🏁 Script executed:
# Let's see what vi.clearAllTimers vs vi.runOnlyPendingTimers does
rg "clearAllTimers|runOnlyPendingTimers" apps/frontend/src --type tsRepository: AndyMik90/Auto-Claude
Length of output: 262
🏁 Script executed:
# Verify if testing-library's act() actually flushes timers in fake timer context
rg -A 5 "beforeEach.*useFakeTimers" apps/frontend/src/renderer/components/terminal/__tests__/useXterm.test.tsRepository: AndyMik90/Auto-Claude
Length of output: 47
Restore requestAnimationFrame and cancelAnimationFrame to prevent test pollution, and add macOS platform coverage.
The global.requestAnimationFrame and global.cancelAnimationFrame assignments (lines 73–78) should be restored in afterEach to match the pattern in useXterm.test.ts. Currently, they're not stored in variables and won't be restored by vi.restoreAllMocks(). Additionally, tests only mock Windows (Win32) and Linux platforms; add a macOS (Darwin) test case to meet the testing requirement for all three platforms.
Suggested pattern (from useXterm.test.ts)
+ const originalRequestAnimationFrame = global.requestAnimationFrame;
+ const originalCancelAnimationFrame = global.cancelAnimationFrame;
beforeEach(() => {
vi.useFakeTimers();
vi.clearAllMocks();
global.requestAnimationFrame = vi.fn((callback) => {
return setTimeout(callback, 0) as unknown as number;
});
global.cancelAnimationFrame = vi.fn((id) => {
clearTimeout(id);
});Then in afterEach:
afterEach(() => {
vi.runOnlyPendingTimers();
vi.useRealTimers();
+ global.requestAnimationFrame = originalRequestAnimationFrame;
+ global.cancelAnimationFrame = originalCancelAnimationFrame;
vi.restoreAllMocks();
});🤖 Prompt for AI Agents
In `@apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts` around
lines 68 - 79, The beforeEach replaces global.requestAnimationFrame and
global.cancelAnimationFrame but doesn't restore them and only covers
Win32/Linux; to fix, capture the originals at top of the test file (e.g., const
_origRequestAnimationFrame = global.requestAnimationFrame; const
_origCancelAnimationFrame = global.cancelAnimationFrame), then in afterEach
restore them (global.requestAnimationFrame = _origRequestAnimationFrame;
global.cancelAnimationFrame = _origCancelAnimationFrame) and call
vi.restoreAllMocks()/vi.useRealTimers()/vi.clearAllMocks() as appropriate; also
add a macOS platform test case (platform === 'darwin') mirroring the existing
Windows and Linux tests to ensure coverage of all three platforms.
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts
Outdated
Show resolved
Hide resolved
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
3 times, most recently
from
cfc0881 to
6360e0b
Compare
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
2 times, most recently
from
0744cd0 to
6c9e255
Compare
|
Superseded by new PR with different branch to trigger CI |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/frontend/src/main/cli-tool-manager.ts (3)
960-974: Thechcp 65001prefix may cause validation to fail on some Windows configurations.On certain Windows systems (older versions or specific multi-byte locale configurations),
chcp 65001can fail and return a non-zero exit code. Since&&is used, this prevents the actual CLI validation command from executing, causing valid CLI installations to be incorrectly marked as invalid.Consider adding a fallback mechanism that retries without the
chcpprefix if the initial command fails:🔧 Suggested resilient approach
if (needsShell) { if (!isSecurePath(unquotedCmd)) { return { valid: false, message: `Claude CLI path failed security validation: ${unquotedCmd}`, }; } const cmdExe = process.env.ComSpec || path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe'); - // Use chcp 65001 to set UTF-8 code page, then run the command with proper quoting. - // This prevents garbled error messages when paths contain special characters. - // Note: We use simple double-quote wrapping since the command doesn't start with - // a quote (chcp prefix), so cmd.exe /s won't strip outer quotes. - const cmdLine = `chcp 65001 >nul && "${unquotedCmd}" --version`; const execOptions: ExecFileSyncOptionsWithVerbatim = { encoding: 'utf-8', timeout: 5000, windowsHide: true, windowsVerbatimArguments: true, env, }; - version = normalizeExecOutput( - execFileSync(cmdExe, ['/d', '/s', '/c', cmdLine], execOptions) - ).trim(); + // Try with UTF-8 code page first, fall back to default if chcp fails + const cmdLineUtf8 = `chcp 65001 >nul && "${unquotedCmd}" --version`; + const cmdLineFallback = `"${unquotedCmd}" --version`; + try { + version = normalizeExecOutput( + execFileSync(cmdExe, ['/d', '/s', '/c', cmdLineUtf8], execOptions) + ).trim(); + } catch { + // chcp 65001 may fail on some systems; retry without it + version = normalizeExecOutput( + execFileSync(cmdExe, ['/d', '/s', '/c', cmdLineFallback], execOptions) + ).trim(); + } }
177-178: Hardcoded Windows paths violate coding guidelines.Per the coding guidelines, platform-specific paths like
C:\Program Files\should not be hardcoded. Use environment variable expansion similar towindows-paths.ts:♻️ Suggested refactor
- 'C:\\Program Files\\Claude\\claude.exe', - 'C:\\Program Files (x86)\\Claude\\claude.exe', + joinPaths(process.env.ProgramFiles || 'C:\\Program Files', 'Claude', 'claude.exe'), + joinPaths(process.env['ProgramFiles(x86)'] || 'C:\\Program Files (x86)', 'Claude', 'claude.exe'),
675-678: Hardcoded Windows paths for GitHub CLI.Same issue as Claude paths - use environment variables instead of hardcoded paths. As per coding guidelines, avoid hardcoding
C:\Program Files\.♻️ Suggested refactor
const windowsPaths = [ - 'C:\\Program Files\\GitHub CLI\\gh.exe', - 'C:\\Program Files (x86)\\GitHub CLI\\gh.exe', + joinPaths(process.env.ProgramFiles || 'C:\\Program Files', 'GitHub CLI', 'gh.exe'), + joinPaths(process.env['ProgramFiles(x86)'] || 'C:\\Program Files (x86)', 'GitHub CLI', 'gh.exe'), ];apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
280-288: Missing macOS platform test coverage.Per the coding guidelines, tests for platform-specific code should cover all three platforms (Windows, macOS, Linux). The clipboard read/paste tests only cover
Win32(line 285) andLinux(line 364), but notDarwin(macOS).Add a test case for macOS:
🧪 Add macOS test case
it('should integrate clipboard.readText() with xterm.paste() on macOS', async () => { const { useXterm } = await import('../../renderer/components/terminal/useXterm'); // Mock macOS platform Object.defineProperty(navigator, 'platform', { value: 'MacIntel', writable: true }); // ... rest of test similar to Win32/Linux versions });apps/frontend/src/main/utils/windows-paths.ts (1)
92-94: Consider usingisWindows()from platform module.Per coding guidelines, prefer platform abstraction functions over direct
process.platformchecks. While this is a Windows-specific utility file, usingisWindows()would maintain consistency:+import { isWindows } from '../platform'; + export function getWindowsExecutablePaths( toolPaths: WindowsToolPaths, logPrefix: string = '[Windows Paths]' ): string[] { - if (process.platform !== 'win32') { + if (!isWindows()) { return []; }This applies to lines 92, 139, 190, and 243.
apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts (1)
39-39: Use platform abstraction functions instead of directprocess.platformchecks.Per coding guidelines, prefer
isWindows(),isMacOS(),isLinux()from the platform module. This file has multiple directprocess.platformchecks (lines 39, 108, 173, 320, 334, 399, 503, etc.).+import { isWindows, isMacOS, isLinux } from '../platform'; + async function validateClaudeCliAsync(cliPath: string): Promise<[boolean, string | null]> { try { - const isWindows = process.platform === 'win32'; + const isWin = isWindows(); // Security validation... - if (isWindows && !isSecurePath(cliPath)) { + if (isWin && !isSecurePath(cliPath)) {
♻️ Duplicate comments (3)
apps/frontend/src/main/cli-tool-manager.ts (1)
1103-1115: Samechcp 65001resilience issue in async validation.The async version has the same vulnerability where
chcp 65001failure would prevent validation. Apply the same fallback pattern as suggested for the sync version.apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts (1)
62-73: Samechcp 65001resilience concern as in cli-tool-manager.ts.This validation function has the same vulnerability where
chcp 65001failure would prevent the CLI from being validated on systems that don't support UTF-8 code page. Consider implementing a fallback mechanism.apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
68-79: GlobalrequestAnimationFrame/cancelAnimationFrameare not restored after tests.The RAF mock replaces the globals but the originals aren't saved and restored in
afterEach. Whilevi.restoreAllMocks()restores mocked functions, it doesn't restore manually assigned globals. This can leak into subsequent test files.🔧 Suggested fix
describe('Terminal copy/paste integration', () => { let mockClipboard: { writeText: Mock; readText: Mock; }; + let originalRequestAnimationFrame: typeof global.requestAnimationFrame; + let originalCancelAnimationFrame: typeof global.cancelAnimationFrame; beforeEach(() => { vi.useFakeTimers(); vi.clearAllMocks(); + originalRequestAnimationFrame = global.requestAnimationFrame; + originalCancelAnimationFrame = global.cancelAnimationFrame; + // Mock requestAnimationFrame to return an ID but not schedule real timers let rafId = 0; global.requestAnimationFrame = vi.fn(() => { return ++rafId; }); global.cancelAnimationFrame = vi.fn();Then in
afterEach:afterEach(() => { vi.runOnlyPendingTimers(); vi.useRealTimers(); + global.requestAnimationFrame = originalRequestAnimationFrame; + global.cancelAnimationFrame = originalCancelAnimationFrame; vi.restoreAllMocks(); });
Two fixes for issue AndyMik90#1050: 1. Skip extensionless paths from `where claude` results on Windows. These stubs are not directly executable and cause ENOENT errors. Only validate .cmd, .exe, and .bat files. 2. Use `chcp 65001` to set UTF-8 code page before running cmd.exe. This prevents garbled error messages when validating CLI paths that contain spaces or special characters. Fixes AndyMik90#1050 Co-Authored-By: Claude Opus 4.5 <[email protected]>
…here Apply the same fix to windows-paths.ts utility functions: - findWindowsExecutableViaWhere (sync) - findWindowsExecutableViaWhereAsync (async) Previously these functions would fall back to extensionless paths (e.g., `paths[0]`) if no .cmd/.exe/.bat was found first. Now they strictly filter to only return valid Windows executable extensions. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Apply the same chcp 65001 fix from claude-code-handlers.ts to both validateClaude and validateClaudeAsync in cli-tool-manager.ts. This ensures error messages are correctly encoded in UTF-8 on Windows when validating CLI paths through any code path (initial setup, settings panel, sidebar click). Addresses Sentry feedback about inconsistent UTF-8 handling. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The /s flag only strips outer quotes when the command starts with a
quote character. Since we prepend 'chcp 65001 >nul && ', the command
no longer starts with a quote, so the ""path"" pattern would result
in unbalanced quotes.
Use simple double-quote wrapping ("path") which works correctly
when the command doesn't rely on /s quote stripping.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Signed-off-by: youngmrz <[email protected]>
Changes: - Use proper quoting for paths in shell-escape utility functions - Fix test: prevent infinite timer loop in requestAnimationFrame mock The requestAnimationFrame mock was using setTimeout(callback, 0) which caused vi.runAllTimersAsync() to enter an infinite loop. Changed to return a unique ID without scheduling timers. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
Signed-off-by: youngmrz <[email protected]>
youngmrz
force-pushed
the
fix/windows-cli-scan-paths
branch
from
0474b6c to
4e3d738
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
280-357: Missing macOS (Darwin) platform test coverage.The clipboard read tests cover Windows (
Win32at line 285) and Linux (line 364), but there's no test case for macOS (Darwin). As per coding guidelines, platform-specific code should be tested on all three platforms.♻️ Add macOS test case
it('should integrate clipboard.readText() with xterm.paste() on macOS', async () => { const { useXterm } = await import('../../renderer/components/terminal/useXterm'); // Mock macOS platform Object.defineProperty(navigator, 'platform', { value: 'MacIntel', // or 'Darwin' depending on what the implementation checks writable: true }); // ... rest of test similar to Windows/Linux versions });
♻️ Duplicate comments (1)
apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts (1)
68-79: RAF mock uses non-scheduling approach to prevent infinite loops - good fix.The comment correctly explains why
setTimeoutis avoided in the RAF mock: it would cause infinite loops withvi.runAllTimersAsync()because RAF callbacks schedule more timers.However, the
requestAnimationFrameandcancelAnimationFrameglobals are overwritten without being stored and restored. While past comments marked this as "addressed," I don't see the restoration code in the currentafterEach.♻️ Suggested fix to properly restore RAF/CAF
+ const originalRequestAnimationFrame = global.requestAnimationFrame; + const originalCancelAnimationFrame = global.cancelAnimationFrame; + beforeEach(() => { vi.useFakeTimers(); vi.clearAllMocks(); // Mock requestAnimationFrame to return an ID but not schedule real timers let rafId = 0; global.requestAnimationFrame = vi.fn(() => { return ++rafId; }); global.cancelAnimationFrame = vi.fn(); // ... rest of setup }); afterEach(() => { vi.runOnlyPendingTimers(); vi.useRealTimers(); + global.requestAnimationFrame = originalRequestAnimationFrame; + global.cancelAnimationFrame = originalCancelAnimationFrame; vi.restoreAllMocks(); });
2 participants
Summary
where claudeon WindowsProblem
When clicking "Claude Code" in the sidebar, the installation scan fails:
D:\Program Files\...\claude→ ENOENTclaude.cmdbut stderr is garbled Chinese charactersSolution
1. Filter extensionless paths
2. Set UTF-8 code page
Test plan
D:\Program Files\...)Fixes #1050
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.