fix(voip): reconnect stale socket before native call accept - #7526
Conversation
This reverts commit da389be.
)" This reverts commit cd6f2a8.
…P socket The SDK send() waits on a 'disconnected' event that nothing ever emitted, so zombie sockets caused in-flight sends to hang forever. Add reopenNow() to force a single shared reconnect and emit 'disconnected' to reject those sends, plus a bounded probe() for gray-zone liveness checks. Restore ddpSocket.test.ts with coverage for probe, reopenNow, subscription preservation, concurrent reconnect deduplication, and the send() listener-leak fix. Add @rocket.chat/sdk and tiny-events to Jest's transform-ignore exceptions so the SDK's TypeScript source is transformed.
After a long suspension the DDP socket can be a zombie: readyState=1 and connected=true while sends hang and no pong arrives. The native accept path previously replayed/answered immediately, so the WebRTC setup timed out at the remote-sdp stage. Add a single guarded accept helper that every accept path funnels through: - classify the socket by lastPing age and force reopenNow() when stale; - wait for login readiness and for the media-signal/media-calls subscriptions to be acked on the current socket; - replay REST state signals and answer only if the call is not already bound; - on timeout/failure terminate the native call, reset the native accepted id, and queue a best-effort hangup. Expose the socket via sdk.current.ddp and add DDPDriver passthroughs plus a waitForNotifyUserMediaSubs readiness helper in the SDK patch. Also guard checkVoipPermission so it does not reset the media session while a call is active or being accepted.
Extract the age/ping classification into classifySocketHealth in waitForLoginReady.ts and make the foreground-saga helper getSocketStaleness delegate to it. The accept gate imports from the lightweight helper file to avoid pulling connect.ts (and its heavy deps) into the VoIP unit tests.
On foreground after long suspension the DDP socket can be zombie while redux still reads connected=true. Classify socket freshness via lastPing and pingInterval: reopen immediately when stale, probe in the gray zone, and keep the existing checkAndReopen path for healthy/fresh sockets. Adds an in-flight probe guard so rapid AppState flaps do not stack probes.
Aborted gates now return early without running the failure ladder (terminate/endCall). activeGates cleanup only deletes its own controller so newer gates survive. Live-signal 'accepted' notifications from the stream listener funnel through acceptNativeCallWithReadiness instead of calling answerCall directly. SDK waitForNotifyUserMediaSubs now polls up to the timeout for media-signal/media-calls subscriptions to appear after a forced reopen. Also type the DDP shape in acceptNativeCall and remove "as any" from the AbortSignal fallback.
GitHub Actions
Error
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds socket health classification, coordinated DDP recovery, login and media-subscription readiness, and a cancellable native-call acceptance gate. VoIP event handling, foreground recovery, reconnect tracing, SDK wiring, and related tests now use these flows. ChangesVoIP readiness and socket recovery
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant MediaCallEvents
participant MediaSessionInstance
participant acceptNativeCallWithReadiness
participant DDPDriver
participant CallStore
MediaCallEvents->>MediaSessionInstance: receive accepted call event
MediaSessionInstance->>acceptNativeCallWithReadiness: pass callId
acceptNativeCallWithReadiness->>DDPDriver: recover socket and await media subscriptions
acceptNativeCallWithReadiness->>CallStore: read or reset native call state
acceptNativeCallWithReadiness->>MediaSessionInstance: apply signals and answer or end call
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (1)
40-62: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTeardown ordering looks right, but
createConnection()can hang indefinitely.Nulling the old handlers before
close()correctly prevents the orphan from scheduling a reopen, and the identity check inonClosecovers the race. However the returned promise only settles viaonOpen(resolve) or the constructor throwing /onerror(reject) — a socket that stalls inCONNECTINGnever settles, soopen()awaits forever.reopenNow()has its own 10s bound, but directopen()callers don't. Consider a connect deadline increateConnection.🤖 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 `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch around lines 40 - 62, Update createConnection so every connection attempt has a finite connect deadline and its returned promise rejects when the socket remains in CONNECTING beyond that deadline. Clear the deadline when onOpen, onerror, or onclose settles the attempt, and ensure timeout cleanup does not affect a newer connection.app/lib/services/connect.ts (1)
145-151: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
waitForLoginReadyfailures before draining hangups.
waitForLoginReady(5000)can resolve tofalsewhen login is not ready within the timeout, butdrainPendingHangups()still runs. If the drain consumes failed hangups while the connection is not authenticated, leave them queued and let the next'connected'retry instead of discarding them silently.🤖 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 145 - 151, Update the `'connected'` handler around `waitForLoginReady` so it checks the returned readiness boolean before calling `mediaSessionInstance.drainPendingHangups()`. Drain pending hangups only when login is ready; otherwise leave them queued for the next connection attempt, while preserving the existing error logging.app/lib/services/voip/MediaCallEvents.ts (1)
90-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
callIdbefore accepting the native call.
acceptNativeCallWithReadiness(callId)requires astring, but this branch only checksif (callId)once and can still proceed with a missing value when the gate is true. IfVoipPayload.callIdis optional forVoipAcceptSucceeded, the type-system guard is bypassed by this non-null assertion andundefinedflows into the native-call accept path.🛡️ Proposed guard
+ if (!callId) { + mediaCallLogger.log(`${TAG} VoipAcceptSucceeded: missing callId`); + return; + } mediaCallLogger.debug(`${TAG} VoipAcceptSucceeded:`, data); NativeVoipModule.clearInitialEvents(); useCallStore.getState().setNativeAcceptedCallId(data.callId); if (data.host && isVoipIncomingHostCurrentWorkspace(data.host, adapters.getActiveServerUrl)) { - mediaSessionInstance.acceptNativeCallWithReadiness(data.callId!).catch(error => { + mediaSessionInstance.acceptNativeCallWithReadiness(callId).catch(error => {🤖 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/MediaCallEvents.ts` around lines 90 - 101, Update the VoipAcceptSucceeded handling before the isVoipIncomingHostCurrentWorkspace branch to require a defined callId before invoking acceptNativeCallWithReadiness; remove the non-null assertion and preserve the existing behavior for valid call IDs.
🧹 Nitpick comments (10)
app/sagas/state.js (1)
40-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an
asynchelper for the non-blocking probe.Replace the
.then().catch().finally()chain with a smallasynchelper usingtry/catch/finally; invoke it with an explicitly handled promise to retain fire-and-forget behavior safely. As per coding guidelines, “Prefer async/await over .then() chains” and “Use explicit error handling with try/catch blocks for async operations.”🤖 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/state.js` around lines 40 - 50, Replace the promise chain around ddp.probe in the socket probing flow with a small async helper that uses try/catch/finally, logs probe or reopen errors, and resets isProbingSocket in finally. Invoke the helper as an explicitly handled fire-and-forget promise so the existing non-blocking behavior is preserved.Source: Coding guidelines
app/sagas/__tests__/state.test.ts (1)
52-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to test helpers.
setupStore,makeDdp, andsetupReadyStorerely on inferred return types. Declare their return contracts (and a mock DDP interface if needed) to preserve strict TypeScript guarantees. As per coding guidelines, “add explicit type annotations to function parameters and return types.”Also applies to: 82-90
🤖 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__/state.test.ts` around lines 52 - 69, Add explicit return type annotations to the test helpers setupStore, makeDdp, and setupReadyStore, defining a suitable mock DDP interface for makeDdp if needed. Preserve their existing behavior and ensure the annotations accurately describe the returned Redux store and DDP mock contracts.Source: Coding guidelines
app/lib/services/waitForLoginReady.ts (2)
25-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onAbortnever detaches the listener.
finish()clears the timer and unsubscribes from the store but leaves the abort listener attached to the caller'sAbortSignal. It's harmless today (finishis idempotent and the controllers inacceptNativeCallare per-call), but if a long-lived signal is ever passed in, listeners accumulate. Returning a disposer keeps the cleanup symmetric.♻️ Proposed refactor
-function onAbort(signal: AbortSignal | undefined, callback: () => void): void { +function onAbort(signal: AbortSignal | undefined, callback: () => void): () => void { if (!signal) { - return; + return () => {}; } if (signal.aborted) { callback(); - return; + return () => {}; } if ('addEventListener' in signal) { signal.addEventListener('abort', callback, { once: true }); + return () => signal.removeEventListener('abort', callback); } else { // Fallback for older runtimes where AbortSignal only exposes onabort. const legacy = signal as unknown as { onabort: (() => void) | null }; legacy.onabort = callback; + return () => { + legacy.onabort = null; + }; } }Then call the returned disposer inside
finish.🤖 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/waitForLoginReady.ts` around lines 25 - 40, Update onAbort to return a disposer that removes the registered abort listener, including the legacy onabort assignment, while preserving immediate callback behavior for already-aborted signals. Capture this disposer where onAbort is called and invoke it from finish alongside timer and store cleanup.
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the
ddpshape into an exported interface.The inline object type is duplicated conceptually by callers (
getSocketStalenesstakesany). An exported interface would letconnect.tstype its parameter too.As per coding guidelines, "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/services/waitForLoginReady.ts` around lines 3 - 8, Extract the inline ddp object shape from classifySocketHealth into an exported interface, preserving lastPing, optional pingInterval, and optional config.ping fields. Update classifySocketHealth and related callers such as getSocketStaleness and connect.ts to use this interface instead of inline or any typing.Source: Coding guidelines
app/lib/services/voip/MediaCallEvents.ios.test.ts (1)
288-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the old path is no longer taken.
Adding
expect(mediaSessionInstance.applyRestStateSignals).not.toHaveBeenCalled()alongside this would pin the contract that cold-start acceptance goes exclusively through the readiness gate, rather than only proving the new call happened.🤖 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/MediaCallEvents.ios.test.ts` at line 288, Add a negative assertion in the cold-start acceptance test alongside the existing acceptNativeCallWithReadiness expectation, verifying mediaSessionInstance.applyRestStateSignals was not called so acceptance uses only the readiness-gated path.app/lib/services/__tests__/waitForLoginReady.socketHealth.test.ts (1)
30-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the exact boundary cases.
The suite covers well inside each band but not the
>comparisons themselves.age === pingIntervalmust behealthyandage === 2 * pingIntervalmust beprobe; those are the cases a future>→>=edit would silently break.💚 Proposed additional tests
+ it('returns healthy when age equals pingInterval', () => { + expect(classifySocketHealth(makeDdp({ lastPing: now - 10000 }))).toBe('healthy'); + }); + + it('returns probe when age equals 2 * pingInterval', () => { + expect(classifySocketHealth(makeDdp({ lastPing: now - 20000 }))).toBe('probe'); + });🤖 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/__tests__/waitForLoginReady.socketHealth.test.ts` around lines 30 - 53, Add exact-boundary tests in the classifySocketHealth suite confirming age equal to pingInterval returns healthy and age equal to 2 * pingInterval returns probe. Use the existing makeDdp and now fixtures with the configured interval, preserving the current behavior for values inside each range.patches/@rocket.chat+sdk+1.3.3-mobile.patch (2)
295-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
ddp?: anyonRocketChatClientloses type safety across the boundary.
getSocketStaleness(ddp: any)and the readiness gate both consume this field, soanypropagates into the app code and hides thereopenNow/probe/lastPingcontract thatconnect.tsdefensively feature-detects. Typing it asDDPDriver(or a narrow interface) would let those runtimetypeof x === 'function'checks be justified by the type rather than by guesswork.Note this is a patch against a vendored SDK, so the effort/benefit tradeoff is yours to weigh.
🤖 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 `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch around lines 295 - 307, Replace the any-typed ddp field on RocketChatClient with DDPDriver or a narrow interface exposing the reopenNow, probe, and lastPing members consumed by getSocketStaleness and the readiness gate. Keep the DDPDriver assignment in the Protocols.DDP initialization path and ensure those consumers use the typed contract while retaining their runtime feature checks.
69-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
open()is now anasyncexecutor wrapper that no longer usesms.The
ms = this.config.reopenparameter is retained in the signature but unreferenced in the new body, and thenew Promise(async ...)executor is redundant now that the body just awaitscreateConnection(). Simplifying to a plainasyncmethod (keeping the parameter only if external callers pass it) removes the antipattern.♻️ Proposed simplification
- open = (ms: number = this.config.reopen) => { - return new Promise(async (resolve, reject) => { - if (this.connected) { - return resolve() - } - - if (this.reopenPromise) { - return this.reopenPromise.then(() => resolve(this.connection)).catch(reject) - } - - try { - await this.createConnection() - resolve(this.connection) - } catch (err) { - reject(err) - } - }) - } + open = async () => { + if (this.connected) { + return + } + + if (this.reopenPromise) { + await this.reopenPromise + return this.connection + } + + await this.createConnection() + return this.connection + }Verify no caller relies on the
msargument.#!/bin/bash rg -nP --type=ts -C2 '\.open\s*\(' app | rg -v 'openURL|openModal' rg -nP -C3 'open\s*=\s*\(ms' patches/🤖 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 `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch around lines 69 - 86, Update the open method to use a plain async implementation instead of wrapping await in new Promise, preserving the existing connected and reopenPromise behavior while returning the connection after createConnection succeeds and propagating failures naturally. Verify callers do not rely on the ms parameter; remove it if unused, otherwise retain it for compatibility without leaving it misleadingly unused.app/lib/services/connect.ts (1)
446-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the
ddpparameter and share the health/staleness constants.
ddp: anydisables checking on the exact field access this function guards (reopenNow,probe,lastPing), and both this function andclassifySocketHealthreturn bare string unions that callers (app/sagas/state.js) compare against literals.As per coding guidelines, "add explicit type annotations to function parameters and return types" and "Use enums for sets of related constants rather than magic strings or numbers".
♻️ Proposed refactor sketch
-export function getSocketStaleness(ddp: any): 'stale' | 'gray' | 'fresh' { +interface IDdpSocketHealthSource { + reopenNow?: unknown; + probe?: unknown; + lastPing?: number | null; + pingInterval?: number; + config?: { ping?: number }; +} + +export function getSocketStaleness(ddp?: IDdpSocketHealthSource): SocketStaleness { if (!ddp || typeof ddp.reopenNow !== 'function' || typeof ddp.probe !== 'function' || ddp.lastPing == null) { - return 'fresh'; + return SocketStaleness.Fresh; }🤖 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 446 - 460, Replace the any-typed ddp parameter in getSocketStaleness and classifySocketHealth with a shared type that declares reopenNow, probe, and lastPing, and use explicit return typing throughout. Define shared enums or equivalent constants for socket health and staleness values, then update both functions and callers such as the state saga to compare against those shared symbols instead of bare string literals.Source: Coding guidelines
app/lib/services/ddpSocket.test.ts (1)
147-160: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIn-flight sends reject with
undefined.The assertion documents that
reopenNow()'sthis.emit('disconnected')rejects pendingsend()promises with no reason at all. Anycatch (err)upstream getsundefined, so logs will show an empty error anderr.messageaccess would throw. Emitting anError('socket reopened')payload (and asserting on it here) makes these rejections diagnosable.🤖 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/ddpSocket.test.ts` around lines 147 - 160, Update the socket reopen flow used by reopenNow() so the disconnected event rejects pending send() promises with an Error describing that the socket was reopened, rather than undefined. Adjust the in-flight send test to assert the rejection contains that Error payload while preserving the existing disconnected emission and reconnection sequence.
🤖 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/services/ddpSocket.test.ts`:
- Around line 288-311: The test around waitForNotifyUserMediaSubs must cover the
intermediate state where only one media subscription exists. Insert either
media-signal or media-calls first, advance the fake timer, and assert the
promise remains pending; then add the second subscription, advance polling
again, and retain the final expectation that the promise resolves true and
subscribe is called twice.
- Around line 180-204: Add an afterEach hook to the describe block containing
the reopenNow timeout test that restores real timers, and remove the trailing
jest.useRealTimers() call from the test body so cleanup runs even when
assertions fail.
In `@app/lib/services/voip/acceptNativeCall.test.ts`:
- Around line 12-14: Remove the redundant jest.mock registration for
../waitForLoginReady in acceptNativeCall.test.ts, keeping the later mock that
merges requireActual and preserves classifySocketHealth. Ensure only the
effective mock definition remains.
In `@app/lib/services/voip/acceptNativeCall.ts`:
- Around line 83-128: Update the try/finally flow surrounding the native call
readiness sequence so any thrown or rejected readiness operation is caught and
passed to handleFailure(callId, mediaSession) before cleanup runs. Preserve the
existing early returns and ensure cleanup remains in finally, with successful
call handling unchanged.
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Around line 258-260: Update the resubscribe callback and the underlying
DDPDriver.subscribe chain so the existing sub.id is used as the subscription
identifier rather than appended to stream arguments. Either add and propagate an
explicit id parameter through DDPDriver.subscribe, or invoke this.ddp.subscribe
directly with that id, while preserving the existing topic, event, and
useCollection behavior.
- Around line 263-282: Update waitForNotifyUserMediaSubs in
patches/@rocket.chat+sdk+1.3.3-mobile.patch at lines 263-282 so attempt requires
every name in names, including both media-signal and media-calls, before calling
resubscribe, and add an in-flight guard to prevent overlapping resubscribe
calls. Update app/lib/services/ddpSocket.test.ts at lines 288-311 with a case
that adds only media-signal, verifies the promise remains unresolved after
polling, then adds media-calls and verifies it resolves true.
- Around line 128-142: Declare the timeout variable before defining or
registering cleanup in the reopen flow, using a hoisted mutable declaration so
synchronous open events cannot access it in the temporal dead zone. Update
cleanup to clear that variable, and optionally replace the inline
10000-millisecond deadline with a named module-level constant.
---
Outside diff comments:
In `@app/lib/services/connect.ts`:
- Around line 145-151: Update the `'connected'` handler around
`waitForLoginReady` so it checks the returned readiness boolean before calling
`mediaSessionInstance.drainPendingHangups()`. Drain pending hangups only when
login is ready; otherwise leave them queued for the next connection attempt,
while preserving the existing error logging.
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Around line 90-101: Update the VoipAcceptSucceeded handling before the
isVoipIncomingHostCurrentWorkspace branch to require a defined callId before
invoking acceptNativeCallWithReadiness; remove the non-null assertion and
preserve the existing behavior for valid call IDs.
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Around line 40-62: Update createConnection so every connection attempt has a
finite connect deadline and its returned promise rejects when the socket remains
in CONNECTING beyond that deadline. Clear the deadline when onOpen, onerror, or
onclose settles the attempt, and ensure timeout cleanup does not affect a newer
connection.
---
Nitpick comments:
In `@app/lib/services/__tests__/waitForLoginReady.socketHealth.test.ts`:
- Around line 30-53: Add exact-boundary tests in the classifySocketHealth suite
confirming age equal to pingInterval returns healthy and age equal to 2 *
pingInterval returns probe. Use the existing makeDdp and now fixtures with the
configured interval, preserving the current behavior for values inside each
range.
In `@app/lib/services/connect.ts`:
- Around line 446-460: Replace the any-typed ddp parameter in getSocketStaleness
and classifySocketHealth with a shared type that declares reopenNow, probe, and
lastPing, and use explicit return typing throughout. Define shared enums or
equivalent constants for socket health and staleness values, then update both
functions and callers such as the state saga to compare against those shared
symbols instead of bare string literals.
In `@app/lib/services/ddpSocket.test.ts`:
- Around line 147-160: Update the socket reopen flow used by reopenNow() so the
disconnected event rejects pending send() promises with an Error describing that
the socket was reopened, rather than undefined. Adjust the in-flight send test
to assert the rejection contains that Error payload while preserving the
existing disconnected emission and reconnection sequence.
In `@app/lib/services/voip/MediaCallEvents.ios.test.ts`:
- Line 288: Add a negative assertion in the cold-start acceptance test alongside
the existing acceptNativeCallWithReadiness expectation, verifying
mediaSessionInstance.applyRestStateSignals was not called so acceptance uses
only the readiness-gated path.
In `@app/lib/services/waitForLoginReady.ts`:
- Around line 25-40: Update onAbort to return a disposer that removes the
registered abort listener, including the legacy onabort assignment, while
preserving immediate callback behavior for already-aborted signals. Capture this
disposer where onAbort is called and invoke it from finish alongside timer and
store cleanup.
- Around line 3-8: Extract the inline ddp object shape from classifySocketHealth
into an exported interface, preserving lastPing, optional pingInterval, and
optional config.ping fields. Update classifySocketHealth and related callers
such as getSocketStaleness and connect.ts to use this interface instead of
inline or any typing.
In `@app/sagas/__tests__/state.test.ts`:
- Around line 52-69: Add explicit return type annotations to the test helpers
setupStore, makeDdp, and setupReadyStore, defining a suitable mock DDP interface
for makeDdp if needed. Preserve their existing behavior and ensure the
annotations accurately describe the returned Redux store and DDP mock contracts.
In `@app/sagas/state.js`:
- Around line 40-50: Replace the promise chain around ddp.probe in the socket
probing flow with a small async helper that uses try/catch/finally, logs probe
or reopen errors, and resets isProbingSocket in finally. Invoke the helper as an
explicitly handled fire-and-forget promise so the existing non-blocking behavior
is preserved.
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Around line 295-307: Replace the any-typed ddp field on RocketChatClient with
DDPDriver or a narrow interface exposing the reopenNow, probe, and lastPing
members consumed by getSocketStaleness and the readiness gate. Keep the
DDPDriver assignment in the Protocols.DDP initialization path and ensure those
consumers use the typed contract while retaining their runtime feature checks.
- Around line 69-86: Update the open method to use a plain async implementation
instead of wrapping await in new Promise, preserving the existing connected and
reopenPromise behavior while returning the connection after createConnection
succeeds and propagating failures naturally. Verify callers do not rely on the
ms parameter; remove it if unused, otherwise retain it for compatibility without
leaving it misleadingly unused.
🪄 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: 6ee933f6-76f4-4484-80fd-95ec5457ad42
📒 Files selected for processing (17)
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/__tests__/waitForLoginReady.socketHealth.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.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/acceptNativeCall.tsapp/lib/services/waitForLoginReady.tsapp/sagas/__tests__/state.test.tsapp/sagas/login.jsapp/sagas/state.jsjest.config.jspatches/@rocket.chat+sdk+1.3.3-mobile.patch
📜 Review details
🧰 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/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/waitForLoginReady.tsapp/sagas/login.jsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/__tests__/waitForLoginReady.socketHealth.test.tsjest.config.jsapp/sagas/state.jsapp/lib/services/ddpSocket.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/sagas/__tests__/state.test.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/acceptNativeCall.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.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/MediaCallEvents.ios.test.tsapp/lib/services/waitForLoginReady.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/__tests__/waitForLoginReady.socketHealth.test.tsapp/lib/services/ddpSocket.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/sagas/__tests__/state.test.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/acceptNativeCall.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.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/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/waitForLoginReady.tsapp/sagas/login.jsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/__tests__/waitForLoginReady.socketHealth.test.tsjest.config.jsapp/sagas/state.jsapp/lib/services/ddpSocket.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/sagas/__tests__/state.test.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/acceptNativeCall.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.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/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/acceptNativeCall.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.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/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/__tests__/waitForLoginReady.socketHealth.test.tsapp/lib/services/ddpSocket.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/sagas/__tests__/state.test.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/MediaSessionInstance.test.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__/state.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
🧠 Learnings (5)
📚 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/MediaCallEvents.ios.test.tsapp/lib/services/waitForLoginReady.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/__tests__/waitForLoginReady.socketHealth.test.tsapp/lib/services/ddpSocket.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/sagas/__tests__/state.test.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/acceptNativeCall.tsapp/lib/services/connect.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.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/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/__tests__/waitForLoginReady.socketHealth.test.tsapp/lib/services/ddpSocket.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/sagas/__tests__/state.test.tsapp/lib/services/voip/acceptNativeCall.test.tsapp/lib/services/voip/MediaSessionInstance.test.ts
📚 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/login.jsapp/sagas/state.js
📚 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/NewMediaCall/VoipCallLifecycle.integration.test.tsx
📚 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 (21)
app/sagas/state.js (1)
7-12: LGTM!app/sagas/__tests__/state.test.ts (2)
1-48: LGTM!Also applies to: 92-256
71-71: 📐 Maintainability & Code QualityRun this Jest suite with
TZ=UTC.Please verify the suite through the repository’s Jest command with
TZ=UTCset. As per coding guidelines, “Run Jest tests withTZ=UTCto ensure deterministic timezone-dependent test behavior.”Source: Coding guidelines
app/sagas/login.js (1)
45-45: LGTM!Also applies to: 264-266
app/lib/services/waitForLoginReady.ts (2)
19-23: LGTM!
42-70: LGTM!patches/@rocket.chat+sdk+1.3.3-mobile.patch (3)
96-103: LGTM!
152-188: LGTM!
193-201: 🔒 Security & PrivacyVerify the
'disconnect'→'disconnected'rename covers every listener.
send()now unregisters'disconnected', andreopenNow()emits'disconnected'. If any other code still registers or emits the old'disconnect'name, those listeners will never fire (or never be removed).#!/bin/bash rg -nP -C3 "'disconnect'|\"disconnect\"" patches/ app/lib/services fd -t f -e ts . node_modules/@rocket.chat/sdk/lib --exec rg -nP -C2 "emit\('disconnect|once\('disconnect|on\('disconnect|off\('disconnect" {}app/lib/services/connect.ts (1)
560-561: LGTM!app/lib/services/ddpSocket.test.ts (2)
67-114: LGTM!
206-239: LGTM!jest.config.js (1)
4-6: LGTM!app/lib/services/voip/MediaCallEvents.ios.test.ts (1)
83-84: LGTM!app/lib/services/voip/acceptNativeCall.ts (1)
1-67: LGTM!app/lib/services/voip/acceptNativeCall.test.ts (1)
79-286: LGTM!app/lib/services/voip/MediaSessionInstance.ts (1)
39-39: LGTM!Also applies to: 52-71, 89-96, 111-129, 145-145
app/lib/services/voip/MediaSessionInstance.test.ts (1)
18-21: LGTM!Also applies to: 67-81, 260-291, 508-691
app/lib/services/voip/MediaCallEvents.ts (1)
274-287: LGTM!app/lib/services/voip/MediaCallEvents.test.ts (1)
64-65: LGTM!Also applies to: 148-160
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx (1)
27-27: LGTM!Also applies to: 161-169, 582-584, 609-609
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109434 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTYd7YygJv4-75T0fYN1Dzj6it9XNwvCvwhzca2fgi0iUVvL5P6F6v8xuruVS3eRdhIV_sPKBLP9BOK5bxp |
|
iOS Build Available Rocket.Chat 4.75.0.109435 |
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.
GitHub Actions
Error
This reverts commit da389be.
)" This reverts commit cd6f2a8.
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.
The base branch was rebased, so its 13 commits (the five reverts plus follow-ups) arrived with new SHAs while dry-badger already carried them as identical patches. Every conflict is that duplicate content, resolved to dry-badger's side, which keeps the socket health work layered on top. Only #7509 (ActionSheet hide guard) and #7510 (UserPreferences.getBool) are new here. jest.config.js keeps the @rocket.chat/sdk and tiny-events transform entries that ddpSocket.test.ts needs; the rebased base dropped them along with the test file it deleted.
GitHub Actions
Error
GitHub Actions
Error
#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.
GitHub Actions
Waiting
GitHub Actions
Waiting
2 participants
Proposed changes
With the device locked and the app suspended ≥30s, accepting an incoming VoIP call from the lock screen died at 10s with
timeout-remote-sdp. iOS freezes the TCP socket silently; the SDK keeps readingconnected === truethrough its 40s tolerance window,send()hangs forever awaiting'open', and the only reconnection trigger (the foreground saga) never fires while the device stays locked. The native accept path had no connection gate at all, so everything needed for audio — inboundremote-sdp, outboundlocal-sdp/local-state— rode a dead socket while the REST-based native accept succeeded.reopenNow()forces a single shared reconnect — clears the pending reopen timer, tears down the orphan socket, rejects in-flightsend()promises via a real'disconnected'emit (previously nothing ever emitted it, so sends hung forever), and preservesSocket.subscriptionsso DDP login'ssubscribeAll()restores every stream. Forced reopens are serialized against concurrentopen(). No forceReopen, no subscription wipe, no synthetic close event —open()'s synchronous'connecting'emit flips redux honestly.probe(): bounded raw-socket liveness check for the gray zone (20s < lastPing age < 40s), so healthy-but-quiet sockets don't get force-reconnected.acceptNativeCallWithReadiness): every native accept/replay path (warm accept, cold start, init replay, live-signal auto-answer) funnels through one gate — classify socket health, reopen if stale, wait for login readiness and media-signal subscription ack (server does no queueing; accepting before the ack loses post-accept signaling), then replay state signals and answer. 8s cap; on failure the native call is terminated and a hangup queued. Concurrent accepts for the same call abort the older gate without terminating the call.Issue(s)
https://rocketchat.atlassian.net/browse/SUP-1078
How to test or reproduce
Automated:
TZ=UTC pnpm test(2045 tests — new coverage inddpSocket.test.ts,acceptNativeCall.test.ts,state.test.ts,waitForLoginReady.socketHealth.test.ts,VoipCallLifecycle.integration.test.tsx).Manual: items 1–3 need physical devices (CallKit / ConnectionService lock-screen UI and WebRTC audio do not work on a simulator). Items 1 and 2 need a second account on a second device to place the call.
timeout-remote-sdp, no silent call.Before this PR, item 1 gave silence and then a drop at 10s with
timeout-remote-sdp.Screenshots
N/A — connection-layer change, no UI.
Types of changes
Checklist
Further comments
Deliberately out of scope — follow-ups to raise after this PR:
Session.requestInputTrackUpdatedrops track updates with no requeue; can independently produce the same 10stimeout-remote-sdpon a healthy socket. Needs a patch-package requeue + sequentialawait processSignal.waiting-for-offer, closing the sub-ack vs first-offer race on slow networks.getInitialMediaCallEventsskipsappInit, so replay no-ops and no login ever runs.Summary by CodeRabbit