Skip to content

feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396

Open
diegolmello wants to merge 7 commits into
feat/native-1275-drizzle-schemafrom
feat/native-1274-driver-adapter
Open

feat: add expo-sqlite driver adapter, key service, and live-query hooks#7396
diegolmello wants to merge 7 commits into
feat/native-1275-drizzle-schemafrom
feat/native-1274-driver-adapter

Conversation

@diegolmello

@diegolmello diegolmello commented Jun 13, 2026

Copy link
Copy Markdown
Member

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.

  • Connection lifecycle (app/lib/database/driver/connection.ts):
    • Opens databases with SQLCipher encryption from byte zero, enforcing the validated open sequence: PRAGMA key first (raw-key x'<64-hex>' form, skipping PBKDF2 since keys are already full-entropy CSPRNG output), then PRAGMA busy_timeout = 500 (required for multi-process WAL safety with the iOS notification service extension), then PRAGMA journal_mode = WAL, then an open-verify read that throws a key-free error on failure.
    • Places iOS database files in the group.ios.chat.rocket App Group container (shared with extensions), with a logged fallback to the default directory; Android uses the default location.
    • Derives clean single-.db filenames from server URLs (drops the legacy .db.db double suffix) and caches handles in a registry so each file opens once.
  • Key service (app/lib/database/driver/keyService.ts):
    • Generates per-database 32-byte (64-hex) keys via randomKey from @rocket.chat/mobile-crypto — chosen over randomBytes, whose bridge encoding is base64 on both platforms — with a format guard before any key is used.
    • Persists through an IKeychainShim interface; 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).
    • Key material never appears in logs, thrown errors, or telemetry.
  • Live-query hooks (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 and React.memo bails out.
    • useRowObserve — per-rowid subscription for hot single-row sites.
    • Both filter events by database file path before table name: addDatabaseChangeListener is global across all open databases and the default and per-server databases share table names (e.g. users).
  • SQLCipher enablement: expo.sqlite.useSQLCipher flag in ios/Podfile.properties.json and android/gradle.properties; adds expo-sqlite ~16.0.10.
  • Tests: 47 unit tests covering the open sequence and pragma order, registry behavior, name derivation (including trailing slashes), key idempotence and deletion, the dev-shim production guard, cross-database event isolation, debounce coalescing, and structural sharing.

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.
  • Full suite: TZ=UTC pnpm test (180 suites / 1576 tests passing locally).

Screenshots

N/A — no UI changes.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

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 IKeychainShim interface here.

Summary by CodeRabbit

  • New Features
    • Enabled encrypted SQLite databases via SQLCipher
    • Added live-query React hooks for table and row updates with debounced re-fetching and render-stable result reuse
  • Chores
    • Updated platform configuration to disable edge-to-edge display on Android
    • Added/updated Expo SQLite + SQLCipher setup
  • Tests
    • Expanded Jest coverage for encrypted DB connection lifecycle, key/salt generation and persistence, and live-query event behavior

…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.
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0ee0f73a-c605-432e-b9f0-5296d73e5119

📥 Commits

Reviewing files that changed from the base of the PR and between 8b6befb and ee159ef.

📒 Files selected for processing (1)
  • app/lib/database/driver/connection.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/lib/database/driver/connection.ts

Walkthrough

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

Changes

Encrypted SQLite Database Integration

Layer / File(s) Summary
Build configuration and dependency setup
android/gradle.properties, ios/Podfile.properties.json, package.json
Enable SQLCipher via expo.sqlite.useSQLCipher=true on Android and iOS. Add expo-sqlite ~16.0.10 runtime dependency.
Database encryption key and salt service
app/lib/database/driver/keyService.ts, app/lib/database/driver/__tests__/keyService.test.ts
Introduce IKeychainShim abstraction for persistent storage, a dev-only in-memory shim, and installKeychainShim for native shim installation. Implement getOrCreateDatabaseKey (32-byte CSPRNG → 64 hex chars) and getOrCreateDatabaseSalt (16-byte CSPRNG → 32 hex chars) with idempotent shim-backed persistence under deterministic db_key_v1: and db_salt_v1: namespaces. Update deleteDatabaseKey to remove both key and salt. Tests cover generation, idempotence, per-name separation, deletion, shim wiring, error-message safety, and __DEV__ behavior.
Database connection lifecycle and registry
app/lib/database/driver/connection.ts, app/lib/database/driver/__tests__/connection.test.ts
Implement DB name derivation from server URLs, iOS App Group directory resolution with fallback, module-scoped registry for handle reuse, and applyOpenPragmas enforcing strict order: PRAGMA key (raw-hex) → cipher_plaintext_header_sizecipher_saltbusy_timeoutjournal_mode = WAL. Verify encryption via sqlite_master query. Wrap raw SQLite with Drizzle using schema kind selection. Expose openServersDb, openServerDb, closeDb, and deleteDb APIs. Include test-only exports for registry inspection. Tests verify name derivation rules, PRAGMA ordering and values, registry caching and reuse, distinct handles per server, and safe rejection messages without key material exposure.
Live query observation hooks
app/lib/database/driver/observe.ts, app/lib/database/driver/__tests__/observe.test.ts
Add useTableQuery hook that subscribes to Expo SQLite change events, filters by database file path and subscribed table names, debounces re-queries (default 16ms), and reconciles rows by rowKey with structural sharing via equalFn. Add useRowObserve hook for single-row observation: subscribes to table/rowId events, loads on mount/rowId change, ignores mismatched database/table/rowId events, and guards against post-unmount updates. Tests validate debounce/coalescing, table and database filtering, reference reuse on content equality, reference creation on change, useRowObserve initial fetch and event-driven re-fetch, null-rowId contract, and cross-database event isolation.

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
Loading
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 }
Loading
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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • RocketChat/Rocket.Chat.ReactNative#7418: The keyService module here calls getOrCreateDatabaseKey to retrieve persisted encryption keys; this related PR introduces the native DatabaseKeyStore TurboModule that backs the keychain shim's persistent storage.

Suggested reviewers

  • OtavioStasiak
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: adding an expo-sqlite driver adapter, key service, and live-query hooks, which matches the core additions across connection.ts, keyService.ts, and observe.ts.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • NATIVE-1274: Request failed with status code 401

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2

🧹 Nitpick comments (2)
app/lib/database/driver/__tests__/keyService.test.ts (1)

53-62: ⚡ Quick win

The test title claims key difference, but the assertions don’t verify it.

With a constant randomKey mock, 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 value

Unused mockSubscription variable.

mockSubscription is defined but never used — each addDatabaseChangeListener call returns a fresh object with its own remove function (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 in beforeEach (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

📥 Commits

Reviewing files that changed from the base of the PR and between 90ceaf8 and 61cea36.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • android/gradle.properties
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/lib/database/driver/__tests__/observe.test.ts
  • app/lib/database/driver/connection.ts
  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/observe.ts
  • ios/Podfile.properties.json
  • package.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.json
  • package.json
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/__tests__/observe.test.ts
  • app/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.ts
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/__tests__/observe.test.ts
  • app/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 numbers

Use TypeScript with strict mode enabled

Files:

  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/__tests__/observe.test.ts
  • app/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Enforce ESLint rules from @rocket.chat/eslint-config with React, React Native, TypeScript, and Jest plugins

Files:

  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/__tests__/observe.test.ts
  • app/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.ts
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/__tests__/observe.test.ts
  • app/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 thread app/lib/database/driver/connection.ts Outdated
Comment thread app/lib/database/driver/keyService.ts Outdated
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.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
app/lib/database/driver/__tests__/keyService.test.ts (1)

115-127: ⚡ Quick win

Consider adding a test for concurrent first-write race condition.

The tests verify idempotency under sequential calls, but don't exercise concurrent calls to getOrCreateDatabaseKey or getOrCreateDatabaseSalt for the same dbName. 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

📥 Commits

Reviewing files that changed from the base of the PR and between e5e3091 and b9c1382.

📒 Files selected for processing (4)
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/lib/database/driver/connection.ts
  • app/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.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/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 numbers

Use TypeScript with strict mode enabled

Files:

  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/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.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/lib/database/driver/connection.ts
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Enforce ESLint rules from @rocket.chat/eslint-config with React, React Native, TypeScript, and Jest plugins

Files:

  • app/lib/database/driver/keyService.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/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.ts
  • app/lib/database/driver/__tests__/keyService.test.ts
  • app/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 getOrCreateDatabaseKey can race: both callers pass the existing !== null check, both generate different keys via randomKey, 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 when applyOpenPragmas fails remains unaddressed.

The past review correctly identified that if applyOpenPragmas throws (e.g., open-verify fails due to wrong key/salt, or file corruption), the sqlite handle from openDatabaseAsync is 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!

Comment thread app/lib/database/driver/keyService.ts Outdated

@diegolmello diegolmello left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Comment thread app/lib/database/driver/connection.ts Outdated

const [keyHex, saltHex] = await Promise.all([getOrCreateDatabaseKey(dbName), getOrCreateDatabaseSalt(dbName)]);

const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🔴 [high] TOCTOU race in openDb

Two concurrent callers both miss the registry cache, both call openDatabaseAsync, and the second _registry.set silently overwrites the first. The first handle is orphaned while still holding a file lock, which can cause SQLITE_BUSY or database is locked on the next open.

Fix: use an in-flight promise registry so concurrent calls coalesce onto one open:

const _inflight = new Map<string, Promise<DbHandle>>();

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 existing = _inflight.get(dbName);
  if (existing) return existing as Promise<DbHandle<K>>;

  const promise = _openDbImpl(dbName, kind).finally(() => _inflight.delete(dbName));
  _inflight.set(dbName, promise);
  return promise;
}

Comment thread app/lib/database/driver/connection.ts Outdated

const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY);

await applyOpenPragmas(sqlite, keyHex, saltHex);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🟡 [medium] Handle leak on PRAGMA failure

openDatabaseAsync succeeds (line 179), then applyOpenPragmas throws (line 181). The raw sqlite handle is never closed, so the file lock is held for the process lifetime. On repeated open attempts this accumulates leaked handles.

Fix: wrap the pragma sequence in a try/catch and close on failure before rethrowing:

const sqlite = await openDatabaseAsync(dbName, { enableChangeListener: true }, DB_DIRECTORY);
try {
  await applyOpenPragmas(sqlite, keyHex, saltHex);
} catch (err) {
  await sqlite.closeAsync().catch(() => {});
  throw err;
}

Comment thread app/lib/database/driver/keyService.ts Outdated
const k = storageKey(KEY_PREFIX, dbName);
const existing = await _shim.getItem(k);
if (existing !== null) {
return existing;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🟡 [medium] No hex validation on keychain read-back

Fresh-generated keys are guarded by !/^[0-9a-fA-F]{64}$/ (line 107), but the loaded path returns the stored value immediately (line 98) without the same check. A tampered keychain entry on a jailbroken device would be interpolated verbatim into PRAGMA key = "x'<value>'", enabling PRAGMA string injection.

Fix: apply the same regex guard before returning the loaded value:

const existing = await _shim.getItem(k);
if (existing !== null) {
  if (!/^[0-9a-fA-F]{64}$/.test(existing)) {
    throw new Error('stored key failed format validation; keychain may be tampered');
  }
  return existing;
}

Apply the same pattern in getOrCreateDatabaseSalt (32-hex guard).

Comment thread app/lib/database/driver/connection.ts Outdated
const sanitized = serverUrl
.replace(/\/+$/, '')
.replace(/(^\w+:|^)\/\//, '')
.replace(/\//g, '.');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🟡 [medium] deriveServerDbName produces colliding filenames

.replace(/\//g, '.') maps both https://foo/bar and https://foo.bar to foo.bar.db. Two different servers share one encrypted SQLite file and one keychain key — a data isolation bug.

Fix: use a separator that can't appear in a hostname. Replace / with _ (hostnames cannot contain _), leaving . as the host-segment delimiter only:

export function deriveServerDbName(serverUrl: string): string {
  const sanitized = serverUrl
    .replace(/\/+$/, '')
    .replace(/(^\w+:|^)\/\//, '')
    .replace(/\//g, '_');  // _ cannot appear in a hostname
  return `${sanitized}.db`;
}

Ensure the keychain storage-key derivation in keyService.ts uses the same function so the key and the file always agree.

if (!tableSet.has(event.tableName)) return;
// Debounce: coalesce all events from a single large transaction into one re-query
if (timerRef.current !== null) clearTimeout(timerRef.current);
timerRef.current = setTimeout(fetchAndReconcile, debounceMs);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🟢 [low] queryFn throw inside setTimeout is unhandled

fetchAndReconcile (which calls queryFnRef.current()) is scheduled via setTimeout. A throw inside it becomes an uncaught exception on the timer thread — React error boundaries cannot catch it, and it surfaces as an unhandled promise rejection or a native crash in some RN versions.

Fix: wrap the timer callback:

timerRef.current = setTimeout(() => {
  try {
    fetchAndReconcile();
  } catch (err) {
    // surface via error state so React boundaries can catch it
    setRows(() => { throw err; });
  }
}, debounceMs);

@diegolmello diegolmello left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

* Returns the same handle on repeated calls for the same name.
*/
async function openDb<K extends DbKind>(dbName: string, kind: K): Promise<DbHandle<K>> {
const cached = _registry.get(dbName);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

bug: high — concurrent-open race leaks a handle. The _registry.get check (here) and _registry.set (~line 189) are separated by several awaits (keychain x2, openDatabaseAsync, the PRAGMA round-trips). Two concurrent callers (e.g. two sagas opening the same server DB on cold boot) both see no cached handle, both run the full open, and the second set overwrites the first — leaving an orphaned open SQLCipher connection that is never closed. Track in-flight opens in a Map<string, Promise<DbHandle>> and return the pending promise on a second concurrent call.

Comment thread app/lib/database/driver/keyService.ts Outdated
export async function getOrCreateDatabaseKey(dbName: string): Promise<string> {
const k = storageKey(KEY_PREFIX, dbName);
const existing = await _shim.getItem(k);
if (existing !== null) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

bug: low — stored key returned without re-validation. The freshly-generated hex is regex-checked before storing, but on a cache hit existing is returned unconditionally and fed straight into PRAGMA key. A future shim bug (truncation/encoding) would surface as the misleading "key may be wrong or file corrupt" verify error. Re-run the same regex on existing; on failure throw a distinct "stored key corrupt" error (with no key material).

rowId: number | null | undefined,
fetchRow: FetchRowFn<T>
): T | null {
const [row, setRow] = useState<T | null>(() => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

bug: low — initial row fetched twice on mount. The useState lazy initializer runs fetchRow(rowId) synchronously, then the effect immediately calls setRow(fetchRowRef.current(rowId)) again before any change event. Drop the lazy initializer and default to null; the effect already handles the initial load.

Comment thread app/lib/database/driver/keyService.ts Outdated
* The key is 32 bytes (256-bit) from the platform CSPRNG, hex-encoded to 64 chars.
* It is never logged, never included in thrown errors, never sent to telemetry.
*/
export async function getOrCreateDatabaseKey(dbName: string): Promise<string> {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

slop: moderate — getOrCreateDatabaseKey / getOrCreateDatabaseSalt are near-duplicates. They differ only by prefix, byte count, expected hex length and one error string (~45 lines duplicated). Extract a private getOrCreate(prefix, dbName, bytes) helper; both public functions become one-liners.

Comment thread app/lib/database/driver/connection.ts Outdated
async function applyOpenPragmas(sqlite: SQLiteDatabase, keyHex: string, saltHex: string): Promise<void> {
// 1. Key MUST be first — before any schema read/write
// Raw-key form: the x'...' prefix tells SQLCipher to skip PBKDF2
await sqlite.execAsync(`PRAGMA key = "x'${keyHex}'";`);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

slop: moderate — five sequential execAsync PRAGMA calls. execAsync accepts a semicolon-separated multi-statement string; the four config pragmas (key through WAL) can be one call, with only the getFirstAsync verify separate. Fewer round-trips and it makes the "configure before any schema access" intent atomic and explicit.

Comment thread app/lib/database/driver/observe.ts Outdated
};
// deps are caller-controlled; tables/debounceMs treated as stable across renders
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [dbHandle, fetchAndReconcile, ...deps]);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

slop: moderate — ...deps spread blinds the hooks linter to tables. tables is closed over inside the effect but isn't in the literal deps array, so if a caller's table set changes without threading it through deps the subscription silently watches the stale set — and the eslint-disable hides it. Derive a stable tableKey from tables and put it in the deps array so re-subscription is automatic and lint-visible.

Comment thread app/lib/database/driver/observe.ts Outdated
return fetchRow(rowId);
});

const mounted = useRef(true);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

slop: nit — mounted ref is dead code. The listener is registered in an effect whose cleanup calls sub.remove(); in JS's single-threaded model the listener can't fire after removal, so the if (!mounted.current) return guard is unreachable. Remove the ref and its effect.

Comment thread app/lib/database/driver/keyService.ts Outdated
* After calling this, the database file is permanently inaccessible.
*/
export async function deleteDatabaseKey(dbName: string): Promise<void> {
await _shim.removeItem(storageKey(KEY_PREFIX, dbName));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

slop: nit — key and salt removed sequentially. The two removeItem calls are independent; Promise.all halves the round-trips.

- 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

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

♻️ Duplicate comments (1)
app/lib/database/driver/observe.ts (1)

140-142: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Unhandled exception in setTimeout callback remains unaddressed.

If fetchAndReconcile (which calls queryFnRef.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

📥 Commits

Reviewing files that changed from the base of the PR and between b9c1382 and 8b6befb.

📒 Files selected for processing (4)
  • app/lib/database/driver/__tests__/connection.test.ts
  • app/lib/database/driver/connection.ts
  • app/lib/database/driver/keyService.ts
  • app/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.ts
  • app/lib/database/driver/connection.ts
  • app/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 numbers

Use TypeScript with strict mode enabled

Files:

  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/connection.ts
  • app/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.ts
  • app/lib/database/driver/connection.ts
  • app/lib/database/driver/keyService.ts
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Enforce ESLint rules from @rocket.chat/eslint-config with React, React Native, TypeScript, and Jest plugins

Files:

  • app/lib/database/driver/observe.ts
  • app/lib/database/driver/connection.ts
  • app/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.ts
  • app/lib/database/driver/connection.ts
  • app/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> {

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant