fix(storage): merge the manifest instead of overwriting a change from another device

A manifest changed by a sync (e.g. a page added on another device) could
be overwritten by the next autosave before disk_watch took it over, which
dropped that page from the list. save_manifest now notices that the file
is not the one the notebook in memory is based on and merges the two, with
that base as common ancestor; if there is nothing new to save here, the
other version simply stays.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

docs/FILE_FORMAT.de.md

@@ -178,6 +178,7 @@ Liegen die Notizbücher in einem Ordner, den Nextcloud, Google Drive, Syncthing
 - Solange alle lokalen Änderungen gespeichert sind, prüft die App den Ordner alle 3 Sekunden (und wenn das Fenster den Fokus bekommt). Geänderte Seitendateien werden neu geladen, ein geändertes `manifest.json` bringt neue, entfernte oder umsortierte Seiten und Titel mit.
 - Konfliktkopien der Sync-Clients (`page_<id> (….json`, `page_<id>_….json`, `page_<id>.sync-conflict-….json`) werden als neue Seiten hinter dem Original eingefügt und in `<name>.imported` umbenannt.
 - Bevor die App eine Seitendatei überschreibt, die sich seit ihrem letzten Lesen oder Schreiben geändert hat, sichert sie den vorgefundenen Inhalt als `page_<id> (conflict <Unix-Zeit>).json` – außer sie hat selbst nichts geändert (dann bleibt die Datei, wie sie ist).
+- Dasselbe gilt für `manifest.json`, nur werden die beiden Fassungen zusammengeführt statt kopiert: Auf beiden Seiten hinzugefügte oder entfernte Seiten, Titel und Zähler werden kombiniert, mit dem zuletzt von der App gelesenen oder geschriebenen Manifest als gemeinsamem Vorgänger.
 
 ### Neuere Versionen
 Dateien mit einer höheren `format_version` als der unterstützten werden nicht geöffnet – so kann eine ältere App-Version keine Inhalte verlieren, die sie nicht kennt.

docs/FILE_FORMAT.md

@@ -178,6 +178,7 @@ If the notebooks are kept in a folder synced by Nextcloud, Google Drive, Syncthi
 - While all local changes are saved, the app checks the folder every 3 seconds (and when the window gets the focus). Changed page files are reloaded, a changed `manifest.json` brings in new, removed or reordered pages and titles.
 - Conflict copies of sync clients (`page_<id> (….json`, `page_<id>_….json`, `page_<id>.sync-conflict-….json`) are added as new pages after the original and renamed to `<name>.imported`.
 - Before the app overwrites a page file that changed since it last read or wrote it, it keeps the content found there as `page_<id> (conflict <unix time>).json`, unless it has no changes of its own (then the file is left as it is).
+- The same goes for `manifest.json`, except that the two versions are merged instead of copied: pages added or removed on either side, titles and counters are combined, with the manifest the app last read or wrote as common ancestor.
 
 ### Newer versions
 Files with a higher `format_version` than supported are not opened – this way an older app version cannot lose content it does not know.

src/disk_watch.rs

@@ -166,12 +166,16 @@ impl DiskWatch {
         // Page order, titles, new and removed pages
         let mut fresh_pages: HashSet<u64> = HashSet::new();
         if changed("manifest.json") {
-            match storage::load_manifest(&self.dir) {
-                Ok(theirs) => match self.apply_manifest(notebook, manifest, theirs) {
+            match storage::load_manifest_as_base(&self.dir) {
+                Ok((theirs, base)) => match self.apply_manifest(notebook, manifest, theirs) {
+                    // Taken over: from now on our saves build on this manifest
                     Ok(Some(new_ids)) => {
+                        base.adopt();
                         fresh_pages = new_ids;
                         outcome.structure_changed = true;
                     }
+                    // Nothing to take over for the pages. Whatever else differs is merged
+                    // at the next save, so the base stays as it is.
                     Ok(None) => {}
                     Err(()) => {
                         retry.insert("manifest.json".to_string());
@@ -557,6 +561,107 @@ mod tests {
         assert_eq!(conflict_copies(dir.path()).len(), 1);
     }
 
+    /// Writes the manifest the way a sync client does: behind the back of `storage`.
+    fn write_manifest_from_another_device(dir: &Path, manifest: &NotebookManifest) {
+        fs::write(
+            dir.join("manifest.json"),
+            serde_json::to_vec(manifest).unwrap(),
+        )
+        .unwrap();
+    }
+
+    fn page_ids_on_disk(dir: &Path) -> Vec<u64> {
+        let bytes = fs::read(dir.join("manifest.json")).unwrap();
+        let manifest: NotebookManifest = serde_json::from_slice(&bytes).unwrap();
+        manifest.pages.iter().map(|p| p.id).collect()
+    }
+
+    #[test]
+    fn test_saving_the_manifest_keeps_a_page_added_on_another_device() {
+        let dir = tempfile::tempdir().unwrap();
+        let (mut notebook, mut manifest) = saved_notebook(dir.path());
+        let now = Instant::now();
+        let mut watch = DiskWatch::new(dir.path().to_path_buf(), now);
+
+        // The other device adds a second page
+        let mut theirs = notebook.clone();
+        let mut their_manifest = manifest.clone();
+        let index = theirs.add_page(PageBackground::Lined);
+        theirs.pages[index].add_stroke(stroke(6, 400.0));
+        theirs.next_stroke_id = 7;
+        their_manifest.sync_from_notebook(&theirs);
+        write_from_another_device(dir.path(), &theirs.pages[index]);
+        write_manifest_from_another_device(dir.path(), &their_manifest);
+        let their_page = theirs.pages[index].id;
+
+        // Before the check ran, a stroke is drawn and saved here, twice
+        for id in [2, 3] {
+            notebook.current_page_mut().add_stroke(stroke(id, 200.0));
+            notebook.next_stroke_id = id + 1;
+            manifest.sync_from_notebook(&notebook);
+            storage::save_page(dir.path(), &PageData::from(notebook.current_page())).unwrap();
+            storage::save_manifest(dir.path(), &manifest).unwrap();
+            assert_eq!(page_ids_on_disk(dir.path()), vec![1, their_page]);
+        }
+
+        // The check takes their page over, and saving stays an ordinary save afterwards
+        let outcome = watch.check(&mut notebook, &mut manifest, now);
+        assert!(outcome.structure_changed);
+        assert_eq!(notebook.pages.len(), 2);
+        assert_eq!(notebook.pages[0].strokes.len(), 3);
+        assert!(notebook.next_stroke_id >= 7);
+        manifest.sync_from_notebook(&notebook);
+        storage::save_manifest(dir.path(), &manifest).unwrap();
+        assert_eq!(page_ids_on_disk(dir.path()), vec![1, their_page]);
+
+        // A page removed here afterwards is really gone
+        notebook.pages.pop();
+        notebook.current_page_index = 0;
+        manifest.sync_from_notebook(&notebook);
+        storage::save_manifest(dir.path(), &manifest).unwrap();
+        assert_eq!(page_ids_on_disk(dir.path()), vec![1]);
+    }
+
+    #[test]
+    fn test_saving_an_unchanged_manifest_keeps_the_one_from_another_device() {
+        let dir = tempfile::tempdir().unwrap();
+        let (notebook, manifest) = saved_notebook(dir.path());
+
+        let mut their_manifest = manifest.clone();
+        their_manifest.title = "Physik".to_string();
+        write_manifest_from_another_device(dir.path(), &their_manifest);
+
+        storage::save_manifest(dir.path(), &manifest).unwrap();
+        let bytes = fs::read(dir.path().join("manifest.json")).unwrap();
+        let on_disk: NotebookManifest = serde_json::from_slice(&bytes).unwrap();
+        assert_eq!(on_disk.title, "Physik");
+        drop(notebook);
+    }
+
+    #[test]
+    fn test_manifest_changed_on_both_sides_is_merged() {
+        let dir = tempfile::tempdir().unwrap();
+        let (mut notebook, mut manifest) = saved_notebook(dir.path());
+        let ours_new = notebook.add_page(PageBackground::Blank);
+        let ours_new = notebook.pages[ours_new].id;
+        manifest.sync_from_notebook(&notebook);
+        storage::save_manifest(dir.path(), &manifest).unwrap();
+
+        // They rename the notebook; we remove our new page again
+        let mut their_manifest = manifest.clone();
+        their_manifest.title = "Physik".to_string();
+        write_manifest_from_another_device(dir.path(), &their_manifest);
+        notebook.pages.pop();
+        notebook.current_page_index = 0;
+        manifest.sync_from_notebook(&notebook);
+        storage::save_manifest(dir.path(), &manifest).unwrap();
+
+        let bytes = fs::read(dir.path().join("manifest.json")).unwrap();
+        let on_disk: NotebookManifest = serde_json::from_slice(&bytes).unwrap();
+        assert_eq!(on_disk.title, "Physik");
+        assert!(!on_disk.pages.iter().any(|p| p.id == ours_new));
+    }
+
     #[test]
     fn test_page_still_being_synced_is_tried_again() {
         let dir = tempfile::tempdir().unwrap();

src/storage.rs

@@ -590,16 +590,32 @@ pub fn atomic_write_json<T: Serialize>(path: &Path, value: &T) -> Result<(), Sto
 
 /// Speichert das Manifest atomar in `manifest.json`.
 /// Sichert das bisherige Manifest als `manifest.json.bak` vor dem Überschreiben.
+///
+/// Like a page (see [`save_page`]), the manifest may have been changed behind our back by a
+/// sync before `disk_watch` took the change over, e.g. the other device added a page.
+/// Overwriting it would drop that page from the list. The manifest on disk is therefore
+/// merged with ours, with the manifest the notebook in memory is based on as common ancestor
+/// ([`ManifestBase`]); if we have nothing new ourselves, theirs simply stays.
 pub fn save_manifest(notebook_dir: &Path, manifest: &NotebookManifest) -> Result<(), StorageError> {
     let path = notebook_dir.join("manifest.json");
-    if path.exists() {
+    let ours = serde_json::to_vec(manifest)?;
+    let key = seen_key(&path);
+    let mut merged = None;
+    if let Ok(theirs) = fs::read(&path) {
+        let base = manifest_bases().get(&key).cloned();
+        if base.as_ref() != Some(&theirs) && theirs != ours {
+            if base.as_ref() == Some(&ours) {
+                // Nothing changed here since the notebook was loaded or last saved
+                return Ok(());
+            }
+            // Theirs unreadable (e.g. half written): ours replaces it
+            merged = merge_manifests(base.as_deref(), &ours, &theirs);
+        }
         let bak_path = notebook_dir.join("manifest.json.v0.bak");
         if !bak_path.exists() {
-            if let Ok(content) = fs::read_to_string(&path) {
-                if let Ok(old_m) = serde_json::from_str::<NotebookManifest>(&content) {
-                    if old_m.format_version == 0 {
-                        let _ = fs::copy(&path, &bak_path);
-                    }
+            if let Ok(old_m) = serde_json::from_slice::<NotebookManifest>(&theirs) {
+                if old_m.format_version == 0 {
+                    let _ = fs::copy(&path, &bak_path);
                 }
             }
         }
@@ -607,10 +623,51 @@ pub fn save_manifest(notebook_dir: &Path, manifest: &NotebookManifest) -> Result
         let rolling_bak = notebook_dir.join("manifest.json.bak");
         let _ = fs::copy(&path, &rolling_bak);
     }
-    atomic_write_json(&path, manifest)
+    atomic_write_file(&path, merged.as_ref().unwrap_or(&ours))?;
+    // Ours, not the merged file: the notebook in memory does not contain their part until
+    // `disk_watch` has taken it over, so the next save has to merge it in again
+    manifest_bases().insert(key, ours);
+    Ok(())
 }
 
-fn load_manifest_from_file(path: &Path) -> Result<NotebookManifest, StorageError> {
+/// Three-way merge of manifests as JSON (page lists by page id, see
+/// `cloud_sync::merge_json`); `None` if one of them can't be read or the result is no
+/// valid manifest.
+fn merge_manifests(base: Option<&[u8]>, ours: &[u8], theirs: &[u8]) -> Option<Vec<u8>> {
+    let ours: serde_json::Value = serde_json::from_slice(ours).ok()?;
+    let theirs: serde_json::Value = serde_json::from_slice(theirs).ok()?;
+    let base: Option<serde_json::Value> = base.and_then(|b| serde_json::from_slice(b).ok());
+    let merged = crate::cloud_sync::merge_json(base.as_ref(), &ours, &theirs);
+    serde_json::from_value::<NotebookManifest>(merged.clone()).ok()?;
+    serde_json::to_vec(&merged).ok()
+}
+
+/// Content of `manifest.json` that the notebooks in memory are based on, by file.
+fn manifest_bases() -> std::sync::MutexGuard<'static, std::collections::HashMap<PathBuf, Vec<u8>>> {
+    static BASES: std::sync::OnceLock<
+        std::sync::Mutex<std::collections::HashMap<PathBuf, Vec<u8>>>,
+    > = std::sync::OnceLock::new();
+    BASES
+        .get_or_init(Default::default)
+        .lock()
+        .unwrap_or_else(|e| e.into_inner())
+}
+
+/// A manifest as read from disk, to be declared the state the notebook in memory is based on
+/// once it has been taken over (see [`save_manifest`]). Reading a manifest alone, e.g. for
+/// the list of notebooks, must not do that.
+pub struct ManifestBase {
+    key: PathBuf,
+    bytes: Vec<u8>,
+}
+
+impl ManifestBase {
+    pub fn adopt(self) {
+        manifest_bases().insert(self.key, self.bytes);
+    }
+}
+
+fn load_manifest_from_file(path: &Path) -> Result<(NotebookManifest, Vec<u8>), StorageError> {
     let mut file = File::open(path)?;
     let mut json_str = String::new();
     file.read_to_string(&mut json_str)?;
@@ -630,7 +687,7 @@ fn load_manifest_from_file(path: &Path) -> Result<NotebookManifest, StorageError
             .map(|&id| PageMeta::new(id, None))
             .collect();
     }
-    Ok(manifest)
+    Ok((manifest, json_str.into_bytes()))
 }
 
 /// Lädt das Manifest aus `manifest.json`.
@@ -640,7 +697,15 @@ fn load_manifest_from_file(path: &Path) -> Result<NotebookManifest, StorageError
 /// `<base>/<id>`. A copied or renamed folder still carries the id of the original in its
 /// manifest, so the id is always taken from the folder.
 pub fn load_manifest(notebook_dir: &Path) -> Result<NotebookManifest, StorageError> {
-    let mut manifest = load_manifest_or_backup(notebook_dir)?;
+    load_manifest_as_base(notebook_dir).map(|(manifest, _)| manifest)
+}
+
+/// Like [`load_manifest`], for a manifest that is taken over into the open notebook: call
+/// [`ManifestBase::adopt`] once that has happened.
+pub fn load_manifest_as_base(
+    notebook_dir: &Path,
+) -> Result<(NotebookManifest, ManifestBase), StorageError> {
+    let (mut manifest, bytes) = load_manifest_or_backup(notebook_dir)?;
     // Canonical, so that a path like `.` still yields the folder name
     let dir = notebook_dir.canonicalize();
     let name = dir
@@ -653,10 +718,16 @@ pub fn load_manifest(notebook_dir: &Path) -> Result<NotebookManifest, StorageErr
             manifest.id = name.to_string();
         }
     }
-    Ok(manifest)
+    let base = ManifestBase {
+        key: seen_key(&notebook_dir.join("manifest.json")),
+        bytes,
+    };
+    Ok((manifest, base))
 }
 
-fn load_manifest_or_backup(notebook_dir: &Path) -> Result<NotebookManifest, StorageError> {
+fn load_manifest_or_backup(
+    notebook_dir: &Path,
+) -> Result<(NotebookManifest, Vec<u8>), StorageError> {
     let path = notebook_dir.join("manifest.json");
     if !path.exists() {
         let bak_path = notebook_dir.join("manifest.json.bak");
@@ -871,7 +942,8 @@ pub fn save_notebook(
 /// Lädt ein Notizbuch aus einem Ordner (erste Seite sofort, Folgeseiten als Platzhalter für Lazy Loading).
 pub fn load_notebook(notebook_dir: &Path) -> Result<(Notebook, NotebookManifest), StorageError> {
     let _ = cleanup_orphaned_temp_files(notebook_dir, std::time::Duration::from_secs(3600));
-    let manifest = load_manifest(notebook_dir)?;
+    let (manifest, base) = load_manifest_as_base(notebook_dir)?;
+    base.adopt();
     let page_ids = manifest.page_ids();
     if page_ids.is_empty() {
         let nb = Notebook::new();