FIX: ensures last read is updated on new message (#26772)

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

@@ -20,6 +20,7 @@ import i18n from "discourse-common/helpers/i18n";
 import discourseDebounce from "discourse-common/lib/debounce";
 import { bind } from "discourse-common/utils/decorators";
 import ChatChannelStatus from "discourse/plugins/chat/discourse/components/chat-channel-status";
+import firstVisibleMessageId from "discourse/plugins/chat/discourse/helpers/first-visible-message-id";
 import ChatChannelSubscriptionManager from "discourse/plugins/chat/discourse/lib/chat-channel-subscription-manager";
 import {
   FUTURE,
@@ -412,27 +413,27 @@ export default class ChatChannel extends Component {
       return;
     }
 
-    if (!this.lastFullyVisibleMessageId) {
+    const firstFullyVisibleMessageId = firstVisibleMessageId(this.scroller);
+    if (!firstFullyVisibleMessageId) {
       return;
     }
 
-    let lastUnreadVisibleMessage = this.messagesManager.findMessage(
+    let firstMessage = this.messagesManager.findMessage(
-      this.lastFullyVisibleMessageId
+      firstFullyVisibleMessageId
     );
-
+    if (!firstMessage) {
-    if (!lastUnreadVisibleMessage) {
       return;
     }
 
     const lastReadId =
       this.args.channel.currentUserMembership?.lastReadMessageId;
-    if (lastReadId >= lastUnreadVisibleMessage.id) {
+    if (lastReadId >= firstMessage.id) {
       return;
     }
 
     return this.chatApi.markChannelAsRead(
       this.args.channel.id,
-      lastUnreadVisibleMessage.id
+      firstMessage.id
     );
   }
 
@@ -460,7 +461,6 @@ export default class ChatChannel extends Component {
         (state.distanceToBottom.pixels > 250 && !state.atBottom);
       this.isScrolling = true;
       this.debouncedUpdateLastReadMessage();
-      this.lastFullyVisibleMessageId = state.lastVisibleId;
 
       if (
         state.atTop ||
@@ -484,7 +484,6 @@ 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 });
@@ -492,7 +491,7 @@ export default class ChatChannel extends Component {
     } else {
       this.chatChannelScrollPositions.set(
         this.args.channel.id,
-        state.lastVisibleId
+        state.firstVisibleId
       );
     }
   }

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

@@ -12,6 +12,7 @@ import { resetIdle } from "discourse/lib/desktop-notifications";
 import { NotificationLevels } from "discourse/lib/notification-levels";
 import discourseDebounce from "discourse-common/lib/debounce";
 import { bind } from "discourse-common/utils/decorators";
+import firstVisibleMessageId from "discourse/plugins/chat/discourse/helpers/first-visible-message-id";
 import ChatChannelThreadSubscriptionManager from "discourse/plugins/chat/discourse/lib/chat-channel-thread-subscription-manager";
 import {
   FUTURE,
@@ -123,7 +124,6 @@ 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,7 +148,6 @@ 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 });
@@ -169,27 +168,27 @@ export default class ChatThread extends Component {
       return;
     }
 
-    if (!this.lastFullyVisibleMessageId) {
+    const firstFullyVisibleMessageId = firstVisibleMessageId(this.scroller);
+    if (!firstFullyVisibleMessageId) {
       return;
     }
 
-    const lastUnreadVisibleMessage = this.messagesManager.findMessage(
+    const firstMessage = this.messagesManager.findMessage(
-      this.lastFullyVisibleMessageId
+      firstFullyVisibleMessageId
     );
-
+    if (!firstMessage) {
-    if (!lastUnreadVisibleMessage) {
       return;
     }
 
     const lastReadId = this.args.thread.currentUserMembership.lastReadMessageId;
-    if (lastReadId >= lastUnreadVisibleMessage.id) {
+    if (lastReadId >= firstMessage.id) {
       return;
     }
 
     return this.chatApi.markThreadAsRead(
       this.args.thread.channel.id,
       this.args.thread.id,
-      lastUnreadVisibleMessage.id
+      firstMessage.id
     );
   }
 

plugins/chat/assets/javascripts/discourse/helpers/first-visible-message-id.js

@@ -0,0 +1,20 @@
+import { checkMessageBottomVisibility } from "discourse/plugins/chat/discourse/lib/check-message-visibility";
+
+export default function firstVisibleMessageId(container) {
+  let _found;
+  const messages = container.querySelectorAll(
+    ":scope .chat-messages-container > [data-id]"
+  );
+
+  for (let i = messages.length - 1; i >= 0; i--) {
+    const message = messages[i];
+
+    if (checkMessageBottomVisibility(container, message)) {
+      _found = message;
+      break;
+    }
+  }
+
+  const id = _found?.dataset?.id;
+  return id ? parseInt(id, 10) : null;
+}

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

@@ -3,7 +3,7 @@ import { cancel, throttle } from "@ember/runloop";
 import Modifier from "ember-modifier";
 import discourseLater from "discourse-common/lib/later";
 import { bind } from "discourse-common/utils/decorators";
-import { checkMessageBottomVisibility } from "discourse/plugins/chat/discourse/lib/check-message-visibility";
+import firstVisibleMessageId from "discourse/plugins/chat/discourse/helpers/first-visible-message-id";
 
 const UP = "up";
 const DOWN = "down";
@@ -54,7 +54,7 @@ export default class ChatScrollableList extends Modifier {
     this.scrollTimer = discourseLater(() => {
       this.options.onScrollEnd?.(
         Object.assign(this.computeState(), {
-          lastVisibleId: this.computeFirstVisibleMessageId(),
+          firstVisibleId: firstVisibleMessageId(this.element),
         })
       );
     }, this.options.delay || 250);
@@ -134,23 +134,4 @@ export default class ChatScrollableList extends Modifier {
 
     return this.element.scrollTop < this.lastScrollTop ? UP : DOWN;
   }
-
-  computeFirstVisibleMessageId() {
-    let firstVisibleMessage;
-    const messages = this.element.querySelectorAll(
-      ":scope .chat-messages-container > [data-id]"
-    );
-
-    for (let i = messages.length - 1; i >= 0; i--) {
-      const message = messages[i];
-
-      if (checkMessageBottomVisibility(this.element, message)) {
-        firstVisibleMessage = message;
-        break;
-      }
-    }
-
-    const id = firstVisibleMessage?.dataset?.id;
-    return id ? parseInt(id, 10) : null;
-  }
 }

plugins/chat/spec/system/update_last_read_spec.rb

@@ -3,7 +3,7 @@
 RSpec.describe "Update last read", type: :system do
   fab!(:current_user) { Fabricate(:user) }
   fab!(:channel_1) { Fabricate(:chat_channel) }
-  fab!(:first_unread) { Fabricate(:chat_message, chat_channel: channel_1) }
+  fab!(:messages) { Fabricate.times(15, :chat_message, chat_channel: channel_1) }
 
   let(:chat_page) { PageObjects::Pages::Chat.new }
   let(:channel_page) { PageObjects::Pages::ChatChannel.new }
@@ -12,21 +12,27 @@ RSpec.describe "Update last read", type: :system do
   before do
     chat_system_bootstrap
     channel_1.add(current_user)
-    membership.update!(last_read_message_id: first_unread.id)
+
-    Fabricate.times(25, :chat_message, chat_channel: channel_1)
+    membership.update!(last_read_message_id: messages.last.id)
     sign_in(current_user)
   end
 
   context "when the full message is visible" do
-    xit "marks it as read" do
+    it "marks it as read" do
       last_message = Fabricate(:chat_message, chat_channel: channel_1)
       chat_page.visit_channel(channel_1)
 
-      try_until_success do
+      try_until_success { expect(membership.reload.last_read_message_id).to eq(last_message.id) }
-        page.execute_script("document.querySelector('.chat-messages-scroller').scrollTo(0, 1)")
+    end
-        page.execute_script("document.querySelector('.chat-messages-scroller').scrollTo(0, 0)")
+  end
-        expect(membership.reload.last_read_message_id).to eq(last_message.id)
+
-      end
+  context "when receiving a messages" do
+    it "marks it as read" do
+      chat_page.visit_channel(channel_1)
+
+      last_message = Fabricate(:chat_message, chat_channel: channel_1, use_service: true)
+
+      try_until_success { expect(membership.reload.last_read_message_id).to eq(last_message.id) }
     end
   end