Implement webRTC Transport layer for screen sharing and for Low-Latency Input#351
Open
PinJinx wants to merge 3 commits into
Open
Implement webRTC Transport layer for screen sharing and for Low-Latency Input#351PinJinx wants to merge 3 commits into
PinJinx wants to merge 3 commits into
Conversation
Contributor
WalkthroughArchitectural shift from WebSocket binary frame streaming to native WebRTC peer connections for screen mirroring. Replaces canvas-based consumer rendering and manual frame capture with WebRTC MediaStream binding to video elements, adds server-side signaling routing, and refactors provider/consumer state tracking through connection context and event subscriptions. ChangesWebRTC Media Streaming Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useRemoteConnection.ts (1)
9-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRoute
combothroughsendInputfor consistent transport.
sendCombostill callssend, while the hook’s publicsendis wired tosendInput. InConnectionProvider,sendis WebSocket-only (wsRef.current.send(JSON.stringify(msg))), socombowill take a different path than other inputs.🔧 Route combos through the same input path
const sendCombo = (msg: string[]) => { - send({ + sendInput({ type: "combo", keys: msg, }) }Confirm
sendInputis intended to be the unified path for allVALID_INPUT_TYPES(including"combo"), and that routingcombothrough it matches the server’s expectations.🤖 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 `@src/hooks/useRemoteConnection.ts` around lines 9 - 16, sendCombo currently calls send directly which bypasses the unified path used by sendInput; update sendCombo to call sendInput with a payload matching the other inputs (e.g., sendInput({ type: "combo", keys: msg })) so combos use the same transport and validation as other VALID_INPUT_TYPES, and verify that sendInput and ConnectionProvider (wsRef.current.send(JSON.stringify(...))) accept the "combo" type and that the server expects that shape; adjust any input typing/VALID_INPUT_TYPES if needed to include "combo".src/components/Trackpad/ScreenMirror.tsx (1)
14-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExternalize user-visible strings for internationalization.
The
TEXTSobject contains hardcoded user-facing strings that should be externalized to i18n resource files. As per coding guidelines, user-visible strings should be externalized for internationalization.🌐 Example i18n integration pattern
+import { useTranslation } from 'next-i18next' // or your i18n library -const TEXTS = { - WAITING: "Waiting for screen...", - AUTOMATIC: "Mirroring will start automatically", -} export const ScreenMirror = ({ scrollMode, isTracking, handlers, }: ScreenMirrorProps) => { + const { t } = useTranslation('common') const { status } = useConnection() const videoRef = useRef<HTMLVideoElement>(null) const { hasStream } = useMirrorStream(videoRef, status) return ( <div className="absolute inset-0 flex items-center justify-center bg-black overflow-hidden select-none touch-none"> {/* ... */} {!hasStream && ( <div className="absolute inset-0 flex flex-col items-center justify-center text-gray-400 gap-4"> <div className="loading loading-spinner loading-lg text-primary" /> <div className="text-center px-6"> - <p className="font-semibold text-lg">{TEXTS.WAITING}</p> - <p className="text-sm opacity-60">{TEXTS.AUTOMATIC}</p> + <p className="font-semibold text-lg">{t('mirror.waiting')}</p> + <p className="text-sm opacity-60">{t('mirror.automatic')}</p> </div> </div> )}🤖 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 `@src/components/Trackpad/ScreenMirror.tsx` around lines 14 - 17, TEXTS contains hardcoded user-facing strings; replace it with i18n keys and use the app's translation helper (e.g., the useTranslation hook or t function) instead of TEXTS so strings live in resource files; find the TEXTS constant in ScreenMirror.tsx and: remove the hardcoded entries ("Waiting for screen...", "Mirroring will start automatically"), add corresponding keys to the i18n resources, and update all usages in the ScreenMirror component to call t('your.key.waiting') and t('your.key.automatic') (or the project's equivalent translation accessor).
🤖 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 `@src/components/Trackpad/ScreenMirror.tsx`:
- Around line 31-39: Add playback error handling and accessibility to the video
element in ScreenMirror.tsx: attach an onError handler (e.g., handleVideoError)
to catch stream/playback failures and surface/log the error and update state
(related to videoRef and hasStream), add an onLoadedMetadata handler (e.g.,
handleLoadedMetadata) to confirm successful attachment and set any ready state,
and include an aria-label (e.g., "Screen share video") to improve screen-reader
accessibility; implement the two handler functions in the component to log
errors, update hasStream/ready flags, and perform any cleanup or retry logic as
needed.
In `@src/hooks/useCaptureProvider.ts`:
- Around line 58-86: The code in useCaptureProvider.ts calls (window as
ElectronWindow).electron.showSourcePicker() and passes the returned sourceId
directly into navigator.mediaDevices.getUserMedia; update this by ensuring the
preload uses contextBridge/exposeInMainWorld to expose showSourcePicker (do not
attach to window directly), validate and sanitize the returned sourceId in the
code that handles sourceId (check type, non-empty string, match an expected
pattern or whitelist of known IDs, and reject/ fallback if it fails validation),
and add a defensive branch in the showSourcePicker handling to fall back to
getDisplayMedia when sourceId is invalid; also confirm the app’s CSP allows
media capture. Reference symbols: useCaptureProvider.ts, ElectronWindow,
electron.showSourcePicker, sourceId, navigator.mediaDevices.getUserMedia, and
getDisplayMedia.
- Around line 103-109: The handler handleIncomingInput currently JSON.parses
event.data and blindly calls send; add basic validation after parsing to ensure
the payload is an object with the expected shape (at minimum check that msg is
an object and has a string 'type' property, and any other required fields for
your protocol) and only call send(msg) when validation passes; if validation
fails, log a warning and do not forward to send. Keep the existing try/catch
around JSON.parse to catch parse errors, and perform type checks (e.g., typeof
msg === 'object' && msg !== null && typeof msg.type === 'string') before
invoking send.
- Around line 62-72: The getUserMedia call inside useCaptureProvider (the block
that sets stream using navigator.mediaDevices.getUserMedia with audio/video
mandatory { chromeMediaSource: "desktop", chromeMediaSourceId: sourceId }) must
keep the Electron-specific mandatory constraints intact; remove the broad "as
unknown as MediaStreamConstraints" cast and instead narrow typing locally—either
declare a local interface/type for the Electron constraint shape and cast to
that, or use a targeted `@ts-expect-error` above the specific mandatory fields—so
chromeMediaSource and chromeMediaSourceId remain inside mandatory and TypeScript
is satisfied without altering the constraint structure.
- Line 89: The current assignment stream.getVideoTracks()[0].onended = () =>
stopSharing() can throw if there are no video tracks; update the
useCaptureProvider logic to first get the video tracks into a variable (e.g.,
const videoTracks = stream.getVideoTracks()), check that videoTracks.length > 0
(or that videoTracks[0] is truthy), and only then assign videoTracks[0].onended
= () => stopSharing(); otherwise skip the assignment or attach an equivalent
fallback so stopSharing isn’t referenced on undefined.
- Around line 32-40: The teardown in useCaptureProvider currently closes
pcRef.current and nulls it but leaves useWebRTC's internal state (like
pendingIceCandidatesRef) intact; add a public method on
ConnectionProvider/useWebRTC named closePeerConnection or resetPeerConnection
that closes pcRef.current, clears pendingIceCandidatesRef and any related
refs/state (e.g., senderRef, local streams, pending offers/answers), and call
that new method from stopSharing in useCaptureProvider (instead of directly
setting pcRef.current = null) so all WebRTC state is consistently reset.
In `@src/hooks/useMirrorStream.ts`:
- Around line 42-46: Cleanup currently unsubscribes and sends "stop-mirror" but
doesn't reset hasStream or clear videoRef.current.srcObject, leaving a frozen
frame visible; in useMirrorStream's cleanup (the returned function inside the
effect) also call the state setter that resets stream presence (e.g.,
setHasStream(false)) and clear the video element by pausing it and nulling
videoRef.current.srcObject (and optionally set videoRef.current.load() or style
opacity if you have helpers) so the standby UI reappears; locate the cleanup
closure that calls unsub() and sendRef.current({ type: "stop-mirror" }) and add
these reset steps there.
- Around line 11-12: Add a type-only React import (import type React from
"react") so useMirrorStream's React.RefObject type resolves; in the effect
cleanup that runs when status changes away from "connected" (inside
useMirrorStream) ensure you reset the hasStream state to false and clear any
held media by setting videoRef.current.srcObject = null (or guard before
assigning), and remove or guard the debug console.log for "[RTC] mirror-stream
received" so it doesn't spam production logs; reference the useMirrorStream
function, the hasStream state variable, and videoRef to locate the spots to
change.
In `@src/hooks/useWebRTC.tsx`:
- Around line 131-171: The async handler handleSignalingMessage should be fully
guarded to prevent unhandled promise rejections: wrap the entire body of
handleSignalingMessage (the branches handling "offer" and "answer") in a
try/catch so any awaited calls like pc.setRemoteDescription, pc.createAnswer,
pc.setLocalDescription, pc.addIceCandidate (and any other async ops on
pcRef.current) are caught; on error log/handle it (e.g. console.error or process
logger) and ensure pendingIceCandidatesRef.current is left in a consistent
state; keep the existing guarding for the "ice-candidate" branch and preserve
usage of createPeerConnection, pcRef, pendingIceCandidatesRef, and sendRef when
implementing the try/catch.
In `@src/server/websocket.ts`:
- Around line 166-178: The WebRTC signaling is being broadcast to all other
clients and payloads are being silently dropped due to MAX_PAYLOAD_SIZE, and
consumers are not tracked per provider so "consumer_left" notifications tear
down sessions incorrectly; update src/server/websocket.ts to route
offer/answer/ice-candidate/lan-info to a specific peer/session using an explicit
peer/session id in the message (or a maintained mapping keyed by
providerId/consumerId) instead of iterating all wss.clients in the broadcast
loop, remove or raise the MAX_PAYLOAD_SIZE check for SDP-bearing messages
(handle large SDP by increasing the limit or allowing chunking) and implement a
per-provider consumer registry so stop-mirror and disconnect handling in the
handlers that call notify providers/emit consumer_left only target the
provider(s) that actually host that consumer and only when that provider's
consumer count falls to zero; factor the repeated “notify providers” loops into
a helper (e.g., notifyProvidersForSession) and update logic in the message
switch and connection close handlers to use these new routing and lifecycle
helpers.
- Around line 218-242: The start/stop mirror handlers and the ws.on("close")
disconnect path must only notify providers when the consumer count transitions
0->1 (consumer_joined) or 1->0 (consumer_left); update the logic around
startMirror(), stopMirror(), and the close handler to compute the number of
active consumers (e.g., scan wss.clients for (client as ExtWebSocket).isConsumer
&& client.readyState === WebSocket.OPEN) and only broadcast consumer_joined if
count_before === 0 and only broadcast consumer_left if count_after === 0 (for
stop-mirror compute count_after excluding the stopping ws, and in close handler
set (ws as ExtWebSocket).isConsumer = false then compute remaining consumers
before sending); factor this into a small helper like
hasConsumers()/countConsumers() and use that in startMirror, stopMirror, and the
ws.on("close") path so providers (useCaptureProvider) only react on first/last
consumer transitions.
---
Outside diff comments:
In `@src/components/Trackpad/ScreenMirror.tsx`:
- Around line 14-17: TEXTS contains hardcoded user-facing strings; replace it
with i18n keys and use the app's translation helper (e.g., the useTranslation
hook or t function) instead of TEXTS so strings live in resource files; find the
TEXTS constant in ScreenMirror.tsx and: remove the hardcoded entries ("Waiting
for screen...", "Mirroring will start automatically"), add corresponding keys to
the i18n resources, and update all usages in the ScreenMirror component to call
t('your.key.waiting') and t('your.key.automatic') (or the project's equivalent
translation accessor).
In `@src/hooks/useRemoteConnection.ts`:
- Around line 9-16: sendCombo currently calls send directly which bypasses the
unified path used by sendInput; update sendCombo to call sendInput with a
payload matching the other inputs (e.g., sendInput({ type: "combo", keys: msg
})) so combos use the same transport and validation as other VALID_INPUT_TYPES,
and verify that sendInput and ConnectionProvider
(wsRef.current.send(JSON.stringify(...))) accept the "combo" type and that the
server expects that shape; adjust any input typing/VALID_INPUT_TYPES if needed
to include "combo".
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 12c04cb0-aca0-485f-bf83-3a2b579c466c
📒 Files selected for processing (8)
src/components/Trackpad/ScreenMirror.tsxsrc/contexts/ConnectionProvider.tsxsrc/hooks/useCaptureProvider.tssrc/hooks/useMirrorStream.tssrc/hooks/useRemoteConnection.tssrc/hooks/useWebRTC.tsxsrc/routes/__root.tsxsrc/server/websocket.ts
Comment on lines
+58
to
+86
| const electron = (window as unknown as ElectronWindow).electron | ||
| if (electron?.showSourcePicker) { | ||
| const sourceId = await electron.showSourcePicker() | ||
| if (sourceId) { | ||
| stream = await navigator.mediaDevices.getUserMedia({ | ||
| audio: { | ||
| mandatory: { chromeMediaSource: "desktop" }, | ||
| }, | ||
| video: { | ||
| mandatory: { | ||
| chromeMediaSource: "desktop", | ||
| chromeMediaSourceId: sourceId, | ||
| }, | ||
| }, | ||
| } as unknown as MediaStreamConstraints) | ||
| } else { | ||
| // Fallback if dialog dismissed with no ID | ||
| stream = await navigator.mediaDevices.getDisplayMedia({ | ||
| video: true, | ||
| audio: true, | ||
| }) | ||
| } | ||
| } else { | ||
| // ── Standard Browser Fallback Flow ── | ||
| stream = await navigator.mediaDevices.getDisplayMedia({ | ||
| video: true, | ||
| audio: true, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Verify Electron security: validate sourceId and consider CSP implications.
The code accesses window.electron.showSourcePicker() and uses the returned sourceId directly in getUserMedia constraints without validation. Ensure:
- The Electron preload script properly exposes
showSourcePickerviacontextBridge(not directly attaching towindow). - The
sourceIdis validated/sanitized if it could be influenced by untrusted sources. - Content Security Policy allows media capture.
Verify the Electron integration follows security best practices:
#!/bin/bash
# Check if Electron contextBridge is used for IPC exposure
fd -e js -e ts preload --exec rg -l 'contextBridge\|exposeInMainWorld'
# Check for direct window assignments (insecure pattern)
rg -n 'window\.electron\s*=' --type ts --type js🤖 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 `@src/hooks/useCaptureProvider.ts` around lines 58 - 86, The code in
useCaptureProvider.ts calls (window as
ElectronWindow).electron.showSourcePicker() and passes the returned sourceId
directly into navigator.mediaDevices.getUserMedia; update this by ensuring the
preload uses contextBridge/exposeInMainWorld to expose showSourcePicker (do not
attach to window directly), validate and sanitize the returned sourceId in the
code that handles sourceId (check type, non-empty string, match an expected
pattern or whitelist of known IDs, and reject/ fallback if it fails validation),
and add a defensive branch in the showSourcePicker handling to fall back to
getDisplayMedia when sourceId is invalid; also confirm the app’s CSP allows
media capture. Reference symbols: useCaptureProvider.ts, ElectronWindow,
electron.showSourcePicker, sourceId, navigator.mediaDevices.getUserMedia, and
getDisplayMedia.
Comment on lines
+166
to
+178
| if ( | ||
| msg.type === "offer" || | ||
| msg.type === "answer" || | ||
| msg.type === "ice-candidate" || | ||
| msg.type === "lan-info" | ||
| ) { | ||
| for (const client of wss.clients) { | ||
| if (client !== ws && client.readyState === WebSocket.OPEN) { | ||
| client.send(JSON.stringify(msg)) | ||
| } | ||
| } | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WebRTC signaling routing for one provider with multiple consumers best practices
💡 Result:
For a one-provider, multiple-consumer (one-to-many) architecture in WebRTC, best practices depend heavily on the scale of the audience. Attempting to use a pure peer-to-peer (P2P) mesh—where the provider maintains a direct connection to every consumer—is generally not recommended for more than a few participants because it causes exponential growth in the provider's upstream bandwidth and CPU usage [1][2][3][4]. For robust, scalable applications, the following architectures and signaling practices are recommended: 1. Use a Media Server (SFU/MCU): Instead of direct P2P connections, the provider should push a single stream to a Selective Forwarding Unit (SFU) or Multipoint Control Unit (MCU) [1][2]. The media server then distributes that stream to all consumers [2][5]. This offloads the heavy lifting of encoding and distribution from the provider's client to the infrastructure [2][6]. 2. Signaling Best Practices: The signaling server acts as a matchmaker to exchange SDP offers/answers and ICE candidates [7][8][9]. While WebRTC does not dictate a signaling protocol, common practices include: * Transport: Use WebSockets, gRPC, or Socket.IO to maintain a persistent, bidirectional channel between clients and the signaling server [7][5][10][11]. * Pub/Sub Integration: For systems requiring scale, use a pub/sub message broker (e.g., Redis or NATS) to allow signaling nodes to communicate across a cluster [5]. * Trickle ICE: Always implement Trickle ICE to reduce call setup latency. This allows ICE candidates to be sent to the signaling server and forwarded to the remote peer as they are discovered, rather than waiting for the entire gathering process to complete [1][6][9]. * Separation of Concerns: Keep the signaling server thin. It should only handle metadata (SDP, ICE, room state, and participant discovery) and not handle actual media traffic [7][12][13]. 3. Modern Ingest/Egress Protocols: Rather than building custom signaling logic for every use case, adopt standardized protocols: * WHIP (WebRTC-HTTP Ingest Protocol): Use this for pushing the live stream from the provider to a media server or CDN [2][14]. * WHEP (WebRTC-HTTP Egress Protocol): Use this for pulling the stream from the server to consumer clients [2]. 4. Scaling Massive Broadcasts: If you need to support thousands of concurrent consumers, implement SFU Cascading. In this model, the origin SFU forwards the provider's stream to multiple regional edge SFUs, which then serve local clusters of consumers [5]. This horizontal scaling prevents any single server from becoming a bandwidth bottleneck [5]. Always ensure you have properly configured STUN/TURN servers to facilitate connections for clients behind symmetric NATs or restrictive firewalls, as these are mandatory for high-reliability production environments [7][8][6][4].
Citations:
- 1: https://web.dev/articles/webrtc-infrastructure
- 2: https://antmedia.io/webrtc-signaling-servers-everything-you-need-to-know/
- 3: https://www.reddit.com/r/javascript/comments/4b64xo/is_webrtc_viable_for_connecting_tons_of_users/
- 4: https://raghu-vijaykumar.github.io/docs/docs/system-design/examples/entertainment-streaming/video-conferencing/
- 5: https://www.hirevoipdeveloper.com/blog/how-to-architect-webrtc-systems-for-10k-concurrent-users/
- 6: https://dev.to/dishant_sethi/building-production-voice-ai-agents-latency-architecture-and-what-nobody-tells-you-3jhj
- 7: https://simplified.media/guides/webrtc-multiplayer
- 8: https://www.digitalsamba.com/blog/what-is-webrtc-signalling
- 9: https://www.youtube.com/watch?v=Kn_3uHaKz7Q
- 10: https://dev.to/snowlyg/webrtc-go-grpc-call-architecture-for-constrained-lan-environments-l91
- 11: https://medium.com/@codewithkm64/build-a-one-to-many-webrtc-broadcast-app-with-node-js-and-socket-io-23a5f4721253
- 12: https://dev.to/ebendttl/reconciling-p2p-collaborative-states-via-webrtc-data-channels-d5g
- 13: https://dev.to/avinash_s_karanth/p2p-ai-distributed-peer-to-peer-ai-inference-network-f38
- 14: https://webrtchacks.com/webrtc-plumbing-with-gstreamer/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify file and print relevant sections with line numbers
FILE="src/server/websocket.ts"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "=== File header / imports (first 120 lines) ==="
nl -ba "$FILE" | sed -n '1,120p'
echo "=== Signaling routing block (120-260) ==="
nl -ba "$FILE" | sed -n '120,260p'
echo "=== Search for MAX_PAYLOAD_SIZE definition and usage ==="
rg -n "MAX_PAYLOAD_SIZE" "$FILE" || true
echo "=== Search for msg.type switch/handlers ==="
rg -n "msg\.type|case\s+['\"]|switch\s*\(msg\.type\)" "$FILE"
echo "=== Inspect around other lifecycle sections referenced (260-450) ==="
nl -ba "$FILE" | sed -n '260,450p'Repository: AOSSIE-Org/Rein
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm file exists
ls -la src/server/websocket.ts
FILE="src/server/websocket.ts"
echo "=== Line numbers 1-140 (imports / types / constants) ==="
cat -n "$FILE" | sed -n '1,140p'
echo "=== Line numbers 140-240 (signaling routing block) ==="
cat -n "$FILE" | sed -n '140,240p'
echo "=== MAX_PAYLOAD_SIZE occurrences ==="
rg -n "MAX_PAYLOAD_SIZE" "$FILE" || true
echo "=== msg.type handling (some surrounding context) ==="
rg -n "msg\.type" "$FILE"
# Print broader region around lifecycle referenced in the review comment
echo "=== Line numbers 240-320 (continue handlers) ==="
cat -n "$FILE" | sed -n '240,320p'
echo "=== Line numbers 320-430 (lifecycle/close path) ==="
cat -n "$FILE" | sed -n '320,430p'
# If JSON payload size check uses other names, search for "payload" checks
echo "=== Search for payload size / length checks ==="
rg -n "payload|length|byteLength|MAX_" "$FILE" || trueRepository: AOSSIE-Org/Rein
Length of output: 15123
Fix WebRTC signaling routing and consumer lifecycle handling in src/server/websocket.ts
offer/answer/ice-candidate/lan-infoare broadcast to every other open client without routing by role/session (lines 166-178), so multiple provider/consumer connections will mix SDP/ICE across sessions and break negotiation.- Messages are hard-capped at 10KB (
MAX_PAYLOAD_SIZE, lines 63 and 151-154); WebRTC SDP can exceed this (especially with simulcast/non-trickle), causing silent drops. consumer_leftis sent to all providers on anystop-mirrorand any consumer disconnect (lines 231-240 and 387-397) with no per-consumer tracking/counting, so providers can tear down even while other consumers remain.
Route signaling by explicit peer/session IDs (or a maintained mapping) and track consumers per session/provider; also factor the repeated “notify providers” loops.
🤖 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 `@src/server/websocket.ts` around lines 166 - 178, The WebRTC signaling is
being broadcast to all other clients and payloads are being silently dropped due
to MAX_PAYLOAD_SIZE, and consumers are not tracked per provider so
"consumer_left" notifications tear down sessions incorrectly; update
src/server/websocket.ts to route offer/answer/ice-candidate/lan-info to a
specific peer/session using an explicit peer/session id in the message (or a
maintained mapping keyed by providerId/consumerId) instead of iterating all
wss.clients in the broadcast loop, remove or raise the MAX_PAYLOAD_SIZE check
for SDP-bearing messages (handle large SDP by increasing the limit or allowing
chunking) and implement a per-provider consumer registry so stop-mirror and
disconnect handling in the handlers that call notify providers/emit
consumer_left only target the provider(s) that actually host that consumer and
only when that provider's consumer count falls to zero; factor the repeated
“notify providers” loops into a helper (e.g., notifyProvidersForSession) and
update logic in the message switch and connection close handlers to use these
new routing and lifecycle helpers.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/websocket.ts (1)
254-263:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
getConsumerCount()for consistency instart-providerhandler.The
hasConsumercheck on lines 257-259 doesn't verifyreadyState === WebSocket.OPEN, inconsistent with thegetConsumerCount()helper used elsewhere. A consumer in a closing state would still triggerconsumer_joined.🔧 Proposed fix
if (msg.type === "start-provider") { ;(ws as ExtWebSocket).isProvider = true logger.info("Client registered as Screen Provider") - const hasConsumer = Array.from(wss.clients).some( - (c) => (c as ExtWebSocket).isConsumer, - ) - if (hasConsumer) { + if (getConsumerCount() > 0) { ws.send(JSON.stringify({ type: "consumer_joined" })) } return }🤖 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 `@src/server/websocket.ts` around lines 254 - 263, Replace the ad-hoc consumer check in the "start-provider" message handler with the existing getConsumerCount() helper to ensure only OPEN consumers are counted: in the block handling msg.type === "start-provider" (where (ws as ExtWebSocket).isProvider is set and you currently compute hasConsumer from wss.clients), call getConsumerCount() and use its result to decide whether to send ws.send({ type: "consumer_joined" }); this keeps behavior consistent with other code paths and respects readyState filtering implemented by getConsumerCount().src/components/Trackpad/ScreenMirror.tsx (1)
14-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExternalize user-visible strings for internationalization.
The
TEXTSobject contains user-facing strings that should be moved to i18n resource files to support localization.As per coding guidelines: "User-visible strings should be externalized to resource files (i18n)".
🤖 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 `@src/components/Trackpad/ScreenMirror.tsx` around lines 14 - 17, The TEXTS constant in ScreenMirror.tsx contains user-facing strings (TEXTS.WAITING and TEXTS.AUTOMATIC) that must be moved into the i18n resource files and loaded via the app's localization API; update ScreenMirror to replace direct references to TEXTS with calls to the i18n helper (e.g., t('screenMirror.waiting') and t('screenMirror.automatic')), add corresponding keys to the locale resource files, and remove or repurpose the TEXTS object so no hard-coded UI strings remain in ScreenMirror.tsx.src/hooks/useCaptureProvider.ts (1)
139-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd bounds check before accessing video track for addTrack.
Line 109 defensively uses optional chaining (
?.), but line 139 directly accessesstream.getVideoTracks()[0]without a guard. If the stream has no video tracks (edge case with audio-only capture),addTrack(undefined, stream)will throw.🛡️ Proposed defensive check
- const track = stream.getVideoTracks()[0] + const track = stream.getVideoTracks()[0] + if (!track) { + console.warn("[Capture] No video track available") + stopSharing() + return + } const sender = pcRef.current.addTrack(track, stream)🤖 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 `@src/hooks/useCaptureProvider.ts` around lines 139 - 141, The code assumes a video track exists when calling stream.getVideoTracks()[0] before pcRef.current.addTrack; add a defensive check in the useCaptureProvider flow to safely handle audio-only streams by retrieving the first video track via optional chaining or checking stream.getVideoTracks().length > 0, only calling pcRef.current.addTrack(track, stream) and assigning senderRef.current when track is non-null, and ensure any previous sender is appropriately handled/removed if absent.
🤖 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.
Outside diff comments:
In `@src/components/Trackpad/ScreenMirror.tsx`:
- Around line 14-17: The TEXTS constant in ScreenMirror.tsx contains user-facing
strings (TEXTS.WAITING and TEXTS.AUTOMATIC) that must be moved into the i18n
resource files and loaded via the app's localization API; update ScreenMirror to
replace direct references to TEXTS with calls to the i18n helper (e.g.,
t('screenMirror.waiting') and t('screenMirror.automatic')), add corresponding
keys to the locale resource files, and remove or repurpose the TEXTS object so
no hard-coded UI strings remain in ScreenMirror.tsx.
In `@src/hooks/useCaptureProvider.ts`:
- Around line 139-141: The code assumes a video track exists when calling
stream.getVideoTracks()[0] before pcRef.current.addTrack; add a defensive check
in the useCaptureProvider flow to safely handle audio-only streams by retrieving
the first video track via optional chaining or checking
stream.getVideoTracks().length > 0, only calling pcRef.current.addTrack(track,
stream) and assigning senderRef.current when track is non-null, and ensure any
previous sender is appropriately handled/removed if absent.
In `@src/server/websocket.ts`:
- Around line 254-263: Replace the ad-hoc consumer check in the "start-provider"
message handler with the existing getConsumerCount() helper to ensure only OPEN
consumers are counted: in the block handling msg.type === "start-provider"
(where (ws as ExtWebSocket).isProvider is set and you currently compute
hasConsumer from wss.clients), call getConsumerCount() and use its result to
decide whether to send ws.send({ type: "consumer_joined" }); this keeps behavior
consistent with other code paths and respects readyState filtering implemented
by getConsumerCount().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2293427-1125-4b36-8218-34bc33d80c37
📒 Files selected for processing (6)
src/components/Trackpad/ScreenMirror.tsxsrc/contexts/ConnectionProvider.tsxsrc/hooks/useCaptureProvider.tssrc/hooks/useMirrorStream.tssrc/hooks/useWebRTC.tsxsrc/server/websocket.ts
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Trackpad/ScreenMirror.tsx (1)
14-17: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUser-visible strings should be externalized for internationalization.
The
TEXTSobject contains hardcoded English strings displayed in the UI. Per coding guidelines, user-visible strings should be externalized to resource files (i18n).🤖 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 `@src/components/Trackpad/ScreenMirror.tsx` around lines 14 - 17, The TEXTS constant in the ScreenMirror component contains hardcoded English UI strings; replace these with i18n resource lookups by moving "Waiting for screen..." and "Mirroring will start automatically" into your translation files and changing TEXTS to use the i18n API (e.g., t('trackpad.waiting') / t('trackpad.automatic')) or directly call the translation function inside ScreenMirror; update any imports to include the i18n hook/utility used in the project (e.g., useTranslation or i18n.t) and ensure keys like trackpad.waiting and trackpad.automatic are added to the locale files for all supported languages.
🤖 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 `@README.md`:
- Line 10: Revise the README intro sentence that begins "A cross-platform,
remote desktop..." to correct grammar and improve clarity: change "non touch" to
"non-touch", "bring" to "brings", tighten phrasing around the KISS
parenthetical, and split or reword the long sentence into two concise sentences
that state (1) the project purpose—allowing touch and non-touch devices to act
as a trackpad/keyboard via a local web interface—and (2) potential use cases
(cloud PC/cloud gaming standardization and improved STT for Linux via phone).
Ensure punctuation and capitalization are fixed and the sentence reads smoothly.
---
Outside diff comments:
In `@src/components/Trackpad/ScreenMirror.tsx`:
- Around line 14-17: The TEXTS constant in the ScreenMirror component contains
hardcoded English UI strings; replace these with i18n resource lookups by moving
"Waiting for screen..." and "Mirroring will start automatically" into your
translation files and changing TEXTS to use the i18n API (e.g.,
t('trackpad.waiting') / t('trackpad.automatic')) or directly call the
translation function inside ScreenMirror; update any imports to include the i18n
hook/utility used in the project (e.g., useTranslation or i18n.t) and ensure
keys like trackpad.waiting and trackpad.automatic are added to the locale files
for all supported languages.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 38237750-34d8-4a62-8b11-089877c74e24
📒 Files selected for processing (7)
README.mdsrc/components/Trackpad/ScreenMirror.tsxsrc/contexts/ConnectionProvider.tsxsrc/hooks/useCaptureProvider.tssrc/hooks/useMirrorStream.tssrc/hooks/useWebRTC.tsxsrc/server/websocket.ts
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.
Addressed Issues:
Fixes #208
Description
This PR fundamentally overhauls the WebRTC and WebSocket transport architecture to ensure stable, ultra-low-latency connections. (Note: WebSockets are currently retained as a signaling fallback but are slated to be replaced by HTTP in a future update).
Key Changes:
State-Driven Screen Mirroring: Introduces a "Consumer-First" WebRTC architecture model. The Desktop host now actively listens for lifecycle events (
consumer_joined/consumer_left) to dynamically initialize and tear down the screen sharing stream.Dual-Channel Input Routing: Formalized the transport layer to eliminate network bottlenecking. High-frequency inputs (
move,scroll,zoom,touch) are now routed through an unordered, UDP-like DataChannel (dc-unordered) to prevent head-of-line blocking. Discrete inputs (click,key) are routed through a reliable, TCP-like ordered DataChannel (dc-ordered).Graceful Transport Fallback: Added a catch wrapper to the input router that seamlessly falls back to WebSocket signaling if the WebRTC DataChannels drop, fail, or are still negotiating.
Screenshots/Recordings:
nil
Functional Verification
Screen Mirror
Authentication
Basic Gestures
One-finger tap: Verified as Left Click.
Two-finger tap: Verified as Right Click.
Click and drag: Verified selection behavior.
Pinch to zoom: Verified zoom functionality (if applicable).
Modes & Settings
Cursor mode: Cursor moves smoothly and accurately.
Scroll mode: Page scrolls as expected.
Sensitivity: Verified changes in cursor speed/sensitivity settings.
Copy and Paste: Verified both Copy and Paste functionality.
Invert Scrolling: Verified scroll direction toggles correctly.
Advanced Input
Key combinations: Verified "hold" behavior for modifiers (e.g., Ctrl+C) and held keys are shown in buffer.
Keyboard input: Verified Space, Backspace, and Enter keys work correctly.
Glide typing: Verified path drawing and text output.
Voice input: Verified speech-to-text functionality for full sentences.
Backspace doesn't send the previous input.
Any other gesture or input behavior introduced:
Additional Notes:
Checklist
My PR addresses a single issue, fixes a single bug or makes a single improvement.
My code follows the project's code style and conventions
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
If applicable, I have made corresponding changes or additions to the documentation
If applicable, I have made corresponding changes or additions to tests
My changes generate no new warnings or errors
I have joined the and I will share a link to this PR with the project maintainers there
I have read the
Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
Incase of UI change I've added a demo video.
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Reliability
Documentation