FIX: cancel fetching messages after channel change (#21689)

This issue was especially visible in tests. the `@debounce(100)` was not cancelled when changing channel which was causing 404s as we were trying to load messages on a channel which was deleted as the channel has been destroyed at the end of the test.

This is still not a perfect solution, as we can only cancel the start of `fetchMessages`, but we can't cancel the actual `chatApi.channel` request which result can potentially happens after channel changed, which we try to mitigate with various checks on to ensure visible channel == loaded messages channel.

This commit also tries to make handler naming and cancelling more consistent.

<!-- NOTE: All pull requests should have tests (rspec in Ruby, qunit in JavaScript). If your code does not include test coverage, please include an explanation of why it was omitted. -->

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

@@ -20,6 +20,7 @@ import {
 } from "discourse/lib/user-presence";
 import isZoomed from "discourse/plugins/chat/discourse/lib/zoom-check";
 import { tracked } from "@glimmer/tracking";
+import discourseDebounce from "discourse-common/lib/debounce";
 
 const PAGE_SIZE = 50;
 const PAST = "past";
@@ -83,11 +84,11 @@ export default class ChatLivePane extends Component {
 
   @action
   teardownListeners() {
+    this.#cancelHandlers();
     document.removeEventListener("scroll", this._forceBodyScroll);
     removeOnPresenceChange(this.onPresenceChangeCallback);
     this.unsubscribeToUpdates(this._loadedChannelId);
     this.requestedTargetMessageId = null;
-    cancel(this._laterComputeHandler);
   }
 
   @action
@@ -104,6 +105,8 @@ export default class ChatLivePane extends Component {
 
   @action
   updateChannel() {
+    this.#cancelHandlers();
+
     this.loadedOnce = false;
 
     // Technically we could keep messages to avoid re-fetching them, but
@@ -138,7 +141,7 @@ export default class ChatLivePane extends Component {
     if (this.requestedTargetMessageId) {
       this.highlightOrFetchMessage(this.requestedTargetMessageId);
     } else {
-      this.fetchMessages({ fetchFromLastMessage: false });
+      this.debounceFetchMessages({ fetchFromLastMessage: false });
     }
   }
 
@@ -149,7 +152,15 @@ export default class ChatLivePane extends Component {
     }
   }
 
-  @debounce(100)
+  debounceFetchMessages(options) {
+    this._debounceFetchMessagesHandler = discourseDebounce(
+      this,
+      this.fetchMessages,
+      options,
+      100
+    );
+  }
+
   fetchMessages(options = {}) {
     if (this._selfDeleted) {
       return;
@@ -292,9 +303,7 @@ export default class ChatLivePane extends Component {
           this.scrollToMessage(messages[0].id, { position: "end" });
         }
       })
-      .catch(() => {
+      .catch(this._handleErrors)
-        this._handleErrors();
-      })
       .finally(() => {
         this[loadingMoreKey] = false;
         this.fillPaneAttempt();
@@ -386,7 +395,7 @@ export default class ChatLivePane extends Component {
       });
       this.requestedTargetMessageId = null;
     } else {
-      this.fetchMessages();
+      this.debounceFetchMessages();
     }
   }
 
@@ -518,7 +527,7 @@ export default class ChatLivePane extends Component {
 
   @action
   computeScrollState() {
-    cancel(this.onScrollEndedHandler);
+    cancel(this._onScrollEndedHandler);
 
     if (!this.scrollable) {
       return;
@@ -536,7 +545,11 @@ export default class ChatLivePane extends Component {
       this.onScrollEnded();
     } else {
       this.isScrolling = true;
-      this.onScrollEndedHandler = discourseLater(this, this.onScrollEnded, 150);
+      this._onScrollEndedHandler = discourseLater(
+        this,
+        this.onScrollEnded,
+        150
+      );
     }
   }
 
@@ -815,17 +828,29 @@ export default class ChatLivePane extends Component {
 
   _fetchAndScrollToLatest() {
     this.loadedOnce = false;
-    return this.fetchMessages({
+    return this.debounceFetchMessages({
       fetchFromLastMessage: true,
     });
   }
 
+  @bind
   _handleErrors(error) {
     switch (error?.jqXHR?.status) {
       case 429:
-      case 404:
         popupAjaxError(error);
         break;
+      case 404:
+        // avoids handling 404 errors from a channel
+        // that is not the current one, this is very likely in tests
+        // which will destroy the channel after the test is done
+        if (
+          this.args.channel?.id &&
+          error.jqXHR?.requestedUrl ===
+            `/chat/api/channels/${this.args.channel.id}`
+        ) {
+          popupAjaxError(error);
+        }
+        break;
       default:
         throw error;
     }
@@ -1015,4 +1040,10 @@ export default class ChatLivePane extends Component {
     // - 5.0 to account for rounding errors, especially on firefox
     return rect.bottom - 5.0 <= containerRect.bottom;
   }
+
+  #cancelHandlers() {
+    cancel(this._onScrollEndedHandler);
+    cancel(this._laterComputeHandler);
+    cancel(this._debounceFetchMessagesHandler);
+  }
 }

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

@@ -24,10 +24,9 @@ export default class ChatMessagesManager {
       message.manager = this;
     });
 
-    this.messages = this.messages
+    this.messages = new TrackedArray(
-      .concat(messages)
+      this.messages.concat(messages).uniqBy("id").sortBy("createdAt")
-      .uniqBy("id")
+    );
-      .sortBy("createdAt");
   }
 
   findMessage(messageId) {

plugins/chat/spec/system/channel_info_pages_spec.rb

@@ -8,8 +8,8 @@ RSpec.describe "Info pages", type: :system, js: true do
 
   before do
     chat_system_bootstrap
-    sign_in(current_user)
     channel_1.add(current_user)
+    sign_in(current_user)
   end
 
   context "when visiting from browse page" do

plugins/chat/spec/system/move_message_to_channel_spec.rb

@@ -83,10 +83,6 @@ RSpec.describe "Move message to channel", type: :system, js: true do
         find("[data-value='#{channel_2.id}']").click
         click_button(I18n.t("js.chat.move_to_channel.confirm_move"))
 
-        expect(page).to have_content(message_1.message)
-
-        chat.visit_channel(channel_1)
-
         expect(channel).to have_deleted_message(message_1)
       end
     end