diff --git a/frontend_tests/node_tests/message_fetch.js b/frontend_tests/node_tests/message_fetch.js index fa173dc329..967d0fe8bf 100644 --- a/frontend_tests/node_tests/message_fetch.js +++ b/frontend_tests/node_tests/message_fetch.js @@ -7,6 +7,7 @@ const _ = require("lodash"); const {mock_esm, set_global, zrequire} = require("../zjsunit/namespace"); const {run_test} = require("../zjsunit/test"); const $ = require("../zjsunit/zjquery"); +const {page_params} = require("../zjsunit/zpage_params"); set_global("document", "document-stub"); @@ -267,6 +268,9 @@ run_test("initialize", () => { reset_lists(); let home_loaded = false; + page_params.unread_msgs = { + old_unreads_missing: false, + }; function home_view_loaded() { home_loaded = true; @@ -351,6 +355,9 @@ run_test("loading_newer", () => { (function test_narrow() { const msg_list = simulate_narrow(); + page_params.unread_msgs = { + old_unreads_missing: true, + }; const data = { req: { diff --git a/frontend_tests/node_tests/unread.js b/frontend_tests/node_tests/unread.js index 7e855fc558..a119ff638c 100644 --- a/frontend_tests/node_tests/unread.js +++ b/frontend_tests/node_tests/unread.js @@ -514,7 +514,7 @@ test("mentions", () => { }; const muted_direct_mention_message = { - id: 17, + id: 18, type: "stream", stream_id: muted_stream_id, topic: "lunch", @@ -542,6 +542,7 @@ test("mentions", () => { mention_me_message.id, mention_all_message.id, muted_mention_all_message.id, + muted_direct_mention_message.id, ]); test_notifiable_count(counts.home_unread_messages, 3); diff --git a/static/js/message_events.js b/static/js/message_events.js index a6c4fc785c..5083b8896c 100644 --- a/static/js/message_events.js +++ b/static/js/message_events.js @@ -116,7 +116,7 @@ function maybe_add_narrowed_messages(messages, msg_list, callback, attempt = 1) export function insert_new_messages(messages, sent_by_this_client) { messages = messages.map((message) => message_helper.process_new_message(message)); - unread.process_loaded_messages(messages); + const any_untracked_unread_messages = unread.process_loaded_messages(messages, false); huddle_data.process_loaded_messages(messages); // all_messages_data is the data that we use to populate @@ -153,7 +153,9 @@ export function insert_new_messages(messages, sent_by_this_client) { notifications.notify_local_mixes(messages, need_user_to_scroll); } - unread_ui.update_unread_counts(); + if (any_untracked_unread_messages) { + unread_ui.update_unread_counts(); + } resize.resize_page_components(); unread_ops.process_visible(); diff --git a/static/js/message_fetch.js b/static/js/message_fetch.js index 8cee10b510..252763b780 100644 --- a/static/js/message_fetch.js +++ b/static/js/message_fetch.js @@ -49,16 +49,9 @@ function process_result(data, opts) { messages = messages.map((message) => message_helper.process_new_message(message)); - // In case any of the newly fetched messages are new, add them to - // our unread data structures. It's important that this run even - // when fetching in a narrow, since we might return unread - // messages that aren't in the home view data set (e.g. on a muted - // stream). - // - // BUG: This code path calls pm_list.update_private_messages, even - // if there were no private messages (or even no new messages at - // all) in data.messages, which is a waste of resources. - message_util.do_unread_count_updates(messages); + // In some rare situations, we expect to discover new unread + // messages not tracked in unread.js during this fetching process. + message_util.do_unread_count_updates(messages, true); // If we're loading more messages into the home view, save them to // the all_messages_data as well, as the message_lists.home is diff --git a/static/js/message_util.js b/static/js/message_util.js index 56e085b60a..3545ac952b 100644 --- a/static/js/message_util.js +++ b/static/js/message_util.js @@ -7,10 +7,15 @@ import * as resize from "./resize"; import * as unread from "./unread"; import * as unread_ui from "./unread_ui"; -export function do_unread_count_updates(messages) { - unread.process_loaded_messages(messages); - unread_ui.update_unread_counts(); - resize.resize_page_components(); +export function do_unread_count_updates(messages, expect_no_new_unreads = false) { + const any_new_unreads = unread.process_loaded_messages(messages, expect_no_new_unreads); + + if (any_new_unreads) { + // The following operations are expensive, and thus should + // only happen if we found any unread messages justifying it. + unread_ui.update_unread_counts(); + resize.resize_page_components(); + } } export function add_messages(messages, msg_list, opts) { diff --git a/static/js/unread.js b/static/js/unread.js index c8c5ae5659..2b43f2b465 100644 --- a/static/js/unread.js +++ b/static/js/unread.js @@ -1,3 +1,4 @@ +import * as blueslip from "./blueslip"; import {FoldDict} from "./fold_dict"; import * as message_store from "./message_store"; import {page_params} from "./page_params"; @@ -537,9 +538,34 @@ export function update_unread_topics(msg, event) { }); } -export function process_loaded_messages(messages) { +export function process_loaded_messages(messages, expect_no_new_unreads = false) { + // Process a set of messages that we have full copies of from the + // server for whether any are unread but not tracked as such by + // our data structures. This can occur due to old_unreads_missing, + // changes in muting configuration, innocent races, or potentially bugs. + // + // Returns whether there were any new unread messages; in that + // case, the caller will need to trigger a rerender of UI + // displaying unread counts. + + let any_untracked_unread_messages = false; for (const message of messages) { if (message.unread) { + if (unread_messages.has(message.id)) { + // If we're already tracking this message as unread, there's nothing to do. + continue; + } + + if (expect_no_new_unreads && !page_params.unread_msgs.old_unreads_missing) { + // This may happen due to races, where someone narrows + // to a view and the message_fetch request returns + // before server_events system delivers the message to + // the client. + // + // For now, log it as a blueslip error so we can learn its prevalence. + blueslip.error("New unread discovered in process_loaded_messages."); + } + const user_ids_string = message.type === "private" ? people.pm_reply_user_string(message) : undefined; @@ -553,8 +579,11 @@ export function process_loaded_messages(messages) { unread: true, user_ids_string, }); + any_untracked_unread_messages = true; } } + + return any_untracked_unread_messages; } export function process_unread_message(message) {