feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396
Conversation
…r-server databases
…ks (NATIVE-1274) Delivers the driver adapter layer sitting between expo-sqlite and the rest of the app, as the only permitted call site for expo-sqlite: - connection.ts: open/close lifecycle with App Group path resolution (iOS), post-open PRAGMA key → busy_timeout=500 → WAL invariant, per-DB handle registry, drizzle() wrapping, deleteDb support. - keyService.ts: getOrCreateDatabaseKey / deleteDatabaseKey backed by a keychainShim interface; shim defaults to an in-memory dev stand-in pending the native Keychain binding (NATIVE-1276). CSPRNG from @rocket.chat/mobile-crypto randomBytes (64 hex chars / 32 bytes). - observe.ts: useTableQuery (V2 structural-sharing list hook, ~16ms debounce, table-filtered addDatabaseChangeListener) and useRowObserve (V3 per-rowid hook), both ported from the on-device validated PoC. - ios/Podfile.properties.json: expo.sqlite.useSQLCipher = "true" - android/gradle.properties: expo.sqlite.useSQLCipher=true - expo-sqlite ~16.0.10 added via `expo install` (SDK-54 compatible) L1 Jest tests cover: key creation/idempotence, no key material in errors, shim replacement, DB name derivation, open-sequence ordering (PRAGMA key first), busy_timeout, WAL, registry dedup, debounce coalescing, table filtering, structural sharing (same ref/new ref), useRowObserve rowId matching. 33 tests added; full suite 1572 tests green.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughEnables SQLCipher for expo-sqlite via build configuration, implements a key and salt service with keychain abstraction and persistence, establishes a database connection module enforcing strict PRAGMA ordering and registry caching, and provides reactive observation hooks (useTableQuery, useRowObserve) with comprehensive tests for behavior, filtering, structural sharing, and error safety. ChangesEncrypted SQLite Database Integration
Sequence Diagram(s)sequenceDiagram
participant App
participant KeyService
participant IKeychainShim
participant randomKey as CSPRNG (randomKey)
App->>KeyService: installKeychainShim(nativeShim)
Note over KeyService: Replace dev shim with native
App->>KeyService: getOrCreateDatabaseKey(dbName)
KeyService->>IKeychainShim: getItem(db_key_v1:dbName)
alt Key exists
IKeychainShim-->>KeyService: 64-hex key
else Key not found
KeyService->>randomKey: Generate 32 bytes
randomKey-->>KeyService: 32-byte buffer
KeyService->>KeyService: Encode as 64 hex chars, validate
KeyService->>IKeychainShim: setItem(db_key_v1:dbName, key)
IKeychainShim-->>KeyService: stored
end
KeyService-->>App: 64-hex key
sequenceDiagram
participant Caller
participant ConnectionModule
participant KeyService
participant openDatabaseAsync as expo-sqlite
participant Drizzle
Caller->>ConnectionModule: openServerDb(serverUrl)
ConnectionModule->>ConnectionModule: deriveServerDbName(serverUrl)
ConnectionModule->>KeyService: getOrCreateDatabaseKey(dbName)
KeyService-->>ConnectionModule: 64-hex key
ConnectionModule->>KeyService: getOrCreateDatabaseSalt(dbName)
KeyService-->>ConnectionModule: 32-hex salt
ConnectionModule->>ConnectionModule: resolveDbDirectory()
ConnectionModule->>openDatabaseAsync: openDatabaseAsync(dbName, options)
openDatabaseAsync-->>ConnectionModule: SQLiteDatabase handle
Note over ConnectionModule: Apply PRAGMAs in strict order
ConnectionModule->>ConnectionModule: PRAGMA key = x'...'
ConnectionModule->>ConnectionModule: PRAGMA cipher_plaintext_header_size = 32
ConnectionModule->>ConnectionModule: PRAGMA cipher_salt = x'...'
ConnectionModule->>ConnectionModule: PRAGMA busy_timeout = 500
ConnectionModule->>ConnectionModule: PRAGMA journal_mode = WAL
ConnectionModule->>ConnectionModule: SELECT COUNT(*) FROM sqlite_master
Note over ConnectionModule: Verify encryption successful
ConnectionModule->>Drizzle: Wrap with schema (app or servers)
Drizzle-->>ConnectionModule: ExpoSQLiteDatabase
ConnectionModule->>ConnectionModule: Register handle in cache
ConnectionModule-->>Caller: DbHandle { db, sqlite, dbName }
sequenceDiagram
participant Component
participant useTableQuery as useTableQuery Hook
participant ExpoSQLite as expo-sqlite Listener
participant queryFn as queryFn Callback
Component->>useTableQuery: useTableQuery(dbHandle, options)
useTableQuery->>queryFn: Call synchronously on mount
queryFn-->>useTableQuery: Initial rows []
useTableQuery->>ExpoSQLite: addDatabaseChangeListener(callback)
Note over useTableQuery: Subscribe to changes
Component->>Component: Rapid table changes fire events
Component->>ExpoSQLite: Emit event (table, rowId, databaseFilePath)
ExpoSQLite->>useTableQuery: Change event
useTableQuery->>useTableQuery: Filter by dbName and allowed tables
useTableQuery->>useTableQuery: Debounce 16ms timer
useTableQuery->>queryFn: Call after debounce window
queryFn-->>useTableQuery: Updated rows []
useTableQuery->>useTableQuery: Reconcile by rowKey, reuse via equalFn
useTableQuery-->>Component: State update with new/reused rows
Component->>Component: Unmount
useTableQuery->>ExpoSQLite: Remove listener
useTableQuery->>useTableQuery: Clear debounce timer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/lib/database/driver/__tests__/keyService.test.ts (1)
53-62: ⚡ Quick winThe test title claims key difference, but the assertions don’t verify it.
With a constant
randomKeymock, this case only checks format. Please either rename the test to match current intent or mock distinct values and assert isolation behavior directly.Stronger deterministic test option
it('returns different keys for different db names', async () => { - // The mock always returns the same hex but each name gets its own entry; - // since we mock randomKey to the same value both will equal the mock value — - // the key isolation per name is structural (separate store entries), tested via delete below. + (randomKey as jest.Mock) + .mockResolvedValueOnce('1111111111111111111111111111111111111111111111111111111111111111') + .mockResolvedValueOnce('2222222222222222222222222222222222222222222222222222222222222222'); const k1 = await getOrCreateDatabaseKey('server-a.db'); const k2 = await getOrCreateDatabaseKey('server-b.db'); - // Both are 64-hex; they happen to be equal under the mock but the storage keys differ - expect(k1).toMatch(/^[0-9a-fA-F]{64}$/); - expect(k2).toMatch(/^[0-9a-fA-F]{64}$/); + expect(k1).not.toBe(k2); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/__tests__/keyService.test.ts` around lines 53 - 62, The test title says "returns different keys for different db names" but it only asserts format because randomKey is mocked to a constant; either rename the test to reflect format-only verification or modify the mock for randomKey and assert isolation: change the mock of randomKey to return distinct deterministic values for successive calls and then assert k1 !== k2 (or verify store-level isolation by checking deletion of one DB key doesn't remove the other) when calling getOrCreateDatabaseKey; update the test title and expectations accordingly.app/lib/database/driver/__tests__/observe.test.ts (1)
19-60: 💤 Low valueUnused
mockSubscriptionvariable.
mockSubscriptionis defined but never used — eachaddDatabaseChangeListenercall returns a fresh object with its ownremovefunction (lines 31-36). Consider removing it to reduce confusion.🧹 Proposed cleanup
-const mockSubscription = { remove: jest.fn() }; - jest.mock('expo-sqlite', () => ({ addDatabaseChangeListener: jest.fn((fn: ChangeListener) => { _listeners.push(fn); return { remove: jest.fn(() => { const idx = _listeners.indexOf(fn); if (idx !== -1) _listeners.splice(idx, 1); }) }; }) }));Also remove the unused
mockSubscription.remove.mockClear()call inbeforeEach(line 55).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/__tests__/observe.test.ts` around lines 19 - 60, The test file defines an unused mockSubscription constant and calls mockSubscription.remove.mockClear() in beforeEach, which is confusing since addDatabaseChangeListener returns its own remove object; remove the mockSubscription declaration and the mockSubscription.remove.mockClear() call, and keep the real per-subscription remove behavior from the jest.mock implementation (reference: mockSubscription, addDatabaseChangeListener and the beforeEach block).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/database/driver/connection.ts`:
- Around line 151-171: openDb currently calls openDatabaseAsync then
applyOpenPragmas; if applyOpenPragmas throws the opened sqlite handle is never
closed. Wrap the applyOpenPragmas call in a try/catch (inside openDb) and in the
catch await/ call sqlite.close() (or the appropriate close method on the
returned sqlite) to ensure the DB handle is released, then rethrow the original
error; keep registry logic unchanged so _registry.set(dbName, ...) only runs
after successful open and pragmas.
In `@app/lib/database/driver/keyService.ts`:
- Around line 81-99: Concurrent callers to getOrCreateDatabaseKey can race and
persist different keys; serialize first-write per dbName by introducing per-name
in-flight serialization: add a Map (e.g., pendingKeys) keyed by
storageKey(dbName) that stores the Promise for the ongoing creation, return that
promise if present, otherwise create and store the promise for the generator;
inside the creation promise perform the existing check via _shim.getItem(k),
generate hex via randomKey(32), validate it, then before setItem re-check
_shim.getItem(k) and if it now exists return that value instead of overwriting;
finally call _shim.setItem(k, hex) and cleanup the pendingKeys entry. Ensure all
references use getOrCreateDatabaseKey, storageKey, _shim.getItem, _shim.setItem,
randomKey and that the pending map is cleared on completion/error.
---
Nitpick comments:
In `@app/lib/database/driver/__tests__/keyService.test.ts`:
- Around line 53-62: The test title says "returns different keys for different
db names" but it only asserts format because randomKey is mocked to a constant;
either rename the test to reflect format-only verification or modify the mock
for randomKey and assert isolation: change the mock of randomKey to return
distinct deterministic values for successive calls and then assert k1 !== k2 (or
verify store-level isolation by checking deletion of one DB key doesn't remove
the other) when calling getOrCreateDatabaseKey; update the test title and
expectations accordingly.
In `@app/lib/database/driver/__tests__/observe.test.ts`:
- Around line 19-60: The test file defines an unused mockSubscription constant
and calls mockSubscription.remove.mockClear() in beforeEach, which is confusing
since addDatabaseChangeListener returns its own remove object; remove the
mockSubscription declaration and the mockSubscription.remove.mockClear() call,
and keep the real per-subscription remove behavior from the jest.mock
implementation (reference: mockSubscription, addDatabaseChangeListener and the
beforeEach block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01046025-0401-439c-ac3a-e3f42052ee89
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
android/gradle.propertiesapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsios/Podfile.properties.jsonpackage.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
ios/Podfile.properties.jsonpackage.jsonapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
🧠 Learnings (3)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
📚 Learning: 2026-05-07T17:47:14.516Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7303
File: package.json:5-5
Timestamp: 2026-05-07T17:47:14.516Z
Learning: When reviewing pnpm `packageManager` version pins in any `package.json` (e.g., `"packageManager": "pnpm@<version>"`), don’t rely solely on web-search results to determine whether a version exists. For very recently published versions, cross-check the target version against the official pnpm release page (https://github.com/pnpm/pnpm/releases) and the npm registry page for pnpm (https://www.npmjs.com/package/pnpm) before flagging the pinned version as non-existent.
Applied to files:
package.json
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.tsapp/lib/database/driver/__tests__/observe.test.tsapp/lib/database/driver/connection.ts
🔇 Additional comments (24)
android/gradle.properties (1)
51-54: LGTM!ios/Podfile.properties.json (1)
1-3: LGTM!package.json (1)
76-76: LGTM!app/lib/database/driver/connection.ts (6)
1-36: LGTM!
90-113: LGTM!
125-144: LGTM!
177-212: LGTM!
214-228: LGTM!
65-77: LGTM!app/lib/database/driver/__tests__/connection.test.ts (6)
18-66: LGTM!
72-91: LGTM!
97-116: LGTM!
122-166: LGTM!
172-193: LGTM!
199-215: LGTM!app/lib/database/driver/observe.ts (4)
32-40: LGTM!
46-67: LGTM!
79-149: LGTM!
164-210: LGTM!app/lib/database/driver/__tests__/observe.test.ts (5)
62-80: LGTM!
86-143: LGTM!
149-194: LGTM!
200-250: LGTM!
256-333: LGTM!
The SQLCipher-encrypted databases run in WAL mode inside the iOS App Group container. iOS kills a suspended app that holds a file lock on a file in a shared container unless it can recognise the WAL file as SQLite — but default SQLCipher encrypts the file header, so iOS denies the idle-WAL background exemption and the held shared lock trips RUNNINGBOARD 0xdead10cc on suspend. Open every database with PRAGMA cipher_plaintext_header_size = 32 so iOS reads the WAL magic and grants the exemption. With a plaintext header SQLCipher no longer stores the salt in the file, so generate and persist a per-database salt alongside the key and supply it via PRAGMA cipher_salt at open time. The 32 plaintext bytes are header metadata only (version/page size), so this is not a security regression. Losing the salt makes the DB unreadable, same as losing the key, so both are destroyed together.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/database/driver/__tests__/keyService.test.ts (1)
115-127: ⚡ Quick winConsider adding a test for concurrent first-write race condition.
The tests verify idempotency under sequential calls, but don't exercise concurrent calls to
getOrCreateDatabaseKeyorgetOrCreateDatabaseSaltfor the samedbName. Adding a test that fires multiple parallel calls and asserts all receive the same value would both document the expected behavior and catch the race condition bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/__tests__/keyService.test.ts` around lines 115 - 127, Add a new test case to verify concurrent first-write behavior for the getOrCreateDatabaseKey and getOrCreateDatabaseSalt functions. The test should fire multiple parallel calls using the same dbName parameter, and assert that all concurrent calls receive the same value back. This test will document the expected concurrent idempotency behavior and help catch potential race condition bugs when multiple async operations try to create the same database salt or key simultaneously.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/database/driver/keyService.ts`:
- Around line 122-137: The function getOrCreateDatabaseSalt has a race condition
where concurrent calls for the same dbName can each generate and store different
salts, causing data loss and permanent database corruption. Apply the same
serialization fix used for getOrCreateDatabaseKey to the getOrCreateDatabaseSalt
function by introducing a locking mechanism (such as a Map of pending promises)
that ensures only one salt generation and storage operation proceeds for each
unique dbName at a time. Alternatively, extract the
check-then-generate-then-store pattern into a shared helper function that both
getOrCreateDatabaseSalt and getOrCreateDatabaseKey can use to eliminate
duplication and ensure consistent race condition handling.
---
Nitpick comments:
In `@app/lib/database/driver/__tests__/keyService.test.ts`:
- Around line 115-127: Add a new test case to verify concurrent first-write
behavior for the getOrCreateDatabaseKey and getOrCreateDatabaseSalt functions.
The test should fire multiple parallel calls using the same dbName parameter,
and assert that all concurrent calls receive the same value back. This test will
document the expected concurrent idempotency behavior and help catch potential
race condition bugs when multiple async operations try to create the same
database salt or key simultaneously.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bae71627-1156-448a-94f9-c82d37ae9942
📒 Files selected for processing (4)
app/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/database/driver/tests/connection.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/driver/keyService.tsapp/lib/database/driver/__tests__/keyService.test.tsapp/lib/database/driver/connection.ts
🔇 Additional comments (11)
app/lib/database/driver/keyService.ts (4)
94-112: Race condition in concurrent first-write remains unaddressed.The past review correctly identified that concurrent first calls to
getOrCreateDatabaseKeycan race: both callers pass theexisting !== nullcheck, both generate different keys viarandomKey, and one overwrites the other's persisted key. The caller that loses the race receives a key that is no longer stored, causing sporadic encrypted-open failures.
2-15: LGTM!
81-85: LGTM!
139-147: LGTM!app/lib/database/driver/connection.ts (4)
171-191: Resource leak whenapplyOpenPragmasfails remains unaddressed.The past review correctly identified that if
applyOpenPragmasthrows (e.g., open-verify fails due to wrong key/salt, or file corruption), thesqlitehandle fromopenDatabaseAsyncis never closed, leaking an open file descriptor.
12-25: LGTM!
43-43: LGTM!
137-164: LGTM!app/lib/database/driver/__tests__/keyService.test.ts (3)
15-27: LGTM!Also applies to: 46-47
91-113: LGTM!
146-157: LGTM!
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Review: driver adapter, key service, live-query hooks
Infrastructure-only PR (nothing imports it yet) and the design is sound — raw-key PRAGMA form is correct, PRAGMA ordering enforced, the plaintext-header + externalised-salt approach for the 0xdead10cc fix is well-reasoned, and the dev-shim production guard is a good rail. Findings below are all fixable before the facade cutover lands; the openDb race and the deriveServerDbName collision are the two worth prioritising.
Reviewed by an automated pass; treat inline comments as suggestions, not blockers.
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Structural review (NATIVE-1274). The hand-rolled reactivity (table-filtered listener + ~16ms debounce + structural sharing) is sound, the stable-ref pattern correctly avoids the stale-closure bug, PRAGMA key ordering and the x'<hex>' form are correct, and key material never reaches logs or thrown errors. Findings inline.
Test gap (low): no test asserts sub.remove() runs on unmount and that a post-unmount change event does not re-query — the most common hook-leak failure, and pinning it would let the dead mounted ref (flagged inline) be removed with confidence.
…unused Insert types - Extract shared messageColumns object spread into messagesTable, threadsTable, and threadMessagesTable; table-specific fields (tmid/tmsg/blocks/tshow/md/comment for messages; tmid/draft_message for threads; subscription_id for thread_messages) added per-table. messages.alias and messages.parse_urls override the shared nullable default to notNull. - Add .notNull() to all columns whose WMDB counterpart lacks isOptional, across subscriptions, rooms, messages, threads, thread_messages, custom_emojis, frequently_used_emojis, uploads, permissions, users (app), and servers_history. - Remove 16 TXxxInsert aliases (InferInsertModel-based) from index.ts; none are consumed in this or any stacked branch. - Regenerate Drizzle migrations (drizzle-kit generate) for both app and servers schemas; new SQL files and snapshots committed. Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
…into 0000 Replace the two-migration sequence (0000 nullable + 0001 ALTER) with a single 0000 baseline that already carries all NOT NULL columns. Nothing has shipped, so there are no existing databases to migrate from the old 0000. App: 0000_wise_mockingbird.sql Servers: 0000_puzzling_colleen_wing.sql Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
… into feat/native-1274-driver-adapter
- connection: coalesce concurrent opens for the same dbName via _inflight map so only one openDatabaseAsync call races; close raw handle on failed PRAGMA application (no fd leak) - connection: replace slashes with '_' not '.' in deriveServerDbName so distinct paths can't collide with host dots - connection: collapse 5 PRAGMA statements into one multi-statement execAsync call; keep the verify getFirstAsync as a separate call - keyService: extract getOrCreate helper with per-storageKey inflight serialization, removing the key/salt duplication; re-validate stored values against the expected hex pattern before returning (corrupt entry throws a safe error with no key material) - keyService: parallelize the two removeItem calls in deleteDatabaseKey - observe: derive stable tableKey string from tables list and use it as the effect dep instead of spreading tables (removes eslint-disable) - observe: remove dead mounted ref and its effect from useRowObserve
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/lib/database/driver/observe.ts (1)
140-142:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnhandled exception in
setTimeoutcallback remains unaddressed.If
fetchAndReconcile(which callsqueryFnRef.current()) throws, the exception escapes the timer callback and becomes an uncaught promise rejection. React error boundaries cannot catch synchronous throws from timer callbacks.🛠️ Suggested fix
- timerRef.current = setTimeout(fetchAndReconcile, debounceMs); + timerRef.current = setTimeout(() => { + try { + fetchAndReconcile(); + } catch (err) { + // Surface via state setter so React boundaries can catch it + setRows(() => { + throw err; + }); + } + }, debounceMs);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/database/driver/observe.ts` around lines 140 - 142, The setTimeout call on the timerRef.current line uses fetchAndReconcile directly as the callback, which means any exception thrown by fetchAndReconcile will become an unhandled error. Wrap the setTimeout callback in an arrow function that contains a try-catch block to handle any errors thrown by fetchAndReconcile. This ensures exceptions are properly caught and can be logged or handled gracefully instead of escaping as uncaught promise rejections that React error boundaries cannot intercept.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/database/driver/keyService.ts`:
- Line 96: The getOrCreate function is declared as async but returns a promise
directly without using await, which violates the require-await linting rule.
Remove the async keyword from the getOrCreate function declaration and let it
return the promise directly. Additionally, apply Prettier's multiline chain
formatting to the cleanup chain code that follows the function (around the lines
that handle promise chaining) to ensure proper code formatting.
---
Duplicate comments:
In `@app/lib/database/driver/observe.ts`:
- Around line 140-142: The setTimeout call on the timerRef.current line uses
fetchAndReconcile directly as the callback, which means any exception thrown by
fetchAndReconcile will become an unhandled error. Wrap the setTimeout callback
in an arrow function that contains a try-catch block to handle any errors thrown
by fetchAndReconcile. This ensures exceptions are properly caught and can be
logged or handled gracefully instead of escaping as uncaught promise rejections
that React error boundaries cannot intercept.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6da02959-7c44-41e0-926a-418bd6caeb2f
📒 Files selected for processing (4)
app/lib/database/driver/__tests__/connection.test.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.tsapp/lib/database/driver/observe.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/database/driver/tests/connection.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/driver/observe.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/lib/database/driver/observe.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/lib/database/driver/observe.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/driver/observe.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/database/driver/observe.tsapp/lib/database/driver/connection.tsapp/lib/database/driver/keyService.ts
🪛 ast-grep (0.43.0)
app/lib/database/driver/keyService.ts
[warning] 103-103: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^[0-9a-fA-F]{${hexLen}}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 113-113: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^[0-9a-fA-F]{${hexLen}}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 ESLint
app/lib/database/driver/connection.ts
[error] 204-204: Insert ⏎↹↹
(prettier/prettier)
[error] 205-205: Insert ↹
(prettier/prettier)
[error] 206-206: Replace }) with ↹})⏎↹↹
(prettier/prettier)
app/lib/database/driver/keyService.ts
[error] 96-96: Async function 'getOrCreate' has no 'await' expression.
(require-await)
[error] 126-126: Insert ⏎↹↹
(prettier/prettier)
[error] 127-127: Insert ↹
(prettier/prettier)
[error] 128-128: Replace }) with ↹})⏎↹↹
(prettier/prettier)
🔇 Additional comments (6)
app/lib/database/driver/keyService.ts (1)
140-142: LGTM!Also applies to: 151-153, 160-162
app/lib/database/driver/connection.ts (3)
79-84: LGTM!
132-158: LGTM!
168-198: LGTM!app/lib/database/driver/observe.ts (2)
123-125: LGTM!
127-154: LGTM!
| * Validates both stored values (corrupt → throw) and generated values (bad bridge → throw). | ||
| * Neither the stored value nor the generated value ever appears in thrown error messages. | ||
| */ | ||
| async function getOrCreate(sk: string, byteLen: number, hexLen: number, label: string): Promise<string> { |
There was a problem hiding this comment.
Make getOrCreate non-async and let Prettier wrap the cleanup chain.
getOrCreate returns the inner promise directly, so async triggers require-await. Lines 126-128 also need Prettier’s multiline chain formatting.
Proposed fix
-async function getOrCreate(sk: string, byteLen: number, hexLen: number, label: string): Promise<string> {
+function getOrCreate(sk: string, byteLen: number, hexLen: number, label: string): Promise<string> {
const inflight = _getOrCreateInflight.get(sk);
if (inflight) return inflight;
@@
- promise.finally(() => {
- _getOrCreateInflight.delete(sk);
- }).catch(() => {});
+ promise
+ .finally(() => {
+ _getOrCreateInflight.delete(sk);
+ })
+ .catch(() => {});
return promise;
}Also applies to: 126-128
🧰 Tools
🪛 ESLint
[error] 96-96: Async function 'getOrCreate' has no 'await' expression.
(require-await)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/database/driver/keyService.ts` at line 96, The getOrCreate function
is declared as async but returns a promise directly without using await, which
violates the require-await linting rule. Remove the async keyword from the
getOrCreate function declaration and let it return the promise directly.
Additionally, apply Prettier's multiline chain formatting to the cleanup chain
code that follows the function (around the lines that handle promise chaining)
to ensure proper code formatting.
Sources: Coding guidelines, Linters/SAST tools
…tion' into feat/native-1274-driver-adapter Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
merged commit f3eab95
into
feat/native-1272-sqlcipher-migration
1 participant
Proposed changes
Second step of the WatermelonDB → expo-sqlite + Drizzle migration, stacked on the schema PR (#7395): adds the encrypted database driver layer. No runtime behavior changes — nothing in the app imports these modules yet; integration arrives with the data-access facade work.
app/lib/database/driver/connection.ts):PRAGMA keyfirst (raw-keyx'<64-hex>'form, skipping PBKDF2 since keys are already full-entropy CSPRNG output), thenPRAGMA busy_timeout = 500(required for multi-process WAL safety with the iOS notification service extension), thenPRAGMA journal_mode = WAL, then an open-verify read that throws a key-free error on failure.group.ios.chat.rocketApp Group container (shared with extensions), with a logged fallback to the default directory; Android uses the default location..dbfilenames from server URLs (drops the legacy.db.dbdouble suffix) and caches handles in a registry so each file opens once.app/lib/database/driver/keyService.ts):randomKeyfrom@rocket.chat/mobile-crypto— chosen overrandomBytes, whose bridge encoding is base64 on both platforms — with a format guard before any key is used.IKeychainShiminterface; the real Keychain/Keystore backend lands with the native-readers work. The built-in in-memory stand-in is dev-only and throws outside__DEV__, because silently using it in production would create databases whose keys vanish on restart (permanent data loss).app/lib/database/driver/observe.ts), patterns validated in the live-query proof of concept:useTableQuery— table-filtered change listener with a 16ms debounce (expo-sqlite fires one event per row, so large transactions must coalesce into one re-query) and structural sharing so unchanged rows keep their object identity andReact.memobails out.useRowObserve— per-rowid subscription for hot single-row sites.addDatabaseChangeListeneris global across all open databases and the default and per-server databases share table names (e.g.users).expo.sqlite.useSQLCipherflag inios/Podfile.properties.jsonandandroid/gradle.properties; addsexpo-sqlite ~16.0.10.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1274
How to test or reproduce
TZ=UTC pnpm test app/lib/database/driver— runs the driver, key-service, and live-query hook tests.pnpm exec tsc --noEmit -p .— type-checks the new modules.TZ=UTC pnpm test(180 suites / 1576 tests passing locally).Screenshots
N/A — no UI changes.
Types of changes
Checklist
Further comments
Stacked on #7395 — merge that first; GitHub will retarget this PR to develop automatically. The native Keychain/Keystore key-storage backend and the native database readers (iOS notification service extension, Android notification path) come in a follow-up, which is why key persistence sits behind the
IKeychainShiminterface here.Summary by CodeRabbit