-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat(terminal): support selecting different profile #8487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some issues that need attention. Focused on aligning UI sentinel values and localization for the preferred terminal profile selector.
b348333 to
e50523f
Compare
|
Re-review completed. The latest commit only restores Korean translations lost during merge resolution. No new issues found. The 3 previously identified issues remain unresolved:
Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request. |
| // Keep our icon and name unless profile specifies otherwise | ||
| terminalOptions.name = "Roo Code" | ||
| terminalOptions.iconPath = iconPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] Comment claims conditional override but code unconditionally sets values. The comment states "Keep our icon and name unless profile specifies otherwise" but lines 44-45 always override any profile-specified name/icon from the Object.assign on line 36. Either remove the "unless" clause from the comment or use conditional assignment like terminalOptions.name = profileOptions.name ?? "Roo Code".
Fix it with Roo Code or mention @roomote and request a fix.
| shellArgs: ["--login"], | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P4] Missing test coverage for empty string equivalence. The code treats empty string and undefined as equivalent (line 150: if (preferredProfile) returns false for both "", null, and undefined) but no test verifies this critical sentinel behavior. Add a test case: getTerminalOptionsForRoo("") should return undefined (using VSCode default), not fall back to default profile.
Fix it with Roo Code or mention @roomote and request a fix.
| <SelectValue placeholder="Select profile..." /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="default">Default (System)</SelectItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P4] Label mismatch between frontend and backend. UI displays "Default (System)" but TerminalProfileService.getAllSelectableProfiles() returns "Default (VSCode Default)" (line 188). This creates inconsistency - either use the backend's label directly via availableProfiles.find(p => p.name === "")?.displayName or localize both to match.
Fix it with Roo Code or mention @roomote and request a fix.
|
Thanks for working on this terminal profile feature! After testing it out with the team, we found a few issues that make it tricky to use in practice. The main problem is that it shows all possible profiles even when they're not actually installed, and if you pick one that doesn't exist, commands just fail. Also since we switched to inline terminal as the default, it doesn't really work with that setup anymore. Ideally something like this would only show profiles that are actually available, gracefully fall back when a profile goes missing, and work well with both inline and regular terminals. The current approach has some rough edges that make it not quite ready. I'm going to close this for now, but definitely let me know if you want to take another shot at it with those things in mind. Happy to help brainstorm or review a different approach! |
Related GitHub Issue
Closes: #7892
Roo Code Task Context (Optional)
Description
This PR adds support for setting preferred profile for the terminal spawned by RooCode. The items are fetched from VSCode's setting so any user added profiles will be present.
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
@elianiva
Important
Adds support for selecting a preferred terminal profile in RooCode, integrating with VSCode settings and updating terminal creation logic.
terminalPreferredProfilesetting inglobal-settings.tsto select a preferred terminal profile.ClineProvider.tsandTerminal.tsto apply the selected profile when creating terminals.TerminalProfileServiceto manage terminal profiles, including fetching available profiles and configurations.SettingsView.tsxandTerminalSettings.tsxto include a dropdown for selecting terminal profiles.TerminalProfileService.spec.tsto test profile fetching and configuration logic.This description was created by
for b348333. You can customize this summary. It will automatically update as commits are pushed.