[codex] Persist editor defaults and streamline exports#560
Open
Sunwood-ai-labs wants to merge 7 commits into
Open
[codex] Persist editor defaults and streamline exports#560Sunwood-ai-labs wants to merge 7 commits into
Sunwood-ai-labs wants to merge 7 commits into
Conversation
# Conflicts: # electron/electron-env.d.ts # electron/ipc/handlers.ts # electron/preload.ts # src/components/video-editor/VideoEditor.tsx # src/lib/userPreferences.test.ts # src/lib/userPreferences.ts
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds background-image storage + validation; changes pickExportSavePath to accept {autoSaveToDownloads?, exportFolder?} with direct Downloads writes; expands and normalizes UserPreferences; hydrates VideoEditor; makes SettingsPanel custom images controlled; updates locales and tests. ChangesAuto-Save Exports & Expanded Preference Persistence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
🧹 Nitpick comments (3)
electron/ipc/handlers.ts (1)
1004-1047: 💤 Low valueauto-save logic looks solid, but lowkey could be clearer
the control flow works: when
autoSaveToDownloadsis true,targetPathis set to downloads (line 1004) and the dialog block (lines 1006-1047) is skipped entirely. when false, it goes through the dialog flow with exportFolder fallback.one nit: setting
targetPathunconditionally on line 1004 then only conditionally using it feels slightly cursed. might be clearer to set it inside anif/elseso the reader doesn't have to trace the control flow to see that the downloads path is overwritten byresult.filePathwhen the dialog runs.💡 slightly clearer control flow
- let targetPath = path.join(app.getPath("downloads"), fileName); - - if (!options?.autoSaveToDownloads) { + let targetPath: string; + + if (options?.autoSaveToDownloads) { + targetPath = path.join(app.getPath("downloads"), fileName); + } else { const exportFolder = options?.exportFolder; let defaultDir = app.getPath("downloads"); // ... rest of dialog logic ... targetPath = result.filePath; }🤖 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 `@electron/ipc/handlers.ts` around lines 1004 - 1047, The initial unconditional assignment of targetPath to path.join(app.getPath("downloads"), fileName) should be moved into an if/else to make the flow explicit: if options?.autoSaveToDownloads then set targetPath to the downloads path, otherwise run the existing exportFolder stat fallback, call buildDialogOptions and await dialog.showSaveDialog, and set targetPath from result.filePath (or return on cancel). Remove the top-level assignment so targetPath is only assigned in the branch where it is actually used, referencing the existing symbols targetPath, options?.autoSaveToDownloads, exportFolder, defaultDir, buildDialogOptions, and dialog.showSaveDialog.src/components/video-editor/backgroundImageUpload.ts (1)
1-1: 💤 Low valuenit: "image/jpg" isn't a standard MIME type
browsers typically use
image/jpegfor both.jpgand.jpegfiles. the official MIME type isimage/jpeg(notimage/jpg). keeping it here won't break anything since browsers won't send it, but it's kinda misleading.🧹 cleaner MIME type list
-const SUPPORTED_BACKGROUND_IMAGE_TYPES = new Set(["image/jpeg", "image/jpg", "image/png"]); +const SUPPORTED_BACKGROUND_IMAGE_TYPES = new Set(["image/jpeg", "image/png"]);🤖 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/backgroundImageUpload.ts` at line 1, The SUPPORTED_BACKGROUND_IMAGE_TYPES set contains a non-standard MIME "image/jpg"; remove that entry and keep the canonical MIME types ("image/jpeg" and "image/png") so the Set only includes standard types — update the constant SUPPORTED_BACKGROUND_IMAGE_TYPES accordingly (and adjust any tests or comments referencing "image/jpg" if present).src/lib/userPreferences.ts (1)
120-193: ⚡ Quick winnit: normalize helpers leak the DEFAULT_PREFS object refs.
normalizeCropRegion,normalizeWebcamPosition,normalizeCursorHighlight, andnormalizeCustomImagesall hand back the liveDEFAULT_PREFS.*object/array reference on the fallback paths. SinceloadUserPreferences()doesn't deep-clone, anything downstream that mutatescropRegion,cursorHighlight, or pushes intocustomImageswill silently corrupt subsequent reads — lowkey risky given how many callers now consume this.Cheap fix: clone the defaults on return.
♻️ Suggested fix
function normalizeCropRegion(value: unknown): CropRegion { - if (!value || typeof value !== "object") return DEFAULT_PREFS.cropRegion; + if (!value || typeof value !== "object") return { ...DEFAULT_PREFS.cropRegion }; const crop = value as Partial<CropRegion>; ... } function normalizeWebcamPosition(value: unknown): WebcamPosition | null { - if (!value || typeof value !== "object") return DEFAULT_PREFS.webcamPosition; + if (!value || typeof value !== "object") + return DEFAULT_PREFS.webcamPosition ? { ...DEFAULT_PREFS.webcamPosition } : null; const position = value as Partial<WebcamPosition>; return finiteNumber(position.cx) && finiteNumber(position.cy) ? { cx: clamp(position.cx, 0, 1), cy: clamp(position.cy, 0, 1) } - : DEFAULT_PREFS.webcamPosition; + : DEFAULT_PREFS.webcamPosition + ? { ...DEFAULT_PREFS.webcamPosition } + : null; } function normalizeCursorHighlight(value: unknown): CursorHighlightConfig { - if (!value || typeof value !== "object") return DEFAULT_PREFS.cursorHighlight; + if (!value || typeof value !== "object") return { ...DEFAULT_PREFS.cursorHighlight }; ... } function normalizeCustomImages(value: unknown): string[] { - if (!Array.isArray(value)) return DEFAULT_PREFS.customImages; + if (!Array.isArray(value)) return [...DEFAULT_PREFS.customImages]; ... }Or freeze
DEFAULT_PREFSonce at module scope if you'd rather rely on runtime guarantees.🤖 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/lib/userPreferences.ts` around lines 120 - 193, The helpers (normalizeCropRegion, normalizeWebcamPosition, normalizeCursorHighlight, normalizeCustomImages) return live references to DEFAULT_PREFS on fallback paths which can be mutated downstream; change those fallback returns to return clones instead (e.g., object spread or Object.assign for cropRegion/webcamPosition/cursorHighlight and array slice or spread for customImages) so callers get a fresh copy rather than the original DEFAULT_PREFS object/array.
🤖 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 `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1659-1673: The Switch for auto-save in SettingsPanel.tsx has no
accessible name; update the Switch element (the one using
checked={autoSaveExportToDownloads} and
onCheckedChange={onAutoSaveExportToDownloadsChange}) to include an accessible
label by adding either aria-labelledby that points to the label div's id (give
the text container a unique id) or an explicit aria-label/aria-labelledby prop
that mirrors t("export.autoSaveToDownloads"), ensuring screen readers announce
the switch label.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 483-529: The effect in VideoEditor.tsx currently writes full
base64 data URLs from customImages to localStorage via saveUserPreferences on
every change, which will exceed storage quota; update the flow to avoid storing
raw data URLs in localStorage: replace saving raw customImages with either (a)
move image bytes to a persistent storage (preferably via an Electron IPC to save
files under app user-data and persist only file paths/ids), or (b) store images
in IndexedDB and persist only IDs in localStorage, or at minimum
downscale/recompress images before serialization and catch QuotaExceededError in
userPreferences.ts to surface a user toast; modify
SettingsPanel.handleImageUpload to send files to the chosen storage (or perform
compression) and update saveUserPreferences calls in the VideoEditor useEffect
to store only references/IDs (not data URLs), and ensure normalizeCustomImages
still enforces count but no longer assumes data URLs.
- Around line 383-391: Remove the user preference autoSaveExportToDownloads from
the project snapshot construction: when building defaultProjectEditorState
(which spreads defaultEditorState and uses prefs.*), delete the
autoSaveExportToDownloads entry so only true project state fields are included;
update any related initializers that reference prefs.autoSaveExportToDownloads
in the project snapshot path and rely on normalizeProjectEditor to filter it
out—ensure normalizeProjectEditor still validates/filters project-only fields
but do not include prefs.autoSaveExportToDownloads in defaultProjectEditorState
or any snapshot input.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 1004-1047: The initial unconditional assignment of targetPath to
path.join(app.getPath("downloads"), fileName) should be moved into an if/else to
make the flow explicit: if options?.autoSaveToDownloads then set targetPath to
the downloads path, otherwise run the existing exportFolder stat fallback, call
buildDialogOptions and await dialog.showSaveDialog, and set targetPath from
result.filePath (or return on cancel). Remove the top-level assignment so
targetPath is only assigned in the branch where it is actually used, referencing
the existing symbols targetPath, options?.autoSaveToDownloads, exportFolder,
defaultDir, buildDialogOptions, and dialog.showSaveDialog.
In `@src/components/video-editor/backgroundImageUpload.ts`:
- Line 1: The SUPPORTED_BACKGROUND_IMAGE_TYPES set contains a non-standard MIME
"image/jpg"; remove that entry and keep the canonical MIME types ("image/jpeg"
and "image/png") so the Set only includes standard types — update the constant
SUPPORTED_BACKGROUND_IMAGE_TYPES accordingly (and adjust any tests or comments
referencing "image/jpg" if present).
In `@src/lib/userPreferences.ts`:
- Around line 120-193: The helpers (normalizeCropRegion,
normalizeWebcamPosition, normalizeCursorHighlight, normalizeCustomImages) return
live references to DEFAULT_PREFS on fallback paths which can be mutated
downstream; change those fallback returns to return clones instead (e.g., object
spread or Object.assign for cropRegion/webcamPosition/cursorHighlight and array
slice or spread for customImages) so callers get a fresh copy rather than the
original DEFAULT_PREFS object/array.
🪄 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: cddeb15a-4c53-49d1-bb0e-88547d4cfdb0
📒 Files selected for processing (19)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/backgroundImageUpload.test.tssrc/components/video-editor/backgroundImageUpload.tssrc/i18n/locales/ar/settings.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/ja-JP/settings.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/userPreferences.browser.test.tssrc/lib/userPreferences.test.tssrc/lib/userPreferences.ts
Comment on lines
483
to
+529
| useEffect(() => { | ||
| if (!prefsHydrated) return; | ||
| saveUserPreferences({ padding, aspectRatio, exportQuality, exportFormat }); | ||
| }, [prefsHydrated, padding, aspectRatio, exportQuality, exportFormat]); | ||
| saveUserPreferences({ | ||
| wallpaper, | ||
| customImages, | ||
| shadowIntensity, | ||
| showBlur, | ||
| motionBlurAmount, | ||
| borderRadius, | ||
| padding, | ||
| cropRegion, | ||
| aspectRatio, | ||
| webcamLayoutPreset, | ||
| webcamMaskShape, | ||
| webcamSizePreset, | ||
| webcamPosition, | ||
| exportQuality, | ||
| exportFormat, | ||
| autoSaveExportToDownloads, | ||
| gifFrameRate, | ||
| gifLoop, | ||
| gifSizePreset, | ||
| cursorHighlight, | ||
| }); | ||
| }, [ | ||
| prefsHydrated, | ||
| wallpaper, | ||
| customImages, | ||
| shadowIntensity, | ||
| showBlur, | ||
| motionBlurAmount, | ||
| borderRadius, | ||
| padding, | ||
| cropRegion, | ||
| aspectRatio, | ||
| webcamLayoutPreset, | ||
| webcamMaskShape, | ||
| webcamSizePreset, | ||
| webcamPosition, | ||
| exportQuality, | ||
| exportFormat, | ||
| autoSaveExportToDownloads, | ||
| gifFrameRate, | ||
| gifLoop, | ||
| gifSizePreset, | ||
| cursorHighlight, | ||
| ]); |
Contributor
There was a problem hiding this comment.
localStorage quota lowkey gonna bite — customImages are data URLs.
This effect now writes the full customImages array into localStorage on every change, and those entries are base64 data URLs from FileReader.readAsDataURL (see SettingsPanel.handleImageUpload). A handful of typical photos (a few MB each, ~33% bigger after base64) will blow past the 5–10MB localStorage quota pretty quickly. The cap of 20 in normalizeCustomImages bounds count but not bytes.
When that happens, saveUserPreferences silently swallows the QuotaExceededError (the catch in userPreferences.ts), so the user’s wallpaper, GIF prefs, etc. just stop persisting and they’ll never know — kinda cursed.
Worth either:
- Storing uploaded images on disk via an Electron IPC (e.g. user-data dir) and persisting only paths, or
- Caching to IndexedDB and keeping only IDs in localStorage, or
- At minimum, downscaling/recompressing before persisting and surfacing a toast on quota failure.
🤖 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 483 - 529, The
effect in VideoEditor.tsx currently writes full base64 data URLs from
customImages to localStorage via saveUserPreferences on every change, which will
exceed storage quota; update the flow to avoid storing raw data URLs in
localStorage: replace saving raw customImages with either (a) move image bytes
to a persistent storage (preferably via an Electron IPC to save files under app
user-data and persist only file paths/ids), or (b) store images in IndexedDB and
persist only IDs in localStorage, or at minimum downscale/recompress images
before serialization and catch QuotaExceededError in userPreferences.ts to
surface a user toast; modify SettingsPanel.handleImageUpload to send files to
the chosen storage (or perform compression) and update saveUserPreferences calls
in the VideoEditor useEffect to store only references/IDs (not data URLs), and
ensure normalizeCustomImages still enforces count but no longer assumes data
URLs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/ipc/handlers.ts (1)
87-100: 💤 Low valuepath resolution looks secure, error messages kinda generic
the randomUUID base name prevents path traversal, and the validation is solid. one small thing: the error messages on lines 92 and 96 could include the actual type/extension that was rejected for easier debugging.
nit example:
- throw new Error("Unsupported background image type"); + throw new Error(`Unsupported background image type: ${normalizedType}`);not critical tho, current impl works fine.
🤖 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 `@electron/ipc/handlers.ts` around lines 87 - 100, Update resolveBackgroundImageOutputPath to include the actual rejected values in the thrown errors: when checking ALLOWED_BACKGROUND_IMAGE_TYPES use the normalizedType variable in the error message (e.g. "Unsupported background image type: <value>"), and when checking ALLOWED_BACKGROUND_IMAGE_EXTENSIONS include the extension variable in its error message (e.g. "Unsupported background image extension: <value>"); this aids debugging while keeping the same validation logic and use of randomUUID() for the output path.
🤖 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 `@electron/ipc/handlers.ts`:
- Around line 1049-1051: When options?.autoSaveToDownloads is true the code sets
targetPath = path.join(app.getPath("downloads"), fileName) and will silently
overwrite an existing file; change the auto-save branch to check for existing
files (using fs.access or fs.stat) and if the file exists append a numeric
counter to the basename (preserve ext) until a free targetPath is found, then
use that path for writing instead of overwriting; reference the
autoSaveToDownloads check, the targetPath/path.join(fileName) assignment, and
the existing showOverwriteConfirmation logic so you avoid bypassing user
confirmation while preventing silent data loss.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 87-100: Update resolveBackgroundImageOutputPath to include the
actual rejected values in the thrown errors: when checking
ALLOWED_BACKGROUND_IMAGE_TYPES use the normalizedType variable in the error
message (e.g. "Unsupported background image type: <value>"), and when checking
ALLOWED_BACKGROUND_IMAGE_EXTENSIONS include the extension variable in its error
message (e.g. "Unsupported background image extension: <value>"); this aids
debugging while keeping the same validation logic and use of randomUUID() for
the output path.
🪄 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: 4821b8dd-0db1-4164-9034-f032a50cd2af
📒 Files selected for processing (10)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/backgroundImageUpload.test.tssrc/components/video-editor/backgroundImageUpload.tssrc/lib/userPreferences.browser.test.tssrc/lib/userPreferences.test.tssrc/lib/userPreferences.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/video-editor/backgroundImageUpload.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- electron/preload.ts
- src/lib/userPreferences.test.ts
- src/components/video-editor/backgroundImageUpload.test.ts
- src/components/video-editor/SettingsPanel.tsx
- src/components/video-editor/VideoEditor.tsx
- src/lib/userPreferences.ts
…r-defaults-png-autosave # Conflicts: # src/components/video-editor/SettingsPanel.tsx # src/components/video-editor/VideoEditor.tsx
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
Why
Editor defaults and uploaded backgrounds were reset between sessions because only a small subset of preferences was persisted. Video exports also always invoked the OS save dialog, which interrupted workflows that should save directly to Downloads.
Validation
npx biome check electron/ipc/handlers.ts electron/preload.ts electron/electron-env.d.ts src/lib/userPreferences.ts src/lib/userPreferences.test.ts src/lib/userPreferences.browser.test.ts src/components/video-editor/VideoEditor.tsx src/components/video-editor/SettingsPanel.tsx src/components/video-editor/backgroundImageUpload.ts src/components/video-editor/backgroundImageUpload.test.tsnpm run test -- src/components/video-editor/backgroundImageUpload.test.ts src/lib/userPreferences.test.ts src/components/video-editor/projectPersistence.test.ts src/lib/wallpaper.test.tsnpm run test:browser -- src/lib/userPreferences.browser.test.tsnpx tsc --noEmitnpm run build-viteSummary by CodeRabbit
New Features
Improvements
Localization
Tests