From 17ca3f8e9af2ffb2a4c2d93cdb212749d7a424a0 Mon Sep 17 00:00:00 2001 From: "gcp-cherry-pick-bot[bot]" <98988430+gcp-cherry-pick-bot[bot]@users.noreply.github.com> Date: Tue, 22 Apr 2025 22:28:24 -0600 Subject: [PATCH] Fix panic when collaborating with new multibuffers (cherry-pick #29245) (#29252) Cherry-picked Fix panic when collaborating with new multibuffers (#29245) Before this change, when syncing a multibuffer (such as find-all-references) to a remote, we would renumber the excerpts from 1. This did not matter in the past because the buffers' list of excerpts could not change. In #27876, I added the ability for excerpts to merge, which meant that the excerpt list could change. This manifested as people seeing "invalid excerpt id" panics when syncing. The initial fix to this (to re-use the excerpt ids from the host) ran into problems because `insert_excerpts_with_ids_after` assumes that you call it in excerpt-id order. This change de-optimizes that code to insert the excerpts 1-by-1 in excerpt-id order, but with the insert_after set to preserve the correct UI order. I hope to soon remove this code path and use something more like set-excerpts-for-path for syncing, but in the meantime we should not panic. Release Notes: - Fix a panic when joining a project with a multibuffer with merged excerpts Co-authored-by: Conrad Irwin --- crates/collab/src/tests/following_tests.rs | 100 +++++++++++++++++++-- crates/editor/src/editor_tests.rs | 19 ++-- crates/editor/src/items.rs | 88 +++++++++++------- 3 files changed, 161 insertions(+), 46 deletions(-) diff --git a/crates/collab/src/tests/following_tests.rs b/crates/collab/src/tests/following_tests.rs index 66b129c7832ce5c8046a4ec8262cc35a91dfb91a..78d30adb4a53ccf5ac2b0bbe9b0e1d93a26e9f2b 100644 --- a/crates/collab/src/tests/following_tests.rs +++ b/crates/collab/src/tests/following_tests.rs @@ -6,17 +6,18 @@ use collab_ui::{ channel_view::ChannelView, notifications::project_shared_notification::ProjectSharedNotification, }; -use editor::{Editor, ExcerptRange, MultiBuffer}; +use editor::{Editor, MultiBuffer, PathKey}; use gpui::{ AppContext as _, BackgroundExecutor, BorrowAppContext, Entity, SharedString, TestAppContext, - VisualTestContext, point, + VisualContext, VisualTestContext, point, }; use language::Capability; use project::WorktreeSettings; use rpc::proto::PeerId; use serde_json::json; use settings::SettingsStore; -use util::path; +use text::{Point, ToPoint}; +use util::{path, test::sample_text}; use workspace::{SplitDirection, Workspace, item::ItemHandle as _}; use super::TestClient; @@ -295,8 +296,20 @@ async fn test_basic_following( .unwrap() }); let mut result = MultiBuffer::new(Capability::ReadWrite); - result.push_excerpts(buffer_a1, [ExcerptRange::new(0..3)], cx); - result.push_excerpts(buffer_a2, [ExcerptRange::new(4..7)], cx); + result.set_excerpts_for_path( + PathKey::for_buffer(&buffer_a1, cx), + buffer_a1, + [Point::row_range(1..2)], + 1, + cx, + ); + result.set_excerpts_for_path( + PathKey::for_buffer(&buffer_a2, cx), + buffer_a2, + [Point::row_range(5..6)], + 1, + cx, + ); result }); let multibuffer_editor_a = workspace_a.update_in(cx_a, |workspace, window, cx| { @@ -2070,6 +2083,83 @@ async fn share_workspace( .await } +#[gpui::test] +async fn test_following_after_replacement(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext) { + let (_server, client_a, client_b, channel) = TestServer::start2(cx_a, cx_b).await; + + let (workspace, cx_a) = client_a.build_test_workspace(cx_a).await; + join_channel(channel, &client_a, cx_a).await.unwrap(); + share_workspace(&workspace, cx_a).await.unwrap(); + let buffer = workspace.update(cx_a, |workspace, cx| { + workspace.project().update(cx, |project, cx| { + project.create_local_buffer(&sample_text(26, 5, 'a'), None, cx) + }) + }); + let multibuffer = cx_a.new(|cx| { + let mut mb = MultiBuffer::new(Capability::ReadWrite); + mb.set_excerpts_for_path( + PathKey::for_buffer(&buffer, cx), + buffer.clone(), + [Point::row_range(1..1), Point::row_range(5..5)], + 1, + cx, + ); + mb + }); + let snapshot = buffer.update(cx_a, |buffer, _| buffer.snapshot()); + let editor: Entity = cx_a.new_window_entity(|window, cx| { + Editor::for_multibuffer( + multibuffer.clone(), + Some(workspace.read(cx).project().clone()), + window, + cx, + ) + }); + workspace.update_in(cx_a, |workspace, window, cx| { + workspace.add_item_to_center(Box::new(editor.clone()) as _, window, cx) + }); + editor.update_in(cx_a, |editor, window, cx| { + editor.change_selections(None, window, cx, |s| { + s.select_ranges([Point::row_range(4..4)]); + }) + }); + let positions = editor.update(cx_a, |editor, _| { + editor + .selections + .disjoint_anchor_ranges() + .map(|range| range.start.text_anchor.to_point(&snapshot)) + .collect::>() + }); + multibuffer.update(cx_a, |multibuffer, cx| { + multibuffer.set_excerpts_for_path( + PathKey::for_buffer(&buffer, cx), + buffer, + [Point::row_range(1..5)], + 1, + cx, + ); + }); + + let (workspace_b, cx_b) = client_b.join_workspace(channel, cx_b).await; + cx_b.run_until_parked(); + let editor_b = workspace_b + .update(cx_b, |workspace, cx| { + workspace + .active_item(cx) + .and_then(|item| item.downcast::()) + }) + .unwrap(); + + let new_positions = editor_b.update(cx_b, |editor, _| { + editor + .selections + .disjoint_anchor_ranges() + .map(|range| range.start.text_anchor.to_point(&snapshot)) + .collect::>() + }); + assert_eq!(positions, new_positions); +} + #[gpui::test] async fn test_following_to_channel_notes_other_workspace( cx_a: &mut TestAppContext, diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 2696cb406ebfb4a9f92047e59a14a77002479e94..1dc8efd30b1fc25470e37772d1a73221f93b0c80 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -12699,19 +12699,22 @@ async fn test_following_with_multiple_excerpts(cx: &mut TestAppContext) { // Insert some excerpts. leader.update(cx, |leader, cx| { leader.buffer.update(cx, |multibuffer, cx| { - let excerpt_ids = multibuffer.push_excerpts( + multibuffer.set_excerpts_for_path( + PathKey::namespaced(1, Arc::from(Path::new("b.txt"))), buffer_1.clone(), - [ - ExcerptRange::new(1..6), - ExcerptRange::new(12..15), - ExcerptRange::new(0..3), + vec![ + Point::row_range(0..3), + Point::row_range(1..6), + Point::row_range(12..15), ], + 0, cx, ); - multibuffer.insert_excerpts_after( - excerpt_ids[0], + multibuffer.set_excerpts_for_path( + PathKey::namespaced(1, Arc::from(Path::new("a.txt"))), buffer_2.clone(), - [ExcerptRange::new(8..12), ExcerptRange::new(0..6)], + vec![Point::row_range(0..6), Point::row_range(8..12)], + 0, cx, ); }); diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 3d493db8ce7b60f3521789992d7ce05c186911c4..d549f1b9c6b201c406bf3d2b2b4aa8083063d177 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -99,25 +99,40 @@ impl FollowableItem for Editor { multibuffer = MultiBuffer::singleton(buffers.pop().unwrap(), cx) } else { multibuffer = MultiBuffer::new(project.read(cx).capability()); - let mut excerpts = state.excerpts.into_iter().peekable(); - while let Some(excerpt) = excerpts.peek() { + let mut sorted_excerpts = state.excerpts.clone(); + sorted_excerpts.sort_by_key(|e| e.id); + let mut sorted_excerpts = sorted_excerpts.into_iter().peekable(); + + while let Some(excerpt) = sorted_excerpts.next() { let Ok(buffer_id) = BufferId::new(excerpt.buffer_id) else { continue; }; - let buffer_excerpts = iter::from_fn(|| { - let excerpt = excerpts.peek()?; - (excerpt.buffer_id == u64::from(buffer_id)) - .then(|| excerpts.next().unwrap()) - }); + + let mut insert_position = ExcerptId::min(); + for e in &state.excerpts { + if e.id == excerpt.id { + break; + } + if e.id < excerpt.id { + insert_position = ExcerptId::from_proto(e.id); + } + } + let buffer = buffers.iter().find(|b| b.read(cx).remote_id() == buffer_id); - if let Some(buffer) = buffer { - multibuffer.push_excerpts( - buffer.clone(), - buffer_excerpts.filter_map(deserialize_excerpt_range), - cx, - ); - } + + let Some(excerpt) = deserialize_excerpt_range(excerpt) else { + continue; + }; + + let Some(buffer) = buffer else { continue }; + + multibuffer.insert_excerpts_with_ids_after( + insert_position, + buffer.clone(), + [excerpt], + cx, + ); } }; @@ -202,25 +217,26 @@ impl FollowableItem for Editor { primary_end: Some(serialize_text_anchor(&range.primary.end)), }) .collect(); + let snapshot = buffer.snapshot(cx); Some(proto::view::Variant::Editor(proto::view::Editor { singleton: buffer.is_singleton(), title: (!buffer.is_singleton()).then(|| buffer.title(cx).into()), excerpts, - scroll_top_anchor: Some(serialize_anchor(&scroll_anchor.anchor)), + scroll_top_anchor: Some(serialize_anchor(&scroll_anchor.anchor, &snapshot)), scroll_x: scroll_anchor.offset.x, scroll_y: scroll_anchor.offset.y, selections: self .selections .disjoint_anchors() .iter() - .map(serialize_selection) + .map(|s| serialize_selection(s, &snapshot)) .collect(), pending_selection: self .selections .pending_anchor() .as_ref() - .map(serialize_selection), + .map(|s| serialize_selection(s, &snapshot)), })) } @@ -279,24 +295,27 @@ impl FollowableItem for Editor { true } EditorEvent::ScrollPositionChanged { autoscroll, .. } if !autoscroll => { + let snapshot = self.buffer.read(cx).snapshot(cx); let scroll_anchor = self.scroll_manager.anchor(); - update.scroll_top_anchor = Some(serialize_anchor(&scroll_anchor.anchor)); + update.scroll_top_anchor = + Some(serialize_anchor(&scroll_anchor.anchor, &snapshot)); update.scroll_x = scroll_anchor.offset.x; update.scroll_y = scroll_anchor.offset.y; true } EditorEvent::SelectionsChanged { .. } => { + let snapshot = self.buffer.read(cx).snapshot(cx); update.selections = self .selections .disjoint_anchors() .iter() - .map(serialize_selection) + .map(|s| serialize_selection(s, &snapshot)) .collect(); update.pending_selection = self .selections .pending_anchor() .as_ref() - .map(serialize_selection); + .map(|s| serialize_selection(s, &snapshot)); true } _ => false, @@ -396,12 +415,7 @@ async fn update_editor_from_message( [excerpt] .into_iter() .chain(adjacent_excerpts) - .filter_map(|excerpt| { - Some(( - ExcerptId::from_proto(excerpt.id), - deserialize_excerpt_range(excerpt)?, - )) - }), + .filter_map(deserialize_excerpt_range), cx, ); } @@ -478,23 +492,28 @@ fn serialize_excerpt( }) } -fn serialize_selection(selection: &Selection) -> proto::Selection { +fn serialize_selection( + selection: &Selection, + buffer: &MultiBufferSnapshot, +) -> proto::Selection { proto::Selection { id: selection.id as u64, - start: Some(serialize_anchor(&selection.start)), - end: Some(serialize_anchor(&selection.end)), + start: Some(serialize_anchor(&selection.start, &buffer)), + end: Some(serialize_anchor(&selection.end, &buffer)), reversed: selection.reversed, } } -fn serialize_anchor(anchor: &Anchor) -> proto::EditorAnchor { +fn serialize_anchor(anchor: &Anchor, buffer: &MultiBufferSnapshot) -> proto::EditorAnchor { proto::EditorAnchor { - excerpt_id: anchor.excerpt_id.to_proto(), + excerpt_id: buffer.latest_excerpt_id(anchor.excerpt_id).to_proto(), anchor: Some(serialize_text_anchor(&anchor.text_anchor)), } } -fn deserialize_excerpt_range(excerpt: proto::Excerpt) -> Option> { +fn deserialize_excerpt_range( + excerpt: proto::Excerpt, +) -> Option<(ExcerptId, ExcerptRange)> { let context = { let start = language::proto::deserialize_anchor(excerpt.context_start?)?; let end = language::proto::deserialize_anchor(excerpt.context_end?)?; @@ -509,7 +528,10 @@ fn deserialize_excerpt_range(excerpt: proto::Excerpt) -> Option