Add selectable webcam layout presets (Picture in Picture, Vertical Stack)#241
Merged
siddharthvaddem merged 5 commits intosiddharthvaddem:mainfrom Mar 19, 2026
Conversation
📝 WalkthroughWalkthroughUpdates Node/npm versions and adds a preset-driven webcam layout system with unified camera-permission handling; threads webcam layout preset through editor state, UI, playback, exporters, and rendering while replacing the old overlay module and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as VideoEditor/UI
participant Hook as useScreenRecorder
participant Helper as requestCameraAccess
participant State as EditorState
participant Renderer as FrameRenderer
participant Exporter as Exporter (GIF/MP4)
User->>UI: toggle webcam
UI->>Hook: setWebcamEnabled(true)
Hook->>Helper: requestCameraAccess()
Helper-->>Hook: CameraAccessResult
Hook-->>UI: webcam enabled state update
UI->>State: pushState(webcamLayoutPreset)
UI->>Renderer: renderFrame(webcamFrame, webcamSize, webcamLayoutPreset)
Renderer->>Renderer: computeCompositeLayout(canvas, screen, webcamSize, preset)
Renderer-->>UI: computed layout (screenRect, webcamRect)
UI->>Exporter: export(..., webcamSize, webcamLayoutPreset)
Exporter->>Renderer: renderFrame(...) (uses composite layout)
Exporter-->>User: exported asset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
marcusschiesser
commented
Mar 19, 2026
| return ( | ||
| <TooltipProvider> | ||
| {content} | ||
| <Toaster theme="dark" className="pointer-events-auto" /> |
Contributor
Author
There was a problem hiding this comment.
bug fix: toaster wasn't working in the control panel - now it is working
marcusschiesser
commented
Mar 19, 2026
| @@ -0,0 +1 @@ | |||
| 22.22.1 | |||
Contributor
Author
There was a problem hiding this comment.
now you can use the right node version with nvm use
marcusschiesser
commented
Mar 19, 2026
Comment on lines
+6
to
+10
| "packageManager": "npm@10.9.4", | ||
| "engines": { | ||
| "node": "22.22.1", | ||
| "npm": "10.9.4" | ||
| }, |
Contributor
Author
There was a problem hiding this comment.
this prevents generating a new lock file when someone is using npm 11
marcusschiesser
commented
Mar 19, 2026
| return "unknown"; | ||
| } | ||
|
|
||
| export async function requestCameraAccess(): Promise<CameraAccessResult> { |
Contributor
Author
There was a problem hiding this comment.
centralizes camera access request (electron or navigator)
marcusschiesser
commented
Mar 19, 2026
| const MIN_SIZE = 96; | ||
| const MAX_BORDER_RADIUS = 24; | ||
| const WEBCAM_LAYOUT_PRESET_MAP: Record<WebcamLayoutPreset, WebcamLayoutPresetDefinition> = { | ||
| "picture-in-picture": { |
Contributor
Author
There was a problem hiding this comment.
here we can add more presets
Contributor
Author
|
@siddharthvaddem good idea with making the webcam layout editable - this PR is starting with some presets - would be great if you can merge it |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useScreenRecorder.ts (1)
161-174:⚠️ Potential issue | 🟠 MajorHandle
requestCameraAccess()exceptions insetWebcamEnabled.
At Line 161, a rejected permission call currently escapes this function, which can skip user-facing error handling and cause unhandled promise rejection behavior in the toggle flow.💡 Suggested fix
const setWebcamEnabled = useCallback(async (enabled: boolean) => { if (!enabled) { setWebcamEnabledState(false); return true; } - - const accessResult = await requestCameraAccess(); + let accessResult; + try { + accessResult = await requestCameraAccess(); + } catch (error) { + console.error("Failed to request camera access:", error); + toast.error("Failed to request camera access."); + return false; + } if (!accessResult.success) { toast.error("Failed to request camera access."); return 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 161 - 174, The setWebcamEnabled function calls requestCameraAccess without catching rejections, so wrap the requestCameraAccess call in a try/catch inside setWebcamEnabled; on catch call toast.error with a user-facing message (and optionally include error.message), ensure you return false on error, and only call setWebcamEnabledState(true) when accessResult.success and accessResult.granted are true.
🧹 Nitpick comments (2)
src/components/video-editor/projectPersistence.ts (1)
347-351: Avoid hard-coding the valid preset list here.This works for the two current presets, but it duplicates the source of truth. When the next preset is added, saved projects will silently normalize back to the default unless this branch is updated too, which undercuts the “easy to add more presets” goal.
♻️ Suggested cleanup
+import { WEBCAM_LAYOUT_PRESETS } from "@/lib/compositeLayout"; + +const VALID_WEBCAM_LAYOUT_PRESETS = new Set<WebcamLayoutPreset>( + WEBCAM_LAYOUT_PRESETS.map(({ value }) => value), +); + +function isWebcamLayoutPreset(value: unknown): value is WebcamLayoutPreset { + return ( + typeof value === "string" && + VALID_WEBCAM_LAYOUT_PRESETS.has(value as WebcamLayoutPreset) + ); +} + return { ... - webcamLayoutPreset: - editor.webcamLayoutPreset === "vertical-stack" || - editor.webcamLayoutPreset === "picture-in-picture" - ? editor.webcamLayoutPreset - : DEFAULT_WEBCAM_LAYOUT_PRESET, + webcamLayoutPreset: isWebcamLayoutPreset(editor.webcamLayoutPreset) + ? editor.webcamLayoutPreset + : DEFAULT_WEBCAM_LAYOUT_PRESET, ... };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 347 - 351, The current normalization for webcamLayoutPreset duplicates the valid-preset list and will regress when new presets are added; replace the inline boolean check with a membership test against the single source-of-truth (e.g., an existing enum/constant like WebcamLayoutPreset or VALID_WEBCAM_LAYOUT_PRESETS) by importing/using that symbol and testing if editor.webcamLayoutPreset is included in it, otherwise fall back to DEFAULT_WEBCAM_LAYOUT_PRESET; update the expression that sets webcamLayoutPreset to use that membership check (referencing webcamLayoutPreset, editor.webcamLayoutPreset, DEFAULT_WEBCAM_LAYOUT_PRESET and the central WebcamLayoutPreset/VALID_WEBCAM_LAYOUT_PRESETS symbol).src/lib/compositeLayout.test.ts (1)
36-59: Add a 9:16 / mismatched-aspect-ratio stack case.This test still uses a 16:9 screen and a 16:9 webcam, so it doesn't exercise the normalization path behind the Shorts/TikTok scenario. A portrait screen plus a differently shaped webcam feed would give this change much better regression coverage.
🧪 Example coverage addition
+ it("centers a portrait stack with a non-matching webcam aspect ratio", () => { + const layout = computeCompositeLayout({ + canvasSize: { width: 1080, height: 1920 }, + maxContentSize: { width: 864, height: 1536 }, + screenSize: { width: 1080, height: 1920 }, + webcamSize: { width: 1920, height: 1080 }, + layoutPreset: "vertical-stack", + }); + + expect(layout).not.toBeNull(); + expect(layout?.webcamRect).not.toBeNull(); + expect(layout?.screenRect.x).toBe(layout?.webcamRect?.x); + expect(layout?.screenRect.width).toBe(layout?.webcamRect?.width); + const stackTop = layout!.screenRect.y; + const stackBottom = layout!.webcamRect!.y + layout!.webcamRect!.height; + expect(stackTop).toBe(Math.floor((1920 - (stackBottom - stackTop)) / 2)); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/compositeLayout.test.ts` around lines 36 - 59, Add a new unit test that calls computeCompositeLayout with a portrait (9:16) screenSize and a mismatched-aspect webcamSize (e.g., screenSize: {width:1080, height:1920}, webcamSize: {width:1280, height:720}), keep layoutPreset: "vertical-stack" and an appropriate canvasSize/maxContentSize to mimic Shorts/TikTok; assert the result is not null and that screenRect and webcamRect are positioned stacked (webcam below screen) and that sizes are normalized to fit maxContentSize (use the same style of expect checks as the existing "centers the combined screen and webcam stack in vertical stack mode" test so it exercises the normalization code path in computeCompositeLayout).
🤖 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/compositeLayout.ts`:
- Around line 105-108: getWebcamLayoutPresetDefinition currently returns
WEBCAM_LAYOUT_PRESET_MAP[preset] which can be undefined for stale/malformed
persisted presets and later causes computeCompositeLayout to crash on
preset.transform; update getWebcamLayoutPresetDefinition to detect a missing
lookup and return a safe default (use the "picture-in-picture" definition from
WEBCAM_LAYOUT_PRESET_MAP) instead of undefined so callers always receive a valid
WebcamLayoutPresetDefinition.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 425-430: The composite layout is being computed with a reserved
webcam size even when renderFrame may receive webcamFrame === null, causing
empty webcam slots; update the computeCompositeLayout call in renderFrame to
pass webcamSize as null when webcamFrame is null (e.g., replace the constant
this.config.webcamSize argument with webcamFrame ? this.config.webcamSize :
null) so computeCompositeLayout and downstream compositeWithShadows receive no
webcam space for frames lacking webcam content.
In `@src/lib/requestCameraAccess.ts`:
- Around line 17-22: The IPC call to window.electronAPI.requestCameraAccess can
reject and must be normalized to a CameraAccessResult; wrap the call in a
try/catch inside requestCameraAccess, call await
window.electronAPI.requestCameraAccess() in the try, and on any thrown error
return a CameraAccessResult like { success: false, granted: false, error:
<string or error object> } (preserve existing electronResult fields when
present). Apply the same try/catch normalization pattern to the similar IPC call
in the later block (lines 33-46) so all electron API failures return a
CameraAccessResult rather than throwing.
---
Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 161-174: The setWebcamEnabled function calls requestCameraAccess
without catching rejections, so wrap the requestCameraAccess call in a try/catch
inside setWebcamEnabled; on catch call toast.error with a user-facing message
(and optionally include error.message), ensure you return false on error, and
only call setWebcamEnabledState(true) when accessResult.success and
accessResult.granted are true.
---
Nitpick comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 347-351: The current normalization for webcamLayoutPreset
duplicates the valid-preset list and will regress when new presets are added;
replace the inline boolean check with a membership test against the single
source-of-truth (e.g., an existing enum/constant like WebcamLayoutPreset or
VALID_WEBCAM_LAYOUT_PRESETS) by importing/using that symbol and testing if
editor.webcamLayoutPreset is included in it, otherwise fall back to
DEFAULT_WEBCAM_LAYOUT_PRESET; update the expression that sets webcamLayoutPreset
to use that membership check (referencing webcamLayoutPreset,
editor.webcamLayoutPreset, DEFAULT_WEBCAM_LAYOUT_PRESET and the central
WebcamLayoutPreset/VALID_WEBCAM_LAYOUT_PRESETS symbol).
In `@src/lib/compositeLayout.test.ts`:
- Around line 36-59: Add a new unit test that calls computeCompositeLayout with
a portrait (9:16) screenSize and a mismatched-aspect webcamSize (e.g.,
screenSize: {width:1080, height:1920}, webcamSize: {width:1280, height:720}),
keep layoutPreset: "vertical-stack" and an appropriate canvasSize/maxContentSize
to mimic Shorts/TikTok; assert the result is not null and that screenRect and
webcamRect are positioned stacked (webcam below screen) and that sizes are
normalized to fit maxContentSize (use the same style of expect checks as the
existing "centers the combined screen and webcam stack in vertical stack mode"
test so it exercises the normalization code path in computeCompositeLayout).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e14f041-9b29-4a4d-9cb9-b2fa0d5676ec
📒 Files selected for processing (21)
.nvmrcpackage.jsonsrc/App.tsxsrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.test.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/hooks/useEditorHistory.tssrc/hooks/useScreenRecorder.tssrc/lib/compositeLayout.test.tssrc/lib/compositeLayout.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.tssrc/lib/requestCameraAccess.tssrc/lib/webcamOverlay.test.tssrc/lib/webcamOverlay.ts
💤 Files with no reviewable changes (2)
- src/lib/webcamOverlay.test.ts
- src/lib/webcamOverlay.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/frameRenderer.ts`:
- Around line 648-664: The preset shadow is being applied unconditionally;
update the block around
getWebcamLayoutPresetDefinition(this.config.webcamLayoutPreset) so that you only
set ctx.shadowColor/blur/offsetX/offsetY when the export shadow controls allow
it — e.g., check this.config.showShadow is true and this.config.shadowIntensity
> 0 before applying preset.shadow, and scale preset.shadow.blur/intensity by
this.config.shadowIntensity when setting ctx.shadowBlur (and adjust color alpha
if needed) so shadows respect the global export controls; keep the existing
roundRect/clip logic but guard and modify preset.shadow usage accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e239b469-c530-4024-b266-891ebe729e80
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/lib/exporter/frameRenderer.tssrc/lib/requestCameraAccess.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/requestCameraAccess.ts
Comment on lines
+648
to
664
| const preset = getWebcamLayoutPresetDefinition(this.config.webcamLayoutPreset); | ||
| ctx.save(); | ||
| ctx.beginPath(); | ||
| ctx.roundRect( | ||
| webcamRect.x, | ||
| webcamRect.y, | ||
| webcamRect.width, | ||
| webcamRect.height, | ||
| webcamRect.borderRadius, | ||
| ); | ||
| ctx.closePath(); | ||
| if (preset.shadow) { | ||
| ctx.shadowColor = preset.shadow.color; | ||
| ctx.shadowBlur = preset.shadow.blur; | ||
| ctx.shadowOffsetX = preset.shadow.offsetX; | ||
| ctx.shadowOffsetY = preset.shadow.offsetY; | ||
| } |
There was a problem hiding this comment.
Webcam preset shadows ignore the export shadow controls.
This branch applies preset.shadow even when showShadow is false or shadowIntensity is 0, so exports can still show a webcam shadow after shadows are disabled in Video Effects.
💡 Suggested fix
- if (preset.shadow) {
+ if (this.config.showShadow && this.config.shadowIntensity > 0 && preset.shadow) {
ctx.shadowColor = preset.shadow.color;
ctx.shadowBlur = preset.shadow.blur;
ctx.shadowOffsetX = preset.shadow.offsetX;
ctx.shadowOffsetY = preset.shadow.offsetY;
}📝 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 preset = getWebcamLayoutPresetDefinition(this.config.webcamLayoutPreset); | |
| ctx.save(); | |
| ctx.beginPath(); | |
| ctx.roundRect( | |
| webcamRect.x, | |
| webcamRect.y, | |
| webcamRect.width, | |
| webcamRect.height, | |
| webcamRect.borderRadius, | |
| ); | |
| ctx.closePath(); | |
| if (preset.shadow) { | |
| ctx.shadowColor = preset.shadow.color; | |
| ctx.shadowBlur = preset.shadow.blur; | |
| ctx.shadowOffsetX = preset.shadow.offsetX; | |
| ctx.shadowOffsetY = preset.shadow.offsetY; | |
| } | |
| const preset = getWebcamLayoutPresetDefinition(this.config.webcamLayoutPreset); | |
| ctx.save(); | |
| ctx.beginPath(); | |
| ctx.roundRect( | |
| webcamRect.x, | |
| webcamRect.y, | |
| webcamRect.width, | |
| webcamRect.height, | |
| webcamRect.borderRadius, | |
| ); | |
| ctx.closePath(); | |
| if (this.config.showShadow && this.config.shadowIntensity > 0 && preset.shadow) { | |
| ctx.shadowColor = preset.shadow.color; | |
| ctx.shadowBlur = preset.shadow.blur; | |
| ctx.shadowOffsetX = preset.shadow.offsetX; | |
| ctx.shadowOffsetY = preset.shadow.offsetY; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/frameRenderer.ts` around lines 648 - 664, The preset shadow
is being applied unconditionally; update the block around
getWebcamLayoutPresetDefinition(this.config.webcamLayoutPreset) so that you only
set ctx.shadowColor/blur/offsetX/offsetY when the export shadow controls allow
it — e.g., check this.config.showShadow is true and this.config.shadowIntensity
> 0 before applying preset.shadow, and scale preset.shadow.blur/intensity by
this.config.shadowIntensity when setting ctx.shadowBlur (and adjust color alpha
if needed) so shadows respect the global export controls; keep the existing
roundRect/clip logic but guard and modify preset.shadow usage accordingly.
Contributor
Author
There was a problem hiding this comment.
@siddharthvaddem i think we should keep shadows (and the other video effects) separate for the webcam, so i think it's still ok to keep it as is for now. later we could have "Video Effects" becoming "Screen Effects" and a dedicated "Webcam Effects"
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!
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
Picture in PictureandVertical StackLayoutaccordion aboveVideo EffectsVertical Stackin both preview and exportMotivation
Type of Change
Screenshots / Video
Screenshot:
Vertical-Stack layout:
PIP layout (existing choice):
New Selector:
Testing
Checklist
Summary by CodeRabbit
New Features
Tests
Chores