messagesReducer: Address a performance TODO for EVENT_NEW_MESSAGE.

src/chat/narrowsReducer.js

@@ -72,8 +72,16 @@ const eventNewMessage = (state, action) => {
       if (!action.caughtUp[key]?.newer) {
         // Don't add a message to the end of the list unless we know
         // it's the most recent message, i.e., unless we know we're
-        // currently looking at (caught up with) the newest messages in
-        // the narrow.
+        // currently looking at (caught up with) the newest messages
+        // in the narrow. We don't want to accidentally show a message
+        // at the end of a message list if there might be messages
+        // between the currently latest-shown message and this
+        // message.
+        //
+        // See a corresponding condition in messagesReducer, where we
+        // don't bother to add to `state.messages` if this condition
+        // (after running on all of `narrowsForMessage`) means the new
+        // message wasn't added anywhere in `state.narrows`.
         return; // i.e., continue
       }
 
@@ -85,6 +93,12 @@ const eventNewMessage = (state, action) => {
         return; // i.e., continue
       }
 
+      // If changing or removing a case where we ignore a message
+      // here: Careful! Every message in `state.narrows` must exist in
+      // `state.messages`. If we choose to include a message in
+      // `state.narrows`, then messagesReducer MUST ALSO choose to
+      // include it in `state.messages`.
+
       stateMutable.set(key, [...value, message.id]);
     });
   });

src/message/__tests__/messagesReducer-test.js

@@ -13,23 +13,53 @@ import {
   EVENT_REACTION_REMOVE,
 } from '../../actionConstants';
 import * as eg from '../../__tests__/lib/exampleData';
-import { ALL_PRIVATE_NARROW, HOME_NARROW } from '../../utils/narrow';
+import { ALL_PRIVATE_NARROW, HOME_NARROW, HOME_NARROW_STR } from '../../utils/narrow';
 import { makeUserId } from '../../api/idTypes';
 
 describe('messagesReducer', () => {
   describe('EVENT_NEW_MESSAGE', () => {
-    test('appends message to state producing a copy of messages', () => {
+    test('appends message to state, if any narrow is caught up to newest', () => {
       const message1 = eg.streamMessage({ id: 1 });
       const message2 = eg.streamMessage({ id: 2 });
       const message3 = eg.streamMessage({ id: 3 });
 
       const prevState = eg.makeMessagesState([message1, message2]);
-      const action = deepFreeze({ ...eg.eventNewMessageActionBase, message: message3 });
+      const action = deepFreeze({
+        ...eg.eventNewMessageActionBase,
+        caughtUp: {
+          [HOME_NARROW_STR]: {
+            older: true,
+            newer: true,
+          },
+        },
+        // `flags` is present in EVENT_NEW_MESSAGE; see note at Message.
+        message: { ...message3, flags: [] },
+      });
       const expectedState = eg.makeMessagesState([message1, message2, message3]);
       const newState = messagesReducer(prevState, action);
       expect(newState).toEqual(expectedState);
       expect(newState).not.toBe(prevState);
     });
+
+    test('does nothing, if no narrow is caught up to newest', () => {
+      const message1 = eg.streamMessage({ id: 1 });
+      const message2 = eg.streamMessage({ id: 2 });
+      const message3 = eg.streamMessage({ id: 3 });
+      const prevState = eg.makeMessagesState([message1, message2]);
+      const action = deepFreeze({
+        ...eg.eventNewMessageActionBase,
+        caughtUp: {
+          [HOME_NARROW_STR]: {
+            older: true,
+            newer: false,
+          },
+        },
+        // `flags` is present in EVENT_NEW_MESSAGE; see note at Message.
+        message: { ...message3, flags: [] },
+      });
+      const newState = messagesReducer(prevState, action);
+      expect(newState).toEqual(prevState);
+    });
   });
 
   describe('EVENT_SUBMESSAGE', () => {

src/message/messagesReducer.js

@@ -17,15 +17,48 @@ import {
   EVENT_UPDATE_MESSAGE,
 } from '../actionConstants';
 import { NULL_ARRAY } from '../nullObjects';
+import { getNarrowsForMessage, keyFromNarrow } from '../utils/narrow';
 
 const initialState: MessagesState = Immutable.Map([]);
 
 const eventNewMessage = (state, action) => {
-  // TODO: Optimize -- Only update if the new message belongs to at least
-  // one narrow that is caught up.
+  const { message, caughtUp } = action;
+  const { flags } = message;
+
+  if (!flags) {
+    throw new Error('EVENT_NEW_MESSAGE message missing flags');
+  }
+
+  // Don't add a message that's already been added. It's probably
+  // very rare for a message to have already been added when we
+  // get an EVENT_NEW_MESSAGE, and perhaps impossible. (TODO:
+  // investigate?)
   if (state.get(action.message.id)) {
     return state;
   }
+
+  const narrowsForMessage = getNarrowsForMessage(message, action.ownUserId, flags);
+  const anyNarrowIsCaughtUp = narrowsForMessage.some(narrow => {
+    const key = keyFromNarrow(narrow);
+    // (No guarantee that `key` is in `action.caughtUp`)
+    // flowlint-next-line unnecessary-optional-chain:off
+    return caughtUp[key]?.newer;
+  });
+
+  // Don't bother adding the message to `state.messages` if it wasn't
+  // added to `state.narrows`. For why the message might not have been
+  // added to `state.narrows`, see the condition on `caughtUp` in
+  // narrowsReducer's handling of EVENT_NEW_MESSAGE.
+  if (!anyNarrowIsCaughtUp) {
+    return state;
+  }
+
+  // If changing or adding case where we ignore a message here:
+  // Careful! Every message in `state.narrows` must exist in
+  // `state.messages`. If we choose not to include a message in
+  // `state.messages`, then narrowsReducer MUST ALSO choose not to
+  // include it in `state.narrows`.
+
   return state.set(action.message.id, omit(action.message, 'flags'));
 };