Fix: Blank screen after creating an account #2575

kidroca · 2021-04-26T17:22:48Z

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>
@marcaaron

Details

Address the problem that no navigator is available during application initialization
We don't need to keep the entire AuthScreens navigator unmounted but just the Report screen
This way the URL still resolves to the correct report after the data is available

Fixed Issues

Fixes #2497

Tests

Start from logged out state and proceed to create a new account
While logged out, type and login with brand new email address so it can trigger sign up flow
Get validation link from e-mail. On dev I'm copying the email link and appending it to localhost - e.g. http://localhost:8080/setpassword/10024706/EHOHMPGWM
Type new password and click "set password"
After a brief loading time you should see an initial chat list in LHN containing only the Concierge chat
- the URL in the address would be /r until the initial report number is resolved
- Depending on internet speed this can be too quick to notice
- In the video samples below it reads /loading but we decided to change it
The Concierge chat should be loaded as the active conversation

Please check the steps listed in the PR here #2346 work as well and there's no regressions

QA Steps

Same as above

Tested On

Screenshots

Web

I'm intentionally setting a slow internet connection to test that the URL (chat) would resolve correctly even when the data comes after the App's initial render

Creating account over a slow internet connection

Expensify.cash.Web.-.Slow.Create.Account.-.Report.And.URL.still.resolve.mp4

Login in over a slow internet connection

Expensify.cash.Web.-.Slow.Login.-.Report.and.URL.still.resolves.mp4

Mobile Web

Logging in on mobile web

Screen.Recording.2021-04-27.at.22.41.33.mov

Desktop

Logging in on the desktop app

Screen.Recording.2021-04-27.at.23.11.10.mov

iOS

A simulated login using the magic link after an account is created

Simulate a magic link to set a password for a new account

iOS.Deep.Link.mp4

Android

Testing on Android to verify everything still works as before

Expensify.cash.Android.-.Sign.In.mp4

Wait for the NavigationContainer and all its children finish mounting for the first time before allowing/performing navigation If there were navigation attempts - the last navigation attempt would be automatically applied when the navigation is enabled

…alReportID Sometimes `currentReportID` might resolve as empty '' and prevent the RootStack navigator from mounting `const currentReportID = firstReportID ? String(firstReportID) : '';` https://github.com/Expensify/Expensify.cash/blob/0da357c30bc1ace6d354a81aabe075f87c12144c/src/libs/actions/Report.js#L688

…hen there are no reports When `response.chatList` is an empty array (new users) - `reportIDs = String(response.chatList).split(',');` Results in `reportIDs` being equal to `['']` and the former control flow `if (reportIDs.length)` evaluated to `true` Not casting the chat list IDs to string is not longer an issue because all the following usages are working with strings as well as with numbers

…tReport` is not used in `reportIDs`

…no router is mounted

kidroca

I think it might be better to remove the conditional rendering in AuthScreens:

https://github.com/kidroca/Expensify.cash/blob/c4d25f5ed8c92131d48adcfa9affeee9baf75c86/src/libs/Navigation/AppNavigator/AuthScreens.js#L170-L174

And let the AuthScreens mount instantly so that stack navigators can swap instantly and a navigator is always available

This would also remove the code inside shouldComponentUpdate:

https://github.com/kidroca/Expensify.cash/blob/c4d25f5ed8c92131d48adcfa9affeee9baf75c86/src/libs/Navigation/AppNavigator/AuthScreens.js#L151-L158

When the app initializes from a stored state it would still load the persisted report
If no report is available in route params by the time the ReportScreen is mounted it can request navigation to transition to the first available report in the chat list

This way most of the changes in this PR, but the bug fixes, will be removed

src/libs/Navigation/Navigation.js

src/libs/actions/Report.js

src/libs/Navigation/AppNavigator/AuthScreens.js

This reverts commit 529f0fb These changes didn't prove as useful as intended

…rk when no router is mounted" This reverts commit c4d25f5 This fix is no longer needed

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

kidroca · 2021-04-27T18:12:25Z

src/libs/actions/Report.js

@@ -950,7 +951,7 @@ function handleReportChanged(report) {
 * @param {Number} reportID
 */
 function updateCurrentlyViewedReportID(reportID) {
-    Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID));
+    Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID || ''));


I've updated this so it can be reused in fetchAllReports and to guard against null , undefined as String(null) would create a "null" (similar situation for undefined and NaN)

Hmm, this is the one thing that I'm not so sure about. If we are calling this with a reportID that is null or undefined why try to set it at all? Couldn't we just leave whatever existing value we have? Seems not ideal to fail silently and set the currentlyViewedReportID to an empty string. Maybe it would be appropriate to log an alert of some kind when this happens instead?

We have some ways to do that e.g.

https://github.com/Expensify/Expensify.cash/blob/f213be0a1be5a04599d2f17d8848b0a7824eb8be/src/libs/actions/Report.js#L717-L720

Hmm, this is the one thing that I'm not so sure about. If we are calling this with a reportID that is null or undefined why try to set it at all?

As you can see the original logic would save anything without any consideration, I've just updated it so that if called with null or undefined they are converted

I'll just revert this, this will also address the used-before-defined problem

Yes, I see. Only mentioning this since logging an alert would perhaps be useful. But no need to add it in this PR.

kidroca · 2021-04-27T18:19:01Z

src/libs/actions/Report.js

-            const currentReportID = firstReportID ? String(firstReportID) : '';
-            Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, currentReportID);
+            // eslint-disable-next-line no-use-before-define
+            updateCurrentlyViewedReportID(firstReportID);


Maybe I should move updateCurrentlyViewedReportID to be declared before this usage so the eslint override can be removed.

About that eslint rule it can be tweaked. There's nothing wrong using a function before it's defined and the rule allows for such configuration. The problem is when you try to use a class or a variable before they are defined.
This style rule is forcing specifics to be defined at the top, which usually degrades readability.

About that eslint rule it can be tweaked.

It has been brought up before. We generally agree with the reasoning laid out here

https://eslint.org/docs/rules/no-use-before-define

kidroca · 2021-04-27T18:30:07Z

Updated

marcaaron · 2021-04-27T18:52:12Z

Thanks, going to let @thienlnam take a first pass at reviewing + testing this then I will jump in.

thienlnam

Changes look good, just a small comment and then going to give it a test

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

kidroca · 2021-04-27T20:25:40Z

I've added sample videos for all platforms:

I don't know how you test deep links on iOS / Android. This is what I do for iOS for example:

iOS.Deep.Link.mp4

create a new account
copy the link from the email
use the link in a terminal to trigger a simulator run:
```
xcrun simctl openurl booted expensify-cash://setpassword/10075909/XGFOJRHBO
```
- note: you have to be logged out from e.cash in order for this particular link to resolve

Maybe something about it can be included in the README

In the event of an error a currently viewed report would never get set This would result in a invalid initial state of the app Moving the logic to execute in `finally` would cover both success and error cases and allow the app to continue and recover

…lved The Report Screen is setup to mount when: - routing information is already available - initial params provide fallback when routing information is missing This would keep the Report Screen mounted in case the `initialReportID` is ever reset to `null` like in the event of clearing Onyx data

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

src/libs/actions/Report.js

kidroca · 2021-04-28T09:15:19Z

Updated

marcaaron

Nothing in this PR is a major blocker for me so I am leaning towards merging so we can unblock the deploy and do a follow up to address any comments. cc @roryabraham

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

src/libs/actions/Report.js

marcaaron · 2021-04-29T00:12:54Z

src/libs/actions/Report.js

-            const currentReportID = firstReportID ? String(firstReportID) : '';
-            Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, currentReportID);
+            // eslint-disable-next-line no-use-before-define
+            updateCurrentlyViewedReportID(firstReportID);


About that eslint rule it can be tweaked.

It has been brought up before. We generally agree with the reasoning laid out here

https://eslint.org/docs/rules/no-use-before-define

marcaaron · 2021-04-29T00:17:03Z

src/libs/actions/Report.js

@@ -950,7 +951,7 @@ function handleReportChanged(report) {
 * @param {Number} reportID
 */
 function updateCurrentlyViewedReportID(reportID) {
-    Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID));
+    Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID || ''));


Hmm, this is the one thing that I'm not so sure about. If we are calling this with a reportID that is null or undefined why try to set it at all? Couldn't we just leave whatever existing value we have? Seems not ideal to fail silently and set the currentlyViewedReportID to an empty string. Maybe it would be appropriate to log an alert of some kind when this happens instead?

We have some ways to do that e.g.

https://github.com/Expensify/Expensify.cash/blob/f213be0a1be5a04599d2f17d8848b0a7824eb8be/src/libs/actions/Report.js#L717-L720

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

src/libs/actions/Report.js

kidroca · 2021-04-29T07:21:13Z

src/libs/actions/Report.js

@@ -950,7 +951,7 @@ function handleReportChanged(report) {
 * @param {Number} reportID
 */
 function updateCurrentlyViewedReportID(reportID) {
-    Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID));
+    Onyx.merge(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, String(reportID || ''));


Hmm, this is the one thing that I'm not so sure about. If we are calling this with a reportID that is null or undefined why try to set it at all?

As you can see the original logic would save anything without any consideration, I've just updated it so that if called with null or undefined they are converted

I'll just revert this, this will also address the used-before-defined problem

src/libs/actions/Report.js

…e.state` usage" This reverts commit a2aa335

…ts ready There's no point in mounting the actual navigator before it has anything to route Added a fallback logic here so that even if `initialReportID` is never set the component would still resolve and function correctly

This reverts commit f394b98

This handling is no longer needed Initial routing data in `MainDrawerNavigator` would resolve with a reportID Then the ReportScreen will store it in Onyx when it is mounted: didMount -> storeCurrentlyViewedReport

When `initialReportId` is not available we can fall back to the last viewed report. BTW `initialReportId` might not be needed at all since it would be storing the last accessed/viewed report

`initialReportID` is always the same as the ID returned by `getLastAccessedReport`

kidroca

With these changes ONYXKEYS.CURRENTLY_VIEWED_REPORTID is no longer needed for initialization purposes.

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

src/libs/actions/Report.js

…ve to navigate to it This would be resolved by initial params Otherwise it still work, but an error is printed out that navigation failed The ReportScreen is not mounted as the change that mounts it happens a few lines before the call to navigate in `fetchOrCreateChatReport`

thienlnam

Nothing glaring is pointing out to me, let's get this deploy out - sending last review to you @marcaaron

roryabraham · 2021-04-29T17:14:27Z

@kidroca, I noticed that you had one approval, and then made substantial changes to this PR. I don't have all the context for why those changes were made, but I did just want to point out that this PR fixes a deploy-blocking issue that has been preventing us from running a production deploy for more than a week now. Generally with deploy blockers we want to get the quickest fix that solves the problem at hand so we're no longer holding up the deploy, even if you then create a follow-up PR to provide a better or more robust solution during the next deploy. Just something to keep in mind for the future 🙂

marcaaron

LGTM, I only have some suggestions about how to improve the documentation around some of the logic in the drawer navigator but no need to block.

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

marcaaron · 2021-04-29T17:08:25Z

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

-            // eslint-disable-next-line react/jsx-props-no-multi-spaces
-            initialParams={{reportID: ''}}
-            options={{
+// Decorated to always returning the result of the first call - keeps Screen initialParams from changing


This comment is a little hard to follow and I might change it to something like:

We are decorating getLastAccessedReport so that it caches the first result once the reports load. This will ensure the initialParams are only set once for the ReportScreen.

Which also begs the question... what happens if the initialParams change? And why do we care? Maybe we can explain that as well here just so it is very obvious?

Nothing will happen if we pass different initialParams after the screen is mounted. And that's why we want to stop calculating them each time MainDrawerNavigator re-renders

It might be better to just remove this _.once optimization and address it if it ever becomes a problem

Nice one, yep I think this is fine in any case but took me a second to get why so a small comment would have been helpful.

src/libs/actions/Report.js

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js

kidroca · 2021-04-29T18:08:25Z

@roryabraham
I saw Marc comment regarding merging this.
It was a good point to explicitly say stop making further changes.

Since it wasn't merged and no one said nothing. I looked at the comments left by Marc and came up with an even more brilliant solution and had the time to implement it.
I always consider how easy would something be to review and wouldn't have done this if the solution wasn't way better
The changes might seem significant but 50% of them are blank lines comments and indentation change

Besides I've left a good description which should have helped reviewing them quickly

OSBotify · 2021-04-29T18:14:26Z

🚀 Deployed to staging in version: 1.0.33-4🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron · 2021-04-30T02:10:18Z

Just a heads up there is an issue with this implementation and it's possible to get into a bad state if you have some partial report data.

Here's what I've observed:

Which, as you can imagine, leads to some pretty broken stuff 😄

That said, I was only able to reproduce this a few times. Still we should be careful to avoid this case and not present any report that has no reportID.

kidroca · 2021-04-30T12:44:52Z

Nice catch @marcaaron

I didn't know that there could be such a report in the collection

I see that this is handled with a reportID check here:

https://github.com/Expensify/Expensify.cash/blob/2ae23e75b9c7b9166fb2a333bfcdd10646508448/src/libs/OptionsListUtils.js#L192-L195

I don't know why incomplete reports aren't stored in a different collection but perhaps we can extract this check to reportUtils for the time being

function isIncompleteReport(report) {
  if (!report || !report.reportID) return true;

  const participants = lodashGet(report, ['participants'], []);
  return _.isEmpty(logins);
}

Then update the code here:

https://github.com/Expensify/Expensify.cash/blob/2ae23e75b9c7b9166fb2a333bfcdd10646508448/src/libs/reportUtils.js#L30-L35

To account for that

function getLastAccessedReport(reports) {
    return _.chain(reports)
        .toArray()
        .filter(report => isIncompleteReport(report) == false)
        .sortBy('lastVisitedTimestamp')
        .last()
        .value();
}

I can't find how a partial report is created to analyze this further, can you point me to when/where this happens?

Added a patch here: #2649

marcaaron · 2021-04-30T16:51:42Z

Yep, perfect plan! I don't quite understand how they are created. It could be a race condition or a failed network request or something else entirely.

OSBotify · 2021-05-08T01:43:19Z

🚀 Deployed to production in version: 1.0.39-5🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

kidroca added 5 commits April 26, 2021 18:16

fix: Report.fetchAllReports - new report created by `fetchOrCreateCha…

da36c16

…tReport` is not used in `reportIDs`

fix: Report.fetchAllReports - Navigation.navigate does not work when …

c4d25f5

…no router is mounted

marcaaron self-requested a review April 26, 2021 17:51

kidroca commented Apr 26, 2021

View reviewed changes

src/libs/Navigation/Navigation.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Show resolved Hide resolved

kidroca commented Apr 26, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/AuthScreens.js Outdated Show resolved Hide resolved

kidroca added 5 commits April 27, 2021 18:42

refactor: Move initialReportID from AuthScreens to MainDrawerNavigator

d6596c5

refactor: Mount the Report Screen only when initial data is resolved

1591d76

refactor: Reuse updateCurrentlyViewedReportID

f394b98

Revert "feat: Navigation navigateLater and enableNavigation"

9faf5e4

This reverts commit 529f0fb These changes didn't prove as useful as intended

Revert "fix: Report.fetchAllReports - Navigation.navigate does not wo…

fdc8259

…rk when no router is mounted" This reverts commit c4d25f5 This fix is no longer needed

kidroca commented Apr 27, 2021

View reviewed changes

kidroca marked this pull request as ready for review April 27, 2021 18:29

kidroca requested a review from a team as a code owner April 27, 2021 18:29

MelvinBot requested review from thienlnam and removed request for a team April 27, 2021 18:29

thienlnam reviewed Apr 27, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js Outdated Show resolved Hide resolved

kidroca added 5 commits April 28, 2021 10:35

refactor: Bring back the cast logic to fetchAllReports

d0a952f

fix: Remove loading from the URL path in the address bar

c71967f

refactor: Replace inline Screen names with values from SCREENS

b20db83

kidroca commented Apr 28, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js Outdated Show resolved Hide resolved

src/libs/actions/Report.js Outdated Show resolved Hide resolved

kidroca requested a review from thienlnam April 28, 2021 09:15

marcaaron previously approved these changes Apr 29, 2021

View reviewed changes

kidroca commented Apr 29, 2021

View reviewed changes

kidroca added 6 commits April 29, 2021 11:55

refactor: remove memo usage

6326456

Revert "refactor: convert MainDrawerNavigator to class to avoid `rout…

3395cda

…e.state` usage" This reverts commit a2aa335

Revert "refactor: Reuse updateCurrentlyViewedReportID"

14df6cb

This reverts commit f394b98

refactor: remove the shouldRedirectToReport and lastReportID setting

fa3eaa2

This handling is no longer needed Initial routing data in `MainDrawerNavigator` would resolve with a reportID Then the ReportScreen will store it in Onyx when it is mounted: didMount -> storeCurrentlyViewedReport

feat: findLastAccessedReport a function to derive the last viewed report

558babb

When `initialReportId` is not available we can fall back to the last viewed report. BTW `initialReportId` might not be needed at all since it would be storing the last accessed/viewed report

kidroca dismissed marcaaron’s stale review via 558babb April 29, 2021 10:41

kidroca force-pushed the kidroca/use-navigation-after-init-is-done branch from d53f945 to 52a3bea Compare April 29, 2021 10:57

refactor: move findLastAccessedReport to reportUtils

f0050f6

When `initialReportId` is not available we can fall back to the last viewed report. BTW `initialReportId` might not be needed at all since it would be storing the last accessed/viewed report

kidroca force-pushed the kidroca/use-navigation-after-init-is-done branch from 52a3bea to f0050f6 Compare April 29, 2021 11:05

refactor: remove initialReportID no longer needed

48f2427

`initialReportID` is always the same as the ID returned by `getLastAccessedReport`

kidroca commented Apr 29, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js Show resolved Hide resolved

src/libs/Navigation/AppNavigator/MainDrawerNavigator.js Show resolved Hide resolved

src/libs/actions/Report.js Show resolved Hide resolved

kidroca requested a review from marcaaron April 29, 2021 11:37

thienlnam approved these changes Apr 29, 2021

View reviewed changes

marcaaron approved these changes Apr 29, 2021

View reviewed changes

marcaaron merged commit cbd78e5 into Expensify:main Apr 29, 2021

OSBotify mentioned this pull request Apr 29, 2021

Deploy Checklist: Expensify.cash 2021-04-13 #2380

Closed

66 tasks

kidroca deleted the kidroca/use-navigation-after-init-is-done branch April 29, 2021 18:27

marcaaron mentioned this pull request Apr 30, 2021

[Hold for payment 5/7] Blank screen after creating an account #2497

Closed

kidroca mentioned this pull request Apr 30, 2021

fix: Incomplete reports would cause the app to crash on startup #2649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Blank screen after creating an account #2575

Fix: Blank screen after creating an account #2575

kidroca commented Apr 26, 2021 •

edited

Loading

kidroca left a comment

kidroca Apr 27, 2021

marcaaron Apr 29, 2021

kidroca Apr 29, 2021

marcaaron Apr 29, 2021

kidroca Apr 27, 2021

marcaaron Apr 29, 2021

kidroca commented Apr 27, 2021

marcaaron commented Apr 27, 2021

thienlnam left a comment

kidroca commented Apr 27, 2021 •

edited

Loading

kidroca commented Apr 28, 2021

marcaaron left a comment

marcaaron Apr 29, 2021

marcaaron Apr 29, 2021

kidroca Apr 29, 2021

kidroca left a comment

thienlnam left a comment

roryabraham commented Apr 29, 2021

marcaaron left a comment

marcaaron Apr 29, 2021

kidroca Apr 29, 2021

marcaaron Apr 29, 2021

kidroca commented Apr 29, 2021

OSBotify commented Apr 29, 2021

marcaaron commented Apr 30, 2021 •

edited

Loading

kidroca commented Apr 30, 2021 •

edited

Loading

marcaaron commented Apr 30, 2021

OSBotify commented May 8, 2021

Fix: Blank screen after creating an account #2575

Fix: Blank screen after creating an account #2575

Conversation

kidroca commented Apr 26, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Creating account over a slow internet connection

Login in over a slow internet connection

Mobile Web

Logging in on mobile web

Desktop

Logging in on the desktop app

iOS

Simulate a magic link to set a password for a new account

Android

kidroca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca commented Apr 27, 2021

marcaaron commented Apr 27, 2021

thienlnam left a comment

Choose a reason for hiding this comment

kidroca commented Apr 27, 2021 • edited Loading

kidroca commented Apr 28, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

roryabraham commented Apr 29, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca commented Apr 29, 2021

OSBotify commented Apr 29, 2021

marcaaron commented Apr 30, 2021 • edited Loading

kidroca commented Apr 30, 2021 • edited Loading

marcaaron commented Apr 30, 2021

OSBotify commented May 8, 2021

kidroca commented Apr 26, 2021 •

edited

Loading

kidroca commented Apr 27, 2021 •

edited

Loading

marcaaron commented Apr 30, 2021 •

edited

Loading

kidroca commented Apr 30, 2021 •

edited

Loading