-
Notifications
You must be signed in to change notification settings - Fork 37.4k
fix remote terminal suggest #286407
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
fix remote terminal suggest #286407
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.
Pull request overview
This PR fixes issue #286155 by ensuring that remote file system authority (such as WSL connections) is preserved when resolving file paths in terminal completions. The fix replaces URI.file() calls with cwd.with({ path: ... }) to maintain the scheme and authority from the current working directory URI.
Key Changes:
- Modified path resolution to preserve remote URI scheme and authority instead of always creating
file://URIs - Added test suite for remote file completion scenarios covering absolute paths, tilde expansion, and relative paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionService.ts |
Updated tilde expansion, absolute path, and CDPATH resolution to preserve remote authority by using cwd.with({ path: ... }) instead of URI.file() |
src/vs/workbench/contrib/terminalContrib/suggest/test/browser/terminalCompletionService.test.ts |
Added remote file completion test suite with tests for absolute paths, tilde expansion, and relative paths in WSL context |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/terminalContrib/suggest/test/browser/terminalCompletionService.test.ts:852
- The test assertion is too weak to verify the fix. While it checks that completions are returned, it doesn't verify that the remote authority is actually preserved in the returned completions. The test should assert that the completion items or their underlying resources have the correct scheme ('vscode-remote') and authority ('wsl+Ubuntu'). Without this verification, the test could pass even if the bug still exists.
// Check that results exist for remote tilde path
assert.ok(result && result.length > 0, 'Should return completions for remote tilde path');
});
fix part of #286155
fix #249896
File completions weren't working in WSL (and other remote environments) because when creating URIs for absolute paths, tilde paths (~), and $CDPATH entries, the code was using URI.file() which always creates a file:// URI pointing to the local host filesystem.
For example, when typing /home/user in a WSL terminal:
Before:
URI.file('/home/user')→file:///home/user→ queries local Windows hostAfter:
createUriFromLocalPath(cwd, '/home/user')→vscode-remote://wsl+Ubuntu/home/user→ queries remote WSL filesystemAdded a helper function
createUriFromLocalPaththat:For local file:// URIs: Uses
URI.file(path)to handle Windows path normalizationFor remote URIs: Uses
cwd.with({ path })to preserve the scheme and authority