feat(timeline): continuous baseline strip behind webcam camera rows#533
Open
Reedaaz wants to merge 7 commits into
Open
feat(timeline): continuous baseline strip behind webcam camera rows#533Reedaaz wants to merge 7 commits into
Reedaaz wants to merge 7 commits into
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
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds webcam timeline region types (size, focus, position), normalizers and interpolators, timeline rows and editor UI, preview drag/resize and focus-aware layout, renderer/export forwarding and focus transforms, tests, and a .gitignore entry. ChangesWebcam regions & integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx (1)
717-731:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing dependencies in callback dependency arrays.
Both
handleTimelineClickandhandleTimelineMouseDownreferenceonSelectWebcamSizeandonSelectWebcamPositionbut only includeonSelectWebcamFocusin their dependency arrays. This can cause stale closure issues if those callbacks change.🛠️ Proposed fix
For
handleTimelineClickdeps (around line 725):onClearBlockSelection, onSelectWebcamFocus, +onSelectWebcamSize, +onSelectWebcamPosition, videoDurationMs,For
handleTimelineMouseDowndeps (around line 778):onSelectWebcamFocus, +onSelectWebcamSize, +onSelectWebcamPosition, videoDurationMs,Also applies to: 770-781
🤖 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 717 - 731, handleTimelineClick and handleTimelineMouseDown currently reference onSelectWebcamSize and onSelectWebcamPosition but only list onSelectWebcamFocus in their useCallback dependency arrays; update the dependency arrays for both handleTimelineClick and handleTimelineMouseDown to include onSelectWebcamSize and onSelectWebcamPosition (in addition to onSelectWebcamFocus and the existing deps like isSeeking, onSeek, onSelectZoom, onSelectClip, onSelectAnnotation, onSelectAudio, onClearBlockSelection, videoDurationMs, sidebarWidth, direction, range.start, pixelsToValue) so the callbacks don't close over stale references.src/components/video-editor/VideoEditor.tsx (2)
2278-2283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear all webcam-region selections on project load.
This resets
selectedWebcamSizeRegionIdbut leaves the focus and position selections intact. Since those ids are sequence-based, opening another project can come up with an unrelated focus/position region preselected.💡 Suggested fix
setSelectedZoomId(null); setSelectedClipId(null); setSelectedAnnotationId(null); setSelectedAudioId(null); setSelectedWebcamSizeRegionId(null); + setSelectedWebcamFocusRegionId(null); + setSelectedWebcamPositionRegionId(null);🤖 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, When clearing selections on project load (the block that calls setSelectedZoomId, setSelectedClipId, setSelectedAnnotationId, setSelectedAudioId, setSelectedWebcamSizeRegionId), also reset the webcam focus and position selections by calling their setters (e.g., setSelectedWebcamFocusRegionId(null) and setSelectedWebcamPositionRegionId(null)); add these two calls alongside setSelectedWebcamSizeRegionId so focus/position region IDs are not left selected when a new project loads.
4411-4427:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClip trims/deletes should prune overlapping webcam regions too.
These paths drop zoom/annotation/speed/audio regions from removed timeline spans, but the new webcam size/focus/position regions are left behind. After trimming or deleting a clip, those orphaned webcam states can still affect the remaining timeline and exports.
💡 Suggested fix
setZoomRegions((prev) => removeTrimmedRegions(prev)); setAnnotationRegions((prev) => removeTrimmedRegions(prev)); setSpeedRegions((prev) => removeTrimmedRegions(prev)); setAudioRegions((prev) => removeTrimmedRegions(prev)); + setWebcamSizeRegions((prev) => removeTrimmedRegions(prev)); + setWebcamFocusRegions((prev) => removeTrimmedRegions(prev)); + setWebcamPositionRegions((prev) => removeTrimmedRegions(prev));Mirror the same overlap filter in
handleClipDelete.Also applies to: 4497-4510
🤖 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 4411 - 4427, The trimming/deletion logic currently removes overlapping zoom/annotation/speed/audio regions but leaves webcam regions orphaned; update the same overlap filter to also prune webcam region state(s) by calling the removeTrimmedRegions helper (or replicating its logic) for the webcam-related setters (e.g., setWebcamSizeRegions, setWebcamFocusRegions, setWebcamPositionRegions or whatever webcam region setters exist) in both places where you handle removedSegments (the block around the existing setZoomRegions/... calls and the handleClipDelete block referenced); ensure the same removedSegments/same overlap test (region.startMs < segment.endMs && region.endMs > segment.startMs) is used so webcam regions are removed when they overlap trimmed/deleted clips.
🧹 Nitpick comments (4)
src/components/launch/hooks/useWebcamPreviewOverlay.ts (1)
143-153: ⚡ Quick winApply the final pointer position before RAF teardown.
On Line 143, the pending RAF is canceled and pending pointer data is cleared before the
pointerupcoordinates are applied. That can drop the last movement on quick releases, leaving the preview slightly behind the cursor.Proposed fix (apply final clamped offset once in finish)
const finishWebcamPreviewDrag = useCallback( ( pointerId: number, clientX: number, clientY: number, captureTarget?: HTMLDivElement | null, ) => { const dragState = webcamPreviewDragStartRef.current; if (!dragState || dragState.pointerId !== pointerId) { return; } + + if (dragState.dragging) { + const latestDeltaX = clientX - dragState.startX; + const latestDeltaY = clientY - dragState.startY; + const viewportWidth = Math.max(window.innerWidth, window.screen?.width ?? 0); + const viewportHeight = Math.max(window.innerHeight, window.screen?.height ?? 0); + const unclampedLeft = dragState.initialLeft + latestDeltaX; + const unclampedTop = dragState.initialTop + latestDeltaY; + const clampedLeft = Math.min( + Math.max(0, unclampedLeft), + Math.max(0, viewportWidth - dragState.previewWidth), + ); + const clampedTop = Math.min( + Math.max(0, unclampedTop), + Math.max(0, viewportHeight - dragState.previewHeight), + ); + const nextOffset = { + x: dragState.originX + (clampedLeft - dragState.initialLeft), + y: dragState.originY + (clampedTop - dragState.initialTop), + }; + webcamPreviewOffsetRef.current = nextOffset; + if (recordingWebcamPreviewContainerRef.current) { + recordingWebcamPreviewContainerRef.current.style.transform = + `translate(${nextOffset.x}px, ${nextOffset.y}px)`; + } + } previewDragWindowCleanupRef.current?.(); previewDragWindowCleanupRef.current = null;🤖 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/launch/hooks/useWebcamPreviewOverlay.ts` around lines 143 - 153, The final pointer position is being cleared before it's applied, so update the finish/cleanup logic in useWebcamPreviewOverlay to first consume previewDragPendingPointerRef.current (compute/clamp the final offset using the same logic as in previewDragMove handler) and call setWebcamPreviewOffset with that final value, and only after that cancel previewDragMoveRafRef.current and set previewDragPendingPointerRef.current = null; reference previewDragMoveRafRef, previewDragPendingPointerRef, webcamPreviewDragStartRef, isWebcamPreviewDraggingRef, and setWebcamPreviewOffset to locate and apply the change.src/components/video-editor/VideoPlayback.tsx (2)
3087-3089: ⚖️ Poor tradeoffWindow event listeners may leak if component unmounts during drag.
The drag handler adds window event listeners but doesn't track them in a ref for cleanup on unmount. If the component unmounts while dragging is active, the
pointermove/pointerup/pointercancellisteners remain attached. The PR stack mentions "Window Listener Cleanup and Pointer Capture Guards" inuseWebcamPreviewOverlay.ts- consider using a similar cleanup ref pattern here.♻️ Sketch of cleanup pattern
// Add a ref to track active cleanup const webcamDragCleanupRef = useRef<(() => void) | null>(null); // In handleWebcamBubblePointerDown, store the cleanup: const cleanup = () => { window.removeEventListener("pointermove", handleMove); window.removeEventListener("pointerup", handleUp); window.removeEventListener("pointercancel", handleUp); }; webcamDragCleanupRef.current = cleanup; // Add a useEffect for unmount cleanup: useEffect(() => { return () => { webcamDragCleanupRef.current?.(); }; }, []);🤖 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 3087 - 3089, The window pointer event listeners added (pointermove/pointerup/pointercancel) in the drag logic (see handlers handleMove and handleUp, e.g., where window.addEventListener is called) can leak if the component unmounts during a drag; add a ref (e.g., webcamDragCleanupRef using useRef<(() => void)|null>) to store a cleanup function that removes those listeners when attaching them, assign webcamDragCleanupRef.current = cleanup inside the routine that registers the listeners (the same place as handleWebcamBubblePointerDown or equivalent), and add a useEffect with an empty dependency array that calls webcamDragCleanupRef.current?.() on unmount to ensure listeners are removed. Ensure the cleanup is also invoked when the drag ends (inside handleUp) and clear the ref afterward.
366-379: 💤 Low valueUnused prop:
selectedWebcamFocusRegionIdis defined but never destructured or used.The prop
selectedWebcamFocusRegionIdis declared in the interface but not destructured in the component. If it's intended for future use, consider removing it until needed to avoid confusion.🤖 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 366 - 379, The prop selectedWebcamFocusRegionId is declared in the VideoPlayback props but never used; either remove it from the props interface or destructure it from the component props and use or pass it through where needed (e.g., include selectedWebcamFocusRegionId in the VideoPlayback function/component parameter list and use it to control focus region selection or forward it to child components). Locate the props type/interface that defines selectedWebcamFocusRegionId and the VideoPlayback component (e.g., the props argument to the VideoPlayback function) and either delete the selectedWebcamFocusRegionId declaration or add it to the destructured props and implement the intended behavior or propagation.src/components/video-editor/SettingsPanel.tsx (1)
4235-4294: 💤 Low valueConsider using SliderControl and constants for position region transitions.
Position region transition inputs use raw
<input type="number">with hardcoded 400ms defaults, while size/focus region transitions useSliderControlwith named constants (DEFAULT_WEBCAM_SIZE_TRANSITION_*_MS,DEFAULT_WEBCAM_FOCUS_TRANSITION_*_MS). This creates inconsistency in both UI and maintainability.♻️ Suggested approach
- Add
DEFAULT_WEBCAM_POSITION_TRANSITION_IN_MSandDEFAULT_WEBCAM_POSITION_TRANSITION_OUT_MSconstants totypes.ts- Replace the raw number inputs with
SliderControlcomponents matching the pattern used for size/focus regions🤖 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 4235 - 4294, Replace the two raw number inputs for position region transitions with SliderControl and add constants for defaults: create DEFAULT_WEBCAM_POSITION_TRANSITION_IN_MS and DEFAULT_WEBCAM_POSITION_TRANSITION_OUT_MS in types.ts (following the pattern of DEFAULT_WEBCAM_SIZE_TRANSITION_*_MS and DEFAULT_WEBCAM_FOCUS_TRANSITION_*_MS), then update the inputs in SettingsPanel.tsx to use <SliderControl> with the same min/max/step used by size/focus transitions and use selected.transitionInMs ?? DEFAULT_WEBCAM_POSITION_TRANSITION_IN_MS and selected.transitionOutMs ?? DEFAULT_WEBCAM_POSITION_TRANSITION_OUT_MS, keeping the onChange handler onWebcamPositionRegionTransitionChange(selected.id, "transitionInMs"/"transitionOutMs", value).
🤖 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/components/viewport/TimelineCanvas.tsx`:
- Around line 703-705: handleTimelineClick and handleTimelineMouseDown clear
webcam size and focus but forget to clear webcam position, leaving position
regions selected; update both handlers (handleTimelineClick and
handleTimelineMouseDown) to call onSelectWebcamPosition?.(null) alongside the
existing onSelectWebcamSize?.(null) and onSelectWebcamFocus?.(null) so all
webcam selections are cleared when clicking the timeline background.
In `@src/components/video-editor/timeline/Item.tsx`:
- Around line 146-147: The "webcam-position" variant is falling through to the
default annotation UI (MessageSquare) instead of being rendered with its
glass/position styling; update the JSX branches that currently decide content
(where isWebcamPosition and showAudioWaveform are computed) to explicitly handle
isWebcamPosition first and render the position-specific content (the glass style
UI) instead of the MessageSquare/annotation branch—search for usages of
isWebcamPosition, MessageSquare, and the annotation/content render blocks
(including the other similar places around the showAudioWaveform/annotation
logic) and add an explicit conditional branch for "webcam-position" that returns
the position UI.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1329-1331: Thumbnail/export code paths are still passing raw
webcamPositionRegions even when webcamPositionEnabled is false; update the
thumbnail capture and both exporter config creation sites to pass
EMPTY_WEBCAM_POSITION_REGIONS instead of webcamPositionRegions when
webcamPositionEnabled is false (mirror the preview logic). In practice, locate
the thumbnail capture and exporter config builders that currently pass
webcamSizeRegions, webcamFocusRegions, webcamPositionRegions and conditionally
substitute EMPTY_WEBCAM_POSITION_REGIONS based on webcamPositionEnabled so saved
thumbnails/exports match the editor preview.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1951-1957: The effect watching currentTime is lacking an isPlaying
guard causing redundant applyWebcamBubbleLayout calls during playback; update
the useEffect that depends on currentTime, webcamSizeRegions,
applyWebcamBubbleLayout, pixiReady and videoReady to early-return when isPlaying
is true (i.e. only run when !isPlaying), so that
applyWebcamBubbleLayout(animationStateRef.current.appliedScale || 1) is invoked
only while paused or on seek, leaving the ticker-driven calls intact; reference
the useEffect block, the isPlaying state/prop, applyWebcamBubbleLayout,
animationStateRef, and currentTime to locate and patch the logic.
In `@src/components/video-editor/webcamFocusRegions.ts`:
- Around line 436-453: The computed focus position (x/y) is only lower-bounded
and can exceed the visible upper bounds; update the logic in
webcamFocusRegions.ts (symbols: scale, scaledWidth, scaledHeight, safeMargin, x,
y, containerWidth, containerHeight, screenCorner) to clamp both lower and upper
bounds by computing the maximum allowed offsets (e.g., maxX = containerWidth -
scaledWidth - safeMargin and maxY = containerHeight - scaledHeight - safeMargin)
and set x = clamp(computedX, 0, maxX) and y = clamp(computedY, 0, maxY) so
positions never push content off-canvas.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 2567-2578: The avoid-cursor branch is using currentTimelineTimeMs
to sample cursor position but the rendered cursor uses cursorTimestamp, so on
trimmed/speed-adjusted exports they can diverge; when you update/draw the cursor
(where cursorTimestamp is used) capture and store that timestamp (e.g.
this.currentCursorTimeMsForCursor or similar) and then in the webcam.avoidCursor
branch call getCursorPosition with that stored cursor time instead of
currentTimelineTimeMs; update references to getCursorPosition,
currentTimelineTimeMs, and cursorTimestamp accordingly so the webcam overlay
dodges the actual rendered cursor.
- Around line 2365-2387: getFocusSceneTransform currently computes a
focus-adjusted transform but only drawFrame uses it, leaving extension hooks
(post-video/post-zoom/post-cursor) to render with the raw sceneTransform; fix by
calling getFocusSceneTransform once at the start of the frame render (in
drawFrame or the method that invokes extension hooks), store it as
focusedTransform, and pass focusedTransform into all extension hook invocations
and any composition logic instead of sceneTransform (keep the same fallback
behavior when focusState is absent). Ensure every place that previously used
sceneTransform for extension rendering (post-video/post-zoom/post-cursor hook
calls) now uses the focusedTransform variable so extensions render against the
focus-adjusted coordinates.
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 3055-3068: The avoid-cursor branch uses
getCursorPosition(this.currentTimelineTimeMs) which samples the wrong timeline;
replace the call to getCursorPosition so it uses the cursor timestamp used by
on-screen cursor (cursorTimestamp) instead of this.currentTimelineTimeMs, then
pass that cursor to getWebcamAvoidCursorPosition; if cursorTimestamp can be
undefined/null mirror the existing null check logic. Update references in the
webcam.avoidCursor branch (focusState check, getCursorPosition, and
getWebcamAvoidCursorPosition invocation) to use cursorTimestamp to ensure
avoid-cursor placement follows the same timeline as the on-screen cursor.
- Around line 2890-2917: applyFocusScreenTransform mutates cameraContainer but
compositeExtensions still publishes/uses the original animationState, so
extension hooks render with pre-focus coordinates; fix by computing and
publishing a focused transform inside compositeExtensions (or by updating
animationState before publishing) using the same values from
getWebcamFocusScreenTransform used in applyFocusScreenTransform: combine
animationState.appliedScale with transform.scale and adjust x/y by
transform.x/transform.y plus animationState.x * transform.scale and
animationState.y * transform.scale (and propagate screenOpacity/screenMode where
relevant) so post-video/post-zoom/post-cursor hooks receive and apply the exact
focused transform that cameraContainer is using.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 1524-1539: The native static-layout path ignores dynamic webcam
position timelines; add a guard for this.config.webcamPositionRegions so the
exporter skips nativeStaticLayoutExport when position regions exist by appending
a new reason. Specifically, in the same block that populates reasons (use the
existing reasons array), check (this.config.webcamPositionRegions ?? []).length
> 0 and push "unsupported-webcam-position-regions" so nativeStaticLayoutExport
(and any decision logic that looks at reasons) will avoid the static-only path.
---
Outside diff comments:
In `@src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx`:
- Around line 717-731: handleTimelineClick and handleTimelineMouseDown currently
reference onSelectWebcamSize and onSelectWebcamPosition but only list
onSelectWebcamFocus in their useCallback dependency arrays; update the
dependency arrays for both handleTimelineClick and handleTimelineMouseDown to
include onSelectWebcamSize and onSelectWebcamPosition (in addition to
onSelectWebcamFocus and the existing deps like isSeeking, onSeek, onSelectZoom,
onSelectClip, onSelectAnnotation, onSelectAudio, onClearBlockSelection,
videoDurationMs, sidebarWidth, direction, range.start, pixelsToValue) so the
callbacks don't close over stale references.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2278-2283: When clearing selections on project load (the block
that calls setSelectedZoomId, setSelectedClipId, setSelectedAnnotationId,
setSelectedAudioId, setSelectedWebcamSizeRegionId), also reset the webcam focus
and position selections by calling their setters (e.g.,
setSelectedWebcamFocusRegionId(null) and
setSelectedWebcamPositionRegionId(null)); add these two calls alongside
setSelectedWebcamSizeRegionId so focus/position region IDs are not left selected
when a new project loads.
- Around line 4411-4427: The trimming/deletion logic currently removes
overlapping zoom/annotation/speed/audio regions but leaves webcam regions
orphaned; update the same overlap filter to also prune webcam region state(s) by
calling the removeTrimmedRegions helper (or replicating its logic) for the
webcam-related setters (e.g., setWebcamSizeRegions, setWebcamFocusRegions,
setWebcamPositionRegions or whatever webcam region setters exist) in both places
where you handle removedSegments (the block around the existing
setZoomRegions/... calls and the handleClipDelete block referenced); ensure the
same removedSegments/same overlap test (region.startMs < segment.endMs &&
region.endMs > segment.startMs) is used so webcam regions are removed when they
overlap trimmed/deleted clips.
---
Nitpick comments:
In `@src/components/launch/hooks/useWebcamPreviewOverlay.ts`:
- Around line 143-153: The final pointer position is being cleared before it's
applied, so update the finish/cleanup logic in useWebcamPreviewOverlay to first
consume previewDragPendingPointerRef.current (compute/clamp the final offset
using the same logic as in previewDragMove handler) and call
setWebcamPreviewOffset with that final value, and only after that cancel
previewDragMoveRafRef.current and set previewDragPendingPointerRef.current =
null; reference previewDragMoveRafRef, previewDragPendingPointerRef,
webcamPreviewDragStartRef, isWebcamPreviewDraggingRef, and
setWebcamPreviewOffset to locate and apply the change.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 4235-4294: Replace the two raw number inputs for position region
transitions with SliderControl and add constants for defaults: create
DEFAULT_WEBCAM_POSITION_TRANSITION_IN_MS and
DEFAULT_WEBCAM_POSITION_TRANSITION_OUT_MS in types.ts (following the pattern of
DEFAULT_WEBCAM_SIZE_TRANSITION_*_MS and DEFAULT_WEBCAM_FOCUS_TRANSITION_*_MS),
then update the inputs in SettingsPanel.tsx to use <SliderControl> with the same
min/max/step used by size/focus transitions and use selected.transitionInMs ??
DEFAULT_WEBCAM_POSITION_TRANSITION_IN_MS and selected.transitionOutMs ??
DEFAULT_WEBCAM_POSITION_TRANSITION_OUT_MS, keeping the onChange handler
onWebcamPositionRegionTransitionChange(selected.id,
"transitionInMs"/"transitionOutMs", value).
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 3087-3089: The window pointer event listeners added
(pointermove/pointerup/pointercancel) in the drag logic (see handlers handleMove
and handleUp, e.g., where window.addEventListener is called) can leak if the
component unmounts during a drag; add a ref (e.g., webcamDragCleanupRef using
useRef<(() => void)|null>) to store a cleanup function that removes those
listeners when attaching them, assign webcamDragCleanupRef.current = cleanup
inside the routine that registers the listeners (the same place as
handleWebcamBubblePointerDown or equivalent), and add a useEffect with an empty
dependency array that calls webcamDragCleanupRef.current?.() on unmount to
ensure listeners are removed. Ensure the cleanup is also invoked when the drag
ends (inside handleUp) and clear the ref afterward.
- Around line 366-379: The prop selectedWebcamFocusRegionId is declared in the
VideoPlayback props but never used; either remove it from the props interface or
destructure it from the component props and use or pass it through where needed
(e.g., include selectedWebcamFocusRegionId in the VideoPlayback
function/component parameter list and use it to control focus region selection
or forward it to child components). Locate the props type/interface that defines
selectedWebcamFocusRegionId and the VideoPlayback component (e.g., the props
argument to the VideoPlayback function) and either delete the
selectedWebcamFocusRegionId declaration or add it to the destructured props and
implement the intended behavior or propagation.
🪄 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: d470e81a-ee4f-4e98-8e36-bf7439db1e70
⛔ 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
+703
to
705
| onSelectWebcamSize?.(null); | ||
| onSelectWebcamFocus?.(null); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing onSelectWebcamPosition?.(null) when clearing selections.
Both handleTimelineClick and handleTimelineMouseDown clear webcam size and focus selections but omit the webcam position selection. Clicking the timeline background will leave webcam position regions selected while clearing all other selections.
🐛 Proposed fix
In handleTimelineClick (around line 705):
onSelectWebcamSize?.(null);
onSelectWebcamFocus?.(null);
+onSelectWebcamPosition?.(null);In handleTimelineMouseDown (around line 762):
onSelectWebcamSize?.(null);
onSelectWebcamFocus?.(null);
+onSelectWebcamPosition?.(null);Also applies to: 761-762
🤖 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 703 - 705, handleTimelineClick and handleTimelineMouseDown clear
webcam size and focus but forget to clear webcam position, leaving position
regions selected; update both handlers (handleTimelineClick and
handleTimelineMouseDown) to call onSelectWebcamPosition?.(null) alongside the
existing onSelectWebcamSize?.(null) and onSelectWebcamFocus?.(null) so all
webcam selections are cleared when clicking the timeline background.
Comment on lines
+146
to
147
| const isWebcamPosition = variant === "webcam-position"; | ||
| const showAudioWaveform = isAudio && Boolean(waveformPeaks); |
Contributor
There was a problem hiding this comment.
Handle webcam-position content explicitly instead of falling back to annotation UI.
webcam-position currently gets its own glass style, but content falls through to the default annotation branch (MessageSquare), which makes position regions look like annotations.
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}Also applies to: 170-172, 292-301
🤖 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 146 - 147, The
"webcam-position" variant is falling through to the default annotation UI
(MessageSquare) instead of being rendered with its glass/position styling;
update the JSX branches that currently decide content (where isWebcamPosition
and showAudioWaveform are computed) to explicitly handle isWebcamPosition first
and render the position-specific content (the glass style UI) instead of the
MessageSquare/annotation branch—search for usages of isWebcamPosition,
MessageSquare, and the annotation/content render blocks (including the other
similar places around the showAudioWaveform/annotation logic) and add an
explicit conditional branch for "webcam-position" that returns the position UI.
Comment on lines
+861
to
+891
| const handleTimelineResizePointerDown = useCallback( | ||
| (event: React.PointerEvent<HTMLDivElement>) => { | ||
| event.preventDefault(); | ||
| const panel = event.currentTarget.parentElement; | ||
| const startHeight = panel?.getBoundingClientRect().height ?? 240; | ||
| timelineResizeRef.current = { startY: event.clientY, startHeight }; | ||
|
|
||
| const handleMove = (moveEvent: PointerEvent) => { | ||
| const drag = timelineResizeRef.current; | ||
| if (!drag) return; | ||
| // Dragging the handle upward grows the timeline. | ||
| const delta = drag.startY - moveEvent.clientY; | ||
| const viewportH = window.innerHeight || 900; | ||
| const next = Math.max( | ||
| 160, | ||
| Math.min(viewportH * 0.85, drag.startHeight + delta), | ||
| ); | ||
| setTimelineHeightPx(next); | ||
| }; | ||
| const handleUp = () => { | ||
| timelineResizeRef.current = null; | ||
| window.removeEventListener("pointermove", handleMove); | ||
| window.removeEventListener("pointerup", handleUp); | ||
| window.removeEventListener("pointercancel", handleUp); | ||
| }; | ||
| window.addEventListener("pointermove", handleMove); | ||
| window.addEventListener("pointerup", handleUp); | ||
| window.addEventListener("pointercancel", handleUp); | ||
| }, | ||
| [], | ||
| ); |
Contributor
There was a problem hiding this comment.
Remove timeline-resize listeners if the component unmounts mid-drag.
pointermove/pointerup/pointercancel are only detached from handleUp. If the editor unmounts during an active resize, those window listeners survive and can keep calling setTimelineHeightPx after teardown.
💡 Suggested fix
+ const timelineResizeCleanupRef = useRef<(() => void) | null>(null);
+
const handleTimelineResizePointerDown = useCallback(
(event: React.PointerEvent<HTMLDivElement>) => {
// ...
const handleUp = () => {
timelineResizeRef.current = null;
window.removeEventListener("pointermove", handleMove);
window.removeEventListener("pointerup", handleUp);
window.removeEventListener("pointercancel", handleUp);
+ timelineResizeCleanupRef.current = null;
};
+ timelineResizeCleanupRef.current = handleUp;
window.addEventListener("pointermove", handleMove);
window.addEventListener("pointerup", handleUp);
window.addEventListener("pointercancel", handleUp);
},
[],
); useEffect(() => {
return () => {
+ timelineResizeCleanupRef.current?.();
exporterRef.current?.cancel();
// ...
};
}, []);Also applies to: 1589-1611
Comment on lines
+1329
to
+1331
| webcamSizeRegions, | ||
| webcamFocusRegions, | ||
| webcamPositionRegions, |
Contributor
There was a problem hiding this comment.
Honor webcamPositionEnabled in thumbnail/export rendering.
The preview already swaps in EMPTY_WEBCAM_POSITION_REGIONS when this gate is off, but thumbnail capture and both exporters still receive the raw webcamPositionRegions. That makes saved thumbnails and exports disagree with what the editor is showing.
💡 Suggested fix
+ const effectiveWebcamPositionRegions = webcamPositionEnabled
+ ? webcamPositionRegions
+ : EMPTY_WEBCAM_POSITION_REGIONS;
+
const captureProjectThumbnail = useCallback(async () => {
// ...
frameRenderer = new FrameRenderer({
// ...
- webcamPositionRegions,
+ webcamPositionRegions: effectiveWebcamPositionRegions,
});- webcamPositionRegions,
+ webcamPositionRegions: effectiveWebcamPositionRegions,Apply the same replacement in both exporter configs too.
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,
Thumbnail/export code paths are still passing raw webcamPositionRegions even
when webcamPositionEnabled is false; update the thumbnail capture and both
exporter config creation sites to pass EMPTY_WEBCAM_POSITION_REGIONS instead of
webcamPositionRegions when webcamPositionEnabled is false (mirror the preview
logic). In practice, locate the thumbnail capture and exporter config builders
that currently pass webcamSizeRegions, webcamFocusRegions, webcamPositionRegions
and conditionally substitute EMPTY_WEBCAM_POSITION_REGIONS based on
webcamPositionEnabled so saved thumbnails/exports match the editor preview.
Comment on lines
+1951
to
+1957
| // Re-apply layout when the playhead moves while paused so that webcam | ||
| // size regions take effect on seek without waiting for the next zoom tick. | ||
| useEffect(() => { | ||
| if (!pixiReady || !videoReady) return; | ||
| if (!webcamSizeRegions || webcamSizeRegions.length === 0) return; | ||
| applyWebcamBubbleLayout(animationStateRef.current.appliedScale || 1); | ||
| }, [currentTime, webcamSizeRegions, applyWebcamBubbleLayout, pixiReady, videoReady]); |
Contributor
There was a problem hiding this comment.
Missing isPlaying guard causes redundant layout calls during playback.
This effect runs on every currentTime change, but the ticker already calls applyWebcamBubbleLayout during playback. The comment says "when the playhead moves while paused" but there's no check for !isPlaying. This causes redundant layout calculations during playback.
🛠️ Proposed fix
// Re-apply layout when the playhead moves while paused so that webcam
// size regions take effect on seek without waiting for the next zoom tick.
useEffect(() => {
if (!pixiReady || !videoReady) return;
+ if (isPlaying) return;
if (!webcamSizeRegions || webcamSizeRegions.length === 0) return;
applyWebcamBubbleLayout(animationStateRef.current.appliedScale || 1);
- }, [currentTime, webcamSizeRegions, applyWebcamBubbleLayout, pixiReady, videoReady]);
+ }, [currentTime, webcamSizeRegions, applyWebcamBubbleLayout, pixiReady, videoReady, isPlaying]);🤖 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 1951 - 1957, The
effect watching currentTime is lacking an isPlaying guard causing redundant
applyWebcamBubbleLayout calls during playback; update the useEffect that depends
on currentTime, webcamSizeRegions, applyWebcamBubbleLayout, pixiReady and
videoReady to early-return when isPlaying is true (i.e. only run when
!isPlaying), so that
applyWebcamBubbleLayout(animationStateRef.current.appliedScale || 1) is invoked
only while paused or on seek, leaving the ticker-driven calls intact; reference
the useEffect block, the isPlaying state/prop, applyWebcamBubbleLayout,
animationStateRef, and currentTime to locate and patch the logic.
Comment on lines
+2365
to
+2387
| private getFocusSceneTransform(sceneTransform: { scale: number; x: number; y: number }): { | ||
| scale: number; | ||
| x: number; | ||
| y: number; | ||
| } { | ||
| const focusState = this.getCurrentWebcamFocusState(); | ||
| if (!focusState || focusState.progress <= 0.001 || focusState.screenMode === "hidden") { | ||
| return sceneTransform; | ||
| } | ||
|
|
||
| const transform = getWebcamFocusScreenTransform({ | ||
| containerWidth: this.config.width, | ||
| containerHeight: this.config.height, | ||
| screenSizePercent: focusState.screenSize, | ||
| screenCorner: focusState.screenCorner, | ||
| margin: this.config.webcam?.margin ?? 24, | ||
| }); | ||
|
|
||
| return { | ||
| scale: sceneTransform.scale * transform.scale, | ||
| x: transform.x + sceneTransform.x * transform.scale, | ||
| y: transform.y + sceneTransform.y * transform.scale, | ||
| }; |
Contributor
There was a problem hiding this comment.
Reuse the focus-adjusted scene transform for extension hooks.
getFocusSceneTransform() is only applied to drawFrame(). The extension paths still compose with the raw scene transform, so post-video / post-zoom / post-cursor hooks will render against the pre-focus coordinates whenever a webcam focus region is 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 2365 - 2387,
getFocusSceneTransform currently computes a focus-adjusted transform but only
drawFrame uses it, leaving extension hooks (post-video/post-zoom/post-cursor) to
render with the raw sceneTransform; fix by calling getFocusSceneTransform once
at the start of the frame render (in drawFrame or the method that invokes
extension hooks), store it as focusedTransform, and pass focusedTransform into
all extension hook invocations and any composition logic instead of
sceneTransform (keep the same fallback behavior when focusState is absent).
Ensure every place that previously used sceneTransform for extension rendering
(post-video/post-zoom/post-cursor hook calls) now uses the focusedTransform
variable so extensions render against the focus-adjusted coordinates.
Comment on lines
+2567
to
+2578
| if (webcam.avoidCursor && (!focusState || focusState.progress <= 0.001)) { | ||
| const cursor = this.getCursorPosition(this.currentTimelineTimeMs); | ||
| const avoidedPosition = getWebcamAvoidCursorPosition({ | ||
| containerWidth: width, | ||
| containerHeight: height, | ||
| size: bubbleWidth, | ||
| height: bubbleHeight, | ||
| margin, | ||
| currentPosition: { x, y }, | ||
| cursor: cursor ? { x: cursor.cx * width, y: cursor.cy * height } : null, | ||
| legacyCorner: webcam.corner, | ||
| }); |
Contributor
There was a problem hiding this comment.
Sample avoid-cursor from the cursor clock, not the timeline clock.
This branch uses currentTimelineTimeMs, but the rendered cursor is driven by cursorTimestamp. On trimmed or speed-adjusted exports those clocks diverge, so the webcam overlay can dodge a cursor position that is not actually on screen. Store the current cursor time alongside currentTimelineTimeMs and use that here.
🤖 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 - 2578, The avoid-cursor
branch is using currentTimelineTimeMs to sample cursor position but the rendered
cursor uses cursorTimestamp, so on trimmed/speed-adjusted exports they can
diverge; when you update/draw the cursor (where cursorTimestamp is used) capture
and store that timestamp (e.g. this.currentCursorTimeMsForCursor or similar) and
then in the webcam.avoidCursor branch call getCursorPosition with that stored
cursor time instead of currentTimelineTimeMs; update references to
getCursorPosition, currentTimelineTimeMs, and cursorTimestamp accordingly so the
webcam overlay dodges the actual rendered cursor.
Comment on lines
+2890
to
+2917
| private applyFocusScreenTransform(): void { | ||
| if (!this.cameraContainer) { | ||
| return; | ||
| } | ||
|
|
||
| const focusState = this.getCurrentWebcamFocusState(); | ||
| if (!focusState || focusState.progress <= 0.001) { | ||
| this.cameraContainer.alpha = 1; | ||
| return; | ||
| } | ||
|
|
||
| this.cameraContainer.alpha = Math.max(0, Math.min(1, focusState.screenOpacity)); | ||
| if (focusState.screenMode === "hidden") { | ||
| return; | ||
| } | ||
|
|
||
| const transform = getWebcamFocusScreenTransform({ | ||
| containerWidth: this.config.width, | ||
| containerHeight: this.config.height, | ||
| screenSizePercent: focusState.screenSize, | ||
| screenCorner: focusState.screenCorner, | ||
| margin: this.config.webcam?.margin ?? 24, | ||
| }); | ||
| this.cameraContainer.scale.set(this.animationState.appliedScale * transform.scale); | ||
| this.cameraContainer.position.set( | ||
| transform.x + this.animationState.x * transform.scale, | ||
| transform.y + this.animationState.y * transform.scale, | ||
| ); |
Contributor
There was a problem hiding this comment.
Propagate the focus transform into extension compositing.
applyFocusScreenTransform() mutates cameraContainer, but compositeExtensions() still publishes and applies the pre-focus animationState transform. With an active focus region, post-video/post-zoom/post-cursor hooks will render in the wrong place over the already-focused scene.
🤖 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 2890 - 2917,
applyFocusScreenTransform mutates cameraContainer but compositeExtensions still
publishes/uses the original animationState, so extension hooks render with
pre-focus coordinates; fix by computing and publishing a focused transform
inside compositeExtensions (or by updating animationState before publishing)
using the same values from getWebcamFocusScreenTransform used in
applyFocusScreenTransform: combine animationState.appliedScale with
transform.scale and adjust x/y by transform.x/transform.y plus animationState.x
* transform.scale and animationState.y * transform.scale (and propagate
screenOpacity/screenMode where relevant) so post-video/post-zoom/post-cursor
hooks receive and apply the exact focused transform that cameraContainer is
using.
Comment on lines
+3055
to
+3068
| if (webcam.avoidCursor && (!focusState || focusState.progress <= 0.001)) { | ||
| const cursor = this.getCursorPosition(this.currentTimelineTimeMs); | ||
| const avoidedPosition = getWebcamAvoidCursorPosition({ | ||
| containerWidth: this.config.width, | ||
| containerHeight: this.config.height, | ||
| size, | ||
| height: webcamHeight, | ||
| margin, | ||
| currentPosition: position, | ||
| cursor: cursor | ||
| ? { x: cursor.cx * this.config.width, y: cursor.cy * this.config.height } | ||
| : null, | ||
| legacyCorner: webcam.corner, | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the cursor timestamp for avoid-cursor placement.
This path samples telemetry with currentTimelineTimeMs, while the on-screen cursor is updated from cursorTimestamp. Those are different timelines once trim/speed mapping is involved, so the exported webcam position can react to the wrong cursor sample.
🤖 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 - 3068, The
avoid-cursor branch uses getCursorPosition(this.currentTimelineTimeMs) which
samples the wrong timeline; replace the call to getCursorPosition so it uses the
cursor timestamp used by on-screen cursor (cursorTimestamp) instead of
this.currentTimelineTimeMs, then pass that cursor to
getWebcamAvoidCursorPosition; if cursorTimestamp can be undefined/null mirror
the existing null check logic. Update references in the webcam.avoidCursor
branch (focusState check, getCursorPosition, and getWebcamAvoidCursorPosition
invocation) to use cursorTimestamp to ensure avoid-cursor placement follows the
same timeline as the on-screen cursor.
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.
Add missing native static-layout guard for webcam position regions.
nativeStaticLayoutExport only receives static webcam coordinates, so position timelines can be silently dropped unless this path is skipped when webcamPositionRegions are present.
Proposed fix
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");
}🤖 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 native
static-layout path ignores dynamic webcam position timelines; add a guard for
this.config.webcamPositionRegions so the exporter skips nativeStaticLayoutExport
when position regions exist by appending a new reason. Specifically, in the same
block that populates reasons (use the existing reasons array), check
(this.config.webcamPositionRegions ?? []).length > 0 and push
"unsupported-webcam-position-regions" so nativeStaticLayoutExport (and any
decision logic that looks at reasons) will avoid the static-only 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>
Reedaaz
force-pushed
the
pr/timeline-camera-baseline
branch
from
de97747 to
c233a68
Compare
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 198-203: The normalization currently assigns fallback ids like
`webcam-focus-${index+1}` without checking for duplicates, which can create
duplicate ids when a later region has the same explicit id; update the
normalization logic in webcamFocusRegions.ts (the part building the object
pushed in normalized.push that reads candidate.id and startMs) to generate a
unique fallback by consulting and updating the used-ids set (e.g., usedIds or
similar) — loop/increment the numeric suffix until you find an id not in
usedIds, assign that unique id to the region, then add it to usedIds so
subsequent regions won’t reuse it.
In `@src/components/video-editor/webcamPositionRegions.ts`:
- Around line 152-156: The fallback id generation for webcam regions currently
sets id = candidate.id || `webcam-position-${index+1}`, which can collide with
existing explicit ids (e.g., "webcam-position-2"); change the normalization so
you first collect all explicit ids into a Set and then assign fallback ids by
incrementing a counter and skipping any name already in that Set, updating the
id assignment in the normalization loop (replace the current candidate.id
fallback logic around the id variable in webcamPositionRegions normalization)
and keep the transition handling via clampWebcamPositionRegionTransitionMs
unchanged.
In `@src/components/video-editor/webcamSizeRegions.ts`:
- Around line 159-163: The fallback id generation for regions
("webcam-size-${index + 1}") can collide with existing persisted ids; update the
normalization to ensure uniqueness by tracking already-used ids (e.g., a Set
usedIds) and, when candidate.id is missing/blank, generate a base fallback id
and then append/increment a numeric suffix until it is not in usedIds before
assigning to the local id variable; mark the id as used by adding it to usedIds.
Keep the clampWebcamSizeRegionTransitionMs(candidate.transitionInMs) call as-is
and only change how id is produced/checked to avoid duplicate ids during
normalization.
🪄 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: 4fe9e787-737c-43a5-8db4-0fad7ad036c4
📒 Files selected for processing (7)
src/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/components/viewport/TimelineCanvas.tsxsrc/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/webcamSizeRegions.test.ts
- src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx
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>
Reedaaz
force-pushed
the
pr/timeline-camera-baseline
branch
from
c233a68 to
fe74d24
Compare
Contributor
|
|
This was referenced May 18, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/components/video-editor/webcamFocusRegions.ts (2)
478-495:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp the focus PiP position to the visible max bounds.
x/yare only lower-bounded. With a small container or a largemargin, the top/left branches can still place the PiP beyondcontainer - scaledSize, which pushes it partially off-canvas.🐛 Proposed fix
const y = screenCorner === "bottom-left" || screenCorner === "bottom-right" ? containerHeight - scaledHeight - safeMargin : safeMargin; + const maxX = Math.max(0, containerWidth - scaledWidth); + const maxY = Math.max(0, containerHeight - scaledHeight); return { scale, - x: Math.max(0, x), - y: Math.max(0, y), + x: clamp(x, 0, maxX), + y: clamp(y, 0, maxY), }; }🤖 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 computed PiP position allows values only lower-bounded, so when margin or scaled size is large it can be placed partially off-canvas; update the logic in the function that computes scale/x/y (variables scale, scaledWidth, scaledHeight, safeMargin, x, y) to also clamp x to the range [0, containerWidth - scaledWidth] and y to [0, containerHeight - scaledHeight] (taking safeMargin into account for the corner branches), then return those clamped values instead of only using Math.max(0, ...).
171-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve explicit focus ids before generating fallbacks.
This still renames persisted ids in one ordering: if an id-less region appears first, it can claim
webcam-focus-Nand force a later explicitid: "webcam-focus-N"region to be rewritten during normalization. That makes region identity unstable for keyed rendering and edit/delete flows.🐛 Proposed fix
const normalized: WebcamFocusRegion[] = []; + const explicitIds = new Set( + input + .map((raw) => + raw && typeof raw === "object" ? (raw as { id?: unknown }).id : undefined, + ) + .filter((id): id is string => typeof id === "string" && id.trim().length > 0), + ); const usedIds = new Set<string>(); @@ let id = typeof candidate.id === "string" && candidate.id.trim().length > 0 ? candidate.id - : `webcam-focus-${index + 1}`; - if (usedIds.has(id)) { - let suffix = index + 1; + : ""; + if (!id || usedIds.has(id)) { + let suffix = 1; let candidateId = `webcam-focus-${suffix}`; - while (usedIds.has(candidateId)) { + while (explicitIds.has(candidateId) || usedIds.has(candidateId)) { suffix += 1; candidateId = `webcam-focus-${suffix}`; } id = candidateId; }🤖 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 171 - 212, The normalization currently assigns fallback ids (via `webcam-focus-${index + 1}`) as it iterates and only then avoids collisions using `usedIds`, which allows later explicit `candidate.id` to be renamed; fix this by first scanning `input` to collect and reserve all explicit, non-empty ids (trimmed) into `usedIds` before the main loop, then in the existing loop (where `candidate.id`, `usedIds`, and the fallback generation logic live) generate fallbacks and resolve conflicts knowing explicit ids are already reserved so persisted ids are never rewritten.src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx (1)
696-705:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear webcam position selection in the fallback clear path.
Background clicks still leave
selectedWebcamPositionRegionIdintact here, so the position row can remain selected after the rest of the timeline selection is cleared.🐛 Proposed fix
onSelectAudio?.(null); onSelectWebcamSize?.(null); onSelectWebcamFocus?.(null); + onSelectWebcamPosition?.(null); } @@ onSelectAudio?.(null); onSelectWebcamSize?.(null); onSelectWebcamFocus?.(null); + onSelectWebcamPosition?.(null); }Also applies to: 754-763
🤖 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 696 - 705, The fallback clear path currently clears zoom/clip/annotation/audio/webcam size/focus but omits clearing the webcam position selection; update the branch that runs when onClearBlockSelection is not provided to also call onSelectWebcamPosition?.(null) so selectedWebcamPositionRegionId is cleared; apply the same change to the other identical block referenced (the block around the other occurrence noted in the comment). Ensure you add the call alongside onSelectWebcamSize and onSelectWebcamFocus and guard it with the optional chaining like the others.src/components/video-editor/webcamPositionRegions.ts (1)
123-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve explicit position ids when filling in missing ids.
This still has the mixed-id collision bug in reverse order: an earlier id-less region can normalize to
webcam-position-Nand force a later explicitid: "webcam-position-N"region to be renamed. That breaks stable identity for existing persisted regions.🐛 Proposed fix
const normalized: WebcamPositionRegion[] = []; + const explicitIds = new Set( + input + .map((raw) => + raw && typeof raw === "object" ? (raw as { id?: unknown }).id : undefined, + ) + .filter((id): id is string => typeof id === "string" && id.trim().length > 0), + ); const usedIds = new Set<string>(); @@ let id = typeof candidate.id === "string" && candidate.id.trim().length > 0 ? candidate.id - : `webcam-position-${index + 1}`; - if (usedIds.has(id)) { - let suffix = index + 1; + : ""; + if (!id || usedIds.has(id)) { + let suffix = 1; let candidateId = `webcam-position-${suffix}`; - while (usedIds.has(candidateId)) { + while (explicitIds.has(candidateId) || usedIds.has(candidateId)) { suffix += 1; candidateId = `webcam-position-${suffix}`; } id = candidateId; }🤖 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/webcamPositionRegions.ts` around lines 123 - 166, The current id-assignment logic lets earlier id-less entries claim a generated `webcam-position-N` before later entries with an explicit `id: "webcam-position-N"` are processed, causing explicit ids to be renamed; fix by first scanning the input to collect all explicit, non-empty string ids into usedIds (check candidate.id) before the main loop so generated ids avoid any explicit ids, then proceed with the existing id-generation/resolution logic to only resolve collisions among generated ids; update references in this file to usedIds, candidate.id and the `webcam-position-` template accordingly.src/components/video-editor/webcamSizeRegions.ts (1)
130-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve explicit size ids before assigning generated ids.
This normalizer still mutates explicit ids when an earlier id-less region claims the same generated name first. For example, a missing id can take
webcam-size-1and force a later persistedid: "webcam-size-1"region to be renamed.🐛 Proposed fix
const normalized: WebcamSizeRegion[] = []; + const explicitIds = new Set( + input + .map((raw) => + raw && typeof raw === "object" ? (raw as { id?: unknown }).id : undefined, + ) + .filter((id): id is string => typeof id === "string" && id.trim().length > 0), + ); const usedIds = new Set<string>(); @@ let id = typeof candidate.id === "string" && candidate.id.trim().length > 0 ? candidate.id - : `webcam-size-${index + 1}`; - if (usedIds.has(id)) { - let suffix = index + 1; + : ""; + if (!id || usedIds.has(id)) { + let suffix = 1; let candidateId = `webcam-size-${suffix}`; - while (usedIds.has(candidateId)) { + while (explicitIds.has(candidateId) || usedIds.has(candidateId)) { suffix += 1; candidateId = `webcam-size-${suffix}`; } id = candidateId; }🤖 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 130 - 173, The normalizer must reserve explicit IDs before generating names: do a preliminary pass over input to collect all non-empty string candidate.id values (trimmed) into usedIds so later id-less regions cannot claim a generated name that belongs to an explicit region; then run the existing loop using usedIds to avoid collisions when creating `webcam-size-${index + 1}` IDs. Ensure you still validate start/end with toIntegerMs, clamp, WEBCAM_SIZE_REGION_MIN_DURATION_MS, and add final ids to usedIds as you already do.
🤖 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/timeline/components/viewport/TimelineCanvas.tsx`:
- Around line 696-705: The fallback clear path currently clears
zoom/clip/annotation/audio/webcam size/focus but omits clearing the webcam
position selection; update the branch that runs when onClearBlockSelection is
not provided to also call onSelectWebcamPosition?.(null) so
selectedWebcamPositionRegionId is cleared; apply the same change to the other
identical block referenced (the block around the other occurrence noted in the
comment). Ensure you add the call alongside onSelectWebcamSize and
onSelectWebcamFocus and guard it with the optional chaining like the others.
In `@src/components/video-editor/webcamFocusRegions.ts`:
- Around line 478-495: The computed PiP position allows values only
lower-bounded, so when margin or scaled size is large it can be placed partially
off-canvas; update the logic in the function that computes scale/x/y (variables
scale, scaledWidth, scaledHeight, safeMargin, x, y) to also clamp x to the range
[0, containerWidth - scaledWidth] and y to [0, containerHeight - scaledHeight]
(taking safeMargin into account for the corner branches), then return those
clamped values instead of only using Math.max(0, ...).
- Around line 171-212: The normalization currently assigns fallback ids (via
`webcam-focus-${index + 1}`) as it iterates and only then avoids collisions
using `usedIds`, which allows later explicit `candidate.id` to be renamed; fix
this by first scanning `input` to collect and reserve all explicit, non-empty
ids (trimmed) into `usedIds` before the main loop, then in the existing loop
(where `candidate.id`, `usedIds`, and the fallback generation logic live)
generate fallbacks and resolve conflicts knowing explicit ids are already
reserved so persisted ids are never rewritten.
In `@src/components/video-editor/webcamPositionRegions.ts`:
- Around line 123-166: The current id-assignment logic lets earlier id-less
entries claim a generated `webcam-position-N` before later entries with an
explicit `id: "webcam-position-N"` are processed, causing explicit ids to be
renamed; fix by first scanning the input to collect all explicit, non-empty
string ids into usedIds (check candidate.id) before the main loop so generated
ids avoid any explicit ids, then proceed with the existing
id-generation/resolution logic to only resolve collisions among generated ids;
update references in this file to usedIds, candidate.id and the
`webcam-position-` template accordingly.
In `@src/components/video-editor/webcamSizeRegions.ts`:
- Around line 130-173: The normalizer must reserve explicit IDs before
generating names: do a preliminary pass over input to collect all non-empty
string candidate.id values (trimmed) into usedIds so later id-less regions
cannot claim a generated name that belongs to an explicit region; then run the
existing loop using usedIds to avoid collisions when creating
`webcam-size-${index + 1}` IDs. Ensure you still validate start/end with
toIntegerMs, clamp, WEBCAM_SIZE_REGION_MIN_DURATION_MS, and add final ids to
usedIds as you already do.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e21981bb-5ffe-4126-8016-61fdd4c0318b
📒 Files selected for processing (7)
src/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/components/viewport/TimelineCanvas.tsxsrc/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/timeline/Row.tsx
- src/components/video-editor/webcamSizeRegions.test.ts
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>
Render a continuous dimmed baseline behind size/focus/position regions so gaps read as the default camera state instead of harsh black. Pure render change keyed off whether the row has regions, so it also fixes existing projects. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the near-invisible foreground/12 tint with an explicit zinc-700/60 background + zinc-600 border so the baseline reads clearly on dark editor backgrounds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reedaaz
force-pushed
the
pr/timeline-camera-baseline
branch
from
fe74d24 to
a6c740e
Compare
Contributor
|
|
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
A continuous dimmed baseline strip rendered behind the webcam size/focus/position rows on the timeline.
Why
Without it, the gaps between camera regions render as harsh black, which reads as "something is wrong / missing" rather than "default camera state here". The baseline makes the empty stretches legible as the resting state, consistent with how the rest of the timeline communicates.
Scope
items.length > 0, so it also fixes existing projects with no migration.bg-zinc-700/60+zinc-600border with a centered dashed divider (iterated on the color so it's visible on the dark editor background without being loud).Dependency
Stacked on #531 — the webcam rows have to exist before there's anything to draw a baseline behind. Merge #531 first; this branch is kept rebased on it, so its own diff is just the two-file render change.
Testing
Manual: with regions present the baseline shows behind/between them; empty rows show no baseline (the existing hint still renders); existing projects render the baseline without migration.
tsc --noEmitclean.Context
Built with Claude Code as a tool (maintainers know I work this way); design and validation are mine.
Summary by CodeRabbit