fix(e2ee): re-fetch subscription inside the write in toggleRoomE2EE - #7554
fix(e2ee): re-fetch subscription inside the write in toggleRoomE2EE#7554OtavioStasiak wants to merge 3 commits into
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe room encryption toggle reloads the current subscription during optimistic updates and rollbacks. Tests cover concurrent writes, cancellation, alert dismissal, failed persistence, and successful persistence. ChangesRoom encryption toggle
Estimated code review effort: 3 (Moderate) | ~20 minutes 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: 2
🤖 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/encryption/helpers/toggleRoomE2EE.ts`:
- Line 10: In app/lib/encryption/helpers/toggleRoomE2EE.ts at optimisticUpdate,
add an explicit Promise<void> return annotation. In
app/lib/encryption/helpers/toggleRoomE2EE.test.ts lines 39-70, add explicit void
or Promise<void> annotations to the fixture helpers as appropriate and define a
named interface for the createStore return object, using it as that helper’s
return type.
- Line 56: The alert configuration in toggleRoomE2EE must treat Android
outside-dismissal as cancellation: add an onDismiss handler that restores the
previous encryption value via optimisticUpdate, while preserving existing
cancellation behavior. Update app/lib/encryption/helpers/toggleRoomE2EE.test.ts
lines 92-104 to cover the outside-dismissal path and verify the rollback.
🪄 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: 9e91218a-0f54-4c40-9de6-e9ecfcbc33e1
📒 Files selected for processing (2)
app/lib/encryption/helpers/toggleRoomE2EE.test.tsapp/lib/encryption/helpers/toggleRoomE2EE.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: E2E Shard Preflight
🧰 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/helpers/toggleRoomE2EE.tsapp/lib/encryption/helpers/toggleRoomE2EE.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/helpers/toggleRoomE2EE.tsapp/lib/encryption/helpers/toggleRoomE2EE.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/helpers/toggleRoomE2EE.tsapp/lib/encryption/helpers/toggleRoomE2EE.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/helpers/toggleRoomE2EE.tsapp/lib/encryption/helpers/toggleRoomE2EE.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/helpers/toggleRoomE2EE.test.ts
🔇 Additional comments (1)
app/lib/encryption/helpers/toggleRoomE2EE.ts (1)
45-45: LGTM!Also applies to: 76-76
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Waiting
GitHub Actions
Waiting
|
|
||
| // Instantly feedback to the user | ||
| await db.write(async () => { | ||
| // Fetch the room again: a stream event may have updated the record while the alert was open or while the request was in flight |
2 participants
Proposed changes
toggleRoomE2EE fetched the subscription record once, before showing the confirm Alert, and then reused that same instance for all three of its writes: the optimistic update, the Cancel revert, and the revert after a failed saveRoomSettings.
The last two happen after the record has crossed user input (the Alert can stay open indefinitely) and a network round trip. If a subscription stream event updates the row in that window, the captured instance is stale, WatermelonDB rejects record.update(), and the revert is silently swallowed by the surrounding catch — leaving the switch showing an encrypted state the server never agreed to.
Each update path now re-fetches the subscription with getSubscriptionByRoomId(rid) inside its own db.write callback and updates the fresh record, bailing out if the row no longer exists. optimisticUpdate takes a rid instead of a record; toggleRoomE2EE(rid) and all of its callers are unchanged.
Added toggleRoomE2EE.test.ts, which mocks database.active with a store that reproduces WatermelonDB's staleness check (a handle remembers the version it was fetched at and throws record has pending changes if another writer bumped it). It covers a concurrent write landing while the Alert is open, one landing during the failing save request, and the happy path. Both concurrency tests fail against the previous code.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1468
How to test or reproduce
immediately and the confirm Alert appears.
message in the room) so a stream event writes to that subscription row.
fetched before the Alert opened.
update landing during the request. The switch must revert.
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit