fix(db): move deleteMessage finds and prepares inside the writer lock

[OtavioStasiak]

Proposed changes

The deleteMessage branch of handleNotifyRoomReceived looked up the message, thread and thread message and called prepareDestroyPermanently 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 on the same id, for instance — left the prepared records stale, so the commit threw Cannot update a record with pending changes (reaching Bugsnag) and the delete never landed locally: the message stayed visible until the next full sync.

The three find calls, the three prepareDestroyPermanently calls and the db.batch now all run inside a single db.write callback, so a remote delete commits its whole batch under the lock.

The three locals are now typed | undefined, which the compiler required once they shared scope with the batch. This is not a behavior change: WatermelonDB's batch is typed (...records: $ReadOnlyArray<Model | Model[] | null | void | false>) and documents that falsy entries are ignored, so a record that isn't found is skipped exactly as before.

Adds one regression test: a concurrent writer races the record being deleted and the batch must commit without a "pending changes" throw. It fails on the current code and passes with the fix.
handleNotifyRoomReceived's signature and its caller are unchanged.

Issue(s)

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

How to test or reproduce

• TZ=UTC pnpm test app/lib/methods/subscriptions/room.test.ts — all pass; revert room.ts and the new test fails with Cannot update a record with pending changes
  • Open a room on this device and delete one of its messages from another client (web or a second device) — the message disappears immediately, no full sync needed
  • Do it in a busy room, so the delete overlaps with incoming message stream events on the same id
  • Delete a thread parent message and a message inside a thread — both vanish from the room and from the threads list
  • Reopen the room after each delete to confirm the message is gone locally, not just hidden from the current list
  • No visual changes; affects RoomView and the threads list only through what the local DB holds

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 deletion reliability during simultaneous updates and deletions.
  • Prevented pending-change errors and ensured prepared deletion records are properly cleared.
  • Continued to safely ignore missing deletion records while processing available records.

[coderabbitai]

Walkthrough

deleteMessage now prepares records inside the WatermelonDB write transaction. A concurrency test verifies serialized writes, complete deletion batching, and cleared prepared state.

Changes

Message deletion concurrency

Layer / File(s)	Summary
Serialized deletion transaction app/lib/methods/subscriptions/room.ts, app/lib/methods/subscriptions/room.test.ts	The deletion flow performs record lookup and prepareDestroyPermanently() calls inside the write transaction. Missing records remain optional. The concurrency test validates serialized updates, complete deletion batches, and cleared prepared records.

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

Possibly related PRs

• RocketChat/Rocket.Chat.ReactNative#7493: Addresses concurrent message-event races in RoomSubscription with serialized database writes.
• RocketChat/Rocket.Chat.ReactNative#7544: Adds related WatermelonDB concurrency handling and regression tests.
• RocketChat/Rocket.Chat.ReactNative#7546: Moves record preparation into the writer transaction for concurrent writes.

Suggested labels: type: bug

Suggested reviewers: diegolmello

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes moving deleteMessage lookups and preparation inside the database writer lock.

---

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]

🧹 Nitpick comments (1)

app/lib/methods/subscriptions/room.test.ts (1)

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

Assert that the interaction task ran.

interactionTask stays null if the deleteMessage branch never calls runAfterInteractions. Promise.all([concurrentWrite, null]) still resolves to a defined array, so Line 272 and the log check at Line 274 pass without exercising the deletion path. Add an explicit non-null assertion so a regression in the event dispatch fails loudly.

💚 Proposed assertion

 			await new Promise(resolve => setImmediate(resolve));
 			concurrentGate.resolve();

+			expect(interactionTask).not.toBeNull();
 			await expect(Promise.all([concurrentWrite, interactionTask])).resolves.toBeDefined();

🤖 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/subscriptions/room.test.ts` around lines 272 - 275, Add an
explicit assertion after the concurrent operation in the test around
interactionTask, verifying that interactionTask is non-null before or alongside
awaiting it. Keep the existing logging assertion, but ensure the test fails if
deleteMessage never invokes runAfterInteractions.

🤖 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/methods/subscriptions/room.test.ts`:
- Around line 272-275: Add an explicit assertion after the concurrent operation
in the test around interactionTask, verifying that interactionTask is non-null
before or alongside awaiting it. Keep the existing logging assertion, but ensure
the test fails if deleteMessage never invokes runAfterInteractions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5039f795-5118-4ac8-a7a2-b72cff703080

📥 Commits

Reviewing files that changed from the base of the PR and between 576377d and 6c447f8.

📒 Files selected for processing (2)

• app/lib/methods/subscriptions/room.test.ts
• app/lib/methods/subscriptions/room.ts

📜 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

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

🔇 Additional comments (4)

app/lib/methods/subscriptions/room.ts (1)

152-178: LGTM!

app/lib/methods/subscriptions/room.test.ts (3)

1-6: LGTM!

---

184-214: LGTM!

---

216-235: 📐 Maintainability & Code Quality

No change needed.

jest.restoreAllMocks() calls .mockRestore() on every local jest mock function, so it resets mockDbBatch’s implementation from the inline mockImplementation.

			> Likely an incorrect or invalid review comment.