diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index b09467ad0ad..1df44439c11 100644 --- a/src/chat/narrowsReducer.js +++ b/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]); }); }); diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js index 3b03a974956..dcc222a616e 100644 --- a/src/message/__tests__/messagesReducer-test.js +++ b/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', () => { diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js index 58690186145..93a13910dbd 100644 --- a/src/message/messagesReducer.js +++ b/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')); };