message_list: Fix message list missing messages.

Since we allow calling `add_messages` without checking fetch status, it can lead to non-contiguous message history due to latest message being added to a message list without previous messages being fetched. To fix it, we only allow adding new messages via message_fetch which properly sets `anchor` to the last message in the list before fetching and adding messages to the list.
2025-11-13 10:26:28 +00:00 · 2024-12-18 22:13:10 +05:30
parent 9e519f8c18
commit 525ae3aaff
12 changed files with 67 additions and 79 deletions
--- a/web/src/message_events.ts
+++ b/web/src/message_events.ts
@@ -322,7 +322,7 @@ export function insert_new_messages(
        }

        // Update the message list's rendering for the newly arrived messages.
-        const render_info = message_util.add_new_messages(messages, list);
+        const render_info = list.add_messages(messages);

        // The render_info.need_user_to_scroll calculation, which
        // looks at message feed scroll positions to see whether the
@@ -341,7 +341,7 @@ export function insert_new_messages(
            // but it is not worth doing so for every new message.
            message_list_data_cache.remove(msg_list_data.filter);
        } else {
-            message_util.add_new_messages_data(messages, msg_list_data);
+            msg_list_data.add_messages(messages);
        }
    }

--- a/web/src/message_fetch.ts
+++ b/web/src/message_fetch.ts
@@ -153,6 +153,7 @@ function process_result(data: MessageFetchResponse, opts: MessageFetchOptions):
    // messages not tracked in unread.ts during this fetching process.
    message_util.do_unread_count_updates(messages, true);

+    const ignore_found_newest = true;
    if (messages.length > 0) {
        if (opts.msg_list) {
            if (opts.validate_filter_topic_post_fetch) {
@@ -160,9 +161,9 @@ function process_result(data: MessageFetchResponse, opts: MessageFetchOptions):
            }
            // Since this adds messages to the MessageList and renders MessageListView,
            // we don't need to call it if msg_list was not defined by the caller.
-            message_util.add_old_messages(messages, opts.msg_list);
+            opts.msg_list.add_messages(messages, {}, ignore_found_newest);
        } else {
-            opts.msg_list_data.add_messages(messages);
+            opts.msg_list_data.add_messages(messages, ignore_found_newest);
        }
    }

--- a/web/src/message_list.ts
+++ b/web/src/message_list.ts
@@ -193,10 +193,11 @@ export class MessageList {
    add_messages(
        messages: Message[],
        append_to_view_opts: {messages_are_new?: boolean} = {},
+        ignore_found_newest = false,
    ): RenderInfo | undefined {
        // This adds all messages to our data, but only returns
        // the currently viewable ones.
-        const info = this.data.add_messages(messages);
+        const info = this.data.add_messages(messages, ignore_found_newest);

        const top_messages = info.top_messages;
        const bottom_messages = info.bottom_messages;
--- a/web/src/message_list_data.ts
+++ b/web/src/message_list_data.ts
@@ -295,7 +295,10 @@ export class MessageListData {
        return this._items.some((message) => message.unread);
    }

-    add_messages(messages: Message[]): {
+    add_messages(
+        messages: Message[],
+        ignore_found_newest = false,
+    ): {
        top_messages: Message[];
        bottom_messages: Message[];
        interior_messages: Message[];
@@ -332,6 +335,13 @@ export class MessageListData {
            top_messages = this.prepend(top_messages);
        }

+        if (!ignore_found_newest && !this.fetch_status.has_found_newest()) {
+            // Don't add messages at the bottom if we haven't found
+            // the latest message to avoid non-contiguous message history.
+            this.fetch_status.update_expected_max_message_id(bottom_messages);
+            bottom_messages = [];
+        }
+
        if (bottom_messages.length > 0) {
            bottom_messages = this.append(bottom_messages);
        }
--- a/web/src/message_util.ts
+++ b/web/src/message_util.ts
@@ -2,7 +2,6 @@ import assert from "minimalistic-assert";

 import {all_messages_data} from "./all_messages_data.ts";
 import type {MessageList, RenderInfo} from "./message_list.ts";
-import type {MessageListData} from "./message_list_data.ts";
 import * as message_lists from "./message_lists.ts";
 import * as message_store from "./message_store.ts";
 import type {Message} from "./message_store.ts";
@@ -40,13 +39,6 @@ export function add_messages(
    return render_info;
 }

-export function add_old_messages(
-    messages: Message[],
-    msg_list: MessageList,
-): RenderInfo | undefined {
-    return add_messages(messages, msg_list, {messages_are_new: false});
-}
-
 export function add_new_messages(
    messages: Message[],
    msg_list: MessageList,
@@ -61,29 +53,6 @@ export function add_new_messages(
    }
    return add_messages(messages, msg_list, {messages_are_new: true});
 }
-
-export function add_new_messages_data(
-    messages: Message[],
-    msg_list_data: MessageListData,
-):
-    | {
-          top_messages: Message[];
-          bottom_messages: Message[];
-          interior_messages: Message[];
-      }
-    | undefined {
-    if (!msg_list_data.fetch_status.has_found_newest()) {
-        const filtered_msgs = msg_list_data.valid_non_duplicated_messages(messages);
-        // The reasoning in add_new_messages applies here as well;
-        // we're trying to maintain a data structure that's a
-        // contiguous range of message history, so we can't append a
-        // new message that might not be adjacent to that range.
-        msg_list_data.fetch_status.update_expected_max_message_id(filtered_msgs);
-        return undefined;
-    }
-    return msg_list_data.add_messages(messages);
-}
-
 export function get_count_of_messages_in_topic_sent_after_current_message(
    stream_id: number,
    topic: string,
--- a/web/src/message_view.ts
+++ b/web/src/message_view.ts
@@ -912,7 +912,8 @@ function load_local_messages(msg_data: MessageListData, superset_data: MessageLi
    // one message the user will expect to see in the new narrow.

    const in_msgs = superset_data.all_messages();
-    msg_data.add_messages(in_msgs);
+    const ignore_found_newest = true;
+    msg_data.add_messages(in_msgs, ignore_found_newest);

    return !msg_data.visibly_empty();
 }
--- a/web/tests/compose_closed_ui.test.cjs
+++ b/web/tests/compose_closed_ui.test.cjs
@@ -66,36 +66,40 @@ run_test("reply_label", () => {
        stream_id: 2,
    };
    stream_data.add_sub(stream_two);
-    list.add_messages([
-        {
-            id: 0,
-            stream_id: stream_one.stream_id,
-            topic: "first_topic",
-        },
-        {
-            id: 1,
-            stream_id: stream_one.stream_id,
-            topic: "second_topic",
-        },
-        {
-            id: 2,
-            stream_id: stream_two.stream_id,
-            topic: "third_topic",
-        },
-        {
-            id: 3,
-            stream_id: stream_two.stream_id,
-            topic: "second_topic",
-        },
-        {
-            id: 4,
-            display_reply_to: "some user",
-        },
-        {
-            id: 5,
-            display_reply_to: "some user, other user",
-        },
-    ]);
+    list.add_messages(
+        [
+            {
+                id: 0,
+                stream_id: stream_one.stream_id,
+                topic: "first_topic",
+            },
+            {
+                id: 1,
+                stream_id: stream_one.stream_id,
+                topic: "second_topic",
+            },
+            {
+                id: 2,
+                stream_id: stream_two.stream_id,
+                topic: "third_topic",
+            },
+            {
+                id: 3,
+                stream_id: stream_two.stream_id,
+                topic: "second_topic",
+            },
+            {
+                id: 4,
+                display_reply_to: "some user",
+            },
+            {
+                id: 5,
+                display_reply_to: "some user, other user",
+            },
+        ],
+        {},
+        true,
+    );

    const expected_labels = [
        "#first_stream > first_topic",
--- a/web/tests/example5.test.cjs
+++ b/web/tests/example5.test.cjs
@@ -24,12 +24,12 @@ const {run_test, noop} = require("./lib/test.cjs");
 const direct_message_group_data = mock_esm("../src/direct_message_group_data");
 const message_lists = mock_esm("../src/message_lists");
 const message_notifications = mock_esm("../src/message_notifications");
-const message_util = mock_esm("../src/message_util");
 const pm_list = mock_esm("../src/pm_list");
 const stream_list = mock_esm("../src/stream_list");
 const unread_ui = mock_esm("../src/unread_ui");
 const activity = mock_esm("../src/activity");

+let added_message = false;
 message_lists.current = {
    data: {
        filter: {
@@ -38,6 +38,9 @@ message_lists.current = {
            },
        },
    },
+    add_messages() {
+        added_message = true;
+    },
 };
 message_lists.all_rendered_message_lists = () => [message_lists.current];
 message_lists.non_rendered_data = () => [];
@@ -103,7 +106,6 @@ run_test("insert_message", ({override}) => {

    helper.redirect(direct_message_group_data, "process_loaded_messages");
    helper.redirect(message_notifications, "received_messages");
-    helper.redirect(message_util, "add_new_messages");
    helper.redirect(stream_list, "update_streams_sidebar");
    helper.redirect(unread_ui, "update_unread_counts");
    helper.redirect(activity, "set_received_new_messages");
@@ -116,12 +118,12 @@ run_test("insert_message", ({override}) => {
    // comes in:
    assert.deepEqual(helper.events, [
        [direct_message_group_data, "process_loaded_messages"],
-        [message_util, "add_new_messages"],
        [unread_ui, "update_unread_counts"],
        [activity, "set_received_new_messages"],
        [message_notifications, "received_messages"],
        [stream_list, "update_streams_sidebar"],
    ]);
+    assert.ok(added_message);

    // Despite all of our stubbing/mocking, the call to
    // insert_new_messages will have created a very important
--- a/web/tests/message_list.test.cjs
+++ b/web/tests/message_list.test.cjs
@@ -463,7 +463,7 @@ run_test("add_remove_rerender", ({override}) => {

    const messages = [{id: 1}, {id: 2}, {id: 3}];

-    list.add_messages(messages);
+    list.add_messages(messages, {}, true);
    assert.equal(list.num_items(), 3);

    {
--- a/web/tests/message_list_data.test.cjs
+++ b/web/tests/message_list_data.test.cjs
@@ -53,7 +53,7 @@ run_test("basics", () => {
    assert_contents(mld, [15, 25, 35, 45]);

    const new_msgs = make_msgs([10, 20, 30, 40, 50, 60, 70]);
-    const info = mld.add_messages(new_msgs);
+    const info = mld.add_messages(new_msgs, true);

    assert.deepEqual(info, {
        top_messages: make_msgs([10]),
@@ -103,7 +103,7 @@ run_test("basics", () => {
        {id: 9, sender_id: 11, type: "private", to_user_ids: "9"},
        ...msgs_sent_by_6,
    ];
-    mld.add_messages(msgs_with_sender_ids);
+    mld.add_messages(msgs_with_sender_ids, true);
    assert.deepEqual(mld.get_messages_sent_by_user(6), msgs_sent_by_6);

    mld.clear();
@@ -111,7 +111,7 @@ run_test("basics", () => {
    assert.equal(mld.closest_id(99), -1);
    assert.equal(mld.get_last_message_sent_by_me(), undefined);

-    mld.add_messages(make_msgs([120, 125.01, 130, 140]));
+    mld.add_messages(make_msgs([120, 125.01, 130, 140]), true);
    assert_contents(mld, [120, 125.01, 130, 140]);
    mld.set_selected_id(125.01);
    assert.equal(mld.selected_id(), 125.01);
@@ -270,7 +270,7 @@ run_test("muting", () => {
        {id: 8, type: "stream", stream_id: 1, topic: "whatever"},
    ];

-    const orig_info = mld.add_messages(orig_messages);
+    const orig_info = mld.add_messages(orig_messages, true);
    assert.deepEqual(orig_info, {
        top_messages: [],
        interior_messages: [],
@@ -293,7 +293,7 @@ run_test("muting", () => {
        {id: 10, type: "stream", stream_id: 1, topic: "whatever"},
    ];

-    const more_info = mld.add_messages(more_messages);
+    const more_info = mld.add_messages(more_messages, true);

    assert_msg_ids(mld._all_items, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
    assert_msg_ids(mld._items, [2, 4, 6, 8, 10]);
--- a/web/tests/message_list_view.test.cjs
+++ b/web/tests/message_list_view.test.cjs
@@ -778,7 +778,7 @@ test("render_windows", ({mock_template}) => {
        list.view.clear_table = noop;
        list.clear();

-        list.add_messages(messages, {});
+        list.add_messages(messages, {}, true);
    }

    function verify_no_move_range(start, end) {
--- a/web/tests/stream_topic_history.test.cjs
+++ b/web/tests/stream_topic_history.test.cjs
@@ -389,7 +389,7 @@ test("all_topics_in_cache", ({override}) => {
    assert.equal(stream_topic_history.all_topics_in_cache(sub), false);

    all_messages_data.all_messages_data.clear();
-    all_messages_data.all_messages_data.add_messages(messages);
+    all_messages_data.all_messages_data.add_messages(messages, true);

    let has_found_newest = false;