Skip to content

fix: loading indicator display when deleting vague distance Expense #51940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Dec 11, 2024
Merged

fix: loading indicator display when deleting vague distance Expense #51940

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1fff908
fix: loading indicator display when deleting vague distance Expense
truph01 Nov 4, 2024
41f25da
merge main
truph01 Nov 7, 2024
0fab945
merge main
truph01 Nov 11, 2024
fcf7f3f
remove InteractionManager.runAfterInteractions
truph01 Nov 11, 2024
65b343b
fix: remove unused lib
truph01 Nov 11, 2024
10fb70b
fix: revert remove InteractionManager.runAfterInteractions
truph01 Nov 11, 2024
3ac6241
merge main
truph01 Nov 24, 2024
38b088c
merge main
truph01 Dec 2, 2024
d332cb7
fix: clear action first then navigate back then clear rest
truph01 Dec 3, 2024
12a0a68
fix: add isWeb logic
truph01 Dec 3, 2024
70fb37d
fix: revert unused change
truph01 Dec 3, 2024
173a51e
fix: lint
truph01 Dec 3, 2024
8353a82
fix: lint
truph01 Dec 3, 2024
3ecf918
merge main
truph01 Dec 6, 2024
a90580a
fix: move all update logic to cleanUpMoneyRequest
truph01 Dec 6, 2024
d71f1eb
fix: lint
truph01 Dec 6, 2024
4149884
fix: use promise
truph01 Dec 6, 2024
5117bec
fix: lint
truph01 Dec 6, 2024
9d570a1
fix: remove param
truph01 Dec 6, 2024
cc1ef7f
merge main
truph01 Dec 9, 2024
f104ffd
fix: remove variable and remove unused export
truph01 Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/components/ReportActionItem/MoneyRequestView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useCallback, useMemo} from 'react';
import {View} from 'react-native';
import {InteractionManager, View} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import * as Expensicons from '@components/Icon/Expensicons';
Expand Down Expand Up @@ -461,8 +461,11 @@ function MoneyRequestView({report, shouldShowAnimatedBackground, readonly = fals
return;
}
if (parentReportAction) {
const urlToNavigateBack = IOU.cleanUpMoneyRequest(transaction?.transactionID ?? linkedTransactionID, parentReportAction, true);
const {urlToNavigateBack} = IOU.prepareToCleanUpMoneyRequest(transaction?.transactionID ?? linkedTransactionID, parentReportAction, true);
Navigation.goBack(urlToNavigateBack);
InteractionManager.runAfterInteractions(() => {
IOU.cleanUpMoneyRequest(transaction?.transactionID ?? linkedTransactionID, parentReportAction, true);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayane-djouah @flodnv After we click on "x" button, the report preview is displayed for a while before it is removed. Do we treat it as a regression

I believe it's not necessary to wait for all interactions to complete in this case. We can modify the behavior so that the report preview is removed immediately:

Screen.Recording.2024-11-09.at.8.48.54.PM.mov
Suggested change
InteractionManager.runAfterInteractions(() => {
IOU.cleanUpMoneyRequest(transaction?.transactionID ?? linkedTransactionID, parentReportAction, true);
});
IOU.cleanUpMoneyRequest(transaction?.transactionID ?? linkedTransactionID, parentReportAction, true);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayane-djouah I applied your fix, so the original bug is still appeared:

Screen.Recording.2024-11-11.at.11.51.05.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayane-djouah In my opinion, in the proposed solution here, we prioritize navigation over updating Onyx data. As a result, there's a case where the user navigates successfully, but the Onyx data hasn't yet updated. This makes the behavior noted here an acceptable outcome. cc @flodnv

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused -- it seems like we should optimistically set the onyx data, which ensures that it's in an expected state before we navigate, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like we should optimistically set the onyx data, which ensures that it's in an expected state before we navigate, no?

@flodnv This decision depends on our priorities, as each approach has its trade-offs:

  1. Optimistically setting Onyx data before navigating: While this ensures the data is in the expected state before navigating, it reintroduces the bug we're currently addressing.

  2. Navigating without waiting for Onyx data to update (current PR's approach): This approach resolves the bug but introduces a potential behavioral issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a few solutions but there is no way to both:

  1. optimistically set the onyx data
  2. don't display the loading indicator.

Each approach has its own trade-offs, as outlined here. In other parts of the NewDot app, we sometimes use the first approach and other times the second.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we want to follow the 1st approach, we just need to close this PR. What do you think @rayane-djouah
@flodnv?

Copy link
Contributor

@rayane-d rayane-d Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@truph01 I think a potential solution would involve:

  1. Cleaning up the transaction preview report action.
  2. Navigating back to the IOU report.
  3. Cleaning up the remaining money request data afterward.

This approach assumes that the root cause of the loading indicator bug is the transaction thread data being cleared before navigation.

Could you try implementing this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rayane-djouah. I am trying your approach

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@truph01 Could you please share an update?

return;
}
}
Expand Down
1 change: 1 addition & 0 deletions src/libs/actions/IOU.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8601,6 +8601,7 @@ export {
completePaymentOnboarding,
payInvoice,
payMoneyRequest,
prepareToCleanUpMoneyRequest,
putOnHold,
replaceReceipt,
requestMoney,
Expand Down
Loading