Add WebRTC peer connection and signaling server implementation#1532
Open
gowthamkishore3799 wants to merge 1 commit into
Open
Add WebRTC peer connection and signaling server implementation#1532gowthamkishore3799 wants to merge 1 commit into
gowthamkishore3799 wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds two new WebRTC infrastructure modules: ChangesWebRTC Peer and Signaling Implementation
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
📝 WalkthroughWalkthroughThis PR introduces two new WebRTC modules: a ChangesWebRTC Peer-to-Peer Communication System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitaidev3[bot]](https://avatars.githubusercontent.com/u/132028505?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
webrtc_signaling_server.js (1)
54-69: Consider adding a default case for debugging.Unknown message types are silently ignored. For operational visibility, log unrecognized types at debug level.
💡 Optional: Add default case
case "leave": this.handleLeave(clientId, message.room); break; - // BUG: no default case — unknown message types silently ignored + default: + console.warn(`Unknown message type from ${clientId}: ${message.type}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webrtc_signaling_server.js` around lines 54 - 69, The switch in handleMessage currently ignores unknown message.type values; add a default case in handleMessage that logs unrecognized message types (include clientId and message.type and optionally message.room/payload) at debug level so operators can observe unexpected inputs; locate the handleMessage method and add the default branch after the existing cases to call your logger (or this.relayMessage for known types) rather than silently dropping messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webrtc_peer.js`:
- Around line 201-206: The sendSignalingMessage method currently opens a fresh
WebSocket and sends immediately (creating leaks and race conditions); update
sendSignalingMessage to reuse the persistent socket created by connectSignaling
(this.signalingSocket) instead of new WebSocket(), check its readyState
(WebSocket.OPEN) before calling send, and if it’s not open either queue the
message until the socket emits open or return/reject an error; remove creation
of temporary sockets to avoid resource leaks and ensure all signaling uses
this.signalingSocket.
- Around line 38-44: The onicecandidate handler is wrongly pushing local
candidates into this.iceCandidatesQueue instead of sending them to the remote
peer; update the handler (this.peerConnection.onicecandidate) to transmit local
candidates over the existing signaling channel (instead of push), and reserve
this.iceCandidatesQueue for buffering incoming remote candidates; change the
incoming-candidate handling (where remote ICEs are received) to push remote
candidates into this.iceCandidatesQueue only if
this.peerConnection.remoteDescription is not yet set, otherwise call
this.peerConnection.addIceCandidate() immediately; in handleAnswer() / after
calling setRemoteDescription() drain this.iceCandidatesQueue by calling
addIceCandidate() for each buffered remote candidate once setRemoteDescription()
completes. Ensure addIceCandidate() calls are awaited/handled for errors.
- Around line 286-293: The close() method currently only calls
this.peerConnection.close(); update it to fully tear down resources: stop all
local tracks via this.localStream?.getTracks().forEach(t => t.stop()), stop
remote tracks via this.remoteStream?.getTracks().forEach(t => t.stop()), close
the data channel with this.dataChannel?.close(), close the signaling socket with
this.signalingSocket?.close(), then call this.peerConnection?.close() and set
this.localStream, this.remoteStream, this.dataChannel, this.signalingSocket, and
this.peerConnection to null to remove references and avoid leaks.
- Around line 21-33: The ICE configuration is hardcoded with invalid servers,
plaintext TURN credentials and a forced relay-only policy; change the code to
accept ICE server details from outside (e.g., add an optional constructor
parameter or factory arg that supplies iceServers and iceTransportPolicy)
instead of using the local config constant, remove any hardcoded
username/credential values from the source (do not initialize iceServers with
"admin"/"password123"), and only set iceTransportPolicy to "relay" when
explicitly provided by the caller; update references to config, iceServers and
iceTransportPolicy (and the constructor/factory that builds the
RTCPeerConnection) to use the injected configuration.
- Around line 115-133: In handleOffer, construct the RTCSessionDescriptionInit
correctly from the incoming offerData (ensure you extract the sdp string, not
pass the whole object), await this.peerConnection.setRemoteDescription(offer)
before creating an answer, call await
this.peerConnection.setLocalDescription(answer) (not setRemoteDescription) after
createAnswer, and normalize the signaling payload in sendSignalingMessage to
send a plain serializable object like { type: "answer", sdp: answer.sdp } so
downstream handleAnswer can recreate an RTCSessionDescriptionInit from the sdp
string.
- Around line 46-50: The ontrack handler recreates this.remoteStream for every
track which drops previously received tracks; update the
this.peerConnection.ontrack callback to use event.streams[0] when available
(assign this.remoteStream = event.streams[0]) otherwise ensure you only create a
new MediaStream once (e.g., create if this.remoteStream is falsy) and then call
this.remoteStream.addTrack(event.track); this preserves existing tracks and uses
the browser-provided stream when present.
- Around line 65-85: getLocalMedia is using overconstrained defaults and blindly
calling addTrack on this.peerConnection which may be uninitialized; change
defaultConstraints in getLocalMedia to use realistic, non-exact/overconstraining
fields (e.g., use ideal or max for width/height/frameRate with lower values like
1280x720 or 1920x1080 and a reasonable frameRate) and restore sensible audio
processing (enable echoCancellation, noiseSuppression, autoGainControl or omit
them so user agent defaults apply). Before adding tracks, guard the addTrack
loop by ensuring this.peerConnection exists (call initialize() or throw/return a
clear error if it’s not initialized) so the code that iterates
this.localStream.getTracks() only runs when this.peerConnection is available.
- Around line 255-265: Filter peerConnection.getSenders() to only target the
video RTCRtpSender (check sender.track?.kind === 'video' or sender.kind ===
'video') before calling replaceTrack(screenTrack), store the original camera
track in a variable (e.g., originalVideoTrack) prior to replacing it, and in
screenTrack.onended call replaceTrack(originalVideoTrack) on that same video
sender (handling the returned Promise and logging errors) so the camera feed is
restored when screen sharing stops.
In `@webrtc_signaling_server.js`:
- Around line 146-153: getRoomInfo currently assumes this.rooms[roomId] exists,
so accessing this.rooms[roomId].length throws when a room is missing; update
getRoomInfo to safely derive peers (e.g., const peers = this.rooms[roomId] ||
[]) and use that peers variable to populate the returned peers and count fields
so missing rooms return an empty array and count 0 instead of throwing.
- Around line 135-140: The broadcast method currently calls ws.send on every
entry in this.clients and will throw if a socket is closed; update broadcast
(the broadcast function that iterates this.clients) to skip or remove
disconnected sockets by first checking each ws.readyState === WebSocket.OPEN (or
equivalent ready state constant) before calling ws.send, and wrap sends in a
try/catch to log and continue on errors; also remove or cleanup clients whose
sockets are CLOSED to avoid future failures.
- Around line 106-114: When forwarding messages to targetWs in the signaling
handler, ensure you check targetWs.readyState === WebSocket.OPEN before calling
targetWs.send; if the socket is not OPEN (CLOSING or CLOSED) drop the send (and
optionally remove the stale socket from your peers map and log the event) to
avoid send() throwing. Locate the code that references targetWs, senderId, and
message and add the readyState guard using WebSocket.OPEN and appropriate
cleanup/logging when the socket is not open.
- Around line 156-160: The module currently creates and starts a SignalingServer
instance on import (const server = new SignalingServer(8080); server.start()),
which prevents testing and hardcodes the port; remove this side-effect by moving
instantiation/startup into a separate entrypoint or guard it with require.main
=== module so the module only exports the SignalingServer class, and
create/start the server (calling new SignalingServer(...) and server.start())
only when the file is executed directly; update references to the exported
symbol (SignalingServer) and remove the top-level server/start calls.
- Around line 118-130: In handleLeave, avoid using indexOf+splice which can
remove the wrong element when indexOf returns -1 and only removes the first
occurrence; instead remove all occurrences of clientId from this.rooms[roomId]
(e.g., by filtering the array or removing while index >= 0) and only perform
splice when a valid index is found; after removal, if this.rooms[roomId] is
empty delete the room key to avoid lingering empty rooms. Ensure you update the
array stored at this.rooms[roomId] (or reassign the filtered result) and
reference handleLeave and this.rooms[roomId] when applying the fix.
- Around line 29-44: Wrap the ws.on("message") handler to validate/sanitize
incoming data and protect JSON.parse with a try-catch: catch parse errors, log
and ignore malformed messages instead of letting exceptions bubble; after
successful parse call this.handleMessage(clientId, ws, message). Add an
ws.on("error") handler to log socket errors and prevent crashes. In the
ws.on("close") handler, after this.clients.delete(clientId) also remove the
clientId from any room membership in this.rooms (iterate affected rooms or use
the room management helper if present) and notify remaining peers safely (skip
missing peer IDs) to avoid NPEs when relaying messages.
- Around line 90-99: When notifying existing peers in this.rooms[roomId] about a
new joiner, guard against undefined or closed sockets and avoid notifying the
joining client itself: iterate this.rooms[roomId], skip entries where peerId ===
clientId, get peerWs via this.clients.get(peerId) and skip if peerWs is falsy or
peerWs.readyState is not WebSocket.OPEN (or equivalent ready-state constant),
and consider removing stale peerId entries from this.rooms[roomId] when peerWs
is missing to prevent future null derefs; update the notification logic around
peerWs.send(...) in the join-handling code accordingly.
- Line 24: Replace the weak, predictable client ID generation that uses
Math.random() (the clientId assignment) with a cryptographically secure
generator; update the clientId creation to use Node's crypto (e.g.,
crypto.randomUUID() or crypto.randomBytes converted to a URL-safe string) so IDs
are unpredictable and collision-resistant, and ensure any consumers expecting
the old format continue to work with the new UUID/secure string.
---
Nitpick comments:
In `@webrtc_signaling_server.js`:
- Around line 54-69: The switch in handleMessage currently ignores unknown
message.type values; add a default case in handleMessage that logs unrecognized
message types (include clientId and message.type and optionally
message.room/payload) at debug level so operators can observe unexpected inputs;
locate the handleMessage method and add the default branch after the existing
cases to call your logger (or this.relayMessage for known types) rather than
silently dropping messages.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b3de2c39-70d8-4739-aede-598ad56d054f
📒 Files selected for processing (2)
webrtc_peer.jswebrtc_signaling_server.js
Comment on lines
+21
to
+33
| // BUG: Using outdated/non-existent STUN/TURN servers | ||
| const config = { | ||
| iceServers: [ | ||
| { urls: "stun:stun.invalid-server.example:19302" }, | ||
| { | ||
| urls: "turn:turn.invalid-server.example:3478", | ||
| username: "admin", | ||
| credential: "password123", // BUG: hardcoded credentials in source | ||
| }, | ||
| ], | ||
| // BUG: using deprecated constraint | ||
| iceTransportPolicy: "relay", // Forces TURN only, but TURN server is invalid | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "webrtc_peer.js" -type fRepository: gowthamkishore3799/coderabbit-test
Length of output: 95
🏁 Script executed:
cat -n webrtc_peer.js | head -50Repository: gowthamkishore3799/coderabbit-test
Length of output: 2029
Externalize the ICE server configuration instead of hardcoding invalid servers and forcing relay-only mode.
With iceTransportPolicy: "relay" enforced against turn.invalid-server.example, peer connections will fail to establish any viable ICE candidate path. The hardcoded TURN credentials (username: "admin", credential: "password123") are also exposed as plaintext in the browser bundle.
Accept ICE server configuration via constructor parameter to allow dynamic credential injection:
Suggested fix
class WebRTCPeer {
- constructor(signalingServerUrl) {
+ constructor(signalingServerUrl, rtcConfig = {}) {
this.signalingServerUrl = signalingServerUrl;
+ this.rtcConfig = rtcConfig;
this.peerConnection = null;
@@
async initialize() {
- const config = {
- iceServers: [
- { urls: "stun:stun.invalid-server.example:19302" },
- {
- urls: "turn:turn.invalid-server.example:3478",
- username: "admin",
- credential: "password123",
- },
- ],
- iceTransportPolicy: "relay",
- };
+ const config = {
+ iceServers: this.rtcConfig.iceServers ?? [],
+ iceTransportPolicy: this.rtcConfig.iceTransportPolicy ?? "all",
+ };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 21 - 33, The ICE configuration is hardcoded with
invalid servers, plaintext TURN credentials and a forced relay-only policy;
change the code to accept ICE server details from outside (e.g., add an optional
constructor parameter or factory arg that supplies iceServers and
iceTransportPolicy) instead of using the local config constant, remove any
hardcoded username/credential values from the source (do not initialize
iceServers with "admin"/"password123"), and only set iceTransportPolicy to
"relay" when explicitly provided by the caller; update references to config,
iceServers and iceTransportPolicy (and the constructor/factory that builds the
RTCPeerConnection) to use the injected configuration.
Comment on lines
+38
to
+44
| this.peerConnection.onicecandidate = (event) => { | ||
| if (event.candidate) { | ||
| // BUG: pushing to local queue instead of sending to remote peer | ||
| this.iceCandidatesQueue.push(event.candidate); | ||
| console.log("ICE candidate queued locally (never sent)"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -name "webrtc_peer.js" -o -name "*webrtc*" -type f | head -20Repository: gowthamkishore3799/coderabbit-test
Length of output: 309
🏁 Script executed:
fd webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 93
🏁 Script executed:
cat -n webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 11270
Local ICE candidates are queued instead of sent to remote peer, breaking trickle ICE.
Your onicecandidate handler (lines 38-44) queues local candidates without sending them via signaling. Then handleAnswer() (lines 145-150) incorrectly applies those local candidates back through addIceCandidate() as if they came from the remote peer. Meanwhile, real remote candidates arriving before remoteDescription is set (line 156-162) will fail because addIceCandidate() doesn't check the connection state. This breaks ICE gathering and exchange entirely.
Send local candidates through your signaling channel, buffer incoming remote candidates in iceCandidatesQueue only if remoteDescription is not yet set, and drain the queue after setRemoteDescription() completes.
Suggested fix
this.peerConnection.onicecandidate = (event) => {
if (event.candidate) {
- this.iceCandidatesQueue.push(event.candidate);
- console.log("ICE candidate queued locally (never sent)");
+ this.sendSignalingMessage({
+ type: "ice-candidate",
+ candidate: event.candidate.toJSON(),
+ });
}
};
@@
async addIceCandidate(candidateData) {
const candidate = new RTCIceCandidate(candidateData);
-
+ if (!this.peerConnection.remoteDescription) {
+ this.iceCandidatesQueue.push(candidate);
+ return;
+ }
await this.peerConnection.addIceCandidate(candidate);
}
@@
async handleAnswer(answerData) {
const answer = new RTCSessionDescription(answerData);
await this.peerConnection.setRemoteDescription(answer);
this.iceCandidatesQueue.forEach((candidate) => {
- this.peerConnection.addIceCandidate(candidate);
+ await this.peerConnection.addIceCandidate(candidate);
});
this.iceCandidatesQueue = [];
}Also applies to: 145-150, 156-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 38 - 44, The onicecandidate handler is wrongly
pushing local candidates into this.iceCandidatesQueue instead of sending them to
the remote peer; update the handler (this.peerConnection.onicecandidate) to
transmit local candidates over the existing signaling channel (instead of push),
and reserve this.iceCandidatesQueue for buffering incoming remote candidates;
change the incoming-candidate handling (where remote ICEs are received) to push
remote candidates into this.iceCandidatesQueue only if
this.peerConnection.remoteDescription is not yet set, otherwise call
this.peerConnection.addIceCandidate() immediately; in handleAnswer() / after
calling setRemoteDescription() drain this.iceCandidatesQueue by calling
addIceCandidate() for each buffered remote candidate once setRemoteDescription()
completes. Ensure addIceCandidate() calls are awaited/handled for errors.
Comment on lines
+46
to
+50
| this.peerConnection.ontrack = (event) => { | ||
| // BUG: overwriting the stream on every track instead of adding tracks | ||
| this.remoteStream = new MediaStream(); | ||
| this.remoteStream.addTrack(event.track); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "webrtc_peer.js" --exec cat -n {} +Repository: gowthamkishore3799/coderabbit-test
Length of output: 11322
🏁 Script executed:
cat > /tmp/verify_webrtc.md << 'EOF'
# WebRTC ontrack event analysis
## Current Code Issue
- recreates `remoteStream` on EACH track event
- with separate audio/video tracks (2 events), 1st track is lost
- severity: CRITICAL (audio or video missing from stream)
## Suggested Fix Verification
The suggested fix uses:
1. `event.streams[0]` - the MediaStream provided by browser
2. Fallback to new MediaStream() if streams[0] is undefined
3. Only add track if it came without a stream
## Key Questions
- Is event.streams[0] always available in modern browsers?
- Is the suggested fix idiomatic for WebRTC?
EOF
cat /tmp/verify_webrtc.mdRepository: gowthamkishore3799/coderabbit-test
Length of output: 646
🏁 Script executed:
rg "remoteStream" webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 200
🌐 Web query:
WebRTC RTCTrackEvent event.streams standard behavior ontrack handler
💡 Result:
In the WebRTC 1.0 spec, pc.ontrack = (event) => { ... } receives an RTCTrackEvent whose event.streams is the list of remote MediaStream objects the UA associated with that incoming track (i.e., the track is added to those streams). The track event is fired with RTCTrackEvent.streams initialized from the internal trackEventInits data computed during setRemoteDescription processing. [1]
When should event.streams be non-empty?
It’s typically non-empty when the sender associated the outbound track with one or more streams—most commonly by calling pc.addTrack(track, stream) (or legacy addStream). The sender-side “streams” list is used to populate the sender’s associated MediaStream IDs (signaled via SDP/MSID), which the receiver uses to create/choose corresponding remote MediaStream objects and then expose them via event.streams. [1][2][3]
When can event.streams be empty (and that’s still standards-consistent)?
event.streams can legitimately be [] when the incoming track is not associated with any MediaStream (e.g., the sender used addTrack(track) without providing any stream arguments, or used a transceiver/workflow that results in no stream association). In that case, you’re expected to use event.track directly (often by creating your own MediaStream and adding the track). MDN explicitly defines streams as “an array of MediaStream objects… for each of the streams that comprise the track being added,” which may be empty if there are no associated streams. [3][4]
Practical ontrack pattern
Most examples do:
- use
event.streams[0]if present - otherwise construct a stream from the track
This is also the pattern shown in common documentation/examples for the track event. [4]
Sources: W3C WebRTC 1.0 spec (track event + stream association), webrtc.org remote streams guide, MDN RTCTrackEvent.streams, MDN RTCPeerConnection track event. [1][2][3][4]
Do not recreate remoteStream on every track event.
A remote peer fires one track event per audio/video track. Reinitializing this.remoteStream here discards the previous stream, so the consumer receives only the last track. Use the stream provided by the browser via event.streams[0], or create one only if the track arrived unassociated.
Suggested fix
this.peerConnection.ontrack = (event) => {
- this.remoteStream = new MediaStream();
- this.remoteStream.addTrack(event.track);
+ this.remoteStream ??= event.streams[0] ?? new MediaStream();
+ if (!event.streams[0]) {
+ this.remoteStream.addTrack(event.track);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.peerConnection.ontrack = (event) => { | |
| // BUG: overwriting the stream on every track instead of adding tracks | |
| this.remoteStream = new MediaStream(); | |
| this.remoteStream.addTrack(event.track); | |
| }; | |
| this.peerConnection.ontrack = (event) => { | |
| this.remoteStream ??= event.streams[0] ?? new MediaStream(); | |
| if (!event.streams[0]) { | |
| this.remoteStream.addTrack(event.track); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 46 - 50, The ontrack handler recreates
this.remoteStream for every track which drops previously received tracks; update
the this.peerConnection.ontrack callback to use event.streams[0] when available
(assign this.remoteStream = event.streams[0]) otherwise ensure you only create a
new MediaStream once (e.g., create if this.remoteStream is falsy) and then call
this.remoteStream.addTrack(event.track); this preserves existing tracks and uses
the browser-provided stream when present.
Comment on lines
+65
to
+85
| const defaultConstraints = { | ||
| video: { | ||
| width: { exact: 7680 }, | ||
| height: { exact: 4320 }, | ||
| frameRate: { ideal: 120 }, | ||
| }, | ||
| audio: { | ||
| echoCancellation: false, // BUG: echo cancellation disabled | ||
| noiseSuppression: false, // BUG: noise suppression disabled | ||
| autoGainControl: false, // BUG: auto gain control disabled | ||
| }, | ||
| }; | ||
|
|
||
| this.localStream = await navigator.mediaDevices.getUserMedia( | ||
| constraints || defaultConstraints | ||
| ); | ||
|
|
||
| // BUG: adding tracks but never checking if peerConnection exists | ||
| this.localStream.getTracks().forEach((track) => { | ||
| this.peerConnection.addTrack(track, this.localStream); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "webrtc_peer.js" -o -name "*webrtc*" -type f | head -20Repository: gowthamkishore3799/coderabbit-test
Length of output: 124
🏁 Script executed:
fd -i "webrtc" --type fRepository: gowthamkishore3799/coderabbit-test
Length of output: 120
🏁 Script executed:
cat -n webrtc_peer.js | head -100Repository: gowthamkishore3799/coderabbit-test
Length of output: 3883
🏁 Script executed:
cat -n webrtc_peer.js | tail -50Repository: gowthamkishore3799/coderabbit-test
Length of output: 1628
🏁 Script executed:
cat -n webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 11270
🏁 Script executed:
fd -i "test\|example\|spec" --type f | head -20Repository: gowthamkishore3799/coderabbit-test
Length of output: 60
🏁 Script executed:
rg "getLocalMedia|new WebRTCPeer" --type jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 130
🏁 Script executed:
find . -name "*.js" -type fRepository: gowthamkishore3799/coderabbit-test
Length of output: 456
🏁 Script executed:
cat -n webrtc_signaling_server.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 5702
Guard getLocalMedia() against an uninitialized peer and overconstrained defaults.
addTrack() on Line 84 will throw if initialize() was not called first, and exact: 7680x4320 will reject on most cameras before media setup ever succeeds.
Suggested fix
async getLocalMedia(constraints) {
+ if (!this.peerConnection) {
+ throw new Error("Call initialize() before getLocalMedia()");
+ }
+
const defaultConstraints = {
video: {
- width: { exact: 7680 },
- height: { exact: 4320 },
- frameRate: { ideal: 120 },
+ width: { ideal: 1280 },
+ height: { ideal: 720 },
+ frameRate: { ideal: 30 },
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 65 - 85, getLocalMedia is using overconstrained
defaults and blindly calling addTrack on this.peerConnection which may be
uninitialized; change defaultConstraints in getLocalMedia to use realistic,
non-exact/overconstraining fields (e.g., use ideal or max for
width/height/frameRate with lower values like 1280x720 or 1920x1080 and a
reasonable frameRate) and restore sensible audio processing (enable
echoCancellation, noiseSuppression, autoGainControl or omit them so user agent
defaults apply). Before adding tracks, guard the addTrack loop by ensuring
this.peerConnection exists (call initialize() or throw/return a clear error if
it’s not initialized) so the code that iterates this.localStream.getTracks()
only runs when this.peerConnection is available.
Comment on lines
+115
to
+133
| async handleOffer(offerData) { | ||
| // BUG: constructing RTCSessionDescription incorrectly | ||
| const offer = new RTCSessionDescription({ | ||
| type: "offer", | ||
| sdp: offerData, // BUG: offerData might be an object, not a string | ||
| }); | ||
|
|
||
| // BUG: not awaiting setRemoteDescription | ||
| this.peerConnection.setRemoteDescription(offer); | ||
|
|
||
| const answer = await this.peerConnection.createAnswer(); | ||
|
|
||
| // BUG: setting remote description AGAIN instead of local description | ||
| await this.peerConnection.setRemoteDescription(answer); | ||
|
|
||
| this.sendSignalingMessage({ | ||
| type: "answer", | ||
| sdp: answer, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "webrtc_peer.js" -o -name "*webrtc*peer*"Repository: gowthamkishore3799/coderabbit-test
Length of output: 95
🏁 Script executed:
cat -n webrtc_peer.js | head -200Repository: gowthamkishore3799/coderabbit-test
Length of output: 7569
🏁 Script executed:
cat -n webrtc_peer.js | sed -n '200,250p'Repository: gowthamkishore3799/coderabbit-test
Length of output: 1876
Fix critical bugs in answer negotiation flow: add missing await, use correct description setter, and normalize signaling format.
Three critical issues prevent proper answer negotiation:
-
Line 123: Missing
awaitonsetRemoteDescription(offer)means the remote offer may not be fully processed before creating the answer. -
Line 128: Calling
setRemoteDescription(answer)instead ofsetLocalDescription(answer)leaves the local peer without an established local answer, breaking the connection. -
Lines 130-133 and 142-143: The answer is sent as
{ type: "answer", sdp: answer }whereansweris anRTCSessionDescriptionobject (not a string). Line 230 then passes the entire message object tohandleAnswer(), which tries to constructRTCSessionDescription(message)on line 142. This fails becausemessageis not a validRTCSessionDescriptionInit.
Suggested fix
async handleOffer(offerData) {
const offer = new RTCSessionDescription({
type: "offer",
sdp: offerData,
});
- this.peerConnection.setRemoteDescription(offer);
+ await this.peerConnection.setRemoteDescription(offer);
const answer = await this.peerConnection.createAnswer();
- await this.peerConnection.setRemoteDescription(answer);
+ await this.peerConnection.setLocalDescription(answer);
this.sendSignalingMessage({
type: "answer",
- sdp: answer,
+ sdp: answer.sdp,
});
}
@@
if (message.type === "answer") {
- this.handleAnswer(message);
+ this.handleAnswer(message.sdp);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 115 - 133, In handleOffer, construct the
RTCSessionDescriptionInit correctly from the incoming offerData (ensure you
extract the sdp string, not pass the whole object), await
this.peerConnection.setRemoteDescription(offer) before creating an answer, call
await this.peerConnection.setLocalDescription(answer) (not setRemoteDescription)
after createAnswer, and normalize the signaling payload in sendSignalingMessage
to send a plain serializable object like { type: "answer", sdp: answer.sdp } so
downstream handleAnswer can recreate an RTCSessionDescriptionInit from the sdp
string.
Comment on lines
+106
to
+114
| if (targetWs) { | ||
| // BUG: not checking WebSocket readyState before sending | ||
| targetWs.send( | ||
| JSON.stringify({ | ||
| ...message, | ||
| from: senderId, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Check WebSocket readyState before sending.
The socket may exist in the map but be in CLOSING/CLOSED state, causing send() to throw.
🐛 Proposed fix
if (targetWs) {
- // BUG: not checking WebSocket readyState before sending
- targetWs.send(
- JSON.stringify({
- ...message,
- from: senderId,
- })
- );
+ if (targetWs.readyState === WebSocket.OPEN) {
+ targetWs.send(
+ JSON.stringify({
+ ...message,
+ from: senderId,
+ })
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (targetWs) { | |
| // BUG: not checking WebSocket readyState before sending | |
| targetWs.send( | |
| JSON.stringify({ | |
| ...message, | |
| from: senderId, | |
| }) | |
| ); | |
| } | |
| if (targetWs) { | |
| if (targetWs.readyState === WebSocket.OPEN) { | |
| targetWs.send( | |
| JSON.stringify({ | |
| ...message, | |
| from: senderId, | |
| }) | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 106 - 114, When forwarding messages
to targetWs in the signaling handler, ensure you check targetWs.readyState ===
WebSocket.OPEN before calling targetWs.send; if the socket is not OPEN (CLOSING
or CLOSED) drop the send (and optionally remove the stale socket from your peers
map and log the event) to avoid send() throwing. Locate the code that references
targetWs, senderId, and message and add the readyState guard using
WebSocket.OPEN and appropriate cleanup/logging when the socket is not open.
Comment on lines
+118
to
+130
| handleLeave(clientId, roomId) { | ||
| if (this.rooms[roomId]) { | ||
| // BUG: uses indexOf + splice which only removes first occurrence | ||
| // If client somehow joined twice, second entry remains | ||
| const index = this.rooms[roomId].indexOf(clientId); | ||
| this.rooms[roomId].splice(index, 1); | ||
|
|
||
| // BUG: not checking if index is -1 before splicing | ||
| // splice(-1, 1) removes the LAST element | ||
|
|
||
| // BUG: empty rooms are never deleted from the rooms object | ||
| } | ||
| } |
There was a problem hiding this comment.
splice(-1, 1) removes the wrong element when client not found.
If indexOf returns -1 (client not in room), splice(-1, 1) removes the last peer in the array—corrupting room state.
🐛 Proposed fix
handleLeave(clientId, roomId) {
if (this.rooms[roomId]) {
- // BUG: uses indexOf + splice which only removes first occurrence
- // If client somehow joined twice, second entry remains
const index = this.rooms[roomId].indexOf(clientId);
- this.rooms[roomId].splice(index, 1);
-
- // BUG: not checking if index is -1 before splicing
- // splice(-1, 1) removes the LAST element
-
- // BUG: empty rooms are never deleted from the rooms object
+ if (index !== -1) {
+ this.rooms[roomId].splice(index, 1);
+ if (this.rooms[roomId].length === 0) {
+ delete this.rooms[roomId];
+ }
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handleLeave(clientId, roomId) { | |
| if (this.rooms[roomId]) { | |
| // BUG: uses indexOf + splice which only removes first occurrence | |
| // If client somehow joined twice, second entry remains | |
| const index = this.rooms[roomId].indexOf(clientId); | |
| this.rooms[roomId].splice(index, 1); | |
| // BUG: not checking if index is -1 before splicing | |
| // splice(-1, 1) removes the LAST element | |
| // BUG: empty rooms are never deleted from the rooms object | |
| } | |
| } | |
| handleLeave(clientId, roomId) { | |
| if (this.rooms[roomId]) { | |
| const index = this.rooms[roomId].indexOf(clientId); | |
| if (index !== -1) { | |
| this.rooms[roomId].splice(index, 1); | |
| if (this.rooms[roomId].length === 0) { | |
| delete this.rooms[roomId]; | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 118 - 130, In handleLeave, avoid
using indexOf+splice which can remove the wrong element when indexOf returns -1
and only removes the first occurrence; instead remove all occurrences of
clientId from this.rooms[roomId] (e.g., by filtering the array or removing while
index >= 0) and only perform splice when a valid index is found; after removal,
if this.rooms[roomId] is empty delete the room key to avoid lingering empty
rooms. Ensure you update the array stored at this.rooms[roomId] (or reassign the
filtered result) and reference handleLeave and this.rooms[roomId] when applying
the fix.
Comment on lines
+135
to
+140
| broadcast(message) { | ||
| // BUG: iterating over Map values but not checking connection state | ||
| this.clients.forEach((ws, clientId) => { | ||
| // BUG: this will throw if any client has disconnected | ||
| ws.send(JSON.stringify(message)); | ||
| }); |
There was a problem hiding this comment.
Broadcast throws on closed sockets, aborting remaining sends.
If any client has disconnected, ws.send() throws and stops the loop, preventing delivery to remaining clients.
🐛 Proposed fix
broadcast(message) {
- // BUG: iterating over Map values but not checking connection state
this.clients.forEach((ws, clientId) => {
- // BUG: this will throw if any client has disconnected
- ws.send(JSON.stringify(message));
+ if (ws.readyState === WebSocket.OPEN) {
+ ws.send(JSON.stringify(message));
+ }
});
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 135 - 140, The broadcast method
currently calls ws.send on every entry in this.clients and will throw if a
socket is closed; update broadcast (the broadcast function that iterates
this.clients) to skip or remove disconnected sockets by first checking each
ws.readyState === WebSocket.OPEN (or equivalent ready state constant) before
calling ws.send, and wrap sends in a try/catch to log and continue on errors;
also remove or cleanup clients whose sockets are CLOSED to avoid future
failures.
Comment on lines
+146
to
+153
| getRoomInfo(roomId) { | ||
| return { | ||
| room: roomId, | ||
| // BUG: returns undefined if room doesn't exist, not an empty array | ||
| peers: this.rooms[roomId], | ||
| count: this.rooms[roomId].length, // BUG: will throw if room is undefined | ||
| }; | ||
| } |
There was a problem hiding this comment.
TypeError when room does not exist.
this.rooms[roomId].length throws if the room is undefined.
🐛 Proposed fix
getRoomInfo(roomId) {
+ const peers = this.rooms[roomId] || [];
return {
room: roomId,
- // BUG: returns undefined if room doesn't exist, not an empty array
- peers: this.rooms[roomId],
- count: this.rooms[roomId].length, // BUG: will throw if room is undefined
+ peers: peers,
+ count: peers.length,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| getRoomInfo(roomId) { | |
| return { | |
| room: roomId, | |
| // BUG: returns undefined if room doesn't exist, not an empty array | |
| peers: this.rooms[roomId], | |
| count: this.rooms[roomId].length, // BUG: will throw if room is undefined | |
| }; | |
| } | |
| getRoomInfo(roomId) { | |
| const peers = this.rooms[roomId] || []; | |
| return { | |
| room: roomId, | |
| peers: peers, | |
| count: peers.length, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 146 - 153, getRoomInfo currently
assumes this.rooms[roomId] exists, so accessing this.rooms[roomId].length throws
when a room is missing; update getRoomInfo to safely derive peers (e.g., const
peers = this.rooms[roomId] || []) and use that peers variable to populate the
returned peers and count fields so missing rooms return an empty array and count
0 instead of throwing.
Comment on lines
+156
to
+160
| // BUG: starts server immediately on require/import with hardcoded port | ||
| const server = new SignalingServer(8080); | ||
| server.start(); | ||
|
|
||
| module.exports = SignalingServer; |
There was a problem hiding this comment.
Server instantiates on import—prevents testing and configuration.
Instantiating and starting the server as a module side effect blocks unit testing and forces a hardcoded port. Move instantiation to a separate entrypoint or guard with require.main === module.
🛠️ Proposed fix
-// BUG: starts server immediately on require/import with hardcoded port
-const server = new SignalingServer(8080);
-server.start();
+// Only start when run directly, not when imported
+if (require.main === module) {
+ const port = process.env.PORT || 8080;
+ const server = new SignalingServer(port);
+ server.start();
+}
module.exports = SignalingServer;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // BUG: starts server immediately on require/import with hardcoded port | |
| const server = new SignalingServer(8080); | |
| server.start(); | |
| module.exports = SignalingServer; | |
| // Only start when run directly, not when imported | |
| if (require.main === module) { | |
| const port = process.env.PORT || 8080; | |
| const server = new SignalingServer(port); | |
| server.start(); | |
| } | |
| module.exports = SignalingServer; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 156 - 160, The module currently
creates and starts a SignalingServer instance on import (const server = new
SignalingServer(8080); server.start()), which prevents testing and hardcodes the
port; remove this side-effect by moving instantiation/startup into a separate
entrypoint or guard it with require.main === module so the module only exports
the SignalingServer class, and create/start the server (calling new
SignalingServer(...) and server.start()) only when the file is executed
directly; update references to the exported symbol (SignalingServer) and remove
the top-level server/start calls.
![gowtham-kishore-dev[bot]](https://avatars.githubusercontent.com/u/70622086?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webrtc_peer.js`:
- Around line 46-50: The ontrack handler currently overwrites this.remoteStream
for every incoming track; change it to reuse the existing MediaStream
(this.remoteStream) or the provided event.streams[0] instead of replacing it: if
event.streams && event.streams[0] exists, assign that to this.remoteStream once;
otherwise ensure this.remoteStream is created only if not already set and call
this.remoteStream.addTrack(event.track) to accumulate tracks. Update the handler
attached to this.peerConnection.ontrack accordingly so multiple tracks
(audio+video) are preserved.
- Around line 271-281: getStats currently collapses multiple RTC stats of the
same type into a single entry by using report[stat.type] = stat; change getStats
to preserve all entries by using stat.id as the unique key (or alternatively
collect stats into arrays per type): iterate stats from
this.peerConnection.getStats(), and for each stat either set report[stat.id] =
stat or push stat into report[stat.type] = report[stat.type] || [] then
report[stat.type].push(stat); return the full report object so callers can
access every individual stat entry; reference function getStats and
this.peerConnection.getStats and use stat.id / stat.type to implement the fix.
- Around line 156-163: The addIceCandidate method should validate candidateData,
buffer candidates until a remote description exists, and handle errors: update
addIceCandidate to check that candidateData has the expected fields, push the
candidateData onto an instance array (_pendingRemoteCandidates) if
this.peerConnection.remoteDescription is null/undefined, and return early;
otherwise wrap new RTCIceCandidate(...) and await
this.peerConnection.addIceCandidate(candidate) inside a try/catch that only
logs/ignores benign errors (malformed or timing errors) without throwing;
finally ensure handleOffer and handleAnswer each flush _pendingRemoteCandidates
by calling addIceCandidate for each buffered entry after they set the remote
description.
- Around line 247-266: startScreenShare currently awaits
navigator.mediaDevices.getDisplayMedia without try/catch, replaces tracks on all
senders (including audio) via sender.replaceTrack(screenTrack), and never
restores the original camera track when screen sharing ends; wrap the
getDisplayMedia call in a try/catch to handle user cancellation, capture the
original video track (e.g., from this.localStream.getVideoTracks()[0] or from
the sender whose sender.track?.kind === "video"), call replaceTrack only on the
sender where sender.track?.kind === "video", and in screenTrack.onended call
replaceTrack(originalVideoTrack) to restore the camera.
- Around line 201-210: sendSignalingMessage currently creates a fresh WebSocket
per call and calls send before the socket is open; instead reuse the existing
this.signalingSocket (established in connectSignaling), queue messages when the
socket is CONNECTING (e.g. push into this._pendingSignaling), send immediately
when ready, and add error handling and optional close logic; update
connectSignaling's onopen to flush this._pendingSignaling and ensure
sendSignalingMessage checks this.signalingSocket.readyState, handles failures
(reject/emit/log) and avoids leaking sockets by not creating new WebSocket
instances.
- Around line 93-110: In createOffer, await the Promise returned by
this.peerConnection.setLocalDescription(offer) before sending the signaling
message and send the full RTCSessionDescription (not just offer.sdp) so the
remote gets both type and sdp; specifically, update createOffer to await
this.peerConnection.setLocalDescription(offer) and then call
this.sendSignalingMessage with the full offer object (or offer.toJSON()) and
then set this.isInitiator = true.
- Around line 62-88: getLocalMedia uses unrealistic hard "exact" 8K constraints
and disables audio processing, lacks error handling for
navigator.mediaDevices.getUserMedia, and assumes this.peerConnection exists;
change defaultConstraints in getLocalMedia to use flexible/ideal ranges (e.g.,
width/height/frameRate as ideal or min/max instead of exact), enable
echoCancellation/noiseSuppression/autoGainControl by default (make them opt-out
via options), wrap the await navigator.mediaDevices.getUserMedia(...) in a
try/catch to handle permission/device errors and return/propagate a clear error
or fallback stream, and before calling this.peerConnection.addTrack check that
this.peerConnection is initialized (or queue tracks until initialize() creates
the RTCPeerConnection); reference getLocalMedia, defaultConstraints,
navigator.mediaDevices.getUserMedia, this.localStream, this.peerConnection, and
initialize when making the changes.
- Around line 286-297: The close() method currently only calls
this.peerConnection.close(); update it to (1) stop all local media tracks by
iterating this.localStream.getTracks() and calling stop() on each track, (2)
close the data channel (this.dataChannel.close()) and remove its event handlers,
(3) close the signaling socket (this.signalingSocket.close()) and remove its
listeners, (4) remove or null out PeerConnection event handlers (e.g.,
this.peerConnection.ontrack, onicecandidate, onconnectionstatechange)
before/after closing, and (5) nullify references this.peerConnection,
this.dataChannel, this.signalingSocket, and this.localStream so the object can
be re-initialized cleanly.
- Around line 38-44: The onicecandidate handler currently pushes new ICE
candidates into this.iceCandidatesQueue (in webrtc_peer.js) which prevents them
being sent to the remote peer; change onicecandidate to forward each non-null
event.candidate immediately over the signaling channel (e.g., call the existing
signaling send function or emit via the same path used for offers/answers) and
only fall back to enqueueing if the signaling channel is not yet connected;
update handleAnswer (which currently flushes this.iceCandidatesQueue back into
the local RTCPeerConnection) so it does not re-add local candidates to the local
connection but instead drains any queued candidates by sending them out over the
signaling channel to the remote peer. Ensure you reference onicecandidate,
iceCandidatesQueue, and handleAnswer when making the changes.
- Around line 168-196: The createDataChannel method currently creates
this.dataChannel without reliability options and sets an unguarded onmessage
that JSON.parse(...) can throw and lacks onerror/onclose; update
createDataChannel to create a reliable/unordered channel if needed, wrap the
onmessage handler in a try/catch that forwards parse errors to
this.onDataChannelError (or logs and ignores) and add this.dataChannel.onerror
and this.dataChannel.onclose handlers to manage state; in sendMessage, validate
that this.dataChannel exists and this.dataChannel.readyState === 'open' before
sending, enforce/validate payload size and either split large
JSON.stringify(message) payloads into chunks (implement a chunking helper like
chunkAndSend) or reject oversized messages with a clear error, and surface any
send errors to a dedicated handler (e.g., this.onDataChannelError) instead of
letting InvalidStateError bubble.
- Around line 115-134: handleOffer constructs the RTCSessionDescription and uses
the RTCPeerConnection methods incorrectly: ensure you accept both object and
string input for offerData (if offerData is an object extract the sdp string,
otherwise use it directly) when building the RTCSessionDescription for the
incoming offer; await this.peerConnection.setRemoteDescription(...) before
calling createAnswer(); after creating the answer call await
this.peerConnection.setLocalDescription(answer) (not setRemoteDescription); and
when calling sendSignalingMessage send a consistent value (e.g., send sdp:
answer.sdp as a string or always send the full answer object across the app) so
the remote side can parse it correctly.
- Around line 222-242: Replace the fragile if/if/if dispatch in the
signalingSocket.onmessage handler with a switch on message.type (or equivalent)
and ensure each branch calls the correct handler signature: call
this.handleOffer(message.sdp) for "offer", call this.handleAnswer(message.sdp)
for "answer" (do not pass the entire message object), and call
this.addIceCandidate(message.candidate) for "ice-candidate" (use
message.candidate, not message.ice). Also add robust close/error handling on
this.signalingSocket (implement onerror and onclose to trigger a reconnection
strategy with exponential backoff and a max retry count) so the peer will
attempt to reconnect cleanly when the signaling connection is lost.
In `@webrtc_signaling_server.js`:
- Around line 156-160: The module currently instantiates SignalingServer with a
hardcoded port and calls server.start() on require/import (the lines creating
const server = new SignalingServer(8080); and server.start()), causing an
immediate side-effect; remove that instantiation and start call so the module
only exports the SignalingServer class, and move the bootstrapping (new
SignalingServer(port) and start()) into a separate entrypoint (e.g.,
bin/signaling-server.js) that reads configuration or CLI args before calling
SignalingServer.start().
- Around line 118-130: handleLeave currently uses indexOf + splice which can
splice(-1) when clientId not found and only removes the first occurrence if
client joined twice, and it never deletes empty rooms; update handleLeave to (1)
locate and remove all occurrences of clientId from this.rooms[roomId] (e.g.
replace array with a filtered version excluding clientId or loop to remove all
matches) (2) guard against index === -1 before splicing if you keep splice-based
logic, and (3) if the resulting array is empty, delete this.rooms[roomId] to
avoid stale empty rooms; refer to the handleLeave method and this.rooms[roomId]
when making the changes.
- Around line 135-153: broadcast is iterating this.clients and calling ws.send
without checking connection state so a disconnected socket can throw; update
broadcast to skip or remove clients whose connection is not open (check
ws.readyState === WebSocket.OPEN or equivalent and only call send for open
sockets, optionally cleaning up closed entries from this.clients) and guard send
calls with try/catch to avoid one failure breaking the loop; getRoomInfo assumes
this.rooms[roomId] exists and reads .length which throws — change getRoomInfo to
treat missing rooms as an empty array (e.g., const peers =
Array.isArray(this.rooms[roomId]) ? this.rooms[roomId] : []; return peers and
peers.length safely) so callers always get peers: [] and count: 0 for unknown
rooms.
- Around line 29-44: Wrap the ws.on("message") handler's JSON.parse(data) in a
try/catch and validate the incoming frame before calling
this.handleMessage(clientId, ws, message) so malformed or non-JSON frames are
rejected/closed instead of throwing; add a ws.on("error", err => { ... })
listener to log the error and gracefully close the socket to prevent uncaught
'error' events from crashing the process; and in the ws.on("close") handler,
besides this.clients.delete(clientId) and logging, remove the clientId from any
room arrays stored on this.rooms (so subsequent handleJoin/broadcast and
peerWs.send calls don't iterate stale ids).
- Around line 71-100: handleJoin currently pushes clientId into this.rooms
without a size cap, returns the peers list including the joining client, and
blindly calls peerWs.send for every peer which throws if peerWs is missing or
not OPEN; fix it by enforcing a max room size (e.g., check
this.rooms[roomId].length against a constant before pushing and reject/close the
join if exceeded), when sending the "room-joined" message to the joining socket
send only existing peers excluding clientId (derive peers =
this.rooms[roomId].filter(id => id !== clientId) before sending), and when
notifying others iterate this.rooms[roomId].forEach(peerId => { if (peerId ===
clientId) return; const peerWs = this.clients.get(peerId); if (!peerWs ||
peerWs.readyState !== WebSocket.OPEN) return; peerWs.send(...); }) so you skip
missing/closed sockets and don’t notify the joiner about itself; use the
handleJoin, this.rooms, this.clients, peerWs, and ws.send symbols to locate the
code to change.
- Around line 17-24: The server currently uses http.createServer and accepts
WebSocket connections without auth/origin checks and generates weak IDs; update
the transport to terminate TLS (use https.createServer with proper certs or
mandate a TLS-terminating proxy before exposing the server used to construct
WebSocket.Server), add authentication and origin validation inside the
this.wss.on("connection", ...) handler (reject unauthenticated or unexpected
origins/clients and implement rate-limiting hooks), and replace the weak
clientId assignment (Math.random().toString(36).substr(2, 9)) with a
cryptographically secure identifier such as crypto.randomUUID() (or another
crypto-secure generator) to avoid predictability and deprecated substr usage.
- Around line 102-116: relayMessage currently trusts message.target and blindly
spreads message back to the receiver and will throw if the target socket is not
OPEN; validate that message.target exists and is a non-empty string/ID, lookup
the targetWs from this.clients and check targetWs.readyState === WebSocket.OPEN
(use the OPEN constant) before sending, and avoid spreading the entire incoming
message — build a safe payload that explicitly sets type and from (e.g., { type:
message.type, from: senderId, payload: message.payload }) so callers cannot
spoof from/type; if target is missing or not open, send a clear error/ack back
to the senderId (via this.clients.get(senderId)) instead of silently dropping or
throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc44a4b6-0578-43b3-9eb7-d3e28f1fbfc4
📒 Files selected for processing (2)
webrtc_peer.jswebrtc_signaling_server.js
Comment on lines
+38
to
+44
| this.peerConnection.onicecandidate = (event) => { | ||
| if (event.candidate) { | ||
| // BUG: pushing to local queue instead of sending to remote peer | ||
| this.iceCandidatesQueue.push(event.candidate); | ||
| console.log("ICE candidate queued locally (never sent)"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
ICE candidates are queued locally and never reach the remote peer — handshake cannot complete.
onicecandidate is supposed to forward each non-null candidate to the remote peer over the signaling channel. Pushing them into a local array means the remote side never learns any of this peer's network paths and ICE will always fail. The "flush" in handleAnswer (Line 147–150) then adds these local candidates back into the local peer connection, which is a no-op (or error) — not what the comment implies.
🛠 Proposed fix
- this.peerConnection.onicecandidate = (event) => {
- if (event.candidate) {
- this.iceCandidatesQueue.push(event.candidate);
- console.log("ICE candidate queued locally (never sent)");
- }
- };
+ this.peerConnection.onicecandidate = (event) => {
+ if (event.candidate) {
+ this.sendSignalingMessage({
+ type: "ice-candidate",
+ candidate: event.candidate.toJSON(),
+ });
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.peerConnection.onicecandidate = (event) => { | |
| if (event.candidate) { | |
| // BUG: pushing to local queue instead of sending to remote peer | |
| this.iceCandidatesQueue.push(event.candidate); | |
| console.log("ICE candidate queued locally (never sent)"); | |
| } | |
| }; | |
| this.peerConnection.onicecandidate = (event) => { | |
| if (event.candidate) { | |
| this.sendSignalingMessage({ | |
| type: "ice-candidate", | |
| candidate: event.candidate.toJSON(), | |
| }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 38 - 44, The onicecandidate handler currently
pushes new ICE candidates into this.iceCandidatesQueue (in webrtc_peer.js) which
prevents them being sent to the remote peer; change onicecandidate to forward
each non-null event.candidate immediately over the signaling channel (e.g., call
the existing signaling send function or emit via the same path used for
offers/answers) and only fall back to enqueueing if the signaling channel is not
yet connected; update handleAnswer (which currently flushes
this.iceCandidatesQueue back into the local RTCPeerConnection) so it does not
re-add local candidates to the local connection but instead drains any queued
candidates by sending them out over the signaling channel to the remote peer.
Ensure you reference onicecandidate, iceCandidatesQueue, and handleAnswer when
making the changes.
Comment on lines
+46
to
+50
| this.peerConnection.ontrack = (event) => { | ||
| // BUG: overwriting the stream on every track instead of adding tracks | ||
| this.remoteStream = new MediaStream(); | ||
| this.remoteStream.addTrack(event.track); | ||
| }; |
There was a problem hiding this comment.
ontrack discards previously-received tracks.
Each track event overwrites this.remoteStream with a fresh MediaStream containing only the latest track, so a peer receiving audio + video ends up with only one of them. Reuse the stream provided by the event (or accumulate tracks onto a single MediaStream).
🛠 Proposed fix
this.peerConnection.ontrack = (event) => {
- this.remoteStream = new MediaStream();
- this.remoteStream.addTrack(event.track);
+ if (event.streams && event.streams[0]) {
+ this.remoteStream = event.streams[0];
+ } else {
+ if (!this.remoteStream) this.remoteStream = new MediaStream();
+ this.remoteStream.addTrack(event.track);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.peerConnection.ontrack = (event) => { | |
| // BUG: overwriting the stream on every track instead of adding tracks | |
| this.remoteStream = new MediaStream(); | |
| this.remoteStream.addTrack(event.track); | |
| }; | |
| this.peerConnection.ontrack = (event) => { | |
| if (event.streams && event.streams[0]) { | |
| this.remoteStream = event.streams[0]; | |
| } else { | |
| if (!this.remoteStream) this.remoteStream = new MediaStream(); | |
| this.remoteStream.addTrack(event.track); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 46 - 50, The ontrack handler currently
overwrites this.remoteStream for every incoming track; change it to reuse the
existing MediaStream (this.remoteStream) or the provided event.streams[0]
instead of replacing it: if event.streams && event.streams[0] exists, assign
that to this.remoteStream once; otherwise ensure this.remoteStream is created
only if not already set and call this.remoteStream.addTrack(event.track) to
accumulate tracks. Update the handler attached to this.peerConnection.ontrack
accordingly so multiple tracks (audio+video) are preserved.
Comment on lines
+62
to
+88
| async getLocalMedia(constraints) { | ||
| // BUG: no error handling if getUserMedia fails (camera/mic denied) | ||
| // BUG: requesting 8K resolution that most cameras don't support | ||
| const defaultConstraints = { | ||
| video: { | ||
| width: { exact: 7680 }, | ||
| height: { exact: 4320 }, | ||
| frameRate: { ideal: 120 }, | ||
| }, | ||
| audio: { | ||
| echoCancellation: false, // BUG: echo cancellation disabled | ||
| noiseSuppression: false, // BUG: noise suppression disabled | ||
| autoGainControl: false, // BUG: auto gain control disabled | ||
| }, | ||
| }; | ||
|
|
||
| this.localStream = await navigator.mediaDevices.getUserMedia( | ||
| constraints || defaultConstraints | ||
| ); | ||
|
|
||
| // BUG: adding tracks but never checking if peerConnection exists | ||
| this.localStream.getTracks().forEach((track) => { | ||
| this.peerConnection.addTrack(track, this.localStream); | ||
| }); | ||
|
|
||
| return this.localStream; | ||
| } |
There was a problem hiding this comment.
Default constraints will fail on virtually every device.
- Lines 67–68:
width.exact: 7680, height.exact: 4320(8K).exactis anOverconstrainedErroron any device that cannot deliver exactly those dimensions — i.e. essentially all webcams. Combined withframeRate.ideal: 120, the default path is effectively a permission denial. - Lines 72–74: disabling
echoCancellation,noiseSuppression, andautoGainControlby default makes group audio unusable; these should be opt-out, not opt-in. - Line 78–80:
await getUserMediais unguarded; permission denial throws and there is no fallback or user-facing message. - Line 83–84: tracks are added without checking
this.peerConnection— callinggetLocalMediabeforeinitialize()throwsCannot read properties of null.
🛠 Proposed fix
- const defaultConstraints = {
- video: {
- width: { exact: 7680 },
- height: { exact: 4320 },
- frameRate: { ideal: 120 },
- },
- audio: {
- echoCancellation: false,
- noiseSuppression: false,
- autoGainControl: false,
- },
- };
-
- this.localStream = await navigator.mediaDevices.getUserMedia(
- constraints || defaultConstraints
- );
-
- this.localStream.getTracks().forEach((track) => {
- this.peerConnection.addTrack(track, this.localStream);
- });
+ const defaultConstraints = {
+ video: { width: { ideal: 1280 }, height: { ideal: 720 }, frameRate: { ideal: 30 } },
+ audio: { echoCancellation: true, noiseSuppression: true, autoGainControl: true },
+ };
+
+ try {
+ this.localStream = await navigator.mediaDevices.getUserMedia(
+ constraints || defaultConstraints
+ );
+ } catch (err) {
+ console.error("getUserMedia failed", err);
+ throw err;
+ }
+
+ if (!this.peerConnection) {
+ throw new Error("initialize() must be called before getLocalMedia()");
+ }
+ for (const track of this.localStream.getTracks()) {
+ this.peerConnection.addTrack(track, this.localStream);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async getLocalMedia(constraints) { | |
| // BUG: no error handling if getUserMedia fails (camera/mic denied) | |
| // BUG: requesting 8K resolution that most cameras don't support | |
| const defaultConstraints = { | |
| video: { | |
| width: { exact: 7680 }, | |
| height: { exact: 4320 }, | |
| frameRate: { ideal: 120 }, | |
| }, | |
| audio: { | |
| echoCancellation: false, // BUG: echo cancellation disabled | |
| noiseSuppression: false, // BUG: noise suppression disabled | |
| autoGainControl: false, // BUG: auto gain control disabled | |
| }, | |
| }; | |
| this.localStream = await navigator.mediaDevices.getUserMedia( | |
| constraints || defaultConstraints | |
| ); | |
| // BUG: adding tracks but never checking if peerConnection exists | |
| this.localStream.getTracks().forEach((track) => { | |
| this.peerConnection.addTrack(track, this.localStream); | |
| }); | |
| return this.localStream; | |
| } | |
| async getLocalMedia(constraints) { | |
| // BUG: no error handling if getUserMedia fails (camera/mic denied) | |
| // BUG: requesting 8K resolution that most cameras don't support | |
| const defaultConstraints = { | |
| video: { width: { ideal: 1280 }, height: { ideal: 720 }, frameRate: { ideal: 30 } }, | |
| audio: { echoCancellation: true, noiseSuppression: true, autoGainControl: true }, | |
| }; | |
| try { | |
| this.localStream = await navigator.mediaDevices.getUserMedia( | |
| constraints || defaultConstraints | |
| ); | |
| } catch (err) { | |
| console.error("getUserMedia failed", err); | |
| throw err; | |
| } | |
| if (!this.peerConnection) { | |
| throw new Error("initialize() must be called before getLocalMedia()"); | |
| } | |
| for (const track of this.localStream.getTracks()) { | |
| this.peerConnection.addTrack(track, this.localStream); | |
| } | |
| return this.localStream; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 62 - 88, getLocalMedia uses unrealistic hard
"exact" 8K constraints and disables audio processing, lacks error handling for
navigator.mediaDevices.getUserMedia, and assumes this.peerConnection exists;
change defaultConstraints in getLocalMedia to use flexible/ideal ranges (e.g.,
width/height/frameRate as ideal or min/max instead of exact), enable
echoCancellation/noiseSuppression/autoGainControl by default (make them opt-out
via options), wrap the await navigator.mediaDevices.getUserMedia(...) in a
try/catch to handle permission/device errors and return/propagate a clear error
or fallback stream, and before calling this.peerConnection.addTrack check that
this.peerConnection is initialized (or queue tracks until initialize() creates
the RTCPeerConnection); reference getLocalMedia, defaultConstraints,
navigator.mediaDevices.getUserMedia, this.localStream, this.peerConnection, and
initialize when making the changes.
Comment on lines
+93
to
+110
| async createOffer() { | ||
| const offer = await this.peerConnection.createOffer({ | ||
| offerToReceiveAudio: true, | ||
| offerToReceiveVideo: true, | ||
| }); | ||
|
|
||
| // BUG: setting local description but not waiting for it | ||
| this.peerConnection.setLocalDescription(offer); | ||
|
|
||
| // BUG: sending offer before setLocalDescription completes | ||
| this.sendSignalingMessage({ | ||
| type: "offer", | ||
| sdp: offer.sdp, | ||
| // BUG: not sending the full offer object, just the SDP string | ||
| }); | ||
|
|
||
| this.isInitiator = true; | ||
| } |
There was a problem hiding this comment.
createOffer does not await setLocalDescription and ships only the sdp string.
Line 100: setLocalDescription(offer) returns a Promise; not awaiting it means the offer can be sent (Line 103–107) before the local PC has applied it, leading to a race where the remote answers an offer that doesn't match the local state. Line 105 also strips the type field — the receiver in handleOffer then has to manufacture it (which it does, fragilely). Send the full description object.
🛠 Proposed fix
async createOffer() {
const offer = await this.peerConnection.createOffer({
offerToReceiveAudio: true,
offerToReceiveVideo: true,
});
-
- this.peerConnection.setLocalDescription(offer);
-
- this.sendSignalingMessage({
- type: "offer",
- sdp: offer.sdp,
- });
-
- this.isInitiator = true;
+ await this.peerConnection.setLocalDescription(offer);
+ this.isInitiator = true;
+ this.sendSignalingMessage({ type: "offer", offer });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async createOffer() { | |
| const offer = await this.peerConnection.createOffer({ | |
| offerToReceiveAudio: true, | |
| offerToReceiveVideo: true, | |
| }); | |
| // BUG: setting local description but not waiting for it | |
| this.peerConnection.setLocalDescription(offer); | |
| // BUG: sending offer before setLocalDescription completes | |
| this.sendSignalingMessage({ | |
| type: "offer", | |
| sdp: offer.sdp, | |
| // BUG: not sending the full offer object, just the SDP string | |
| }); | |
| this.isInitiator = true; | |
| } | |
| async createOffer() { | |
| const offer = await this.peerConnection.createOffer({ | |
| offerToReceiveAudio: true, | |
| offerToReceiveVideo: true, | |
| }); | |
| await this.peerConnection.setLocalDescription(offer); | |
| this.isInitiator = true; | |
| this.sendSignalingMessage({ type: "offer", offer }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 93 - 110, In createOffer, await the Promise
returned by this.peerConnection.setLocalDescription(offer) before sending the
signaling message and send the full RTCSessionDescription (not just offer.sdp)
so the remote gets both type and sdp; specifically, update createOffer to await
this.peerConnection.setLocalDescription(offer) and then call
this.sendSignalingMessage with the full offer object (or offer.toJSON()) and
then set this.isInitiator = true.
Comment on lines
+115
to
+134
| async handleOffer(offerData) { | ||
| // BUG: constructing RTCSessionDescription incorrectly | ||
| const offer = new RTCSessionDescription({ | ||
| type: "offer", | ||
| sdp: offerData, // BUG: offerData might be an object, not a string | ||
| }); | ||
|
|
||
| // BUG: not awaiting setRemoteDescription | ||
| this.peerConnection.setRemoteDescription(offer); | ||
|
|
||
| const answer = await this.peerConnection.createAnswer(); | ||
|
|
||
| // BUG: setting remote description AGAIN instead of local description | ||
| await this.peerConnection.setRemoteDescription(answer); | ||
|
|
||
| this.sendSignalingMessage({ | ||
| type: "answer", | ||
| sdp: answer, | ||
| }); | ||
| } |
There was a problem hiding this comment.
handleOffer is broken in three ways — answer will never be accepted by the offerer.
- Line 117–120:
RTCSessionDescriptionexpectssdpto be the SDP string.offerDatais the full message payload fromconnectSignaling(this.handleOffer(message.sdp)— Line 227 — passes the sdp field, but the server relays the whole object so callers in other paths pass the object). This is ambiguous and breaks on the first non-string path. - Line 123:
setRemoteDescriptionis not awaited;createAnsweron Line 125 will race and frequently throwInvalidStateError. - Line 128: calls
setRemoteDescription(answer)on the locally created answer. That must besetLocalDescription(answer). Until this is fixed, no connection in this codebase can complete. - Line 132: sends
sdp: answer(full object) where the offerer side readsmessage.sdpas a string. Either sendanswer.sdpand reconstruct, or send the wholeanswerobject consistently.
🛠 Proposed fix
async handleOffer(offerData) {
- const offer = new RTCSessionDescription({
- type: "offer",
- sdp: offerData,
- });
-
- this.peerConnection.setRemoteDescription(offer);
-
- const answer = await this.peerConnection.createAnswer();
-
- this.peerConnection.setRemoteDescription(answer);
-
- this.sendSignalingMessage({
- type: "answer",
- sdp: answer,
- });
+ await this.peerConnection.setRemoteDescription(
+ new RTCSessionDescription(offerData) // expects { type: "offer", sdp }
+ );
+ const answer = await this.peerConnection.createAnswer();
+ await this.peerConnection.setLocalDescription(answer);
+ this.sendSignalingMessage({ type: "answer", answer });
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 115 - 134, handleOffer constructs the
RTCSessionDescription and uses the RTCPeerConnection methods incorrectly: ensure
you accept both object and string input for offerData (if offerData is an object
extract the sdp string, otherwise use it directly) when building the
RTCSessionDescription for the incoming offer; await
this.peerConnection.setRemoteDescription(...) before calling createAnswer();
after creating the answer call await
this.peerConnection.setLocalDescription(answer) (not setRemoteDescription); and
when calling sendSignalingMessage send a consistent value (e.g., send sdp:
answer.sdp as a string or always send the full answer object across the app) so
the remote side can parse it correctly.
Comment on lines
+71
to
+100
| handleJoin(clientId, ws, roomId) { | ||
| // BUG: no limit on room size — could have thousands of peers | ||
| if (!this.rooms[roomId]) { | ||
| this.rooms[roomId] = []; | ||
| } | ||
|
|
||
| this.rooms[roomId].push(clientId); | ||
|
|
||
| // BUG: sending the full client list including the joining client | ||
| ws.send( | ||
| JSON.stringify({ | ||
| type: "room-joined", | ||
| room: roomId, | ||
| peers: this.rooms[roomId], // Includes self | ||
| clientId: clientId, | ||
| }) | ||
| ); | ||
|
|
||
| // BUG: notifying ALL clients in room, even clients that already know about each other | ||
| this.rooms[roomId].forEach((peerId) => { | ||
| const peerWs = this.clients.get(peerId); | ||
| // BUG: not checking if peerWs exists or is open | ||
| peerWs.send( | ||
| JSON.stringify({ | ||
| type: "peer-joined", | ||
| peerId: clientId, | ||
| }) | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
handleJoin will throw when any peer's socket is not OPEN.
- Line 90–99: the loop sends to every peer including ones whose ws may be
undefined(already removed fromthis.clientsbut still in the room — see the close-handler bug above) or inCLOSING/CLOSEDstate.ws.sendon a non-open socket throws synchronously and aborts the rest of the loop, leaving the room half-notified. - The room is also notified about the joining peer that the joiner itself just received in the
peerslist (Line 84 —peersincludes self), causing each existing peer to learn of itself as "peer-joined". - No room-size cap; a single malicious client can push unbounded ids into
this.rooms[roomId].
🛠 Proposed fix
handleJoin(clientId, ws, roomId) {
- if (!this.rooms[roomId]) {
- this.rooms[roomId] = [];
- }
-
- this.rooms[roomId].push(clientId);
-
- ws.send(
- JSON.stringify({
- type: "room-joined",
- room: roomId,
- peers: this.rooms[roomId], // Includes self
- clientId: clientId,
- })
- );
-
- this.rooms[roomId].forEach((peerId) => {
- const peerWs = this.clients.get(peerId);
- peerWs.send(
- JSON.stringify({
- type: "peer-joined",
- peerId: clientId,
- })
- );
- });
+ if (!this.rooms[roomId]) this.rooms[roomId] = [];
+ if (this.rooms[roomId].length >= MAX_ROOM_SIZE) {
+ ws.send(JSON.stringify({ type: "error", error: "room_full" }));
+ return;
+ }
+
+ const existingPeers = [...this.rooms[roomId]];
+ this.rooms[roomId].push(clientId);
+
+ ws.send(JSON.stringify({
+ type: "room-joined",
+ room: roomId,
+ peers: existingPeers,
+ clientId,
+ }));
+
+ for (const peerId of existingPeers) {
+ const peerWs = this.clients.get(peerId);
+ if (peerWs && peerWs.readyState === WebSocket.OPEN) {
+ peerWs.send(JSON.stringify({ type: "peer-joined", peerId: clientId }));
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handleJoin(clientId, ws, roomId) { | |
| // BUG: no limit on room size — could have thousands of peers | |
| if (!this.rooms[roomId]) { | |
| this.rooms[roomId] = []; | |
| } | |
| this.rooms[roomId].push(clientId); | |
| // BUG: sending the full client list including the joining client | |
| ws.send( | |
| JSON.stringify({ | |
| type: "room-joined", | |
| room: roomId, | |
| peers: this.rooms[roomId], // Includes self | |
| clientId: clientId, | |
| }) | |
| ); | |
| // BUG: notifying ALL clients in room, even clients that already know about each other | |
| this.rooms[roomId].forEach((peerId) => { | |
| const peerWs = this.clients.get(peerId); | |
| // BUG: not checking if peerWs exists or is open | |
| peerWs.send( | |
| JSON.stringify({ | |
| type: "peer-joined", | |
| peerId: clientId, | |
| }) | |
| ); | |
| }); | |
| } | |
| handleJoin(clientId, ws, roomId) { | |
| if (!this.rooms[roomId]) this.rooms[roomId] = []; | |
| if (this.rooms[roomId].length >= MAX_ROOM_SIZE) { | |
| ws.send(JSON.stringify({ type: "error", error: "room_full" })); | |
| return; | |
| } | |
| const existingPeers = [...this.rooms[roomId]]; | |
| this.rooms[roomId].push(clientId); | |
| ws.send(JSON.stringify({ | |
| type: "room-joined", | |
| room: roomId, | |
| peers: existingPeers, | |
| clientId, | |
| })); | |
| for (const peerId of existingPeers) { | |
| const peerWs = this.clients.get(peerId); | |
| if (peerWs && peerWs.readyState === WebSocket.OPEN) { | |
| peerWs.send(JSON.stringify({ type: "peer-joined", peerId: clientId })); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 71 - 100, handleJoin currently
pushes clientId into this.rooms without a size cap, returns the peers list
including the joining client, and blindly calls peerWs.send for every peer which
throws if peerWs is missing or not OPEN; fix it by enforcing a max room size
(e.g., check this.rooms[roomId].length against a constant before pushing and
reject/close the join if exceeded), when sending the "room-joined" message to
the joining socket send only existing peers excluding clientId (derive peers =
this.rooms[roomId].filter(id => id !== clientId) before sending), and when
notifying others iterate this.rooms[roomId].forEach(peerId => { if (peerId ===
clientId) return; const peerWs = this.clients.get(peerId); if (!peerWs ||
peerWs.readyState !== WebSocket.OPEN) return; peerWs.send(...); }) so you skip
missing/closed sockets and don’t notify the joiner about itself; use the
handleJoin, this.rooms, this.clients, peerWs, and ws.send symbols to locate the
code to change.
Comment on lines
+102
to
+116
| relayMessage(senderId, message) { | ||
| // BUG: message.target could be undefined — no validation | ||
| const targetWs = this.clients.get(message.target); | ||
|
|
||
| if (targetWs) { | ||
| // BUG: not checking WebSocket readyState before sending | ||
| targetWs.send( | ||
| JSON.stringify({ | ||
| ...message, | ||
| from: senderId, | ||
| }) | ||
| ); | ||
| } | ||
| // BUG: silently drops message if target not found, no error to sender | ||
| } |
There was a problem hiding this comment.
relayMessage will throw on disconnected targets and silently drops on bad input.
Line 108: no readyState === OPEN guard — sending to a socket in CLOSING/CLOSED throws. Line 104: message.target is unvalidated; a missing key fetches undefined from the Map which is undefined, but the body trusts the rest of message and spreads it back to the target — any sender can forge arbitrary type/from fields by spreading.
🛠 Proposed fix
relayMessage(senderId, message) {
- const targetWs = this.clients.get(message.target);
-
- if (targetWs) {
- targetWs.send(
- JSON.stringify({
- ...message,
- from: senderId,
- })
- );
- }
+ if (typeof message.target !== "string") return;
+ const targetWs = this.clients.get(message.target);
+ if (!targetWs || targetWs.readyState !== WebSocket.OPEN) return;
+
+ targetWs.send(JSON.stringify({
+ type: message.type,
+ sdp: message.sdp,
+ candidate: message.candidate,
+ from: senderId,
+ }));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| relayMessage(senderId, message) { | |
| // BUG: message.target could be undefined — no validation | |
| const targetWs = this.clients.get(message.target); | |
| if (targetWs) { | |
| // BUG: not checking WebSocket readyState before sending | |
| targetWs.send( | |
| JSON.stringify({ | |
| ...message, | |
| from: senderId, | |
| }) | |
| ); | |
| } | |
| // BUG: silently drops message if target not found, no error to sender | |
| } | |
| relayMessage(senderId, message) { | |
| if (typeof message.target !== "string") return; | |
| const targetWs = this.clients.get(message.target); | |
| if (!targetWs || targetWs.readyState !== WebSocket.OPEN) return; | |
| targetWs.send(JSON.stringify({ | |
| type: message.type, | |
| sdp: message.sdp, | |
| candidate: message.candidate, | |
| from: senderId, | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 102 - 116, relayMessage currently
trusts message.target and blindly spreads message back to the receiver and will
throw if the target socket is not OPEN; validate that message.target exists and
is a non-empty string/ID, lookup the targetWs from this.clients and check
targetWs.readyState === WebSocket.OPEN (use the OPEN constant) before sending,
and avoid spreading the entire incoming message — build a safe payload that
explicitly sets type and from (e.g., { type: message.type, from: senderId,
payload: message.payload }) so callers cannot spoof from/type; if target is
missing or not open, send a clear error/ack back to the senderId (via
this.clients.get(senderId)) instead of silently dropping or throwing.
Comment on lines
+118
to
+130
| handleLeave(clientId, roomId) { | ||
| if (this.rooms[roomId]) { | ||
| // BUG: uses indexOf + splice which only removes first occurrence | ||
| // If client somehow joined twice, second entry remains | ||
| const index = this.rooms[roomId].indexOf(clientId); | ||
| this.rooms[roomId].splice(index, 1); | ||
|
|
||
| // BUG: not checking if index is -1 before splicing | ||
| // splice(-1, 1) removes the LAST element | ||
|
|
||
| // BUG: empty rooms are never deleted from the rooms object | ||
| } | ||
| } |
There was a problem hiding this comment.
handleLeave corrupts the room when the client is not in it.
indexOf returns -1 when the client isn't present (race with disconnect-cleanup, double-leave, or wrong room). splice(-1, 1) then removes the last element of the array — i.e. evicts an unrelated peer. Empty rooms are also never deleted.
🛠 Proposed fix
handleLeave(clientId, roomId) {
- if (this.rooms[roomId]) {
- const index = this.rooms[roomId].indexOf(clientId);
- this.rooms[roomId].splice(index, 1);
- }
+ const room = this.rooms[roomId];
+ if (!room) return;
+ const index = room.indexOf(clientId);
+ if (index === -1) return;
+ room.splice(index, 1);
+ if (room.length === 0) delete this.rooms[roomId];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handleLeave(clientId, roomId) { | |
| if (this.rooms[roomId]) { | |
| // BUG: uses indexOf + splice which only removes first occurrence | |
| // If client somehow joined twice, second entry remains | |
| const index = this.rooms[roomId].indexOf(clientId); | |
| this.rooms[roomId].splice(index, 1); | |
| // BUG: not checking if index is -1 before splicing | |
| // splice(-1, 1) removes the LAST element | |
| // BUG: empty rooms are never deleted from the rooms object | |
| } | |
| } | |
| handleLeave(clientId, roomId) { | |
| const room = this.rooms[roomId]; | |
| if (!room) return; | |
| const index = room.indexOf(clientId); | |
| if (index === -1) return; | |
| room.splice(index, 1); | |
| if (room.length === 0) delete this.rooms[roomId]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 118 - 130, handleLeave currently
uses indexOf + splice which can splice(-1) when clientId not found and only
removes the first occurrence if client joined twice, and it never deletes empty
rooms; update handleLeave to (1) locate and remove all occurrences of clientId
from this.rooms[roomId] (e.g. replace array with a filtered version excluding
clientId or loop to remove all matches) (2) guard against index === -1 before
splicing if you keep splice-based logic, and (3) if the resulting array is
empty, delete this.rooms[roomId] to avoid stale empty rooms; refer to the
handleLeave method and this.rooms[roomId] when making the changes.
Comment on lines
+135
to
+153
| broadcast(message) { | ||
| // BUG: iterating over Map values but not checking connection state | ||
| this.clients.forEach((ws, clientId) => { | ||
| // BUG: this will throw if any client has disconnected | ||
| ws.send(JSON.stringify(message)); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Get room info | ||
| */ | ||
| getRoomInfo(roomId) { | ||
| return { | ||
| room: roomId, | ||
| // BUG: returns undefined if room doesn't exist, not an empty array | ||
| peers: this.rooms[roomId], | ||
| count: this.rooms[roomId].length, // BUG: will throw if room is undefined | ||
| }; | ||
| } |
There was a problem hiding this comment.
broadcast and getRoomInfo both throw on the common path.
broadcast(Line 137–140) callsws.sendwithout checkingreadyState; one disconnected client kills the entire broadcast loop.getRoomInfo(Line 150–151) readsthis.rooms[roomId].lengthwithout checking existence — any caller passing an unknown room id raisesTypeError: Cannot read properties of undefined.
🛠 Proposed fix
broadcast(message) {
- this.clients.forEach((ws, clientId) => {
- ws.send(JSON.stringify(message));
- });
+ const payload = JSON.stringify(message);
+ for (const ws of this.clients.values()) {
+ if (ws.readyState === WebSocket.OPEN) {
+ try { ws.send(payload); } catch (_) { /* swallow per-client */ }
+ }
+ }
}
getRoomInfo(roomId) {
+ const peers = this.rooms[roomId] || [];
return {
room: roomId,
- peers: this.rooms[roomId],
- count: this.rooms[roomId].length,
+ peers,
+ count: peers.length,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| broadcast(message) { | |
| // BUG: iterating over Map values but not checking connection state | |
| this.clients.forEach((ws, clientId) => { | |
| // BUG: this will throw if any client has disconnected | |
| ws.send(JSON.stringify(message)); | |
| }); | |
| } | |
| /** | |
| * Get room info | |
| */ | |
| getRoomInfo(roomId) { | |
| return { | |
| room: roomId, | |
| // BUG: returns undefined if room doesn't exist, not an empty array | |
| peers: this.rooms[roomId], | |
| count: this.rooms[roomId].length, // BUG: will throw if room is undefined | |
| }; | |
| } | |
| broadcast(message) { | |
| const payload = JSON.stringify(message); | |
| for (const ws of this.clients.values()) { | |
| if (ws.readyState === WebSocket.OPEN) { | |
| try { ws.send(payload); } catch (_) { /* swallow per-client */ } | |
| } | |
| } | |
| } | |
| /** | |
| * Get room info | |
| */ | |
| getRoomInfo(roomId) { | |
| const peers = this.rooms[roomId] || []; | |
| return { | |
| room: roomId, | |
| peers, | |
| count: peers.length, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 135 - 153, broadcast is iterating
this.clients and calling ws.send without checking connection state so a
disconnected socket can throw; update broadcast to skip or remove clients whose
connection is not open (check ws.readyState === WebSocket.OPEN or equivalent and
only call send for open sockets, optionally cleaning up closed entries from
this.clients) and guard send calls with try/catch to avoid one failure breaking
the loop; getRoomInfo assumes this.rooms[roomId] exists and reads .length which
throws — change getRoomInfo to treat missing rooms as an empty array (e.g.,
const peers = Array.isArray(this.rooms[roomId]) ? this.rooms[roomId] : [];
return peers and peers.length safely) so callers always get peers: [] and count:
0 for unknown rooms.
Comment on lines
+156
to
+160
| // BUG: starts server immediately on require/import with hardcoded port | ||
| const server = new SignalingServer(8080); | ||
| server.start(); | ||
|
|
||
| module.exports = SignalingServer; |
There was a problem hiding this comment.
Side effect on import — instantiating and listening on port 8080 the moment any code require()s this module.
This makes the module impossible to import for tests, in another process, or in a different topology without immediately binding port 8080. The class export is the right contract; the auto-start belongs in a separate entrypoint (e.g. bin/signaling-server.js).
🛠 Proposed fix
-// BUG: starts server immediately on require/import with hardcoded port
-const server = new SignalingServer(8080);
-server.start();
-
module.exports = SignalingServer;
+
+if (require.main === module) {
+ const port = Number(process.env.PORT) || 8080;
+ new SignalingServer(port).start();
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // BUG: starts server immediately on require/import with hardcoded port | |
| const server = new SignalingServer(8080); | |
| server.start(); | |
| module.exports = SignalingServer; | |
| module.exports = SignalingServer; | |
| if (require.main === module) { | |
| const port = Number(process.env.PORT) || 8080; | |
| new SignalingServer(port).start(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 156 - 160, The module currently
instantiates SignalingServer with a hardcoded port and calls server.start() on
require/import (the lines creating const server = new SignalingServer(8080); and
server.start()), causing an immediate side-effect; remove that instantiation and
start call so the module only exports the SignalingServer class, and move the
bootstrapping (new SignalingServer(port) and start()) into a separate entrypoint
(e.g., bin/signaling-server.js) that reads configuration or CLI args before
calling SignalingServer.start().
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
webrtc_signaling_server.js (1)
10-10: Add JSDoc to the public entry points.
constructor,start,handleMessage,handleJoin,relayMessage, andhandleLeaveare public methods on the exported class but aren't documented.As per coding guidelines, "Verify public functions have docstrings".
Also applies to: 16-16, 54-54, 71-71, 102-102, 118-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webrtc_signaling_server.js` at line 10, Add JSDoc comments to all public methods of the exported class that currently lack documentation. For each of the methods constructor, start, handleMessage, handleJoin, relayMessage, and handleLeave, add a JSDoc block above the method definition that describes what the method does, its parameters, and its return value. Ensure each JSDoc comment follows the standard documentation format with descriptions of purpose, parameters using `@param`, and return type using `@returns`.webrtc_peer.js (1)
271-280: Don't key stats only bystat.type.Several report types occur multiple times, so this keeps only the last entry and drops the rest. Group by type into arrays if callers need a complete report.
📊 Suggested fix
const report = {}; stats.forEach((stat) => { - // BUG: overwriting stats with same type instead of collecting them - report[stat.type] = stat; + (report[stat.type] ??= []).push(stat); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webrtc_peer.js` around lines 271 - 280, The getStats method is currently keying each stat into report by stat.type, which overwrites multiple entries of the same type; change getStats (the async function using this.peerConnection.getStats) to aggregate stats by creating an array per type (e.g., if !report[stat.type] set report[stat.type] = [] then push stat) so all stat reports are preserved instead of only the last one for each stat.type; keep the rest of the method and return the grouped report.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webrtc_peer.js`:
- Around line 46-50: The ontrack handler recreates this.remoteStream for every
incoming track causing previously added tracks to be lost; change the handler in
the this.peerConnection.ontrack function to reuse an existing remote stream
(this.remoteStream) if present, otherwise prefer event.streams[0] when
available, and for each incoming event call remoteStream.addTrack(event.track)
(do not assign a new MediaStream on each track). Ensure initialization happens
once (e.g., set this.remoteStream = event.streams[0] || new MediaStream()) and
subsequent events only addTrack to that existing this.remoteStream.
- Around line 21-33: The current config object (iceServers and
iceTransportPolicy) hardcodes invalid STUN/TURN endpoints and credentials;
remove the embedded TURN secret and stop forcing relay by deleting the hardcoded
iceTransportPolicy value and hardcoded credential fields, and instead accept an
injected ICE configuration at construction time (e.g., via a constructor or init
parameter used by the module that builds the config), validate that the provided
iceServers is a non-empty array before using it, and fall back to a safe default
(e.g., empty iceServers and no iceTransportPolicy override) when none is
supplied; update uses of config, iceServers, and iceTransportPolicy in this file
so no secrets remain in source and servers can be supplied from outside the
module.
- Around line 286-297: The close() method currently only calls
this.peerConnection.close() and leaves local media, data channel, signaling
socket, event listeners, and object references alive; update close() to (1) stop
all tracks on this.localStream (and any tracks on senders from
this.peerConnection), (2) close and nullify this.dataChannel if present, (3)
close and nullify this.signalingSocket, (4) remove any event listeners attached
to this.peerConnection, this.dataChannel and this.signalingSocket, then close
the peerConnection, and finally nullify this.peerConnection, this.localStream,
this.dataChannel, this.signalingSocket and related state to avoid leaks (use the
method name close, and symbols this.peerConnection, this.localStream,
this.dataChannel, this.signalingSocket to locate what to change).
- Around line 93-100: The SDP offer/answer flow mishandles async calls and
session description usage in createOffer/createAnswer paths: await
this.peerConnection.setLocalDescription(offer) in createOffer; await
this.peerConnection.setRemoteDescription(offer) where the remote offer is
applied (reference setRemoteDescription); and when applying the generated
answer, call this.peerConnection.setLocalDescription(answer) (not
setRemoteDescription) and send answer.sdp over signaling (not the full answer
object). Update the createOffer/createAnswer functions to await
setLocalDescription/setRemoteDescription and to use .sdp when serializing
messages, referencing methods createOffer, setLocalDescription,
setRemoteDescription, and the answer handling code that sends the signaling
payload.
- Around line 35-57: Add an ondatachannel handler to the RTCPeerConnection
(this.peerConnection.ondatachannel) so the answering peer binds the incoming
channel into this.dataChannel and attaches the same event handlers the initiator
uses (onopen, onclose, onmessage); inside the onmessage handler wrap JSON.parse
in a try/catch to ignore or log malformed frames instead of throwing, and ensure
you set up any message dispatch logic the initiator expects so replies work
symmetrically with createDataChannel.
- Around line 65-85: The defaultConstraints are too strict and audio options are
disabled, and addTrack is called without ensuring peerConnection exists; update
defaultConstraints in getLocalMedia/getUserMedia to use realistic ideal values
(e.g., width: { ideal: 1280 }, height: { ideal: 720 }, frameRate: { ideal: 30 })
instead of exact 7680x4320@120 and enable audio helpers (echoCancellation: true,
noiseSuppression: true, autoGainControl: true), and wrap the track-adding block
with a guard that checks this.peerConnection before calling
this.peerConnection.addTrack (if missing, throw a clear error or return early
and advise calling initialize()/creating the RTCPeerConnection first); keep
function names like defaultConstraints, getUserMedia, getLocalMedia and
this.peerConnection to locate the changes.
- Around line 38-44: The onicecandidate handler
(this.peerConnection.onicecandidate) currently pushes local candidates into
this.iceCandidatesQueue instead of sending them to the remote peer; change it to
transmit each event.candidate over your signaling channel (e.g., call your
signaling send method or emit an "ice-candidate" message) so the remote peer
receives them. Conversely, when processing incoming remote candidates where
addIceCandidate(...) is called (the code around the addIceCandidate usage),
ensure you check peerConnection.remoteDescription first and if it's not set,
temporarily queue inbound candidates in a separate inbound queue and apply them
after setRemoteDescription completes; replace the current behavior of feeding
locally generated candidates back into addIceCandidate and use distinct
queues/flows for outbound (sent via signaling) vs inbound (queued until
remoteDescription is set).
- Around line 201-206: sendSignalingMessage currently creates a new WebSocket
for every message and calls send immediately, which can throw; instead, reuse
the connected socket created by connectSignaling by writing to
this.signalingSocket after connectSignaling has succeeded and is in OPEN state.
Update sendSignalingMessage to check this.signalingSocket, wait for or verify
readyState === WebSocket.OPEN (or queue messages until open), and call
this.signalingSocket.send(JSON.stringify(message)); ensure connectSignaling
assigns the active socket to this.signalingSocket and handles open/error events
so sendSignalingMessage can rely on it.
- Around line 247-265: The startScreenShare method needs error handling and must
only replace the video sender's track and restore the original camera track when
screen sharing ends: wrap the navigator.mediaDevices.getDisplayMedia call in a
try/catch inside startScreenShare to handle permission or API errors; when
successful, save the current local video track (e.g., from this.localStream or
by finding the video sender's track via this.peerConnection.getSenders()), then
find the RTCRtpSender whose track.kind === 'video' and call
replaceTrack(screenTrack) only on that sender (not all senders); set
screenTrack.onended to replace the screenTrack back with the saved original
video track using the same video sender and clear any saved state. Ensure
null/undefined guards around sender.track and this.localStream when locating the
original video track.
In `@webrtc_signaling_server.js`:
- Around line 12-13: The rooms storage using a plain object {} is vulnerable to
prototype pollution attacks because untrusted roomId values from websocket
payloads could be special keys like "__proto__" or "constructor" that break room
creation and lookup. Replace this.rooms = {} with this.rooms = new Map() to use
a Map instead, then update all room access patterns throughout the code
(including the room creation/lookup logic around lines 71-75) to use Map methods
such as .get(), .set(), .has(), and .delete() instead of direct object property
access.
- Around line 37-41: The WebSocket close event handler removes the client from
this.clients but leaves the clientId in all rooms it was a member of, causing
handleJoin() to return dead peer references. In the close event handler where
this.clients.delete(clientId) is called, also iterate through all rooms in
this.rooms and remove the clientId from each room's member list before logging
the disconnection. Apply this same fix to both occurrences of the close handler
(at lines 37-41 and 90-93).
- Around line 73-77: join and leave in webrtc_signaling_server.js allow
duplicate memberships and can remove the wrong client. In join(roomId,
clientId), initialize this.rooms[roomId] to a Set (or, if kept as an array,
check includes before adding) so the client is only added once. In leave(roomId,
clientId), guard for missing rooms, locate the exact clientId (return early if
not found), remove only that client, and delete the room when empty. Apply the
same dedup/guard logic to the membership handling block referenced at 118-128
that mutates this.rooms.
- Around line 156-160: The SignalingServer class instantiation and the
server.start() call are being executed at module load time with a hardcoded
port, creating unwanted side effects whenever the module is imported. Remove the
instantiation of SignalingServer and the server.start() call from the module
level in webrtc_signaling_server.js, and ensure only the SignalingServer class
is exported. This allows consumers of the module to instantiate the server with
their own configuration and control when it starts, making the module reusable
in tests and other contexts.
- Around line 146-151: The getRoomInfo() method does not handle the case where a
room does not exist in this.rooms. When roomId is not found, this.rooms[roomId]
returns undefined, causing the .length access to throw an error. Modify the
method to check if the room exists before accessing it; if the room does not
exist, return an empty array for the peers property and set count to 0,
otherwise return the actual peers array and its length.
- Around line 29-35: Wrap the ws.on("message") handler so incoming data is
validated and parsed inside a try/catch: check the payload is a string or
Buffer, attempt JSON.parse(data) inside a try block, and on any error send a
protocol error message or close the socket (e.g., close with a 1003/1007 code)
rather than letting the exception bubble; on success call
this.handleMessage(clientId, ws, message). Ensure you reference the
ws.on("message") listener, JSON.parse call, and this.handleMessage to locate and
modify the code.
- Around line 102-115: relayMessage currently assumes message.target exists and
the target's WebSocket is sendable, silently dropping messages; update
relayMessage to validate that message.target is present and that
this.clients.get(message.target) returns a socket whose readyState is OPEN (use
the ws readyState constant), and if either check fails send a JSON-formatted
error response back to the sender's socket (include an error code/reason and the
original message.id/from) instead of silently returning; reference the
relayMessage function, this.clients map, and the target WebSocket's readyState
when implementing these checks and error responses.
---
Nitpick comments:
In `@webrtc_peer.js`:
- Around line 271-280: The getStats method is currently keying each stat into
report by stat.type, which overwrites multiple entries of the same type; change
getStats (the async function using this.peerConnection.getStats) to aggregate
stats by creating an array per type (e.g., if !report[stat.type] set
report[stat.type] = [] then push stat) so all stat reports are preserved instead
of only the last one for each stat.type; keep the rest of the method and return
the grouped report.
In `@webrtc_signaling_server.js`:
- Line 10: Add JSDoc comments to all public methods of the exported class that
currently lack documentation. For each of the methods constructor, start,
handleMessage, handleJoin, relayMessage, and handleLeave, add a JSDoc block
above the method definition that describes what the method does, its parameters,
and its return value. Ensure each JSDoc comment follows the standard
documentation format with descriptions of purpose, parameters using `@param`, and
return type using `@returns`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bb3ef487-a5e5-48d2-9276-a76d7b125ae5
📒 Files selected for processing (2)
webrtc_peer.jswebrtc_signaling_server.js
Comment on lines
+21
to
+33
| // BUG: Using outdated/non-existent STUN/TURN servers | ||
| const config = { | ||
| iceServers: [ | ||
| { urls: "stun:stun.invalid-server.example:19302" }, | ||
| { | ||
| urls: "turn:turn.invalid-server.example:3478", | ||
| username: "admin", | ||
| credential: "password123", // BUG: hardcoded credentials in source | ||
| }, | ||
| ], | ||
| // BUG: using deprecated constraint | ||
| iceTransportPolicy: "relay", // Forces TURN only, but TURN server is invalid | ||
| }; |
There was a problem hiding this comment.
Remove the embedded TURN secret and stop forcing relay through dead servers.
With iceTransportPolicy: "relay" and only invalid ICE endpoints, this peer will never connect outside a mocked environment. Inject ICE config at construction time instead of baking credentials into source. As per coding guidelines, "Ensure no hardcoded secrets or API keys are present".
🔐 Suggested fix
- const config = {
- iceServers: [
- { urls: "stun:stun.invalid-server.example:19302" },
- {
- urls: "turn:turn.invalid-server.example:3478",
- username: "admin",
- credential: "password123", // BUG: hardcoded credentials in source
- },
- ],
- // BUG: using deprecated constraint
- iceTransportPolicy: "relay", // Forces TURN only, but TURN server is invalid
- };
-
- this.peerConnection = new RTCPeerConnection(config);
+ this.peerConnection = new RTCPeerConnection(this.rtcConfig ?? {});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 21 - 33, The current config object (iceServers
and iceTransportPolicy) hardcodes invalid STUN/TURN endpoints and credentials;
remove the embedded TURN secret and stop forcing relay by deleting the hardcoded
iceTransportPolicy value and hardcoded credential fields, and instead accept an
injected ICE configuration at construction time (e.g., via a constructor or init
parameter used by the module that builds the config), validate that the provided
iceServers is a non-empty array before using it, and fall back to a safe default
(e.g., empty iceServers and no iceTransportPolicy override) when none is
supplied; update uses of config, iceServers, and iceTransportPolicy in this file
so no secrets remain in source and servers can be supplied from outside the
module.
Comment on lines
+35
to
+57
| this.peerConnection = new RTCPeerConnection(config); | ||
|
|
||
| // BUG: never actually handling the ICE candidate event properly | ||
| this.peerConnection.onicecandidate = (event) => { | ||
| if (event.candidate) { | ||
| // BUG: pushing to local queue instead of sending to remote peer | ||
| this.iceCandidatesQueue.push(event.candidate); | ||
| console.log("ICE candidate queued locally (never sent)"); | ||
| } | ||
| }; | ||
|
|
||
| this.peerConnection.ontrack = (event) => { | ||
| // BUG: overwriting the stream on every track instead of adding tracks | ||
| this.remoteStream = new MediaStream(); | ||
| this.remoteStream.addTrack(event.track); | ||
| }; | ||
|
|
||
| // BUG: not handling connection state changes | ||
| // this.peerConnection.onconnectionstatechange = ... | ||
|
|
||
| // BUG: not handling ICE connection state | ||
| // this.peerConnection.oniceconnectionstatechange = ... | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When only one side calls RTCPeerConnection.createDataChannel(), does the other side need an ondatachannel handler to receive the channel?
💡 Result:
Yes, when one side calls RTCPeerConnection.createDataChannel to initiate a data channel using the default in-band negotiation, the other side must have an ondatachannel event handler set up to receive and access that channel [1][2][3]. When the remote peer calls createDataChannel, the RTCPeerConnection on the receiving side dispatches a datachannel event [1][4]. The handler for this event receives an RTCDataChannelEvent object, which contains the RTCDataChannel instance in its channel property [1][5]. Without this handler, the receiving side will not be notified that a new data channel has been added to the connection, and it will have no reference to the channel object needed to send or receive data [1][6]. This in-band negotiation is the default behavior [2][3]. Alternatively, if you use out-of-band negotiation by setting the negotiated option to true in createDataChannel on both sides, you do not need the ondatachannel handler, as both sides manually create the channel with the same agreed-upon ID [2][6][3].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/datachannel_event
- 2: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/createDataChannel
- 3: https://devdoc.net/web/developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/createDataChannel.html
- 4: https://udn.realityripple.com/docs/Web/API/RTCPeerConnection/ondatachannel
- 5: http://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannelEvent
- 6: https://stackoverflow.com/questions/29790784/difference-between-rtcpeerconnection-createdatachannel-and-rtcpeerconnection-o
🏁 Script executed:
find . -name "webrtc_peer.js" -type fRepository: gowthamkishore3799/coderabbit-test
Length of output: 95
🏁 Script executed:
cat -n ./webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 11270
The answering peer never binds the data channel.
Only the initiator creates this.dataChannel via createDataChannel() at line 170; the remote side never listens to peerConnection.ondatachannel, so it cannot receive or reply on the channel. Add an ondatachannel handler to the peer connection. Also guard the JSON parsing at line 178 so one malformed frame does not crash the message handler.
💬 Suggested fix
+ this.peerConnection.ondatachannel = (event) => {
+ this.dataChannel = event.channel;
+ this.dataChannel.onmessage = (messageEvent) => {
+ try {
+ this.onDataChannelMessage(JSON.parse(messageEvent.data));
+ } catch {
+ this.onDataChannelMessage(messageEvent.data);
+ }
+ };
+ };
@@
this.dataChannel.onmessage = (event) => {
- const message = JSON.parse(event.data);
+ try {
+ this.onDataChannelMessage(JSON.parse(event.data));
+ } catch {
+ this.onDataChannelMessage(event.data);
+ }
- this.onDataChannelMessage(message);
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 35 - 57, Add an ondatachannel handler to the
RTCPeerConnection (this.peerConnection.ondatachannel) so the answering peer
binds the incoming channel into this.dataChannel and attaches the same event
handlers the initiator uses (onopen, onclose, onmessage); inside the onmessage
handler wrap JSON.parse in a try/catch to ignore or log malformed frames instead
of throwing, and ensure you set up any message dispatch logic the initiator
expects so replies work symmetrically with createDataChannel.
Comment on lines
+38
to
+44
| this.peerConnection.onicecandidate = (event) => { | ||
| if (event.candidate) { | ||
| // BUG: pushing to local queue instead of sending to remote peer | ||
| this.iceCandidatesQueue.push(event.candidate); | ||
| console.log("ICE candidate queued locally (never sent)"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In WebRTC, should RTCPeerConnection.onicecandidate candidates be sent to the remote peer via signaling, and should addIceCandidate wait until remoteDescription is set?
💡 Result:
Yes, RTCPeerConnection.onicecandidate candidates must be sent to the remote peer via your signaling channel [1][2]. When the onicecandidate event fires, the event's candidate property contains an RTCIceCandidate object that you should transmit to the remote peer [1][3]. If the candidate property is null, it indicates that ICE gathering is complete, and no further candidates need to be sent [4][3]. Regarding addIceCandidate, you must ensure that the remote description is set before calling it [5][6]. If you attempt to add a candidate while the remote description is null, the operation will fail [5][6]. However, you do not need to manually wait for the setRemoteDescription promise to resolve before calling addIceCandidate [7]. The WebRTC specification implements an operations chain that ensures these tasks are executed in the correct order, meaning you can queue addIceCandidate calls even if the setRemoteDescription operation is still pending [7]. Similarly, you do not need to wait for one addIceCandidate promise to resolve before adding the next one; they can be added in parallel, and the browser will handle them in the correct sequence [7].
Citations:
- 1: https://wiki.developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/icecandidate_event
- 2: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/icecandidate_event
- 3: https://github.com/mdn/content/blob/main/files/en-us/web/api/rtcpeerconnection/icecandidate_event/index.md?plain=1
- 4: https://udn.realityripple.com/docs/Web/API/RTCPeerConnection/onicecandidate
- 5: ERROR Failed to add ice candidate: [Error: The remote description was null] when using addIceCandidate react-native-webrtc/react-native-webrtc#1377
- 6: https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addIceCandidate
- 7: https://stackoverflow.com/questions/64726652/should-i-wait-for-rtcpeerconnection-setremotedescription-rtcpeerconnection-a
🏁 Script executed:
find . -name "webrtc_peer.js" -type fRepository: gowthamkishore3799/coderabbit-test
Length of output: 95
🏁 Script executed:
cat -n ./webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 11270
Send local ICE candidates to the remote peer via signaling; don't queue and then feed them back into addIceCandidate().
The onicecandidate event fires when this peer generates its own candidates. These must be sent to the remote peer through your signaling channel. The current code queues them locally and later attempts to add them back as if they came from the remote side, which breaks ICE negotiation entirely.
Additionally, addIceCandidate() will fail if the remote description is not set yet. While the browser's operations chain allows queueing calls in advance, the safer approach is to check peerConnection.remoteDescription before adding.
Suggested fix
- this.peerConnection.onicecandidate = (event) => {
- if (event.candidate) {
- this.iceCandidatesQueue.push(event.candidate);
- console.log("ICE candidate queued locally (never sent)");
- }
- };
+ this.peerConnection.onicecandidate = (event) => {
+ if (event.candidate) {
+ this.sendSignalingMessage({
+ type: "ice-candidate",
+ candidate: event.candidate.toJSON(),
+ });
+ }
+ };
@@
- this.iceCandidatesQueue.forEach((candidate) => {
- this.peerConnection.addIceCandidate(candidate);
- });
- this.iceCandidatesQueue = [];
+ while (this.iceCandidatesQueue.length > 0) {
+ await this.peerConnection.addIceCandidate(this.iceCandidatesQueue.shift());
+ }
@@
- const candidate = new RTCIceCandidate(candidateData);
-
- // BUG: no try-catch, addIceCandidate can throw if called at wrong time
- await this.peerConnection.addIceCandidate(candidate);
+ const candidate = new RTCIceCandidate(candidateData);
+ if (!this.peerConnection.remoteDescription) {
+ this.iceCandidatesQueue.push(candidate);
+ return;
+ }
+ await this.peerConnection.addIceCandidate(candidate);Applies to lines 38–44 and 145–162.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 38 - 44, The onicecandidate handler
(this.peerConnection.onicecandidate) currently pushes local candidates into
this.iceCandidatesQueue instead of sending them to the remote peer; change it to
transmit each event.candidate over your signaling channel (e.g., call your
signaling send method or emit an "ice-candidate" message) so the remote peer
receives them. Conversely, when processing incoming remote candidates where
addIceCandidate(...) is called (the code around the addIceCandidate usage),
ensure you check peerConnection.remoteDescription first and if it's not set,
temporarily queue inbound candidates in a separate inbound queue and apply them
after setRemoteDescription completes; replace the current behavior of feeding
locally generated candidates back into addIceCandidate and use distinct
queues/flows for outbound (sent via signaling) vs inbound (queued until
remoteDescription is set).
Comment on lines
+46
to
+50
| this.peerConnection.ontrack = (event) => { | ||
| // BUG: overwriting the stream on every track instead of adding tracks | ||
| this.remoteStream = new MediaStream(); | ||
| this.remoteStream.addTrack(event.track); | ||
| }; |
There was a problem hiding this comment.
Keep a single remoteStream.
This recreates the stream for every incoming track, so the next track drops the previous one. Reuse event.streams[0] when available, or initialize once and append tracks.
🎥 Suggested fix
- this.peerConnection.ontrack = (event) => {
- // BUG: overwriting the stream on every track instead of adding tracks
- this.remoteStream = new MediaStream();
- this.remoteStream.addTrack(event.track);
- };
+ this.peerConnection.ontrack = (event) => {
+ if (event.streams?.[0]) {
+ this.remoteStream = event.streams[0];
+ return;
+ }
+ this.remoteStream ??= new MediaStream();
+ this.remoteStream.addTrack(event.track);
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.peerConnection.ontrack = (event) => { | |
| // BUG: overwriting the stream on every track instead of adding tracks | |
| this.remoteStream = new MediaStream(); | |
| this.remoteStream.addTrack(event.track); | |
| }; | |
| this.peerConnection.ontrack = (event) => { | |
| if (event.streams?.[0]) { | |
| this.remoteStream = event.streams[0]; | |
| return; | |
| } | |
| this.remoteStream ??= new MediaStream(); | |
| this.remoteStream.addTrack(event.track); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 46 - 50, The ontrack handler recreates
this.remoteStream for every incoming track causing previously added tracks to be
lost; change the handler in the this.peerConnection.ontrack function to reuse an
existing remote stream (this.remoteStream) if present, otherwise prefer
event.streams[0] when available, and for each incoming event call
remoteStream.addTrack(event.track) (do not assign a new MediaStream on each
track). Ensure initialization happens once (e.g., set this.remoteStream =
event.streams[0] || new MediaStream()) and subsequent events only addTrack to
that existing this.remoteStream.
Comment on lines
+65
to
+85
| const defaultConstraints = { | ||
| video: { | ||
| width: { exact: 7680 }, | ||
| height: { exact: 4320 }, | ||
| frameRate: { ideal: 120 }, | ||
| }, | ||
| audio: { | ||
| echoCancellation: false, // BUG: echo cancellation disabled | ||
| noiseSuppression: false, // BUG: noise suppression disabled | ||
| autoGainControl: false, // BUG: auto gain control disabled | ||
| }, | ||
| }; | ||
|
|
||
| this.localStream = await navigator.mediaDevices.getUserMedia( | ||
| constraints || defaultConstraints | ||
| ); | ||
|
|
||
| // BUG: adding tracks but never checking if peerConnection exists | ||
| this.localStream.getTracks().forEach((track) => { | ||
| this.peerConnection.addTrack(track, this.localStream); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In MediaDevices.getUserMedia constraints, how do exact resolution constraints behave compared with ideal constraints on unsupported cameras?
💡 Result:
In MediaDevices.getUserMedia, exact and ideal constraints behave fundamentally differently when a camera cannot satisfy the requested resolution [1][2]. Exact Constraints (Mandatory) When you use the exact keyword (or min/max constraints), the browser treats these as mandatory requirements [1][2]. If the underlying hardware cannot satisfy these constraints, the promise returned by getUserMedia will be rejected with an OverconstrainedError [1][3]. In this scenario, the user will not be prompted for camera permission because the browser determines the request cannot be fulfilled before initiating the stream [1][3]. Ideal Constraints (Optional) In contrast, ideal constraints are treated as preferences rather than hard requirements [1][2]. When you provide an ideal value, the browser attempts to find a camera and setting that has the smallest fitness distance from your requested value [1][2]. If the camera does not support the exact ideal resolution, the browser will still return a stream using the best available resolution that it can provide, without rejecting the promise [1][2]. Summary of Differences - Exact: Mandatory. If unsupported, the promise rejects with OverconstrainedError, and no stream is created [1][4]. - Ideal: Optional. If unsupported, the browser provides the closest possible match without failing [1][2]. Because of this, exact constraints are considered highly restrictive and should be used only when a specific capability is strictly required for your application to function [2][5]. For most use cases, ideal constraints are preferred to ensure the application remains functional across a wider range of hardware [6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Media_Capture_and_Streams_API/Constraints
- 3: https://blog.addpipe.com/getusermedia-video-constraints/
- 4: https://www.webrtc-developers.com/getusermedia-constraints-explained/
- 5: https://www.typeerror.org/docs/dom/media_streams_api/constraints
- 6: https://webrtchacks.com/getusermedia-resolutions-3/
🏁 Script executed:
# Find and examine the webrtc_peer.js file
fd webrtc_peer.jsRepository: gowthamkishore3799/coderabbit-test
Length of output: 93
🏁 Script executed:
# Read the webrtc_peer.js file, focusing on lines 65-85
head -90 webrtc_peer.js | tail -30Repository: gowthamkishore3799/coderabbit-test
Length of output: 1003
🏁 Script executed:
# Search for initialize() method and any peerConnection initialization
rg -n "initialize|peerConnection" webrtc_peer.js | head -20Repository: gowthamkishore3799/coderabbit-test
Length of output: 1243
Exact constraints will cause OverconstrainedError, and addTrack() is unguarded.
The exact 8K resolution (7680×4320) with 120fps will overconstrain most cameras and cause getUserMedia() to reject with OverconstrainedError—not a TypeError. Additionally, if peerConnection is missing, calling addTrack() will throw TypeError when trying to access the method on undefined.
Use moderate ideal defaults for broader device compatibility and add a guard to ensure initialize() was called before getLocalMedia().
Suggested fix
const defaultConstraints = {
video: {
- width: { exact: 7680 },
- height: { exact: 4320 },
- frameRate: { ideal: 120 },
+ width: { ideal: 1280 },
+ height: { ideal: 720 },
+ frameRate: { ideal: 30 },
},
@@
+ if (!this.peerConnection) {
+ throw new Error("Call initialize() before getLocalMedia()");
+ }
+
this.localStream = await navigator.mediaDevices.getUserMedia(
constraints || defaultConstraints
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_peer.js` around lines 65 - 85, The defaultConstraints are too strict
and audio options are disabled, and addTrack is called without ensuring
peerConnection exists; update defaultConstraints in getLocalMedia/getUserMedia
to use realistic ideal values (e.g., width: { ideal: 1280 }, height: { ideal:
720 }, frameRate: { ideal: 30 }) instead of exact 7680x4320@120 and enable audio
helpers (echoCancellation: true, noiseSuppression: true, autoGainControl: true),
and wrap the track-adding block with a guard that checks this.peerConnection
before calling this.peerConnection.addTrack (if missing, throw a clear error or
return early and advise calling initialize()/creating the RTCPeerConnection
first); keep function names like defaultConstraints, getUserMedia, getLocalMedia
and this.peerConnection to locate the changes.
Comment on lines
+37
to
+41
| ws.on("close", () => { | ||
| // BUG: removing client from map but not from rooms | ||
| this.clients.delete(clientId); | ||
| console.log(`Client disconnected: ${clientId}`); | ||
| }); |
There was a problem hiding this comment.
Clean room membership on disconnect before the next join hits this room.
close removes the socket from this.clients but leaves the client ID in every room. The next handleJoin() for that room can return dead peers and call send() on undefined.
🔧 One straightforward fix
ws.on("close", () => {
+ for (const roomId of Object.keys(this.rooms)) {
+ this.handleLeave(clientId, roomId);
+ }
this.clients.delete(clientId);
console.log(`Client disconnected: ${clientId}`);
});Also applies to: 90-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 37 - 41, The WebSocket close event
handler removes the client from this.clients but leaves the clientId in all
rooms it was a member of, causing handleJoin() to return dead peer references.
In the close event handler where this.clients.delete(clientId) is called, also
iterate through all rooms in this.rooms and remove the clientId from each room's
member list before logging the disconnection. Apply this same fix to both
occurrences of the close handler (at lines 37-41 and 90-93).
Comment on lines
+73
to
+77
| if (!this.rooms[roomId]) { | ||
| this.rooms[roomId] = []; | ||
| } | ||
|
|
||
| this.rooms[roomId].push(clientId); |
There was a problem hiding this comment.
Membership updates need deduplication and a missing-member guard.
join blindly appends the client every time, and leave removes whatever index it gets back. That lets repeated joins duplicate the peer list and lets an out-of-sync leave evict the wrong client.
✅ Safer membership handling
if (!this.rooms[roomId]) {
this.rooms[roomId] = [];
}
- this.rooms[roomId].push(clientId);
+ if (!this.rooms[roomId].includes(clientId)) {
+ this.rooms[roomId].push(clientId);
+ }
@@
handleLeave(clientId, roomId) {
- if (this.rooms[roomId]) {
- const index = this.rooms[roomId].indexOf(clientId);
- this.rooms[roomId].splice(index, 1);
-
- // BUG: not checking if index is -1 before splicing
- // splice(-1, 1) removes the LAST element
-
- // BUG: empty rooms are never deleted from the rooms object
- }
+ const peers = this.rooms[roomId];
+ if (!peers) {
+ return;
+ }
+
+ this.rooms[roomId] = peers.filter((id) => id !== clientId);
+ if (this.rooms[roomId].length === 0) {
+ delete this.rooms[roomId];
+ }
}Also applies to: 118-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 73 - 77, join and leave in
webrtc_signaling_server.js allow duplicate memberships and can remove the wrong
client. In join(roomId, clientId), initialize this.rooms[roomId] to a Set (or,
if kept as an array, check includes before adding) so the client is only added
once. In leave(roomId, clientId), guard for missing rooms, locate the exact
clientId (return early if not found), remove only that client, and delete the
room when empty. Apply the same dedup/guard logic to the membership handling
block referenced at 118-128 that mutates this.rooms.
Comment on lines
+102
to
+115
| relayMessage(senderId, message) { | ||
| // BUG: message.target could be undefined — no validation | ||
| const targetWs = this.clients.get(message.target); | ||
|
|
||
| if (targetWs) { | ||
| // BUG: not checking WebSocket readyState before sending | ||
| targetWs.send( | ||
| JSON.stringify({ | ||
| ...message, | ||
| from: senderId, | ||
| }) | ||
| ); | ||
| } | ||
| // BUG: silently drops message if target not found, no error to sender |
There was a problem hiding this comment.
Surface relay failures back to the sender.
If message.target is missing or no longer connected, this just drops the signaling message. That leaves both peers stuck waiting with no recoverable error path.
📨 Suggested fallback
relayMessage(senderId, message) {
- const targetWs = this.clients.get(message.target);
+ const senderWs = this.clients.get(senderId);
+ const targetWs = this.clients.get(message.target);
- if (targetWs) {
+ if (!message.target || !targetWs) {
+ senderWs?.send(
+ JSON.stringify({
+ type: "error",
+ error: "peer-unavailable",
+ target: message.target,
+ })
+ );
+ return;
+ }
+
+ if (targetWs) {
targetWs.send(
JSON.stringify({
...message,
from: senderId,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 102 - 115, relayMessage currently
assumes message.target exists and the target's WebSocket is sendable, silently
dropping messages; update relayMessage to validate that message.target is
present and that this.clients.get(message.target) returns a socket whose
readyState is OPEN (use the ws readyState constant), and if either check fails
send a JSON-formatted error response back to the sender's socket (include an
error code/reason and the original message.id/from) instead of silently
returning; reference the relayMessage function, this.clients map, and the target
WebSocket's readyState when implementing these checks and error responses.
Comment on lines
+146
to
+151
| getRoomInfo(roomId) { | ||
| return { | ||
| room: roomId, | ||
| // BUG: returns undefined if room doesn't exist, not an empty array | ||
| peers: this.rooms[roomId], | ||
| count: this.rooms[roomId].length, // BUG: will throw if room is undefined |
There was a problem hiding this comment.
getRoomInfo() should be safe for unknown rooms.
getRoomInfo("missing-room") currently throws on .length. Returning an empty peer list here makes the helper usable without defensive checks at every call site.
🧩 Suggested fix
getRoomInfo(roomId) {
+ const peers = this.rooms[roomId] ?? [];
return {
room: roomId,
- peers: this.rooms[roomId],
- count: this.rooms[roomId].length, // BUG: will throw if room is undefined
+ peers,
+ count: peers.length,
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 146 - 151, The getRoomInfo() method
does not handle the case where a room does not exist in this.rooms. When roomId
is not found, this.rooms[roomId] returns undefined, causing the .length access
to throw an error. Modify the method to check if the room exists before
accessing it; if the room does not exist, return an empty array for the peers
property and set count to 0, otherwise return the actual peers array and its
length.
Comment on lines
+156
to
+160
| // BUG: starts server immediately on require/import with hardcoded port | ||
| const server = new SignalingServer(8080); | ||
| server.start(); | ||
|
|
||
| module.exports = SignalingServer; |
There was a problem hiding this comment.
Avoid binding a port as a module side effect.
Requiring this file immediately starts a server on 8080, which makes the exported class hard to reuse in tests or other processes and can fail just by importing the module.
🚦 Safer startup pattern
-const server = new SignalingServer(8080);
-server.start();
+if (require.main === module) {
+ const server = new SignalingServer(Number(process.env.PORT) || 8080);
+ server.start();
+}
module.exports = SignalingServer;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webrtc_signaling_server.js` around lines 156 - 160, The SignalingServer class
instantiation and the server.start() call are being executed at module load time
with a hardcoded port, creating unwanted side effects whenever the module is
imported. Remove the instantiation of SignalingServer and the server.start()
call from the module level in webrtc_signaling_server.js, and ensure only the
SignalingServer class is exported. This allows consumers of the module to
instantiate the server with their own configuration and control when it starts,
making the module reusable in tests and other contexts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit