fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS

[diegolmello]

Proposed changes

This pull request hardens the native iOS DDP WebSocket client used for VoIP media-signal monitoring.

• Receive failures: When URLSessionWebSocketTask.receive completes with an error, the client now performs a full teardown (invalidate session, clear task, clear callbacks) instead of leaving a "connected" shell that no longer reads messages.
• TLS / client certificates: The DDP URLSession is created with a small delegate that forwards authentication challenges to the existing Challenge implementation in SSLPinning.mm—the same path already used by React Native networking swizzles—so WebSocket handshakes honor the app's client-certificate flow without introducing a second pinning stack.
• Disconnect ordering: disconnect() runs cleanup synchronously when it is already invoked from the client's internal serial queue, so failure handling cannot enqueue snapshot work ahead of an asynchronous disconnect that has not finished yet.
• Message routing: The default branch in DDP JSON handling still forwards collection-style payloads only when collection is a string, matching the shape used elsewhere; a redundant unused binding was removed.
• Delegate wiring cleanup: Dropped the retained urlSessionChallengeDelegate ivar (URLSession already retains its delegate) and collapsed the duplicated URLSessionTaskDelegate challenge method — the session-level URLSessionDelegate method handles all challenges once implemented.

Challenge is declared in SSLPinning.h and imported through RocketChatRN-Bridging-Header.h so Swift can call the existing Objective-C entry point.

Unit tests for receive-failure teardown will land in a follow-up PR once the Xcode unit test target wiring is in place.

Issue(s)

How to test or reproduce

• Manual: Optional proxy / MITM check on the DDP WebSocket host to confirm TLS behavior matches the rest of the app's certificate story (not automated in CI).

Screenshots

N/A (native networking only).

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• Improvement (non-breaking change which improves a current function)
• New feature (non-breaking change which adds functionality)
• Documentation update (if none of the other choices apply)

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• I have added necessary documentation (if applicable)
• Any dependent changes have been merged and published in downstream modules

Further comments

Deferred items (test target wiring, send-path parity, manual TLS checklist) are recorded under iOS DDPClient — WebSocket client hardening in the VoIP review future.md follow-up list for separate tickets.

Summary by CodeRabbit

• Bug Fixes

  • More reliable WebSocket handling: failures now trigger immediate disconnect and cleanup to avoid stale connection state.
  • Safer, race-free disconnect behavior to prevent lingering callbacks, queued tasks, or stale receive loops.
  • Preserve incoming message payloads for collection updates to avoid emitting mutated messages.

• New Features

  • Improved TLS authentication routing via a new challenge handler for WebSocket sessions.

[coderabbitai]

Note

Reviews paused

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

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds URLSession-based TLS challenge delegation (forwarding to a new Challenge API), refactors DDPClient disconnect/cleanup for queue-safety, improves WebSocket receive/error handling, and exposes Challenge to Swift via the bridging header.

Changes

Cohort / File(s)	Summary
WebSocket & connection logic ios/Libraries/DDPClient.swift	Added deinit cleanup; connect(...) now builds URLSession with DDPClientURLSessionChallengeDelegate; refactored disconnect() to use disconnectOnStateQueue() and consolidated cleanup/failure of pending callbacks; extracted receive handling to handleWebSocketReceiveResult(..., for:) and now disconnects on receive failures; adjusted collection message dispatch to avoid mutating payloads.
TLS challenge delegation API ios/Libraries/SSLPinning.h	Added Objective‑C Challenge interface declaring +runChallenge:didReceiveChallenge:completionHandler: returning an NSURLSessionAuthChallengeDisposition and optional NSURLCredential.
Swift bridging ios/RocketChatRN-Bridging-Header.h	Imported SSLPinning.h so the new Challenge API is available to Swift code.

Sequence Diagram(s)

sequenceDiagram
    participant Client as DDPClient
    participant URLSession as URLSession
    participant Delegate as DDPClientURLSessionChallengeDelegate
    participant Challenge as Challenge
    participant WebSocket as WebSocketTask

    Client->>URLSession: connect(with: Delegate)
    URLSession->>Delegate: encounter TLS auth challenge
    Delegate->>Challenge: runChallenge(session, challenge, completionHandler)
    Challenge-->>Delegate: disposition, credential
    Delegate-->>URLSession: call completionHandler(disposition, credential)
    URLSession->>WebSocket: establish websocket
    WebSocket->>Client: receive(message/result)
    Client->>Client: handleWebSocketReceiveResult(result, for: task)
    alt success
        Client->>Client: handleMessage(string)
        Client->>WebSocket: re-arm receive
    else failure
        Client->>Client: disconnectOnStateQueue(failPending: true)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS' accurately describes the main objectives of the changeset. It directly addresses the two primary improvements: receive failure handling and TLS authentication, which are the core focuses of the substantial changes to DDPClient.swift and the new Challenge integration.

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

ios/Libraries/DDPClient.swift (1)

192-214: ⚠️ Potential issue | 🟡 Minor

Send-path parity with receive-failure teardown.

The PR intentionally defers this (noted in "future work"), but flagging for visibility: send(...) at Line 204 logs send errors in DEBUG and simply invokes completion(false) — it does not tear down the session the way handleWebSocketReceiveResult now does on failure. Net effect: a send-side failure on a broken socket leaves the client in the same "non-reading connected shell" state this PR set out to eliminate on the receive side, until the receive loop also trips. Consider tracking a follow-up to route send errors through disconnect() (or a shared teardown helper) for symmetry.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/DDPClient.swift` around lines 192 - 214, The send(_
dict:completion:) path currently logs send errors and returns completion(false)
but does not perform the same teardown as handleWebSocketReceiveResult, leaving
the client in a broken-but-not-teardown state; update send(_:completion:) to
route send failures into the common teardown (call disconnect() or the shared
teardown helper used by handleWebSocketReceiveResult), ensuring the
webSocketTask/session is cleaned up and any state flags are reset before
invoking completion(false) so send-side failures mirror receive-side failures.

🧹 Nitpick comments (4)

ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift (1)

8-27: Test correctness looks good; one hardening note.

The ordering is safe — testing_applyReceiveResult and testing_readConnectionState both enqueue async onto the serial stateQueue, so the assertion block is guaranteed to run after the receive-failure handling. testing_installWebSocketSync runs synchronously so the task reference is in place before the failure is applied.

Minor: the injected URLSession is never invalidated by the test itself — it relies on disconnect() to call invalidateAndCancel. If a future change skipped that on a certain path, the ephemeral session would leak past the test. Consider adding a defer { session.invalidateAndCancel() } as belt-and-suspenders cleanup.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift` around lines 8 -
27, In testReceiveFailureClearsConnectionAndSession add explicit cleanup to the
ephemeral URLSession used by the test by calling session.invalidateAndCancel()
in a defer right after the session is created; this ensures the injected session
is always invalidated even if future changes to DDPClient.disconnect() skip
invalidateAndCancel, and keeps the existing use of testing_installWebSocketSync,
testing_applyReceiveResult and testing_readConnectionState unchanged.

ios/Libraries/DDPClient.swift (2)

432-457: DEBUG test hooks: one small gap.

testing_installWebSocketSync replaces urlSession and webSocketTask but does not touch urlSessionChallengeDelegate. If a prior connect(...) installed a challenge delegate, it will linger across the test install (the new injected session doesn't have one anyway, so behaviour is unaffected). More importantly, after the simulated failure triggers disconnect(), the test's urlSessionIsNil assertion passes, but there's no assertion that urlSessionChallengeDelegate was also cleared — even though disconnectOnStateQueue now clears it on Line 174. Consider extending testing_readConnectionState (or adding a second helper) to also report whether the challenge delegate was released, so the test locks in the full teardown contract the PR is establishing.

Also: testing_installWebSocketSync uses stateQueue.sync, which would deadlock if ever invoked from inside stateQueue. Fine for the current test caller (main thread), but worth an assertion or doc comment so future usage doesn't foot-gun.

♻️ Suggested extension

-    func testing_readConnectionState(_ completion: `@escaping` (_ isConnected: Bool, _ webSocketTaskIsNil: Bool, _ urlSessionIsNil: Bool) -> Void) {
+    func testing_readConnectionState(_ completion: `@escaping` (_ isConnected: Bool, _ webSocketTaskIsNil: Bool, _ urlSessionIsNil: Bool, _ challengeDelegateIsNil: Bool) -> Void) {
         stateQueue.async {
-            completion(self.isConnected, self.webSocketTask == nil, self.urlSession == nil)
+            completion(self.isConnected, self.webSocketTask == nil, self.urlSession == nil, self.urlSessionChallengeDelegate == nil)
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/DDPClient.swift` around lines 432 - 457,
testing_readConnectionState currently doesn't report whether
urlSessionChallengeDelegate was cleared; update it to include a Bool indicating
whether urlSessionChallengeDelegate == nil so tests can assert full teardown
(reference testing_readConnectionState and urlSessionChallengeDelegate). Also
update testing_installWebSocketSync to avoid potential deadlock when called from
stateQueue by replacing stateQueue.sync with stateQueue.async or by asserting
caller is not on stateQueue (reference testing_installWebSocketSync and
stateQueue); keep behavior of replacing urlSession and webSocketTask and setting
isConnected.

---

306-313: Nit: nested [weak self] capture is redundant.

The inner stateQueue.async closure already runs only if the outer self?. dereference succeeded, so the inner [weak self] + guard let self is redundant. Capturing self strongly inside the async block is safe (the block is short-lived and stateQueue doesn't outlive the client for the lifetime of the receive callback).

♻️ Optional simplification

     private func listenForMessages(task: URLSessionWebSocketTask) {
         task.receive { [weak self] result in
-            self?.stateQueue.async { [weak self] in
-                guard let self else { return }
-                self.handleWebSocketReceiveResult(result, for: task)
+            self?.stateQueue.async {
+                self?.handleWebSocketReceiveResult(result, for: task)
             }
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/DDPClient.swift` around lines 306 - 313, The nested `[weak
self]` capture inside listenForMessages is redundant; change the outer receive
closure to early-return if self is nil (guard let self = self else { return })
and then call stateQueue.async with a normal closure that uses self strongly to
invoke handleWebSocketReceiveResult(result, for: task). Remove the inner `[weak
self]` capture and the inner `guard let self` block so the async closure simply
calls self.handleWebSocketReceiveResult(...).

ios/Libraries/SSLPinning.h (1)

13-14: Minor: consider importing <Foundation/NSURLSession.h> directly.

Forward-declaring NSURLSession/NSURLAuthenticationChallenge while using NSURLSessionAuthChallengeDisposition (an enum that cannot be forward-declared) and NSURLCredential (not forward-declared) relies on a transitive import via <React/RCTHTTPRequestHandler.h>. An explicit #import <Foundation/NSURLSession.h> (or @import Foundation;) would make the header self-contained and robust to upstream RN header refactors.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/SSLPinning.h` around lines 13 - 14, The header currently
forward-declares NSURLSession and NSURLAuthenticationChallenge but uses
NSURLSessionAuthChallengeDisposition and NSURLCredential so make the header
self-contained by adding an explicit import of Foundation (e.g. `#import`
<Foundation/NSURLSession.h> or `@import` Foundation;) at the top of SSLPinning.h
so references to NSURLSession, NSURLAuthenticationChallenge,
NSURLSessionAuthChallengeDisposition, and NSURLCredential are resolved without
relying on transitive imports.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ios/Libraries/SSLPinning.h`:
- Around line 13-21: The header Challenge declaration is using
NSURLSessionAuthChallengeDisposition and NSURLCredential without including their
Foundation definitions; update SSLPinning.h to be self-contained by adding an
explicit import of the Foundation session types (e.g. `#import`
<Foundation/NSURLSession.h>) and/or forward-declaring NSURLCredential (e.g.
`@class` NSURLCredential) so the
+runChallenge:didReceiveChallenge:completionHandler: signature is fully resolved
without relying on transitive imports from RCTHTTPRequestHandler.

In `@ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift`:
- Around line 4-6: The test DDPClientReceiveFailureTests in
DDPClientReceiveFailureTests.swift is not included in an Xcode unit-test target,
so CI won't run it; update the test target wiring in the project to include this
file (add the DDPClientReceiveFailureTests test class to the appropriate unit
test target) so CI verifies the receive-failure teardown behavior, or
alternatively block merging until that wiring is added and ensure the DEBUG
helper APIs referenced by DDPClientReceiveFailureTests remain in sync with the
test; locate the test class name DDPClientReceiveFailureTests and any DEBUG
helper symbols used in the test to adjust the test target membership or gating
accordingly.

---

Outside diff comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 192-214: The send(_ dict:completion:) path currently logs send
errors and returns completion(false) but does not perform the same teardown as
handleWebSocketReceiveResult, leaving the client in a broken-but-not-teardown
state; update send(_:completion:) to route send failures into the common
teardown (call disconnect() or the shared teardown helper used by
handleWebSocketReceiveResult), ensuring the webSocketTask/session is cleaned up
and any state flags are reset before invoking completion(false) so send-side
failures mirror receive-side failures.

---

Nitpick comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 432-457: testing_readConnectionState currently doesn't report
whether urlSessionChallengeDelegate was cleared; update it to include a Bool
indicating whether urlSessionChallengeDelegate == nil so tests can assert full
teardown (reference testing_readConnectionState and
urlSessionChallengeDelegate). Also update testing_installWebSocketSync to avoid
potential deadlock when called from stateQueue by replacing stateQueue.sync with
stateQueue.async or by asserting caller is not on stateQueue (reference
testing_installWebSocketSync and stateQueue); keep behavior of replacing
urlSession and webSocketTask and setting isConnected.
- Around line 306-313: The nested `[weak self]` capture inside listenForMessages
is redundant; change the outer receive closure to early-return if self is nil
(guard let self = self else { return }) and then call stateQueue.async with a
normal closure that uses self strongly to invoke
handleWebSocketReceiveResult(result, for: task). Remove the inner `[weak self]`
capture and the inner `guard let self` block so the async closure simply calls
self.handleWebSocketReceiveResult(...).

In `@ios/Libraries/SSLPinning.h`:
- Around line 13-14: The header currently forward-declares NSURLSession and
NSURLAuthenticationChallenge but uses NSURLSessionAuthChallengeDisposition and
NSURLCredential so make the header self-contained by adding an explicit import
of Foundation (e.g. `#import` <Foundation/NSURLSession.h> or `@import` Foundation;)
at the top of SSLPinning.h so references to NSURLSession,
NSURLAuthenticationChallenge, NSURLSessionAuthChallengeDisposition, and
NSURLCredential are resolved without relying on transitive imports.

In `@ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift`:
- Around line 8-27: In testReceiveFailureClearsConnectionAndSession add explicit
cleanup to the ephemeral URLSession used by the test by calling
session.invalidateAndCancel() in a defer right after the session is created;
this ensures the injected session is always invalidated even if future changes
to DDPClient.disconnect() skip invalidateAndCancel, and keeps the existing use
of testing_installWebSocketSync, testing_applyReceiveResult and
testing_readConnectionState 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: d6ad14ce-0006-4c1a-9dc4-9394e1c0a7cd

📥 Commits

Reviewing files that changed from the base of the PR and between 9c84d4c and ea0a890.

📒 Files selected for processing (4)

• ios/Libraries/DDPClient.swift
• ios/Libraries/SSLPinning.h
• ios/RocketChatRN-Bridging-Header.h
• ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: ESLint and Test / run-eslint-and-test

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate

📚 Learning: 2026-03-05T06:06:19.755Z

Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.

Applied to files:

• ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift

🔇 Additional comments (4)

ios/Libraries/DDPClient.swift (4)

151-175: Disconnect ordering fix is correct.

The getSpecific(key: stateQueueKey) check reliably routes on-queue callers through the synchronous path (so teardown cannot be preempted by other queued work like listenForMessages rescheduling), while off-queue callers serialize safely via async. disconnectOnStateQueue() cleans up state, callbacks, task, session, and the new challenge delegate in the right order.

One observation: webSocketTask?.cancel(with: .normalClosure, reason: nil) is still invoked even when we reached here from a receive failure (connection already dead). That's harmless — URLSessionWebSocketTask.cancel is safe on a failed task — but worth being aware of when reading logs.

---

315-334: Receive-failure teardown is correct.

Identity check via task === webSocketTask cleanly drops stale callbacks from a previously canceled task. Because this runs on stateQueue, the disconnect() call on failure dispatches through the synchronous on-queue path in Line 152, so teardown completes before any later-queued work executes — exactly the invariant described in the PR objectives.

One consideration: on .success with .data(...) (the default: branch at Line 323), binary frames are silently dropped. DDP is JSON-text so this is fine in practice, but a single DEBUG log line would aid future diagnosis if the server ever sends an unexpected binary frame.

---

367-383: Default-branch routing change looks good; pre-existing no-op above.

(json["collection"] as? String) != nil correctly gates forwarding on collection being a string, matching the PR description.

Unrelated pre-existing smell in the "changed" / "added" / "removed" branch at Line 368-372: message["collection"] = collection reassigns the same string back onto the dictionary, which is a no-op. Not introduced by this PR, but worth cleaning up next time you're in this file.

---

411-430: Delegate wiring is correct; manual TLS verification is optional.

The implementation correctly covers both challenge paths: session-level delegates (typically server-trust) and task-level delegates (typically client-cert) both forward to Challenge.runChallenge(...), reusing the existing pinning/client-cert flow as intended. The code comment on line 18 explicitly acknowledges this design. URLSession manages delegate callback serialization per session, so no reentrancy issues arise from synchronous challenge handling.

If integration testing reveals the need for visibility into TLS challenge flow (e.g., on pinned hosts), temporarily adding logging to the delegate methods is a reasonable debugging step—but it's not required for correctness.

[coderabbitai]

🧹 Nitpick comments (2)

ios/Libraries/DDPClient.swift (2)

362-367: Redundant re-assignment of collection into the copied dictionary.

message is a value-type copy of json, so message["collection"] already holds the same string. The var message = json + message["collection"] = collection dance does nothing beyond forwarding json itself. Matches the simplification already done in the default branch below.

♻️ Proposed change

         case "changed", "added", "removed":
-            if let collection = json["collection"] as? String {
-                var message = json
-                message["collection"] = collection
-                onCollectionMessage?(message)
+            if (json["collection"] as? String) != nil {
+                onCollectionMessage?(json)
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/DDPClient.swift` around lines 362 - 367, Redundant
copy-and-reassign: in the switch handling case "changed","added","removed"
inside DDPClient.swift, remove the unnecessary var message = json and
message["collection"] = collection lines and instead call
onCollectionMessage?(json) directly (same behavior as the default branch);
update the branch that references onCollectionMessage to pass json unchanged to
avoid the no-op reassignment.

---

30-71: Add a deinit that invalidates the URLSession to prevent leaks if DDPClient is released without calling disconnect().

URLSession created with a delegate retains that delegate strongly until invalidateAndCancel() or finishTasksAndInvalidate() is called. Per Apple's official documentation, not invalidating leaks the session (and its delegate) for the app's lifetime. While disconnect() handles this, any code path that drops a DDPClient reference without calling disconnect() will silently leak the URLSession and its live WebSocket task. A small deinit closes this gap defensively:

♻️ Proposed change

     init() {
         stateQueue.setSpecific(key: stateQueueKey, value: ())
     }
+
+    deinit {
+        // Ensure the URLSession (which strongly retains its delegate) is always invalidated,
+        // even if disconnect() was never explicitly invoked by the owner.
+        webSocketTask?.cancel(with: .normalClosure, reason: nil)
+        urlSession?.invalidateAndCancel()
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/DDPClient.swift` around lines 30 - 71, Add a deinit to
DDPClient that defensively cleans up the live WebSocket and URLSession if the
instance is released without calling disconnect(); specifically, implement
deinit { webSocketTask?.cancel(with: .goingAway, reason: nil);
urlSession?.invalidateAndCancel(); webSocketTask = nil; urlSession = nil } so
the URLSession delegate created in connect and any running webSocketTask are
released and do not leak if disconnect() is not invoked.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 362-367: Redundant copy-and-reassign: in the switch handling case
"changed","added","removed" inside DDPClient.swift, remove the unnecessary var
message = json and message["collection"] = collection lines and instead call
onCollectionMessage?(json) directly (same behavior as the default branch);
update the branch that references onCollectionMessage to pass json unchanged to
avoid the no-op reassignment.
- Around line 30-71: Add a deinit to DDPClient that defensively cleans up the
live WebSocket and URLSession if the instance is released without calling
disconnect(); specifically, implement deinit { webSocketTask?.cancel(with:
.goingAway, reason: nil); urlSession?.invalidateAndCancel(); webSocketTask =
nil; urlSession = nil } so the URLSession delegate created in connect and any
running webSocketTask are released and do not leak if disconnect() is not
invoked.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 770aff75-3438-47cd-a3af-ff6b820a51f4

📥 Commits

Reviewing files that changed from the base of the PR and between ea0a890 and ef1bdb5.

📒 Files selected for processing (4)

• ios/Libraries/DDPClient.swift
• ios/Libraries/SSLPinning.h
• ios/RocketChatRN-Bridging-Header.h
• ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift

✅ Files skipped from review due to trivial changes (1)

• ios/RocketChatRN-Bridging-Header.h

🚧 Files skipped from review as they are similar to previous changes (1)

• ios/RocketChatRNTests/DDPClientReceiveFailureTests.swift

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: ESLint and Test / run-eslint-and-test

🧰 Additional context used

🧠 Learnings (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)

ios/Libraries/SSLPinning.h (1)

13-21: Header still relies on transitive imports for NSURLSessionAuthChallengeDisposition/NSURLCredential.

The Challenge signature uses NSURLSessionAuthChallengeDisposition and NSURLCredential directly, but the header only imports <React/RCTHTTPRequestHandler.h>. It resolves today through transitive inclusion, which is fragile to upstream React Native header reshuffles. Making the header self-contained (e.g., #import <Foundation/NSURLSession.h> and/or @class NSURLCredential;) keeps it robust.

♻️ Proposed change

 `#import` <React/RCTHTTPRequestHandler.h>
+#import <Foundation/NSURLSession.h>

 NS_ASSUME_NONNULL_BEGIN

 `@class` NSURLSession;
 `@class` NSURLAuthenticationChallenge;
+@class NSURLCredential;

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 157-165: disconnectOnStateQueue currently clears pendingCallbacks
and queued method calls without invoking them, causing in-flight
connect/login/subscribe/callMethod to hang; update disconnectOnStateQueue to
iterate pendingCallbacks and invoke each callback delivering a failure (e.g.,
false or an error) before calling pendingCallbacks.removeAll() and
clearQueuedMethodCalls(), then nil out connectedCallback and
onCollectionMessage. Apply the same change in the receive-failure teardown path
(the code around lines 323-327) so that any pending callbacks are drained with a
failure result before clearing state.
- Around line 314-321: After calling handleMessage within the case .success
branch, ensure the WebSocket task hasn't been replaced or canceled by callbacks
(e.g., disconnect()) by comparing the local task parameter to the current
webSocketTask; only call listenForMessages(task:) if task === webSocketTask.
Update the case .success(let message) -> .string(let text) path in the receive
loop to perform this identity check after handleMessage(text) and before
re-arming the receive with listenForMessages(task: task).

🪄 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: 8b8fea08-911c-4ae5-9e5f-8fabbc9981f0

📥 Commits

Reviewing files that changed from the base of the PR and between ef1bdb5 and 5dcbc0c.

📒 Files selected for processing (1)

• ios/Libraries/DDPClient.swift

📜 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 (2)

ios/Libraries/DDPClient.swift (2)

46-50: TLS challenge delegate wiring looks good.

The WebSocket session now routes authentication challenges through the shared Challenge path.

Also applies to: 406-415

---

374-377: Collection routing guard looks good.

The default branch now only forwards collection-style payloads when collection is actually a string.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

ios/Libraries/DDPClient.swift (1)

28-30: ⚠️ Potential issue | 🟠 Major

Keep deinit on the state queue before tearing down.

disconnectOnStateQueue() still mutates state-queue-confined fields, but deinit can run from any thread. Please hop synchronously when not already on stateQueue to avoid racing URLSession callbacks or queued client operations.

Proposed fix

 deinit {
-    disconnectOnStateQueue()
+    if DispatchQueue.getSpecific(key: stateQueueKey) != nil {
+        disconnectOnStateQueue()
+    } else {
+        stateQueue.sync {
+            disconnectOnStateQueue()
+        }
+    }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ios/Libraries/DDPClient.swift` around lines 28 - 30, deinit currently calls
disconnectOnStateQueue() from an arbitrary thread and must instead ensure it
runs on stateQueue synchronously; update deinit to check whether the current
execution is already on stateQueue (using the queue's specific key or equivalent
check used elsewhere in the class) and if so call disconnectOnStateQueue()
directly, otherwise call stateQueue.sync { disconnectOnStateQueue() } so all
state-queue-confined fields are mutated only while on stateQueue and avoid races
with URLSession callbacks or queued operations.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 167-183: The teardown drains pendingCallbacks, queuedMethodCalls
and clears connectedCallback but send-failure paths in login, subscribe and
callMethod may later call those same completions again; fix by guarding each
send-failure block to only invoke the completion if its corresponding entry
still exists (e.g., check pendingCallbacks[someId] != nil for
login/subscribe/callMethod, or that the queuedMethodCalls element still remains)
and remove the entry before calling, and similarly guard connectedCallback
before calling it; update the send-failure handlers referenced in login,
subscribe, and callMethod to perform this existence check before invoking the
stored completion.

---

Duplicate comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 28-30: deinit currently calls disconnectOnStateQueue() from an
arbitrary thread and must instead ensure it runs on stateQueue synchronously;
update deinit to check whether the current execution is already on stateQueue
(using the queue's specific key or equivalent check used elsewhere in the class)
and if so call disconnectOnStateQueue() directly, otherwise call stateQueue.sync
{ disconnectOnStateQueue() } so all state-queue-confined fields are mutated only
while on stateQueue and avoid races with URLSession callbacks or queued
operations.

🪄 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: e78e0fb7-2dea-4467-b1ce-ec9aa8245ec4

📥 Commits

Reviewing files that changed from the base of the PR and between 4d6eaf7 and 814e45c.

📒 Files selected for processing (1)

• ios/Libraries/DDPClient.swift

📜 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 (3)

ios/Libraries/DDPClient.swift (3)

50-50: LGTM — TLS challenge routing is centralized.

Creating the DDP URLSession with a delegate and forwarding challenges through Challenge.runChallenge matches the app’s existing SSL/client-certificate path.

Also applies to: 419-428

---

315-343: LGTM — receive failures now tear down safely.

The current-task guards and state-queue dispatch address stale receive callbacks, and the failure path now routes through centralized cleanup.

---

377-390: LGTM — collection routing is stricter without mutating payloads.

Forwarding only messages with a string collection keeps malformed DDP payloads out of the VoIP handler while preserving the original message dictionary.