improvise/context/design-principles.md

# Improvise Design Principles

## 1. Functional-First Architecture

### Commands Are Pure, Effects Are Side-Effectful

Every user action flows through a two-phase pipeline:

1. **Command** (`Cmd` trait) — reads immutable context, returns a list of effects.
   The `CmdContext` is a read-only snapshot: model, layout, mode, cursor position.
   Commands never touch `&mut App`. All decision logic is pure.

2. **Effect** (`Effect` trait) — a small struct with an `apply(&self, app: &mut App)` method.
   Each effect is one discrete, debuggable state change. The app applies them in order.

This separation means:
- Commands are testable without a terminal or an `App` instance.
- Effects can be logged, replayed, or composed.
- The only place `App` is mutated is inside `Effect::apply`.

### Prefer Transformations to Mutation

Where possible, build new values rather than mutating in place:
- `CellKey::with(cat, item)` returns a new key with an added/replaced coordinate.
- `CellKey::without(cat)` returns a new key with a coordinate removed.
- Viewport positioning is computed as a pure function (`viewport_effects`) that
  returns a `Vec<Effect>`, not a method that pokes at scroll offsets directly.

### Compose Small Pieces

Commands compose via `Binding::Sequence` — a keymap entry can chain multiple
commands, each contributing effects independently. The `o` key (add row + begin
editing) is two commands composed at the binding level, not a monolithic handler.

---

## 2. Polymorphism Over Conditionals

### Dispatch Through Traits and Registries, Not Match Blocks

- **Commands**: 40+ types each implement `Cmd`. A `CmdRegistry` maps names to
  constructor closures. Dispatching a key presses looks up the binding, resolves
  the command name through the registry, and calls `execute`. No central
  `match command_name { ... }` block.

- **Effects**: 50+ types each implement `Effect`. Collected into a `Vec<Box<dyn Effect>>`
  and applied in order. No `match effect_kind { ... }`.

- **Keymaps**: Each mode has its own `Keymap` (a `HashMap<KeyPattern, Binding>`).
  Mode dispatch is one table lookup, not a nested `match (mode, key)`.

### Use Enums to Make Invalid States Unrepresentable

- `BinOp` is an enum (`Add | Sub | Mul | ...`), not a string. Invalid operators
  are caught at parse time, not silently ignored at eval time.

- `Axis` is `Row | Column | Page | None`. A category is on exactly one axis.
  Cycling is a four-state rotation — no boolean flags, no "row_or_column" ambiguity.

- `Binding` is `Cmd | Prefix | Sequence`. The keymap lookup returns one of these
  three shapes; dispatch pattern-matches exhaustively.

- `CategoryKind` is `Regular | VirtualIndex | VirtualDim | Label`. Business rules
  (e.g., the 12-category limit counts only `Regular`) are enforced by matching
  on the enum, not by checking name prefixes.

### When You Add a Variant, the Compiler Finds Every Call Site

Prefer exhaustive `match` over `if let` or `_ =>` wildcards. When a new `Axis`
variant or `AppMode` is added, non-exhaustive matches produce compile errors
that guide you to every place that needs updating.

---

## 3. Correctness by Construction

### Canonical Forms Prevent Equivalence Bugs

`CellKey::new()` sorts coordinates by category name. Two keys that name the same
intersection but in different order are identical after construction. Equality,
hashing, and storage all work correctly without callers needing to remember to
sort. Property tests verify this invariant.

### Smart Constructors Enforce Invariants

- `CellKey::new()` is the only way to build a key — it always sorts.
- `Category::add_item()` deduplicates by name and auto-assigns IDs via a private
  counter. External code cannot fabricate an `ItemId`.
- `Model::add_category()` checks the 12-category limit before insertion.
- `Formula::new()` takes all required fields; there is no default/empty formula
  to accidentally leave half-initialized.

### Type-Safe Identifiers

`CategoryId` and `ItemId` are typed aliases. While they are `usize` underneath,
using named types signals intent and prevents accidentally passing an item count
where an item ID is expected.

### Symbol Interning for Data Integrity

`DataStore` interns category and item names into `Symbol` values (small copyable
handles). This means:
- String comparison is integer comparison — fast and allocation-free.
- A secondary index maps `(Symbol, Symbol)` pairs to cell sets, enabling O(1)
  lookups for aggregation queries.
- Symbols can only be created through the `SymbolTable`, so misspelled names
  produce a distinct symbol rather than silently matching a wrong cell.

### Parse-Time Validation

Formulas are parsed into a typed AST (`Expr` enum) at entry time. If the syntax
is invalid, the user gets an error immediately. The evaluator only sees
well-formed trees — it does not need to handle malformed input.

---

## 4. Separation of Concerns

### Four Layers

| Layer | Directory | Responsibility |
|-------|-----------|----------------|
| **Model** | `src/model/` | Categories, items, groups, cell data, formulas. Pure data, no rendering. |
| **View** | `src/view/` | Axis assignments, page selection, hidden items, layout computation. Derived from model. |
| **Command / Effect** | `src/command/`, `src/ui/effect.rs` | Intent (commands) and state mutation (effects). Bridges user input to model changes. |
| **Rendering** | `src/draw.rs`, `src/ui/` | Terminal drawing. Reads model + view, writes pixels. No mutation. |

### Formulas Are Data, Not Code

A formula is a serializable struct: raw text, target name, category, AST, optional
filter. It is stored in the model alongside regular data. The evaluator walks the
AST at read time. Formulas never become closures or runtime-generated code.

### Display Rounding Is View-Only

Number formatting (`format_f64`) rounds for display. Formula evaluation always
operates on full `f64` precision. The rounding function is only called in
rendering paths — never in `eval_formula` or aggregation.

### Drill State Isolates Edits

When editing aggregated (drill-down) cells, a `DrillState` snapshot freezes the
current cell set. Pending edits accumulate in a staging map. On commit,
`ApplyAndClearDrill` writes them all atomically. On cancel, the snapshot is
discarded. No partial writes reach the model.

---

## 5. Guidelines for New Code

- **Add a command, not a special case.** If you need new behavior on a keypress,
  implement `Cmd`, register it, and bind it in the keymap. Do not add an
  `if key == 'x'` branch inside `handle_key`.

- **Return effects, do not mutate.** Your command's `execute` receives `&CmdContext`
  (immutable). Produce a `Vec<Box<dyn Effect>>`. If you need a new kind of state
  change, create a new `Effect` struct.

- **Use the type system.** If a value can only be one of N things, make it an enum.
  If an invariant must hold, enforce it in the constructor. If a field is
  optional, use `Option` — do not use sentinel values.

- **Test the logic, not the wiring.** Commands are pure functions of context;
  test them by building a `CmdContext` and asserting on the returned effects.
  You do not need a terminal.

- **Keep `Option`/`Result`/`Box` at the boundaries.** Core logic should work with
  concrete values. Wrap in `Option` at the edges (parsing, lookup, I/O) and
  unwrap early. Do not thread `Option` through deep call chains.

---

## 6. Testing

### Coverage and ambition

Aim for **~80% line and branch coverage** on logic code. This is a quality floor —
go higher where the code is tricky or load-bearing, but don't pad coverage by
testing trivial getters or chasing 100% on rendering widgets. The test suite
should remain fast (under 2 seconds). Slow tests erode the habit of running them.

### Demonstrate bugs before fixing them

Write a test that **fails on the current code** before writing the fix. Prefer a
small unit test targeting the broken function over an end-to-end test. After the
fix, the test stays as a regression guard. Document the bug in the test's
doc-comment (see `model/types.rs` → `formula_tests` for examples).

### Use property tests judiciously

Property tests (`proptest`) are for **invariants that must hold across all
inputs** — not a replacement for example-based tests. Good candidates:

- Structural invariants: CellKey is always sorted, each category lives on exactly
  one axis, toggle-collapse is involutive, hide/show roundtrips.
- Serialization roundtrips: save/load identity.
- Determinism: `evaluate` returns the same result for the same inputs.

Keep case counts at the default (256). Don't crank them to thousands — if a
property needs more cases to feel safe, constrain the input space with better
strategies rather than brute-forcing. Property tests that take hundreds of
milliseconds each are a sign something is wrong.

### What to test

- **Model, formula, view**: the core logic. Unit tests for each operation and
  edge case. Property tests for invariants. These are the highest-value tests.
- **Commands**: build a `CmdContext`, call `execute`, assert on the returned
  effects. Pure functions — no terminal needed.
- **Persistence**: round-trip tests (`save → load → save` produces identical
  output). Cover groups, formulas, views, hidden items, legacy JSON.
- **Format**: boundary cases for comma placement, rounding, negative numbers.
- **Import**: field classification heuristics, CSV quoting, multi-file merge.

### What not to test

- Ratatui `Widget::render` implementations — pure drawing code that changes
  often. Test the data they consume (layout, cat_tree, format) instead.
- Trivial data definitions (`ast.rs`, `axis.rs`).
- Module re-export files.

### Test the property, not the implementation

A test like "calling `set_axis(cat, Row)` sets the internal map entry to `Row`"
is brittle — it mirrors the implementation and breaks if the storage changes.
Instead test the observable contract: "after `set_axis(cat, Row)`,
`axis_of(cat)` returns `Row` and `categories_on(Row)` includes `cat`." This
style survives refactoring and catches real bugs.