Repository navigation
fix(terminal): honor Settings custom shell when Terminal button clicked (#4085) - #4086
Conversation
…ed (#4085) Vault/host-tree Terminal buttons wired onClick={handler}, so React passed the MouseEvent as the shell arg and skipped resolveShellSetting(terminalSettings.localShell). Only treat objects with a string command as explicit shells; wrap onClick sites.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce3a4c06f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| style={{ color: theme.mutedFg }} | ||
| disabled={!canCreateLocalTerminal} | ||
| onClick={onCreateLocalTerminal} | ||
| onClick={() => onCreateLocalTerminal()} |
There was a problem hiding this comment.
The existing test host tree toolbar keeps every default primary action reachable at min width in components/terminalLayer/TerminalHostTreeToolbar.test.ts:52 still requires the literal onClick={onCreateLocalTerminal}. Wrapping both handlers makes that assertion fail, so the repository's npm test suite fails despite the intended behavior being correct. Reproduced by running the toolbar test directly; update the assertion to match the new callback or verify the click behavior instead.
Useful? React with 👍 / 👎.
…ar (#4085) Codex GH review: source assertion still expected onClick={onCreateLocalTerminal} after wrappers; update to () => onCreateLocalTerminal() so npm test passes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Merge gate
|
Merge gate
Ready to squash-merge. Fixes #4085. |
Electron smoke PASS (tip
|
Summary
Fixes Settings → Terminal custom absolute shell path being ignored when opening a local terminal from the host-page / host-tree Terminal button on Windows (and elsewhere). CLI
--open-terminal-shell=<path>already worked.Root cause
Vault/host-tree buttons used
onClick={handleCreateLocalTerminal}. React passes theMouseEventas the first argument.handleCreateLocalTerminalImpltreated any truthy first arg as an explicit shell, soresolveShellSetting(terminalSettings.localShell, …)never ran → sessionlocalShellstayed empty → bridge fell back to the OS default shell.Fix
commandas an explicit shell; otherwise resolve from Settings.() => onCreateLocalTerminal().Notes
Fixes #4085