From b4406861aecdb14fb26dc2fa919a363f0dc9c320 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Wed, 27 Nov 2024 17:33:31 +0100 Subject: [PATCH] FIX: simplify desktop notifications behavior (#29957) Historically the behavior of this file has been complexified to attempt to answer this use case: A user has two tabs open, tab 1 is on a topic, tab 2 is on a chat channel. If your active tab is tab 1 and someones sends you a mention in chat. We will show a desktop notification, but in which tab the channel should open if you click it? The changes made years ago said: in tab 2. I think this is complexifying too much this codepath and is also confusing. You might wonder why this discourse notification you clicked opened in some of your 50 tabs in the background when you had a discourse tab active currently in front of you. Moreover, a recent change has made the notification to only happen on desktop, but all the subscription stuff was happening regardless of mobile or desktop. --- .../services/chat-notification-manager.js | 146 +++--------------- .../javascripts/discourse/services/chat.js | 4 +- plugins/chat/plugin.rb | 11 -- 3 files changed, 20 insertions(+), 141 deletions(-) diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-notification-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-notification-manager.js index 74a8f483546..850e6d2894d 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-notification-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-notification-manager.js @@ -3,165 +3,57 @@ import { alertChannel, onNotification as onDesktopNotification, } from "discourse/lib/desktop-notifications"; -import { withPluginApi } from "discourse/lib/plugin-api"; import { isTesting } from "discourse-common/config/environment"; import { bind } from "discourse-common/utils/decorators"; export default class ChatNotificationManager extends Service { - @service presence; @service chat; - @service chatChannelsManager; - @service chatStateManager; @service currentUser; @service appEvents; @service site; - _subscribedToCore = true; - _subscribedToChat = false; - _countChatInDocTitle = true; - willDestroy() { super.willDestroy(...arguments); - if (!this._shouldRun()) { + if (!this.#shouldRun) { return; } - this._chatPresenceChannel.off( - "change", - this._subscribeToCorrectNotifications - ); - this._chatPresenceChannel.unsubscribe(); - this._chatPresenceChannel.leave(); - - this._corePresenceChannel.off( - "change", - this._subscribeToCorrectNotifications - ); - this._corePresenceChannel.unsubscribe(); - this._corePresenceChannel.leave(); + this.messageBus.unsubscribe(this.messageBusChannel, this.onMessage); } start() { - if (!this._shouldRun()) { + if (!this.#shouldRun) { return; } - this.set( - "_chatPresenceChannel", - this.presence.getChannel(`/chat-user/chat/${this.currentUser.id}`) - ); - this.set( - "_corePresenceChannel", - this.presence.getChannel(`/chat-user/core/${this.currentUser.id}`) - ); - this._chatPresenceChannel.subscribe(); - this._corePresenceChannel.subscribe(); - - withPluginApi("0.12.1", (api) => { - api.onPageChange(this._pageChanged); - }); - - this._pageChanged(); - - this._chatPresenceChannel.on( - "change", - this._subscribeToCorrectNotifications - ); - this._corePresenceChannel.on( - "change", - this._subscribeToCorrectNotifications - ); + this.messageBus.subscribe(this.messageBusChannel, this.onMessage); } - shouldCountChatInDocTitle() { - return this._countChatInDocTitle; - } - - @bind - _pageChanged() { - if (this.chatStateManager.isActive) { - this._chatPresenceChannel.enter({ onlyWhileActive: false }); - this._corePresenceChannel.leave(); - } else { - this._chatPresenceChannel.leave(); - this._corePresenceChannel.enter({ onlyWhileActive: false }); - } - } - - _coreAlertChannel() { - return alertChannel(this.currentUser); - } - - _chatAlertChannel() { + get messageBusChannel() { return `/chat${alertChannel(this.currentUser)}`; } - @bind - _subscribeToCorrectNotifications() { - const oneTabForEachOpen = - this._chatPresenceChannel.count > 0 && - this._corePresenceChannel.count > 0; - if (oneTabForEachOpen) { - this.chatStateManager.isActive - ? this._subscribeToChat({ only: true }) - : this._subscribeToCore({ only: true }); - } else { - this._subscribeToBoth(); - } - } - - _subscribeToBoth() { - this._subscribeToChat(); - this._subscribeToCore(); - } - - _subscribeToChat(opts = { only: false }) { - this.set("_countChatInDocTitle", true); - - if (!this._subscribedToChat) { - this.messageBus.subscribe(this._chatAlertChannel(), this.onMessage); - this.set("_subscribedToChat", true); - } - - if (opts.only && this._subscribedToCore) { - this.messageBus.unsubscribe(this._coreAlertChannel(), this.onMessage); - this.set("_subscribedToCore", false); - } - } - - _subscribeToCore(opts = { only: false }) { - if (opts.only) { - this.set("_countChatInDocTitle", false); - } - if (!this._subscribedToCore) { - this.messageBus.subscribe(this._coreAlertChannel(), this.onMessage); - this.set("_subscribedToCore", true); - } - - if (opts.only && this._subscribedToChat) { - this.messageBus.unsubscribe(this._chatAlertChannel(), this.onMessage); - this.set("_subscribedToChat", false); - } - } - @bind async onMessage(data) { - if (data.channel_id === this.chat.activeChannel?.id) { + // if the user is currently focused on this tab and channel, + // we don't want to show a desktop notification + if ( + this.session.hasFocus && + data.channel_id === this.chat.activeChannel?.id + ) { return; } - if (this.site.desktopView) { - return onDesktopNotification( - data, - this.siteSettings, - this.currentUser, - this.appEvents - ); - } + return onDesktopNotification( + data, + this.siteSettings, + this.currentUser, + this.appEvents + ); } - _shouldRun() { - return this.chat.userCanChat && !isTesting(); + get #shouldRun() { + return this.site.desktopView && this.chat.userCanChat && !isTesting(); } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index da8ea4f4a97..1fe55408fa9 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -273,9 +273,7 @@ export default class Chat extends Service { } getDocumentTitleCount() { - return this.chatNotificationManager.shouldCountChatInDocTitle() - ? this.chatTrackingStateManager.allChannelUrgentCount - : 0; + return this.chatTrackingStateManager.allChannelUrgentCount; } switchChannelUpOrDown(direction, unreadOnly = false) { diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb index 08b2b1475c5..9deed76a1c6 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -331,17 +331,6 @@ after_initialize do nil end - register_presence_channel_prefix("chat-user") do |channel_name| - if user_id = channel_name[%r{/chat-user/(chat|core)/(\d+)}, 2] - user = User.find(user_id) - config = PresenceChannel::Config.new - config.allowed_user_ids = [user.id] - config - end - rescue ActiveRecord::RecordNotFound - nil - end - register_push_notification_filter do |user, payload| if user.user_option.only_chat_push_notifications && user.user_option.chat_enabled payload[:notification_type].in?(::Notification.types.values_at(:chat_mention, :chat_message))