From eb83d23040116024afcab94d9f21fabceabf95a7 Mon Sep 17 00:00:00 2001 From: aiongg Date: Tue, 13 Oct 2026 14:22:27 +0000 Subject: [PATCH] fix(file): saving never reports success without a file (#741, #708) to_bytes returns a Result and every caller propagates it, so a document that can't be written is an error: the existing file, the unsaved state and the recovery checkpoint stay. Asset entries near or over 4 GiB are written with zip64 sizes. On the web, Save marks the document saved only after the browser download has started. --- ROADMAP.md | 1 + apps/designcraft-web/src/web.rs | 11 +++--- crates/engine/src/cmd/file.rs | 59 ++++++++++++++++++++++++++---- crates/engine/src/cmd/mod.rs | 2 +- crates/engine/src/cmd/package.rs | 2 +- crates/engine/src/cmd/path_type.rs | 2 +- crates/engine/src/tests.rs | 6 +-- crates/engine/src/tests_table.rs | 6 +-- crates/format/src/lib.rs | 23 ++++++++++++ crates/ui-egui/src/lib.rs | 7 ++-- crates/ui-egui/src/menus.rs | 32 ++++++++++++++-- 11 files changed, 123 insertions(+), 28 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 8a0f82071..3c85c1ef9 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -129,6 +129,7 @@ list) · [roadmap.md](docs/roadmap.md) (milestones, plan) · [parity-checklist.m Newest first. +- **2026-10-13** · Saving never reports success without a file: a document that can't be written is an error from Save, Save As, Save a Copy, Package, snippet export and the web serializer, leaving the existing file, the unsaved state and the recovery checkpoint as they were. Placed files over 4 GiB are written with zip64 sizes. A web Save whose browser download can't start reports the error and keeps the document unsaved (#741, #708). No readiness percentage change. - **2026-10-12** · macOS Dock Quit uses the same unsaved-document prompts as window and menu Quit. Repeated requests keep the current answers; Cancel retains every tab and unsaved recovery checkpoint, and completed Save/Don't Save answers exit through the ordinary event loop. No readiness percentage change. - **2026-10-12** · Orderly native exit writes the latest recovery state after finalizing current quit approvals; incomplete or stale plans keep their documents, and Undo to the saved state clears stale recovery. Unavailable or failed recovery is reported. Cancelable macOS Dock Quit remains open (#544). No readiness percentage change. - **2026-10-12** · Recovered documents keep a checkpoint until saved, closed or reverted, including a second crash before autosave. Restarted document IDs and explicit recovery folders preserve other snapshots; failed republishing retains the original. No readiness percentage change. diff --git a/apps/designcraft-web/src/web.rs b/apps/designcraft-web/src/web.rs index fe7cca4ae..43f6ece9e 100644 --- a/apps/designcraft-web/src/web.rs +++ b/apps/designcraft-web/src/web.rs @@ -140,11 +140,7 @@ fn services(inbox: Inbox, ctx: egui::Context) -> Services { // Exports ask for a name; the browser decides where the download goes. pick_save: Some(Box::new(|name: &str| Some(name.to_string()))), write: Some(Box::new(|name: &str, bytes: &[u8]| download(name, bytes))), - download: Some(Box::new(|name: &str, bytes: &[u8]| { - if let Err(e) = download(name, bytes) { - log::error!("download of {name} failed: {e}"); - } - })), + download: Some(Box::new(|name: &str, bytes: &[u8]| download(name, bytes))), inbox: Some(inbox), ..Default::default() } @@ -173,6 +169,9 @@ fn download(path: &str, bytes: &[u8]) -> Result<(), String> { let revoke = wasm_bindgen::closure::Closure::once_into_js(move || { web_sys::Url::revoke_object_url(&url).ok(); }); - window.set_timeout_with_callback_and_timeout_and_arguments_0(revoke.unchecked_ref(), 10_000).map_err(js)?; + // The download has started; a missed revoke only keeps the blob until the page closes. + if let Err(e) = window.set_timeout_with_callback_and_timeout_and_arguments_0(revoke.unchecked_ref(), 10_000) { + log::warn!("could not schedule revoking the download URL: {}", js(e)); + } Ok(()) } diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index a1919b89e..b2545b5b6 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -27,7 +27,7 @@ pub fn specs() -> Vec { let path = str_param(p, "path").ok_or_else(|| bad("file.saveACopy", "missing `path`"))?.to_string(); let mut copy = (*s.doc()?.doc).clone(); super::datamerge::refresh_relative_paths(&mut copy, std::path::Path::new(&path)); - let bytes = to_bytes(©); + let bytes = to_bytes(©)?; #[cfg(not(target_arch = "wasm32"))] crate::atomic::write_file(&path, &bytes)?; Ok(json!({"path": path, "bytes": bytes.len()})) @@ -49,7 +49,7 @@ pub fn specs() -> Vec { let st = s.doc()?; // Preview fill is session state. Serialize the unfilled template. let doc = st.preview_stash.as_deref().unwrap_or(st.doc.as_ref()); - let bytes = to_bytes(doc); + let bytes = to_bytes(doc)?; Ok(json!({"base64": base64_encode(&bytes), "bytes": bytes.len()})) }), cmd!(noundo "file.close", "Close", ["File"], Some("Cmd+W"), "{index?}", has_doc, |s, p| { @@ -109,8 +109,19 @@ pub fn specs() -> Vec { ] } -pub fn to_bytes(d: &Document) -> Vec { - designcraft_format::save(d).unwrap_or_default() +#[cfg(test)] +thread_local! { + /// Makes [`to_bytes`] fail on this thread, standing in for writer errors that need gigabytes to reach. + pub(crate) static FAIL_TO_BYTES: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + +/// The document as `.designcraft` bytes, or an error when it can't be written; never an empty file. +pub fn to_bytes(d: &Document) -> Result> { + #[cfg(test)] + if FAIL_TO_BYTES.with(std::cell::Cell::get) { + return Err(EngineError::Other("could not write the document: injected failure".into())); + } + designcraft_format::save(d).map_err(|e| EngineError::Other(format!("could not write the document: {e}"))) } pub fn from_bytes(b: &[u8]) -> Result { @@ -244,7 +255,7 @@ fn file_save(s: &mut Session, p: &Value) -> Result { super::datamerge::refresh_relative_paths(&mut saved, std::path::Path::new(&path)); let paths_changed = saved.data_merge != st.doc.data_merge; let saved = if paths_changed { Arc::new(saved) } else { st.doc.clone() }; - let bytes = to_bytes(&saved); + let bytes = to_bytes(&saved)?; #[cfg(not(target_arch = "wasm32"))] crate::atomic::write_file(&path, &bytes)?; if paths_changed { @@ -513,12 +524,12 @@ fn place_drop(s: &mut Session, p: &Value) -> Result { /// The selection as `.designcraft` snippet bytes. pub(crate) fn snippet_bytes(s: &mut Session) -> Result> { - Ok(to_bytes(&super::edit::clip_doc(s)?)) + to_bytes(&super::edit::clip_doc(s)?) } fn snippet_export(s: &mut Session, p: &Value) -> Result { let d = super::edit::clip_doc(s)?; - let bytes = to_bytes(&d); + let bytes = to_bytes(&d)?; match str_param(p, "path") { Some(path) => { #[cfg(not(target_arch = "wasm32"))] @@ -767,6 +778,40 @@ mod save_tests { assert!(e.to_string().contains("missing"), "{e}"); let _ = std::fs::remove_dir_all(&dir); } + + /// A document that can't be written is an error: the file on disk, the unsaved state and the + /// recovery checkpoint all stay as they were. + #[test] + fn a_save_that_cannot_write_the_document_changes_nothing() { + let dir = std::env::temp_dir().join(format!("dc-save-fails-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("doc.designcraft"); + let recovery = dir.join("recovery"); + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + s.execute("file.save", &json!({"path": path.to_string_lossy()})).unwrap(); + let before = std::fs::read(&path).unwrap(); + s.execute("frame.create", &json!({"rect": [36, 36, 300, 200], "content": "text", "text": "Unsaved", "caret": false})).unwrap(); + s.execute("file.recovery.save", &json!({"dir": recovery.to_string_lossy()})).unwrap(); + let checkpoint = recovery.join(format!("{}.designcraft", s.doc().unwrap().uid)); + assert!(checkpoint.exists()); + + super::FAIL_TO_BYTES.with(|f| f.set(true)); + let save = s.execute("file.save", &json!({})); + let copy = s.execute("file.saveACopy", &json!({"path": dir.join("copy.designcraft").to_string_lossy()})); + let serialized = s.execute("file.serialize", &json!({})); + super::FAIL_TO_BYTES.with(|f| f.set(false)); + + let e = save.unwrap_err(); + assert!(e.to_string().contains("could not write the document"), "{e}"); + assert!(copy.is_err() && serialized.is_err(), "no empty copy and no empty download"); + assert_eq!(std::fs::read(&path).unwrap(), before, "the saved file is untouched"); + assert!(!dir.join("copy.designcraft").exists()); + assert!(s.doc().unwrap().is_dirty(), "the document still has unsaved changes"); + assert!(checkpoint.exists(), "the recovery checkpoint stays"); + let _ = std::fs::remove_dir_all(&dir); + } } #[cfg(test)] diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 31e4544f4..442aeb352 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -302,7 +302,7 @@ pub(crate) fn ok() -> Result { Ok(Value::Null) } -pub fn file_bytes(d: &designcraft_doc::Document) -> Vec { +pub fn file_bytes(d: &designcraft_doc::Document) -> Result> { file::to_bytes(d) } pub fn file_from(b: &[u8]) -> Result { diff --git a/crates/engine/src/cmd/package.rs b/crates/engine/src/cmd/package.rs index 25f09537c..b2bdc3a8d 100644 --- a/crates/engine/src/cmd/package.rs +++ b/crates/engine/src/cmd/package.rs @@ -365,7 +365,7 @@ fn package(s: &mut Session, p: &Value) -> Result { let links_dir = root.child("Links")?; let (packed, mut files) = write_links(&d, &root, &links_dir, None)?; let doc_path = package_output_path(&dir, &title, "designcraft")?; - root.write(&doc_path, &super::to_bytes(&packed))?; + root.write(&doc_path, &super::to_bytes(&packed)?)?; files.insert(0, doc_path); // The font files, and what the report says about each font. let db = designcraft_fonts::FontDb::global().scoped(d.font_scope); diff --git a/crates/engine/src/cmd/path_type.rs b/crates/engine/src/cmd/path_type.rs index ffd852f97..6926d464c 100644 --- a/crates/engine/src/cmd/path_type.rs +++ b/crates/engine/src/cmd/path_type.rs @@ -210,7 +210,7 @@ mod tests { assert!(s.execute("type.pathOptions", &json!({"id": id, "end": "far"})).is_err()); // Saved with the document. let d = s.doc().unwrap().doc.clone(); - let back = crate::cmd::file_from(&crate::cmd::file_bytes(&d)).unwrap(); + let back = crate::cmd::file_from(&crate::cmd::file_bytes(&d).unwrap()).unwrap(); let path = |d: &designcraft_doc::Document| d.item(designcraft_doc::ItemId(id)).unwrap().text_frame().unwrap().options.path.clone(); assert_eq!(path(&back), path(&d)); } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 17d1ed098..5ef9f7bec 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -173,7 +173,7 @@ fn sample_document_is_valid_and_composes() { let i = s.execute("document.inspect", &json!({})).unwrap(); assert!(i["stories"].as_array().unwrap().len() >= 8); // Save/load round trip. - let bytes = cmd::file_bytes(&s.doc().unwrap().doc); + let bytes = cmd::file_bytes(&s.doc().unwrap().doc).unwrap(); let back = cmd::file_from(&bytes).unwrap(); assert_eq!(back.page_count(), 4); assert_eq!(back.assets.len(), s.doc().unwrap().doc.assets.len()); @@ -326,7 +326,7 @@ fn hyperlinks_and_bookmarks_reach_the_pdf() { let pdf = hayro_syntax::Pdf::new(bytes).expect("valid pdf"); assert_eq!(pdf.pages().len(), 4); // Round trip through the native format keeps them. - let back = cmd::file_from(&cmd::file_bytes(&s.doc().unwrap().doc)).unwrap(); + let back = cmd::file_from(&cmd::file_bytes(&s.doc().unwrap().doc).unwrap()).unwrap(); assert_eq!(back.hyperlinks.len(), 2); assert_eq!(back.bookmarks.len(), 2); } @@ -338,7 +338,7 @@ fn serialized_document_opens_again() { s.execute("frame.create", &json!({"rect": [36, 36, 300, 200], "content": "text", "text": "Saved on the web"})).unwrap(); let r = s.execute("file.serialize", &json!({})).unwrap(); let bytes = cmd::base64_decode(r["base64"].as_str().expect("base64")); - assert_eq!(bytes, cmd::to_bytes(&s.doc().unwrap().doc)); + assert_eq!(bytes, cmd::to_bytes(&s.doc().unwrap().doc).unwrap()); s.execute("file.openBytes", &json!({"name": "copy", "base64": r["base64"]})).unwrap(); let d = s.execute("document.inspect", &json!({})).unwrap(); assert_eq!(d["stories"][0]["preview"], "Saved on the web"); diff --git a/crates/engine/src/tests_table.rs b/crates/engine/src/tests_table.rs index 3934aabec..0084d76e2 100644 --- a/crates/engine/src/tests_table.rs +++ b/crates/engine/src/tests_table.rs @@ -170,7 +170,7 @@ fn file_roundtrip_keeps_tables() { s.execute("table.insert", &json!({"rows": 2, "cols": 2})).unwrap(); s.execute("text.insert", &json!({"text": "A1"})).unwrap(); let d = s.doc().unwrap().doc.clone(); - let bytes = crate::cmd::file_bytes(&d); + let bytes = crate::cmd::file_bytes(&d).unwrap(); let back = crate::cmd::file_from(&bytes).unwrap(); back.check().unwrap(); let st = back.story(designcraft_doc::StoryId(sid)).unwrap(); @@ -230,7 +230,7 @@ fn explicit_cell_edges_win_imported_priorities_and_keep_undo_and_file_roundtrip( assert_eq!(table_for(&s, sid, tid), &before); s.execute("edit.redo", &json!({})).unwrap(); assert_eq!(table_for(&s, sid, tid), &after); - let bytes = crate::cmd::file_bytes(&s.doc().unwrap().doc); + let bytes = crate::cmd::file_bytes(&s.doc().unwrap().doc).unwrap(); let back = crate::cmd::file_from(&bytes).unwrap(); assert_eq!(back.story(doc::StoryId(sid)).unwrap().tables.get(&tid).unwrap().as_ref(), &after); check(&s); @@ -514,7 +514,7 @@ fn split_selected_block_and_merged_cells_preserves_geometry_and_history() { s.execute("edit.redo", &json!({})).unwrap(); assert!(std::sync::Arc::ptr_eq(&s.doc().unwrap().doc, &after_doc)); assert_eq!(s.doc().unwrap().selection, after_selection); - let bytes = crate::cmd::file_bytes(&s.doc().unwrap().doc); + let bytes = crate::cmd::file_bytes(&s.doc().unwrap().doc).unwrap(); let reopened = crate::cmd::file_from(&bytes).unwrap(); reopened.check().unwrap(); assert_eq!(reopened.story(doc::StoryId(sid)).unwrap().tables.get(&tid).unwrap().as_ref(), &after); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 408bb3ccc..4d43eca2b 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -46,6 +46,14 @@ fn ext_for(mime: &str) -> &'static str { /// Serialize a document to `.designcraft` bytes. The file holds each distinct placed file once /// and leaves out assets nothing references; `doc` itself keeps them (undo can bring them back). pub fn save(doc: &Document) -> Result, FormatError> { + save_with_large_entries_from(doc, LARGE_ENTRY) +} + +/// Asset entries this size or larger are written with zip64 sizes. Zip rejects an entry over +/// 4 GiB written without them; the margin covers deflate output that is larger than its input. +const LARGE_ENTRY: usize = (u32::MAX as usize).saturating_sub(64 << 20); + +fn save_with_large_entries_from(doc: &Document, large_entry: usize) -> Result, FormatError> { let mut compact = doc.clone(); compact.compact_assets(); let doc = &compact; @@ -69,6 +77,7 @@ pub fn save(doc: &Document) -> Result, FormatError> { } // Already-compressed images are stored as-is. let opts = if matches!(a.mime.as_str(), "image/png" | "image/jpeg" | "image/gif" | "image/webp") { stored } else { deflate }; + let opts = opts.large_file(a.data.len() >= large_entry); z.start_file(format!("assets/{}.{}", id.0, ext_for(&a.mime)), opts).map_err(io)?; z.write_all(&a.data).map_err(|e| FormatError::Io(e.to_string()))?; } @@ -293,6 +302,20 @@ mod tests { assert_eq!(back.stories, d.stories); } + /// Assets over the zip size limit are written with zip64 sizes (scaled down here: the real + /// threshold is near 4 GiB); smaller ones are written exactly as before. + #[test] + fn large_assets_are_written_with_zip64_sizes_and_open_again() { + let mut d = Document::new(&NewDocument::default()); + let aid = png_asset(&mut d, &[5; 64]); + place(&mut d, aid, 0.0); + let normal = save(&d).unwrap(); + assert_eq!(normal, save_with_large_entries_from(&d, 65).unwrap(), "an asset under the threshold is unchanged"); + let large = save_with_large_entries_from(&d, 64).unwrap(); + assert!(large.len() > normal.len(), "the asset entry carries zip64 sizes"); + assert_eq!(*load(&large).unwrap().assets[&aid].data, vec![5; 64]); + } + #[test] fn saves_hold_each_used_file_once() { let mut d = Document::new(&NewDocument::default()); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index fc7a3e634..6d76a6cac 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -52,7 +52,8 @@ pub struct ImportedFile { pub bytes: Vec, } pub type OpenAsyncFn = Box; -pub type DownloadFn = Box; +/// Starts a browser download; an error means no file was handed to the user. +pub type DownloadFn = Box Result<(), String>>; /// Files delivered asynchronously with their original operation and document identity. pub type Inbox = std::sync::Arc>>; @@ -2173,7 +2174,7 @@ mod browser_import_tests { let mut source = Session::new(); source.execute("file.new", &json!({"title": "source"})).unwrap(); source.execute("frame.create", &json!({"rect": [10, 10, 50, 50], "content": "text", "text": "placed"})).unwrap(); - let native = designcraft_engine::cmd::to_bytes(&source.doc().unwrap().doc); + let native = designcraft_engine::cmd::to_bytes(&source.doc().unwrap().doc).unwrap(); let idml = designcraft_idml::export_idml(&source.doc().unwrap().doc); for (name, bytes) in [("source.designcraft", native), ("source.idml", idml)] { let mut app = app(); @@ -2243,7 +2244,7 @@ mod browser_import_tests { #[test] fn drop_routing_and_nonlayout_open_remain_compatible() { let mut app = app(); - let native = designcraft_engine::cmd::to_bytes(&app.session.doc().unwrap().doc); + let native = designcraft_engine::cmd::to_bytes(&app.session.doc().unwrap().doc).unwrap(); let request = app.import_request("drop"); deliver(&mut app, request, "document.designcraft", native); assert_eq!(app.session.documents().len(), 2); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 872cb81d0..e2c2790a0 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1519,9 +1519,8 @@ fn download_document(app: &mut DesignApp) -> Result { let name = app.session.active().map(|d| format!("{}.designcraft", d.doc.title)).ok_or("no document")?; let ser = app.run("file.serialize", json!({}))?; let bytes = designcraft_engine::cmd::base64_decode(ser["base64"].as_str().unwrap_or_default()); - if let Some(download) = app.services.download.as_mut() { - download(&name, &bytes); - } + let download = app.services.download.as_mut().ok_or("no download service")?; + download(&name, &bytes).map_err(|e| format!("could not download {name}: {e}"))?; // Marks the document saved; on the web the engine doesn't touch a file system. app.run("file.saveAs", json!({"path": name})) } @@ -3604,4 +3603,31 @@ mod tests { let (_, Some((_, _, open))) = frame(500.0, vec![]) else { panic!("test menu remains open") }; assert!(!open, "an open submenu closes when its row becomes fully clipped"); } + + /// Web save: a download that couldn't start is an error and leaves the document unsaved; one + /// that started marks it saved. + #[test] + fn web_save_marks_the_document_saved_only_after_the_download_starts() { + let dir = tempfile::tempdir().unwrap(); + // The title names a temporary path: on native, file.saveAs writes where the web build doesn't. + let title = dir.path().join("doc").to_string_lossy().to_string(); + let fails = std::rc::Rc::new(std::cell::Cell::new(true)); + let f = fails.clone(); + let services = crate::Services { + download: Some(Box::new(move |_: &str, _: &[u8]| if f.get() { Err("blocked".into()) } else { Ok(()) })), + ..Default::default() + }; + let mut app = DesignApp::new(designcraft_engine::Session::new(), services); + app.run("file.new", json!({"title": title})).unwrap(); + app.run("frame.create", json!({"rect": [36, 36, 300, 200]})).unwrap(); + assert!(app.session.active().unwrap().is_dirty()); + + let e = app.run("app.save", json!({})).unwrap_err(); + assert!(e.contains("blocked"), "{e}"); + assert!(app.session.active().unwrap().is_dirty(), "a failed download leaves the document unsaved"); + + fails.set(false); + app.run("app.save", json!({})).unwrap(); + assert!(!app.session.active().unwrap().is_dirty()); + } }