fix(export): preserve cropped native aspect ratio#747
Conversation
📝 WalkthroughWalkthroughNative MP4 dimension calculation now accounts for crop bounds. The video editor waits for preview readiness, passes crop state into the calculation, and debounces native crop-driven support probes with stale-request protection. Tests cover cropped dimensions and probe debounce decisions. ChangesMP4 export dimensions
Sequence Diagram(s)sequenceDiagram
participant PreviewVideo
participant VideoEditor
participant ExportDimensions
participant MP4SupportProbe
PreviewVideo->>VideoEditor: Preview readiness and video dimensions
VideoEditor->>ExportDimensions: Source dimensions, aspect ratio, and crop region
ExportDimensions-->>VideoEditor: Desired MP4 source dimensions
VideoEditor->>ExportDimensions: Previous and current probe snapshots
ExportDimensions-->>VideoEditor: Debounce decision
VideoEditor->>MP4SupportProbe: Delayed native crop probe
MP4SupportProbe-->>VideoEditor: Supported source dimensions
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/components/video-editor/VideoEditor.tsx (1)
1488-1496: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebounce crop-driven MP4 support probing
When the crop editor is in native aspect ratio, each drag update can change
desiredMp4SourceDimensionsand re-runprobeSupportedMp4Dimensions. The cache only helps on exact repeats; unique intermediate sizes still hit the encoder support check. Debounce the crop updates or probe only on commit/close.🤖 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/VideoEditor.tsx` around lines 1488 - 1496, Debounce crop-driven MP4 support probing to avoid checking every unique intermediate dimension during dragging. Update the logic around desiredMp4SourceDimensions and probeSupportedMp4Dimensions to defer probing until crop changes settle, or trigger it only when the crop is committed/closed, while preserving immediate handling of non-crop dependency changes.
🤖 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/VideoEditor.tsx`:
- Around line 1488-1496: Debounce crop-driven MP4 support probing to avoid
checking every unique intermediate dimension during dragging. Update the logic
around desiredMp4SourceDimensions and probeSupportedMp4Dimensions to defer
probing until crop changes settle, or trigger it only when the crop is
committed/closed, while preserving immediate handling of non-crop dependency
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b43a363b-9b0e-48a0-8e8a-d9b1307441f8
📒 Files selected for processing (3)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/exportDimensions.test.tssrc/components/video-editor/exportDimensions.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/video-editor/exportDimensions.test.ts (1)
28-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for crop ignored on fixed-aspect exports.
The PR explicitly states fixed-aspect-ratio exports keep their prior (non-cropped) behavior, but no test passes a
cropRegiontogether with a non-"native"aspect ratio to confirm it's ignored. Worth locking this in given it's the exact boundary this PR carves out.♻️ Suggested additional test
it("uses the rotated source bounds for 9:16 original exports", () => { expect(calculateMp4SourceDimensions(1920, 1080, "9:16")).toEqual({ width: 1080, height: 1920, }); }); + + it("ignores cropRegion for non-native aspect ratios", () => { + expect( + calculateMp4SourceDimensions(1920, 1080, "9:16", { width: 0.5, height: 0.5 }), + ).toEqual({ + width: 1080, + height: 1920, + }); + });🤖 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/exportDimensions.test.ts` around lines 28 - 47, Add a regression test alongside the existing calculateMp4SourceDimensions tests that passes a cropRegion with a fixed non-"native" aspect ratio and asserts the result matches the uncropped dimensions. Use calculateMp4SourceDimensions and a representative ratio such as "4:3", confirming cropRegion is ignored for fixed-aspect exports.
🤖 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/exportDimensions.test.ts`:
- Around line 28-47: Add a regression test alongside the existing
calculateMp4SourceDimensions tests that passes a cropRegion with a fixed
non-"native" aspect ratio and asserts the result matches the uncropped
dimensions. Use calculateMp4SourceDimensions and a representative ratio such as
"4:3", confirming cropRegion is ignored for fixed-aspect exports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c257064-3972-4729-b12e-9562845f5c15
📒 Files selected for processing (3)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/exportDimensions.test.tssrc/components/video-editor/exportDimensions.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
With Remove background enabled, a cropped recording could still export with the selected
wallpaper visible above and below the content. This made MP4 output disagree with the editor
preview.
Root cause and fix
The preview derives Native aspect ratio from the cropped source bounds, while MP4 sizing used the
uncropped source dimensions. It could also memoize the 1920x1080 fallback before video metadata
became ready.
This change makes Native MP4 sizing crop-aware and recalculates it when the preview reports usable
video metadata. Fixed aspect-ratio exports keep their existing behavior. Both modern and legacy
MP4 pipelines consume the corrected shared dimensions.
Verification
320x180instead of320x144) and nowpasses; focused suite: 7/7.
1440x810, visible marker wallpaper above/below the cropped recording;240x108, 60 frames / 2 seconds, no marker wallpaper;240x108, 60 frames / 2 seconds.remain blocked by pre-existing baseline diagnostics outside this diff.
Risk
The change is limited to Native MP4 output dimensions. Windows was exercised directly; macOS is
reasoned from shared TypeScript paths but was not run locally. GIF uses a separate sizing model and
is intentionally outside this focused fix.
Related: #746, #692
Summary by CodeRabbit