[feat] Refactor WebRTC Pipeline and Add Debug Dashboard - #370
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThe PR replaces SSE and dual-peer WebRTC signaling with a WebSocket-based flow, adds RTP-over-UDP media delivery, introduces server and client diagnostics, adds a ChangesWebRTC transport and debugging
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
PinJinx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
🤖 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/contexts/ConnectionProvider.tsx`:
- Around line 96-105: Update the pong handling in ConnectionProvider’s
parsed.type === "pong" branch to stop posting latency on every heartbeat; route
the measurement through the existing DataChannel/WebSocket, or only report it
when an authenticated debug consumer is active and throttle reports to roughly
once every 10 seconds. Preserve setLatency(ms) for local latency state.
In `@src/contexts/DebugContext.tsx`:
- Around line 77-106: Update the console interception useEffect in DebugContext
so it only patches methods in development or when the debug route is active,
avoiding production overhead. Preserve the original console methods separately
from the bound call targets, and restore the actual originals during cleanup so
unmount/remount cycles cannot stack wrappers.
- Around line 68-71: Update DebugProvider’s context value to use useMemo so its
identity remains stable unless the exposed state or callbacks change, and ensure
addLog does not force whole-tree renders for every intercepted console call by
storing log entries in a ref with a throttled state flush (or
useSyncExternalStore). Apply the same optimization to the corresponding context
value at the additional location.
- Around line 42-49: Update serialize so converting non-string arguments cannot
throw for circular structures, DOM/React objects, Error values, or BigInt. Wrap
the JSON.stringify path with safe fallback handling that always produces a
useful string, preserving the existing message/details partition and ensuring
patched console.error continues logging instead of propagating serialization
errors.
In `@src/hooks/useWebRtcStream.ts`:
- Around line 43-52: Update the max-retry failure branch in useWebRtcStream to
pass both user-visible failure strings through the existing t() translation
helper instead of literals, using the errorComponent category keys. Add the
corresponding English entries to i18n.en.errorComponent and preserve the current
displayed text as their values.
- Around line 186-217: Update the ws.onmessage handler to parse signaling data
into an explicitly typed, validated discriminated union for offer, ice, and
error messages. Replace unchecked msg.sdp, msg.candidate, msg.errorType, and
msg.message accesses with fields narrowed by the message type, rejecting
malformed payloads before calling setRemoteDescription, addIceCandidate, or
rendering error state. Avoid any throughout the parsing and validation flow.
- Around line 127-129: The WebRTC WebSocket connection created in
useWebRtcStream must authenticate using its existing token before /ws accepts
the upgrade. Update the client handshake to send token credentials through a
supported mechanism, and update the corresponding /ws upgrade handling to
validate and reject missing or invalid tokens before establishing the socket.
- Around line 54-65: In src/hooks/useWebRtcStream.ts#L54-L65 and
src/hooks/useWebRtcStream.ts#L93-L104, add a shared teardown() helper that
detaches all listed WebSocket and RTCPeerConnection handlers before closing and
clearing each ref. Replace the duplicated close logic in triggerRetry and
reconnect with teardown(), and reuse the same helper in the effect cleanup.
In `@src/routes/__root.tsx`:
- Around line 32-34: Change the DebugProvider integration in RootComponent so
console interception and log buffering are scoped to the /debug route rather
than wrapping the entire application. Preserve the debug dashboard’s access to
captured logs while preventing unrelated routes, including useWebRtcStream
logging, from triggering app-wide state updates or retaining logs when the
dashboard is not active.
In `@src/routes/debug.tsx`:
- Around line 80-87: Update fetchServerState and the corresponding fetch flow
around line 118 to capture non-OK responses and network errors instead of
silently swallowing them. Add and render an error state in the debug dashboard
so failed debug endpoint requests display a clear explanation rather than only
showing the default stopped state.
- Around line 476-492: In the session button rendered in the debug route, remove
the explicit tabIndex and redundant onKeyDown handler, since the native button
already handles keyboard activation. Extract the existing selection and
search-query toggle logic into a shared handler and reuse it from onClick.
- Around line 247-670: Externalize every user-visible dashboard string in the
rendered JSX, including status labels, panel headings, empty states,
placeholders, buttons, log labels, and tooltip text, through the shared i18n
resource used by the debug route. Update the relevant i18n resource entries and
replace literals throughout the main dashboard component, preserving dynamic
values and formatting while supporting translated strings.
- Around line 166-167: The logEndRef and copied-state indicator in the debug log
view are unused; either implement the intended behavior or remove both dead
state paths. Prefer wiring logEndRef to scroll the log viewport to the newest
entry and rendering copied-state feedback driven by copiedId after
copyToClipboard, or remove logEndRef, copiedId, and their associated updates if
these behaviors are not needed.
- Around line 134-161: Update the telemetry useEffect around setInterval so it
is created once rather than recreated when serverState.sessions or latency
changes. Store the latest selectedSessionId, sessions, and latency values in a
ref (or equivalent stable latest-state mechanism), have the interval callback
read from that ref, and retain cleanup on unmount while preserving the existing
sampling and history behavior.
- Around line 299-304: Remove the redundant nested ternary in the selected
session label within the debug route. Simplify the expression so a truthy
selectedSessionId renders itself and the falsy case renders "all sessions"; do
not retain the unreachable "Invalid Id" branch.
In `@src/server/gstreamer/gstManager.ts`:
- Around line 42-46: Extract the RTP sink host and port into shared constants
and update the GStreamer pipeline near "udpsink" to use them. Reuse those same
constants in the UDP bind within the webRTC server, replacing the duplicated
"127.0.0.1" and 5004 literals while preserving the existing bind and sink
behavior.
- Around line 129-135: Update GstManager.stop() to retain the process reference
after sending SIGTERM, wait briefly for graceful termination, and escalate to
SIGKILL if it remains alive before clearing this.process. Ensure cleanup and
subsequent start() cannot leave or overlap with an unresponsive GStreamer
pipeline.
- Around line 52-70: Update GstManager.start() so initialization errors are
rethrown after cleanup, allowing its callers to avoid reporting hostStatus as
"running" when startup fails. Also update the asynchronous pipeline spawn error
handling in the relevant process error callback to record an internal failed
state and ensure subsequent status checks reflect that no pipeline is running.
In `@src/server/server.ts`:
- Around line 262-282: The GET handler for /api/debug/logs needs a periodic SSE
keep-alive. In the block that registers the response in sseClients, start a
comment ping interval, clear it when the request closes, and remove the client;
ensure the interval is also cleared on request errors so no timer or stale
client remains.
- Around line 181-189: Await and handle the promise returned by
GstManager.stop() at both affected sites in src/server/server.ts:181-189 and
src/server/server.ts:296-299. In the /api/host/stop handler, update hostStatus
and send the response only after teardown settles, while logging failures; make
stopServer async and await/catch both webrtcManager.shutdown() and
gstManager.stop() so shutdown completes without racing provider disposal.
- Around line 58-70: The LAN IPv4 discovery logic is duplicated between
getPrimaryIp and getLanIp. In src/server/server.ts lines 58-70, remove
getPrimaryIp and import/use the shared helper; in src/server/welcome.ts lines
5-15, export getLanIp from a shared module such as src/server/net.ts and update
both consumers to use that single implementation.
- Around line 235-282: Protect all three `/api/debug/*`
handlers—`/api/debug/sessions`, `/api/debug/report-latency`, and
`/api/debug/logs`—with the existing `requireAuth` mechanism before processing
requests. Ensure authentication accepts the established `?token=` query form so
the EventSource logs client can authenticate, while preserving the current
endpoint behavior for authorized callers.
- Around line 72-87: Update parseJsonBody to enforce a maximum request-body size
while receiving data, tracking accumulated bytes before appending each chunk.
When the limit is exceeded, destroy the request, reject with an appropriate
error, and stop processing further chunks; preserve the existing JSON parsing
and request-error behavior for bodies within the limit.
In `@src/server/webRTC.ts`:
- Around line 292-298: Update the WebRTC server constructor and shutdown() to
retain the bound HTTP upgrade handler registered on the server, then remove that
exact handler with server.off("upgrade", handler) during shutdown before or
alongside closing resources. Ensure subsequent /ws upgrades no longer reach the
closed WebSocketServer.
- Around line 270-284: Update getSessions to derive hasInputConnection from the
actual input data-channel readyState, preserving false when the channel is
absent or not open. Replace the magic WebSocket readyState value in
sseViewerCount with WebSocket.OPEN, and use the appropriate client data-channel
symbol visible in the surrounding implementation.
- Around line 92-110: Update getInitialConfig so server-config.json is resolved
relative to the module rather than process.cwd(), using the same module-relative
approach as src/utils/logger.ts. Preserve the existing parsing, validation,
warning, and fallback behavior while ensuring configuration loading works from
any working directory.
- Around line 71-74: Update the UDP socket error handler in the WebRTC manager
so an error does not leave future sessions using a permanently closed socket:
recreate and rebind the socket with backoff, or propagate the failure into
hostStatus for /api/debug/sessions. Preserve the existing error logging while
ensuring the manager’s media pipeline recovers or clearly reports its unusable
state.
- Around line 206-223: Update the ICE state subscription in the WebRTC
connection setup to call cleanupClient only for failed or closed states,
allowing disconnected sessions to recover. In the onIceCandidate subscription,
guard ws.send with ws.readyState === WebSocket.OPEN, matching the existing
InputHandler error path.
- Around line 244-252: Remove the raw SDP console.log call from the offer
creation flow around pc.createOffer and pc.setLocalDescription, while preserving
the existing ws.send offer handling, error logging, and cleanup behavior.
- Around line 169-193: Update handleDataMessage to receive or otherwise identify
the originating data channel, then send each ping’s pong only through that
channel and preserve accurate error classification for send failures. Correct
the session.bytesSent accounting so inbound raw message bytes are not recorded
as outbound “Input Sent” telemetry; use the appropriate receive-byte field or
rename the metric consistently with the dashboard.
- Around line 47-64: Authenticate `/ws` upgrades before calling
`this.wss.handleUpgrade`: extract the client token from the agreed query
parameter or `Sec-WebSocket-Protocol` header, validate it with `isKnownToken`
from `tokenStore`, and destroy the socket when validation fails. Preserve the
existing URL routing and WebSocket connection flow for valid tokens.
- Around line 113-115: Replace the Math.random-based session ID generation in
the wss connection handler with crypto.randomUUID(), importing randomUUID from
node:crypto. Preserve using the generated ID for sessionCreatedAt and subsequent
client tracking.
In `@src/server/welcome.ts`:
- Around line 43-44: Update the welcome output around the status and port rows
to use localized strings from i18n.<locale>.server, adding the corresponding
Status, Running, and Port entries beside localLabel, networkLabel, and
debugLabel. Render both rows through the existing row() helper so their padding
matches the other labels, while preserving the current values.
In `@src/utils/logger.ts`:
- Around line 22-31: Update the verboseLogs initialization in the module-level
config-loading block to resolve server-config.json using an ESM-safe
import.meta.url/parentURL-based path rather than __dirname, ensuring it works
after bundling. Replace the empty catch with a direct stderr fallback that
reports the read or parse failure before retaining the default false value.
In `@vite.config.ts`:
- Around line 22-41: Extract the duplicated HTTP listening and welcome-message
logic into a shared helper, such as wireServer, defined above the plugin object.
Update both configureServer and configurePreviewServer to call the helper after
attachSignalingRoutes, preserving the existing port resolution and printWelcome
behavior.
🪄 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 Plus
Run ID: cccdd928-d667-4f0f-b425-5362496aa490
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
biome.jsonpackage.jsonsrc/components/Trackpad/ScreenMirror.tsxsrc/contexts/ConnectionProvider.tsxsrc/contexts/DebugContext.tsxsrc/hooks/useWebRtcStream.tssrc/routeTree.gen.tssrc/routes/__root.tsxsrc/routes/debug.tsxsrc/routes/trackpad.tsxsrc/server-config.jsonsrc/server/api/InputPeerConnection.tssrc/server/api/apiHandlers.tssrc/server/api/apiState.tssrc/server/api/getLocalIp.tssrc/server/drivers/linux/index.tssrc/server/drivers/linux/structs.tssrc/server/drivers/mac/structs.tssrc/server/drivers/windows/structs.tssrc/server/gstreamer/gstManager.tssrc/server/gstreamer/hostRunner.tssrc/server/server.tssrc/server/webRTC.tssrc/server/welcome.tssrc/utils/i18n.tssrc/utils/logger.tsvite.config.ts
💤 Files with no reviewable changes (5)
- src/server/gstreamer/hostRunner.ts
- src/server/api/InputPeerConnection.ts
- src/server/api/getLocalIp.ts
- src/server/api/apiState.ts
- src/server/api/apiHandlers.ts
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 (3)
src/server/webRTC.ts (1)
252-258: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
ws.sendin the ICE-candidate subscriber with a try/catch.Unlike the offer-send path (Lines 279-288) which wraps
ws.sendin try/catch, this callback can throw synchronously (e.g. socket mid-close) with nothing to catch it, since it runs inside a werift event dispatch, not application code.🛡️ Proposed fix
pc.onIceCandidate.subscribe((candidate) => { - if (candidate && ws.readyState === WebSocket.OPEN) { - ws.send( - JSON.stringify({ type: "ice", candidate: candidate.toJSON() }), - ) - } + try { + if (candidate && ws.readyState === WebSocket.OPEN) { + ws.send( + JSON.stringify({ type: "ice", candidate: candidate.toJSON() }), + ) + } + } catch (e) { + logger.error(`Failed to send ICE candidate: ${String(e)}`) + } })🤖 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/webRTC.ts` around lines 252 - 258, Wrap the ws.send call in the pc.onIceCandidate subscriber with a try/catch, matching the existing offer-send handling. Keep the current candidate and WebSocket.OPEN guards, and handle synchronous send failures within the callback so they do not escape werift event dispatch.src/server/server.ts (1)
117-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyfor the Vite server parameter.
attachSignalingRoutes(server: any)loses type safety for both call sites (configureServer/configurePreviewServerinvite.config.ts). Consider a minimal structural type capturing what's actually used (httpServer, optionalmiddlewares) instead ofany.interface SignalingServerLike { httpServer?: import("node:http").Server | null middlewares?: { use: (fn: (req: IncomingMessage, res: ServerResponse, next?: () => void) => void) => void } }As per path instructions, "TypeScript: Avoid 'any', use explicit types".
🤖 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/server.ts` around lines 117 - 122, Replace the any parameter type on attachSignalingRoutes with a minimal structural SignalingServerLike type covering the httpServer and optional middlewares members used by the signaling route setup. Add the necessary Node HTTP type imports, and ensure the configureServer/configurePreviewServer call sites in vite.config.ts remain assignable without changing their behavior.Source: Path instructions
src/server/gstreamer/gstManager.ts (1)
53-72: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftConcurrent
start()/stop()calls can spawn overlapping pipelines.
start()'s only guard isif (this.process) return, butthis.processisn't set until after twoawaits (provider.initialize,getGStreamerSource). A secondstart()call arriving during that window (e.g. a/api/host/startPOST racing the module's own startupstart()inserver.ts) bypasses the guard and spawns a second capture provider/pipeline. Separately,stop()nullsthis.processsynchronously and resolves oncecleanup()finishes, without waiting for the SIGTERM/SIGKILL escalation to actually terminate the child — an immediatestart()right afterstop()can therefore spawn a newgst-launchprocess while the old one is still exiting, and both compete for the same RTP UDP port.🔒 Proposed fix
export class GstManager { private process: ChildProcess | null = null private provider: CaptureProvider | null = null + private starting = false public async start(): Promise<void> { - if (this.process) return + if (this.process || this.starting) return + this.starting = true logger.info("Spawning GStreamer UDP engine") try { this.provider = createCaptureProvider() ... this.executePipeline(pipelineArgs) } catch (error) { logger.error(`Capture initialization failed: ${String(error)}`) await this.cleanup() throw error + } finally { + this.starting = false } } public async stop(): Promise<void> { if (this.process) { logger.info("Terminating GStreamer video pipeline") const proc = this.process this.process = null proc.kill("SIGTERM") - const killTimer = setTimeout(() => { - if (proc.exitCode === null) { - logger.warn("GStreamer process did not exit on SIGTERM, sending SIGKILL") - proc.kill("SIGKILL") - } - }, 2000) - proc.once("close", () => clearTimeout(killTimer)) + await new Promise<void>((resolve) => { + const killTimer = setTimeout(() => { + if (proc.exitCode === null) { + logger.warn("GStreamer process did not exit on SIGTERM, sending SIGKILL") + proc.kill("SIGKILL") + } + }, 2000) + proc.once("close", () => { + clearTimeout(killTimer) + resolve() + }) + }) } await this.cleanup() }Also applies to: 131-148
🤖 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/gstreamer/gstManager.ts` around lines 53 - 72, Serialize GstManager.start() and stop() operations so concurrent starts share or await one in-progress startup, and a new start cannot begin until the prior stop has completed child-process termination. Replace the process-only guard in start() with a lifecycle synchronization mechanism covering provider initialization, pipeline creation, cleanup, and SIGTERM/SIGKILL escalation; update stop() and cleanup() to participate in that same sequencing while preserving idempotent calls.
♻️ Duplicate comments (2)
src/hooks/useWebRtcStream.ts (1)
43-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStill hardcoded failure strings (prior comment unresolved).
"Connection Failed"and the retry-exhausted message remain literals, even thoughi18n.en.errorComponentalready exists insrc/utils/i18n.tsfor this purpose.🌐 Proposed i18n wiring
- setErrorHandle("Connection Failed") - setError("Failed to establish stream session after multiple attempts") + setErrorHandle(t("errorComponent", "connectionFailedTitle")) + setError(t("errorComponent", "connectionFailedBody"))Add the corresponding keys to
i18n.en.errorComponent.As per path instructions, "Internationalization: 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/hooks/useWebRtcStream.ts` around lines 43 - 52, Replace the hardcoded user-facing strings in the retry-exhaustion branch of useWebRtcStream with corresponding entries from i18n.en.errorComponent, adding those resource keys if they do not exist. Keep the existing retry state updates and failure behavior unchanged, and use the localized values for both setErrorHandle and setError.Source: Path instructions
src/server/webRTC.ts (1)
98-104: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUDP socket death is still unrecoverable (unresolved from prior review).
On any UDP error, the socket is closed with no rebind path; every future viewer will negotiate successfully but receive no RTP until the process restarts. This was flagged previously and remains unaddressed in this diff.
🤖 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/webRTC.ts` around lines 98 - 104, Update setupUdpSocket so UDP socket errors trigger recovery rather than only closing the socket: clean up the failed socket, create and bind a replacement socket, and reattach the required handlers so subsequent viewers can receive RTP without restarting the process. Preserve the existing error logging and avoid rebinding multiple times for the same failure.
🤖 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 `@CodeRabbittemp`:
- Around line 1-3: Remove the committed CodeRabbittemp artifact entirely from
the repository; do not modify the surrounding ConnectionProvider logic or
replace the artifact with application code.
In `@src/contexts/ConnectionProvider.tsx`:
- Around line 105-109: Update the latency report fetch in ConnectionProvider to
reuse the existing bearer-token authentication mechanism, then inspect the
response and handle non-OK results instead of silently swallowing unauthorized
failures. Preserve the current POST payload while ensuring
/api/debug/report-latency receives the client’s authentication token and res.ok
is checked.
In `@src/contexts/DebugContext.tsx`:
- Around line 47-50: Update serialize() so the JSON.stringify(a) result falls
back to String(a) when it is undefined, while preserving the existing catch-path
behavior for thrown serialization errors.
In `@src/server/webRTC.ts`:
- Around line 72-82: Extract the loopback address comparison from the WebRTC
authentication block into a shared isLoopbackAddress(addr) helper, then reuse it
in both this request handling path and the /api/auth/token/requireAuth logic in
server.ts. Preserve recognition of 127.0.0.1, ::1, and ::ffff:127.0.0.1 while
ensuring both authentication bypass paths use the same helper.
---
Outside diff comments:
In `@src/server/gstreamer/gstManager.ts`:
- Around line 53-72: Serialize GstManager.start() and stop() operations so
concurrent starts share or await one in-progress startup, and a new start cannot
begin until the prior stop has completed child-process termination. Replace the
process-only guard in start() with a lifecycle synchronization mechanism
covering provider initialization, pipeline creation, cleanup, and
SIGTERM/SIGKILL escalation; update stop() and cleanup() to participate in that
same sequencing while preserving idempotent calls.
In `@src/server/server.ts`:
- Around line 117-122: Replace the any parameter type on attachSignalingRoutes
with a minimal structural SignalingServerLike type covering the httpServer and
optional middlewares members used by the signaling route setup. Add the
necessary Node HTTP type imports, and ensure the
configureServer/configurePreviewServer call sites in vite.config.ts remain
assignable without changing their behavior.
In `@src/server/webRTC.ts`:
- Around line 252-258: Wrap the ws.send call in the pc.onIceCandidate subscriber
with a try/catch, matching the existing offer-send handling. Keep the current
candidate and WebSocket.OPEN guards, and handle synchronous send failures within
the callback so they do not escape werift event dispatch.
---
Duplicate comments:
In `@src/hooks/useWebRtcStream.ts`:
- Around line 43-52: Replace the hardcoded user-facing strings in the
retry-exhaustion branch of useWebRtcStream with corresponding entries from
i18n.en.errorComponent, adding those resource keys if they do not exist. Keep
the existing retry state updates and failure behavior unchanged, and use the
localized values for both setErrorHandle and setError.
In `@src/server/webRTC.ts`:
- Around line 98-104: Update setupUdpSocket so UDP socket errors trigger
recovery rather than only closing the socket: clean up the failed socket, create
and bind a replacement socket, and reattach the required handlers so subsequent
viewers can receive RTP without restarting the process. Preserve the existing
error logging and avoid rebinding multiple times for the same failure.
🪄 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 Plus
Run ID: 64fa169f-90a2-4912-8768-70aa79b5cb08
📒 Files selected for processing (14)
CodeRabbittempsrc/contexts/ConnectionProvider.tsxsrc/contexts/DebugContext.tsxsrc/hooks/useWebRtcStream.tssrc/routes/debug.tsxsrc/server/constants.tssrc/server/gstreamer/gstManager.tssrc/server/net.tssrc/server/server.tssrc/server/webRTC.tssrc/server/welcome.tssrc/utils/i18n.tssrc/utils/logger.tsvite.config.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/server/server.ts (1)
116-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyfor the server parameter.
attachSignalingRoutes(server: any)bypasses type checking onserver.httpServer/server.middlewaresused throughout this function. A minimal structural interface covering the vite-dev-server and plain-http.Servershapes actually used here would preserve the same flexibility with type safety.As per path instructions, "TypeScript: Avoid 'any', use explicit types."
🤖 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/server.ts` around lines 116 - 121, Replace the any parameter type in attachSignalingRoutes with a minimal structural interface that represents the supported Vite dev-server and plain http.Server shapes, including the httpServer and middlewares members accessed throughout the function. Preserve compatibility with both server forms while allowing TypeScript to validate those property accesses.Source: Path instructions
src/server/gstreamer/gstManager.ts (1)
96-105: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUnexpected pipeline exit is not surfaced to
hostStatus.Neither the spawn
"error"handler nor the"close"handler notify any external caller; they only log andcleanup().attachSignalingRoutes/handleApiRequestinsrc/server/server.tsonly sethostStatus = "running"/"error"from the initialstart()call's.then()/.catch()— there's no callback for a later crash. After an unexpected exit,hostStatusstays"running"forever (no auto-restart either), so/api/debug/sessionsand the/debugdashboard report a healthy host with no actual video pipeline.Consider accepting an optional exit callback in
GstManager's constructor/start()(similar to theonErrorcallback already used forprovider.initialize) soserver.tscan react to unplanned terminations.Also applies to: 114-129
🤖 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/gstreamer/gstManager.ts` around lines 96 - 105, Update GstManager’s process lifecycle handling in its constructor/start flow so unexpected spawn errors and close events invoke an optional exit callback after cleanup, while preserving the existing logging and nulling behavior. Wire this callback through attachSignalingRoutes/handleApiRequest in server.ts to set hostStatus to "error" when the pipeline terminates unexpectedly, distinguishing planned shutdowns from unplanned exits and keeping the initial start promise handling unchanged.src/utils/logger.ts (1)
58-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep terminal logs enabled by default in dev.
verboseLogsis shipped asfalse, so the current logger only writes tolog.txtunless the flag is manually enabled. Keep the defaulttrueso local execution preserves visible terminal output.🤖 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/utils/logger.ts` around lines 58 - 59, Update the verboseLogs default used by the logger in src/utils/logger.ts to true, ensuring terminal output remains enabled during local development when no explicit configuration is provided. Preserve the existing conditional logging behavior for explicitly configured values.src/hooks/useWebRtcStream.ts (2)
93-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMemoize
reconnectfor referential stability.Unlike
triggerRetry/handleNetworkFailure,reconnectis a plain function recreated on every render, yet it's exposed in the hook's return value to consumers (e.g. for retry buttons). If any consumer places it in auseEffect/useCallbackdependency array or memoized prop, this causes unnecessary re-runs/re-renders.♻️ Proposed fix
- const reconnect = () => { + const reconnect = useCallback(() => { if (retryTimerRef.current) { clearTimeout(retryTimerRef.current) retryTimerRef.current = null } ... retryCountRef.current = 0 setReconnectAttempt((prev) => prev + 1) - } + }, [])Also applies to: 314-322
🤖 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/useWebRtcStream.ts` around lines 93 - 126, Memoize the reconnect function with useCallback so its reference remains stable across renders when exposed from the hook’s return value. Update its dependency list to include every referenced value that can change, while preserving the existing cleanup and state-reset behavior in reconnect.Source: Coding guidelines
234-239: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExternalize the remaining hardcoded host-error strings.
"Host Error"and"Host reported an error"bypasst(), unlike the sibling max-retry error strings fixed just above (lines 47-48). These are user-visible strings rendered viaerrorHandle/error.As per path instructions, "User-visible strings should be externalized to resource files (i18n)."
🌐 Proposed fix
} else if (msg.type === "error") { console.error("[WebRTC] Host error received:", msg) setConnecting(false) - setErrorHandle(msg.errorType || "Host Error") - setError(msg.message || "Host reported an error") + setErrorHandle(msg.errorType || t("errorComponent", "hostErrorTitle")) + setError(msg.message || t("errorComponent", "hostErrorBody")) }🤖 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/useWebRtcStream.ts` around lines 234 - 239, Update the host-error branch in useWebRtcStream’s WebRTC message handler to replace the fallback strings passed to setErrorHandle and setError with the corresponding t() i18n lookups, matching the existing localized max-retry error handling and preserving the current message/error fallback behavior.Source: Path instructions
♻️ Duplicate comments (2)
src/server/webRTC.ts (2)
98-104: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUDP socket error still permanently kills media for all future sessions (unresolved).
On any UDP error, the socket is closed with no rebind/backoff and no propagation into
hostStatus, so every subsequent viewer negotiates successfully but receives no RTP with no way to observe the failure via/api/debug/sessions.🤖 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/webRTC.ts` around lines 98 - 104, Update setupUdpSocket and its UDP error handling to recover from socket failures by scheduling a controlled rebind with backoff instead of permanently leaving the socket closed. Propagate the failure and recovery state into hostStatus so /api/debug/sessions exposes the condition, while preserving protection against repeated concurrent rebind attempts.
72-77: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLoopback check still hand-rolled instead of reusing the new
isLoopbackAddresshelper.
src/utils/net.tsnow exportsisLoopbackAddress(), andsrc/server/server.ts'srequireAuthalready uses it, but this auth-bypass check here still duplicates the same three-address comparison inline. Since this gates WebSocket upgrade auth, any future drift between the two copies (e.g. missing an address form) creates an auth inconsistency — exactly the risk flagged previously.♻️ Proposed fix
+import { isLoopbackAddress } from "../utils/net" ... if (url.pathname === "/ws") { const addr = request.socket.remoteAddress - const isLocal = - addr === "127.0.0.1" || - addr === "::1" || - addr === "::ffff:127.0.0.1" + const isLocal = isLoopbackAddress(addr)🤖 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/webRTC.ts` around lines 72 - 77, Update the loopback determination in the WebSocket authentication flow around the isLocal variable to call the existing isLoopbackAddress helper from net utilities instead of comparing address strings inline. Preserve the existing token and auth-bypass behavior while ensuring this check uses the shared implementation.
🤖 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/server/gstreamer/gstManager.ts`:
- Around line 131-148: Update stop() to await the GStreamer process’s "close"
event, including the existing SIGKILL timeout fallback, before setting
this.process to null and calling cleanup(). Ensure cleanup() releases
this.provider only after actual process termination, so a subsequent start()
cannot launch an overlapping pipeline.
In `@src/server/webRTC.ts`:
- Around line 331-341: Update shutdown() and/or the cleanupClient flow to
explicitly close each established client WebSocket via client.ws.close() while
iterating through this.clients, ensuring existing connections terminate during
shutdown while preserving the current peer-connection and input-handler cleanup.
In `@src/utils/logger.ts`:
- Around line 72-80: Extract the pure serialize helper from logger.ts into a
dependency-free shared module, preserving its handling of strings, Error values,
JSON serialization, and fallback conversion. Update logger.ts and
DebugContext.tsx to import and reuse that shared serialize symbol, removing the
duplicated client-side implementation.
---
Outside diff comments:
In `@src/hooks/useWebRtcStream.ts`:
- Around line 93-126: Memoize the reconnect function with useCallback so its
reference remains stable across renders when exposed from the hook’s return
value. Update its dependency list to include every referenced value that can
change, while preserving the existing cleanup and state-reset behavior in
reconnect.
- Around line 234-239: Update the host-error branch in useWebRtcStream’s WebRTC
message handler to replace the fallback strings passed to setErrorHandle and
setError with the corresponding t() i18n lookups, matching the existing
localized max-retry error handling and preserving the current message/error
fallback behavior.
In `@src/server/gstreamer/gstManager.ts`:
- Around line 96-105: Update GstManager’s process lifecycle handling in its
constructor/start flow so unexpected spawn errors and close events invoke an
optional exit callback after cleanup, while preserving the existing logging and
nulling behavior. Wire this callback through
attachSignalingRoutes/handleApiRequest in server.ts to set hostStatus to "error"
when the pipeline terminates unexpectedly, distinguishing planned shutdowns from
unplanned exits and keeping the initial start promise handling unchanged.
In `@src/server/server.ts`:
- Around line 116-121: Replace the any parameter type in attachSignalingRoutes
with a minimal structural interface that represents the supported Vite
dev-server and plain http.Server shapes, including the httpServer and
middlewares members accessed throughout the function. Preserve compatibility
with both server forms while allowing TypeScript to validate those property
accesses.
In `@src/utils/logger.ts`:
- Around line 58-59: Update the verboseLogs default used by the logger in
src/utils/logger.ts to true, ensuring terminal output remains enabled during
local development when no explicit configuration is provided. Preserve the
existing conditional logging behavior for explicitly configured values.
---
Duplicate comments:
In `@src/server/webRTC.ts`:
- Around line 98-104: Update setupUdpSocket and its UDP error handling to
recover from socket failures by scheduling a controlled rebind with backoff
instead of permanently leaving the socket closed. Propagate the failure and
recovery state into hostStatus so /api/debug/sessions exposes the condition,
while preserving protection against repeated concurrent rebind attempts.
- Around line 72-77: Update the loopback determination in the WebSocket
authentication flow around the isLocal variable to call the existing
isLoopbackAddress helper from net utilities instead of comparing address strings
inline. Preserve the existing token and auth-bypass behavior while ensuring this
check uses the shared implementation.
🪄 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 Plus
Run ID: 2bd95eb2-fc6c-41ef-a68f-749d6ab214a6
📒 Files selected for processing (13)
src/contexts/ConnectionProvider.tsxsrc/contexts/DebugContext.tsxsrc/hooks/useWebRtcStream.tssrc/routes/debug.tsxsrc/server/constants.tssrc/server/gstreamer/gstManager.tssrc/server/server.tssrc/server/webRTC.tssrc/server/welcome.tssrc/utils/i18n.tssrc/utils/logger.tssrc/utils/net.tsvite.config.ts
2 participants
Addressed Issues:
N/A
Description
This PR introduces a major refactor of Rein's screen mirroring and communication architecture by replacing the previous implementation with a new WebRTC-based pipeline powered by werift.
Changes
webrtc-based architecture.WebRTCManagerto handle peer connections, media streaming, and input DataChannels./debugpage for monitoring client and server state.DebugProvider.Screenshots/Recordings:
Functional Verification
Screen Mirror
Authentication
Basic Gestures
One-finger tap: Verified as Left Click.
Two-finger tap: Verified as Right Click.
[ x] 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:
This PR is primarily an architectural refactor. It removes the previous signaling implementation and introduces a new WebRTC-based streaming pipeline together with a debugging interface to simplify development and troubleshooting.
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.
In case 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