From 731db7357da0b8646827120ce986af1b2a1bedfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sava=20Dimitrijevi=C4=87?= Date: Sun, 9 Aug 2026 15:58:30 +0200 Subject: [PATCH] editor: Honor lsp_results_location in the cmd-click references fallback (#61187) Follow-up to #59838, implementing what was discussed at the end of #59829: cmd-click navigation now respects `lsp_results_location` when go-to-definition falls back to find-all-references (invited in https://github.com/zed-industries/zed/issues/59829#issuecomment-4989966493: "It would! Feel free to hook that up if you'd like to!"). ## Problem Cmd-clicking a symbol's definition falls back to find-all-references, but the results always open in a multibuffer even with `"lsp_results_location": "picker"`. The hover-link click path calls the editor navigation methods directly, so the action handlers registered by `lsp_locations` never get a chance to intercept. ## Solution - `handle_click_hovered_link`'s fallback now dispatches the `FindAllReferences` action (with `open_results_in: None`, deferring to the global setting) instead of calling the method, so the `lsp_locations` handler can intercept it, or propagate to the editor's built-in handler when the setting is `multi_buffer`, preserving today's behavior exactly. - The plain cmd-click arm of `cmd_click_reveal_task` now runs the definition query via `go_to_definition_of_kind` (no internal references fallback) instead of `go_to_definition`, so the click path has a single fallback decision point: the dispatching one. Without this, the method's baked-in fallback opened a multibuffer before the dispatch could run. - `go_to_definition_of_kind` visibility widened to `pub(crate)` for the call from `hover_links.rs`. Shift/alt click variants (type definition, splits) are untouched. Keyboard invocations were already intercepted and are unchanged. ## Testing - New test `test_cmd_click_fallback_honors_lsp_results_location` in `lsp_locations`, following the module's existing test patterns: fake LSP returning no definition and two references, `lsp_results_location: picker`, simulated cmd-click at the cursor's pixel position, asserts the picker opens. The test fails without this change. - `cargo nextest run -p lsp_locations`: 6/6. - `cargo nextest run -p editor -E 'test(hover) or test(fallback) or test(go_to_definition) or test(references)'`: 54/54. - `cargo fmt` and `./script/clippy` clean. - Verified manually in a release build: with the setting on, cmd-clicking a definition opens the picker; with it off, behavior is unchanged. Per the contributing guidelines' note on AI assistance: this change was developed with heavy AI assistance (Claude Code). I have reviewed and understand the full diff and the reasoning behind each hunk, and I'm the one answering review feedback. Release Notes: - Fixed cmd-click go-to-definition falling back to a references multibuffer even when `lsp_results_location` is set to `picker`. --- crates/editor/src/hover_links.rs | 43 +++++++++++------------ crates/editor/src/navigation.rs | 2 +- crates/lsp_locations/src/lsp_locations.rs | 43 +++++++++++++++++++++++ 3 files changed, 64 insertions(+), 24 deletions(-) diff --git a/crates/editor/src/hover_links.rs b/crates/editor/src/hover_links.rs index fa530f4434..00d02a11bd 100644 --- a/crates/editor/src/hover_links.rs +++ b/crates/editor/src/hover_links.rs @@ -1,11 +1,10 @@ use crate::{ - Anchor, Editor, EditorSettings, EditorSnapshot, FindAllReferences, GoToDefinition, - GoToDefinitionSplit, GoToTypeDefinition, GoToTypeDefinitionSplit, GotoDefinitionKind, - HighlightKey, Navigated, PointForPosition, SelectPhase, - editor_settings::GoToDefinitionFallback, scroll::ScrollAmount, + Anchor, Editor, EditorSettings, EditorSnapshot, FindAllReferences, GoToDefinitionSplit, + GoToTypeDefinition, GoToTypeDefinitionSplit, GotoDefinitionKind, HighlightKey, Navigated, + PointForPosition, SelectPhase, editor_settings::GoToDefinitionFallback, scroll::ScrollAmount, }; use gpui::{ - App, AsyncWindowContext, Context, Entity, HighlightStyle, Modifiers, Pixels, Task, + App, AsyncWindowContext, Context, Entity, Focusable, HighlightStyle, Modifiers, Pixels, Task, UnderlineStyle, Window, px, }; use language::{Bias, ToOffset}; @@ -207,26 +206,22 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { + let focus_handle = self.focus_handle(cx); let reveal_task = self.cmd_click_reveal_task(point, modifiers, window, cx); - cx.spawn_in(window, async move |editor, cx| { + cx.spawn_in(window, async move |_, cx| { let definition_revealed = reveal_task.await.log_err().unwrap_or(Navigated::No); - let find_references = editor - .update_in(cx, |editor, window, cx| { - if definition_revealed == Navigated::Yes { - return None; - } - match EditorSettings::get_global(cx).go_to_definition_fallback { - GoToDefinitionFallback::None => None, - GoToDefinitionFallback::FindAllReferences => { - editor.find_all_references(&FindAllReferences::default(), window, cx) - } - } - }) - .ok() - .flatten(); - if let Some(find_references) = find_references { - find_references.await.log_err(); + if definition_revealed == Navigated::Yes { + return; } + cx.update(|window, cx| { + match EditorSettings::get_global(cx).go_to_definition_fallback { + GoToDefinitionFallback::None => {} + GoToDefinitionFallback::FindAllReferences => { + focus_handle.dispatch_action(&FindAllReferences::default(), window, cx); + } + } + }) + .ok(); }) .detach(); } @@ -328,7 +323,9 @@ impl Editor { } (true, false) => self.go_to_type_definition(&GoToTypeDefinition, window, cx), (false, true) => self.go_to_definition_split(&GoToDefinitionSplit, window, cx), - (false, false) => self.go_to_definition(&GoToDefinition::default(), window, cx), + (false, false) => { + self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, false, window, cx) + } } } else { Task::ready(Ok(Navigated::No)) diff --git a/crates/editor/src/navigation.rs b/crates/editor/src/navigation.rs index fef44102f6..737227777c 100644 --- a/crates/editor/src/navigation.rs +++ b/crates/editor/src/navigation.rs @@ -2372,7 +2372,7 @@ impl Editor { }) } - fn go_to_definition_of_kind( + pub(crate) fn go_to_definition_of_kind( &mut self, kind: GotoDefinitionKind, split: bool, diff --git a/crates/lsp_locations/src/lsp_locations.rs b/crates/lsp_locations/src/lsp_locations.rs index 90f29ae7bc..807856eb81 100644 --- a/crates/lsp_locations/src/lsp_locations.rs +++ b/crates/lsp_locations/src/lsp_locations.rs @@ -778,6 +778,7 @@ mod tests { use editor::test::editor_lsp_test_context::EditorLspTestContext; use gpui::TestAppContext; use indoc::indoc; + use workspace::Item as _; async fn rust_cx( capabilities: lsp::ServerCapabilities, @@ -969,4 +970,46 @@ mod tests { assert_eq!(matches(&mut cx, "zzzz"), 0); assert_eq!(matches(&mut cx, ""), 2); } + + #[gpui::test] + async fn test_cmd_click_fallback_honors_lsp_results_location(cx: &mut TestAppContext) { + cx.update(crate::init); + let mut cx = rust_cx( + lsp::ServerCapabilities { + definition_provider: Some(lsp::OneOf::Left(true)), + references_provider: Some(lsp::OneOf::Left(true)), + ..Default::default() + }, + cx, + ) + .await; + cx.update(|_window, cx| { + cx.update_global::(|settings, cx| { + settings.update_user_settings(cx, |settings| { + settings.editor.lsp_results_location = Some(OpenResultsIn::Picker); + }); + }); + }); + cx.set_state(SOURCE); + cx.lsp + .set_request_handler::(async move |_params, _| { + Ok(None) + }); + cx.lsp + .set_request_handler::(async move |params, _| { + let uri = params.text_document_position.text_document.uri; + Ok(Some(references(uri, &[(1, 8, 11), (2, 14, 17)]))) + }); + + let screen_coord = cx + .editor(|editor, _, cx| editor.pixel_position_of_cursor(cx)) + .unwrap(); + cx.simulate_click(screen_coord, gpui::Modifiers::secondary_key()); + cx.run_until_parked(); + + assert!( + active_picker(&mut cx).is_some(), + "the cmd-click go-to-definition fallback should open the references picker when lsp_results_location is picker" + ); + } }