feat: webcam camera regions (size / focus / position)#531
Open
Reedaaz wants to merge 5 commits into
Open
Conversation
feat(video-editor): webcam camera regions (size/focus/position) + timeline UX Camera-as-timeline-regions feature set: - Webcam size & focus regions (deterministic interpolation, preview+export). - New webcam position regions: drag camera in preview to create/update an animated position region at the playhead; snaps to corners/center; reuses the region under the playhead; copies an active size region span. - Fullscreen webcam button (focus region focusSize=100, screen hidden). - webcamPositionEnabled gate (on by default, fully reversible). - Timeline: dedicated rows, select/drag/resize/delete, keyboard delete, fit-to-height (no vertical scroll, all tracks visible), draggable height bar. - Backward-compatible persistence (optional arrays, no migration). - Threaded through all 5 export paths; stable empty-array ref to avoid a preview play/pause render loop. Tests: webcam region suites 45/45 green; tsc clean; no new suite failures (4 pre-existing unrelated failures untouched by design). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> @
Webcam size/focus/position regions now span the entire timeline when first created instead of a fixed 3-second window, so the default state covers the whole recording without manual resizing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds timeline-driven webcam region types (size, focus, position) with normalization/interpolation, timeline UI and DnD/selection support, SettingsPanel editing, preview drag/resize interactions, persistence/history, tests, and exporter rendering integration. ChangesWebcam Timeline Regions Feature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
|
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/video-editor/timeline/Item.tsx (1)
285-308:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender a dedicated
webcam-positionchip instead of falling back to annotation UI.At Line 301,
webcam-positioncurrently falls through to the default annotation branch (MessageSquare), which is misleading for timeline semantics.Proposed fix
) : isWebcamFocus ? ( <> <ArrowsOutSimple className="w-3.5 h-3.5 shrink-0" /> <span className="text-[11px] font-semibold tracking-tight whitespace-nowrap"> {webcamFocusPercent !== undefined ? `Focus ${Math.round(webcamFocusPercent)}%` : "Focus"} </span> </> + ) : isWebcamPosition ? ( + <> + <VideoCamera className="w-3.5 h-3.5 shrink-0" /> + <span className="text-[11px] font-semibold tracking-tight whitespace-nowrap"> + {children} + </span> + </> ) : ( <> <MessageSquare className="w-3.5 h-3.5 shrink-0" /> <span className="text-[11px] font-semibold tracking-tight whitespace-nowrap"> {children} </span> </> )}🤖 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/components/video-editor/timeline/Item.tsx` around lines 285 - 308, Add a dedicated branch for the webcam-position case in the JSX conditional so it doesn't fall through to the annotation/default branch; locate the conditional block in Item.tsx around the existing isWebcamSize and isWebcamFocus checks and add an isWebcamPosition check (e.g., isWebcamPosition) that renders a distinct chip with an appropriate icon and label (use the existing small-icon + span pattern, and use webcamPositionLabel or a "Position" fallback) before the final MessageSquare/default branch so timeline semantics are correct.src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx (1)
687-696:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear webcam position selection in fallback deselect path
Line 694–695 and Line 752–753 clear size/focus but not position, so webcam-position selection can stick when
onClearBlockSelectionis absent. Please also align hook deps with used callbacks.Proposed fix
@@ } else { onSelectZoom?.(null); onSelectClip?.(null); onSelectAnnotation?.(null); onSelectAudio?.(null); onSelectWebcamSize?.(null); onSelectWebcamFocus?.(null); + onSelectWebcamPosition?.(null); } @@ onClearBlockSelection, + onSelectWebcamSize, onSelectWebcamFocus, + onSelectWebcamPosition, videoDurationMs, @@ } else { onSelectZoom?.(null); onSelectClip?.(null); onSelectAnnotation?.(null); onSelectAudio?.(null); onSelectWebcamSize?.(null); onSelectWebcamFocus?.(null); + onSelectWebcamPosition?.(null); } @@ onSelectClip, onSelectZoom, + onSelectWebcamSize, onSelectWebcamFocus, + onSelectWebcamPosition, videoDurationMs, ], );Also applies to: 708-722, 745-754, 761-771
🤖 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/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx` around lines 687 - 696, The fallback deselect branch in TimelineCanvas currently clears webcam size and focus but omits clearing webcam position, causing webcam-position selection to persist when onClearBlockSelection is not provided; update the fallback to call onSelectWebcamPosition?.(null) alongside onSelectWebcamSize?.(null) and onSelectWebcamFocus?.(null) (apply the same fix to the other similar fallback blocks in the component), and also update the related useEffect/useCallback dependency arrays to include every onSelect* callback used (e.g., onSelectWebcamPosition, onSelectWebcamSize, onSelectWebcamFocus, onSelectClip, onSelectAnnotation, onSelectAudio, onSelectZoom) so hook deps align with referenced callbacks.src/components/video-editor/VideoEditor.tsx (2)
2278-2283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset all webcam-region selections when loading a project.
Only the size-region selection is cleared here. Focus and position selections from the previous project can leak into the newly loaded project, and if IDs are reused they will stay selected incorrectly.
🤖 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/components/video-editor/VideoEditor.tsx` around lines 2278 - 2283, The reset logic clears many selection states but misses the webcam focus/position region selections, causing previous project's webcam-region focus/position to leak; in the same block where setSelectedZoomId, setSelectedClipId, setSelectedAnnotationId, setSelectedAudioId, and setSelectedWebcamSizeRegionId are cleared, also call the webcam region reset setters (e.g. setSelectedWebcamFocusRegionId(null) and setSelectedWebcamPositionRegionId(null) — or the actual state setter names used in this file for webcam focus and position) so all webcam-region selections are cleared when loading a project.
5391-5470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the webcam region arrays to
handleExport's dependencies.This callback reads
webcamSizeRegions,webcamFocusRegions, andwebcamPositionRegionswhen building the exporter config, but none of them are in the dependency list. Export can therefore use stale webcam-region state until some unrelated dependency happens to recreate the callback.🤖 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/components/video-editor/VideoEditor.tsx` around lines 5391 - 5470, handleExport is missing webcam region state in its dependency array, causing stale webcamSizeRegions, webcamFocusRegions, and webcamPositionRegions to be captured; update the dependency list used when creating the handleExport callback (the array currently listing clearPendingExportSave, videoPath, webcam, resolvedWebcamVideoUrl, etc.) to also include webcamSizeRegions, webcamFocusRegions, and webcamPositionRegions so the callback is recreated whenever those webcam region arrays change.src/components/video-editor/SettingsPanel.tsx (1)
3554-3561:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror default is out of sync with the preview runtime.
This switch falls back to
true, but the preview layer falls back tofalsewhenwebcam.mirroris unset. Existing projects without a persisted mirror flag will render unmirrored footage while the UI shows the toggle as enabled, and the first toggle writes the opposite of what the user expects.Suggested fix
- checked={webcam?.mirror ?? true} + checked={webcam?.mirror ?? false}🤖 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/components/video-editor/SettingsPanel.tsx` around lines 3554 - 3561, The UI toggle for webcam mirroring is defaulting to true while the preview runtime defaults to false, causing mismatches; update the Switch checked prop to use the same fallback as the preview (use webcam?.mirror ?? false) so the Switch, the preview rendering, and the first call to updateWebcam({ mirror }) are consistent; locate the Switch in SettingsPanel (the checked prop currently using webcam?.mirror ?? true) and change its default to false to align behavior.
🧹 Nitpick comments (1)
src/components/video-editor/webcamSizeRegions.ts (1)
30-45: 💤 Low valueConsider extracting shared utilities to reduce duplication.
isFiniteNumber,clamp,toIntegerMs, andlerpare duplicated acrosswebcamSizeRegions.ts,webcamPositionRegions.ts, andwebcamFocusRegions.ts. Extracting them into a shared utility module would reduce maintenance burden.🤖 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/components/video-editor/webcamSizeRegions.ts` around lines 30 - 45, The helper functions isFiniteNumber, clamp, toIntegerMs, and lerp are duplicated across webcamSizeRegions.ts, webcamPositionRegions.ts, and webcamFocusRegions.ts; extract them into a shared utility module (e.g., create a new file exporting isFiniteNumber, clamp, toIntegerMs, lerp with the same type signatures), replace the local duplicates in webcamSizeRegions.ts, webcamPositionRegions.ts, and webcamFocusRegions.ts with imports from that module, and ensure any consumers still use the same function names/signatures and update exports/imports accordingly so types remain consistent.
🤖 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/timeline/model/timelineModel.ts`:
- Around line 133-138: The timeline labels for webcam positions are emitted as
raw numbers and need percent units; update the label construction in the
webcamPositions mapping (use webcamPositionRegions → webcamPositions) to append
"%" to both rounded X and Y values so the label becomes e.g. "12%,34%"; ensure
the change is made where label: `${Math.round(region.positionX *
100)},${Math.round(region.positionY * 100)}` is created in timelineModel.ts so
other code consuming TimelineRenderItem remains unchanged.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1329-1331: The thumbnail capture and export code paths are still
using raw webcamPositionRegions even when the UI feature is disabled; update the
thumbnail capture and both export routines to respect webcamPositionEnabled by
only passing webcamPositionRegions when webcamPositionEnabled is true (otherwise
pass null/undefined or omit them), mirroring the preview/timeline gating;
specifically modify the call sites that currently forward webcamSizeRegions,
webcamFocusRegions, webcamPositionRegions so they conditionally include
webcamPositionRegions based on the webcamPositionEnabled flag (look for
functions handling thumbnail capture and the two export functions that forward
these region props).
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 3025-3089: Window-level pointer listeners (handleMove/handleUp)
are added but not removed on component unmount which can cause stray callbacks;
fix by adding a cleanup that removes window.removeEventListener for
"pointermove", "pointerup", and "pointercancel" and clears webcamPositionDragRef
on teardown (same pattern for the other drag block at 3192-3331). In practice,
wrap the addEventListener calls in the effect where they are registered and
return a cleanup function that calls window.removeEventListener(handleMove),
window.removeEventListener(handleUp) (for both "pointerup" and "pointercancel"),
resets webcamPositionDragRef.current = null, and restores
webcamBubbleRef.current.style.cursor if needed so stale refs/callbacks
(overlayRef, webcamBubbleRef, onWebcamPositionDragStart, onWebcamPositionDrag,
onSelectWebcamPositionRegion) are not invoked after unmount.
- Around line 3135-3190: The top/left resize handlers update
currentLeftPx/currentTopPx but persistResizePosition bails out when
drag.positionRegionId or onWebcamPositionDrag are missing, causing the UI to
snap back; either persist a fallback global position there or prevent those
handles when position callbacks are absent. Update persistResizePosition
(referenced via webcamSizeResizeRef.current and function persistResizePosition)
so that if !drag.positionRegionId || !onWebcamPositionDrag you compute
nextX/nextY from leftPx/topPx the same way and call your global position setter
(e.g., props.onWebcamPositionChange or a local setWebcamPosition state updater),
or alternatively disable the top/left handles where webcamSizeResizeRef is
initialized when onWebcamPositionDrag/onWebcamPositionDragStart are undefined so
those handles never update currentLeftPx/currentTopPx.
In `@src/components/video-editor/webcamOverlay.ts`:
- Around line 306-314: The vertical-reveal path can produce a positive drawY
leaving a blank band; update the logic around canRevealVertically/drawY so that
reveal mode is only used when the computed reveal offset is non-positive (or
clamp drawY to <= 0) and otherwise fall back to coverScale. Concretely, after
computing scale (revealScale vs coverScale) and drawHeight, compute the
tentative revealOffset = (baseSize - drawHeight) / 2 + (safeTargetHeight -
baseSize) and if revealOffset > 0 then treat canRevealVertically as false (use
coverScale) or set drawY = Math.min(revealOffset, 0); adjust the drawY
assignment accordingly to ensure no positive drawY in canRevealVertically branch
(references: canRevealVertically, revealScale, coverScale, drawHeight, drawY,
safeTargetHeight, safeTargetWidth, baseSize).
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 2567-2569: The avoid-cursor call uses the background timeline time
(this.currentTimelineTimeMs) but cursor telemetry is sampled in the cursor
timebase, causing desync when trims/speed mapping are active; change the
argument to getCursorPosition to use the cursor-sampled time instead of
this.currentTimelineTimeMs — either pass the same cursor timestamp source used
elsewhere (the variable or getter that samples cursor telemetry) or convert
timeline time to cursor time via the existing timeline→cursor mapping helper
(e.g., mapTimelineTimeToCursorTime or similar) before calling getCursorPosition
so avoidance and telemetry use the same timebase.
- Around line 2503-2513: The code incorrectly uses the truthiness of focusState
to disable zoom behavior; change the checks to test actual active focus progress
(e.g., focusState?.progress > 0) in both places where zoomScale and reactToZoom
are computed so zoomScale only becomes 1 and reactToZoom only becomes false when
focus is actively in-progress; update the two expressions that currently use
focusState ? ... : ... to use focusState?.progress > 0 ? ... : ... (leave
animationState.appliedScale and webcam.reactToZoom logic unchanged) and adjust
both the call that constructs the object and the getWebcamOverlaySizePx
invocation.
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 3055-3057: The avoidance lookup is using timeline time
(this.currentTimelineTimeMs) instead of the cursor telemetry timestamp, causing
misplacement under trims/speed changes; change the call to
getCursorPosition(...) to use the cursor's telemetry time (the same cursor
timestamp used elsewhere — e.g., the current cursor time variable such as
this.currentCursorTimeMs or the cursor object's timestamp) so getCursorPosition
uses cursor-time, not timeline-time, when computing avoidedPosition for
webcam.avoidCursor and focusState checks.
- Around line 2993-3003: The code currently suppresses zoom-reactive behavior
whenever focusState is truthy; update both places (the zoomScale and reactToZoom
arguments passed to getWebcamOverlaySizePx and the earlier call) to only treat
focus as suppressing zoom when focus is fully active — e.g., replace the
ternaries that use focusState ? ... with checks against the focus
progress/active flag (for example use focusState?.progress === 1 or
focusState?.active) so the expressions become something like
(focusState?.progress === 1 ? 1 : this.animationState.appliedScale || 1) and
(focusState?.progress === 1 ? false : (webcam.reactToZoom ?? true)), ensuring
normal reactToZoom behavior when focus exists but is not fully active; apply the
same change to both occurrences that reference focusState,
animationState.appliedScale, and webcam.reactToZoom.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 1524-1539: The code that accumulates unsupported export reasons
(the reasons array in modernVideoExporter.ts) is missing a check for
webcamPositionRegions which causes position regions to be ignored when using
getNativeStaticLayoutWebcamOverlay; add a check similar to the existing
webcamSizeRegions and webcamFocusRegions checks to push
"unsupported-webcam-position-regions" when (this.config.webcamPositionRegions ??
[]).length > 0 so exports with position interpolation are flagged and routed
away from the native static layout path.
---
Outside diff comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 3554-3561: The UI toggle for webcam mirroring is defaulting to
true while the preview runtime defaults to false, causing mismatches; update the
Switch checked prop to use the same fallback as the preview (use webcam?.mirror
?? false) so the Switch, the preview rendering, and the first call to
updateWebcam({ mirror }) are consistent; locate the Switch in SettingsPanel (the
checked prop currently using webcam?.mirror ?? true) and change its default to
false to align behavior.
In `@src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx`:
- Around line 687-696: The fallback deselect branch in TimelineCanvas currently
clears webcam size and focus but omits clearing webcam position, causing
webcam-position selection to persist when onClearBlockSelection is not provided;
update the fallback to call onSelectWebcamPosition?.(null) alongside
onSelectWebcamSize?.(null) and onSelectWebcamFocus?.(null) (apply the same fix
to the other similar fallback blocks in the component), and also update the
related useEffect/useCallback dependency arrays to include every onSelect*
callback used (e.g., onSelectWebcamPosition, onSelectWebcamSize,
onSelectWebcamFocus, onSelectClip, onSelectAnnotation, onSelectAudio,
onSelectZoom) so hook deps align with referenced callbacks.
In `@src/components/video-editor/timeline/Item.tsx`:
- Around line 285-308: Add a dedicated branch for the webcam-position case in
the JSX conditional so it doesn't fall through to the annotation/default branch;
locate the conditional block in Item.tsx around the existing isWebcamSize and
isWebcamFocus checks and add an isWebcamPosition check (e.g., isWebcamPosition)
that renders a distinct chip with an appropriate icon and label (use the
existing small-icon + span pattern, and use webcamPositionLabel or a "Position"
fallback) before the final MessageSquare/default branch so timeline semantics
are correct.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2278-2283: The reset logic clears many selection states but misses
the webcam focus/position region selections, causing previous project's
webcam-region focus/position to leak; in the same block where setSelectedZoomId,
setSelectedClipId, setSelectedAnnotationId, setSelectedAudioId, and
setSelectedWebcamSizeRegionId are cleared, also call the webcam region reset
setters (e.g. setSelectedWebcamFocusRegionId(null) and
setSelectedWebcamPositionRegionId(null) — or the actual state setter names used
in this file for webcam focus and position) so all webcam-region selections are
cleared when loading a project.
- Around line 5391-5470: handleExport is missing webcam region state in its
dependency array, causing stale webcamSizeRegions, webcamFocusRegions, and
webcamPositionRegions to be captured; update the dependency list used when
creating the handleExport callback (the array currently listing
clearPendingExportSave, videoPath, webcam, resolvedWebcamVideoUrl, etc.) to also
include webcamSizeRegions, webcamFocusRegions, and webcamPositionRegions so the
callback is recreated whenever those webcam region arrays change.
---
Nitpick comments:
In `@src/components/video-editor/webcamSizeRegions.ts`:
- Around line 30-45: The helper functions isFiniteNumber, clamp, toIntegerMs,
and lerp are duplicated across webcamSizeRegions.ts, webcamPositionRegions.ts,
and webcamFocusRegions.ts; extract them into a shared utility module (e.g.,
create a new file exporting isFiniteNumber, clamp, toIntegerMs, lerp with the
same type signatures), replace the local duplicates in webcamSizeRegions.ts,
webcamPositionRegions.ts, and webcamFocusRegions.ts with imports from that
module, and ensure any consumers still use the same function names/signatures
and update exports/imports accordingly so types remain consistent.
🪄 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: 82f6a082-502d-409e-aeac-b3f66ed7a211
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.gitignoresrc/components/launch/hooks/useWebcamPreviewOverlay.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/components/viewport/TimelineCanvas.tsxsrc/components/video-editor/timeline/core/constants.tssrc/components/video-editor/timeline/core/timelineTypes.tssrc/components/video-editor/timeline/hooks/useTimelineDndBindings.tssrc/components/video-editor/timeline/hooks/useTimelineEditorRuntime.tssrc/components/video-editor/timeline/hooks/useTimelineKeyboardShortcuts.tssrc/components/video-editor/timeline/hooks/useTimelineNormalization.tssrc/components/video-editor/timeline/hooks/useTimelineSelection.tssrc/components/video-editor/timeline/hooks/utils/timelineSelectionUtils.tssrc/components/video-editor/timeline/model/timelineModel.tssrc/components/video-editor/types.tssrc/components/video-editor/webcamFocusRegions.test.tssrc/components/video-editor/webcamFocusRegions.tssrc/components/video-editor/webcamOverlay.test.tssrc/components/video-editor/webcamOverlay.tssrc/components/video-editor/webcamPositionRegions.test.tssrc/components/video-editor/webcamPositionRegions.tssrc/components/video-editor/webcamSizeRegions.test.tssrc/components/video-editor/webcamSizeRegions.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.ts
Comment on lines
+133
to
+138
| const webcamPositions: TimelineRenderItem[] = webcamPositionRegions.map((region) => ({ | ||
| id: region.id, | ||
| rowId: WEBCAM_POSITION_ROW_ID, | ||
| span: { start: region.startMs, end: region.endMs }, | ||
| label: `${Math.round(region.positionX * 100)},${Math.round(region.positionY * 100)}`, | ||
| webcamPositionX: region.positionX, |
Contributor
There was a problem hiding this comment.
Add % units to webcam-position timeline labels.
At Line 137, the label is emitted as raw numbers (x,y) which is ambiguous in the timeline UI. Including % aligns with the other webcam labels and avoids misread values.
Proposed fix
- label: `${Math.round(region.positionX * 100)},${Math.round(region.positionY * 100)}`,
+ label: `${Math.round(region.positionX * 100)}%,${Math.round(region.positionY * 100)}%`,📝 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 webcamPositions: TimelineRenderItem[] = webcamPositionRegions.map((region) => ({ | |
| id: region.id, | |
| rowId: WEBCAM_POSITION_ROW_ID, | |
| span: { start: region.startMs, end: region.endMs }, | |
| label: `${Math.round(region.positionX * 100)},${Math.round(region.positionY * 100)}`, | |
| webcamPositionX: region.positionX, | |
| const webcamPositions: TimelineRenderItem[] = webcamPositionRegions.map((region) => ({ | |
| id: region.id, | |
| rowId: WEBCAM_POSITION_ROW_ID, | |
| span: { start: region.startMs, end: region.endMs }, | |
| label: `${Math.round(region.positionX * 100)}%,${Math.round(region.positionY * 100)}%`, | |
| webcamPositionX: region.positionX, |
🤖 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/components/video-editor/timeline/model/timelineModel.ts` around lines 133
- 138, The timeline labels for webcam positions are emitted as raw numbers and
need percent units; update the label construction in the webcamPositions mapping
(use webcamPositionRegions → webcamPositions) to append "%" to both rounded X
and Y values so the label becomes e.g. "12%,34%"; ensure the change is made
where label: `${Math.round(region.positionX *
100)},${Math.round(region.positionY * 100)}` is created in timelineModel.ts so
other code consuming TimelineRenderItem remains unchanged.
Comment on lines
+1329
to
+1331
| webcamSizeRegions, | ||
| webcamFocusRegions, | ||
| webcamPositionRegions, |
Contributor
There was a problem hiding this comment.
Honor webcamPositionEnabled outside the live preview too.
Preview/timeline correctly gate position regions behind webcamPositionEnabled, but thumbnail capture and both export paths still pass raw webcamPositionRegions. Turning the feature off can therefore hide the motion in-editor while it still shows up in saved thumbnails and exported output.
Also applies to: 4978-4980, 5153-5155
🤖 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/components/video-editor/VideoEditor.tsx` around lines 1329 - 1331, The
thumbnail capture and export code paths are still using raw
webcamPositionRegions even when the UI feature is disabled; update the thumbnail
capture and both export routines to respect webcamPositionEnabled by only
passing webcamPositionRegions when webcamPositionEnabled is true (otherwise pass
null/undefined or omit them), mirroring the preview/timeline gating;
specifically modify the call sites that currently forward webcamSizeRegions,
webcamFocusRegions, webcamPositionRegions so they conditionally include
webcamPositionRegions based on the webcamPositionEnabled flag (look for
functions handling thumbnail capture and the two export functions that forward
these region props).
Comment on lines
+3025
to
+3089
| const handleMove = (moveEvent: PointerEvent) => { | ||
| const drag = webcamPositionDragRef.current; | ||
| if (!drag || moveEvent.pointerId !== drag.pointerId) return; | ||
| const overlayElement = overlayRef.current; | ||
| if (!overlayElement) return; | ||
|
|
||
| const deltaX = moveEvent.clientX - drag.startClientX; | ||
| const deltaY = moveEvent.clientY - drag.startClientY; | ||
| if (!drag.active && Math.abs(deltaX) < 4 && Math.abs(deltaY) < 4) { | ||
| return; | ||
| } | ||
|
|
||
| const availableWidth = Math.max( | ||
| 1, | ||
| overlayElement.clientWidth - drag.startWidthPx - webcamMargin * 2, | ||
| ); | ||
| const availableHeight = Math.max( | ||
| 1, | ||
| overlayElement.clientHeight - drag.startHeightPx - webcamMargin * 2, | ||
| ); | ||
| const rawX = Math.max( | ||
| 0, | ||
| Math.min(1, (drag.startLeftPx + deltaX - webcamMargin) / availableWidth), | ||
| ); | ||
| const rawY = Math.max( | ||
| 0, | ||
| Math.min(1, (drag.startTopPx + deltaY - webcamMargin) / availableHeight), | ||
| ); | ||
| const snappedPosition = getSnappedWebcamPositionPoint({ x: rawX, y: rawY }); | ||
| const nextX = snappedPosition.x; | ||
| const nextY = snappedPosition.y; | ||
|
|
||
| if (!drag.active) { | ||
| drag.active = true; | ||
| if (onWebcamPositionDragStart) { | ||
| drag.regionId = onWebcamPositionDragStart(nextX, nextY) ?? null; | ||
| } | ||
| } | ||
|
|
||
| if (drag.regionId && onWebcamPositionDrag) { | ||
| onWebcamPositionDrag(drag.regionId, nextX, nextY); | ||
| } | ||
| }; | ||
|
|
||
| const handleUp = (upEvent: PointerEvent) => { | ||
| const drag = webcamPositionDragRef.current; | ||
| if (!drag || upEvent.pointerId !== drag.pointerId) return; | ||
| window.removeEventListener("pointermove", handleMove); | ||
| window.removeEventListener("pointerup", handleUp); | ||
| window.removeEventListener("pointercancel", handleUp); | ||
| if (webcamBubbleRef.current) { | ||
| webcamBubbleRef.current.style.cursor = | ||
| onWebcamPositionDragStart || onWebcamPositionDrag ? "grab" : ""; | ||
| } | ||
| if (!drag.active && onSelectWebcamPositionRegion) { | ||
| onSelectWebcamPositionRegion( | ||
| drag.regionId ?? selectedWebcamPositionRegionId ?? null, | ||
| ); | ||
| } | ||
| webcamPositionDragRef.current = null; | ||
| }; | ||
|
|
||
| window.addEventListener("pointermove", handleMove); | ||
| window.addEventListener("pointerup", handleUp); | ||
| window.addEventListener("pointercancel", handleUp); |
Contributor
There was a problem hiding this comment.
Clean up the window-level pointer listeners on teardown.
These handlers only unregister themselves from pointerup/pointercancel. If the component unmounts or the webcam overlay disappears mid-drag, the window listeners keep firing against stale refs and callbacks. Add a cleanup path that removes any active drag/resize listeners on unmount as well.
Also applies to: 3192-3331
🤖 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/components/video-editor/VideoPlayback.tsx` around lines 3025 - 3089,
Window-level pointer listeners (handleMove/handleUp) are added but not removed
on component unmount which can cause stray callbacks; fix by adding a cleanup
that removes window.removeEventListener for "pointermove", "pointerup", and
"pointercancel" and clears webcamPositionDragRef on teardown (same pattern for
the other drag block at 3192-3331). In practice, wrap the addEventListener calls
in the effect where they are registered and return a cleanup function that calls
window.removeEventListener(handleMove), window.removeEventListener(handleUp)
(for both "pointerup" and "pointercancel"), resets webcamPositionDragRef.current
= null, and restores webcamBubbleRef.current.style.cursor if needed so stale
refs/callbacks (overlayRef, webcamBubbleRef, onWebcamPositionDragStart,
onWebcamPositionDrag, onSelectWebcamPositionRegion) are not invoked after
unmount.
Comment on lines
+3135
to
+3190
| const positionRegionId = onWebcamPositionDragStart | ||
| ? (onWebcamPositionDragStart(startPositionX, startPositionY) ?? null) | ||
| : null; | ||
| webcamSizeResizeRef.current = { | ||
| handle, | ||
| startClientX: event.clientX, | ||
| startClientY: event.clientY, | ||
| startSizePx: rect.width, | ||
| startWidthPx, | ||
| startHeightPx, | ||
| startLeftPx, | ||
| startTopPx, | ||
| startBottomPx: startTopPx + startHeightPx, | ||
| positionRegionId, | ||
| currentPercent: isVerticalStretch ? webcamHeight : webcamSize, | ||
| currentWidthPercent: webcamSize, | ||
| currentHeightPercent: webcamHeight, | ||
| currentWidthPx: startWidthPx, | ||
| currentHeightPx: startHeightPx, | ||
| currentLeftPx: startLeftPx, | ||
| currentTopPx: startTopPx, | ||
| }; | ||
| onWebcamSizeResizeStart?.(); | ||
|
|
||
| const updateBadge = (clientX: number, clientY: number, label: string) => { | ||
| const badge = webcamResizeBadgeRef.current; | ||
| if (!badge) return; | ||
| badge.style.display = "block"; | ||
| badge.style.left = `${clientX + 12}px`; | ||
| badge.style.top = `${clientY + 12}px`; | ||
| badge.textContent = label; | ||
| }; | ||
|
|
||
| const persistResizePosition = ( | ||
| drag: NonNullable<typeof webcamSizeResizeRef.current>, | ||
| activeOverlay: HTMLDivElement, | ||
| widthPx: number, | ||
| heightPx: number, | ||
| leftPx: number, | ||
| topPx: number, | ||
| ) => { | ||
| if (!drag.positionRegionId || !onWebcamPositionDrag) { | ||
| return; | ||
| } | ||
| const availableWidth = Math.max( | ||
| 1, | ||
| activeOverlay.clientWidth - widthPx - webcamMargin * 2, | ||
| ); | ||
| const availableHeight = Math.max( | ||
| 1, | ||
| activeOverlay.clientHeight - heightPx - webcamMargin * 2, | ||
| ); | ||
| const nextX = Math.max(0, Math.min(1, (leftPx - webcamMargin) / availableWidth)); | ||
| const nextY = Math.max(0, Math.min(1, (topPx - webcamMargin) / availableHeight)); | ||
| onWebcamPositionDrag(drag.positionRegionId, nextX, nextY); | ||
| }; |
Contributor
There was a problem hiding this comment.
Top/left resize loses its position change when position regions are unavailable.
Line 3135 makes positional persistence optional, and Line 3176 skips it entirely when the position-region callbacks are missing. But the top/left resize paths still update currentLeftPx/currentTopPx, so the bubble moves during the drag and then snaps back on mouseup because only size/height were saved. Persist the fallback global position here, or disable the handles that require a position write when those callbacks are absent.
Also applies to: 3223-3314
🤖 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/components/video-editor/VideoPlayback.tsx` around lines 3135 - 3190, The
top/left resize handlers update currentLeftPx/currentTopPx but
persistResizePosition bails out when drag.positionRegionId or
onWebcamPositionDrag are missing, causing the UI to snap back; either persist a
fallback global position there or prevent those handles when position callbacks
are absent. Update persistResizePosition (referenced via
webcamSizeResizeRef.current and function persistResizePosition) so that if
!drag.positionRegionId || !onWebcamPositionDrag you compute nextX/nextY from
leftPx/topPx the same way and call your global position setter (e.g.,
props.onWebcamPositionChange or a local setWebcamPosition state updater), or
alternatively disable the top/left handles where webcamSizeResizeRef is
initialized when onWebcamPositionDrag/onWebcamPositionDragStart are undefined so
those handles never update currentLeftPx/currentTopPx.
Comment on lines
+306
to
+314
| const canRevealVertically = | ||
| safeTargetHeight > safeTargetWidth + 0.5 && sh * revealScale >= safeTargetHeight; | ||
| const scale = canRevealVertically ? revealScale : coverScale; | ||
| const drawWidth = sw * scale; | ||
| const drawHeight = sh * scale; | ||
| const drawX = (safeTargetWidth - drawWidth) / 2; | ||
| const drawY = canRevealVertically | ||
| ? (baseSize - drawHeight) / 2 + (safeTargetHeight - baseSize) | ||
| : (safeTargetHeight - drawHeight) / 2; |
Contributor
There was a problem hiding this comment.
Prevent positive drawY in vertical-reveal mode.
canRevealVertically can be true while reveal drawY is positive, which can create a visible blank band at the top of the webcam target area for tall stretched frames. Gate reveal mode on non-positive reveal offset (or clamp it) and otherwise fall back to cover.
💡 Proposed fix
const coverScale = Math.max(safeTargetWidth / sw, safeTargetHeight / sh);
const baseSize = Math.min(safeTargetWidth, safeTargetHeight);
const revealScale = Math.max(safeTargetWidth / sw, baseSize / sh);
+ const revealDrawY = (baseSize - sh * revealScale) / 2 + (safeTargetHeight - baseSize);
const canRevealVertically =
- safeTargetHeight > safeTargetWidth + 0.5 && sh * revealScale >= safeTargetHeight;
+ safeTargetHeight > safeTargetWidth + 0.5 &&
+ sh * revealScale >= safeTargetHeight &&
+ revealDrawY <= 0;
const scale = canRevealVertically ? revealScale : coverScale;
const drawWidth = sw * scale;
const drawHeight = sh * scale;
const drawX = (safeTargetWidth - drawWidth) / 2;
const drawY = canRevealVertically
- ? (baseSize - drawHeight) / 2 + (safeTargetHeight - baseSize)
+ ? revealDrawY
: (safeTargetHeight - drawHeight) / 2;📝 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 canRevealVertically = | |
| safeTargetHeight > safeTargetWidth + 0.5 && sh * revealScale >= safeTargetHeight; | |
| const scale = canRevealVertically ? revealScale : coverScale; | |
| const drawWidth = sw * scale; | |
| const drawHeight = sh * scale; | |
| const drawX = (safeTargetWidth - drawWidth) / 2; | |
| const drawY = canRevealVertically | |
| ? (baseSize - drawHeight) / 2 + (safeTargetHeight - baseSize) | |
| : (safeTargetHeight - drawHeight) / 2; | |
| const revealDrawY = (baseSize - sh * revealScale) / 2 + (safeTargetHeight - baseSize); | |
| const canRevealVertically = | |
| safeTargetHeight > safeTargetWidth + 0.5 && | |
| sh * revealScale >= safeTargetHeight && | |
| revealDrawY <= 0; | |
| const scale = canRevealVertically ? revealScale : coverScale; | |
| const drawWidth = sw * scale; | |
| const drawHeight = sh * scale; | |
| const drawX = (safeTargetWidth - drawWidth) / 2; | |
| const drawY = canRevealVertically | |
| ? revealDrawY | |
| : (safeTargetHeight - drawHeight) / 2; |
🤖 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/components/video-editor/webcamOverlay.ts` around lines 306 - 314, The
vertical-reveal path can produce a positive drawY leaving a blank band; update
the logic around canRevealVertically/drawY so that reveal mode is only used when
the computed reveal offset is non-positive (or clamp drawY to <= 0) and
otherwise fall back to coverScale. Concretely, after computing scale
(revealScale vs coverScale) and drawHeight, compute the tentative revealOffset =
(baseSize - drawHeight) / 2 + (safeTargetHeight - baseSize) and if revealOffset
> 0 then treat canRevealVertically as false (use coverScale) or set drawY =
Math.min(revealOffset, 0); adjust the drawY assignment accordingly to ensure no
positive drawY in canRevealVertically branch (references: canRevealVertically,
revealScale, coverScale, drawHeight, drawY, safeTargetHeight, safeTargetWidth,
baseSize).
Comment on lines
+2503
to
+2513
| zoomScale: focusState ? 1 : this.animationState.appliedScale || 1, | ||
| reactToZoom: focusState ? false : (webcam.reactToZoom ?? true), | ||
| }); | ||
| const bubbleHeight = getWebcamOverlaySizePx({ | ||
| containerWidth: width, | ||
| containerHeight: height, | ||
| sizePercent: effectiveHeightPercent, | ||
| margin, | ||
| zoomScale: focusState ? 1 : this.animationState.appliedScale || 1, | ||
| reactToZoom: focusState ? false : (webcam.reactToZoom ?? true), | ||
| }); |
Contributor
There was a problem hiding this comment.
Focus-state truthiness is disabling normal reactToZoom behavior.
Line 2503 and Line 2504 gate on focusState existence, not active focus progress. That forces zoomScale=1 and reactToZoom=false whenever a focus state object exists, including inactive focus periods.
Suggested fix
+ const isFocusActive = (focusState?.progress ?? 0) > 0.001;
const bubbleWidth = getWebcamOverlaySizePx({
containerWidth: width,
containerHeight: height,
sizePercent: effectiveWidthPercent,
margin,
- zoomScale: focusState ? 1 : this.animationState.appliedScale || 1,
- reactToZoom: focusState ? false : (webcam.reactToZoom ?? true),
+ zoomScale: isFocusActive ? 1 : this.animationState.appliedScale || 1,
+ reactToZoom: isFocusActive ? false : (webcam.reactToZoom ?? true),
});
const bubbleHeight = getWebcamOverlaySizePx({
containerWidth: width,
containerHeight: height,
sizePercent: effectiveHeightPercent,
margin,
- zoomScale: focusState ? 1 : this.animationState.appliedScale || 1,
- reactToZoom: focusState ? false : (webcam.reactToZoom ?? true),
+ zoomScale: isFocusActive ? 1 : this.animationState.appliedScale || 1,
+ reactToZoom: isFocusActive ? false : (webcam.reactToZoom ?? true),
});📝 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
| zoomScale: focusState ? 1 : this.animationState.appliedScale || 1, | |
| reactToZoom: focusState ? false : (webcam.reactToZoom ?? true), | |
| }); | |
| const bubbleHeight = getWebcamOverlaySizePx({ | |
| containerWidth: width, | |
| containerHeight: height, | |
| sizePercent: effectiveHeightPercent, | |
| margin, | |
| zoomScale: focusState ? 1 : this.animationState.appliedScale || 1, | |
| reactToZoom: focusState ? false : (webcam.reactToZoom ?? true), | |
| }); | |
| const isFocusActive = (focusState?.progress ?? 0) > 0.001; | |
| const bubbleWidth = getWebcamOverlaySizePx({ | |
| containerWidth: width, | |
| containerHeight: height, | |
| sizePercent: effectiveWidthPercent, | |
| margin, | |
| zoomScale: isFocusActive ? 1 : this.animationState.appliedScale || 1, | |
| reactToZoom: isFocusActive ? false : (webcam.reactToZoom ?? true), | |
| }); | |
| const bubbleHeight = getWebcamOverlaySizePx({ | |
| containerWidth: width, | |
| containerHeight: height, | |
| sizePercent: effectiveHeightPercent, | |
| margin, | |
| zoomScale: isFocusActive ? 1 : this.animationState.appliedScale || 1, | |
| reactToZoom: isFocusActive ? false : (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/frameRenderer.ts` around lines 2503 - 2513, The code
incorrectly uses the truthiness of focusState to disable zoom behavior; change
the checks to test actual active focus progress (e.g., focusState?.progress > 0)
in both places where zoomScale and reactToZoom are computed so zoomScale only
becomes 1 and reactToZoom only becomes false when focus is actively in-progress;
update the two expressions that currently use focusState ? ... : ... to use
focusState?.progress > 0 ? ... : ... (leave animationState.appliedScale and
webcam.reactToZoom logic unchanged) and adjust both the call that constructs the
object and the getWebcamOverlaySizePx invocation.
Comment on lines
+2567
to
+2569
| if (webcam.avoidCursor && (!focusState || focusState.progress <= 0.001)) { | ||
| const cursor = this.getCursorPosition(this.currentTimelineTimeMs); | ||
| const avoidedPosition = getWebcamAvoidCursorPosition({ |
Contributor
There was a problem hiding this comment.
Avoid-cursor uses the wrong timebase for cursor telemetry lookup.
Line 2568 calls getCursorPosition(this.currentTimelineTimeMs), but cursor telemetry elsewhere is sampled using cursor timestamps, not background timeline time. This desyncs avoidance when trims/speed mapping are active.
🤖 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/frameRenderer.ts` around lines 2567 - 2569, The avoid-cursor
call uses the background timeline time (this.currentTimelineTimeMs) but cursor
telemetry is sampled in the cursor timebase, causing desync when trims/speed
mapping are active; change the argument to getCursorPosition to use the
cursor-sampled time instead of this.currentTimelineTimeMs — either pass the same
cursor timestamp source used elsewhere (the variable or getter that samples
cursor telemetry) or convert timeline time to cursor time via the existing
timeline→cursor mapping helper (e.g., mapTimelineTimeToCursorTime or similar)
before calling getCursorPosition so avoidance and telemetry use the same
timebase.
Comment on lines
+2993
to
+3003
| zoomScale: focusState ? 1 : this.animationState.appliedScale || 1, | ||
| reactToZoom: focusState ? false : (webcam.reactToZoom ?? true), | ||
| }); | ||
| const webcamHeight = getWebcamOverlaySizePx({ | ||
| containerWidth: this.config.width, | ||
| containerHeight: this.config.height, | ||
| sizePercent: effectiveHeightPercent, | ||
| margin, | ||
| zoomScale: focusState ? 1 : this.animationState.appliedScale || 1, | ||
| reactToZoom: focusState ? false : (webcam.reactToZoom ?? true), | ||
| }); |
Contributor
There was a problem hiding this comment.
reactToZoom is suppressed even when focus is inactive.
Line 2993 and Line 2994 use focusState ? ... instead of checking active focus progress, so normal zoom-reactive webcam sizing is bypassed whenever focus state exists.
Suggested fix
+ const isFocusActive = (focusState?.progress ?? 0) > 0.001;
const size = getWebcamOverlaySizePx({
containerWidth: this.config.width,
containerHeight: this.config.height,
sizePercent: effectiveWidthPercent,
margin,
- zoomScale: focusState ? 1 : this.animationState.appliedScale || 1,
- reactToZoom: focusState ? false : (webcam.reactToZoom ?? true),
+ zoomScale: isFocusActive ? 1 : this.animationState.appliedScale || 1,
+ reactToZoom: isFocusActive ? false : (webcam.reactToZoom ?? true),
});
const webcamHeight = getWebcamOverlaySizePx({
containerWidth: this.config.width,
containerHeight: this.config.height,
sizePercent: effectiveHeightPercent,
margin,
- zoomScale: focusState ? 1 : this.animationState.appliedScale || 1,
- reactToZoom: focusState ? false : (webcam.reactToZoom ?? true),
+ zoomScale: isFocusActive ? 1 : this.animationState.appliedScale || 1,
+ reactToZoom: isFocusActive ? false : (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/modernFrameRenderer.ts` around lines 2993 - 3003, The code
currently suppresses zoom-reactive behavior whenever focusState is truthy;
update both places (the zoomScale and reactToZoom arguments passed to
getWebcamOverlaySizePx and the earlier call) to only treat focus as suppressing
zoom when focus is fully active — e.g., replace the ternaries that use
focusState ? ... with checks against the focus progress/active flag (for example
use focusState?.progress === 1 or focusState?.active) so the expressions become
something like (focusState?.progress === 1 ? 1 :
this.animationState.appliedScale || 1) and (focusState?.progress === 1 ? false :
(webcam.reactToZoom ?? true)), ensuring normal reactToZoom behavior when focus
exists but is not fully active; apply the same change to both occurrences that
reference focusState, animationState.appliedScale, and webcam.reactToZoom.
Comment on lines
+3055
to
+3057
| if (webcam.avoidCursor && (!focusState || focusState.progress <= 0.001)) { | ||
| const cursor = this.getCursorPosition(this.currentTimelineTimeMs); | ||
| const avoidedPosition = getWebcamAvoidCursorPosition({ |
Contributor
There was a problem hiding this comment.
Cursor-avoidance lookup is tied to timeline time instead of cursor time.
Line 3056 uses this.currentTimelineTimeMs for getCursorPosition(...). That diverges from cursor telemetry timing used elsewhere and can misplace avoidance under trim/speed remapping.
🤖 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/modernFrameRenderer.ts` around lines 3055 - 3057, The
avoidance lookup is using timeline time (this.currentTimelineTimeMs) instead of
the cursor telemetry timestamp, causing misplacement under trims/speed changes;
change the call to getCursorPosition(...) to use the cursor's telemetry time
(the same cursor timestamp used elsewhere — e.g., the current cursor time
variable such as this.currentCursorTimeMs or the cursor object's timestamp) so
getCursorPosition uses cursor-time, not timeline-time, when computing
avoidedPosition for webcam.avoidCursor and focusState checks.
Comment on lines
+1524
to
+1539
| if ((this.config.webcamSizeRegions ?? []).length > 0) { | ||
| reasons.push("unsupported-webcam-size-regions"); | ||
| } | ||
| if ((this.config.webcamFocusRegions ?? []).length > 0) { | ||
| reasons.push("unsupported-webcam-focus-regions"); | ||
| } | ||
| if (this.config.webcam?.enabled && this.config.webcam.avoidCursor) { | ||
| reasons.push("unsupported-webcam-avoid-cursor"); | ||
| } | ||
| if ( | ||
| this.config.webcam?.enabled && | ||
| Math.round(this.config.webcam.height ?? this.config.webcam.size ?? 40) !== | ||
| Math.round(this.config.webcam.size ?? 40) | ||
| ) { | ||
| reasons.push("unsupported-webcam-vertical-stretch"); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing skip reason for webcamPositionRegions.
The native static layout path flags webcamSizeRegions and webcamFocusRegions as unsupported, but webcamPositionRegions is not checked. Since getNativeStaticLayoutWebcamOverlay() computes a fixed static position and does not interpolate position regions, exports with position regions would render the webcam at incorrect positions throughout the video.
🐛 Proposed fix to add missing skip reason
if ((this.config.webcamFocusRegions ?? []).length > 0) {
reasons.push("unsupported-webcam-focus-regions");
}
+if ((this.config.webcamPositionRegions ?? []).length > 0) {
+ reasons.push("unsupported-webcam-position-regions");
+}
if (this.config.webcam?.enabled && this.config.webcam.avoidCursor) {
reasons.push("unsupported-webcam-avoid-cursor");
}📝 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
| if ((this.config.webcamSizeRegions ?? []).length > 0) { | |
| reasons.push("unsupported-webcam-size-regions"); | |
| } | |
| if ((this.config.webcamFocusRegions ?? []).length > 0) { | |
| reasons.push("unsupported-webcam-focus-regions"); | |
| } | |
| if (this.config.webcam?.enabled && this.config.webcam.avoidCursor) { | |
| reasons.push("unsupported-webcam-avoid-cursor"); | |
| } | |
| if ( | |
| this.config.webcam?.enabled && | |
| Math.round(this.config.webcam.height ?? this.config.webcam.size ?? 40) !== | |
| Math.round(this.config.webcam.size ?? 40) | |
| ) { | |
| reasons.push("unsupported-webcam-vertical-stretch"); | |
| } | |
| if ((this.config.webcamSizeRegions ?? []).length > 0) { | |
| reasons.push("unsupported-webcam-size-regions"); | |
| } | |
| if ((this.config.webcamFocusRegions ?? []).length > 0) { | |
| reasons.push("unsupported-webcam-focus-regions"); | |
| } | |
| if ((this.config.webcamPositionRegions ?? []).length > 0) { | |
| reasons.push("unsupported-webcam-position-regions"); | |
| } | |
| if (this.config.webcam?.enabled && this.config.webcam.avoidCursor) { | |
| reasons.push("unsupported-webcam-avoid-cursor"); | |
| } | |
| if ( | |
| this.config.webcam?.enabled && | |
| Math.round(this.config.webcam.height ?? this.config.webcam.size ?? 40) !== | |
| Math.round(this.config.webcam.size ?? 40) | |
| ) { | |
| reasons.push("unsupported-webcam-vertical-stretch"); | |
| } |
🤖 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` around lines 1524 - 1539, The code
that accumulates unsupported export reasons (the reasons array in
modernVideoExporter.ts) is missing a check for webcamPositionRegions which
causes position regions to be ignored when using
getNativeStaticLayoutWebcamOverlay; add a check similar to the existing
webcamSizeRegions and webcamFocusRegions checks to push
"unsupported-webcam-position-regions" when (this.config.webcamPositionRegions ??
[]).length > 0 so exports with position interpolation are flagged and routed
away from the native static layout path.
…g inputs normalize* now clips an earlier region to end where the next one begins (dropping it if the clip falls below the min duration), so preview and export always agree on a single active region per instant instead of relying on an arbitrary tie-break. Also clamp easing transition input to [0,1] at the source so no branch can feed an out-of-domain progress value into the cubic-bezier. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
|
1 similar comment
Contributor
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/webcamFocusRegions.ts`:
- Around line 226-238: The overlap-resolution loop for sorted -> resolved can
leave a fake unfocused gap because after resolved.pop() the code immediately
falls through to resolved.push(region) without re-checking the new tail; change
the logic in the for (const region of sorted) loop (variables: resolved,
previous, region, clippedEndMs, WEBCAM_FOCUS_REGION_MIN_DURATION_MS) so that
when you pop the previous entry you re-run the overlap check against the new
previous (e.g., convert the single check into a while loop or loop-back) and
only push region once it no longer overlaps or has been accepted/clipped; keep
the existing clipping behavior (set endMs = clippedEndMs) when appropriate and
only push after all backtracking/resolution is complete.
In `@src/components/video-editor/webcamPositionRegions.ts`:
- Around line 181-193: The overlap-resolution loop in webcamPositionRegions uses
resolved.pop() to remove a too-short previous region but then immediately pushes
the current region without re-checking it against the new last element, which
can leave a hole; change the logic in the loop that iterates over sorted regions
(the block using previous, resolved.pop(), and
WEBCAM_POSITION_REGION_MIN_DURATION_MS) so that after popping you reassign
previous = resolved[resolved.length - 1] and re-evaluate the current region
against the new previous (for example by converting the single if into a while
loop or by using a continue to re-enter the overlap check) and only push region
into resolved once all overlapping adjustments are resolved.
In `@src/components/video-editor/webcamSizeRegions.ts`:
- Around line 192-204: The current normalization loop can leave gaps when a
short overlapping predecessor is popped because `region` is pushed without
re-checking against the new tail; update the logic in the loop that iterates
over `sorted` so that instead of a single `if (previous && region.startMs <
previous.endMs)` check you use a while loop to repeatedly compare `region`
against the current tail (`resolved[resolved.length - 1]`) until no overlap
remains or `region` has been clipped/dropped: inside the loop recompute
`previous`, pop when the clipped duration is below
WEBCAM_SIZE_REGION_MIN_DURATION_MS, otherwise adjust `previous.endMs` to the
clipped end, and only after the while loop push `region` into `resolved`; this
ensures `region` is re-evaluated after any popped predecessors and prevents
leftover gaps.
🪄 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: be945548-8683-4352-be0d-c4c4388f6c0e
📒 Files selected for processing (5)
src/components/video-editor/webcamFocusRegions.tssrc/components/video-editor/webcamPositionRegions.test.tssrc/components/video-editor/webcamPositionRegions.tssrc/components/video-editor/webcamSizeRegions.test.tssrc/components/video-editor/webcamSizeRegions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/video-editor/webcamPositionRegions.test.ts
- src/components/video-editor/webcamSizeRegions.test.ts
Address CodeRabbit review on the overlap normalization: - The single-pass clip left a false gap when a short middle region was dropped (it was never re-checked against the new tail). Replace with a two-phase pass: greedily keep regions (dropping any predecessor left below the minimum once clipped, re-checking the new tail), then clip each kept region to the next one's start. This fully resolves the A/B/C example where B is dropped and A must extend to C. - normalize* now tracks used ids and generates a collision-free numeric suffix for blank ids, so a fallback id can't duplicate a persisted one and break keyed rendering / id-targeted mutations. Adds regression tests for both cases. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
|
2 similar comments
Contributor
|
|
Contributor
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/webcamFocusRegions.ts`:
- Around line 394-405: The gap-branch restarts blending at previousRegion.endMs
causing a visible jump; instead compute the blend window start using the
incoming region's transition window so blending is continuous across the
boundary. Replace the hard restart (using previousRegion.endMs) with a start
time derived as min(previousRegion.endMs, nextRegion.startMs - transitionInMs)
where transitionInMs = getTransitionInMs(nextRegion, resolvedDefaults), then
compute progress = easeFocusTransition((roundedTimeMs - blendWindowStart) /
transitionInMs) and pass that progress into blendFocusStates with
buildFullFocusState({ baseWebcamSize, region: nextRegion, webcamCorner }). Do
the same change symmetrically in the other gap-handling branch that computes
progress for previousRegion transitions so both sides share the same blend
window and produce continuous interpolation.
- Around line 478-495: The x/y offsets are only clamped to >=0, allowing the PIP
to overflow the right/bottom edges when margin is large or scale is small;
update the final returned x and y to also be clamped to the max allowed
positions so x + scaledWidth <= containerWidth and y + scaledHeight <=
containerHeight. Concretely, after computing scaledWidth/scaledHeight and x/y
(using safeMargin and screenCorner as currently done), replace the Math.max(0,
x) and Math.max(0, y) with a clamp that constrains x between 0 and
(containerWidth - scaledWidth) and y between 0 and (containerHeight -
scaledHeight) (use the existing clamp helper), referencing the variables scale,
scaledWidth, scaledHeight, x, y, containerWidth, containerHeight,
margin/safeMargin and screenCorner.
In `@src/components/video-editor/webcamPositionRegions.ts`:
- Around line 325-333: The current logic resets the blend timeline at
previousRegion.endMs causing a snap; instead compute and reuse the same
transition window (transitionInMs and transitionStartMs derived from nextRegion
via getTransitionInMs) for both the active-region branch and the gap branch so
progress is consistently computed as (roundedTimeMs -
transitionStartMs)/transitionInMs (clamped) and passed through
easeWebcamPositionTransition before lerpPoint(regionToPoint(previousRegion),
regionToPoint(nextRegion), progress); update the code in the blocks using
nextRegion, transitionInMs, transitionStartMs, roundedTimeMs,
easeWebcamPositionTransition, lerpPoint and regionToPoint (also apply the same
change to the similar block at the 342-357 region) so the blend window is
continuous instead of restarting at previousRegion.endMs.
In `@src/components/video-editor/webcamSizeRegions.ts`:
- Around line 323-335: The interpolation snaps because the overlap branch resets
progress at previousRegion.endMs; change the blend origin to the earlier of the
previous-region end and the computed transition start so both branches compute
the same progress. In getInterpolatedWebcamSizeAtTime and
getInterpolatedWebcamDimensionsAtTime, compute transitionInMs
(nextRegion.transitionInMs), then set transitionStartMs =
Math.min(previousRegion.endMs, nextRegion.startMs - transitionInMs) (use the
same symbol where currently transitionStartMs is computed), and derive progress
= easeWebcamSizeTransition((roundedTimeMs - transitionStartMs) / transitionInMs)
before calling lerp/clampWebcamSizeRegionSize (and the equivalent
dimension-clamping helpers); apply the same change to the other duplicated
blocks noted (the ranges around 344-363, 435-443, 452-471) so continuity is
preserved.
🪄 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: c8c40e8c-9921-4255-9799-d743a31655bc
📒 Files selected for processing (5)
src/components/video-editor/webcamFocusRegions.tssrc/components/video-editor/webcamPositionRegions.test.tssrc/components/video-editor/webcamPositionRegions.tssrc/components/video-editor/webcamSizeRegions.test.tssrc/components/video-editor/webcamSizeRegions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/video-editor/webcamPositionRegions.test.ts
- src/components/video-editor/webcamSizeRegions.test.ts
Comment on lines
+478
to
+495
| const scale = clamp(screenSizePercent, 0, 100) / 100; | ||
| const scaledWidth = containerWidth * scale; | ||
| const scaledHeight = containerHeight * scale; | ||
| const safeMargin = Math.max(0, margin); | ||
| const x = | ||
| screenCorner === "top-right" || screenCorner === "bottom-right" | ||
| ? containerWidth - scaledWidth - safeMargin | ||
| : safeMargin; | ||
| const y = | ||
| screenCorner === "bottom-left" || screenCorner === "bottom-right" | ||
| ? containerHeight - scaledHeight - safeMargin | ||
| : safeMargin; | ||
|
|
||
| return { | ||
| scale, | ||
| x: Math.max(0, x), | ||
| y: Math.max(0, y), | ||
| }; |
Contributor
There was a problem hiding this comment.
Clamp the PIP offset to the available viewport, not just zero.
For left/top corners, a large margin or very small container can make x + scaledWidth or y + scaledHeight exceed the container bounds. The current Math.max(0, ...) only prevents negative offsets, so the screen can still render partially off-canvas.
Proposed fix
return {
scale,
- x: Math.max(0, x),
- y: Math.max(0, y),
+ x: clamp(x, 0, Math.max(0, containerWidth - scaledWidth)),
+ y: clamp(y, 0, Math.max(0, containerHeight - scaledHeight)),
};📝 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 scale = clamp(screenSizePercent, 0, 100) / 100; | |
| const scaledWidth = containerWidth * scale; | |
| const scaledHeight = containerHeight * scale; | |
| const safeMargin = Math.max(0, margin); | |
| const x = | |
| screenCorner === "top-right" || screenCorner === "bottom-right" | |
| ? containerWidth - scaledWidth - safeMargin | |
| : safeMargin; | |
| const y = | |
| screenCorner === "bottom-left" || screenCorner === "bottom-right" | |
| ? containerHeight - scaledHeight - safeMargin | |
| : safeMargin; | |
| return { | |
| scale, | |
| x: Math.max(0, x), | |
| y: Math.max(0, y), | |
| }; | |
| const scale = clamp(screenSizePercent, 0, 100) / 100; | |
| const scaledWidth = containerWidth * scale; | |
| const scaledHeight = containerHeight * scale; | |
| const safeMargin = Math.max(0, margin); | |
| const x = | |
| screenCorner === "top-right" || screenCorner === "bottom-right" | |
| ? containerWidth - scaledWidth - safeMargin | |
| : safeMargin; | |
| const y = | |
| screenCorner === "bottom-left" || screenCorner === "bottom-right" | |
| ? containerHeight - scaledHeight - safeMargin | |
| : safeMargin; | |
| return { | |
| scale, | |
| x: clamp(x, 0, Math.max(0, containerWidth - scaledWidth)), | |
| y: clamp(y, 0, Math.max(0, containerHeight - scaledHeight)), | |
| }; |
🤖 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/components/video-editor/webcamFocusRegions.ts` around lines 478 - 495,
The x/y offsets are only clamped to >=0, allowing the PIP to overflow the
right/bottom edges when margin is large or scale is small; update the final
returned x and y to also be clamped to the max allowed positions so x +
scaledWidth <= containerWidth and y + scaledHeight <= containerHeight.
Concretely, after computing scaledWidth/scaledHeight and x/y (using safeMargin
and screenCorner as currently done), replace the Math.max(0, x) and Math.max(0,
y) with a clamp that constrains x between 0 and (containerWidth - scaledWidth)
and y between 0 and (containerHeight - scaledHeight) (use the existing clamp
helper), referencing the variables scale, scaledWidth, scaledHeight, x, y,
containerWidth, containerHeight, margin/safeMargin and screenCorner.
The active-region branch blends toward the next region from `nextStart - transitionIn`, but the overlap branch restarted progress from `previousRegion.endMs`. When the next region's transition-in began before the previous region ended, size/position/focus snapped at the boundary (e.g. ~71 -> 40 in 1ms). Unify the blend origin to `min(previousRegion.endMs, nextStart - nextIn)` with `nextStart - blendStart` as the duration, so the overlap branch is C0-continuous with the active branch. When the transition-in did not start early this reduces exactly to the previous gap blend, so no behaviour change in that case. Applies to getInterpolatedWebcamSizeAtTime, getInterpolatedWebcam DimensionsAtTime, the position resolver and the focus resolver. Adds a boundary-continuity regression test. Addresses CodeRabbit review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
|
1 similar comment
Contributor
|
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/video-editor/webcamFocusRegions.ts (1)
502-505:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp the PiP offset to the viewport bounds.
Line 504 and Line 505 only prevent negative offsets. On small containers or large margins,
x + scaledWidthory + scaledHeightcan still exceed the container and render the PiP partially off-canvas.Suggested fix
return { scale, - x: Math.max(0, x), - y: Math.max(0, y), + x: clamp(x, 0, Math.max(0, containerWidth - scaledWidth)), + y: clamp(y, 0, Math.max(0, containerHeight - scaledHeight)), };🤖 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/components/video-editor/webcamFocusRegions.ts` around lines 502 - 505, The returned x/y only clamp negatives and can still place the PiP off-canvas; modify the return to additionally clamp x and y to not exceed the viewport by subtracting the scaled element size (i.e. compute scaledWidth = originalWidth * scale and scaledHeight = originalHeight * scale and then set x = Math.min(Math.max(0, x), containerWidth - scaledWidth) and y = Math.min(Math.max(0, y), containerHeight - scaledHeight)); update the return in the function that returns { scale, x, y } (same scope where scale, x, y are computed) and use the actual container/viewport variable names present in webcamFocusRegions.ts.
🤖 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.
Duplicate comments:
In `@src/components/video-editor/webcamFocusRegions.ts`:
- Around line 502-505: The returned x/y only clamp negatives and can still place
the PiP off-canvas; modify the return to additionally clamp x and y to not
exceed the viewport by subtracting the scaled element size (i.e. compute
scaledWidth = originalWidth * scale and scaledHeight = originalHeight * scale
and then set x = Math.min(Math.max(0, x), containerWidth - scaledWidth) and y =
Math.min(Math.max(0, y), containerHeight - scaledHeight)); update the return in
the function that returns { scale, x, y } (same scope where scale, x, y are
computed) and use the actual container/viewport variable names present in
webcamFocusRegions.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad1b8abe-e7ed-4bae-a840-6a0d425a5a32
📒 Files selected for processing (4)
src/components/video-editor/webcamFocusRegions.tssrc/components/video-editor/webcamPositionRegions.tssrc/components/video-editor/webcamSizeRegions.test.tssrc/components/video-editor/webcamSizeRegions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/video-editor/webcamSizeRegions.test.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.
What this adds
Time-based webcam camera regions on the editor timeline. Today the webcam overlay is static for the whole recording — you pick a size/position once and that's it. This adds three independent, keyframeable region tracks so the webcam can change over time, the same way zoom regions already work:
Each region has eased in/out transitions and the regions blend on overlap. The interpolation is deterministic and shared between the live preview and the export path so what you see is what you render.
Why
This is the feature I personally kept needing while editing screen recordings: the webcam should be small while you talk over code, large when you're explaining to camera, and move out of the way when it covers something. Doing that today means cutting the video externally. Regions make it a first-class timeline edit.
Scope / integration
VideoPlayback.tsx,webcamOverlay.ts,frameRenderer.ts,modernFrameRenderer.ts.modernVideoExporter.tsgates the GPU fast-path off when regions / vertical-stretch / avoid-cursor are in play (cases the native path doesn't implement yet) and falls back to the correct slow path instead of exporting something wrong.Testing
webcamSizeRegions/webcamFocusRegions/webcamPositionRegions(interpolation, transitions, normalization, overlap resolution, id uniqueness).tsc --noEmitclean.modernVideoExporter.fallback.test.tshas 5 failures that reproduce on a pristinev1.3.0-beta.3checkout with none of this branch applied — pre-existing, unrelated.Review status
CodeRabbit's automated pass flagged real things and I worked through them. Notably it caught a genuine edge-case bug in the overlap normalization (a short middle region being dropped could leave a false gap). I went further than its suggested
while-loop fix — that fix didn't fully resolve CodeRabbit's own example — and implemented a two-phase resolution (greedy keep + clip-to-next) that does, with regression tests. Remaining CodeRabbit comments are either pre-existing in the base feature work or stylistic; happy to address anything a maintainer considers blocking.On the Anti-Slop flag
This was built using Claude Code as a tool — the maintainers are aware I work this way. The design, the testing, and the judgement calls (including overriding the bot's suggested fix where it was wrong) are mine and I stand behind them. This is my first open-source contribution; happy to split further, rework, or add whatever context helps. Targets
v1.3.0-beta.3.Summary by CodeRabbit
Release Notes
New Features
Improvements