From c98010cffa7b34535d461a5d588909548984fc40 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Fri, 9 Oct 2026 18:16:32 -0400 Subject: [PATCH] Synchronize Folder: act on the scan shown, never rescan on the UI thread (review fixes) A scan went stale on any catalog change anywhere (a background import, a rating elsewhere), and folder.synchronize then scanned again inline: on the UI thread, and acting on results the person never saw. A scan now stays current while the folder's own photos are the same records (Session::take_folder_changes compares them by identity). The dialog asks with `scanned: true`, which refuses a stale scan instead of redoing it; the dialog then stays open and scans again in the background. Agents that just call folder.synchronize still get a fresh scan. The app dropped session.folder_changes every frame no dialog was open, so an agent's folder.scanChanges never reached its folder.synchronize in the running app; it now only lets go of a scan its dialog made. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/sync.rs | 4 +++- crates/engine/src/sync.rs | 32 ++++++++++++++++++---------- crates/engine/src/tests_sync.rs | 32 ++++++++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 3 +++ crates/ui-egui/src/panels/dialogs.rs | 1 + crates/ui-egui/src/sync.rs | 23 +++++++++++++++----- crates/ui-egui/src/tests_sync.rs | 32 ++++++++++++++++++++++++++++ 7 files changed, 110 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/cmd/sync.rs b/crates/engine/src/cmd/sync.rs index 505377a7e..81693e5ae 100644 --- a/crates/engine/src/cmd/sync.rs +++ b/crates/engine/src/cmd/sync.rs @@ -33,7 +33,7 @@ pub fn specs() -> Vec { "Synchronize Folder", [], None, - "{path, disk?: bool, importNew?: true, relinkMoved?: true, removeMissing?: false, readMetadata?: false} — bring a folder of the library up to date with the disk (see folder.scanChanges): import its new files in place, relink photos whose file was renamed or moved within it, move photos whose file is gone to Recently Deleted, read XMP sidecars saved by other apps (the sidecar wins); acts on the last folder.scanChanges of the folder while nothing changed in the library since (else scans again); one undo step, no file is touched → {imported, relinked, removed, read, failed: [[path, error]]}", + "{path, disk?: bool, scanned?: bool, importNew?: true, relinkMoved?: true, removeMissing?: false, readMetadata?: false} — bring a folder of the library up to date with the disk (see folder.scanChanges): import its new files in place, relink photos whose file was renamed or moved within it, move photos whose file is gone to Recently Deleted, read XMP sidecars saved by other apps (the sidecar wins); acts on the last folder.scanChanges of the folder while it is current (the folder's photos unchanged), else scans again — or, with `scanned: true`, refuses; one undo step, no file is touched → {imported, relinked, removed, read, failed: [[path, error]]}", always, sync ), @@ -52,6 +52,8 @@ fn sync(s: &mut Session, p: &Value) -> Result { }; let changes = match s.take_folder_changes(&path) { Some(c) => c, + // the person agreed to what a scan showed: never act on something else + None if bool_or(p, "scanned", false) => return Err(bad(C, format!("{path} changed since it was scanned: scan again"))), None => scan(s, &path, bool_or(p, "disk", false)).map_err(|e| bad(C, e.to_string()))?, }; let r = synchronize(s, changes, choice)?; diff --git a/crates/engine/src/sync.rs b/crates/engine/src/sync.rs index 0b5cc73db..92b60aeab 100644 --- a/crates/engine/src/sync.rs +++ b/crates/engine/src/sync.rs @@ -42,7 +42,6 @@ const MAX_SIDECAR_BYTES: u64 = 16 * 1024 * 1024; /// What a scan of a folder needs, taken from the session so it can run on a worker thread. pub struct SyncInput { folder: String, - revision: u64, scan: ScanInput, /// The library's photos in the folder, each with how its sidecar is named. photos: Vec<(Arc, SidecarNaming)>, @@ -73,9 +72,10 @@ pub struct FolderChanges { /// The probes of the new files, kept for the import that follows. #[serde(skip)] probes: HashMap, - /// The catalog revision the scan was made against. + /// The folder's photos as the scan saw them: the scan is current while they are the same + /// records (any change to one replaces it in the catalog). #[serde(skip)] - revision: u64, + seen: Vec>, } /// A photo of the folder and its file. @@ -147,7 +147,7 @@ impl SyncInput { } let photos = ids.iter().filter_map(|id| s.catalog.photo(*id).map(|p| (Arc::clone(p), s.sidecar_naming(*id)))).collect(); let (scan, _) = ScanInput::new(s, &[path.to_string()]); - Ok(SyncInput { folder: path.to_string(), revision: s.catalog.revision, scan, photos, labels: s.catalog.clone() }) + Ok(SyncInput { folder: path.to_string(), scan, photos, labels: s.catalog.clone() }) } } @@ -157,9 +157,10 @@ impl SyncInput { /// the look for new files (which probes them on its own threads) beside the checks of the /// library's photos, which run on several threads too. Stops early when `progress.cancel` is set. pub fn scan_with(input: SyncInput, progress: &ScanProgress) -> FolderChanges { - let SyncInput { folder, revision, scan, photos, labels } = input; + let SyncInput { folder, scan, photos, labels } = input; + let seen: Vec> = photos.iter().map(|(p, _)| Arc::clone(p)).collect(); if !cfg!(target_arch = "wasm32") && !Path::new(&folder).is_dir() { - return FolderChanges { path: folder, revision, offline: true, ..Default::default() }; + return FolderChanges { path: folder, seen, offline: true, ..Default::default() }; } // (no threads on the web: one after the other there) #[cfg(target_arch = "wasm32")] @@ -173,7 +174,7 @@ pub fn scan_with(input: SyncInput, progress: &ScanProgress) -> FolderChanges { // (a check that panicked counts as nothing found; the scan still reports the rest) (out, checks.join().unwrap_or_default()) }); - let mut changes = FolderChanges { path: folder, revision, ..Default::default() }; + let mut changes = FolderChanges { path: folder, seen, ..Default::default() }; for ((p, _), check) in photos.iter().zip(checks) { let Some(file) = crate::cmd::missing::checked_path(p) else { continue }; let found = FolderPhoto { id: p.id.0, path: file.to_string() }; @@ -290,12 +291,21 @@ fn not_there(path: &str) -> EngineError { } impl Session { - /// The last scan ([`Session::folder_changes`]) if it is of folder `path` and nothing changed - /// in the library since: what the person was shown is what gets done. Used once. + /// The last scan ([`Session::folder_changes`]) if it is of folder `path` and still current: + /// the folder holds the same photos, none of them changed. What the person was shown is what + /// gets done. Used once. (Changes elsewhere in the library don't matter.) pub fn take_folder_changes(&mut self, path: &str) -> Option { let c = self.folder_changes.take()?; - let same = lightcraft_catalog::query::folder_key(&c.path) == lightcraft_catalog::query::folder_key(path); - (same && c.revision == self.catalog.revision).then_some(c) + if lightcraft_catalog::query::folder_key(&c.path) != lightcraft_catalog::query::folder_key(path) { + return None; + } + let f = Filter { library_folder: Some(path.to_string()), ..Default::default() }; + let mut now: Vec = self.catalog.query(&f, &Sort::default()); + let mut then: Vec = c.seen.iter().map(|p| p.id).collect(); + now.sort(); + then.sort(); + let unchanged = now == then && c.seen.iter().all(|p| self.catalog.photo(p.id).is_some_and(|q| Arc::ptr_eq(p, q))); + unchanged.then_some(c) } } diff --git a/crates/engine/src/tests_sync.rs b/crates/engine/src/tests_sync.rs index 411ded3ab..583e448d2 100644 --- a/crates/engine/src/tests_sync.rs +++ b/crates/engine/src/tests_sync.rs @@ -361,3 +361,35 @@ fn synchronizing_is_one_undo_step_even_with_a_full_undo_history() { assert_eq!(undo["undone"], "Synchronize Folder"); assert_eq!(holdings(&s), before, "one step undoes the import and the removal"); } + +#[test] +fn a_scan_stays_current_while_only_other_folders_change() { + let dir = Scratch::new("current"); + let mut s = library(&dir); + write_png(&dir.path("other/x.png"), 9); + s.execute("library.import", &json!({"paths": [dir.path("other")]})).unwrap(); + write_png(&dir.path("trip/c.png"), 3); + scan(&mut s, &dir.path("trip")); + // something elsewhere in the library changes while the dialog is open (a background import…) + let x = s.catalog.photos().find(|p| p.file_name == "x.png").unwrap().id; + s.execute("photo.rate", &json!({"ids": [x.0], "rating": 3})).unwrap(); + let r = s.execute("folder.synchronize", &json!({"path": dir.path("trip"), "scanned": true})).unwrap(); + assert_eq!(r["imported"], 1, "{r}"); +} + +#[test] +fn a_change_to_the_folders_photos_makes_the_scan_stale() { + let dir = Scratch::new("stale-folder"); + let mut s = library(&dir); + write_png(&dir.path("trip/c.png"), 3); + scan(&mut s, &dir.path("trip")); + let a = s.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id; + s.execute("photo.rate", &json!({"ids": [a.0], "rating": 2})).unwrap(); + // the dialog asks for exactly what it showed: refused, never redone on the spot + let e = s.execute("folder.synchronize", &json!({"path": dir.path("trip"), "scanned": true})).unwrap_err().to_string(); + assert!(e.contains("scan again"), "{e}"); + assert_eq!(s.catalog.photos().filter(|p| p.in_library()).count(), 2, "nothing was done"); + // an agent that just asks to synchronize gets a fresh scan + let r = s.execute("folder.synchronize", &json!({"path": dir.path("trip")})).unwrap(); + assert_eq!(r["imported"], 1, "{r}"); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 61c226f1c..8f1b41822 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -245,6 +245,8 @@ pub struct LightcraftApp { pub scan: Option, /// A Synchronize Folder scan in progress (feeds its dialog). pub sync: Option, + /// `session.folder_changes` came from the Synchronize Folder dialog (and goes with it). + pub sync_owns_changes: bool, /// A Lightroom catalog inspect/import in progress. pub lightroom: Option, /// Last terminal Lightroom result, exposed by the command's status/wait response. @@ -317,6 +319,7 @@ impl LightcraftApp { import: None, scan: None, sync: None, + sync_owns_changes: false, lightroom: None, lightroom_last: None, export: None, diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index 2890ae1fc..9f124ae80 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -881,6 +881,7 @@ pub fn show(app: &mut LightcraftApp, ctx: &egui::Context) { | Dialog::NewAlbum { .. } | Dialog::NewSmartAlbum { .. } | Dialog::SmartRules { .. } + | Dialog::SynchronizeFolder { .. } ) => { app.toast(ctx, e) diff --git a/crates/ui-egui/src/sync.rs b/crates/ui-egui/src/sync.rs index d774076ae..456a1d492 100644 --- a/crates/ui-egui/src/sync.rs +++ b/crates/ui-egui/src/sync.rs @@ -50,12 +50,15 @@ pub fn open(app: &mut LightcraftApp, path: &str, name: &str, disk: bool) -> Resu Ok(()) } -/// Stop a running scan and let go of a finished one. +/// Stop a running scan and let go of a finished one the dialog made (one an agent made with +/// `folder.scanChanges` is the agent's). fn cancel(app: &mut LightcraftApp) { if let Some(t) = app.sync.take() { t.progress.cancel.store(true, Ordering::Relaxed); } - app.session.folder_changes = None; + if std::mem::take(&mut app.sync_owns_changes) { + app.session.folder_changes = None; + } } /// Collect a finished scan into the dialog (called every frame). A scan whose dialog was closed @@ -66,7 +69,7 @@ pub fn poll(app: &mut LightcraftApp, ctx: &egui::Context) { _ => None, }; let Some(open_for) = open_for else { - if app.sync.is_some() || app.session.folder_changes.is_some() { + if app.sync.is_some() || app.sync_owns_changes { cancel(app); } return; @@ -98,6 +101,7 @@ pub fn poll(app: &mut LightcraftApp, ctx: &egui::Context) { moved: changes.moved.len(), }; app.session.folder_changes = Some(changes); + app.sync_owns_changes = true; if let Some(Dialog::SynchronizeFolder { counts, .. }) = &mut app.ui.dialog { *counts = Some(c); } @@ -166,8 +170,17 @@ pub fn confirm(app: &mut LightcraftApp, dlg: &Dialog) -> Result { }; let r = app.run( "folder.synchronize", - json!({"path": path, "disk": disk, "importNew": import_new, "relinkMoved": relink_moved, "removeMissing": remove_missing, "readMetadata": read_metadata}), + json!({"path": path, "disk": disk, "scanned": true, "importNew": import_new, "relinkMoved": relink_moved, "removeMissing": remove_missing, "readMetadata": read_metadata}), ); - app.session.folder_changes = None; + app.sync_owns_changes = false; + if r.is_err() { + // (the folder changed since the scan, or went offline): look again, in the background + let (path, disk) = (path.clone(), *disk); + let name = match dlg { + Dialog::SynchronizeFolder { name, .. } => name.clone(), + _ => String::new(), + }; + open(app, &path, &name, disk)?; + } r } diff --git a/crates/ui-egui/src/tests_sync.rs b/crates/ui-egui/src/tests_sync.rs index bc831ca00..631aaa6b5 100644 --- a/crates/ui-egui/src/tests_sync.rs +++ b/crates/ui-egui/src/tests_sync.rs @@ -149,3 +149,35 @@ fn cancel_leaves_the_library_as_it_was() { assert_eq!(in_library(&h), vec!["a.png", "b.png"]); assert!(h.app.session.folder_changes.is_none(), "the scan is let go"); } + +#[test] +fn an_agents_scan_is_kept_between_frames() { + let dir = Scratch::new("agent"); + let mut h = changed_folder(&dir); + let r = h.request("engine.execute", json!({"command": "folder.scanChanges", "params": {"path": dir.path("trip")}}), T); + assert_eq!(r["ok"], true, "{r}"); + for _ in 0..5 { + h.step(); + } + assert!(h.app.session.folder_changes.is_some(), "the app doesn't drop a scan it didn't make"); +} + +#[test] +fn a_scan_gone_stale_in_the_dialog_is_made_again_in_the_background() { + let dir = Scratch::new("restale"); + let mut h = changed_folder(&dir); + open_and_scan(&mut h, &dir.path("trip")); + // a photo of the folder changes while the dialog is open + let a = h.app.session.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id; + let r = h.request("engine.execute", json!({"command": "photo.rate", "params": {"ids": [a.0], "rating": 3}}), T); + assert_eq!(r["ok"], true, "{r}"); + let r = h.request("ui.dialog.confirm", json!({}), T); + assert_eq!(r["ok"], false, "not done on a stale scan: {r}"); + assert_eq!(in_library(&h), vec!["a.png", "b.png"]); + assert!(matches!(h.app.ui.dialog, Some(Dialog::SynchronizeFolder { .. })), "the dialog stays open"); + let rescanned = h.step_until(T, |h| matches!(&h.app.ui.dialog, Some(Dialog::SynchronizeFolder { counts: Some(_), .. }))); + assert!(rescanned, "and scans again"); + let r = h.request("ui.dialog.confirm", json!({}), T); + assert_eq!(r["ok"], true, "{r}"); + assert_eq!(in_library(&h), vec!["a.png", "b.png", "c.png"]); +}