From 62a0b683e5479980e45ffc728026bae7fe82110e Mon Sep 17 00:00:00 2001 From: grovebit <337789029+grovebit@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:57:03 +0100 Subject: [PATCH] Ask before Save As replaces another session Save As, Save Copy In, Save As Template and Export Selected Tracks as New Session wrote over any existing file without asking. The Save As prompt proposes ~//.scraft, so every new session suggests ~/Untitled/Untitled.scraft, and saving a second new session with the suggested path replaced the first one. These commands now refuse to write over an existing file unless `replace` is true; Save, Save As and Save Copy In may still write the open session's own file. Scripts, MCP agents and the control channel can't replace a session by accident. While the path in the prompt names an existing file, the prompt asks whether to replace it and its OK button becomes Replace; Enter alone doesn't replace. Saving also creates the session's folder, so a new session without audio can be saved to the suggested path. The CLI's --save names its output explicitly and keeps replacing it, like --bounce. --- apps/soundcraft-cli/src/main.rs | 2 +- crates/automation/src/mcp.rs | 7 +++- crates/engine/src/cmd/file.rs | 29 ++++++++++---- crates/engine/src/io.rs | 51 ++++++++++++++++++++++++- crates/ui-egui/src/dialogs.rs | 10 ++++- crates/ui-egui/tests/save_as_replace.rs | 38 ++++++++++++++++++ docs/mcp.md | 2 +- 7 files changed, 126 insertions(+), 13 deletions(-) create mode 100644 crates/ui-egui/tests/save_as_replace.rs diff --git a/apps/soundcraft-cli/src/main.rs b/apps/soundcraft-cli/src/main.rs index 46271c9..2d8861c 100644 --- a/apps/soundcraft-cli/src/main.rs +++ b/apps/soundcraft-cli/src/main.rs @@ -187,7 +187,7 @@ fn finish(e: &mut Engine, args: &[String]) -> ExitCode { } } if let Some(path) = arg_value(args, "--save") { - match e.execute("session.save", &json!({"path": path})) { + match e.execute("session.save", &json!({"path": path, "replace": true})) { Ok(v) => outln!("{v}"), Err(err) => return fail(err), } diff --git a/crates/automation/src/mcp.rs b/crates/automation/src/mcp.rs index 1b38dcc..b3d21ab 100644 --- a/crates/automation/src/mcp.rs +++ b/crates/automation/src/mcp.rs @@ -54,7 +54,12 @@ pub fn tool_definitions() -> Vec { &[], ), tool("open_session", "Open a .scraft session file.", json!({"path": {"type": "string"}}), &["path"]), - tool("save_session", "Save the session (path required the first time).", json!({"path": {"type": "string"}}), &[]), + tool( + "save_session", + "Save the session (path required the first time; replace: true to save over another existing file).", + json!({"path": {"type": "string"}, "replace": {"type": "boolean"}}), + &[], + ), tool( "import_audio", "Import an audio file (WAV, AIFF, FLAC, MP3, OGG, AAC/M4A, ALAC, CAF) onto a new or existing track.", diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index d58af41..8c91d0a 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -15,32 +15,37 @@ pub fn specs() -> Vec { Ok(json!({"name": e.session().name, "tracks": e.session().tracks.len(), "missing": missing})) }), cmd!(noundo "session.close", "Close Session", ["File"], Some("Cmd+Shift+W"), "{}", always, |e, _| { e.replace_session(Session::default()); e.path = None; Ok(json!({})) }), - cmd!(noundo "session.save", "Save Session", ["File"], Some("Cmd+S"), "{path?}", always, |e, p| { + cmd!(noundo "session.save", "Save Session", ["File"], Some("Cmd+S"), "{path?, replace?: false}", always, |e, p| { let path = str_param(p, "path").map(str::to_string).or_else(|| e.path.clone()).ok_or_else(|| bad("session.save", "no path yet: pass `path` (Save As)"))?; + refuse_replacing(e, "session.save", &crate::io::session_file(&path), p)?; let written = crate::io::save_session(e, &path)?; Ok(json!({"path": path, "audio_files_written": written})) }), - cmd!(noundo "session.save_as", "Save Session As...", ["File"], Some("Cmd+Shift+S"), "{path}", always, |e, p| { + cmd!(noundo "session.save_as", "Save Session As...", ["File"], Some("Cmd+Shift+S"), "{path, replace?: false}", always, |e, p| { let path = str_param(p, "path").ok_or_else(|| bad("session.save_as", "`path` required"))?.to_string(); + refuse_replacing(e, "session.save_as", &crate::io::session_file(&path), p)?; let written = crate::io::save_session(e, &path)?; Ok(json!({"path": path, "audio_files_written": written})) }), - cmd!(noundo "session.save_copy", "Save Session Copy In...", ["File"], None, "{path}", always, |e, p| { + cmd!(noundo "session.save_copy", "Save Session Copy In...", ["File"], None, "{path, replace?: false}", always, |e, p| { let path = str_param(p, "path").ok_or_else(|| bad("session.save_copy", "`path` required"))?.to_string(); + refuse_replacing(e, "session.save_copy", &crate::io::session_file(&path), p)?; let keep = e.path.clone(); let written = crate::io::save_session(e, &path)?; e.path = keep; Ok(json!({"path": path, "audio_files_written": written})) }), - cmd!(noundo "session.save_template", "Save As Template...", ["File"], None, "{path}", always, |e, p| { + cmd!(noundo "session.save_template", "Save As Template...", ["File"], None, "{path, replace?: false}", always, |e, p| { let path = str_param(p, "path").ok_or_else(|| bad("session.save_template", "`path` required"))?.to_string(); + let file = crate::io::session_file(&path); + refuse_replacing(e, "session.save_template", &file, p)?; let mut s = e.session().clone(); for t in &mut s.tracks { for pl in &mut t.playlists { pl.clips.clear() } } s.sources.clear(); s.videos.clear(); let text = s.to_json().map_err(|err| EngineError::Io(err.to_string()))?; - std::fs::write(&path, text).map_err(|err| EngineError::Io(format!("{path}: {err}")))?; - Ok(json!({"path": path})) + std::fs::write(&file, text).map_err(|err| EngineError::Io(format!("{}: {err}", file.display())))?; + Ok(json!({"path": file})) }), cmd!(noundo "session.revert", "Revert Session to Saved...", ["File"], None, "{}", always, |e, _| { let path = e.path.clone().ok_or_else(|| bad("session.revert", "the session has not been saved"))?; @@ -115,8 +120,9 @@ pub fn specs() -> Vec { let n = crate::io::export_clips(e, &ids, &dir)?; Ok(json!({"written": n})) }), - cmd!(noundo "file.export_selected_tracks_as_session", "Selected Tracks as New Session...", ["File", "Export"], None, "{path, tracks?}", has_selection, |e, p| { + cmd!(noundo "file.export_selected_tracks_as_session", "Selected Tracks as New Session...", ["File", "Export"], None, "{path, tracks?, replace?: false}", has_selection, |e, p| { let path = str_param(p, "path").ok_or_else(|| bad("file.export_selected_tracks_as_session", "`path` required"))?.to_string(); + refuse_replacing(e, "file.export_selected_tracks_as_session", &crate::io::session_file(&path), p)?; let keep = tracks_required(e, "file.export_selected_tracks_as_session", p)?; let mut s = e.session().clone(); s.tracks.retain(|t| keep.contains(&t.id) || t.kind == soundcraft_model::TrackKind::Master); @@ -180,6 +186,14 @@ pub fn specs() -> Vec { ] } +/// Writing a session over an existing file needs `replace: true` (see `io::replaces`). +fn refuse_replacing(e: &Engine, id: &str, file: &std::path::Path, p: &Value) -> Result<()> { + if crate::io::replaces(e, id, file) && !bool_or(p, "replace", false) { + return Err(bad(id, format!("{} already exists: pass `replace: true` to replace it", file.display()))); + } + Ok(()) +} + fn new_session(e: &mut Engine, p: &Value) -> Result { let name = str_param(p, "name").unwrap_or("Untitled").to_string(); let sr = @@ -380,6 +394,7 @@ mod tests { fn session_open_reports_missing_media() { let mut e = crate::demo::demo_engine(); let dir = std::env::temp_dir().join(format!("soundcraft-open-missing-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); let path = dir.join("Gone.scraft").to_string_lossy().into_owned(); e.execute("session.save_as", &json!({"path": path})).unwrap(); let mut ok = Engine::default(); diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index a563387..655955e 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -474,8 +474,7 @@ pub fn save_copy(e: &Engine, path: &str) -> Result { /// Write `session` to `path`; returns the session with updated media paths, the number of audio /// files written and the final path. fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Session, usize, PathBuf)> { - let p = PathBuf::from(path); - let p = if p.extension().is_none() { p.with_extension(soundcraft_model::SESSION_EXTENSION) } else { p }; + let p = session_file(path); let dir = p.parent().map(Path::to_path_buf).unwrap_or_default(); let audio_dir = dir.join("Audio Files"); let mut s = session.clone(); @@ -519,6 +518,7 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess s.name = stem.to_string(); } let text = s.to_json().map_err(|err| EngineError::Io(err.to_string()))?; + std::fs::create_dir_all(&dir).map_err(|err| EngineError::Io(format!("{}: {err}", dir.display())))?; // Atomic-ish write: temp file then rename. let tmp = p.with_extension("scraft.tmp"); std::fs::write(&tmp, text).map_err(|err| EngineError::Io(format!("{}: {err}", tmp.display())))?; @@ -526,6 +526,25 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess Ok((s, written, p)) } +/// The file a session saved to `path` goes to: `path`, with `.scraft` added when it has no +/// extension and isn't an existing file. +pub fn session_file(path: &str) -> PathBuf { + let p = PathBuf::from(path); + if p.extension().is_none() && !p.is_file() { p.with_extension(soundcraft_model::SESSION_EXTENSION) } else { p } +} + +/// True when `command` would write a session over the existing `file`. Save, Save As and Save +/// Copy In may write the open session's own file; other commands never ask. +pub fn replaces(e: &Engine, command: &str, file: &Path) -> bool { + match command { + "session.save" | "session.save_as" | "session.save_copy" => { + file.exists() && !e.path.as_deref().is_some_and(|own| same_path(Path::new(own), file)) + } + "session.save_template" | "file.export_selected_tracks_as_session" => file.exists(), + _ => false, + } +} + fn same_path(a: &Path, b: &Path) -> bool { let full = |p: &Path| if p.as_os_str().is_empty() { Path::new(".").canonicalize() } else { p.canonicalize() }; a == b || matches!((full(a), full(b)), (Ok(x), Ok(y)) if x == y) @@ -745,6 +764,34 @@ mod save_tests { assert_eq!(open_session(&mut r, &path("B/Moved.scraft")).unwrap(), vec![dir.join("A/Audio Files/Audio 1_02.wav").display().to_string()]); let _ = std::fs::remove_dir_all(&dir); } + + #[test] + fn saving_over_another_session_needs_replace() { + let dir = std::env::temp_dir().join(format!("soundcraft-replace-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let path = dir.join("Song.scraft").to_string_lossy().into_owned(); + let mut first = with_take(0.5); + first.execute("session.save_as", &json!({"path": path})).unwrap(); + first.execute("session.save", &json!({})).unwrap(); + first.execute("session.save_as", &json!({"path": dir.join("Song")})).unwrap(); + assert!(first.execute("session.save_template", &json!({"path": path})).is_err()); + let mut second = with_take(-0.25); + let track = second.session().tracks[0].id.0; + for id in ["session.save", "session.save_as", "session.save_copy", "session.save_template", "file.export_selected_tracks_as_session"] { + let err = second.execute(id, &json!({"path": path, "tracks": [track]})).unwrap_err(); + assert!(err.to_string().contains("`replace: true`"), "{id}: {err}"); + } + assert_eq!(std::fs::read_dir(dir.join("Audio Files")).unwrap().count(), 1); + Engine::default().execute("session.save_as", &json!({"path": dir.join("New/New.scraft")})).unwrap(); + std::fs::copy(&path, dir.join("Song")).unwrap(); + let mut bare = Engine::default(); + open_session(&mut bare, &dir.join("Song").to_string_lossy()).unwrap(); + bare.execute("session.save", &json!({})).unwrap(); + second.execute("session.save_as", &json!({"path": path, "replace": true})).unwrap(); + let r = reopen(&path); + assert_eq!(r.session().pool.get(r.session().sources[0].id).unwrap().buffer.channels[0][0], -0.25); + let _ = std::fs::remove_dir_all(&dir); + } } #[cfg(test)] diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index d212634..d49b2af 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -303,9 +303,17 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) { { *path = p; } - if buttons(ui, "OK", enter) { + let file = soundcraft_engine::io::session_file(path); + let replaces = soundcraft_engine::io::replaces(&app.engine, cmd, &file); + if replaces { + ui.label(format!("“{}” already exists. Do you want to replace it?", file.file_name().unwrap_or_default().to_string_lossy())); + } + if buttons(ui, if replaces { "Replace" } else { "OK" }, enter && !replaces) { let mut v = json!({}); v[key.as_str()] = json!(path); + if replaces { + v["replace"] = json!(true); + } action = Some((cmd.clone(), v)); } } diff --git a/crates/ui-egui/tests/save_as_replace.rs b/crates/ui-egui/tests/save_as_replace.rs new file mode 100644 index 0000000..717e2a6 --- /dev/null +++ b/crates/ui-egui/tests/save_as_replace.rs @@ -0,0 +1,38 @@ +//! Save As over another session asks first, and Enter alone never replaces it. +use egui::vec2; +use egui_kittest::{Harness, kittest::Queryable}; +use serde_json::json; +use soundcraft_ui_egui::{Services, SoundApp, dialogs::Dialog}; + +#[test] +fn saving_over_another_session_takes_the_replace_button() { + let dir = std::env::temp_dir().join(format!("soundcraft-ui-replace-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("Song.scraft"); + soundcraft_engine::Engine::default().execute("session.save_as", &json!({"path": path})).unwrap(); + let other = std::fs::read(&path).unwrap(); + let mut app = SoundApp::new(soundcraft_engine::demo::demo_engine(), None, Services::default()); + app.dialogs.open = Some(Dialog::PathPrompt { + cmd: "session.save_as".into(), + title: "Save Session As".into(), + path: path.to_string_lossy().into(), + key: "path".into(), + }); + let mut h = Harness::builder().with_size(vec2(1600.0, 1000.0)).build_ui_state( + |ui, app: &mut SoundApp| { + app.logic(&ui.ctx().clone()); + app.ui(ui); + }, + app, + ); + h.run_steps(3); + h.key_press(egui::Key::Enter); + h.run_steps(3); + assert_eq!(std::fs::read(&path).unwrap(), other); + h.get_by_label("Replace").click(); + h.run_steps(3); + assert_ne!(std::fs::read(&path).unwrap(), other); + assert_eq!(h.state().engine.path, Some(path.to_string_lossy().into_owned())); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/docs/mcp.md b/docs/mcp.md index 2949871..deb026d 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -24,7 +24,7 @@ claude mcp add soundcraft -- soundcraft-cli mcp --demo | `execute` | Run one command with JSON params. | | `batch` | Run several commands in order (`keep_going` optional). | | `inspect_session` | Verify work: tracks, clips, mixer, plugins, sends, automation, markers, selection; each `sources[]` entry includes `loaded` (decoded audio in the pool). | -| `new_session`, `open_session`, `save_session` | Session files (`.scraft`, audio in `Audio Files/`). `open_session` returns `missing`: the media files that could not be loaded (empty when all are found). | +| `new_session`, `open_session`, `save_session` | Session files (`.scraft`, audio in `Audio Files/`). `open_session` returns `missing`: the media files that could not be loaded (empty when all are found). `save_session` won't save over another existing file unless `replace` is true. | | `import_audio` | WAV, AIFF, FLAC, MP3, OGG, AAC/M4A, ALAC, CAF onto a new or existing track. | | `bounce_mix` | Render to WAV/AIFF/FLAC; returns peak dBFS and integrated LUFS. | | `parity` | Feature-parity report. |