FIX: Improve the reliability of the unread channel keyboard shortcuts (#29814)

Additionally, a bunch of related flaky tests were unflakified.

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

@@ -201,6 +201,16 @@ export default class ChatChannel {
     return this.meta.can_join_chat_channel;
   }
 
+  get hasUnread() {
+    return (
+      this.tracking.unreadCount +
+        this.tracking.mentionCount +
+        this.tracking.watchedThreadsUnreadCount +
+        this.threadsManager.unreadThreadCount >
+      0
+    );
+  }
+
   async stageMessage(message) {
     message.id = guid();
     message.staged = true;

plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js

@@ -133,14 +133,7 @@ export default class ChatChannelsManager extends Service {
 
   @cached
   get publicMessageChannelsWithActivity() {
-    return this.publicMessageChannels.filter(
+    return this.publicMessageChannels.filter((channel) => channel.hasUnread);
-      (channel) =>
-        channel.tracking.unreadCount +
-          channel.tracking.mentionCount +
-          channel.tracking.watchedThreadsUnreadCount +
-          channel.threadsManager.unreadThreadCount >
-        0
-    );
   }
 
   get publicMessageChannelsByActivity() {
@@ -159,14 +152,7 @@ export default class ChatChannelsManager extends Service {
 
   @cached
   get directMessageChannelsWithActivity() {
-    return this.directMessageChannels.filter(
+    return this.directMessageChannels.filter((channel) => channel.hasUnread);
-      (channel) =>
-        channel.tracking.unreadCount +
-          channel.tracking.mentionCount +
-          channel.tracking.watchedThreadsUnreadCount +
-          channel.threadsManager.unreadThreadCount >
-        0
-    );
   }
 
   get truncatedDirectMessageChannels() {

plugins/chat/assets/javascripts/discourse/services/chat.js

@@ -291,6 +291,33 @@ export default class Chat extends Service {
         this.chatChannelsManager.publicMessageChannelsWithActivity;
       directChannels =
         this.chatChannelsManager.directMessageChannelsWithActivity;
+
+      // If the active channel has no unread messages, we need to manually insert it into
+      // the list, so we can find the next/previous unread channel.
+      if (!activeChannel.hasUnread) {
+        const allChannels = activeChannel.isDirectMessageChannel
+          ? this.chatChannelsManager.directMessageChannels
+          : this.chatChannelsManager.publicMessageChannels;
+
+        // Find the ID of the channel before the active channel, which is unread
+        let checkChannelIndex =
+          allChannels.findIndex((c) => c.id === activeChannel.id) - 1;
+
+        // If we get back to the start of the list, we can stop
+        while (checkChannelIndex >= 0) {
+          if (allChannels[checkChannelIndex].hasUnread) {
+            break;
+          }
+          checkChannelIndex--;
+        }
+
+        // Insert the active channel after unread channel we found (or at the start of the list)
+        if (activeChannel.isDirectMessageChannel) {
+          directChannels.splice(checkChannelIndex + 1, 0, activeChannel);
+        } else {
+          publicChannels.splice(checkChannelIndex + 1, 0, activeChannel);
+        }
+      }
     } else {
       publicChannels = this.chatChannelsManager.publicMessageChannels;
       directChannels = this.chatChannelsManager.directMessageChannels;

plugins/chat/spec/system/page_objects/sidebar/sidebar.rb

@@ -64,14 +64,24 @@ module PageObjects
       end
 
       def has_unread_channel?(channel)
-        has_css?(".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread")
+        has_css?(
+          ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix:is(.unread, .urgent)",
+        )
       end
 
       def has_no_unread_channel?(channel)
         has_no_css?(
-          ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread",
+          ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix:is(.unread, .urgent)",
         )
       end
+
+      def has_active_channel?(channel)
+        has_css?(".sidebar-section-link.channel-#{channel.id}.active")
+      end
+
+      def has_no_active_channel?(channel)
+        has_no_css?(".sidebar-section-link.channel-#{channel.id}.active")
+      end
     end
   end
 end

plugins/chat/spec/system/shortcuts/sidebar_spec.rb

@@ -4,6 +4,7 @@ RSpec.describe "Shortcuts | sidebar", type: :system do
   fab!(:current_user) { Fabricate(:admin) }
 
   let(:chat) { PageObjects::Pages::Chat.new }
+  let(:sidebar_page) { PageObjects::Pages::Sidebar.new }
 
   before do
     SiteSetting.navigation_menu = "sidebar"
@@ -22,41 +23,43 @@ RSpec.describe "Shortcuts | sidebar", type: :system do
         visit("/")
         find("body").send_keys(%i[alt arrow_down])
 
-        expect(page).to have_no_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_no_active_channel(channel_1)
-        expect(page).to have_no_selector(".channel-#{dm_channel_1.id}.active")
+        expect(sidebar_page).to have_no_active_channel(dm_channel_1)
       end
     end
 
     context "when on chat page" do
       it "navigates through the channels" do
         chat.visit_channel(channel_1)
-
+        expect(sidebar_page).to have_active_channel(channel_1)
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
 
         find("body").send_keys(%i[alt arrow_down])
 
-        expect(page).to have_selector(".channel-#{dm_channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(dm_channel_1)
 
         find("body").send_keys(%i[alt arrow_down])
 
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(channel_1)
 
         find("body").send_keys(%i[alt arrow_up])
 
-        expect(page).to have_selector(".channel-#{dm_channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(dm_channel_1)
       end
     end
   end
 
   context "when using Alt+Shift+Up/Down arrows" do
-    fab!(:channel_1) { Fabricate(:chat_channel) }
+    fab!(:channel_1) { Fabricate(:chat_channel, name: "Channel 1") }
-    fab!(:channel_2) { Fabricate(:chat_channel) }
+    fab!(:channel_2) { Fabricate(:chat_channel, name: "Channel 2") }
+    fab!(:channel_3) { Fabricate(:chat_channel, name: "Channel 3") }
     fab!(:dm_channel_1) { Fabricate(:direct_message_channel, users: [current_user]) }
-    fab!(:dm_channel_2) { Fabricate(:direct_message_channel, users: [current_user]) }
+    fab!(:other_user) { Fabricate(:user) }
+    fab!(:dm_channel_2) { Fabricate(:direct_message_channel, users: [current_user, other_user]) }
 
     before do
       channel_1.add(current_user)
       channel_2.add(current_user)
+      channel_3.add(current_user)
     end
 
     context "when on homepage" do
@@ -64,34 +67,37 @@ RSpec.describe "Shortcuts | sidebar", type: :system do
         visit("/")
         find("body").send_keys(%i[alt shift arrow_down])
 
-        expect(page).to have_no_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_no_active_channel(channel_1)
-        expect(page).to have_no_selector(".channel-#{channel_2.id}.active")
+        expect(sidebar_page).to have_no_active_channel(channel_2)
-        expect(page).to have_no_selector(".channel-#{dm_channel_1.id}.active")
+        expect(sidebar_page).to have_no_active_channel(dm_channel_1)
-        expect(page).to have_no_selector(".channel-#{dm_channel_2.id}.active")
+        expect(sidebar_page).to have_no_active_channel(dm_channel_2)
       end
     end
 
     context "when on chat page" do
       it "does nothing when no channels have activity" do
         chat.visit_channel(channel_1)
-
+        expect(sidebar_page).to have_active_channel(channel_1)
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
 
         find("body").send_keys(%i[alt shift arrow_down])
 
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(channel_1)
 
         find("body").send_keys(%i[alt shift arrow_down])
 
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(channel_1)
 
         find("body").send_keys(%i[alt shift arrow_up])
 
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(channel_1)
       end
 
       it "navigates through the channels with activity" do
+        chat.visit_channel(channel_1)
+        expect(sidebar_page).to have_active_channel(channel_1)
+
         Fabricate(:chat_message, chat_channel: channel_2, message: "hello!", use_service: true)
+        expect(sidebar_page).to have_unread_channel(channel_2)
 
         Fabricate(
           :chat_message,
@@ -99,18 +105,17 @@ RSpec.describe "Shortcuts | sidebar", type: :system do
           message: "hello here!",
           use_service: true,
         )
-
+        expect(sidebar_page).to have_unread_channel(dm_channel_2)
-        chat.visit_channel(channel_1)
-
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
 
         find("body").send_keys(%i[alt shift arrow_down])
 
-        expect(page).to have_selector(".channel-#{channel_2.id}.active")
+        expect(sidebar_page).to have_active_channel(channel_2)
+        expect(sidebar_page).to have_no_unread_channel(channel_2)
 
         find("body").send_keys(%i[alt shift arrow_down])
 
-        expect(page).to have_selector(".channel-#{dm_channel_2.id}.active")
+        expect(sidebar_page).to have_active_channel(dm_channel_2)
+        expect(sidebar_page).to have_no_unread_channel(dm_channel_2)
 
         Fabricate(
           :chat_message,
@@ -118,16 +123,36 @@ RSpec.describe "Shortcuts | sidebar", type: :system do
           message: "hello again!",
           use_service: true,
         )
+        expect(sidebar_page).to have_unread_channel(channel_1)
 
         find("body").send_keys(%i[alt shift arrow_up])
 
-        expect(page).to have_selector(".channel-#{channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(channel_1)
+        expect(sidebar_page).to have_no_unread_channel(channel_1)
 
         Fabricate(:chat_message, chat_channel: dm_channel_1, message: "bye now!", use_service: true)
+        expect(sidebar_page).to have_unread_channel(dm_channel_1)
 
         find("body").send_keys(%i[alt shift arrow_up])
 
-        expect(page).to have_selector(".channel-#{dm_channel_1.id}.active")
+        expect(sidebar_page).to have_active_channel(dm_channel_1)
+        expect(sidebar_page).to have_no_unread_channel(dm_channel_1)
+      end
+
+      it "remembers where the current channel is, even if that channel is unread" do
+        chat.visit_channel(channel_2)
+        expect(sidebar_page).to have_active_channel(channel_2)
+
+        Fabricate(:chat_message, chat_channel: channel_1, message: "hello!", use_service: true)
+        expect(sidebar_page).to have_unread_channel(channel_1)
+
+        Fabricate(:chat_message, chat_channel: channel_3, message: "hello!", use_service: true)
+        expect(sidebar_page).to have_unread_channel(channel_3)
+
+        find("body").send_keys(%i[alt shift arrow_down])
+
+        expect(sidebar_page).to have_active_channel(channel_3)
+        expect(sidebar_page).to have_no_unread_channel(channel_3)
       end
     end
   end