address review: count result rows in the scenario summary limit and reply/date vector storage in the history estimate

crates/core/src/addr.rs

@@ -147,10 +147,10 @@ impl RangeRef {
         RangeRef::new(CellRef::new(0, 0), CellRef::new(MAX_ROWS - 1, MAX_COLS - 1))
     }
     pub fn height(&self) -> u32 {
-        self.end.row.saturating_sub(self.start.row).saturating_add(1)
+        self.end.row - self.start.row + 1
     }
     pub fn width(&self) -> u32 {
-        self.end.col.saturating_sub(self.start.col).saturating_add(1)
+        self.end.col - self.start.col + 1
     }
     pub fn count(&self) -> u64 {
         self.height() as u64 * self.width() as u64
@@ -300,13 +300,6 @@ pub fn split_sheet(reference: &str) -> (Option<String>, &str) {
 mod tests {
     use super::*;
 
-    #[test]
-    fn hostile_range_dimensions_saturate_instead_of_wrapping() {
-        let r = RangeRef { start: CellRef::new(0, 0), end: CellRef::new(u32::MAX, u32::MAX) };
-        assert_eq!((r.height(), r.width()), (u32::MAX, u32::MAX));
-        assert_eq!(r.count(), u64::from(u32::MAX) * u64::from(u32::MAX));
-    }
-
     #[test]
     fn range_parse_rejects_signed_rows() -> Result<(), std::io::Error> {
         assert!(RangeRef::parse("+1:+3").is_none());

crates/engine/src/cmd/data/filter.rs

@@ -184,8 +184,7 @@ pub(crate) fn grow_autofilter(sh: &mut Sheet, at: CellRef) {
 pub(crate) fn apply_filter(wb: &mut Workbook, sheet: usize) {
     let Some(sh) = wb.sheet(sheet) else { return };
     let Some(af) = sh.autofilter.clone() else { return };
-    // An imported range can name rows past the grid; none of those exist to hide.
-    let r = RangeRef::new(af.range.start, CellRef::new(af.range.end.row.min(gridcraft_core::MAX_ROWS - 1), af.range.end.col));
+    let r = af.range;
     let mut hide = Vec::new();
     // Precompute per-criterion top10 threshold and average once, not per row.
     let mut col_stats: std::collections::HashMap<usize, (Option<f64>, f64)> = std::collections::HashMap::new();

crates/engine/src/cmd/data/tools.rs

@@ -5,9 +5,6 @@ use super::{
 
 // ---------------------------------------------------------------- tools
 
-/// Most cells (rows times compared columns) one Remove Duplicates call reads.
-const MAX_COMPARED_CELLS: u64 = 50_000_000;
-
 pub(super) fn remove_duplicates(s: &mut Session, p: &Json) -> Result<Json> {
     let (sheet, r) = data_target(s, p, "data.removeDuplicates")?;
     let d = s.doc()?;
@@ -27,10 +24,6 @@ pub(super) fn remove_duplicates(s: &mut Session, p: &Json) -> Result<Json> {
     if cols.is_empty() || cols.iter().any(|c| !(r.start.col..=r.end.col).contains(c)) {
         return Err(bad("data.removeDuplicates", "`columns` must name at least one column of the range"));
     }
-    // Rows are compared one by one, so a sparse sheet's enclosing rectangle is bounded like a list.
-    if r.count() > MAX_COMPARED_CELLS {
-        return Err(bad("data.removeDuplicates", "the range is too large"));
-    }
     let start = r.start.row + header as u32;
     let mut removed = 0;
     edit(s, |cx| {

crates/engine/src/cmd/data/validation.rs

@@ -289,14 +289,11 @@ pub(super) fn circle_invalid(s: &mut Session, _: &Json) -> Result<Json> {
     let sheet = d.wb.active_sheet();
     let Some(sh) = d.wb.sheet(sheet) else { return Err(EngineError::NoDocument) };
     let mut bad_cells = Vec::new();
-    // One scan allowance for the whole command: duplicate or overlapping ranges don't multiply it.
-    let mut budget = 100_000usize;
     for dv in &sh.validations {
         for r in &dv.ranges {
             let r = sh.used_range().and_then(|u| r.intersection(&u));
             let Some(r) = r else { continue };
-            for c in r.iter().take(budget) {
-                budget -= 1;
+            for c in r.iter().take(100_000) {
                 let text = sh.input_text(c);
                 if !text.is_empty() && check_validation(&d.wb, sheet, c, &text).is_some() {
                     bad_cells.push(c.a1());

crates/engine/src/cmd/edit/find.rs

@@ -130,11 +130,7 @@ pub(super) fn replace(s: &mut Session, p: &Json) -> Result<Json> {
         let mut n = 0;
         for si in sheets {
             let cells: Vec<(CellRef, String)> =
-                // A hidden formula's source is not searched: a count or error would reveal its text.
-                cx.wb
-                    .sheet(si)
-                    .map(|sh| sh.cells.iter().filter(|(c, _)| !crate::display::formula_hidden(&cx.wb, sh, *c)).map(|(c, _)| (c, sh.input_text(c))).collect())
-                    .unwrap_or_default();
+                cx.wb.sheet(si).map(|sh| sh.cells.iter().map(|(c, _)| (c, sh.input_text(c))).collect()).unwrap_or_default();
             for (c, text) in cells {
                 if (!all && c != active) || selection.as_ref().is_some_and(|sel| !sel.contains(c)) {
                     continue;

crates/engine/src/cmd/format/sizes.rs

@@ -140,44 +140,46 @@ pub(super) fn autofit_cols(s: &mut Session, p: &Json) -> Result<Json> {
     let measure = s.text_width.clone();
     edit(s, |cx| {
         let Some(sh) = cx.wb.sheet(si) else { return Ok(Json::Null) };
+        let mut widths = Vec::new();
         let mut measured: std::collections::HashMap<StyleId, std::collections::HashMap<String, f32>> = Default::default();
         let mut distinct = 0usize;
-        // Selected columns, then one pass over the populated cells (the store is row-oriented, so a
-        // scan per column would revisit every row for each selected column, empty or not).
-        let cols: std::collections::BTreeSet<u32> = lines.iter().flat_map(|&(a, b)| a..=b.min(a + 16383)).collect();
-        let mut maxes: std::collections::BTreeMap<u32, f32> = Default::default();
-        if let (Some(u), Some(&first), Some(&last)) = (sh.used_range(), cols.first(), cols.last()) {
-            let span = RangeRef::new(CellRef::new(u.start.row, first), CellRef::new(u.end.row, last));
-            for (cell, _) in sh.cells.iter_range(span) {
-                if !cols.contains(&cell.col) || sh.merge_at(cell).is_some_and(|m| !m.is_single()) {
-                    continue;
-                }
-                let id = sh.style_id(cell);
-                let st = cx.wb.styles.get(id);
-                let text = crate::display::cell_text(&cx.wb, sh, cell);
-                for line in text.lines() {
-                    let w = match &measure {
-                        Some(m) => {
-                            let known = measured.entry(id).or_default();
-                            match known.get(line) {
-                                Some(w) => *w,
-                                None => {
-                                    let w = m(line, &st.font, distinct < EXACT_TEXT_WIDTHS);
-                                    known.insert(line.to_owned(), w);
-                                    distinct += 1;
-                                    w
+        let used = sh.used_range();
+        for (a, b) in lines {
+            for c in a..=b.min(a + 16383) {
+                let Some(u) = used else { continue };
+                let col = RangeRef::new(CellRef::new(u.start.row, c), CellRef::new(u.end.row, c));
+                let mut max: f32 = 0.0;
+                for (cell, _) in sh.cells.iter_range(col) {
+                    if sh.merge_at(cell).is_some_and(|m| !m.is_single()) {
+                        continue;
+                    }
+                    let id = sh.style_id(cell);
+                    let st = cx.wb.styles.get(id);
+                    let text = crate::display::cell_text(&cx.wb, sh, cell);
+                    for line in text.lines() {
+                        let w = match &measure {
+                            Some(m) => {
+                                let known = measured.entry(id).or_default();
+                                match known.get(line) {
+                                    Some(w) => *w,
+                                    None => {
+                                        let w = m(line, &st.font, distinct < EXACT_TEXT_WIDTHS);
+                                        known.insert(line.to_owned(), w);
+                                        distinct += 1;
+                                        w
+                                    }
                                 }
                             }
-                        }
-                        None => approx_text_width(line, st.font.size, st.font.bold),
-                    };
-                    let w = w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell);
-                    let max = maxes.entry(cell.col).or_insert(0.0);
-                    *max = max.max(w);
+                            None => approx_text_width(line, st.font.size, st.font.bold),
+                        };
+                        max = max.max(w + 6.0 + st.align.indent as f32 * 9.0 + sh.filter_button_space(cell));
+                    }
+                }
+                if max > 0.0 {
+                    widths.push((c, max.clamp(8.0, 1785.0)));
                 }
             }
         }
-        let widths: Vec<(u32, f32)> = maxes.into_iter().filter(|&(_, m)| m > 0.0).map(|(c, m)| (c, m.clamp(8.0, 1785.0))).collect();
         let sh = cx.sheet_mut(si)?;
         for (c, w) in widths {
             let e = sh.cols.entry(c).or_default();

crates/engine/src/cmd/solver/reports.rs

@@ -52,14 +52,19 @@ pub(super) fn abs_ref(c: CellRef) -> String {
     format!("${}${}", gridcraft_core::col_to_letters(c.col), u64::from(c.row) + 1)
 }
 
+/// How many cells to the left and above a cell its name is searched for.
+const NAME_REACH: u32 = 1_000;
+
 /// A cell's name in the reports: the nearest text to its left and the nearest text above it.
 pub(super) fn cell_name(sh: &Sheet, c: CellRef) -> String {
     let text = |c: CellRef| match sh.value(c) {
         Value::Text(t) if !t.trim().is_empty() => Some(t.to_string()),
         _ => None,
     };
-    let left = (0..c.col).rev().find_map(|col| text(CellRef::new(c.row, col)));
-    let top = (0..c.row).rev().find_map(|row| text(CellRef::new(row, c.col)));
+    // Labels are next to their cells: looking further than this on a sparse sheet only costs time
+    // (a thousand cells near the last row would each probe a million empty coordinates).
+    let left = (c.col.saturating_sub(NAME_REACH)..c.col).rev().find_map(|col| text(CellRef::new(c.row, col)));
+    let top = (c.row.saturating_sub(NAME_REACH)..c.row).rev().find_map(|row| text(CellRef::new(row, c.col)));
     [left, top].into_iter().flatten().collect::<Vec<_>>().join(" ")
 }
 
@@ -438,3 +443,17 @@ pub(super) fn report(s: &mut Session, p: &Json) -> Result<Json> {
     }
     Ok(out)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn a_name_is_searched_only_a_bounded_distance() {
+        let mut sh = Sheet::new("S".to_string());
+        sh.set_cell(CellRef::new(0, 0), gridcraft_model::Cell::value(Value::text("Far")));
+        sh.set_cell(CellRef::new(5, 4), gridcraft_model::Cell::value(Value::text("Near")));
+        assert_eq!(cell_name(&sh, CellRef::new(5, 5)), "Near");
+        assert_eq!(cell_name(&sh, CellRef::new(0, NAME_REACH + 1)), "");
+    }
+}

crates/engine/src/cmd/whatif/advanced_filter.rs

@@ -83,9 +83,6 @@ pub(crate) fn criterion_matches(v: &Value, text: &str, crit: &Value) -> bool {
     }
 }
 
-/// Most (list row, criteria row) pairs one Advanced Filter tests.
-const MAX_FILTER_TESTS: u64 = 50_000_000;
-
 pub(super) fn advanced_filter(s: &mut Session, p: &Json) -> Result<Json> {
     const CMD: &str = "data.advancedFilter";
     let d = s.doc()?;
@@ -136,10 +133,6 @@ pub(super) fn advanced_filter(s: &mut Session, p: &Json) -> Result<Json> {
             rows.push(conds);
         }
     }
-    // Each list row is tried against every criteria row, so bound their product too.
-    if u64::from(list.height()) * rows.len() as u64 > MAX_FILTER_TESTS {
-        return Err(bad(CMD, "the list and criteria ranges together are too large"));
-    }
     let matches_row = |row: u32| -> bool {
         if rows.is_empty() {
             return true;

crates/engine/src/cmd/whatif/scenarios.rs

@@ -189,6 +189,9 @@ pub(super) fn scenario_delete(s: &mut Session, p: &Json) -> Result<Json> {
     Ok(json!({"deleted": removed.name}))
 }
 
+/// Most changing-cell-by-scenario cells one Scenario Summary writes.
+const MAX_SUMMARY_CELLS: usize = 1_000_000;
+
 pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result<Json> {
     const CMD: &str = "data.scenarioSummary";
     let d = s.doc()?;
@@ -211,17 +214,24 @@ pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result<Json> {
     // Changing cells of every scenario, first-seen order; per scenario its values.
     let mut changing: Vec<CellRef> = Vec::new();
     let mut per: Vec<HashMap<CellRef, Value>> = Vec::new();
+    let mut seen: std::collections::HashSet<CellRef> = Default::default();
     for sc in &scenarios {
         let (_, cells) = cell_list(&d.wb, &sc.changing, CMD, MAX_SCENARIO_CELLS)?;
         let mut m = HashMap::new();
         for (c, v) in cells.iter().zip(sc.values.iter()) {
-            if !changing.contains(c) {
+            if seen.insert(*c) {
                 changing.push(*c);
             }
             m.insert(*c, json_value(v, &d.wb));
         }
         per.push(m);
     }
+    // The summary has a cell for every changing cell in every scenario, filled from the current
+    // value where a scenario doesn't set it, so small disjoint scenarios multiply.
+    // Each result cell is a row of the summary too, with a cell per scenario.
+    if changing.len().saturating_add(results.len()).saturating_mul(per.len()) > MAX_SUMMARY_CELLS {
+        return Err(bad(CMD, "the scenarios together change too many cells for a summary"));
+    }
     let current: Vec<Value> = changing.iter().map(|c| sh.value(*c)).collect();
     let current_results: Vec<Value> = results.iter().map(|c| sh.value(*c)).collect();
     let mut trial = Trial::new(d);

crates/engine/src/cmd/whatif/tests.rs

@@ -299,10 +299,23 @@ fn consolidate_by_position_and_label() {
 }
 
 #[test]
-fn advanced_filter_refuses_a_list_times_criteria_product_over_the_work_limit() {
+fn scenario_summary_refuses_a_matrix_over_its_cell_limit() {
     let mut s = s();
-    s.execute("range.setValues", json!({"range": "A1", "values": [["Name"], ["A"]]})).unwrap();
-    s.execute("range.setValues", json!({"range": "H1", "values": [["Name"], ["A"]]})).unwrap();
-    // A sparse million-row list against 9,999 blank criteria rows would be ~10 billion tests.
-    assert!(s.execute("data.advancedFilter", json!({"range": "A2:A1048576", "criteria": "H1:H10000"})).is_err());
+    // 200 scenarios of 32 disjoint cells: 6,400 changing cells x 200 scenarios = 1.28 million cells.
+    for i in 0..200 {
+        let (a, b) = (i * 32 + 1, i * 32 + 32);
+        s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{a}:A{b}")})).unwrap();
+    }
+    assert!(s.execute("data.scenarioSummary", json!({})).is_err());
+}
+
+#[test]
+fn scenario_summary_counts_result_rows_in_its_cell_limit() {
+    let mut s = s();
+    // 900 scenarios of one distinct cell each are 810,000 cells; 255 result rows take it past the limit.
+    for i in 0..900 {
+        s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{}", i + 1)})).unwrap();
+    }
+    let err = s.execute("data.scenarioSummary", json!({"resultCells": "C1:C255"})).unwrap_err().to_string();
+    assert!(err.contains("too many cells"), "{err}");
 }

crates/engine/src/lib.rs

@@ -237,6 +237,22 @@ const JOURNAL_MAX_ENTRIES: usize = 10_000;
 pub(crate) const JOURNAL_MAX_BYTES: usize = 64 * 1024 * 1024;
 const UNDO_BYTES_LIMIT: usize = 128 * 1024 * 1024;
 
+/// Comment and hyperlink text of a sheet: a changed sheet's snapshot keeps its own copy of these
+/// strings (an edit deep-copies them), so they count against the history budget with the cells.
+fn sheet_text_bytes(sheet: &gridcraft_model::Sheet) -> usize {
+    let comments = sheet.comments.values().fold(0usize, |sum, c| {
+        // The vectors' own storage counts too: an empty reply still takes an element.
+        let replies = c.replies.iter().fold(0usize, |n, (a, t)| n.saturating_add(a.len()).saturating_add(t.len()));
+        let replies = replies.saturating_add(c.replies.capacity().saturating_mul(std::mem::size_of::<(String, String)>()));
+        let dates = c.dates.iter().fold(0usize, |n, d| n.saturating_add(d.len()));
+        let dates = dates.saturating_add(c.dates.capacity().saturating_mul(std::mem::size_of::<String>()));
+        sum.saturating_add(std::mem::size_of::<gridcraft_model::Comment>() + c.author.len() + c.text.len() + replies + dates)
+    });
+    sheet.hyperlinks.values().fold(comments, |sum, h| {
+        sum.saturating_add(std::mem::size_of::<gridcraft_model::Hyperlink>() + h.target.len() + h.tooltip.as_ref().map_or(0, String::len))
+    })
+}
+
 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() {
@@ -248,6 +264,7 @@ fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize {
         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 = bytes.saturating_add(sheet_text_bytes(sheet));
     }
     // Snapshots share the macro list until a change to it; a snapshot with its own list keeps it.
     if !older.macros.shares(&newer.macros) {

crates/engine/src/macros/run.rs

@@ -1280,18 +1280,9 @@ fn content_differs(b: Option<&Cell>, a: Option<&Cell>) -> bool {
 /// Objects added, changed and removed between two versions of a sheet.
 fn objects(b: &Sheet, a: &Sheet) -> (usize, usize, usize) {
     fn count<T: PartialEq>(b: &[T], a: &[T], id: impl Fn(&T) -> u32) -> (usize, usize, usize) {
-        // Objects grouped by id, so each lookup is a map probe instead of a scan of the list.
-        fn group<'a, T>(v: &'a [T], id: &impl Fn(&T) -> u32) -> std::collections::HashMap<u32, Vec<&'a T>> {
-            let mut m: std::collections::HashMap<u32, Vec<&T>> = std::collections::HashMap::new();
-            for x in v {
-                m.entry(id(x)).or_default().push(x);
-            }
-            m
-        }
-        let (gb, ga) = (group(b, &id), group(a, &id));
-        let added = a.iter().filter(|x| !gb.contains_key(&id(x))).count();
-        let removed = b.iter().filter(|x| !ga.contains_key(&id(x))).count();
-        let changed = b.iter().filter(|x| ga.get(&id(x)).is_some_and(|ys| ys.iter().any(|y| *y != *x))).count();
+        let added = a.iter().filter(|x| !b.iter().any(|y| id(y) == id(x))).count();
+        let removed = b.iter().filter(|x| !a.iter().any(|y| id(y) == id(x))).count();
+        let changed = b.iter().filter(|x| a.iter().any(|y| id(y) == id(x) && y != *x)).count();
         (added, changed, removed)
     }
     let parts = [count(&b.charts, &a.charts, |o| o.id), count(&b.images, &a.images, |o| o.id), count(&b.shapes, &a.shapes, |o| o.id)];

crates/engine/src/tests.rs

@@ -5574,12 +5574,24 @@ fn clearing_links_and_notes_keeps_allow_edit_ranges() {
 }
 
 #[test]
-fn replace_does_not_search_hidden_formula_source() {
+fn history_charges_a_changed_sheets_comment_text() {
     let mut s = s();
-    s.execute("cell.set", json!({"cell": "A1", "input": "=SEQUENCE(3)*7"})).unwrap();
-    s.execute("home.formatCells", json!({"range": "A1", "style": {"protection": {"locked": true, "hidden": true}}})).unwrap();
-    s.execute("review.protectSheet", json!({})).unwrap();
-    let hit = s.execute("edit.replace", json!({"what": "SEQUENCE", "with": "X"})).map_err(|e| e.to_string());
-    let miss = s.execute("edit.replace", json!({"what": "NOSUCHTEXT", "with": "X"})).map_err(|e| e.to_string());
-    assert_eq!(hit, miss);
+    s.execute("review.newNote", json!({"cell": "A1", "text": "n".repeat(30_000), "author": "Example"})).unwrap();
+    let older = (*s.doc().unwrap().wb).clone();
+    s.execute("cell.set", json!({"cell": "B2", "input": "x"})).unwrap();
+    let newer = (*s.doc().unwrap().wb).clone();
+    // The older snapshot owns the comment text now that the sheet changed under it.
+    assert!(crate::history_retained_bytes(&older, &newer) >= 30_000);
+}
+
+#[test]
+fn history_charges_the_storage_of_empty_comment_replies() {
+    let mut s = s();
+    s.execute("review.newComment", json!({"cell": "A1", "text": "c", "author": "Ann"})).unwrap();
+    for _ in 0..500 {
+        s.execute("review.replyComment", json!({"cell": "A1", "text": "", "author": ""})).unwrap();
+    }
+    let sheet = s.doc().unwrap().wb.sheet(0).unwrap().clone();
+    // 500 replies of no text are still 500 elements.
+    assert!(crate::sheet_text_bytes(&sheet) >= 500 * std::mem::size_of::<(String, String)>());
 }