Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix panic when collaborating with new multibuffers (#29245)

Browse source 
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
This commit is contained in:
Conrad Irwin 2025-04-22 22:04:21 -06:00 • committed by GitHub
parent ce1a674eba
commit 4a8f114528
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 161 additions and 46 deletions
Download patch file Download diff file Expand all files Collapse all files

100
crates/collab/src/tests/following_tests.rs
View file

@ -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<Editor> = 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::<Vec<_>>()
});
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::<Editor>())
})
.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::<Vec<_>>()
});
assert_eq!(positions, new_positions);
}
#[gpui::test]
async fn test_following_to_channel_notes_other_workspace(
cx_a: &mut TestAppContext,

19
crates/editor/src/editor_tests.rs
View file

@ -12629,19 +12629,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,
);
});

82
crates/editor/src/items.rs
View file

@ -99,26 +99,41 @@ 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(
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(),
buffer_excerpts.filter_map(deserialize_excerpt_range),
[excerpt],
cx,
);
}
}
};
if let Some(title) = &state.title {
@ -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<Anchor>) -> proto::Selection {
fn serialize_selection(
selection: &Selection<Anchor>,
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<ExcerptRange<language::Anchor>> {
fn deserialize_excerpt_range(
excerpt: proto::Excerpt,
) -> Option<(ExcerptId, ExcerptRange<language::Anchor>)> {
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<ExcerptRange<lan
Some(start..end)
})
.unwrap_or_else(|| context.clone());
Some(ExcerptRange { context, primary })
Some((
ExcerptId::from_proto(excerpt.id),
ExcerptRange { context, primary },
))
}
fn deserialize_selection(