134 lines
No EOL
9.1 KiB
Markdown
134 lines
No EOL
9.1 KiB
Markdown
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**
|
||
```rust
|
||
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<SpreadsheetData> 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 `Command`s.
|
||
- `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. |