Fix: Address bar URL is not updated after login or landing on root #2346

kidroca · 2021-04-12T15:16:13Z

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

Details

Update navigation initial state logic.

Fixed Issues

Fixes #2325

Tests

All the steps described here are for web and mWeb. For desktop and the native apps there should be no regressions. E.g. opening the mobile app should still go to the Home screen where you select a conversation.

Best to use Incognito Mode for the below 2 flows so that you're sure something in existing storage doesn't help in passing these steps

Login Flow

Start from logged out state on / (with no traces of previous report in the url)
Log In
Observe the URL updating to the currently loaded report - e.g. /r/71403817

Create Account

Start from logged out state and proceed to create a new account
On dev I'm copying the email link and appending it to localhost - e.g. http://localhost:8080//setpassword/10024706/EHOHMPGWM
After successfully creating an account you should see the URL representing the currently viewed report - e.g. /r/71403817

Opening the app when authenticated

Be logged in
Close the app window/tab
Open a browser window and go to the root address of the app (for me it's localhost:8080)
Observe the URL updating to the currently loaded report - e.g. /r/71403817

Navigating to the root of the app

Be logged in
Edit the address bar to go to the root of the app - e.g. type localhost:8080/
Observe the URL updating to the currently loaded report - e.g. /r/71403817

Refreshing

Be logged in
Refresh the page
The url should not change, it should still show the currently loaded report - e.g. /r/71403817

QA Steps

Same as the tests above

Tested On

Screenshots

Web

Expensify.cash.-.Google.Chrome.2021-04-12.18-13-28.mp4

Mobile Web

Android.Emulator.-.Pixel_2_API_29_5554.2021-04-12.18-14-50.mp4

With the current configuration it should be possible for the navigation to resolve initial state entirely through `linkingConfig`

1. Either initial report data is available right from the start from Onyx 2. Or it is fetched and restored by `fetchAllReports` 3. Or if all else fails start with no report opened

src/libs/Navigation/AppNavigator/AuthScreens.js

src/libs/Navigation/NavigationRoot.js

src/libs/Navigation/AppNavigator/AuthScreens.js

src/libs/Navigation/NavigationRoot.js

kidroca · 2021-04-12T17:59:56Z

Found one more thing that this PR fixes. I don't think it was reported. It's Android only and related to the keyboard issues I've discovered and tracked to Navigation state:

Navigation returns to the same screen/modal on Android

Android.Emulator.-.Pixel_2_API_29_5554.2021-04-12.20-51-32.mp4

I've stumbled on this while testing if my changes also cover: #1855

It looks like this PR together with #2221 address fully the keyboard issues in #1855 and #2150 (and the above that's not yet reported) 🦸

A video summary of all the fixes

Android.Emulator.-.Pixel_2_API_29_5554.2021-04-12.21-06-52.mp4

marcaaron

Just leaving some initial thoughts since this is in draft.

src/libs/Navigation/NavigationRoot.js

src/libs/Navigation/AppNavigator/AuthScreens.js

The Report Screen renders the "currently viewed report" - it can persist it This way we doesn't have to check per each navigation state change

kidroca · 2021-04-13T10:48:02Z

src/pages/home/ReportScreen.js

@@ -37,6 +38,7 @@ class ReportScreen extends React.Component {

        if (reportChanged) {
            this.prepareTransition();
+            this.storeCurrentlyViewedReport();


This replaces the logic here:

https://github.com/Expensify/Expensify.cash/blob/d3f1d28478357e2a66b155e660def191d6ed79a8/src/libs/Navigation/NavigationRoot.js#L104-L109

The ReportScreen renders the last viewed report so we don't need to check per each navigation change, but only save the last report that was viewed in ReportScreen

This change makes sense to me. I don't see how it's different from what we had before (maybe a bit cleaner). Was there any other motivation for the change or just cleaning stuff up?

Primarily clean up, it should still work the same as before. I can revert this you think it might cause some issue down the road

src/pages/home/ReportScreen.js

…-updating-after-login-or-refresh Conflicts: src/libs/Navigation/AppNavigator/AuthScreens.js

kidroca · 2021-04-13T11:09:19Z

Updated.
Merged latest changes from master and resolved conflicts

marcaaron

Changes look great to me just one small comment left about adding a method doc.

marcaaron · 2021-04-13T16:48:57Z

src/pages/home/ReportScreen.js

@@ -37,6 +38,7 @@ class ReportScreen extends React.Component {

        if (reportChanged) {
            this.prepareTransition();
+            this.storeCurrentlyViewedReport();


This change makes sense to me. I don't see how it's different from what we had before (maybe a bit cleaner). Was there any other motivation for the change or just cleaning stuff up?

marcaaron · 2021-04-13T16:49:53Z

src/pages/home/ReportScreen.js

@@ -73,6 +75,11 @@ class ReportScreen extends React.Component {
        this.loadingTimerId = setTimeout(() => this.setState({isLoading: false}), 300);
    }

+    storeCurrentlyViewedReport() {


All our components methods must have a method doc even if they don't take params or return anything.

Avoid docs that don't add any additional information.
Method descriptions are optional and should be added when it's not obvious what the purpose of the method is.
https://github.com/Expensify/Expensify.cash/blob/master/STYLE.md#jsdocs 🤓

This is a class method and I'll just be adding this as a description: "Persists the currently viewed report id". I could extend it to "...with Onyx" but this is just an implementation detail

Anyway this is not the first time, someone would bring up func/method documentation. In fact it is always the case that I should add documentation so you might consider updating the guide - I would have just always added documentation if I hadn't read the above in the style guide.

Thanks for calling that out. I will update it to be more clear.

kidroca · 2021-04-13T17:51:36Z

Updated

marcaaron

LGTM! 👍

I'm not sure if we need/want to do anything about this yet. But an interesting side effect of this change is that on a fresh sign in the report with Concierge loads first on web, but the header component does not say we are chatting with Concierge.

My guess would be this happens because the HeaderView is still waiting for the personalDetails to be set. And ideally we would wait for both of these things before showing the very first report.

Compared to the main branch where we wait (but also never redirect which is what we are fixing)

I'd propose making a follow up issue for this.

OSBotify · 2021-04-13T19:50:56Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

kidroca · 2021-04-13T20:12:52Z

Strange, on my sample video for web above - the header information loads/updates as well. Though I made a new user and there's only the Concierge chat. I'll have a look tomorrow

marcaaron · 2021-04-13T21:04:50Z

Perhaps because there are very few users and everything loads fast.

kidroca · 2021-04-14T07:32:24Z

@marcaaron

I'm not sure if we need/want to do anything about this yet. But an interesting side effect of this change is that on a fresh sign in the report with Concierge loads first on web, but the header component does not say we are chatting with Concierge.

I thought you mean the header never updated and stayed the way it is in the your screenshot
I'm just saying that it eventually loads

It's not a side effect of this PR or the loader addition - it was always the case that elements would render in their own time as soon as they have data:

A video from `1359f9f` before any of the loader/url fixes

Expensify.cash.-.Google.Chrome.2021-04-14.10-17-01.mp4

the header render blank then loads

A video from current state `d10d52c`

Expensify.cash.-.Google.Chrome.2021-04-14.10-11-58.mp4

To fine tune loading we could replace the timeout logic with state change polling like it's done here: kidroca/Expensify.cash@ae376b4

marcaaron · 2021-04-14T16:10:28Z

it was always the case that elements would render in their own time as soon as they have data

Apologies, sounds like the case here. To clarify, I didn't mean to suggest it should be fixed as a direct follow up or anything (but looks like it re-reading my comment 🤦). Perhaps more accurate to say the effect or more noticeable/pronounced now.

isagoico · 2021-04-14T19:26:14Z

@marcaaron Question about QAing this. I think we're only able to execute the steps in Web and mWeb to be able to check the URL. Is there a way to test this on Apps (android, iOS and desktop) or should we go ahead with just web/mweb?

marcaaron · 2021-04-14T20:17:38Z

Thanks @isagoico, it should be testable by using deep links. We're using universal links so in most cases if we can reach a link like https://expensify.cash/settings then as long as the mobile app is installed when opened from a device it should deep link to the correct place in the app.

marcaaron · 2021-04-14T20:18:20Z

Of course, there is no "url" to verify but we can see if things are working at least.

isagoico · 2021-04-14T21:20:37Z

@marcaaron Links are not working for the iOS app. It only open mWeb and doesn't show the option to open in cash app 🤔 Android is working as expected. Should I go ahead an open a separate issue for this?

isagoico · 2021-04-14T22:00:03Z

The Create Account scenario is failing because of this issue:

Sign Up - Blank screen after setting the password

Expected result

User proceeds to expensify.cash chat list

Actual Result

User is redirected to a blank screen and a JS error can be seen in the console

Action Performed

Open staging E.cash URL
While logged out, type and login with brand new email address so it can trigger sign up flow
Get validation link from e-mail, change it to staging accordingly
Type new password and click "set password"

Platform

Issue is confirmed in:

Android ✔️
Web ✔️

Build: 1.0.21-0
Notes/images/video:
Issue is not reproducible in production

Video
Console log

isagoico · 2021-04-14T22:08:05Z

I'm not 100% sure if this PR is the one responsible for this behaviour so please let me know if it's not the case so I can open a separate issue :)

marcaaron · 2021-04-14T22:27:51Z

Not sure, but I'm not able to reproduce that issue locally.

kidroca · 2021-04-15T10:24:14Z

I'm not 100% sure if this PR is the one responsible for this behaviour so please let me know if it's not the case so I can open a separate issue :)

I think it kinda is 😭

@marcaaron
It probably doesn't happen all the time but in the shared video you can see the line of code in Report.js where the exception was triggered: Report.js:601

I've traced this to

AuthScreens mounts
Invokes fetchAllReports -> Reports.fetchAll
Reports.fetchAll -> actually calls Reports.fetchChatReports
There's a case that this leads to Reports.fetchOrCreateChatReport
Which will execute the code on line 601

This would happen milliseconds before fetchAll sets CURRENTLY_VIEWED_REPORTID and mount the RootStack

It doesn't have to perform navigation for this specific case, as it's being resolved from initial params, but I see the function is used on a couple of other places, to create a new chat and redirect navigation to it.

For the moment it can be addressed as suggested here: https://reactnavigation.org/docs/navigating-without-navigation-prop/#handling-initialization

If you try to navigate without rendering a navigator or before the navigator finishes mounting, it will throw and crash your app if not handled. So you'll need to add an additional check to decide what to do until your app mounts.

We can update Navigation.navigate so that it doesn't try to perform navigation while no Router is mounted. This will address the case and other errors that might result from this brief period when no navigator is mounted

An alternative solution might be to only switch to AuthScreens when initial data is ready, so the swap between navigators can be instant, but this means some of the fetching would need to be moved out of there.
And it still doesn't guarantee if navigation event was triggered from the Public stack but in the meantime we're switching to the Auth stack what would happen...

Or maybe simply calling Onyx.set(ONYXKEYS.CURRENTLY_VIEWED_REPORTID, reportId); before the Navigation.navigate on 601 would be enough - confirmed this works too, though it might be better to let the method fetchOrCreateChatReport return data - this way the parent can decide what to do with it

marcaaron · 2021-04-15T17:28:39Z

Ah interesting. I see what you're saying here. So we need to do something like:

Allow NavigationContainer to initialize with no report
Record when the NavigationContainer is ready
Allow navigation attempts once things are "ready"

I'm not sure what would be best in this case, but one possibly naive idea would be to:

add on onReady={onNavigatorReady} to the NavigationContainer and the callback would be exported from Navigation. If any attempts to navigate are made before called we can put the route in a temp variable and then call them once the navigator is ready.

import {onNavigationContainerReady} from '/Navigation'

<NavigationContainer
    onReady={onNavigationContainerReady}

let lastAttemptedRoute;
let isReady = false;

function navigate(route) {
    if (!isReady) {
        lastAttemptedRoute = route;
        return
    }
    ...
}

function onNavigationContainerReady() {
    isReady = true;
    navigate(lastAttemptedRoute);
}

kidroca · 2021-04-15T19:30:20Z

In their documentation they would just skip doing anything. Your solution seem an improvement over that. Looks like it would work

It might be a bit too clever but I you can do something like this:

let lastAttemptedRoute;

function navigate(route) {
 // unmodified
}

function navigateLater(route) {
  lastAttemptedRoute = route;
}

function onNavigationContainerReady() {
    navigate(lastAttemptedRoute);
    exports.navigate = navigate;
}

const exports = {
  navigate: navigateLater,
  onNavigationContainerReady,
};

export default exports;

Because that check is only needed initially but will be performed for the lifetime of the Navigation
Also it gets in the way of reading the actual code

github-actions · 2021-04-15T19:30:39Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

marcaaron · 2021-04-15T19:47:46Z

Yep I like that improvement.

OSBotify · 2021-04-19T22:41:56Z

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

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

kidroca · 2021-04-20T16:26:12Z

Hey @marcaaron I'm not sure if you expect anything more from me? Should I open another PR add work on a fix?

marcaaron · 2021-04-20T16:55:52Z

If you want to create an issue for this follow up we can make a new job out of it ?

kidroca · 2021-04-20T17:44:05Z

That would be great!
I can use the requirements you listed above and make a ticket: #2346 (comment)

marcaaron · 2021-04-20T17:55:12Z

Sounds good thanks :)

roryabraham · 2021-04-23T01:03:56Z

Hi guys! Just noticing that this pull request is still marked as a deploy blocker.

@isagoico, can you please confirm if the issue describe here is still present on staging but not production? If so, I think we should create a new issue and get it resolved ASAP. Thanks!

kidroca · 2021-04-23T05:14:17Z

The new issue is already created, the link is just above your comment #2497

roryabraham · 2021-04-23T16:53:00Z

Thanks @kidroca, I am going to remove the DeployBlockerCash label from this pull request and get it added to that other issue.

kidroca added 4 commits April 12, 2021 16:05

refactor: NavigationRoot extract renderFallback

d1a2aa5

refactor: NavigationRoot extract change handler

ceac113

refactor: NavigationRoot remove Linking handling

0c53948

With the current configuration it should be possible for the navigation to resolve initial state entirely through `linkingConfig`

feat: AuthScreens setup initial Home screen params

d47683e

1. Either initial report data is available right from the start from Onyx 2. Or it is fetched and restored by `fetchAllReports` 3. Or if all else fails start with no report opened

kidroca commented Apr 12, 2021

View reviewed changes

kidroca mentioned this pull request Apr 12, 2021

[HOLD] Going back to main list after keyboard call won't close keypad #2150

Closed

kidroca marked this pull request as ready for review April 12, 2021 18:10

kidroca requested a review from a team as a code owner April 12, 2021 18:10

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

marcaaron reviewed Apr 12, 2021

View reviewed changes

marcaaron assigned kidroca Apr 12, 2021

kidroca added 3 commits April 13, 2021 12:31

refactor: AuthScreens address PR comments

0df407f

refactor: NavigationRoot address PR comments

5311e74

refactor: NavigationRoot move viewed report persistence logic

afd5a06

The Report Screen renders the "currently viewed report" - it can persist it This way we doesn't have to check per each navigation state change

kidroca commented Apr 13, 2021

View reviewed changes

Merge remote-tracking branch 'e.cash/master' into kidroca/fix-url-not…

b6872ae

…-updating-after-login-or-refresh Conflicts: src/libs/Navigation/AppNavigator/AuthScreens.js

kidroca mentioned this pull request Apr 13, 2021

Update timezone to update if user has it set to automatic #2318

Merged

5 tasks

kidroca requested a review from marcaaron April 13, 2021 14:22

marcaaron requested changes Apr 13, 2021

View reviewed changes

docs: ReportScreen add method documentation

e6ef81f

kidroca requested a review from marcaaron April 13, 2021 17:51

marcaaron approved these changes Apr 13, 2021

View reviewed changes

marcaaron merged commit 82a2898 into Expensify:master Apr 13, 2021

kidroca mentioned this pull request Apr 13, 2021

fix: ReportScreen is blank, indefinitely loading on web/desktop #2322

Merged

5 tasks

OSBotify mentioned this pull request Apr 13, 2021

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

Closed

66 tasks

isagoico added the DeployBlockerCash This issue or pull request should block deployment label Apr 14, 2021

kidroca deleted the kidroca/fix-url-not-updating-after-login-or-refresh branch April 15, 2021 09:07

kidroca mentioned this pull request Apr 20, 2021

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

Closed

roryabraham removed the DeployBlockerCash This issue or pull request should block deployment label Apr 23, 2021

kidroca mentioned this pull request Apr 28, 2021

Fix: Blank screen after creating an account #2575

Merged

5 tasks

Fix: Address bar URL is not updated after login or landing on root #2346

Fix: Address bar URL is not updated after login or landing on root #2346

Conversation

kidroca commented Apr 12, 2021 • edited Loading

Details

Fixed Issues

Tests

Login Flow

Create Account

Opening the app when authenticated

Navigating to the root of the app

Refreshing

QA Steps

Tested On

Screenshots

Web

Mobile Web

kidroca commented Apr 12, 2021 • edited Loading

Navigation returns to the same screen/modal on Android

A video summary of all the fixes

marcaaron left a comment

Choose a reason for hiding this comment

kidroca Apr 13, 2021

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

kidroca Apr 13, 2021

Choose a reason for hiding this comment

kidroca commented Apr 13, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

kidroca Apr 13, 2021

Choose a reason for hiding this comment

marcaaron Apr 13, 2021

Choose a reason for hiding this comment

kidroca commented Apr 13, 2021

marcaaron left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 13, 2021

kidroca commented Apr 13, 2021 • edited Loading

marcaaron commented Apr 13, 2021

kidroca commented Apr 14, 2021 • edited Loading

A video from 1359f9f before any of the loader/url fixes

A video from current state d10d52c

marcaaron commented Apr 14, 2021 • edited Loading

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

marcaaron commented Apr 14, 2021

isagoico commented Apr 14, 2021

isagoico commented Apr 14, 2021

Sign Up - Blank screen after setting the password

Expected result

Actual Result

Action Performed

Platform

isagoico commented Apr 14, 2021

marcaaron commented Apr 14, 2021

kidroca commented Apr 15, 2021 • edited Loading

marcaaron commented Apr 15, 2021

kidroca commented Apr 15, 2021

github-actions bot commented Apr 15, 2021 • edited Loading

marcaaron commented Apr 15, 2021

OSBotify commented Apr 19, 2021

kidroca commented Apr 20, 2021 • edited Loading

marcaaron commented Apr 20, 2021

kidroca commented Apr 20, 2021

marcaaron commented Apr 20, 2021

roryabraham commented Apr 23, 2021

kidroca commented Apr 23, 2021 • edited Loading

roryabraham commented Apr 23, 2021

kidroca commented Apr 12, 2021 •

edited

Loading

kidroca commented Apr 12, 2021 •

edited

Loading

kidroca commented Apr 13, 2021 •

edited

Loading

kidroca commented Apr 14, 2021 •

edited

Loading

A video from `1359f9f` before any of the loader/url fixes

A video from current state `d10d52c`

marcaaron commented Apr 14, 2021 •

edited

Loading

kidroca commented Apr 15, 2021 •

edited

Loading

github-actions bot commented Apr 15, 2021 •

edited

Loading

kidroca commented Apr 20, 2021 •

edited

Loading

kidroca commented Apr 23, 2021 •

edited

Loading