narrow: Fix combined feed selecting random messages on narrow.

Reproducer:

* Have some unreads in the Combined feed view.
* Scroll up and select a message that was not part of initial fetch.
* Reload.
* Go a another narrow.
* Come back to combined feed to find your at a random message. This
  message is actually last message of the current fetch of
  combined feed view which was returned via `first_unread_message_id`.

web/src/message_list.js

@@ -11,6 +11,7 @@ import * as narrow_state from "./narrow_state";
 import {page_params} from "./page_params";
 import {web_mark_read_on_scroll_policy_values} from "./settings_config";
 import * as stream_data from "./stream_data";
+import * as unread from "./unread";
 import {user_settings} from "./user_settings";
 
 export class MessageList {
@@ -68,18 +69,57 @@ export class MessageList {
         // the user. Possibly this can be unified in some nice way.
         this.reading_prevented = false;
 
-        // Whether this message list's is preserved in the DOM even
+        return this;
+    }
+
+    should_preserve_current_rendered_state() {
+        // Whether this message list is preserved in the DOM even
         // when viewing other views -- a valuable optimization for
         // fast toggling between the combined feed and other views,
         // which we enable only when that is the user's home view.
         //
         // This is intentionally not live-updated when web_home_view
         // changes, since it's easier to reason about if this
         // optimization is active or not for an entire session.
-        this.preserve_rendered_state =
-            user_settings.web_home_view === "all_messages" && !this.narrowed;
+        if (user_settings.web_home_view !== "all_messages" || this.narrowed) {
+            return false;
+        }
 
-        return this;
+        // If we click on a narrow, we go the first unread message.
+        // If first unread message is not available in a cached message list,
+        // we render by selecting the `message_list.last()` message.
+        // This is incorrect unless we have found the newest message.
+        //
+        // So, we don't preserve the rendered state of this list if first unread message
+        // is not available to us. Otherwise, this leads to confusion when we are
+        // restoring the rendered list but our first unread message is not available
+        // and fetching it from the server could lead to non-contiguous messages history.
+        //
+        // NOTE: Non-contiguous message history can still happen in the opposite situation
+        // where user is narrowing to a message id which is not present in the rendered list.
+        // In this case, we create a new list and if the new fetched history contains first
+        // unread message, we preserve this list and discard others with the same filter.
+        //
+        // This nicely supports the common workflow of user reloading with a `then_select_id`
+        // and then scrolling to the first unread message; then narrowing to the unread topic
+        // and combing back to combined feed. The combined feed will be rendered in this case
+        // but not if we decided to discard this list based on if anchor was on `first_unread.
+        //
+        // Since we know we are checking for first unread unmuted message in combined feed,
+        // we can use `unread.first_unread_unmuted_message_id` to correctly check if we have
+        // fetched the first unread message.
+        //
+        // TODO: For supporting other narrows, we need to check if we have fetched the first
+        // unread message for that narrow, for which we will have to query server for the
+        // first unread message id. Maybe that can be part of the narrow fetch query itself.
+        const first_unread_message = this.get(unread.first_unread_unmuted_message_id);
+        if (!first_unread_message?.unread) {
+            // If we have found the newest message, we can preserve the rendered state.
+            return this.data.fetch_status.has_found_newest();
+        }
+
+        // If we have found the first unread message, we can preserve the rendered state.
+        return true;
     }
 
     is_current_message_list() {
@@ -146,6 +186,10 @@ export class MessageList {
         return this.data.get(id);
     }
 
+    msg_id_in_fetched_range(msg_id) {
+        return this.data.msg_id_in_fetched_range(msg_id);
+    }
+
     num_items() {
         return this.data.num_items();
     }

web/src/message_list_data.ts

@@ -100,6 +100,14 @@ export class MessageListData {
         return this._all_items.at(-1);
     }
 
+    msg_id_in_fetched_range(msg_id: number): boolean {
+        if (this.empty()) {
+            return false;
+        }
+
+        return this.first().id <= msg_id && msg_id <= this.last()!.id;
+    }
+
     ids_greater_or_equal_than(my_id: number): number[] {
         const result = [];
 
@@ -284,13 +292,20 @@ export class MessageListData {
     }
 
     first_unread_message_id(): number | undefined {
+        // NOTE: This function returns the first unread that was fetched and is not
+        // necessarily the first unread for the narrow. There could be more unread messages.
+        // See https://github.com/zulip/zulip/pull/30008#discussion_r1597279862 for how
+        // ideas on how to possibly improve this.
+        // before `first_unread` calculated below that we haven't fetched yet.
         const first_unread = this._items.find((message) => unread.message_unread(message));
 
         if (first_unread) {
             return first_unread.id;
         }
 
-        // if no unread, return the bottom message
+        // If no unread, return the bottom message
+        // NOTE: This is only valid if we have found the latest message for the narrow as
+        // there could be more message that we haven't fetched yet.
         return this.last()?.id;
     }
 

web/src/message_lists.ts

@@ -58,6 +58,7 @@ export type MessageList = {
     last: () => Message | undefined;
     visibly_empty: () => boolean;
     selected_message: () => Message;
+    should_preserve_current_rendered_state: () => boolean;
 };
 
 export let current: MessageList | undefined;
@@ -70,12 +71,34 @@ export function set_current(msg_list: MessageList | undefined): void {
 }
 
 export function update_current_message_list(msg_list: MessageList | undefined): void {
-    if (current && !current.preserve_rendered_state) {
+    // Since we change `current` message list in the function, we need to decide if the
+    // current message list needs to be cached or discarded.
+    //
+    // If we are caching the current message list, we need to remove any other message lists
+    // that we have cached with the same filter.
+    //
+    // If we are discarding the current message list, we need to remove the
+    // current message list from the DOM.
+    if (current && !current.should_preserve_current_rendered_state()) {
         // Remove the current message list from the DOM.
         current.view.$list.remove();
         rendered_message_lists.delete(current.id);
     } else {
+        // We plan to keep the current message list cached.
         current?.view.$list.removeClass("focused-message-list");
+        // Remove any existing message lists that we have with the same filter.
+        // TODO: If we start supporting more messages lists than just Combined feed,
+        // make this a proper filter comparison between the lists.
+        if (current?.data.filter.is_in_home()) {
+            for (const [id, msg_list] of rendered_message_lists) {
+                if (id !== current.id && msg_list.data.filter.is_in_home()) {
+                    msg_list.view.$list.remove();
+                    rendered_message_lists.delete(id);
+                    // We only expect to have one instance of a message list filter cached.
+                    break;
+                }
+            }
+        }
     }
 
     current = msg_list;

web/src/narrow.js

@@ -102,9 +102,15 @@ function create_and_update_message_list(filter, id_info, opts) {
     // we need to add a `is_equal` function to `Filter` to compare the filters.
     let msg_list;
     let restore_rendered_list = false;
+    const is_combined_feed_global_view = filter.is_in_home();
     for (const list of message_lists.all_rendered_message_lists()) {
-        if (filter.is_in_home() && list.preserve_rendered_state) {
-            assert(list.data.filter.is_in_home());
+        if (is_combined_feed_global_view && list.data.filter.is_in_home()) {
+            if (opts.then_select_id > 0 && !list.msg_id_in_fetched_range(opts.then_select_id)) {
+                // We don't have the target message in the current rendered list.
+                // Read MessageList.should_preserve_current_rendered_state for details.
+                break;
+            }
+
             msg_list = list;
             restore_rendered_list = true;
             break;

web/src/unread.ts

@@ -26,6 +26,8 @@ import * as util from "./util";
 // for more details on how this system is designed.
 
 export let old_unreads_missing = false;
+// Note this doesn't handle the case of `old_unreads_missing` because
+// it is simpler and we as a client are not expected to.
 export let first_unread_unmuted_message_id = Number.POSITIVE_INFINITY;
 
 export function clear_old_unreads_missing(): void {