feat(desktop): custom recordings storage path#1964
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn recordings_dir(app: &AppHandle<Wry>) -> std::path::PathBuf { | ||
| let custom = Self::get(app) | ||
| .ok() | ||
| .flatten() | ||
| .and_then(|s| s.recordings_path) |
There was a problem hiding this comment.
Self::get(app) returns an Err (e.g. the settings store is corrupted or fails to deserialize), .ok() silently converts it to None and the custom path is dropped — the user's choice is ignored without any indication in the logs. Adding a tracing::warn! here makes the silent fallback visible when diagnosing "why isn't my custom folder being used?"
| pub fn recordings_dir(app: &AppHandle<Wry>) -> std::path::PathBuf { | |
| let custom = Self::get(app) | |
| .ok() | |
| .flatten() | |
| .and_then(|s| s.recordings_path) | |
| pub fn recordings_dir(app: &AppHandle<Wry>) -> std::path::PathBuf { | |
| let custom = Self::get(app) | |
| .map_err(|e| tracing::warn!("Failed to load general settings, using default recordings path: {e}")) | |
| .ok() | |
| .flatten() | |
| .and_then(|s| s.recordings_path) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/general_settings.rs
Line: 334-338
Comment:
When `Self::get(app)` returns an `Err` (e.g. the settings store is corrupted or fails to deserialize), `.ok()` silently converts it to `None` and the custom path is dropped — the user's choice is ignored without any indication in the logs. Adding a `tracing::warn!` here makes the silent fallback visible when diagnosing "why isn't my custom folder being used?"
```suggestion
pub fn recordings_dir(app: &AppHandle<Wry>) -> std::path::PathBuf {
let custom = Self::get(app)
.map_err(|e| tracing::warn!("Failed to load general settings, using default recordings path: {e}"))
.ok()
.flatten()
.and_then(|s| s.recordings_path)
```
How can I resolve this? If you propose a fix, please make it concise.| app.dialog() | ||
| .file() | ||
| .set_title("Choose Recordings Folder") | ||
| .pick_folder(move |path| { | ||
| let _ = tx.send( | ||
| path.as_ref() | ||
| .and_then(|p| p.as_path()) | ||
| .map(|p| p.to_string_lossy().to_string()), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
to_string_lossy() replaces invalid UTF-8 bytes with \u{FFFD}. On Windows, if the user picks a folder whose path cannot round-trip through UTF-8, the stored string is silently corrupted; on the next launch recordings_dir will try to create a directory with \u{FFFD} in its name and may silently fall through to the default. Prefer to_str().map(|s| s.to_string()) and surface the error instead.
| app.dialog() | |
| .file() | |
| .set_title("Choose Recordings Folder") | |
| .pick_folder(move |path| { | |
| let _ = tx.send( | |
| path.as_ref() | |
| .and_then(|p| p.as_path()) | |
| .map(|p| p.to_string_lossy().to_string()), | |
| ); | |
| }); | |
| app.dialog() | |
| .file() | |
| .set_title("Choose Recordings Folder") | |
| .pick_folder(move |path| { | |
| let _ = tx.send( | |
| path.as_ref() | |
| .and_then(|p| p.as_path()) | |
| .and_then(|p| p.to_str().map(|s| s.to_string())), | |
| ); | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 4265-4274
Comment:
`to_string_lossy()` replaces invalid UTF-8 bytes with `\u{FFFD}`. On Windows, if the user picks a folder whose path cannot round-trip through UTF-8, the stored string is silently corrupted; on the next launch `recordings_dir` will try to create a directory with `\u{FFFD}` in its name and may silently fall through to the default. Prefer `to_str().map(|s| s.to_string())` and surface the error instead.
```suggestion
app.dialog()
.file()
.set_title("Choose Recordings Folder")
.pick_folder(move |path| {
let _ = tx.send(
path.as_ref()
.and_then(|p| p.as_path())
.and_then(|p| p.to_str().map(|s| s.to_string())),
);
});
```
How can I resolve this? If you propose a fix, please make it concise.| <StorageSection | ||
| recordingsPath={settings.recordingsPath ?? null} | ||
| onPick={async () => { | ||
| const path = await commands.pickRecordingsFolder(); | ||
| if (path !== null) { | ||
| setSettings("recordingsPath", path); | ||
| } | ||
| }} | ||
| onReset={async () => { | ||
| await commands.resetRecordingsFolder(); | ||
| setSettings("recordingsPath", null); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
onPick and onReset are async but have no try/catch. If the Tauri command throws (e.g. the store write fails), the rejection goes unhandled, setSettings is never called, and the user gets no feedback. A minimal console.error at minimum would surface the failure for debugging.
| <StorageSection | |
| recordingsPath={settings.recordingsPath ?? null} | |
| onPick={async () => { | |
| const path = await commands.pickRecordingsFolder(); | |
| if (path !== null) { | |
| setSettings("recordingsPath", path); | |
| } | |
| }} | |
| onReset={async () => { | |
| await commands.resetRecordingsFolder(); | |
| setSettings("recordingsPath", null); | |
| }} | |
| /> | |
| <StorageSection | |
| recordingsPath={settings.recordingsPath ?? null} | |
| onPick={async () => { | |
| try { | |
| const path = await commands.pickRecordingsFolder(); | |
| if (path !== null) { | |
| setSettings("recordingsPath", path); | |
| } | |
| } catch (e) { | |
| console.error("Failed to pick recordings folder", e); | |
| } | |
| }} | |
| onReset={async () => { | |
| try { | |
| await commands.resetRecordingsFolder(); | |
| setSettings("recordingsPath", null); | |
| } catch (e) { | |
| console.error("Failed to reset recordings folder", e); | |
| } | |
| }} | |
| /> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/general.tsx
Line: 642-654
Comment:
Both `onPick` and `onReset` are async but have no `try/catch`. If the Tauri command throws (e.g. the store write fails), the rejection goes unhandled, `setSettings` is never called, and the user gets no feedback. A minimal `console.error` at minimum would surface the failure for debugging.
```suggestion
<StorageSection
recordingsPath={settings.recordingsPath ?? null}
onPick={async () => {
try {
const path = await commands.pickRecordingsFolder();
if (path !== null) {
setSettings("recordingsPath", path);
}
} catch (e) {
console.error("Failed to pick recordings folder", e);
}
}}
onReset={async () => {
try {
await commands.resetRecordingsFolder();
setSettings("recordingsPath", null);
} catch (e) {
console.error("Failed to reset recordings folder", e);
}
}}
/>
```
How can I resolve this? If you propose a fix, please make it concise.![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let path = custom.unwrap_or_else(|| { | ||
| app.path().app_data_dir().unwrap().join("recordings") | ||
| }); | ||
| std::fs::create_dir_all(&path).unwrap_or_default(); |
There was a problem hiding this comment.
Swallowing create_dir_all errors makes it hard to diagnose why recording later fails (especially for a custom path). Logging the error here would help a lot.
| std::fs::create_dir_all(&path).unwrap_or_default(); | |
| if let Err(e) = std::fs::create_dir_all(&path) { | |
| tracing::warn!(?path, %e, "Failed to create recordings directory"); | |
| } |
| }); | ||
|
|
||
| let result = rx.await.map_err(|e| e.to_string())?; | ||
| if let Some(ref path) = result { |
There was a problem hiding this comment.
Minor edge case: recordings_dir() ignores non-absolute stored paths, but this command will persist whatever string comes back. Validating before saving avoids a confusing "UI shows custom path but app uses default" mismatch (and lets you surface errors early).
| if let Some(ref path) = result { | |
| if let Some(ref path) = result { | |
| let path_buf = std::path::PathBuf::from(path); | |
| if !path_buf.is_absolute() { | |
| return Err("Picked recordings folder must be an absolute path".to_string()); | |
| } | |
| std::fs::create_dir_all(&path_buf) | |
| .map_err(|e| format!("Failed to create recordings folder '{path}': {e}"))?; | |
| general_settings::GeneralSettingsStore::update(&app, |s| { | |
| s.recordings_path = Some(path.clone()); | |
| })?; | |
| } |
|
|
||
| <StorageSection | ||
| recordingsPath={settings.recordingsPath ?? null} | ||
| onPick={async () => { |
There was a problem hiding this comment.
Worth adding a small try/catch here so a failed invoke (store write error, dialog failure, etc) doesn’t become an unhandled rejection. Since this page already imports toast, surfacing the error is pretty cheap.
| onPick={async () => { | |
| onPick={async () => { | |
| try { | |
| const path = await commands.pickRecordingsFolder(); | |
| if (path !== null) { | |
| setSettings("recordingsPath", path); | |
| } | |
| } catch (e) { | |
| const message = e instanceof Error ? e.message : String(e); | |
| toast.error(`Failed to choose recordings folder: ${message}`); | |
| } | |
| }} | |
| onReset={async () => { | |
| try { | |
| await commands.resetRecordingsFolder(); | |
| setSettings("recordingsPath", null); | |
| } catch (e) { | |
| const message = e instanceof Error ? e.message : String(e); | |
| toast.error(`Failed to reset recordings folder: ${message}`); | |
| } | |
| }} |
…dings folder commands
|
please re-review @greptileai |
| .and_then(|p| { | ||
| let path = std::path::PathBuf::from(&p); | ||
| if path.is_absolute() { Some(path) } else { None } | ||
| }); |
There was a problem hiding this comment.
app.path().app_data_dir().unwrap() will panic if path resolution fails (rare, but then any recording/logging path lookup could crash the app). Since this is already best-effort (warn on create_dir_all), it probably makes sense to warn + fall back instead.
| }); | |
| let path = custom.unwrap_or_else(|| match app.path().app_data_dir() { | |
| Ok(dir) => dir.join("recordings"), | |
| Err(e) => { | |
| tracing::warn!("Failed to get app data dir, falling back to temp dir: {e}"); | |
| std::env::temp_dir().join("cap").join("recordings") | |
| } | |
| }); |
| #[tauri::command] | ||
| #[specta::specta] | ||
| #[instrument(skip(app))] | ||
| async fn pick_recordings_folder(app: AppHandle) -> Result<Option<String>, String> { |
There was a problem hiding this comment.
Small UX/diagnostics edge case: if the user picks a folder whose path can’t round-trip through UTF-8, to_str() returns None and this currently looks identical to a cancel (Ok(None)). Returning an error instead lets the UI show a toast (you already added try/catch there).
| async fn pick_recordings_folder(app: AppHandle) -> Result<Option<String>, String> { | |
| let (tx, rx) = tokio::sync::oneshot::channel::<Result<Option<String>, String>>(); | |
| app.dialog() | |
| .file() | |
| .set_title("Choose Recordings Folder") | |
| .pick_folder(move |path| { | |
| let _ = tx.send((|| { | |
| let Some(path) = path else { | |
| return Ok(None); | |
| }; | |
| let Some(path) = path.as_path() else { | |
| return Err("Invalid path returned from dialog".to_string()); | |
| }; | |
| let Some(path) = path.to_str() else { | |
| return Err("Selected folder path is not valid UTF-8".to_string()); | |
| }; | |
| Ok(Some(path.to_string())) | |
| })()); | |
| }); | |
| let result = rx.await.map_err(|e| e.to_string())??; |
2 participants
Adds a setting that lets users choose where Cap saves recordings, instead of always using the default Application Support directory.
A new Storage section in General Settings shows the current recordings folder with a Choose Folder button (native folder picker) and a Reset to Default button that appears when a custom path is active.
The chosen path persists across restarts.
Logging and crash recovery also respect the custom path.
Greptile Summary
This PR adds a user-configurable recordings storage path in General Settings, replacing the hardcoded Application Support directory. The chosen path is persisted in the settings store, and logging and crash-recovery scanning are updated to respect it.
GeneralSettingsStore::recordings_dir()centralises path resolution: it reads the stored custom path, validates it is absolute, falls back to the default with a warning on failure, and ensures the directory exists viacreate_dir_all.pick_recordings_folder/reset_recordings_folder) drive a newStorageSectioncomponent with "Choose Folder" and "Reset to Default" buttons; both async handlers have propertry/catchwith toast notifications on error.app_data_dir().join("recordings")sites (lib.rs, logging.rs, recovery.rs) are unified through the new helper.Confidence Score: 5/5
Safe to merge; the core path-resolution and security logic is sound and all three previously hardcoded directory sites are consistently unified.
The change is internally consistent: error paths log warnings, the absolute-path validation guards against relative inputs, and the existing symlink/traversal protections in
delete_recording_directoryare unaffected. The two findings are minor UX gaps (a platform-mismatched label and deferred write-permission feedback) that don't affect data integrity or correctness.The platform label in
general.tsxand the write-permission check inlib.rs(pick_recordings_folder) are worth a second look before shipping to Windows/Linux users.Important Files Changed
recordings_path: Option<String>field and a newrecordings_dir()helper that validates the stored path is absolute, logs on error, and falls back to the default Application Support directory.pick_recordings_folderandreset_recordings_folderTauri commands; delegatesrecordings_path()to the newGeneralSettingsStore::recordings_dir()helper.upload_log_fileto derive the recordings directory fromGeneralSettingsStore::recordings_dirinstead of the hardcoded Application Support path.find_incomplete_recordingsto useGeneralSettingsStore::recordings_dirfor crash recovery scanning, correctly respecting custom paths.StorageSectioncomponent with path display, Choose Folder button, and Reset to Default button; both async handlers have try/catch with toast.error on failure. The default path label is macOS-specific.pickRecordingsFolder/resetRecordingsFolderbindings and extendsGeneralSettingsStoretype with the newrecordingsPathfield.Reviews (2): Last reviewed commit: "fix: add error logging, safe path conver..." | Re-trigger Greptile