| author | |
| committer | |
| log | 9a109461103375723db50473c2562c7195eea36c |
| tree | 03bd6f82500086f311815dc1299e8e520693d555 |
| parent | 34a0250c70ab90c59387f05aa7840d58535392a6 |
| signature | Signed by SSH key SHA256:52mNGHRsVFBDED9IAX5pe+LRWUefqTbxEReunq21QvU (~clover) |
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-sol6 files changed, 129 insertions(+), 14 deletions(-)
apps/ios/Snowbound/Notebooks.swift+8-6| ... | @@ -523,9 +523,10 @@ enum Notebooks { | ... | @@ -523,9 +523,10 @@ enum Notebooks { |
| 523 | list = [] | 523 | list = [] |
| 524 | return [] | 524 | return [] |
| 525 | } | 525 | } |
| 526 | guard let names = notebooks(in: folder) else { return [] } | ||
| 526 | var added: [Notebook] = [] | 527 | var added: [Notebook] = [] |
| 527 | list = ordered( | 528 | list = ordered( |
| 528 | notebooks(in: folder).map { [list] name in | 529 | names.map { [list] name in |
| 529 | if let kept = list.first(where: { $0.id == prefix + name }) { return kept } | 530 | if let kept = list.first(where: { $0.id == prefix + name }) { return kept } |
| 530 | let notebook = Notebook( | 531 | let notebook = Notebook( |
| 531 | id: prefix + name, name: (name as NSString).deletingPathExtension, source: source(name)) | 532 | id: prefix + name, name: (name as NSString).deletingPathExtension, source: source(name)) |
| ... | @@ -564,15 +565,16 @@ enum Notebooks { | ... | @@ -564,15 +565,16 @@ enum Notebooks { |
| 564 | /// Whether Snowbound's folder in iCloud Drive holds other notebooks than the list shows, as | 565 | /// Whether Snowbound's folder in iCloud Drive holds other notebooks than the list shows, as |
| 565 | /// when another device added or removed one. | 566 | /// when another device added or removed one. |
| 566 | static var inCloudChanged: Bool { | 567 | static var inCloudChanged: Bool { |
| 567 | guard let folder = ICloud.documents, scripted == nil else { return false } | 568 | guard let folder = ICloud.documents, scripted == nil, let names = notebooks(in: folder) else { return false } |
| 568 | return notebooks(in: folder).map { "iCloud/" + $0 } != inCloud.map(\.id) | 569 | return names.map { "iCloud/" + $0 } != inCloud.map(\.id) |
| 569 | } | 570 | } |
| 570 | 571 | ||
| 571 | /// The notebook folders and section files in `folder`, by name, as Files orders them. | 572 | /// The notebook folders and section files in `folder`, by name, as Files orders them. |
| 572 | private static func notebooks(in folder: URL) -> [String] { | 573 | private static func notebooks(in folder: URL) -> [String]? { |
| 573 | let listed = try? FileManager.default.contentsOfDirectory( | 574 | guard let listed = try? FileManager.default.contentsOfDirectory( |
| 574 | at: folder, includingPropertiesForKeys: [.isDirectoryKey], options: .skipsHiddenFiles) | 575 | at: folder, includingPropertiesForKeys: [.isDirectoryKey], options: .skipsHiddenFiles) |
| 575 | return (listed ?? []).filter { url in | 576 | else { return nil } |
| 577 | return listed.filter { url in | ||
| 576 | (try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) == true | 578 | (try? url.resourceValues(forKeys: [.isDirectoryKey]).isDirectory) == true |
| 577 | || url.pathExtension.lowercased() == "one" | 579 | || url.pathExtension.lowercased() == "one" |
| 578 | } | 580 | } |
crates/mobile/src/tests.rs+59| ... | @@ -317,6 +317,65 @@ fn typing_is_stored_in_the_cache_and_published_to_the_file() { | ... | @@ -317,6 +317,65 @@ fn typing_is_stored_in_the_cache_and_published_to_the_file() { |
| 317 | assert!(canvas::search::page_text(&page).contains("Saved Collapsed parent")); | 317 | assert!(canvas::search::page_text(&page).contains("Saved Collapsed parent")); |
| 318 | } | 318 | } |
| 319 | 319 | ||
| 320 | #[test] | ||
| 321 | fn cached_cloud_sections_reopen_with_offline_edits_after_eviction() { | ||
| 322 | let directory = tempfile::tempdir().unwrap(); | ||
| 323 | let root = directory.path().join("notebook"); | ||
| 324 | let cache = directory.path().join("cache"); | ||
| 325 | let creation = onestore::PageCreation::new(None, Some("Kept offline"), "Fixture").unwrap(); | ||
| 326 | let notebook = | ||
| 327 | notebook::session::Notebook::create(&root, &cache, 0x00f0_a0c0, &creation).unwrap(); | ||
| 328 | let path = notebook.catalog().sections[0].path.clone(); | ||
| 329 | drop(notebook); | ||
| 330 | let library = Arc::new(Library::open(&root, &cache, false).unwrap()); | ||
| 331 | sb_library_set_offline(&library, true); | ||
| 332 | let section = open(&library, &path); | ||
| 333 | let space = space(&section, "Kept offline"); | ||
| 334 | section | ||
| 335 | .shared | ||
| 336 | .apply(Edit { | ||
| 337 | at: library::filetime(), | ||
| 338 | ops: vec![Op::Page { | ||
| 339 | space, | ||
| 340 | op: PageOp::Color(Some(0x00ef_cdab)), | ||
| 341 | }], | ||
| 342 | }) | ||
| 343 | .unwrap(); | ||
| 344 | let shared = Arc::clone(&section.shared); | ||
| 345 | drop((section, library)); | ||
| 346 | Arc::into_inner(shared).unwrap().section.close().unwrap(); | ||
| 347 | let file = root.join(&path); | ||
| 348 | let held = directory.path().join("held.one"); | ||
| 349 | std::fs::rename(&file, &held).unwrap(); | ||
| 350 | let placeholder = root.join(format!(".{path}.icloud")); | ||
| 351 | std::fs::write(&placeholder, b"placeholder").unwrap(); | ||
| 352 | |||
| 353 | let library = Arc::new(Library::open(&root, &cache, false).unwrap()); | ||
| 354 | sb_library_set_offline(&library, true); | ||
| 355 | for _ in 0..2 { | ||
| 356 | let tabs = library.tabs().unwrap(); | ||
| 357 | assert_eq!(tabs[0].path, path); | ||
| 358 | assert!(tabs[0].readable && !tabs[0].downloading); | ||
| 359 | } | ||
| 360 | let section = open(&library, &path); | ||
| 361 | assert_eq!(section.rows().unwrap()[0].title, "Kept offline"); | ||
| 362 | assert_eq!( | ||
| 363 | section.shared.page(space).unwrap().0.color, | ||
| 364 | Some(0x00ef_cdab) | ||
| 365 | ); | ||
| 366 | assert_eq!(section.shared.section.pending().unwrap().len(), 1); | ||
| 367 | assert!(!file.exists()); | ||
| 368 | |||
| 369 | std::fs::rename(held, &file).unwrap(); | ||
| 370 | std::fs::remove_file(placeholder).unwrap(); | ||
| 371 | assert!(library.tabs().unwrap()[0].readable); | ||
| 372 | sb_library_set_offline(&library, false); | ||
| 373 | assert!(section.flush(Duration::from_secs(20))); | ||
| 374 | let arena = onestore::Arena::default(); | ||
| 375 | let stored = onestore::Section::open(&arena, onestore::read_file(file).unwrap()).unwrap(); | ||
| 376 | assert_eq!(stored.page(space).unwrap().color, Some(0x00ef_cdab)); | ||
| 377 | } | ||
| 378 | |||
| 320 | #[test] | 379 | #[test] |
| 321 | fn a_new_page_and_subpage_list_after_their_parent_and_delete_to_the_recycle_bin() { | 380 | fn a_new_page_and_subpage_list_after_their_parent_and_delete_to_the_recycle_bin() { |
| 322 | let (directory, root) = copy("m6/native-features-01/notebook"); | 381 | let (directory, root) = copy("m6/native-features-01/notebook"); |
crates/notebook/src/discover.rs+2-4| ... | @@ -408,11 +408,9 @@ fn scan( | ... | @@ -408,11 +408,9 @@ fn scan( |
| 408 | }; | 408 | }; |
| 409 | let reused = cached | 409 | let reused = cached |
| 410 | .get(&child) | 410 | .get(&child) |
| 411 | .filter(|known| known.listed == entry.listed) | 411 | .filter(|known| entry.kind == EntryKind::Evicted || known.listed == entry.listed) |
| 412 | .and_then(|known| Some((known.file_id()?, known))); | 412 | .and_then(|known| Some((known.file_id()?, known))); |
| 413 | // An evicted file listed as it was last read lists as then. Without an evicted TOC the | 413 | // A placeholder's metadata describes the placeholder, not the cached section. |
| 414 | // folder lists in path order; nothing writes a second one, as its placeholder, or the | ||
| 415 | // dataless file itself, blocks creating it. | ||
| 416 | if entry.kind == EntryKind::Evicted && reused.is_none() { | 414 | if entry.kind == EntryKind::Evicted && reused.is_none() { |
| 417 | if expected == FileType::Section { | 415 | if expected == FileType::Section { |
| 418 | result.unavailable.push(Unavailable { | 416 | result.unavailable.push(Unavailable { |
crates/notebook/src/session.rs+13-2| ... | @@ -1651,8 +1651,19 @@ impl Notebook { | ... | @@ -1651,8 +1651,19 @@ impl Notebook { |
| 1651 | ) | 1651 | ) |
| 1652 | .into()); | 1652 | .into()); |
| 1653 | }; | 1653 | }; |
| 1654 | // A section replaced by a link out of the notebook is not the catalog's section. | 1654 | let file = root.join(path); |
| 1655 | let file = fs::canonicalize(root.join(path))?; | 1655 | // Evicted files resume their replicas; existing links still resolve within the root. |
| 1656 | let file = match fs::symlink_metadata(&file) { | ||
| 1657 | Err(error) if error.kind() == io::ErrorKind::NotFound => fs::canonicalize( | ||
| 1658 | file.parent() | ||
| 1659 | .ok_or(io::Error::from(io::ErrorKind::InvalidInput))?, | ||
| 1660 | )? | ||
| 1661 | .join( | ||
| 1662 | file.file_name() | ||
| 1663 | .ok_or(io::Error::from(io::ErrorKind::InvalidInput))?, | ||
| 1664 | ), | ||
| 1665 | _ => fs::canonicalize(&file)?, | ||
| 1666 | }; | ||
| 1656 | if !file.starts_with(root) { | 1667 | if !file.starts_with(root) { |
| 1657 | return Err(io::Error::from(io::ErrorKind::PermissionDenied).into()); | 1668 | return Err(io::Error::from(io::ErrorKind::PermissionDenied).into()); |
| 1658 | } | 1669 | } |
crates/notebook/src/session/tests.rs+19| ... | @@ -1,5 +1,24 @@ | ... | @@ -1,5 +1,24 @@ |
| 1 | use super::*; | 1 | use super::*; |
| 2 | 2 | ||
| 3 | #[cfg(unix)] | ||
| 4 | #[test] | ||
| 5 | fn cached_sections_refuse_links_outside_the_notebook_and_dangling_links() { | ||
| 6 | let temporary = tempfile::tempdir().unwrap(); | ||
| 7 | let folder = temporary.path().join("notebook"); | ||
| 8 | let cache = temporary.path().join("cache"); | ||
| 9 | let creation = PageCreation::new(None, Some("Mine"), "Fixture").unwrap(); | ||
| 10 | let notebook = Notebook::create(&folder, &cache, 0x00f0_a0c0, &creation).unwrap(); | ||
| 11 | let path = &notebook.catalog().sections[0].path; | ||
| 12 | notebook.section(path, || {}).unwrap().close().unwrap(); | ||
| 13 | let file = folder.join(path); | ||
| 14 | let outside = temporary.path().join("outside.one"); | ||
| 15 | fs::rename(&file, &outside).unwrap(); | ||
| 16 | std::os::unix::fs::symlink(&outside, &file).unwrap(); | ||
| 17 | assert!(notebook.section(path, || {}).is_err()); | ||
| 18 | fs::remove_file(outside).unwrap(); | ||
| 19 | assert!(notebook.section(path, || {}).is_err()); | ||
| 20 | } | ||
| 21 | |||
| 3 | /// A mounted notebook where another writer commits `revision` to the section file just | 22 | /// A mounted notebook where another writer commits `revision` to the section file just |
| 4 | /// before Snowbound's rewritten file takes its place. | 23 | /// before Snowbound's rewritten file takes its place. |
| 5 | struct Racing { | 24 | struct Racing { |
crates/notebook/tests/discovery.rs+28-2| ... | @@ -197,6 +197,33 @@ fn evicted_files_list_under_their_names_until_downloaded() { | ... | @@ -197,6 +197,33 @@ fn evicted_files_list_under_their_names_until_downloaded() { |
| 197 | assert!(!root.join("Open Notebook.onetoc2").exists()); | 197 | assert!(!root.join("Open Notebook.onetoc2").exists()); |
| 198 | } | 198 | } |
| 199 | 199 | ||
| 200 | #[test] | ||
| 201 | fn cached_sections_and_tocs_survive_eviction_to_different_placeholder_metadata() { | ||
| 202 | let root = tempfile::tempdir().unwrap(); | ||
| 203 | fixture(root.path()); | ||
| 204 | let mut cache = Cache::default(); | ||
| 205 | let mut source = Local::open(root.path()).unwrap(); | ||
| 206 | let before = cache.discover(&mut source, limits()).unwrap(); | ||
| 207 | for name in ["one.one", "Open Notebook.onetoc2"] { | ||
| 208 | fs::remove_file(root.path().join(name)).unwrap(); | ||
| 209 | fs::write(root.path().join(format!(".{name}.icloud")), b"placeholder").unwrap(); | ||
| 210 | } | ||
| 211 | for _ in 0..2 { | ||
| 212 | let evicted = cache.discover(&mut source, limits()).unwrap(); | ||
| 213 | assert_eq!(evicted.sections[0].file_id, before.sections[0].file_id); | ||
| 214 | assert_eq!(evicted.order, before.order); | ||
| 215 | assert!(evicted.toc.is_some() && evicted.unavailable.is_empty()); | ||
| 216 | } | ||
| 217 | fs::write( | ||
| 218 | root.path().join("one.one"), | ||
| 219 | onestore::create_section("one.one", "Downloaded change", "Fixture").unwrap(), | ||
| 220 | ) | ||
| 221 | .unwrap(); | ||
| 222 | touch(&root.path().join("one.one"), 1_000_000_000); | ||
| 223 | let downloaded = cache.discover(&mut source, limits()).unwrap(); | ||
| 224 | assert_ne!(downloaded.sections[0].file_id, before.sections[0].file_id); | ||
| 225 | } | ||
| 226 | |||
| 200 | /// A source whose files list as macOS lists those iCloud Drive evicted: dataless, under their | 227 | /// A source whose files list as macOS lists those iCloud Drive evicted: dataless, under their |
| 201 | /// names and listings, and never read. | 228 | /// names and listings, and never read. |
| 202 | struct Dataless(Local); | 229 | struct Dataless(Local); |
| ... | @@ -215,8 +242,7 @@ impl Source for Dataless { | ... | @@ -215,8 +242,7 @@ impl Source for Dataless { |
| 215 | } | 242 | } |
| 216 | } | 243 | } |
| 217 | 244 | ||
| 218 | /// A dataless file lists as its last read found it, where it lists unchanged since, and as not | 245 | /// A dataless file lists as its last read found it, and waits for download without a cache. |
| 219 | /// downloaded otherwise. | ||
| 220 | #[test] | 246 | #[test] |
| 221 | fn dataless_files_list_as_last_read_or_wait_for_their_download() { | 247 | fn dataless_files_list_as_last_read_or_wait_for_their_download() { |
| 222 | let root = tempfile::tempdir().unwrap(); | 248 | let root = tempfile::tempdir().unwrap(); |