-
Notifications
You must be signed in to change notification settings - Fork 37.3k
fix remote terminal completions #286850
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 completions #286850
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 an issue where terminal completions in remote scenarios (e.g., WSL) were incorrectly filtered out due to using the local platform's path separator instead of the remote extension host's path separator.
Key changes:
- Detects path separator from completion labels coming from the extension host
- Passes the detected path separator to
TerminalCompletionItemfor proper path normalization - Removes hardcoded reliance on local platform path separator (
isWindowsandsep)
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/terminalSuggestAddon.ts | Changes _pathSeparator from hardcoded sep to detected value from completion labels; passes path separator to TerminalCompletionItem constructor; adds null checks before using path separator |
| src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionItem.ts | Adds optional pathSeparator parameter to constructor; replaces isWindows check with explicit path separator comparison; defaults to Unix-style paths when separator is undefined |
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalCompletionItem.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/terminalContrib/suggest/browser/terminalSuggestAddon.ts
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Tyriar
left a comment
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.
Possible to write a test for this?
Debugging showed the completions were getting created correctly, but then were filtered out.
This was happening because we were using the local path sep instead of the one from the extension host. The fix is to pass
pathSepintoTerminalCompletionItem.fixes #286155