Allow MERGE operations into empty array values by chuckdries · Pull Request #754 · Expensify/react-native-onyx

chuckdries · 2026-03-18T18:51:40Z

Details

An alternative solution to #753. Rather than treating empty arrays as null at set-time, we should allow merge updates to clobber empty array values. Also logs an alert with ENSURE_BUGBOT so we can know when it happens.

See #expert-contributors

Related Issues

https://github.com/Expensify/Expensify/issues/611769

Automated Tests

Add should merge an object into an empty array and should still reject merging an object into a non-empty array

Manual Tests

Use Onyx.set('someKey', []) in the browser console to create or set a key to []
Use Onyx.get('someKey').then(data => console.log('someKey', data)) to log the value and make sure it's []
Use Onyx.merge('someKey', {asdf: {}}) to merge into the empty array
Use Onyx.get('someKey').then(data => console.log('someKey', data)) to log the value and make sure it's {asdf: {}}
Ensure that you saw a bugbot log in your console

Author Checklist

Screenshots/Videos

Android: Native

onyx.patch.working.android.mp4

iOS: Native

Onyx.patch.working.ios.mp4

MacOS: Chrome / Safari

onyx.patch.working.mp4

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 479bc113d0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-18T21:07:35Z

lib/utils.ts

+    if (Array.isArray(existingValue) && existingValue.length === 0 && !Array.isArray(value)) {
+        return {isCompatible: true};


Restrict empty-array compatibility to merge-only updates

checkCompatibilityWithExistingValue() is shared by setWithRetry() and mergeCollectionWithPatches(), not only Onyx.merge(), so this new early return makes all non-array writes compatible whenever the current value is []. That means a key that is legitimately array-typed (and currently empty) can now be silently overwritten by an object/primitive via set/mergeCollection, whereas the previous array-vs-non-array guard would log and skip it. Please scope this empty-array exception to the merge-object path instead of globally in the compatibility helper.

Useful? React with 👍 / 👎.

I think this is right, and we should be checking explicitly for object here, rather than non-array

rafecolton

I tested this locally and it worked well.

I think the AI reviewer's comments are valid, and this will allow overwriting arrays with scalar values in addition to objects. Is that intended?

rafecolton · 2026-03-18T22:37:56Z

lib/utils.ts

+    if (Array.isArray(existingValue) && existingValue.length === 0 && !Array.isArray(value)) {
+        return {isCompatible: true};


I think this is right, and we should be checking explicitly for object here, rather than non-array

rafecolton · 2026-03-18T22:39:02Z

lib/Onyx.ts

+                if (Array.isArray(existingValue) && existingValue.length === 0) {
+                    Logger.logAlert(`[ENSURE_BUGBOT] Onyx merge called on key "${key}" whose existing value is an empty array. Will coerce to object.`);
+                }


Will this fire for any merge attempt into an existing array or only if attempting to merge an object?

I'm pretty sure merging into an array is unsupported anyway. Checking

…improve logging

…e consistent

rafecolton

Tested on dev via red/green, worked great!

tgolen · 2026-03-19T19:07:15Z

lib/Onyx.ts

-                    const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(change, existingValue);
+                    const {isCompatible, existingValueType, newValueType, isEmptyArrayCoercion} = utils.checkCompatibilityWithExistingValue(change, existingValue);
+                    if (isEmptyArrayCoercion) {
+                        Logger.logAlert(`[ENSURE_BUGBOT] Onyx merge called on key "${key}" whose existing value is an empty array. Will coerce to object.`);


NAB: Is this enough data for someone to tell what went wrong and fix it? Having the key is nice, but I feel like having the value would almost be necessary. Though, I don't love the idea of logging the value since there is not a good way to know if it's sensitive or not. Is there any other data that we can include that would be helpful? I think probably the most useful thing would be the requestID (if one exists), but that sounds difficult to grab.

Yeah, I don't see a great way to get the requestID that delivered the update. We could maybe log in SaveResponseInOnyx for any SET [] operation, but that would be potentially noisier than we want, and that wouldn't cover pusher updates. I also don't want to log the value for the reason you stated

The log statements that make it to the backend will have a user email and a datetime. It'll take a bit of legwork but it's probably not impossible to work backwards from that and the onyx key and figure out what the user was doing

tgolen · 2026-03-19T19:08:26Z

lib/OnyxUtils.ts

            },
            {
-                result: (existingValue ?? {}) as TChange,
+                result: ((Array.isArray(existingValue) && existingValue.length === 0 ? {} : existingValue) ?? {}) as TChange,


Would you mind adding a code comment explaining why this is being done?

Well, I thought it was necessary because I was misinterpreting this comment in fastMerge, but I was mixing up source and target. Turns out it works fine without this line, so I reverted it.

tgolen · 2026-03-19T19:11:56Z

lib/utils.ts

            isCompatible: true,
        };
    }
+    // An empty array existing value is compatible with an object update.


Hm, this comment is kind of odd. It is making a statement, but it's not obvious to me why this statement would be true or not. I think the explanation about PHP below is helpful, but this comment could probably be more verbose to help understand why we chose to do this.

tests/unit/onyxTest.ts

rafecolton

Re-tested, confirmed it still works

tgolen

Cool, that looks great. Thanks for the improved comments!

os-botify · 2026-03-20T16:54:48Z

🚀 Published to npm in 3.0.49 🎉

Update react-native-onyx to include Expensify/react-native-onyx#754

Allow MERGE operations into empty array values

98e4511

melvin-bot bot assigned chuckdries Mar 18, 2026

Log alert when merging into an empty array

479bc11

chuckdries marked this pull request as ready for review March 18, 2026 21:03

chuckdries requested a review from a team as a code owner March 18, 2026 21:03

melvin-bot bot requested review from cristipaval and removed request for a team March 18, 2026 21:04

chatgpt-codex-connector bot reviewed Mar 18, 2026

View reviewed changes

chuckdries requested a review from rafecolton March 18, 2026 21:56

rafecolton reviewed Mar 18, 2026

View reviewed changes

chuckdries added 4 commits March 18, 2026 16:38

Don't coerce empty arrays for merge when value is not an object, and …

6eec8b4

…improve logging

Do not allow empty array coercion in setWithRetry

8a881f4

Actually, allow empty array coercion in setWithRetry. Might as well b…

97ae1fb

…e consistent

Prettier diff

08939c9

rafecolton previously approved these changes Mar 19, 2026

View reviewed changes

tgolen requested changes Mar 19, 2026

View reviewed changes

chuckdries added 3 commits March 19, 2026 13:57

Cover setWithRetry and mergeCollection for empty array coercion behavior

dfaa39b

Improve checkCompatibilityWithExistingValue comment

f00e111

Revert unnecessary changes to mergeInternal

dad8aa4

chuckdries dismissed rafecolton’s stale review via dad8aa4 March 19, 2026 21:09

Better comments for future engineers

9ce1bc7

rafecolton approved these changes Mar 20, 2026

View reviewed changes

rafecolton requested review from tgolen and removed request for cristipaval March 20, 2026 00:14

tgolen approved these changes Mar 20, 2026

View reviewed changes

tgolen merged commit 109b22e into main Mar 20, 2026
10 checks passed

rafecolton added a commit to Expensify/App that referenced this pull request Mar 20, 2026

Update react-native-onyx to include Expensify/react-native-onyx#754

29623ff

This was referenced Mar 20, 2026

Update react-native-onyx to include https://github.com/Expensify/react-native-onyx/pull/754 Expensify/App#85983

Merged

Revert "Remove macro/micro tasks during subscriber update" #758

Merged

rafecolton added a commit to Expensify/App that referenced this pull request Mar 20, 2026

Merge pull request #85983 from Expensify/onyx-update

d446740

Update react-native-onyx to include Expensify/react-native-onyx#754

		if (Array.isArray(existingValue) && existingValue.length === 0 && !Array.isArray(value)) {
		return {isCompatible: true};

Conversation

chuckdries commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Manual Tests

Author Checklist

Screenshots/Videos

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafecolton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafecolton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rafecolton left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

os-botify bot commented Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chuckdries commented Mar 18, 2026 •

edited

Loading