From 2d6f4fcfdf29a52b20c1784715406d2b09a0123d Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Tue, 29 Sep 2026 04:28:16 +0100 Subject: [PATCH] fix(tabs): close by stable document id instead of vector index Bug: Message::TabClose carried a vector index captured when the view message was built. Closing several tabs in one batch (Close All / Close Others with unsaved-changes dialogs, or a queue of continuation updates) let an earlier close shift the vector before a later TabClose(idx) was handled: self.tabs.remove(idx) either panicked (index past the end) or closed a different tab than the one the user confirmed. Reproduce: 1. Open several drawings and modify each so they all prompt on close. 2. Use Close All, or close two tabs in quick succession so both messages are handled in one batch. 3. Before: panic in remove(idx), or the wrong tab closes while the tab the user chose stays open (or its work is lost). 4. After: the chosen tab closes; an id already closed by an earlier message is a no-op. Fix: TabClose now carries the tab's stable u64 id, resolved to a vector position when the message is handled, not when it is built; PendingClose::Tab stores ids the same way. All construction sites pass tab.id (a raw usize index no longer type-checks), and DocTabCloseAll/DocTabCloseOthers build their queues from live ids at handling time. A missing id means the tab is already gone, so the handler returns Task::none(). --- src/app/commands/fileops.rs | 5 +- src/app/control/actions.rs | 2 +- src/app/control/sheets.rs | 5 +- src/app/mod.rs | 14 +++-- src/app/update/command.rs | 108 +++++++++++++++++++++++++++++++++++- src/app/update/dialog.rs | 30 ++++++++-- src/app/update/file.rs | 18 +++--- src/app/update/mod.rs | 14 ++++- src/app/view/mod.rs | 2 +- src/app/view/modal.rs | 5 +- 10 files changed, 178 insertions(+), 25 deletions(-) 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