Screen mirror via web api#179
Closed
Rozerxshashank wants to merge 5 commits into
Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a ScreenMirror UI and capture/decoding hooks with WebSocket relay changes; removes the left‑click control; improves IME/composition-aware input handling and typing message types; tightens token validation and centralized logging; wires createWsServer into Vite. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Mobile User
participant Client as ScreenMirror Client
participant WS as WebSocket Server
participant Provider as Desktop Provider
User->>Client: Request Start Sharing
Client->>Provider: getDisplayMedia (useCaptureProvider)
Provider-->>Client: MediaStream
rect rgba(100, 150, 255, 0.5)
loop Frame loop
Client->>Client: Capture & downscale canvas
Client->>Client: JPEG encode (quality ~0.3)
Client->>WS: Send binary frame
WS->>WS: Relay to consumers
WS->>Client: Deliver frame blob to clients
Client->>Client: decode ImageBitmap
Client->>Client: render to canvas (requestAnimationFrame)
end
end
User->>Client: Stop Sharing
Client->>WS: send stop-provider / stop-mirror
Provider->>Provider: Stop stream & cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (2 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 |
d2a948c to
2c6b104
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/server/InputHandler.ts (1)
1-267:⚠️ Potential issue | 🟡 MinorBiome formatting is currently blocking CI for this file.
Please run Biome formatting and commit the result; CI already reports formatter drift here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 1 - 267, Run the Biome formatter on this file and commit the updated formatting so CI passes; specifically format and stage the file that contains the exported interface InputMessage and the class InputHandler (including its handleMessage method and supporting helpers isFiniteNumber/clamp), then run your tests/CI locally and commit the formatted file with only formatting changes.src/hooks/useTrackpadGesture.ts (1)
1-279:⚠️ Potential issue | 🟡 MinorBiome formatting is currently blocking CI for this file.
Please apply Biome formatting and commit the updated output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useTrackpadGesture.ts` around lines 1 - 279, The file fails Biome formatting; run the Biome formatter on this file (e.g., biome format) and commit the resulting changes so CI is no longer blocked. Specifically format the exported hook and its functions (useTrackpadGesture, handleTouchStart, handleTouchMove, handleTouchEnd, processMovement, etc.) to match project Biome rules, then stage and commit the formatted file and push the commit to unblock CI.src/server/websocket.ts (1)
226-308:⚠️ Potential issue | 🔴 CriticalRestrict
update-configto localhost; remote clients can reconfigure the server.Current code allows any authenticated remote client to change host/port/address, which is a high-impact security risk.
💡 Proposed fix
if (msg.type === "update-config") { + if (!isLocal) { + logger.warn("Remote client attempted to update server config"); + ws.send( + JSON.stringify({ + type: "config-updated", + success: false, + error: "Only localhost can update server config", + }), + ); + return; + } + try { if ( !msg.config ||🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 226 - 308, Before handling the msg.type === "update-config" block, gate the operation to only allow local connections by checking the websocket's remote address (use ws._socket.remoteAddress) and reject any non-loopback addresses (accept 127.0.0.1, ::1 and IPv4-mapped ::ffff:127.0.0.1 forms); if the check fails, log the attempt via logger.warn and ws.send a config-updated failure and return. Keep the rest of the validation (SERVER_CONFIG_KEYS, port checks, configPath, fs writes, and error handling) unchanged, but ensure the remote-address check happens at the top of the update-config branch so remote clients cannot reach the file write logic.src/routes/settings.tsx (1)
304-337:⚠️ Potential issue | 🟠 MajorInput validation accepts partial numeric strings and redirect ignores server confirmation.
The
Number.parseInt()call accepts partial numeric input (e.g.,"3000abc"parses to3000) and the code redirects after a fixed 1000ms timeout regardless of server response. The server sendsconfig-updatedmessages with success/error status that should be awaited before redirecting.Replace the port validation to use a stricter regex check and replace the fixed timeout with a
socket.onmessagehandler that waits for server confirmation before redirecting.Suggested approach
-const port = Number.parseInt(frontendPort, 10); -if (!Number.isFinite(port) || port < 1 || port > 65535) { +const normalizedPort = frontendPort.trim(); +if (!/^\d+$/.test(normalizedPort)) { + alert("Please enter a valid port number (1–65535)."); + return; +} +const port = Number(normalizedPort); +if (port < 1 || port > 65535) { alert("Please enter a valid port number (1–65535)."); return; } @@ socket.onopen = () => { socket.send( JSON.stringify({ type: "update-config", config: { frontendPort: port, }, }), ); }; + +socket.onmessage = (event) => { + try { + const data = JSON.parse(event.data); + if (data.type !== "config-updated") return; + + if (!data.success) { + alert(data.error || "Failed to update configuration."); + socket.close(); + return; + } + + socket.close(); + const newProtocol = window.location.protocol; + const newHostname = window.location.hostname; + const newUrl = `${newProtocol}//${newHostname}:${port}/settings`; + window.location.href = newUrl; + } catch { + alert("Unexpected server response."); + socket.close(); + } +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 304 - 337, The current port parsing (frontendPort -> port) uses Number.parseInt which accepts trailing characters and the client blindly redirects after a 1000ms setTimeout instead of waiting for server confirmation; update the validation to use a strict regex (e.g., /^\d+$/) on frontendPort and then parse and range-check the result (1–65535), open the WebSocket to wsUrl as before, remove the setTimeout-based redirect, and implement socket.onmessage to listen for the server's "config-updated" response (check the message.type === "config-updated" and its success/error payload) and only close the socket and navigate to the new settings URL when a successful confirmation is received (handle error responses by showing an alert and not redirecting).vite.config.ts (1)
1-10:⚠️ Potential issue | 🟡 MinorFix Biome formatting to pass CI.
The pipeline indicates a formatting mismatch. Run
npx biome format --write vite.config.tsor your project's format script to fix the formatting issues and pass CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 1 - 10, Run the project's formatter (e.g., npx biome format --write vite.config.ts or your repo's format script) on the file containing the top-level imports and createWsServer (the block with imports like "node:url", "@tanstack/devtools-vite", "vite-react", "nitro/vite", "vite-tsconfig-paths" and the createWsServer import) to fix Biome formatting mismatches so CI will pass; after formatting, re-run lint/CI and commit the formatted vite.config.ts changes.
🤖 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/components/Trackpad/ControlBar.tsx`:
- Line 1: The import line in ControlBar.tsx (import type { ModifierState } from
"@/types";) does not match the project's formatter expectations; run the
repository's format script (e.g., npm run format or yarn format) or apply the
Biome/Prettier configuration to reformat src/components/Trackpad/ControlBar.tsx
so the import and surrounding file match CI style rules, then stage the updated
file and push the commit.
- Around line 61-62: The Copy and Paste buttons in the ControlBar component are
missing click handlers; add two handler functions (e.g., handleCopy and
handlePaste) inside the ControlBar component and attach them to the respective
buttons' onClick props (replace the bare <button> for "Copy" and "Paste" with
buttons that call handleCopy and handlePaste). Implement handleCopy to obtain
the text to copy (from selectedTrackpadText prop/state or a passed-in value) and
call navigator.clipboard.writeText(text) with try/catch to log or surface
errors; implement handlePaste to call navigator.clipboard.readText() and then
dispatch the pasted text to the component (via a callback prop like onPaste or
updating local state), also handling errors. Add a feature-gate: if
navigator.clipboard is undefined, disable the buttons (disabled attribute) and
add a short TODO comment referencing Clipboard API fallback or issue tracking.
In `@src/components/Trackpad/ScreenMirror.tsx`:
- Line 1: The file ScreenMirror.tsx fails Biome formatting; run the project's
format script (e.g., npm run format or yarn format) and reformat
src/components/Trackpad/ScreenMirror.tsx so the import statement and the entire
file match the project's Biome/Prettier rules; after formatting, stage the
updated file and push the changes to resolve the CI formatting error.
- Around line 45-52: Add a biome-ignore a11y lint suppression before the touch
surface div in the ScreenMirror component to document the intentional lack of
keyboard accessibility for this touch-only gesture area; place a single-line
comment (e.g. // biome-ignore-next-line jsx-a11y/no-static-element-interactions)
immediately above the <div> that uses handleStart, handlers.onTouchMove,
handlers.onTouchEnd and handlePreventFocus so the linter ignores the
non-interactive element touch handlers similarly to TouchArea.
In `@src/hooks/useCaptureProvider.ts`:
- Around line 125-127: The catch block in useCaptureProvider.ts currently only
resets isBusy.current and swallows errors; update that catch (err) to log/report
the error (including err.message and err.stack) and contextual info (e.g.,
whether the failure occurred during drawImage or toBlob) — use the project's
logger or console.error (or a telemetry helper if available) and still ensure
isBusy.current is reset (or move reset into a finally block) so failures are
visible for debugging.
- Around line 130-167: Wrap the startSharing function in useCallback to keep a
stable reference when the hook re-renders; update its declaration to
useCallback(...) and include any external dependencies (streamRef, videoRef,
wsRef, workerRef, isSharingRef, setIsSharing) in the dependency array, and
ensure stopSharing is accessible (define stopSharing before startSharing or read
it via a ref to avoid a circular dependency) so the onended handler and
stopSharing call still work.
In `@src/hooks/useMirrorStream.ts`:
- Around line 72-95: The dependency array incorrectly includes the ref object
wsRef (a RefObject), which won't trigger the effect when the underlying
WebSocket instance changes; update the effect to depend on the actual values
that should trigger re-subscription by removing wsRef from the deps and keep
[status, handleMessage] (use wsRef.current inside the effect as already done);
if the linter complains about missing deps, add a focused eslint-disable comment
for react-hooks/exhaustive-deps above this useEffect or explicitly justify the
omission in a short comment, but do not add wsRef itself to the dependency array
— reference symbols: useEffect block in useMirrorStream.ts, wsRef.current,
status, handleMessage, rAFRef, frameRef.
In `@src/hooks/useRemoteConnection.ts`:
- Around line 1-2: Reorder the import statements at the top of
useRemoteConnection.ts to match Biome's organize-imports ordering: group and
sort external/react imports (useCallback, useEffect, useRef, useState) before
local/project imports, and place the type import InputMessage after or within
the local imports as Biome expects; then run Biome/organize-imports to verify
formatting so CI passes.
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 257-260: In useTrackpadGesture, the pending draggingTimeout
(draggingTimeout.current) can fire after the component unmounts and dispatch a
stale click-up; add cleanup in the hook’s teardown to
clearTimeout(draggingTimeout.current) and set draggingTimeout.current = null (or
undefined) so handleDraggingTimeout cannot run after unmount. Locate the timeout
usage (draggingTimeout, handleDraggingTimeout, TOUCH_TIMEOUT) inside the
useTrackpadGesture hook and add a return cleanup (or appropriate effect cleanup)
that calls clearTimeout for draggingTimeout.current and nulls it out.
In `@src/routes/settings.tsx`:
- Around line 306-317: Replace hardcoded alert strings in this settings.tsx
block (the port validation alert and the WebSocket error alert in the port
validation + WebSocket creation flow, including the socket.onerror handler) with
i18n resource lookups (e.g., useTranslation()/t('settings.invalidPort') and
t('settings.wsConnectFailed')). Import and use the project's translation
hook/utility at the top of the component, add appropriate keys to the i18n
resource files, and call the translation function instead of alert("...") so
messages are externalized.
- Around line 73-112: The WebSocket token generation effect (useEffect) runs on
non-local hosts; restrict it to localhost by short-circuiting early if
window.location.hostname is not a local host (e.g., not "localhost" or
"127.0.0.1") so the WebSocket creation (wsUrl, new WebSocket, socket.onopen send
of "generate-token", socket.onmessage handling, setAuthToken and
localStorage.setItem("rein_auth_token")) only runs locally; ensure the same
early-return prevents socket creation and cleanup for non-local environments
while keeping the existing socket.close() logic for the local case.
In `@src/routes/trackpad.tsx`:
- Around line 69-73: The current loop in trackpad.tsx that sends individual
backspace key events (using deletions, prevData, commonLen and send({ type:
"key", key: "backspace" })) can be slow for large deletions; change it to batch
deletions into a single message when possible (e.g., send({ type: "delete", key:
"backspace", count: deletions }) or send({ type: "delete", count: deletions }))
and update the receiving/handling code to accept a count, falling back to the
existing per-key loop only if the protocol/receiver doesn't support a count;
locate and modify the code paths around send, deletions, prevData and commonLen
to implement this optimization.
- Around line 257-259: The onBlur handler currently force-refocuses the hidden
input via setTimeout(() => hiddenInputRef.current?.focus(), 10), which can trap
keyboard focus; remove this automatic refocus and rely on explicit focus flows
already provided by handleContainerClick and onInputFocus, or change the onBlur
to only refocus when the blur event indicates the user is still interacting with
the trackpad (e.g., detect pointer/touch state or event.relatedTarget within the
trackpad area) rather than always calling hiddenInputRef.current.focus(); update
the onBlur in the component that uses hiddenInputRef to implement one of these
two approaches and ensure focus management is tied to explicit user interaction
handlers like handleContainerClick/onInputFocus.
In `@src/server/InputHandler.ts`:
- Around line 89-91: The log message in the scroll-throttle branch is
misleading: when calling this.handleMessage(pending).catch(...) you should
change the logger.error call to reference the correct event type (e.g., "pending
scroll event") and keep the error details (err) and any context (the pending
object) in the formatted log; update the logger.error invocation near
handleMessage and pending to a clear message like "Error processing pending
scroll event: %O" and include the err (and optionally pending) so diagnostics
reflect the actual branch.
In `@src/server/websocket.ts`:
- Line 114: The "connected" message sent via ws.send(JSON.stringify({ type:
"connected", serverIp: LAN_IP })) is missing the platform field that
useRemoteConnection.ts expects; update the payload to include the server
platform (e.g., process.platform or os.platform()) as a "platform" property
alongside serverIp so clients populate platform state when receiving type
"connected".
- Around line 132-145: The relay loop in websocket message handling (the
isBinary branch where (ws as ExtWebSocket).isProvider is checked) currently
forwards binary frames to all consumers without size checks; add a maximum
allowed binary payload size constant (e.g., MAX_BINARY_FRAME_BYTES) and validate
data.byteLength or Buffer.byteLength before fanout, and if it exceeds the limit
either drop the frame and log a warning via the existing logger or close the
provider connection with an explanatory close code; ensure the size check
happens before iterating wss.clients so oversized frames are never sent to
consumers.
---
Outside diff comments:
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 1-279: The file fails Biome formatting; run the Biome formatter on
this file (e.g., biome format) and commit the resulting changes so CI is no
longer blocked. Specifically format the exported hook and its functions
(useTrackpadGesture, handleTouchStart, handleTouchMove, handleTouchEnd,
processMovement, etc.) to match project Biome rules, then stage and commit the
formatted file and push the commit to unblock CI.
In `@src/routes/settings.tsx`:
- Around line 304-337: The current port parsing (frontendPort -> port) uses
Number.parseInt which accepts trailing characters and the client blindly
redirects after a 1000ms setTimeout instead of waiting for server confirmation;
update the validation to use a strict regex (e.g., /^\d+$/) on frontendPort and
then parse and range-check the result (1–65535), open the WebSocket to wsUrl as
before, remove the setTimeout-based redirect, and implement socket.onmessage to
listen for the server's "config-updated" response (check the message.type ===
"config-updated" and its success/error payload) and only close the socket and
navigate to the new settings URL when a successful confirmation is received
(handle error responses by showing an alert and not redirecting).
In `@src/server/InputHandler.ts`:
- Around line 1-267: Run the Biome formatter on this file and commit the updated
formatting so CI passes; specifically format and stage the file that contains
the exported interface InputMessage and the class InputHandler (including its
handleMessage method and supporting helpers isFiniteNumber/clamp), then run your
tests/CI locally and commit the formatted file with only formatting changes.
In `@src/server/websocket.ts`:
- Around line 226-308: Before handling the msg.type === "update-config" block,
gate the operation to only allow local connections by checking the websocket's
remote address (use ws._socket.remoteAddress) and reject any non-loopback
addresses (accept 127.0.0.1, ::1 and IPv4-mapped ::ffff:127.0.0.1 forms); if the
check fails, log the attempt via logger.warn and ws.send a config-updated
failure and return. Keep the rest of the validation (SERVER_CONFIG_KEYS, port
checks, configPath, fs writes, and error handling) unchanged, but ensure the
remote-address check happens at the top of the update-config branch so remote
clients cannot reach the file write logic.
In `@vite.config.ts`:
- Around line 1-10: Run the project's formatter (e.g., npx biome format --write
vite.config.ts or your repo's format script) on the file containing the
top-level imports and createWsServer (the block with imports like "node:url",
"@tanstack/devtools-vite", "vite-react", "nitro/vite", "vite-tsconfig-paths" and
the createWsServer import) to fix Biome formatting mismatches so CI will pass;
after formatting, re-run lint/CI and commit the formatted vite.config.ts
changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
biome.jsonsrc/components/Trackpad/ControlBar.tsxsrc/components/Trackpad/ScreenMirror.tsxsrc/hooks/useCaptureProvider.tssrc/hooks/useMirrorStream.tssrc/hooks/useRemoteConnection.tssrc/hooks/useTrackpadGesture.tssrc/routes/__root.tsxsrc/routes/settings.tsxsrc/routes/trackpad.tsxsrc/server/InputHandler.tssrc/server/tokenStore.tssrc/server/websocket.tssrc/types.tsxsrc/utils/logger.tsvite.config.ts
| @@ -1,55 +1,53 @@ | |||
| import type { ModifierState } from "@/types" | |||
| import type React from "react" | |||
| import type { ModifierState } from "@/types"; | |||
Contributor
There was a problem hiding this comment.
Fix Biome formatting to pass CI.
Run your project's format script to resolve the formatting mismatch flagged in the pipeline.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: File content differs from formatting output. Biome formatting check failed for this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ControlBar.tsx` at line 1, The import line in
ControlBar.tsx (import type { ModifierState } from "@/types";) does not match
the project's formatter expectations; run the repository's format script (e.g.,
npm run format or yarn format) or apply the Biome/Prettier configuration to
reformat src/components/Trackpad/ControlBar.tsx so the import and surrounding
file match CI style rules, then stage the updated file and push the commit.
Comment on lines
+61
to
+62
| <button type="button" className="btn btn-sm btn-outline">Copy</button> | ||
| <button type="button" className="btn btn-sm btn-outline">Paste</button> |
Contributor
There was a problem hiding this comment.
Copy/Paste buttons have no click handlers.
These buttons are rendered but have no onClick or onPointerDown handlers, making them non-functional. If this is intentional placeholder UI, consider disabling them or adding a TODO comment.
Do you want me to help implement copy/paste functionality using the Clipboard API, or should I open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ControlBar.tsx` around lines 61 - 62, The Copy and
Paste buttons in the ControlBar component are missing click handlers; add two
handler functions (e.g., handleCopy and handlePaste) inside the ControlBar
component and attach them to the respective buttons' onClick props (replace the
bare <button> for "Copy" and "Paste" with buttons that call handleCopy and
handlePaste). Implement handleCopy to obtain the text to copy (from
selectedTrackpadText prop/state or a passed-in value) and call
navigator.clipboard.writeText(text) with try/catch to log or surface errors;
implement handlePaste to call navigator.clipboard.readText() and then dispatch
the pasted text to the component (via a callback prop like onPaste or updating
local state), also handling errors. Add a feature-gate: if navigator.clipboard
is undefined, disable the buttons (disabled attribute) and add a short TODO
comment referencing Clipboard API fallback or issue tracking.
| @@ -0,0 +1,137 @@ | |||
| import type React from "react"; | |||
Contributor
There was a problem hiding this comment.
Fix Biome formatting to pass CI.
Run your project's format script to resolve the formatting mismatch flagged in the pipeline.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: File content differs from formatting output. Biome formatting check failed for this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` at line 1, The file
ScreenMirror.tsx fails Biome formatting; run the project's format script (e.g.,
npm run format or yarn format) and reformat
src/components/Trackpad/ScreenMirror.tsx so the import statement and the entire
file match the project's Biome/Prettier rules; after formatting, stage the
updated file and push the changes to resolve the CI formatting error.
Comment on lines
+45
to
+52
| return ( | ||
| <div | ||
| className="flex-1 bg-neutral-900 relative touch-none select-none flex items-center justify-center overflow-hidden" | ||
| onTouchStart={handleStart} | ||
| onTouchMove={handlers.onTouchMove} | ||
| onTouchEnd={handlers.onTouchEnd} | ||
| onMouseDown={handlePreventFocus} | ||
| > |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a11y lint suppression for touch surface.
Similar to TouchArea (per retrieved learning), this component handles continuous touch gestures. Add a lint suppression comment to clarify the intentional lack of keyboard accessibility.
💡 Add biome-ignore comment
return (
+ // biome-ignore lint/a11y/noStaticElementInteractions: Touch-only surface for continuous trackpad gestures
<div
className="flex-1 bg-neutral-900 relative touch-none select-none flex items-center justify-center overflow-hidden"
onTouchStart={handleStart}Based on learnings, the TouchArea component uses this pattern for similar touch-only interfaces.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <div | |
| className="flex-1 bg-neutral-900 relative touch-none select-none flex items-center justify-center overflow-hidden" | |
| onTouchStart={handleStart} | |
| onTouchMove={handlers.onTouchMove} | |
| onTouchEnd={handlers.onTouchEnd} | |
| onMouseDown={handlePreventFocus} | |
| > | |
| return ( | |
| // biome-ignore lint/a11y/noStaticElementInteractions: Touch-only surface for continuous trackpad gestures | |
| <div | |
| className="flex-1 bg-neutral-900 relative touch-none select-none flex items-center justify-center overflow-hidden" | |
| onTouchStart={handleStart} | |
| onTouchMove={handlers.onTouchMove} | |
| onTouchEnd={handlers.onTouchEnd} | |
| onMouseDown={handlePreventFocus} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` around lines 45 - 52, Add a
biome-ignore a11y lint suppression before the touch surface div in the
ScreenMirror component to document the intentional lack of keyboard
accessibility for this touch-only gesture area; place a single-line comment
(e.g. // biome-ignore-next-line jsx-a11y/no-static-element-interactions)
immediately above the <div> that uses handleStart, handlers.onTouchMove,
handlers.onTouchEnd and handlePreventFocus so the linter ignores the
non-interactive element touch handlers similarly to TouchArea.
Comment on lines
+125
to
+127
| } catch (err) { | ||
| isBusy.current = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Silent catch block hides canvas errors.
The catch block only resets isBusy without logging or reporting the error. This could hide issues with drawImage or toBlob that would be useful for debugging.
🛠️ Add error logging
} catch (err) {
isBusy.current = false;
+ console.error("Frame capture error:", err);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| isBusy.current = false; | |
| } | |
| } catch (err) { | |
| isBusy.current = false; | |
| console.error("Frame capture error:", err); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useCaptureProvider.ts` around lines 125 - 127, The catch block in
useCaptureProvider.ts currently only resets isBusy.current and swallows errors;
update that catch (err) to log/report the error (including err.message and
err.stack) and contextual info (e.g., whether the failure occurred during
drawImage or toBlob) — use the project's logger or console.error (or a telemetry
helper if available) and still ensure isBusy.current is reset (or move reset
into a finally block) so failures are visible for debugging.
Comment on lines
+69
to
73
| // Send backspaces for removed/changed characters | ||
| const deletions = prevData.length - commonLen; | ||
| for (let i = 0; i < deletions; i++) { | ||
| send({ type: "key", key: "backspace" }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sending backspaces in a loop may cause latency for large deletions.
For significant composition changes, sending many individual backspace events could be slow. Consider batching or sending a count if the protocol supports it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trackpad.tsx` around lines 69 - 73, The current loop in
trackpad.tsx that sends individual backspace key events (using deletions,
prevData, commonLen and send({ type: "key", key: "backspace" })) can be slow for
large deletions; change it to batch deletions into a single message when
possible (e.g., send({ type: "delete", key: "backspace", count: deletions }) or
send({ type: "delete", count: deletions })) and update the receiving/handling
code to accept a count, falling back to the existing per-key loop only if the
protocol/receiver doesn't support a count; locate and modify the code paths
around send, deletions, prevData and commonLen to implement this optimization.
Comment on lines
257
to
259
| onBlur={() => { | ||
| setTimeout(() => hiddenInputRef.current?.focus(), 10) | ||
| setTimeout(() => hiddenInputRef.current?.focus(), 10); | ||
| }} |
Contributor
There was a problem hiding this comment.
Aggressive refocus may trap keyboard focus.
The onBlur handler immediately refocuses the input after 10ms, which could prevent users from navigating to other elements. This may cause accessibility issues and frustrate users trying to interact with other controls.
🛠️ Suggested fix: Only refocus if trackpad area was touched
onBlur={() => {
- setTimeout(() => hiddenInputRef.current?.focus(), 10);
+ // Only refocus if user taps the trackpad area, not automatically
}}Consider removing automatic refocus and instead trigger focus explicitly when the user interacts with the trackpad surface (which is already handled via handleContainerClick and onInputFocus).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trackpad.tsx` around lines 257 - 259, The onBlur handler currently
force-refocuses the hidden input via setTimeout(() =>
hiddenInputRef.current?.focus(), 10), which can trap keyboard focus; remove this
automatic refocus and rely on explicit focus flows already provided by
handleContainerClick and onInputFocus, or change the onBlur to only refocus when
the blur event indicates the user is still interacting with the trackpad (e.g.,
detect pointer/touch state or event.relatedTarget within the trackpad area)
rather than always calling hiddenInputRef.current.focus(); update the onBlur in
the component that uses hiddenInputRef to implement one of these two approaches
and ensure focus management is tied to explicit user interaction handlers like
handleContainerClick/onInputFocus.
Comment on lines
89
to
91
| this.handleMessage(pending).catch((err) => { | ||
| console.error("Error processing pending move event:", err) | ||
| logger.error("Error processing pending move event: %O", err) | ||
| }) |
Contributor
There was a problem hiding this comment.
Fix misleading log text in pending scroll error path.
The scroll throttle branch logs "pending move event" which obscures diagnostics.
💡 Proposed fix
-logger.error("Error processing pending move event: %O", err)
+logger.error("Error processing pending scroll event: %O", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/InputHandler.ts` around lines 89 - 91, The log message in the
scroll-throttle branch is misleading: when calling
this.handleMessage(pending).catch(...) you should change the logger.error call
to reference the correct event type (e.g., "pending scroll event") and keep the
error details (err) and any context (the pending object) in the formatted log;
update the logger.error invocation near handleMessage and pending to a clear
message like "Error processing pending scroll event: %O" and include the err
(and optionally pending) so diagnostics reflect the actual branch.
| } | ||
|
|
||
| ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })) | ||
| ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })); |
Contributor
There was a problem hiding this comment.
connected payload omits platform, so client platform state never populates.
src/hooks/useRemoteConnection.ts reads msg.platform on "connected"; this payload currently only sends serverIp.
💡 Proposed fix
-ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP }));
+ws.send(
+ JSON.stringify({
+ type: "connected",
+ serverIp: LAN_IP,
+ platform: process.platform,
+ }),
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })); | |
| ws.send( | |
| JSON.stringify({ | |
| type: "connected", | |
| serverIp: LAN_IP, | |
| platform: process.platform, | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` at line 114, The "connected" message sent via
ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })) is missing the
platform field that useRemoteConnection.ts expects; update the payload to
include the server platform (e.g., process.platform or os.platform()) as a
"platform" property alongside serverIp so clients populate platform state when
receiving type "connected".
Comment on lines
+132
to
+145
| if (isBinary) { | ||
| // Relay frames from Providers to Consumers | ||
| if ((ws as ExtWebSocket).isProvider) { | ||
| for (const client of wss.clients) { | ||
| if ( | ||
| client !== ws && | ||
| (client as ExtWebSocket).isConsumer && | ||
| client.readyState === WebSocket.OPEN | ||
| ) { | ||
| client.send(data, { binary: true }); | ||
| } | ||
| } | ||
| } | ||
| return; |
Contributor
There was a problem hiding this comment.
Binary mirror frames are relayed without size limits.
A provider can push oversized binary payloads and fan them out to all consumers, which is a DoS vector.
💡 Proposed fix
-const MAX_PAYLOAD_SIZE = 10 * 1024; // 10KB limit
+const MAX_PAYLOAD_SIZE = 10 * 1024; // control messages
+const MAX_FRAME_PAYLOAD_SIZE = 2 * 1024 * 1024; // 2MB per frame
@@
if (isBinary) {
+ const frameSize = Buffer.isBuffer(data)
+ ? data.length
+ : Array.isArray(data)
+ ? data.reduce((total, chunk) => total + chunk.length, 0)
+ : data.byteLength;
+ if (frameSize > MAX_FRAME_PAYLOAD_SIZE) {
+ logger.warn(`Binary payload too large (${frameSize} bytes), dropping frame`);
+ return;
+ }
// Relay frames from Providers to Consumers
if ((ws as ExtWebSocket).isProvider) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` around lines 132 - 145, The relay loop in websocket
message handling (the isBinary branch where (ws as ExtWebSocket).isProvider is
checked) currently forwards binary frames to all consumers without size checks;
add a maximum allowed binary payload size constant (e.g.,
MAX_BINARY_FRAME_BYTES) and validate data.byteLength or Buffer.byteLength before
fanout, and if it exceeds the limit either drop the frame and log a warning via
the existing logger or close the provider connection with an explanatory close
code; ensure the size check happens before iterating wss.clients so oversized
frames are never sent to consumers.
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/hooks/useRemoteConnection.ts (1)
39-43:⚠️ Potential issue | 🟡 MinorUnset
onmessageduring socket teardown.
onmessageis now assigned, but reconnect/unmount cleanup does not clear it. A closing old socket can still deliver late message callbacks.💡 Suggested fix
if (wsRef.current) { wsRef.current.onopen = null + wsRef.current.onmessage = null wsRef.current.onclose = null wsRef.current.onerror = null wsRef.current.close() wsRef.current = null } @@ if (wsRef.current) { // Nullify handlers to prevent cascading error/close events wsRef.current.onopen = null + wsRef.current.onmessage = null wsRef.current.onclose = null wsRef.current.onerror = null wsRef.current.close() wsRef.current = null }Also applies to: 53-63, 88-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` around lines 39 - 43, The teardown currently nulls wsRef.current.onopen/onclose/onerror but misses clearing wsRef.current.onmessage, so stale sockets can still invoke message handlers; update all teardown/cleanup paths that reference wsRef.current (the places around the current teardown block, the reconnect/unmount cleanup, and the other socket-close paths) to also set wsRef.current.onmessage = null (guarded by a truthy wsRef.current check) before calling wsRef.current.close() and before nulling wsRef.current so the message handler is removed consistently.
♻️ Duplicate comments (4)
src/routes/settings.tsx (1)
215-220:⚠️ Potential issue | 🟠 MajorExternalize edited helper texts to i18n keys.
The touched UI strings are still hardcoded (Line 218-219 and Line 257). Please move them to translation resources to keep localization consistent.
Proposed patch
+import { useTranslation } from "react-i18next" @@ function SettingsPage() { + const { t } = useTranslation() @@ - {invertScroll - ? "Traditional scrolling enabled" - : "Natural scrolling"} + {invertScroll + ? t("settings.scroll.traditionalEnabled") + : t("settings.scroll.natural")} @@ - This Computer's LAN IP + {t("settings.server.lanIpHint")}As per coding guidelines, "User-visible strings should be externalized to resource files (i18n)".
Also applies to: 255-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 215 - 220, Replace the hardcoded user-visible strings in the Settings component with i18n keys: where the UI currently renders the invertScroll-dependent text ("Traditional scrolling enabled" / "Natural scrolling") and the other hardcoded strings at the second occurrence (around the block referenced by invertScroll), call the translation function (e.g., useTranslation().t or t) instead of literal strings and use descriptive keys such as settings.scroll.traditional and settings.scroll.natural; add those keys and translated values to the translation resource files for all supported locales and ensure the Settings component imports/uses the i18n hook (useTranslation/t) so the JSX uses t('settings.scroll.traditional') / t('settings.scroll.natural') rather than the hardcoded English text.src/server/websocket.ts (2)
114-114:⚠️ Potential issue | 🟠 MajorInclude
platformin theconnectedpayload.
useRemoteConnectionreadsmsg.platformon"connected", so this payload currently leaves platform state unset.💡 Suggested fix
-ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })) +ws.send( + JSON.stringify({ + type: "connected", + serverIp: LAN_IP, + platform: process.platform, + }), +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 114, The "connected" message sent via ws.send currently omits the platform field used by useRemoteConnection; update the payload sent in websocket.ts (the ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP }))) to include platform (for example platform: process.platform or the existing PLATFORM constant) so the message becomes { type: "connected", serverIp: LAN_IP, platform: <platform value> } ensuring useRemoteConnection can read msg.platform.
48-49:⚠️ Potential issue | 🟠 MajorAdd a max binary frame-size check before provider fanout.
Provider frames are currently relayed without size limits. A single oversized payload can be broadcast to all consumers.
💡 Suggested fix
const MAX_PAYLOAD_SIZE = 10 * 1024 // 10KB limit +const MAX_FRAME_PAYLOAD_SIZE = 2 * 1024 * 1024 // 2MB/frame @@ if (isBinary) { + const frameSize = Buffer.isBuffer(data) + ? data.length + : Array.isArray(data) + ? data.reduce((total, chunk) => total + chunk.length, 0) + : data.byteLength + if (frameSize > MAX_FRAME_PAYLOAD_SIZE) { + logger.warn( + `Binary payload too large (${frameSize} bytes), dropping frame`, + ) + return + } // Relay frames from Providers to Consumers if ((ws as ExtWebSocket).isProvider) {Also applies to: 132-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 48 - 49, Add a hard check that rejects/ignores binary frames exceeding MAX_PAYLOAD_SIZE before any fanout to consumers: in the provider frame handling path (look for the provider message handler function(s) such as the code that reads incoming frames and the fanout/broadcast routine, e.g., any function named like handleProviderFrame, onProviderMessage, or broadcastToConsumers), validate Buffer.byteLength (or the frame's byteLength) against MAX_PAYLOAD_SIZE and return/log an error or close the provider connection if exceeded; apply the same check in the other provider-to-consumer fanout location referenced around the block for lines 132-145 so oversized payloads are not relayed.src/server/InputHandler.ts (1)
89-91:⚠️ Potential issue | 🟡 MinorFix mislabeled error text in the pending scroll branch.
This code path handles pending scroll events but logs
"pending move event", which makes troubleshooting noisy.💡 Suggested fix
- this.handleMessage(pending).catch((err) => { - logger.error("Error processing pending move event: %O", err) - }) + this.handleMessage(pending).catch((err) => { + logger.error("Error processing pending scroll event: %O", err) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 89 - 91, The log message in the pending scroll branch is mislabelled as "pending move event"; update the error text passed to logger.error in the pending handling path where this.handleMessage(pending) is called so it correctly says "Error processing pending scroll event" (reference: the call site using this.handleMessage(pending).catch and logger.error) to avoid noisy/misleading logs.
🤖 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/routes/trackpad.tsx`:
- Around line 186-190: The sendText function currently appends a trailing space
for multi-character inputs which mutates user input; update sendText so it does
not force-append whitespace — keep the early-empty check (if (!val) return) and
call send({ type: "text", text: val }) using the original val unchanged,
removing the `${val} ` branch; ensure references to sendText and the send({
type: "text", ... }) call are updated accordingly.
---
Outside diff comments:
In `@src/hooks/useRemoteConnection.ts`:
- Around line 39-43: The teardown currently nulls
wsRef.current.onopen/onclose/onerror but misses clearing
wsRef.current.onmessage, so stale sockets can still invoke message handlers;
update all teardown/cleanup paths that reference wsRef.current (the places
around the current teardown block, the reconnect/unmount cleanup, and the other
socket-close paths) to also set wsRef.current.onmessage = null (guarded by a
truthy wsRef.current check) before calling wsRef.current.close() and before
nulling wsRef.current so the message handler is removed consistently.
---
Duplicate comments:
In `@src/routes/settings.tsx`:
- Around line 215-220: Replace the hardcoded user-visible strings in the
Settings component with i18n keys: where the UI currently renders the
invertScroll-dependent text ("Traditional scrolling enabled" / "Natural
scrolling") and the other hardcoded strings at the second occurrence (around the
block referenced by invertScroll), call the translation function (e.g.,
useTranslation().t or t) instead of literal strings and use descriptive keys
such as settings.scroll.traditional and settings.scroll.natural; add those keys
and translated values to the translation resource files for all supported
locales and ensure the Settings component imports/uses the i18n hook
(useTranslation/t) so the JSX uses t('settings.scroll.traditional') /
t('settings.scroll.natural') rather than the hardcoded English text.
In `@src/server/InputHandler.ts`:
- Around line 89-91: The log message in the pending scroll branch is mislabelled
as "pending move event"; update the error text passed to logger.error in the
pending handling path where this.handleMessage(pending) is called so it
correctly says "Error processing pending scroll event" (reference: the call site
using this.handleMessage(pending).catch and logger.error) to avoid
noisy/misleading logs.
In `@src/server/websocket.ts`:
- Line 114: The "connected" message sent via ws.send currently omits the
platform field used by useRemoteConnection; update the payload sent in
websocket.ts (the ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP
}))) to include platform (for example platform: process.platform or the existing
PLATFORM constant) so the message becomes { type: "connected", serverIp: LAN_IP,
platform: <platform value> } ensuring useRemoteConnection can read msg.platform.
- Around line 48-49: Add a hard check that rejects/ignores binary frames
exceeding MAX_PAYLOAD_SIZE before any fanout to consumers: in the provider frame
handling path (look for the provider message handler function(s) such as the
code that reads incoming frames and the fanout/broadcast routine, e.g., any
function named like handleProviderFrame, onProviderMessage, or
broadcastToConsumers), validate Buffer.byteLength (or the frame's byteLength)
against MAX_PAYLOAD_SIZE and return/log an error or close the provider
connection if exceeded; apply the same check in the other provider-to-consumer
fanout location referenced around the block for lines 132-145 so oversized
payloads are not relayed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/components/Trackpad/ControlBar.tsxsrc/hooks/useRemoteConnection.tssrc/hooks/useTrackpadGesture.tssrc/routes/settings.tsxsrc/routes/trackpad.tsxsrc/server/InputHandler.tssrc/server/tokenStore.tssrc/server/websocket.tssrc/utils/logger.ts
💤 Files with no reviewable changes (1)
- src/components/Trackpad/ControlBar.tsx
Comment on lines
+186
to
+190
| const sendText = (val: string) => { | ||
| if (!val) return | ||
| const toSend = val.length > 1 ? `${val} ` : val | ||
| send({ type: "text", text: toSend }) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t force-append whitespace in sendText.
Appending " " for every multi-character value mutates user input and can create extra trailing spaces (e.g., paste/voice/predictive text).
💡 Suggested fix
const sendText = (val: string) => {
if (!val) return
- const toSend = val.length > 1 ? `${val} ` : val
- send({ type: "text", text: toSend })
+ send({ type: "text", text: val })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trackpad.tsx` around lines 186 - 190, The sendText function
currently appends a trailing space for multi-character inputs which mutates user
input; update sendText so it does not force-append whitespace — keep the
early-empty check (if (!val) return) and call send({ type: "text", text: val })
using the original val unchanged, removing the `${val} ` branch; ensure
references to sendText and the send({ type: "text", ... }) call are updated
accordingly.
|
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: |
2c6b104 to
5eb1a89
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useRemoteConnection.ts (1)
86-93:⚠️ Potential issue | 🟡 MinorMissing
onmessagehandler cleanup.The cleanup function nullifies
onopen,onclose, andonerrorbut omitsonmessage. For consistency and to prevent potential handler invocation after unmount, include it.💡 Proposed fix
if (wsRef.current) { // Nullify handlers to prevent cascading error/close events wsRef.current.onopen = null wsRef.current.onclose = null wsRef.current.onerror = null + wsRef.current.onmessage = null wsRef.current.close() wsRef.current = null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRemoteConnection.ts` around lines 86 - 93, The cleanup block for wsRef.current in useRemoteConnection.ts misses nullifying the onmessage handler; update the shutdown sequence where wsRef.current is cleared (the section that currently sets onopen/onclose/onerror to null, calls close(), and sets wsRef.current = null) to also set wsRef.current.onmessage = null before calling close() to prevent any stray message events after unmount.
♻️ Duplicate comments (9)
src/server/InputHandler.ts (1)
89-91:⚠️ Potential issue | 🟡 MinorFix misleading log text in pending scroll error path.
The scroll throttle branch logs
"pending move event"which obscures diagnostics. This should say"pending scroll event".💡 Proposed fix
-logger.error("Error processing pending move event: %O", err) +logger.error("Error processing pending scroll event: %O", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 89 - 91, The log message in the scroll throttle branch is misleading—update the logger call inside the catch of the this.handleMessage(pending).catch(...) to reference "pending scroll event" instead of "pending move event"; locate the catch where handleMessage is invoked with the pending variable (in InputHandler) and change the logged text passed to logger.error so it accurately reflects a pending scroll event while preserving the error formatting (%O) and err argument.src/hooks/useCaptureProvider.ts (2)
130-173: 🧹 Nitpick | 🔵 Trivial
startSharingshould be wrapped inuseCallback.The function is returned from the hook and may be passed to child components. Without
useCallback, it creates a new reference on every render, potentially causing unnecessary re-renders in consuming components and breaking effect dependencies.💡 Proposed fix
-const startSharing = async () => { +const startSharing = useCallback(async () => { try { const stream = await navigator.mediaDevices.getDisplayMedia({ // ... rest of implementation } catch (err) { console.error("Failed to start screen share:", err) } -} +}, [wsRef, stopSharing])Note: Ensure
stopSharingis defined beforestartSharingor read it via a ref to handle the circular dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCaptureProvider.ts` around lines 130 - 173, startSharing is recreated on every render and should be wrapped in useCallback inside the useCaptureProvider hook to avoid breaking consumers and effect deps; wrap the existing startSharing function in React.useCallback, include only stable dependencies (e.g. wsRef.current, workerRef.current, setIsSharing, streamRef, videoRef, isSharingRef) and avoid a circular dependency on stopSharing by either defining stopSharing before startSharing or referencing stopSharing via a ref (e.g. stopSharingRef.current) inside the callback so the callback does not directly depend on a changing stopSharing function.
125-127:⚠️ Potential issue | 🟡 MinorSilent catch block hides canvas errors.
The catch block only resets
isBusywithout logging the error. This could hide issues withdrawImageortoBlobthat would be useful for debugging.💡 Proposed fix — add error logging
} catch (err) { isBusy.current = false + console.error("Frame capture error:", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCaptureProvider.ts` around lines 125 - 127, In useCaptureProvider's try/catch that currently only sets isBusy.current = false, update the catch to log the caught error (e.g., include the thrown error object/message) so canvas failures from drawImage/toBlob are recorded; locate the catch in the function useCaptureProvider (the block that references isBusy.current) and add a clear error log (console.error or the module's logger) mentioning the operation (drawImage/toBlob) and the error, then still reset isBusy.current = false.src/hooks/useTrackpadGesture.ts (1)
39-41:⚠️ Potential issue | 🟡 MinorPending drag timeout can fire after unmount and send a stale click-up.
The
draggingTimeoutref has no cleanup on unmount, which can cause ghost events if the component unmounts while the timeout is pending.💡 Proposed fix — add useEffect cleanup
import { useRef, useState } from "react" +import { useEffect, useRef, useState } from "react"Then add cleanup effect:
const draggingTimeout = useRef<NodeJS.Timeout | null>(null) + +useEffect(() => { + return () => { + if (draggingTimeout.current) { + clearTimeout(draggingTimeout.current) + draggingTimeout.current = null + } + } +}, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useTrackpadGesture.ts` around lines 39 - 41, Add a cleanup effect in the useTrackpadGesture hook that clears any pending draggingTimeout to prevent stale events after unmount: inside a useEffect with no deps return a cleanup function that checks draggingTimeout.current, calls clearTimeout on it, sets draggingTimeout.current = null, and also reset dragging.current = false (and lastPinchDist.current = null) to ensure no leftover state fires after unmount; this targets the draggingTimeout, dragging, and lastPinchDist refs declared in the hook.src/server/websocket.ts (2)
114-114:⚠️ Potential issue | 🟠 Major
connectedpayload omitsplatform, so client platform state never populates.
src/hooks/useRemoteConnection.tsreadsmsg.platformon"connected", but this payload only sendsserverIp.💡 Proposed fix
-ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })) +ws.send( + JSON.stringify({ + type: "connected", + serverIp: LAN_IP, + platform: process.platform, + }), +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 114, The "connected" message sent from websocket.ts currently only includes serverIp so the client never receives platform; update the payload sent in the ws.send for the `{ type: "connected" }` message to include the server platform (e.g. add a platform field using Node's process.platform or os.platform()), so useRemoteConnection.ts can read msg.platform and populate client platform state correctly.
132-146:⚠️ Potential issue | 🟠 MajorBinary mirror frames are relayed without size limits.
A provider can push oversized binary payloads and fan them out to all consumers, which is a DoS vector.
💡 Proposed fix
+const MAX_FRAME_PAYLOAD_SIZE = 2 * 1024 * 1024 // 2MB per frame if (isBinary) { + const frameSize = Buffer.isBuffer(data) + ? data.length + : Array.isArray(data) + ? data.reduce((total, chunk) => total + chunk.length, 0) + : data.byteLength + if (frameSize > MAX_FRAME_PAYLOAD_SIZE) { + logger.warn(`Binary payload too large (${frameSize} bytes), dropping frame`) + return + } // Relay frames from Providers to Consumers if ((ws as ExtWebSocket).isProvider) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 132 - 146, The binary relay loop in websocket.ts currently forwards any binary frame from a provider (check: (ws as ExtWebSocket).isProvider) to all consumers via wss.clients and client.send with no size checks; add a maximum allowed size constant (e.g., MAX_BINARY_FRAME_SIZE) and before relaying inspect the incoming data length (use data.byteLength or data.length depending on Buffer/ArrayBuffer) and if it exceeds the limit, drop the frame (and optionally log and/or close the provider connection) instead of forwarding; apply this check inside the isBinary branch before iterating wss.clients so oversized frames are not sent to consumers and consider emitting a warning via the same processLogger/WebSocket close logic used elsewhere.src/routes/trackpad.tsx (3)
69-73: 🧹 Nitpick | 🔵 TrivialBatch composition deletions instead of sending one backspace per character.
The per-character loop can become slow for large IME edits and increases message overhead. Prefer a counted delete message (with fallback to loop only if unsupported).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trackpad.tsx` around lines 69 - 73, Replace the per-character backspace loop that computes deletions = prevData.length - commonLen and calls send({ type: "key", key: "backspace" }) repeatedly with a single batched delete message (e.g., send({ type: "delete", count: deletions })) to reduce message overhead; implement a fallback that detects or handles unsupported receivers by falling back to the original loop (use the existing send function and the deletions variable, or try sending the batched message and on failure iterate sending individual backspaces).
186-190:⚠️ Potential issue | 🟡 MinorDo not append trailing whitespace in
sendText.This mutates user input and can introduce unintended spaces for paste/voice/predictive text.
💡 Proposed fix
const sendText = (val: string) => { if (!val) return - const toSend = val.length > 1 ? `${val} ` : val - send({ type: "text", text: toSend }) + send({ type: "text", text: val }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trackpad.tsx` around lines 186 - 190, The sendText function currently appends a trailing space for multi-character input which mutates user input; change sendText (the function that calls send({ type: "text", text: ... })) to send the input value unchanged (after the existing empty check) so that send is invoked with text: val for all non-empty inputs instead of adding `${val} `.
257-259:⚠️ Potential issue | 🟠 MajorAvoid unconditional refocus on blur; this can trap focus.
Always re-focusing the hidden input prevents normal navigation to other controls and can break accessibility flows.
💡 Safer direction
onBlur={() => { - setTimeout(() => hiddenInputRef.current?.focus(), 10) + // Refocus only on explicit trackpad interaction, not on every blur. }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trackpad.tsx` around lines 257 - 259, The onBlur handler currently always refocuses hiddenInputRef which traps focus; modify the onBlur on the trackpad component to accept the blur event, check event.relatedTarget (or document.activeElement after a tick) and only call setTimeout(() => hiddenInputRef.current?.focus(), 10) when focus moved to an element inside the trackpad that requires refocusing (e.g., when relatedTarget is contained by your trackpad/container ref) — otherwise do nothing so normal keyboard/tab navigation can proceed; reference the onBlur handler, hiddenInputRef and your trackpad/container ref (e.g., trackpadRef or containerRef) when implementing the containment check.
🤖 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/components/Trackpad/ScreenMirror.tsx`:
- Around line 37-47: The effect in ScreenMirror re-triggers because startSharing
(from useCaptureProvider) is unstable; to fix, stabilize that function by
wrapping startSharing in useCallback inside useCaptureProvider so its reference
doesn't change every render, then keep the current useEffect([status, canShare,
isSharing, startSharing]) as-is; alternatively (if you cannot change the
provider), expose startSharing via a ref or copy it into a ref in ScreenMirror
and use that ref.current inside the effect to avoid re-scheduling—target the
startSharing function in useCaptureProvider and the useEffect in
ScreenMirror.tsx.
- Around line 117-132: In the ScreenMirror component replace the hardcoded user
strings "LIVE SHARING" and "Stop" (rendered when isSharing is true and the
stopSharing button) with i18n keys using your project's translation helper
(e.g., useTranslation()/t) — add keys like "screenMirror.liveSharing" and
"screenMirror.stop" to your locale files and call t('screenMirror.liveSharing')
and t('screenMirror.stop') where the badge text and button label are rendered so
strings are externalized while still invoking stopSharing on click.
- Around line 80-107: The ScreenMirror component contains hardcoded user-facing
strings (e.g., "Mirror Standby", "Connecting...", "Start Screen Share", the Tip
text, and "Waiting for Desktop stream..."); replace these literals with i18n
resource lookups (e.g., use a translation hook or t function) and move the
strings into your locale resource files, updating usages inside the JSX
conditional blocks that reference status and canShare and the Start button
handler (startSharing) to call the translator (e.g.,
t('screenMirror.mirrorStandby'), t('screenMirror.connecting'),
t('screenMirror.startScreenShare'), t('screenMirror.tipSharingEntireScreen'),
t('screenMirror.waitingForDesktop')). Ensure keys are added to the resource
files for all supported locales and update any tests or snapshots that depend on
the original text.
- Around line 1-5: This file uses React hooks and browser APIs (useEffect,
useRef) and imports client-only hooks (useCaptureProvider, useMirrorStream) but
lacks the Next.js "use client" directive; add the literal "use client" as the
very first line of src/components/Trackpad/ScreenMirror.tsx (above the imports)
so the ScreenMirror component and its useEffect/useRef hooks run on the client.
In `@src/utils/logger.ts`:
- Around line 55-56: The variables originalConsoleLog and originalConsoleError
in src/utils/logger.ts are assigned but never used; either remove those two
declarations to eliminate dead code, or keep them and implement/export a restore
function (e.g., restoreOriginalConsole or restoreConsole) that reassigns
console.log = originalConsoleLog and console.error = originalConsoleError and
export that function so the originals are actually referenced; update any
related wrapper logic in the file (the code that replaces console methods) to
use the originals when restoring.
---
Outside diff comments:
In `@src/hooks/useRemoteConnection.ts`:
- Around line 86-93: The cleanup block for wsRef.current in
useRemoteConnection.ts misses nullifying the onmessage handler; update the
shutdown sequence where wsRef.current is cleared (the section that currently
sets onopen/onclose/onerror to null, calls close(), and sets wsRef.current =
null) to also set wsRef.current.onmessage = null before calling close() to
prevent any stray message events after unmount.
---
Duplicate comments:
In `@src/hooks/useCaptureProvider.ts`:
- Around line 130-173: startSharing is recreated on every render and should be
wrapped in useCallback inside the useCaptureProvider hook to avoid breaking
consumers and effect deps; wrap the existing startSharing function in
React.useCallback, include only stable dependencies (e.g. wsRef.current,
workerRef.current, setIsSharing, streamRef, videoRef, isSharingRef) and avoid a
circular dependency on stopSharing by either defining stopSharing before
startSharing or referencing stopSharing via a ref (e.g. stopSharingRef.current)
inside the callback so the callback does not directly depend on a changing
stopSharing function.
- Around line 125-127: In useCaptureProvider's try/catch that currently only
sets isBusy.current = false, update the catch to log the caught error (e.g.,
include the thrown error object/message) so canvas failures from
drawImage/toBlob are recorded; locate the catch in the function
useCaptureProvider (the block that references isBusy.current) and add a clear
error log (console.error or the module's logger) mentioning the operation
(drawImage/toBlob) and the error, then still reset isBusy.current = false.
In `@src/hooks/useTrackpadGesture.ts`:
- Around line 39-41: Add a cleanup effect in the useTrackpadGesture hook that
clears any pending draggingTimeout to prevent stale events after unmount: inside
a useEffect with no deps return a cleanup function that checks
draggingTimeout.current, calls clearTimeout on it, sets draggingTimeout.current
= null, and also reset dragging.current = false (and lastPinchDist.current =
null) to ensure no leftover state fires after unmount; this targets the
draggingTimeout, dragging, and lastPinchDist refs declared in the hook.
In `@src/routes/trackpad.tsx`:
- Around line 69-73: Replace the per-character backspace loop that computes
deletions = prevData.length - commonLen and calls send({ type: "key", key:
"backspace" }) repeatedly with a single batched delete message (e.g., send({
type: "delete", count: deletions })) to reduce message overhead; implement a
fallback that detects or handles unsupported receivers by falling back to the
original loop (use the existing send function and the deletions variable, or try
sending the batched message and on failure iterate sending individual
backspaces).
- Around line 186-190: The sendText function currently appends a trailing space
for multi-character input which mutates user input; change sendText (the
function that calls send({ type: "text", text: ... })) to send the input value
unchanged (after the existing empty check) so that send is invoked with text:
val for all non-empty inputs instead of adding `${val} `.
- Around line 257-259: The onBlur handler currently always refocuses
hiddenInputRef which traps focus; modify the onBlur on the trackpad component to
accept the blur event, check event.relatedTarget (or document.activeElement
after a tick) and only call setTimeout(() => hiddenInputRef.current?.focus(),
10) when focus moved to an element inside the trackpad that requires refocusing
(e.g., when relatedTarget is contained by your trackpad/container ref) —
otherwise do nothing so normal keyboard/tab navigation can proceed; reference
the onBlur handler, hiddenInputRef and your trackpad/container ref (e.g.,
trackpadRef or containerRef) when implementing the containment check.
In `@src/server/InputHandler.ts`:
- Around line 89-91: The log message in the scroll throttle branch is
misleading—update the logger call inside the catch of the
this.handleMessage(pending).catch(...) to reference "pending scroll event"
instead of "pending move event"; locate the catch where handleMessage is invoked
with the pending variable (in InputHandler) and change the logged text passed to
logger.error so it accurately reflects a pending scroll event while preserving
the error formatting (%O) and err argument.
In `@src/server/websocket.ts`:
- Line 114: The "connected" message sent from websocket.ts currently only
includes serverIp so the client never receives platform; update the payload sent
in the ws.send for the `{ type: "connected" }` message to include the server
platform (e.g. add a platform field using Node's process.platform or
os.platform()), so useRemoteConnection.ts can read msg.platform and populate
client platform state correctly.
- Around line 132-146: The binary relay loop in websocket.ts currently forwards
any binary frame from a provider (check: (ws as ExtWebSocket).isProvider) to all
consumers via wss.clients and client.send with no size checks; add a maximum
allowed size constant (e.g., MAX_BINARY_FRAME_SIZE) and before relaying inspect
the incoming data length (use data.byteLength or data.length depending on
Buffer/ArrayBuffer) and if it exceeds the limit, drop the frame (and optionally
log and/or close the provider connection) instead of forwarding; apply this
check inside the isBinary branch before iterating wss.clients so oversized
frames are not sent to consumers and consider emitting a warning via the same
processLogger/WebSocket close logic used elsewhere.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/components/Trackpad/ControlBar.tsxsrc/components/Trackpad/ScreenMirror.tsxsrc/hooks/useCaptureProvider.tssrc/hooks/useRemoteConnection.tssrc/hooks/useTrackpadGesture.tssrc/routes/settings.tsxsrc/routes/trackpad.tsxsrc/server/InputHandler.tssrc/server/tokenStore.tssrc/server/websocket.tssrc/utils/logger.ts
💤 Files with no reviewable changes (1)
- src/components/Trackpad/ControlBar.tsx
Comment on lines
+1
to
+5
| import type React from "react" | ||
| import { useEffect, useRef } from "react" | ||
| import { useCaptureProvider } from "../../hooks/useCaptureProvider" | ||
| import { useMirrorStream } from "../../hooks/useMirrorStream" | ||
|
|
Contributor
There was a problem hiding this comment.
Missing "use client" directive.
Per coding guidelines, NextJS files should include "use client" when using client-side features. This component uses hooks and browser APIs.
💡 Proposed fix
+"use client"
+
import type React from "react"
import { useEffect, useRef } from "react"As per coding guidelines: "Ensure that 'use client' is being used" for NextJS files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type React from "react" | |
| import { useEffect, useRef } from "react" | |
| import { useCaptureProvider } from "../../hooks/useCaptureProvider" | |
| import { useMirrorStream } from "../../hooks/useMirrorStream" | |
| "use client" | |
| import type React from "react" | |
| import { useEffect, useRef } from "react" | |
| import { useCaptureProvider } from "../../hooks/useCaptureProvider" | |
| import { useMirrorStream } from "../../hooks/useMirrorStream" | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` around lines 1 - 5, This file uses
React hooks and browser APIs (useEffect, useRef) and imports client-only hooks
(useCaptureProvider, useMirrorStream) but lacks the Next.js "use client"
directive; add the literal "use client" as the very first line of
src/components/Trackpad/ScreenMirror.tsx (above the imports) so the ScreenMirror
component and its useEffect/useRef hooks run on the client.
Comment on lines
+37
to
+47
| // Automatically start sharing when connected | ||
| useEffect(() => { | ||
| if (status === "connected" && canShare && !isSharing) { | ||
| const timer = setTimeout(() => { | ||
| startSharing().catch(() => { | ||
| // Fallback: browser might block auto-popup without gesture | ||
| }) | ||
| }, 1000) // Small delay to ensure everything is ready | ||
| return () => clearTimeout(timer) | ||
| } | ||
| }, [status, canShare, isSharing, startSharing]) |
Contributor
There was a problem hiding this comment.
Effect may re-run unnecessarily due to unstable startSharing reference.
Since startSharing from useCaptureProvider is not wrapped in useCallback, it creates a new function reference on every render. This causes the effect to re-run, though it's self-limiting because of the !isSharing check.
However, this still causes unnecessary effect scheduling. Consider either:
- Wrapping
startSharinginuseCallbackinuseCaptureProvider(recommended). - Using a ref for
startSharingin this component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` around lines 37 - 47, The effect in
ScreenMirror re-triggers because startSharing (from useCaptureProvider) is
unstable; to fix, stabilize that function by wrapping startSharing in
useCallback inside useCaptureProvider so its reference doesn't change every
render, then keep the current useEffect([status, canShare, isSharing,
startSharing]) as-is; alternatively (if you cannot change the provider), expose
startSharing via a ref or copy it into a ref in ScreenMirror and use that
ref.current inside the effect to avoid re-scheduling—target the startSharing
function in useCaptureProvider and the useEffect in ScreenMirror.tsx.
Comment on lines
+80
to
+107
| <div className="absolute inset-0 flex flex-col items-center justify-center z-40 bg-neutral-900/40 backdrop-blur-[2px] pointer-events-none px-6"> | ||
| <div className="text-neutral-500 text-center pointer-events-none"> | ||
| <div className="text-2xl mb-4 opacity-40"> | ||
| {status === "connected" ? "Mirror Standby" : "Connecting..."} | ||
| </div> | ||
|
|
||
| {status === "connected" && canShare && ( | ||
| <div className="flex flex-col items-center gap-4 animate-in fade-in zoom-in duration-300 pointer-events-auto"> | ||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| startSharing() | ||
| }} | ||
| className="btn btn-primary btn-wide shadow-xl relative z-50" | ||
| > | ||
| Start Screen Share | ||
| </button> | ||
| <p className="text-[10px] text-neutral-500 max-w-[200px]"> | ||
| Tip: Sharing <b>"Entire Screen"</b> is required for full remote control. | ||
| </p> | ||
| </div> | ||
| )} | ||
|
|
||
| {status === "connected" && !canShare && ( | ||
| <div className="bg-primary/10 text-primary text-xs p-4 rounded-2xl border border-primary/20 mb-6 max-w-xs mx-auto animate-in fade-in slide-in-from-bottom-2 duration-700 backdrop-blur-sm pointer-events-auto"> | ||
| Waiting for Desktop stream... | ||
| </div> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded user-facing strings should be externalized for i18n.
Strings like "Mirror Standby", "Connecting...", "Start Screen Share", "Tip: Sharing...", and "Waiting for Desktop stream..." should be externalized to resource files.
As per coding guidelines: "User-visible strings should be externalized to resource files (i18n)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` around lines 80 - 107, The
ScreenMirror component contains hardcoded user-facing strings (e.g., "Mirror
Standby", "Connecting...", "Start Screen Share", the Tip text, and "Waiting for
Desktop stream..."); replace these literals with i18n resource lookups (e.g.,
use a translation hook or t function) and move the strings into your locale
resource files, updating usages inside the JSX conditional blocks that reference
status and canShare and the Start button handler (startSharing) to call the
translator (e.g., t('screenMirror.mirrorStandby'), t('screenMirror.connecting'),
t('screenMirror.startScreenShare'), t('screenMirror.tipSharingEntireScreen'),
t('screenMirror.waitingForDesktop')). Ensure keys are added to the resource
files for all supported locales and update any tests or snapshots that depend on
the original text.
Comment on lines
+117
to
+132
| {/* Sharing state overlay */} | ||
| {isSharing && ( | ||
| <div className="absolute top-4 left-4 z-30 flex items-center gap-2"> | ||
| <div className="badge badge-error gap-2 p-3"> | ||
| <span className="w-2 h-2 rounded-full bg-white animate-pulse" /> | ||
| LIVE SHARING | ||
| </div> | ||
| <button | ||
| type="button" | ||
| onClick={stopSharing} | ||
| className="btn btn-xs btn-outline btn-error bg-black/40 backdrop-blur-sm" | ||
| > | ||
| Stop | ||
| </button> | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Additional hardcoded strings: "LIVE SHARING" and "Stop".
These user-facing strings should also be externalized for i18n consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` around lines 117 - 132, In the
ScreenMirror component replace the hardcoded user strings "LIVE SHARING" and
"Stop" (rendered when isSharing is true and the stopSharing button) with i18n
keys using your project's translation helper (e.g., useTranslation()/t) — add
keys like "screenMirror.liveSharing" and "screenMirror.stop" to your locale
files and call t('screenMirror.liveSharing') and t('screenMirror.stop') where
the badge text and button label are rendered so strings are externalized while
still invoking stopSharing on click.
Comment on lines
+55
to
+56
| const originalConsoleLog = console.log | ||
| const originalConsoleError = console.error |
Contributor
There was a problem hiding this comment.
Unused variables storing original console methods.
originalConsoleLog and originalConsoleError are declared but never referenced after assignment. If the intent is to allow restoring original console behavior, consider exporting a restore function. Otherwise, remove these declarations.
💡 Proposed fix — remove unused variables
-const originalConsoleLog = console.log
-const originalConsoleError = console.error
-
console.log = (...args: unknown[]) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const originalConsoleLog = console.log | |
| const originalConsoleError = console.error | |
| console.log = (...args: unknown[]) => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/logger.ts` around lines 55 - 56, The variables originalConsoleLog
and originalConsoleError in src/utils/logger.ts are assigned but never used;
either remove those two declarations to eliminate dead code, or keep them and
implement/export a restore function (e.g., restoreOriginalConsole or
restoreConsole) that reassigns console.log = originalConsoleLog and
console.error = originalConsoleError and export that function so the originals
are actually referenced; update any related wrapper logic in the file (the code
that replaces console methods) to use the originals when restoring.
5eb1a89 to
fe77a78
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/settings.tsx (1)
1-8:⚠️ Potential issue | 🟡 MinorMissing
"use client"directive.Per coding guidelines, ensure
"use client"is used for client-side React components in NextJS. This file uses hooks (useState,useEffect) and browser APIs (window,localStorage,WebSocket).💡 Proposed fix
+"use client" + import { createFileRoute } from "@tanstack/react-router" import QRCode from "qrcode"As per coding guidelines:
"**/*.{ts,tsx,js,jsx}": 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/settings.tsx` around lines 1 - 8, This file defines a client-side React component (SettingsPage) and uses hooks and browser APIs but lacks the Next.js "use client" directive; add the exact string "use client" as the very first line of the file (before any imports) so the SettingsPage/Route module is treated as a client component and can safely use useState, useEffect, window/localStorage/WebSocket.
♻️ Duplicate comments (8)
src/hooks/useTrackpadGesture.ts (1)
40-40:⚠️ Potential issue | 🟠 MajorPending
draggingTimeoutcan fire after unmount.The
draggingTimeoutref (line 40) is used inhandleTouchEnd(lines 257-260) but there's no cleanup effect to clear it on unmount. This can cause stale click-up events.💡 Proposed fix — add cleanup effect
+import { useEffect, useRef, useState } from "react" -import { useRef, useState } from "react" import type { InputMessage } from "../server/InputHandler" @@ const draggingTimeout = useRef<NodeJS.Timeout | null>(null) + + useEffect(() => { + return () => { + if (draggingTimeout.current) { + clearTimeout(draggingTimeout.current) + draggingTimeout.current = null + } + } + }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useTrackpadGesture.ts` at line 40, The draggingTimeout ref in useTrackpadGesture can fire after the component unmounts; add a cleanup effect in the useTrackpadGesture hook that runs on unmount (useEffect with empty deps) to check draggingTimeout.current, call clearTimeout on it if non-null, and set draggingTimeout.current = null so handleTouchEnd and any pending timeout callbacks won't run against an unmounted component; locate the draggingTimeout ref and handleTouchEnd function to implement this cleanup.src/server/InputHandler.ts (1)
89-91:⚠️ Potential issue | 🟡 MinorMisleading log text: "pending move event" should be "pending scroll event".
This is within the scroll throttle branch, but the error message references "move event".
💡 Proposed fix
this.handleMessage(pending).catch((err) => { - logger.error("Error processing pending move event: %O", err) + logger.error("Error processing pending scroll event: %O", err) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/InputHandler.ts` around lines 89 - 91, The log message inside the scroll throttle branch incorrectly says "pending move event"; update the error text to "pending scroll event" where the code calls this.handleMessage(pending).catch((err) => logger.error(...)) so the logger.error call reflects the correct context (e.g., logger.error("Error processing pending scroll event: %O", err)). Ensure only the message string is changed and retain the error object formatting and handling in the this.handleMessage(pending) promise chain.src/server/websocket.ts (2)
130-146:⚠️ Potential issue | 🟠 MajorBinary frame relay lacks size validation — potential DoS vector.
Provider clients can push oversized binary payloads that get fanned out to all consumers without any size limit check.
💡 Proposed fix — add frame size limit
+const MAX_FRAME_PAYLOAD_SIZE = 2 * 1024 * 1024 // 2MB per frame + ws.on("message", async (data: WebSocket.RawData, isBinary: boolean) => { try { if (isBinary) { + const frameSize = Buffer.isBuffer(data) + ? data.length + : Array.isArray(data) + ? data.reduce((total, chunk) => total + chunk.length, 0) + : (data as ArrayBuffer).byteLength + if (frameSize > MAX_FRAME_PAYLOAD_SIZE) { + logger.warn(`Binary payload too large (${frameSize} bytes), dropping frame`) + return + } // Relay frames from Providers to Consumers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 130 - 146, The binary-frame relay in the ws.on("message") handler forwards unchecked data from providers to all consumers, so add a maximum size check (e.g., MAX_BINARY_FRAME_SIZE) before relaying: in the isBinary branch of ws.on("message") (and referencing ExtWebSocket.isProvider, isConsumer and wss.clients) validate Buffer.byteLength(data) against the limit, and if it exceeds the limit, reject the frame (do not fan out), log the event via your logger, and either ignore the frame or terminate/close the offending provider connection with an appropriate close code; only proceed to iterate wss.clients and client.send(data, { binary: true }) when the size is within limits.
114-114:⚠️ Potential issue | 🟠 Major
connectedpayload missingplatformfield.The client (
useRemoteConnection.ts) expectsmsg.platformon the"connected"message, but this payload only sendsserverIp.💡 Proposed fix
-ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP })) +ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP, platform: process.platform }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 114, The "connected" message sent via ws.send currently only includes serverIp (ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP }))) but the client (useRemoteConnection.ts) expects msg.platform; modify the payload to include a platform field (e.g., platform: os.platform() or the existing server platform constant) so the JSON becomes { type: "connected", serverIp: LAN_IP, platform: <server platform> }. Ensure you import/obtain the platform value in websocket.ts and include it in the ws.send call.src/hooks/useCaptureProvider.ts (2)
147-149:⚠️ Potential issue | 🟡 MinorSilent catch block hides canvas/encoding errors.
The catch block only resets
isBusywithout logging. This can hide issues withdrawImageortoBlobthat would be useful for debugging.💡 Proposed fix
} catch (err) { isBusy.current = false + console.error("Frame capture error:", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCaptureProvider.ts` around lines 147 - 149, The catch block that only resets isBusy.current hides errors from canvas operations (e.g., drawImage/toBlob) — update the error handling in the try/catch inside useCaptureProvider (the block that sets isBusy.current) to log the caught error (include err or err.message) using the existing logger or console.error and optionally surface it (e.g., set an error state or rethrow) so debugging information is preserved; ensure you reference the same isBusy.current handling and do not swallow the error silently.
152-199: 🧹 Nitpick | 🔵 Trivial
startSharingshould be wrapped inuseCallback.This function is returned from the hook and may be passed to child components. Without
useCallback, it creates a new reference on every render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCaptureProvider.ts` around lines 152 - 199, Wrap the startSharing function in a React useCallback to stabilize its reference when returned from the hook; locate startSharing in useCaptureProvider and replace the inline async function with useCallback(async () => { ... }, [wsRef.current, workerRef.current, setIsSharing, stopSharing]) (include any other reactive values used inside such as refs/state setters) so the hook returns a memoized function and avoids recreating the callback on every render.src/routes/settings.tsx (1)
306-319:⚠️ Potential issue | 🟡 MinorHardcoded user-facing alert strings should be externalized for i18n.
The alert messages ("Please enter a valid port number" and "Failed to connect to the server") are hardcoded. Per coding guidelines, user-visible strings should be externalized to resource files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 306 - 319, The two hardcoded user-facing strings in the port validation and WebSocket error handlers must be moved to the app's i18n resources and consumed via the localization API instead of alert literals: replace the alert("Please enter a valid port number (1–65535).") in the port validation block that reads frontendPort and replace alert("Failed to connect to the server.") inside socket.onerror so they call the project translation function (e.g., useTranslation().t or i18n.t) or a getString helper, add keys like "settings.invalidPort" and "settings.wsConnectFailed" to the resource files, and import/use the translation hook or helper at the top of the settings component so socket and validation use the localized messages.src/utils/logger.ts (1)
55-56: 🧹 Nitpick | 🔵 TrivialUnused variables
originalConsoleLogandoriginalConsoleErrorremain unreferenced.These variables are declared but never used. Either implement a restore function that uses them, or remove the declarations to eliminate dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger.ts` around lines 55 - 56, The variables originalConsoleLog and originalConsoleError are declared but never used; either remove these dead declarations or implement a restore utility that reassigns console.log and console.error back to those originals. If you choose the restore approach, add a function (e.g., restoreConsole or restoreOriginalConsole) that sets console.log = originalConsoleLog and console.error = originalConsoleError and export it (or call it where appropriate); otherwise simply delete the originalConsoleLog and originalConsoleError declarations to eliminate unused variables.
🤖 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/components/Trackpad/ScreenMirror.tsx`:
- Around line 115-119: The hardcoded badge text "SCROLL Active" in the
ScreenMirror component should be replaced with an i18n lookup: update the JSX
where scrollMode is rendered (inside the ScreenMirror component) to call your
localization helper (e.g., t('trackpad.scrollActive') or the project's
translation function) instead of the literal string, and add the corresponding
key ("trackpad.scrollActive": "SCROLL Active") to the locale/resource files for
all supported languages; ensure the import/usage of the translation function (or
useTranslation hook) is added to ScreenMirror if not already present.
In `@src/hooks/useCaptureProvider.ts`:
- Around line 47-55: The cleanup effect currently references stopSharing (and
workerRef/isMounted) but omits stopSharing from the dependency array; to fix,
either move the stopSharing function declaration above the useEffect so it is
stable when referenced, or store stopSharing in a ref (e.g.,
stopSharingRef.current = stopSharing) and call that ref inside the cleanup, then
keep the dependency array empty; ensure the cleanup uses isMounted.current,
stopSharingRef.current (or the moved stopSharing), and workerRef.current
consistently so the linter stops warning and behavior remains unchanged.
- Around line 57-84: The effect currently auto-starts screen sharing by
attaching global click/touchstart listeners and omits startSharing from its
dependency array; change this to remove implicit global gesture listeners and
require an explicit user action (e.g., only attach listeners when an explicit
opt-in flag or prop like allowAutoStart is true, or better yet remove the
listeners and call startSharing from a UI button handler), and add startSharing
to the useEffect dependency list to avoid stale closures; update references to
isSharingRef, wsRef, status, and the navigator.mediaDevices?.getDisplayMedia
check accordingly so the auto-start path only runs when explicitly permitted and
uses the latest startSharing function.
- Line 1: The file is missing the "use client" directive required for hooks that
access browser APIs; add the exact string "use client" as the first line of
src/hooks/useCaptureProvider.ts (before any imports) so the module runs as a
client component and permits use of navigator.mediaDevices,
document.createElement, window events and the hook implementation (e.g.,
useCaptureProvider, any exported functions) to work correctly.
- Around line 167-170: Replace the unsafe any cast used when calling
navigator.mediaDevices.getDisplayMedia in useCaptureProvider: define a specific
type (e.g., ExtendedDisplayMediaOptions that extends DisplayMediaStreamOptions
with selfBrowserSurface, monitorTypeSurfaces, systemAudio) or use unknown with a
type assertion to that extended type, then pass that typed variable into
getDisplayMedia so the call is strongly typed instead of casting constraints as
any; update the parameter/variable name (constraints) and the call site to use
the new explicit type.
In `@src/hooks/useRemoteConnection.ts`:
- Around line 52-63: In useRemoteConnection, the socket.onmessage handler
swallows JSON parse errors and isn't removed during cleanup; update the handler
to catch exceptions but log them at debug/verbose level (including the
event.data and error) instead of silently ignoring, and ensure socket.onmessage
is set to null in the effect cleanup alongside socket.onopen/socket.onclose to
prevent stale handlers; locate the onmessage setup in useRemoteConnection and
the effect cleanup block and apply these changes to maintain setPlatform
behavior while surfacing parse errors for debugging.
In `@src/routes/trackpad.tsx`:
- Around line 99-116: The delayed reset of isComposingRef in
handleCompositionEnd uses setTimeout(..., 0) which introduces a macrotask race
window; replace that with a microtask-based reset to deterministically clear the
composing flag after the current event handlers finish. In handleCompositionEnd,
change the setTimeout call to queueMicrotask(() => { isComposingRef.current =
false }) (or Promise.resolve().then(() => { isComposingRef.current = false }))
so checks in handleInput or other synchronous code after the compositionend
event see the correct state; keep the existing logic that clears
hiddenInputRef.current and updates prevCompositionDataRef.current.
In `@src/server/websocket.ts`:
- Around line 120-128: The leading semicolons before the casted assignments in
startMirror and stopMirror are unconventional; instead remove the semicolons and
avoid ASI risk by making the cast explicit once and reusing it — e.g. create a
local variable (ExtWebSocket) like const ext = ws as ExtWebSocket and then set
ext.isConsumer = true in startMirror and ext.isConsumer = false in stopMirror
(symbols: startMirror, stopMirror, ExtWebSocket, ws); this eliminates the need
for semicolon-prefixed statements and clarifies intent.
---
Outside diff comments:
In `@src/routes/settings.tsx`:
- Around line 1-8: This file defines a client-side React component
(SettingsPage) and uses hooks and browser APIs but lacks the Next.js "use
client" directive; add the exact string "use client" as the very first line of
the file (before any imports) so the SettingsPage/Route module is treated as a
client component and can safely use useState, useEffect,
window/localStorage/WebSocket.
---
Duplicate comments:
In `@src/hooks/useCaptureProvider.ts`:
- Around line 147-149: The catch block that only resets isBusy.current hides
errors from canvas operations (e.g., drawImage/toBlob) — update the error
handling in the try/catch inside useCaptureProvider (the block that sets
isBusy.current) to log the caught error (include err or err.message) using the
existing logger or console.error and optionally surface it (e.g., set an error
state or rethrow) so debugging information is preserved; ensure you reference
the same isBusy.current handling and do not swallow the error silently.
- Around line 152-199: Wrap the startSharing function in a React useCallback to
stabilize its reference when returned from the hook; locate startSharing in
useCaptureProvider and replace the inline async function with useCallback(async
() => { ... }, [wsRef.current, workerRef.current, setIsSharing, stopSharing])
(include any other reactive values used inside such as refs/state setters) so
the hook returns a memoized function and avoids recreating the callback on every
render.
In `@src/hooks/useTrackpadGesture.ts`:
- Line 40: The draggingTimeout ref in useTrackpadGesture can fire after the
component unmounts; add a cleanup effect in the useTrackpadGesture hook that
runs on unmount (useEffect with empty deps) to check draggingTimeout.current,
call clearTimeout on it if non-null, and set draggingTimeout.current = null so
handleTouchEnd and any pending timeout callbacks won't run against an unmounted
component; locate the draggingTimeout ref and handleTouchEnd function to
implement this cleanup.
In `@src/routes/settings.tsx`:
- Around line 306-319: The two hardcoded user-facing strings in the port
validation and WebSocket error handlers must be moved to the app's i18n
resources and consumed via the localization API instead of alert literals:
replace the alert("Please enter a valid port number (1–65535).") in the port
validation block that reads frontendPort and replace alert("Failed to connect to
the server.") inside socket.onerror so they call the project translation
function (e.g., useTranslation().t or i18n.t) or a getString helper, add keys
like "settings.invalidPort" and "settings.wsConnectFailed" to the resource
files, and import/use the translation hook or helper at the top of the settings
component so socket and validation use the localized messages.
In `@src/server/InputHandler.ts`:
- Around line 89-91: The log message inside the scroll throttle branch
incorrectly says "pending move event"; update the error text to "pending scroll
event" where the code calls this.handleMessage(pending).catch((err) =>
logger.error(...)) so the logger.error call reflects the correct context (e.g.,
logger.error("Error processing pending scroll event: %O", err)). Ensure only the
message string is changed and retain the error object formatting and handling in
the this.handleMessage(pending) promise chain.
In `@src/server/websocket.ts`:
- Around line 130-146: The binary-frame relay in the ws.on("message") handler
forwards unchecked data from providers to all consumers, so add a maximum size
check (e.g., MAX_BINARY_FRAME_SIZE) before relaying: in the isBinary branch of
ws.on("message") (and referencing ExtWebSocket.isProvider, isConsumer and
wss.clients) validate Buffer.byteLength(data) against the limit, and if it
exceeds the limit, reject the frame (do not fan out), log the event via your
logger, and either ignore the frame or terminate/close the offending provider
connection with an appropriate close code; only proceed to iterate wss.clients
and client.send(data, { binary: true }) when the size is within limits.
- Line 114: The "connected" message sent via ws.send currently only includes
serverIp (ws.send(JSON.stringify({ type: "connected", serverIp: LAN_IP }))) but
the client (useRemoteConnection.ts) expects msg.platform; modify the payload to
include a platform field (e.g., platform: os.platform() or the existing server
platform constant) so the JSON becomes { type: "connected", serverIp: LAN_IP,
platform: <server platform> }. Ensure you import/obtain the platform value in
websocket.ts and include it in the ws.send call.
In `@src/utils/logger.ts`:
- Around line 55-56: The variables originalConsoleLog and originalConsoleError
are declared but never used; either remove these dead declarations or implement
a restore utility that reassigns console.log and console.error back to those
originals. If you choose the restore approach, add a function (e.g.,
restoreConsole or restoreOriginalConsole) that sets console.log =
originalConsoleLog and console.error = originalConsoleError and export it (or
call it where appropriate); otherwise simply delete the originalConsoleLog and
originalConsoleError declarations to eliminate unused variables.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/components/Trackpad/ControlBar.tsxsrc/components/Trackpad/ScreenMirror.tsxsrc/hooks/useCaptureProvider.tssrc/hooks/useRemoteConnection.tssrc/hooks/useTrackpadGesture.tssrc/routes/settings.tsxsrc/routes/trackpad.tsxsrc/server/InputHandler.tssrc/server/tokenStore.tssrc/server/websocket.tssrc/utils/logger.ts
💤 Files with no reviewable changes (1)
- src/components/Trackpad/ControlBar.tsx
Comment on lines
+115
to
+119
| {scrollMode && ( | ||
| <div className="absolute top-4 right-4 badge badge-info z-10"> | ||
| SCROLL Active | ||
| </div> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded string: "SCROLL Active".
This user-facing badge text should be externalized to resource files for i18n.
As per coding guidelines: "User-visible strings should be externalized to resource files (i18n)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Trackpad/ScreenMirror.tsx` around lines 115 - 119, The
hardcoded badge text "SCROLL Active" in the ScreenMirror component should be
replaced with an i18n lookup: update the JSX where scrollMode is rendered
(inside the ScreenMirror component) to call your localization helper (e.g.,
t('trackpad.scrollActive') or the project's translation function) instead of the
literal string, and add the corresponding key ("trackpad.scrollActive": "SCROLL
Active") to the locale/resource files for all supported languages; ensure the
import/usage of the translation function (or useTranslation hook) is added to
ScreenMirror if not already present.
| @@ -0,0 +1,231 @@ | |||
| import { useCallback, useEffect, useRef, useState } from "react" | |||
Contributor
There was a problem hiding this comment.
Missing "use client" directive.
This hook uses browser APIs (navigator.mediaDevices, document.createElement, window events) and should have the "use client" directive per coding guidelines.
💡 Proposed fix
+"use client"
+
import { useCallback, useEffect, useRef, useState } from "react"As per coding guidelines: "**/*.{ts,tsx,js,jsx}": Ensure that "use client" is being used.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { useCallback, useEffect, useRef, useState } from "react" | |
| "use client" | |
| import { useCallback, useEffect, useRef, useState } from "react" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useCaptureProvider.ts` at line 1, The file is missing the "use
client" directive required for hooks that access browser APIs; add the exact
string "use client" as the first line of src/hooks/useCaptureProvider.ts (before
any imports) so the module runs as a client component and permits use of
navigator.mediaDevices, document.createElement, window events and the hook
implementation (e.g., useCaptureProvider, any exported functions) to work
correctly.
Comment on lines
+47
to
+55
| return () => { | ||
| isMounted.current = false | ||
| stopSharing() | ||
| if (workerRef.current) { | ||
| workerRef.current.terminate() | ||
| workerRef.current = null | ||
| } | ||
| } | ||
| }, []) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing dependencies in cleanup effect.
The cleanup effect calls stopSharing (line 49), but stopSharing is not in the dependency array. While this works due to hoisting and the ref-based implementation, it's cleaner to define stopSharing before this effect or use a ref to avoid the lint warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useCaptureProvider.ts` around lines 47 - 55, The cleanup effect
currently references stopSharing (and workerRef/isMounted) but omits stopSharing
from the dependency array; to fix, either move the stopSharing function
declaration above the useEffect so it is stable when referenced, or store
stopSharing in a ref (e.g., stopSharingRef.current = stopSharing) and call that
ref inside the cleanup, then keep the dependency array empty; ensure the cleanup
uses isMounted.current, stopSharingRef.current (or the moved stopSharing), and
workerRef.current consistently so the linter stops warning and behavior remains
unchanged.
Comment on lines
+57
to
+84
| // Diagnostics/Re-registration + Auto-start attempt | ||
| useEffect(() => { | ||
| if (status === "connected" && isSharingRef.current && wsRef.current) { | ||
| wsRef.current.send(JSON.stringify({ type: "start-provider" })) | ||
| } | ||
|
|
||
| // Auto-start logic: listen for the FIRST user gesture to trigger sharing | ||
| const canShareImplicitly = | ||
| typeof navigator !== "undefined" && | ||
| !!navigator.mediaDevices?.getDisplayMedia && | ||
| !isSharingRef.current | ||
|
|
||
| if (canShareImplicitly && status === "connected") { | ||
| const trigger = () => { | ||
| if (!isSharingRef.current) { | ||
| startSharing() | ||
| } | ||
| window.removeEventListener("click", trigger) | ||
| window.removeEventListener("touchstart", trigger) | ||
| } | ||
| window.addEventListener("click", trigger) | ||
| window.addEventListener("touchstart", trigger) | ||
| return () => { | ||
| window.removeEventListener("click", trigger) | ||
| window.removeEventListener("touchstart", trigger) | ||
| } | ||
| } | ||
| }, [status, wsRef]) |
Contributor
There was a problem hiding this comment.
Auto-start gesture listeners may cause unintended screen share prompts.
The effect adds click/touchstart listeners that auto-trigger startSharing on the first user interaction when connected. This could surprise users who click anywhere on the page. Consider requiring an explicit button click instead.
Also, startSharing is not in the dependency array, which could cause stale closure issues if startSharing were to change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useCaptureProvider.ts` around lines 57 - 84, The effect currently
auto-starts screen sharing by attaching global click/touchstart listeners and
omits startSharing from its dependency array; change this to remove implicit
global gesture listeners and require an explicit user action (e.g., only attach
listeners when an explicit opt-in flag or prop like allowAutoStart is true, or
better yet remove the listeners and call startSharing from a UI button handler),
and add startSharing to the useEffect dependency list to avoid stale closures;
update references to isSharingRef, wsRef, status, and the
navigator.mediaDevices?.getDisplayMedia check accordingly so the auto-start path
only runs when explicitly permitted and uses the latest startSharing function.
Comment on lines
+167
to
+170
| const stream = await navigator.mediaDevices.getDisplayMedia( | ||
| // biome-ignore lint/suspicious/noExplicitAny: Newer MediaDevices properties not in local typings | ||
| constraints as any, | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid any cast — use a more specific type or type assertion.
Per coding guidelines, avoid using any. The cast works around missing typings for newer DisplayMediaStreamOptions properties.
💡 Proposed fix — use unknown with assertion
const stream = await navigator.mediaDevices.getDisplayMedia(
- // biome-ignore lint/suspicious/noExplicitAny: Newer MediaDevices properties not in local typings
- constraints as any,
+ constraints as DisplayMediaStreamOptions,
)Alternatively, extend the type definition locally:
interface ExtendedDisplayMediaOptions extends DisplayMediaStreamOptions {
selfBrowserSurface?: "include" | "exclude"
monitorTypeSurfaces?: "include" | "exclude"
systemAudio?: "include" | "exclude"
}As per coding guidelines: "Avoid 'any', use explicit types".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stream = await navigator.mediaDevices.getDisplayMedia( | |
| // biome-ignore lint/suspicious/noExplicitAny: Newer MediaDevices properties not in local typings | |
| constraints as any, | |
| ) | |
| const stream = await navigator.mediaDevices.getDisplayMedia( | |
| constraints as DisplayMediaStreamOptions, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useCaptureProvider.ts` around lines 167 - 170, Replace the unsafe
any cast used when calling navigator.mediaDevices.getDisplayMedia in
useCaptureProvider: define a specific type (e.g., ExtendedDisplayMediaOptions
that extends DisplayMediaStreamOptions with selfBrowserSurface,
monitorTypeSurfaces, systemAudio) or use unknown with a type assertion to that
extended type, then pass that typed variable into getDisplayMedia so the call is
strongly typed instead of casting constraints as any; update the
parameter/variable name (constraints) and the call site to use the new explicit
type.
Comment on lines
+52
to
+63
|
|
||
| socket.onmessage = (event) => { | ||
| if (!isMounted || typeof event.data !== "string") return | ||
| try { | ||
| const msg = JSON.parse(event.data) | ||
| if (msg.type === "connected") { | ||
| setPlatform(msg.platform) | ||
| } | ||
| } catch (e) { | ||
| // Ignore non-json or binary messages handled by other hooks | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent catch block hides JSON parsing errors.
While the comment indicates ignoring non-JSON messages is intentional, completely swallowing errors can hide unexpected issues. Consider logging at debug level.
Also, socket.onmessage should be nullified in the cleanup function (lines 86-93) alongside the other handlers to prevent potential memory leaks or stale handler invocations.
💡 Proposed fixes
socket.onmessage = (event) => {
if (!isMounted || typeof event.data !== "string") return
try {
const msg = JSON.parse(event.data)
if (msg.type === "connected") {
setPlatform(msg.platform)
}
} catch (e) {
- // Ignore non-json or binary messages handled by other hooks
+ // Non-JSON or binary messages handled by other hooks (e.g., useMirrorStream)
}
}Add to cleanup:
if (wsRef.current) {
wsRef.current.onopen = null
wsRef.current.onclose = null
wsRef.current.onerror = null
+ wsRef.current.onmessage = null
wsRef.current.close()
wsRef.current = null
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useRemoteConnection.ts` around lines 52 - 63, In
useRemoteConnection, the socket.onmessage handler swallows JSON parse errors and
isn't removed during cleanup; update the handler to catch exceptions but log
them at debug/verbose level (including the event.data and error) instead of
silently ignoring, and ensure socket.onmessage is set to null in the effect
cleanup alongside socket.onopen/socket.onclose to prevent stale handlers; locate
the onmessage setup in useRemoteConnection and the effect cleanup block and
apply these changes to maintain setPlatform behavior while surfacing parse
errors for debugging.
Comment on lines
+99
to
116
| const handleCompositionEnd = ( | ||
| e: React.CompositionEvent<HTMLInputElement>, | ||
| ) => { | ||
| // Fallback to target value if data is empty (some mobile browsers) | ||
| const currentData = e.data || e.currentTarget.value || "" | ||
| processCompositionDiff(currentData, prevCompositionDataRef.current) | ||
| prevCompositionDataRef.current = "" | ||
|
|
||
| // Clear input to prevent buffer accumulation | ||
| if (hiddenInputRef.current) { | ||
| hiddenInputRef.current.value = "" | ||
| } | ||
|
|
||
| resetInput() | ||
| // Delay flag reset so the onChange firing after compositionend is suppressed | ||
| setTimeout(() => { | ||
| isComposingRef.current = false | ||
| }, 0) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential race condition between compositionEnd and delayed flag reset.
The setTimeout(() => { isComposingRef.current = false }, 0) creates a brief window where the flag is still true but composition has ended. If handleInput fires synchronously before the timeout callback, input is correctly suppressed. However, if external code or future refactors check isComposingRef after the event but before the microtask, they may see stale state.
Consider using a more deterministic approach:
💡 Suggested improvement
const handleCompositionEnd = (
e: React.CompositionEvent<HTMLInputElement>,
) => {
const currentData = e.data || e.currentTarget.value || ""
processCompositionDiff(currentData, prevCompositionDataRef.current)
prevCompositionDataRef.current = ""
if (hiddenInputRef.current) {
hiddenInputRef.current.value = ""
}
- setTimeout(() => {
- isComposingRef.current = false
- }, 0)
+ // Use queueMicrotask for more predictable timing after onChange
+ queueMicrotask(() => {
+ isComposingRef.current = false
+ })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trackpad.tsx` around lines 99 - 116, The delayed reset of
isComposingRef in handleCompositionEnd uses setTimeout(..., 0) which introduces
a macrotask race window; replace that with a microtask-based reset to
deterministically clear the composing flag after the current event handlers
finish. In handleCompositionEnd, change the setTimeout call to queueMicrotask(()
=> { isComposingRef.current = false }) (or Promise.resolve().then(() => {
isComposingRef.current = false })) so checks in handleInput or other synchronous
code after the compositionend event see the correct state; keep the existing
logic that clears hiddenInputRef.current and updates
prevCompositionDataRef.current.
Comment on lines
+120
to
+128
| const startMirror = () => { | ||
| ;(ws as ExtWebSocket).isConsumer = true | ||
| logger.info("Client registered as Screen Consumer") | ||
| } | ||
|
|
||
| const stopMirror = () => { | ||
| ;(ws as ExtWebSocket).isConsumer = false | ||
| logger.info("Client unregistered as Screen Consumer") | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unusual semicolon-prefixed statements.
Lines 121 and 126 use ;(ws as ExtWebSocket)... which is valid but unconventional. This pattern prevents ASI issues when the previous line could be interpreted as a function call. Consider adding a brief comment or restructuring.
♻️ Alternative approach
const startMirror = () => {
- ;(ws as ExtWebSocket).isConsumer = true
+ const extWs = ws as ExtWebSocket
+ extWs.isConsumer = true
logger.info("Client registered as Screen Consumer")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const startMirror = () => { | |
| ;(ws as ExtWebSocket).isConsumer = true | |
| logger.info("Client registered as Screen Consumer") | |
| } | |
| const stopMirror = () => { | |
| ;(ws as ExtWebSocket).isConsumer = false | |
| logger.info("Client unregistered as Screen Consumer") | |
| } | |
| const startMirror = () => { | |
| const extWs = ws as ExtWebSocket | |
| extWs.isConsumer = true | |
| logger.info("Client registered as Screen Consumer") | |
| } | |
| const stopMirror = () => { | |
| ;(ws as ExtWebSocket).isConsumer = false | |
| logger.info("Client unregistered as Screen Consumer") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/websocket.ts` around lines 120 - 128, The leading semicolons
before the casted assignments in startMirror and stopMirror are unconventional;
instead remove the semicolons and avoid ASI risk by making the cast explicit
once and reusing it — e.g. create a local variable (ExtWebSocket) like const ext
= ws as ExtWebSocket and then set ext.isConsumer = true in startMirror and
ext.isConsumer = false in stopMirror (symbols: startMirror, stopMirror,
ExtWebSocket, ws); this eliminates the need for semicolon-prefixed statements
and clarifies intent.
fe77a78 to
6b7bf60
Compare
Contributor
|
@Rozerxshashank Keep this PR as it is and raise a separate one with WebRTC. In the future, if we consider this, we'll minimize the existing complexity and shift to WebP for better quality and move screen mirror auto-start to _root.tsx so the mirror doesn't stop when we switch to a different page. Make it modular for easy WebRTC swapping in the future. And consider PR from a new branch |
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 #132
Description
This PR implements high-performance screen mirroring and remote control functionality via the Web MediaDevices API and Nut-JS
Key Features:
Screen Mirroring: Real-time screen capture with server-side downscaling and adaptive frame rates.
Congestion Control: Implemented a feedback loop to adjust capture frequency based on network performance, preventing buffer bloat.
Improved Trackpad Gestures : Smooth pinch-to-zoom support, adjustable scroll sensitivity, accurate single and multi-touch translation.
Functional Verification
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.
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.
Backspace doesn't send the previous input.
Any other gesture or input behavior introduced:
Additional Notes:
TO activate screen share you needed to go to rein teackpad on your PC and then click on share screen button to give permission.
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 contributing guidlines
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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor