From 72db972de306a165af5296cb2e5f932a19c0cc02 Mon Sep 17 00:00:00 2001 From: derechopiano <257026466+derechopiano@users.noreply.github.com> Date: Wed, 14 Oct 2026 08:23:57 +0100 Subject: [PATCH] fix(engine): bound Undo history by retained cell storage (#1227) --- crates/engine/src/cmd/edit.rs | 10 ++++--- crates/engine/src/lib.rs | 44 ++++++++++++++++++++++++++++--- crates/engine/src/tests.rs | 44 +++++++++++++++++++++++++++++++ crates/model/src/store.rs | 49 +++++++++++++++++++++++++++++++++++ docs/architecture.md | 6 ++++- 5 files changed, 144 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 82dbe0be7..30effd9fa 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -601,11 +601,12 @@ fn undo(s: &mut Session, p: &Json) -> Result { let now = d.wb.clone(); for _ in 0..steps { let Some(e) = d.undo.pop() else { break }; - let cur = crate::HistoryEntry { label: e.label.clone(), wb: d.wb.clone(), selection: d.selection.clone() }; - d.redo.push(cur); + let cur_wb = d.wb.clone(); + let cur_selection = d.selection.clone(); label = e.label.clone(); d.wb = e.wb; d.selection = e.selection; + crate::push_history(&mut d.redo, label.clone(), cur_wb, &d.wb, cur_selection); } super::extra::keep_watch_list(&now, &mut d.wb); d.calc.rebuild(&d.wb); @@ -620,11 +621,12 @@ fn redo(s: &mut Session, p: &Json) -> Result { let now = d.wb.clone(); for _ in 0..steps { let Some(e) = d.redo.pop() else { break }; - let cur = crate::HistoryEntry { label: e.label.clone(), wb: d.wb.clone(), selection: d.selection.clone() }; - d.undo.push(cur); + let cur_wb = d.wb.clone(); + let cur_selection = d.selection.clone(); label = e.label.clone(); d.wb = e.wb; d.selection = e.selection; + crate::push_history(&mut d.undo, label.clone(), cur_wb, &d.wb, cur_selection); } super::extra::keep_watch_list(&now, &mut d.wb); d.calc.rebuild(&d.wb); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 258a1cb72..b97dd23c6 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -61,6 +61,8 @@ pub struct HistoryEntry { pub label: String, pub wb: Arc, pub selection: Selection, + /// Estimate of storage this snapshot retains beyond the next workbook version. + pub retained_bytes: usize, } /// The clipboard: a block of cells (with formulas and styles) or marching-ants reference. @@ -116,6 +118,43 @@ pub struct DocState { static NEXT_UID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); const UNDO_LIMIT: usize = 100; +const UNDO_BYTES_LIMIT: usize = 128 * 1024 * 1024; + +fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize { + let mut bytes = std::mem::size_of::(); + for (index, sheet) in older.sheets.iter().enumerate() { + // Moving or inserting sheets must not charge unchanged sheets at a new index. + if newer.sheets.iter().any(|next| Arc::ptr_eq(sheet, next)) { + continue; + } + bytes = bytes.saturating_add(std::mem::size_of::()); + let empty = gridcraft_model::CellStore::new(); + let next = newer.sheets.get(index).map_or(&empty, |next| &next.cells); + bytes = bytes.saturating_add(sheet.cells.retained_bytes_against(next)); + } + bytes +} + +fn push_history(history: &mut Vec, label: String, wb: Arc, next: &Workbook, selection: Selection) { + push_history_with_budget(history, label, wb, next, selection, UNDO_BYTES_LIMIT); +} + +fn push_history_with_budget( + history: &mut Vec, + label: String, + wb: Arc, + next: &Workbook, + selection: Selection, + byte_budget: usize, +) { + let retained_bytes = history_retained_bytes(&wb, next); + history.push(HistoryEntry { label, wb, selection, retained_bytes }); + let mut total = history.iter().fold(0usize, |sum, entry| sum.saturating_add(entry.retained_bytes)); + // Always preserve the latest action, even if that one edit exceeds the budget. + while history.len() > 1 && (history.len() > UNDO_LIMIT || total > byte_budget) { + total = total.saturating_sub(history.remove(0).retained_bytes); + } +} impl DocState { pub fn new(mut wb: Workbook, path: Option, title: String) -> DocState { @@ -350,10 +389,7 @@ impl Session { && let Some(d) = self.docs.iter_mut().find(|d| d.uid == uid) && !Arc::ptr_eq(&d.wb, &wb) { - d.undo.push(HistoryEntry { label: spec.label.trim_end_matches('…').to_string(), wb, selection: sel }); - if d.undo.len() > UNDO_LIMIT { - d.undo.remove(0); - } + push_history(&mut d.undo, spec.label.trim_end_matches('…').to_string(), wb, &d.wb, sel); d.redo.clear(); d.revision += 1; } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ae499a189..c4fb1cc8e 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -253,6 +253,50 @@ fn undo_redo() { assert!(s.doc().unwrap().is_dirty()); } +#[test] +fn bulk_formula_history_is_bounded_and_latest_edit_still_undoes() { + use std::sync::Arc; + + use gridcraft_model::{Cell, Formula, Workbook}; + + let mut current = Arc::new(Workbook::new()); + let mut history = Vec::new(); + let budget = 64 * 1024; + for version in 0..5 { + let previous = current.clone(); + let mut next = (*previous).clone(); + let sheet = next.sheet_mut(0).unwrap(); + for row in 0..128 { + sheet.cells.set(CellRef::new(row, 0), Cell::formula(Formula::new(&format!("=B{}+{}", row + 1, version)))); + } + current = Arc::new(next); + crate::push_history_with_budget(&mut history, format!("edit {version}"), previous, ¤t, crate::Selection::default(), budget); + } + assert!(history.len() < 5, "bulk edits should evict old steps before reaching the count limit"); + assert_eq!(history.last().unwrap().label, "edit 4"); + assert!(history.len() == 1 || history.iter().map(|e| e.retained_bytes).sum::() <= budget); + + let mut session = s(); + let doc = session.doc_mut().unwrap(); + doc.wb = current; + doc.undo = history; + session.execute("edit.undo", json!({})).unwrap(); + assert_eq!(session.doc().unwrap().wb.active().unwrap().cells.get(CellRef::new(0, 0)).unwrap().formula.as_ref().unwrap().text, "B1+3"); + session.execute("edit.redo", json!({})).unwrap(); + assert_eq!(session.doc().unwrap().wb.active().unwrap().cells.get(CellRef::new(0, 0)).unwrap().formula.as_ref().unwrap().text, "B1+4"); +} + +#[test] +fn small_edits_keep_the_full_undo_depth() { + let mut session = s(); + for number in 0..100 { + session.execute("cell.set", json!({"cell": "A1", "input": number.to_string()})).unwrap(); + } + assert_eq!(session.doc().unwrap().undo.len(), 100); + session.execute("edit.undo", json!({"steps": 100})).unwrap(); + assert_eq!(v(&session, "A1"), Value::Empty); +} + #[test] fn shape_text_edit_undo_redo_and_noop() { let mut s = s(); diff --git a/crates/model/src/store.rs b/crates/model/src/store.rs index d6a9c4561..939358764 100644 --- a/crates/model/src/store.rs +++ b/crates/model/src/store.rs @@ -154,6 +154,55 @@ impl CellStore { } out } + /// Approximate heap storage kept alive by this snapshot's bands that `newer` no longer + /// shares. This is for bounding Undo history, not for reporting process memory: BTreeMap + /// nodes and allocator overhead vary by platform. Shared formulas and values are counted + /// only when the newer band no longer holds the same Arc. + pub fn retained_bytes_against(&self, newer: &CellStore) -> usize { + use std::mem::size_of; + + let mut bytes = 0usize; + for (key, band) in &self.bands { + let next = newer.bands.get(key); + if next.is_some_and(|b| Arc::ptr_eq(b, band)) { + continue; + } + bytes = bytes.saturating_add(size_of::()); + for (row, cols) in &band.rows { + bytes = bytes.saturating_add(size_of::() + 3 * size_of::()); + for (col, cell) in cols { + // Cells and BTreeMap nodes are duplicated with a cloned band even when + // their formula/value Arcs are still shared with the next snapshot. + bytes = bytes.saturating_add(size_of::() + size_of::() + 3 * size_of::()); + let next_cell = next.and_then(|b| b.rows.get(row)).and_then(|r| r.get(col)); + if let Some(formula) = &cell.formula + && !next_cell.and_then(|c| c.formula.as_ref()).is_some_and(|f| Arc::ptr_eq(f, formula)) + { + bytes = bytes.saturating_add(size_of::() + formula.text.len()); + if let Some(expr) = &formula.expr { + expr.walk(&mut |_| bytes = bytes.saturating_add(size_of::())); + } + } + match &cell.value { + gridcraft_core::Value::Text(text) + if !next_cell.and_then(|c| c.value.as_text()).is_some_and(|t| std::ptr::eq(t, text.as_ref())) => + { + bytes = bytes.saturating_add(text.len()); + } + gridcraft_core::Value::Array(array) + if !next_cell.is_some_and(|c| matches!(&c.value, gridcraft_core::Value::Array(a) if Arc::ptr_eq(a, array))) => + { + bytes = bytes.saturating_add( + size_of::() + array.data.len().saturating_mul(size_of::()), + ); + } + _ => {} + } + } + } + } + bytes + } /// Removes every cell in `r` and returns them. pub fn take_range(&mut self, r: RangeRef) -> Vec<(CellRef, Cell)> { let keys: Vec = self.iter_range(r).map(|(c, _)| c).collect(); diff --git a/docs/architecture.md b/docs/architecture.md index 5695036b2..80d1062d5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1,6 +1,6 @@ # Architecture -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-10 · **Change:** major (created from the code on main; supersedes the local-only plan/architecture.md for committed docs) · **Target:** Microsoft Excel (Microsoft 365) +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-14 · **Change:** minor (bound Undo's retained cell storage) · **Target:** Microsoft Excel (Microsoft 365) How GridCraft is built today. Rules for contributors are in [`AGENTS.md`](../AGENTS.md). @@ -35,6 +35,9 @@ including tests, 2026-10-10. `data.sortAscending`) plus primitives (`cell.set`, `selection.set`, `range.setValues`). `Session::execute` catches escaped panics and restores the workbook (never-crash guard). 2. **Edits** change the model through copy-on-write bands; undo keeps the previous snapshot. + Undo retains at most 100 steps and trims older steps when its estimated retained cell storage + exceeds 128 MiB. The newest step remains undoable even if that step alone exceeds the budget. + The estimate is a history policy, not a measure or hard cap of process memory. 3. **Recalc** marks dependents of changed cells dirty through the dependency graph (with range nodes for range dependents), evaluates in topological order, spills dynamic arrays and reports `#SPILL!`/cycles. Volatile functions are always dirty. @@ -68,4 +71,5 @@ rpm, tarball), FreeBSD, and web (WASM; WebGPU or WebGL2). Release workflows in | Date | Change | Summary | |---|---|---| +| 2026-10-14 | minor | Document Undo's retained-cell-storage budget and latest-step guarantee | | 2026-10-10 | major | Created from the code on main: crates, layers, data flow, agent control, known gaps |