diff --git a/tools/test-js-with-node b/tools/test-js-with-node index cea9e57f1e..06ca238b27 100755 --- a/tools/test-js-with-node +++ b/tools/test-js-with-node @@ -137,6 +137,7 @@ EXEMPT_FILES = make_set( "web/src/message_edit.js", "web/src/message_edit_history.ts", "web/src/message_events.js", + "web/src/message_events_util.js", "web/src/message_feed_loading.ts", "web/src/message_feed_top_notices.ts", "web/src/message_fetch.js", diff --git a/web/src/echo.js b/web/src/echo.js index 7e1a17051c..a3a84a0abf 100644 --- a/web/src/echo.js +++ b/web/src/echo.js @@ -7,6 +7,7 @@ import * as compose_notifications from "./compose_notifications"; import * as compose_ui from "./compose_ui"; import * as local_message from "./local_message"; import * as markdown from "./markdown"; +import * as message_events_util from "./message_events_util"; import * as message_lists from "./message_lists"; import * as message_live_update from "./message_live_update"; import * as message_store from "./message_store"; @@ -377,7 +378,7 @@ export function update_message_lists({old_id, new_id}) { } export function process_from_server(messages) { - const msgs_to_rerender = []; + const msgs_to_rerender_or_add_to_narrow = []; const non_echo_messages = []; for (const message of messages) { @@ -424,17 +425,25 @@ export function process_from_server(messages) { client_message.is_me_message = message.is_me_message; client_message.submessages = message.submessages; - msgs_to_rerender.push(client_message); + msgs_to_rerender_or_add_to_narrow.push(client_message); waiting_for_ack.delete(local_id); } - if (msgs_to_rerender.length > 0) { - // In theory, we could just rerender messages where there were - // changes in either the rounded timestamp we display or the - // message content, but in practice, there's no harm to just - // doing it unconditionally. + if (msgs_to_rerender_or_add_to_narrow.length > 0) { for (const msg_list of message_lists.all_rendered_message_lists()) { - msg_list.view.rerender_messages(msgs_to_rerender); + if (!msg_list.data.filter.can_apply_locally()) { + message_events_util.maybe_add_narrowed_messages( + msgs_to_rerender_or_add_to_narrow, + msg_list, + message_util.add_new_messages, + ); + } else { + // In theory, we could just rerender messages where there were + // changes in either the rounded timestamp we display or the + // message content, but in practice, there's no harm to just + // doing it unconditionally. + msg_list.view.rerender_messages(msgs_to_rerender_or_add_to_narrow); + } } } diff --git a/web/src/message_events.js b/web/src/message_events.js index edb47a4ff0..0b532c7158 100644 --- a/web/src/message_events.js +++ b/web/src/message_events.js @@ -3,8 +3,6 @@ import assert from "minimalistic-assert"; import * as alert_words from "./alert_words"; import {all_messages_data} from "./all_messages_data"; -import * as blueslip from "./blueslip"; -import * as channel from "./channel"; import * as compose_fade from "./compose_fade"; import * as compose_notifications from "./compose_notifications"; import * as compose_state from "./compose_state"; @@ -13,6 +11,7 @@ import * as direct_message_group_data from "./direct_message_group_data"; import * as drafts from "./drafts"; import * as message_edit from "./message_edit"; import * as message_edit_history from "./message_edit_history"; +import * as message_events_util from "./message_events_util"; import * as message_helper from "./message_helper"; import * as message_lists from "./message_lists"; import * as message_notifications from "./message_notifications"; @@ -35,86 +34,6 @@ import * as unread_ops from "./unread_ops"; import * as unread_ui from "./unread_ui"; import * as util from "./util"; -function maybe_add_narrowed_messages(messages, msg_list, callback, attempt = 1) { - const ids = []; - - for (const elem of messages) { - ids.push(elem.id); - } - - channel.get({ - url: "/json/messages/matches_narrow", - data: { - msg_ids: JSON.stringify(ids), - narrow: JSON.stringify(narrow_state.public_search_terms()), - }, - timeout: 5000, - success(data) { - if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) { - // We unnarrowed or moved to Recent Conversations in the meantime. - return; - } - - let new_messages = []; - const elsewhere_messages = []; - - for (const elem of messages) { - if (Object.hasOwn(data.messages, elem.id)) { - util.set_match_data(elem, data.messages[elem.id]); - new_messages.push(elem); - } else { - elsewhere_messages.push(elem); - } - } - - // This second call to process_new_message in the - // insert_new_messages code path is designed to replace - // our slightly stale message object with the latest copy - // from the message_store. This helps in very rare race - // conditions, where e.g. the current user's name was - // edited in between when they sent the message and when - // we hear back from the server and can echo the new - // message. - new_messages = new_messages.map((message) => - message_helper.process_new_message(message), - ); - - callback(new_messages, msg_list); - unread_ops.process_visible(); - compose_notifications.notify_messages_outside_current_search(elsewhere_messages); - }, - error(xhr) { - if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) { - return; - } - if (xhr.status === 400) { - // This narrow was invalid -- don't retry it, and don't display the message. - return; - } - if (attempt >= 5) { - // Too many retries -- bail out. However, this means the `messages` are potentially - // missing from the search results view. Since this is a very unlikely circumstance - // (Tornado is up, Django is down for 5 retries, user is in a search view that it - // cannot apply itself) and the failure mode is not bad (it will simply fail to - // include live updates of new matching messages), just log an error. - blueslip.error( - "Failed to determine if new message matches current narrow, after 5 tries", - ); - return; - } - // Backoff on retries, with full jitter: up to 2s, 4s, 8s, 16s, 32s - const delay = Math.random() * 2 ** attempt * 2000; - setTimeout(() => { - if (msg_list === message_lists.current) { - // Don't actually try again if we un-narrowed - // while waiting - maybe_add_narrowed_messages(messages, msg_list, callback, attempt + 1); - } - }, delay); - }, - }); -} - export function insert_new_messages(messages, sent_by_this_client) { messages = messages.map((message) => message_helper.process_new_message(message)); @@ -133,7 +52,11 @@ export function insert_new_messages(messages, sent_by_this_client) { // new messages match the narrow, and use that to // determine which new messages to add to the current // message list (or display a notification). - maybe_add_narrowed_messages(messages, list, message_util.add_new_messages); + message_events_util.maybe_add_narrowed_messages( + messages, + list, + message_util.add_new_messages, + ); continue; } @@ -504,7 +427,7 @@ export function update_messages(events) { updated_messages.map((msg) => msg.id), ); // For filters that cannot be processed locally, ask server. - maybe_add_narrowed_messages( + message_events_util.maybe_add_narrowed_messages( event_messages, message_lists.current, message_util.add_messages, diff --git a/web/src/message_events_util.js b/web/src/message_events_util.js new file mode 100644 index 0000000000..c9ec203714 --- /dev/null +++ b/web/src/message_events_util.js @@ -0,0 +1,89 @@ +import * as blueslip from "./blueslip"; +import * as channel from "./channel"; +import * as compose_notifications from "./compose_notifications"; +import * as message_helper from "./message_helper"; +import * as message_lists from "./message_lists"; +import * as narrow_state from "./narrow_state"; +import * as unread_ops from "./unread_ops"; +import * as util from "./util"; + +// TODO: Move this function to 'message_util.ts' once #30702 is merged. +export function maybe_add_narrowed_messages(messages, msg_list, callback, attempt = 1) { + const ids = []; + + for (const elem of messages) { + ids.push(elem.id); + } + + channel.get({ + url: "/json/messages/matches_narrow", + data: { + msg_ids: JSON.stringify(ids), + narrow: JSON.stringify(narrow_state.public_search_terms()), + }, + timeout: 5000, + success(data) { + if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) { + // We unnarrowed or moved to Recent Conversations in the meantime. + return; + } + + let new_messages = []; + const elsewhere_messages = []; + + for (const elem of messages) { + if (Object.hasOwn(data.messages, elem.id)) { + util.set_match_data(elem, data.messages[elem.id]); + new_messages.push(elem); + } else { + elsewhere_messages.push(elem); + } + } + + // This second call to process_new_message in the + // insert_new_messages code path is designed to replace + // our slightly stale message object with the latest copy + // from the message_store. This helps in very rare race + // conditions, where e.g. the current user's name was + // edited in between when they sent the message and when + // we hear back from the server and can echo the new + // message. + new_messages = new_messages.map((message) => + message_helper.process_new_message(message), + ); + + callback(new_messages, msg_list); + unread_ops.process_visible(); + compose_notifications.notify_messages_outside_current_search(elsewhere_messages); + }, + error(xhr) { + if (!narrow_state.is_message_feed_visible() || msg_list !== message_lists.current) { + return; + } + if (xhr.status === 400) { + // This narrow was invalid -- don't retry it, and don't display the message. + return; + } + if (attempt >= 5) { + // Too many retries -- bail out. However, this means the `messages` are potentially + // missing from the search results view. Since this is a very unlikely circumstance + // (Tornado is up, Django is down for 5 retries, user is in a search view that it + // cannot apply itself) and the failure mode is not bad (it will simply fail to + // include live updates of new matching messages), just log an error. + blueslip.error( + "Failed to determine if new message matches current narrow, after 5 tries", + ); + return; + } + // Backoff on retries, with full jitter: up to 2s, 4s, 8s, 16s, 32s + const delay = Math.random() * 2 ** attempt * 2000; + setTimeout(() => { + if (msg_list === message_lists.current) { + // Don't actually try again if we un-narrowed + // while waiting + maybe_add_narrowed_messages(messages, msg_list, callback, attempt + 1); + } + }, delay); + }, + }); +} diff --git a/web/tests/echo.test.js b/web/tests/echo.test.js index c5c3a6b18b..55c8e15d86 100644 --- a/web/tests/echo.test.js +++ b/web/tests/echo.test.js @@ -12,6 +12,7 @@ const {current_user} = require("./lib/zpage_params"); const compose_notifications = mock_esm("../src/compose_notifications"); const markdown = mock_esm("../src/markdown"); const message_lists = mock_esm("../src/message_lists"); +const message_events_util = mock_esm("../src/message_events_util"); let disparities = []; @@ -39,6 +40,13 @@ message_lists.current = { rerender_messages: noop, change_message_id: noop, }, + data: { + filter: { + can_apply_locally() { + return true; + }, + }, + }, change_message_id: noop, }; const home_msg_list = { @@ -46,6 +54,13 @@ const home_msg_list = { rerender_messages: noop, change_message_id: noop, }, + data: { + filter: { + can_apply_locally() { + return true; + }, + }, + }, preserver_rendered_state: true, change_message_id: noop, }; @@ -123,6 +138,53 @@ run_test("process_from_server for differently rendered messages", ({override}) = ]); }); +run_test("process_from_server for messages to add to narrow", ({override}) => { + let messages_to_add_to_narrow = []; + + override(message_lists.current.data.filter, "can_apply_locally", () => false); + override(message_events_util, "maybe_add_narrowed_messages", (msgs, msg_list) => { + messages_to_add_to_narrow = msgs; + assert.equal(msg_list, message_lists.current); + }); + + const old_value = "old_value"; + const new_value = "new_value"; + const waiting_for_ack = new Map([ + [ + "100.1", + { + content: "
rendered message
", + timestamp: old_value, + is_me_message: old_value, + submessages: old_value, + topic_links: old_value, + }, + ], + ]); + const server_messages = [ + { + local_id: "100.1", + content: "rendered message
", + timestamp: new_value, + is_me_message: new_value, + submessages: new_value, + topic_links: new_value, + }, + ]; + echo._patch_waiting_for_ack(waiting_for_ack); + const non_echo_messages = echo.process_from_server(server_messages); + assert.deepEqual(non_echo_messages, []); + assert.deepEqual(messages_to_add_to_narrow, [ + { + content: server_messages[0].content, + timestamp: new_value, + is_me_message: new_value, + submessages: new_value, + topic_links: new_value, + }, + ]); +}); + run_test("build_display_recipient", () => { current_user.user_id = 123;