diff --git a/src/account/AccountItem.js b/src/account/AccountItem.js index 459f2efa57d..e1a3a0d9e73 100644 --- a/src/account/AccountItem.js +++ b/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'; diff --git a/src/users/userSelectors.js b/src/users/userSelectors.js index 7d2fa5d4532..8070092de07 100644 --- a/src/users/userSelectors.js +++ b/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; };