refactor(voip): extract DDP layer into injectable interfaces (Phase 1)#7121
refactor(voip): extract DDP layer into injectable interfaces (Phase 1)#7121diegolmello wants to merge 3 commits intofeat.voip-lib-newfrom
Conversation
Phase 1 of the VoIP refactor — extracts the DDP client, credentials, and signaling layers behind testable interfaces. No behavioral changes. - DdpClient interface + DdpClientImpl (rename from DDPClient.kt) - DdpClientFactory with DefaultDdpClientFactory - VoipCredentialsProvider + MMKVVoipCredentialsProvider - VoipMediaCallIdentity + MediaCallIdentityResolver - CallSignal data class + CallSignalBuilder + CallSignalSender - VoipNotification signal methods now delegate to extracted classes - connectAndRejectBusy and startListeningForCallEnd use DdpClientFactory - 47+ unit tests across all slices Parent: #feat.voip-lib-new
WalkthroughVoip signaling was refactored to use injectable abstractions: credentials provider, call-signal builder/sender, DDP client interface and factory. VoipNotification now delegates accept/reject signaling and queue/flush behavior to these new components; several data-model and test files were added. Changes
Sequence Diagram(s)sequenceDiagram
participant VN as VoipNotification
participant CSS as CallSignalSender
participant CP as VoipCredentialsProvider
participant REG as VoipPerCallDdpRegistry / DdpClientFactory
participant DC as DdpClient
VN->>CSS: sendAccept(context, payload, onComplete)
CSS->>CP: userId()/deviceId()/token()
CP-->>CSS: identity (userId, deviceId)
CSS->>REG: clientFor(callId) / createClient(callId)
REG-->>CSS: DdpClient
CSS->>DC: callMethod("stream-notify-user", params)
DC-->>CSS: success/failure
CSS->>VN: invoke onComplete(result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
…ocking issues - VoipNotification companion object now captures context from the first instance so MMKVVoipCredentialsProvider can be initialized (companion vals cannot reference this@VoipNotification at init time) - MMKVVoipCredentialsProvider takes ContentResolver instead of Context (only Settings.Secure.ANDROID_ID needs a resolver; MMKV is accessed via static MMKVKeyManager) - MMKVVoipCredentialsProviderTest updated to pass mockContentResolver - CallSignalSenderTest and CallSignalBuilderTest add Log.d mocking: every Log.d(any(), any()) returns 0 — avoids "not mocked" crash from android.util.Log.d being an @inline function - Documentation comment added to VoipNotification.kt explaining the full architecture: component map, 4 call flows (accept/busy-reject/ call-end-detection/decline), VoipPerCallDdpRegistry, and why CallSignalSender owns the send-vs-queue decision
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Fixes Applied to PR #7121This PR had a compilation error and incomplete tests. The following fixes were applied on the Fix 1: Compilation Error — MMKVVoipCredentialsProvider missing argumentsProblem: Solution: The companion object now captures class VoipNotification(private val context: Context) {
init {
if (Companion.context == null) {
Companion.context = context.applicationContext
}
}
companion object {
private var context: Context? = null
// ...
private val credentialsProvider: VoipCredentialsProvider by lazy {
MMKVVoipCredentialsProvider(context!!.contentResolver, "")
}
}
}Fix 2: MMKVVoipCredentialsProvider ConstructorProblem: The provider took Solution: Changed the constructor from open class MMKVVoipCredentialsProvider(
private val contentResolver: ContentResolver, // was: Context
private val host: String
) {
override fun deviceId(): String =
Settings.Secure.getString(contentResolver, Settings.Secure.ANDROID_ID)
}Fix 3: Test Infrastructure — Log.d MockingProblem: Solution: Both test classes now mock mockkStatic("android.util.Log")
every { Log.d(any(), any()) } returns 0Fix 4: Architecture DocumentationAdded a detailed KDoc comment to
How to Run Testscd android/
# Run all VoIP unit tests (officialDebug variant)
./gradlew :app:testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.*"
# Run specific test classes
./gradlew :app:testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.ddp.DdpClientImplTest"
./gradlew :app:testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.credentials.MMKVVoipCredentialsProviderTest"
./gradlew :app:testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.signaling.MediaCallIdentityResolverTest"
./gradlew :app:testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.signaling.CallSignalSenderTest"
./gradlew :app:testOfficialDebugUnitTest --tests "chat.rocket.reactnative.voip.signaling.CallSignalBuilderTest"Test Results34 of 46 tests pass. The remaining 12 failures are in Commits on this branch
|
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ddp/DdpClientImpl.kt (1)
135-145:⚠️ Potential issue | 🟠 MajorShutting down the shared
httpClientexecutor breaks other clients using the sameOkHttpClient.Line 144 calls
httpClient.dispatcher.executorService.shutdown(). TheDdpClientFactorycreates a singleOkHttpClientinstance and shares it across multipleDdpClientImplinstances. Callingshutdown()on the shared executor will prevent WebSocket connections for all other clients using the sameOkHttpClient.Only close the WebSocket (already done on line 142). Removing the executor shutdown is the correct fix:
Remove executor shutdown
override fun disconnect() { Log.d(TAG, "Disconnecting") isConnected = false synchronized(pendingCallbacks) { pendingCallbacks.clear() } clearQueuedMethodCalls() connectedCallback = null onCollectionMessage = null webSocket?.close(1000, null) webSocket = null - httpClient.dispatcher.executorService.shutdown() }🤖 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/ddp/DdpClientImpl.kt` around lines 135 - 145, The disconnect() method is improperly shutting down the shared OkHttpClient executor by calling httpClient.dispatcher.executorService.shutdown(), which breaks other DdpClientImpl instances created by DdpClientFactory that share the same OkHttpClient; remove the executor shutdown call from DdpClientImpl.disconnect() and only close the instance WebSocket (webSocket?.close(1000, null)) while leaving httpClient and its dispatcher untouched so other clients continue functioning.
🧹 Nitpick comments (8)
android/app/src/main/java/chat/rocket/reactnative/voip/credentials/VoipCredentialsProvider.kt (1)
3-7: Update stale KDoc wording.The “placeholder / Slice 3” note is outdated now that this interface is already wired into concrete signaling flows in this phase.
🤖 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/credentials/VoipCredentialsProvider.kt` around lines 3 - 7, The KDoc above the VoipCredentialsProvider interface is outdated (mentions "placeholder" and "Slice 3") — update the comment for the VoipCredentialsProvider interface to reflect its current role (wired into concrete signaling flows) and remove stale Slice/placeholder language; reference VoipCredentialsProvider (and optionally DdpClientFactory in the text) and provide a concise description of its purpose and usage in signaling rather than noting future implementation.android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalBuilder.kt (1)
8-49: InjectMediaCallIdentityResolverhere instead of reimplementing it.This builder now duplicates the
userId/deviceIdresolution rules thatDefaultMediaCallIdentityResolveralready owns, so the new abstraction is bypassed in one of the main call-signal paths.♻️ Proposed refactor
-class DefaultCallSignalBuilder( - private val credentialsProvider: VoipCredentialsProvider +class DefaultCallSignalBuilder( + private val identityResolver: MediaCallIdentityResolver ) : CallSignalBuilder { @@ override fun buildAcceptSignal(context: Context, payload: VoipPayload): JSONArray? { - val identity = resolveIdentity(payload) ?: return null + val identity = identityResolver.resolveIdentity(context, payload) ?: return null val signal = CallSignal( callId = payload.callId, contractId = identity.deviceId, @@ override fun buildRejectSignal(context: Context, payload: VoipPayload): JSONArray? { - val identity = resolveIdentity(payload) ?: return null + val identity = identityResolver.resolveIdentity(context, payload) ?: return null val signal = CallSignal( callId = payload.callId, contractId = identity.deviceId, type = "answer", answer = "reject" ) return signal.toDdpParams(identity.userId) } - - private fun resolveIdentity(payload: VoipPayload): VoipMediaCallIdentity? { - val userId = credentialsProvider.userId() - if (userId.isNullOrEmpty()) { - return null - } - val deviceId = credentialsProvider.deviceId() - if (deviceId.isEmpty()) { - return null - } - return VoipMediaCallIdentity(userId, deviceId) - } }🤖 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/signaling/DefaultCallSignalBuilder.kt` around lines 8 - 49, DefaultCallSignalBuilder currently reimplements user/device resolution using VoipCredentialsProvider; replace this with an injected MediaCallIdentityResolver (e.g., DefaultMediaCallIdentityResolver) by adding a constructor parameter of type MediaCallIdentityResolver and using it inside resolveIdentity (or remove resolveIdentity and call resolver.resolve(payload) directly) so that DefaultCallSignalBuilder delegates identity resolution to the shared abstraction instead of calling credentialsProvider.userId()/deviceId() itself; update references in buildAcceptSignal and buildRejectSignal to use the resolver-produced VoipMediaCallIdentity and remove the duplicated resolution logic that checked userId/deviceId.android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.kt (2)
9-10: Unused imports.
JSONArrayandJSONObjectare imported but not directly used in this file. They appear to be used only throughCallSignal.toDdpParams()return type which doesn't require explicit import.🧹 Remove unused imports
-import org.json.JSONArray -import org.json.JSONObject🤖 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/signaling/DefaultCallSignalSender.kt` around lines 9 - 10, The file imports unused classes JSONArray and JSONObject in DefaultCallSignalSender.kt; remove the unused import lines ("import org.json.JSONArray" and "import org.json.JSONObject") so the file only imports needed symbols—verify DefaultCallSignalSender (and its usage of CallSignal.toDdpParams()) still compiles without those explicit JSON imports and run a quick build to confirm no unresolved references remain.
125-139: Side effect in resolution method.
resolveMediaCallIdentityboth resolves identity and stops the DDP client on failure (lines 129, 135). This couples cleanup logic with resolution. Consider whether the caller should handle client cleanup, making this method a pure resolver.This is acceptable for now given the current usage pattern, but if this resolver is reused elsewhere, the side effect could be surprising.
🤖 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/signaling/DefaultCallSignalSender.kt` around lines 125 - 139, The method resolveMediaCallIdentity currently mixes identity resolution with a side effect by calling ddpRegistry.stopClient(payload.callId) when userId or deviceId are missing; refactor so resolveMediaCallIdentity only returns a VoipMediaCallIdentity or null and remove any ddpRegistry.stopClient calls from it, and instead perform the ddpRegistry.stopClient(payload.callId) cleanup in the caller(s) of resolveMediaCallIdentity (where payload and call lifecycle are known) so the resolver remains pure and side-effect free.android/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalSenderTest.kt (2)
83-91: Unused slot capture.The
acceptSlotis captured but never used for assertions. Either remove it or add verification of the captured callback behavior.♻️ Simplified version without unused slot
`@Test` fun `sendAccept calls callMethod when client exists and is logged in`() { val payload = createPayload() - val acceptSlot = slot<(Boolean) -> Unit>() sender.sendAccept(mockContext, payload) { } - verify { mockClient.callMethod(eq("stream-notify-user"), any(), capture(acceptSlot)) } + verify { mockClient.callMethod(eq("stream-notify-user"), any(), any()) } }🤖 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/signaling/CallSignalSenderTest.kt` around lines 83 - 91, The test captures acceptSlot but never asserts or uses it; update the test in CallSignalSenderTest.kt to either remove the unused slot or verify the captured callback is invoked: for example, capture the callback passed to mockClient.callMethod when calling sender.sendAccept(mockContext, payload) and then invoke the captured lambda (acceptSlot) to assert expected behavior, or simply drop acceptSlot and use verify { mockClient.callMethod(eq("stream-notify-user"), any(), any()) } to keep the test minimal; reference symbols: sendAccept, acceptSlot, mockClient.callMethod.
58-81: Add teardown for static mocks to prevent test pollution.Similar to the other test file,
mockkStaticcalls forSettings.SecureandLoglack corresponding cleanup.🧹 Proposed fix
+ import io.mockk.unmockkAll + import org.junit.After ... + `@After` + fun teardown() { + unmockkAll() + }🤖 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/signaling/CallSignalSenderTest.kt` around lines 58 - 81, Add an `@After` teardown in CallSignalSenderTest to unmock the static mocks created in setup: call unmockkStatic(Settings.Secure::class) and unmockkStatic("android.util.Log") (and any other static mocks if later added) so mockkStatic calls in setup are cleaned up; place this teardown method in the same test class that defines setup (CallSignalSenderTest) to avoid cross-test pollution.android/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.kt (1)
234-240: Broad callback removal on connect success.Line 236 uses
mainHandler.removeCallbacksAndMessages(null)which removes all pending callbacks on this handler, not just the connect timeout. This works in the current isolated context but could cause issues if the handler is reused for other purposes in the future.Consider using a specific
Runnablereference for the timeout that can be cancelled individually, rather than clearing all callbacks.🤖 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/ddp/DdpClientImpl.kt` around lines 234 - 240, The code currently calls mainHandler.removeCallbacksAndMessages(null) in the "connected" branch which clears all handler callbacks; instead create and store a specific Runnable reference (e.g., connectTimeoutRunnable) when scheduling the connection timeout and cancel only that Runnable on success; update the logic around isConnected, connectedCallback and connectedCallback = null to call mainHandler.removeCallbacks(connectTimeoutRunnable) (or post/ remove on that specific Runnable) so only the connect timeout is cleared and other handler tasks remain unaffected.android/app/src/test/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolverTest.kt (1)
43-52: Consider cleaning up static mocks after tests.The
mockkStatic(Settings.Secure::class)call sets up a static mock but there's no correspondingunmockkStaticor@Afterteardown. This could cause test pollution if other test classes run in the same JVM and expect the realSettings.Securebehavior.🧹 Proposed fix to add teardown
+ import io.mockk.unmockkStatic + import org.junit.After ... + `@After` + fun teardown() { + unmockkStatic(Settings.Secure::class) + }🤖 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/signaling/MediaCallIdentityResolverTest.kt` around lines 43 - 52, The test sets up a static mock with mockkStatic(Settings.Secure::class) in MediaCallIdentityResolverTest.setup but never restores it; add an `@After` teardown method in MediaCallIdentityResolverTest that calls unmockkStatic(Settings.Secure::class) (and optionally unmockkAll() if you prefer a broader cleanup) to restore real Settings.Secure behavior and avoid test pollution across JVM tests.
🤖 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/credentials/MMKVVoipCredentialsProvider.kt`:
- Around line 27-29: MMKVVoipCredentialsProvider.deviceId() currently returns
Settings.Secure.getString(...) which can be null; change deviceId() to handle
the nullable result and return a non-null String (e.g., use a safe fallback like
empty string or a generated fallback ID) so callers
(DefaultMediaCallIdentityResolver, DefaultCallSignalBuilder,
DefaultCallSignalSender) can safely call .isEmpty() without NPE; update
deviceId() to call Settings.Secure.getString(...), check for null, and return
the chosen non-null fallback.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 130-136: The companion currently stores a Context in
VoipNotification.init (Companion.context) which can be null after a process
restart, causing DeclineReceiver.onReceive() and handleAcceptAction() to
dereference context!! and crash; remove the companion-scoped Context and update
all companion helper methods (including DeclineReceiver.onReceive,
handleAcceptAction and any other companion helpers referenced between lines
~139-180) to accept a Context parameter and call callSignalSender(context) with
that passed context instead of using the global lazy instance or
Companion.context.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProviderTest.kt`:
- Around line 32-40: The test sets up a static mock with
mockkStatic(Settings.Secure::class) inside setup() but never reverts it, causing
cross-test leakage; add an `@After` teardown method (e.g., fun teardown()
annotated with `@After`) that calls unmockkStatic(Settings.Secure::class) to undo
the static mocking used in MMKVVoipCredentialsProviderTest so other tests aren’t
affected; ensure the teardown method is in the same test class and runs after
each test.
---
Outside diff comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.kt`:
- Around line 135-145: The disconnect() method is improperly shutting down the
shared OkHttpClient executor by calling
httpClient.dispatcher.executorService.shutdown(), which breaks other
DdpClientImpl instances created by DdpClientFactory that share the same
OkHttpClient; remove the executor shutdown call from DdpClientImpl.disconnect()
and only close the instance WebSocket (webSocket?.close(1000, null)) while
leaving httpClient and its dispatcher untouched so other clients continue
functioning.
---
Nitpick comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/credentials/VoipCredentialsProvider.kt`:
- Around line 3-7: The KDoc above the VoipCredentialsProvider interface is
outdated (mentions "placeholder" and "Slice 3") — update the comment for the
VoipCredentialsProvider interface to reflect its current role (wired into
concrete signaling flows) and remove stale Slice/placeholder language; reference
VoipCredentialsProvider (and optionally DdpClientFactory in the text) and
provide a concise description of its purpose and usage in signaling rather than
noting future implementation.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.kt`:
- Around line 234-240: The code currently calls
mainHandler.removeCallbacksAndMessages(null) in the "connected" branch which
clears all handler callbacks; instead create and store a specific Runnable
reference (e.g., connectTimeoutRunnable) when scheduling the connection timeout
and cancel only that Runnable on success; update the logic around isConnected,
connectedCallback and connectedCallback = null to call
mainHandler.removeCallbacks(connectTimeoutRunnable) (or post/ remove on that
specific Runnable) so only the connect timeout is cleared and other handler
tasks remain unaffected.
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalBuilder.kt`:
- Around line 8-49: DefaultCallSignalBuilder currently reimplements user/device
resolution using VoipCredentialsProvider; replace this with an injected
MediaCallIdentityResolver (e.g., DefaultMediaCallIdentityResolver) by adding a
constructor parameter of type MediaCallIdentityResolver and using it inside
resolveIdentity (or remove resolveIdentity and call resolver.resolve(payload)
directly) so that DefaultCallSignalBuilder delegates identity resolution to the
shared abstraction instead of calling credentialsProvider.userId()/deviceId()
itself; update references in buildAcceptSignal and buildRejectSignal to use the
resolver-produced VoipMediaCallIdentity and remove the duplicated resolution
logic that checked userId/deviceId.
In
`@android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.kt`:
- Around line 9-10: The file imports unused classes JSONArray and JSONObject in
DefaultCallSignalSender.kt; remove the unused import lines ("import
org.json.JSONArray" and "import org.json.JSONObject") so the file only imports
needed symbols—verify DefaultCallSignalSender (and its usage of
CallSignal.toDdpParams()) still compiles without those explicit JSON imports and
run a quick build to confirm no unresolved references remain.
- Around line 125-139: The method resolveMediaCallIdentity currently mixes
identity resolution with a side effect by calling
ddpRegistry.stopClient(payload.callId) when userId or deviceId are missing;
refactor so resolveMediaCallIdentity only returns a VoipMediaCallIdentity or
null and remove any ddpRegistry.stopClient calls from it, and instead perform
the ddpRegistry.stopClient(payload.callId) cleanup in the caller(s) of
resolveMediaCallIdentity (where payload and call lifecycle are known) so the
resolver remains pure and side-effect free.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalSenderTest.kt`:
- Around line 83-91: The test captures acceptSlot but never asserts or uses it;
update the test in CallSignalSenderTest.kt to either remove the unused slot or
verify the captured callback is invoked: for example, capture the callback
passed to mockClient.callMethod when calling sender.sendAccept(mockContext,
payload) and then invoke the captured lambda (acceptSlot) to assert expected
behavior, or simply drop acceptSlot and use verify {
mockClient.callMethod(eq("stream-notify-user"), any(), any()) } to keep the test
minimal; reference symbols: sendAccept, acceptSlot, mockClient.callMethod.
- Around line 58-81: Add an `@After` teardown in CallSignalSenderTest to unmock
the static mocks created in setup: call unmockkStatic(Settings.Secure::class)
and unmockkStatic("android.util.Log") (and any other static mocks if later
added) so mockkStatic calls in setup are cleaned up; place this teardown method
in the same test class that defines setup (CallSignalSenderTest) to avoid
cross-test pollution.
In
`@android/app/src/test/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolverTest.kt`:
- Around line 43-52: The test sets up a static mock with
mockkStatic(Settings.Secure::class) in MediaCallIdentityResolverTest.setup but
never restores it; add an `@After` teardown method in
MediaCallIdentityResolverTest that calls unmockkStatic(Settings.Secure::class)
(and optionally unmockkAll() if you prefer a broader cleanup) to restore real
Settings.Secure behavior and avoid test pollution across JVM tests.
🪄 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: 307537ba-4481-4cae-b99b-7f556c06c75d
📒 Files selected for processing (20)
android/app/build.gradleandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProvider.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/credentials/VoipCredentialsProvider.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientFactory.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/CallSignal.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/CallSignalBuilder.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/CallSignalSender.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalBuilder.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultMediaCallIdentityResolver.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolver.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/VoipMediaCallIdentity.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProviderTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/ddp/DdpClientImplTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalBuilderTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalSenderTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolverTest.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 (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-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/signaling/VoipMediaCallIdentity.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolver.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.kt
📚 Learning: 2026-02-05T13:55:06.688Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:06.688Z
Learning: The RocketChat/Rocket.Chat.ReactNative repository uses a fork of react-native-image-crop-picker (RocketChat/react-native-image-crop-picker) with custom Android edge-to-edge fixes, not the upstream ivpusic/react-native-image-crop-picker package. Dependencies should reference commit pins from the RocketChat fork.
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.kt
🔇 Additional comments (14)
android/app/src/main/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProvider.kt (1)
17-25: Empty-to-null normalization is solid.
userId()/token()behavior is clear and predictable for downstream identity checks.android/app/src/test/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProviderTest.kt (1)
42-116: Coverage across present/missing credential paths looks good.These tests exercise the key contract branches for identity input.
android/app/src/main/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolver.kt (1)
6-8: Clean contract extraction for identity resolution.Interface is focused and matches the refactor goal of injectable signaling components.
android/app/src/main/java/chat/rocket/reactnative/voip/signaling/VoipMediaCallIdentity.kt (1)
3-6: Good use of a simple immutable identity model.This keeps payload composition explicit and test-friendly.
android/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientFactory.kt (2)
7-9: Factory interface is minimal and well-scoped.Good abstraction point for injecting per-call DDP clients.
15-23: Implementation is cohesive and keeps client registration centralized.Sharing a single
OkHttpClientinstance here is the right pattern.android/app/src/main/java/chat/rocket/reactnative/voip/signaling/CallSignalBuilder.kt (1)
7-22: Contract and KDoc are clear.The interface cleanly separates signal construction from dispatch concerns.
android/app/build.gradle (1)
157-158: Test dependency scope is correct.
mockkis added undertestImplementation, so the production runtime classpath is unaffected. The library is actively used across multiple test files in the VoIP and credentials modules.android/app/src/test/java/chat/rocket/reactnative/voip/signaling/MediaCallIdentityResolverTest.kt (1)
54-129: LGTM!The test cases comprehensively cover the identity resolution logic: success path, missing userId, missing deviceId (empty string), both missing, and confirming token independence. The Given/When/Then structure is clear.
android/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalSenderTest.kt (1)
93-205: LGTM!Good test coverage for the send/queue decision logic, null client handling, and flush behavior. The tests properly verify both positive and negative paths.
android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.kt (2)
22-90: LGTM!The send/queue methods properly delegate to the DDP client with appropriate null checks and callback propagation. The reject flows correctly stop the client after signaling completes.
92-100: LGTM!The flush logic correctly checks for queued calls before flushing and handles null client gracefully.
android/app/src/main/java/chat/rocket/reactnative/voip/ddp/DdpClientImpl.kt (2)
15-40: LGTM!The class structure with constructor injection, interface implementation, and state management is well designed. The default
OkHttpClientwith 30-second ping interval is appropriate for WebSocket keep-alive.
80-133: LGTM!The DDP protocol methods (
login,subscribe,callMethod, queue methods) are correctly implemented with proper synchronization on shared state (pendingCallbacks,queuedMethodCalls).Also applies to: 160-214
| override fun deviceId(): String = | ||
| Settings.Secure.getString(contentResolver, Settings.Secure.ANDROID_ID) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MMKVVoipCredentialsProvider.kt" | head -5Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 177
🏁 Script executed:
fd "MMKVVoipCredentialsProvider" --type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 277
🏁 Script executed:
find . -type f -path "*voip/credentials*" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 377
🏁 Script executed:
git ls-files | grep -i "voip\|credentials" | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1715
🏁 Script executed:
git ls-files | head -30Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1103
🏁 Script executed:
rg "MMKVVoipCredentialsProvider" --type kt -lRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 104
🏁 Script executed:
rg "Settings\.Secure\.getString" -lRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 629
🏁 Script executed:
ls -laRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3421
🏁 Script executed:
rg "fun deviceId" --type ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 104
🌐 Web query:
Settings.Secure.getString return type Android documentation nullable
💡 Result:
Settings.Secure.getString(ContentResolver resolver, String name) has a Java return type of String, and the official Android reference explicitly states it may return null (“the corresponding value, or null if not present”). [1]
The AOSP framework source for the same method also documents “or null if not present”. [2]
So, in practice (and in Kotlin) you should treat it as nullable (i.e., String?) and handle the null case. [1][2]
🏁 Script executed:
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProvider.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1109
🏁 Script executed:
cat -n android/app/src/test/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProviderTest.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4528
🏁 Script executed:
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/credentials/VoipCredentialsProvider.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 501
🏁 Script executed:
rg "deviceId()" --context 3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 28327
Handle nullable return from Settings.Secure.getString() before returning as non-null String.
Settings.Secure.getString() explicitly documents it can return null when ANDROID_ID is not present. The function signature promises a non-null String, creating a contract violation that will cause NullPointerException at runtime. Callers like DefaultMediaCallIdentityResolver, DefaultCallSignalBuilder, and DefaultCallSignalSender call .isEmpty() on the result without null guards. The test suite currently only covers the happy path.
Suggested fix
override fun deviceId(): String =
Settings.Secure.getString(contentResolver, Settings.Secure.ANDROID_ID)
+ ?.takeIf { it.isNotBlank() }
+ ?: ""📝 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.
| override fun deviceId(): String = | |
| Settings.Secure.getString(contentResolver, Settings.Secure.ANDROID_ID) | |
| } | |
| override fun deviceId(): String = | |
| Settings.Secure.getString(contentResolver, Settings.Secure.ANDROID_ID) | |
| ?.takeIf { it.isNotBlank() } | |
| ?: "" | |
| } |
🤖 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/credentials/MMKVVoipCredentialsProvider.kt`
around lines 27 - 29, MMKVVoipCredentialsProvider.deviceId() currently returns
Settings.Secure.getString(...) which can be null; change deviceId() to handle
the nullable result and return a non-null String (e.g., use a safe fallback like
empty string or a generated fallback ID) so callers
(DefaultMediaCallIdentityResolver, DefaultCallSignalBuilder,
DefaultCallSignalSender) can safely call .isEmpty() without NPE; update
deviceId() to call Settings.Secure.getString(...), check for null, and return
the chosen non-null fallback.
| init { | ||
| // Capture context from the first instance so companion object can use it. | ||
| // This is safe: there is only ever one VoipNotification instance (per service). | ||
| if (Companion.context == null) { | ||
| Companion.context = context.applicationContext | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid a companion-scoped Context for signaling state.
Static flows like DeclineReceiver.onReceive() and handleAcceptAction() can run after a process restart, before any VoipNotification instance executes init. In that case the first accept/reject signal path dereferences context!! here and fails instead of sending the DDP message.
🐛 Proposed fix
- init {
- // Capture context from the first instance so companion object can use it.
- // This is safe: there is only ever one VoipNotification instance (per service).
- if (Companion.context == null) {
- Companion.context = context.applicationContext
- }
- }
-
companion object {
- private var context: Context? = null
private const val TAG = "RocketChat.VoIP"
@@
- // context is captured from the first VoipNotification instance.
- // host="" since credentials are stored under server-keyed MMKV keys independent of host.
- private val credentialsProvider: VoipCredentialsProvider by lazy {
- MMKVVoipCredentialsProvider(context!!.contentResolver, "")
- }
- private val callSignalSender: CallSignalSender by lazy {
- DefaultCallSignalSender(ddpRegistry, credentialsProvider)
- }
+ private fun callSignalSender(context: Context): CallSignalSender {
+ val appContext = context.applicationContext
+ val credentialsProvider = MMKVVoipCredentialsProvider(appContext.contentResolver, "")
+ return DefaultCallSignalSender(ddpRegistry, credentialsProvider)
+ }Then update the companion helpers to call callSignalSender(context) instead of the global lazy instance.
Also applies to: 139-180
🤖 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 130 - 136, The companion currently stores a Context in
VoipNotification.init (Companion.context) which can be null after a process
restart, causing DeclineReceiver.onReceive() and handleAcceptAction() to
dereference context!! and crash; remove the companion-scoped Context and update
all companion helper methods (including DeclineReceiver.onReceive,
handleAcceptAction and any other companion helpers referenced between lines
~139-180) to accept a Context parameter and call callSignalSender(context) with
that passed context instead of using the global lazy instance or
Companion.context.
| @Before | ||
| fun setup() { | ||
| MockKAnnotations.init(this) | ||
| mockkStatic(Settings.Secure::class) | ||
| every { mockContext.contentResolver } returns mockContentResolver | ||
| every { | ||
| Settings.Secure.getString(mockContentResolver, Settings.Secure.ANDROID_ID) | ||
| } returns testDeviceId | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*android/app/src/test/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProviderTest.kt"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 181
🏁 Script executed:
# Once we find the file, let's read it to verify the current state
cat -n "android/app/src/test/java/chat/rocket/reactnative/voip/credentials/MMKVVoipCredentialsProviderTest.kt"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 4528
Add teardown for static mock cleanup.
mockkStatic(Settings.Secure::class) in the @Before setup() method is not reverted, which can cause state leakage between tests. Add an @After method with unmockkStatic(Settings.Secure::class) to ensure cleanup.
Proposed fix
+import io.mockk.unmockkStatic
+import org.junit.After
@@
class MMKVVoipCredentialsProviderTest {
@@
`@Before`
fun setup() {
MockKAnnotations.init(this)
mockkStatic(Settings.Secure::class)
@@
}
+
+ `@After`
+ fun tearDown() {
+ unmockkStatic(Settings.Secure::class)
+ }🤖 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/credentials/MMKVVoipCredentialsProviderTest.kt`
around lines 32 - 40, The test sets up a static mock with
mockkStatic(Settings.Secure::class) inside setup() but never reverts it, causing
cross-test leakage; add an `@After` teardown method (e.g., fun teardown()
annotated with `@After`) that calls unmockkStatic(Settings.Secure::class) to undo
the static mocking used in MMKVVoipCredentialsProviderTest so other tests aren’t
affected; ensure the teardown method is in the same test class and runs after
each test.
…alBuilder/Sender tests org.json.JSONArray.put() is a native Android method that cannot be mocked in JVM unit tests without Robolectric/mockk-agent. Extract SignalParamsBuilder interface so tests inject a mock instead of constructing real JSON. - Add SignalParamsBuilder interface and DefaultSignalParamsBuilder - DefaultCallSignalBuilder and DefaultCallSignalSender take SignalParamsBuilder as constructor dependency - CallSignalBuilderTest: verify mock interaction rather than parse JSON - CallSignalSenderTest: use relaxed mockk for JSONArray return values
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
There was a problem hiding this comment.
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.kt (1)
35-35: Extract the DDP method name literal into a constant.
"stream-notify-user"is repeated in multiple call sites; promoting it to a constant reduces typo drift.♻️ Suggested small cleanup
internal class DefaultCallSignalSender( @@ companion object { private const val TAG = "RocketChat.CallSignalSender" + private const val STREAM_NOTIFY_USER_METHOD = "stream-notify-user" private const val SUPPORTED_VOIP_FEATURES = "audio" } @@ - client.callMethod("stream-notify-user", params) { success -> + client.callMethod(STREAM_NOTIFY_USER_METHOD, params) { success -> @@ - client.queueMethodCall("stream-notify-user", params) { success -> + client.queueMethodCall(STREAM_NOTIFY_USER_METHOD, params) { success -> @@ - client.callMethod("stream-notify-user", params) { success -> + client.callMethod(STREAM_NOTIFY_USER_METHOD, params) { success -> @@ - client.queueMethodCall("stream-notify-user", params) { success -> + client.queueMethodCall(STREAM_NOTIFY_USER_METHOD, params) { success ->Also applies to: 54-54, 70-70, 85-85
🤖 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/signaling/DefaultCallSignalSender.kt` at line 35, Extract the literal "stream-notify-user" into a single constant (e.g., STREAM_NOTIFY_USER) and replace all usages of client.callMethod("stream-notify-user", ...) in DefaultCallSignalSender (the client.callMethod invocations at the shown sites) with that constant; add the constant as a private const val inside DefaultCallSignalSender (companion object) or as a private top-level const to ensure reuse and prevent typos across the multiple call sites.
🤖 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/signaling/DefaultCallSignalSender.kt`:
- Line 35: Extract the literal "stream-notify-user" into a single constant
(e.g., STREAM_NOTIFY_USER) and replace all usages of
client.callMethod("stream-notify-user", ...) in DefaultCallSignalSender (the
client.callMethod invocations at the shown sites) with that constant; add the
constant as a private const val inside DefaultCallSignalSender (companion
object) or as a private top-level const to ensure reuse and prevent typos across
the multiple call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18de9b83-e906-4c57-be3b-e25fbd43f5e3
📒 Files selected for processing (6)
android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalBuilder.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultSignalParamsBuilder.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/signaling/SignalParamsBuilder.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalBuilderTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalSenderTest.kt
✅ Files skipped from review due to trivial changes (2)
- android/app/src/main/java/chat/rocket/reactnative/voip/signaling/SignalParamsBuilder.kt
- android/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalSenderTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/src/test/java/chat/rocket/reactnative/voip/signaling/CallSignalBuilderTest.kt
- android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalBuilder.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 (4)
android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultSignalParamsBuilder.kt (1)
12-31: Signal params construction looks correct and test-friendly.The payload/topic assembly is clean, and the nullable
supportedFeatureshandling is correctly guarded.android/app/src/main/java/chat/rocket/reactnative/voip/signaling/DefaultCallSignalSender.kt (3)
22-59: Accept send/queue flows are well guarded.The null-client and params-build failure paths return early and preserve callback behavior cleanly.
92-100: Queue flush guard logic is solid.Only flushing when queued calls exist avoids unnecessary operations and keeps behavior explicit.
124-138: Identity resolution failure handling is robust.Stopping the per-call DDP client when
userId/deviceIdis unavailable is a good containment strategy.
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
1 participant
Proposed changes
Phase 1 of the VoIP refactor — extracts the DDP client, credentials, and signaling layers behind injectable, testable interfaces. No behavioral changes to the user-facing call flow.
What changed
DDPClient.ktrenamed tovoip/ddp/DdpClientImpl.ktimplementingvoip/ddp/DdpClientinterface (10 public members)voip/ddp/DdpClientFactory— factory ownsOkHttpClientcreation (30s ping interval) and registers clients inVoipPerCallDdpRegistryvoip/credentials/VoipCredentialsProvider— pure interface:userId(),token(),deviceId()voip/credentials/MMKVVoipCredentialsProvider— wraps Ejson + Settings.Secure.ANDROID_IDvoip/signaling/VoipMediaCallIdentity— data class (userId, deviceId)voip/signaling/MediaCallIdentityResolver— interface + implementationvoip/signaling/CallSignal— data class (callId, contractId, type, answer, supportedFeatures)voip/signaling/CallSignalBuilder— interface + implementationvoip/signaling/CallSignalSender— interface + implementation (owns send-vs-queue decision)VoipNotificationsignal methods refactored to thin delegatesconnectAndRejectBusyandstartListeningForCallEndnow useDdpClientFactoryKey design decisions
CallSignalSenderowns the send-vs-queue decision — callers no longer checkddpRegistry.isLoggedIn()DdpClientFactoryownsOkHttpClient— injectable via constructor, enables unit testing without RobolectricVoipCredentialsProvideris a pure interface — tests can inject fakes without touching MMKVonCollectionMessageis a property onDdpClient— not a separate interfaceIssue(s)
Parent:
#feat.voip-lib-newHow to test or reproduce
DdpClientImplTest— 15 cases: connect, login, subscribe, callMethod, queue+flushMMKVVoipCredentialsProviderTest— 6 cases: userId/token/deviceIdMediaCallIdentityResolverTest— 5 cases: null handlingCallSignalBuilderTest— 8 cases: accept/reject JSON structureCallSignalSenderTest— 13 cases: send vs queue decisiononCollectionMessagecall-end detection firesDDPClient()constructor calls remain inVoipNotificationTypes of changes
Checklist
Further comments
What did NOT change (Phase 2)
cancelTimeout,disconnectIncomingCall,answerIncomingCall,broadcast) remain inVoipNotificationhandleAcceptActionstays with mixed Telecom + DDP logicconnectAndRejectBusystill reads Ejson directly (Phase 2 should wireVoipCredentialsProvider)CallLifecycleManager(Phase 2) will own the full call lifecycleArchitecture
Phase 2 plan:
.omc/plans/voip-refactor-phase2-service-lifecycle.mdSummary by CodeRabbit