From 4a6409f37b33fb640c391791835e09dbb8074011 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:37:18 +0000 Subject: [PATCH 1/2] chore(spreadsheet): drop Workbook::load's dead legacy-migrate branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Workbook::load()` carried a fallback — if the saved payload was not a V2 workbook but "looked legacy", migrate it — plus the two private helpers behind it, `is_legacy_workbook_payload` and `migrate_legacy_workbook`. The branch is unreachable. `deserialize_workbook` already treats both legacy V1 formats (single-sheet `#MP_SHEET_V2` and the older CSV) as `WorkbookFormat::V1Legacy` and returns a migrated one-sheet workbook, so by the time `load()` sees a legacy payload the `if let` above the fallback has already returned `Some`. The two conditions are exact complements — any string `deserialize_workbook` rejects is also rejected by `is_legacy_workbook_payload` — so the migrate branch and both helpers are dead code, confirmed by direct probe against `detect_workbook_format`. Behavior is unchanged: the legacy migration still happens, inside `deserialize_workbook`, which the existing `legacy_v1_migration_clears_undo_history` test already pins. --- .../spreadsheet-engine/src/workbook_api.rs | 48 +++---------------- 1 file changed, 6 insertions(+), 42 deletions(-) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs index 0cb4a68..3cb5c74 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs @@ -144,18 +144,15 @@ impl Workbook { } /// Load from the default save path (`generated/current.workbook.tsv`). - /// Falls back to legacy single-sheet format if the workbook file + /// Falls back to the legacy single-sheet format if the workbook file /// doesn't exist. pub fn load() -> Option { + // `deserialize_workbook` already migrates the legacy V1 formats + // (single-sheet `#MP_SHEET_V2` and the older CSV) into a one-sheet + // workbook, so there is no separate legacy branch to take here. let saved = load_saved_spreadsheet_state()?; - if let Some((sheets, active)) = deserialize_workbook(&saved) { - return Some(Self { sheets, active }); - } - if is_legacy_workbook_payload(&saved) { - Some(migrate_legacy_workbook(&saved)) - } else { - None - } + let (sheets, active) = deserialize_workbook(&saved)?; + Some(Self { sheets, active }) } /// Save to the default save path. @@ -495,22 +492,6 @@ impl Workbook { } } -fn is_legacy_workbook_payload(serialized: &str) -> bool { - let first = serialized.lines().next().unwrap_or_default().trim(); - first == "#MP_SHEET_V2" || first.contains(',') -} - -fn migrate_legacy_workbook(serialized: &str) -> Workbook { - let mut data = SpreadsheetData::default(); - data.deserialize(serialized); - data.undo_stack.clear(); - data.redo_stack.clear(); - Workbook { - sheets: vec![Sheet::new("Sheet 1", data)], - active: 0, - } -} - impl Default for Workbook { fn default() -> Self { Self::new() @@ -692,23 +673,6 @@ mod tests { assert_eq!(wb.get_raw(0, 0), "test"); } - #[test] - fn legacy_payload_detection_rejects_arbitrary_text() { - assert!(is_legacy_workbook_payload("#MP_SHEET_V2\n")); - assert!(is_legacy_workbook_payload("0,0,legacy\n")); - assert!(!is_legacy_workbook_payload("not a workbook")); - } - - #[test] - fn legacy_payload_migrates_to_one_sheet() { - let mut data = SpreadsheetData::default(); - data.set_cell(0, 0, "legacy"); - let migrated = migrate_legacy_workbook(&data.serialize()); - assert_eq!(migrated.sheet_count(), 1); - assert_eq!(migrated.get_raw(0, 0), "legacy"); - assert_eq!(migrated.active_sheet(), 0); - } - #[test] fn workbook_try_deserialize_reports_invalid_input() { let mut wb = Workbook::new(); From 89437838b7738ede47f00ee889774306a8e3a7b4 Mon Sep 17 00:00:00 2001 From: andodeki Date: Mon, 17 Aug 2026 05:37:28 +0000 Subject: [PATCH 2/2] test(spreadsheet): cover workbook_api.rs and util.rs remaining branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two weakest files left after the formula2 tranche. workbook_api.rs 88.80% -> 99.58%: - with_sheets_active with an empty sheet list falls back to a fresh single-sheet workbook. - set_active_sheet with an out-of-range index is a no-op. - remove_sheet: removing a sheet before the active one shifts the index down; removing the active one (or one after it) clamps to the new end. - apply(): the style arms the earlier tests did not touch — italic, underline, number format, background — and both SetBorders shapes (per-edge target writes each requested edge, BorderTarget::None clears all four). - Workbook::default() is a fresh single-sheet workbook. - save()/load() round-trip through the default path, a legacy payload still loads as a one-sheet workbook, and a non-workbook payload yields None. The disk test takes a new crate::test_disk lock so it cannot race the persistence module's disk test under parallel `cargo test`; that pre-existing test now takes the same lock. util.rs 90.91% -> 100%: - write_col_letters matches col_letters across one-, two- and three- letter columns and reuses the caller's buffer without stale tails. - adjust_formula_refs: a digit-bearing function name (LOG10) is not a cell ref; an 8-letter column overflowing u32 and a 30-digit row overflowing i64 are copied verbatim through the overflow fallbacks. Engine total 96.54% -> 98.24% (floor 96). Unit tests 308 -> 318; 9 integration tests unchanged. Deliberately uncovered: the mutex-poison recovery closure in the test_disk lock (unreachable unless a test panics while holding it), and the disk test's restore-to-clean branch (runs only when no save file pre-exists — the CI-normal path; its inverse is the branch that runs otherwise). Both match the existing persistence test's save/restore convention. --- .../spreadsheet/spreadsheet-engine/src/lib.rs | 13 ++ .../spreadsheet-engine/src/persistence.rs | 1 + .../spreadsheet-engine/src/util.rs | 42 ++++ .../spreadsheet-engine/src/workbook_api.rs | 186 ++++++++++++++++++ 4 files changed, 242 insertions(+) diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs index 654d220..460d0ef 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs @@ -31,3 +31,16 @@ pub use sheet::Sheet; pub use style::{BorderEdge, BorderTarget, CellAlign, CellStyle, Color, NumberFormat}; pub use undo::{Change, ChangeSet}; pub use workbook_api::{Workbook, WorkbookCommand}; + +#[cfg(test)] +pub(crate) mod test_disk { + use std::sync::{Mutex, MutexGuard}; + + static LOCK: Mutex<()> = Mutex::new(()); + + /// Serialize tests that touch the shared `generated/` save files, so + /// parallel `cargo test` workers cannot clobber each other's fixtures. + pub(crate) fn lock() -> MutexGuard<'static, ()> { + LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) + } +} diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs index a2ed84e..7b9946a 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs @@ -197,6 +197,7 @@ mod tests { /// clobbering another test's fixture. #[test] fn the_legacy_file_is_read_when_the_current_one_is_missing() { + let _disk = crate::test_disk::lock(); let current = sheet_generated_file_path(); let legacy = sheet_generated_legacy_file_path(); let saved_current = fs::read_to_string(¤t).ok(); diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs index 044cf5b..10124b2 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/util.rs @@ -368,4 +368,46 @@ mod tests { // =AZ10 with 0,+1 → =BA10 assert_eq!(adjust_formula_refs("=AZ10", 0, 1), "=BA10"); } + + // --- write_col_letters (no-alloc variant used by the grid draw loop) --- + + #[test] + fn write_col_letters_matches_col_letters() { + for i in [0u32, 1, 25, 26, 27, 51, 52, 701, 702, 18277] { + let mut buf = String::from("leftover"); + write_col_letters(i, &mut buf); + assert_eq!(buf, col_letters(i), "write_col_letters({i}) diverged"); + } + // The buffer is cleared on entry, so a shorter label leaves no + // stale tail from the previous call. + let mut buf = String::new(); + write_col_letters(702, &mut buf); // "AAA" + write_col_letters(0, &mut buf); // "A" + assert_eq!(buf, "A"); + } + + // --- adjust_formula_refs: digit-bearing names and overflow guards --- + + #[test] + fn adjust_refs_function_name_with_digits() { + // LOG10 has digits in its identifier; it must not be treated as a + // cell reference. + assert_eq!(adjust_formula_refs("=LOG10(100)", 1, 1), "=LOG10(100)"); + } + + #[test] + fn adjust_refs_overflowing_column_is_left_alone() { + // An 8-letter column exceeds u32 in `col_str_to_index`, so the + // scanner hits the overflow fallback. With a zero delta the output + // is verbatim, proving the fallback copies rather than mangles. + assert_eq!(adjust_formula_refs("=AAAAAAAA1", 0, 0), "=AAAAAAAA1"); + } + + #[test] + fn adjust_refs_overflowing_row_is_left_alone() { + // A 30-digit row does not fit i64, so the scanner copies it verbatim. + let row = "9".repeat(30); + let formula = format!("=A{}", row); + assert_eq!(adjust_formula_refs(&formula, 1, 1), formula); + } } diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs index 3cb5c74..f6fd8a9 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs @@ -937,4 +937,190 @@ mod tests { wb.set_active_sheet(1); assert_eq!(wb.get_display_value(0, 0), "20"); } + + // ── Previously-uncovered branches ─────────────────────────────────── + + /// An empty sheet list cannot build a workbook, so `with_sheets_active` + /// falls back to a fresh single-sheet workbook. + #[test] + fn with_sheets_active_empty_defaults_to_one_sheet() { + let wb = Workbook::with_sheets_active(vec![], 7); + assert_eq!(wb.sheet_count(), 1); + assert_eq!(wb.active_sheet(), 0); + } + + /// Setting an out-of-range active index is a no-op, not a panic. + #[test] + fn set_active_sheet_out_of_range_is_noop() { + let mut wb = Workbook::new(); + wb.set_active_sheet(99); + assert_eq!(wb.active_sheet(), 0); + } + + /// Removing a sheet *before* the active one shifts the active index + /// down so it keeps pointing at the same sheet. + #[test] + fn remove_sheet_before_active_shifts_active_down() { + let mut wb = Workbook::new(); + wb.add_sheet("B"); + wb.add_sheet("C"); + wb.add_sheet("D"); // ["Sheet 1", "B", "C", "D"] + wb.set_active_sheet(2); // C + let removed = wb.remove_sheet(0); // remove "Sheet 1" (before C) + assert!(removed.is_some()); + assert_eq!(wb.sheet_count(), 3); + assert_eq!(wb.active_sheet(), 1); // shifted down, still C + assert_eq!(wb.active_sheet_name(), "C"); + } + + /// Removing the active sheet (or one after it) clamps the active index + /// to the new last sheet. + #[test] + fn remove_sheet_at_or_after_active_clamps_to_end() { + let mut wb = Workbook::new(); + wb.add_sheet("B"); + wb.add_sheet("C"); // ["Sheet 1", "B", "C"] + wb.set_active_sheet(2); // C (last) + let removed = wb.remove_sheet(1); // remove B; active 2 >= new len 2 + assert!(removed.is_some()); + assert_eq!(wb.sheet_count(), 2); + assert_eq!(wb.active_sheet(), 1); // clamped to the new last sheet + assert_eq!(wb.active_sheet_name(), "C"); + } + + /// `apply` forwards the remaining style commands to the active sheet: + /// italic, underline, number format, and background. + #[test] + fn apply_covers_italic_underline_number_format_and_background() { + let mut wb = Workbook::new(); + wb.apply_batch([ + WorkbookCommand::ToggleItalic { row: 0, col: 0 }, + WorkbookCommand::ToggleUnderline { row: 0, col: 0 }, + WorkbookCommand::SetNumberFormat { + row: 0, + col: 0, + format: NumberFormat::Currency, + }, + WorkbookCommand::SetBackground { + row: 0, + col: 0, + color: Some(Color::new(1.0, 0.0, 0.0, 1.0)), + }, + ]); + let cell = wb + .active_sheet_data() + .cells + .get(&CellId::new(0, 0)) + .unwrap(); + assert!(cell.style.italic); + assert!(cell.style.underline); + assert_eq!(cell.style.number_format, NumberFormat::Currency); + assert_eq!(cell.style.bg_color, Some(Color::new(1.0, 0.0, 0.0, 1.0))); + } + + /// `apply`'s SetBorders arm: a per-edge target writes each requested + /// edge, and `BorderTarget::None` clears all four. + #[test] + fn apply_set_borders_covers_clear_and_per_edge() { + let mut wb = Workbook::new(); + let edge = BorderEdge { + color: Color::new(0.0, 0.0, 1.0, 1.0), + width: 2.0, + }; + wb.apply(WorkbookCommand::SetBorders { + row: 0, + col: 0, + target: BorderTarget::All, + edge, + top: true, + bottom: true, + left: true, + right: true, + }); + let cell = wb + .active_sheet_data() + .cells + .get(&CellId::new(0, 0)) + .unwrap(); + assert_eq!(cell.style.border_top, Some(edge)); + assert_eq!(cell.style.border_bottom, Some(edge)); + assert_eq!(cell.style.border_left, Some(edge)); + assert_eq!(cell.style.border_right, Some(edge)); + + wb.apply(WorkbookCommand::SetBorders { + row: 0, + col: 0, + target: BorderTarget::None, + edge, + top: false, + bottom: false, + left: false, + right: false, + }); + let cell = wb + .active_sheet_data() + .cells + .get(&CellId::new(0, 0)) + .unwrap(); + assert_eq!(cell.style.border_top, None); + assert_eq!(cell.style.border_bottom, None); + assert_eq!(cell.style.border_left, None); + assert_eq!(cell.style.border_right, None); + } + + /// `Workbook::default()` is a fresh single-sheet workbook. + #[test] + fn workbook_default_is_a_fresh_single_sheet() { + let wb = Workbook::default(); + assert_eq!(wb.sheet_count(), 1); + assert_eq!(wb.active_sheet_name(), "Sheet 1"); + } + + /// `save()` / `load()` round-trip through the default path, a legacy + /// single-sheet payload still loads, and a non-workbook payload yields + /// `None`. + /// + /// Runs single-threaded against the shared default filenames via + /// `crate::test_disk::lock()`, and saves/restores whatever was there + /// before — the same convention as the persistence module's disk tests. + #[test] + fn workbook_save_load_legacy_and_reject_via_default_path() { + let _disk = crate::test_disk::lock(); + let dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("generated"); + let current = dir.join("current.workbook.tsv"); + let saved_current = std::fs::read_to_string(¤t).ok(); + + // Round-trip through the default path. + let mut wb = Workbook::new(); + wb.set_cell(0, 0, "disk"); + wb.set_cell(0, 1, "=A1&\"!\""); + wb.save().expect("save failed"); + let loaded = Workbook::load().expect("load failed"); + assert_eq!(loaded.sheet_count(), 1); + assert_eq!(loaded.active_sheet(), 0); + assert_eq!(loaded.get_raw(0, 0), "disk"); + assert_eq!(loaded.get_display_value(0, 1), "disk!"); + + // A legacy single-sheet payload still loads as a one-sheet workbook. + let mut legacy_data = SpreadsheetData::default(); + legacy_data.set_cell(0, 0, "legacy"); + std::fs::write(¤t, legacy_data.serialize()).expect("write legacy"); + let migrated = Workbook::load().expect("legacy load failed"); + assert_eq!(migrated.sheet_count(), 1); + assert_eq!(migrated.get_raw(0, 0), "legacy"); + + // A non-workbook payload yields None. + std::fs::write(¤t, "not a workbook\n").expect("write garbage"); + assert!(Workbook::load().is_none()); + + // Restore whatever was there before. + match saved_current { + Some(text) => { + std::fs::write(¤t, text).ok(); + } + None => { + std::fs::remove_file(¤t).ok(); + } + } + } }