[codex] clean up clip-first timeline editing#406
Open
webadderall wants to merge 4 commits intomainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdded renderer-side chunked Blob export streaming with fallback, migrated timeline state and time-mapping to a clip-centric model (new mapping utilities), introduced ghost zoom hover/placement UI, updated playback seek/trim behavior, and removed trim/speed selection/shortcuts in favor of clip-oriented controls. ChangesExport streaming & pending-save
Timeline & clip-model refactor
Timeline UI & interactions
Seek / playback behavior
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (VideoEditor)
participant Main as Electron Main
participant FS as Filesystem
Renderer->>Main: openExportStream(filename, extension)
Main-->>Renderer: streamId / tempFilePath
Renderer->>Main: writeExportStreamChunk(streamId, chunk)
Main->>FS: append chunk to temp file
Main-->>Renderer: ack
alt all chunks written
Renderer->>Main: closeExportStream(streamId)
Main-->>Renderer: finalize / return tempFilePath
else streaming unavailable/fails
Renderer->>Renderer: convert Blob -> ArrayBuffer
Renderer->>Main: saveArrayBuffer(filePath, arrayBuffer)
Main->>FS: write full buffer to path
Main-->>Renderer: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Review rate limit: 8/10 reviews remaining, refill in 12 minutes. Comment |
![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: 3
🤖 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/timeline/TimelineEditor.tsx`:
- Around line 831-845: The click handler handleZoomRowClick is short-circuiting
invalid clicks by checking canPlaceZoomAtMs and returning early, which prevents
the placement logic and error toast inside onAddZoomAtMs from running; change it
so after computing startMs (using zoomRowHoverMs and videoDurationMs) you always
call onAddZoomAtMs(startMs) when onAddZoomAtMs and zoomRowHoverMs are present,
and remove the early-return guard that calls canPlaceZoomAtMs — keep
event.stopPropagation and the null checks for onAddZoomAtMs/zoomRowHoverMs but
do not pre-validate placement here so addZoomAtMs can handle validation and
feedback.
In `@src/components/video-editor/types.ts`:
- Around line 196-203: The clip-mapper loops (the one iterating sortedClips
using roundedTimeMs and calling getSafeClipSpeed to compute returned time) treat
clip end as inclusive; change the end check to be end-exclusive (use
roundedTimeMs >= clip.endMs rather than >) so boundary times map to the next
clip like findClipAtTimelineTime does, and apply the same half-open fix to the
other similar loop/block later in the file (the block around the other mapper at
the noted second diff).
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 2774-2778: When converting legacy trims to clips in the branch
where trimRegions.length > 0 (the block that calls trimsToClips(trimRegions,
totalMs), setClipRegions(...), and marks clipInitializedRef.current = true), you
must advance nextClipIdRef.current so newly created clip ids won’t collide with
the generated "clip-*" ids; after calling trimsToClips, inspect the returned
clip objects to compute the highest numeric suffix used in their id strings
(they are created as "clip-<n>") and set nextClipIdRef.current to that max+1 (or
otherwise ensure nextClipIdRef.current is greater than any existing clip id)
before returning so subsequent split/add operations use unique ids.
🪄 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: 4e278f63-7c44-4171-afb8-2bfdc9007607
📒 Files selected for processing (8)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.test.tssrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/videoEventHandlers.test.tssrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/lib/shortcuts.ts
💤 Files with no reviewable changes (1)
- src/lib/shortcuts.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/VideoEditor.tsx`:
- Around line 2774-2781: When migrating trims to clips in the trimsToClips
branch, clear the auto-full-clip tracking refs so the temporary full-track clip
doesn't get treated as auto-generated later: after calling trimsToClips and
before returning/setting state in the block that uses migratedClips, set
autoFullTrackClipIdRef.current = null and autoFullTrackClipEndMsRef.current =
null (these refs are the ones primed earlier and used by
extendAutoFullTrackClip), ensuring extendAutoFullTrackClip cannot identify a
migrated clip as the auto-generated clip.
- Around line 2898-2917: When creating a new zoom in handleZoomAdded, normalize
selection state by clearing any clip/audio/other selections and switching the
side panel to the zoom inspector so only the new zoom is highlighted; after
setSelectedZoomId(id) (and setSelectedAnnotationId(null)) also call the relevant
setters to clear clip/audio selection (e.g. setSelectedClipId(null),
setSelectedAudioId(null), or their equivalents) and set the side panel/inspector
to the zoom view (e.g. setSidePanel("zoom") or setInspectorPanel("zoom")) so the
inspector shows the new zoom.
🪄 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: df759516-a926-4ffb-aa51-eb893a75ea42
📒 Files selected for processing (4)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.test.tssrc/components/video-editor/types.ts
✅ Files skipped from review due to trivial changes (2)
- src/components/video-editor/types.test.ts
- src/components/video-editor/types.ts
Comment on lines
+2774
to
+2781
| if (trimRegions.length > 0) { | ||
| const migratedClips = trimsToClips(trimRegions, totalMs); | ||
| nextClipIdRef.current = deriveNextId( | ||
| "clip", | ||
| migratedClips.map((clip) => clip.id), | ||
| ); | ||
| setClipRegions(migratedClips); | ||
| clipInitializedRef.current = true; |
Contributor
There was a problem hiding this comment.
Clear the auto-full-clip refs in the trim-migration branch.
Line 2774 returns early after trimsToClips(...), but autoFullTrackClipIdRef.current / autoFullTrackClipEndMsRef.current were already primed with the temporary full-track clip just above. If duration changes again, extendAutoFullTrackClip(...) can misidentify a migrated clip as auto-generated and stretch it.
Suggested fix
if (trimRegions.length > 0) {
const migratedClips = trimsToClips(trimRegions, totalMs);
+ autoFullTrackClipIdRef.current = null;
+ autoFullTrackClipEndMsRef.current = null;
nextClipIdRef.current = deriveNextId(
"clip",
migratedClips.map((clip) => clip.id),
);
setClipRegions(migratedClips);🤖 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 2774 - 2781, When
migrating trims to clips in the trimsToClips branch, clear the auto-full-clip
tracking refs so the temporary full-track clip doesn't get treated as
auto-generated later: after calling trimsToClips and before returning/setting
state in the block that uses migratedClips, set autoFullTrackClipIdRef.current =
null and autoFullTrackClipEndMsRef.current = null (these refs are the ones
primed earlier and used by extendAutoFullTrackClip), ensuring
extendAutoFullTrackClip cannot identify a migrated clip as the auto-generated
clip.
Comment on lines
2898
to
2917
| const handleZoomAdded = useCallback( | ||
| (span: Span) => { | ||
| const id = `zoom-${nextZoomIdRef.current++}`; | ||
| const defaultDepth: ZoomDepth = 2; | ||
| const newRegion: ZoomRegion = { | ||
| id, | ||
| startMs: Math.round(span.start), | ||
| endMs: Math.round(span.end), | ||
| depth: DEFAULT_ZOOM_DEPTH, | ||
| focus: { cx: 0.5, cy: 0.5 }, | ||
| mode: "manual", | ||
| depth: defaultDepth, | ||
| focus: clampFocusToDepth({ cx: 0.5, cy: 0.5 }, defaultDepth), | ||
| mode: "auto", | ||
| }; | ||
| if (videoPath && pendingFreshRecordingAutoZoomPathRef.current === videoPath) { | ||
| autoSuggestedVideoPathRef.current = videoPath; | ||
| pendingFreshRecordingAutoZoomPathRef.current = null; | ||
| } | ||
| setZoomRegions((prev) => [...prev, newRegion]); | ||
| setSelectedZoomId(id); | ||
| setSelectedTrimId(null); | ||
| setSelectedAnnotationId(null); | ||
| extensionHost.emitEvent({ |
Contributor
There was a problem hiding this comment.
Normalize selection state when a new zoom is created.
Line 2915 selects the new zoom, but the add flow still leaves clip/audio selection and the current side panel untouched. That can leave multiple items highlighted and keeps the inspector on the previous section instead of the newly created zoom.
Suggested fix
setZoomRegions((prev) => [...prev, newRegion]);
setSelectedZoomId(id);
+ setSelectedClipId(null);
setSelectedAnnotationId(null);
+ setSelectedAudioId(null);
+ setActiveEffectSection("zoom");
extensionHost.emitEvent({
type: "timeline:region-added",
data: { id, startMs: newRegion.startMs, endMs: newRegion.endMs },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleZoomAdded = useCallback( | |
| (span: Span) => { | |
| const id = `zoom-${nextZoomIdRef.current++}`; | |
| const defaultDepth: ZoomDepth = 2; | |
| const newRegion: ZoomRegion = { | |
| id, | |
| startMs: Math.round(span.start), | |
| endMs: Math.round(span.end), | |
| depth: DEFAULT_ZOOM_DEPTH, | |
| focus: { cx: 0.5, cy: 0.5 }, | |
| mode: "manual", | |
| depth: defaultDepth, | |
| focus: clampFocusToDepth({ cx: 0.5, cy: 0.5 }, defaultDepth), | |
| mode: "auto", | |
| }; | |
| if (videoPath && pendingFreshRecordingAutoZoomPathRef.current === videoPath) { | |
| autoSuggestedVideoPathRef.current = videoPath; | |
| pendingFreshRecordingAutoZoomPathRef.current = null; | |
| } | |
| setZoomRegions((prev) => [...prev, newRegion]); | |
| setSelectedZoomId(id); | |
| setSelectedTrimId(null); | |
| setSelectedAnnotationId(null); | |
| extensionHost.emitEvent({ | |
| const handleZoomAdded = useCallback( | |
| (span: Span) => { | |
| const id = `zoom-${nextZoomIdRef.current++}`; | |
| const defaultDepth: ZoomDepth = 2; | |
| const newRegion: ZoomRegion = { | |
| id, | |
| startMs: Math.round(span.start), | |
| endMs: Math.round(span.end), | |
| depth: defaultDepth, | |
| focus: clampFocusToDepth({ cx: 0.5, cy: 0.5 }, defaultDepth), | |
| mode: "auto", | |
| }; | |
| if (videoPath && pendingFreshRecordingAutoZoomPathRef.current === videoPath) { | |
| autoSuggestedVideoPathRef.current = videoPath; | |
| pendingFreshRecordingAutoZoomPathRef.current = null; | |
| } | |
| setZoomRegions((prev) => [...prev, newRegion]); | |
| setSelectedZoomId(id); | |
| setSelectedClipId(null); | |
| setSelectedAnnotationId(null); | |
| setSelectedAudioId(null); | |
| setActiveEffectSection("zoom"); | |
| extensionHost.emitEvent({ |
🤖 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 2898 - 2917, When
creating a new zoom in handleZoomAdded, normalize selection state by clearing
any clip/audio/other selections and switching the side panel to the zoom
inspector so only the new zoom is highlighted; after setSelectedZoomId(id) (and
setSelectedAnnotationId(null)) also call the relevant setters to clear
clip/audio selection (e.g. setSelectedClipId(null), setSelectedAudioId(null), or
their equivalents) and set the side panel/inspector to the zoom view (e.g.
setSidePanel("zoom") or setInspectorPanel("zoom")) so the inspector shows the
new zoom.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR removes the most confusing clip/trim split-brain paths so the editor behaves more consistently as a clip-first workflow.
What changed
Why
The clip feature still exposed legacy trim and speed-region behaviors in shortcuts, playback, and timeline interactions. That made the product feel unpredictable: users could land in removed space, trigger hidden edit modes, or work with timeline semantics that did not match what the UI emphasized.
Impact
This keeps the existing clip cleanup behavior you wanted, while reducing the unintuitive parts around seeking, gap handling, and hidden legacy editing paths.
Validation
npx vitest run src/components/video-editor/types.test.ts src/components/video-editor/videoPlayback/videoEventHandlers.test.tsnpx tsc --noEmitNotes
The local worktree had unrelated in-flight changes, so this PR is intentionally scoped to the staged clip/timeline files only.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores