Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix infinite spinner on OldDot transition #5552

Closed
wants to merge 4 commits into from
Closed

Fix infinite spinner on OldDot transition #5552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe08bb3
Fix infinite spinner on OldDot transition
roryabraham Sep 28, 2021
807e095
Add method doc for removeLeadingForwardSlash
roryabraham Sep 28, 2021
7e04098
Switch loadPoliciesBehindBeta to early return
roryabraham Sep 28, 2021
ad860ca
Add comment explanation in shouldComponentUpdate
roryabraham Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 45 additions & 15 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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 = {
Expand All @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -235,11 +231,23 @@ 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add some comments here to explain this check? I see that we want to prevent updating AuthScreens when moving from /transition or /workspace/new to another route, but unsure why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, my reasoning here was since I added the currentURL Onyx key, that will change any time we navigate anywhere:

  1. Opening settings modal
  2. Switching reports
  3. Etc...

And by default we probably don't want to re-render the whole component tree from AuthScreens down every time the route changes. The only time we do want to force a re-render and trigger componentDidUpdate is when we are moving from transition or workspace/new to anywhere else, because that means it's now safe to load policies without risking overwriting a newly-created policy. Overall this design doesn't feel great, but I think it works.

If that all makes sense, I could add a comment like this:

/*
 * We don't want to 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. 
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way we could maybe simplify this code without breaking the E/App philosophy would be to trigger this.loadPoliciesBehindBeta directly here in shouldComponentUpdate, but in general I'm hesitant to put side-effects in shouldComponentUpdate. Not sure I can explain why exactly, it just feels like a bad pattern. 馃し

Copy link
Contributor

@nickmurray47 nickmurray47 Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to leaving the above comment for an explanation but I also think it could use a bit of re-working and additional lines to help with clarity. e.g.

We don't want to re-render this component every time the currentURL prop changes because xyz

return true;
}

return false;
}

componentDidUpdate() {
loadPoliciesBehindBeta(this.props.betas);
this.loadPoliciesBehindBeta();
}

componentWillUnmount() {
Expand All @@ -252,7 +260,26 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we just keep calling this method anytime the component updates until all the conditions are met?

Seems fine but also kind of inside out to me. This might be off base, but I feel like we are starting to get into some territory where Onyx's async philosophy is hurting us in the code readability department.

Would there be any worry about a race condition or even a need to explain it if we could have something like...

const promises = [getBetas()];
if (shouldCreateWorkspace) {
    promises.push(createPolicy());
}

Promise.all(promises)
    .then(() => {
        getPolicyList();
        getPolicySummaries();
    });

Not sure if we can actually do this or where this logic would run. We can't do it in componentDidMount() because we are probably on /transition and not /workspace/new when that happens. Then again we don't have to switch to a route called /workspace/new and could instead trigger policy creation with a parameter instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree, and was suggesting something similar in this thread. In cases when we're just displaying a full-screen loading indicator while waiting for things to happen, our Onyx async philosophy doesn't seem to serve us well/leads to overly-complicated code.

I think that this whole flow could be simplified by just consolidating everything into a single /transition page (i.e: no workspace/new). It would just display a loading indicator, and depending on URL params would do any number of things that need to happen before we can actually display the app. It waits for all those things to be done using promises, and then displays the exitTo page or falls back on the homepage (optionally with an error growl).

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this conversation here.

*/
loadPoliciesBehindBeta() {
// 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() {
Expand Down Expand Up @@ -459,5 +486,8 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
currentURL: {
key: ONYXKEYS.CURRENT_URL,
},
}),
)(AuthScreens);
17 changes: 16 additions & 1 deletion src/libs/Url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import Str from 'expensify-common/lib/str';

/**
* Add / to the end of any URL if not present
* @param {String} url
Expand All @@ -10,7 +12,20 @@ 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);
}
return url;
}

export {
// eslint-disable-next-line import/prefer-default-export
addTrailingForwardSlash,
removeLeadingForwardSlash,
};
9 changes: 2 additions & 7 deletions src/pages/LogInWithShortLivedTokenPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', '');
Expand All @@ -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;
}

Expand Down