Skip to content

Conversation

@pierre-lehnen-rc
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Nov 5, 2025

Proposed changes (including videos or screenshots)

Issue(s)

Feature Branch
VGA-19

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes

    • Resolved an error where Voice Calls could fail to negotiate WebRTC parameters if both clients requested renegotiation simultaneously.
  • New Features

    • Enhanced tracking of remote peer hold and mute states during active calls.
    • Improved negotiation queue management and data channel handling for peer-to-peer command transmission.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Nov 5, 2025

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Nov 5, 2025

🦋 Changeset detected

Latest commit: 17291c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/media-signaling Minor
@rocket.chat/media-calls Minor
@rocket.chat/meteor Patch
@rocket.chat/core-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/ui-voip Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/livechat Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/cron Patch
@rocket.chat/freeswitch Patch
@rocket.chat/http-router Patch
@rocket.chat/model-typings Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/models Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 5, 2025

Walkthrough

This PR introduces negotiation management with sequence-based processing to resolve simultaneous WebRTC renegotiations between peers, consolidates new-call signal construction via a shared helper, adds remote hold/mute state synchronization across call participants, and refactors WebRTC processor negotiations with data-channel support for peer-to-peer commands.

Changes

Cohort / File(s) Summary
New-Call Signal Consolidation
.changeset/six-adults-lie.md, ee/packages/media-calls/src/internal/SignalProcessor.ts, ee/packages/media-calls/src/internal/agents/UserActorAgent.ts, ee/packages/media-calls/src/server/buildNewCallSignal.ts
Introduces buildNewCallSignal helper module and consolidates per-role new-call signal construction across multiple call paths (processCallSignal, reactToUnknownCall, getExistingRequestedCall), replacing inline signal objects with unified function calls.
Negotiation Manager & Sequence-Based Glare Resolution
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts, packages/media-signaling/src/lib/NegotiationManager.ts, packages/media-signaling/src/lib/services/webrtc/Negotiation.ts
Adds NegotiationManager class orchestrating queued, sequence-based WebRTC negotiations and Negotiation class for individual negotiation lifecycle, enabling polite/impolite peer distinction to resolve simultaneous renegotiation requests. Introduces startNewNegotiation helper in CallSignalProcessor.
Remote State Synchronization
packages/media-signaling/src/definition/call/IClientMediaCall.ts, packages/media-signaling/src/lib/Call.ts, packages/media-signaling/src/lib/services/webrtc/Processor.ts
Adds remoteHeld and remoteMute state properties to media calls, integrates data-channel-based P2P commands for mute/unmute/end signaling, and updates processor to track and synchronize remote hold/mute status.
Interface & Type Updates
packages/media-signaling/src/definition/client.ts, packages/media-signaling/src/definition/services/IServiceProcessor.ts, packages/media-signaling/src/definition/services/negotiation.ts, packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts, packages/media-signaling/src/definition/signals/server/new.ts, packages/media-signaling/src/definition/signals/client/local-state.ts
Removes has-offer/has-answer ClientState enums; makes IServiceProcessor generic with unique-events support; adds NegotiationManagerEvents, NegotiationData, INegotiationCompatibleMediaCall types; extends IWebRTCProcessor with setLocalDescription, setRemoteDescription, getLocalDescription, waitForIceGathering, isRemoteHeld, isRemoteMute methods; adds flags?: CallFlag[] to ServerMediaSignalNewCall.
Module Exports & Signaling Infrastructure
packages/media-signaling/src/definition/services/index.ts
Re-exports negotiation type definitions to expand public API surface.
WebRTC Processor Refactor
packages/media-signaling/src/lib/services/webrtc/Processor.ts
Refactors negotiation flow replacing startNewNegotiation with sequence-driven NegotiationManager integration; adds data-channel creation/lifecycle for P2P commands; introduces centralized initialization with input-track loading; adds audio-direction management methods (updateAudioDirectionBeforeNegotiation, updateAudioDirectionAfterNegotiation); updates createOffer/createAnswer to return RTCSessionDescriptionInit directly and adds setLocalDescription/setRemoteDescription with SDP validation.

Sequence Diagram

sequenceDiagram
    actor A as Peer A
    participant NM_A as NegotiationManager<br/>(Impolite)
    participant WR_A as WebRTC<br/>Processor A
    participant WR_B as WebRTC<br/>Processor B
    participant NM_B as NegotiationManager<br/>(Polite)
    actor B as Peer B

    autonumber
    
    Note over A,B: Simultaneous Renegotiation Request (Glare Scenario)
    
    A->>NM_A: negotiationNeeded event (seq=100)
    B->>NM_B: negotiationNeeded event (seq=100)
    
    rect rgb(200, 220, 255)
        Note over NM_A: Impolite: Create Offer<br/>Add to queue, mark other negotiations for end
    end
    
    rect rgb(220, 200, 255)
        Note over NM_B: Polite: Wait for remote offer<br/>Add to queue as pending
    end
    
    NM_A->>WR_A: processNegotiation() → createOffer()
    WR_A-->>NM_A: local-sdp (offer)
    NM_A->>A: emit local-sdp event
    A->>B: send offer signal

    B->>NM_B: addNegotiation(offerId, remoteOffer)
    rect rgb(220, 200, 255)
        Note over NM_B: Polite: Remote offer received<br/>Now process as local-answer
    end
    
    NM_B->>WR_B: processNegotiation() → createAnswer()
    WR_B-->>NM_B: local-sdp (answer)
    NM_B->>B: emit local-sdp event
    B->>A: send answer signal
    
    A->>NM_A: setRemoteDescription(answerId, remoteAnswer)
    NM_A->>WR_A: setRemoteDescription(answer)
    rect rgb(200, 220, 255)
        Note over WR_A: Negotiation ended
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas requiring extra attention:

  • NegotiationManager.ts: Complex sequence-based queuing logic with impolite/polite peer distinction, highest-sequence tracking, and negotiation lifecycle. Verify glare-resolution correctness and queue ordering invariants.
  • Negotiation.ts: Lifecycle state management (started, ended, failed flags) and branching logic for offer vs. answer creation. Ensure error handling and event emission are complete.
  • CallSignalProcessor.ts: Multi-branch renegotiation logic with per-path decision trees (polite vs. impolite, duplicate detection). Verify all code paths correctly delegate to startNewNegotiation.
  • Processor.ts: Large refactor replacing startNewNegotiation with initialization flow, data-channel lifecycle, and audio-direction management. Check that ICE gathering, input-track initialization, and SDP validation align with WebRTC spec expectations.
  • Call.ts: Integration with NegotiationManager, removal of prior negotiation state fields (currentNegotiationId, hasLocalDescription, etc.), and wiring of negotiation events to signal delivery. Verify state reporting and signal sequencing are correct.

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • KevLehman
  • tassoevan

Poem

🐰 A negotiation dance, no more glare!
When impolite meets polite with care,
Sequences queued, no hearts conflare—
Both hold and mute now, the whole affair,
Synchronized whispers through data's lair! 📡

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: sync call's "on hold" and "mute" states' directly reflects the main objective of synchronizing on-hold and mute states across call participants.
Linked Issues check ✅ Passed The PR implements all functional requirements from VGA-19: adds remote hold/mute state synchronization, introduces negotiation flow improvements to prevent simultaneous renegotiation conflicts, and provides infrastructure for real-time state updates across participants.
Out of Scope Changes check ✅ Passed All changes are directly related to enabling on-hold/mute state synchronization: refactored negotiation handling, added remote state tracking, introduced data channel for P2P commands, and consolidated signal building logic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/voip-sync-audio-direction

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

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

@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.99%. Comparing base (2b79c70) to head (17291c3).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #37386      +/-   ##
===========================================
+ Coverage    68.96%   68.99%   +0.02%     
===========================================
  Files         3358     3358              
  Lines       114228   114228              
  Branches     20537    20537              
===========================================
+ Hits         78778    78812      +34     
+ Misses       33357    33325      -32     
+ Partials      2093     2091       -2     
Flag Coverage Δ
e2e 57.42% <ø> (+0.01%) ⬆️
e2e-api 43.85% <ø> (+0.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pierre-lehnen-rc pierre-lehnen-rc added this to the 7.13.0 milestone Nov 5, 2025
@gabriellsh gabriellsh marked this pull request as ready for review November 14, 2025 17:35
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
ee/packages/media-calls/src/server/buildNewCallSignal.ts (1)

23-42: Consider making flags field optional when empty.

The signal builder correctly constructs the payload and handles optional fields appropriately. However, Line 37 always includes the flags field, even when it's an empty array.

Consider conditionally including flags only when non-empty to reduce payload size:

 export function buildNewCallSignal(call: IMediaCall, role: CallRole): ServerMediaSignalNewCall {
 	const self = role === 'caller' ? call.caller : call.callee;
 	const contact = role === 'caller' ? call.callee : call.caller;
 	const transferredBy = getNewCallTransferredBy(call);
 	const flags = getCallFlags(call, role);
 
 	return {
 		callId: call._id,
 		type: 'new',
 		service: call.service,
 		kind: call.kind,
 		role,
 		self: { ...self },
 		contact: { ...contact },
-		flags,
+		...(flags.length > 0 && { flags }),
 		...(call.parentCallId && { replacingCallId: call.parentCallId }),
 		...(transferredBy && { transferredBy }),
 		...(call.callerRequestedId && role === 'caller' && { requestedCallId: call.callerRequestedId }),
 	};
 }
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)

179-240: Renegotiation flow looks consistent with “polite/impolite” rules

The revised processNegotiationNeeded logic plus startNewNegotiation() reads coherent: unsigned clients are rejected, initial‑negotiation absence short‑circuits, completed negotiations always spawn a fresh one, and concurrent/outdated requests are resolved based on a fixed impolite side (caller) and whether the request references the latest negotiation.

I don’t see a concrete correctness issue here; the sequencing and guards should prevent overlapping negotiations while still allowing the impolite side to preempt a pending polite one. If you see noisy states in practice, the only thing I’d consider is enriching the error/debug logs (e.g., include oldNegotiationId, negotiation._id, and callId in the two 'Invalid state for renegotiation request' branches) to make triaging easier, but that’s optional.

Also applies to: 242-247

packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)

13-14: WebRTC processor API extensions align with negotiation and remote mute/hold needs

The additions to WebRTCInternalStateMap (remoteMute) and the isRemoteHeld / isRemoteMute helpers give a clear contract for tracking remote audio direction, which should support the “sync mute/hold state” feature well. Extending IWebRTCProcessor from IServiceProcessor<WebRTCInternalStateMap, WebRTCUniqueEvents> and defining WebRTCProcessorEvents keeps the negotiation‑specific negotiationNeeded event strongly typed on the emitter.

Two small points you might consider:

  • The explicit emitter: Emitter<WebRTCProcessorEvents>; is now redundant with the inherited emitter from IServiceProcessor (it has the same effective type). You could drop the explicit property here to avoid duplication, but it’s not required.
  • With createOffer / createAnswer returning RTCSessionDescriptionInit directly, it’s worth double‑checking that all call sites (especially older ones) have been updated to use the new return shape.

Also applies to: 16-18, 20-24, 32-39, 46-47

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7d90c9d and 9984ab6.

📒 Files selected for processing (17)
  • .changeset/six-adults-lie.md (1 hunks)
  • ee/packages/media-calls/src/internal/SignalProcessor.ts (3 hunks)
  • ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1 hunks)
  • ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (2 hunks)
  • ee/packages/media-calls/src/server/buildNewCallSignal.ts (1 hunks)
  • packages/media-signaling/src/definition/call/IClientMediaCall.ts (2 hunks)
  • packages/media-signaling/src/definition/client.ts (0 hunks)
  • packages/media-signaling/src/definition/services/IServiceProcessor.ts (1 hunks)
  • packages/media-signaling/src/definition/services/index.ts (1 hunks)
  • packages/media-signaling/src/definition/services/negotiation.ts (1 hunks)
  • packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (2 hunks)
  • packages/media-signaling/src/definition/signals/client/local-state.ts (1 hunks)
  • packages/media-signaling/src/definition/signals/server/new.ts (2 hunks)
  • packages/media-signaling/src/lib/Call.ts (18 hunks)
  • packages/media-signaling/src/lib/NegotiationManager.ts (1 hunks)
  • packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1 hunks)
  • packages/media-signaling/src/lib/services/webrtc/Processor.ts (16 hunks)
💤 Files with no reviewable changes (1)
  • packages/media-signaling/src/definition/client.ts
🧰 Additional context used
🧬 Code graph analysis (11)
packages/media-signaling/src/definition/signals/server/new.ts (1)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
  • CallFlag (67-67)
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (2)
ee/packages/media-calls/src/logger.ts (1)
  • logger (3-3)
packages/models/src/index.ts (1)
  • MediaCallNegotiations (186-186)
packages/media-signaling/src/definition/services/negotiation.ts (2)
packages/media-signaling/src/definition/logger.ts (1)
  • IMediaSignalLogger (1-6)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
  • IClientMediaCall (69-107)
ee/packages/media-calls/src/server/buildNewCallSignal.ts (4)
packages/core-typings/src/mediaCalls/IMediaCall.ts (1)
  • IMediaCall (35-68)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (2)
  • CallRole (17-17)
  • CallFlag (67-67)
packages/media-signaling/src/definition/signals/server/new.ts (1)
  • ServerMediaSignalNewCall (4-23)
ee/packages/media-calls/src/server/getNewCallTransferredBy.ts (1)
  • getNewCallTransferredBy (4-20)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
packages/media-signaling/src/definition/services/IServiceProcessor.ts (2)
  • ServiceProcessorEvents (14-17)
  • IServiceProcessor (19-26)
ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (1)
ee/packages/media-calls/src/server/buildNewCallSignal.ts (1)
  • buildNewCallSignal (23-42)
packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (2)
  • NegotiationEvents (14-18)
  • NegotiationData (20-26)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
  • IWebRTCProcessor (22-48)
packages/media-signaling/src/definition/logger.ts (1)
  • IMediaSignalLogger (1-6)
packages/media-signaling/src/lib/Call.ts (4)
packages/media-signaling/src/lib/NegotiationManager.ts (1)
  • NegotiationManager (6-289)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
  • CallFlag (67-67)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (2)
  • isRemoteHeld (269-294)
  • isRemoteMute (296-298)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
  • WebRTCInternalStateMap (7-14)
ee/packages/media-calls/src/internal/SignalProcessor.ts (2)
ee/packages/media-calls/src/server/buildNewCallSignal.ts (1)
  • buildNewCallSignal (23-42)
packages/media-signaling/src/lib/Call.ts (1)
  • role (62-64)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (2)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
  • WebRTCInternalStateMap (7-14)
packages/media-signaling/src/lib/Call.ts (2)
  • muted (112-118)
  • state (68-70)
packages/media-signaling/src/lib/NegotiationManager.ts (3)
packages/media-signaling/src/definition/services/negotiation.ts (3)
  • NegotiationManagerEvents (4-8)
  • INegotiationCompatibleMediaCall (28-30)
  • NegotiationManagerConfig (10-12)
packages/media-signaling/src/lib/services/webrtc/Negotiation.ts (1)
  • Negotiation (5-163)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
  • IWebRTCProcessor (22-48)
⏰ 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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (82)
.changeset/six-adults-lie.md (1)

1-6: LGTM!

The changeset correctly documents the minor version bumps and describes the fix for simultaneous WebRTC renegotiation issues.

packages/media-signaling/src/definition/services/negotiation.ts (1)

1-30: LGTM!

The type definitions are well-structured and align with the PR's objective to fix negotiation race conditions. The sequence field in NegotiationData (Line 22) appears to be a key component for resolving simultaneous renegotiation attempts.

packages/media-signaling/src/definition/services/index.ts (1)

4-4: LGTM!

The export addition correctly exposes the negotiation types introduced in the PR.

packages/media-signaling/src/definition/call/IClientMediaCall.ts (3)

67-67: LGTM!

The CallFlag type enables feature toggling for internal calls and data channel creation, which aligns with the new negotiation architecture.


73-73: LGTM!

The flags property enables clients to receive behavioral configuration from the server, supporting the new call features introduced in this PR.


84-86: LGTM! Directly addresses PR objectives.

The remoteHeld and remoteMute properties are essential for synchronizing hold and mute states across all call participants, which directly fulfills acceptance criteria AC1-AC4 mentioned in the PR objectives.

ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (2)

2-2: LGTM!

The import changes correctly reflect the refactoring to use the external signal builder.

Also applies to: 8-8


80-80: LGTM! Centralizes signal construction.

Replacing inline signal construction with the external buildNewCallSignal helper eliminates code duplication and ensures consistent signal generation across different code paths.

packages/media-signaling/src/definition/signals/server/new.ts (2)

1-1: LGTM!

The import correctly adds the CallFlag type needed for the new flags field.


20-22: LGTM!

The optional flags field enables the server to communicate behavioral toggles to clients, supporting the new negotiation and data channel features.

ee/packages/media-calls/src/server/buildNewCallSignal.ts (1)

6-21: LGTM!

The getCallFlags helper correctly determines call flags based on participant types. The logic ensures that only the caller creates the data channel for internal calls, which prevents conflicts.

packages/media-signaling/src/definition/signals/client/local-state.ts (1)

44-44: Migration from removed client states is clean and safe.

The removed states ('has-offer', 'has-answer', 'has-new-offer', 'has-new-answer') do not appear in the codebase. The new enum at line 44 correctly defines the valid states, and Call.ts implementation uses only these valid states. No dangling references detected.

packages/media-signaling/src/definition/services/IServiceProcessor.ts (1)

19-24: Emitter typing extension is clean and backward compatible

Parameterizing IServiceProcessor with ServiceUniqueEvents and intersecting it into the emitter type is a neat way to add service‑specific events without losing the shared internal ones. The default keeps existing behavior for callers that don’t care about unique events.

ee/packages/media-calls/src/internal/SignalProcessor.ts (1)

3-10: Consolidating “new call” signal construction via buildNewCallSignal looks good

Routing both reactToUnknownCall and getExistingRequestedCall through buildNewCallSignal removes duplicated payload construction and should keep flags / optional fields in sync across all “new call” flows. The added isPendingState usage around notifications maintains the distinction between pending vs already‑accepted calls.

Assuming buildNewCallSignal encodes the same fields as the previous inline objects (including flags and any requestedCallId/transfer metadata), this is a solid cleanup.

Also applies to: 17-18, 150-151, 244-245

packages/media-signaling/src/lib/Call.ts (18)

5-5: LGTM!

The new imports for NegotiationManager and CallFlag are correctly added to support the centralized negotiation orchestration and flag-based feature control.

Also applies to: 15-15


129-139: LGTM!

The remoteHeld state accessors follow the established pattern used for other state properties in the class.


135-139: LGTM!

The remoteMute state accessors are consistent with the pattern used for other state properties.


181-181: LGTM!

The NegotiationManager is properly initialized in the constructor with the call instance and logger configuration. The initialization order is correct.

Also applies to: 234-234


191-195: LGTM!

The flags accessors follow the established pattern. The flags array enables feature control such as data channel creation.


230-232: LGTM!

The remote state fields are appropriately initialized to false and the flags array to empty, representing the initial state before remote data is received.


296-296: LGTM!

The flags initialization with a fallback to an empty array ensures backward compatibility with signals that don't include flags.


414-423: LGTM!

The logic correctly triggers deferred negotiations when an input track is added. This aligns with the NegotiationManager.isConfigured() check that waits for the input track before processing negotiations.


672-672: LGTM!

The conditional inclusion of negotiationId in the state report uses appropriate optional property spreading and allows the server to track the client's current negotiation state.


765-786: LGTM!

The addition of the hidden check prevents processing offer requests for calls not happening on this session, and the delegation to negotiationManager.addNegotiation correctly centralizes negotiation handling.


802-812: LGTM!

The method correctly delegates answer request processing to the NegotiationManager with the remote SDP.


845-850: LGTM!

The SDP type validation and delegation to negotiationManager.setRemoteDescription correctly handle remote answer processing.


853-853: LGTM!

The updated signature with negotiationId enables proper tracking of which negotiation the SDP belongs to, supporting the sequence-based negotiation management.


1013-1028: LGTM!

The updateRemoteStates method correctly queries the WebRTC processor for remote state changes and only emits the trackStateChange event when actual changes occur, preventing unnecessary event notifications.


1030-1051: LGTM!

The type check on line 1037 correctly filters for string state values (excluding the boolean remoteMute), and calling updateRemoteStates ensures the UI reflects the latest remote hold/mute states after any internal state change.


1053-1067: LGTM!

The new event handlers correctly manage negotiation lifecycle events:

  • onNegotiationNeeded requests renegotiation with the old negotiation ID
  • onNegotiationError reports errors with negotiation context as non-critical

1087-1087: LGTM!

The inclusion of negotiationId in connection failure error reports provides valuable context for debugging which negotiation encountered issues.

Also applies to: 1099-1099


1163-1167: LGTM!

The event wiring correctly connects NegotiationManager events to the Call's handlers, and calling setWebRTCProcessor last ensures the processor is fully configured before negotiations can begin processing.

packages/media-signaling/src/lib/NegotiationManager.ts (13)

1-46: LGTM!

The class structure is well-organized with proper initialization of all state fields. The currentNegotiationId getter correctly falls back to highestNegotiationId when no negotiation is actively processing.


48-87: LGTM!

The addNegotiation method implements the perfect negotiation pattern correctly:

  • Duplicate and type validation prevent errors
  • Sequence-based skipping prevents processing stale negotiations
  • Politeness determination (remoteOffer !== isPoliteClient()) correctly identifies which side should yield during conflicts

89-113: LGTM!

The method correctly handles both offers (by delegating to addNegotiation) and answers (by validating against the current negotiation). Error handling properly cleans up state and emits error events.


115-121: LGTM!

The WebRTC processor integration correctly wires internal error and negotiation needed events to the manager's handlers.


123-139: LGTM!

The method correctly guards against processing negotiations when not configured or when a negotiation is already in progress, preventing race conditions.


168-191: LGTM!

The queue selection logic correctly implements the perfect negotiation pattern by prioritizing impolite negotiations and only processing polite negotiations when the queue is otherwise empty.


193-227: LGTM!

The negotiation processing correctly wires event handlers and includes proper error handling. The recursive call to processNegotiations on line 207 ensures the queue continues processing after a negotiation completes.


229-247: LGTM!

The configuration checks ensure negotiations only proceed when the call is active, the processor is available, and the input track is ready. The type guard correctly narrows the type for subsequent operations.


249-257: LGTM!

The logic correctly determines whether a queued negotiation will fulfill current needs based on the polite/impolite pattern, preventing redundant renegotiation requests.


259-277: LGTM!

The negotiation-needed handler correctly guards against redundant renegotiation requests and provides the old negotiation ID as a reference point for the server to generate a properly sequenced new negotiation.


279-288: LGTM!

The error handler correctly extracts error codes from both string and Error object types and only emits errors when there's a negotiation context.


291-293: LGTM!

The abstract class correctly serves as a type assertion helper for when the WebRTC processor is guaranteed to exist, enabling type-safe operations in processNegotiation.


145-166: Concurrent negotiation handling is safe. The Negotiation class properly guards against mid-processing termination through assertNegotiationIsActive() checks at all critical points (lines 101, 104, 107, 135, 144, 147, 150), and the end() method is idempotent. When currentNegotiation.end() is called from NegotiationManager, subsequent operations fail gracefully without race conditions or cascading errors.

packages/media-signaling/src/lib/services/webrtc/Processor.ts (37)

9-10: LGTM!

The data channel label constant and P2P command type provide clear identification and type safety for peer-to-peer signaling.


59-69: LGTM!

The new fields properly support centralized initialization and data channel lifecycle management.


79-79: LGTM!

The constructor correctly initializes the data channel state and ensures the processor stops if initialization fails, preventing operations in an invalid state.

Also applies to: 91-94


101-111: LGTM!

Awaiting initialization before setting the input track prevents race conditions and ensures the processor is in a valid state.


113-140: LGTM!

The updated createOffer method correctly initializes the data channel (offerer's responsibility), manages audio direction before negotiation, and returns a clean offer description.


142-150: LGTM!

The mute handler correctly updates local state and synchronizes with the remote peer via the data channel, fulfilling the PR's objective of synchronizing mute states.


152-162: LGTM!

The hold handler correctly updates local state and adjusts audio direction without triggering unnecessary renegotiations when in a stable signaling state.


164-175: LGTM!

The stop method correctly cleans up the data channel before stopping the processor, ensuring a clean shutdown.


177-195: LGTM!

The simplified createAnswer focuses on validation and delegates audio direction management to the set description methods, improving code clarity.


197-214: LGTM!

The new setLocalDescription method correctly validates SDP types and updates audio direction after setting a local answer, ensuring transceivers reflect negotiated states.


216-237: LGTM!

The setRemoteDescription method correctly manages audio direction before processing offers and after processing answers, implementing proper WebRTC negotiation flow.


254-256: LGTM!

Adding remoteMute to the internal state map enables the Call class to track and react to remote mute state changes via the existing state change event mechanism.


259-267: LGTM!

Awaiting initialization before getting stats ensures the processor is in a valid state for querying statistics.


269-294: LGTM!

The isRemoteHeld method correctly detects when the remote peer has placed the call on hold by checking if audio transceivers have stopped sending, fulfilling the PR's objective of synchronizing hold states.


296-298: LGTM!

The isRemoteMute accessor provides access to the remote mute state synchronized via the data channel.


300-306: LGTM!

The isStable helper correctly determines when the signaling state allows non-negotiation operations.


308-315: LGTM!

The method correctly provides access to the local description and throws an appropriate error when the processor is stopped.


317-342: LGTM!

Awaiting initialization before ICE gathering ensures the processor is properly set up before gathering candidates.


344-348: LGTM!

The initialize method provides centralized initialization logic for loading the input track.


350-358: LGTM!

The renamed method more accurately reflects its purpose of resetting ICE gathering state rather than managing broader negotiation lifecycle.


360-377: LGTM!

The method correctly sets transceiver directions before negotiation based on the hold state, indicating whether the local peer wants to receive audio.


379-403: LGTM!

The post-negotiation audio direction update intelligently prevents redundant renegotiations by accepting the negotiated direction when it's acceptable, even if it differs from the initially desired direction.


405-409: LGTM!

The helper method provides a clean way to filter audio transceivers, used consistently throughout the audio direction management logic.


411-431: LGTM!

The method correctly updates audio direction when hold state changes mid-call, but only when the signaling state is stable, allowing the browser to trigger renegotiation if the change requires it.


433-441: LGTM!

The data channel creation is properly guarded to prevent recreation when unnecessary and respects the feature flag for conditional enablement.


443-446: LGTM!

The method correctly signals the remote peer and prevents recreation of the data channel after intentional shutdown.


491-504: LGTM!

The command sending method correctly guards against unavailable or closed channels and uses JSON encoding for structured messaging.


506-521: LGTM!

The command validation and parsing logic correctly handles JSON parsing errors and provides type-safe command validation.


523-536: LGTM!

The command handler correctly processes remote peer commands, updating local state to reflect the remote peer's mute status and handling channel shutdown.


538-545: LGTM!

The method correctly updates remote mute state only when changed and emits the appropriate internal state change event for UI updates.


547-550: LGTM!

The method ensures the remote peer is notified of local mute state changes, completing the bidirectional mute synchronization required by the PR objectives.


563-563: LGTM!

Registering the ondatachannel event allows the answerer to receive and initialize the data channel created by the offerer.


578-578: LGTM!

The data channel event handler is properly cleaned up during processor shutdown.


629-633: LGTM!

The ICE restart correctly uses the renamed startNewGathering method.


654-660: LGTM!

The guard prevents negotiation-needed events from firing during ongoing negotiations, following WebRTC best practices.


696-713: LGTM!

The ICE gathering state change handler correctly resets the candidate counter when gathering starts, ensuring accurate tracking.


726-729: LGTM!

The handler correctly initializes data channels received from the remote peer, completing the peer-to-peer setup.

@Harmeet221 Harmeet221 added the stat: QA assured Means it has been tested and approved by a company insider label Nov 17, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Nov 17, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2025

📦 Docker Image Size Report

📈 Changes

Service Current Baseline Change Percent
sum of all images 1.2GiB 1.2GiB +12MiB
rocketchat 365MiB 354MiB +12MiB
omnichannel-transcript-service 141MiB 141MiB +6.3KiB
queue-worker-service 141MiB 141MiB +5.8KiB
ddp-streamer-service 127MiB 127MiB +9.3KiB
account-service 114MiB 114MiB +8.2KiB
stream-hub-service 111MiB 111MiB +9.6KiB
authorization-service 111MiB 111MiB +8.0KiB
presence-service 111MiB 111MiB +9.3KiB

📊 Historical Trend

---
config:
  theme: "dark"
  xyChart:
    width: 900
    height: 400
---
xychart
  title "Image Size Evolution by Service (Last 30 Days + This PR)"
  x-axis ["11/15 19:34", "11/15 19:47", "11/15 20:39", "11/15 21:23", "11/15 21:37", "11/15 22:04", "11/15 22:28", "11/16 00:55", "11/16 01:28", "11/17 12:35", "11/17 12:48", "11/17 12:54", "11/17 14:13", "11/17 15:17", "11/17 15:18 (PR)"]
  y-axis "Size (GB)" 0 --> 0.5
  line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
  line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
  line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
  line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14, 0.14]
  line "rocketchat" [0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.36, 0.35, 0.35, 0.35, 0.35, 0.35, 0.36]
  line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Loading

Statistics (last 14 days):

  • 📊 Average: 1.4GiB
  • ⬇️ Minimum: 1.2GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 1.2GiB
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-37386
  • Baseline: develop
  • Timestamp: 2025-11-17 15:18:34 UTC
  • Historical data points: 14

Updated: Mon, 17 Nov 2025 15:18:35 GMT

@kodiakhq kodiakhq bot merged commit fcb2d1f into develop Nov 17, 2025
49 checks passed
@kodiakhq kodiakhq bot deleted the feat/voip-sync-audio-direction branch November 17, 2025 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants