FEATURE: implements last read message for threads (#26702)

This commit will now allow us to track read position in a thread and returns to this position when you open the thread.

Note this commit is also extracting the following components to make it possible:
- `<ChatMessagesScroller />`
- `<ChatMessagesContainer />`

The `UpdateUserThreadLastRead` has been updated to allow this.

Various refactorings have also been done to the code and specs to improve the support of last read.

plugins/chat/app/controllers/chat/api/channels_read_controller.rb

@@ -1,17 +1,15 @@
 # frozen_string_literal: true
 
-class Chat::Api::ReadsController < Chat::ApiController
+class Chat::Api::ChannelsReadController < Chat::ApiController
   def update
-    params.require(%i[channel_id message_id])
+    with_service(Chat::UpdateUserChannelLastRead) do
-
-    with_service(Chat::UpdateUserLastRead) do
       on_success { render(json: success_json) }
       on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:ensure_message_id_recency) do
         raise Discourse::InvalidParameters.new(:message_id)
       end
       on_model_not_found(:message) { raise Discourse::NotFound }
-      on_model_not_found(:active_membership) { raise Discourse::NotFound }
+      on_model_not_found(:membership) { raise Discourse::NotFound }
       on_model_not_found(:channel) { raise Discourse::NotFound }
       on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
       on_failed_contract do |contract|

plugins/chat/app/controllers/chat/api/channels_threads_read_controller.rb

@@ -1,13 +1,12 @@
 # frozen_string_literal: true
 
-class Chat::Api::ThreadReadsController < Chat::ApiController
+class Chat::Api::ChannelsThreadsReadController < Chat::ApiController
   def update
-    params.require(%i[channel_id thread_id])
-
     with_service(Chat::UpdateUserThreadLastRead) do
       on_success { render(json: success_json) }
       on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:thread) { raise Discourse::NotFound }
+      on_model_not_found(:message) { raise Discourse::NotFound }
       on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
       on_failed_contract do |contract|
         render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)

plugins/chat/app/models/chat/thread.rb

@@ -55,12 +55,6 @@ module Chat
       user_chat_thread_memberships.find_by(user: user)
     end
 
-    def mark_read_for_user!(user, last_read_message_id: nil)
-      membership_for(user)&.update!(
-        last_read_message_id: last_read_message_id || self.last_message_id,
-      )
-    end
-
     def replies
       self.chat_messages.where.not(id: self.original_message_id).order("created_at ASC, id ASC")
     end

plugins/chat/app/models/chat/user_chat_thread_membership.rb

@@ -9,6 +9,10 @@ module Chat
     belongs_to :thread, class_name: "Chat::Thread", foreign_key: :thread_id
 
     enum :notification_level, Chat::NotificationLevels.all
+
+    def mark_read!(new_last_read_id = nil)
+      update!(last_read_message_id: new_last_read_id || thread.last_message_id)
+    end
   end
 end
 

plugins/chat/app/services/chat/list_channel_thread_messages.rb

@@ -69,9 +69,9 @@ module Chat
     end
 
     def determine_target_message_id(contract:, membership:, guardian:)
-      if contract.fetch_from_last_read
+      if contract.fetch_from_last_read || !contract.target_message_id
         context.target_message_id = membership&.last_read_message_id
-      else
+      elsif contract.target_message_id
         context.target_message_id = contract.target_message_id
       end
     end

plugins/chat/app/services/chat/update_user_channel_last_read.rb

@@ -4,9 +4,9 @@ module Chat
   # Service responsible for updating the last read message id of a membership.
   #
   # @example
-  #  Chat::UpdateUserLastRead.call(channel_id: 2, message_id: 3, guardian: guardian)
+  #  Chat::UpdateUserChannelLastRead.call(channel_id: 2, message_id: 3, guardian: guardian)
   #
-  class UpdateUserLastRead
+  class UpdateUserChannelLastRead
     include ::Service::Base
 
     # @!method call(channel_id:, message_id:, guardian:)
@@ -17,7 +17,7 @@ module Chat
 
     contract
     model :channel
-    model :active_membership
+    model :membership
     policy :invalid_access
     model :message
     policy :ensure_message_id_recency
@@ -41,31 +41,30 @@ module Chat
       ::Chat::Channel.find_by(id: contract.channel_id)
     end
 
-    def fetch_active_membership(guardian:, channel:)
+    def fetch_membership(guardian:, channel:)
       ::Chat::ChannelMembershipManager.new(channel).find_for_user(guardian.user, following: true)
     end
 
-    def invalid_access(guardian:, active_membership:)
+    def invalid_access(guardian:, membership:)
-      guardian.can_join_chat_channel?(active_membership.chat_channel)
+      guardian.can_join_chat_channel?(membership.chat_channel)
     end
 
     def fetch_message(channel:, contract:)
       ::Chat::Message.with_deleted.find_by(chat_channel_id: channel.id, id: contract.message_id)
     end
 
-    def ensure_message_id_recency(message:, active_membership:)
+    def ensure_message_id_recency(message:, membership:)
-      !active_membership.last_read_message_id ||
+      !membership.last_read_message_id || message.id >= membership.last_read_message_id
-        message.id >= active_membership.last_read_message_id
     end
 
-    def update_membership_state(message:, active_membership:)
+    def update_membership_state(message:, membership:)
-      active_membership.update!(last_read_message_id: message.id, last_viewed_at: Time.zone.now)
+      membership.update!(last_read_message_id: message.id, last_viewed_at: Time.zone.now)
     end
 
-    def mark_associated_mentions_as_read(active_membership:, message:)
+    def mark_associated_mentions_as_read(membership:, message:)
       ::Chat::Action::MarkMentionsRead.call(
-        active_membership.user,
+        membership.user,
-        channel_ids: [active_membership.chat_channel.id],
+        channel_ids: [membership.chat_channel.id],
         message_id: message.id,
       )
     end

plugins/chat/app/services/chat/update_user_thread_last_read.rb

@@ -2,13 +2,10 @@
 
 module Chat
   # Service responsible for marking messages in a thread
-  # as read. For now this just marks any mentions in the thread
+  # as read.
-  # as read and marks the entire thread as read.
-  # As we add finer-grained user tracking state to threads it
-  # will work in a similar way to Chat::UpdateUserLastRead.
   #
   # @example
-  #  Chat::UpdateUserThreadLastRead.call(channel_id: 2, thread_id: 3, guardian: guardian)
+  #  Chat::UpdateUserThreadLastRead.call(channel_id: 2, thread_id: 3, message_id: 4, guardian: guardian)
   #
   class UpdateUserThreadLastRead
     include ::Service::Base
@@ -16,20 +13,25 @@ module Chat
     # @!method call(channel_id:, thread_id:, guardian:)
     #   @param [Integer] channel_id
     #   @param [Integer] thread_id
+    #   @param [Integer] message_id
     #   @param [Guardian] guardian
     #   @return [Service::Base::Context]
 
     contract
     model :thread
     policy :invalid_access
+    model :membership
+    model :message
+    policy :ensure_valid_message
     step :mark_associated_mentions_as_read
     step :mark_thread_read
     step :publish_new_last_read_to_clients
 
     # @!visibility private
     class Contract
-      attribute :thread_id, :integer
       attribute :channel_id, :integer
+      attribute :thread_id, :integer
+      attribute :message_id, :integer
 
       validates :thread_id, :channel_id, presence: true
     end
@@ -40,31 +42,41 @@ module Chat
       ::Chat::Thread.find_by(id: contract.thread_id, channel_id: contract.channel_id)
     end
 
+    def fetch_message(contract:, thread:)
+      ::Chat::Message.with_deleted.find_by(
+        id: contract.message_id || thread.last_message_id,
+        thread_id: contract.thread_id,
+        chat_channel_id: contract.channel_id,
+      )
+    end
+
+    def fetch_membership(guardian:, thread:)
+      thread.membership_for(guardian.user)
+    end
+
     def invalid_access(guardian:, thread:)
       guardian.can_join_chat_channel?(thread.channel)
     end
 
-    # NOTE: In future we will pass in a specific last_read_message_id
+    def ensure_valid_message(message:, membership:)
-    # to the service, so this will need to change because currently it's
+      !membership.last_read_message_id || message.id >= membership.last_read_message_id
-    # just using the thread's last_message_id.
-    def mark_thread_read(thread:, guardian:)
-      thread.mark_read_for_user!(guardian.user)
     end
 
-    def mark_associated_mentions_as_read(thread:, guardian:)
+    def mark_thread_read(membership:, message:)
+      membership.mark_read!(message.id)
+    end
+
+    def mark_associated_mentions_as_read(thread:, guardian:, message:)
       ::Chat::Action::MarkMentionsRead.call(
         guardian.user,
         channel_ids: [thread.channel_id],
         thread_id: thread.id,
+        message_id: message.id,
       )
     end
 
-    def publish_new_last_read_to_clients(guardian:, thread:)
+    def publish_new_last_read_to_clients(guardian:, thread:, message:)
-      ::Chat::Publisher.publish_user_tracking_state!(
+      ::Chat::Publisher.publish_user_tracking_state!(guardian.user, thread.channel, message)
-        guardian.user,
-        thread.channel,
-        thread.last_message,
-      )
     end
   end
 end

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

@@ -1,7 +1,6 @@
 import Component from "@glimmer/component";
 import { cached, tracked } from "@glimmer/tracking";
 import { getOwner } from "@ember/application";
-import { hash } from "@ember/helper";
 import { action } from "@ember/object";
 import didInsert from "@ember/render-modifiers/modifiers/did-insert";
 import didUpdate from "@ember/render-modifiers/modifiers/did-update";
@@ -28,10 +27,7 @@ import {
   READ_INTERVAL_MS,
 } from "discourse/plugins/chat/discourse/lib/chat-constants";
 import ChatMessagesLoader from "discourse/plugins/chat/discourse/lib/chat-messages-loader";
-import {
+import { checkMessageTopVisibility } from "discourse/plugins/chat/discourse/lib/check-message-visibility";
-  checkMessageBottomVisibility,
-  checkMessageTopVisibility,
-} from "discourse/plugins/chat/discourse/lib/check-message-visibility";
 import DatesSeparatorsPositioner from "discourse/plugins/chat/discourse/lib/dates-separators-positioner";
 import { extractCurrentTopicInfo } from "discourse/plugins/chat/discourse/lib/extract-current-topic-info";
 import {
@@ -40,14 +36,14 @@ import {
 } from "discourse/plugins/chat/discourse/lib/scroll-helpers";
 import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
 import { stackingContextFix } from "../lib/chat-ios-hacks";
-import ChatOnResize from "../modifiers/chat/on-resize";
-import ChatScrollableList from "../modifiers/chat/scrollable-list";
 import ChatComposerChannel from "./chat/composer/channel";
 import ChatScrollToBottomArrow from "./chat/scroll-to-bottom-arrow";
 import ChatSelectionManager from "./chat/selection-manager";
 import ChatChannelPreviewCard from "./chat-channel-preview-card";
 import ChatMentionWarnings from "./chat-mention-warnings";
 import Message from "./chat-message";
+import ChatMessagesContainer from "./chat-messages-container";
+import ChatMessagesScroller from "./chat-messages-scroller";
 import ChatNotices from "./chat-notices";
 import ChatSkeleton from "./chat-skeleton";
 import ChatUploadDropZone from "./chat-upload-drop-zone";
@@ -78,7 +74,7 @@ export default class ChatChannel extends Component {
   @tracked uploadDropZone;
   @tracked isScrolling = false;
 
-  scrollable = null;
+  scroller = null;
   _mentionWarningsSeen = {};
   _unreachableGroupMentions = [];
   _overMembersLimitGroupMentions = [];
@@ -101,8 +97,8 @@ export default class ChatChannel extends Component {
   }
 
   @action
-  setScrollable(element) {
+  registerScroller(element) {
-    this.scrollable = element;
+    this.scroller = element;
   }
 
   @action
@@ -117,7 +113,8 @@ export default class ChatChannel extends Component {
   @action
   didResizePane() {
     this.debounceFillPaneAttempt();
-    DatesSeparatorsPositioner.apply(this.scrollable);
+    this.debouncedUpdateLastReadMessage();
+    DatesSeparatorsPositioner.apply(this.scroller);
   }
 
   @action
@@ -180,7 +177,7 @@ export default class ChatChannel extends Component {
       return;
     }
 
-    stackingContextFix(this.scrollable, () => {
+    stackingContextFix(this.scroller, () => {
       this.messagesManager.addMessages([message]);
     });
     this.debouncedUpdateLastReadMessage();
@@ -244,7 +241,7 @@ export default class ChatChannel extends Component {
     }
 
     const targetMessageId = this.messagesManager.messages.lastObject.id;
-    stackingContextFix(this.scrollable, () => {
+    stackingContextFix(this.scroller, () => {
       this.messagesManager.addMessages(messages);
     });
 
@@ -261,14 +258,14 @@ export default class ChatChannel extends Component {
   @action
   async scrollToBottom() {
     this._ignoreNextScroll = true;
-    await scrollListToBottom(this.scrollable);
+    await scrollListToBottom(this.scroller);
     this.debouncedUpdateLastReadMessage();
   }
 
   scrollToMessageId(messageId, options = {}) {
     this._ignoreNextScroll = true;
     const message = this.messagesManager.findMessage(messageId);
-    scrollListToMessage(this.scrollable, message, options);
+    scrollListToMessage(this.scroller, message, options);
   }
 
   debounceFillPaneAttempt() {
@@ -309,12 +306,12 @@ export default class ChatChannel extends Component {
 
     schedule("afterRender", () => {
       const firstMessageId = this.messagesManager.messages.firstObject?.id;
-      const messageContainer = this.scrollable.querySelector(
+      const messageContainer = this.scroller.querySelector(
         `.chat-message-container[data-id="${firstMessageId}"]`
       );
       if (
         messageContainer &&
-        checkMessageTopVisibility(this.scrollable, messageContainer)
+        checkMessageTopVisibility(this.scroller, messageContainer)
       ) {
         this.fetchMoreMessages({ direction: PAST });
       }
@@ -415,41 +412,28 @@ export default class ChatChannel extends Component {
       return;
     }
 
-    schedule("afterRender", () => {
+    if (!this.lastFullyVisibleMessageId) {
-      let lastFullyVisibleMessageNode = null;
+      return;
+    }
 
-      this.scrollable
+    let lastUnreadVisibleMessage = this.messagesManager.findMessage(
-        .querySelectorAll(".chat-message-container")
+      this.lastFullyVisibleMessageId
-        .forEach((item) => {
+    );
-          if (checkMessageBottomVisibility(this.scrollable, item)) {
-            lastFullyVisibleMessageNode = item;
-          }
-        });
 
-      if (!lastFullyVisibleMessageNode) {
+    if (!lastUnreadVisibleMessage) {
-        return;
+      return;
-      }
+    }
 
-      let lastUnreadVisibleMessage = this.messagesManager.findMessage(
+    const lastReadId =
-        lastFullyVisibleMessageNode.dataset.id
+      this.args.channel.currentUserMembership?.lastReadMessageId;
-      );
+    if (lastReadId >= lastUnreadVisibleMessage.id) {
+      return;
+    }
 
-      if (!lastUnreadVisibleMessage) {
+    return this.chatApi.markChannelAsRead(
-        return;
+      this.args.channel.id,
-      }
+      lastUnreadVisibleMessage.id
-
+    );
-      const lastReadId =
-        this.args.channel.currentUserMembership?.lastReadMessageId;
-      // we don't return early if === as we want to ensure different tabs will do the check
-      if (lastReadId > lastUnreadVisibleMessage.id) {
-        return;
-      }
-
-      return this.chatApi.markChannelAsRead(
-        this.args.channel.id,
-        lastUnreadVisibleMessage.id
-      );
-    });
   }
 
   @action
@@ -457,7 +441,7 @@ export default class ChatChannel extends Component {
     if (this.messagesLoader.canLoadMoreFuture) {
       this.fetchMessages();
     } else if (this.messagesManager.messages.length > 0) {
-      this.scrollToBottom(this.scrollable);
+      this.scrollToBottom(this.scroller);
     }
   }
 
@@ -468,7 +452,7 @@ export default class ChatChannel extends Component {
         return;
       }
 
-      DatesSeparatorsPositioner.apply(this.scrollable);
+      DatesSeparatorsPositioner.apply(this.scroller);
 
       this.needsArrow =
         (this.messagesLoader.fetchedOnce &&
@@ -476,6 +460,7 @@ export default class ChatChannel extends Component {
         (state.distanceToBottom.pixels > 250 && !state.atBottom);
       this.isScrolling = true;
       this.debouncedUpdateLastReadMessage();
+      this.lastFullyVisibleMessageId = state.lastVisibleId;
 
       if (
         state.atTop ||
@@ -499,6 +484,7 @@ export default class ChatChannel extends Component {
       (state.distanceToBottom.pixels > 250 && !state.atBottom);
     this.isScrolling = false;
     this.atBottom = state.atBottom;
+    this.lastFullyVisibleMessageId = state.lastVisibleId;
 
     if (state.atBottom) {
       this.fetchMoreMessages({ direction: FUTURE });
@@ -537,7 +523,7 @@ export default class ChatChannel extends Component {
     this.resetComposerMessage();
 
     try {
-      stackingContextFix(this.scrollable, async () => {
+      stackingContextFix(this.scroller, async () => {
         await this.chatApi.editMessage(this.args.channel.id, message.id, data);
       });
     } catch (e) {
@@ -553,7 +539,7 @@ export default class ChatChannel extends Component {
 
     resetIdle();
 
-    stackingContextFix(this.scrollable, async () => {
+    stackingContextFix(this.scroller, async () => {
       await this.args.channel.stageMessage(message);
     });
 
@@ -712,19 +698,12 @@ export default class ChatChannel extends Component {
       <ChatNotices @channel={{@channel}} />
       <ChatMentionWarnings />
 
-      <div
+      <ChatMessagesScroller
-        class="chat-messages-scroll chat-messages-container popper-viewport"
+        @onRegisterScroller={{this.registerScroller}}
-        {{didInsert this.setScrollable}}
+        @onScroll={{this.onScroll}}
-        {{ChatScrollableList
+        @onScrollEnd={{this.onScrollEnd}}
-          (hash
-            onScroll=this.onScroll onScrollEnd=this.onScrollEnd reverse=true
-          )
-        }}
       >
-        <div
+        <ChatMessagesContainer @didResizePane={{this.didResizePane}}>
-          class="chat-messages-container"
-          {{ChatOnResize this.didResizePane (hash delay=100 immediate=true)}}
-        >
           {{#each this.messagesManager.messages key="id" as |message|}}
             <Message
               @message={{message}}
@@ -738,7 +717,7 @@ export default class ChatChannel extends Component {
               <ChatSkeleton />
             {{/unless}}
           {{/each}}
-        </div>
+        </ChatMessagesContainer>
 
         {{! at bottom even if shown at top due to column-reverse  }}
         {{#if this.messagesLoader.loadedPast}}
@@ -746,7 +725,7 @@ export default class ChatChannel extends Component {
             {{i18n "chat.all_loaded"}}
           </div>
         {{/if}}
-      </div>
+      </ChatMessagesScroller>
 
       <ChatScrollToBottomArrow
         @onScrollToBottom={{this.scrollToLatestMessage}}
@@ -769,7 +748,6 @@ export default class ChatChannel extends Component {
             @channel={{@channel}}
             @uploadDropZone={{this.uploadDropZone}}
             @onSendMessage={{this.onSendMessage}}
-            @scrollable={{this.scrollable}}
           />
         {{/if}}
       {{/if}}

plugins/chat/assets/javascripts/discourse/components/chat-messages-container.gjs

@@ -0,0 +1,13 @@
+import { hash } from "@ember/helper";
+import ChatOnResize from "../modifiers/chat/on-resize";
+
+const ChatMessagesContainer = <template>
+  <div
+    class="chat-messages-container"
+    {{ChatOnResize @didResizePane (hash delay=100 immediate=true)}}
+  >
+    {{yield}}
+  </div>
+</template>;
+
+export default ChatMessagesContainer;

plugins/chat/assets/javascripts/discourse/components/chat-messages-scroller.gjs

@@ -0,0 +1,17 @@
+import { hash } from "@ember/helper";
+import didInsert from "@ember/render-modifiers/modifiers/did-insert";
+import ChatScrollableList from "../modifiers/chat/scrollable-list";
+
+const ChatMessagesScroller = <template>
+  <div
+    class="chat-messages-scroller popper-viewport"
+    {{didInsert @onRegisterScroller}}
+    {{ChatScrollableList
+      (hash onScroll=@onScroll onScrollEnd=@onScrollEnd reverse=true)
+    }}
+  >
+    {{yield}}
+  </div>
+</template>;
+
+export default ChatMessagesScroller;

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

@@ -1,7 +1,6 @@
 import Component from "@glimmer/component";
 import { cached, tracked } from "@glimmer/tracking";
 import { getOwner } from "@ember/application";
-import { hash } from "@ember/helper";
 import { action } from "@ember/object";
 import didInsert from "@ember/render-modifiers/modifiers/did-insert";
 import willDestroy from "@ember/render-modifiers/modifiers/will-destroy";
@@ -30,12 +29,12 @@ import {
 } from "discourse/plugins/chat/discourse/lib/scroll-helpers";
 import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
 import UserChatThreadMembership from "discourse/plugins/chat/discourse/models/user-chat-thread-membership";
-import ChatOnResize from "../modifiers/chat/on-resize";
-import ChatScrollableList from "../modifiers/chat/scrollable-list";
 import ChatComposerThread from "./chat/composer/thread";
 import ChatScrollToBottomArrow from "./chat/scroll-to-bottom-arrow";
 import ChatSelectionManager from "./chat/selection-manager";
 import Message from "./chat-message";
+import ChatMessagesContainer from "./chat-messages-container";
+import ChatMessagesScroller from "./chat-messages-scroller";
 import ChatSkeleton from "./chat-skeleton";
 import ChatUploadDropZone from "./chat-upload-drop-zone";
 
@@ -58,7 +57,7 @@ export default class ChatThread extends Component {
   @tracked needsArrow = false;
   @tracked uploadDropZone;
 
-  scrollable = null;
+  scroller = null;
 
   @action
   resetIdle() {
@@ -116,7 +115,7 @@ export default class ChatThread extends Component {
         return;
       }
 
-      DatesSeparatorsPositioner.apply(this.scrollable);
+      DatesSeparatorsPositioner.apply(this.scroller);
 
       this.needsArrow =
         (this.messagesLoader.fetchedOnce &&
@@ -124,6 +123,7 @@ export default class ChatThread extends Component {
         (state.distanceToBottom.pixels > 250 && !state.atBottom);
       this.isScrolling = true;
       this.debounceUpdateLastReadMessage();
+      this.lastFullyVisibleMessageId = state.lastVisibleId;
 
       if (
         state.atTop ||
@@ -148,6 +148,7 @@ export default class ChatThread extends Component {
     this.resetIdle();
     this.atBottom = state.atBottom;
     this.args.setFullTitle?.(state.atTop);
+    this.lastFullyVisibleMessageId = state.lastVisibleId;
 
     if (state.atBottom) {
       this.fetchMoreMessages({ direction: FUTURE });
@@ -164,16 +165,37 @@ export default class ChatThread extends Component {
 
   @bind
   updateLastReadMessage() {
-    // HACK: We don't have proper scroll visibility over
+    if (!this.args.thread?.currentUserMembership) {
-    // what message we are looking at, don't have the lastReadMessageId
+      return;
-    // for the thread, and this updateLastReadMessage function is only
+    }
-    // called when scrolling all the way to the bottom.
+
-    this.markThreadAsRead();
+    if (!this.lastFullyVisibleMessageId) {
+      return;
+    }
+
+    const lastUnreadVisibleMessage = this.messagesManager.findMessage(
+      this.lastFullyVisibleMessageId
+    );
+
+    if (!lastUnreadVisibleMessage) {
+      return;
+    }
+
+    const lastReadId = this.args.thread.currentUserMembership.lastReadMessageId;
+    if (lastReadId >= lastUnreadVisibleMessage.id) {
+      return;
+    }
+
+    return this.chatApi.markThreadAsRead(
+      this.args.thread.channel.id,
+      this.args.thread.id,
+      lastUnreadVisibleMessage.id
+    );
   }
 
   @action
-  setScrollable(element) {
+  registerScroller(element) {
-    this.scrollable = element;
+    this.scroller = element;
   }
 
   @action
@@ -191,7 +213,7 @@ export default class ChatThread extends Component {
     this._ignoreNextScroll = true;
     this.debounceFillPaneAttempt();
     this.debounceUpdateLastReadMessage();
-    DatesSeparatorsPositioner.apply(this.scrollable);
+    DatesSeparatorsPositioner.apply(this.scroller);
   }
 
   async fetchMessages(findArgs = {}) {
@@ -215,17 +237,13 @@ export default class ChatThread extends Component {
     }
 
     const [messages, meta] = this.processMessages(this.args.thread, result);
-    stackingContextFix(this.scrollable, () => {
+    stackingContextFix(this.scroller, () => {
       this.messagesManager.addMessages(messages);
     });
     this.args.thread.details = meta;
 
-    if (this.args.targetMessageId) {
+    if (meta.target_message_id) {
-      this.scrollToMessageId(this.args.targetMessageId, { highlight: true });
+      this.scrollToMessageId(meta.target_message_id, { highlight: true });
-    } else if (this.args.thread.currentUserMembership?.lastReadMessageId) {
-      this.scrollToMessageId(
-        this.args.thread.currentUserMembership?.lastReadMessageId
-      );
     } else {
       this.scrollToTop();
     }
@@ -249,7 +267,7 @@ export default class ChatThread extends Component {
       return;
     }
 
-    stackingContextFix(this.scrollable, () => {
+    stackingContextFix(this.scroller, () => {
       this.messagesManager.addMessages(messages);
     });
     this.args.thread.details = meta;
@@ -311,7 +329,7 @@ export default class ChatThread extends Component {
   ) {
     this._ignoreNextScroll = true;
     const message = this.messagesManager.findMessage(messageId);
-    scrollListToMessage(this.scrollable, message, opts);
+    scrollListToMessage(this.scroller, message, opts);
   }
 
   @bind
@@ -322,7 +340,7 @@ export default class ChatThread extends Component {
       return;
     }
 
-    stackingContextFix(this.scrollable, () => {
+    stackingContextFix(this.scroller, () => {
       this.messagesManager.addMessages([message]);
     });
   }
@@ -345,20 +363,6 @@ export default class ChatThread extends Component {
     return [messages, result.meta];
   }
 
-  // NOTE: At some point we want to do this based on visible messages
-  // and scrolling; for now it's enough to do it when the thread panel
-  // opens/messages are loaded since we have no pagination for threads.
-  markThreadAsRead() {
-    if (!this.args.thread) {
-      return;
-    }
-
-    return this.chatApi.markThreadAsRead(
-      this.args.thread.channel.id,
-      this.args.thread.id
-    );
-  }
-
   @action
   async onSendMessage(message) {
     resetIdle();
@@ -424,7 +428,7 @@ export default class ChatThread extends Component {
 
     this.chatThreadPane.sending = true;
     this._ignoreNextScroll = true;
-    stackingContextFix(this.scrollable, async () => {
+    stackingContextFix(this.scroller, async () => {
       await this.args.thread.stageMessage(message);
     });
     this.resetComposerMessage();
@@ -495,13 +499,13 @@ export default class ChatThread extends Component {
   @action
   async scrollToBottom() {
     this._ignoreNextScroll = true;
-    await scrollListToBottom(this.scrollable);
+    await scrollListToBottom(this.scroller);
   }
 
   @action
   async scrollToTop() {
     this._ignoreNextScroll = true;
-    await scrollListToTop(this.scrollable);
+    await scrollListToTop(this.scroller);
   }
 
   @action
@@ -538,19 +542,12 @@ export default class ChatThread extends Component {
       {{didInsert this.setup}}
       {{willDestroy this.teardown}}
     >
-      <div
+      <ChatMessagesScroller
-        class="chat-thread__body popper-viewport chat-messages-scroll"
+        @onRegisterScroller={{this.registerScroller}}
-        {{didInsert this.setScrollable}}
+        @onScroll={{this.onScroll}}
-        {{ChatScrollableList
+        @onScrollEnd={{this.onScrollEnd}}
-          (hash
-            onScroll=this.onScroll onScrollEnd=this.onScrollEnd reverse=true
-          )
-        }}
       >
-        <div
+        <ChatMessagesContainer @didResizePane={{this.didResizePane}}>
-          class="chat-messages-container"
-          {{ChatOnResize this.didResizePane (hash delay=100 immediate=true)}}
-        >
           {{#each this.messagesManager.messages key="id" as |message|}}
             <Message
               @message={{message}}
@@ -566,8 +563,8 @@ export default class ChatThread extends Component {
               <ChatSkeleton />
             {{/if}}
           {{/unless}}
-        </div>
+        </ChatMessagesContainer>
-      </div>
+      </ChatMessagesScroller>
 
       <ChatScrollToBottomArrow
         @onScrollToBottom={{this.scrollToLatestMessage}}
@@ -582,7 +579,6 @@ export default class ChatThread extends Component {
           @thread={{@thread}}
           @onSendMessage={{this.onSendMessage}}
           @uploadDropZone={{this.uploadDropZone}}
-          @scrollable={{this.scrollable}}
         />
       {{/if}}
 

plugins/chat/assets/javascripts/discourse/lib/chat-channel-subscription-manager.js

@@ -36,7 +36,6 @@ export default class ChatChannelSubscriptionManager {
 
   teardown() {
     this.messageBus.unsubscribe(this.messageBusChannel, this.onMessage);
-    this.modelId = null;
   }
 
   @bind
@@ -194,7 +193,7 @@ export default class ChatChannelSubscriptionManager {
     if (message) {
       message.deletedAt = null;
     } else {
-      const newMessage = ChatMessage.create(this.model, data.chat_message);
+      const newMessage = ChatMessage.create(this.channel, data.chat_message);
       newMessage.manager = this.messagesManager;
       this.messagesManager.addMessages([newMessage]);
     }

plugins/chat/assets/javascripts/discourse/modifiers/chat/scrollable-list.js

@@ -27,6 +27,8 @@ export default class ChatScrollableList extends Modifier {
     this.element.addEventListener("wheel", this.handleWheel, {
       passive: true,
     });
+
+    this.throttleComputeScroll();
   }
 
   @bind

plugins/chat/assets/javascripts/discourse/services/chat-api.js

@@ -493,21 +493,25 @@ export default class ChatApi extends Service {
    * @returns {Promise}
    */
   markChannelAsRead(channelId, messageId = null) {
-    return this.#putRequest(`/channels/${channelId}/read/${messageId}`);
+    return this.#putRequest(
+      `/channels/${channelId}/read?message_id=${messageId}`
+    );
   }
 
   /**
-   * Marks all messages and mentions in a thread as read. This is quite
+   * Marks messages for a single user chat thread membership as read. If no
-   * far-reaching for now, and is not granular since there is no membership/
+   * message ID is provided, then the latest message for the channel is fetched
-   * read state per-user for threads. In future this will be expanded to
+   * on the server and used for the last read message.
-   * also pass message ID in the same way as markChannelAsRead
    *
    * @param {number} channelId - The ID of the channel for the thread being marked as read.
    * @param {number} threadId - The ID of the thread being marked as read.
+   * @param {number} messageId - The ID of the message being marked as read.
    * @returns {Promise}
    */
-  markThreadAsRead(channelId, threadId) {
+  markThreadAsRead(channelId, threadId, messageId) {
-    return this.#putRequest(`/channels/${channelId}/threads/${threadId}/read`);
+    return this.#putRequest(
+      `/channels/${channelId}/threads/${threadId}/read?message_id=${messageId}`
+    );
   }
 
   /**

plugins/chat/assets/stylesheets/common/base-common.scss

@@ -195,7 +195,7 @@ body.has-full-page-chat {
     top: var(--header-offset);
   }
 
-  .chat-messages-scroll {
+  .chat-messages-scroller {
     box-sizing: border-box;
     height: 100%;
   }

plugins/chat/assets/stylesheets/common/chat-channel.scss

@@ -9,31 +9,18 @@
   min-width: 250px;
   @include chat-height(var(--chat-header-offset, 0px));
 
-  .chat-messages-scroll {
+  .join-channel-btn.in-float {
-    flex-grow: 1;
+    position: absolute;
-    overflow-y: scroll;
+    transform: translateX(-50%);
-    overscroll-behavior: contain;
+    left: 50%;
-    display: flex;
+    top: 10px;
-    flex-direction: column-reverse;
+    z-index: 10;
-    z-index: 1;
+  }
-    margin: 0 1px 0 0;
-    will-change: transform;
-    @include chat-scrollbar();
-    min-height: 1px;
 
-    .join-channel-btn.in-float {
+  .all-loaded-message {
-      position: absolute;
+    text-align: center;
-      transform: translateX(-50%);
+    color: var(--primary-medium);
-      left: 50%;
+    font-size: var(--font-down-1);
-      top: 10px;
+    padding: 0.5em 0.25em 0.25em;
-      z-index: 10;
-    }
-
-    .all-loaded-message {
-      text-align: center;
-      color: var(--primary-medium);
-      font-size: var(--font-down-1);
-      padding: 0.5em 0.25em 0.25em;
-    }
   }
 }

plugins/chat/assets/stylesheets/common/chat-messages-scroller.scss

@@ -0,0 +1,14 @@
+.chat-messages-scroller {
+  flex-grow: 1;
+  overflow-y: scroll;
+  overscroll-behavior: contain;
+  display: flex;
+  flex-direction: column-reverse;
+  z-index: 1;
+  margin: 0 1px 0 0;
+  will-change: transform;
+  @include chat-scrollbar();
+  min-height: 1px;
+  box-sizing: border-box;
+  transition: padding-top 0.2s ease-in-out;
+}

plugins/chat/assets/stylesheets/common/chat-thread.scss

@@ -3,15 +3,4 @@
   flex-direction: column;
   position: relative;
   @include chat-height(var(--chat-header-expanded-offset, 0px));
-
-  &__body {
-    overflow-y: scroll;
-    @include chat-scrollbar();
-    box-sizing: border-box;
-    flex-grow: 1;
-    overscroll-behavior: contain;
-    display: flex;
-    flex-direction: column-reverse;
-    transition: padding-top 0.2s ease-in-out;
-  }
 }

plugins/chat/assets/stylesheets/common/index.scss

@@ -69,3 +69,4 @@
 @import "chat-thread-title";
 @import "chat-audio-upload";
 @import "chat-message-text";
+@import "chat-messages-scroller";

plugins/chat/assets/stylesheets/mobile/chat-channel.scss

@@ -1,5 +1,5 @@
 .chat-channel {
-  .chat-messages-scroll {
+  .chat-messages-scroller {
     padding-bottom: 5px;
   }
 }

plugins/chat/assets/stylesheets/mobile/chat-thread.scss

@@ -3,7 +3,7 @@
     margin: 0 1px 0 0;
   }
 
-  .chat-messages-scroll {
+  .chat-messages-scroller {
     padding: 10px 10px 0 10px;
   }
 }

plugins/chat/config/routes.rb

@@ -7,8 +7,8 @@ Chat::Engine.routes.draw do
     get "/me/channels" => "current_user_channels#index"
     get "/me/threads" => "current_user_threads#index"
     post "/channels" => "channels#create"
-    put "/channels/read/" => "reads#update_all"
+    put "/channels/read" => "channels_read#update_all"
-    put "/channels/:channel_id/read/:message_id" => "reads#update"
+    put "/channels/:channel_id/read" => "channels_read#update"
     post "/channels/:channel_id/messages/:message_id/flags" => "channels_messages_flags#create"
     post "/channels/:channel_id/drafts" => "channels_drafts#create"
     delete "/channels/:channel_id" => "channels#destroy"
@@ -45,7 +45,7 @@ Chat::Engine.routes.draw do
     put "/channels/:channel_id/threads/:thread_id" => "channel_threads#update"
     get "/channels/:channel_id/threads/:thread_id" => "channel_threads#show"
     get "/channels/:channel_id/threads/:thread_id/messages" => "channel_thread_messages#index"
-    put "/channels/:channel_id/threads/:thread_id/read" => "thread_reads#update"
+    put "/channels/:channel_id/threads/:thread_id/read" => "channels_threads_read#update"
     post "/channels/:channel_id/threads/:thread_id/drafts" => "channels_threads_drafts#create"
     put "/channels/:channel_id/threads/:thread_id/notifications-settings/me" =>
           "channel_threads_current_user_notifications_settings#update"

plugins/chat/spec/requests/chat/api/channels_read_controller_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe Chat::Api::ReadsController do
+RSpec.describe Chat::Api::ChannelsReadController do
   fab!(:current_user) { Fabricate(:user) }
 
   before do
@@ -17,7 +17,7 @@ RSpec.describe Chat::Api::ReadsController do
       fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) }
 
       it "returns a 404 when the user is not a channel member" do
-        put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
+        put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json"
         expect(response.status).to eq(404)
       end
 
@@ -29,7 +29,7 @@ RSpec.describe Chat::Api::ReadsController do
           following: false,
         )
 
-        put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
+        put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json"
         expect(response.status).to eq(404)
       end
 
@@ -49,7 +49,7 @@ RSpec.describe Chat::Api::ReadsController do
           before { membership.update!(last_read_message_id: message_2.id) }
 
           it "raises an invalid request" do
-            put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
+            put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json"
             expect(response.status).to eq(400)
             expect(response.parsed_body["errors"][0]).to match(/message_id/)
           end
@@ -59,14 +59,14 @@ RSpec.describe Chat::Api::ReadsController do
           before { message_1.trash!(Discourse.system_user) }
 
           it "works" do
-            put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}"
+            put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}"
             expect(response.status).to eq(200)
           end
         end
 
         it "updates timing records" do
           expect {
-            put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json"
+            put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_1.id}.json"
           }.not_to change { Chat::UserChatChannelMembership.count }
 
           membership.reload
@@ -78,7 +78,7 @@ RSpec.describe Chat::Api::ReadsController do
         it "marks all mention notifications as read for the channel" do
           notification = create_notification_and_mention_for(current_user, other_user, message_1)
 
-          put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json"
+          put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_2.id}.json"
           expect(response.status).to eq(200)
           expect(notification.reload.read).to eq(true)
         end
@@ -87,7 +87,7 @@ RSpec.describe Chat::Api::ReadsController do
           message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user)
           notification = create_notification_and_mention_for(current_user, other_user, message_3)
 
-          put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json"
+          put "/chat/api/channels/#{chat_channel.id}/read?message_id=#{message_2.id}.json"
           expect(response.status).to eq(200)
           expect(notification.reload.read).to eq(false)
         end

plugins/chat/spec/requests/chat/api/channels_threads_read_controller_spec.rb

@@ -0,0 +1,64 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::Api::ChannelsThreadsReadController do
+  fab!(:current_user) { Fabricate(:user) }
+
+  before do
+    SiteSetting.chat_enabled = true
+    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
+    sign_in(current_user)
+  end
+
+  describe "#update" do
+    context "with valid params" do
+      fab!(:thread_1) { Fabricate(:chat_thread) }
+
+      before { thread_1.add(current_user) }
+
+      it "is a success" do
+        put "/chat/api/channels/#{thread_1.channel.id}/threads/#{thread_1.id}/read.json"
+
+        expect(response.status).to eq(200)
+      end
+
+      context "when a message_id is provided" do
+        fab!(:message_1) do
+          Fabricate(:chat_message, thread: thread_1, chat_channel: thread_1.channel)
+        end
+
+        it "updates the last read" do
+          expect {
+            put "/chat/api/channels/#{thread_1.channel.id}/threads/#{thread_1.id}/read?message_id=#{message_1.id}.json"
+          }.to change { thread_1.membership_for(current_user).last_read_message_id }.from(nil).to(
+            message_1.id,
+          )
+
+          expect(response.status).to eq(200)
+        end
+      end
+    end
+
+    context "when the thread doesn't exist" do
+      fab!(:channel_1) { Fabricate(:chat_channel) }
+
+      it "raises a not found" do
+        put "/chat/api/channels/#{channel_1.id}/threads/-999/read.json"
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context "when the user can't join associated channel" do
+      fab!(:channel_1) { Fabricate(:private_category_channel) }
+      fab!(:thread_1) { Fabricate(:chat_thread, channel: channel_1) }
+
+      before { thread_1.add(current_user) }
+
+      it "raises a not found" do
+        put "/chat/api/channels/#{thread_1.channel.id}/threads/#{thread_1.id}/read.json"
+
+        expect(response.status).to eq(403)
+      end
+    end
+  end
+end

plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb

@@ -1,72 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe Chat::Api::ThreadReadsController do
-  fab!(:current_user) { Fabricate(:user) }
-
-  before do
-    SiteSetting.chat_enabled = true
-    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
-    sign_in(current_user)
-  end
-
-  describe "#update" do
-    describe "marking the thread messages as read" do
-      fab!(:channel) { Fabricate(:chat_channel) }
-      fab!(:other_user) { Fabricate(:user) }
-      fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
-      fab!(:message_1) do
-        Fabricate(:chat_message, chat_channel: channel, user: other_user, thread: thread)
-      end
-      fab!(:message_2) do
-        Fabricate(:chat_message, chat_channel: channel, user: other_user, thread: thread)
-      end
-
-      context "when the user cannot access the channel" do
-        fab!(:channel) { Fabricate(:private_category_channel) }
-        fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
-        it "raises invalid access" do
-          put "/chat/api/channels/#{channel.id}/threads/#{thread.id}/read.json"
-          expect(response.status).to eq(403)
-        end
-      end
-
-      context "when the channel_id and thread_id params do not match" do
-        it "raises a not found" do
-          put "/chat/api/channels/#{Fabricate(:chat_channel).id}/threads/#{thread.id}/read.json"
-          expect(response.status).to eq(404)
-        end
-      end
-
-      it "marks all mention notifications as read for the thread" do
-        notification_1 = create_notification_and_mention_for(current_user, other_user, message_1)
-        notification_2 = create_notification_and_mention_for(current_user, other_user, message_2)
-
-        put "/chat/api/channels/#{channel.id}/threads/#{thread.id}/read.json"
-        expect(response.status).to eq(200)
-        expect(notification_1.reload.read).to eq(true)
-        expect(notification_2.reload.read).to eq(true)
-      end
-    end
-  end
-
-  def create_notification_and_mention_for(user, sender, msg)
-    Notification
-      .create!(
-        notification_type: Notification.types[:chat_mention],
-        user: user,
-        high_priority: true,
-        read: false,
-        data: {
-          message: "chat.mention_notification",
-          chat_message_id: msg.id,
-          chat_channel_id: msg.chat_channel_id,
-          chat_channel_title: msg.chat_channel.title(user),
-          chat_channel_slug: msg.chat_channel.slug,
-          mentioned_by_username: sender.username,
-        }.to_json,
-      )
-      .tap do |notification|
-        Chat::UserMention.create!(user: user, chat_message: msg, notifications: [notification])
-      end
-  end
-end

plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb

@@ -170,9 +170,9 @@ RSpec.describe ::Chat::LookupChannelThreads do
 
           [thread_4, thread_5, thread_6, thread_7].each do |t|
             t.add(current_user)
-            t.mark_read_for_user!(current_user)
+            t.membership_for(current_user).mark_read!
           end
-          [thread_1, thread_2, thread_3].each { |t| t.mark_read_for_user!(current_user) }
+          [thread_1, thread_2, thread_3].each { |t| t.membership_for(current_user).mark_read! }
 
           # The old unread messages.
           Fabricate(:chat_message, chat_channel: channel_1, thread: thread_7).update!(

plugins/chat/spec/services/chat/update_user_channel_last_read_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
-RSpec.describe Chat::UpdateUserLastRead do
+RSpec.describe Chat::UpdateUserChannelLastRead do
-  describe Chat::UpdateUserLastRead::Contract, type: :model do
+  describe Chat::UpdateUserChannelLastRead::Contract, type: :model do
     it { is_expected.to validate_presence_of :channel_id }
     it { is_expected.to validate_presence_of :message_id }
   end
@@ -32,7 +32,7 @@ RSpec.describe Chat::UpdateUserLastRead do
       context "when user has no membership" do
         before { membership.destroy! }
 
-        it { is_expected.to fail_to_find_a_model(:active_membership) }
+        it { is_expected.to fail_to_find_a_model(:membership) }
       end
 
       context "when user can’t access the channel" do

plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb

@@ -11,15 +11,23 @@ RSpec.describe Chat::UpdateUserThreadLastRead do
 
     fab!(:chatters) { Fabricate(:group) }
     fab!(:current_user) { Fabricate(:user, group_ids: [chatters.id]) }
-    fab!(:channel) { Fabricate(:chat_channel) }
+    fab!(:thread) { Fabricate(:chat_thread, old_om: true) }
-    fab!(:thread) { Fabricate(:chat_thread, channel: channel, old_om: true) }
+    fab!(:reply_1) { Fabricate(:chat_message, thread: thread, chat_channel_id: thread.channel.id) }
-    fab!(:thread_reply_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
-    fab!(:thread_reply_2) { Fabricate(:chat_message, chat_channel: channel, thread: thread) }
 
     let(:guardian) { Guardian.new(current_user) }
-    let(:params) { { guardian: guardian, channel_id: channel.id, thread_id: thread.id } }
+    let(:params) do
+      {
+        message_id: reply_1.id,
+        guardian: guardian,
+        channel_id: thread.channel.id,
+        thread_id: thread.id,
+      }
+    end
 
-    before { SiteSetting.chat_allowed_groups = [chatters] }
+    before do
+      thread.add(current_user)
+      SiteSetting.chat_allowed_groups = [chatters]
+    end
 
     context "when params are not valid" do
       before { params.delete(:thread_id) }
@@ -27,81 +35,108 @@ RSpec.describe Chat::UpdateUserThreadLastRead do
       it { is_expected.to fail_a_contract }
     end
 
+    context "when thread cannot be found" do
+      before { params[:channel_id] = Fabricate(:chat_channel).id }
+
+      it { is_expected.to fail_to_find_a_model(:thread) }
+    end
+
+    context "when user has no membership" do
+      before { thread.remove(current_user) }
+
+      it { is_expected.to fail_to_find_a_model(:membership) }
+    end
+
+    context "when user can’t access the channel" do
+      fab!(:channel) { Fabricate(:private_category_channel) }
+      fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+
+      it { is_expected.to fail_a_policy(:invalid_access) }
+    end
+
     context "when params are valid" do
-      context "when user can’t access the channel" do
+      it "sets the service result as successful" do
-        fab!(:channel) { Fabricate(:private_category_channel) }
+        expect(result).to be_a_success
-        fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
-
-        it { is_expected.to fail_a_policy(:invalid_access) }
       end
 
-      context "when thread cannot be found" do
+      it "publishes new last read to clients" do
-        before { params[:channel_id] = Fabricate(:chat_channel).id }
+        messages = MessageBus.track_publish { result }
-
+        expect(messages.map(&:channel)).to include("/chat/user-tracking-state/#{current_user.id}")
-        it { is_expected.to fail_to_find_a_model(:thread) }
       end
 
-      context "when everything is fine" do
+      context "when the user is a member of the thread" do
-        fab!(:notification_1) do
+        fab!(:membership) do
-          Fabricate(
+          Fabricate(:user_chat_thread_membership, user: current_user, thread: thread)
-            :notification,
-            notification_type: Notification.types[:chat_mention],
-            user: current_user,
-          )
         end
-        fab!(:notification_2) do
+
-          Fabricate(
+        it "updates the last_read_message_id of the thread" do
-            :notification,
+          expect { result }.to change { membership.reload.last_read_message_id }.from(nil).to(
-            notification_type: Notification.types[:chat_mention],
+            reply_1.id,
-            user: current_user,
           )
         end
 
-        let(:messages) { MessageBus.track_publish { result } }
+        context "when the provided last read id is before the existing one" do
+          fab!(:reply_2) { Fabricate(:chat_message, thread: thread) }
 
-        before do
+          before { thread.membership_for(current_user).update!(last_read_message_id: reply_2.id) }
-          Jobs.run_immediately!
+
-          Chat::UserMention.create!(
+          it { is_expected.to fail_a_policy(:ensure_valid_message) }
-            notifications: [notification_1],
-            user: current_user,
-            chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread),
-          )
-          Chat::UserMention.create!(
-            notifications: [notification_2],
-            user: current_user,
-            chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread),
-          )
         end
 
-        it "sets the service result as successful" do
+        context "when the message doesn’t exist" do
-          expect(result).to be_a_success
+          it "fails" do
-        end
+            params[:message_id] = 999
-
+            is_expected.to fail_to_find_a_model(:message)
-        it "marks existing notifications related to all messages in the thread as read" do
-          expect { result }.to change {
-            Notification.where(
-              notification_type: Notification.types[:chat_mention],
-              user: current_user,
-              read: false,
-            ).count
-          }.by(-2)
-        end
-
-        it "publishes new last read to clients" do
-          expect(messages.map(&:channel)).to include("/chat/user-tracking-state/#{current_user.id}")
-        end
-
-        context "when the user is a member of the thread" do
-          fab!(:membership) do
-            Fabricate(:user_chat_thread_membership, user: current_user, thread: thread)
-          end
-
-          it "updates the last_read_message_id of the thread" do
-            result
-            expect(membership.reload.last_read_message_id).to eq(thread.reload.last_message.id)
           end
         end
       end
     end
+
+    context "when unread messages have associated notifications" do
+      before_all do
+        Jobs.run_immediately!
+        thread.channel.add(current_user)
+      end
+
+      fab!(:reply_2) do
+        Fabricate(
+          :chat_message,
+          thread: thread,
+          message: "hi @#{current_user.username}",
+          use_service: true,
+        )
+      end
+
+      fab!(:reply_3) do
+        Fabricate(
+          :chat_message,
+          thread: thread,
+          message: "hi @#{current_user.username}",
+          use_service: true,
+        )
+      end
+
+      it "marks notifications as read" do
+        params[:message_id] = reply_2.id
+
+        expect { described_class.call(params) }.to change {
+          ::Notification
+            .where(notification_type: Notification.types[:chat_mention])
+            .where(user: current_user)
+            .where(read: false)
+            .count
+        }.by(-1)
+
+        params[:message_id] = reply_3.id
+
+        expect { described_class.call(params) }.to change {
+          ::Notification
+            .where(notification_type: Notification.types[:chat_mention])
+            .where(user: current_user)
+            .where(read: false)
+            .count
+        }.by(-1)
+      end
+    end
   end
 end

plugins/chat/spec/system/chat_channel_spec.rb

@@ -146,7 +146,7 @@ RSpec.describe "Chat channel", type: :system do
   end
 
   context "when returning to a channel where last read is not last message" do
-    it "jumps to the bottom of the channel" do
+    it "scrolls to the correct last read message" do
       channel_1.membership_for(current_user).update!(last_read_message: message_1)
       messages = Fabricate.times(50, :chat_message, chat_channel: channel_1)
       chat_page.visit_channel(channel_1)
@@ -348,7 +348,7 @@ RSpec.describe "Chat channel", type: :system do
         ".chat-message-actions-container[data-id='#{last_message["data-id"]}']",
       )
 
-      find(".chat-messages-scroll").scroll_to(0, -1000)
+      find(".chat-messages-scroller").scroll_to(0, -1000)
 
       expect(page).to have_no_css(
         ".chat-message-actions-container[data-id='#{last_message["data-id"]}']",

plugins/chat/spec/system/page_objects/chat/components/messages.rb

@@ -6,7 +6,7 @@ module PageObjects
       class Messages < PageObjects::Components::Base
         attr_reader :context
 
-        SELECTOR = ".chat-messages-scroll"
+        SELECTOR = ".chat-messages-scroller"
 
         def initialize(context)
           @context = context

plugins/chat/spec/system/single_thread_spec.rb

@@ -34,6 +34,17 @@ describe "Single thread in side panel", type: :system do
     fab!(:channel) { Fabricate(:chat_channel, threading_enabled: true) }
     fab!(:thread) { chat_thread_chain_bootstrap(channel: channel, users: [current_user, user_2]) }
 
+    context "when returning to a thread where last read is not last message" do
+      it "scrolls to the correct last read message" do
+        message_1 = Fabricate(:chat_message, thread: thread, chat_channel: channel)
+        thread.membership_for(current_user).update!(last_read_message: message_1)
+        messages = Fabricate.times(50, :chat_message, thread: thread, chat_channel: channel)
+        chat_page.visit_thread(thread)
+
+        expect(page).to have_css("[data-id='#{message_1.id}'].-highlighted")
+      end
+    end
+
     context "when in full page" do
       context "when switching channel" do
         fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: true) }

plugins/chat/spec/system/update_last_read_spec.rb

@@ -23,8 +23,8 @@ RSpec.describe "Update last read", type: :system do
       chat_page.visit_channel(channel_1)
 
       try_until_success do
-        page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, 1)")
+        page.execute_script("document.querySelector('.chat-messages-scroller').scrollTo(0, 1)")
-        page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, 0)")
+        page.execute_script("document.querySelector('.chat-messages-scroller').scrollTo(0, 0)")
         expect(membership.reload.last_read_message_id).to eq(last_message.id)
       end
     end