From b3c94839edbc675dd5d72b7476783d5a0c47ee5f Mon Sep 17 00:00:00 2001
From: David Battersby <info@davidbattersby.com>
Date: Thu, 5 Dec 2024 13:33:55 +0400
Subject: [PATCH] FIX: improve chat channel sorting for DMs (#30124)

This change sorts unread channels in descending order based on last message date, so channels with the latest activity will always appear at the top. It also adds some improvements for sorting channels with unread threads, now when multiple channels have unread threads, they will be sorted by last thread reply date to ensure more active channels rise to the top.

For DM channels, the order is now:

- Urgent (green badge) - unread messages, mentions and unread watched threads (most recent activity at top)
- Unread (blue badge) - unread tracked threads (most recent thread reply at top)
- Everything else (most recent message at top)
---
 .../components/chat-channel-metadata.gjs      | 13 ++-
 .../discourse/models/chat-channel.js          | 10 ++
 .../services/chat-channels-manager.js         | 31 ++++--
 .../spec/system/list_channels/drawer_spec.rb  | 96 ++++++++++++-------
 4 files changed, 106 insertions(+), 44 deletions(-)

diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.gjs
index e628c932e87..681c472b8c0 100644
--- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.gjs
+++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-metadata.gjs
@@ -3,7 +3,11 @@ import { i18n } from "discourse-i18n";
 
 export default class ChatChannelMetadata extends Component {
   get lastMessageFormattedDate() {
-    return moment(this.args.channel.lastMessage.createdAt).calendar(null, {
+    const lastMessageDate = this.showThreadUnreadDate
+      ? this.args.channel.lastUnreadThreadDate
+      : this.args.channel.lastMessage.createdAt;
+
+    return moment(lastMessageDate).calendar(null, {
       sameDay: "LT",
       lastDay: `[${i18n("chat.dates.yesterday")}]`,
       lastWeek: "dddd",
@@ -11,6 +15,13 @@ export default class ChatChannelMetadata extends Component {
     });
   }
 
+  get showThreadUnreadDate() {
+    return (
+      this.args.channel.lastUnreadThreadDate >
+      this.args.channel.lastMessage.createdAt
+    );
+  }
+
   <template>
     <div class="chat-channel__metadata">
       {{#if @channel.lastMessage}}
diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js
index 440f9f84a62..4f32e8af9f6 100644
--- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js
+++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js
@@ -119,6 +119,16 @@ export default class ChatChannel {
     return this.threadsManager.unreadThreadCount;
   }
 
+  get lastUnreadThreadDate() {
+    if (this.unreadThreadsCount === 0) {
+      return this.lastMessage.createdAt;
+    }
+
+    return Array.from(this.threadsManager.unreadThreadOverview.values())
+      .sort((a, b) => b - a)
+      .pop();
+  }
+
   get watchedThreadsUnreadCount() {
     return this.threadsManager.threads.reduce((unreadCount, thread) => {
       return unreadCount + thread.tracking.watchedThreadsUnreadCount;
diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js
index 66a134ef1d4..8825dd76c22 100644
--- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js
+++ b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js
@@ -263,20 +263,33 @@ export default class ChatChannelsManager extends Service {
         b.tracking.mentionCount +
         b.tracking.watchedThreadsUnreadCount;
 
-      if (aUrgent > 0 || bUrgent > 0) {
-        return aUrgent > bUrgent ? -1 : 1;
-      }
+      const aUnread = a.unreadThreadsCountSinceLastViewed;
+      const bUnread = b.unreadThreadsCountSinceLastViewed;
 
-      if (
-        a.unreadThreadsCountSinceLastViewed > 0 ||
-        b.unreadThreadsCountSinceLastViewed > 0
-      ) {
-        return a.unreadThreadsCountSinceLastViewed >
-          b.unreadThreadsCountSinceLastViewed
+      // if both channels have urgent count, sort by last message date
+      if (aUrgent > 0 && bUrgent > 0) {
+        return new Date(a.lastMessage.createdAt) >
+          new Date(b.lastMessage.createdAt)
           ? -1
           : 1;
       }
 
+      // otherwise prioritize channel with urgent count
+      if (aUrgent > 0 || bUrgent > 0) {
+        return aUrgent > bUrgent ? -1 : 1;
+      }
+
+      // if both channels have unread threads, sort by last thread reply date
+      if (aUnread > 0 && bUnread > 0) {
+        return a.lastUnreadThreadDate > b.lastUnreadThreadDate ? -1 : 1;
+      }
+
+      // otherwise prioritize channel with unread thread count
+      if (aUnread > 0 || bUnread > 0) {
+        return aUnread > bUnread ? -1 : 1;
+      }
+
+      // read channels are sorted by last message date
       return new Date(a.lastMessage.createdAt) >
         new Date(b.lastMessage.createdAt)
         ? -1
diff --git a/plugins/chat/spec/system/list_channels/drawer_spec.rb b/plugins/chat/spec/system/list_channels/drawer_spec.rb
index 757d76baad4..62defaf8706 100644
--- a/plugins/chat/spec/system/list_channels/drawer_spec.rb
+++ b/plugins/chat/spec/system/list_channels/drawer_spec.rb
@@ -145,45 +145,73 @@ RSpec.describe "List channels | Drawer", type: :system do
         expect(drawer_page).to have_channel_at_position(dm_channel_3, 4)
       end
 
-      it "sorts channels with threads by urgency" do
-        drawer_page.visit_index
-        drawer_page.click_direct_messages
+      context "with unread threads" do
+        fab!(:message_1) do
+          Fabricate(
+            :chat_message,
+            chat_channel: dm_channel_3,
+            user: current_user,
+            use_service: true,
+          )
+        end
+        fab!(:thread_1) do
+          Fabricate(
+            :chat_thread,
+            channel: dm_channel_3,
+            original_message: message_1,
+            use_service: true,
+          )
+        end
+        fab!(:message_2) do
+          Fabricate(
+            :chat_message,
+            chat_channel: dm_channel_4,
+            user: current_user,
+            use_service: true,
+          )
+        end
+        fab!(:thread_2) do
+          Fabricate(
+            :chat_thread,
+            channel: dm_channel_4,
+            original_message: message_2,
+            use_service: true,
+          )
+        end
 
-        Fabricate(
-          :chat_thread,
-          notification_level: :watching,
-          original_message:
-            Fabricate(
-              :chat_message,
-              chat_channel: dm_channel_4,
-              user: current_user,
-              use_service: true,
-            ),
-          with_replies: 2,
-          use_service: true,
-        )
+        before do
+          dm_channel_3.membership_for(current_user).mark_read!(message_1.id)
+          dm_channel_4.membership_for(current_user).mark_read!(message_2.id)
 
-        Fabricate(
-          :chat_thread,
-          original_message:
-            Fabricate(
-              :chat_message,
-              chat_channel: dm_channel_3,
-              user: current_user,
-              use_service: true,
-            ),
-          with_replies: 2,
-          use_service: true,
-        )
+          drawer_page.visit_index
+          drawer_page.click_direct_messages
+        end
 
-        expect(drawer_page).to have_channel_at_position(dm_channel_4, 1)
-        expect(drawer_page).to have_urgent_channel(dm_channel_4)
+        it "sorts channels with unread threads by last reply" do
+          Fabricate(:chat_message, thread: thread_1, user: user_2, use_service: true)
+          Fabricate(:chat_message, thread: thread_2, user: user_3, use_service: true)
 
-        expect(drawer_page).to have_channel_at_position(dm_channel_3, 2)
-        expect(drawer_page).to have_unread_channel(dm_channel_3)
+          expect(drawer_page).to have_channel_at_position(dm_channel_4, 1)
+          expect(drawer_page).to have_unread_channel(dm_channel_4)
 
-        expect(drawer_page).to have_channel_at_position(dm_channel_1, 3)
-        expect(drawer_page).to have_channel_at_position(dm_channel_2, 4)
+          expect(drawer_page).to have_channel_at_position(dm_channel_3, 2)
+          expect(drawer_page).to have_unread_channel(dm_channel_3)
+        end
+
+        it "sorts channels with unread threads by importance" do
+          thread_1.membership_for(current_user).update!(
+            notification_level: ::Chat::NotificationLevels.all[:watching],
+          )
+
+          Fabricate(:chat_message, thread: thread_1, user: user_2, use_service: true)
+          Fabricate(:chat_message, thread: thread_2, user: user_3, use_service: true)
+
+          expect(drawer_page).to have_channel_at_position(dm_channel_3, 1)
+          expect(drawer_page).to have_urgent_channel(dm_channel_3)
+
+          expect(drawer_page).to have_channel_at_position(dm_channel_4, 2)
+          expect(drawer_page).to have_unread_channel(dm_channel_4)
+        end
       end
     end
   end