From bfcf50aad3b0fc33af790d6ee4940688949c22f0 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:00:13 -0400 Subject: [PATCH 1/2] Engine: rolling back an undo group takes back the steps it recorded Stop Action (or any rollback) put a group's edits back but left the steps recorded while the group was open in the recording, so playing the recording back made edits the workbook no longer had. A group remembers where the recording was when it began, and its rollback cuts the recording back to there. - Test: `stop_action_runs_only_on_its_own_and_is_not_recorded`, which kept the rolled-back edit before. --- crates/engine/src/lib.rs | 9 +++++++++ crates/engine/src/record.rs | 15 +++++++++++++++ crates/engine/src/tests/seams.rs | 5 +++-- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 3f84203a3..b2aabd7da 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -96,6 +96,9 @@ struct OpenGroup { selection: Selection, /// The redo stack when the group began (a rollback puts it back). redo: Vec, + /// Where the recording was when the group began (`None` when nothing was recording): a + /// rollback takes back the steps recorded since, as it takes back their edits. + recorded: Option<(usize, usize)>, } /// Where the last [`Session::group_end`] or [`Session::group_rollback`] left its document: the @@ -776,6 +779,7 @@ impl Session { wb: d.wb.clone(), selection: d.selection.clone(), redo: d.redo.clone(), + recorded: self.recorder.as_ref().map(record::Recorder::mark), }; let token = GroupToken { id: g.id, doc: g.doc }; self.next_group = self.next_group.saturating_add(1); @@ -872,6 +876,11 @@ impl Session { d.undo.truncate(g.start); d.redo = g.redo; d.revision += 1; + // The steps recorded while the group was open go with their edits (all of them when the + // recording started inside the group). + if let Some(rec) = self.recorder.as_mut() { + rec.cut_back(g.recorded.unwrap_or((0, 0))); + } // The record of the last group step stays when the document is back exactly where that // step left it (this group began there); otherwise the rollback is where the group ended. let kept = self.group_step.as_ref().is_some_and(|st| st.doc == g.doc && st.len == g.start && Arc::ptr_eq(&st.after, &d.wb)); diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs index 4a5d7c4c9..f1a7e29e5 100644 --- a/crates/engine/src/record.rs +++ b/crates/engine/src/record.rs @@ -67,4 +67,19 @@ impl Recorder { self.dropped = self.dropped.saturating_add(1); } } + + /// Where the recording is now, for [`Recorder::cut_back`]. + pub(crate) fn mark(&self) -> (usize, usize) { + (self.steps.len(), self.dropped) + } + + /// Takes back the steps recorded since `mark` (an undo group that recorded them was rolled + /// back, so playing them back would make edits the workbook no longer has). + pub(crate) fn cut_back(&mut self, (len, dropped): (usize, usize)) { + if self.steps.len() > len { + self.steps.truncate(len); + self.bytes = self.steps.iter().map(|s| serde_json::to_vec(s).map_or(0, |v| v.len())).sum(); + } + self.dropped = self.dropped.min(dropped); + } } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index e93116120..48d5abf4c 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -837,12 +837,13 @@ fn stop_action_runs_only_on_its_own_and_is_not_recorded() { r.unwrap(); assert_eq!(v(&s, "A2").display(), "two"); assert_eq!(undo_labels(&s), ["Outer"], "the action kept its group"); - // On its own it stops a group left open; the recording keeps the edits and not the stop. + // On its own it stops a group left open; the recording keeps neither the stop nor the edits + // the stop rolled back, so playing it back makes only the edits the workbook has. let _left = s.group_begin("Left open").unwrap(); set(&mut s, "A3", "three"); assert_eq!(s.execute("automate.stopAction", json!({})).unwrap(), json!({"stopped": true})); assert!(v(&s, "A3").is_empty()); - assert_eq!(recorded_ids(&s), ["cell.set", "cell.set", "cell.set"]); + assert_eq!(recorded_ids(&s), ["cell.set", "cell.set"]); assert_eq!(s.stop_recording().unwrap().dropped, 0); } From 0aae1ffeea37e9d6a86deebb9c1c594d0aa9d835 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:00:13 -0400 Subject: [PATCH 2/2] Engine: an xlsx save refuses a macro that skipped the check, as a JSON save does `with_names` checked only the size of each item, so a macro built in code past the model's limits (a name with a line break) was written to the file and dropped the next time it opened. The xlsx save now runs `check` on every macro first. - Test: the macro past the limits in `a_macro_built_in_code_goes_through_the_size_check_too`. --- crates/engine/src/macros/mod.rs | 5 +++++ crates/engine/src/macros/tests.rs | 14 +++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/macros/mod.rs b/crates/engine/src/macros/mod.rs index ab692b5b7..8c23fbf0e 100644 --- a/crates/engine/src/macros/mod.rs +++ b/crates/engine/src/macros/mod.rs @@ -106,6 +106,11 @@ pub fn with_names(wb: &Workbook) -> crate::Result> { if wb.macros.is_empty() && wb.macros_unread.is_empty() && !wb.names.iter().any(stored) { return Ok(None); } + // A macro that skipped `check` (built in code) fails the save, as the JSON save does, rather + // than being written and dropped the next time the file opens. + for m in wb.macros.iter() { + check(m).map_err(crate::EngineError::Other)?; + } let mut out = wb.clone(); // One pass drops the items of the last save, and one writes the macros (a file can hold a // thousand macros and many thousands of names). diff --git a/crates/engine/src/macros/tests.rs b/crates/engine/src/macros/tests.rs index dc25de621..5f5531207 100644 --- a/crates/engine/src/macros/tests.rs +++ b/crates/engine/src/macros/tests.rs @@ -211,12 +211,16 @@ fn a_macro_built_in_code_goes_through_the_size_check_too() { }; assert!(big.check_limits().is_ok(), "the model's limits say nothing about the size"); assert!(check(&big).unwrap_err().contains("too large")); - let mut wb = Workbook::new(); - wb.macros.push(big); // A macro that fails `check` fails the save in either format, rather than being written and - // dropped the next time the file opens. - for file in ["book.xlsx", "book.json"] { - assert!(crate::io::save_bytes(&wb, file).is_err(), "{file}"); + // dropped the next time the file opens: too large, or past the model's limits. + let bad_name = Macro { name: "Two\nlines".into(), ..Macro::default() }; + assert!(check(&bad_name).is_err()); + for m in [big, bad_name] { + let mut wb = Workbook::new(); + wb.macros.push(m); + for file in ["book.xlsx", "book.json"] { + assert!(crate::io::save_bytes(&wb, file).is_err(), "{file}"); + } } }