fix: bound audio waveform to clip row; cascade-delete regions on clip removal#270
Closed
webadderall wants to merge 2 commits intomainfrom
Closed
fix: bound audio waveform to clip row; cascade-delete regions on clip removal#270webadderall wants to merge 2 commits intomainfrom
webadderall wants to merge 2 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughVideoEditor internals updated: webcam URL resolution and export progress logic changed, clip deletion now cascade-removes enclosed timeline regions, thumbnail background hard-coded, UI theme/styling adjusted; minor timeline component styling and import-order edits; CI workflow asset naming and package version bumped. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoEditor.tsx (1)
5213-5217:⚠️ Potential issue | 🟠 MajorWire clip deletion into
TimelineEditor.
TimelineEditor’s Delete/Backspace path callsonClipDelete, but this invocation never passeshandleClipDelete, so deleting a selected clip from the timeline still does nothing and bypasses the new cascade behavior.Proposed fix
clipRegions={clipRegions} onClipSplit={handleClipSplit} onClipSpanChange={handleClipSpanChange} + onClipDelete={handleClipDelete} selectedClipId={selectedClipId} onSelectClip={handleSelectClip}🤖 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 5213 - 5217, TimelineEditor is wired to call onClipDelete on Delete/Backspace but the prop wasn’t passed; update the TimelineEditor props (the JSX block that currently sets clipRegions, onClipSplit, onClipSpanChange, selectedClipId, onSelectClip) to include onClipDelete={handleClipDelete} so the component invokes the existing handleClipDelete cascade logic when a clip is deleted.
🤖 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 2811-2826: The cascade that removes zoom regions inside the
clip-delete block filters zoom state directly, which skips the existing
handleZoomDelete/extensionHost notifications; change the zoom removal to first
compute the list of zoom regions being removed (those where r.startMs >= startMs
&& r.endMs <= endMs or your existing "fully within" predicate), call the same
removal notifier used by handleZoomDelete (e.g., invoke handleZoomDelete or emit
extensionHost.notify('timeline:region-removed', region) for each removed zoom)
and then update setZoomRegions with the filtered result, leaving the other
cascaded filters (setAnnotationRegions, setTrimRegions, setSpeedRegions,
setAudioRegions) unchanged. Ensure you reference clipRegions, setZoomRegions,
startMs, endMs, handleZoomDelete/extensionHost and perform notification before
committing the new zoom state.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 5213-5217: TimelineEditor is wired to call onClipDelete on
Delete/Backspace but the prop wasn’t passed; update the TimelineEditor props
(the JSX block that currently sets clipRegions, onClipSplit, onClipSpanChange,
selectedClipId, onSelectClip) to include onClipDelete={handleClipDelete} so the
component invokes the existing handleClipDelete cascade logic when a clip is
deleted.
🪄 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: 23f02a36-2eb0-4e34-8fd6-2d11d16f8494
📒 Files selected for processing (4)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/timeline/AudioWaveform.tsxsrc/components/video-editor/timeline/Row.tsxsrc/components/video-editor/timeline/TimelineEditor.tsx
Comment on lines
+2811
to
+2826
| const deletedClip = clipRegions.find((c) => c.id === id); | ||
| setClipRegions((prev) => prev.filter((clip) => clip.id !== id)); | ||
| if (deletedClip) { | ||
| const { startMs, endMs } = deletedClip; | ||
| // Cascade: remove all timeline items fully within the deleted clip's span | ||
| setZoomRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | ||
| setAnnotationRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | ||
| setTrimRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | ||
| setSpeedRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | ||
| setAudioRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | ||
| } | ||
| if (selectedClipId === id) { | ||
| setSelectedClipId(null); | ||
| } | ||
| }, | ||
| [selectedClipId], | ||
| [clipRegions, selectedClipId], |
Contributor
There was a problem hiding this comment.
Emit removal events for cascaded zoom deletions.
handleZoomDelete notifies extensionHost, but the new cascade removes zooms by filtering state directly. Extensions listening for timeline:region-removed can miss zooms removed via clip deletion.
Proposed fix
const handleClipDelete = useCallback(
(id: string) => {
const deletedClip = clipRegions.find((c) => c.id === id);
setClipRegions((prev) => prev.filter((clip) => clip.id !== id));
if (deletedClip) {
const { startMs, endMs } = deletedClip;
+ const isInsideDeletedClip = (region: { startMs: number; endMs: number }) =>
+ region.startMs >= startMs && region.endMs <= endMs;
+ const removedZoomIds = zoomRegions
+ .filter(isInsideDeletedClip)
+ .map((region) => region.id);
// Cascade: remove all timeline items fully within the deleted clip's span
- setZoomRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs));
- setAnnotationRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs));
- setTrimRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs));
- setSpeedRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs));
- setAudioRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs));
+ setZoomRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r)));
+ setAnnotationRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r)));
+ setTrimRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r)));
+ setSpeedRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r)));
+ setAudioRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r)));
+ removedZoomIds.forEach((removedId) => {
+ extensionHost.emitEvent({
+ type: "timeline:region-removed",
+ data: { id: removedId },
+ });
+ });
}
if (selectedClipId === id) {
setSelectedClipId(null);
}
},
- [clipRegions, selectedClipId],
+ [clipRegions, selectedClipId, zoomRegions],
);📝 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 deletedClip = clipRegions.find((c) => c.id === id); | |
| setClipRegions((prev) => prev.filter((clip) => clip.id !== id)); | |
| if (deletedClip) { | |
| const { startMs, endMs } = deletedClip; | |
| // Cascade: remove all timeline items fully within the deleted clip's span | |
| setZoomRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | |
| setAnnotationRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | |
| setTrimRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | |
| setSpeedRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | |
| setAudioRegions((prev) => prev.filter((r) => r.startMs < startMs || r.endMs > endMs)); | |
| } | |
| if (selectedClipId === id) { | |
| setSelectedClipId(null); | |
| } | |
| }, | |
| [selectedClipId], | |
| [clipRegions, selectedClipId], | |
| const handleClipDelete = useCallback( | |
| (id: string) => { | |
| const deletedClip = clipRegions.find((c) => c.id === id); | |
| setClipRegions((prev) => prev.filter((clip) => clip.id !== id)); | |
| if (deletedClip) { | |
| const { startMs, endMs } = deletedClip; | |
| const isInsideDeletedClip = (region: { startMs: number; endMs: number }) => | |
| region.startMs >= startMs && region.endMs <= endMs; | |
| const removedZoomIds = zoomRegions | |
| .filter(isInsideDeletedClip) | |
| .map((region) => region.id); | |
| // Cascade: remove all timeline items fully within the deleted clip's span | |
| setZoomRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r))); | |
| setAnnotationRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r))); | |
| setTrimRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r))); | |
| setSpeedRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r))); | |
| setAudioRegions((prev) => prev.filter((r) => !isInsideDeletedClip(r))); | |
| removedZoomIds.forEach((removedId) => { | |
| extensionHost.emitEvent({ | |
| type: "timeline:region-removed", | |
| data: { id: removedId }, | |
| }); | |
| }); | |
| } | |
| if (selectedClipId === id) { | |
| setSelectedClipId(null); | |
| } | |
| }, | |
| [clipRegions, selectedClipId, zoomRegions], | |
| ); |
🤖 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 2811 - 2826, The
cascade that removes zoom regions inside the clip-delete block filters zoom
state directly, which skips the existing handleZoomDelete/extensionHost
notifications; change the zoom removal to first compute the list of zoom regions
being removed (those where r.startMs >= startMs && r.endMs <= endMs or your
existing "fully within" predicate), call the same removal notifier used by
handleZoomDelete (e.g., invoke handleZoomDelete or emit
extensionHost.notify('timeline:region-removed', region) for each removed zoom)
and then update setZoomRegions with the filtered result, leaving the other
cascaded filters (setAnnotationRegions, setTrimRegions, setSpeedRegions,
setAudioRegions) unchanged. Ensure you reference clipRegions, setZoomRegions,
startMs, endMs, handleZoomDelete/extensionHost and perform notification before
committing the new zoom state.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
303-313:⚠️ Potential issue | 🟠 MajorUpload the dynamically named merged macOS metadata.
Line 303 writes
{channel_filename}, but Lines 308-313 still upload onlylatest-mac.yml. Abeta-mac.ymlor other channel release will fail withif-no-files-found: error.🐛 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 303 - 313, The upload step currently hardcodes path: release-assets/merged/latest-mac.yml while the workflow writes release-assets/merged/{channel_filename}; update the "Upload merged latest-mac.yml artifact" step so its path uses the same dynamic channel filename variable (the one used when writing channel_filename -> release-assets/merged/{channel_filename}) instead of the static latest-mac.yml, ensuring the artifact upload references the matching dynamic file name and preserves the if-no-files-found behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 303-313: The upload step currently hardcodes path:
release-assets/merged/latest-mac.yml while the workflow writes
release-assets/merged/{channel_filename}; update the "Upload merged
latest-mac.yml artifact" step so its path uses the same dynamic channel filename
variable (the one used when writing channel_filename ->
release-assets/merged/{channel_filename}) instead of the static latest-mac.yml,
ensuring the artifact upload references the matching dynamic file name and
preserves the if-no-files-found behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d703e885-b313-44e3-be82-9c6ea37815d3
📒 Files selected for processing (2)
.github/workflows/release.ymlpackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
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.
Changes\n\n### Waveform bounding\nAdd
overflow-hiddento the timeline Row inner container so the audio waveform canvas (absolute inset-0) is properly clipped to its track row and cannot visually overflow into adjacent rows.\n\n### Cascade delete\nWhen a clip region is deleted, all zoom, annotation, trim, speed, and audio regions whose spans fall fully within the deleted clip's time range are also removed. This prevents orphaned timeline items from accumulating after clip deletion.\n\n## Files changed\n-timeline/Row.tsx— addedoverflow-hiddento row inner container\n-VideoEditor.tsx—handleClipDeletenow cascades deletion to child regions within the clip span\n-timeline/TimelineEditor.tsx— existing timeline rendering (no functional change, restoring clean pre-refactor baseline)\n-timeline/AudioWaveform.tsx— existing waveform canvas (no functional change)Summary by CodeRabbit
Bug Fixes
New Features
Style
Chores