From 6562ea94e43a0085244d850e7927da6efe91bc04 Mon Sep 17 00:00:00 2001
From: Tim Abbott <tabbott@zulip.com>
Date: Wed, 31 Jan 2024 13:22:34 -0800
Subject: [PATCH] unread: Stop treating bottom of render windows as the global
 bottom.

The previous logic for both scrolling down and using pagedown would
incorrectly mark an entire conversation as read when reaching the
bottom of a render window, even if there were more messages loaded or
to fetch from the server.

Fix this error in the calculation by asking the correct data
structures if we're actually at the bottom.

To avoid the navigate.js keyboard shortcut code paths circumventing
this new logic, or needing to duplicate it, they now call
process_visible, rather than its helper.
---
 web/src/message_list_view.js | 12 ++++++++++++
 web/src/message_viewport.ts  |  4 ++++
 web/src/navigate.js          |  6 +++---
 web/src/unread_ops.js        | 10 +++++++---
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/web/src/message_list_view.js b/web/src/message_list_view.js
index 87c042e60e..a729fac4b8 100644
--- a/web/src/message_list_view.js
+++ b/web/src/message_list_view.js
@@ -1439,6 +1439,18 @@ export class MessageListView {
         this.message_containers.clear();
     }
 
+    is_end_rendered() {
+        // Used as a helper in checks for whether a given scroll
+        // position is actually the very end of this view. It could
+        // fail to be for two reasons: Either some newer messages are
+        // not rendered due to a render window, or we haven't finished
+        // fetching the newest messages for this view from the server.
+        return (
+            this._render_win_end === this.list.num_items() &&
+            this.list.data.fetch_status.has_found_newest()
+        );
+    }
+
     get_row(id) {
         const $row = this._rows.get(id);
 
diff --git a/web/src/message_viewport.ts b/web/src/message_viewport.ts
index 00207eb955..ca5d93b7d7 100644
--- a/web/src/message_viewport.ts
+++ b/web/src/message_viewport.ts
@@ -71,6 +71,10 @@ export function message_viewport_info(): MessageViewportInfo {
     };
 }
 
+// Important note: These functions just look at the state of the
+// rendered message feed; messages that are not displayed due to a
+// limited render window or because they have not been fetched from
+// the server are not considered.
 export function at_bottom(): boolean {
     const bottom = scrollTop() + height();
     const full_height = $scroll_container.prop("scrollHeight");
diff --git a/web/src/navigate.js b/web/src/navigate.js
index c1fb520f0d..e0ee50bb77 100644
--- a/web/src/navigate.js
+++ b/web/src/navigate.js
@@ -26,7 +26,7 @@ export function down(with_centering) {
             message_viewport.scrollTop(
                 ($current_msg_list.outerHeight(true) ?? 0) - message_viewport.height() * 0.1,
             );
-            unread_ops.process_scrolled_to_bottom();
+            unread_ops.process_visible();
         }
 
         return;
@@ -50,7 +50,7 @@ export function to_end() {
     const next_id = message_lists.current.last().id;
     message_viewport.set_last_movement_direction(1);
     message_lists.current.select_id(next_id, {then_scroll: true, from_scroll: true});
-    unread_ops.process_scrolled_to_bottom();
+    unread_ops.process_visible();
 }
 
 function amount_to_paginate() {
@@ -110,7 +110,7 @@ export function page_up() {
 export function page_down() {
     if (message_viewport.at_bottom() && !message_lists.current.visibly_empty()) {
         message_lists.current.select_id(message_lists.current.last().id, {then_scroll: false});
-        unread_ops.process_scrolled_to_bottom();
+        unread_ops.process_visible();
     } else {
         page_down_the_right_amount();
     }
diff --git a/web/src/unread_ops.js b/web/src/unread_ops.js
index 693b4e8b8f..ab637c8091 100644
--- a/web/src/unread_ops.js
+++ b/web/src/unread_ops.js
@@ -435,7 +435,7 @@ export function notify_server_message_read(message, options) {
     notify_server_messages_read([message], options);
 }
 
-export function process_scrolled_to_bottom() {
+function process_scrolled_to_bottom() {
     if (!narrow_state.is_message_feed_visible()) {
         // First, verify the current message list is visible.
         return;
@@ -455,9 +455,13 @@ export function process_scrolled_to_bottom() {
 }
 
 // If we ever materially change the algorithm for this function, we
-// may need to update notifications.received_messages as well.
+// may need to update message_notifications.received_messages as well.
 export function process_visible() {
-    if (viewport_is_visible_and_focused() && message_viewport.bottom_message_visible()) {
+    if (
+        viewport_is_visible_and_focused() &&
+        message_viewport.bottom_message_visible() &&
+        message_lists.current.view.is_end_rendered()
+    ) {
         process_scrolled_to_bottom();
     }
 }