fix(db): move sendMessage reads and prepares inside the writer lock

[OtavioStasiak]

Proposed changes

sendMessage and changeMessageStatus read records and called prepareUpdate/prepareCreate outside
db.write, committing the batch in a separate write later. A concurrent writer touching the same cached record during
that window left the prepared records stale, so the commit threw Cannot update a record with pending changes (reaching Bugsnag) and the pending change was lost — a sent message stuck in TEMP, a decrypted message still encrypted.

Both functions now do their reads, prepares and batch inside a single db.write callback. Encryption and the network call stay outside the lock, so nothing is held longer than needed. Same approach already used by RoomSubscription.updateMessage.

Adds one regression test per function: a concurrent writer races the same record and the batch must commit without a "pending changes" throw. Both fail on the current code and pass with the fix. Signatures and callers unchanged.

Issue(s)

https://rocketchat.atlassian.net/browse/NATIVE-1463

How to test or reproduce

• TZ=UTC pnpm test app/lib/methods/sendMessage.test.ts — both tests pass; revert
  sendMessage.ts and they fail with Cannot update a record with pending changes
• Send a message in a busy room (lots of incoming stream activity) — no message
  stuck in the temp/sending state
• Send a thread reply, both in a new thread and an existing one
• Send in an E2EE room — message decrypts instead of staying encrypted
• Resend a failed message from the message actions
• Send from the share extension (text-only share into a room)

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 message-sending reliability when multiple updates occur simultaneously.
  • Prevented pending-change errors during concurrent message or subscription updates.
  • Ensured message, thread, status, and draft changes are committed together consistently.
  • Preserved expected message content and delivery statuses during concurrent activity.

• Tests

  • Added comprehensive coverage for concurrent sending, status updates, encryption handling, and database commits.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ec233cef-3b45-486e-af02-2a71f23c293c

📥 Commits

Reviewing files that changed from the base of the PR and between 11bddb7 and 2f34e44.

📒 Files selected for processing (1)

• app/lib/methods/sendMessage.ts

💤 Files with no reviewable changes (1)

• app/lib/methods/sendMessage.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: ESLint and Test / run-eslint-and-test
• GitHub Check: E2E Shard Preflight
• GitHub Check: format

---

Walkthrough

sendMessage now performs database reads and record preparation inside WatermelonDB writer transactions. New tests cover concurrent message creation and status updates.

Changes

sendMessage concurrency handling

Layer / File(s)	Summary
Writer-locked message operations app/lib/methods/sendMessage.ts	Message creation and status updates now prepare thread, message, and draft records inside db.write.
Concurrency regression validation app/lib/methods/sendMessage.test.ts	Tests race message creation and status updates. They verify committed records, expected statuses, cleared drafts, and no pending prepared records.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant sendMessage
  participant changeMessageStatus
  participant db.write
  participant WatermelonDB
  sendMessage->>db.write: Prepare thread, message, and draft records
  changeMessageStatus->>db.write: Read and prepare status records
  db.write->>WatermelonDB: Commit prepared records

Loading

Possibly related PRs

• RocketChat/Rocket.Chat.ReactNative#7493: Both changes move WatermelonDB preparation into writer locks and add concurrency tests.
• RocketChat/Rocket.Chat.ReactNative#7544: Both changes address concurrent WatermelonDB updates with regression tests.
• RocketChat/Rocket.Chat.ReactNative#7550: Both changes move database lookups, preparation, and batching into one writer transaction.

Suggested labels: type: bug

🚥 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 the main database race fix in sendMessage.
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-1463: 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/sendMessage.test.ts (2)

128-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add explicit return types to the test helpers.

deferred, flush, and loggedPendingChanges rely on inference. flush infers Promise<unknown>.

♻️ Proposed refactor

-const deferred = () => {
+const deferred = (): { promise: Promise<void>; resolve: () => void } => {
 	let resolve: () => void = () => undefined;
 	const promise = new Promise<void>(r => {
 		resolve = r;
 	});
 	return { promise, resolve };
 };
 
 // Let every already-queued microtask/promise chain settle.
-const flush = () => new Promise(resolve => setImmediate(resolve));
+const flush = (): Promise<void> => new Promise<void>(resolve => setImmediate(() => resolve()));
 
-const loggedPendingChanges = () => (log as jest.Mock).mock.calls.some(([error]) => /pending changes/.test(error?.message ?? ''));
+const loggedPendingChanges = (): boolean =>
+	(log as jest.Mock).mock.calls.some(([error]) => /pending changes/.test(error?.message ?? ''));

As per coding guidelines: "Use TypeScript for type safety; 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/sendMessage.test.ts` around lines 128 - 139, Add explicit
return type annotations to the test helpers deferred, flush, and
loggedPendingChanges, including a concrete resolved type for flush instead of
inferred Promise<unknown>; preserve their existing behavior and parameter
definitions.

Source: Coding guidelines

---

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

Make the fake reject batch outside a writer.

Real WatermelonDB throws when database.batch runs outside a writer. This fake accepts it. A future change that moves db.batch back outside db.write would therefore still pass these tests, which is the exact regression the PR guards against. Track writer depth in the fake to enforce the invariant.

♻️ Proposed refactor

 jest.mock('../database', () => {
 	let writerQueue: Promise<unknown> = Promise.resolve();
+	let writerDepth = 0;
 	return {
 		__esModule: true,
 		default: {
 			active: {
 				get: (name: string) => mockGetCollection(name),
 				// Serialized writer lock, like WatermelonDB's.
 				write: (callback: () => Promise<void>) => {
-					const run = writerQueue.then(() => callback());
+					const run = writerQueue.then(async () => {
+						writerDepth += 1;
+						try {
+							return await callback();
+						} finally {
+							writerDepth -= 1;
+						}
+					});
 					writerQueue = run.catch(() => undefined);
 					return run;
 				},
-				batch: (...args: unknown[]) => mockDbBatch(...args)
+				batch: (...args: unknown[]) => {
+					if (writerDepth === 0) {
+						// Mirrors WatermelonDB: batch() must run inside a writer.
+						return Promise.reject(new Error('batch() can not be called outside of a writer'));
+					}
+					return mockDbBatch(...args);
+				}
 			}
 		}
 	};
 });

🤖 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/sendMessage.test.ts` around lines 69 - 91, Update the
database mock’s writer implementation around write and batch to track whether
execution is currently inside a writer, incrementing depth for the callback and
reliably restoring it afterward. Make mockDbBatch reject or throw when invoked
with no active writer, while preserving its existing prepared-state reset
behavior for valid calls.

🤖 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/sendMessage.test.ts`:
- Around line 78-111: Rename the hoisted mock dependencies `getCollection` and
`encryptionGate` to `mockGetCollection` and `mockEncryptionGate` throughout the
test, including the database mock’s `active.get` implementation and encryption
mock factory. Update all remaining references consistently so Jest recognizes
them as hoist-safe.

---

Nitpick comments:
In `@app/lib/methods/sendMessage.test.ts`:
- Around line 128-139: Add explicit return type annotations to the test helpers
deferred, flush, and loggedPendingChanges, including a concrete resolved type
for flush instead of inferred Promise<unknown>; preserve their existing behavior
and parameter definitions.
- Around line 69-91: Update the database mock’s writer implementation around
write and batch to track whether execution is currently inside a writer,
incrementing depth for the callback and reliably restoring it afterward. Make
mockDbBatch reject or throw when invoked with no active writer, while preserving
its existing prepared-state reset behavior for valid calls.

🪄 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: 6fbfb188-4a64-4463-8fca-9edfbe7f4544

📥 Commits

Reviewing files that changed from the base of the PR and between 8a4ca9d and 49c3714.

📒 Files selected for processing (2)

• app/lib/methods/sendMessage.test.ts
• app/lib/methods/sendMessage.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: E2E Build iOS / ios-build
• GitHub Check: E2E Build Android / android-build
• 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/sendMessage.test.ts
• app/lib/methods/sendMessage.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/sendMessage.test.ts
• app/lib/methods/sendMessage.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/sendMessage.test.ts
• app/lib/methods/sendMessage.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/sendMessage.test.ts
• app/lib/methods/sendMessage.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/sendMessage.test.ts

🔇 Additional comments (6)

app/lib/methods/sendMessage.ts (3)

18-51: LGTM!

---

116-189: LGTM!

---

191-231: LGTM!

app/lib/methods/sendMessage.test.ts (3)

15-58: LGTM!

---

153-194: LGTM!

---

198-260: LGTM!