🧹 Nitpick comments (1)

packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
`603-629`: **Consider renaming `anyTransceiverNotSending` for clarity.** The loop returns `false` as soon as any audio transceiver is still sending, so the flag only ever represents "at least one non-stopped audio transceiver exists and none of them are sending" — effectively the held condition itself. A name like `hasHeldAudioTransceiver` (or simply computing once at the end) would read closer to intent. Non-blocking.

♻️ Optional rename
```diff - let anyTransceiverNotSending = false; + let hasHeldAudioTransceiver = false; const transceivers = this.getTransceivers('audio'); for (const transceiver of transceivers) { if (!transceiver.currentDirection || transceiver.currentDirection === 'stopped') { continue; } if (transceiver.currentDirection.includes('send')) { this.setRemoteHeld(false); return; } - anyTransceiverNotSending = true; + hasHeldAudioTransceiver = true; } - this.setRemoteHeld(anyTransceiverNotSending); + this.setRemoteHeld(hasHeldAudioTransceiver); ```

🤖 Prompt for AI Agents
``` Verify each finding against the current code and only fix it if needed. In `@packages/media-signaling/src/lib/services/webrtc/Processor.ts` around lines 603 - 629, The variable anyTransceiverNotSending in updateRemoteHeld is misleading; change it to a clearer name like hasHeldAudioTransceiver (or compute the held condition directly) so the intent matches the logic that if any audio transceiver is sending we call setRemoteHeld(false) and otherwise we call setRemoteHeld(true); update references in updateRemoteHeld and keep the same behavior with getTransceivers('audio') and setRemoteHeld. ```

🤖 Prompt for all review comments with AI agents

``` Verify each finding against the current code and only fix it if needed. Nitpick comments: In `@packages/media-signaling/src/lib/services/webrtc/Processor.ts`: - Around line 603-629: The variable anyTransceiverNotSending in updateRemoteHeld is misleading; change it to a clearer name like hasHeldAudioTransceiver (or compute the held condition directly) so the intent matches the logic that if any audio transceiver is sending we call setRemoteHeld(false) and otherwise we call setRemoteHeld(true); update references in updateRemoteHeld and keep the same behavior with getTransceivers('audio') and setRemoteHeld. ```

---

ℹ️ Review info

⚙️ Run configuration

**Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `1c034ffa-6d05-4f8f-8fe3-1f5a28d8a96a`

📥 Commits

Reviewing files that changed from the base of the PR and between 92dbb53c915645428d7455178ac8e974f8bf305b and 2aca7de0aa21fa660120a11ef1daebc16c99bf5a.

📒 Files selected for processing (2)

* `packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts` * `packages/media-signaling/src/lib/services/webrtc/Processor.ts`

📜 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: 📦 Build Packages * GitHub Check: CodeQL-Build

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{ts,tsx,js}

**📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)** > `**/*.{ts,tsx,js}`: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests > Avoid code comments in the implementation Files: - `packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts` - `packages/media-signaling/src/lib/services/webrtc/Processor.ts`

🧠 Learnings (3)

📓 Common learnings

``` Learnt from: pierre-lehnen-rc Repo: RocketChat/Rocket.Chat PR: 36718 File: packages/media-signaling/src/lib/Call.ts:633-642 Timestamp: 2025-09-23T00:27:05.438Z Learning: In PR `#36718`, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work. ```

📚 Learning: 2026-02-26T19:25:44.063Z

``` Learnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 38778 File: packages/ui-voip/src/providers/useMediaSession.ts:192-192 Timestamp: 2026-02-26T19:25:44.063Z Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules. ``` **Applied to files:** - `packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts` - `packages/media-signaling/src/lib/services/webrtc/Processor.ts`

📚 Learning: 2026-02-26T19:25:44.063Z

``` Learnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 38778 File: packages/ui-voip/src/providers/useMediaSession.ts:192-192 Timestamp: 2026-02-26T19:25:44.063Z Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead. ``` **Applied to files:** - `packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts` - `packages/media-signaling/src/lib/services/webrtc/Processor.ts`

🔇 Additional comments (5)

packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
`16-16`: **LGTM!** Adding `remoteHeld: boolean` to `WebRTCInternalStateMap` cleanly aligns the public internal-state surface with the new cached value emitted by `MediaCallWebRTCProcessor` through `internalStateChange`. `getInternalState('remoteHeld')` in `Processor.ts` and the `updateRemoteStates()` consumer in `Call.ts` become properly typed.

packages/media-signaling/src/lib/services/webrtc/Processor.ts (4)
`49-49`: **Caching `_remoteHeld` and routing reads through it looks correct.** The field is initialized to `false`, exposed via both `isRemoteHeld()` and `getInternalState('remoteHeld')`, and only mutated through `setRemoteHeld()` which emits `internalStateChange`. This matches the `Call.updateRemoteStates()` consumer that reads `isRemoteHeld()` off the same `internalStateChange` event, so the cached value is guaranteed to be observed in sync. Also applies to: 234-235, 249-251 --- `110-110`: **Pre/post-negotiation wiring covers both offer and answer paths.** - Outgoing: `createOffer` → `processPreNegotiation`; `setRemoteDescription(answer)` → `processPostNegotiation`. - Incoming: `setRemoteDescription(offer)` → `processPreNegotiation`; `setLocalDescription(answer)` → `processPostNegotiation`. Running `updateRemoteHeld()` only from `processPostNegotiation` (after the `await peer.setLocal/RemoteDescription` resolves) rather than from arbitrary `onsignalingstatechange`/`onconnectionstatechange` callbacks is the right fix for the mobile case where `transceiver.currentDirection` lags the state-change event. Also applies to: 190-192, 207-214 --- `577-585`: **`setRemoteHeld` short-circuit and emit pattern are consistent with `setRemoteMute`.** Early return on no-change prevents spurious `internalStateChange` emissions (and therefore redundant `trackStateChange` emissions via `Call.updateRemoteStates()`). Good symmetry with `setRemoteMute` above. --- `603-610`: **The `connectionState` guard is correct and will not swallow hold updates during mid-call renegotiation.** During initial setup, `connectionState` is `'new'` and nothing is held yet, so early return is safe. During mid-call renegotiation, the connection is already established, so `connectionState` remains `'connected'` and `updateRemoteHeld()` executes normally to apply transceiver-based hold state. The guard only prevents hold updates if the connection has actually transitioned to `'closed'`, `'failed'`, or (unlikely) back to `'new'` via explicit ICE restart, which is correct behavior.