unread: Convert unread.streams to Immutable, and update efficiently

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<number, string> = 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

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`.
 };
 

src/topics/topicSelectors.js

@@ -52,15 +52,8 @@ export const getTopicsForStream: Selector<?(TopicExtended[]), number> = 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 };
     });
   },
 );

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<number, Immutable.Map<string, number[]>> =
-      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);
     });
   });
 });

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,
-    },
-  ];
-};