Nathan SoboandMikayla Maki
0a2186c87b
Release Notes:
- Added channel reordering for administrators (use `cmd-up` and
`cmd-down` on macOS or `ctrl-up` `ctrl-down` on Linux to move channels
up or down within their parent)
## Summary
This PR introduces the ability for channel administrators to reorder
channels within their parent context, providing better organizational
control over channel hierarchies. Users can now move channels up or down
relative to their siblings using keyboard shortcuts.
## Problem
Previously, channels were displayed in alphabetical order with no way to
customize their arrangement. This made it difficult for teams to
organize channels in a logical order that reflected their workflow or
importance, forcing users to prefix channel names with numbers or
special characters as a workaround.
## Solution
The implementation adds a persistent `channel_order` field to channels
that determines their display order within their parent. Channels with
the same parent are sorted by this field rather than alphabetically.
## Implementation Details
### Database Schema
Added a new column and index to support efficient ordering:
```sql
-- crates/collab/migrations/20250530175450_add_channel_order.sql
ALTER TABLE channels ADD COLUMN channel_order INTEGER NOT NULL DEFAULT 1;
CREATE INDEX CONCURRENTLY "index_channels_on_parent_path_and_order" ON "channels" ("parent_path", "channel_order");
```
### RPC Protocol
Extended the channel proto with ordering support:
```proto
// crates/proto/proto/channel.proto
message Channel {
uint64 id = 1;
string name = 2;
ChannelVisibility visibility = 3;
int32 channel_order = 4;
repeated uint64 parent_path = 5;
}
message ReorderChannel {
uint64 channel_id = 1;
enum Direction {
Up = 0;
Down = 1;
}
Direction direction = 2;
}
```
### Server-side Logic
The reordering is handled by swapping `channel_order` values between
adjacent channels:
```rust
// crates/collab/src/db/queries/channels.rs
pub async fn reorder_channel(
&self,
channel_id: ChannelId,
direction: proto::reorder_channel::Direction,
user_id: UserId,
) -> Result<Vec<Channel>> {
// Find the sibling channel to swap with
let sibling_channel = match direction {
proto::reorder_channel::Direction::Up => {
// 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?
}
// Similar logic for Down...
};
// Swap the channel_order values
let temp_order = channel.channel_order;
channel.channel_order = sibling_channel.channel_order;
sibling_channel.channel_order = temp_order;
}
```
### Client-side Sorting
Optimized the sorting algorithm to avoid O(n²) complexity:
```rust
// crates/collab/src/db/queries/channels.rs
// Pre-compute sort keys for efficient O(n log n) sorting
let mut channels_with_keys: Vec<(Vec<i32>, Channel)> = channels
.into_iter()
.map(|channel| {
let mut sort_key = Vec::with_capacity(channel.parent_path.len() + 1);
// Build sort key from parent path orders
for parent_id in &channel.parent_path {
sort_key.push(channel_order_map.get(parent_id).copied().unwrap_or(i32::MAX));
}
sort_key.push(channel.channel_order);
(sort_key, channel)
})
.collect();
channels_with_keys.sort_by(|a, b| a.0.cmp(&b.0));
```
### User Interface
Added keyboard shortcuts and proper context handling:
```json
// assets/keymaps/default-macos.json
{
"context": "CollabPanel && not_editing",
"bindings": {
"cmd-up": "collab_panel::MoveChannelUp",
"cmd-down": "collab_panel::MoveChannelDown"
}
}
```
The CollabPanel now properly sets context to distinguish between editing
and navigation modes:
```rust
// crates/collab_ui/src/collab_panel.rs
fn dispatch_context(&self, window: &Window, cx: &Context<Self>) -> 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
}
```
## Testing
Comprehensive tests were added to verify:
- Basic reordering functionality (up/down movement)
- Boundary conditions (first/last channels)
- Permission checks (non-admins cannot reorder)
- Ordering persistence across server restarts
- Correct broadcasting of changes to channel members
## Migration Strategy
Existing channels are assigned initial `channel_order` values based on
their current alphabetical sorting to maintain the familiar order users
expect:
```sql
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
);
```
## Future Enhancements
While this PR provides basic reordering functionality, potential future
improvements could include:
- Drag-and-drop reordering in the UI
- Bulk reordering operations
- Custom sorting strategies (by activity, creation date, etc.)
## Checklist
- [x] Database migration included
- [x] Tests added for new functionality
- [x] Keybindings work on macOS and Linux
- [x] Permissions properly enforced
- [x] Error handling implemented throughout
- [x] Manual testing completed
- [x] Documentation updated
---------
Co-authored-by: Mikayla Maki <mikayla.c.maki@gmail.com>
490 lines
14 KiB
Rust
490 lines
14 KiB
Rust
use crate::channel_chat::ChannelChatEvent;
|
||
|
||
use super::*;
|
||
use client::{Client, UserStore, test::FakeServer};
|
||
use clock::FakeSystemClock;
|
||
use gpui::{App, AppContext as _, Entity, SemanticVersion, TestAppContext};
|
||
use http_client::FakeHttpClient;
|
||
use rpc::proto::{self};
|
||
use settings::SettingsStore;
|
||
|
||
#[gpui::test]
|
||
fn test_update_channels(cx: &mut App) {
|
||
let channel_store = init_test(cx);
|
||
|
||
update_channels(
|
||
&channel_store,
|
||
proto::UpdateChannels {
|
||
channels: vec![
|
||
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,
|
||
},
|
||
],
|
||
..Default::default()
|
||
},
|
||
cx,
|
||
);
|
||
assert_channels(
|
||
&channel_store,
|
||
&[
|
||
//
|
||
(0, "b".to_string()),
|
||
(0, "a".to_string()),
|
||
],
|
||
cx,
|
||
);
|
||
|
||
update_channels(
|
||
&channel_store,
|
||
proto::UpdateChannels {
|
||
channels: vec![
|
||
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,
|
||
},
|
||
],
|
||
..Default::default()
|
||
},
|
||
cx,
|
||
);
|
||
assert_channels(
|
||
&channel_store,
|
||
&[
|
||
(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<T: Clone>(items: Vec<T>) -> Vec<Vec<T>> {
|
||
if items.len() == 1 {
|
||
vec![items]
|
||
} else {
|
||
let mut output: Vec<Vec<T>> = 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);
|
||
|
||
update_channels(
|
||
&channel_store,
|
||
proto::UpdateChannels {
|
||
channels: vec![
|
||
proto::Channel {
|
||
id: 0,
|
||
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()
|
||
},
|
||
cx,
|
||
);
|
||
// Sanity check
|
||
assert_channels(
|
||
&channel_store,
|
||
&[
|
||
//
|
||
(0, "a".to_string()),
|
||
(1, "b".to_string()),
|
||
(2, "c".to_string()),
|
||
],
|
||
cx,
|
||
);
|
||
|
||
update_channels(
|
||
&channel_store,
|
||
proto::UpdateChannels {
|
||
delete_channels: vec![1, 2],
|
||
..Default::default()
|
||
},
|
||
cx,
|
||
);
|
||
|
||
// Make sure that the 1/2/3 path is gone
|
||
assert_channels(&channel_store, &[(0, "a".to_string())], cx);
|
||
}
|
||
|
||
#[gpui::test]
|
||
async fn test_channel_messages(cx: &mut TestAppContext) {
|
||
let user_id = 5;
|
||
let channel_id = 5;
|
||
let channel_store = cx.update(init_test);
|
||
let client = channel_store.read_with(cx, |s, _| s.client());
|
||
let server = FakeServer::for_client(user_id, &client, cx).await;
|
||
|
||
// Get the available channels.
|
||
server.send(proto::UpdateChannels {
|
||
channels: vec![proto::Channel {
|
||
id: channel_id,
|
||
name: "the-channel".to_string(),
|
||
visibility: proto::ChannelVisibility::Members as i32,
|
||
parent_path: vec![],
|
||
channel_order: 1,
|
||
}],
|
||
..Default::default()
|
||
});
|
||
cx.executor().run_until_parked();
|
||
cx.update(|cx| {
|
||
assert_channels(&channel_store, &[(0, "the-channel".to_string())], cx);
|
||
});
|
||
|
||
let get_users = server.receive::<proto::GetUsers>().await.unwrap();
|
||
assert_eq!(get_users.payload.user_ids, vec![5]);
|
||
server.respond(
|
||
get_users.receipt(),
|
||
proto::UsersResponse {
|
||
users: vec![proto::User {
|
||
id: 5,
|
||
github_login: "nathansobo".into(),
|
||
avatar_url: "http://avatar.com/nathansobo".into(),
|
||
name: None,
|
||
email: None,
|
||
}],
|
||
},
|
||
);
|
||
|
||
// Join a channel and populate its existing messages.
|
||
let channel = channel_store.update(cx, |store, cx| {
|
||
let channel_id = store.ordered_channels().next().unwrap().1.id;
|
||
store.open_channel_chat(channel_id, cx)
|
||
});
|
||
let join_channel = server.receive::<proto::JoinChannelChat>().await.unwrap();
|
||
server.respond(
|
||
join_channel.receipt(),
|
||
proto::JoinChannelChatResponse {
|
||
messages: vec![
|
||
proto::ChannelMessage {
|
||
id: 10,
|
||
body: "a".into(),
|
||
timestamp: 1000,
|
||
sender_id: 5,
|
||
mentions: vec![],
|
||
nonce: Some(1.into()),
|
||
reply_to_message_id: None,
|
||
edited_at: None,
|
||
},
|
||
proto::ChannelMessage {
|
||
id: 11,
|
||
body: "b".into(),
|
||
timestamp: 1001,
|
||
sender_id: 6,
|
||
mentions: vec![],
|
||
nonce: Some(2.into()),
|
||
reply_to_message_id: None,
|
||
edited_at: None,
|
||
},
|
||
],
|
||
done: false,
|
||
},
|
||
);
|
||
|
||
cx.executor().start_waiting();
|
||
|
||
// Client requests all users for the received messages
|
||
let mut get_users = server.receive::<proto::GetUsers>().await.unwrap();
|
||
get_users.payload.user_ids.sort();
|
||
assert_eq!(get_users.payload.user_ids, vec![6]);
|
||
server.respond(
|
||
get_users.receipt(),
|
||
proto::UsersResponse {
|
||
users: vec![proto::User {
|
||
id: 6,
|
||
github_login: "maxbrunsfeld".into(),
|
||
avatar_url: "http://avatar.com/maxbrunsfeld".into(),
|
||
name: None,
|
||
email: None,
|
||
}],
|
||
},
|
||
);
|
||
|
||
let channel = channel.await.unwrap();
|
||
channel.update(cx, |channel, _| {
|
||
assert_eq!(
|
||
channel
|
||
.messages_in_range(0..2)
|
||
.map(|message| (message.sender.github_login.clone(), message.body.clone()))
|
||
.collect::<Vec<_>>(),
|
||
&[
|
||
("nathansobo".into(), "a".into()),
|
||
("maxbrunsfeld".into(), "b".into())
|
||
]
|
||
);
|
||
});
|
||
|
||
// Receive a new message.
|
||
server.send(proto::ChannelMessageSent {
|
||
channel_id,
|
||
message: Some(proto::ChannelMessage {
|
||
id: 12,
|
||
body: "c".into(),
|
||
timestamp: 1002,
|
||
sender_id: 7,
|
||
mentions: vec![],
|
||
nonce: Some(3.into()),
|
||
reply_to_message_id: None,
|
||
edited_at: None,
|
||
}),
|
||
});
|
||
|
||
// Client requests user for message since they haven't seen them yet
|
||
let get_users = server.receive::<proto::GetUsers>().await.unwrap();
|
||
assert_eq!(get_users.payload.user_ids, vec![7]);
|
||
server.respond(
|
||
get_users.receipt(),
|
||
proto::UsersResponse {
|
||
users: vec![proto::User {
|
||
id: 7,
|
||
github_login: "as-cii".into(),
|
||
avatar_url: "http://avatar.com/as-cii".into(),
|
||
name: None,
|
||
email: None,
|
||
}],
|
||
},
|
||
);
|
||
|
||
assert_eq!(
|
||
channel.next_event(cx).await,
|
||
ChannelChatEvent::MessagesUpdated {
|
||
old_range: 2..2,
|
||
new_count: 1,
|
||
}
|
||
);
|
||
channel.update(cx, |channel, _| {
|
||
assert_eq!(
|
||
channel
|
||
.messages_in_range(2..3)
|
||
.map(|message| (message.sender.github_login.clone(), message.body.clone()))
|
||
.collect::<Vec<_>>(),
|
||
&[("as-cii".into(), "c".into())]
|
||
)
|
||
});
|
||
|
||
// Scroll up to view older messages.
|
||
channel.update(cx, |channel, cx| {
|
||
channel.load_more_messages(cx).unwrap().detach();
|
||
});
|
||
let get_messages = server.receive::<proto::GetChannelMessages>().await.unwrap();
|
||
assert_eq!(get_messages.payload.channel_id, 5);
|
||
assert_eq!(get_messages.payload.before_message_id, 10);
|
||
server.respond(
|
||
get_messages.receipt(),
|
||
proto::GetChannelMessagesResponse {
|
||
done: true,
|
||
messages: vec![
|
||
proto::ChannelMessage {
|
||
id: 8,
|
||
body: "y".into(),
|
||
timestamp: 998,
|
||
sender_id: 5,
|
||
nonce: Some(4.into()),
|
||
mentions: vec![],
|
||
reply_to_message_id: None,
|
||
edited_at: None,
|
||
},
|
||
proto::ChannelMessage {
|
||
id: 9,
|
||
body: "z".into(),
|
||
timestamp: 999,
|
||
sender_id: 6,
|
||
nonce: Some(5.into()),
|
||
mentions: vec![],
|
||
reply_to_message_id: None,
|
||
edited_at: None,
|
||
},
|
||
],
|
||
},
|
||
);
|
||
|
||
assert_eq!(
|
||
channel.next_event(cx).await,
|
||
ChannelChatEvent::MessagesUpdated {
|
||
old_range: 0..0,
|
||
new_count: 2,
|
||
}
|
||
);
|
||
channel.update(cx, |channel, _| {
|
||
assert_eq!(
|
||
channel
|
||
.messages_in_range(0..2)
|
||
.map(|message| (message.sender.github_login.clone(), message.body.clone()))
|
||
.collect::<Vec<_>>(),
|
||
&[
|
||
("nathansobo".into(), "y".into()),
|
||
("maxbrunsfeld".into(), "z".into())
|
||
]
|
||
);
|
||
});
|
||
}
|
||
|
||
fn init_test(cx: &mut App) -> Entity<ChannelStore> {
|
||
let settings_store = SettingsStore::test(cx);
|
||
cx.set_global(settings_store);
|
||
release_channel::init(SemanticVersion::default(), cx);
|
||
client::init_settings(cx);
|
||
|
||
let clock = Arc::new(FakeSystemClock::new());
|
||
let http = FakeHttpClient::with_404_response();
|
||
let client = Client::new(clock, http.clone(), cx);
|
||
let user_store = cx.new(|cx| UserStore::new(client.clone(), cx));
|
||
|
||
client::init(&client, cx);
|
||
crate::init(&client, user_store, cx);
|
||
|
||
ChannelStore::global(cx)
|
||
}
|
||
|
||
fn update_channels(
|
||
channel_store: &Entity<ChannelStore>,
|
||
message: proto::UpdateChannels,
|
||
cx: &mut App,
|
||
) {
|
||
let task = channel_store.update(cx, |store, cx| store.update_channels(message, cx));
|
||
assert!(task.is_none());
|
||
}
|
||
|
||
#[track_caller]
|
||
fn assert_channels(
|
||
channel_store: &Entity<ChannelStore>,
|
||
expected_channels: &[(usize, String)],
|
||
cx: &mut App,
|
||
) {
|
||
let actual = channel_store.update(cx, |store, _| {
|
||
store
|
||
.ordered_channels()
|
||
.map(|(depth, channel)| (depth, channel.name.to_string()))
|
||
.collect::<Vec<_>>()
|
||
});
|
||
assert_eq!(actual, expected_channels);
|
||
}
|