Onyx.update doesn't apply updates in order with set and merge

[neil-marcellini]

Problem

Onyx.update doesn't apply updates in order when mixing set and merge

If Onyx.update is called with an update to merge an object on a key, and then the next update removes it by calling set with the value as null, then the updates are not applied in order. The Onyx.set update is applied first and the end state of the system is that the key is not removed.

Also, subscribers are notified with each update. In some cases we want to only notify subscribers with the correct end state of the system.

Related to [Tracking] "Replay effect" when sequential actions are taken offline, and user subsequently comes online

Example

I found this to be the root problem of this app issue Expensify/App#15550. Here is the user action and results.

Action Performed:

1. Turn off network
2. Create a workspace
3. Delete the workspace
4. Turn on network
5. Notice that the deleted workspace blinks

Expected Result:

The deleted workspace should not be visible

Actual Result:

The deleted workspace blinks

After we go back online and the sequential queue finishes processing, we call Onyx.update with a list of queued updates. We have already optimistically created and deleted the policy, and the list of updates includes creating and deleting the policy from the network responses. We create the policy with a call to merge and delete it with a call to set. To test I added some log lines and ran the following code in the JS console. You can see that the set call is applied first when it should be applied second.

Logs

Onyx.update([
    {
        onyxMethod: 'merge',
        key: 'policy_BEB826F4F75AD818',
        value: {
            id: 'BEB826F4F75AD818',
            name: "Expensifail's Workspace 2",
            role: 'admin',
            type: 'free',
            owner: '[email protected]',
            outputCurrency: 'USD',
            employeeList: [
                {
                    email: '[email protected]',
                    forwardsTo: '',
                    role: 'admin',
                    submitsTo: '[email protected]',
                },
            ],
        },
    },
    {
        onyxMethod: 'set',
        key: 'policy_BEB826F4F75AD818',
        value: null,
    },
]);
web.development.js:1083 [Onyx] top of merge, key = policy_BEB826F4F75AD818 value =  {id: 'BEB826F4F75AD818', name: "Expensifail's Workspace 2", role: 'admin', type: 'free', owner: '[email protected]', …}
web.development.js:941 [Onyx] top of set, key = policy_BEB826F4F75AD818 value =  null
web.development.js:891 [Onyx] top of remove
web.development.js:1101 [Onyx] merge method calling set, modifiedData = {id: 'BEB826F4F75AD818', name: "Expensifail's Workspace 2", role: 'admin', type: 'free', owner: '[email protected]', …}
web.development.js:941 [Onyx] top of set, key = policy_BEB826F4F75AD818 value =  {id: 'BEB826F4F75AD818', name: "Expensifail's Workspace 2", role: 'admin', type: 'free', owner: '[email protected]', …}
web.development.js:962 [Onyx] set method called cache.set key =  policy_BEB826F4F75AD818 value = {id: 'BEB826F4F75AD818', name: "Expensifail's Workspace 2", role: 'admin', type: 'free', owner: '[email protected]', …}
PolicyUtils.js:10 [policy_BEB826F4F75AD818] subscriber callback, val =  null
PolicyUtils.js:10 [policy_BEB826F4F75AD818] subscriber callback, val =  {id: 'BEB826F4F75AD818', name: "Expensifail's Workspace 2", role: 'admin', type: 'free', owner: '[email protected]', …}
Promise {<pending>}

Why it's important

Applying all updates from the Networks responses at once fixes the "replay effect" by updating the App with the proper end state as determined by the server without replaying any intermediate and redundant updates. If we applied these updates as soon as each API response resolved, then we would see each optimistic action we took while offline replayed in the UI.

However, because of this bug we end up in the wrong state and so the replay effect bug is not solved.

Solution

Modify Onyx.update so that it always applies all updates in order. Also, as an additional improvement, prevent notifying subscribers until all the updates have been applied.

[marcaaron]

Can we add a practical real world example of how we are getting into this situation? I think it would help everyone understand exactly what is happening and encourage more participation.

[marcaaron]

we end up in the wrong state because there is a call to create an object and then remove it

e.g. what object is this? What are we actually trying to do?

[neil-marcellini]

Updated

[roryabraham]

Hey @neil-marcellini there's maybe some overlap between this issue and Expensify/App#15321. Check out this comment for my latest thoughts.

Solution C in that comment describes adding a queue for Onyx writes that's shared across tabs. That would also maybe apply updates in order.

[neil-marcellini]

That's an interesting problem, but I think it's somewhat different. It would probably help both problems if each update in the list passed to Onyx.update was applied and notified about in order.

[roryabraham]

It's definitely a different problem but having a FIFO write queue for Onyx seems like a potentially overlapping solution for both? Someone from Callstack is investigating already, so I just don't want us duplicating efforts.

[neil-marcellini]

Ah I see good. I'll read up on what's happening there before I make a proposal.

[sophiepintoraetz]

@neil-marcellini - is there an update here?

[neil-marcellini]

No 🙈 I haven't had time. I have 4 other weekly issues that are higher priority. We can discuss if it should be higher priority though.

[kadiealexander]

@neil-marcellini gentle bump for an update, just for Expensify/App#15550 which is on hold for this. Thanks in advance!

[neil-marcellini]

I just tested again and the problem is still reproducible by pasting the following in the console in NewDot. The expected result is that policy_BEB826F4F75AD818 should not be in indexedDB, but it is.

Onyx.update([
    {
        onyxMethod: 'merge',
        key: 'policy_BEB826F4F75AD818',
        value: {
            id: 'BEB826F4F75AD818',
            name: "Expensifail's Workspace 2",
            role: 'admin',
            type: 'free',
            owner: '[email protected]',
            outputCurrency: 'USD',
            employeeList: [
                {
                    email: '[email protected]',
                    forwardsTo: '',
                    role: 'admin',
                    submitsTo: '[email protected]',
                },
            ],
        },
    },
    {
        onyxMethod: 'set',
        key: 'policy_BEB826F4F75AD818',
        value: null,
    },
]);

[neil-marcellini]

One of the resulting problems in App is still reproducible, which likely means more are. I'm going to mark this external and recruit someone from Margelo to work on it.

Screen.Recording.2023-11-22.at.6.30.50.PM.mov

[neil-marcellini]

Asked in Slack here.

[chrispader]

going to take a look at this 👍

[neil-marcellini]

PR merged! Please put up another to update the Onyx version in App.

[neil-marcellini]

❗ 🔥 Heads up, I'm going to be OOO working 0% from 1/1-1/12/24. I'll be back on 1/15/24. I'm planning to work 50% tomorrow 12/29. Please feel free to un-assign, re-assign, whatever. When I return I'll pick back up whatever I can and do my best to GSD in order of priority. I will also post all of my assigned issues in #engineering-chat in Slack to try to recruit volunteers.

[neil-marcellini]

This is done! Expensify/App#15550 (comment)