Permanent delete of several photos is one consistent op list; restored photos join album/selection

Review of the onDeleted work found that purging photos one at a time built each
op list against the unchanged catalog, so photos sharing an album or a stack made
Empty Recently Deleted (and Fresh imports) fail with NoStack or leave dangling
album entries. Catalog::delete_photos_permanently_ops builds one list for all of
them; Empty, Delete Permanently and the Fresh purge use it. Photos an import
restores now take part in album/albumName/selection, and undo labels say Restore.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

crates/catalog/src/lib.rs

@@ -612,6 +612,22 @@ impl Catalog {
         Op::Batch { ops }
     }
 
+    /// [`Self::delete_permanently_ops`] for several photos at once. One op list built against the
+    /// catalog as it is now: albums lose all of them in one write each, and stacks are shortened
+    /// (or dissolved) once, however many of their photos go.
+    pub fn delete_photos_permanently_ops(&self, ids: &[PhotoId]) -> Op {
+        let gone: std::collections::HashSet<PhotoId> = ids.iter().copied().collect();
+        let mut ops: Vec<Op> = self
+            .albums
+            .values()
+            .filter(|a| a.photos.iter().any(|p| gone.contains(p)))
+            .map(|a| Op::SetAlbumPhotos { id: a.id, photos: a.photos.iter().copied().filter(|p| !gone.contains(p)).collect() })
+            .collect();
+        ops.extend(self.remove_from_stacks_ops(ids));
+        ops.extend(gone.iter().map(|id| Op::RemovePhoto { id: *id }));
+        Op::Batch { ops }
+    }
+
     // ---- persistence
 
     /// Full snapshot as JSON.

crates/engine/src/cmd/library.rs

@@ -94,7 +94,9 @@ pub fn import_params(s: &Session, p: &Value) -> Result<ImportRequest> {
 /// `album`).
 pub fn import_batch_done(s: &mut Session, report: crate::import::ImportReport, mut album: Option<u64>, album_name: Option<&str>) -> Result<Value> {
     let mut report = serde_json::to_value(report).unwrap_or_default();
-    let imported: Vec<u64> = report["imported"].as_array().map(|a| a.iter().filter_map(Value::as_u64).collect()).unwrap_or_default();
+    // photos an import restored from Recently Deleted take part like new ones (album, selection)
+    let imported: Vec<u64> =
+        ["imported", "restored"].iter().flat_map(|k| report[*k].as_array().into_iter().flatten().filter_map(Value::as_u64)).collect();
     if album.is_none()
         && let Some(name) = album_name
         && !imported.is_empty()
@@ -579,16 +581,16 @@ pub fn specs() -> Vec<CommandSpec> {
                 if ids.is_empty() {
                     return Ok(json!({"deleted": 0}));
                 }
-                let ops = ids.iter().map(|id| s.catalog.delete_permanently_ops(*id)).collect::<Vec<_>>();
-                s.commit("Empty Recently Deleted", Op::Batch { ops })?;
+                let op = s.catalog.delete_photos_permanently_ops(&ids);
+                s.commit("Empty Recently Deleted", op)?;
                 s.selection = Selection::default();
                 Ok(json!({"deleted": ids.len()}))
             }
         ),
         cmd!("photo.deletePermanently", "Delete Permanently", ["Photo"], None, "{ids?}", has_selection, |s, p| {
             let t = s.targets(p);
-            let ops = t.iter().map(|id| s.catalog.delete_permanently_ops(*id)).collect::<Vec<_>>();
-            s.commit("Delete Permanently", Op::Batch { ops })?;
+            let op = s.catalog.delete_photos_permanently_ops(&t);
+            s.commit("Delete Permanently", op)?;
             s.selection = Selection::default();
             Ok(json!({"deleted": t.len()}))
         }),
@@ -992,9 +994,12 @@ pub fn specs() -> Vec<CommandSpec> {
                 let report = crate::import::import_with(s, &req.paths, &req.opts)?;
                 let report = import_batch_done(s, report, req.album, req.album_name.as_deref())?;
                 // one undo step for the whole import
-                let imported: Vec<u64> = report["imported"].as_array().map(|a| a.iter().filter_map(Value::as_u64).collect()).unwrap_or_default();
+                let ids_of = |k: &str| -> Vec<u64> { report[k].as_array().map(|a| a.iter().filter_map(Value::as_u64).collect()).unwrap_or_default() };
+                let (imported, restored) = (ids_of("imported"), ids_of("restored"));
                 let n = s.undo.len().saturating_sub(undo0);
-                s.merge_undo(n, &format!("Add {} Photo{}", imported.len(), if imported.len() == 1 { "" } else { "s" }));
+                let (verb, count) = if imported.is_empty() && !restored.is_empty() { ("Restore", restored.len()) } else { ("Add", imported.len()) };
+                s.merge_undo(n, &format!("{verb} {count} Photo{}", if count == 1 { "" } else { "s" }));
+                let imported = if imported.is_empty() { restored } else { imported };
                 if let Some(f) = imported.first() {
                     s.selection = Selection::single(PhotoId(*f));
                 }

crates/engine/src/import.rs

@@ -887,6 +887,7 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare
             }
         }
     }
+    let mut purge: Vec<PhotoId> = Vec::new();
     let mut purged: std::collections::HashSet<PhotoId> = Default::default();
     for it in prepared.items {
         match it {
@@ -923,7 +924,7 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare
                 }
                 for old in info.content_hash.as_ref().and_then(|h| trash_hash.get(h)).into_iter().chain(trash_path.get(&stored)) {
                     if purged.insert(*old) {
-                        ops.push(s.catalog.delete_permanently_ops(*old));
+                        purge.push(*old);
                     }
                 }
                 let sidecar = sidecar.map(|sc| sc.resolve_label(&s.catalog)).filter(|sc| *sc != crate::sidecar::SidecarData::default());
@@ -969,9 +970,18 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare
             }
         }
     }
+    // (built against the catalog as it is: one op list for all of them, ahead of the additions)
+    if !purge.is_empty() {
+        ops.insert(0, s.catalog.delete_photos_permanently_ops(&purge));
+    }
     let log0 = s.pending_log.len();
+    let (verb, n) = if report.imported.is_empty() && !report.restored.is_empty() {
+        ("Restore", report.restored.len())
+    } else {
+        ("Add", report.imported.len().max(1))
+    };
     if !ops.is_empty()
-        && let Err(e) = s.commit(&format!("Add {} Photo{}", ops.len(), if ops.len() == 1 { "" } else { "s" }), Op::Batch { ops })
+        && let Err(e) = s.commit(&format!("{verb} {n} Photo{}", if n == 1 { "" } else { "s" }), Op::Batch { ops })
     {
         // nothing was catalogued: take the moves back (their sources were never touched)
         placed.iter().for_each(crate::import_move::rollback);

crates/engine/src/tests_import.rs

@@ -931,3 +931,74 @@ fn empty_recently_deleted_removes_only_trashed_photos() {
     s.execute("edit.undo", &json!({})).unwrap();
     let _ = std::fs::remove_dir_all(&src);
 }
+
+/// Two trashed photos that share an album and a two-photo stack: emptying (or permanently
+/// deleting) them together must not fail or leave dangling references.
+fn two_trashed_in_album_and_stack(tag: &str) -> (Session, std::path::PathBuf, [u64; 2], u64) {
+    let src = temp_dir(tag);
+    write_png(&src.join("a.png"), 1);
+    write_png(&src.join("b.png"), 2);
+    let mut s = Session::new().with_fs();
+    let r = s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap();
+    let ids = [r["imported"][0].as_u64().unwrap(), r["imported"][1].as_u64().unwrap()];
+    let al = s.execute("album.create", &json!({"name": "Trip"})).unwrap()["id"].as_u64().unwrap();
+    s.execute("album.addPhotos", &json!({"id": al, "ids": ids})).unwrap();
+    s.execute("library.select", &json!({"ids": ids})).unwrap();
+    s.execute("stack.group", &json!({"ids": ids})).unwrap();
+    s.execute("photo.delete", &json!({"ids": ids})).unwrap();
+    (s, src, ids, al)
+}
+
+#[test]
+fn emptying_trashed_photos_of_one_album_and_stack_leaves_nothing_dangling() {
+    let (mut s, src, ids, al) = two_trashed_in_album_and_stack("trash-empty-refs");
+    let r = s.execute("library.emptyRecentlyDeleted", &json!({})).unwrap();
+    assert_eq!(r["deleted"], 2, "{r}");
+    assert_eq!(s.catalog.len(), 0);
+    assert!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.is_empty(), "album still lists a removed photo");
+    assert!(ids.iter().all(|i| s.catalog.stack_of(lightcraft_catalog::PhotoId(*i)).is_none()));
+    s.execute("edit.undo", &json!({})).unwrap();
+    assert_eq!(s.catalog.len(), 2, "one undo step brings both back");
+    assert_eq!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.len(), 2);
+    assert!(s.catalog.stack_of(lightcraft_catalog::PhotoId(ids[0])).is_some());
+    assert!(s.catalog.photos().all(|p| p.deleted), "back in Recently Deleted");
+    let _ = std::fs::remove_dir_all(&src);
+}
+
+#[test]
+fn deleting_several_photos_permanently_leaves_nothing_dangling() {
+    let (mut s, src, ids, al) = two_trashed_in_album_and_stack("trash-perm-refs");
+    s.execute("library.select", &json!({"ids": ids})).unwrap();
+    s.execute("photo.deletePermanently", &json!({"ids": ids})).unwrap();
+    assert!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.is_empty());
+    let _ = std::fs::remove_dir_all(&src);
+}
+
+#[test]
+fn fresh_import_of_two_trashed_photos_in_one_album_and_stack_works() {
+    let (mut s, src, ids, al) = two_trashed_in_album_and_stack("trash-fresh-refs");
+    let r = s.execute("library.import", &json!({"paths": [src.to_string_lossy()], "onDeleted": "fresh"})).unwrap();
+    assert_eq!(ids_len(&r), 2, "{r}");
+    assert!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.is_empty());
+    assert!(ids.iter().all(|i| s.catalog.photo(lightcraft_catalog::PhotoId(*i)).is_none()));
+    let _ = std::fs::remove_dir_all(&src);
+}
+
+fn ids_len(r: &Value) -> usize {
+    ids(r, "imported")
+}
+
+#[test]
+fn restore_import_applies_the_album_to_restored_photos() {
+    let (mut s, src, ids, _) = trashed_photo_ids("trash-restore-album");
+    let r = s.execute("library.import", &json!({"paths": [src.to_string_lossy()], "onDeleted": "restore", "albumName": "Back"})).unwrap();
+    assert_eq!(r["restored"], json!(ids), "{r}");
+    let album = r["album"].as_u64().expect("album created for the restored photos");
+    assert_eq!(s.catalog.album(lightcraft_catalog::AlbumId(album)).unwrap().photos.len(), 1);
+    let _ = std::fs::remove_dir_all(&src);
+}
+
+fn trashed_photo_ids(tag: &str) -> (Session, std::path::PathBuf, [u64; 1], u64) {
+    let (s, src, id) = trashed_photo(tag);
+    (s, src, [id], 0)
+}