From 45d200f2f8b86605c5137bf49441c74b5288cc51 Mon Sep 17 00:00:00 2001
From: Kirill Bulatov <kirill@zed.dev>
Date: Wed, 9 Jul 2025 14:44:29 +0300
Subject: [PATCH] Wrap back around in context menu properly (#34112)

When navigating back in the context menu, it was not possible to get
past first element, if it was not selectable.
The other way around works, hence the fix.

Release Notes:

- N/A
---
 crates/language_tools/src/lsp_tool.rs    |  2 -
 crates/ui/Cargo.toml                     |  3 +
 crates/ui/src/components/context_menu.rs | 89 +++++++++++++++++++++---
 3 files changed, 81 insertions(+), 13 deletions(-)

diff --git a/crates/language_tools/src/lsp_tool.rs b/crates/language_tools/src/lsp_tool.rs
index 81cc38d33f6c7f19b304a4b52043329d448fd45a..9e542f582a994aeba8920ab3563899df8ed5bb94 100644
--- a/crates/language_tools/src/lsp_tool.rs
+++ b/crates/language_tools/src/lsp_tool.rs
@@ -735,8 +735,6 @@ impl LspTool {
                             state.update(cx, |state, cx| state.fill_menu(menu, cx))
                         });
                         lsp_tool.lsp_menu = Some(menu.clone());
-                        // TODO kb will this work?
-                        // what about the selections?
                         lsp_tool.popover_menu_handle.refresh_menu(
                             window,
                             cx,
diff --git a/crates/ui/Cargo.toml b/crates/ui/Cargo.toml
index 625bdc62f5e899912929539e89c5357ea4e7e8f6..c0472917721eaca37214cc21505c94fab54d21cd 100644
--- a/crates/ui/Cargo.toml
+++ b/crates/ui/Cargo.toml
@@ -34,6 +34,9 @@ workspace-hack.workspace = true
 [target.'cfg(windows)'.dependencies]
 windows.workspace = true
 
+[dev-dependencies]
+gpui = { workspace = true, features = ["test-support"] }
+
 [features]
 default = []
 stories = ["dep:story"]
diff --git a/crates/ui/src/components/context_menu.rs b/crates/ui/src/components/context_menu.rs
index 075cf7a7d7a881fc308b0d2a7dcee3c9bdabcd57..873b7b9e63c87decfc321bd104170813587ba4c9 100644
--- a/crates/ui/src/components/context_menu.rs
+++ b/crates/ui/src/components/context_menu.rs
@@ -690,20 +690,15 @@ impl ContextMenu {
         cx: &mut Context<Self>,
     ) {
         if let Some(ix) = self.selected_index {
-            if ix == 0 {
-                self.handle_select_last(&SelectLast, window, cx);
-            } else {
-                for (ix, item) in self.items.iter().enumerate().take(ix).rev() {
-                    if item.is_selectable() {
-                        self.select_index(ix, window, cx);
-                        cx.notify();
-                        break;
-                    }
+            for (ix, item) in self.items.iter().enumerate().take(ix).rev() {
+                if item.is_selectable() {
+                    self.select_index(ix, window, cx);
+                    cx.notify();
+                    return;
                 }
             }
-        } else {
-            self.handle_select_last(&SelectLast, window, cx);
         }
+        self.handle_select_last(&SelectLast, window, cx);
     }
 
     fn select_index(
@@ -1171,3 +1166,75 @@ impl Render for ContextMenu {
             })))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use gpui::TestAppContext;
+
+    use super::*;
+
+    #[gpui::test]
+    fn can_navigate_back_over_headers(cx: &mut TestAppContext) {
+        let cx = cx.add_empty_window();
+        let context_menu = cx.update(|window, cx| {
+            ContextMenu::build(window, cx, |menu, _, _| {
+                menu.header("First header")
+                    .separator()
+                    .entry("First entry", None, |_, _| {})
+                    .separator()
+                    .separator()
+                    .entry("Last entry", None, |_, _| {})
+            })
+        });
+
+        context_menu.update_in(cx, |context_menu, window, cx| {
+            assert_eq!(
+                None, context_menu.selected_index,
+                "No selection is in the menu initially"
+            );
+
+            context_menu.select_first(&SelectFirst, window, cx);
+            assert_eq!(
+                Some(2),
+                context_menu.selected_index,
+                "Should select first selectable entry, skipping the header and the separator"
+            );
+
+            context_menu.select_next(&SelectNext, window, cx);
+            assert_eq!(
+                Some(5),
+                context_menu.selected_index,
+                "Should select next selectable entry, skipping 2 separators along the way"
+            );
+
+            context_menu.select_next(&SelectNext, window, cx);
+            assert_eq!(
+                Some(2),
+                context_menu.selected_index,
+                "Should wrap around to first selectable entry"
+            );
+        });
+
+        context_menu.update_in(cx, |context_menu, window, cx| {
+            assert_eq!(
+                Some(2),
+                context_menu.selected_index,
+                "Should start from the first selectable entry"
+            );
+
+            context_menu.select_previous(&SelectPrevious, window, cx);
+            assert_eq!(
+                Some(5),
+                context_menu.selected_index,
+                "Should wrap around to previous selectable entry (last)"
+            );
+
+            context_menu.select_previous(&SelectPrevious, window, cx);
+            assert_eq!(
+                Some(2),
+                context_menu.selected_index,
+                "Should go back to previous selectable entry (first)"
+            );
+        });
+    }
+}