feat: add phone remote camera and microphone#426
Open
xsourabhsharma wants to merge 4 commits intowebadderallorg:mainfrom
Open
feat: add phone remote camera and microphone#426xsourabhsharma wants to merge 4 commits intowebadderallorg:mainfrom
xsourabhsharma wants to merge 4 commits intowebadderallorg:mainfrom
Conversation
Add a phone-to-laptop remote media workflow for recording the laptop screen with phone camera overlay and phone microphone audio. Refs webadderallorg#418
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a phone-remote WebRTC system: types and renderer API, an in-memory session store, an HTTP bridge (optional cloudflared tunnel) and mobile client page, main-process IPC + preload wiring, a renderer hook and LaunchWindow integration for remote media, QR encoder, FFmpeg diagnostics fallback, tests, CSS, and an Electron build entry tweak. ChangesPhone Remote (single cohesive DAG)
Utilities, Tests & Build
Sequence DiagramsequenceDiagram
participant Phone as Mobile Browser (Phone Client)
participant Tunnel as Cloudflared Tunnel
participant Server as Electron HTTP Server
participant Store as Session Store
participant Renderer as Renderer Process
participant AppPeer as App Recording/Peer
Phone->>Server: GET /phone (mobile page)
Server-->>Phone: HTML + JS (mobile client)
Phone->>Phone: getUserMedia / acquire tracks
Phone->>Server: POST /api/phone-remote/session { code? }
Server->>Store: lookup/create session
Store-->>Server: session public payload
Server-->>Phone: { session, joinUrl, ... }
Phone->>Phone: create RTCPeerConnection + local tracks
Phone->>Server: POST /api/phone-remote/signal (offer / ice)
Server->>Store: addPhoneSignal()
Store->>Renderer: emit "phone-remote-signal"
Renderer->>AppPeer: forward signal via IPC to in-app peer
AppPeer->>Server: phone-remote:send-signal (answer / ice)
Server->>Store: addLaptopSignal()
Store->>Renderer: emit "phone-remote-signal"
Renderer-->>Phone: forwarded answer/ice via server polling
Phone->>Server: GET /api/phone-remote/signals?after=idx
Server->>Store: getLaptopSignalsSince(after)
Store-->>Server: [signals], nextIndex
Server-->>Phone: { signals, nextIndex }
Phone->>Phone: apply remote descriptions / candidates
Phone->>Server: POST /api/phone-remote/status {status, hasAudio,...}
Server->>Store: updatePhoneRemoteStatus()
Store->>Renderer: emit "phone-remote-status"
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
electron/ipc/recording/diagnostics.ts (1)
222-224: ⚡ Quick winHarden missing-FFmpeg detection to avoid message-coupling.
On Line 223, fallback is keyed to a specific message string. If
getFfmpegBinaryPath()wording changes, this path silently stops working and dev recordings regress. Prefer a tolerant match (or a structured error code from the source).Proposed localized tweak
- const message = error instanceof Error ? error.message : String(error); - if (message.includes("FFmpeg binary is unavailable")) { + const message = error instanceof Error ? error.message : String(error); + if (/ffmpeg\s+binary\s+is\s+unavailable/i.test(message)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/recording/diagnostics.ts` around lines 222 - 224, The check in diagnostics.ts that detects missing FFmpeg is tightly coupled to an exact message string; update the detection logic around the block that builds "message" (where it currently does message.includes("FFmpeg binary is unavailable")) to use a tolerant match or structured code: first, if the thrown error exposes a code (e.g., error.code === 'FFMPEG_UNAVAILABLE' or similar) prefer that; otherwise replace the exact string check with a case-insensitive, fuzzy test such as testing for /ffmpeg/i and one of "unavail|missing|not found" (or using message.toLowerCase().includes("ffmpeg") && (message.includes("unavailable") || message.includes("not found") || message.includes("missing"))) so the detection still fires if wording changes.src/hooks/usePhoneRemoteMedia.ts (2)
334-338: 💤 Low valueConsider adding fallback ICE servers for reliability.
The peer connection uses only
stun:stun.l.google.com:19302. While this is a reliable public STUN server, having a single ICE server can cause connectivity failures if it's unreachable. Consider adding backup STUN servers or making the ICE server list configurable.♻️ Optional: Add backup STUN servers
const peer = new RTCPeerConnection({ - iceServers: [{ urls: "stun:stun.l.google.com:19302" }], + iceServers: [ + { urls: "stun:stun.l.google.com:19302" }, + { urls: "stun:stun1.l.google.com:19302" }, + { urls: "stun:stun2.l.google.com:19302" }, + ], });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/usePhoneRemoteMedia.ts` around lines 334 - 338, The RTCPeerConnection created in createPeer (inside the useCallback) currently uses only "stun:stun.l.google.com:19302"; update the ICE configuration to include additional fallback STUN servers or read the iceServers list from a configurable source (env/config) so connectivity is more robust: modify the RTCPeerConnection constructor call in createPeer to use an array of multiple STUN (and optionally TURN) entries or inject a configurable iceServers value so callers can override the list.
476-521: 💤 Low valueAsync effect callback may access stale refs after cleanup.
The
onPhoneRemoteSignalcallback is async, and if the effect's cleanup runs while the callback is mid-execution, subsequentpeerRef.currentaccess could reference a closed or null peer. While theif (!peer)guard at line 487 helps, the peer could be closed between the check and usage.This is a common pattern in React hooks and the current guards are reasonable, but for critical WebRTC operations you may want to track a mounted flag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/usePhoneRemoteMedia.ts` around lines 476 - 521, The async onPhoneRemoteSignal callback can observe stale/closed refs after the effect cleanup; to fix, introduce an "active" (or "mounted") boolean captured by the effect and flipped to false in its cleanup, and inside the callback bail out early if active is false (and/or capture local snapshots of sessionRef.current and peerRef.current into consts at the start of the callback and re-check active before doing any await-bound operations). Update the useEffect that registers window.electronAPI.onPhoneRemoteSignal (and the code paths handling "answer" and "ice-candidate") to check the active flag before calling peer.setRemoteDescription, peer.addIceCandidate, mutating pendingRemoteIceCandidatesRef, or calling setStatus/setStatusDetail; reference symbols: useEffect/onPhoneRemoteSignal, sessionRef, peerRef, pendingRemoteIceCandidatesRef, setStatus, setStatusDetail.electron/ipc/register/phoneRemote.ts (1)
84-101: 💤 Low valueHandler registration lacks idempotency guard.
The
subscribedflag only prevents duplicate store subscriptions, but theipcMain.handle()calls on lines 103, 127, and 131 will throw ifregisterPhoneRemoteHandlers()is invoked more than once. While the current call site inregisterIpcHandlers()appears to be invoked only once duringapp.whenReady(), the guard pattern is incomplete.Consider either wrapping the handler registrations in the same guard, or using
ipcMain.removeHandler()before re-registering to make this function idempotent.♻️ Optional: Make handler registration idempotent
export function registerPhoneRemoteHandlers() { if (!subscribed) { subscribed = true; subscribePhoneRemoteStore((event) => { // ... existing subscription code ... }); - } - ipcMain.handle("phone-remote:create-session", async (event) => { + ipcMain.handle("phone-remote:create-session", async (event) => { // ... - }); + }); - ipcMain.handle("phone-remote:end-session", (_event, sessionId: string) => { + ipcMain.handle("phone-remote:end-session", (_event, sessionId: string) => { // ... - }); + }); - ipcMain.handle("phone-remote:send-signal", (event, sessionId: string, value: unknown) => { + ipcMain.handle("phone-remote:send-signal", (event, sessionId: string, value: unknown) => { // ... - }); + }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/register/phoneRemote.ts` around lines 84 - 101, The handler registration in registerPhoneRemoteHandlers is not fully idempotent: the subscribed flag only guards subscribePhoneRemoteStore but ipcMain.handle registrations (the handlers registered in this function) will throw if called twice; update registerPhoneRemoteHandlers to either include the ipcMain.handle(...) calls inside the same subscribed guard (so they run only once alongside subscribePhoneRemoteStore) or call ipcMain.removeHandler for each IPC channel before calling ipcMain.handle (ensuring safe re-registration); reference registerPhoneRemoteHandlers, subscribed, subscribePhoneRemoteStore and the ipcMain.handle-registered channel names when applying the change.src/components/launch/LaunchWindow.tsx (2)
1070-1083: 💤 Low valueConsider a non-blocking confirmation for better UX.
Using
window.confirm()blocks the UI thread and can feel jarring. Consider using a toast with action buttons or a custom modal component for a smoother experience consistent with the rest of the UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 1070 - 1083, Replace the blocking window.confirm() in the LaunchWindow component with a non-blocking UI element (e.g., existing Modal or Toast with action buttons) so the thread is not blocked; locate the confirm call in the conditional that checks webcamSource === "phone" && webcamEnabled && !phoneRemote.videoActive && !phoneRemote.micActive, and instead render your modal/toast that shows the same message and callbacks: if the user confirms call the current logic (proceed with recording), otherwise call setActiveDropdown("webcam") and return/abort; ensure the modal’s confirm and cancel handlers mirror the existing behavior and keep state (e.g., showModal boolean) local to LaunchWindow or use the app’s shared modal/toast component.
1012-1023: 💤 Low valueSide-effect coupling in toggle function.
togglePhoneMicrophoneenables webcam and starts a phone session when phone mic is enabled without an existing session. This coupling might surprise users who only want microphone access. Consider separating these concerns or adding a clear UI indication that enabling phone mic will also initiate the camera session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 1012 - 1023, The togglePhoneMicrophone function is doing two unrelated things—toggling the phone mic and starting a phone camera/session via selectPhoneAsCamera when no session exists—which surprises users; change it so togglePhoneMicrophone (referenced symbols: togglePhoneMicrophone, phoneMicrophoneEnabled, phoneRemote.session, setPhoneMicrophoneEnabled, setMicrophoneEnabled) only flips mic state and updates setMicrophoneEnabled when enabling, and remove the automatic void selectPhoneAsCamera() call; instead introduce a separate explicit action (e.g., startPhoneSession or a UI confirmation) that calls selectPhoneAsCamera when the user intends to start the phone camera/session, or show a prompt before calling selectPhoneAsCamera if a session is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/phoneRemote/server.ts`:
- Around line 126-145: The current block handling offer/answer allows mismatched
envelope types (e.g., record.type === "offer" but description.type ===
"answer"), so update the validation in the offer/answer branch (the code that
calls asRecord(record.description) and inspects description.type/sdp) to also
require description.type === record.type and return null if they differ; adjust
the checks around description.type and description.sdp in the offer/answer
branch (the logic around record.type, description, and their return object) so
only matching type pairs are considered valid.
- Around line 391-423: The timeout handler for TUNNEL_READY_TIMEOUT_MS must also
terminate the spawned cloudflared process to avoid leaking a background child;
update the timeout callback (the one that sets tunnelError = "Secure tunnel did
not become ready in time." and calls settle(null)) to check tunnelProcess,
remove or ignore its listeners (stdout/stderr 'data', 'error', 'exit'), attempt
to gracefully kill it (e.g., tunnelProcess.kill() / fallback to force kill), and
null out tunnelProcess after killing, then call settle(null); ensure the rest of
the handlers (inspectOutput, tunnelProcess.once('error'),
tunnelProcess.once('exit')) still tolerate the case where tunnelProcess has been
killed/nullified so they don't re-settle.
In `@electron/phoneRemote/sessionStore.ts`:
- Around line 168-180: updatePhoneRemoteStatus currently updates status fields
but never refreshes the session TTL causing live sessions to expire after 10
minutes; update the function (updatePhoneRemoteStatus) to also set
session.expiresAt to Date.now() plus the session TTL whenever a heartbeat/status
arrives (use the existing SESSION_TTL / PHONE_SESSION_TTL constant if present,
otherwise set to 10*60*1000) before calling emit so active sessions won't be
pruned prematurely.
- Around line 18-25: The createPairingCode function can produce fewer than 8
chars because randomBytes(5) yields too few base64url characters and filtering
removes '-'/'_', so change createPairingCode to generate until you have 8
alphanumeric chars: repeatedly call crypto.randomBytes (e.g., in a loop) to
append more base64url output, filter with .replace(/[^A-Z0-9]/gi, "") and
.toUpperCase(), and only .slice(0, 8) once the filtered string length is >= 8;
keep the logic inside createPairingCode so it always returns an 8-character
alphanumeric code.
In `@src/hooks/useScreenRecorder.ts`:
- Line 142: The default state for phone microphone is currently true which can
mislead users; change the initial value of phoneMicrophoneEnabled in
useScreenRecorder (the useState call that defines phoneMicrophoneEnabled and
setPhoneMicrophoneEnabled) to false, and add logic to
setPhoneMicrophoneEnabled(true) only when a phone session is actually
established (e.g., where phone connection/session is detected in the hook — call
setPhoneMicrophoneEnabled(true) in that connection-success handler and keep it
false on disconnect).
---
Nitpick comments:
In `@electron/ipc/recording/diagnostics.ts`:
- Around line 222-224: The check in diagnostics.ts that detects missing FFmpeg
is tightly coupled to an exact message string; update the detection logic around
the block that builds "message" (where it currently does
message.includes("FFmpeg binary is unavailable")) to use a tolerant match or
structured code: first, if the thrown error exposes a code (e.g., error.code ===
'FFMPEG_UNAVAILABLE' or similar) prefer that; otherwise replace the exact string
check with a case-insensitive, fuzzy test such as testing for /ffmpeg/i and one
of "unavail|missing|not found" (or using
message.toLowerCase().includes("ffmpeg") && (message.includes("unavailable") ||
message.includes("not found") || message.includes("missing"))) so the detection
still fires if wording changes.
In `@electron/ipc/register/phoneRemote.ts`:
- Around line 84-101: The handler registration in registerPhoneRemoteHandlers is
not fully idempotent: the subscribed flag only guards subscribePhoneRemoteStore
but ipcMain.handle registrations (the handlers registered in this function) will
throw if called twice; update registerPhoneRemoteHandlers to either include the
ipcMain.handle(...) calls inside the same subscribed guard (so they run only
once alongside subscribePhoneRemoteStore) or call ipcMain.removeHandler for each
IPC channel before calling ipcMain.handle (ensuring safe re-registration);
reference registerPhoneRemoteHandlers, subscribed, subscribePhoneRemoteStore and
the ipcMain.handle-registered channel names when applying the change.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 1070-1083: Replace the blocking window.confirm() in the
LaunchWindow component with a non-blocking UI element (e.g., existing Modal or
Toast with action buttons) so the thread is not blocked; locate the confirm call
in the conditional that checks webcamSource === "phone" && webcamEnabled &&
!phoneRemote.videoActive && !phoneRemote.micActive, and instead render your
modal/toast that shows the same message and callbacks: if the user confirms call
the current logic (proceed with recording), otherwise call
setActiveDropdown("webcam") and return/abort; ensure the modal’s confirm and
cancel handlers mirror the existing behavior and keep state (e.g., showModal
boolean) local to LaunchWindow or use the app’s shared modal/toast component.
- Around line 1012-1023: The togglePhoneMicrophone function is doing two
unrelated things—toggling the phone mic and starting a phone camera/session via
selectPhoneAsCamera when no session exists—which surprises users; change it so
togglePhoneMicrophone (referenced symbols: togglePhoneMicrophone,
phoneMicrophoneEnabled, phoneRemote.session, setPhoneMicrophoneEnabled,
setMicrophoneEnabled) only flips mic state and updates setMicrophoneEnabled when
enabling, and remove the automatic void selectPhoneAsCamera() call; instead
introduce a separate explicit action (e.g., startPhoneSession or a UI
confirmation) that calls selectPhoneAsCamera when the user intends to start the
phone camera/session, or show a prompt before calling selectPhoneAsCamera if a
session is missing.
In `@src/hooks/usePhoneRemoteMedia.ts`:
- Around line 334-338: The RTCPeerConnection created in createPeer (inside the
useCallback) currently uses only "stun:stun.l.google.com:19302"; update the ICE
configuration to include additional fallback STUN servers or read the iceServers
list from a configurable source (env/config) so connectivity is more robust:
modify the RTCPeerConnection constructor call in createPeer to use an array of
multiple STUN (and optionally TURN) entries or inject a configurable iceServers
value so callers can override the list.
- Around line 476-521: The async onPhoneRemoteSignal callback can observe
stale/closed refs after the effect cleanup; to fix, introduce an "active" (or
"mounted") boolean captured by the effect and flipped to false in its cleanup,
and inside the callback bail out early if active is false (and/or capture local
snapshots of sessionRef.current and peerRef.current into consts at the start of
the callback and re-check active before doing any await-bound operations).
Update the useEffect that registers window.electronAPI.onPhoneRemoteSignal (and
the code paths handling "answer" and "ice-candidate") to check the active flag
before calling peer.setRemoteDescription, peer.addIceCandidate, mutating
pendingRemoteIceCandidatesRef, or calling setStatus/setStatusDetail; reference
symbols: useEffect/onPhoneRemoteSignal, sessionRef, peerRef,
pendingRemoteIceCandidatesRef, setStatus, setStatusDetail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a87f013-b4c3-49b8-98d4-b4a0ec448d69
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/recording/diagnostics.test.tselectron/ipc/recording/diagnostics.tselectron/ipc/register/phoneRemote.tselectron/main.tselectron/phoneRemote/mobilePage.tselectron/phoneRemote/server.tselectron/phoneRemote/sessionStore.test.tselectron/phoneRemote/sessionStore.tselectron/phoneRemote/types.tselectron/preload.tssrc/components/launch/LaunchWindow.module.csssrc/components/launch/LaunchWindow.tsxsrc/hooks/usePhoneRemoteMedia.tssrc/hooks/useScreenRecorder.tssrc/lib/simpleQr.tsvite.config.ts
Author
|
@coderabbitai Thanks for the review. I pushed a follow-up that resolves the merge conflict with latest main and addresses the actionable review items:
Verified locally with |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
I'll kick off a fresh review to verify the follow-up commits against the outstanding items. [review] |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/components/video-editor/projectPersistence.ts (1)
331-333: ⚡ Quick winPlease add a regression test for the new overlap fallback clamp.
This logic fix is good; adding a focused test for “invalid
zoomInOverlapMs+ very smallzoomInDurationMs” will lock in the behavior and prevent backslides.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/projectPersistence.ts` around lines 331 - 333, Add a regression test that exercises the fallback/clamping behavior around normalizedZoomInOverlapMs: create a case where editor.zoomInOverlapMs is invalid (e.g., NaN or Infinity) and zoomInDurationMs is very small so normalizedZoomInDurationMs < DEFAULT_ZOOM_IN_OVERLAP_MS, then assert that normalizedZoomInOverlapMs equals clamp(DEFAULT_ZOOM_IN_OVERLAP_MS, 0, normalizedZoomInDurationMs) (i.e., it gets clamped down to the small duration). Use the same helper or function that computes normalizedZoomInOverlapMs (referencing editor.zoomInOverlapMs, normalizedZoomInDurationMs, DEFAULT_ZOOM_IN_OVERLAP_MS and clamp) to ensure the behavior is locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/phoneRemote/sessionStore.ts`:
- Around line 77-97: When creating a new session in createPhoneRemoteSession,
ensure the generated pairing code is unique among currently active sessions
before storing: after pruneExpiredPhoneRemoteSessions(now) call, generate a code
with createPairingCode(), then check sessions (the sessions Map) for any
non-expired session whose code equals that code; if a collision exists,
re-generate the code (loop until unique) and only then construct the
PhoneRemoteSession object, set it in sessions, and return
toPublicSession(session). Keep using session.id = crypto.randomUUID(),
ownerWebContentsId, createdAt/expiresAt, and status/laptopSignals/phoneSignals
as before.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 554-575: The sidecar recorder started by
startMicrophoneSidecarRecorder (micFallbackRecorder, micFallbackChunks,
micFallbackStartDelayMs) is not hooked into the native lifecycle; update
pauseRecording and resumeRecording to call micFallbackRecorder.current?.pause()
and micFallbackRecorder.current?.resume() respectively (guarding for null and
MediaRecorder state), and modify cancelRecording to stop and cleanup the
micFallbackRecorder (call stop(), remove ondataavailable, clear
micFallbackChunks.current and micFallbackRecorder.current) and also stop
underlying AudioTrack(s) if any remain; ensure these branches mirror the main
recorder lifecycle handling so paused sections aren’t captured and cancel fully
terminates microphone capture.
- Around line 1298-1324: The fallback currently auto-selects sources[0] after
window.electronAPI.getSources when the portal flow fails; instead abort and
re-prompt the user for a source selection. Replace the automatic selection of
source = sources[0] (and the subsequent navigator.mediaDevices.getUserMedia
call) with logic that either throws a clear error or invokes the existing
source-selection UI/flow (e.g., call the same selection prompt used elsewhere)
so the user explicitly chooses which screen to record; keep the alert about
audio if withAudio is true and ensure you reference
window.electronAPI.getSources, withAudio, sources, and
navigator.mediaDevices.getUserMedia when making the change.
In `@src/i18n/locales/es/settings.json`:
- Around line 45-49: The listed Spanish locale entries are still in English;
update the values for keys such as "temporalZoomMotionBlur",
"temporalZoomMotionBlurDescription", "zoomMotionBlurSamples",
"zoomMotionBlurShutter", "auto" (and the additional keys in the ranges
mentioned: lines 62-75 and 93-95) with proper Spanish translations so the UI
reads in Spanish; locate these keys in src/i18n/locales/es/settings.json and
replace the English strings with concise Spanish equivalents (e.g., "Temporal
Zoom Blur" → "Desenfoque de zoom temporal", "Blur Samples" → "Muestras de
desenfoque", "Shutter" → "Obturador", "Auto" → "Automático") while keeping
punctuation and JSON structure intact.
In `@src/i18n/locales/nl/settings.json`:
- Around line 45-49: Replace the English strings in the Dutch locale with proper
Dutch translations: update keys like "temporalZoomMotionBlur",
"temporalZoomMotionBlurDescription", "zoomMotionBlurSamples",
"zoomMotionBlurShutter", and "auto" to Dutch equivalents and similarly localize
other affected keys referenced in the comment (rows around keys in the same file
at the ranges mentioned) so the settings UI is fully Dutch; edit the values for
those keys in src/i18n/locales/nl/settings.json (e.g., translate labels,
descriptions, motion preset texts, and cursor spring labels) ensuring the JSON
values are in Dutch and preserve the exact keys.
In `@src/i18n/locales/pt-BR/settings.json`:
- Around line 45-49: The listed pt-BR locale entries are still in English;
update the values for the keys temporalZoomMotionBlur,
temporalZoomMotionBlurDescription, zoomMotionBlurSamples, zoomMotionBlurShutter,
auto (and the other untranslated ranges referenced at lines 62-75 and 93-95) to
Portuguese equivalents so the UI is fully localized; locate those keys in
src/i18n/locales/pt-BR/settings.json and replace the English strings with
appropriate Portuguese translations while preserving the JSON keys and
punctuation.
In `@src/i18n/locales/zh-CN/settings.json`:
- Around line 45-49: The Chinese locale file contains untranslated English
values for keys temporalZoomMotionBlur, temporalZoomMotionBlurDescription,
zoomMotionBlurSamples, zoomMotionBlurShutter, and auto; replace the English
strings with appropriate Simplified Chinese translations for each key so the UI
is fully localized (update "temporalZoomMotionBlur" label,
"temporalZoomMotionBlurDescription" description, "zoomMotionBlurSamples" label,
"zoomMotionBlurShutter" label, and "auto" value).
In `@src/i18n/locales/zh-TW/settings.json`:
- Around line 45-49: The listed zh-TW locale entries are still in
English—translate the keys "temporalZoomMotionBlur",
"temporalZoomMotionBlurDescription", "zoomMotionBlurSamples",
"zoomMotionBlurShutter", and "auto" into Traditional Chinese (and likewise
translate other untranslated strings in this file). Update the JSON values with
proper Traditional Chinese text, preserving the exact JSON keys, punctuation and
any escape sequences; keep the surrounding quotes and commas intact and ensure
the strings are natural for UI use.
---
Nitpick comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 331-333: Add a regression test that exercises the
fallback/clamping behavior around normalizedZoomInOverlapMs: create a case where
editor.zoomInOverlapMs is invalid (e.g., NaN or Infinity) and zoomInDurationMs
is very small so normalizedZoomInDurationMs < DEFAULT_ZOOM_IN_OVERLAP_MS, then
assert that normalizedZoomInOverlapMs equals clamp(DEFAULT_ZOOM_IN_OVERLAP_MS,
0, normalizedZoomInDurationMs) (i.e., it gets clamped down to the small
duration). Use the same helper or function that computes
normalizedZoomInOverlapMs (referencing editor.zoomInOverlapMs,
normalizedZoomInDurationMs, DEFAULT_ZOOM_IN_OVERLAP_MS and clamp) to ensure the
behavior is locked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5007818b-b3c8-48c0-b58b-42047dc5f6e4
📒 Files selected for processing (20)
electron/electron-env.d.tselectron/ipc/recording/diagnostics.tselectron/ipc/register/phoneRemote.tselectron/phoneRemote/server.tselectron/phoneRemote/sessionStore.test.tselectron/phoneRemote/sessionStore.tselectron/preload.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/editorPreferences.test.tssrc/components/video-editor/projectPersistence.tssrc/hooks/usePhoneRemoteMedia.tssrc/hooks/useScreenRecorder.tssrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/nl/settings.jsonsrc/i18n/locales/pt-BR/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/simpleQr.ts
✅ Files skipped from review due to trivial changes (6)
- src/components/video-editor/SettingsPanel.tsx
- electron/phoneRemote/sessionStore.test.ts
- src/i18n/locales/ko/settings.json
- src/i18n/locales/fr/settings.json
- src/lib/simpleQr.ts
- electron/ipc/register/phoneRemote.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- electron/preload.ts
- electron/ipc/recording/diagnostics.ts
- electron/phoneRemote/server.ts
- src/hooks/usePhoneRemoteMedia.ts
Comment on lines
+77
to
+97
| export function createPhoneRemoteSession( | ||
| ownerWebContentsId: number, | ||
| urls: PhoneRemoteJoinUrls, | ||
| now = Date.now(), | ||
| ): PhoneRemotePublicSession { | ||
| pruneExpiredPhoneRemoteSessions(now); | ||
|
|
||
| const session: PhoneRemoteSession = { | ||
| ...urls, | ||
| id: crypto.randomUUID(), | ||
| code: createPairingCode(), | ||
| createdAt: now, | ||
| expiresAt: now + SESSION_TTL_MS, | ||
| ownerWebContentsId, | ||
| status: "waiting", | ||
| laptopSignals: [], | ||
| phoneSignals: [], | ||
| }; | ||
|
|
||
| sessions.set(session.id, session); | ||
| return toPublicSession(session); |
Contributor
There was a problem hiding this comment.
Enforce active-session pairing code uniqueness before storing the session.
Right now, a rare code collision can create two active sessions with the same code, and code-based lookup will resolve to whichever entry is encountered first. That can mis-pair devices.
Suggested minimal fix
+function isPairingCodeInUse(code: string): boolean {
+ for (const session of sessions.values()) {
+ if (session.code === code) {
+ return true;
+ }
+ }
+ return false;
+}
+
export function createPhoneRemoteSession(
ownerWebContentsId: number,
urls: PhoneRemoteJoinUrls,
now = Date.now(),
): PhoneRemotePublicSession {
pruneExpiredPhoneRemoteSessions(now);
+
+ let pairingCode = createPairingCode();
+ while (isPairingCodeInUse(pairingCode)) {
+ pairingCode = createPairingCode();
+ }
const session: PhoneRemoteSession = {
...urls,
id: crypto.randomUUID(),
- code: createPairingCode(),
+ code: pairingCode,
createdAt: now,
expiresAt: now + SESSION_TTL_MS,
ownerWebContentsId,
status: "waiting",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/phoneRemote/sessionStore.ts` around lines 77 - 97, When creating a
new session in createPhoneRemoteSession, ensure the generated pairing code is
unique among currently active sessions before storing: after
pruneExpiredPhoneRemoteSessions(now) call, generate a code with
createPairingCode(), then check sessions (the sessions Map) for any non-expired
session whose code equals that code; if a collision exists, re-generate the code
(loop until unique) and only then construct the PhoneRemoteSession object, set
it in sessions, and return toPublicSession(session). Keep using session.id =
crypto.randomUUID(), ownerWebContentsId, createdAt/expiresAt, and
status/laptopSignals/phoneSignals as before.
Comment on lines
+554
to
+575
| const startMicrophoneSidecarRecorder = useCallback( | ||
| (audioStreamForSidecar: MediaStream | null, mainStartedAt: number) => { | ||
| const audioTrack = audioStreamForSidecar?.getAudioTracks()[0]; | ||
| if (!audioTrack) { | ||
| return false; | ||
| } | ||
|
|
||
| micFallbackChunks.current = []; | ||
| const recorder = new MediaRecorder(new MediaStream([audioTrack]), { | ||
| mimeType: "audio/webm;codecs=opus", | ||
| }); | ||
| recorder.ondataavailable = (event) => { | ||
| if (event.data.size > 0) { | ||
| micFallbackChunks.current.push(event.data); | ||
| } | ||
| }; | ||
| micFallbackStartDelayMs.current = Math.max(0, Date.now() - mainStartedAt); | ||
| recorder.start(RECORDER_TIMESLICE_MS); | ||
| micFallbackRecorder.current = recorder; | ||
| return true; | ||
| }, | ||
| [], |
Contributor
There was a problem hiding this comment.
Integrate the sidecar recorder with the native pause/cancel lifecycle.
This new recorder is started for phone/fallback mic capture, but pauseRecording() / resumeRecording() never pause it and the native cancelRecording() branch returns without stopping it. The result is that paused sections still get recorded into the sidecar, and cancel can leave microphone capture running after the user thinks recording has ended.
Also applies to: 1225-1230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useScreenRecorder.ts` around lines 554 - 575, The sidecar recorder
started by startMicrophoneSidecarRecorder (micFallbackRecorder,
micFallbackChunks, micFallbackStartDelayMs) is not hooked into the native
lifecycle; update pauseRecording and resumeRecording to call
micFallbackRecorder.current?.pause() and micFallbackRecorder.current?.resume()
respectively (guarding for null and MediaRecorder state), and modify
cancelRecording to stop and cleanup the micFallbackRecorder (call stop(), remove
ondataavailable, clear micFallbackChunks.current and
micFallbackRecorder.current) and also stop underlying AudioTrack(s) if any
remain; ensure these branches mirror the main recorder lifecycle handling so
paused sections aren’t captured and cancel fully terminates microphone capture.
Comment on lines
+1298
to
+1324
| if (withAudio) { | ||
| alert( | ||
| "System audio is not supported in fallback mode. Recording will continue without audio.", | ||
| ); | ||
| } | ||
|
|
||
| const sources = await window.electronAPI.getSources({ types: ["screen"] }); | ||
|
|
||
| if (!sources.length) { | ||
| throw new Error("No screen sources available"); | ||
| } | ||
|
|
||
| const source = sources[0]; | ||
| console.log("Using fallback source:", source); | ||
|
|
||
| return await navigator.mediaDevices.getUserMedia({ | ||
| audio: false, //intentional | ||
| video: { | ||
| mandatory: { | ||
| chromeMediaSource: "desktop", | ||
| chromeMediaSourceId: source.id, | ||
| maxWidth: TARGET_WIDTH, | ||
| maxHeight: TARGET_HEIGHT, | ||
| maxFrameRate: TARGET_FRAME_RATE, | ||
| }, | ||
| }, | ||
| } as unknown as MediaStreamConstraints); |
Contributor
There was a problem hiding this comment.
Don't silently fall back to the first Linux screen source.
When the portal flow fails, this code records sources[0]. On a multi-monitor setup that can capture a completely different display than the user intended, and the only user-facing message is about losing system audio. Please abort here and re-prompt for source selection instead of choosing an arbitrary source.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useScreenRecorder.ts` around lines 1298 - 1324, The fallback
currently auto-selects sources[0] after window.electronAPI.getSources when the
portal flow fails; instead abort and re-prompt the user for a source selection.
Replace the automatic selection of source = sources[0] (and the subsequent
navigator.mediaDevices.getUserMedia call) with logic that either throws a clear
error or invokes the existing source-selection UI/flow (e.g., call the same
selection prompt used elsewhere) so the user explicitly chooses which screen to
record; keep the alert about audio if withAudio is true and ensure you reference
window.electronAPI.getSources, withAudio, sources, and
navigator.mediaDevices.getUserMedia when making the change.
Comment on lines
+45
to
+49
| "temporalZoomMotionBlur": "Temporal Zoom Blur", | ||
| "temporalZoomMotionBlurDescription": "Control the shutter window and frame samples used by the newer zoom blur pass.", | ||
| "zoomMotionBlurSamples": "Blur Samples", | ||
| "zoomMotionBlurShutter": "Shutter", | ||
| "auto": "Auto", |
Contributor
There was a problem hiding this comment.
New es locale strings are currently English.
Please localize these newly introduced entries (temporal blur, motion presets, cursor spring labels) to keep Spanish UX consistent.
Also applies to: 62-75, 93-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/es/settings.json` around lines 45 - 49, The listed Spanish
locale entries are still in English; update the values for keys such as
"temporalZoomMotionBlur", "temporalZoomMotionBlurDescription",
"zoomMotionBlurSamples", "zoomMotionBlurShutter", "auto" (and the additional
keys in the ranges mentioned: lines 62-75 and 93-95) with proper Spanish
translations so the UI reads in Spanish; locate these keys in
src/i18n/locales/es/settings.json and replace the English strings with concise
Spanish equivalents (e.g., "Temporal Zoom Blur" → "Desenfoque de zoom temporal",
"Blur Samples" → "Muestras de desenfoque", "Shutter" → "Obturador", "Auto" →
"Automático") while keeping punctuation and JSON structure intact.
Comment on lines
+45
to
+49
| "temporalZoomMotionBlur": "Temporal Zoom Blur", | ||
| "temporalZoomMotionBlurDescription": "Control the shutter window and frame samples used by the newer zoom blur pass.", | ||
| "zoomMotionBlurSamples": "Blur Samples", | ||
| "zoomMotionBlurShutter": "Shutter", | ||
| "auto": "Auto", |
Contributor
There was a problem hiding this comment.
Several newly added nl locale entries are still in English.
Temporal blur labels, motion preset texts, and cursor spring labels should be localized to Dutch to avoid mixed-language settings UI.
Also applies to: 62-75, 93-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/nl/settings.json` around lines 45 - 49, Replace the English
strings in the Dutch locale with proper Dutch translations: update keys like
"temporalZoomMotionBlur", "temporalZoomMotionBlurDescription",
"zoomMotionBlurSamples", "zoomMotionBlurShutter", and "auto" to Dutch
equivalents and similarly localize other affected keys referenced in the comment
(rows around keys in the same file at the ranges mentioned) so the settings UI
is fully Dutch; edit the values for those keys in
src/i18n/locales/nl/settings.json (e.g., translate labels, descriptions, motion
preset texts, and cursor spring labels) ensuring the JSON values are in Dutch
and preserve the exact keys.
Comment on lines
+45
to
+49
| "temporalZoomMotionBlur": "Temporal Zoom Blur", | ||
| "temporalZoomMotionBlurDescription": "Control the shutter window and frame samples used by the newer zoom blur pass.", | ||
| "zoomMotionBlurSamples": "Blur Samples", | ||
| "zoomMotionBlurShutter": "Shutter", | ||
| "auto": "Auto", |
Contributor
There was a problem hiding this comment.
New pt-BR locale strings are still English.
Please translate these added entries (temporal blur, motion presets, cursor spring labels) to Portuguese to avoid mixed-language UI.
Also applies to: 62-75, 93-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/pt-BR/settings.json` around lines 45 - 49, The listed pt-BR
locale entries are still in English; update the values for the keys
temporalZoomMotionBlur, temporalZoomMotionBlurDescription,
zoomMotionBlurSamples, zoomMotionBlurShutter, auto (and the other untranslated
ranges referenced at lines 62-75 and 93-95) to Portuguese equivalents so the UI
is fully localized; locate those keys in src/i18n/locales/pt-BR/settings.json
and replace the English strings with appropriate Portuguese translations while
preserving the JSON keys and punctuation.
Comment on lines
+45
to
+49
| "temporalZoomMotionBlur": "Temporal Zoom Blur", | ||
| "temporalZoomMotionBlurDescription": "Control the shutter window and frame samples used by the newer zoom blur pass.", | ||
| "zoomMotionBlurSamples": "Blur Samples", | ||
| "zoomMotionBlurShutter": "Shutter", | ||
| "auto": "Auto", |
Contributor
There was a problem hiding this comment.
New zh-CN strings are untranslated (English values).
These added effects labels/descriptions are currently English, which causes mixed-language UI in the Chinese locale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/zh-CN/settings.json` around lines 45 - 49, The Chinese
locale file contains untranslated English values for keys
temporalZoomMotionBlur, temporalZoomMotionBlurDescription,
zoomMotionBlurSamples, zoomMotionBlurShutter, and auto; replace the English
strings with appropriate Simplified Chinese translations for each key so the UI
is fully localized (update "temporalZoomMotionBlur" label,
"temporalZoomMotionBlurDescription" description, "zoomMotionBlurSamples" label,
"zoomMotionBlurShutter" label, and "auto" value).
Comment on lines
+45
to
+49
| "temporalZoomMotionBlur": "Temporal Zoom Blur", | ||
| "temporalZoomMotionBlurDescription": "Control the shutter window and frame samples used by the newer zoom blur pass.", | ||
| "zoomMotionBlurSamples": "Blur Samples", | ||
| "zoomMotionBlurShutter": "Shutter", | ||
| "auto": "Auto", |
Contributor
There was a problem hiding this comment.
New zh-TW entries are untranslated (left in English).
These newly added settings strings should be localized to Traditional Chinese for a consistent locale experience.
Also applies to: 62-75, 93-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/zh-TW/settings.json` around lines 45 - 49, The listed zh-TW
locale entries are still in English—translate the keys "temporalZoomMotionBlur",
"temporalZoomMotionBlurDescription", "zoomMotionBlurSamples",
"zoomMotionBlurShutter", and "auto" into Traditional Chinese (and likewise
translate other untranslated strings in this file). Update the JSON values with
proper Traditional Chinese text, preserving the exact JSON keys, punctuation and
any escape sequences; keep the surrounding quotes and commas intact and ensure
the strings are natural for UI use.
Author
|
Follow-up: upstream |
Author
|
Follow-up fix pushed in Verified locally with |
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
1 participant
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.
Summary
Testing
Refs #418
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization