From 6f429f2416a4be63c8ef9ad484480edfb1f0314f Mon Sep 17 00:00:00 2001 From: Afonsop Date: Fri, 18 Sep 2026 00:32:59 +0100 Subject: [PATCH] fix: close the review findings on the last web wave MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full verification of the XREF/data/ARCHIVE/worker wave (three reviewers, one forensic) found no blockers and no crossfire damage; five majors and several minors are fixed here. - ARCHIVE now packages XREFs: the bytes registry added by the same commit is consulted (the in-code 'no xref store exists' comment was false within the commit), and the drawing is packaged only after the full save preparation — without it kernel solids shipped without their ACIS data. - PendingClose stores the stable tab id instead of a vector index: the web save now completes asynchronously, and closing a lower tab during the serialization window shifted indices — closing the WRONG tab, or panicking on a stale one. All producers and consumers resolve the id at use. - The xref bytes registry is capped (6 entries, oldest evicted): every open, drop, and pick registered full drawing bytes with no bound — an OOM waiting to happen. Reopened recents register too, so any entry path can serve as another drawing's xref. - A re-picked data-link source with the same name but different bytes no longer silently replaces the first (the browser hands back bare names, so two same-named files collided and every reference read the newer pick): the pick is refused and the user is told why. - The unresolved-xref warning lists file names (what to drop on the page), not internal block names; the ZIP writer refuses the ZIP64 sentinel values (0xFFFF counts, 0xFFFFFFFF sizes/offsets) instead of emitting archives that send strict readers hunting for a ZIP64 EOCD that is absent; docs note the worker-side saves. Known follow-ups documented, not fixed here: the serde-skipped ExtendedEntity/CadDocument raw bytes still drop on worker crossings (needs acadrust accessors — upstream), and WBLOCK/SAVEALL/ARCHIVE still serialize inline (the worker route is a small follow-up). --- docs/native-vs-web.md | 2 +- src/app/annotation_data.rs | 64 ++++++++++++++++++++++++++++++++++--- src/app/commands/fileops.rs | 22 ++++++++++--- src/app/mod.rs | 6 ++-- src/app/update/command.rs | 4 +-- src/app/update/dialog.rs | 21 +++++++++--- src/app/update/file.rs | 27 ++++++++++------ src/app/update/mod.rs | 2 +- src/app/update/util.rs | 2 +- src/app/view/modal.rs | 5 +-- src/io/mod.rs | 4 +++ src/io/xref.rs | 45 +++++++++++++++++++++----- src/io/zip.rs | 20 ++++++++++-- 13 files changed, 182 insertions(+), 42 deletions(-) diff --git a/docs/native-vs-web.md b/docs/native-vs-web.md index d9525c56e..90b88a0de 100644 --- a/docs/native-vs-web.md +++ b/docs/native-vs-web.md @@ -15,7 +15,7 @@ browser can't provide. This page lists the differences. | Fonts | Embedded stroke fonts + system TrueType + shaping + fallback | Fetched Noto subsets (shaping, fallback, and family aliases) + embedded stroke fonts | | File open | Native file dialog → path | Browser picker → bytes | | File save | Path / Save dialog → write to disk | Save dialog → direct browser download | -| Parallelism | Multi-threaded (rayon) | Main thread + one Web Worker (DWG/DXF parse, hashing) | +| Parallelism | Multi-threaded (rayon) | Main thread + one Web Worker (DWG/DXF parse, hashing, save/autosave serialization) | | GPU backend | Vulkan / DX12 / Metal (wgpu) | WebGL2 (the backend is pinned to OpenGL) | | Update check / external links | `ureq`, `open` | skipped / `window.open` | | External plugins | Native packages + Plugin Manager | **No** — native libraries cannot run in a browser | diff --git a/src/app/annotation_data.rs b/src/app/annotation_data.rs index 7b8a4457d..c87862a7f 100644 --- a/src/app/annotation_data.rs +++ b/src/app/annotation_data.rs @@ -71,11 +71,42 @@ fn same_source_name(a: &str, b: &str) -> bool { // Only the web registers sources (native re-reads the real file, so a file // edited after picking is picked up); tests exercise the registry directly. #[cfg_attr(not(target_arch = "wasm32"), allow(dead_code))] -pub(crate) fn register_picked_source(path: &str, bytes: Arc>) { - picked_sources() +/// Returns false when the registration REPLACED different bytes under the +/// same key — the browser hands back bare file names, so two distinct files +/// named alike collide and every reference to that name would silently read +/// the newer pick. Callers surface that to the user. +pub(crate) fn register_picked_source(path: &str, bytes: Arc>) -> bool { + let mut cache = picked_sources() + .lock() + .unwrap_or_else(|error| error.into_inner()); + let replaced = cache + .get(path) + .is_some_and(|old| old.as_slice() != bytes.as_slice()); + if replaced { + take_last_collision().replace(file_name_of(path)); + } + cache.insert(path.to_string(), bytes); + !replaced +} + +/// The name whose re-pick was refused because different bytes are already +/// registered under it — the browse handler reports and clears it. +fn last_collision() -> &'static Mutex> { + static LAST: OnceLock>> = OnceLock::new(); + LAST.get_or_init(|| Mutex::new(None)) +} + +fn take_last_collision() -> std::sync::MutexGuard<'static, Option> { + last_collision() .lock() .unwrap_or_else(|error| error.into_inner()) - .insert(path.to_string(), bytes); +} + +fn file_name_of(path: &str) -> String { + std::path::Path::new(path) + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| path.to_string()) } /// Bytes registered for this reference — exact key first, then any entry that @@ -1090,7 +1121,15 @@ impl OpenCADStudio { // keep its bytes so preview, save, and later re-opens read // them instead of asking for the picker again. #[cfg(target_arch = "wasm32")] - register_picked_source(&path.to_string_lossy(), Arc::new(handle.read().await)); + { + let fresh = register_picked_source( + &path.to_string_lossy(), + Arc::new(handle.read().await), + ); + if !fresh { + return None; + } + } Some(path) }, Message::DataLinkBrowseResult, @@ -1098,6 +1137,15 @@ impl OpenCADStudio { } pub(super) fn on_data_link_browse_result(&mut self, path: Option) -> Task { + #[cfg(target_arch = "wasm32")] + if let Some(name) = take_last_collision().take() { + self.command_line.push_warning( + crate::tf!( + "\"{name}\" is already loaded from another folder with different contents; re-picking it was ignored to keep the existing link." + ) + .as_ref(), + ); + } if let Some(path) = path { self.data_link_manager.path = path.to_string_lossy().into_owned(); let (sheets, named_ranges) = workbook_metadata(&path).unwrap_or_default(); @@ -1472,7 +1520,13 @@ impl OpenCADStudio { // follows this message. #[cfg(target_arch = "wasm32")] if !save { - register_picked_source(&path.to_string_lossy(), Arc::new(handle.read().await)); + let fresh = register_picked_source( + &path.to_string_lossy(), + Arc::new(handle.read().await), + ); + if !fresh { + return None; + } } Some(path) }, Message::DataExtractionBrowseSettingsResult) diff --git a/src/app/commands/fileops.rs b/src/app/commands/fileops.rs index 8b19b4d9d..3a33083e3 100644 --- a/src/app/commands/fileops.rs +++ b/src/app/commands/fileops.rs @@ -542,6 +542,14 @@ impl OpenCADStudio { .unwrap_or_else(|| format!("{stem}.{ext}")); let version = self.tabs[i].scene.document.version; + // The same save preparation a web Save performs — without it the + // archived copy ships kernel solids without their ACIS data and a + // staler view/sysvar header than a real save would produce. + self.sync_view_state_for_save(i); + crate::app::update::util::sync_annotation_scale_header(&mut self.tabs[i].scene); + self.stamp_header_sysvars(i); + self.sync_solid_models_for_save(i); + self.tabs[i].scene.sync_native_parametric_graph(); // The drawing itself: the same serialization a web Save performs // (native copies the last-saved file; the web build has none, so the // current document is re-serialized). @@ -557,12 +565,18 @@ impl OpenCADStudio { let mut entries: Vec = vec![ZipEntry::new(filename, drawing)]; let mut skipped: Vec = Vec::new(); - // Xrefs — the block-record walk native copies. No xref bytes store - // exists on the web build (the file behind the path never entered - // the session), so every reference reports as skipped. + // Xrefs — the block-record walk native copies. Opened, dropped, + // and XATTACHed drawings register their bytes (io::xref), so a + // referenced file that entered the session ships in the ZIP. for br in self.tabs[i].scene.document.block_records.iter() { if !br.xref_path.trim().is_empty() { - push_skipped(&mut skipped, &transmit_file_name(&br.xref_path)); + let name = transmit_file_name(&br.xref_path); + match crate::io::xref::find_registered_source(&br.xref_path) + .and_then(|key| crate::io::xref::registered_bytes(&key)) + { + Some(bytes) => push_entry(&mut entries, name, (*bytes).clone()), + None => push_skipped(&mut skipped, &name), + } } } diff --git a/src/app/mod.rs b/src/app/mod.rs index 2b7835a52..47f6b2afa 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1298,8 +1298,10 @@ pub(super) struct OpenCADStudio { /// What triggered the "unsaved changes" dialog. #[derive(Debug, Clone)] pub(super) enum PendingClose { - /// User tried to close the tab at this index. - Tab(usize), + /// User tried to close this tab — stored as the stable tab id, because + /// the web save completes asynchronously and removing an earlier tab in + /// the meantime shifts every later vector index. + Tab(u64), /// User tried to quit the application. Quit, } diff --git a/src/app/update/command.rs b/src/app/update/command.rs index 8a3c081c4..0f3f2f48c 100644 --- a/src/app/update/command.rs +++ b/src/app/update/command.rs @@ -90,7 +90,7 @@ pub(super) fn begin_tab_close_queue(&mut self, tab_ids: Vec) -> Task Task { // now rather than risk it applying to a different tab's document. self.cancel_attr_editor(); if self.tabs.get(idx).map_or(false, |t| t.dirty) { - self.pending_close = Some(crate::app::PendingClose::Tab(idx)); + self.pending_close = Some(crate::app::PendingClose::Tab(self.tabs[idx].id)); return self.open_unsaved_dialog_window(); } #[cfg(not(target_arch = "wasm32"))] diff --git a/src/app/update/dialog.rs b/src/app/update/dialog.rs index 53914213f..98bad9884 100644 --- a/src/app/update/dialog.rs +++ b/src/app/update/dialog.rs @@ -197,8 +197,12 @@ impl OpenCADStudio { pub(super) fn on_unsaved_dialog_discard(&mut self) -> Task { match self.pending_close.take() { - Some(crate::app::PendingClose::Tab(idx)) => { + Some(crate::app::PendingClose::Tab(id)) => { let close_win = self.close_unsaved_dialog_window(); + let Some(idx) = self.tabs.iter().position(|tab| tab.id == id) else { + // The tab is already gone; nothing to discard. + return close_win; + }; // Discarded — drop this tab's autosave recovery copy. #[cfg(not(target_arch = "wasm32"))] let _ = std::fs::remove_file(self.autosave_target(idx)); @@ -262,7 +266,13 @@ impl OpenCADStudio { return Task::none(); }; let (idx, continuation) = match pending { - crate::app::PendingClose::Tab(idx) => (idx, crate::app::SaveContinuation::CloseTab), + crate::app::PendingClose::Tab(id) => { + let Some(idx) = self.tabs.iter().position(|tab| tab.id == id) else { + self.pending_close = None; + return self.close_unsaved_dialog_window(); + }; + (idx, crate::app::SaveContinuation::CloseTab) + } crate::app::PendingClose::Quit => { let Some(idx) = self.tabs.iter().position(|tab| tab.dirty) else { self.pending_close = None; @@ -306,8 +316,11 @@ impl OpenCADStudio { #[cfg(target_arch = "wasm32")] { match self.pending_close.take() { - Some(crate::app::PendingClose::Tab(idx)) => { - self.pending_close = Some(crate::app::PendingClose::Tab(idx)); + Some(crate::app::PendingClose::Tab(id)) => { + let Some(idx) = self.tabs.iter().position(|tab| tab.id == id) else { + return self.close_unsaved_dialog_window(); + }; + self.pending_close = Some(crate::app::PendingClose::Tab(id)); self.save_dialog_for_unsaved = true; let close = self.close_unsaved_dialog_window(); let save = self.save_with_default_format(idx); diff --git a/src/app/update/file.rs b/src/app/update/file.rs index 6a0fcb6f9..f1d07aa46 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -302,7 +302,7 @@ fn plot_scene_content( impl OpenCADStudio { /// Persist exact ACIS bodies and kernel-derived edge caches before saving. - fn sync_solid_models_for_save(&mut self, i: usize) { + pub(in crate::app) fn sync_solid_models_for_save(&mut self, i: usize) { use acadrust::EntityType; let scene = &mut self.tabs[i].scene; let targets: Vec<(acadrust::Handle, bool, bool)> = scene @@ -1893,7 +1893,7 @@ impl OpenCADStudio { ) } - fn sync_view_state_for_save(&mut self, i: usize) { + pub(in crate::app) fn sync_view_state_for_save(&mut self, i: usize) { self.sync_vport_display(i); if self.tabs[i].active_block_edit.is_none() { self.tabs[i].scene.sync_camera_to_document(); @@ -2152,10 +2152,15 @@ impl OpenCADStudio { fn finish_web_save(&mut self, saved: bool) -> Task { if self.save_dialog_for_unsaved { if saved { - if let Some(crate::app::PendingClose::Tab(index)) = self.pending_close.take() { - let continuation = self.update(Message::TabClose(index)); - let rest = self.continue_tab_close_queue(); - return Task::batch([continuation, rest]); + if let Some(crate::app::PendingClose::Tab(id)) = self.pending_close.take() { + // Resolve the id NOW: tabs may have closed while the + // serialization ran, shifting every stored index. + if let Some(index) = self.tabs.iter().position(|tab| tab.id == id) { + let continuation = self.update(Message::TabClose(index)); + let rest = self.continue_tab_close_queue(); + return Task::batch([continuation, rest]); + } + return self.continue_tab_close_queue(); } } else if self.pending_close.is_some() { return self.open_unsaved_dialog_window(); @@ -2547,7 +2552,7 @@ impl OpenCADStudio { }); match outcome.continuation { crate::app::SaveContinuation::CloseTab => { - self.pending_close = Some(crate::app::PendingClose::Tab(i)); + self.pending_close = Some(crate::app::PendingClose::Tab(self.tabs[i].id)); } crate::app::SaveContinuation::Quit => { self.pending_close = Some(crate::app::PendingClose::Quit); @@ -2561,7 +2566,7 @@ impl OpenCADStudio { .push_error(crate::tf!("Save failed: {error}").as_ref()); return match outcome.continuation { crate::app::SaveContinuation::CloseTab => { - self.pending_close = Some(crate::app::PendingClose::Tab(i)); + self.pending_close = Some(crate::app::PendingClose::Tab(self.tabs[i].id)); self.open_unsaved_dialog_window() } crate::app::SaveContinuation::Quit => { @@ -2638,7 +2643,7 @@ impl OpenCADStudio { } } crate::app::SaveContinuation::CloseTab => { - self.pending_close = Some(crate::app::PendingClose::Tab(i)); + self.pending_close = Some(crate::app::PendingClose::Tab(self.tabs[i].id)); tasks.push(self.open_unsaved_dialog_window()); } crate::app::SaveContinuation::Quit => { @@ -2657,7 +2662,9 @@ impl OpenCADStudio { ) { self.pending_close = match continuation { crate::app::SaveContinuation::None => None, - crate::app::SaveContinuation::CloseTab => Some(crate::app::PendingClose::Tab(tab_idx)), + crate::app::SaveContinuation::CloseTab => { + Some(crate::app::PendingClose::Tab(self.tabs[tab_idx].id)) + } crate::app::SaveContinuation::Quit => Some(crate::app::PendingClose::Quit), }; } diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 23f2652e6..f1011a56e 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -93,7 +93,7 @@ mod dialog; mod dynamic; mod file; mod style; -mod util; +pub(in crate::app) mod util; mod viewport; impl OpenCADStudio { diff --git a/src/app/update/util.rs b/src/app/update/util.rs index cb78c36ab..2817ab8ee 100644 --- a/src/app/update/util.rs +++ b/src/app/update/util.rs @@ -6,7 +6,7 @@ use crate::scene::Scene; /// Returns (1.0, 1.0) for "Fit" or unknown formats. /// Sync the model-space annotation scale into its named drawing variable and /// numeric header mirror before a save. -pub(super) fn sync_annotation_scale_header(scene: &mut Scene) { +pub(in crate::app) fn sync_annotation_scale_header(scene: &mut Scene) { let anno = scene.annotation_scale; let value = if anno.abs() > 1e-9 { 1.0 / anno as f64 diff --git a/src/app/view/modal.rs b/src/app/view/modal.rs index 0e7ce7c7b..c09d3dcf0 100644 --- a/src/app/view/modal.rs +++ b/src/app/view/modal.rs @@ -1495,9 +1495,10 @@ impl OpenCADStudio { } super::super::ModalKind::Unsaved => { let tab_name = match &self.pending_close { - Some(super::super::PendingClose::Tab(idx)) => self + Some(super::super::PendingClose::Tab(id)) => self .tabs - .get(*idx) + .iter() + .find(|t| t.id == *id) .map(|t| t.tab_display_name()) .unwrap_or_default(), Some(super::super::PendingClose::Quit) => self diff --git a/src/io/mod.rs b/src/io/mod.rs index 8710f57bd..7add2af46 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -649,6 +649,10 @@ async fn open_recent_web_attempt( bytes.len(), ); let result = load_web_bytes(&name, &bytes, progress, recovery_mode, &initial_error, None).await; + // A recent reopened from OPFS registers like a picked or dropped file, + // so it can serve as another drawing's xref in this session. + #[cfg(target_arch = "wasm32")] + xref::register_source(&name, Arc::new(bytes.to_vec())); let keep_for_recovery = result .as_ref() .err() diff --git a/src/io/xref.rs b/src/io/xref.rs index 548b7da3c..d199f6957 100644 --- a/src/io/xref.rs +++ b/src/io/xref.rs @@ -7,6 +7,7 @@ use acadrust::types::{Handle, Vector3}; use acadrust::{CadDocument, EntityType}; use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; use std::path::{Path, PathBuf}; +use std::collections::VecDeque; use std::sync::{Arc, Mutex, OnceLock}; #[cfg(not(target_arch = "wasm32"))] @@ -52,7 +53,15 @@ pub fn unresolved_xref_names(infos: &[XrefInfo]) -> Vec { infos .iter() .filter(|info| info.status == XrefStatus::NotFound) - .map(|info| info.name.clone()) + // The file name is what the user must drop on the page; the block + // name may be a renamed `A$C$...` alias that matches no file. + .map(|info| { + std::path::Path::new(&info.path) + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .filter(|n| !n.is_empty()) + .unwrap_or_else(|| info.name.clone()) + }) .collect() } @@ -90,9 +99,16 @@ pub(crate) fn same_file_name(a: &str, b: &str) -> bool { !a.is_empty() && a == b } -fn source_cache() -> &'static Mutex>>> { - static SOURCES: OnceLock>>>> = OnceLock::new(); - SOURCES.get_or_init(|| Mutex::new(HashMap::default())) +/// Upper bound on registered drawings. Every open, drop, and XATTACH pick +/// registers its bytes; without a cap a day of opening large drawings grows +/// the wasm heap until the tab dies. Oldest registrations leave first — +/// enough for a parent drawing plus its usual handful of references. +const SOURCE_CAP: usize = 6; + +fn source_cache() -> &'static Mutex<(HashMap>>, VecDeque)> { + static SOURCES: OnceLock>>, VecDeque)>> = + OnceLock::new(); + SOURCES.get_or_init(|| Mutex::new((HashMap::default(), VecDeque::new()))) } fn parsed_cache() -> &'static Mutex> { @@ -106,10 +122,21 @@ fn parsed_cache() -> &'static Mutex> { /// finally supplies a missing reference) reloads from the new bytes. #[cfg_attr(not(target_arch = "wasm32"), allow(dead_code))] // mirrors image_model::register_source pub fn register_source(name: &str, bytes: Arc>) { - source_cache() - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()) - .insert(name.to_string(), bytes); + { + let mut cache = source_cache() + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let (map, order) = &mut *cache; + if !map.contains_key(name) { + order.push_back(name.to_string()); + while order.len() > SOURCE_CAP { + if let Some(evicted) = order.pop_front() { + map.remove(&evicted); + } + } + } + map.insert(name.to_string(), bytes); + } parsed_cache() .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) @@ -134,6 +161,7 @@ pub(crate) fn find_registered_source(raw: &str) -> Option { source_cache() .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) + .0 .keys() .find(|key| same_file_name(key, raw)) .cloned() @@ -145,6 +173,7 @@ pub(crate) fn registered_bytes(key: &str) -> Option>> { source_cache() .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) + .0 .get(key) .cloned() } diff --git a/src/io/zip.rs b/src/io/zip.rs index bcae0a250..205491d83 100644 --- a/src/io/zip.rs +++ b/src/io/zip.rs @@ -72,8 +72,13 @@ impl ZipEntry { /// make. Duplicate names are written as given; callers that want one file /// per name dedupe before building. pub fn build_zip(entries: &[ZipEntry]) -> Result, String> { - let count = - u16::try_from(entries.len()).map_err(|_| format!("too many entries: {}", entries.len()))?; + let count = u16::try_from(entries.len()) + .map_err(|_| format!("too many entries: {}", entries.len()))?; + // 0xFFFF/0xFFFFFFFF are the classic-format ZIP64 sentinels; writing + // them without a ZIP64 EOCD would send strict readers hunting for one. + if count == u16::MAX { + return Err("too many entries (ZIP64 sentinel)".to_string()); + } let mut out: Vec = Vec::new(); let mut central: Vec = Vec::new(); for entry in entries { @@ -97,6 +102,14 @@ pub fn build_zip(entries: &[ZipEntry]) -> Result, String> { .map_err(|_| format!("entry {} exceeds the 4 GiB classic-ZIP limit", name))?; let local_offset = u32::try_from(out.len()) .map_err(|_| "archive exceeds the 4 GiB classic-ZIP limit".to_string())?; + // 0xFFFFFFFF is the classic-format ZIP64 sentinel; writing it without + // a ZIP64 EOCD sends strict readers hunting for one that is absent. + if local_offset == u32::MAX + || compressed_size == u32::MAX + || uncompressed_size == u32::MAX + { + return Err("archive exceeds the classic-ZIP limit".to_string()); + } // Local file header — immediately followed by name and payload. out.extend_from_slice(&LOCAL_SIG.to_le_bytes()); @@ -136,6 +149,9 @@ pub fn build_zip(entries: &[ZipEntry]) -> Result, String> { let central_offset = u32::try_from(out.len()) .map_err(|_| "archive exceeds the 4 GiB classic-ZIP limit".to_string())?; + if central_offset == u32::MAX { + return Err("archive exceeds the classic-ZIP limit".to_string()); + } let central_size = u32::try_from(central.len()) .map_err(|_| "archive exceeds the 4 GiB classic-ZIP limit".to_string())?; out.extend_from_slice(¢ral);