Skip to content

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

Open
diegolmello wants to merge 8 commits intofeat.voip-lib-newfrom
refactor.ddp-android-2
Open

feat(voip): migrate Android accept/reject from DDP to REST#7127
diegolmello wants to merge 8 commits intofeat.voip-lib-newfrom
refactor.ddp-android-2

Conversation

@diegolmello
Copy link
Copy Markdown
Member

@diegolmello diegolmello commented Apr 10, 2026

Proposed changes

Migrate the VoIP accept/reject signal path on Android from the DDP (WebSocket) transport to a synchronous REST call (POST /api/v1/media-calls.answer), matching the pattern already implemented on iOS.

Summary

Slice Description Commit
1 New MediaCallsAnswerRequest.kt REST client 9692a23c0
2 handleAcceptAction → REST 669c19105
3 handleDeclineAction → REST 01d119185
4 rejectBusyCall → REST d0dc50874
5 Remove dead DDP methods from VoipNotification 8d543e45a

Deferred

  • Slice 6startListeningForCallEnd off DDP (deferred to separate PRD)
  • Slice 7 — Delete DDPClient.kt / VoipPerCallDdpRegistry.kt (blocked by Slice 6)

Issue(s)

https://rocketchat.atlassian.net/browse/VMUX-67

How to test or reproduce

  1. Receive an incoming VoIP call push on Android
  2. Accept — check logs for MediaCallsAnswerRequest response
  3. Decline — same verification
  4. Be on a call and receive a second VoIP call — verify rejectBusyCall sends REST reject

Screenshots

N/A — no UI changes.

Types of changes

  • Bugfix
  • Improvement
  • New feature
  • Documentation update

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) — N/A
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

  • Improvements

    • Media call accept/decline now use a REST-based signaling path with stronger auth checks and clearer failure handling.
    • Busy-call rejection unified to the same REST flow for consistency.
    • Logging for call answer outcomes improved.
  • Stability

    • WebSocket/network client consolidated to a shared connection to improve connection stability and resource usage.

@diegolmello
Copy link
Copy Markdown
Member Author

PRD

PRD: Migrate VoIP Accept/Reject from DDP to REST (Android)

Problem Statement

The native Android VoIP implementation opens a per-call DDP WebSocket connection solely to send two signals: accept and reject. This requires DDPClient with full connect/login/subscribe/callback-queue lifecycle, and VoipPerCallDdpRegistry to manage multiple per-call DDP clients.

The same accept/reject signal can be sent via a single HTTP POST to POST /api/v1/media-calls.answer.

Solution

Replace DDP-based accept/reject signaling from VoipNotification.handleAcceptAction, VoipNotification.handleDeclineAction, and VoipNotification.rejectBusyCall with REST calls. The DDP listener for call-end detection (startListeningForCallEnd) is not migrated — it requires a persistent WebSocket subscription and is a separate problem.

User Stories

  1. Reliable accept/reject signals
  2. Understandable VoipNotification handlers
  3. Testable without WebSocket
  4. Same Ejson auth mechanism
  5. Immediate response (no DDP handshake)
  6. Remove DDP from VoIP stack (partial)
  7. Independent from iOS

Architecture

VoipNotification.handleAcceptAction(payload)
  → MediaCallsAnswerRequest.build()
    → Ejson token resolution (x-user-id / x-auth-token)
      → OkHttp POST to /api/v1/media-calls.answer
        → VoipModule.storeInitialEvents / storeAcceptFailureForJs

Out of Scope

  • Migrating startListeningForCallEnd (DDP subscription for call-end detection)
  • Migrating MediaSessionInstance.ts (JS layer WebRTC ICE signaling)
  • Server-side endpoint changes
  • iOS VoIP flow (separate PRD)
  • Deleting DDPClient.kt / VoipPerCallDdpRegistry.kt while startListeningForCallEnd still depends on them

@diegolmello
Copy link
Copy Markdown
Member Author

Progress

Slice Description Status
1 New MediaCallsAnswerRequest.kt REST client DONE — 9692a23c0
2 handleAcceptAction → REST DONE — 669c19105
3 handleDeclineAction → REST DONE — 01d119185
4 rejectBusyCall → REST DONE — d0dc50874
5 Remove dead DDP methods from VoipNotification DONE — 8d543e45a
6 startListeningForCallEnd off DDP DEFERRED (separate PRD)
7 Delete DDPClient.kt / VoipPerCallDdpRegistry.kt DEFERRED (blocked by Slice 6)

Slice 1 — MediaCallsAnswerRequest (new file)

  • Complexity: LOW
  • Blocked by: None
  • What: New MediaCallsAnswerRequest.ktPOST /api/v1/media-calls.answer with { callId, contractId, type: "answer", answer: "accept"|"reject", supportedFeatures }. Auth via Ejson (x-user-id / x-auth-token).
  • Status: DONE

Slice 2 — Migrate handleAcceptAction to REST

  • Complexity: LOW
  • Blocked by: Slice 1
  • What: Replace DDP sendAcceptSignal/queueAcceptSignal block with MediaCallsAnswerRequest.fetch(answer="accept", supportedFeatures=["audio"]). 10-second timeout, finish callback, Telecom answer all unchanged.
  • Status: DONE

Slice 3 — Migrate handleDeclineAction to REST

  • Complexity: LOW
  • Blocked by: Slice 1
  • What: Replace sendRejectSignal/queueRejectSignal with MediaCallsAnswerRequest.fetch(answer="reject"). Remove queueRejectSignal.
  • Status: DONE

Slice 4 — Migrate rejectBusyCall to REST

  • Complexity: LOW
  • Blocked by: Slice 1
  • What: Replace connectAndRejectBusy DDP cycle with MediaCallsAnswerRequest.fetch(answer="reject"). Delete connectAndRejectBusy.
  • Status: DONE

Slice 5 — Remove dead DDP methods

  • Complexity: LOW
  • Blocked by: Slices 2 and 3
  • What: Remove sendAcceptSignal, queueAcceptSignal, sendRejectSignal, queueRejectSignal, buildAcceptSignalParams, buildRejectSignalParams, resolveVoipMediaCallIdentity. Keep ddpRegistry (used by startListeningForCallEnd), isLiveClient, flushPendingQueuedSignalsIfNeeded.
  • Status: DONE

Deferred Slices

Slice 6startListeningForCallEnd off DDP: MEDIUM complexity. Requires designing an alternative transport (push notification, SSE, or persistent subscription). Separate PRD.

Slice 7 — Delete DDPClient.kt / VoipPerCallDdpRegistry.kt: LOW complexity. Blocked by Slice 6.

PRD Coverage

User Story Covered by
1 — Reliable accept/reject signals Slices 1–4
2 — Understandable VoipNotification handlers Slices 2–5
3 — Testable without WebSocket Slices 1, 2, 3
4 — Same Ejson auth mechanism Slice 1
5 — Immediate response (no DDP handshake) Slices 2, 3, 4
6 — Remove DDP from VoIP stack Slice 5 (partial)
7 — Independent from iOS All slices

@diegolmello
Copy link
Copy Markdown
Member Author

File: MediaCallsAnswerRequest.kt (Slice 1)

package chat.rocket.reactnative.voip

import android.util.Log
import chat.rocket.reactnative.notification.Ejson
import okhttp3.Call
import okhttp3.Callback
import okhttp3.MediaType.Companion.toMediaType
import okhttp3.OkHttpClient
import okhttp3.Request
import okhttp3.RequestBody.Companion.toRequestBody
import org.json.JSONArray
import org.json.JSONObject
import java.io.IOException

/**
 * REST client for `POST /api/v1/media-calls.answer` used by accept/reject flows.
 *
 * Mirrors the iOS [MediaCallsAnswerRequest.swift][ios/Shared/RocketChat/API/MediaCallsAnswerRequest.swift]
 * and replaces the DDP signal methods in [VoipNotification].
 *
 * Auth headers (`x-user-id` / `x-auth-token`) are resolved from [Ejson] at call time.
 *
 * @param callId       The call identifier from the VoIP payload.
 * @param contractId   The device-unique contract identifier (`Settings.Secure.ANDROID_ID`).
 * @param answer       Either `"accept"` or `"reject"`.
 * @param supportedFeatures Optional list of supported features (e.g. `["audio"]`); sent only for accept.
 */
class MediaCallsAnswerRequest(
    private val callId: String,
    private val contractId: String,
    private val answer: String,
    private val supportedFeatures: List<String>? = null
) {
    companion object {
        private const val TAG = "RocketChat.MediaCallsAnswerRequest"
        private val JSON_MEDIA_TYPE = "application/json; charset=utf-8".toMediaType()

        @JvmStatic
        fun fetch(
            context: android.content.Context,
            host: String,
            callId: String,
            contractId: String,
            answer: String,
            supportedFeatures: List<String>? = null,
            onResult: (Boolean) -> Unit
        ) {
            val request = MediaCallsAnswerRequest(callId, contractId, answer, supportedFeatures)
            request.execute(context, host, onResult)
        }
    }

    private fun buildBody(): JSONObject {
        val json = JSONObject().apply {
            put("callId", callId)
            put("contractId", contractId)
            put("type", "answer")
            put("answer", answer)
        }
        supportedFeatures?.let { features ->
            val arr = JSONArray()
            features.forEach { arr.put(it) }
            json.put("supportedFeatures", arr)
        }
        return json
    }

    private fun execute(
        context: android.content.Context,
        host: String,
        onResult: (Boolean) -> Unit
    ) {
        val ejson = Ejson().apply { this.host = host }
        val userId = ejson.userId()
        val token = ejson.token()

        if (userId.isNullOrEmpty() || token.isNullOrEmpty()) {
            Log.w(TAG, "Missing credentials for $host — cannot send media-call answer")
            onResult(false)
            return
        }

        val serverUrl = host.removeSuffix("/")
        val url = "$serverUrl/api/v1/media-calls.answer"

        val body = buildBody().toString()
        val requestBody = body.toRequestBody(JSON_MEDIA_TYPE)

        val request = Request.Builder()
            .header("x-user-id", userId)
            .header("x-auth-token", token)
            .url(url)
            .post(requestBody)
            .build()

        val client = OkHttpClient()
        client.newCall(request).enqueue(object : Callback {
            override fun onFailure(call: Call, e: IOException) {
                Log.e(TAG, "MediaCallsAnswerRequest failed for callId=$callId: ${e.message}")
                onResult(false)
            }

            override fun onResponse(call: Call, response: okhttp3.Response) {
                val code = response.code
                val success = code in 200..299
                Log.d(TAG, "MediaCallsAnswerRequest response for callId=$callId: code=$code success=$success")
                onResult(success)
            }
        })
    }
}

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a new Kotlin REST client to POST /api/v1/media-calls.answer, updates VoIP notification handlers to use that REST client instead of DDP for accept/reject, and switches DDP WebSocket usage to a shared OkHttpClient instance.

Changes

Cohort / File(s) Summary
REST API Client
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt
New public Kotlin class with fetch(...) (@JvmStatic). Resolves x-user-id/x-auth-token via Ejson at runtime, builds JSON (callId, contractId, type:"answer", answer, optional supportedFeatures), sends POST to /api/v1/media-calls.answer via OkHttp, and invokes onResult(Boolean) on the main thread based on HTTP outcome or failures.
VoIP Notification Refactor
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
Removed VoipMediaCallIdentity and DDP signaling helpers. handleAcceptAction, handleDeclineAction, and busy-call rejection now call MediaCallsAnswerRequest.fetch(...), derive contractId from Settings.Secure.ANDROID_ID, set answer ("accept"/"reject"), and pass supportedFeatures (null for decline, listOf("audio") for accept). Kept finish(success) wiring for accept.
DDPClient: networking change
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt
Introduced a shared singleton OkHttpClient (sharedClient) with WebSocket ping interval; connect() uses shared client for newWebSocket() and disconnect() no longer shuts down or nulls the shared client. Public API unchanged.

Sequence Diagram(s)

sequenceDiagram
    participant VN as VoipNotification
    participant Settings as Settings.Secure
    participant MCAR as MediaCallsAnswerRequest
    participant Ejson as Ejson (Auth)
    participant HTTP as OkHttp
    participant API as Media Calls API

    VN->>Settings: get ANDROID_ID (contractId)
    VN->>MCAR: fetch(context, host, callId, contractId, answer, features, onResult)
    MCAR->>Ejson: resolve x-user-id & x-auth-token
    alt credentials missing
        MCAR->>VN: onResult(false)
    else credentials present
        MCAR->>MCAR: build JSON body (callId, contractId, type, answer, supportedFeatures?)
        MCAR->>HTTP: POST /api/v1/media-calls.answer with headers & body
        HTTP->>API: send request
        API-->>HTTP: HTTP response
        HTTP->>MCAR: onResponse(status)
        alt 2xx
            MCAR->>VN: onResult(true)
        else
            MCAR->>VN: onResult(false)
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check ❓ Inconclusive The linked issue VMUX-67 is too generic without specific coding requirements, making full compliance validation inconclusive. Clarify specific coding requirements in VMUX-67 or link more specific issues detailing the DDP-to-REST migration requirements.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: migrating Android VoIP accept/reject signaling from DDP (WebSocket) to REST, which is the primary objective of this PR.
Out of Scope Changes check ✅ Passed All changes remain within scope: MediaCallsAnswerRequest REST client, VoipNotification handler migrations, and DDPClient singleton optimization. startListeningForCallEnd and DDPClient deletion are correctly deferred.

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

@diegolmello
Copy link
Copy Markdown
Member Author

File: VoipNotification.kt — changed methods

handleDeclineAction (Slice 3)

@JvmStatic
fun handleDeclineAction(context: Context, payload: VoipPayload) {
    Log.d(TAG, "Decline action triggered for callId: ${payload.callId}")
    cancelTimeout(payload.callId)
    val deviceId = Settings.Secure.getString(context.contentResolver, Settings.Secure.ANDROID_ID)
    MediaCallsAnswerRequest.fetch(
        context = context,
        host = payload.host,
        callId = payload.callId,
        contractId = deviceId,
        answer = "reject",
        supportedFeatures = null
    ) { _ -> }
    rejectIncomingCall(payload.callId)
    cancelById(context, payload.notificationId)
    LocalBroadcastManager.getInstance(context).sendBroadcast(
        Intent(ACTION_DISMISS).apply {
            putExtras(payload.toBundle())
        }
    )
}

handleAcceptAction (Slice 2)

@JvmStatic
@JvmOverloads
fun handleAcceptAction(context: Context, payload: VoipPayload, skipLaunchMainActivity: Boolean = false) {
    Log.d(TAG, "Accept action triggered for callId: ${payload.callId}")
    cancelTimeout(payload.callId)

    val appCtx = context.applicationContext
    val finished = AtomicBoolean(false)
    val timeoutHandler = Handler(Looper.getMainLooper())
    var timeoutRunnable: Runnable? = null

    fun finish(ddpSuccess: Boolean) {
        if (!finished.compareAndSet(false, true)) return
        timeoutRunnable?.let { timeoutHandler.removeCallbacks(it) }
        ddpRegistry.stopClient(payload.callId)
        if (ddpSuccess) {
            answerIncomingCall(payload.callId)
            VoipModule.storeInitialEvents(payload)
        } else {
            Log.d(TAG, "Native accept did not succeed for ${payload.callId}; opening app for JS recovery")
            disconnectIncomingCall(payload.callId, false)
            VoipModule.storeAcceptFailureForJs(payload)
        }
        cancelById(appCtx, payload.notificationId)
        LocalBroadcastManager.getInstance(appCtx).sendBroadcast(
            Intent(ACTION_DISMISS).apply {
                putExtras(payload.toBundle())
            }
        )
        if (!skipLaunchMainActivity) {
            launchMainActivityForVoip(context, payload)
        }
    }

    val postedTimeout = Runnable {
        Log.w(TAG, "Native accept timed out for ${payload.callId}; falling back to JS recovery")
        finish(false)
    }
    timeoutRunnable = postedTimeout
    timeoutHandler.postDelayed(postedTimeout, 10_000L)

    val deviceId = Settings.Secure.getString(context.contentResolver, Settings.Secure.ANDROID_ID)
    MediaCallsAnswerRequest.fetch(
        context = context,
        host = payload.host,
        callId = payload.callId,
        contractId = deviceId,
        answer = "accept",
        supportedFeatures = listOf("audio")
    ) { success ->
        finish(success)
    }
}

rejectBusyCall (Slice 4)

/**
 * Rejects an incoming call because the user is already on another call.
 *
 * Uses [MediaCallsAnswerRequest] over REST to send the reject signal.
 * Unlike [startListeningForCallEnd] (used by the normal incoming-call path), this
 * does NOT subscribe to `stream-notify-user` or install a collection-message
 * handler, because no incoming-call UI was ever shown and there is nothing
 * to dismiss if the caller hangs up or another device answers.
 */
@JvmStatic
fun rejectBusyCall(context: Context, payload: VoipPayload) {
    Log.d(TAG, "Rejected busy call ${payload.callId} — user already on a call")
    cancelTimeout(payload.callId)
    val deviceId = Settings.Secure.getString(context.contentResolver, Settings.Secure.ANDROID_ID)
    MediaCallsAnswerRequest.fetch(
        context = context,
        host = payload.host,
        callId = payload.callId,
        contractId = deviceId,
        answer = "reject",
        supportedFeatures = null
    ) { _ -> }
}

Deleted methods (Slice 5)

The following dead DDP methods were removed from VoipNotification:

  • sendAcceptSignal()
  • queueAcceptSignal()
  • sendRejectSignal()
  • queueRejectSignal()
  • buildAcceptSignalParams()
  • buildRejectSignalParams()
  • resolveVoipMediaCallIdentity()
  • connectAndRejectBusy() (removed in Slice 4)

Retained (still used by startListeningForCallEnd):

  • ddpRegistry property
  • isLiveClient() helper
  • flushPendingQueuedSignalsIfNeeded()

@diegolmello diegolmello requested a deployment to experimental_android_build April 10, 2026 12:55 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to official_android_build April 10, 2026 12:55 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to experimental_ios_build April 10, 2026 12:55 — with GitHub Actions Waiting
@diegolmello diegolmello had a problem deploying to experimental_android_build April 10, 2026 12:57 — with GitHub Actions Error
@diegolmello diegolmello had a problem deploying to experimental_ios_build April 10, 2026 12:57 — with GitHub Actions Error
@diegolmello diegolmello had a problem deploying to official_android_build April 10, 2026 12:57 — with GitHub Actions Error
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (2)

153-161: Consider logging REST failures for decline action.

The callback { _ -> } silently discards the result. While fire-and-forget semantics may be acceptable for decline (local UI proceeds regardless), logging failures would aid debugging when reject signals don't reach the server.

Proposed fix
             MediaCallsAnswerRequest.fetch(
                 context = context,
                 host = payload.host,
                 callId = payload.callId,
                 contractId = deviceId,
                 answer = "reject",
                 supportedFeatures = null
-            ) { _ -> }
+            ) { success ->
+                if (!success) {
+                    Log.w(TAG, "REST reject failed for ${payload.callId}; local decline proceeds")
+                }
+            }
🤖 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/VoipNotification.kt`
around lines 153 - 161, The current decline call to
MediaCallsAnswerRequest.fetch in VoipNotification.kt discards the result via `{
_ -> }`; update the callback to capture the result or error, and log failures
using the app logger (include context such as payload.callId and deviceId) so
rejected signals that fail to reach the server are recorded; modify the
anonymous callback passed to MediaCallsAnswerRequest.fetch (the closure after
answer="reject") to inspect the response/error and call the existing logging
facility rather than ignoring it.

418-426: Same suggestion: log failures for busy-call rejection.

Consistent with the decline path, logging REST failures here would help diagnose issues where busy-call rejections don't reach the server.

Proposed fix
             MediaCallsAnswerRequest.fetch(
                 context = context,
                 host = payload.host,
                 callId = payload.callId,
                 contractId = deviceId,
                 answer = "reject",
                 supportedFeatures = null
-            ) { _ -> }
+            ) { success ->
+                if (!success) {
+                    Log.w(TAG, "REST reject (busy) failed for ${payload.callId}")
+                }
+            }
🤖 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/VoipNotification.kt`
around lines 418 - 426, The busy-call rejection path currently calls
MediaCallsAnswerRequest.fetch without logging failures; update the callback
passed to MediaCallsAnswerRequest.fetch (in VoipNotification.kt where deviceId
and payload.callId are used) to handle the response/error and call the same
logger used on the decline path (e.g., process or repository logger) to record
any non-success or exception details, including host, callId and the error
message so REST failures are visible in logs. Ensure you preserve the existing
parameters (context, host, callId, contractId=deviceId, answer="reject") and
only add failure logging in the completion lambda.
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (1)

82-95: Unused context parameter.

The context parameter is passed to execute() but never used — Ejson is instantiated without it. If Ejson relies on static/shared storage keyed by host, consider removing the parameter to avoid confusion. Otherwise, if Ejson should use the context, pass it to the constructor.

🤖 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/MediaCallsAnswerRequest.kt`
around lines 82 - 95, The execute function's context parameter is unused; either
remove it from the execute signature and all callers, or thread it into Ejson by
constructing Ejson with the context (e.g., Ejson(context).apply { host = host })
so the instance can access Android resources; update the execute method
signature and every place that calls MediaCallsAnswerRequest.execute, or change
the Ejson instantiation inside execute to accept context, and ensure imports and
constructors are updated accordingly.
🤖 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/MediaCallsAnswerRequest.kt`:
- Around line 110-123: Replace per-request OkHttpClient instantiation with a
shared/reused client (e.g., a companion object or singleton HttpClient) instead
of calling OkHttpClient() inside MediaCallsAnswerRequest; also ensure the
okhttp3.Response is closed in onResponse to avoid leaks (use response.close() or
response.body?.close() / response.use { ... } before returning) and continue to
pass the success boolean to onResult; locate these changes around the
newCall(request).enqueue callback and the onFailure/onResponse overrides
referenced in the diff.

---

Nitpick comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt`:
- Around line 82-95: The execute function's context parameter is unused; either
remove it from the execute signature and all callers, or thread it into Ejson by
constructing Ejson with the context (e.g., Ejson(context).apply { host = host })
so the instance can access Android resources; update the execute method
signature and every place that calls MediaCallsAnswerRequest.execute, or change
the Ejson instantiation inside execute to accept context, and ensure imports and
constructors are updated accordingly.

In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 153-161: The current decline call to MediaCallsAnswerRequest.fetch
in VoipNotification.kt discards the result via `{ _ -> }`; update the callback
to capture the result or error, and log failures using the app logger (include
context such as payload.callId and deviceId) so rejected signals that fail to
reach the server are recorded; modify the anonymous callback passed to
MediaCallsAnswerRequest.fetch (the closure after answer="reject") to inspect the
response/error and call the existing logging facility rather than ignoring it.
- Around line 418-426: The busy-call rejection path currently calls
MediaCallsAnswerRequest.fetch without logging failures; update the callback
passed to MediaCallsAnswerRequest.fetch (in VoipNotification.kt where deviceId
and payload.callId are used) to handle the response/error and call the same
logger used on the decline path (e.g., process or repository logger) to record
any non-success or exception details, including host, callId and the error
message so REST failures are visible in logs. Ensure you preserve the existing
parameters (context, host, callId, contractId=deviceId, answer="reject") and
only add failure logging in the completion lambda.
🪄 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: ee27a59b-e07a-4dfa-8c3d-34dd29c7989a

📥 Commits

Reviewing files that changed from the base of the PR and between 1110468 and 8d543e4.

📒 Files selected for processing (2)
  • android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.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/java/chat/rocket/reactnative/voip/VoipNotification.kt
🔇 Additional comments (3)
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (1)

1-53: Clean implementation of the REST client.

The overall structure is well-designed: good KDoc documentation, clear separation between the factory method and instance execution, and proper credential resolution pattern matching the existing ReplyBroadcast approach.

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

276-286: Accept path handles REST result correctly.

The accept flow properly integrates with the existing finish() callback pattern, preserving the 10-second timeout and JS recovery path on failure. The ddpRegistry.stopClient call in finish() appropriately cleans up any DDP listener started by startListeningForCallEnd.


395-403: Retained helper for DDP subscription flush.

This function is correctly preserved for use in startListeningForCallEnd — it flushes any queued DDP method calls after login completes. Since call-end detection remains on DDP (deferred slice), this is still needed.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)

48-48: Protect connect() against duplicate calls.

Line 48 overwrites the current webSocket reference. If the same DDPClient reconnects without a prior disconnect(), the old listener can keep delivering messages into the same callback state. Either reject re-entry up front or tag listener callbacks with a connection generation before swapping the socket.

🤖 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/DDPClient.kt` at line
48, The connect() method currently assigns a new WebSocket to the webSocket
field unconditionally which can overwrite an active socket and keep old
WebSocketListener callbacks delivering messages into the new state; either
prevent re-entrancy by checking webSocket (or a boolean isConnected) and
returning/erroring if already connected, or implement a connection
generation/tag (e.g., connectionId or generation counter stored on DDPClient)
and have the anonymous WebSocketListener capture that generation and ignore
callbacks if it does not match before swapping webSocket; update disconnect() to
clear/reset the flag or advance the generation so stale listeners are ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Line 48: The connect() method currently assigns a new WebSocket to the
webSocket field unconditionally which can overwrite an active socket and keep
old WebSocketListener callbacks delivering messages into the new state; either
prevent re-entrancy by checking webSocket (or a boolean isConnected) and
returning/erroring if already connected, or implement a connection
generation/tag (e.g., connectionId or generation counter stored on DDPClient)
and have the anonymous WebSocketListener capture that generation and ignore
callbacks if it does not match before swapping webSocket; update disconnect() to
clear/reset the flag or advance the generation so stale listeners are ignored.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ccc4ee55-682e-4ba5-8fd4-2faba85f9f0e

📥 Commits

Reviewing files that changed from the base of the PR and between a611d73 and 38ab136.

📒 Files selected for processing (2)
  • android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt
  • android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
  • android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.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). (2)
  • GitHub Check: ESLint and Test / run-eslint-and-test
  • GitHub Check: format
🧰 Additional context used
🧠 Learnings (1)
📓 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
🔇 Additional comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)

24-30: This ownership shift looks good.

Making the transport instance shared removes the need for one DDPClient to tear it down for every other caller.

Adds android equivalent of ios/Shared/RocketChat/API/MediaCallsAnswerRequest.swift.
POST /api/v1/media-calls.answer with { callId, contractId, type, answer, supportedFeatures }.
Auth via Ejson (x-user-id / x-auth-token) — same pattern as ReplyBroadcast.
Unblocks Slices 2, 3, 4.
…swerRequest

Address CodeRabbit resource leak warning: reuse the OkHttpClient singleton
instead of creating a new instance per request, and wrap the Response in
response.use {} to ensure the body is closed after use.
@diegolmello diegolmello force-pushed the refactor.ddp-android-2 branch from 38ab136 to b85201a Compare April 10, 2026 21:07
@diegolmello diegolmello temporarily deployed to official_android_build April 10, 2026 21:10 — with GitHub Actions Inactive
@diegolmello diegolmello had a problem deploying to experimental_ios_build April 10, 2026 21:10 — with GitHub Actions Error
@diegolmello diegolmello temporarily deployed to experimental_android_build April 10, 2026 21:10 — with GitHub Actions Inactive
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (1)

47-49: Constrain answer and gate supportedFeatures by answer type.

answer is free-form on Line 47, and supportedFeatures is serialized whenever non-null on Line 75. This allows invalid payloads if a caller passes unexpected combinations.

Proposed hardening diff
 class MediaCallsAnswerRequest(
     private val callId: String,
     private val contractId: String,
-    private val answer: String,
+    private val answer: String,
     private val supportedFeatures: List<String>? = null
 ) {
     companion object {
@@
+        enum class Answer(val value: String) { ACCEPT("accept"), REJECT("reject") }
+
         `@JvmStatic`
         fun fetch(
             context: android.content.Context,
             host: String,
             callId: String,
             contractId: String,
-            answer: String,
+            answer: Answer,
             supportedFeatures: List<String>? = null,
             onResult: (Boolean) -> Unit
         ) {
-            val request = MediaCallsAnswerRequest(callId, contractId, answer, supportedFeatures)
+            val request = MediaCallsAnswerRequest(callId, contractId, answer.value, supportedFeatures)
             request.execute(context, host, onResult)
         }
     }
@@
-        supportedFeatures?.let { features ->
-            val arr = JSONArray()
-            features.forEach { arr.put(it) }
-            json.put("supportedFeatures", arr)
-        }
+        if (answer == "accept") {
+            supportedFeatures?.let { features ->
+                val arr = JSONArray()
+                features.forEach { arr.put(it) }
+                json.put("supportedFeatures", arr)
+            }
+        }
         return json
     }

Also applies to: 75-79

🤖 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/MediaCallsAnswerRequest.kt`:
- Around line 111-123: The OkHttp callbacks in MediaCallsAnswerRequest (methods
onFailure and onResponse) call onResult directly on a background thread; change
both calls to post to the main thread using Handler(Looper.getMainLooper()).post
{ ... } so that onResult (and downstream VoipNotification.finish) runs on the UI
thread; update the onFailure block and the onResponse block to wrap their
onResult(...) invocations in Handler(Looper.getMainLooper()).post {
onResult(...) }.
🪄 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: 469fe359-318f-486a-b399-39328e9bcaf3

📥 Commits

Reviewing files that changed from the base of the PR and between 38ab136 and b85201a.

📒 Files selected for processing (3)
  • android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt
  • android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt
  • android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
🚧 Files skipped from review as they are similar to previous changes (2)
  • android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.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). (2)
  • GitHub Check: ESLint and Test / run-eslint-and-test
  • GitHub Check: format
🧰 Additional context used
🧠 Learnings (1)
📓 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
🔇 Additional comments (2)
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (2)

92-96: Credential guard looks solid.

Failing fast when x-user-id/x-auth-token are missing (Line 92) is the right behavior for this path and avoids pointless network attempts.


117-123: Great response lifecycle handling.

Using response.use { ... } on Line 118 correctly closes the response and prevents resource leaks.

@diegolmello diegolmello had a problem deploying to upload_official_android April 10, 2026 21:53 — with GitHub Actions Error
@github-actions
Copy link
Copy Markdown

OkHttp callbacks run on a background thread. Wrapping onResult in
mainHandler.post {} ensures finish() in VoipNotification runs on the
main thread — LocalBroadcastManager.sendBroadcast(), VoiceConnection
callbacks, and startActivity() all require it.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (1)

95-98: ⚠️ Potential issue | 🟠 Major

Post credential-failure onResult to main thread to maintain callback contract consistency.

Line 97 calls onResult(false) directly on the caller's thread, while both network paths (lines 117 and 125) post onResult to mainHandler. This inconsistency violates the callback contract—downstream code expecting main-thread delivery may encounter thread-safety issues or incorrect behavior.

Proposed fix
         if (userId.isNullOrEmpty() || token.isNullOrEmpty()) {
             Log.w(TAG, "Missing credentials for $host — cannot send media-call answer")
-            onResult(false)
+            mainHandler.post { onResult(false) }
             return
         }
🤖 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/MediaCallsAnswerRequest.kt`
around lines 95 - 98, In MediaCallsAnswerRequest, the early-return path after
checking userId/token calls onResult(false) directly on the caller thread;
change this to post the callback to mainHandler just like the network paths do
to maintain main-thread delivery consistency—locate the null/empty check around
TAG log warning and replace the direct onResult(false) with mainHandler.post {
onResult(false) } so all onResult invocations (including the credential-failure
case) are executed on the main thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt`:
- Around line 95-98: In MediaCallsAnswerRequest, the early-return path after
checking userId/token calls onResult(false) directly on the caller thread;
change this to post the callback to mainHandler just like the network paths do
to maintain main-thread delivery consistency—locate the null/empty check around
TAG log warning and replace the direct onResult(false) with mainHandler.post {
onResult(false) } so all onResult invocations (including the credential-failure
case) are executed on the main thread.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8900a49e-7cd3-4597-8938-176cc3e77ceb

📥 Commits

Reviewing files that changed from the base of the PR and between b85201a and dbdda14.

📒 Files selected for processing (1)
  • android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.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). (2)
  • GitHub Check: ESLint and Test / run-eslint-and-test
  • GitHub Check: format
🧰 Additional context used
🧠 Learnings (1)
📓 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
🔇 Additional comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (1)

114-127: Good improvements in HTTP execution path.

Shared OkHttpClient, response.use { ... }, and main-thread posting in onFailure/onResponse are solid and reduce leak/threading risk.

@diegolmello
Copy link
Copy Markdown
Member Author

Call Signaling Flow — Before vs After

Before (DDP WebSocket)

handleAcceptAction
  → ddpRegistry.isLoggedIn?
      ├─ YES: sendAcceptSignal() → client.callMethod() → ddpRegistry.stopClient()
      └─ NO: queueAcceptSignal() → client.queueMethodCall() → ddpRegistry.stopClient()
  → finish(success)

After (REST)

handleAcceptAction
  → MediaCallsAnswerRequest.fetch(answer="accept")
      → POST /api/v1/media-calls.answer
      → mainHandler.post { onResult(success) }   ← background thread → main thread
  → finish(success)   ← always on main thread

What changed

  • Removed: connectAndRejectBusy, sendAcceptSignal, queueAcceptSignal, sendRejectSignal, queueRejectSignal, buildAcceptSignalParams, buildRejectSignalParams, resolveVoipMediaCallIdentity
  • Added: MediaCallsAnswerRequest.kt — REST client with shared OkHttpClient, auth from Ejson, main-thread dispatch
  • handleDeclineAction and rejectBusyCall now use the same REST path
  • DDPClient now shares a single OkHttpClient across all connections (ping 30s)

Deferred (separate PRD)

  • Slice 6: startListeningForCallEnd off DDP
  • Slice 7: Delete DDPClient.kt / VoipPerCallDdpRegistry.kt

Server rejects request with "must NOT have additional properties" —
"type" is not part of the server schema. iOS implementation does not
include it; match that behavior.
@diegolmello diegolmello requested a deployment to approve_e2e_testing April 13, 2026 17:20 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to experimental_android_build April 13, 2026 17:24 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to official_android_build April 13, 2026 17:24 — with GitHub Actions Waiting
@diegolmello diegolmello requested a deployment to experimental_ios_build April 13, 2026 17:24 — with GitHub Actions Waiting
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