diff --git a/web/src/message_events.ts b/web/src/message_events.ts index 2eca809448..8173b2d1d8 100644 --- 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); } } diff --git a/web/src/message_fetch.ts b/web/src/message_fetch.ts index 4d5bbd88ae..5a92a5e85b 100644 --- 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); } } diff --git a/web/src/message_list.ts b/web/src/message_list.ts index 551790253a..dcd7e3e42b 100644 --- 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; diff --git a/web/src/message_list_data.ts b/web/src/message_list_data.ts index 53c7c0bcaa..789745a6a6 100644 --- 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); } diff --git a/web/src/message_util.ts b/web/src/message_util.ts index 99608de45a..5fbc59024c 100644 --- 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, diff --git a/web/src/message_view.ts b/web/src/message_view.ts index 293c1cb0e3..f749d6bc8d 100644 --- 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(); } diff --git a/web/tests/compose_closed_ui.test.cjs b/web/tests/compose_closed_ui.test.cjs index 7e88fe63e4..f0d3342efe 100644 --- 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", diff --git a/web/tests/example5.test.cjs b/web/tests/example5.test.cjs index b3bd1df8f1..8601eb3267 100644 --- 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 diff --git a/web/tests/message_list.test.cjs b/web/tests/message_list.test.cjs index 8860a3ebf3..b3c43f3f74 100644 --- 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); { diff --git a/web/tests/message_list_data.test.cjs b/web/tests/message_list_data.test.cjs index c73008362c..af86a4e606 100644 --- 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]); diff --git a/web/tests/message_list_view.test.cjs b/web/tests/message_list_view.test.cjs index 41332e9fe2..339078fab7 100644 --- 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) { diff --git a/web/tests/stream_topic_history.test.cjs b/web/tests/stream_topic_history.test.cjs index b23a80a1dc..a3065aed61 100644 --- 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;