Add dynamic webcam overlay dimensions#682
Open
surim0n wants to merge 1 commit into
Open
Conversation
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 (22)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThe PR replaces the single square webcam overlay size model with independent ChangesRectangular Webcam Overlay
Sequence Diagram(s)sequenceDiagram
participant SettingsPanel
participant WebcamCropControl
participant getCropMatchedWebcamHeightPercent
participant updateWebcam
SettingsPanel->>WebcamCropControl: render with webcamWidth, webcamHeight
WebcamCropControl-->>SettingsPanel: onCropChange(cropRegion, previewFrame)
SettingsPanel->>getCropMatchedWebcamHeightPercent: webcamWidth, webcamHeight, previewFrame.w/h, cropRegion
getCropMatchedWebcamHeightPercent-->>SettingsPanel: matched heightPercent
SettingsPanel->>updateWebcam: patch { cropRegion, height: matchedHeight }
sequenceDiagram
participant VideoPlayback
participant getCropMatchedWebcamHeightPercent
participant getWebcamOverlayDimensionsPx
participant getWebcamOverlayPosition
participant BubbleElement
VideoPlayback->>getCropMatchedWebcamHeightPercent: webcamWidth, rawWebcamHeight, videoDims, cropRegion
getCropMatchedWebcamHeightPercent-->>VideoPlayback: webcamHeight
VideoPlayback->>getWebcamOverlayDimensionsPx: containerDims, webcamWidth, webcamHeight, margin, zoom
getWebcamOverlayDimensionsPx-->>VideoPlayback: { width, height } px
VideoPlayback->>getWebcamOverlayPosition: containerDims, width, height, margin, preset
getWebcamOverlayPosition-->>VideoPlayback: { x, y }
VideoPlayback->>BubbleElement: style { width, height, aspectRatio, clipPath, boxShadow }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
surim0n
force-pushed
the
codex/dynamic-webcam-overlay-shape
branch
from
1b2260a to
f58576d
Compare
![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: 5
🤖 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 `@src/components/video-editor/projectPersistence.ts`:
- Around line 1030-1040: The size property at line 1030 can diverge from the
width property (lines 1031-1035) when persisted data has webcam.width but no
webcam.size, causing size to default while width gets restored from the data. To
fix this, update the size property to follow the same fallback logic as width:
first check if webcam.width is a finite number and clamp it, then fall back to
checking webcam.size, and finally default to DEFAULT_WEBCAM_SIZE. This ensures
the size and width properties remain synchronized and consistent regardless of
which properties exist in the persisted data.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 3920-3933: In the onCropChange callback within SettingsPanel.tsx,
the call to getCropMatchedWebcamHeightPercent is passing webcamWidth as both the
first and second parameters. The function expects the first parameter to be
widthPercent and the second parameter to be heightPercent. Replace the second
parameter (currently webcamWidth) with webcamHeight to ensure correct height
calculations when the crop region changes.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 2500-2503: The hardcoded fallback values of 50 in the
frameRenderer.ts file for widthPercent and heightPercent calculations do not
align with the playback defaults which use DEFAULT_WEBCAM_SIZE set to 40,
causing size mismatches for legacy webcam settings. Import DEFAULT_WEBCAM_SIZE
from src/components/video-editor/types.ts and replace both instances of the 50
fallback values (in the widthPercent assignment and in the heightPercent
parameter passed to getCropMatchedWebcamHeightPercent) with DEFAULT_WEBCAM_SIZE
to ensure consistency between preview and export rendering.
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 2919-2925: The call to getCropMatchedWebcamHeightPercent is
double-applying the crop aspect because renderableWebcamSource dimensions are
already crop-trimmed in the cache-backed path for non-default crops, yet the
webcam.cropRegion is still being passed as a parameter. To fix this,
conditionally pass the cropRegion parameter only when renderableWebcamSource has
not already been crop-trimmed, or pass null/undefined for the cropRegion
argument when the source dimensions are already cropped. This ensures the crop
aspect is only applied once during the heightPercent computation.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 1505-1507: The nullish coalescing operator at lines 1505 and 1506
does not filter out NaN values, so width and height variables can remain
non-finite. This causes Math.abs(width - height) at line 1507 to potentially
return NaN, which makes the comparison NaN > 0.001 evaluate to false and
incorrectly bypasses the unsupported-rectangular-webcam-overlay guard for
invalid persisted values. Add validation using isFinite() on the width and
height variables to ensure they are valid finite numbers before performing the
Math.abs comparison in the return statement.
🪄 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: c107421f-048b-4464-8a0f-e07fb5a4cd12
📒 Files selected for processing (22)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/WebcamCropControl.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/types.tssrc/components/video-editor/webcamOverlay.test.tssrc/components/video-editor/webcamOverlay.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/it/settings.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/nl/settings.jsonsrc/i18n/locales/pt-BR/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.ts
surim0n
force-pushed
the
codex/dynamic-webcam-overlay-shape
branch
from
f58576d to
e8eb226
Compare
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
sizevalue for existing projects.Verification
npx vitest --run src/components/video-editor/webcamOverlay.test.ts src/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tsnpx vitest --run src/lib/exporter/frameRenderer.test.ts src/lib/exporter/modernFrameRenderer.test.ts src/components/video-editor/webcamOverlay.test.ts src/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tsnpx tsc --noEmit --noUnusedLocals false --noUnusedParameters falsenpx biome check src/components/video-editor/WebcamCropControl.tsx src/components/video-editor/webcamOverlay.ts src/components/video-editor/webcamOverlay.test.ts src/components/video-editor/types.ts src/components/video-editor/projectPersistence.ts src/lib/exporter/modernVideoExporter.ts src/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tsKnown existing check failures
npx tsc --noEmitfails on existing unused code insrc/components/video-editor/videoPlayback/zoomRegionUtils.ts.npm run i18n:checkfails on existing missing/extra locale keys unrelated to these new webcam labels.npm testhas one unrelated failure inelectron/ipc/paths/binaries.test.tsaround Windows helper path resolution; the focused renderer/webcam tests pass.Summary by CodeRabbit