Merge pull request #9006 from notbakaneko/feature/channel-mark-as-rea…

…d-tweak

Improve chat channel unread marker handling

resources/assets/lib/chat/chat-state-store.ts

@@ -144,10 +144,10 @@ export default class ChatStateStore implements DispatchListener {
     // noticeable when navigating via ?sendto= on existing channel.
     if (this.selected === channelId) return;
 
-    // mark the channel being switched away from as read.
-    if (this.selectedChannel != null) {
-      this.channelStore.markAsRead(this.selectedChannel.channelId);
-    }
+    // Mark the channel being switched away from as read.
+    // Marking as read is done here to avoid constantly sending mark-as-read requests
+    // while receiving messages when autoScroll is enabled on the channel.
+    this.selectedChannel?.throttledSendMarkAsRead();
 
     this.selected = channelId;
 

resources/assets/lib/chat/conversation-view.tsx

@@ -101,8 +101,10 @@ export default class ConversationView extends React.Component<Props> {
     disposeOnUnmount(
       this,
       reaction(() => core.windowFocusObserver.hasFocus, (value) => {
-        if (value) {
-          this.maybeMarkAsRead();
+        // mark as read when regaining focus and at the bottom to the channel.
+        if (value && this.currentChannel?.uiState.autoScroll) {
+          this.currentChannel.moveMarkAsReadMarker();
+          this.currentChannel.throttledSendMarkAsRead();
         }
       }),
     );
@@ -264,18 +266,17 @@ export default class ConversationView extends React.Component<Props> {
 
     this.currentChannel.uiState.autoScroll = chatView.scrollTop + chatView.clientHeight >= chatView.scrollHeight;
     this.currentChannel.uiState.scrollY = chatView.scrollTop;
+    // keep marker at the end when autoScrolling but only if window has focus.
+    if (this.currentChannel.uiState.autoScroll && core.windowFocusObserver.hasFocus) {
+      this.currentChannel.moveMarkAsReadMarker();
+    }
   };
 
   private loadEarlierMessages = () => {
     if (this.currentChannel == null) return;
     core.dataStore.channelStore.loadChannelEarlierMessages(this.currentChannel.channelId);
   };
 
-  private maybeMarkAsRead() {
-    if (this.currentChannel == null) return;
-    core.dataStore.channelStore.markAsRead(this.currentChannel.channelId);
-  }
-
   private renderCannotSendMessage() {
     if (this.currentChannel == null) {
       // this shouldn't happen...

resources/assets/lib/interfaces/chat/channel-json.ts

@@ -16,7 +16,7 @@ export default interface ChannelJson {
   current_user_attributes?: {
     can_message: boolean;
     can_message_error: string | null;
-    last_read_id: number;
+    last_read_id: number | null;
   };
   description?: string;
   icon?: string;

resources/assets/lib/models/chat/channel.ts

@@ -1,10 +1,10 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the GNU Affero General Public License v3.0.
 // See the LICENCE file in the repository root for full licence text.
 
-import { getChannel, getMessages } from 'chat/chat-api';
+import { markAsRead, getChannel, getMessages } from 'chat/chat-api';
 import ChannelJson, { ChannelType, SupportedChannelType, supportedTypeLookup } from 'interfaces/chat/channel-json';
 import MessageJson from 'interfaces/chat/message-json';
-import { minBy, sortBy } from 'lodash';
+import { minBy, sortBy, throttle } from 'lodash';
 import { action, computed, makeObservable, observable, runInAction } from 'mobx';
 import User, { usernameSortAscending } from 'models/user';
 import core from 'osu-core-singleton';
@@ -25,13 +25,15 @@ export default class Channel {
   @observable name = '';
   needsRefresh = true;
   @observable newPmChannel = false;
+  readonly throttledSendMarkAsRead = throttle(() => this.sendMarkAsRead(), 1000);
   @observable type: ChannelType = 'TEMPORARY'; // TODO: look at making this support channels only
   @observable uiState = {
     autoScroll: true,
     scrollY: 0,
   };
   @observable userIds: number[] = [];
 
+  private markAsReadLastSent = 0;
   @observable private messagesMap = new Map<number | string, Message>();
   private serverLastMessageId?: number;
   @observable private usersLoaded = false;
@@ -162,7 +164,7 @@ export default class Channel {
   @action
   addSendingMessage(message: Message) {
     this.messagesMap.set(message.messageId, message);
-    this.markAsRead();
+    this.moveMarkAsReadMarker();
   }
 
   @action
@@ -229,7 +231,7 @@ export default class Channel {
   }
 
   @action
-  markAsRead() {
+  moveMarkAsReadMarker() {
     this.setLastReadId(this.lastMessageId);
   }
 
@@ -259,7 +261,11 @@ export default class Channel {
 
     if (json.current_user_attributes != null) {
       this.canMessageError = json.current_user_attributes.can_message_error;
-      this.setLastReadId(json.current_user_attributes.last_read_id);
+      const lastReadId = json.current_user_attributes.last_read_id ?? 0;
+      this.setLastReadId(lastReadId);
+      if (lastReadId > this.markAsReadLastSent) {
+        this.markAsReadLastSent = lastReadId;
+      }
     }
   }
 
@@ -311,6 +317,15 @@ export default class Channel {
     this.messagesMap.set(message.messageId, message);
   }
 
+  @action
+  private sendMarkAsRead() {
+    const lastReadId = this.lastReadId ?? 0;
+    if (this.markAsReadLastSent >= lastReadId) return;
+
+    this.markAsReadLastSent = lastReadId;
+    markAsRead(this.channelId, lastReadId);
+  }
+
   @action
   private setLastReadId(id: number) {
     if (id > (this.lastReadId ?? 0)) {

resources/assets/lib/stores/channel-store.ts

@@ -6,7 +6,7 @@ import { ChatNewConversationAdded } from 'actions/chat-new-conversation-added';
 import ChatUpdateSilences from 'actions/chat-update-silences';
 import DispatcherAction from 'actions/dispatcher-action';
 import { dispatch, dispatchListener } from 'app-dispatcher';
-import { markAsRead as apiMarkAsRead, newConversation, partChannel as apiPartChannel, sendMessage } from 'chat/chat-api';
+import { newConversation, partChannel as apiPartChannel, sendMessage } from 'chat/chat-api';
 import MessageNewEvent from 'chat/message-new-event';
 import DispatchListener from 'dispatch-listener';
 import ChannelJson, { filterSupportedChannelTypes, SupportedChannelType, supportedChannelTypes } from 'interfaces/chat/channel-json';
@@ -54,8 +54,6 @@ export default class ChannelStore implements DispatchListener {
   @observable channels = observable.map<number, Channel>();
   lastReceivedMessageId = 0;
 
-  private markingAsRead: Partial<Record<number, number>> = {};
-
   @computed
   get groupedChannels() {
     const grouped = makeEmptyGroupedChannels();
@@ -135,39 +133,6 @@ export default class ChannelStore implements DispatchListener {
     this.get(channelId)?.loadEarlierMessages();
   }
 
-  @action
-  markAsRead(channelId: number) {
-    const channel = this.get(channelId);
-
-    if (channel == null || !channel.isUnread || !channel.uiState.autoScroll) {
-      return;
-    }
-
-    if (this.markingAsRead[channelId] != null) {
-      return;
-    }
-
-    channel.markAsRead();
-
-    const currentTimeout = window.setTimeout(action(() => {
-      // allow next debounce to be queued again
-      if (this.markingAsRead[channelId] === currentTimeout) {
-        delete this.markingAsRead[channelId];
-      }
-
-      // TODO: need to mark again in case the marker has moved?
-
-      // We don't need to send mark-as-read for our own messages, as the cursor is automatically bumped forward server-side when sending messages.
-      if (channel.lastMessage?.sender.id === core.currentUser?.id) {
-        return;
-      }
-
-      apiMarkAsRead(channel.channelId, channel.lastMessageId);
-    }), 1000);
-
-    this.markingAsRead[channelId] = currentTimeout;
-  }
-
   @action
   partChannel(channelId: number, remote = true) {
     if (channelId > 0 && remote) {