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.

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