fix(terminals): correct Windows shell defaults#2385
Conversation
Greptile SummaryThis PR fixes Windows terminal shell defaults:
Confidence Score: 5/5Safe to merge; all changed paths are well-tested and the one cosmetic regression is minor and easily patched. The shell resolver, PTY spawn, and drawer-management changes are each covered by focused regression tests that were run and passed. The previous two thread concerns (System32-only WSL detection and the session-restore empty-drawer edge case) are both addressed with test coverage. The only new finding is a cosmetic icon regression in the system shell row of the terminal shell picker, which does not affect functionality. src/renderer/lib/components/terminal-shell-option-label.tsx — the entry.id change loses the shell icon for the system entry.
|
| Filename | Overview |
|---|---|
| src/main/core/terminal-shell/resolver.ts | Adds wsl shell id with System32-only detection (no PATH fallback), Git Bash resolution that filters WSL's bash launcher, consolidated explicitShellLabel helper, and removes pwsh from the default display list while preserving stored-settings resolution. |
| src/main/core/pty/pty-spawn-platform.ts | Adds wsl family support throughout the Windows spawn path: interactive shells launch wsl.exe directly with no args, run-command routes through argvToPosixShellLine + windowsShellLineSpawn, and POSIX spawn guards throw for wsl family. |
| src/renderer/features/tasks/stores/workspace-view-model.tsx | Replaces the old auto-create-terminal reaction with a close-empty-drawer reaction using fireImmediately:true, handling both the session-restore case (previous===undefined) and the last-terminal-closed case (previous.terminalCount > 0). The isCreatingTerminal guard prevents mid-flight closure. |
| src/renderer/features/tasks/commands.ts | Ctrl+J now opens a new terminal when no tabs exist instead of toggling the drawer; closes the drawer when already open, and opens it when tabs exist. openNewTerminal already sets isTerminalDrawerOpen=true internally so the UX flow is consistent. |
| src/renderer/lib/components/terminal-shell-option-label.tsx | Switches icon lookup from entry.label to entry.id, fixing icon resolution for human-readable labels like "Git Bash" and "PowerShell", but causes the system entry (id='system') to always render the generic Terminal icon instead of the actual shell's icon. |
| src/shared/terminal-settings.ts | Adds wsl to TERMINAL_SHELL_IDS, RUNTIME_TERMINAL_SHELL_IDS, TerminalShellFamily, terminalShellFamily, terminalInteractiveShellArgs, terminalCommandArgs, and terminalEnvCaptureArgs with appropriate wsl semantics (--exec sh -lc, no env capture). |
| src/main/core/terminal-shell/resolver.test.ts | Adds tests for WSL System32-only resolution, WSL rejection from arbitrary PATH, Git Bash vs WSL bash launcher distinction, and the new single-PowerShell availability display. |
| src/renderer/features/tasks/stores/workspace-view-model.test.ts | Adds three new reaction tests: restore with already-loaded empty state, restore with late-loading empty state (isLoaded transitions to true), and user-closes-last-terminal; all verify the drawer closes and focusedRegion resets to main. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Ctrl+J pressed"] --> B{drawer open?}
B -- yes --> C[close drawer]
B -- no --> D{terminal tabs exist?}
D -- yes --> E[open drawer]
D -- no --> F[openNewTerminal]
F --> G[isTerminalDrawerOpen = true
_isCreatingTerminal = true]
G --> H[createDefaultTerminal]
H --> I[terminal added to registry]
I --> J[_isCreatingTerminal = false]
K["Session restored / terminal closed"] --> L{isLoaded && terminalCount==0
&& isDrawerOpen}
L -- "previous==undefined OR
prev.terminalCount>0 OR
!prev.isLoaded" --> M[close drawer
clear activeItem
focusedRegion → main]
L -- no --> N[no-op]
subgraph "bash resolve - Windows"
O["intent: bash"] --> P{Git Bash in known paths?}
P -- yes --> Q[return Git Bash exe]
P -- no --> R[findOnPath bash.exe
excluding WSL launcher]
R --> S{isWindowsWslBashLauncher?}
S -- yes --> T[skip candidate]
S -- no --> U[return candidate]
end
subgraph "wsl resolve - Windows"
V["intent: wsl"] --> W{SystemRoot set?}
W -- yes --> X[check System32/wsl.exe only]
X -- exists --> Y[return wsl.exe]
X -- missing --> Z[ShellUnavailableError]
W -- no --> Z
end
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/renderer/lib/components/terminal-shell-option-label.tsx:58
The `system` entry has `id === 'system'`, which is not in `SHELL_DEVICON_CLASS`, so it always falls through to the generic `Terminal` lucide icon. On macOS the zsh or bash icon disappears; on Windows the cmd or PowerShell icon disappears. The `system` entry's `label` still carries the actual shell basename (e.g. `"zsh"`, `"bash"`, `"cmd"`), so a simple fallback — try the `id` first, then fall back to the `label` — restores the correct icon for the system row while keeping the explicit-shell fix intact.
```suggestion
<TerminalShellIcon shell={entry.id === 'system' ? entry.label : entry.id} />
```
Reviews (2): Last reviewed commit: "fix(terminals): address Windows shell re..." | Re-trigger Greptile
|
@greptile-apps Updated summary: This PR fixes Windows terminal shell defaults and terminal drawer behavior.
|
…3-terminal-shell-windows
|
Hi @Drakaniia, Thank you for opening this PR, and for your other contributions and issue reports lately. The feedback has been highly valuable and is much appreciated. Since this PR was opened, we’ve done some refactoring across the codebase to prepare for the next larger topic we’re working toward. That said, the changes in this PR still make a lot of sense. Would you be able to resolve the merge conflicts caused by the file moves? Once that’s done, I’d be happy to merge it into |
- Move the terminal shell fixes onto apps/emdash-desktop after the upstream monorepo refactor. - Preserve the new browser command tests alongside the terminal drawer regression test. - Apply the system shell icon fallback requested in the PR review.
|
Resolved and pushed PR #2385 conflict. Merged current Verification passed:
|
Description
Corrects Windows terminal shell behavior so the default terminal flow matches the selected shell and avoids the empty terminal placeholder after the last terminal is closed.
Implementation notes:
bashshell id to Git Bash on Windows instead of the Windows WSLbash.exelauncher.wslshell id for WSL-backed terminals.PowerShelloption in Windows terminal settings, while keeping explicitpwshresolution available for existing stored settings.Ctrl+Jcreate/open the configured default terminal when no terminal tabs exist, and hide the drawer when it is already open.Related issues
Fixes #2383.
Testing
pnpm exec vitest run src/renderer/features/tasks/commands.test.ts src/renderer/features/tasks/stores/workspace-view-model.test.ts src/renderer/features/tasks/terminals/terminal-manager.test.tspnpm exec vitest run src/main/core/terminal-shell/resolver.test.ts src/main/core/pty/pty-spawn-platform.test.ts src/renderer/features/tasks/commands.test.ts src/renderer/features/tasks/stores/workspace-view-model.test.ts src/renderer/features/tasks/terminals/terminal-manager.test.tspnpm run typecheckpnpm run lintpnpm exec oxfmt --check src/main/core/pty/pty-spawn-platform.test.ts src/main/core/pty/pty-spawn-platform.ts src/main/core/terminal-shell/resolver.test.ts src/main/core/terminal-shell/resolver.ts src/renderer/features/tasks/commands.test.ts src/renderer/features/tasks/commands.ts src/renderer/features/tasks/stores/workspace-view-model.test.ts src/renderer/features/tasks/stores/workspace-view-model.tsx src/renderer/lib/components/terminal-shell-option-label.tsx src/shared/terminal-settings.tsgit diff --checkNote:
pnpm run testwas also attempted locally, but it fails on this Windows checkout in unrelated path/temp-fixture and native PTY suites. The focused terminal-related tests above pass.Recording
Video_260605094718.mp4
Checklist
messages and, when possible, the PR title