fix(editor): guard timeline speed overlap#585
Conversation
|
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 (1)
📝 WalkthroughWalkthroughThis PR introduces clip speed change planning logic to validate and apply per-clip playback speed adjustments while maintaining clip and zoom region timing consistency. The planning module is integrated into VideoEditor's speed-change handler and the timeline is updated to expose and display clip-specific speeds throughout the UI. ChangesClip Speed Change Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/video-editor/clipSpeedChange.test.ts (2)
18-78: ⚡ Quick winConsider expanding test coverage for additional scenarios.
The current test suite covers the core behaviors well. To further strengthen the tests, consider adding scenarios for:
Invalid inputs:
selectedClipIdthat doesn't exist → should returnnull- Invalid speed values (0, negative, NaN, Infinity) → should return
nullSpeeding up clips:
- Test with
speed: 2to verify clip shortens correctlyDefault speed handling:
- Clip with
speedundefined or invalid → should be treated as 1xZoom boundary cases:
- Zoom region that starts outside the clip → should not scale
- Multiple zooms inside clip that both scale and overlap each other
These additions would provide more comprehensive coverage and document edge-case behavior.
🤖 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/clipSpeedChange.test.ts` around lines 18 - 78, Add unit tests for planClipSpeedChange to cover missing/invalid inputs and edge cases: add a test where selectedClipId doesn't exist (expect null), tests for invalid speeds (0, negative, NaN, Infinity) returning null, a test for speeding up (speed: 2) asserting the selected clip shortens accordingly via clipRegions, a test where a clip has undefined/invalid speed and is treated as 1x, a test with a zoom region that starts outside the selected clip (ensure it is not scaled), and a test with multiple zooms inside the clip that when scaled would overlap (assert blockedReason "zoom-overlap"); reference planClipSpeedChange and the BlockedClipSpeedChange shape for expected outputs and use existing patterns from clipSpeedChange.test.ts to structure assertions.
9-16: ⚡ Quick winConsider adding edge case tests for completeness.
The current tests cover the main behaviors, but could be strengthened by adding test cases for:
- Negative speeds:
formatClipSpeedLabel(-1)→null- Non-finite values:
formatClipSpeedLabel(Infinity)→null,formatClipSpeedLabel(NaN)→nullThese edge cases are handled by the implementation (line 15), and adding explicit tests would improve coverage and prevent regressions.
📋 Suggested additional test cases
it("returns labels only for non-default positive speeds", () => { expect(formatClipSpeedLabel(1)).toBeNull(); expect(formatClipSpeedLabel(0)).toBeNull(); + expect(formatClipSpeedLabel(-1)).toBeNull(); + expect(formatClipSpeedLabel(Number.POSITIVE_INFINITY)).toBeNull(); + expect(formatClipSpeedLabel(Number.NaN)).toBeNull(); expect(formatClipSpeedLabel(0.5)).toBe("0.5x"); expect(formatClipSpeedLabel(2)).toBe("2x"); });🤖 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/clipSpeedChange.test.ts` around lines 9 - 16, Add edge-case assertions to the formatClipSpeedLabel test: verify that negative speeds (e.g., formatClipSpeedLabel(-1)) return null and that non-finite values (e.g., formatClipSpeedLabel(Infinity) and formatClipSpeedLabel(NaN)) also return null; update the "returns labels only for non-default positive speeds" spec in clipSpeedChange.test.ts to include these three expectations so the tests explicitly cover the negative and non-finite branches in formatClipSpeedLabel.
🤖 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.
Nitpick comments:
In `@src/components/video-editor/clipSpeedChange.test.ts`:
- Around line 18-78: Add unit tests for planClipSpeedChange to cover
missing/invalid inputs and edge cases: add a test where selectedClipId doesn't
exist (expect null), tests for invalid speeds (0, negative, NaN, Infinity)
returning null, a test for speeding up (speed: 2) asserting the selected clip
shortens accordingly via clipRegions, a test where a clip has undefined/invalid
speed and is treated as 1x, a test with a zoom region that starts outside the
selected clip (ensure it is not scaled), and a test with multiple zooms inside
the clip that when scaled would overlap (assert blockedReason "zoom-overlap");
reference planClipSpeedChange and the BlockedClipSpeedChange shape for expected
outputs and use existing patterns from clipSpeedChange.test.ts to structure
assertions.
- Around line 9-16: Add edge-case assertions to the formatClipSpeedLabel test:
verify that negative speeds (e.g., formatClipSpeedLabel(-1)) return null and
that non-finite values (e.g., formatClipSpeedLabel(Infinity) and
formatClipSpeedLabel(NaN)) also return null; update the "returns labels only for
non-default positive speeds" spec in clipSpeedChange.test.ts to include these
three expectations so the tests explicitly cover the negative and non-finite
branches in formatClipSpeedLabel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c061a103-5b09-4f5b-a51b-527de9fe22df
📒 Files selected for processing (7)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/clipSpeedChange.test.tssrc/components/video-editor/clipSpeedChange.tssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/components/viewport/TimelineCanvas.tsxsrc/components/video-editor/timeline/model/timelineModel.test.tssrc/components/video-editor/timeline/model/timelineModel.ts
1 participant
Summary
Root Cause
Changing a clip to a slower speed extends the clip duration, but the current timeline model does not separate source-time anchors from timeline-time anchors. Auto-shifting downstream clips would be a broader architecture change, so this hotfix prevents creating an overlapping timeline state.
Scope
This PR only addresses issue #581 item 3/4: speed-edited timeline overlap and visible speed information. It does not change the Windows HUD/input blocker or Lightning export routing.
Local Checks
npm test -- src/components/video-editor/clipSpeedChange.test.ts src/components/video-editor/timeline/model/timelineModel.test.tsnpx tsc --noEmit --pretty falsenpx biome check src/components/video-editor/clipSpeedChange.ts src/components/video-editor/clipSpeedChange.test.ts src/components/video-editor/VideoEditor.tsx src/components/video-editor/timeline/Item.tsx src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx src/components/video-editor/timeline/model/timelineModel.ts src/components/video-editor/timeline/model/timelineModel.test.ts --formatter-enabled=falsegit diff --checkCloses part of #581.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests