fix(db): move decryptPendingMessages prepares inside the writer lock - #7548
fix(db): move decryptPendingMessages prepares inside the writer lock#7548OtavioStasiak wants to merge 2 commits into
Conversation
GitHub Actions
Error
Walkthrough
ChangesPending message decryption
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant decryptPendingMessages
participant PendingMessageRecords
participant DatabaseWriter
decryptPendingMessages->>PendingMessageRecords: decrypt pending records
decryptPendingMessages->>DatabaseWriter: prepare updates inside serialized write
DatabaseWriter->>DatabaseWriter: batch valid updates
🚥 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
Inactive
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.
🧹 Nitpick comments (1)
app/lib/encryption/encryption.test.ts (1)
175-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the reusable test helpers.
makeMessageRecordanddeferredinfer their return shapes.makeMessageRecordalso erases the updater contract withany. Define focused interfaces and explicit return types so an invalidprepareUpdatemock cannot satisfy this regression test.Proposed change
+interface DeferredGate { + promise: Promise<void>; + resolve: () => void; +} + +interface TestMessageRecord { + id: string; + t: string; + msg: string; + e2e?: string; + subscription: { id: string }; + _preparedState: 'update' | null; + prepareUpdate(recordUpdater: (message: TestMessageRecord) => void): TestMessageRecord; +} + - const makeMessageRecord = (id: string) => { - const record: any = { + const makeMessageRecord = (id: string): TestMessageRecord => { + const record: TestMessageRecord = { // ... - prepareUpdate(recordUpdater: (m: any) => void) { + prepareUpdate(recordUpdater: (message: TestMessageRecord) => void): TestMessageRecord { // ... } }; return record; }; - const deferred = () => { + const deferred = (): DeferredGate => { // ... }; - record.prepareUpdate((m: any) => { - m.msg = 'written by another writer'; + record.prepareUpdate(message => { + message.msg = 'written by another writer'; })As per coding guidelines,
**/*.{ts,tsx}requires explicit annotations for function parameters and return types and prefers interfaces over type aliases for object shapes.Also applies to: 226-229
🤖 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/encryption/encryption.test.ts` around lines 175 - 200, Define focused interfaces for the message record and deferred helper, then update makeMessageRecord and deferred with explicit parameter and return types. Replace the any-based prepareUpdate updater with the interface’s typed updater contract, while preserving the existing pending-state behavior and promise resolver API.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.
Nitpick comments:
In `@app/lib/encryption/encryption.test.ts`:
- Around line 175-200: Define focused interfaces for the message record and
deferred helper, then update makeMessageRecord and deferred with explicit
parameter and return types. Replace the any-based prepareUpdate updater with the
interface’s typed updater contract, while preserving the existing pending-state
behavior and promise resolver API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab44418d-3a6d-4819-ade6-47f4caaff1df
📒 Files selected for processing (2)
app/lib/encryption/encryption.test.tsapp/lib/encryption/encryption.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/encryption/encryption.tsapp/lib/encryption/encryption.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/encryption/encryption.tsapp/lib/encryption/encryption.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/encryption/encryption.tsapp/lib/encryption/encryption.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/encryption/encryption.tsapp/lib/encryption/encryption.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/encryption/encryption.test.ts
🔇 Additional comments (6)
app/lib/encryption/encryption.ts (3)
338-343: LGTM!
378-378: 📐 Maintainability & Code QualityNo type change needed.
TThreadModel | TThreadMessageModelcorrectly describes the prepared message/thread records sent todb.batch;Modelwould be a broader type that loses the known message fields.> Likely an incorrect or invalid review comment.
344-379: 🗄️ Data Integrity & IntegrationNo change needed for deferred decrypt freshness.
A concurrent writer changes
record.msgonly before the decrypt result reachesdb.batch, and the test still closes on the decrypted update; this is not stale-decrypt overwrite behavior.app/lib/encryption/encryption.test.ts (3)
4-4: LGTM!
52-83: LGTM!
169-174: LGTM!Also applies to: 202-212
1 participant
Proposed changes
decryptPendingMessages
decrypted each pending e2e message and calledprepareUpdateoutsidedb.write, committing the batch in a separate write later. A concurrent writer touching the same cached record during that window — a new message arriving in the room, for instance — left the prepared records stale, so the commit threwCannot update a record with pending changes` (reaching Bugsnag) and the message stayed encrypted.Decryption now happens first, outside the lock, and the
prepareUpdatecalls plus thedb.batchrun inside a singledb.writecallback. Records whoseprepareUpdatethrows are now filtered out instead of being batched asnull.Adds one regression test: a concurrent writer races the same record and the batch must commit without a "pending changes" throw. It fails on the current code and passes with the fix. Signature and both callers (
Encryption.initialize,createOrUpdateSubscription) unchanged.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1464
How to test or reproduce
TZ=UTC pnpm test app/lib/encryption/encryption.test.ts— all pass; revertencryption.tsand the new test fails withCannot update a record with pending changespassword after opening the room) — they all decrypt, none stay encrypted
createOrUpdateSubscriptiontriggers a per-room decryptEncryption.initializedecrypts them allScreenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Bug Fixes
Tests