d00cea2477
improvise-e0ex. `Margin = Profit / Revenue` drew as `1`: `View::new` seeded
`,.0` — zero decimals — so a 0.66 ratio rounded half-away-from-zero. The cause
is the format, not the arithmetic; exact decimal math would not change the
rendering by one character, so nothing here touches the evaluator and gotcha 8
is unchanged.
Two halves, per the user's ruling:
* `View::new` seeds `,.2`. Revenue draws `18,400.00` and Margin `0.66`. The
noise on whole numbers is the cheaper error — a currency column with two
redundant zeroes is legible, a ratio with none is wrong — and a format cannot
be guessed from a value.
* A formula may carry its own format, which wins for its target; a raw measure
keeps the view's. `formula.pest` gains
`format_clause = ${ ^"FORMAT" ~ sp+ ~ format_spec }`, with `sp+` for `WHERE`'s
reason (a predicate is invisible to the grammar-walking generator) and
`FORMAT` added to `quote::KEYWORDS`.
`improv.pest` is deliberately unchanged: `formula_raw` already carries the
`## Formulas` line verbatim, so the format rides in the formula text with
nothing to add — and the generator, which reads `improv.pest`, sees no new
spelling to violate.
Resolution has one home. `FormatPlan::of(model, view)` replaces the
`(comma, decimals)` pair every render path threaded — `GridLayout::display_text`,
`viewport::grid_body`/`compute_col_widths`, `project::grid_in` and
`persistence/export.rs::render_csv` — rather than being added beside it, which
would be two answers to "what format is this cell". It is asked of the cell's
`CellKey`, so a cell aggregating over `_Measure` keeps the view's format by
construction. `GridBody::col_formats` carries the answer to the totals strip,
which is formatted on its own path.
Persistence is untouched except for metadata: `format_f64` is display,
`persistence/format.rs::format_number` writes round-trip precision, and the two
have no call between them. Newly created views write `format: ,.2`; existing
documents are unaffected, because `format_md` has always written the line
explicitly. `web/demo.improv` moves to `,.2` on both views.
The trap this sets is gotcha 34's: `format_md` writes `Formula::raw`, so a
`FORMAT` suffix round-trips whether or not the walker parsed it. The round-trip
test therefore asserts the parsed field — injecting a walker that drops the
clause fails that one test and nothing else in `improvise-io`, 242 tests
including the whole characterization suite and the grammar properties.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
4247 lines
222 KiB
Markdown
4247 lines
222 KiB
Markdown
# `improvise-command` — test evidence
|
||
|
||
Why each of these tests exists, what bug it reproduced, and what fault was
|
||
injected to prove it can fail. Keyed by **exact function name**; a rename
|
||
orphans the entry, and `scripts/check-comment-runs.py --check-evidence` reports
|
||
that (improvise-9c8c, design-principles §7).
|
||
|
||
Entries cover tests and the helpers whose *shape* is the evidence.
|
||
|
||
---
|
||
|
||
## `ui/reduce.rs`
|
||
|
||
### a_view_slice_command_does_not_move_the_authoritative_view_state
|
||
|
||
This test used to assert the opposite, under the name
|
||
`view_slice_commands_land_in_the_authoritative_view_state`, and it passed against
|
||
a path no host walks (improvise-s17o): `apply_command`'s fallback arm folded a
|
||
wire view command into `app.view_state`, the *authoritative* slice. Nothing in
|
||
production reached it — `Sessions::apply_checked` folds a view command into the
|
||
asking client's mirror — so the behaviour it pinned was wrong and safe only
|
||
because it was dead. The model is shared, the UI is not (principle 5, gotcha 36);
|
||
a client's `SetStatus` must not move the host's status line.
|
||
|
||
The fixture writes `"host"` into the status line first, so the assertion
|
||
distinguishes "the fold was removed" from "the field happened to be empty" — with
|
||
the default empty string, deleting the fold and leaving it in place look the same
|
||
for `SetStatus`. Restoring `reduce_view(&mut app.view_state, other)` fails on the
|
||
first field.
|
||
|
||
#### Helper — `checked`
|
||
|
||
Not a test — the fixture every `reduce.rs` test below reaches the door through.
|
||
|
||
For the tests whose subject is not staleness: a command the current state
|
||
accepts, which every command built by `caught_up` is by construction. The tests
|
||
whose subject *is* staleness call `Checked::against` themselves and assert on the
|
||
`Rejection`, so neither kind can pass by accident through the other's
|
||
convenience.
|
||
|
||
### switch_view_moves_the_client_and_leaves_the_documents_default
|
||
|
||
The successor to `switch_view_moves_the_document_and_the_client_together`, which
|
||
asserted the pre-improvise-jytm lockstep. Which view a client is on is now that
|
||
client's; `Workbook::active_view` is the *default* a fresh client opens and what
|
||
`.improv` writes. The fault it rules out is the old behaviour returning by inertia
|
||
— any writer that moves the workbook's copy from a client's switch fails it.
|
||
|
||
Fault injected 2026-09-20: restoring `App::switch_to_view`'s
|
||
`workbook.switch_view(name)` call fails this and
|
||
`the_documents_default_view_outlives_this_clients_switches`.
|
||
|
||
### a_wire_view_switch_records_history_the_way_a_key_switch_does
|
||
|
||
A wire-driven view switch owes the same history a key-driven one leaves behind.
|
||
|
||
`effect::ViewBack` pops `view_state.view_back_stack`, and the only thing
|
||
that ever pushes onto it is `effect::SwitchView`. While `apply_command` performed
|
||
the switch itself — calling `App::switch_to_view` directly rather than running
|
||
the effect — the stack stayed empty, so a client that switched over the wire
|
||
could not go back, though the same two keypresses could. Wave 4 is what made that
|
||
reachable: before `ModelCommand` there was no wire way to ask.
|
||
|
||
Nothing else in the suite could see it.
|
||
`switch_view_moves_the_client_and_leaves_the_documents_default` checks where a
|
||
switch *landed*; this checks what it *recorded*.
|
||
|
||
Bug demonstrated 2026-09-19 (Copilot, PR #11): the `ViewBack` left the document
|
||
on "Second". Since improvise-jytm the step back is driven through `effect::ViewBack`
|
||
rather than `ModelCommand::ViewBack`, which left the wire with that ticket — the
|
||
stack it pops is one client's history, so a nullary wire command could not address
|
||
it (gotcha 38). What the test asserts is unchanged.
|
||
|
||
### switching_views_projects_the_axis_widgets_only
|
||
|
||
Switching views changes what the axis-bearing widgets read, so they are told; the
|
||
list of views itself did not change, so it is not.
|
||
|
||
### an_inbound_projection_is_ignored
|
||
|
||
Projections travel one way. A client that sends one back must not be able to
|
||
write it into the server's state.
|
||
|
||
### a_keypress_that_changes_the_model_projects_it
|
||
|
||
The key path is the model's door, and it reports the widgets it touched so a host
|
||
can project them.
|
||
|
||
### a_view_only_keypress_projects_nothing
|
||
|
||
A keypress that moves only this client's cursor reaches no other client
|
||
(gotcha 3).
|
||
|
||
### every_model_command_names_an_effect_that_declares_a_model_change
|
||
|
||
The derivation, stated as a check. This is what "derived, not invented" means
|
||
operationally (`doctrine(derive_the_enum_from_the_effects)`). A variant pointed
|
||
at a `ViewOnly` effect would be a wire message that changed the document for the
|
||
client that sent it and for nobody else — `Dirty::of` would report `NOTHING`, and
|
||
no projection would ever correct the others (gotcha 35).
|
||
|
||
#### Helper — `model_app`
|
||
|
||
Not a test — the fixture the three `every_model_command` walks run against.
|
||
|
||
`app()` with a cell actually stored, so a surrogate for it resolves through the
|
||
index rather than only through the layout. The walks over `every_model_command`
|
||
run against this: a `SetCell` whose record cannot be resolved produces *no
|
||
effects*, and every one of those tests asserts on the effects, so the fixture has
|
||
to name something real or they would all fail for a reason that is not theirs.
|
||
|
||
### no_model_command_commits_another_clients_session_local_state
|
||
|
||
improvise-3x3u: the wire vocabulary is the *addressable* model effects, not all
|
||
of them.
|
||
|
||
`Sessions::apply` routes a `Command::Model` with no `SessionId`, which is right
|
||
exactly when the change is addressed to no one session (gotcha 36).
|
||
`ApplyAndClearDrill` and `AcceptWizardModel` are addressed to precisely one, and
|
||
it is not the sender: an unaddressed `ApplyAndClearDrill` commits another
|
||
client's private typing (improvise-tom1) and an unaddressed `AcceptWizardModel`
|
||
swaps the document out of a dialogue the sender never opened. Both were
|
||
demonstrated against `Sessions::apply` before the variants were removed.
|
||
|
||
Re-adding either variant means making it *addressed* — carrying a `SessionId`,
|
||
with the staging map moved onto the session — not restoring the nullary form this
|
||
test forbids.
|
||
|
||
`SESSION_LOCAL_EFFECTS` is the list of the two effect names.
|
||
|
||
### the_session_local_effects_are_real_model_effects
|
||
|
||
The guard above is only worth having while the effects it names still exist and
|
||
are still model changes — otherwise it would pass by naming nothing
|
||
(`doctrine(check_the_checker)`).
|
||
|
||
### the_effect_name_reader_reads_all_three_debug_shapes
|
||
|
||
The reader above is itself a check, so it gets the injection too
|
||
(`doctrine(check_the_checker)`): one effect of each `Debug` shape, because the
|
||
leading identifier is followed by a different delimiter in each — nothing, `(`,
|
||
and ` {`.
|
||
|
||
#### Helper — `effect_type_name`
|
||
|
||
Not a test — the reader that test injects against, used by the two guards above.
|
||
|
||
The type name of an effect, read off the leading identifier of its derived
|
||
`Debug`. `Effect` is a trait object, so `std::any::type_name` is not reachable
|
||
through it; every effect derives `Debug`, and a derived `Debug` opens with the
|
||
type's own name for a unit struct, a tuple struct and a braced struct alike. So
|
||
the name is already on the value — this only reads it back.
|
||
|
||
### every_model_command_names_the_effect_of_the_same_name
|
||
|
||
The mapping is name-for-name by construction, so state that: every
|
||
`ModelCommand` variant names the effect that carries its own name.
|
||
|
||
This is the check that separates an effect from its inverse.
|
||
`every_model_command_names_an_effect_that_declares_a_model_change` cannot:
|
||
`HideItem` and `ShowItem`, `AddItem` and `RemoveItem`, `CreateView` and
|
||
`DeleteView`, `SetCell` and `ClearCell` are each two effects of the *same*
|
||
`EffectKind`, so a swapped pair clears that bar while inverting what the wire
|
||
message does. And `a_model_command_lands_where_its_keypress_lands` cannot reach
|
||
them either — most variants have no single keypress to compare against, so it
|
||
drives three of twenty-eight.
|
||
|
||
Proven by injection 2026-09-19: `HideItem` → `effect::ShowItem`, `CreateView` →
|
||
`effect::DeleteView` and `AddItem` → `effect::RemoveItem` all left
|
||
`cargo test --workspace` at 918 passing, 0 failing. This test fails on each.
|
||
|
||
The name-for-name rule is a design constraint, not a coincidence, and it is what
|
||
makes the derivation checkable at all
|
||
(`doctrine(derive_the_enum_from_the_effects)`): a variant whose effect must be
|
||
named something else is a variant that stopped being derived, and it should fail
|
||
here and be argued about rather than pass quietly.
|
||
|
||
### the_enumeration_names_each_variant_once
|
||
|
||
The `every_model_command` list covers the enum once. A duplicated entry would
|
||
leave a variant untested while the count still looked right.
|
||
|
||
#### Helper — `every_model_command`
|
||
|
||
Not a test — the enumeration that test, and the three walks above it, cover.
|
||
|
||
One value per `ModelCommand` variant. The enum is the derived set, so a test that
|
||
walks "the set" has to walk all of it. `variant_name` is the completeness guard:
|
||
it matches exhaustively, so a variant added to the protocol fails the build here
|
||
until it is listed in this vector as well.
|
||
|
||
### a_model_command_lands_where_its_keypress_lands
|
||
|
||
The point of the whole ticket: a model `Command` applied authoritatively leaves
|
||
the document exactly where the keypress bound to the same mutation leaves it.
|
||
That equivalence is what lets the two entry points share one door.
|
||
|
||
The `Dirty` is compared too, because a host with connected clients projects off
|
||
it: a command that produced the right document and the wrong `Dirty` would leave
|
||
every other client stale.
|
||
|
||
Its `pivoted()` fixture has **two** categories on different axes, because the
|
||
one-category fixture cannot tell a transpose from a cycle: with only `Region` on
|
||
Row, swapping the axes and rotating Row→Column land on the same view, and an
|
||
equivalence test that cannot distinguish the wrong effect from the right one
|
||
proves nothing (`doctrine(check_the_checker)`).
|
||
|
||
### a_stale_wire_model_command_cannot_be_applied
|
||
|
||
A wire `Command::Model` cannot reach the document without its basis being checked
|
||
(ADR 001, improvise-ondj).
|
||
|
||
Bug demonstrated 2026-09-19: `reduce_full` and `apply_command` — the functions
|
||
this module's own doc points every future host at, the browser worker included —
|
||
took a `&Command` and matched `Command::Model { cmd, .. }`, so a batch composed
|
||
against a document that had since moved landed anyway. The `..` discarded the
|
||
basis with no compile error.
|
||
|
||
**The regression guard is the signature, not this test.** Both now take a
|
||
`Checked`, whose only constructor runs `stale_basis`, so the version of this test
|
||
that reproduced the bug — `reduce_full(&mut app, &wire)` with a stale `wire` — no
|
||
longer compiles, and `reduce_full` itself no longer exists (improvise-s17o: having
|
||
no caller was the condition that let the defect hide in the first place). What is left to assert is that the constructor refuses, and that
|
||
refusing leaves the document alone: there is no half-applied batch, because the
|
||
type is obtained before anything is applied.
|
||
|
||
### a_model_commands_dirty_comes_from_its_effects_kind
|
||
|
||
The `Dirty` a model command reports is the one its effect declares, not a second
|
||
table kept alongside the enum (gotcha 35).
|
||
|
||
### a_wire_model_command_marks_the_document_dirty
|
||
|
||
improvise-gv9r. **The bug**: a client's edit left the authoritative document
|
||
reporting *clean*, on every transport.
|
||
|
||
`MarkDirty` used to be a separate effect that the *command* layer composed
|
||
alongside the mutation, and improvise-kmgs derived `ModelCommand` from the
|
||
effects **one at a time** — so the wire got `SetCell` without its companion. A
|
||
derivation correct per element and wrong for a set. "Save if dirty" was therefore
|
||
a trigger that never fired, and a host built on it loses every client edit
|
||
silently; the browser worker found that out the hard way and had to
|
||
over-approximate off the command variant instead.
|
||
|
||
The fix is the unrepresentable one: dirtiness is raised by `App::apply_effects`
|
||
off the same `EffectKind` the `Dirty` and the two basis tiers are read from
|
||
(gotcha 40), so no path can compose a document change without it. There is
|
||
nothing left to forget, and `MarkDirty` is gone rather than left as a second
|
||
answer.
|
||
|
||
The three levels are asserted separately because they are three different
|
||
effects: an effect that is only ever reached by one of them would otherwise pass
|
||
on a sibling's evidence. The negative arm — that a `ViewOnly` effect leaves the
|
||
document clean — is `a_view_only_batch_leaves_the_document_clean` in `ui/app.rs`,
|
||
and it has to live there: a view-slice `Command` never reaches `apply_effects` at
|
||
all (`apply_command` folds it straight into `ViewState`), so a negative written
|
||
against this door would pass however unconditional the raise was.
|
||
|
||
### a_wire_view_switch_leaves_the_document_clean
|
||
|
||
The inversion of `a_wire_view_switch_marks_the_document_dirty`, which asserted the
|
||
pre-improvise-jytm behaviour. A switch used to move `Workbook::active_view`, which
|
||
persists as the `Initial View` line, so it really did put the document out of step
|
||
with the file. It no longer moves any document state, so there is nothing to
|
||
persist and the browser worker's persist trigger (improvise-gv9r) must not fire.
|
||
|
||
The direction matters: the worker asks the document's `dirty` flag rather than a
|
||
table of variants, so a switch that dirtied spuriously would cost an OPFS rewrite
|
||
of the whole document per keystroke (gotcha 41). This is the negative arm — the
|
||
positive one, that a real model command *does* dirty, is the loop above it.
|
||
|
||
The consequence to keep in view is a product one, filed as improvise-azhb rather
|
||
than decided here: a TUI user who switches view and saves now writes the
|
||
`Initial View` the document was loaded with.
|
||
|
||
---
|
||
|
||
## `ui/effect.rs`
|
||
|
||
### the_document_surface_runs_on_a_non_filesystem_backend
|
||
|
||
improvise-fuj: every effect that touches a document goes through `App::storage`,
|
||
so the whole save/load/import/export surface works on a backend that is not a
|
||
filesystem. This is what a browser worker supplies (improvise-i34); here
|
||
`MemoryFs` stands in for it, and the test would fail if any of these effects
|
||
reached for `std::fs`.
|
||
|
||
### view_only_effects_leave_shared_state_alone
|
||
|
||
improvise-mae: a declared `kind()` is only worth having if it matches what the
|
||
effect actually does. Rather than restate each declaration, this applies the
|
||
effect and checks the shared state it claims not to touch is genuinely unchanged
|
||
— a `ViewOnly` effect that quietly wrote a cell would never reach connected
|
||
clients.
|
||
|
||
No exemptions: since improvise-8uz moved the cursor out of `View`,
|
||
`SetSelected`/`SetRowOffset`/`SetColOffset` touch nothing shared, so every effect
|
||
in the list must clear the same bar.
|
||
|
||
### structural_effects_declare_their_kind
|
||
|
||
The structural effects, each pinned to the kind it must declare.
|
||
|
||
The bug this guards: `AddCategory` and `RemoveCategory` declared `CellData`, but
|
||
both go through `Workbook`, whose `on_category_added` / `on_category_removed`
|
||
rewrite **every** view's axis map. That is a re-layout, so a client told only
|
||
"cell data changed" would keep rendering the old grid shape. The two "actually
|
||
change" tests beside it cannot see this — a category change moves model data
|
||
*and* view config, so it passes either list. Only pinning the declaration does.
|
||
|
||
### add_category_past_the_limit_reports_the_failure
|
||
|
||
Bug (improvise-djn): `AddCategory` discarded the `Result` from
|
||
`Workbook::add_category`, so pressing `n` in the category panel with the
|
||
12-regular-category limit already reached reported the optimistic
|
||
`Added category "X"` its command had queued and added nothing. The effect runs
|
||
last precisely so its failure can overwrite that message (improvise-6t3) — it
|
||
just never wrote one.
|
||
|
||
### add_formula_adds_target_item_to_category
|
||
|
||
Regression: `AddFormula` must add the target item to the target category so the
|
||
grid layout includes cells for it. Without this, a formula like
|
||
`Margin = Profit / Revenue` would be registered but invisible in the grid.
|
||
|
||
### abort_chain_short_circuits_apply_effects
|
||
|
||
`AbortChain` must cause subsequent effects in the same `apply_effects` batch to
|
||
be skipped, and the flag must reset so the next dispatch starts clean.
|
||
|
||
### save_without_file_path_aborts_the_chain
|
||
|
||
BUG (improvise-8tk): `Save` reported failure only through the status bar, so the
|
||
`Quit` that `wq`/`ZZ` chains after it landed anyway and the whole session was
|
||
discarded without the user seeing the error. A failed save must abort the rest of
|
||
the batch.
|
||
|
||
### failed_save_as_aborts_the_chain
|
||
|
||
BUG (improvise-lae, found reviewing it): improvise-8tk fixed **one arm** of a rule
|
||
with two. `Save` aborts the batch when the write fails; `SaveAs` reported the
|
||
error to the status bar and let the batch continue, which is the same silent
|
||
discard 8tk closed — the shape
|
||
`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)` names,
|
||
and the three tests above it are evidence about `Save` only.
|
||
|
||
It is reachable today: the `:` line composes with ` . `, so `:w /bad/path . q`
|
||
builds exactly this batch, and `ExecuteCommand` concatenates the two commands'
|
||
effects into one. improvise-lae's save prompt also commits to `SaveAs`, so the
|
||
arm was about to carry more traffic.
|
||
|
||
Note what the abort does and does not buy the prompt: the typed path survives for
|
||
the user to correct, but the message does not reach the screen, because
|
||
`draw_bottom_bar` draws the minibuffer instead of the status line while a prompt
|
||
is open. That is pre-existing and reported separately; the data-loss half is what
|
||
this fixes.
|
||
|
||
### successful_save_as_lets_the_chain_continue
|
||
|
||
The positive arm: a `SaveAs` that succeeds must not abort, or every `:w <path>`
|
||
would swallow whatever followed it. Without this, "always abort" passes
|
||
`failed_save_as_aborts_the_chain`.
|
||
|
||
### enter_edit_at_cursor_uses_target_mode_not_app_mode
|
||
|
||
`EnterEditAtCursor` must use its `target_mode` field, *not* whatever
|
||
`app.view_state.mode` happens to be when applied. The previous implementation
|
||
branched on `app.view_state.mode.is_records()`; the parameterized version trusts
|
||
the caller (keymap or composing command).
|
||
|
||
### enter_edit_at_cursor_reads_post_batch_layout
|
||
|
||
improvise-ehz: the edit buffer must be loaded from layout state that already
|
||
reflects earlier effects in the same batch. `apply_effects` only rebuilds *after*
|
||
the batch, so a preceding `RebuildLayout` is what makes the cursor resolve to the
|
||
post-mutation cell. Removing the row-0 item slides a different cell under the
|
||
cursor; the buffer must show that cell's value, not the pre-batch one.
|
||
|
||
### set_buffer_does_not_special_case_the_search_name
|
||
|
||
`SetBuffer` no longer treats "search" as an alias for the search query
|
||
(improvise-gdl): it is a plain buffer write whatever the name says, and the query
|
||
is reached through `SetSearchQuery`. Commands pick between the two in
|
||
`cmd::core::write_buffer`.
|
||
|
||
### apply_and_clear_drill_coord_rename_moves_every_measure_in_the_row
|
||
|
||
improvise-c5v: in wide format a records row groups one cell per measure, so
|
||
renaming a coordinate has to move every one of them. The long-format code moved
|
||
only the cell under the cursor, which would now split the row in half.
|
||
|
||
### import_headless_csv_short_rows_surface_warning_in_status
|
||
|
||
BUG (improvise-k8i): `ImportJsonHeadless` parsed CSVs with `parse_csv`, which
|
||
sends short-row warnings to stderr — invisible inside the TUI. Short rows must
|
||
surface in the status message, naming the affected column, never silently
|
||
succeed.
|
||
|
||
#### Note — the drill tests drive `apply_effects`, not `apply`
|
||
|
||
Several `ApplyAndClearDrill` tests call `app.apply_effects(vec![...])` rather
|
||
than `effect.apply(&mut app)`. That is deliberate: the dirty flag is raised off
|
||
the `EffectKind` now (improvise-gv9r, gotcha 45), so driving the effect directly
|
||
would assert a flag nothing in production sets.
|
||
|
||
### every_file_writing_effect_aborts_the_batch_on_failure
|
||
|
||
improvise-z05r, the **third** instance of one rule: improvise-8tk fixed `Save` and
|
||
not `SaveAs`; PR #27 found `SaveAs`; `ExportCsv` was the arm still missing, so a
|
||
`Binding::Sequence` or a `:` line composing an export with anything after it
|
||
continued past a failed write.
|
||
|
||
The fix is structural rather than a third patch: all three now report through one
|
||
`report_file_write(app, outcome)`, which writes the status message either way and
|
||
sets `abort_effects` on `Err`. That is not sufficient by itself — a fourth writer
|
||
could still be written without calling it — so the test names the three, and a
|
||
fourth has to come here and be named. Naming is the mechanism; the shared function
|
||
is what makes the naming cheap to honour.
|
||
|
||
Each case drives `App::apply_effects` with `[writer, ChangeMode(Quit)]` against a
|
||
path under a directory that does not exist, and asserts both `last_batch_aborted()`
|
||
and that the trailing mode change never landed.
|
||
|
||
Injected fault: drop `app.abort_effects = true` from `report_file_write`. Fails by
|
||
name, along with the four older abort tests — which is the point: they and this one
|
||
now share one mechanism.
|
||
|
||
Injected fault, narrower: route `ExportCsv` around the reporter and write its status
|
||
directly. Only this test fails, naming `ExportCsv`, with 460 passing — the exact
|
||
state the ticket describes.
|
||
|
||
### a_batch_that_writes_nothing_leaves_the_document_clean
|
||
|
||
improvise-748f. `ViewNavigate { forward: false }` emits `ApplyAndClearDrill`
|
||
unconditionally, and that effect returns immediately when `drill_state` is `None`.
|
||
It declares `Model(CellData)` regardless — `Effect::kind` takes no `App`, so the
|
||
classification cannot be state-dependent — so `apply_effects` raised
|
||
`model_state.dirty` on a document nothing had touched. Press view-back, press `:q`,
|
||
and the quit is refused; autosave arms thirty seconds later.
|
||
|
||
The older test here, `start_drill_and_apply_clear_drill_with_no_edits`, pinned the
|
||
wrong behaviour in an assertion message arguing it was the same over-approximation a
|
||
failed `LoadModel` gets. It is not: a succeeding `LoadModel` leaves a document that
|
||
really does differ from the file its path names, and that remains declared. This was
|
||
a wrong answer to no divergence at all.
|
||
|
||
### a_cell_data_effect_no_raw_cell_records_still_dirties_the_document
|
||
|
||
The guard against the cure improvise-748f **proposed**, which is unsound. The ticket
|
||
suggested raising `dirty` off `bump_changed_records`' per-record fingerprint of the
|
||
raw cells, on the reading that `CellData` means cells. Five of the ten `CellData`
|
||
effects change the document without writing a raw cell: `AddItem`, `AddItemInGroup`
|
||
and `SortData` (which reorders — and `.improv` carries the order), and `AddFormula`
|
||
and `RemoveFormula`, whose results live in `Model::formula_cache` and not in the
|
||
store. Observing raw cells would have left every one of them reading clean, which is
|
||
a *lost edit* — the opposite direction from the spurious prompt it was fixing.
|
||
|
||
So the observation is `document_content` — `document_config` plus the cells in
|
||
iteration order and the formulas — and **only the `CellData` arm reads it**. The
|
||
`LayoutGeometry` arm keeps the narrow `document_config`. A first version pointed both
|
||
arms at the wide fingerprint, and review benchmarked what that costs at 18,000 cells
|
||
in release over 200 batches after warm-up: one `SetCell` batch 8.07 ms → 10.21 ms
|
||
(+27%, a constant factor on a path that was already O(cells) through
|
||
`record_fingerprints`), but one `SetNumberFormat` batch **78 µs → 2.63 ms, 33×** —
|
||
an *asymptotic* change from O(config) to O(cells + config) on the held-key path that
|
||
`[`/`]`, axis cycle, hide/show, group toggle, transpose and prune toggle all take.
|
||
~380/sec where they had been ~12,800/sec. The split costs no correctness because all
|
||
five blind effects are `CellData`; that every `LayoutGeometry` effect really is
|
||
visible to the narrow fingerprint is asserted separately, and not left as a reading.
|
||
|
||
`WholeWorkbook` stays *declared*, and that is the remaining over-report: a `LoadModel`
|
||
whose incoming content happens to be identical still marks the document dirty, and a
|
||
failed one does too. Both cost one redundant write.
|
||
|
||
The `SortData` case needs two cells inserted out of sorted order, or it reorders
|
||
nothing and its case asserts the opposite of what it names. The `AddFormula` case
|
||
removes the fixture's formula first, or adding it is a no-op.
|
||
|
||
Injected fault: narrow `document_content` back to the configuration it hashed before.
|
||
Fails naming `SortData`, with five other dirty-flag tests.
|
||
|
||
### every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint
|
||
|
||
The guard that makes the two-fingerprint split safe rather than a trap. Splitting
|
||
`CellData` (wide) from `LayoutGeometry` (narrow) is only sound while every effect
|
||
wearing `LayoutGeometry` writes something `document_config` hashes; an effect it
|
||
cannot see would leave the document reading clean **forever**, which is the same
|
||
lost-edit failure the wide fingerprint exists to prevent, arriving by the other door.
|
||
|
||
The thirteen are enumerated by brace-matching the `impl Effect` blocks, not by
|
||
grepping — the naive grep reports fourteen, picking up `HelpPageSet`, which is
|
||
`ViewOnly` and is simply the last `pub struct` before `#[cfg(test)]` (gotcha 38 names
|
||
this exact artefact). Each case asserts its effect really declares
|
||
`Model(LayoutGeometry)` before asserting anything else, so a reclassified effect fails
|
||
here rather than silently leaving the list unrepresentative.
|
||
|
||
**Four** cases carry a `prepare` step run **directly against the workbook** and not
|
||
through an effect, because the interesting state is the precondition. What makes such
|
||
a case honest is the **no-op-fails** property: without its precondition the effect
|
||
writes nothing, an unwritten document leaves the fingerprint unmoved, and the case
|
||
therefore *fails* rather than passing vacuously.
|
||
|
||
Measured, by deleting each `prepare` in turn, **two** of the four are load-bearing in
|
||
exactly that way and two are not:
|
||
|
||
- `DeleteView` needs a second view; without it, `dirty` reads false and the case fails.
|
||
- `ShowItem` needs a hidden item; without it, `dirty` reads false and the case fails.
|
||
- `ToggleGroup` is **not** load-bearing here. `ToggleGroup::apply` calls
|
||
`toggle_group_collapse` unconditionally — its membership check gates only
|
||
`reset_cursor` — so `collapsed_groups` moves whether or not the group is real, and
|
||
the case passes with the `prepare` deleted. The step documents the realistic input;
|
||
it is the *cursor* arm, tested elsewhere, that needs a real group.
|
||
- `TransposeAxes` is **not** load-bearing either, and its step is literally a no-op:
|
||
`View::on_category_added` already assigns the first regular category `Row` and the
|
||
second `Column`, so the fixture arrives with `Type`/`Month` differing. The step
|
||
states the precondition rather than establishing it, and the case passes without it.
|
||
|
||
`dirty` is cleared **after** `prepare` and before the effect. That clear is defensive
|
||
hygiene for a *future* `prepare` that goes through `apply_effects`, and it is not what
|
||
makes any case honest today: `dirty` is raised **only** inside `App::apply_effects`,
|
||
and no `prepare` in this test runs an effect, so no preparation can set it. Demonstrated
|
||
by removing the clear **and** blinding `document_config` to `0` together: all thirteen
|
||
cases still read `dirty == false`. An earlier version of this entry claimed the
|
||
opposite — that without the clear "all four cases pass against any fingerprint" — and
|
||
measurement contradicts it.
|
||
|
||
Injected fault: `document_config` returns `0`. Fails, with five other dirty-flag tests
|
||
across three files. Injected fault, narrower and the one that proves the test is
|
||
pointed at the right thing: drop `view.number_format` from `document_config`. Fails
|
||
naming **`SetNumberFormat`**, with only one other test failing anywhere.
|
||
|
||
|
||
---
|
||
|
||
## `ui/app.rs`
|
||
|
||
### model_state_and_view_state_are_constructible
|
||
|
||
improvise-3vr: `ModelState` and `ViewState` are the named slices of `App` state
|
||
introduced by the vb4 refactor. Step 1 only requires that the types exist and are
|
||
constructible; subsequent steps move fields in.
|
||
|
||
#### Note — the `ViewState: Send` static assertion
|
||
|
||
improvise-h0b: `ViewState` is the portable slice — the one thing serialized into
|
||
the Snapshot a newly-connected client receives (improvise-q08). `Send` is the
|
||
cheapest compile-time proxy for "portable": a field that cannot cross a thread
|
||
boundary certainly cannot cross a serialization one. Before h0b this failed to
|
||
compile, because `drill_state: Option<DrillState>` holds an `Rc<Vec<RecordRow>>`.
|
||
Session-local state lives on `App` instead. It is a `const _: fn()` in the test
|
||
module, not a named test, so it has no heading of its own.
|
||
|
||
### app_view_state_owns_ui_session_fields
|
||
|
||
improvise-ew0: `ViewState` owns the UI session slice — mode, status, search,
|
||
panels, navigation, drill, yanked, buffers. `App` accesses them through
|
||
`view_state`.
|
||
|
||
### view_state_survives_a_json_round_trip
|
||
|
||
improvise-a2x: `ViewState` is what a newly-connected client receives as a Snapshot
|
||
(improvise-q08), so it has to survive a round trip through the wire format.
|
||
Populated with non-default values in every field family — a mode carrying a
|
||
`MinibufferConfig`, a navigation frame, a yanked cell, a keyed buffer, an armed
|
||
prefix — because a roundtrip of `ViewState::default()` would pass while proving
|
||
nothing.
|
||
|
||
### the_documents_default_view_outlives_this_clients_switches
|
||
|
||
The replacement for `the_two_active_view_names_never_diverge`, and it is a
|
||
replacement rather than a deletion on purpose: improvise-jytm makes the two copies
|
||
diverge deliberately, and a deliberate split with no guard is how one silently
|
||
regresses into the other.
|
||
|
||
What it pins, all four of which have a silent failure mode. A switch moves the
|
||
client's copy and **not** the document's default. `App::view()` — what
|
||
`rebuild_layout` and `cmd_context` read — follows the client, so the split is
|
||
observable in the layout and not merely in a string. A switch to a view the
|
||
document does not have moves nothing, because the slice keys `ViewState::cursors`
|
||
and `grid_in` indexes the workbook by it. And `install_workbook` still seats this
|
||
client on the incoming document's default and clears the cursor map, since the
|
||
outgoing document's view names mean nothing in the new one (gotcha 24).
|
||
|
||
Fault injected 2026-09-20: `App::switch_to_view` calling
|
||
`workbook.switch_view(name)` again fails the second assertion.
|
||
|
||
### moving_the_cursor_does_not_touch_the_shared_workbook
|
||
|
||
improvise-8uz: moving the cursor must leave the shared document untouched, so that
|
||
one client scrolling cannot move another's viewport. Before the move these three
|
||
effects wrote into `model_state.workbook`, and this assertion would have failed.
|
||
|
||
### app_owns_the_session_local_fields_not_view_state
|
||
|
||
improvise-h0b: the two session-local fields moved off `ViewState` and onto `App`.
|
||
Each fails both membership tests: `ImportWizard` is an `improvise-io` type (not
|
||
portable) and a half-open dialog (not snapshot-safe); `DrillState` holds an `Rc`
|
||
(not portable — see the `Send` assertion) and stages uncommitted edits (not
|
||
snapshot-safe).
|
||
|
||
### staged_drill_edits_are_unreachable_from_view_state
|
||
|
||
improvise-h0b: a fresh client must not inherit another session's staged,
|
||
uncommitted drill edits. Those edits are unreachable from `ViewState`, so a
|
||
Snapshot built from it cannot carry them.
|
||
|
||
### switching_views_restores_each_views_own_cursor
|
||
|
||
improvise-8uz: the cursor is **per view**. Written as a characterization test
|
||
against the old `View`-owned fields and kept verbatim across the move to
|
||
`ViewState::cursors`, because a refactor about *where* state lives must not change
|
||
*what the app does*.
|
||
|
||
### the_improv_format_does_not_persist_the_cursor
|
||
|
||
improvise-8uz: the ticket asserted these fields were "PERSISTED in .improv". They
|
||
were not — the grammar has no cursor production and the writer emitted none. That
|
||
is what made the move free: there was no restore-where-you-left-off behaviour to
|
||
lose, and no format change to make. The assertion still guards the format going
|
||
forward.
|
||
|
||
### entering_records_mode_sorts_existing_data
|
||
|
||
Pressing `R` to enter records mode sorts existing data by `CellKey`, so display
|
||
order is deterministic regardless of insertion order.
|
||
|
||
*Note:* this test's doc comment carried a stray first line from a neighbouring
|
||
test ("Regression: pressing `o` in an empty records view should create the") that
|
||
was never finished. It was dropped in the comment-cap wave; the sentence above is
|
||
what the test actually asserts.
|
||
|
||
### the_new_category_column_creates_a_category_and_its_column
|
||
|
||
improvise-2zq step 2: the `+cat` column creates a category and its column, without
|
||
leaving the grid.
|
||
|
||
Driven through real keys. The assertion that earns its place is the last one: the
|
||
new column is **usable immediately**, which is the whole point —
|
||
`records_columns` filters on `kind.is_regular()` and never requires items, so a
|
||
category with no items is a column the moment it exists.
|
||
|
||
### enter_on_plus_cat_in_the_last_row_does_not_reopen_category_creation
|
||
|
||
improvise-2zq, found by Copilot on PR #27: creating a category from `+cat` shifted
|
||
the grid under the advance that followed it.
|
||
|
||
`add_category` inserts the new coordinate column **immediately before `+cat`**, so
|
||
every column index from `+cat` rightwards moves by one. `CommitAndAdvance` then
|
||
computed its advance from `ctx`, whose layout is the pre-commit one (gotcha 23:
|
||
`apply_effects` rebuilds only after the whole batch). On the last row `Enter`
|
||
wraps to the next column — and the index it wrapped to was `+cat` *again* after
|
||
the rebuild.
|
||
|
||
So the failure is not "lands one column off". It **reopens category creation**: a
|
||
user naming two categories in a row never reaches the column they just made, and
|
||
gets an unbounded `+cat` loop instead. The last row is the arm that wraps, so it
|
||
is the arm tested.
|
||
|
||
### tab_on_plus_cat_does_not_reopen_category_creation
|
||
|
||
The same stale index reached `Tab`, which advances right rather than wrapping.
|
||
Both directions read the pre-commit layout, so both are covered — one arm of a
|
||
two-arm rule is the shape
|
||
`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)` names.
|
||
|
||
### the_new_category_column_creates_the_column_but_writes_no_coordinate
|
||
|
||
Committing the `+cat` name creates the **column** and does not write the
|
||
coordinate on the row the cursor was on.
|
||
|
||
The deliberate answer to the ticket's third question, and the reason is gotcha 1:
|
||
setting a coordinate moves *every* measure cell in the row. That is a data
|
||
migration across N cells, and attaching it to the same `Enter` that made a schema
|
||
change — in an app with no undo — is a bigger single keystroke than the measure
|
||
`+` asks for. The follow-up edit is one column away.
|
||
|
||
### the_new_category_column_refuses_past_the_limit_where_the_user_can_see_it
|
||
|
||
`MAX_CATEGORIES` is a reachable refusal on this path, and the refusal has to be
|
||
**visible**.
|
||
|
||
`draw_bottom_bar` renders the minibuffer instead of the status line, so a message
|
||
written while an editor is open is never drawn. The commit path therefore drops
|
||
out of editing *before* running the add, and `AddCategory` aborts the batch on
|
||
refusal so the trailing `EnterEditAtCursor` cannot re-open the editor over the
|
||
message.
|
||
|
||
The limit itself is checked in exactly one place — `Model::add_category` — and
|
||
nothing here restates it; the test fills the document to the limit by asking for
|
||
categories until it refuses.
|
||
|
||
### naming_a_category_in_records_mode_does_not_eject_the_user
|
||
|
||
improvise-2zq step 1: naming a category in records mode used to eject the user
|
||
from the grid they were standing in.
|
||
|
||
Records mode is `_Index` on Row and `_Dim` on Column, with every regular category
|
||
on `Axis::None`. `View::on_category_added` seated a new regular category on the
|
||
first axis no *regular* category held — Row — and **bumped the virtual out of the
|
||
way** to get there. So `N`, a name, `Enter` moved `_Index` off Row, records
|
||
detection stopped matching, and the grid silently became a pivot.
|
||
|
||
Driven through the real keys rather than through `on_category_added`, because that
|
||
is how the bug was found and it is what makes this evidence rather than a
|
||
restatement of the fix.
|
||
|
||
### i_and_o_diverge_on_an_empty_records_grid
|
||
|
||
improvise-6sq: `i` and `o` want opposite behaviour on an empty records grid, which
|
||
is why `edit-or-drill` and `enter-edit-at-cursor` cannot be merged into one
|
||
command. Entering records mode puts every regular category on `Axis::None`, so
|
||
`EditOrDrill`'s aggregation check is true unless the cursor is on a synthetic
|
||
record — and on an empty grid there is none, so `i` reports "cannot drill". `o`
|
||
must still open an edit there (see
|
||
`add_record_row_in_empty_records_view_creates_first_row`), so routing it through
|
||
the same drill check would break it.
|
||
|
||
improvise-ct5 changed the *message* and deliberately not the divergence. The
|
||
outcome — `i` opens no editor where there is nothing to edit — is right, and `o`
|
||
one key away is the affordance for adding a row. What was wrong is that the
|
||
message said "cannot drill", a pivot-mode concept in a mode whose model is "a
|
||
table I type into". Making `i` behave like `o` here (the ticket's option (b))
|
||
would collapse exactly the divergence improvise-6sq exists to record, so the
|
||
assertion still pins that `i` does not enter editing; only the wording assertion
|
||
moved, and it now also names what must *not* appear.
|
||
|
||
### the_empty_grid_message_is_not_the_records_one_in_pivot_mode
|
||
|
||
The negative arm of improvise-ct5, and the reason the wording is keyed off the
|
||
mode the keymap supplies rather than reworded once.
|
||
|
||
Without this, "say the records sentence everywhere" passes
|
||
`i_and_o_diverge_on_an_empty_records_grid` — and it would promise `o`, which adds
|
||
no row in pivot mode, trading one misleading message for another.
|
||
|
||
The fixture is an empty pivot grid that still counts as aggregated: `Region` on
|
||
Row with no items makes `cell_key()` `None`, and `Extra` cycled to `Axis::None` is
|
||
the regular category that makes `EditOrDrill` take the drill branch.
|
||
|
||
### blank_model_bootstraps_its_first_measure_from_the_new_measure_column
|
||
|
||
A blank model has no categories and no measures, so wide format shows exactly the
|
||
two creation columns. Typing a name into the new-measure one creates the measure,
|
||
which gives it a real column to type values into — records mode stays
|
||
self-sufficient for onboarding (improvise-c5v).
|
||
|
||
improvise-2zq widened that: a blank grid now offers `+cat` beside `+`, so both
|
||
halves of a schema can be bootstrapped without leaving the grid. This test still
|
||
pins the measure half; the category half is
|
||
`the_new_category_column_creates_a_category_and_its_column`.
|
||
|
||
Inside it, the cursor lands at column 0, which in a blank grid is now `+cat`
|
||
rather than `+` (improvise-2zq) — a record needs coordinates before it needs
|
||
measures, so that order is deliberate. `o` leaves the test editing column 0, so
|
||
the `Esc` before moving to the new-measure column is load-bearing.
|
||
|
||
### ctrl_w_prompts_for_a_path_and_a_failed_write_reports_it_where_it_shows
|
||
|
||
improvise-lae end to end: `Ctrl-W`, a path, `Enter`.
|
||
|
||
The failing case is the interesting one, and it pins an **ordering** (found by
|
||
Copilot on PR #27). `SaveAs` aborts the batch on a failed write, so whatever
|
||
`CommitSave` emits after it is skipped. A first version emitted
|
||
`[SaveAs, ChangeMode(Normal)]`, which left the app in `SavePrompt` — and
|
||
`draw_bottom_bar` renders the minibuffer *instead of* the status line, so the
|
||
"Save error:" the effect had just set was never drawn. The save appeared to do
|
||
nothing whatsoever.
|
||
|
||
Leading with the mode change puts the error in `Normal`, where the bottom row is
|
||
the status bar. This test therefore asserts the mode *left* the prompt, which is
|
||
the opposite of what it asserted before — deliberately, and it is the only thing
|
||
keeping the two effects in that order.
|
||
|
||
### every_minibuffer_mode_entered_from_records_returns_to_records_normal
|
||
|
||
Copilot round 2 reported that `Ctrl-W` from a records grid returns to `Normal`,
|
||
losing the records keymap while the layout is still a records layout. It is right,
|
||
and it is a **class**: every minibuffer mode reachable from `RecordsNormal` exited
|
||
to `Normal`, because the exit was a constant baked into
|
||
`TextEntry::new(..., "normal")` and into each commit command.
|
||
|
||
The class **predates** `SavePrompt`: `:` and `Ctrl-E` do exactly the same thing
|
||
and always have. That is why the fix was not "make `SavePrompt` remember its
|
||
origin" — that would leave two of the three arms broken, which is the shape
|
||
`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)` names.
|
||
|
||
The binding *table* is not involved and was checked rather than reasoned about: an
|
||
`IMPROVISE_BINDING_DUMP` diff shows `RecordsNormal` gains `Ctrl-W` and loses
|
||
nothing, so `with_parent` inheritance (gotcha 13) is intact. What was lost is the
|
||
runtime mode.
|
||
|
||
improvise-sihr fixes it by **observation rather than memory**, which is what
|
||
took the exit out of the keymap entirely: `App::apply_effects` calls
|
||
`settle_normal_mode` after `rebuild_layout`, so the mode a batch *ends* in
|
||
matches the grid it ends on and nothing carries an entry mode. The ticket
|
||
proposed the opposite — carrying the entry mode on `MinibufferConfig` — and that
|
||
was declined for a concrete reason beyond doctrine: carried generically it would
|
||
also return `CategoryAdd` to `CategoryPanel`, a behaviour change nobody asked
|
||
for, while carried for only three modes it is the one-arm shape again.
|
||
|
||
This test still drives **both exits** — Esc and Enter — where the original drove
|
||
only Esc, because they reach the mode through different effects
|
||
(`ChangeMode` and `ReturnToNormalUnlessMoved`) even though one settle now covers
|
||
both.
|
||
|
||
**The first version of the fix resolved per-effect and was wrong in a new
|
||
direction**, which the adversarial review of PR #91 caught. Set at the moment
|
||
the mode is written, the resolution reads the batch's *opening* layout (gotcha
|
||
23) — so a batch that **crosses** settled against the grid it started on. See
|
||
`a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal`.
|
||
|
||
Injected fault: `settle_normal_mode()`'s call site removed. Exactly three tests
|
||
fail across the whole workspace — this one, the `:`-line sibling and the
|
||
crossing test — which is the per-edit coverage question answered by there being
|
||
one edit.
|
||
|
||
### a_colon_line_run_from_records_settles_back_into_records_normal
|
||
|
||
The sibling above cannot see `ReturnToNormalUnlessMoved`, and that was found by
|
||
injection rather than by reading: forcing *only* that effect back to
|
||
`AppMode::Normal` left the sibling green. An **empty** `:` line — which is what
|
||
pressing Enter straight after `:` gives — takes `ExecuteCommand`'s
|
||
`raw.is_empty()` branch and settles through `ChangeMode`, so the effect that
|
||
settles every line that actually runs was untested.
|
||
|
||
So this test types a line. `page-next` rather than `yank`, because the *script*
|
||
parser requires coordinates for `yank` and an unparseable line takes the error
|
||
branch — which settles through `ChangeMode` too, and would have made this test
|
||
a second copy of its sibling while looking like a new case. The setup assertion
|
||
that `parse_line_with` accepts the line is there for exactly that: a future
|
||
rename of the command would otherwise turn this test vacuous silently.
|
||
|
||
Injected fault (against the per-effect version of the fix):
|
||
`ReturnToNormalUnlessMoved` forced to `AppMode::Normal`. This test failed,
|
||
`left: Normal right: RecordsNormal`, while the sibling stayed green — which is
|
||
the point, and is how this test came to exist. Against the shipped end-of-batch
|
||
settle the discriminating injection is removing `settle_normal_mode()`'s call
|
||
site, which fails this test with the same values.
|
||
|
||
### a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal
|
||
|
||
**New wrong state introduced by PR #91's first attempt**, found by the
|
||
adversarial review and verified by reverting only `ui/app.rs` and
|
||
`ui/effect.rs` to base and re-running the identical probe:
|
||
|
||
| `:toggle-records-mode . enter-mode normal` | base | #91 round 1 |
|
||
|---|---|---|
|
||
| from a pivot grid | `Normal` (the sihr bug) | `Normal` still |
|
||
| **from a records grid** | `Normal` (correct) | **`RecordsNormal`** |
|
||
|
||
The cause is gotcha 23: resolving at the moment the mode is *written* reads the
|
||
batch's **opening** layout, so a batch that crosses settles against the grid it
|
||
started on. `:toggle-records-mode` is the case the round-1 prose offered as
|
||
proof that the staleness was a *property* — and it is not, because
|
||
`ToggleRecordsMode` emits `change_mode(RecordsNormal)` explicitly on entry and
|
||
`ViewBack` restores the pushed mode on exit, so the resolution's answer is
|
||
overwritten in both directions and the case exercises nothing. That is a careful
|
||
comment standing in for a test, in the PR that recorded
|
||
`doctrine(an_injection_that_does_not_fail_names_an_untested_path)`.
|
||
|
||
The fix moves the resolution to **one** place, `settle_normal_mode` after
|
||
`rebuild_layout`, where there is no crossing case left to get wrong.
|
||
|
||
This test drives **both directions** deliberately. One direction alone is
|
||
satisfied by a settle that only ever answers one way, and the pivot→records
|
||
direction was *also* wrong in round 1 for the mirror-image reason.
|
||
|
||
### a_wizard_abandoned_over_a_records_grid_returns_to_records_normal
|
||
|
||
`CloseWizard` writes `view_state.mode` directly rather than through an effect
|
||
that could resolve it, so under round 1's per-effect design it was a **third
|
||
writer** needing its own edit — and reverting that edit alone left the whole
|
||
workspace green, which the review filed. The end-of-batch settle removes the
|
||
per-edit question entirely (there is one edit), and this test covers the path
|
||
the review named: the wizard abandoned over a records grid.
|
||
|
||
It drives `CloseWizard` as an effect rather than typing `:import`, because the
|
||
wizard needs a real CSV and the contract under test is one batch's settling, not
|
||
the import pipeline.
|
||
|
||
### a_failed_colon_write_lands_in_normal_where_its_error_is_drawn
|
||
|
||
A failing `:` command must not strand the user in `CommandMode`.
|
||
|
||
Found by Copilot on PR #27, and it is the sharper form of gotcha 21. The abort is
|
||
*supposed* to stop the chain — `:w /bad . q` must not quit. What it must not also
|
||
stop is the **bookkeeping**: the mode restoration that `ExecuteCommand` appends.
|
||
Aborting before that ran left the app in `CommandMode` with the stale command line
|
||
still drawn, and `draw_bottom_bar` renders the minibuffer *instead of* the status
|
||
line — so the "Save error:" set one effect earlier was invisible. A failed save
|
||
looked exactly like a key that did nothing.
|
||
|
||
The answer is **ordering, not classification**: a batch puts its tidy-up where an
|
||
abort cannot reach it, which is in front. No effect needs a new "runs anyway"
|
||
flag, and the rule generalises to any command composing bookkeeping with something
|
||
that can fail.
|
||
|
||
Both arms are here because they fail in different places: no path at all is
|
||
`Save`'s guard (aborting since improvise-8tk, so this arm was broken before that
|
||
branch), and a bad path is `SaveAs`'s write (aborting since f0f400d).
|
||
|
||
### a_failed_colon_write_still_refuses_to_quit_after_it
|
||
|
||
The other half of the same rule: the abort must still stop the *chain*.
|
||
`:w <bad> . q` is improvise-8tk's data loss spelled on one `:` line, and moving the
|
||
mode restoration to the front must not have bought the fix above at the price of
|
||
that one.
|
||
|
||
### a_sequence_whose_first_step_aborts_does_not_run_the_second
|
||
|
||
Gotcha 21 claims a failed `Save` aborts across `Binding::Sequence` steps as well
|
||
as within one command's effect list. Nothing tested it: no shipped `Sequence` has
|
||
a first step that can abort, and `ZZ` is a *prefix* resolving to the single `wq`
|
||
command, so `prefix_z_then_z_reaches_save_and_quit` exercises the
|
||
within-a-command case only. The claim was structurally true and unguarded.
|
||
|
||
It is true because `Keymap::dispatch` flattens a `Sequence` into ONE effect vec and
|
||
`App::apply_effects` runs that as one batch, breaking on `abort_effects`. It is
|
||
*load-bearing*, because it is the reason a `[w, force-quit]` Sequence would be
|
||
safe — the argument the grooming note unblocked on.
|
||
|
||
And it is exactly what improvise-0hb would remove. Command-level transactionality
|
||
means applying effects *between* steps, and `apply_effects` resets
|
||
`abort_effects` at entry and again on abort, so step 2 would be dispatched into a
|
||
fresh batch with the flag cleared — the `Quit` after a failed save would land,
|
||
silently reopening the improvise-8tk P1 data loss. This is the guard that would
|
||
catch it.
|
||
|
||
### unmatched_key_after_prefix_is_swallowed
|
||
|
||
A prefix is consumed whether or not the next key matches, so a stray key after `g`
|
||
does nothing rather than being treated as a fresh normal-mode binding.
|
||
|
||
### renaming_a_coordinate_moves_every_measure_in_the_row
|
||
|
||
improvise-c5v: a records row groups one cell per measure, so editing a category
|
||
column has to move every one of them. The long-format code moved only the cell
|
||
under the cursor, which in wide format would tear the row apart — half its
|
||
measures under the old coordinate, half under the new one.
|
||
|
||
### leaving_records_mode_cleans_empty_key_cells
|
||
|
||
improvise-3zq (bug #2): `AddRecordRow` creates a cell with an empty `CellKey` when
|
||
no Page-axis categories supply coords — that cell serialises as ` = 0` in
|
||
`.improv` and re-appears on every records toggle. Leaving records mode must clean
|
||
up any such meaningless records (the inverse of the `SortData` that runs on
|
||
entry).
|
||
|
||
### enter_at_bottom_right_of_records_view_exits_editing
|
||
|
||
improvise-3zq (bug #1): `Enter` on the bottom-right cell of records view should
|
||
commit and leave edit mode. Previously `CommitAndAdvance` pushed an
|
||
`EnterEditAtCursor` effect unconditionally, so the cursor stayed put and editing
|
||
re-opened on the same cell.
|
||
|
||
#### Helper — `records_model_with_two_rows`
|
||
|
||
Not a test — the fixture for every Tab/Enter boundary test in this file.
|
||
|
||
A records-mode app with two data rows, for testing `Tab`/`Enter` behaviour at
|
||
boundaries. In wide format a row is one coordinate tuple, so the two rows are two
|
||
`Region` items, each carrying both measures (improvise-c5v).
|
||
|
||
### tab_on_bottom_right_of_records_inserts_below
|
||
|
||
improvise-hmu: `Tab` on the bottom-right cell of records view inserts a new record
|
||
below and moves to the first cell of the new row in edit mode.
|
||
|
||
#### Note — the `wizard_key_tests` module
|
||
|
||
Behavioural coverage for the import wizard's key handling (improvise-2wm). The
|
||
wizard is driven entirely through `handle_key`, so these tests pin the
|
||
user-visible contract independently of whether the step/key decisions live in an
|
||
effect or in a command.
|
||
|
||
### name_model_typing_edits_the_name_and_enter_confirms_the_import
|
||
|
||
`Enter` at `NameModel` builds and parks on the `Done` confirmation; a second
|
||
`Enter` there installs the workbook and closes the wizard (improvise-907, from
|
||
improvise-c6j).
|
||
|
||
### review_proposals_blocks_when_no_category_is_accepted
|
||
|
||
BUG (improvise-907, from improvise-gfq): an import whose fields all classify as
|
||
`Measure` used to walk the user through `DefineFormulas` and `NameModel` before
|
||
`build_model` refused. The refusal now happens at `ReviewProposals`, where `c` can
|
||
still change a field's kind.
|
||
|
||
### left_walks_back_through_the_steps_taken
|
||
|
||
BUG (improvise-907, from improvise-o1c): reaching a later step and wanting to
|
||
change a field kind meant `Esc`, which discarded every decision. `Left` walks back
|
||
through the path actually taken (gotcha 22).
|
||
|
||
### done_esc_returns_to_name_model_and_drops_the_built_model
|
||
|
||
`Esc` at `Done` goes back to `NameModel` rather than discarding the import, and
|
||
the workbook built for confirmation is dropped so a later accept cannot install
|
||
stale field decisions.
|
||
|
||
#### Note — the `basis_tests` module fixture
|
||
|
||
The two lower tiers of a batch's basis (ADR 001, improvise-xwi6). Fixture note:
|
||
two regular categories, two measures and two records, deliberately. A one-category
|
||
fixture cannot tell `TransposeAxes` from `CycleAxis`, a one-record fixture cannot
|
||
tell "the record it touched" from "every record", and a one-measure fixture cannot
|
||
tell a record from a cell — the three distinctions these tests exist to draw.
|
||
|
||
### a_records_two_measures_share_one_record_key
|
||
|
||
The record is the coordinate tuple with `_Measure` gone, and both measures of one
|
||
coordinate name the same record, which is the whole point of the unit.
|
||
|
||
### a_layout_change_moves_the_layout_version_and_no_record
|
||
|
||
Tier 2 moves, tier 3 does not. A shape change invalidates every in-flight
|
||
`CellData` batch through `layout_version` alone; it must not also masquerade as a
|
||
write to any record.
|
||
|
||
Fixture repaired under improvise-75ld: it asked for
|
||
`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where
|
||
`on_category_added` had already seated `Month` on Column — so the "layout change"
|
||
was a no-op, and the test passed only because the tier was bumped off the
|
||
declaration. It now asserts the axis before and after, and asks for `Axis::Row`.
|
||
This is the failure that turned "reclassify three effects" into "stop reading the
|
||
tier off the classification". Injected fault: the same as
|
||
`both_shape_counters_move_for_every_reshaping_command` — fails naming `SetAxis`.
|
||
|
||
### a_loaded_document_is_dirty_because_its_path_still_names_the_old_one
|
||
|
||
**A loaded document is dirty, and that is a behaviour change with a reason**
|
||
(improvise-gv9r, raised by the independent review of PR #26).
|
||
|
||
`LoadModel` is `Model(WholeWorkbook)`, so `apply_effects` raises the flag — where
|
||
before that branch `:e other.improv` left the document reporting clean. Clean was
|
||
the wrong answer: `LoadModel` does **not** move `model_state.file_path`, so after
|
||
it the in-memory document is a different one from the file the path names, and the
|
||
bytes on disk and the bytes in memory really have parted company. That is all
|
||
`dirty` claims.
|
||
|
||
It is worth a test rather than a comment because of what it gates:
|
||
`autosave_if_needed` and the `:q` refusal both read this flag, so a user who loads
|
||
and quits is now asked rather than silently dropping a document whose path is
|
||
stale. `:q!` still discards. The path-staleness itself is pre-existing and is
|
||
reported separately; this pins only that the flag follows from it.
|
||
|
||
### a_view_only_batch_leaves_the_document_clean
|
||
|
||
The negative arm of improvise-gv9r's raise, and it must be aimed at
|
||
`apply_effects` rather than at a `Command`: a view-slice command is folded
|
||
straight into `ViewState` and never reaches this loop, so a negative written
|
||
against the session door would pass however unconditional the raise was. It is a
|
||
`ViewOnly` effect **run through the loop** that discriminates.
|
||
|
||
What it pins: a client's cursor, status line or panel does not put the document
|
||
out of step with the bytes on disk, so it must not arm an unsaved-changes prompt
|
||
or an autosave.
|
||
|
||
### a_cell_edit_moves_only_the_record_it_touched
|
||
|
||
Tier 3 moves for the record written and for no other, and tier 2 stays put: a cell
|
||
edit does not invalidate batches that merely share the document.
|
||
|
||
### a_coordinate_rename_is_one_write_to_each_record_not_one_per_measure
|
||
|
||
The unit is the record, not the cell. Renaming a coordinate in wide records mode
|
||
moves *every* measure cell in the row (gotcha 1), and that is ONE write to the
|
||
record it left and ONE to the record it arrived at — not one per measure.
|
||
|
||
### an_aborted_batch_bumps_nothing_it_did_not_run
|
||
|
||
Gotcha 21: a batch that aborts halfway must bump nothing after the abort — in
|
||
either tier. `apply_effects` accumulates the `Dirty` inside the loop for exactly
|
||
this reason, and the versions are read off the same place.
|
||
|
||
### a_write_that_changes_nothing_moves_no_version
|
||
|
||
A write that lands the value already there invalidates nobody's basis, so it moves
|
||
no version. This is a deliberate choice of semantics — observed change, not "a
|
||
`CellData` effect ran" — and the safe direction, because the alternative rejects
|
||
batches for nothing.
|
||
|
||
### the_version_tiers_are_not_in_the_view_state
|
||
|
||
The counters are shared document state on `App`, never on the portable session
|
||
slice (gotcha 30): a client inheriting another's idea of how far the document has
|
||
moved is exactly the stale basis this machinery exists to catch.
|
||
|
||
### a_page_flip_under_prune_empty_invalidates_the_cursor
|
||
|
||
improvise-7kvw, the case improvise-9mo9 decided the other nine and deliberately left
|
||
open. `SetPageSelection` changes which records are visible and not which categories
|
||
frame them, so a grid position normally keeps naming the same cell. Under
|
||
`View::prune_empty` that is false: which rows survive pruning is a function of the
|
||
page selection, so a page flip renumbers the grid while neither `reset_cursor` nor
|
||
`cursor_epoch` moved — gotcha 24's failure in both directions at once, the host's own
|
||
cursor stale and no client told.
|
||
|
||
9mo9 did not patch it because `[`/`]` paging is constant navigation and sending the
|
||
cursor home on every flip is worse than the hole when `prune_empty` is off, which is
|
||
the default. So the reset is conditional on `prune_empty`, and on the selection
|
||
actually moving.
|
||
|
||
The fixture has three regions and two quarters whose non-empty rows differ, so
|
||
position 0 names `North` on Q1 and `South` on Q2. The test asserts the row labels
|
||
really differ before asserting the epoch moved; the negative arm asserts they really
|
||
*do not* differ with pruning off, so neither case can pass vacuously.
|
||
|
||
Injected fault: drop the reset. The prune arm fails. Injected fault: make the reset
|
||
unconditional. The no-prune arm fails.
|
||
|
||
### a_page_flip_without_prune_empty_leaves_the_cursor_alone
|
||
|
||
The negative arm of the above, and it is load-bearing for the same reason gotcha 24
|
||
gives for `SetNumberFormat`: without it, "reset on every page flip" passes the
|
||
positive test and costs the user their position on every `[`/`]`.
|
||
|
||
### selecting_the_page_already_selected_invalidates_no_cursor
|
||
|
||
The third arm, and the one the user actually hits. `SetPageSelection` resets the
|
||
cursor only when the selection **moved**, and that guard shipped untested: deleting
|
||
`moved &&` left the whole workspace green at 1416, which review caught.
|
||
|
||
It is not an edge case. `PageNext`/`PagePrev` are odometer-style and emit a
|
||
`SetPageSelection` for **every** page category on every keypress, so with more than
|
||
one page category the non-carrying categories are always no-ops, and a single
|
||
one-item category re-selects the same item every time. Unguarded, each such keystroke
|
||
bumps `cursor_epoch`, sends the host's cursor to the origin and broadcasts
|
||
`Command::ResetCursor` to every client on that view — improvise-75ld's "a no-op that
|
||
refused everybody else", one tier down and aimed at the cursor instead of the basis.
|
||
|
||
The test asserts both preconditions **before** applying the effect — that the fixture
|
||
really is on `Q1`, and that the grid really does prune — and that the row labels did
|
||
not move before asserting the epoch stood still. So it cannot pass by re-selecting a
|
||
page that was never selected, or by testing a grid that does not prune.
|
||
|
||
Both `before` placements are load-bearing, and both were wrong once. The page check
|
||
used to run **after** the effect, which sets the page to `Q1`, so it could not see a
|
||
fixture that started elsewhere: with the fixture moved to `Q2` the test did fail, but
|
||
at the *row-labels* assertion, under a message about renumbering. The sentence naming
|
||
the property was not the assertion guarding it. The `prune_empty` check did not exist
|
||
at all, and with pruning off the reset never fires whatever `moved` says — so
|
||
`paged_model(false)` plus the deleted guard left the test **green**, proving nothing,
|
||
with nothing saying so.
|
||
|
||
Both preconditions read `App::view()`, the view the effect writes through
|
||
`app.view_mut()`, and not `Workbook::active_view()`, which is the document's
|
||
*default* and is deliberately allowed to differ from it (gotcha 29). They agree in
|
||
this single-client fixture, so the distinction is latent rather than a live bug —
|
||
but an assertion about what an effect did must read the view that effect wrote.
|
||
|
||
Injected fault: delete `moved &&`. Fails by name, alone.
|
||
Injected fault: `paged_model(false)` **and** delete `moved &&`. Fails on the
|
||
`prune_empty` assertion; before that assertion existed, this combination passed.
|
||
Injected fault: `paged_model`'s fixture starts on `Q2`. Fails on the page-selection
|
||
assertion, which now names it; before the move it failed on the row labels instead.
|
||
|
||
### a_foreign_layout_is_not_shown_the_hosts_staged_drill_text
|
||
|
||
improvise-pubt, filed with an argument for unreachability that the test disproves.
|
||
`project::cell_display` fills `edit_text` from `App::display_value_in(layout, key)`,
|
||
and since improvise-jytm that layout may be another client's.
|
||
`drill_state.pending_edits` are keyed by `(row index, RecordCol)` against the
|
||
**host's** drill layout (gotcha 1), and a row index in one layout names nothing in
|
||
another.
|
||
|
||
The ticket read the arm as unreachable because "a foreign layout built by
|
||
`GridLayout::new` carries no frozen records, so it should not produce a synthetic
|
||
key". That is wrong: records mode **auto-detects** from the axes (gotcha 1), so an
|
||
ordinary saved view with `_Index` on Row and `_Dim` on Column gives `GridLayout::new`
|
||
a records layout whose `cell_key` returns exactly the synthetic `(_Index, _Dim)` key
|
||
the drill arm matches on. The test fails on the original code, naming the host's
|
||
staged text read out of a layout it was never staged against.
|
||
|
||
The fix is `App::staged_drill_edit`, which consults `drill_state` only when
|
||
`std::ptr::eq(layout, &self.layout)`. A pointer comparison rather than a name
|
||
comparison because `grid_in` reuses `&app.layout` by reference where the view names
|
||
agree and derives a fresh one otherwise, so identity *is* the question being asked.
|
||
Whether the host's pending drill text should reach any other client at all is
|
||
improvise-tom1 and is untouched here.
|
||
|
||
The positive arm — the host's own layout still shows `STAGED` — is in the same test,
|
||
or "never consult the drill" passes, which would blank the TUI's own drill editor.
|
||
The drill fixture needs a real `RecordRow`, not an empty `Vec`: the host's layout
|
||
iterates the frozen snapshot, so an empty one gives a zero-row grid and the positive
|
||
arm cannot be built.
|
||
|
||
### a_default_that_names_nothing_says_which_invariant_broke
|
||
|
||
improvise-h074. `App::resolve_view` falls back to `workbook.active_view` for a name
|
||
the document has not got and then indexes `workbook.views[default]`. Several of these
|
||
call sites previously went through `Workbook::active_view()`, which carries
|
||
`.expect("active_view always names an existing view")`; `IndexMap`'s `Index` impl
|
||
panics with no message at all, so a workbook whose default names nothing — reachable
|
||
only by constructing the struct directly, since `create_view`/`delete_view` maintain
|
||
it — failed with a bare index panic where it used to name the broken invariant.
|
||
|
||
Injected fault: restore `&workbook.views[default]`. The test fails on the expected
|
||
substring.
|
||
|
||
|
||
---
|
||
|
||
## `ui/project.rs`
|
||
|
||
### a_window_reaching_the_last_row_carries_the_trailing_group_header
|
||
|
||
A collapsed group at the bottom of the grid has no items to hang off, and is still
|
||
the only row that says those rows exist. A window that reaches the last row
|
||
therefore carries it.
|
||
|
||
It must be *seen*; it is not yet actionable in a browser. `web/app.js` registers no
|
||
listener on a group `<th>`, and `toggle-group-under-cursor` is still in
|
||
`the_command_names_that_still_need_the_host_are_these`, so `z` resolves `Upstream`
|
||
into `Host::handle`'s empty vec — a black hole, not a slow path
|
||
(`doctrine(an_upstream_key_is_a_black_hole)`). The TUI can expand it today; the
|
||
browser can only show it.
|
||
|
||
#### Helper — `app_with_a_trailing_group`
|
||
|
||
Not a test — the fixture for that test and for its stops-short arm.
|
||
|
||
`Region` on Row with `East` in a group that comes **last**, so the group header is
|
||
the final entry on the axis and nothing follows it. That is the shape
|
||
`axis_frame`'s trailing rule is about.
|
||
|
||
### a_window_that_stops_short_does_not_carry_what_is_below_it
|
||
|
||
The other arm, and the one that would make a browser scrolled to the middle of a
|
||
document draw the bottom of it: a window that stops short of the last row carries
|
||
nothing below its last item.
|
||
|
||
### a_window_opening_inside_a_group_names_only_the_nearest
|
||
|
||
A window opening *inside* a group still names it — otherwise a browser could not
|
||
say which group a column or row belongs to — while `body_from` declines to paint a
|
||
header that sits above the window.
|
||
|
||
**And only the nearest one comes.** The fixture puts **two** headers above the
|
||
window (`Hidden` is collapsed, so it emits a header and no items, and `Late`'s
|
||
follows it immediately) because that is the only way to tell "the nearest header
|
||
comes" from "every header above comes". With one group above, deleting the
|
||
compaction leaves this green — it is a *cost* property, O(the groups a deep scroll
|
||
crossed), and correctness alone cannot see it: `group_of` still finds the nearest
|
||
and `body_from` still starts at the first item either way.
|
||
|
||
#### Helper — `app_with_consecutive_headers`
|
||
|
||
Not a test — the fixture for that test and for `consecutive_headers_between_two_rows_all_come`.
|
||
|
||
Two consecutive group headers between two rows: `Hidden` is collapsed, so it emits
|
||
a header and no items, and `Late`'s header follows it immediately.
|
||
|
||
### consecutive_headers_between_two_rows_all_come
|
||
|
||
Headers between two in-window rows all come, not just the nearest.
|
||
|
||
Only the nearest wins `group_of`, so a compaction that kept one would still answer
|
||
every question about *membership* correctly — and would silently lose the `Hidden`
|
||
header, which is the only thing on screen that says those rows exist at all, and
|
||
the only thing that *will* be clickable once the browser can toggle a group. It
|
||
cannot today: `z` is `Upstream` and answered with nothing.
|
||
|
||
Above the window the rule is the opposite, and that arm is
|
||
`a_window_opening_inside_a_group_names_it_without_painting_it`.
|
||
|
||
### folding_what_the_server_emits_reproduces_what_the_server_has
|
||
|
||
The contract of the whole module: a client that folds what the server sends ends up
|
||
holding what the server has. Not the fold axiom — the yardstick (`server_truth`) is
|
||
built independently of the projections.
|
||
|
||
#### Helper — `server_truth`
|
||
|
||
Not a test — the yardstick that test and its siblings measure a fold against.
|
||
|
||
What the server believes the client should be holding, built straight from the
|
||
authoritative state — the yardstick every fold is measured against, and built
|
||
independently of the projections so the fold tests are not vacuous
|
||
(`doctrine(fold_tests_must_test_order_not_the_axiom)`).
|
||
|
||
### removing_a_category_empties_the_clients_formula_panel
|
||
|
||
Removing a category removes every formula that targets it —
|
||
`Model::remove_category` ends with `formulas.retain(|f| f.target_category != name)`.
|
||
`RemoveCategory` is `LayoutGeometry`, and `Dirty::of(LayoutGeometry)` reported
|
||
`formulas: false`, so the formula panel was never told: a client's `FormulaCache`
|
||
kept showing a formula the document no longer has, indefinitely, with nothing to
|
||
bring it back short of a reconnect. (Gotcha 35.)
|
||
|
||
### a_late_client_converges_with_one_that_followed_from_the_start
|
||
|
||
Bootstrap is a compaction, not a replay: a client that connects late and folds only
|
||
the seed lands exactly where one that followed the whole stream is.
|
||
|
||
### the_projected_extent_is_the_whole_grid_not_the_window
|
||
|
||
The extent is the **whole** grid, not the window (improvise-bpq2).
|
||
|
||
A client resolving its own cursor keys clamps against this, so a producer that read
|
||
it off the projected ranges would stop every browser cursor at the bottom of the
|
||
screen — and would look right in the TUI, whose cache covers the whole grid at
|
||
offsets zero (gotcha 44). The fixture is deliberately **not square** (3 rows, 2
|
||
columns) and the window is smaller in both directions, so it separates "the whole
|
||
grid" from "the window" *and* from "rows and cols swapped".
|
||
|
||
### a_reshaped_grid_folds_to_the_desired_state
|
||
|
||
Reshaping is the case the ordering exists for. Growing is the half that bites:
|
||
cells at positions the client does not have yet are dropped, so the labels that
|
||
create those positions must go first. Shrinking is here for the other direction —
|
||
cells that no longer fit must not be sent into a grid that has already been cut
|
||
down.
|
||
|
||
### a_renamed_column_whose_total_did_not_change_still_gets_its_total_back
|
||
|
||
improvise-hkt6, found by Copilot on PR 20. The diff must carry the mirror to
|
||
`desired`, and it stopped doing so for one shape: a column **renamed** while its
|
||
total stayed the same number.
|
||
|
||
`apply` blanks the total under a relabelled column (gotcha 33(b)), so the client's
|
||
total went blank; but the diff compared `mirror.totals` against `desired.totals`,
|
||
found them equal, and sent no `Totals`. `ClientSession::project_diff` then assigns
|
||
`self.grid = desired`, so the server believed the number was still on screen — a
|
||
cache wrong **forever**, with nothing in the stream to correct it, which is
|
||
precisely the direction gotcha 35 keeps `Dirty::of` sloppy to avoid.
|
||
|
||
The assertion is the one `a_reshaped_grid_folds_to_the_desired_state` makes — fold
|
||
the updates onto the mirror and require the result to be `desired` — because "a
|
||
`Totals` update was emitted" would pass for a producer that emitted the wrong
|
||
totals.
|
||
|
||
The fixture renames the **middle** column and keeps every total equal, so it
|
||
separates "re-sends what the relabel blanked" from "re-sends whenever the labels
|
||
move at all".
|
||
|
||
### renaming_an_axis_category_resends_every_cell
|
||
|
||
The producer half of the category-rename rule, and the reason the `apply` half is
|
||
not cosmetic: the two compose into "wrong forever" without it.
|
||
|
||
`grid_updates` diffs the cells against the **framed** mirror — the mirror with the
|
||
frame updates already folded in. So an `apply` that kept the cells through a `cats`
|
||
change made the producer agree they matched and send nothing,
|
||
`ClientSession::project_diff` then assigned `self.grid = desired`, and the client
|
||
held cells answering the old coordinates with no update in the stream that would
|
||
ever correct them.
|
||
|
||
An axis swap is the case that bites: two categories can legally share an item name,
|
||
so `Region = [North]` becoming `Department = [North]` leaves the labels identical
|
||
and the `CellId`s **valid and pointing at the other record**.
|
||
|
||
### a_session_that_switches_views_is_projected_the_view_it_switched_to
|
||
|
||
The original bug: `ClientSession::apply` folded straight into the session's mirror,
|
||
so a `Command::SwitchView` moved the mirrored `active_view` while the workbook's
|
||
stayed put (gotcha 29). `viewport()` then read the cursor under the new view's key
|
||
and the next `project()` built the grid from the *other* view.
|
||
|
||
improvise-jytm answers it the other way round — the mirror is now authoritative and
|
||
`grid_in` is built from it — so the assertions moved with it: the asker's mirror
|
||
names "Second", the document's default is still "Default", and the rows it is sent
|
||
are the ones "Second" has. The fixture's two views are transposes of each other, so
|
||
"was I sent the right grid" cannot be answered by accident.
|
||
|
||
Two faults injected 2026-09-20, both of which fail it. Pointing `grid_in` at
|
||
`app.view_name()` regardless of the session reproduces the original defect exactly.
|
||
Removing `ClientSession`'s `projected_view` observation is the *new* way to
|
||
reproduce it: a switch is `ViewOnly`, so nothing in `Dirty` carries it and the
|
||
session must notice its own move or never rebuild its grid at all.
|
||
|
||
#### Helper — `app_with_two_views`
|
||
|
||
Not a test — the fixture for every lockstep and rebase test in this file.
|
||
|
||
A second view whose axes are the first one's transposed, so the two grids cannot be
|
||
confused for each other: "Default" rows are regions, "Second" rows are measures.
|
||
|
||
### a_view_switch_moves_only_the_switchers_mirror
|
||
|
||
The successor to `a_view_switch_moves_every_sessions_mirror_not_just_the_switchers`,
|
||
which pinned the lockstep improvise-jytm removes. The model is shared and the UI is
|
||
not, so a bystander keeps its view, its scroll and its grid.
|
||
|
||
Three assertions, because the old bug can come back through three different doors:
|
||
the bystander's mirrored `active_view`, its `row_offset` (nonzero, so a mirror reset
|
||
shows as a move rather than coinciding with the origin), and whether `project_all`
|
||
names it at all. That third one is the sharpest — a bystander repainted for
|
||
somebody else's switch is invisible in the first two, since it would be repainted
|
||
with its *own* view's content and look right.
|
||
|
||
The scroll is drained through `project_all` before the switch, so the fan-out the
|
||
test reads names only what the switch caused.
|
||
|
||
Fault injected 2026-09-20: restoring the `adopt_active_view` loop (push the accepted
|
||
view into every mirror) fails it, as does removing the `projected_view` observation.
|
||
|
||
### a_refused_switch_corrects_the_asker_and_nobody_else
|
||
|
||
improvise-6c6q. `apply_switch` relayed `Broadcast::ToOthers`, so on the refused path
|
||
every client *but* the one that was wrong was told the accepted name. The asker's
|
||
own slice kept the name it had folded optimistically while its server-side mirror
|
||
held the accepted one — gotcha 24's mismatch, arriving through the asker's own
|
||
keystroke. `apply_switch`'s doc comment claimed the hole was closed, which is worse
|
||
than the hole.
|
||
|
||
**The test drives the asker's own `ViewState`**, folding the command the way a real
|
||
client would before handing it to the door. That is the whole cost of the ticket:
|
||
the sibling bug in `apply_model` shipped green because its test read a
|
||
"what do the others hear" helper, which looks away from the asker by construction.
|
||
There is deliberately no such helper in this test.
|
||
|
||
**The bystander is asserted from a view it can be moved OFF.** A first version left it
|
||
on the default, where a wrongly-addressed relay folds to a no-op — so the assertion
|
||
read as coverage and was vacuous, which review demonstrated by injecting `ToAll` and
|
||
watching this test pass. It now sits on "Second", and the audience itself is asserted
|
||
(`Broadcast::To(asker, …)`) rather than inferred from what somebody's slice folded
|
||
to: the audience **is** the message (gotcha 36), so it is the thing to read.
|
||
|
||
Two faults injected 2026-09-20, both of which fail it: relaying `ToOthers` (the
|
||
original improvise-6c6q bug) and relaying `ToAll` (the over-correction). The second
|
||
also fails `folding_the_relay_puts_a_refused_asker_back`, which reads frames rather
|
||
than folded state.
|
||
|
||
### deleting_the_view_this_client_is_on_moves_it_to_the_default
|
||
|
||
The host-side half of `deleting_a_view_moves_the_client_that_was_on_it_and_tells_it`,
|
||
and a desync that predates improvise-jytm: `Workbook::delete_view` moved the
|
||
document's active view and nothing moved `ViewState::active_view`, so the TUI's
|
||
cursor was read under a key naming a view that was gone. It was invisible while
|
||
`draw_status` read the workbook's copy; improvise-k69y makes it read the client's,
|
||
which is what turns it into a wrong badge.
|
||
|
||
The negative arm is the discriminating one: deleting a *different* view must leave
|
||
this client where it is. Without it, unconditionally seating the client on the
|
||
default would pass the positive assertion. The fixture therefore carries three
|
||
views, and the first delete is of the one nobody is on.
|
||
|
||
Fault injected 2026-09-20: dropping the `view_state.active_view` line from
|
||
`effect::DeleteView` fails it.
|
||
|
||
### deleting_a_view_moves_the_client_that_was_on_it_and_tells_it
|
||
|
||
A hole improvise-jytm opens and must therefore close. Before the split,
|
||
`Workbook::delete_view` moved the document's active view and `adopt_active_view`
|
||
pushed the result into every mirror, so deleting the view a client was on moved that
|
||
client. The split removes both, and without `adopt_deleted_views` the client is left
|
||
naming a view the document has not got: `App::resolve_view` falls back to the
|
||
default, so it is *projected* the default's grid while its slice says otherwise, and
|
||
`viewport()` reads its cursor under a key nothing will ever write again.
|
||
|
||
The fixture carries **three** views and the bystander sits on the one the delete does
|
||
not remove. That is the whole discrimination: a first version left the bystander on
|
||
the default, where a relay addressed to everybody folds to a no-op, and review
|
||
demonstrated the vacuity by replacing `Broadcast::To` with `ToAll` and watching the
|
||
test pass. The evidence entry had even *stated* the risk in one sentence and claimed
|
||
the assertion covered it in the next — the exact shape this file exists to stop.
|
||
|
||
So four assertions, over three different surfaces. The **audience** is read off the
|
||
`Broadcast` itself, because the audience is the message (gotcha 36). The client that
|
||
lost its view moved, in its mirror *and* in its own slice, the latter driven by
|
||
folding what the host relayed. And the bystander is still on "Third" — in its slice
|
||
and in its mirror — which only means something because there is somewhere for a
|
||
wrong relay to have moved it from.
|
||
|
||
The deletion itself is asserted, because `Workbook::delete_view` refuses to remove
|
||
the last view: a fixture with one view would pass every other line here while
|
||
deleting nothing.
|
||
|
||
Two faults injected 2026-09-20, both of which fail it: dropping the
|
||
`adopt_deleted_views` call from `apply_model`, and addressing its relay `ToAll`.
|
||
|
||
### a_session_on_another_view_is_projected_that_views_axes
|
||
|
||
The hole review found in improvise-jytm's first cut: only the **grid** half of
|
||
"every projection follows the session's view" was checked. `category_tree_in` and
|
||
`tile_bar_in` are reached through `ClientSession::view_in`, and replacing that
|
||
function's body with `app.view()` — the host's view — left the whole suite green.
|
||
|
||
The harm is worse than a wrong pixel. A category's axis is what the **tile bar** is
|
||
for: it is the control a user reassigns axes with. Under the fault a client on
|
||
another view sees the wrong axis for every category, in the widget it would use to
|
||
change them, while the grid beside it is correct — nothing to notice and nothing
|
||
that fails.
|
||
|
||
`app_with_two_views` is what makes two assertions enough: "Second" is "Default"
|
||
transposed, so reading the wrong view **swaps** Row and Column rather than blurring
|
||
them, and the test names both categories rather than one.
|
||
|
||
Fault injected 2026-09-20: `ClientSession::view_in` returning `app.view()` fails it.
|
||
|
||
### walking_the_view_history_moves_no_document_state
|
||
|
||
The other hole review found: of the three effects improvise-jytm reclassified to
|
||
`ViewOnly`, only `SwitchView` had a guard (`a_wire_view_switch_leaves_the_document_clean`).
|
||
Putting `ViewBack` or `ViewForward` back to `Model(LayoutGeometry)` left all 1313
|
||
tests green — which is gotcha 3's whole point: a mis-declared kind is silent.
|
||
|
||
It asserts the **consequences** rather than the declaration, because those are what a
|
||
reader can price. `App::apply_effects` raises `model_state.dirty` for any
|
||
`Model(_)`, and the browser worker persists on that flag — so a mis-declared
|
||
view-back rewrites the entire OPFS document (gotcha 41: `Storage` is whole-document)
|
||
for a move that changed nothing. And it bumps `layout_version`, so every *other*
|
||
client's next `Command::Model` is refused `LayoutMoved` (ADR 001) because somebody
|
||
pressed a navigation key.
|
||
|
||
The `assert_ne!` on the active view before each pair of assertions is the
|
||
check-the-checker: an empty history stack makes `ViewBack` a no-op, and a no-op
|
||
dirties nothing whatever its kind.
|
||
|
||
Both effects are driven, because they are separate `kind()` implementations and
|
||
review injected them separately.
|
||
|
||
### a_fresh_session_opens_the_documents_default_view
|
||
|
||
The other half of improvise-jytm's contract, and the reason `Workbook::active_view`
|
||
survives at all: it is what a freshly connected client opens. The fixture switches
|
||
an *existing* session first, so a `connect` that read any client's view rather than
|
||
the document's would be visible.
|
||
|
||
Fault injected 2026-09-20: pushing the accepted view into every mirror in
|
||
`apply_switch` moves the document's default nowhere, but making `apply_switch` write
|
||
`workbook.active_view` fails it.
|
||
|
||
#### Helper — `diff`
|
||
|
||
Not a test — the accessor every diff-subject test in this file reads its answer through.
|
||
|
||
The two halves of an `Applied::Diff`, for the tests whose subject is not the
|
||
rebase. It panics rather than defaulting, so a command that starts replacing the
|
||
document is a loud failure here and not a silent `Dirty::NOTHING`.
|
||
|
||
*Note:* before the comment-cap wave this helper's doc comment was preceded by an
|
||
orphaned paragraph ("One connected client, as its host really drives it…")
|
||
describing a type that no longer exists at that site. Dropped.
|
||
|
||
*Both `a_model_command_that_moves_the_active_view_binds_every_mirror` and
|
||
`a_model_commands_view_switch_reaches_the_asker_too` were retired by
|
||
improvise-jytm, along with the `to_others` helper whose blind spot the second one
|
||
existed to cover. No `ModelCommand` moves anybody's active view any more:
|
||
`ViewBack`/`ViewForward` left the wire because the stack they pop is one client's
|
||
history, and a document swap lands in the rebase arm. The property the second test
|
||
defended — a correction must reach the client that folded it — is now
|
||
`a_refused_switch_corrects_the_asker_and_nobody_else`, which drives the asker's own
|
||
slice rather than a helper.*
|
||
|
||
### a_geometry_command_invalidates_every_clients_cursor_including_the_askers
|
||
|
||
`TransposeAxes` and `CycleAxis` call `app.reset_cursor()` because renumbering every
|
||
row and column makes the old indices name different cells (gotcha 24) — but that
|
||
reset only reached the authoritative `App`'s own view slice. Each `ClientSession`
|
||
keeps its own cursor in its mirrored `ViewState`, and nothing reset those, so every
|
||
client was left indexing a grid that had changed shape under it: `Viewport::window`
|
||
clips, so what a bystander actually saw was a **blank** grid, until it happened to
|
||
scroll (improvise-gvnb).
|
||
|
||
The asker is checked alongside the bystander, and that is not symmetry for its own
|
||
sake. A `Command::Model` is the one kind a client cannot fold optimistically, so
|
||
the asker is in exactly the same position as everyone else, which is why the reset
|
||
is `Broadcast::ToAll` and not the `To(id, …)` a refused `SwitchView` correction is.
|
||
|
||
### a_model_command_that_does_not_reshape_the_grid_leaves_cursors_alone
|
||
|
||
The other arm. A model command that leaves the grid's shape alone must not throw
|
||
anyone's cursor away: where a client is looking is its own UI, and only a change to
|
||
the shared geometry may invalidate it. Without this, "reset on every model command"
|
||
would pass the test above.
|
||
|
||
### a_bystanders_own_slice_follows_the_invalidation
|
||
|
||
And the bystander really is shown the new grid from the origin, rather than a
|
||
window clipped to nothing. This is the half the mirror reset alone cannot give: the
|
||
client's own slice is driven by what the host relays, so the test drives it the way
|
||
`Host::command` does.
|
||
|
||
#### Helper — `host_dispatch`
|
||
|
||
Not a test — the relay stand-in that test and its siblings drive a client's own slice with.
|
||
|
||
A stand-in for `Host::command`'s relay, driving one client's own slice. `to` is the
|
||
client being driven and `from` the one that asked, because that pair is exactly
|
||
what `Broadcast`'s audience is about.
|
||
|
||
### a_model_command_that_leaves_the_active_view_alone_broadcasts_no_switch
|
||
|
||
Renamed in review: it was `..._broadcasts_nothing`, and since improvise-9mo9 it
|
||
asserts a `ResetCursor` broadcast, so the name read as a lie about its own
|
||
assertion. No client folded an active view, so there is no `SwitchView`
|
||
correction to make.
|
||
What the others are told is the `Dirty` it produced — and, since improvise-9mo9,
|
||
the `ResetCursor` an `AddCategory` owes, because a new category lands on an axis
|
||
in every view. The assertion is the exact broadcast list rather than "contains no
|
||
`SwitchView`": a weaker form would go on passing if the cursor invalidation were
|
||
dropped again.
|
||
|
||
### every_reshaping_model_command_invalidates_every_cursor
|
||
|
||
Only `TransposeAxes` and `CycleAxis` reset the cursor, while `SetAxis`,
|
||
`HideItem`, `ShowItem`, `ToggleGroup`, `TogglePruneEmpty`, `AddCategory`,
|
||
`RemoveCategory`, `AddItem`, `AddItemInGroup` and `RemoveItem` renumbered the
|
||
grid and did not (improvise-9mo9). `SetAxis` is the case that shows the shape of
|
||
the bug: `CycleAxis` *is* `SetAxis` to `Axis::next()`, so a pair differing only
|
||
by an argument disagreed. Because `reset_cursor` is also the only writer of
|
||
`cursor_epoch`, every one of these left the host's own cursor stale *and*
|
||
broadcast nothing, which gotcha 24 records as strictly worse than doing neither —
|
||
the diff then believes each client already holds origin-positioned content and
|
||
sends nothing, so they stay stale permanently rather than until the next scroll.
|
||
|
||
Two faults were injected. Deleting `app.reset_cursor()` from `SetAxis` fails the
|
||
bystander-cursor assertion (`left: 1, right: 0`). Changing the fixture so the
|
||
item `ShowItem` reveals was never hidden fails the *first* assertion — "ShowItem
|
||
reshaped nothing, so this case is not evidence of anything" — which is the one
|
||
that stops this from becoming a table of commands asserted against a grid that
|
||
did not move. That guard matters most for `AddItem`: the fixture puts two
|
||
categories on Row precisely so an append to the inner one inserts a row inside
|
||
every outer block, because on a single-level axis an append lands at the end and
|
||
moves nothing, and a one-level fixture would have made the reset look unearned.
|
||
|
||
#### Helper — `app_with_a_reshapeable_grid`
|
||
|
||
Not a test. Two categories on Row, one of them grouped, one item hidden and one
|
||
item left with no data — so `ShowItem` has something to reveal, `ToggleGroup` has
|
||
a group to fold and `TogglePruneEmpty` has an empty row to prune. Every command in
|
||
the table has something real to reshape, which is what the first assertion checks.
|
||
|
||
### drilling_resets_only_the_drill_views_cursor
|
||
|
||
improvise-9mo9 made `SetAxis` call `app.reset_cursor()`, and `DrillIntoCell`
|
||
issues eight or so `SetAxis` effects to build the `_Drill` view. The PR claimed
|
||
that was safe and **did not test it** — the claim was that `SwitchView` precedes
|
||
the batch, so `reset_cursor` writes the *active* view's cursor, which by then is
|
||
the freshly created `_Drill` one, while the view drilled *from* keeps its entry
|
||
in `ViewState::cursors`. That is an ordering argument about effects inside a
|
||
batch, which is exactly the class of claim gotcha 23 exists because people get
|
||
wrong.
|
||
|
||
Two faults injected. Removing the `SwitchView` from `DrillIntoCell`'s batch fails
|
||
the **first** assertion — "the drill did not switch views, so this proves nothing
|
||
about per-view cursors" — which is the guard that stops the test passing for the
|
||
wrong reason if the drill ever stops switching. Changing `reset_cursor` to clear
|
||
the whole `cursors` map rather than the active entry fails the third:
|
||
`left: (0, 0), right: (1, 1)`, the cursor in the origin view thrown away.
|
||
|
||
It asserts both sides on purpose. That the `_Drill` view opens at its origin is
|
||
half the property; that the *other* view keeps `(1, 1)` and `row_offset: 1` is
|
||
the half that would have broken, and a test asserting only the first would pass
|
||
against a map that had been emptied.
|
||
|
||
### an_accepted_no_op_invalidates_no_cursor
|
||
|
||
The third negative arm, found in review. Gating the reset on `Ok` is not the same
|
||
as gating it on "something changed", and six commands the document *accepts*
|
||
changed nothing and reset every cursor anyway: `SetAxis` to the axis already
|
||
held, `HideItem` on an already-hidden item, `ShowItem` on a never-hidden one,
|
||
`AddItem` of an existing item (`Category::add_item` dedupes and returns `Ok`),
|
||
and `RemoveItem`/`RemoveCategory` of something absent.
|
||
|
||
The first assertion is the non-vacuity guard and it is doing real work here: it
|
||
uses `grid_shape` to prove each case really is a no-op, so a fixture drifting
|
||
into one where the command *does* reshape fails rather than passes.
|
||
|
||
**Review then found five more cases the table could not reach by construction**,
|
||
because `app_with_a_reshapeable_grid` had no formula, no category on `Axis::None`
|
||
and no unknown-name case — the one-category-fixture shape the doctrine names. The
|
||
fixture now carries a `Profit` formula, a `Channel` category parked on
|
||
`Axis::None`, and a `Ghost` entry hidden under a name the model never had, and
|
||
the table covers: `SetAxis` on an unregistered category, `HideItem`/`ShowItem` on
|
||
a name the grid never drew, `ToggleGroup` on a group that does not exist, and
|
||
`AddItem { _Measure, Profit }`.
|
||
|
||
That last one is the one to remember. The guard it replaced read
|
||
`category.items`, but `_Measure`'s effective items are explicit ∪ formula targets
|
||
(gotcha 7), so it called a drawn column "new". **Two of the five injections
|
||
initially failed to reproduce, and both times the injection was wrong rather than
|
||
the guard**: the `ShowItem` case needed a hidden *ghost* name, since
|
||
`is_hidden` is false for a name nobody hid; and the `AddItem` case had to compute
|
||
its predicate **before** the add, because afterwards the item really is in
|
||
`category.items`. An injection that does not reproduce the original ordering
|
||
tests nothing — the same shape as the simulated-fix confirmation in gotcha 48.
|
||
|
||
`ToggleGroup`'s first guard was also wrong and this test caught it:
|
||
`add_item_in_group` sets `item.group` without pushing to `Category::groups`, so
|
||
asking `groups` said "no such group" about the fixture's real one.
|
||
|
||
Faults injected, two, both caught by name: removing `SetAxis`'s `moved` check,
|
||
and removing `AddItem`'s `fresh` check. The TUI repro needs no test harness —
|
||
TileSelect's `r` emits `SetAxis` with no comparison against the current axis.
|
||
|
||
### naming_a_measure_at_the_bottom_right_leaves_the_cursor_put
|
||
|
||
A regression this PR introduced and review caught. `AddItem` gained a
|
||
`reset_cursor`, and `CommitAndAdvance{Down}` on the trailing `+` column routes
|
||
`RecordCol::NewMeasure` through `add_measure` → `effect::AddItem`.
|
||
`inserts_a_column` matches only `NewCategory`, so `NewMeasure` falls through to
|
||
the advance — and `EnterAdvance` at the bottom-right returns `AbortChain`,
|
||
emitting no `SetSelected`. Nothing restored the cursor. The `SetSelected` repair
|
||
added for `+cat` covered that arm only.
|
||
|
||
The fix moves the restore **before** the advance, which is gotcha 21's rule
|
||
(tidy-up goes where an abort cannot reach it) applied to the cursor rather than
|
||
to the mode. Injecting the old ordering fails it: `left: (0, 0), right: (0, 3)`.
|
||
|
||
### a_refused_model_command_invalidates_no_cursor
|
||
|
||
The second negative arm, and the one the first version of improvise-9mo9's fix
|
||
got wrong. **It was a loop of one until review**: it named a class and tested
|
||
`AddItem` alone, and injecting `reset_cursor()` into `AddCategory`'s `Err` arm —
|
||
the very bug it was written for, one effect over — left it green. It is now a
|
||
table over all three refusable effects, and that injection fails it by name. `AddItem` and `AddItemInGroup` report a failure — an unknown category
|
||
— through `status_msg`, and the reset was written *after* the `if let Err`, so a
|
||
refused add bumped `cursor_epoch` and broadcast `Command::ResetCursor` to every
|
||
client for a change the document never made. `AddCategory` had been gated on
|
||
success in the same commit, which is what made the inconsistency visible.
|
||
|
||
`SetNumberFormat`'s arm cannot see this: it tests a command that *succeeds* and
|
||
reshapes nothing, where this tests one that reshapes nothing because it was
|
||
*refused*. Injecting the fault — moving the reset back outside the match —
|
||
fails it on the bystander's cursor. The first assertion is the non-vacuity
|
||
guard: if the category ever became valid, the case would stop being about a
|
||
refusal and the test would say so rather than pass.
|
||
|
||
#### Helper — `grid_shape`
|
||
|
||
Not a test. The `row_items`/`col_items` of the layout, as text: which cell each
|
||
position names. It is compared before and after rather than counted, because a
|
||
`cats` change can re-identify every position without changing how many there are.
|
||
|
||
### a_clients_view_only_command_moves_only_that_clients_mirror
|
||
|
||
The other half of the door: a command that is nobody else's business must stay that
|
||
way. A client's scroll must not move the authoritative `App`'s own view slice — the
|
||
model is shared, the UI is not.
|
||
|
||
### a_document_swap_reseeds_every_session
|
||
|
||
Replacing the document must clear the cursor map, because the incoming document's
|
||
view names have nothing to do with the outgoing one's (gotcha 24).
|
||
`install_workbook` does that for the `App`'s own view slice, but nothing touched the
|
||
session mirrors: a connected client went on naming the old document's view and
|
||
scrolling by the old document's cursor, so it was projected a window past the end of
|
||
a document it had never seen — an empty grid, with nothing that would ever bring it
|
||
back.
|
||
|
||
#### Helper — `other_document`
|
||
|
||
Not a test — that test's incoming document, and the one whose rename hides a bug.
|
||
|
||
A different document, with a differently-named active view, so a mirror still keyed
|
||
to the outgoing one is visible rather than coincidentally right.
|
||
|
||
### rebase_hands_every_session_a_snapshot_of_the_new_document
|
||
|
||
The reseed is reached from the projection path itself, not from a hook a host must
|
||
remember to call — `Sessions` is not reachable from the effect that installs a
|
||
workbook, so a convention there would be unenforceable. `rebase` exists for the
|
||
other half: the client's *own* view slice lives on the client, and only a fresh
|
||
`Snapshot` resets it.
|
||
|
||
### a_document_swap_over_the_wire_rebases_every_client
|
||
|
||
A document swap asked for by one client must reach every *other* client's own view
|
||
slice, and the host must not be able to miss it.
|
||
|
||
`Command::Model` put the `WholeWorkbook` mutations on the wire (improvise-kmgs), so
|
||
`LoadModel` is now something another client can ask for. The server stayed honest —
|
||
the generation bump reseeds every mirror on the next `project` — but a bystander
|
||
went on holding `cursors["Default"] = row_offset 3` from a document that no longer
|
||
exists, and `Applied { dirty, broadcast }` said nothing that would tell its host to
|
||
send a `Snapshot`. `Sessions::rebase`, the only thing that produces one, had no
|
||
non-test caller in the tree.
|
||
|
||
That is gotcha 24's cursor-vs-geometry mismatch, arriving through another client's
|
||
command — the same shape `doctrine(lockstep_binds_every_session_not_the_asker)`
|
||
names, one layer out: the obligation was addressed to a host in prose instead of
|
||
being carried by the value it gets back.
|
||
|
||
The server's own mirror self-heals off the generation bump
|
||
(`a_document_swap_reseeds_every_session` covers that half); this is the other one,
|
||
the client's slice, which only a `Snapshot` resets.
|
||
|
||
#### Helper — `a_document_shaped_differently_under_the_same_view_name`
|
||
|
||
Not a test — the fixture that test needs instead of `other_document`.
|
||
|
||
A different document whose active view has the *same name* as the outgoing one, and
|
||
a different shape: one row where the outgoing document had three.
|
||
|
||
`other_document` renames the active view, and that rename is what makes the bug
|
||
below invisible: `apply_model` broadcasts a `SwitchView` when the name moves, a
|
||
client folds it, and the stale cursor is then read under a key nothing has written
|
||
yet — right by accident. Same name, different geometry is the case with nothing to
|
||
hide behind, and it is the ordinary one: two revisions of a document both call
|
||
their first view `Default`. (Gotcha 39's fixture warning.)
|
||
|
||
#### Helper — `app_over_a_loadable_document`
|
||
|
||
Not a test — the `App` that makes a wire `LoadModel` in that test really replace the document.
|
||
|
||
An `App` whose storage already holds that document, so a `Model(LoadModel)`
|
||
arriving over the wire really does replace the document rather than setting an
|
||
error status.
|
||
|
||
### a_command_that_leaves_the_document_in_place_is_not_a_rebase
|
||
|
||
The other arm of the same rule, so the check is not one that always says "rebase"
|
||
(`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`). A model
|
||
command that mutates the document in place, and a view-only one, are both diffs —
|
||
the question `apply_model` asks is whether the generation moved, not which variant
|
||
arrived.
|
||
|
||
### ui::project::tests::a_load_that_fails_is_not_a_rebase
|
||
|
||
Qualified because `ui::host::tests` has a test of the same name: this is the
|
||
`apply_model` one, and a bare leaf was satisfiable by either.
|
||
|
||
A `LoadModel` naming a document the storage does not hold leaves the document
|
||
alone, so it is a diff too. This is why `apply_model` asks the generation rather
|
||
than the variant: "is this one of the three `WholeWorkbook` commands" is not the
|
||
same question as "was the document replaced", and a client would be reseeded onto
|
||
the document it was already holding, losing its cursors to a failed load.
|
||
|
||
#### Note — the `cell_identity` module
|
||
|
||
Emitting and resolving the surrogate a client names a cell by (ADR 002). The two
|
||
halves are tested together on purpose: an id that is emitted and never resolved, or
|
||
resolved by re-deriving it the same way it was emitted, would agree with itself
|
||
while agreeing with nothing else.
|
||
|
||
### the_two_measures_of_a_row_share_a_record_and_differ_by_measure
|
||
|
||
Gotcha 1's unit, projected: two measure columns of one row are one record and two
|
||
cells. Both halves matter — a surrogate that also varied by measure would break
|
||
`record_versions`, and one that did not vary by measure at all would make the two
|
||
columns the same cell.
|
||
|
||
### a_projected_record_id_resolves_to_the_cell_it_was_projected_for
|
||
|
||
The round trip that makes the surrogate usable: what the server projected for a
|
||
position resolves back to the key that position has. Every row is checked, because
|
||
one row cannot tell "resolves correctly" from "resolves to the first record".
|
||
|
||
#### Helper — `raw_app`
|
||
|
||
Not a test — the fixture for every stored-cell surrogate test in `cell_identity`.
|
||
|
||
`Region` on Row, `_Measure` on Column. Nothing regular sits on None, so every cell
|
||
is a stored cell. Three rows and two measure columns, so "the record" cannot be
|
||
confused with "the row" or with "the cell".
|
||
|
||
### a_projected_aggregate_id_resolves_to_the_same_constraint
|
||
|
||
An aggregate names a coordinate *constraint*, and it must come back as the same
|
||
constraint — including the page and filter coordinates baked into the layout's key
|
||
(gotcha 27), which is why this resolves against `cell_key` rather than against the
|
||
row and column labels.
|
||
|
||
#### Helper — `aggregated_app`
|
||
|
||
Not a test — the fixture for the aggregate half of `cell_identity`.
|
||
|
||
The same, with a second *regular* category left on None, so every visible cell sums
|
||
over `Product` rather than being one stored cell. That is the fork `EditOrDrill`
|
||
makes, and the one the wire projects.
|
||
|
||
### a_surrogate_for_a_removed_row_fails_to_resolve_and_the_others_do_not
|
||
|
||
The first arm of staleness. The identifiers are held constant and only the thing
|
||
that makes the id stale is varied (`doctrine(a_rename_can_hide_a_stale_key)`):
|
||
"East" is removed, so its surrogate must fail to resolve — while "North"'s, taken
|
||
from the same projection, still resolves to the same cell. Without that second
|
||
assertion a `resolve_cell` that returned `None` for everything would pass.
|
||
|
||
### an_empty_cell_can_be_written_through_its_projected_id
|
||
|
||
improvise-1ey is "type a number into a cell and see the value update", and the cell
|
||
a user types into is usually **empty** — so it is named by the grid and by nothing
|
||
in the store. The index cannot answer it, which is why resolution falls back to the
|
||
layout, and this is the case that makes that fallback load-bearing rather than
|
||
defensive.
|
||
|
||
### a_write_by_surrogate_lands_on_exactly_that_cell
|
||
|
||
Two rows and two measure columns, so a write that went to "the first cell", "the
|
||
whole row" or "the whole column" is visible.
|
||
|
||
### a_write_through_a_dead_surrogate_changes_nothing
|
||
|
||
The second half of "reject, nothing to write to": a surrogate that does not resolve
|
||
must apply *nothing*, not fall back to some other cell. The document is compared
|
||
whole, because a write to the wrong place is exactly the failure being excluded.
|
||
|
||
It goes through `model_effects` rather than the wire form, because the session door
|
||
reaches the same conclusion first and typed — `Checked::against` refuses this
|
||
command with `Rejection::RecordGone`, so a `Checked` over it does not exist. What
|
||
is asserted here is the **floor underneath** that door: the key path and any host
|
||
resolving a command locally get an empty batch rather than a misdirected write.
|
||
|
||
### a_records_mode_cell_is_named_by_its_coordinates_not_by_its_position
|
||
|
||
The trap gotcha 1 sets. `GridLayout::cell_key` returns a synthetic `(_Index, _Dim)`
|
||
key here, carrying *positions* — the very thing ADR 002 rejected `_Index` for,
|
||
because position names a different record after a filter, an insert or a delete.
|
||
The projected id must come from `RecordRow::coords` instead, which is what
|
||
resolving it back to the real cell proves.
|
||
|
||
#### Helper — `records_app`
|
||
|
||
Not a test — the fixture for every records-mode surrogate test.
|
||
|
||
Wide format: one row per coordinate tuple, a column per raw measure, the coordinate
|
||
columns in front and a `+` column behind. Two records and two measures, so neither
|
||
"the row" nor "the column" can stand in for the record by accident.
|
||
|
||
### the_measure_columns_of_a_record_row_share_its_record
|
||
|
||
Two measure columns of one row are two cells of one record, and the next row is a
|
||
different record — the same shape the pivot case has, checked here because records
|
||
mode reaches it by a different path.
|
||
|
||
### a_coordinate_column_is_projected_with_no_identity
|
||
|
||
A coordinate column is not a cell to write into: editing it renames that coordinate
|
||
across *every* measure cell in the row (gotcha 1), which is a different intent from
|
||
`SetCell`. So it is projected with no id at all, and a client cannot compose a
|
||
write against it.
|
||
|
||
### a_write_moves_the_version_under_the_clients_own_surrogate
|
||
|
||
The version counter is keyed by the same surrogate the client holds, which is what
|
||
lets one identity answer both ADR 002's question and ADR 001's. A write must move
|
||
the counter the client would check.
|
||
|
||
#### Helper — `apply_caught_up`
|
||
|
||
Not a test — and note it is `#[cfg(test)] fn`, not `#[test] fn`.
|
||
|
||
`Sessions::apply` for a client that is fully caught up, for the tests whose subject
|
||
is not the basis. The staleness tests build the `Basis` by hand, so neither kind
|
||
can pass by accident through the other's convenience.
|
||
|
||
---
|
||
|
||
#### Note — the `basis_check` module
|
||
|
||
ADR 001's basis check: a batch composed against a picture of the document that has
|
||
since moved is refused, and one composed against the current picture is not
|
||
(improvise-xwi6, improvise-fkfg).
|
||
|
||
**Both arms, per tier.** A check nobody has watched *reject* is indistinguishable
|
||
from one that always accepts, and a check nobody has watched *accept* is
|
||
indistinguishable from one that always rejects (`doctrine(check_the_checker)`). So
|
||
every tier appears twice.
|
||
|
||
The fixtures hold every identifier constant and vary only what makes the basis
|
||
stale (`doctrine(a_rename_can_hide_a_stale_key)`): the record a batch names is
|
||
projected once and reused, so a rejection can never be an artifact of the test
|
||
naming something different the second time.
|
||
|
||
### a_cell_edit_is_rejected_when_the_layout_moved_under_it
|
||
|
||
**The class-2 case, and the one test to keep if only one survives.**
|
||
|
||
A `CellData` batch is stale when the *layout* moved, even though no record it names
|
||
was written: the coordinates its key hangs off changed underneath it. This is A
|
||
removing an item while B commits an edit keyed by it, and it is corruption rather
|
||
than a lost keystroke — which is exactly what a cell-level lock would not have
|
||
covered, because holding a cell does not stop anyone deleting the row.
|
||
|
||
#### Helper — `basis_for`
|
||
|
||
Not a test — the claim every `basis_check` test moves exactly one tier of.
|
||
|
||
The basis a caught-up client holds for one record. Built by hand rather than
|
||
through `caught_up` so each test can move exactly one tier of it and leave the rest
|
||
true.
|
||
|
||
### a_cell_edit_composed_after_the_reshape_is_applied
|
||
|
||
The other arm of the same tier. A batch composed after the reshape is applied — so
|
||
the rule is "the layout moved since you looked", not "the layout has ever moved".
|
||
|
||
### a_write_is_rejected_when_the_record_it_names_moved
|
||
|
||
Class 1: somebody wrote the record since this client read it. **Data** staleness —
|
||
the record still resolves, so force is coherent and the reason says so. The
|
||
identifiers do not move, only the value, so a rejection cannot be a renamed key in
|
||
disguise.
|
||
|
||
### a_write_survives_another_record_moving
|
||
|
||
The arm that keeps the tier from being "reject whenever anything was written". A
|
||
record moving does not invalidate a batch that names a *different* one — which is
|
||
the whole reason the tier is per-record rather than a single counter.
|
||
|
||
### a_write_to_a_record_that_no_longer_exists_says_so
|
||
|
||
Class 2 by the other route, and the reason ADR 002 put a *maintained* index in
|
||
`DataStore` rather than regenerating one from the layout: "the record is gone" and
|
||
"the record changed" must not collapse into one answer. There is nothing to write
|
||
to here, so nothing to force — force never creates structure.
|
||
|
||
The test brings the layout claim up to date so that, whatever kind `RemoveItem`
|
||
wears, only the record tier is left to answer. It is `CellData` today — removing an
|
||
item drops its cells and moves no view's axes — so that line is currently a no-op;
|
||
it is kept because a reclassification to `LayoutGeometry` would otherwise reject on
|
||
the tier above and hide the case this test is about, silently turning it into a
|
||
second copy of `a_cell_edit_is_rejected_when_the_layout_moved_under_it`.
|
||
|
||
### a_claim_that_omits_the_record_it_writes_is_not_a_free_pass
|
||
|
||
The force-by-omission hole, closed. Which records are checked is the **server's**
|
||
decision, read off the command, so a client that simply left its record out of the
|
||
claim does not skip the tier — it is told it never stated one. Without this,
|
||
ADR 001's "force is a re-composition, not a bypass" would have a bypass sitting in
|
||
the message format.
|
||
|
||
### a_document_swap_invalidates_every_claim
|
||
|
||
The top tier invalidates everything, whatever the other two say. It is checked
|
||
first so the answer names the largest thing that moved: a client told "a record
|
||
moved" after a document swap would go looking for a record that is not there.
|
||
|
||
### a_rejected_batch_changes_nothing_anywhere
|
||
|
||
Rejection is a *normal outcome*: nothing is applied, nobody else hears about it,
|
||
and no basis moves. A rejection that dirtied something would tell every other
|
||
client to redraw for a change that did not happen.
|
||
|
||
### a_published_basis_follows_the_grid_updates_it_describes
|
||
|
||
The order `ClientSession::basis_update` claims is load-bearing, pinned.
|
||
|
||
The basis names record counters for the cells the grid updates in the same batch
|
||
carry, so a `Projection::Basis` that overtook them would let a client claim a
|
||
version for a value it had not been shown — the mirror image of gotcha 33's rule
|
||
that labels precede the cells they frame, and unlike that rule it had nothing
|
||
holding it but the order of two statements in `project_diff`. Proved by injecting
|
||
the swap (gotcha 43).
|
||
|
||
### a_client_that_folds_the_stream_composes_an_accepted_batch
|
||
|
||
The check is only satisfiable because the server *publishes* the basis — which
|
||
ADR 001 does not mention, and without which a client could only forge one. So: a
|
||
client that folds what it is sent can compose a batch that is accepted, end to end,
|
||
with no test-only constructor anywhere in the path.
|
||
|
||
### re_composing_against_the_republished_basis_is_accepted
|
||
|
||
And the other side of publication: after somebody else writes, the stream carries a
|
||
*new* basis, so the client that folds it can re-compose and be accepted. That is
|
||
ADR 001's force — a re-composition against the current basis — reachable without
|
||
any mechanism that skips the check.
|
||
|
||
## `ui/host.rs`
|
||
|
||
### connecting_hands_that_client_a_snapshot_that_draws_the_document
|
||
|
||
AC 2: the bootstrap is sent on connect, addressed to the new client, and it is not
|
||
empty — a `Snapshot` whose seed carried nothing would satisfy "a snapshot was sent"
|
||
while leaving the client blank.
|
||
|
||
### the_reported_extent_bounds_the_snapshot
|
||
|
||
The extent is what the snapshot is built at, so a client that says it can see one
|
||
row is not sent two. This is the property that makes `Viewport` the handshake rather
|
||
than an afterthought.
|
||
|
||
### without_the_command_the_cell_is_empty
|
||
|
||
`doctrine(check_the_checker)`: the preceding assertion would also pass if the seed
|
||
happened to contain the value already. Inject the fault — never send the command —
|
||
and it must fail.
|
||
|
||
### a_bystander_is_projected_another_clients_edit
|
||
|
||
A cell edit is the whole document's, so a client that did not ask for it is told
|
||
about it too. The projection is per-session because the viewports differ, not
|
||
because the change does.
|
||
|
||
#### Helper — `model`
|
||
|
||
Not a test — and note it is `#[cfg(test)] fn`, not `#[test] fn`.
|
||
|
||
A model command from a client that is fully caught up, for the tests whose subject
|
||
is not ADR 001's basis. It goes through `handle` like any other frame; only the
|
||
basis is supplied rather than folded, and `caught_up` can do that because it holds
|
||
the `App` (a client cannot). The staleness tests build their own `Basis`.
|
||
|
||
#### Helper — `addressed`
|
||
|
||
Not a test — how every `host.rs` test names a cell the way a client would.
|
||
|
||
The same cell as a client names it: the opaque surrogate the server projected on
|
||
`CellDisplay::id`, which is the only handle a client has (ADR 002). A `CellKey` is
|
||
not constructible over there.
|
||
|
||
### a_scroll_reaches_only_the_client_that_scrolled
|
||
|
||
A view-only command reaches nobody else. The counterpart to the test above: if every
|
||
command fanned out, that one would pass for the wrong reason.
|
||
|
||
### a_batch_of_commands_is_applied_whole_and_in_order
|
||
|
||
One keypress resolved on a client is up to three commands, and the host must take
|
||
all of them (improvise-bpq2).
|
||
|
||
The fixture is the shape that actually arrives: a cursor moved past the bottom of a
|
||
one-row window, so the selection *and* the offset that follows it travel together.
|
||
The assertion is the offset, not the selection, because a host that applied only the
|
||
first command would still look right on `selected` — the window is what goes wrong,
|
||
and it goes wrong silently.
|
||
|
||
### a_batch_is_abandoned_at_the_first_rejection
|
||
|
||
A refused batch applies nothing *after* the refusal. The stronger half — nothing
|
||
*before* it either — is `a_refused_batch_leaves_the_commands_before_it_unapplied`;
|
||
this is kept because the two halves fail independently.
|
||
|
||
### a_refused_batch_leaves_the_commands_before_it_unapplied
|
||
|
||
improvise-955n / improvise-08wb. **The bug**: ADR 001 is emphatic that a stale batch
|
||
is never applied, *not even partially*, and improvise-ondj went to the trouble of a
|
||
type (`Checked`) to make the unchecked path unconstructible. But `Checked` is **per
|
||
command**, and a `ClientFrame::Commands` is a sequence of them: `Host::commands`
|
||
applied in order and abandoned at the first rejection, so a frame whose *second*
|
||
command was refused had already applied its first. The guarantee was true of a
|
||
single command and false of a batch.
|
||
|
||
The earlier command is a **view** command, which is not a concession: it is the
|
||
shape the one real producer emits (`keys::commit` — one mutation among view
|
||
commands), and it is still observable, because the host folds it into that client's
|
||
mirror. A frame may carry more than one mutation; what it may not do is apply some
|
||
of them and refuse the rest, which is clause 4 and open.
|
||
|
||
**A refusal test needs an accepting one**
|
||
(`doctrine(a_helper_that_builds_the_world_the_code_wants_tests_nothing)`: pair a
|
||
refusal with an acceptance, or the refusal can pass by refusing everything). The
|
||
second half runs the same leading `SetSelected` followed by a command whose basis is
|
||
*current*, and requires both to land. Without it, a `SetSelected` that simply never
|
||
worked would clear the first assertion and prove nothing at all.
|
||
|
||
Inside it, the stale claim is built from `caught_up` and then has a *single* tier
|
||
bumped, so the refusal can only be about the tier that was moved — a hand-built
|
||
basis that is stale in three ways at once cannot tell which check answered.
|
||
|
||
### a_later_write_in_a_chain_still_answers_for_its_own_record
|
||
|
||
improvise-955n. **The regression 90ea26e introduced, and the reason clause 2 cannot
|
||
be applied to all three tiers.**
|
||
|
||
Hoisting the basis check to the frame hoisted *all* of it, so every mutation after
|
||
the first was admitted with no record check at all. Two clients, one record: A
|
||
composes `[SetCell(North), SetCell(South)]` against an honest current basis, B
|
||
writes South, A's frame arrives — and A's write to South landed on top of B's
|
||
**silently**, which is exactly the lost update ADR 001's record tier exists to
|
||
prevent. At the base commit the second command was checked and the frame refused.
|
||
|
||
**The per-command check was doing two jobs and the commit noticed one.** "Is the
|
||
client's picture of the *document* current" is a frame-level fact and hoists
|
||
correctly. "Is the client's picture of *this record* current" is not: which records
|
||
are at stake is a property of the command, which is why `records_named` is an
|
||
exhaustive match over `ModelCommand` (gotcha 43). The incapability argument that
|
||
motivated clause 3 is true of records a frame **creates** and was over-generalised
|
||
to records it merely **writes** — and those are the ones the tier is for.
|
||
|
||
Three arms, because a fix that refuses everything would pass the first alone and a
|
||
fix that checks nothing would pass the last two.
|
||
|
||
In arm 2, B moves South with a `SetCell`, which is `CellData`: neither the document
|
||
nor the layout tier moves, so only the record tier can discriminate — which is what
|
||
makes this a test of the record tier and not of the hoist in general.
|
||
|
||
**Arm 3 — the same record twice in one chain.** Legitimate and likely (two measures
|
||
on one record is one record) and it works for a reason worth pinning rather than
|
||
rediscovering: **every check happens before any command is applied**, so the second
|
||
write is compared to the record's *pre-frame* counter, not to the one its own
|
||
sibling just bumped. Were the checks interleaved with the applies, this frame would
|
||
refuse itself with `RecordMoved`.
|
||
|
||
**Read this before closing clause 4.** Interleaving is tempting there, because it
|
||
would close the *silence*: `[RemoveItem(North), SetCell(North)]` under interleaving
|
||
is refused `RecordGone` instead of silently half-applying, which is half of what
|
||
clause 4 owes. So check-before-apply is not a free win — it buys the legitimate
|
||
double-write and **sells** the loud failure on a broken chain, and clause 4 has to
|
||
pay that back some other way. Measured rather than argued: injecting the
|
||
interleaving fails exactly two tests, this one and
|
||
`a_frame_whose_later_mutation_fails_is_not_yet_undone`, and the second failing *is*
|
||
the trade. Arm 3 is what stops someone taking the trade without noticing they made
|
||
it.
|
||
|
||
### a_data_dependent_chain_lands_in_order
|
||
|
||
**The capability the contract is for**: a chain of data-dependent commands, applied
|
||
in order, each seeing what the previous one left.
|
||
|
||
`[AddItem("East"), SetCell(the East record)]` is the archetype, and it is the case a
|
||
per-command basis check **cannot** admit however it is ordered: a client cannot
|
||
claim a counter for a record its own chain is about to create, because
|
||
`BasisCache::claim` filters to records the cache has been told about. So the basis
|
||
is answered once, on the first command that carries one, and the rest of the chain
|
||
runs against what its predecessors left.
|
||
|
||
The second assertion is the one that earns the test. Asserting only that the item
|
||
exists would pass against a host that applied the first command and dropped the
|
||
second — which is exactly what the previous rule did, loudly, and what the pre-frame
|
||
check did, silently.
|
||
|
||
### a_frame_whose_later_mutation_fails_is_undone_whole
|
||
|
||
**ADR 001 clause 4, delivered (improvise-yqpd). This entry keeps the hole it
|
||
characterized, because the inversion is only legible against it.**
|
||
|
||
The three assertions flipped. The removal is **undone**, the cell reads its
|
||
**pre-frame** value — `Some(Number(1.0))`, which is neither `5.0` (the frame's own
|
||
write landing) nor `None` (the removal surviving), and that middle value is the
|
||
whole difference between a rollback and an abort — and the frame is **refused out
|
||
loud** as `Rejection::ChainBroken`. A fourth assertion was added that the previous
|
||
version could not have: the **record counter** goes back too, or the client's next
|
||
batch is refused for a write that never happened.
|
||
|
||
**The mechanism, and where it deviates from the ticket.** The ticket asked for
|
||
snapshot-and-restore at `App::apply_effects`. What landed is `App::snapshot_document`
|
||
/ `App::restore_document` — the workbook, its dirty marker and the two counted tiers,
|
||
the record tier riding inside the workbook's `DataStore` — driven from
|
||
`Host::commands`, which takes the snapshot before the frame and restores it on
|
||
failure. It is **not** taken inside `apply_effects` itself, and the reason is cost:
|
||
`apply_effects` runs on every keypress, and an unconditional `Workbook` clone there
|
||
would put a full document copy on the held-key path that gotcha 24's `mcq4` work
|
||
exists to keep cheap. The primitive is on `App`, so a future `RunScript` opens the
|
||
same transaction; only the *trigger* is at the frame.
|
||
|
||
**What counts as a failure, and the split the ticket did not name.** Two things:
|
||
an effect that sets `abort_effects` (`Rejection::MutationRefused`), and a command
|
||
whose record no longer resolves (`Rejection::ChainBroken`). The second is the
|
||
interesting one. The ticket says interleaving the record check with the applies
|
||
would close clause 4's silence for free but would sell the legitimate double-write —
|
||
a frame writing one record twice works only because the second write is compared to
|
||
the record's *pre-frame* counter. That is true of the **counter** half of the record
|
||
tier and **not** of the **structural** half: writing a record twice cannot make it
|
||
stop resolving. So the record tier splits — *does it resolve* is asked again before
|
||
every mutation (`reduce::broken_chain`), *is its counter what you claimed* stays
|
||
pre-frame — and clause 4 gets its loud failure without paying the price the ticket
|
||
costed. `a_later_write_in_a_chain_still_answers_for_its_own_record` arm 3 still
|
||
passes, which is the guard that the trade was not taken by accident.
|
||
|
||
**Injected faults, each watched to fail.** Making `undo_frame` restore nothing
|
||
fails this test alone. Making `broken_chain` return `None` unconditionally fails
|
||
this test and the mirror test.
|
||
|
||
**A first version of this entry claimed `broken_chain`'s create-exemption was
|
||
"correctness-preserving but not currently observable", and that was wrong in both
|
||
halves.** Review built the fixture it said did not exist and found the exemption
|
||
was *swallowing a write* — see
|
||
`a_frame_whose_write_would_land_nowhere_is_refused_rather_than_dropped`. The
|
||
exemption is gone from `broken_chain` and stays in the pre-frame check, where it
|
||
belongs. The lesson worth keeping: **"no test fails when I remove it" is evidence
|
||
the tests are thin, not evidence the code is inert** — the honest next step is to
|
||
build the discriminating fixture, and the reasoning that excused not building one
|
||
here ("`resolve_record` falls back to the layout, so the case cannot arise") was
|
||
an argument about the *common* case presented as an argument about all of them.
|
||
|
||
---
|
||
|
||
The hole this replaced, kept because the inversion is only legible against it:
|
||
|
||
The contract says a frame is all-or-nothing. It was not: `App::apply_effects`
|
||
aborts without undoing (gotcha 21 — an abort is not a rollback), so a chain whose
|
||
later mutation fails leaves the earlier one standing.
|
||
`[RemoveItem("North"), SetCell(the North record)]` is the shape: the removal lands,
|
||
the write then resolves to nothing, `model_effects` returns an **empty** batch, and
|
||
the frame is reported accepted having applied half of itself. Both the partial
|
||
application and the silence are open.
|
||
|
||
**This test did not pin that, and the way it failed is the repo's own signature
|
||
defect sitting on the one test whose job is to fail loudly if anyone believes the
|
||
hole is closed.** It asserted that North was removed and that nothing was rejected —
|
||
both true of any *wholly successful* frame. The independent reviewer retargeted the
|
||
second command from the removed North to the live South, making the frame succeed
|
||
completely, and the test still passed. It could not tell half-applied from
|
||
fully-applied, so it would never have inverted when clause 4 landed; it would have
|
||
gone on passing. The missing assertion is the one that names the hole: **the second
|
||
mutation did not land.**
|
||
|
||
It is unreachable from any client here — the only producer of a multi-command frame
|
||
is `keys::commit`, which emits one mutation among view commands — which is what
|
||
makes it safe to document rather than rush.
|
||
|
||
The frame claims North at its current counter, so it passes the check and fails at
|
||
*apply* time — which is the only way to reach the hole. A frame refused by the check
|
||
never applies anything and would be testing clause 2 instead. That fixture note
|
||
survives the inversion unchanged: it is still the only route to the arm.
|
||
|
||
### a_rolled_back_load_leaves_no_ghost_view_and_no_false_status
|
||
|
||
**Clause 4 was delivered for the document and the mirrors and NOT for the host's
|
||
own session slice**, found by an integration review of the first two commits.
|
||
`App::restore` said `view_state` was "deliberately untouched", citing
|
||
improvise-yqpd's design note: *`AddCategory` writes its refusal into
|
||
`view_state.status_msg` and THEN aborts, so rolling that back would undo the
|
||
explanation along with the damage.*
|
||
|
||
**That note is about an `apply_effects`-level rollback, which is not what
|
||
landed.** At the *frame* level the asker receives `ServerFrame::Rejected(reason)`
|
||
— the explanation is delivered on the wire, addressed to the client that earned
|
||
it — so the host's status line has no explanatory duty for a client's frame. The
|
||
note still governs the host's own key path, which aborts without undoing exactly
|
||
as gotcha 21 describes, and is untouched here.
|
||
|
||
Three measured consequences of a refused frame, all now closed: the TUI host's
|
||
cursor still moved and `cursor_epoch` was still bumped; a rolled-back `LoadModel`
|
||
left `view_state.active_view` naming a view of the **discarded** document; and
|
||
`status_msg` kept the rejected frame's sentence, so **a frame that did nothing
|
||
claimed it had loaded a file**.
|
||
|
||
**The fixture had to be renamed before the test could see any of it.**
|
||
`host_over_a_loadable_document` built its incoming workbook with
|
||
`Workbook::new`, whose default view is `Default` — the same name the outgoing
|
||
document uses — so a ghost `active_view` was indistinguishable from a correct
|
||
one. This is gotcha 39's trap one level up, and the incoming view is now
|
||
`Loaded`. Checked first: neither existing user of that fixture depends on the
|
||
names matching, and gotcha 39's own same-name fixture is
|
||
`a_document_shaped_differently_under_the_same_view_name` in `project.rs`, which
|
||
is untouched.
|
||
|
||
Why restoring the whole slice is safe rather than clobbering a TUI user's
|
||
in-progress typing: `Host::commands` is synchronous, so the snapshot and the
|
||
restore bracket **only the frame's own writes**. There is no "in between" for a
|
||
keystroke to land in.
|
||
|
||
Injected fault: dropping `self.view_state = snapshot.view_state` from
|
||
`App::restore` fails this test and only this test.
|
||
|
||
### a_refused_mutation_rolls_back_its_predecessor_and_the_abort_flag
|
||
|
||
The `Rejection::MutationRefused` arm — the one the first hand-back wrongly called
|
||
unreachable — plus the batch-outcome flags going back with everything else.
|
||
|
||
**`batch_aborted` was left `true` after a rolled-back frame.** Inert at the time,
|
||
since every `Command::Model` resets it at the top of `apply_effects`, but it is
|
||
the same shape as the stale-flag bug two commits earlier and `App::restore` is its
|
||
natural home. `App`'s own doc comment names four more "last batch" fields and
|
||
says `last_reshaped_views` is "the shape `batch_aborted` takes"; `restore` was
|
||
clearing three of the four and neither snapshotting nor restoring any of them.
|
||
All six now round-trip through `AppSnapshot`, which is why the type was renamed
|
||
from `DocumentSnapshot` — it is no longer only the document.
|
||
|
||
**The fixture detail that matters**: a *duplicate* `AddCategory` is idempotent, so
|
||
the obvious way to reach the refusal proves nothing. This fills to
|
||
`MAX_CATEGORIES` (12) and overflows. And the predecessor is an `AddItem` rather
|
||
than a `SetCell`, because adding eleven categories reshapes the view enough that
|
||
a record-addressed predecessor is refused `RecordGone` **first** — hiding the arm
|
||
under test behind an earlier one.
|
||
|
||
Injected fault: dropping `self.batch_aborted = snapshot.batch_aborted` fails this
|
||
test and only this test.
|
||
|
||
### a_frame_whose_write_would_land_nowhere_is_refused_rather_than_dropped
|
||
|
||
**Found by reviewing improvise-yqpd's own work, and it was a hole inside the fix
|
||
for the hole.** The first version of `reduce::broken_chain` carried a
|
||
create-exemption — the same `will_be_created_by_the_frame` predicate the pre-frame
|
||
check uses — on the reasoning that the two must not drift. The hand-back recorded
|
||
that arm as "correctness-preserving but unobservable", because removing the filter
|
||
failed no test.
|
||
|
||
That reasoning was wrong, and the fixture that shows it is the one the hand-back
|
||
said did not exist. `[AddItem("West"), HideItem("West"), SetCell(West)]` observed
|
||
**`rejected=false landed=false`**: the frame reported accepted, with the write
|
||
silently gone — *precisely* the partial-and-silent failure clause 4 exists to
|
||
eliminate, surviving inside clause 4's own implementation.
|
||
|
||
**The argument that settles it.** `App::resolve_cell` is gated by
|
||
`resolve_record`, so `resolve_record(hash).is_none()` is exactly the condition
|
||
under which `model_effects` returns an empty vec and the write vanishes. And
|
||
`SetCell`/`ClearCell` are the only variants `records_named` names, so there is no
|
||
command for which a non-resolving record still does something useful. Refusing is
|
||
therefore strictly better than dropping, everywhere.
|
||
|
||
**Scope: this closes the record-addressed INSTANCE, not the class.**
|
||
`broken_chain` asks `records_named`, which names only `SetCell`/`ClearCell`, so a
|
||
frame can still apply partially and silently through a command that names no
|
||
record. Two measured as accepted: `[DeleteView("Scratch"),
|
||
SetNumberFormat{view:"Scratch"}]`, where the effect runs and no-ops inside
|
||
`apply`; and `[RemoveCategory("Region"), AddItem{category:"Region"}]`, where the
|
||
effect *fails* and writes its error to the **host's** status line without
|
||
aborting, so the client is never told. Closing the class needs effects to report
|
||
failure rather than narrate it — roughly fifty of them — which is a different
|
||
change from clause 4 and is stated here rather than implied.
|
||
|
||
**The exemption belongs at CHECK time and nowhere else**, and that asymmetry is
|
||
the point: before the frame runs, a record the frame will create legitimately does
|
||
not exist, so `[AddItem(East), SetCell(East)]` must pass — `kb.pl`'s
|
||
`the_create_exemption_is_a_property_not_a_list` is unchanged and still describes
|
||
`stale_records_in_a_frame`. At apply time the record has either been created or it
|
||
has not, and "has not" means the write is lost.
|
||
|
||
Injected fault: restoring any exemption to `broken_chain` fails this test, plus
|
||
`a_frame_whose_later_mutation_fails_is_undone_whole` and the mirror test.
|
||
|
||
### a_frame_of_view_commands_is_not_refused_by_an_earlier_batchs_abort
|
||
|
||
**The second review finding, and a live bug improvise-yqpd introduced.**
|
||
`App::batch_aborted` is reset *only* inside `apply_effects` (gotcha 21), and
|
||
`Host::commands` read `last_batch_aborted()` after **every** command in a frame.
|
||
A command that runs no effects — every view command, and `Command::SwitchView`,
|
||
which `apply_checked` routes to `apply_switch` rather than through
|
||
`apply_command` — leaves the flag exactly as some earlier batch left it.
|
||
|
||
So a TUI `:w /bad/path` raised the flag, and the next frame a connected browser
|
||
sent was refused `MutationRefused` and rolled back, having done nothing wrong.
|
||
Reproduced by aborting a pathless `effect::Save` directly on the `App` and then
|
||
sending `[SetSelected]`.
|
||
|
||
**Two arms, because frame-level gating alone is not the fix.** The first arm is a
|
||
pure view frame, which the `Command::Model`-presence gate in `commands` handles.
|
||
The second is the **mixed** frame `[SetSelected, SetCell]`, which that gate lets
|
||
through and which only the per-command `mutation &&` guard answers — the view
|
||
command runs no effects, so the flag under it is still the stale one. Injecting
|
||
`if { let _ = mutation; self.app.last_batch_aborted() }` fails this test and only
|
||
this test, which is what shows the per-command guard is load-bearing rather than
|
||
belt-and-braces.
|
||
|
||
The same `Command::Model`-presence predicate is why a frame of pure cursor keys
|
||
now pays for neither the `Workbook` snapshot nor the `Sessions` clone — the
|
||
browser's common path, one frame per keypress.
|
||
|
||
### the_mirror_settles_its_mode_against_every_grid_it_is_later_projected
|
||
|
||
improvise-el9h. `ClientSession::fold` settled `view.mode` against `self.grid` — the
|
||
grid that session was **last projected** — and `project_diff` then assigned
|
||
`self.grid = desired` and never settled again. The client does not have that shape:
|
||
`ClientState::fold` calls `settle_mode()` on every fold including
|
||
`Command::Project`, so when the new view's grid arrives the client re-settles and
|
||
the mirror does not.
|
||
|
||
`[SwitchView(records-shaped view), ChangeMode(Normal)]` is the shape — precisely the
|
||
case PR #98's settle exists to serve. **Demonstrated before the fix**: the client
|
||
ended on `RecordsNormal` and the mirror on `Normal`. The fix is `settle_mode` called
|
||
from `fold` *and* at the end of `project_diff` once the grid has moved, which also
|
||
closes the three bypass doors (`apply_switch`, `adopt_deleted_views`,
|
||
`adopt_cursor_epoch` write the mirror with bare `reduce_view`): whatever they leave,
|
||
the next projection settles.
|
||
|
||
**Two preconditions, and one of them caught a broken fixture on its first run.** The
|
||
test asserts the client starts on `Normal`, so the settle has somewhere to move the
|
||
mode *from*. And `app_with_a_records_shaped_second_view` asserts the second view
|
||
really reaches records mode — which failed immediately, because the view was built
|
||
with `View::new` and a fresh `View` carries no axis entry for `_Index`/`_Dim`, so
|
||
`is_records_mode` can never fire on one. Without that assertion the test would have
|
||
compared two modes that were both `Normal` for a reason having nothing to do with
|
||
the bug, and passed against the unfixed code.
|
||
|
||
The yardstick is a real `improvise_protocol::ClientState` fed the same projections
|
||
in arrival order, not a hand-written expectation: the property is that the mirror
|
||
and the client run the same function over the same stream.
|
||
|
||
### a_failed_frame_puts_the_askers_mirror_back_on_the_view_it_was_on
|
||
|
||
Clause 4's least obvious cost, in the ticket's own words: **rolling back the `App`
|
||
is not enough.** `Host::command` projects per command and `apply_switch` /
|
||
`adopt_deleted_views` / `adopt_cursor_epoch` move mirrors inline, so by the time a
|
||
later command fails, every session's mirror has already moved. `Host::commands`
|
||
therefore snapshots the whole `Sessions` beside the document and restores both.
|
||
|
||
**The first version of this test was vacuous, and the injection is what showed it.**
|
||
It seated a bystander on a second view and asserted the bystander was told nothing
|
||
and its mirror was unmoved — both of which hold when `undo_frame` restores *nothing
|
||
at all*, because the frame's projections are discarded from `out` either way and
|
||
nothing ever moved that bystander. It answered *did the frame return early* where it
|
||
meant *was the mirror put back*.
|
||
|
||
The rewrite aims at the one mirror move a failed frame really makes and that
|
||
**nothing in the stream can ever correct**: the asker's own `SwitchView`. An
|
||
accepted switch is relayed to nobody (gotcha 36), so a mirror left on the switched-to
|
||
view diverges from a client that was told the frame was refused, permanently and
|
||
silently. The fixture asserts up front that the mirror starts on `Default` — the
|
||
direction the default cannot supply — so the closing assertion is not satisfied by a
|
||
mirror nothing ever wrote. Removing `self.sessions = mirrors` while keeping the
|
||
document restore fails this test and only this test.
|
||
|
||
### a_frame_that_creates_the_record_it_writes_is_not_a_broken_chain
|
||
|
||
The other direction: clause 4 must not fire on `[AddItem("West"), SetCell(West)]`,
|
||
which is the exemption ADR 001 states as a **property** — a record that neither
|
||
resolves nor is claimed is one the frame is creating.
|
||
|
||
**Be clear about what this does and does not catch.** It catches `broken_chain`
|
||
being asked against the *pre-frame* document instead of the running one, which is
|
||
the plausible implementation error (passing the snapshot where the live `App`
|
||
belongs) and would refuse every data-dependent chain. It does **not** catch the
|
||
exemption filter being dropped, for the reason recorded above: `resolve_record`
|
||
falls back to the layout, so `West` already resolves once `AddItem` has run. A
|
||
fixture that discriminated the filter would need a created record that never enters
|
||
the layout; none exists in this workspace and one was not invented for it.
|
||
|
||
#### Helper — `frame_basis`
|
||
|
||
Not a test — the frame-level claim that test and the 955n tests state.
|
||
|
||
The basis a caught-up client would state for a **frame** naming `regions`: the two
|
||
document tiers plus each named record's current counter.
|
||
|
||
`reduce::caught_up` cannot stand in for this, and the difference is exactly what a
|
||
frame is: it builds the claim for **one command**, so a frame's records beyond the
|
||
first are simply absent from it and come back `RecordUnstated`. A real client calls
|
||
`BasisCache::claim` once with every record its frame names, which is what this
|
||
reproduces.
|
||
|
||
**Name only records that exist.** `claim` *omits* a record the cache has never
|
||
heard of; this helper always includes one, so pointed at a record that is not there
|
||
it fabricates `(hash, 0)` — a claim no real client could compose, and precisely the
|
||
input that flips the create exemption in `CheckedBatch`. For a record the frame is
|
||
*creating*, leave it out, which is what a client would do.
|
||
|
||
### an_accepted_switch_is_relayed_to_nobody
|
||
|
||
The successor to `a_bystander_is_sent_the_broadcast_and_the_asker_is_not`, which
|
||
pinned the pre-improvise-jytm relay (Copilot, PR #10). An accepted switch is now
|
||
owed to nobody: the asker folded it, and no other client's view moves.
|
||
|
||
It asserts the bystander gets **no frame at all**, not merely no `SwitchView`. A
|
||
relay that vanished while the projection fan-out still repainted every client would
|
||
pass the narrower check and still be pushing one client's view change at the rest.
|
||
|
||
### a_geometry_command_relays_the_cursor_reset_to_the_asker_too
|
||
|
||
The other audience, and the reason `Broadcast` carries one rather than the relay
|
||
deciding. A geometry change invalidates every cursor into the document
|
||
(improvise-gvnb), and the asker's is not exempt: a `Command::Model` is the one kind
|
||
a client cannot fold optimistically, so nothing has reset it locally and skipping it
|
||
would leave the very client that asked positioned in a grid it no longer has.
|
||
|
||
### a_stale_batch_is_refused_to_the_asker_alone
|
||
|
||
A refused batch is answered to the **asker and nobody else** (ADR 001). Nothing
|
||
happened, so there is nothing to tell the others — and the asker is told only which
|
||
class of staleness it was, because it already holds what is current.
|
||
|
||
### folding_the_relay_puts_a_refused_asker_back
|
||
|
||
What `Broadcast::To` is *for*, asserted end to end through `Host` rather than
|
||
through `Sessions`: a client that folded a view name the document refused is put
|
||
back by the frame the host sends it, so the cursor its next grid is positioned by is
|
||
read under a key the document has (gotcha 24).
|
||
|
||
The client slice is folded **before** the command is handed to the host, which is
|
||
what makes the assertion mean anything: a test that never folded would see
|
||
"Default" whatever the relay did. The bystander is asserted silent in the same test,
|
||
because a `ToAll` correction would also put the asker back and would be wrong.
|
||
|
||
### a_document_swap_sends_every_client_a_fresh_snapshot
|
||
|
||
AC of gotcha 39: a command that replaced the document owes *every* client a fresh
|
||
`Snapshot`, not a diff — including the client that did not ask.
|
||
|
||
#### Helper — `host_over_a_loadable_document`
|
||
|
||
Not a test — the host that test and `a_load_that_fails_is_not_a_rebase` drive.
|
||
|
||
A host whose document can be replaced out from under its clients: a second document
|
||
sits in the storage backend under a name `LoadModel` can reach.
|
||
|
||
### a_client_can_ask_its_host_to_save
|
||
|
||
improvise-368h. **The bug**: `effect::Save` is `EffectKind::ViewOnly` — correctly,
|
||
since it writes *this host's* path — so the derivation that builds `ModelCommand`
|
||
out of the `EffectKind::Model` effects could not reach it (gotcha 38). There was no
|
||
save on the wire at all, so **no client on any transport could cause its host to
|
||
write**: a websocket session edited the document in memory and the file on disk
|
||
never moved unless a human was at a TUI that was not there.
|
||
|
||
The derivation was not wrong; it was complete over the mutations and saving is not
|
||
one. So the variant is *added* rather than derived, and it carries no path: a client
|
||
cannot see the host's storage, and which document a host is hosting is the host's
|
||
own fact.
|
||
|
||
### a_save_with_nowhere_to_write_is_refused_and_told_only_to_the_asker
|
||
|
||
improvise-368h: the "what if there is no path" half. Not a panic and not a silent
|
||
no-op — the two answers the ticket ruled out — but the vocabulary that already
|
||
exists for "nothing happened and here is why", addressed to the asker alone
|
||
(gotcha 39).
|
||
|
||
The second client is not decoration: a refusal that leaked to the bystanders would
|
||
be telling them about a failure of somebody else's host-local request, and this
|
||
asserts it does not.
|
||
|
||
### a_write_that_fails_is_refused_rather_than_reported_as_saved
|
||
|
||
improvise-368h: a write that was attempted and failed must not be reported as a
|
||
success. `Save` signals failure by aborting its batch (gotcha 21), and the batch
|
||
here is exactly `[Save]`, so an abort is that failure and nothing else.
|
||
|
||
The fixture fails for a reason that is real rather than mocked: a `PhysicalFs`
|
||
pointed at a directory that does not exist. It creates nothing, so the test leaves
|
||
no trace on disk.
|
||
|
||
### every_frame_survives_a_json_round_trip
|
||
|
||
The wire is JSON to start with (the ticket's choice, kept), so both directions must
|
||
survive a round trip through it — including the one point at which `ViewState` is
|
||
serialized (gotcha 25).
|
||
|
||
The inbound half carries a payload-bearing variant on each side of the envelope
|
||
(`doctrine(populate_before_roundtrip)`): a `Viewport` and a `Command` carrying a
|
||
`CellValue::Text`, which is the arm the outbound half cannot reach — a text cell
|
||
aggregates to nothing under a None-axis category, so it never becomes a
|
||
`CellDisplay`.
|
||
|
||
*Note:* `host.rs` also carries a second `a_load_that_fails_is_not_a_rebase`
|
||
(`doctrine(check_the_checker)`: inject the fault the test targets — a load that does
|
||
not happen — and the snapshot must not appear; a test that saw `Snapshot` on every
|
||
command would pass `a_document_swap_sends_every_client_a_fresh_snapshot`). It shares
|
||
its name with the `ui/project.rs` test of the same name, which is why there is one
|
||
heading for both above.
|
||
|
||
---
|
||
|
||
## `ui/cat_tree.rs`
|
||
|
||
### the_tree_carries_the_virtual_categories_and_measure_includes_formula_targets
|
||
|
||
improvise-vjms: the panel lists the virtual categories alongside the regular ones,
|
||
and `_Measure`'s items are *derived* — explicit items ∪ formula targets (gotcha 7).
|
||
An implementation reading `category("_Measure").items` passes every other assertion
|
||
in the module and silently drops `Profit`, which is the failure improvise-ete was
|
||
closed for proposing.
|
||
|
||
---
|
||
|
||
## `command/keymap.rs`
|
||
|
||
### f1_and_question_mark_both_open_help
|
||
|
||
improvise-th7m: `F1` was bound twice on consecutive lines — first to a bare
|
||
`enter-mode` with no args, then to `enter-mode ["help"]`. The first was dead,
|
||
because `Keymap::bind` *inserts* and the later binding wins; had it ever been the
|
||
surviving one it would have failed `require_args` at dispatch and made `F1` do
|
||
nothing.
|
||
|
||
The risk in deleting a line of a duplicated pair is deleting the wrong one, so this
|
||
pins which binding `F1` is meant to carry: the same one `?` carries, which is what
|
||
identified it as the intended survivor.
|
||
|
||
### every_bound_command_name_is_registered
|
||
|
||
improvise-9cn: `Keymap::dispatch` resolves a binding through
|
||
`registry.interactive(name, args, ctx).ok()?`, so a binding is a silent no-op key in
|
||
two ways, not one: the name may not be registered, *or* the args may not parse. Both
|
||
walks live in this test.
|
||
|
||
The args half matters because improvise-s13 moved bindings out of the nullary class
|
||
the name check covers: `enter-mode ["export-prompt"]` and
|
||
`enter-mode ["formula-edit"]` carry a mode name that `parse_mode_name` must accept,
|
||
and a typo like `"export_prompt"` compiles, registers, and produces a dead
|
||
`Ctrl-e`. (Gotcha 15.)
|
||
|
||
It builds the context in the mode whose keymap it is walking, because a few parsers
|
||
read `ctx.mode` — `commit-buffer item` resolves its target category off
|
||
`AppMode::ItemAdd`.
|
||
|
||
improvise-piyp. This entry is about the test's **reach**, not its logic — the logic was right
|
||
and demonstrably so, which is what made the gap hard to see.
|
||
|
||
It had two blind spots, both found by injection rather than by reading:
|
||
|
||
- the walk matched `Some(Binding::Prefix(_)) | None => {}`, so it **never
|
||
descended into a prefix sub-keymap**. Everything under `g`, `y` and `Z` was
|
||
unchecked, in every mode — and `RecordsNormal` reaches Normal's three
|
||
through `Keymaps::prefix_keymap`, so it was unchecked twice over.
|
||
- its key space was a **second copy** of `tests_support::probe_keys_pub`,
|
||
identical but for the two F-keys the public one carries. So a name bound on
|
||
any function key was invisible here while `dump_every_binding` probed it.
|
||
|
||
Injected at `523d571`: one unregistered name under the `g` prefix and one on
|
||
`F(5)` in Normal. `cargo test --workspace` came back **1595 passed, 0 failed**
|
||
— byte-identical to the clean baseline. The positive control that separates
|
||
"untested path" from "broken test" is the same bogus name on `Char('~')`, a
|
||
probed non-prefix key: that fails. The guard worked; it could not see there.
|
||
|
||
Both are closed here. The walk is `reachable_names`, which recurses into
|
||
`Binding::Prefix` — terminating because `prefix_keymaps_are_flat` says the
|
||
nesting is one deep — and `probe_keys` now **delegates** to `probe_keys_pub`
|
||
rather than restating it, which is the part that matters: two lists of "the
|
||
keys a test probes" is the same disease as two keymap spines, and the F-key
|
||
gap was exactly the drift between them. `probe_keys_pub` covers `F(1)..=F(12)`
|
||
rather than the two that happen to be bound, since the point is to see a
|
||
binding nobody expected.
|
||
|
||
Re-running the two injections against the fixed test: the prefix one fails
|
||
with `Normal binds "no-such-command-under-a-prefix", which no registry entry
|
||
provides`, and the F-key one with the matching message for `F(5)`.
|
||
|
||
Worth noting for contrast, because reading either test alone misleads:
|
||
`nothing_declined_is_also_composed` in `resolve.rs` already recursed into
|
||
prefixes and into parents. The **resolution** side was guarded where the
|
||
**registration** side was not, and only comparing the two shows which half is
|
||
real.
|
||
|
||
### prefix_keymaps_are_flat
|
||
|
||
improvise-jc8: an armed prefix is stored as the key that opened it and resolved
|
||
against the CURRENT MODE's keymap. That is only correct while prefixes are flat — a
|
||
`Prefix` binding nested inside a sub-keymap would be resolved against the mode map,
|
||
where it does not exist, and the second half of the sequence would silently do
|
||
nothing. Adding a nested prefix must fail here rather than in the terminal.
|
||
|
||
### every_panel_keymap_shares_the_same_spine
|
||
|
||
improvise-4pg: the three panel keymaps were written out separately, so a spine
|
||
binding could be changed in one and forgotten in the others. They share
|
||
`PanelKeymap` now; this pins what "the spine" is, including that each panel's cursor
|
||
keys move its *own* cursor. (Gotcha 12.)
|
||
|
||
### every_mode_with_panel_toggles_binds_all_of_them
|
||
|
||
Every panel's toggle key is bound in Normal and in all three panel modes, so focus
|
||
can move between any two and the focused one closes. A fourth panel gets all four
|
||
keymaps from one `Panel::ALL` entry.
|
||
|
||
### panel_toggle_keys_dispatch_to_real_effects
|
||
|
||
`Panel::name()` has to spell what the registry's panel parser accepts. A lookup-only
|
||
assertion would not catch a mismatch — `dispatch` swallows an unresolvable command
|
||
with `.ok()?` — so the toggle key must come back with real effects.
|
||
|
||
### every_text_entry_mode_types_into_the_buffer_its_prompt_names
|
||
|
||
improvise-k8h: the buffer name used to be written on both sides of the crate —
|
||
`AppMode`'s `MinibufferConfig` names the buffer the prompt describes, and the keymap
|
||
names the buffer `append-char` edits. If those disagree the mode prompts over one
|
||
buffer while every keystroke edits another, with nothing to notice. `TextEntry` now
|
||
reads the name off the mode, and this pins the result for every text-entry mode.
|
||
(Gotcha 11.)
|
||
|
||
#### Helper — `text_entry_modes_with_exits`
|
||
|
||
Not a test — the list that test and the two below it walk.
|
||
|
||
The seven modes that carry a `MinibufferConfig`, each with the keys that commit or
|
||
leave it. `SearchMode` is not among them: it is `Normal` plus a flag, so it has no
|
||
`AppMode` variant and no minibuffer config to agree with.
|
||
|
||
### every_typing_keymap_belongs_to_a_text_entry_mode
|
||
|
||
The list in `text_entry_modes_with_exits` must stay complete: every keymap that
|
||
types into a buffer belongs to a mode whose minibuffer names that same buffer. Only
|
||
`SearchMode`, which has no `AppMode` variant, is exempt.
|
||
|
||
### text_entry_modes_clear_the_buffer_they_type_into
|
||
|
||
Every minibuffer mode types into exactly one buffer, and every key that commits or
|
||
leaves must clear that same buffer on the way — a buffer left behind pre-fills the
|
||
next entry with the last one. `SearchMode` is excluded on purpose: it has no commit
|
||
and its query is owned by `exit-search-mode`, not by `clear-buffer`.
|
||
|
||
The buffer is derived from each mode rather than restated in the test —
|
||
`every_text_entry_mode_types_into_the_buffer_its_prompt_names` owns pinning what it
|
||
should be (improvise-k8h).
|
||
|
||
### minibuffer_enter_dispatches_to_a_registered_commit
|
||
|
||
improvise-6t3: the three commit commands collapsed into a single
|
||
`commit-buffer <kind>`. `dispatch` swallows an unresolvable command name with
|
||
`.ok()?`, so a lookup-only assertion would still pass had the rename missed the
|
||
registry — Enter has to come back with real effects.
|
||
|
||
### dump_every_binding
|
||
|
||
The instrument `doctrine(binding_dump_diff)` asks for: every `(mode, key, modifier)`
|
||
lookup across all the modes, written to the file named by `IMPROVISE_BINDING_DUMP`.
|
||
|
||
Most bindings have no direct test, so a green suite is not evidence that a keymap
|
||
refactor preserved them. Dump before, dump after, diff. It is inert without the
|
||
variable: a test that writes a file nobody asked for is a surprise, not a check.
|
||
|
||
This is what proved the keymap move to `improvise-protocol` changed nothing — 4021
|
||
resolved lookups, byte-identical either side — and the diff was shown to be
|
||
discriminating by renaming one binding and watching eight lines move.
|
||
|
||
---
|
||
|
||
## `command/cmd/commit.rs`
|
||
|
||
|
||
Not an assertion — `doctrine(binding_dump_diff)`'s instrument, and the
|
||
evidence improvise-636d actually rests on, because a green suite is not
|
||
evidence for a keymap refactor.
|
||
|
||
Diffed across the global change: **84 added lookups, 0 removed, 0 changed**.
|
||
No binding anywhere answers differently than it did, which is the property
|
||
that matters for a change that moves bindings between tables. The additions
|
||
are 16 rows of `?` reaching the three panels and TileSelect, 1 of Help's new
|
||
`?`, 24 of the panel toggles reaching Help and TileSelect, and 40 that are not
|
||
a binding change at all — the widened `probe_keys_pub` hitting ImportWizard's
|
||
`KeyPattern::Any` on `F(3)..=F(12)`.
|
||
|
||
**`:` produced zero rows**, which is the result worth keeping: every mode that
|
||
now inherits it already bound it locally, so moving it into the global changed
|
||
no answer anywhere. That is what "the duplication was real and consistent"
|
||
looks like from the outside — and it is also why nothing in the suite could
|
||
have caught the two spines drifting apart, had they ever done so.
|
||
|
||
### invalid_formula_reports_the_parse_error_not_success
|
||
|
||
improvise-6t3: the commit pushed `set_status("Formula added")` *after* `AddFormula`,
|
||
so the parse error that `AddFormula::apply` writes to the status bar was immediately
|
||
overwritten. An unparseable formula was dropped while the UI reported success.
|
||
|
||
The add effect is the only party that knows whether it worked, so it has to get the
|
||
last word on the status line.
|
||
|
||
### commit_and_advance_threads_edit_mode_to_enter_edit_at_cursor
|
||
|
||
`CommitAndAdvance` must thread its `edit_mode` through to the trailing
|
||
`EnterEditAtCursor` effect so the post-commit re-edit lands in the mode the keymap
|
||
requested. The command never reads `ctx.mode`.
|
||
|
||
### commit_save_writes_the_model_to_the_typed_path
|
||
|
||
improvise-lae: the save prompt has to commit to `SaveAs` and not to `ExportCsv`,
|
||
which is what makes it a separate mode from `ExportPrompt` rather than a second
|
||
binding onto it — `ExportCsv` would write CSV under whatever `.improv` name the user
|
||
typed.
|
||
|
||
`SaveAs` and not `Save`, because `Save` writes the path the document already has,
|
||
and the whole reason this prompt exists is that there isn't one.
|
||
|
||
### commit_save_on_an_empty_path_writes_nothing
|
||
|
||
An empty prompt must not reach `SaveAs`: `SaveAs(PathBuf::from(""))` asks the
|
||
backend to write to `""`, and the user then reads whatever I/O error that produces
|
||
instead of "you typed nothing".
|
||
|
||
### commit_export_bails_on_an_empty_path
|
||
|
||
improvise-z05r's first arm. `CommitExport` did not bail on an empty path where
|
||
`CommitBuffer`'s three kinds all do (gotcha 20: buffer to trim to bail-on-empty to
|
||
dirty to status is the shared spine), so Enter on an empty export prompt asked the
|
||
backend to write to `""` and the user read whatever I/O error that produced.
|
||
|
||
The same change reorders the batch: the mode change now comes **first**, because
|
||
`ExportCsv` aborts on a failed write as of z05r's other arm, and an abort at the tail
|
||
would strand the user in `ExportPrompt` — where `draw_bottom_bar` renders the
|
||
minibuffer *instead of* the status line, so the export error is never drawn at all
|
||
and the operation appears to do nothing (gotcha 21). That ordering is asserted in
|
||
`commit_export_produces_export_and_normal_mode`, by index and not by presence.
|
||
|
||
|
||
---
|
||
|
||
## `command/cmd/fit.rs`
|
||
|
||
### the_command_installs_a_formula_the_document_evaluates
|
||
|
||
The end-to-end claim of improvise-vx3, asserted against a real document: a command
|
||
fits, the formula lands through the ordinary `add_formula` path, and the new measure
|
||
evaluates to numbers.
|
||
|
||
#### Helper — `workbook`
|
||
|
||
Not a test — the document every `fit.rs` test fits against.
|
||
|
||
A document a tree can learn exactly — `Score` steps at `Cost` 50 — in the units a
|
||
real document uses.
|
||
|
||
**Every category is a `Label`**, so the only feature is the numeric measure `Cost`
|
||
and the champion cannot reach an unresolvable one. An earlier version left `Region`
|
||
regular with one item and claimed `simplify` would prune the constant one-hot; it
|
||
does not, because `build_categorical` records `(0.0, 1.0)` for every one-hot
|
||
whatever the column holds — so these tests were passing on seed luck (Copilot on
|
||
PR #25). See `improvise_gp::fit`'s own fixture for the same correction.
|
||
|
||
### the_status_line_reports_quality_against_the_baseline
|
||
|
||
improvise-vx3 is emphatic that the rule alone is not the deliverable: a tidy tree
|
||
that generalizes no better than the mean is worse than useless. So the status line
|
||
carries the held-out comparison, what the rule leans on, and the seed that
|
||
reproduces it.
|
||
|
||
### a_refusal_names_what_went_wrong_and_stores_nothing
|
||
|
||
A refusal reaches the user *whole*. "fit failed" is not actionable; naming the
|
||
measure that does not exist is (improvise-8xhy's argument, arriving at the place a
|
||
user meets it).
|
||
|
||
### the_seed_argument_changes_the_search
|
||
|
||
Gotcha 37b: the seed is half of what reproduces a fit, so it is an argument — and a
|
||
different one really does search differently.
|
||
|
||
It asserts on the **rule**, never on the batch's debug string. The first version
|
||
compared `effects_debug` over the whole vec, which was vacuous: `status_line` prints
|
||
`seed {:#x}`, so the two strings differ because the seed was *printed*, and
|
||
`fit_measure` could have ignored `request.seed` entirely with the test still green
|
||
(`doctrine(check_the_checker)`).
|
||
|
||
`AddFormula` has exactly two fields — the formula text and the target category — and
|
||
the seed reaches neither, so its `Debug` cannot differ for the reason the batch's
|
||
did. (A substring guard was tried and is wrong: the seeds `1` and `99` occur inside
|
||
perfectly ordinary thresholds.)
|
||
|
||
### the_seed_the_status_line_prints_is_a_seed_this_command_accepts
|
||
|
||
Copilot on PR #25. The status line advertises the seed as `{:#x}` and tells the user
|
||
that is what reproduces the fit, so the one instruction it gives has to work on the
|
||
value it just showed them — a decimal-only parser made copying it back an error.
|
||
|
||
Asserted as a *round trip through the status line*, not against a hand-written
|
||
`"0x…"`: a literal would still pass if `status_line` changed how it spells the
|
||
number, which is the half that breaks.
|
||
|
||
---
|
||
|
||
## `command/cmd/grid.rs`
|
||
|
||
### drill_into_formula_cell_returns_data_records
|
||
|
||
Drilling into a formula cell (`Profit = Revenue - Cost`) must return the underlying
|
||
data records, not an empty result set. The formula target coordinate is stripped
|
||
from the drill key so that `matching_cells` finds the raw data backing the formula
|
||
(gotcha 10).
|
||
|
||
### drilling_fixes_its_coordinates_on_filter_not_page
|
||
|
||
improvise-dz8: a drilled coordinate is a constraint of the drill, not a dimension
|
||
the user may page through, so it lands on `Filter`. It used to land on `Page`, which
|
||
put it in the tile bar and under `[`/`]` as if the user could move it (gotcha 27).
|
||
|
||
---
|
||
|
||
## `command/cmd/mode.rs`
|
||
|
||
### enter_mode_export_prompt_sets_mode_and_clears_its_buffer
|
||
|
||
The `enter-export-prompt` shim is gone (improvise-s13); the generic `enter-mode`
|
||
carries the mode name instead, and clears the prompt's buffer on the way in.
|
||
|
||
### enter_mode_clears_a_search_keyed_minibuffer_through_write_buffer
|
||
|
||
improvise-gdl made `write_buffer` the single accessor for a runtime-named buffer,
|
||
because "search" is not an entry in the buffers map but its own `ViewState` field.
|
||
`EnterMode` built a `SetBuffer` directly, so a mode whose `buffer_key` is "search"
|
||
would clear a map entry nobody reads and leave the previous query on screen — the
|
||
exact desync gdl closed. No shipped `AppMode` names that buffer yet, so the mode here
|
||
is constructed by hand; that is what makes the test a demonstration rather than a
|
||
restatement.
|
||
|
||
### edit_or_drill_passes_records_editing_mode_through
|
||
|
||
`EditOrDrill` must trust its `edit_mode` parameter rather than checking `ctx.mode` —
|
||
the records-normal keymap supplies `records-editing`, but the command itself never
|
||
inspects the runtime mode. This is the parallel of the deleted
|
||
`enter_edit_mode_produces_editing_mode` test for the records branch.
|
||
|
||
### enter_edit_at_cursor_cmd_passes_target_mode_to_effect
|
||
|
||
`EnterEditAtCursorCmd` must hand its `target_mode` straight through to the
|
||
`EnterEditAtCursor` effect — the keymap (records `o` sequence or
|
||
commit-and-advance) decides; the command never inspects `ctx`. It fronts the pair
|
||
with `RebuildLayout` so the cursor is read against a layout that includes whatever
|
||
ran earlier in the sequence (improvise-ehz, gotcha 23).
|
||
|
||
---
|
||
|
||
## `command/cmd/navigation.rs`
|
||
|
||
### page_cycling_skips_filter_coordinates
|
||
|
||
improvise-dz8: `[` and `]` page through Page-axis categories. A coordinate the drill
|
||
fixed sits on `Filter` and must be invisible to them, or the user could page out of
|
||
the drill they are inside.
|
||
|
||
### enter_advance_at_bottom_right_emits_abort_chain
|
||
|
||
At bottom-right `EnterAdvance` has no place to go, so it emits a single `AbortChain`
|
||
effect. Trailing effects in a `CommitAndAdvance` batch (`EnterEditAtCursor`) are
|
||
then skipped, which is how "Enter at bottom-right commits and exits editing" is
|
||
realised.
|
||
|
||
---
|
||
|
||
## `command/cmd/panel.rs`
|
||
|
||
### cycle_panel_focus_advances_from_formula_to_category
|
||
|
||
Bug improvise-dqn: `CyclePanelFocus` always picked the first open panel, ignoring
|
||
the currently focused panel. With formula + category open and focus on the formula
|
||
panel, cycling must move to the category panel.
|
||
|
||
### create_and_switch_view_names_incrementally
|
||
|
||
improvise-aaw asked for this command to be replaced by a keymap
|
||
`Binding::Sequence [create-view X, switch-view X, enter-mode normal]`. It cannot be:
|
||
a `Sequence` carries **literal** args, and the name here is computed from `ctx` — so
|
||
a Sequence would hard-code one name and the second `n` in the view panel would
|
||
collide with the first.
|
||
|
||
The test was called `..._names_incrementally` and asserted only that
|
||
`CreateView`/`SwitchView`/`Normal` appeared — it never looked at the name, which is
|
||
the one thing that makes this command a command and not a composition. Now it does,
|
||
at two different view counts, because a single count cannot tell "derived from the
|
||
workbook" from "constant".
|
||
|
||
---
|
||
|
||
## `command/cmd/registry.rs`
|
||
|
||
### every_entry_constructs_a_command_with_its_registered_name
|
||
|
||
improvise-9cn / improvise-61f: every registration derives its name from a prototype
|
||
instance, so a typo in the name is impossible. What the types do *not* catch is the
|
||
commands whose `name()` branches on a field — `ViewNavigate`, `TileAxisOp`,
|
||
`ToggleGroupAtCursor`, `CommitAndAdvance`. Their prototype and the value their
|
||
constructor builds are the same type, so registering `ViewNavigate { forward: true }`
|
||
while building `forward: false` compiles cleanly and files the command under the
|
||
wrong name, where dispatch silently misses. Constructing every entry both ways and
|
||
comparing names is the only check that reaches that class. (Gotcha 15.)
|
||
|
||
Some entries are deliberately one-path — `commit-and-advance-right` reads the edit
|
||
buffer and cursor, so its parse fn always errors — but every entry must be reachable
|
||
by at least one path, or its name goes unchecked.
|
||
|
||
### registered_names_are_unique
|
||
|
||
A variant mix-up does not show up as a name mismatch — it shows up as a **collision**.
|
||
Registering `ViewNavigate { forward: false }` where `forward: true` belongs files a
|
||
second entry under "view-back" and removes "view-forward" from the registry
|
||
entirely; `parse` takes the first match, so every surviving entry still agrees with
|
||
its own name. Uniqueness is what catches it.
|
||
|
||
### the_context_reports_each_panels_own_open_flag
|
||
|
||
improvise-y4xg and improvise-qsmp, two shapes of one defect. `registry.rs` mapped
|
||
`Panel` to a `CmdContext` field in two places, and `App::cmd_context` copies the
|
||
three flags out of `ViewState` in a third. **Both were demonstrated unguarded**:
|
||
swapping Formula and Category at the two registry sites left the workspace at 1400
|
||
passing, and hardcoding the `category_panel_open` propagator to `true` left it at
|
||
1394.
|
||
|
||
Nothing exercised either, because `cmd/mod.rs::test_helpers` sets the panel flags
|
||
explicitly on the context it builds — the command tests construct the value they then
|
||
assert on, so no test ever drove the real propagator.
|
||
|
||
The registry mapping is gone: both sites call `CmdContext::panel_open(panel)`, the
|
||
one mapping on this side of the wall. That leaves two permutable sites, and the test
|
||
is aimed at both from a path neither controls — it writes `view_state`'s three fields
|
||
**by name** and reads back through the enum accessor on a context built by the real
|
||
`App::cmd_context`. PR #70's review caught the weaker shape of this: a panel test that
|
||
zeroed and read through the same four accessors passed a consistent permutation.
|
||
|
||
Injected fault: swap `Panel::Formula` and `Panel::Category` in `panel_open`. Fails.
|
||
Injected fault: hardcode the `category_panel_open` propagator to `true`. Fails.
|
||
|
||
### toggle_panel_visibility_reads_the_panel_it_names
|
||
|
||
The behavioural half, and the one that reaches the registry. Its interactive arm
|
||
passes `currently_open` straight through, so with exactly one panel open the command
|
||
built for each of the three names carries its own panel's flag. Both injections above
|
||
fail here too.
|
||
|
||
### a_focused_open_panel_closes_and_a_closed_one_opens
|
||
|
||
improvise-qsmp's own scenario: `toggle-panel-and-focus` decides close only when the
|
||
panel is *both* open and focused, so the case that separates a right propagator from
|
||
a stuck-true one needs the mode set to the panel's own. With the flag stuck true,
|
||
pressing a panel's key on a closed panel decides to *close* it and the panel becomes
|
||
unopenable by its own key — which is what this test's second arm names.
|
||
|
||
### Helper — `open_only`
|
||
|
||
Not a test. It spells `formula_panel_open`, `category_panel_open` and
|
||
`view_panel_open` as fields rather than looping over `Panel::ALL` through an
|
||
accessor, and that is the whole evidentiary value: a permutation in any mapping
|
||
between the `Panel` enum and those three booleans has to disagree with this helper.
|
||
|
||
|
||
---
|
||
|
||
## `command/cmd/text_buffer.rs`
|
||
|
||
### writing_the_search_buffer_targets_the_search_query
|
||
|
||
Writing the "search" buffer means writing the search query, and `write_buffer` is
|
||
the one place that knows it (improvise-gdl). Every buffer-writing command routes
|
||
through it, so `clear-buffer search` clears the query rather than parking an unread
|
||
entry in the map.
|
||
|
||
### colon_wq_saves_then_quits
|
||
|
||
improvise-git proposed deleting the `SaveAndQuit` command and binding `ZZ` to a
|
||
`Binding::Sequence [w, force-quit]` instead — the repo's "do not name a command
|
||
AAndB" principle.
|
||
|
||
That premise is incomplete, and this test is why. `wq` is not only a keybinding: it
|
||
is a **registered command name**, so `:wq` reaches it through the registry, and
|
||
`Quit`'s own unsaved-changes message advertises it ("Use :q! to force quit or :wq to
|
||
save+quit"). A keymap `Sequence` is not reachable from the `:` line, so deleting the
|
||
command deletes the ex-command with it.
|
||
|
||
Keeping the command *and* adding the Sequence would leave two independently-written
|
||
spellings of "save then quit", which is the duplication this codebase keeps curing
|
||
rather than creating. So this pins the surface, and the conversion is declined on it.
|
||
|
||
### colon_enter_edit_at_cursor_is_not_snapped_back_to_normal
|
||
|
||
improvise-6sq: `ExecuteCommand` appends `change_mode(Normal)` unless some effect
|
||
reports `changes_mode()`. `EnterEditAtCursor` sets `view_state.mode` but did not
|
||
override it, so `:enter-edit-at-cursor editing` produced
|
||
`[RebuildLayout, EnterEditAtCursor(Editing), ChangeMode(Normal)]` — the trailing
|
||
snap-back undid the edit mode and the user never got a cursor in the minibuffer.
|
||
|
||
Asserting on `changes_mode()` alone is not enough here: the appended
|
||
`ChangeMode(Normal)` reports `true` itself, so the assertion has to name the
|
||
snap-back it is ruling out.
|
||
|
||
### colon_import_stays_in_the_wizard_on_success
|
||
|
||
improvise-toc: `StartImportWizard` swaps the mode only when the file parses, so a
|
||
per-type `changes_mode()` cannot describe it. Reporting `false` made
|
||
`ExecuteCommand` append a return-to-Normal on top of the success path:
|
||
`:import d.json` opened the wizard and was immediately snapped back, leaving the
|
||
wizard drawn under the Normal keymap. (Gotcha 31.)
|
||
|
||
### colon_import_falls_back_to_normal_on_failure
|
||
|
||
The other half of the same bug: reporting `true` unconditionally would strand the
|
||
user in `CommandMode` when the import fails. A failed `:import` must still fall back
|
||
to Normal.
|
||
|
||
### colon_view_back_respects_whether_history_exists
|
||
|
||
The `ViewBack` half of improvise-toc: it restores the stored frame's mode only when
|
||
the back-stack is non-empty. With history present, `:view-back` must keep the mode it
|
||
restored; with an empty stack it falls through to Normal like any other no-op
|
||
command.
|
||
|
||
---
|
||
|
||
## `command/cmd/wizard.rs`
|
||
|
||
### name_model_enter_builds_the_model
|
||
|
||
Enter on the name step builds the workbook into the wizard rather than advancing the
|
||
step machine; every other printable key edits the name (gotcha 22).
|
||
|
||
### done_confirms_on_enter_and_steps_back_on_esc
|
||
|
||
On the confirmation step Enter installs the built workbook and Esc steps back — it
|
||
does not throw the import away, and no key closes the wizard outright
|
||
(improvise-907, from improvise-c6j).
|
||
|
||
### a_formula_owned_cell_is_projected_as_derived_and_its_siblings_are_not
|
||
|
||
improvise-ir7l. `project::cell_id` decided `Record` vs `Aggregate` purely on
|
||
whether a regular category sat on `Axis::None`; it never asked whether the
|
||
`_Measure` coordinate named a formula target. So a formula cell was a `Record`,
|
||
`render.rs::editable` said true, and a write to it was accepted, stored and
|
||
then shadowed forever by `formula_cache`.
|
||
|
||
The fault injected: replacing the `derived` lookup with `None`, which is the
|
||
pre-fix code exactly. The test then reports `Record(RecordCell { measure:
|
||
Some("Profit") })` where it wants `Derived { target: "Profit" }`. The siblings
|
||
are asserted in the same test so a producer that answered `Derived` for *every*
|
||
cell could not pass.
|
||
|
||
### an_aggregating_formula_cell_is_still_projected_as_an_aggregate
|
||
|
||
The negative arm of the same change, and the one a careless fix breaks: gotcha
|
||
10 says drilling into a formula cell is supported, so `Aggregate` must still
|
||
win where the cell aggregates. Proved discriminating by returning `derived`
|
||
before the aggregate branch — the cell then reads `Derived` and becomes
|
||
undrillable, with nothing else in the suite noticing.
|
||
|
||
### edit_or_drill_refuses_a_formula_owned_cell
|
||
|
||
The TUI half. `EditOrDrill` never asked the question either, so the terminal
|
||
offered the same editor over the same shadowed cell. Landing only the
|
||
projection half would have made the two front ends disagree, which is the
|
||
defect design-principles §5 is about. Fault injected by deleting the guard: the
|
||
command returns `[SetBuffer{edit}, ChangeMode(Editing)]` again.
|
||
|
||
### a_save_clears_the_dirty_marker_on_a_client_although_nothing_is_dirty
|
||
|
||
improvise-z754, and the arm the whole design turns on. `effect::Save` is
|
||
`EffectKind::ViewOnly`, so `Dirty::of` reports `NOTHING` — the test asserts
|
||
that first, so it cannot quietly stop being the interesting case if `Save` is
|
||
ever reclassified. A title bar gated on `Dirty` would therefore leave every
|
||
client showing `[+]` for a document that has just been saved, forever, with
|
||
nothing in the stream to correct it.
|
||
|
||
Fault injected: wrapping `document_update` in `if !dirty.is_empty()`, which is
|
||
the obvious implementation and the wrong one. The test fails on the cleared
|
||
arm. `a_title_bar_that_did_not_move_is_not_projected` is its counterweight —
|
||
without it, "project the title bar every batch" would pass this one.
|
||
|
||
### the_title_bar_names_the_document_without_naming_the_path
|
||
|
||
The fixture's path is `/home/someone/private/books.improv` on purpose: a test
|
||
whose fixture is a bare file name cannot tell `file_name()` from the whole
|
||
path, and the failure mode is a socket server handing a remote client the
|
||
host's directory layout.
|
||
|
||
### a_creating_column_is_named_rather_than_left_as_no_cell
|
||
|
||
improvise-poia. Both `+` and `+cat` projected `None`, which the protocol
|
||
defines as "no projected cell" — the same conflation gotcha 42 names for
|
||
aggregates, arriving a third time.
|
||
|
||
The test walks **every** column of a records grid and counts, rather than
|
||
asserting the two creating columns alone. That matters in the direction a
|
||
weaker test fails: a producer that answered `Create` for everything, or that
|
||
made the `Region` coordinate column nameable, would pass a two-position check
|
||
and fails this one. The fault injected — returning `None` for both creating
|
||
columns, which is the pre-fix code — reports three nameless columns where one
|
||
is allowed.
|
||
|
||
### a_records_coordinate_is_text_however_it_is_spelled
|
||
|
||
improvise-tom1. `project::cell_display`'s records arm called `records_display`
|
||
— which returns a `String` — and then inferred numeric-ness with
|
||
`text.parse::<f64>()`. A records coordinate column holds an item **name**, so a
|
||
`Year` category with items `2024`/`2025` projected a coordinate the browser
|
||
right-aligns as a number.
|
||
|
||
Nothing in the suite could see it: every records fixture in the tree names its
|
||
categories `Region`/`East`/`North`, so the guess happened to be right. The
|
||
fixture here is the whole test — a category whose items are spelled like
|
||
numbers — and the measure column beside it is asserted numeric in the same
|
||
test, so a producer that answered `false` for everything would not pass.
|
||
|
||
Fault injected: the `parse::<f64>()` guess restored verbatim. It reports the
|
||
coordinate as numeric.
|
||
|
||
### a_staged_drill_edit_reaches_its_own_session_and_no_projection
|
||
|
||
The user's ruling, pinned in both directions (improvise-tom1). A remote client
|
||
must NOT see another client's staged drill edits, so `grid_of` must project the
|
||
committed value; the session that staged one must still see its own typing,
|
||
through `App::display_value_of`.
|
||
|
||
Asserting only the second half would pass against a producer that leaked, and
|
||
asserting only the first would pass against one where staging did nothing at
|
||
all — so both are here. The fault injected is the leak: teaching
|
||
`cell_display`'s records arm to prefer `drill_state.pending_edits`, which is
|
||
the plausible "fix" this ticket exists to forbid. The projected cell then reads
|
||
`999` instead of `1`.
|
||
|
||
The fixture takes its `DrillState::records` from the layout the app really
|
||
built, because an empty frozen snapshot silently empties the drill grid and the
|
||
test then asserts nothing about a cell that does not exist — the first version
|
||
did exactly that and panicked on `expect`.
|
||
|
||
### a_page_axis_carries_its_items_and_a_filter_axis_does_not
|
||
|
||
The producer half of improvise-p1ym, added during self-review because
|
||
`a_filter_axis_is_projected_but_not_cycled` builds its own `PageAxis` by hand —
|
||
a test that fabricates the very data the code under test is supposed to
|
||
produce, which is this repo's characteristic defect. Only this one can say what
|
||
`grid_of` actually emits.
|
||
|
||
The fixture puts `Quarter` on `Axis::Page` and `Year` on `Axis::Filter`, which
|
||
is the shape a drill makes (gotcha 27), and asserts the whole projected list in
|
||
one comparison so a producer that emitted items for **everything** or for
|
||
**nothing** both fail. Fault injected: `let navigable = true`, which is
|
||
`filtering_cats()` leaking into the protocol — the exact trap the ticket names.
|
||
`Year` comes back carrying `["2024"]`.
|
||
|
||
### a_records_grid_has_no_column_for_a_formula_target
|
||
|
||
A records grid has no column for a formula target that is **only** a formula
|
||
target, because `GridLayout::records_columns` builds its measure columns from
|
||
`Category::ordered_item_names` — the explicit `_Measure` items — while
|
||
`add_formula` adds none (gotcha 7). Fault injected: swapping
|
||
`ordered_item_names` for `measure_item_names` in `records_columns`, which is a
|
||
plausible tidy-up — a `Profit` column appears.
|
||
|
||
**This test's original premise was FALSE, and the review caught it.** It was
|
||
written claiming the records branch of `cell_id` was *safe* because "a formula
|
||
target lives only in `measure_item_names`". An item can be **both** an explicit
|
||
`_Measure` item and a formula target, and improvise-poia — in the same PR —
|
||
made the `+` column that builds exactly that reachable. So this test asserted
|
||
the accident in the one configuration where the accident holds, and its own
|
||
failure message named a repair for a case it could not reach. The real guards
|
||
are `a_records_column_a_formula_owns_is_derived_not_a_record` and
|
||
`typing_into_a_records_formula_column_is_refused_on_the_key_path`; this one now
|
||
claims only what it can see.
|
||
|
||
### a_records_column_a_formula_owns_is_derived_not_a_record
|
||
|
||
The hole improvise-ir7l's first fix left, found by adversarial review. A measure
|
||
can be **both** an explicit `_Measure` item and a formula target — the records
|
||
`+` column creates precisely that — and `cell_id`'s records branch asked
|
||
`formula_for` about nothing, so it projected `CellId::Record{measure:"Profit"}`.
|
||
A write there is accepted, stored, and then shadowed forever by `formula_cache`,
|
||
which is the defect the ticket exists to close, surviving in records mode in
|
||
**both** front ends since `editable` is `Record`-only.
|
||
|
||
The repair asks the document about the record's **real** coordinate —
|
||
`GridLayout::record_cell_key`, the one mapping from a records position to a
|
||
model cell — never the synthetic `(_Index, _Dim)` key, which names no measure
|
||
and so can never match a formula. Fault injected: deleting the lookup, which is
|
||
the pre-review code. `Profit` reads back as a `Record`. The `Revenue` column is
|
||
asserted still writable in the same test, so a producer answering `Derived` for
|
||
everything could not pass.
|
||
|
||
### typing_into_a_records_formula_column_is_refused_on_the_key_path
|
||
|
||
The same defect through `App::handle_key` and no test API at all: `R`, walk to
|
||
`+`, type `Profit`, Enter, Esc, then `i` on the column just created. It exists
|
||
because the projection half and the `EditOrDrill` half are **separately**
|
||
breakable, and a unit test of either would have missed the other — proved by
|
||
injecting each alone. Restoring `cell_id` while leaving `CmdContext::
|
||
owning_formula` reading the raw key fails on the mode assertion; the reverse
|
||
fails on the identity assertion.
|
||
|
||
One fixture note that cost a debugging round: `CommitAndAdvance` **re-enters
|
||
editing** on the cell it advances to, so the mode after Enter is
|
||
`RecordsEditing` and an `i` there is a literal character. Without the Esc the
|
||
test passes its mode assertion for the wrong reason — nothing happened at all.
|
||
|
||
### a_save_tells_the_client_the_document_is_clean
|
||
|
||
improvise-z754's own stated failure mode, shipped: "every client showing a `[+]`
|
||
for a document that has been saved". `Host::save` applied `effect::Save` and
|
||
returned only a possible `Rejected`, never projecting, so the dirty marker
|
||
cleared **outside** any projection and no client was ever told.
|
||
|
||
The z754 test that was supposed to cover this called `sessions.project_all` by
|
||
hand — which is exactly the call the production path omits. It is not a test
|
||
that compares a thing with itself; it is a test that exercises a call the
|
||
deployment never makes, which is the subtler shape. This one goes through
|
||
`Host::handle(ClientFrame::Save)`.
|
||
|
||
Two assertions guard the fixture: the edit must first report `dirty: true`, and
|
||
`app.model_state.dirty` must really be `false` afterwards — otherwise a host
|
||
that saved nothing would pass by projecting nothing. Fault injected: returning
|
||
an empty vec from the success arm. No `Document` update is emitted.
|
||
|
||
### the_automatic_persist_tells_the_client_the_document_is_clean
|
||
|
||
The worker's half of the same defect, and a different cause: `on_frame` built
|
||
`out` via `frames_for` and *then* called `persist()`, so the projections were
|
||
built while `dirty` was still true. The browser is the deployment where this
|
||
bites hardest, because nothing there ever sends `ClientFrame::Save` — the worker
|
||
persists on its own behalf (gotcha 45), so this was the only path.
|
||
|
||
Fault injected: dropping the re-projection after `persist()`. The client's
|
||
`DocumentCache` stays dirty. `MainThread` grew a `document` mirror and a
|
||
projection dispatch for it, rather than a bespoke reach into the worker — the
|
||
point is what a client folding the real stream ends up holding.
|
||
|
||
**A first version asserted only `!document.dirty`, and `DocumentCache::default()`
|
||
is already clean** — so it could not tell "told it cleared" from "never told
|
||
anything", and passed under an injection that stopped projecting the widget
|
||
entirely. Found in re-review. It now records **every** marker the client is
|
||
told and asserts `[true, false]` for the edit, with the bootstrap's own clean
|
||
marker cleared first so the sequence is about the edit rather than the
|
||
handshake. The stronger form fails under both injections.
|
||
|
||
### a_refused_or_duplicate_add_category_moves_no_counter
|
||
|
||
improvise-75ld. `App::apply_effects` bumped `layout_version` and raised
|
||
`model_state.dirty` off the `EffectKind` **before** the `abort_effects` check, so
|
||
the refusing effect counted itself. Two arms, and the second is the reachable one:
|
||
a 13th category is refused (`MAX_CATEGORIES`) and aborts, while a *duplicate*
|
||
`add-cat Region` returns `Ok` and does not abort at all — a pure no-op that still
|
||
refused every other client's next `Command::Model` with `Rejection::LayoutMoved`
|
||
and made the browser worker rewrite the whole OPFS document (gotcha 41).
|
||
|
||
Injected fault: put the two bumps back on the declared kind, inside the loop
|
||
(`if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry))`). Both no-op
|
||
arms fail; so do `neither_shape_counter_moves_for_an_accepted_no_op`,
|
||
`both_shape_counters_move_for_every_reshaping_command` and
|
||
`a_configuration_change_that_draws_nothing_is_still_a_document_change`, with 450
|
||
other tests still passing — the build ran.
|
||
|
||
The positive arm is not decoration: without it, "never bump anything" passes the
|
||
two no-op arms. It also fails under a second injection — dropping the
|
||
`document_config` comparison — because `AddCategory` is `LayoutGeometry` and the
|
||
declared half of `dirty` no longer covers it.
|
||
|
||
### both_shape_counters_move_for_every_reshaping_command
|
||
|
||
improvise-pdvm. `AddItem`, `AddItemInGroup` and `RemoveItem` declare
|
||
`ModelChange::CellData` and reshape the grid. Since improvise-9mo9 they call
|
||
`App::reset_cursor`, so `cursor_epoch` moved while `layout_version` did not: a
|
||
client was told its cursor was invalid while its layout basis tier still read
|
||
current, and a batch composed against the old shape was still accepted.
|
||
|
||
It drives `reshaping_commands()` — the same table
|
||
`every_reshaping_model_command_invalidates_every_cursor` drives, extracted into a
|
||
shared function precisely so the two cannot answer for different cases — and
|
||
asserts the grid shape really moved before asserting either counter, so a case
|
||
that reshapes nothing fails loudly rather than proving nothing.
|
||
|
||
Injected fault: as above. Fails naming `SetAxis`, then `AddItem` once the earlier
|
||
cases are removed.
|
||
|
||
### a_configuration_change_that_draws_nothing_is_still_a_document_change
|
||
|
||
The third case the shape/config split creates, and the one that forced two
|
||
fingerprints rather than one. `hide_item` records a name whether or not the model
|
||
has one, and `SetNumberFormat` writes the file while moving no row or column: both
|
||
are `dirty` and neither is `layout_version`. A single fingerprint covering the
|
||
persisted configuration made a ghost hide bump the basis; one covering only the
|
||
drawn shape let a format change go unsaved.
|
||
|
||
Injected fault: remove the `document_config` comparison in `observe_reshape`.
|
||
Fails naming `HideItem { category: "Region", item: "NoSuchItem" }`.
|
||
|
||
### a_reshape_of_one_view_leaves_a_client_on_another_view_alone
|
||
|
||
improvise-nynk. improvise-jytm made "which view a client is on" per-client;
|
||
`Sessions::adopt_cursor_epoch` was not updated and still reset **every** mirror
|
||
and returned one `Broadcast::ToAll(Command::ResetCursor)`. A reshape of view
|
||
`Default` therefore threw away the cursor of a client sitting on `Second`, which
|
||
nothing reshaped.
|
||
|
||
The fixture is two views of one document with different axes, and the bystander
|
||
switches to `Second` and scrolls there before the asker reshapes `Default`. The
|
||
positive arm is kept in the same test — the asker is on the reshaped view, is
|
||
reset, and is told — because without it "never invalidate anyone" passes.
|
||
|
||
Injected fault: drop the `cursor_invalidated` filter (`.filter(|_| true)`). Fails
|
||
naming the bystander's thrown-away cursor, with 453 other tests passing.
|
||
|
||
### an_unknown_reshape_set_invalidates_everybody
|
||
|
||
The fail-safe arm of the same narrowing, tested separately because it is the half
|
||
gotcha 24 calls strictly worse than doing nothing. An empty reshaped set means the
|
||
batch moved a cursor with no view observed to change shape; the answer must be
|
||
"everybody", not "nobody", or the diff believes each client already holds
|
||
origin-positioned content and sends nothing, leaving them stale permanently.
|
||
|
||
Injected fault: delete the `reshaped.is_empty() ||` disjunct. Fails by name. That
|
||
injection is why the predicate is a named function rather than a closure — a
|
||
fail-safe nothing can point at is a fail-safe that gets tidied away.
|
||
|
||
### a_batch_that_changes_the_layout_and_then_saves_ends_clean
|
||
|
||
A regression introduced by the first version of the observed-`dirty` change and
|
||
caught in review of PR #70, not by the suite — the workspace was green at 1400 with
|
||
this broken. `observe_reshape` ran **after** the effect loop and re-raised
|
||
`model_state.dirty` from the `document_config` comparison, while `Save` had cleared
|
||
it **inside** the loop. So gotcha 45's stated ordering property — "`Save` clears it
|
||
in loop order, so a batch that edits and then saves ends clean" — survived for
|
||
`CellData`/`WholeWorkbook` and was broken for `LayoutGeometry`.
|
||
|
||
`[SetNumberFormat, Save]` reported a successful save and left the document dirty:
|
||
the TUI draws "Saved to …" while `Quit`'s check refuses a later `:q`, and the
|
||
browser worker — whose persist trigger *is* the flag — pays a whole-document OPFS
|
||
rewrite. Reachable from `:format ,.2f . w` and from any `Binding::Sequence` pairing
|
||
a geometry effect with a save.
|
||
|
||
The fix is position, not logic: the comparison moved inside the loop, taken after
|
||
each `LayoutGeometry` effect against the value held after the previous one. Both
|
||
arms are in the test — `[SetCell, Save]` never left the loop, so the geometry arm
|
||
alone could not say whether the rule is general — and each asserts the save really
|
||
happened (`last_batch_aborted`, and a status line starting "Saved") before
|
||
asserting the flag, or a failed save would pass the dirty assertion for the wrong
|
||
reason.
|
||
|
||
Injected fault: move the comparison back after the loop. Fails naming the layout
|
||
arm, with 454 others passing.
|
||
|
||
The lesson worth carrying: **an observation that is correct about *what* changed
|
||
can still be wrong about *when* it is read.** Everything above this entry argues
|
||
that observing beats declaring; this is the cost of that move, and it lands
|
||
precisely on the one effect that *unsets* what the observation sets.
|
||
|
||
### the_published_basis_names_the_records_the_grid_draws_and_no_others
|
||
|
||
improvise-23qd. The set of records a `Projection::Basis` carries was derived
|
||
**twice** — `ClientSession::basis_update`, the producer, and `server_truth`, a
|
||
`#[cfg(test)]` helper that is the **yardstick every fold test measures against**.
|
||
So a change to which records get published moved the thing and its own measure
|
||
together, and every test stayed green whatever the change was. Not a helper that
|
||
fabricates an input; a helper that fabricates the expected *output*.
|
||
|
||
Both now call `published_basis`. Note what that alone does **not** buy: the
|
||
ticket's stated proof ("alter the filter and watch the basis tests fail") is
|
||
impossible once one function feeds both sides — they still move together. A shared
|
||
derivation removes the drift and removes the yardstick with it.
|
||
|
||
So the yardstick is spelled out rather than derived: this test names North, South
|
||
and East by hand for `app_with_a_reshapeable_grid`'s three records over six cells,
|
||
and asserts that a grid where every cell is an `Aggregate` publishes **none** —
|
||
because an aggregate names no record, so there is nothing to claim a version for.
|
||
|
||
Fault injected: `.take(2)` on the one surviving filter — the ticket's exact "change
|
||
which records get published" — fails this test **and nothing else in the crate**.
|
||
A second injection, `record_version_of(..) + 1`, is caught instead by the two
|
||
end-to-end `basis_check` tests, which is the split worth knowing: the counters have
|
||
other guards, the membership had none.
|
||
|
||
The fixture carries its own premise check. The first version assumed a new category
|
||
lands on `Axis::None`; it lands on `Column`, and the assertion said so instead of
|
||
the test quietly proving nothing.
|
||
|
||
### an_aborted_batch_bumps_layout_version_for_what_it_did_run
|
||
|
||
improvise-2avl, with its negative twin `an_aborted_batch_bumps_nothing_for_effects_it_never_ran`.
|
||
PR #70 moved the `layout_version` bump from "once per `LayoutGeometry` effect,
|
||
inside the loop" to "once per batch, after the loop". Gotcha 40 records the in-loop
|
||
position as load-bearing — an aborted batch must not claim effects it never ran —
|
||
and the author's argument for why end-of-batch is safe *for `layout_version`
|
||
specifically* (nothing ever clears it, so an end comparison sees only what ran,
|
||
whereas `dirty` must stay in the loop because `Save` unsets it mid-batch) is sound
|
||
and was written into `context/code/command.md`. **An argument is not a test**, and
|
||
neither arm had one.
|
||
|
||
Both arms are proved **independently**, which is the part that earns them: bumping
|
||
unconditionally fails only the second test, and skipping `observe_reshape` on abort
|
||
fails only the first. A single injection failing both would not have shown that the
|
||
two tests see different things.
|
||
|
||
Each fixture asserts `app.last_batch_aborted()` before asserting anything about the
|
||
counter, because a batch that did not actually abort would pass both arms while
|
||
testing neither.
|
||
|
||
### sorting_the_store_in_records_mode_invalidates_the_cursor
|
||
|
||
improvise-ni6k names `SortData` as its **sharpest** case: it changes neither the
|
||
row count nor any label, and every position still names a different record. A guard
|
||
written as "did the shape change" misses it; `pending_edits` are keyed by row index
|
||
(gotcha 1), so row 0 being West and then East is gotcha 18's misdirected-write
|
||
disease arriving through a different door.
|
||
|
||
`view_frame` therefore hashes each record's **coordinates** alongside the axis
|
||
entries, because records-mode `row_items` are positional — `0,1,2…` whatever order
|
||
the store is in. Fault injected: deleting those three lines fails this test and
|
||
**nothing else in the workspace**.
|
||
|
||
**The fixture trap, which is the durable part.** PR #78 first shipped without the
|
||
coords hash, on the stated ground that no fixture could be built where `SortData`
|
||
reorders anything. That was wrong, and the reason is worth more than the fix:
|
||
`ToggleRecordsMode` pushes `effect::SortData` as its **first** effect, so a
|
||
records-mode fixture built by pressing `R` has an already-sorted store *by
|
||
construction* and cannot demonstrate anything order-sensitive. The store itself was
|
||
fine all along. So this fixture sets `_Index`/`_Dim` directly rather than going
|
||
through the key path.
|
||
|
||
The test asserts the count and the labels are **unchanged** before asserting the
|
||
cursor moved, so it cannot pass by accidentally becoming an ordinary shape change.
|
||
|
||
### paging_with_nothing_pruned_invalidates_no_cursor
|
||
|
||
The negative twin of `paging_across_a_prune_that_drops_a_row_invalidates_the_cursor`,
|
||
and the reason `view_frame` hashes **no page selection**.
|
||
|
||
improvise-7kvw is real only under `prune_empty`: a page flip changes which records
|
||
are visible, not which categories frame them, so with pruning off a grid position
|
||
keeps naming the same cell. PR #76 ruled that homing the cursor on every `[`/`]`
|
||
regardless — `[`/`]` being navigation a user does constantly — is **worse than the
|
||
hole**. Hashing the page selection does exactly that.
|
||
|
||
The seductive argument for hashing it anyway is that two pages with identical row
|
||
labels really are different data. True, and irrelevant: that is `record_versions`'
|
||
question, and `view_frame` answers the cursor's. A basis argument applied to a
|
||
cursor decision is how this regression got written in the first place.
|
||
|
||
Fault injected: restoring the three `page_selections` lines fails this test and
|
||
nothing else. The positive arm keeps passing either way, because
|
||
`with_frozen_records` applies `prune_empty` and so already sees a dropped row in
|
||
the axis entries — which is why the negative arm is the one that had to exist.
|
||
|
||
### a_write_that_prunes_a_row_out_of_another_clients_view_invalidates_it
|
||
|
||
improvise-mcq4's sting, found by the adversarial review of PR #78. The mcq4 fix
|
||
narrowed the frame fingerprint from every view to the active one, on the strength
|
||
of a measurement showing the narrowing cost nothing. **It bought its speed partly
|
||
with correctness**: a `CellData` write that prunes a row out of *another* client's
|
||
view became invisible. `view_shapes` is config-only — it hashes the `prune_empty`
|
||
flag, not which rows survive — so `last_reshaped_views` is empty, the one-view
|
||
frame covers only the host's, and a client on the other view is never sent
|
||
`ResetCursor` while its grid loses a row underneath it. That is gotcha 24's
|
||
"strictly worse than doing nothing": the diff then believes the bystander is
|
||
current, so it stays stale **permanently**.
|
||
|
||
The repair keeps the narrow one-view frame on the `LayoutGeometry` arm — the
|
||
held-key path improvise-748f exists for, where other views cannot reframe because
|
||
`view_mut()` writes only the active one — and restores the all-views frame on the
|
||
`CellData` arm. Measured: **0%** on the held-key path, **+5.7%** on an 18k-cell
|
||
two-view `SetCell`, scaling linearly in view count.
|
||
|
||
**Writing this test found a second bug, which was masking the first.** The
|
||
precondition failed at first: `grid_in` built a non-active view with
|
||
`GridLayout::new`, which does **not** apply `prune_empty`, so a client on another
|
||
view was projected three rows where its own config gives two. Their grid never
|
||
pruned, so it never lost a row, so the staleness was unobservable — a bug hiding a
|
||
bug. It is the third instance of the `new`-versus-`with_frozen_records` trap
|
||
(`view_frame` was the second), and fixing it broke no other test.
|
||
|
||
Fault injected: removing the all-views arm fails this test, plus
|
||
`sorting_the_store_in_records_mode_invalidates_the_cursor` (`SortData` is
|
||
`CellData`) and both cursor-audience tests — four in all.
|
||
|
||
### a_switch_then_a_geometry_effect_compares_within_one_view
|
||
|
||
Found by the same review. `frame_basis` was taken once at batch open against
|
||
`view_state.active_view`; `SwitchView` is `ViewOnly`, so it did not re-base, and
|
||
the next model effect hashed the **new** view and compared it against the **old**
|
||
view's hash. A near-certain mismatch, so a spurious `reset_cursor()` and a wrong
|
||
name in `last_reframed_views`.
|
||
|
||
Today's instances are benign — `DrillIntoCell` and `EnterRecordsMode` both emit
|
||
`[CreateView, SwitchView, SetAxis, …]` in one batch, but against a view they just
|
||
created. The harmful case needs a switch to an *existing* view with a saved cursor
|
||
plus a model effect in the same batch, which a `.`-chained `:` line reaches. Low
|
||
severity, unsound mechanism.
|
||
|
||
The basis is now keyed by view name and re-bases rather than comparing when the
|
||
name moves. Fault injected: dropping the `was == self.view_name()` guard fails
|
||
this test, with `cursor_epoch` moving for a batch that reframed nothing.
|
||
|
||
improvise-9c9o moved *where* the re-base happens, and this test was unchanged by
|
||
it because it names the property, not the mechanism — the re-base now fires on
|
||
the switch rather than on the next geometry effect, and a batch that reframes
|
||
nothing still invalidates nothing.
|
||
|
||
Its fixture carried a line that did nothing at all:
|
||
`views["Second"].clone_into(&mut views["Default"].clone())` clones into a
|
||
temporary that is immediately dropped. It read as the thing making the two views
|
||
differ, so had `create_view` ever started copying the active view's config this
|
||
test would have gone vacuous with that line standing there looking like it
|
||
prevented exactly that. It is deleted and replaced by the assertion it was
|
||
pretending to be: the two views' `view_frame` hashes really differ.
|
||
|
||
### a_geometry_effect_after_a_mid_batch_switch_still_invalidates_the_cursor
|
||
|
||
The other side of the guard above, and introduced by it. PR #78 round 1 compared
|
||
frame hashes across a view change, which threw a cursor away spuriously; round 2
|
||
added `was == self.view_name()`, which fixed that and also **exempted the first
|
||
geometry effect after a mid-batch `SwitchView` from the check altogether** —
|
||
there is no basis for the new view yet, so the code re-based and skipped.
|
||
Passes on pre-#78 `main` and on round-1 head; fails at the merged head.
|
||
|
||
Direction is why this is worth more than the bug it replaced. Round 1 kept a
|
||
cursor it should have thrown away; round 2 threw away nothing it should have
|
||
kept — which is gotcha 35's expensive side, since a stale cursor has nothing in
|
||
the stream to correct it where a spurious invalidation costs one projection.
|
||
|
||
The fix is the ticket's: **the basis follows `active_view`**, re-based in the
|
||
loop as soon as the name moves, rather than at the next geometry effect. It did
|
||
**not** need `SwitchView` to stop being `ViewOnly` and it is not a protocol
|
||
change — the basis is `App`'s own bookkeeping, and nothing about it is on the
|
||
wire. The `was == self.view_name()` guard is gone from the comparison because it
|
||
is now unreachable: by the time the geometry arm runs, the name and the basis
|
||
agree by construction.
|
||
|
||
What it changes elsewhere, and why that is fine: `DrillIntoCell` and
|
||
`EnterRecordsMode` both emit `[CreateView, SwitchView, SetAxis, SetAxis, …]`, so
|
||
they now reset the cursor on the *first* `SetAxis` where they used to skip it and
|
||
reset on the second. Same outcome, one effect earlier. `DeleteView` is the one
|
||
`LayoutGeometry` effect that moves `active_view` itself; the re-base fires for it
|
||
too, so the geometry arm compares the new view against itself and resets nothing
|
||
— which is what the old skip did.
|
||
|
||
The fixture measures the row count the transpose must move rather than spelling
|
||
it: a first version asserted `row_count() == 1` and the real answer is 2, which
|
||
is the shape of a precondition that would have gone quietly wrong later. It
|
||
fails on the unfixed tree at the epoch assertion, `left: 0 right: 0`.
|
||
|
||
### a_frozen_drill_does_not_take_its_opening_frame_from_the_held_layout
|
||
|
||
improvise-mcq4's fix reads the batch's opening frame off `App::layout` rather than
|
||
building a second pruned layout — that is where the 4.1×→1.9× came from. It is
|
||
guarded on `drill_state.is_none()`, because a drill freezes the record list and the
|
||
held layout is then **not** what `view_frame` would build for that view.
|
||
|
||
**The guard was correct, load-bearing, and tested by nothing**: reverting it to the
|
||
unconditional form left all 1439 tests passing. Textbook
|
||
`doctrine(a_careful_comment_is_weak_evidence_that_anything_checks_it)`, and it was
|
||
the most carefully argued comment in the PR.
|
||
|
||
The fixture freezes one record where the live query finds three — asserted, so a
|
||
freeze that did not narrow could not pass — then runs a `SetNumberFormat` batch,
|
||
which reframes nothing. Without the guard the opening basis is the frozen layout
|
||
and the in-loop hash is the live one, so they differ and the cursor is reset for a
|
||
batch that changed no row. Fault injected: replacing `self.drill_state.is_none()`
|
||
with `true` fails this test and nothing else in the workspace.
|
||
|
||
### a_clients_grid_does_not_depend_on_which_view_its_host_is_on
|
||
|
||
improvise-t6db. The one-line `grid_in` fix landed in PR #78 with no test of its
|
||
own; this is the general property the reviewer specced — **host-view
|
||
independence**: `grid_in(app_on_A, "B", vp) == grid_in(app_on_B, "B", vp)` for
|
||
every ordered pair of views. It needs no second derivation of what a view's
|
||
config means, which is why it is a property and not a second copy of
|
||
`GridLayout`.
|
||
|
||
**The trap is the fixture, not the assertion.** The reviewer's first version made
|
||
the two views differ by turning `prune_empty` **off** in the second — plenty of
|
||
differing config, and vacuous: with nothing to prune, `GridLayout::new` and
|
||
`GridLayout::with_frozen_records` agree and the property passed on the buggy
|
||
tree. So `app_with_three_views_and_an_empty_row` gives Service no cell at all and
|
||
leaves all three views pruning, and the test asserts every view really is two rows
|
||
before comparing anything — a fixture that stopped pruning would fail there rather
|
||
than passing quietly.
|
||
|
||
The property is stated with `drill_state.is_none()` asserted, because a frozen
|
||
drill snapshot is the one thing `grid_in` cannot reproduce for a view the host is
|
||
not on (`with_frozen_records(.., None)` on the elsewhere path).
|
||
|
||
Fault injected 2026-09-21: restoring `GridLayout::new` in `grid_in` — the pre-#78
|
||
line — fails this test at the `assert_eq!`, not at the precondition. It also fails
|
||
`a_write_that_prunes_a_row_out_of_another_clients_view_invalidates_it` and
|
||
`a_hosts_none_cats_leak_into_another_views_formula_columns`: **three** tests, not
|
||
the one an earlier draft of this entry claimed. The #78 fix has more coverage than
|
||
it was credited with.
|
||
|
||
**The `none_cats` asymmetry is REAL, and the first draft of this entry reported it
|
||
as unobserved from a fixture that lacked the condition.** That correction is worth
|
||
more than the test. The claim was that the fixture put `Product` on None in
|
||
`Default` and on Column in `Second`; it did not — `add_category` defaults a new
|
||
category to Column and nothing moved it, so all three views had `none_cats`
|
||
`{_Index, _Dim}`, the stated input to the hazard was absent, and `Default` and
|
||
`Second` were indistinguishable to `grid_in` apart from a `number_format` that is
|
||
itself a no-op. An unprobed path reported as a negative result, which is
|
||
`doctrine(an_injection_that_does_not_fail_names_an_untested_path)` applied to a
|
||
**setup**, and the expensive kind: it stops the next person looking.
|
||
|
||
Setting `Product` to `Axis::None` explicitly made the property fail on the first
|
||
run, and not on the value of a cell — `Second` lost **both `Total` columns
|
||
entirely** when the host stood on `Default`. Filed as improvise-ac2z, pinned by
|
||
the characterization test below, and the property now skips pairs whose
|
||
`none_cats` differ. That skip is a **bug being worked around**, not a carve-out;
|
||
it asserts `compared > 0` so a future fixture cannot skip everything and pass.
|
||
|
||
|
||
### a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths
|
||
|
||
improvise-jmjz. `project::grid_of` evaluated the whole grid roughly three times
|
||
per frame — `compute_col_widths` walked every cell for its text and then every
|
||
cell again for the column totals it must size, and `grid_of` then evaluated the
|
||
window a third time. `ui/viewport.rs::grid_body` now evaluates each cell once and
|
||
derives the widths, the totals and the window from that value.
|
||
|
||
That fusion creates a hazard the three separate passes did not have: one walk now
|
||
serves three consumers with different scopes. **Two** faults follow, not one — a
|
||
window that leaks into the totals makes a column total the sum of the visible
|
||
rows, and a window that leaks into the widths sizes a column by what happens to
|
||
be on screen. Neither is visible from the output: the numbers simply get smaller,
|
||
consistently.
|
||
|
||
**The fixture took three attempts, and what each one was blind to is the entry.**
|
||
|
||
*First*: rows of `1`, `20`, `300000` under a measure named `Revenue`, asserting
|
||
`col_widths[0] > "300021".len()`. It could not see a windowed **width** at all —
|
||
`Revenue` is 7 characters against a 6-character value, so the assertion passed on
|
||
the **header** alone. Worse, its other half was structurally incapable: comparing
|
||
`grid_body`'s widths against `compute_col_widths` cannot test windowing, because
|
||
`compute_col_widths` **is** `grid_body` with an empty window. Windowing degrades
|
||
both sides identically, both collapsing to the `MIN_COL_WIDTH` clamp. **The same
|
||
function twice is not a check, and it reads exactly like one.** Measured by the
|
||
PR #92 review: `if drawn { widen(..) }` left **1476 passing, 0 failing**.
|
||
|
||
*Second*, the reviewer's: `1`, `888888888`, `-888888888` with a one-character
|
||
measure name, so the total **cancels to 1.0** and neither it nor the header can
|
||
supply the width. This fixed the width half and broke the other: the windowed row
|
||
is `1.0` and the cancelled total is *also* `1.0`, so the **totals** assertion went
|
||
blind to the row-window fault, which the widths assertion then happened to catch.
|
||
Two assertions, one no longer discriminating, and nothing saying which.
|
||
|
||
*Third*, and current: the same values with the **large ones first**, so the window
|
||
holds `888888888` while the total is `1.0`. Each injection now fails on the
|
||
assertion that names it:
|
||
|
||
- `for ri in rows.clone()` — the whole walk windowed → **totals**
|
||
`[888888888.0]` against `[1.0]`
|
||
- `if drawn { widen(..) }` — widths only → **widths** `[10]` against `[11]`
|
||
|
||
`10` rather than `5` is a smaller gap than the reviewer's fixture gave and a
|
||
better witness: the missing character is the **minus sign**, which only
|
||
`-888888888` carries and the window does not contain. The test also asserts the
|
||
window really is narrower than the grid, so a fixture that stopped windowing
|
||
would fail at the precondition.
|
||
|
||
**What this test does not do is prove the speedup**, and no test can — the outputs
|
||
are identical by construction, which is why the whole suite passed unchanged
|
||
across the change. The evidence is the benchmark, before and after, in
|
||
`context/code/command.md`: about 3x, the number of passes removed.
|
||
|
||
### a_formulas_columns_survive_a_host_that_aggregates_differently
|
||
|
||
improvise-ac2z, **fixed**. This replaces the characterization test
|
||
`a_hosts_none_cats_leak_into_another_views_formula_columns`, whose message said
|
||
that failing meant the bug was fixed; it did, and it is gone.
|
||
|
||
`App::rebuild_layout` calls `recompute_formulas(&none_cats)` for the **host's**
|
||
view, populating `Model::formula_cache`. `project::grid_in` on the elsewhere
|
||
path built the other view's `GridLayout` against that same cache, which is keyed
|
||
by the coordinates the recompute used: with `Product` on None in the host's view
|
||
the `Total` cells are keyed *without* a `Product` coordinate, a view with
|
||
`Product` on Column asks for `(Region, Product, _Measure=Total)`, finds nothing,
|
||
and — `prune_empty` being on — drops both `Total` columns. Not a wrong number:
|
||
absent columns.
|
||
|
||
host on `Second`, projecting `Second` 6 columns, `Total` present
|
||
host on `Default`, projecting `Second` 4 columns, no `Total` at all
|
||
|
||
The fix is `grid_in::rederived_for`: where the target view aggregates over
|
||
different categories than the host's, it clones the model and recomputes the
|
||
formulas for **that** view's `none_cats`. Three conditions gate the clone —
|
||
another view, different `none_cats`, and at least one formula — so the common
|
||
case pays nothing and a formula-free document pays nothing ever.
|
||
|
||
**This is an interim, and the cost is the reason.** A clone of the whole model
|
||
plus a fixed-point recompute, per projection, on exactly the path that is broken
|
||
today. The ticket's first candidate — keying `formula_cache` by `none_cats` so
|
||
one `Model` can hold more than one view's derived values — is the durable answer
|
||
and is filed separately; it is an `improvise-core` change and did not belong in
|
||
this wave's ground. What is bought meanwhile is correctness on a path that was
|
||
silently wrong, `doctrine(the_elsewhere_path_is_only_as_correct_as_the_inputs_it_can_rebuild)`.
|
||
|
||
**Fault injected**: `rederived_for`'s condition forced to `false`. Two tests
|
||
fail — this one, and
|
||
`a_clients_grid_does_not_depend_on_which_view_its_host_is_on`, whose `none_cats`
|
||
carve-out was removed at the same time and which is the general property this
|
||
one is an instance of. The test asserts its two **preconditions** first — that
|
||
`Second` on its own really draws the formula's columns, and that the two views
|
||
really do aggregate over different categories — so a fixture that lost either
|
||
fails rather than passing on two empties.
|
||
|
||
### a_view_named_command_lands_on_the_view_it_names
|
||
|
||
improvise-lltt, the oldest open P1 here, opened by improvise-jytm making
|
||
`ViewState::active_view` authoritative per client.
|
||
|
||
Nine `ModelCommand` variants configure a view — `SetAxis`, `CycleAxis`,
|
||
`TransposeAxes`, `SetPageSelection`, `HideItem`, `ShowItem`, `ToggleGroup`,
|
||
`TogglePruneEmpty`, `SetNumberFormat` — and each named its view *implicitly*:
|
||
`model_effects` built an effect that wrote `App::view_mut()`, which is the
|
||
**host's** slice. A client that had switched to `Second` therefore moved
|
||
`Default`: its own grid did not change, and every other client's did.
|
||
|
||
The fix is the protocol change the ticket asks for. Each of the nine carries a
|
||
`view: String`, `model_effects` maps it to `ViewTarget::Named`, and the nine
|
||
effects carry a `ViewTarget` rather than reading an ambient one. There is no
|
||
operandless form left: `App::view_mut()` is **deleted**, replaced by
|
||
`view_mut_of(&ViewTarget)`, so a future geometry effect has to say which view it
|
||
writes. The host's own key path passes `ViewTarget::Active`, which is right
|
||
because the TUI *is* the host and has no session to ask.
|
||
|
||
**Fault injected**: `model_effects`' `SetAxis` arm rebuilt with
|
||
`ViewTarget::Active` (the pre-fix behaviour, with `view` bound and discarded so
|
||
it still compiles). The test fails on `the asker's own view never moved` —
|
||
`Second`'s `Channel` stayed on `Row` while `Default`'s moved to `Page`.
|
||
|
||
The fixture must seat the asker somewhere the host is not, and the two views must
|
||
disagree about the category being moved: `Channel` is `Axis::None` in `Default`
|
||
and `Axis::Row` in `Second`, and the command asks for `Axis::Page`, so both
|
||
assertions separate the fixed code from the broken code in both directions.
|
||
|
||
### a_reframe_of_a_view_the_host_is_not_on_invalidates_its_clients
|
||
|
||
improvise-lltt's second half, and the part the ticket did not predict. Naming the
|
||
view made a reframe of a view the host is not on reachable for the first time,
|
||
and `Sessions::adopt_cursor_epoch` could not see one: it returned early unless
|
||
`App::cursor_epoch` had moved, and the epoch is bumped only by `reset_cursor`,
|
||
which `apply_effects` calls only for the **host's own** view. So the clients on
|
||
the reframed view were reset by nobody and told nothing — gotcha 24's
|
||
strictly-worse-than-nothing state, permanently stale rather than stale until the
|
||
next scroll.
|
||
|
||
`App::last_reframed_elsewhere` is the second trigger. Its opening frames are
|
||
taken lazily inside `App::view_mut_of`, which is the one door such a write goes
|
||
through, so the held-key path still hashes exactly one view: the wide-observation
|
||
cost improvise-mcq4 declined is not paid.
|
||
|
||
**Fault injected**: the gate restored to `if !host_moved { return Vec::new(); }`.
|
||
The test fails on `a client on the reframed view was never told`.
|
||
|
||
Two negative arms, and they are not decoration. A third client left on `Default`
|
||
must keep its offset, or "invalidate everyone" passes; and
|
||
`paging_with_nothing_pruned_invalidates_no_cursor` is what forbids relaying
|
||
against `last_reframed_views`, which is a union with the *reshaped* set and so
|
||
fires for a page selection that pruned nothing. That test failed during
|
||
development and is why the elsewhere set is precise.
|
||
|
||
### a_command_naming_a_view_the_document_has_not_got_writes_nothing
|
||
|
||
`App::resolve_view` falls back to the document's default for a name the document
|
||
has not got, which is right for *reading* — a slice naming a deleted view must
|
||
not panic in `rebuild_layout` — and wrong for **writing**, where it would move a
|
||
view nobody asked about. `view_mut_of` therefore looks the name up directly and
|
||
returns `None`.
|
||
|
||
**Fault injected**: `view_mut_of`'s `Named` arm routed through
|
||
`self.resolve_view(name).0`. The test fails on `a name the document has not got
|
||
fell back onto a view that has one`.
|
||
|
||
The window this closes is narrow — `Sessions::adopt_deleted_views` already moves
|
||
a mirror off a deleted view and tells it — but it is a race, not an
|
||
impossibility, and the silent alternative is a write to the wrong document state.
|
||
Not closed here: the client hears nothing. A `Rejection` variant for it is filed
|
||
rather than smuggled in, since `Rejection` is wire vocabulary.
|
||
|
||
### the_diff_tells_a_client_the_grid_became_a_records_grid
|
||
|
||
The producer half of `GridCache::records` (improvise-e5pf). `grid_of` setting
|
||
the field is exercised by `leaving_a_panel_in_records_mode_lands_where_the_host_lands`;
|
||
the **diff** is a separate arm, and a client that connects before records mode
|
||
is entered learns only through it.
|
||
|
||
Both preconditions are asserted — the fixture starts on an ordinary grid, and
|
||
`R` really reaches a records one — so the comparison is between two answers
|
||
that had something to disagree about.
|
||
|
||
**Fault injected**: the `records` arm deleted from `grid_updates`. The test
|
||
fails on `the client is never told`. The negative arm (an unchanged flag is not
|
||
resent) is what stops "always push it" from passing.
|
||
|
||
### a_batch_that_leaves_records_settles_out_of_records_normal
|
||
|
||
improvise-4dpj, gap 1. `settle_normal_mode` matches `Normal | RecordsNormal`,
|
||
and replacing that with a one-way `AppMode::Normal` left the **entire workspace
|
||
suite passing**. The existing crossing test,
|
||
`a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal`, drives the
|
||
layout both ways, but both of its branches end in `enter-mode normal`, so the
|
||
mode handed to the settle is `AppMode::Normal` in both directions. It
|
||
discriminates "settles at all" and "settles against the ending layout", and not
|
||
"settles from **either** mode".
|
||
|
||
This drives the production repro: from a records grid,
|
||
`apply_effects([SetAxis { "_Index", Axis::None }])` leaves
|
||
`layout.is_records_mode()` false with the mode still `RecordsNormal`, so the
|
||
`RecordsNormal` arm is live and not defensive. Both the records-ness of the
|
||
grid and the mode before the batch are asserted as preconditions, so a fixture
|
||
that stopped entering records mode fails loudly rather than proving nothing.
|
||
|
||
Injected fault: `if matches!(self.view_state.mode, AppMode::Normal)`. Fails by
|
||
name, `left: RecordsNormal, right: Normal`. It is the only test in the
|
||
workspace that fails.
|
||
|
||
### switching_views_across_records_ness_settles_against_the_view_switched_to
|
||
|
||
improvise-4dpj, gap 2 — raised by ChatGPT Codex against PR #91 and verified
|
||
correct on `main` before being tested. `SwitchViewAtCursor` emits
|
||
`[SwitchView(target), ChangeMode(Normal)]`, and this is a **different path**
|
||
from the one gap 1's siblings drive: that test toggles records mode explicitly,
|
||
where a view switch crosses records-ness only because it is auto-detected from
|
||
the axes (gotcha 1). Nothing in the batch says "records".
|
||
|
||
Driven in both directions, with the crossing asserted before and after so a
|
||
fixture that stopped crossing fails rather than passes. Injected fault: move
|
||
`settle_normal_mode()` back above `rebuild_layout()` at the end of
|
||
`apply_effects` — the exact regression PR #91's round 2 fixed. This test fails
|
||
by name; so does gap 1's, along with six pre-existing records-mode tests
|
||
(measured in review; an earlier draft of this entry said five).
|
||
|
||
**Scope note, stated because an overclaim is worse than a gap**: this drives
|
||
the *effects* `SwitchViewAtCursor` emits, not the view-panel key path that
|
||
composes them. Reaching the panel needs a `view_panel_cursor` on a `CmdContext`
|
||
built in `command/cmd/panel.rs`, which is another wave's ground this round. The
|
||
command's own coverage lives there; what is unexercised is the two joined.
|
||
|
||
### the_layout_geometry_cases_are_every_wire_effect_of_that_kind
|
||
|
||
improvise-26dm. `layout_geometry_effects_actually_change_document_config` was
|
||
guarded by `assert_eq!(cases.len(), 13)`, which catches a case being *deleted*
|
||
from the list and never one being *absent* from it — and the two are written
|
||
identically as an `assert_eq` over a number. The adversarial review of PR #78
|
||
added a fourteenth `pub struct` declaring `LayoutGeometry`, pointed at by
|
||
nothing, and the whole suite stayed green.
|
||
|
||
improvise-5ubs's mechanism cannot reach it: an effect is a struct implementing
|
||
a trait, so serde has no derived enumeration to diff against. `model_effects`
|
||
maps each `ModelCommand` variant back to the effect it came from (gotcha 38),
|
||
so the list is diffed against the `LayoutGeometry` effects reachable through
|
||
`ModelCommand`. The two sets are equal today, which is the evidence the
|
||
substitution lost nothing.
|
||
|
||
**The right-hand side is bounded by the derive, and that took a second round.**
|
||
Review of PR #96 injected the deletion of `ModelCommand::TogglePruneEmpty` from
|
||
`every_model_command()`: this was the **only** test in the workspace that
|
||
failed, and `every_model_command`'s doc comment claiming the list was
|
||
compiler-forced was wrong — `variant_name`'s exhaustive match forces an arm
|
||
*there*, not an entry in the list. ChatGPT Codex raised the same thing on the
|
||
PR. `the_enumeration_names_every_variant_the_derive_declares` closes it, so the
|
||
chain is derive → enumeration → `model_effects` → case list with every link
|
||
checked. The assertion here still names **both** directions, because a short
|
||
right-hand side reads as a spurious case on the left and invites deleting it —
|
||
degrading both sides together.
|
||
|
||
Injected fault: drop `TogglePruneEmpty` from `layout_geometry_cases()`. The old
|
||
count said `12 != 13`; this says
|
||
`right: [… "TogglePruneEmpty" …]` and **names the effect**.
|
||
|
||
**Residual, stated rather than closed**: this is total over the wire and not
|
||
over the effects. An effect declaring `LayoutGeometry` that no `ModelCommand`
|
||
names is still invisible — which is precisely PR #78's probe. Closing that
|
||
needs an enumeration of effects the compiler maintains, and none exists without
|
||
a proc macro or inventory-style registration.
|
||
|
||
### the_enumeration_names_every_variant_the_derive_declares
|
||
|
||
The bound the entry above rests on. Injection is Codex's own, run end to end: a
|
||
`ModelCommand::ProbeGeometry` whose effect declares `LayoutGeometry`, with the
|
||
`model_effects` and `variant_name` arms the compiler forces, omitted from
|
||
**both** lists. Before this test, the whole workspace stayed green. After it,
|
||
the failure is staged and each stage names the probe — first
|
||
`every_model_command() never constructs ["ProbeGeometry"]`, then, once the
|
||
enumeration is repaired and only the case list is short,
|
||
`the_layout_geometry_cases_are_every_wire_effect_of_that_kind` with
|
||
`ProbeGeometry` on the right. That staging is the point: the two tests answer
|
||
different questions and the injection separates them.
|
||
|
||
The name reader is shared, not copied: `reduce.rs`'s `effect_type_name` and its
|
||
own `the_effect_name_reader_reads_all_three_debug_shapes` are what this leans
|
||
on, because a second reader here would be a derivation done twice and could
|
||
drift while one copy was repaired. It reads the leading identifier off the
|
||
derived `Debug`, `Effect` being a trait object that `type_name` cannot reach.
|
||
|
||
---
|
||
|
||
## `command/cmd/panel.rs` — the TUI half of the category-panel defects
|
||
|
||
Both of these have a twin in `improvise-protocol/src/resolve.rs` whose leaf
|
||
name is identical, so both headings are module-qualified (gotcha 48). Neither
|
||
defect was browser-specific: the same `Cmd` backs the same key in the TUI, and
|
||
confirming that before fixing is what kept the fix out of the client.
|
||
|
||
### command::cmd::panel::tests::deleting_a_categorys_last_item_leaves_the_cursor_inside_that_category
|
||
|
||
improvise-uej8 in the TUI. The full argument, including why the rival
|
||
"always step back" repair is worse than the bug, is under
|
||
`resolve::tests::deleting_a_categorys_last_item_leaves_the_cursor_inside_that_category`
|
||
in `clients.md`; it is not repeated here, because two copies of an argument
|
||
are two things to keep true.
|
||
|
||
What this test adds that its twin cannot: the TUI tree carries the three
|
||
**virtual** categories ahead of the regular ones (improvise-vjms, gotcha 7),
|
||
so its indices are the ones a real user's cursor holds. Row 5 is `Type`'s last
|
||
item and row 6 is `Month`'s header; the test asserts both before acting, so it
|
||
cannot pass by landing somewhere the escalation was never possible. Inject the
|
||
fault by deleting the `effects.extend(retreat_from_deleted_item(...))` line.
|
||
|
||
### command::cmd::panel::tests::deleting_an_item_with_a_sibling_below_it_leaves_the_cursor_alone
|
||
|
||
The discriminating input for the rival fix, in the TUI. Fails under an
|
||
unconditional step-back, passes under the conditional one.
|
||
|
||
### command::cmd::panel::tests::deleting_a_categorys_only_item_does_not_arm_the_next_d_with_remove_category
|
||
|
||
The singleton arm in the TUI. The enumeration and the landing argument are
|
||
under the `resolve.rs` twin of this name in `clients.md`; they are not repeated
|
||
here, because two copies of an argument are two things to keep true.
|
||
|
||
What this fixture adds: `Month` is seated **before** `Type` and carries two
|
||
items, so the safe landing is found by the **backward** search — the preferred
|
||
direction — and lands on a real item rather than falling off the end of the
|
||
tree. Its twin exercises the same case with nothing below. Both assert the
|
||
consequence (a second `d` composes no `RemoveCategory`) rather than the landing
|
||
index, and both fail under the pre-review rule while the two adjacent-case
|
||
tests pass.
|
||
|
||
### command::cmd::panel::tests::cycling_an_axis_from_the_category_panel_says_so_as_the_tile_bar_does
|
||
|
||
improvise-25bf in the TUI. The category panel and the tile bar are two routes
|
||
to one operation and only the tile route told the user, which is
|
||
design-principles §5 in miniature — a derivation done twice will diverge, and
|
||
the divergence is invisible until someone puts them side by side. The wording
|
||
now has one home, `viewmodel::axis_change_status`, called by both routes here
|
||
and by both in the browser.
|
||
|
||
The fixture sets `Type` to `Row` explicitly rather than reading the default,
|
||
so the expected `Type → Col` is a literal and not a restatement of the code
|
||
under test. It asserts `cat_at_cursor()` is `Type` first, because cursor 3 is
|
||
only the first regular category while the three virtual ones lead the tree —
|
||
an off-by-one would otherwise pin the wrong category's sentence and still pass.
|
||
|
||
---
|
||
|
||
## The view stack is the client's (improvise-gd28)
|
||
|
||
Two user rulings, 2026-09-21: *"The state drill manipulates (the view stack)
|
||
should be local to the client"* and *"Drilling should be an ephemeral view, not
|
||
something that gets persisted."* The first one is what these three tests serve.
|
||
|
||
### a_client_folding_a_switch_records_the_history_the_host_does
|
||
|
||
**The bug, demonstrated before the fix.** `effect::SwitchView` pushes the view
|
||
it is leaving onto `ViewState::view_back_stack` and clears the forward stack;
|
||
`reduce_view`'s `Command::SwitchView` arm set `active_view` and **nothing
|
||
else**. Both write the same portable field of the same type, and the fold is
|
||
supposed to be the mirror's copy of the effect (improvise-cqq/m91) — so a
|
||
connected client's own history was empty forever, however many views it visited.
|
||
|
||
That is the whole reason `view-back` reads as unavailable in a browser: not
|
||
that stepping back is hard, but that the client has no stack to pop. The
|
||
`declined()` comment attributes it to the drill commit instead, which is a
|
||
second, separable reason (see the drill note below).
|
||
|
||
Written against the unfixed tree it fails with
|
||
`left: [] right: [ViewFrame { view_name: "Default", mode: Normal }]`. It drives
|
||
the host through `apply_command` and the mirror through `reduce_view`, so it
|
||
compares the two writers rather than one writer against itself.
|
||
|
||
### a_client_stepping_back_lands_where_the_host_lands
|
||
|
||
The lockstep for the two new commands. `effect::ViewBack` restores
|
||
`frame.mode` as well as the view name, so a fixture left in `Normal` cannot
|
||
tell "restores the mode" from "never touched the mode" — the mode the default
|
||
supplies is exactly the mode a broken restore leaves behind. The fixture
|
||
therefore enters `Help`, switches away, drops to `Normal`, and asserts `Help`
|
||
comes back. Deleting the `view.mode = frame.mode` line fails it; deleting it
|
||
from a `Normal`-only fixture would not.
|
||
|
||
`stepping_forward_undoes_stepping_back` and
|
||
`a_step_off_the_end_of_the_history_moves_nothing` bound the two ends: the
|
||
direction is a `Rewind` enum rather than a `bool` so the two arms cannot read
|
||
each other's stack, and an empty stack is a no-op rather than a panic.
|
||
|
||
### a_step_back_cannot_land_on_a_view_the_document_has_lost
|
||
|
||
The case the client-local stack **creates**, stated where a user feels it: a
|
||
frame on my stack can name a view somebody else deleted while I was away.
|
||
|
||
**This test replaced a guard.** The first version answered it inside
|
||
`Sessions::apply_step`, which folded the step and then corrected a dead landing
|
||
the way `apply_switch` corrects a refused switch. Two injections did fail it —
|
||
but the correction folds a `SwitchView`, which **pushes the dead view straight
|
||
back onto the stack**, so pressing `<` again popped it, was corrected again,
|
||
and pushed it again: a back button that never progresses. Pruning the history
|
||
when the view vanishes (see the entry above) makes the dead frame
|
||
unrepresentable instead, which left `apply_step`'s guard with no reachable
|
||
input. It was **deleted rather than kept as defence in depth**, because a guard
|
||
whose fault cannot be injected is indistinguishable from one that always
|
||
passes, and `apply_checked`'s existing fold arm is then the whole behaviour.
|
||
|
||
The fixture visits `Third` and then `Second`, so the stack is
|
||
`[Default, Third, Second]` and only the **top** frame is doomed. Landing on
|
||
`Third` is what discriminates: a fixture whose stack held only the dead frame
|
||
would land on `Default` and could not tell "skipped it" from "gave up and went
|
||
home". Injected fault: drop `forget_vanished_views` from `apply_model` — the
|
||
step lands on `Second`, a view the document has not got.
|
||
|
||
### deleting_a_view_forgets_every_frame_that_names_it
|
||
|
||
**A defect the client-local stack creates, and a pre-existing one on the host.**
|
||
A `ViewFrame` is somewhere you can go *back to*, so a frame naming a view that
|
||
has been deleted is an offer the document cannot honour. `effect::DeleteView`
|
||
moved `active_view` off the doomed view and left both stacks alone, which was
|
||
invisible while no client had a stack at all — the TUI user could already reach
|
||
a dead frame with `<`, and making `reduce_view` record history would have given
|
||
every browser client the same.
|
||
|
||
Worse than merely dead: without the pruning, `Sessions::apply_step` corrects
|
||
the landing by folding `SwitchView(default)`, which **pushes the dead view back
|
||
onto the stack**. Pressing `<` again pops it, is corrected again, and pushes it
|
||
again — a back button that never progresses, and one that looks from the
|
||
outside like a key that does nothing.
|
||
|
||
Injected fault: delete the `forget_view` call from `effect::DeleteView`. The
|
||
test names the surviving frame. Its fixture asserts the doomed view is on the
|
||
stack *before* the delete, so it cannot pass by pruning nothing from nothing.
|
||
|
||
### a_deleted_view_leaves_the_history_of_a_client_that_did_not_delete_it
|
||
|
||
The multi-client half, and the one `adopt_deleted_views` cannot cover: that
|
||
helper looks at mirrors **sitting on** the deleted view, and a client that
|
||
merely *visited* it is sitting somewhere else entirely. The bystander here is
|
||
therefore on `Third`, holding `Second` on its back stack — which is also what
|
||
makes `ToAll` the right audience and checkable: every client may have been
|
||
there, and the asker is not special.
|
||
|
||
Which views vanished is **observed** by diffing the name set across
|
||
`apply_command`, for the reason `adopt_cursor_epoch` watches a counter and
|
||
`apply_model` watches `document_generation` (gotchas 24, 39): which effects
|
||
drop a view is a per-effect decision, so a table of command variants would be a
|
||
second description of it, free to disagree.
|
||
|
||
Injected fault: drop the `forget_vanished_views` call from `apply_model`. The
|
||
bystander keeps its dead frame and the test names it.
|
||
|
||
### a_refused_switch_puts_no_phantom_on_the_history
|
||
|
||
**Found by ChatGPT Codex on PR #115, and it is that PR's own regression.** Once
|
||
`reduce_view` records history, the *correction* for a refused switch records
|
||
history too. Starting on `Default`, the asker folds `Missing` (back gains
|
||
`Default`), then folds the correction back to `Default` (back gains
|
||
**`Missing`**) — while the mirror, which never folded either, stays empty. So
|
||
`<` afterwards lands on a view the document never had, and `forget_view` cannot
|
||
help: nothing *vanished*, the name was never real.
|
||
|
||
**It is reachable**, which is why it was fixed rather than argued away. No
|
||
shipped client can invent a view name — `resolve.rs` composes `SwitchView` from
|
||
a `ViewSummary` in the projected list — but a client can pick a name that
|
||
another client deletes before the frame arrives. That race is exactly the case
|
||
`forget_vanished_views` narrows and does not close.
|
||
|
||
Two things the fix needed, and the test separates them:
|
||
|
||
- **The mirror folds what the client folded**, refused or not. A correction can
|
||
only undo what both ends hold; guarding the mirror is what let the two drift.
|
||
- **The correction is `[ViewBack, ForgetView(asked)]`** — the exact inverse of
|
||
the push, then the refused name off the forward stack it lands on.
|
||
|
||
Injections, each failing for its own reason:
|
||
|
||
- **Drop `ForgetView`**: the phantom assertion fires — `Missing` is on the
|
||
history.
|
||
- **The rival fix, `SwitchView(leaving)` instead of `ViewBack`** (which is what
|
||
the review's own wording suggests, and what the code did before): no phantom,
|
||
but `left: [Default, Second]` against `[Default]` — the redundant frame the
|
||
correction pushed. That injection is the reason the test asserts the back
|
||
stack is *restored* rather than merely free of the phantom.
|
||
|
||
**One residue, asserted rather than hidden**: the optimistic fold clears the
|
||
forward stack and no correction can refill it, so a refused switch costs the
|
||
redo history — exactly as an accepted one does. The test pins it empty, and the
|
||
fixture loads **both** stacks first so a lost forward stack is visible at all.
|
||
|
||
The neighbouring test `a_refused_switch_corrects_the_asker_and_nobody_else`
|
||
compared `active_view` alone and stayed green through all of this: the check
|
||
that answers a neighbouring question, in the place the doctrine warns about.
|
||
|
||
---
|
||
|
||
## The format a projected cell is drawn with (improvise-e0ex)
|
||
|
||
### a_formulas_format_reaches_its_own_column_and_no_other
|
||
|
||
`context/code/core.md` recorded, as the more useful half of improvise-711n's
|
||
entry, that `ui/project.rs::grid_of` "has no test that renders a cell and
|
||
asserts its digits". This is that test, and it is why the gap was worth naming:
|
||
the per-formula format is resolved in `improvise-core` and could have been
|
||
resolved correctly there while never reaching the grid.
|
||
|
||
It asserts four things that fail independently: the formula column's cell, the
|
||
raw column's cell beside it, and **both** totals. The totals matter separately
|
||
because they are formatted on their own path — `GridBody::col_formats`, read off
|
||
the top row — so a correct cell says nothing about the strip under it.
|
||
|
||
Two faults injected:
|
||
|
||
- `FormatPlan::at` returning the view's format for every key fails the cell
|
||
assertion (`left: Some("0.50") right: Some("0.5000")`).
|
||
- Dropping only the `col_formats` write in `viewport::grid_body`, leaving the
|
||
cells correct, fails the totals assertion (`left: "1.50" right: "1.5000"`).
|
||
|
||
The second is the one that earns the test its length: without it, the totals
|
||
strip could round every formula column to the view's format and no assertion in
|
||
the workspace would move.
|