[$1000] Skeleton UI is displayed when clicking on any chat reported by @thesahindia

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open app
2. Click on any chat

Expected Result:

Chat should be loading instantly

Actual Result:

Skeleton UI appears

Workaround:

Refresh the page or close and reopen the app (Native apps)

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.26-0
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

az_recorder_20221110_084137.1.mp4

Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1668015943860829

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[JmillsExpensify]

Taking this one, because this is a pretty bad regression and I'd like to raise the price to $1000 immediately.

[JmillsExpensify]

Calling all contributors, we need your help resolving this performance regression! Upwork job is here: https://www.upwork.com/jobs/~01d434cec2c2807231. Compensation starts at $1,000.

[melvin-bot]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[melvin-bot]

Triggered auto assignment to @dangrous (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[JmillsExpensify]

@dangrous Can you take at look at this issue and the related Slack thread. We need to figure out a couple of things, though I'm keeping the external label on this issue until we rule that out.

• Any theories as to how this regression got introduced?
• Do you think external is appropriate, or do we need to take internal?

And that's probably it for now. Thanks!

[dangrous]

Will investigate!

[dangrous]

Just putting the info I found so far here as well as in Slack:

This appears to be caused not by any slow responses from the API, but from a long delayed call to the API - AuthenticatePusher is sent about 3-4 seconds (on web) after the previous API call, and possibly never on iOS. Either OpenReport is not called until after that response is returned, or it's called at the same time (but therefore is still delayed). As soon as that response comes, the UI loads in.

Do you think external is appropriate, or do we need to take internal?

• It's likely this is an issue with the app, not with the API, since there are no outstanding requests during the delay period, and so External is fine.

Any theories as to how this regression got introduced?

• One potential cause might be this PR which actively (on purpose) delays calling subscribeToReportTypingEvents because AuthenticatePusher - part of that function - was being called too early when a new report is created. However, I'm not sure why that would cause this behavior, but the timeline and commands do match up, so it's worth looking into.

EDIT: On refresh, we still get a delay before calling OpenReport (which causes the long loading UI) but AuthenticatePusher isn't delayed. So maybe it's related but not the cause. You can see the delay here:

[dangrous]

@mollfpr This could potentially be a regression from #12365 so wanted to see if you had any thoughts!

[s77rt]

@dangrous
I think this is due to OpenApp delay, so maybe it's indeed internal?

One thing i can add is this (not directly related to this issue):

diff --git a/src/pages/home/ReportScreen.js b/src/pages/home/ReportScreen.js
index 7d6828b01..3e3a300f2 100644
--- a/src/pages/home/ReportScreen.js
+++ b/src/pages/home/ReportScreen.js
@@ -159,7 +159,7 @@ class ReportScreen extends React.Component {
 
         // This is necessary so that when we are retrieving the next report data from Onyx the ReportActionsView will remount completely
         const isTransitioning = this.props.report && this.props.report.reportID !== reportIDFromPath;
-        return reportIDFromPath && this.props.report.reportID && !isTransitioning && !isLoadingInitialAppData;
+        return reportIDFromPath && this.props.report.reportID && !isTransitioning && isLoadingInitialAppData === false;
     }
 
     fetchReportIfNeeded() {

initially isLoadingInitialAppData will be undefined and !undefined is true which is not the desired results. Comparing with false is safer.

[s77rt]

Changing the chat will also trigger the skeleton but the chat gets loaded normally after calling OpenReport
I'm unable to consistently reproduce the reported issue. I was only able to reproduce it with the case of delayed OpenApp response and just few times

[mollfpr]

@dangrous

I think the skeleton view loader is not depending on the AuthenticatePusher response.

We use !this.isReportReadyForDisplay() to show the skeleton view. The isReportReadyForDisplay value depends on isLoadingInitialAppData which is from the OpenApp success response and isTransitioning where the report data load from the OpenReport success response.

App/src/pages/home/ReportScreen.js

Lines 148 to 163 in ae385f9

	/**
	* When false the ReportActionsView will completely unmount and we will show a loader until it returns true.
	*
	* @returns {Boolean}
	*/
	isReportReadyForDisplay() {
	const reportIDFromPath = getReportID(this.props.route);
	// We need to wait for the initial app data to load as it tells us which reports are unread. Waiting will allow the new marker to be set
	// correctly when the report mounts. If we don't do this, the report may initialize in the "read" state and the marker won't be set at all.
	const isLoadingInitialAppData = this.props.isLoadingInitialAppData;
	// This is necessary so that when we are retrieving the next report data from Onyx the ReportActionsView will remount completely
	const isTransitioning = this.props.report && this.props.report.reportID !== reportIDFromPath;
	return reportIDFromPath && this.props.report.reportID && !isTransitioning && !isLoadingInitialAppData;
	}

[mananjadhav]

My take here:

The issue is happening due to isLoadingInitialAppData being true and hence isReadyForDisplay is false.

Because I am not able to consistently reproduce this on Web, I haven't debugged the API response. But the OpenApp API is returning 407 (NOT_AUTHENTICATED), which should probably get into failureData of the API call.

I saw this part inSaveResponseInOnyx.

} else if (responseData.jsonCode !== 200 && request.failureData) {
                    Onyx.update(request.failureData);
                }

Logs I captured:

[info] Making API request - {"command":"OpenApp"}
DEBUG  [info] Finished API request - {"command":"OpenApp","jsonCode":407,"requestID":"76845a4cdff5f35f-BOM"}

LOG  Flag Info {"isReadyForDisplayFlag": false, "isLoadingInitialAppData": true, "isTransitioning": false, "reportID": "77921477", "reportIDFromPath": "77921477"}

What I need to check is if failureData is set when we get 407 Unauthenticated request error.

I'll try to dig further but meanwhile if it helps identify the issue further.

[mollfpr]

Proposal

I can reproduce this with a high-traffic account applausetester+0704sveta@applause.expensifail.com. The issue workarounds don't resolve the issue.

I think it's a regression from #12169 where we update the Onyx response data first then the success data.

App/src/libs/Middleware/SaveResponseInOnyx.js

Lines 16 to 30 in ae385f9

	// First apply any onyx data updates that are being sent back from the API. We wait for this to complete and then
	// apply successData or failureData. This ensures that we do not update any pending, loading, or other UI states contained
	// in successData/failureData until after the component has received and API data.
	const onyxDataUpdatePromise = responseData.onyxData
	? Onyx.update(responseData.onyxData)
	: Promise.resolve();
	onyxDataUpdatePromise.then(() => {
	// Handle the request's success/failure data (client-side data)
	if (responseData.jsonCode === 200 && request.successData) {
	Onyx.update(request.successData);
	} else if (responseData.jsonCode !== 200 && request.failureData) {
	Onyx.update(request.failureData);
	}
	});

Screen.Recording.2022-11-11.at.13.46.42.mov

In my case, the personalDetails response data has a value of around 5817 data (even my laptop can't expand the object). I guess this also makes the API OpenApp take a long time to return the response.

If I'm excluding the personalDetails key when updating the Onyx response data, the success data from OpenApp is set immediately.

So my suggestion is, we should separate the personalDetails data from the OpenApp API response. We should create another API for fetching the personalDetails after the OpenApp response is resolved. In this way, we will prioritize other data responses from the OpenApp API.

[JmillsExpensify]

@Santhosh-Sellavel thoughts on this latest proposal?

[s77rt]

I think this still needs more investigation. I can (randomly) reproduce the issue even with a barely new account.

[mananjadhav]

@mollfpr You might be onto something, but as @s77rt I can also reproduce this on low traffic account. I am unable to reproduce every time. Of 3-4 times I reproduced, I've waited for like upto 3-4 mins to see if it loads something but it doesn't and always fails with 407 auth code. It only works if I reload/refresh the app.

[Santhosh-Sellavel]

I faced this issue with both low/high traffic accounts, but this has the most impact on high-traffic accounts which is for most of us (for our personal accounts as well)!

So my suggestion is, we should separate the personalDetails data from the OpenApp API response. We should create another API for fetching the personalDetails after the OpenApp response is resolved. In this way, we will prioritize other data responses from the OpenApp API.

Thanks, @mollfpr, I like the idea. If you have a full proposal please submit let's test this one and move this forward quickly!

[mollfpr]

I'm able to reproduce it with App that has a session expired. To repro this in the DEV I'm customizing the authToken parameters, so the response from OpenApp will have jsonCode=407.

App/src/libs/Network/enhanceParameters.js

Lines 33 to 38 in 0d653ca

	export default function enhanceParameters(command, parameters) {
	const finalParameters = {...parameters};
	if (isAuthTokenRequired(command) && !parameters.authToken) {
	finalParameters.authToken = NetworkStore.getAuthToken();
	}

if (isAuthTokenRequired(command) && !parameters.authToken) {
        finalParameters.authToken = NetworkStore.getAuthToken() + 'test';
    }

The thing that I found is OpenApp Onyx failure data is never updated to Onyx because turns out the response data is undefined on SaveResponseInOnyx where we not updating the Onyx.

App/src/libs/Middleware/SaveResponseInOnyx.js

Lines 11 to 14 in 0d653ca

	// Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and responseData undefined)
	if (!responseData) {
	return;
	}

So what I understand is, when the jsonCode from the API response is 407, it will catch in Reauthentication middleware. When OpenApp session is expired, it's indeed called the Reauthenticate API and will set the new authToken to the Onyx session. But in OpenApp we use the API request type as read, where after Reauthenticate is resolved, there's a condition to check if the request type is read run another function and resolve the promise but without sending back the API response data.

App/src/libs/Middleware/Reauthentication.js

Lines 87 to 90 in 0d653ca

	if (apiRequestType === CONST.API_REQUEST_TYPE.READ) {
	NetworkConnection.triggerReconnectionCallbacks('read request made with expired authToken');
	return Promise.resolve();
	}

Because of the promise resolve from Reauthenicate not passing the response data, the OpenApp doesn't have a chance to update the failure data.

We should update the return promise with the response data.

diff --git a/src/libs/Middleware/Reauthentication.js b/src/libs/Middleware/Reauthentication.js
index bf1bbad1a..dab836f56 100644
--- a/src/libs/Middleware/Reauthentication.js
+++ b/src/libs/Middleware/Reauthentication.js
@@ -86,7 +86,7 @@ function Reauthentication(response, request, isFromSequentialQueue) {

                         if (apiRequestType === CONST.API_REQUEST_TYPE.READ) {
                             NetworkConnection.triggerReconnectionCallbacks('read request made with expired authToken');
-                            return Promise.resolve();
+                            return Promise.resolve(data);
                         }

                         MainQueue.replay(request);

Before

UI.Skeleton.-.Before.mov

After

UI.Skeleton.-.After.mov

[mollfpr]

That also explains why the App works again after refreshing the page or closing the App. Because the OpenApp will be called again with the new authToken and the success data is updated to Onyx.

[fedirjh]

Proposal

Edit : my proposal will only fix this issue #12574 so I posted it in other thread , i agree with @mollfpr proposal #12635 (comment) , there is an update that will not be triggered for OpenApp command when session is expired

164f309#diff-b900fb367aed6ac57460755de185e8cb616f156a58fbc167b948a700e10889ff this introduced the change and caused the miss update

[Santhosh-Sellavel]

Thanks, @fedirjh, that makes sense!

[Santhosh-Sellavel]

cc: @dangrous @mollfpr's proposal looks good!

[dangrous]

Cool! @mollfpr's proposal makes sense to me too.

@JmillsExpensify just wanted to run by you as well since I want to make sure I'm not missing any context or side effect trickiness from not knowing the codebase as well. Let us know and then we can get this approved, PRd, and merged!

[marcaaron]

The proposal by @mollfpr definitely seems like a much bigger bug.

Mind if we slow down a bit?

For this issue specifically, my read is that there is likely a relationship to slow API calls since, yes, the report page will wait for OpenApp. But the expectation is that the "cached" report comments are shown immediately. That's the behavior that was reported to have "regressed". And I think we maybe introduced it by waiting for the report actions to load from the server in my PR here.

I added that code so that the "new markers" would work correctly - so we can revert it, but cause a regression to the new markers.

I think any proposal here should include that consideration.

[JmillsExpensify]

For this issue specifically, my read is that there is likely a relationship to slow API calls since, yes, the report page will wait for OpenApp. But the expectation is that the "cached" report comments are shown immediately. That's the behavior that was reported to have "regressed".

I like slowing down. I also agree that the regression seems to have occurred around not showing anything cached immediately, which is a big problem given that our design is Offline First.

[marcaaron]

Pretty sure this is a dupe of this issue -> #12574

I'm reverting my PR here to fix the new marker since I think that's where the trouble started. It does look like there's maybe some other issue related to authentication happening being discussed here. But we should explore it separately since those changes could have far reaching implications.

[JmillsExpensify]

Oh thanks for making the connection! Ok, so let's align in this issue what to do for the authentication issue for the time being then, sound good? I can update the title and what not, though we should also really decide if we're going to do anything about this at the moment. These bugs are tricky, as you rightly note. If we aren't going to move forward on the authentication side, then probably better to close out this issue.

[marcaaron]

If we aren't going to move forward on the authentication side

It does sound like the cases where chats are not loading at all are due to an expired authToken. I can add those changes into my PR and test it out to get this done quickly. @mollfpr we'll of course compensate you for spotting this.

[JmillsExpensify]

Cool. I also think we do need to circle back on auth more holistically, as this general consideration keeps biting us.

[marcaaron]

Looked into Authentication issues more and I am not seeing where the connection is WRT the skeleton UI issue. It's clear something broke re-authentication at some point. But if your authToken expires today you will just get logged out (definite regression - but I think not recent).

[marcaaron]

this general consideration keeps biting us

Agree

[JmillsExpensify]

Alright, so coming back to this one. I think we need to circle back and close this issue, yeah? It being a dupe of #12574 and handled via your PR revert?

[marcaaron]

Yeah I think so. But with the caveat that @mollfpr may be onto something with the authentication issues.

I think we should:

1. Close this
2. Someone please report a clear and reproducible bug for the re-authentication issue and we'll create a new one to look into that.

[JmillsExpensify]

Ok that sounds like a plan! @mollfpr you probably have the best context on this issue, would appreciate you reporting in #expensify-bugs.