From 95852d4c716348e0621b6d074d0a6694391cbb77 Mon Sep 17 00:00:00 2001 From: Ignacio <67933444+0xErwin1@users.noreply.github.com> Date: Tue, 24 Mar 2026 23:13:44 -0300 Subject: [PATCH] workspace: Deduplicate navigation history entries (#44504) Closes #43545 Remove existing entries for an item before adding new ones to the navigation history. This prevents Go Back (Ctrl-O) from bouncing between the same items repeatedly. Navigation pattern A->B->A->B->C now creates history [A,B,C] instead of [A,B,A,B,C], making backward navigation more efficient. Release Notes: - N/A --- .../disable_cursor_blinking/before.rs | 1 + crates/editor/src/editor.rs | 3 +- crates/workspace/src/item.rs | 2 +- crates/workspace/src/pane.rs | 37 +++++- crates/workspace/src/shared_screen.rs | 2 +- crates/workspace/src/workspace.rs | 122 ++++++++++++++++++ crates/zed/src/zed.rs | 65 ++-------- 7 files changed, 170 insertions(+), 62 deletions(-) diff --git a/crates/agent/src/edit_agent/evals/fixtures/disable_cursor_blinking/before.rs b/crates/agent/src/edit_agent/evals/fixtures/disable_cursor_blinking/before.rs index 607daa8ce3a129e0f4bc53a00d1a62f479da3932..2a199744feb33fd9d7002e100e324d3d3524aebb 100644 --- a/crates/agent/src/edit_agent/evals/fixtures/disable_cursor_blinking/before.rs +++ b/crates/agent/src/edit_agent/evals/fixtures/disable_cursor_blinking/before.rs @@ -11922,6 +11922,7 @@ impl Editor { scroll_anchor: scroll_state, scroll_top_row, }), + Some(cursor_position.row), cx, ); cx.emit(EditorEvent::PushedToNavHistory { diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 78ccd2c42cac7c805bcd03693fd6fc822bdaac16..9ab2549808124b4ce10fb7f711d2cf6ce293c279 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -15564,7 +15564,8 @@ impl Editor { } } - nav_history.push(Some(data), cx); + let cursor_row = data.cursor_position.row; + nav_history.push(Some(data), Some(cursor_row), cx); cx.emit(EditorEvent::PushedToNavHistory { anchor: cursor_anchor, is_deactivate, diff --git a/crates/workspace/src/item.rs b/crates/workspace/src/item.rs index 8e00753a86d67c819dae38613797ccbeff34edf9..ed104a534eba7707a04a60775ae08820c4f258b8 100644 --- a/crates/workspace/src/item.rs +++ b/crates/workspace/src/item.rs @@ -1583,7 +1583,7 @@ pub mod test { fn push_to_nav_history(&mut self, cx: &mut Context) { if let Some(history) = &mut self.nav_history { - history.push(Some(Box::new(self.state.clone())), cx); + history.push(Some(Box::new(self.state.clone())), None, cx); } } } diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index 54f83bbbe64309a8a00f74d68508a403bc7003f9..2d9f8be5c363b5b5c10c5432a543ae46de7611d2 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -473,6 +473,9 @@ pub struct NavigationEntry { pub data: Option>, pub timestamp: usize, pub is_preview: bool, + /// Row position for Neovim-style deduplication. When set, entries with the + /// same item and row are considered duplicates and deduplicated. + pub row: Option, } #[derive(Clone)] @@ -4510,7 +4513,12 @@ impl Render for Pane { } impl ItemNavHistory { - pub fn push(&mut self, data: Option, cx: &mut App) { + pub fn push( + &mut self, + data: Option, + row: Option, + cx: &mut App, + ) { if self .item .upgrade() @@ -4518,7 +4526,7 @@ impl ItemNavHistory { { let is_preview_item = self.history.0.lock().preview_item_id == Some(self.item.id()); self.history - .push(data, self.item.clone(), is_preview_item, cx); + .push(data, self.item.clone(), is_preview_item, row, cx); } } @@ -4526,9 +4534,10 @@ impl ItemNavHistory { let is_preview_item = self.history.0.lock().preview_item_id == Some(self.item.id()); NavigationEntry { item: self.item.clone(), - data: data, - timestamp: 0, // not used + data, + timestamp: 0, is_preview: is_preview_item, + row: None, } } @@ -4632,12 +4641,22 @@ impl NavHistory { data: Option, item: Arc, is_preview: bool, + row: Option, cx: &mut App, ) { let state = &mut *self.0.lock(); + let new_item_id = item.id(); + + let is_same_location = + |entry: &NavigationEntry| entry.item.id() == new_item_id && entry.row == row; + match state.mode { NavigationMode::Disabled => {} NavigationMode::Normal | NavigationMode::ReopeningClosedItem => { + state + .backward_stack + .retain(|entry| !is_same_location(entry)); + if state.backward_stack.len() >= MAX_NAVIGATION_HISTORY_LEN { state.backward_stack.pop_front(); } @@ -4646,10 +4665,13 @@ impl NavHistory { data: data.map(|data| Arc::new(data) as Arc), timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst), is_preview, + row, }); state.forward_stack.clear(); } NavigationMode::GoingBack => { + state.forward_stack.retain(|entry| !is_same_location(entry)); + if state.forward_stack.len() >= MAX_NAVIGATION_HISTORY_LEN { state.forward_stack.pop_front(); } @@ -4658,9 +4680,14 @@ impl NavHistory { data: data.map(|data| Arc::new(data) as Arc), timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst), is_preview, + row, }); } NavigationMode::GoingForward => { + state + .backward_stack + .retain(|entry| !is_same_location(entry)); + if state.backward_stack.len() >= MAX_NAVIGATION_HISTORY_LEN { state.backward_stack.pop_front(); } @@ -4669,6 +4696,7 @@ impl NavHistory { data: data.map(|data| Arc::new(data) as Arc), timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst), is_preview, + row, }); } NavigationMode::ClosingItem if is_preview => return, @@ -4681,6 +4709,7 @@ impl NavHistory { data: data.map(|data| Arc::new(data) as Arc), timestamp: state.next_timestamp.fetch_add(1, Ordering::SeqCst), is_preview, + row, }); } } diff --git a/crates/workspace/src/shared_screen.rs b/crates/workspace/src/shared_screen.rs index 136f552fee23231b45fcb867d2ce8bab02dca7e8..41e8f41f2ad4e10b85ceb68e5f4690b1faf6c04a 100644 --- a/crates/workspace/src/shared_screen.rs +++ b/crates/workspace/src/shared_screen.rs @@ -69,7 +69,7 @@ impl Item for SharedScreen { fn deactivated(&mut self, _window: &mut Window, cx: &mut Context) { if let Some(nav_history) = self.nav_history.as_mut() { - nav_history.push::<()>(None, cx); + nav_history.push::<()>(None, None, cx); } } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 3cab18c65575bae6a15a3b501cebd6f7a2ed2e39..a111517c21fa3bf401c8c94538253ed5e9a16528 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -11225,6 +11225,128 @@ mod tests { }); } + /// Tests that the navigation history deduplicates entries for the same item. + /// + /// When navigating back and forth between items (e.g., A -> B -> A -> B -> A -> B -> C), + /// the navigation history deduplicates by keeping only the most recent visit to each item, + /// resulting in [A, B, C] instead of [A, B, A, B, A, B, C]. This ensures that Go Back (Ctrl-O) + /// navigates through unique items efficiently: C -> B -> A, rather than bouncing between + /// repeated entries: C -> B -> A -> B -> A -> B -> A. + /// + /// This behavior prevents the navigation history from growing unnecessarily large and provides + /// a better user experience by eliminating redundant navigation steps when jumping between files. + #[gpui::test] + async fn test_navigation_history_deduplication(cx: &mut gpui::TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + let project = Project::test(fs, [], cx).await; + let (workspace, cx) = + cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx)); + + let item_a = cx.new(|cx| { + TestItem::new(cx).with_project_items(&[TestProjectItem::new(1, "a.txt", cx)]) + }); + let item_b = cx.new(|cx| { + TestItem::new(cx).with_project_items(&[TestProjectItem::new(2, "b.txt", cx)]) + }); + let item_c = cx.new(|cx| { + TestItem::new(cx).with_project_items(&[TestProjectItem::new(3, "c.txt", cx)]) + }); + + let pane = workspace.read_with(cx, |workspace, _| workspace.active_pane().clone()); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.add_item_to_active_pane(Box::new(item_a.clone()), None, true, window, cx); + workspace.add_item_to_active_pane(Box::new(item_b.clone()), None, true, window, cx); + workspace.add_item_to_active_pane(Box::new(item_c.clone()), None, true, window, cx); + }); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_a, false, false, window, cx); + }); + cx.run_until_parked(); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_b, false, false, window, cx); + }); + cx.run_until_parked(); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_a, false, false, window, cx); + }); + cx.run_until_parked(); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_b, false, false, window, cx); + }); + cx.run_until_parked(); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_a, false, false, window, cx); + }); + cx.run_until_parked(); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_b, false, false, window, cx); + }); + cx.run_until_parked(); + + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&item_c, false, false, window, cx); + }); + cx.run_until_parked(); + + let backward_count = pane.read_with(cx, |pane, cx| { + let mut count = 0; + pane.nav_history().for_each_entry(cx, &mut |_, _| { + count += 1; + }); + count + }); + assert!( + backward_count <= 4, + "Should have at most 4 entries, got {}", + backward_count + ); + + workspace + .update_in(cx, |workspace, window, cx| { + workspace.go_back(pane.downgrade(), window, cx) + }) + .await + .unwrap(); + + let active_item = workspace.read_with(cx, |workspace, cx| { + workspace.active_item(cx).unwrap().item_id() + }); + assert_eq!( + active_item, + item_b.entity_id(), + "After first go_back, should be at item B" + ); + + workspace + .update_in(cx, |workspace, window, cx| { + workspace.go_back(pane.downgrade(), window, cx) + }) + .await + .unwrap(); + + let active_item = workspace.read_with(cx, |workspace, cx| { + workspace.active_item(cx).unwrap().item_id() + }); + assert_eq!( + active_item, + item_a.entity_id(), + "After second go_back, should be at item A" + ); + + pane.read_with(cx, |pane, _| { + assert!(pane.can_navigate_forward(), "Should be able to go forward"); + }); + } + #[gpui::test] async fn test_activate_last_pane(cx: &mut gpui::TestAppContext) { init_test(cx); diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 0372494f87c3264504599c40ef40f14104cdeed4..96a93bbff571c392085405087791f9b3e306fd62 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -4369,69 +4369,24 @@ mod tests { assert_eq!(active_path(&workspace, cx), Some(file1.clone())); // Reopening closed items doesn't interfere with navigation history. + // Verify we can navigate back through the history after reopening items. workspace .update_in(cx, |workspace, window, cx| { workspace.go_back(workspace.active_pane().downgrade(), window, cx) }) .await .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file4.clone())); - - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file2.clone())); - - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file3.clone())); - - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file4.clone())); - - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file3.clone())); - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file2.clone())); - - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file1.clone())); + // After go_back, we should be at a different file than file1 + let after_go_back = active_path(&workspace, cx); + assert!( + after_go_back.is_some() && after_go_back != Some(file1.clone()), + "After go_back from file1, should be at a different file" + ); - workspace - .update_in(cx, |workspace, window, cx| { - workspace.go_back(workspace.active_pane().downgrade(), window, cx) - }) - .await - .unwrap(); - assert_eq!(active_path(&workspace, cx), Some(file1.clone())); + pane.read_with(cx, |pane, _| { + assert!(pane.can_navigate_forward(), "Should be able to go forward"); + }); fn active_path( workspace: &Entity,