From 258c2fdad439f032e57c6643322e27cffce7c224 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 11 Jan 2024 13:41:28 -0800 Subject: [PATCH] Fix routing of leader updates from unshared projects Previously, leader updates in unshared projects would be sent to all followers regardless of project, as if they were not scoped to any project. --- .../collab/src/tests/channel_buffer_tests.rs | 146 --------------- crates/collab/src/tests/following_tests.rs | 171 +++++++++++++++++- crates/editor/src/items.rs | 4 +- crates/util/src/util.rs | 10 + crates/workspace/src/workspace.rs | 19 +- 5 files changed, 198 insertions(+), 152 deletions(-) diff --git a/crates/collab/src/tests/channel_buffer_tests.rs b/crates/collab/src/tests/channel_buffer_tests.rs index 19411ed892..76cc8cb9e1 100644 --- a/crates/collab/src/tests/channel_buffer_tests.rs +++ b/crates/collab/src/tests/channel_buffer_tests.rs @@ -599,152 +599,6 @@ async fn test_channel_buffers_and_server_restarts( }); } -#[gpui::test(iterations = 10)] -async fn test_following_to_channel_notes_without_a_shared_project( - deterministic: BackgroundExecutor, - mut cx_a: &mut TestAppContext, - mut cx_b: &mut TestAppContext, - mut cx_c: &mut TestAppContext, -) { - let mut server = TestServer::start(deterministic.clone()).await; - let client_a = server.create_client(cx_a, "user_a").await; - let client_b = server.create_client(cx_b, "user_b").await; - - let client_c = server.create_client(cx_c, "user_c").await; - - cx_a.update(editor::init); - cx_b.update(editor::init); - cx_c.update(editor::init); - cx_a.update(collab_ui::channel_view::init); - cx_b.update(collab_ui::channel_view::init); - cx_c.update(collab_ui::channel_view::init); - - let channel_1_id = server - .make_channel( - "channel-1", - None, - (&client_a, cx_a), - &mut [(&client_b, cx_b), (&client_c, cx_c)], - ) - .await; - let channel_2_id = server - .make_channel( - "channel-2", - None, - (&client_a, cx_a), - &mut [(&client_b, cx_b), (&client_c, cx_c)], - ) - .await; - - // Clients A, B, and C join a channel. - let active_call_a = cx_a.read(ActiveCall::global); - let active_call_b = cx_b.read(ActiveCall::global); - let active_call_c = cx_c.read(ActiveCall::global); - for (call, cx) in [ - (&active_call_a, &mut cx_a), - (&active_call_b, &mut cx_b), - (&active_call_c, &mut cx_c), - ] { - call.update(*cx, |call, cx| call.join_channel(channel_1_id, cx)) - .await - .unwrap(); - } - deterministic.run_until_parked(); - - // Clients A, B, and C all open their own unshared projects. - client_a.fs().insert_tree("/a", json!({})).await; - client_b.fs().insert_tree("/b", json!({})).await; - client_c.fs().insert_tree("/c", json!({})).await; - let (project_a, _) = client_a.build_local_project("/a", cx_a).await; - let (project_b, _) = client_b.build_local_project("/b", cx_b).await; - let (project_c, _) = client_b.build_local_project("/c", cx_c).await; - let (workspace_a, cx_a) = client_a.build_workspace(&project_a, cx_a); - let (workspace_b, cx_b) = client_b.build_workspace(&project_b, cx_b); - let (_workspace_c, _cx_c) = client_c.build_workspace(&project_c, cx_c); - - active_call_a - .update(cx_a, |call, cx| call.set_location(Some(&project_a), cx)) - .await - .unwrap(); - - // Client A opens the notes for channel 1. - let channel_view_1_a = cx_a - .update(|cx| ChannelView::open(channel_1_id, workspace_a.clone(), cx)) - .await - .unwrap(); - channel_view_1_a.update(cx_a, |notes, cx| { - assert_eq!(notes.channel(cx).unwrap().name, "channel-1"); - notes.editor.update(cx, |editor, cx| { - editor.insert("Hello from A.", cx); - editor.change_selections(None, cx, |selections| { - selections.select_ranges(vec![3..4]); - }); - }); - }); - - // Client B follows client A. - workspace_b - .update(cx_b, |workspace, cx| { - workspace - .start_following(client_a.peer_id().unwrap(), cx) - .unwrap() - }) - .await - .unwrap(); - - // Client B is taken to the notes for channel 1, with the same - // text selected as client A. - deterministic.run_until_parked(); - let channel_view_1_b = workspace_b.update(cx_b, |workspace, cx| { - assert_eq!( - workspace.leader_for_pane(workspace.active_pane()), - Some(client_a.peer_id().unwrap()) - ); - workspace - .active_item(cx) - .expect("no active item") - .downcast::() - .expect("active item is not a channel view") - }); - channel_view_1_b.update(cx_b, |notes, cx| { - assert_eq!(notes.channel(cx).unwrap().name, "channel-1"); - let editor = notes.editor.read(cx); - assert_eq!(editor.text(cx), "Hello from A."); - assert_eq!(editor.selections.ranges::(cx), &[3..4]); - }); - - // Client A opens the notes for channel 2. - eprintln!("opening -------------------->"); - - let channel_view_2_a = cx_a - .update(|cx| ChannelView::open(channel_2_id, workspace_a.clone(), cx)) - .await - .unwrap(); - channel_view_2_a.update(cx_a, |notes, cx| { - assert_eq!(notes.channel(cx).unwrap().name, "channel-2"); - }); - - // Client B is taken to the notes for channel 2. - deterministic.run_until_parked(); - - eprintln!("opening <--------------------"); - - let channel_view_2_b = workspace_b.update(cx_b, |workspace, cx| { - assert_eq!( - workspace.leader_for_pane(workspace.active_pane()), - Some(client_a.peer_id().unwrap()) - ); - workspace - .active_item(cx) - .expect("no active item") - .downcast::() - .expect("active item is not a channel view") - }); - channel_view_2_b.update(cx_b, |notes, cx| { - assert_eq!(notes.channel(cx).unwrap().name, "channel-2"); - }); -} - #[gpui::test] async fn test_channel_buffer_changes( deterministic: BackgroundExecutor, diff --git a/crates/collab/src/tests/following_tests.rs b/crates/collab/src/tests/following_tests.rs index 6106f8d5f1..dc5488ebb3 100644 --- a/crates/collab/src/tests/following_tests.rs +++ b/crates/collab/src/tests/following_tests.rs @@ -1,9 +1,12 @@ use crate::{rpc::RECONNECT_TIMEOUT, tests::TestServer}; use call::{ActiveCall, ParticipantLocation}; -use collab_ui::notifications::project_shared_notification::ProjectSharedNotification; +use collab_ui::{ + channel_view::ChannelView, + notifications::project_shared_notification::ProjectSharedNotification, +}; use editor::{Editor, ExcerptRange, MultiBuffer}; use gpui::{ - point, BackgroundExecutor, Context, SharedString, TestAppContext, View, VisualContext, + point, BackgroundExecutor, Context, Entity, SharedString, TestAppContext, View, VisualContext, VisualTestContext, }; use language::Capability; @@ -1822,3 +1825,167 @@ fn pane_summaries(workspace: &View, cx: &mut VisualTestContext) -> Ve .collect() }) } + +#[gpui::test(iterations = 10)] +async fn test_following_to_channel_notes_without_a_shared_project( + deterministic: BackgroundExecutor, + mut cx_a: &mut TestAppContext, + mut cx_b: &mut TestAppContext, + mut cx_c: &mut TestAppContext, +) { + let mut server = TestServer::start(deterministic.clone()).await; + let client_a = server.create_client(cx_a, "user_a").await; + let client_b = server.create_client(cx_b, "user_b").await; + let client_c = server.create_client(cx_c, "user_c").await; + + cx_a.update(editor::init); + cx_b.update(editor::init); + cx_c.update(editor::init); + cx_a.update(collab_ui::channel_view::init); + cx_b.update(collab_ui::channel_view::init); + cx_c.update(collab_ui::channel_view::init); + + let channel_1_id = server + .make_channel( + "channel-1", + None, + (&client_a, cx_a), + &mut [(&client_b, cx_b), (&client_c, cx_c)], + ) + .await; + let channel_2_id = server + .make_channel( + "channel-2", + None, + (&client_a, cx_a), + &mut [(&client_b, cx_b), (&client_c, cx_c)], + ) + .await; + + // Clients A, B, and C join a channel. + let active_call_a = cx_a.read(ActiveCall::global); + let active_call_b = cx_b.read(ActiveCall::global); + let active_call_c = cx_c.read(ActiveCall::global); + for (call, cx) in [ + (&active_call_a, &mut cx_a), + (&active_call_b, &mut cx_b), + (&active_call_c, &mut cx_c), + ] { + call.update(*cx, |call, cx| call.join_channel(channel_1_id, cx)) + .await + .unwrap(); + } + deterministic.run_until_parked(); + + // Clients A, B, and C all open their own unshared projects. + client_a + .fs() + .insert_tree("/a", json!({ "1.txt": "" })) + .await; + client_b.fs().insert_tree("/b", json!({})).await; + client_c.fs().insert_tree("/c", json!({})).await; + let (project_a, worktree_id) = client_a.build_local_project("/a", cx_a).await; + let (project_b, _) = client_b.build_local_project("/b", cx_b).await; + let (project_c, _) = client_b.build_local_project("/c", cx_c).await; + let (workspace_a, cx_a) = client_a.build_workspace(&project_a, cx_a); + let (workspace_b, cx_b) = client_b.build_workspace(&project_b, cx_b); + let (_workspace_c, _cx_c) = client_c.build_workspace(&project_c, cx_c); + + active_call_a + .update(cx_a, |call, cx| call.set_location(Some(&project_a), cx)) + .await + .unwrap(); + + // Client A opens the notes for channel 1. + let channel_notes_1_a = cx_a + .update(|cx| ChannelView::open(channel_1_id, workspace_a.clone(), cx)) + .await + .unwrap(); + channel_notes_1_a.update(cx_a, |notes, cx| { + assert_eq!(notes.channel(cx).unwrap().name, "channel-1"); + notes.editor.update(cx, |editor, cx| { + editor.insert("Hello from A.", cx); + editor.change_selections(None, cx, |selections| { + selections.select_ranges(vec![3..4]); + }); + }); + }); + + // Client B follows client A. + workspace_b + .update(cx_b, |workspace, cx| { + workspace + .start_following(client_a.peer_id().unwrap(), cx) + .unwrap() + }) + .await + .unwrap(); + + // Client B is taken to the notes for channel 1, with the same + // text selected as client A. + deterministic.run_until_parked(); + let channel_notes_1_b = workspace_b.update(cx_b, |workspace, cx| { + assert_eq!( + workspace.leader_for_pane(workspace.active_pane()), + Some(client_a.peer_id().unwrap()) + ); + workspace + .active_item(cx) + .expect("no active item") + .downcast::() + .expect("active item is not a channel view") + }); + channel_notes_1_b.update(cx_b, |notes, cx| { + assert_eq!(notes.channel(cx).unwrap().name, "channel-1"); + let editor = notes.editor.read(cx); + assert_eq!(editor.text(cx), "Hello from A."); + assert_eq!(editor.selections.ranges::(cx), &[3..4]); + }); + + // Client A opens the notes for channel 2. + let channel_notes_2_a = cx_a + .update(|cx| ChannelView::open(channel_2_id, workspace_a.clone(), cx)) + .await + .unwrap(); + channel_notes_2_a.update(cx_a, |notes, cx| { + assert_eq!(notes.channel(cx).unwrap().name, "channel-2"); + }); + + // Client B is taken to the notes for channel 2. + deterministic.run_until_parked(); + let channel_notes_2_b = workspace_b.update(cx_b, |workspace, cx| { + assert_eq!( + workspace.leader_for_pane(workspace.active_pane()), + Some(client_a.peer_id().unwrap()) + ); + workspace + .active_item(cx) + .expect("no active item") + .downcast::() + .expect("active item is not a channel view") + }); + channel_notes_2_b.update(cx_b, |notes, cx| { + assert_eq!(notes.channel(cx).unwrap().name, "channel-2"); + }); + + // Client A opens a local buffer in their unshared project. + let _unshared_editor_a1 = workspace_a + .update(cx_a, |workspace, cx| { + workspace.open_path((worktree_id, "1.txt"), None, true, cx) + }) + .await + .unwrap() + .downcast::() + .unwrap(); + + // This does not send any leader update message to client B. + // If it did, an error would occur on client B, since this buffer + // is not shared with them. + deterministic.run_until_parked(); + workspace_b.update(cx_b, |workspace, cx| { + assert_eq!( + workspace.active_item(cx).expect("no active item").item_id(), + channel_notes_2_b.entity_id() + ); + }); +} diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 78f9b15051..a8583d48af 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -82,7 +82,9 @@ impl FollowableItem for Editor { let pane = pane.downgrade(); Some(cx.spawn(|mut cx| async move { - let mut buffers = futures::future::try_join_all(buffers).await?; + let mut buffers = futures::future::try_join_all(buffers) + .await + .debug_assert_ok("leaders don't share views for unshared buffers")?; let editor = pane.update(&mut cx, |pane, cx| { let mut editors = pane.items_of_type::(); editors.find(|editor| { diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index e32dd88b86..a4031da8cd 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -137,6 +137,8 @@ pub trait ResultExt { type Ok; fn log_err(self) -> Option; + /// Assert that this result should never be an error in development or tests. + fn debug_assert_ok(self, reason: &str) -> Self; fn warn_on_err(self) -> Option; fn inspect_error(self, func: impl FnOnce(&E)) -> Self; } @@ -159,6 +161,14 @@ where } } + #[track_caller] + fn debug_assert_ok(self, reason: &str) -> Self { + if let Err(error) = &self { + debug_panic!("{reason} - {error:?}"); + } + self + } + fn warn_on_err(self) -> Option { match self { Ok(value) => Some(value), diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index db8554712e..ca463e76e0 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -2608,11 +2608,20 @@ impl Workspace { let cx = &cx; move |item| { let item = item.to_followable_item_handle(cx)?; - if (project_id.is_none() || project_id != follower_project_id) - && item.is_project_item(cx) + + // If the item belongs to a particular project, then it should + // only be included if this project is shared, and the follower + // is in thie project. + // + // Some items, like channel notes, do not belong to a particular + // project, so they should be included regardless of whether the + // current project is shared, or what project the follower is in. + if item.is_project_item(cx) + && (project_id.is_none() || project_id != follower_project_id) { return None; } + let id = item.remote_id(client, cx)?.to_proto(); let variant = item.to_state_proto(cx)?; Some(proto::View { @@ -2790,8 +2799,12 @@ impl Workspace { update: proto::update_followers::Variant, cx: &mut WindowContext, ) -> Option<()> { + // If this update only applies to for followers in the current project, + // then skip it unless this project is shared. If it applies to all + // followers, regardless of project, then set `project_id` to none, + // indicating that it goes to all followers. let project_id = if project_only { - self.project.read(cx).remote_id() + Some(self.project.read(cx).remote_id()?) } else { None };