From 03d32f26bbda90597e8443de1a930858c52f34ec Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Fri, 23 Dec 2022 19:48:14 +0100 Subject: [PATCH] FIX: correctly handles navigating to a message (#19614) Recent changes surfaced the various issues with this codepath: - we were not correctly reseting `messageLookup` leading to us trying to scroll to a non existing message in the view - we were calling markAsRead which would scroll to the bottom, even if we had a target message - we were not debouncing fetchMessages, which could cause multiple reload of the messages when loading it with a targetMessageId: first fetch from last read and then immediately fetch from targetMessageId - other naming inconsistencies - not handling drawer This commit also adds tests for classic scenarios related to this use case. --- .../discourse/components/chat-drawer.js | 12 +- .../discourse/components/chat-live-pane.js | 34 ++--- .../discourse/routes/chat-channel.js | 25 ++-- .../javascripts/discourse/services/chat.js | 3 +- .../spec/system/navigating_to_message_spec.rb | 140 ++++++++++++++++++ plugins/chat/spec/system/navigation_spec.rb | 27 ---- .../spec/system/page_objects/chat/chat.rb | 4 + 7 files changed, 186 insertions(+), 59 deletions(-) create mode 100644 plugins/chat/spec/system/navigating_to_message_spec.rb diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js b/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js index 35dab11417d..df5fb52819e 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js @@ -7,7 +7,7 @@ import { LIST_VIEW, } from "discourse/plugins/chat/discourse/services/chat"; import { equal } from "@ember/object/computed"; -import { cancel, next, throttle } from "@ember/runloop"; +import { cancel, next, schedule, throttle } from "@ember/runloop"; import { inject as service } from "@ember/service"; export default Component.extend({ @@ -224,10 +224,18 @@ export default Component.extend({ return this.chatChannelsManager .find(route.params.channelId) .then((channel) => { - this.chat.set("messageId", route.queryParams.messageId); this.chat.setActiveChannel(channel); this.set("view", CHAT_VIEW); this.appEvents.trigger("chat:float-toggled", false); + + if (route.queryParams.messageId) { + schedule("afterRender", () => { + this.appEvents.trigger( + "chat-live-pane:highlight-message", + route.queryParams.messageId + ); + }); + } }); } }, diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js index c60f7434d73..8b01739743b 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js @@ -5,6 +5,7 @@ import Component from "@ember/component"; import discourseComputed, { afterRender, bind, + debounce, observes, } from "discourse-common/utils/decorators"; import discourseDebounce from "discourse-common/lib/debounce"; @@ -171,7 +172,6 @@ export default Component.extend({ this._super(...arguments); this.currentUserTimezone = this.currentUser?.user_option.timezone; - this.set("targetMessageId", this.chat.messageId); if ( this.chatChannel?.id && @@ -227,6 +227,7 @@ export default Component.extend({ } }, + @debounce(100) fetchMessages(channel, options = {}) { this.set("loading", true); @@ -256,8 +257,9 @@ export default Component.extend({ } this.setMessageProps(messages, fetchingFromLastRead); - if (this.targetMessageId) { - this.highlightOrFetchMessage(this.targetMessageId); + if (options.fetchFromLastMessage) { + this.set("stickyScroll", true); + this._stickScrollToBottom(); } this._focusComposer(); @@ -268,7 +270,6 @@ export default Component.extend({ return; } - this.chat.set("messageId", null); this.set("loading", false); }); }); @@ -407,18 +408,19 @@ export default Component.extend({ setMessageProps(messages, fetchingFromLastRead) { this._unloadedReplyIds = []; + this.messageLookup = {}; + const meta = messages.resultSetMeta; this.setProperties({ messages: this._prepareMessages(messages), details: { chat_channel_id: this.chatChannel.id, chatable_type: this.chatChannel.chatable_type, - can_delete_self: messages.resultSetMeta.can_delete_self, - can_delete_others: messages.resultSetMeta.can_delete_others, - can_flag: messages.resultSetMeta.can_flag, - user_silenced: messages.resultSetMeta.user_silenced, - can_moderate: messages.resultSetMeta.can_moderate, - channel_message_bus_last_id: - messages.resultSetMeta.channel_message_bus_last_id, + can_delete_self: meta.can_delete_self, + can_delete_others: meta.can_delete_others, + can_flag: meta.can_flag, + user_silenced: meta.user_silenced, + can_moderate: meta.can_moderate, + channel_message_bus_last_id: meta.channel_message_bus_last_id, }, registeredChatChannelId: this.chatChannel.id, }); @@ -434,6 +436,7 @@ export default Component.extend({ position: "top", autoExpand: true, }); + this.set("targetMessageId", null); } else if (fetchingFromLastRead) { this._markLastReadMessage(); @@ -566,7 +569,7 @@ export default Component.extend({ this.messages.findIndex((m) => m.id === lastReadId) || 0; let newestUnreadMessage = this.messages[indexOfLastReadMessage + 1]; - if (newestUnreadMessage) { + if (newestUnreadMessage && !this.targetMessageId) { newestUnreadMessage.set("newestMessage", true); next(() => this.scrollToMessage(newestUnreadMessage.id)); @@ -1522,13 +1525,6 @@ export default Component.extend({ _fetchAndScrollToLatest() { return this.fetchMessages(this.chatChannel, { fetchFromLastMessage: true, - }).then(() => { - if (this._selfDeleted) { - return; - } - - this.set("stickyScroll", true); - this._stickScrollToBottom(); }); }, diff --git a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js index 7e49d1f764e..152348e1f99 100644 --- a/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/routes/chat-channel.js @@ -1,6 +1,8 @@ import DiscourseRoute from "discourse/routes/discourse"; import { inject as service } from "@ember/service"; import slugifyChannel from "discourse/plugins/chat/discourse/lib/slugify-channel"; +import { action } from "@ember/object"; +import { schedule } from "@ember/runloop"; export default class ChatChannelRoute extends DiscourseRoute { @service chat; @@ -14,19 +16,24 @@ export default class ChatChannelRoute extends DiscourseRoute { afterModel(model) { this.chat.setActiveChannel(model); - const queryParams = this.paramsFor(this.routeName); + const { channelTitle, messageId } = this.paramsFor(this.routeName); const slug = slugifyChannel(model); - if (queryParams?.channelTitle !== slug) { - this.router.replaceWith("chat.channel.index", model.id, slug); + if (channelTitle !== slug) { + this.router.replaceWith("chat.channel.index", model.id, slug, { + queryParams: { messageId }, + }); } } - setupController(controller) { - super.setupController(...arguments); - - if (controller.messageId) { - this.chat.set("messageId", controller.messageId); - this.controller.set("messageId", null); + @action + didTransition() { + const { channelId, messageId } = this.paramsFor(this.routeName); + if (channelId && messageId) { + schedule("afterRender", () => { + this.chat.openChannelAtMessage(channelId, messageId); + this.controller.set("messageId", null); // clear the query param + }); } + return true; } } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index a1d14431121..0cb1c56ace3 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -38,11 +38,10 @@ export default class Chat extends Service { activeChannel = null; cook = null; - - messageId = null; presenceChannel = null; sidebarActive = false; isNetworkUnreliable = false; + @and("currentUser.has_chat_enabled", "siteSettings.chat_enabled") userCanChat; @computed("currentUser.staff", "currentUser.groups.[]") diff --git a/plugins/chat/spec/system/navigating_to_message_spec.rb b/plugins/chat/spec/system/navigating_to_message_spec.rb new file mode 100644 index 00000000000..f23097373e4 --- /dev/null +++ b/plugins/chat/spec/system/navigating_to_message_spec.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +RSpec.describe "Navigating to message", type: :system, js: true do + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:category_channel) } + fab!(:first_message) { Fabricate(:chat_message, chat_channel: channel_1) } + + let(:chat_page) { PageObjects::Pages::Chat.new } + let(:chat_drawer_page) { PageObjects::Pages::ChatDrawer.new } + let(:link) { "My favorite message" } + + before do + chat_system_bootstrap + channel_1.add(current_user) + 75.times { Fabricate(:chat_message, chat_channel: channel_1) } + sign_in(current_user) + end + + context "when in full page mode" do + before { chat_page.prefers_full_page } + + context "when clicking a link containing a message id" do + fab!(:topic_1) { Fabricate(:topic) } + + before do + Fabricate( + :post, + topic: topic_1, + raw: + "#{link}", + ) + end + + it "highlights the correct message" do + visit("/t/-/#{topic_1.id}") + click_link(link) + + expect(page).to have_css( + ".chat-message-container.highlighted[data-id='#{first_message.id}']", + ) + end + end + + context "when clicking a link to a message from the current channel" do + before do + Fabricate(:chat_message, chat_channel: channel_1, message: "[#{link}](/chat/channel/#{channel_1.id}/-?messageId=#{first_message.id})") + end + + it "highglights the correct message" do + chat_page.visit_channel(channel_1) + click_link(link) + + expect(page).to have_css(".chat-message-container.highlighted[data-id='#{first_message.id}']") + end + + it "highlights the correct message after using the bottom arrow" do + chat_page.visit_channel(channel_1) + click_link(link) + click_link(I18n.t("js.chat.scroll_to_bottom")) + click_link(link) + + expect(page).to have_css(".chat-message-container.highlighted[data-id='#{first_message.id}']") + end + end + + context "when clicking a link to a message from another channel" do + fab!(:channel_2) { Fabricate(:category_channel) } + + before do + Fabricate(:chat_message, chat_channel: channel_2, message: "[#{link}](/chat/channel/#{channel_1.id}/-?messageId=#{first_message.id})") + channel_2.add(current_user) + end + + it "highglights the correct message" do + chat_page.visit_channel(channel_2) + click_link(link) + + expect(page).to have_css(".chat-message-container.highlighted[data-id='#{first_message.id}']") + end + end + + context "when navigating directly to a message link" do + it "highglights the correct message" do + visit("/chat/channel/#{channel_1.id}/-?messageId=#{first_message.id}") + + expect(page).to have_css(".chat-message-container.highlighted[data-id='#{first_message.id}']") + end + end + end + + context "when in drawer" do + context "when clicking a link containing a message id" do + fab!(:topic_1) { Fabricate(:topic) } + + before do + Fabricate( + :post, + topic: topic_1, + raw: + "#{link}", + ) + end + + it "highlights correct message" do + visit("/t/-/#{topic_1.id}") + click_link(link) + + expect(page).to have_css( + ".chat-drawer.is-expanded .chat-message-container.highlighted[data-id='#{first_message.id}']", + ) + end + end + + context "when clicking a link to a message from the current channel" do + before do + Fabricate(:chat_message, chat_channel: channel_1, message: "[#{link}](/chat/channel/#{channel_1.id}/-?messageId=#{first_message.id})") + end + + it "highglights the correct message" do + visit("/") + chat_page.open_from_header + chat_drawer_page.open_channel(channel_1) + click_link(link) + + expect(page).to have_css(".chat-message-container.highlighted[data-id='#{first_message.id}']") + end + + it "highlights the correct message after using the bottom arrow" do + visit("/") + chat_page.open_from_header + chat_drawer_page.open_channel(channel_1) + click_link(link) + click_link(I18n.t("js.chat.scroll_to_bottom")) + click_link(link) + + expect(page).to have_css(".chat-message-container.highlighted[data-id='#{first_message.id}']") + end + end + end +end diff --git a/plugins/chat/spec/system/navigation_spec.rb b/plugins/chat/spec/system/navigation_spec.rb index 7c86250da1d..e9ee13b565f 100644 --- a/plugins/chat/spec/system/navigation_spec.rb +++ b/plugins/chat/spec/system/navigation_spec.rb @@ -126,33 +126,6 @@ RSpec.describe "Navigation", type: :system, js: true do end end - context "when opening full page with a link containing a message id" do - it "highlights correct message" do - visit("/chat/channel/#{category_channel.id}/#{category_channel.slug}?messageId=#{message.id}") - - expect(page).to have_css( - ".full-page-chat .chat-message-container.highlighted[data-id='#{message.id}']", - ) - end - end - - context "when opening drawer with a link containing a message id" do - it "highlights correct message" do - Fabricate( - :post, - topic: topic, - raw: - "foo", - ) - visit("/t/-/#{topic.id}") - find("a", text: "foo").click - - expect(page).to have_css( - ".chat-drawer.is-expanded .chat-message-container.highlighted[data-id='#{message.id}']", - ) - end - end - context "when sidebar is configured as the navigation menu" do before { SiteSetting.navigation_menu = "sidebar" } diff --git a/plugins/chat/spec/system/page_objects/chat/chat.rb b/plugins/chat/spec/system/page_objects/chat/chat.rb index c9e42661808..a070c700a10 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat.rb @@ -3,6 +3,10 @@ module PageObjects module Pages class Chat < PageObjects::Pages::Base + def prefers_full_page + page.execute_script("window.localStorage.setItem('discourse_chat_preferred_mode', '\"FULL_PAGE_CHAT\"');") + end + def open_from_header find(".chat-header-icon").click end