message_util: Avoid unnecessary unreads work processing new messages.

It should be very rare to discover new unread messages during a
message_fetch call. This can potentially happen due to races (fetching
just as a new message arrives), but it shouldn't be the common case.

Previously, we would trigger a full rerender of all UI displaying
unread messages every time a bulk message fetch operation returned
(including every time one narrowed), regardless of whether any actual
state had changed.

Fix this by actually checking if we discovered any new unread messages.

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: {

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);
 

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();

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
+    // In some rare situations, we expect to discover new unread
-    // our unread data structures.  It's important that this run even
+    // messages not tracked in unread.js during this fetching process.
-    // when fetching in a narrow, since we might return unread
+    message_util.do_unread_count_updates(messages, true);
-    // 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);
 
     // 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

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) {
+export function do_unread_count_updates(messages, expect_no_new_unreads = false) {
-    unread.process_loaded_messages(messages);
+    const any_new_unreads = unread.process_loaded_messages(messages, expect_no_new_unreads);
-    unread_ui.update_unread_counts();
+
-    resize.resize_page_components();
+    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) {

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) {