From 2e8284c191aa575f29d0da4a734ad580d82e0baa Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:26:51 -0400 Subject: [PATCH 1/2] Engine: a panic during a dry run puts the journal pause and the touch log back at any depth The pause of a dry run was cleared only by a panic at the top level. A command that started a dry run, nested in another, and panicked left journaling paused for the rest of the session (and the touch log open) once the outer command finished. Each command now keeps the pause count it started with, and its panic guard puts it back, closing the touch logs, whatever its depth. - Test: `a_dry_run_cut_short_by_a_panic_leaves_journaling_on`. --- crates/engine/src/lib.rs | 11 +++++++++-- crates/engine/src/tests/macros.rs | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index daea7f7e8..e492c4dd6 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -599,6 +599,9 @@ impl Session { return Err(EngineError::Disabled(id.to_string(), why)); } let before = self.active().map(|d| (d.uid, d.wb.clone(), d.selection.clone())); + // A dry run inside this command that a panic cuts short leaves journaling paused and its + // touch log open; the panic branch puts both back. + let paused_before = self.journal_paused; let params = if params.is_null() { Json::Object(Default::default()) } else { params }; let (first_group, group_epoch) = (self.next_group, self.group_epoch); // Whether a group was open on the document before the command, and how long its history was. @@ -666,11 +669,15 @@ impl Session { } d.calc.rebuild(&d.wb); } + if self.journal_paused != paused_before { + self.journal_paused = paused_before; + for d in self.docs.iter_mut() { + d.touched = None; + } + } if self.depth == 0 { self.trim_history(); self.group_step = None; - // A dry run the panic cut short never put journaling back on. - self.journal_paused = 0; } return Err(EngineError::Internal(id.to_string(), msg)); } diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 5df8faa11..153c832ca 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -417,6 +417,27 @@ fn a_dry_run_leaves_a_clean_workbook_clean() { assert!(!s.doc().unwrap().is_dirty()); } +#[test] +fn a_dry_run_cut_short_by_a_panic_leaves_journaling_on() { + // A panic inside a command that started a dry run, nested in another command: the panic + // guard of the nested call puts the pause and the touch log back, so the session journals + // the person's commands again. + crate::cmd::register_undoable_test_command("test.dryRunThenPanic", |s, _| { + s.journal_paused = s.journal_paused.saturating_add(1); + if let Ok(d) = s.doc_mut() { + d.touched = Some(Vec::new()); + } + panic!("in a dry run"); + }); + let mut s = s(); + let err = s.execute_nested("test.dryRunThenPanic", json!({})).unwrap_err().to_string(); + assert!(err.contains("in a dry run"), "{err}"); + assert_eq!((s.journal_paused, s.doc().unwrap().touched.is_none()), (0, true)); + let n = s.journal.len(); + s.execute("selection.set", json!({"range": "B2"})).unwrap(); + assert_eq!(s.journal.len(), n + 1, "journaled again"); +} + #[test] fn a_dry_run_leaves_the_journal_and_the_recording_start_as_they_were() { // A dry run journals nothing: journaling its nested commands would push a full journal past From 0a929eaf6884a038845c1f1587d501ffc9fdefa4 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:27:53 -0400 Subject: [PATCH 2/2] Engine: a rolled-back group takes back only the steps recorded in its workbook A rollback took back every step recorded since its group began, so an edit recorded in another workbook while the group was open was lost from the recording although the edit stayed. It now takes back only the steps recorded in the group's own workbook. - Test: the other workbook's edit in `stop_action_runs_only_on_its_own_and_is_not_recorded`. --- crates/engine/src/lib.rs | 2 +- crates/engine/src/record.rs | 17 +++++++++++------ crates/engine/src/tests/seams.rs | 10 ++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index b2aabd7da..a0305b5a8 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -879,7 +879,7 @@ impl Session { // 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))); + rec.cut_back(g.recorded.unwrap_or((0, 0)), g.doc); } // 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. diff --git a/crates/engine/src/record.rs b/crates/engine/src/record.rs index f1a7e29e5..b0d28ef40 100644 --- a/crates/engine/src/record.rs +++ b/crates/engine/src/record.rs @@ -73,13 +73,18 @@ impl Recorder { (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); + /// Takes back the steps recorded in the document with uid `doc` since `mark` (an undo group + /// on it was rolled back, so playing them back would make edits it no longer has). Steps in + /// other documents stay: their edits do too. Steps the buffer dropped stay counted. + pub(crate) fn cut_back(&mut self, (len, _): (usize, usize), doc: u64) { + let before = self.steps.len(); + let mut i = 0; + self.steps.retain(|s| { + i += 1; + i <= len || s.doc != doc + }); + if self.steps.len() != before { 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 48d5abf4c..3fa37b100 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -844,6 +844,16 @@ fn stop_action_runs_only_on_its_own_and_is_not_recorded() { 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"]); + // An edit recorded in another workbook while the group was open stays, as its edit does. + let first = s.active_index(); + let _left = s.group_begin("Left open").unwrap(); + set(&mut s, "A4", "four"); + s.new_workbook(); + set(&mut s, "B1", "other"); + s.set_active(first); + assert_eq!(s.execute("automate.stopAction", json!({})).unwrap(), json!({"stopped": true})); + assert_eq!(recorded_ids(&s), ["cell.set", "cell.set", "cell.set"], "the other workbook's edit stays"); + assert_eq!(s.recorder.as_ref().unwrap().steps.last().map(|st| st.params["cell"].clone()), Some(json!("B1"))); assert_eq!(s.stop_recording().unwrap().dropped, 0); }