Fix virtual path resolution in TerminalInstance for Live Share (#307642)#307652
Fix virtual path resolution in TerminalInstance for Live Share (#307642)#307652ajoyag wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve terminal CWD/path handling when the workspace uses Live Share virtual resources (vsls: URIs), so the terminal resolves and uses the correct virtual paths.
Changes:
- Update
getCwdto treatvsls:URIs specially by usingURI.pathinstead offsPath. - Adjust how workspace root paths are combined with relative custom CWD values.
- Large edits in
TerminalInstancearound constructor initialization and mouse/right-click handling (currently introducing regressions/compile issues).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/terminal/common/terminalEnvironment.ts |
Adds vsls:-aware CWD/root path resolution for Live Share. |
src/vs/workbench/contrib/terminal/browser/terminalInstance.ts |
Restructures constructor/workspace-folder initialization and changes mouse event handling (currently broken/incomplete). |
| const rootPath = root.scheme === 'vsls' ? root.path : root.fsPath; | ||
| cwd = path.join(rootPath, customCwd); |
There was a problem hiding this comment.
When root.scheme === 'vsls', rootPath is taken from URI.path (POSIX-style). Using path.join(rootPath, customCwd) is platform-dependent and will introduce backslashes on Windows clients, producing an invalid virtual path for Live Share. Use a POSIX join (path.posix.join) or URI.joinPath(root, customCwd).path when dealing with non-file schemes like vsls to preserve forward slashes.
| const rootPath = root.scheme === 'vsls' ? root.path : root.fsPath; | |
| cwd = path.join(rootPath, customCwd); | |
| cwd = root.scheme === 'vsls' | |
| ? URI.joinPath(root, customCwd).path | |
| : path.join(root.fsPath, customCwd); |
| case TerminalCapability.PromptTypeDetection: { | ||
| capabilityListeners.set(e.id, e.capability.onPromptTypeChanged(refreshInfo)); | ||
| break; | ||
| this._scopedInstantiationService = this._register(instantiationService.createChild(new ServiceCollection( |
There was a problem hiding this comment.
There is an extra } at the end of the if (this.shellLaunchConfig.cwd) block that appears to close the constructor early. As a result, the if (!this._workspaceFolder) { ... } and subsequent initialization statements end up at class scope, which will not compile. Move the workspace-folder fallback and the scoped context key/instantiation initialization back inside the constructor and fix the brace structure/indentation.
| if (rightClickBehavior === 'nothing') { | ||
| if (!event.shiftKey) { | ||
| return { cancelContextMenu: true }; | ||
| } | ||
| return; |
There was a problem hiding this comment.
handleMouseEvent's right-click handling is incomplete: it only special-cases rightClickBehavior === 'nothing' (and shift-click), but it no longer implements the documented behaviors for default, copyPaste, paste, and selectWord. This will break terminal right-click behavior and related context menu/paste flows. Restore the missing branches so the configured terminal.integrated.rightClickBehavior options continue to work as intended.
| if (rightClickBehavior === 'nothing') { | |
| if (!event.shiftKey) { | |
| return { cancelContextMenu: true }; | |
| } | |
| return; | |
| switch (rightClickBehavior) { | |
| case 'nothing': | |
| return { cancelContextMenu: true }; | |
| case 'copyPaste': | |
| if (this.hasSelection()) { | |
| this.copySelection(); | |
| } else { | |
| this.paste(); | |
| } | |
| return { cancelContextMenu: true }; | |
| case 'paste': | |
| this.paste(); | |
| return { cancelContextMenu: true }; | |
| case 'selectWord': | |
| this.xterm?.selectWordAtCursor(event); | |
| openContextMenu(dom.getActiveWindow(), event, this, contextMenu, this._contextMenuService); | |
| return; | |
| case 'default': | |
| default: | |
| openContextMenu(dom.getActiveWindow(), event, this, contextMenu, this._contextMenuService); | |
| return; |
|
@microsoft-github-policy-service agree |
|
thanks @ajoyag |
No description provided.