Skip to content

[Onyx bump] Bump react-native-onyx to 3.0.119 - Onyx GET asynchronous - #102135

Merged
chuckdries merged 32 commits into
Expensify:mainfrom
callstack-internal:feat/onyx-get-async-mutable
Oct 26, 2026
Merged

chuckdries merged 32 commits into
Expensify:mainfrom
callstack-internal:feat/onyx-get-async-mutable

Conversation

@LukasMod

@LukasMod LukasMod commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

  1. Bumps react-native-onyx from 3.0.117 to 3.0.119. The bump contains 2 Onyx PRs:
    • 3.0.118: fix(storage): reopen the IDB connection on UnknownError: Internal error. (fix(storage): reopen the IDB connection on UnknownError: Internal error. react-native-onyx#853). Onyx classified UnknownError: Internal error. as an unclassifiable error, so the operation layer retried each write six times and gave up, while the connection layer never reopened the database. This maps that exact wording to the class the connection layer already owns, so a failed read or write now drops the cached connection, reopens it, and retries (budgeted at 3 attempts, reset on any success). If the reopen does not help, the existing heal budget exhausted alert fires and the write is dropped, instead of every operation burning its full retry budget.
    • 3.0.119: Onyx.get(key) and Onyx.multiGet(keys) become public and typed (Add public Onyx.get - mutable react-native-onyx#850). No app code reads with them yet. The only app change the bump needs is src/setup/addUtilsToWindow.ts and react-native-onyx.d.ts dropping the hand-made connectWithoutView shim and its Window declaration (16 lines), so the dev console Onyx.get and Onyx.log go through the library method. 3.0.119 also adds a cache-miss check to OnyxSubscriptionManager.getState, which backs useOnyx, Onyx.connect, Onyx.get and Onyx.multiGet: when a non-collection key that storage lists is missing from the cache, Onyx reads storage and, if a value is there, logs a hmmm [Onyx] Cache held no value for a key storage still has: <key> once per collection per session; it only logs and still returns undefined.
  2. Two new lint rules, both error repo-wide. rulesdir/no-unsafe-onyx-read decides where a read may run: only in src/components, src/pages, src/hooks and tests, inside a component or hook body or a function nested in one, and never during render, at module scope or inside an effect. A same-file helper that render calls counts as render. Such a read returns stale data or re-runs on every render, so the rule cannot be disabled. Onyx.get and Onyx.multiGet must be called directly, and the Onyx object may only be used through a member: Onyx.get.call, keys.map(Onyx.get), const read = Onyx.get, const {get} = Onyx, {...Onyx}, Onyx[name] or a re-export is reported in every file outside test code, since it hides the key and the call site from lint. window.Onyx = Onyx for the dev console is the one allowed value use. rulesdir/no-onyx-get-snapshot-key decides which keys may be read. It bans the Search snapshot keys (SEARCH_SNAPSHOT_ONYX_KEYS) and any key lint cannot resolve, including each multiGet element, because useOnyx redirects those keys to a Search snapshot and Onyx.get would return live data where the component saw the snapshot. Some reads are known safe, such as a migrated module-level Onyx.connect or a useOnyxWithoutSnapshots subscription, so this rule can be disabled with a mandatory reason.
  3. The Concierge report key is exempt. Inside a Search scope useOnyx redirects it to the snapshot like any other report_ key, but Search never returns the Concierge chat, so no snapshot holds it and the live report_ entry is the only place it exists. The exemption applies only to ${ONYXKEYS.COLLECTION.REPORT}${id} where id is a same-file const read from ONYXKEYS.CONCIERGE_REPORT_ID.
  4. Two lint-pipeline scripts enforce the disable policies. scripts/onyxLintBypass.ts (renamed from onyxConnectBypass.ts) fails lint on any inline disable of no-onyx-connect, no-unsafe-onyx-read or the OnyxUtils import restriction. It also fails on any eslint.seatbelt.tsv row for no-unsafe-onyx-read or no-onyx-get-snapshot-key, and it counts a blanket disable over a by-reference use or a value use of the Onyx object, not only over a call. scripts/onyxLintDisableReason.ts accepts a disable of no-onyx-get-snapshot-key only in the -line or -next-line form with at least 12 letters or digits after --.
  5. Review and docs: ONYX-1 in the coding standards skill lists what lint enforces so the reviewer does not re-check it, and covers what lint cannot see (cross-file render reach, effects in another file, reads ordered after writes, derived keys, values parked in state) plus judging the content of a disable reason. ONYX-DATA-MANAGEMENT.md lists Onyx.get and Onyx.multiGet as read paths, names when a live read is right (a migrated module-level Onyx.connect used only by handlers), and shows the disable shape, including where it sits for a multiGet.
  6. .github/workflows/onyxGetReviewers.yml requests the Onyx performance reviewers and leaves a comment whenever a non-draft PR adds a net-new Onyx.get or Onyx.multiGet call under src/, ignoring mocks, comment lines and the dev console's window.Onyx.get. It mirrors onyxConnectWithoutViewReviewers.yml.
  7. .github/workflows/lint.yml adds eslint-plugin-local-rules/**, src/libs/SnapshotUtils.ts and src/CONST/runtimeDefaults.ts to the ESLint cache key, since ESLint's own cache hashes the config without rule functions, so a rule edit alone never invalidated it.

Automated coverage: tests/unit/NoUnsafeOnyxReadRuleTest.ts (294 cases), tests/unit/NoOnyxGetSnapshotKeyRuleTest.ts (101, including an exhaustive check that the rule rejects exactly the ONYXKEYS entries useOnyx would redirect), tests/unit/OnyxLintBypassTest.ts (88), tests/unit/OnyxLintDisableReasonTest.ts (13) and tests/unit/SnapshotUtilsTest.ts (2, the prefix check useOnyx and the rule now share).

Fixed Issues

$ #101910
PROPOSAL:

Tests

  • Verify that no errors appear in the JS console

Onyx 3.0.118

The steps below cover Onyx #853 (reopen the IDB connection on UnknownError: Internal error.).

Test 1: Data loads and persists after the IndexedDB database is deleted (web only)

  1. Sign in and confirm the app loads your data normally, with no "Something went wrong" or "we have not been able to load all of your data" message.
  2. In the DevTools console run indexedDB.deleteDatabase('OnyxDB');.
  3. Refresh and sign in again.
  4. Confirm your data loads and is persisted across another refresh.
  5. Change a setting (for example the theme), refresh, and confirm it was saved. Alternatively run (await import('/node_modules/idb-keyval/dist/index.js')).get('test') in the console and confirm it resolves without an error.

Onyx 3.0.119

The steps below cover the public Onyx.get and Onyx.multiGet API, the two lint rules with their scripts, and the reviewers workflow. No extra code is needed.

Test 2: Dev console helpers go through the library method (no other usage currently)

  1. Sign in and open a chat.
  2. Run await Onyx.get('session') in the console and confirm it returns the session object.
  3. Run Onyx.log('session') and confirm the session is logged.
  4. Confirm no errors appear in the console.

Offline tests

QA Steps

  • Verify that no errors appear in the JS console

Test 1: Sign in, send a message, sign out and back in (all platforms)

  1. Sign in and confirm your chats load.
  2. Send a message and confirm it appears.
  3. Sign out, then sign back in.
  4. Confirm the chats and the sent message are still there

Test 2: Two web tabs

Prerequisite: signed in on Web in Tab A and Tab B.

  1. Send a message in Tab A and confirm it appears in Tab B.
  2. Sign out in Tab A and confirm Tab B lands on the sign-in page.
  3. Sign back in and confirm both tabs load your chats with no errors.

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
android.native.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.native.mov
iOS: mWeb Safari
ios.web.mov
MacOS: Chrome / Safari

TEST 1:

onyxget-web-1.mov

TEST 2:

Screenshot 2026-09-24 at 11 08 01
onyxget-web-2.mov
web.mov
web.2.tabs.mov

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

@LukasMod
LukasMod force-pushed the feat/onyx-get-async-mutable branch 3 times, most recently from 02c7b44 to f27fb19 Compare September 24, 2026 09:34
Comment thread scripts/onyxConnectBypass.ts Outdated
Comment thread src/setup/addUtilsToWindow.ts
@LukasMod
LukasMod requested a review from tgolen September 25, 2026 08:44
@LukasMod

Copy link
Copy Markdown
Contributor Author

I’ll keep it as a draft until the Onyx change is merged and we can properly bump the Onyx version here

Comment thread .github/workflows/onyxGetReviewers.yml Outdated
@LukasMod
LukasMod force-pushed the feat/onyx-get-async-mutable branch from a8691eb to 4eff1c8 Compare October 16, 2026 12:40
@LukasMod

Copy link
Copy Markdown
Contributor Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Code review

The main risk: both lint rules can be bypassed without any eslint-disable, through ordinary code patterns. I'd fix items 1–4 before merging. Items 5–10 are smaller.

Rule gaps (reads that are not flagged)

  1. Render-time reads through a helper are not flagged. In no-unsafe-onyx-read.js:342, any non-hook, non-component function becomes DEFERRED, which is then treated as EVENT. isReferencedAtRenderTime never checks for a plain call. So these all pass: const read = () => Onyx.get(K); const v = read(); in a component body, const cb = useCallback(() => Onyx.get(K), []); cb();, and a module helper called from render. That is the stale render read the rule exists to stop.
  2. TS-wrapped objects are invisible to both rules. onyxReadUtils.js:263 requires node.object to be a bare Identifier. (Onyx as typeof Onyx).get(snapshotKey) and Onyx!.get(...) are skipped by both rules. onyxLintBypass.ts already unwraps these with unwrapExpression, so the rules should do the same.
  3. Alias tracking depends on source order. Aliases are registered in the VariableDeclarator visitor (onyxReadUtils.js:301). A component that calls read(KEY) above a later module-level const read = Onyx.get; is never recognized. Collect aliases in Program first, or resolve them in Program:exit.

Config robustness

  1. The repo root comes from process.cwd() (onyxReadUtils.js:21). If ESLint runs from outside App/, for example from an editor in a multi-root workspace, REPO_ROOT is null. Then no-onyx-get-snapshot-key throws at load and breaks the whole config, and no-unsafe-onyx-read allows every read because getRepoRelativePath returns null. Resolve the root from __dirname instead.
  2. The lint cache goes stale. no-onyx-get-snapshot-key.js:54 reads SEARCH_SNAPSHOT_ONYX_KEYS from another file at load time. Lint runs with --cache --cache-strategy content, so when a key is added to that list, unchanged files that already read it keep their cached "pass" result. Consider adding that file's hash to the cache key, or note this in the rule docs.
  3. The grandfather entry looks unused. onyxLintBypass.ts:71 allows one bypass in src/setup/addUtilsToWindow.ts. That file only calls window.Onyx.get(...), which the rule does not match, and it has no disable for the read rule. If the entry isn't needed, remove it so it can't silently allow a future disable there.

Workflow (onyxGetReviewers.yml)

  1. Comments can repeat (line 84). If a reviewer drops their review request, the next push requests them again and reposts the whole comment. Check for an existing bot comment first. Also, mentions is built from REVIEWERS rather than reviewersToRequest, so when the PR author is one of the reviewers, the comment pings them.
  2. Detection is text-only (line 40). It misses aliased reads (O.get, const {get: read} = Onyx) and files with no patch in a large diff. This may be acceptable as a best-effort signal, but it's worth stating that in the file.

Repo conventions (App/CLAUDE.md)

  1. Two test files are in the wrong place. OnyxLintBypassTest.ts and OnyxLintDisableReasonTest.ts test scripts/ code. Per CLAUDE.md, they belong in tests/tooling/ and should run under bun:test, not in tests/unit/.
  2. Test cases lack comments. The cases in NoUnsafeOnyxReadRuleTest.ts have no // Given / When / Then comments, which CLAUDE.md requires. This matters most for the cases that are deliberately valid, such as selector: () => Onyx.get(...) and module-scope .then(() => Onyx.get(...)).
Scope

I reviewed the diff at cc5b636 against main, focusing on the ESLint rules, the bypass and disable-reason scripts, and the workflow. I confirmed items 2, 4, 6 and 9 in the source. The other items come from reading the code paths; I didn't run them.


view run

@LukasMod
LukasMod force-pushed the feat/onyx-get-async-mutable branch from 14c55ac to b2af92b Compare October 19, 2026 08:13

@TaduJR TaduJR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @LukasMod. Sorry for the delay. This one needed in-depth review. One route left, see inline.

PR description:

  • Item 3: "Search never returns it" isn't accurate. Inside a Search scope, useOnyx redirects report_ keys to the snapshot by prefix, Concierge included.
  • Item 1: worth mentioning the new cache-miss check in OnyxSubscriptionManager.getState, which can log Cache held no value for a key storage still has.
  • Item 2: missing the direct-call and component-or-hook checks.
  • The test counts are out of date.

Onyx follow-ups Expensify/react-native-onyx#850 (NAB):

  • getState(undefined) now throws. Checking cache.getAllKeys().has(key) first in reportCacheMissIfStorageHasValue fixes it.
  • The cache-miss check marks a collection as reported before storage confirms the miss, and never re-checks the cache afterwards.
  • ?? undefined in get/multiGet is dead code.

Could you check and raise a PR on Onyx side? cc @fabioh8010 for viz

Also

contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md:74

Onyx.clear belongs in this list too. A read right after an un-awaited clear returns the old value.

contributingGuides/philosophies/ONYX-DATA-MANAGEMENT.md:80

Worth adding: mutating a read result and then calling Onyx.set with it is skipped as unchanged, so nothing is notified or saved.

}

return {
MemberExpression(node) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Onyx object used as a value still passes both rules:

const api = {...Onyx};
await api.get(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);

Its only value use in src/ is window.Onyx = Onyx in addUtilsToWindow.ts. Could the rule report any other non-member use outside tests/?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b69f95d. Any use of the Onyx object other than a member access now reports in every file outside test code: spread, alias, argument, return, export, re-export and dynamic keys like Onyx[name]. window.Onyx = Onyx is the one allowed shape, so addUtilsToWindow stays as is. 17fe7b0 makes a blanket disable over such a line count as a bypass. Hope it resolves those edge case scenario. I added tests to cover that too

@github-actions
github-actions Bot requested a review from luacmartins October 23, 2026 10:41
@github-actions

Copy link
Copy Markdown
Contributor

This PR adds a new Onyx.connectWithoutView call, so I've requested a review from the Onyx performance reviewers (@tgolen, @mountiny, @luacmartins, @chuckdries) — a review from any one of them is enough. Please add a link in your PR description to the Slack discussion where the @frontend-performance team approved using connectWithoutView here.

@LukasMod

Copy link
Copy Markdown
Contributor Author

@TaduJR Thanks! The description is updated. Please check whether the reasoning for Item 3 works for you. This is what I confirmed with Carlos (@luacmartins): Search should never return that chat. It's primarily used for handlers and is a frequently changing candidate.

I run extra sessions to look for some exceptions and also updated the comments and JSDoc to make the lint work easier to understand. I’ll validate those Onyx follow-ups separately.

@TaduJR TaduJR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@LukasMod LMK when Expensify/react-native-onyx#863 is ready.

Thanks!

@melvin-bot
melvin-bot Bot requested a review from chuckdries October 23, 2026 18:05
@chuckdries

Copy link
Copy Markdown
Contributor

Everyone has approved, or had previously approved. Merging!

@chuckdries
chuckdries merged commit 6d86f63 into Expensify:main Oct 26, 2026
48 of 49 checks passed
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions

Copy link
Copy Markdown
Contributor

🚧 chuckdries has triggered a test Expensify/App build. You can view the workflow run here.

@github-actions

Copy link
Copy Markdown
Contributor

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/chuckdries in version: 9.5.21-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site update needed. This PR only bumps react-native-onyx, adds lint rules, CI scripts, and contributor docs, so nothing changes for users.


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/chuckdries in version: 9.5.23-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

No help site update needed. This PR only bumps react-native-onyx, adds lint rules, CI scripts, and contributor docs, so nothing changes for users.


view run

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.23-3 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.