Fix/remember export folder#519
Conversation
The save dialog now opens in the last folder the user exported to, instead of always defaulting to Downloads. The folder is persisted in userPreferences (localStorage) and passed through the IPC layer to the Electron save dialog as defaultPath. Closes siddharthvaddem#503
Addresses code review feedback: the previous substring/lastIndexOf approach would store empty string as export folder if saved to root directory. path.dirname in the Electron handler handles edge cases correctly and is returned as a new dir field in the IPC response.
📝 WalkthroughWalkthroughAdded "remember last export folder" feature by extending the export API to accept and return an optional folder path. Changes persist the export folder in user preferences, hydrate it on load, pass it through the IPC layer, and update UI state when the user selects a new location. ChangesExport Folder Persistence Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The logic is pretty straightforward—no wild branching or edge cases—but you've got a coherent pattern threaded through 5 different files across electron and react layers. The IPC handler's baseFolder selection and the consistent dir-extraction pattern in three export flows are solid, though you'll wanna spot-check that Possibly related PRs
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. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
🧹 Nitpick comments (1)
src/components/video-editor/VideoEditor.tsx (1)
1417-1427: 💤 Low valueBoth export paths correctly wired; dep array is complete
exportFolderis passed to both GIF and MP4saveExportedVideocalls and updated fromsaveResult.diron success. The dep array at line 1629 ensureshandleExportcaptures the latest folder after each export.
gifFrameRate/gifLoop/gifSizePresetare correctly absent fromhandleExport's dep array — they flow in via thesettingsparameter fromhandleOpenExportDialog, not from the closure directly.One nit: the
if (saveResult.dir) setExportFolder(saveResult.dir)guard appears identically in three places (lines 1319, 1427, 1572). Extracting it into a tiny inline helper would make future changes to the post-save behavior a one-line edit rather than three — totally optional, but kinda nice to have.♻️ Optional — extract post-save folder update
+ const handleExportFolderUpdate = useCallback((dir?: string) => { + if (dir) setExportFolder(dir); + }, []);Then replace all three occurrences of:
- if (saveResult.dir) setExportFolder(saveResult.dir); + handleExportFolderUpdate(saveResult.dir);Also applies to: 1562-1572, 1608-1634
🤖 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 1417 - 1427, The post-save folder-update guard "if (saveResult.dir) setExportFolder(saveResult.dir)" is duplicated in multiple places (within the GIF/MP4 saveResult handling). Create a small inline helper function (e.g., applySaveResultDir or updateExportFolderFromSave) that accepts saveResult, checks saveResult.dir, and calls setExportFolder(saveResult.dir) when present, then replace each duplicated guard in the handlers that call window.electronAPI.saveExportedVideo with a single call to that helper; reference the existing setExportFolder and saveResult symbols so the change is localized and behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Line 835: The assignment to baseFolder uses nullish coalescing (const
baseFolder = exportFolder ?? app.getPath("downloads")) which doesn't guard
against an empty string and can make path.join("", fileName) resolve relative;
change the check to treat empty string as falsy (e.g., use || or an explicit
empty-string check) so that when exportFolder is "" the fallback
app.getPath("downloads") is used; update the line referencing exportFolder and
baseFolder (and any subsequent path.join usage) to rely on the corrected
baseFolder value.
---
Nitpick comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1417-1427: The post-save folder-update guard "if (saveResult.dir)
setExportFolder(saveResult.dir)" is duplicated in multiple places (within the
GIF/MP4 saveResult handling). Create a small inline helper function (e.g.,
applySaveResultDir or updateExportFolderFromSave) that accepts saveResult,
checks saveResult.dir, and calls setExportFolder(saveResult.dir) when present,
then replace each duplicated guard in the handlers that call
window.electronAPI.saveExportedVideo with a single call to that helper;
reference the existing setExportFolder and saveResult symbols so the change is
localized and behavior remains identical.
🪄 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: e4a36174-133c-4227-9307-50b67742279d
📒 Files selected for processing (5)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/components/video-editor/VideoEditor.tsxsrc/lib/userPreferences.ts
| message: "Export canceled", | ||
| }; | ||
| } | ||
| const baseFolder = exportFolder ?? app.getPath("downloads"); |
There was a problem hiding this comment.
?? won't protect against empty string exportFolder
nullish coalescing only catches null/undefined — an empty "" sneaks right through. path.join("", fileName) resolves to a relative path, so the save dialog would open at the process CWD instead of Downloads on that edge case.
The userPreferences.ts validation already guards against "" in storage, but the IPC boundary itself has no such check. Swapping to || closes the gap for zero cost.
🛡️ Proposed fix
- const baseFolder = exportFolder ?? app.getPath("downloads");
+ const baseFolder = exportFolder || app.getPath("downloads");📝 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 baseFolder = exportFolder ?? app.getPath("downloads"); | |
| const baseFolder = exportFolder || app.getPath("downloads"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` at line 835, The assignment to baseFolder uses
nullish coalescing (const baseFolder = exportFolder ?? app.getPath("downloads"))
which doesn't guard against an empty string and can make path.join("", fileName)
resolve relative; change the check to treat empty string as falsy (e.g., use ||
or an explicit empty-string check) so that when exportFolder is "" the fallback
app.getPath("downloads") is used; update the line referencing exportFolder and
baseFolder (and any subsequent path.join usage) to rely on the corrected
baseFolder value.
|
Already resolved by other user: #512 |
1 participant
Closes #503
The save dialog now opens in the last folder the user exported to, instead of always defaulting to Downloads.
What changed
exportFoldertoUserPreferencesso the last used folder persists across sessionsdefaultPathto Electron's save dialogpath.dirnamein the Electron handler for correct edge case handling (root paths etc.)Test plan
Summary by CodeRabbit