FEATURE: Sort thread list by unread threads first (#22272)

* FEATURE: Sort thread list by unread threads first

This commit changes the thread list to show the threads that
have unread messages at the top of the list sorted by the
last reply date + time, then all other threads sorted by
last reply date + time.

This also fixes some issues by removing the last_reply
relationship on the thread, which did not work for complex
querying scenarios because its order would be discarded.

* FIX: Various fixes for thread list loading

* Use the channel.threadsManager and find the channel first rather
  than use activeChannel in the threads manager, otherwise we may
  be looking at differenct channels.
* Look at threadsManager directly instead of storing result for threads
  list otherwise it can get out of sync because of replace: true in
  other places we are loading threads into the store.
* Fix sorting for thread.last_reply, needed a resort.

plugins/chat/app/models/chat/thread.rb

@@ -24,15 +24,25 @@ module Chat
              class_name: "Chat::Message"
     has_many :user_chat_thread_memberships
 
-    # Since the `replies` for the thread can all be deleted, to avoid errors
-    # in lists and previews of the thread, we can consider the original message
-    # as the last "reply" in this case, so we don't exclude that here.
-    has_one :last_reply, -> { order("created_at DESC, id DESC") }, class_name: "Chat::Message"
-
     enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false
 
     validates :title, length: { maximum: Chat::Thread::MAX_TITLE_LENGTH }
 
+    # Since the `replies` for the thread can all be deleted, to avoid errors
+    # in lists and previews of the thread, we can consider the original message
+    # as the last "reply" in this case, so we don't exclude that here.
+    #
+    # This is a manual getter/setter so we can avoid N1 queries. This used to be
+    # a has_one relationship on the model, but that has some awkward behaviour
+    # and still caused N1s, and ordering was not applied in complex AR queries.
+    def last_reply
+      @last_reply ||= self.chat_messages.reorder("created_at DESC, id DESC").first
+    end
+
+    def last_reply=(message)
+      @last_reply = message
+    end
+
     def add(user)
       Chat::UserChatThreadMembership.find_or_create_by!(user: user, thread: self)
     end

plugins/chat/app/services/chat/lookup_channel_threads.rb

@@ -15,6 +15,8 @@ module Chat
   class LookupChannelThreads
     include Service::Base
 
+    MAX_THREADS = 50
+
     # @!method call(channel_id:, guardian:)
     #   @param [Integer] channel_id
     #   @param [Guardian] guardian
@@ -54,41 +56,54 @@ module Chat
     end
 
     def fetch_threads(guardian:, channel:, **)
-      Chat::Thread
-        .strict_loading
-        .includes(
-          :channel,
-          last_reply: %i[user uploads],
-          original_message_user: :user_status,
-          original_message: [
-            :chat_webhook_event,
-            :chat_mentions,
-            :chat_channel,
-            user: :user_status,
-          ],
-        )
-        .joins(:chat_messages, :user_chat_thread_memberships)
-        .joins(
-          "LEFT JOIN chat_messages original_messages ON chat_threads.original_message_id = original_messages.id",
-        )
-        .where(
-          "chat_threads.channel_id = :channel_id AND chat_messages.chat_channel_id = :channel_id",
-          channel_id: channel.id,
-        )
-        .where("user_chat_thread_memberships.user_id = ?", guardian.user.id)
-        .where(
-          "user_chat_thread_memberships.notification_level IN (?)",
-          [
-            Chat::UserChatThreadMembership.notification_levels[:normal],
-            Chat::UserChatThreadMembership.notification_levels[:tracking],
-          ],
-        )
-        .where(
-          "original_messages.deleted_at IS NULL AND chat_messages.deleted_at IS NULL AND original_messages.id IS NOT NULL",
-        )
-        .group("chat_threads.id")
-        .order("MAX(chat_messages.created_at) DESC")
-        .limit(50)
+      read_threads = []
+
+      unread_threads =
+        threads_query(guardian, channel)
+          .where(<<~SQL)
+            user_chat_thread_memberships_chat_threads.last_read_message_id IS NULL
+              OR tracked_threads_subquery.latest_message_id > user_chat_thread_memberships_chat_threads.last_read_message_id
+          SQL
+          .order("tracked_threads_subquery.latest_message_created_at DESC")
+          .limit(MAX_THREADS)
+          .to_a
+
+      # We do this to avoid having to query additional threads if the user
+      # already has a lot of unread threads.
+      if unread_threads.length < MAX_THREADS
+        final_limit = MAX_THREADS - unread_threads.length
+        read_threads =
+          threads_query(guardian, channel)
+            .where(<<~SQL)
+              tracked_threads_subquery.latest_message_id <= user_chat_thread_memberships_chat_threads.last_read_message_id
+            SQL
+            .order("tracked_threads_subquery.latest_message_created_at DESC")
+            .limit(final_limit)
+            .to_a
+      end
+
+      threads = unread_threads + read_threads
+
+      last_replies =
+        Chat::Message
+          .strict_loading
+          .includes(:user, :uploads)
+          .from(<<~SQL)
+            (
+              SELECT thread_id, MAX(created_at) AS latest_created_at, MAX(id) AS latest_message_id
+              FROM chat_messages
+              WHERE thread_id IN (#{threads.map(&:id).join(",")})
+              GROUP BY thread_id
+            ) AS last_replies_subquery
+          SQL
+          .joins(
+            "INNER JOIN chat_messages ON chat_messages.id = last_replies_subquery.latest_message_id",
+          )
+          .index_by(&:thread_id)
+
+      threads.each { |thread| thread.last_reply = last_replies[thread.id] }
+
+      threads
     end
 
     def fetch_tracking(guardian:, threads:, **)
@@ -107,5 +122,55 @@ module Chat
           user_id: guardian.user.id,
         )
     end
+
+    def threads_query(guardian, channel)
+      Chat::Thread
+        .strict_loading
+        .includes(
+          :channel,
+          :user_chat_thread_memberships,
+          original_message_user: :user_status,
+          original_message: [
+            :chat_webhook_event,
+            :chat_mentions,
+            :chat_channel,
+            user: :user_status,
+          ],
+        )
+        .joins(
+          "JOIN (#{tracked_threads_subquery(guardian, channel)}) tracked_threads_subquery
+              ON tracked_threads_subquery.thread_id = chat_threads.id",
+        )
+        .joins(:user_chat_thread_memberships)
+        .where(user_chat_thread_memberships_chat_threads: { user_id: guardian.user.id })
+    end
+
+    def tracked_threads_subquery(guardian, channel)
+      Chat::Thread
+        .joins(:chat_messages, :user_chat_thread_memberships)
+        .joins(
+          "LEFT JOIN chat_messages original_messages ON chat_threads.original_message_id = original_messages.id",
+        )
+        .where(user_chat_thread_memberships: { user_id: guardian.user.id })
+        .where(
+          "chat_threads.channel_id = :channel_id AND chat_messages.chat_channel_id = :channel_id",
+          channel_id: channel.id,
+        )
+        .where(
+          "user_chat_thread_memberships.notification_level IN (?)",
+          [
+            Chat::UserChatThreadMembership.notification_levels[:normal],
+            Chat::UserChatThreadMembership.notification_levels[:tracking],
+          ],
+        )
+        .where(
+          "original_messages.deleted_at IS NULL AND chat_messages.deleted_at IS NULL AND original_messages.id IS NOT NULL",
+        )
+        .group("chat_threads.id")
+        .select(
+          "chat_threads.id AS thread_id, MAX(chat_messages.created_at) AS latest_message_created_at, MAX(chat_messages.id) AS latest_message_id",
+        )
+        .to_sql
+    end
   end
 end

plugins/chat/app/services/chat/lookup_thread.rb

@@ -44,7 +44,6 @@ module Chat
     def fetch_thread(contract:, **)
       Chat::Thread.includes(
         :channel,
-        last_reply: :user,
         original_message_user: :user_status,
         original_message: :chat_webhook_event,
       ).find_by(id: contract.thread_id, channel_id: contract.channel_id)

plugins/chat/assets/javascripts/discourse/components/chat-channel.js

@@ -213,6 +213,12 @@ export default class ChatLivePane extends Component {
               thread,
               { replace: true }
             );
+
+            this.#preloadThreadTrackingState(
+              storedThread,
+              result.tracking.thread_tracking
+            );
+
             const originalMessage = messages.findBy(
               "id",
               storedThread.originalMessage.id
@@ -323,6 +329,12 @@ export default class ChatLivePane extends Component {
               thread,
               { replace: true }
             );
+
+            this.#preloadThreadTrackingState(
+              storedThread,
+              result.tracking.thread_tracking
+            );
+
             const originalMessage = messages.findBy(
               "id",
               storedThread.originalMessage.id
@@ -1098,4 +1110,15 @@ export default class ChatLivePane extends Component {
     cancel(this._laterComputeHandler);
     cancel(this._debounceFetchMessagesHandler);
   }
+
+  #preloadThreadTrackingState(storedThread, threadTracking) {
+    if (!threadTracking[storedThread.id]) {
+      return;
+    }
+
+    storedThread.tracking.unreadCount =
+      threadTracking[storedThread.id].unread_count;
+    storedThread.tracking.mentionCount =
+      threadTracking[storedThread.id].mention_count;
+  }
 }

plugins/chat/assets/javascripts/discourse/components/chat/thread-list.hbs

@@ -13,7 +13,7 @@
       {{#if this.loading}}
         {{loading-spinner size="medium"}}
       {{else}}
-        {{#each this.threads as |thread|}}
+        {{#each this.sortedThreads as |thread|}}
           <Chat::ThreadList::Item @thread={{thread}} />
         {{else}}
           <div class="chat-thread-list__no-threads">

plugins/chat/assets/javascripts/discourse/components/chat/thread-list.js

@@ -6,9 +6,49 @@ import { inject as service } from "@ember/service";
 export default class ChatThreadList extends Component {
   @service chat;
 
-  @tracked threads;
   @tracked loading = true;
 
+  // NOTE: This replicates sort logic from the server. We need this because
+  // the thread unread count + last reply date + time update when new messages
+  // are sent to the thread, and we want the list to react in realtime to this.
+  get sortedThreads() {
+    if (!this.args.channel.threadsManager.threads) {
+      return [];
+    }
+
+    return this.args.channel.threadsManager.threads.sort((threadA, threadB) => {
+      // If both are unread we just want to sort by last reply date + time descending.
+      if (threadA.tracking.unreadCount && threadB.tracking.unreadCount) {
+        if (
+          threadA.preview.lastReplyCreatedAt >
+          threadB.preview.lastReplyCreatedAt
+        ) {
+          return -1;
+        } else {
+          return 1;
+        }
+      }
+
+      // If one is unread and the other is not, we want to sort the unread one first.
+      if (threadA.tracking.unreadCount) {
+        return -1;
+      }
+
+      if (threadB.tracking.unreadCount) {
+        return 1;
+      }
+
+      // If both are read, we want to sort by last reply date + time descending.
+      if (
+        threadA.preview.lastReplyCreatedAt > threadB.preview.lastReplyCreatedAt
+      ) {
+        return -1;
+      } else {
+        return 1;
+      }
+    });
+  }
+
   get shouldRender() {
     return !!this.args.channel;
   }
@@ -16,21 +56,13 @@ export default class ChatThreadList extends Component {
   @action
   loadThreads() {
     this.loading = true;
-    this.args.channel.threadsManager
-      .index(this.args.channel.id)
-      .then((result) => {
-        if (result.meta.channel_id === this.args.channel.id) {
-          this.threads = result.threads;
-        }
-      })
-      .finally(() => {
-        this.loading = false;
-      });
+    this.args.channel.threadsManager.index(this.args.channel.id).finally(() => {
+      this.loading = false;
+    });
   }
 
   @action
   teardown() {
     this.loading = true;
-    this.threads = null;
   }
 }

plugins/chat/assets/javascripts/discourse/lib/chat-threads-manager.js

@@ -38,21 +38,21 @@ export default class ChatThreadsManager {
   }
 
   async index(channelId) {
-    return this.#loadIndex(channelId).then((result) => {
-      const threads = result.threads.map((thread) => {
-        return this.chat.activeChannel.threadsManager.store(
-          this.chat.activeChannel,
-          thread,
-          { replace: true }
+    return this.chatChannelsManager.find(channelId).then((channel) => {
+      return this.#loadIndex(channelId).then((result) => {
+        const threads = result.threads.map((thread) => {
+          return channel.threadsManager.store(channel, thread, {
+            replace: true,
+          });
+        });
+
+        this.chatTrackingStateManager.setupChannelThreadState(
+          channel,
+          result.tracking
         );
+
+        return { threads, meta: result.meta };
       });
-
-      this.chatTrackingStateManager.setupChannelThreadState(
-        this.chat.activeChannel,
-        result.tracking
-      );
-
-      return { threads, meta: result.meta };
     });
   }
 

plugins/chat/assets/javascripts/discourse/models/chat-thread-preview.js

@@ -21,8 +21,9 @@ export default class ChatThreadPreview {
 
     this.replyCount = args.reply_count || args.replyCount || 0;
     this.lastReplyId = args.last_reply_id || args.lastReplyId;
-    this.lastReplyCreatedAt =
-      args.last_reply_created_at || args.lastReplyCreatedAt;
+    this.lastReplyCreatedAt = new Date(
+      args.last_reply_created_at || args.lastReplyCreatedAt
+    );
     this.lastReplyExcerpt = args.last_reply_excerpt || args.lastReplyExcerpt;
     this.lastReplyUser = args.last_reply_user || args.lastReplyUser;
     this.participantCount =

plugins/chat/assets/javascripts/discourse/services/chat-tracking-state-manager.js

@@ -34,10 +34,6 @@ export default class ChatTrackingStateManager extends Service {
 
   setupChannelThreadState(channel, threadTracking) {
     channel.threadsManager.threads.forEach((thread) => {
-      // TODO (martin) Since we didn't backfill data for thread membership,
-      // there are cases where we are getting threads the user "participated"
-      // in but don't have tracking state for them. We need a migration to
-      // backfill this data.
       if (threadTracking[thread.id.toString()]) {
         this.#setState(thread, threadTracking[thread.id.toString()]);
       }

plugins/chat/lib/chat/message_creator.rb

@@ -233,7 +233,8 @@ module Chat
       return if resolved_thread.blank?
 
       resolved_thread.increment_replies_count_cache
-      resolved_thread.add(@user)
+      current_user_thread_membership = resolved_thread.add(@user)
+      current_user_thread_membership.update!(last_read_message_id: @chat_message.id)
 
       if resolved_thread.original_message_user != @user
         resolved_thread.add(resolved_thread.original_message_user)

plugins/chat/spec/components/chat/message_creator_spec.rb

@@ -779,6 +779,26 @@ describe Chat::MessageCreator do
           }.not_to change { Chat::UserChatThreadMembership.count }
         end
 
+        it "sets the last_read_message_id of the existing UserChatThreadMembership for the user to the new message id" do
+          message = Fabricate(:chat_message, thread: existing_thread)
+          membership =
+            Fabricate(
+              :user_chat_thread_membership,
+              user: user1,
+              thread: existing_thread,
+              last_read_message_id: message.id,
+            )
+          new_message =
+            described_class.create(
+              chat_channel: public_chat_channel,
+              user: user1,
+              content: "this is a message",
+              thread_id: existing_thread.id,
+            ).chat_message
+
+          expect(membership.reload.last_read_message_id).to eq(new_message.id)
+        end
+
         it "errors when the thread ID is for a different channel" do
           other_channel_thread = Fabricate(:chat_thread, channel: Fabricate(:chat_channel))
           result =

plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb

@@ -49,10 +49,16 @@ RSpec.describe Chat::LookupChannelThreads do
           expect(result).to be_a_success
         end
 
-        it "returns the threads ordered by the last thread the current user posted in" do
+        it "returns the threads ordered by the last reply created_at date and time for the thread" do
           expect(result.threads.map(&:id)).to eq([thread_3.id, thread_1.id, thread_2.id])
         end
 
+        it "orders threads with unread messages at the top even if their last reply created_at date and time is older" do
+          unread_message = Fabricate(:chat_message, chat_channel: channel, thread: thread_2)
+          unread_message.update!(created_at: 2.days.ago)
+          expect(result.threads.map(&:id)).to eq([thread_2.id, thread_3.id, thread_1.id])
+        end
+
         it "does not return threads where the original message is trashed" do
           thread_1.original_message.trash!
           expect(result.threads.map(&:id)).to eq([thread_3.id, thread_2.id])

plugins/chat/spec/system/message_thread_indicator_spec.rb

@@ -126,7 +126,10 @@ describe "Thread indicator for chat messages", type: :system do
       )
     end
 
-    xit "shows an excerpt of the last reply in the thread" do
+    it "shows an excerpt of the last reply in the thread" do
+      thread_1.last_reply.update!(message: "test for excerpt")
+      thread_1.last_reply.rebake!
+
       chat_page.visit_channel(channel)
 
       excerpt_text = thread_excerpt(thread_1.last_reply)
@@ -136,10 +139,12 @@ describe "Thread indicator for chat messages", type: :system do
       ).to have_content(excerpt_text)
     end
 
-    xit "updates the last reply excerpt and participants when a new message is added to the thread" do
+    it "updates the last reply excerpt and participants when a new message is added to the thread" do
       new_user = Fabricate(:user)
       chat_system_user_bootstrap(user: new_user, channel: channel)
       original_last_reply = thread_1.replies.last
+      original_last_reply.update!(message: "test for excerpt")
+      original_last_reply.rebake!
 
       chat_page.visit_channel(channel)
 
@@ -163,7 +168,8 @@ describe "Thread indicator for chat messages", type: :system do
         new_user,
       )
 
-      excerpt_text = thread_excerpt(thread_1.replies.where(user: new_user).first)
+      new_user_reply = thread_1.replies.where(user: new_user).first
+      excerpt_text = thread_excerpt(new_user_reply)
 
       expect(
         channel_page.message_thread_indicator(thread_1.original_message).excerpt,

plugins/chat/spec/system/thread_tracking/drawer_spec.rb

@@ -20,10 +20,10 @@ describe "Thread tracking state | drawer", type: :system do
   end
 
   context "when the user has unread messages for a thread" do
-    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
-    fab!(:message_2) do
+    fab!(:message_1) do
       Fabricate(:chat_message, chat_channel: channel, thread: thread, user: current_user)
     end
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
 
     it "shows the count of threads with unread messages on the thread list button" do
       visit("/")
@@ -53,7 +53,7 @@ describe "Thread tracking state | drawer", type: :system do
     end
 
     it "shows unread indicators for the header icon and the list when a new unread arrives" do
-      message_1.trash!
+      thread.membership_for(current_user).update!(last_read_message_id: message_2.id)
       visit("/")
       chat_page.open_from_header
       drawer_page.open_channel(channel)

plugins/chat/spec/system/thread_tracking/full_page_spec.rb

@@ -19,10 +19,10 @@ describe "Thread tracking state | full page", type: :system do
   end
 
   context "when the user has unread messages for a thread" do
-    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
-    fab!(:message_2) do
+    fab!(:message_1) do
       Fabricate(:chat_message, chat_channel: channel, thread: thread, user: current_user)
     end
+    fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
 
     it "shows the count of threads with unread messages on the thread list button" do
       chat_page.visit_channel(channel)
@@ -45,7 +45,7 @@ describe "Thread tracking state | full page", type: :system do
     end
 
     it "shows unread indicators for the header of the list when a new unread arrives" do
-      message_1.trash!
+      thread.membership_for(current_user).update!(last_read_message_id: message_2.id)
       chat_page.visit_channel(channel)
       channel_page.open_thread_list
       expect(thread_list_page).to have_no_unread_item(thread.id)