feat: port client data into the encrypted database at first boot#7408
Open
diegolmello wants to merge 2 commits into
Open
feat: port client data into the encrypted database at first boot#7408diegolmello wants to merge 2 commits into
diegolmello wants to merge 2 commits into
Conversation
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)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
WalkthroughAdds a complete one-shot legacy WatermelonDB-to-encrypted-SQLite migration system with MMKV-backed phase state, platform-aware legacy DB discovery and reading, column-drift-safe row porting helpers, and a phase-driven orchestrator that resumes from persisted state on every boot. The ChangesLegacy DB Migration System
CI Workflow Update
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 early
else detect: legacy servers DB missing
orchestrator->>state: markSkipped()
orchestrator-->>restore: return
else detect: legacy servers DB present
orchestrator->>state: setPhase(porting_servers)
orchestrator->>legacyReader: openLegacy(servers DB)
orchestrator->>legacyReader: readLegacyUsers / readLegacyServerLockFields / readLegacyServersHistory
orchestrator->>port: portUsers, portServerLockFields, portServersHistory
orchestrator->>state: setPhase(porting_active)
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
orchestrator->>orchestrator: secureDelete(per-server DB + WAL/SHM)
orchestrator->>state: markServer(url, wiped)
end
orchestrator->>orchestrator: secureDelete(servers DB)
orchestrator->>state: markDone()
orchestrator-->>restore: return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (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!
Comment on lines
+125
to
+130
| export function enumeratePresentServerDbs(serverUrls: string[]): string[] { | ||
| return serverUrls.filter(url => { | ||
| const dbName = deriveServerDbName(url); | ||
| return legacyFileExists(dbName); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Return content mismatches the function contract
enumeratePresentServerDbs claims to return DB names, but it currently returns filtered server URLs. Any caller using this output for file operations will use the wrong identifiers.
Suggested fix
export function enumeratePresentServerDbs(serverUrls: string[]): string[] {
- return serverUrls.filter(url => {
- const dbName = deriveServerDbName(url);
- return legacyFileExists(dbName);
- });
+ return serverUrls.map(url => deriveServerDbName(url)).filter(dbName => legacyFileExists(dbName));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function enumeratePresentServerDbs(serverUrls: string[]): string[] { | |
| return serverUrls.filter(url => { | |
| const dbName = deriveServerDbName(url); | |
| return legacyFileExists(dbName); | |
| }); | |
| } | |
| export function enumeratePresentServerDbs(serverUrls: string[]): string[] { | |
| return serverUrls.map(url => deriveServerDbName(url)).filter(dbName => legacyFileExists(dbName)); | |
| } |
🤖 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 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.
Comment on lines
+147
to
+156
| // Record servers as pending in state so porting_active can resume per-server | ||
| setPhase('porting_active'); | ||
| state = readState(); | ||
| // Initialise per-server entries if not already set (fresh run from detect) | ||
| if (state && Object.keys(state.servers).length === 0) { | ||
| for (const url of serverUrls) { | ||
| markServer(url, 'pending'); | ||
| } | ||
| state = readState(); | ||
| } |
Contributor
There was a problem hiding this comment.
Phase is advanced before server workset is durable
setPhase('porting_active') is called before persisting the full servers map. A crash in this window can resume with empty/partial state.servers, skip active-server porting, and still finish migration as done.
Suggested fix (persist workset first, then flip phase)
// Capture server URLs before closing the legacy DB; we need them for porting_active
const serverUrls = await readLegacyServerUrls(legacyDb);
- // Record servers as pending in state so porting_active can resume per-server
- setPhase('porting_active');
- state = readState();
- // Initialise per-server entries if not already set (fresh run from detect)
- if (state && Object.keys(state.servers).length === 0) {
- for (const url of serverUrls) {
- markServer(url, 'pending');
- }
- state = readState();
- }
+ // Persist per-server workset first; resume stays in porting_servers until this is durable
+ for (const url of serverUrls) {
+ const current = readState();
+ if (!current?.servers[url]) {
+ markServer(url, 'pending');
+ }
+ }
+
+ setPhase('porting_active');
+ state = readState();🤖 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/orchestrator.ts` around lines 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').
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
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
|
iOS Build Available Rocket.Chat 4.74.0.109131 |
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
Bug Fixes
Tests