From e530af50136f938ccec92333db11f9bb8b0a4459 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Tue, 21 May 2024 03:57:30 +0000 Subject: [PATCH] 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 | 52 +++++++++++++++++++++++++++++++++--- web/src/message_list_data.ts | 17 +++++++++++- web/src/message_lists.ts | 25 ++++++++++++++++- web/src/narrow.js | 10 +++++-- web/src/unread.ts | 2 ++ 5 files changed, 98 insertions(+), 8 deletions(-) diff --git a/web/src/message_list.js b/web/src/message_list.js index 0c8210faeb..4618b51b27 100644 --- a/web/src/message_list.js +++ b/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,7 +69,11 @@ 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. @@ -76,10 +81,45 @@ export class MessageList { // 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(); } diff --git a/web/src/message_list_data.ts b/web/src/message_list_data.ts index f3421c2ecc..8d1324eef5 100644 --- a/web/src/message_list_data.ts +++ b/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; } diff --git a/web/src/message_lists.ts b/web/src/message_lists.ts index cf411f3c60..792e65a5e8 100644 --- a/web/src/message_lists.ts +++ b/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; diff --git a/web/src/narrow.js b/web/src/narrow.js index 69d96a112c..861f5e156e 100644 --- a/web/src/narrow.js +++ b/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; diff --git a/web/src/unread.ts b/web/src/unread.ts index dfb0cd479c..338888f75c 100644 --- a/web/src/unread.ts +++ b/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 {