From a1b35601fcd209d83471e269e23b7c4f9a42465a Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Wed, 26 Apr 2023 18:18:23 +0200 Subject: [PATCH] FIX: Improve chat route cleanup (#20557) 1. `this.chat.activeChannel = null` was being done in twice 2. using `willTransition()` and checking transition.to.name prefix for route cleanup rather than using `deactivate()` was unnecessarily verbose and could be premature (if something aborted the transition you'd end up in a broken state) 3. `activeChannel` on Chat service can be null, check for that before accessing --- .../routes/chat-channel-decorator.js | 2 ++ .../discourse/routes/chat-channel.js | 27 ++++++------------- .../javascripts/discourse/routes/chat.js | 21 +++++---------- .../discourse/services/chat-state-manager.js | 12 ++++++--- .../unit/services/chat-state-manager-test.js | 5 ---- 5 files changed, 26 insertions(+), 41 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-decorator.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-decorator.js index e64ef877273..f6c1770e707 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel-decorator.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel-decorator.js @@ -23,6 +23,8 @@ export default function withChatChannel(extendedClass) { } afterModel(model) { + super.afterModel?.(...arguments); + this.controllerFor("chat-channel").set("targetMessageId", null); this.chat.activeChannel = model; diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js index f610f301c08..f4ecc9c275f 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js @@ -1,32 +1,21 @@ import DiscourseRoute from "discourse/routes/discourse"; import withChatChannel from "./chat-channel-decorator"; import { inject as service } from "@ember/service"; -import { action } from "@ember/object"; @withChatChannel export default class ChatChannelRoute extends DiscourseRoute { @service chat; @service chatStateManager; - @action - willTransition(transition) { + beforeModel() { + if (this.chat.activeChannel) { + // When moving between channels + this.#closeThread(); + } + } + + deactivate() { this.#closeThread(); - - if (transition?.to?.name === "chat.channel.index") { - const targetChannelId = transition?.to?.parent?.params?.channelId; - if ( - targetChannelId && - parseInt(targetChannelId, 10) !== this.chat.activeChannel.id - ) { - this.chat.activeChannel.messagesManager.clearMessages(); - } - } - - if (!transition?.to?.name?.startsWith("chat.")) { - this.chatStateManager.storeChatURL(); - this.chat.activeChannel = null; - this.chat.updatePresence(); - } } #closeThread() { diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat.js b/plugins/chat/assets/javascripts/discourse/routes/chat.js index 796e3e8f34e..d28562bb431 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat.js @@ -4,7 +4,6 @@ import { defaultHomepage } from "discourse/lib/utilities"; import { inject as service } from "@ember/service"; import { scrollTop } from "discourse/mixins/scroll-top"; import { schedule } from "@ember/runloop"; -import { action } from "@ember/object"; export default class ChatRoute extends DiscourseRoute { @service chat; @@ -65,23 +64,17 @@ export default class ChatRoute extends DiscourseRoute { }); } - deactivate() { + deactivate(transition) { + const url = this.router.urlFor(transition.from.name); + this.chatStateManager.storeChatURL(url); + + this.chat.activeChannel = null; + this.chat.updatePresence(); + schedule("afterRender", () => { document.body.classList.remove("has-full-page-chat"); document.documentElement.classList.remove("has-full-page-chat"); scrollTop(); }); } - - @action - willTransition(transition) { - if (!transition?.to?.name?.startsWith("chat.channel")) { - this.chat.activeChannel = null; - } - - if (!transition?.to?.name?.startsWith("chat.")) { - this.chatStateManager.storeChatURL(); - this.chat.updatePresence(); - } - } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js index 79f67e15a2e..6dd753c1692 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-state-manager.js @@ -113,11 +113,17 @@ export default class ChatStateManager extends Service { } storeAppURL(url = null) { - this._appURL = url || this.router.currentURL; + if (url) { + this._appURL = url; + } else if (this.router.currentURL?.startsWith("/chat")) { + this._appURL = "/"; + } else { + this._appURL = this.router.currentURL; + } } - storeChatURL(url = null) { - this._chatURL = url || this.router.currentURL; + storeChatURL(url) { + this._chatURL = url; } get lastKnownAppURL() { diff --git a/plugins/chat/test/javascripts/unit/services/chat-state-manager-test.js b/plugins/chat/test/javascripts/unit/services/chat-state-manager-test.js index 7c7ba806b2e..1242caea51a 100644 --- a/plugins/chat/test/javascripts/unit/services/chat-state-manager-test.js +++ b/plugins/chat/test/javascripts/unit/services/chat-state-manager-test.js @@ -53,11 +53,6 @@ module( test("lastKnownChatURL", function (assert) { assert.strictEqual(this.subject.lastKnownChatURL, "/chat"); - sinon.stub(this.subject.router, "currentURL").value("/foo"); - this.subject.storeChatURL(); - - assert.strictEqual(this.subject.lastKnownChatURL, "/foo"); - this.subject.storeChatURL("/bar"); assert.strictEqual(this.subject.lastKnownChatURL, "/bar");