fix: run handleDelete finds and prepares inside the writer lock - #7552
fix: run handleDelete finds and prepares inside the writer lock#7552OtavioStasiak wants to merge 2 commits into
Conversation
Walkthrough
ChangesMessage deletion transaction
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 (2)
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
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/containers/MessageErrorActions.tsx (3)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit TypeScript signatures to the new functions.
app/containers/MessageErrorActions.tsx#L29-L29: declare the writer callback return type, for exampleasync (): Promise<void> =>.app/containers/MessageErrorActions.test.tsx#L28-L146: declare parameter and return types for the new helpers andFakeDatabasemethods.As per coding guidelines, TypeScript function parameters and return types must have explicit annotations.
🤖 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/containers/MessageErrorActions.tsx` at line 29, Annotate the writer callback passed to db.write in MessageErrorActions.tsx with an explicit Promise<void> return type. In MessageErrorActions.test.tsx, add explicit parameter and return-type annotations to every new helper and FakeDatabase method within lines 28-146; no direct changes are needed outside these affected functions.Source: Coding guidelines
32-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the comments explain the transaction reason.
The comments describe the next operation. Replace them with one short comment that explains why preparation must occur inside
db.write: a concurrent writer must not create conflicting pending changes.As per coding guidelines, comments must explain the “why” behind code decisions, not the “what”.
🤖 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/containers/MessageErrorActions.tsx` around lines 32 - 60, Update the comments in the message/thread deletion preparation flow within the relevant db.write transaction to explain why these operations must be prepared there: preventing concurrent writers from creating conflicting pending changes. Remove the comments that merely describe deleting objects, finding the thread tree, updating the header, or deleting the thread.Source: Coding guidelines
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse role-specific record names.
Line 38 uses
msgfor the persisted failed-message record. Line 46 usesmsgfor the thread header. Rename these variables to names such asfailedMessageandthreadHeader.As per coding guidelines, function variables must use descriptive names that convey their purpose.
🤖 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/containers/MessageErrorActions.tsx` around lines 38 - 46, In the message cleanup flow, rename the persisted failed-message variable in the first try block to a descriptive name such as failedMessage, and rename the thread-header variable in the following try block to threadHeader. Update all corresponding method calls and references while preserving the existing behavior.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/containers/MessageErrorActions.tsx`:
- Line 29: Annotate the writer callback passed to db.write in
MessageErrorActions.tsx with an explicit Promise<void> return type. In
MessageErrorActions.test.tsx, add explicit parameter and return-type annotations
to every new helper and FakeDatabase method within lines 28-146; no direct
changes are needed outside these affected functions.
- Around line 32-60: Update the comments in the message/thread deletion
preparation flow within the relevant db.write transaction to explain why these
operations must be prepared there: preventing concurrent writers from creating
conflicting pending changes. Remove the comments that merely describe deleting
objects, finding the thread tree, updating the header, or deleting the thread.
- Around line 38-46: In the message cleanup flow, rename the persisted
failed-message variable in the first try block to a descriptive name such as
failedMessage, and rename the thread-header variable in the following try block
to threadHeader. Update all corresponding method calls and references while
preserving the existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23ba2ef6-6f9b-4dba-9b8f-aa2789f9e027
📒 Files selected for processing (2)
app/containers/MessageErrorActions.test.tsxapp/containers/MessageErrorActions.tsx
📜 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/containers/MessageErrorActions.test.tsxapp/containers/MessageErrorActions.tsx
**/*.{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/containers/MessageErrorActions.test.tsxapp/containers/MessageErrorActions.tsx
**/*.{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/containers/MessageErrorActions.test.tsxapp/containers/MessageErrorActions.tsx
🧠 Learnings (3)
📚 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/containers/MessageErrorActions.test.tsxapp/containers/MessageErrorActions.tsx
📚 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/containers/MessageErrorActions.test.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/containers/MessageErrorActions.test.tsxapp/containers/MessageErrorActions.tsx
GitHub Actions
Waiting
GitHub Actions
Waiting
1 participant
Proposed changes
handleDelete in MessageErrorActions prepared its WatermelonDB changes before acquiring the writer lock. On the thread branch (tmid set) it called message.prepareDestroyPermanently() and then awaited three find calls messages.find(message.id), messages.find(tmid), threads.find(tmid) — all outside db.write, only opening the write at the very end to run the batch.
That leaves a window where the record carries a pending prepared change but nothing holds the lock. If a saga writes the same record during it, one of two things happens:
create/update/delete.
Either way, deleting a failed thread message fails and the thread count / thread record are left inconsistent. The plain (non-thread) branch had noawait between prepare and batch, so it was never exposed.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1467
How to test or reproduce
record with pending changes (thread_messages#msg-1)), then git stash pop.
(tmid set).
failed state → have a second user post to the same thread (or reconnect so the sync saga writes it) → tap the failed message and choose Delete while
that write lands.
stale tcount.
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit