Check active account, too, in getHaveServerData

src/account/AccountItem.js

@@ -51,7 +51,12 @@ type Props = $ReadOnly<{|
 export default function AccountItem(props: Props): Node {
   const { email, realm, isLoggedIn } = props.account;
 
+  // Don't show the "remove account" button (the "trash" icon) for the
+  // active account when it's logged in.  This prevents removing it when the
+  // main app UI, relying on that account's data, may be on the nav stack.
+  // See `getHaveServerData`.
   const showDoneIcon = props.index === 0 && isLoggedIn;
+
   const backgroundItemColor = isLoggedIn ? 'hsla(177, 70%, 47%, 0.1)' : 'hsla(0,0%,50%,0.1)';
   const textColor = isLoggedIn ? BRAND_COLOR : 'gray';
 

src/users/userSelectors.js

@@ -3,6 +3,7 @@ import { createSelector } from 'reselect';
 
 import type { GlobalState, UserOrBot, Selector, User, UserId } from '../types';
 import { getUsers, getCrossRealmBots, getNonActiveUsers } from '../directSelectors';
+import { getHasAuth, tryGetActiveAccount } from '../account/accountsSelectors';
 
 /**
  * All users in this Zulip org (aka realm).
@@ -201,16 +202,60 @@ export const getHaveServerData = (state: GlobalState): boolean => {
   // time the app ran.  In particular, if the user switched accounts (so
   // that we cleared server data in Redux) and then the app promptly
   // crashed, or was killed, that clearing-out may have reached some
-  // subtrees but not others.  See #4587.
+  // subtrees but not others.  See #4587 for an example, and #4841 overall.
+
+  // It's important that we never stick around in a state where we're trying
+  // to show the main UI but this function returns false.  When in that
+  // state, we just show a loading screen with no UI, so there has to be
+  // something happening in the background that will get us out of it.
+  //
+  // The basic strategy is:
+  //  * When we start showing the main UI, we always kick off an initial
+  //    fetch.  Specifically:
+  //    * If at startup (upon rehydrate) we show the main UI, we do so.
+  //      This is controlled by `getInitialRouteInfo`, together with
+  //      `sessionReducer` as it sets `needsInitialFetch`.
+  //    * When we navigate to the main UI (via `resetToMainTabs`), we always
+  //      also dispatch an action that causes `needsInitialFetch` to be set.
+  //    * Plus, that initial fetch has a timeout, so it will always take us
+  //      away from a loading screen regardless of server/network behavior.
+  //
+  //  * When we had server data and we stop having it, we always also either
+  //    navigate away from the main UI, or kick off a new initial fetch.
+  //    Specifically:
+  //    * Between this function and the reducers, we should only stop having
+  //      server data upon certain actions in `accountActions`.
+  //    * Some of those actions cause `needsInitialFetch` to be set, as above.
+  //    * Those that don't should always be accompanied by navigating away
+  //      from the main UI, with `resetToAccountPicker`.
+  //
+  // Ideally the decisions "should we show the loading screen" and "should
+  // we kick off a fetch" would be made together in one place, so that it'd
+  // be possible to confirm they align without so much nonlocal reasoning.
+
+  // Specific facts used in the reasoning below (within the strategy above):
+  //  * The actions LOGIN_SUCCESS and ACCOUNT_SWITCH cause
+  //    `needsInitialFetch` to be set.
+  //  * The action LOGOUT is always accompanied by navigating away from the
+  //    main UI.
+  //  * A successful initial fetch causes a REALM_INIT action.  A failed one
+  //    causes either LOGOUT, or an abort that ensures we're not at a
+  //    loading screen.
 
   // Valid server data must have a user: the self user, at a minimum.
   if (getUsers(state).length === 0) {
+    // From `usersReducer`:
+    //  * This condition is resolved by REALM_INIT.
+    //  * It's created only by LOGIN_SUCCESS, LOGOUT, and ACCOUNT_SWITCH.
     return false;
   }
 
   // It must also have the self user's user ID.
   const ownUserId = state.realm.user_id;
   if (ownUserId === undefined) {
+    // From `realmReducer`:
+    //  * This condition is resolved by REALM_INIT.
+    //  * It's created only by LOGIN_SUCCESS, LOGOUT, and ACCOUNT_SWITCH.
     return false;
   }
 
@@ -222,9 +267,52 @@ export const getHaveServerData = (state: GlobalState): boolean => {
   // then this check would fire.  And in that situation without this check,
   // we crash early on because `getOwnUser` fails.)
   if (!getUsersById(state).get(ownUserId)) {
+    // From the reducers (and assumptions about the server's data):
+    //  * This condition is resolved by REALM_INIT.
+    //  * It's never created (post-rehydrate.)
+    return false;
+  }
+
+  // Any valid server data is about the active account.  So if there is no
+  // active account, then any server data we appear to have can't be valid.
+  if (!tryGetActiveAccount(state)) {
+    // From `accountsReducer`:
+    //  * This condition is resolved by LOGIN_SUCCESS.
+    //  * It's created only by ACCOUNT_REMOVE.
+    //
+    // When this condition applies, LOGIN_SUCCESS is the only way we might
+    // navigate to the main UI.
+    //
+    // ACCOUNT_REMOVE is available only from the account-picker screen (not
+    // the main UI), and moreover is available for the active account only
+    // when not logged in, in which case the main UI can't be on the
+    // navigation stack either.
+    return false;
+  }
+
+  // Similarly, any valid server data comes from the active account being
+  // logged in.
+  if (!getHasAuth(state)) {
+    // From `accountsReducer`:
+    //  * This condition is resolved by LOGIN_SUCCESS.
+    //  * It's created only by ACCOUNT_REMOVE, by LOGOUT, and by (a
+    //    hypothetical) ACCOUNT_SWITCH for a logged-out account.
+    //
+    // When this condition applies, LOGIN_SUCCESS is the only way we might
+    // navigate to the main UI.
+    //
+    // For ACCOUNT_REMOVE, see the previous condition.
+    // ACCOUNT_SWITCH we only do for logged-in accounts.
     return false;
   }
 
+  // TODO: A nice bonus would be to check that the account matches the
+  // server data, given any of:
+  //  * user ID in `Account` (#4951)
+  //  * realm URL in `RealmState`
+  //  * delivery email in `RealmState` and/or `User` (though not sure this
+  //    is available from server, even for self, in all configurations)
+
   // Any other subtree could also have been emptied while others weren't,
   // or otherwise be out of sync.
   //
@@ -240,8 +328,9 @@ export const getHaveServerData = (state: GlobalState): boolean => {
   // that `state.messages` doesn't, or `state.messages` have a message sent
   // by a user that `state.users` has no record of.
   //
-  // But given that shortly after startup we go fetch fresh data from the
-  // server anyway, the checks above are hopefully enough to let the app
-  // survive that long.
+  // But given that shortly after starting to show the main app UI (whether
+  // that's at startup, or after picking an account or logging in) we go
+  // fetch fresh data from the server anyway, the checks above are hopefully
+  // enough to let the app survive that long.
   return true;
 };