fix: clip coordinate model with sourceStartMs, remove trim/speed UI#272
fix: clip coordinate model with sourceStartMs, remove trim/speed UI#272webadderall wants to merge 3 commits intomainfrom
Conversation
…s user concepts - Add sourceStartMs to ClipRegion for explicit source video position tracking - Fix split+speed bug: splitting a speed≠1 clip now correctly maps the right half to the correct source footage position - Fix audio region sync: convert source time → timeline time before comparing against audio region positions (was using mismatched coordinate spaces) - Fix cascade: clip move and speed change now cascade to annotations + audio regions (was only cascading to zoom regions) - Update effectiveSpeedRegions to use source coordinates via getClipSourceStartMs - Update mapTimelineTimeToSourceTime/mapSourceTimeToTimelineTime to use sourceStartMs - Preserve sourceStartMs through project save/load - Remove trim and speed as standalone user-editable concepts (clips are the only editing primitive; trims and speed regions are derived for export) - Remove trim/speed UI from SettingsPanel, TimelineEditor, and shortcuts
📝 WalkthroughWalkthroughThis PR refactors the video editor's region state management architecture. Trim and speed regions transition from standalone state to derived values computed from clip regions. A new Changes
Sequence DiagramsequenceDiagram
participant Clip as Clip State
participant Hook as useEditorRegions
participant VE as VideoEditor
participant UI as Timeline/Settings UI
Clip->>Hook: clipRegions update
Hook->>Hook: derive trimRegions via clipsToTrims()
Hook->>Hook: derive speedRegions from clip speeds
Hook->>VE: provide derived regions + handlers
VE->>VE: useMemo computes trim/speed from clips
VE->>UI: pass derived regions to Timeline/Settings
UI->>UI: render without trim/speed deletion controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/release.yml (1)
263-313:⚠️ Potential issue | 🟠 MajorUpload the dynamically named merged macOS metadata.
The merge now writes
release-assets/merged/${channel_filename}, but the artifact upload still requiresrelease-assets/merged/latest-mac.yml. For beta metadata, this producesbeta-mac.yml, so the upload step can fail or omit the updater metadata.🔧 Proposed fix
- - name: Upload merged latest-mac.yml artifact + - name: Upload merged macOS metadata artifact uses: actions/upload-artifact@v4 with: name: macos-merged-metadata - path: release-assets/merged/latest-mac.yml + path: release-assets/merged/*-mac.yml if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 263 - 313, The upload step currently hardcodes release-assets/merged/latest-mac.yml but the Python block writes release-assets/merged/{channel_filename} (variable channel_filename), causing missing-artifact failures when the file is named e.g. beta-mac.yml; modify the merge step to emit channel_filename as a step output (write "channel_filename=..." to $GITHUB_OUTPUT or otherwise persist it) and then update the "Upload merged latest-mac.yml artifact" step to use that step output for the path (reference the step that runs the Python merge and use its output instead of the static latest-mac.yml); keep the step name "Upload merged latest-mac.yml artifact" but change its with.path to the dynamic release-assets/merged/${{ steps.<merge-step>.outputs.channel_filename }}.src/components/video-editor/VideoEditor.tsx (2)
2630-2664:⚠️ Potential issue | 🟠 MajorLeft-edge clip resize doesn't update
sourceStartMs, corrupting source↔timeline mapping.
handleClipSpanChangeonly cascades on move (startDelta ≈ endDelta). On a resize, it just writes the newstartMs/endMsand leavessourceStartMsuntouched. BecausegetClipSourceEndMs = sourceStart + (endMs − startMs) × speed, a left-edge trim (startMs ↑, endMs fixed) shrinks the source range from its end instead of its start — the user visually crops the beginning of the clip but the exported source frames are cut from the tail. This directly undermines the coordinate-model fix that is the stated goal of this PR, and gets baked into saved projects.The same issue exists in
src/components/video-editor/hooks/useEditorRegions.ts(lines 324‑353).🛠️ Suggested fix: shift `sourceStartMs` proportionally on left-edge resize
const oldClip = clipRegions.find((c) => c.id === id); const newStart = Math.round(span.start); const newEnd = Math.round(span.end); if (oldClip) { const startDelta = newStart - oldClip.startMs; const endDelta = newEnd - oldClip.endMs; const isMove = Math.abs(startDelta - endDelta) < 1 && Math.abs(startDelta) > 0; if (isMove) { const delta = startDelta; const moveContained = <T extends { startMs: number; endMs: number }>( regions: T[], ): T[] => regions.map((r) => r.startMs >= oldClip.startMs && r.endMs <= oldClip.endMs ? { ...r, startMs: r.startMs + delta, endMs: r.endMs + delta } : r, ); setZoomRegions((prev) => moveContained(prev)); setAnnotationRegions((prev) => moveContained(prev)); setAudioRegions((prev) => moveContained(prev)); } } setClipRegions((prev) => prev.map((clip) => { - return clip.id === id ? { ...clip, startMs: newStart, endMs: newEnd } : clip; + if (clip.id !== id) return clip; + const oldStart = clip.startMs; + const leftDelta = newStart - oldStart; + const isLeftResize = + Math.abs(leftDelta) > 0 && Math.abs(leftDelta - (newEnd - clip.endMs)) >= 1; + const speed = + Number.isFinite(clip.speed) && clip.speed > 0 ? clip.speed : 1; + const sourceStart = clip.sourceStartMs ?? clip.startMs; + const nextSourceStart = isLeftResize + ? Math.max(0, Math.round(sourceStart + leftDelta * speed)) + : sourceStart; + return { + ...clip, + startMs: newStart, + endMs: newEnd, + sourceStartMs: nextSourceStart, + }; }), );
2666-2699:⚠️ Potential issue | 🟠 MajorSubsequent clips don't auto-shift when changing clip speed — creating timeline gaps or overlaps.
handleClipSpeedChangeupdates only the selected clip'sendMs. Any following clip withstartMs === oldEndMs(whichhandleClipSplitcreates) stays fixed, leaving a gap if speed increases or overlapping the right half if speed decreases. The timeline UI will render this broken layout; the export pipeline's source-time sorting may partially mask the issue, but the UX is broken.
handleClipSpanChangealready implements the shift pattern correctly — use the same approach to shift subsequent clips by(newEndMs − oldEndMs), preserving their source ranges viasourceStartMs. Alternatively, disallow speed changes on non-trailing clips and document this constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 2666 - 2699, handleClipSpeedChange currently updates only the selected clip's speed/endMs which leaves subsequent clips fixed and creates gaps/overlaps; change its logic (the function handleClipSpeedChange and the setClipRegions update) to compute oldEndMs and newEndMs for the selected clip, then apply a delta = newEndMs - oldEndMs to shift all following clipRegions (those with startMs >= oldEndMs) by that delta while preserving their source timing via sourceStartMs/sourceEndMs adjustments as handleClipSpanChange does; update setClipRegions to map over prev regions and for the selected id replace speed/endMs, and for subsequent regions shift startMs/endMs (and adjust sourceStartMs if needed) by delta so timeline remains contiguous (alternative: explicitly reject speed changes for non-trailing clips if shifting is undesired).
🧹 Nitpick comments (2)
src/components/video-editor/hooks/useEditorRegions.ts (1)
238-258:handleZoomDepthChange/handleZoomModeChangeshould bail out when no zoom is selected.
VideoEditor.tsxguards both withif (!selectedZoomId) return;. Here theselectedZoomId === nullbranch quietly maps over every region (all non-matches) and still produces a newzoomRegionsarray, defeating React's referential equality and invalidating downstream memos (e.g.effectiveZoomRegions, export config) on every call.const handleZoomDepthChange = useCallback( (depth: ZoomDepth) => { + if (!selectedZoomId) return; setZoomRegions((prev) => prev.map((r) => r.id === selectedZoomId ? { ...r, depth, focus: clampFocusToDepth(r.focus, depth) } : r, ), ); }, [selectedZoomId], ); const handleZoomModeChange = useCallback( (mode: ZoomMode) => { + if (!selectedZoomId) return; setZoomRegions((prev) => prev.map((r) => (r.id === selectedZoomId ? { ...r, mode } : r)), ); }, [selectedZoomId], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/hooks/useEditorRegions.ts` around lines 238 - 258, The handlers handleZoomDepthChange and handleZoomModeChange currently always call setZoomRegions even when selectedZoomId is null, causing a new array to be produced and breaking referential equality; update both callbacks to bail out early when selectedZoomId is null (or falsy) and do nothing, or within setZoomRegions return the previous state unchanged when no region matches, so that no new array reference is created; target the functions handleZoomDepthChange, handleZoomModeChange and the setZoomRegions usage to implement the early-return/no-op behavior.src/components/video-editor/VideoEditor.tsx (1)
570-582:speedRegionsis a zero-value alias overeffectiveSpeedRegions.Only one alias is referenced downstream (the two audio-sync effects) and both could read
effectiveSpeedRegionsdirectly. Droppingconst speedRegions = effectiveSpeedRegions;(and the matching references at lines 3165, 3207) removes a redundant indirection now that standalone speed regions are gone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/VideoEditor.tsx` around lines 570 - 582, The variable speedRegions is an unnecessary alias of effectiveSpeedRegions; remove the declaration const speedRegions = effectiveSpeedRegions; and update any downstream uses (currently the two audio-sync effects that reference speedRegions) to read effectiveSpeedRegions directly so only effectiveSpeedRegions (computed from clipRegions via getClipSourceStartMs/getClipSourceEndMs) remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/hooks/useEditorRegions.ts`:
- Around line 165-214: Replace the hardcoded depth values in handleZoomAdded and
handleZoomSuggested with the named constants DEFAULT_ZOOM_DEPTH and
DEFAULT_AUTO_ZOOM_DEPTH (import them from ./types), and update the
clampFocusToDepth call in handleZoomSuggested to use DEFAULT_AUTO_ZOOM_DEPTH
instead of 2; specifically, set newRegion.depth in handleZoomAdded to
DEFAULT_ZOOM_DEPTH, set newRegion.depth in handleZoomSuggested to
DEFAULT_AUTO_ZOOM_DEPTH, and call clampFocusToDepth(focus,
DEFAULT_AUTO_ZOOM_DEPTH) so the hook stays in sync with VideoEditor.tsx.
- Around line 45-50: The new hook useEditorRegions is implemented but not used
in VideoEditor.tsx, leaving duplicate inline region state/handlers; replace
VideoEditor.tsx's inline region state (zoom/clip/annotation/audio and their
handlers, including setActiveEffectSection usage) with the useEditorRegions hook
by importing and calling useEditorRegions, and ensure depth constants
align—either update useEditorRegions to consume DEFAULT_ZOOM_DEPTH and
DEFAULT_AUTO_ZOOM_DEPTH or pass those constants into the hook so both
implementations use the same depth values to avoid drift.
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 997-1002: The current hasAnyTimelineBlocks boolean includes
trimRegions which now represent derived, non-rendered/non-deletable trims;
remove trimRegions from the OR-list used to compute hasAnyTimelineBlocks (i.e.,
only consider zoomRegions, clipRegions, annotationRegions, and audioRegions) or
replace trimRegions with a filtered visibleTrimRegions that excludes derived
trims so select-all/delete reflects only visible, deletable blocks; update the
expression computing hasAnyTimelineBlocks in TimelineEditor (symbol:
hasAnyTimelineBlocks) accordingly.
In `@src/components/video-editor/types.ts`:
- Around line 173-179: The trim cursor can move backward when clips overlap; in
the loop over sorted clips (variables: clip, cursor, trims, trimId) replace the
assignment cursor = getClipSourceEndMs(clip) with advancing the cursor
monotonically (e.g. cursor = Math.max(cursor, getClipSourceEndMs(clip))) so
overlapping source ranges can't create a backward cursor and incorrect
subsequent trims; keep the existing gap check that uses
getClipSourceStartMs(clip).
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2682-2696: The scaleInClip helper currently checks only r.startMs
against clip bounds causing regions that extend beyond clip.endMs to be
mis-scaled; update scaleInClip (used by setZoomRegions, setAnnotationRegions,
setAudioRegions) to either require full containment (r.startMs >= clip.startMs
&& r.endMs <= clip.endMs) before scaling or, if partial overlaps must be
preserved, compute the scaled start and end then clamp the resulting endMs to
the scaled clip end (and ensure endMs >= startMs after rounding) so no child
region ends up outside the new clip bounds or inverted.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 263-313: The upload step currently hardcodes
release-assets/merged/latest-mac.yml but the Python block writes
release-assets/merged/{channel_filename} (variable channel_filename), causing
missing-artifact failures when the file is named e.g. beta-mac.yml; modify the
merge step to emit channel_filename as a step output (write
"channel_filename=..." to $GITHUB_OUTPUT or otherwise persist it) and then
update the "Upload merged latest-mac.yml artifact" step to use that step output
for the path (reference the step that runs the Python merge and use its output
instead of the static latest-mac.yml); keep the step name "Upload merged
latest-mac.yml artifact" but change its with.path to the dynamic
release-assets/merged/${{ steps.<merge-step>.outputs.channel_filename }}.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2666-2699: handleClipSpeedChange currently updates only the
selected clip's speed/endMs which leaves subsequent clips fixed and creates
gaps/overlaps; change its logic (the function handleClipSpeedChange and the
setClipRegions update) to compute oldEndMs and newEndMs for the selected clip,
then apply a delta = newEndMs - oldEndMs to shift all following clipRegions
(those with startMs >= oldEndMs) by that delta while preserving their source
timing via sourceStartMs/sourceEndMs adjustments as handleClipSpanChange does;
update setClipRegions to map over prev regions and for the selected id replace
speed/endMs, and for subsequent regions shift startMs/endMs (and adjust
sourceStartMs if needed) by delta so timeline remains contiguous (alternative:
explicitly reject speed changes for non-trailing clips if shifting is
undesired).
---
Nitpick comments:
In `@src/components/video-editor/hooks/useEditorRegions.ts`:
- Around line 238-258: The handlers handleZoomDepthChange and
handleZoomModeChange currently always call setZoomRegions even when
selectedZoomId is null, causing a new array to be produced and breaking
referential equality; update both callbacks to bail out early when
selectedZoomId is null (or falsy) and do nothing, or within setZoomRegions
return the previous state unchanged when no region matches, so that no new array
reference is created; target the functions handleZoomDepthChange,
handleZoomModeChange and the setZoomRegions usage to implement the
early-return/no-op behavior.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 570-582: The variable speedRegions is an unnecessary alias of
effectiveSpeedRegions; remove the declaration const speedRegions =
effectiveSpeedRegions; and update any downstream uses (currently the two
audio-sync effects that reference speedRegions) to read effectiveSpeedRegions
directly so only effectiveSpeedRegions (computed from clipRegions via
getClipSourceStartMs/getClipSourceEndMs) remains.
🪄 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
Run ID: 1f653b7c-865e-4b6d-8c52-55a74f401b0c
📒 Files selected for processing (11)
.github/workflows/release.ymlpackage.jsonsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/hooks/useEditorRegions.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/AudioWaveform.tsxsrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.tssrc/lib/shortcuts.ts
💤 Files with no reviewable changes (1)
- src/lib/shortcuts.ts
| export function useEditorRegions({ | ||
| duration, | ||
| currentTime, | ||
| videoPath, | ||
| setActiveEffectSection, | ||
| }: UseEditorRegionsParams) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 '\buseEditorRegions\b'Repository: webadderall/Recordly
Length of output: 780
🏁 Script executed:
rg -n "useEditorRegions" src/components/video-editor/VideoEditor.tsxRepository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
rg -n "import.*useEditorRegions" --type=tsRepository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
cat -n src/components/video-editor/VideoEditor.tsx | head -100Repository: webadderall/Recordly
Length of output: 3743
🏁 Script executed:
rg "import.*useEditorRegions|from.*useEditorRegions" --type=tsRepository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
rg "useState.*region|useState.*clip|useState.*annotation|useState.*audio" src/components/video-editor/VideoEditor.tsx -nRepository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
wc -l src/components/video-editor/VideoEditor.tsxRepository: webadderall/Recordly
Length of output: 112
🏁 Script executed:
rg "useEditorRegions" --type=tsRepository: webadderall/Recordly
Length of output: 272
🏁 Script executed:
cat -n src/components/video-editor/hooks/useEditorRegions.tsRepository: webadderall/Recordly
Length of output: 26354
🏁 Script executed:
rg "useState" src/components/video-editor/VideoEditor.tsx | head -20Repository: webadderall/Recordly
Length of output: 1479
🏁 Script executed:
rg "zoomRegions|clipRegions|annotationRegions|audioRegions" src/components/video-editor/VideoEditor.tsx -nRepository: webadderall/Recordly
Length of output: 4916
🏁 Script executed:
rg "const \[.*Regions" src/components/video-editor/VideoEditor.tsxRepository: webadderall/Recordly
Length of output: 352
🏁 Script executed:
fd "VideoEditor" src/components/video-editor --type fRepository: webadderall/Recordly
Length of output: 107
🏁 Script executed:
rg "handleZoom|handleClip|handleAnnotation|handleAudio" src/components/video-editor/VideoEditor.tsx | head -30Repository: webadderall/Recordly
Length of output: 1863
🏁 Script executed:
rg "depth.*3|depth.*2|depth.*1" src/components/video-editor/VideoEditor.tsx -C1 | head -40Repository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
rg "depth: [0-9]|depth=[0-9]" src/components/video-editor/hooks/useEditorRegions.ts -nRepository: webadderall/Recordly
Length of output: 99
🏁 Script executed:
rg "depth: [0-9]|depth=[0-9]" src/components/video-editor/VideoEditor.tsx -nRepository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
sed -n '165,214p' src/components/video-editor/hooks/useEditorRegions.tsRepository: webadderall/Recordly
Length of output: 1560
🏁 Script executed:
rg -n "const handleZoomAdded|const handleZoomSuggested" src/components/video-editor/VideoEditor.tsx -A 25Repository: webadderall/Recordly
Length of output: 1993
Hook is defined but not imported by VideoEditor.tsx.
useEditorRegions ships as dead code. VideoEditor.tsx declares its own region state (zoom, clip, annotation, audio) and all associated handlers inline. Both implementations currently differ in depth constants (hardcoded 3/2 in the hook vs. DEFAULT_ZOOM_DEPTH/DEFAULT_AUTO_ZOOM_DEPTH in VideoEditor) and documentation, creating drift risk.
Either integrate the hook into VideoEditor.tsx in this PR or document in the commit/PR description that the migration is deferred, so reviewers know both copies must remain synchronized until then.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/hooks/useEditorRegions.ts` around lines 45 - 50,
The new hook useEditorRegions is implemented but not used in VideoEditor.tsx,
leaving duplicate inline region state/handlers; replace VideoEditor.tsx's inline
region state (zoom/clip/annotation/audio and their handlers, including
setActiveEffectSection usage) with the useEditorRegions hook by importing and
calling useEditorRegions, and ensure depth constants align—either update
useEditorRegions to consume DEFAULT_ZOOM_DEPTH and DEFAULT_AUTO_ZOOM_DEPTH or
pass those constants into the hook so both implementations use the same depth
values to avoid drift.
| const handleZoomAdded = useCallback( | ||
| (span: Span) => { | ||
| const id = `zoom-${nextZoomIdRef.current++}`; | ||
| const newRegion: ZoomRegion = { | ||
| id, | ||
| startMs: Math.round(span.start), | ||
| endMs: Math.round(span.end), | ||
| depth: 3, | ||
| focus: { cx: 0.5, cy: 0.5 }, | ||
| mode: "manual", | ||
| }; | ||
| if (videoPath && pendingFreshRecordingAutoZoomPathRef.current === videoPath) { | ||
| autoSuggestedVideoPathRef.current = videoPath; | ||
| pendingFreshRecordingAutoZoomPathRef.current = null; | ||
| } | ||
| setZoomRegions((prev) => [...prev, newRegion]); | ||
| setSelectedZoomId(id); | ||
|
|
||
| setSelectedAnnotationId(null); | ||
| extensionHost.emitEvent({ | ||
| type: "timeline:region-added", | ||
| data: { id, startMs: newRegion.startMs, endMs: newRegion.endMs }, | ||
| }); | ||
| }, | ||
| [videoPath], | ||
| ); | ||
|
|
||
| const handleZoomSuggested = useCallback( | ||
| (span: Span, focus: ZoomFocus) => { | ||
| const id = `zoom-${nextZoomIdRef.current++}`; | ||
| const newRegion: ZoomRegion = { | ||
| id, | ||
| startMs: Math.round(span.start), | ||
| endMs: Math.round(span.end), | ||
| depth: 2, | ||
| focus: clampFocusToDepth(focus, 2), | ||
| mode: "auto", | ||
| }; | ||
| if (videoPath && pendingFreshRecordingAutoZoomPathRef.current === videoPath) { | ||
| autoSuggestedVideoPathRef.current = videoPath; | ||
| pendingFreshRecordingAutoZoomPathRef.current = null; | ||
| } | ||
| setZoomRegions((prev) => [...prev, newRegion]); | ||
| extensionHost.emitEvent({ | ||
| type: "timeline:region-added", | ||
| data: { id, startMs: newRegion.startMs, endMs: newRegion.endMs }, | ||
| }); | ||
| }, | ||
| [videoPath], | ||
| ); |
There was a problem hiding this comment.
Hardcoded zoom-depth magic numbers diverge from VideoEditor.tsx defaults.
handleZoomAdded uses depth: 3 and handleZoomSuggested uses depth: 2 / clampFocusToDepth(focus, 2). VideoEditor.tsx imports DEFAULT_ZOOM_DEPTH and DEFAULT_AUTO_ZOOM_DEPTH from ./types for the same paths. Once this hook is wired up, the editor will silently change its default zoom strengths unless the constants coincidentally line up. Reuse the named constants to keep the two paths in lock-step.
🔧 Suggested change
-import {
+import {
type AnnotationRegion,
type AudioRegion,
type ClipRegion,
clampFocusToDepth,
clipsToTrims,
DEFAULT_ANNOTATION_POSITION,
DEFAULT_ANNOTATION_SIZE,
DEFAULT_ANNOTATION_STYLE,
+ DEFAULT_AUTO_ZOOM_DEPTH,
DEFAULT_FIGURE_DATA,
+ DEFAULT_ZOOM_DEPTH,
type EditorEffectSection,
...
} from "../types";
@@
- depth: 3,
+ depth: DEFAULT_ZOOM_DEPTH,
focus: { cx: 0.5, cy: 0.5 },
mode: "manual",
@@
- depth: 2,
- focus: clampFocusToDepth(focus, 2),
+ depth: DEFAULT_AUTO_ZOOM_DEPTH,
+ focus: clampFocusToDepth(focus, DEFAULT_AUTO_ZOOM_DEPTH),
mode: "auto",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/hooks/useEditorRegions.ts` around lines 165 -
214, Replace the hardcoded depth values in handleZoomAdded and
handleZoomSuggested with the named constants DEFAULT_ZOOM_DEPTH and
DEFAULT_AUTO_ZOOM_DEPTH (import them from ./types), and update the
clampFocusToDepth call in handleZoomSuggested to use DEFAULT_AUTO_ZOOM_DEPTH
instead of 2; specifically, set newRegion.depth in handleZoomAdded to
DEFAULT_ZOOM_DEPTH, set newRegion.depth in handleZoomSuggested to
DEFAULT_AUTO_ZOOM_DEPTH, and call clampFocusToDepth(focus,
DEFAULT_AUTO_ZOOM_DEPTH) so the hook stays in sync with VideoEditor.tsx.
| const hasAnyTimelineBlocks = | ||
| zoomRegions.length > 0 || | ||
| trimRegions.length > 0 || | ||
| clipRegions.length > 0 || | ||
| annotationRegions.length > 0 || | ||
| speedRegions.length > 0 || | ||
| audioRegions.length > 0; |
There was a problem hiding this comment.
Exclude derived trims from “select all blocks”.
trimRegions are no longer rendered or user-deletable here, so counting them can enable select-all/delete behavior even when no visible block corresponds to those trims.
🔧 Proposed fix
const hasAnyTimelineBlocks =
zoomRegions.length > 0 ||
- trimRegions.length > 0 ||
clipRegions.length > 0 ||
annotationRegions.length > 0 ||
audioRegions.length > 0;📝 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.
| const hasAnyTimelineBlocks = | |
| zoomRegions.length > 0 || | |
| trimRegions.length > 0 || | |
| clipRegions.length > 0 || | |
| annotationRegions.length > 0 || | |
| speedRegions.length > 0 || | |
| audioRegions.length > 0; | |
| const hasAnyTimelineBlocks = | |
| zoomRegions.length > 0 || | |
| clipRegions.length > 0 || | |
| annotationRegions.length > 0 || | |
| audioRegions.length > 0; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 997 -
1002, The current hasAnyTimelineBlocks boolean includes trimRegions which now
represent derived, non-rendered/non-deletable trims; remove trimRegions from the
OR-list used to compute hasAnyTimelineBlocks (i.e., only consider zoomRegions,
clipRegions, annotationRegions, and audioRegions) or replace trimRegions with a
filtered visibleTrimRegions that excludes derived trims so select-all/delete
reflects only visible, deletable blocks; update the expression computing
hasAnyTimelineBlocks in TimelineEditor (symbol: hasAnyTimelineBlocks)
accordingly.
| for (const clip of sorted) { | ||
| if (clip.startMs > cursor) { | ||
| trims.push({ id: `trim-gap-${trimId++}`, startMs: cursor, endMs: clip.startMs }); | ||
| const sourceStart = getClipSourceStartMs(clip); | ||
| if (sourceStart > cursor) { | ||
| trims.push({ id: `trim-gap-${trimId++}`, startMs: cursor, endMs: sourceStart }); | ||
| } | ||
| cursor = getClipSourceEndMs(clip); | ||
| } |
There was a problem hiding this comment.
Keep the trim cursor monotonic for overlapping source ranges.
If two clips overlap in source coordinates, Line 178 can move cursor backward and generate an incorrect later trim that removes already-kept footage. Advance with Math.max instead.
🐛 Proposed fix
- cursor = getClipSourceEndMs(clip);
+ cursor = Math.max(cursor, getClipSourceEndMs(clip));📝 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.
| for (const clip of sorted) { | |
| if (clip.startMs > cursor) { | |
| trims.push({ id: `trim-gap-${trimId++}`, startMs: cursor, endMs: clip.startMs }); | |
| const sourceStart = getClipSourceStartMs(clip); | |
| if (sourceStart > cursor) { | |
| trims.push({ id: `trim-gap-${trimId++}`, startMs: cursor, endMs: sourceStart }); | |
| } | |
| cursor = getClipSourceEndMs(clip); | |
| } | |
| for (const clip of sorted) { | |
| const sourceStart = getClipSourceStartMs(clip); | |
| if (sourceStart > cursor) { | |
| trims.push({ id: `trim-gap-${trimId++}`, startMs: cursor, endMs: sourceStart }); | |
| } | |
| cursor = Math.max(cursor, getClipSourceEndMs(clip)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/types.ts` around lines 173 - 179, The trim cursor
can move backward when clips overlap; in the loop over sorted clips (variables:
clip, cursor, trims, trimId) replace the assignment cursor =
getClipSourceEndMs(clip) with advancing the cursor monotonically (e.g. cursor =
Math.max(cursor, getClipSourceEndMs(clip))) so overlapping source ranges can't
create a backward cursor and incorrect subsequent trims; keep the existing gap
check that uses getClipSourceStartMs(clip).
| // Scale all child regions within this clip proportionally | ||
| const scaleInClip = <T extends { startMs: number; endMs: number }>(regions: T[]): T[] => | ||
| regions.map((r) => { | ||
| if (r.startMs < clip.startMs || r.startMs >= clip.endMs) return r; | ||
| return { | ||
| ...zoom, | ||
| ...r, | ||
| startMs: Math.round( | ||
| clip.startMs + (zoom.startMs - clip.startMs) * scaleFactor, | ||
| clip.startMs + (r.startMs - clip.startMs) * scaleFactor, | ||
| ), | ||
| endMs: Math.round(clip.startMs + (zoom.endMs - clip.startMs) * scaleFactor), | ||
| endMs: Math.round(clip.startMs + (r.endMs - clip.startMs) * scaleFactor), | ||
| }; | ||
| }), | ||
| ); | ||
| }); | ||
| setZoomRegions((prev) => scaleInClip(prev)); | ||
| setAnnotationRegions((prev) => scaleInClip(prev)); | ||
| setAudioRegions((prev) => scaleInClip(prev)); |
There was a problem hiding this comment.
scaleInClip containment check only looks at r.startMs.
A child region that starts inside the clip but ends past clip.endMs will have both endpoints scaled using clip.startMs as the pivot, and its endMs can land past the new clip.endMs (or even before its new startMs when speeding up). Worth either filtering by full containment (r.endMs <= clip.endMs) or clamping the scaled endMs to the new clip bound.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/VideoEditor.tsx` around lines 2682 - 2696, The
scaleInClip helper currently checks only r.startMs against clip bounds causing
regions that extend beyond clip.endMs to be mis-scaled; update scaleInClip (used
by setZoomRegions, setAnnotationRegions, setAudioRegions) to either require full
containment (r.startMs >= clip.startMs && r.endMs <= clip.endMs) before scaling
or, if partial overlaps must be preserved, compute the scaled start and end then
clamp the resulting endMs to the scaled clip end (and ensure endMs >= startMs
after rounding) so no child region ends up outside the new clip bounds or
inverted.
1 participant
Summary
Fixes critical coordinate-space bugs in the clip editing system and removes trim/speed as standalone user-editable concepts (clips are now the only editing primitive).
Coordinate Model Fix:
sourceStartMsThe bug: When splitting a clip with speed ≠ 1, the right half pointed to the wrong source footage.
clip.startMswas serving double duty as both timeline position and source video position — these diverge as soon as speed changes or clips are rearranged.The fix: Added
sourceStartMstoClipRegionto explicitly track where in the source video each clip starts (defaults tostartMsfor backward compat). All coordinate conversion now usesgetClipSourceStartMs():mapTimelineTimeToSourceTime/mapSourceTimeToTimelineTime— usesourceStartMsfor source range checks and mappingeffectiveSpeedRegions—startMsfield uses source coordinateshandleClipSplit— computesrightSourceStartso the right half maps to correct source footagesourceStartMson save/loadAudio Region Sync Fix
currentTime(fromvideo.currentTime) is in source coordinates, butaudioRegion.startMs/endMsare timeline coordinates. The sync effect was comparing them directly. Now converts source time → timeline time viamapSourceTimeToTimelineTimebefore the comparison.Cascade Fix
Clip move (
handleClipSpanChange) and clip speed change (handleClipSpeedChange) now cascade position/scale changes to annotations and audio regions, not just zoom regions.Trim/Speed Removal
Removed trim and speed as standalone user-editable region types:
SettingsPanel,TimelineEditor, and keyboard shortcutstrimRegionsandspeedRegionsare now derived from clips viauseMemo(for export pipeline consumption)Verification
npx tsc --noEmit)Summary by CodeRabbit
Release Notes
Chores
Bug Fixes
Style