fix(db): move persistMessage lookups and prepares inside the writer lock - #7551
fix(db): move persistMessage lookups and prepares inside the writer lock#7551OtavioStasiak wants to merge 1 commit into
Conversation
Walkthrough
ChangesMedia persistence transaction
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/lib/methods/handleMediaDownload.ts (1)
225-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the exported result type.
Declare
Promise<void>onpersistMessage. This keeps the exported API contract explicit.Proposed change
-export const persistMessage = async (messageId: string, uri: string, encryption: boolean, downloadUrl: string) => { +export const persistMessage = async ( + messageId: string, + uri: string, + encryption: boolean, + downloadUrl: string +): Promise<void> => {As per coding guidelines, add explicit type annotations to function parameters and return types.
🤖 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/methods/handleMediaDownload.ts` at line 225, Update the exported persistMessage function signature to explicitly declare a Promise<void> return type, while preserving its existing parameter annotations and implementation behavior.Source: Coding guidelines
app/lib/methods/handleMediaDownload.test.ts (1)
7-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the test doubles.
mockDbBatchandmakeRecorduseany.makeRecordanddeferredalso infer their return types. Define minimalPreparedRecordandDeferredinterfaces, then annotate these helper contracts.This keeps the concurrency test checked when the mocked WatermelonDB record shape changes.
As per coding guidelines, use TypeScript for type safety, add explicit type annotations to function parameters and return types, and prefer interfaces for object shapes.
Also applies to: 158-180
🤖 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/methods/handleMediaDownload.test.ts` around lines 7 - 29, Replace the any-based test doubles with minimal PreparedRecord and Deferred interfaces, then update mockDbBatch, makeRecord, and deferred with explicit parameter and return-type annotations. Ensure the mocked database batch and record helpers use these interfaces so changes to the WatermelonDB record shape remain type-checked.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/methods/handleMediaDownload.ts`:
- Around line 226-230: Update persistMessage and the getMessageById,
getThreadById, and getThreadMessageById lookup flow to use the captured db
instance inside db.write instead of database.active. Add a regression test that
switches the active database while the writer is queued and verifies records are
read from the captured database.
---
Nitpick comments:
In `@app/lib/methods/handleMediaDownload.test.ts`:
- Around line 7-29: Replace the any-based test doubles with minimal
PreparedRecord and Deferred interfaces, then update mockDbBatch, makeRecord, and
deferred with explicit parameter and return-type annotations. Ensure the mocked
database batch and record helpers use these interfaces so changes to the
WatermelonDB record shape remain type-checked.
In `@app/lib/methods/handleMediaDownload.ts`:
- Line 225: Update the exported persistMessage function signature to explicitly
declare a Promise<void> return type, while preserving its existing parameter
annotations and implementation behavior.
🪄 Autofix
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 Plus
Run ID: 87c843dc-98c1-40b6-a6be-ad443cf47a2d
📒 Files selected for processing (2)
app/lib/methods/handleMediaDownload.test.tsapp/lib/methods/handleMediaDownload.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 (3)
**/*.{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/methods/handleMediaDownload.tsapp/lib/methods/handleMediaDownload.test.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
Files:
app/lib/methods/handleMediaDownload.tsapp/lib/methods/handleMediaDownload.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Format JavaScript and TypeScript code with Oxfmt using the repository configuration: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where possible, and same-line brackets.
Follow Oxlint rules configured in.oxlintrc.json, including the import, React, Jest, TypeScript, and React Native plugins.
Files:
app/lib/methods/handleMediaDownload.tsapp/lib/methods/handleMediaDownload.test.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/methods/handleMediaDownload.tsapp/lib/methods/handleMediaDownload.test.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/handleMediaDownload.test.ts
| const db = database.active; | ||
| const batch: Model[] = []; | ||
| const messageRecord = await getMessageById(messageId); | ||
| if (messageRecord) { | ||
| batch.push( | ||
| messageRecord.prepareUpdate(m => { | ||
| m.attachments = mapAttachments({ attachments: m.attachments, uri, encryption, downloadUrl }); | ||
| }) | ||
| ); | ||
| } | ||
| const threadRecord = await getThreadById(messageId); | ||
| if (threadRecord) { | ||
| batch.push( | ||
| threadRecord.prepareUpdate(m => { | ||
| m.attachments = mapAttachments({ attachments: m.attachments, uri, encryption, downloadUrl }); | ||
| }) | ||
| ); | ||
| } | ||
| const threadMessageRecord = await getThreadMessageById(messageId); | ||
| if (threadMessageRecord) { | ||
| batch.push( | ||
| threadMessageRecord.prepareUpdate(m => { | ||
| m.attachments = mapAttachments({ attachments: m.attachments, uri, encryption, downloadUrl }); | ||
| }) | ||
| ); | ||
| } | ||
| if (batch.length) { | ||
| await db.write(async () => { | ||
|
|
||
| await db.write(async () => { | ||
| const batch: Model[] = []; | ||
| const messageRecord = await getMessageById(messageId); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find active-database transitions and their lifecycle context.
rg -n -C 5 --glob '*.{ts,tsx}' '\bsetActiveDB\s*\(' app
# Find persistence callers that can outlive a database switch.
rg -n -C 5 --glob '*.{ts,tsx}' '\bpersistMessage\s*\(' appRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handleMediaDownload relevant sections =="
sed -n '1,280p' app/lib/methods/handleMediaDownload.ts | nl -ba | sed -n '1,280p'
echo "== database active implementation =="
sed -n '80,105p' app/lib/database/index.ts | nl -ba
echo "== package version hints for watermelon =="
rg -n '"watermelondb"' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
echo "== imports of persistMessage and setImmediate usage =="
rg -n --glob '*.{ts,tsx}' 'persistMessage|setImmediate|setImmediate\(' app test 2>/dev/null || trueRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handleMediaDownload relevant sections =="
sed -n '1,280p' app/lib/methods/handleMediaDownload.ts | awk '{printf "%6d\t%s\n", NR, $0}'
echo "== database active implementation =="
sed -n '80,105p' app/lib/database/index.ts | awk '{printf "%6d\t%s\n", NR, $0}'
echo "== package version hints for watermelon =="
for f in package.json pnpm-lock.yaml package-lock.json yarn.lock; do
if [ -f "${f}" ]; then
echo "--- ${f} ---"
rg -n '"watermelondb"|watermelondb|`@nozbe/watermelondb`' "${f}" || true
fi
done
echo "== imports/usages =="
rg -n --glob '*.{ts,tsx}' 'persistMessage|setImmediate\(' app test 2>/dev/null || trueRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 13455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate database service implementations =="
fd -a 'Message|Thread|ThreadMessage' app/lib/database/services | sed 's#^\./##' | while read -r f; do
echo "--- ${f} ---"
rg -n -C 4 'export async function (getMessageById|getThreadById|getThreadMessageById)|collection|database.active|database' "$f" || true
done
echo "== full handleMediaDownload test relevant block =="
sed -n '140,235p' app/lib/methods/handleMediaDownload.test.ts | awk '{printf "%6d\t%s\n", NR, $0}'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 6026
🌐 Web query:
@nozbe/watermelondb 0.28.1-0 src Database index.js write batch database writer
💡 Result:
In WatermelonDB, the database.write() method and batch() function are central components for handling database modifications safely and efficiently [1][2]. The database.write() method ensures that operations are executed within a writer transaction [2]. This is critical for maintaining data consistency, as it guarantees that no other write operations occur concurrently [3]. When working inside a write block, you must use specific "prepare" methods (such as prepareUpdate, prepareCreate, prepareMarkAsDeleted, and prepareDestroyPermanently) rather than their asynchronous counterparts (like update() or create()) to prepare operations for a batch [1]. The batch() function, available on the Database instance or within an @writer method via this.batch(), accepts an array of these prepared operations [4][1]. By grouping these operations, WatermelonDB can execute them as a single transaction [5]. This is significantly more performant than executing individual write commands, as it reduces the overhead of database communication and transaction management [1][5]. Implementation details for batch include: - You must call batch() from inside an Action (a @writer method or a database.write() block) [4][1]. - It takes a list of prepared operations as arguments (or a single array) [1]. - Falsy values (null, undefined, false) passed to batch are ignored, allowing for conditional inclusion of operations [4][1]. - The method processes these operations by generating efficient SQL and notifying affected collections of the changes [4][5]. For example, using batching within a writer: await database.write(async => { await database.batch( record1.prepareUpdate(r => { r.title = 'New' }), collection.prepareCreate(r => { r.name = 'New' })) }) [1]
Citations:
- 1: https://watermelondb.dev/docs/Writers
- 2: https://watermelondb.dev/docs/CRUD
- 3: RFC: Improved Reader/Writer API Nozbe/WatermelonDB#1015
- 4: https://github.com/Nozbe/WatermelonDB/blob/22188ee5b6e3af08e48e8af52d14e0d90db72925/src/Database/index.js
- 5: SQLiteAdapter native implementations cleanup - part 2 Nozbe/WatermelonDB#1037
🌐 Web query:
raw.githubusercontent.com/Nozbe/WatermelonDB/v0.28.1-0/src/Database/index.js lines 200 260
💡 Result:
In WatermelonDB version 0.28.1-0, the src/Database/index.js file handles the batch method, which is a core feature for executing multiple database operations efficiently [1][2][3]. The batch method allows developers to execute multiple record operations (such as prepareCreate, prepareUpdate, and prepareDestroyPermanently) in a single atomic transaction [4][5][6]. Based on the codebase patterns established in WatermelonDB [1][5][7], the batch method and its surrounding logic in src/Database/index.js generally adhere to these critical requirements: 1. Mandatory Action Wrapper: The batch method must be called from within a database.write() action [5][8][7]. This ensures that all batched operations are processed within a consistent, thread-safe environment [4][9]. 2. Synchronous Preparation: All operations passed to batch (like those returned by record.prepareUpdate()) must be prepared synchronously before the batch method is invoked [5][10][7]. Failing to do so—for example, by introducing asynchronous gaps between preparation and the batch call—will trigger warnings or errors in the library, as these records must be committed to the database in one atomic, synchronous step [10][7]. If you examine the specific code block spanning lines 200–260 of src/Database/index.js in this version, you will find the implementation of these batching mechanics, including the underlying calls to the database adapter that perform the actual writes and the logic that ensures the operations are valid and correctly ordered [1][8]. Using batch is the recommended way to perform bulk operations, as it significantly reduces the overhead of individual database transactions [4][9].
Citations:
- 1: https://github.com/Nozbe/WatermelonDB/blob/22188ee5b6e3af08e48e8af52d14e0d90db72925/src/Database/index.js
- 2: Nozbe/WatermelonDB@f2a29e7
- 3: https://npmx.dev/package/@nozbe/watermelondb/v/0.28.1-0
- 4: https://viewlytics.ai/blog/watermelondb-react-native-complete-guide
- 5: Create or update record (batch) Nozbe/WatermelonDB#252
- 6: Cannot assign to read-only property '_status' issue on bulk delete Nozbe/WatermelonDB#1941
- 7: Error:
record.prepareUpdate was called on ${this.table}#${this.id} but wasn't sent to batch() synchronously -- this is bad!Nozbe/WatermelonDB#1368 - 8: database observable calls on every new record Nozbe/WatermelonDB#212
- 9: https://www.pkgpulse.com/guides/expo-sqlite-vs-watermelondb-vs-realm-react-native-local-2026
- 10: Better debugging information for Diagnostic Errors Nozbe/WatermelonDB#612
🌐 Web query:
github.com Nozbe WatermelonDB v0.28.1-0 src Database index.js const _writeBatch
💡 Result:
In WatermelonDB, there is no identifier or variable named _writeBatch in the src/Database/index.js file [1]. Based on the WatermelonDB source code, the database batching functionality is implemented via the batch method within the Database class [1]. This method is designed to execute multiple prepared operations (created using prepareCreate, prepareUpdate, etc.) atomically within an Action or Writer context [1][2]. The term _writeBatch is frequently associated with Google Cloud Firestore's Node.js SDK, where it is used as an internal property within the Transaction class to manage a collection of write operations [3][4]. It is possible that confusion between these two libraries has occurred, as _writeBatch is a documented internal implementation detail in Firestore [3][5], but it does not exist in the WatermelonDB source code [1].
Citations:
- 1: https://github.com/Nozbe/WatermelonDB/blob/22188ee5b6e3af08e48e8af52d14e0d90db72925/src/Database/index.js
- 2: https://watermelondb.dev/docs/Writers
- 3: https://github.com/googleapis/nodejs-firestore/blob/a13afe2997e7f641856e340c574f610ec6b98cf0/dev/src/transaction.ts
- 4: https://googleapis.dev/nodejs/firestore/3.4.0/transaction.js.html
- 5: JSON value of type NSMutableArray cannot be converted to NSString - Crash due to httpsCallable function invertase/react-native-firebase#3649
🌐 Web query:
Nozbe WatermelonDB 0.28.1-0 "prepareUpdate" "Cannot update a record"
💡 Result:
The error "Cannot update a record with pending changes" in WatermelonDB occurs when you attempt to call prepareUpdate on a model that already has an uncommitted prepared state (either from a previous prepareUpdate, prepareCreate, or similar operation) [1][2][3]. In WatermelonDB, prepareUpdate marks a record with a pending flag (_preparedState) to ensure data consistency [2][3]. This flag must be cleared by passing the record into a database.batch operation synchronously within the same write transaction [3][4][5]. Common causes and solutions for this error include: 1. Stale Prepared States: If a batch operation fails midway (e.g., due to an error in one of the operations), records already prepared in that batch may remain stuck with the _preparedState flag [6][3]. Ensure that your batch operations are robust and that errors are handled so they do not leave the database in an inconsistent state [6][3]. 2. Asynchronous Mismatches: The prepareUpdate call must be followed by a synchronous execution of database.batch [4][5]. If you perform asynchronous work between preparing the update and executing the batch, or if you prepare an update but fail to include it in a batch, the record will remain locked [3][5]. Always call batch within the same @writer or database.write block as the preparation [4][7]. 3. Duplicate Operations: Attempting to prepare multiple updates for the same record within the same batch or across overlapping operations can trigger this error [6]. Verify that your code de-duplicates records before adding them to the batch, especially when processing sync data from a server [6]. 4. Developer Error/Debugging: If you suspect a record is stuck, it is often due to an error in the logic flow where the "prepare" phase is invoked but the "execute/batch" phase is skipped or fails [3]. In development, WatermelonDB may issue warnings if prepareUpdate is called but not followed by a synchronous batch [4][5]. If you are encountering this in the context of sync (where a remote operation might conflict with a local one), ensure your sync logic correctly checks whether a record already exists locally and handles the resolution (update vs. create) appropriately before calling the preparation methods [1][6].
Citations:
- 1: Sync - Cannot update a record with pending changes Nozbe/WatermelonDB#1309
- 2: database observable calls on every new record Nozbe/WatermelonDB#212
- 3: Better debugging information for Diagnostic Errors Nozbe/WatermelonDB#612
- 4: https://stackoverflow.com/questions/73234694/error-record-prepareupdate-was-called-on-this-tablethis-id-but-wasnt-se
- 5: record.prepareUpdate was called on ${this.table}#${this.id} but wasn't sent to batch() synchronously -- this is bad! Nozbe/WatermelonDB#553
- 6: Cannot update a record with pending changes error during bidirectional sync Nozbe/WatermelonDB#1948
- 7: https://watermelondb.dev/docs/Writers
Use the captured database for persisted record lookups.
persistMessage queues a writer for db, but getMessageById, getThreadById, and getThreadMessageById read database.active inside db.write. If another operation switches the active database before the callback runs, db.batch can receive records prepared from a different database. Pass the captured db into these helpers, or query its collections directly, and add a regression test that switches databases while the queued writer is pending.
🤖 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/methods/handleMediaDownload.ts` around lines 226 - 230, Update
persistMessage and the getMessageById, getThreadById, and getThreadMessageById
lookup flow to use the captured db instance inside db.write instead of
database.active. Add a regression test that switches the active database while
the writer is queued and verifies records are read from the captured database.
1 participant
Proposed changes
persistMessagelooked up the message, thread and thread message and calledprepareUpdateon each outsidedb.write, committing the batch in a separate write later. A concurrent writer touching any of those cached records during that window — an incoming message update in a busy room, for instance — left the prepared records stale, so the commit threwCannot update a record with pending changes(reaching Bugsnag) and the finished download never attached to its message: the attachment stayed in a to-download state until the next sync, and the user had to tap it again.The three lookups, the three
prepareUpdatecalls and thedb.batchnow all run inside a singledb.writecallback. The file and download work indownloadMediaFilestays outside the lock.Because the lookups now happen under the lock, the
if (batch.length)guard moved inside it and only wrapsdb.batch. A download for a message that no longer exists locally acquires the writer lock and does nothing, rather than skipping it — one no-op acquisition per completed download. There's a test pinning that no empty batch is committed.persistMessageis now exported so the regression test can drive it directly; it was only reachable throughdownloadMediaFile's real file-download path. Its signature and its single caller are unchanged.Adds one regression test: a concurrent writer races the record being updated and the batch must commit without a "pending changes" throw. It fails on the current code and passes with the fix.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1466
How to test or reproduce
TZ=UTC pnpm test app/lib/methods/handleMediaDownload.test.ts— all pass; reverthandleMediaDownload.tsand the new test fails withCannot update a record with pending changestitle_linkwas saved, not just held in memoryTypes of changes
Checklist
Further comments
Summary by CodeRabbit