Skip to content

fix(voip): resolve closure capture ordering in handleNativeAccept#7209

Merged
diegolmello merged 2 commits into
feat.voip-lib-newfrom
fix/voip-service-closure-capture
Apr 22, 2026
Merged

fix(voip): resolve closure capture ordering in handleNativeAccept#7209
diegolmello merged 2 commits into
feat.voip-lib-newfrom
fix/voip-service-closure-capture

Conversation

@diegolmello

@diegolmello diegolmello commented Apr 22, 2026

Copy link
Copy Markdown
Member

Proposed changes

Fixes two issues in handleNativeAccept:

  1. Swift compiler error: Resolves "Closure captures 'finishAccept' before it is declared" by reordering finishAccept declaration before timeoutWorkItem.
  2. Cold-start accept failures: Ensures iOS correctly reports accept failures when the app is not running at call accept time. Without this fix, cold-started apps misclassify accept failures as unanswered incoming calls.

Issue(s)

N/A

How to test or reproduce

  1. Start a VoIP call to a user
  2. Accept the call on iOS while the app is in the background or killed
  3. Simulate REST failure (e.g. network timeout or server error)
  4. Launch the app — verify the accept failure is correctly reported instead of being treated as a missed incoming call

Screenshots

N/A

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

N/A

Move finishAccept declaration before timeoutWorkItem to fix Swift
compiler error "Closure captures 'finishAccept' before it is declared".
Use a flag (finishAcceptInvoked) to prevent double invocation instead
of capturing DispatchWorkItem weakly in the closure.
@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Refactored VoipService accept flow: unified the 10s timeout handling into a single finishAccept(success:) closure guarded by finishAcceptInvoked to prevent double invocation; timeout work item no longer attempts cross-cancellation. Failure notifications now post failedPayload.toDictionary() instead of a manually assembled userInfo dictionary.

Changes

Cohort / File(s) Summary
VoipService Timeout and Notification Refactor
ios/Libraries/VoipService.swift
Replaced separate cancellation/DispatchWorkItem race pattern with a single guarded finishAccept(success:) closure and finishAcceptInvoked flag for 10s timeout handling. Failure notification now posts failedPayload.toDictionary() instead of constructing a manual VoipAcceptFailed userInfo dictionary. Success path unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change: a closure capture ordering fix in handleNativeAccept. It is specific, concise, and accurately reflects the core issue being resolved.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ios/Libraries/VoipService.swift (1)

461-508: ⚠️ Potential issue | 🟠 Major

Remove weak captures to preserve payload lifecycle and observe completion state.

payload is deallocated after handleNativeAccept returns, but finishAccept is called asynchronously from the timeout handler (10+ seconds later) and the API response callback. With [weak payload], the guard fails on deallocation, silently skipping cleanup and notifications. Also, [finishAcceptInvoked] snapshots the initial value false, so the timeout handler never observes the completion flag being set to true, defeating the guard's intent.

Proposed fix
-        let finishAccept: (Bool) -> Void = { [weak payload] success in
+        let finishAccept: (Bool) -> Void = { success in
             guard !finishAcceptInvoked else { return }
             finishAcceptInvoked = true
-            guard let payload else { return }
             stopDDPClientInternal(callId: payload.callId)
             if success {
                 storeInitialEvents(payload)
@@
-        let timeoutWorkItem = DispatchWorkItem { [weak payload, finishAcceptInvoked] in
-            guard let payload else { return }
+        let timeoutWorkItem = DispatchWorkItem {
             guard !finishAcceptInvoked else { return }
             // Check the callId is still tracked (not already cleaned up).
             nativeAcceptLock.lock()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/VoipService.swift` around lines 461 - 508, The closures
finishAccept and the DispatchWorkItem timeoutWorkItem currently capture payload
weakly and snapshot finishAcceptInvoked, causing payload to be deallocated and
the completion flag to not be observed; remove the explicit capture lists (i.e.,
drop “[weak payload]” from finishAccept and “[weak payload,
finishAcceptInvoked]” from timeoutWorkItem) so payload is captured normally and
finishAcceptInvoked is referenced by closure (not captured by value), and if you
need to avoid retain cycles capture only self weakly (not payload) — update the
closures in VoipService.swift where finishAccept, stopDDPClientInternal,
clearNativeAcceptDedupe, nativeAcceptLock, nativeAcceptHandledCallIds and the
DispatchWorkItem are defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@ios/Libraries/VoipService.swift`:
- Around line 461-508: The closures finishAccept and the DispatchWorkItem
timeoutWorkItem currently capture payload weakly and snapshot
finishAcceptInvoked, causing payload to be deallocated and the completion flag
to not be observed; remove the explicit capture lists (i.e., drop “[weak
payload]” from finishAccept and “[weak payload, finishAcceptInvoked]” from
timeoutWorkItem) so payload is captured normally and finishAcceptInvoked is
referenced by closure (not captured by value), and if you need to avoid retain
cycles capture only self weakly (not payload) — update the closures in
VoipService.swift where finishAccept, stopDDPClientInternal,
clearNativeAcceptDedupe, nativeAcceptLock, nativeAcceptHandledCallIds and the
DispatchWorkItem are defined.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 88ba80b7-f6e4-4405-82d8-fd407d06c659

📥 Commits

Reviewing files that changed from the base of the PR and between 7301db6 and 0ce408b.

📒 Files selected for processing (1)
  • ios/Libraries/VoipService.swift
📜 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
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate

Applied to files:

  • ios/Libraries/VoipService.swift

…recovery

Missing since closure capture reorder in 0ce408b. The failure path
must store failedPayload to initialEventsData so getInitialEvents()
returns the correct voipAcceptFailed=true state on cold start, when no
VoipAcceptFailed notification is delivered to JS (app was not running).
@diegolmello diegolmello had a problem deploying to official_android_build April 22, 2026 18:45 — with GitHub Actions Failure
@diegolmello diegolmello temporarily deployed to experimental_ios_build April 22, 2026 18:45 — with GitHub Actions Inactive
@diegolmello diegolmello had a problem deploying to experimental_android_build April 22, 2026 18:45 — with GitHub Actions Failure
@github-actions

Copy link
Copy Markdown

iOS Build Available

Rocket.Chat Experimental 4.72.0.108588

@diegolmello diegolmello merged commit df5e134 into feat.voip-lib-new Apr 22, 2026
8 of 12 checks passed
@diegolmello diegolmello deleted the fix/voip-service-closure-capture branch April 22, 2026 19:35
diegolmello added a commit that referenced this pull request Apr 22, 2026
…/Decline (#7215)

* merge feat.voip-lib

* feat(voip): enhance call handling with UUID mapping and event listeners

* Base call UI

* feat(voip): integrate Zustand for call state management and enhance CallView UI

* feat(voip): add simulateCall function for mock call handling in UI development

* refactor(CallView): update button handlers and improve UI responsiveness

* Add pause-shape-unfilled icon

* Base CallHeader

* toggleFocus

* collapse buttons

* Header components

* Hide header when no call

* Timer

* Add use memo

* Add voice call item on sidebar

* cleanup

* Temp use @rocket.chat/media-signaling from .tgz

* cleanup

* Check module and permissions to enable voip

* Refactor stop method to use optional chaining for media signal listeners

* voip push first test

* Add VoIP call handling with pending call management

- Implemented VoIP push notification handling in index.js, including storing call info for later processing.
- Added CallKeep event handlers for answering and ending calls from a cold start.
- Introduced a new CallIdUUID module to convert call IDs to deterministic UUIDs for compatibility with CallKit.
- Created a pending call store to manage incoming calls when the app is not fully initialized.
- Updated deep linking actions to include VoIP call handling.
- Enhanced MediaSessionInstance to process pending calls and manage call states effectively.

* Remove pending store and create getInitialEvents on app/index

* Attempt to make iOS calls work from cold state

* lint and format

* Patch callkeep ios

* Temp send iOS voip push token on gcm

* Temp fix require cycle

* chore: format code and fix lint issues [skip ci]

* CallIDUUID module on android and voip push

* Add setCallUUID on useCallStore to persist calls accepted on native Android

* remove callkeep from notification

* Android Incoming Call UI POC

* Refactor VoIP handling: Migrate VoIP-related classes to a new package structure, removing deprecated modules and consolidating functionality. Update imports in MainApplication and NotificationIntentHandler to reflect changes. This cleanup enhances code organization and prepares for future VoIP feature enhancements.

* Remove VoipForegroundService

* cleanup and use caller instead of callerName

* Cleanup and make iOS build again

* Refactor VoIP handling: Remove unused event emissions for call answered and declined, switch from SharedPreferences to in-memory storage for pending VoIP call data, and update method signatures for better clarity. This cleanup enhances performance and prepares for future VoIP feature improvements.

* Refactor VoIP handling: Introduce a new VoipPayload class to encapsulate call data, streamline notification processing, and enhance method signatures across the VoIP module. This update improves code clarity and prepares for future feature enhancements.

* Migrate react-native-voip-push-notifications to VoipModule

* Refactor VoIP module: Update package structure by moving VoipTurboPackage to the main package and removing the obsolete NativeVoipSpec class. Adjust imports in MainApplication and VoipModule to reflect these changes, enhancing code organization and maintainability.

* Unify emitters

* Move CallKeep listeners from MediaSessionInstance to getInitialEvents

* Clear callkeep on endcall

* Unify getInitialEvents logic

* getInitialEvents -> MediaCallEvents

* chore: format code and fix lint issues [skip ci]

* feat(Android): Add full screen incoming call (#6977)

* feat: Update call UI (#6990)

* feat: Handle audio routing, e.g., Bluetooth headset vs. internal speaker switching (#6992)

* fix: empty space when not on call (#6993)

* feat: Dialpad (#7000)

* action: organized translations

* feat: start call (#7024)

* chore: format code and fix lint issues

* feat: Pre flight (#7038)

* action: organized translations

* feat: Receive voip push notifications from backend (#7045)

* feat: Refactor media session handling and improve disconnect logic (#7065)

* feat: Control incoming call from native (#7066)

* feat: Voice message blocks (#7057)

* feat: native accept success event (#7068)

* feat(voip): call waiting, busy detection, and videoconf blocking (#7077)

* action: organized translations

* feat(voip): tap-to-hide call controls with animations (#7078)

* feat(voip): navigate to call DM from message button and header (#7082)

* feat(voip): tablet and landscape layout (#7110)

* chore: develop into feat.voip-lib-new (RN 81 + Expo 54 + reanimated 4 + true-sheet + iOS 26) (#7114)

* chore: format code and fix lint issues

* feat(voip): android landscape layout for IncomingCallActivity (#7116)

* Update agents files

* feat(voip): Support a11y (#7106)

* Fix content cutting on iOS on some edge cases

* pods

* Ignore .worktrees on jest

* chore: Merge develop into feat.voip-lib-new (#7129)

* fix(voip): show CallKit UI when call is active in background (#7128)

* chore: Update media-signaling to 0.2.0 (#7153)

* feat(voip): migrate iOS accept/reject from DDP to REST (#7124)

* Fix icons

* feat(voip): migrate Android accept/reject from DDP to REST (#7127)

* test(voip): integration tests for CallView pipeline (#7161)

* feat(voip): display video conf provider as subtitle (#7160)

* fix(voip): CallView button grid and correct landscape/dialpad layouts (#7164)

* fix(voip): prevent stale MMKV cache on Android first-install accept

MMKVKeyManager.initialize ran in MainApplication.onCreate before the JS
engine started and opened the default MMKV file via the Tencent 1.2 JAR
when it was still empty. Tencent caches instances per-ID in a singleton
registry, so that empty-state view was held for the rest of the process.
JS later wrote credentials through react-native-mmkv (MMKV Core 2.0),
which has its own separate registry. When a VoIP push arrived,
Ejson.getMMKV() got the cached empty Tencent instance and reported
"No userId found in MMKV for server". Closing and reopening the app
cleared the cache, which is why only the very first call after install
failed.

Drop the open/verify block — the encryption key is already cached from
SecureKeystore, so no MMKV handle is needed here. The first Tencent
instance is now created inside Ejson.getMMKV() after JS has written,
so it scans the file fresh.

* fix(voip): prevent duplicate ringtone on Android incoming call (#7158)

* fix(voip): set explicit snaps for NewMediaCall bottom sheet (#7165)

* Update app/lib/services/voip/MediaSessionStore.ts

Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>

* fix: make startVoipFork reactive to permissions-changed (#7151)

* fix(android): remove MediaProjectionService from merged manifest (#7190)

* fix(voip): Phone account creation (#7170)

* feat: add Enable Mobile Ringing toggle in user preferences (#7155)

* fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens (#7167)

* fix(android): Play Store mic discoverability, safer FCM logs, avatar auth via headers (#7171)

* fix(ios): serialize VoipService bridge statics (#7169)

* fix(voip): Android DDP thread safety and VoipPayload bundle parity (#7168)

* chore(voip): dead-code and hygiene sweep (#7174)

* refactor(voip): decouple navigateToCallRoom from Redux and backfill REST/connect tests (#7176)

* test(voip): tighten ringing endCall assertion and add VideoConf VoIP-lock saga coverage (#7177)

* fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS (#7173)

* refactor(voip): MediaCallEvents Redux adapters and resetVoipState (#7178)

* refactor(voip): decouple peer autocomplete from Redux; simplify NewMediaCall (#7175)

* fix(ios): add NS_SWIFT_NAME to Challenge.runChallenge for Swift 6.2 compatibility

Swift 6.2 (Xcode 26.x / macos-26 runner) auto-renames the Objective-C
method runChallenge:didReceiveChallenge:completionHandler: to
run(_:didReceive:completionHandler:) when imported into Swift.

Add NS_SWIFT_NAME to explicitly pin the Swift import name, preventing
the compiler from applying its heuristics. This keeps the existing
Swift call site in DDPClient.swift working without changes.

* fix(ios): cancel old URLSession/webSocketTask before reconnecting in DDPClient.connect (#7197)

* fix(ios): add NSLock to nativeAcceptHandledCallIds and 10s REST timeout to handleNativeAccept (#7198)

* feat(android): create VoipCallService with FOREGROUND_SERVICE_MICROPHONE (#7199)

* fix(android): start VoipCallService on accept, stop on hangup/timeout, install end-call listener (#7200)

* fix(voip): enable DM nav for users with SIP extension (#7203)

* fix(android): handle null VoiceConnection in answerIncomingCall, notify JS (#7201)

* fix(voip): resolve closure capture ordering in handleNativeAccept (#7209)

* fix(android): integrate VoIP modules with SSL-pinned OkHttpClient (#7208)

* fix(push): gate id and voipToken behind server version checks, fix VideoConf caller extra (#7210)

* fix(voip): remove sensitive data from production logs (#7207)

* fix(android): remove isRunning guard + add double-tap guard on Accept/Decline

- VoipCallService: remove if (!isRunning) guard, call startForeground unconditionally
  (idempotent on Android, fixes Android 14+ foreground service requirement)
- IncomingCallActivity: add AtomicBoolean guard on handleAccept/handleDecline
  to prevent double-tap from triggering multiple service starts

---------

Co-authored-by: diegolmello <diegolmello@users.noreply.github.com>
Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant