diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index b43d6870a32..7042a7f2673 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -18,6 +18,7 @@ import type { } from '../../api/modelTypes'; import { makeUserId } from '../../api/idTypes'; import type { InitialData } from '../../api/apiTypes'; +import { EventTypes, type UpdateMessageEvent } from '../../api/eventTypes'; import type { AccountSwitchAction, LoginSuccessAction, @@ -44,6 +45,7 @@ import { EVENT_NEW_MESSAGE, MESSAGE_FETCH_START, MESSAGE_FETCH_COMPLETE, + EVENT_UPDATE_MESSAGE, } from '../../actionConstants'; import rootReducer from '../../boot/reducers'; import { authOfAccount } from '../../account/accountMisc'; @@ -51,6 +53,7 @@ import { HOME_NARROW } from '../../utils/narrow'; import type { BackgroundData } from '../../webview/MessageList'; import { getSettings, getStreamsById, getSubscriptionsById } from '../../selectors'; import { getGlobalSettings } from '../../directSelectors'; +import { messageMoved } from '../../api/misc'; /* ======================================================================== * Utilities @@ -464,7 +467,7 @@ export const streamMessage = (args?: {| content_type: 'text/html', id: randMessageId(), subject: 'example topic', - subject_links: [], + topic_links: ([]: {| text: string, url: string |}[]), timestamp: 1556579727, type: 'stream', }; @@ -900,6 +903,25 @@ export const mkActionEventNewMessage = ( message: { ...message, flags: message.flags ?? [] }, }); +/** + * An EVENT_UPDATE_MESSAGE action. + */ +export const mkActionEventUpdateMessage = (args: {| + ...$Rest, +|}): PerAccountAction => { + const event = { + id: 1, + type: EventTypes.update_message, + flags: [], + ...args, + }; + return { + type: EVENT_UPDATE_MESSAGE, + event, + move: messageMoved(event), + }; +}; + // If a given action is only relevant to a single test file, no need to // provide a generic factory for it here; just define the test data there. diff --git a/src/actionTypes.js b/src/actionTypes.js index 9716811dbd4..0cf7ef4f3e4 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -69,8 +69,10 @@ import type { RealmUpdateDictEvent, SubmessageEvent, RestartEvent, + UpdateMessageEvent, } from './api/eventTypes'; import type { MutedTopicTuple, PresenceSnapshot } from './api/apiTypes'; +import type { MessageMove } from './api/misc'; import type { Orientation, @@ -342,45 +344,11 @@ type EventMessageDeleteAction = $ReadOnly<{| messageIds: $ReadOnlyArray, |}>; -// This is current to feature level 109: -// https://zulip.com/api/get-events#update_message type EventUpdateMessageAction = $ReadOnly<{| - ...ServerEvent, type: typeof EVENT_UPDATE_MESSAGE, - - // Future servers might send `null` here: - // https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1309241 - // TODO(server-5.0): Update this and/or simplify. - user_id?: UserId | null, - - // Any content changes apply to just message_id. - message_id: number, - - // Any stream/topic changes apply to all of message_ids, which is - // guaranteed to include message_id. - message_ids: $ReadOnlyArray, - - flags: $ReadOnlyArray, - edit_timestamp?: number, - stream_name?: string, - stream_id?: number, - new_stream_id?: number, - propagate_mode?: 'change_one' | 'change_later' | 'change_all', - orig_subject?: string, - subject?: string, - - // TODO(server-4.0): Changed in feat. 46 to array-of-objects shape, from $ReadOnlyArray - topic_links?: $ReadOnlyArray<{| +text: string, +url: string |}> | $ReadOnlyArray, - - // TODO(server-3.0): Replaced in feat. 1 by topic_links - subject_links?: $ReadOnlyArray, - - orig_content?: string, - orig_rendered_content?: string, - prev_rendered_content_version?: number, - content?: string, - rendered_content?: string, - is_me_message?: boolean, + /** Here `message_ids` is sorted. (This isn't guaranteed in the server event.) */ + event: UpdateMessageEvent, + move: null | MessageMove, |}>; type EventReactionCommon = $ReadOnly<{| diff --git a/src/api/eventTypes.js b/src/api/eventTypes.js index 92e72b4ef81..0fff7ae90be 100644 --- a/src/api/eventTypes.js +++ b/src/api/eventTypes.js @@ -207,3 +207,52 @@ export type RealmUpdateDictEvent = $ReadOnly<{| property: 'default', data: $Rest, |}>; + +/** + * https://zulip.com/api/get-events#update_message + * + * See also `messageMoved` in `misc.js`. + */ +// This is current to feature level 109. +// NB if this ever gains a feature of moving PMs, `messageMoved` needs updating. +export type UpdateMessageEvent = $ReadOnly<{| + ...EventCommon, + type: typeof EventTypes.update_message, + + // Future servers might send `null` here: + // https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1309241 + // TODO(server-5.0): Update this and/or simplify. + user_id?: UserId | null, + + // Any content changes apply to just message_id. + message_id: number, + + // Any stream/topic changes apply to all of message_ids, which is + // guaranteed to include message_id. + // TODO(server-future): It'd be nice for these to be sorted; NB they may + // not be. As of server 5.0-dev-3868-gca17a452f (2022-02), there's no + // guarantee of that in the API nor, apparently, in the implementation. + message_ids: $ReadOnlyArray, + + flags: $ReadOnlyArray, + edit_timestamp?: number, + stream_name?: string, + stream_id?: number, + new_stream_id?: number, + propagate_mode?: 'change_one' | 'change_later' | 'change_all', + orig_subject?: string, + subject?: string, + + // TODO(server-4.0): Changed in feat. 46 to array-of-objects shape, from $ReadOnlyArray + topic_links?: $ReadOnlyArray<{| +text: string, +url: string |}> | $ReadOnlyArray, + + // TODO(server-3.0): Replaced in feat. 1 by topic_links + subject_links?: $ReadOnlyArray, + + orig_content?: string, + orig_rendered_content?: string, + prev_rendered_content_version?: number, + content?: string, + rendered_content?: string, + is_me_message?: boolean, +|}>; diff --git a/src/api/misc.js b/src/api/misc.js new file mode 100644 index 00000000000..9ce1ae767d3 --- /dev/null +++ b/src/api/misc.js @@ -0,0 +1,54 @@ +/** + * Functions to interpret some data from the API. + * + * @flow strict-local + */ +import invariant from 'invariant'; + +import type { UpdateMessageEvent } from './eventTypes'; +import * as logging from '../utils/logging'; + +export type MessageMove = $ReadOnly<{| + orig_stream_id: number, + new_stream_id: number, + orig_topic: string, + new_topic: string, +|}>; + +/** + * Determine if and where this edit moved the message. + * + * If a move did occur, returns the relevant information in a form with a + * consistent naming convention. + * + * Returns null if the message stayed at the same conversation. + */ +export function messageMoved(event: UpdateMessageEvent): null | MessageMove { + // The API uses "new" for the stream IDs and "orig" for the topics. + // Put them both in a consistent naming convention. + const orig_stream_id = event.stream_id; + if (orig_stream_id == null) { + // Not stream messages, or else a pure content edit (no stream/topic change.) + // TODO(server-5.0): Simplify comment: since FL 112 this means it's + // just not a stream message. + return null; + } + const new_stream_id = event.new_stream_id ?? orig_stream_id; + const orig_topic = event.orig_subject; + const new_topic = event.subject ?? orig_topic; + + if (new_topic === orig_topic && new_stream_id === orig_stream_id) { + // Stream and topic didn't change. + return null; + } + + if (orig_topic == null) { + // `orig_subject` is documented to be present when either the + // stream or topic changed. + logging.warn('Got update_message event with stream/topic change and no orig_subject'); + return null; + } + invariant(new_topic != null, 'new_topic must be non-nullish when orig_topic is, by `??`'); + + return { orig_stream_id, new_stream_id, orig_topic, new_topic }; +} diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index ece397dfff2..124db80901f 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -675,7 +675,15 @@ export type StreamMessage = $ReadOnly<{| * (see point 4). We assume this has always been the case. */ subject: string, - subject_links: $ReadOnlyArray, + + // We don't actually use this property. If and when we do, we'll probably + // want in our internal model to canonicalize it to the newest form (with + // the name topic_links rather than subject_links, and newest type.) + // TODO(server-4.0): Changed in feat. 46 to array-of-objects shape, from $ReadOnlyArray + topic_links?: $ReadOnlyArray<{| +text: string, +url: string |}> | $ReadOnlyArray, + + // TODO(server-3.0): Replaced in feat. 1 by topic_links + subject_links?: $ReadOnlyArray, |}>; /** diff --git a/src/caughtup/__tests__/caughtUpReducer-test.js b/src/caughtup/__tests__/caughtUpReducer-test.js index 193a34bf952..8f674b571fa 100644 --- a/src/caughtup/__tests__/caughtUpReducer-test.js +++ b/src/caughtup/__tests__/caughtUpReducer-test.js @@ -4,7 +4,15 @@ import deepFreeze from 'deep-freeze'; import * as eg from '../../__tests__/lib/exampleData'; import caughtUpReducer from '../caughtUpReducer'; import { MESSAGE_FETCH_ERROR } from '../../actionConstants'; -import { HOME_NARROW, HOME_NARROW_STR, SEARCH_NARROW } from '../../utils/narrow'; +import { + HOME_NARROW, + HOME_NARROW_STR, + keyFromNarrow, + SEARCH_NARROW, + streamNarrow, + topicNarrow, +} from '../../utils/narrow'; +import { objectFromEntries } from '../../jsBackport'; describe('caughtUpReducer', () => { describe('MESSAGE_FETCH_START', () => { @@ -140,4 +148,76 @@ describe('caughtUpReducer', () => { expect(newState).toEqual(expectedState); }); + + describe('EVENT_UPDATE_MESSAGE', () => { + const mkAction = args => { + const { messages, ...restArgs } = args; + const message = messages[0]; + return eg.mkActionEventUpdateMessage({ + message_id: message.id, + message_ids: messages.map(m => m.id), + stream_id: message.stream_id, + orig_subject: message.subject, + ...restArgs, + }); + }; + + const mkKey = (stream, topic) => + topic !== undefined + ? keyFromNarrow(topicNarrow(stream.stream_id, topic)) + : keyFromNarrow(streamNarrow(stream.stream_id)); + + const topic1 = 'topic foo'; + const topic2 = 'topic bar'; + // const message1a = eg.streamMessage({ subject: topic1, id: 1 }); + const message1b = eg.streamMessage({ subject: topic1, id: 2 }); + // const message1c = eg.streamMessage({ subject: topic1, id: 3 }); + // const message2a = eg.streamMessage({ subject: topic2, id: 4 }); + + test('new topic, same stream', () => { + expect( + caughtUpReducer( + objectFromEntries([ + [mkKey(eg.stream, topic1), { older: true, newer: true }], + [mkKey(eg.stream, topic2), { older: true, newer: true }], + [mkKey(eg.stream), { older: true, newer: true }], + ]), + mkAction({ messages: [message1b], subject: topic2 }), + ), + ).toEqual( + objectFromEntries([ + // old topic narrow remains caught up: + [mkKey(eg.stream, topic1), { older: true, newer: true }], + // new topic narrow gets cleared + // stream narrow unchanged: + [mkKey(eg.stream), { older: true, newer: true }], + ]), + ); + }); + + test('same topic, new stream', () => { + expect( + caughtUpReducer( + objectFromEntries([ + [mkKey(eg.stream, topic1), { older: true, newer: true }], + [mkKey(eg.stream), { older: true, newer: true }], + [mkKey(eg.otherStream, topic1), { older: true, newer: true }], + [mkKey(eg.otherStream), { older: true, newer: true }], + ]), + mkAction({ messages: [message1b], new_stream_id: eg.otherStream.stream_id }), + ), + ).toEqual( + objectFromEntries([ + // old topic and stream narrows remain caught up: + [mkKey(eg.stream, topic1), { older: true, newer: true }], + [mkKey(eg.stream), { older: true, newer: true }], + // new topic and stream narrows both cleared + ]), + ); + }); + + // Try to keep these tests corresponding closely to those for the + // narrows reducer. (In the future these should really be a single + // sub-reducer.) + }); }); diff --git a/src/caughtup/caughtUpReducer.js b/src/caughtup/caughtUpReducer.js index 83f2f96546d..a291a1c724a 100644 --- a/src/caughtup/caughtUpReducer.js +++ b/src/caughtup/caughtUpReducer.js @@ -8,13 +8,27 @@ import { MESSAGE_FETCH_START, MESSAGE_FETCH_ERROR, MESSAGE_FETCH_COMPLETE, + EVENT_UPDATE_MESSAGE, + EVENT_UPDATE_MESSAGE_FLAGS, } from '../actionConstants'; import { NULL_OBJECT } from '../nullObjects'; import { DEFAULT_CAUGHTUP } from './caughtUpSelectors'; -import { isSearchNarrow, keyFromNarrow } from '../utils/narrow'; +import { isSearchNarrow, keyFromNarrow, streamNarrow, topicNarrow } from '../utils/narrow'; const initialState: CaughtUpState = NULL_OBJECT; +/** Corresponds to the same-name function in the narrows reducer. */ +function addMessages(state: CaughtUpState, narrow, messageIds): CaughtUpState { + // NOTE: This behavior must stay parallel with how the narrows reducer + // handles the same cases. + // See narrowsReducer.js for discussion. + const key = keyFromNarrow(narrow); + + // eslint-disable-next-line no-unused-vars + const { [key]: ignored, ...rest } = state; + return rest; +} + export default ( state: CaughtUpState = initialState, action: PerAccountApplicableAction, @@ -61,6 +75,33 @@ export default ( }; } + case EVENT_UPDATE_MESSAGE: { + // Compare the corresponding narrowsReducer case. + + let result = state; + const { event, move } = action; + + if (move) { + const { orig_stream_id, new_stream_id, new_topic } = move; + result = addMessages(result, topicNarrow(new_stream_id, new_topic), event.message_ids); + if (new_stream_id !== orig_stream_id) { + result = addMessages(result, streamNarrow(new_stream_id), event.message_ids); + } + } + + // We don't attempt to update search narrows. + + // The other way editing a message can affect what narrows it falls + // into is by changing its flags. Those cause a separate event; see + // the EVENT_UPDATE_MESSAGE_FLAGS case. + + return result; + } + + case EVENT_UPDATE_MESSAGE_FLAGS: + // TODO(#3408): Handle this to parallel narrowsReducer. + return state; + default: return state; } diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 522b031587d..98067393d6c 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -25,10 +25,11 @@ import { getShownMessagesForNarrow, isNarrowValid as getIsNarrowValid } from './ import { getFirstUnreadIdInNarrow } from '../message/messageSelectors'; import { getDraftForNarrow } from '../drafts/draftsSelectors'; import { addToOutbox } from '../actions'; -import { getAuth } from '../selectors'; +import { getAuth, getCaughtUpForNarrow } from '../selectors'; import { showErrorAlert } from '../utils/info'; import { TranslationContext } from '../boot/TranslationProvider'; import * as api from '../api'; +import { useEdgeTriggeredEffect } from '../reactUtils'; type Props = $ReadOnly<{| navigation: AppNavigationProp<'chat'>, @@ -61,6 +62,7 @@ const useMessagesWithFetch = args => { const loading = useSelector(getLoading); const fetching = useSelector(state => getFetchingForNarrow(state, narrow)); const isFetching = fetching.older || fetching.newer || loading; + const caughtUp = useSelector(state => getCaughtUpForNarrow(state, narrow)); const messages = useSelector(state => getShownMessagesForNarrow(state, narrow)); const firstUnreadIdInNarrow = useSelector(state => getFirstUnreadIdInNarrow(state, narrow)); @@ -69,6 +71,9 @@ const useMessagesWithFetch = args => { // like using instance variables in class components: // https://reactjs.org/docs/hooks-faq.html#is-there-something-like-instance-variables const shouldFetchWhenNextFocused = React.useRef(false); + const scheduleFetch = () => { + shouldFetchWhenNextFocused.current = true; + }; const [fetchError, setFetchError] = React.useState(null); @@ -85,13 +90,22 @@ const useMessagesWithFetch = args => { // set this to null from a non-null value, so this really does mean the // event queue has changed; it can't mean that we had a queue ID and // dropped it.) - React.useEffect(() => { - shouldFetchWhenNextFocused.current = true; - }, [eventQueueId]); - - // On first mount, fetch. Also unset `shouldFetchWhenNextFocused.current` - // that was set in the previous `useEffect`, so the fetch below doesn't - // also fire. + React.useEffect(scheduleFetch, [eventQueueId]); + + // If we stop having any data at all about the messages in this narrow -- + // we don't know any, and nor do we know if there are some -- then + // schedule a fetch. (As long as we have *some* messages, we'll show a + // proper MessageList, and scroll events can cause us to fetch newer or + // older messages. But with none, we'll show NoMessages.) + // + // TODO: We may still briefly show NoMessages; this render will have + // isFetching false, even though the fetch effect will cause a rerender + // with isFetching true. It'd be nice to avoid that. + const nothingKnown = messages.length === 0 && !caughtUp.older && !caughtUp.newer; + useEdgeTriggeredEffect(scheduleFetch, nothingKnown, true); + + // On first mount, fetch. (This also makes a fetch no longer scheduled, + // so the if-scheduled fetch below doesn't also fire.) React.useEffect(() => { fetch(); }, [fetch]); @@ -101,8 +115,9 @@ const useMessagesWithFetch = args => { if (shouldFetchWhenNextFocused.current && isFocused === true) { fetch(); } - // `eventQueueId` needed here because it affects `shouldFetchWhenNextFocused`. - }, [isFocused, eventQueueId, fetch]); + // No dependencies list, to run this effect after every render. This + // effect does its own checking of whether any work needs to be done. + }); return { fetchError, isFetching, messages, firstUnreadIdInNarrow }; }; diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 822ebcfc938..51228fd1bf6 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -497,6 +497,75 @@ describe('narrowsReducer', () => { }); }); + describe('EVENT_UPDATE_MESSAGE', () => { + const mkAction = args => { + const { messages, ...restArgs } = args; + const message = messages[0]; + return eg.mkActionEventUpdateMessage({ + message_id: message.id, + message_ids: messages.map(m => m.id), + stream_id: message.stream_id, + orig_subject: message.subject, + ...restArgs, + }); + }; + + const mkKey = (stream, topic) => + topic !== undefined + ? keyFromNarrow(topicNarrow(stream.stream_id, topic)) + : keyFromNarrow(streamNarrow(stream.stream_id)); + + const topic1 = 'topic foo'; + const topic2 = 'topic bar'; + // const message1a = eg.streamMessage({ subject: topic1, id: 1 }); + const message1b = eg.streamMessage({ subject: topic1, id: 2 }); + // const message1c = eg.streamMessage({ subject: topic1, id: 3 }); + // const message2a = eg.streamMessage({ subject: topic2, id: 4 }); + + test('new topic, same stream', () => { + expect( + narrowsReducer( + Immutable.Map([ + [mkKey(eg.stream, topic1), [1, 2, 3]], + [mkKey(eg.stream, topic2), [4]], + [mkKey(eg.stream), [1, 2, 3, 4]], + ]), + mkAction({ messages: [message1b], subject: topic2 }), + ), + ).toEqual( + Immutable.Map([ + [mkKey(eg.stream, topic1), [1, 3]], // removed from old topic narrow + // new topic narrow gets cleared + [mkKey(eg.stream), [1, 2, 3, 4]], // stream narrow unchanged + ]), + ); + }); + + test('same topic, new stream', () => { + expect( + narrowsReducer( + Immutable.Map([ + [mkKey(eg.stream, topic1), [1, 2, 3]], + [mkKey(eg.stream), [1, 2, 3]], + [mkKey(eg.otherStream, topic1), [4]], + [mkKey(eg.otherStream), [4]], + ]), + mkAction({ messages: [message1b], new_stream_id: eg.otherStream.stream_id }), + ), + ).toEqual( + Immutable.Map([ + [mkKey(eg.stream, topic1), [1, 3]], // removed from old topic narrow + [mkKey(eg.stream), [1, 3]], // removed from old stream narrow + // new topic narrow and stream narrow both cleared + ]), + ); + }); + + // Try to keep these tests corresponding closely to those for the + // caughtUp reducer. (In the future these should really be a single + // sub-reducer.) + }); + describe('EVENT_UPDATE_MESSAGE_FLAGS', () => { const allMessages = eg.makeMessagesState([ eg.streamMessage({ id: 1 }), diff --git a/src/chat/narrowsReducer.js b/src/chat/narrowsReducer.js index 0ee59dde570..c521959e106 100644 --- a/src/chat/narrowsReducer.js +++ b/src/chat/narrowsReducer.js @@ -16,6 +16,7 @@ import { EVENT_NEW_MESSAGE, EVENT_MESSAGE_DELETE, EVENT_UPDATE_MESSAGE_FLAGS, + EVENT_UPDATE_MESSAGE, } from '../actionConstants'; import { LAST_MESSAGE_ANCHOR, FIRST_UNREAD_ANCHOR } from '../anchor'; import { @@ -24,10 +25,51 @@ import { STARRED_NARROW_STR, isSearchNarrow, keyFromNarrow, + topicNarrow, + streamNarrow, } from '../utils/narrow'; const initialState: NarrowsState = Immutable.Map(); +function removeMessages(state: NarrowsState, narrow, messageIds): NarrowsState { + return state.update( + keyFromNarrow(narrow), + messages => messages && messages.filter(id => !messageIds.has(id)), + ); +} + +/** + * Incorporate possibly old, possibly discontiguous, messages in the narrow. + * + * This differs from the MESSAGE_FETCH_COMPLETE case in that we aren't + * assured that the messages listed are a contiguous segment of the full + * list of messages in the narrow. (For example, someone may have merged + * one conversation into another, where some messages already in the + * destination conversation fall in between some of the moved messages.) + */ +// We need to maintain the state.narrows invariant -- the fact that the +// message IDs we have in a narrow should be a contiguous segment of the +// full list of messages that actually exist in the narrow. That can +// prevent us from adding the messages to our record of the narrow, and +// force us to instead downgrade how much we think we know about the narrow. +function addMessages(state: NarrowsState, narrow, messageIds): NarrowsState { + // NOTE: This behavior must stay parallel with how the caughtUp reducer + // handles the same cases. + const key = keyFromNarrow(narrow); + + // TODO: If the state at the narrow covers the given messages, then + // incorporate them. + + // TODO: If the state at a *parent* narrow -- in particular the stream + // narrow, if this is a topic narrow -- covers the given messages, then + // use that. + + // Do what's simple and always correct, even when not optimal: stop + // claiming to know anything about the narrow. (The caughtUp reducer must + // also delete its record.) + return state.delete(key); +} + const messageFetchComplete = (state, action) => { // We don't want to accumulate old searches that we'll never need again. if (isSearchNarrow(action.narrow)) { @@ -189,6 +231,33 @@ export default ( case EVENT_MESSAGE_DELETE: return eventMessageDelete(state, action); + case EVENT_UPDATE_MESSAGE: { + // Compare the corresponding caughtUpReducer case. + + let result: NarrowsState = state; + const { event, move } = action; + + if (move) { + // The edit changed topic and/or stream. + const { orig_stream_id, orig_topic, new_stream_id, new_topic } = move; + const messageIdSet = new Set(event.message_ids); + result = addMessages(result, topicNarrow(new_stream_id, new_topic), event.message_ids); + result = removeMessages(result, topicNarrow(orig_stream_id, orig_topic), messageIdSet); + if (new_stream_id !== orig_stream_id) { + result = addMessages(result, streamNarrow(new_stream_id), event.message_ids); + result = removeMessages(result, streamNarrow(orig_stream_id), messageIdSet); + } + } + + // We don't attempt to update search narrows. + + // The other way editing a message can affect what narrows it falls + // into is by changing its flags. Those cause a separate event; see + // the EVENT_UPDATE_MESSAGE_FLAGS case. + + return result; + } + case EVENT_UPDATE_MESSAGE_FLAGS: return eventUpdateMessageFlags(state, action); diff --git a/src/events/doEventActionSideEffects.js b/src/events/doEventActionSideEffects.js index 28c5baf3ccc..765d6e85cc9 100644 --- a/src/events/doEventActionSideEffects.js +++ b/src/events/doEventActionSideEffects.js @@ -1,10 +1,19 @@ /* @flow strict-local */ // import { Vibration } from 'react-native'; +import { CommonActions } from '@react-navigation/native'; -import type { ThunkAction } from '../types'; +import type { Narrow, ThunkAction } from '../types'; import type { EventAction } from '../actionTypes'; -import { EVENT_TYPING_START } from '../actionConstants'; +import { EVENT_TYPING_START, EVENT_UPDATE_MESSAGE } from '../actionConstants'; import { ensureTypingStatusExpiryLoop } from '../typing/typingActions'; +import * as NavigationService from '../nav/NavigationService'; +import { + isStreamNarrow, + isTopicNarrow, + streamIdOfNarrow, + topicNarrow, + topicOfNarrow, +} from '../utils/narrow'; /** * React to actions dispatched for Zulip server events. @@ -16,6 +25,68 @@ export default (action: EventAction): ThunkAction> => async (dispa case EVENT_TYPING_START: dispatch(ensureTypingStatusExpiryLoop()); break; + + case EVENT_UPDATE_MESSAGE: { + // If the conversation we were looking at got moved, follow it. + // See #5251 for background. + + const { event, move } = action; + const { propagate_mode } = event; + if (!move || !(propagate_mode === 'change_all' || propagate_mode === 'change_later')) { + // This edit wasn't a move, or was targeted to a specific message. + break; + } + + const navState = NavigationService.getState(); + for (const route of navState.routes) { + if (route.name !== 'chat') { + continue; + } + + // TODO(#5005): Only make these updates if this ChatScreen is for + // the account that this event applies to. (I.e., just if this is + // the active account.) + + // $FlowFixMe[incompatible-use]: relying on ChatScreen having route params + // $FlowFixMe[incompatible-type]: relying on ChatScreen route-params type + const narrow: Narrow = route.params.narrow; + if ( + isTopicNarrow(narrow) + && streamIdOfNarrow(narrow) === move.orig_stream_id + && topicOfNarrow(narrow) === move.orig_topic + ) { + // A ChatScreen showing the very conversation that was moved. + + // Change the ChatScreen's narrow to follow the move. + // + // Web does this only if the blue box is on one of the affected + // messages, in case only some of the conversation was moved. + // We don't have a blue box, but: + // TODO: Ideally if the moved messages are all offscreen we'd skip this. + NavigationService.dispatch({ + ...CommonActions.setParams({ + narrow: topicNarrow(move.new_stream_id, move.new_topic), + }), + // Spreading the `setParams` action and adding `source` like + // this is the documented way to specify a route (rather than + // apply to the focused route): + // https://reactnavigation.org/docs/5.x/navigation-actions#setparams + source: route.key, + }); + + // TODO(#5251): If compose box is open and topic was resolved, warn. + } else if (isStreamNarrow(narrow) && streamIdOfNarrow(narrow) === move.orig_stream_id) { + // A ChatScreen showing the stream that contained the moved messages. + // + // TODO(#5251): Update topic input, if it matches. (If the stream + // changed too, unclear what to do. Possibly change the + // screen's narrow, as if narrowed to the moved topic?) + } + } + + break; + } + default: } }; diff --git a/src/events/eventToAction.js b/src/events/eventToAction.js index a56766063ab..a4a548ae901 100644 --- a/src/events/eventToAction.js +++ b/src/events/eventToAction.js @@ -36,6 +36,7 @@ import { import { getOwnUserId, tryGetUserForId } from '../users/userSelectors'; import { AvatarURL } from '../utils/avatar'; import { getRealmUrl } from '../account/accountsSelectors'; +import { messageMoved } from '../api/misc'; const opToActionUserGroup = { add: EVENT_USER_GROUP_ADD, @@ -56,7 +57,6 @@ const opToActionTyping = { }; const actionTypeOfEventType = { - update_message: EVENT_UPDATE_MESSAGE, subscription: EVENT_SUBSCRIPTION, presence: EVENT_PRESENCE, muted_topics: EVENT_MUTED_TOPICS, @@ -161,7 +161,13 @@ export default (state: PerAccountState, event: $FlowFixMe): EventAction | null = event, }; - case 'update_message': + case EventTypes.update_message: + return { + type: EVENT_UPDATE_MESSAGE, + event: { ...event, message_ids: event.message_ids.sort((a, b) => a - b) }, + move: messageMoved(event), + }; + case 'subscription': case 'presence': case 'muted_topics': diff --git a/src/message/__tests__/messagesReducer-test.js b/src/message/__tests__/messagesReducer-test.js index f2137837692..943ed661948 100644 --- a/src/message/__tests__/messagesReducer-test.js +++ b/src/message/__tests__/messagesReducer-test.js @@ -8,7 +8,6 @@ import { MESSAGE_FETCH_COMPLETE, EVENT_SUBMESSAGE, EVENT_MESSAGE_DELETE, - EVENT_UPDATE_MESSAGE, EVENT_REACTION_ADD, EVENT_REACTION_REMOVE, } from '../../actionConstants'; @@ -33,7 +32,7 @@ describe('messagesReducer', () => { }, }); const expectedState = eg.makeMessagesState([message1, message2, message3]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); expect(newState).not.toBe(prevState); }); @@ -51,7 +50,7 @@ describe('messagesReducer', () => { }, }, }); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(prevState); }); }); @@ -71,7 +70,7 @@ describe('messagesReducer', () => { msg_type: 'widget', content: eg.randString(), }); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toBe(prevState); }); @@ -119,7 +118,7 @@ describe('messagesReducer', () => { ], }, ]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); expect(newState).not.toBe(prevState); }); @@ -131,7 +130,7 @@ describe('messagesReducer', () => { const prevState = eg.makeMessagesState([message1]); const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, messageIds: [2] }); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(prevState); expect(newState).toBe(prevState); }); @@ -143,7 +142,7 @@ describe('messagesReducer', () => { const prevState = eg.makeMessagesState([message1, message2]); const action = deepFreeze({ type: EVENT_MESSAGE_DELETE, messageIds: [message2.id] }); const expectedState = eg.makeMessagesState([message1]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); }); @@ -158,7 +157,7 @@ describe('messagesReducer', () => { messageIds: [message2.id, message3.id, 4], }); const expectedState = eg.makeMessagesState([message1]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); }); }); @@ -171,17 +170,26 @@ describe('messagesReducer', () => { // stealth server-edits.) const forEdit: { user_id?: UserId } = restArgs.edit_timestamp != null ? { user_id: message.sender_id } : Object.freeze({}); - return { - id: 1, - type: EVENT_UPDATE_MESSAGE, + return eg.mkActionEventUpdateMessage({ ...forEdit, message_id: message.id, message_ids: [message.id], - flags: [], propagate_mode: 'change_one', is_me_message: false, ...restArgs, - }; + }); + }; + + const mkMoveAction = args => { + const { message, ...restArgs } = args; + return mkAction({ + message, + // stream_id and orig_subject are always present when either + // the stream or the topic was changed. + stream_id: message.stream_id, + orig_subject: message.subject, + ...restArgs, + }); }; test('if a message does not exist no changes are made', () => { @@ -199,10 +207,92 @@ describe('messagesReducer', () => { rendered_content: eg.randString(), content: eg.randString(), }); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toBe(prevState); }); + describe('move', () => { + test('edited topic', () => { + const message = eg.streamMessage(); + const newTopic = `${message.subject}abc`; + const action = mkMoveAction({ message, subject: newTopic }); + expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual( + eg.makeMessagesState([{ ...message, subject: newTopic }]), + ); + }); + + test('other messages in conversation are unaffected', () => { + const topic = 'some topic'; + const message1 = eg.streamMessage({ subject: topic }); + const message2 = eg.streamMessage({ subject: topic }); + const message3 = eg.streamMessage({ subject: topic }); + const newTopic = 'some revised topic'; + const action = mkMoveAction({ + message: message1, + message_ids: [message1.id, message2.id], + subject: newTopic, + }); + expect( + messagesReducer( + eg.makeMessagesState([message1, message2, message3]), + action, + eg.plusReduxState, + ), + ).toEqual( + eg.makeMessagesState([ + { ...message1, subject: newTopic }, + { ...message2, subject: newTopic }, + message3, + ]), + ); + }); + + test('new stream', () => { + const message = eg.streamMessage(); + const action = mkMoveAction({ message, new_stream_id: eg.otherStream.stream_id }); + expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual( + eg.makeMessagesState([ + { + ...message, + stream_id: eg.otherStream.stream_id, + display_recipient: eg.otherStream.name, + }, + ]), + ); + }); + + test('new stream + edited topic', () => { + const message = eg.streamMessage(); + const newTopic = `${message.subject}abc`; + const action = mkMoveAction({ + message, + new_stream_id: eg.otherStream.stream_id, + subject: newTopic, + }); + expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual( + eg.makeMessagesState([ + { + ...message, + stream_id: eg.otherStream.stream_id, + display_recipient: eg.otherStream.name, + subject: newTopic, + }, + ]), + ); + }); + + test('new, unknown stream', () => { + const message = eg.streamMessage(); + const unknownStream = eg.makeStream(); + const action = mkMoveAction({ message, new_stream_id: unknownStream.stream_id }); + expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual( + eg.makeMessagesState([ + { ...message, stream_id: unknownStream.stream_id, display_recipient: 'unknown' }, + ]), + ); + }); + }); + test('when a message exists in state, it is updated', () => { const message1 = eg.streamMessage(); const message2 = eg.streamMessage(); @@ -210,14 +300,6 @@ describe('messagesReducer', () => { const message3New = { ...message3Old, content: '

New content

', - edit_history: [ - { - prev_rendered_content: '

Old content

', - prev_rendered_content_version: 1, - timestamp: 123, - user_id: message3Old.sender_id, - }, - ], last_edit_timestamp: 123, }; @@ -232,7 +314,7 @@ describe('messagesReducer', () => { content: 'New content', }); const expectedState = eg.makeMessagesState([message1, message2, message3New]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); }); @@ -241,42 +323,29 @@ describe('messagesReducer', () => { content: 'Old content', subject: 'Old subject', last_edit_timestamp: 123, - subject_links: [], edit_history: [], }); const message1New = { ...message1Old, subject: 'New topic', last_edit_timestamp: 123, - subject_links: [], - edit_history: [ - { - timestamp: 123, - user_id: message1Old.sender_id, - prev_subject: message1Old.subject, - }, - ], }; const prevState = eg.makeMessagesState([message1Old]); - const action = mkAction({ + const action = mkMoveAction({ edit_timestamp: 123, - message: message1New, - stream_id: message1Old.stream_id, - orig_subject: message1Old.subject, - subject_links: [], + message: message1Old, subject: message1New.subject, }); const expectedState = eg.makeMessagesState([message1New]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); }); - test('when event contains a new subject and a new content, update both and update edit history object', () => { + test('when event contains a new subject and a new content, update both', () => { const message1Old = eg.streamMessage({ content: '

Old content

', subject: 'Old subject', last_edit_timestamp: 123, - subject_links: [], edit_history: [ { prev_subject: 'Old subject', @@ -290,21 +359,10 @@ describe('messagesReducer', () => { content: '

New content

', subject: 'New updated topic', last_edit_timestamp: 456, - subject_links: [], - edit_history: [ - { - prev_rendered_content: '

Old content

', - prev_rendered_content_version: 1, - prev_subject: 'Old subject', - timestamp: 456, - user_id: message1Old.sender_id, - }, - ...message1Old.edit_history, - ], }; const prevState = eg.makeMessagesState([message1Old]); - const action = mkAction({ + const action = mkMoveAction({ edit_timestamp: 456, message: message1Old, orig_content: message1Old.content, @@ -312,12 +370,10 @@ describe('messagesReducer', () => { rendered_content: message1New.content, content: message1New.content, subject: message1New.subject, - orig_subject: message1Old.subject, prev_rendered_content_version: 1, - subject_links: [], }); const expectedState = eg.makeMessagesState([message1New]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); }); @@ -340,7 +396,7 @@ describe('messagesReducer', () => { rendered_content: messageAfter.content, prev_rendered_content_version: 1, }); - expect(messagesReducer(eg.makeMessagesState([message]), action)).toEqual( + expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual( eg.makeMessagesState([messageAfter]), ); }); @@ -367,7 +423,7 @@ describe('messagesReducer', () => { rendered_content: messageAfter.content, prev_rendered_content_version: 1, }); - expect(messagesReducer(eg.makeMessagesState([message]), action)).toEqual( + expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual( eg.makeMessagesState([messageAfter]), ); }); @@ -393,7 +449,7 @@ describe('messagesReducer', () => { reactions: [reaction], }, ]); - const actualState = messagesReducer(prevState, action); + const actualState = messagesReducer(prevState, action, eg.plusReduxState); expect(actualState).toEqual(expectedState); }); }); @@ -411,7 +467,7 @@ describe('messagesReducer', () => { ...reaction, }); const expectedState = eg.makeMessagesState([message1]); - const actualState = messagesReducer(prevState, action); + const actualState = messagesReducer(prevState, action, eg.plusReduxState); expect(actualState).toEqual(expectedState); }); @@ -446,7 +502,7 @@ describe('messagesReducer', () => { const expectedState = eg.makeMessagesState([ { ...message1, reactions: [reaction2, reaction3] }, ]); - const actualState = messagesReducer(prevState, action); + const actualState = messagesReducer(prevState, action, eg.plusReduxState); expect(actualState).toEqual(expectedState); }); }); @@ -478,7 +534,7 @@ describe('messagesReducer', () => { message4, message5, ]); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState).toEqual(expectedState); }); @@ -500,7 +556,7 @@ describe('messagesReducer', () => { ownUserId: eg.selfUser.user_id, }); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState.get(message2.id)).toEqual(message2); expect(newState.get(message3.id)).toEqual(message3); @@ -525,7 +581,7 @@ describe('messagesReducer', () => { foundNewest: false, ownUserId: eg.selfUser.user_id, }); - const newState = messagesReducer(prevState, action); + const newState = messagesReducer(prevState, action, eg.plusReduxState); expect(newState.get(message2.id)).toEqual(message2); expect(newState.get(message3.id)).toEqual(message3); expect(newState.get(message4New.id)).toEqual(message4New); diff --git a/src/message/messagesReducer.js b/src/message/messagesReducer.js index ea646b1c927..1a0cb993fd5 100644 --- a/src/message/messagesReducer.js +++ b/src/message/messagesReducer.js @@ -3,7 +3,7 @@ import omit from 'lodash.omit'; import Immutable from 'immutable'; -import type { MessagesState, Message, PerAccountApplicableAction } from '../types'; +import type { MessagesState, Message, PerAccountApplicableAction, PerAccountState } from '../types'; import { REGISTER_COMPLETE, LOGOUT, @@ -18,6 +18,8 @@ import { EVENT_UPDATE_MESSAGE, } from '../actionConstants'; import { getNarrowsForMessage, keyFromNarrow } from '../utils/narrow'; +import * as logging from '../utils/logging'; +import { getStreamsById } from '../selectors'; const initialState: MessagesState = Immutable.Map([]); @@ -65,6 +67,7 @@ const eventNewMessage = (state, action) => { export default ( state: MessagesState = initialState, action: PerAccountApplicableAction, + globalState: PerAccountState, ): MessagesState => { switch (action.type) { case REGISTER_COMPLETE: @@ -135,72 +138,69 @@ export default ( case EVENT_MESSAGE_DELETE: return state.deleteAll(action.messageIds); - case EVENT_UPDATE_MESSAGE: - return state.update(action.message_id, (oldMessage: M): M => { + case EVENT_UPDATE_MESSAGE: { + const { event, move } = action; + let result = state; + + result = result.update(event.message_id, (oldMessage: M): M => { if (!oldMessage) { return oldMessage; } - const historyEntry = (() => { - if (action.edit_timestamp == null || action.user_id == null) { - // The update isn't a real edit; rather it's just filling in an - // inline URL preview (which can require the server to make an - // external request, so we don't let it block delivering the - // message to clients) and shouldn't appear in the edit history. - return null; - } - // TODO(#3408): This should handle the full complexity of update_message - // events: in particular, moves between streams. (Probably the - // first step is to refactor this logic for less duplication.) - // This is OK for now because we don't actually have any UI that - // exposes this history: #4134. - if (action.orig_rendered_content !== undefined) { - if (action.orig_subject !== undefined) { - return { - prev_rendered_content: action.orig_rendered_content, - prev_subject: oldMessage.subject, - timestamp: action.edit_timestamp, - prev_rendered_content_version: action.prev_rendered_content_version, - user_id: action.user_id, - }; - } else { - return { - prev_rendered_content: action.orig_rendered_content, - timestamp: action.edit_timestamp, - prev_rendered_content_version: action.prev_rendered_content_version, - user_id: action.user_id, - }; - } - } else { - return { - prev_subject: oldMessage.subject, - timestamp: action.edit_timestamp, - user_id: action.user_id, - }; - } - })(); - - const messageWithNewCommonFields: M = { + return { ...(oldMessage: M), - content: action.rendered_content ?? oldMessage.content, - edit_history: historyEntry - ? [historyEntry, ...(oldMessage.edit_history ?? [])] - : oldMessage.edit_history, - last_edit_timestamp: action.edit_timestamp ?? oldMessage.last_edit_timestamp, + content: event.rendered_content ?? oldMessage.content, + last_edit_timestamp: event.edit_timestamp ?? oldMessage.last_edit_timestamp, + // TODO(#3408): Update edit_history, too. This is OK for now + // because we don't actually have any UI to expose it: #4134. }; - - // FlowIssue: https://github.com/facebook/flow/issues/8833 - // The cast `: 'stream'` is silly but harmless, and works - // around a Flow issue which causes an error. - return messageWithNewCommonFields.type === ('stream': 'stream') - ? { - ...messageWithNewCommonFields, - subject: action.subject ?? messageWithNewCommonFields.subject, - subject_links: action.subject_links ?? messageWithNewCommonFields.subject_links, - } - : messageWithNewCommonFields; }); + if (move) { + const update: { subject: string, stream_id?: number, display_recipient?: string } = { + subject: move.new_topic, + // TODO(#3408): Update topic_links. This is OK for now + // because we don't have any UI to expose it. + // TODO(#3408): Update last_edit_timestamp, probably. But want to + // say "moved" in the UI in this case, not "edited". + }; + if (move.new_stream_id !== move.orig_stream_id) { + update.stream_id = move.new_stream_id; + const newStream = getStreamsById(globalState).get(move.new_stream_id); + // It's normal for newStream to potentially be missing here: it + // happens when the move was to a stream our user can't see. + // TODO(i18n): Not sure this "unknown" ever reaches the UI, but + // it'd be nice to somehow translate it in case it can. + update.display_recipient = newStream?.name ?? 'unknown'; + } + + // eslint-disable-next-line no-shadow + result = result.withMutations(state => { + for (const id of event.message_ids) { + state.update(id, (oldMessage: M | void): M | void => { + if (!oldMessage) { + return oldMessage; + } + + // FlowIssue: https://github.com/facebook/flow/issues/8833 + // The cast `: 'stream'` is silly but harmless, and works + // around a Flow issue which causes an error. + if (oldMessage.type !== ('stream': 'stream')) { + logging.warn('messagesReducer: got update_message with stream/topic move on PM'); + return oldMessage; + } + + // TODO(#3408): Update edit_history, too. This is OK for now + // because we don't actually have any UI to expose it: #4134. + return { ...oldMessage, ...update }; + }); + } + }); + } + + return result; + } + default: return state; } diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 05db356fe04..6d7135dff51 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -42,6 +42,7 @@ export const resetToMainTabs = (): GenericNavigationAction => /** Only call this via `doNarrow`. See there for details. */ export const navigateToChat = (narrow: Narrow): GenericNavigationAction => + // This route name 'chat' appears in one more place than usual: doEventActionSideEffects.js . StackActions.push('chat', { narrow, editMessage: null }); export const replaceWithChat = (narrow: Narrow): GenericNavigationAction => diff --git a/src/reactUtils.js b/src/reactUtils.js index 3f21e4b21fe..1d028d34aed 100644 --- a/src/reactUtils.js +++ b/src/reactUtils.js @@ -83,3 +83,35 @@ export const useHasStayedTrueForMs = (value: boolean, duration: number): boolean return result; }; + +/** + * Invoke the callback as an effect when `value` turns from false to true. + * + * This differs from putting `value` in a `useEffect` dependency list in + * that the callback will *not* be invoked when the value changes in the + * other direction, from true to false. + * + * `includeStart` controls what happens if `value` is true on the very first + * render. If `includeStart` is true, then the effect is triggered (so + * treating its previous state of nonexistence as equivalent to being + * false). If `includeStart` is false, then the effect is not triggered (so + * treating its previous state of nonexistence as not a valid state to + * compare to.) + * + * The callback is not permitted to return a cleanup function, because it's + * not clear what the semantics should be of when such a cleanup function + * would be run. + */ +export const useEdgeTriggeredEffect = ( + cb: () => void, + value: boolean, + includeStart: boolean, +): void => { + const prev = usePrevious(value, !includeStart); + useEffect(() => { + if (value && !prev) { + cb(); + } + // No dependencies list -- the effect itself decides whether to act. + }); +}; diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js index fbc27025a7a..caa2720ca64 100644 --- a/src/unread/__tests__/unreadModel-test.js +++ b/src/unread/__tests__/unreadModel-test.js @@ -1,11 +1,7 @@ /* @flow strict-local */ import Immutable from 'immutable'; -import { - ACCOUNT_SWITCH, - EVENT_UPDATE_MESSAGE_FLAGS, - EVENT_UPDATE_MESSAGE, -} from '../../actionConstants'; +import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants'; import { reducer } from '../unreadModel'; import { type UnreadState } from '../unreadModelTypes'; import * as eg from '../../__tests__/lib/exampleData'; @@ -65,16 +61,13 @@ describe('stream substate', () => { describe('EVENT_UPDATE_MESSAGE', () => { const mkAction = args => { const { message_ids, ...restArgs } = args; - return { - id: 1, - type: EVENT_UPDATE_MESSAGE, + return eg.mkActionEventUpdateMessage({ user_id: eg.selfUser.user_id, message_id: message_ids[0], message_ids, - flags: [], edit_timestamp: 10000, ...restArgs, - }; + }); }; const baseState = (() => { diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js index b0bc9164da4..1d844999b1b 100644 --- a/src/unread/unreadModel.js +++ b/src/unread/unreadModel.js @@ -27,7 +27,6 @@ import { MESSAGE_FETCH_COMPLETE, REGISTER_COMPLETE, } from '../actionConstants'; -import * as logging from '../utils/logging'; import DefaultMap from '../utils/DefaultMap'; // @@ -224,46 +223,25 @@ function streamsReducer( } case EVENT_UPDATE_MESSAGE: { - // The API uses "new" for the stream IDs and "orig" for the topics. - // Put them both in a consistent naming convention. - const origStreamId = action.stream_id; - if (origStreamId == null) { - // Not stream messages, or else a pure content edit (no stream/topic change.) - // TODO(server-5.0): Simplify comment: since FL 112 this means it's - // just not a stream message. + const { event, move } = action; + if (!move) { return state; } - const newStreamId = action.new_stream_id ?? origStreamId; - const origTopic = action.orig_subject; - const newTopic = action.subject ?? origTopic; - if (newTopic === origTopic && newStreamId === origStreamId) { - // Stream and topic didn't change. - return state; - } - - if (origTopic == null) { - // `orig_subject` is documented to be present when either the - // stream or topic changed. - logging.warn('Got update_message event with stream/topic change and no orig_subject'); - return state; - } - invariant(newTopic != null, 'newTopic must be non-nullish when origTopic is, by `??`'); - - const actionIds = new Set(action.message_ids); + const eventIds = new Set(event.message_ids); const matchingIds = state - .getIn([origStreamId, origTopic], Immutable.List()) - .filter(id => actionIds.has(id)); + .getIn([move.orig_stream_id, move.orig_topic], Immutable.List()) + .filter(id => eventIds.has(id)); if (matchingIds.size === 0) { // None of the updated messages were unread. return state; } return state - .updateIn([origStreamId, origTopic], (messages = Immutable.List()) => - messages.filter(id => !actionIds.has(id)), + .updateIn([move.orig_stream_id, move.orig_topic], (messages = Immutable.List()) => + messages.filter(id => !eventIds.has(id)), ) - .updateIn([newStreamId, newTopic], (messages = Immutable.List()) => + .updateIn([move.new_stream_id, move.new_topic], (messages = Immutable.List()) => messages.push(...matchingIds).sort(), ); }