Add webcam recording overlay support#229
Merged
siddharthvaddem merged 8 commits intosiddharthvaddem:mainfrom Mar 18, 2026
Merged
Add webcam recording overlay support#229siddharthvaddem merged 8 commits intosiddharthvaddem:mainfrom
siddharthvaddem merged 8 commits intosiddharthvaddem:mainfrom
Conversation
Contributor
Author
|
@siddharthvaddem the feature works, but I haven't reviewed/cleaned the code yet, will do this tomorrow |
📝 WalkthroughWalkthroughAdds webcam/camera overlay support across recording, IPC/session management, project persistence (schema v2), playback overlay/sync, and exporter pipelines to composite webcam frames into exports. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LaunchWindow
participant useScreenRecorder
participant MediaDevices
participant ElectronIPC
participant FileSystem
User->>LaunchWindow: Toggle webcam on
LaunchWindow->>useScreenRecorder: setWebcamEnabled(true)
User->>LaunchWindow: Start recording
LaunchWindow->>useScreenRecorder: toggleRecording()
useScreenRecorder->>MediaDevices: getDisplayMedia (screen)
MediaDevices-->>useScreenRecorder: screenStream
useScreenRecorder->>MediaDevices: getUserMedia (webcam)
MediaDevices-->>useScreenRecorder: webcamStream
useScreenRecorder->>useScreenRecorder: start screenRecorder & webcamRecorder
Note over useScreenRecorder: Recording...
User->>LaunchWindow: Stop recording
LaunchWindow->>useScreenRecorder: toggleRecording()
useScreenRecorder->>useScreenRecorder: stop both recorders, assemble blobs
useScreenRecorder->>ElectronIPC: store-recorded-session({screen, webcam, createdAt})
ElectronIPC->>FileSystem: write screen file, write webcam file, write session manifest
FileSystem-->>ElectronIPC: paths
ElectronIPC-->>useScreenRecorder: {success, session, path}
useScreenRecorder->>LaunchWindow: open editor with session media
sequenceDiagram
participant VideoEditor
participant ProjectPersistence
participant VideoExporter
participant FrameRenderer
participant FileSystem
VideoEditor->>ProjectPersistence: resolveProjectMedia(project)
ProjectPersistence-->>VideoEditor: {screenVideoPath, webcamVideoPath}
VideoEditor->>VideoExporter: export({screenVideoUrl, webcamVideoUrl})
VideoExporter->>VideoExporter: init screenDecoder & webcamDecoder
par decode main video
VideoExporter->>FrameRenderer: decode screen frames
and decode webcam
VideoExporter->>VideoExporter: decode webcam frames -> AsyncVideoFrameQueue
end
VideoExporter->>FrameRenderer: renderFrame(screenFrame, timestamp, webcamFrame)
FrameRenderer->>FrameRenderer: composite main + webcam overlay
FrameRenderer-->>VideoExporter: composited frame
VideoExporter->>FileSystem: encode and write MP4/GIF
FileSystem-->>VideoExporter: path
VideoExporter-->>VideoEditor: {success, path}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
📝 Coding Plan
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fb5b3b574
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
electron/ipc/handlers.ts (1)
562-573: Consider extracting duplicate session derivation logic.The logic for deriving a recording session from project data (handling both
mediaand legacyvideoPathfields) is duplicated betweenload-project-fileandload-current-project-filehandlers. Consider extracting to a helper function for maintainability.Proposed helper extraction
function deriveSessionFromProject(project: unknown): RecordingSession | null { if (!project || typeof project !== "object") { return null; } const rawProject = project as { media?: unknown; videoPath?: unknown }; const media = normalizeProjectMedia(rawProject.media) ?? (typeof rawProject.videoPath === "string" ? { screenVideoPath: normalizeVideoSourcePath(rawProject.videoPath) ?? rawProject.videoPath, } : null); return media ? { ...media, createdAt: Date.now() } : null; }Then use in both handlers:
const project = JSON.parse(content); currentProjectPath = filePath; -if (project && typeof project === "object") { - const rawProject = project as { media?: unknown; videoPath?: unknown }; - const media = ... - setCurrentRecordingSessionState(media ? { ...media, createdAt: Date.now() } : null); -} +setCurrentRecordingSessionState(deriveSessionFromProject(project));Also applies to: 598-609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 562 - 573, The session-derivation logic duplicated inside the "load-project-file" and "load-current-project-file" handlers should be extracted into a single helper (e.g., deriveSessionFromProject) that accepts project: unknown, uses normalizeProjectMedia and normalizeVideoSourcePath to build the media object (or handle legacy videoPath), and returns RecordingSession | null; replace the inline code in both handlers with a call to this helper and pass its result into setCurrentRecordingSessionState so createdAt is set once in the helper.src/components/launch/LaunchWindow.tsx (1)
249-258: Add explicit accessibility semantics to the new icon-only webcam button.Please add
aria-labelandaria-pressed; relying ontitlealone is not robust for assistive tech.Suggested a11y update
<button className={`${hudIconBtnClasses} ${webcamEnabled ? "drop-shadow-[0_0_4px_rgba(74,222,128,0.4)]" : ""}`} onClick={() => !recording && setWebcamEnabled(!webcamEnabled)} disabled={recording} title={webcamEnabled ? "Disable webcam" : "Enable webcam"} + aria-label={webcamEnabled ? "Disable webcam" : "Enable webcam"} + aria-pressed={webcamEnabled} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow.tsx` around lines 249 - 258, The webcam icon-only button in LaunchWindow.tsx (the <button> using hudIconBtnClasses and rendering getIcon based on webcamEnabled) needs explicit accessibility semantics: add an aria-label that reflects the action (e.g., "Enable webcam" when webcamEnabled is false and "Disable webcam" when true) and add aria-pressed set to the boolean webcamEnabled; ensure these attributes are applied alongside the existing title, disabled={recording}, and the onClick handler that calls setWebcamEnabled(!webcamEnabled).
🤖 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/launch/LaunchWindow.tsx`:
- Around line 251-253: The webcam toggle is currently blocked during recording
because the button uses disabled={recording} and guards the onClick with
!recording; remove the recording-based guard and allow setWebcamEnabled to be
called regardless of recording state by (1) deleting disabled={recording} from
the button props and (2) updating the onClick to simply call
setWebcamEnabled(!webcamEnabled) (keep the title logic webcamEnabled ? "Disable
webcam" : "Enable webcam"); ensure no other surrounding logic blocks toggling
during recording (search for usages of recording in the LaunchWindow toggle
block and remove or adjust any extra guards that prevent setWebcamEnabled from
running while recording).
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 965-986: The webcam preview effect currently only nudges
currentTime and calls play() every tick; change it to mirror the main video's
playbackRate and play/pause state and only correct currentTime for seeks or
large drift: inside the useEffect that reads webcamVideoRef, webcamVideoPath,
isPlaying, and currentTime, set webcamVideo.playbackRate = <mainPlaybackRate>
(the main video's playbackRate state/ref) and toggle webcamVideo.play()/pause()
based on isPlaying instead of always calling play(); only set
webcamVideo.currentTime when Math.abs(webcamVideo.currentTime - currentTime)
exceeds a larger drift threshold (e.g. 0.15 or higher for seeks) to avoid
frequent snaps. Ensure you stop calling play() on every tick and use the main
video's playbackRate and play state to keep the preview in sync.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 343-346: In the catch block handling cameraError, call
setWebcamEnabled(false) to mirror the microphone handling and keep the UI state
consistent; also ensure any partially acquired webcam stream is cleaned up by
stopping its tracks and clearing state (e.g., setWebcamStream(null)) so the app
doesn't think the webcam is active after access denial.
In `@src/lib/exporter/gifExporter.ts`:
- Around line 168-190: webcamFrameQueue can remain filled and webcam decoding
can hang when the main GIF decode stops; ensure the queue is always torn down on
every exit path by adding a finally handler around the webcam decoding promise
(the block that creates webcamFrameQueue and assigns webcamDecodePromise) so
that regardless of success, error, or cancellation you call
webcamFrameQueue.close() or webcamFrameQueue.fail(...) and release any buffered
VideoFrame(s). Update the decodeAll().then().catch() chain (or wrap the await of
webcamDecodePromise) that references webcamDecoder, decodeAll,
webcamDecodePromise and this.cancelled to include a finally that closes/fails
webcamFrameQueue, and apply the same pattern to the other similar spots where
webcamFrameQueue is created.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 119-141: The webcam frame queue (webcamFrameQueue) is only closed
on normal completion which can leave VideoFrames allocated or deadlock the
producer at backpressure; wrap the decodeAll invocation and its await
(webcamDecodePromise / this.webcamDecoder.decodeAll callback) in a local
try/finally so that in every exit path you explicitly drain/destroy the queue
(e.g., call a destroy/close-and-release helper on webcamFrameQueue or dequeue
and close in finally), and ensure any errors call webcamFrameQueue.fail(...)
before rethrowing; apply the same pattern where webcamFrameQueue is used (also
around the other occurrences noted at the same spots) so the queue is always
cleaned up even on cancellation or failure.
In `@src/lib/webcamOverlay.ts`:
- Around line 10-20: The computeWebcamOverlayLayout function and its hardcoded
constants (MAX_STAGE_FRACTION, MARGIN_FRACTION, MIN_SIZE, MAX_BORDER_RADIUS)
make the overlay fixed; change the function signature to accept a
configuration/options object (e.g., overlayPosition/corner, sizeFraction,
marginFraction, minSize, borderRadius, and optional requested width/height/x/y)
and compute the rectangle based on those inputs so callers can choose corner or
absolute position and honor user resizing; update any callers to pass through
the new options and keep default values that map to the existing constants if
options are omitted; ensure the returned WebcamOverlayLayout uses the provided
position/size when present and falls back to the previous behavior otherwise.
- Around line 27-36: The overlay width/height can exceed the stage when MIN_SIZE
is larger than the available canvas; after computing margin, width and height in
the function (symbols: margin, width, height, stageWidth, stageHeight,
MIN_SIZE), clamp width to at most Math.max(0, stageWidth - margin) and clamp
height to at most Math.max(0, stageHeight - margin) (replace the current
width/height assignments or add adjustments immediately after them) so the
overlay never draws off-canvas even on tiny stages; keep the existing rounding
logic and ensure x/y calculations remain valid.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 562-573: The session-derivation logic duplicated inside the
"load-project-file" and "load-current-project-file" handlers should be extracted
into a single helper (e.g., deriveSessionFromProject) that accepts project:
unknown, uses normalizeProjectMedia and normalizeVideoSourcePath to build the
media object (or handle legacy videoPath), and returns RecordingSession | null;
replace the inline code in both handlers with a call to this helper and pass its
result into setCurrentRecordingSessionState so createdAt is set once in the
helper.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 249-258: The webcam icon-only button in LaunchWindow.tsx (the
<button> using hudIconBtnClasses and rendering getIcon based on webcamEnabled)
needs explicit accessibility semantics: add an aria-label that reflects the
action (e.g., "Enable webcam" when webcamEnabled is false and "Disable webcam"
when true) and add aria-pressed set to the boolean webcamEnabled; ensure these
attributes are applied alongside the existing title, disabled={recording}, and
the onClick handler that calls setWebcamEnabled(!webcamEnabled).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d933682-b25d-49bb-bed2-0a842aa57e0f
📒 Files selected for processing (18)
electron-builder.json5electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/hooks/useScreenRecorder.tssrc/lib/exporter/asyncVideoFrameQueue.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.tssrc/lib/recordingSession.tssrc/lib/webcamOverlay.test.tssrc/lib/webcamOverlay.tssrc/vite-env.d.ts
Comment on lines
+10
to
+20
| const MAX_STAGE_FRACTION = 0.18; | ||
| const MARGIN_FRACTION = 0.02; | ||
| const MIN_SIZE = 96; | ||
| const MAX_BORDER_RADIUS = 24; | ||
|
|
||
| export function computeWebcamOverlayLayout(params: { | ||
| stageWidth: number; | ||
| stageHeight: number; | ||
| videoWidth: number; | ||
| videoHeight: number; | ||
| }): WebcamOverlayLayout | null { |
There was a problem hiding this comment.
Make the overlay layout configurable.
If this PR is meant to close #193, this helper is still too rigid: every caller gets a lower-right overlay at a hardcoded ~18% of the stage. There is no way to move it to another corner or honor user resizing, so preview/export cannot support the movable/resizable overlay requirement yet.
Also applies to: 34-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/webcamOverlay.ts` around lines 10 - 20, The
computeWebcamOverlayLayout function and its hardcoded constants
(MAX_STAGE_FRACTION, MARGIN_FRACTION, MIN_SIZE, MAX_BORDER_RADIUS) make the
overlay fixed; change the function signature to accept a configuration/options
object (e.g., overlayPosition/corner, sizeFraction, marginFraction, minSize,
borderRadius, and optional requested width/height/x/y) and compute the rectangle
based on those inputs so callers can choose corner or absolute position and
honor user resizing; update any callers to pass through the new options and keep
default values that map to the existing constants if options are omitted; ensure
the returned WebcamOverlayLayout uses the provided position/size when present
and falls back to the previous behavior otherwise.
Contributor
Author
There was a problem hiding this comment.
@siddharthvaddem i kept this fixed on intention for simplification
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Owner
There was a problem hiding this comment.
reasonable. I'm happy to limit the scope of this PR for it being fixed. It would be even awesome if there was a follow-up PR for customization of shape/ location on the screen. Don't feel obliged, but thought I'd bring it up as I'm sure people will request for this.
Comment on lines
+27
to
+36
| const margin = Math.max(12, Math.round(Math.min(stageWidth, stageHeight) * MARGIN_FRACTION)); | ||
| const maxWidth = Math.max(MIN_SIZE, stageWidth * MAX_STAGE_FRACTION); | ||
| const maxHeight = Math.max(MIN_SIZE, stageHeight * MAX_STAGE_FRACTION); | ||
| const scale = Math.min(maxWidth / videoWidth, maxHeight / videoHeight); | ||
| const width = Math.round(videoWidth * scale); | ||
| const height = Math.round(videoHeight * scale); | ||
|
|
||
| return { | ||
| x: Math.max(0, Math.round(stageWidth - margin - width)), | ||
| y: Math.max(0, Math.round(stageHeight - margin - height)), |
There was a problem hiding this comment.
Clamp the minimum size against tiny stages.
MIN_SIZE can be larger than the available canvas. On small previews/exports, width/height can exceed the stage and the overlay gets drawn off-canvas even though x/y clamp to 0.
Suggested fix
const margin = Math.max(12, Math.round(Math.min(stageWidth, stageHeight) * MARGIN_FRACTION));
- const maxWidth = Math.max(MIN_SIZE, stageWidth * MAX_STAGE_FRACTION);
- const maxHeight = Math.max(MIN_SIZE, stageHeight * MAX_STAGE_FRACTION);
+ const availableWidth = Math.max(1, stageWidth - margin);
+ const availableHeight = Math.max(1, stageHeight - margin);
+ const maxWidth = Math.min(
+ availableWidth,
+ Math.max(MIN_SIZE, stageWidth * MAX_STAGE_FRACTION),
+ );
+ const maxHeight = Math.min(
+ availableHeight,
+ Math.max(MIN_SIZE, stageHeight * MAX_STAGE_FRACTION),
+ );
const scale = Math.min(maxWidth / videoWidth, maxHeight / videoHeight);📝 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 margin = Math.max(12, Math.round(Math.min(stageWidth, stageHeight) * MARGIN_FRACTION)); | |
| const maxWidth = Math.max(MIN_SIZE, stageWidth * MAX_STAGE_FRACTION); | |
| const maxHeight = Math.max(MIN_SIZE, stageHeight * MAX_STAGE_FRACTION); | |
| const scale = Math.min(maxWidth / videoWidth, maxHeight / videoHeight); | |
| const width = Math.round(videoWidth * scale); | |
| const height = Math.round(videoHeight * scale); | |
| return { | |
| x: Math.max(0, Math.round(stageWidth - margin - width)), | |
| y: Math.max(0, Math.round(stageHeight - margin - height)), | |
| const margin = Math.max(12, Math.round(Math.min(stageWidth, stageHeight) * MARGIN_FRACTION)); | |
| const availableWidth = Math.max(1, stageWidth - margin); | |
| const availableHeight = Math.max(1, stageHeight - margin); | |
| const maxWidth = Math.min( | |
| availableWidth, | |
| Math.max(MIN_SIZE, stageWidth * MAX_STAGE_FRACTION), | |
| ); | |
| const maxHeight = Math.min( | |
| availableHeight, | |
| Math.max(MIN_SIZE, stageHeight * MAX_STAGE_FRACTION), | |
| ); | |
| const scale = Math.min(maxWidth / videoWidth, maxHeight / videoHeight); | |
| const width = Math.round(videoWidth * scale); | |
| const height = Math.round(videoHeight * scale); | |
| return { | |
| x: Math.max(0, Math.round(stageWidth - margin - width)), | |
| y: Math.max(0, Math.round(stageHeight - margin - height)), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/webcamOverlay.ts` around lines 27 - 36, The overlay width/height can
exceed the stage when MIN_SIZE is larger than the available canvas; after
computing margin, width and height in the function (symbols: margin, width,
height, stageWidth, stageHeight, MIN_SIZE), clamp width to at most Math.max(0,
stageWidth - margin) and clamp height to at most Math.max(0, stageHeight -
margin) (replace the current width/height assignments or add adjustments
immediately after them) so the overlay never draws off-canvas even on tiny
stages; keep the existing rounding logic and ensure x/y calculations remain
valid.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/hooks/useScreenRecorder.ts (1)
402-416:⚠️ Potential issue | 🟡 MinorWebcam state not reset on access denial during recording start.
When webcam access fails in
startRecording(lines 412-415), the error is logged and a toast is shown, butsetWebcamEnabledState(false)is not called. This leaves the UI showing webcam as enabled even though no webcam stream was acquired. Compare with the microphone handling at line 398 which does reset the state.🛡️ Proposed fix
} catch (cameraError) { console.warn("Failed to get webcam access:", cameraError); toast.error("Camera access denied. Recording will continue without webcam."); + setWebcamEnabledState(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 402 - 416, The startRecording webcam access catch block does not update UI state when getUserMedia fails; update the catch handler in useScreenRecorder's startRecording flow to call setWebcamEnabledState(false) (and ensure webcamStream.current stays null) after logging/toasting the error so the UI reflects that the webcam is disabled when acquisition fails.
🧹 Nitpick comments (2)
src/components/video-editor/VideoPlayback.tsx (1)
965-1003: Webcam sync now handles playbackRate — verify remaining play() churn.The implementation now syncs
playbackRatefrom active speed regions (lines 971-976), which addresses the main concern from the past review. However,play()is still called on every effect run whenisPlayingis true (line 990), which may cause unnecessary churn.Consider guarding the
play()call to only execute when the webcam video is paused:♻️ Proposed refinement
if (Math.abs(webcamVideo.currentTime - currentTime) > 0.15) { webcamVideo.currentTime = currentTime; } - webcamVideo.play().catch(() => { - // Ignore webcam autoplay restoration failures. - }); + if (webcamVideo.paused) { + webcamVideo.play().catch(() => { + // Ignore webcam autoplay restoration failures. + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoPlayback.tsx` around lines 965 - 1003, The effect that syncs the webcam video (useEffect referencing webcamVideoRef, currentTime, isPlaying, speedRegions, webcamVideoPath) should avoid calling webcamVideo.play() on every run; change the logic so after setting playbackRate and adjusting currentTime you only call webcamVideo.play() if isPlaying is true AND webcamVideo.paused is true (or webcamVideo.readyState indicates it isn’t already playing), so you don't trigger unnecessary play() churn; keep the existing catch to ignore autoplay failures.electron/ipc/handlers.ts (1)
603-614: Consider extracting duplicated session initialization logic.Lines 603-614 and 639-650 contain identical logic for extracting media from loaded projects and initializing the recording session state. This could be extracted to a helper function to reduce duplication.
♻️ Proposed refactor
+function initializeSessionFromProject(project: { media?: unknown; videoPath?: unknown }) { + const media = + normalizeProjectMedia(project.media) ?? + (typeof project.videoPath === "string" + ? { + screenVideoPath: + normalizeVideoSourcePath(project.videoPath) ?? project.videoPath, + } + : null); + setCurrentRecordingSessionState(media ? { ...media, createdAt: Date.now() } : null); +}Then use
initializeSessionFromProject(project)in both handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 603 - 614, Extract the duplicated logic that builds a media object from a project into a helper like initializeSessionFromProject(project): move the block that casts project to rawProject, computes media via normalizeProjectMedia(rawProject.media) or fallback using normalizeVideoSourcePath(rawProject.videoPath), then calls setCurrentRecordingSessionState(media ? { ...media, createdAt: Date.now() } : null). Replace the two duplicated inline blocks with a single call to initializeSessionFromProject(project) in both handlers to keep behavior identical and re-use normalizeProjectMedia, normalizeVideoSourcePath, and setCurrentRecordingSessionState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 402-416: The startRecording webcam access catch block does not
update UI state when getUserMedia fails; update the catch handler in
useScreenRecorder's startRecording flow to call setWebcamEnabledState(false)
(and ensure webcamStream.current stays null) after logging/toasting the error so
the UI reflects that the webcam is disabled when acquisition fails.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 603-614: Extract the duplicated logic that builds a media object
from a project into a helper like initializeSessionFromProject(project): move
the block that casts project to rawProject, computes media via
normalizeProjectMedia(rawProject.media) or fallback using
normalizeVideoSourcePath(rawProject.videoPath), then calls
setCurrentRecordingSessionState(media ? { ...media, createdAt: Date.now() } :
null). Replace the two duplicated inline blocks with a single call to
initializeSessionFromProject(project) in both handlers to keep behavior
identical and re-use normalizeProjectMedia, normalizeVideoSourcePath, and
setCurrentRecordingSessionState.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 965-1003: The effect that syncs the webcam video (useEffect
referencing webcamVideoRef, currentTime, isPlaying, speedRegions,
webcamVideoPath) should avoid calling webcamVideo.play() on every run; change
the logic so after setting playbackRate and adjusting currentTime you only call
webcamVideo.play() if isPlaying is true AND webcamVideo.paused is true (or
webcamVideo.readyState indicates it isn’t already playing), so you don't trigger
unnecessary play() churn; keep the existing catch to ignore autoplay failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e972a806-5667-47bf-9032-560e83e7765a
📒 Files selected for processing (8)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/preload.tssrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/hooks/useScreenRecorder.tssrc/vite-env.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/vite-env.d.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/lib/exporter/videoExporter.ts (1)
120-148:⚠️ Potential issue | 🔴 CriticalThe webcam tail can still deadlock the MP4 export.
AsyncVideoFrameQueue.close()does not drain buffered frames (src/lib/exporter/asyncVideoFrameQueue.ts, Lines 42-49), and this queue is only destroyed in the outerfinallyafterawait webcamDecodePromise. That still leaves the producer able to block forever in the 12-frame backpressure loop once the screen path stops consuming and the webcam stream runs longer. Tear the queue down before awaiting the webcam decoder, or stop webcam decoding after the last screen frame.Also applies to: 221-221, 269-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 120 - 148, The webcam producer can deadlock because AsyncVideoFrameQueue.close() doesn't drain buffered frames and the producer may block in the while(backpressure) loop; update the teardown so the queue is torn down before awaiting webcamDecodePromise: ensure webcamFrameQueue is signalled/closed (or failed) immediately when the screen consumer finishes (or when cancellation occurs) rather than waiting for await webcamDecodePromise, and modify the webcam decoding flow (webcamDecoder.decodeAll and its catch/finally handlers) to stop producing when webcamFrameQueue is closed or when you call an explicit stop flag so the producer won't block in the 12-frame loop; reference AsyncVideoFrameQueue.close(), webcamFrameQueue, webcamDecodePromise, and webcamDecoder.decodeAll when making the change.src/lib/exporter/gifExporter.ts (1)
169-197:⚠️ Potential issue | 🔴 CriticalThe webcam tail can still deadlock the GIF export.
AsyncVideoFrameQueue.close()only marks EOF; it does not drain buffered frames (src/lib/exporter/asyncVideoFrameQueue.ts, Lines 42-49). Here, the queue is only destroyed in the outerfinally, which runs afterawait webcamDecodePromise. If the webcam stream is more than 12 frames longer than the screen stream, the producer can still park forever inwhile (webcamFrameQueue.length >= 12)and this await never resolves. Destroy/drain the queue before awaiting the webcam decoder once the screen decode loop ends, or cap webcam decoding to the screen frame count.Also applies to: 242-242, 287-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.ts` around lines 169 - 197, The webcam producer can deadlock waiting for webcamFrameQueue.length < 12 because the consumer awaits webcamDecodePromise before the queue is closed/drained; update the logic around webcamDecodePromise/webcamFrameQueue so the queue is closed or drained as soon as the screen decode/consumer finishes (instead of only in the outer finally after awaiting webcamDecodePromise) — e.g., when the screen decode loop ends call webcamFrameQueue.close() (or fail with an error) to unblock the producer, or limit the webcamDecoder.decodeAll call to the screen frame count; apply the same fix for the other similar webcam decode blocks that create webcamFrameQueue/webcamDecodePromise (the other occurrences around the gif export flow).
🤖 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/lib/exporter/gifExporter.ts`:
- Around line 210-216: The dequeue/render block can throw or be cancelled
causing videoFrame and webcamFrame to leak; wrap the await
webcamFrameQueue.dequeue() and await this.renderer!.renderFrame(...) in a
try/finally that always calls videoFrame.close() and webcamFrame?.close() to
release resources, and after the dequeue but before rendering check and bail if
this.cancelled or !this.renderer (so you don't call renderer.renderFrame on a
cleaned-up renderer); update the logic around webcamFrameQueue.dequeue,
this.renderer!.renderFrame, videoFrame.close, webcamFrame?.close and
this.cancelled accordingly.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 162-168: The block that dequeues webcamFrame and calls
this.renderer!.renderFrame may throw or be cancelled, leaking
videoFrame/webcamFrame; wrap the dequeue+render logic in a local try/finally so
videoFrame.close() and webcamFrame?.close() always run, and after awaiting
webcamFrameQueue.dequeue() re-check cancellation/this.renderer before calling
this.renderer!.renderFrame to avoid operating on a cleaned-up exporter state
(references: webcamFrameQueue.dequeue(), this.renderer!.renderFrame(),
videoFrame.close(), webcamFrame?.close()).
---
Duplicate comments:
In `@src/lib/exporter/gifExporter.ts`:
- Around line 169-197: The webcam producer can deadlock waiting for
webcamFrameQueue.length < 12 because the consumer awaits webcamDecodePromise
before the queue is closed/drained; update the logic around
webcamDecodePromise/webcamFrameQueue so the queue is closed or drained as soon
as the screen decode/consumer finishes (instead of only in the outer finally
after awaiting webcamDecodePromise) — e.g., when the screen decode loop ends
call webcamFrameQueue.close() (or fail with an error) to unblock the producer,
or limit the webcamDecoder.decodeAll call to the screen frame count; apply the
same fix for the other similar webcam decode blocks that create
webcamFrameQueue/webcamDecodePromise (the other occurrences around the gif
export flow).
In `@src/lib/exporter/videoExporter.ts`:
- Around line 120-148: The webcam producer can deadlock because
AsyncVideoFrameQueue.close() doesn't drain buffered frames and the producer may
block in the while(backpressure) loop; update the teardown so the queue is torn
down before awaiting webcamDecodePromise: ensure webcamFrameQueue is
signalled/closed (or failed) immediately when the screen consumer finishes (or
when cancellation occurs) rather than waiting for await webcamDecodePromise, and
modify the webcam decoding flow (webcamDecoder.decodeAll and its catch/finally
handlers) to stop producing when webcamFrameQueue is closed or when you call an
explicit stop flag so the producer won't block in the 12-frame loop; reference
AsyncVideoFrameQueue.close(), webcamFrameQueue, webcamDecodePromise, and
webcamDecoder.decodeAll when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6945cfbd-92a7-4fd2-8081-a252831e7123
📒 Files selected for processing (2)
src/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/hooks/useScreenRecorder.ts (2)
402-421: Inconsistent indentation in video constraints object.The
videoconstraint object properties (lines 407-410) are not properly indented relative to the parent object structure. This appears to be a formatting artifact.Proposed fix
if (webcamEnabled) { try { webcamStream.current = await navigator.mediaDevices.getUserMedia({ audio: false, video: { - width: { ideal: WEBCAM_TARGET_WIDTH }, - height: { ideal: WEBCAM_TARGET_HEIGHT }, - frameRate: { ideal: WEBCAM_TARGET_FRAME_RATE, max: WEBCAM_TARGET_FRAME_RATE }, - }, + width: { ideal: WEBCAM_TARGET_WIDTH }, + height: { ideal: WEBCAM_TARGET_HEIGHT }, + frameRate: { ideal: WEBCAM_TARGET_FRAME_RATE, max: WEBCAM_TARGET_FRAME_RATE }, + }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 402 - 421, The video constraint object in the getUserMedia call is mis-indented; update the block inside navigator.mediaDevices.getUserMedia so the video properties (width, height, frameRate) are properly aligned under the video key for readability and to match surrounding style. Locate the getUserMedia call in useScreenRecorder (check webcamEnabled, webcamStream.current usage) and reformat the video: { width: { ideal: ... }, height: { ideal: ... }, frameRate: { ideal: ..., max: ... } } so the braces and properties line up with the parent object; leave logic around setWebcamEnabledState and toast.error unchanged.
262-295: Using a ref forstopRecordingcapturesfinalizeRecordingby closure.
stopRecordingis defined as a ref initialized with a function that capturesfinalizeRecording. IffinalizeRecording's dependencies ever change,stopRecording.currentwould still hold the stale reference. Currently this works becauseteardownMediahas an empty dependency array and is stable, but this pattern is fragile for future maintenance.Consider converting to
useCallbackwith explicit dependencies, or update the ref's function when dependencies change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 262 - 295, The stopRecording ref captures finalizeRecording by closure and can become stale if finalizeRecording changes; replace this pattern by defining stopRecording as a stable callback with useCallback (include finalizeRecording, screenRecorder, webcamRecorder, startTime, recordingId, and teardownMedia as dependencies) so the latest finalizeRecording is always used, or if you prefer to keep the ref approach, update stopRecording.current inside a useEffect whenever finalizeRecording (or any of the other captured values) changes; target the stopRecording ref declaration and any place that reads stopRecording.current, and ensure teardownMedia remains stable in the dependency list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 402-421: The video constraint object in the getUserMedia call is
mis-indented; update the block inside navigator.mediaDevices.getUserMedia so the
video properties (width, height, frameRate) are properly aligned under the video
key for readability and to match surrounding style. Locate the getUserMedia call
in useScreenRecorder (check webcamEnabled, webcamStream.current usage) and
reformat the video: { width: { ideal: ... }, height: { ideal: ... }, frameRate:
{ ideal: ..., max: ... } } so the braces and properties line up with the parent
object; leave logic around setWebcamEnabledState and toast.error unchanged.
- Around line 262-295: The stopRecording ref captures finalizeRecording by
closure and can become stale if finalizeRecording changes; replace this pattern
by defining stopRecording as a stable callback with useCallback (include
finalizeRecording, screenRecorder, webcamRecorder, startTime, recordingId, and
teardownMedia as dependencies) so the latest finalizeRecording is always used,
or if you prefer to keep the ref approach, update stopRecording.current inside a
useEffect whenever finalizeRecording (or any of the other captured values)
changes; target the stopRecording ref declaration and any place that reads
stopRecording.current, and ensure teardownMedia remains stable in the dependency
list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13bd737a-b30a-4c9f-bb9a-b21ccb56bce4
📒 Files selected for processing (2)
src/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.ts
marcusschiesser
commented
Mar 17, 2026
Contributor
Author
marcusschiesser
left a comment
marcusschiesser
left a comment
There was a problem hiding this comment.
reviewed code and tested it locally
Owner
|
Thanks @marcusschiesser for working on this 🙏. Will take a look this evening when I get the chance. |
siddharthvaddem
approved these changes
Mar 18, 2026
Owner
siddharthvaddem
left a comment
siddharthvaddem
left a comment
There was a problem hiding this comment.
Thanks a ton for working on this 🙏 This has been requested for a really long time.
This was referenced Mar 19, 2026
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.
Description
Type of Change
Related Issue(s)
Fixes #193
Screenshots / Video
Screenshot:
Shows webcam toggle:
Video:
showing the added overlay
export-1773746807492.mp4
Checklist
Summary by CodeRabbit
New Features
Chores
Tests