From 0764dc3452961316674cc86827814b001d2d3c07 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 23 May 2023 16:01:47 +0200 Subject: [PATCH] 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. --- .../discourse/components/chat-channel.js | 53 +++++++++++++++---- .../discourse/lib/chat-messages-manager.js | 7 ++- .../spec/system/channel_info_pages_spec.rb | 2 +- .../system/move_message_to_channel_spec.rb | 4 -- 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index df6c7184f10..6f558a3902e 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/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(() => { - this._handleErrors(); - }) + .catch(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); + } } diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js b/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js index 1a12be16663..5ccaf95ec93 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-messages-manager.js +++ b/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 - .concat(messages) - .uniqBy("id") - .sortBy("createdAt"); + this.messages = new TrackedArray( + this.messages.concat(messages).uniqBy("id").sortBy("createdAt") + ); } findMessage(messageId) { diff --git a/plugins/chat/spec/system/channel_info_pages_spec.rb b/plugins/chat/spec/system/channel_info_pages_spec.rb index f7992326723..72516d8a85c 100644 --- a/plugins/chat/spec/system/channel_info_pages_spec.rb +++ b/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 diff --git a/plugins/chat/spec/system/move_message_to_channel_spec.rb b/plugins/chat/spec/system/move_message_to_channel_spec.rb index a1b10f7c149..fe16cfaaf3c 100644 --- a/plugins/chat/spec/system/move_message_to_channel_spec.rb +++ b/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