fix: shell selection not applying correct shell#60
Merged
Conversation
When users select "PowerShell 7" from the shell dropdown, the terminal
was spawning with Windows PowerShell 5 instead. This happened because
the frontend passed shell.name ("pwsh") to the spawn function, but the
backend's resolve_shell_path() couldn't find pwsh.exe in PATH and fell
back to powershell.exe.
By passing shell.path (the already-detected absolute path), we bypass
PATH resolution entirely and use the correct shell executable.
Fixes #59
Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFrontend now passes and stores shell executable paths ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant ShellAPI as shellApi.getAvailableShells()
participant Hook as use-terminal-restore
participant TerminalAPI as terminalApi.spawn
participant Tauri as Tauri backend
participant Pty as PtyManager.resolve_shell_path
UI->>Hook: user requests new terminal or selects shell (`shell.path`)
alt persisted or selected shell is a name/ambiguous
Hook->>ShellAPI: getAvailableShells()
ShellAPI-->>Hook: list of ShellInfo {name, path, displayName}
Hook->>Hook: resolveShellToPath -> pick matching `path`
end
Hook->>TerminalAPI: spawn(normalizedShellPath, pane)
TerminalAPI->>Tauri: invoke spawn with shell path
Tauri->>Pty: resolve_shell_path(shellPath) (explicit-path prioritized)
Pty-->>Tauri: resolved executable path
Tauri-->>TerminalAPI: spawn result
TerminalAPI-->>UI: terminal created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
gnoviawan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/layouts/WorkspaceLayout.tsx (1)
763-765: LGTM! This correctly passes the shell executable path instead of the display name.The fix aligns with how
resolve_shell_path()in the backend works—it validates paths containing separators directly, ensuring the exact detected shell spawns rather than relying on PATH resolution.Optional: The
handleCreateTerminalInPaneparameter is namedshellName(line 527) but now receives a path. Consider renaming toshellPathfor clarity.,
♻️ Optional: Rename parameter for clarity
const handleCreateTerminalInPane = useCallback( - async (paneId: string, shellName?: string) => { + async (paneId: string, shellPath?: string) => { if (terminals.length >= maxTerminals) { toast.error(`Maximum ${maxTerminals} terminals per project`) return } - const shell = shellName || activeProject?.defaultShell || appDefaultShell || undefined + const shell = shellPath || activeProject?.defaultShell || appDefaultShell || undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/layouts/WorkspaceLayout.tsx` around lines 763 - 765, The call to handleCreateTerminalInPane is now correctly passing a shell path, but the handler's parameter is still named shellName; rename the parameter in the handleCreateTerminalInPane function (and its usages) from shellName to shellPath to reflect it accepts an executable path rather than a display name, update any related JSDoc/Type annotations and tests referencing shellName, and ensure the function signature and all callers (including the onNewTerminalWithShell lambda) remain consistent after the rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/layouts/WorkspaceLayout.tsx`:
- Around line 763-765: The call to handleCreateTerminalInPane is now correctly
passing a shell path, but the handler's parameter is still named shellName;
rename the parameter in the handleCreateTerminalInPane function (and its usages)
from shellName to shellPath to reflect it accepts an executable path rather than
a display name, update any related JSDoc/Type annotations and tests referencing
shellName, and ensure the function signature and all callers (including the
onNewTerminalWithShell lambda) remain consistent after the rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b51690e-f63b-4507-a352-d4738ec0de0e
📒 Files selected for processing (1)
src/renderer/layouts/WorkspaceLayout.tsx
The defaultShell setting was storing shell.name (e.g., "pwsh") instead of shell.path (e.g., "C:\Program Files\PowerShell\7\pwsh.exe"). This caused the backend to fail PATH resolution and fall back to the wrong shell (Windows PowerShell 5 instead of PowerShell 7). Changes: - AppPreferences.tsx: Use shell.path as option value - ProjectSidebar.tsx: Use shell.path for project default shell - Display shell.displayName for cleaner UI Fixes #59 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/pages/AppPreferences.tsx`:
- Around line 294-300: The dropdown uses shell.path as option values which
breaks selection display for users with older saved settings that are plain
shell names; update the binding in AppPreferences (the select that maps
availableShells?.available) so the select's current value compares a normalized
shell identifier (extract basename from the stored setting or from shell.path)
instead of raw absolute paths — e.g., derive and use the shell name (basename of
path) for matching while still passing the absolute path to terminal spawn, and
ensure compatibility with resolve_shell_path by accepting either the stored name
or full path when determining the displayed selected option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbcb94f5-3e0c-4bec-922e-260e3d96ea72
📒 Files selected for processing (2)
src/renderer/components/ProjectSidebar.tsxsrc/renderer/pages/AppPreferences.tsx
Added resolveShellToPath helper function that: - Returns shell as-is if it's already a path (contains \ or /) - Looks up the path from available shells if it's just a name - Falls back to the original value if lookup fails This ensures backward compatibility for users who have shell names stored in their settings from before the fix. Now both old (name-only) and new (full path) settings work correctly. Applied to: - createDefaultTerminal: resolves shell before spawning - restoreFromLayout: resolves shell when restoring persisted terminals Fixes #59 Co-Authored-By: Claude <noreply@anthropic.com>
…ution The shell detection and resolution were checking PATH entries before explicit paths, causing the wrong PowerShell version to be found. Changes: - lib.rs: Check PowerShell 7 explicit paths BEFORE PATH entries in get_available_shells() - manager.rs: Check explicit paths BEFORE PATH entries in resolve_shell_path() This ensures PowerShell 7 (C:\Program Files\PowerShell\7\pwsh.exe) is found and used when installed, rather than falling back to Windows PowerShell 5 (powershell.exe) from PATH. Fixes #59 Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/lib.rs`:
- Around line 215-217: The fallback entry for PowerShell 7 is being treated as
an always-available built-in; change is_shell_available() so "pwsh" is not
considered a built-in shell and must be resolved from PATH instead of returning
true unconditionally. Locate the shell tuples (the ("pwsh", "pwsh.exe", None)
entry) and adjust the logic in is_shell_available() to skip auto-availability
for the "pwsh" name (or treat its tuple as a PATH-only entry), ensuring that
ShellInfo for "pwsh" is only emitted when the executable is actually
discoverable on PATH.
In `@src-tauri/src/pty/manager.rs`:
- Around line 997-1007: In resolve_shell_path, avoid letting the generic
*.exe/direct lookup short-circuit PowerShell handling: detect the requested
shell value (shell == "pwsh" or shell == "powershell") and run the
PowerShell-specific resolution using the explicit paths vec (the list with
r"C:\Program Files\PowerShell\7\pwsh.exe", r"C:\Program
Files\PowerShell\6\pwsh.exe",
r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", "pwsh.exe",
"powershell.exe") before the generic lookup so name-only tokens like "pwsh" or
"powershell" hit these explicit paths first and so the branch chosen preserves
the intended mapping rather than returning the raw token from the generic
resolver.
In `@src/renderer/hooks/use-terminal-restore.ts`:
- Around line 146-172: resolveShellToPath currently only treats inputs with path
separators as paths and only matches shells by available[*].name, so legacy
executable names like "pwsh.exe" won't be resolved; update the lookup to also
compare the executable basename of each available shell path to the input (e.g.,
path.basename(s.path) === shell) and treat that as a match. In practice, inside
resolveShellToPath (and the block that calls shellApi.getAvailableShells()),
after obtaining result.data.available, find a match by checking s.name === shell
OR the basename of s.path equals shell; return match.path when found. Ensure you
import/use the appropriate path basename helper for the renderer environment
when implementing this comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f61a722c-76d1-4e56-9d51-0752b8c57bbb
📒 Files selected for processing (3)
src-tauri/src/lib.rssrc-tauri/src/pty/manager.rssrc/renderer/hooks/use-terminal-restore.ts
The -NoProfile flag was preventing PowerShell from loading the user's profile, which contains custom prompts, aliases, and other settings. Changes: - Removed -NoProfile from PowerShell spawn arguments - Kept -NoLogo to hide the copyright banner Users will now see their custom PowerShell prompts (e.g., Oh My Posh, Starship, or custom profile configurations) when terminals spawn. Fixes #59 Co-Authored-By: Claude <noreply@anthropic.com>
1. lib.rs: Removed pwsh/pwsh.exe from is_builtin_windows_shell() - pwsh is NOT a built-in shell and must be resolved from PATH - Only cmd, powershell, and wsl are true Windows built-ins 2. manager.rs: Moved PowerShell handling BEFORE generic *.exe lookup - Ensures name-only tokens like "pwsh" hit explicit paths first - Prevents generic resolver from short-circuiting PowerShell handling 3. use-terminal-restore.ts: Added basename matching in resolveShellToPath() - Now matches by s.name OR basename of s.path - Allows "pwsh.exe" to match shell with path ending in "pwsh.exe" Fixes #59 Co-Authored-By: Claude <noreply@anthropic.com>
gnoviawan
changed the title
The dropdowns used shell.path as option values, but older saved settings may have plain shell names stored. This broke the selection display because the stored name didn't match the option path. Changes: - AppPreferences.tsx: Normalize stored defaultShell for display by finding matching shell (by name or basename) and using its path - ProjectSidebar.tsx: Update isSelected check to match by path, name, or basename for legacy compatibility This ensures users with older name-based settings see the correct selected shell in the UI while new full-path settings work as before. Fixes #59 Co-Authored-By: Claude <noreply@anthropic.com>
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 `@src-tauri/src/pty/manager.rs`:
- Around line 969-987: The resolve_shell_path implementation treats "powershell"
and "pwsh" the same, causing the "powershell" token to prefer pwsh paths; update
resolve_shell_path so it matches the mapping in get_available_shells: for the
"pwsh" token check PowerShell 7 then 6 then "pwsh.exe", and for the "powershell"
token check the Windows PowerShell v1.0 path
(C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe) then fallback to
"powershell.exe", using the existing self.get_absolute_shell_path lookup logic
and returning the first found path.
In `@src/renderer/layouts/WorkspaceLayout.tsx`:
- Around line 490-491: The useEffect references handleCreateTerminalInPane
before it's declared, causing a TDZ/build error; fix by moving the
handleCreateTerminalInPane callback (and its dependent handleNewTerminal) above
the useEffect that lists it in the dependency array, or alternatively remove
handleCreateTerminalInPane from that useEffect's deps and switch to a ref-based
pattern to call the latest callback. Ensure you update the declarations for
handleCreateTerminalInPane and handleNewTerminal (the useCallback block) to
appear before the useEffect that currently depends on them so the symbols are
defined when the hook runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48df6875-b133-4b1a-8142-60ba4181703c
📒 Files selected for processing (6)
src-tauri/src/lib.rssrc-tauri/src/pty/manager.rssrc/renderer/components/ProjectSidebar.tsxsrc/renderer/hooks/use-terminal-restore.tssrc/renderer/layouts/WorkspaceLayout.tsxsrc/renderer/pages/AppPreferences.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/hooks/use-terminal-restore.ts
- src/renderer/pages/AppPreferences.tsx
- src-tauri/src/lib.rs
- use-terminal-restore.ts: Capture projectId at effect run time and add eslint-disable comment for intentional ref access in cleanup - WorkspaceLayout.tsx: Note that handleCreateTerminalInPane cannot be in deps array as it's defined after this useEffect Fixes CI lint failures Co-Authored-By: Claude <noreply@anthropic.com>
gnoviawan
force-pushed
the
fix/shell-selection-resolution-59
branch
from
17d8487 to
bbdc77d
Compare
- manager.rs: Separate "pwsh" and "powershell" resolution paths
- "pwsh" now checks PowerShell 7/6 paths then pwsh.exe
- "powershell" now checks Windows PowerShell 5 path then powershell.exe
- Previously both tokens used the same mixed path list
- WorkspaceLayout.tsx: Fix temporal dead zone (TDZ) error
- Moved handleCreateTerminalInPane and handleNewTerminal callbacks
above the keyboard shortcuts useEffect
- Added handleCreateTerminalInPane to useEffect dependency array
- Fixes TypeScript "used before declaration" error
Fixes #59
Co-Authored-By: Claude <noreply@anthropic.com>
The app now stores full shell paths instead of shell names for defaultShell. Update the ProjectSidebar submenu test to expect the full path value passed to onUpdateProject. This aligns the test with the current implementation and fixes the failing CI test job. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shell.pathinstead ofshell.namewhen spawning a terminalProblem
When users selected "PowerShell 7" from the shell dropdown, the terminal spawned with Windows PowerShell 5 instead. This happened because the frontend passed
shell.name("pwsh") to the spawn function, but the backend'sresolve_shell_path()couldn't findpwsh.exein PATH and fell back topowershell.exe.Solution
Pass the full
shell.path(the already-detected absolute path likeC:\Program Files\PowerShell\7\pwsh.exe) instead ofshell.name. This bypasses PATH resolution entirely and uses the correct shell executable.Test Plan
$PSVersionTable.PSVersionshows 7.xFixes #59
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
User Interface
Improvements