Merged
Conversation
📝 WalkthroughWalkthroughThe changes integrate aspect ratio support throughout the video export pipeline, strengthening export success validation to require a file path and refactoring dimension calculations to preserve aspect ratios across GIF and MP4 exports with even dimension constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/lib/exporter/gifExporter.ts (2)
70-73: Consider inlining the intermediate variable intoEven.The helper works correctly but the intermediate
evenValuevariable is unnecessary.♻️ Minor simplification
const toEven = (value: number) => { - const evenValue = Math.max(2, Math.floor(value / 2) * 2); - return evenValue; + return Math.max(2, Math.floor(value / 2) * 2); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.ts` around lines 70 - 73, The helper function toEven can be simplified by removing the unnecessary intermediate variable; replace the body of toEven to directly return Math.max(2, Math.floor(value / 2) * 2) instead of assigning it to evenValue first so the function becomes a single-return expression while preserving existing behavior.
88-94: Non-original presets don't clamp to source dimensions.For presets like "medium" (maxHeight=720) or "large" (maxHeight=1080), the code always scales to
maxHeighteven if the source is smaller (e.g., a 480p video would be upscaled to 720p). This may be intentional for consistent output sizes, but could result in quality loss from upscaling small sources.Consider clamping to source dimensions when source is smaller than the preset:
💡 Optional: Avoid upscaling small sources
+ const effectiveMaxHeight = Math.min(maxHeight, sourceHeight); - const targetHeight = maxHeight; + const targetHeight = effectiveMaxHeight; const targetWidth = Math.round(targetHeight * aspectRatio);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.ts` around lines 88 - 94, The current sizing always uses maxHeight and can upscale small sources; change the target height computation to clamp to the source height (e.g., targetHeight = Math.min(maxHeight, sourceHeight or inputHeight)) before computing targetWidth from aspectRatio, then keep using Math.round and toEven for targetWidth/height; update any references to aspectRatio, maxHeight, targetHeight, targetWidth and toEven in gifExporter.ts so presets won't upscale sources.src/components/video-editor/VideoEditor.tsx (1)
1493-1499: Duplicated aspect ratio computation logic.The
aspectRatio === "native"conditional withgetNativeAspectRatioValue/getAspectRatioValueis repeated in multiple places (Lines 990-993, 1235-1238, 1339-1345, 1493-1499). Consider extracting this into a shared helper or memoized value to reduce duplication and risk of divergence.♻️ Extract aspect ratio computation
Add a memoized helper near the top of the component:
const currentAspectRatioValue = useMemo(() => { const video = videoPlaybackRef.current?.video; const sourceWidth = video?.videoWidth || 1920; const sourceHeight = video?.videoHeight || 1080; return aspectRatio === "native" ? getNativeAspectRatioValue(sourceWidth, sourceHeight, cropRegion) : getAspectRatioValue(aspectRatio); }, [aspectRatio, cropRegion, /* video dimensions need special handling */]);Then use
currentAspectRatioValuein all locations.Note: Since
videoPlaybackRef.current?.videodimensions may change after load, you may need to track them in state or use a different approach.🤖 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 1493 - 1499, Extract the duplicated aspect-ratio logic into a memoized helper (e.g., const currentAspectRatioValue = useMemo(...)) computed from aspectRatio, cropRegion and the video dimensions from videoPlaybackRef.current?.video (use video?.videoWidth || 1920 and video?.videoHeight || 1080 inside the memo); replace all inline conditionals that call getNativeAspectRatioValue(...) / getAspectRatioValue(...) with currentAspectRatioValue in usages across the component (locations where aspectRatio === "native" is repeated, e.g., around VideoEditor render logic and any functions using those values); ensure the memo's dependencies include aspectRatio and cropRegion and add handling to update video dimensions (store dimensions in state or include a derived ref value) so the memo updates after the video loads.src/lib/exporter/gifExporter.test.ts (1)
1-19: Tests look correct but coverage is limited.The two tests verify key scenarios:
- Scaled export with explicit aspect ratio (16:9 "medium" → 1280×720) ✓
- Original preset fitting within source bounds (1080×1920 with 16:9 → 1080×606) ✓
Consider adding tests for edge cases:
- Invalid/zero aspect ratio (fallback behavior)
- Portrait aspect ratios (e.g., 9:16)
- "large" preset
- Source dimensions smaller than preset maxHeight
💡 Additional test cases to consider
it("falls back to source aspect ratio when targetAspectRatio is invalid", () => { expect(calculateOutputDimensions(1920, 1080, "medium", GIF_SIZE_PRESETS, 0)).toEqual({ width: 1280, height: 720, }); }); it("handles portrait aspect ratios", () => { expect(calculateOutputDimensions(1920, 1080, "medium", GIF_SIZE_PRESETS, 9 / 16)).toEqual({ width: 404, // 720 * (9/16) ≈ 405 → toEven → 404 height: 720, }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.test.ts` around lines 1 - 19, Add additional unit tests for calculateOutputDimensions to cover edge cases: (1) invalid/zero targetAspectRatio — verify it falls back to source aspect ratio (use GIF_SIZE_PRESETS and the "medium" preset in a test like "falls back to source aspect ratio when targetAspectRatio is invalid"), (2) portrait aspect ratios — test with 9/16 to ensure width/height are computed and evened correctly (e.g., "handles portrait aspect ratios"), (3) the "large" preset — ensure scaled dimensions match expected large preset behavior, and (4) source dimensions smaller than a preset's maxHeight — ensure output is clamped to source bounds. Reference calculateOutputDimensions and GIF_SIZE_PRESETS when writing each new it() case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1493-1499: Extract the duplicated aspect-ratio logic into a
memoized helper (e.g., const currentAspectRatioValue = useMemo(...)) computed
from aspectRatio, cropRegion and the video dimensions from
videoPlaybackRef.current?.video (use video?.videoWidth || 1920 and
video?.videoHeight || 1080 inside the memo); replace all inline conditionals
that call getNativeAspectRatioValue(...) / getAspectRatioValue(...) with
currentAspectRatioValue in usages across the component (locations where
aspectRatio === "native" is repeated, e.g., around VideoEditor render logic and
any functions using those values); ensure the memo's dependencies include
aspectRatio and cropRegion and add handling to update video dimensions (store
dimensions in state or include a derived ref value) so the memo updates after
the video loads.
In `@src/lib/exporter/gifExporter.test.ts`:
- Around line 1-19: Add additional unit tests for calculateOutputDimensions to
cover edge cases: (1) invalid/zero targetAspectRatio — verify it falls back to
source aspect ratio (use GIF_SIZE_PRESETS and the "medium" preset in a test like
"falls back to source aspect ratio when targetAspectRatio is invalid"), (2)
portrait aspect ratios — test with 9/16 to ensure width/height are computed and
evened correctly (e.g., "handles portrait aspect ratios"), (3) the "large"
preset — ensure scaled dimensions match expected large preset behavior, and (4)
source dimensions smaller than a preset's maxHeight — ensure output is clamped
to source bounds. Reference calculateOutputDimensions and GIF_SIZE_PRESETS when
writing each new it() case.
In `@src/lib/exporter/gifExporter.ts`:
- Around line 70-73: The helper function toEven can be simplified by removing
the unnecessary intermediate variable; replace the body of toEven to directly
return Math.max(2, Math.floor(value / 2) * 2) instead of assigning it to
evenValue first so the function becomes a single-return expression while
preserving existing behavior.
- Around line 88-94: The current sizing always uses maxHeight and can upscale
small sources; change the target height computation to clamp to the source
height (e.g., targetHeight = Math.min(maxHeight, sourceHeight or inputHeight))
before computing targetWidth from aspectRatio, then keep using Math.round and
toEven for targetWidth/height; update any references to aspectRatio, maxHeight,
targetHeight, targetWidth and toEven in gifExporter.ts so presets won't upscale
sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 029e9566-0b61-4dcb-999c-fc4b2b268bb1
📒 Files selected for processing (3)
src/components/video-editor/VideoEditor.tsxsrc/lib/exporter/gifExporter.test.tssrc/lib/exporter/gifExporter.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69f1b4d20f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
Pull Request Template
Description
Motivation
Type of Change
Related Issue(s)
#231
Screenshots / Video
Screenshot (if applicable):
Video (if applicable):
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements