fix(terminal): support Windows shell selection for SSH hosts#6466
Conversation
📝 WalkthroughWalkthroughThis PR extracts shared preflight command execution helpers, adds remote Windows Terminal capability plumbing through main IPC, runtime RPC, preload, web, and relay layers, forwards SSH PTY shell override fields, updates renderer capability loading and tab bar behavior to use SSH connection state and remote host platform data, and localizes workspace cleanup UI strings and fixtures. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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
🧹 Nitpick comments (3)
src/main/ipc/preflight-remote-windows-terminal-capabilities.ts (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturning a shared mutable default could leak mutations.
EMPTY_REMOTE_WINDOWS_TERMINAL_CAPABILITIES(including itswslDistros: []array) is returned by reference on both the disposed and missing-result paths. Any downstream consumer that mutateswslDistros(e.g..push) would corrupt the shared constant for every future empty result. Consider returning a fresh object/array.♻️ Proposed fix
-const EMPTY_REMOTE_WINDOWS_TERMINAL_CAPABILITIES: RemoteWindowsTerminalCapabilities = { - wslAvailable: false, - wslDistros: [], - pwshAvailable: false, - gitBashAvailable: false, - hostPlatform: null -} +function emptyRemoteWindowsTerminalCapabilities(): RemoteWindowsTerminalCapabilities { + return { + wslAvailable: false, + wslDistros: [], + pwshAvailable: false, + gitBashAvailable: false, + hostPlatform: null + } +}(and return
emptyRemoteWindowsTerminalCapabilities()at both sites)src/relay/preflight-handler.ts (1)
70-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefensive
.catch()does not protect against synchronous throws.
isWslAvailable,isPwshAvailable,isGitBashAvailable, andlistWslDistrosare synchronous functions. InPromise.resolve(isWslAvailable()).catch(() => false),isWslAvailable()is evaluated synchronously beforePromise.resolve, so a synchronous throw bypasses.catch, rejectsPromise.all, and fails the whole handler. Today these helpers swallow errors internally so it's latent, but the guard doesn't deliver its intended fallback. Wrapping each call in an async thunk makes the.catcheffective.🛡️ Proposed fix
- const [wslAvailable, pwshAvailable, gitBashAvailable] = await Promise.all([ - Promise.resolve(isWslAvailable()).catch(() => false), - Promise.resolve(isPwshAvailable()).catch(() => false), - Promise.resolve(isGitBashAvailable()).catch(() => false) - ]) - const wslDistros = wslAvailable ? await Promise.resolve(listWslDistros()).catch(() => []) : [] + const [wslAvailable, pwshAvailable, gitBashAvailable] = await Promise.all([ + (async () => isWslAvailable())().catch(() => false), + (async () => isPwshAvailable())().catch(() => false), + (async () => isGitBashAvailable())().catch(() => false) + ]) + const wslDistros = wslAvailable + ? await (async () => listWslDistros())().catch(() => []) + : []src/renderer/src/components/tab-bar/windows-shell-menu-visibility.ts (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short why-comment for the SSH-only branch.
The new early return is the non-obvious policy change here, but the existing note only explains runtime-host gating. A one-line comment above Line 11 would clarify that SSH worktrees must use the remote host platform and ignore the client OS.
As per coding guidelines, "**/*.{ts,tsx,js,jsx}: When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does.`"Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51be9d1a-9738-4f54-9391-6a06cba1c39a
📒 Files selected for processing (21)
src/main/ipc/preflight-command-exec.tssrc/main/ipc/preflight-remote-windows-terminal-capabilities.tssrc/main/ipc/preflight.test.tssrc/main/ipc/preflight.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/runtime/rpc/methods/preflight.test.tssrc/main/runtime/rpc/methods/preflight.tssrc/preload/api-types.tssrc/preload/index.tssrc/relay/preflight-handler.test.tssrc/relay/preflight-handler.tssrc/relay/pty-handler.test.tssrc/relay/pty-handler.tssrc/renderer/src/components/tab-bar/TabBar.tsxsrc/renderer/src/components/tab-bar/TabBar.windows-shell-launch.test.tssrc/renderer/src/components/tab-bar/windows-shell-menu-visibility.test.tssrc/renderer/src/components/tab-bar/windows-shell-menu-visibility.tssrc/renderer/src/lib/windows-terminal-capabilities.test.tssrc/renderer/src/lib/windows-terminal-capabilities.tssrc/renderer/src/web/web-preload-api.ts
Jinwoo-H
left a comment
Jinwoo-H
left a comment
There was a problem hiding this comment.
Thanks, this is in good shape now. I pushed review fixes to preserve Windows shell selection only for Windows SSH hosts, keep non-Windows SSH relays from receiving Windows-only shell overrides, and resolve the current main merge conflict.\n\nValidated with typecheck, lint, focused Vitest coverage, Electron/CDP UI state checks for Windows/Linux SSH shell menu behavior, and a real Docker SSH relay/PTY run.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e235ca6-0509-4820-a813-06a0852ecb68
📒 Files selected for processing (13)
src/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/preload/api-types.tssrc/preload/index.tssrc/relay/pty-handler.test.tssrc/relay/pty-handler.tssrc/renderer/src/components/workspace-cleanup/WorkspaceCleanupDialog.tsxsrc/renderer/src/components/workspace-cleanup/workspace-cleanup-presentation-fixtures.tssrc/renderer/src/components/workspace-cleanup/workspace-cleanup-presentation.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.json
💤 Files with no reviewable changes (7)
- src/renderer/src/components/workspace-cleanup/workspace-cleanup-presentation-fixtures.ts
- src/renderer/src/components/workspace-cleanup/workspace-cleanup-presentation.ts
- src/renderer/src/components/workspace-cleanup/WorkspaceCleanupDialog.tsx
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/i18n/locales/ja.json
- src/renderer/src/i18n/locales/es.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/preload/index.ts
- src/main/providers/ssh-pty-provider.test.ts
- src/main/providers/ssh-pty-provider.ts
- src/preload/api-types.ts
| function resolvePtyShellOverride(shellOverride: string): string { | ||
| if (!shellOverride) { | ||
| return '' | ||
| } | ||
| if (process.platform !== 'win32') { | ||
| return '' | ||
| } | ||
| const normalized = shellOverride.toLowerCase() | ||
| if (!ALLOWED_WINDOWS_SHELL_OVERRIDES.has(normalized)) { | ||
| throw new Error(`Unsupported Windows shell override: ${shellOverride}`) | ||
| } | ||
| return resolveWindowsGitBashShellPath(shellOverride) ?? shellOverride |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Let the Git Bash resolver run before the named-shell allowlist.
resolveWindowsGitBashShellPath() already accepts the canonical git-bash token, bare bash.exe, and validated Git-for-Windows paths, but the new allowlist rejects the latter two before they ever reach that resolver. That means a valid Git Bash override can now throw Unsupported Windows shell override instead of being normalized and launched.
Suggested fix
function resolvePtyShellOverride(shellOverride: string): string {
if (!shellOverride) {
return ''
}
if (process.platform !== 'win32') {
return ''
}
+ const resolvedGitBash = resolveWindowsGitBashShellPath(shellOverride)
+ if (resolvedGitBash) {
+ return resolvedGitBash
+ }
const normalized = shellOverride.toLowerCase()
if (!ALLOWED_WINDOWS_SHELL_OVERRIDES.has(normalized)) {
throw new Error(`Unsupported Windows shell override: ${shellOverride}`)
}
- return resolveWindowsGitBashShellPath(shellOverride) ?? shellOverride
+ return shellOverride
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function resolvePtyShellOverride(shellOverride: string): string { | |
| if (!shellOverride) { | |
| return '' | |
| } | |
| if (process.platform !== 'win32') { | |
| return '' | |
| } | |
| const normalized = shellOverride.toLowerCase() | |
| if (!ALLOWED_WINDOWS_SHELL_OVERRIDES.has(normalized)) { | |
| throw new Error(`Unsupported Windows shell override: ${shellOverride}`) | |
| } | |
| return resolveWindowsGitBashShellPath(shellOverride) ?? shellOverride | |
| function resolvePtyShellOverride(shellOverride: string): string { | |
| if (!shellOverride) { | |
| return '' | |
| } | |
| if (process.platform !== 'win32') { | |
| return '' | |
| } | |
| const resolvedGitBash = resolveWindowsGitBashShellPath(shellOverride) | |
| if (resolvedGitBash) { | |
| return resolvedGitBash | |
| } | |
| const normalized = shellOverride.toLowerCase() | |
| if (!ALLOWED_WINDOWS_SHELL_OVERRIDES.has(normalized)) { | |
| throw new Error(`Unsupported Windows shell override: ${shellOverride}`) | |
| } | |
| return shellOverride | |
| } |
2 participants
Summary
connectionId, so non-Windows SSH hosts and unrelated connections keep existing behavior.Testing
pnpm lintpnpm typecheckpnpm testpnpm buildFocused local validation:
pnpm exec vitest run --config config/vitest.config.ts src/relay/pty-handler.test.ts src/main/ipc/preflight.test.ts src/main/runtime/rpc/methods/preflight.test.ts src/main/providers/ssh-pty-provider.test.ts src/relay/preflight-handler.test.tspnpm exec vitest run --config config/vitest.config.ts src/renderer/src/components/tab-bar/windows-shell-menu-visibility.test.ts src/renderer/src/components/tab-bar/TabBar.windows-shell-launch.test.ts src/renderer/src/lib/windows-terminal-capabilities.test.tspnpm run typecheck:webpnpm run typecheck:nodeAI Review Report
preflight.detectRemoteWindowsTerminalCapabilities; that method and its dispatcher test were added.git-bashsentinel into relay spawn; the relay now resolves that sentinel to the remotebash.exepath and covers it with a regression test.Security Audit
connectionIdcontract and do not widen into arbitrary command execution.Notes