[$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving

[lanitochka17]

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

---

Version Number: 9.0.60-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace chat
3. Submit a distance expense
4. Go to transaction thread
5. Click Distance
6. Add another waypoint
7. Refresh the page while distance editor RHP is open (close and reopen the app on Android and iOS)
8. Click outside the RHP to dismiss it
9. Click Distance

Expected Result:

The waypoint added in Step 6 will not appear because the new distance is not saved

Actual Result:

The waypoint added in Step 6 appears even when the new distance is not saved

Workaround:

Unknown

Platforms:

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

• [ ] Android: Standalone
• [x] Android: HybridApp
• [x] Android: mWeb Chrome
• [ ] iOS: Standalone
• [x] iOS: HybridApp
• [x] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/e05e6cae-1ada-41ef-be8f-78724d3ac55d

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021857103747653162057
• Upwork Job ID: 1857103747653162057
• Last Price Increase: 2024-12-05

Issue Owner

Current Issue Owner: @rayane-djouah

[melvin-bot[bot]]

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-11-21 19:03:45 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving

What is the root cause of that problem?

We already have a code that is run on unmount to restore transaction here https://github.com/Expensify/App/blob/8a83e2baa1c7ce7ee2c31103574937c0484854be/src/pages/iou/request/step/IOURequestStepDistance.tsx#L230 But it is never called for the case of a reloading the page so when it is opened after refresh we re-load the changed transaction to the backup so if we dismiss it will have the new waypoints https://github.com/Expensify/App/blob/8a83e2baa1c7ce7ee2c31103574937c0484854be/src/pages/iou/request/step/IOURequestStepDistance.tsx#L221

What changes do you think we should make in order to solve the problem?

We should update to only load backup transaction if the the transactionBackup is empty we can use Onyx.connect and run the code inside the callback in createBackupTransaction by updating createBackupTransaction

function createBackupTransaction(transaction: OnyxEntry<Transaction>, isDraft: boolean) {
    if (!transaction) {
        return;
    }

    // In Strict Mode, the backup logic useEffect is triggered twice on mount. The restore logic is delayed because we need to connect to the onyx first,
    // so it's possible that the restore logic is executed after creating the backup for the 2nd time which will completely clear the backup.
    // To avoid that, we need to cancel the pending connection.
    Onyx.disconnect(connection);
    const newTransaction = {
        ...transaction,
    };
    const conn = Onyx.connect({
        key: `${ONYXKEYS.COLLECTION.TRANSACTION_BACKUP}${transaction.transactionID}`,
        callback: (transactionBackup) => {
            Onyx.disconnect(conn);
            if (transactionBackup) {
                Onyx.set(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transactionBackup);
                return;
            }
            // Use set so that it will always fully overwrite any backup transaction that could have existed before
            Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION_BACKUP}${transaction.transactionID}`, newTransaction);
        },
    });
}

https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/pages/iou/request/step/IOURequestStepDistance.tsx#L221

        TransactionEdit.createBackupTransaction(transaction, IOUUtils.shouldUseTransactionDraft(action));

Note that this will fix the bug not only on page reloadrefresh but also for app closing and reopening 👍

Or optionally Or optionally

we need the setTimeout b/c by the time the effect is run useOnyx will not populate transactionBackup but

 setTimeout(() => {
            if (!transactionBackup) {
                // On mount, create the backup transaction.
                TransactionEdit.createBackupTransaction(transaction);
            }
        }, 0);

[huult]

Edited by proposal-police: This proposal was edited at 2024-11-13 06:06:56 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Waypoint is saved when distance editor RHP is refreshed and dismissed without saving

What is the root cause of that problem?

When the page is reloaded, we can't execute this code to restore the original transaction from the backup. https://github.com/Expensify/App/blob/3897516558d81945d20f6d0910b6f3070cd60533/src/pages/iou/request/step/IOURequestStepDistance.tsx#L222-L236

After the reload is complete, create a backup transaction with data that has not been restored to the original transaction. This causes the issue where we still see the waypoint edited but not saved https://github.com/Expensify/App/blob/3897516558d81945d20f6d0910b6f3070cd60533/src/pages/iou/request/step/IOURequestStepDistance.tsx#L214-L220

What changes do you think we should make in order to solve the problem?

To resolve this issue, we need to check if the page is reloaded, then we don't need to create a new backup with the edited data. By the following steps:

1 Create a new Onyx to indicate whether the page is reloaded or not

// src/ONYXKEYS.ts#L459
+      EDIT_DISTANCE_PAGE_RELOAD: 'editDistancePageReload_',

2 Listen for the event before the reload

// src/pages/iou/request/step/IOURequestStepDistance.tsx#L214
+    useEffect(() => {
+        window.addEventListener('beforeunload', () => {
+            Onyx.set(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`, true);
+        });

+        return () => {
+            window.removeEventListener('beforeunload', () => {
+                Onyx.set(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`, false);
+            });

+            Onyx.set(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`, false);
+        };
+    }, [transactionID]);

3 Add loading to wait value of edit distance reload

// src/pages/iou/request/step/IOURequestStepDistance.tsx#L75
+    const [isEditDistancePageRefreshing, isEditDistancePageResult] = useOnyx(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`);
// src/pages/iou/request/step/IOURequestStepDistance.tsx#L530

+    if (isLoadingOnyxValue(isEditDistancePageResult)) {
+        return <FullScreenLoadingIndicator />;
+    }

4 Add condition to check create new back up or not.

// src/pages/iou/request/step/IOURequestStepDistance.tsx#L215
 useEffect(() => {
        if (isCreatingNewRequest) {
            return () => {};
        }

        // On mount, create the backup transaction.
-        TransactionEdit.createBackupTransaction(transaction);
+        InteractionManager.runAfterInteractions(() => {
+            requestAnimationFrame(() => {
+                // On mount, create the backup transaction.
+                if (isTransactionPageRefreshing === true || isTransactionPageRefreshing === undefined) {
+                    if (transactionWasSaved.current) {
+                        TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
+                        return;
+                    }
+                    TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));

+                    if (!transaction?.reportID) {
+                        return;
+                    }
+                    Report.openReport(transaction?.reportID);
+                    return;
+                }

+                TransactionEdit.createBackupTransaction(transaction);
+            });
+        });

        return () => {
            // If the user cancels out of the modal without without saving changes, then the original transaction
            // needs to be restored from the backup so that all changes are removed.
            if (transactionWasSaved.current) {
                TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
                return;
            }
            TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));

            // If the user opens IOURequestStepDistance in offline mode and then goes online, re-open the report to fill in missing fields from the transaction backup
            if (!transaction?.reportID) {
                return;
            }
            Report.openReport(transaction?.reportID);
        };
        // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
    }, []);

POC

https://github.com/user-attachments/assets/63fe3c16-8039-40a6-8b3d-b609cc7b13ca

Test Branch

What alternative solutions did you explore? (Optional)

[joekaufmanexpensify]

Reproduced.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@joekaufmanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

[rayane-d]

Reviewing

[rayane-d]

we need the setTimeout b/c by the time the effect is run useOnyx will not populate transactionBackup but we can also optionally use Onyx.connect and run the code inside the callback in createBackupTransaction

@FitseTLT, could you please share a code example that demonstrates how to fix the bug by modifying the createBackupTransaction implementation?

[FitseTLT]

we need the setTimeout b/c by the time the effect is run useOnyx will not populate transactionBackup but we can also optionally use Onyx.connect and run the code inside the callback in createBackupTransaction

@FitseTLT, could you please share a code example that demonstrates how to fix the bug by modifying the createBackupTransaction implementation?

Will provide tomorrow 👍

[huult]

@rayane-djouah What do you think about my proposal above?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[FitseTLT]

@rayane-djouah U can check it now. Updated

[rayane-d]

@joekaufmanexpensify could you please clarify the expected behavior? When we open a distance request, edit the itinerary on the Distance Editor RHP page, and then refresh the page without saving, should the changes be discarded (similar to dismissing the Distance Editor RHP)? Or should the changes persist as a draft until we either dismiss the page or click "Save"?

[joekaufmanexpensify]

Left thoughts here. I would expect us not to save the behavior if you refresh the page without saving.

[rayane-d]

@FitseTLT @huult Could you please update your proposals based on this?

[FitseTLT]

Updated according to the expectation above.

[huult]

@FitseTLT @huult Could you please update your proposals based on this?

I will update it today.

[melvin-bot[bot]]

@joekaufmanexpensify @rayane-djouah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[huult]

Proposal update

• Add functionality to restore transactions when the page is reloaded

[FitseTLT]

@rayane-djouah BTW I misunderstood the expected behaviour yesterday if it is to save the changes then my previous proposal was already aligned to it so I have reverted back to it. 👍

[joekaufmanexpensify]

@rayane-djouah as an FYI, I am OOO the rest of the week after today, back next Monday. Not reassigning as a lot of the US team will be off and I don't think anything BZ will be needed during that time. Please raise in slack if anything comes up.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@joekaufmanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

[rayane-d]

@rayane-djouah What do you think about https://github.com/Expensify/App/issues/52413#issuecomment-2472472338?

@huult I have concerns about adding another Onyx storage entry; I think it could be more complex than necessary and might impact performance.

[rayane-d]

@rayane-djouah BTW I misunderstood the expected behaviour yesterday if it is to save the changes then my previous proposal was already aligned to it so I have reverted back to it. 👍

@FitseTLT Actually, the expected behavior is to discard the changes made without saving and restore the original transaction from the backup after the page refresh.

[FitseTLT]

@rayane-djouah Updated 👍

[rayane-d]

@FitseTLT, using

        if (transactionBackup) {
            Onyx.set(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transactionBackup);
            return;
        }

is duplicating code from TransactionEdit.restoreOriginalTransactionFromBackup. To address the root cause of the bug, a more effective approach might be to ensure that TransactionEdit.restoreOriginalTransactionFromBackup is invoked appropriately during a page refresh instead of TransactionEdit.createBackupTransaction

[rayane-d]

@FitseTLT, could you update your proposal based on this feedback?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸