diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs index 392dbee..ee26f1e 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/data.rs @@ -306,6 +306,90 @@ mod serialization_tests { sheet.redo(); assert_eq!(sheet.get_raw(0, 0), "test"); } + + /// Named ranges must survive the serialize→deserialize round-trip: + /// `serialize_block` writes a `NAME` line per range and + /// `deserialize_block_line` reads it back. + #[test] + fn named_ranges_survive_roundtrip() { + let mut original = SpreadsheetData::default(); + original.set_cell(0, 0, "10"); + original.set_cell(1, 0, "20"); + original + .named_ranges + .insert("Total".to_string(), (0, 0, 1, 0)); + + let serialized = original.serialize(); + let mut restored = SpreadsheetData::default(); + restored.deserialize(&serialized); + + assert_eq!( + restored.named_ranges.get("Total"), + Some(&(0, 0, 1, 0)), + "the named range must come back verbatim" + ); + } + + /// A cell's text color survives the round-trip. The background colour + /// is covered by the existing round-trip test; the text colour was + /// serialized but never read back in a test. + #[test] + fn text_color_survives_roundtrip() { + let mut original = SpreadsheetData::default(); + original.set_cell(0, 0, "x"); + original + .cells + .get_mut(&CellId::new(0, 0)) + .unwrap() + .style + .text_color = Some(Color::new(1.0, 0.5, 0.25, 1.0)); + + let serialized = original.serialize(); + let mut restored = SpreadsheetData::default(); + restored.deserialize(&serialized); + + assert_eq!( + restored + .cells + .get(&CellId::new(0, 0)) + .unwrap() + .style + .text_color, + Some(Color::new(1.0, 0.5, 0.25, 1.0)) + ); + } + + /// Unknown block tags are ignored for forward compatibility, and a + /// `NAME` line with a malformed range inserts nothing. + #[test] + fn unknown_tags_and_malformed_name_lines_are_ignored() { + let mut sheet = SpreadsheetData::default(); + sheet.deserialize_block_line("FUTURE\twhatever\tgoes\there"); + assert!(sheet.cells.is_empty()); + assert!(sheet.named_ranges.is_empty()); + + sheet.deserialize_block_line("NAME\tBroken\t0,1,2"); // only 3 of 4 parts + assert!(sheet.named_ranges.is_empty()); + } + + /// The legacy CSV format carries cell metadata as `|`-separated + /// segments (`F` formula, `B` bold). Both must be honoured. + #[test] + fn legacy_csv_metadata_segments_are_honoured() { + let mut sheet = SpreadsheetData::default(); + sheet.deserialize("0,0,plain\n1,0,=A1|F=1+1\n2,0,bold|B"); + assert_eq!(sheet.get_raw(0, 0), "plain"); + assert_eq!( + sheet + .cells + .get(&CellId::new(1, 0)) + .unwrap() + .formula + .as_deref(), + Some("=1+1") + ); + assert!(sheet.cells.get(&CellId::new(2, 0)).unwrap().style.bold); + } } #[cfg(test)] @@ -714,6 +798,50 @@ mod mutation_helper_tests { assert_eq!(sheet.get_raw(0, 0), "1"); } + /// `BorderTarget::None` clears all four edges — the "remove borders" + /// toolbar action — leaving no stale edge behind. + #[test] + fn apply_command_set_borders_none_clears_all_edges() { + let mut sheet = SpreadsheetData::default(); + sheet.apply_command(WorkbookCommand::SetBorders { + row: 0, + col: 0, + target: BorderTarget::All, + edge: BorderEdge { + color: Color::new(0.0, 0.0, 0.0, 1.0), + width: 1.0, + }, + top: true, + bottom: true, + left: true, + right: true, + }); + let cell = sheet.cells.get(&CellId::new(0, 0)).unwrap(); + assert!(cell.style.border_top.is_some()); + assert!(cell.style.border_bottom.is_some()); + assert!(cell.style.border_left.is_some()); + assert!(cell.style.border_right.is_some()); + + sheet.apply_command(WorkbookCommand::SetBorders { + row: 0, + col: 0, + target: BorderTarget::None, + edge: BorderEdge { + color: Color::new(0.0, 0.0, 0.0, 1.0), + width: 1.0, + }, + top: false, + bottom: false, + left: false, + right: false, + }); + let cell = sheet.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); + } + #[test] fn mutate_cell_creates_and_records() { let mut sheet = SpreadsheetData::default(); @@ -1254,7 +1382,13 @@ impl<'a> formula2::EvalContext for DataEvalContext<'a> { if !cell.computed_value.is_empty() { values.push(Self::parse_cell_computed_value(&cell.computed_value)?); } else if !self.visiting.borrow_mut().insert((r, c)) { - values.push(Value2::Number(0.0)); + // Re-entered a formula cell mid-evaluation: a + // reference cycle. The small-range path in + // `get_cell_value` reports CycleDetected here; + // the large-range path used to silently push + // 0.0, so a cyclic formula inside a >64-cell + // range contributed zero instead of #CYCLE!. + return Err(FormulaError::CycleDetected); } else { let result = match cell.cached_ast { Some(ref ast) => formula2::evaluate(ast, self), @@ -3449,4 +3583,28 @@ mod dep_graph_branch_tests { sheet.set_cell(9, 0, "=SUM(A1:I9)"); assert!(sheet.get_display_value(9, 0).starts_with("#ERROR!")); } + + /// A self-referential formula inside a large (>64-cell) range reports a + /// cycle instead of silently contributing zero. The small-range path + /// already returned `CycleDetected`; the large-range fast path pushed + /// `0.0`, so a cyclic formula in a big range evaluated as zero. + #[test] + fn large_range_self_reference_reports_a_cycle() { + let mut sheet = SpreadsheetData::default(); + // Insert the formula without recalculating: a normal `set_cell` + // would already mark it #CYCLE! through the topological sort, which + // is exactly the guard that never fires here. + let cell = CellData { + value: "=SUM(A1:I9)".to_string(), + formula: Some("=SUM(A1:I9)".to_string()), + ..Default::default() + }; + sheet.put_cell(0, 0, cell); // A1 = SUM over A1:I9, includes itself + + let mut visiting = HashSet::new(); + assert!(matches!( + sheet.get_cell_value(0, 0, &mut visiting), + Err(FormulaError::CycleDetected) + )); + } }