diff --git a/src/app/commands/fileops.rs b/src/app/commands/fileops.rs index 07a7f69c0..1a5d74f76 100644 --- a/src/app/commands/fileops.rs +++ b/src/app/commands/fileops.rs @@ -141,7 +141,10 @@ impl OpenCADStudio { // CLOSE — close the active drawing tab (with the unsaved-changes // prompt the tab-close handler already runs). "CLOSE" => { - return Some(Task::done(Message::TabClose(self.active_tab))); + return Some(match self.tabs.get(self.active_tab) { + Some(tab) => Task::done(Message::TabClose(tab.id)), + None => Task::none(), + }); } // ARCHIVE / ETRANSMIT — package the drawing and its referenced files diff --git a/src/app/control/actions.rs b/src/app/control/actions.rs index b4e73a9f7..0b9cb9c91 100644 --- a/src/app/control/actions.rs +++ b/src/app/control/actions.rs @@ -727,7 +727,7 @@ impl OpenCADStudio { Some(crate::app::ModalKind::BlockDefinition) => Message::BlockDefApply, _ => return Err(failure("no_dialog", "No dialog with an OK button is open")), }, - "close_document" => Message::TabClose(self.active_tab), + "close_document" => Message::TabClose(self.tabs[self.active_tab].id), "toggle_properties" => Message::ToggleProperties, "toggle_layers" => Message::ToggleLayers, "toggle_grid" => Message::ToggleGrid, diff --git a/src/app/control/sheets.rs b/src/app/control/sheets.rs index 3657483f8..dd3b49bdc 100644 --- a/src/app/control/sheets.rs +++ b/src/app/control/sheets.rs @@ -28,9 +28,12 @@ impl OpenCADStudio { } self.tabs[index].dirty = false; let remaining = self.tabs.len(); + // Resolve the stable id now; the message may be handled after other + // batched messages have shifted `index`. + let tab_id = self.tabs[index].id; // The close may queue async work (autosave cleanup) — return the // task so the runtime drives it. - let close_task = self.update(Message::TabClose(index)); + let close_task = self.update(Message::TabClose(tab_id)); let closed_document = req["document_id"].as_u64(); self.set_control_result(json!({ "closed": true, diff --git a/src/app/mod.rs b/src/app/mod.rs index 1602c6392..62991eee8 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1438,8 +1438,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 the tab with this document id. Ids stay valid + /// while the dialog is open; a vector index would go stale the moment + /// another tab closes and Discard would remove the wrong tab. + Tab(u64), /// User tried to quit the application. Quit, } @@ -2417,8 +2419,12 @@ pub enum Message { to: usize, after: bool, }, - /// Close the given tab index. - TabClose(usize), + /// Close the tab with this document id. Carries the stable id, not a + /// vector index: iced dispatches every message built from one view + /// snapshot before rebuilding it, so an index captured alongside other + /// messages can already be stale (and point at a different tab) by the + /// time it is handled. + TabClose(u64), /// Save every drawing that already has a file path. DocTabSaveAll, /// Close every non-Start drawing tab. diff --git a/src/app/update/command.rs b/src/app/update/command.rs index 4d41d3785..58fd3aca1 100644 --- a/src/app/update/command.rs +++ b/src/app/update/command.rs @@ -149,7 +149,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(); } // Pending client interactive requests pinned to this drawing @@ -4559,3 +4560,106 @@ mod plot_style_ctb_guard_tests { ); } } + +/// Reproductions for the stale-tab-index crashes: iced dispatches every +/// message produced from one view snapshot before rebuilding it, so a second +/// `TabClose` (or a `PendingClose::Tab` stashed while the unsaved dialog is +/// open) can carry an index the first removal already invalidated. +#[cfg(test)] +mod tab_close_tests { + use crate::app::{Message, OpenCADStudio}; + + /// `[Start, Drawing A, Drawing B]` — three clean tabs. + fn three_tabs() -> OpenCADStudio { + let mut app = OpenCADStudio::new_for_test(); + app.push_test_document(); + app.push_test_document(); + assert_eq!(app.tabs.len(), 3, "start page plus two drawings"); + assert!(!app.tabs[1].is_start && !app.tabs[2].is_start); + app + } + + /// Two close messages built from the same view snapshot: the first one + /// removes the tab, the second carries the now-outdated reference. The + /// close request must degrade to a no-op, not panic. + #[test] + fn stale_tab_close_is_a_no_op() { + let mut app = three_tabs(); + let id = app.tabs[app.tabs.len() - 1].id; + + let _ = app.update(Message::TabClose(id)); + assert_eq!(app.tabs.len(), 2, "first close removed the tab"); + + // Same target again, as the batched second message from one snapshot. + let _ = app.update(Message::TabClose(id)); + assert_eq!( + app.tabs.len(), + 2, + "stale close must not remove another tab or panic" + ); + } + + /// Double-clicking one tab's × in a single batch must close that tab + /// exactly once; the second message must not slide onto the tab that + /// shifted into its place. + #[test] + fn double_close_from_one_snapshot_closes_only_the_clicked_tab() { + let mut app = three_tabs(); + let first_id = app.tabs[1].id; + let other_id = app.tabs[2].id; + + let _ = app.update(Message::TabClose(first_id)); + let _ = app.update(Message::TabClose(first_id)); + + assert!( + app.tabs.iter().all(|t| t.id != first_id), + "the clicked tab must be closed" + ); + assert!( + app.tabs.iter().any(|t| t.id == other_id), + "the tab that shifted into index 1 must survive" + ); + } + + /// The unsaved dialog stashes a close target while it is open. Another + /// close landing before the user answers shifts the vector, so Discard + /// must still discard the tab the dialog was opened for. + #[test] + fn unsaved_dialog_discard_closes_the_tab_it_was_opened_for() { + let mut app = OpenCADStudio::new_for_test(); + app.push_test_document(); // A + app.push_test_document(); // B + app.push_test_document(); // C + assert_eq!(app.tabs.len(), 4); + let a_id = app.tabs[1].id; + let b_id = app.tabs[2].id; + let c_id = app.tabs[3].id; + app.tabs[2].dirty = true; + + // Close dirty B: opens the dialog and stashes the pending target. + let _ = app.update(Message::TabClose(b_id)); + assert!( + app.pending_close.is_some(), + "dirty close opens the unsaved dialog" + ); + + // A second close lands while the dialog is pending (batched message / + // automation `close` / save continuation) and shifts indices. + let _ = app.update(Message::TabClose(a_id)); + assert!(app.tabs.iter().all(|t| t.id != a_id)); + + // User picks Discard. + let _ = app.update(Message::UnsavedDialogDiscard); + + assert!( + app.tabs.iter().all(|t| t.id != b_id), + "the dirty tab the dialog was opened for must be discarded" + ); + assert!( + app.tabs.iter().any(|t| t.id == c_id), + "C must survive the stale pending index" + ); + assert!(app.pending_close.is_none(), "pending target consumed"); + assert!(app.active_tab < app.tabs.len(), "active tab stays valid"); + } +} diff --git a/src/app/update/dialog.rs b/src/app/update/dialog.rs index 92a555c6d..8dd7ecf96 100644 --- a/src/app/update/dialog.rs +++ b/src/app/update/dialog.rs @@ -200,8 +200,14 @@ pub(super) fn on_ribbon_tool_click(&mut self, tool_id: String, event: ModuleEven 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(tab_id)) => { let close_win = self.close_unsaved_dialog_window(); + // Resolve the id now: the tab may have closed while + // the dialog was open (batched close, automation), + // in which case there is nothing left to discard. + let Some(idx) = self.tabs.iter().position(|t| t.id == tab_id) else { + return Task::batch([close_win, self.continue_tab_close_queue()]); + }; // Discarded — drop this tab's autosave recovery copy. #[cfg(not(target_arch = "wasm32"))] let _ = std::fs::remove_file(self.autosave_target(idx)); @@ -249,7 +255,17 @@ pub(super) fn on_ribbon_tool_click(&mut self, tool_id: String, event: ModuleEven return Task::none(); }; let (idx, continuation) = match pending { - crate::app::PendingClose::Tab(idx) => { + crate::app::PendingClose::Tab(tab_id) => { + // The pending target is an id; resolve it against the + // current tab list so a tab closed in the meantime can + // neither panic the save path nor save the wrong file. + let Some(idx) = self.tabs.iter().position(|t| t.id == tab_id) else { + self.pending_close = None; + return Task::batch([ + self.close_unsaved_dialog_window(), + self.continue_tab_close_queue(), + ]); + }; (idx, crate::app::SaveContinuation::CloseTab) } crate::app::PendingClose::Quit => { @@ -298,8 +314,14 @@ pub(super) fn on_ribbon_tool_click(&mut self, tool_id: String, event: ModuleEven #[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(tab_id)) => { + let Some(idx) = self.tabs.iter().position(|t| t.id == tab_id) else { + return Task::batch([ + self.close_unsaved_dialog_window(), + self.continue_tab_close_queue(), + ]); + }; + self.pending_close = Some(crate::app::PendingClose::Tab(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 7d9455805..57abaaa0f 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -2226,8 +2226,8 @@ impl OpenCADStudio { }; 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)); + if let Some(crate::app::PendingClose::Tab(tab_id)) = self.pending_close.take() { + let continuation = self.update(Message::TabClose(tab_id)); let rest = self.continue_tab_close_queue(); return Task::batch([recent_task, continuation, rest]); } @@ -2649,7 +2649,8 @@ 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); @@ -2663,7 +2664,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 => { @@ -2754,7 +2755,7 @@ impl OpenCADStudio { crate::app::SaveContinuation::CloseTab if snapshot_is_current => { self.pending_close = None; tasks.push(self.close_unsaved_dialog_window()); - tasks.push(self.update(Message::TabClose(i))); + tasks.push(self.update(Message::TabClose(self.tabs[i].id))); tasks.push(self.continue_tab_close_queue()); } crate::app::SaveContinuation::Quit if snapshot_is_current => { @@ -2768,7 +2769,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 => { @@ -2787,7 +2788,10 @@ 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 => self + .tabs + .get(tab_idx) + .map(|tab| crate::app::PendingClose::Tab(tab.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 3d0dd18ef..8560ba1c0 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -1954,13 +1954,23 @@ impl OpenCADStudio { Task::none() } - Message::TabClose(idx) => { + Message::TabClose(tab_id) => { self.hovered_doc_tab = None; - self.on_tab_close(idx) + // Resolve the stable id when the message is handled, not when + // it was built: an earlier close in the same batch may have + // shifted or removed the tab a captured index referred to. + match self.tabs.iter().position(|tab| tab.id == tab_id) { + Some(idx) => self.on_tab_close(idx), + // Already closed by an earlier message — nothing to do. + None => Task::none(), + } } Message::DocTabSaveAll => self.dispatch_command("SAVEALL"), + // Both close-* variants resolve ids against the live tab list at + // handling time (like `TabClose` above), so a batch that closed + // tabs earlier cannot leave them stale. Message::DocTabCloseAll => { let ids = self .tabs diff --git a/src/app/view/mod.rs b/src/app/view/mod.rs index 0c75a7a34..3fcf1149a 100644 --- a/src/app/view/mod.rs +++ b/src/app/view/mod.rs @@ -3127,7 +3127,7 @@ pub(super) fn doc_tab_bar<'a>( row![title_btn].spacing(0).align_y(iced::Center) } else { let close_btn = button(text("×").size(12)) - .on_press(Message::TabClose(idx)) + .on_press(Message::TabClose(tab.id)) .height(Fill) .padding([5, 9]) .style(move |theme: &Theme, status| { diff --git a/src/app/view/modal.rs b/src/app/view/modal.rs index 2915a28ec..473c16275 100644 --- a/src/app/view/modal.rs +++ b/src/app/view/modal.rs @@ -1632,9 +1632,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(tab_id)) => self .tabs - .get(*idx) + .iter() + .find(|t| t.id == *tab_id) .map(|t| t.tab_display_name()) .unwrap_or_default(), Some(super::super::PendingClose::Quit) => self