fix(voip): force reconnect on warm-locked accept when AppState != active#7296
fix(voip): force reconnect on warm-locked accept when AppState != active#7296diegolmello wants to merge 5 commits into
Conversation
When the device stays locked through CallKit accept, the foreground saga never runs (AppState never reaches 'active'), so nothing nudges the SDK to reopen its DDP socket if it went stale during background suspension. The local accept resolves and CallKit shows "connected", but answer SDP and ICE candidates can never reach the peer because send() blocks waiting on a socket that won't reopen on its own. Call checkAndReopen() before each signal send. It is a no-op when the socket is alive; otherwise it triggers the existing reconnect → relogin → resubscribe chain so signaling can flow over a fresh DDP session.
Naive checkAndReopen no-ops when SDK alive() still returns true, but iOS TCP can zombie inside that 20s window. Pre-emptively dispatch disconnect, close the socket, then checkAndReopen on warm-locked accept; foreground accept skips this so healthy sessions are unaffected. setSendSignalFn now gates methodCall on ddp.loggedIn (not just open) with 8s timeout. send() only awaits open but stream-notify-user requires this.userId server-side, so sending before relogin = silent drop. Adds voip-debug.log instrumentation: accept-marked, login-ready, first-signal-sent timings.
WalkthroughAdds a persistent VoIP debug logger and reconnect-timing helper, instruments push and VoIP flows with structured debug traces, extracts/restores media-call state signals, introduces a forced reconnect path for background-accepted VoIP calls, and enables iOS file-sharing/open-in-place. ChangesVoIP logging, reconnect timing, and integration
Sequence Diagram(s)sequenceDiagram
participant Native as Native OS (VoIP push)
participant JS as JS runtime
participant SDK as sdk.current
participant DDP as DDP/socket
participant REST as REST API
Native->>JS: VoIP push (accept)
JS->>JS: voipDebugLog accept entry
JS->>JS: check AppState
alt AppState != active
JS->>JS: markVoipReconnectStart(forced=true)
JS->>store: dispatch disconnect
JS->>DDP: best-effort close
JS->>SDK: checkAndReopen()
SDK->>DDP: reopen/login
DDP-->>JS: login event
JS->>JS: logVoipLoginElapsed()
end
JS->>REST: fetch media-calls.stateSignals
REST-->>JS: signals list
JS->>JS: process signals (applyRestStateSignals)
JS->>DDP: setSendSignalFn -> await login (8s) -> stream-notify-user
JS->>JS: logVoipFirstSignalElapsed()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labelstype: bug 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 4/8 reviews remaining, refill in 22 minutes and 42 seconds.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: 4
🤖 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/voip/MediaCallEvents.ts`:
- Around line 116-134: The async immediately-invoked function expression (the
anonymous async IIFE that calls sdk.current, closes ddpSocket, calls
sdk.current?.checkAndReopen and sets ddpSocket.once to logVoipLoginElapsed) must
have its returned promise handled; append a .catch(...) to the IIFE to log any
unhandled rejection (use voipDebugLog and include the error string) so the
floating promise is not left unhandled per the codebase ESLint rules.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 108-138: The pattern inside mediaSessionStore.setSendSignalFn
currently uses a void-wrapped async IIFE to fire-and-forget (void (async () => {
... })()), which triggers the ESLint no-void rule; change this to call the async
IIFE and attach a .catch(...) to the returned promise instead (e.g., (async
function sendSignalWorker() { ... })().catch(err => {
voipDebugLog('sendSignal','unhandled',String(err)); })), keeping the same
internal behavior (calls to sdk.current?.checkAndReopen, awaiting ddpSocket
login, sdk.methodCall, and logVoipFirstSignalElapsed) and using the same
voipDebugLog/error handling inside the try/catch blocks so any unhandled
rejection is logged via .catch rather than silenced with void.
In `@app/lib/services/voip/voipDebugLogger.ts`:
- Line 37: Replace the indirect console call `(globalThis as
any).console['log'](line)` in voipDebugLogger.ts with a direct call to the
global console (use `console.log(line)`), removing the globalThis cast and
bracket notation to satisfy ESLint and React Native environment expectations;
locate the call site where the module logs `line` and update it to use the
direct console API.
In `@ios/RocketChatRN/Info.plist`:
- Around line 98-101: The Info.plist currently enables UIFileSharingEnabled and
LSSupportsOpeningDocumentsInPlace for all builds; change the main Info.plist to
set UIFileSharingEnabled and LSSupportsOpeningDocumentsInPlace to false (or
remove them) and move the true settings into a debug-only plist or configuration
so only debug/test builds expose the Documents folder; ensure the debug plist or
build configuration overrides these keys (UIFileSharingEnabled,
LSSupportsOpeningDocumentsInPlace) for Debug targets rather than leaving them
true in the production Info.plist.
🪄 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
Run ID: ba6fe065-bf87-4af1-8aaa-11e2678a91b5
📒 Files selected for processing (7)
app/lib/notifications/push.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/voipDebugLogger.tsapp/lib/services/voip/voipReconnectTiming.tsios/RocketChatRN/Info.plist
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/voipReconnectTiming.tsapp/lib/notifications/push.tsapp/lib/services/voip/voipDebugLogger.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/restApi.tsapp/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 numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/voipReconnectTiming.tsapp/lib/notifications/push.tsapp/lib/services/voip/voipDebugLogger.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/voipReconnectTiming.tsapp/lib/notifications/push.tsapp/lib/services/voip/voipDebugLogger.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/voipReconnectTiming.tsapp/lib/services/voip/voipDebugLogger.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/restApi.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use REST API via fetch in app/lib/services/restApi.ts for HTTP requests
Files:
app/lib/services/restApi.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/voipReconnectTiming.tsapp/lib/notifications/push.tsapp/lib/services/voip/voipDebugLogger.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaSessionInstance.ts
🪛 ESLint
app/lib/services/voip/voipDebugLogger.ts
[error] 37-37: 'globalThis' is not defined.
(no-undef)
[error] 37-37: ["log"] is better written in dot notation.
(dot-notation)
app/lib/services/voip/MediaSessionInstance.ts
[error] 110-137: Expected 'undefined' and instead saw 'void'.
(no-void)
🔇 Additional comments (8)
app/lib/services/voip/voipReconnectTiming.ts (1)
1-28: LGTM!Clean timing utility with proper guards for null state and one-time logging. The module correctly tracks reconnect timing state and provides clear functions for marking, logging, and clearing timing data.
app/lib/services/voip/voipDebugLogger.ts (1)
18-33: LGTM!The serialized write chain pattern is a solid approach to prevent file corruption from concurrent writes. Error handling preserves chain continuity while tracking the last error.
app/lib/services/restApi.ts (1)
1090-1158: LGTM!The debug instrumentation is comprehensive and non-intrusive. The error serialization pattern at line 1155 safely captures error properties including non-enumerable ones. The logging points cover entry, bail conditions, request details, success, and failure paths.
app/lib/notifications/push.ts (1)
190-210: LGTM!Promise handling correctly uses
.then()/.catch()chains rather thanvoidexpressions, complying with the codebase'sno-void: errorESLint rule. The debug logging covers both initial registration and token refresh paths.app/lib/services/voip/MediaSessionInstance.ts (2)
89-106: LGTM!The
initmethod properly chains async operations and the REST state signals replay is well-structured with appropriate error logging.
207-252: LGTM!The
answerCallmethod has comprehensive error handling with proper cleanup paths (terminating native call, resetting state) when accept fails or call is not found. Debug logging covers all branches.app/lib/services/voip/MediaCallEvents.ts (2)
160-170: LGTM!The VoIP push token registration correctly uses
.then()/.catch()for promise handling with appropriate debug logging on both success and failure paths.
273-350: LGTM!The
getInitialMediaCallEventsfunction has proper async/await structure with a comprehensive try/catch wrapper. Debug logging at entry and for initial events provides good visibility into cold-start behavior.
| (async () => { | ||
| try { | ||
| const driver = await (sdk.current as any)?.socket; | ||
| const ddpSocket = driver?.ddp; | ||
| try { | ||
| ddpSocket?.connection?.close(); | ||
| voipDebugLog('VoipAcceptSucceeded', 'forced ws close'); | ||
| } catch (e) { | ||
| voipDebugLog('VoipAcceptSucceeded', 'forced ws close threw', String(e)); | ||
| } | ||
| sdk.current?.checkAndReopen?.(); | ||
| voipDebugLog('VoipAcceptSucceeded', 'checkAndReopen kicked'); | ||
| ddpSocket?.once?.('login', () => { | ||
| logVoipLoginElapsed(); | ||
| }); | ||
| } catch (e) { | ||
| voipDebugLog('VoipAcceptSucceeded', 'force reconnect threw', String(e)); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Attach .catch() to the async IIFE for consistent promise handling.
While the body has internal try/catch, the floating async IIFE should attach .catch() per codebase ESLint conventions. This also catches any edge cases where the async boundary itself might reject.
Proposed fix
if (forceReconnect) {
// connectedListener (connect.ts:113) early-returns if redux meteor.connected is still true.
// Force redux flip so loginRequest fires on the new 'connected' event.
store.dispatch(disconnectAction());
(async () => {
try {
const driver = await (sdk.current as any)?.socket;
const ddpSocket = driver?.ddp;
try {
ddpSocket?.connection?.close();
voipDebugLog('VoipAcceptSucceeded', 'forced ws close');
} catch (e) {
voipDebugLog('VoipAcceptSucceeded', 'forced ws close threw', String(e));
}
sdk.current?.checkAndReopen?.();
voipDebugLog('VoipAcceptSucceeded', 'checkAndReopen kicked');
ddpSocket?.once?.('login', () => {
logVoipLoginElapsed();
});
} catch (e) {
voipDebugLog('VoipAcceptSucceeded', 'force reconnect threw', String(e));
}
- })();
+ })().catch(e => voipDebugLog('VoipAcceptSucceeded', 'reconnect IIFE rejected', String(e)));
}Based on learnings: "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"), handle the promise explicitly by attaching .catch(...)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (async () => { | |
| try { | |
| const driver = await (sdk.current as any)?.socket; | |
| const ddpSocket = driver?.ddp; | |
| try { | |
| ddpSocket?.connection?.close(); | |
| voipDebugLog('VoipAcceptSucceeded', 'forced ws close'); | |
| } catch (e) { | |
| voipDebugLog('VoipAcceptSucceeded', 'forced ws close threw', String(e)); | |
| } | |
| sdk.current?.checkAndReopen?.(); | |
| voipDebugLog('VoipAcceptSucceeded', 'checkAndReopen kicked'); | |
| ddpSocket?.once?.('login', () => { | |
| logVoipLoginElapsed(); | |
| }); | |
| } catch (e) { | |
| voipDebugLog('VoipAcceptSucceeded', 'force reconnect threw', String(e)); | |
| } | |
| })(); | |
| if (forceReconnect) { | |
| // connectedListener (connect.ts:113) early-returns if redux meteor.connected is still true. | |
| // Force redux flip so loginRequest fires on the new 'connected' event. | |
| store.dispatch(disconnectAction()); | |
| (async () => { | |
| try { | |
| const driver = await (sdk.current as any)?.socket; | |
| const ddpSocket = driver?.ddp; | |
| try { | |
| ddpSocket?.connection?.close(); | |
| voipDebugLog('VoipAcceptSucceeded', 'forced ws close'); | |
| } catch (e) { | |
| voipDebugLog('VoipAcceptSucceeded', 'forced ws close threw', String(e)); | |
| } | |
| sdk.current?.checkAndReopen?.(); | |
| voipDebugLog('VoipAcceptSucceeded', 'checkAndReopen kicked'); | |
| ddpSocket?.once?.('login', () => { | |
| logVoipLoginElapsed(); | |
| }); | |
| } catch (e) { | |
| voipDebugLog('VoipAcceptSucceeded', 'force reconnect threw', String(e)); | |
| } | |
| })().catch(e => voipDebugLog('VoipAcceptSucceeded', 'reconnect IIFE rejected', String(e))); | |
| } |
🤖 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 116 - 134, The async
immediately-invoked function expression (the anonymous async IIFE that calls
sdk.current, closes ddpSocket, calls sdk.current?.checkAndReopen and sets
ddpSocket.once to logVoipLoginElapsed) must have its returned promise handled;
append a .catch(...) to the IIFE to log any unhandled rejection (use
voipDebugLog and include the error string) so the floating promise is not left
unhandled per the codebase ESLint rules.
| <key>UIFileSharingEnabled</key> | ||
| <true/> | ||
| <key>LSSupportsOpeningDocumentsInPlace</key> | ||
| <true/> |
There was a problem hiding this comment.
Restrict file-sharing/document-in-place to debug builds only
Setting both keys to true in the main app plist broadens data exposure (Documents visible over Finder/iTunes and editable in place by other apps). For a messaging app, this is a production privacy/security posture regression unless strictly required. Please move this behavior to a debug/test-only plist (or set these to false in release).
Suggested safe default in main plist
- <key>UIFileSharingEnabled</key>
- <true/>
- <key>LSSupportsOpeningDocumentsInPlace</key>
- <true/>
+ <key>UIFileSharingEnabled</key>
+ <false/>
+ <key>LSSupportsOpeningDocumentsInPlace</key>
+ <false/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <key>UIFileSharingEnabled</key> | |
| <true/> | |
| <key>LSSupportsOpeningDocumentsInPlace</key> | |
| <true/> | |
| <key>UIFileSharingEnabled</key> | |
| <false/> | |
| <key>LSSupportsOpeningDocumentsInPlace</key> | |
| <false/> |
🤖 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 `@ios/RocketChatRN/Info.plist` around lines 98 - 101, The Info.plist currently
enables UIFileSharingEnabled and LSSupportsOpeningDocumentsInPlace for all
builds; change the main Info.plist to set UIFileSharingEnabled and
LSSupportsOpeningDocumentsInPlace to false (or remove them) and move the true
settings into a debug-only plist or configuration so only debug/test builds
expose the Documents folder; ensure the debug plist or build configuration
overrides these keys (UIFileSharingEnabled, LSSupportsOpeningDocumentsInPlace)
for Debug targets rather than leaving them true in the production Info.plist.
…-warm-accept-appstate-gate # Conflicts: # app/lib/notifications/push.ts # app/lib/services/voip/MediaSessionInstance.ts
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
107-134:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
voidIIFE still present — ESLintno-void: erroris failing CI.The fix proposed in the previous review was marked as addressed but the
void (async () => {...})()pattern is still in the current code. Static analysis confirms the failure on line 107.🔧 Proposed fix
- void (async () => { + (async () => { try { try { sdk.current?.checkAndReopen?.(); } catch (e) { voipDebugLog('sendSignal', 'checkAndReopen threw', String(e)); } const driver = await (sdk.current as any)?.socket; const ddpSocket = driver?.ddp; if (ddpSocket && !ddpSocket.loggedIn) { voipDebugLog('sendSignal', 'awaiting login', { type: (signal as any)?.type }); await Promise.race([ new Promise<void>(resolve => ddpSocket.once('login', () => resolve())), new Promise<void>(resolve => setTimeout(resolve, 8000)) ]); if (!ddpSocket.loggedIn) { voipDebugLog('sendSignal', 'login wait timed out, dropping', { type: (signal as any)?.type }); return; } voipDebugLog('sendSignal', 'login ready, proceeding'); } await sdk.methodCall('stream-notify-user', `${userId}/media-calls`, JSON.stringify(signal)); voipDebugLog('sendSignal', 'methodCall resolved', { type: (signal as any)?.type }); logVoipFirstSignalElapsed(); } catch (e: unknown) { voipDebugLog('sendSignal', 'methodCall rejected', String(e)); } - })(); + })().catch(e => voipDebugLog('sendSignal', 'unhandled', String(e)));Based on learnings: "In this Rocket.Chat React Native codebase, the ESLint rule
no-void: erroris enforced. When you see a promise returned from an async call that is not awaited (a 'floating promise'), do not silence it with thevoid somePromise()pattern. Instead, handle the promise explicitly by attaching.catch(...)."🤖 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 107 - 134, The IIFE uses the disallowed "void (async () => {...})()" pattern causing the no-void ESLint error; replace the void-silenced IIFE with an explicit async call whose returned promise is handled (attach .catch) or move the body into a named async helper and call it, then call .catch(...) to log errors; keep the existing internals (sdk.current?.checkAndReopen, reading driver/ddpSocket, awaiting login, sdk.methodCall, voipDebugLog, logVoipFirstSignalElapsed) but remove the leading "void" and ensure any rejection is handled by the appended .catch that calls voipDebugLog('sendSignal', 'methodCall rejected', String(e)) (or similar) so no floating promise remains.
🤖 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.
Duplicate comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 107-134: The IIFE uses the disallowed "void (async () => {...})()"
pattern causing the no-void ESLint error; replace the void-silenced IIFE with an
explicit async call whose returned promise is handled (attach .catch) or move
the body into a named async helper and call it, then call .catch(...) to log
errors; keep the existing internals (sdk.current?.checkAndReopen, reading
driver/ddpSocket, awaiting login, sdk.methodCall, voipDebugLog,
logVoipFirstSignalElapsed) but remove the leading "void" and ensure any
rejection is handled by the appended .catch that calls
voipDebugLog('sendSignal', 'methodCall rejected', String(e)) (or similar) so no
floating promise remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a71d572-7f8b-42dd-9fec-330ea3a9d9aa
📒 Files selected for processing (2)
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.ts
📜 Review details
🧰 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/MediaCallEvents.tsapp/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 numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/MediaCallEvents.tsapp/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/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.ts
🪛 ESLint
app/lib/services/voip/MediaSessionInstance.ts
[error] 107-134: Expected 'undefined' and instead saw 'void'.
(no-void)
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/lib/services/voip/MediaSessionInstance.ts
[failure] 107-107:
Expected 'undefined' and instead saw 'void'
🔇 Additional comments (1)
app/lib/services/voip/MediaCallEvents.ts (1)
116-134: ⚡ Quick winAttach
.catch()to the fire-and-forget async IIFE.The reconnect IIFE's returned Promise is unhandled. While the internal
try/catchprevents observable rejections, attaching.catch()is the codebase convention for floating async IIFEs and was flagged in the prior review round without being resolved.♻️ Proposed fix
- })(); + })().catch(e => voipDebugLog('VoipAcceptSucceeded', 'reconnect IIFE rejected', String(e)));Based on learnings: "In this Rocket.Chat React Native codebase, the ESLint rule
no-void: erroris enforced. When you see a promise returned from an async call that is not awaited (a 'floating promise'), handle the promise explicitly by attaching.catch(...)."
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/services/voip/MediaCallEvents.ios.test.ts (2)
46-58: ⚡ Quick win
AppStateis missing from thereact-nativemock.The new production code gates reconnect logic on
AppState.currentState !== 'active'. The mock only providesPlatform,DeviceEventEmitter, andNativeEventEmitter, soAppStateresolves toundefinedin any test that reaches that branch. The existingVoipAcceptSucceededtest is safe today (it uses a foreign host and exits on the deep-link branch), but any test for the same-workspace warm-accept path will throwTypeError: Cannot read properties of undefined (reading 'currentState').🛠️ Proposed fix
jest.mock('react-native', () => ({ Platform: { OS: 'ios' }, + AppState: { currentState: 'active' }, DeviceEventEmitter: { addListener(eventType: string, listener: (payload: unknown) => void) { return mockMakeAddListener(mockNativeVoipListeners)(eventType, listener); } }, NativeEventEmitter: class { addListener(eventType: string, listener: (payload: unknown) => void) { return mockMakeAddListener(mockNativeVoipListeners)(eventType, listener); } } }));Tests that want to exercise the reconnect path can then override it per-test via
jest.spyOnor by mutating the mock'scurrentStatevalue.🤖 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` around lines 46 - 58, The react-native mock used in MediaCallEvents.ios.test.ts is missing AppState, causing AppState.currentState to be undefined; update the jest.mock block to include an AppState export with a currentState property (e.g., AppState: { currentState: 'active' }) so production code that checks AppState.currentState won't throw, and document that tests can override it per-test via jest.spyOn or by mutating the mock's AppState.currentState; ensure this change is applied alongside the existing Platform, DeviceEventEmitter and NativeEventEmitter mocks so behavior remains unchanged for other tests.
121-138: 🏗️ Heavy liftNew mocks are wired up but the reconnect path has no test coverage.
The four mocks correctly support the background-accept reconnect path, but no
it()block exercises it. The core bug fix — dispatchingdisconnect, closing the socket, and callingcheckAndReopenwhenAppState.currentState !== 'active'on a same-workspace accept — is entirely unverified by this test file.Additionally,
sdk.currentis mocked asnull, so even if a test were added today,sdk.current?.disconnect()andsdk.current?.checkAndReopen()would silently no-op. To assert those calls, the mock needs a non-null ref with spy methods:-jest.mock('../sdk', () => ({ - __esModule: true, - default: { current: null } -})); +const mockSdkRef = { + disconnect: jest.fn(), + checkAndReopen: jest.fn(), + loggedIn: false +}; +jest.mock('../sdk', () => ({ + __esModule: true, + default: { current: mockSdkRef } +}));A minimal test for the new path would look roughly like:
it('forces reconnect when VoipAcceptSucceeded fires with AppState !== active (same workspace)', async () => { (AppState as any).currentState = 'background'; mockServerSelector.mockReturnValue('https://same.example.com'); setupMediaCallEvents(makeTestAdapters()); emitNativeVoipEvent('VoipAcceptSucceeded', buildIncomingPayload({ callId: 'bg-uuid', host: 'https://same.example.com' })); await Promise.resolve(); const { store } = jest.requireMock('../../store/auxStore'); expect(store.dispatch).toHaveBeenCalledWith({ type: 'METEOR.DISCONNECT' }); expect(mockSdkRef.disconnect).toHaveBeenCalled(); expect(mockSdkRef.checkAndReopen).toHaveBeenCalled(); });🤖 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` around lines 121 - 138, Add a test that exercises the background-accept reconnect path: create a non-null mocked sdk ref with spy methods (e.g., mockSdkRef = { disconnect: jest.fn(), checkAndReopen: jest.fn() } and set the jest mock for '../sdk' to return default: { current: mockSdkRef }), set (AppState as any).currentState = 'background', make mockServerSelector return the same workspace URL, call setupMediaCallEvents(makeTestAdapters()), emit the native VoIP event (emitNativeVoipEvent('VoipAcceptSucceeded', buildIncomingPayload({ callId: 'bg-uuid', host: 'https://same.example.com' }))), await a microtask, then assert that the auxStore store.dispatch was called with disconnect (jest.requireMock('../../store/auxStore').store.dispatch), and that mockSdkRef.disconnect and mockSdkRef.checkAndReopen were called.
🤖 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/MediaCallEvents.ios.test.ts`:
- Around line 46-58: The react-native mock used in MediaCallEvents.ios.test.ts
is missing AppState, causing AppState.currentState to be undefined; update the
jest.mock block to include an AppState export with a currentState property
(e.g., AppState: { currentState: 'active' }) so production code that checks
AppState.currentState won't throw, and document that tests can override it
per-test via jest.spyOn or by mutating the mock's AppState.currentState; ensure
this change is applied alongside the existing Platform, DeviceEventEmitter and
NativeEventEmitter mocks so behavior remains unchanged for other tests.
- Around line 121-138: Add a test that exercises the background-accept reconnect
path: create a non-null mocked sdk ref with spy methods (e.g., mockSdkRef = {
disconnect: jest.fn(), checkAndReopen: jest.fn() } and set the jest mock for
'../sdk' to return default: { current: mockSdkRef }), set (AppState as
any).currentState = 'background', make mockServerSelector return the same
workspace URL, call setupMediaCallEvents(makeTestAdapters()), emit the native
VoIP event (emitNativeVoipEvent('VoipAcceptSucceeded', buildIncomingPayload({
callId: 'bg-uuid', host: 'https://same.example.com' }))), await a microtask,
then assert that the auxStore store.dispatch was called with disconnect
(jest.requireMock('../../store/auxStore').store.dispatch), and that
mockSdkRef.disconnect and mockSdkRef.checkAndReopen were called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d40ee847-62a5-4082-aaaa-0de04cdde7e6
📒 Files selected for processing (3)
app/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaSessionInstance.test.ts
✅ Files skipped from review due to trivial changes (1)
- app/lib/services/voip/MediaCallEvents.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- 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/services/voip/MediaCallEvents.ios.test.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 numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaSessionInstance.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaSessionInstance.test.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaSessionInstance.test.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/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaSessionInstance.test.ts
🔇 Additional comments (1)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
59-65: ⚡ Quick winThe test is correct; the mock setup appropriately reflects the code path being tested.
The production code path in
setSendSignalFnextracts the DDP socket viaconst ddpSocket = (await sdk.current?.socket)?.ddp(not directly from the mockedsdkdefault export). Since the test does not mocksdk.current.socket,ddpSocketisundefined, and the login gate conditionif (ddpSocket && !ddpSocket.loggedIn)is correctly skipped. The test verifies the happy path where the socket is unavailable andmethodCallproceeds directly. ThesetImmediateflush allows the async IIFE to execute before asserting the call.
|
iOS Build Available Rocket.Chat Experimental 4.72.0.108753 |
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Failure
|
Android Build Available Rocket.Chat Experimental 4.72.0.108752 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQ2i7C5YNXO5Cd71c7kFYSHjLFozZq9jflF-Mz-BsFNXgnd7G2ZHuFc6Ci5cbFsbygi11aJvP0D3steMXiq |
1 participant
Proposed changes
Iterates on the previous warm-locked-accept fix. Naive
checkAndReopenno-ops when the SDK still believes the socket is alive (alive()is a 20s window fromlastPing). On iOS, TCP can zombie inside that window — the no-op leaves answer SDP and ICE candidates writing to a dead socket and the peer times out at ~10s.MediaCallEvents.handleVoipAcceptSucceededFromNativewhenAppState.currentState !== 'active': dispatchdisconnect, close the socket, thencheckAndReopen. Foreground accept skips this entirely so healthy sessions are unaffected.setSendSignalFninMediaSessionInstancenow gatesmethodCallonddp.loggedIn(not just socket'open') with an 8s timeout.ddp.sendonly awaits'open', but server-sidestream-notify-userrequiresthis.userId— sending before relogin = silent server-side drop.voip-debug.logreconnect timing markers (accept marked→login ready→first signal sent).Issue(s)
Follow-up to #7293.
How to test or reproduce
voip-debug.logfrom the app's Documents folder via Files app — look for[reconnect-timing]lines for elapsed ms.Types of changes
Checklist
Further comments
Not aimed for merge — opened to trigger CI iOS build for on-device testing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests