feat: Add webcam size with slider#345
Open
GarryLaly wants to merge 3 commits intosiddharthvaddem:mainfrom
Open
feat: Add webcam size with slider#345GarryLaly wants to merge 3 commits intosiddharthvaddem:mainfrom
GarryLaly wants to merge 3 commits intosiddharthvaddem:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds a webcam sizing preset feature: a numeric webcamSizePreset (default 25) threaded through UI, editor state/history, composite layout computation, exporters/frame rendering, project persistence, and i18n; SettingsPanel exposes a slider for picture-in-picture webcam sizing and layout logic uses a geometric-mean reference for scaling. Changes
Sequence DiagramsequenceDiagram
participant User
participant SettingsPanel
participant VideoEditor as VideoEditor (State)
participant VideoPlayback
participant LayoutUtils
participant CompositeLayout
User->>SettingsPanel: Drag webcam size slider
SettingsPanel->>SettingsPanel: onWebcamSizePresetChange(pct)
SettingsPanel->>VideoEditor: onWebcamSizePresetChange callback
VideoEditor->>VideoEditor: Update editorState.webcamSizePreset
VideoEditor->>VideoPlayback: Pass webcamSizePreset prop
VideoPlayback->>LayoutUtils: layoutVideoContent(params including webcamSizePreset)
LayoutUtils->>CompositeLayout: computeCompositeLayout(webcamSizePreset)
CompositeLayout->>CompositeLayout: compute referenceDim (sqrt) & clamp fraction
CompositeLayout->>VideoPlayback: Return composite layout
VideoPlayback->>User: Render resized webcam overlay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Apr 5, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/compositeLayout.test.ts (1)
131-150: Rename this test or assert the centering contract again.The title still says the stack is centered, but the assertions only verify full-canvas placement and never use
maxContentSize. As written, this would stay green even if centering was removed accidentally.🧪 Suggested adjustment
-it("centers the combined screen and webcam stack in vertical stack mode", () => { +it("fills the canvas in vertical stack mode", () => {If centering is still the intended contract, keep the current name and restore assertions that actually use
maxContentSize.🤖 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 131 - 150, The test "centers the combined screen and webcam stack in vertical stack mode" currently verifies full-canvas placement but never exercises maxContentSize, so either rename the test to reflect full-canvas behavior or restore assertions to verify centering using maxContentSize; update the test around computeCompositeLayout to assert that the combined content width/height respects maxContentSize and is centered within canvas (e.g., check that (canvas.width - maxContentSize.width)/2 equals the left x offset of the stacked group and similarly for vertical offsets), or change the test name to something like "fills canvas in vertical-stack mode when maxContentSize >= canvas" if you intend to keep the existing assertions.src/components/video-editor/types.ts (1)
6-9: Export the slider bounds from the same source as the default.
10and50are now duplicated insrc/lib/compositeLayout.tsandsrc/components/video-editor/projectPersistence.ts, while only25is shared here. If the range changes later, preview, export, persistence, and tests can drift silently.♻️ Suggested cleanup
/** Webcam size as a percentage of the canvas reference dimension (10–50). */ export type WebcamSizePreset = number; +export const MIN_WEBCAM_SIZE_PRESET: WebcamSizePreset = 10; +export const MAX_WEBCAM_SIZE_PRESET: WebcamSizePreset = 50; export const DEFAULT_WEBCAM_SIZE_PRESET: WebcamSizePreset = 25;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/types.ts` around lines 6 - 9, Declare and export explicit min/max constants alongside DEFAULT_WEBCAM_SIZE_PRESET in the same module (e.g., MIN_WEBCAM_SIZE_PRESET and MAX_WEBCAM_SIZE_PRESET) and change usages in compositeLayout and projectPersistence to import these constants instead of hardcoded 10 and 50; update any related types or comments for WebcamSizePreset if needed so the slider bounds and default come from a single source of truth.
🤖 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 61-65: webcamSizeToFraction currently lets NaN propagate through
Math.min/Math.max; update the function (webcamSizeToFraction) to guard the
incoming percent by first checking Number.isFinite(percent) (or
!Number.isNaN(percent)) and substituting a sensible default (e.g., 10) when it’s
invalid, then perform the existing clamping (clamped) and return clamped/100;
ensure the guard covers non-numeric and infinite values so scale/width/height
won't become NaN.
---
Nitpick comments:
In `@src/components/video-editor/types.ts`:
- Around line 6-9: Declare and export explicit min/max constants alongside
DEFAULT_WEBCAM_SIZE_PRESET in the same module (e.g., MIN_WEBCAM_SIZE_PRESET and
MAX_WEBCAM_SIZE_PRESET) and change usages in compositeLayout and
projectPersistence to import these constants instead of hardcoded 10 and 50;
update any related types or comments for WebcamSizePreset if needed so the
slider bounds and default come from a single source of truth.
In `@src/lib/compositeLayout.test.ts`:
- Around line 131-150: The test "centers the combined screen and webcam stack in
vertical stack mode" currently verifies full-canvas placement but never
exercises maxContentSize, so either rename the test to reflect full-canvas
behavior or restore assertions to verify centering using maxContentSize; update
the test around computeCompositeLayout to assert that the combined content
width/height respects maxContentSize and is centered within canvas (e.g., check
that (canvas.width - maxContentSize.width)/2 equals the left x offset of the
stacked group and similarly for vertical offsets), or change the test name to
something like "fills canvas in vertical-stack mode when maxContentSize >=
canvas" if you intend to keep the existing assertions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62b66e5b-2a2c-4608-9d85-de9d7876be55
📒 Files selected for processing (15)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/hooks/useEditorHistory.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/compositeLayout.test.tssrc/lib/compositeLayout.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/compositeLayout.ts (1)
154-155: Consider lowercase naming for the computed variable.
MAX_STAGE_FRACTIONuses constant-style naming (SCREAMING_CASE) but is now a computed value derived from input. A name likestageFractionwould better signal that it varies per call. Very minor style nit.♻️ Optional rename
- const MAX_STAGE_FRACTION = webcamSizeToFraction(webcamSizePreset); + const stageFraction = webcamSizeToFraction(webcamSizePreset);Then update line 214-215:
- const maxWidth = Math.max(transform.minSize, referenceDim * MAX_STAGE_FRACTION); - const maxHeight = Math.max(transform.minSize, referenceDim * MAX_STAGE_FRACTION); + const maxWidth = Math.max(transform.minSize, referenceDim * stageFraction); + const maxHeight = Math.max(transform.minSize, referenceDim * stageFraction);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/compositeLayout.ts` around lines 154 - 155, Rename the computed constant MAX_STAGE_FRACTION to a lowercased, non-constant-style name (e.g., stageFraction) because it’s derived from input via webcamSizeToFraction(webcamSizePreset); update all references to MAX_STAGE_FRACTION (including usages near the previous lines referenced around where MAX_STAGE_FRACTION was used) to the new name so the code consistently reflects that this value varies per call.
🤖 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/lib/compositeLayout.ts`:
- Around line 154-155: Rename the computed constant MAX_STAGE_FRACTION to a
lowercased, non-constant-style name (e.g., stageFraction) because it’s derived
from input via webcamSizeToFraction(webcamSizePreset); update all references to
MAX_STAGE_FRACTION (including usages near the previous lines referenced around
where MAX_STAGE_FRACTION was used) to the new name so the code consistently
reflects that this value varies per call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8aaa7c4a-60c9-4bca-9521-50bd05d5665f
📒 Files selected for processing (1)
src/lib/compositeLayout.ts
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.
Pull Request Template
Description
Continues and improves the webcam resize feature from PR #289. Merges the original work with main, resolves all conflicts (including new
webcamMaskShapefeature), fixes broken type-checking and missing wiring, and replaces the small/medium/large preset dropdown with a continuous percentage slider (10%–50%) for finer control over webcam overlay size.Motivation
PR #289 introduced webcam size presets but had several issues:
MAX_STAGE_FRACTIONwas referenced at module scope before being defined, causing TypeScript compilation errorswebcamSizePresetwas never wired through the rendering pipeline (canvas preview, export, project persistence), so changing the preset had no visible effectcanvasWidth/canvasHeightindependently instead of a unified reference dimensionType of Change
Related Issue(s)
Screenshots / Video
Screenshot:
Video:
Screen.Recording.2026-04-05.at.20.05.27.mov
Testing
webcamSizePreset) — verify it defaults to 25%npx vitest run --environment node src/lib/compositeLayout.test.ts— all 9 tests should passChecklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Tests
Localization