# 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`, 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. ### Decompose rather than early return Early Returns usually are a signal of mixed responsibilities: if an early return would clarify a function, consider how the function could be decomposed for the same effect without the early return. --- ## 2. Polymorphism Over Conditionals ### Dispatch Through Traits and Registries, Not Match Blocks - **Commands**: 40+ types each implement `Cmd`, organized by concern across submodules in `command/cmd/` (navigation, cell, commit, grid, mode, panel, search, text_buffer, tile, effect_cmds). A `CmdRegistry` maps names to constructor closures. Dispatching a key press 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>` and applied in order. No `match effect_kind { ... }`. - **Keymaps**: Each mode has its own `Keymap` (a `HashMap`). 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 | VirtualMeasure | Label`. Business rules (e.g., the 12-category limit counts only `Regular`) are enforced by matching on the enum, not by checking name prefixes. Virtual categories (`_Index`, `_Dim`, `_Measure`) always exist: `_Index` and `_Dim` support drill-down/records mode; `_Measure` holds numeric data fields and formula targets (added automatically by `add_formula`). Use `Model::regular_category_names()` when selecting a default category for prompts or other user-visible choices. ### 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. ### Grammar-Defined File Format The `.improv` file format is defined by a PEG grammar (`persistence/improv.pest`) and parsed by pest. The grammar is the single source of truth — the parser is a tree-walker over the grammar's parse tree, not an ad-hoc line scanner. This means: - Adding a new format feature means updating the grammar first, then the walker. - The grammar can be read as a specification independent of the Rust code. - A grammar-walking test generator reads the grammar AST at test time (via `pest_meta`) and produces random valid files, ensuring the parser accepts everything the grammar describes. ### CL-Style Pipe Quoting for Names Names in the `.improv` format use CL-style `|...|` pipe quoting. A name is bare if it matches `[A-Za-z_][A-Za-z0-9_-]*`; everything else must be pipe-quoted. Escapes inside pipes: `\|` (literal pipe), `\\` (backslash), `\n` (newline). This convention is shared between the `.improv` persistence format and the formula parser's identifier syntax. ### Formula Identifiers and Quoting **Bare identifiers** are `(ASCII_ALPHA | "_") ~ (ASCII_ALPHANUMERIC | "_" | "-")*` — no spaces, the same shape `improv.pest`'s `bare_name` has. A multi-word name such as `Total Revenue` must be pipe-quoted. **Keywords need a word boundary after them, not before.** `IS`, `WHERE` and the aggregate names are spelled `^"…"` in `formula.pest`, and `is_expr`/`where_clause`/ `inline_where` are compound-atomic with `sp+` so that `Region ISLAND` and `SUM(Rev WHEREAS x = y)` are parse errors rather than silently-wrong filters. A `SUM`/`IF` without parens falls through to `ref_expr`, which is why a bare keyword still names an item; the writer quotes it anyway (`quote::quote_name`). **Pipe-quoted identifiers** (`|...|`) allow any characters — including spaces, keywords, and operators — inside the delimiters. Use pipes when a category or item name collides with a keyword or contains special characters: ``` |WHERE| — category named "WHERE" |Revenue (USD)| — name with parens |Cost + Tax| — name with operator chars SUM(|Net Revenue| WHERE |Region Name| = |East Coast|) ``` `identifier` is `pipe_quoted | bare_ident`, so a pipe-quoted name works everywhere a bare one does: expressions, aggregate arguments, and a `WHERE` clause's category name. A double-quoted string is a `filter_value` only — the right-hand side of a `WHERE` or an `IS`, never an expression. --- ## 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. ### Formula Evaluation Is Fixed-Point `recompute_formulas(none_cats)` iterates formula evaluation until values stabilize. Each pass evaluates all formula cells using the current cache (for formula-derived values) and raw data aggregation (for data values). This avoids recursive evaluation through `evaluate_aggregated` and naturally handles chained formulas (`Margin = Profit / Revenue` where `Profit = Revenue - Cost`). Circular references converge to `CellValue::Error("circular")` after `MAX_EVAL_DEPTH` iterations. ### 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. A Front End Displays State; It Does Not Reimplement Behaviour **The goal is to minimise the amount of new code a new UI framework needs.** Logic is centralised in a redux-like store, so a second or third front end is mostly a function from state to pixels plus a wire for events — not a second implementation of how the application behaves. The pieces already exist and are worth naming as what they are: | redux | here | |---|---| | action | `Command` / `ModelCommand` (`improvise-protocol`) | | reducer | `reduce_view`, and `WidgetCache::apply` per widget | | store | `ViewState` + the per-widget caches | | dispatch | `Sessions::apply_checked` — the one door (gotcha 36) | So the question to ask of any code in a front end is: **would a third front end have to write this again?** If yes, it is behaviour and it belongs in the shared layer. Three things make this concrete rather than aspirational: **The measure is lines of framework-specific code, and it should trend to "paint and wire".** `web/app.js` is the current benchmark: a few hundred lines that know no protocol, reaching the application through a handful of string calls. A front end that needs to understand `CellKey` coordinates, or which category a row label belongs to, or what a keypress means, is one that has been handed a job the store should have done. **A derivation done twice is the defect, even when both copies are correct today.** Two front ends deriving the same fact will diverge, and the divergence is invisible until someone compares them side by side. `improvise-edp` is the worked example — the category tree was flattened twice, two different ways, and the fix put `viewmodel::flatten_cat_tree` where both ends call it. Gotcha 44 was the same defect and is now closed the same way (improvise-anr9): the grid's *frame* — header levels, label tuples, group runs and their collapse state — is `improvise_protocol::AxisFrame`, and both `src/ui/grid.rs` and `improvise-wasm-client/src/render.rs` ask it rather than deriving one. Note what the second cure cost that the first did not: a `Vec` of labels had to change **type**, and a shared derivation could not be added beside the old one — `row_levels` next to `row_labels` would have been two answers to "what are the labels", which is the disease and not the cure. **Sharing the table beats testing two copies of it.** Where a front end must turn input into actions, share the mapping rather than mirroring it: two hand-kept copies held in step by a test are one definition waiting to be made (`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)`). That is why the keymap is shared rather than reimplemented per front end, and why `doctrine(mirror_the_cmd_line_for_line)` is a rule about *resolvers* — the narrow, unavoidable residue — and never a licence to restate a table. **What legitimately stays per-framework.** Painting, event wiring, and the mapping from a semantic value to a framework type: `AxisColor → ratatui::Color` in `draw.rs::axis_fg`, `MinibufColor → minibuf_fg`. That mapping is the shape to imitate — the *decision* is in the shared layer as a semantic enum, and only the last translation is local. Anything larger than a lookup table is a candidate for the store. --- ## 6. 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>`. 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. - **Reasoning lives in `context/`, not in comments.** A comment run is **at most two consecutive lines**, `///` and `//!` included, enforced by `scripts/check-comment-runs.py`. Where a reader would otherwise re-derive a rule wrongly, name the `context/` file that carries the argument — and only there, because a pointer on every function is noise. The prose lives in `context/code/.md`; the rule exists because the arguments in this codebase are its most expensive artefact and a comment is the one place nothing checks them against the code (user, 2026-09-20; improvise-9c8c). A ``` fence in a doc comment is a doctest — a running test, not prose — and is exempt. - **And an argument must not live only in a commit message.** A message is write-once and unauditable: it cannot be corrected without rewriting merged history, and no check in this repo can read one — the ID audit greps `context`, `README.md`, `AGENTS.md`, `CLAUDE.md`, `crates`, `src`, `tests`, `examples`, `web`, `harness` and `docs`, and a commit message is in none of them. So a reason recorded only there is a reason nothing can ever find wrong. When one turns out to be wrong, correct `context/` and say in `context/` that the message is superseded; that is the fix rather than a compromise. PR #51's `7d25762` is the worked example, corrected by #56. --- ## 7. 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, and the rule is **two numbers** because the ambiguity between them is what let the old single "under 2 seconds" drift: **no single test binary may exceed 2 seconds**, and the workspace wall clock is the sum of them. Excludes compilation, and excludes `harness/`, which is Playwright and takes minutes in its own CI job. Slow tests erode the habit of running them. **The threshold is a tripwire, and the seconds are not portable.** The same commit measured `improvise-io`'s lib binary at 1.4s on one machine and 3.7–5.2s on another (improvise-8ec3), so a PR body citing "longest single binary 1.47s" is checkable and *false* for the reader who checks it, which is worse than no claim at all. Three consequences: - **Re-measure; never re-assert.** `cargo test --workspace` prints a `finished in` per binary. Read your own run. - **If you cite a figure, name the machine you took it on.** A number with no machine is a property of the suite, which it is not. - **The portable signal is the ratio, not the seconds.** One binary an order of magnitude above the run's **median** is the thing to look at — not above second place, which it need not be: on one machine `improvise-io` is 2.2× the runner-up and ~15× the median. It is the same binary on all three machines it has been measured on: `improvise-io`'s lib, which is proptest-dominated by the grammar-walking `.improv` generator and the round-trip properties over it. Constraining that generator's strategy is the open work (improvise-8shu); lowering the case count below 256 is not (see below). ### 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. **Record the bug in `context/test-evidence/`, keyed by test name** — what broke, what fault was injected, and why the test can fail. It does not go in the test's doc-comment: comment runs are capped at two lines (see below), and the evidence is longer than that in every case worth having. The cost of the split is that a renamed test orphans its entry. `scripts/check-comment-runs.py --check-evidence` catches that, in two directions. Every `###`/`####` heading shaped like a test name must name a test that **runs** — the names are diffed against `cargo test --workspace -- --list` minus its `--ignored` twin, never scraped out of the source, because text that looks like a test and a test are indistinguishable to a scanner (improvise-rm4s). And a test whose doc comment *cites* `context/test-evidence/` must have an entry there, or the pointer reads as a promise the explanation exists. Neither direction is "every test has evidence", deliberately: most tests have none and should not be forced to have one. The reasoning and the residual holes are in `context/code/tooling.md`. ### A check that looks like the answer, answering a neighbouring question The characteristic defect in this repo is not a wrong line; it is a check that is *adjacent* to the thing it names. Three instances landed in one campaign (PRs #59/#60), in three different layers, which is why it is worth stating as a shape rather than three gotchas: - **A vacuous assertion.** `set_panel_open_and_cursor` asserted `category_panel_open` was true after an effect set it true — once the launch default supplied true, the effect could be replaced by a **no-op** and 1391 tests passed. It answered *is the flag true* where it meant *did the effect set it*. - **A vacuous repair of it.** The first fix folded `[open:false, open:true]` and asserted the final state — which a reducer that folds nothing also satisfies. It answered *does the text mention the other direction* where it meant *does the writer move the flag both ways*. **Reading the diff is what failed**, and it failed because the vacuous version contained `open: false`. - **A clean auto-merge.** `git merge-tree` reporting no conflict answers *can git combine these texts*, where the question is *does the combination do what both sides meant*. Two PRs edited the same four files with no conflict and neither side CI-tested together. Each has the same remedy shape: **name the question you actually mean, then find an input that separates it from its neighbour.** - For an assertion: **assert the direction the default cannot supply.** Closing the panel discriminates; opening it proves nothing. - For a repair: inject a **one-directional** fault (`if open { set true }` — can raise, never lower). A vacuous repair cannot see it, because a test that only asserts the true direction is satisfied by a writer that only ever writes true. - For a merge: **materialise it and run the gates**, and check that a count which should MOVE actually moved. Playwright going 16 → 17 is what showed the other side survived; "no conflict" and an unchanged total would both have been consistent with losing it. This generalises `doctrine(fold_tests_must_test_order_not_the_axiom)` — asserting `fold(prefix)+fold(rest) == fold(whole)` is the definition of a left fold, so it answers a neighbouring question too. What the campaign adds is that the pattern recurs in **test repairs** and in **merge hygiene**, not only in production code and not only in fold tests. ### E2E pins the boundaries; unit tests work between them **E2E and integration tests are rare and pin *boundary contracts*. Unit tests *assume* those contracts and test between the boundaries** (user, 2026-09-20). So the question to ask of a slow test is never "what feature does this cover" — it is **"which boundary is this, and what is the contract at it?"** The answer changes what you write. Four browser widgets had never worked — the tile bar, the view panel, the formula panel, and the category sidebar ignoring `category_panel_open` — and all four were **one** boundary failure: a `RenderModel` field with no element painting it. Four per-widget Playwright specs cost four slow tests and would still have missed the fifth; a single contract assertion that *every field the render model exposes is painted* catches all four and the next one. `harness/tests/opfs.spec.js` is the cleanest instance of the right shape already in the tree: it runs the **storage contract**, not a feature. Three corollaries, in the order they bite: - **A unit test may assume the contract rather than re-establishing it.** That assumption is what makes it cheap, and it is only sound because something slow pins the contract — so deleting the contract test silently invalidates every unit test that leaned on it. - **Never weaken a contract to make it cheap.** If the honest contract needs a real browser, a real `Worker` or real OPFS, it stays slow. Moving it to a faster harness that cannot see the thing it names is how a suite goes green over a broken product. - **Each slow test must say which contract it pins.** The difference between a contract test and a feature test is invisible from the code, so it has to be written down or the next person adds the fifth per-widget spec. ### 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. Tests are colocated in each command submodule (`command/cmd/.rs` → `mod tests`), with shared test helpers in `command/cmd/mod.rs::test_helpers`. - **Persistence**: round-trip tests (`save → load → save` produces identical output) plus grammar-driven property tests. The generator walks the pest grammar AST to produce random valid files; proptests verify `parse(generate())` succeeds and `parse(format(parse(generate())))` is stable. Cover groups, formulas, views, hidden items, pipe quoting edges. - **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.