From 38cb1dda2ffa95f14334fecce423741b64c78533 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 17:56:59 -0700 Subject: [PATCH 01/21] nav state: Remove several unnecessary fixture details about nav state. --- src/message/__tests__/fetchActions-test.js | 9 ----- src/message/__tests__/messageActions-test.js | 4 +-- src/title/__tests__/titleSelectors-test.js | 5 --- src/utils/testHelpers.js | 35 -------------------- 4 files changed, 1 insertion(+), 52 deletions(-) delete mode 100644 src/utils/testHelpers.js diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index ec9113b97fc..071986d7a86 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -16,7 +16,6 @@ import { FIRST_UNREAD_ANCHOR } from '../../anchor'; import type { Message } from '../../api/modelTypes'; import type { ServerMessage } from '../../api/messages/getMessages'; import { streamNarrow, HOME_NARROW, HOME_NARROW_STR } from '../../utils/narrow'; -import { navStateWithNarrow } from '../../utils/testHelpers'; import * as eg from '../../__tests__/lib/exampleData'; const mockStore = configureStore([thunk]); @@ -41,7 +40,6 @@ describe('fetchActions', () => { older: true, }, }, - ...navStateWithNarrow(HOME_NARROW), narrows: { [streamNarrowStr]: [], }, @@ -64,7 +62,6 @@ describe('fetchActions', () => { older: false, }, }, - ...navStateWithNarrow(HOME_NARROW), narrows: { [streamNarrowStr]: [1], }, @@ -116,7 +113,6 @@ describe('fetchActions', () => { const message1 = eg.streamMessage({ id: 1 }); const baseState = eg.reduxState({ - ...navStateWithNarrow(HOME_NARROW), accounts: [eg.makeAccount()], narrows: { [streamNarrowStr]: [message1.id], @@ -295,7 +291,6 @@ describe('fetchActions', () => { test('when messages to be fetched both before and after anchor, numBefore and numAfter are greater than zero', async () => { const store = mockStore( eg.reduxState({ - ...navStateWithNarrow(HOME_NARROW), accounts: [eg.selfAccount], narrows: { [streamNarrowStr]: [1], @@ -322,7 +317,6 @@ describe('fetchActions', () => { test('when no messages to be fetched before the anchor, numBefore is not greater than zero', async () => { const store = mockStore( eg.reduxState({ - ...navStateWithNarrow(HOME_NARROW), accounts: [eg.selfAccount], narrows: { [streamNarrowStr]: [1], @@ -348,7 +342,6 @@ describe('fetchActions', () => { test('when no messages to be fetched after the anchor, numAfter is not greater than zero', async () => { const store = mockStore( eg.reduxState({ - ...navStateWithNarrow(HOME_NARROW), accounts: [eg.selfAccount], narrows: { [streamNarrowStr]: [1], @@ -377,7 +370,6 @@ describe('fetchActions', () => { const message2 = eg.streamMessage({ id: 2 }); const baseState = eg.reduxState({ - ...navStateWithNarrow(HOME_NARROW), accounts: [eg.selfAccount], narrows: { ...eg.baseReduxState.narrows, @@ -471,7 +463,6 @@ describe('fetchActions', () => { const message2 = eg.streamMessage({ id: 2 }); const baseState = eg.reduxState({ - ...navStateWithNarrow(HOME_NARROW), accounts: [eg.selfAccount], narrows: { ...eg.baseReduxState.narrows, diff --git a/src/message/__tests__/messageActions-test.js b/src/message/__tests__/messageActions-test.js index 213968e188d..406109aa2e2 100644 --- a/src/message/__tests__/messageActions-test.js +++ b/src/message/__tests__/messageActions-test.js @@ -3,8 +3,7 @@ import configureStore from 'redux-mock-store'; import thunk from 'redux-thunk'; import { doNarrow } from '../messagesActions'; -import { streamNarrow, HOME_NARROW } from '../../utils/narrow'; -import { navStateWithNarrow } from '../../utils/testHelpers'; +import { streamNarrow } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; import type { GlobalState } from '../../reduxTypes'; import type { Action } from '../../actionTypes'; @@ -20,7 +19,6 @@ describe('messageActions', () => { eg.reduxState({ accounts: [eg.selfAccount], session: { ...eg.baseReduxState.session, isHydrated: true }, - ...navStateWithNarrow(HOME_NARROW), streams: [eg.makeStream({ name: 'some stream' })], }), ); diff --git a/src/title/__tests__/titleSelectors-test.js b/src/title/__tests__/titleSelectors-test.js index 7b964df99bd..da3eaca83ed 100644 --- a/src/title/__tests__/titleSelectors-test.js +++ b/src/title/__tests__/titleSelectors-test.js @@ -2,14 +2,12 @@ import deepFreeze from 'deep-freeze'; import { DEFAULT_TITLE_BACKGROUND_COLOR, getTitleBackgroundColor } from '../titleSelectors'; import { groupNarrow, streamNarrow, privateNarrow } from '../../utils/narrow'; -import { defaultNav, otherNav } from '../../utils/testHelpers'; const subscriptions = [{ name: 'all', color: '#fff' }, { name: 'announce', color: '#000' }]; describe('getTitleBackgroundColor', () => { test('return default for screens other than chat, i.e narrow is undefined', () => { const state = deepFreeze({ - nav: otherNav, subscriptions, }); @@ -18,7 +16,6 @@ describe('getTitleBackgroundColor', () => { test('return stream color for stream and topic narrow', () => { const state = deepFreeze({ - nav: defaultNav, subscriptions, }); @@ -27,7 +24,6 @@ describe('getTitleBackgroundColor', () => { test('return null stream color for invalid stream or unknown subscriptions', () => { const state = deepFreeze({ - nav: defaultNav, subscriptions, }); @@ -36,7 +32,6 @@ describe('getTitleBackgroundColor', () => { test('return default for non topic/stream narrow', () => { const state = deepFreeze({ - nav: defaultNav, subscriptions, }); diff --git a/src/utils/testHelpers.js b/src/utils/testHelpers.js deleted file mode 100644 index e913b89283a..00000000000 --- a/src/utils/testHelpers.js +++ /dev/null @@ -1,35 +0,0 @@ -/* @flow strict-local */ -import type { Narrow } from '../types'; -import type { NavigationState } from '../reduxTypes'; - -export const navStateWithNarrow = (narrow: Narrow): {| nav: NavigationState |} => ({ - nav: { - key: 'StackRouterRoot', - index: 1, - isTransitioning: false, - routes: [ - { - key: 'id-1592948746166-1', - params: {}, - routeName: 'main', - }, - { - key: 'id-1592948746166-2', - routeName: 'chat', - params: { - narrow, - }, - }, - ], - }, -}); - -export const defaultNav = { - index: 0, - routes: [{ routeName: 'chat' }], -}; - -export const otherNav = { - index: 1, - routes: [{ routeName: 'main' }, { routeName: 'account' }], -}; From 3390f0033a75372a58dbeed51c82070ce32438ed Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 25 Sep 2020 14:15:31 -0700 Subject: [PATCH 02/21] NavigationService: Add, to start taking nav responsibility from Redux. Following a doc from React Navigation [1] that outlines a way to navigate without access to the `navigation` prop. The integration of React Navigation with our Redux setup has meant that we don't regularly use the `navigation` prop. We've gotten used to two essential functions being provided by Redux through `react-redux`, much like how we use Redux for other bits of state management: - getting information based on the navigation state - carrying out navigation actions It would be quite disruptive to suddenly start using the `navigation` prop everywhere (especially in hard-to-reach places like the message action sheet). This guide suggests a place we could go that turns out to be a useful stepping-stone. We take the main idea of the guide -- set up a React ref to the "app container" (the result of invoking `createAppContainer`) and call methods on that -- and make a few departures, in both the interface and the implementation. Notable differences in the interface: - Instead of a `navigate` function that takes `routeName` and `params`, we expose `dispatch`. This is `dispatch` from React Navigation's lexicon, not Redux's, but it will take the exact same set of actions as we create in navActions.js -- those action creators come to us, after all, straight from `StackActions` in react-navigation. - We also expose a way to get the current navigation state; this isn't suggested in the doc, but it's perfectly possible. We call it `getState` because that's a perfectly clear, logical name; it also happens to be familiar from the Redux world. And in the implementation: - The doc suggests that the ref gets set on the root navigator component; for us, that would be our invocation of `createStackNavigator` in AppNavigator.js. When one actually follows their instructions, though, the ref gets set to the "app container": the result of calling react-navigation's `createAppContainer` (passing the root nav to that). This is seen in logging, and also in the fact that the special, reserved `ref` attribute is set directly on the app container component. It's not too hard to see why the doc makes this mistake -- it went through 2.x to 3.x to 4.x without dramatic changes, and `createAppContainer` didn't exist in 2.x. So they probably just forgot to update it properly. - The ref isn't set on React Navigation's "app container" component, but rather on the "Redux container" component that react-navigation-redux-helpers has asked us to replace it with, with `createReduxContainer`. (see 99be838c2 for more on that replacement). We'll move to `createAppContainer` later in the process, and when we do so, we expect to make slight tweaks to the implementation to adapt. But we've confirmed that it'll still be quite possible to expose `getState` and `dispatch`. [2] [1] https://reactnavigation.org/docs/4.x/navigating-without-navigation-prop [2] See more discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/decouple.20nav.20from.20redux.20%28.23M3804%29/near/1025422. --- src/ZulipMobile.js | 3 +- src/nav/AppWithNavigation.js | 14 +++++++--- src/nav/NavigationService.js | 54 ++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 src/nav/NavigationService.js diff --git a/src/ZulipMobile.js b/src/ZulipMobile.js index 3e305a32c5b..c068aeeb9a5 100644 --- a/src/ZulipMobile.js +++ b/src/ZulipMobile.js @@ -11,6 +11,7 @@ import AppEventHandlers from './boot/AppEventHandlers'; import AppDataFetcher from './boot/AppDataFetcher'; import BackNavigationHandler from './nav/BackNavigationHandler'; import AppWithNavigation from './nav/AppWithNavigation'; +import NavigationService from './nav/NavigationService'; import './i18n/locale'; import { initializeSentry } from './sentry'; @@ -28,7 +29,7 @@ export default (): React$Node => ( - + diff --git a/src/nav/AppWithNavigation.js b/src/nav/AppWithNavigation.js index 45bd8b86ff9..5ca71a17cab 100644 --- a/src/nav/AppWithNavigation.js +++ b/src/nav/AppWithNavigation.js @@ -2,10 +2,16 @@ import { createReduxContainer } from 'react-navigation-redux-helpers'; -import { connect } from '../react-redux'; +import { connect } from 'react-redux'; import { getNav } from '../selectors'; import AppNavigator from './AppNavigator'; -export default connect(state => ({ - state: getNav(state), -}))(createReduxContainer(AppNavigator, 'root')); +// $FlowFixMe - should use a type-checked `connect` +export default connect( + state => ({ + state: getNav(state), + }), + null, + null, + { forwardRef: true }, +)((createReduxContainer(AppNavigator, 'root'): React$ComponentType<{||}>)); diff --git a/src/nav/NavigationService.js b/src/nav/NavigationService.js new file mode 100644 index 00000000000..bc20cf6a2b2 --- /dev/null +++ b/src/nav/NavigationService.js @@ -0,0 +1,54 @@ +/* @flow strict-local */ +import React from 'react'; +import type { + NavigationAction, + NavigationNavigatorProps, + NavigationState, + NavigationDispatch, + SupportedThemes, +} from 'react-navigation'; + +type ReduxContainerProps = { + ...$Diff, { navigation: mixed }>, + state: NavigationState, + dispatch: NavigationDispatch, + theme: SupportedThemes | 'no-preference', +}; + +// Should mimic return type of react-navigation-redux-helpers's +// `createReduxContainer`. +type ReduxContainer = React$Component; + +// TODO: This will eventually be a ref to the component instance +// created by React Navigation's `createAppContainer`, not +// react-navigation-redux-helpers's `createReduxContainer`. +const reduxContainerRef = React.createRef(); + +const getState = (): NavigationState => { + if (reduxContainerRef.current === null) { + throw new Error('Tried to use NavigationService before reduxContainerRef was set.'); + } + return ( + reduxContainerRef.current + // $FlowFixMe - how to tell Flow about this method? + .getCurrentNavigation().state + ); +}; + +const dispatch = (navigationAction: NavigationAction): boolean => { + if (reduxContainerRef.current === null) { + throw new Error('Tried to use NavigationService before reduxContainerRef was set.'); + } + return ( + reduxContainerRef.current + // $FlowFixMe - how to tell Flow about this method? + .getCurrentNavigation() + .dispatch(navigationAction) + ); +}; + +export default { + getState, + dispatch, + reduxContainerRef, +}; From 0e2cc0bfe9a5fbeef5c9376d3665135941467e9a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 25 Sep 2020 16:19:14 -0700 Subject: [PATCH 03/21] navSelectors: Dissolve `getCanGoBack`. Working toward #3804. We're about to reimplement most of the functions in navSelectors.js so that they grab navigation state from the recently added `NavigationService` instead of from `state.nav` in Redux. For now, it's convenient to keep sourcing data from `state.nav` in just a few places. In this case, the one call site of `getCanGoBack` (the `connect` wrapper for `BackNavigationHandler`) is placed such that it's not convenient for it to get navigation state from `NavigationService`. In particular, `NavigationService` relies on a ref being set -- to a component that's deeper down in the component hierarchy. If we ask `NavigationService` for the navigation state, that component won't have been constructed, the ref will come up empty, and we'll have an error. `BackNavigationHandler` can disappear as #3804 is completed; you can see how and why this is done in an upcoming commit. --- src/nav/BackNavigationHandler.js | 3 +-- src/nav/__tests__/navSelectors-test.js | 21 --------------------- src/nav/navSelectors.js | 2 -- 3 files changed, 1 insertion(+), 25 deletions(-) diff --git a/src/nav/BackNavigationHandler.js b/src/nav/BackNavigationHandler.js index 12965f31a86..e098d81255b 100644 --- a/src/nav/BackNavigationHandler.js +++ b/src/nav/BackNavigationHandler.js @@ -6,7 +6,6 @@ import { BackHandler } from 'react-native'; import type { Dispatch } from '../types'; import { connect } from '../react-redux'; -import { getCanGoBack } from '../selectors'; import { navigateBack } from '../actions'; type Props = $ReadOnly<{| @@ -38,5 +37,5 @@ class BackNavigationHandler extends PureComponent { } export default connect(state => ({ - canGoBack: getCanGoBack(state), + canGoBack: state.nav.index > 0, }))(BackNavigationHandler); diff --git a/src/nav/__tests__/navSelectors-test.js b/src/nav/__tests__/navSelectors-test.js index 47d70d80711..26ea2424d2d 100644 --- a/src/nav/__tests__/navSelectors-test.js +++ b/src/nav/__tests__/navSelectors-test.js @@ -4,7 +4,6 @@ import { getCurrentRouteName, getCurrentRouteParams, getChatScreenParams, - getCanGoBack, getSameRoutesCount, } from '../navSelectors'; @@ -61,26 +60,6 @@ describe('getChatScreenParams', () => { }); }); -describe('getCanGoBack', () => { - test('return true if current route in the stack is not the only route', () => { - const state = deepFreeze({ - nav: { - index: 1, - }, - }); - expect(getCanGoBack(state)).toBe(true); - }); - - test('return false if current route in the stack is the only route', () => { - const state = deepFreeze({ - nav: { - index: 0, - }, - }); - expect(getCanGoBack(state)).toBe(false); - }); -}); - describe('getSameRoutesCount', () => { test('if no routes the count of same routes is 0', () => { const state = deepFreeze({ diff --git a/src/nav/navSelectors.js b/src/nav/navSelectors.js index c4d5f5d2ae1..79f39829c0b 100644 --- a/src/nav/navSelectors.js +++ b/src/nav/navSelectors.js @@ -20,8 +20,6 @@ export const getCurrentRouteParams = (state: GlobalState): void | { narrow?: Nar export const getChatScreenParams = (state: GlobalState): { narrow?: Narrow } => getCurrentRouteParams(state) ?? { narrow: undefined }; -export const getCanGoBack = (state: GlobalState) => state.nav.index > 0; - export const getSameRoutesCount = (state: GlobalState): number => { const nav = getNav(state); let i = nav.routes.length - 1; From da7637274b81ac88d218a64edc99274c1f0252ee Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 25 Sep 2020 16:41:46 -0700 Subject: [PATCH 04/21] AppWithNavigation: Use `state.nav` instead of `getNav`. To ensure that the `state` prop passed to the component returned by `createReduxContainer` [1] still comes from Redux, even as we reimplement `getNav` to get the navigation state from the newly added `NavigationService`. [1] https://github.com/react-navigation/redux-helpers#createreduxcontainer-required --- src/nav/AppWithNavigation.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nav/AppWithNavigation.js b/src/nav/AppWithNavigation.js index 5ca71a17cab..abf650f5e9e 100644 --- a/src/nav/AppWithNavigation.js +++ b/src/nav/AppWithNavigation.js @@ -3,13 +3,12 @@ import { createReduxContainer } from 'react-navigation-redux-helpers'; import { connect } from 'react-redux'; -import { getNav } from '../selectors'; import AppNavigator from './AppNavigator'; // $FlowFixMe - should use a type-checked `connect` export default connect( state => ({ - state: getNav(state), + state: state.nav, }), null, null, From 3ad1d1b9d10eba0c474105c4a4c76ceb9de8941e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 25 Sep 2020 18:04:43 -0700 Subject: [PATCH 05/21] store: Spell out `getNav` here, before we reimplement `getNav`. As in the previous few commits, this is one of a small handful of places where we need to write code to explicitly get the navigation state from Redux, before we reimplement `getNav` to get the state from the recently added `NavigationService`. Like in those previous few commits, this is a place where we'll no longer need the navigation state, when #3804 is completed. --- src/boot/store.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/boot/store.js b/src/boot/store.js index e3193d753ef..91d04f1e8ac 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -17,7 +17,6 @@ import { REHYDRATE } from '../actionConstants'; import rootReducer from './reducers'; import ZulipAsyncStorage from './ZulipAsyncStorage'; import createMigration from '../redux-persist-migrate/index'; -import { getNav } from '../nav/navSelectors'; // AsyncStorage.clear(); // use to reset storage during development @@ -212,7 +211,7 @@ function listMiddleware() { const result = [ // Allow us to cause navigation by dispatching Redux actions. // See docs: https://github.com/react-navigation/redux-helpers - createReactNavigationReduxMiddleware(getNav, 'root'), + createReactNavigationReduxMiddleware((state: GlobalState) => state.nav, 'root'), // Delay ("buffer") actions until a REHYDRATE action comes through. // After dispatching the latter, this will go back and dispatch From 026bcdc23648281574b741d06b826ca9ec61d5ab Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 25 Sep 2020 16:49:27 -0700 Subject: [PATCH 06/21] navSelectors: Get data from `NavigationService`, not from Redux. We haven't been using any of these functions in react-redux `connect` calls; if we had, we might have been relying on them for the important task of triggering updates (re-`render`s) in some components, when the nav state changes. We've recently set up the `NavigationService`, which should provide the nav state just as readily as Redux could. Using React Navigation's types, instead of our own, reveals an unchecked assumption we've been making, at the single call site of `getChatScreenParams`. The value at `.narrow` in the returned object, if that value is not undefined, won't obviously be a `Narrow`. So, add a FlowFixMe there. --- src/events/doEventActionSideEffects.js | 4 +- src/nav/__tests__/navSelectors-test.js | 62 +++++++++++++------------- src/nav/navActions.js | 2 +- src/nav/navSelectors.js | 33 +++++++------- 4 files changed, 51 insertions(+), 50 deletions(-) diff --git a/src/events/doEventActionSideEffects.js b/src/events/doEventActionSideEffects.js index 9301d109dc8..58beaaecf66 100644 --- a/src/events/doEventActionSideEffects.js +++ b/src/events/doEventActionSideEffects.js @@ -28,7 +28,9 @@ const messageEvent = (state: GlobalState, message: Message): void => { } const activeAccount = getActiveAccount(state); - const { narrow } = getChatScreenParams(state); + // Assume (unchecked) that `narrow` is `Narrow` if present + // $FlowFixMe + const narrow: Narrow | void = getChatScreenParams().narrow; const isUserInSameNarrow = activeAccount && narrow !== undefined // chat screen is not at top diff --git a/src/nav/__tests__/navSelectors-test.js b/src/nav/__tests__/navSelectors-test.js index 26ea2424d2d..5bb98d93158 100644 --- a/src/nav/__tests__/navSelectors-test.js +++ b/src/nav/__tests__/navSelectors-test.js @@ -6,21 +6,23 @@ import { getChatScreenParams, getSameRoutesCount, } from '../navSelectors'; +import NavigationService from '../NavigationService'; describe('getCurrentRouteName', () => { test('return name of the current route', () => { - const state = deepFreeze({ - nav: { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ index: 1, routes: [ { routeName: 'first', params: { email: 'a@a.com' } }, { routeName: 'second', params: { email: 'b@a.com' } }, ], - }, - }); + }), + ); + const expectedResult = 'second'; - const actualResult = getCurrentRouteName(state); + const actualResult = getCurrentRouteName(); expect(actualResult).toEqual(expectedResult); }); @@ -28,18 +30,18 @@ describe('getCurrentRouteName', () => { describe('getCurrentRouteParams', () => { test('return params of the current route', () => { - const state = deepFreeze({ - nav: { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ index: 1, routes: [ { routeName: 'first', params: { email: 'a@a.com' } }, { routeName: 'second', params: { email: 'b@a.com' } }, ], - }, - }); + }), + ); const expectedResult = { email: 'b@a.com' }; - const actualResult = getCurrentRouteParams(state); + const actualResult = getCurrentRouteParams(); expect(actualResult).toEqual(expectedResult); }); @@ -47,14 +49,14 @@ describe('getCurrentRouteParams', () => { describe('getChatScreenParams', () => { test('when no params are passed do not return "undefined"', () => { - const state = deepFreeze({ - nav: { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ index: 0, routes: [{ routeName: 'chat' }], - }, - }); + }), + ); - const actualResult = getChatScreenParams(state); + const actualResult = getChatScreenParams(); expect(actualResult).toBeDefined(); }); @@ -62,32 +64,32 @@ describe('getChatScreenParams', () => { describe('getSameRoutesCount', () => { test('if no routes the count of same routes is 0', () => { - const state = deepFreeze({ - nav: { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ routes: [], - }, - }); + }), + ); - const count = getSameRoutesCount(state); + const count = getSameRoutesCount(); expect(count).toEqual(0); }); test('if last route differs from routes the count of same routes is 0', () => { - const state = deepFreeze({ - nav: { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ routes: [{ routeName: 'main' }, { routeName: 'chat' }], - }, - }); + }), + ); - const count = getSameRoutesCount(state); + const count = getSameRoutesCount(); expect(count).toEqual(1); }); test('if several of the routes are the same ignore the params and return their count', () => { - const state = deepFreeze({ - nav: { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ routes: [ { routeName: 'login' }, { routeName: 'main' }, @@ -95,10 +97,10 @@ describe('getSameRoutesCount', () => { { routeName: 'chat', params: { key: 'another value' } }, { routeName: 'chat', params: { anotherKey: 'some value' } }, ], - }, - }); + }), + ); - const count = getSameRoutesCount(state); + const count = getSameRoutesCount(); expect(count).toEqual(3); }); diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 5e9234b7215..ef3a5de2bd3 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -14,7 +14,7 @@ import type { import { getSameRoutesCount } from '../selectors'; export const navigateBack = () => (dispatch: Dispatch, getState: GetState): NavigationAction => - dispatch(StackActions.pop({ n: getSameRoutesCount(getState()) })); + dispatch(StackActions.pop({ n: getSameRoutesCount() })); // Other stack routes reached through `navReducer`: // StackActions.push({ routeName: 'loading' }); diff --git a/src/nav/navSelectors.js b/src/nav/navSelectors.js index 79f39829c0b..44ef8084817 100644 --- a/src/nav/navSelectors.js +++ b/src/nav/navSelectors.js @@ -1,33 +1,30 @@ /* @flow strict-local */ -import type { GlobalState, NavigationRouteState, NavigationState } from '../types'; -import type { Narrow } from '../api/apiTypes'; +import type { NavigationState, NavigationRoute } from 'react-navigation'; -export const getNav = (state: GlobalState): NavigationState => state.nav; +import NavigationService from './NavigationService'; -const getNavigationRoutes = (state: GlobalState): NavigationRouteState[] => state.nav.routes; +export const getNavState = (): NavigationState => NavigationService.getState(); -const getNavigationIndex = (state: GlobalState): number => getNav(state).index; +const getNavigationRoutes = () => getNavState().routes; -const getCurrentRoute = (state: GlobalState): void | NavigationRouteState => - getNavigationRoutes(state)[getNavigationIndex(state)]; +const getNavigationIndex = () => getNavState().index; -export const getCurrentRouteName = (state: GlobalState): void | string => - getCurrentRoute(state)?.routeName; +const getCurrentRoute = (): void | NavigationRoute => getNavigationRoutes()[getNavigationIndex()]; -export const getCurrentRouteParams = (state: GlobalState): void | { narrow?: Narrow } => - getCurrentRoute(state)?.params; +export const getCurrentRouteName = () => getCurrentRoute()?.routeName; -export const getChatScreenParams = (state: GlobalState): { narrow?: Narrow } => - getCurrentRouteParams(state) ?? { narrow: undefined }; +export const getCurrentRouteParams = () => getCurrentRoute()?.params; -export const getSameRoutesCount = (state: GlobalState): number => { - const nav = getNav(state); - let i = nav.routes.length - 1; +export const getChatScreenParams = () => getCurrentRouteParams() ?? { narrow: undefined }; + +export const getSameRoutesCount = () => { + const routes = getNavigationRoutes(); + let i = routes.length - 1; while (i >= 0) { - if (nav.routes[i].routeName !== nav.routes[nav.routes.length - 1].routeName) { + if (routes[i].routeName !== routes[routes.length - 1].routeName) { break; } i--; } - return nav.routes.length - i - 1; + return routes.length - i - 1; }; From b0a54c59d402764b84f9164b6c2b7cc500a7c0c7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 28 Sep 2020 11:25:19 -0700 Subject: [PATCH 07/21] navActions: Make `navigateBack` a non-thunk action creator. Now, conveniently, all of the functions in this file create plain-object actions that can be passed directly to the `dispatch` function on the recently added `NavigationService` [1]. [1] This is documented in the case of using `dispatch` on the `navigation` prop (https://reactnavigation.org/docs/4.x/navigation-prop/#dispatch---send-an-action-to-the-router) but it also applies to our `NavigationService.dispatch`. --- src/nav/navActions.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/nav/navActions.js b/src/nav/navActions.js index ef3a5de2bd3..8c2fb9d2243 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -2,9 +2,7 @@ import { StackActions } from 'react-navigation'; import type { - Dispatch, NavigationAction, - GetState, Message, Narrow, UserOrBot, @@ -13,8 +11,7 @@ import type { } from '../types'; import { getSameRoutesCount } from '../selectors'; -export const navigateBack = () => (dispatch: Dispatch, getState: GetState): NavigationAction => - dispatch(StackActions.pop({ n: getSameRoutesCount() })); +export const navigateBack = (): NavigationAction => StackActions.pop({ n: getSameRoutesCount() }); // Other stack routes reached through `navReducer`: // StackActions.push({ routeName: 'loading' }); From f5ca194f569830820ea6ec7cd9171305f7a0bb50 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 2 Oct 2020 17:55:06 -0700 Subject: [PATCH 08/21] RealmScreen [nfc]: Give `initialRealm` a better name. This is a string that's passed to `SmartUrlInput` as its `defaultValue`. The use of react-redux's `connect` appears to be done just for convenience; the function we pass as `mapStateToProps` doesn't use the Redux state. --- src/start/RealmScreen.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/start/RealmScreen.js b/src/start/RealmScreen.js index 7b5fe3ab157..b652d7051aa 100644 --- a/src/start/RealmScreen.js +++ b/src/start/RealmScreen.js @@ -12,7 +12,7 @@ import * as api from '../api'; import { realmAdd, navigateToAuth } from '../actions'; type SelectorProps = {| - +initialRealm: string, + +initialRealmInputValue: string, |}; type Props = $ReadOnly<{| @@ -44,7 +44,7 @@ type State = {| class RealmScreen extends PureComponent { state = { progress: false, - realmInputValue: this.props.initialRealm, + realmInputValue: this.props.initialRealmInputValue, error: null, }; @@ -92,14 +92,14 @@ class RealmScreen extends PureComponent { handleRealmChange = (value: string) => this.setState({ realmInputValue: value }); componentDidMount() { - const { initialRealm } = this.props; - if (initialRealm && initialRealm.length > 0) { + const { initialRealmInputValue } = this.props; + if (initialRealmInputValue && initialRealmInputValue.length > 0) { this.tryRealm(); } } render() { - const { initialRealm, navigation } = this.props; + const { initialRealmInputValue, navigation } = this.props; const { progress, error, realmInputValue } = this.state; const styles = { @@ -124,7 +124,7 @@ class RealmScreen extends PureComponent { defaultProtocol="https://" defaultOrganization="your-org" defaultDomain="zulipchat.com" - defaultValue={initialRealm} + defaultValue={initialRealmInputValue} onChangeText={this.handleRealmChange} onSubmitEditing={this.tryRealm} enablesReturnKeyAutomatically @@ -147,5 +147,5 @@ class RealmScreen extends PureComponent { } export default connect((state, props) => ({ - initialRealm: props.navigation.state.params?.realm?.toString() ?? '', + initialRealmInputValue: props.navigation.state.params?.realm?.toString() ?? '', }))(RealmScreen); From c70947cbbeaffafe33abf9021f44a8087c396181 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 11:11:06 -0700 Subject: [PATCH 09/21] navActions [nfc]: Use a params object for `navigateToRealmScreen`. We're about to have it take an optional `initial` argument. When a function has multiple optional positional parameters, there is unnecessary confusion about what a caller should do when it doesn't want to pass one of them. --- src/account/AccountPickScreen.js | 2 +- src/nav/navActions.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/account/AccountPickScreen.js b/src/account/AccountPickScreen.js index e620be5f937..1f2a4525f85 100644 --- a/src/account/AccountPickScreen.js +++ b/src/account/AccountPickScreen.js @@ -32,7 +32,7 @@ class AccountPickScreen extends PureComponent { dispatch(accountSwitch(index)); }); } else { - dispatch(navigateToRealmScreen(realm)); + dispatch(navigateToRealmScreen({ realm })); } }; diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 8c2fb9d2243..82c86644f68 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -48,8 +48,8 @@ export const navigateToAccountDetails = (userId: number): NavigationAction => export const navigateToGroupDetails = (recipients: UserOrBot[]): NavigationAction => StackActions.push({ routeName: 'group-details', params: { recipients } }); -export const navigateToRealmScreen = (realm?: URL): NavigationAction => - StackActions.push({ routeName: 'realm', params: { realm } }); +export const navigateToRealmScreen = (args: { realm?: URL } = {}): NavigationAction => + StackActions.push({ routeName: 'realm', params: { realm: args.realm } }); export const navigateToLightbox = (src: string, message: Message): NavigationAction => StackActions.push({ routeName: 'lightbox', params: { src, message } }); From cc6e9190113ffcd56ddf1459d114edd20327eb5a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 14:12:04 -0700 Subject: [PATCH 10/21] navActions: Let `navigateToRealmScreen` take `initial`. Our current nav-state logic has a case where `RealmScreen` has `params.initial` in its navigation state. So, allow this to be passed in the action creator. The case is a weird one: it came about in 7a04a5638, when `getStateForRoute` started including `initial: true` when the passed `route` was 'realm'. This has coincided with the fact that `getStateForRoute`'s only call site with 'realm' for `route` has been one where it's appropriate to have `initial: true`. Anyway, that call site is in the `rehydrate` handler in `navReducer`, which we're going to dissolve quite soon, using the `navigateToRealmScreen` action creator instead. --- src/nav/navActions.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 82c86644f68..1d8dc07ef3b 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -48,8 +48,10 @@ export const navigateToAccountDetails = (userId: number): NavigationAction => export const navigateToGroupDetails = (recipients: UserOrBot[]): NavigationAction => StackActions.push({ routeName: 'group-details', params: { recipients } }); -export const navigateToRealmScreen = (args: { realm?: URL } = {}): NavigationAction => - StackActions.push({ routeName: 'realm', params: { realm: args.realm } }); +export const navigateToRealmScreen = ( + args: { realm?: URL, initial?: boolean } = {}, +): NavigationAction => + StackActions.push({ routeName: 'realm', params: { realm: args.realm, initial: args.initial } }); export const navigateToLightbox = (src: string, message: Message): NavigationAction => StackActions.push({ routeName: 'lightbox', params: { src, message } }); From e2c102d89df8dc176a5174d81b6afff4bfbcaaba Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 13:21:38 -0700 Subject: [PATCH 11/21] navActions: Add "reset" actions, to be used in initial navigation. Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (#3804). The various handlers will be transplanted into instructions to dispatch navigation actions at the appropriate times. These new functions will create those actions. It makes sense for them to be "reset" actions because the effect of those handlers in `navReducer` was to construct a state based on no previous state; this was ensured by passing no second argument (`lastState`) to `AppNavigator.router.getStateForAction` in our `getStateForRoute`. It also makes sense intuitively; if we're doing something like logging out, we don't want to allow back navigation. [1] https://reactnavigation.org/docs/4.x/stack-actions/#reset --- src/nav/navActions.js | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 1d8dc07ef3b..6f360cb4e79 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import { StackActions } from 'react-navigation'; +import { StackActions, NavigationActions } from 'react-navigation'; import type { NavigationAction, @@ -13,11 +13,35 @@ import { getSameRoutesCount } from '../selectors'; export const navigateBack = (): NavigationAction => StackActions.pop({ n: getSameRoutesCount() }); -// Other stack routes reached through `navReducer`: -// StackActions.push({ routeName: 'loading' }); -// StackActions.push({ routeName: 'realm' }); -// StackActions.push({ routeName: 'account' }); -// StackActions.push({ routeName: 'main' }); +/* + * "Reset" actions, to explicitly prohibit back navigation. + */ + +export const resetToLoading = (): NavigationAction => + StackActions.reset({ index: 0, actions: [NavigationActions.navigate({ routeName: 'loading' })] }); + +export const resetToRealmScreen = ( + args: { realm?: URL, initial?: boolean } = {}, +): NavigationAction => + StackActions.reset({ + index: 0, + actions: [ + NavigationActions.navigate({ + routeName: 'realm', + params: { realm: args.realm, initial: args.initial }, + }), + ], + }); + +export const resetToAccountPicker = (): NavigationAction => + StackActions.reset({ index: 0, actions: [NavigationActions.navigate({ routeName: 'account' })] }); + +export const resetToMainTabs = (): NavigationAction => + StackActions.reset({ index: 0, actions: [NavigationActions.navigate({ routeName: 'main' })] }); + +/* + * Ordinary "push" actions that will push to the stack. + */ /** Only call this via `doNarrow`. See there for details. */ export const navigateToChat = (narrow: Narrow): NavigationAction => From 5e409a549cd27f90fddef1123d35086e4b3e0e60 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 14:04:43 -0700 Subject: [PATCH 12/21] AppNavigator: Set `initialRouteName` to 'loading'. I'm not sure if this actually changes anything; 'loading' has effectively (and intentionally) been the initial route for quite some time. See `navReducer`: ``` export const initialState = getStateForRoute('loading'); ``` --- src/nav/AppNavigator.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nav/AppNavigator.js b/src/nav/AppNavigator.js index 1dc1b47c0f5..6a8c164c837 100644 --- a/src/nav/AppNavigator.js +++ b/src/nav/AppNavigator.js @@ -94,7 +94,7 @@ export default createStackNavigator( ios: TransitionPresets.DefaultTransition, }), }, - initialRouteName: 'main', + initialRouteName: 'loading', headerMode: 'none', }, ); From a770a348d7b31d2a08f91af1047180695515f17b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 2 Oct 2020 17:22:06 -0700 Subject: [PATCH 13/21] navReducer: Transplant away `REHYDRATE` logic. Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (#3804). So, convert the `navReducer`'s `rehydrate` handler into instructions to dispatch the right navigation actions at the right time. --- src/ZulipMobile.js | 9 +- src/nav/InitialNavigationDispatcher.js | 81 ++++++++++++++ src/nav/__tests__/navReducer-test.js | 143 +------------------------ src/nav/navReducer.js | 42 +------- src/session/sessionReducer.js | 3 +- 5 files changed, 92 insertions(+), 186 deletions(-) create mode 100644 src/nav/InitialNavigationDispatcher.js diff --git a/src/ZulipMobile.js b/src/ZulipMobile.js index c068aeeb9a5..5bb53d63d35 100644 --- a/src/ZulipMobile.js +++ b/src/ZulipMobile.js @@ -10,6 +10,7 @@ import CompatibilityChecker from './boot/CompatibilityChecker'; import AppEventHandlers from './boot/AppEventHandlers'; import AppDataFetcher from './boot/AppDataFetcher'; import BackNavigationHandler from './nav/BackNavigationHandler'; +import InitialNavigationDispatcher from './nav/InitialNavigationDispatcher'; import AppWithNavigation from './nav/AppWithNavigation'; import NavigationService from './nav/NavigationService'; @@ -28,9 +29,11 @@ export default (): React$Node => ( - - - + + + + + diff --git a/src/nav/InitialNavigationDispatcher.js b/src/nav/InitialNavigationDispatcher.js new file mode 100644 index 00000000000..7dc73475652 --- /dev/null +++ b/src/nav/InitialNavigationDispatcher.js @@ -0,0 +1,81 @@ +/* @flow strict-local */ +import type { Node as React$Node } from 'react'; +import { PureComponent } from 'react'; + +import type { Dispatch, Account } from '../types'; +import { resetToAccountPicker, resetToRealmScreen, resetToMainTabs } from '../actions'; +import { connect } from '../react-redux'; +import { getIsHydrated, hasAuth as getHasAuth, getHaveServerData } from '../selectors'; + +type SelectorProps = $ReadOnly<{| + isHydrated: boolean, + hasAuth: boolean, + accounts: Account[], + haveServerData: boolean, +|}>; + +type Props = $ReadOnly<{| + children: React$Node, + + dispatch: Dispatch, + ...SelectorProps, +|}>; + +class InitialNavigationDispatcher extends PureComponent { + componentDidMount() { + if (this.props.isHydrated) { + this.doInitialNavigation(); + } + } + + componentDidUpdate(prevProps) { + if (!prevProps.isHydrated && this.props.isHydrated) { + this.doInitialNavigation(); + } + } + + /** + * Data has been loaded, so open the app to the right screen. + * + * Not to be called before the REHYDRATE action, and not to be + * called more than once. + */ + doInitialNavigation = () => { + const { hasAuth, accounts, haveServerData, dispatch } = this.props; + + // If the active account is not logged in, show account screen. + if (!hasAuth) { + if (accounts.length > 1) { + dispatch(resetToAccountPicker()); + return; + } else { + dispatch(resetToRealmScreen({ initial: true })); + return; + } + } + + // If there's an active, logged-in account but no server data, then behave + // like `ACCOUNT_SWITCH`: show loading screen. Crucially, `sessionReducer` + // will have set `needsInitialFetch`, too, so we really will be loading. + if (!haveServerData) { + // We're already on the loading screen -- see `initialRouteName` + // in `AppNavigator`. + return; + } + + // Great: we have an active, logged-in account, and server data for it. + // Show the main UI. + dispatch(resetToMainTabs()); + }; + + render() { + return this.props.children; + } +} + +export default connect(state => ({ + hasAuth: getHasAuth(state), + accounts: state.accounts, + haveServerData: getHaveServerData(state), + isHydrated: getIsHydrated(state), +}))(InitialNavigationDispatcher); diff --git a/src/nav/__tests__/navReducer-test.js b/src/nav/__tests__/navReducer-test.js index add69acfc58..ffde22bb222 100644 --- a/src/nav/__tests__/navReducer-test.js +++ b/src/nav/__tests__/navReducer-test.js @@ -1,14 +1,9 @@ import deepFreeze from 'deep-freeze'; -import { LOGIN_SUCCESS, INITIAL_FETCH_COMPLETE, REHYDRATE } from '../../actionConstants'; -import navReducer, { - getStateForRoute, - initialState as initialNavigationState, -} from '../navReducer'; +import { LOGIN_SUCCESS, INITIAL_FETCH_COMPLETE } from '../../actionConstants'; +import navReducer, { getStateForRoute } from '../navReducer'; describe('navReducer', () => { - const initialState = deepFreeze(initialNavigationState); - describe('LOGIN_SUCCESS', () => { test('replaces the existing route stack with "loading" on sign in', () => { const prevState = deepFreeze({ @@ -45,138 +40,4 @@ describe('navReducer', () => { expect(newState).toBe(prevState); }); }); - - describe('REHYDRATE', () => { - test('when no previous navigation is given do not throw but return some result', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [{ apiKey: '123' }], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - }); - - test('if logged in and users is empty, go to loading', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [{ apiKey: '123' }], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('loading'); - }); - - test('if logged in and users is not empty, go to main', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [{ apiKey: '123' }], - users: [{ user_id: 123 }], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('main'); - }); - - test('if not logged in, and no previous accounts, show login screen', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('realm'); - }); - - test('if more than one account and no active account, display account list', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [{ apiKey: '' }, { apiKey: '' }], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('account'); - }); - - test('when only a single account and no other properties, redirect to login screen', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [{ apiKey: '', realm: new URL('https://example.com') }], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('realm'); - }); - - test('when multiple accounts and default one has realm and email, show account list', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [ - { apiKey: '', realm: new URL('https://example.com'), email: 'johndoe@example.com' }, - { apiKey: '', realm: new URL('https://example.com'), email: 'janedoe@example.com' }, - ], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('account'); - }); - - test('when default account has server and email set, redirect to login screen', () => { - const action = deepFreeze({ - type: REHYDRATE, - payload: { - accounts: [ - { apiKey: '', realm: new URL('https://example.com'), email: 'johndoe@example.com' }, - ], - users: [], - realm: {}, - }, - }); - - const nav = navReducer(initialState, action); - - expect(nav.routes).toHaveLength(1); - expect(nav.routes[0].routeName).toEqual('realm'); - }); - }); }); diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index 69b94bd6129..867ce1d660a 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -3,14 +3,7 @@ import type { NavigationAction } from 'react-navigation'; import type { NavigationState, Action } from '../types'; import AppNavigator from './AppNavigator'; -import { - REHYDRATE, - INITIAL_FETCH_COMPLETE, - ACCOUNT_SWITCH, - LOGIN_SUCCESS, - LOGOUT, -} from '../actionConstants'; -import { hasAuth } from '../account/accountsSelectors'; +import { INITIAL_FETCH_COMPLETE, ACCOUNT_SWITCH, LOGIN_SUCCESS, LOGOUT } from '../actionConstants'; /** * Get the initial state for the given route. @@ -41,43 +34,10 @@ export const getStateForRoute = (route: string): NavigationState => { return state; }; -const rehydrate = (state, action) => { - // If there's no data to rehydrate, or no account data, show login screen. - if (!action.payload || !action.payload.accounts) { - return getStateForRoute('realm'); - } - - // If there are accounts but the active account is not logged in, - // show account screen. - const rehydratedState = action.payload; - if (!hasAuth(rehydratedState)) { - const { accounts } = rehydratedState; - return getStateForRoute(accounts && accounts.length > 1 ? 'account' : 'realm'); - } - - // If there's an active, logged-in account but no server data, then behave - // like `ACCOUNT_SWITCH`: show loading screen. Crucially, `sessionReducer` - // will have set `needsInitialFetch`, too, so we really will be loading. - // - // (Valid server data must have a user: the self user, at a minimum. - // Compare getHaveServerData; this has an extra check because `users` - // may be missing entirely.) - if (rehydratedState.users === undefined || rehydratedState.users.length === 0) { - return getStateForRoute('loading'); - } - - // Great: we have an active, logged-in account, and server data for it. - // Show the main UI. - return getStateForRoute('main'); -}; - export const initialState = getStateForRoute('loading'); export default (state: NavigationState = initialState, action: Action): NavigationState => { switch (action.type) { - case REHYDRATE: - return rehydrate(state, action); - case ACCOUNT_SWITCH: case LOGIN_SUCCESS: return getStateForRoute('loading'); diff --git a/src/session/sessionReducer.js b/src/session/sessionReducer.js index f8ae8cefaf4..32a735211ae 100644 --- a/src/session/sessionReducer.js +++ b/src/session/sessionReducer.js @@ -91,7 +91,8 @@ const rehydrate = (state, action) => { // On rehydration, do an initial fetch if we have access to an account // (indicated by the presence of an api key). Otherwise, the initial fetch // will be initiated on loginSuccess. - // NB navReducer's rehydrate logic depends intimately on this behavior. + // NB `InitialNavigationDispatcher`'s `doInitialNavigation` + // depends intimately on this behavior. needsInitialFetch: haveApiKey, }; }; From a34d3ba163405d47e4365a74c6ec360e83e0c831 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 17:32:33 -0700 Subject: [PATCH 14/21] navReducer [nfc]: Remove an `initial: true` params hack. In the previous commit, we removed our last call site of `getStateWithRoute` where 'realm' was passed for `route`. Therefore, this conditional can disappear. See more about this hack at https://github.com/zulip/zulip-mobile/pull/4273#discussion_r499114689. --- src/nav/navReducer.js | 12 +----------- src/start/RealmScreen.js | 3 --- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index 867ce1d660a..b01daa0bace 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -11,17 +11,7 @@ import { INITIAL_FETCH_COMPLETE, ACCOUNT_SWITCH, LOGIN_SUCCESS, LOGOUT } from '. * Private; exported only for tests. */ export const getStateForRoute = (route: string): NavigationState => { - // TODO: this is kind of a hack! Refactor to a better way. - // * Perhaps pass `initial: true` unconditionally in this initialization - // code, to all routes? Then that'd just be part of the interface of - // making a screen work as an initial screen. - // * Alternatively, we could replace the whole system of `ModalNavBar`, - // `canGoBack`, etc., with our own "navigation view" that would be more - // directly integrated into the navigation framework: - // https://reactnavigation.org/docs/en/2.x/navigation-views.html - const params = route === 'realm' ? { initial: true } : undefined; - - const action = AppNavigator.router.getActionForPathAndParams(route, params); + const action = AppNavigator.router.getActionForPathAndParams(route); if (!action) { // The argument should be a constant string that is a genuine nav route; // so this condition can only happen if we've gotten that wrong. diff --git a/src/start/RealmScreen.js b/src/start/RealmScreen.js index b652d7051aa..254f0eafe3e 100644 --- a/src/start/RealmScreen.js +++ b/src/start/RealmScreen.js @@ -24,9 +24,6 @@ type Props = $ReadOnly<{| ...NavigationStateRoute, params?: {| realm: URL | void, - // Currently passed as `true` in a hack in `navReducer.js`; see - // https://github.com/zulip/zulip-mobile/pull/4273#discussion_r499114689. - // TODO: Stop using that hack. initial?: boolean, |}, |}>, From e41a9535828c31c922b4aa3da107aa1b9b24cced Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 16:42:47 -0700 Subject: [PATCH 15/21] navReducer: Transplant away `LOGOUT` logic. Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (#3804). So, convert the `navReducer`'s `LOGOUT` case into instructions to navigate to the account-picker screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition. --- src/account/accountActions.js | 10 ++++++++-- src/nav/navReducer.js | 5 +---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/account/accountActions.js b/src/account/accountActions.js index 056e00269d8..bcfefe39bfe 100644 --- a/src/account/accountActions.js +++ b/src/account/accountActions.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import type { Action } from '../types'; +import type { Action, Dispatch, GetState } from '../types'; import { ACCOUNT_SWITCH, REALM_ADD, @@ -7,6 +7,7 @@ import { LOGIN_SUCCESS, LOGOUT, } from '../actionConstants'; +import { resetToAccountPicker } from '../nav/navActions'; import type { ZulipVersion } from '../utils/zulipVersion'; export const accountSwitch = (index: number): Action => ({ @@ -37,6 +38,11 @@ export const loginSuccess = (realm: URL, email: string, apiKey: string): Action apiKey, }); -export const logout = (): Action => ({ +const logoutPlain = (): Action => ({ type: LOGOUT, }); + +export const logout = () => async (dispatch: Dispatch, getState: GetState) => { + dispatch(resetToAccountPicker()); + dispatch(logoutPlain()); +}; diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index b01daa0bace..93cddf8e977 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -3,7 +3,7 @@ import type { NavigationAction } from 'react-navigation'; import type { NavigationState, Action } from '../types'; import AppNavigator from './AppNavigator'; -import { INITIAL_FETCH_COMPLETE, ACCOUNT_SWITCH, LOGIN_SUCCESS, LOGOUT } from '../actionConstants'; +import { INITIAL_FETCH_COMPLETE, ACCOUNT_SWITCH, LOGIN_SUCCESS } from '../actionConstants'; /** * Get the initial state for the given route. @@ -35,9 +35,6 @@ export default (state: NavigationState = initialState, action: Action): Navigati case INITIAL_FETCH_COMPLETE: return state.routes[0].routeName === 'main' ? state : getStateForRoute('main'); - case LOGOUT: - return getStateForRoute('account'); - default: { // The `react-navigation` libdef says this only takes a NavigationAction, // but docs say pass arbitrary action. $FlowFixMe From c3ffa52084771f67a21d245241a58adc25af2899 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 17:07:41 -0700 Subject: [PATCH 16/21] navReducer: Transplant away `ACCOUNT_SWITCH` logic. Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (#3804). So, convert the `navReducer`'s `ACCOUNT_SWITCH` case into instructions to navigate to the loading screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition. --- src/account/accountActions.js | 9 +++++++-- src/nav/navReducer.js | 3 +-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/account/accountActions.js b/src/account/accountActions.js index bcfefe39bfe..7452e045a37 100644 --- a/src/account/accountActions.js +++ b/src/account/accountActions.js @@ -7,14 +7,19 @@ import { LOGIN_SUCCESS, LOGOUT, } from '../actionConstants'; -import { resetToAccountPicker } from '../nav/navActions'; +import { resetToAccountPicker, resetToLoading } from '../nav/navActions'; import type { ZulipVersion } from '../utils/zulipVersion'; -export const accountSwitch = (index: number): Action => ({ +const accountSwitchPlain = (index: number): Action => ({ type: ACCOUNT_SWITCH, index, }); +export const accountSwitch = (index: number) => (dispatch: Dispatch, getState: GetState) => { + dispatch(resetToLoading()); + dispatch(accountSwitchPlain(index)); +}; + export const realmAdd = ( realm: URL, zulipFeatureLevel: number, diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index 93cddf8e977..e7e67082a49 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -3,7 +3,7 @@ import type { NavigationAction } from 'react-navigation'; import type { NavigationState, Action } from '../types'; import AppNavigator from './AppNavigator'; -import { INITIAL_FETCH_COMPLETE, ACCOUNT_SWITCH, LOGIN_SUCCESS } from '../actionConstants'; +import { INITIAL_FETCH_COMPLETE, LOGIN_SUCCESS } from '../actionConstants'; /** * Get the initial state for the given route. @@ -28,7 +28,6 @@ export const initialState = getStateForRoute('loading'); export default (state: NavigationState = initialState, action: Action): NavigationState => { switch (action.type) { - case ACCOUNT_SWITCH: case LOGIN_SUCCESS: return getStateForRoute('loading'); From 45c477531e7f15e7338fffe2eed22b45f7ee72c0 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 17:12:52 -0700 Subject: [PATCH 17/21] navReducer: Transplant away `LOGIN_SUCCESS` logic. Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (#3804). So, convert the `navReducer`'s `LOGIN_SUCCESS` case into instructions to navigate to the loading screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition. --- src/account/accountActions.js | 10 +++++++++- src/nav/__tests__/navReducer-test.js | 25 +------------------------ src/nav/navReducer.js | 5 +---- 3 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/account/accountActions.js b/src/account/accountActions.js index 7452e045a37..52fbcf057d6 100644 --- a/src/account/accountActions.js +++ b/src/account/accountActions.js @@ -36,13 +36,21 @@ export const removeAccount = (index: number): Action => ({ index, }); -export const loginSuccess = (realm: URL, email: string, apiKey: string): Action => ({ +const loginSuccessPlain = (realm: URL, email: string, apiKey: string): Action => ({ type: LOGIN_SUCCESS, realm, email, apiKey, }); +export const loginSuccess = (realm: URL, email: string, apiKey: string) => ( + dispatch: Dispatch, + getState: GetState, +) => { + dispatch(resetToLoading()); + dispatch(loginSuccessPlain(realm, email, apiKey)); +}; + const logoutPlain = (): Action => ({ type: LOGOUT, }); diff --git a/src/nav/__tests__/navReducer-test.js b/src/nav/__tests__/navReducer-test.js index ffde22bb222..2b468f38cdb 100644 --- a/src/nav/__tests__/navReducer-test.js +++ b/src/nav/__tests__/navReducer-test.js @@ -1,32 +1,9 @@ import deepFreeze from 'deep-freeze'; -import { LOGIN_SUCCESS, INITIAL_FETCH_COMPLETE } from '../../actionConstants'; +import { INITIAL_FETCH_COMPLETE } from '../../actionConstants'; import navReducer, { getStateForRoute } from '../navReducer'; describe('navReducer', () => { - describe('LOGIN_SUCCESS', () => { - test('replaces the existing route stack with "loading" on sign in', () => { - const prevState = deepFreeze({ - index: 2, - routes: [{ key: 'one' }, { key: 'two' }, { key: 'password' }], - }); - - const action = deepFreeze({ - type: LOGIN_SUCCESS, - }); - - const expectedState = { - index: 0, - routes: [{ routeName: 'loading' }], - }; - - const newState = navReducer(prevState, action); - - expect(newState.index).toEqual(expectedState.index); - expect(newState.routes[0].routeName).toEqual(expectedState.routes[0].routeName); - }); - }); - describe('INITIAL_FETCH_COMPLETE', () => { test('do not mutate navigation state if already at the same route', () => { const prevState = getStateForRoute('main'); diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index e7e67082a49..5375e10bcc4 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -3,7 +3,7 @@ import type { NavigationAction } from 'react-navigation'; import type { NavigationState, Action } from '../types'; import AppNavigator from './AppNavigator'; -import { INITIAL_FETCH_COMPLETE, LOGIN_SUCCESS } from '../actionConstants'; +import { INITIAL_FETCH_COMPLETE } from '../actionConstants'; /** * Get the initial state for the given route. @@ -28,9 +28,6 @@ export const initialState = getStateForRoute('loading'); export default (state: NavigationState = initialState, action: Action): NavigationState => { switch (action.type) { - case LOGIN_SUCCESS: - return getStateForRoute('loading'); - case INITIAL_FETCH_COMPLETE: return state.routes[0].routeName === 'main' ? state : getStateForRoute('main'); From f1f041a8351608bae7bdd0a4616cd763e981d6e5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 17:27:40 -0700 Subject: [PATCH 18/21] navReducer: Use `getNavigationRoutes`. Also add a comment, with Greg's description [1] of what's going on here. We have this handy function for grabbing what we want from the navigation state, so we might as well use it. When we use this function, the navigation state is gotten from `NavigationService`, not from Redux, which moves us closer to #3804. To be more rigorous, though, we wouldn't want this to stay this way in `navReduce` forever: reducers are supposed to be pure functions of state and actions. But the incorrectness isn't obviously harmful here, and we're about to transplant this code somewhere else, and it's helpful to separate a small tweak like this from the commit where we move the code. [1] https://github.com/zulip/zulip-mobile/pull/4274#discussion_r500688470 --- src/nav/__tests__/navReducer-test.js | 7 +++++++ src/nav/navReducer.js | 12 +++++++++++- src/nav/navSelectors.js | 2 +- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/nav/__tests__/navReducer-test.js b/src/nav/__tests__/navReducer-test.js index 2b468f38cdb..8b3215b0538 100644 --- a/src/nav/__tests__/navReducer-test.js +++ b/src/nav/__tests__/navReducer-test.js @@ -2,10 +2,17 @@ import deepFreeze from 'deep-freeze'; import { INITIAL_FETCH_COMPLETE } from '../../actionConstants'; import navReducer, { getStateForRoute } from '../navReducer'; +import NavigationService from '../NavigationService'; describe('navReducer', () => { describe('INITIAL_FETCH_COMPLETE', () => { test('do not mutate navigation state if already at the same route', () => { + NavigationService.getState = jest.fn().mockReturnValue( + deepFreeze({ + index: 0, + routes: [{ routeName: 'main' }], + }), + ); const prevState = getStateForRoute('main'); const action = deepFreeze({ diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index 5375e10bcc4..4db70b7a7c3 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -2,6 +2,7 @@ import type { NavigationAction } from 'react-navigation'; import type { NavigationState, Action } from '../types'; +import { getNavigationRoutes } from './navSelectors'; import AppNavigator from './AppNavigator'; import { INITIAL_FETCH_COMPLETE } from '../actionConstants'; @@ -29,7 +30,16 @@ export const initialState = getStateForRoute('loading'); export default (state: NavigationState = initialState, action: Action): NavigationState => { switch (action.type) { case INITIAL_FETCH_COMPLETE: - return state.routes[0].routeName === 'main' ? state : getStateForRoute('main'); + // If we're anywhere in the normal UI of the app, then remain + // where we are. Only reset the nav state if we're elsewhere, + // and in that case, go to the main screen. + // + // TODO: "elsewhere" is probably just a way of saying "on the + // loading screen", but we're not sure. We could adjust the + // conditional accordingly, if we found out we're not depending on + // the more general condition; see + // https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875 + return getNavigationRoutes()[0].routeName === 'main' ? state : getStateForRoute('main'); default: { // The `react-navigation` libdef says this only takes a NavigationAction, diff --git a/src/nav/navSelectors.js b/src/nav/navSelectors.js index 44ef8084817..dce36f6bc65 100644 --- a/src/nav/navSelectors.js +++ b/src/nav/navSelectors.js @@ -5,7 +5,7 @@ import NavigationService from './NavigationService'; export const getNavState = (): NavigationState => NavigationService.getState(); -const getNavigationRoutes = () => getNavState().routes; +export const getNavigationRoutes = () => getNavState().routes; const getNavigationIndex = () => getNavState().index; From 8049976c80d4c5b95cb4d277f2de682a31387d45 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 5 Oct 2020 17:25:30 -0700 Subject: [PATCH 19/21] navReducer: Transplant away `INITIAL_FETCH_COMPLETE` logic. Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (#3804). So, convert the `navReducer`'s `INITIAL_FETCH_COMPLETE` case into instructions to navigate to the main-tabs screen, if not already there. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition. --- src/message/fetchActions.js | 20 +++++++++++++++++++- src/nav/__tests__/navReducer-test.js | 27 --------------------------- src/nav/navReducer.js | 14 -------------- 3 files changed, 19 insertions(+), 42 deletions(-) delete mode 100644 src/nav/__tests__/navReducer-test.js diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 07d2d4c355d..1b7dd7cdc98 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -10,6 +10,7 @@ import type { } from '../types'; import type { InitialData } from '../api/initialDataTypes'; import * as api from '../api'; +import { resetToMainTabs } from '../actions'; import { isClientError } from '../api/apiErrors'; import { getAuth, @@ -18,6 +19,7 @@ import { getLastMessageId, getCaughtUpForNarrow, getFetchingForNarrow, + getNavigationRoutes, } from '../selectors'; import config from '../config'; import { @@ -156,10 +158,26 @@ const initialFetchStart = (): Action => ({ type: INITIAL_FETCH_START, }); -const initialFetchComplete = (): Action => ({ +const initialFetchCompletePlain = (): Action => ({ type: INITIAL_FETCH_COMPLETE, }); +export const initialFetchComplete = () => async (dispatch: Dispatch, getState: GetState) => { + if (getNavigationRoutes()[0].routeName !== 'main') { + // If we're anywhere in the normal UI of the app, then remain + // where we are. Only reset the nav state if we're elsewhere, + // and in that case, go to the main screen. + // + // TODO: "elsewhere" is probably just a way of saying "on the + // loading screen", but we're not sure. We could adjust the + // conditional accordingly, if we found out we're not depending on + // the more general condition; see + // https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875 + dispatch(resetToMainTabs()); + } + dispatch(initialFetchCompletePlain()); +}; + /** Private; exported only for tests. */ export const isFetchNeededAtAnchor = ( state: GlobalState, diff --git a/src/nav/__tests__/navReducer-test.js b/src/nav/__tests__/navReducer-test.js deleted file mode 100644 index 8b3215b0538..00000000000 --- a/src/nav/__tests__/navReducer-test.js +++ /dev/null @@ -1,27 +0,0 @@ -import deepFreeze from 'deep-freeze'; - -import { INITIAL_FETCH_COMPLETE } from '../../actionConstants'; -import navReducer, { getStateForRoute } from '../navReducer'; -import NavigationService from '../NavigationService'; - -describe('navReducer', () => { - describe('INITIAL_FETCH_COMPLETE', () => { - test('do not mutate navigation state if already at the same route', () => { - NavigationService.getState = jest.fn().mockReturnValue( - deepFreeze({ - index: 0, - routes: [{ routeName: 'main' }], - }), - ); - const prevState = getStateForRoute('main'); - - const action = deepFreeze({ - type: INITIAL_FETCH_COMPLETE, - }); - - const newState = navReducer(prevState, action); - - expect(newState).toBe(prevState); - }); - }); -}); diff --git a/src/nav/navReducer.js b/src/nav/navReducer.js index 4db70b7a7c3..07eee0d51b4 100644 --- a/src/nav/navReducer.js +++ b/src/nav/navReducer.js @@ -2,9 +2,7 @@ import type { NavigationAction } from 'react-navigation'; import type { NavigationState, Action } from '../types'; -import { getNavigationRoutes } from './navSelectors'; import AppNavigator from './AppNavigator'; -import { INITIAL_FETCH_COMPLETE } from '../actionConstants'; /** * Get the initial state for the given route. @@ -29,18 +27,6 @@ export const initialState = getStateForRoute('loading'); export default (state: NavigationState = initialState, action: Action): NavigationState => { switch (action.type) { - case INITIAL_FETCH_COMPLETE: - // If we're anywhere in the normal UI of the app, then remain - // where we are. Only reset the nav state if we're elsewhere, - // and in that case, go to the main screen. - // - // TODO: "elsewhere" is probably just a way of saying "on the - // loading screen", but we're not sure. We could adjust the - // conditional accordingly, if we found out we're not depending on - // the more general condition; see - // https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875 - return getNavigationRoutes()[0].routeName === 'main' ? state : getStateForRoute('main'); - default: { // The `react-navigation` libdef says this only takes a NavigationAction, // but docs say pass arbitrary action. $FlowFixMe From 2821bea02b28d2ad1dfdf9b90aec4be7dbf5faf6 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 12 Oct 2020 12:13:52 -0700 Subject: [PATCH 20/21] fetchActions: Compare all routes to 'main', not just the first. I think we generally expect the 'main' route to be index zero of the routes, when it's among the routes. But it's not obvious that this will always hold, and it's quite easy to check all the routes. --- src/message/fetchActions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 1b7dd7cdc98..90b17f919ae 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -163,7 +163,7 @@ const initialFetchCompletePlain = (): Action => ({ }); export const initialFetchComplete = () => async (dispatch: Dispatch, getState: GetState) => { - if (getNavigationRoutes()[0].routeName !== 'main') { + if (!getNavigationRoutes().some(navigationRoute => navigationRoute.routeName === 'main')) { // If we're anywhere in the normal UI of the app, then remain // where we are. Only reset the nav state if we're elsewhere, // and in that case, go to the main screen. From 2e049c4de3d266165d4d152fd7bc305fb54fdb8f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 20 Oct 2020 13:29:10 -0700 Subject: [PATCH 21/21] InitialNavigationDispatcher: Clarify and improve `doInitialNavigation`. When the active account isn't logged in and there's just one account in total, we can reasonably pre-fill that account's realm in the realm-input screen. It seems like this has been the expected behavior [1], but I just tested it now, and it hasn't been happening. Also, for readability, separate the `accounts.length === 1` check into its own `else if`. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/decouple.20nav.20from.20redux.20%28.23M3804%29/near/1042602 --- src/nav/InitialNavigationDispatcher.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/nav/InitialNavigationDispatcher.js b/src/nav/InitialNavigationDispatcher.js index 7dc73475652..dde72678b28 100644 --- a/src/nav/InitialNavigationDispatcher.js +++ b/src/nav/InitialNavigationDispatcher.js @@ -43,12 +43,26 @@ class InitialNavigationDispatcher extends PureComponent { doInitialNavigation = () => { const { hasAuth, accounts, haveServerData, dispatch } = this.props; - // If the active account is not logged in, show account screen. + // If the active account is not logged in, bring the user as close + // as we can to AuthScreen, the place where they can log in. if (!hasAuth) { if (accounts.length > 1) { + // We can't guess which account, of multiple, the user wants + // to use. Let them pick one. dispatch(resetToAccountPicker()); return; + } else if (accounts.length === 1) { + // We already know the realm, so give that to the realm + // screen. If that screen finds that the realm is valid, it'll + // send the user along to AuthScreen for that realm right + // away. If this means you're on the AuthScreen when you don't + // want to be (i.e., you want to choose a different realm), + // you can always go back to RealmScreen. + dispatch(resetToRealmScreen({ initial: true, realm: accounts[0].realm })); + return; } else { + // Just go to the realm screen and have the user type out the + // realm. dispatch(resetToRealmScreen({ initial: true })); return; }