Fix waypoints removal

[paultsimura]

Details

Fix the bug when it was impossible to remove a waypoint when editing a Distance request.

Fixed Issues

$ #34686
PROPOSAL: #34686 (comment)

Tests

Same as QA

Offline tests

Same as QA

QA Steps

1. Create a distance request with 3 waypoints
2. Navigate to the distance request details
3. Disable the internet connection
4. Click on the distance field to edit
5. Click on a waypoint
6. Click on the 3 dot menu > Delete waypoint
7. Hit Save
8. Go back online

• Verify the distance & amount are updated
• Verify the waypoint was deleted successfully

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2024-01-26.at.22.09.06-compressed.mp4

MacOS: Desktop

Screen.Recording.2024-01-26.at.22.20.18-compressed.mp4

[paultsimura]

@alitoshmatov there are some complications on the Onyx side. Asked in Slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1706359180399219?thread_ts=1705484273.717249&cid=C01GTK53T8Q

[paultsimura]

Hey @tgolen, I think I'm facing the Onyx race condition once again – would appreciate your closer look 🙏

I'm attaching a screen recording with every network call expanded and checked key-by-key, you can see that in all the operations after the first OpenReport, the transaction has either 2 waypoints or 3 (with the last one explicitly set to null, which you implemented).

Screen.Recording.2024-02-22.at.22.28.29-compressed.mp4

The steps to reproduce are:

1. Create a Distance request
2. Visit it once (so it's loaded into Onyx)
3. Move back to the parent Expense report
4. Go offline
5. Open the created Distance request (this is important so that we load the OpenReport call into the queue)
6. Remove a waypoint and save the result
7. Go online

I suspect there is a race condition between the merge and mergecollection operations once they are batched when coming online: the mergecollection, even though it comes first chronologically, overwrites the merge operations that come later.

We saw a similar faulty behavior here: #33544 (comment) (the fix was a kinda workaround), and here: #28737 (this one was supposed to fix this very issue, but it remains, apparently).

cc: @Julesssss as you were handling the original Onyx out-of-order issue.

[tgolen]

Do you think it's an issue with the App's API queue, or internal to Onyx? Can we isolate the bug and create a unit test in react-native-onyx that demonstrates the bug and allows us to fix it?

[paultsimura]

Do you think it's an issue with the App's API queue, or internal to Onyx?

I tried batching 2 Onyx operations (mergecollection + merge) in one call using a browser console like this:

Promise.all([
    Onyx.mergeCollection('transactions_', {
        'transactions_4605945495455879570': {
            comment: {
                waypoints: {
                    waypoint0: "a",
                    waypoint1: "b",
                    waypoint2: "c",
                },
            },
        }
    }),
    Onyx.merge(`transactions_4605945495455879570`, {
        comment: {
            waypoints: {
                waypoint0: "a",
                waypoint1: "c",
                waypoint2: null,
            },
        },
    })
]);

This way, the data gets processed in the correct order.
So I would say it might be more related to the App's API queue, but I cannot speak with 100% confidence – I'm not that familiar with how Onyx works under the hood, unfortunately.

[tgolen]

Could you try that same kind of test from the console using Onyx.update() with both a mergeCollection and a merge in that same order and see if you get the same or a different result?

…

On Thu, Feb 22, 2024 at 3:49 PM Pavlo Tsimura ***@***.***> wrote: Do you think it's an issue with the App's API queue, or internal to Onyx? I tried batching 2 Onyx operations (mergecollection + merge) in one call using a browser console like this: Promise.all([ Onyx.mergeCollection('transactions_', { 'transactions_4605945495455879570': { comment: { waypoints: { waypoint0: "a", waypoint1: "b", waypoint2: "c", }, }, } }), Onyx.merge(`transactions_4605945495455879570`, { comment: { waypoints: { waypoint0: "a", waypoint1: "c", waypoint2: null, }, }, })]); This way, the data gets processed in the correct order. So I would say it might be more related to the App's API queue, but I cannot speak with 100% confidence – I'm not that familiar with how Onyx works under the hood, unfortunately. — Reply to this email directly, view it on GitHub <#35268 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB3FE7SOHMUMYFCB4ULYU7DQXAVCNFSM6AAAAABCMXQPWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGQ3DINZWGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[tgolen]

You could also try two calls to Onyx.update(). The first with a mergeCollection and the second with a merge. That will probably be more accurate with how the updates are applied to Onyx in the network layer.

…

On Thu, Feb 22, 2024 at 4:36 PM Tim Golen ***@***.***> wrote: Could you try that same kind of test from the console using Onyx.update() with both a mergeCollection and a merge in that same order and see if you get the same or a different result? On Thu, Feb 22, 2024 at 3:49 PM Pavlo Tsimura ***@***.***> wrote: > Do you think it's an issue with the App's API queue, or internal to Onyx? > > I tried batching 2 Onyx operations (mergecollection + merge) in one call > using a browser console like this: > > Promise.all([ > Onyx.mergeCollection('transactions_', { > 'transactions_4605945495455879570': { > comment: { > waypoints: { > waypoint0: "a", > waypoint1: "b", > waypoint2: "c", > }, > }, > } > }), > Onyx.merge(`transactions_4605945495455879570`, { > comment: { > waypoints: { > waypoint0: "a", > waypoint1: "c", > waypoint2: null, > }, > }, > })]); > > This way, the data gets processed in the correct order. > So I would say it might be more related to the App's API queue, but I > cannot speak with 100% confidence – I'm not that familiar with how Onyx > works under the hood, unfortunately. > > — > Reply to this email directly, view it on GitHub > <#35268 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAJMAB3FE7SOHMUMYFCB4ULYU7DQXAVCNFSM6AAAAABCMXQPWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGQ3DINZWGQ> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[paultsimura]

Both options work correctly (this and with both updates in one call):

Onyx.update([{
    "onyxMethod": "mergecollection",
    "key": "transactions_",
    "value": {
        "transactions_4605945495455879570": {
            comment: {
                waypoints: {
                    waypoint0: "a",
                    waypoint1: "b",
                    waypoint2: "c",
                },
            },
        }
    }
}]);
Onyx.update([{
    "onyxMethod": "merge",
    "key": "transactions_4605945495455879570",
    "value": {
        comment: {
            waypoints: {
                waypoint0: "a",
                waypoint1: "c",
                waypoint2: null,
            },
        },
    }
}]);

[tgolen]

Well, that's sure interesting. I guess the next step would be to add some debugging inside Onyx to see what order they are being applied in during the network requests.

You could start here:
https://github.com/Expensify/react-native-onyx/blob/main/lib/Onyx.js#L1477-L1519

and follow the instructions here for an easy way to build/debug it.

[paultsimura]

@tgolen thanks for your guidance on this.
I found eventually that we do have the race condition of how these events are being processed.

I've added logs here¹ to track when we build the promise for each operation, and here² to track when each promise is resolved.

You can see on the recording that we enqueue the promises correctly, but the merge ones get resolved before the mergeCollection:

Screen.Recording.2024-02-23.at.11.14.58-compressed.mp4

I suppose the issue lies here³:

        getCustomStore()('readwrite', (store) => {
            // Note: we are using the manual store transaction here, to fit the read and update
            // of the items in one transaction to achieve best performance.
            const getValues = Promise.all(pairs.map(([key]) => promisifyRequest<Value>(store.get(key))));

            return getValues.then((values) => {
                const upsertMany = pairs.map(([key, value], index) => {
                    const prev = values[index];
                    // eslint-disable-next-line @typescript-eslint/no-explicit-any
                    const newValue = utils.fastMerge(prev as any, value);
                    return promisifyRequest(store.put(newValue, key));
                });
                return Promise.all(upsertMany);
            });
        }),

Since the mergeCollection reads the values asynchronously, it takes longer to execute than the regular merge, and therefore its final promise, which modifies the data in store, is executed after the merge, which results in overwriting that merge operation.

Footnotes

1. https://github.com/Expensify/react-native-onyx/blob/8cca556b08bf858c17a7d4f5ddd2879ac3f456d4/lib/Onyx.js#L1497-L1515 ↩

2. https://github.com/Expensify/react-native-onyx/blob/63b07a3c9f82b9b5be5782c96dff9e1138ac1821/lib/storage/providers/IDBKeyVal.ts#L18-L39 ↩

3. https://github.com/Expensify/react-native-onyx/blob/63b07a3c9f82b9b5be5782c96dff9e1138ac1821/lib/storage/providers/IDBKeyVal.ts#L22-L34 ↩

[tgolen]

Oh awesome! Way to dig into that. @hannojg could you take a look at this and see if you can provide any guidance? I think it was you that added this originally.

[hannojg]

Hey, sorry, but I have never worked on that code 😄