chore: sync call's "mute" state between clients #37389
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughCentralizes new-call signal construction into a new buildNewCallSignal(call, role) function, adds a CallFlag type and propagates flags through signals and client calls, and implements optional WebRTC data-channel handling with remote-mute synchronization gated by a Changes
Sequence Diagram(s)sequenceDiagram
participant Creator as Call Creator
participant SignalProc as SignalProcessor
participant Builder as buildNewCallSignal
participant ServerSig as Server Signal
participant Client as Client Call
participant WebRTC as WebRTC Processor
participant DC as DataChannel
Creator->>SignalProc: processCallSignal(call, role)
SignalProc->>Builder: buildNewCallSignal(call, role)
Builder-->>SignalProc: ServerMediaSignalNewCall (includes flags)
SignalProc->>ServerSig: sendSignal(new-call)
ServerSig->>Client: receive new-call (flags)
Client->>WebRTC: initialize with signal (flags)
alt flags contains create-data-channel
WebRTC->>WebRTC: createDataChannel()
WebRTC->>DC: initializeDataChannel(label)
DC-->>WebRTC: open / onmessage handlers set
else
WebRTC->>WebRTC: skip data channel creation
end
alt remote peer creates data channel
WebRTC->>WebRTC: ondatachannel(event)
WebRTC->>DC: initialize received channel
DC-->>WebRTC: ready
end
DC-->>WebRTC: onmessage ("mute"/"unmute"/"end")
WebRTC->>Client: update remote mute state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
…into chore/voip-data-channel
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/media-signaling/src/lib/Call.ts (1)
289-289: Consider using nullish coalescing operator for consistency.While
|| []works correctly here, the nullish coalescing operator?? []is more semantically accurate for handlingnull/undefinedvalues and aligns with modern TypeScript patterns.Apply this diff:
- this._flags = signal.flags || []; + this._flags = signal.flags ?? [];
📜 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.
📒 Files selected for processing (7)
ee/packages/media-calls/src/internal/SignalProcessor.ts(3 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(1 hunks)packages/media-signaling/src/definition/signals/server/new.ts(2 hunks)packages/media-signaling/src/lib/Call.ts(4 hunks)packages/media-signaling/src/lib/services/webrtc/Processor.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/media-signaling/src/lib/Call.ts (1)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
CallFlag(67-67)
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/signals/server/new.ts (1)
packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
CallFlag(67-67)
ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (1)
ee/packages/media-calls/src/server/buildNewCallSignal.ts (1)
buildNewCallSignal(23-42)
ee/packages/media-calls/src/internal/SignalProcessor.ts (3)
ee/packages/media-calls/src/server/buildNewCallSignal.ts (1)
buildNewCallSignal(23-42)packages/media-signaling/src/lib/Call.ts (1)
role(62-64)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
role(44-46)
⏰ 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 comments (10)
packages/media-signaling/src/definition/signals/server/new.ts (1)
1-23: Flags plumbing looks goodImporting
CallFlagand exposingflags?: CallFlag[]keeps the signal aligned with the new call metadata without breaking existing payloads.packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
67-106: Call flags surface is clearMaking
CallFlagexplicit and addingflagsto the client call contract keeps the type system in sync with the new signaling data.ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (1)
2-82: Centralized signal builder makes senseRouting
onCallCreatedthrough the sharedbuildNewCallSignalhelper removes duplicate payload logic and keeps the agent aligned with the server-side defaults.ee/packages/media-calls/src/internal/SignalProcessor.ts (1)
4-245: Good reuse of buildNewCallSignalSwitching the various call-entry paths to
buildNewCallSignalkeeps role-specific payloads consistent and adds the new flag list without duplicating logic.ee/packages/media-calls/src/server/buildNewCallSignal.ts (3)
1-4: LGTM! Clean imports and dependencies.The imports are well-organized and include all necessary types and helper functions.
23-42: LGTM! Well-structured signal builder with proper optional field handling.The function correctly:
- Determines self/contact based on role
- Retrieves transferred-by information
- Computes flags using the helper
- Uses spread operators to conditionally include optional fields
- Creates shallow copies of contact objects to avoid reference issues
6-21: Data channel creation logic is correctly implemented.Verification confirms the implementation aligns with the PR objective:
- The client properly checks the
create-data-channelflag at line 397 before attempting creation- The callee safely returns early due to the missing flag, preventing duplicate channel creation
- Both parties use symmetric initialization via
initializeDataChannel(), enabling bidirectional communication once the channel is established- The WebRTC pattern (caller creates, callee receives via
ondatachannelevent) is correctly implementedpackages/media-signaling/src/lib/Call.ts (3)
15-15: LGTM! Import addition is correct.
185-189: LGTM! Proper encapsulation of flags.The private field with public getter pattern correctly exposes the flags as read-only to consumers.
225-225: LGTM! Proper initialization for outbound calls.The empty array initialization is correct for outbound calls created before receiving server data.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (2)
420-428: Handle closed data channels to allow recreation.If the peer closes the data channel,
_dataChannelcontinues to reference the closed instance, preventingcreateDataChannelfrom creating a new channel during subsequent negotiations. This breaks data exchange for the remainder of the call.Apply this diff to detect and clear closed channels:
private createDataChannel(): void { + if (this._dataChannel?.readyState === 'closed') { + this._dataChannel = null; + } + if (this._dataChannel || !this.config.call.flags.includes('create-data-channel')) { return; }
430-450: Add onclose and onerror handlers for robustness.The data channel lacks lifecycle and error handling:
- Missing
onclosehandler prevents clearing_dataChannel, blocking recreation (flagged in past review).- Missing
onerrorhandler leaves errors unlogged and unhandled.Apply this diff to add both handlers:
channel.onmessage = (event) => { this.config.logger?.debug('Data Channel Message', event.data); }; + channel.onerror = (event) => { + this.config.logger?.error('Data Channel Error', event); + }; + + channel.onclose = () => { + this.config.logger?.debug('Data Channel Close', channel.label); + if (this._dataChannel === channel) { + this._dataChannel = null; + } + }; + this._dataChannel = channel; }
🧹 Nitpick comments (2)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (2)
463-463: Add ondatachannel cleanup in unregisterPeerEvents.The
ondatachannelhandler is registered but not explicitly unregistered inunregisterPeerEvents(lines 466-481), which cleans up all other peer event handlers.Apply this diff to add the cleanup:
peer.onicegatheringstatechange = null as any; peer.onsignalingstatechange = null as any; + peer.ondatachannel = null as any; } catch {
158-168: Consider explicit data channel cleanup in stop().While
peer.close()should close all associated channels, explicitly closing and clearing_dataChannelwould make the cleanup more defensive and clear.Apply this diff to add explicit cleanup:
this.stopped = true; // Stop only the remote stream; the track of the local stream may still be in use by another call so it's up to the session to stop it. this.remoteStream.stopAudio(); + + if (this._dataChannel) { + this._dataChannel.close(); + this._dataChannel = null; + } + this.unregisterPeerEvents(); this.unregisterAudioLevelTracker();
📜 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.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts(7 hunks)
⏰ 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/voip-sync-audio-direction #37389 +/- ##
==================================================================
+ Coverage 68.05% 68.07% +0.01%
==================================================================
Files 3364 3364
Lines 115626 115626
Branches 20869 20874 +5
==================================================================
+ Hits 78687 78710 +23
+ Misses 34251 34227 -24
- Partials 2688 2689 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
changed the title
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)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
239-255: Handle the newremoteMutestate ingetInternalState
setRemoteMute()emitsinternalStateChange('remoteMute'), butgetInternalState('remoteMute')currently falls through the switch and returnsundefined. Any consumer reacting to the event will never see the actual boolean value. Please add an explicit branch forremoteMute.switch (stateName) { case 'signaling': return this.peer.signalingState; case 'connection': return this.peer.connectionState; case 'iceConnection': return this.peer.iceConnectionState; case 'iceGathering': return this.peer.iceGatheringState; case 'iceUntrickler': if (this.iceGatheringTimedOut) { return 'timeout'; } return this.iceGatheringWaiters.size > 0 ? 'waiting' : 'not-waiting'; + case 'remoteMute': + return this._remoteMute; }
🧹 Nitpick comments (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
563-571: ClearondatachannelinunregisterPeerEventsWe now register
peer.ondatachannel, but the teardown path doesn’t null it like the other handlers. Please clear it to keep the cleanup symmetrical.peer.onicecandidateerror = null as any; peer.onconnectionstatechange = null as any; peer.oniceconnectionstatechange = null as any; peer.onnegotiationneeded = null as any; peer.onicegatheringstatechange = null as any; peer.onsignalingstatechange = null as any; + peer.ondatachannel = null as any;
📜 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.
📒 Files selected for processing (3)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts(2 hunks)packages/media-signaling/src/lib/Call.ts(8 hunks)packages/media-signaling/src/lib/services/webrtc/Processor.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/media-signaling/src/lib/Call.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
packages/media-signaling/src/lib/Call.ts (1)
muted(112-118)
🔇 Additional comments (1)
packages/media-signaling/src/definition/services/webrtc/IWebRTCProcessor.ts (1)
13-48: Remote mute accessor looks solidAdding the
remoteMutestate andisRemoteMute()accessor keeps the interface aligned with the processor changes. 👍
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
294-296: Add stopped state check for consistency.Unlike
isRemoteHeld()(lines 268-270), this method doesn't check the stopped state. For consistency with the existing API, consider returningfalsewhen the processor has been stopped.Apply this diff:
public isRemoteMute(): boolean { + if (this.stopped) { + return false; + } return this._remoteMute; }
📜 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.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
packages/media-signaling/src/lib/Call.ts (1)
muted(112-118)
⏰ 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 (9)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (9)
9-10: LGTM: Well-defined constants.The data channel label and P2P command types are clear and properly scoped.
65-69: LGTM: Proper field declarations.The new fields for data channel state tracking are correctly typed and follow the existing naming conventions.
132-132: LGTM: Appropriate placement.Creating the data channel before negotiation ensures it's included in the offer when the flag is present.
149-149: LGTM: Proper mute synchronization.Updating the remote peer's mute state after changing the local state maintains consistency.
166-166: LGTM: Clean shutdown signal.Ending the data channel at the beginning of
stop()properly notifies the remote peer before teardown.
441-444: LGTM: Safe end sequence.Setting
_dataChannelEndedbefore sending the command ensures the flag is set even if sending fails.
446-487: LGTM: Comprehensive data channel initialization.The initialization properly handles:
- Label validation to reject unexpected channels
- Smart assignment logic that avoids overwriting open channels
- Channel recreation on close (when appropriate)
- Message validation and parsing
The stopped state concern in the
onclosehandler is already covered in the earlier review comment.
489-548: LGTM: Robust P2P command implementation.The command handling demonstrates good practices:
- sendP2PCommand: Guards against invalid channel states and returns success status
- getCommandFromDataChannelMessage: Properly validates and parses with error handling
- onP2PCommand: Clean command routing with appropriate state updates
- setRemoteMute: Prevents redundant state changes and properly emits events
- updateMuteForRemote: Concise synchronization of local mute state to remote peer
723-726: LGTM: Clean event handler delegation.The handler properly delegates to
initializeDataChannel(), which performs all necessary validation and setup.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
433-441: Addthis.stoppedguard for consistency and defensive programming.As flagged in a previous review, the guard at line 434 should check
this.stoppedto prevent callingthis.peer.createDataChannel()on a closedRTCPeerConnection. While_dataChannelEndedprovides technical protection, the class establishes a pattern elsewhere of checkingthis.stoppedbefore peer operations for clarity and consistency. Based on learningsApply this diff:
private createDataChannel(): void { - if (this._dataChannel || this._dataChannelEnded || !this.config.call.flags.includes('create-data-channel')) { + if (this.stopped || this._dataChannel || this._dataChannelEnded || !this.config.call.flags.includes('create-data-channel')) { return; }
📜 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.
📒 Files selected for processing (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (1)
packages/media-signaling/src/lib/utils/getExternalWaiter.ts (1)
PromiseWaiterData(1-7)
⏰ 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 (12)
packages/media-signaling/src/lib/services/webrtc/Processor.ts (12)
9-10: LGTM!The data channel label and command type definitions are clear and appropriate for P2P mute synchronization.
65-69: LGTM!The data channel state variables are properly typed and initialized.
132-132: LGTM!Calling
createDataChannelincreateOfferis the appropriate place to establish the data channel during negotiation.
149-149: LGTM!Notifying the remote peer of mute state changes via
updateMuteForRemoteproperly synchronizes the mute state across clients.
254-256: LGTM!The
remoteMutecase correctly returns the internal_remoteMutestate for monitoring.
296-298: LGTM!The public
isRemoteMutemethod appropriately exposes the remote mute state as per VGA-48 requirements.
443-446: LGTM!Setting
_dataChannelEndedand sending the 'end' command properly signals teardown to the remote peer.
491-504: LGTM!The
sendP2PCommandmethod correctly guards against sending on a closed or non-existent channel and returns a boolean to indicate success.
506-521: LGTM!The command validation and parsing logic is robust, with proper error handling for malformed messages.
523-545: LGTM!The command routing and remote mute state management correctly handle mute/unmute/end commands, with proper change detection to avoid unnecessary event emissions.
547-550: LGTM!The
updateMuteForRemotemethod correctly synchronizes the local mute state with the remote peer via the data channel.
726-729: LGTM!The
onDataChannelhandler appropriately delegates toinitializeDataChannelfor inbound data channel setup.
tassoevan
merged commit b4dc8ba
into
feat/voip-sync-audio-direction
Proposed changes (including videos or screenshots)
Issue(s)
VGA-47
VGA-48
Steps to test or reproduce
Further comments
Added a new
flagsattribute to the call initialization signal, which the server can use to instruct the client to execute certain actions or not. I'm using this to let the server determine which client may create data channels and when.Summary by CodeRabbit
New Features
Refactor