feat: add frame rate selector for MP4 export (24/30/60 FPS)#424
Open
Scottlexium wants to merge 1 commit intosiddharthvaddem:mainfrom
Open
feat: add frame rate selector for MP4 export (24/30/60 FPS)#424Scottlexium wants to merge 1 commit intosiddharthvaddem:mainfrom
Scottlexium wants to merge 1 commit intosiddharthvaddem:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds MP4 frame-rate support (24/30/60 fps): new types/validators, UI controls in SettingsPanel, state wired through VideoEditor, persistence/normalization updates, user-preference storage, exporter surface re-exports, and i18n labels in five locales. (50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoEditor.tsx (1)
275-297:⚠️ Potential issue | 🟠 MajorUnsaved-change snapshot is missing
webcamSizePreset(lowkey risky).Line 275+ builds
currentProjectSnapshotwithoutwebcamSizePreset, while project save still includes it (Line 436). This can make unsaved-change detection inaccurate for webcam size changes.🛠️ Proposed fix
return createProjectSnapshot(currentProjectMedia, { wallpaper, shadowIntensity, showBlur, motionBlurAmount, borderRadius, padding, cropRegion, zoomRegions, trimRegions, speedRegions, annotationRegions, aspectRatio, webcamLayoutPreset, webcamMaskShape, + webcamSizePreset, webcamPosition, exportQuality, exportFormat, mp4FrameRate, gifFrameRate, gifLoop, gifSizePreset, }); }, [ currentProjectMedia, wallpaper, shadowIntensity, showBlur, motionBlurAmount, borderRadius, padding, cropRegion, zoomRegions, trimRegions, speedRegions, annotationRegions, aspectRatio, webcamLayoutPreset, webcamMaskShape, + webcamSizePreset, webcamPosition, exportQuality, exportFormat, mp4FrameRate, gifFrameRate, gifLoop, gifSizePreset, ]);Also applies to: 298-321
🤖 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 275 - 297, The unsaved-change snapshot created via createProjectSnapshot is missing the webcamSizePreset field; update the createProjectSnapshot calls (the one building currentProjectSnapshot and the other around lines 298-321) to include webcamSizePreset in the argument object using the same symbol/name used in the project save path so that webcam size changes are included in snapshots and unsaved-change detection stays accurate; verify the variable passed matches the existing project save usage (webcamSizePreset).
🧹 Nitpick comments (3)
src/lib/exporter/types.ts (1)
63-70: Keep MP4 frame-rate values single-sourced (less drift, less 2am pain).Right now Lines 63-67 and Line 70 duplicate the same values. lowkey risky long-term if one list changes and the other doesn’t.
♻️ Suggested cleanup
+const MP4_FRAME_RATE_VALUES = [24, 30, 60] as const; + -export const MP4_FRAME_RATES: { value: Mp4FrameRate; label: string }[] = [ - { value: 24, label: "24 FPS" }, - { value: 30, label: "30 FPS" }, - { value: 60, label: "60 FPS" }, -]; +export const MP4_FRAME_RATES: { value: Mp4FrameRate; label: string }[] = + MP4_FRAME_RATE_VALUES.map((value) => ({ value, label: `${value} FPS` })); // Valid frame rates for validation -export const VALID_MP4_FRAME_RATES: readonly Mp4FrameRate[] = [24, 30, 60] as const; +export const VALID_MP4_FRAME_RATES: readonly Mp4FrameRate[] = MP4_FRAME_RATE_VALUES;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/types.ts` around lines 63 - 70, MP4 frame-rate values are duplicated between MP4_FRAME_RATES and VALID_MP4_FRAME_RATES; to single-source them, extract the numeric constants into one canonical array (e.g., a readonly array of Mp4FrameRate values) and derive both MP4_FRAME_RATES (map to {value,label}) and VALID_MP4_FRAME_RATES from that single array; update references to use the canonical array and keep types as Mp4FrameRate to avoid duplication between the symbols MP4_FRAME_RATES and VALID_MP4_FRAME_RATES.src/lib/userPreferences.ts (1)
1-1: ReuseisValidMp4FrameRateinstead of hardcoding checks again.The inline check works, but it duplicates validation logic. centralizing this keeps behavior aligned with other persistence code.
♻️ Suggested refactor
-import type { ExportFormat, ExportQuality, Mp4FrameRate } from "@/lib/exporter"; +import { + isValidMp4FrameRate, + type ExportFormat, + type ExportQuality, + type Mp4FrameRate, +} from "@/lib/exporter"; @@ mp4FrameRate: - raw.mp4FrameRate === 24 || raw.mp4FrameRate === 30 || raw.mp4FrameRate === 60 + typeof raw.mp4FrameRate === "number" && isValidMp4FrameRate(raw.mp4FrameRate) ? (raw.mp4FrameRate as Mp4FrameRate) : DEFAULT_PREFS.mp4FrameRate,Also applies to: 82-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/userPreferences.ts` at line 1, Replace the duplicated hardcoded MP4 framerate checks with the centralized validator: import and use isValidMp4FrameRate instead of manually comparing values wherever the inline check appears (the current hardcoded check and the similar block later in the file). Update the import line to include isValidMp4FrameRate, remove the manual comparisons, and call isValidMp4FrameRate(Mp4FrameRateCandidate) where Mp4FrameRateCandidate is the value being validated so behavior matches other persistence code and avoids duplication.src/components/video-editor/projectPersistence.ts (1)
1-7: Use shared MP4 FPS validator here too (nit: cleaner, safer).Lines 394-397 re-implement the same check that
isValidMp4FrameRatealready provides. Reusing the shared guard avoids split logic across files.♻️ Suggested refactor
-import type { +import { + isValidMp4FrameRate, + type ExportFormat, ExportQuality, GifFrameRate, GifSizePreset, Mp4FrameRate, } from "@/lib/exporter"; @@ exportFormat: editor.exportFormat === "gif" ? "gif" : "mp4", mp4FrameRate: - editor.mp4FrameRate === 24 || editor.mp4FrameRate === 30 || editor.mp4FrameRate === 60 + typeof editor.mp4FrameRate === "number" && + isValidMp4FrameRate(editor.mp4FrameRate) ? editor.mp4FrameRate : 60,Also applies to: 394-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 1 - 7, The code re-implements MP4 FPS validation instead of reusing the shared guard; import and call the shared validator isValidMp4FrameRate (from the exporter module) in place of the custom check around the MP4 frame rate (the logic currently at lines 394-397), remove the duplicated logic, and ensure the function that sets/validates the MP4 frame rate (e.g., in projectPersistence.ts where ExportFormat/Mp4FrameRate are used) uses isValidMp4FrameRate to gate values and error handling so there’s a single authoritative validation point.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1319-1370: The two selector rows (quality and FPS) lack labels and
mp4Settings.frameRate isn’t shown; add accessible labels above each grid (e.g.,
a small text node or <label> before the first grid for export quality and before
the second for "Frame rate (fps)") and wire them to your i18n keys (or add new
keys) so users know what they control; ensure the MP4 frame-rate selector uses
or displays mp4Settings.frameRate by initializing/reading mp4FrameRate from
mp4Settings.frameRate (or rendering the current mp4Settings.frameRate next to
the label) and keep the existing handlers exportQuality/onExportQualityChange
and mp4FrameRate/onMp4FrameRateChange and MP4_FRAME_RATES intact.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 275-297: The unsaved-change snapshot created via
createProjectSnapshot is missing the webcamSizePreset field; update the
createProjectSnapshot calls (the one building currentProjectSnapshot and the
other around lines 298-321) to include webcamSizePreset in the argument object
using the same symbol/name used in the project save path so that webcam size
changes are included in snapshots and unsaved-change detection stays accurate;
verify the variable passed matches the existing project save usage
(webcamSizePreset).
---
Nitpick comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 1-7: The code re-implements MP4 FPS validation instead of reusing
the shared guard; import and call the shared validator isValidMp4FrameRate (from
the exporter module) in place of the custom check around the MP4 frame rate (the
logic currently at lines 394-397), remove the duplicated logic, and ensure the
function that sets/validates the MP4 frame rate (e.g., in projectPersistence.ts
where ExportFormat/Mp4FrameRate are used) uses isValidMp4FrameRate to gate
values and error handling so there’s a single authoritative validation point.
In `@src/lib/exporter/types.ts`:
- Around line 63-70: MP4 frame-rate values are duplicated between
MP4_FRAME_RATES and VALID_MP4_FRAME_RATES; to single-source them, extract the
numeric constants into one canonical array (e.g., a readonly array of
Mp4FrameRate values) and derive both MP4_FRAME_RATES (map to {value,label}) and
VALID_MP4_FRAME_RATES from that single array; update references to use the
canonical array and keep types as Mp4FrameRate to avoid duplication between the
symbols MP4_FRAME_RATES and VALID_MP4_FRAME_RATES.
In `@src/lib/userPreferences.ts`:
- Line 1: Replace the duplicated hardcoded MP4 framerate checks with the
centralized validator: import and use isValidMp4FrameRate instead of manually
comparing values wherever the inline check appears (the current hardcoded check
and the similar block later in the file). Update the import line to include
isValidMp4FrameRate, remove the manual comparisons, and call
isValidMp4FrameRate(Mp4FrameRateCandidate) where Mp4FrameRateCandidate is the
value being validated so behavior matches other persistence code and avoids
duplication.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5261a401-ddd3-4783-a5e7-4f34f60af836
📒 Files selected for processing (11)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/exporter/index.tssrc/lib/exporter/types.tssrc/lib/userPreferences.ts
Comment on lines
+1319
to
+1370
| <div className="mb-3 space-y-1.5"> | ||
| <div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg"> | ||
| <button | ||
| onClick={() => onExportQualityChange?.("medium")} | ||
| className={cn( | ||
| "rounded-md transition-all text-[10px] font-medium", | ||
| exportQuality === "medium" | ||
| ? "bg-white text-black" | ||
| : "text-slate-400 hover:text-slate-200", | ||
| )} | ||
| > | ||
| {t("exportQuality.low")} | ||
| </button> | ||
| <button | ||
| onClick={() => onExportQualityChange?.("good")} | ||
| className={cn( | ||
| "rounded-md transition-all text-[10px] font-medium", | ||
| exportQuality === "good" | ||
| ? "bg-white text-black" | ||
| : "text-slate-400 hover:text-slate-200", | ||
| )} | ||
| > | ||
| {t("exportQuality.medium")} | ||
| </button> | ||
| <button | ||
| onClick={() => onExportQualityChange?.("source")} | ||
| className={cn( | ||
| "rounded-md transition-all text-[10px] font-medium", | ||
| exportQuality === "source" | ||
| ? "bg-white text-black" | ||
| : "text-slate-400 hover:text-slate-200", | ||
| )} | ||
| > | ||
| {t("exportQuality.high")} | ||
| </button> | ||
| </div> | ||
| <div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg"> | ||
| {MP4_FRAME_RATES.map((rate) => ( | ||
| <button | ||
| key={rate.value} | ||
| onClick={() => onMp4FrameRateChange?.(rate.value)} | ||
| className={cn( | ||
| "rounded-md transition-all text-[10px] font-medium", | ||
| mp4FrameRate === rate.value | ||
| ? "bg-white text-black" | ||
| : "text-slate-400 hover:text-slate-200", | ||
| )} | ||
| > | ||
| {rate.value} | ||
| </button> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
Add labels for the two MP4 selector rows (currently kinda ambiguous).
Line 1319 renders quality and FPS rows back-to-back with no text labels, so it’s not obvious which row controls what. Also, mp4Settings.frameRate is added but not surfaced here.
💡 Suggested tweak
{exportFormat === "mp4" && (
- <div className="mb-3 space-y-1.5">
+ <div className="mb-3 space-y-2">
+ <span className="text-[10px] text-slate-400">{t("exportQuality.title")}</span>
<div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg">
...
</div>
+ <span className="text-[10px] text-slate-400">{t("mp4Settings.frameRate")}</span>
<div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg">
...
</div>
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="mb-3 space-y-1.5"> | |
| <div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg"> | |
| <button | |
| onClick={() => onExportQualityChange?.("medium")} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| exportQuality === "medium" | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {t("exportQuality.low")} | |
| </button> | |
| <button | |
| onClick={() => onExportQualityChange?.("good")} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| exportQuality === "good" | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {t("exportQuality.medium")} | |
| </button> | |
| <button | |
| onClick={() => onExportQualityChange?.("source")} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| exportQuality === "source" | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {t("exportQuality.high")} | |
| </button> | |
| </div> | |
| <div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg"> | |
| {MP4_FRAME_RATES.map((rate) => ( | |
| <button | |
| key={rate.value} | |
| onClick={() => onMp4FrameRateChange?.(rate.value)} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| mp4FrameRate === rate.value | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {rate.value} | |
| </button> | |
| ))} | |
| </div> | |
| <div className="mb-3 space-y-2"> | |
| <span className="text-[10px] text-slate-400">{t("exportQuality.title")}</span> | |
| <div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg"> | |
| <button | |
| onClick={() => onExportQualityChange?.("medium")} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| exportQuality === "medium" | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {t("exportQuality.low")} | |
| </button> | |
| <button | |
| onClick={() => onExportQualityChange?.("good")} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| exportQuality === "good" | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {t("exportQuality.medium")} | |
| </button> | |
| <button | |
| onClick={() => onExportQualityChange?.("source")} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| exportQuality === "source" | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {t("exportQuality.high")} | |
| </button> | |
| </div> | |
| <span className="text-[10px] text-slate-400">{t("mp4Settings.frameRate")}</span> | |
| <div className="bg-white/5 border border-white/5 p-0.5 w-full grid grid-cols-3 h-7 rounded-lg"> | |
| {MP4_FRAME_RATES.map((rate) => ( | |
| <button | |
| key={rate.value} | |
| onClick={() => onMp4FrameRateChange?.(rate.value)} | |
| className={cn( | |
| "rounded-md transition-all text-[10px] font-medium", | |
| mp4FrameRate === rate.value | |
| ? "bg-white text-black" | |
| : "text-slate-400 hover:text-slate-200", | |
| )} | |
| > | |
| {rate.value} | |
| </button> | |
| ))} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/SettingsPanel.tsx` around lines 1319 - 1370, The
two selector rows (quality and FPS) lack labels and mp4Settings.frameRate isn’t
shown; add accessible labels above each grid (e.g., a small text node or <label>
before the first grid for export quality and before the second for "Frame rate
(fps)") and wire them to your i18n keys (or add new keys) so users know what
they control; ensure the MP4 frame-rate selector uses or displays
mp4Settings.frameRate by initializing/reading mp4FrameRate from
mp4Settings.frameRate (or rendering the current mp4Settings.frameRate next to
the label) and keep the existing handlers exportQuality/onExportQualityChange
and mp4FrameRate/onMp4FrameRateChange and MP4_FRAME_RATES intact.
Scottlexium
force-pushed
the
feature/mp4-fps-control
branch
from
8ff583c to
43f915f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoEditor.tsx (1)
275-321:⚠️ Potential issue | 🟠 MajorUnsaved-change snapshot is missing
webcamSizePreset(major correctness bug).Line 275+ snapshot data omits
webcamSizePreset, but save/load includes it. This can make dirty-state detection inaccurate and prompt behavior kinda cursed for projects with non-default webcam size.💡 Proposed fix
return createProjectSnapshot(currentProjectMedia, { wallpaper, shadowIntensity, showBlur, motionBlurAmount, borderRadius, padding, cropRegion, zoomRegions, trimRegions, speedRegions, annotationRegions, aspectRatio, webcamLayoutPreset, webcamMaskShape, + webcamSizePreset, webcamPosition, exportQuality, exportFormat, mp4FrameRate, gifFrameRate, gifLoop, gifSizePreset, }); }, [ currentProjectMedia, wallpaper, shadowIntensity, showBlur, motionBlurAmount, borderRadius, padding, cropRegion, zoomRegions, trimRegions, speedRegions, annotationRegions, aspectRatio, webcamLayoutPreset, webcamMaskShape, + webcamSizePreset, webcamPosition, exportQuality, exportFormat, mp4FrameRate, gifFrameRate, gifLoop, gifSizePreset, ]);🤖 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 275 - 321, The snapshot created by createProjectSnapshot is missing the webcamSizePreset property, causing save/load/dirty-state mismatches; update the object passed to createProjectSnapshot to include webcamSizePreset (alongside wallpaper, webcamPosition, webcamLayoutPreset, etc.) and also add webcamSizePreset to the dependency array for the useMemo/useCallback that returns the snapshot so changes to webcamSizePreset trigger snapshot regeneration; look for createProjectSnapshot(...) and currentProjectMedia to locate the correct block to edit.
♻️ Duplicate comments (1)
src/components/video-editor/SettingsPanel.tsx (1)
1319-1370:⚠️ Potential issue | 🟡 MinorMP4 quality/FPS rows are still unlabeled (same issue as earlier).
Line 1319 still renders two similar selector rows without labels, so it’s not obvious which row is quality vs frame rate.
mp4Settings.frameRateexists but isn’t surfaced here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/SettingsPanel.tsx` around lines 1319 - 1370, The two unlabeled selector rows render exportQuality options and MP4_FRAME_RATES but lack visible labels, so add distinct labels (e.g., "Quality" and "Frame rate") above each selector and surface the existing mp4Settings.frameRate state if needed; update the JSX around exportQuality/ onExportQualityChange and the MP4_FRAME_RATES mapping (where mp4FrameRate and onMp4FrameRateChange are used) to include a small <label> or text element with accessible text tied to each control and ensure the frame-rate label reflects mp4Settings.frameRate if that prop/state exists.
🧹 Nitpick comments (1)
src/components/video-editor/projectPersistence.ts (1)
394-397: Nit: avoid hardcoding MP4 FPS values in multiple places.This works, but it’s a tiny drift risk long-term. lowkey cleaner to validate against a shared exported list (same source as UI options) instead of repeating
24 | 30 | 60here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/projectPersistence.ts` around lines 394 - 397, Replace the hardcoded 24/30/60 check for mp4FrameRate by validating editor.mp4FrameRate against the shared exported FPS list used by the UI (e.g., MP4_FPS_OPTIONS or ALLOWED_MP4_FRAME_RATES) — import that array, use its .includes(editor.mp4FrameRate) to decide validity, and fall back to the existing default (60) if not present; update the mp4FrameRate assignment (the expression referencing editor.mp4FrameRate) to use this centralized list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 275-321: The snapshot created by createProjectSnapshot is missing
the webcamSizePreset property, causing save/load/dirty-state mismatches; update
the object passed to createProjectSnapshot to include webcamSizePreset
(alongside wallpaper, webcamPosition, webcamLayoutPreset, etc.) and also add
webcamSizePreset to the dependency array for the useMemo/useCallback that
returns the snapshot so changes to webcamSizePreset trigger snapshot
regeneration; look for createProjectSnapshot(...) and currentProjectMedia to
locate the correct block to edit.
---
Duplicate comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1319-1370: The two unlabeled selector rows render exportQuality
options and MP4_FRAME_RATES but lack visible labels, so add distinct labels
(e.g., "Quality" and "Frame rate") above each selector and surface the existing
mp4Settings.frameRate state if needed; update the JSX around exportQuality/
onExportQualityChange and the MP4_FRAME_RATES mapping (where mp4FrameRate and
onMp4FrameRateChange are used) to include a small <label> or text element with
accessible text tied to each control and ensure the frame-rate label reflects
mp4Settings.frameRate if that prop/state exists.
---
Nitpick comments:
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 394-397: Replace the hardcoded 24/30/60 check for mp4FrameRate by
validating editor.mp4FrameRate against the shared exported FPS list used by the
UI (e.g., MP4_FPS_OPTIONS or ALLOWED_MP4_FRAME_RATES) — import that array, use
its .includes(editor.mp4FrameRate) to decide validity, and fall back to the
existing default (60) if not present; update the mp4FrameRate assignment (the
expression referencing editor.mp4FrameRate) to use this centralized list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc11721e-4a87-495e-9004-834f2c592df1
📒 Files selected for processing (11)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/lib/exporter/index.tssrc/lib/exporter/types.tssrc/lib/userPreferences.ts
✅ Files skipped from review due to trivial changes (4)
- src/i18n/locales/zh-CN/settings.json
- src/i18n/locales/fr/settings.json
- src/i18n/locales/es/settings.json
- src/i18n/locales/en/settings.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/i18n/locales/tr/settings.json
- src/lib/userPreferences.ts
- src/lib/exporter/types.ts
- src/lib/exporter/index.ts
Adds a 3-button FPS toggle (24 / 30 / 60) to the MP4 export panel, directly below the existing quality selector. The selected frame rate replaces the previously hardcoded 60 FPS value and is persisted to user preferences and project files. - Add Mp4FrameRate type and MP4_FRAME_RATES constant to exporter types - Add mp4FrameRate to UserPreferences and ProjectEditorState - Wire state, persistence, and export config in VideoEditor - Add FPS button row UI in SettingsPanel matching the GIF FPS pattern - Add mp4Settings.frameRate i18n key to all 5 locale files
Scottlexium
force-pushed
the
feature/mp4-fps-control
branch
from
43f915f to
ae471b5
Compare
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.
Description
Adds a user-selectable frame rate option (24 / 30 / 60 FPS) to the MP4 export panel, replacing the previously hardcoded 60 FPS value.
Motivation
Users exporting screencasts or cinematic-style recordings benefit from being able to target 24 FPS (film look) or 30 FPS (broadcast) rather than always exporting at 60 FPS. The selected rate is
persisted to both user preferences and project files so it survives app restarts and project reloads.
Type of Change
Related Issue(s)
N/A
Screenshots / Video
The new FPS toggle (24 / 30 / 60) appears below the quality selector when MP4 format is active, matching the style of the existing GIF frame rate controls.
Testing
Checklist
Summary by CodeRabbit