feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396
Open
diegolmello wants to merge 3 commits into
Open
feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396diegolmello wants to merge 3 commits into
diegolmello wants to merge 3 commits into
Conversation
…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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughEnables SQLCipher for expo-sqlite, adds a key service for per-db encryption keys, implements a connection lifecycle enforcing PRAGMA ordering and registry caching, and provides reactive hooks (useTableQuery, useRowObserve) with tests for behavior and 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
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->>ConnectionModule: resolveDbDirectory()
ConnectionModule->>openDatabaseAsync: openDatabaseAsync(dbName, options)
openDatabaseAsync-->>ConnectionModule: SQLiteDatabase handle
Note over ConnectionModule: Apply PRAGMAs in order
ConnectionModule->>ConnectionModule: PRAGMA key = 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)
Drizzle-->>ConnectionModule: ExpoSQLiteDatabase
ConnectionModule->>ConnectionModule: Register handle in cache
ConnectionModule-->>Caller: DbHandle { db, sqlite, dbName }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labelstype: feature 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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}
Contributor
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!
Comment on lines
+151
to
+171
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | ||
| const cached = _registry.get(dbName); | ||
| if (cached) { | ||
| return cached as DbHandle<K>; | ||
| } | ||
|
|
||
| const keyHex = await getOrCreateDatabaseKey(dbName); | ||
|
|
||
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | ||
|
|
||
| await applyOpenPragmas(sqlite, keyHex); | ||
|
|
||
| const schema = kind === 'servers' ? serversSchema : appSchema; | ||
| // The conditional type SchemaForKind<K> cannot be narrowed by the JS runtime check above; | ||
| // casting through unknown is the standard TS pattern for this shape. | ||
| const db = drizzle(sqlite, { schema }) as unknown as ExpoSQLiteDatabase<SchemaForKind<K>>; | ||
|
|
||
| const handle: DbHandle<K> = { db, sqlite, dbName }; | ||
| _registry.set(dbName, handle as DbHandle); | ||
| return handle; | ||
| } |
Contributor
There was a problem hiding this comment.
Resource leak when applyOpenPragmas fails.
If applyOpenPragmas throws (e.g., open-verify fails), the sqlite handle returned by openDatabaseAsync is never closed. This leaks an open file descriptor.
🛠️ Proposed fix to close handle on failure
async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> {
const cached = _registry.get(dbName);
if (cached) {
return cached as DbHandle<K>;
}
const keyHex = await getOrCreateDatabaseKey(dbName);
const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY);
- await applyOpenPragmas(sqlite, keyHex);
+ try {
+ await applyOpenPragmas(sqlite, keyHex);
+ } catch (e) {
+ await sqlite.closeAsync();
+ throw e;
+ }
const schema = kind === 'servers' ? serversSchema : appSchema;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | |
| const cached = _registry.get(dbName); | |
| if (cached) { | |
| return cached as DbHandle<K>; | |
| } | |
| const keyHex = await getOrCreateDatabaseKey(dbName); | |
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | |
| await applyOpenPragmas(sqlite, keyHex); | |
| const schema = kind === 'servers' ? serversSchema : appSchema; | |
| // The conditional type SchemaForKind<K> cannot be narrowed by the JS runtime check above; | |
| // casting through unknown is the standard TS pattern for this shape. | |
| const db = drizzle(sqlite, { schema }) as unknown as ExpoSQLiteDatabase<SchemaForKind<K>>; | |
| const handle: DbHandle<K> = { db, sqlite, dbName }; | |
| _registry.set(dbName, handle as DbHandle); | |
| return handle; | |
| } | |
| async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> { | |
| const cached = _registry.get(dbName); | |
| if (cached) { | |
| return cached as DbHandle<K>; | |
| } | |
| const keyHex = await getOrCreateDatabaseKey(dbName); | |
| const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY); | |
| try { | |
| await applyOpenPragmas(sqlite, keyHex); | |
| } catch (e) { | |
| await sqlite.closeAsync(); | |
| throw e; | |
| } | |
| const schema = kind === 'servers' ? serversSchema : appSchema; | |
| // The conditional type SchemaForKind<K> cannot be narrowed by the JS runtime check above; | |
| // casting through unknown is the standard TS pattern for this shape. | |
| const db = drizzle(sqlite, { schema }) as unknown as ExpoSQLiteDatabase<SchemaForKind<K>>; | |
| const handle: DbHandle<K> = { db, sqlite, dbName }; | |
| _registry.set(dbName, handle as DbHandle); | |
| return handle; | |
| } |
🤖 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/connection.ts` around lines 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.
Comment on lines
+81
to
+99
| export async function getOrCreateDatabaseKey(dbName: string): Promise<string> { | ||
| const k = storageKey(dbName); | ||
| const existing = await _shim.getItem(k); | ||
| if (existing !== null) { | ||
| return existing; | ||
| } | ||
|
|
||
| // randomKey (not randomBytes): the mobile-crypto bridge encodes randomBytes as | ||
| // BASE64 on both platforms, while randomKey returns hex (SecureRandom/SecRandomCopyBytes | ||
| // + bytesToHex). The argument is the byte count: 32 bytes → 64 hex chars. | ||
| const hex = await randomKey(32); | ||
| // Guard the bridge contract: anything but 64 hex chars must not be used as a key | ||
| if (!/^[0-9a-fA-F]{64}$/.test(hex)) { | ||
| // Sanitize error — do not include the value in the message | ||
| throw new Error('key generation produced unexpected output; cannot open database safely'); | ||
| } | ||
|
|
||
| await _shim.setItem(k, hex); | ||
| return hex; |
Contributor
There was a problem hiding this comment.
Serialize first-write key creation per database name.
Concurrent first calls can generate different keys for the same dbName; one caller may receive a key that is no longer persisted. That breaks idempotence and can cause sporadic encrypted-open failures.
Proposed fix
const KEY_PREFIX = 'db_key_v1:';
+const _inflight = new Map<string, Promise<string>>();
function storageKey(dbName: string): string {
return `${KEY_PREFIX}${dbName}`;
}
export async function getOrCreateDatabaseKey(dbName: string): Promise<string> {
const k = storageKey(dbName);
- const existing = await _shim.getItem(k);
- if (existing !== null) {
- return existing;
- }
-
- const hex = await randomKey(32);
- if (!/^[0-9a-fA-F]{64}$/.test(hex)) {
- throw new Error('key generation produced unexpected output; cannot open database safely');
- }
-
- await _shim.setItem(k, hex);
- return hex;
+ const pending = _inflight.get(k);
+ if (pending) {
+ return pending;
+ }
+
+ const task = (async (): Promise<string> => {
+ const existing = await _shim.getItem(k);
+ if (existing !== null) {
+ return existing;
+ }
+
+ const hex = await randomKey(32);
+ if (!/^[0-9a-fA-F]{64}$/.test(hex)) {
+ throw new Error('key generation produced unexpected output; cannot open database safely');
+ }
+
+ await _shim.setItem(k, hex);
+ return hex;
+ })();
+
+ _inflight.set(k, task);
+ try {
+ return await task;
+ } finally {
+ _inflight.delete(k);
+ }
}🤖 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` around lines 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.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Chores
Tests