From fe08bb39ff5d59ed61cbff0cc5ce73ca24e95c02 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Mon, 27 Sep 2021 17:58:43 -0700 Subject: [PATCH 1/4] Fix infinite spinner on OldDot transition --- .../Navigation/AppNavigator/AuthScreens.js | 51 +++++++++++++------ src/libs/Url.js | 11 +++- src/pages/LogInWithShortLivedTokenPage.js | 9 +--- 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 38c3639ce2a..20ec961b535 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -1,6 +1,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import Onyx, {withOnyx} from 'react-native-onyx'; +import Str from 'expensify-common/lib/str'; import moment from 'moment'; import _ from 'underscore'; import lodashGet from 'lodash/get'; @@ -33,6 +34,7 @@ import createCustomModalStackNavigator from './createCustomModalStackNavigator'; import Permissions from '../../Permissions'; import getOperatingSystem from '../../getOperatingSystem'; import {fetchFreePlanVerifiedBankAccount} from '../../actions/BankAccounts'; +import {removeLeadingForwardSlash} from '../../Url'; // Main drawer navigator import MainDrawerNavigator from './MainDrawerNavigator'; @@ -111,20 +113,12 @@ const modalScreenListeners = { }, }; -let hasLoadedPolicies = false; - /** - * We want to only load policy info if you are in the freePlan beta. - * @param {Array} betas + * @param {String} route + * @returns {Boolean} */ -function loadPoliciesBehindBeta(betas) { - // When removing the freePlan beta, simply load the policyList and the policySummaries in componentDidMount(). - // Policy info loading should not be blocked behind the defaultRooms beta alone. - if (!hasLoadedPolicies && (Permissions.canUseFreePlan(betas) || Permissions.canUseDefaultRooms(betas))) { - getPolicyList(); - getPolicySummaries(); - hasLoadedPolicies = true; - } +function isTransitionOrNewWorkspacePage(route) { + return route && !_.any([ROUTES.LOGIN_WITH_SHORT_LIVED_TOKEN, ROUTES.WORKSPACE_NEW], badRoute => Str.startsWith(removeLeadingForwardSlash(route), badRoute)); } const propTypes = { @@ -149,6 +143,8 @@ class AuthScreens extends React.Component { constructor(props) { super(props); + this.hasLoadedPolicies = false; + Timing.start(CONST.TIMING.HOMEPAGE_INITIAL_RENDER); Timing.start(CONST.TIMING.HOMEPAGE_REPORTS_LOADED); } @@ -191,7 +187,7 @@ class AuthScreens extends React.Component { UnreadIndicatorUpdater.listenForReportChanges(); fetchFreePlanVerifiedBankAccount(); - loadPoliciesBehindBeta(this.props.betas); + this.loadPoliciesBehindBeta(); // Refresh the personal details, timezone and betas every 30 minutes // There is no pusher event that sends updated personal details data yet @@ -235,11 +231,15 @@ class AuthScreens extends React.Component { return true; } + if (isTransitionOrNewWorkspacePage(this.props.currentURL) && !isTransitionOrNewWorkspacePage(nextProps.currentURL)) { + return true; + } + return false; } componentDidUpdate() { - loadPoliciesBehindBeta(this.props.betas); + this.loadPoliciesBehindBeta(); } componentWillUnmount() { @@ -252,7 +252,25 @@ class AuthScreens extends React.Component { NetworkConnection.stopListeningForReconnect(); clearInterval(this.interval); this.interval = null; - hasLoadedPolicies = false; + this.hasLoadedPolicies = false; + } + + /** + * We want to only load policy info if: + * - We are on the free plan beta + * - We are not on the transition or new-workspace pages (if we load policies while creating a new one we may accidentally overwrite the new policy due to race condition) + * - We have not already loaded policies + */ + loadPoliciesBehindBeta() { + // When removing the freePlan beta, simply load the policyList and the policySummaries in componentDidMount(). + // Policy info loading should not be blocked behind the defaultRooms beta alone. + if (!this.hasLoadedPolicies + && !isTransitionOrNewWorkspacePage(this.props.currentURL) + && (Permissions.canUseFreePlan(this.props.betas) || Permissions.canUseDefaultRooms(this.props.betas))) { + getPolicyList(); + getPolicySummaries(); + this.hasLoadedPolicies = true; + } } render() { @@ -459,5 +477,8 @@ export default compose( betas: { key: ONYXKEYS.BETAS, }, + currentURL: { + key: ONYXKEYS.CURRENT_URL, + }, }), )(AuthScreens); diff --git a/src/libs/Url.js b/src/libs/Url.js index e166c127a83..0b3c9c48662 100644 --- a/src/libs/Url.js +++ b/src/libs/Url.js @@ -1,3 +1,5 @@ +import Str from 'expensify-common/lib/str'; + /** * Add / to the end of any URL if not present * @param {String} url @@ -10,7 +12,14 @@ function addTrailingForwardSlash(url) { return url; } +function removeLeadingForwardSlash(url) { + if (Str.startsWith(url, '/')) { + return url.slice(1); + } + return url; +} + export { - // eslint-disable-next-line import/prefer-default-export addTrailingForwardSlash, + removeLeadingForwardSlash, }; diff --git a/src/pages/LogInWithShortLivedTokenPage.js b/src/pages/LogInWithShortLivedTokenPage.js index 88265401f24..f00765ac0f6 100644 --- a/src/pages/LogInWithShortLivedTokenPage.js +++ b/src/pages/LogInWithShortLivedTokenPage.js @@ -52,11 +52,6 @@ const defaultProps = { }; class LogInWithShortLivedTokenPage extends Component { - constructor(props) { - super(props); - this.state = {hasRun: false}; - } - componentDidMount() { const accountID = parseInt(lodashGet(this.props.route.params, 'accountID', ''), 10); const email = lodashGet(this.props.route.params, 'email', ''); @@ -69,11 +64,11 @@ class LogInWithShortLivedTokenPage extends Component { } signInWithShortLivedToken(accountID, email, shortLivedToken, encryptedAuthToken); - this.setState({hasRun: true}); } componentDidUpdate() { - if (this.state.hasRun || !this.props.betas) { + const email = lodashGet(this.props.route.params, 'email', ''); + if (!this.props.betas || !this.props.session.authToken || email !== this.props.session.email) { return; } From 807e095633ef651b2d81c3d40fffcb64b7247969 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Mon, 27 Sep 2021 18:20:25 -0700 Subject: [PATCH 2/4] Add method doc for removeLeadingForwardSlash --- src/libs/Url.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libs/Url.js b/src/libs/Url.js index 0b3c9c48662..ffb3f1601f4 100644 --- a/src/libs/Url.js +++ b/src/libs/Url.js @@ -12,6 +12,12 @@ function addTrailingForwardSlash(url) { return url; } +/** + * Remove / from the beginning of any URL if present + * + * @param {String} url + * @returns {String} + */ function removeLeadingForwardSlash(url) { if (Str.startsWith(url, '/')) { return url.slice(1); From 7e040981a7f291d4256e87f6bd031d2aa747c3cd Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Tue, 28 Sep 2021 11:22:18 -0700 Subject: [PATCH 3/4] Switch loadPoliciesBehindBeta to early return --- src/libs/Navigation/AppNavigator/AuthScreens.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 20ec961b535..5390b2e2334 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -262,15 +262,16 @@ class AuthScreens extends React.Component { * - We have not already loaded policies */ loadPoliciesBehindBeta() { - // When removing the freePlan beta, simply load the policyList and the policySummaries in componentDidMount(). - // Policy info loading should not be blocked behind the defaultRooms beta alone. - if (!this.hasLoadedPolicies - && !isTransitionOrNewWorkspacePage(this.props.currentURL) - && (Permissions.canUseFreePlan(this.props.betas) || Permissions.canUseDefaultRooms(this.props.betas))) { - getPolicyList(); - getPolicySummaries(); - this.hasLoadedPolicies = true; + // Do not load policies if we're not in the correct betas or if we're in the process of creating a new policy + if (this.hasLoadedPolicies + || isTransitionOrNewWorkspacePage(this.props.currentURL) + || !Permissions.canUseFreePlan(this.props.betas) + || !Permissions.canUseDefaultRooms(this.props.betas)) { + return; } + getPolicyList(); + getPolicySummaries(); + this.hasLoadedPolicies = true; } render() { From ad860ca1d3db2a12eb40c1d8c8880630828662c2 Mon Sep 17 00:00:00 2001 From: Rory Abraham Date: Tue, 28 Sep 2021 20:23:28 -0700 Subject: [PATCH 4/4] Add comment explanation in shouldComponentUpdate --- src/libs/Navigation/AppNavigator/AuthScreens.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.js b/src/libs/Navigation/AppNavigator/AuthScreens.js index 5390b2e2334..b22db72dd9d 100644 --- a/src/libs/Navigation/AppNavigator/AuthScreens.js +++ b/src/libs/Navigation/AppNavigator/AuthScreens.js @@ -231,6 +231,14 @@ class AuthScreens extends React.Component { return true; } + /* + * We don't want to unnecessarily re-render this component every time the `currentURL` prop changes, + * but we do want to re-render when we're switching from `transition` or `workspace/new` to any other page. + * Re-rendering in that case will re-trigger `componentDidUpdate` and `loadPoliciesBehindBeta`, + * which we only want to do after we're done with the `transition` and `workspace/new` pages. + * If we don't wait to load the policies until after we're done with those pages, + * we may accidentally overwrite the newly-created policy and land on an infinite loading spinner. + */ if (isTransitionOrNewWorkspacePage(this.props.currentURL) && !isTransitionOrNewWorkspacePage(nextProps.currentURL)) { return true; }