fix(editor): cascade clip modifier deletion - #242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesTimeline cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
EtienneLescot
left a comment
There was a problem hiding this comment.
This is the strongest PR in the batch, and the reason is a path neither the description nor CodeRabbit names.
The old guard was oldClips.length > 0 && newClips.length > 0. The first half was dead — arr.length !== oldClips.length already implies oldClips.length >= 1. The second half is the bug: delete the last clip and every anchored zoom, annotation, speed and camera region survives with clipId still pointing at the clip that just went away, and useTimeline.ts:977 writes that straight to disk.
It doesn't stay dangling. Reopen the project: Preview mounts the asset even with an empty timeline, handleLoadedMetadata sees clips.length === 0 and calls replaceTimeline to seed a full-duration clip (NewEditorShell.tsx:364-380). reconcileRegionsAfterReplace then finds those orphans anchored to a clip that isn't in surviving, falls through to anchorRegionsWithDerivedMs, and re-ventilates them from their stale ruler ms onto the new clip. placed comes back true, so they're kept. The zooms the user deleted reappear, at whatever source time their old ms happens to land on. Your change is what stops that.
I re-checked this against the region/pill work that landed since your merge base — no interaction. Clip deletion is only reachable from the per-clip trash button, which renders only while the clip is selected, and selectClip clears the pill selection first.
Merging. Two notes inline, neither blocking.
EtienneLescot
left a comment
There was a problem hiding this comment.
Threads cleared — the isAnchored asymmetry is unreachable on current main and the multi-asset coverage gap is tracked in #249, neither worth holding this for.
Approving. This is the one I most want in the tree: deleting the last clip left every anchored zoom, annotation, speed and camera region pointing at a clip that no longer existed, and the next project open reseeded a clip and re-ventilated them onto new footage. Deleted pills coming back on top of different video is about as bad as an editor bug gets, and the two new tests fail on main and pass here.
Summary
Related issue
No linked issue; found while auditing destructive timeline operations for orphaned state.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; this fixes document cleanup and timeline math.
Testing
npm exec -- vitest run src/lib/ai-edition/document/timeline.test.ts electron/ai-edition/document-service.test.tsnpm exec -- biome check src/lib/ai-edition/document/timeline.ts src/lib/ai-edition/document/timeline.test.ts electron/ai-edition/document-service.ts electron/ai-edition/document-service.test.tsnpm run build-vitenpm run wb:typecheckSummary by CodeRabbit