FIX: improves reliability of last visit line in chat (#30948)

This commit does several changes:

- it moves the ownership of the last message info to the channel instead
of storing it on the message, it avoids the need to iterate over every
messages
- makes an optimistic update of the last read message id
- adds a spec to confirm this behavior

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

@@ -322,7 +322,7 @@ export default class ChatChannel extends Component {
   processMessages(channel, result) {
     const messages = [];
     let foundFirstNew = false;
-    const hasNewest = this.messagesManager.messages.some((m) => m.newest);
+    channel.newestMessage = null;
 
     result?.messages?.forEach((messageData, index) => {
       messageData.firstOfResults = index === 0;
@@ -342,20 +342,23 @@ export default class ChatChannel extends Component {
         messageData.expanded = !(messageData.hidden || messageData.deleted_at);
       }
 
+      const message = ChatMessage.create(channel, messageData);
+      message.manager = channel.messagesManager;
+
       // newest has to be in after fetch callback as we don't want to make it
       // dynamic or it will make the pane jump around, it will disappear on reload
       if (
-        !hasNewest &&
         !foundFirstNew &&
         messageData.id > this.currentUserMembership?.lastReadMessageId
       ) {
         foundFirstNew = true;
-        messageData.newest = true;
+        if (message !== channel.lastMessage) {
+          channel.newestMessage = message;
+        } else {
+          channel.newestMessage = null;
+        }
       }
 
-      const message = ChatMessage.create(channel, messageData);
-      message.manager = channel.messagesManager;
-
       if (message.thread) {
         this.#preloadThreadTrackingState(
           message.thread,
@@ -430,6 +433,10 @@ export default class ChatChannel extends Component {
       return;
     }
 
+    // optimistic update
+    this.args.channel.currentUserMembership.lastReadMessageId = firstMessage.id;
+    this.args.channel.updateLastViewedAt();
+
     return this.chatApi.markChannelAsRead(
       this.args.channel.id,
       firstMessage.id

plugins/chat/assets/javascripts/discourse/components/chat-message-separator.gjs

@@ -62,6 +62,10 @@ export default class ChatMessageSeparator extends Component {
     }
   }
 
+  get isNewestMessage() {
+    return this.args.message.id === this.args.message.channel.newestMessage?.id;
+  }
+
   #areDatesOnSameDay(a, b) {
     return (
       a.getFullYear() === b.getFullYear() &&
@@ -87,17 +91,19 @@ export default class ChatMessageSeparator extends Component {
     {{#if this.formattedFirstMessageDate}}
       <div
         class={{concatClass
+          "chat-message-separator"
           "chat-message-separator-date"
-          (if @message.newest "with-last-visit")
+          (if this.isNewestMessage "with-last-visit")
         }}
         role="button"
         {{on "click" this.onDateClick passive=true}}
+        data-id={{@message.id}}
       >
         <div class="chat-message-separator__text-container" {{this.track}}>
           <span class="chat-message-separator__text">
             {{this.formattedFirstMessageDate}}
 
-            {{#if @message.newest}}
+            {{#if this.isNewestMessage}}
               <span class="chat-message-separator__last-visit">
                 <span
                   class="chat-message-separator__last-visit-separator"
@@ -112,8 +118,11 @@ export default class ChatMessageSeparator extends Component {
       <div class="chat-message-separator__line-container">
         <div class="chat-message-separator__line"></div>
       </div>
-    {{else if @message.newest}}
-      <div class="chat-message-separator-new">
+    {{else if this.isNewestMessage}}
+      <div
+        class="chat-message-separator chat-message-separator-new"
+        data-id={{@message.id}}
+      >
         <div class="chat-message-separator__text-container">
           <span class="chat-message-separator__text">
             {{i18n "chat.last_visit"}}

plugins/chat/assets/javascripts/discourse/components/styleguide/chat-message.hbs

@@ -4,12 +4,6 @@
   </Styleguide::Component>
 
   <Styleguide::Controls>
-    <Styleguide::Controls::Row @name="Last Visit">
-      <DToggleSwitch
-        @state={{this.message.newest}}
-        {{on "click" this.toggleLastVisit}}
-      />
-    </Styleguide::Controls::Row>
     <Styleguide::Controls::Row @name="Deleted">
       <DToggleSwitch
         @state={{not (not this.message.deletedAt)}}

plugins/chat/assets/javascripts/discourse/components/styleguide/chat-message.js

@@ -46,11 +46,6 @@ export default class ChatStyleguideChatMessage extends Component {
     this.message.edited = !this.message.edited;
   }
 
-  @action
-  toggleLastVisit() {
-    this.message.newest = !this.message.newest;
-  }
-
   @action
   toggleThread() {
     if (this.message.thread) {

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

@@ -72,6 +72,7 @@ export default class ChatChannel {
   @tracked tracking;
   @tracked threadingEnabled;
   @tracked draft;
+  @tracked newestMessage;
 
   threadsManager = new ChatThreadsManager(getOwnerWithFallback(this));
   messagesManager = new ChatMessagesManager(getOwnerWithFallback(this));

plugins/chat/assets/javascripts/discourse/models/chat-message.js

@@ -45,7 +45,6 @@ export default class ChatMessage {
   @tracked chatWebhookEvent;
   @tracked mentionWarning;
   @tracked availableFlags;
-  @tracked newest;
   @tracked highlighted;
   @tracked firstOfResults;
   @tracked message;
@@ -62,7 +61,6 @@ export default class ChatMessage {
     this.channel = channel;
     this.streaming = args.streaming;
     this.manager = args.manager;
-    this.newest = args.newest ?? false;
     this.draftSaved = args.draftSaved ?? args.draft_saved ?? false;
     this.firstOfResults = args.firstOfResults ?? args.first_of_results ?? false;
     this.staged = args.staged ?? false;

plugins/chat/spec/system/last_visit_spec.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+RSpec.describe "Last visit", type: :system do
+  fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: false) }
+  fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: false) }
+
+  fab!(:user_1) { Fabricate(:user) }
+  fab!(:current_user) { Fabricate(:user) }
+
+  let(:chat_page) { PageObjects::Pages::Chat.new }
+  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+  let(:sidebar_page) { PageObjects::Pages::Sidebar.new }
+
+  before do
+    chat_system_bootstrap
+    channel_1.add(user_1)
+    channel_1.add(current_user)
+    channel_2.add(current_user)
+    sign_in(current_user)
+  end
+
+  it "correctly updates the last visit line" do
+    # a slightly complicated setup to ensure we test against a non trivial case
+    message_1 = Fabricate(:chat_message, user: user_1, chat_channel: channel_1, use_service: true)
+    Fabricate(:chat_message, user: user_1, chat_channel: channel_1, use_service: true)
+    Fabricate(
+      :chat_message,
+      user: user_1,
+      chat_channel: channel_1,
+      in_reply_to: message_1,
+      use_service: true,
+    )
+    chat_page.visit_channel(channel_1)
+
+    expect(channel_page).to have_last_visit_line_at_id(message_1.id)
+
+    sidebar_page.open_channel(channel_2)
+    sidebar_page.open_channel(channel_1)
+
+    expect(channel_page).to have_no_last_visit_line
+
+    sidebar_page.open_channel(channel_2)
+    message_4 = Fabricate(:chat_message, user: user_1, chat_channel: channel_1, use_service: true)
+    sidebar_page.open_channel(channel_1)
+
+    expect(channel_page).to have_last_visit_line_at_id(message_4.id)
+  end
+end

plugins/chat/spec/system/page_objects/chat/chat_channel.rb

@@ -52,6 +52,16 @@ module PageObjects
         find(message_by_id_selector(id))
       end
 
+      def has_last_visit_line_at_id?(id)
+        find(".chat-message-separator[data-id=\"#{id}\"]").has_content?(
+          I18n.t("js.chat.last_visit"),
+        )
+      end
+
+      def has_no_last_visit_line?
+        has_no_content?(I18n.t("js.chat.last_visit"))
+      end
+
       def has_no_loading_skeleton?
         has_no_css?(".chat-skeleton")
       end