feat: add YouTube Shorts export preset#806
Closed
kseungyong wants to merge 2 commits into
Closed
Conversation
Adds optional projectType discriminator on TProjectMetadata so downstream features (preset pickers, export defaults, UI affordances) can branch on intent without inferring from canvas dimensions. - New const PROJECT_TYPES and type TProjectType - TProjectMetadata.projectType (optional for back-compat with stored projects) - ProjectManager.createNewProject accepts optional projectType, defaults to 'standard' - Unit tests for PROJECT_TYPES invariants
|
@kseungyong is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAdds a TProjectType system ("standard" | "shorts"), export presets with a YouTube Shorts preset, a getDefaultExportOptions selector, tests for types and presets, ProjectManager storage of projectType, and Export UI defaulting to project-specific export options. ChangesProject Type and Export Presets
Sequence DiagramsequenceDiagram
participant User
participant ExportPopover
participant ProjectManager
participant Defaults as getDefaultExportOptions
participant Presets as EXPORT_PRESETS
User->>ExportPopover: open export UI (activeProject)
ExportPopover->>ProjectManager: read activeProject.metadata.projectType
ExportPopover->>Defaults: getDefaultExportOptions({ projectType })
Defaults->>Presets: select preset by id ("shorts" -> YOUTUBE_SHORTS_EXPORT_PRESET.options)
Presets-->>Defaults: preset ExportOptions
Defaults-->>ExportPopover: return ExportOptions
ExportPopover-->>User: initialize UI with format/quality/includeAudio
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
apps/web/src/export/__tests__/presets.test.ts (1)
56-70: ⚡ Quick winAdd a regression test for reference isolation in defaults.
These tests assert value equality, but not that each call returns a fresh object. Add
not.toBe(...)assertions to prevent shared-mutation regressions.Suggested test addition
describe("getDefaultExportOptions", () => { + test("returns fresh objects (no shared references)", () => { + const shortsA = getDefaultExportOptions({ projectType: "shorts" }); + const shortsB = getDefaultExportOptions({ projectType: "shorts" }); + expect(shortsA).not.toBe(shortsB); + expect(shortsA).not.toBe(YOUTUBE_SHORTS_EXPORT_PRESET.options); + + const standardA = getDefaultExportOptions({ projectType: "standard" }); + const standardB = getDefaultExportOptions({ projectType: "standard" }); + expect(standardA).not.toBe(standardB); + expect(standardA).not.toBe(DEFAULT_EXPORT_OPTIONS); + });🤖 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 `@apps/web/src/export/__tests__/presets.test.ts` around lines 56 - 70, The tests for getDefaultExportOptions currently only check deep equality and miss verifying that each call returns a fresh object; add identity assertions using not.toBe to each test so they assert that the returned object is not the same reference as the presets/constants (e.g., in the "shorts" case assert opts not.toBe(YOUTUBE_SHORTS_EXPORT_PRESET.options) and for "standard" and undefined cases assert opts not.toBe(DEFAULT_EXPORT_OPTIONS)), ensuring getDefaultExportOptions returns isolated copies rather than shared references.
🤖 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.
Inline comments:
In `@apps/web/src/export/defaults.ts`:
- Around line 22-25: The function currently returns shared singleton objects
(YOUTUBE_SHORTS_EXPORT_PRESET.options and DEFAULT_EXPORT_OPTIONS), which allows
callers to mutate global presets; change the return to provide fresh copies
instead—e.g., return a cloned object instead of the original reference (use
structuredClone(YOUTUBE_SHORTS_EXPORT_PRESET.options) or
JSON.parse(JSON.stringify(...)) or a shallow copy like
{...DEFAULT_EXPORT_OPTIONS} depending on nested structure) so
DEFAULT_EXPORT_OPTIONS and YOUTUBE_SHORTS_EXPORT_PRESET.options are not mutated
by callers.
---
Nitpick comments:
In `@apps/web/src/export/__tests__/presets.test.ts`:
- Around line 56-70: The tests for getDefaultExportOptions currently only check
deep equality and miss verifying that each call returns a fresh object; add
identity assertions using not.toBe to each test so they assert that the returned
object is not the same reference as the presets/constants (e.g., in the "shorts"
case assert opts not.toBe(YOUTUBE_SHORTS_EXPORT_PRESET.options) and for
"standard" and undefined cases assert opts not.toBe(DEFAULT_EXPORT_OPTIONS)),
ensuring getDefaultExportOptions returns isolated copies rather than shared
references.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 121393d9-299a-4044-be12-4e0f14323184
📒 Files selected for processing (6)
apps/web/src/core/managers/project-manager.tsapps/web/src/export/__tests__/presets.test.tsapps/web/src/export/defaults.tsapps/web/src/export/index.tsapps/web/src/project/__tests__/project-type.test.tsapps/web/src/project/types.ts
Adds an export preset registry and the first preset entry — YouTube Shorts — so users with projectType='shorts' get sensible export defaults aligned with YouTube's Shorts encoding recommendations.
- New types TExportPresetId and TExportPreset
- YOUTUBE_SHORTS_EXPORT_PRESET (mp4, very_high quality, audio on)
- EXPORT_PRESETS registry + getExportPresetById helper
- getDefaultExportOptions({projectType}) resolves Shorts vs standard defaults
- DEFAULT_EXPORT_OPTIONS export preserved for back-compat
- Unit tests cover preset shape, registry exhaustiveness, default resolution
kseungyong
force-pushed
the
feat/youtube-shorts-export-preset
branch
from
ad0185b to
5913abb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@apps/web/src/components/editor/export-button.tsx`:
- Around line 104-111: The export state (format, quality, shouldIncludeAudio) is
initialized from getDefaultExportOptions once and becomes stale when switching
projects because ExportPopover stays mounted; force a remount on project change
by adding a stable key prop tied to the active project (e.g., use
activeProject?.metadata?.projectType or activeProject?.id) on the ExportPopover
(or the parent component that renders the export-button UI) so the component
fully reinitializes and re-runs the useState initializers; alternatively, if you
prefer not to remount, implement a useEffect inside export-button.tsx that
watches activeProject (or activeProject.metadata.projectType) and resets
setFormat, setQuality, and setShouldIncludeAudio from getDefaultExportOptions
when it changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfa94bd4-0b27-4f40-a727-93b3130184ed
📒 Files selected for processing (4)
apps/web/src/components/editor/export-button.tsxapps/web/src/export/__tests__/presets.test.tsapps/web/src/export/defaults.tsapps/web/src/export/index.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/export/tests/presets.test.ts
Comment on lines
+104
to
111
| const initialOptions = getDefaultExportOptions({ | ||
| projectType: activeProject?.metadata?.projectType, | ||
| }); | ||
| const [format, setFormat] = useState<ExportFormat>(initialOptions.format); | ||
| const [quality, setQuality] = useState<ExportQuality>(initialOptions.quality); | ||
| const [shouldIncludeAudio, setShouldIncludeAudio] = useState<boolean>( | ||
| DEFAULT_EXPORT_OPTIONS.includeAudio ?? true, | ||
| initialOptions.includeAudio ?? true, | ||
| ); |
Contributor
There was a problem hiding this comment.
Export defaults become stale when switching projects without remounting.
useState only consumes initialOptions on the first render. If the user switches to a different active project while ExportPopover stays mounted (line 90 keeps it mounted when hasProject is true), the format/quality/audio states retain values from the previous project instead of reflecting the new project's projectType.
Example: User opens a "shorts" project (gets very_high quality) → closes dialog → switches to "standard" project → reopens dialog → still sees very_high instead of high.
🔑 Recommended fix: Add a key prop to force remount on project change
- {hasProject && <ExportPopover onOpenChange={setIsExportPopoverOpen} />}
+ {hasProject && <ExportPopover key={activeProject.id} onOpenChange={setIsExportPopoverOpen} />}Alternatively, replace the useState initialization with useEffect to sync when activeProject changes, though the key-based remount is simpler and ensures full component reset.
🤖 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 `@apps/web/src/components/editor/export-button.tsx` around lines 104 - 111, The
export state (format, quality, shouldIncludeAudio) is initialized from
getDefaultExportOptions once and becomes stale when switching projects because
ExportPopover stays mounted; force a remount on project change by adding a
stable key prop tied to the active project (e.g., use
activeProject?.metadata?.projectType or activeProject?.id) on the ExportPopover
(or the parent component that renders the export-button UI) so the component
fully reinitializes and re-runs the useState initializers; alternatively, if you
prefer not to remount, implement a useEffect inside export-button.tsx that
watches activeProject (or activeProject.metadata.projectType) and resets
setFormat, setQuality, and setShouldIncludeAudio from getDefaultExportOptions
when it changes.
Author
|
Closing — continuing on a private fork. The branch |
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.
Summary
Introduces a small export-preset registry and the first preset — `youtube-shorts` — so a project with `projectType === "shorts"` can resolve sensible export defaults without the UI hard-coding them.
Changes
Why preset-level (not bitrate-level) in this PR
YouTube Shorts encoding recommends 8–12 Mbps for 1080p30 and 12–20 Mbps for 1080p60. The mapping from `quality: "very_high"` to an actual bitrate lives in the `mediabunny` package (imported in `scene-exporter.ts` as `QUALITY_VERY_HIGH`), not in `export/` TS. This PR sets the right `ExportOptions` shape so the existing quality→bitrate path produces near-target rates. A follow-up PR can introduce explicit bitrate overrides if measured output deviates from YouTube's recommendation.
Depends On
#804 (the `projectType` field PR) — `getDefaultExportOptions` switches on `TProjectType`. The base of this branch is `feat/project-type-field`, so PR2's commit is included. After #804 merges to main, this PR can rebase onto main.
Test Plan
Summary by CodeRabbit
New Features
Behavior Changes
Tests