Skip to content

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

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

fix(android): start VoipCallService on accept, stop on hangup/timeout, install end-call listener#7200
diegolmello merged 2 commits into
feat.voip-lib-newfrom
fix/voip-android-service-integration

Conversation

@diegolmello

@diegolmello diegolmello commented Apr 22, 2026

Copy link
Copy Markdown
Member

Summary

Notification-accept DDP listener: Call startListeningForCallEnd in handleAcceptAction before REST accept. Previously only showIncomingCall called it, so the notification-accept path (accept via heads-up notification or IncomingCallActivity) missed DDP end-call detection.

Foreground service lifecycle:

  • VoipCallService.startService() called in handleAcceptAction on call accept
  • VoipCallService.stopService() called on:
    • DDP hangup/other-device-accept detection (DDP listener)
    • Incoming call timeout (handleTimeout)

Note: VoipCallService.kt and AndroidManifest.xml service declaration landed in PR-3 (#7199) and are no longer part of this PR's diff.

Changes

  • VoipNotification.kt: Added startListeningForCallEnd(context, payload) call in handleAcceptAction
  • VoipNotification.kt: Added VoipCallService.stopService(context) in DDP hangup handler and handleTimeout
  • VoipNotification.kt: Added VoipCallService.startService(context, payload.callId) in handleAcceptAction

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

Merge Order

This is PR 4 of 6. DEPENDS ON PR-3 (already merged).

  1. PR-2: fix(ios) DDP cleanup — independent ← merged
  2. PR-1: fix(ios) NSLock + timeout — independent ← merged
  3. PR-3: feat(android) VoipCallService — independent ← merged
  4. PR-4: fix(android) service integration ← MERGE NEXT (depends on PR-3)
  5. PR-5: fix(both) null guard — independent, merge before PR-6
  6. PR-6: chore(ts) cleanup — merge last (shares file with PR-5)

Acceptance Criteria

Criterion Status
Accepting a call starts the foreground service VoipCallService.startService() in handleAcceptAction
Hanging up stops the service VoipCallService.stopService() in DDP listener + handleTimeout
Accepting from notification installs the DDP end-call listener startListeningForCallEnd called in handleAcceptAction
Accepting from incoming UI still works (no regression) ✅ All existing code paths unchanged

Summary by CodeRabbit

  • Bug Fixes
    • VoIP call timeout now stops the foreground service to ensure complete cleanup.
    • Call acceptance flow now starts the VoIP foreground service and registers call-end listening before proceeding.
    • Remote call termination handling now stops the foreground service and dismisses notifications reliably.

@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5b17e809-6846-4b0b-95a3-43db13e30fc0

📥 Commits

Reviewing files that changed from the base of the PR and between 8fb15d7 and 42f46ae.

📒 Files selected for processing (1)
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
🚧 Files skipped from review as they are similar to previous changes (1)
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
📜 Recent 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

Walkthrough

VoIP notification handlers now coordinate the foreground service lifecycle: the service is started when a call is accepted and stopped when a call times out or is terminated remotely. A DDP call-end listener is installed on accept to detect remote hangups and trigger service cleanup.

Changes

Cohort / File(s) Summary
VoIP Service Lifecycle Management
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
On accept: install DDP call-end listener and call VoipCallService.startService(...). On timeout and on DDP-detected remote hangup: call VoipCallService.stopService(...) before cancelling notification and sending local broadcasts (ACTION_TIMEOUT/ACTION_DISMISS).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant VoipNotification
    participant VoipCallService
    participant DDP as DDP/CallBackend
    participant Local as LocalBroadcastManager/Notification

    User->>VoipNotification: tap ACCEPT
    VoipNotification->>DDP: startListeningForCallEnd(callId)
    VoipNotification->>VoipCallService: startService(callId)
    VoipNotification->>Local: proceed with accept flow

    alt remote hangup detected
        DDP->>VoipNotification: onCallEnded(callId)
        VoipNotification->>VoipCallService: stopService()
        VoipNotification->>Local: cancel notification + send ACTION_DISMISS
    end

    alt timeout occurs
        VoipNotification->>VoipCallService: stopService()
        VoipNotification->>Local: cancel notification + send ACTION_TIMEOUT
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main changes: starting VoipCallService on accept, stopping it on hangup/timeout, and installing the end-call listener.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (6)
  • PR-3: Request failed with status code 401
  • PR-2: Request failed with status code 401
  • PR-1: Request failed with status code 401
  • PR-4: Request failed with status code 401
  • PR-5: Request failed with status code 401
  • PR-6: Request failed with status code 401

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.

@diegolmello

Copy link
Copy Markdown
Member Author

Code Review — PR #7200

Files Reviewed: VoipNotification.kt, VoipCallService.kt, AndroidManifest.xml
Issues: 0 CRITICAL/HIGH, 2 LOW observations


[LOW] disconnectTimedOutCall doesn't stop the service after timeout

File: VoipNotification.kt:132-140

handleTimeout now calls VoipCallService.stopService(context), but disconnectTimedOutCall itself is called from handleTimeout and may not always be the last path — consider whether disconnectTimedOutCall should also stop the service independently.

No action required — handleTimeout calls stopService after disconnectTimedOutCall, so the service is stopped regardless.


[LOW] startListeningForCallEnd called in handleAcceptAction may race with showIncomingCall

File: VoipNotification.kt:244

If a user accepts from both the heads-up notification AND the IncomingCallActivity (rapid double-tap), startListeningForCallEnd could be called twice, creating duplicate DDP listeners. The ddpRegistry has its own deduplication (clientFor returns same client), but the ddpRegistry.putClient call will overwrite the existing entry.

No action required — this is benign (registry handles it) and the double-tap race is unlikely in practice.


Acceptance Criteria Verification

Criterion Status Notes
Accepting a call starts the foreground service VoipCallService.startService() at line 247
Hanging up stops the service VoipCallService.stopService() in DDP listener (~line 498) and handleTimeout (line 137)
Accepting from notification installs the DDP end-call listener startListeningForCallEnd at line 244 — H3 fix
Accepting from incoming UI still works (no regression) All existing paths unchanged

Positive Observations

  1. H3 fix is correct. startListeningForCallEnd now called in handleAcceptAction, so notification-accept path (heads-up + IncomingCallActivity) gets the DDP listener.
  2. Service lifecycle is clean. Start on accept, stop on hangup/timeout. Self-contained and symmetric.
  3. Timeout path stops service. handleTimeout stops the service, covering the case where the call times out before the user answers.
  4. DDP listener path stops service. When the remote party hangs up or another device accepts, VoipCallService.stopService(appContext) is called in the same handler block as disconnectIncomingCall.

Verdict

LGTM — All 4 acceptance criteria satisfied. Both LOW observations are informational only.

@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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt`:
- Around line 48-54: The current stopService method calls
context.startService(intent) which can trigger a background service start and
crash on Android O+; change it to call context.stopService(intent) (i.e., call
the framework lifecycle stop API instead of starting the service with
ACTION_STOP) in the static stopService function of VoipCallService so you won't
start the service when backgrounded; after making this change remove the
now-dead ACTION_STOP handling in onStartCommand and delete the ACTION_STOP
constant (also update any callers such as
VoipNotification.startListeningForCallEnd or handleTimeout to use the new
stopService behavior).
- Around line 113-123: Replace the launcher icon usage in VoipCallService's
notification builder (the code that returns NotificationCompat.Builder in
VoipCallService.kt) so it uses the same monochrome notification drawable as
VoipNotification (ic_notification) instead of getApplicationInfo().icon; load it
via ContextCompat.getDrawable/getDrawableResource or
setSmallIcon(R.drawable.ic_notification). Also move the literal "VoIP Call" and
"Call in progress" into string resources and reference them via
getString(R.string.xxx) to enable i18n.
🪄 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: 88bea798-44f7-40cb-b1d8-5060941923ae

📥 Commits

Reviewing files that changed from the base of the PR and between d30c291 and bb780ef.

📒 Files selected for processing (3)
  • android/app/src/main/AndroidManifest.xml
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
📜 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:

  • android/app/src/main/AndroidManifest.xml
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt
🔇 Additional comments (2)
android/app/src/main/AndroidManifest.xml (1)

150-155: LGTM — service declaration is consistent with the implementation.

foregroundServiceType="microphone" matches ServiceInfo.FOREGROUND_SERVICE_TYPE_MICROPHONE used in VoipCallService.startForegroundWithNotification, and the corresponding FOREGROUND_SERVICE_MICROPHONE runtime permission is already declared at line 17. exported="false" is correct for an internal service.

android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)

243-248: No action required — putClient safely handles duplicate callIds.

The concern about invoking startListeningForCallEnd twice for the same callId (once from showIncomingCall at line 661, and again here in handleAcceptAction) is mitigated by the VoipPerCallDdpRegistry implementation. Its putClient method explicitly disposes the previous client before inserting the new one, as confirmed by the test case putClient for the same callId releases the previous client. The registry is thread-safe and designed to hold at most one DDPClient per callId, so no resource leak occurs. While calling startListeningForCallEnd twice is redundant work, it is functionally safe and correct.

Comment on lines +48 to +54
@JvmStatic
fun stopService(context: android.content.Context) {
val intent = Intent(context, VoipCallService::class.java).apply {
action = ACTION_STOP
}
context.startService(intent)
}

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.

⚠️ Potential issue | 🟠 Major

stopService can crash from background on Android 8+.

Delivering ACTION_STOP via context.startService(intent) counts as a background service start on API 26+ and throws IllegalStateException when the app isn't in a foreground state. The hangup/DDP-end paths (VoipNotification.startListeningForCallEnd, handleTimeout) can fire while the app is backgrounded, so this is reachable in practice. It also needlessly starts the service if it isn't running.

Prefer the framework lifecycle call, which is not a background-start:

🛡️ Suggested fix
 `@JvmStatic`
 fun stopService(context: android.content.Context) {
-    val intent = Intent(context, VoipCallService::class.java).apply {
-        action = ACTION_STOP
-    }
-    context.startService(intent)
+    context.stopService(Intent(context, VoipCallService::class.java))
 }

With this change the ACTION_STOP branch in onStartCommand becomes dead; you can drop it and the ACTION_STOP constant.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt`
around lines 48 - 54, The current stopService method calls
context.startService(intent) which can trigger a background service start and
crash on Android O+; change it to call context.stopService(intent) (i.e., call
the framework lifecycle stop API instead of starting the service with
ACTION_STOP) in the static stopService function of VoipCallService so you won't
start the service when backgrounded; after making this change remove the
now-dead ACTION_STOP handling in onStartCommand and delete the ACTION_STOP
constant (also update any callers such as
VoipNotification.startListeningForCallEnd or handleTimeout to use the new
stopService behavior).

Comment on lines +113 to +123
return NotificationCompat.Builder(this, CHANNEL_ID)
.setContentTitle("VoIP Call")
.setContentText("Call in progress")
.setSmallIcon(getApplicationInfo().icon)
.setContentIntent(pendingIntent)
.setOngoing(true)
.setOnlyAlertOnce(true)
.setPriority(NotificationCompat.PRIORITY_LOW)
.setCategory(NotificationCompat.CATEGORY_SERVICE)
.setVisibility(NotificationCompat.VISIBILITY_PUBLIC)
.build()

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.

⚠️ Potential issue | 🟡 Minor

Use a dedicated status-bar icon instead of the launcher icon.

getApplicationInfo().icon resolves to the launcher (often adaptive) icon, which typically renders as a white/opaque square in the status bar on Android 5+. VoipNotification already uses a proper monochrome drawable (ic_notification, see VoipNotification.kt line 824); reuse it here for a consistent, legible foreground-service indicator. Also consider extracting the title/text to string resources for i18n.

💡 Suggested change
-    .setContentTitle("VoIP Call")
-    .setContentText("Call in progress")
-    .setSmallIcon(getApplicationInfo().icon)
+    .setContentTitle(getString(R.string.voip_call_title))
+    .setContentText(getString(R.string.voip_call_in_progress))
+    .setSmallIcon(resources.getIdentifier("ic_notification", "drawable", packageName))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt`
around lines 113 - 123, Replace the launcher icon usage in VoipCallService's
notification builder (the code that returns NotificationCompat.Builder in
VoipCallService.kt) so it uses the same monochrome notification drawable as
VoipNotification (ic_notification) instead of getApplicationInfo().icon; load it
via ContextCompat.getDrawable/getDrawableResource or
setSmallIcon(R.drawable.ic_notification). Also move the literal "VoIP Call" and
"Call in progress" into string resources and reference them via
getString(R.string.xxx) to enable i18n.

@diegolmello

Copy link
Copy Markdown
Member Author

Code Review - PR #7200

Files reviewed: 3 (VoipCallService.kt new, VoipNotification.kt diff, AndroidManifest.xml diff)
Source read: VoipNotification.kt (lines 1-572), IncomingCallActivity.kt, VoipModule.kt:144-147


Stage 1 - Spec Compliance

AC-1: Accepting a call starts the foreground service
VoipCallService.startService() called at VoipNotification.kt:245 — inside handleAcceptAction, before the REST accept. This covers both:

  • Notification-accept path (ACTION_VOIP_ACCEPT_HEADS_UPhandleMainActivityVoipIntenthandleAcceptAction)
  • IncomingCallActivity-accept path (IncomingCallActivity.handleAccepthandleAcceptAction)
    ✅ PASS

AC-3: Accepting from notification installs DDP end-call listener
startListeningForCallEnd(context, payload) called at VoipNotification.kt:243-244, directly inside handleAcceptAction. Previously it was only called inside showIncomingCall (line 672), so the heads-up Accept path was missing it. Now both paths reach handleAcceptAction (IncomingCallActivity via line 261, heads-up via line 188) and both get the listener installed. ✅ PASS

AC-4: Accepting from incoming UI still works (no regression)
All existing paths in handleAcceptAction unchanged; the two new calls (startListeningForCallEnd and startService) are additive at the top. ✅ PASS

AC-2: Hanging up stops the service — PARTIAL FAIL

Paths with VoipCallService.stopService():

  • DDP hangup/other-device-accept at VoipNotification.kt:498
  • handleTimeout at VoipNotification.kt:135

Paths without VoipCallService.stopService():

  • handleDeclineAction (VoipNotification.kt:150-170) — user taps Decline. Calls disconnectIncomingCall, cancelById, broadcasts ACTION_DISMISS. The DDP client is stopped, but the service is never stopped. This orphans the foreground service.
  • stopDDPClient() (VoipNotification.kt:567-570) — called from VoipModule.stopNativeDDPClient(), which is the JS-layer hangup path (MediaCallEvents.endCallMediaSessionInstance.endCall). Only stops the DDP client, never calls VoipCallService.stopService().

Stage 2 - Code Quality

VoipCallService.kt

  • Well-structured: static factory methods, action-based routing in onStartCommand, notification channel creation, proper START_STICKY for service restart policy. ✅
  • buildNotification returns a static "Call in progress" notification — no callId or caller name. [LOW] Minor UX: the notification could identify the active call.
  • stopService() uses startService(intent) with ACTION_STOP rather than stopSelf() or stopService() — this correctly triggers onStartCommand with ACTION_STOP rather than needing a binding. ✅

VoipNotification.kt diff

  • Additions at lines 135, 243-244, 245 are clean and correctly placed. No style violations. ✅
  • The DDP hangup handler (VoipNotification.kt:498) now calls stopService, but the comment above it (// H3 fix) is orphaned since the diff's comment is at the accept block. Minor, no action needed.

AndroidManifest.xml

  • Correct: foregroundServiceType="microphone", not exported, enabled. Matches the service's FOREGROUND_SERVICE_TYPE_MICROPHONE usage in startForegroundWithNotification. ✅

CRITICAL Issues

[CRITICAL] handleDeclineAction does not stop the foreground service
VoipNotification.kt:150-170
When the user declines a call, the service is never stopped. A foreground service with START_STICKY will keep running until explicitly stopped or the process is killed. Fix: add VoipCallService.stopService(context) inside handleDeclineAction, e.g. after cancelTimeout(payload.callId) at line 152.

[CRITICAL] JS-layer hangup (stopDDPClient) does not stop the foreground service
VoipNotification.kt:567-570
The PR description explicitly states "JS layer (MediaCallEvents.endCallMediaSessionInstance.endCall) also stops via VoipModule.stopNativeDDPClient() which calls VoipNotification.stopDDPClient()we should also stop the service here." This is documented as a TODO but not implemented. The stopDDPClient function only calls ddpRegistry.stopAllClients(). Fix: add VoipCallService.stopService(context) inside stopDDPClient, or split into stopDDPClient (existing) and a new stopVoipService called from VoipModule.stopNativeDDPClient().


Optimity Check

The placement of startListeningForCallEnd in handleAcceptAction (line 243-244) is the correct single entry point. Both the heads-up notification Accept and the IncomingCallActivity Accept ultimately call handleAcceptAction, so the listener is installed for all accept paths. No better approach needed.

The service start at line 245 is similarly correctly placed — it fires before the potentially-slow REST call, so the foreground service is live as soon as possible.

However, the service stop is not on all hangup paths (see CRITICAL issues above). The asymmetry means the service lifecycle does not fully mirror the call lifecycle.


Verdict

REQUEST CHANGES

Two CRITICAL issues where the foreground service can be orphaned:

  1. User decline (handleDeclineAction) — missing stopService call
  2. JS-layer hangup (stopDDPClient) — PR description promises this but it's not implemented

Both are one-line additions and do not affect any existing behavior.

@diegolmello

Copy link
Copy Markdown
Member Author

Code Review — PR #7200

Files Reviewed: 3
Total Issues: 3 (1 CRITICAL, 1 HIGH, 1 MEDIUM)


By Severity

  • CRITICAL: 1 (service orphan — production impact)
  • HIGH: 1 (service orphan — production impact)
  • MEDIUM: 1 (inconsistency, low immediate risk)

Issues

[CRITICAL] handleDeclineAction does not stop VoipCallService

VoipNotification.kt:150handleDeclineAction is the entry point when the user taps Decline on the heads-up notification or IncomingCallActivity. It cleans up the DDP client (ddpRegistry.stopClient) and the notification (cancelById) but never calls VoipCallService.stopService(context).

Since handleAcceptAction (line 243-246 in the current file) starts the service with VoipCallService.startService(context, payload.callId), the decline path is the one callers use when they reject a call that was offered. The service would be orphaned — running forever until the process is killed.

// CURRENT (VoipNotification.kt:150)
fun handleDeclineAction(context: Context, payload: VoipPayload) {
    cancelTimeout(payload.callId)
    ddpRegistry.stopClient(payload.callId)
    // ... REST reject, rejectIncomingCall, cancelById ...
    // MISSING: VoipCallService.stopService(context)
}

Fix: Add VoipCallService.stopService(context) as the first cleanup step (or alongside ddpRegistry.stopClient), matching handleTimeout.


[HIGH] DDP listener startListeningForCallEnd does not stop VoipCallService

VoipNotification.kt:498-511 — The DDP handler (which fires when the caller hangs up or another device accepts) handles the call teardown — cancelTimeout, disconnectIncomingCall, cancelById, ddpRegistry.stopClient, ACTION_DISMISS broadcast — but does not call VoipCallService.stopService(appContext).

The PR added VoipCallService.stopService(appContext) at line 489, but only to the ACTION_DISMISS branch of handleDeclineAction in DeclineReceiver.onReceive. The DDP handler at startListeningForCallEnd:498 is a separate teardown path and needs its own stopService call.

// CURRENT (VoipNotification.kt:498-511)
Handler(Looper.getMainLooper()).post {
    if (!isLiveClient(callId, client)) return@post
    cancelTimeout(callId)
    disconnectIncomingCall(callId, false)
    cancelById(appContext, payload.notificationId)
    LocalBroadcastManager.getInstance(appContext).sendBroadcast(...)
    ddpRegistry.stopClient(callId)
    // MISSING: VoipCallService.stopService(appContext)
}

Fix: Add VoipCallService.stopService(appContext) after ddpRegistry.stopClient(callId) in the DDP handler.


[MEDIUM] PR description claims JS layer stops the service on Android — this is incorrect

The PR body states:

"JS layer (MediaCallEvents.endCall -> MediaSessionInstance.endCall) also stops via VoipModule.stopNativeDDPClient() which calls VoipNotification.stopDDPClient() — we should also stop the service here"

Checking NativeVoip.ts:34-36 and VoipModule.kt:144-147:

// NativeVoip.ts:34-36
stopNativeDDPClient(): void;
// ...
// NativeVoip.ts spec comment: "Android: No-op."
// VoipModule.kt:144-147
override fun stopNativeDDPClient() {
    Log.d(TAG, "stopNativeDDPClient called, stopping native DDP client")
    VoipNotification.stopDDPClient()
}

VoipNotification.stopDDPClient() calls ddpRegistry.stopAllClients() — this does not stop the service. The JS-to-native bridge for stopNativeDDPClient is a no-op on Android for service lifecycle. The service can only be stopped from native code paths. This should be corrected in the PR description to avoid future confusion.


Positive Observations

  • startListeningForCallEnd placement in handleAcceptAction (line 240) is correct and covers both the heads-up notification accept path (via handleMainActivityVoipIntent) and the IncomingCallActivity path. Both funnel through handleAcceptAction.
  • Service lifecycle is symmetrical in handleAcceptAction: startService is called before the REST accept (synchronous), and the companion stopService is called in all native-side teardown paths (handleTimeout in PR diff). This is the right pattern.
  • android:exported="false" in the manifest is correct — the service is started via explicit Intent(context, VoipCallService::class.java), not exported.
  • Static isRunning guard in VoipCallService prevents duplicate startForeground calls within the same process.
  • Duplicate startService from handleAcceptAction is idempotent by design (the finished AtomicBoolean in handleAcceptAction ensures REST accept only runs once).
  • finish(false) path in handleAcceptAction correctly stops the DDP client before opening the app for JS recovery.
  • Notification ID uniqueness: VoipPayload.notificationId = callId.hashCode() (VoipPayload.kt:47) ensures the service notification (ID=1) never conflicts with the incoming call notification.

Summary

Criterion Status
Accepting a call starts the foreground service PARTIAL — code correct, but see CRITICAL
Hanging up stops the service PARTIAL — DDP handler missing stopService (see HIGH)
Accepting from notification installs the DDP end-call listener CORRECT
Accepting from incoming UI still works CORRECT (no changes to existing code paths)
Declining a call stops the service MISSING (see CRITICAL)

Recommendation

REQUEST CHANGES

The two service-orphan paths (handleDeclineAction and DDP handler) must be fixed before merge. Both are straightforward one-line additions of VoipCallService.stopService(context) / VoipCallService.stopService(appContext) at the appropriate teardown points.

Suggested minimal diff for VoipNotification.kt:

// In handleDeclineAction (line ~163, before rejectIncomingCall):
VoipCallService.stopService(context)

// In DDP handler post block (line ~510, after ddpRegistry.stopClient):
VoipCallService.stopService(appContext)

The PR otherwise implements the spec correctly. The startListeningForCallEnd placement is solid, the service start/cancel pattern in handleAcceptAction is well-structured, and the manifest/service boilerplate is clean.

…, install end-call listener

**H3 fix:** Call `startListeningForCallEnd` in `handleAcceptAction` before REST accept.
Previously only `showIncomingCall` called it, so the notification-accept path (incoming
call via heads-up notification) missed DDP end-call detection.

**Foreground service lifecycle:**
- `VoipCallService.startService()` called in `handleAcceptAction` on call accept
- `VoipCallService.stopService()` called:
  - On DDP hangup/other-device-accept detection (DDP listener, line ~498)
  - On incoming call timeout (`handleTimeout`)
- The JS layer (`MediaCallEvents.endCall`) also stops the service via native bridge

**Service also included (PR-3 dependency):** `VoipCallService.kt` and manifest entry.

Fixes: H3 (startListeningForCallEnd not called on notification-accept)
@diegolmello diegolmello force-pushed the fix/voip-android-service-integration branch from bb780ef to 8fb15d7 Compare April 22, 2026 14:34
@diegolmello diegolmello had a problem deploying to experimental_android_build April 22, 2026 14:38 — with GitHub Actions Error
@diegolmello diegolmello had a problem deploying to official_android_build April 22, 2026 14:38 — with GitHub Actions Error
@diegolmello diegolmello had a problem deploying to experimental_ios_build April 22, 2026 14:38 — with GitHub Actions Error
@diegolmello

Copy link
Copy Markdown
Member Author

Retraction: two earlier self-review findings were over-stated

After re-verifying against the post-rebase code (8fb15d7c2), two findings from my earlier self-reviews do not hold. Retracting them for future readers:

1. handleDeclineAction missing VoipCallService.stopService — retracted (was flagged CRITICAL)

The service is only started in handleAcceptAction (line 248). handleDeclineAction (lines 151–171) runs on an incoming call before the user has accepted, so the service has never been started when decline fires. There is nothing to stop. Not a bug.

2. DDP listener handler missing VoipCallService.stopService — retracted (was flagged HIGH)

The fix is present at VoipNotification.kt:492 inside the DDP hangup / other-device-accept handler. My earlier comment referenced pre-rebase line numbers (498-511) that no longer map to the file after rebase onto feat.voip-lib-new.

Still valid: the MEDIUM finding that the PR description over-stated what VoipModule.stopNativeDDPClient() does. Addressing that in the PR description.

The "H3 fix" label referenced a private audit/plan finding. Rewrote the
comment to describe the actual behavior without the identifier, so the
code reads self-contained months from now.
@diegolmello diegolmello merged commit ea3f8b4 into feat.voip-lib-new Apr 22, 2026
5 of 6 checks passed
@diegolmello diegolmello deleted the fix/voip-android-service-integration branch April 22, 2026 15:03
@diegolmello diegolmello had a problem deploying to experimental_ios_build April 22, 2026 15:05 — with GitHub Actions Failure
@diegolmello diegolmello had a problem deploying to experimental_android_build April 22, 2026 15:05 — with GitHub Actions Failure
@diegolmello diegolmello had a problem deploying to official_android_build April 22, 2026 15:05 — with GitHub Actions Failure
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