Skip to content

feat(storage): use NitroSQLite 10.2 connections and statements - #851

Open
chrispader wants to merge 17 commits into
Expensify:mainfrom
margelo:@chrispader/chore/nitro-sqlite-10-peers
Open

chrispader wants to merge 17 commits into
Expensify:mainfrom
margelo:@chrispader/chore/nitro-sqlite-10-peers

Conversation

@chrispader

@chrispader chrispader commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Onyx's SQLite provider shares one connection for reads and writes and prepares single-key queries on every call. I updated it to use NitroSQLite 10.2's independent reader and reusable statements while preserving read-after-write ordering and the stored JSON format.

Details

The native provider now requires NitroSQLite ^10.2.0, so native apps must upgrade and rebuild. The peer remains optional for other storage providers.

The reader uses a bounded cache and a two-second lock timeout. If reader setup fails, reads use the writer; primary database-open failures retain the memory fallback. Repeated initialization preserves the active connections and queued writes, and chunked reads share one transaction snapshot.

Related Issues

Expensify/App#102424

Linked E/App PR

Expensify/App#102422

Automated Tests

SQLite provider tests use the published NitroSQLite mock and cover statement reuse, write ordering, snapshot consistency, batch rollback, JSON merges, reader setup failures, error classification, and repeated initialization.

Manual Tests

  1. Follow the Tests and Offline tests in the linked App PR with a High Traffic account on existing and fresh iOS and Android installations. Use its linked Mobile-Expensify PR for hybrid builds, and repeat shared flows on web.
  2. After chat and expense edits, force-close and reopen the app. Verify the latest values persist and offline changes sync once after reconnecting.
  3. Export Onyx state through developer tools and verify known chat and expense keys contain their latest values. Clear the cache and reconnect, then verify account data restores correctly.
  4. Verify no JS console or unexpected storage errors appear.

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 - [x] I linked the correct issue in the ### Related Issues section above
  • I linked the corresponding Expensify/App PR in the ### Linked E/App PR section above, and verified this change against it (E/App CI passed and manual testing completed)

  • I wrote clear testing steps that cover the changes made in this PR

    • I added steps for local testing in the Tests section
    • 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 / Chrome
    • iOS / native
    • iOS / 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 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 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.js 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 a new component is created I verified that:

    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. onClick={this.toggleReport} should be bound in the constructor).
    • Any internal methods bound to this are necessary to be bound (i.e. avoid unnecessary binds)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • 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 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 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.

  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.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 followed proper code patterns (see Reviewing the code)

    • 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 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)
  • 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)

  • 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.
  • 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.

  • Verify that no errors appear in the JS console

  • Verify that no errors appear in the JS console

  • I verified any copy / text that was added to the app 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)

  • If the PR adds or modifies the UI:

    • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
    • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
    • I verified all new/changed elements have meaningful accessible names and roles.
    • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).

Screenshots/Videos

No screenshots or videos are included because this storage-provider change has no visible UI.

@chrispader
chrispader force-pushed the @chrispader/chore/nitro-sqlite-10-peers branch from 1a8b643 to 1fa8054 Compare September 28, 2026 12:02
@chrispader chrispader changed the title chore(deps): Support NitroSQLite 10 chore(deps): Require NitroSQLite 10 Sep 28, 2026
@chrispader chrispader changed the title chore(deps): Require NitroSQLite 10 feat(storage): use NitroSQLite 10 connections and statements Sep 28, 2026
@chrispader chrispader changed the title feat(storage): use NitroSQLite 10 connections and statements [WIP] feat(storage): use NitroSQLite 10 connections and statements Sep 28, 2026
@chrispader
chrispader marked this pull request as ready for review September 28, 2026 14:58
@chrispader
chrispader requested a review from a team as a code owner September 28, 2026 14:58
@melvin-bot
melvin-bot Bot requested review from inimaga and removed request for a team September 28, 2026 14:58
@inimaga

inimaga commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

@chrispader Is this ready for review? Held off looking at it since it has WIP in the title

@chrispader

Copy link
Copy Markdown
Contributor Author

@chrispader Is this ready for review? Held off looking at it since it has WIP in the title

@inimaga not yet! I'll ping you once this is ready 🙌🏼

@chrispader chrispader changed the title [WIP] feat(storage): use NitroSQLite 10 connections and statements [WIP] feat(storage): use NitroSQLite 10.1 connections and statements Oct 1, 2026
@chrispader chrispader changed the title [WIP] feat(storage): use NitroSQLite 10.1 connections and statements feat(storage): use NitroSQLite 10.1 connections and statements Oct 2, 2026
@chrispader chrispader changed the title feat(storage): use NitroSQLite 10.1 connections and statements [WIP] feat(storage): use NitroSQLite 10.1 connections and statements Oct 2, 2026
@chrispader chrispader changed the title [WIP] feat(storage): use NitroSQLite 10.1 connections and statements feat(storage): use NitroSQLite 10.1 connections and statements Oct 9, 2026
@chrispader

Copy link
Copy Markdown
Contributor Author

@inimaga this is ready for review!

also cc @fabioh8010 so you can include this in your Onyx improvements list 🙌🏼

@fabioh8010

Copy link
Copy Markdown
Contributor

Thanks! I would like to review this one too 😄

@chrispader chrispader changed the title [WIP] feat(storage): use NitroSQLite 10.2 connections and statements feat(storage): use NitroSQLite 10.2 connections and statements Oct 12, 2026

@fabioh8010 fabioh8010 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.

@chrispader I would appreciate if you could perform some performance measurements with this change (base with store-based solution vs this PR) over the main metrics and flows for Native. We need to make sure performance impact is verified.

Also, would be good to have more extensive test steps/QA testing here as it affects the whole app.

Comment thread lib/storage/providers/classifySQLiteError.ts Outdated
Comment thread lib/storage/providers/SQLiteProvider.ts Outdated
Comment thread lib/storage/providers/SQLiteProvider.ts
Comment thread lib/storage/providers/SQLiteProvider.ts Outdated
Comment thread lib/storage/providers/SQLiteProvider.ts Outdated
Comment thread lib/storage/providers/SQLiteProvider.ts Outdated
Comment thread tests/unit/mocks/sqliteMock.ts Outdated
Comment thread tests/unit/storage/providers/SQLiteProviderTest.ts Outdated
@chrispader chrispader changed the title feat(storage): use NitroSQLite 10.2 connections and statements [WIP] feat(storage): use NitroSQLite 10.2 connections and statements Oct 12, 2026
@chrispader

Copy link
Copy Markdown
Contributor Author

Chris's AI crew member here. 🤖

Following up on Fabio's review, I expanded the manual test steps here and in App #102422 and Mobile-Expensify #14179. They now cover High Traffic accounts, existing and fresh installations, chat history, expenses, tasks, profile changes, offline persistence, reconnects, and account switching. App pins Onyx commit 6ee48a7f and includes the derived-value fixes from the store-based App update. All 693 Onyx automated tests pass.

@chrispader chrispader changed the title [WIP] feat(storage): use NitroSQLite 10.2 connections and statements feat(storage): use NitroSQLite 10.2 connections and statements Oct 13, 2026
@chrispader
chrispader requested a review from inimaga October 13, 2026 11:57
@chrispader

Copy link
Copy Markdown
Contributor Author

Also cc for review @NicolasBonet @ikevin127 since Melvin assigned you on the corresponding issue too 🙌🏼

@chrispader

Copy link
Copy Markdown
Contributor Author

@fabioh8010 i'm working on the benchmarks now!

Comment thread lib/storage/providers/SQLiteProvider.ts Outdated
Comment thread lib/storage/providers/SQLiteProvider.ts Outdated
Comment thread lib/types/modules/react-native-nitro-sqlite.d.ts
Comment thread package.json
Comment thread tests/unit/storage/providers/SQLiteProviderTest.ts Outdated
Comment thread tests/unit/storage/providers/SQLiteProviderTest.ts Outdated
Comment thread lib/storage/providers/classifySQLiteError.ts
@ikevin127

Copy link
Copy Markdown
Contributor

@chrispader That's all from me on code-review, (7) comments ☝️

@fabioh8010

Copy link
Copy Markdown
Contributor

I will wait for Chris to check @ikevin127 's review before I review again

@ikevin127

Copy link
Copy Markdown
Contributor

Thanks for addressing my first-pass comments, I'm doing another pass 🔄

independentReader.execute('PRAGMA busy_timeout=2000;');
getItemStatement = independentReader.prepare('SELECT record_key, valueJSON FROM keyvaluepairs WHERE record_key = ?;');
readStore = independentReader;
} catch {

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.

lib/storage/providers/SQLiteProvider.ts line 182

🟡 This fallback is silent now. If reader setup fails on some devices in production, we quietly lose the main benefit of this PR (reads that don't queue behind writes), and neither telemetry nor the requested benchmarks can tell. IDBKeyValProvider/createStore.ts already logs similar connection events with Logger.logInfo.

Can we do the same here ?

import * as Logger from '../../Logger';
        } catch (error) {
            // SQLite can report failures on first use, so protect all reader setup.
            Logger.logInfo(`SQLite independent reader unavailable, reading through the writer. Error: ${error instanceof Error ? error.message : String(error)}`);

export {NitroSQLiteError, NitroSQLiteExceptionType} from 'react-native-nitro-sqlite';
}

declare module 'react-native-nitro-sqlite/mock' {

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.

lib/types/modules/react-native-nitro-sqlite.d.ts line 6

🟡 This block copies NitroSQLite's shipped declarations by hand. The Pick<NitroSQLiteConnection, 'close' | 'delete' | ...> list is identical to the upstream lib/typescript/commonjs/mock/MockConnection.d.ts, and open, NitroSQLite and resetAllDatabases match lib/typescript/commonjs/mock.d.ts.

When upstream changes the mock, this copy drifts without any error. Legacy Node resolution ignores exports, so it should resolve the physical declaration file.

Could we re-export it, the same way the /errors block re-exports ?

declare module 'react-native-nitro-sqlite/mock' {
    export * from 'react-native-nitro-sqlite/lib/typescript/commonjs/mock';
}

Comment thread CHANGELOG.md
@@ -0,0 +1,9 @@
# Changelog

## Unreleased

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.

CHANGELOG.md line 3

🟡 The repo had no changelog before this PR, and the publish workflow (Expensify/GitHub-Actions npmPublish.yml) runs npm version patch on merge. So this ships as 3.0.120 under an ## Unreleased heading that nothing will ever rename.

A consumer on ^3.0.x with NitroSQLite 9.x still fails dependency resolution on a patch update, and never sees this note. Can we confirm with the Onyx maintainers (@inimaga, @fabioh8010) whether this release should get a manual minor bump or a GitHub release note instead ?

If neither, I'd drop the file rather than start a convention nobody maintains.

@ikevin127

Copy link
Copy Markdown
Contributor

@chrispader That's it from me - ready after these 3 items:

  1. 🟡 Log the reader fallback in SQLiteProvider.ts L182.
  2. 🟡 Replace the hand-copied /mock types with a re-export of the shipped declaration.
  3. 🟡 Settle how the breaking peer change gets released (manual minor bump or GitHub release note), or drop CHANGELOG.md.

Merge separately waits on Fabio's benchmarks 🙌

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.

4 participants