per-account 6/n: Stop letting GlobalState cast to PerAccountState

src/__tests__/lib/exampleData.js

@@ -24,12 +24,14 @@ import type {
   MessageFetchStartAction,
   MessageFetchCompleteAction,
   Action,
+  PerAccountState,
   GlobalState,
   CaughtUpState,
   MessagesState,
   RealmState,
 } from '../../reduxTypes';
 import type { Auth, Account, StreamOutbox } from '../../types';
+import { dubJointState } from '../../reduxTypes';
 import { UploadedAvatarURL } from '../../utils/avatar';
 import { ZulipVersion } from '../../utils/zulipVersion';
 import {
@@ -497,19 +499,30 @@ const privateReduxStore = createStore(rootReducer);
  * See `plusReduxState` for a version of the state that incorporates
  * `selfUser` and other standard example data.
  */
-export const baseReduxState: GlobalState = deepFreeze(privateReduxStore.getState());
+// TODO(#5006): Split this (and its friends below) into global and
+//   per-account versions.  (This may be easiest to do after actually
+//   migrating settings and session to split them per-account vs global, so
+//   that the global and per-account states have disjoint sets of
+//   properties.)
+// For now, the intersection type (with `&`) says this value can be used as
+// either kind of state.
+export const baseReduxState: GlobalState & PerAccountState = dubJointState(
+  deepFreeze(privateReduxStore.getState()),
+);
 
 /**
  * A global Redux state, with `baseReduxState` plus the given data.
  *
  * See `reduxStatePlus` for a version that automatically includes `selfUser`
  * and other standard example data.
  */
-export const reduxState = (extra?: $Rest<GlobalState, { ... }>): GlobalState =>
-  deepFreeze({
-    ...baseReduxState,
-    ...extra,
-  });
+export const reduxState = (extra?: $Rest<GlobalState, { ... }>): GlobalState & PerAccountState =>
+  dubJointState(
+    deepFreeze({
+      ...(baseReduxState: GlobalState),
+      ...extra,
+    }),
+  );
 
 /**
  * The global Redux state, reflecting standard example data like `selfUser`.
@@ -541,7 +554,7 @@ export const reduxState = (extra?: $Rest<GlobalState, { ... }>): GlobalState =>
  *
  * See `baseReduxState` for a minimal version of the state.
  */
-export const plusReduxState: GlobalState = reduxState({
+export const plusReduxState: GlobalState & PerAccountState = reduxState({
   accounts: [
     {
       ...selfAuth,
@@ -567,8 +580,11 @@ export const plusReduxState: GlobalState = reduxState({
  *
  * See `reduxState` for a version starting from a minimal state.
  */
-export const reduxStatePlus = (extra?: $Rest<GlobalState, { ... }>): GlobalState =>
-  deepFreeze({ ...plusReduxState, ...extra });
+export const reduxStatePlus = (
+  extra?: $Rest<GlobalState, { ... }>,
+  // $FlowFixMe[not-an-object]
+): GlobalState & PerAccountState =>
+  dubJointState(deepFreeze({ ...(plusReduxState: GlobalState), ...extra }));
 
 export const realmState = (extra?: $Rest<RealmState, { ... }>): RealmState =>
   deepFreeze({

src/account/accountsSelectors.js

@@ -11,7 +11,7 @@ import type {
   Selector,
   GlobalSelector,
 } from '../types';
-import { assumeSecretlyGlobalState } from '../reduxTypes';
+import { dubPerAccountState, assumeSecretlyGlobalState } from '../reduxTypes';
 import { getAccounts } from '../directSelectors';
 import { identityOfAccount, keyOfIdentity, identityOfAuth, authOfAccount } from './accountMisc';
 import { ZulipVersion } from '../utils/zulipVersion';
@@ -67,7 +67,9 @@ export const getAccountsByIdentity: GlobalSelector<(Identity) => Account | void>
  */
 export const tryGetActiveAccountState = (state: GlobalState): PerAccountState | void => {
   const accounts = getAccounts(state);
-  return accounts && accounts.length > 0 ? state : undefined;
+  // TODO(#5006): This is the inverse of where getAccount uses the same
+  //   assumption that the two state types are really the same objects.
+  return accounts && accounts.length > 0 ? dubPerAccountState(state) : undefined;
 };
 
 /**

src/boot/AppEventHandlers.js

@@ -7,6 +7,7 @@ import NetInfo from '@react-native-community/netinfo';
 import * as ScreenOrientation from 'expo-screen-orientation';
 
 import type { GlobalDispatch, Orientation as OrientationT } from '../types';
+import { dubPerAccountState } from '../reduxTypes';
 import { createStyleSheet } from '../styles';
 import { connectGlobal } from '../react-redux';
 import { getUnreadByHuddlesMentionsAndPMs } from '../selectors';
@@ -168,7 +169,11 @@ class AppEventHandlersInner extends PureComponent<Props> {
 }
 
 const AppEventHandlers: ComponentType<OuterProps> = connectGlobal(state => ({
-  unreadCount: getUnreadByHuddlesMentionsAndPMs(state),
+  // TODO(#5006): The use of this per-account state in this global component
+  //   highlights how this feature (a badge count based on unreads, on
+  //   Android only) is pretty broken if you use multiple accounts -- it
+  //   reflects only the one last account you used.  Maybe just cut it?
+  unreadCount: getUnreadByHuddlesMentionsAndPMs(dubPerAccountState(state)),
 }))(AppEventHandlersInner);
 
 export default AppEventHandlers;

src/boot/reducers.js

@@ -102,6 +102,7 @@ export default (state: void | GlobalState, action: Action): GlobalState => {
     subscriptions: applyReducer('subscriptions', subscriptions, state?.subscriptions, action, state),
     topics: applyReducer('topics', topics, state?.topics, action, state),
     typing: applyReducer('typing', typing, state?.typing, action, state),
+    // $FlowFixMe[incompatible-call] TODO(#5006)
     unread: applyReducer('unread', unread, state?.unread, action, state),
     userGroups: applyReducer('userGroups', userGroups, state?.userGroups, action, state),
     userStatus: applyReducer('userStatus', userStatus, state?.userStatus, action, state),

src/events/eventActions.js

@@ -55,7 +55,7 @@ export const startEventPolling = (
   while (true) {
     const globalState = assumeSecretlyGlobalState(getState());
     const state = tryGetActiveAccountState(globalState);
-    const auth = state && tryGetAuth(state);
+    const auth = state ? tryGetAuth(state) : undefined;
     if (!auth) {
       // There is no logged-in active account.
       break;

src/notification/index.js

@@ -26,6 +26,7 @@ import { getAccounts } from '../directSelectors';
  * Identify the account the notification is for, if possible.
  *
  * Returns an index into `identities`, or `null` if we can't tell.
+ * In the latter case, logs a warning.
  *
  * @param identities Identities corresponding to the accounts state in Redux.
  */

src/notification/notificationActions.js

@@ -25,7 +25,7 @@ import { identityOfAccount, authOfAccount } from '../account/accountMisc';
 import { getAllUsersByEmail, getOwnUserId } from '../users/userSelectors';
 import { doNarrow } from '../message/messagesActions';
 import { accountSwitch } from '../account/accountActions';
-import { getIdentities, getAccount } from '../account/accountsSelectors';
+import { getIdentities, getAccount, tryGetActiveAccountState } from '../account/accountsSelectors';
 
 export const gotPushToken = (pushToken: string | null): Action => ({
   type: GOT_PUSH_TOKEN,
@@ -51,15 +51,27 @@ export const narrowToNotification = (data: ?Notification): GlobalThunkAction<voi
     return;
   }
 
-  const state = getState();
-  const accountIndex = getAccountFromNotificationData(data, getIdentities(state));
+  const globalState = getState();
+  const accountIndex = getAccountFromNotificationData(data, getIdentities(globalState));
   if (accountIndex !== null && accountIndex > 0) {
     // Notification is for a non-active account.  Switch there.
     dispatch(accountSwitch(accountIndex));
     // TODO actually narrow to conversation.
     return;
   }
 
+  // If accountIndex is null, then `getAccountFromNotificationData` has
+  // already logged a warning.  We go on to treat it as if it's 0, i.e. the
+  // active account, in the hopes that the user has one account they use
+  // regularly and it's the same one this notification is for.
+
+  const state = tryGetActiveAccountState(globalState);
+  if (!state) {
+    // There are no accounts at all.  (Which also means accountIndex is null
+    // and we've already logged a warning.)
+    return;
+  }
+
   const narrow = getNarrowFromNotificationData(
     data,
     getAllUsersByEmail(state),

src/reduxTypes.js

@@ -405,7 +405,7 @@ export type UsersState = $ReadOnlyArray<User>;
  * parts of our code will in that future operate on a particular account and
  * which parts will operate on all accounts' data or none.
  */
-export type PerAccountState = $ReadOnly<{
+type PerAccountStateImpl = $ReadOnly<{
   // TODO(#5006): Secretly we assume these objects also have `Account` data,
   //   like so:
   // accounts: [Account, ...mixed],
@@ -445,6 +445,8 @@ export type PerAccountState = $ReadOnly<{
   ...
 }>;
 
+export opaque type PerAccountState: PerAccountStateImpl = PerAccountStateImpl;
+
 /**
  * Our complete Redux state tree.
  *
@@ -474,10 +476,6 @@ export type GlobalState = $ReadOnly<{|
   accounts: AccountsState,
 |}>;
 
-// For now, under our single-active-account model, we want a GlobalState
-// to be seamlessly usable as a PerAccountState.
-(s: GlobalState): PerAccountState => s; // eslint-disable-line no-unused-expressions
-
 /**
  * Assume the given per-account state object is secretly a GlobalState.
  *
@@ -493,6 +491,28 @@ export function assumeSecretlyGlobalState(state: PerAccountState): GlobalState {
   return state;
 }
 
+/**
+ * Use the given state object as a per-account state.
+ *
+ * TODO(#5006): We'll have to fix and eliminate each call to this.
+ */
+export function dubPerAccountState(state: GlobalState): PerAccountState {
+  // Here in this file, we can make this cast with no fixmes (for now, under
+  // our single-active-account model.)  But from anywhere outside this file,
+  // that's forbidden because PerAccountState is opaque, so the way to do it
+  // is by calling this function.
+  return state;
+}
+
+/**
+ * For tests only.  Use the given state object as *both* kinds of state.
+ *
+ * TODO(#5006): We'll have to fix and eliminate each call to this.
+ */
+export function dubJointState(state: GlobalState): GlobalState & PerAccountState {
+  return state;
+}
+
 // No substate should allow `undefined`; our use of AsyncStorage
 // depends on it. (This check will also complain on `null`, which I
 // don't think we'd have a problem with. We could try to write this