From 64315583c82370900735e609231746bd34aba571 Mon Sep 17 00:00:00 2001 From: "Joseph T. Lyons" Date: Mon, 30 Mar 2026 09:07:13 -0400 Subject: [PATCH] Add the ability to reorder favorited collab channels (#52649) Currently, if you try to re-order a favorite, the favorite will not reorder, but the actual channels will. https://github.com/user-attachments/assets/1fbab9ea-4ff4-473f-8de3-d3b60696c5a1 Additionally, a new bug seems to be that if you reorder channels, focus jumps to a favorite: https://github.com/user-attachments/assets/fa776ad2-8648-4e68-a253-a98f57bd4951 This PR allows for re-ordering of favorites independent of the actual channels, and fixes the focusing bug https://github.com/user-attachments/assets/977e575a-055c-4f26-8183-2744ff7f8f56 I didn't feel comfortable adding just the functionality/fixes alone, so I added a function to represent the state of the collab panel as a list of strings, like how testing around the project panel is, and wrote a few tests to ensure the behavior was pinned down. The tests cover testing: - Favoriting/unfavoriting - Reordering favorites without impacting order of channels in the channels list - Reordering channels in the channels list without impacting order of favorites Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #ISSUE Release Notes: - Added the ability to reorder favorited collab channels. --- crates/channel/src/channel_store.rs | 17 +- .../tests/integration/collab_panel_tests.rs | 356 ++++++++++++++++++ .../collab/tests/integration/collab_tests.rs | 1 + crates/collab_ui/src/collab_panel.rs | 218 +++++++++-- 4 files changed, 561 insertions(+), 31 deletions(-) create mode 100644 crates/collab/tests/integration/collab_panel_tests.rs diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 7bd06159df037a870d5d4a9bb8af36872ec0454f..bc629f8740d5cb1969cf99746a87f81f191db122 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -166,17 +166,18 @@ impl ChannelStore { } pub fn is_channel_favorited(&self, channel_id: ChannelId) -> bool { - self.favorite_channel_ids.binary_search(&channel_id).is_ok() + self.favorite_channel_ids.contains(&channel_id) } pub fn toggle_favorite_channel(&mut self, channel_id: ChannelId, cx: &mut Context) { - match self.favorite_channel_ids.binary_search(&channel_id) { - Ok(ix) => { - self.favorite_channel_ids.remove(ix); - } - Err(ix) => { - self.favorite_channel_ids.insert(ix, channel_id); - } + if let Some(ix) = self + .favorite_channel_ids + .iter() + .position(|id| *id == channel_id) + { + self.favorite_channel_ids.remove(ix); + } else { + self.favorite_channel_ids.push(channel_id); } cx.notify(); } diff --git a/crates/collab/tests/integration/collab_panel_tests.rs b/crates/collab/tests/integration/collab_panel_tests.rs new file mode 100644 index 0000000000000000000000000000000000000000..f1b65655e77bd9902970af6acbf5c89575df90ae --- /dev/null +++ b/crates/collab/tests/integration/collab_panel_tests.rs @@ -0,0 +1,356 @@ +use crate::TestServer; +use collab_ui::CollabPanel; +use collab_ui::collab_panel::{MoveChannelDown, MoveChannelUp, ToggleSelectedChannelFavorite}; +use gpui::TestAppContext; +use menu::{SelectNext, SelectPrevious}; + +#[gpui::test] +async fn test_reorder_favorite_channels_independently_of_channels(cx: &mut TestAppContext) { + let (server, client) = TestServer::start1(cx).await; + let root = server + .make_channel("root", None, (&client, cx), &mut []) + .await; + let _ = server + .make_channel("channel-a", Some(root), (&client, cx), &mut []) + .await; + let _ = server + .make_channel("channel-b", Some(root), (&client, cx), &mut []) + .await; + let _ = server + .make_channel("channel-c", Some(root), (&client, cx), &mut []) + .await; + + let (workspace, cx) = client.build_test_workspace(cx).await; + let panel = workspace.update_in(cx, |workspace, window, cx| { + let panel = CollabPanel::new(workspace, window, cx); + workspace.add_panel(panel.clone(), window, cx); + panel + }); + cx.run_until_parked(); + + // Verify initial state. + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Select channel-b. + panel.update_in(cx, |panel, window, cx| { + panel.select_next(&SelectNext, window, cx); + panel.select_next(&SelectNext, window, cx); + panel.select_next(&SelectNext, window, cx); + panel.select_next(&SelectNext, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b <== selected", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Favorite channel-b. + panel.update_in(cx, |panel, window, cx| { + panel.toggle_selected_channel_favorite(&ToggleSelectedChannelFavorite, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-b", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b <== selected", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Select channel-c. + panel.update_in(cx, |panel, window, cx| { + panel.select_next(&SelectNext, window, cx); + }); + // Favorite channel-c. + panel.update_in(cx, |panel, window, cx| { + panel.toggle_selected_channel_favorite(&ToggleSelectedChannelFavorite, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c <== selected", + "[Contacts]", + ] + ); + + // Navigate up to favorite channel-b . + panel.update_in(cx, |panel, window, cx| { + panel.select_previous(&SelectPrevious, window, cx); + panel.select_previous(&SelectPrevious, window, cx); + panel.select_previous(&SelectPrevious, window, cx); + panel.select_previous(&SelectPrevious, window, cx); + panel.select_previous(&SelectPrevious, window, cx); + panel.select_previous(&SelectPrevious, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-b <== selected", + " #️⃣ channel-c", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Move favorite channel-b down. + // The Channels section should remain unchanged + panel.update_in(cx, |panel, window, cx| { + panel.move_channel_down(&MoveChannelDown, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-c", + " #️⃣ channel-b <== selected", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Move favorite channel-b down again when it's already last (should be no-op). + panel.update_in(cx, |panel, window, cx| { + panel.move_channel_down(&MoveChannelDown, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-c", + " #️⃣ channel-b <== selected", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Move favorite channel-b back up. + // The Channels section should remain unchanged. + panel.update_in(cx, |panel, window, cx| { + panel.move_channel_up(&MoveChannelUp, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-b <== selected", + " #️⃣ channel-c", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Move favorite channel-b up again when it's already first (should be no-op). + panel.update_in(cx, |panel, window, cx| { + panel.move_channel_up(&MoveChannelUp, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-b <== selected", + " #️⃣ channel-c", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Unfavorite channel-b. + // Selection should move to the next favorite (channel-c). + panel.update_in(cx, |panel, window, cx| { + panel.toggle_selected_channel_favorite(&ToggleSelectedChannelFavorite, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-c <== selected", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Unfavorite channel-c. + // Favorites section should disappear entirely. + // Selection should move to the next available item. + panel.update_in(cx, |panel, window, cx| { + panel.toggle_selected_channel_favorite(&ToggleSelectedChannelFavorite, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Channels]", + " v root <== selected", + " #️⃣ channel-a", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); +} + +#[gpui::test] +async fn test_reorder_channels_independently_of_favorites(cx: &mut TestAppContext) { + let (server, client) = TestServer::start1(cx).await; + let root = server + .make_channel("root", None, (&client, cx), &mut []) + .await; + let _ = server + .make_channel("channel-a", Some(root), (&client, cx), &mut []) + .await; + let _ = server + .make_channel("channel-b", Some(root), (&client, cx), &mut []) + .await; + let _ = server + .make_channel("channel-c", Some(root), (&client, cx), &mut []) + .await; + + let (workspace, cx) = client.build_test_workspace(cx).await; + let panel = workspace.update_in(cx, |workspace, window, cx| { + let panel = CollabPanel::new(workspace, window, cx); + workspace.add_panel(panel.clone(), window, cx); + panel + }); + cx.run_until_parked(); + + // Select channel-a. + panel.update_in(cx, |panel, window, cx| { + panel.select_next(&SelectNext, window, cx); + panel.select_next(&SelectNext, window, cx); + panel.select_next(&SelectNext, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Channels]", + " v root", + " #️⃣ channel-a <== selected", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Favorite channel-a. + panel.update_in(cx, |panel, window, cx| { + panel.toggle_selected_channel_favorite(&ToggleSelectedChannelFavorite, window, cx); + }); + + // Select channel-b. + // Favorite channel-b. + panel.update_in(cx, |panel, window, cx| { + panel.select_next(&SelectNext, window, cx); + panel.toggle_selected_channel_favorite(&ToggleSelectedChannelFavorite, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-a", + " #️⃣ channel-b", + "[Channels]", + " v root", + " #️⃣ channel-a", + " #️⃣ channel-b <== selected", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Select channel-a in the Channels section. + panel.update_in(cx, |panel, window, cx| { + panel.select_previous(&SelectPrevious, window, cx); + }); + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-a", + " #️⃣ channel-b", + "[Channels]", + " v root", + " #️⃣ channel-a <== selected", + " #️⃣ channel-b", + " #️⃣ channel-c", + "[Contacts]", + ] + ); + + // Move channel-a down. + // The Favorites section should remain unchanged. + // Selection should remain on channel-a in the Channels section, + // not jump to channel-a in Favorites. + panel.update_in(cx, |panel, window, cx| { + panel.move_channel_down(&MoveChannelDown, window, cx); + }); + cx.run_until_parked(); + + assert_eq!( + panel.read_with(cx, |panel, _| panel.entries_as_strings()), + &[ + "[Favorites]", + " #️⃣ channel-a", + " #️⃣ channel-b", + "[Channels]", + " v root", + " #️⃣ channel-b", + " #️⃣ channel-a <== selected", + " #️⃣ channel-c", + "[Contacts]", + ] + ); +} diff --git a/crates/collab/tests/integration/collab_tests.rs b/crates/collab/tests/integration/collab_tests.rs index 8c568c5c4e1f9b8414b48d5b7175763ded5e89c9..5079698a96a1d04e0dd5da4baca9d8bae7cf9665 100644 --- a/crates/collab/tests/integration/collab_tests.rs +++ b/crates/collab/tests/integration/collab_tests.rs @@ -6,6 +6,7 @@ mod agent_sharing_tests; mod channel_buffer_tests; mod channel_guest_tests; mod channel_tests; +mod collab_panel_tests; mod db_tests; mod editor_tests; mod following_tests; diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index 305e06680e57c7e9c3f1b08d5bdcfc80456313ee..602a208c686caedcd396d83dcb5dd503925ad044 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -23,7 +23,7 @@ use menu::{Cancel, Confirm, SecondaryConfirm, SelectNext, SelectPrevious}; use project::{Fs, Project}; use rpc::{ ErrorCode, ErrorExt, - proto::{self, ChannelVisibility, PeerId}, + proto::{self, ChannelVisibility, PeerId, reorder_channel::Direction}, }; use serde::{Deserialize, Serialize}; use settings::Settings; @@ -306,6 +306,7 @@ enum ListEntry { channel: Arc, depth: usize, has_children: bool, + is_favorite: bool, // `None` when the channel is a parent of a matched channel. string_match: Option, }, @@ -727,6 +728,7 @@ impl CollabPanel { channel: (*channel).clone(), depth: 0, has_children: false, + is_favorite: true, string_match: matches_by_candidate.get(&ix).cloned().cloned(), }); } @@ -827,6 +829,7 @@ impl CollabPanel { channel: channel.clone(), depth, has_children: false, + is_favorite: false, string_match: matches_by_id.get(&channel.id).map(|mat| (*mat).clone()), }); self.entries @@ -843,6 +846,7 @@ impl CollabPanel { channel: channel.clone(), depth, has_children, + is_favorite: false, string_match: matches_by_id.get(&channel.id).map(|mat| (*mat).clone()), }); } @@ -994,13 +998,22 @@ impl CollabPanel { if select_same_item { if let Some(prev_selected_entry) = prev_selected_entry { - self.selection.take(); + let prev_selection = self.selection.take(); for (ix, entry) in self.entries.iter().enumerate() { if *entry == prev_selected_entry { self.selection = Some(ix); break; } } + if self.selection.is_none() { + self.selection = prev_selection.and_then(|prev_ix| { + if self.entries.is_empty() { + None + } else { + Some(prev_ix.min(self.entries.len() - 1)) + } + }); + } } } else { self.selection = self.selection.and_then(|prev_selection| { @@ -1654,7 +1667,7 @@ impl CollabPanel { self.update_entries(false, cx); } - fn select_next(&mut self, _: &SelectNext, _: &mut Window, cx: &mut Context) { + pub fn select_next(&mut self, _: &SelectNext, _: &mut Window, cx: &mut Context) { let ix = self.selection.map_or(0, |ix| ix + 1); if ix < self.entries.len() { self.selection = Some(ix); @@ -1666,7 +1679,7 @@ impl CollabPanel { cx.notify(); } - fn select_previous(&mut self, _: &SelectPrevious, _: &mut Window, cx: &mut Context) { + pub fn select_previous(&mut self, _: &SelectPrevious, _: &mut Window, cx: &mut Context) { let ix = self.selection.take().unwrap_or(0); if ix > 0 { self.selection = Some(ix - 1); @@ -1922,7 +1935,7 @@ impl CollabPanel { self.collapsed_channels.binary_search(&channel_id).is_ok() } - fn toggle_favorite_channel(&mut self, channel_id: ChannelId, cx: &mut Context) { + pub fn toggle_favorite_channel(&mut self, channel_id: ChannelId, cx: &mut Context) { self.channel_store.update(cx, |store, cx| { store.toggle_favorite_channel(channel_id, cx); }); @@ -1934,6 +1947,12 @@ impl CollabPanel { } fn persist_favorites(&mut self, cx: &mut Context) { + // GlobalKeyValueStore uses a sqlez worker thread that the test + // scheduler can't control, causing non-determinism failures. + if cfg!(any(test, feature = "test-support")) { + return; + } + let favorite_ids: Vec = self .channel_store .read(cx) @@ -2069,7 +2088,7 @@ impl CollabPanel { } } - fn toggle_selected_channel_favorite( + pub fn toggle_selected_channel_favorite( &mut self, _: &ToggleSelectedChannelFavorite, _window: &mut Window, @@ -2160,33 +2179,79 @@ 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) - }); - } + pub fn move_channel_up( + &mut self, + _: &MoveChannelUp, + window: &mut Window, + cx: &mut Context, + ) { + self.reorder_selected_channel(Direction::Up, window, cx); } - fn move_channel_down( + pub fn move_channel_down( &mut self, _: &MoveChannelDown, window: &mut Window, cx: &mut Context, ) { - if let Some(channel) = self.selected_channel() { + self.reorder_selected_channel(Direction::Down, window, cx); + } + + fn reorder_selected_channel( + &mut self, + direction: Direction, + window: &mut Window, + cx: &mut Context, + ) { + if let Some(channel) = self.selected_channel().cloned() { + if self.selected_entry_is_favorite() { + self.reorder_favorite(channel.id, direction, cx); + return; + } + 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 - }) + .reorder_channel(channel.id, direction, cx) + .detach_and_prompt_err( + match direction { + Direction::Up => "Failed to move channel up", + Direction::Down => "Failed to move channel down", + }, + window, + cx, + |_, _, _| None, + ) }); } } + pub fn reorder_favorite( + &mut self, + channel_id: ChannelId, + direction: Direction, + cx: &mut Context, + ) { + self.channel_store.update(cx, |store, cx| { + let favorite_ids = store.favorite_channel_ids(); + let Some(channel_index) = favorite_ids.iter().position(|id| *id == channel_id) else { + return; + }; + let target_channel_index = match direction { + Direction::Up => channel_index.checked_sub(1), + Direction::Down => { + let next = channel_index + 1; + (next < favorite_ids.len()).then_some(next) + } + }; + if let Some(target_channel_index) = target_channel_index { + let mut new_ids = favorite_ids.to_vec(); + new_ids.swap(channel_index, target_channel_index); + store.set_favorite_channel_ids(new_ids, cx); + } + }); + self.persist_favorites(cx); + } + fn open_channel_notes( &mut self, channel_id: ChannelId, @@ -2255,6 +2320,20 @@ impl CollabPanel { }) } + fn selected_entry_is_favorite(&self) -> bool { + self.selection + .and_then(|ix| self.entries.get(ix)) + .is_some_and(|entry| { + matches!( + entry, + ListEntry::Channel { + is_favorite: true, + .. + } + ) + }) + } + fn selected_contact(&self) -> Option> { self.selection .and_then(|ix| self.entries.get(ix)) @@ -2552,6 +2631,7 @@ impl CollabPanel { depth, has_children, string_match, + .. } => self .render_channel( channel, @@ -3445,13 +3525,17 @@ impl PartialEq for ListEntry { } } ListEntry::Channel { - channel: channel_1, .. + channel: channel_1, + is_favorite: is_favorite_1, + .. } => { if let ListEntry::Channel { - channel: channel_2, .. + channel: channel_2, + is_favorite: is_favorite_2, + .. } = other { - return channel_1.id == channel_2.id; + return channel_1.id == channel_2.id && is_favorite_1 == is_favorite_2; } } ListEntry::ChannelNotes { channel_id } => { @@ -3557,3 +3641,91 @@ impl Render for JoinChannelTooltip { }) } } + +#[cfg(any(test, feature = "test-support"))] +impl CollabPanel { + pub fn entries_as_strings(&self) -> Vec { + let mut string_entries = Vec::new(); + for (index, entry) in self.entries.iter().enumerate() { + let selected_marker = if self.selection == Some(index) { + " <== selected" + } else { + "" + }; + match entry { + ListEntry::Header(section) => { + let name = match section { + Section::ActiveCall => "Active Call", + Section::FavoriteChannels => "Favorites", + Section::Channels => "Channels", + Section::ChannelInvites => "Channel Invites", + Section::ContactRequests => "Contact Requests", + Section::Contacts => "Contacts", + Section::Online => "Online", + Section::Offline => "Offline", + }; + string_entries.push(format!("[{name}]")); + } + ListEntry::Channel { + channel, + depth, + has_children, + .. + } => { + let indent = " ".repeat(*depth + 1); + let icon = if *has_children { + "v " + } else if channel.visibility == proto::ChannelVisibility::Public { + "🛜 " + } else { + "#️⃣ " + }; + string_entries.push(format!("{indent}{icon}{}{selected_marker}", channel.name)); + } + ListEntry::ChannelNotes { .. } => { + string_entries.push(format!(" (notes){selected_marker}")); + } + ListEntry::ChannelEditor { depth } => { + let indent = " ".repeat(*depth + 1); + string_entries.push(format!("{indent}[editor]{selected_marker}")); + } + ListEntry::ChannelInvite(channel) => { + string_entries.push(format!(" (invite) #{}{selected_marker}", channel.name)); + } + ListEntry::CallParticipant { user, .. } => { + string_entries.push(format!(" {}{selected_marker}", user.github_login)); + } + ListEntry::ParticipantProject { + worktree_root_names, + .. + } => { + string_entries.push(format!( + " {}{selected_marker}", + worktree_root_names.join(", ") + )); + } + ListEntry::ParticipantScreen { .. } => { + string_entries.push(format!(" (screen){selected_marker}")); + } + ListEntry::IncomingRequest(user) => { + string_entries.push(format!( + " (incoming) {}{selected_marker}", + user.github_login + )); + } + ListEntry::OutgoingRequest(user) => { + string_entries.push(format!( + " (outgoing) {}{selected_marker}", + user.github_login + )); + } + ListEntry::Contact { contact, .. } => { + string_entries + .push(format!(" {}{selected_marker}", contact.user.github_login)); + } + ListEntry::ContactPlaceholder => {} + } + } + string_entries + } +}