diff --git a/.rules b/.rules index 6e9b304c66..b9eea27b67 100644 --- a/.rules +++ b/.rules @@ -5,6 +5,12 @@ * Prefer implementing functionality in existing files unless it is a new logical component. Avoid creating many small files. * Avoid using functions that panic like `unwrap()`, instead use mechanisms like `?` to propagate errors. * Be careful with operations like indexing which may panic if the indexes are out of bounds. +* Never silently discard errors with `let _ =` on fallible operations. Always handle errors appropriately: + - Propagate errors with `?` when the calling function should handle them + - Use `.log_err()` or similar when you need to ignore errors but want visibility + - Use explicit error handling with `match` or `if let Err(...)` when you need custom logic + - Example: avoid `let _ = client.request(...).await?;` - use `client.request(...).await?;` instead +* When implementing async operations that may fail, ensure errors propagate to the UI layer so users get meaningful feedback. * Never create files with `mod.rs` paths - prefer `src/some_module.rs` instead of `src/some_module/mod.rs`. # GPUI diff --git a/assets/keymaps/default-linux.json b/assets/keymaps/default-linux.json index 9012c1b092..dd9610d4a1 100644 --- a/assets/keymaps/default-linux.json +++ b/assets/keymaps/default-linux.json @@ -911,7 +911,9 @@ "context": "CollabPanel && not_editing", "bindings": { "ctrl-backspace": "collab_panel::Remove", - "space": "menu::Confirm" + "space": "menu::Confirm", + "ctrl-up": "collab_panel::MoveChannelUp", + "ctrl-down": "collab_panel::MoveChannelDown" } }, { diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json index 05aa67f8a7..4dfce63b46 100644 --- a/assets/keymaps/default-macos.json +++ b/assets/keymaps/default-macos.json @@ -967,7 +967,9 @@ "use_key_equivalents": true, "bindings": { "ctrl-backspace": "collab_panel::Remove", - "space": "menu::Confirm" + "space": "menu::Confirm", + "cmd-up": "collab_panel::MoveChannelUp", + "cmd-down": "collab_panel::MoveChannelDown" } }, { diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 64ae7cd157..b7162998cc 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -56,6 +56,7 @@ pub struct Channel { pub name: SharedString, pub visibility: proto::ChannelVisibility, pub parent_path: Vec, + pub channel_order: i32, } #[derive(Default, Debug)] @@ -614,7 +615,24 @@ impl ChannelStore { to: to.0, }) .await?; + Ok(()) + }) + } + pub fn reorder_channel( + &mut self, + channel_id: ChannelId, + direction: proto::reorder_channel::Direction, + cx: &mut Context, + ) -> Task> { + let client = self.client.clone(); + cx.spawn(async move |_, _| { + client + .request(proto::ReorderChannel { + channel_id: channel_id.0, + direction: direction.into(), + }) + .await?; Ok(()) }) } @@ -1027,6 +1045,18 @@ impl ChannelStore { }); } + #[cfg(any(test, feature = "test-support"))] + pub fn reset(&mut self) { + self.channel_invitations.clear(); + self.channel_index.clear(); + self.channel_participants.clear(); + self.outgoing_invites.clear(); + self.opened_buffers.clear(); + self.opened_chats.clear(); + self.disconnect_channel_buffers_task = None; + self.channel_states.clear(); + } + pub(crate) fn update_channels( &mut self, payload: proto::UpdateChannels, @@ -1051,6 +1081,7 @@ impl ChannelStore { visibility: channel.visibility(), name: channel.name.into(), parent_path: channel.parent_path.into_iter().map(ChannelId).collect(), + channel_order: channel.channel_order, }), ), } diff --git a/crates/channel/src/channel_store/channel_index.rs b/crates/channel/src/channel_store/channel_index.rs index c3311ad879..8eb633e25f 100644 --- a/crates/channel/src/channel_store/channel_index.rs +++ b/crates/channel/src/channel_store/channel_index.rs @@ -61,11 +61,13 @@ impl ChannelPathsInsertGuard<'_> { ret = existing_channel.visibility != channel_proto.visibility() || existing_channel.name != channel_proto.name - || existing_channel.parent_path != parent_path; + || existing_channel.parent_path != parent_path + || existing_channel.channel_order != channel_proto.channel_order; existing_channel.visibility = channel_proto.visibility(); existing_channel.name = channel_proto.name.into(); existing_channel.parent_path = parent_path; + existing_channel.channel_order = channel_proto.channel_order; } else { self.channels_by_id.insert( ChannelId(channel_proto.id), @@ -74,6 +76,7 @@ impl ChannelPathsInsertGuard<'_> { visibility: channel_proto.visibility(), name: channel_proto.name.into(), parent_path, + channel_order: channel_proto.channel_order, }), ); self.insert_root(ChannelId(channel_proto.id)); @@ -100,17 +103,18 @@ impl Drop for ChannelPathsInsertGuard<'_> { fn channel_path_sorting_key( id: ChannelId, channels_by_id: &BTreeMap>, -) -> impl Iterator { - let (parent_path, name) = channels_by_id - .get(&id) - .map_or((&[] as &[_], None), |channel| { - ( - channel.parent_path.as_slice(), - Some((channel.name.as_ref(), channel.id)), - ) - }); +) -> impl Iterator { + let (parent_path, order_and_id) = + channels_by_id + .get(&id) + .map_or((&[] as &[_], None), |channel| { + ( + channel.parent_path.as_slice(), + Some((channel.channel_order, channel.id)), + ) + }); parent_path .iter() - .filter_map(|id| Some((channels_by_id.get(id)?.name.as_ref(), *id))) - .chain(name) + .filter_map(|id| Some((channels_by_id.get(id)?.channel_order, *id))) + .chain(order_and_id) } diff --git a/crates/channel/src/channel_store_tests.rs b/crates/channel/src/channel_store_tests.rs index 20afdf0ec6..d0fb1823a3 100644 --- a/crates/channel/src/channel_store_tests.rs +++ b/crates/channel/src/channel_store_tests.rs @@ -21,12 +21,14 @@ fn test_update_channels(cx: &mut App) { name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: Vec::new(), + channel_order: 1, }, proto::Channel { id: 2, name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: Vec::new(), + channel_order: 2, }, ], ..Default::default() @@ -37,8 +39,8 @@ fn test_update_channels(cx: &mut App) { &channel_store, &[ // - (0, "a".to_string()), (0, "b".to_string()), + (0, "a".to_string()), ], cx, ); @@ -52,12 +54,14 @@ fn test_update_channels(cx: &mut App) { name: "x".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: vec![1], + channel_order: 1, }, proto::Channel { id: 4, name: "y".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: vec![2], + channel_order: 1, }, ], ..Default::default() @@ -67,15 +71,111 @@ fn test_update_channels(cx: &mut App) { assert_channels( &channel_store, &[ - (0, "a".to_string()), - (1, "y".to_string()), (0, "b".to_string()), (1, "x".to_string()), + (0, "a".to_string()), + (1, "y".to_string()), ], cx, ); } +#[gpui::test] +fn test_update_channels_order_independent(cx: &mut App) { + /// Based on: https://stackoverflow.com/a/59939809 + fn unique_permutations(items: Vec) -> Vec> { + if items.len() == 1 { + vec![items] + } else { + let mut output: Vec> = vec![]; + + for (ix, first) in items.iter().enumerate() { + let mut remaining_elements = items.clone(); + remaining_elements.remove(ix); + for mut permutation in unique_permutations(remaining_elements) { + permutation.insert(0, first.clone()); + output.push(permutation); + } + } + output + } + } + + let test_data = vec![ + proto::Channel { + id: 6, + name: "β".to_string(), + visibility: proto::ChannelVisibility::Members as i32, + parent_path: vec![1, 3], + channel_order: 1, + }, + proto::Channel { + id: 5, + name: "α".to_string(), + visibility: proto::ChannelVisibility::Members as i32, + parent_path: vec![1], + channel_order: 2, + }, + proto::Channel { + id: 3, + name: "x".to_string(), + visibility: proto::ChannelVisibility::Members as i32, + parent_path: vec![1], + channel_order: 1, + }, + proto::Channel { + id: 4, + name: "y".to_string(), + visibility: proto::ChannelVisibility::Members as i32, + parent_path: vec![2], + channel_order: 1, + }, + proto::Channel { + id: 1, + name: "b".to_string(), + visibility: proto::ChannelVisibility::Members as i32, + parent_path: Vec::new(), + channel_order: 1, + }, + proto::Channel { + id: 2, + name: "a".to_string(), + visibility: proto::ChannelVisibility::Members as i32, + parent_path: Vec::new(), + channel_order: 2, + }, + ]; + + let channel_store = init_test(cx); + let permutations = unique_permutations(test_data); + + for test_instance in permutations { + channel_store.update(cx, |channel_store, _| channel_store.reset()); + + update_channels( + &channel_store, + proto::UpdateChannels { + channels: test_instance, + ..Default::default() + }, + cx, + ); + + assert_channels( + &channel_store, + &[ + (0, "b".to_string()), + (1, "x".to_string()), + (2, "β".to_string()), + (1, "α".to_string()), + (0, "a".to_string()), + (1, "y".to_string()), + ], + cx, + ); + } +} + #[gpui::test] fn test_dangling_channel_paths(cx: &mut App) { let channel_store = init_test(cx); @@ -89,18 +189,21 @@ fn test_dangling_channel_paths(cx: &mut App) { name: "a".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: vec![], + channel_order: 1, }, proto::Channel { id: 1, name: "b".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: vec![0], + channel_order: 1, }, proto::Channel { id: 2, name: "c".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: vec![0, 1], + channel_order: 1, }, ], ..Default::default() @@ -147,6 +250,7 @@ async fn test_channel_messages(cx: &mut TestAppContext) { name: "the-channel".to_string(), visibility: proto::ChannelVisibility::Members as i32, parent_path: vec![], + channel_order: 1, }], ..Default::default() }); diff --git a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql index 1d8344045b..a1129bdeba 100644 --- a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql +++ b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql @@ -266,11 +266,14 @@ CREATE TABLE "channels" ( "created_at" TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, "visibility" VARCHAR NOT NULL, "parent_path" TEXT NOT NULL, - "requires_zed_cla" BOOLEAN NOT NULL DEFAULT FALSE + "requires_zed_cla" BOOLEAN NOT NULL DEFAULT FALSE, + "channel_order" INTEGER NOT NULL DEFAULT 1 ); CREATE INDEX "index_channels_on_parent_path" ON "channels" ("parent_path"); +CREATE INDEX "index_channels_on_parent_path_and_order" ON "channels" ("parent_path", "channel_order"); + CREATE TABLE IF NOT EXISTS "channel_chat_participants" ( "id" INTEGER PRIMARY KEY AUTOINCREMENT, "user_id" INTEGER NOT NULL REFERENCES users (id), diff --git a/crates/collab/migrations/20250530175450_add_channel_order.sql b/crates/collab/migrations/20250530175450_add_channel_order.sql new file mode 100644 index 0000000000..977a4611cd --- /dev/null +++ b/crates/collab/migrations/20250530175450_add_channel_order.sql @@ -0,0 +1,16 @@ +-- Add channel_order column to channels table with default value +ALTER TABLE channels ADD COLUMN channel_order INTEGER NOT NULL DEFAULT 1; + +-- Update channel_order for existing channels using ROW_NUMBER for deterministic ordering +UPDATE channels +SET channel_order = ( + SELECT ROW_NUMBER() OVER ( + PARTITION BY parent_path + ORDER BY name, id + ) + FROM channels c2 + WHERE c2.id = channels.id +); + +-- Create index for efficient ordering queries +CREATE INDEX "index_channels_on_parent_path_and_order" ON "channels" ("parent_path", "channel_order"); diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 93ccc1ba03..b319abc5e7 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -582,6 +582,7 @@ pub struct Channel { pub visibility: ChannelVisibility, /// parent_path is the channel ids from the root to this one (not including this one) pub parent_path: Vec, + pub channel_order: i32, } impl Channel { @@ -591,6 +592,7 @@ impl Channel { visibility: value.visibility, name: value.clone().name, parent_path: value.ancestors().collect(), + channel_order: value.channel_order, } } @@ -600,8 +602,13 @@ impl Channel { name: self.name.clone(), visibility: self.visibility.into(), parent_path: self.parent_path.iter().map(|c| c.to_proto()).collect(), + channel_order: self.channel_order, } } + + pub fn root_id(&self) -> ChannelId { + self.parent_path.first().copied().unwrap_or(self.id) + } } #[derive(Debug, PartialEq, Eq, Hash)] diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index a7ea49167c..e26da783b7 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/crates/collab/src/db/queries/channels.rs @@ -4,7 +4,7 @@ use rpc::{ ErrorCode, ErrorCodeExt, proto::{ChannelBufferVersion, VectorClockEntry, channel_member::Kind}, }; -use sea_orm::{DbBackend, TryGetableMany}; +use sea_orm::{ActiveValue, DbBackend, TryGetableMany}; impl Database { #[cfg(test)] @@ -59,16 +59,32 @@ impl Database { parent = Some(parent_channel); } + let parent_path = parent + .as_ref() + .map_or(String::new(), |parent| parent.path()); + + // Find the maximum channel_order among siblings to set the new channel at the end + let max_order = if parent_path.is_empty() { + 0 + } else { + max_order(&parent_path, &tx).await? + }; + + log::info!( + "Creating channel '{}' with parent_path='{}', max_order={}, new_order={}", + name, + parent_path, + max_order, + max_order + 1 + ); + let channel = channel::ActiveModel { id: ActiveValue::NotSet, name: ActiveValue::Set(name.to_string()), visibility: ActiveValue::Set(ChannelVisibility::Members), - parent_path: ActiveValue::Set( - parent - .as_ref() - .map_or(String::new(), |parent| parent.path()), - ), + parent_path: ActiveValue::Set(parent_path), requires_zed_cla: ActiveValue::NotSet, + channel_order: ActiveValue::Set(max_order + 1), } .insert(&*tx) .await?; @@ -531,11 +547,7 @@ impl Database { .get_channel_descendants_excluding_self(channels.iter(), tx) .await?; - for channel in channels { - if let Err(ix) = descendants.binary_search_by_key(&channel.path(), |c| c.path()) { - descendants.insert(ix, channel); - } - } + descendants.extend(channels); let roles_by_channel_id = channel_memberships .iter() @@ -952,11 +964,14 @@ impl Database { } let root_id = channel.root_id(); + let new_parent_path = new_parent.path(); let old_path = format!("{}{}/", channel.parent_path, channel.id); - let new_path = format!("{}{}/", new_parent.path(), channel.id); + let new_path = format!("{}{}/", &new_parent_path, channel.id); + let new_order = max_order(&new_parent_path, &tx).await? + 1; let mut model = channel.into_active_model(); model.parent_path = ActiveValue::Set(new_parent.path()); + model.channel_order = ActiveValue::Set(new_order); let channel = model.update(&*tx).await?; let descendent_ids = @@ -986,6 +1001,137 @@ impl Database { }) .await } + + pub async fn reorder_channel( + &self, + channel_id: ChannelId, + direction: proto::reorder_channel::Direction, + user_id: UserId, + ) -> Result> { + self.transaction(|tx| async move { + let mut channel = self.get_channel_internal(channel_id, &tx).await?; + + if channel.is_root() { + log::info!("Skipping reorder of root channel {}", channel.id,); + return Ok(vec![]); + } + + log::info!( + "Reordering channel {} (parent_path: '{}', order: {})", + channel.id, + channel.parent_path, + channel.channel_order + ); + + // Check if user is admin of the channel + self.check_user_is_channel_admin(&channel, user_id, &tx) + .await?; + + // Find the sibling channel to swap with + let sibling_channel = match direction { + proto::reorder_channel::Direction::Up => { + log::info!( + "Looking for sibling with parent_path='{}' and order < {}", + channel.parent_path, + channel.channel_order + ); + // Find channel with highest order less than current + channel::Entity::find() + .filter( + channel::Column::ParentPath + .eq(&channel.parent_path) + .and(channel::Column::ChannelOrder.lt(channel.channel_order)), + ) + .order_by_desc(channel::Column::ChannelOrder) + .one(&*tx) + .await? + } + proto::reorder_channel::Direction::Down => { + log::info!( + "Looking for sibling with parent_path='{}' and order > {}", + channel.parent_path, + channel.channel_order + ); + // Find channel with lowest order greater than current + channel::Entity::find() + .filter( + channel::Column::ParentPath + .eq(&channel.parent_path) + .and(channel::Column::ChannelOrder.gt(channel.channel_order)), + ) + .order_by_asc(channel::Column::ChannelOrder) + .one(&*tx) + .await? + } + }; + + let mut sibling_channel = match sibling_channel { + Some(sibling) => { + log::info!( + "Found sibling {} (parent_path: '{}', order: {})", + sibling.id, + sibling.parent_path, + sibling.channel_order + ); + sibling + } + None => { + log::warn!("No sibling found to swap with"); + // No sibling to swap with + return Ok(vec![]); + } + }; + + let current_order = channel.channel_order; + let sibling_order = sibling_channel.channel_order; + + channel::ActiveModel { + id: ActiveValue::Unchanged(sibling_channel.id), + channel_order: ActiveValue::Set(current_order), + ..Default::default() + } + .update(&*tx) + .await?; + sibling_channel.channel_order = current_order; + + channel::ActiveModel { + id: ActiveValue::Unchanged(channel.id), + channel_order: ActiveValue::Set(sibling_order), + ..Default::default() + } + .update(&*tx) + .await?; + channel.channel_order = sibling_order; + + log::info!( + "Reorder complete. Swapped channels {} and {}", + channel.id, + sibling_channel.id + ); + + let swapped_channels = vec![ + Channel::from_model(channel), + Channel::from_model(sibling_channel), + ]; + + Ok(swapped_channels) + }) + .await + } +} + +async fn max_order(parent_path: &str, tx: &TransactionHandle) -> Result { + let max_order = channel::Entity::find() + .filter(channel::Column::ParentPath.eq(parent_path)) + .select_only() + .column_as(channel::Column::ChannelOrder.max(), "max_order") + .into_tuple::>() + .one(&**tx) + .await? + .flatten() + .unwrap_or(0); + + Ok(max_order) } #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)] diff --git a/crates/collab/src/db/tables/channel.rs b/crates/collab/src/db/tables/channel.rs index 7625e4775f..cd3b867e13 100644 --- a/crates/collab/src/db/tables/channel.rs +++ b/crates/collab/src/db/tables/channel.rs @@ -10,6 +10,9 @@ pub struct Model { pub visibility: ChannelVisibility, pub parent_path: String, pub requires_zed_cla: bool, + /// The order of this channel relative to its siblings within the same parent. + /// Lower values appear first. Channels are sorted by parent_path first, then by channel_order. + pub channel_order: i32, } impl Model { diff --git a/crates/collab/src/db/tests.rs b/crates/collab/src/db/tests.rs index d7967fac98..2fc00fd13c 100644 --- a/crates/collab/src/db/tests.rs +++ b/crates/collab/src/db/tests.rs @@ -172,16 +172,40 @@ impl Drop for TestDb { } } +#[track_caller] +fn assert_channel_tree_matches(actual: Vec, expected: Vec) { + let expected_channels = expected.into_iter().collect::>(); + let actual_channels = actual.into_iter().collect::>(); + pretty_assertions::assert_eq!(expected_channels, actual_channels); +} + fn channel_tree(channels: &[(ChannelId, &[ChannelId], &'static str)]) -> Vec { - channels - .iter() - .map(|(id, parent_path, name)| Channel { + use std::collections::HashMap; + + let mut result = Vec::new(); + let mut order_by_parent: HashMap, i32> = HashMap::new(); + + for (id, parent_path, name) in channels { + let parent_key = parent_path.to_vec(); + let order = if parent_key.is_empty() { + 1 + } else { + *order_by_parent + .entry(parent_key.clone()) + .and_modify(|e| *e += 1) + .or_insert(1) + }; + + result.push(Channel { id: *id, name: name.to_string(), visibility: ChannelVisibility::Members, - parent_path: parent_path.to_vec(), - }) - .collect() + parent_path: parent_key, + channel_order: order, + }); + } + + result } static GITHUB_USER_ID: AtomicI32 = AtomicI32::new(5); diff --git a/crates/collab/src/db/tests/channel_tests.rs b/crates/collab/src/db/tests/channel_tests.rs index a4ff43bb37..1dd16fb50a 100644 --- a/crates/collab/src/db/tests/channel_tests.rs +++ b/crates/collab/src/db/tests/channel_tests.rs @@ -1,15 +1,15 @@ use crate::{ db::{ Channel, ChannelId, ChannelRole, Database, NewUserParams, RoomId, UserId, - tests::{channel_tree, new_test_connection, new_test_user}, + tests::{assert_channel_tree_matches, channel_tree, new_test_connection, new_test_user}, }, test_both_dbs, }; use rpc::{ ConnectionId, - proto::{self}, + proto::{self, reorder_channel}, }; -use std::sync::Arc; +use std::{collections::HashSet, sync::Arc}; test_both_dbs!(test_channels, test_channels_postgres, test_channels_sqlite); @@ -59,28 +59,28 @@ async fn test_channels(db: &Arc) { .unwrap(); let result = db.get_channels_for_user(a_id).await.unwrap(); - assert_eq!( + assert_channel_tree_matches( result.channels, channel_tree(&[ (zed_id, &[], "zed"), (crdb_id, &[zed_id], "crdb"), - (livestreaming_id, &[zed_id], "livestreaming",), + (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",) - ],) + (cargo_ra_id, &[rust_id, cargo_id], "cargo-ra"), + ]), ); let result = db.get_channels_for_user(b_id).await.unwrap(); - assert_eq!( + assert_channel_tree_matches( result.channels, channel_tree(&[ (zed_id, &[], "zed"), (crdb_id, &[zed_id], "crdb"), - (livestreaming_id, &[zed_id], "livestreaming",), - (replace_id, &[zed_id], "replace") - ],) + (livestreaming_id, &[zed_id], "livestreaming"), + (replace_id, &[zed_id], "replace"), + ]), ); // Update member permissions @@ -94,14 +94,14 @@ async fn test_channels(db: &Arc) { assert!(set_channel_admin.is_ok()); let result = db.get_channels_for_user(b_id).await.unwrap(); - assert_eq!( + assert_channel_tree_matches( result.channels, channel_tree(&[ (zed_id, &[], "zed"), (crdb_id, &[zed_id], "crdb"), - (livestreaming_id, &[zed_id], "livestreaming",), - (replace_id, &[zed_id], "replace") - ],) + (livestreaming_id, &[zed_id], "livestreaming"), + (replace_id, &[zed_id], "replace"), + ]), ); // Remove a single channel @@ -313,8 +313,8 @@ async fn test_channel_renames(db: &Arc) { test_both_dbs!( test_db_channel_moving, - test_channels_moving_postgres, - test_channels_moving_sqlite + test_db_channel_moving_postgres, + test_db_channel_moving_sqlite ); async fn test_db_channel_moving(db: &Arc) { @@ -343,16 +343,14 @@ async fn test_db_channel_moving(db: &Arc) { .await .unwrap(); - let livestreaming_dag_id = db - .create_sub_channel("livestreaming_dag", livestreaming_id, a_id) + let livestreaming_sub_id = db + .create_sub_channel("livestreaming_sub", livestreaming_id, a_id) .await .unwrap(); - // ======================================================================== // sanity check - // Initial DAG: // /- gpui2 - // zed -- crdb - livestreaming - livestreaming_dag + // zed -- crdb - livestreaming - livestreaming_sub let result = db.get_channels_for_user(a_id).await.unwrap(); assert_channel_tree( result.channels, @@ -360,10 +358,242 @@ async fn test_db_channel_moving(db: &Arc) { (zed_id, &[]), (crdb_id, &[zed_id]), (livestreaming_id, &[zed_id, crdb_id]), - (livestreaming_dag_id, &[zed_id, crdb_id, livestreaming_id]), + (livestreaming_sub_id, &[zed_id, crdb_id, livestreaming_id]), (gpui2_id, &[zed_id]), ], ); + + // Check that we can do a simple leaf -> leaf move + db.move_channel(livestreaming_sub_id, crdb_id, a_id) + .await + .unwrap(); + + // /- gpui2 + // zed -- crdb -- livestreaming + // \- livestreaming_sub + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_channel_tree( + result.channels, + &[ + (zed_id, &[]), + (crdb_id, &[zed_id]), + (livestreaming_id, &[zed_id, crdb_id]), + (livestreaming_sub_id, &[zed_id, crdb_id]), + (gpui2_id, &[zed_id]), + ], + ); + + // Check that we can move a whole subtree at once + db.move_channel(crdb_id, gpui2_id, a_id).await.unwrap(); + + // zed -- gpui2 -- crdb -- livestreaming + // \- livestreaming_sub + let result = db.get_channels_for_user(a_id).await.unwrap(); + assert_channel_tree( + result.channels, + &[ + (zed_id, &[]), + (gpui2_id, &[zed_id]), + (crdb_id, &[zed_id, gpui2_id]), + (livestreaming_id, &[zed_id, gpui2_id, crdb_id]), + (livestreaming_sub_id, &[zed_id, gpui2_id, crdb_id]), + ], + ); +} + +test_both_dbs!( + test_channel_reordering, + test_channel_reordering_postgres, + test_channel_reordering_sqlite +); + +async fn test_channel_reordering(db: &Arc) { + let admin_id = db + .create_user( + "admin@example.com", + None, + false, + NewUserParams { + github_login: "admin".into(), + github_user_id: 1, + }, + ) + .await + .unwrap() + .user_id; + + let user_id = db + .create_user( + "user@example.com", + None, + false, + NewUserParams { + github_login: "user".into(), + github_user_id: 2, + }, + ) + .await + .unwrap() + .user_id; + + // Create a root channel with some sub-channels + let root_id = db.create_root_channel("root", admin_id).await.unwrap(); + + // Invite user to root channel so they can see the sub-channels + db.invite_channel_member(root_id, user_id, admin_id, ChannelRole::Member) + .await + .unwrap(); + db.respond_to_channel_invite(root_id, user_id, true) + .await + .unwrap(); + + let alpha_id = db + .create_sub_channel("alpha", root_id, admin_id) + .await + .unwrap(); + let beta_id = db + .create_sub_channel("beta", root_id, admin_id) + .await + .unwrap(); + let gamma_id = db + .create_sub_channel("gamma", root_id, admin_id) + .await + .unwrap(); + + // Initial order should be: root, alpha (order=1), beta (order=2), gamma (order=3) + let result = db.get_channels_for_user(admin_id).await.unwrap(); + assert_channel_tree_order( + result.channels, + &[ + (root_id, &[], 1), + (alpha_id, &[root_id], 1), + (beta_id, &[root_id], 2), + (gamma_id, &[root_id], 3), + ], + ); + + // Test moving beta up (should swap with alpha) + let updated_channels = db + .reorder_channel(beta_id, reorder_channel::Direction::Up, admin_id) + .await + .unwrap(); + + // Verify that beta and alpha were returned as updated + assert_eq!(updated_channels.len(), 2); + let updated_ids: std::collections::HashSet<_> = updated_channels.iter().map(|c| c.id).collect(); + assert!(updated_ids.contains(&alpha_id)); + assert!(updated_ids.contains(&beta_id)); + + // Now order should be: root, beta (order=1), alpha (order=2), gamma (order=3) + let result = db.get_channels_for_user(admin_id).await.unwrap(); + assert_channel_tree_order( + result.channels, + &[ + (root_id, &[], 1), + (beta_id, &[root_id], 1), + (alpha_id, &[root_id], 2), + (gamma_id, &[root_id], 3), + ], + ); + + // Test moving gamma down (should be no-op since it's already last) + let updated_channels = db + .reorder_channel(gamma_id, reorder_channel::Direction::Down, admin_id) + .await + .unwrap(); + + // Should return just nothing + assert_eq!(updated_channels.len(), 0); + + // Test moving alpha down (should swap with gamma) + let updated_channels = db + .reorder_channel(alpha_id, reorder_channel::Direction::Down, admin_id) + .await + .unwrap(); + + // Verify that alpha and gamma were returned as updated + assert_eq!(updated_channels.len(), 2); + let updated_ids: std::collections::HashSet<_> = updated_channels.iter().map(|c| c.id).collect(); + assert!(updated_ids.contains(&alpha_id)); + assert!(updated_ids.contains(&gamma_id)); + + // Now order should be: root, beta (order=1), gamma (order=2), alpha (order=3) + let result = db.get_channels_for_user(admin_id).await.unwrap(); + assert_channel_tree_order( + result.channels, + &[ + (root_id, &[], 1), + (beta_id, &[root_id], 1), + (gamma_id, &[root_id], 2), + (alpha_id, &[root_id], 3), + ], + ); + + // Test that non-admin cannot reorder + let reorder_result = db + .reorder_channel(beta_id, reorder_channel::Direction::Up, user_id) + .await; + assert!(reorder_result.is_err()); + + // Test moving beta up (should be no-op since it's already first) + let updated_channels = db + .reorder_channel(beta_id, reorder_channel::Direction::Up, admin_id) + .await + .unwrap(); + + // Should return nothing + assert_eq!(updated_channels.len(), 0); + + // Adding a channel to an existing ordering should add it to the end + let delta_id = db + .create_sub_channel("delta", root_id, admin_id) + .await + .unwrap(); + + let result = db.get_channels_for_user(admin_id).await.unwrap(); + assert_channel_tree_order( + result.channels, + &[ + (root_id, &[], 1), + (beta_id, &[root_id], 1), + (gamma_id, &[root_id], 2), + (alpha_id, &[root_id], 3), + (delta_id, &[root_id], 4), + ], + ); + + // And moving a channel into an existing ordering should add it to the end + let eta_id = db + .create_sub_channel("eta", delta_id, admin_id) + .await + .unwrap(); + + let result = db.get_channels_for_user(admin_id).await.unwrap(); + assert_channel_tree_order( + result.channels, + &[ + (root_id, &[], 1), + (beta_id, &[root_id], 1), + (gamma_id, &[root_id], 2), + (alpha_id, &[root_id], 3), + (delta_id, &[root_id], 4), + (eta_id, &[root_id, delta_id], 1), + ], + ); + + db.move_channel(eta_id, root_id, admin_id).await.unwrap(); + let result = db.get_channels_for_user(admin_id).await.unwrap(); + assert_channel_tree_order( + result.channels, + &[ + (root_id, &[], 1), + (beta_id, &[root_id], 1), + (gamma_id, &[root_id], 2), + (alpha_id, &[root_id], 3), + (delta_id, &[root_id], 4), + (eta_id, &[root_id], 5), + ], + ); } test_both_dbs!( @@ -422,6 +652,20 @@ async fn test_db_channel_moving_bugs(db: &Arc) { (livestreaming_id, &[zed_id, projects_id]), ], ); + + // Can't un-root a root channel + db.move_channel(zed_id, livestreaming_id, user_id) + .await + .unwrap_err(); + let result = db.get_channels_for_user(user_id).await.unwrap(); + assert_channel_tree( + result.channels, + &[ + (zed_id, &[]), + (projects_id, &[zed_id]), + (livestreaming_id, &[zed_id, projects_id]), + ], + ); } test_both_dbs!( @@ -745,10 +989,29 @@ fn assert_channel_tree(actual: Vec, expected: &[(ChannelId, &[ChannelId let actual = actual .iter() .map(|channel| (channel.id, channel.parent_path.as_slice())) - .collect::>(); - pretty_assertions::assert_eq!( - actual, - expected.to_vec(), - "wrong channel ids and parent paths" - ); + .collect::>(); + let expected = expected + .iter() + .map(|(id, parents)| (*id, *parents)) + .collect::>(); + pretty_assertions::assert_eq!(actual, expected, "wrong channel ids and parent paths"); +} + +#[track_caller] +fn assert_channel_tree_order(actual: Vec, expected: &[(ChannelId, &[ChannelId], i32)]) { + let actual = actual + .iter() + .map(|channel| { + ( + channel.id, + channel.parent_path.as_slice(), + channel.channel_order, + ) + }) + .collect::>(); + let expected = expected + .iter() + .map(|(id, parents, order)| (*id, *parents, *order)) + .collect::>(); + pretty_assertions::assert_eq!(actual, expected, "wrong channel ids and parent paths"); } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 4364d9f677..4f371b8135 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -384,6 +384,7 @@ impl Server { .add_request_handler(get_notifications) .add_request_handler(mark_notification_as_read) .add_request_handler(move_channel) + .add_request_handler(reorder_channel) .add_request_handler(follow) .add_message_handler(unfollow) .add_message_handler(update_followers) @@ -3220,6 +3221,51 @@ async fn move_channel( Ok(()) } +async fn reorder_channel( + request: proto::ReorderChannel, + response: Response, + session: Session, +) -> Result<()> { + let channel_id = ChannelId::from_proto(request.channel_id); + let direction = request.direction(); + + let updated_channels = session + .db() + .await + .reorder_channel(channel_id, direction, session.user_id()) + .await?; + + if let Some(root_id) = updated_channels.first().map(|channel| channel.root_id()) { + let connection_pool = session.connection_pool().await; + for (connection_id, role) in connection_pool.channel_connection_ids(root_id) { + let channels = updated_channels + .iter() + .filter_map(|channel| { + if role.can_see_channel(channel.visibility) { + Some(channel.to_proto()) + } else { + None + } + }) + .collect::>(); + + if channels.is_empty() { + continue; + } + + let update = proto::UpdateChannels { + channels, + ..Default::default() + }; + + session.peer.send(connection_id, update.clone())?; + } + } + + response.send(Ack {})?; + Ok(()) +} + /// Get the list of channel members async fn get_channel_members( request: proto::GetChannelMembers, diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 3d03a987ed..8ec1395903 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -14,9 +14,9 @@ use fuzzy::{StringMatchCandidate, match_strings}; use gpui::{ AnyElement, App, AsyncWindowContext, Bounds, ClickEvent, ClipboardItem, Context, DismissEvent, Div, Entity, EventEmitter, FocusHandle, Focusable, FontStyle, InteractiveElement, IntoElement, - ListOffset, ListState, MouseDownEvent, ParentElement, Pixels, Point, PromptLevel, Render, - SharedString, Styled, Subscription, Task, TextStyle, WeakEntity, Window, actions, anchored, - canvas, deferred, div, fill, list, point, prelude::*, px, + KeyContext, ListOffset, ListState, MouseDownEvent, ParentElement, Pixels, Point, PromptLevel, + Render, SharedString, Styled, Subscription, Task, TextStyle, WeakEntity, Window, actions, + anchored, canvas, deferred, div, fill, list, point, prelude::*, px, }; use menu::{Cancel, Confirm, SecondaryConfirm, SelectNext, SelectPrevious}; use project::{Fs, Project}; @@ -52,6 +52,8 @@ actions!( StartMoveChannel, MoveSelected, InsertSpace, + MoveChannelUp, + MoveChannelDown, ] ); @@ -1961,6 +1963,33 @@ impl CollabPanel { }) } + fn move_channel_up(&mut self, _: &MoveChannelUp, window: &mut Window, cx: &mut Context) { + if let Some(channel) = self.selected_channel() { + self.channel_store.update(cx, |store, cx| { + store + .reorder_channel(channel.id, proto::reorder_channel::Direction::Up, cx) + .detach_and_prompt_err("Failed to move channel up", window, cx, |_, _, _| None) + }); + } + } + + fn move_channel_down( + &mut self, + _: &MoveChannelDown, + window: &mut Window, + cx: &mut Context, + ) { + if let Some(channel) = self.selected_channel() { + self.channel_store.update(cx, |store, cx| { + store + .reorder_channel(channel.id, proto::reorder_channel::Direction::Down, cx) + .detach_and_prompt_err("Failed to move channel down", window, cx, |_, _, _| { + None + }) + }); + } + } + fn open_channel_notes( &mut self, channel_id: ChannelId, @@ -1974,7 +2003,7 @@ impl CollabPanel { fn show_inline_context_menu( &mut self, - _: &menu::SecondaryConfirm, + _: &Secondary, window: &mut Window, cx: &mut Context, ) { @@ -2003,6 +2032,21 @@ impl CollabPanel { } } + fn dispatch_context(&self, window: &Window, cx: &Context) -> KeyContext { + let mut dispatch_context = KeyContext::new_with_defaults(); + dispatch_context.add("CollabPanel"); + dispatch_context.add("menu"); + + let identifier = if self.channel_name_editor.focus_handle(cx).is_focused(window) { + "editing" + } else { + "not_editing" + }; + + dispatch_context.add(identifier); + dispatch_context + } + fn selected_channel(&self) -> Option<&Arc> { self.selection .and_then(|ix| self.entries.get(ix)) @@ -2965,7 +3009,7 @@ fn render_tree_branch( impl Render for CollabPanel { fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { v_flex() - .key_context("CollabPanel") + .key_context(self.dispatch_context(window, cx)) .on_action(cx.listener(CollabPanel::cancel)) .on_action(cx.listener(CollabPanel::select_next)) .on_action(cx.listener(CollabPanel::select_previous)) @@ -2977,6 +3021,8 @@ impl Render for CollabPanel { .on_action(cx.listener(CollabPanel::collapse_selected_channel)) .on_action(cx.listener(CollabPanel::expand_selected_channel)) .on_action(cx.listener(CollabPanel::start_move_selected_channel)) + .on_action(cx.listener(CollabPanel::move_channel_up)) + .on_action(cx.listener(CollabPanel::move_channel_down)) .track_focus(&self.focus_handle(cx)) .size_full() .child(if self.user_store.read(cx).current_user().is_none() { diff --git a/crates/proto/proto/channel.proto b/crates/proto/proto/channel.proto index cf960c3f34..324380048a 100644 --- a/crates/proto/proto/channel.proto +++ b/crates/proto/proto/channel.proto @@ -8,6 +8,7 @@ message Channel { uint64 id = 1; string name = 2; ChannelVisibility visibility = 3; + int32 channel_order = 4; repeated uint64 parent_path = 5; } @@ -207,6 +208,15 @@ message MoveChannel { uint64 to = 2; } +message ReorderChannel { + uint64 channel_id = 1; + enum Direction { + Up = 0; + Down = 1; + } + Direction direction = 2; +} + message JoinChannelBuffer { uint64 channel_id = 1; } diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index 8bf418b10b..71daa99a7e 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -190,6 +190,7 @@ message Envelope { GetChannelMessagesById get_channel_messages_by_id = 144; MoveChannel move_channel = 147; + ReorderChannel reorder_channel = 349; SetChannelVisibility set_channel_visibility = 148; AddNotification add_notification = 149; diff --git a/crates/proto/src/proto.rs b/crates/proto/src/proto.rs index 9c012a758f..32ad407a19 100644 --- a/crates/proto/src/proto.rs +++ b/crates/proto/src/proto.rs @@ -176,6 +176,7 @@ messages!( (LspExtClearFlycheck, Background), (MarkNotificationRead, Foreground), (MoveChannel, Foreground), + (ReorderChannel, Foreground), (MultiLspQuery, Background), (MultiLspQueryResponse, Background), (OnTypeFormatting, Background), @@ -389,6 +390,7 @@ request_messages!( (RemoveContact, Ack), (RenameChannel, RenameChannelResponse), (RenameProjectEntry, ProjectEntryResponse), + (ReorderChannel, Ack), (RequestContact, Ack), ( ResolveCompletionDocumentation,