fix: reduce WebSocket log noise and protect connection data #230
fix: reduce WebSocket log noise and protect connection data #230RohitKumar717 wants to merge 2 commits into
Conversation
WalkthroughAdds a new React WebRTC hook for screen sharing, wires it into the root route, and updates WebSocket server logging and connection lifecycle handling (downgraded verbose logs, replaced console.error with logger.error, added close handler and early return for localhost upgrades). Also small package.json edits. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant Hook as useWebRTCProvider
participant PC as RTCPeerConnection
participant WS as Signaling WebSocket
participant Server as Remote Server
User->>Hook: click "Start Sharing"
Hook->>Hook: navigator.mediaDevices.getDisplayMedia
Hook->>PC: create RTCPeerConnection (STUN)
Hook->>PC: add tracks (local stream)
PC->>WS: send ice-candidate (via wsRef)
PC->>PC: createOffer / setLocalDescription
Hook->>WS: send webrtc-offer (SDP)
WS->>Server: forward offer
Server->>WS: send webrtc-answer
WS->>Hook: deliver answer
Hook->>PC: setRemoteDescription (answer)
PC->>PC: establish connection
User->>Hook: click "Stop Sharing"
Hook->>PC: close, stop tracks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/websocket.ts`:
- Line 127: The "Client disconnected" log should be downgraded to debug for
consistency with the recently-changed connect logging; locate the logger call
that emits the literal "Client disconnected" message in src/server/websocket.ts
(the same file where logger.debug(`Client connected from
${request.socket.remoteAddress}`) appears) and change the logger.info (or
logger.warn) invocation to logger.debug so both connect and disconnect events
use the same debug level.
| // Localhost: only store token if it's already known (trusted scan) | ||
| // Remote: token is already validated in the upgrade handler | ||
| logger.info(`Client connected from ${request.socket.remoteAddress}`) | ||
| logger.debug(`Client connected from ${request.socket.remoteAddress}`) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good change, but consider consistency with disconnect logging.
The connection log is correctly downgraded to debug. However, line 364 still logs "Client disconnected" at info level. Since disconnect events fire just as frequently as connect events, consider downgrading that log to debug as well for consistency with the PR's noise-reduction goal.
🔧 Suggested fix for consistency
ws.on("close", () => {
stopMirror()
- logger.info("Client disconnected")
+ logger.debug("Client disconnected")
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` at line 127, The "Client disconnected" log should be
downgraded to debug for consistency with the recently-changed connect logging;
locate the logger call that emits the literal "Client disconnected" message in
src/server/websocket.ts (the same file where logger.debug(`Client connected from
${request.socket.remoteAddress}`) appears) and change the logger.info (or
logger.warn) invocation to logger.debug so both connect and disconnect events
use the same debug level.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/__root.tsx (1)
1-15:⚠️ Potential issue | 🟡 MinorMissing
"use client"directive.This file uses React hooks (
useEffect,useRef), accesses browser APIs (navigator.userAgent,localStorage), and renders interactive components. Per coding guidelines, files with client-side rendering features must include the"use client"directive at the top.🛡️ Proposed fix
+"use client" + import { HeadContent, Link,As per coding guidelines:
**/*.{ts,tsx,js,jsx}: NextJS: Ensure that "use client" is being used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/__root.tsx` around lines 1 - 15, This file uses client-only hooks and browser APIs (useEffect, useRef, navigator.userAgent, localStorage) and must be marked as a client component; add the "use client" directive as the very first line of the file (before any imports) so React treats the module as client-rendered; ensure this is applied to the module that declares useEffect/useRef and imports ConnectionProvider and useWebRTCProvider so the runtime and bundler know to include client-side behavior.
♻️ Duplicate comments (1)
src/server/websocket.ts (1)
366-369: 🛠️ Refactor suggestion | 🟠 MajorDowngrade disconnect log to
debugfor consistency with PR goals.The PR downgrades connection logs to debug level to reduce noise, but both disconnect handlers still use
logger.info. Since disconnect events fire just as frequently as connect events, downgrade this tologger.debugfor consistency.♻️ Proposed fix
ws.on("close", () => { stopMirror() - logger.info("Client disconnected") + logger.debug("Client disconnected") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 366 - 369, The disconnect handler registered on the websocket (ws.on("close", ...)) uses logger.info; change that call to logger.debug to match the PR's reduced-noise logging policy and be consistent with the connect handler; update the closure that calls stopMirror() and logger.info("Client disconnected") so it instead calls logger.debug("Client disconnected") while leaving stopMirror() intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/useWebRTCProvider.ts`:
- Around line 49-51: The catch block in useWebRTCProvider (the try/catch that
currently calls console.error("Failed to start WebRTC capture:", err)) should
use the project's structured logger instead of console.error; replace that call
with logger.error and include the error object and context message, and ensure
the module either imports the shared logger (e.g., import { logger } from '...')
or uses the injected/logger available in the hook (so the error is logged
consistently with other server logs).
- Around line 39-44: The offer send currently calls wsRef.current?.send(...)
unguarded; update the send in useWebRTCProvider (the block that constructs
{type: "webrtc-offer", offer}) to first verify wsRef.current?.readyState ===
WebSocket.OPEN before calling send, and otherwise handle the not-open case
(e.g., queue the offer, retry, or log an error) so the offer won't be silently
dropped.
- Around line 21-23: The track 'ended' event isn't handled so stopping
screen-share via browser leaves isSharing true; update the track setup in
startSharing (where stream.getTracks().forEach and pc.addTrack are used) to
attach a 'ended' listener on each track that calls stopSharing (or the shared
cleanup routine) to update isSharing and remove tracks/peer connections; to
avoid a circular reference, either define stopSharing before startSharing or
extract the cleanup logic into a separate function (e.g., cleanupSharing) and
call that from both stopSharing and the track 'ended' handler.
In `@src/server/websocket.ts`:
- Around line 127-130: Remove the duplicate close event handler that calls
stopMirror() and logs "Client disconnected": locate the earlier ws.on("close",
...) registration (the one that invokes stopMirror() and logger.info("Client
disconnected")) and delete the redundant ws.on("close", ...) block so
stopMirror() and logger.info are only executed once; ensure only the intended
close handler remains and that any necessary cleanup logic is preserved in the
retained handler.
---
Outside diff comments:
In `@src/routes/__root.tsx`:
- Around line 1-15: This file uses client-only hooks and browser APIs
(useEffect, useRef, navigator.userAgent, localStorage) and must be marked as a
client component; add the "use client" directive as the very first line of the
file (before any imports) so React treats the module as client-rendered; ensure
this is applied to the module that declares useEffect/useRef and imports
ConnectionProvider and useWebRTCProvider so the runtime and bundler know to
include client-side behavior.
---
Duplicate comments:
In `@src/server/websocket.ts`:
- Around line 366-369: The disconnect handler registered on the websocket
(ws.on("close", ...)) uses logger.info; change that call to logger.debug to
match the PR's reduced-noise logging policy and be consistent with the connect
handler; update the closure that calls stopMirror() and logger.info("Client
disconnected") so it instead calls logger.debug("Client disconnected") while
leaving stopMirror() intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/hooks/useWebRTCProvider.tssrc/routes/__root.tsxsrc/server/websocket.ts
| stream.getTracks().forEach((track) => { | ||
| pc.addTrack(track, stream) | ||
| }) |
There was a problem hiding this comment.
Handle track ended event to sync isSharing state.
When users stop sharing via the browser's native "Stop sharing" button, the track fires an ended event but isSharing remains true. This causes UI state to become stale.
🛡️ Proposed fix: Add track ended listener
stream.getTracks().forEach((track) => {
pc.addTrack(track, stream)
+ track.onended = () => {
+ stopSharing()
+ }
})Note: You'll need to define stopSharing before startSharing or extract the cleanup logic to avoid the circular reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useWebRTCProvider.ts` around lines 21 - 23, The track 'ended' event
isn't handled so stopping screen-share via browser leaves isSharing true; update
the track setup in startSharing (where stream.getTracks().forEach and
pc.addTrack are used) to attach a 'ended' listener on each track that calls
stopSharing (or the shared cleanup routine) to update isSharing and remove
tracks/peer connections; to avoid a circular reference, either define
stopSharing before startSharing or extract the cleanup logic into a separate
function (e.g., cleanupSharing) and call that from both stopSharing and the
track 'ended' handler.
| wsRef.current?.send( | ||
| JSON.stringify({ | ||
| type: "webrtc-offer", | ||
| offer, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Check WebSocket readyState before sending offer.
Line 26 correctly checks wsRef.current?.readyState === WebSocket.OPEN before sending ICE candidates, but the offer is sent without this check. If the WebSocket is not open, the offer will silently fail to send.
🐛 Proposed fix
- wsRef.current?.send(
- JSON.stringify({
- type: "webrtc-offer",
- offer,
- }),
- )
+ if (wsRef.current?.readyState === WebSocket.OPEN) {
+ wsRef.current.send(
+ JSON.stringify({
+ type: "webrtc-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.
| wsRef.current?.send( | |
| JSON.stringify({ | |
| type: "webrtc-offer", | |
| offer, | |
| }), | |
| ) | |
| if (wsRef.current?.readyState === WebSocket.OPEN) { | |
| wsRef.current.send( | |
| JSON.stringify({ | |
| type: "webrtc-offer", | |
| offer, | |
| }), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useWebRTCProvider.ts` around lines 39 - 44, The offer send
currently calls wsRef.current?.send(...) unguarded; update the send in
useWebRTCProvider (the block that constructs {type: "webrtc-offer", offer}) to
first verify wsRef.current?.readyState === WebSocket.OPEN before calling send,
and otherwise handle the not-open case (e.g., queue the offer, retry, or log an
error) so the offer won't be silently dropped.
| } catch (err) { | ||
| console.error("Failed to start WebRTC capture:", err) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a structured logger instead of console.error.
The server-side code uses logger.error for consistency. Using console.error here works but diverges from the project's logging patterns. This is a minor inconsistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useWebRTCProvider.ts` around lines 49 - 51, The catch block in
useWebRTCProvider (the try/catch that currently calls console.error("Failed to
start WebRTC capture:", err)) should use the project's structured logger instead
of console.error; replace that call with logger.error and include the error
object and context message, and ensure the module either imports the shared
logger (e.g., import { logger } from '...') or uses the injected/logger
available in the hook (so the error is logged consistently with other server
logs).
| ws.on("close", () => { | ||
| stopMirror() | ||
| logger.info("Client disconnected") | ||
| }) |
There was a problem hiding this comment.
Duplicate close event handler will cause double execution.
A second ws.on("close") handler already exists at lines 366-369. Registering this additional handler means stopMirror() runs twice and "Client disconnected" is logged twice on each disconnect. This can cause unexpected behavior if stopMirror() has side effects.
Remove this duplicate handler and keep only the one at lines 366-369.
🐛 Proposed fix: Remove the duplicate handler
- ws.on("close", () => {
- stopMirror()
- logger.info("Client disconnected")
- })
-
- 📝 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.
| ws.on("close", () => { | |
| stopMirror() | |
| logger.info("Client disconnected") | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` around lines 127 - 130, Remove the duplicate close
event handler that calls stopMirror() and logs "Client disconnected": locate the
earlier ws.on("close", ...) registration (the one that invokes stopMirror() and
logger.info("Client disconnected")) and delete the redundant ws.on("close", ...)
block so stopMirror() and logger.info are only executed once; ensure only the
intended close handler remains and that any necessary cleanup logic is preserved
in the retained handler.
Description -
Downgraded high-frequency WebSocket connection logs from 'logger.info' to 'logger.debug' to reduce log noise and protect sensitive connection data. The following logs were changed:
These logs fire on every single connection and cluttered the log output. Moving them to
debuglevel means they only appear when debug logging is explicitly enabled.Related Issue -
Closes #213
Testing -
infolevelChecklist -
Summary by CodeRabbit
New Features
Bug Fixes
Chores