refactor(voip): extract interfaces and add unit tests for Android DDP layer#7143
Open
diegolmello wants to merge 17 commits intofeat.voip-lib-newfrom
Open
refactor(voip): extract interfaces and add unit tests for Android DDP layer#7143diegolmello wants to merge 17 commits intofeat.voip-lib-newfrom
diegolmello wants to merge 17 commits intofeat.voip-lib-newfrom
Conversation
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.
Add 5 new tests to VoipPerCallDdpRegistryTest: thread safety with concurrent put/stop, clientCount/clientIds correctness, stop non-existent on empty registry, stop non-existent with other clients, and stopAllClients clears loggedIn state.
Add mockk 1.13.16 test dependency. Remove dead SUPPORTED_VOIP_FEATURES constant and flushPendingQueuedSignalsIfNeeded() method — both orphaned after REST migration of accept/reject.
Extract media-signal parsing from VoipNotification.onCollectionMessage into CallEndSignalDetector with evaluate() API returning NOT_CALL_END, OTHER_DEVICE_ACCEPTED, or CALLER_HUNG_UP. Add 9 unit tests.
Extract credential resolution from VoipNotification into VoipCredentialsProvider interface + EjsonVoipCredentialsProvider impl. startListeningForCallEnd accepts provider param with default.
Extract DdpClient interface from DDPClient class. Rename class to DdpClientImpl implementing DdpClient. Both in DDPClient.kt to avoid macOS case-insensitive filesystem collision. Update VoipNotification references to use interface type.
Add 9 unit tests for MediaCallsAnswerRequest covering accept/reject body shape, auth headers, success/failure callbacks, missing credentials, and URL normalization. Make buildBody() and httpClient internal for test access.
Add 26 unit tests for DdpClientImpl covering connect/login/subscribe/ callMethod, queue+flush+clear, ping/pong, disconnect, onCollectionMessage, and buildWebSocketURL. Uses mockk for OkHttp/Handler mocking.
Add DdpClientFactory with injectable OkHttpClient. DdpClientImpl now accepts OkHttpClient constructor param. Wire factory into startListeningForCallEnd via Kotlin default parameter. Add 4 factory unit tests.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 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)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
WalkthroughRefactors native DDP client into an interface + implementation, introduces credential provider and factory patterns, adds call-end signal detector, adjusts VoIP notification wiring to use injectable providers, exposes some MediaCallsAnswerRequest internals for testing, and adds extensive unit tests and test deps. Changes
Sequence Diagram(s)sequenceDiagram
participant VN as VoipNotification
participant CP as VoipCredentialsProvider (Ejson)
participant DF as DdpClientFactory
participant DC as DdpClient (WebSocket)
participant DET as CallEndSignalDetector
VN->>CP: request token/userId/deviceId
CP-->>VN: returns token, userId, deviceId
VN->>DF: create()
DF-->>VN: returns DdpClientImpl
VN->>DC: connect(host)
DC-->>VN: connected event
VN->>DC: login(token)
VN->>DC: subscribe("stream-room-messages", ...)
DC-->>VN: onCollectionMessage(message)
VN->>DET: evaluate(message) with callId, deviceId
DET-->>VN: Result (NOT_CALL_END / OTHER_DEVICE_ACCEPTED / CALLER_HUNG_UP)
alt CALL_END detected
VN->>VN: cancel timeouts, disconnect call, cancel notification
VN->>DC: stop/cleanup client
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labelstype: chore 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
diegolmello
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
55-84:⚠️ Potential issue | 🟠 MajorOnly complete the connect callback while the handshake is still pending.
onFailure()currently postscallback(false)unconditionally. That can double-fire the initial connect callback when the socket dies afteronOpen()but before"connected", and it can also report a failed connect after a previously successful handshake.Proposed fix
webSocket = client.newWebSocket(request, object : WebSocketListener() { override fun onOpen(webSocket: WebSocket, response: Response) { Log.d(TAG, "WebSocket opened") val connectMsg = JSONObject().apply { @@ override fun onFailure(webSocket: WebSocket, t: Throwable, response: Response?) { Log.e(TAG, "WebSocket failure: ${t.message}") - isConnected = false - mainHandler.post { callback(false) } + val wasConnected = isConnected + isConnected = false + val pendingConnectCallback = connectedCallback + connectedCallback = null + if (!wasConnected) { + mainHandler.post { (pendingConnectCallback ?: callback)(false) } + } }🤖 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` around lines 55 - 84, The onFailure handler currently always posts callback(false) which can double-fire the initial connect callback or report failure after a successful handshake; change onFailure in the WebSocketListener so it only invokes mainHandler.post { callback(false) } when the handshake hasn't completed (e.g., guard with if (!isConnected) before posting), keep setting isConnected = false as needed, and rely on waitForConnected/onOpen to complete the callback when the handshake succeeds.
🧹 Nitpick comments (2)
android/app/src/test/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistryTest.kt (1)
85-111: Make executor shutdown failure-safe in concurrency test.If an assertion fails before
shutdown(), threads can leak and affect subsequent tests. Move pool shutdown intofinally(or useshutdownNow()in failure paths).Safer teardown pattern
- repeat(threads) { t -> - pool.submit { - try { - for (i in 0 until iterations) { - val callId = "call-${t}-${i}" - reg.putClient(callId, "client-${t}-${i}") - if (i % 3 == 0) reg.stopClient(callId) - } - } finally { - latch.countDown() - } - } - } - - assertTrue(latch.await(10, TimeUnit.SECONDS)) - pool.shutdown() + try { + repeat(threads) { t -> + pool.submit { + try { + for (i in 0 until iterations) { + val callId = "call-${t}-${i}" + reg.putClient(callId, "client-${t}-${i}") + if (i % 3 == 0) reg.stopClient(callId) + } + } finally { + latch.countDown() + } + } + } + assertTrue(latch.await(10, TimeUnit.SECONDS)) + } finally { + pool.shutdownNow() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/test/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistryTest.kt` around lines 85 - 111, The test VoipPerCallDdpRegistryTest::`concurrent put and stop does not crash or corrupt state` can leak threads if an assertion fails before pool.shutdown(); wrap the executor lifecycle in a try/finally so pool.shutdown()/pool.shutdownNow() is always called: move pool.shutdown() into a finally block, call pool.shutdownNow() if awaitTermination times out or an exception occurs, and still assert latch.await(...) and perform pool.awaitTermination(...) to ensure threads are terminated; reference the local variables pool, latch, threads, iterations, and the test method name when applying the change.android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt (1)
39-40: Prefer avoiding mutable globalhttpClientstate.
internal varin companion creates cross-call shared mutable state. Consider injecting client per request/factory with an immutable default to keep tests isolated without widening production mutability.Refactor sketch (immutable default + injectable override)
class MediaCallsAnswerRequest( private val callId: String, private val contractId: String, private val answer: String, - private val supportedFeatures: List<String>? = null + private val supportedFeatures: List<String>? = null, + private val httpClient: OkHttpClient = defaultHttpClient ) { companion object { private const val TAG = "RocketChat.MediaCallsAnswerRequest" private val JSON_MEDIA_TYPE = "application/json; charset=utf-8".toMediaType() - `@JvmStatic` - internal var httpClient: OkHttpClient = OkHttpClient() + `@JvmStatic` + internal val defaultHttpClient: OkHttpClient = OkHttpClient() `@JvmStatic` fun fetch( context: android.content.Context, host: String, callId: String, contractId: String, answer: String, supportedFeatures: List<String>? = null, + httpClient: OkHttpClient = defaultHttpClient, onResult: (Boolean) -> Unit ) { - val request = MediaCallsAnswerRequest(callId, contractId, answer, supportedFeatures) + val request = MediaCallsAnswerRequest(callId, contractId, answer, supportedFeatures, httpClient) request.execute(context, host, onResult) } }🤖 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 39 - 40, The companion-level mutable httpClient (internal var httpClient) creates shared mutable state; change it to an immutable default and allow injection per request: make the companion hold a private val DEFAULT_HTTP_CLIENT = OkHttpClient() and update MediaCallsAnswerRequest (or its request factory/method) to accept an OkHttpClient parameter (optional, defaulting to DEFAULT_HTTP_CLIENT) so callers/tests can provide a custom client without widening global mutability.
🤖 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/VoipCredentialsProvider.kt`:
- Around line 3-6: The interface VoipCredentialsProvider declares deviceId():
String but Android's Settings.Secure.getString(...) can return null, causing a
contract violation; update the contract by making deviceId() nullable
(deviceId(): String?) or explicitly document and implement a guaranteed fallback
inside implementations (e.g., generate/persist a UUID) and adjust all usages of
VoipCredentialsProvider.deviceId to handle null or the fallback; reference the
VoipCredentialsProvider interface and Settings.Secure.getString when applying
the change.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/DdpClientFactoryTest.kt`:
- Around line 6-11: The test imports for MockK are missing the any() matcher;
update the imports in DdpClientFactoryTest.kt to include io.mockk.any so the
stubs and verifications that reference any() (used in the mock setups and verify
calls) compile and run correctly—look for usages of any() in the test stubs and
verify blocks and add the import alongside other io.mockk imports.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequestTest.kt`:
- Around line 6-12: The test file MediaCallsAnswerRequestTest.kt is missing
MockK helper imports causing compilation failures; add the missing imports for
any, anyConstructed, and capture from MockK (e.g., import io.mockk.any, import
io.mockk.anyConstructed, import io.mockk.capture) so usages like any(),
anyConstructed<Ejson>(), and capture(slot) resolve; update the import block
alongside the existing mockk imports (mockk, mockkConstructor, mockkStatic,
slot, unmockkAll, verify, every) to include these helpers.
---
Outside diff comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 55-84: The onFailure handler currently always posts
callback(false) which can double-fire the initial connect callback or report
failure after a successful handshake; change onFailure in the WebSocketListener
so it only invokes mainHandler.post { callback(false) } when the handshake
hasn't completed (e.g., guard with if (!isConnected) before posting), keep
setting isConnected = false as needed, and rely on waitForConnected/onOpen to
complete the callback when the handshake succeeds.
---
Nitpick comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.kt`:
- Around line 39-40: The companion-level mutable httpClient (internal var
httpClient) creates shared mutable state; change it to an immutable default and
allow injection per request: make the companion hold a private val
DEFAULT_HTTP_CLIENT = OkHttpClient() and update MediaCallsAnswerRequest (or its
request factory/method) to accept an OkHttpClient parameter (optional,
defaulting to DEFAULT_HTTP_CLIENT) so callers/tests can provide a custom client
without widening global mutability.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistryTest.kt`:
- Around line 85-111: The test VoipPerCallDdpRegistryTest::`concurrent put and
stop does not crash or corrupt state` can leak threads if an assertion fails
before pool.shutdown(); wrap the executor lifecycle in a try/finally so
pool.shutdown()/pool.shutdownNow() is always called: move pool.shutdown() into a
finally block, call pool.shutdownNow() if awaitTermination times out or an
exception occurs, and still assert latch.await(...) and perform
pool.awaitTermination(...) to ensure threads are terminated; reference the local
variables pool, latch, threads, iterations, and the test method name when
applying the change.
🪄 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: f460a446-780e-421e-8dbc-6f1a7a90d131
📒 Files selected for processing (15)
android/app/build.gradleandroid/app/src/main/java/chat/rocket/reactnative/voip/CallEndSignalDetector.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/DdpClientFactory.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/EjsonVoipCredentialsProvider.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequest.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipCredentialsProvider.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistry.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/CallEndSignalDetectorTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DdpClientFactoryTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DdpClientImplTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/EjsonVoipCredentialsProviderTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequestTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistryTest.kt
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📓 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-03-31T11:59:31.061Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: android/build.gradle:3-8
Timestamp: 2026-03-31T11:59:31.061Z
Learning: In the RocketChat/Rocket.Chat.ReactNative repository, the React Native upgrade helper (https://react-native-community.github.io/upgrade-helper/?from=0.79.4&to=0.81.5) recommends kotlinVersion = "2.1.20", compileSdkVersion = 36, targetSdkVersion = 36, and buildToolsVersion = "36.0.0" in android/build.gradle for the RN 0.79.4 → 0.81.5 upgrade. These are the sanctioned values for this upgrade path and should not be flagged as compatibility concerns.
Applied to files:
android/app/build.gradle
📚 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/VoipCredentialsProvider.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistryTest.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
🔇 Additional comments (5)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipPerCallDdpRegistry.kt (1)
4-4: KDoc type reference fix is correct.Nice cleanup — the symbol now matches the actual
DdpClienttype name.android/app/build.gradle (1)
158-159: Test dependency additions are appropriately scoped.Using
testImplementationhere keeps runtime APK surface unchanged while enabling the new unit tests.android/app/src/main/java/chat/rocket/reactnative/voip/DdpClientFactory.kt (1)
5-8: Factory injection pattern looks solid.This is a clean testability/DI improvement and keeps construction concerns centralized.
android/app/src/test/java/chat/rocket/reactnative/voip/EjsonVoipCredentialsProviderTest.kt (1)
33-55: Coverage is well-targeted for provider behavior.Good set of tests for delegation and null propagation, including the Android ID branch.
android/app/src/main/java/chat/rocket/reactnative/voip/EjsonVoipCredentialsProvider.kt (1)
12-16: Helper extraction is a nice cleanup.Centralizing
Ejsoncreation inejson()makes the delegation methods simpler and less repetitive.
Comment on lines
+3
to
+6
| interface VoipCredentialsProvider { | ||
| fun userId(): String? | ||
| fun token(): String? | ||
| fun deviceId(): String |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to official Android documentation, can Settings.Secure.getString(ContentResolver, Settings.Secure.ANDROID_ID) return null? If yes, list the documented conditions.
💡 Result:
Yes, according to official Android documentation, Settings.Secure.getString(ContentResolver, Settings.Secure.ANDROID_ID) can return null. The documented condition is when the setting is not present in the database.
Citations:
- 1: https://developer.android.google.cn/reference/kotlin/android/provider/Settings.Secure
- 2: https://developer.android.com/reference/android/provider/Settings.Secure
🏁 Script executed:
# Find and inspect the VoipCredentialsProvider file
find . -type f -name "VoipCredentialsProvider.kt" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 161
🏁 Script executed:
# Also search for any implementations of VoipCredentialsProvider
rg "implements VoipCredentialsProvider|: VoipCredentialsProvider" -t kt -A 10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 104
🏁 Script executed:
# Read the VoipCredentialsProvider.kt file
cat -n ./android/app/src/main/java/chat/rocket/reactnative/voip/VoipCredentialsProvider.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 280
🏁 Script executed:
# Search for implementations using broader pattern matching
rg "VoipCredentialsProvider" --type java -A 15 | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Search for implementations in broader scope
rg "class.*implements|: VoipCredentialsProvider" --type java -A 20 -B 2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 10521
🏁 Script executed:
# Also search within voip directory for deviceId implementations
find ./android/app/src/main/java/chat/rocket/reactnative/voip -type f -name "*.java" -o -name "*.kt" | xargs grep -l "deviceId" 2>/dev/nullRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 409
🏁 Script executed:
# Read the implementation that handles deviceId
cat -n ./android/app/src/main/java/chat/rocket/reactnative/voip/EjsonVoipCredentialsProvider.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 842
deviceId() contract is too strict for Android ID source.
Returning non-null String here creates a type contract violation: Settings.Secure.getString() is documented to return null when the setting is absent, yet the implementation assigns it directly without null handling. This forces unsafe behavior or crashes at runtime. Make this nullable or define a guaranteed fallback policy.
Proposed contract adjustment
interface VoipCredentialsProvider {
fun userId(): String?
fun token(): String?
- fun deviceId(): String
+ fun deviceId(): String?
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| interface VoipCredentialsProvider { | |
| fun userId(): String? | |
| fun token(): String? | |
| fun deviceId(): String | |
| interface VoipCredentialsProvider { | |
| fun userId(): String? | |
| fun token(): String? | |
| fun deviceId(): String? | |
| } |
🤖 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/VoipCredentialsProvider.kt`
around lines 3 - 6, The interface VoipCredentialsProvider declares deviceId():
String but Android's Settings.Secure.getString(...) can return null, causing a
contract violation; update the contract by making deviceId() nullable
(deviceId(): String?) or explicitly document and implement a guaranteed fallback
inside implementations (e.g., generate/persist a UUID) and adjust all usages of
VoipCredentialsProvider.deviceId to handle null or the fallback; reference the
VoipCredentialsProvider interface and Settings.Secure.getString when applying
the change.
Comment on lines
+6
to
+11
| import io.mockk.every | ||
| import io.mockk.mockk | ||
| import io.mockk.mockkConstructor | ||
| import io.mockk.mockkStatic | ||
| import io.mockk.unmockkAll | ||
| import io.mockk.verify |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
file="android/app/src/test/java/chat/rocket/reactnative/voip/DdpClientFactoryTest.kt"
# Expect: `any(` usages are present, but no `import io.mockk.any`.
rg -n 'import io\.mockk\.|\bany\(' "$file"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 410
Add missing import for any() matcher.
The file uses explicit MockK imports but references any() in the stubs (lines 30–31) and verification (line 63) without importing it. Add import io.mockk.any to the import block.
Proposed fix
+import io.mockk.any
import io.mockk.every
import io.mockk.mockk
import io.mockk.mockkConstructor
import io.mockk.mockkStatic
import io.mockk.unmockkAll
import io.mockk.verify📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import io.mockk.every | |
| import io.mockk.mockk | |
| import io.mockk.mockkConstructor | |
| import io.mockk.mockkStatic | |
| import io.mockk.unmockkAll | |
| import io.mockk.verify | |
| import io.mockk.any | |
| import io.mockk.every | |
| import io.mockk.mockk | |
| import io.mockk.mockkConstructor | |
| import io.mockk.mockkStatic | |
| import io.mockk.unmockkAll | |
| import io.mockk.verify |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/DdpClientFactoryTest.kt`
around lines 6 - 11, The test imports for MockK are missing the any() matcher;
update the imports in DdpClientFactoryTest.kt to include io.mockk.any so the
stubs and verifications that reference any() (used in the mock setups and verify
calls) compile and run correctly—look for usages of any() in the test stubs and
verify blocks and add the import alongside other io.mockk imports.
Comment on lines
+6
to
+12
| import io.mockk.every | ||
| import io.mockk.mockk | ||
| import io.mockk.mockkConstructor | ||
| import io.mockk.mockkStatic | ||
| import io.mockk.slot | ||
| import io.mockk.unmockkAll | ||
| import io.mockk.verify |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
file="android/app/src/test/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequestTest.kt"
# Expect: helper usages below without matching imports for `any`, `capture`, and `anyConstructed`.
rg -n 'import io\.mockk\.|anyConstructed<|capture\(|\bany\(' "$file"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1103
Add the missing MockK imports or this test file won't compile.
The test uses any(), anyConstructed<Ejson>(), and capture(...) at lines 42–216, but these helpers are not imported. Add:
Proposed fix
+import io.mockk.any
+import io.mockk.anyConstructed
+import io.mockk.capture
import io.mockk.every
import io.mockk.mockk
import io.mockk.mockkConstructor
import io.mockk.mockkStatic
import io.mockk.slot
import io.mockk.unmockkAll
import io.mockk.verify🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/MediaCallsAnswerRequestTest.kt`
around lines 6 - 12, The test file MediaCallsAnswerRequestTest.kt is missing
MockK helper imports causing compilation failures; add the missing imports for
any, anyConstructed, and capture from MockK (e.g., import io.mockk.any, import
io.mockk.anyConstructed, import io.mockk.capture) so usages like any(),
anyConstructed<Ejson>(), and capture(slot) resolve; update the import block
alongside the existing mockk imports (mockk, mockkConstructor, mockkStatic,
slot, unmockkAll, verify, every) to include these helpers.
Member
Author
CI Flow — Android Unit TestsThe Android unit tests (67 tests across 7 classes in Key points:
Test files added in this PR:
|
Member
Author
Architecture Flowflowchart TB
subgraph VoipNotification["VoipNotification"]
A([onMessageReceived]) --> B{decideIncomingVoipPushAction}
B -->|STALE| B1([skip])
B -->|REJECT_BUSY| C1[rejectBusyCall]
B -->|SHOW_INCOMING| D[showIncomingCall]
D --> D1[registerCallWithTelecomManager]
D --> D2[showIncomingCallNotification]
D --> D3[scheduleTimeout]
D --> D4[startListeningForCallEnd]
end
subgraph startListeningForCallEnd["startListeningForCallEnd"]
E1[VoipCredentialsProvider<br/>EjsonVoipCredentialsProvider]
E2[DdpClientFactory<br/>creates DdpClientImpl]
E3[DdpClient<br/>interface]
E4[VoipPerCallDdpRegistry<br/>per-call client slots]
E1 -->|userId / token / deviceId| E3
E2 -->|create| E3
E3 -->|putClient| E4
E3 -->|onCollectionMessage| E5[CallEndSignalDetector<br/>evaluate]
E5 -->|Result| E6{Result?}
E6 -->|OTHER_DEVICE_ACCEPTED<br/>or CALLER_HUNG_UP| E7[disconnectIncomingCall<br/>cancelTimeout<br/>cancelNotification<br/>ACTION_DISMISS<br/>stopClient]
end
subgraph DDP_Lifecycle["DDP Lifecycle (inside startListeningForCallEnd)"]
F1[connect] -->|WebSocket open| F2[login<br/>resume token]
F2 -->|loggedIn| F3[subscribe<br/>stream-notify-user]
F3 -->|onCollectionMessage| F4[handleMessage<br/>ping/pong, result, ready<br/>changed/added/removed]
F4 -->|collection msg| F5[CallEndSignalDetector]
end
subgraph Actions["Actions (from notification/activity)"]
G1[handleDeclineAction] -->|REST| H1[MediaCallsAnswerRequest<br/>answer: reject]
G2[handleAcceptAction] -->|REST| H2[MediaCallsAnswerRequest<br/>answer: accept]
H2 -->|success| H3[answerIncomingCall<br/>VoiceConnection.onAnswer]
G3[rejectBusyCall] -->|REST| H4[MediaCallsAnswerRequest<br/>answer: reject]
end
subgraph Interfaces["Extracted Interfaces (this PR)"]
I1[DdpClient<br/>interface]
I2[VoipCredentialsProvider<br/>interface]
I3[CallEndSignalDetector<br/>extracted from onMessage parsing]
I4[DdpClientFactory<br/>factory for OkHttpClient/DdpClientImpl]
end
style I1 fill:#bbf,stroke:#0066cc,stroke-width:2px
style I2 fill:#bbf,stroke:#0066cc,stroke-width:2px
style I3 fill:#bbf,stroke:#0066cc,stroke-width:2px
style I4 fill:#bbf,stroke:#0066cc,stroke-width:2px
Key design decisionsConstructor injection with defaults — every injectable dep has a Kotlin default param in No DI framework needed. Tests swap any dep with mocks/fakes. VoipPerCallDdpRegistry — isolates teardown per MediaCallsAnswerRequest — replaces dead DDP accept/reject/busy methods. REST-only via CallEndSignalDetector — pure parsing logic extracted from Test coverage (67 tests, 7 classes)
All deps injectable → every class fully testable without framework. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Refactor the Android native VoIP DDP signaling code behind injectable interfaces with unit test coverage. The remaining WebSocket-based call-end listener (
startListeningForCallEnd) is now clean, testable, and maintainable.Production code:
DdpClientinterface fromDDPClient(renamed toDdpClientImpl)VoipCredentialsProviderinterface +EjsonVoipCredentialsProviderimpl from inline Ejson accessCallEndSignalDetectorfrom inline signal parsing inonCollectionMessageDdpClientFactoryforOkHttpClient/DdpClientImplinstantiationVoipNotificationvia constructor injection with Kotlin default paramsMediaCallsAnswerRequest)Test code (67 tests across 7 classes):
DdpClientImplTest— 26 tests covering DDP protocol (connect, login, subscribe, callMethod, ping/pong, disconnect, queue)CallEndSignalDetectorTest— 9 tests covering signal parsing edge casesMediaCallsAnswerRequestTest— 9 tests covering REST accept/rejectVoipPerCallDdpRegistryTest— 11 tests including thread safetyDdpClientFactoryTest— 4 tests covering factory creation and custom client injectionEjsonVoipCredentialsProviderTest— 4 tests covering credential delegationVoipIncomingCallDispatchTest— 4 tests (pre-existing, unchanged)Note:
flushPendingQueuedSignalsIfNeededremoval is intentional — confirmed zero production callers ofqueueMethodCall.Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-67
How to test or reproduce
Run Android VoIP unit tests:
```bash
cd android && ./gradlew testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.*"
```
No behavioral changes — all refactoring is behind interfaces with default production implementations. Incoming call flow (accept, reject, caller hangup, other-device accept) should work identically.
Screenshots
N/A — no UI changes
Types of changes
Checklist
Further comments
This is the testability/maintainability layer on top of PR #7127 (REST migration). The dependency chain is:
```
feat.voip-lib-new
← refactor.ddp-android-2 (#7127 — REST migration)
← this PR (interface extraction + test coverage)
```
All extracted components use Kotlin default parameter values for DI — no framework needed. In tests, every dependency can be replaced with mocks/fakes.
Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests