Fix handling of Go-to-Commands to always call editor.action.goToLocations#5896
Fix handling of Go-to-Commands to always call editor.action.goToLocations#5896
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5896 +/- ##
==========================================
+ Coverage 67.39% 67.44% +0.04%
==========================================
Files 168 168
Lines 12918 12925 +7
Branches 2559 2560 +1
==========================================
+ Hits 8706 8717 +11
+ Misses 3758 3754 -4
Partials 454 454 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
We've moved to VS Code's goToLocation command, which flashes the range, but leaves the selection at just the start.
... but looking at current editor first. We don't have a source document/position when the server calls `goToLocation` so it needs the original implementation. We just shouldn't be doing it in the case where we have a set of results, even if there is only one in there.
DanTup
force-pushed
the
fix-go-to-command
branch
from
325719c to
3138b88
Compare
We can't try to search for a range in a document if it might not be open yet, so we need to wait on the whole operation not just the equality check.
|
@codex review /gemini review |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, unifying the 'Go-to' commands to consistently use editor.action.goToLocations. This simplifies the code and fixes a bug with multi-group editor layouts by delegating editor management to VS Code. The refactoring of the base LspGoToCommand and its subclasses is clean, and the introduction of failureMessage makes the commands more robust. The test updates are also solid, using waitForResult to improve reliability.
I've added one suggestion for LspGoToLocationCommand to further simplify the code and align it with the PR's goal of avoiding manual editor navigation.
| protected async goToLocation(location: ls.Location): Promise<void> { | ||
| const codeLocation = this.analyzer.client.protocol2CodeConverter.asLocation(location); | ||
| const uri = codeLocation.uri; | ||
| const elementDocument = await vs.workspace.openTextDocument(uri); | ||
|
|
||
| let editor = vs.window.activeTextEditor?.document === elementDocument ? vs.window.activeTextEditor : undefined; | ||
| if (!editor) { | ||
| const sourceUriString = uriComparisonString(uri); | ||
| const existingTab = vs.window.tabGroups.all | ||
| .flatMap((group) => group.tabs) | ||
| .find((tab) => tab.input instanceof vs.TabInputText && uriComparisonString(tab.input.uri) === sourceUriString); | ||
| const tabGroup = existingTab?.group.viewColumn; | ||
|
|
||
| editor = await vs.window.showTextDocument(elementDocument, tabGroup); | ||
| } | ||
| showCode(editor, codeLocation.range, codeLocation.range, codeLocation.range); | ||
| } |
There was a problem hiding this comment.
This implementation of goToLocation re-introduces manual editor and tab group management, which the pull request description identifies as a source of bugs. While editor.action.goToLocations might not be suitable here due to the lack of a source position, the logic can be greatly simplified by using vs.window.showTextDocument with options. This would delegate editor handling to VS Code, aligning with the goal of this PR and making the code less prone to bugs related to multi-group editor layouts.
protected async goToLocation(location: ls.Location): Promise<void> {
const codeLocation = this.analyzer.client.protocol2CodeConverter.asLocation(location);
await vs.window.showTextDocument(codeLocation.uri, { selection: codeLocation.range });
}
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously when there was only a single result, we'd bypass VS Code and compute the target editor ourselves which led to #5894
This way, we always let VS Code determine the target editor.
Fixes #5894