Aggressively simplify channel permissions:

Conrad Irwin created

- Only allow setting permissions on the root channel
- Only allow public channels to be children of public channels

Change summary

crates/channel/src/channel_buffer.rs               |   5 
crates/channel/src/channel_store.rs                |  58 +
crates/channel/src/channel_store/channel_index.rs  |   7 
crates/channel/src/channel_store_tests.rs          |  42 
crates/collab/src/db.rs                            |  37 
crates/collab/src/db/ids.rs                        |   9 
crates/collab/src/db/queries/channels.rs           | 616 ++++-----------
crates/collab/src/db/tables/channel.rs             |   8 
crates/collab/src/db/tests.rs                      |   5 
crates/collab/src/db/tests/channel_tests.rs        | 231 ++---
crates/collab/src/db/tests/message_tests.rs        |   9 
crates/collab/src/rpc.rs                           | 175 ++--
crates/collab/src/tests/channel_guest_tests.rs     |   7 
crates/collab/src/tests/channel_tests.rs           | 161 ---
crates/collab/src/tests/test_server.rs             |   1 
crates/collab_ui/src/collab_panel.rs               |  43 
crates/collab_ui/src/collab_panel/channel_modal.rs |  16 
crates/rpc/proto/zed.proto                         |  11 
18 files changed, 474 insertions(+), 967 deletions(-)

Detailed changes

crates/channel/src/channel_buffer.rs 🔗

@@ -61,11 +61,12 @@ impl ChannelBuffer {
             .map(language::proto::deserialize_operation)
             .collect::<Result<Vec<_>, _>>()?;
 
-        let buffer = cx.new_model(|_| {
+        let buffer = cx.new_model(|cx| {
+            let capability = channel_store.read(cx).channel_capability(channel.id);
             language::Buffer::remote(
                 response.buffer_id,
                 response.replica_id as u16,
-                channel.channel_buffer_capability(),
+                capability,
                 base_text,
             )
         })?;

crates/channel/src/channel_store.rs 🔗

@@ -13,11 +13,11 @@ use gpui::{
 };
 use language::Capability;
 use rpc::{
-    proto::{self, ChannelVisibility},
+    proto::{self, ChannelRole, ChannelVisibility},
     TypedEnvelope,
 };
 use std::{mem, sync::Arc, time::Duration};
-use util::{async_maybe, ResultExt};
+use util::{async_maybe, maybe, ResultExt};
 
 pub fn init(client: &Arc<Client>, user_store: Model<UserStore>, cx: &mut AppContext) {
     let channel_store =
@@ -58,7 +58,6 @@ pub struct Channel {
     pub id: ChannelId,
     pub name: SharedString,
     pub visibility: proto::ChannelVisibility,
-    pub role: proto::ChannelRole,
     pub parent_path: Vec<u64>,
 }
 
@@ -68,6 +67,7 @@ pub struct ChannelState {
     latest_notes_versions: Option<NotesVersion>,
     observed_chat_message: Option<u64>,
     observed_notes_versions: Option<NotesVersion>,
+    role: Option<ChannelRole>,
 }
 
 impl Channel {
@@ -90,11 +90,12 @@ impl Channel {
     }
 
     pub fn channel_buffer_capability(&self) -> Capability {
-        if self.role == proto::ChannelRole::Member || self.role == proto::ChannelRole::Admin {
-            Capability::ReadWrite
-        } else {
-            Capability::ReadOnly
-        }
+        todo!() // go ask the channel store
+                // if self.role == proto::ChannelRole::Member || self.role == proto::ChannelRole::Admin {
+                //     Capability::ReadWrite
+                // } else {
+                //     Capability::ReadOnly
+                // }
     }
 }
 
@@ -114,8 +115,7 @@ impl ChannelMembership {
             },
             kind_order: match self.kind {
                 proto::channel_member::Kind::Member => 0,
-                proto::channel_member::Kind::AncestorMember => 1,
-                proto::channel_member::Kind::Invitee => 2,
+                proto::channel_member::Kind::Invitee => 1,
             },
             username_order: self.user.github_login.as_str(),
         }
@@ -479,10 +479,25 @@ impl ChannelStore {
     }
 
     pub fn is_channel_admin(&self, channel_id: ChannelId) -> bool {
-        let Some(channel) = self.channel_for_id(channel_id) else {
-            return false;
-        };
-        channel.role == proto::ChannelRole::Admin
+        self.channel_role(channel_id) == proto::ChannelRole::Admin
+    }
+
+    pub fn channel_capability(&self, channel_id: ChannelId) -> Capability {
+        match self.channel_role(channel_id) {
+            ChannelRole::Admin | ChannelRole::Member => Capability::ReadWrite,
+            _ => Capability::ReadOnly,
+        }
+    }
+
+    pub fn channel_role(&self, channel_id: ChannelId) -> proto::ChannelRole {
+        maybe!({
+            let channel = self.channel_for_id(channel_id)?;
+            let root_channel_id = channel.parent_path.first()?;
+            let root_channel_state = self.channel_states.get(&root_channel_id);
+            debug_assert!(root_channel_state.is_some());
+            root_channel_state?.role
+        })
+        .unwrap_or(proto::ChannelRole::Guest)
     }
 
     pub fn channel_participants(&self, channel_id: ChannelId) -> &[Arc<User>] {
@@ -533,7 +548,7 @@ impl ChannelStore {
     pub fn move_channel(
         &mut self,
         channel_id: ChannelId,
-        to: Option<ChannelId>,
+        to: ChannelId,
         cx: &mut ModelContext<Self>,
     ) -> Task<Result<()>> {
         let client = self.client.clone();
@@ -791,6 +806,14 @@ impl ChannelStore {
             for message_id in message.payload.observed_channel_message_id {
                 this.acknowledge_message_id(message_id.channel_id, message_id.message_id, cx);
             }
+            for membership in message.payload.channel_memberships {
+                if let Some(role) = ChannelRole::from_i32(membership.role) {
+                    this.channel_states
+                        .entry(membership.channel_id)
+                        .or_insert_with(|| ChannelState::default())
+                        .set_role(role)
+                }
+            }
         })
     }
 
@@ -956,7 +979,6 @@ impl ChannelStore {
                     Arc::new(Channel {
                         id: channel.id,
                         visibility: channel.visibility(),
-                        role: channel.role(),
                         name: channel.name.into(),
                         parent_path: channel.parent_path,
                     }),
@@ -1071,6 +1093,10 @@ impl ChannelStore {
 }
 
 impl ChannelState {
+    fn set_role(&mut self, role: ChannelRole) {
+        self.role = Some(role);
+    }
+
     fn has_channel_buffer_changed(&self) -> bool {
         if let Some(latest_version) = &self.latest_notes_versions {
             if let Some(observed_version) = &self.observed_notes_versions {

crates/channel/src/channel_store/channel_index.rs 🔗

@@ -54,19 +54,18 @@ impl<'a> ChannelPathsInsertGuard<'a> {
             let existing_channel = Arc::make_mut(existing_channel);
 
             ret = existing_channel.visibility != channel_proto.visibility()
-                || existing_channel.role != channel_proto.role()
-                || existing_channel.name != channel_proto.name;
+                || existing_channel.name != channel_proto.name
+                || existing_channel.parent_path != channel_proto.parent_path;
 
             existing_channel.visibility = channel_proto.visibility();
-            existing_channel.role = channel_proto.role();
             existing_channel.name = channel_proto.name.into();
+            existing_channel.parent_path = channel_proto.parent_path.into();
         } else {
             self.channels_by_id.insert(
                 channel_proto.id,
                 Arc::new(Channel {
                     id: channel_proto.id,
                     visibility: channel_proto.visibility(),
-                    role: channel_proto.role(),
                     name: channel_proto.name.into(),
                     parent_path: channel_proto.parent_path,
                 }),

crates/channel/src/channel_store_tests.rs 🔗

@@ -19,14 +19,12 @@ fn test_update_channels(cx: &mut AppContext) {
                     id: 1,
                     name: "b".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Admin.into(),
                     parent_path: Vec::new(),
                 },
                 proto::Channel {
                     id: 2,
                     name: "a".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Member.into(),
                     parent_path: Vec::new(),
                 },
             ],
@@ -38,8 +36,8 @@ fn test_update_channels(cx: &mut AppContext) {
         &channel_store,
         &[
             //
-            (0, "a".to_string(), proto::ChannelRole::Member),
-            (0, "b".to_string(), proto::ChannelRole::Admin),
+            (0, "a".to_string()),
+            (0, "b".to_string()),
         ],
         cx,
     );
@@ -52,14 +50,12 @@ fn test_update_channels(cx: &mut AppContext) {
                     id: 3,
                     name: "x".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Admin.into(),
                     parent_path: vec![1],
                 },
                 proto::Channel {
                     id: 4,
                     name: "y".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Member.into(),
                     parent_path: vec![2],
                 },
             ],
@@ -70,10 +66,10 @@ fn test_update_channels(cx: &mut AppContext) {
     assert_channels(
         &channel_store,
         &[
-            (0, "a".to_string(), proto::ChannelRole::Member),
-            (1, "y".to_string(), proto::ChannelRole::Member),
-            (0, "b".to_string(), proto::ChannelRole::Admin),
-            (1, "x".to_string(), proto::ChannelRole::Admin),
+            (0, "a".to_string()),
+            (1, "y".to_string()),
+            (0, "b".to_string()),
+            (1, "x".to_string()),
         ],
         cx,
     );
@@ -91,21 +87,18 @@ fn test_dangling_channel_paths(cx: &mut AppContext) {
                     id: 0,
                     name: "a".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Admin.into(),
                     parent_path: vec![],
                 },
                 proto::Channel {
                     id: 1,
                     name: "b".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Admin.into(),
                     parent_path: vec![0],
                 },
                 proto::Channel {
                     id: 2,
                     name: "c".to_string(),
                     visibility: proto::ChannelVisibility::Members as i32,
-                    role: proto::ChannelRole::Admin.into(),
                     parent_path: vec![0, 1],
                 },
             ],
@@ -118,9 +111,9 @@ fn test_dangling_channel_paths(cx: &mut AppContext) {
         &channel_store,
         &[
             //
-            (0, "a".to_string(), proto::ChannelRole::Admin),
-            (1, "b".to_string(), proto::ChannelRole::Admin),
-            (2, "c".to_string(), proto::ChannelRole::Admin),
+            (0, "a".to_string()),
+            (1, "b".to_string()),
+            (2, "c".to_string()),
         ],
         cx,
     );
@@ -135,11 +128,7 @@ fn test_dangling_channel_paths(cx: &mut AppContext) {
     );
 
     // Make sure that the 1/2/3 path is gone
-    assert_channels(
-        &channel_store,
-        &[(0, "a".to_string(), proto::ChannelRole::Admin)],
-        cx,
-    );
+    assert_channels(&channel_store, &[(0, "a".to_string())], cx);
 }
 
 #[gpui::test]
@@ -156,18 +145,13 @@ async fn test_channel_messages(cx: &mut TestAppContext) {
             id: channel_id,
             name: "the-channel".to_string(),
             visibility: proto::ChannelVisibility::Members as i32,
-            role: proto::ChannelRole::Member.into(),
             parent_path: vec![],
         }],
         ..Default::default()
     });
     cx.executor().run_until_parked();
     cx.update(|cx| {
-        assert_channels(
-            &channel_store,
-            &[(0, "the-channel".to_string(), proto::ChannelRole::Member)],
-            cx,
-        );
+        assert_channels(&channel_store, &[(0, "the-channel".to_string())], cx);
     });
 
     let get_users = server.receive::<proto::GetUsers>().await.unwrap();
@@ -368,13 +352,13 @@ fn update_channels(
 #[track_caller]
 fn assert_channels(
     channel_store: &Model<ChannelStore>,
-    expected_channels: &[(usize, String, proto::ChannelRole)],
+    expected_channels: &[(usize, String)],
     cx: &mut AppContext,
 ) {
     let actual = channel_store.update(cx, |store, _| {
         store
             .ordered_channels()
-            .map(|(depth, channel)| (depth, channel.name.to_string(), channel.role))
+            .map(|(depth, channel)| (depth, channel.name.to_string()))
             .collect::<Vec<_>>()
     });
     assert_eq!(actual, expected_channels);

crates/collab/src/db.rs 🔗

@@ -40,7 +40,7 @@ use std::{
     sync::Arc,
     time::Duration,
 };
-use tables::*;
+pub use tables::*;
 use tokio::sync::{Mutex, OwnedMutexGuard};
 
 pub use ids::*;
@@ -502,35 +502,6 @@ pub struct NewUserResult {
     pub signup_device_id: Option<String>,
 }
 
-/// The result of moving a channel.
-#[derive(Debug)]
-pub struct MoveChannelResult {
-    pub previous_participants: Vec<ChannelMember>,
-    pub descendent_ids: Vec<ChannelId>,
-}
-
-/// The result of renaming a channel.
-#[derive(Debug)]
-pub struct RenameChannelResult {
-    pub channel: Channel,
-    pub participants_to_update: HashMap<UserId, Channel>,
-}
-
-/// The result of creating a channel.
-#[derive(Debug)]
-pub struct CreateChannelResult {
-    pub channel: Channel,
-    pub participants_to_update: Vec<(UserId, ChannelsForUser)>,
-}
-
-/// The result of setting a channel's visibility.
-#[derive(Debug)]
-pub struct SetChannelVisibilityResult {
-    pub participants_to_update: HashMap<UserId, ChannelsForUser>,
-    pub participants_to_remove: HashSet<UserId>,
-    pub channels_to_remove: Vec<ChannelId>,
-}
-
 /// The result of updating a channel membership.
 #[derive(Debug)]
 pub struct MembershipUpdated {
@@ -570,18 +541,16 @@ pub struct Channel {
     pub id: ChannelId,
     pub name: String,
     pub visibility: ChannelVisibility,
-    pub role: ChannelRole,
     /// parent_path is the channel ids from the root to this one (not including this one)
     pub parent_path: Vec<ChannelId>,
 }
 
 impl Channel {
-    fn from_model(value: channel::Model, role: ChannelRole) -> Self {
+    fn from_model(value: channel::Model) -> Self {
         Channel {
             id: value.id,
             visibility: value.visibility,
             name: value.clone().name,
-            role,
             parent_path: value.ancestors().collect(),
         }
     }
@@ -591,7 +560,6 @@ impl Channel {
             id: self.id.to_proto(),
             name: self.name.clone(),
             visibility: self.visibility.into(),
-            role: self.role.into(),
             parent_path: self.parent_path.iter().map(|c| c.to_proto()).collect(),
         }
     }
@@ -617,6 +585,7 @@ impl ChannelMember {
 #[derive(Debug, PartialEq)]
 pub struct ChannelsForUser {
     pub channels: Vec<Channel>,
+    pub channel_memberships: Vec<channel_member::Model>,
     pub channel_participants: HashMap<ChannelId, Vec<UserId>>,
     pub latest_buffer_versions: Vec<proto::ChannelBufferVersion>,
     pub latest_channel_messages: Vec<proto::ChannelMessageId>,

crates/collab/src/db/ids.rs 🔗

@@ -129,6 +129,15 @@ impl ChannelRole {
         }
     }
 
+    pub fn can_see_channel(&self, visibility: ChannelVisibility) -> bool {
+        use ChannelRole::*;
+        match self {
+            Admin | Member => true,
+            Guest => visibility == ChannelVisibility::Public,
+            Banned => false,
+        }
+    }
+
     /// True if the role allows access to all descendant channels
     pub fn can_see_all_descendants(&self) -> bool {
         use ChannelRole::*;

crates/collab/src/db/queries/channels.rs 🔗

@@ -19,7 +19,7 @@ impl Database {
 
     #[cfg(test)]
     pub async fn create_root_channel(&self, name: &str, creator_id: UserId) -> Result<ChannelId> {
-        Ok(self.create_channel(name, None, creator_id).await?.id)
+        Ok(self.create_channel(name, None, creator_id).await?.0.id)
     }
 
     #[cfg(test)]
@@ -32,6 +32,7 @@ impl Database {
         Ok(self
             .create_channel(name, Some(parent), creator_id)
             .await?
+            .0
             .id)
     }
 
@@ -41,10 +42,15 @@ impl Database {
         name: &str,
         parent_channel_id: Option<ChannelId>,
         admin_id: UserId,
-    ) -> Result<Channel> {
+    ) -> Result<(
+        Channel,
+        Option<channel_member::Model>,
+        Vec<channel_member::Model>,
+    )> {
         let name = Self::sanitize_channel_name(name)?;
         self.transaction(move |tx| async move {
             let mut parent = None;
+            let mut membership = None;
 
             if let Some(parent_channel_id) = parent_channel_id {
                 let parent_channel = self.get_channel_internal(parent_channel_id, &*tx).await?;
@@ -68,18 +74,25 @@ impl Database {
             .await?;
 
             if parent.is_none() {
-                channel_member::ActiveModel {
-                    id: ActiveValue::NotSet,
-                    channel_id: ActiveValue::Set(channel.id),
-                    user_id: ActiveValue::Set(admin_id),
-                    accepted: ActiveValue::Set(true),
-                    role: ActiveValue::Set(ChannelRole::Admin),
-                }
-                .insert(&*tx)
-                .await?;
+                membership = Some(
+                    channel_member::ActiveModel {
+                        id: ActiveValue::NotSet,
+                        channel_id: ActiveValue::Set(channel.id),
+                        user_id: ActiveValue::Set(admin_id),
+                        accepted: ActiveValue::Set(true),
+                        role: ActiveValue::Set(ChannelRole::Admin),
+                    }
+                    .insert(&*tx)
+                    .await?,
+                );
             }
 
-            Ok(Channel::from_model(channel, ChannelRole::Admin))
+            let channel_members = channel_member::Entity::find()
+                .filter(channel_member::Column::ChannelId.eq(channel.root_id()))
+                .all(&*tx)
+                .await?;
+
+            Ok((Channel::from_model(channel), membership, channel_members))
         })
         .await
     }
@@ -122,16 +135,9 @@ impl Database {
                     );
                 } else if channel.visibility == ChannelVisibility::Public {
                     role = Some(ChannelRole::Guest);
-                    let channel_to_join = self
-                        .public_ancestors_including_self(&channel, &*tx)
-                        .await?
-                        .first()
-                        .cloned()
-                        .unwrap_or(channel.clone());
-
                     channel_member::Entity::insert(channel_member::ActiveModel {
                         id: ActiveValue::NotSet,
-                        channel_id: ActiveValue::Set(channel_to_join.id),
+                        channel_id: ActiveValue::Set(channel.root_id()),
                         user_id: ActiveValue::Set(user_id),
                         accepted: ActiveValue::Set(true),
                         role: ActiveValue::Set(ChannelRole::Guest),
@@ -140,7 +146,7 @@ impl Database {
                     .await?;
 
                     accept_invite_result = Some(
-                        self.calculate_membership_updated(&channel_to_join, user_id, &*tx)
+                        self.calculate_membership_updated(&channel, user_id, &*tx)
                             .await?,
                     );
 
@@ -173,76 +179,43 @@ impl Database {
         channel_id: ChannelId,
         visibility: ChannelVisibility,
         admin_id: UserId,
-    ) -> Result<SetChannelVisibilityResult> {
+    ) -> Result<(Channel, Vec<channel_member::Model>)> {
         self.transaction(move |tx| async move {
             let channel = self.get_channel_internal(channel_id, &*tx).await?;
-
             self.check_user_is_channel_admin(&channel, admin_id, &*tx)
                 .await?;
 
-            let previous_members = self
-                .get_channel_participant_details_internal(&channel, &*tx)
-                .await?;
-
-            let mut model = channel.into_active_model();
-            model.visibility = ActiveValue::Set(visibility);
-            let channel = model.update(&*tx).await?;
-
-            let mut participants_to_update: HashMap<UserId, ChannelsForUser> = self
-                .participants_to_notify_for_channel_change(&channel, &*tx)
-                .await?
-                .into_iter()
-                .collect();
+            if visibility == ChannelVisibility::Public {
+                if let Some(parent_id) = channel.parent_id() {
+                    let parent = self.get_channel_internal(parent_id, &*tx).await?;
 
-            let mut channels_to_remove: Vec<ChannelId> = vec![];
-            let mut participants_to_remove: HashSet<UserId> = HashSet::default();
-            match visibility {
-                ChannelVisibility::Members => {
-                    let all_descendents: Vec<ChannelId> = self
-                        .get_channel_descendants_including_self(vec![channel_id], &*tx)
-                        .await?
-                        .into_iter()
-                        .map(|channel| channel.id)
-                        .collect();
-
-                    channels_to_remove = channel::Entity::find()
-                        .filter(
-                            channel::Column::Id
-                                .is_in(all_descendents)
-                                .and(channel::Column::Visibility.eq(ChannelVisibility::Public)),
-                        )
-                        .all(&*tx)
-                        .await?
-                        .into_iter()
-                        .map(|channel| channel.id)
-                        .collect();
-
-                    channels_to_remove.push(channel_id);
-
-                    for member in previous_members {
-                        if member.role.can_only_see_public_descendants() {
-                            participants_to_remove.insert(member.user_id);
-                        }
+                    if parent.visibility != ChannelVisibility::Public {
+                        Err(anyhow!("public channels must descend from public channels"))?;
                     }
                 }
-                ChannelVisibility::Public => {
-                    if let Some(public_parent) = self.public_parent_channel(&channel, &*tx).await? {
-                        let parent_updates = self
-                            .participants_to_notify_for_channel_change(&public_parent, &*tx)
-                            .await?;
-
-                        for (user_id, channels) in parent_updates {
-                            participants_to_update.insert(user_id, channels);
-                        }
-                    }
+            } else if visibility == ChannelVisibility::Members {
+                if self
+                    .get_channel_descendants_including_self(vec![channel_id], &*tx)
+                    .await?
+                    .into_iter()
+                    .any(|channel| {
+                        channel.id != channel_id && channel.visibility == ChannelVisibility::Public
+                    })
+                {
+                    Err(anyhow!("cannot make a parent of a public channel private"))?;
                 }
             }
 
-            Ok(SetChannelVisibilityResult {
-                participants_to_update,
-                participants_to_remove,
-                channels_to_remove,
-            })
+            let mut model = channel.into_active_model();
+            model.visibility = ActiveValue::Set(visibility);
+            let channel = model.update(&*tx).await?;
+
+            let channel_members = channel_member::Entity::find()
+                .filter(channel_member::Column::ChannelId.eq(channel.root_id()))
+                .all(&*tx)
+                .await?;
+
+            Ok((Channel::from_model(channel), channel_members))
         })
         .await
     }
@@ -275,7 +248,7 @@ impl Database {
                 .await?;
 
             let members_to_notify: Vec<UserId> = channel_member::Entity::find()
-                .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self()))
+                .filter(channel_member::Column::ChannelId.eq(channel.root_id()))
                 .select_only()
                 .column(channel_member::Column::UserId)
                 .distinct()
@@ -312,6 +285,9 @@ impl Database {
             let channel = self.get_channel_internal(channel_id, &*tx).await?;
             self.check_user_is_channel_admin(&channel, inviter_id, &*tx)
                 .await?;
+            if !channel.is_root() {
+                Err(ErrorCode::NotARootChannel.anyhow())?
+            }
 
             channel_member::ActiveModel {
                 id: ActiveValue::NotSet,
@@ -323,7 +299,7 @@ impl Database {
             .insert(&*tx)
             .await?;
 
-            let channel = Channel::from_model(channel, role);
+            let channel = Channel::from_model(channel);
 
             let notifications = self
                 .create_notification(
@@ -362,35 +338,24 @@ impl Database {
         channel_id: ChannelId,
         admin_id: UserId,
         new_name: &str,
-    ) -> Result<RenameChannelResult> {
+    ) -> Result<(Channel, Vec<channel_member::Model>)> {
         self.transaction(move |tx| async move {
             let new_name = Self::sanitize_channel_name(new_name)?.to_string();
 
             let channel = self.get_channel_internal(channel_id, &*tx).await?;
-            let role = self
-                .check_user_is_channel_admin(&channel, admin_id, &*tx)
+            self.check_user_is_channel_admin(&channel, admin_id, &*tx)
                 .await?;
 
             let mut model = channel.into_active_model();
             model.name = ActiveValue::Set(new_name.clone());
             let channel = model.update(&*tx).await?;
 
-            let participants = self
-                .get_channel_participant_details_internal(&channel, &*tx)
+            let channel_members = channel_member::Entity::find()
+                .filter(channel_member::Column::ChannelId.eq(channel.root_id()))
+                .all(&*tx)
                 .await?;
 
-            Ok(RenameChannelResult {
-                channel: Channel::from_model(channel.clone(), role),
-                participants_to_update: participants
-                    .iter()
-                    .map(|participant| {
-                        (
-                            participant.user_id,
-                            Channel::from_model(channel.clone(), participant.role),
-                        )
-                    })
-                    .collect(),
-            })
+            Ok((Channel::from_model(channel), channel_members))
         })
         .await
     }
@@ -565,10 +530,7 @@ impl Database {
 
             let channels = channels
                 .into_iter()
-                .filter_map(|channel| {
-                    let role = *role_for_channel.get(&channel.id)?;
-                    Some(Channel::from_model(channel, role))
-                })
+                .filter_map(|channel| Some(Channel::from_model(channel)))
                 .collect();
 
             Ok(channels)
@@ -576,6 +538,26 @@ impl Database {
         .await
     }
 
+    pub async fn get_channel_memberships(
+        &self,
+        user_id: UserId,
+    ) -> Result<(Vec<channel_member::Model>, Vec<channel::Model>)> {
+        self.transaction(|tx| async move {
+            let memberships = channel_member::Entity::find()
+                .filter(channel_member::Column::UserId.eq(user_id))
+                .all(&*tx)
+                .await?;
+            let channels = self
+                .get_channel_descendants_including_self(
+                    memberships.iter().map(|m| m.channel_id),
+                    &*tx,
+                )
+                .await?;
+            Ok((memberships, channels))
+        })
+        .await
+    }
+
     /// Returns all channels for the user with the given ID.
     pub async fn get_channels_for_user(&self, user_id: UserId) -> Result<ChannelsForUser> {
         self.transaction(|tx| async move {
@@ -594,12 +576,16 @@ impl Database {
         ancestor_channel: Option<&channel::Model>,
         tx: &DatabaseTransaction,
     ) -> Result<ChannelsForUser> {
+        let mut filter = channel_member::Column::UserId
+            .eq(user_id)
+            .and(channel_member::Column::Accepted.eq(true));
+
+        if let Some(ancestor) = ancestor_channel {
+            filter = filter.and(channel_member::Column::ChannelId.eq(ancestor.root_id()));
+        }
+
         let channel_memberships = channel_member::Entity::find()
-            .filter(
-                channel_member::Column::UserId
-                    .eq(user_id)
-                    .and(channel_member::Column::Accepted.eq(true)),
-            )
+            .filter(filter)
             .all(&*tx)
             .await?;
 
@@ -610,56 +596,20 @@ impl Database {
             )
             .await?;
 
-        let mut roles_by_channel_id: HashMap<ChannelId, ChannelRole> = HashMap::default();
-        for membership in channel_memberships.iter() {
-            roles_by_channel_id.insert(membership.channel_id, membership.role);
-        }
-
-        let mut visible_channel_ids: HashSet<ChannelId> = HashSet::default();
+        let roles_by_channel_id = channel_memberships
+            .iter()
+            .map(|membership| (membership.channel_id, membership.role))
+            .collect::<HashMap<_, _>>();
 
         let channels: Vec<Channel> = descendants
             .into_iter()
             .filter_map(|channel| {
-                let parent_role = channel
-                    .parent_id()
-                    .and_then(|parent_id| roles_by_channel_id.get(&parent_id));
-
-                let role = if let Some(parent_role) = parent_role {
-                    let role = if let Some(existing_role) = roles_by_channel_id.get(&channel.id) {
-                        existing_role.max(*parent_role)
-                    } else {
-                        *parent_role
-                    };
-                    roles_by_channel_id.insert(channel.id, role);
-                    role
+                let parent_role = roles_by_channel_id.get(&channel.root_id())?;
+                if parent_role.can_see_channel(channel.visibility) {
+                    Some(Channel::from_model(channel))
                 } else {
-                    *roles_by_channel_id.get(&channel.id)?
-                };
-
-                let can_see_parent_paths = role.can_see_all_descendants()
-                    || role.can_only_see_public_descendants()
-                        && channel.visibility == ChannelVisibility::Public;
-                if !can_see_parent_paths {
-                    return None;
-                }
-
-                visible_channel_ids.insert(channel.id);
-
-                if let Some(ancestor) = ancestor_channel {
-                    if !channel
-                        .ancestors_including_self()
-                        .any(|id| id == ancestor.id)
-                    {
-                        return None;
-                    }
+                    None
                 }
-
-                let mut channel = Channel::from_model(channel, role);
-                channel
-                    .parent_path
-                    .retain(|id| visible_channel_ids.contains(&id));
-
-                Some(channel)
             })
             .collect();
 
@@ -694,6 +644,7 @@ impl Database {
         let latest_messages = self.latest_channel_messages(&channel_ids, &*tx).await?;
 
         Ok(ChannelsForUser {
+            channel_memberships,
             channels,
             channel_participants,
             latest_buffer_versions,
@@ -701,73 +652,6 @@ impl Database {
         })
     }
 
-    pub async fn new_participants_to_notify(
-        &self,
-        parent_channel_id: ChannelId,
-    ) -> Result<Vec<(UserId, ChannelsForUser)>> {
-        self.weak_transaction(|tx| async move {
-            let parent_channel = self.get_channel_internal(parent_channel_id, &*tx).await?;
-            self.participants_to_notify_for_channel_change(&parent_channel, &*tx)
-                .await
-        })
-        .await
-    }
-
-    // TODO: this is very expensive, and we should rethink
-    async fn participants_to_notify_for_channel_change(
-        &self,
-        new_parent: &channel::Model,
-        tx: &DatabaseTransaction,
-    ) -> Result<Vec<(UserId, ChannelsForUser)>> {
-        let mut results: Vec<(UserId, ChannelsForUser)> = Vec::new();
-
-        let members = self
-            .get_channel_participant_details_internal(new_parent, &*tx)
-            .await?;
-
-        for member in members.iter() {
-            if !member.role.can_see_all_descendants() {
-                continue;
-            }
-            results.push((
-                member.user_id,
-                self.get_user_channels(member.user_id, Some(new_parent), &*tx)
-                    .await?,
-            ))
-        }
-
-        let public_parents = self
-            .public_ancestors_including_self(new_parent, &*tx)
-            .await?;
-        let public_parent = public_parents.last();
-
-        let Some(public_parent) = public_parent else {
-            return Ok(results);
-        };
-
-        // could save some time in the common case by skipping this if the
-        // new channel is not public and has no public descendants.
-        let public_members = if public_parent == new_parent {
-            members
-        } else {
-            self.get_channel_participant_details_internal(public_parent, &*tx)
-                .await?
-        };
-
-        for member in public_members {
-            if !member.role.can_only_see_public_descendants() {
-                continue;
-            };
-            results.push((
-                member.user_id,
-                self.get_user_channels(member.user_id, Some(public_parent), &*tx)
-                    .await?,
-            ))
-        }
-
-        Ok(results)
-    }
-
     /// Sets the role for the specified channel member.
     pub async fn set_channel_member_role(
         &self,
@@ -805,7 +689,7 @@ impl Database {
                 ))
             } else {
                 Ok(SetMemberRoleResult::InviteUpdated(Channel::from_model(
-                    channel, role,
+                    channel,
                 )))
             }
         })
@@ -835,22 +719,30 @@ impl Database {
         if role == ChannelRole::Admin {
             Ok(members
                 .into_iter()
-                .map(|channel_member| channel_member.to_proto())
+                .map(|channel_member| proto::ChannelMember {
+                    role: channel_member.role.into(),
+                    user_id: channel_member.user_id.to_proto(),
+                    kind: if channel_member.accepted {
+                        Kind::Member
+                    } else {
+                        Kind::Invitee
+                    }
+                    .into(),
+                })
                 .collect())
         } else {
             return Ok(members
                 .into_iter()
                 .filter_map(|member| {
-                    if member.kind == proto::channel_member::Kind::Invitee {
+                    if !member.accepted {
                         return None;
                     }
-                    Some(ChannelMember {
-                        role: member.role,
-                        user_id: member.user_id,
-                        kind: proto::channel_member::Kind::Member,
+                    Some(proto::ChannelMember {
+                        role: member.role.into(),
+                        user_id: member.user_id.to_proto(),
+                        kind: Kind::Member.into(),
                     })
                 })
-                .map(|channel_member| channel_member.to_proto())
                 .collect());
         }
     }
@@ -859,83 +751,11 @@ impl Database {
         &self,
         channel: &channel::Model,
         tx: &DatabaseTransaction,
-    ) -> Result<Vec<ChannelMember>> {
-        #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)]
-        enum QueryMemberDetails {
-            UserId,
-            Role,
-            IsDirectMember,
-            Accepted,
-            Visibility,
-        }
-
-        let mut stream = channel_member::Entity::find()
-            .left_join(channel::Entity)
-            .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self()))
-            .select_only()
-            .column(channel_member::Column::UserId)
-            .column(channel_member::Column::Role)
-            .column_as(
-                channel_member::Column::ChannelId.eq(channel.id),
-                QueryMemberDetails::IsDirectMember,
-            )
-            .column(channel_member::Column::Accepted)
-            .column(channel::Column::Visibility)
-            .into_values::<_, QueryMemberDetails>()
-            .stream(&*tx)
-            .await?;
-
-        let mut user_details: HashMap<UserId, ChannelMember> = HashMap::default();
-
-        while let Some(user_membership) = stream.next().await {
-            let (user_id, channel_role, is_direct_member, is_invite_accepted, visibility): (
-                UserId,
-                ChannelRole,
-                bool,
-                bool,
-                ChannelVisibility,
-            ) = user_membership?;
-            let kind = match (is_direct_member, is_invite_accepted) {
-                (true, true) => proto::channel_member::Kind::Member,
-                (true, false) => proto::channel_member::Kind::Invitee,
-                (false, true) => proto::channel_member::Kind::AncestorMember,
-                (false, false) => continue,
-            };
-
-            if channel_role == ChannelRole::Guest
-                && visibility != ChannelVisibility::Public
-                && channel.visibility != ChannelVisibility::Public
-            {
-                continue;
-            }
-
-            if let Some(details_mut) = user_details.get_mut(&user_id) {
-                if channel_role.should_override(details_mut.role) {
-                    details_mut.role = channel_role;
-                }
-                if kind == Kind::Member {
-                    details_mut.kind = kind;
-                // the UI is going to be a bit confusing if you already have permissions
-                // that are greater than or equal to the ones you're being invited to.
-                } else if kind == Kind::Invitee && details_mut.kind == Kind::AncestorMember {
-                    details_mut.kind = kind;
-                }
-            } else {
-                user_details.insert(
-                    user_id,
-                    ChannelMember {
-                        user_id,
-                        kind,
-                        role: channel_role,
-                    },
-                );
-            }
-        }
-
-        Ok(user_details
-            .into_iter()
-            .map(|(_, details)| details)
-            .collect())
+    ) -> Result<Vec<channel_member::Model>> {
+        Ok(channel_member::Entity::find()
+            .filter(channel_member::Column::ChannelId.eq(channel.root_id()))
+            .all(tx)
+            .await?)
     }
 
     /// Returns the participants in the given channel.
@@ -1014,7 +834,7 @@ impl Database {
         tx: &DatabaseTransaction,
     ) -> Result<Option<channel_member::Model>> {
         let row = channel_member::Entity::find()
-            .filter(channel_member::Column::ChannelId.is_in(channel.ancestors_including_self()))
+            .filter(channel_member::Column::ChannelId.eq(channel.root_id()))
             .filter(channel_member::Column::UserId.eq(user_id))
             .filter(channel_member::Column::Accepted.eq(false))
             .one(&*tx)
@@ -1023,33 +843,6 @@ impl Database {
         Ok(row)
     }
 
-    async fn public_parent_channel(
-        &self,
-        channel: &channel::Model,
-        tx: &DatabaseTransaction,
-    ) -> Result<Option<channel::Model>> {
-        let mut path = self.public_ancestors_including_self(channel, &*tx).await?;
-        if path.last().unwrap().id == channel.id {
-            path.pop();
-        }
-        Ok(path.pop())
-    }
-
-    pub(crate) async fn public_ancestors_including_self(
-        &self,
-        channel: &channel::Model,
-        tx: &DatabaseTransaction,
-    ) -> Result<Vec<channel::Model>> {
-        let visible_channels = channel::Entity::find()
-            .filter(channel::Column::Id.is_in(channel.ancestors_including_self()))
-            .filter(channel::Column::Visibility.eq(ChannelVisibility::Public))
-            .order_by_asc(channel::Column::ParentPath)
-            .all(&*tx)
-            .await?;
-
-        Ok(visible_channels)
-    }
-
     /// Returns the role for a user in the given channel.
     pub async fn channel_role_for_user(
         &self,
@@ -1057,77 +850,25 @@ impl Database {
         user_id: UserId,
         tx: &DatabaseTransaction,
     ) -> Result<Option<ChannelRole>> {
-        #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)]
-        enum QueryChannelMembership {
-            ChannelId,
-            Role,
-            Visibility,
-        }
-
-        let mut rows = channel_member::Entity::find()
-            .left_join(channel::Entity)
+        let membership = channel_member::Entity::find()
             .filter(
                 channel_member::Column::ChannelId
-                    .is_in(channel.ancestors_including_self())
+                    .eq(channel.root_id())
                     .and(channel_member::Column::UserId.eq(user_id))
                     .and(channel_member::Column::Accepted.eq(true)),
             )
-            .select_only()
-            .column(channel_member::Column::ChannelId)
-            .column(channel_member::Column::Role)
-            .column(channel::Column::Visibility)
-            .into_values::<_, QueryChannelMembership>()
-            .stream(&*tx)
+            .one(&*tx)
             .await?;
 
-        let mut user_role: Option<ChannelRole> = None;
-
-        let mut is_participant = false;
-        let mut current_channel_visibility = None;
-
-        // note these channels are not iterated in any particular order,
-        // our current logic takes the highest permission available.
-        while let Some(row) = rows.next().await {
-            let (membership_channel, role, visibility): (
-                ChannelId,
-                ChannelRole,
-                ChannelVisibility,
-            ) = row?;
+        let Some(membership) = membership else {
+            return Ok(None);
+        };
 
-            match role {
-                ChannelRole::Admin | ChannelRole::Member | ChannelRole::Banned => {
-                    if let Some(users_role) = user_role {
-                        user_role = Some(users_role.max(role));
-                    } else {
-                        user_role = Some(role)
-                    }
-                }
-                ChannelRole::Guest if visibility == ChannelVisibility::Public => {
-                    is_participant = true
-                }
-                ChannelRole::Guest => {}
-            }
-            if channel.id == membership_channel {
-                current_channel_visibility = Some(visibility);
-            }
-        }
-        // free up database connection
-        drop(rows);
-
-        if is_participant && user_role.is_none() {
-            if current_channel_visibility.is_none() {
-                current_channel_visibility = channel::Entity::find()
-                    .filter(channel::Column::Id.eq(channel.id))
-                    .one(&*tx)
-                    .await?
-                    .map(|channel| channel.visibility);
-            }
-            if current_channel_visibility == Some(ChannelVisibility::Public) {
-                user_role = Some(ChannelRole::Guest);
-            }
+        if !membership.role.can_see_channel(channel.visibility) {
+            return Ok(None);
         }
 
-        Ok(user_role)
+        Ok(Some(membership.role))
     }
 
     // Get the descendants of the given set if channels, ordered by their
@@ -1180,11 +921,10 @@ impl Database {
     pub async fn get_channel(&self, channel_id: ChannelId, user_id: UserId) -> Result<Channel> {
         self.transaction(|tx| async move {
             let channel = self.get_channel_internal(channel_id, &*tx).await?;
-            let role = self
-                .check_user_is_channel_participant(&channel, user_id, &*tx)
+            self.check_user_is_channel_participant(&channel, user_id, &*tx)
                 .await?;
 
-            Ok(Channel::from_model(channel, role))
+            Ok(Channel::from_model(channel))
         })
         .await
     }
@@ -1241,61 +981,39 @@ impl Database {
     pub async fn move_channel(
         &self,
         channel_id: ChannelId,
-        new_parent_id: Option<ChannelId>,
+        new_parent_id: ChannelId,
         admin_id: UserId,
-    ) -> Result<Option<MoveChannelResult>> {
+    ) -> Result<(Vec<Channel>, Vec<channel_member::Model>)> {
         self.transaction(|tx| async move {
             let channel = self.get_channel_internal(channel_id, &*tx).await?;
             self.check_user_is_channel_admin(&channel, admin_id, &*tx)
                 .await?;
+            let new_parent = self.get_channel_internal(new_parent_id, &*tx).await?;
 
-            let new_parent_path;
-            let new_parent_channel;
-            if let Some(new_parent_id) = new_parent_id {
-                let new_parent = self.get_channel_internal(new_parent_id, &*tx).await?;
-                self.check_user_is_channel_admin(&new_parent, admin_id, &*tx)
-                    .await?;
-
-                if new_parent
-                    .ancestors_including_self()
-                    .any(|id| id == channel.id)
-                {
-                    Err(anyhow!("cannot move a channel into one of its descendants"))?;
-                }
+            if new_parent.root_id() != channel.root_id() {
+                Err(anyhow!("cannot move a channel into a different root"))?;
+            }
 
-                new_parent_path = new_parent.path();
-                new_parent_channel = Some(new_parent);
-            } else {
-                new_parent_path = String::new();
-                new_parent_channel = None;
-            };
+            if new_parent
+                .ancestors_including_self()
+                .any(|id| id == channel.id)
+            {
+                Err(anyhow!("cannot move a channel into one of its descendants"))?;
+            }
 
-            let previous_participants = self
-                .get_channel_participant_details_internal(&channel, &*tx)
-                .await?;
+            if channel.visibility == ChannelVisibility::Public
+                && new_parent.visibility != ChannelVisibility::Public
+            {
+                Err(anyhow!("public channels must descend from public channels"))?;
+            }
 
+            let root_id = channel.root_id();
             let old_path = format!("{}{}/", channel.parent_path, channel.id);
-            let new_path = format!("{}{}/", new_parent_path, channel.id);
-
-            if old_path == new_path {
-                return Ok(None);
-            }
+            let new_path = format!("{}{}/", new_parent.path(), channel.id);
 
             let mut model = channel.into_active_model();
-            model.parent_path = ActiveValue::Set(new_parent_path);
-            model.update(&*tx).await?;
-
-            if new_parent_channel.is_none() {
-                channel_member::ActiveModel {
-                    id: ActiveValue::NotSet,
-                    channel_id: ActiveValue::Set(channel_id),
-                    user_id: ActiveValue::Set(admin_id),
-                    accepted: ActiveValue::Set(true),
-                    role: ActiveValue::Set(ChannelRole::Admin),
-                }
-                .insert(&*tx)
-                .await?;
-            }
+            model.parent_path = ActiveValue::Set(new_parent.path());
+            let channel = model.update(&*tx).await?;
 
             let descendent_ids =
                 ChannelId::find_by_statement::<QueryIds>(Statement::from_sql_and_values(
@@ -1310,10 +1028,22 @@ impl Database {
                 .all(&*tx)
                 .await?;
 
-            Ok(Some(MoveChannelResult {
-                previous_participants,
-                descendent_ids,
-            }))
+            let all_moved_ids = Some(channel.id).into_iter().chain(descendent_ids);
+
+            let channels = channel::Entity::find()
+                .filter(channel::Column::Id.is_in(all_moved_ids))
+                .all(&*tx)
+                .await?
+                .into_iter()
+                .map(|c| Channel::from_model(c))
+                .collect::<Vec<_>>();
+
+            let channel_members = channel_member::Entity::find()
+                .filter(channel_member::Column::ChannelId.eq(root_id))
+                .all(&*tx)
+                .await?;
+
+            Ok((channels, channel_members))
         })
         .await
     }

crates/collab/src/db/tables/channel.rs 🔗

@@ -17,6 +17,14 @@ impl Model {
         self.ancestors().last()
     }
 
+    pub fn is_root(&self) -> bool {
+        self.parent_path.is_empty()
+    }
+
+    pub fn root_id(&self) -> ChannelId {
+        self.ancestors().next().unwrap_or(self.id)
+    }
+
     pub fn ancestors(&self) -> impl Iterator<Item = ChannelId> + '_ {
         self.parent_path
             .trim_end_matches('/')

crates/collab/src/db/tests.rs 🔗

@@ -150,14 +150,13 @@ impl Drop for TestDb {
     }
 }
 
-fn channel_tree(channels: &[(ChannelId, &[ChannelId], &'static str, ChannelRole)]) -> Vec<Channel> {
+fn channel_tree(channels: &[(ChannelId, &[ChannelId], &'static str)]) -> Vec<Channel> {
     channels
         .iter()
-        .map(|(id, parent_path, name, role)| Channel {
+        .map(|(id, parent_path, name)| Channel {
             id: *id,
             name: name.to_string(),
             visibility: ChannelVisibility::Members,
-            role: *role,
             parent_path: parent_path.to_vec(),
         })
         .collect()

crates/collab/src/db/tests/channel_tests.rs 🔗

@@ -62,23 +62,13 @@ async fn test_channels(db: &Arc<Database>) {
     assert_eq!(
         result.channels,
         channel_tree(&[
-            (zed_id, &[], "zed", ChannelRole::Admin),
-            (crdb_id, &[zed_id], "crdb", ChannelRole::Admin),
-            (
-                livestreaming_id,
-                &[zed_id],
-                "livestreaming",
-                ChannelRole::Admin
-            ),
-            (replace_id, &[zed_id], "replace", ChannelRole::Admin),
-            (rust_id, &[], "rust", ChannelRole::Admin),
-            (cargo_id, &[rust_id], "cargo", ChannelRole::Admin),
-            (
-                cargo_ra_id,
-                &[rust_id, cargo_id],
-                "cargo-ra",
-                ChannelRole::Admin
-            )
+            (zed_id, &[], "zed"),
+            (crdb_id, &[zed_id], "crdb"),
+            (livestreaming_id, &[zed_id], "livestreaming",),
+            (replace_id, &[zed_id], "replace"),
+            (rust_id, &[], "rust"),
+            (cargo_id, &[rust_id], "cargo"),
+            (cargo_ra_id, &[rust_id, cargo_id], "cargo-ra",)
         ],)
     );
 
@@ -86,15 +76,10 @@ async fn test_channels(db: &Arc<Database>) {
     assert_eq!(
         result.channels,
         channel_tree(&[
-            (zed_id, &[], "zed", ChannelRole::Member),
-            (crdb_id, &[zed_id], "crdb", ChannelRole::Member),
-            (
-                livestreaming_id,
-                &[zed_id],
-                "livestreaming",
-                ChannelRole::Member
-            ),
-            (replace_id, &[zed_id], "replace", ChannelRole::Member)
+            (zed_id, &[], "zed"),
+            (crdb_id, &[zed_id], "crdb"),
+            (livestreaming_id, &[zed_id], "livestreaming",),
+            (replace_id, &[zed_id], "replace")
         ],)
     );
 
@@ -112,15 +97,10 @@ async fn test_channels(db: &Arc<Database>) {
     assert_eq!(
         result.channels,
         channel_tree(&[
-            (zed_id, &[], "zed", ChannelRole::Admin),
-            (crdb_id, &[zed_id], "crdb", ChannelRole::Admin),
-            (
-                livestreaming_id,
-                &[zed_id],
-                "livestreaming",
-                ChannelRole::Admin
-            ),
-            (replace_id, &[zed_id], "replace", ChannelRole::Admin)
+            (zed_id, &[], "zed"),
+            (crdb_id, &[zed_id], "crdb"),
+            (livestreaming_id, &[zed_id], "livestreaming",),
+            (replace_id, &[zed_id], "replace")
         ],)
     );
 
@@ -271,14 +251,19 @@ async fn test_channel_invites(db: &Arc<Database>) {
         &[
             proto::ChannelMember {
                 user_id: user_1.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
                 user_id: user_2.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Member.into(),
             },
+            proto::ChannelMember {
+                user_id: user_3.to_proto(),
+                kind: proto::channel_member::Kind::Invitee.into(),
+                role: proto::ChannelRole::Admin.into(),
+            },
         ]
     );
 }
@@ -420,13 +405,6 @@ async fn test_db_channel_moving_bugs(db: &Arc<Database>) {
         .await
         .unwrap();
 
-    // Move to same parent should be a no-op
-    assert!(db
-        .move_channel(projects_id, Some(zed_id), user_id)
-        .await
-        .unwrap()
-        .is_none());
-
     let result = db.get_channels_for_user(user_id).await.unwrap();
     assert_channel_tree(
         result.channels,
@@ -437,20 +415,8 @@ async fn test_db_channel_moving_bugs(db: &Arc<Database>) {
         ],
     );
 
-    // Move the project channel to the root
-    db.move_channel(projects_id, None, user_id).await.unwrap();
-    let result = db.get_channels_for_user(user_id).await.unwrap();
-    assert_channel_tree(
-        result.channels,
-        &[
-            (zed_id, &[]),
-            (projects_id, &[]),
-            (livestreaming_id, &[projects_id]),
-        ],
-    );
-
     // Can't move a channel into its ancestor
-    db.move_channel(projects_id, Some(livestreaming_id), user_id)
+    db.move_channel(projects_id, livestreaming_id, user_id)
         .await
         .unwrap_err();
     let result = db.get_channels_for_user(user_id).await.unwrap();
@@ -458,8 +424,8 @@ async fn test_db_channel_moving_bugs(db: &Arc<Database>) {
         result.channels,
         &[
             (zed_id, &[]),
-            (projects_id, &[]),
-            (livestreaming_id, &[projects_id]),
+            (projects_id, &[zed_id]),
+            (livestreaming_id, &[zed_id, projects_id]),
         ],
     );
 }
@@ -476,32 +442,39 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     let guest = new_test_user(db, "guest@example.com").await;
 
     let zed_channel = db.create_root_channel("zed", admin).await.unwrap();
-    let active_channel_id = db
+    let internal_channel_id = db
         .create_sub_channel("active", zed_channel, admin)
         .await
         .unwrap();
-    let vim_channel_id = db
-        .create_sub_channel("vim", active_channel_id, admin)
+    let public_channel_id = db
+        .create_sub_channel("vim", zed_channel, admin)
         .await
         .unwrap();
 
-    db.set_channel_visibility(vim_channel_id, crate::db::ChannelVisibility::Public, admin)
+    db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
         .await
         .unwrap();
-    db.invite_channel_member(active_channel_id, member, admin, ChannelRole::Member)
+    db.set_channel_visibility(
+        public_channel_id,
+        crate::db::ChannelVisibility::Public,
+        admin,
+    )
+    .await
+    .unwrap();
+    db.invite_channel_member(zed_channel, member, admin, ChannelRole::Member)
         .await
         .unwrap();
-    db.invite_channel_member(vim_channel_id, guest, admin, ChannelRole::Guest)
+    db.invite_channel_member(zed_channel, guest, admin, ChannelRole::Guest)
         .await
         .unwrap();
 
-    db.respond_to_channel_invite(active_channel_id, member, true)
+    db.respond_to_channel_invite(zed_channel, member, true)
         .await
         .unwrap();
 
     db.transaction(|tx| async move {
         db.check_user_is_channel_participant(
-            &db.get_channel_internal(vim_channel_id, &*tx).await?,
+            &db.get_channel_internal(public_channel_id, &*tx).await?,
             admin,
             &*tx,
         )
@@ -511,7 +484,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     .unwrap();
     db.transaction(|tx| async move {
         db.check_user_is_channel_participant(
-            &db.get_channel_internal(vim_channel_id, &*tx).await?,
+            &db.get_channel_internal(public_channel_id, &*tx).await?,
             member,
             &*tx,
         )
@@ -521,7 +494,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     .unwrap();
 
     let mut members = db
-        .get_channel_participant_details(vim_channel_id, admin)
+        .get_channel_participant_details(public_channel_id, admin)
         .await
         .unwrap();
 
@@ -532,12 +505,12 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         &[
             proto::ChannelMember {
                 user_id: admin.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
                 user_id: member.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Member.into(),
             },
             proto::ChannelMember {
@@ -548,13 +521,13 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         ]
     );
 
-    db.respond_to_channel_invite(vim_channel_id, guest, true)
+    db.respond_to_channel_invite(zed_channel, guest, true)
         .await
         .unwrap();
 
     db.transaction(|tx| async move {
         db.check_user_is_channel_participant(
-            &db.get_channel_internal(vim_channel_id, &*tx).await?,
+            &db.get_channel_internal(public_channel_id, &*tx).await?,
             guest,
             &*tx,
         )
@@ -564,23 +537,29 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     .unwrap();
 
     let channels = db.get_channels_for_user(guest).await.unwrap().channels;
-    assert_channel_tree(channels, &[(vim_channel_id, &[])]);
+    assert_channel_tree(
+        channels,
+        &[(zed_channel, &[]), (public_channel_id, &[zed_channel])],
+    );
     let channels = db.get_channels_for_user(member).await.unwrap().channels;
     assert_channel_tree(
         channels,
         &[
-            (active_channel_id, &[]),
-            (vim_channel_id, &[active_channel_id]),
+            (zed_channel, &[]),
+            (internal_channel_id, &[zed_channel]),
+            (public_channel_id, &[zed_channel]),
         ],
     );
 
-    db.set_channel_member_role(vim_channel_id, admin, guest, ChannelRole::Banned)
+    db.set_channel_member_role(zed_channel, admin, guest, ChannelRole::Banned)
         .await
         .unwrap();
     assert!(db
         .transaction(|tx| async move {
             db.check_user_is_channel_participant(
-                &db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(),
+                &db.get_channel_internal(public_channel_id, &*tx)
+                    .await
+                    .unwrap(),
                 guest,
                 &*tx,
             )
@@ -590,7 +569,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         .is_err());
 
     let mut members = db
-        .get_channel_participant_details(vim_channel_id, admin)
+        .get_channel_participant_details(public_channel_id, admin)
         .await
         .unwrap();
 
@@ -601,12 +580,12 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         &[
             proto::ChannelMember {
                 user_id: admin.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
                 user_id: member.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Member.into(),
             },
             proto::ChannelMember {
@@ -617,11 +596,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         ]
     );
 
-    db.remove_channel_member(vim_channel_id, guest, admin)
-        .await
-        .unwrap();
-
-    db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
+    db.remove_channel_member(zed_channel, guest, admin)
         .await
         .unwrap();
 
@@ -631,7 +606,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
 
     // currently people invited to parent channels are not shown here
     let mut members = db
-        .get_channel_participant_details(vim_channel_id, admin)
+        .get_channel_participant_details(public_channel_id, admin)
         .await
         .unwrap();
 
@@ -642,14 +617,19 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         &[
             proto::ChannelMember {
                 user_id: admin.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
                 user_id: member.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Member.into(),
             },
+            proto::ChannelMember {
+                user_id: guest.to_proto(),
+                kind: proto::channel_member::Kind::Invitee.into(),
+                role: proto::ChannelRole::Guest.into(),
+            },
         ]
     );
 
@@ -670,7 +650,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     assert!(db
         .transaction(|tx| async move {
             db.check_user_is_channel_participant(
-                &db.get_channel_internal(active_channel_id, &*tx)
+                &db.get_channel_internal(internal_channel_id, &*tx)
                     .await
                     .unwrap(),
                 guest,
@@ -683,7 +663,9 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
 
     db.transaction(|tx| async move {
         db.check_user_is_channel_participant(
-            &db.get_channel_internal(vim_channel_id, &*tx).await.unwrap(),
+            &db.get_channel_internal(public_channel_id, &*tx)
+                .await
+                .unwrap(),
             guest,
             &*tx,
         )
@@ -693,7 +675,7 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     .unwrap();
 
     let mut members = db
-        .get_channel_participant_details(vim_channel_id, admin)
+        .get_channel_participant_details(public_channel_id, admin)
         .await
         .unwrap();
 
@@ -704,17 +686,17 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
         &[
             proto::ChannelMember {
                 user_id: admin.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Admin.into(),
             },
             proto::ChannelMember {
                 user_id: member.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Member.into(),
             },
             proto::ChannelMember {
                 user_id: guest.to_proto(),
-                kind: proto::channel_member::Kind::AncestorMember.into(),
+                kind: proto::channel_member::Kind::Member.into(),
                 role: proto::ChannelRole::Guest.into(),
             },
         ]
@@ -723,67 +705,10 @@ async fn test_user_is_channel_participant(db: &Arc<Database>) {
     let channels = db.get_channels_for_user(guest).await.unwrap().channels;
     assert_channel_tree(
         channels,
-        &[(zed_channel, &[]), (vim_channel_id, &[zed_channel])],
+        &[(zed_channel, &[]), (public_channel_id, &[zed_channel])],
     )
 }
 
-test_both_dbs!(
-    test_user_joins_correct_channel,
-    test_user_joins_correct_channel_postgres,
-    test_user_joins_correct_channel_sqlite
-);
-
-async fn test_user_joins_correct_channel(db: &Arc<Database>) {
-    let admin = new_test_user(db, "admin@example.com").await;
-
-    let zed_channel = db.create_root_channel("zed", admin).await.unwrap();
-
-    let active_channel = db
-        .create_sub_channel("active", zed_channel, admin)
-        .await
-        .unwrap();
-
-    let vim_channel = db
-        .create_sub_channel("vim", active_channel, admin)
-        .await
-        .unwrap();
-
-    let vim2_channel = db
-        .create_sub_channel("vim2", vim_channel, admin)
-        .await
-        .unwrap();
-
-    db.set_channel_visibility(zed_channel, crate::db::ChannelVisibility::Public, admin)
-        .await
-        .unwrap();
-
-    db.set_channel_visibility(vim_channel, crate::db::ChannelVisibility::Public, admin)
-        .await
-        .unwrap();
-
-    db.set_channel_visibility(vim2_channel, crate::db::ChannelVisibility::Public, admin)
-        .await
-        .unwrap();
-
-    let most_public = db
-        .transaction(|tx| async move {
-            Ok(db
-                .public_ancestors_including_self(
-                    &db.get_channel_internal(vim_channel, &*tx).await.unwrap(),
-                    &tx,
-                )
-                .await?
-                .first()
-                .cloned())
-        })
-        .await
-        .unwrap()
-        .unwrap()
-        .id;
-
-    assert_eq!(most_public, zed_channel)
-}
-
 test_both_dbs!(
     test_guest_access,
     test_guest_access_postgres,

crates/collab/src/db/tests/message_tests.rs 🔗

@@ -15,7 +15,7 @@ test_both_dbs!(
 
 async fn test_channel_message_retrieval(db: &Arc<Database>) {
     let user = new_test_user(db, "user@example.com").await;
-    let channel = db.create_channel("channel", None, user).await.unwrap();
+    let channel = db.create_channel("channel", None, user).await.unwrap().0;
 
     let owner_id = db.create_server("test").await.unwrap().0 as u32;
     db.join_channel_chat(channel.id, rpc::ConnectionId { owner_id, id: 0 }, user)
@@ -291,7 +291,12 @@ async fn test_channel_message_mentions(db: &Arc<Database>) {
     let user_b = new_test_user(db, "user_b@example.com").await;
     let user_c = new_test_user(db, "user_c@example.com").await;
 
-    let channel = db.create_channel("channel", None, user_a).await.unwrap().id;
+    let channel = db
+        .create_channel("channel", None, user_a)
+        .await
+        .unwrap()
+        .0
+        .id;
     db.invite_channel_member(channel, user_b, user_a, ChannelRole::Member)
         .await
         .unwrap();

crates/collab/src/rpc.rs 🔗

@@ -5,8 +5,7 @@ use crate::{
     db::{
         self, BufferId, ChannelId, ChannelRole, ChannelsForUser, CreatedChannelMessage, Database,
         InviteMemberResult, MembershipUpdated, MessageId, NotificationId, ProjectId,
-        RemoveChannelMemberResult, RenameChannelResult, RespondToChannelInvite, RoomId, ServerId,
-        SetChannelVisibilityResult, User, UserId,
+        RemoveChannelMemberResult, RespondToChannelInvite, RoomId, ServerId, User, UserId,
     },
     executor::Executor,
     AppState, Error, Result,
@@ -602,6 +601,7 @@ impl Server {
                 let mut pool = this.connection_pool.lock();
                 pool.add_connection(connection_id, user_id, user.admin);
                 this.peer.send(connection_id, build_initial_contacts_update(contacts, &pool))?;
+                this.peer.send(connection_id, build_update_user_channels(&channels_for_user.channel_memberships))?;
                 this.peer.send(connection_id, build_channels_update(
                     channels_for_user,
                     channel_invites
@@ -2300,7 +2300,7 @@ async fn create_channel(
     let db = session.db().await;
 
     let parent_id = request.parent_id.map(|id| ChannelId::from_proto(id));
-    let channel = db
+    let (channel, owner, channel_members) = db
         .create_channel(&request.name, parent_id, session.user_id)
         .await?;
 
@@ -2309,20 +2309,30 @@ async fn create_channel(
         parent_id: request.parent_id,
     })?;
 
-    let participants_to_update;
-    if let Some(parent) = parent_id {
-        participants_to_update = db.new_participants_to_notify(parent).await?;
-    } else {
-        participants_to_update = vec![];
+    let connection_pool = session.connection_pool().await;
+    if let Some(owner) = owner {
+        let update = proto::UpdateUserChannels {
+            channel_memberships: vec![proto::ChannelMembership {
+                channel_id: owner.channel_id.to_proto(),
+                role: owner.role.into(),
+            }],
+            ..Default::default()
+        };
+        for connection_id in connection_pool.user_connection_ids(owner.user_id) {
+            session.peer.send(connection_id, update.clone())?;
+        }
     }
 
-    let connection_pool = session.connection_pool().await;
-    for (user_id, channels) in participants_to_update {
-        let update = build_channels_update(channels, vec![]);
-        for connection_id in connection_pool.user_connection_ids(user_id) {
-            if user_id == session.user_id {
-                continue;
-            }
+    for channel_member in channel_members {
+        if !channel_member.role.can_see_channel(channel.visibility) {
+            continue;
+        }
+
+        let update = proto::UpdateChannels {
+            channels: vec![channel.to_proto()],
+            ..Default::default()
+        };
+        for connection_id in connection_pool.user_connection_ids(channel_member.user_id) {
             session.peer.send(connection_id, update.clone())?;
         }
     }
@@ -2439,7 +2449,9 @@ async fn remove_channel_member(
     Ok(())
 }
 
-/// Toggle the channel between public and private
+/// Toggle the channel between public and private.
+/// Care is taken to maintain the invariant that public channels only descend from public channels,
+/// (though members-only channels can appear at any point in the heirarchy).
 async fn set_channel_visibility(
     request: proto::SetChannelVisibility,
     response: Response<proto::SetChannelVisibility>,
@@ -2449,27 +2461,25 @@ async fn set_channel_visibility(
     let channel_id = ChannelId::from_proto(request.channel_id);
     let visibility = request.visibility().into();
 
-    let SetChannelVisibilityResult {
-        participants_to_update,
-        participants_to_remove,
-        channels_to_remove,
-    } = db
+    let (channel, channel_members) = db
         .set_channel_visibility(channel_id, visibility, session.user_id)
         .await?;
 
     let connection_pool = session.connection_pool().await;
-    for (user_id, channels) in participants_to_update {
-        let update = build_channels_update(channels, vec![]);
-        for connection_id in connection_pool.user_connection_ids(user_id) {
-            session.peer.send(connection_id, update.clone())?;
-        }
-    }
-    for user_id in participants_to_remove {
-        let update = proto::UpdateChannels {
-            delete_channels: channels_to_remove.iter().map(|id| id.to_proto()).collect(),
-            ..Default::default()
+    for member in channel_members {
+        let update = if member.role.can_see_channel(channel.visibility) {
+            proto::UpdateChannels {
+                channels: vec![channel.to_proto()],
+                ..Default::default()
+            }
+        } else {
+            proto::UpdateChannels {
+                delete_channels: vec![channel.id.to_proto()],
+                ..Default::default()
+            }
         };
-        for connection_id in connection_pool.user_connection_ids(user_id) {
+
+        for connection_id in connection_pool.user_connection_ids(member.user_id) {
             session.peer.send(connection_id, update.clone())?;
         }
     }
@@ -2478,7 +2488,7 @@ async fn set_channel_visibility(
     Ok(())
 }
 
-/// Alter the role for a user in the channel
+/// Alter the role for a user in the channel.
 async fn set_channel_member_role(
     request: proto::SetChannelMemberRole,
     response: Response<proto::SetChannelMemberRole>,
@@ -2534,10 +2544,7 @@ async fn rename_channel(
 ) -> Result<()> {
     let db = session.db().await;
     let channel_id = ChannelId::from_proto(request.channel_id);
-    let RenameChannelResult {
-        channel,
-        participants_to_update,
-    } = db
+    let (channel, channel_members) = db
         .rename_channel(channel_id, session.user_id, &request.name)
         .await?;
 
@@ -2546,13 +2553,15 @@ async fn rename_channel(
     })?;
 
     let connection_pool = session.connection_pool().await;
-    for (user_id, channel) in participants_to_update {
-        for connection_id in connection_pool.user_connection_ids(user_id) {
-            let update = proto::UpdateChannels {
-                channels: vec![channel.to_proto()],
-                ..Default::default()
-            };
-
+    for channel_member in channel_members {
+        if !channel_member.role.can_see_channel(channel.visibility) {
+            continue;
+        }
+        let update = proto::UpdateChannels {
+            channels: vec![channel.to_proto()],
+            ..Default::default()
+        };
+        for connection_id in connection_pool.user_connection_ids(channel_member.user_id) {
             session.peer.send(connection_id, update.clone())?;
         }
     }
@@ -2567,57 +2576,38 @@ async fn move_channel(
     session: Session,
 ) -> Result<()> {
     let channel_id = ChannelId::from_proto(request.channel_id);
-    let to = request.to.map(ChannelId::from_proto);
+    let to = ChannelId::from_proto(request.to);
 
-    let result = session
+    let (channels, channel_members) = session
         .db()
         .await
         .move_channel(channel_id, to, session.user_id)
         .await?;
 
-    if let Some(result) = result {
-        let participants_to_update: HashMap<_, _> = session
-            .db()
-            .await
-            .new_participants_to_notify(to.unwrap_or(channel_id))
-            .await?
-            .into_iter()
-            .collect();
-
-        let mut moved_channels: HashSet<ChannelId> = HashSet::default();
-        for id in result.descendent_ids {
-            moved_channels.insert(id);
-        }
-        moved_channels.insert(channel_id);
-
-        let mut participants_to_remove: HashSet<UserId> = HashSet::default();
-        for participant in result.previous_participants {
-            if participant.kind == proto::channel_member::Kind::AncestorMember {
-                if !participants_to_update.contains_key(&participant.user_id) {
-                    participants_to_remove.insert(participant.user_id);
+    let connection_pool = session.connection_pool().await;
+    for member in channel_members {
+        let channels = channels
+            .iter()
+            .filter_map(|channel| {
+                if member.role.can_see_channel(channel.visibility) {
+                    Some(channel.to_proto())
+                } else {
+                    None
                 }
-            }
+            })
+            .collect::<Vec<_>>();
+        if channels.is_empty() {
+            continue;
         }
 
-        let moved_channels: Vec<u64> = moved_channels.iter().map(|id| id.to_proto()).collect();
-
-        let connection_pool = session.connection_pool().await;
-        for (user_id, channels) in participants_to_update {
-            let mut update = build_channels_update(channels, vec![]);
-            update.delete_channels = moved_channels.clone();
-            for connection_id in connection_pool.user_connection_ids(user_id) {
-                session.peer.send(connection_id, update.clone())?;
-            }
-        }
+        let update = proto::UpdateChannels {
+            channels,
+            ..Default::default()
+        };
+        dbg!(&member, &update);
 
-        for user_id in participants_to_remove {
-            let update = proto::UpdateChannels {
-                delete_channels: moved_channels.clone(),
-                ..Default::default()
-            };
-            for connection_id in connection_pool.user_connection_ids(user_id) {
-                session.peer.send(connection_id, update.clone())?;
-            }
+        for connection_id in connection_pool.user_connection_ids(member.user_id) {
+            session.peer.send(connection_id, update.clone())?;
         }
     }
 
@@ -3305,6 +3295,21 @@ fn notify_membership_updated(
     }
 }
 
+fn build_update_user_channels(
+    memberships: &Vec<db::channel_member::Model>,
+) -> proto::UpdateUserChannels {
+    proto::UpdateUserChannels {
+        channel_memberships: memberships
+            .iter()
+            .map(|m| proto::ChannelMembership {
+                channel_id: m.channel_id.to_proto(),
+                role: m.role.into(),
+            })
+            .collect(),
+        ..Default::default()
+    }
+}
+
 fn build_channels_update(
     channels: ChannelsForUser,
     channel_invites: Vec<db::Channel>,

crates/collab/src/tests/channel_guest_tests.rs 🔗

@@ -195,6 +195,13 @@ async fn test_channel_requires_zed_cla(cx_a: &mut TestAppContext, cx_b: &mut Tes
         })
         .await
         .unwrap();
+    client_a
+        .channel_store()
+        .update(cx_a, |store, cx| {
+            store.set_channel_visibility(parent_channel_id, proto::ChannelVisibility::Public, cx)
+        })
+        .await
+        .unwrap();
     client_a
         .channel_store()
         .update(cx_a, |store, cx| {

crates/collab/src/tests/channel_tests.rs 🔗

@@ -48,13 +48,11 @@ async fn test_core_channels(
                 id: channel_a_id,
                 name: "channel-a".into(),
                 depth: 0,
-                role: ChannelRole::Admin,
             },
             ExpectedChannel {
                 id: channel_b_id,
                 name: "channel-b".into(),
                 depth: 1,
-                role: ChannelRole::Admin,
             },
         ],
     );
@@ -94,7 +92,6 @@ async fn test_core_channels(
             id: channel_a_id,
             name: "channel-a".into(),
             depth: 0,
-            role: ChannelRole::Member,
         }],
     );
 
@@ -141,13 +138,11 @@ async fn test_core_channels(
             ExpectedChannel {
                 id: channel_a_id,
                 name: "channel-a".into(),
-                role: ChannelRole::Member,
                 depth: 0,
             },
             ExpectedChannel {
                 id: channel_b_id,
                 name: "channel-b".into(),
-                role: ChannelRole::Member,
                 depth: 1,
             },
         ],
@@ -169,19 +164,16 @@ async fn test_core_channels(
             ExpectedChannel {
                 id: channel_a_id,
                 name: "channel-a".into(),
-                role: ChannelRole::Member,
                 depth: 0,
             },
             ExpectedChannel {
                 id: channel_b_id,
                 name: "channel-b".into(),
-                role: ChannelRole::Member,
                 depth: 1,
             },
             ExpectedChannel {
                 id: channel_c_id,
                 name: "channel-c".into(),
-                role: ChannelRole::Member,
                 depth: 2,
             },
         ],
@@ -213,19 +205,16 @@ async fn test_core_channels(
                 id: channel_a_id,
                 name: "channel-a".into(),
                 depth: 0,
-                role: ChannelRole::Admin,
             },
             ExpectedChannel {
                 id: channel_b_id,
                 name: "channel-b".into(),
                 depth: 1,
-                role: ChannelRole::Admin,
             },
             ExpectedChannel {
                 id: channel_c_id,
                 name: "channel-c".into(),
                 depth: 2,
-                role: ChannelRole::Admin,
             },
         ],
     );
@@ -247,7 +236,6 @@ async fn test_core_channels(
             id: channel_a_id,
             name: "channel-a".into(),
             depth: 0,
-            role: ChannelRole::Admin,
         }],
     );
     assert_channels(
@@ -257,7 +245,6 @@ async fn test_core_channels(
             id: channel_a_id,
             name: "channel-a".into(),
             depth: 0,
-            role: ChannelRole::Admin,
         }],
     );
 
@@ -280,7 +267,6 @@ async fn test_core_channels(
             id: channel_a_id,
             name: "channel-a".into(),
             depth: 0,
-            role: ChannelRole::Admin,
         }],
     );
 
@@ -311,7 +297,6 @@ async fn test_core_channels(
             id: channel_a_id,
             name: "channel-a-renamed".into(),
             depth: 0,
-            role: ChannelRole::Admin,
         }],
     );
 }
@@ -420,7 +405,6 @@ async fn test_channel_room(
             id: zed_id,
             name: "zed".into(),
             depth: 0,
-            role: ChannelRole::Member,
         }],
     );
     cx_b.read(|cx| {
@@ -681,7 +665,6 @@ async fn test_permissions_update_while_invited(
             depth: 0,
             id: rust_id,
             name: "rust".into(),
-            role: ChannelRole::Member,
         }],
     );
     assert_channels(client_b.channel_store(), cx_b, &[]);
@@ -709,7 +692,6 @@ async fn test_permissions_update_while_invited(
             depth: 0,
             id: rust_id,
             name: "rust".into(),
-            role: ChannelRole::Member,
         }],
     );
     assert_channels(client_b.channel_store(), cx_b, &[]);
@@ -748,7 +730,6 @@ async fn test_channel_rename(
             depth: 0,
             id: rust_id,
             name: "rust-archive".into(),
-            role: ChannelRole::Admin,
         }],
     );
 
@@ -760,7 +741,6 @@ async fn test_channel_rename(
             depth: 0,
             id: rust_id,
             name: "rust-archive".into(),
-            role: ChannelRole::Member,
         }],
     );
 }
@@ -889,7 +869,6 @@ async fn test_lost_channel_creation(
             depth: 0,
             id: channel_id,
             name: "x".into(),
-            role: ChannelRole::Member,
         }],
     );
 
@@ -913,13 +892,11 @@ async fn test_lost_channel_creation(
                 depth: 0,
                 id: channel_id,
                 name: "x".into(),
-                role: ChannelRole::Admin,
             },
             ExpectedChannel {
                 depth: 1,
                 id: subchannel_id,
                 name: "subchannel".into(),
-                role: ChannelRole::Admin,
             },
         ],
     );
@@ -944,13 +921,11 @@ async fn test_lost_channel_creation(
                 depth: 0,
                 id: channel_id,
                 name: "x".into(),
-                role: ChannelRole::Member,
             },
             ExpectedChannel {
                 depth: 1,
                 id: subchannel_id,
                 name: "subchannel".into(),
-                role: ChannelRole::Member,
             },
         ],
     );
@@ -1035,7 +1010,7 @@ async fn test_channel_link_notifications(
     let vim_channel = client_a
         .channel_store()
         .update(cx_a, |channel_store, cx| {
-            channel_store.create_channel("vim", None, cx)
+            channel_store.create_channel("vim", Some(zed_channel), cx)
         })
         .await
         .unwrap();
@@ -1048,26 +1023,16 @@ async fn test_channel_link_notifications(
         .await
         .unwrap();
 
-    client_a
-        .channel_store()
-        .update(cx_a, |channel_store, cx| {
-            channel_store.move_channel(vim_channel, Some(active_channel), cx)
-        })
-        .await
-        .unwrap();
-
-    executor.run_until_parked();
-
     // the new channel shows for b and c
     assert_channels_list_shape(
         client_a.channel_store(),
         cx_a,
-        &[(zed_channel, 0), (active_channel, 1), (vim_channel, 2)],
+        &[(zed_channel, 0), (active_channel, 1), (vim_channel, 1)],
     );
     assert_channels_list_shape(
         client_b.channel_store(),
         cx_b,
-        &[(zed_channel, 0), (active_channel, 1), (vim_channel, 2)],
+        &[(zed_channel, 0), (active_channel, 1), (vim_channel, 1)],
     );
     assert_channels_list_shape(
         client_c.channel_store(),
@@ -1078,7 +1043,7 @@ async fn test_channel_link_notifications(
     let helix_channel = client_a
         .channel_store()
         .update(cx_a, |channel_store, cx| {
-            channel_store.create_channel("helix", None, cx)
+            channel_store.create_channel("helix", Some(zed_channel), cx)
         })
         .await
         .unwrap();
@@ -1086,7 +1051,7 @@ async fn test_channel_link_notifications(
     client_a
         .channel_store()
         .update(cx_a, |channel_store, cx| {
-            channel_store.move_channel(helix_channel, Some(vim_channel), cx)
+            channel_store.move_channel(helix_channel, vim_channel, cx)
         })
         .await
         .unwrap();
@@ -1102,6 +1067,7 @@ async fn test_channel_link_notifications(
         })
         .await
         .unwrap();
+    cx_a.run_until_parked();
 
     // the new channel shows for b and c
     assert_channels_list_shape(
@@ -1110,8 +1076,8 @@ async fn test_channel_link_notifications(
         &[
             (zed_channel, 0),
             (active_channel, 1),
-            (vim_channel, 2),
-            (helix_channel, 3),
+            (vim_channel, 1),
+            (helix_channel, 2),
         ],
     );
     assert_channels_list_shape(
@@ -1119,41 +1085,6 @@ async fn test_channel_link_notifications(
         cx_c,
         &[(zed_channel, 0), (vim_channel, 1), (helix_channel, 2)],
     );
-
-    client_a
-        .channel_store()
-        .update(cx_a, |channel_store, cx| {
-            channel_store.set_channel_visibility(vim_channel, proto::ChannelVisibility::Members, cx)
-        })
-        .await
-        .unwrap();
-
-    executor.run_until_parked();
-
-    // the members-only channel is still shown for c, but hidden for b
-    assert_channels_list_shape(
-        client_b.channel_store(),
-        cx_b,
-        &[
-            (zed_channel, 0),
-            (active_channel, 1),
-            (vim_channel, 2),
-            (helix_channel, 3),
-        ],
-    );
-    cx_b.read(|cx| {
-        client_b.channel_store().read_with(cx, |channel_store, _| {
-            assert_eq!(
-                channel_store
-                    .channel_for_id(vim_channel)
-                    .unwrap()
-                    .visibility,
-                proto::ChannelVisibility::Members
-            )
-        })
-    });
-
-    assert_channels_list_shape(client_c.channel_store(), cx_c, &[(zed_channel, 0)]);
 }
 
 #[gpui::test]
@@ -1169,24 +1100,15 @@ async fn test_channel_membership_notifications(
     let user_b = client_b.user_id().unwrap();
 
     let channels = server
-        .make_channel_tree(
-            &[
-                ("zed", None),
-                ("active", Some("zed")),
-                ("vim", Some("active")),
-            ],
-            (&client_a, cx_a),
-        )
+        .make_channel_tree(&[("zed", None), ("vim", Some("zed"))], (&client_a, cx_a))
         .await;
     let zed_channel = channels[0];
-    let _active_channel = channels[1];
-    let vim_channel = channels[2];
+    let vim_channel = channels[1];
 
     try_join_all(client_a.channel_store().update(cx_a, |channel_store, cx| {
         [
             channel_store.set_channel_visibility(zed_channel, proto::ChannelVisibility::Public, cx),
             channel_store.set_channel_visibility(vim_channel, proto::ChannelVisibility::Public, cx),
-            channel_store.invite_member(vim_channel, user_b, proto::ChannelRole::Member, cx),
             channel_store.invite_member(zed_channel, user_b, proto::ChannelRole::Guest, cx),
         ]
     }))
@@ -1203,14 +1125,6 @@ async fn test_channel_membership_notifications(
         .await
         .unwrap();
 
-    client_b
-        .channel_store()
-        .update(cx_b, |channel_store, cx| {
-            channel_store.respond_to_channel_invite(vim_channel, true, cx)
-        })
-        .await
-        .unwrap();
-
     executor.run_until_parked();
 
     // we have an admin (a), and a guest (b) with access to all of zed, and membership in vim.
@@ -1222,45 +1136,14 @@ async fn test_channel_membership_notifications(
                 depth: 0,
                 id: zed_channel,
                 name: "zed".into(),
-                role: ChannelRole::Guest,
             },
             ExpectedChannel {
                 depth: 1,
                 id: vim_channel,
                 name: "vim".into(),
-                role: ChannelRole::Member,
             },
         ],
     );
-
-    client_a
-        .channel_store()
-        .update(cx_a, |channel_store, cx| {
-            channel_store.remove_member(vim_channel, user_b, cx)
-        })
-        .await
-        .unwrap();
-
-    executor.run_until_parked();
-
-    assert_channels(
-        client_b.channel_store(),
-        cx_b,
-        &[
-            ExpectedChannel {
-                depth: 0,
-                id: zed_channel,
-                name: "zed".into(),
-                role: ChannelRole::Guest,
-            },
-            ExpectedChannel {
-                depth: 1,
-                id: vim_channel,
-                name: "vim".into(),
-                role: ChannelRole::Guest,
-            },
-        ],
-    )
 }
 
 #[gpui::test]
@@ -1329,25 +1212,6 @@ async fn test_guest_access(
         assert_eq!(participants.len(), 1);
         assert_eq!(participants[0].id, client_b.user_id().unwrap());
     });
-
-    client_a
-        .channel_store()
-        .update(cx_a, |channel_store, cx| {
-            channel_store.set_channel_visibility(channel_a, proto::ChannelVisibility::Members, cx)
-        })
-        .await
-        .unwrap();
-    executor.run_until_parked();
-
-    assert_channels_list_shape(client_b.channel_store(), cx_b, &[]);
-
-    active_call_b
-        .update(cx_b, |call, cx| call.join_channel(channel_b, cx))
-        .await
-        .unwrap();
-
-    executor.run_until_parked();
-    assert_channels_list_shape(client_b.channel_store(), cx_b, &[(channel_b, 0)]);
 }
 
 #[gpui::test]
@@ -1451,7 +1315,7 @@ async fn test_channel_moving(
     client_a
         .channel_store()
         .update(cx_a, |channel_store, cx| {
-            channel_store.move_channel(channel_d_id, Some(channel_b_id), cx)
+            channel_store.move_channel(channel_d_id, channel_b_id, cx)
         })
         .await
         .unwrap();
@@ -1476,7 +1340,6 @@ struct ExpectedChannel {
     depth: usize,
     id: ChannelId,
     name: SharedString,
-    role: ChannelRole,
 }
 
 #[track_caller]
@@ -1494,7 +1357,6 @@ fn assert_channel_invitations(
                     depth: 0,
                     name: channel.name.clone(),
                     id: channel.id,
-                    role: channel.role,
                 })
                 .collect::<Vec<_>>()
         })
@@ -1516,7 +1378,6 @@ fn assert_channels(
                     depth,
                     name: channel.name.clone().into(),
                     id: channel.id,
-                    role: channel.role,
                 })
                 .collect::<Vec<_>>()
         })

crates/collab/src/tests/test_server.rs 🔗

@@ -125,6 +125,7 @@ impl TestServer {
         let channel_id = server
             .make_channel("a", None, (&client_a, cx_a), &mut [(&client_b, cx_b)])
             .await;
+        cx_a.run_until_parked();
 
         (client_a, client_b, channel_id)
     }

crates/collab_ui/src/collab_panel.rs 🔗

@@ -1548,7 +1548,7 @@ impl CollabPanel {
         if let Some(clipboard) = self.channel_clipboard.take() {
             self.channel_store
                 .update(cx, |channel_store, cx| {
-                    channel_store.move_channel(clipboard.channel_id, Some(to_channel_id), cx)
+                    channel_store.move_channel(clipboard.channel_id, to_channel_id, cx)
                 })
                 .detach_and_prompt_err("Failed to move channel", cx, |_, _| None)
         }
@@ -1980,32 +1980,19 @@ impl CollabPanel {
             | Section::Offline => true,
         };
 
-        h_flex()
-            .w_full()
-            .group("section-header")
-            .child(
-                ListHeader::new(text)
-                    .when(can_collapse, |header| {
-                        header.toggle(Some(!is_collapsed)).on_toggle(cx.listener(
-                            move |this, _, cx| {
-                                this.toggle_section_expanded(section, cx);
-                            },
-                        ))
-                    })
-                    .inset(true)
-                    .end_slot::<AnyElement>(button)
-                    .selected(is_selected),
-            )
-            .when(section == Section::Channels, |el| {
-                el.drag_over::<Channel>(|style| style.bg(cx.theme().colors().ghost_element_hover))
-                    .on_drop(cx.listener(move |this, dragged_channel: &Channel, cx| {
-                        this.channel_store
-                            .update(cx, |channel_store, cx| {
-                                channel_store.move_channel(dragged_channel.id, None, cx)
-                            })
-                            .detach_and_prompt_err("Failed to move channel", cx, |_, _| None)
-                    }))
-            })
+        h_flex().w_full().group("section-header").child(
+            ListHeader::new(text)
+                .when(can_collapse, |header| {
+                    header
+                        .toggle(Some(!is_collapsed))
+                        .on_toggle(cx.listener(move |this, _, cx| {
+                            this.toggle_section_expanded(section, cx);
+                        }))
+                })
+                .inset(true)
+                .end_slot::<AnyElement>(button)
+                .selected(is_selected),
+        )
     }
 
     fn render_contact(
@@ -2276,7 +2263,7 @@ impl CollabPanel {
             .on_drop(cx.listener(move |this, dragged_channel: &Channel, cx| {
                 this.channel_store
                     .update(cx, |channel_store, cx| {
-                        channel_store.move_channel(dragged_channel.id, Some(channel_id), cx)
+                        channel_store.move_channel(dragged_channel.id, channel_id, cx)
                     })
                     .detach_and_prompt_err("Failed to move channel", cx, |_, _| None)
             }))

crates/collab_ui/src/collab_panel/channel_modal.rs 🔗

@@ -10,7 +10,6 @@ use gpui::{
     WeakView,
 };
 use picker::{Picker, PickerDelegate};
-use rpc::proto::channel_member;
 use std::sync::Arc;
 use ui::{prelude::*, Avatar, Checkbox, ContextMenu, ListItem, ListItemSpacing};
 use util::TryFutureExt;
@@ -359,10 +358,8 @@ impl PickerDelegate for ChannelModalDelegate {
                     Some(proto::channel_member::Kind::Invitee) => {
                         self.remove_member(selected_user.id, cx);
                     }
-                    Some(proto::channel_member::Kind::AncestorMember) | None => {
-                        self.invite_member(selected_user, cx)
-                    }
                     Some(proto::channel_member::Kind::Member) => {}
+                    None => self.invite_member(selected_user, cx),
                 },
             }
         }
@@ -402,10 +399,6 @@ impl PickerDelegate for ChannelModalDelegate {
                             .children(
                                 if request_status == Some(proto::channel_member::Kind::Invitee) {
                                     Some(Label::new("Invited"))
-                                } else if membership.map(|m| m.kind)
-                                    == Some(channel_member::Kind::AncestorMember)
-                                {
-                                    Some(Label::new("Parent"))
                                 } else {
                                     None
                                 },
@@ -563,16 +556,9 @@ impl ChannelModalDelegate {
         let Some(membership) = self.member_at_index(ix) else {
             return;
         };
-        if membership.kind == proto::channel_member::Kind::AncestorMember {
-            return;
-        }
         let user_id = membership.user.id;
         let picker = cx.view().clone();
         let context_menu = ContextMenu::build(cx, |mut menu, _cx| {
-            if membership.kind == channel_member::Kind::AncestorMember {
-                return menu.entry("Inherited membership", None, |_| {});
-            };
-
             let role = membership.role;
 
             if role == ChannelRole::Admin || role == ChannelRole::Member {

crates/rpc/proto/zed.proto 🔗

@@ -212,6 +212,7 @@ enum ErrorCode {
     Forbidden = 5;
     WrongReleaseChannel = 6;
     NeedsCla = 7;
+    NotARootChannel = 8;
 }
 
 message Test {
@@ -1001,6 +1002,12 @@ message UpdateChannels {
 message UpdateUserChannels {
     repeated ChannelMessageId observed_channel_message_id = 1;
     repeated ChannelBufferVersion observed_channel_buffer_version = 2;
+    repeated ChannelMembership channel_memberships = 3;
+}
+
+message ChannelMembership {
+    uint64 channel_id = 1;
+    ChannelRole role = 2;
 }
 
 message ChannelMessageId {
@@ -1042,7 +1049,6 @@ message ChannelMember {
     enum Kind {
         Member = 0;
         Invitee = 1;
-        AncestorMember = 2;
     }
 }
 
@@ -1149,7 +1155,7 @@ message GetChannelMessagesById {
 
 message MoveChannel {
     uint64 channel_id = 1;
-    optional uint64 to = 2;
+    uint64 to = 2;
 }
 
 message JoinChannelBuffer {
@@ -1587,7 +1593,6 @@ message Channel {
     uint64 id = 1;
     string name = 2;
     ChannelVisibility visibility = 3;
-    ChannelRole role = 4;
     repeated uint64 parent_path = 5;
 }