Uh oh!
There was an error while loading. Please reload this page.
feat: allow selecting terminal shell on Windows - #6073
feat: allow selecting terminal shell on Windows#6073mohamedmastouri-hue wants to merge 1 commit into
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
Two Effect service convention issues in apps/server/src/terminal/Manager.ts: the new serverSettings.ts import uses a named service import (and is never used/wired), and the shellResolver option was switched to an Effect without updating existing callers.
Posted via Macroscope — Effect Service Conventions
| import { ServerSettingsService } from "../serverSettings.ts"; |
There was a problem hiding this comment.
At a service boundary the local service module should be imported as a namespace (ServerSettings.ServerSettingsService), not as a named import. This import is also currently unused — production make() never does yield* ServerSettings.ServerSettingsService, so the new windowsTerminalShell setting is never fed into shellResolver and the dependency is not reflected in make/layer requirements. Consider either wiring it up or dropping the import.
-import { ServerSettingsService } from "../serverSettings.ts";+import * as ServerSettings from "../serverSettings.ts";Posted via Macroscope — Effect Service Conventions
| shellResolver?: Effect.Effect<string, never, never>; |
There was a problem hiding this comment.
Changing this seam from () => string to an Effect leaves existing consumers on the old shape: apps/server/src/terminal/Manager.test.ts still declares shellResolver?: () => string (line 205) and passes thunks (lines 1287, 1344, 1482), which no longer type-check against this option. Consider updating those callers mechanically (e.g. shellResolver: Effect.succeed("/bin/zsh")) as part of this change.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8150931. Configure here.
| const baseEnv = options.env ?? process.env; | ||
| const shellResolver = | ||
| options.shellResolver ?? | ||
| Effect.succeed(defaultShellResolver(platform, baseEnv)); |
There was a problem hiding this comment.
Terminal shell setting unused
High Severity
ServerSettingsService is imported but never used, and make() never passes a shellResolver that reads windowsTerminalShell. Production terminals always fall back to defaultShellResolver, so the Windows shell setting persists in UI/settings but has no effect on spawned PTYs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8150931. Configure here.
There was a problem hiding this comment.
🟡 Medium
changedSettingLabels reads settings.windowsTerminalShell but the useMemo dependency array omits it. When the user changes only the terminal-shell setting, the memo stays stale, so the restore-defaults control fails to include "Terminal shell" in the confirmation until another listed dependency changes. Add settings.windowsTerminalShell to the dependency array.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/settings/SettingsPanels.tsx around line 654:
`changedSettingLabels` reads `settings.windowsTerminalShell` but the `useMemo` dependency array omits it. When the user changes only the terminal-shell setting, the memo stays stale, so the restore-defaults control fails to include "Terminal shell" in the confirmation until another listed dependency changes. Add `settings.windowsTerminalShell` to the dependency array.
| const settings = usePrimarySettings(); | ||
| const updateSettings = useUpdatePrimarySettings(); | ||
| if ( |
There was a problem hiding this comment.
🟡 Mediumsettings/SettingsPanels.tsx:1887
WindowsTerminalShellRow gates visibility on navigator.platform (the client's browser OS), but the setting configures the connected server's terminal. A user on macOS/Linux browsing to a Windows server gets null and never sees the shell selector, and a user on Windows connected to a non-Windows server sees a control that doesn't apply. Visibility should be driven by the server platform, not the browser platform.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/settings/SettingsPanels.tsx around line 1887:
`WindowsTerminalShellRow` gates visibility on `navigator.platform` (the client's browser OS), but the setting configures the connected server's terminal. A user on macOS/Linux browsing to a Windows server gets `null` and never sees the shell selector, and a user on Windows connected to a non-Windows server sees a control that doesn't apply. Visibility should be driven by the server platform, not the browser platform.
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces a new terminal shell selection feature for Windows, which is a new user-facing capability requiring human review. Additionally, unresolved findings indicate the feature is incomplete - the shell setting is persisted in the UI but is not wired up to actually affect terminal spawning. You can customize Macroscope's approvability policy. Learn more. |
t3dotgg
commented
Aug 28, 2026
Note 🤖 GPT-5.6 Sol responding on behalf of Theo We're closing this PR as we clean up the T3 Code backlog. Thank you for taking the time to put this together. #5268 is the retained shell setting implementation and limits choices to shells reported by the connected host. This Windows-only branch adds a much larger shell parser without focused tests, so it has no unique behavior to keep. If you believe we closed this in error, please reopen the PR and leave a comment explaining what we missed. |
Fixes issue #123. Adds a setting to select terminal shell on Windows.
Note
Medium Risk
Changes how every terminal session picks its shell at spawn time; wrong resolution or incomplete settings wiring could break terminal startup on Windows.
Overview
Prepares terminal shell selection on Windows by resolving the preferred shell asynchronously when a PTY session starts, instead of reading it synchronously at manager construction time.
shellResolveris now anEffect(tests can still injectEffect.succeed(...)).resolveShellCandidatestakes a resolvedpreferredShellstring;startSessionrunsyield* shellResolveron each spawn so a user-configured shell can be read from settings at launch. Existing Windows fallback order (custom choice →pwsh.exe→ Windows PowerShell →ComSpec/cmd) is unchanged.The diff also adds a
ServerSettingsServiceimport (not wired in this hunk) and is largely formatting acrossManager.ts.Reviewed by Cursor Bugbot for commit 8150931. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add terminal shell selector for Windows in General settings
WindowsTerminalShellRowcomponent to the General settings panel, visible only on Windows, letting users pick their preferred terminal shell from a predefined list.windowsTerminalShellinServerSettings(defaults to empty string) and supports reset to default.TerminalManagerOptions.shellResolvertype changes from a synchronous function to anEffect.Effect<string>, evaluated at session start to resolve the preferred shell.stopProcessno longer immediately marks a session as exited; cleanup is now deferred until the process exit event is observed viadrainProcessEvents.📊 Macroscope summarized 8150931. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.