Files
improvise/context/code/core.md
T
Claude (agent-hier) e09ec75226 docs: group hierarchies and fold lines, with the evidence for each rival (improvise-felo.6)
The harness fixtures open their groups explicitly now that groups start closed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
2026-09-24 21:54:23 -07:00

1686 lines
99 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# `improvise-core` — the arguments
Written for a reader with the code open. This is where the reasoning that used
to live in `crates/improvise-core/**`'s comments now lives; the comments
themselves are capped at two lines (design-principles §6). Bug narratives keyed
by test name are in `context/test-evidence/core.md`.
The crate is pure data: `Model`, `View`, `GridLayout`, `Workbook`, number
formatting. It depends on `improvise-formula` for AST types and knows nothing
about UI, IO or commands.
---
## 1. `CellKey` is canonical by construction
`CellKey(Vec<(category, item)>)` is **always sorted by category name**, and the
constructors are the only way to get one: `CellKey::new(coords)` sorts,
`with(cat, item)` replaces-or-inserts-and-re-sorts, `without(cat)` filters and
so preserves the order. Equality, hashing and storage all rely on the canonical
form, so two keys naming the same intersection in different orders *are* the
same key. Never build the inner `Vec` directly (gotcha 5).
That `without` preserves the sort is what lets `record_of` be a one-liner rather
than a normalisation step — see §5.
## 2. Aggregating a cell's raw values
Three functions, deliberately with three different emptiness rules, and the
differences are the whole content of the design (improvise-kdp):
- **`apply_agg(values: &[f64], func)`** — reduces numbers. `None` on empty: an
empty cell reads as *absent*, not as zero, or every blank intersection in a
pivot would render `0`. Both aggregation paths (`evaluate_aggregated` and
`aggregate_raw`) go through it so their semantics cannot drift.
- **`aggregate_cell_values(values: &[&CellValue], func)`** — reduces a mixed
set. See below.
- **`eval_agg(...)`** — evaluates a formula-level `SUM(x)` / `AVG(x WHERE …)`.
Its emptiness rule is *not* `apply_agg`'s: an aggregate over no rows is `0`
for `SUM`/`COUNT`, which have an identity, and an error for
`AVG`/`MIN`/`MAX`, which do not.
### `evaluate_aggregated` takes `Aggregation`, not a list it cannot read
The parameter was `none_cats: &[String]`, and the body consulted it exactly
once, for emptiness — a boolean wearing a list's type, since the aggregation
itself runs over `key.0` through `matching_values`. It never filtered by that
list, in any revision: `git log -S` back to the commit that introduced
`Axis::None` shows the same `is_empty()` and nothing else. So the name promised
a filter the body has never performed, and the next caller to build a careful
list would have threaded it into a parameter that cannot use it.
`Aggregation::{Exact, OverHidden}` admits only the two states the body
distinguishes, and `Aggregation::of(none_cats)` performs the reduction once, at
the boundary that holds the list (improvise-5jm5,
`doctrine(an_invariant_is_made_unrepresentable_not_validated)` — the same answer
`prune_axis_items` and `gp-tree`'s `Row<T>` got). It is an enum rather than a
`bool` because the two states have names worth reading at the call site, and the
regression guard is the **signature**: the thing the ticket describes — two
different lists, same emptiness, same answer — is no longer constructible.
Note what does *not* change: `recompute_formulas` and `aggregate_raw` keep
`&[String]`, because they walk the elements (stripping each hidden coordinate
off a stem). Two parameters that looked alike were doing two different jobs.
### Non-numeric values reduce by unanimity, and the branch order is the rule
`aggregate_cell_values` checks the **all-numeric branch first**. If every value
is a number it reduces through `apply_agg`; otherwise, if every value equals the
first, the result is that value; otherwise it is `CellValue::Error("mixed")`.
That ordering is the entire difference between this rule and "a single value
passes through". `Count` is the only function that can tell them apart — every
other function is the identity on a one-element set — so `Count` of a lone
number must still be `1` and not the number itself. `a_lone_number_still_counts_
as_one_rather_than_passing_through` is the pin.
What the rule forbids is **dropping**. The old code did
`filter_map(as_f64)`, and since `_Index`/`_Dim` sit on `Axis::None` in every
default view, `none_cats` is non-empty almost always and the aggregating path
was almost always taken — so every `CellValue::Text` in every document rendered
blank, including the pipe-quoted text `.improv` carries as a first-class value
(`Region=West, Measure=Revenue = |pending|`). A user could type a text value,
save it, load it, and never see it again (improvise-l4qx).
Both `evaluate_aggregated` and `aggregate_raw` share the helper, so a formula
ref over mixed values now errors `"mixed"` rather than silently summing the
numeric ones and reading as authoritative while under-reporting.
## 3. Formula evaluation
**Fixed point.** `recompute_formulas(none_cats)` clears `formula_cache`,
collects the distinct coordinate *stems* (each data key with the formula's
target category and every `none_cat` stripped), and iterates
`stem × formula target` until nothing changes, bounded by `MAX_EVAL_DEPTH = 16`.
Chained formulas (`Margin = Profit / Revenue` over `Profit = Revenue - Cost`)
converge naturally; circular references converge to `CellValue::Error("circular")`
rather than overflowing the stack.
**A cell still moving when the bound runs out has not converged, and the cached
path had to be told so** (improvise-7iww). The direct evaluator gets this free:
`evaluate_depth` bottoms out at `depth == 0` and returns `Error("circular")`.
The fixed point had no such arm — it simply stopped iterating and left the
sixteenth iteration in `formula_cache`, which `evaluate_aggregated` then served
as a valid number. `Revenue = Revenue + 1` over a stored `Revenue = 10` read
`Error("circular")` directly and `Number(26)` through the cache, and the cached
one is the **displayed** path, because the default views put the virtual
categories on `Axis::None`. So a circular formula looked like a real calculated
value. `recompute_formulas` now records the keys that changed on each pass and,
if the last pass still changed any, writes `Error("circular")` over them.
Note what this costs and why it is the right cost: the fixed point can only
*confirm* convergence with a pass that changes nothing, so a chain needing all
sixteen passes is now reported circular. That is the same bound the direct
evaluator already enforces — a sixteen-deep chain exhausts `depth` there too —
so the two paths agree rather than diverge, which is the property §3 exists to
protect. The anti-clamp guard is
`a_chained_formula_still_converges_after_the_circular_marker`. The stems are an `IndexSet`: dedupe is O(1)
per cell and insertion order is preserved (improvise-6os replaced an O(n²)
`Vec::contains`).
There are **two** evaluators, and they must be changed together:
- `eval_formula_depth` / `eval_expr` — the direct, uncached path, recursion
bounded by `depth`.
- `eval_formula_with_cache` / `eval_expr_cached` — the fixed-point path, which
resolves a `Expr::Ref` through `formula_cache` first and falls back to
`aggregate_raw`.
A rule stated in one and not the other is a live bug; improvise-0bf was exactly
that (see §4).
**`find_item_category`** searches real categories first and then formula
targets, because `add_formula` never inserts its target into `_Measure`
(improvise-nrs). Without the fallback, a reference to a computed measure fails
to resolve.
**Display rounding never reaches eval.** `format_f64` is called only in
rendering paths. Feeding a rounded value back into a formula is the bug
`formula_chain_preserves_full_precision` exists to catch.
## 4. Float equality has one epsilon, and the div/0 guard shares it
`FLOAT_EQ_EPSILON = 1e-10` is used by `=`/`!=` **and** by the division-by-zero
guard, in both evaluators. The two must agree or `IF(x = 0, a, y / x)` is
incoherent: the condition calls `x` zero and the else branch divides by it
anyway. With `x = 1e-11` the old exact `rv == 0.0` guard returned `1e13`
(improvise-0bf). `1e-10` is far below typical data magnitudes but absorbs
accumulated f64 rounding noise from chained formulas.
Note the repo map's gotcha 17 (which called this "currently mixed") describes
the *pre-fix* state; the guard and the comparison share the constant now.
## 5. Surrogate identities — `model/record.rs` (ADR 002)
A client sees a grid of label strings indexed by viewport position. It holds no
`Model`, no category names and no page/filter constraints, so it cannot build a
`CellKey` — which is why improvise-q08 found a thin client cannot compose
`SetCell`. The answer is an opaque surrogate the server projects per visible
cell and the client echoes back.
**Two surrogates, because two kinds of thing are nameable and only one is
settable.** `RecordHash` names a **record** — the coordinate tuple minus
`_Measure` (`record_of`). That unit is not chosen here: it is already ADR 001's
unit of atomicity and already what the write counters are keyed by, because
renaming a category coordinate in wide records mode moves every measure cell in
the row (§6). One identity therefore serves addressing *and* staleness.
`AggregateHash` names an **aggregate's coordinate constraint** — row and column
coordinates plus the page *and filter* selections that decide which stored cells
it sums (`View::filtering_cats()`, §7). An aggregate is not the *absence* of an
identity; it is a different kind of identity, which is what makes `Drill`
addressable at all. It is deliberately **not** normalised: a measure on Row or
Column is part of what the aggregate sums, so dropping `_Measure` would give two
visibly different totals one name.
**The hash is specified here, not inherited.** Three ways it breaks silently,
each producing a *wrong write* rather than an error:
1. **Not `DefaultHasher`, not `RandomState`.** `std`'s default `HashMap` hasher
is randomly seeded per process, so a surrogate would not survive a restart or
agree between two processes. This is SipHash-2-4 under a constant key.
2. **Not `#[derive(Hash)]`.** Rust gives no stability guarantee for a derived
`Hash` across compiler versions or platforms. What is hashed is the explicit
byte serialization in `hash_coords`, so the surrogate is defined by that
format rather than by whatever the compiler did this week.
3. **128 bits, argued rather than defaulted.** A collision is a silent write to
the wrong record — precisely what got `_Index` rejected as a surrogate — and
no later check catches it: a colliding surrogate resolves *successfully* to a
record the client never saw. 64 bits reaches birthday risk around 2^32
records, which a pivot tool will not see by live record count; but the risk is
over every record a long-lived document *ever names*, and the consequence is
unbounded and undetectable. The other choice costs 8 more bytes per *visible*
cell — 8 KB on a full 50×20 projection and far less on the diffs that carry
steady-state traffic. Bounded cost against silent corruption is not close.
**Serialization details that are load-bearing.** The byte format is
`tag ‖ count ‖ (len(category) ‖ category ‖ len(item) ‖ item)*`, every length a
little-endian `u64`. The lengths are what make it injective: without them
`("ab","c")` and `("a","bc")` serialize alike. Only `Hasher::write` is used,
never `write_u64`/`write_u8`, which are free to feed the hasher in native byte
order. The SipHash key constants are written as big-endian readings of
`b"improvis"`/`b"e-record"` so the constant is the same on every platform. A
domain tag byte (`0x01` record, `0x02` aggregate) is hashed first, because a
record and an aggregate may be built from the same coordinates — an aggregate
over a single record is the degenerate case — and they must not hash alike.
**Hex on the wire, not a JSON number.** `serde_json` writes a `u128` as a bare
number and a JavaScript host parses every JSON number as `f64`, so anything
above 2^53 comes back changed — which for a surrogate means resolving to a
different record or to none. The wire form is a fixed 32-character lowercase hex
string.
## 6. Records mode is wide format (`view/layout.rs`)
A row is one **coordinate tuple** — the cells sharing everything but `_Measure`
— and each raw measure gets its own editable column. The columns are
`[coordinates…, +cat, measures…, +]`: one per regular category, then the column
that creates a **new category** (improvise-2zq), then one per raw measure, then
the column that creates a new measure (improvise-c5v). Each `+` sits adjacent to
what it adds to; two trailing columns would have made the user read a label to
tell them apart, and position says it instead. Formula targets are not columns —
they are computed, so there is nothing to type into them.
Consequences: two measures on one coordinate are **one row**; editing a category
column moves **every** measure cell in the row, not just the one under the
cursor; and `_Dim` in the synthetic key carries the column **index**, not a
label, because a category and a measure item may share a name and a label could
not say which column an edit landed on. Resolve the index through
`GridLayout::record_col` into a `RecordCol`. Drill pending edits are keyed by
`(row, RecordCol)`.
`RecordRow::group` collapses loose cells into wide rows in **first-appearance**
order, so the result follows the store's order rather than a hash walk (§8). A
cell with no `_Measure` coordinate still anchors a row — that is how
`add-record-row` seeds one — but contributes no column value.
**Records mode is a property of the view's axes, and `View` owns the one
definition.** `View::is_records_layout()` (`_Index` on Row and `_Dim` on Column)
is what `GridLayout::new` asks and what `on_category_added` asks; two
derivations would drift (improvise-edp, the disease this codebase keeps curing).
### `prune_axis_items` takes a predicate, not a flag vector
`prune_empty` hands it `impl Fn(usize) -> bool` over the **data** index — the
position a `row`/`col` argument means, which group headers do not consume. The
predicate's domain is derived from `items` itself, so there is no parallel
length a caller can get wrong and debug and release builds cannot disagree about
an out-of-range flag. The earlier `&[bool]` + `debug_assert` pair could have,
had a third caller ever sized `keep` from something other than
`row_count()`/`col_count()` (improvise-n3wt,
`doctrine(an_invariant_is_made_unrepresentable_not_validated)`).
A group header survives only when a data item that follows it survives. In
`prune_empty` the row pass does not touch `col_items` or `has_value`, so the
column pass still reads the original grid. Records mode is exempt entirely — the
user drilled in to see all the raw data.
### Group runs nest, and pruning re-marks rather than filters (improvise-e265)
On an axis with two grouped categories the runs **nest**: `North` in `Upper`
holds `Jan` in `Q1`, and after `Q1` ends `North/Mar` is still in `Upper`. So a
marker names its level. `GroupHeader { level, depth, .. }` opens a run at
`(level, depth)` and closes every deeper one; `GroupEnd { level, depth }` closes
that run and every deeper one. "Deeper" is the ordering of the pair: the level
first, then the depth inside one category's hierarchy (§19, improvise-felo.6).
`expand_category` reads the level off its prefix, which holds one item per
outer category, so it is derived rather than passed. A row's group, for
`row_group_for`/`col_group_for` and so for `z`, is the **innermost** run open
at it. That is the user's ruling on the ticket: membership is per level, and
`z` toggles the group nearest the cursor.
The marker used to carry no level, and every reader was flat: the nearest
marker decided. Under a flat reading, pruning and membership could not both be
right. Before #200 a pruned axis could draw a header over nothing; after it, a
surviving row changed group when an inner header was pruned, because the
inner `GroupEnd` went with it (45,806 of 300,580 enumerated cases, every one
that shape). The unpruned answer was also lossy: `North/Mar` was in no group
while it was drawn under the `Upper` header row.
**Pruning therefore re-marks the survivors instead of filtering the markers.**
`prune_axis_items` tracks which run the input has open at each level (keyed by
the header's position, so two runs under two equal headers stay two, as
improvise-h0iv requires) and which run the kept entries have open. Before each
surviving item, `remark` emits a header wherever a different run should be open
and a `GroupEnd` wherever none should. Three properties follow by construction:
- each survivor keeps exactly the runs it was in;
- no header is emitted without a member, because headers are emitted only
before an item;
- no trailing `GroupEnd`, because a `GroupEnd` is also emitted only before an
item.
This replaced the `pending`/`close_runs_of`/`run_open` machinery, which is
where improvise-gd83, u6gc, lhi4 and b5zh each landed. When nothing is pruned
the output is the input, marker for marker, so the exact-sequence tests from
those tickets still pass unchanged. That equality needs one rule: `remark`
closes the deeper runs innermost-out before it touches a level, as
`expand_category`'s trailing `GroupEnd`s do. Without it the output is
equivalent but not equal (`End0` where the input has `End1, End0`), and the
enumeration test asserts equality whenever every row survives.
**How it is tested** (improvise-6u6s). An enumeration compares each
surviving row's whole open-run stack, and whether it shares a run with the
previous survivor, pruned against unpruned. It runs over five fixtures,
including a non-contiguous group, a level with no groups, and two adjacent
outer groups. Without the non-contiguous fixture, a pruner that keyed runs by
name instead of header position passed every test. `walk_open_runs` is the
one reading of the markers: `innermost_group` and the tests both use it. It
keeps one stack and lends it to a visitor at each data item, and the visitor
may stop the walk. So a single lookup (`z`, which asks about one row) walks
the axis only up to that row and copies nothing for the rows before it
(improvise-0r01). An iterator could not do this. Its first version yielded an
owned stack per row, and ChatGPT Codex found on #273 that `.nth(idx)` still
cloned one for every row it skipped. The tests collect every row's stack
through the same visitor.
A header's level also names its category (`cats[level]`) on the wire, where
`FrameGroup` carries `level` and not the category (improvise-7xyx removed that
field): see `context/code/protocol.md` §3a.
## 7. Axes: `Page` and `Filter` are two different questions
`Axis` is `Row | Column | Page | Filter | None`. `Filter` names a coordinate the
**drill** fixed (improvise-dz8): like `Page` it constrains which records are
visible, but unlike `Page` the user cannot navigate it. `Axis::next()` maps
`Filter` to itself and no navigable axis rotates into it, so tile select cannot
hand one out, and `is_user_assignable()` is false for it.
Picking the wrong question is the trap:
- **"what data is visible"** → `View::filtering_cats()` (Page ∪ Filter). Used by
`GridLayout`, by `AggregateHash`, and by `add-record-row` when it builds a key
from the current constraints.
- **"what can the user act on"** → `categories_on(Axis::Page)` for `[`/`]` page
cycling, or `View::tileable_cats()` for all three tile-select sites.
### Leaving the constraining axes forgets the page selection (improvise-z336)
`set_axis` used to leave `page_selections` alone; only `on_category_removed`
and `on_item_removed` ever cleared one. So cycling a category off `Page` merely
**hid** its filter, and cycling back to `Page` silently reinstated it — the
same item, unasked. Measured in the browser: `Pag(Espresso) → None → Row → Col
→ Page` and Espresso is back.
That mattered more than it reads, because there is no clear-a-page-selection
operation anywhere in this workspace — not in `ModelCommand`, not in the `View`
API, and **not in the TUI**; `[` and `]` cycle among the category's items with
no "All" to land on. So a user who had applied a filter had no way out of it at
either front end, which is a product gap at parity rather than a browser
regression. Forgetting the selection on the way out closes it with no new
command, no new wire variant and no new key: cycling the axis round is now a
way out.
The predicate is `Axis::filters_data()` — Page ∪ Filter — and not "is it
`Page`". That is this section's own trap, seen from the writing side: a drill
fixes its coordinates on `Filter` and `.improv` round-trips a selection under
it, so clearing on everything-but-`Page` would pass a test written for the
browser symptom and silently break drill.
`Axis` also owns four presentation vocabularies — `plain_label()`, `glyph()`
(which `Display` appends, giving "Row ↕"), `short_label()` (three columns for
the tile bar; `None` is "·", **not** a truncation of "None") and `initial()`
(one letter for the view panel; `None` is empty) — plus `color() -> AxisColor`.
`AxisColor` is a *semantic* palette because `improvise-core` must not name a
ratatui type; `draw.rs::axis_fg` does the one mapping, exactly as `minibuf_fg`
does for `MinibufColor` (improvise-rml). The tile bar and the category panel each
carried a copy of the colour table before, with nothing keeping them in
agreement.
### `on_category_added`: a virtual on an axis is a configuration, not a vacancy
Virtual (underscore) categories seat on `Axis::None`. A regular category takes
the first slot no *regular* category holds — Row, then Column, then Page.
This used to **bump** a virtual off Row or Column to make room, assuming a
virtual sitting there was a placeholder. It never is, and the assumption cost
two live bugs (improvise-2zq):
- `_Index` on Row together with `_Dim` on Column *is* records mode, and the only
production writers of those axes are the records and drill setups. Bumping
`_Index` **ejected the user from the grid they were standing in**: naming a
category with `N` in records mode silently returned them to a pivot.
- `_Measure` on Column is an ordinary user configuration — it is what
`web/demo.improv` ships. Bumping it made every measure vanish from the grid
the moment a second regular category was added.
So a virtual is never moved. A records view seats the new category on
`Axis::None`, which is where records mode keeps **every** regular category and
where `records_columns` — which filters on `kind.is_regular()` and never looks at
an axis — picks it up as a column immediately. Elsewhere the new category simply
joins whatever virtual is already there, nesting a header rather than destroying
one.
The negative arm is load-bearing and must stay tested: without
`a_pivot_view_still_seats_categories_row_then_column_then_page`, "always seat on
`None`" would pass every assertion about records mode. Probing confirmed the
asymmetry — seating everything on `None` fails 12 tests, while removing the
virtual bump failed none at all, which is how the bump came to be wrong in two
directions with nothing noticing.
### Known defect: two answers to "is this name a virtual"
`on_category_added` tests `cat_name.starts_with('_')` (twice) while
`is_records_layout()` four lines away asks `CategoryKind::of_name`. That is
**improvise-bidq** and it is open; do not assume either spelling is the intended
one when editing near it.
## 8. Ordering is correctness, not tidiness (`model/cell.rs`)
`DataStore` holds cells in an `IndexMap` keyed by `InternedKey`, plus a
secondary index `HashMap<(Symbol, Symbol), HashSet<InternedKey>>`.
**An index whose values are `HashSet`s is for O(1) lookup, never for
iteration.** `matching_cells`/`matching_values` used to walk the index's
`HashSet` directly, so their order was hash order — different in every process,
and different between two stores in one process, because `RandomState` keys each
`HashMap` afresh. That defeated the whole reason `cells` is an `IndexMap`, and
it was not tidiness (improvise-shyt). It caused, in ascending order of
seriousness:
- **records-mode rows reordering between sessions**;
- **`SUM`/`AVG` drifting in the last `f64` bits**, because f64 addition is not
associative (`Min`/`Max`/`Count` and the text-unanimity rule of §2 are
order-free, so they were never affected);
- worst, the **drill snapshot** freezing its record list in hash order while
`drill_state.pending_edits` are keyed by *row index* (§6) — so the same drill
opened in a second process addressed **different records**. Silent misdirected
writes, and nothing in the tracker knew.
`matching_positions` is the fix: the index still **narrows** the candidates, and
the primary map **orders** them — each surviving candidate resolves to its
position with one hash lookup and the positions are sorted. Shape stays
O(smallest set × |partial|) plus O(m log m) over the *matches*, not a scan.
Measured at 18,000 cells: +20% worst case on `matching_values`, and
`matching_cells` 7% *faster*, sorted positions resolving `CellKey`s with better
locality. The alternatives both move the cost — an ordered index value type pays
on every `set`; scanning the primary is O(store) per query.
**An empty `partial` matches every cell, vacuously, and `matching_positions` is
where that is answered** (improvise-hkt6). It used to index `sets[0]` and panic;
both public callers happened to guard, so it was unreachable — but the guard
lived one level away from the indexing, which is how this class of bug returns.
The guards could not simply be pushed down as `vec![]`: "no coordinates
constrain anything" means *all* positions, and answering "none" here would have
made removing a caller's fast path change behaviour silently. The callers keep
theirs as a fast path, not as a guard.
**`live_records` has a decided order, not an inherited one** (improvise-2yc2).
It walks `cells` and projects insertion order onto records: a record sits where
its **first** cell sits. It deliberately pays O(cells) rather than O(records),
because `cells`' order is already the answer `matching_positions` gives and a
second answer to "what order is this store in" is the disease. It is derived per
call rather than memoised on `RecordEntry`, since a stored position is a copy
and a record deleted and re-created would hold its old one while its cells sit
at the end of the store. Note what is **not** on offer: an unordered return.
Every iterator has an order, and a type that merely disclaims one still hands the
caller hash order — the guard living one level away from the hazard, which is
the shape this whole class of bug returns through. The function had no callers
when this was decided, so the order was still free to *decide* rather than to
discover; with no caller to benchmark against, correctness is the defensible
default, and a hot-path caller can argue for a memoised position with numbers.
### The record index: `RecordEntry`
`DataStore::records: HashMap<RecordHash, RecordEntry>` answers both of a client
surrogate's questions — "which record do you mean?" (ADR 002) and "is your basis
stale?" (ADR 001) — from **one** entry in **one** lookup, because both are keyed
by the record and both are invalidated at the same moment.
It lives in the store because this is where the same job is already done: the
store already interns every name and already maintains a secondary index updated
at exactly these two points (`set`, `remove`), and it is that existing
`(Symbol, Symbol)` index which makes **invalidation** tractable — when a
category or item disappears, the cells naming it are found without a scan, so
the records built from them are too.
It is deliberately **not** regenerated from the layout on demand. That would be
O(visible cells) per resolve *and* would only ever cover the visible window, so
a record scrolled out of view would look identical to a record that no longer
exists — conflating "reject, nothing to write to" with "apply".
It is **not serialized**: `Deserialize` rebuilds it through `set`, and every
version starts at zero, which is correct because a freshly loaded document is a
new `document_generation` and no client holds a basis against it.
**`cells == 0` is a tombstone, not a reason to forget the entry.** A record that
is deleted and then re-created must not hand a client holding the old basis its
own version number back; dropping the entry would reset the counter to zero and
make a stale basis look *current*, which is the silent accept the basis check
exists to prevent. The counter tracks **liveness, not writes** — `set` bumps
`cells` only for a genuinely new cell, and `version` is moved by the observed
diff in `App::apply_effects`, never from `set`.
`resolve_record` returning `None` means **the record is gone** — never stored,
or every cell removed, which is what happens when a category or item it named is
deleted. That is one arm of staleness and needs no version comparison: there is
nothing to write to. `record_version` reporting `0` for an unknown surrogate is
deliberate: a basis composed against an absent record and one composed against an
untouched record are the same claim.
## 9. Categories
`CategoryKind` is `Regular | VirtualIndex | VirtualDim | VirtualMeasure |
Label`. Business rules match on the enum rather than sniffing name prefixes.
`CategoryKind::category_name()` is the **one** place the name↔kind mapping is
written down, so adding a variant forces a decision there (improvise-2lh).
`of_name(name)` is safe to use in place of looking the `Category` up, because
`Model::add_category` hands back the existing virtual category rather than
shadowing a reserved name — `stored_virtual_categories_match_their_names` is
what makes that sound. `Label` categories are user-named and so read as
`Regular` from `of_name`; consult the stored `Category::kind` when the
distinction matters.
`VirtualMeasure` items come from two sources: numeric data fields listed in the
file, and formula targets added implicitly. They are virtual because
formula-derived items are *implied* by the formula definitions — listing them
explicitly would be redundant in the file format and confusing in the UI.
`add_formula` does **not** add items to `_Measure`; use `measure_item_names()` /
`effective_item_names("_Measure")`.
`Label` is a high-cardinality per-row field (description, id, note): stored
alongside the data so it shows up in record/drill views, but defaulting to
`Axis::None` and excluded from the `MAX_CATEGORIES` pivot limit and from
auto Row/Column assignment.
`Category::add_item` deduplicates by name and assigns IDs from a private
counter, so external code cannot fabricate an `ItemId`.
### The limit is 24, and nothing is exponential in it (improvise-felo.1)
`MAX_CATEGORIES` counts **regular** categories only. It is `pub` in
`model/types.rs` and re-exported as `improvise_core::model::MAX_CATEGORIES`,
and it has one home: the import wizard's precondition reads it rather than
keeping a copy. It had a copy until felo.1, a private `const` of its own, and
raising the model's number alone would have left the wizard refusing a 13th
category the model accepts. Tests that fill to the cap derive it from the
constant; `add_category_max_limit` is the one place the number 24 is spelled.
**Why 12.** Nothing recorded says. The limit arrived in the initial commit
with no comment, ADR or ticket behind it, and nothing in `context/` or `kb.pl`
argues for it. The likeliest source is Lotus Improv, which this tool follows
by name (`context/plan.md`) and whose worksheets are remembered as capped at
12 categories. That is not verified here. It was never a cost bound. The user raised it on 2026-09-25: "12 categories is
feeling small, we should increase to 24". An E*Trade import hit exactly 12,
leaving no room for improvise-cgqt's disambiguating category.
**What scales with the number of categories**, read off the code:
- `CellKey` is a sorted `Vec` of pairs: building one is O(k log k), and
comparing or hashing one is O(k).
- The `DataStore` secondary index holds one `(category, item)` entry per
coordinate per cell. That is O(k) per insert and O(cells × k) in memory.
- Records mode has one column per regular category, so it is linear in k.
- `cross_product` is the product of the item counts of the categories **on
an axis**. That is exponential in what the user places on the axis, and the
cap never bounded it: 12 categories of 4 items on rows is already 16M rows.
`prune_empty` is what keeps it drawable.
- `improvise-gp` bounds its encoded width separately (`MAX_FEATURES = 64`),
so a wide fit is refused by name rather than slowed.
**Measured**, `cargo run --release -p improvise-core --example
category_width_bench`, on an Apple M4 Max. Each category has 4 items and each
record carries 2 measures. The pivot is one category on rows, `_Measure` on
columns and the rest aggregated away. Figures are the best of 5 (the store
build is the best of 3), and they held across two runs.
| categories | records | build store | pivot, every cell | records build | records draw |
|---|---|---|---|---|---|
| 12 | 2,500 | 26–30 ms | 1.6 ms | 79 ms | 1.6 ms |
| 24 | 2,500 | 80–83 ms | 3.0–3.7 ms | 82 ms | 3.7 ms |
| 12 | 5,000 | 54–57 ms | 3.2–3.9 ms | 302 ms | 3.1 ms |
| 24 | 5,000 | 163–172 ms | 6.3–8.0 ms | 321 ms | 7.0 ms |
Doubling the width doubles the per-cell work and triples loading the store,
which also builds and hashes CellKeys twice as long. The records-mode build is
the outlier, and width is not its cause: it grows 4× when the **records**
double, at either width. `RecordRow::group` finds each cell's row with a linear
scan (improvise-sk32).
#### Coupling found
Raising the constant was used to look for code that assumes few categories.
This inventory was taken at felo.1's merge of `gh/main` `4f38a35e`. Line
numbers are dated, so re-derive them before citing.
**Reads the constant (fixed in felo.1).** The wizard's precondition, and the
duplicate step's cap check that improvise-cgqt added (`separate_blocked`). A
fixture that fills to the cap either derives it or fills until
`add_category` refuses.
**Separate limits, left alone.** None of these is a category count:
- `MAX_ROW_HEADER_W = 24` (`improvise-protocol/src/render.rs`) is the widest
one header level is drawn, in cells. It equals the new cap by coincidence.
- `MAX_FEATURES = 64` (`improvise-gp/src/fit.rs`) is the encoded-width budget
of one fit.
- `CATEGORY_THRESHOLD = 20` (`import/analyzer.rs`) is the distinct-value count
at which a text field becomes a `Label`.
**Degrades as categories grow.** These are not fixed here; each is a place
where 24 is worse than 12. Most were already reachable at 12.
- **The TUI row header is the sum of its levels** (improvise-gtn7.5). In `src/ui/grid.rs`, around
`row_header_width`/`data_area_width` (and
`ui::viewport::compute_row_header_width`), each level takes 4 to 24 cells.
Categories on Row therefore cost at least 4 cells each, and the data area
saturates to zero width with no data drawn.
- **Column headers take one line per level, but the keypress path does not
count them** (improvise-gtn7.6). `App`'s `visible_rows` is `term_height − 8`, a fixed allowance
that ignores the column levels, the group row and the page bar. So with
several categories on Column, the cursor can scroll below the drawn grid.
- **The page bar is one unclipped line** (improvise-gtn7.7). It is
`GridWidget::render`'s `page_coords` join, drawn with a bare `set_string`
that is not clipped to the inner rectangle, so a long bar can overwrite the
block's right border. Default seating (`View::on_category_added`) sends every
regular category after the second to Page. A 24-category import therefore
puts 22 selectors on one line, clips most of them without a mark, and opens
on a single slice.
- **Side panels do not scroll** (improvise-gtn7.8). In `src/ui/panel.rs`, an item past
`item_height` is not drawn, and the cursor can move onto it unseen. The
category panel lists every category plus its expanded items.
- **The wizard's field list does not scroll** (improvise-8jk6.1). Its `ReviewProposals` arm in
`src/ui/import_wizard_ui.rs` breaks at the popup's bottom (the popup is at
most 30 rows high), unlike the steps that use `scrolling`. About 22 fields
fit, so the over-the-cap case cannot show every field it asks the user to
fix.
- **Loading the store is O(k log k) per cell and O(cells × k) in memory**
(improvise-felo.2). It
measured 3× from 12 to 24 (the table above).
- **A fit declines more columns.** `unaffordable_categories` drops the widest
category columns once the encoding passes `MAX_FEATURES`. A wide document
is fit on fewer of its categories, and the fit says so.
- **`cross_product` is exponential in the categories placed on one axis**, and
`prune_empty`'s `has_value` matrix is rows × columns. The cap never bounded
either. Virtualizing the grid is improvise-felo.3.
- **Header repeat suppression costs O(levels) per position and level**
(`render.rs::shows`). That is O(rows × levels²) per frame, which is small at
24.
The records-mode build's quadratic (improvise-sk32) is not category coupling;
it is named in the measurement paragraph above because this measurement found it.
The browser adapts where the TUI does not. `#tiles` and `#categories` scroll,
and `web/shell.js` charges each column level to the extent.
### Seating takes the kind, because a name cannot carry it (improvise-cj4u)
`View::on_category_added` takes a `CategoryKind` beside the name. It used to
take the name alone and ask `of_name`, which reads `Label` as `Regular` by
design — so the *default* view got a label on `Axis::None` (because
`Workbook::add_label_category` set it explicitly afterwards) while
`Workbook::create_view`, which only ever had names to hand, seated the same
label on Row. A high-cardinality description column therefore became a pivot
axis in every view created after it, and on an already-pivoted document it
landed on `Page`, filtering the whole document down to one description.
Two things follow, and the second is the one worth carrying.
**The parameter is the fix, not a `set_axis` after the fact.** The old
`add_label_category` seated the category and then corrected it, which is two
answers to "where does this category go" — and only one of the two callers of
`on_category_added` knew to write the second. The correction is now deleted:
seating is told the kind and answers once. `add_label_category_sets_none_axis_
on_all_views` and `label_category_defaults_to_none_axis` both still pass, and
both fail if the parameter is ignored, so the consolidation kept its evidence.
**`of_name` survives, narrowed.** Every remaining caller either holds no
`Category` (the view's own `is_records_layout`, `holds_a_regular`) or is test
code building a view from bare names. That residue is real: `holds_a_regular`
asks of the categories *already seated* on an axis whether one of them pivots,
and it asks by name, so a label a user deliberately moved onto Row still counts
as occupying it. That is the right answer — a category the user put on Row is
pivoting there — but it is an answer reached by a different route from the one
above, and a future `View` that stores kinds should collapse the two.
### Removing a name removes the formulas targeting it (improvise-g3no)
`Model::remove_category` has always ended by dropping every formula whose
`target_category` is the category — that is why `Dirty::of(LayoutGeometry)`
reports `formulas: true` (gotcha 35). `remove_item` did **not** do the same one
level down, and the asymmetry was a live bug rather than an omission of
symmetry for its own sake.
`_Measure`'s items are explicit items ∪ formula targets, so a `Profit` that
exists only because a `Formula` does is not in the category's own item list.
`remove_item` called `Category::remove_item`, which no-opped, then swept the
cells naming it, of which there are none because the value is derived.
`effective_item_names` re-synthesised the row from the surviving formula and it
came straight back — while both front ends reported success, each having
composed an optimistic past-tense status beside the mutation.
Two things about the shape of the answer. **The lie was a symptom, not the
defect.** The obvious reading is gotcha 20's ordering rule violated — the
fallible effect first, the optimistic status second — and the obvious repair is
to give `remove_item` an answer the front ends let win. That repair is
unavailable on the browser side, in two independent ways: `SetStatus` is
`ViewOnly` and never crosses the wire, so a host-side refusal cannot reach a
client; and a client cannot decide for itself, because nothing in the
projection stream says which `_Measure` rows are derived — `FormulaCache` is
`Vec<String>` of `Formula::raw`, and recovering a target from that is a second
home for the unquoting §34 says has exactly one. Restoring the sweep makes the
status both ends already compose *true*, with no wire change and no second
classification.
**What this does not answer, and is declined rather than forgotten.**
`doctrine(a_status_line_composed_beside_a_mutation_lies_when_the_mutation_no_ops)`
generalises to "a mutation whose failure is reportable must RETURN whether it
acted", and `remove_item` still returns unit. Restoring the sweep removes the
*reachable* lie — from either panel the item name comes off the tree, so it
exists — and leaves a narrow one: a wire `ModelCommand::RemoveItem` naming an
item the document does not have is still answered with a past-tense success.
Closing that means threading an answer through `Workbook::remove_item` and
`effect::RemoveItem`, and it is improvise-nwgk rather than done here.
**And the sweep is not restricted to `_Measure`.** A non-measure target is a
real item (`add_formula` creates it), so removing that item used to leave a
formula aimed at nothing. The predicate is `target_category` **and** `target`:
deleting `Cost` must not delete `Profit = Revenue - Cost`, which references it
but does not target it.
### A required virtual category cannot be removed (improvise-3b2b)
The category panel deliberately lists `_Index`/`_Dim`/`_Measure`
(improvise-vjms — a decision about *visibility*), and its delete key treated
them as ordinary removable categories. `d` at the panel's default cursor 0
therefore deleted `_Index` from a fresh session, and `GridLayout`'s records-mode
detection keys off `_Index` on Row, so records mode silently stopped working
while the status line said it was on.
`Model::remove_category` refuses when `CategoryKind::of_name(name).is_virtual()`.
The guard is at the model rather than in the two `Cmd`s because hiding a key
leaves `ModelCommand::RemoveCategory` unguarded on the wire. It reads
`of_name`, the one home of the mapping, so a `Label` category — user-named, and
`Regular` to `of_name` — stays removable.
## 10. `Workbook` exists to break the `Model ↔ View` cycle
`Model` knows nothing about views; `View` depends on `Model`, one direction.
Cross-slice operations — adding or removing a category must update both the
model's categories and **every** view's axis map — live on `Workbook`, so
`Model` stays pure data and `improvise-core` can be extracted without pulling
view code along. That is also why `AddCategory`/`RemoveCategory` are
`LayoutGeometry` effects rather than `CellData` ones (gotcha 3).
`Workbook::new` seeds a `Default` view, registers the three virtual categories
through `on_category_added` (so all three land on `Axis::None`), then bumps
`_Measure` to `Axis::Page` so aggregated pivot views show a single measure at a
time (improvise-kos, commit 709f2df). Leaving `_Index`/`_Dim` on `None` keeps
pivot mode the default — records mode activates only when the user moves both
onto axes.
`Workbook::add_item` errors on an unknown category so callers do not reach
through `model.category_mut(..)` and invent their own handling (improvise-tvt).
There is no `Model::add_item`.
## 11. Virtual views
`_Records` and `_Drill` (`view::VIRTUAL_VIEW_NAMES`) are scratch views the app
builds on demand. Both put the virtual `_Index`/`_Dim` categories on axes, and
those categories are never persisted — so these views are rebuilt from scratch
each time rather than saved (improvise-60z).
### What the view must be is model-side; how a front end gets there is not
`view::layout` answers **what** a virtual view is, as data: `records_spine()`
(`_Index` on Row, `_Dim` on Column — the pair `GridLayout` auto-detects records
mode by), `records_steps(model)` and `drill_steps(key, none_cats)`, each a
`Vec<ViewStep>` of `Axis` and `PageSelection` assignments. Nothing in them
names an `Effect`, an `App` or a front end.
The command layer answers **how**: `cmd/grid.rs::steps_into_active_view` maps
each step to the `effect::SetAxis`/`SetPageSelection` a TUI keypress applies to
`ViewTarget::Active`. That is the only thing the two rebuilders still have in
common, and it is three lines of translation.
The split exists because a second front end has to reach the same view without
the effect layer — a browser composes `ModelCommand`s naming `_Drill`, not
effects targeting an active view it does not own. Design-principles §5 is the
rule: a derivation done twice is the defect even when both copies are right
today, and before this split the drill's filter placement and the records
spine were spelled out twice in `cmd/grid.rs` with nothing keeping them in
agreement. `drill_constraint` and `drill_snapshot` were already model-side for
the same reason and are untouched; §8 says why their **order** is the thing
that cannot be derived a second time.
What deliberately did **not** move is `DrillState`. It holds an `Rc` and its
`pending_edits` are uncommitted writes belonging to whoever staged them — the
session-local residue of gotcha 30 — so it stays on `App`, which is where that
tag puts it. Separating the view configuration does not make the staging map
portable, and a decomposition that claimed otherwise would be the interesting
half of this ticket answered by assertion.
### `create_view` replaces, and `unused_view_name` is why it may keep doing so
`Workbook::create_view` inserts over whatever holds the name. That is not an
oversight: "rebuilt from scratch each time" **is** that replacement, and both
rebuilders reach it through `effect::CreateView` — `DrillIntoCell` and the
records-mode command each emit a `CreateView` of their fixed name on every
entry, so an entry into records mode would otherwise stack views or refuse.
The third production caller, `parse.rs::build_view`, guards with
`contains_key` and so cannot observe the difference.
So the two callers that need a **new** name are the user-facing ones, and they
are the ones that must not name a view that exists. Before improvise-vw47 both
spelled `format!("View {}", views.len() + 1)`, which is the count and not the
name: delete `View 2` from a workbook holding `Default`, `View 2`, `View 3` and
the count says `View 3`, which is taken. `create_view` then did exactly what
its other callers need and replaced it — the user's configured view silently
reset to defaults, and no view added. **A symptom that looks like nothing
happened.**
Two fixes were rejected in favour of the helper, and both are worth naming
because each turns a quiet wrong answer into a loud one at a caller that was
never wrong. Making `create_view` **refuse** a name it holds breaks records
mode outright — the key stops working, which reads as a better error and is a
worse bug. Making it **rename** silently gives the rebuilders a `_Records 2`
that `RECORDS_VIEW` does not name and nothing switches to.
`view::unused_view_name` therefore answers only the question the two user-facing
resolvers ask, and `create_view` keeps its semantics untouched. It takes an
iterator of names rather than a `&Workbook` because the browser resolver holds
no workbook — it has `ViewListCache`, a `Vec<ViewSummary>` that
`project::view_summaries` maps one-for-one off `workbook.views`, so the two
front ends see the same list and derive the same name **by calling the same
function**, which is what design-principles §5 asks for and what
`tests/lockstep.rs` witnesses. It starts its search at the count rather than at
1 so that the common no-gap case keeps its existing answer; starting at 1 would
rename the first view a fresh workbook creates from `View 2` to `View 1`.
## 12. `axis_of` panics, `try_axis_of` does not
Registration is guaranteed for every model category: `Workbook::new`,
`add_category`, `add_label_category` and `create_view` all call
`View::on_category_added` for every view. So any name obtained from
`Model::categories` or from the view's own `category_axes` is safe for
`axis_of`. Use `try_axis_of` when the name comes from anywhere else —
`cycle_axis` does, and no-ops on an unknown category to match `set_axis`
(improvise-t8s).
### `record_cell_key` is a match, and the match is the guard (improvise-ir7l)
`GridLayout::record_cell_key(row, col)` is the one mapping from a records
position to a model cell, and it answers `Some` for `RecordCol::Measure` alone.
The other three arms are spelled out rather than swept into a `_`, and that
exhaustive match is the whole guard: a new `RecordCol` variant fails the build
until someone decides whether it names a stored cell.
**There is no test for the `Category` arm, and the reason is not that the wrong
answer would be harmless.** It would be a real fault, and a subtle one worth
naming so nobody re-derives it wrongly. `RecordCol::Category(name)` carries a
**category** name, while `RecordRow::cell_key(arg)` puts its argument under
`_Measure`. So answering `Some` there yields `coords + _Measure=<category
name>`, and `Model::formula_for` would match it whenever a `_Measure` item
happens to share a name with a regular category — projecting `CellId::Derived`
for a **coordinate column**, which is never a formula target and which the
pivot path does not have at all. The two front ends would then disagree, which
is the design-principles §5 defect this ticket exists to close. `RecordCol`
exists for exactly that collision: its doc says the enum resolves a column from
its index "so a category and a measure sharing a name stay distinguishable".
What earns the decision to leave it untested is narrower: the **reachable**
version of that fault is a new variant answered carelessly, and the exhaustive
match makes that unrepresentable rather than merely unlikely
(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`). A test would
pin behaviour the type system already pins, and would not survive the arm being
renamed. Found in review, and the first recorded reason for declining it was
**wrong** — it claimed the injected behaviour was defensible by analogy with a
formula targeting a regular category, which is a different key entirely.
## 13. Route then aggregate: `SUM(<expr>)` folds over the member records
improvise-4zl7, and the one place the evaluator reaches past the cell it was
asked about.
**The problem.** A formula is evaluated at a `CellKey`. In an aggregating cell
`evaluate_aggregated` aggregates each `Expr::Ref` *first* and evaluates the
expression once on the totals. That is exactly right for a linear formula —
`SUM(Revenue) - SUM(Cost)` and `SUM(Revenue - Cost)` are the same number — and
exactly wrong for anything that **routes**. `IF(Cost < 50, 1, 10)` over records
costing 10 and 100 answers `1 + 10 = 11` per record and `10` on the total,
because *aggregate-then-route is not route-then-aggregate*. Every fitted tree
(improvise-8xhy, improvise-vx3) is such a formula, so it meant one thing on a
record row and something else one row up. The user's ruling was to **aggregate
the rule over the members**.
**The mechanism, and why it needed no new vocabulary.** `SUM(e)` already means
"fold `e` over the members". It simply did not: `eval_agg` specialised
`Expr::Ref` and, for every other inner expression, summed the raw cells matching
the partial key and *ignored the expression entirely*. It now folds — one entry
per member record, the expression evaluated at that record's own coordinates:
```
partial = context.without(target_category) (plus any inline WHERE)
records = distinct(matching_cells(partial).map(key.without("_Measure")))
values = records.map(|r| eval(inner, context.with(every coord of r)))
```
Three consequences worth stating.
The rule is **written once, and so is the evaluator it folds with.** `eval_agg`
is shared by `eval_expr` and `eval_expr_cached`, and **both** hand it the same
`eval_at`: the record-level `eval_expr`. That is not a convenience, it is the
correctness. A member key is a *record*, and `formula_cache` is keyed by
**stems with `none_cats` stripped** — so the cached evaluator's `Expr::Ref` can
never hit the cache at a member key, falls through to `aggregate_raw`, finds no
raw cell behind a formula target, and answers `Err("ref:…")`. A first version
did exactly that, and the two evaluators then disagreed on any routing formula
reading a *derived* measure: `Number(4.0)` uncached, `Error("ref:Profit")`
cached, which is the very divergence this section exists to remove (§3: "a rule
stated in one and not the other is a live bug").
**And making the lookup "work" would have been worse than the error.** The only
way to hit the cache from a member key is to strip `none_cats` off it — which
returns the *group's* value for that formula and hands it to every member. That
is aggregate-then-route again, laundered through a cache lookup, and it would
have been silent. The cache cannot answer a per-record question, so the fold
does not ask it. Only `Expr::Is` needed an arm in each evaluator, and since the
fold routes through `eval_expr`, the cached arm is now reached **only by a bare
`Is` outside a `SUM`** — which is what
`the_cached_evaluator_has_its_own_working_coordinate_predicate` pins.
The `Expr::Ref` fast path **stays**, and it is the same fold specialised: a
record's `Ref` is that record's own cell, so summing them is summing the
matching values. **Two of its premises were unstated and both were false**
(improvise-s08t, improvise-s2w3), which is worth recording as a shape: a
specialisation defended by an argument is only as good as the conditions the
argument assumed, and neither condition was being checked.
*The name must resolve.* The narrowing was written `if let Expr::Ref(name) =
inner && let Some(cat) = find_item_category(name)`, so a name that resolved to
nothing left the partial key **unconstrained** — `SUM(Typo)` aggregated every
numeric measure matching the context and answered a plausible total. A missing
identifier now fails before the fast path is reached, with the same `ref:{name}`
the record-level evaluator uses.
*The name must be stored.* A formula target is a `_Measure` item that nothing
wrote (improvise-nrs), so there are no raw cells behind it and the raw scan
returns nothing: `SUM(Double)` answered `0` where `SUM(Double + 0)` folded and
answered `20`. The premise of the specialisation — "a record's `Ref` is that
record's own cell" — is exactly what fails there, so the answer is to ask
whether it holds rather than to delete the path. `formula_for` is the one home
of "is this cell derived", so the check is that call and not a second
derivation; a derived ref falls into the fold, where `eval_expr`'s own `Ref`
arm routes through `evaluate_depth` and sees the formula. A target that also has
stored cells reads as the formula, because `formula_for` wins there too.
It differs in exactly one place — a record with no such cell is
*skipped* there and an **error** in the fold. That is deliberate: a bare
`SUM(Revenue)` asks for the values that exist, while `SUM(<expr>)` asks the
expression of each member, and an expression that cannot be evaluated is an
error rather than a zero. Silently dropping members is how a routed sum would
quietly answer for a subset.
The asymmetry's **blast radius** is wider than it sounds, and worth stating:
gotcha 7 makes a pipe-quoted text cell a first-class `.improv` value, so **one**
text cell anywhere in a member set turns the whole aggregated fitted cell into
`Error("ref:Cost")`, where the bare-ref path would have dropped it and returned
a number. That is the loud failure this repo prefers to a quietly wrong sum, but
it is a real behaviour difference and not a detail.
Records come out in `DataStore` insertion order, deduplicated by an `IndexSet`,
for §8's reason — hash order here would make a fitted cell drift in its last
bits between processes.
**Interaction with the fixed point (§3) and `formula_cache`.** None of this is
new state. The fold reads *raw* data through the same `evaluate_depth` /
`aggregate_raw` the non-folding path uses, so a formula cell still depends only
on raw cells and on other formula cells' cached values; `recompute_formulas`
iterates the same stems to the same fixed point, and `formula_cache` still holds
exactly the aggregated formula cells it held before. The fold does not cache and
does not memoise: it is O(members) per evaluation per pass.
**Measured**, because the cost was a blocker and not a footnote
(improvise-9iyv; `crates/improvise-core/examples/fold_bench.rs`, five runs per
point, best–worst). At 18,050 cells with one routing formula and `Tag` on
`Axis::None`, `recompute_formulas` takes **18.1–19.2 ms** release and
132.9–135.1 ms debug, against 7.1–9.0 ms / 71.7–74.6 ms for a linear
`Rev - Cost`. That is **2.3×** a linear formula and **linear in cells**: 400 →
0.4 ms, 2,000 → 2.0 ms, 18,050 → 18.1 ms, so 45× the cells costs 45× the time.
It was not always linear. Routing the fold through `eval_expr_cached` — the
first version, above — made every member's `Ref` miss the cache and fall into
`aggregate_raw`, one poorly-narrowed scan **per member**: 51.1–53.3 ms at the
same 18,050 cells and superlinear with it (45× the cells cost 85× the time).
So the divergence and the stall were **one defect**, and fixing the first fixed
the second.
**And "18,050 cells" was not a sufficient description of that workload.** At the
same cell count the old code cost 51 ms at a 25×361 shape and 244 ms at 95×95 —
a 4.8× spread from group *count* alone, which is what a per-member scan predicts
and a constant factor does not. After the fix the two shapes are 16.9–17.2 ms
and 16.4–17.5 ms: indistinguishable. **The shape-dependence being gone is the
confirmation of the cause**, more than the absolute number is. Re-measuring this
means stating the shape, not just the cells.
The fixed point runs exactly **2** passes (measured, not assumed), so hoisting
the member set out of the loop has a 2× ceiling on the fold's share and was
declined: a memo is state, and a memo keyed by anything less than the whole
`partial` is a second answer to "what does this cell depend on".
One asymmetry not to tidy: the cached path hands `eval_expr` a fresh
`MAX_EVAL_DEPTH` at each `Agg`, while the uncached `Agg` threads its own `depth`
through. The second is what bounds a chain of folded formulas; making them agree
by resetting both would unbound it.
**`Expr::Is(Filter)` — a coordinate predicate as a number.** `Cat IS Item` is
`1.0` when the cell's own `Cat` coordinate is `Item` and `0.0` otherwise. It
keeps the evaluator `f64`-only — no string comparison enters `eval_expr` — and
it reuses the primitive `Formula::filter` already had, `context.get(&cat)`,
including its `unwrap_or(false)`: a cell with **no** `Cat` coordinate is not
that item. Under route-then-aggregate that arm is reached only where a record
genuinely lacks the coordinate, because the fold hands the expression a record
key. Evaluated bare, outside a `SUM`, it degrades exactly as `Formula::filter`
does — which is why a fitted tree is always emitted wrapped.
## 14. A collapsed group is one row, and its cell folds the group (improvise-6qet)
**The ruling.** "When collapsing a group, I'd generally expect the cursor to move
to whichever place the group collapsed into" (user, 2026-09-23). The shape chosen
from a presented fork: **a collapsed group contributes one data row, whose cell is
an aggregate over the group.** Before this, `expand_category` emitted the group's
`GroupHeader` and then `continue`d past every item, so a collapsed group had no
data row at all. `row_group_for` is indexed by data row, so nothing on the grid
could name the group once it was collapsed: `z` pressed twice collapsed the
*neighbour*, and a collapsed group could be reopened only by `:toggle-group`.
The fix is to emit **one** `DataItem` where the `continue` was, and every
cursor-addressed question then answers itself by arithmetic. With
`Q1={Jan,Feb}, Q2={Apr,May}` and the cursor on `Apr` (row 2), collapsing Q2 gives
rows `[Jan, Feb, Q2]`, so row 2 *is* Q2 and a second `z` names Q2 again. `z z` is
an involution without anything being taught about involution, and neither
`ViewCursor` nor `row_group_for` changed.
### The row
- **Its tuple is the group name** (`Month=Q1`). `AxisEntry::DataItem` became a
struct variant `{ tuple, collapsed }` rather than growing a second variant,
because a new variant would be silently missed by `matches!(e, DataItem(_))`
sites; changing the shape made the compiler list all six external matches.
- **`collapsed` cannot be derived from the tuple**: a group and an item may
legally share a name. A parallel `Vec<bool>` was rejected for the reason gotcha
1 gives `prune_axis_items` a predicate — pruning shifts indices.
- **The flag is inherited** down `cross_product`, so a category *nested under* a
collapsed group (`Q1/North`) is also a fold: no record carries `Month=Q1`
whichever inner coordinate sits beside it.
- **A group's items need not be contiguous** (`A[G1], B[G2], C[G1]` is legal), so
`expand_category` keeps a `folded` list: a collapsed non-contiguous group folds
**once**, and its header is not re-emitted on the repeat — that header would
open a run with no row in it.
### The cell
`Model::evaluate_covering` expands, at its head, any coordinate that names a group
rather than an item into one key per covered member (a cross product where
several coordinates do). The grid reads every pivot key through
`GridLayout::evaluate`, which passes the members it draws. Since
improvise-eccl/f2oo, display text, pruning, column totals, `grid_of`, search,
yank and CSV export all go through that one evaluation, rather than each calling
`Model::evaluate_aggregated`. That remains the view-less answer, and it covers
every member. Five decisions:
- **Membership is read off each item's own `group` field, never off
`Category::groups`.** `add_item_in_group` registers no `Group`; that list is
filled only by `.improv` `>` lines. Asking it answers "not a group" for every
document built in code, and the symptom is the fold's row pruned as empty.
- **An item with the same name as the group wins.** `items.contains_key`
short-circuits the expansion, so an existing document with an item `Q1` reads
exactly as before.
- **Stored values fold once over every record the group covers.** Folding the
members' *aggregates* is right for `Sum` and wrong for `Avg`/`Count` under
`Axis::None`: `avg(avg(2,4), 9) = 6` where the group's average is `5`.
- **A formula is evaluated over the fold, never folded itself** (improvise-eccl).
This bullet used to say a formula target "has no stored records to pool", so
its members' results were folded with the measure's aggregation, and it called
that "the usual pivot-table limitation". **The premise was wrong.** A formula's
*inputs* are stored and can be pooled, and the model already evaluated a
formula over pooled inputs whenever a category sat on `Axis::None`:
`recompute_formulas` strips the None categories from the stems. Folding the
members' results summed ratios instead, so `Margin = (Revenue - Cost) / Revenue`
at 0.5 and 0.9 showed **1.4** on the Q1 row where Month-on-None showed **0.8**.
Spreadsheet calculated fields do not have that limitation either. The formula
is now evaluated at the group key, with each reference read through the fold
(`Reach::Fold` in `model/types.rs`):
- a raw measure reads the stored fold;
- another formula target is evaluated over the fold in turn;
- an aggregate inside the formula (`SUM`, `MAX`, …) pools every covered
member's records. `MAX(Revenue)` had been *summing* the members' maxima.
The collapsed row now agrees with the None-axis answer. **Test with a ratio**:
a sum agrees under both rules, which is how this was missed.
- **The folded category reads as absent** (improvise-lnkb, the orchestrator's
ruling). `Month IS Q1` is false on the Q1 fold, and a formula's own
`WHERE Month = "Q1"` does not apply there. Both answers are what the None axis
gives, and what an inline `SUM(… WHERE Month = "Q1")` already gave. This
PR's first version said `IS` saw the group's name and called that "stated
rather than hidden". That made the fold row the one place a formula could
observe collapse state, breaking this section's own "fold = None over the
drawn members" rule. `Model::read_coordinate` is the one reading of a
coordinate by name at a fold.
- A formula with its own `WHERE Month = "Jan"` shows **empty** on the Q1 fold,
because the fold is not Jan. The None axis gives the same.
- **A fold evaluates each formula once** (improvise-o1y0). A reference to
another formula target is evaluated over the fold in turn, so without memory
a doubling chain re-evaluates exponentially. The first version did exactly
that: a chain of eight took 511 formula evaluations for one cell, and the
reviewer measured 9.83 s on an 18k-cell grid. `evaluate_covering` now carries
a memo for one evaluation, and a key met while still in progress is reported
`circular`. Measured on an Apple M4 Max in release, on 100 products × 12
months folded to quarters with the F0…F8 chain (4,000 cells): 0.94–1.04 s
without the memo, about 60 ms with it, and about 10 ms on gh/main. gh/main
was cheap because it read the members out of the formula cache, which is
what gave the wrong ratios. The remaining ~6× is each column's cell
re-deriving the chain below it (45 evaluations per row where 9 would do).
**The memo is now shared across one grid build** (improvise-h6z1).
`viewport::grid_body` takes one `GridLayout::evaluator` and asks it for
every cell and total, so a fold row's chain is evaluated 9 times, not 45.
`prune_empty` does the same (improvise-metf). A memo is valid only for the
model state it read. That is why it is made per build and never stored on
`GridLayout`, which outlives cell writes.
The memo is keyed by the fold key **and** the span's categories. Within one
grid build the span half cannot yet differ for one key, because a cell's key
always names the row categories and a total's never does. It is kept because
`evaluate_spanning_in` is public, and a caller sharing one memo across two
spans would otherwise read the wrong fold. No test can see it today.
**The aggregation and the covers are the memo's, not the call's**
(improvise-pntu). They are the same argument as the span, but they were
left as parameters. So one memo passed with covers=all and then with Feb
hidden returned the first fold's answer (5.0 where 2.0 was right). Widening
the key instead would have needed a closure's identity, and a closure has
none worth hashing. So `FoldMemo::new(agg, covers)` owns both, and
`evaluate_covering_in`/`evaluate_spanning_in` read them off the memo. A
mismatched pair can no longer be passed.
**A grid's evaluations cannot be handed another grid's memo either**
(improvise-opbo). `GridLayout::evaluate_in` and `column_total` used to take
any `&FoldMemo`. A memo built from a layout showing Feb, passed to a layout
hiding it, gave 5.0 where 2.0 was right: a consistent pair, only for a
different grid. `GridLayout::evaluator(model)` now returns a `GridEval` that
owns a memo its own layout built, with `cell` and `column_total` on it, and
the memo-taking methods are gone. `Model::evaluate_covering_in` and
`evaluate_spanning_in` still take a `&FoldMemo`, but a memo there carries
its own agg and covers, so the only mismatch left is a memo over a
different *model state*. That is the per-build rule above, unchanged.
- **A fold covers the members the grid draws** (improvise-f2oo). Hidden members
are not in it. So a fold is exactly the rows it replaces, the total of a
column's total does not move on `z`, and a group whose every member is hidden
has no row. That holds for every aggregator and for a formula column only
since improvise-wm20, because the strip used to **sum** the drawn rows. That
moved a Max measure's total on `z`, and a ratio's (PR #203's review measured
Margin 1.90 expanded and 1.30 collapsed). A total is now the column's cell as
if every drawn row were one fold, so collapsing a group changes nothing it
reads. The fold's machinery is shared through `Model::evaluate_spanning`: its
`Span` adds, to the groups a key names, the categories a key **omits**, which
it expands to their covered items. That last
point was already true, and it contradicted the old rule, under which a hidden
value appeared only while a sibling was visible. This deliberately **differs
from `Axis::None`**, where hidden items still count, because the two replace
different things. A fold replaces *rows the user sees*. A None axis replaces a
*dimension*, and hiding an item on it hides nothing that was drawn. The view's
hidden set reaches the model as the `covers` predicate `GridLayout::evaluate`
passes, since the model knows no view.
### Settability and drill
The row is not a record, so `project.rs::cell_id` answers
`CellId::Aggregate` when `GridLayout::folds_a_group(row, col)` — the one home of
that question — whatever is on `Axis::None`. Without it, a document with nothing
regular on None projected the fold as `CellId::Record`, offered it for editing,
and a `SetCell` would have written the coordinate `Month=Q1`, which is not an
item. `render.rs::editable` is untouched (ADR 002: the variant decides).
`EditOrDrill` asks the same predicate and declines out loud: drilling would filter
on `Month=Q1`, which no record carries, and show an empty list. Guarding that one
command left `>`, `p`, `H` and an `Enter` advanced onto the fold all still writing
or drilling there (improvise-gw18, yk2s). Every command that takes the cursor's
cell now goes through a door that asks this predicate, or asks `cell_id`, which
does: `context/code/command.md`, "The cursor's cell has three doors".
### The cursor, and why the order is the fix
`ToggleGroup` is `Model(LayoutGeometry)`, and `apply_effects` resets the cursor
when it observes the frame move *inside* the loop (gotcha 24). So
`ToggleGroupAtCursor` emits `ToggleGroup` **first** and the cursor placement
**after**; the swap is the rival, and it lands every toggle at `(0, 0)`. The
placement also restores the scroll offsets the reset zeroed.
Where the cursor lands is asked of the layout the toggle **will** produce
(`GridLayout::row_run_start(cat, group, prefix)` on a view with the toggle
applied), not of the current one. The same answer serves collapse and expand —
the group's first row — and it matters when the group is at an inner level:
collapsing `Month` under `Region` shrinks every earlier `Region`'s run too, so
`South/Apr` at row 6 lands on `South/Q2` at row 5, not row 6. That is the only
test the "ask the current layout" rival fails.
### What this does not do
- **Part 2 of the ruling** (a toggle carrying the group it acted on, so a later
command in the same batch can be invalidated) is not added: nothing binds `z`
as a `Binding::Sequence`, and if something did, both toggles would resolve
against one pre-batch context and name the same group, so the pair is a no-op —
which is what `z z` means. A mechanism with no reachable case is not built.
- **CSV export writes the fold as a row**, because an export writes what the grid
draws (improvise-iot6); a collapsed group is its own total row there too.
- Under `prune_empty` an **empty** collapsed group now disappears header and all,
where improvise-lhi4 kept its header. It is a narrowing, and deliberate: the
header was kept because it was the only way back to the rows, and the fold row
is now that way back — an empty fold has nothing to show.
- The browser still cannot press `z` (`toggle-group-under-cursor` resolves
`Upstream`), so the cursor half takes effect in the TUI only.
## `PageAxis` — what constrains, and what the user may turn (improvise-p1ym)
`GridLayout::page_coords` answers two questions that gotcha 27 says must not be
confused. The bar at the top of a view names **every** category constraining
the visible records, which is `View::filtering_cats()` — Page ∪ Filter — because
a drill fixes its coordinates on `Filter` and the user should still see them.
`[`/`]` cycle only what the user may act on, which is `categories_on(Axis::Page)`.
Those were two walks before: `GridLayout` did the first, `navigation.rs::
page_cat_data` did the second, and each spelled the Page-vs-Filter rule itself.
Two spellings of one rule is the shape design-principles §5 calls the disease,
and here it had already leaked into the protocol — `GridCache::page_coords` was
a verbatim copy of the layout's list, so a browser handed it would have tried to
page a Filter axis.
So `page_coords` is now `Vec<PageAxis>`, and `items` is `Some` exactly when the
axis is navigable. The rule lives in the *type*: a `Filter` axis carries no item
list, so no caller can compose a cycle for one, and `PageAxis::cycle()` is the
one place that pairs a list with the index its selection sits at. `page_cat_data`
and the browser's `page_cycle` both read it rather than deriving it.
Two consequences worth stating. The list is `Model::effective_item_names`, the
same call that already chose the default selection, so the selection and the
list it indexes into come from one place by construction — `page_cat_data` used
`ordered_item_names` before, which for a `_Measure` page axis meant the default
selection could be a formula target `[`/`]` could not reach. That incoherence is
gone; the cost is that `[`/`]` over a `_Measure` page axis now visits formula
targets, which is what the column list shows anyway.
And the list is **not** filtered to visible items. Hidden items are still cycled,
because that is what the TUI has always done, and making the browser differ would
be the divergence rather than the fix. Changing it is a change to both, together.
## The number format — `format.rs` (improvise-711n)
A view's `number_format` is a **printf-style picture, and only that**. A `,`
anywhere turns on thousands grouping; a `.` introduces the precision, which is
the **leading digit run after the last `.`**; anything following those digits —
a conversion letter — is ignored. So `,.0`, `,.2`, `.2`, `,.2f` and `%.2f` all
name what they look like they name.
**Why the trailing `f` is accepted rather than refused.** It round-trips into
documents today and always has. `improv.pest` spells the line
`format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines }` — the
grammar takes the format string as an **opaque line**, so `format: ,.2f` is a
document the grammar accepts; `:set-number-format` stores whatever the user
typed verbatim (`cmd/effect_cmds.rs`), and `format_md` writes `view.number_format`
back out unchanged. Refusing the spelling at load would therefore reject
documents this repo's own grammar admits and its own `.improv` example spells,
to no benefit: by gotcha 8 the format is display-only, so misreading it costs a
wrong pixel and can never cost a wrong number. A refusal here buys nothing and
costs a document.
**Why printf and not Excel.** The two conventions disagree on exactly the
spelling that distinguishes them: `.00` is *precision zero* to printf and *two
places* to Excel, so a parser that tried to honour both would have to guess. One
is picked, and it is the one every spelling in this repository already uses.
Consequences worth stating rather than discovering: `0.00` reads as
`(false, 0)`, and an Excel picture such as `#,##0.00` reads as
`(true, 0)` — grouped, no decimals.
**Nothing is validated, deliberately.** A spelling the rule cannot read names no
precision and formats with none; there is no error channel and no `Result`,
because the reader is display-only and four call sites would have to thread one.
**What the bug was, and why six tests sat on top of it.** The precision was read
as `fmt[i+1..].parse::<u8>()` over the *whole* tail, so `"2f"` failed to parse
and fell through to `unwrap_or(0)` — `,.2f` displayed **zero** decimals, in the
TUI, in the browser projection and in CSV export. `",.2f"` and its `,.3f`/`,.4f`
siblings are what this workspace's own fixtures reach for, in about ten tests,
and none could see it: they assert the stored *string* round-trips or use it as
an opaque value to dirty a widget, or — in `export_csv_quotes_commas_per_rfc4180`
— they assert the rendered value `starts_with("1,200")`, which `1,200` and
`1,200.00` both satisfy. A prefix check over a value whose *suffix* is the thing
in question answers a neighbouring question, which is why that one is now an
exact equality.
**Only two of the three consumers are guarded.** Reverting the fix fails
`format.rs`'s own unit tests and `export_csv_quotes_commas_per_rfc4180`, and
nothing else in the workspace: `ui/project.rs::grid_of` (the browser projection)
and `ui/app.rs` both call `parse_number_format` and neither has a test that
renders a cell and asserts its digits. That gap is named rather than closed —
those files are `improvise-command`, which was another wave's ground when this
landed — and it is the more useful half of this entry, because it says where the
next regression would go unseen.
**That gap is now closed, and the sentence above is kept as the record of it.**
`ui::project::cell_identity::a_formulas_format_reaches_its_own_column_and_no_other`
renders cells *and* the totals strip and asserts their digits (improvise-e0ex);
`ui/app.rs`'s own call is still unguarded, and now reaches the same `FormatPlan`
rather than a second `parse_number_format`, so there is one fewer place to drift.
## The decimal default and a formula's own format (improvise-e0ex)
`View::new` seeds `,.2`, not `,.0`. The symptom that forced it was
`Margin = Profit / Revenue` drawing as `1`: a 0.66 ratio under *zero* decimals,
rounded half-away-from-zero by `format_f64`. Three things worth stating, because
each was a candidate answer that is wrong.
**It is a formatting bug and not an arithmetic one.** Exact decimal arithmetic
(ADR 005) would not change the rendering by one character — `0.66` under `,.0`
is still `1` — so nothing here waits on it and nothing here touches the
evaluator. Gotcha 8 is unchanged: `format_f64` is called in rendering and
nowhere else, and full `f64` is what eval sees.
**The cost is paid on whole numbers, knowingly.** Revenue now draws
`18,400.00`. That is what an accounting application does, and it is the cheaper
error: a currency column with two redundant zeroes is *legible*, where a ratio
with none is **wrong**. A format cannot be guessed from a value — 0.66 is a
ratio and 0.66 is also two-thirds of a dollar — so the default must be the one
whose failure mode is noise rather than a false number.
**The override is spelled in the formula, not per `_Measure` item.** The user's
ruling ("unless the format is explicit in the formula") picks the narrower of
two nearly-coincident units: a formula *target* is a `_Measure` item (gotcha 7),
so per-formula and per-measure differ only for a measure no formula owns — and
such a measure is raw data, whose format is the view's job. So a formula may
carry its own format, which wins for its target; a raw measure keeps the view's.
### Where the format is resolved: `FormatPlan`
`FormatPlan::of(model, view)` is the one answer, and every render path takes it
where it used to take a `(comma, decimals)` pair: `GridLayout::display_text`,
`ui::viewport::grid_body`/`compute_col_widths`, `ui::project::grid_in` and
`persistence/export.rs::render_csv`. Replacing the pair rather than adding a
second argument beside it is the `row_levels`-next-to-`row_labels` rule from
design-principles §5 — two answers to "what format is this cell" is the disease.
**It is asked of the cell's `CellKey`, never of the axes.** A cell aggregating
over `_Measure` carries no target coordinate and so keeps the view's format,
which falls out of the key rather than needing a rule. Deriving it from "is
`_Measure` on a visible axis" would be a second derivation that could agree
with itself while disagreeing with the cells.
**The empty plan short-circuits**, so a document with no formula format pays one
`is_empty` per cell and nothing else; the lookup is otherwise a scan of the
formula list, which is short and must stay ordered for the reason below.
**The owner is the formula `Model::formula_for` names, and nothing else.**
`FormatPlan` holds every formula's `(target_category, target, Option<format>)`
in the model's own order and takes the **first** coordinate match, which is the
same rule and the same order evaluation uses to decide whose value the cell is.
Two things that shape enforces, both of which a `HashMap` keyed by target got
wrong and review caught (Codex, PR #125):
- **A key may be claimed by two formulas**, because a target category need not
be `_Measure` — `- Tax = Revenue * 0.1 [CustomCat]` is in the format's own
documentation. Iterating a `HashMap` answered from whichever category hashed
first, so the *displayed* format was nondeterministic per process while the
*value* was not. That is gotcha 18 exactly: an index whose values are hashed
is for lookup, never for choosing between candidates.
- **An owner that names no format still owns the cell.** Indexing only the
formatted formulas let a later formula's `FORMAT` reach a cell whose value it
does not produce. So the list holds *every* formula and a match with
`format: None` falls through to the view — it is a match, not a miss.
The list is dropped entirely when no formula names a format, so the common
document pays one `is_empty` per cell rather than a scan.
**A column total is resolved from the column, never from a row.**
`GridLayout::column_key(col)` is the page/filter constraints plus the column's
own tuple — the coordinates that survive the row aggregation — and `GridBody`
carries the resulting `col_formats` so the strip and the cells cannot disagree.
Reading it off row 0 was the first version and is wrong twice over, also found
by review: a formula may target a **row-axis** item (its target category is any
category), so the total inherited a row-local override for a row it had summed
away, **and** re-ordering the rows changed the total's spelling. The guard is
`a_column_total_does_not_inherit_a_row_local_format`, which builds the same grid
with the owned row first and last and requires the two strips to agree.
### The grammar: `FORMAT <spec>` in the formula, not in the `.improv` line
`formula.pest` gains `format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }` after
the optional `WHERE`. **`improv.pest` is unchanged**, and that is the design
rather than an omission: `formula_raw` already takes the `## Formulas` line
verbatim up to the optional `[Category]` suffix, so a format inside the formula
*text* is carried by the existing grammar with nothing to add. Two consequences.
The grammar-walking generator (which reads `improv.pest`, not `formula.pest`) is
untouched, so there is no new spelling for it to violate. And the same text a
user types into the TUI formula minibuffer is the text the file carries — one
namespace, one spelling rule, exactly as gotcha 34 says of names.
`sp+` rather than a `!bare_char` lookahead, for `WHERE`'s reason (gotcha 34):
a predicate is invisible to a generator. `FORMAT` joins `quote::KEYWORDS`, so an
item literally named `FORMAT` is written `|FORMAT|` — the keyword guard is
canonical and this is one more entry in it, not a new rule.
**`format_spec` is `(ASCII_ALPHANUMERIC | "," | "." | "%")+`** — a bounded token
where the view's `format:` line is `rest_of_line`. The line can afford to be
opaque because a newline ends it; a formula clause cannot, because something may
follow. The set covers every spelling `parse_number_format` can read (`,.2f`,
`.4`, `%.2f`) and nothing the tokenizer would have to disambiguate.
### The trap this feature sets, and what is asserted instead
`format_md` writes `Formula::raw`, so a format inside the raw text round-trips
**whether or not the walker parsed it into a field**. That is gotcha 34's bug
exactly — a target that kept its pipes survived every round-trip test because
`raw` was unaffected. So the round-trip test asserts `Formula::format`, the
parsed field, and its `raw` assertion is there to *show* that `raw` cannot be
the witness. Injecting the fault (a walker that drops the clause) fails that one
test and **nothing else in `improvise-io`** — 242 other tests, including the
whole `format_md`↔`parse_md` characterization suite and the grammar-walking
properties, pass. That is the measurement, not an argument.
### What persistence does and does not do here
The user's constraint — "should not affect persistence at all, except to store
format metadata" — holds by construction and the job was to not break it.
`format_f64` is display; `persistence/format.rs::format_number` is a different
function in a different crate writing Rust's shortest round-trip form. **The
regression to watch for is routing a save through the display formatter** so the
file matches what the user sees: a model has many views (gotcha 29), so there is
no single right answer and the last save would win. Gotcha 8 forbids feeding a
rounded value back into *eval*; this is its persistence twin.
What did change on disk is metadata only: newly created views write
`format: ,.2` where they wrote `format: ,.0`, and a formula line may now carry
a `FORMAT` suffix. Existing documents are unaffected — `format_md` has always
written the `format:` line explicitly whenever `number_format` is non-empty and
`View::new`'s value has never been empty, so every document improvise has ever
written carries its own and no migration exists to get wrong.
One consumer that is display but reads like persistence: **CSV export** goes
through `display_text` and therefore through the view's format, as it always
has. A document with no explicit `format:` line now exports two decimals. That
is pre-existing behaviour meeting a new default, not a new coupling.
## 16. A non-finite cell value has one spelling, and the format chose it (improvise-55v2)
`serde_json` writes a non-finite `f64` as `null`, and `null` does not read back
as one. So `CellValue::Number(f64::INFINITY)` crossed the wire as
`{"Number":null}` and the frame carrying it was **unreadable**: the websocket
reader skips an unparseable frame and the worker logs a console error, so one
poisoned cell silently dropped a whole batch. `to_string` returned `Ok`, so the
send site could not notice either.
The ticket offered two answers — an explicit representation, or a refusal before
sending — and **the `.improv` format had already chosen**. It carries non-finite
numbers as first-class values: `format_number` writes `inf`, `-inf` and `nan`,
the grammar's `bare_value` accepts them, and the walker maps them back. All
three round-trip losslessly, measured before anything was changed. A refusal
would therefore make a legal saved document unprojectable to a browser, which is
a new failure rather than a fix. It also settles the second location the ticket
named: `resolve.rs`'s `typed()` accepting `inf` is correct, not half a defect.
So `Number` serializes through an untagged adaptor — a JSON number when finite,
one of the three tokens when not. A finite cell's bytes are unchanged, which is
the whole of the compatibility story, and a JSON integer still reads as a number
rather than being taken for a token.
**The tokens are one home**: `nonfinite_token` / `parse_nonfinite` beside
`CellValue`, called by the adaptor, by `persistence/format.rs` and by
`persistence/parse.rs`. Three copies stood before and a fourth was about to be
written, which is the shape gotcha 34 named for pipe quoting. `format_f64` is
deliberately **not** folded in: it is the display layer (gotcha 8) and may want a
different glyph one day, where these three are a *format*.
What makes this checkable rather than asserted is that the guard compares the
**two carriers to each other** —
`a_nonfinite_cell_is_spelled_the_same_on_disk_and_on_the_wire` reads the token
off the `## Data` line and off `serde_json` and requires them equal. A second
home that happens to agree is harmless; one that drifts fails there, and
re-copying the tokens into `format.rs` with one letter changed was injected to
prove it does.
## 17. An editor opens on the value, not on its `Display` (improvise-umi4)
`CellValue`'s `Display` is a **screen** spelling: a non-whole number is written
`{:.4}`, so `1.23456789` reads `1.2346`. Every editor seed used to be that
string (`display_value_in`'s pivot arm through `get_cell().to_string()`, its
records arm through `records_display`, and `project::cell_display`'s records
arm spelling it again), so pressing `i` and then `Enter` with nothing typed
stored `1.2346`. The screen looked the same before and after, which is what
made it silent data loss rather than a visible bug. The browser seeds from the
projected `CellDisplay::edit_text`, which came from the same call, so it had
the same bug.
`CellValue::edit_text` is the one home of the seed. A number is written as the
**shortest decimal that parses back to the same `f64`**, which is what Rust's
`f64` `Display` already guarantees (no precision argument, no exponent). The
brief pointed at improvise-gp's `number_text` as the implementation to reuse.
It is not a second implementation of this one: it takes an `f32` and searches
fixed precisions for the shortest *f64* expansion of it, because a threshold
must route the same under f32 and f64 evaluation. For an `f64` the standard
library's spelling is the same answer with no search. A non-finite number takes
§16's token (`inf`, `-inf`, `nan`), which `typed()` also reads back, so the
editor and the `.improv` format spell it one way.
Text and errors keep `Display`. Text is written unchanged, and an error cell has
no edit text on the wire anyway. Display itself is untouched: the grid still
rounds (gotcha 8), only the seed changed.
What this does **not** fix: a text cell whose text reads as a number. `"007"`
seeds `"007"`, and `typed()` classifies that as `Number(7.0)`. The seed is
exact, but the classifier has no way to know the cell was text. That is
improvise-ixxx.
## 18. Which formula an edit means (improvise-km71)
A `Formula` has no stored id. It lives in `Model::formulas: Vec<Formula>` and is
identified, in practice and by `add_formula`'s upsert, by `(target,
target_category)`. **`FormulaId` is that pair behind one opaque type**: its
fields are private, `Formula::id()` is its only constructor, and nothing
outside `improvise-formula` can read a name out of it. That is deliberate. The
stable-identity work (improvise-sboz, ADR 007) will make a formula's identity
something other than its names, and every signature that already says
`FormulaId` — `Model::formula`, `Model::replace_formula`,
`ModelCommand::ReplaceFormula`, `ModelCommand::RemoveFormula` (since improvise-tej1),
`Model::delete_formula`, `FormulaSlot::Existing`, `FormulaEntry` — then
changes by changing this one struct. A name pair spread across those signatures
would have made the swap a sweep.
**The index is not a candidate.** A position is what `formula_cursor` is, and
the editor must not recompute its target from the cursor at commit
(improvise-ixxx/2wu3's lesson): another client deleting an earlier formula
shifts every index after it, and the commit would land on a neighbour. It would
also put a position on the wire, which `every_model_command_field_is_position_free`
makes a compile error.
**An edit is its own operation, never remove-then-add and never
`add_formula`.** `add_formula` upserts on `(target, target_category)`: fed an
edit that changes the target, it appends a second formula and leaves the old
one; fed an edit onto another formula's target, it silently replaces *that*
formula. `Model::replace_formula(id, formula)` writes into the slot `id` names,
so the position (and with it `_Measure`'s item order, since
`measure_item_names` appends formula targets in formula order) is kept, and it
**refuses** when the new target belongs to a different formula.
**A target change is a rename of the same formula.** The id travels with the
edit, so `Profit = …` edited to `Gross = …` is the formula that was `Profit`,
in the same place. That is the least surprising reading of "edit this row", and
it is what ADR 007 proposes. What a rename cannot carry under name identity is
everything else that names the old target: other formulas' references, a
view's hidden list, a `measure_agg` entry. Those cannot follow without rewriting
`raw` text the parser does not print back, so the edit says so out loud:
`ReplaceFormula` reports which formulas still mention the old name
(`Formula::mentions`, over the expression and its `WHERE`), and their cells
show the `ref` error rather than a silent wrong number. Under a stable id those
references are what sboz makes follow. **This holds for `_Measure` targets only.**
Outside `_Measure` a target is a real item, and a rename is refused until sboz
(improvise-kb1d, `context/code/command.md`). A rename also leaves stored cells under
the old `_Measure` name in `## Data`, invisible because the formula shadowed them.
**Concurrency is checked twice.** `FormulaSlot::Existing { id, was }` is
captured when the editor opens: the id must still resolve, and the formula must
still read `was`. Either failing means another client removed, renamed or
rewrote it meanwhile, and the edit is refused with the batch stopped, so the
editor stays open over the user's text (gotcha 21). `was` is not redundant
with a stable id: a concurrent *replace* keeps the id and changes the text,
which only `was` can see.
## 19. Group hierarchies: parents nest runs, and every group starts closed (improvise-felo.6)
`Group::parent` was parsed (`> Q1[2024]`), written, round-tripped and hashed,
and nothing read it: `Q1` was never drawn inside `2024`. It is now the
hierarchy.
### The user's rulings
- "I think hierarchies should start closed unless we somehow opt into a
specific one opened to a certain depth (view config maybe?)" — and the
reason: "in most cases, the closed by default start is preferable because it
makes the grid more dense."
- "Everything opens closed, including old files." There is no version gate: a
file written before this change opens with its groups closed, and its
`collapsed:` lines simply agree with the default.
- "I think we'll need a depth for expanded, something like expanded(2): ...".
- "expanded+collapsed in the same group is an error".
### The model
- **A group's chain** is the group and its ancestors, root first
(`Category::group_chain`). An item's chain is its group's
(`Category::item_chain`); an ungrouped item has none. A group's depth is its
number of ancestors.
- **A group covers every leaf beneath it.** `items_in_group` reads each item's
chain, so `2024` covers the months of `Q1-2024` and `Q2-2024`. That is the
whole change to evaluation: `evaluate_covering` already expanded a group
coordinate through `items_in_group` (§14), so a year's fold is the fold over
its months with no second path.
- **An item may sit directly in a parent group, beside its child groups.**
Chosen because nothing is simpler: the chain handles it (`Jun[H1]` after a
run of `Q2` inside `H1` ends the `Q2` run and stays in `H1`), and refusing it
would be a rule with nothing to protect. A date import never produces one.
- **A cycle is refused on load, naming its members** (`Refusal::HierarchyCycle`,
`persistence/parse.rs`). A cycle built in code cannot hang anything either:
`group_chain` stops where it would repeat.
- **There is no such thing as a dangling parent in this format.** A root group
is declared *only* by being named as a parent: the writer emits `>` lines
only for groups that have a parent, and nothing else can name `2024`. So
"a parent that names no group" and "a root" are the same line, and refusing
one refuses the other.
### Which groups are open
State lives on the `View`, in two places that cannot disagree about one group:
- `expand_depth: cat → N` — `expanded(N): Cat`, the category's top N levels
open.
- `group_folds: cat → group → GroupFold` — ONE map, so a group has at most one
fold. `GroupFold::Collapsed` is `collapsed: Cat/Group`;
`GroupFold::Expanded(N)` is `expanded(N): Cat/Group`, the group open and
N-1 levels beneath it. Two sets would have made "both" representable, which
is the state the user ruled an error
(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`).
`View::is_group_collapsed(cat, chain)` answers, in order:
1. the group's own fold, if it has one — so a `collapsed:` group inside an
`expanded(N)` region stays collapsed, and an `expanded:` group below the
category's depth opens;
2. open, if an ancestor's `Expanded(n)` reaches it (the ancestor `k` levels up
reaches it when `k < n`), or the category's depth does (`depth < N`);
3. closed.
**`z` writes one fold, or none.** `toggle_group_collapse` flips the answer and
records the flip as `Collapsed` or `Expanded(1)` — unless the flip lands on
what the group would inherit from rule 2, in which case it removes the group's
fold. So `z z` leaves the view as it was, and no sequence of toggles writes a
file that names one group twice
(`no_sequence_of_toggles_writes_a_conflicting_file`). Closing a group that
carried `Expanded(3)` drops the 3: reopening it with `z` opens one level.
### The layout
`expand_category` walks each drawn item's chain. The **outermost** collapsed
group in the chain folds everything beneath it: the row's tuple is that group's
name, it folds once however non-contiguous its members are (§14), and the
headers above it are drawn. The run markers move from item to item exactly as
`remark` moves them after pruning — past the shared prefix of the two chains,
end the deeper runs innermost-out, then open what differs — so an unpruned axis
survives `prune_axis_items` marker for marker, and the enumeration in
`group_membership` (now with a `NESTED` fixture: a non-contiguous quarter, an
item directly in a half-year, an outer grouped category) asserts it.
A header carries its own `collapsed`, computed once in `expand_category`, so
the frame projection and the frame hash read it rather than asking the view
again with a chain they would have to rebuild.
### Persistence
`expanded(N): Cat`, `expanded(N): Cat/Group`, bare `expanded:` for N = 1, and
the existing `collapsed: Cat/Group` (`improv.pest`, `expanded_line`). N is at
least 1 by the grammar (`'1'..'9' ~ ASCII_DIGIT*`), so `expanded(0)` is a parse
error rather than a value to explain. Two further decisions:
- **The writer keeps what the view holds.** An old file's `collapsed:` lines
load as `Collapsed` folds and are written back verbatim, though they agree
with the default. They are the user's explicit closes, and they survive a
later `expanded(N): Cat` that would otherwise open those groups; dropping
them on save would change what that later line means. A `z` round trip
removes such a line, because `z` normalises (above).
- **A category depth is never written bare.** `axis_line` is tried before
`expanded_line`, so a category literally named `expanded` keeps its
`expanded: row`. The price is that `expanded: row` meaning "category `row`,
depth 1" would read as that axis line, so the writer always spells the
category form `expanded(N): Cat`. The group form has a `/` and is never
ambiguous.
A view naming one group with two different folds, or one category with two
depths, is refused on load, naming the view and the group
(`Refusal::FoldConflict`). The same line twice is not a conflict.
The grammar-walking generator emits cycles and conflicts with ordinary
probability (`> ||[||]` is a self-parent), so `generated_file_parses` accepts a
`Refusal` — a typed error, so it cannot accept a parse failure by accident.
### Known limits
- The TUI draws nested headers exactly as it draws any other: one line per
header, not indented by depth, and the column band shows each column's
innermost group only. Both are pre-existing presentation choices; the frame
carries `depth` for the day a front end wants to indent.
- Two `>` lines giving one group two different parents: the last wins, as it
did before this change. That makes the hierarchy a function of line order.