docs: complete the corrections, per adversarial review of this PR
Four findings, all verified against the tree before acting.
F10 — this PR's own edit to design-principles §5 made a sibling note in
context/code/command.md false: it said §5 "still names Sessions::apply" and
recommended apply_checked. A PR whose whole subject is stale prose had left a
new stale copy behind, which is gotcha 48 exactly ("changing it is a grep, not
an edit"). Note deleted; the verified claim above it stands.
F11 — §5 named both apply and apply_checked as "the one door". apply has no
production caller anywhere in the workspace; Host calls apply_checked. Naming
two weakens the claim §5 makes, so it now names the one production uses.
F12 — gotcha 17 said "four call sites". There are six uses across the two
evaluators: a div/0 guard plus the Eq and Ne arms in each. In a file agents
follow literally, a reader who greps and finds six cannot tell whether the
prose or the code moved.
F13 — gotcha 17's sibling said the Dirty::of doc comment "keeps only the
one-line reason LayoutGeometry is spelled out rather than EVERYTHING". It says
no such thing: it points at context/code/command.md, and the EVERYTHING reason
is a two-line comment inside the match body. A correction to prose that
asserted the negation of the code should not itself misdescribe the code.
Two findings deliberately not fixed here and filed instead: improvise-ahdf
(repo-map gotchas 36 and 43 carry the same stale Sessions::apply claim —
pre-existing, and inside long paragraphs that need reading rather than a sed)
and improvise-rm4s (--check-evidence accepts headings naming tests that never
run).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+2
-2
@@ -175,7 +175,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
14. **Keybindings are per-mode.** `ModeKey::from_app_mode()` resolves the mode; Normal + `search_mode=true` maps to `SearchMode`. Minibuffer modes bind Enter/Esc as `Binding::Sequence` so `clear-buffer` fires alongside commit/cancel. Hints are checked against the keymaps: `every_hint_key_resolves_in_its_mode` (`command/keymap.rs`) splits each `AppMode::hint_text()` and requires every key it names to resolve in that mode's map, because `only_quit_has_no_hint` asserts only that a hint is *present*. It checks which keys are named, not what they do. Its tokenizer knows the hint dialect explicitly (named keys, `Ctrl-<c>`, `F/C/V` alternatives, character runs like `hjkl`/`[:]`, the em-dash prose tail, CommandMode's `:q` ex-commands) and panics on a token it cannot classify rather than skipping it.
|
||||
15. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — there is no name literal in `registry.rs`. What types can't reach: four commands name themselves off a field (`ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side, and checks **both** halves of `dispatch`'s `registry.interactive(name, args, ctx).ok()?` — the name resolves *and* the args parse — because improvise-s13 moved `enter-mode ["export-prompt"]`/`["formula-edit"]` out of the nullary class, where a typo'd mode name compiles into a dead key. It builds the `CmdContext` in each keymap's own `AppMode`, since `commit-buffer item` reads `ctx.mode`.
|
||||
16. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic. Its counterpart on the effect side is `simple_effect!` in `ui/effect.rs`, which declares a one-field newtype that writes one place on the `App`. Its **kind argument is mandatory** — `simple_effect!(Name(T) => |app| place, EffectKind::ViewOnly)` — even though every call site so far answers `ViewOnly`: hard-coding it reintroduced exactly the default gotcha 3 does without, since `$place` is an unconstrained expression and a one-line `active_view_mut().number_format` setter would have reported `ViewOnly` with the suite green (improvise-8fy). Likewise, mode entry has no per-mode shim commands — bind `enter-mode <mode-name>` (improvise-s13); the name is decoded by `parse_mode_name` in `registry.rs`, and `EnterMode` clears the mode's minibuffer on the way in.
|
||||
17. **Float equality inside formula eval has one epsilon, and the div/0 guard shares it.** `FLOAT_EQ_EPSILON = 1e-10` in `model/types.rs` is used by `=`/`!=` **and** by the division-by-zero guard (`rv.abs() < FLOAT_EQ_EPSILON`), in both `eval_expr` and its aggregated twin — four call sites, one constant. This gotcha previously recorded the opposite (`=`/`!=` epsilon, div/0 exact) and was stale; the mix is gone. Why an epsilon at all and why this one: `context/code/core.md` §4.
|
||||
17. **Float equality inside formula eval has one epsilon, and the div/0 guard shares it.** `FLOAT_EQ_EPSILON = 1e-10` in `model/types.rs` is used by `=`/`!=` **and** by the division-by-zero guard (`rv.abs() < FLOAT_EQ_EPSILON`), in both `eval_expr` and its aggregated twin — six uses across the two evaluators (a div/0 guard plus the `Eq` and `Ne` arms in each), one constant. This gotcha previously recorded the opposite (`=`/`!=` epsilon, div/0 exact) and was stale; the mix is gone. Why an epsilon at all and why this one: `context/code/core.md` §4.
|
||||
18. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this — and so, since improvise-shyt, do `DataStore::matching_cells`/`matching_values`: the secondary index **narrows** the candidates, then `matching_positions` resolves each to its position in the primary `IndexMap`, sorts, and reads back. **An index whose values are `HashSet`s is for O(1) lookup, never for iteration.** Walking it gave a different order in every process, which was not tidiness. Records-mode rows reordered between sessions; `SUM`/`AVG` drifted in the last bits because `f64` addition is not associative (`Min`/`Max`/`Count` and the text-unanimity rule of gotcha 7 are order-free, so they were never affected); and worst, the **drill snapshot** froze its record list in hash order while `drill_state.pending_edits` are keyed by *row index* (gotcha 1), so the same drill opened in a second process addressed **different records** — silent misdirected writes, and nothing in the tracker knew. Measured at 18,000 cells: +20% worst case on `matching_values`, and `matching_cells` 7% *faster*, sorted positions resolving `CellKey`s with better locality. `DataStore::live_records` was the open choice and is now decided (improvise-2yc2): it walks `cells` and projects insertion order onto records, deliberately paying 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. 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, which is the guard living one level away from the hazard and the shape this whole class of bug returns through. **The second real instance was `import::analyzer::analyze_records`** (improvise-7yx), which built its distinct values through a `HashSet`: `build_model` adds items in exactly that order and `IndexMap` preserves it, so the *same CSV imported twice* produced two different `.improv` files and two different row orders. Now first-seen order, pinned by a five-iteration test because each `HashSet` gets its own `RandomState`. Also answered here since: an **empty `partial`** matches every cell vacuously (improvise-hkt6) — it used to index `sets[0]` and panic, unreachable only because both callers guarded.
|
||||
19. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior.
|
||||
20. **Minibuffer commits go through one command.** `commit-buffer <formula|category|item>` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line.
|
||||
@@ -205,7 +205,7 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
|
||||
34. **A formula target is unquoted, and pipe quoting has one home.** `parse_formula` strips the pipes off a target exactly as it does off an `Expr::Ref` — the target names a `_Measure` item, so a target that kept its delimiters made `add_formula` create an item literally named `|Total Revenue|`: a ghost column the user's real item never matched, and `Expr::Ref("Total Revenue")` never resolved to. Both the TUI formula minibuffer (`AddFormula`) and `.improv` load go through that one call, so both were affected. The writer emits `Formula::raw`, not `target`, so file text is unaffected either way — which is exactly why the round-trip tests never saw it, and why the fix repairs existing files on next load with no migration. Pipe quoting has exactly **one home**: `improvise_formula::quote`, owning both halves — `quote_name`/`is_bare_name`, `pipe_quote` (unconditional, for a `.improv` text cell that must not read back as a number) and `unescape_pipe_body`. `improvise-gp` re-exports it, `improvise-io` calls it from `persistence/{format,parse}.rs`, `parser.rs::unquote_pipe` is a shim over it, and io's own copy is **deleted** (improvise-i2m5). **The keyword guard is canonical — user-confirmed, so do not revert it as over-caution**: an item literally named `SUM` is written `|SUM|`. Bare keywords do parse, but only because `agg_call` needs a `(` and PEG backtracking falls through to `ref_expr` — rule ordering, not a stated rule — and a `## Formulas` line carries formula text verbatim, so a name `format_md` writes is read back by `parse_formula`: one namespace, one spelling rule. One thing the guard does not cover, and one that used to be an exception and no longer is. **A section header takes its name as the whole line, totally** (improvise-pczv): `header_name = { pipe_quoted ~ &NEWLINE | line_name }` — the pipe-quoted arm needs the lookahead, or `|A|B` would parse as `A` and silently drop `B`, and `line_name` catches everything else, so a header name can no longer fail to parse. Four rules share it (`model_name`, `initial_view`, `category_section`, `view_section`), which is why it is a rule and not four copies. Note a header is still *written* raw where a reference is quoted — the grammar accepts both arms, so the quoting asymmetry between a header and a reference is a writer convention, not a format one. And the two former *unquoting* copies differed only on a trailing lone backslash, unreachable from the grammar, where io returned it and `parser.rs` panicked — the surviving `unescape_pipe_body` is total.
|
||||
|
||||
35. **`Dirty` over-reports on purpose, and the asymmetry is the reason.** `Dirty::of` maps an `EffectKind` to the widgets that kind can reach, and the two directions do not cost the same: over-reporting pays one redundant `Projection`, under-reporting leaves a client's cache wrong **forever**, with nothing in the stream to correct it. `LayoutGeometry` therefore includes `formulas`, because `RemoveCategory` wears it and `Model::remove_category` ends by dropping every formula targeting the category — a client's formula panel otherwise kept showing a formula the document no longer had. It now coincides with `Dirty::EVERYTHING` and is still spelled out, because the two are derived from different effects and need not stay equal. The derivation now lives in `context/code/command.md` (the doc comment on `Dirty::of` keeps only the one-line reason `LayoutGeometry` is spelled out rather than written as `EVERYTHING`). It went stale once already — claiming `AddCategory` was `CellData` after it was reclassified — so reclassifying an effect means re-reading the prose, not just the comment.
|
||||
35. **`Dirty` over-reports on purpose, and the asymmetry is the reason.** `Dirty::of` maps an `EffectKind` to the widgets that kind can reach, and the two directions do not cost the same: over-reporting pays one redundant `Projection`, under-reporting leaves a client's cache wrong **forever**, with nothing in the stream to correct it. `LayoutGeometry` therefore includes `formulas`, because `RemoveCategory` wears it and `Model::remove_category` ends by dropping every formula targeting the category — a client's formula panel otherwise kept showing a formula the document no longer had. It now coincides with `Dirty::EVERYTHING` and is still spelled out, because the two are derived from different effects and need not stay equal. The derivation now lives in `context/code/command.md`, and the doc comment on `Dirty::of` points there rather than restating it; the reason `LayoutGeometry` is spelled out rather than written as `EVERYTHING` is a two-line comment inside the match body. It went stale once already — claiming `AddCategory` was `CellData` after it was reclassified — so reclassifying an effect means re-reading the prose, not just the comment.
|
||||
|
||||
36. **A client's command has one door, and the door takes the `App`.** `Sessions::apply(app, id, cmd)` is the only way a connected client's `Command` reaches the server; `ClientSession::fold` and the mirror's `ViewState` are private, so a transport cannot reach the mirror at all. The door takes the authoritative `App` because some commands have a **document half**: `SwitchView` routes through `apply_command`/`App::switch_to_view` (gotcha 29), and **every** mirror then takes what the document *accepted*, not what was asked — so a switch to a view that does not exist leaves the mirrors put. Not just the asker's: `grid_of` builds every client's grid from `workbook.active_view()`, so a bystander left naming the outgoing view has its cursor read under a stale key and is projected the new view's rows positioned by the old view's scroll — gotcha 24's mismatch arriving through somebody else's keystroke. `apply_switch` therefore takes **no `SessionId`**; the switch is addressed to no one session and that absence is the statement. A client's own slice lives on the client and stays in step only by folding the same command, so `Sessions::apply` returns `Applied { dirty, broadcast }`, where `broadcast` is a **`Vec<Broadcast>`** and each item names its own audience (improvise-gvnb). **The audience follows from whether the asker folded.** `Broadcast::ToOthers` is a *correction* of something the asker already applied optimistically; `Broadcast::ToAll` is a *consequence* the asker is owed like everybody else. A `Command::Model` is the one kind a client **cannot** fold optimistically — it cannot predict what the document will accept — so **every** relay originating in `apply_model` is `ToAll`: the `ResetCursor` from `adopt_cursor_epoch`, and the `SwitchView` that `ViewBack`/`ViewForward` move. `apply_switch` is the exception, and only because its asker really did fold. **Copying `ToOthers` from `apply_switch` into `apply_model` is the trap, and it is invisible from the bystander's side** — `adopt_active_view` moves the asker's *mirror* either way, so the drift is between the server's picture of the asker and the asker's own slice, and a test asserting through a "what do the others hear" helper looks away from it by construction. (`apply_switch` has the mirror-image hole on its *refused* path, where the asker folded something the document turned down and is the one client not told: improvise-6c6q.) A `Vec` rather than an `Option` because one command can owe two relays with different audiences, and an `Option` would have had to drop one silently. Everything else is that client's alone and never touches the `App`'s own view slice: **the model is shared, the UI is not.** Folding `SwitchView` into the mirror alone was the bug — `viewport()` then read the cursor under the new view's key and the next `project()` built that client a grid out of the *other* view.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user