crates/apps/spreadsheet/spreadsheet-engine/src/lib.rs

@@ -31,16 +31,3 @@ 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())
-    }
-}

crates/apps/spreadsheet/spreadsheet-engine/src/persistence.rs

@@ -197,7 +197,6 @@ 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(&current).ok();

crates/apps/spreadsheet/spreadsheet-engine/src/util.rs

@@ -368,46 +368,4 @@ 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);
-    }
 }

crates/apps/spreadsheet/spreadsheet-engine/src/workbook_api.rs

@@ -144,15 +144,18 @@ impl Workbook {
     }
 
     /// Load from the default save path (`generated/current.workbook.tsv`).
-    /// Falls back to the legacy single-sheet format if the workbook file
+    /// Falls back to legacy single-sheet format if the workbook file
     /// doesn't exist.
     pub fn load() -> Option<Self> {
-        // `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()?;
-        let (sheets, active) = deserialize_workbook(&saved)?;
+        if let Some((sheets, active)) = deserialize_workbook(&saved) {
-        Some(Self { sheets, active })
+            return Some(Self { sheets, active });
+        }
+        if is_legacy_workbook_payload(&saved) {
+            Some(migrate_legacy_workbook(&saved))
+        } else {
+            None
+        }
     }
 
     /// Save to the default save path.
@@ -492,6 +495,22 @@ 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()
@@ -673,6 +692,23 @@ 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();
@@ -937,190 +973,4 @@ 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(&current).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(&current, 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(&current, "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(&current, text).ok();
-            }
-            None => {
-                std::fs::remove_file(&current).ok();
-            }
-        }
-    }
 }