feat: port client data into the encrypted database at first boot - #7408
Merged
diegolmello merged 37 commits intoJun 23, 2026
Merged
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.
…y store (NATIVE-1276) - ios/Libraries/DatabaseKeyStore.swift + .m: new RCTBridgeModule backed by kSecClassGenericPassword with kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, kSecAttrSynchronizable=false, and the full team-prefixed access group S6UPZG7ZR3.chat.rocket.reactnative; exposes getItem/setItem/removeItem to JS and a static read() helper callable from Database.swift in the NSE - ios/Shared/RocketChat/Database.swift: rewrites plain sqlite3_open to open SQLCipher-encrypted DBs; derives clean filenames via deriveServerDbName() (mirrors JS connection.ts); reads keys from DatabaseKeyStore using the JS KEY_PREFIX "db_key_v1:<dbName>"; applies PRAGMA key (raw x'...' form) then busy_timeout=500 then open-verify; ARC hazard fixed (handle private, never exposed to callers) - ios/Podfile: adds pod 'SQLCipher' ~> 4.7.0 to abstract_target defaults, adds SQLCipher to $static_framework list, and adds the two post_install xcconfig fixups from the spike (strip SDK sqlite3 defines that break Swift module maps, re-inject LIBRARY_SEARCH_PATHS suppressed by static_framework override); Dir.glob rooted with __dir__ for worktree/CI safety - android/app/src/.../storage/DatabaseKeyStore.java: new native module with AES-GCM AndroidKeyStore wrapping; SharedPreferences file "RCDatabaseKeyStore"; static getItemInternal/setItemInternal callable from Encryption.java without ReactApplicationContext - android/app/src/.../storage/DatabaseKeyStorePackage.java: ReactPackage wrapper - android/app/src/.../MainApplication.kt: registers DatabaseKeyStorePackage - android/app/src/.../notification/Encryption.java: replaces WMDatabase.getInstance with net.zetetic:sqlcipher-android SQLiteDatabase; uses raw-key string form "x'<64 hex>'" (never byte[] overload); derives clean .db filename via deriveDbName() matching JS convention; reads key from DatabaseKeyStore - android/app/build.gradle: adds net.zetetic:sqlcipher-android:4.7.0@aar - app/lib/database/driver/keyStore.ts: thin JS shim wrapping NativeModules.DatabaseKeyStore into an IKeychainShim; exports installNativeKeychainShim() to wire into keyService - app/lib/database/driver/__tests__/keyStore.test.ts: Jest tests for the shim delegation and missing-module error path
- ios/RocketChatRN.xcodeproj: register DatabaseKeyStore.swift in both the Rocket.Chat app and NotificationService Sources phases (Database.swift in the NSE references it) and the .m bridge in the app target; without this the files never compile - ios bridging header: import <React/RCTBridgeModule.h> explicitly rather than relying on RNCallKeep's transitive include - Encryption.java: add PRAGMA busy_timeout = 500 after open to match the JS driver and iOS reader (avoids SQLITE_BUSY when the notification path reads while the app holds a WAL lock); drop two unused SQLCipher imports
The app runs the New Architecture (bridgeless), so the key store must be a
TurboModule with a codegen-style spec, not the legacy RCT_EXTERN_MODULE /
NativeModules path it used before.
iOS: replace the .m with a .mm module class (DatabaseKeyStoreModule) that
implements the spec and returns its JSI instance from getTurboModule. The
Swift Keychain helper stays a separate class (DatabaseKeyStore) so the JS
registration name does not collide with an existing ObjC class — required by
the bridgeless NSClassFromString fallback. getItem resolves explicit JS null
on a miss (nil bridges to undefined and breaks the Promise<string|null>
contract and the !== null check in getOrCreateDatabaseKey).
Android: add an abstract spec (NativeDatabaseKeyStoreSpec) plus a concrete
module and a TurboReactPackage that registers it with isTurboModule=true.
The AndroidKeyStore AES-GCM crypto is unchanged; only the registration name
and class layout move. Encryption.java (NSE reader) now calls the static
helpers on the concrete module class.
JS resolves the module via TurboModuleRegistry.get('DatabaseKeyStoreModule').
The iOS open-verify only prepared the probe statement; sqlite3_prepare_v2 parses SQL without decrypting a page, so a wrong key surfaced later at first real use instead of at open. Step the statement (SELECT count(*) FROM sqlite_master) so SQLCipher decrypts and the wrong-key case is caught at open time. On Android, the NSE subscription-read cursor leaked if getColumnIndex or getString threw between open and close. Wrap the cursor body in try/finally so it always closes.
getOrCreateDatabaseKey treats a null getItem result as "no key exists" and generates a replacement. Both platforms previously collapsed every read/decrypt failure into that same null, so a Keychain access error (iOS) or an invalidated Keystore master key / corrupt blob (Android) would silently mint a new key over an existing encrypted database and orphan its data permanently. Distinguish a genuine not-found from a read failure on the JS-facing path: getItem now resolves null only for a true miss and rejects on any other failure, so the caller aborts the open instead of regenerating. iOS: read gains an NSError out-param (errSecItemNotFound -> nil/no error; any other status -> nil + error). The .mm rejects when the error is set; the NSE reader in Database.swift fails the open on error without regenerating. Android: getItemInternal returns null only when the SharedPreferences blob is absent and throws otherwise (missing alias for an existing blob, decrypt failure). getItem rejects on those throws; the NSE reader in Encryption.java catches and skips the room rather than regenerating.
Introduce a temporary compatibility layer that exposes the WatermelonDB
public API (Database, Collection, Query, Model, decorators, Q clauses,
sanitizedRaw, appSchema/tableSchema) on top of the synchronous Drizzle
expo-sqlite driver. A fetched Drizzle row is column-keyed and therefore
structurally identical to WatermelonDB's _raw, so the facade Model wraps
the row directly and field getters read _raw[column].
This lets the ~80 existing @nozbe/watermelondb import sites be re-pointed
onto the facade without touching call-site logic, ahead of removing the
WatermelonDB package entirely in a later step.
Notable deviations from WatermelonDB, required by Drizzle:
- sanitizedRaw emits { id, ...coercedColumns } only — no _status/_changed,
which have no Drizzle columns; a random 16-char id is generated when absent.
- write/batch wrap the synchronous Drizzle transaction in a Promise surface;
a single-writer WriterQueue serializes writers.
- observe()/observeWithColumns() bridge expo-sqlite's change listener to RxJS,
filtering by database file + table, debouncing 16ms, and structurally
sharing unchanged row references.
Verified: tsc, eslint, and 32 unit tests pass on a fresh empty database.
Comparison operators (eq, notEq, gt, gte, lt, lte, like, notLike, oneOf) take
only the right-hand value and return a Comparison; Q.where(column, valueOrComparison)
wraps it, treating a raw value as an implicit eq. This matches every call site
(e.g. Q.where('ts', Q.gt(date)), Q.where('id', Q.oneOf(ids))) ahead of the cutover.
Null comparisons now lower to IS NULL / IS NOT NULL rather than = NULL, which is
never true in SQL — required for the many Q.where(col, null) and Q.notEq(null) sites.
Replace @nozbe/watermelondb imports across the app with the WatermelonDB-shaped facade over the Drizzle/expo-sqlite driver, so the app runs on the encrypted SQLCipher database while keeping call sites unchanged. Add the table/model maps the facade needs to construct each Database, wire the servers and app schemas through it, and add an ESLint rule banning new direct watermelondb imports. WatermelonDB stays installed only for the legacy migration reader; no JS runtime path uses it anymore.
Add a wipe-and-restore migration that runs once at cold boot, before any server data is read or re-auth is evaluated, while the bootsplash is still up. It ports users, server lock fields, server history, pending messages, drafts, uploads and frequently-used emojis from the legacy plaintext WatermelonDB files into the new SQLCipher database, then deletes the legacy files and their WAL/SHM sidecars. The orchestrator is a crash-safe resumable state machine: each phase transition is recorded in MMKV before the destructive step that follows, so a crash resumes from the last durable phase. A done marker fast-paths every subsequent boot. Wire it into the init saga's restore: open the servers database first so the native key store is installed and the migration ports through the real key, then run the migration. A migration failure is logged and never blocks boot.
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughAdds a complete one-shot legacy WatermelonDB-to-encrypted-SQLite migration system. It introduces an MMKV-backed phase state machine, a platform-aware legacy SQLite reader, column-drift-safe row port helpers, and a phase-driven orchestrator. The ChangesLegacy DB Migration System
Sequence Diagram(s)sequenceDiagram
participant restore as restore (init.js)
participant orchestrator as runMigrationIfNeeded
participant state as state.ts
participant legacyReader as legacyReader.ts
participant port as port.ts
restore->>orchestrator: runMigrationIfNeeded()
orchestrator->>state: isMigrationDone()
alt already done or skipped
orchestrator-->>restore: return (fast path)
else detect phase
orchestrator->>legacyReader: legacyFileExists(LEGACY_SERVERS_DB_NAME)
alt legacy DB absent
orchestrator->>state: markSkipped()
orchestrator-->>restore: return
else legacy DB present
orchestrator->>state: setPhase(porting_servers)
orchestrator->>legacyReader: openLegacy(servers DB)
orchestrator->>port: portUsers, portServerLockFields, portServersHistory
orchestrator->>state: startPortingActive(serverUrls)
loop each server URL
orchestrator->>legacyReader: openLegacy(per-server DB)
orchestrator->>port: portPendingMessages, portSubscriptionDrafts, portThreadDrafts, portUploads, portFrequentlyUsedEmojis
orchestrator->>state: markServer(url, ported)
end
orchestrator->>state: setPhase(wiping)
loop each server URL
orchestrator->>orchestrator: secureDelete(per-server DB + WAL/SHM)
orchestrator->>state: markServer(url, wiped)
end
orchestrator->>orchestrator: secureDelete(servers DB + WAL/SHM)
orchestrator->>state: markDone()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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. 🔧 OSV Scanner (2.4.0)Error: ENOENT: no such file or directory, scandir '/inmem/1292/nsjail-21a1b8fe-0b02-4431-912b-f0437a9d7c38/merged/node_modules/eslint-plugin-react-native-globals' Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (3)
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 (1)
app/lib/database/migration/__tests__/migration.test.ts (1)
101-140: ⚡ Quick winAdd explicit return types (and typed mock interfaces) for test helpers.
Line 101, Line 118, Line 215, and Line 226 rely on inferred return types. In strict TypeScript suites, this makes mock contract drift easier to miss.
Proposed typed patch
+interface MockSqlite { + runAsync: (sql: string, args?: unknown[]) => Promise<void>; + execAsync: () => Promise<void>; + getFirstAsync: () => Promise<{ count: number }>; + getAllAsync: (sql: string) => Promise<unknown[]>; + closeAsync: () => Promise<void>; +} + -function mockMakeNewSqlite(dbName: string) { +function mockMakeNewSqlite(dbName: string): MockSqlite { if (!mockNewDbWrites[dbName]) mockNewDbWrites[dbName] = []; return { runAsync: jest.fn(async (sql: string, args?: unknown[]) => { mockNewDbWrites[dbName].push({ sql, args: args ?? [] }); }), execAsync: jest.fn(async () => {}), getFirstAsync: jest.fn(async () => ({ count: 0 })), getAllAsync: jest.fn(async (sql: string) => { const tbl = sql.match(/PRAGMA\s+table_info\((\w+)\)/i)?.[1]; if (tbl) return (mockNewDbColumns[tbl] ?? []).map(name => ({ name })); return []; }), closeAsync: jest.fn(async () => {}) }; } -function mockMakeLegacySqlite(dbName: string) { +function mockMakeLegacySqlite(dbName: string): MockSqlite { return { runAsync: jest.fn(async () => {}), execAsync: jest.fn(async () => {}), getFirstAsync: jest.fn(async () => ({ count: 0 })), getAllAsync: jest.fn(async (sql: string) => { const tbl = sql.match(/FROM\s+(\w+)/i)?.[1]; if (!tbl) return []; const all = (mockLegacyRows[dbName]?.[tbl] ?? []) as Record<string, unknown>[]; @@ closeAsync: jest.fn(async () => {}) }; } @@ -function clearAll() { +function clearAll(): void { mockMmkvStore.clear(); @@ } -function seedLegacyDb(dbName: string, table: string, rows: Record<string, unknown>[]) { +function seedLegacyDb(dbName: string, table: string, rows: Record<string, unknown>[]): void { if (!mockLegacyRows[dbName]) mockLegacyRows[dbName] = {}; mockLegacyRows[dbName][table] = rows; }As per coding guidelines, “Use TypeScript for type safety; add explicit type annotations to function parameters and return types” and “Prefer interfaces over type aliases for defining object shapes in TypeScript.”
Also applies to: 215-229
🤖 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/migration/__tests__/migration.test.ts` around lines 101 - 140, Add explicit return type annotations to the test helper functions mockMakeNewSqlite and mockMakeLegacySqlite (and any other similar functions in the 215-229 range) to prevent type inference issues. First, create typed interfaces that define the shape of the mock database objects being returned, then apply these interfaces as explicit return types to each helper function. This ensures the mock contract is clearly defined and makes any drift in the actual implementation immediately obvious during type checking.Source: Coding guidelines
🤖 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/migration/legacyReader.ts`:
- Around line 125-130: The function enumeratePresentServerDbs is returning
filtered server URLs instead of the database names it derives. Modify the
function to return the actual database names that are derived using
deriveServerDbName and validated with legacyFileExists, rather than returning
the original serverUrls array. Use map instead of filter or adjust the filter
approach to capture and return the dbName values that pass the legacyFileExists
check.
In `@app/lib/database/migration/orchestrator.ts`:
- Around line 147-156: The phase is being advanced to 'porting_active' before
the server workset is persisted, creating a crash-window vulnerability. Move the
setPhase('porting_active') call to after the for loop that marks all servers as
pending and after the final readState() call, ensuring the complete servers map
is durable before advancing the phase. The correct order should be: initialize
servers with markServer calls, read the updated state, then call
setPhase('porting_active').
---
Nitpick comments:
In `@app/lib/database/migration/__tests__/migration.test.ts`:
- Around line 101-140: Add explicit return type annotations to the test helper
functions mockMakeNewSqlite and mockMakeLegacySqlite (and any other similar
functions in the 215-229 range) to prevent type inference issues. First, create
typed interfaces that define the shape of the mock database objects being
returned, then apply these interfaces as explicit return types to each helper
function. This ensures the mock contract is clearly defined and makes any drift
in the actual implementation immediately obvious during type checking.
🪄 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: 2d04edb8-4cbc-480d-b2ee-dfbf4a160789
📒 Files selected for processing (7)
app/lib/database/migration/__tests__/legacyReader.android.test.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/legacyReader.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/port.tsapp/lib/database/migration/state.tsapp/sagas/init.js
📜 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/migration/__tests__/legacyReader.android.test.tsapp/sagas/init.jsapp/lib/database/migration/state.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/port.tsapp/lib/database/migration/legacyReader.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/migration/__tests__/legacyReader.android.test.tsapp/lib/database/migration/state.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/port.tsapp/lib/database/migration/legacyReader.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/migration/__tests__/legacyReader.android.test.tsapp/sagas/init.jsapp/lib/database/migration/state.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/port.tsapp/lib/database/migration/legacyReader.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/migration/__tests__/legacyReader.android.test.tsapp/sagas/init.jsapp/lib/database/migration/state.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/port.tsapp/lib/database/migration/legacyReader.ts
🧠 Learnings (2)
📚 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/migration/__tests__/legacyReader.android.test.tsapp/lib/database/migration/state.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/port.tsapp/lib/database/migration/legacyReader.ts
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.
Applied to files:
app/sagas/init.js
🔇 Additional comments (1)
app/lib/database/migration/__tests__/legacyReader.android.test.ts (1)
1-43: LGTM!
The pull_request trigger's branches filter is matched against the base ref. The '*' glob does not match refs containing '/', so PRs based on a feature branch (e.g. feat/native-1277-facade-cutover) never ran the build pipeline. '**' matches across slashes, restoring builds for stacked PRs.
diegolmello
had a problem deploying
to
approve_e2e_testing
June 17, 2026 21:35 — with 
GitHub Actions
Error
GitHub Actions
Error
diegolmello
had a problem deploying
to
android_build
June 17, 2026 21:40 — with
GitHub Actions
Error
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.74.0.109131 |
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.
diegolmello
commented
Jun 19, 2026
diegolmello
left a comment
diegolmello
left a comment
Member
Author
There was a problem hiding this comment.
Review: wipe-and-restore migration
Boot trigger verified fixed 🎉 — initServers now runs as the first yield call inside the takeLatest(APP.INIT, restore) handler, so the previously-observed dropped-APP.INIT problem (root-saga prefix delaying the listener registration) does not occur here; migration fires on every cold boot and the MMKV fast-path keeps subsequent boots O(1).
The ported set is correctly scoped and the wipe ordering is right. Main concern is the non-atomic phase transition (inline) — a crash in that window silently destroys the local-only set it is meant to preserve.
Reviewed by an automated pass; treat inline comments as suggestions, not blockers.
diegolmello
commented
Jun 19, 2026
diegolmello
left a comment
diegolmello
left a comment
Member
Author
There was a problem hiding this comment.
Structural review (NATIVE-1278). The phase state machine is cleanly typed and the boot trigger is correctly wired (moving database.initServers inside restore unblocks takeLatest(APP.INIT)). No file over ~230 lines. Two structural bugs are inline — fix the phase-advance ordering first; it can permanently lose per-server data on a crash. The rest is cleanup.
…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
… into feat/native-1276-native-readers
- rid lookup: Encryption.java queried by wrong column (id == ?) instead of
WHERE rid = ? LIMIT 1; now matches iOS Database.swift and Drizzle schema
- PRAGMA parity: readRoom now applies cipher_plaintext_header_size = 32 and
cipher_salt (from db_salt_v1: keychain entry) to match the JS driver's
post-merge open sequence; without these the reader cannot open the DB
- static lib load: System.loadLibrary("sqlcipher") moved from readRoom into
a static {} initializer so it runs once per process, not per push
- AndroidKeyStore alias leak: removeItemInternal now deletes the keystore
alias after clearing the SharedPreferences blob; previously every logout
orphaned an alias and a later setItem would reuse stale key material
- removeItem rejects on failure instead of silently resolving null
- hasConstants: DatabaseKeyStoreTurboPackage passes false — module has no
getConstants() override
- dead @ReactMethod: removed three annotations and the ReactMethod import from
NativeDatabaseKeyStoreSpec; TurboModule dispatch is codegen, not reflection
- spurious import: removed #import <SSLPinning/SSLPinning.h> from
DatabaseKeyStore.mm (copy-paste; no SSL symbols used)
- access group: made DatabaseKeyStore.accessGroup internal (was private) so it
is the single source of truth for both the keychain module and Database.swift;
added team-id coupling comment
- idempotent shim: installNativeKeychainShim is now a no-op after the first
install (via a module-level _installed flag in keyStore.ts); installKeychainShim
in keyService stays unrestricted for test isolation
Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
…S driver - Android deriveDbName: interior '/' → '_' (was '.'); corrects storage-key derivation for subpath server URLs (db_key_v1:, db_salt_v1:) - iOS deriveServerDbName: interior '/' → '_' (was '.'); same parity fix plus adjacent comment and init-docstring updated to match - iOS open: add cipher_plaintext_header_size = 32 and cipher_salt from keychain key db_salt_v1:<dbName> after PRAGMA key, before busy_timeout; fail-closed if salt is missing — mirrors Android Encryption.java readRoom Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
… into feat/native-1277-facade-cutover # Conflicts: # app/lib/database/driver/connection.ts
- children: query through registered collection instead of constructing bare Collection(Model)
- _pendingOp: clear after transaction commits, not inside the loop (prevents clearing on rollback)
- observeRow: add 16ms debounce matching observeTable
- _fetchSync id check: presence (=== undefined) not truthiness, so find('') rejects rather than full-scans
- writer enqueue: catch sync throws from fn so they reject the caller and don't stall the queue
- sameByColumns: diff by id map instead of array index so reordering is not a false positive
- Q.on: throw instead of silently returning undefined
- schema setRawCoerced: drop dead `|| 0` inside isValidNumber branch
- Collection.prepareCreate: tag _pendingOp after fn() to match Model.prepareCreate order
- updateMessages.ts: merge two consecutive imports from database/facade into one
- Database.ts: type db via SyncDb alias (BaseSQLiteDatabase<'sync',...>) to drop all (db as any) casts; resolve PK column via getTableColumns instead of unsafe cast
- Model: remove _jsonDecoratorCache (declared but never read)
Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
- Restore SQLite/ subdir in resolveDbDirectory() (lost in merge-forward from #7398 which took incoming and reverted this branch's isolation): new encrypted DBs must not land at the App Group root where legacy plaintext WatermelonDB files live — openServersDb() would open the legacy default.db, fail PRAGMA key, and crash every existing user on upgrade - Drop dead _relationCache field from Model (never read; @relation caches under _rel_* keys, not here) - Correct observeRow JSDoc: listener re-fetches on any table change, never consults event.rowId
- Atomic porting_active transition: replace separate setPhase + per-server markServer loop with startPortingActive(), which writes phase and all server URLs as 'pending' in a single MMKV write — eliminates the crash window where phase advanced before servers were populated - readState schema validation: reject entries with unknown/missing schema or phase fields, returning null so a corrupt entry restarts from detect rather than silently misrouting - deriveLegacyServerDbName: rename from deriveServerDbName to avoid silent confusion with connection.deriveServerDbName (same signature, different output — legacy suffix vs. new DB single .db) - portDraftColumn: factor the shared INSERT OR IGNORE + UPDATE body out of portSubscriptionDrafts and portThreadDrafts; keep both exported names - Remove dead enumeratePresentServerDbs (exported but never called) - Top-import File from expo-file-system, replacing lazy require + eslint-disable - Extract LegacyServerLockFields named type (inlined twice) - Unify detect block on state?.phase (drop local phase variable) - Update keyService mock to include getOrCreateDatabaseSalt (added by merge) - Tests: cover startPortingActive atomic write; update test imports and mock Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
diegolmello
had a problem deploying
to
approve_e2e_testing
June 22, 2026 15:25 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/database/migration/state.ts (1)
83-84: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse an enum-backed phase set to avoid drift in validation logic.
Line 83 hardcodes phase strings again for
KNOWN, which can diverge from transition writers over time. Centralizing these values in an enum (and deriving the validator set from it) keeps the state machine contract single-sourced.As per coding guidelines,
**/*.{ts,tsx}should use enums for sets of related constants instead of magic strings.🤖 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/migration/state.ts` around lines 83 - 84, The KNOWN array hardcodes phase strings separately from the MigrationPhase type definition, creating a maintenance risk. Convert MigrationPhase from a type union to an enum (if not already), then derive the KNOWN validator set from the enum keys or values instead of hardcoding the literal strings. This ensures the phase values used in validation logic stay synchronized with the phase definitions across the codebase.Source: Coding guidelines
app/lib/database/migration/legacyReader.ts (1)
164-170: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer an interface for
LegacyServerLockFieldsobject shape.Line 164 defines an object contract with a type alias; switch to
interfaceto match repository TS conventions and keep shape contracts consistent across the migration layer.Proposed change
-type LegacyServerLockFields = { +interface LegacyServerLockFields { id: string; auto_lock: number | null; auto_lock_time: number | null; last_local_authenticated_session: number | null; biometry: number | null; -}; +}As per coding guidelines,
**/*.{ts,tsx}should prefer interfaces over type aliases for defining object shapes in TypeScript.🤖 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/migration/legacyReader.ts` around lines 164 - 170, The LegacyServerLockFields is currently defined as a type alias but should be converted to an interface to align with repository conventions for defining object shapes. Replace the type keyword with interface keyword for LegacyServerLockFields and adjust the syntax accordingly by removing the equals sign and placing the object properties directly in the interface body. This maintains consistency with TypeScript best practices used throughout the migration layer.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@app/lib/database/migration/legacyReader.ts`:
- Around line 164-170: The LegacyServerLockFields is currently defined as a type
alias but should be converted to an interface to align with repository
conventions for defining object shapes. Replace the type keyword with interface
keyword for LegacyServerLockFields and adjust the syntax accordingly by removing
the equals sign and placing the object properties directly in the interface
body. This maintains consistency with TypeScript best practices used throughout
the migration layer.
In `@app/lib/database/migration/state.ts`:
- Around line 83-84: The KNOWN array hardcodes phase strings separately from the
MigrationPhase type definition, creating a maintenance risk. Convert
MigrationPhase from a type union to an enum (if not already), then derive the
KNOWN validator set from the enum keys or values instead of hardcoding the
literal strings. This ensures the phase values used in validation logic stay
synchronized with the phase definitions across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a0e3396-7e0c-4583-93db-a95a66a6422b
📒 Files selected for processing (6)
app/lib/database/migration/__tests__/legacyReader.android.test.tsapp/lib/database/migration/__tests__/migration.test.tsapp/lib/database/migration/legacyReader.tsapp/lib/database/migration/orchestrator.tsapp/lib/database/migration/port.tsapp/lib/database/migration/state.ts
✅ Files skipped from review due to trivial changes (1)
- app/lib/database/migration/tests/legacyReader.android.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/lib/database/migration/orchestrator.ts
- app/lib/database/migration/tests/migration.test.ts
- app/lib/database/migration/port.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 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/migration/state.tsapp/lib/database/migration/legacyReader.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/migration/state.tsapp/lib/database/migration/legacyReader.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/migration/state.tsapp/lib/database/migration/legacyReader.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/migration/state.tsapp/lib/database/migration/legacyReader.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/migration/state.tsapp/lib/database/migration/legacyReader.ts
🔇 Additional comments (2)
app/lib/database/migration/state.ts (1)
126-137: LGTM!app/lib/database/migration/legacyReader.ts (1)
22-22: LGTM!Also applies to: 124-130
…tion' into feat/native-1276-native-readers # Conflicts: # android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreModule.java # android/app/src/main/java/chat/rocket/reactnative/storage/DatabaseKeyStoreTurboPackage.java # android/app/src/main/java/chat/rocket/reactnative/storage/NativeDatabaseKeyStoreSpec.java # app/lib/native/NativeDatabaseKeyStore.ts # ios/Libraries/DatabaseKeyStore.mm # ios/Libraries/DatabaseKeyStore.swift
… into feat/native-1277-facade-cutover # Conflicts: # app/lib/hooks/useFrequentlyUsedEmoji.ts # app/lib/methods/emojis.ts # app/lib/methods/search.ts
getSubscriptionSearchClause builds its OR clause with the facade Q so it returns the facade clause type query() expects; drops the WatermelonDB Q import and the test's WMDB-shape accessors. Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
Reorders/merges imports the nested-worktree eslint could not check locally: facade Database.ts duplicate sqlite-core import merged, decorators.ts type-only import, and import-group spacing in emojis.ts/utils.ts/utils.test.ts. Claude-Session: https://claude.ai/code/session_01TE9VsFTeXsXc8ssqeR8MJ7
… into feat/native-1278-migration
diegolmello
had a problem deploying
to
approve_e2e_testing
June 23, 2026 01:28 — with
GitHub Actions
Failure
GitHub Actions
Failure
diegolmello
had a problem deploying
to
android_build
June 23, 2026 01:32 — with
GitHub Actions
Failure
GitHub Actions
Failure
Base automatically changed from
feat/native-1277-facade-cutover
to
feat/native-1272-sqlcipher-migration
June 23, 2026 14:04
diegolmello
merged commit Jun 23, 2026
1adf0dc
into
feat/native-1272-sqlcipher-migration
6 of 9 checks passed
This was referenced Jun 23, 2026
10 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
Adds a wipe-and-restore migration that runs once at cold boot, before any server data is read or re-auth is evaluated, while the bootsplash is still up. It ports the client-owned data — users, server lock fields, server history, pending messages, subscription and thread drafts, uploads (only those whose file still exists), and frequently-used emojis — from the legacy plaintext WatermelonDB files into the new SQLCipher database, then deletes the legacy files and their WAL/SHM sidecars. Everything else is dropped and resynced from the server.
The orchestrator is a crash-safe resumable state machine: each phase transition is recorded in MMKV before the destructive step that follows, so a crash or kill at any step resumes from the last durable phase. A done marker fast-paths every subsequent boot.
Boot wiring lives in the init saga's
restore: the servers database opens first (installing the native key store so the migration ports through the real device key), then the migration runs. A migration failure is logged and never blocks boot or logs the user out.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1278
How to test or reproduce
.db/.db.dbfixtures), cold-boot the app.-wal/-shmsidecars are deleted, the app proceeds past splash, and the session is preserved (no logout).TZ=UTC pnpm test— migration, legacy-reader and facade/driver suites pass.Verified end-to-end on an Android emulator from a fresh build: full detect → port → wipe → done, both legacy fixtures removed, idempotent on relaunch.
Types of changes
Checklist
Further comments
Stacked on
feat/native-1277-facade-cutover(#7407); review/merge that first. Part of the WatermelonDB → encrypted SQLite cutover (NATIVE-1272).Summary by CodeRabbit
New Features
Tests