Fix terminal default profile selection for workspaces with custom profiles #282054
Conversation
Co-authored-by: meganrogge <29464607+meganrogge@users.noreply.github.com>
Co-authored-by: meganrogge <29464607+meganrogge@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where "Select Default Profile" had no effect in workspaces with custom terminal settings. The issue was that the command only updated user settings, which were overridden by workspace settings.
Key Changes
- Target selection logic: When workspace and user default profiles differ, a second picker allows users to choose whether to update User, Workspace, or Both settings
- Workspace profiles section: Added a "workspace" separator in the profile picker to show workspace-defined profiles
- Scope-aware configuration: Profile updates now use
inspect()to read from and write to specific scopes (user vs workspace) instead of merging configurations
| // Add workspace profiles section if workspace has any profiles defined | ||
| const workspaceProfilesConfig = this._configurationService.inspect(profilesKey); | ||
| if (workspaceProfilesConfig.workspaceValue && typeof workspaceProfilesConfig.workspaceValue === 'object') { | ||
| const workspaceProfiles = Object.keys(workspaceProfilesConfig.workspaceValue as { [key: string]: unknown }); | ||
| if (workspaceProfiles.length > 0) { | ||
| quickPickItems.push({ type: 'separator', label: nls.localize('terminalProfiles.workspace', "workspace") }); | ||
| // Filter available profiles to only include workspace-defined ones | ||
| const workspaceProfileItems = profiles | ||
| .filter(e => workspaceProfiles.includes(e.profileName)) | ||
| .map(e => this._createProfileQuickPickItem(e)); | ||
| quickPickItems.push(...this._sortProfileQuickPickItems(workspaceProfileItems, defaultProfileName!)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The workspace profiles section (lines 203-215) lacks test coverage. Consider adding tests that verify:
- Workspace profiles are displayed in a separate section when workspace has profile definitions
- The section is not shown when workspace has no profile definitions
- Workspace profiles are correctly filtered and sorted
| for (const target of targets) { | ||
| if (hasKey(result.profile, { id: true })) { | ||
| // extension contributed profile | ||
| await this._configurationService.updateValue(defaultProfileKey, result.profile.title, target); | ||
| } else { | ||
| // Add the profile to settings if necessary | ||
| if (hasKey(result.profile, { profileName: true })) { | ||
| // Get the appropriate configuration based on target | ||
| const profilesInspect = this._configurationService.inspect(profilesKey); | ||
| let profilesConfig: { [key: string]: ITerminalProfileObject }; | ||
| if (target === ConfigurationTarget.WORKSPACE) { | ||
| profilesConfig = profilesInspect.workspaceValue || {}; | ||
| } else { | ||
| profilesConfig = profilesInspect.userValue || {}; | ||
| } | ||
|
|
||
| if (typeof profilesConfig === 'object') { | ||
| const newProfilesConfig = { ...profilesConfig }; | ||
| newProfilesConfig[result.profile.profileName] = this._createNewProfileConfig(result.profile); | ||
| await this._configurationService.updateValue(profilesKey, newProfilesConfig, target); | ||
| } | ||
| } | ||
| // Set the default profile | ||
| await this._configurationService.updateValue(defaultProfileKey, result.profileName, target); | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The configuration update loop (lines 60-85) contains nested conditional logic that handles two different code paths (extension profiles vs. regular profiles). Consider extracting this into a separate private method like _updateProfileConfiguration(target: ConfigurationTarget, result: IProfileQuickPickItem, profilesKey: string, defaultProfileKey: string) to improve readability and maintainability. This would make the main flow clearer and make the logic easier to test.
| // Add workspace profiles section if workspace has any profiles defined | ||
| const workspaceProfilesConfig = this._configurationService.inspect(profilesKey); | ||
| if (workspaceProfilesConfig.workspaceValue && typeof workspaceProfilesConfig.workspaceValue === 'object') { | ||
| const workspaceProfiles = Object.keys(workspaceProfilesConfig.workspaceValue as { [key: string]: unknown }); | ||
| if (workspaceProfiles.length > 0) { | ||
| quickPickItems.push({ type: 'separator', label: nls.localize('terminalProfiles.workspace', "workspace") }); | ||
| // Filter available profiles to only include workspace-defined ones | ||
| const workspaceProfileItems = profiles | ||
| .filter(e => workspaceProfiles.includes(e.profileName)) | ||
| .map(e => this._createProfileQuickPickItem(e)); | ||
| quickPickItems.push(...this._sortProfileQuickPickItems(workspaceProfileItems, defaultProfileName!)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The workspace profiles section may show duplicate profiles. This code filters profiles to only include workspace-defined ones (lines 210-212), but these profiles may already be shown in the "profiles" section above (lines 198-201). Since profiles includes all available profiles (both user and workspace), and workspace profiles are typically also in the merged profiles list, this will result in the same profiles appearing twice in the picker - once in the "profiles" section and once in the "workspace" section. Consider checking if a profile is already included before adding it to the workspace section, or filter the "profiles" section to exclude workspace-only profiles.
| // Check if workspace has a different default profile than user | ||
| const configInspect = this._configurationService.inspect(defaultProfileKey); | ||
| const userDefault = configInspect.userValue; | ||
| const workspaceDefault = configInspect.workspaceValue; | ||
|
|
||
| // If workspace and user defaults differ, ask where to apply the change | ||
| let targets: ConfigurationTarget[] = [ConfigurationTarget.USER]; | ||
| if (workspaceDefault !== undefined && workspaceDefault !== userDefault) { | ||
| const targetChoice = await this._showTargetPicker(); | ||
| if (!targetChoice) { | ||
| return; // User cancelled | ||
| } | ||
| targets = targetChoice; | ||
| } |
There was a problem hiding this comment.
The new workspace vs. user configuration target selection logic (lines 44-57) lacks test coverage. The existing tests in terminalProfileService.integrationTest.ts only test the basic setDefault functionality without workspace settings. Consider adding tests that verify:
- The target picker is shown when workspace and user defaults differ
- The target picker is not shown when they match or when workspace is undefined
- Configuration is correctly applied to the selected targets (USER, WORKSPACE, or both)
- User cancellation is handled correctly
| // Add workspace profiles section if workspace has any profiles defined | ||
| const workspaceProfilesConfig = this._configurationService.inspect(profilesKey); | ||
| if (workspaceProfilesConfig.workspaceValue && typeof workspaceProfilesConfig.workspaceValue === 'object') { | ||
| const workspaceProfiles = Object.keys(workspaceProfilesConfig.workspaceValue as { [key: string]: unknown }); | ||
| if (workspaceProfiles.length > 0) { | ||
| quickPickItems.push({ type: 'separator', label: nls.localize('terminalProfiles.workspace', "workspace") }); | ||
| // Filter available profiles to only include workspace-defined ones | ||
| const workspaceProfileItems = profiles | ||
| .filter(e => workspaceProfiles.includes(e.profileName)) | ||
| .map(e => this._createProfileQuickPickItem(e)); | ||
| quickPickItems.push(...this._sortProfileQuickPickItems(workspaceProfileItems, defaultProfileName!)); | ||
| } | ||
| } |
There was a problem hiding this comment.
@copilot this should do the inspect in detectAvailableProfiles in terminalProfiles.ts and then pass that info back from the profile via ITerminalProfile.configScope:
vscode/src/vs/platform/terminal/node/terminalProfiles.ts
Lines 29 to 67 in ff0ce2c
We want a single consolidated place that the config is touched, otherwise features that depend on this scope in the future will need to reimplement, refactor, etc. It also introduces the chance of mismatches; what if a user and workspace profile have the same name for example.
There was a problem hiding this comment.
Done. Added configScope property to ITerminalProfile and moved the config inspection logic to detectAvailableProfiles. The quickpick now uses this property to separate user and workspace profiles without re-inspecting configuration. (34e9571)
- Add configScope property to ITerminalProfile interface - Detect and set configScope in detectAvailableProfiles function - Update quickpick to use configScope instead of re-inspecting - Separate user and workspace profiles based on configScope - Addresses @Tyriar feedback about consolidated config inspection Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
Co-authored-by: Tyriar <2193314+Tyriar@users.noreply.github.com>
Description
"Select Default Profile" had no effect in workspaces with custom terminal settings because it only updated user settings, which workspace settings override.
Changes
Architectural Refactoring
Centralized Profile Scope Detection (addresses review feedback):
configScopeproperty toITerminalProfileinterface to track configuration origin (USER, WORKSPACE, or undefined for auto-detected profiles)detectAvailableProfilesinterminalProfiles.tsto inspect configuration once and setconfigScopefor each profileconfigScopeTarget Selection
When workspace and user default profiles differ, show a second picker to choose where to apply:
Implementation in
showAndGetResult():Workspace Profiles Section
Added "workspace" separator in profile picker showing workspace-defined profiles. Profiles are separated based on their
configScopeproperty:Scope-Aware Configuration
Profile updates now read from and write to the appropriate scope (user vs workspace) using
inspect()instead ofgetValue()to avoid merging configurations across scopes.Benefits
detectAvailableProfiles)configScopeconfigScopewithout reimplementing detection logicBackward Compatibility
Original prompt
This section details on the original issue you should resolve
<issue_title>Select Default Profile (terminal) doesn't work for projects with terminal specified</issue_title>
<issue_description>
Type: Bug
Open VScode
Open Terminal
See
zshis the default terminalClone https://github.com/valkey-io/valkey-search/
Open cloned repository in vscode
Open Terminal
See
bashis the default terminalUse the drop down next to the plus next to bash
Select
Select Default ProfileSelect
zsh -lClose and reopen vscode
Open Terminal
See
bashis still the default terminalThe problem is that the Select Default Profile setting doesn't change the project specific file and nothing else in the menu appears to let you:
https://github.com/valkey-io/valkey-search/blob/main/.vscode/settings.json#L3-L4
Preferably, the UI should edit the project settings, but if it doesn't, it should warn users that the setting is useless for the project and point to the file they can hand edit.
VS Code version: Code 1.100.3 (Universal) (258e40f, 2025-06-02T13:30:54.273Z)
OS version: Darwin arm64 24.5.0
Modes:
System Info
canvas_oop_rasterization: enabled_on
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: enabled_on
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_graphite: disabled_off
video_decode: enabled
video_encode: enabled
webgl: enabled
webgl2: enabled
webgpu: enabled
webnn: disabled_off
Extensions (23)
A/B Experiments
<agent_instructions>If the workspace has a defaultProfile different from the one defined for the user, add a second step to the picker that allows choosing whether the value will apply to the User, Workspace, both or neither.
Add a new section with title workspace that displays any terminal.integrated.profiles defined for the workspace</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@meganrogge I cannot repro, probably has been fixed, thus closing. @meganrogge Yep, tried with those lines. In fact, that's what I have had as my default shell for several months. @meganrogge Why would a specific repo be needed here? @meganrogge Oh i see- you're defining that s...💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.