[HOLD Manual Requests][$1000] IOU Details page reload again after opening them same IOU

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Login with any account.
2. Go to any report, try to tap any IOU to open IOU Detail page, after that go back.
3. Swipe from bottom to make the app enter background mode, open the app again. Now tap to the same IOU that you tap before.
4. Notice that IOU Detail page doesn't show loading for the first time and it reloads after that few seconds.

Expected Result:

After enter foreground mode and tap the IOU, IOU Detail page should show loading and load the IOU Detail.

Actual Result:

IOU Detail page doesn't show loading after bringing to foreground then after a brief moment loading indicator appears

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• iOS / native

Version Number: 1.2.56-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

RPReplay_Final1674041550.MP4

HCZK7715.MP4

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674042038386489

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f5892f51566457de
• Upwork Job ID: 1615954837681774592
• Last Price Increase: 2023-01-26

[tjferriss]

I was able to reproduce this one on staging on my iPhone. The spinner after the page had already loaded definitely feels like a bug.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f5892f51566457de

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Proposal

Every time we open the IOU details, we perform a read request to the API. When we open back from the background, there is 2 write requests happening, ReconnectApp and OpenReport. If we take a look at the API.read implementation, it will wait for the write request to be completed first, so the optimistic data won't be applied until the write request is done. We can verify this with waiting for a few seconds before opening the IOU details after coming back from background.

The simplest way I think is to apply the optimistic data first on read function.

function read(command, apiCommandParameters, onyxData) {
+   // Optimistically update Onyx
+   if (onyxData.optimisticData) {
+       Onyx.update(onyxData.optimisticData);
+   }
    // Ensure all write requests on the sequential queue have finished responding before running read requests.
    // Responses from read requests can overwrite the optimistic data inserted by
    // write requests that use the same Onyx keys and haven't responded yet.
    SequentialQueue.waitForIdle().then(() => makeRequestWithSideEffects(command, apiCommandParameters, onyxData, CONST.API_REQUEST_TYPE.READ));
}

With this change, we will do the onyx update twice, once in read, once in makeRequestWithSideEffects. But looking at the comment, I am not sure with this approach.

Another approach I would like to propose is to create a new Onyx key to let a component knows whether a read request is pending or not.

diff --git a/src/ONYXKEYS.js b/src/ONYXKEYS.js
index 77c3f0396..c4b7227fb 100755
--- a/src/ONYXKEYS.js
+++ b/src/ONYXKEYS.js
@@ -175,4 +175,7 @@ export default {
 
     // Whether we should show the compose input or not
     SHOULD_SHOW_COMPOSE_INPUT: 'shouldShowComposeInput',
+
+    // Set when the read request is waiting for all write requests to be finished
+    IS_PENDING_READ: 'isPendingRead',
 };
diff --git a/src/libs/API.js b/src/libs/API.js
index d86fc099a..a917f1b18 100644
--- a/src/libs/API.js
+++ b/src/libs/API.js
@@ -4,6 +4,7 @@ import * as Request from './Request';
 import * as SequentialQueue from './Network/SequentialQueue';
 import pkg from '../../package.json';
 import CONST from '../CONST';
+import ONYXKEYS from '../ONYXKEYS';
 
 /**
  * All calls to API.write() will be persisted to disk as JSON with the params, successData, and failureData.
@@ -105,10 +106,14 @@ function makeRequestWithSideEffects(command, apiCommandParameters = {}, onyxData
  * @param {Object} [onyxData.failureData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode !== 200.
  */
 function read(command, apiCommandParameters, onyxData) {
+    Onyx.set(ONYXKEYS.IS_PENDING_READ, true);
     // Ensure all write requests on the sequential queue have finished responding before running read requests.
     // Responses from read requests can overwrite the optimistic data inserted by
     // write requests that use the same Onyx keys and haven't responded yet.
-    SequentialQueue.waitForIdle().then(() => makeRequestWithSideEffects(command, apiCommandParameters, onyxData, CONST.API_REQUEST_TYPE.READ));
+    SequentialQueue.waitForIdle().then(() => {
+        Onyx.set(ONYXKEYS.IS_PENDING_READ, false);
+        makeRequestWithSideEffects(command, apiCommandParameters, onyxData, CONST.API_REQUEST_TYPE.READ);
+    });
 }
 
 export {
diff --git a/src/pages/iou/IOUDetailsModal.js b/src/pages/iou/IOUDetailsModal.js
index ab600cfb5..00e2ede54 100644
--- a/src/pages/iou/IOUDetailsModal.js
+++ b/src/pages/iou/IOUDetailsModal.js
@@ -165,7 +165,7 @@ class IOUDetailsModal extends Component {
                         title={this.props.translate('common.details')}
                         onCloseButtonPress={Navigation.dismissModal}
                     />
-                    {this.props.iou.loading ? <ActivityIndicator color={themeColors.text} /> : (
+                    {(this.props.iou.loading || this.props.isPendingRead) ? <ActivityIndicator color={themeColors.text} /> : (
                         <View style={[styles.flex1, styles.justifyContentBetween]}>
                             <ScrollView contentContainerStyle={styles.iouDetailsContainer}>
                                 <IOUPreview
@@ -225,5 +225,8 @@ export default compose(
             key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${route.params.chatReportID}`,
             canEvict: false,
         },
+        isPendingRead: {
+            key: ONYXKEYS.IS_PENDING_READ,
+        },
     }),
 )(IOUDetailsModal);

Result

329005.t.mp4

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[mollfpr]

@bernhardoj Thanks for the proposal, but I think your proposal is over-engineered. Do you think this issue can be done without changing the API function?

[bernhardoj]

@mollfpr I think we can, but I personally don't like the solution. So here is the idea.

I believe we will show the loading every time we open the details page. So, to make sure that the isLoading value from iou is always true every time we open the page, we can update it every time the component unmount. It will look like this.

componentWillUnmount() {
    Onyx.merge(ONYXKEYS.IOU, {isLoading: true});
}

I am sure people will confuse when read this 😅.

=========
With the new onyx key solution, we can use it also on another component if there is a similar issue. I haven't checked yet which component that potentially suffer the same issue.

[mollfpr]

Yeah, that's not ideal and easy in the eyes.

+                    {(this.props.iou.loading || this.props.isPendingRead) ? <ActivityIndicator color={themeColors.text} /> : (

IMO having a global loading state will cause unnecessary rerender. For example, if we have another API read after the iou data is fetched, we will see the loader that triggers from another API request.

[bernhardoj]

Ah yeah, good point. I didn't realized that. I will try to think for another approach. 👍

[fedirjh]

Proposal

Root cause

For IOUDetailsModal we are fetching new data when componentDidMount , this will trigger a read request in openPaymentDetailsPage hence the loading prop of the IOU became true and render the loading indicator , most the time the read request is executed instantly so the indicator shows instantly too .
Now the problem occurs If we have Onyx data for IOUReport and the openPaymentDetailsPage read request is queued, in this case this will happen :

1. IOUDetailsModal will render with the Onyx persistent data for IOUReport.
2. The loading indicator will not show until the read request is triggered.
3. Once the request is triggered the Loading Indicator will be rendered.
4. Finally when the request finishes , Onyx updates IOUReport and IOUDetailsModal will render again with new data.

When we get from background, there are two request that will be triggered ReconnectApp and OpenReport and if we open IOUDetailsModal before those requests finish then , the openPaymentDetailsPage read request is queued , and the above scenario will happen.

This is a regression from #14028 , @mollfpr you already mentioned this case

@Julesssss Are we okay with showing the loading when the fetching happens while the data is already in Onyx?
Yeah, I don't see why not. It should be a very short period and we can improve this later by matching the regular chats:

Solution 1

we can handle loading in state in the IOUDetailsModal , and display a loading indicator when online , when offline display the persistent Onyx data

diff --git a/src/pages/iou/IOUDetailsModal.js b/src/pages/iou/IOUDetailsModal.js
index 4da6d6792f..699c230089 100644
--- a/src/pages/iou/IOUDetailsModal.js
+++ b/src/pages/iou/IOUDetailsModal.js
@@ -85,6 +85,14 @@ const defaultProps = {
 };
 
 class IOUDetailsModal extends Component {
+    constructor(props) {
+        super(props);
+
+        this.state = {
+            loading: true,
+        };
+    }
+
     componentDidMount() {
         if (this.props.network.isOffline) {
             return;
@@ -94,6 +102,15 @@ class IOUDetailsModal extends Component {
     }
 
     componentDidUpdate(prevProps) {
+        // Update state when there is a change ,in the iou loading prop , between prevProps and actual props
+        if (prevProps.iou !== this.props.iou) {
+            const prevLoading = lodashGet(prevProps.iou, 'loading');
+            const currentLoading = lodashGet(this.props.iou, 'loading');
+
+            if (prevLoading !== currentLoading && prevLoading === this.state.loading) {
+                this.setState({loading: currentLoading});
+            }
+        }
         if (!prevProps.network.isOffline || this.props.network.isOffline) {
             return;
         }
@@ -165,7 +182,7 @@ class IOUDetailsModal extends Component {
                         title={this.props.translate('common.details')}
                         onCloseButtonPress={Navigation.dismissModal}
                     />
-                    {this.props.iou.loading ? <View style={styles.flex1}><FullScreenLoadingIndicator /></View> : (
+                    {!this.props.network.isOffline && this.state.loading ? <View style={styles.flex1}><FullScreenLoadingIndicator /></View> : (
                         <View style={[styles.flex1, styles.justifyContentBetween]}>
                             <ScrollView contentContainerStyle={styles.iouDetailsContainer}>
                                 <IOUPreview

Solution 2

we can display only the loading indicator when we don't have data in onyx and the read request hasn't yet finished

diff --git a/src/pages/iou/IOUDetailsModal.js b/src/pages/iou/IOUDetailsModal.js
index 4da6d6792f..53bd37a2e8 100644
--- a/src/pages/iou/IOUDetailsModal.js
+++ b/src/pages/iou/IOUDetailsModal.js
@@ -157,7 +157,7 @@ class IOUDetailsModal extends Component {
         return (
             <ScreenWrapper>
                 <FullPageNotFoundView
-                    shouldShow={_.isEmpty(this.props.iouReport)}
+                    shouldShow={!this.props.iou.loading && _.isEmpty(this.props.iouReport)}
                     subtitleKey="notFound.iouReportNotFound"
                     onBackButtonPress={Navigation.dismissModal}
                 >
@@ -165,7 +165,7 @@ class IOUDetailsModal extends Component {
                         title={this.props.translate('common.details')}
                         onCloseButtonPress={Navigation.dismissModal}
                     />
-                    {this.props.iou.loading ? <View style={styles.flex1}><FullScreenLoadingIndicator /></View> : (
+                    {this.props.iou.loading && _.isEmpty(this.props.iouReport) ? <View style={styles.flex1}><FullScreenLoadingIndicator /></View> : (
                         <View style={[styles.flex1, styles.justifyContentBetween]}>
                             <ScrollView contentContainerStyle={styles.iouDetailsContainer}>
                                 <IOUPreview

[mollfpr]

@fedirjh Thanks for the proposal. I did have concerns about this issue, but my expected result was to only show the loading indicator where we don't have the Onyx data.

I believe solution 2 will be updating IOUPreview after getting the data back from the API without showing the loading indicator?

[fedirjh]

I believe solution 2 will be updating IOUPreview after getting the data back from the API without showing the loading indicator?

@mollfpr Yes , the loading indicator will only show when there is no data in Onyx and there is a read request that is being executed , once the request is fulfilled then it will either show IOUPreview or show the FullPageNotFoundView , If we have data in Onyx then it will be updated without showing the loading indicator.

After enter foreground mode and tap the IOU, IOU Detail page should show loading and load the IOU Detail.

Also I don't think this is the expected result, I think that once we have data in onyx then we don't have to show loading indicator anymore , this behaviour (OP expected result) will be different in offline mode , there should be no indicator in offline mode , in other words if we expect loading indicator to show then the IOUPreview will not work on offline and will show loading indicator until you get back online and execute the read request , that's why I proposed the first solution to only show loading indicator only on online mode

[mollfpr]

Thanks @fedirjh! I agree with you, the OP expected result was not specified in the offline mode but I think the result from your solution 1 is ideal.

@dangrous We can go with @fedirjh proposal solution 1.

🎀 👀 🎀 C+ reviewed!

[dangrous]

Reading through, I have a question - does it make sense to have loading states in both the iou and the modal (which would happen if we implemented this)? It seems a bit odd... And iou.loading is only used here, and in IOUParticipantsPage (i think). Is there a way to combine the functionality added in https://github.com/Expensify/App/pull/14028/files as well as that suggested here, and still ensure that the not found page is shown? Let me know if I'm misunderstanding anything!

[mollfpr]

@dangrous Yeah I know that feels odd to have 2 loading states, but so far I see that we need to initiate the loading state to true when the modal first opens.

@fedirjh Is it possible to make the iou.loading true right after the modal open?

[fedirjh]

@dangrous the loading states are different, the iou.loading is the loading state of the read request that is being triggered, the component state will handle the delay that happen when the read request is queued .

Is it possible to make the iou.loading true right after the modal open ?

@mollfpr I just deleted my proposal (you can check it in edit history) , I don't think this is a right pattern to reset iou.loading this will make another bug when the request is canceled , the loading will still load until you switch to other page.

Edit

I will provide another proposal , we can reset the iou.loading before we send the read request , this is the only place where you can reset iou.loading

Solution 3

diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 3cbb5a2b01..d96c97726f 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -458,6 +458,8 @@ function readOldestAction(reportID, oldestActionSequenceNumber) {
  * @param {Number} iouReportID
  */
 function openPaymentDetailsPage(chatReportID, iouReportID) {
+    // Resets the IOU loading state before we enqueue request
+    Onyx.merge(ONYXKEYS.IOU, {loading: true});
     API.read('OpenPaymentDetailsPage', {
         reportID: chatReportID,
         iouReportID,

Solution 4

we can also execute the optimisticData before we make a request

diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 3cbb5a2b01..5c84665c49 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -458,19 +458,20 @@ function readOldestAction(reportID, oldestActionSequenceNumber) {
  * @param {Number} iouReportID
  */
 function openPaymentDetailsPage(chatReportID, iouReportID) {
+    // Resets the IOU loading state before enqueueing request
+    Onyx.update([
+        {
+            onyxMethod: CONST.ONYX.METHOD.MERGE,
+            key: ONYXKEYS.IOU,
+            value: {
+                loading: true,
+            },
+        },
+    ]);
     API.read('OpenPaymentDetailsPage', {
         reportID: chatReportID,
         iouReportID,
     }, {
-        optimisticData: [
-            {
-                onyxMethod: CONST.ONYX.METHOD.MERGE,
-                key: ONYXKEYS.IOU,
-                value: {
-                    loading: true,
-                },
-            },
-        ],
         successData: [
             {
                 onyxMethod: CONST.ONYX.METHOD.MERGE,

[s77rt]

Proposal

diff --git a/src/libs/API.js b/src/libs/API.js
index d86fc099a0..850bcb626a 100644
--- a/src/libs/API.js
+++ b/src/libs/API.js
@@ -104,11 +104,16 @@ function makeRequestWithSideEffects(command, apiCommandParameters = {}, onyxData
  * @param {Object} [onyxData.successData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode === 200.
  * @param {Object} [onyxData.failureData] - Onyx instructions that will be passed to Onyx.update() when the response has jsonCode !== 200.
  */
-function read(command, apiCommandParameters, onyxData) {
+function read(command, apiCommandParameters = {}, onyxData = {}) {
     // Ensure all write requests on the sequential queue have finished responding before running read requests.
     // Responses from read requests can overwrite the optimistic data inserted by
     // write requests that use the same Onyx keys and haven't responded yet.
-    SequentialQueue.waitForIdle().then(() => makeRequestWithSideEffects(command, apiCommandParameters, onyxData, CONST.API_REQUEST_TYPE.READ));
+
+    // Optimistically update Onyx
+    if (onyxData.optimisticData) {
+        Onyx.update(onyxData.optimisticData);
+    }
+    SequentialQueue.waitForIdle().then(() => makeRequestWithSideEffects(command, apiCommandParameters, _.omit(onyxData, 'optimisticData'), CONST.API_REQUEST_TYPE.READ));
 }
 
 export {

RCA

I think @fedirjh did a great job explaining the root cause and I agree with his findings. However I'm not okay with any of the proposed solutions above except for @bernhardoj very first proposal which was almost just okay, he just forget to fix one thing and that's the double onyx update.

Personally I think the IOU logic is implemented correctly, so any change there is going to only be a patch especially since the same issue may happen in other places as well.

The fix is to apply the optimisticData just before queuing the request. That makes more sense and fix the issue globally.
And the tiny part that @bernhardoj missed is to actually omit the optimisticData from onyxData before passing it to makeRequestWithSideEffects so we avoid the double update.

Also to address the comments and the concern about data conflicts, the comments state that "Responses from read requests can overwrite the optimistic data". So the issue is actually on the returned data from the server that may cause a conflict and we are respecting that as the request is still queued.

FWIW write requests also set the optimisticData just before queuing the request.

[fedirjh]

Personally I think the IOU logic is implemented correctly

If we agree about this statement then we should close this issue without any changes.

That makes more sense and fix the issue globally.

You propose a global solution so you have to test all other usages of concurrent read and write requests . Also this has to be discussed wider in slack.

So the issue is actually on the returned data from the server that may cause a conflict and we are respecting that as the request is still queued.

Nope we are not respecting that, write request can have optimistic data too. So if we target same key with two queued requests (read and write) then what will happen ?

[JmillsExpensify]

@tjferriss Tom and I will take this from you, if you don't mind, and we'll wrap it up in the Manual Requests project.

[trjExpensify]

Yeah, I don't think it matters too much either way. But waiting for the page redesign would reduce the chance of any conflicts when we begin work on the changes. It's likely that the component structure will change, so it's a bit simpler to hold.

Cool, fair enough. Works for me! 👍

[trjExpensify]

New feature then for the skelly UI?

[dangrous]

Ah okay cool - so to confirm, we're going to close this issue, and we'll revisit the details as part of Manual Requests?

If so, do we still pay out for reporting? I don't think we'd need any of the other jobs.

Just want to make sure we're on the same page!

[trjExpensify]

I think we keep this issue open and on hold for manual requests, then revisit to implement it. Payments will be handled after it's on prod.

[JmillsExpensify]

Totally, nothing to do for the time being. Let's keep it open and hold.

[dangrous]

Sounds good!

[trjExpensify]

On hold for manual requests. Going to pop it on monthly till' we're on the way with the implementation.

[dangrous]

Still on hold

[dangrous]

Still on hold, I think we will be able to open it up again soon though!

[JmillsExpensify]

Yes indeed! Several weeks at the least.

[trjExpensify]

@mountiny @Julesssss @JmillsExpensify how do we feel about this one now? We have the skeleton UI loading in the expense/iou report screen and the request detailed thread. One improvement might be to include the page header(s) in the skeleton UI, right now I think it's just the comment sections?

[mountiny]

Yes, but I think we should create a fresh issue for that one. This issue is about component we wont support anymore.

[dangrous]

Cool, so do we just close this out instead?

[mountiny]

I think we can, I am not even sure if we need the updated skeleton, ti would not work when using URL and fetching the data fresh as then we have no clue what type of a report we have so we could only potentially do this when navigating from within the app, but I dont think its something we have to focus on

[dangrous]

Okay I'll close this for now, and if we end up needing to reopen, we can do that.