fix(config): preserve preset saves from other sessions

Cargo.toml

@@ -21,6 +21,7 @@ serde_json = "1.0"
 tokio = { version = "1.39", features = ["rt-multi-thread", "macros", "time"] }
 async-trait = "0.1"
 pulldown-cmark = "0.12"
+tempfile = "3.27"
 
 # The profile that 'dist' will build with
 [profile.dist]

FEATURES.md

@@ -100,4 +100,5 @@ See the [feature demo](DEMO.md) for a visual walkthrough of these capabilities i
 - Scrollable help overlay with keyboard and mouse wheel controls; input stays in help until it is dismissed
 - Configurable keybindings via `~/.config/blippy/keybinds.toml`
 - Configurable close-comment presets in `~/.config/blippy/config.toml`
+- Preset saves retain presets and configuration changes saved by other sessions
 - Failed preset saves keep the editor and draft open so the write can be retried

README.md

@@ -86,6 +86,8 @@ See [FEATURES.md](FEATURES.md) for a full feature breakdown.
 
 Bindings in `keybinds.toml` override `[keybinds]` in `config.toml`. Saving comment presets preserves each file's bindings, so removing an override restores the binding from the main config.
 
+Saving a preset also keeps presets, themes, and bindings saved by other sessions since this session started. Failed writes leave the previous configuration intact so saving can be retried.
+
 Theme example:
 
 ```toml

src/app/state.rs

@@ -282,11 +282,7 @@ impl App {
     }
 
     pub fn save_comment_default(&mut self, preset: CommentDefault) -> Result<()> {
-        self.config.comment_defaults.push(preset);
-        if let Err(error) = self.config.save() {
-            self.config.comment_defaults.pop();
-            return Err(error);
-        }
+        self.config.save_comment_default(preset)?;
         self.preset.choice = 0;
         Ok(())
     }

src/config.rs

@@ -1,6 +1,7 @@
 use std::collections::HashMap;
 use std::env;
 use std::fs;
+use std::io::Write;
 use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result};
@@ -54,16 +55,62 @@ impl Config {
         keybinds
     }
 
-    pub fn save(&self) -> Result<()> {
-        let path = config_path();
-        if let Some(parent) = path.parent() {
-            fs::create_dir_all(parent)
-                .with_context(|| format!("Failed to create config dir at {}", parent.display()))?;
+    pub fn save_comment_default(&mut self, preset: CommentDefault) -> Result<()> {
+        let mut path = config_path();
+        if path.exists() {
+            path = path
+                .canonicalize()
+                .with_context(|| format!("Failed to resolve config at {}", path.display()))?;
+        }
+        let parent = path
+            .parent()
+            .context("Config path has no parent directory")?;
+        fs::create_dir_all(parent)
+            .with_context(|| format!("Failed to create config dir at {}", parent.display()))?;
+        // Lock a stable file: replacing config.toml changes its file identity.
+        let lock = fs::OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(false)
+            .open(path.with_extension("lock"))
+            .with_context(|| format!("Failed to lock config at {}", path.display()))?;
+        lock.lock()
+            .with_context(|| format!("Failed to lock config at {}", path.display()))?;
+        let mut current: Self = match fs::read_to_string(&path) {
+            Ok(contents) => toml::from_str(&contents)
+                .with_context(|| format!("Failed to parse config at {}", path.display()))?,
+            Err(error) if error.kind() == std::io::ErrorKind::NotFound => Self::default(),
+            Err(error) => {
+                return Err(error)
+                    .with_context(|| format!("Failed to read config at {}", path.display()));
+            }
+        };
+        current.comment_defaults.push(preset);
+        let contents = toml::to_string_pretty(&current).context("Failed to serialize config")?;
+        let mut file = tempfile::NamedTempFile::new_in(parent)
+            .with_context(|| format!("Failed to stage config at {}", path.display()))?;
+        if path.exists() {
+            let permissions = fs::metadata(&path)
+                .with_context(|| {
+                    format!("Failed to read config permissions at {}", path.display())
+                })?
+                .permissions();
+            file.as_file()
+                .set_permissions(permissions)
+                .with_context(|| {
+                    format!(
+                        "Failed to preserve config permissions at {}",
+                        path.display()
+                    )
+                })?;
         }
-        let contents =
-            toml::to_string_pretty(self).with_context(|| "Failed to serialize config")?;
-        fs::write(&path, contents)
+        file.write_all(contents.as_bytes())
+            .and_then(|()| file.as_file().sync_all())
             .with_context(|| format!("Failed to write config at {}", path.display()))?;
+        file.persist(&path)
+            .map_err(|error| error.error)
+            .with_context(|| format!("Failed to replace config at {}", path.display()))?;
+        self.comment_defaults = current.comment_defaults;
         Ok(())
     }
 }
@@ -158,11 +205,12 @@ mod tests {
         );
 
         let mut config = Config::load().expect("load config for preset");
-        config.comment_defaults.push(super::CommentDefault {
-            name: "resolved".into(),
-            body: "Closing as resolved.".into(),
-        });
-        config.save().expect("save preset");
+        config
+            .save_comment_default(super::CommentDefault {
+                name: "resolved".into(),
+                body: "Closing as resolved.".into(),
+            })
+            .expect("save preset");
         let saved: Config = toml::from_str(
             &std::fs::read_to_string(dir.join("config.toml")).expect("read saved config"),
         )
@@ -189,6 +237,234 @@ mod tests {
         );
     }
 
+    #[test]
+    fn preset_saves_preserve_changes_from_other_sessions() {
+        const CONFIG_ENV: &str = "BLIPPY_TEST_SHARED_PRESETS";
+        let Some(config_dir) = std::env::var_os(CONFIG_ENV) else {
+            let nanos = std::time::SystemTime::now()
+                .duration_since(std::time::UNIX_EPOCH)
+                .unwrap()
+                .as_nanos();
+            let dir = std::env::temp_dir().join(format!("blippy-shared-presets-{nanos}"));
+            std::fs::create_dir_all(dir.join("blippy")).unwrap();
+            std::fs::write(
+                dir.join("blippy/config.toml"),
+                "[keybinds]\nquit = \"ctrl+q\"\n",
+            )
+            .unwrap();
+            let output = std::process::Command::new(std::env::current_exe().unwrap())
+                .args([
+                    "--exact",
+                    "config::tests::preset_saves_preserve_changes_from_other_sessions",
+                    "--nocapture",
+                ])
+                .env(CONFIG_ENV, &dir)
+                .env("XDG_CONFIG_HOME", &dir)
+                .output()
+                .unwrap();
+            std::fs::remove_dir_all(&dir).unwrap();
+            assert!(
+                output.status.success(),
+                "{}\n{}",
+                String::from_utf8_lossy(&output.stdout),
+                String::from_utf8_lossy(&output.stderr)
+            );
+            return;
+        };
+        let mut first = crate::app::App::new(Config::load().unwrap());
+        let mut second = crate::app::App::new(Config::load().unwrap());
+        let path = std::path::PathBuf::from(config_dir).join("blippy/config.toml");
+        // A config edit and both sessions' saves affect the same shared file.
+        std::fs::write(
+            &path,
+            "theme = \"midnight\"\n[keybinds]\nquit = \"ctrl+x\"\n",
+        )
+        .unwrap();
+        for (app, name) in [(&mut first, "first"), (&mut second, "second")] {
+            app.save_comment_default(super::CommentDefault {
+                name: name.into(),
+                body: format!("{name} body"),
+            })
+            .unwrap();
+        }
+        let saved = Config::load().unwrap();
+        let names = saved
+            .comment_defaults
+            .iter()
+            .map(|preset| preset.name.as_str())
+            .collect::<Vec<_>>();
+        eprintln!(
+            "saved presets={names:?}, theme={:?}, quit={:?}",
+            saved.theme,
+            saved.keybinds.get("quit")
+        );
+        assert_eq!(names, ["first", "second"]);
+        assert_eq!(saved.theme.as_deref(), Some("midnight"));
+        assert_eq!(
+            saved.keybinds.get("quit").map(String::as_str),
+            Some("ctrl+x")
+        );
+        assert_eq!(second.comment_defaults(), saved.comment_defaults);
+
+        let barrier = std::sync::Arc::new(std::sync::Barrier::new(3));
+        let writers = (0..2)
+            .map(|index| {
+                let mut app = crate::app::App::new(Config::load().unwrap());
+                let barrier = barrier.clone();
+                std::thread::spawn(move || {
+                    barrier.wait();
+                    app.save_comment_default(super::CommentDefault {
+                        name: format!("writer{index}"),
+                        body: "Parallel save".into(),
+                    })
+                    .unwrap();
+                })
+            })
+            .collect::<Vec<_>>();
+        barrier.wait();
+        for writer in writers {
+            writer.join().unwrap();
+        }
+        let saved = Config::load().unwrap();
+        let mut names = saved
+            .comment_defaults
+            .iter()
+            .map(|preset| preset.name.as_str())
+            .collect::<Vec<_>>();
+        names.sort_unstable();
+        assert_eq!(names, ["first", "second", "writer0", "writer1"]);
+
+        std::fs::write(&path, "").unwrap();
+        second
+            .save_comment_default(super::CommentDefault {
+                name: "after-reset".into(),
+                body: "Only the new preset".into(),
+            })
+            .unwrap();
+        let saved = Config::load().unwrap();
+        assert_eq!(saved.comment_defaults.len(), 1);
+        assert_eq!(saved.comment_defaults[0].name, "after-reset");
+        assert!(saved.theme.is_none());
+        assert!(saved.keybinds.is_empty());
+
+        std::fs::remove_file(&path).unwrap();
+        second
+            .save_comment_default(super::CommentDefault {
+                name: "after-delete".into(),
+                body: "Only the new preset after deletion".into(),
+            })
+            .unwrap();
+        let saved = Config::load().unwrap();
+        assert_eq!(saved.comment_defaults.len(), 1);
+        assert_eq!(saved.comment_defaults[0].name, "after-delete");
+        assert!(saved.theme.is_none());
+        assert!(saved.keybinds.is_empty());
+
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt;
+
+            let target = path.with_file_name("linked-config.toml");
+            std::fs::rename(&path, &target).unwrap();
+            std::os::unix::fs::symlink(&target, &path).unwrap();
+            std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o640)).unwrap();
+            second
+                .save_comment_default(super::CommentDefault {
+                    name: "linked".into(),
+                    body: "Keep the symlink".into(),
+                })
+                .unwrap();
+            assert!(path.is_symlink());
+            assert_eq!(Config::load().unwrap().comment_defaults[1].name, "linked");
+            assert_eq!(
+                std::fs::metadata(&target).unwrap().permissions().mode() & 0o777,
+                0o640
+            );
+        }
+
+        std::fs::write(&path, "theme = \"").unwrap();
+        assert!(
+            second
+                .save_comment_default(super::CommentDefault {
+                    name: "invalid".into(),
+                    body: "Do not overwrite malformed config".into(),
+                })
+                .is_err()
+        );
+        assert_eq!(std::fs::read_to_string(&path).unwrap(), "theme = \"");
+    }
+
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn failed_preset_writes_leave_config_available_for_retry() {
+        const CONFIG_ENV: &str = "BLIPPY_TEST_PARTIAL_PRESET";
+        const FAIL_ENV: &str = "BLIPPY_TEST_PARTIAL_PRESET_FAIL";
+        let Some(config_dir) = std::env::var_os(CONFIG_ENV) else {
+            let dir = tempfile::tempdir().unwrap();
+            let config_dir = dir.path().join("blippy");
+            std::fs::create_dir(&config_dir).unwrap();
+            std::fs::write(
+                config_dir.join("config.toml"),
+                "theme = \"midnight\"\n[[comment_defaults]]\nname = \"existing\"\nbody = \"Keep me\"\n",
+            )
+            .unwrap();
+            for fail in [true, false] {
+                let executable = std::env::current_exe().unwrap();
+                let mut command = if fail {
+                    let mut command = std::process::Command::new("/bin/sh");
+                    // Restrict only this child; a later child retries without the limit.
+                    command
+                        .args(["-c", "trap '' XFSZ; ulimit -f 1; exec \"$@\"", "sh"])
+                        .arg(&executable);
+                    command
+                } else {
+                    std::process::Command::new(executable)
+                };
+                let output = command
+                    .args([
+                        "--exact",
+                        "config::tests::failed_preset_writes_leave_config_available_for_retry",
+                        "--nocapture",
+                    ])
+                    .env(CONFIG_ENV, dir.path())
+                    .env(FAIL_ENV, if fail { "1" } else { "0" })
+                    .env("XDG_CONFIG_HOME", dir.path())
+                    .output()
+                    .unwrap();
+                assert!(
+                    output.status.success(),
+                    "{}\n{}",
+                    String::from_utf8_lossy(&output.stdout),
+                    String::from_utf8_lossy(&output.stderr)
+                );
+            }
+            assert_eq!(std::fs::read_dir(config_dir).unwrap().count(), 2);
+            return;
+        };
+        let path = std::path::PathBuf::from(config_dir).join("blippy/config.toml");
+        let before = std::fs::read(&path).unwrap();
+        let mut config = Config::load().unwrap();
+        let result = config.save_comment_default(super::CommentDefault {
+            name: "new".into(),
+            body: "x".repeat(16384),
+        });
+        if std::env::var(FAIL_ENV).unwrap() == "1" {
+            let error = result.expect_err("force a partial write with the child file-size limit");
+            eprintln!("{error:#}");
+            assert!(error.to_string().starts_with("Failed to write config"));
+            assert_eq!(std::fs::read(&path).unwrap(), before);
+            assert_eq!(config.comment_defaults.len(), 1);
+            return;
+        }
+        result.expect("retry after removing the write restriction");
+        let saved = Config::load().unwrap();
+        assert_eq!(saved.theme.as_deref(), Some("midnight"));
+        assert_eq!(saved.comment_defaults, config.comment_defaults);
+        assert_eq!(saved.comment_defaults.len(), 2);
+        assert_eq!(saved.comment_defaults[0].body, "Keep me");
+        assert_eq!(saved.comment_defaults[1].body.len(), 16384);
+    }
+
     #[test]
     fn config_paths_use_the_windows_profile_without_home() {
         use std::path::PathBuf;

src/main/tests.rs

@@ -289,6 +289,7 @@ fn preset_save_failures_keep_the_draft_and_allow_retry() {
         }],
         ..Config::default()
     };
+    let original_config = toml::to_string(&config).expect("original config");
     let mut app = crate::app::App::new(config);
     app.editor_mut().reset_for_preset_name();
     for ch in "resolved".chars() {
@@ -304,6 +305,7 @@ fn preset_save_failures_keep_the_draft_and_allow_retry() {
     for writable in [false, true] {
         if writable {
             fs::remove_dir(&config_path).expect("allow config writes");
+            fs::write(&config_path, &original_config).expect("restore writable config");
         }
         app.on_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
         let (event_tx, _) = channel();