Allow workspace folder selection option when creating external terminals #295073
Conversation
anthonykim1
changed the title
There was a problem hiding this comment.
Pull request overview
Adds workspace-folder selection support when opening an external terminal (native console) in multi-root workspaces, aligning behavior more closely with integrated terminal folder selection.
Changes:
- Prompt to pick a workspace folder (via
_workbench.pickWorkspaceFolder) when the workspace has multiple folders before launching the external terminal. - Add a new electron-browser test suite covering single-folder, multi-folder (pick/cancel), and fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/externalTerminal/electron-browser/externalTerminal.contribution.ts | Prompts for workspace folder selection in multi-root workspaces before opening the external terminal. |
| src/vs/workbench/contrib/externalTerminal/test/electron-browser/externalTerminal.contribution.test.ts | Adds tests validating folder picking and fallback cwd behavior for the external terminal command. |
| let root; | ||
| if (folders.length > 1) { | ||
| const workspace = await commandService.executeCommand<IWorkspaceFolder>(PICK_WORKSPACE_FOLDER_COMMAND_ID); | ||
| if (!workspace) { | ||
| return; | ||
| } | ||
| root = workspace.uri; | ||
| } else { | ||
| root = historyService.getLastActiveWorkspaceRoot(); | ||
| } |
There was a problem hiding this comment.
let root; gives root an implicit any type under noImplicitAny, which should fail compilation in this repo. Please give it an explicit type (eg URI | undefined) so the command handler stays type-safe.
| const folders = workspaceContextService.getWorkspace().folders; | ||
| let root; | ||
| if (folders.length > 1) { | ||
| const workspace = await commandService.executeCommand<IWorkspaceFolder>(PICK_WORKSPACE_FOLDER_COMMAND_ID); |
There was a problem hiding this comment.
The picker can return undefined when canceled, but the generic type argument is IWorkspaceFolder, which implies a value is always returned. Consider using executeCommand<IWorkspaceFolder | undefined>(...) so the subsequent cancellation check is reflected in types.
| const workspace = await commandService.executeCommand<IWorkspaceFolder>(PICK_WORKSPACE_FOLDER_COMMAND_ID); | |
| const workspace = await commandService.executeCommand<IWorkspaceFolder | undefined>(PICK_WORKSPACE_FOLDER_COMMAND_ID); |
| const folderUri = URI.file('/workspace/project'); | ||
| const folder = createWorkspaceFolder(folderUri, 'project', 0); | ||
|
|
||
| setupServices({ | ||
| folders: [folder], | ||
| lastActiveRoot: folderUri, | ||
| }); | ||
|
|
||
| const handler = CommandsRegistry.getCommand('workbench.action.terminal.openNativeConsole')!.handler; | ||
| await instantiationService.invokeFunction(handler); | ||
|
|
||
| assert.deepStrictEqual(openTerminalCalls, [{ cwd: '/workspace/project' }]); | ||
| assert.deepStrictEqual(executeCommandCalls, []); | ||
| }); |
There was a problem hiding this comment.
These assertions hard-code POSIX paths. URI.file(...).fsPath is platform-dependent (eg Windows will use backslashes / drive letters), so these tests are likely to fail on Windows CI. Build expected values from folderUri.fsPath / paths.dirname(fileUri.fsPath) instead of hard-coded strings.
| override executeCommand(id: string): Promise<any> { | ||
| executeCommandCalls.push(id); | ||
| if (id === PICK_WORKSPACE_FOLDER_COMMAND_ID) { | ||
| return Promise.resolve(options.pickedFolder); | ||
| } | ||
| return Promise.resolve(undefined); |
There was a problem hiding this comment.
The ICommandService.executeCommand mock is typed as Promise<any>. It can be typed more narrowly (eg Promise<IWorkspaceFolder | undefined>) to avoid any and keep the test aligned with the command contract.
| override executeCommand(id: string): Promise<any> { | |
| executeCommandCalls.push(id); | |
| if (id === PICK_WORKSPACE_FOLDER_COMMAND_ID) { | |
| return Promise.resolve(options.pickedFolder); | |
| } | |
| return Promise.resolve(undefined); | |
| override executeCommand<T = any>(id: string, ..._args: any[]): Promise<T> { | |
| executeCommandCalls.push(id); | |
| if (id === PICK_WORKSPACE_FOLDER_COMMAND_ID) { | |
| return Promise.resolve(options.pickedFolder) as Promise<T>; | |
| } | |
| return Promise.resolve(undefined as T); |
1 participant
Resolves: #279137
multi-root-terminal-working.mov