Handle pagination errors in chats #40610

janicduplessis · 2024-04-19T19:54:16Z

Details

Currently we do not handle errors properly in getOlderActions and getNewerActions. All we do is set the loading state back to false, but that will cause the list onEndReached or onStartReached to be called again and a new network request to load more will be triggered. If we are near any edge of the chat, or even worse if we have a chat with few messages we will trigger these requests in a loop.

To solve this we should add an error state to those pagination methods and display an error UI with an option to retry loading. This will prevent this request loop from happening.

Here's the UI I implemented for this error state

Top error:

Bottom error:

Example with comment linking:

Screen.Recording.2024-04-19.at.16.39.20.mov

Fixed Issues

$ #40641
PROPOSAL:

Tests

Verify that no errors appear in the JS console
Test error state by failing pagination requests adding the following code in HttpUtils.ts here

if (url.includes('GetNewerActions') || url.includes('GetOlderActions')) {
    return {jsonCode: CONST.JSON_CODE.EXP_ERROR};
}

Test comment linking

Offline tests

QA Steps

Verify that no errors appear in the JS console
Test error state by failing pagination requests adding the following code in HttpUtils.ts here. It might also be possible to simulate this by turning off internet after loading a chat.

if (url.includes('GetNewerActions') || url.includes('GetOlderActions')) {
    return {jsonCode: CONST.JSON_CODE.EXP_ERROR};
}

Test comment linking

Go in a long chat and scroll to an old message
Right click and "Copy link"
Go in another chat and paste the link
Clear the cache (Account Settings -> Troubleshoot -> Clear cache and restart)
Go back to the chat with the link and click it
You will be in the middle of a chat and can test loading older / newer messages

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-04-19.at.16.39.20.mov

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2024-04-19T19:54:44Z

@Gonals Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

janicduplessis · 2024-04-19T19:59:05Z

src/pages/home/report/ReportActionsView.tsx


-    const loadNewerChats = useCallback(() => {
-        if (isLoadingInitialReportActions || isLoadingOlderReportActions || network.isOffline || newestReportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {


I noticed this was checking isLoadingOlderReportActions which I assume was an error since this is loadNewerChats, but maybe someone can confirm that this change is ok.

janicduplessis · 2024-04-19T20:02:31Z

Is someone familiar with this code? I noticed the loading indicator for newer messages never seem to show even when comment linking in the middle of a chat that has new messages to load. This also causes the error state not to show since I'm implementing it in ListBoundaryLoader, which I think is the ideal place.

To test it I am currently bypassing the check.

janicduplessis · 2024-04-19T20:05:24Z

@mountiny Do you know if there is an issue related to this already? Should I create one?

This is currently coming from this convo on slack.

janicduplessis · 2024-04-19T20:11:55Z

Might also need help here with design and texts, not sure who to tag and I can't seem to add the Design label.

mountiny · 2024-04-20T16:35:20Z

@janicduplessis I dont think there is one, feel free to create it please.

Then we can add design label there too to ensure the looks are as we expect

janicduplessis · 2024-04-20T17:00:42Z

@mountiny Thanks! Done #40641

mountiny · 2024-04-20T18:28:19Z

@dannymcclain would you be able to help with the design on this PR? thanks!

dannymcclain · 2024-04-22T15:58:24Z

Going to cc @Expensify/design in here as well. How do you all feel about the Couldn't load UI?

A couple thoughts from me:

I think the message text should be text-supporting color
I think I would propose swapping the button for a spinner when you tap Try again instead of swapping out the whole message for the wall of skeleton loaders—it's kinda weird when the whole message disappears, a bunch of skeleton loaders show up, and then it comes back.

I realize this is unlikely to happen "in real life" as much as it during me testing this, but still, I think it's pretty weird. I would rather the Try again button just change to a spinner, and then if we still can't load, it changes back to the button. If we CAN load new messages, the new messages just replace the whole error message block.

CleanShot.2024-04-22.at.10.48.07.mp4

janicduplessis · 2024-04-22T16:21:37Z

@dannymcclain Thanks, I like the suggestion, will look at making those changes this week.

shawnborton · 2024-04-22T16:22:59Z

Hmm before diving into the UI details, I would be curious to know what exactly causes this? Whatever it is, why can't we solve the root of the problem instead of dressing up the symptoms with nice UI?

That being said, I love everything Danny did. But I am just a bit confused why we would ever need to even show something like this or how it would happen to a real user. I've never seen this happen in any of my other chat apps.

@quinthar since you reported this issue - is this how you would expect to handle it?

shawnborton · 2024-04-22T16:24:40Z

Maybe said another way, why exactly does the "Try again" button do to fix the pagination error? Why wouldn't we just do that automatically for the user instead of making them tap a button?

janicduplessis · 2024-04-22T19:34:02Z

The network requests can fail for various reasons, whether it be a network issue, or some outage on our end. This happened recently to both @mountiny and @quinthar, with the current behaviour the requests are retried in a loop so this is why I suggested introducing an error ui. I’m pretty sure I’ve seen similar in other chat apps before.

dannymcclain · 2024-04-22T19:37:35Z

with the current behaviour the requests are retried in a loop

@shawnborton This is shown in the attached video here. I've been seeing it a lot in the product too—it's super disorienting and there's seemingly no way to make it stop other than quitting the app and reopening it. So this Try again UI feels like a big improvement over those crazy flashing skeletons.

shawnborton · 2024-04-22T19:38:55Z

Yeah that's fair, but why does the Try again button work and the automatic retry doesn't?

janicduplessis · 2024-04-22T19:43:20Z

The main goal is to let the user know there is a problem, and avoid spamming our servers with network requests. Currently it just keeps flashing the loading skeleton while immediately retrying the request. If we show some ui it stops the loop and warn the use about the problem. They can decide to try again and if it fails again then it will show the error state again.

janicduplessis · 2024-04-22T19:46:02Z

It would be possible to retry the requests a few times, but at some point we need to stop if it keeps failing and let the user know. That is the main goal of this change. If we want to add some automatic retry behaviour it should probably be implemented somewhere else though, more at the networking layer.

shawnborton · 2024-04-22T19:52:01Z

So just to make sure I understand correctly, in this PR, if the request fails we'll show you a button to retry. But when you retry, it's likely just going to fail again?

janicduplessis · 2024-04-22T21:15:37Z

Yes, depending on what the problem was, if it was a random network error it might succeed when retrying, but if it is an outage it probably won’t. At least the user will be warned that an error occurred instead of being stuck in a loading loop.

janicduplessis · 2024-04-22T21:16:53Z

The current behaviour that causes a loading loop can also be very bad in case of an outage where all clients will start spamming the backend with network requests.

shawnborton · 2024-04-23T12:11:16Z

Cool, thanks for explaining. I definitely don't want to block on progress here and agree that this is better than what we currently experience. Thanks for hearing me out!

janicduplessis · 2024-04-23T13:10:27Z

There is definitely a good case for retrying failed requests automatically, but that would be a different project. It would be more like whenever a request fail we can try to see what the reason for the failure is and if we think it could work if trying again (let’s say its a network error) then we can re-send the request automatically. Those retries could be limited to only a few times and possibly with a delay. Then in case the retries fail it would throw an error which would be handled by the error states of the app (including the one added here).

janicduplessis · 2024-04-23T13:12:22Z

If there are no further design comments I can go ahead and implement the improvements tomorrow and this can be ready for final review.

Gonals · 2024-04-25T09:22:02Z

@janicduplessis, let me know when this is ready for review!

rojiphil · 2024-05-09T19:50:27Z

Addressed your review feedback too, let me know if it's good.

@janicduplessis Thanks for the changes. I just tested error condition for GetNewerActions and the error UI came up.
But we have another problem to address. When we retry GetNewerActions, the entire error message UI disappears for a moment and shows up again on failure of the API request. Like the GetOlderActions, shouldn't the error message Ui remain on retry until the API response is known? Here is a video to demonstrate this:

40610-new-action-retry-issue.mp4

dannymcclain · 2024-05-09T19:53:28Z

@rojiphil good catch. I think they should behave the same like you're suggesting.

janicduplessis · 2024-05-09T21:35:19Z

It currently disappears during loading because canShowHeader is false, which I think shouldn’t be. I’m a bit hesitant to add some workaround here since it seems like canShowHeader shouldn’t be false here and there might be a bug with it that could be addressed separately. What do you think?

rojiphil

I’m a bit hesitant to add some workaround here

I agree that we should not implement any workaround around canShowHeader. But we may overcome the problem as mentioned in the review comments.
Would this work?

rojiphil · 2024-05-10T03:56:27Z

src/libs/actions/Report.ts

@@ -1081,6 +1085,7 @@ function getNewerActions(reportID: string, reportActionID: string) {
            key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
            value: {
                isLoadingNewerReportActions: true,
+                hasLoadingNewerReportActionsError: false,


We can prevent setting hasLoadingNewerReportActionsError to false optimistically on retry of the failed request. Since we are forcefully getting newer actions on retry, we can do something like this here:
...(!force ? {hasLoadingNewerReportActionsError: false} : {}),
And when the API request succeeds, we can set it to false in successData

rojiphil · 2024-05-10T03:56:33Z

src/libs/actions/Report.ts

@@ -1038,6 +1040,7 @@ function getOlderActions(reportID: string, reportActionID: string) {
            key: `${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`,
            value: {
                isLoadingOlderReportActions: true,
+                hasLoadingOlderReportActionsError: false,


And we may want to do the same for getOlderActions here

janicduplessis · 2024-05-10T15:32:29Z

I think it would work, but I am still hesitant on adding more complexity. For example we do rely on loading state in a few places like here to not trigger additional requests. I am worried that if we have cases of loading data, but not setting this loading state might cause problems and make it more likely to introduce bugs in the future.

My current idea would be to merge it like this, and investigate the canShowHeader issue separately since currently there are just no loading states for new messages so it seems bugged. If canShowHeader is actually working as expected then maybe we can consider another solution.

mountiny · 2024-05-10T23:30:48Z

I agree we should avoid adding too much complexity. If it comes done to the issue with the button disappearing until new failure comes in, i think its fine for use to merge. Its rare case

@rojiphil can you please continue with the checklist and treat that issues as nab?

rojiphil

LGTM and tests well.

rojiphil · 2024-05-11T03:45:42Z

My current idea would be to merge it like this, and investigate the canShowHeader issue separately since currently there are just no loading states for new messages so it seems bugged. If canShowHeader is actually working as expected then maybe we can consider another solution.

Would be happy to work on this when we do.

rojiphil · 2024-05-11T03:48:21Z

can you please continue with the checklist and treat that issues as nab?

@mountiny Checklist is done.

mountiny

Just couple NABs, as this is useful performance / UX-wise, I am going to merge it as it, but I think we havent asked marketing for copy on the error so I will follow up with that

mountiny · 2024-05-13T11:38:23Z

src/pages/home/report/ReportActionsView.tsx

-                isTransactionThread: !isEmptyObject(transactionThreadReport),
-            })}`,
-        );
+    const loadOlderChats = useCallback(


I feel like these names here are misleading since we are not loading older or newer chats/ reports but only messages. NAB

mountiny · 2024-05-13T11:38:35Z

src/styles/index.ts

+            paddingVertical: 15,
+            paddingHorizontal: 20,
+        },
+        listBoundaryErrorText: {


Suggested change

listBoundaryErrorText: {

listBoundaryErrorText: {

mountiny · 2024-05-13T11:38:39Z

src/styles/index.ts

            position: 'absolute',
            top: 0,
            bottom: 0,
            left: 0,
            right: 0,
            height: CONST.CHAT_HEADER_LOADER_HEIGHT,
        },
+        listBoundaryError: {


Suggested change

listBoundaryError: {

listBoundaryError: {

OSBotify · 2024-05-13T11:47:58Z

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

janicduplessis · 2024-05-13T16:42:09Z

Thanks @mountiny !

OSBotify · 2024-05-15T10:12:41Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.74-0 🚀

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

kavimuru · 2024-05-15T10:35:56Z

@janicduplessis Can you help us with this step?

Test error state by failing pagination requests adding the following code in HttpUtils.ts here. It might also be possible to simulate this by turning off internet after loading a chat.

kavimuru · 2024-05-16T11:44:49Z

@rojiphil @mountiny could you help with the QA steps.

Test error state by failing pagination requests adding the following code in HttpUtils.ts here. It might also be possible to simulate this by turning off internet after loading a chat.

kavimuru · 2024-05-16T13:12:17Z

@janicduplessis @rojiphil @mountiny Can we run only the following step?

Go in a long chat and scroll to an old message

Right click and "Copy link"

Go in another chat and paste the link

Clear the cache (Account Settings -> Troubleshoot -> Clear cache and restart)

Go back to the chat with the link and click it

You will be in the middle of a chat and can test loading older / newer messages

rojiphil · 2024-05-16T16:14:23Z

Can we run only the following step?

Well! Running the mentioned steps alone will not help. If we can purposely fail GetOlderActions and GetNewerActions API requests for a specific long chat report in BE then we can test this with the mentioned steps. But I doubt if we can carry out the tests with any invalid Onyx entry in FE.
@mountiny @Gonals Do you see any better/easier option?

janicduplessis · 2024-05-16T16:46:10Z

Yea I think this might be hard to test on staging without being able to change the code. I will see if I can think of something.

kavimuru · 2024-05-16T16:54:49Z

@rojiphil @janicduplessis It would be great if you can validate this PR internally then.

kavimuru · 2024-05-16T19:32:33Z

@Beamanator Could this PR be validated internally?

janicduplessis · 2024-05-17T04:14:49Z

@rojiphil I figured out the issue that was preventing the loading state from showing, fix here #42332

Beamanator · 2024-05-17T15:37:10Z

@mountiny @rojiphil @janicduplessis can one of you please test this internally in staging? QA is having trouble 🙏

mountiny · 2024-05-20T19:00:42Z

@janicduplessis is that PR required to be CPed to staging or it can go through the normal deploy process?

I am not sure how this can be tested without changing code. I think we could however mark this off the checklist and we can monitor this change in the #newdot-quality room

janicduplessis · 2024-05-20T19:06:08Z

I think it can just go to normal deploy process. It is hard to test since it requires specific network failures or outage to happen. We can monitor it and make sure we do not have reports of infinite loading after network errors.

OSBotify · 2024-05-22T23:57:42Z

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.74-6 🚀

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

aldo-expensify · 2024-06-06T22:49:00Z

Getting old report actions gets stuck in a retry infinite loop, could it be related to this PR: https://expensify.slack.com/archives/C05LX9D6E07/p1717711999602319?thread_ts=1717462134.645329&cid=C05LX9D6E07

?

janicduplessis requested review from a team as code owners April 19, 2024 19:54

melvin-bot bot requested review from Gonals and removed request for a team April 19, 2024 19:54

janicduplessis commented Apr 19, 2024

View reviewed changes

mountiny assigned janicduplessis Apr 20, 2024

janicduplessis mentioned this pull request Apr 20, 2024

[HOLD for payment 2024-05-30] Loading loop when opening a chat #40641

Closed

6 tasks

rojiphil reviewed May 10, 2024

View reviewed changes

rojiphil approved these changes May 11, 2024

View reviewed changes

mountiny approved these changes May 13, 2024

View reviewed changes

mountiny merged commit d796699 into Expensify:main May 13, 2024
18 of 21 checks passed

janicduplessis deleted the @janic/pagination-errors branch May 13, 2024 17:40

github-actions bot mentioned this pull request May 15, 2024

Deploy Checklist: New Expensify 2024-05-15 #42185

Closed

janicduplessis mentioned this pull request May 20, 2024

Fix canShowHeader logic #42332

Merged

52 tasks


		const loadNewerChats = useCallback(() => {
		if (isLoadingInitialReportActions \|\| isLoadingOlderReportActions \|\| network.isOffline \|\| newestReportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {

Handle pagination errors in chats #40610

Handle pagination errors in chats #40610

Conversation

janicduplessis commented Apr 19, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Apr 19, 2024

janicduplessis Apr 19, 2024

Choose a reason for hiding this comment

janicduplessis commented Apr 19, 2024 • edited Loading

janicduplessis commented Apr 19, 2024

janicduplessis commented Apr 19, 2024

mountiny commented Apr 20, 2024

janicduplessis commented Apr 20, 2024 • edited Loading

mountiny commented Apr 20, 2024

dannymcclain commented Apr 22, 2024

janicduplessis commented Apr 22, 2024

shawnborton commented Apr 22, 2024 • edited Loading

shawnborton commented Apr 22, 2024

janicduplessis commented Apr 22, 2024

dannymcclain commented Apr 22, 2024

shawnborton commented Apr 22, 2024

janicduplessis commented Apr 22, 2024

janicduplessis commented Apr 22, 2024

shawnborton commented Apr 22, 2024

janicduplessis commented Apr 22, 2024

janicduplessis commented Apr 22, 2024

shawnborton commented Apr 23, 2024

janicduplessis commented Apr 23, 2024

janicduplessis commented Apr 23, 2024

Gonals commented Apr 25, 2024

rojiphil commented May 9, 2024

dannymcclain commented May 9, 2024

janicduplessis commented May 9, 2024

rojiphil left a comment

Choose a reason for hiding this comment

rojiphil May 10, 2024

Choose a reason for hiding this comment

rojiphil May 10, 2024

Choose a reason for hiding this comment

janicduplessis commented May 10, 2024

mountiny commented May 10, 2024

rojiphil left a comment

Choose a reason for hiding this comment

rojiphil commented May 11, 2024

rojiphil commented May 11, 2024

mountiny left a comment

Choose a reason for hiding this comment

mountiny May 13, 2024

Choose a reason for hiding this comment

mountiny May 13, 2024

Choose a reason for hiding this comment

mountiny May 13, 2024

Choose a reason for hiding this comment

OSBotify commented May 13, 2024

janicduplessis commented May 13, 2024

OSBotify commented May 15, 2024

kavimuru commented May 15, 2024

kavimuru commented May 16, 2024

kavimuru commented May 16, 2024

rojiphil commented May 16, 2024

janicduplessis commented May 16, 2024

kavimuru commented May 16, 2024 • edited Loading

kavimuru commented May 16, 2024

janicduplessis commented May 17, 2024

Beamanator commented May 17, 2024

mountiny commented May 20, 2024

janicduplessis commented May 20, 2024

OSBotify commented May 22, 2024

aldo-expensify commented Jun 6, 2024

janicduplessis commented Apr 19, 2024 •

edited

Loading

janicduplessis commented Apr 19, 2024 •

edited

Loading

janicduplessis commented Apr 20, 2024 •

edited

Loading

shawnborton commented Apr 22, 2024 •

edited

Loading

kavimuru commented May 16, 2024 •

edited

Loading