Issue#77 #191
Issue#77 #191
Conversation
WalkthroughAdds per-socket media-state caching and cleanup on the backend, simplifies chat join handling, and stops including sender id in media broadcasts. Frontend gates video acquisition on camOn, resets remote streams before new connections, ensures tracks are replaced (not duplicated), and propagates camOn/micOn into peer teardown and renegotiation flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend
participant Peers
rect rgb(200,220,255)
note right of Client: Client updates local media state
Client->>Backend: emit media:state {camOn, micOn}
Backend->>Backend: lastMediaState[socketId] = {camOn,micOn}
Backend->>Peers: broadcast media:state {camOn, micOn} -- without sender id
end
sequenceDiagram
participant Local as Local Client
participant Remote as Remote Peer
participant Signaling as Signaling Server
rect rgb(220,255,220)
Local->>Local: camOn true/false (gate)
alt camOn true
Local->>Local: acquire video track & add to PC
Local->>Signaling: send offer (with video)
else camOn false
Local->>Local: skip acquiring video track
Local->>Signaling: send offer (no video)
end
Signaling->>Remote: deliver offer
Remote->>Remote: reset/create remote MediaStream
Remote->>Remote: ontrack -> replace existing video tracks then add new
end
rect rgb(255,235,205)
Local->>Local: click Next -> teardownPeers(reason, camOn, micOn)
note right of Local: teardown uses current cam/mic to update UI/state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/components/RTC/webrtc-utils.tsx (1)
274-291: Answerer path lacks renegotiation after addTrack; remote won’t receive camera.When pc === receivingPcRef.current, you add a video track but never renegotiate.
Apply this minimal fix:
if (videoSenderRef.current) { await videoSenderRef.current.replaceTrack(track); } else if (pc) { const sender = pc.addTrack(track); videoSenderRef.current = sender; // console.log("Added new video track to existing connection"); - - if (sendingPcRef.current === pc) { - const offer = await pc.createOffer(); - await pc.setLocalDescription(offer); - socketRef.current?.emit("renegotiate-offer", { - roomId, - sdp: offer, - role: "caller" - }); - // console.log("📤 Sent renegotiation offer for camera turn on"); - } + const role = sendingPcRef.current === pc ? "caller" : "answerer"; + const offer = await pc.createOffer(); + await pc.setLocalDescription(offer); + socketRef.current?.emit("renegotiate-offer", { roomId, sdp: offer, role }); + // console.log("📤 Sent renegotiation offer for camera turn on"); }Optional follow‑up: centralize on pc.onnegotiationneeded in setupPeerConnection to avoid manual branches.
frontend/components/RTC/Room.tsx (2)
486-491: Screen share UI/stream mismatch on Next.You intentionally no‑op setScreenShareOn, but teardownPeers still stops screen share tracks; UI shows “on” with no stream.
Either preserve streams on Next or reset the UI. Minimal fix (reset UI to match teardown):
- setScreenShareOn: () => {}, // Don't reset screen share on next + setScreenShareOn,If the goal is to preserve screen share, gate teardownPeers’ screen-share cleanup on reason === "next".
588-601: Duplicate chat:join emits.You emit chat:join immediately and again after 100 ms; risks duplicate joins.
- s.emit("chat:join", { roomId: rid, name }); ... - setTimeout(() => { - s.emit("chat:join", { roomId: rid, name }); - }, 100); + s.emit("chat:join", { roomId: rid, name });Apply in both send-offer and offer handlers.
Also applies to: 623-635
🧹 Nitpick comments (4)
frontend/components/RTC/webrtc-utils.tsx (3)
321-325: Stopping a prop-owned localVideoTrack may have unintended side‑effects.localVideoTrack is passed in via props; stopping it here can affect upstream owners.
Guard to only stop tracks created by this module (currentVideoTrackRef), or pass an explicit flag indicating ownership.
249-251: Type the socket ref.Use Socket | null instead of any for socketRef param to catch event payload mistakes.
- socketRef: React.RefObject<any>, + socketRef: React.RefObject<Socket | null>,
120-122: All teardownPeers call sites are correctly ordered—refactoring to options object remains a good practice but not urgent.Verification found 2 call sites (lines 399 and 470 in Room.tsx), both passing arguments in correct order:
reason, camOn, micOn. While positional parameters are fragile compared to an options object, no actual misorderings were detected. The suggested refactoring to an options object would improve API resilience but is not critical.frontend/components/RTC/Room.tsx (1)
119-136: If no initial localVideoTrack, new camera track isn’t attached to local preview.When camOn is true and you create a fresh track, you add it to pc but not to localVideoRef.
if (camOn) { let videoTrack = currentVideoTrackRef.current; if (!videoTrack || videoTrack.readyState === "ended") { try { const stream = await navigator.mediaDevices.getUserMedia({ video: true }); videoTrack = stream.getVideoTracks()[0]; currentVideoTrackRef.current = videoTrack; + // Ensure local preview reflects the new camera track + if (localVideoRef.current) { + const ms = (localVideoRef.current.srcObject as MediaStream) || new MediaStream(); + ms.getVideoTracks().forEach(t => ms.removeTrack(t)); + ms.addTrack(videoTrack); + if (!localVideoRef.current.srcObject) localVideoRef.current.srcObject = ms; + await localVideoRef.current.play().catch(() => {}); + } } catch (err) { console.error("Error creating video track:", err); videoTrack = null; } }(Note: localVideoRef is in closure scope; if not, pass it into setupPeerConnection.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/bun.lockbis excluded by!**/bun.lockbfrontend/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (3)
backend/src/index.ts(4 hunks)frontend/components/RTC/Room.tsx(9 hunks)frontend/components/RTC/webrtc-utils.tsx(2 hunks)
🔇 Additional comments (2)
backend/src/index.ts (1)
162-163: Good cleanupRemoving the socket’s entry from
lastMediaStateon disconnect avoids leaks. LGTM.Confirm that
chat.ts(disconnecting handler) already notifies peers so UI can clear avatars/tracks promptly.frontend/components/RTC/Room.tsx (1)
559-562: Good: sync ref with peerCamOn.Keeps the ref in lockstep for render-agnostic checks.
| // store latest media state per-socket so late joiners can be informed | ||
| const lastMediaState = new Map<string, { micOn?: boolean; camOn?: boolean }>(); |
There was a problem hiding this comment.
lastMediaState is never written/read — late‑joiner hydration won’t work
You introduce the cache but don’t persist to it or read from it. As-is, late joiners can’t receive existing peers’ media states; this misses the issue #77 requirement.
Hook writes in media events and hydrate on join (see fixes on Lines 78-82 and 118-120).
| // ⬇️ Keep UserManager in sync when client explicitly joins later | ||
| socket.on("chat:join", ({ roomId }: { roomId: string; name?: string }) => { | ||
| if (roomId) userManager.setRoom(socket.id, roomId); | ||
| if (roomId) socket.join(roomId); | ||
| }); |
There was a problem hiding this comment.
Normalize room names and hydrate late joiners on chat:join
The handler joins the raw roomId while most emits use the chat: namespace. This mismatch will drop events. Also, use this hook to send existing peers’ media states to the joiner.
Apply this diff:
- // ⬇️ Keep UserManager in sync when client explicitly joins later
- socket.on("chat:join", ({ roomId }: { roomId: string; name?: string }) => {
- if (roomId) userManager.setRoom(socket.id, roomId);
- if (roomId) socket.join(roomId);
- });
+ // ⬇️ Keep UserManager in sync when client explicitly joins later
+ socket.on("chat:join", async ({ roomId }: { roomId: string; name?: string }) => {
+ const r = normalizeRoom(roomId);
+ if (!r) return;
+ userManager.setRoom(socket.id, r);
+ socket.join(r);
+ // Hydrate the joiner with current peers' media states
+ const socketsInRoom = await io.in(r).fetchSockets();
+ const peers = socketsInRoom
+ .filter((s) => s.id !== socket.id)
+ .map((s) => ({ id: s.id, state: lastMediaState.get(s.id) ?? {} }));
+ socket.emit("room:peers-media-state", { peers });
+ });Optional hardening (outside this range): make normalizeRoom idempotent to avoid double prefixing.
// replace existing normalizeRoom
const normalizeRoom = (r: string) => (r ? (r.startsWith("chat:") ? r : `chat:${r}`) : "");🤖 Prompt for AI Agents
In backend/src/index.ts around lines 78 to 82, the chat:join handler currently
uses the raw roomId which causes a mismatch with events that use the "chat:"
prefix and doesn't hydrate late joiners; update the handler to call
normalizeRoom(roomId) and use that normalizedRoom for userManager.setRoom and
socket.join, and after joining emit the current peers' media states to the
joining socket (e.g., iterate known peers in the room and send their media state
events to the joiner); also replace or ensure normalizeRoom is idempotent so it
prefixes "chat:" only once (optionally use the provided implementation).
| socket.on("media:state", ({ roomId, state }: { roomId: string; state: { micOn?: boolean; camOn?: boolean } }) => { | ||
| const r = toRoom(roomId); | ||
| if (r) socket.to(r).emit("peer:media-state", { state, from: socket.id }); | ||
| socket.to(roomId).emit("peer:media-state", { state }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
media:state drops from and uses raw roomId; also not updating cache
Without from, clients can’t attribute state changes; using raw roomId bypasses the chat: namespace. Also, not persisting to lastMediaState blocks late‑joiner sync.
Apply this diff:
- socket.on("media:state", ({ roomId, state }: { roomId: string; state: { micOn?: boolean; camOn?: boolean } }) => {
- socket.to(roomId).emit("peer:media-state", { state });
- });
+ socket.on("media:state", ({ roomId, state }: { roomId: string; state: { micOn?: boolean; camOn?: boolean } }) => {
+ const r = normalizeRoom(roomId);
+ // persist latest media state for late joiners
+ lastMediaState.set(socket.id, { ...(lastMediaState.get(socket.id) ?? {}), ...state });
+ if (r) socket.to(r).emit("peer:media-state", { state, from: socket.id });
+ });Keep the cache in sync for individual toggles too (outside-range snippet):
// media:cam
socket.on("media:cam", ({ roomId, on }: { roomId: string; on: boolean }) => {
const r = normalizeRoom(roomId);
lastMediaState.set(socket.id, { ...(lastMediaState.get(socket.id) ?? {}), camOn: on });
if (r) socket.to(r).emit("media:cam", { on, from: socket.id });
});
// media:mic
socket.on("media:mic", ({ roomId, on }: { roomId: string; on: boolean }) => {
const r = normalizeRoom(roomId);
lastMediaState.set(socket.id, { ...(lastMediaState.get(socket.id) ?? {}), micOn: on });
if (r) socket.to(r).emit("media:mic", { on, from: socket.id });
});Verify frontend expectations for peer:media-state:
🏁 Script executed:
#!/bin/bash
# Find handlers/usages of 'peer:media-state' and check if 'from' is referenced.
rg -n -C2 --type ts --type tsx "peer:media-state|from" frontend || rg -n -C2 "peer:media-state|from"Length of output: 30928
Now let me search for the actual functions used in the codebase:
🏁 Script executed:
#!/bin/bash
# Find toRoom function and lastMediaState usage
rg -n -B5 -A10 'toRoom|lastMediaState' backend/src/index.ts | head -100Length of output: 60
🏁 Script executed:
#!/bin/bash
# Find toRoom function definition and lastMediaState usage in backend
rg -n -B2 -A8 'const toRoom|function toRoom|toRoom\s*=' backend/src/index.ts
echo "---"
# Find lastMediaState declaration and usage
rg -n 'lastMediaState' backend/src/index.ts | head -20
echo "---"
# Show context around media:state handler and nearby handlers
sed -n '115,135p' backend/src/index.tsLength of output: 1388
Add from, use toRoom(), and update cache consistently across all media handlers
The media:state handler is indeed inconsistent with media:cam and media:mic—it bypasses the chat: namespace normalization and omits the sender ID. However, the cache persistence gap is wider: none of the media handlers update lastMediaState.
Apply the diff as provided, but also update media:cam and media:mic to persist state:
socket.on("media:cam", ({ roomId, on }: { roomId: string; on: boolean }) => {
const r = toRoom(roomId);
+ lastMediaState.set(socket.id, { ...(lastMediaState.get(socket.id) ?? {}), camOn: on });
if (r) socket.to(r).emit("media:cam", { on, from: socket.id });
});
socket.on("media:mic", ({ roomId, on }: { roomId: string; on: boolean }) => {
const r = toRoom(roomId);
+ lastMediaState.set(socket.id, { ...(lastMediaState.get(socket.id) ?? {}), micOn: on });
if (r) socket.to(r).emit("media:mic", { on, from: socket.id });
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/src/index.ts around lines 118 to 120, the media handlers are
inconsistent: update the "media:state" handler to include the sender id ("from")
and emit using toRoom(chatNamespace, roomId) (i.e., normalize the chat:
namespace) instead of socket.to(roomId).emit, and persist the new state into
lastMediaState cache; likewise, modify the "media:cam" and "media:mic" handlers
to normalize the namespace via toRoom(chatNamespace, roomId), include the sender
id in emitted payloads, and update lastMediaState with the combined per-user
state (micOn/camOn) so the cache is consistently persisted across all three
handlers.
| camOn, | ||
| micOn, | ||
| sendingPcRef, |
There was a problem hiding this comment.
Pass actual media liveness to teardownPeers during Next.
You compute currentCamState/currentMicState but pass camOn/micOn, which can be stale (e.g., track ended).
- camOn,
- micOn,
+ currentCamState,
+ currentMicState,📝 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.
| camOn, | |
| micOn, | |
| sendingPcRef, | |
| currentCamState, | |
| currentMicState, | |
| sendingPcRef, |
🤖 Prompt for AI Agents
In frontend/components/RTC/Room.tsx around lines 472 to 474, the teardownPeers
call is passing camOn and micOn which may be stale; instead pass the computed
currentCamState and currentMicState that reflect actual media liveness (e.g.,
derived from track.enabled/readyState or your currentCamState/currentMicState
variables). Update the teardownPeers invocation to use currentCamState and
currentMicState (ensure those variables are in scope at this call site) so peer
teardown uses the real-time media state.
| setters.setPeerMicOn(micOn); | ||
| setters.setPeerCamOn(camOn); | ||
| setters.setScreenShareOn(false); |
There was a problem hiding this comment.
Peer state reset uses local flags; likely incorrect UI semantics.
Setting setPeerMicOn(micOn) / setPeerCamOn(camOn) mirrors self into “peer” state and can mislead UI during lobby/transition.
Apply neutral defaults until the peer’s state arrives:
- setters.setPeerMicOn(micOn);
- setters.setPeerCamOn(camOn);
+ // Reset to a neutral/unknown state; will be updated by peer events
+ setters.setPeerMicOn(false);
+ setters.setPeerCamOn(false);📝 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.
| setters.setPeerMicOn(micOn); | |
| setters.setPeerCamOn(camOn); | |
| setters.setScreenShareOn(false); | |
| // Reset to a neutral/unknown state; will be updated by peer events | |
| setters.setPeerMicOn(false); | |
| setters.setPeerCamOn(false); | |
| setters.setScreenShareOn(false); |
🤖 Prompt for AI Agents
In frontend/components/RTC/webrtc-utils.tsx around lines 211 to 213, the code
sets peer state using local micOn/camOn flags which mirrors the local user into
the peer UI; change these to neutral defaults so the peer UI doesn’t falsely
reflect local state while waiting for the peer’s actual state — e.g.,
setPeerMicOn(false) and setPeerCamOn(false) (or null/undefined if your state
model supports an "unknown" value) and keep setScreenShareOn(false), so the peer
controls remain neutral until the peer’s presence/state update arrives.
|
@sarthakNITT will review it |
when i click on next the and turn on the camera the video tracks are not visible properly i mean it's totally blank @sarthakNITT
|
|
@sarthakNITT status? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/RTC/Room.tsx (1)
578-594: Remove duplicatechat:joinemissions.Both
send-offerandofferhandlers emitchat:jointwice: once immediately (lines 580, 614) and again after a 100ms delay (lines 592, 626). This duplication is unnecessary and may cause backend state issues or race conditions.Remove the immediate emission and keep only the delayed one:
s.on("send-offer", async ({ roomId: rid }) => { setRoomId(rid); - s.emit("chat:join", { roomId: rid, name }); setLobby(false);And:
s.on("offer", async ({ roomId: rid, sdp: remoteSdp }) => { setRoomId(rid); - s.emit("chat:join", { roomId: rid, name }); setLobby(false);Also applies to: 612-628
♻️ Duplicate comments (2)
frontend/components/RTC/Room.tsx (1)
464-465: Pass computed actual media state to teardownPeers.You compute
actualCamStateandactualMicStateat lines 358-359 but still pass the potentially stalecamOnandmicOnstate variables toteardownPeers. This does not address the previous review comment.Apply this diff:
reason, - camOn, - micOn, + actualCamState, + actualMicState, sendingPcRef,frontend/components/RTC/webrtc-utils.tsx (1)
211-212: Reset peer media state to neutral defaults, not local state.Setting
setPeerMicOn(micOn)andsetPeerCamOn(camOn)uses the local user's media state to represent the peer's state, which is semantically incorrect. After teardown there is no peer, so these should be reset to neutral defaults.Apply this diff:
// Reset UI states setters.setShowChat(false); - setters.setPeerMicOn(micOn); - setters.setPeerCamOn(camOn); + setters.setPeerMicOn(false); + setters.setPeerCamOn(false); setters.setScreenShareOn(false); setters.setPeerScreenShareOn(false);
🧹 Nitpick comments (2)
frontend/components/RTC/Room.tsx (2)
74-74: Remove unusedpeerCamOnRefor use it.
peerCamOnRefis initialized and synced viauseEffectbut never referenced elsewhere in the codebase. This appears to be dead code from an incomplete refactoring.Remove the unused ref and effect:
- const peerCamOnRef = useRef<boolean>(false);And:
- useEffect(() => { - peerCamOnRef.current = peerCamOn; - }, [peerCamOn]);Also applies to: 551-553
391-394: Compute actual media state before passing to teardownPeers.For consistency with
handleNextConnection, compute the actual live state of camera and microphone tracks before passing toteardownPeers.Apply this diff:
+ const actualCamState = !!(currentVideoTrackRef.current && currentVideoTrackRef.current.readyState === "live" && camOn); + const actualMicState = !!(localAudioTrack && localAudioTrack.readyState === "live" && micOn); + teardownPeers( "teardown", - camOn, - micOn, + actualCamState, + actualMicState, sendingPcRef,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/components/RTC/Room.tsx(9 hunks)frontend/components/RTC/webrtc-utils.tsx(3 hunks)
🔇 Additional comments (7)
frontend/components/RTC/Room.tsx (4)
119-136: LGTM: Video acquisition correctly gated on camera state.The video track is now only acquired and added to the peer connection when
camOnis true. This properly implements the requirement to stop the physical camera stream when the camera is toggled off.
142-147: LGTM: Remote video tracks handled correctly.The
ontrackhandler now removes existing video tracks before adding the new incoming track, preventing duplicates. The track is always added regardless of peer camera state, which correctly addresses the previous review concern about permanent blank video.
595-599: LGTM: Remote stream properly reset before new connections.Resetting the remote stream and clearing video/audio elements before establishing new peer connections ensures a clean slate and prevents track accumulation. This correctly supports the camera state preservation requirements.
Also applies to: 629-632
354-369: LGTM: Next button handler correctly computes media state.The
handleNextfunction properly computes the actual camera and microphone state before cleanup and navigation. Ensure these computed states are used in the subsequentteardownPeerscall withinhandleNextConnection(see related comment).frontend/components/RTC/webrtc-utils.tsx (3)
289-289: Verify thatroomIdis not null before emitting.The
media:camemission on line 289 usesroomIdwhich may be null if the user toggles the camera before joining a room or during transitions.Add a check:
- try { socketRef.current?.emit("media:cam", { roomId, on: true }); } catch {} + try { + if (socketRef.current && roomId) { + socketRef.current.emit("media:cam", { roomId, on: true }); + } + } catch {}
290-302: Add null check forroomIdin renegotiation flow.The renegotiation logic emits an offer with
roomIdwhich may be null during transitions or before room establishment. While wrapped in try/catch, a null roomId would send invalid data to the backend.Apply this diff:
try { - if (pc) { + if (pc && roomId) { const offer = await pc.createOffer(); await pc.setLocalDescription(offer); socketRef.current?.emit("renegotiate-offer", {
275-287: LGTM: ReplaceTrack fallback is defensive.The fallback logic that removes and re-adds the video track when
replaceTrackfails is a good defensive approach for handling browser incompatibilities or edge cases. The implementation correctly updates thevideoSenderRefafter adding the new track.
|
@jayanthjamin-web Sorry for late response. I forgot to push the got back then.
video - https://drive.google.com/file/d/1zM7Atiab1yFersqS--tCYcAMHXTtVohZ/view?usp=sharing |
|
@sarthakNITT will check you code |
|
Hey @sarthakNITT, great job on this! 👏 |
|
@sarthakNITT status |
4 participants
🌟 Pre-submission Checklist
#pull-requestchannel to ensure no one else is working on this issue#pull-requestchannelSummary
Type of Changes
Testing Completed
Development Setup Verification
Code Quality
Related Issues
Closes #77
Screenshots/Videos
Send in discord group
Additional Notes
Any additional information or context about the changes.
Summary by CodeRabbit
Bug Fixes
Enhancements