From 8968e799429fe34ccdb393a5f34b07cd9cd640da 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 refused 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 codec accessors — upstream), and WBLOCK/SAVEALL/ARCHIVE still serialize inline (the worker route is a small follow-up). (cherry picked from commit 6f429f24) Ported onto main (#1717 review): main already keys PendingClose by tab id, so those hunks stay as on main; the ZIP64 guard is moot with the zip crate; the unresolved-xref names split on both path separators, since Path does not split Windows paths in the browser build. --- docs/native-vs-web.md | 2 +- src/app/annotation_data.rs | 64 ++++++++++++++++++++++++++++++++++--- src/app/commands/fileops.rs | 8 +++++ src/app/update/file.rs | 4 +-- src/app/update/util.rs | 2 +- src/io/mod.rs | 4 +++ src/io/xref.rs | 48 +++++++++++++++++++++++----- 7 files changed, 115 insertions(+), 17 deletions(-) diff --git a/docs/native-vs-web.md b/docs/native-vs-web.md index 09440bde2..6488fec28 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 a448534ea..e549a4a11 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 @@ -1283,7 +1314,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, @@ -1291,6 +1330,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(); @@ -1780,7 +1828,13 @@ impl OpenCADStudio { // that 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) }, diff --git a/src/app/commands/fileops.rs b/src/app/commands/fileops.rs index 289d5483c..c9a19b416 100644 --- a/src/app/commands/fileops.rs +++ b/src/app/commands/fileops.rs @@ -585,6 +585,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). diff --git a/src/app/update/file.rs b/src/app/update/file.rs index 555cf0620..8d59055b9 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -671,7 +671,7 @@ impl OpenCADStudio { /// have no ACIS data before saving. A solid that has ACIS data keeps its /// own wireframe cache: display edges are not written into a solid OCS /// did not create or edit (those get their wires when they are built). - 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 codec::EntityType; let scene = &mut self.tabs[i].scene; let targets: Vec<(codec::Handle, bool)> = scene @@ -2460,7 +2460,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(); diff --git a/src/app/update/util.rs b/src/app/update/util.rs index 70235ff1b..666dfaace 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/io/mod.rs b/src/io/mod.rs index 869336908..60c42e6e2 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -693,6 +693,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 3dc782c29..e0cc9aaac 100644 --- a/src/io/xref.rs +++ b/src/io/xref.rs @@ -6,6 +6,7 @@ use codec::tables::TableEntry; use codec::types::{Handle, Vector3}; use codec::{CadDocument, EntityType}; use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use std::collections::VecDeque; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex, OnceLock}; @@ -51,7 +52,18 @@ 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. + // Split on both separators: drawings carry Windows paths, which + // `Path` does not split off Windows. + .map(|info| { + info.path + .rsplit(['/', '\\']) + .next() + .map(str::trim) + .filter(|n| !n.is_empty()) + .map_or_else(|| info.name.clone(), str::to_string) + }) .collect() } @@ -89,9 +101,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> { @@ -105,10 +124,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()) @@ -133,6 +163,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() @@ -144,6 +175,7 @@ pub(crate) fn registered_bytes(key: &str) -> Option>> { source_cache() .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) + .0 .get(key) .cloned() }