WIP fix: Make storage calls async and await the init task #727

adhorodyski · 2026-01-29T23:48:14Z

Details

This PR makes Onyx storage calls async so we can await the initialization task before executing them.

Related Issues

Automated Tests

Manual Tests

(With EApp)
2. In dev, put a new Log.info call really early on, before Onyx initializes (example)
3. Log into the app
4. Navigate to Workspaces -> New -> New Workspace (/workspaces/confirmation)
5. Refresh the page
6. Observe the Currency field reporting undefined and navigating into it results in a missing list of currencies to choose from
7. Remove the newly added Log.info call
8. Go back to the app, refresh it
9. Observe currency is displayed correctly and the list (while navigating into the pick list) is displayed correctly

Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

tgolen · 2026-01-30T01:49:59Z

lib/Onyx.ts

I think we still need to return promises in all these cases. There is still code that uses them, isn't there?

ordering

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e176ab0db3

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-01-30T02:18:22Z

lib/Onyx.ts

 function update<TKey extends OnyxKey>(data: Array<OnyxUpdate<TKey>>): Promise<void> {
-    // First, validate the Onyx object is in the format we expect
-    for (const {onyxMethod, key, value} of data) {
-        if (!Object.values(OnyxUtils.METHOD).includes(onyxMethod)) {
-            throw new Error(`Invalid onyxMethod ${onyxMethod} in Onyx update.`);
-        }
-        if (onyxMethod === OnyxUtils.METHOD.MULTI_SET) {
-            // For multiset, we just expect the value to be an object
-            if (typeof value !== 'object' || Array.isArray(value) || typeof value === 'function') {
-                throw new Error('Invalid value provided in Onyx multiSet. Onyx multiSet value must be of type object.');
+    return OnyxUtils.getDeferredInitTask().promise.then(() => {
+        // First, validate the Onyx object is in the format we expect
+        for (const {onyxMethod, key, value} of data) {
+            if (!Object.values(OnyxUtils.METHOD).includes(onyxMethod)) {


Preserve synchronous validation errors in update

Wrapping the entire body in getDeferredInitTask().promise.then(...) makes the validation errors throw asynchronously. Callers (and tests like tests/unit/onyxTest.ts around the “should throw an error…” case) currently rely on a synchronous throw from Onyx.update to fail fast when invalid data is passed; with this change, the same misuse becomes an unhandled promise rejection unless every caller awaits/catches the returned promise. This is a behavior change that breaks existing try/catch usage and can let invalid updates slip past immediate error handling.

Useful? React with 👍 / 👎.

@adhorodyski @roryabraham @tgolen

Overall the comment is right about these throws in Onyx.update. In the particular case where some Onyx.updates were called before initialisation and they happen to fail after Onyx.init, they won't throw synchronous errors anymore as they were chained in getDeferredInitTask().promise.then(...). That would represent a problem in E/App if we were handling them with try/catch, but:

I couldn't find any logic in E/App where we are using try/catch to handle Onyx.update synchronous failures. In fact, we rarely handle the asynchronous Onyx failures too.

This synchronous error throwing would made the application crash, it's the only Onyx method where we do this and it's even different from how we are handling wrong mergeCollection updates, just a few lines below in the same function.

So in this commit I made some adjustments to:

Instead of throwing errors, handle these cases with Logger.logInfo and skipping the individual operation in the same way we are doing with mergeCollection at the same function, so the error handling there are now standartised.

Fix the tests to accommodate these changes (actually these tests were never "working" before because of the way they were written)

Let me know what you think.

fabioh8010 · 2026-02-02T18:31:36Z

Updates:

I've applied this change to make the Onyx methods be executed directly (without additional promise chain) if Onyx is already initialised, which fixed all the failing unit tests here.
I tested the changes against E/App in this PR and unit tests were getting stuck due to lack of Onyx.init in some files, which I fixed with these changes.
However, after putting the missing Onyx.init, there are a few tests that started to fail. I noticed that even without the Onyx changes the exact same tests are going to fail anyway if I put the Onyx.init in them, so they were probably already "broken" before.

tgolen · 2026-02-02T20:50:39Z

lib/Onyx.ts

+    if (OnyxUtils.getDeferredInitTask().isResolved) {
+        return OnyxUtils.setWithRetry({key, value, options});
+    }
+
+    return OnyxUtils.getDeferredInitTask().promise.then(() => OnyxUtils.setWithRetry({key, value, options}));


Let's DRY this pattern up into a utility method like performAction(() => OnyxUtils.setWithRetry({key, value, options}))

fair, applying this one

applied with afterInit, happy to change to a better name (wanted something more explicit)

Replace repeated deferredInitTask checks with OnyxUtils.afterInit to simplify async initialization handling for set, multiSet, merge, clear, update, and collection methods.

tgolen

That looks great. Thank you!

fabioh8010 · 2026-02-03T17:33:54Z

Let's hold merge a bit, I'm adding some unit tests

adhorodyski · 2026-02-04T13:07:57Z

tests/unit/onyxUtilsTest.ts

+            expect(callback).toHaveBeenCalledTimes(1);
+        });
+
+        it('should only execute the callback after Onyx initialization', async () => {


roryabraham

One other idea to clean this up a bit would be to create a new file, OnyxInit, that encapsulates OnyxInit.init and the logic for the initialization promise. Right now, it's kind of awkwardly split between Onyx.ts and OnyxUtils.ts.

Ultimately I don't really have any strictly blocking comments though, so I'm approving.

roryabraham · 2026-02-04T02:33:20Z

lib/createDeferredTask.ts

Taking a look at this file, I think it could be replaced by Promise.withResolvers. We have a Polyfill for it in E/App (for Hermes).

I think that could end up being a nice simplification.

Even if you don't go that route, is there any advantage to adding isResolved as opposed to just awaiting the promise we've already got? How important is it the Onyx.set proceeds in the same microtask?

Taking a look at this file, I think it could be replaced by Promise.withResolvers. We have a Polyfill for it in E/App (for Hermes).

I think that could end up being a nice simplification.

Will have a look on this.

Even if you don't go that route, is there any advantage to adding isResolved as opposed to just awaiting the promise we've already got? How important is it the Onyx.set proceeds in the same microtask?

Answered here.

Promise.withResolvers

Done, much better now 🎉

roryabraham · 2026-02-04T02:37:04Z

lib/Onyx.ts

-                // The key is a skippable one, so we set the new changes to undefined.
-                // eslint-disable-next-line no-param-reassign
-                changes = undefined;
+    const mergeOperation = () => {


In my opinion keeping this anonymous inside Onyx.afterInit is a bit cleaner.

roryabraham · 2026-02-04T03:02:49Z

lib/OnyxUtils.ts

@@ -136,6 +136,21 @@ function getDeferredInitTask(): DeferredTask {
    return deferredInitTask;
 }


I know you didn't add them here, but I feel like the getters in this function don't provide any useful abstraction. In java, it's a common practice to have private members with public getters and public (semantic) setters.

This doesn't accomplish that - the "getters" just provide a mutable reference to a variable, which isn't really any more helpful than just exporting the variable directly. We could just get rid of these functions and return the variable directly, and it would be a simplification.

Alternatively, we could create semantic getters and setters based on logical usage (i.e: enqueueMerge, hasPendingMergeForKey, etc...), but keep the variables private. That seems like a better improvement.

If we're planning to keep the merge queue for the time being, I think it'd be cleaner to move that into a dedicated file like MergeQueue instead of OnyxUtils.ts. These utils files can grow very large and hard to follow.

(the comments in this thread can be out-of-scope for this PR btw, but I'd be happy to review a follow-up PR if you agree)

lib/OnyxUtils.ts

roryabraham · 2026-02-04T03:06:07Z

lib/Onyx.ts

-            // For multiset, we just expect the value to be an object
-            if (typeof value !== 'object' || Array.isArray(value) || typeof value === 'function') {
-                throw new Error('Invalid value provided in Onyx multiSet. Onyx multiSet value must be of type object.');
+    const updateOperation = () => {


I think this is a bit clearer if you just keep the function anonymous.

Makes sense, it's like this because we wasn't using the extracted afterInit function before. I will adjust.

fabioh8010 · 2026-02-05T09:46:58Z

Updates:

Minor adjustments (fdf95e5, 6d22df1).
Addressed Codex comment (5cf81ca).
Refactored createDeferredTask to use Promise.withResolvers (f351223).
E/App PR testing these Onyx changes can be checked here -> Fix stuck tests because of lack of Onyx.init App#81207

@roryabraham I agree we check/address these comments (#727 (comment), #727 (comment)) in a follow-up.

melvin-bot bot assigned adhorodyski Jan 29, 2026

tgolen requested changes Jan 30, 2026

View reviewed changes

lib/Onyx.ts

Copy link

Collaborator

tgolen Jan 30, 2026

Choose a reason for hiding this comment

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

I think we still need to return promises in all these cases. There is still code that uses them, isn't there?

Wait for deferred initialization before Onyx operations to ensure proper

e176ab0

ordering

adhorodyski force-pushed the fix/await-onyx-init branch from 5cf97ad to e176ab0 Compare January 30, 2026 02:05

adhorodyski marked this pull request as ready for review January 30, 2026 02:16

adhorodyski requested a review from a team as a code owner January 30, 2026 02:16

melvin-bot bot requested review from carlosmiceli and removed request for a team January 30, 2026 02:17

chatgpt-codex-connector bot reviewed Jan 30, 2026

View reviewed changes

roryabraham self-requested a review January 30, 2026 02:20

adhorodyski and others added 2 commits January 29, 2026 18:22

run prettier

fd8dc85

Execute Onyx methods directly if Onyx initialization is already done

772918f

tgolen requested changes Feb 2, 2026

View reviewed changes

Refactor Onyx operations to use afterInit helper

82c7735

Replace repeated deferredInitTask checks with OnyxUtils.afterInit to simplify async initialization handling for set, multiSet, merge, clear, update, and collection methods.

tgolen previously approved these changes Feb 3, 2026

View reviewed changes

Implement tests for Onyx initialization and afterInit()

ceb5a16

fabioh8010 dismissed tgolen’s stale review via ceb5a16 February 4, 2026 09:53

adhorodyski commented Feb 4, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix/await-onyx-init

143afe9

roryabraham previously approved these changes Feb 4, 2026

View reviewed changes

carlosmiceli removed their request for review February 4, 2026 17:00

fabioh8010 added 3 commits February 4, 2026 18:13

Improve afterInit TS

fdf95e5

Adjust Onyx methods logic to be inside afterInit

6d22df1

Standartise validations in Onyx.update

5cf81ca

fabioh8010 dismissed roryabraham’s stale review via 5cf81ca February 5, 2026 08:45

Use Promise.withResolvers in createDeferredTask

f351223

mountiny mentioned this pull request Feb 9, 2026

Onyx write methods can execute before Onyx.init() completes, causing race conditions Expensify/App#81835

Open

fabioh8010 added 2 commits February 9, 2026 10:41

Merge branch 'main' into fix/await-onyx-init

82c28d3

Fix tests

78c9a19

		@@ -136,6 +136,21 @@ function getDeferredInitTask(): DeferredTask {
		return deferredInitTask;
		}

WIP fix: Make storage calls async and await the init task #727

Are you sure you want to change the base?

WIP fix: Make storage calls async and await the init task #727

Conversation

adhorodyski commented Jan 29, 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!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabioh8010 commented Feb 2, 2026

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!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

fabioh8010 commented Feb 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabioh8010 commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adhorodyski commented Jan 29, 2026 •

edited

Loading