From 9729b1a4ad51b69c98ce4f8374c9d9f8cf69430c Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Sat, 28 Apr 2018 14:32:37 -0700 Subject: [PATCH] search: Remove buggy double-call of set_message_booleans. In a refactor last fall, we changed `set_message_booleans` to mutate state (specifically, destroying msg.flags in favor of setting properties like `msg.unread`). This was fine for most code paths, but the maybe_add_narrowed_messages code path called `message_store.add_message_metadata` twice (once after talking to the server to find out whether the messages go into the current narrow), and so when we extracted set_message_booleans from that, the second call didn't properly short-circuit. We fix this by just removing the second call, and also add a comment warning about the add_message_metadata call there as being dangerous. Fixes #8184. --- static/js/message_events.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/static/js/message_events.js b/static/js/message_events.js index a6602de0e2..20777e44d0 100644 --- a/static/js/message_events.js +++ b/static/js/message_events.js @@ -31,8 +31,14 @@ function maybe_add_narrowed_messages(messages, msg_list, messages_are_new) { } }); - _.each(new_messages, message_store.set_message_booleans); + // This second call to add_message_metadata in the + // insert_new_messages code path 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. Arguably, it's counterproductive complexity. new_messages = _.map(new_messages, message_store.add_message_metadata); + message_util.add_messages( new_messages, msg_list,