fix(db): move persistMessage lookups and prepares inside the writer lock

[OtavioStasiak]

Proposed changes

persistMessage looked up the message, thread and thread message and called prepareUpdate on each outside db.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 threw Cannot 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 prepareUpdate calls and the db.batch now all run inside a single db.write callback. The file and download work in downloadMediaFile stays outside the lock.

Because the lookups now happen under the lock, the if (batch.length) guard moved inside it and only wraps db.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.

persistMessage is now exported so the regression test can drive it directly; it was only reachable through downloadMediaFile'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; revert handleMediaDownload.ts and the new test fails with Cannot update a record with pending changes
• Tap an image, video or audio attachment in a room to download it — it renders as soon as the download finishes, with no second tap needed
• Do the same in a busy room where messages keep arriving, so the download completes while other writes are in flight
• Download an attachment from a message inside a thread, and from a thread parent — both the room view and the thread view show it
• Download an attachment in an E2EE room, so the decrypt-file queue runs before the record is persisted
• Reopen the room after each download to confirm the local title_link was saved, not just held in memory
• Affects RoomView and the thread views, only through what the local DB holds for the attachment## Screenshots

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

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when multiple media downloads update the same message records concurrently.
  • Ensured message, thread, and attachment updates are saved together to prevent partial changes.
  • Prevented unnecessary database updates when matching records are unavailable.

[coderabbitai]

Walkthrough

persistMessage is now exported and performs record lookup, update preparation, and batching inside one serialized database write transaction. Tests cover concurrent writers, attachment updates, prepared-record handling, and missing records.

Changes

Media persistence transaction

Layer / File(s)	Summary
Transaction boundary app/lib/methods/handleMediaDownload.ts	persistMessage is exported. Record lookup, update preparation, and conditional batching now run inside one database write transaction.
Concurrency and no-op validation app/lib/methods/handleMediaDownload.test.ts	Mocks model serialized writes and prepared records. Tests validate concurrent updates, attachment persistence, batching, and the no-op path for missing records.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• RocketChat/Rocket.Chat.ReactNative#7493: Uses the same transactional approach for concurrent WatermelonDB updates.
• RocketChat/Rocket.Chat.ReactNative#7544: Adds similar writer-lock handling and regression tests.
• RocketChat/Rocket.Chat.ReactNative#7550: Applies the same transaction pattern to deleteMessage.

Suggested labels: type: bug

Suggested reviewers: diegolmello

🚥 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 clearly and concisely describes moving persistMessage lookups and preparation inside the database writer lock.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)

• NATIVE-1466: 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

app/lib/methods/handleMediaDownload.ts (1)

225-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Declare the exported result type.

Declare Promise<void> on persistMessage. 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 win

Type the test doubles.

mockDbBatch and makeRecord use any. makeRecord and deferred also infer their return types. Define minimal PreparedRecord and Deferred interfaces, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 576377d and 1b0c2c0.

📒 Files selected for processing (2)

• app/lib/methods/handleMediaDownload.test.ts
• app/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.ts
• app/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.ts
• app/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.ts
• app/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.ts
• app/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