feat: custom resolution picker#429
Conversation
- Add missing props for custom resolution and export modal - Fix export quality callback type mismatch - Add CustomExportModal component integration
📝 WalkthroughWalkthroughThis PR introduces custom resolution export capability to the video editor. It adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 3
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)
1573-1596:⚠️ Potential issue | 🔴 CriticalMissing
customResolutionin dependency arraylowkey risky -
customResolutionis used inhandleExport(line 1463) but isn't in the dependency array. if user changes the custom resolution and then exports, they might get stale values due to closure capture.🐛 Proposed fix
[ videoPath, webcamVideoPath, wallpaper, zoomRegions, trimRegions, speedRegions, shadowIntensity, showBlur, motionBlurAmount, borderRadius, padding, cropRegion, annotationRegions, isPlaying, aspectRatio, webcamLayoutPreset, webcamMaskShape, webcamSizePreset, webcamPosition, exportQuality, + customResolution, handleExportSaved, cursorTelemetry, ],🤖 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 1573 - 1596, The dependency array for the effect/callback that references handleExport (and ultimately reads customResolution) is missing customResolution, which can cause stale closure values; update the dependency array (the array listing videoPath, webcamVideoPath, ..., cursorTelemetry) to also include customResolution so that handleExport and any effect re-evaluate when customResolution changes, ensuring the exported video uses the latest resolution value.
🧹 Nitpick comments (3)
src/components/video-editor/CustomExportModal.tsx (2)
33-41: Silent failure when validation failsif width or height is < 100,
handleApplyjust... does nothing. user has no idea why clicking Apply didn't work. might want a toast or inline error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/CustomExportModal.tsx` around lines 33 - 41, handleApply silently returns when computed width or height is < 100; add user feedback so the user knows why Apply failed. Update the CustomExportModal component to validate the computed values in handleApply (using tempCustomWidth/tempCustomHeight) and when invalid set an error state (e.g., customResolutionError) or call the existing toast/notification utility, then show that message inline in the modal or as a toast instead of doing nothing; keep the existing success flow that calls onCustomResolutionChange, onExportQualityChange("custom"), and onOpenChange(false) when values are valid.
59-69: Hardcoded English labelsnit: "Width (px)" and "Height (px)" aren't using
t()for i18n. not a blocker but inconsistent with the rest of the modal which does uset("exportQuality.custom").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/CustomExportModal.tsx` around lines 59 - 69, Replace the hardcoded label strings "Width (px)" and "Height (px)" in the CustomExportModal component with i18n calls using the existing translation function (t). Locate the label elements near the inputs that use tempCustomWidth / setTempCustomWidth and tempCustomHeight / setTempCustomHeight and change their text to t("export.width") and t("export.height") (or equivalent keys used in your locale files), ensuring the same translation namespace/keys style as t("exportQuality.custom") used elsewhere in the modal and update locale files with those keys if missing.src/components/video-editor/SettingsPanel.tsx (1)
1574-1576: nit: cleaner type handlingthe cast here is a bit verbose. since
onExportQualityChangealready acceptsExportQuality, you could just do:onExportQualityChange={() => onExportQualityChange?.("custom")}or if you need to keep the callback signature for the child:
onExportQualityChange={onExportQualityChange as (quality: "custom") => void}not a big deal, just reads kinda cursed at first glance.
🤖 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 1574 - 1576, The current prop assignment for onExportQualityChange uses an awkward inline cast that reduces readability; replace the verbose wrapper with a simpler form by either invoking the parent callback with the literal value (call onExportQualityChange?.("custom") inside a zero-arg arrow) or, if preserving the child callback signature is required, cast the prop directly (onExportQualityChange as (quality: "custom") => void); update the occurrence of onExportQualityChange in SettingsPanel.tsx so it delegates cleanly to the existing onExportQualityChange: ExportQuality handler instead of using the complex inline type assertion.
🤖 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/CustomExportModal.tsx`:
- Around line 30-31: The tempCustomWidth/tempCustomHeight local state is only
initialized once so it becomes stale when the prop customResolution changes; add
a useEffect that watches customResolution and calls
setTempCustomWidth(customResolution?.width ?? 1920) and
setTempCustomHeight(customResolution?.height ?? 1080) to sync the inputs
whenever the prop changes (preserve the current defaults if customResolution is
undefined).
In `@src/i18n/locales/en/settings.json`:
- Around line 79-80: The English locale added a new "custom" key under
exportQuality but other locale files (e.g., Spanish, French, Chinese, Korean,
Turkish) are missing it; update every translation file that defines
exportQuality to include the "custom" key with the appropriate translated string
(or a temporary translated placeholder) so all locales mirror the English keys
and prevent fallback text — locate the exportQuality object in each locale JSON
and add the "custom" entry alongside "high" (and any other quality keys).
In `@src/lib/exporter/types.ts`:
- Line 30: The exportQuality validation logic must be updated to accept the new
"custom" variant: update the validation in userPreferences.ts (the function that
checks/sanitizes the exportQuality field, e.g., validateUserPreferences or
similar) so it recognizes "medium" | "good" | "source" | "custom" (matching the
ExportQuality type) instead of only the prior set, and likewise update the
project persistence check in projectPersistence.ts (the routine that sanitizes
project export preferences, e.g., sanitizeProjectPreferences or the
exportQuality validation block) to allow "custom" — ensure both places validate
against all ExportQuality options and only fall back to defaults for truly
unknown values.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1573-1596: The dependency array for the effect/callback that
references handleExport (and ultimately reads customResolution) is missing
customResolution, which can cause stale closure values; update the dependency
array (the array listing videoPath, webcamVideoPath, ..., cursorTelemetry) to
also include customResolution so that handleExport and any effect re-evaluate
when customResolution changes, ensuring the exported video uses the latest
resolution value.
---
Nitpick comments:
In `@src/components/video-editor/CustomExportModal.tsx`:
- Around line 33-41: handleApply silently returns when computed width or height
is < 100; add user feedback so the user knows why Apply failed. Update the
CustomExportModal component to validate the computed values in handleApply
(using tempCustomWidth/tempCustomHeight) and when invalid set an error state
(e.g., customResolutionError) or call the existing toast/notification utility,
then show that message inline in the modal or as a toast instead of doing
nothing; keep the existing success flow that calls onCustomResolutionChange,
onExportQualityChange("custom"), and onOpenChange(false) when values are valid.
- Around line 59-69: Replace the hardcoded label strings "Width (px)" and
"Height (px)" in the CustomExportModal component with i18n calls using the
existing translation function (t). Locate the label elements near the inputs
that use tempCustomWidth / setTempCustomWidth and tempCustomHeight /
setTempCustomHeight and change their text to t("export.width") and
t("export.height") (or equivalent keys used in your locale files), ensuring the
same translation namespace/keys style as t("exportQuality.custom") used
elsewhere in the modal and update locale files with those keys if missing.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1574-1576: The current prop assignment for onExportQualityChange
uses an awkward inline cast that reduces readability; replace the verbose
wrapper with a simpler form by either invoking the parent callback with the
literal value (call onExportQualityChange?.("custom") inside a zero-arg arrow)
or, if preserving the child callback signature is required, cast the prop
directly (onExportQualityChange as (quality: "custom") => void); update the
occurrence of onExportQualityChange in SettingsPanel.tsx so it delegates cleanly
to the existing onExportQualityChange: ExportQuality handler instead of using
the complex inline type assertion.
🪄 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: d5258c09-884d-4196-ab61-4d1668984f85
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
src/components/video-editor/CustomExportModal.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/i18n/locales/en/settings.jsonsrc/lib/exporter/index.tssrc/lib/exporter/types.ts
| const [tempCustomWidth, setTempCustomWidth] = useState(customResolution?.width || 1920); | ||
| const [tempCustomHeight, setTempCustomHeight] = useState(customResolution?.height || 1080); |
There was a problem hiding this comment.
Stale state when modal reopens with different customResolution
the useState initializers only run on mount. if user opens modal, sets 1280x720, closes, then you change customResolution externally and reopen - the inputs still show 1280x720 instead of the new value.
you probably want a useEffect to sync when the prop changes:
🔧 Proposed fix
+import { useState, useEffect } from "react";
const [tempCustomWidth, setTempCustomWidth] = useState(customResolution?.width || 1920);
const [tempCustomHeight, setTempCustomHeight] = useState(customResolution?.height || 1080);
+useEffect(() => {
+ if (open) {
+ setTempCustomWidth(customResolution?.width || 1920);
+ setTempCustomHeight(customResolution?.height || 1080);
+ }
+}, [open, customResolution]);📝 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.
| const [tempCustomWidth, setTempCustomWidth] = useState(customResolution?.width || 1920); | |
| const [tempCustomHeight, setTempCustomHeight] = useState(customResolution?.height || 1080); | |
| import { useState, useEffect } from "react"; | |
| const [tempCustomWidth, setTempCustomWidth] = useState(customResolution?.width || 1920); | |
| const [tempCustomHeight, setTempCustomHeight] = useState(customResolution?.height || 1080); | |
| useEffect(() => { | |
| if (open) { | |
| setTempCustomWidth(customResolution?.width || 1920); | |
| setTempCustomHeight(customResolution?.height || 1080); | |
| } | |
| }, [open, customResolution]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/CustomExportModal.tsx` around lines 30 - 31, The
tempCustomWidth/tempCustomHeight local state is only initialized once so it
becomes stale when the prop customResolution changes; add a useEffect that
watches customResolution and calls setTempCustomWidth(customResolution?.width ??
1920) and setTempCustomHeight(customResolution?.height ?? 1080) to sync the
inputs whenever the prop changes (preserve the current defaults if
customResolution is undefined).
| "high": "High", | ||
| "custom": "Custom" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all locale files for exportQuality.custom key
echo "=== Checking all locale files for 'custom' key in exportQuality ==="
fd -t f "settings.json" src/i18n/locales --exec grep -l "exportQuality" {} \; | while read f; do
echo "--- $f ---"
grep -A6 '"exportQuality"' "$f"
doneRepository: siddharthvaddem/openscreen
Length of output: 1145
Missing translation across all locales
the english locale got the new "custom" key in exportQuality, but it's missing from literally every other locale - spanish, french, chinese, korean, turkish all need it. this is gonna cause fallback text for like 90% of your users, lowkey risky for a export feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/i18n/locales/en/settings.json` around lines 79 - 80, The English locale
added a new "custom" key under exportQuality but other locale files (e.g.,
Spanish, French, Chinese, Korean, Turkish) are missing it; update every
translation file that defines exportQuality to include the "custom" key with the
appropriate translated string (or a temporary translated placeholder) so all
locales mirror the English keys and prevent fallback text — locate the
exportQuality object in each locale JSON and add the "custom" entry alongside
"high" (and any other quality keys).
| } | ||
|
|
||
| export type ExportQuality = "medium" | "good" | "source"; | ||
| export type ExportQuality = "medium" | "good" | "source" | "custom"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the userPreferences.ts validation logic
rg -n -A5 "exportQuality:" src/lib/userPreferences.tsRepository: siddharthvaddem/openscreen
Length of output: 617
🏁 Script executed:
# First, verify "custom" is actually in the ExportQuality type
cat -n src/lib/exporter/types.ts | head -40Repository: siddharthvaddem/openscreen
Length of output: 1217
🏁 Script executed:
# Check if there are other places validating or checking exportQuality values
rg -n "exportQuality.*==|exportQuality.*===|ExportQuality" src/ --type tsRepository: siddharthvaddem/openscreen
Length of output: 3098
🏁 Script executed:
# Check the context of the validation in projectPersistence.ts
sed -n '450,460p' src/components/video-editor/projectPersistence.tsRepository: siddharthvaddem/openscreen
Length of output: 482
Adding "custom" breaks user preference validation in two places
heads up - both userPreferences.ts (lines 70-72) and projectPersistence.ts (lines 454-456) have incomplete validation for exportQuality. when a user picks custom quality, saves preferences/project, and reloads, it'll silently revert to defaults. lowkey risky for user experience.
need to update userPreferences.ts validation:
userPreferences.ts fix
exportQuality:
raw.exportQuality === "medium" ||
raw.exportQuality === "good" ||
- raw.exportQuality === "source"
+ raw.exportQuality === "source" ||
+ raw.exportQuality === "custom"
? (raw.exportQuality as ExportQuality)
: DEFAULT_PREFS.exportQuality,also fix projectPersistence.ts - the current check only validates "medium" || "source" and silently reverts "custom" to "good". update it to handle all quality options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/exporter/types.ts` at line 30, The exportQuality validation logic
must be updated to accept the new "custom" variant: update the validation in
userPreferences.ts (the function that checks/sanitizes the exportQuality field,
e.g., validateUserPreferences or similar) so it recognizes "medium" | "good" |
"source" | "custom" (matching the ExportQuality type) instead of only the prior
set, and likewise update the project persistence check in projectPersistence.ts
(the routine that sanitizes project export preferences, e.g.,
sanitizeProjectPreferences or the exportQuality validation block) to allow
"custom" — ensure both places validate against all ExportQuality options and
only fall back to defaults for truly unknown values.
![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: 09114a48d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| export type ExportQuality = "medium" | "good" | "source"; | ||
| export type ExportQuality = "medium" | "good" | "source" | "custom"; |
There was a problem hiding this comment.
After adding "custom" to ExportQuality, the deserializers still reject it: normalizeProjectEditor in src/components/video-editor/projectPersistence.ts (lines 453-456) coerces anything other than "medium"/"source" to "good", and loadUserPreferences in src/lib/userPreferences.ts (lines 69-74) only accepts "medium"|"good"|"source". This means users who choose Custom will lose that mode (and thus their custom dimensions) after reopening a project or restarting the app, causing exports to silently revert to preset resolutions.
Useful? React with 👍 / 👎.
| } else if (quality === "custom" && customResolution) { | ||
| // Use custom resolution | ||
| exportWidth = Math.floor(customResolution.width / 2) * 2; | ||
| exportHeight = Math.floor(customResolution.height / 2) * 2; |
There was a problem hiding this comment.
handleExport now reads customResolution for quality === "custom", but customResolution is not in that useCallback dependency list. If the user edits custom width/height while quality stays "custom", the callback can keep the previous captured resolution and export with stale dimensions. Adding customResolution to the dependencies (or reading from a ref) is needed so each Apply uses the latest values.
Useful? React with 👍 / 👎.
|
@siddharthvaddem Please take a look at this feature |
1 participant
Description
Added a custom resolution selector modal that allows users to specify exact width and height for video exports, giving more flexibility beyond the preset quality options (low, medium, high, source).
Motivation
Users need fine-grained control over export resolution. The existing preset options don't accommodate specific use cases like creating videos for particular display sizes or social media platforms with specific dimension requirements.
Type of Change
Changes Made
Screenshots
Related Issue(s)
#227
Testing
Checklist
Thank you for contributing!
Summary by CodeRabbit