FIX: improves unread state precision (#20615)

- Will consider a message read only one the bottom of the message has been read
- Will allow to mark a message bigger than the view port as read
- Code should be more performant as the scroll is doing less (albeit more often)
- Gives us a very precise scroll state. Problem with throttling scroll is that you could end up never getting the even where scrollTop is at 0, opening a whole range of edge cases to handle

plugins/chat/assets/javascripts/discourse/components/chat-live-pane.hbs

@@ -40,17 +40,16 @@
 
   <div
     class="chat-messages-scroll chat-messages-container"
-    {{chat/on-throttled-scroll this.computeScrollState (hash delay=50)}}
+    {{on "scroll" this.computeScrollState passive=true}}
     {{chat/on-throttled-scroll this.resetIdle (hash delay=500)}}
+    {{chat/on-throttled-scroll this.computeArrow (hash delay=150)}}
   >
     <div class="chat-message-actions-desktop-anchor"></div>
     <div
       class="chat-messages-container"
       {{chat/did-mutate-childlist this.computeDatesSeparators}}
     >
-
       {{#if this.loadedOnce}}
-
         {{#each @channel.messages key="id" as |message|}}
           <ChatMessage
             @message={{message}}
@@ -66,8 +65,8 @@
             @isHovered={{eq message.id this.hoveredMessageId}}
             @onHoverMessage={{this.onHoverMessage}}
             @resendStagedMessage={{this.resendStagedMessage}}
-            @didShowMessage={{this.didShowMessage}}
+            @messageDidEnterViewport={{this.messageDidEnterViewport}}
-            @didHideMessage={{this.didHideMessage}}
+            @messageDidLeaveViewport={{this.messageDidLeaveViewport}}
             @forceRendering={{this.forceRendering}}
           />
         {{/each}}

plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js

@@ -53,14 +53,8 @@ export default class ChatLivePane extends Component {
   @tracked needsArrow = false;
   @tracked loadedOnce = false;
 
-  isAtBottom = true;
-  isTowardsBottom = false;
-  isTowardsTop = false;
-  isAtTop = false;
-
   _loadedChannelId = null;
   _scrollerEl = null;
-  _previousScrollTop = 0;
   _lastSelectedMessage = null;
   _mentionWarningsSeen = {};
   _unreachableGroupMentions = [];
@@ -97,6 +91,8 @@ export default class ChatLivePane extends Component {
 
   @action
   updateChannel() {
+    this.loadedOnce = false;
+
     // Technically we could keep messages to avoid re-fetching them, but
     // it's not worth the complexity for now
     this.args.channel?.clearMessages();
@@ -109,7 +105,7 @@ export default class ChatLivePane extends Component {
     }
 
     this.loadMessages();
-    this._subscribeToUpdates(this.args.channel.id);
+    this._subscribeToUpdates(this.args.channel?.id);
   }
 
   @action
@@ -158,8 +154,6 @@ export default class ChatLivePane extends Component {
       return;
     }
 
-    this.loadedOnce = false;
-    this._previousScrollTop = 0;
     this.loadingMorePast = true;
 
     const findArgs = { pageSize: PAGE_SIZE };
@@ -167,7 +161,8 @@ export default class ChatLivePane extends Component {
     if (this.requestedTargetMessageId) {
       findArgs["targetMessageId"] = this.requestedTargetMessageId;
     } else if (fetchingFromLastRead) {
-      findArgs["targetMessageId"] = this._getLastReadId();
+      findArgs["targetMessageId"] =
+        this.args.channel.currentUserMembership.last_read_message_id;
     }
 
     return this.chatApi
@@ -208,6 +203,7 @@ export default class ChatLivePane extends Component {
         this.requestedTargetMessageId = null;
         this.loadingMorePast = false;
         this.fillPaneAttempt();
+        this.updateLastReadMessage();
       });
   }
 
@@ -224,7 +220,7 @@ export default class ChatLivePane extends Component {
       !canLoadMore ||
       this.loading ||
       this[loadingMoreKey] ||
-      !this.args.channel.messages.length
+      !this.args.channel.messages?.length > 0
     ) {
       return Promise.resolve();
     }
@@ -254,7 +250,7 @@ export default class ChatLivePane extends Component {
 
         // prevents an edge case where user clicks bottom arrow
         // just after scrolling to top
-        if (loadingPast && this.isAtBottom) {
+        if (loadingPast && this.#isAtBottom()) {
           return;
         }
 
@@ -268,7 +264,6 @@ export default class ChatLivePane extends Component {
         }
 
         this.args.channel.details = meta;
-
         this.args.channel.addMessages(messages);
 
         // Edge case for IOS to avoid blank screens
@@ -293,14 +288,14 @@ export default class ChatLivePane extends Component {
       });
   }
 
-  @debounce(500, false)
+  @debounce(500)
   fillPaneAttempt() {
     if (this._selfDeleted) {
       return;
     }
 
     // safeguard
-    if (this.args.channel.messages.length > 200) {
+    if (this.args.channel.messages?.length > 200) {
       return;
     }
 
@@ -313,9 +308,7 @@ export default class ChatLivePane extends Component {
       return;
     }
 
-    this.fetchMoreMessages({
+    this.fetchMoreMessages({ direction: PAST });
-      direction: PAST,
-    });
   }
 
   @bind
@@ -361,10 +354,6 @@ export default class ChatLivePane extends Component {
     return [messages, results.meta];
   }
 
-  _getLastReadId() {
-    return this.args.channel.currentUserMembership.last_read_message_id;
-  }
-
   @debounce(100)
   highlightOrFetchMessage(messageId) {
     const message = this.args.channel.findMessage(messageId);
@@ -423,30 +412,52 @@ export default class ChatLivePane extends Component {
   }
 
   @action
-  didShowMessage(message) {
+  messageDidEnterViewport(message) {
     message.visible = true;
-    this.updateLastReadMessage(message);
   }
 
   @action
-  didHideMessage(message) {
+  messageDidLeaveViewport(message) {
     message.visible = false;
   }
 
   @debounce(READ_INTERVAL_MS)
   updateLastReadMessage() {
-    if (this._selfDeleted) {
+    schedule("afterRender", () => {
-      return;
+      if (this._selfDeleted) {
-    }
+        return;
+      }
+
+      const lastReadId =
+        this.args.channel.currentUserMembership?.last_read_message_id;
+      let lastUnreadVisibleMessage = this.args.channel.visibleMessages.findLast(
+        (message) => !lastReadId || message.id > lastReadId
+      );
+
+      // all intersecting messages are read
+      if (!lastUnreadVisibleMessage) {
+        return;
+      }
+
+      const element = this._scrollerEl.querySelector(
+        `[data-id='${lastUnreadVisibleMessage.id}']`
+      );
+
+      // if the last visible message is not fully visible, we don't want to mark it as read
+      // attempt to mark previous one as read
+      if (!this.#isBottomOfMessageVisible(element, this._scrollerEl)) {
+        lastUnreadVisibleMessage = lastUnreadVisibleMessage.previousMessage;
+
+        if (
+          !lastUnreadVisibleMessage &&
+          lastReadId > lastUnreadVisibleMessage.id
+        ) {
+          return;
+        }
+      }
 
-    const lastReadId =
-      this.args.channel.currentUserMembership?.last_read_message_id;
-    const lastUnreadVisibleMessage = this.args.channel.visibleMessages.findLast(
-      (message) => !lastReadId || message.id > lastReadId
-    );
-    if (lastUnreadVisibleMessage) {
       this.args.channel.updateLastReadMessage(lastUnreadVisibleMessage.id);
-    }
+    });
   }
 
   @action
@@ -454,7 +465,7 @@ export default class ChatLivePane extends Component {
     schedule("afterRender", () => {
       if (this.args.channel.canLoadMoreFuture) {
         this._fetchAndScrollToLatest();
-      } else {
+      } else if (this.args.channel.messages?.length > 0) {
         this.scrollToMessage(
           this.args.channel.messages[this.args.channel.messages.length - 1].id
         );
@@ -463,51 +474,31 @@ export default class ChatLivePane extends Component {
   }
 
   @action
-  computeScrollState(event) {
+  computeArrow() {
-    if (this._selfDeleted) {
+    this.needsArrow = Math.abs(this._scrollerEl.scrollTop) >= 100;
-      return;
+  }
-    }
 
+  @action
+  computeScrollState() {
     cancel(this.onScrollEndedHandler);
 
-    this.isScrolling = true;
+    if (this.#isAtTop()) {
-
+      this.fetchMoreMessages({ direction: PAST });
-    const scrollPosition = Math.abs(event.target.scrollTop);
+      this.onScrollEnded();
-    const total = event.target.scrollHeight - event.target.clientHeight;
+    } else if (this.#isAtBottom()) {
-    const difference = total - scrollPosition;
+      this.updateLastReadMessage();
-    this.isTowardsTop = difference >= 50 && difference <= 150;
+      this.hasNewMessages = false;
-    this.isTowardsBottom = scrollPosition >= 50 && scrollPosition <= 150;
+      this.fetchMoreMessages({ direction: FUTURE });
-    this.needsArrow = scrollPosition >= 50;
+      this.onScrollEnded();
-    this.isAtBottom = scrollPosition < 50;
-    this.isAtTop = difference < 50;
-
-    if (this._previousScrollTop - scrollPosition <= 0) {
-      if (this.isAtTop) {
-        this.fetchMoreMessages({ direction: PAST });
-      }
     } else {
-      if (this.isAtBottom) {
+      this.isScrolling = true;
-        this.fetchMoreMessages({ direction: FUTURE });
+      this.onScrollEndedHandler = discourseLater(this, this.onScrollEnded, 150);
-      }
     }
-
-    this._previousScrollTop = scrollPosition;
-    this.onScrollEndedHandler = discourseLater(this, this.onScrollEnded, 150);
   }
 
   @bind
   onScrollEnded() {
     this.isScrolling = false;
-
-    if (this.isAtBottom) {
-      this.hasNewMessages = false;
-    }
-  }
-
-  _isBetween(target, a, b) {
-    const min = Math.min.apply(Math, [a, b]);
-    const max = Math.max.apply(Math, [a, b]);
-    return target > min && target < max;
   }
 
   removeMessage(msgData) {
@@ -594,7 +585,7 @@ export default class ChatLivePane extends Component {
     if (this.args.channel.canLoadMoreFuture) {
       // If we can load more messages, we just notice the user of new messages
       this.hasNewMessages = true;
-    } else if (this.isAtBottom || this.isTowardsBottom) {
+    } else if (this.#isTowardsBottom()) {
       // If we are at the bottom, we append the message and scroll to it
       const message = ChatMessage.create(this.args.channel, data.chat_message);
       this.args.channel.addMessages([message]);
@@ -1141,6 +1132,10 @@ export default class ChatLivePane extends Component {
   }
 
   _subscribeToUpdates(channelId) {
+    if (!channelId) {
+      return;
+    }
+
     this._unsubscribeToUpdates(channelId);
     this.messageBus.subscribe(
       `/chat/${channelId}`,
@@ -1277,4 +1272,26 @@ export default class ChatLivePane extends Component {
         });
     });
   }
+
+  #isAtBottom() {
+    return Math.abs(this._scrollerEl.scrollTop) <= 2;
+  }
+
+  #isTowardsBottom() {
+    return Math.abs(this._scrollerEl.scrollTop) <= 50;
+  }
+
+  #isAtTop() {
+    return (
+      Math.abs(this._scrollerEl.scrollTop) >=
+      this._scrollerEl.scrollHeight - this._scrollerEl.offsetHeight - 2
+    );
+  }
+
+  #isBottomOfMessageVisible(element, container) {
+    const rect = element.getBoundingClientRect();
+    const containerRect = container.getBoundingClientRect();
+    // - 1.0 to account for rounding errors, especially on firefox
+    return rect.bottom - 1.0 <= containerRect.bottom;
+  }
 }

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

@@ -56,8 +56,10 @@
   }}
   data-id={{@message.id}}
   {{chat/track-message
-    (fn @didShowMessage @message)
+    (hash
-    (fn @didHideMessage @message)
+      didEnterViewport=(fn @messageDidEnterViewport @message)
+      didLeaveViewport=(fn @messageDidLeaveViewport @message)
+    )
   }}
 >
   {{#if this.show}}

plugins/chat/assets/javascripts/discourse/modifiers/chat/track-message.js

@@ -3,23 +3,23 @@ import { registerDestructor } from "@ember/destroyable";
 import { bind } from "discourse-common/utils/decorators";
 
 export default class ChatTrackMessage extends Modifier {
-  visibleCallback = null;
+  didEnterViewport = null;
-  notVisibleCallback = null;
+  didLeaveViewport = null;
 
   constructor(owner, args) {
     super(owner, args);
     registerDestructor(this, (instance) => instance.cleanup());
   }
 
-  modify(element, [visibleCallback, notVisibleCallback]) {
+  modify(element, [callbacks = {}]) {
-    this.visibleCallback = visibleCallback;
+    this.didEnterViewport = callbacks.didEnterViewport;
-    this.notVisibleCallback = notVisibleCallback;
+    this.didLeaveViewport = callbacks.didLeaveViewport;
 
     this.intersectionObserver = new IntersectionObserver(
       this._intersectionObserverCallback,
       {
         root: document,
-        threshold: 0.9,
+        threshold: 0,
       }
     );
 
@@ -34,9 +34,9 @@ export default class ChatTrackMessage extends Modifier {
   _intersectionObserverCallback(entries) {
     entries.forEach((entry) => {
       if (entry.isIntersecting) {
-        this.visibleCallback?.();
+        this.didEnterViewport?.();
       } else {
-        this.notVisibleCallback?.();
+        this.didLeaveViewport?.();
       }
     });
   }

plugins/chat/assets/stylesheets/common/chat-message-left-gutter.scss

@@ -6,16 +6,15 @@
   width: var(--message-left-width);
 }
 
+.chat-message-container.is-hovered .chat-message-left-gutter {
+  .chat-time {
+    color: var(--secondary-mediumy);
+  }
+}
+
 .chat-message-left-gutter__date {
   color: var(--primary-high);
   font-size: var(--font-down-1);
-
-  &:hover,
-  &:focus {
-    .chat-time {
-      color: var(--primary);
-    }
-  }
 }
 
 .chat-message-left-gutter__flag {

plugins/chat/spec/system/update_last_read.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+RSpec.describe "Update last read", type: :system, js: true do
+  fab!(:current_user) { Fabricate(:user) }
+  fab!(:channel_1) { Fabricate(:chat_channel) }
+  fab!(:first_unread) { Fabricate(:chat_message, chat_channel: channel_1) }
+
+  let(:chat_page) { PageObjects::Pages::Chat.new }
+  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+  let(:membership) { Chat::ChatChannelMembershipManager.new(channel_1).find_for_user(current_user) }
+
+  before do
+    chat_system_bootstrap
+    channel_1.add(current_user)
+    membership.update!(last_read_message_id: first_unread.id)
+    25.times { |i| Fabricate(:chat_message, chat_channel: channel_1) }
+    sign_in(current_user)
+  end
+
+  context "when the full message is not visible" do
+    it "doesn’t mark it as read" do
+      before_last_message = Fabricate(:chat_message, chat_channel: channel_1)
+      last_message = Fabricate(:chat_message, chat_channel: channel_1)
+      chat_page.visit_channel(channel_1)
+
+      page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, -5)")
+
+      try_until_success(timeout: 5) do
+        expect(membership.reload.last_read_message_id).to eq(before_last_message.id)
+      end
+    end
+  end
+
+  context "when the full message is visible" do
+    it "marks it as read" do
+      last_message = Fabricate(:chat_message, chat_channel: channel_1)
+      chat_page.visit_channel(channel_1)
+
+      page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, 0)")
+
+      try_until_success(timeout: 5) do
+        expect(membership.reload.last_read_message_id).to eq(last_message.id)
+      end
+    end
+  end
+
+  context "when user had not previous last read" do
+    before { membership.update!(last_read_message_id: nil) }
+
+    it "marks last message as read" do
+      last_message = Fabricate(:chat_message, chat_channel: channel_1)
+      chat_page.visit_channel(channel_1)
+
+      try_until_success(timeout: 5) do
+        expect(membership.reload.last_read_message_id).to eq(last_message.id)
+      end
+    end
+  end
+
+  context "when scrolling from not visible to bottom" do
+    it "marks last message as read" do
+      before_last_message = Fabricate(:chat_message, chat_channel: channel_1)
+      last_message = Fabricate(:chat_message, chat_channel: channel_1)
+      chat_page.visit_channel(channel_1)
+
+      page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, -15)")
+
+      try_until_success(timeout: 5) do
+        expect(membership.reload.last_read_message_id).to eq(before_last_message.id)
+      end
+
+      page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, -1)")
+
+      try_until_success(timeout: 5) do
+        expect(membership.reload.last_read_message_id).to eq(last_message.id)
+      end
+    end
+  end
+end

plugins/chat/test/javascripts/components/chat-message-test.js

@@ -55,8 +55,8 @@ module("Discourse Chat | Component | chat-message", function (hooks) {
       onSelectMessage: () => {},
       bulkSelectMessages: () => {},
       onHoverMessage: () => {},
-      didShowMessage: () => {},
+      messageDidEnterViewport: () => {},
-      didHideMessage: () => {},
+      messageDidLeaveViewport: () => {},
       forceRendering: () => {},
     };
   }
@@ -74,9 +74,8 @@ module("Discourse Chat | Component | chat-message", function (hooks) {
       @onSelectMessage={{this.onSelectMessage}}
       @bulkSelectMessages={{this.bulkSelectMessages}}
       @onHoverMessage={{this.onHoverMessage}}
-      @didShowMessage={{this.didShowMessage}}
+      @messageDidEnterViewport={{this.messageDidEnterViewport}}
-      @didHideMessage={{this.didHideMessage}}
+      @messageDidLeaveViewport={{this.messageDidLeaveViewport}}
-      @didHideMessage={{this.didHideMessage}}
       @forceRendering={{this.forceRendering}}
     />
   `;