nigig-org/REVIEWS/SPREADSHEET REVIEW.md

Here is the unvarnished truth about this codebase, followed by a surgical execution plan to fix it.

---

Brutal Assessment

1. The Formula Engine Is Fundamentally Defective

You do not have a formula parser. You have a tokenizer duct-taped to an evaluator that mutates state while parsing. The skip_expression hack for IF lazy evaluation is a confession of architectural bankruptcy. Every formula re-tokenizes and re-parses on every single recalculation. There is no AST, which means:

• No formula introspection (you can't build a formula bar with syntax highlighting)
• No optimization (common subexpressions evaluate repeatedly)
• No proper error recovery (a typo in cell ZZ9999 poisons the whole sheet)
• The "lazy IF" is a token-skipping kludge that will break on nested functions

Verdict: The core engine is a toy, not a product.

2. The UI Clones Entire Datasets for Fun

SpreadsheetWorkspace::set_active_sheet does grid.data = self.sheets[idx].data.clone() and sheet.data = grid.data.clone(). Switching tabs is O(cells). For a 100k-cell sheet, you are copying megabytes of HashMap data every time the user clicks a tab. This is not a spreadsheet; it is a memory allocator stress test.

3. The Render Loop Allocates on Every Frame

draw_cells_in_rect builds Vec<f64> for column and row offsets every frame. draw_edit_overlay clones EditState (including its String) every frame to appease the borrow checker. The draw loop is supposed to be a read-only phase, yet it performs heap allocations and string clones at 60fps. This is amateurish.

4. God Objects and Zero Separation of Concerns

SpreadsheetGrid is 50+ fields of pure chaos. It handles:

• Rendering
• Touch/mouse/keyboard input
• Hit testing
• Scrolling
• In-cell editing
• Copy/paste
• Autofill
• Border styling
• Undo/redo delegation
• Pinch-to-zoom

This is not a widget; it is an application masquerading as a struct. The "engine/UI split" is a lie—the UI directly mutates SpreadsheetData internals and even rebuilds dependency graphs.

5. Stringly-Typed Everything

Result<Value, String> is used throughout. Errors are concatenated strings like "#DIV/0!" instead of structured enums. This makes it impossible to localize, impossible to programmatically handle, and guarantees that error messages will be inconsistent.

6. The Serialization Format Is a Liability

A custom TSV format with manual \n/\\ escaping? In 2026? This will corrupt user data the moment someone pastes a Windows path or a regex. There is no schema versioning, no migration path, and the parser silently ignores unknown tags instead of failing loudly.

7. Performance Landmines Everywhere

• col_at_x and row_at_y do linear scans from scroll offset on every hit test.
• cell_abs_rect sums column widths from first_col on every call (O(N) per cell lookup).
• call_fn allocates String for uppercase function names on every function invocation.
• parse_cell_ref allocates a String via to_uppercase() just to parse a reference.
• The autofill fallback branch rebuilds cell-value vectors and re-runs detect_series repeatedly (despite a memoization comment that admits the original was O(N²)).

8. The Code Apologizes Instead of Fixing

Comments like "The only way to fully avoid the clone is to refactor..." or "For extremely deep dependency chains... this could stack-overflow" are admissions of defeat. Recursive DFS for topological sort on a data structure that can hold 36,000 cells? That is not a theoretical concern; that is a stack overflow waiting to happen on user data.

9. No Reactive Data Flow

The workspace manually syncs grid data back to the sheet vector before saving. There is no single source of truth. The grid owns a copy, the workspace owns a vector of copies, and they are kept in sync via manual cloning. This is begging for desynchronization bugs.

10. Touch Handling Is Spaghetti

Touch, mouse, and keyboard events are interleaved in a 400-line handle_event method with mutable state scattered across drag_state, last_touch_pos, active_touches, pinch_last_dist, and pinch_last_mid. Pinch-to-zoom mutates global col_width and row_height directly, resizing every cell in the sheet instead of using a view transform.

---

Execution Plan: From Toy to Product

Phase 1: Kill the Parser and Rebuild the Engine (Weeks 1–3)

Goal: A real formula engine with an AST.

1. Define a proper AST

   enum Expr { Number(f64), String(String), Ref(CellRef), Range(Range), 
              Binary(Op, Box<Expr>, Box<Expr>), Unary(Op, Box<Expr>),
              Call(String, Vec<Expr>) }
   

2. Build a two-phase parser: Tokenizer → AST. The evaluator walks the AST.
3. Structured errors: Replace Result<..., String> with Result<..., FormulaError> where FormulaError is an enum (DivByZero, CycleDetected, InvalidRef, etc.).
4. Lazy evaluation becomes trivial: Walk the AST; for Call("IF", args), evaluate only the taken branch.
5. Reference extraction: Walk the AST to build dependencies instead of re-tokenizing.
6. CellId everywhere: Replace HashMap<(u32, u32), _> with HashMap<CellId, _>. The packing into u64 was already done; use it.

Phase 2: Fix the Data Architecture (Weeks 4–5)

Goal: Single source of truth, zero cloning on tab switch.

1. Arc or Rc<RefCell<_>>: The grid gets a reference. The workspace owns the Vec<Sheet>. When switching tabs, you swap an Arc pointer, not clone a HashMap.
2. Move mutation authority out of the grid: The grid should emit intents (SetCell, DeleteRange, Paste). A central Workbook actor applies them, updates the data, and notifies the grid to redraw.
3. Remove SpreadsheetGrid::data: The grid renders; it does not own. This breaks the god object.

Phase 3: Zero-Allocation Rendering (Weeks 6–7)

Goal: The draw loop must not allocate.

1. Precomputed layout cache: Store Vec<f64> column offsets and Vec<f64> row offsets in the grid, recomputed only when column widths or row heights change (not every frame).
2. Eliminate EditState clone: Restructure draw_edit_overlay to compute the rect first, then borrow self.edit_state immutably for the text draw.
3. Text measurement: Use the actual font metrics from makepad instead of display.len() * 7.0. That heuristic breaks on bold fonts, variable-width fonts, or any non-ASCII text.
4. Dirty-region rendering: Only redraw cells that changed, not the entire visible grid.

Phase 4: Performance and Scale (Weeks 8–9)

Goal: Handle 100k+ cells without choking.

1. Iterative topological sort: Replace recursive DFS with an explicit stack. Eliminate the stack overflow risk.
2. Parallel recalculation: Use rayon to evaluate independent formula branches in parallel.
3. Formula result caching: Cache parsed ASTs per cell instead of re-parsing on every recalc.
4. Sparse matrix optimization: For large ranges in SUM(A1:A10000), don't iterate 10k cells if most are empty. Use a spatial index or skip empty ranges.
5. Virtual scrolling: Only draw cells in the viewport. You already compute first_row/last_row—actually enforce it and stop iterating over frozen panes redundantly.

Phase 5: Serialization and Persistence (Week 10)

Goal: Robust, versioned, standard format.

1. Replace custom TSV with JSON or MessagePack. Use serde with schema versioning.
2. Migration strategy: struct WorkbookV1, struct WorkbookV2. Deserialize into tagged union, migrate up.
3. Autosave: Debounced save to disk using the standard format, not ad-hoc string concatenation.

Phase 6: UI Architecture Cleanup (Weeks 11–12)

Goal: The grid is a view, not a controller.

1. Extract controllers:
   • InputController: Handles all keyboard/touch, emits Commands.
   • SelectionController: Manages anchor/head/range logic.
   • EditController: Manages formula bar and in-cell editing.
   • ClipboardController: Copy/paste/autofill logic.
2. Command pattern: All mutations go through Command enums that are undo-aware. The grid never calls data.snapshot() directly.
3. Proper hit testing: Build a spatial index (interval tree or simple sorted Vec) for column/row positions so col_at_x is O(log N), not O(N).

Phase 7: Hardening (Week 13)

Goal: It doesn't crash on user data.

1. Fuzz the formula parser: Generate random formula strings, ensure it doesn't panic.
2. Cycle torture tests: 10,000-cell dependency chains, circular references, self-referential ranges.
3. Memory profiling: Ensure tab switching and undo don't leak or allocate excessively.
4. Remove all unwrap and expect in the engine. Every parse must fail gracefully.

---

Summary

Right now, this is a demo that accidentally grew tests. The formula engine cannot scale, the UI clones data like it's free, and the architecture is a monolith with file separators.

If you follow this plan, by week 13 you will have a spreadsheet engine with a real AST, a reactive data layer, a zero-allocation renderer, and a UI that doesn't asphyxiate under its own state. If you don't, you have a very well-commented toy that will collapse the moment a user opens a 50-sheet workbook.