FIX: prevents subscribing with an old id (#21509)

This issue was for example possibly causing the last visit indicator to be reset by `sent` messages events.

The following was happening:
- a user (bob) had a last message bus ID of 1 on a channel (id:1) subscription
- bob then go to another channel (id:2), unsubscribing from updates of channel (id:1)
- another user (laura) then send messages to channel (id:1)
- bob goes back to channel (id:1)

At this point we we doing in the same sequence:
- loading channel with messages, getting a new last message bus id
- subscribing to updates using the last known message bus id

Most of the times we were lucky enough for this to work (no events while away, or just got the new id in time...) but it was also very likely to do a double fetch of messages as MessageBus would think we were late.

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

@@ -85,7 +85,7 @@ export default class ChatLivePane extends Component {
   teardownListeners() {
     document.removeEventListener("scroll", this._forceBodyScroll);
     removeOnPresenceChange(this.onPresenceChangeCallback);
-    this._unsubscribeToUpdates(this._loadedChannelId);
+    this.unsubscribeToUpdates(this._loadedChannelId);
     this.requestedTargetMessageId = null;
     cancel(this._laterComputeHandler);
   }
@@ -111,7 +111,7 @@ export default class ChatLivePane extends Component {
     this.args.channel?.clearMessages();
 
     if (this._loadedChannelId !== this.args.channel?.id) {
-      this._unsubscribeToUpdates(this._loadedChannelId);
+      this.unsubscribeToUpdates(this._loadedChannelId);
       this.chatChannelPane.selectingMessages = false;
       this.chatChannelComposer.message =
         this.args.channel.draft ||
@@ -123,7 +123,6 @@ export default class ChatLivePane extends Component {
     }
 
     this.loadMessages();
-    this._subscribeToUpdates(this.args.channel);
   }
 
   @action
@@ -215,6 +214,7 @@ export default class ChatLivePane extends Component {
         this.loadingMorePast = false;
         this.fillPaneAttempt();
         this.updateLastReadMessage();
+        this.subscribeToUpdates(this.args.channel);
       });
   }
 
@@ -766,7 +766,7 @@ export default class ChatLivePane extends Component {
     });
   }
 
-  _unsubscribeToUpdates(channelId) {
+  unsubscribeToUpdates(channelId) {
     if (!channelId) {
       return;
     }
@@ -775,12 +775,12 @@ export default class ChatLivePane extends Component {
     this.messageBus.unsubscribe(`/chat/${channelId}`, this.onMessage);
   }
 
-  _subscribeToUpdates(channel) {
+  subscribeToUpdates(channel) {
     if (!channel) {
       return;
     }
 
-    this._unsubscribeToUpdates(channel.id);
+    this.unsubscribeToUpdates(channel.id);
     this.messageBus.subscribe(
       `/chat/${channel.id}`,
       this.onMessage,

plugins/chat/spec/system/dates_separators_spec.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+RSpec.describe "Dates separators", type: :system, js: true do
+  fab!(:current_user) { Fabricate(:user) }
+  fab!(:channel_1) { Fabricate(:chat_channel) }
+
+  let(:chat_page) { PageObjects::Pages::Chat.new }
+  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+
+  before do
+    chat_system_bootstrap
+    channel_1.add(current_user)
+    sign_in(current_user)
+  end
+
+  context "when today separator is out of screen" do
+    before do
+      20.times { Fabricate(:chat_message, chat_channel: channel_1, created_at: 1.day.ago) }
+      25.times { Fabricate(:chat_message, chat_channel: channel_1) }
+    end
+
+    it "shows it as a sticky date" do
+      chat_page.visit_channel(channel_1)
+
+      expect(page.find(".chat-message-separator__text-container.is-pinned")).to have_content(
+        I18n.t("js.chat.chat_message_separator.today"),
+      )
+      expect(page).to have_css(
+        ".chat-message-separator__text-container:not(.is-pinned)",
+        visible: :hidden,
+        text:
+          "#{I18n.t("js.chat.chat_message_separator.yesterday")} - #{I18n.t("js.chat.last_visit")}",
+      )
+    end
+  end
+
+  context "when receiving messages on a different channel" do
+    fab!(:channel_2) { Fabricate(:chat_channel) }
+    fab!(:user_1) { Fabricate(:user) }
+
+    before do
+      channel_2.add(current_user)
+      channel_1.add(user_1)
+    end
+
+    it "doesn't impact the last visit separator" do
+      chat_page.visit_channel(channel_1)
+      channel_page.send_message("message1")
+      chat_page.visit_channel(channel_2)
+
+      using_session(:user_1) do |session|
+        sign_in(user_1)
+        chat_page.visit_channel(channel_1)
+        channel_page.send_message("message2")
+        session.quit
+      end
+
+      chat_page.visit_channel(channel_1)
+
+      expect(page).to have_css(
+        ".chat-message-separator__text-container",
+        text: I18n.t("js.chat.last_visit"),
+      )
+    end
+  end
+end

plugins/chat/spec/system/sticky_date_spec.rb

@@ -1,32 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe "Sticky date", type: :system, js: true do
-  fab!(:current_user) { Fabricate(:user) }
-  fab!(:channel_1) { Fabricate(:category_channel) }
-
-  let(:chat_page) { PageObjects::Pages::Chat.new }
-
-  before do
-    chat_system_bootstrap
-    channel_1.add(current_user)
-    20.times { Fabricate(:chat_message, chat_channel: channel_1, created_at: 1.day.ago) }
-    25.times { Fabricate(:chat_message, chat_channel: channel_1) }
-    sign_in(current_user)
-  end
-
-  context "when today separator is out of screen" do
-    it "shows it as a sticky date" do
-      chat_page.visit_channel(channel_1)
-
-      expect(page.find(".chat-message-separator__text-container.is-pinned")).to have_content(
-        I18n.t("js.chat.chat_message_separator.today"),
-      )
-      expect(page).to have_css(
-        ".chat-message-separator__text-container:not(.is-pinned)",
-        visible: :hidden,
-        text:
-          "#{I18n.t("js.chat.chat_message_separator.yesterday")} - #{I18n.t("js.chat.last_visit")}",
-      )
-    end
-  end
-end