diff --git a/docs/style.md b/docs/style.md index 4fabcad636d..9a9b37df7af 100644 --- a/docs/style.md +++ b/docs/style.md @@ -275,7 +275,7 @@ pick just one, and that's the one we use. [gh-close-issue-keywords]: https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords -## JavaScript and Flow +## JavaScript, Flow, JS libraries **Use `invariant` for runtime assertions the type-checker can use**: If there's a fact you're sure is true at a certain point in the code, @@ -294,6 +294,21 @@ definitely mean a bug within our own zulip-mobile codebase. [flow-invariant-pseudodocs]: https://github.com/facebook/flow/issues/6052 +**Always provide a type when writing an empty `Immutable` value**: +Whenever you create an empty `Immutable.Map`, `Immutable.List`, or +so on, specify the intended type explicitly. For example: +```js +const map: Immutable.Map = Immutable.Map(); // good + +const map = Immutable.Map(); // BAD -- don't do +``` + +This is essential in order to get effective type-checking of the +code that uses the new collection. (It's not clear if this is a bug +in Flow, or a design limitation of Flow, or an issue in the Flow types +provided by Immutable.js, or some combination.) + + ## Internal to Zulip and our codebase ### Zulip API bindings diff --git a/src/boot/store.js b/src/boot/store.js index c1f538d2941..8e823a5653d 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -260,6 +260,9 @@ const migrations: {| [string]: (GlobalState) => GlobalState |} = { // See `purge` call in src/third/redux-persist/persistStore.js. '24': dropCache, + // Convert `unread.streams` from over-the-wire array to `Immutable.Map`. + '25': dropCache, + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index f25b482b22c..7f0eabcba83 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -52,15 +52,8 @@ export const getTopicsForStream: Selector = createSe return topicList.map(({ name, max_id }) => { const isMuted = !!mute.find(x => x[0] === stream.name && x[1] === name); - const unreadStream = unreadStreams.find( - x => x.stream_id === stream.stream_id && x.topic === name, - ); - return { - name, - max_id, - isMuted, - unreadCount: unreadStream ? unreadStream.unread_message_ids.length : 0, - }; + const unreadCount = unreadStreams.get(stream.stream_id)?.get(name)?.size ?? 0; + return { name, max_id, isMuted, unreadCount }; }); }, ); diff --git a/src/unread/__tests__/unreadModel-test.js b/src/unread/__tests__/unreadModel-test.js index 813feb6f04e..287c2d4c815 100644 --- a/src/unread/__tests__/unreadModel-test.js +++ b/src/unread/__tests__/unreadModel-test.js @@ -3,6 +3,7 @@ import Immutable from 'immutable'; 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'; import { initialState, mkMessageAction } from './unread-testlib'; @@ -12,19 +13,10 @@ import { initialState, mkMessageAction } from './unread-testlib'; // but this way simplifies the conversion from the old tests. describe('stream substate', () => { // Summarize the state, for convenient comparison to expectations. - // In particular, abstract away irrelevant details of the ordering of - // streams and topics in the data structure -- those should never matter - // to selectors, and in a better data structure they wouldn't exist in the - // first place. - const summary = state => { - // prettier-ignore - const result: Immutable.Map> = - Immutable.Map().asMutable(); - for (const { stream_id, topic, unread_message_ids } of state.streams) { - result.setIn([stream_id, topic], unread_message_ids); - } - return result.asImmutable(); - }; + // Specifically just turn the inner `Immutable.List`s into arrays, + // to shorten writing the expected data. + const summary = (state: UnreadState) => + state.streams.map(perStream => perStream.map(perTopic => perTopic.toArray())); describe('ACCOUNT_SWITCH', () => { test('resets state to initial state', () => { @@ -87,15 +79,6 @@ describe('stream substate', () => { ])); }); - test('if message id already exists, do not mutate state', () => { - const state = reducer( - baseState, - action(eg.streamMessage({ id: 1, subject: 'some topic' })), - eg.plusReduxState, - ); - expect(state).toBe(baseState); - }); - test('if message is not stream, return original state', () => { const state = reducer(baseState, action(eg.pmMessage({ id: 4 })), eg.plusReduxState); expect(state.streams).toBe(baseState.streams); @@ -163,18 +146,28 @@ describe('stream substate', () => { }; }; + const messages = [ + eg.streamMessage({ stream_id: 123, subject: 'foo', id: 1 }), + eg.streamMessage({ stream_id: 123, subject: 'foo', id: 2 }), + eg.streamMessage({ stream_id: 123, subject: 'foo', id: 3 }), + eg.streamMessage({ stream_id: 234, subject: 'bar', id: 4 }), + eg.streamMessage({ stream_id: 234, subject: 'bar', id: 5 }), + ]; + const baseState = (() => { - const streamAction = args => mkMessageAction(eg.streamMessage(args)); const r = (state, action) => reducer(state, action, eg.plusReduxState); let state = initialState; - state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 })); - state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 2 })); - state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 3 })); - state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 4 })); - state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 5 })); + for (const message of messages) { + state = r(state, mkMessageAction(message)); + } return state; })(); + const baseGlobalState = eg.reduxStatePlus({ + messages: eg.makeMessagesState(messages), + unread: baseState, + }); + test('(base state, for comparison)', () => { // prettier-ignore expect(summary(baseState)).toEqual(Immutable.Map([ @@ -185,30 +178,55 @@ describe('stream substate', () => { test('when operation is "add" but flag is not "read" do not mutate state', () => { const action = mkAction({ messages: [1, 2, 3], flag: 'star' }); - expect(reducer(initialState, action, eg.plusReduxState)).toBe(initialState); + expect(reducer(initialState, action, baseGlobalState)).toBe(initialState); }); test('if id does not exist do not mutate state', () => { const action = mkAction({ messages: [6, 7] }); - expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState); + expect(reducer(baseState, action, baseGlobalState)).toBe(baseState); }); test('if ids are in state remove them', () => { const action = mkAction({ messages: [3, 4, 5, 6] }); // prettier-ignore - expect(summary(reducer(baseState, action, eg.plusReduxState))).toEqual(Immutable.Map([ + expect(summary(reducer(baseState, action, baseGlobalState))).toEqual(Immutable.Map([ [123, Immutable.Map([['foo', [1, 2]]])], ])); }); + test("when removing, don't touch unaffected topics or streams", () => { + const message = eg.streamMessage({ stream_id: 123, subject: 'qux', id: 7 }); + const state = reducer(baseState, mkMessageAction(message), baseGlobalState); + const globalState = eg.reduxStatePlus({ + messages: eg.makeMessagesState([...messages, message]), + unread: state, + }); + + // prettier-ignore + expect(summary(state)).toEqual(Immutable.Map([ + [123, Immutable.Map([['foo', [1, 2, 3]], ['qux', [7]]])], + [234, Immutable.Map([['bar', [4, 5]]])], + ])); + + const action = mkAction({ messages: [1, 2] }); + const newState = reducer(state, action, globalState); + // prettier-ignore + expect(summary(newState)).toEqual(Immutable.Map([ + [123, Immutable.Map([['foo', [3]], ['qux', [7]]])], + [234, Immutable.Map([['bar', [4, 5]]])], + ])); + expect(newState.streams.get(123)?.get('qux')).toBe(state.streams.get(123)?.get('qux')); + expect(newState.streams.get(234)).toBe(state.streams.get(234)); + }); + test('when operation is "remove" do nothing', () => { const action = mkAction({ messages: [1, 2], operation: 'remove' }); - expect(reducer(baseState, action, eg.plusReduxState)).toBe(baseState); + expect(reducer(baseState, action, baseGlobalState)).toBe(baseState); }); test('when "all" is true reset state', () => { const action = mkAction({ messages: [], all: true }); - expect(reducer(baseState, action, eg.plusReduxState).streams).toBe(initialState.streams); + expect(reducer(baseState, action, baseGlobalState).streams).toBe(initialState.streams); }); }); }); diff --git a/src/unread/unreadHelpers.js b/src/unread/unreadHelpers.js index 72ae97fbf8f..1fa8635b4f2 100644 --- a/src/unread/unreadHelpers.js +++ b/src/unread/unreadHelpers.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import type { HuddlesUnreadItem, PmsUnreadItem, StreamUnreadItem, UserId } from '../types'; +import type { HuddlesUnreadItem, PmsUnreadItem, UserId } from '../types'; import { addItemsToArray, removeItemsFromArray, filterArray } from '../utils/immutability'; type SomeUnreadItem = { unread_message_ids: number[], ... }; @@ -86,24 +86,3 @@ export const addItemsToHuddleArray = ( }, ]; }; - -export const addItemsToStreamArray = ( - input: StreamUnreadItem[], - itemsToAdd: number[], - streamId: number, - topic: string, -): StreamUnreadItem[] => { - const index = input.findIndex(s => s.stream_id === streamId && s.topic === topic); - - if (index !== -1) { - return addItemsDeeply(input, itemsToAdd, index); - } - return [ - ...input, - { - stream_id: streamId, - topic, - unread_message_ids: itemsToAdd, - }, - ]; -}; diff --git a/src/unread/unreadModel.js b/src/unread/unreadModel.js index 9c8878b1c1d..b81f048b5c6 100644 --- a/src/unread/unreadModel.js +++ b/src/unread/unreadModel.js @@ -1,18 +1,33 @@ /* @flow strict-local */ +import Immutable from 'immutable'; import type { Action } from '../actionTypes'; import type { UnreadState, + UnreadStreamsState, UnreadPmsState, UnreadHuddlesState, UnreadMentionsState, - UnreadStreamsState, } from './unreadModelTypes'; import type { GlobalState } from '../reduxTypes'; -import unreadStreamsReducer from './unreadStreamsReducer'; import unreadPmsReducer from './unreadPmsReducer'; import unreadHuddlesReducer from './unreadHuddlesReducer'; import unreadMentionsReducer from './unreadMentionsReducer'; +import { + ACCOUNT_SWITCH, + EVENT_MESSAGE_DELETE, + EVENT_NEW_MESSAGE, + EVENT_UPDATE_MESSAGE_FLAGS, + LOGOUT, + MESSAGE_FETCH_COMPLETE, + REALM_INIT, +} from '../actionConstants'; +import { getOwnUserId } from '../users/userSelectors'; + +// +// +// Selectors. +// export const getUnreadStreams = (state: GlobalState): UnreadStreamsState => state.unread.streams; @@ -22,13 +37,275 @@ export const getUnreadHuddles = (state: GlobalState): UnreadHuddlesState => stat export const getUnreadMentions = (state: GlobalState): UnreadMentionsState => state.unread.mentions; +// +// +// Reducer. +// + +const initialStreamsState: UnreadStreamsState = Immutable.Map(); + +// Like `Immutable.Map#update`, but prune returned values equal to `zero`. +function updateAndPrune( + map: Immutable.Map, + zero: V, + key: K, + updater: (V | void) => V, +): Immutable.Map { + const value = map.get(key); + const newValue = updater(value); + if (newValue === zero) { + return map.delete(key); + } + if (newValue === value) { + return map; + } + return map.set(key, newValue); +} + +/** + * Remove the given values from the list. + * + * This is equivalent to + * list_.filter(x => toDelete.indexOf(x) < 0) + * but more efficient. + * + * Specifically, for n items in the list and k to delete, this takes time + * O(n log n) in the worst case. + * + * In the case where the items to delete all appear at the beginning of the + * list, and in the same order, it takes time O(k log n). (This is the + * common case when marking messages as read, which motivates this + * optimization.) + */ +// In principle this should be doable in time O(k + log n) in the +// all-at-start case. We'd need the iterator on Immutable.List to support +// iterating through the first k elements in O(k + log n) time. It seems +// like it should be able to do that, but the current implementation (as of +// Immutable 4.0.0-rc.12) takes time O(k log n): each step of the iterator +// passes through a stack of log(n) helper functions. Ah well. +// +// The logs are base 32, so in practice our log(n) is never more than 3 +// (which would be enough for 32**3 = 32768 items), usually at most 2 +// (enough for 1024 items); and for the messages in one conversation, very +// commonly 1, i.e. there are commonly just ≤32 messages. So the difference +// between O(k log n) and O(k + log n) might be noticeable but is unlikely +// to be catastrophic. +function deleteFromList( + list_: Immutable.List, + toDelete_: Immutable.List, +): Immutable.List { + // Alias the parameters because Flow doesn't accept mutating them. + let list = list_; + let toDelete = toDelete_; + + // First, see if some items to delete happen to be at the start, and + // remove those. This is the common case for marking messages as read, + // so it's worth some effort to optimize. And we can do it efficiently: + // for deleting the first k out of n messages, we take time O(k log n) + // rather than O(n). + + const minSize = Math.min(list.size, toDelete.size); + let i = 0; + for (; i < minSize; i++) { + // This loop takes time O(log n) per iteration, O(k log n) total. + if (list.get(i) !== toDelete.get(i)) { + break; + } + } + + if (i > 0) { + // This takes time O(log n). + list = list.slice(i); + // This takes time O(log k) ≤ O(log n). + toDelete = toDelete.slice(i); + } + + // That might have been all the items we wanted to delete. + // In fact that's the most common case when marking items as read. + if (toDelete.isEmpty()) { + return list; + } + + // It wasn't; we have more to delete. We'll have to find them in the + // middle of the list and delete them wherever they are. + // + // This takes time O(n log n), probably (though an ideal implementation of + // Immutable should be able to make it O(n).) + const toDeleteSet = new Set(toDelete); + return list.filterNot(id => toDeleteSet.has(id)); +} + +const emptyList = Immutable.List(); + +/** + * Remove the given values, given where to find them; and prune. + * + * That is, for each entry in `toDelete`, we apply `deleteFromList` with the + * given list to the corresponding entry in `state`. When the resulting + * list is empty, we prune that entry entirely. + */ +function deleteFromListMap( + state: Immutable.Map>, + toDelete: Immutable.Map>, +): Immutable.Map> { + // prettier-ignore + return state.withMutations(mut => { + toDelete.forEach((msgIds, key) => { + updateAndPrune(mut, emptyList, key, list => + list && deleteFromList(list, msgIds)); + }); + }); +} + +/** + * Delete the given messages from the unreads state. + * + * Relies on `globalMessages` to look up exactly where in the unreads data + * structure the messages are expected to appear. + * + * This is efficient at deleting some messages even when the total number of + * existing messages is much larger. Specifically the time spent should be + * O(N' log n + c log C), where the messages to delete appear in c out of a + * total of C conversations, and the affected conversations have a total of + * N' messages and at most n in any one conversation. If the messages to be + * deleted are all at the start of the list for their respective + * conversations the time should be O(k log n + c log C), where there are + * k messages to delete. + * + * For the common case of marking some messages as read, we expect that all + * the affected messages will indeed be at the start of their respective + * conversations, and the number c of affected conversations will be small, + * typically 1. (It could be more than 1 if reading a stream narrow, or + * other interleaved narrow.) + */ +function deleteMessages( + state: UnreadStreamsState, + ids: $ReadOnlyArray, + globalMessages, +): UnreadStreamsState { + const byConversation = + // prettier-ignore + (Immutable.Map(): Immutable.Map>>) + .withMutations(mut => { + for (const id of ids) { + const message = globalMessages.get(id); + if (!message || message.type !== 'stream') { + continue; + } + const { stream_id, subject: topic } = message; + mut.updateIn([stream_id, topic], (l = Immutable.List()) => l.push(id)); + } + }); + + const emptyMap = Immutable.Map(); + // prettier-ignore + return state.withMutations(stateMut => { + byConversation.forEach((byTopic, streamId) => { + updateAndPrune(stateMut, emptyMap, streamId, perStream => + perStream && deleteFromListMap(perStream, byTopic), + ); + }); + }); +} + +function streamsReducer( + state: UnreadStreamsState = initialStreamsState, + action: Action, + globalState: GlobalState, +): UnreadStreamsState { + switch (action.type) { + case LOGOUT: + case ACCOUNT_SWITCH: + // TODO(#4446) also LOGIN_SUCCESS, presumably + return initialStreamsState; + + case REALM_INIT: { + // This may indeed be unnecessary, but it's legacy; have not investigated + // if it's this bit of our API types that is too optimistic. + // flowlint-next-line unnecessary-optional-chain:off + const data = action.data.unread_msgs?.streams ?? []; + + // First, collect together all the data for a given stream, just in a + // plain old Array. + const byStream = new Map(); + for (const { stream_id, topic, unread_message_ids } of data) { + let perStream = byStream.get(stream_id); + if (!perStream) { + perStream = []; + byStream.set(stream_id, perStream); + } + // unread_message_ids is already sorted; see comment at its + // definition in src/api/initialDataTypes.js. + perStream.push([topic, Immutable.List(unread_message_ids)]); + } + + // Then, for each of those plain Arrays build an Immutable.Map from it + // all in one shot. This is quite a bit faster than building the Maps + // incrementally. For a user with lots of unreads in a busy org, we + // can be handling 50k message IDs here, across perhaps 2-5k threads + // in dozens of streams, so the effect is significant. + return Immutable.Map(Immutable.Seq.Keyed(byStream.entries()).map(Immutable.Map)); + } + + case MESSAGE_FETCH_COMPLETE: + // TODO handle MESSAGE_FETCH_COMPLETE here. This rarely matters, but + // could in principle: we could be fetching some messages from + // before the (long) window included in the initial unreads data. + // For comparison, the webapp does handle this case; see the call to + // message_util.do_unread_count_updates in message_fetch.js. + return state; + + case EVENT_NEW_MESSAGE: { + const { message } = action; + if (message.type !== 'stream') { + return state; + } + + if (message.sender_id === getOwnUserId(globalState)) { + return state; + } + + // prettier-ignore + return state.updateIn([message.stream_id, message.subject], + (perTopic = Immutable.List()) => perTopic.push(message.id)); + } + + case EVENT_MESSAGE_DELETE: + return deleteMessages(state, action.messageIds, globalState.messages); + + case EVENT_UPDATE_MESSAGE_FLAGS: { + if (action.flag !== 'read') { + return state; + } + + if (action.all) { + return initialStreamsState; + } + + if (action.operation === 'remove') { + // Zulip doesn't support un-reading a message. Ignore it. + return state; + } + + return deleteMessages(state, action.messages, globalState.messages); + } + + default: + return state; + } +} + export const reducer = ( state: void | UnreadState, action: Action, globalState: GlobalState, ): UnreadState => { const nextState = { - streams: unreadStreamsReducer(state?.streams, action, globalState), + streams: streamsReducer(state?.streams, action, globalState), + + // Note for converting these other sub-reducers to the new design: + // Probably first push this four-part data structure down through the + // `switch` statement, and the other logic that's duplicated between them. pms: unreadPmsReducer(state?.pms, action), huddles: unreadHuddlesReducer(state?.huddles, action), mentions: unreadMentionsReducer(state?.mentions, action), diff --git a/src/unread/unreadModelTypes.js b/src/unread/unreadModelTypes.js index e495bce9203..0af4f24ad32 100644 --- a/src/unread/unreadModelTypes.js +++ b/src/unread/unreadModelTypes.js @@ -1,9 +1,12 @@ /* @flow strict-local */ +import Immutable from 'immutable'; -import type { HuddlesUnreadItem, PmsUnreadItem, StreamUnreadItem } from '../api/apiTypes'; +import type { HuddlesUnreadItem, PmsUnreadItem } from '../api/apiTypes'; // These four are fragments of UnreadState; see below. -export type UnreadStreamsState = StreamUnreadItem[]; +// prettier-ignore +export type UnreadStreamsState = + Immutable.Map>>; export type UnreadHuddlesState = HuddlesUnreadItem[]; export type UnreadPmsState = PmsUnreadItem[]; export type UnreadMentionsState = number[]; diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 66569154906..d2e28b2c867 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -10,7 +10,7 @@ import { isTopicMuted } from '../utils/message'; import { caseNarrow } from '../utils/narrow'; import { NULL_SUBSCRIPTION } from '../nullObjects'; import { pmUnreadsKeyFromPmKeyIds } from '../utils/recipient'; -import { getUnreadStreams, getUnreadPms, getUnreadHuddles, getUnreadMentions } from './unreadModel'; +import { getUnreadPms, getUnreadHuddles, getUnreadMentions, getUnreadStreams } from './unreadModel'; /** The number of unreads in each stream, excluding muted topics, by stream ID. */ export const getUnreadByStream: Selector<{| [number]: number |}> = createSelector( @@ -19,17 +19,18 @@ export const getUnreadByStream: Selector<{| [number]: number |}> = createSelecto getMute, (unreadStreams, subscriptionsById, mute) => { const totals = ({}: {| [number]: number |}); - unreadStreams.forEach(stream => { - if (!totals[stream.stream_id]) { - totals[stream.stream_id] = 0; + for (const [streamId, streamData] of unreadStreams.entries()) { + let total = 0; + for (const [topic, msgIds] of streamData) { + const isMuted = isTopicMuted( + (subscriptionsById.get(streamId) || NULL_SUBSCRIPTION).name, + topic, + mute, + ); + total += isMuted ? 0 : msgIds.size; } - const isMuted = isTopicMuted( - (subscriptionsById.get(stream.stream_id) || NULL_SUBSCRIPTION).name, - stream.topic, - mute, - ); - totals[stream.stream_id] += isMuted ? 0 : stream.unread_message_ids.length; - }); + totals[streamId] = total; + } return totals; }, ); @@ -130,38 +131,37 @@ export const getUnreadStreamsAndTopics: Selector = createSel getMute, (subscriptionsById, unreadStreams, mute) => { const totals = new Map(); - unreadStreams.forEach(stream => { + for (const [streamId, streamData] of unreadStreams.entries()) { const { name, color, in_home_view, invite_only, pin_to_top } = - subscriptionsById.get(stream.stream_id) || NULL_SUBSCRIPTION; + subscriptionsById.get(streamId) || NULL_SUBSCRIPTION; - let total = totals.get(stream.stream_id); - if (!total) { - total = { - key: `stream:${name}`, - streamName: name, - isMuted: !in_home_view, - isPrivate: invite_only, - isPinned: pin_to_top, - color, - unread: 0, - data: [], - }; - totals.set(stream.stream_id, total); - } + const total = { + key: `stream:${name}`, + streamName: name, + isMuted: !in_home_view, + isPrivate: invite_only, + isPinned: pin_to_top, + color, + unread: 0, + data: [], + }; + totals.set(streamId, total); - const isMuted = !mute.every(x => x[0] !== name || x[1] !== stream.topic); - if (!isMuted) { - total.unread += stream.unread_message_ids.length; - } + for (const [topic, msgIds] of streamData) { + const isMuted = !mute.every(x => x[0] !== name || x[1] !== topic); + if (!isMuted) { + total.unread += msgIds.size; + } - total.data.push({ - key: stream.topic, - topic: stream.topic, - unread: stream.unread_message_ids.length, - lastUnreadMsgId: stream.unread_message_ids[stream.unread_message_ids.length - 1], - isMuted, - }); - }); + total.data.push({ + key: topic, + topic, + unread: msgIds.size, + lastUnreadMsgId: msgIds.last(), + isMuted, + }); + } + } const sortedStreams = Array.from(totals.values()) .sort((a, b) => caseInsensitiveCompareFunc(a.streamName, b.streamName)) @@ -222,10 +222,8 @@ export const getUnreadCountForNarrow: Selector = createSelector( state => getUnreadHuddles(state), state => getUnreadPms(state), state => getMute(state), - (narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) => { - const sumLengths = unreads => unreads.reduce((sum, x) => sum + x.unread_message_ids.length, 0); - - return caseNarrow(narrow, { + (narrow, streams, ownUserId, unreadTotal, unreadStreams, unreadHuddles, unreadPms, mute) => + caseNarrow(narrow, { home: () => unreadTotal, stream: name => { @@ -233,10 +231,15 @@ export const getUnreadCountForNarrow: Selector = createSelector( if (!stream) { return 0; } - return sumLengths( - unreadStreams.filter( - x => x.stream_id === stream.stream_id && !isTopicMuted(name, x.topic, mute), - ), + // prettier-ignore + return ( + unreadStreams + .get(stream.stream_id) + ?.entrySeq() + .filterNot(([topic, _]) => isTopicMuted(name, topic, mute)) + .map(([_, msgIds]) => msgIds.size) + .reduce((s, x) => s + x, 0) + ?? 0 ); }, @@ -245,9 +248,7 @@ export const getUnreadCountForNarrow: Selector = createSelector( if (!stream) { return 0; } - return sumLengths( - unreadStreams.filter(x => x.stream_id === stream.stream_id && x.topic === topic), - ); + return unreadStreams.get(stream.stream_id)?.get(topic)?.size ?? 0; }, pm: ids => { @@ -277,6 +278,5 @@ export const getUnreadCountForNarrow: Selector = createSelector( // because we never use this selector for that narrow (because we // don't expose it as one you can narrow to in the UI.) allPrivate: () => 0, - }); - }, + }), ); diff --git a/src/unread/unreadStreamsReducer.js b/src/unread/unreadStreamsReducer.js deleted file mode 100644 index e36d1bf0884..00000000000 --- a/src/unread/unreadStreamsReducer.js +++ /dev/null @@ -1,78 +0,0 @@ -/* @flow strict-local */ -import type { Action, GlobalState } from '../types'; -import type { UnreadStreamsState } from './unreadModelTypes'; -import { - REALM_INIT, - LOGOUT, - ACCOUNT_SWITCH, - EVENT_NEW_MESSAGE, - EVENT_MESSAGE_DELETE, - EVENT_UPDATE_MESSAGE_FLAGS, -} from '../actionConstants'; -import { addItemsToStreamArray, removeItemsDeeply } from './unreadHelpers'; -import { NULL_ARRAY } from '../nullObjects'; -import { getOwnUserId } from '../users/userSelectors'; - -const initialState: UnreadStreamsState = NULL_ARRAY; - -const eventNewMessage = (state, action, globalState) => { - if (action.message.type !== 'stream') { - return state; - } - - if (getOwnUserId(globalState) === action.message.sender_id) { - return state; - } - - return addItemsToStreamArray( - state, - [action.message.id], - action.message.stream_id, - action.message.subject, - ); -}; - -const eventUpdateMessageFlags = (state, action) => { - if (action.flag !== 'read') { - return state; - } - - if (action.all) { - return initialState; - } - - if (action.operation === 'add') { - return removeItemsDeeply(state, action.messages); - } else if (action.operation === 'remove') { - // we do not support that operation - } - - return state; -}; - -export default ( - state: UnreadStreamsState = initialState, - action: Action, - globalState: GlobalState, -): UnreadStreamsState => { - switch (action.type) { - case LOGOUT: - case ACCOUNT_SWITCH: - return initialState; - - case REALM_INIT: - return (action.data.unread_msgs && action.data.unread_msgs.streams) || initialState; - - case EVENT_NEW_MESSAGE: - return eventNewMessage(state, action, globalState); - - case EVENT_MESSAGE_DELETE: - return removeItemsDeeply(state, action.messageIds); - - case EVENT_UPDATE_MESSAGE_FLAGS: - return eventUpdateMessageFlags(state, action); - - default: - return state; - } -};