Add dynamic webcam overlay dimensions#682
Open
surim0n wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces hardcoded ChangesNon-finite Webcam Dimension Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 |
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
surim0n
force-pushed
the
codex/dynamic-webcam-overlay-shape
branch
from
e8eb226 to
9567b9c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/exporter/modernVideoExporter.ts (1)
2048-2048: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the shared webcam default constant in native static-layout size fallback.
Line 2048 still uses a literal
40; switching toDEFAULT_WEBCAM_SIZEkeeps this path consistent with normalized/renderer defaults and avoids future drift.Suggested change
const rawSize = getWebcamOverlaySizePx({ containerWidth: this.config.width, containerHeight: this.config.height, - sizePercent: webcam.width ?? webcam.size ?? 40, + sizePercent: webcam.width ?? webcam.size ?? DEFAULT_WEBCAM_SIZE, margin, zoomScale: 1, reactToZoom: webcam.reactToZoom ?? true, });🤖 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/lib/exporter/modernVideoExporter.ts` at line 2048, Replace the hardcoded literal value 40 with the DEFAULT_WEBCAM_SIZE constant in the sizePercent property fallback. Locate the line containing sizePercent: webcam.width ?? webcam.size ?? 40 and change the final 40 to DEFAULT_WEBCAM_SIZE to maintain consistency with the normalized and renderer defaults throughout the codebase.
🤖 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.
Nitpick comments:
In `@src/lib/exporter/modernVideoExporter.ts`:
- Line 2048: Replace the hardcoded literal value 40 with the DEFAULT_WEBCAM_SIZE
constant in the sizePercent property fallback. Locate the line containing
sizePercent: webcam.width ?? webcam.size ?? 40 and change the final 40 to
DEFAULT_WEBCAM_SIZE to maintain consistency with the normalized and renderer
defaults throughout the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb536aa7-72f9-4fcf-a33c-0db6567a0e5d
📒 Files selected for processing (5)
src/components/video-editor/projectPersistence.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.nativeStaticLayout.test.tssrc/lib/exporter/modernVideoExporter.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.
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
Bug Fixes
Tests