From 9a109461103375723db50473c2562c7195eea36c Mon Sep 17 00:00:00 2001 From: clover caruso Date: Sat, 10 Oct 2026 19:02:40 -0700 Subject: [PATCH] fix: keep cached mobile notebook contents available offline Retain notebook objects after failed cloud-folder listings, reuse cached section identities when iCloud replaces files with placeholders, and resume cached sections while the source file is absent. Preserve canonical parent and symlink boundary checks. Cover placeholder eviction, queued offline edits, restoration, and links outside the notebook. Fixes #110 Assisted-by: gpt-6.1-sol --- apps/ios/Snowbound/Notebooks.swift | 14 ++++--- crates/mobile/src/tests.rs | 59 ++++++++++++++++++++++++++++ crates/notebook/src/discover.rs | 6 +-- crates/notebook/src/session.rs | 15 ++++++- crates/notebook/src/session/tests.rs | 19 +++++++++ crates/notebook/tests/discovery.rs | 30 +++++++++++++- 6 files changed, 129 insertions(+), 14 deletions(-) diff --git a/apps/ios/Snowbound/Notebooks.swift b/apps/ios/Snowbound/Notebooks.swift index 0474147c78e2e58412ea1fe8ec14217bfdffc191..2d468a4463f41c3d9826762dd5bc93abd9942798 100644 --- a/apps/ios/Snowbound/Notebooks.swift +++ b/apps/ios/Snowbound/Notebooks.swift @@ -523,9 +523,10 @@ enum Notebooks { list = [] return [] } + guard let names = notebooks(in: folder) else { return [] } var added: [Notebook] = [] list = ordered( - notebooks(in: folder).map { [list] name in + names.map { [list] name in if let kept = list.first(where: { $0.id == prefix + name }) { return kept } let notebook = Notebook( id: prefix + name, name: (name as NSString).deletingPathExtension, source: source(name)) @@ -564,15 +565,16 @@ enum Notebooks { /// Whether Snowbound's folder in iCloud Drive holds other notebooks than the list shows, as /// when another device added or removed one. static var inCloudChanged: Bool { - guard let folder = ICloud.documents, scripted == nil else { return false } - return notebooks(in: folder).map { "iCloud/" + $0 } != inCloud.map(\.id) + guard let folder = ICloud.documents, scripted == nil, let names = notebooks(in: folder) else { return false } + return names.map { "iCloud/" + $0 } != inCloud.map(\.id) } /// The notebook folders and section files in `folder`, by name, as Files orders them. - private static func notebooks(in folder: URL) -> [String] { - let listed = try? FileManager.default.contentsOfDirectory( + private static func notebooks(in folder: URL) -> [String]? { + guard let listed = try? FileManager.default.contentsOfDirectory( at: folder, includingPropertiesForKeys: [.isDirectoryKey], options: .skipsHiddenFiles) - return (listed ?? []).filter { url in + else { return nil } + return listed.filter { url in (try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) == true || url.pathExtension.lowercased() == "one" } diff --git a/crates/mobile/src/tests.rs b/crates/mobile/src/tests.rs index b84331cb5d08c06eba79cefa53c39812cf384353..a7c91fb2b9fb50020232d8b3c7594e07b654fcf4 100644 --- a/crates/mobile/src/tests.rs +++ b/crates/mobile/src/tests.rs @@ -317,6 +317,65 @@ fn typing_is_stored_in_the_cache_and_published_to_the_file() { assert!(canvas::search::page_text(&page).contains("Saved Collapsed parent")); } +#[test] +fn cached_cloud_sections_reopen_with_offline_edits_after_eviction() { + let directory = tempfile::tempdir().unwrap(); + let root = directory.path().join("notebook"); + let cache = directory.path().join("cache"); + let creation = onestore::PageCreation::new(None, Some("Kept offline"), "Fixture").unwrap(); + let notebook = + notebook::session::Notebook::create(&root, &cache, 0x00f0_a0c0, &creation).unwrap(); + let path = notebook.catalog().sections[0].path.clone(); + drop(notebook); + let library = Arc::new(Library::open(&root, &cache, false).unwrap()); + sb_library_set_offline(&library, true); + let section = open(&library, &path); + let space = space(§ion, "Kept offline"); + section + .shared + .apply(Edit { + at: library::filetime(), + ops: vec![Op::Page { + space, + op: PageOp::Color(Some(0x00ef_cdab)), + }], + }) + .unwrap(); + let shared = Arc::clone(§ion.shared); + drop((section, library)); + Arc::into_inner(shared).unwrap().section.close().unwrap(); + let file = root.join(&path); + let held = directory.path().join("held.one"); + std::fs::rename(&file, &held).unwrap(); + let placeholder = root.join(format!(".{path}.icloud")); + std::fs::write(&placeholder, b"placeholder").unwrap(); + + let library = Arc::new(Library::open(&root, &cache, false).unwrap()); + sb_library_set_offline(&library, true); + for _ in 0..2 { + let tabs = library.tabs().unwrap(); + assert_eq!(tabs[0].path, path); + assert!(tabs[0].readable && !tabs[0].downloading); + } + let section = open(&library, &path); + assert_eq!(section.rows().unwrap()[0].title, "Kept offline"); + assert_eq!( + section.shared.page(space).unwrap().0.color, + Some(0x00ef_cdab) + ); + assert_eq!(section.shared.section.pending().unwrap().len(), 1); + assert!(!file.exists()); + + std::fs::rename(held, &file).unwrap(); + std::fs::remove_file(placeholder).unwrap(); + assert!(library.tabs().unwrap()[0].readable); + sb_library_set_offline(&library, false); + assert!(section.flush(Duration::from_secs(20))); + let arena = onestore::Arena::default(); + let stored = onestore::Section::open(&arena, onestore::read_file(file).unwrap()).unwrap(); + assert_eq!(stored.page(space).unwrap().color, Some(0x00ef_cdab)); +} + #[test] fn a_new_page_and_subpage_list_after_their_parent_and_delete_to_the_recycle_bin() { let (directory, root) = copy("m6/native-features-01/notebook"); diff --git a/crates/notebook/src/discover.rs b/crates/notebook/src/discover.rs index 7b194d230e5c223a053fc49c3e07e1a9ee6a929c..ceb22da656f51ee3113258e728d1a49e887dbcf9 100644 --- a/crates/notebook/src/discover.rs +++ b/crates/notebook/src/discover.rs @@ -408,11 +408,9 @@ fn scan( }; let reused = cached .get(&child) - .filter(|known| known.listed == entry.listed) + .filter(|known| entry.kind == EntryKind::Evicted || known.listed == entry.listed) .and_then(|known| Some((known.file_id()?, known))); - // An evicted file listed as it was last read lists as then. Without an evicted TOC the - // folder lists in path order; nothing writes a second one, as its placeholder, or the - // dataless file itself, blocks creating it. + // A placeholder's metadata describes the placeholder, not the cached section. if entry.kind == EntryKind::Evicted && reused.is_none() { if expected == FileType::Section { result.unavailable.push(Unavailable { diff --git a/crates/notebook/src/session.rs b/crates/notebook/src/session.rs index 84d642abec9b0bc0423898bbd8f7b65671f3b4a7..107153511fef8e69ffd93154e814538dcec39f8c 100644 --- a/crates/notebook/src/session.rs +++ b/crates/notebook/src/session.rs @@ -1651,8 +1651,19 @@ impl Notebook { ) .into()); }; - // A section replaced by a link out of the notebook is not the catalog's section. - let file = fs::canonicalize(root.join(path))?; + let file = root.join(path); + // Evicted files resume their replicas; existing links still resolve within the root. + let file = match fs::symlink_metadata(&file) { + Err(error) if error.kind() == io::ErrorKind::NotFound => fs::canonicalize( + file.parent() + .ok_or(io::Error::from(io::ErrorKind::InvalidInput))?, + )? + .join( + file.file_name() + .ok_or(io::Error::from(io::ErrorKind::InvalidInput))?, + ), + _ => fs::canonicalize(&file)?, + }; if !file.starts_with(root) { return Err(io::Error::from(io::ErrorKind::PermissionDenied).into()); } diff --git a/crates/notebook/src/session/tests.rs b/crates/notebook/src/session/tests.rs index 06490e7945375ff2795823c74af7df3559751e40..4cda501114d64f6b1e83eda52cc8bdc1700e3525 100644 --- a/crates/notebook/src/session/tests.rs +++ b/crates/notebook/src/session/tests.rs @@ -1,5 +1,24 @@ use super::*; +#[cfg(unix)] +#[test] +fn cached_sections_refuse_links_outside_the_notebook_and_dangling_links() { + let temporary = tempfile::tempdir().unwrap(); + let folder = temporary.path().join("notebook"); + let cache = temporary.path().join("cache"); + let creation = PageCreation::new(None, Some("Mine"), "Fixture").unwrap(); + let notebook = Notebook::create(&folder, &cache, 0x00f0_a0c0, &creation).unwrap(); + let path = ¬ebook.catalog().sections[0].path; + notebook.section(path, || {}).unwrap().close().unwrap(); + let file = folder.join(path); + let outside = temporary.path().join("outside.one"); + fs::rename(&file, &outside).unwrap(); + std::os::unix::fs::symlink(&outside, &file).unwrap(); + assert!(notebook.section(path, || {}).is_err()); + fs::remove_file(outside).unwrap(); + assert!(notebook.section(path, || {}).is_err()); +} + /// A mounted notebook where another writer commits `revision` to the section file just /// before Snowbound's rewritten file takes its place. struct Racing { diff --git a/crates/notebook/tests/discovery.rs b/crates/notebook/tests/discovery.rs index 5d9689260efbb2f07c9c59bd4581399c2120baf3..76d14d80a44707492fa77f6f56f9e6e2668b9d48 100644 --- a/crates/notebook/tests/discovery.rs +++ b/crates/notebook/tests/discovery.rs @@ -197,6 +197,33 @@ fn evicted_files_list_under_their_names_until_downloaded() { assert!(!root.join("Open Notebook.onetoc2").exists()); } +#[test] +fn cached_sections_and_tocs_survive_eviction_to_different_placeholder_metadata() { + let root = tempfile::tempdir().unwrap(); + fixture(root.path()); + let mut cache = Cache::default(); + let mut source = Local::open(root.path()).unwrap(); + let before = cache.discover(&mut source, limits()).unwrap(); + for name in ["one.one", "Open Notebook.onetoc2"] { + fs::remove_file(root.path().join(name)).unwrap(); + fs::write(root.path().join(format!(".{name}.icloud")), b"placeholder").unwrap(); + } + for _ in 0..2 { + let evicted = cache.discover(&mut source, limits()).unwrap(); + assert_eq!(evicted.sections[0].file_id, before.sections[0].file_id); + assert_eq!(evicted.order, before.order); + assert!(evicted.toc.is_some() && evicted.unavailable.is_empty()); + } + fs::write( + root.path().join("one.one"), + onestore::create_section("one.one", "Downloaded change", "Fixture").unwrap(), + ) + .unwrap(); + touch(&root.path().join("one.one"), 1_000_000_000); + let downloaded = cache.discover(&mut source, limits()).unwrap(); + assert_ne!(downloaded.sections[0].file_id, before.sections[0].file_id); +} + /// A source whose files list as macOS lists those iCloud Drive evicted: dataless, under their /// names and listings, and never read. struct Dataless(Local); @@ -215,8 +242,7 @@ impl Source for Dataless { } } -/// A dataless file lists as its last read found it, where it lists unchanged since, and as not -/// downloaded otherwise. +/// A dataless file lists as its last read found it, and waits for download without a cache. #[test] fn dataless_files_list_as_last_read_or_wait_for_their_download() { let root = tempfile::tempdir().unwrap(); -- 2.54.0