fix(db): move sendMessage reads and prepares inside the writer lock - #7546
Conversation
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
Walkthrough
ChangessendMessage concurrency handling
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
Possibly related PRs
Suggested labels: 🚥 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
Error
![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/sendMessage.test.ts (2)
128-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit return types to the test helpers.
deferred,flush, andloggedPendingChangesrely on inference.flushinfersPromise<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 winMake the fake reject
batchoutside a writer.Real WatermelonDB throws when
database.batchruns outside a writer. This fake accepts it. A future change that movesdb.batchback outsidedb.writewould 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
📒 Files selected for processing (2)
app/lib/methods/sendMessage.test.tsapp/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.tsapp/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.tsapp/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.tsapp/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.tsapp/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!
GitHub Actions
Inactive
GitHub Actions
Error
…atus-inside-writer-lock
GitHub Actions
Error
GitHub Actions
Error
…atus-inside-writer-lock
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
…atus-inside-writer-lock
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
…atus-inside-writer-lock
GitHub Actions
Error
GitHub Actions
Error
…atus-inside-writer-lock
GitHub Actions
Waiting
GitHub Actions
Waiting
…7546) * fix(db): move sendMessage reads and prepares inside the writer lock * fix: test improvements * chore: remove comments
2 participants
Proposed changes
sendMessageandchangeMessageStatusread records and calledprepareUpdate/prepareCreateoutsidedb.write, committing the batch in a separate write later. A concurrent writer touching the same cached record duringthat 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.writecallback. Encryption and the network call stay outside the lock, so nothing is held longer than needed. Same approach already used byRoomSubscription.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; revertsendMessage.tsand they fail withCannot update a record with pending changesstuck in the temp/sending state
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Tests