feat: Add terminal shell selection to settings#603
Conversation
Add the ability to select an available shell for the integrated terminal. This includes: - A new `getAvailableShells` IPC event to retrieve system shells. - A new `terminal` section in `SettingsData` and default settings. - Updates to localization files for the new terminal settings. - A new `TerminalSettings` component for the UI. - Integration into the main `Settings` page. - Updates to the `TerminalManager` to use the selected shell.
Summary of ChangesHello @jutaz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant user experience improvement by allowing users to customize their integrated terminal's shell. It establishes the necessary backend infrastructure to detect and manage available shells, integrates these options into the application's settings, and updates the terminal's launch logic to respect user preferences. This feature provides greater flexibility and control over the development environment within the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature allowing users to configure their preferred terminal shell within the application settings. The changes involve adding a ShellInfo type and a terminal setting to store the user's chosen shell path. A new API endpoint and IPC handler (getAvailableShells) were implemented to detect and provide a list of available shells on the system (Windows and Unix-like). The TerminalManager was updated to use the stored shell setting, and a new settings migration (v17-to-v18) was added to initialize the terminal settings. On the frontend, a new TerminalSettings component was created, integrated into the main Settings page, to display a dropdown of detected shells and a custom input field for the shell path. Review comments suggest removing the unused args property from ShellInfo and the associated getShellArgs method to reduce complexity, logging errors when reading /etc/shells instead of silently swallowing them, and improving the UI for shell selection to avoid confusion caused by separate dropdown and text input fields modifying the same setting.
| export interface ShellInfo { | ||
| path: string; | ||
| name: string; | ||
| args?: string[]; |
There was a problem hiding this comment.
The args property is populated in getAvailableShells but it's not used by the frontend or the TerminalManager. The TerminalManager has its own logic to determine shell arguments. To reduce complexity and remove dead code, this property should be removed. This would also allow for the removal of the getShellArgs method in src/main/utils/shell.ts.
| } catch { | ||
| // Ignore | ||
| } |
There was a problem hiding this comment.
Swallowing errors without logging can hide potential issues and make debugging harder. For instance, if reading /etc/shells fails due to permissions, it would be helpful to see this in the logs. Please consider logging the error.
| } catch { | |
| // Ignore | |
| } | |
| } catch (error) { | |
| logger.warn('Failed to read /etc/shells, continuing without it.', { error: error instanceof Error ? error.message : String(error) }); | |
| } |
Adds a more user-friendly interface for selecting the terminal shell, including auto-detect, preset list, and custom path options.
wladimiiir
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
I found some issues during the testing. Could you please address them? Thank you.
There was a problem hiding this comment.
Seems like there has been unwanted changes in the file.
There was a problem hiding this comment.
Also in here, lots of the unnecessary changes.
There was a problem hiding this comment.
yep, my bad. Will fix.
|
|
||
| // Also check /etc/shells | ||
| try { | ||
| if (fs.existsSync('/etc/shells')) { |
| name="shell-mode" | ||
| value="custom" | ||
| checked={mode === 'custom'} | ||
| onChange={() => handleModeChange('custom')} |
| const envShell = process.env.SHELL; | ||
| if (envShell && fs.existsSync(envShell)) { | ||
| const name = path.basename(envShell); | ||
| return { path: envShell, name, args: this.getShellArgs(name) }; |
There was a problem hiding this comment.
I don't think removing this is a good idea. Is this fixing some issue?


Add the ability to select an available shell for the integrated terminal. This includes:
getAvailableShellsIPC event to retrieve system shells.terminalsection inSettingsDataand default settings.TerminalSettingscomponent for the UI.Settingspage.TerminalManagerto use the selected shell.