fix(voip): Android DDP thread safety and VoipPayload bundle parity - #7168
Conversation
GitHub Actions
Error
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Robolectric test dependency; makes DDPClient ensure single connect outcome delivery with connect-timeout lifecycle, atomic send ids, stale-event guarding, and test hooks; converts VoipModule initial-event storage to an AtomicReference with claim semantics; persists Changes
Sequence Diagram(s)sequenceDiagram
actor App as App/Client
participant DDP as DDPClient
participant WS as WebSocket
participant Handler as MainHandler
participant CB as ConnectCallback
App->>DDP: waitForConnected(callback)
DDP->>Handler: post connect-timeout runnable
DDP->>WS: open/connect()
alt WS receives "connected"
WS->>DDP: onMessage({"msg":"connected"})
DDP->>DDP: tryDeliverConnectOutcome(true)
DDP->>Handler: cancel timeout runnable
DDP->>CB: invoke(callback, true)
else WS failure
WS->>DDP: onFailure(...)
DDP->>DDP: validate webSocket identity
DDP->>DDP: tryDeliverConnectOutcome(false)
DDP->>Handler: cancel timeout runnable
DDP->>CB: invoke(callback, false)
else Timeout fires
Handler->>DDP: timeout runnable runs
DDP->>DDP: tryDeliverConnectOutcome(false)
DDP->>CB: invoke(callback, false)
end
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 |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
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
🤖 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/DDPClient.kt`:
- Line 45: connectTimeoutRunnable is concurrently accessed from OkHttp threads
and the main looper, risking stale reads; modify waitForConnected(),
cancelConnectTimeout(), and the timeout Runnable so they use the same
cross-thread protection as connectedCallback — either marshal all reads/writes
to the main Handler (post/execute mutations on the main looper) or guard
connectTimeoutRunnable and connectedCallback with the same lock (synchronized
block or ReentrantLock) so cancellation cannot be missed and the timeout
Runnable cannot fire after cancelConnectTimeout() returns.
- Around line 74-81: The connect callback is invoked from multiple paths
(onFailure, handleMessage("connected") via connectedCallback, and the
waitForConnected timeout), causing duplicate success/failure reports; introduce
an AtomicBoolean (e.g., connectionResultDelivered) and use compareAndSet(false,
true) at the start of each place that posts the final result (onFailure, the
code that invokes connectedCallback/connectedCallback?.invoke(true) in
handleMessage, and the timeout runnable in waitForConnected) so only the first
outcome is delivered to the caller; ensure each path still cancels the timeout
(cancelConnectTimeout) and clears connectedCallback/connectedResult references
as before when compareAndSet succeeds, and skip invoking
callback/connectedCallback when compareAndSet returns false.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt`:
- Around line 24-25: Replace the Volatile backing field initialEventsData with
an AtomicReference<VoipPayload?> and update callers (storeInitialEvents(),
storeAcceptFailureForJs()) to use AtomicReference.set/get; in getInitialEvents()
read the current payload via initialEventsData.get(), check expiration, and
clear only the exact consumed payload using
initialEventsData.compareAndSet(data, null) so a concurrent writer that replaced
the payload is not lost; ensure you perform the CAS both on expired and
on-successful-consume paths so only the payload you observed is cleared.
🪄 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: 0215890c-29ca-43f0-a9c2-004b6c379df3
📒 Files selected for processing (6)
android/app/build.gradleandroid/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/VoipPayloadBundleTest.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-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/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt
📚 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/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt
🔇 Additional comments (5)
android/app/build.gradle (1)
158-158: Test dependency is correctly scoped.
Robolectricis added astestImplementation, so the new JVM tests are enabled without adding it to the production APK.android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (1)
75-75: Bundle parity looks good.
toBundle()now preservesvoipAcceptFailed, matching the existingfromBundle()path and preventing cold-start accept-failure state loss.android/app/src/test/java/chat/rocket/reactnative/voip/VoipPayloadBundleTest.kt (1)
15-60: Good regression coverage for bundle round-trip.The tests cover both boolean values, and the
falsecase would fail if the key were omitted because it reads with defaulttrue.android/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.kt (1)
19-60: Timeout behavior coverage looks focused.The tests validate success, timeout failure, and that canceling the connect timeout no longer removes unrelated main-handler work.
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
14-14: Atomic message IDs look good.Switching
sendCountertoAtomicIntegermakes concurrent DDP message ID generation safe without changing the generated ID format.Also applies to: 32-32, 158-163
1756096 to
fb4de9e
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
349-357: Prefer@VisibleForTestingfor test-only entry points.These are package-visible via
internalbut aren't conceptually part of the production API surface. Annotating withandroidx.annotation.VisibleForTestingdocuments intent and lets lint warn on accidental production use.♻️ Suggested annotation
+import androidx.annotation.VisibleForTesting @@ - internal fun testStartConnectTimeout(timeoutMs: Long, callback: (Boolean) -> Unit) { + `@VisibleForTesting` + internal fun testStartConnectTimeout(timeoutMs: Long, callback: (Boolean) -> Unit) { resetConnectHandshakeState() waitForConnected(timeoutMs, callback) } - internal fun testDeliverRawMessage(text: String) { + `@VisibleForTesting` + internal fun testDeliverRawMessage(text: String) { handleMessage(text) }🤖 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 349 - 357, Annotate the test-only entry points with androidx.annotation.VisibleForTesting to document intent and enable lint checks: add `@VisibleForTesting` (importing androidx.annotation.VisibleForTesting) above the internal functions testStartConnectTimeout and testDeliverRawMessage in DDPClient (DDPClient.kt) so they remain package-visible but are marked explicitly as testing-only API.
🤖 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`:
- Around line 349-357: Annotate the test-only entry points with
androidx.annotation.VisibleForTesting to document intent and enable lint checks:
add `@VisibleForTesting` (importing androidx.annotation.VisibleForTesting) above
the internal functions testStartConnectTimeout and testDeliverRawMessage in
DDPClient (DDPClient.kt) so they remain package-visible but are marked
explicitly as testing-only API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c11971fd-36af-451b-ac8d-cd9a19a55119
📒 Files selected for processing (6)
android/app/build.gradleandroid/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/VoipPayloadBundleTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt
- android/app/src/test/java/chat/rocket/reactnative/voip/VoipPayloadBundleTest.kt
- android/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.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 comments (4)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt (1)
106-123: LGTM — atomic consume-and-clear correctly addresses the prior race.The retry loop now safely handles concurrent writers: each iteration re-reads via
get(), and both the expired-discard and successful-consume paths usecompareAndSet(data, null)so only the exact observed payload is cleared. If a concurrentstoreInitialEvents/storeAcceptFailureForJsreplaces the reference betweenget()and CAS, the CAS fails and the loop re-reads the new payload instead of dropping it — directly resolving the earlier feedback.One minor note (non-blocking): on the consume path,
toWritableMap()is built before the CAS, so if CAS fails theWritableMapis discarded and rebuilt on the next iteration. In practice writers are rare (notification/REST/timeout handlers) and payloads are small, so the wasted work is negligible.android/app/build.gradle (1)
157-158: LGTM — Robolectric added astestImplementationonly.Scoped correctly so it won't affect release APK size, and pairs fine with the existing JUnit 4.13.2. This is the minimum needed to run the new
DDPClientTest/VoipPayloadBundleTestsuites under the JVM unit test task.android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (2)
33-49: Thread-safety hardening looks correct.
AtomicIntegerfor id generation,@VolatileonisConnected,connectedCallback, andconnectTimeoutRunnable, and theAtomicBooleangating for one-shot callback delivery together address the previously flagged races (OkHttp thread vs. main looper). The cross-thread visibility story for these fields is now consistent.
53-93: The per-call DDPClient architecture prevents the described race condition from occurring in practice.The codebase creates a fresh
DDPClientinstance for eachcallId(line 447 of VoipNotification.kt), callsconnect()exactly once per instance (line 499), and manages cleanup viaVoipPerCallDdpRegistry. When a new client replaces an old one for the samecallId, the registry invokesdisconnect()on the old client (via thereleaseClientlambda at line 79), which comprehensively tears down the prior state:
- Closes the WebSocket
- Nulls
connectedCallback- Cancels the timeout via
cancelConnectTimeout()- Clears all pending callbacks
No code path calls
connect()multiple times on the same instance, and no retry logic exists. The "cross-connect race" scenario outlined in the review (whereonFailurefrom an old socket fires afterconnect()is called again on the same instance) cannot occur given the current single-use-per-call architecture. The suggested hardening would be defensive but is not required by any observable usage pattern.
- DDPClient: AtomicInteger for DDP ids; @volatile isConnected/connectedCallback - Track connect-timeout Runnable; cancel only that runnable (not all Handler work) - Clear connect timeout on WebSocket failure to avoid duplicate callbacks - VoipPayload.toBundle: persist voipAcceptFailed for process-death restore - VoipModule: @volatile initialEventsData for cross-thread visibility - Add Robolectric unit tests for timeout/callback races and bundle round-trip Made-with: Cursor
- DDPClient: @volatile connectTimeoutRunnable; AtomicBoolean gates single connect outcome - VoipModule: AtomicReference + CAS for initialEventsData consume/clear - DDPClientTest: duplicate connected delivers callback once Made-with: Cursor
fb4de9e to
7c63d4c
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
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)
53-93:⚠️ Potential issue | 🟠 MajorClose any prior
webSocketbefore opening a new one.
connect()reassignswebSocketwithout closing/nulling the previous instance. If a caller invokesconnect()twice (or re-connects withoutdisconnect()), the oldWebSocketListenerstays registered and can still fireonOpen/onFailure/onMessage. Because those callbacks route through shared fields (connectedCallback,connectResultDelivered,isConnected) rather than closure state, a stale listener can overwrite/deliver into the new handshake's flow (e.g., an oldonFailurepostingtryDeliverConnectOutcome(false)after the newwaitForConnectedhas already installedconnectedCallback = newCb).🔒 Suggested hardening
fun connect(host: String, callback: (Boolean) -> Unit) { resetConnectHandshakeState() + // Ensure any prior socket/listener is torn down so stale events cannot + // deliver into the new handshake via shared state. + webSocket?.close(1000, null) + webSocket = null + connectedCallback = null + cancelConnectTimeout() val wsUrl = buildWebSocketURL(host)🤖 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 53 - 93, connect() currently reassigns the webSocket field without closing the prior socket, allowing stale listeners to interfere with a new handshake; before creating a new WebSocket in connect(), if webSocket is non-null call webSocket.close(1000, "reconnect") (or webSocket.cancel() if you prefer immediate termination) and null out the field to ensure the old listener won't race into the new connect flow; update the connect method to perform this close/null sequence right before building/assigning the new webSocket (referencing the webSocket field and the connect() function and the client.newWebSocket call).
🤖 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/DDPClient.kt`:
- Around line 150-161: The disconnect() implementation should stop resetting the
connect-handshake atomic so late events from the old socket cannot hijack a
newly installed callback: remove the line that clears or sets
connectResultDelivered (the atomic used by tryDeliverConnectOutcome) from
disconnect(), and rely on resetConnectHandshakeState() which connect() already
calls to rearm connectResultDelivered; keep cancelling timeouts, clearing
pendingCallbacks, clearing connectedCallback, nulling onCollectionMessage and
closing webSocket as-is so that onFailure/onMessage from the old socket will
fail the CAS in tryDeliverConnectOutcome and not invoke a new connectedCallback.
---
Outside diff comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 53-93: connect() currently reassigns the webSocket field without
closing the prior socket, allowing stale listeners to interfere with a new
handshake; before creating a new WebSocket in connect(), if webSocket is
non-null call webSocket.close(1000, "reconnect") (or webSocket.cancel() if you
prefer immediate termination) and null out the field to ensure the old listener
won't race into the new connect flow; update the connect method to perform this
close/null sequence right before building/assigning the new webSocket
(referencing the webSocket field and the connect() function and the
client.newWebSocket call).
🪄 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: fbcfd3fe-11f1-4cc9-93e4-aab7435c989b
📒 Files selected for processing (6)
android/app/build.gradleandroid/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/VoipPayloadBundleTest.kt
✅ Files skipped from review due to trivial changes (2)
- android/app/build.gradle
- android/app/src/test/java/chat/rocket/reactnative/voip/VoipPayloadBundleTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt
- android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt
- android/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.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
…nect() - Add stale-listener guard (webSocket !== this@DDPClient.webSocket) to onMessage, onFailure, and onClosed; a socket closed via disconnect() can still fire OkHttp callbacks after webSocket is nulled or replaced, and without the guard those callbacks could win the connectResultDelivered CAS and invoke a newly installed connectedCallback with a spurious false. - Remove connectResultDelivered.set(false) from disconnect(); connect() already rearms the flag via resetConnectHandshakeState(), so the reset in disconnect() only narrows the guard window unnecessarily. - Add testDeliverConnectFailure() internal hook and two new tests: "connection failure delivers false exactly once" (covers previously untested onFailure path) and "stale failure after completed connect and disconnect is a no-op" (regression lock for the CAS hijack). Addresses CodeRabbit review comment on PR #7168 (discussion_r3111670137).
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
352-363: Test-only hooks on the production class — acceptable, but consider tightening.Exposing
testStartConnectTimeout,testDeliverRawMessage, andtestDeliverConnectFailureasinternalis fine for unit testing, but they widen the production surface area (any code in the same module can call them). If you want to keep the handshake helpers testable without this, a common alternative is@VisibleForTesting(otherwise = PRIVATE)fromandroidx.annotationpaired with Lint enforcement, or route tests through a narrow package-private seam. Not a blocker.🤖 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 352 - 363, The three test hooks testStartConnectTimeout, testDeliverRawMessage, and testDeliverConnectFailure are currently declared internal which broadens the production API; change their visibility to private and annotate them with `@VisibleForTesting`(otherwise = PRIVATE) from androidx.annotation (or move them behind a narrow package-private test seam) so tests can access them while preventing other production-module code from calling them; update imports to include androidx.annotation.VisibleForTesting and keep the existing implementations of resetConnectHandshakeState, waitForConnected, handleMessage, and tryDeliverConnectOutcome unchanged.
🤖 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/DDPClient.kt`:
- Around line 81-88: The onFailure handler posts a runnable that calls
tryDeliverConnectOutcome(false) but only checks webSocket identity before
posting, allowing a stale failure to hijack a new connect; fix by re-checking
the failing WebSocket inside the posted runnable (or capture the failing
WebSocket and pass it into tryDeliverConnectOutcome so that
tryDeliverConnectOutcome verifies it against this@DDPClient.webSocket) before
performing the CAS/delivery, similar to the pre-post guard; update onFailure
(and any other WebSocket-thread callbacks that post to mainHandler) to
capture/compare the event's webSocket and no-op if it no longer matches the
current this@DDPClient.webSocket, ensuring
disconnect()/connect()/resetConnectHandshakeState() can't be hijacked.
In `@android/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.kt`:
- Around line 92-117: The test only verifies the CAS gate because
testDeliverConnectFailure() posts tryDeliverConnectOutcome(false) directly, so
add a scenario that simulates the real hijack window where disconnect() is
followed by a quick connect() before a stale WebSocket onFailure() arrives:
after client.disconnect(), call client.testStartConnectTimeout(...) to arm a new
callback (i.e., simulate a new connect()), then invoke
client.testDeliverConnectFailure() (so the stale onFailure is delivered while a
new handshake is armed) and assert the new callback does not receive false; this
also verifies resetConnectHandshakeState()/disconnect() doesn’t re-arm the
handshake state incorrectly and coordinates with DDPClient.onFailure handling.
---
Nitpick comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 352-363: The three test hooks testStartConnectTimeout,
testDeliverRawMessage, and testDeliverConnectFailure are currently declared
internal which broadens the production API; change their visibility to private
and annotate them with `@VisibleForTesting`(otherwise = PRIVATE) from
androidx.annotation (or move them behind a narrow package-private test seam) so
tests can access them while preventing other production-module code from calling
them; update imports to include androidx.annotation.VisibleForTesting and keep
the existing implementations of resetConnectHandshakeState, waitForConnected,
handleMessage, and tryDeliverConnectOutcome unchanged.
🪄 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: 052737ab-77bf-410b-a8cb-5775b3642f7d
📒 Files selected for processing (2)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.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/test/java/chat/rocket/reactnative/voip/DDPClientTest.kt (1)
19-90: Handshake behavior coverage looks solid.Good coverage for the refactor: at-most-once delivery on
connectedbefore timeout,cancelConnectTimeoutno longer nuking unrelated main-handler runnables (regression for the priorremoveCallbacksAndMessages(null)), timeout-without-connected singlefalse, duplicateconnectedidempotency, and duplicate-failure dedup. Robolectricidle()/idleFor()usage correctly exercisesmainHandler.post/postDelayedpaths.
…check
CodeRabbit flagged that the outer stale-listener guard in onFailure() runs
on the OkHttp thread before mainHandler.post. If disconnect() and a new
connect() interleave on the main thread between the outer guard and the
runnable's execution, the new connect() calls resetConnectHandshakeState()
(delivered=false) and installs a fresh connectedCallback via waitForConnected.
The stale runnable then wins the CAS and hijacks the new caller's callback
with false.
Fix:
- Re-check webSocket !== this@DDPClient.webSocket INSIDE the mainHandler.post
block in onFailure so stale failures remain no-ops even across a quick
disconnect/reconnect.
- Extend testDeliverConnectFailure to optionally take a fromWebSocket so the
test harness can simulate the real OkHttp-thread → main-thread post flow
including the identity guard.
- Add testSetActiveWebSocket() helper to simulate the current-active-socket
transition that disconnect()+connect() would produce in production.
- Add StubWebSocket and a new regression test
("stale failure during reconnect window does not hijack new callback")
that exercises the full hijack vector end-to-end.
- Rename existing "stale failure after completed connect and disconnect is
a no-op" test to clarify it covers only the CAS-gate case, not the
reconnect-window case.
Addresses CodeRabbit reviews discussion_r3112263091 (onFailure guard) and
related regression-test weakness comment on the new DDPClientTest.
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
There was a problem hiding this comment.
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)
63-74:⚠️ Potential issue | 🟠 MajorAdd the stale-socket guard to
onOpentoo.
onOpencan still run from an old listener afterdisconnect()/quick reconnect. Without the same identity check, it can callwaitForConnected()and overwrite the activeconnectedCallback/timeout with stale state.Suggested fix
override fun onOpen(webSocket: WebSocket, response: Response) { + if (webSocket !== this@DDPClient.webSocket) return Log.d(TAG, "WebSocket opened") val connectMsg = JSONObject().apply { put("msg", "connect")🤖 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 63 - 74, onOpen currently proceeds unconditionally and can act on a stale WebSocket; add the same stale-socket guard used elsewhere to verify the incoming WebSocket matches the active instance (e.g., compare the webSocket parameter to the class's current socket/ws field) and bail out if they differ, so onOpen does not call waitForConnected or overwrite the active connected callback/timeout for an old listener; update the onOpen handler in DDPClient.kt accordingly (check socket identity before building/sending connectMsg and before invoking waitForConnected).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 63-74: onOpen currently proceeds unconditionally and can act on a
stale WebSocket; add the same stale-socket guard used elsewhere to verify the
incoming WebSocket matches the active instance (e.g., compare the webSocket
parameter to the class's current socket/ws field) and bail out if they differ,
so onOpen does not call waitForConnected or overwrite the active connected
callback/timeout for an old listener; update the onOpen handler in DDPClient.kt
accordingly (check socket identity before building/sending connectMsg and before
invoking waitForConnected).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a977ad0c-95e4-4285-96cf-01fd148adc1e
📒 Files selected for processing (2)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.kt
📜 Review details
🧰 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 (12)
android/app/src/test/java/chat/rocket/reactnative/voip/DDPClientTest.kt (5)
18-25: Stub is minimal and fit for these listener-identity tests.The fake
WebSocketkeeps the reconnect-window tests focused on object identity without pulling in OkHttp internals.
31-72: Good coverage for timeout ordering and targeted timeout cancellation.These tests lock down both “connected wins before timeout” and “cancel only the connect timeout, not unrelated main-handler work.”
74-102: Good single-delivery coverage for duplicate success/failure paths.This directly validates the
AtomicBooleanhandshake gate from both connected-message and failure-delivery directions.
104-127: Good regression lock for late failure after disconnect.This confirms
disconnect()no longer re-armsconnectResultDelivered, so a late failure cannot emit a spuriousfalsewhen no reconnect is active.
129-169: Reconnect-window hijack regression is covered.This addresses the prior stale-failure gap by queuing a failure for the old socket, installing a new socket/callback, and asserting the stale runnable cannot claim the new callback.
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (7)
33-49: Thread-safety hardening looks good.
AtomicInteger, volatile connection state, volatile callback storage, and the handshakeAtomicBooleanare appropriate for the OkHttp/main-thread split here.
76-99: Stale listener checks and the inner failure re-check look solid.The second identity check inside the posted
onFailurerunnable closes the reconnect-window race where an old failure could otherwise claim a freshly installed callback.
158-168:disconnect()now preserves the handshake CAS state correctly.Cancelling the timeout and clearing callbacks without resetting
connectResultDeliveredprevents late old-socket events from becoming deliverable before the next explicit connect re-arms state.
170-175: Atomic DDP ids are a good fix.Using
incrementAndGet()removes the cross-thread race around message id generation.
239-276: Centralized connect outcome delivery is much safer.The CAS gate plus targeted timeout removal keeps success, failure, and timeout paths single-delivery and avoids wiping unrelated main-handler callbacks.
286-290: Connected-message handling now uses the same delivery gate.Routing
"connected"throughtryDeliverConnectOutcome(true)keeps it consistent with failure and timeout paths.
357-375: Test hooks are appropriately narrow for the new race tests.The hooks expose only handshake timing, raw message delivery, failure simulation, and active-socket identity needed by the Robolectric coverage.
…/Decline (#7215) * merge feat.voip-lib * feat(voip): enhance call handling with UUID mapping and event listeners * Base call UI * feat(voip): integrate Zustand for call state management and enhance CallView UI * feat(voip): add simulateCall function for mock call handling in UI development * refactor(CallView): update button handlers and improve UI responsiveness * Add pause-shape-unfilled icon * Base CallHeader * toggleFocus * collapse buttons * Header components * Hide header when no call * Timer * Add use memo * Add voice call item on sidebar * cleanup * Temp use @rocket.chat/media-signaling from .tgz * cleanup * Check module and permissions to enable voip * Refactor stop method to use optional chaining for media signal listeners * voip push first test * Add VoIP call handling with pending call management - Implemented VoIP push notification handling in index.js, including storing call info for later processing. - Added CallKeep event handlers for answering and ending calls from a cold start. - Introduced a new CallIdUUID module to convert call IDs to deterministic UUIDs for compatibility with CallKit. - Created a pending call store to manage incoming calls when the app is not fully initialized. - Updated deep linking actions to include VoIP call handling. - Enhanced MediaSessionInstance to process pending calls and manage call states effectively. * Remove pending store and create getInitialEvents on app/index * Attempt to make iOS calls work from cold state * lint and format * Patch callkeep ios * Temp send iOS voip push token on gcm * Temp fix require cycle * chore: format code and fix lint issues [skip ci] * CallIDUUID module on android and voip push * Add setCallUUID on useCallStore to persist calls accepted on native Android * remove callkeep from notification * Android Incoming Call UI POC * Refactor VoIP handling: Migrate VoIP-related classes to a new package structure, removing deprecated modules and consolidating functionality. Update imports in MainApplication and NotificationIntentHandler to reflect changes. This cleanup enhances code organization and prepares for future VoIP feature enhancements. * Remove VoipForegroundService * cleanup and use caller instead of callerName * Cleanup and make iOS build again * Refactor VoIP handling: Remove unused event emissions for call answered and declined, switch from SharedPreferences to in-memory storage for pending VoIP call data, and update method signatures for better clarity. This cleanup enhances performance and prepares for future VoIP feature improvements. * Refactor VoIP handling: Introduce a new VoipPayload class to encapsulate call data, streamline notification processing, and enhance method signatures across the VoIP module. This update improves code clarity and prepares for future feature enhancements. * Migrate react-native-voip-push-notifications to VoipModule * Refactor VoIP module: Update package structure by moving VoipTurboPackage to the main package and removing the obsolete NativeVoipSpec class. Adjust imports in MainApplication and VoipModule to reflect these changes, enhancing code organization and maintainability. * Unify emitters * Move CallKeep listeners from MediaSessionInstance to getInitialEvents * Clear callkeep on endcall * Unify getInitialEvents logic * getInitialEvents -> MediaCallEvents * chore: format code and fix lint issues [skip ci] * feat(Android): Add full screen incoming call (#6977) * feat: Update call UI (#6990) * feat: Handle audio routing, e.g., Bluetooth headset vs. internal speaker switching (#6992) * fix: empty space when not on call (#6993) * feat: Dialpad (#7000) * action: organized translations * feat: start call (#7024) * chore: format code and fix lint issues * feat: Pre flight (#7038) * action: organized translations * feat: Receive voip push notifications from backend (#7045) * feat: Refactor media session handling and improve disconnect logic (#7065) * feat: Control incoming call from native (#7066) * feat: Voice message blocks (#7057) * feat: native accept success event (#7068) * feat(voip): call waiting, busy detection, and videoconf blocking (#7077) * action: organized translations * feat(voip): tap-to-hide call controls with animations (#7078) * feat(voip): navigate to call DM from message button and header (#7082) * feat(voip): tablet and landscape layout (#7110) * chore: develop into feat.voip-lib-new (RN 81 + Expo 54 + reanimated 4 + true-sheet + iOS 26) (#7114) * chore: format code and fix lint issues * feat(voip): android landscape layout for IncomingCallActivity (#7116) * Update agents files * feat(voip): Support a11y (#7106) * Fix content cutting on iOS on some edge cases * pods * Ignore .worktrees on jest * chore: Merge develop into feat.voip-lib-new (#7129) * fix(voip): show CallKit UI when call is active in background (#7128) * chore: Update media-signaling to 0.2.0 (#7153) * feat(voip): migrate iOS accept/reject from DDP to REST (#7124) * Fix icons * feat(voip): migrate Android accept/reject from DDP to REST (#7127) * test(voip): integration tests for CallView pipeline (#7161) * feat(voip): display video conf provider as subtitle (#7160) * fix(voip): CallView button grid and correct landscape/dialpad layouts (#7164) * fix(voip): prevent stale MMKV cache on Android first-install accept MMKVKeyManager.initialize ran in MainApplication.onCreate before the JS engine started and opened the default MMKV file via the Tencent 1.2 JAR when it was still empty. Tencent caches instances per-ID in a singleton registry, so that empty-state view was held for the rest of the process. JS later wrote credentials through react-native-mmkv (MMKV Core 2.0), which has its own separate registry. When a VoIP push arrived, Ejson.getMMKV() got the cached empty Tencent instance and reported "No userId found in MMKV for server". Closing and reopening the app cleared the cache, which is why only the very first call after install failed. Drop the open/verify block — the encryption key is already cached from SecureKeystore, so no MMKV handle is needed here. The first Tencent instance is now created inside Ejson.getMMKV() after JS has written, so it scans the file fresh. * fix(voip): prevent duplicate ringtone on Android incoming call (#7158) * fix(voip): set explicit snaps for NewMediaCall bottom sheet (#7165) * Update app/lib/services/voip/MediaSessionStore.ts Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com> * fix: make startVoipFork reactive to permissions-changed (#7151) * fix(android): remove MediaProjectionService from merged manifest (#7190) * fix(voip): Phone account creation (#7170) * feat: add Enable Mobile Ringing toggle in user preferences (#7155) * fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens (#7167) * fix(android): Play Store mic discoverability, safer FCM logs, avatar auth via headers (#7171) * fix(ios): serialize VoipService bridge statics (#7169) * fix(voip): Android DDP thread safety and VoipPayload bundle parity (#7168) * chore(voip): dead-code and hygiene sweep (#7174) * refactor(voip): decouple navigateToCallRoom from Redux and backfill REST/connect tests (#7176) * test(voip): tighten ringing endCall assertion and add VideoConf VoIP-lock saga coverage (#7177) * fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS (#7173) * refactor(voip): MediaCallEvents Redux adapters and resetVoipState (#7178) * refactor(voip): decouple peer autocomplete from Redux; simplify NewMediaCall (#7175) * fix(ios): add NS_SWIFT_NAME to Challenge.runChallenge for Swift 6.2 compatibility Swift 6.2 (Xcode 26.x / macos-26 runner) auto-renames the Objective-C method runChallenge:didReceiveChallenge:completionHandler: to run(_:didReceive:completionHandler:) when imported into Swift. Add NS_SWIFT_NAME to explicitly pin the Swift import name, preventing the compiler from applying its heuristics. This keeps the existing Swift call site in DDPClient.swift working without changes. * fix(ios): cancel old URLSession/webSocketTask before reconnecting in DDPClient.connect (#7197) * fix(ios): add NSLock to nativeAcceptHandledCallIds and 10s REST timeout to handleNativeAccept (#7198) * feat(android): create VoipCallService with FOREGROUND_SERVICE_MICROPHONE (#7199) * fix(android): start VoipCallService on accept, stop on hangup/timeout, install end-call listener (#7200) * fix(voip): enable DM nav for users with SIP extension (#7203) * fix(android): handle null VoiceConnection in answerIncomingCall, notify JS (#7201) * fix(voip): resolve closure capture ordering in handleNativeAccept (#7209) * fix(android): integrate VoIP modules with SSL-pinned OkHttpClient (#7208) * fix(push): gate id and voipToken behind server version checks, fix VideoConf caller extra (#7210) * fix(voip): remove sensitive data from production logs (#7207) * fix(android): remove isRunning guard + add double-tap guard on Accept/Decline - VoipCallService: remove if (!isRunning) guard, call startForeground unconditionally (idempotent on Android, fixes Android 14+ foreground service requirement) - IncomingCallActivity: add AtomicBoolean guard on handleAccept/handleDecline to prevent double-tap from triggering multiple service starts --------- Co-authored-by: diegolmello <diegolmello@users.noreply.github.com> Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
1 participant
Proposed changes
Android VoIP DDP and cold-start payload hardening (split-plan PR 2):
DDPClient: useAtomicIntegerfor DDP message ids; markisConnectedandconnectedCallback@Volatilefor visibility across OkHttp and main-thread handlers.Runnablefor the connect timeout and remove only that callback instead ofremoveCallbacksAndMessages(null), so unrelatedHandlerwork is not cleared. Cancel that timeout onconnected,disconnect(), and WebSocketonFailure.onMessage,onFailure, andonClosednow bail early when theWebSocketparameter does not match the current active socket (webSocket !== this@DDPClient.webSocket). Prevents a closed socket's late OkHttp callbacks from hijacking a newly installedconnectedCallbackvia a CAS win after reconnect.disconnect()no longer resetsconnectResultDelivered:connect()already rearms the flag viaresetConnectHandshakeState(); the extra reset indisconnect()narrowed the stale-listener guard window unnecessarily.VoipPayload.toBundle(): includevoipAcceptFailedso process-death / bundle restore matchesfromBundle()andtoWritableMap().VoipModule:@Volatileon companioninitialEventsDatafor cross-thread visibility.connectedordering,voipAcceptFailedbundle round-trip, connection failure delivery (previously untestedonFailurepath), and stale-failure no-op regression lock.Issue(s)
How to test or reproduce
From repo root after
yarn:Or run all VoIP JVM unit tests:
Screenshots
N/A (native concurrency and serialization only).
Types of changes
Checklist
Further comments
Draft PR: branch
pr2-android-ddp-thread-safety. Base branch isfeat.voip-lib-new.Summary by CodeRabbit
Bug Fixes
Tests
Chores