Fix routing of leader updates from unshared projects (#4028)

Max Brunsfeld created 2 years ago

Previously, leader updates in unshared projects would be sent to all
followers regardless of project, as if they were not scoped to any
project.

- Fixes a crash that could sometimes happen when following someone if
they were focused on an unshared project.

Change summary

crates/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(-)

Detailed changes

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::<ChannelView>()
-            .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::<usize>(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::<ChannelView>()
-            .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,

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<Workspace>, 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::<ChannelView>()
+            .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::<usize>(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::<ChannelView>()
+            .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::<Editor>()
+        .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()
+        );
+    });
+}

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::<Self>();
                 editors.find(|editor| {

crates/util/src/util.rs

@@ -137,6 +137,8 @@ pub trait ResultExt<E> {
     type Ok;
 
     fn log_err(self) -> Option<Self::Ok>;
+    /// 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<Self::Ok>;
     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<T> {
         match self {
             Ok(value) => Some(value),

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
         };