[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 contributors@expensify.com 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:

https://user-images.githubusercontent.com/43996225/213301802-066556e4-cf3d-452b-bd26-7e129b44e290.MP4

https://user-images.githubusercontent.com/43996225/213301972-b4b506b7-6015-4464-a95c-440725ed5d10.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[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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

https://user-images.githubusercontent.com/50919443/213376631-bd32a834-2060-4e7e-894f-2dc2ce4e8df2.mp4

[melvin-bot[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 https://github.com/Expensify/App/pull/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 ?

[s77rt]

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

No, we should fix the root cause, what I'm saying is that we may pretty much have the same exact issue on other places and it wouldn't make sense to patch each of those.

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.

Yes, we should probably discuss this as I can't test all the cases it does not seem to be that easy / possible.

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 ?

Same things goes if we make two write requests that target the same key, I don't see an issue here. Besides your proposal 3 and 4 do exactly the same as what I'm proposing as a global fix.

[fedirjh]

Same things goes if we make two write requests that target the same key, I don't see an issue here. Besides your proposal 3 and 4 do exactly the same as what I'm proposing as a global fix.

@s77rt After further investigation, I found that before PR #12219, all read requests were running directly (synchronously). The PR attempted to fix an issue where read requests were overwriting write requests. As a result, it made all read requests wait for the sequential queue to finish processing the write requests before executing any read requests. However, I believe that there are some specific read requests that need to be run directly, as mentioned in this issue by @jasperhuangg in this comment https://github.com/Expensify/App/pull/12219#discussion_r1010755384

If there are any specific read requests that need to run directly, perhaps we can expose different functions API functions for the different cases? I guess I'm not entirely sure what these would look like.

To address this, we need to create another global fix that will allow specific read requests that need to run directly to do so. I think the best option here is to add another boolean parameter to the read function. If this boolean is set to true, the request should be executed immediately, bypassing the sequential queue of pending write requests.

Here is the code :

diff --git a/src/libs/API.js b/src/libs/API.js
index d86fc099a0..8e16fd3f4e 100644
--- a/src/libs/API.js
+++ b/src/libs/API.js
@@ -100,15 +100,22 @@ function makeRequestWithSideEffects(command, apiCommandParameters = {}, onyxData
  * @param {Object} onyxData  - Object containing errors, loading states, and optimistic UI data that will be merged
  *                             into Onyx before and after a request is made. Each nested object will be formatted in
  *                             the same way as an API response.
+ * @param {Boolean} sync - If true, we should execute the read request immediately. Otherwise, we should wait for all write
+ *                         requests on the sequential queue to finish processing before executing the read request.
  * @param {Object} [onyxData.optimisticData] - Onyx instructions that will be passed to Onyx.update() before the request is made.
  * @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, sync = false) {
     // 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));
+
+    if (!sync) {
+        SequentialQueue.waitForIdle().then(() => makeRequestWithSideEffects(command, apiCommandParameters, onyxData, CONST.API_REQUEST_TYPE.READ));
+    } else {
+        makeRequestWithSideEffects(command, apiCommandParameters, onyxData, CONST.API_REQUEST_TYPE.READ);
+    }
 }

 export {

We can then use the new parameters with any specific request that need to be run directly

diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 3cbb5a2b01..b46180f6ff 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -489,7 +489,8 @@ function openPaymentDetailsPage(chatReportID, iouReportID) {
                 },
             },
         ],
-    });
+    },
+    true);
 }

 /**

[s77rt]

@fedirjh I see, but I don't think we should process the requests, openPaymentDetailsPage does not look like the case where it should be processed sync. What I'm proposing is only to set it's optimistic data. I don't think this will cause the flickering effect because the flickering effect mostly occurs from the read requests responses. The optimistic data set in read requests won't and shouldn't cause the flickering effect.

cc @jasperhuangg Since you worked on that.

[bernhardoj]

Updated Proposal

I agree with something that can be run without needed to wait for all write requests to finish. I have actually been thinking about this, but I am not too sure.

The solution is to introduce a new property called readOptimisticData (idk what we should call this) and it will always be executed wtihout waiting for all read requests to finish. This will be useful when we want to update some onyx data that we know for sure won't affect any write request.

diff --git a/src/libs/API.js b/src/libs/API.js
index d86fc099a..540d35c89 100644
--- a/src/libs/API.js
+++ b/src/libs/API.js
@@ -105,6 +105,9 @@ 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) {
+    // Some Onyx data need to be optimistically updated without waiting for all write requests to finish.
+    if (onyxData.readOptimisticData) {
+        Onyx.update(onyxData.readOptimisticData);
+    }
     // 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.
diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 78faeaa3e..b48fade82 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -489,7 +489,7 @@ function openPaymentDetailsPage(chatReportID, iouReportID) {
         reportID: chatReportID,
         iouReportID,
     }, {
-        optimisticData: [
+        readOptimisticData: [
             {
                 onyxMethod: CONST.ONYX.METHOD.MERGE,
                 key: ONYXKEYS.IOU,

[s77rt]

@bernhardoj Right, that would work but it's only a solution that fixes this issue. We can also simplify this by adding a param to API.read and act accordingly where we only set the optimistic data if that flag is true.

 function read(command, apiCommandParameters = {}, onyxData = {}, preProcess = false) {
    onyxDataRest = {...onyxData};
    if (preProcess) {
        if (onyxData.optimisticData) {
            Onyx.update(onyxData.optimisticData);
            onyxDataRest = _.omit(onyxData, 'optimisticData');
        }
    }
    SequentialQueue.waitForIdle().then(() => makeRequestWithSideEffects(command, apiCommandParameters, onyxDataRest, CONST.API_REQUEST_TYPE.READ));
}

However I'm more inclined to fixing this once and for all by https://github.com/Expensify/App/issues/14399#issuecomment-1399234161 given that there is no drawbacks / regressions. Is there any scenario where this solution would cause a problem?

[mollfpr]

@dangrous What do you think about @s77rt proposal? I think it still avoids the conflict of read response and fixing this issue.

[fedirjh]

@mollfpr I have concerns that @s77rt approach may result in regression and negatively affect some offline features . Also be sure to create a slack thread if we want to make any changes to read function as it affects the entire App

[mollfpr]

@fedirjh That makes sense to me, the only regression I think is when updating the optimistic data from write while we have read on the way and overwrite the same key with another optimistic data.

@s77rt Could you open a discussion on Slack regarding the changes on your proposal?

[s77rt]

Opened a slack discussion here https://expensify.slack.com/archives/C01GTK53T8Q/p1674490790123139

[dangrous]

I think this proposal probably makes sense, but we should definitely see what comes of the discussion in Slack, since there might be some things I'm missing due to context. Thanks for starting that!

[s77rt]

Proposal 2

diff --git a/src/libs/actions/Report.js b/src/libs/actions/Report.js
index 3cbb5a2b01..04f087b075 100644
--- a/src/libs/actions/Report.js
+++ b/src/libs/actions/Report.js
@@ -461,34 +461,6 @@ function openPaymentDetailsPage(chatReportID, iouReportID) {
     API.read('OpenPaymentDetailsPage', {
         reportID: chatReportID,
         iouReportID,
-    }, {
-        optimisticData: [
-            {
-                onyxMethod: CONST.ONYX.METHOD.MERGE,
-                key: ONYXKEYS.IOU,
-                value: {
-                    loading: true,
-                },
-            },
-        ],
-        successData: [
-            {
-                onyxMethod: CONST.ONYX.METHOD.MERGE,
-                key: ONYXKEYS.IOU,
-                value: {
-                    loading: false,
-                },
-            },
-        ],
-        failureData: [
-            {
-                onyxMethod: CONST.ONYX.METHOD.MERGE,
-                key: ONYXKEYS.IOU,
-                value: {
-                    loading: false,
-                },
-            },
-        ],
     });
 }

diff --git a/src/pages/iou/IOUDetailsModal.js b/src/pages/iou/IOUDetailsModal.js
index ab600cfb5c..7ea7e6ecf8 100644
--- a/src/pages/iou/IOUDetailsModal.js
+++ b/src/pages/iou/IOUDetailsModal.js
@@ -1,5 +1,5 @@
 import React, {Component} from 'react';
-import {View, ActivityIndicator, ScrollView} from 'react-native';
+import {View, ScrollView} from 'react-native';
 import PropTypes from 'prop-types';
 import {withOnyx} from 'react-native-onyx';
 import lodashGet from 'lodash/get';
@@ -7,7 +7,6 @@ import _ from 'underscore';
 import styles from '../../styles/styles';
 import ONYXKEYS from '../../ONYXKEYS';
 import {withNetwork} from '../../components/OnyxProvider';
-import themeColors from '../../styles/themes/default';
 import HeaderWithCloseButton from '../../components/HeaderWithCloseButton';
 import Navigation from '../../libs/Navigation/Navigation';
 import ScreenWrapper from '../../components/ScreenWrapper';
@@ -39,15 +38,6 @@ const propTypes = {
     }).isRequired,

     /* Onyx Props */
-    /** Holds data related to IOU view state, rather than the underlying IOU data. */
-    iou: PropTypes.shape({
-        /** Is the IOU Report currently being loaded? */
-        loading: PropTypes.bool,
-
-        /** Error message, empty represents no error */
-        error: PropTypes.bool,
-    }),
-
     /** IOU Report data object */
     iouReport: PropTypes.shape({
         /** ID for the chatReport that this IOU is linked to */
@@ -79,7 +69,6 @@ const propTypes = {
 };

 const defaultProps = {
-    iou: {},
     reportActions: {},
     iouReport: undefined,
 };
@@ -165,37 +154,35 @@ class IOUDetailsModal extends Component {
                         title={this.props.translate('common.details')}
                         onCloseButtonPress={Navigation.dismissModal}
                     />
-                    {this.props.iou.loading ? <ActivityIndicator color={themeColors.text} /> : (
-                        <View style={[styles.flex1, styles.justifyContentBetween]}>
-                            <ScrollView contentContainerStyle={styles.iouDetailsContainer}>
-                                <IOUPreview
+                    <View style={[styles.flex1, styles.justifyContentBetween]}>
+                        <ScrollView contentContainerStyle={styles.iouDetailsContainer}>
+                            <IOUPreview
+                                chatReportID={this.props.route.params.chatReportID}
+                                iouReportID={this.props.route.params.iouReportID}
+                                pendingAction={pendingAction}
+                                shouldHidePayButton
+                            />
+                            <IOUTransactions
+                                chatReportID={this.props.route.params.chatReportID}
+                                iouReportID={this.props.route.params.iouReportID}
+                                isIOUSettled={iouReportStateNum === CONST.REPORT.STATE_NUM.SUBMITTED}
+                                userEmail={sessionEmail}
+                            />
+                        </ScrollView>
+                        {(hasOutstandingIOU && this.props.iouReport.managerEmail === sessionEmail && (
+                            <FixedFooter>
+                                <SettlementButton
+                                    onPress={paymentMethodType => this.payMoneyRequest(paymentMethodType)}
+                                    shouldShowPaypal={Boolean(lodashGet(this.props, 'iouReport.submitterPayPalMeAddress'))}
+                                    currency={lodashGet(this.props, 'iouReport.currency')}
+                                    enablePaymentsRoute={ROUTES.IOU_DETAILS_ENABLE_PAYMENTS}
+                                    addBankAccountRoute={ROUTES.IOU_DETAILS_ADD_BANK_ACCOUNT}
+                                    addDebitCardRoute={ROUTES.IOU_DETAILS_ADD_DEBIT_CARD}
                                     chatReportID={this.props.route.params.chatReportID}
-                                    iouReportID={this.props.route.params.iouReportID}
-                                    pendingAction={pendingAction}
-                                    shouldHidePayButton
                                 />
-                                <IOUTransactions
-                                    chatReportID={this.props.route.params.chatReportID}
-                                    iouReportID={this.props.route.params.iouReportID}
-                                    isIOUSettled={iouReportStateNum === CONST.REPORT.STATE_NUM.SUBMITTED}
-                                    userEmail={sessionEmail}
-                                />
-                            </ScrollView>
-                            {(hasOutstandingIOU && this.props.iouReport.managerEmail === sessionEmail && (
-                                <FixedFooter>
-                                    <SettlementButton
-                                        onPress={paymentMethodType => this.payMoneyRequest(paymentMethodType)}
-                                        shouldShowPaypal={Boolean(lodashGet(this.props, 'iouReport.submitterPayPalMeAddress'))}
-                                        currency={lodashGet(this.props, 'iouReport.currency')}
-                                        enablePaymentsRoute={ROUTES.IOU_DETAILS_ENABLE_PAYMENTS}
-                                        addBankAccountRoute={ROUTES.IOU_DETAILS_ADD_BANK_ACCOUNT}
-                                        addDebitCardRoute={ROUTES.IOU_DETAILS_ADD_DEBIT_CARD}
-                                        chatReportID={this.props.route.params.chatReportID}
-                                    />
-                                </FixedFooter>
-                            ))}
-                        </View>
-                    )}
+                            </FixedFooter>
+                        ))}
+                    </View>
                 </FullPageNotFoundView>
             </ScreenWrapper>
         );
@@ -209,9 +196,6 @@ export default compose(
     withLocalize,
     withNetwork(),
     withOnyx({
-        iou: {
-            key: ONYXKEYS.IOU,
-        },
         chatReport: {
             key: ({route}) => `${ONYXKEYS.COLLECTION.REPORT}${route.params.chatReportID}`,
         },
diff --git a/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsPage.js b/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsPage.js
index e87c4ac983..3b50ca57fa 100644
--- a/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsPage.js
+++ b/src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsPage.js
@@ -1,12 +1,7 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import {withOnyx} from 'react-native-onyx';
-import {ActivityIndicator, View} from 'react-native';
-import ONYXKEYS from '../../../../ONYXKEYS';
 import IOUParticipantsSplit from './IOUParticipantsSplit';
 import IOUParticipantsRequest from './IOUParticipantsRequest';
-import themeColors from '../../../../styles/themes/default';
-import styles from '../../../../styles/styles';

 const propTypes = {
     /** Callback to inform parent modal of success */
@@ -32,32 +27,14 @@ const propTypes = {
         phoneNumber: PropTypes.string,
         payPalMeAddress: PropTypes.string,
     })),
-
-    /* Onyx Props */
-
-    /** Holds data related to IOU view state, rather than the underlying IOU data. */
-    iou: PropTypes.shape({
-
-        /** Whether or not the IOU step is loading (retrieving participants) */
-        loading: PropTypes.bool,
-    }),
 };

 const defaultProps = {
-    iou: {},
     participants: [],
 };

-const IOUParticipantsPage = (props) => {
-    if (props.iou.loading) {
-        return (
-            <View style={styles.pageWrapper}>
-                <ActivityIndicator color={themeColors.text} />
-            </View>
-        );
-    }
-
-    return (props.hasMultipleParticipants
+const IOUParticipantsPage = props => (
+    props.hasMultipleParticipants
         ? (
             <IOUParticipantsSplit
                 onStepComplete={props.onStepComplete}
@@ -71,13 +48,10 @@ const IOUParticipantsPage = (props) => {
                 onAddParticipants={props.onAddParticipants}
             />
         )
-    );
-};
+);

 IOUParticipantsPage.displayName = 'IOUParticipantsPage';
 IOUParticipantsPage.propTypes = propTypes;
 IOUParticipantsPage.defaultProps = defaultProps;

-export default withOnyx({
-    iou: {key: ONYXKEYS.IOU},
-})(IOUParticipantsPage);
+export default IOUParticipantsPage;

Details

As per the above linked slack discussion, the path to move forward seems to just remove the loading indicator as it's not really needed.

[jatinsonijs]

We shouldn't remove Loader as with deep link it will not show anything until data load, In current situation its showing Not Found page until data loaded. So I have added condition in Page not found.

It will work like this: If data is empty and current state is loading show loader. If data is empty and current state is not loading and show Not found page. If data is not empty show data even if it is stale data.

Proposal

diff --git a/src/pages/iou/IOUDetailsModal.js b/src/pages/iou/IOUDetailsModal.js
index 4da6d6792f..237f3c3c0f 100644
--- a/src/pages/iou/IOUDetailsModal.js
+++ b/src/pages/iou/IOUDetailsModal.js
@@ -154,10 +154,11 @@ class IOUDetailsModal extends Component {
         const pendingAction = this.findPendingAction();
         const iouReportStateNum = lodashGet(this.props.iouReport, 'stateNum');
         const hasOutstandingIOU = lodashGet(this.props.iouReport, 'hasOutstandingIOU');
+
         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 +166,7 @@ class IOUDetailsModal extends Component {
                         title={this.props.translate('common.details')}
                         onCloseButtonPress={Navigation.dismissModal}
                     />
-                    {this.props.iou.loading ? <View style={styles.flex1}><FullScreenLoadingIndicator /></View> : (
+                    {_.isEmpty(this.props.iouReport) ? <View style={styles.flex1}><FullScreenLoadingIndicator /></View> : (
                         <View style={[styles.flex1, styles.justifyContentBetween]}>
                             <ScrollView contentContainerStyle={styles.iouDetailsContainer}>
                                 <IOUPreview

We can remove iou loading from Participants page if needed.

[bernhardoj]

Looks very similar to @fedirjh 2nd solution.

[jatinsonijs]

Yeah @bernhardoj I have missed it, its exactly same we can ignore it and use @fedirjh sol.

[s77rt]

@jatinsonijs @fedirjh Can you upload a video with that case? In my case using deep linking always appears to have the data.

As far as I can see OpenPaymentDetailsPage is always called first before OpenApp so the app will looks to be loading until we actually have IOU data.

[mollfpr]

@s77rt Yeah, I do not agree with removing the loader. It's introducing new regression for deep links where it's showing not found page first before data is loaded.

[s77rt]

@mollfpr It's not showing the not found page, this has been brought up in the slack discussion, but wasn't able to reproduce and didn't notice any regression thus I just removed the loading indicator

Can you upload a video showing the regression?

[bernhardoj]

Maybe you should sign out first to clear the onyx data and don't open the report yet, then open the iou details by deep link.

[s77rt]

Thanks @bernhardoj now I see the not found page

[mollfpr]

@dangrous This proposal from @fedirjh will have the OP expected result. I am personally okay with the solution to get away from SequentialQueue to just set the loading earlier.

Worth to mention the solution 2 from https://github.com/Expensify/App/issues/14399#issuecomment-1397205364 also looks good to me but it's has different expected result where the loader only appear one time where no IOU data available yet. The result will similar to the report chat skeleton and the updated data will added when there's updated IOU.

[s77rt]

@mollfpr, @fedirjh proposal 3 and 4 are basically the same as my first proposal, but it's only for that specefic case.

I have found that the issue occurs also on bank-account/BankAccountStep so we should fix the issue everywhere, and preferably globally (fix one place, fix it all).

[mollfpr]

@s77rt Could elaborate on the issue with bank-account/BankAccountStep and what the expected should be?

[s77rt]

@mollfpr Just the same issue but in another page and I guess in many other pages that share the same concept (read request call that is responsible for the loading indicator)

https://user-images.githubusercontent.com/16493223/214276644-ef4d57b0-0a35-4df5-9cbe-6127fb60a4bb.mp4

[mollfpr]

@s77rt You should add that video to the thread and maybe we can get the answer what they concern.

[s77rt]

Latest updates from slack

1. Calling openPaymentDetailsPage should return an info whether the report is found or not (internal)
2. We should remove the iou.loading key

The rendering flow in pseudo code is as follow:

if (_.isEmpty(iouReport)) {
    show skeleton view
} else if (iouReport.notFound) {
    show not found view
} else {
    show IOU report
}

• This issue should only handle IOU pages including IOUParticipantsPage (bank-account/BankAccountStep may have a different logic so let's not include it here)
• This issue should be put on hold until server changes are done (the internal part)

[mollfpr]

@dangrous Can we hold this until the changes request API is merged?

[melvin-bot[bot]]

Upwork job price has been updated to $1000

[dangrous]

Good call @mollfpr, I've put on hold and reverted the price doubling for now. Planning on getting that backend PR up this week!

[dangrous]

Sorry have not had the time I had hoped for. Will tackle the backend situation early next week.

[dangrous]

Apologies. Should have time for this shortly!