revert: roll back SDK socket probing (#7298) and dependent reconnect fixes - #7521
Conversation
GitHub Actions
Inactive
WalkthroughThe PR changes socket recovery, connection readiness, room subscription handling, VoIP acceptance and signal processing, deep-link gating, foreground checks, and related tests. ChangesReconnect and media flows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DeepLinkSaga
participant ServerSelection
participant VoIPSession
Client->>DeepLinkSaga: open deep link or call push
DeepLinkSaga->>ServerSelection: wait for SERVER.SELECT_SUCCESS
ServerSelection-->>DeepLinkSaga: report selection success
DeepLinkSaga->>VoIPSession: resume login and navigation
VoIPSession->>VoIPSession: apply REST state signals
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
67-80: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
processSignal(signal)rejections before calling native handlers.
MediaSignalingSession.processSignalis async in this module’s async signaling path, but the two call sites now fire it withoutawaitor.catch(). The rejected promise is not handled by the surroundingtryblock, and in the loop the next signal can start before the previous one settles; attach.catch(...)or await the call so signal application and notification handling stay ordered.🤖 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/services/voip/MediaSessionInstance.ts` around lines 67 - 80, Update applyRestStateSignals to await this.instance.processSignal(signal) inside the signals loop before calling tryAnswerIfNativeAcceptedNotification, so rejections are caught by the surrounding try/catch and each signal is processed in order.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
app/sagas/__tests__/deepLinking.test.ts (1)
126-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit return type to
setupStore.Line 126 relies on inferred return typing despite the TypeScript guideline requiring explicit function return types. As per coding guidelines, “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/sagas/__tests__/deepLinking.test.ts` around lines 126 - 130, Update the setupStore function to declare an explicit return type matching the store returned by createStore, while preserving its existing preloadedState parameter and saga middleware setup.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.
Inline comments:
In `@app/sagas/state.js`:
- Line 29: Update the call to checkAndReopen() in the surrounding saga flow so
its promise rejection is explicitly handled, either by yielding it within the
existing try/catch or by attaching the established rejection handler. Preserve
the current success behavior while ensuring later rejections cannot become
unhandled.
In `@jest.config.js`:
- Around line 4-6: Update the transformIgnorePatterns allowlist in
jest.config.js to include `@rocket.chat/sdk` alongside the existing permitted
packages. Preserve the current regular-expression structure so Jest transforms
that dependency’s TypeScript sources instead of applying the default
node_modules ignore behavior.
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 67-80: Update applyRestStateSignals to await
this.instance.processSignal(signal) inside the signals loop before calling
tryAnswerIfNativeAcceptedNotification, so rejections are caught by the
surrounding try/catch and each signal is processed in order.
---
Nitpick comments:
In `@app/sagas/__tests__/deepLinking.test.ts`:
- Around line 126-130: Update the setupStore function to declare an explicit
return type matching the store returned by createStore, while preserving its
existing preloadedState parameter and saga middleware setup.
🪄 Autofix (Beta)
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: 129c29a8-f913-41c5-af51-9d705098623c
📒 Files selected for processing (17)
app/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/ddpSocket.test.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.testHelpers.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/deepLinking.jsapp/sagas/state.jsjest.config.jspatches/@rocket.chat+sdk+1.3.3-mobile.patch
💤 Files with no reviewable changes (5)
- app/lib/methods/subscriptions/room.test.ts
- app/lib/services/ddpSocket.test.ts
- app/lib/services/voip/MediaCallEvents.testHelpers.ts
- app/sagas/deepLinking.js
- app/lib/services/voip/MediaCallEvents.ios.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/sagas/state.jsjest.config.jsapp/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/sagas/__tests__/deepLinking.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the repository Prettier style: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where allowed, and same-line brackets.
Files:
app/sagas/state.jsjest.config.jsapp/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/sagas/__tests__/deepLinking.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 numbersUse TypeScript strict mode; resolve application imports relative to the
app/base URL.
Files:
app/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/sagas/__tests__/deepLinking.test.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP as a separate WebRTC peer-to-peer audio-call feature using Zustand stores and native CallKit/Telecom integrations; do not conflate it with VideoConf.
Files:
app/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Jest tests with
TZ=UTCto ensure deterministic timezone-dependent test behavior.
Files:
app/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/connect.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/sagas/__tests__/deepLinking.test.ts
app/lib/services/{sdk,restApi,connect}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the SDK service for WebSocket subscriptions, the REST API service for HTTP requests via
fetch, and the connect service for server connection management.
Files:
app/lib/services/connect.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use sagas for side effects such as initialization, authentication, rooms, messages, encryption, deep linking, and video conferencing.
Files:
app/sagas/__tests__/deepLinking.test.ts
🧠 Learnings (4)
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.
Applied to files:
app/sagas/state.js
📚 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/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/sagas/__tests__/deepLinking.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/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/connect.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/sagas/__tests__/deepLinking.test.ts
📚 Learning: 2026-05-05T21:08:33.177Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7298
File: patches/@rocket.chat+sdk+1.3.3-mobile.patch:79-79
Timestamp: 2026-05-05T21:08:33.177Z
Learning: In the RocketChat/Rocket.Chat.ReactNative repo, for patches under patches/*.patch (especially those touching rocket.chat/sdk), remember that the patching sandbox's node_modules reflects the pre-patch state. Grepping node_modules for symbols (e.g., userDisconnectCloseCode = 4000 in node_modules/rocket.chat/sdk/lib/drivers/ddp.ts) can yield false positives. Review patches by inspecting the diff and applying it to a fresh copy of the SDK or diffing against the SDK source, rather than relying on node_modules. Ensure the patch actually introduces/updates symbols in the SDK source and run the test suite to validate behavior after applying the patch.
Applied to files:
patches/@rocket.chat+sdk+1.3.3-mobile.patch
🔇 Additional comments (8)
app/sagas/__tests__/deepLinking.test.ts (1)
501-511: 📐 Maintainability & Code QualityRun this updated Jest suite with UTC configured.
Validate with
TZ=UTCbefore merge to keep timer-dependent tests deterministic. As per coding guidelines, “Run Jest tests withTZ=UTC.”Source: Coding guidelines
patches/@rocket.chat+sdk+1.3.3-mobile.patch (1)
1-16: LGTM!app/lib/services/connect.ts (1)
43-46: LGTM!Also applies to: 145-169
app/lib/services/connect.test.ts (1)
54-63: LGTM!Also applies to: 446-480
app/lib/services/voip/MediaCallEvents.ts (1)
81-106: LGTM!app/lib/services/voip/MediaCallEvents.test.ts (1)
147-147: LGTM!app/lib/services/voip/MediaCallLogger.test.ts (1)
21-35: LGTM!Also applies to: 55-69
app/lib/services/voip/MediaSessionInstance.test.ts (1)
191-200: LGTM!Also applies to: 522-522, 556-556
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109419 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNR8Uq418LHAqTVUytyTT7VESEAy53Nd-sxfEui2_KlD05yvgWVv0dz673DxyGLpF2blpe0zE2cXIU_Wk7qS |
|
iOS Build Available Rocket.Chat 4.75.0.109420 |
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Consolidated review
Method: 8 finder angles (line-by-line, removed-behavior audit, cross-file trace, altitude, efficiency, simplification, conventions, reuse), deduped to 16 findings, each verified against worktree code, node_modules SDK source, and git history. Verdicts below only include what survived verification.
The revert is mechanically clean: no dangling references to awaitDdpLoggedIn/forceReopen/probe, tsc clean, targeted jest green. Two items are over-broad — they roll back work independent of #7298:
Over-broad (recommend restoring)
-
SDK orphan-socket guard —
patches/@rocket.chat+sdk+1.3.3-mobile.patch. The removedopen()old-socket teardown andonCloseidentity guard came from #7380 (80989f51e), not #7298. Upstreamddp.tsreplacesthis.connectionwithout closing the old socket and emits'close'unconditionally, so a zombie socket's late close flips Redux to disconnected and tears down a healthy live connection ("Waiting for network" on a good socket). Restoring those two hunks is independent of the probing logic being reverted. -
#7493
updateMessageconcurrency coverage —room.test.tsis deleted wholesale, but itsdescribe('updateMessage concurrency')block (added by360cabcbb, not in the revert set) covers the writer-serialization fix that still lives atroom.ts:258. No other test file covers it. Keep that describe (and thesubscribeblock); delete only the describes tied to reverted behavior (handleLogin,handleClose, DDP recovery,isAlive).
Accepted revert costs (confirmed pre-#7298 bugs, restored by design — for the record)
- VoIP re-entrancy —
MediaSessionInstance.ts:60:answerCallruns synchronously inside the lib'sprocessSignaltick; theaccept()state change is dropped under theupdatingInputTrackre-entrancy lock → mic never opens → 10sTIMEOUT_TO_PROGRESS_SIGNALINGhangup. - Fire-and-forget
processSignal—MediaSessionInstance.ts:74,120: replayed signals race (answer can process before its offer's async setup finishes); rejections are unhandled. - Lock-screen accept —
MediaCallEvents.ts:97: same-workspace accept no longer heals a zombie socket before replaying signals; answer SDP goes out over a half-open session. Stated intent of the revert. - Poisoned subscription array —
room.ts:73: SDKsubscriberesolvesundefinedonnosub; baresub.unsubscribe()then aborts theforEach, leaking every later subscription server-side. - Room streams never re-created after reconnect (pre-#7426 state) and
readMessagescan mark a room read after the user exits, via aconnected/closeevent landing during the asyncremoveListenerwindow with noisAliveguard (room.ts:54-55,96). - Silent socket death without a
closeevent —state.js:22-24: foreground handler gated onisAuthAndConnected()skipscheckAndReopen()precisely when the socket died silently in background. (The "suspended reopen timer → indefinite stall" variant was rejected during verification: the SDK timer fires on foreground.) - Minor: unhandled rejection from
checkAndReopen()when foregrounded offline (state.js:29).
Rejected during verification
- deepLinking
METEOR.SUCCESSgate removal — both proposed failure modes fail: SDKlogin()awaitsconnect()before sending, so the resume login stalls rather than throwing or being rejected. Residual risk is a duplicate/misorderedloginRequestracing the'connected'-event login inconnect.ts. The gate was a #7298 dependent, so the rollback is legitimate. - Missed-wakeup in the inline login wait (
connect.ts:150-163) — impossible: Redux dispatch is synchronous and JS is single-threaded, so state cannot change between the outerisLoginReady()check andstore.subscribe. Behavior-equivalent to the removedawaitDdpLoggedIn. Worth restoring the one-line rationale comment (why redux state overddp.loggedIn).
Nits
connect.ts:44const s→state;connect.test.tscbparam, module-levelmockStoreSubscribeCallbacksserving one test, positionalconnectedHandlers[1](correct today, fragile to listener order);MediaSessionInstance.ts:121stray blank line.
Verdict: ship after restoring the SDK orphan-socket hunks and the #7493 test block. Everything else is the revert's intended cost and should be tracked against the replacement work.
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/methods/subscriptions/room.test.ts (2)
132-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the recovery-listener contract.
This only verifies the initial
subscribeRoomcall; it neither invokes theconnectedhandler nor asserts thatcloseis registered. Assert both registrations and verify triggeringconnecteddoes not create a second room subscription.🤖 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 132 - 137, Extend the subscription test around sub.subscribe() to assert that both the connected recovery handler and close handler are registered. Invoke the captured connected handler and verify mockSubscribeRoom remains called exactly once with rid, preserving the no-duplicate-subscription contract.
141-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the prepared-record fixture explicitly.
Add a
PreparedRecordinterface plus explicit return types formakeRecordandprepareUpdate; theanyupdater currently hides fixture-contract mistakes.Proposed refactor
+ interface PreparedRecord { + _preparedState: 'update' | null; + prepareUpdate(recordUpdater: (record: PreparedRecord) => void): PreparedRecord; + } + - const makeRecord = (debugName: string) => ({ + const makeRecord = (debugName: string): PreparedRecord => ({ _preparedState: null as string | null, - prepareUpdate(recordUpdater: (m: any) => void) { + prepareUpdate(recordUpdater: (record: PreparedRecord) => void): PreparedRecord {As per coding guidelines, “add explicit type annotations to function parameters and return types” and “Prefer interfaces over type aliases for defining object shapes in TypeScript.”
🤖 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 141 - 151, The prepared-record fixture in makeRecord is under-typed and its updater uses any. Define a PreparedRecord interface describing _preparedState and prepareUpdate, then use it as makeRecord’s return type and as the prepareUpdate return type; replace the updater parameter’s any with the appropriate explicitly typed record interface while preserving the existing pending-update 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/lib/methods/subscriptions/room.test.ts`:
- Around line 132-137: Extend the subscription test around sub.subscribe() to
assert that both the connected recovery handler and close handler are
registered. Invoke the captured connected handler and verify mockSubscribeRoom
remains called exactly once with rid, preserving the no-duplicate-subscription
contract.
- Around line 141-151: The prepared-record fixture in makeRecord is under-typed
and its updater uses any. Define a PreparedRecord interface describing
_preparedState and prepareUpdate, then use it as makeRecord’s return type and as
the prepareUpdate return type; replace the updater parameter’s any with the
appropriate explicitly typed record interface while preserving the existing
pending-update behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fd36135-c912-4b78-ab1e-ee9b2969cca4
📒 Files selected for processing (5)
app/lib/methods/subscriptions/room.test.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaSessionInstance.tspatches/@rocket.chat+sdk+1.3.3-mobile.patch
💤 Files with no reviewable changes (1)
- app/lib/services/voip/MediaSessionInstance.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/lib/services/connect.ts
- patches/@rocket.chat+sdk+1.3.3-mobile.patch
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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.tsapp/lib/services/connect.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 numbersUse TypeScript strict mode; resolve application imports relative to the
app/base URL.
Files:
app/lib/methods/subscriptions/room.test.tsapp/lib/services/connect.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the repository Prettier style: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where allowed, and same-line brackets.
Files:
app/lib/methods/subscriptions/room.test.tsapp/lib/services/connect.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Jest tests with
TZ=UTCto ensure deterministic timezone-dependent test behavior.
Files:
app/lib/methods/subscriptions/room.test.tsapp/lib/services/connect.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/methods/subscriptions/room.test.tsapp/lib/services/connect.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/methods/subscriptions/room.test.tsapp/lib/services/connect.test.ts
🔇 Additional comments (4)
app/lib/services/connect.test.ts (3)
54-55: LGTM!Also applies to: 101-115
422-422: LGTM!Also applies to: 441-442, 452-461, 472-473, 488-488
504-504: LGTM!Also applies to: 521-521, 533-534
app/lib/methods/subscriptions/room.test.ts (1)
153-175: LGTM!
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109425 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRd9VMd9lck4FjsDlCnsyyb_hi6qPf6198niaQK9_g-SV3qS2qN5bfXGX1GmLWwbYYs-5ctsYO3dpMfrwSi |
|
iOS Build Available Rocket.Chat 4.75.0.109426 |
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
113-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the callback return type.
As per coding guidelines, declare the async callback’s return type explicitly as
Promise<void>.Proposed change
- this.mediaSignalListener = sdk.onStreamData('stream-notify-user', async (ddpMessage: IDDPMessage) => { + this.mediaSignalListener = sdk.onStreamData('stream-notify-user', async (ddpMessage: IDDPMessage): Promise<void> => {🤖 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/services/voip/MediaSessionInstance.ts` at line 113, Update the async callback passed to sdk.onStreamData in MediaSessionInstance to explicitly declare a Promise<void> return type, preserving its existing implementation and 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/lib/services/voip/MediaSessionInstance.ts`:
- Line 113: Update the async callback passed to sdk.onStreamData in
MediaSessionInstance to explicitly declare a Promise<void> return type,
preserving its existing implementation and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bb1a92d-105b-4fb2-b3b1-e41566766769
📒 Files selected for processing (2)
app/lib/services/connect.tsapp/lib/services/voip/MediaSessionInstance.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/services/connect.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/services/voip/MediaSessionInstance.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 numbersUse TypeScript strict mode; resolve application imports relative to the
app/base URL.
Files:
app/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the repository Prettier style: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where allowed, and same-line brackets.
Files:
app/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP as a separate WebRTC peer-to-peer audio-call feature using Zustand stores and native CallKit/Telecom integrations; do not conflate it with VideoConf.
Files:
app/lib/services/voip/MediaSessionInstance.ts
🧠 Learnings (1)
📚 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/services/voip/MediaSessionInstance.ts
🔇 Additional comments (2)
app/lib/services/voip/MediaSessionInstance.ts (2)
113-124: 🎯 Functional CorrectnessVerify that stream callbacks are serialized.
awaitsequences processing only within one callback invocation. Ifsdk.onStreamDatainvokes another async callback before the previousprocessSignalresolves, signals may still mutate the media session concurrently and out of order. Confirm the SDK contract or add a shared promise queue.
60-63: LGTM!Also applies to: 74-78, 122-129
GitHub Actions
Error
processSignal returns a promise and mutates the state that tryAnswerIfNativeAcceptedNotification reads, so both call sites must sequence on it. Also corrects the isLoginReady comment: close does clear meteor.connected, but neither it nor ddp.loggedIn survives a silent background death.
44d451b to
8af1086
Compare
GitHub Actions
Failure
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/connect.ts (1)
138-171: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
drainPendingHangups()return the hangup work it performs.
drainPendingHangups(): voidcallstransporter.hangup(id, 'normal')synchronously and does not expose pending work to callers inconnect.ts. Rewording theawaitor ignoring this makes callers think the drain is complete while hangup delivery may still run. Iftransporter.hangup()can be async or reject, havedrainPendingHangups()return/collect those promises and await them internally before resolving.🤖 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/services/connect.ts` around lines 138 - 171, Update mediaSessionInstance.drainPendingHangups and its drainPendingHangups method contract to return a Promise representing all transporter.hangup operations; collect the promises for each pending hangup and await them before resolving, propagating rejection. Keep the existing connect.ts await in the connected listener so it waits for actual hangup delivery.
🤖 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/subscriptions/room.ts`:
- Around line 54-55: Split the shared handleConnection flow in the room
subscription setup: have the 'connected' listener perform loadMissedMessages and
read, while the 'close' listener only clears user typing. Update the listener
callbacks and the handleConnection implementation without changing the existing
connected behavior or error logging.
---
Outside diff comments:
In `@app/lib/services/connect.ts`:
- Around line 138-171: Update mediaSessionInstance.drainPendingHangups and its
drainPendingHangups method contract to return a Promise representing all
transporter.hangup operations; collect the promises for each pending hangup and
await them before resolving, propagating rejection. Keep the existing connect.ts
await in the connected listener so it waits for actual hangup delivery.
🪄 Autofix (Beta)
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: 28ff30c1-0622-4324-9432-9dd30a45c817
📒 Files selected for processing (17)
app/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/ddpSocket.test.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.testHelpers.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallLogger.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/sagas/__tests__/deepLinking.test.tsapp/sagas/deepLinking.jsapp/sagas/state.jsjest.config.jspatches/@rocket.chat+sdk+1.3.3-mobile.patch
💤 Files with no reviewable changes (4)
- app/lib/services/ddpSocket.test.ts
- app/lib/services/voip/MediaCallEvents.ios.test.ts
- app/lib/services/voip/MediaCallEvents.testHelpers.ts
- app/sagas/deepLinking.js
🚧 Files skipped from review as they are similar to previous changes (10)
- app/lib/services/voip/MediaCallLogger.test.ts
- jest.config.js
- app/lib/services/voip/MediaSessionInstance.test.ts
- app/lib/services/voip/MediaCallEvents.ts
- app/lib/services/voip/MediaCallEvents.test.ts
- app/lib/services/connect.test.ts
- app/sagas/state.js
- app/lib/methods/subscriptions/room.test.ts
- patches/@rocket.chat+sdk+1.3.3-mobile.patch
- app/sagas/tests/deepLinking.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- 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/services/voip/MediaSessionInstance.tsapp/lib/services/connect.tsapp/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/services/voip/MediaSessionInstance.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Before committing changes to JavaScript or TypeScript files, runpnpm prettier-lintandTZ=UTC pnpm testfor the modified files.
Use the local-first data flow: the UI reads from WatermelonDB, while sagas synchronize data with the server.
Use Redux and Redux-Saga for global or server state, and use Zustand for feature-local stores; do not assume all state is in Redux.
Files:
app/lib/services/voip/MediaSessionInstance.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.ts
🧠 Learnings (1)
📚 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/services/voip/MediaSessionInstance.tsapp/lib/services/connect.tsapp/lib/methods/subscriptions/room.ts
🔇 Additional comments (6)
app/lib/services/connect.ts (2)
43-49: LGTM!
466-470: LGTM!app/lib/methods/subscriptions/room.ts (2)
37-37: LGTM!
73-79: LGTM!app/lib/services/voip/MediaSessionInstance.ts (2)
60-64: LGTM!
122-128: LGTM!
#7521 squash-merged the purring-seahorse reverts into develop as a single commit, and GitHub retargeted this PR at develop once that branch was deleted. The squash tree is byte-identical to the purring-seahorse tip dry-badger already merged, so develop carries no content this branch is missing and the resulting tree is unchanged. Every conflict is that same duplicate content, resolved to dry-badger's side. jest.config.js again needed restoring by hand: the revert drops the @rocket.chat/sdk and tiny-events transform entries, and because this branch has no net change there the removal wins the auto-merge silently and breaks ddpSocket.test.ts.
2 participants
Proposed changes
Full rollback of #7298 (
fix(voip,connect): probe Meteor Connect on foreground and lock-screen accept) and the four follow-up fixes that exist only to patch bugs it introduced:probe/forceReopen/checkAndReopenpatch + app-side wiring)The SDK patch (
patches/@rocket.chat+sdk+1.3.3-mobile.patch) returns to its pre-7298 content (only themedia-signal/media-callssubscription additions). Since #7298 merged (2026-05-06), the SDK/connect surface has needed a steady stream of reconnect/resubscribe fixes, and the aggressive socket-lifecycle healing it introduced is the root of the instability (foregroundcheckAndReopenracing the SDK reconnect timer;forceReopenshort-circuiting the login path, stranding the app connected with zero subscriptions).Preserved through the rollback:
awaitDdpLoggedInKnown trade-off: this reintroduces the lock-screen VoIP accept failure #7298 originally fixed (answer SDP through a half-open session →
TIMEOUT_TO_PROGRESS_SIGNALING). A forward fix for socket-liveness should be designed separately rather than keeping the probing approach.Issue(s)
Reverts #7298 and its follow-ups #7362, #7380, #7426, #7475.
How to test or reproduce
Screenshots
N/A
Types of changes
Checklist
Further comments
Rollback strategy and per-commit dependency analysis were mapped before execution: the five reverts restore pre-7298 semantics without clobbering independent work that landed on the same files.
Summary by CodeRabbit