mirror of
https://github.com/storytold/gridcraft.git
synced 2026-10-24 10:46:50 -06:00
Merge PR #1263: Bound Undo history growth after bulk edits
crates/engine/src/lib.rs: kept both the journal caps (#1230) and the undo byte budget; separate constants and call sites. crates/engine/src/cmd/edit.rs: undo pushes through push_history; the redo entry keeps the undone edit's selection (#1855 behaviour). crates/engine/src/tests.rs: kept both tests (redo_selects_the_redone_cell and the two history-bound tests).
This commit is contained in:
@@ -612,12 +612,13 @@ fn undo(s: &mut Session, p: &Json) -> Result<Json> {
|
||||
let now = d.wb.clone();
|
||||
for _ in 0..steps {
|
||||
let Some(e) = d.undo.pop() else { break };
|
||||
let cur_wb = d.wb.clone();
|
||||
// Redo goes back to the undone edit's cells, not wherever the user clicked before undoing (#1855).
|
||||
let cur = crate::HistoryEntry { label: e.label.clone(), wb: d.wb.clone(), selection: e.selection.clone() };
|
||||
d.redo.push(cur);
|
||||
let cur_selection = e.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);
|
||||
@@ -632,11 +633,12 @@ fn redo(s: &mut Session, p: &Json) -> Result<Json> {
|
||||
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);
|
||||
|
||||
@@ -61,6 +61,8 @@ pub struct HistoryEntry {
|
||||
pub label: String,
|
||||
pub wb: Arc<Workbook>,
|
||||
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.
|
||||
@@ -123,6 +125,43 @@ const JOURNAL_MAX_ENTRIES: usize = 10_000;
|
||||
/// `engine.journal`), and a `file.open` holds the whole file as base64: without a size cap a
|
||||
/// few opens of a large workbook keep tens of MB alive.
|
||||
pub(crate) const JOURNAL_MAX_BYTES: usize = 64 * 1024 * 1024;
|
||||
const UNDO_BYTES_LIMIT: usize = 128 * 1024 * 1024;
|
||||
|
||||
fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize {
|
||||
let mut bytes = std::mem::size_of::<Workbook>();
|
||||
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::<gridcraft_model::Sheet>());
|
||||
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<HistoryEntry>, label: String, wb: Arc<Workbook>, next: &Workbook, selection: Selection) {
|
||||
push_history_with_budget(history, label, wb, next, selection, UNDO_BYTES_LIMIT);
|
||||
}
|
||||
|
||||
fn push_history_with_budget(
|
||||
history: &mut Vec<HistoryEntry>,
|
||||
label: String,
|
||||
wb: Arc<Workbook>,
|
||||
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<String>, title: String) -> DocState {
|
||||
@@ -389,10 +428,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;
|
||||
}
|
||||
|
||||
@@ -269,6 +269,50 @@ fn redo_selects_the_redone_cell() {
|
||||
assert_eq!(s.doc().unwrap().selection.a1(), "A3");
|
||||
}
|
||||
|
||||
#[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::<usize>() <= 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();
|
||||
|
||||
@@ -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::<Band>());
|
||||
for (row, cols) in &band.rows {
|
||||
bytes = bytes.saturating_add(size_of::<u32>() + 3 * size_of::<usize>());
|
||||
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::<u32>() + size_of::<Cell>() + 3 * size_of::<usize>());
|
||||
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::<crate::cell::Formula>() + formula.text.len());
|
||||
if let Some(expr) = &formula.expr {
|
||||
expr.walk(&mut |_| bytes = bytes.saturating_add(size_of::<gridcraft_formula::Expr>()));
|
||||
}
|
||||
}
|
||||
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::<gridcraft_core::Array>() + array.data.len().saturating_mul(size_of::<gridcraft_core::Value>()),
|
||||
);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
bytes
|
||||
}
|
||||
/// Removes every cell in `r` and returns them.
|
||||
pub fn take_range(&mut self, r: RangeRef) -> Vec<(CellRef, Cell)> {
|
||||
let keys: Vec<CellRef> = self.iter_range(r).map(|(c, _)| c).collect();
|
||||
|
||||
@@ -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 |
|
||||
|
||||
Reference in New Issue
Block a user