Skip to content

Commit 8221825

Browse files
fix 8 bugs: search input, multi-byte chars, dependency ordering, and more
- Allow typing 'n'/'p' in search queries by removing them from nav arms - Prevent panics on multi-byte characters by using char-based indexing - Preserve search results after finishing search for n/N navigation - Use topological sort (Kahn's algorithm) for diamond dependency recalc - Pass visited set through circular reference checker to avoid exponential blowup - Allow auto_resize_column to shrink columns, not just grow - Replace non-existent ROW() in help text with valid CONCAT example - Prevent i64 overflow in format_number for values > 9.2e18 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 1732e8d commit 8221825

5 files changed

Lines changed: 187 additions & 74 deletions

File tree

src/application/state.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -599,24 +599,24 @@ impl App {
599599
}
600600

601601
/// Finishes search and returns to normal mode while keeping the current selection.
602+
/// Search results are preserved for n/N navigation in normal mode.
602603
pub fn finish_search(&mut self) {
603604
self.mode = AppMode::Normal;
604-
605+
605606
let num_results = self.search_results.len();
606607
if num_results > 0 {
607608
self.status_message = Some(format!(
608-
"Search completed: {} result{} found for '{}'",
609+
"Search completed: {} result{} found for '{}' (n/N to navigate)",
609610
num_results,
610611
if num_results == 1 { "" } else { "s" },
611612
self.search_query
612613
));
613614
} else {
614615
self.status_message = Some(format!("No results found for '{}'", self.search_query));
615616
}
616-
617+
617618
self.search_query.clear();
618-
self.search_results.clear();
619-
self.search_result_index = 0;
619+
// Don't clear search_results — keep them for n/N navigation
620620
self.cursor_position = 0;
621621
}
622622

src/domain/models.rs

Lines changed: 107 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
//! This module contains the core data structures that represent
44
//! spreadsheet cells and the spreadsheet itself.
55
6-
use std::collections::{HashMap, HashSet};
6+
use std::collections::{HashMap, HashSet, VecDeque};
77
use serde::{Deserialize, Serialize};
88

99
/// Represents the data contained within a single spreadsheet cell.
@@ -233,41 +233,57 @@ impl Spreadsheet {
233233
self.recalculate_dependents(row, col);
234234
}
235235

236-
/// Recalculates all cells that depend on the given cell.
236+
/// Recalculates all cells that depend on the given cell using topological ordering.
237237
fn recalculate_dependents(&mut self, row: usize, col: usize) {
238238
let cell_pos = (row, col);
239-
240-
// Get all cells that depend on this cell
241-
if let Some(dependents) = self.dependents.get(&cell_pos).cloned() {
242-
// Use a breadth-first approach with cycle detection
243-
let mut to_recalc: Vec<_> = dependents.into_iter().collect();
244-
let mut visited = HashSet::new();
245-
let mut in_progress = HashSet::new();
246-
247-
while let Some(dependent) = to_recalc.pop() {
248-
if visited.contains(&dependent) {
249-
continue;
239+
240+
// 1. Collect all transitive dependents
241+
let mut to_recalc = HashSet::new();
242+
let mut queue = VecDeque::new();
243+
if let Some(deps) = self.dependents.get(&cell_pos).cloned() {
244+
for dep in deps {
245+
queue.push_back(dep);
246+
}
247+
}
248+
while let Some(dep) = queue.pop_front() {
249+
if to_recalc.insert(dep) {
250+
if let Some(next) = self.dependents.get(&dep).cloned() {
251+
for n in next {
252+
queue.push_back(n);
253+
}
250254
}
251-
252-
// Check for circular dependency
253-
if in_progress.contains(&dependent) {
254-
// Circular dependency detected - skip this cell
255-
continue;
255+
}
256+
}
257+
258+
if to_recalc.is_empty() {
259+
return;
260+
}
261+
262+
// 2. Compute in-degrees within recalc set
263+
let mut in_degree: HashMap<(usize, usize), usize> = to_recalc.iter().map(|&c| (c, 0)).collect();
264+
for &cell in &to_recalc {
265+
if let Some(deps) = self.dependencies.get(&cell) {
266+
for dep in deps {
267+
if to_recalc.contains(dep) {
268+
*in_degree.entry(cell).or_insert(0) += 1;
269+
}
256270
}
257-
258-
in_progress.insert(dependent);
259-
260-
// Recalculate this dependent cell
261-
self.recalculate_cell(dependent.0, dependent.1);
262-
263-
visited.insert(dependent);
264-
in_progress.remove(&dependent);
265-
266-
// Add its dependents to the queue
267-
if let Some(next_deps) = self.dependents.get(&dependent).cloned() {
268-
for next_dep in next_deps {
269-
if !visited.contains(&next_dep) && !in_progress.contains(&next_dep) {
270-
to_recalc.push(next_dep);
271+
}
272+
}
273+
274+
// 3. Process in topological order (Kahn's algorithm)
275+
let mut ready: VecDeque<_> = in_degree.iter()
276+
.filter(|&(_, d)| *d == 0)
277+
.map(|(&c, _)| c)
278+
.collect();
279+
while let Some(cell) = ready.pop_front() {
280+
self.recalculate_cell(cell.0, cell.1);
281+
if let Some(deps) = self.dependents.get(&cell).cloned() {
282+
for dep in deps {
283+
if let Some(d) = in_degree.get_mut(&dep) {
284+
*d -= 1;
285+
if *d == 0 {
286+
ready.push_back(dep);
271287
}
272288
}
273289
}
@@ -467,21 +483,18 @@ impl Spreadsheet {
467483
///
468484
/// * `col` - Zero-based column index
469485
pub fn auto_resize_column(&mut self, col: usize) {
470-
let current_width = self.get_column_width(col);
471-
let mut max_width = Self::column_label(col).len().max(current_width);
472-
486+
let mut max_width = Self::column_label(col).len();
487+
473488
for row in 0..self.rows {
474489
let cell = self.get_cell(row, col);
475490
let value_width = cell.value.len();
476491
let formula_width = cell.formula.as_ref().map(|f| f.len()).unwrap_or(0);
477492
let content_width = value_width.max(formula_width);
478493
max_width = max_width.max(content_width);
479494
}
480-
495+
481496
max_width = max_width.max(3).min(50);
482-
if max_width > current_width {
483-
self.set_column_width(col, max_width);
484-
}
497+
self.set_column_width(col, max_width);
485498
}
486499

487500
/// Automatically resizes all columns to fit their content.
@@ -1044,4 +1057,59 @@ mod tests {
10441057
assert_eq!(loaded.get_cell(0, 1).value, "10"); // B1 = 5*2 = 10
10451058
assert_eq!(loaded.get_cell(0, 2).value, "20"); // C1 = 10*2 = 20
10461059
}
1060+
1061+
#[test]
1062+
fn test_diamond_dependency_recalculation() {
1063+
let mut sheet = Spreadsheet::default();
1064+
1065+
// Diamond pattern: A1 -> B1, A1 -> C1, B1 -> C1
1066+
// A1 = 10
1067+
sheet.set_cell(0, 0, CellData { value: "10".to_string(), formula: None });
1068+
// B1 = A1 * 2
1069+
sheet.set_cell(0, 1, CellData {
1070+
value: "20".to_string(),
1071+
formula: Some("=A1*2".to_string()),
1072+
});
1073+
// C1 = A1 + B1 (depends on both A1 and B1)
1074+
sheet.set_cell(0, 2, CellData {
1075+
value: "30".to_string(),
1076+
formula: Some("=A1+B1".to_string()),
1077+
});
1078+
1079+
// Verify initial state
1080+
assert_eq!(sheet.get_cell(0, 0).value, "10");
1081+
assert_eq!(sheet.get_cell(0, 1).value, "20");
1082+
assert_eq!(sheet.get_cell(0, 2).value, "30"); // 10 + 20
1083+
1084+
// Change A1 — B1 must update before C1 for correct result
1085+
sheet.set_cell(0, 0, CellData { value: "5".to_string(), formula: None });
1086+
assert_eq!(sheet.get_cell(0, 1).value, "10"); // 5*2 = 10
1087+
assert_eq!(sheet.get_cell(0, 2).value, "15"); // 5 + 10 = 15 (not 5 + 20 = 25)
1088+
}
1089+
1090+
#[test]
1091+
fn test_auto_resize_column_shrinks() {
1092+
let mut sheet = Spreadsheet::default();
1093+
1094+
// Add wide content and auto-resize
1095+
sheet.set_cell(0, 0, CellData {
1096+
value: "This is very wide content".to_string(),
1097+
formula: None,
1098+
});
1099+
sheet.auto_resize_column(0);
1100+
let wide_width = sheet.get_column_width(0);
1101+
assert!(wide_width >= "This is very wide content".len());
1102+
1103+
// Replace with short content
1104+
sheet.set_cell(0, 0, CellData {
1105+
value: "Hi".to_string(),
1106+
formula: None,
1107+
});
1108+
sheet.auto_resize_column(0);
1109+
let narrow_width = sheet.get_column_width(0);
1110+
1111+
// Column should have shrunk
1112+
assert!(narrow_width < wide_width);
1113+
assert!(narrow_width >= 3); // minimum width
1114+
}
10471115
}

src/domain/services.rs

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,21 @@ impl<'a> FormulaEvaluator<'a> {
161161
evaluator.evaluate(&ast)
162162
}
163163

164+
/// Checks for circular references in a formula string, reusing the visited set.
165+
fn check_circular_in_formula(&self, formula: &str, target_cell: (usize, usize), visited: &mut HashSet<(usize, usize)>) -> bool {
166+
if !formula.starts_with('=') {
167+
return false;
168+
}
169+
let expr = &formula[1..];
170+
match Parser::new(expr) {
171+
Ok(mut parser) => match parser.parse() {
172+
Ok(ast) => self.check_circular_reference_in_ast(&ast, target_cell, visited),
173+
Err(_) => false,
174+
},
175+
Err(_) => false,
176+
}
177+
}
178+
164179
/// Checks for circular references in an AST.
165180
fn check_circular_reference_in_ast(&self, expr: &Expr, target_cell: (usize, usize), visited: &mut HashSet<(usize, usize)>) -> bool {
166181
match expr {
@@ -170,21 +185,19 @@ impl<'a> FormulaEvaluator<'a> {
170185
if (row, col) == target_cell {
171186
return true;
172187
}
173-
188+
174189
if visited.contains(&(row, col)) {
175190
return false;
176191
}
177-
192+
178193
visited.insert((row, col));
179-
194+
180195
let cell = self.spreadsheet.get_cell(row, col);
181196
if let Some(ref cell_formula) = cell.formula {
182-
if self.would_create_circular_reference(cell_formula, target_cell) {
197+
if self.check_circular_in_formula(cell_formula, target_cell, visited) {
183198
return true;
184199
}
185200
}
186-
187-
visited.remove(&(row, col));
188201
}
189202
false
190203
}
@@ -699,7 +712,11 @@ impl AutofillPattern {
699712
/// Format a number smartly: show as integer if whole, otherwise as decimal.
700713
fn format_number(n: f64) -> String {
701714
if n.fract().abs() < 1e-9 {
702-
format!("{}", n as i64)
715+
if n.abs() < (i64::MAX as f64) {
716+
format!("{}", n as i64)
717+
} else {
718+
format!("{:.0}", n)
719+
}
703720
} else {
704721
// Remove trailing zeros
705722
let s = format!("{}", n);
@@ -2104,4 +2121,21 @@ Break""#).expect("Failed to write to temp file");
21042121
assert_eq!(pattern.generate(4), "Sun");
21052122
assert_eq!(pattern.generate(5), "Mon");
21062123
}
2124+
2125+
#[test]
2126+
fn test_format_number_large_values() {
2127+
// Values within i64 range should format as integers
2128+
assert_eq!(AutofillPattern::format_number(1000.0), "1000");
2129+
assert_eq!(AutofillPattern::format_number(-1000.0), "-1000");
2130+
2131+
// Values beyond i64 range should not panic and should format correctly
2132+
let result = AutofillPattern::format_number(1e19);
2133+
assert!(!result.is_empty());
2134+
// Should not produce incorrect i64-saturated value
2135+
assert!(result.starts_with("1000000000000000000"));
2136+
2137+
let result = AutofillPattern::format_number(1e20);
2138+
assert!(!result.is_empty());
2139+
assert!(result.starts_with("1000000000000000000"));
2140+
}
21072141
}

0 commit comments

Comments
 (0)