d9b10905d0
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
3079 lines
176 KiB
Markdown
3079 lines
176 KiB
Markdown
# `improvise-command` — the arguments
|
||
|
||
The reasoning that used to live in this crate's comments (improvise-9c8c). Read
|
||
it with the code open; it is written for someone about to change one of these
|
||
files, not as an overview. `context/repo-map.md` is the "where",
|
||
`context/design-principles.md` is the "why in general"; this is the "why here".
|
||
|
||
Test bug-narratives live in `context/test-evidence/command.md`, keyed by test
|
||
name.
|
||
|
||
---
|
||
|
||
## `ui/reduce.rs` — the one door from a command to the authoritative state
|
||
|
||
### What the module is (improvise-gxi, corrected by improvise-s17o)
|
||
|
||
This module used to say that every deployment mode — the native TUI, a websocket
|
||
server, a browser worker — reaches the model through it. **That was aspirational,
|
||
and it is what let improvise-ondj's missing basis check sit unnoticed.** The
|
||
sentence described a door nobody walked: `reduce_full` had no production caller
|
||
anywhere in the workspace, so nothing exercised its `Command::Model` arm and
|
||
nothing could notice it skipped the check. A door advertised by documentation and
|
||
used only by its own tests is exactly the shape that hides a defect until the next
|
||
host arrives — which is the general form, not an anecdote about one bug.
|
||
|
||
So `reduce_full` is **gone**, and the argument for deleting rather than wiring it
|
||
up is in its own last doc comment: *"Per-client grid updates need a viewport and
|
||
come from `Sessions::project_all`."* It returned half of what a host needs. The
|
||
browser worker was the natural candidate caller and was being built at the time;
|
||
it reaches the model through `Host::handle` → `Sessions::apply_checked`, because
|
||
that is the half that has the viewport. A function that hands a host half its
|
||
answer is not a host API, and the test helper that was its only caller now spells
|
||
the two lines it was.
|
||
|
||
What remains, and what each is for:
|
||
|
||
- `apply_command` takes a `Checked` — a `Command` the authoritative state has
|
||
answered for — and applies its **document half**. That vocabulary is derived
|
||
from the effects: the view slice from the `ViewOnly` ones
|
||
(`decision(improvise-cqi, command_set_derived_not_invented)`) and the document
|
||
from the `EffectKind::Model` ones (improvise-kmgs). Its production callers are
|
||
`Sessions::apply_switch` and `apply_model`, which pass only `SwitchView` and
|
||
`Command::Model`.
|
||
- `reduce_key` takes a keypress and runs it through the keymap, registry and
|
||
effect pipeline. A keypress names an *intent* whose meaning depends on the
|
||
`CmdContext` (where the cursor is, what mode is current); a `Command` names
|
||
the mutation that intent resolved to. That is why the key path survives the
|
||
vocabulary rather than being replaced by it. It returns the document-scoped
|
||
projections to send onward; the grid is per-viewport and comes from
|
||
`Sessions::project_all` instead.
|
||
|
||
What is *not* two is the mutation: `model_effects` maps a `ModelCommand` back to
|
||
the effect it was derived from, and both paths run it through
|
||
`App::apply_effects`. Effects remain the only place the `App` is written.
|
||
|
||
`apply_command`'s remaining arms **no longer fold**. They used to route a wire
|
||
view command into `app.view_state` — the *authoritative* slice — which nothing in
|
||
production did and nothing should: `Sessions::apply_checked` folds it into that
|
||
client's mirror, because the model is shared and the UI is not (principle 5,
|
||
gotcha 36). A client's `SetStatus` moving the host's status line is not a feature
|
||
the module was missing a caller for; it is behaviour that was safe only because
|
||
the arm was dead. `a_view_slice_command_does_not_move_the_authoritative_view_state`
|
||
now asserts the inverse of what the old test asserted.
|
||
|
||
**A wire command arrives here through `Checked`, and nowhere else**
|
||
(improvise-ondj, ADR 001). The basis check is a property of the session door
|
||
rather than of the mutation path — a host holding the `App` *is* the truth and
|
||
not a claimant — but "the mutation path does not check" was implemented as a
|
||
second door rather than as an exemption: both functions used to take a
|
||
`&Command` and their `Command::Model { cmd, .. }` arm discarded the basis with
|
||
`..`, which is the syntax for ignoring a field and costs no compile error. So
|
||
the answer is not a guard but a type: the only way to obtain a `Checked` over a
|
||
`Command::Model` is `Checked::against`, which runs `stale_basis` against the
|
||
authoritative `App`. A host that wants to apply a client's batch must face the
|
||
`Rejection`, and `Sessions::apply_checked` is the door a host goes through.
|
||
|
||
`Host::commands` answers the whole frame first with `CheckedBatch::against`, so
|
||
`Sessions::apply` — which answers one command and *then* dispatches — is not a
|
||
door a host may use: checking per command is the lost update improvise-955n
|
||
fixed. It is now `#[cfg(test)]` (improvise-s17o), which makes "no production
|
||
caller" a fact the compiler keeps rather than a note verified at one commit, and
|
||
leaves `apply_checked` as the one door.
|
||
|
||
### `Checked` — a wire `Command` the authoritative state has answered for
|
||
|
||
The one thing it makes unrepresentable: a `Command::Model` applied without its
|
||
basis being checked (improvise-ondj). The inner field is private and
|
||
`Checked::against` is the only constructor, so there is no spelling of "apply
|
||
this batch and ignore what it claims" — the `..` that used to discard the basis
|
||
is now a `Rejection` the caller must handle.
|
||
|
||
It is **not** a claim about which `App` was asked: a host with two documents
|
||
could check against one and apply to the other. That is not a hole this closes,
|
||
because a `Sessions` is already bound to one document's generation sequence
|
||
(ADR 001) and a `Registry` hands out one pair at a time.
|
||
|
||
#### Why the lifetime does not close it (improvise-7v62)
|
||
|
||
The obvious answer — make the lifetime the `&App`'s, so the borrow checker
|
||
forbids checking one document and applying to another — was tried, and it is not
|
||
merely inconvenient: it is **contradictory with what the type is for**. A
|
||
`Checked` is a proof discharged *before* a mutation, and its whole job is to be
|
||
carried across that mutation; a borrow of the `App` is a claim that must stay
|
||
live *during* it. The two cannot both hold. Tying them makes every call site an
|
||
`E0502`, starting with the canonical one:
|
||
|
||
```text
|
||
error[E0502]: cannot borrow `*app` as mutable because it is also borrowed as immutable
|
||
match Checked::against(app, cmd) {
|
||
--- immutable borrow occurs here
|
||
Ok(checked) => self.apply_checked(app, id, checked),
|
||
^^^^^ mutable borrow occurs here
|
||
```
|
||
|
||
A generative brand would express it without borrowing, but that needs a
|
||
closure-scoped API and would distort every host. The remaining option is
|
||
carrying a document identity, and `DocumentId` lives in the *server's* registry:
|
||
an `App` does not have one and does not want one, precisely because the unit of
|
||
authority is one document per `App`. So this is a **deliberate won't-fix**, not
|
||
an oversight. What keeps it unreachable is stated above and is load-bearing —
|
||
improvise-j31x made this a multi-document server on purpose, and ADR 001 never
|
||
evicts a document so that a reconnecting client cannot have a stale basis
|
||
accepted as current.
|
||
|
||
### `CheckedBatch` — a whole frame, answered before any of it is applied
|
||
|
||
(improvise-955n / improvise-08wb, ADR 001.)
|
||
|
||
`Checked` alone was not enough, and the gap was exactly one level up from the
|
||
one improvise-ondj closed. A `Checked` answers for *one* command against the
|
||
`App` **as it stands**; a frame is a sequence of them, applied in order, so a
|
||
frame whose second command was refused had already applied its first. ADR 001
|
||
says a stale batch is never applied, *not even partially*, and that was true of
|
||
a single command and false of a batch.
|
||
|
||
#### One basis, checked once, and then the chain runs
|
||
|
||
The contract, as the user stated it: *"a sequence of commands sent to the server
|
||
should assume that it's played in order from start to end and that each command
|
||
sees the state after the previous command runs"*, sent as *"a single transaction
|
||
from a given base"*. Four clauses:
|
||
|
||
1. A frame states **one basis**, describing the base the client composed against.
|
||
2. That basis is checked **once**, against that base, before anything is applied.
|
||
3. Commands are applied **in order**, and command N sees the state command N-1
|
||
left. Data dependence between them is expected, not an error.
|
||
4. **All or nothing.** If any command fails, the frame leaves the document
|
||
exactly as the base described.
|
||
|
||
Clauses 1–3 are implemented. **Clause 4 is not** — see the hole below and the
|
||
design note on PR #26.
|
||
|
||
#### Two of the three tiers hoist, and the third does not
|
||
|
||
Clause 2 hoists the check, but **not every tier is a frame-level fact**, and a
|
||
first version hoisted all three. That was a lost update: `document_generation`
|
||
and `layout_version` describe the *document*, so answering them once for the
|
||
frame is exactly right — but the record tier asks "is the client's picture of
|
||
**this record** current", and which records are at stake is a property of the
|
||
*command*. That is why `records_named` is an exhaustive match over
|
||
`ModelCommand` and not something a frame could summarise (gotcha 43).
|
||
|
||
Skipping it for later commands let `[SetCell(North), SetCell(South)]` overwrite a
|
||
South that somebody else had moved, silently, with the frame reported accepted —
|
||
the precise loss ADR 001's record tier exists to prevent, and invisible to the
|
||
other two tiers because a `SetCell` is `CellData` and moves neither of them. So
|
||
**every** mutation answers for its own records, against the **frame's** basis:
|
||
the one the first basis-carrying command stated, not each command's own field,
|
||
so a later command cannot state a fresher claim than the frame was composed
|
||
under.
|
||
|
||
##### Except the records the frame is creating
|
||
|
||
The argument that motivated clause 3 is true, and it is true of a narrower thing
|
||
than it was first applied to. 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 `[AddItem(East), SetCell(East)]` would be
|
||
refused by a naive per-command check however it is ordered. But that argument was
|
||
then stretched to records the frame merely **writes**, and those are the ones the
|
||
tier is for.
|
||
|
||
So the exemption is stated over the *property* rather than over a list of
|
||
commands: a record that **neither resolves in the document nor is claimed by the
|
||
basis** is one the frame is about to create, and is not answered for. Every other
|
||
combination still is — including a record that does not resolve but *is* claimed,
|
||
which is a client believing in something that has been removed, and is
|
||
`RecordGone` as before. See `will_be_created_by_the_frame`.
|
||
|
||
What the base means is therefore: **what the client saw**. The transaction
|
||
promises the whole chain lands on top of it or none of it does. The chain's
|
||
internal coherence is the transaction's job, not the basis check's.
|
||
|
||
#### The hole clause 4 leaves, stated rather than hidden
|
||
|
||
`App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a
|
||
rollback), so a frame whose second mutation fails leaves the first applied.
|
||
`[RemoveItem("North"), SetCell(the North record)]` is the example: the removal
|
||
lands, the write then resolves to nothing and `model_effects` returns an
|
||
**empty** batch, so the frame is reported accepted having applied half of
|
||
itself. Both halves of that are open — the partial application and the silence.
|
||
|
||
The create exemption has a residue of the same kind and it is the same hole: a
|
||
later command naming a record that is neither present nor claimed is *presumed*
|
||
to be one the frame creates, so if the frame does not in fact create it, the
|
||
write resolves to nothing and silently does not happen. That is `model_effects`'
|
||
empty batch again, not a second defect, and it closes when clause 4 makes a
|
||
mid-chain failure reportable.
|
||
|
||
Both are **unreachable from any client in this workspace**: the only producer of
|
||
a multi-command frame is `improvise-wasm-client::keys::commit`, which emits one
|
||
mutation among view commands. **Reachability is a reason not to rush a fix and
|
||
not a reason not to notice one** — the lost update above had the same
|
||
reachability argument as this hole and none of its documentation, which is how it
|
||
shipped. That is what makes it safe to document rather than rush, and it is why
|
||
an earlier "at most one document mutation per frame" rule — which closed the hole
|
||
by forbidding the capability — was rejected.
|
||
|
||
#### Two things the rule that was rejected got wrong, worth keeping
|
||
|
||
**Arity is not a property of a variant.** A future `Command::RunScript` is one
|
||
variant performing an unbounded number of mutations, so no count over the
|
||
vocabulary can be right: a classifier can honestly answer *whether* a command
|
||
changes the document, never *how many times*. A `mutates_the_document` predicate
|
||
written for the rejected rule is deleted with it rather than kept unused — but it
|
||
was made an **exhaustive match** on review, because a `matches!` answers `false`
|
||
for a future variant in silence, and clause 4 will need that classifier back.
|
||
Write it exhaustive when it returns.
|
||
|
||
**The transaction boundary is not the frame.** Because one command may already be
|
||
a chain, clause 4 is a property the system needs *inside* a command as much as
|
||
across a frame of them, which is why the design note puts the mechanism at
|
||
`App::apply_effects` rather than at the batch.
|
||
|
||
### `CheckedBatch::against`
|
||
|
||
Answer for every command in a frame, against the state the frame was composed
|
||
against — which is the state *before* any of them runs. The refusal is the
|
||
frame's answer and nothing is applied. It is a `Result` rather than a per-command
|
||
verdict for the same reason `Checked` is a type rather than a guard: the only
|
||
enforceable version of "answer before applying" is not being able to reach the
|
||
commands without having done so.
|
||
|
||
**The two document tiers are answered once**, on the first command that carries a
|
||
basis — clause 2 — and every later command is admitted without re-asking them,
|
||
which is clause 3. **The record tier is asked of every mutation**, against that
|
||
same frame basis, because it is not a frame-level fact. A frame with no mutation
|
||
answers trivially.
|
||
|
||
The `Basis` on a *later* `Command::Model` is ignored rather than compared, and
|
||
that is an honest interim rather than the end state: clause 1 says the **frame**
|
||
states one basis, but the wire still carries a `Basis` inside each
|
||
`Command::Model` — right when one command was one transaction, and now one field
|
||
per command describing something that belongs to the frame. Moving it to
|
||
`ClientFrame::Commands` would make "one basis per frame"
|
||
unrepresentable-otherwise instead of a convention, and that protocol change
|
||
belongs with clause 4.
|
||
|
||
Inside the loop, `frame_basis` is the one its first mutation stated; later
|
||
commands' own `Basis` fields are deliberately not read, for the reason above.
|
||
|
||
### `Checked::against`
|
||
|
||
Total over the vocabulary, and only `Command::Model` can be refused: everything
|
||
else is either this client's own view slice or a `SwitchView`, which carries no
|
||
basis and cannot corrupt — the workbook simply refuses a view it does not have
|
||
(ADR 001 names that second hole and leaves it open deliberately).
|
||
|
||
Saving is not in this vocabulary and deliberately so: it is `ClientFrame::Save`,
|
||
answered by the host, because it mutates no shared state and has no basis to
|
||
answer for (improvise-368h).
|
||
|
||
There is no argument that suppresses the answer. ADR 001's force is a fresh batch
|
||
under a fresh basis, composed by the client; it is not a way past this
|
||
constructor.
|
||
|
||
### `Checked::within_a_frame`
|
||
|
||
A command in a frame whose **document** tiers have already been answered (clause
|
||
2), answering only for its own records. The two document tiers are not re-asked,
|
||
which is clause 3: they describe a document this command is not running against,
|
||
since its predecessors have moved it. The record tier *is* asked, because it is a
|
||
fact about the command rather than the frame.
|
||
|
||
Private, and that is what keeps it from being a bypass: from outside the module
|
||
the only doors are still `Checked::against` and `CheckedBatch::against`, and the
|
||
only caller is the loop that has just answered the frame's basis.
|
||
|
||
### `Checked::command`
|
||
|
||
Hands back a `&'a Command` rather than one borrowed from `&self`, so a caller can
|
||
dispatch on the variant and still move the proof into the function it dispatched
|
||
to. Crate-private: a proof that could be unwrapped from outside would be a proof
|
||
nobody had to obtain.
|
||
|
||
### `apply_command`
|
||
|
||
`Command::SwitchView` does not go through `reduce_view` alone, but not for the
|
||
reason it once did. Since improvise-jytm the document's copy is the *default* — what
|
||
a fresh client opens — and the client's slice is authoritative, so a switch moves no
|
||
document state at all. What it still has that `reduce_view` has not is the **view
|
||
history**: `effect::SwitchView` pushes the frame `effect::ViewBack` pops, and that
|
||
stack is this client's. Everything else is pure view slice.
|
||
|
||
The `SwitchView` arm specifically: the mutation is `effect::SwitchView`'s, not a
|
||
second copy of it. The effect records the view history the key path leaves
|
||
behind, and `effect::ViewBack` pops exactly that stack. Switching by
|
||
calling `App::switch_to_view` here instead left the stack empty, so a wire client
|
||
could not go back where a keyboard one could — see
|
||
`a_wire_view_switch_records_history_the_way_a_key_switch_does`. `apply_effects`
|
||
rebuilds the layout.
|
||
|
||
The `Dirty` that arm returns is **not** `Dirty::of` of anything, and since
|
||
improvise-jytm that is the point rather than a narrowing. `effect::SwitchView` is
|
||
`ViewOnly`, so its `Dirty` is `NOTHING`; what this arm returns is what the *host's
|
||
own* widgets must repaint, which is a different question from what the document
|
||
changed. The view list is left out for the old reason (improvise-pca): its names and
|
||
axes belong to the document, and which view is active is not in `ViewListCache` at
|
||
all. A connected client never reaches this arm — `Sessions::apply_switch` moves that
|
||
client's mirror and `ClientSession::project` observes the move.
|
||
|
||
An inbound `Command::Project` is ignored. Projections travel server→client; a
|
||
client that sends one is describing state it does not own.
|
||
|
||
The `Command::Model` arm needs no basis check of its own: the basis was answered
|
||
for by `Checked::against`, and there is no other way to have got here with that
|
||
variant.
|
||
|
||
### `model_effects`
|
||
|
||
The other half of the derivation: a `ModelCommand` back to the effect it was
|
||
derived from (improvise-kmgs). This is the seam that keeps a command from
|
||
becoming a second way to mutate. A `ModelCommand` names a mutation; it does not
|
||
perform one. The effect it names is the same value the keymap path builds, and it
|
||
is run through the same `App::apply_effects`, so both doors go through the one
|
||
mutation site and both get their `Dirty` from the effect's own `EffectKind`
|
||
rather than from a second classification that could drift.
|
||
|
||
The match is exhaustive on purpose: a new `ModelCommand` variant fails the build
|
||
here until it names an effect, exactly as a new effect fails the build until it
|
||
classifies itself (gotcha 3).
|
||
|
||
It takes the `App` because two variants name their operand by **surrogate**
|
||
rather than by coordinates (ADR 002), and resolving one is a lookup against the
|
||
authoritative state. Resolution failing is not an error path bolted on: it is one
|
||
of the two arms of staleness — the record is gone, a category or item it named
|
||
was removed — and the answer is an empty batch, which applies nothing.
|
||
`Sessions::apply` reaches that same conclusion first and typed, as
|
||
`Rejection::RecordGone`; this stays as the floor under the key path and under any
|
||
host that bypasses the session door.
|
||
|
||
### `records_named` — which records a `ModelCommand` writes
|
||
|
||
Decided by the **server** (ADR 001, improvise-fkfg). This is deliberately not
|
||
"whichever records the client's basis mentions". A client that simply left a
|
||
record out of its claim would otherwise have a **force by omission** — the record
|
||
tier skipped, with no flag to remove and nothing in the message saying so — and
|
||
force is supposed to be a re-composition against the current basis, never a
|
||
bypass of the check. The server decides what is checked; the client only supplies
|
||
what it saw.
|
||
|
||
The match is exhaustive, and the long list of "names no record" is the point
|
||
rather than noise: a new variant that writes a record fails the build here until
|
||
it says so. That matters because the failure mode is silent — an unlisted
|
||
record-writing variant would simply never be checked against its own tier, and
|
||
the suite would stay green.
|
||
|
||
Only `CellData` variants that carry a `RecordCell` name a record. The item and
|
||
formula commands are `CellData` too but name no *record*: they move the axes a
|
||
record's coordinates are drawn from, which is what the layout tier is for.
|
||
|
||
### `stale_basis`
|
||
|
||
`None` means apply it. The three tiers are checked in order of how much they
|
||
invalidate, so the answer a client is given names the largest thing that moved
|
||
rather than an arbitrary one: a document swap is not usefully reported as "a
|
||
record moved".
|
||
|
||
The record tier separates **structural** from **data** staleness, and keeping
|
||
them apart is the whole reason ADR 002 put a maintained `RecordHash` index in
|
||
`DataStore` rather than regenerating one from the layout. A record that does not
|
||
resolve is *gone* — there is nothing to write to and nothing coherent to force,
|
||
because force never creates structure. A record that resolves at a different
|
||
version merely *changed*, and force there is one keystroke.
|
||
|
||
Nothing in it mutates, and there is no argument that makes it return `None`
|
||
regardless: a stale batch is never applied, and ADR 001's force is a fresh batch
|
||
under a fresh basis rather than a way past this function.
|
||
|
||
### `stale_records_in_a_frame`
|
||
|
||
The record tier for a command inside a frame whose document tiers are already
|
||
answered. It reuses `stale_record` unchanged — one home for what each of the four
|
||
combinations means — and differs only in *which* records it asks about: the ones
|
||
the frame is about to create are skipped.
|
||
|
||
### `will_be_created_by_the_frame`
|
||
|
||
**A property, not a list of commands.** The document does not have it and the
|
||
basis claims nothing about it — which is precisely what "I am creating this"
|
||
looks like from here, because `BasisCache::claim` can only state a counter for a
|
||
record the client has been told about.
|
||
|
||
The near miss is the other half of that conjunction: a record that does not
|
||
resolve but *is* claimed is a client believing in something that has been
|
||
removed, and stays `Rejection::RecordGone`. Only the pair exempts.
|
||
|
||
It is asked **only inside a frame**, never of a lone command: a first command has
|
||
no predecessor that could have created anything, so `Checked::against` still
|
||
refuses a write to a record that is not there.
|
||
|
||
### `caught_up`
|
||
|
||
The command a client that is **fully caught up** would compose. It reads the
|
||
tiers straight off the `App`, which looks like forging and is not: it takes
|
||
`&App`, so only the side that *is* the authoritative state can call it, and for
|
||
that side "the current basis" is not a claim to be checked but a fact. A remote
|
||
client has no `App` and no other constructor — `BasisCache::claim` withholds
|
||
until the server has spoken — so nothing reachable from a wire can get here.
|
||
|
||
It exists for two callers: a host that resolves a command locally and wants to
|
||
push it through the same door a client's would take, and the tests whose subject
|
||
is not staleness. The tests whose subject *is* staleness build the `Basis` by
|
||
hand, so neither kind can pass by accident through the other's convenience.
|
||
|
||
### `reduce_key`
|
||
|
||
Resolve a keypress through the keymap, run the commands, apply the effects, and
|
||
report what to send onward. The returned commands are the document-scoped
|
||
projections; per-client grid updates need a session's viewport and come from
|
||
`Sessions::project_all`, driven by the `Dirty` that `apply_command` returns.
|
||
|
||
It used to have a wire-command twin, `reduce_full`, described right here as "the
|
||
signature the three hosts share". None of them shared it; the module section
|
||
above says why it is gone (improvise-s17o).
|
||
|
||
---
|
||
|
||
## `ui/effect.rs` — the only mutation site
|
||
|
||
### `EffectKind` has no default, deliberately (improvise-mae, gotcha 3)
|
||
|
||
`EffectKind::ViewOnly` changes only the acting client's own view — cursor,
|
||
scroll, mode, panels, minibuffer text, drill staging, status line — and is never
|
||
projected. `EffectKind::Model(ModelChange)` changes something every client
|
||
renders.
|
||
|
||
Cursor and scroll live in the shared `View` (they are persisted in `.improv`),
|
||
but they are a per-client concern: one client scrolling must not move another's
|
||
viewport. See improvise-8uz.
|
||
|
||
`Effect::kind` deliberately has **no default**: a new effect that forgot to
|
||
classify itself would silently never reach connected clients, so the compiler
|
||
asks. The three traps a classifier walks into are gotcha 3's and are not
|
||
restated here.
|
||
|
||
### `Effect::apply` stays on `&mut App` — declined, with counts (improvise-drg)
|
||
|
||
improvise-drg proposed narrowing each `apply` to the slice it touches: `&mut
|
||
ModelState` or `&mut ViewState` where an effect only needs one, `&mut App` for
|
||
the rest. The win it claimed is type-level documentation and finer-grained
|
||
tests, explicitly not line count. Declined, on four grounds.
|
||
|
||
**The narrow set is shrinking, for an architectural reason.** Counted over all
|
||
**57** effects — 49 `impl Effect` blocks plus the 8 `simple_effect!`
|
||
invocations, which also expand to one — **16** touch only `view_state`, **9**
|
||
only `model_state`, and **32** cross-cut. At the wave's base commit it was
|
||
16 / 16 / 25, so the narrow set went from **32 to 25**, and the seven movers are
|
||
`SetAxis`, `HideItem`, `ShowItem`, `ToggleGroup`, `TogglePruneEmpty`,
|
||
`RemoveItem` and `RemoveCategory` — every one of them an effect improvise-9mo9
|
||
gave a `reset_cursor`.
|
||
|
||
**These are the corrected figures, and the first ones were wrong twice.** They
|
||
said "25 to 17 of 49": they omitted the `simple_effect!` invocations, all of
|
||
which are view-only, and they misclassified `AddItem` at the base commit because
|
||
the extraction regex could not see `app` followed by a newline and
|
||
`.model_state`. That is `doctrine(derive_then_verify)` catching the author of
|
||
this paragraph with the *exact* failure it was written about — the kb entry names
|
||
multi-line method chains and three effects they had already mis-sorted. **The
|
||
correction weakens this bullet and is recorded rather than quietly restated**: a
|
||
narrow set of 25 of 57 is 44%, not the 35% first claimed, and the fall is 22%
|
||
rather than 32%. The direction survives; the headline was overstated.
|
||
|
||
The reason the direction is what matters: a grid reshape lives in
|
||
`model_state.workbook` and the cursor it invalidates lives in `ViewState`, so
|
||
the effects that matter most are exactly the ones that reconcile the two. Gotcha
|
||
3 already records the same asymmetry from the other side — the *View* lives
|
||
inside `model_state.workbook`, so touching `model_state` does not mean being a
|
||
model change. The `App` is where the slices meet, and narrowing bets against the
|
||
direction the code moves in.
|
||
|
||
**The type-level documentation exists and is checked.** `Effect::kind()` has no
|
||
default, so the compiler demands the classification (gotcha 3), and
|
||
`apply_effects` reads that one answer for the `Dirty`, and for the `CellData` and
|
||
`WholeWorkbook` half of `model_state.dirty` (gotchas 40, 45; the shape tiers are
|
||
observed rather than declared — see `apply_effects` below). A narrowed signature would be a
|
||
*second* statement of what an effect touches, free to disagree with the first.
|
||
Two answers to one question is the defect this repo names, not the cure.
|
||
|
||
**Every mechanism the ticket offered weakens a guard.** Its (A), three traits,
|
||
needs `Vec<Box<dyn Effect>>` to become three queues or an enum wrapper: three
|
||
queues destroy batch order, which gotcha 21's abort, gotcha 23's stale layout
|
||
and gotcha 43's frame contract all lean on, and an enum wrapper is a fourth
|
||
classification beside `EffectKind`. Its (C), default methods falling through to
|
||
`apply(&mut App)`, gives `apply` a **default** — the precise thing `kind()` does
|
||
without, and for the precise reason: an effect that overrode `apply_view` but
|
||
needed the model would compile and write the wrong slice silently, and one that
|
||
overrode neither would compile as a no-op. Its (B) rebinds inside the body and
|
||
does not meet the ticket's own criterion.
|
||
|
||
**Finer-grained tests are not blocked today.** effect.rs's 64 tests already call
|
||
`Effect.apply(&mut app)` against an `App::new(wb, None)` built in a line;
|
||
narrowing the signature would not shrink that fixture.
|
||
|
||
The 17 that could narrow may still be worth *reading* as narrow, which is
|
||
approach (B) — rebinding the slice at the top of the body. That is a style
|
||
choice inside a function, not a trait change, and it is not being mandated.
|
||
|
||
### `simple_effect!` — the kind argument is mandatory (improvise-8fy)
|
||
|
||
Declare a setter effect: a one-field newtype whose `apply` writes that field into
|
||
one place on the `App`.
|
||
|
||
The kind is a **mandatory** argument, even though every call site so far answers
|
||
`ViewOnly`. Hard-coding it reintroduced exactly the default that `Effect::kind`
|
||
deliberately does without (gotcha 3): `$place` is an unconstrained expression, so
|
||
|
||
```
|
||
simple_effect!(X(String) => |app| app.model_state.workbook.active_view_mut().number_format, …)
|
||
```
|
||
|
||
is a textbook `LayoutGeometry` write that the macro would have reported as
|
||
`ViewOnly`, silently, with the whole suite green. Writing the answer per call
|
||
site is the same reasoning that gave `kind()` no default.
|
||
|
||
Within that, the macro is still the single hook for anything these setters should
|
||
all do — today none of them marks the document dirty or rebuilds the layout, and
|
||
the macro makes that one decision instead of fifteen silent ones.
|
||
|
||
`Clone::clone` rather than `.clone()`: the field is `usize` or `bool` as often as
|
||
it is a `String`, and the call form keeps `clippy::clone_on_copy` off the
|
||
generated code.
|
||
|
||
### `AddCategory` aborts the batch on refusal
|
||
|
||
A refusal aborts the batch, for the same reason a failed save does (gotcha 21):
|
||
what follows assumes the category exists. From the `+cat` column (improvise-2zq)
|
||
the rest of the batch would advance the cursor and re-open an editor on a column
|
||
that was never created — and re-opening the minibuffer is also what would *hide*
|
||
this message, since `draw_bottom_bar` renders the minibuffer instead of the
|
||
status line.
|
||
|
||
`MAX_CATEGORIES` is a reachable refusal on that path rather than a theoretical
|
||
one, and the limit is checked in exactly one place — `Model::add_category` — so
|
||
nothing in the effect restates it.
|
||
|
||
### `AddFormula` and non-`_Measure` targets
|
||
|
||
For non-`_Measure` targets the item is added to the category so it appears in the
|
||
grid. `_Measure` targets are dynamically included via
|
||
`Model::measure_item_names()` (gotcha 7).
|
||
|
||
### `RebuildLayout` (improvise-ehz)
|
||
|
||
`apply_effects` rebuilds the layout once after the whole batch, so any effect that
|
||
must *read* the layout after an earlier effect changed it needs a `RebuildLayout`
|
||
in front of it. That is gotcha 23; the command decides the order.
|
||
|
||
### `EnterEditAtCursor` — the mode is the caller's, never `app.view_state.mode`
|
||
|
||
Re-enter edit mode by reading the cell value at the current cursor; used after
|
||
commit+advance to continue data entry. It reads the *current* layout, so emit
|
||
`RebuildLayout` first when an earlier effect in the batch moved the cursor or
|
||
changed the grid.
|
||
|
||
`target_mode` is supplied by the caller (a keymap binding via
|
||
`EnterEditAtCursorCmd`, or `CommitAndAdvance` from its own `edit_mode` field).
|
||
The effect itself never inspects `app.view_state.mode`: the mode is decided
|
||
statically by whoever invoked it. An earlier version branched on
|
||
`app.view_state.mode.is_records()` and got it wrong mid-batch.
|
||
|
||
### Which effects reset the cursor, and why it is decided one at a time
|
||
|
||
Swapping the axes renumbers every row and column, so the old indices name
|
||
different cells. That reset used to live inside `View::transpose_axes`; it moved
|
||
here with the cursor (improvise-8uz, gotcha 24).
|
||
|
||
Nothing declares which effects renumber the grid, deliberately — so for a long
|
||
while only `TransposeAxes` and `CycleAxis` reset, while eight siblings that
|
||
renumber just as thoroughly did not (improvise-9mo9). `SetAxis` was the sharp
|
||
case: `CycleAxis` *is* `SetAxis` to `Axis::next()`, so one of a pair that differ
|
||
by an argument reset and the other did not. The gap was invisible in **both**
|
||
directions, because `reset_cursor` is also the only thing that bumps
|
||
`cursor_epoch`, which is the only thing `Sessions::adopt_cursor_epoch` watches:
|
||
a forgotten reset leaves the host's own cursor stale *and* tells no client.
|
||
|
||
Ten effects now reset, and the test that holds them there
|
||
(`every_reshaping_model_command_invalidates_every_cursor`) asserts of each case
|
||
that the grid's `row_items`/`col_items` really changed before asserting anything
|
||
about a cursor, so a case that has stopped reshaping fails rather than passing
|
||
vacuously. The per-effect arguments:
|
||
|
||
- `SetAxis` rebuilds the cross product out of different categories, and a `cats`
|
||
change re-identifies every position, not merely the ones whose labels differ
|
||
(gotcha 44). `HideItem`/`ShowItem` and `ToggleGroup` each add or drop a run in
|
||
the middle of an axis, so everything after it shifts. `TogglePruneEmpty` drops
|
||
or restores every empty row and column at once.
|
||
- `AddCategory`/`RemoveCategory` are `cats` changes in **every** view, since
|
||
`Workbook::add_category` calls `View::on_category_added` on all of them and
|
||
`on_category_added` lands a regular category on Row or Column.
|
||
- `AddItem` and `AddItemInGroup` reset **only on success**, because a refused
|
||
add reshapes nothing and the epoch is document-wide: the first version of this
|
||
change reset after the error branch, so a typo'd category name sent every
|
||
client a `ResetCursor` for a change that never happened.
|
||
- `AddItem`/`AddItemInGroup`/`RemoveItem` are the ones worth spelling out,
|
||
because "an append cannot move an existing position" is true only of the
|
||
**outermost** category on an axis. `expand_category` runs inside the cross
|
||
product, so an item appended to an inner category repeats once per outer
|
||
block: adding a third `Product` inserts a row into the `North` block and
|
||
pushes the whole `South` block down. `RemoveItem` is a `shift_remove` and
|
||
moves positions on any level.
|
||
- **The condition is "did this change anything", and it took three passes to get
|
||
right.** A refused add reshapes nothing, so the first version — which reset
|
||
after the error branch — sent every client a `ResetCursor` for a typo'd
|
||
category name. The second version gated on `Ok`, which is *still wrong* for
|
||
six of them, because accepted and changed-nothing are different: `SetAxis` to
|
||
the axis a category is already on, `HideItem` on an already-hidden item,
|
||
`ShowItem` on a never-hidden one, `AddItem` of an existing item (`add_item`
|
||
dedupes and returns the existing id), and `RemoveItem`/`RemoveCategory` of
|
||
something absent (both infallible no-ops). Each now asks the state *before*
|
||
mutating. The one-keystroke case that made this urgent needs no second client:
|
||
TileSelect binds `r` to `set-axis-for-tile row`, which emits `SetAxis` with no
|
||
comparison against the current axis, so `r` on a Row category sent the user's
|
||
own cursor to the origin.
|
||
- **The predicate must ask the same question the grid does, and twice it did
|
||
not.** Review found six commands the document accepts where the guards still
|
||
reset. Four were the guard being absent or asking the wrong thing:
|
||
`SetAxis` on a category the view does not register (`set_axis` no-ops, but
|
||
`try_axis_of` returns `None`, which compares unequal to `Some(axis)`);
|
||
`HideItem`/`ShowItem` naming something the grid never drew (`hide_item`
|
||
records any string); and `ToggleGroup` naming a group that does not exist,
|
||
which never got a guard at all because it merged **cleanly** through the #48
|
||
merge and so nobody looked at it. The fifth is the one worth remembering:
|
||
`AddItem { _Measure, <a formula target> }` passed a `has_item` that read
|
||
`category.items`, while `_Measure`'s effective items are explicit ∪ formula
|
||
targets (gotcha 7) — so the guard said "new" about a column that was already
|
||
drawn. That is fixed by **observing** rather than predicting: the three item
|
||
effects now compare `effective_item_names` across the mutation, which is
|
||
correct in both directions at once and needs no separate rule for dedupe, for
|
||
formula targets, or for an unknown category. The sixth, `TogglePruneEmpty` on
|
||
a grid with nothing empty, is **not fixed** — guarding it means computing the
|
||
pruned layout — and it is one keystroke in ordinary use, which is why it is
|
||
filed rather than patched.
|
||
- **This is the cost of a per-effect table**, and it is the argument for
|
||
improvise-ni6k. An `EffectKind` cannot say whether anything moved, so six
|
||
effects each had to learn to answer separately — six chances to get it wrong,
|
||
three of which were taken. Observing the grid frame across a batch answers it
|
||
once, and would close improvise-7kvw for free. It is not done here because it
|
||
belongs in `App::apply_effects`, which three other branches were editing.
|
||
|
||
What does **not** reset, and why:
|
||
|
||
- `SetNumberFormat` changes how a number is drawn and never which cell a
|
||
position names. It is the negative arm, and without it "reset on every model
|
||
command" would pass the positive test.
|
||
- `CreateView`, `DeleteView`, `SwitchView`, `ViewBack` and `ViewForward` move
|
||
which view is active. `ViewState::cursors` is keyed by view name (gotcha 24),
|
||
so the cursor being read after the switch is a different one and the outgoing
|
||
view's entry stays valid for when the user comes back.
|
||
- `SetPageSelection` changes which records are visible, not which categories
|
||
frame them — **except** under `prune_empty`, where a page with different empty
|
||
rows really does renumber. That residual is improvise-7kvw rather than fixed here,
|
||
because `[`/`]` is navigation and sending the cursor home on every page flip
|
||
is a worse answer than the hole.
|
||
|
||
**Drilling is the case this ordering has to survive, and it is tested rather
|
||
than argued** (`drilling_resets_only_the_drill_views_cursor`). `DrillIntoCell`
|
||
and `ToggleRecordsMode` each issue several `SetAxis` effects, so each now resets
|
||
several times — but both push `SwitchView` *first*, and `reset_cursor` writes the
|
||
**active** view's cursor, which by then is the freshly created `_Drill`/`_Records`
|
||
one. The view drilled *from* keeps its entry in `ViewState::cursors`. What is
|
||
still only argued is the `cursor_epoch`: a drill bumps it once per `SetAxis`,
|
||
`Sessions::adopt_cursor_epoch` compares before and after per command so it
|
||
relays once rather than eight times, and `DrillIntoCell` is reachable only from
|
||
the key path today — `Host::handle` answers `ClientFrame::Key` with an empty vec
|
||
— so no client sees it at all. That changes when drill becomes addressable.
|
||
|
||
The one caller that depended on a *missing* reset is `CommitAndAdvance`'s
|
||
`inserts_a_column` branch: typing into the `+cat` column creates a category and
|
||
then deliberately stays put, so the cursor lands on the column it just created
|
||
(improvise-2zq). That worked because nothing moved the cursor. Now `AddCategory`
|
||
does, so the branch ends with an explicit `SetSelected` back to where it was —
|
||
which states the intent instead of inheriting it from an absence, and still sits
|
||
after `AddCategory`'s abort so a refused category leaves the batch unchanged.
|
||
|
||
### `SwitchView` — the history records a move that happened (improvise-swzk)
|
||
|
||
`switch_to_view` is guarded: it writes `active_view` only if the document has the
|
||
view. `SwitchView` used to push the leaving frame onto `view_back_stack` **before**
|
||
calling it, under the prediction `previous != self.0` — which answers *did the user
|
||
ask for a different name*, where the history means *did this client go somewhere*.
|
||
Those two questions differ on exactly one input, the name the document has not got,
|
||
and there `:view NoSuchView` buried a real frame under a frame naming the view the
|
||
user was already on. `<` then "went back" to where it already was, and the frame
|
||
behind it needed a second `<` nobody knew to press.
|
||
|
||
The wire path was never wrong here and is not the model for the fix: `reduce_view`
|
||
**cannot** check — the client holds no `Workbook` — so `apply_switch` relays the
|
||
exact inverse, `[ViewBack, ForgetView]` (improvise-6c6q, and see "A refused switch
|
||
is undone, not overwritten"). The host holds the document, so it does not need an
|
||
inverse; it needs to *look*. `apply` now clones `previous`, calls `switch_to_view`,
|
||
and pushes only if `active_view` actually differs afterwards.
|
||
|
||
That is `ReturnToNormalUnlessMoved`'s rule one effect over, and it is the same
|
||
answer `cursor_epoch` gives instead of matching on `CycleAxis` and
|
||
`document_generation` gives instead of matching on `LoadModel` (gotchas 24, 39, 45):
|
||
**observe the state that moved, never predict it from the request.** It also
|
||
subsumes the case the prediction was originally written for — switching to the view
|
||
you are already on is a `switch_to_view` that changes nothing — so the special case
|
||
is gone rather than joined by a second one. A guard spelled here as
|
||
`workbook.views.contains_key(&self.0)` would have been correct today and a second
|
||
copy of `switch_to_view`'s own condition, free to disagree with it the day that
|
||
condition grows a clause.
|
||
|
||
Not fixed, and reachable only by construction: `ViewBack` and `ViewForward` call
|
||
`switch_to_view` just as unguardedly, pushing the current view onto the opposite
|
||
stack before the switch they cannot be sure of. They are safe **because
|
||
`ViewState::forget_view` purges both stacks when a view is deleted**, so a popped
|
||
frame always names a live view — a guard living one level away from the hazard,
|
||
which is the shape that keeps returning here.
|
||
|
||
### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc)
|
||
|
||
Return to `Normal` unless something earlier in this batch already moved the mode
|
||
away from `from` — the mode the batch started in.
|
||
|
||
`ExecuteCommand` used to decide this up front by asking each effect a per-type
|
||
`changes_mode()`. That cannot describe `ViewBack`, `ViewForward` or
|
||
`StartImportWizard`, which swap the mode only on some runs: answering "no"
|
||
snapped a successful `:import` straight back out of the wizard, while answering
|
||
"yes" would have stranded a failed one in `CommandMode`. Reading the mode here,
|
||
after the batch has run, needs no prediction. That is gotcha 31.
|
||
|
||
### `App::normal_mode` — what `Normal` means on this grid (improvise-sihr)
|
||
|
||
`AppMode::Normal` and `AppMode::RecordsNormal` are the same mode over two
|
||
layouts, and every minibuffer mode opened from a records grid used to exit to
|
||
the first: the exit was a constant, spelled in `TextEntry::new(.., "normal")`
|
||
and again in each commit command, and nothing carried where the user came from.
|
||
Three arms — `:`, `Ctrl-E` and `Ctrl-W` — and the class predates the last of
|
||
them, which is why Copilot's finding against `SavePrompt` alone was declined on
|
||
PR #27.
|
||
|
||
**It is resolved once, at the end of the batch.** `App::apply_effects` calls
|
||
`settle_normal_mode` **after `rebuild_layout`**, and that is the only caller:
|
||
`Normal` and `RecordsNormal` are one mode over two layouts, so the mode a batch
|
||
ends in must match the grid it ends on. No effect resolves anything —
|
||
`ChangeMode` is an ordinary `simple_effect!` again, and `ReturnToNormalUnlessMoved`
|
||
and `CloseWizard` write plain `Normal`. The ticket proposed the opposite — an
|
||
exit mode beside `MinibufferConfig` — and it was declined for a reason beyond
|
||
doctrine: carried generically it also returns `CategoryAdd` to `CategoryPanel`,
|
||
which nobody asked for, and carried for three modes it is the one-arm shape the
|
||
ticket itself argues against.
|
||
|
||
**Resolving per-effect was tried first and was wrong, in a new direction.** Set
|
||
at the moment the mode is written, the resolution reads the batch's *opening*
|
||
layout (gotcha 23), so a batch that **crosses** — `:toggle-records-mode . enter-mode normal`
|
||
— settled against the grid it started on and left a records mode over a pivot
|
||
grid. One edit, after the rebuild, has no crossing case to get wrong, and it
|
||
also removes the per-edit coverage question three separate writers created.
|
||
|
||
**The guard is two-armed and the crossing test could only see one arm**
|
||
(improvise-4dpj). `settle_normal_mode` matches `Normal | RecordsNormal`, and
|
||
replacing that with a one-way `Normal` left the whole workspace green: the `:`
|
||
crossing 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`
|
||
either way. It discriminates "settles at all" and "settles against the ending
|
||
layout", and not "settles from **either** mode" — three questions that look
|
||
like one until an injection separates them. The `RecordsNormal` arm is live:
|
||
`SetAxis { "_Index", Axis::None }` applied from a records grid leaves
|
||
`is_records_mode()` false with the mode still `RecordsNormal`.
|
||
|
||
**And records-ness is auto-detected from the axes (gotcha 1), so a view switch
|
||
crosses it too.** `SwitchViewAtCursor` emits `[SwitchView(target),
|
||
ChangeMode(Normal)]`, which is a genuinely different path from the toggle the
|
||
`:` test drives — nothing in it says "records", the crossing falls out of the
|
||
target view's axis map. Moving the settle back before `rebuild_layout` is the
|
||
injection for both, and it fails each by name.
|
||
|
||
**This fixes the TUI and not the browser, and that is LIVE (improvise-e5pf).**
|
||
`Command::ChangeMode(AppMode::Normal)` is on the wire and `reduce.rs` folds it
|
||
as `view.mode = mode.clone()` with no resolution, so the browser front end still
|
||
has the improvise-sihr bug. The live browser key resolver is
|
||
`improvise_protocol::resolve` — re-exported as `improvise_wasm_client::resolve`
|
||
— and it emits that command in five places. **An earlier version of this
|
||
paragraph said the gap was latent because "`keys.rs` emits no `ChangeMode`",
|
||
which is true and irrelevant: `keys.rs` holds only `BrowserKey`.** Nothing
|
||
checks a claim in `context/`, which is exactly why one must not be written from
|
||
a grep of the wrong file (design-principles §6).
|
||
|
||
It compounds, and that is the part worth knowing: once the client's mode reads
|
||
plain `Normal`, its only records-ness fact — `state.view.mode.is_records()` —
|
||
is false, so a later commit over a records grid takes the non-records branch.
|
||
|
||
It is still not closeable *here*. By the time `Esc` arrives from a panel the
|
||
client's mode no longer says records, and a client holds a `GridCache` rather
|
||
than a `GridLayout`, so records-ness is not a fact it has at all. Closing it
|
||
means **enriching the projection**, the shape gotcha 47 names, never moving
|
||
resolution back into the host.
|
||
|
||
**`:toggle-records-mode` is not the case that proves any of this**, and an
|
||
earlier version of this paragraph claimed it was. It works, but not for the
|
||
reason given: `ToggleRecordsMode` emits `change_mode(RecordsNormal)` explicitly
|
||
on entry, and on exit `ViewBack` restores the mode `SwitchView` pushed, so any
|
||
resolution's answer is **overwritten in both directions** and the case exercises
|
||
nothing. `a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal`
|
||
drives the crossing in both directions instead, because a careful comment
|
||
standing in for a test is what let the per-effect version ship.
|
||
|
||
### `SetBuffer` — "search" is not a buffer (improvise-gdl)
|
||
|
||
Every name goes into the buffers map. The search query is not a buffer — it has
|
||
its own `ViewState` field and its own `SetSearchQuery` effect — and commands
|
||
reach the right one through `cmd::core::write_buffer`, next to the `read_buffer`
|
||
it mirrors.
|
||
|
||
### `ApplyAndClearDrill`
|
||
|
||
`RecordCol::NewMeasure` / `NewCategory` are never staged: `commit_cell_value`
|
||
applies a new measure or category immediately, since a schema change is not a
|
||
cell edit and has nothing to roll back with the drill.
|
||
|
||
There is deliberately **no `dirty = true`** in it: it declares
|
||
`Model(CellData)` and `App::apply_effects` raises the flag off that
|
||
(improvise-gv9r, gotcha 45). The line that used to be there was the last survivor
|
||
of `MarkDirty` — harmless, and a second home for the one thing that commit set
|
||
out to give exactly one.
|
||
|
||
### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae)
|
||
|
||
A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains
|
||
after it never lands and the error stays on screen. `SaveAs` is the other arm of
|
||
that rule and was left out of the original fix, so `:w /bad/path . q` quit on a
|
||
failed write. Gotcha 21 is the general statement.
|
||
|
||
### `WizardOp`
|
||
|
||
One discrete mutation of the import wizard's own state. Which op a key means in a
|
||
given step is decided by `HandleWizardKey`; the enum is just the verb
|
||
(improvise-2wm).
|
||
|
||
### `AcceptWizardModel`
|
||
|
||
Commit the wizard: build a workbook from its proposals and install it. On failure
|
||
the wizard stays open carrying the error so the user can go back and fix their
|
||
field choices.
|
||
|
||
### `json_import_records` / `ImportJsonHeadless` (improvise-oaq, improvise-k8i)
|
||
|
||
`json_import_records` extracts the records array from an already-parsed JSON
|
||
import value. Precedence: explicit `array_path` → root array → first
|
||
auto-detected array path. It is pure, which is what lets `ImportJsonHeadless`
|
||
parse the file exactly **once** and reuse the parsed value as the pipeline's
|
||
`raw`. CSV warnings (short rows, improvise-k8i) are threaded into the status
|
||
message rather than written to stderr, which is invisible inside the TUI.
|
||
|
||
### `AbortChain`
|
||
|
||
Signals `App::apply_effects` to skip the remaining effects in the batch. The flag
|
||
is reset at the start of every `apply_effects` call, so each dispatch starts
|
||
clean. Use it when a sequence's premise no longer holds — "advance to next cell"
|
||
at bottom-right — and later effects ("re-enter editing there") should be
|
||
short-circuited.
|
||
|
||
### `CleanEmptyRecords`
|
||
|
||
Removes cells whose `CellKey` has no coordinates. These are meaningless records
|
||
that can only be produced by `AddRecordRow` when no page filters are set. Pushed
|
||
by `ToggleRecordsMode` when leaving records mode, as the inverse of the
|
||
`SortData` that runs on entry.
|
||
|
||
---
|
||
|
||
## `ui/app.rs` — the authoritative state, and the three tiers read off one classification
|
||
|
||
### The slice boundary, and the residue
|
||
|
||
`App` is two named slices plus a residue of fields that belong to neither:
|
||
|
||
- `model_state` — document state (`Workbook` + IO bookkeeping). Persists. It is
|
||
distinct from `Workbook` itself (pure document semantics, `improvise-core`)
|
||
because `file_path` and `dirty` are persistence-layer concerns.
|
||
- `view_state` — UI session state (mode, cursors, panels, buffers, …). Does not
|
||
persist. The portable half lives in `improvise-protocol` (improvise-cqi) and is
|
||
re-exported from `ui::app` so every existing caller keeps resolving.
|
||
- The remaining direct fields are tagged in the source with one of five residue
|
||
categories, so the boundary stays explicit:
|
||
- **derived cache** — a pure function of the slices, recomputed on demand
|
||
(`layout`). Owned by `App` because rebuilding belongs to the host.
|
||
- **runtime metadata** — data the runtime feeds in: `last_autosave` (wall-clock
|
||
debounce for `autosave_if_needed`), terminal dimensions.
|
||
- **transient** — a flag whose lifetime is bounded to one method call on `App`,
|
||
reset on entry, never observed across calls (`abort_effects`).
|
||
- **config** — loaded once at startup, read but not written during a session:
|
||
`keymap_set`, `storage`.
|
||
- **session-local** — deliberately *not* portable and *not* snapshot-safe, so
|
||
it cannot live in `ViewState` (improvise-h0b, gotcha 30).
|
||
|
||
`wizard` is session-local on both counts: `ImportWizard` is an `improvise-io`
|
||
type, outside the allowlist a serialized `ViewState` may draw on, and a client
|
||
that connects mid-import must not open onto someone else's half-finished dialog.
|
||
`drill_state` likewise: it holds an `Rc`, so `ViewState` would not even be `Send`,
|
||
and `pending_edits` are uncommitted writes belonging to whoever staged them. Its
|
||
frozen records list is what records-mode layouts iterate, so records do not
|
||
disappear when filters would change.
|
||
|
||
`storage` is where documents are read and written; the host chooses, and every
|
||
effect that touches a document goes through it rather than `std::fs`, so the
|
||
command layer has no filesystem of its own (improvise-fuj, gotcha 2).
|
||
|
||
`DrillState::pending_edits` is keyed by `(record_idx, RecordCol)` — the column as
|
||
a `RecordCol` rather than an index, so applying them does not depend on the
|
||
layout still being the drill layout.
|
||
|
||
### `record_of` and `RecordHash` — the unit of atomicity
|
||
|
||
`record_of` is a cell's coordinate tuple with `_Measure` dropped, re-exported from
|
||
`improvise-core` where it lives beside the `RecordHash` it feeds: the surrogate is
|
||
defined as the hash of exactly this, so two definitions of "the record" would be
|
||
two things to keep in step (ADR 002).
|
||
|
||
It is the unit ADR 001 settles on, and the reason it is not the *cell* is
|
||
gotcha 1: renaming a category coordinate in wide records mode moves *every*
|
||
measure cell in the row, so one legal edit spans a whole record. **A unit finer
|
||
than the coarsest legal write cannot be maintained.**
|
||
|
||
### `record_fingerprints` — a process-local XOR fold
|
||
|
||
A fingerprint per record of the raw cells it holds, keyed by the same surrogate
|
||
the index and the version counter are keyed by. One `u64` folded over each cell's
|
||
key and value. The fold is XOR, so it does not care what order `iter_cells`
|
||
yields — and it cannot cancel a pair against itself, because a key appears at
|
||
most once in the store.
|
||
|
||
This hasher is **not** the surrogate's: it is process-local, compared only against
|
||
a fingerprint taken moments earlier in the same process, and never crosses a wire.
|
||
`RecordHash` is the opposite case and is specified accordingly
|
||
(`improvise_core::model::record`).
|
||
|
||
The value hash is exhaustive and by **bits** rather than by `Debug`: two values
|
||
that print alike must not fingerprint alike, and it runs once per cell per editing
|
||
batch.
|
||
|
||
Only *raw* data is fingerprinted. Formula results live in the model's separate
|
||
`formula_cache`, and a record whose derived value moved because someone edited a
|
||
different record has not itself been written.
|
||
|
||
### `document_generation`, `layout_version`, `cursor_epoch`
|
||
|
||
`document_generation` — how many times the whole document has been replaced.
|
||
Bumped by `install_workbook` and by nothing else, so a client mirror can tell that
|
||
the document it was attached to is gone without the effect that replaced it having
|
||
to reach the session list (improvise-cqq). Not part of `ViewState`: it describes
|
||
the document's identity over this process's lifetime, not a client's view.
|
||
|
||
`layout_version` — shared document state: how many times the grid's *shape* has
|
||
moved, bumped once per batch that is **observed** to move it, never off a
|
||
declaration (ADR 001, improvise-xwi6; improvise-pdvm). The middle tier of the basis: a `CellData` batch
|
||
composed before this moved is stale even if no record it names was touched,
|
||
because the keys it carries hang off a shape that has changed under them.
|
||
|
||
`cursor_epoch` — shared document state: how many times a shape change has declared
|
||
a cursor into this document invalid. Bumped by `reset_cursor` and nothing else
|
||
(improvise-gvnb). **It is a counter rather than a list of the commands that reset
|
||
the cursor**, for the reason gotcha 39 gives for watching `document_generation`
|
||
instead of matching on `LoadModel`: *which* effects renumber the grid is a
|
||
per-effect decision (gotcha 24 asks a new geometry effect to make it), so a table
|
||
of variants here would be a second description of that decision, free to disagree
|
||
with it. Observing the write is not.
|
||
|
||
### `batch_aborted` — the outcome, where `abort_effects` is the mechanism
|
||
|
||
`abort_effects` is transient by design — it is reset the moment the loop breaks,
|
||
so nothing outside `apply_effects` can ever observe it — and that is right for the
|
||
mechanism but leaves the *outcome* unreadable. A caller that ran a one-effect
|
||
batch and needs to know whether that effect refused has no other way to ask:
|
||
`Effect::apply` returns `()` on purpose, so an effect reports by writing the `App`.
|
||
|
||
Its one consumer is `Host::save`, where the batch is literally `[Save]` and so
|
||
"the batch stopped" and "the save failed" are the same fact (gotcha 21,
|
||
improvise-368h). **Naming the real call site matters** because that identity is
|
||
what makes the flag readable at all: over a longer batch it says that *something*
|
||
refused, never which. `App::last_batch_aborted` is the accessor and carries the
|
||
same caveat.
|
||
|
||
### `App::new` recomputes formulas before the first layout
|
||
|
||
The formula cache is rebuilt before building the initial layout so
|
||
formula-derived values are available on the first frame. The cache is keyed by the
|
||
active view's None-axis categories, so the caller gathers them explicitly
|
||
(gotcha 9).
|
||
|
||
### `install_workbook`
|
||
|
||
Replace the whole document. The client's view slice is scoped to the old one — its
|
||
cursor map is keyed by that document's view names — so it is reset to the incoming
|
||
document's active view rather than carried over (improvise-8uz, improvise-cqi).
|
||
|
||
Every connected client's mirror is scoped to the outgoing document in the same
|
||
way, and the effect that got here cannot reach the session list. Bumping
|
||
`document_generation` is how the reset reaches them: `ClientSession::project`
|
||
compares it and reseeds.
|
||
|
||
### `adopt_cursor_epoch` — the epoch is one trigger of two, the view is the audience
|
||
|
||
`reset_cursor` resets the host's own slice; every connected client owes the same
|
||
invalidation, and `cursor_epoch` is how the reset reaches them. **The trigger stays
|
||
observed** — which effects renumber the grid is a per-effect decision, so a
|
||
table of variants here would be a second description of it (gotcha 24).
|
||
|
||
**The epoch is the host's own cursor, so it is blind to a view the host is not on**,
|
||
and since improvise-lltt a view-configuration command names its view, so such a
|
||
write is reachable. The second trigger is `App::last_reframed_elsewhere` — the
|
||
non-active views this batch was observed to reframe — and it is a *precise* set
|
||
where `last_reframed_views` is deliberately a union with the reshaped set. Two
|
||
consequences. A batch that reframes only elsewhere relays against that precise set,
|
||
because the union would invalidate a client whose view merely *reshaped* (a page
|
||
selection that prunes nothing), which `paging_with_nothing_pruned_invalidates_no_cursor`
|
||
forbids. And the opening frames it diffs against are taken **lazily, at
|
||
`App::view_mut_of`** — the one door a non-active view can be written through — so
|
||
the held-key path still hashes one view and pays nothing for the wider question
|
||
(the premise improvise-mcq4 narrowed that arm on was `view_mut()` writing only the
|
||
active view, and that premise is gone).
|
||
|
||
**The audience is not everybody**, and has not been since improvise-jytm made
|
||
"which view a client is on" per-client: a reshape of view A threw away the cursor of
|
||
a client sitting on view B, which nothing reshaped. `cursor_invalidated` asks
|
||
`App::last_reshaped_views` — the observed set, so `AddCategory`, which seats itself
|
||
on an axis in *every* view, still reaches everybody — and the relay is one
|
||
`Broadcast::To(id, ResetCursor)` per affected client rather than one `ToAll`.
|
||
|
||
An **empty** reshaped set means "not known", never "nobody": a batch that moved the
|
||
cursor with no view observed to change shape gets the safe answer, everybody.
|
||
Telling too few is strictly worse than telling too many — the diff then believes
|
||
each client already holds origin-positioned content and sends nothing, so they stay
|
||
stale permanently rather than until their next scroll. That is the half-fix
|
||
gotcha 24 warns about, and `cursor_invalidated` is a named function with both arms
|
||
tested so the fail-safe cannot be tidied away.
|
||
|
||
### `record_version` / `resolve_record` / `resolve_aggregate`
|
||
|
||
`record_version` takes a cell key or the record's own key indifferently —
|
||
`RecordHash::of` normalizes. It is **zero** for a record nothing has written,
|
||
which is deliberately also what a record that does not exist reports: a basis
|
||
composed against an absent record and one composed against an untouched record are
|
||
the same claim. The counters live in the `DataStore` beside the index that
|
||
resolves a surrogate, because both are keyed by the record and both are
|
||
invalidated at the same moment (ADR 002). They are still *shared document state*,
|
||
never `ViewState` (gotcha 30); what moved is where on the document they sit, not
|
||
whose they are.
|
||
|
||
`resolve_record` maps a client's record surrogate back to the cell it names
|
||
(ADR 002). `None` means **reject**: there is nothing to write to. Two sources, in
|
||
this order, and the order is the point.
|
||
|
||
- The **index** in `DataStore` is primary. It covers every record that holds data
|
||
whether or not it is on screen, which is what a regenerate-from-the-layout
|
||
scheme could not do: a record scrolled out of the viewport would have been
|
||
indistinguishable from a record that no longer exists, conflating "apply" with
|
||
"reject".
|
||
- The **layout** is the fallback, and it is not a weakening of that. A cell with no
|
||
value yet is named by the grid and by nothing in the store, and typing into an
|
||
empty cell is the whole of improvise-1ey — so the layout is consulted for
|
||
exactly the records that have no data to index. It is bounded by the grid rather
|
||
than the viewport, so an off-screen empty cell resolves too, and a layout change
|
||
correctly makes a surrogate composed against the old shape unresolvable.
|
||
|
||
`resolve_aggregate` goes against the layout and *only* the layout — not because
|
||
that is cheaper but because an aggregate has no existence outside the layout that
|
||
derives it. There is nothing stored to index. A layout change therefore makes an
|
||
old aggregate id unresolvable, which is the right answer: the cell it named is not
|
||
there any more.
|
||
|
||
`layout_cell_keys` excludes records mode: its `cell_key` is a synthetic
|
||
`(_Index, _Dim)` pair carrying *positions*, which is the positional-identity trap
|
||
ADR 002 rejected `_Index` for. A records-mode cell is projected from
|
||
`RecordRow::coords`, which is already in the store, so the index answers it.
|
||
|
||
### `switch_to_view` and `reset_cursor`
|
||
|
||
`switch_to_view` points the view slice at `name` and switches the workbook to it.
|
||
The two copies of "which view is active" must move together: the workbook's is the
|
||
document's, `ViewState`'s is the client's, and the client's is what keys the
|
||
cursor map (improvise-cqi, gotcha 29).
|
||
|
||
`reset_cursor` sends the active view's cursor back to the origin, called when the
|
||
grid's shape changes underneath it. It resets *this* slice, which on a server is
|
||
the host's own and nobody's client. Every connected client owes the same
|
||
invalidation, and `cursor_epoch` is how the reset reaches them: the effect that
|
||
got here cannot see the session list, so `Sessions::apply` watches the counter
|
||
instead — the same shape as `document_generation` and for the same reason
|
||
(improvise-gvnb, gotcha 24).
|
||
|
||
### `display_value_at_cursor` / `display_value_of`
|
||
|
||
`display_value_at_cursor` is editable text for the cell under the cursor: a staged
|
||
drill edit if one exists, else the layout's resolved value for synthetic record
|
||
cells, else the stored cell; empty when the cursor is off-grid. It reads
|
||
`self.layout`, so callers that have mutated the grid must `rebuild_layout()` (or
|
||
emit `RebuildLayout`) first (gotcha 23).
|
||
|
||
`display_value_of` is the value an editor opened on `key` starts with —
|
||
unformatted, and so **not** `CellDisplay::text` (design-principles §4: display
|
||
rounding is view-only and must not feed back into the model). Crate-visible so
|
||
`project::cell_display` can put it on the wire as `CellDisplay::edit_text`; a
|
||
browser that un-formatted the drawn text instead would be a second derivation, and
|
||
a lossy one.
|
||
|
||
### `apply_effects` — one classification, three readers
|
||
|
||
Apply a batch of effects and report which widgets it could have changed, so a host
|
||
with connected clients knows what to project (improvise-cqq). **The `Dirty`
|
||
accumulates inside the loop: an aborted batch must not claim the effects it never
|
||
ran** (gotcha 21).
|
||
|
||
**`layout_version` and `model_state.dirty` are no longer read off the
|
||
`EffectKind`** (improvise-75ld, improvise-pdvm). A kind says what an effect *means*
|
||
to do, and both counters were believing it. Two failures, in opposite directions,
|
||
from the one assumption:
|
||
|
||
- **`AddItem`/`AddItemInGroup`/`RemoveItem` declare `CellData` and reshape the
|
||
grid.** improvise-9mo9 made them call `reset_cursor`, so `cursor_epoch` moved
|
||
while `layout_version` did not — one event moving one of two counters that
|
||
answer the same question, and a client told its cursor is invalid while its
|
||
layout tier still reads current. (Since improvise-ni6k **no effect calls
|
||
`reset_cursor`**; see "two observations" below.)
|
||
- **A refused or duplicate `AddCategory` declares `LayoutGeometry` and changes
|
||
nothing.** `Model::add_category` returns `Ok` for a name it already has, so the
|
||
reachable case does not even abort. Every other client's next `Command::Model`
|
||
was refused with `Rejection::LayoutMoved`, and the browser worker rewrote the
|
||
whole OPFS document, for a keystroke that did nothing.
|
||
|
||
So **a counter is bumped on an observed change, not on a declared intent** — the
|
||
answer `record_versions` already had, and the reason `cursor_epoch` is a counter
|
||
rather than a table of variants. Two fingerprints are taken around the batch, both
|
||
gated on the batch containing *any* `EffectKind::Model(_)`:
|
||
|
||
`view_shape` is one view's **drawn** shape: the axis map, the effective item names
|
||
minus this view's hidden ones, the collapsed groups that have something drawn under
|
||
them, the page selection, `prune_empty`. Drawn, not configured — `hide_item`
|
||
records a name whether or not the model has one, and a hidden ghost moves no row.
|
||
`view_shapes` is that per view name, and it answers two questions: did anything
|
||
move (`layout_version`), and *which views* moved (`Sessions::adopt_cursor_epoch`,
|
||
improvise-nynk). `layout_version` is therefore bumped **once per batch** rather
|
||
than once per effect; nothing reads it but "did it move".
|
||
|
||
### Two observations, not one (improvise-ni6k)
|
||
|
||
`view_shape` above is not the only fingerprint taken per batch, and the reason is
|
||
the part worth keeping: **the cursor's question and `layout_version`'s question are
|
||
different questions, and a single hash cannot answer both.**
|
||
|
||
`layout_version` asks *are the coordinates a client's keys hang off still the ones
|
||
it composed against*. The cursor asks *does position `(r, c)` still name what it
|
||
named*. `AddCategory` of a category with no items separates them exactly: no row
|
||
and no column moves, so no cursor is invalid — but every `CellKey` now needs a
|
||
coordinate it did not before, so the basis tier must move. Unifying the two broke
|
||
tests in **both** directions, which is how the distinction was found rather than
|
||
reasoned to; do not re-merge them.
|
||
|
||
So `view_frame` sits beside `view_shape`, and `last_reframed_views` beside
|
||
`last_reshaped_views`. `view_frame` hashes the **drawn frame** — `row_cats`,
|
||
`col_cats`, and the `AxisEntry` sequence of both axes in order, with each group
|
||
header's collapse state. Four things about it are load-bearing:
|
||
|
||
- It builds through **`GridLayout::with_frozen_records`, never `GridLayout::new`**.
|
||
`new` does not apply `prune_empty`; a fingerprint built on it cannot see a pruned
|
||
row, which is the entire class improvise-7kvw belongs to.
|
||
- It hashes each record's **coordinates** as well as the axis entries, because
|
||
records mode numbers its rows by position (gotcha 1): `row_items` is `0,1,2…`
|
||
whatever order the store is in. `SortData` changes neither the row count nor any
|
||
label and still makes every position name a different record — and drill
|
||
`pending_edits` are keyed by row index, so this is gotcha 18's disease exactly.
|
||
- It hashes **no page selection**, deliberately. A page flip renumbers the grid
|
||
only when `prune_empty` is on, and then `with_frozen_records` already sees it in
|
||
the entries; hashing the selection as well homes the cursor on every `[`/`]` with
|
||
pruning off, which PR #76 ruled is worse than the hole improvise-7kvw names. Both
|
||
arms are tested. The temptation is to argue the selection belongs in a *basis*
|
||
because the cells under it differ — true, and irrelevant: that is
|
||
`record_versions`' job, and this hash answers the cursor.
|
||
- The diff runs **inside the effect loop**, per model effect, for the same reason
|
||
`document_config` does. At end-of-batch it overwrites a cursor a *later* effect
|
||
in the same batch placed deliberately — `[AddItem(measure), <place cursor>]`,
|
||
which is how naming a measure at the bottom-right of a records grid works, sent
|
||
the cursor home.
|
||
|
||
**No effect declares any of this.** improvise-9mo9's twelve `reset_cursor` calls
|
||
are gone, along with the six inline no-op predicates (`moved`, `drawn`, `real`,
|
||
`existed`) that existed only to stop those calls firing on a no-op, and the two
|
||
helpers `effective_items` and `item_is_drawn` that served them. That table was the
|
||
shape gotcha 24 names as the disease, and it had a hole (improvise-7kvw) the day it
|
||
was written; six more were enumerated on the ticket before it was fixed.
|
||
|
||
**Is every reframe also a reshape? No — measured, and the answer matters
|
||
(improvise-9v0j).** The relation between the two observations was separated by
|
||
example and never stated as a property. One direction is the known asymmetry and
|
||
the reason there are two: `AddCategory` of an empty category reshapes without
|
||
reframing. The other direction was open, and the ticket guessed "probably yes,
|
||
and if not, `layout_version` under-reports, which per gotcha 35 leaves a client
|
||
wrong forever".
|
||
|
||
Probed 2026-09-21 by driving each `ModelCommand` through `apply_command` and
|
||
reading `cursor_epoch()` and `layout_version()`: every command in
|
||
`reshaping_commands()` moves both, but a **`ClearCell` that prunes a row out of a
|
||
pruning view moves the cursor epoch and not `layout_version`**. That is not a
|
||
defect in the observation — it is improvise-mcq4 working exactly as designed,
|
||
since a cell write changes the data and not the view's *configuration*, and
|
||
`view_shape` hashes configuration. So the candidate property test 9v0j proposes
|
||
would fail, and writing it would be wrong.
|
||
|
||
What is left open is narrower and worth stating as a question rather than
|
||
answered here: after such a write the grid has renumbered while `layout_version`
|
||
has not moved, so a client still claiming the old layout tier is accepted. The
|
||
record tier covers a write to the *pruned* record; it says nothing about a write
|
||
to a surviving one whose row index moved under it. Whether that is a hole in ADR
|
||
001's middle tier or correctly delegated to the record tier is an ADR question,
|
||
not a test to land quietly, and is filed as improvise-6nsd. **Note the probe
|
||
needed the right fixture to say anything**: run against `app_with_two_pruning_views`, seven of the twelve
|
||
commands are no-ops because that document has no `Product` category, and a table
|
||
of `false`/`false` rows reads exactly like a finding.
|
||
|
||
**Cost, measured rather than asserted** (`benches/reshape_observation.rs`, the
|
||
fixtures `grid_projection` uses): one `SetCell` batch goes from 172µs to 209µs at
|
||
400 cells, 7.09ms to 7.97ms at 18,000, and 35.2ms to 38.0ms at 90,000 — +8% to
|
||
+22%, the largest share on the smallest fixture where the batch is cheapest. The
|
||
batch is dominated by `rebuild_layout` and `recompute_formulas`, not by the
|
||
fingerprint. The ticket required this measurement before landing, because "much
|
||
cheaper than `grid_of`" is exactly the kind of claim that criterion exists to
|
||
refuse.
|
||
|
||
**The cost interacts with improvise-748f, and the fix is where the surprise was.**
|
||
748f split the geometry fingerprint so a batch writing no cell stays O(config) off
|
||
the held-key path. `view_frame` builds through `with_frozen_records`, which applies
|
||
`prune_empty`, which walks cells — so for a pruned view the walk came back, four
|
||
builds per batch. Measured, one `CycleAxis` batch with pruning **on**:
|
||
|
||
| cells | first version | now | fingerprint off |
|
||
|---|---|---|---|
|
||
| 400 | 283.2µs | 167.0µs | 93.6µs |
|
||
| 9,600 | 7.34ms | 3.879ms | 1.967ms |
|
||
| 18,000 | 12.41ms | 5.752ms | 3.043ms |
|
||
| 90,000 | 91.69ms | 56.54ms | 28.52ms |
|
||
|
||
4.1× down to 1.9× at the canonical size, back inside the 16.7ms budget.
|
||
|
||
**The obvious fix bought nothing, and then bought a bug.** Narrowing the
|
||
fingerprint from every view to the active one looks like the answer and is not: a
|
||
second view is usually a fresh one with everything on `Axis::None`, which costs
|
||
nothing to build. The per-view multiplier was never the cost — two *expensive*
|
||
builds were. What worked instead is that the **opening basis is free**: at batch
|
||
start `self.layout` already is the active view's pruned layout, so
|
||
`opening_view_frame` hashes it rather than building a second. 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, and it is opening-use only
|
||
because `self.layout` goes stale mid-batch (gotcha 23).
|
||
|
||
**The narrowing was kept anyway, and that was wrong.** It reads as the honest
|
||
shape — the cursor's question is about one view — but a `CellData` write can prune
|
||
a row out of a view *nobody is looking at*, and `view_shapes` cannot see it
|
||
(config-only: it hashes the `prune_empty` flag, not which rows survive). So a
|
||
bystander was left positioned on a grid that had lost a row, permanently, which is
|
||
gotcha 24's worst case. **A measurement showing a change is free is not evidence
|
||
that it is correct**, and the two questions were answered in the wrong order.
|
||
|
||
The arms are now split by the thing that differs: the **`LayoutGeometry`** arm
|
||
keeps the cheap one-view frame, because `view_mut()` writes only the active view
|
||
and the structural cases are already covered by `view_shapes`; the **`CellData`**
|
||
arm pays for every view. Measured: **0%** on the held-key path, **+5.7%** on an
|
||
18k-cell two-view `SetCell`, scaling linearly in view count.
|
||
|
||
The frame basis is **keyed by view name**, and **it follows `active_view`**. A
|
||
batch that switches and then runs a model effect was comparing the new view's
|
||
hash against the old view's — a near-certain mismatch, so a spurious reset. The
|
||
first fix guarded the *comparison* on the names agreeing, which exempted the
|
||
first geometry effect after a mid-batch switch from the check altogether
|
||
(improvise-9c9o): under-reporting, gotcha 35's expensive side. So the re-base
|
||
happens at the **switch** — in the loop, as soon as the name moves — and by the
|
||
time the geometry arm runs the name and the basis agree by construction. Note
|
||
what this did *not* need: `SwitchView` stays `ViewOnly`, because the basis is
|
||
`App`'s own bookkeeping and none of it is on the wire.
|
||
|
||
It costs one extra `view_frame` per batch that both switches and moves geometry
|
||
— the re-base builds the incoming view's layout and the geometry arm builds it
|
||
again. `DrillIntoCell` already pays one per `SetAxis`, so this is the batch that
|
||
pays most, and the direction is gotcha 35's cheap one.
|
||
|
||
**And writing the test for all this found the bug that was hiding it.**
|
||
### The elsewhere path is only as good as the inputs it can rebuild (improvise-ac2z)
|
||
|
||
`grid_in` on a view the host is not on rebuilds a `GridLayout` from the `Model`
|
||
and that view's config. Everything the *host* derived and cached stays the host's,
|
||
and `Model::formula_cache` is exactly that: `rebuild_layout` fills it with
|
||
`recompute_formulas(&none_cats)` for the **host's** view. A client on a view whose
|
||
`none_cats` differ therefore reads formula values keyed by somebody else's
|
||
coordinates — and since `prune_empty` drops what resolves to nothing, its formula
|
||
columns **vanish** rather than showing wrong numbers.
|
||
|
||
`prune_empty` (improvise-t6db) was the first instance of this shape and was one
|
||
line. This is the second, and it is not local: `grid_in` takes `&App` while
|
||
`recompute_formulas` needs `&mut Model`, so it cannot be fixed where it is
|
||
observed. Filed rather than bodged; candidates are on the ticket.
|
||
|
||
**How it was found is the part to carry.** The PR #78 hand-back reported this
|
||
hazard as "argued and *not* observed", from a fixture whose stated setup —
|
||
a category on `Axis::None` in one view and on Column in another — was **absent**:
|
||
`add_category` defaults a new category to Column and nothing moved it. A negative
|
||
result from a setup lacking its own condition reads identically to a real one, and
|
||
is worse than silence, because it stops the next person looking. Setting the axis
|
||
explicitly failed the property on the first run.
|
||
|
||
`project::grid_in` built a non-active view with `GridLayout::new`, which does not
|
||
apply `prune_empty` — so a client on another view was projected rows its own
|
||
config prunes, and its grid never lost a row, so the staleness was unobservable.
|
||
Third instance of the `new`-versus-`with_frozen_records` trap. Fixed, and it broke
|
||
no other test.
|
||
|
||
**Two fixtures nearly hid all of this, and both are the same mistake.** The `SetCell`
|
||
benchmark shows the fingerprint at +2–5%, because 748f's own `document_content` walk
|
||
dominates that arm. And the geometry benchmark, written first against the existing
|
||
fixture, had `prune_empty` **off** — where `with_frozen_records` does not walk cells
|
||
at all and the batch reads 17–176µs. A fixture that cannot reach the expensive branch
|
||
measures the cheap one and reports it as the cost.
|
||
|
||
**What is still guarded in one direction only, and is filed**: nothing asserts that
|
||
a batch which reframes a view is *always* one that `view_shape` also moves, or the
|
||
converse. The separation is argued here and tested by example, not by property.
|
||
|
||
`document_config` is everything a `LayoutGeometry` effect can write that the file
|
||
would carry: the categories with their items and groups, and every view's
|
||
configuration whether it draws or not. It is what raises `model_state.dirty`, and
|
||
it is a *different* hash from `view_shape` on purpose — the number format and a
|
||
hidden ghost belong to the bytes on disk and not to the shape. The cell data is
|
||
absent from both, because `record_fingerprints` answers for it and a whole-document
|
||
hash per keystroke is not affordable (gotcha 41: `Storage` is whole-document).
|
||
|
||
`ModelChange::CellData` and `ModelChange::WholeWorkbook` still raise `dirty` off
|
||
the declaration, because they write things no structural fingerprint can see: a
|
||
formula, a sort order, a replaced document.
|
||
|
||
**Both halves are read inside the loop, and the position is the property.** The
|
||
observed half compares `document_config` after each `LayoutGeometry` effect against
|
||
the value it held after the previous one, rather than once around the whole batch.
|
||
An after-the-loop comparison is *correct* about whether the document changed and
|
||
still wrong, because it re-raises a flag `Save` cleared **earlier in the same
|
||
batch**: `[SetNumberFormat, Save]` — reachable from `:format ,.2f . w` and from any
|
||
`Binding::Sequence` pairing a geometry effect with a save — then reported a
|
||
successful save and a dirty document, so the TUI drew "Saved to …" while `:q`
|
||
refused, and the worker paid a whole-document OPFS rewrite. That is the failure
|
||
class this whole section exists to close, reintroduced one level up. Guarded by
|
||
`a_batch_that_changes_the_layout_and_then_saves_ends_clean`, over both arms. It still
|
||
over-approximates there — a `LoadModel` that cannot read its path leaves the flag
|
||
raised, costing one redundant write, where the other direction costs an edit. It
|
||
used to be a separate `MarkDirty` effect that each *command* composed alongside its
|
||
mutation, which improvise-kmgs's one-at-a-time derivation put `SetCell` on the wire
|
||
without (improvise-gv9r): a derivation correct per element and wrong for a set.
|
||
|
||
`Save` is `ViewOnly` and clears the flag itself, so a batch that edits and then
|
||
saves ends clean: the order in the loop is the order the effects were composed in.
|
||
That is why the observation is in the loop and not after it.
|
||
|
||
`layout_version` **is** bumped after the loop, and that is not the same hazard:
|
||
nothing clears it, and an aborted batch's effects never ran, so a comparison taken
|
||
at the end sees only what did run. Neither arm of that is tested today; filed with
|
||
the orchestrator rather than cited here, because an id that was never read back is
|
||
worse than no id.
|
||
|
||
**What the split does not reach**, and is not a regression because the declared
|
||
scheme did not reach it either: `prune_empty` makes a view's shape depend on the
|
||
data, so a cell edit that empties a row prunes it without moving `view_shape`. The
|
||
record tier covers the write; the shape tier does not see the reshape.
|
||
|
||
Both fingerprints sort every `HashMap`/`HashSet` they touch. That is not tidiness:
|
||
an unsorted iteration order is per-process (gotcha 18), and a fingerprint that
|
||
re-randomises every run would bump both counters on every batch.
|
||
|
||
**`record_versions` cannot be read off the kind**, because an `EffectKind` says
|
||
*that* a batch writes cells, never *which* — so the records are read off the data
|
||
itself, fingerprinted around the batch. Two consequences worth stating. A write
|
||
that lands the value already there bumps nothing, which is the right answer:
|
||
nobody's basis was invalidated by it. And a `LayoutGeometry` effect that drops
|
||
cells will bump their records when it shares a batch with a `CellData` one — an
|
||
over-bump, costing a rejection that need not have happened, which is the same
|
||
asymmetry `Dirty::of` is deliberately sloppy in.
|
||
|
||
**`CellData` reporting `formulas: true` stopped being slack (improvise-g3no).**
|
||
It was written as over-reporting in the cheap direction: a cell write cannot
|
||
change a formula, so the entry cost one redundant `Projection` and bought
|
||
nothing. Since `Model::remove_item` drops the formulas targeting the item, and
|
||
`effect::RemoveItem` is `CellData`, that entry is now **load-bearing** — a
|
||
delete really can take a formula out of the document, and tightening `CellData`
|
||
to drop `formulas` would leave every client's formula panel showing it forever,
|
||
which is `Dirty`'s expensive direction. Nothing guards it: the existing
|
||
`removing_a_category_empties_the_clients_formula_panel` covers the
|
||
`LayoutGeometry` twin only, and its item-level sibling is improvise-en4s
|
||
rather than written. Read this paragraph before narrowing that arm.
|
||
|
||
**The case list is checked for completeness, not for length** (improvise-26dm).
|
||
`layout_geometry_effects_actually_change_document_config` walked a hand-built
|
||
list under an `assert_eq!(cases.len(), 13)`, and a count catches a case being
|
||
*deleted* and never one being *absent* — the two are written identically. The
|
||
adversarial review of PR #78 added a fourteenth `pub struct` declaring
|
||
`LayoutGeometry` and the suite stayed green.
|
||
`doctrine(a_list_checked_for_length_reads_like_a_list_checked_for_completeness)`.
|
||
|
||
improvise-5ubs's answer — read serde's derive-generated variant list out of its
|
||
`unknown variant … expected one of` message — cannot reach this one: an effect
|
||
is a struct implementing a trait, so there is no derived enumeration of effects
|
||
at all. But gotcha 38 says `ModelCommand` is derived from the model effects,
|
||
with `model_effects` mapping each variant back to the effect it came from, so
|
||
the list is now diffed against the `LayoutGeometry` effects *reachable through
|
||
`ModelCommand`*, and the failure names the missing effect rather than a number.
|
||
The two lists agree exactly today, which is itself the evidence the
|
||
substitution lost nothing.
|
||
|
||
**The right-hand side had to be bounded before it could be believed** (PR #96,
|
||
found independently by this branch's review and by ChatGPT Codex).
|
||
`every_model_command()`'s doc comment claimed the enumeration was
|
||
compiler-forced — `variant_name` matches exhaustively, so a new variant fails
|
||
the build. That inference is invalid: the build fails in *`variant_name`*, and
|
||
nothing obliged the value list to gain an entry. Deleting one from it failed
|
||
this check and nothing else in the workspace.
|
||
|
||
So the list is now itself checked, by `improvise-5ubs`'s mechanism:
|
||
`the_enumeration_names_every_variant_the_derive_declares` diffs it against
|
||
`variants::declared::<ModelCommand>()`, serde's derive-generated variant list.
|
||
That makes the chain **derive → `every_model_command()` → `model_effects` →
|
||
case list**, every link checked, and the whole thing is what the ticket asked
|
||
for. `variants` was `#[cfg(test)] pub(crate)`, so reaching it costs a
|
||
`test-support` feature on `improvise-protocol` gating an optional `serde_json`
|
||
— **off in every shipped build**, which is why the wasm gate is unaffected.
|
||
The assertion message still names **both** directions, because a short
|
||
right-hand side reads as a spurious case and invites deleting the left,
|
||
degrading both sides together — the failure the ticket exists to close.
|
||
|
||
**Its bound is stated rather than assumed**: this closes the effects on the
|
||
wire, and an effect declaring `LayoutGeometry` that no `ModelCommand` names is
|
||
still invisible — which is precisely the probe variant PR #78 injected. That is
|
||
an honest residual and not a fix, and it is the reason the ticket asked for a
|
||
compiler-maintained enumeration of *effects*: none exists without a proc macro
|
||
or inventory-style registration, and a fourth hand-rolled list in a fourth
|
||
place is the disease. A second `[…; 13]` array survives one function away in
|
||
`every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint` and is
|
||
filed, not smuggled in.
|
||
|
||
On abort, the loop resets `abort_effects` so the next dispatch starts clean and
|
||
records `batch_aborted` where a caller can still read it.
|
||
|
||
### `bump_changed_records`
|
||
|
||
Bump every record whose raw cells read differently than they did at `before`. Both
|
||
directions count: a record that gained cells, lost them, or had one rewritten has
|
||
moved under anybody holding an older basis.
|
||
|
||
The diff collects into a **set**, not a list: a record present in both maps is
|
||
named twice by the chain, and a rewritten record would otherwise count as two
|
||
writes. (That bug survived one test run — gotcha 40.)
|
||
|
||
### `dispatch_key`
|
||
|
||
Resolve one keypress through the keymap and apply what it produces, reporting
|
||
which widgets changed. The whole key path, with the projection half factored out:
|
||
a host with no connected clients (the native TUI) drops the `Dirty`, and one with
|
||
clients hands it to `Sessions::project_all`. `ui::reduce` is the named door onto
|
||
this (improvise-gxi).
|
||
|
||
---
|
||
|
||
## `ui/project.rs` — turning what the server did into what each client is told
|
||
|
||
### `grid_of`'s whole-grid cost, measured (improvise-i5tk)
|
||
|
||
The TUI builds its `GridCache` over the whole grid at offsets zero, so a cache
|
||
position is an absolute position (gotcha 44). improvise-i5tk proposed narrowing
|
||
that to the viewport, as the browser client already does, and made the benchmark
|
||
acceptance criterion 1 — because "this can dominate TUI rendering" is exactly as
|
||
unevidenced as "it is fine". `benches/grid_projection.rs` is that benchmark and it
|
||
says **no**. One `grid_of` call, dense pivot grids, this machine:
|
||
|
||
```
|
||
typical 100 x 4 = 400 cells whole-grid 1.8-1.9ms ratio 1.2x col_widths 45%
|
||
large 1200 x 8 = 9600 cells whole-grid 175-177ms ratio 1.4x col_widths 46-49%
|
||
18k cells (gotcha 41) 3000 x 6 = 18000 cells whole-grid 318-328ms ratio 1.5x col_widths 45-46%
|
||
pathological 9000 x 10 = 90000 cells whole-grid 4.8-4.9s ratio 1.3-1.4x col_widths 45-47%
|
||
```
|
||
|
||
**Those are min-of-N figures, and the first version of this harness could not
|
||
reproduce its own.** It took the *mean* over a handful of iterations, which on a
|
||
loaded machine is dominated by whatever else is running: review measured the same
|
||
`col_widths` share at 8% and at **131%** — a part taking longer than the whole
|
||
containing it, printed flat because nothing checked. It also passed
|
||
`(comma, decimals) = (false, 2)` where `grid_of` passes the active view's
|
||
`(true, 0)`, so it timed a call that never happens and reported it as a share of
|
||
one that does. Both are fixed: the arguments come from `parse_number_format` on
|
||
the view, the estimator is min-of-N, and a share above 100% prints `UNSTABLE`
|
||
rather than a number that reads like a measurement. **A benchmark that cannot
|
||
detect its own impossible output is not evidence** — the standard
|
||
`doctrine(check_the_checker)` sets for a test, applied to a measurement.
|
||
|
||
`ratio` is whole-grid over a 45×12 terminal window: **1.2–1.5x**, against a fix
|
||
whose stated cost is teaching `GridWidget` that a cache index means something else
|
||
and putting every scroll site in scope. That is the wrong trade, so i5tk is closed
|
||
on the measurement rather than on the reasoning either way.
|
||
|
||
The reason the ratio is so low is the finding, and it is bigger than the ticket
|
||
was. Two of the three whole-grid passes a frame performs are **viewport-
|
||
independent by construction**, so no amount of windowing reaches them:
|
||
`ui/viewport.rs::compute_col_widths` takes no viewport and walks every `(row, col)`
|
||
*twice* — once for cell text, once for the column totals it must also size — and a
|
||
column total has to sum every row including the ones a viewport scrolled past
|
||
(gotcha 44). `grid_of`'s own `cell_display` loop is the third pass and the only one
|
||
narrowing would touch. So the whole grid is evaluated about three times per frame,
|
||
every result discarded, and at 18,000 cells a keypress costs four tenths of a
|
||
second. That is improvise-jmjz, and its fix is caching or one fused pass — not
|
||
windowing, which this benchmark has now ruled out.
|
||
|
||
### One walk, and what it did and did not buy (improvise-jmjz)
|
||
|
||
`ui/viewport.rs::grid_body` is now the one walk. It evaluates each cell **once**
|
||
and derives all three answers from that value: widen the column, add to the
|
||
column's sum, and — if the position is inside the requested window — keep the
|
||
`CellValue` for `cell_display` to dress. `compute_col_widths` survives as
|
||
`grid_body` with an **empty window**, because the keypress path
|
||
(`App::apply_effects`) wants widths and no cells, and two width rules would be
|
||
the disease `improvise-edp` names.
|
||
|
||
Measured on the orchestrator's machine, 2026-09-21, before and after, with
|
||
`cargo bench --bench grid_projection`:
|
||
|
||
| case | before | after |
|
||
|---|---|---|
|
||
| typical (400 cells) | 1.840 ms | 0.687 ms |
|
||
| large (9,600) | 172.6 ms | 50.0 ms |
|
||
| 18k (gotcha 41) | 294.4 ms | 95.2 ms |
|
||
| pathological (90,000) | 4.481 s | 1.320 s |
|
||
|
||
**About 3x, which is the number of passes removed** — the shape the acceptance
|
||
criterion asked for, rather than a speedup argued from the structure.
|
||
|
||
Three things to carry, none of which the ticket predicted.
|
||
|
||
**The `col_widths` column of that benchmark now measures something else.** It
|
||
read 45–47% of the frame and now reads 64–82%, which looks like a regression and
|
||
is the opposite: `compute_col_widths` *is* the fused walk, so its share rose
|
||
while the frame it is a share of fell by two thirds. A ratio whose denominator
|
||
moved is not comparable across the change, and the benchmark's own comment now
|
||
says so — otherwise the next reader concludes the fix made widths worse.
|
||
|
||
**The windowing ratio did not improve, and that is correct.** It stays 1.2–1.4x,
|
||
because the pass that remains is exactly the one a viewport cannot narrow: the
|
||
widths and the totals are whole-grid by definition (gotcha 44). improvise-i5tk's
|
||
decline stands, and the two findings are now cleanly separated — windowing was
|
||
never the fix, and removing the redundancy never made windowing worth it.
|
||
|
||
**The keypress path got the same fix for free.** `compute_col_widths` is the
|
||
walk now rather than two of them, so `App::apply_effects`' own width computation
|
||
roughly halved on the same fixtures (134 ms to 68 ms at 18,000 cells). That is
|
||
the 748f held-key path, and it was not the ticket's subject.
|
||
|
||
**It is not solved, only three times cheaper.** 18,000 cells is 95 ms, still 5.7
|
||
frames at 60fps, and the remaining cost is one genuine evaluation per cell. The
|
||
next lever is caching *across* frames, which needs an invalidation story the
|
||
`record_versions`/`layout_version` tiers could plausibly supply.
|
||
|
||
The guard is `a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths`:
|
||
with one walk serving three consumers, a window that leaked into the other two
|
||
would silently make a column total the sum of the visible rows, or size a column
|
||
by what happens to be on screen. **Two** faults, then, and its first fixture could
|
||
see only one — see `context/test-evidence/command.md`, because the way it was
|
||
blind is more instructive than the fixture that fixed it. Briefly: the widths half
|
||
was asserted by comparing `grid_body` against `compute_col_widths`, which **is**
|
||
`grid_body` with an empty window, so windowing degrades both sides identically.
|
||
**The same function twice is not a check**, and it reads exactly like one.
|
||
|
||
The benchmark carries `test = false`: it takes seconds by design, and the suite's
|
||
rule is that no single test binary exceeds two of them.
|
||
|
||
### What the module is (improvise-cqq)
|
||
|
||
The **producer** half of the per-widget event architecture (improvise-rkeq):
|
||
`improvise-protocol` says how a client folds a `Projection`, and this module says
|
||
which projections the server emits. Nothing here renders, and nothing here holds a
|
||
transport — a websocket or a worker port calls `Sessions::project_all` and writes
|
||
the result out.
|
||
|
||
Three decisions shape it.
|
||
|
||
**What changed is decided by diff, not by the effect that changed it.** An effect
|
||
knows its `EffectKind` but not which cells it touched, and asking every effect to
|
||
report its cell set would be a second, silently divergent description of what it
|
||
did. So the server rebuilds the *desired* state of each widget and compares it with
|
||
the mirror of what that client was last told. The effect kinds still earn their
|
||
keep: `Dirty` turns a batch's kinds into the set of widgets worth rebuilding, so a
|
||
`ViewOnly` batch costs nothing, and a cell edit outside a client's viewport
|
||
produces no traffic for that client because the desired viewport is unchanged.
|
||
|
||
**Only the grid gets a mirror.** The other widgets' updates replace their cache
|
||
wholesale (`Projection::Formulas` *is* the whole list), so "has it changed" and
|
||
"send it" collapse into one decision, which `Dirty` already made. The grid's
|
||
`Cells` update is positional and incremental, so it needs to know what the client
|
||
already holds.
|
||
|
||
**A viewport is the client's report, not the server's guess.** The scroll offsets
|
||
are already in the command stream and reach the server's mirror through
|
||
`reduce_view`, so the server reads them off the mirrored `ViewState`. The
|
||
*extent* — how many rows and columns fit — is a fact about a terminal or a browser
|
||
window that no `Command` carries, and inventing one would break
|
||
`decision(improvise-cqi, command_set_derived_not_invented)`. It is set on the
|
||
session by the host instead (`ClientSession::set_extent`).
|
||
|
||
The emission order is load-bearing: a `GridUpdate::Cells` that overtakes the labels
|
||
naming its positions is dropped by the consumer, so `grid_updates` emits labels
|
||
first and diffs the cells against the frame those labels will leave behind
|
||
(gotcha 33).
|
||
|
||
### `Dirty` — a gate, not a description
|
||
|
||
Which widgets a batch of effects could have changed. The projections themselves are
|
||
computed by comparing current state with what the client holds, so a `Dirty` that
|
||
over-reports costs work and never correctness. Under-reporting *is* a correctness
|
||
bug, which is why `Dirty::of` matches exhaustively on `EffectKind` — a new
|
||
`ModelChange` variant fails the build.
|
||
|
||
#### `Dirty::of` — the derivation (gotcha 35)
|
||
|
||
**This section is the record of the derivation that the doc comment used to be.
|
||
Re-read it when an effect is reclassified.** It has gone stale once already,
|
||
claiming `AddCategory` was `CellData` after it had been moved.
|
||
|
||
Read off the effects that carry each kind, not guessed:
|
||
|
||
- `ModelChange::CellData` is worn by `AddItem`, `RemoveItem`, `AddFormula` and
|
||
`RemoveFormula` as well as `SetCell`, so it reaches the category panel and the
|
||
formula panel, but no view's axes move under it.
|
||
- `ModelChange::LayoutGeometry` is worn by `SetAxis`, `CreateView`, `DeleteView` —
|
||
and by `AddCategory`/`RemoveCategory`, which rewrite every view's axis map
|
||
(gotcha 3) — so it reaches every axis-bearing widget *and* the formula list,
|
||
because `Model::remove_category` ends by dropping every formula that targets the
|
||
category it removed.
|
||
|
||
The formula list is the one entry where over- and under-reporting are not
|
||
symmetric in cost: a `LayoutGeometry` batch that leaves the formulas alone pays one
|
||
redundant `Projection::Formulas`, while one that dropped a formula and said
|
||
nothing leaves the client's `FormulaCache` wrong forever.
|
||
|
||
The `LayoutGeometry` arm is spelled out rather than written as `EVERYTHING`: the
|
||
two coincide today, but they are derived from different effects and an entry added
|
||
for one must not silently arrive for the other.
|
||
|
||
### `grid_in`'s elsewhere path and the inputs it must rebuild (improvise-ac2z)
|
||
|
||
`grid_in` projects a view the host may not be on, and
|
||
`doctrine(the_elsewhere_path_is_only_as_correct_as_the_inputs_it_can_rebuild)`
|
||
says to enumerate the inputs and ask of **each** whether it was derived for the
|
||
caller or for the subject. Two have been found that way. `prune_empty` was the
|
||
first (improvise-t6db): `GridLayout::new` does not apply it, so the fix was
|
||
`with_frozen_records`. `Model::formula_cache` is the second, and it was worse,
|
||
because it cannot be fixed where it is observed — `grid_in` takes `&App` and
|
||
`recompute_formulas` needs `&mut Model`.
|
||
|
||
`rederived_for` answers it by **copying**: where the target view's `none_cats`
|
||
differ from the host's and the document has formulas, it clones the model and
|
||
recomputes for that view. The three conditions are what keep the clone rare —
|
||
the common case is a client on the host's own view, which pays nothing, and a
|
||
formula-free document pays nothing ever. It is an interim: keying
|
||
`formula_cache` by `none_cats`, so one `Model` holds more than one view's
|
||
derived values, is the durable answer and is an `improvise-core` change.
|
||
|
||
The list is not closed. Anything else `rebuild_layout` computes for the host's
|
||
view and `grid_in` then reads is the next instance, and nothing enumerates them
|
||
— which is the doctrine's whole point.
|
||
|
||
### `grid_of` — the extent is the whole grid
|
||
|
||
`GridExtent` carries the whole grid, not the window: it is what a client clamps its
|
||
own cursor against (improvise-bpq2, gotcha 47). It is read off the layout rather
|
||
than off the projected ranges, which are already clipped.
|
||
|
||
### `axis_frame` — one axis's frame, windowed
|
||
|
||
Three edge rules, and every one of them is about a group header, because a header
|
||
occupies a row without consuming a data index (gotcha 1's cousin: the data index
|
||
counts items, the visual position counts entries).
|
||
|
||
- **One header above the window comes with it.** A column window opening inside
|
||
`Q1` must still be able to say the column is in `Q1` (`AxisFrame::group_of`), so
|
||
the nearest header above the first in-window item is carried — and only that one,
|
||
or the payload grows with the groups a deep scroll passed. `AxisFrame::body_from`
|
||
then declines to *paint* it, which is the cut `GridLayout::data_row_to_visual`
|
||
always made. In the loop, `entries.is_empty()` is true exactly until the first
|
||
in-window item lands, which is what makes the compaction correct.
|
||
- **Consecutive headers *inside* the window all come.**
|
||
`[North, Q1 (collapsed), Q2, Feb]` is a real shape — a collapsed group emits its
|
||
header and no items — and only the nearest wins `group_of`, but dropping the far
|
||
one would lose the only row saying those items exist. (Seen, not yet acted on, in
|
||
a browser: `z` is `Upstream` and `web/app.js` puts no listener on a group
|
||
header.) **Above** the window it is the other way round: nothing there is
|
||
painted, so only the nearest is kept.
|
||
- **A trailing header comes only when the window reaches the last item.** A
|
||
collapsed group at the bottom of the grid has no items to hang off and is still a
|
||
row the user must see; but a window scrolled to the middle must not be handed the
|
||
bottom of the document.
|
||
|
||
### `column_totals`
|
||
|
||
Each visible column summed over **every** row of the grid, not just the rows in the
|
||
viewport. That is why it is projected rather than left to the client, and it is the
|
||
same sum the TUI's total row has always drawn. Empty in records mode, where a
|
||
column is a measure or a coordinate and a column sum means nothing, and empty for a
|
||
grid with no rows or no columns — an empty strip says "no totals", which is a
|
||
different statement from a strip of blanks (gotcha 33).
|
||
|
||
### `cell_display`
|
||
|
||
One cell as the client draws it. The server is the side that knows whether a value
|
||
was a number, so it says so rather than letting the client re-derive it from the
|
||
text — and by the same argument it is the side that knows whether the cell is
|
||
settable, so it says that too (ADR 002, `cell_id`).
|
||
|
||
Records mode resolves through the frozen snapshot, which yields display text and
|
||
nothing else; numeric-ness is read back off the text, **the one place this module
|
||
infers rather than knows**.
|
||
|
||
`edit_text` is the *unformatted* value, which is what the TUI's own
|
||
`display_value_at_cursor` seeds its edit buffer from. Projecting it rather than
|
||
letting a client un-format `text` is the rule from gotcha 47: where a client cannot
|
||
compose the command, enrich the projection.
|
||
|
||
### `cell_id` — stored cell or aggregate
|
||
|
||
What the cell at `(row, col)` *is*, expressed as the surrogate the client echoes
|
||
back. **The fork is not invented here.** `EditOrDrill` (`cmd/mode.rs`) already
|
||
makes exactly this decision on every keypress in the TUI, and asks it the same way:
|
||
a cell is aggregated when a **regular** category sits on the None axis. Virtual
|
||
categories are always on None in pivot mode and imply nothing, and a records-mode
|
||
cell is never aggregated whatever the axes say — which is a property of the layout,
|
||
not of a mode flag.
|
||
|
||
In records mode it uses the row's *coordinates*, never the synthetic
|
||
`(_Index, _Dim)` key `cell_key` returns there: that one carries positions, and a
|
||
position is what ADR 002 rejected `_Index` for.
|
||
|
||
`RecordCol::Category`, `NewMeasure` and `NewCategory` are **not cells to write
|
||
into** and get no id. Editing a category column renames a coordinate across every
|
||
measure cell in the row (gotcha 1) and the `+`/`+cat` columns create a measure or a
|
||
category; all are different intents from `SetCell`, so none gets an id it could be
|
||
composed with.
|
||
|
||
### `category_rows`
|
||
|
||
The categories the category panel draws: **every** category, virtual ones included,
|
||
with their items and their axis in the active view.
|
||
|
||
`category_names`, not `regular_category_names` (improvise-vjms): the TUI panel has
|
||
always listed `_Index`/`_Dim`/`_Measure` and the browser panel is to list the same
|
||
rows, so the filter belonged in neither.
|
||
|
||
`effective_item_names`, not `category(name).items`: `_Measure`'s items are explicit
|
||
items ∪ formula targets (gotcha 7), and the raw list omits every target. That is
|
||
the whole reason this is one function and not two.
|
||
|
||
`try_axis_of`, not `axis_of`: a view that has never seen a category has no entry
|
||
for it and `axis_of` panics. Unseen is `None`, which is what the view will show it
|
||
as.
|
||
|
||
### The four whole-cache accessors, for a host that holds the `App`
|
||
|
||
A host that renders in the same process as the authoritative `App` — the ratatui
|
||
binary today — has no transport to fold a projection stream out of, but must draw
|
||
the same widgets from the same data as a remote client. These four give it each
|
||
widget's cache outright. They are the same derivation `document_projections` sends,
|
||
spelled once: a projection is built *from* the cache, so an in-process renderer and
|
||
a remote one cannot disagree about what a widget holds.
|
||
|
||
There is deliberately **no** function returning all of them together, and no struct
|
||
to hold them: that aggregate is the shape improvise-rkeq rejected, and a caller
|
||
that wants several calls several.
|
||
|
||
**Their use by the TUI is temporary.** The native binary calls these once per
|
||
frame, rebuilding from the `App` rather than folding updates, so that widgets can
|
||
migrate off `&Model`/`&View` one at a time while the binary split is in flight. The
|
||
per-frame rebuild is what goes away when the TUI becomes a client; the functions
|
||
themselves stay, because the server needs them. Nothing in `src/` should come to
|
||
depend on *when* they are called.
|
||
|
||
### `document_projections`
|
||
|
||
The projections for every widget whose content does not depend on a client's
|
||
viewport. Each of these updates replaces its cache wholesale, so `dirty` is the
|
||
whole decision.
|
||
|
||
### `wizard_of` / `wizard_cache` (improvise-764)
|
||
|
||
The import wizard as the widget draws it, or `None` when no import is open. **This
|
||
is where the wizard's `improvise-io` types stop.** `FieldKind`, `DateComponent`,
|
||
`ImportPipeline` and the built `Workbook` are all read here and none of them
|
||
leaves; what comes out is `WizardCache`, which names only strings, booleans and its
|
||
own two enums.
|
||
|
||
Unlike every other `*_of` in the module it feeds no `Projection`: the wizard is
|
||
session-local (gotcha 30) and broadcasting it would draw one client's dialogue on
|
||
every client — the rendering-side twin of the bug improvise-tom1 removed from
|
||
`ModelCommand`.
|
||
|
||
`wizard_cache` is split out from `wizard_of` so it can be exercised against a
|
||
wizard driven through its steps, with no `App` in the way. The `App` half is one
|
||
`Option::map`; this half is where every `improvise-io` type is spent.
|
||
|
||
`DATE_COMPONENTS` lists the date components the wizard offers, **in the order it
|
||
offers them**. The order is load-bearing: `ImportWizard::date_config_at_cursor`
|
||
counts these same rows, so the cursor the cache carries indexes this list.
|
||
|
||
`summarize_workbook` is the one-line description of the workbook awaiting
|
||
confirmation. It lived in the widget, which is how a renderer came to be calling
|
||
`regular_category_names()` and `measure_item_names()` on a `Model`.
|
||
|
||
### `grid_updates` — the diff, and what it diffs against
|
||
|
||
The updates that carry `mirror` to `desired`. Labels come first and cells last,
|
||
because the consumer's fold is not commutative: a `Cells` update that overtakes the
|
||
labels naming its positions is dropped. The cells are therefore compared not
|
||
against `mirror` but against the frame the label updates will leave — which is also
|
||
what makes the result exact rather than merely sufficient, since a relabelled grid
|
||
keeps the cells that still fit.
|
||
|
||
**The totals are diffed against that same framed mirror** (improvise-hkt6, found by
|
||
Copilot). `apply` blanks the total under a *relabelled* column (gotcha 33(b)), so
|
||
comparing against `mirror` missed exactly the case where the label moved and the
|
||
number did not: no `Totals` went out, the client blanked its own, and
|
||
`ClientSession::project_diff`'s `self.grid = desired` left the server believing it
|
||
was still there. A client cache wrong **forever**, which is the direction
|
||
`Dirty::of` is deliberately sloppy to avoid (gotcha 35).
|
||
|
||
**Only the totals.** The widths stay on `mirror`: folding the labels re-fits them
|
||
too, so diffing widths against the fold would suppress the `ColWidths` that
|
||
`a_client_on_a_socket_is_seeded_and_gets_its_edit_back` asserts a reframe carries —
|
||
redundant, but it is gotcha 33(c)'s standing evidence and not this ticket's to
|
||
spend.
|
||
|
||
### `ClientSession`
|
||
|
||
One connected client as the server sees it: the server's mirror of that client's
|
||
`ViewState` — maintained by folding the same commands the client folds, never by
|
||
shipping state — and the grid cache the client is believed to hold.
|
||
|
||
`basis` is the basis this client was last told (ADR 001, improvise-fkfg). It is
|
||
mirrored for the same reason the grid is: it is diffed, so a client hears about it
|
||
exactly when it moves. Without publication the check would be unsatisfiable —
|
||
nothing else in the stream carries any of the three tiers, so a client could only
|
||
ever forge one, and ADR 001 does not say where a client gets its basis from. **That
|
||
omission is the one place the ADR met the code and was wrong.**
|
||
|
||
`fold` folds a client→server command into this client's mirrored view slice — the
|
||
same function the client runs, so the two cannot drift. It is private, and reached
|
||
only through `Sessions::apply`: a command with a document half cannot be honoured
|
||
from inside one session, and a door that takes every `Command` while honouring only
|
||
some of them is a convention every transport author would have to be told about.
|
||
|
||
`project` is everything this client must be told, given what the last batch
|
||
changed. The grid is emitted when the document dirtied it *or* when this client's
|
||
own viewport moved — a scroll is `ViewOnly` and reaches no other client, but it
|
||
still changes what this one can see. A mirror seeded against a document that has
|
||
since been replaced is reseeded instead of diffed: its cursor map is keyed by the
|
||
outgoing document's view names (gotcha 24), so every position in it means nothing
|
||
here. The check lives on this path rather than on a hook because the effect that
|
||
installs a workbook cannot reach the session list, and a hook nothing can call is a
|
||
convention, not a mechanism.
|
||
|
||
`basis_update` is the basis to publish, if it has moved since this client last
|
||
heard. It is emitted **after** the grid updates in the same batch, and that order
|
||
is load-bearing: the record counters describe the cells those updates carry, so a
|
||
basis arriving first would let a client claim a version for a value it had not yet
|
||
been shown. It is the mirror image of gotcha 33's rule that labels precede the
|
||
cells they frame. The records published are the ones this client's grid *names*,
|
||
which is exactly the set it can address a write to (ADR 002); publishing the whole
|
||
document's counters would be both larger and no more useful, since a surrogate the
|
||
client has never been projected is one it cannot compose a `SetCell` for.
|
||
|
||
`reseed` points the mirror at the app's current document, holding nothing, and
|
||
returns the projections that carry it back — a compaction of the stream so far, not
|
||
a replay of it. That is what `connect` does for a new client, and what a document
|
||
swap does for every existing one.
|
||
|
||
### `Broadcast` — the audience is part of the message
|
||
|
||
The two cases mean different things and a host cannot infer which from the command.
|
||
Making it a field of the thing being sent is the same move as `Applied` being an
|
||
enum: a host must name the audience to get at the command
|
||
(`doctrine(an_invariant_is_made_unrepresentable_not_validated)`).
|
||
|
||
- `To(SessionId, _)` — one named client. A *correction* of something only that
|
||
client's UI holds, which no other client is affected by. It has two cases, and
|
||
they are why it names a session rather than "the asker": a switch to a view the
|
||
document has not got, which is the asker (improvise-6c6q), and a view deleted out
|
||
from under whoever was on it, which is not. `ToAll` would double-apply the first
|
||
on the accepted path and move views nobody asked to move in the second.
|
||
- `ToAll` — every client, the asker included. Not a correction but a
|
||
**consequence**: the document did something that invalidates a fact each client
|
||
holds privately, and the asker holds that fact too. A `Command::Model` is never
|
||
folded optimistically — a client cannot predict what the document will accept —
|
||
so the asker is owed it on exactly the same terms as everyone else
|
||
(improvise-gvnb, gotcha 36).
|
||
|
||
### `Applied` — an enum because a document swap is not a diff
|
||
|
||
A command that *replaced the document* obliges the host to do something
|
||
categorically different, and an obligation stated in a field is one a host can read
|
||
past. Matching is how it is made unavoidable: a host that wants the `Dirty` has to
|
||
name the other cases to get at it.
|
||
|
||
The distinction is not cosmetic. The server's own mirrors self-heal —
|
||
`ClientSession::project` reseeds the moment the generation moves — but
|
||
`ViewState::cursors` lives on the **client**, keyed by the outgoing document's view
|
||
names, and only a fresh `Snapshot` replaces it (gotcha 24). Before `Command::Model`
|
||
existed, a document swap could only come from this host's own keypress; now another
|
||
client can ask for one, and the host that must send the snapshots is not the host
|
||
that asked.
|
||
|
||
- `Diff { dirty, broadcast }` — the document is still the same one. `dirty` feeds
|
||
`Sessions::project_all`. `broadcast` carries what clients must fold into their
|
||
own view slices: the server moved their mirrors itself, but a client's slice
|
||
lives on the client, and the only thing keeping the two from drifting is that
|
||
both run `reduce_view` over the same command. It is a `Vec` rather than an
|
||
`Option` because one command can owe two relays with different audiences, and the
|
||
two are not mutually exclusive by construction — only by which variants happen to
|
||
exist today. An `Option` would have had to drop one silently.
|
||
- `Rebased { snapshots }` — the document was replaced and every connected client
|
||
owes a bootstrap. There is no `Dirty` and nothing to broadcast: each `Snapshot`
|
||
already carries a whole view slice and the seed that reproduces the new document,
|
||
so a diff against the old one would describe nothing that still exists. The
|
||
snapshots are addressed by `SessionId` because a transport delivers them one
|
||
connection at a time.
|
||
- `Rejected { reason }` — the batch was composed against a picture of the document
|
||
that has since moved, and **nothing was applied** (ADR 001, improvise-fkfg). A
|
||
normal outcome, not an error: the client already holds the current state through
|
||
projections, and what it owes the user is to keep the typing, say what moved, and
|
||
offer force where force is coherent — which is what `Rejection` distinguishes. It
|
||
is a third case for the reason the enum exists at all (gotcha 39): a host that
|
||
wants the `Dirty` has to match, and matching is what stops "the server refused
|
||
this" being silently read past. The rejection is addressed to the one session
|
||
that asked, because unlike a document change it is nobody else's business —
|
||
nothing happened.
|
||
|
||
### `Sessions::connect`
|
||
|
||
Attach a client and hand it the bootstrap `Snapshot`: a fresh view slice pointed at
|
||
the document's active view, plus the seed it folds to catch up. The snapshot's view
|
||
is the mirror's, **cloned — not a second value built to the same recipe**. `reseed`
|
||
is what decides what a freshly seeded mirror holds, and a client told anything else
|
||
starts out disagreeing with the server's picture of it.
|
||
|
||
### `Sessions::apply` — the one door (gotcha 36)
|
||
|
||
It takes the authoritative `App` because some commands are not one client's
|
||
business alone. `Command::Model` is a document mutation by definition and goes to
|
||
`apply_model`, never to a mirror: the model is shared, so what the other clients are
|
||
told is the `Dirty` it produced, not the command itself. `SwitchView` has a
|
||
**document half** — the workbook's active view and the client's must move together,
|
||
and `App::switch_to_view` is one of the only two writers allowed to touch either
|
||
(gotcha 29) — so it is routed through `apply_command` rather than folded into the
|
||
mirror. Folding it into the mirror alone left the session naming a view the
|
||
workbook had never switched to, and the next `project()` built that client a grid
|
||
out of the other view.
|
||
|
||
Everything else has no document half and stays this client's: it never touches the
|
||
`App`'s own view slice, because the model is shared and the UI is not.
|
||
|
||
Lockstep on the active view is today's answer, not the last one: improvise-g0f
|
||
allows two clients to sit on different views, and improvise-jytm owns that split.
|
||
What this shape fixes survives it — the mirror and the state the projection is
|
||
built from move in one call, so whichever way g0f lands there is one place to
|
||
change.
|
||
|
||
Lockstep is a property of *every* session, not of the one that asked. `grid_of`
|
||
builds every client's grid from the workbook's active view, so a session left
|
||
naming the outgoing one is read under a stale cursor key: it is projected the new
|
||
view's rows positioned by the old view's scroll, which is gotcha 24's
|
||
cursor-vs-geometry mismatch arriving through another client. So the accepted switch
|
||
reaches every mirror, and `Applied::broadcast` carries it on to the clients whose
|
||
own slices must fold it too.
|
||
|
||
The basis check is the **first** thing, before the dispatch, and it is a guard
|
||
rather than a branch inside the apply: a batch whose basis moved is never applied,
|
||
not even partially (ADR 001). It is `Checked::against` that runs it, because the
|
||
answer is what buys the `Checked` every mutation below needs — the check is not a
|
||
step this function could forget (improvise-ondj).
|
||
|
||
**Note:** at this commit `Sessions::apply` has no production caller; hosts go
|
||
through `apply_checked`. See the `ui/reduce.rs` section.
|
||
|
||
### `Sessions::apply_checked`
|
||
|
||
The same door, for a command whose answer is already in hand. A frame is a
|
||
*sequence* of commands and ADR 001's atomicity is the frame's, so `Host::commands`
|
||
answers for all of them with a `CheckedBatch` and then applies them through here —
|
||
re-asking per command would reject the later ones for staleness their own siblings
|
||
caused (improvise-955n). It cannot return `Applied::Rejected`: the refusal
|
||
happened, if it happened, before the batch was built.
|
||
|
||
### `apply_switch`
|
||
|
||
Which view a client is on is **that client's** (improvise-jytm, improvise-g0f): the
|
||
model is shared and the UI is not. So this takes a `SessionId` — it is addressed to
|
||
exactly one session, which is the opposite of what it once was — moves only that
|
||
session's mirror, and dirties nothing.
|
||
|
||
The one thing it still asks the document is whether the view exists. A slice keys
|
||
`ViewState::cursors` and is read by `grid_in`, so a mirror naming a view the
|
||
document lacks would index nothing; what is adopted is therefore what was
|
||
*accepted*, which on refusal is the view the session was already on.
|
||
|
||
The audience follows from that, and it is conditional. On the accepted path the
|
||
asker folded the right thing and nobody else is affected, so **nothing is relayed**.
|
||
On the refused path the asker — and only the asker — folded a name the document
|
||
turned down, so it gets `Broadcast::To(id, …)`. `ToOthers` was the pre-jytm shape and
|
||
had the mirror-image hole: it told every client but the one that was wrong. A blanket
|
||
`ToAll` is equally wrong here, and for a reason that survives jytm: the asker really
|
||
did fold, so `ToAll` double-applies on the common path.
|
||
|
||
Note what is **not** here any more. `adopt_active_view` pushed the accepted view into
|
||
every mirror; under a per-client active view that is precisely the bug, so it is
|
||
gone along with the `Broadcast::ToOthers` variant it was the only producer of. A
|
||
mechanism whose reason has gone is deleted rather than left.
|
||
|
||
### `apply_model` (improvise-kmgs)
|
||
|
||
It goes through the same `apply_command`, which runs the effect the command was
|
||
derived from and reads the `Dirty` off that effect's own `EffectKind` — there is no
|
||
second classification here that could disagree with gotcha 35's table.
|
||
|
||
It takes no `SessionId`, and since improvise-jytm that is the *whole* distinction
|
||
from `apply_switch`: a document change is addressed to no one session, a view switch
|
||
to exactly one. No model mutation moves anybody's active view any more —
|
||
`ViewBack`/`ViewForward` left `ModelCommand` with jytm, because the stack they pop is
|
||
one client's history (the `ApplyAndClearDrill` argument, gotcha 38), and a
|
||
`LoadModel` that installs a different document bumps the generation and so lands in
|
||
the rebase arm, where every client gets a whole fresh slice.
|
||
|
||
Two things are left to relay. `adopt_cursor_epoch`'s reset is `Broadcast::ToAll`:
|
||
nothing was folded optimistically, so the asker is not a client being corrected but
|
||
one being told, exactly like the others. And `adopt_deleted_views` is the case a
|
||
per-client active view creates — see below.
|
||
|
||
### `adopt_deleted_views` (improvise-jytm)
|
||
|
||
A `DeleteView` can take away the view a *different* client is sitting on. Its mirror
|
||
then names nothing, `App::resolve_view` falls back to the document's default, and
|
||
that client is projected the default's grid while its own slice still names the
|
||
view that is gone — the server and the client disagreeing about which view keys the
|
||
cursor map, which is gotcha 24's mismatch arriving through somebody else's keystroke.
|
||
|
||
Before the split this could not happen: `Workbook::delete_view` moved the document's
|
||
active view and `adopt_active_view` pushed the result into every mirror. The split
|
||
removed both, so the case has to be answered on its own.
|
||
|
||
It is **observed, not predicted** — the check is "does this mirror still name a view
|
||
the document has", not "was this command a `DeleteView`" — for the reason
|
||
`cursor_epoch` beats matching on `CycleAxis` (gotcha 24) and `document_generation`
|
||
beats matching on `LoadModel` (gotcha 39): a table of which commands can remove a
|
||
view would be a second description of `Workbook`, free to disagree with it.
|
||
|
||
Each affected client is told with `Broadcast::To`, and the others are not: a client
|
||
on another view lost nothing.
|
||
|
||
The basis was answered for by `Checked::against` before the dispatch that reached
|
||
here. That it is a *type* and not a line at the top of the function is what stops a
|
||
second mutation path from being written without it (improvise-ondj). `stale_basis`
|
||
is the only decision, and it has no argument that suppresses it — ADR 001's force is
|
||
a fresh batch under a fresh basis, composed by the client, not a flag that walks
|
||
past it.
|
||
|
||
**Rebase is decided by the generation, not by the variant.** The three
|
||
`WholeWorkbook` commands can land here, and whether they actually replaced the
|
||
document is not a property of the variant: a `LoadModel` that fails to read leaves
|
||
the document alone, and an `AcceptWizardModel` with nothing built does nothing at
|
||
all. So the question asked is the decisive one — did the generation move
|
||
(gotcha 24) — and the rebase is done here rather than handed to the host as an
|
||
instruction, for the same reason `apply_switch` adopts the accepted view itself:
|
||
**the only enforceable version of "you must also do X" is doing X.**
|
||
|
||
### `adopt_cursor_epoch` (improvise-gvnb)
|
||
|
||
If the batch renumbered the grid, invalidate every mirror's cursor and say so on the
|
||
wire.
|
||
|
||
Two halves, and **neither works alone**. Resetting only the mirrors would leave the
|
||
diff believing each client already holds origin-positioned content, so nothing
|
||
would be sent and every client would stay stale *permanently* rather than until its
|
||
next scroll: a partial fix strictly worse than none. Broadcasting only would leave
|
||
the server projecting each client a window it is no longer looking at.
|
||
|
||
The audience is `Broadcast::ToAll`, and the asker is not an exception. Its mirror
|
||
was reset here like everyone else's, and it folded nothing optimistically, because
|
||
a `Command::Model` is not something a client can apply to itself.
|
||
|
||
What is *not* broadcast is a position: another client's transpose must not move
|
||
where I am looking — that is UI and it is mine (improvise-g0f). It may only declare
|
||
that where I was looking has stopped meaning anything, which is a fact about the
|
||
shared geometry.
|
||
|
||
### A refused switch is undone, not overwritten (Codex, PR #115)
|
||
|
||
`apply_switch` used to guard the mirror — folding the switch only if the
|
||
document had the view — and correct a refusal with `SwitchView(accepted)`. Once
|
||
`reduce_view` records history (improvise-gd28), that correction **records
|
||
history too**: the asker folds `Missing` (back gains the view it left), then
|
||
folds the correction (back gains **`Missing`**), while the guarded mirror holds
|
||
neither. `<` then lands on a view the document never had, and
|
||
`forget_vanished_views` cannot help — nothing vanished, the name was never real.
|
||
|
||
Two changes, and the first is the one worth carrying:
|
||
|
||
**The mirror folds what the client folded, refused or not.** A correction can
|
||
only undo what *both* ends hold; guarding one end is what let them drift. This
|
||
is the same rule as `improvise-w3bf` — the mirror runs the client's fold, never
|
||
a second copy of the arm — applied to the refusal path, which had been the
|
||
exception.
|
||
|
||
**The correction is `[ViewBack, ForgetView(asked)]`**, not a `SwitchView` back.
|
||
`ViewBack` is the exact inverse of the push `switched` made; a `SwitchView`
|
||
would push *another* frame and leave the back stack one entry longer than it
|
||
started, which is the bug in miniature rather than its fix.
|
||
|
||
The residue, stated 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. Closing that needs either a
|
||
client that does not fold optimistically or a correction carrying the whole
|
||
slice, and both are larger than the hole. Asking to switch to the view you are
|
||
already on is excluded, because nothing moved and `ViewBack` would then pop
|
||
somebody else's frame.
|
||
|
||
**The user's ruling of 2026-09-21 — "switching the persisted views should also
|
||
be client-only" — will delete this whole path**, and it was not treated as
|
||
having done so already: the refusal is genuinely reachable (a client picks a
|
||
view from its projected list and another client deletes it before the frame
|
||
lands), and the ruling's client-side half is a validation against
|
||
`ViewListCache` that lives in `resolve.rs`. Until that exists, removing the
|
||
server's refusal would leave a client sitting on a name the document has not
|
||
got, showing the default's grid under it.
|
||
|
||
### `forget_vanished_views` (improvise-gd28)
|
||
|
||
Its neighbour, and the two do not subsume each other. `adopt_deleted_views` asks
|
||
which mirrors are **sitting on** a view the document has lost; this one asks whose
|
||
*history* names one. A client that visited a view and left is sitting somewhere
|
||
else entirely, so the first helper cannot see it, and after improvise-gd28 gave
|
||
clients a real `view_back_stack` that client could step back into a view that no
|
||
longer exists.
|
||
|
||
Which views vanished is **observed** — the document's view-name set is read before
|
||
`apply_command` and diffed after — for the reason `adopt_cursor_epoch` watches a
|
||
counter rather than matching on `CycleAxis`: which effects drop a view is a
|
||
per-effect decision, so a list of command variants would be a second description
|
||
of it, free to disagree with the first. `effect::DeleteView` is the obvious one and
|
||
deliberately not the only one contemplated.
|
||
|
||
`ToAll`, because any client may have been there and the asker is not special; a
|
||
client whose history does not name it folds a no-op. The argument for the shape,
|
||
and for the guard that was written and then deleted, is
|
||
`context/code/protocol.md` §2.
|
||
|
||
### `Sessions::rebase`
|
||
|
||
Re-attach every session to a document that has just replaced the one they were
|
||
connected to, and hand each client the bootstrap it needs. The server's own mirrors
|
||
are reset by `ClientSession::project` the moment the generation moves, so this is
|
||
not what keeps the server honest. It is the other half: a client's view slice lives
|
||
on the client, and only a fresh `Snapshot` tells it to drop a cursor that names a
|
||
view of a document that no longer exists.
|
||
|
||
`apply_model` calls it itself when a command moved the generation, and hands the
|
||
result back as `Applied::Rebased`. It stays public because a host also replaces the
|
||
document by its own keypress path, where no `Applied` is produced.
|
||
|
||
### `fresh_view`
|
||
|
||
The view slice a newly-connected client starts from: empty, but pointed at the
|
||
document's active view, because that is what keys its cursor map (improvise-8uz,
|
||
gotcha 29).
|
||
|
||
---
|
||
|
||
## `ui/host.rs` — the session loop
|
||
|
||
### What the module is
|
||
|
||
Messages in, addressed messages out. A websocket is awkward to test, so nothing
|
||
that decides anything lives in one. `Host` is a pure-ish state machine over the
|
||
authoritative `App` — it takes one client's frame and returns the frames that owes
|
||
every client — and `crate::transport` is the shell that moves bytes in and out of
|
||
it. Every test in this crate drives `Host` directly; the socket is exercised by
|
||
running the binary.
|
||
|
||
**One `App` per document, many clients per `App`.** The ticket (improvise-q08)
|
||
asked for one `App` per *connection*, "no shared authoritative state across sessions
|
||
yet". That premise no longer holds (`doctrine(check_the_ticket_premise)`):
|
||
`Sessions` exists precisely to hold many `ClientSession`s against one authoritative
|
||
`App`, `apply_model` invalidates every mirror's cursor rather than the asker's
|
||
(`doctrine(lockstep_binds_every_session_not_the_asker)`, now scoped to the *document*
|
||
half since improvise-jytm made the active view per-client), and
|
||
`Applied::Diff::broadcast` exists only because other clients must hear what this one
|
||
did. An `App` per connection would make all of that dead code and give two browser
|
||
tabs on the same document two documents.
|
||
|
||
Nor is it one `App` per *server*. improvise is a multi-document app, and the unit of
|
||
authority is the **document**: a `Host` is one `App`, one `Sessions`, and the clients
|
||
attached to them. That grain is what `Sessions` was already built for — it never
|
||
owns the `App`, every method takes it as a parameter — and it is what makes
|
||
concurrency scope for free: sessions editing independent models cannot conflict,
|
||
and the basis versions ADR 001 reasons about live on the `App`, one sequence per
|
||
document.
|
||
|
||
So a second document is a second `Host`, and hosting several means a registry of
|
||
them keyed by document id. That registry is `improvise_ws_server::registry`
|
||
(improvise-j31x), and it cost this file **nothing**: a `Host` was always a value
|
||
rather than a singleton, and `handle` is already addressed to one document by virtue
|
||
of being a method. What the registry adds is the only thing a `Host` cannot supply —
|
||
a server-wide address, since `SessionId`s are minted per `Sessions` and every
|
||
document's first client is `SessionId(0)`.
|
||
|
||
What sharing one document costs is honest and stated: there is no per-client
|
||
permission, no conflict resolution beyond last-writer-wins, and a client that can
|
||
connect can rewrite the document. ADR 001 owns where that goes next; this host is
|
||
the localhost single-user case that ADR describes the starting point of.
|
||
|
||
### `Addressed`
|
||
|
||
One outbound frame and the connection it goes to. The host addresses by `SessionId`
|
||
and knows nothing about sockets; the transport owns the map from a session to a
|
||
connection.
|
||
|
||
### `Host`
|
||
|
||
One authoritative document and every client attached to it — the unit a
|
||
multi-document server keys its registry by. A `Host` is a value, never a global, so
|
||
hosting several is a map of these and not a change to anything inside one.
|
||
|
||
It is deliberately **not `Debug`**: `App` is not, and a `Host` printing the whole
|
||
document into a log line would not be wanted even if it were.
|
||
|
||
### `Host::app_mut`
|
||
|
||
The authoritative `App`, for work the **host itself** initiates. Deliberately narrow
|
||
in intent, and not a second door: a *client's* `Command` must go through
|
||
`Host::handle`, which routes it into `Sessions::apply` — the one place a basis is
|
||
checked and the mirrors are kept in lockstep (gotchas 36, 43). What this is for is
|
||
the work a host does on nobody's behalf, where there is no session to address and no
|
||
claim to check: the browser worker writes the document back through the `Save`
|
||
effect after a batch dirties it, because saving is `EffectKind::ViewOnly` and so
|
||
reaches no wire vocabulary.
|
||
|
||
### `Host::connect`
|
||
|
||
Attach a client that has reported an extent of `rows` x `cols`, and hand back its
|
||
bootstrap. Total: there is no way for a connection to fail here, which is why the
|
||
transport reads the extent off the first frame before calling. Nothing is owed to
|
||
the *other* clients — a new connection changes no document state and moves no mirror
|
||
but its own.
|
||
|
||
### `ClientFrame::Key` answers nothing, and that is settled (improvise-bpq2)
|
||
|
||
Resolving a keypress here needs `reduce_key` against the authoritative `App`, which
|
||
emits no `Command` a client could fold — so the two would diverge, which
|
||
`improvise-worker-server`'s
|
||
`a_key_resolved_on_the_host_moves_state_no_client_can_hear` demonstrates. bpq2
|
||
closed that by moving resolution to the client, so a key a client can answer never
|
||
arrives as a `Key` frame at all, and one that does is a binding nothing on this side
|
||
could answer without diverging. Growing the set is a matter of enriching the
|
||
projection until the client can compose the command itself — see
|
||
`improvise-wasm-client::keys`, which carries the list of what is still upstream.
|
||
(Gotcha 47.)
|
||
|
||
### `Host::save` (improvise-368h)
|
||
|
||
Write the document back, because a client asked.
|
||
|
||
**No session state moves and nobody else is told**, which is why it does not go
|
||
through `Sessions` at all: saving changes no document state and no client's view
|
||
slice, so there is no mirror to adopt and nothing to broadcast. What moved is bytes
|
||
in *this host's* storage — the same fact that makes `effect::Save`
|
||
`EffectKind::ViewOnly` and the reason the `ModelCommand` derivation could never reach
|
||
it (gotchas 3, 38). Success is silent, on the same terms as any accepted command
|
||
that dirties no widget.
|
||
|
||
Both refusals are the host's to answer, not `Checked`'s: neither is about a basis.
|
||
The missing path is decided **before** anything runs, because a save that cannot land
|
||
should be refused rather than attempted and then reported. The failed write can only
|
||
be found by trying, and `Save` signals it by aborting its batch (gotcha 21) — the
|
||
batch here is exactly `[Save]`, so an abort is that failure and nothing else.
|
||
Reporting a failed write as success is the thing this exists to stop.
|
||
|
||
### `Host::resize`
|
||
|
||
A resize dirties no document state at all — but it moves this client's viewport, and
|
||
`ClientSession::project` emits the grid when the viewport moved even under
|
||
`Dirty::NOTHING`. So the fan-out is the same call as everywhere else, with nothing
|
||
dirty. A frame for a session that has already gone is dropped: there is no
|
||
connection left to answer.
|
||
|
||
### `Host::commands` — answered whole, then applied whole (improvise-955n)
|
||
|
||
Order is why a batch exists at all: one keypress resolved on a client is
|
||
`SetSelected` plus the scroll offsets that follow it, and a host that folded the
|
||
selection but not the offset projects that client the wrong window.
|
||
|
||
It used to apply in order and abandon at the first rejection, which left a hole
|
||
ADR 001's "not even partially" does not have: a refused frame's *earlier* commands
|
||
stayed applied. The check has moved from the command to the frame, so there is no
|
||
longer a moment at which part of a frame has landed and the rest has been refused —
|
||
the refusal, if any, happens before anything is applied, and
|
||
`Sessions::apply_checked` cannot produce one. `CheckedBatch` carries what a batch's
|
||
basis *means* and the one residue this does not close (clause 4).
|
||
|
||
### `Host::command`
|
||
|
||
The one door (gotcha 36), and the match that follows it is the mechanism
|
||
(gotcha 39): asking for the `Dirty` means naming the case where a `Snapshot` is owed
|
||
instead.
|
||
|
||
- `Rebased` — the document was replaced. Nothing to diff against and nothing to
|
||
broadcast; each snapshot already carries a whole view slice and the seed that
|
||
reproduces the new document.
|
||
- `Diff` — **the broadcast precedes the projections**, so a client's view slice is in
|
||
the state the grid it is about to be sent was built from. Dropping it for the
|
||
bystanders leaves their slices naming the outgoing view while that grid comes from
|
||
the incoming one — a stale mirror arriving through somebody else's keystroke
|
||
(gotcha 36), which is the bug PR #10 was found to have. Who each item reaches is
|
||
carried by the item, not decided here.
|
||
- `Rejected` — unreachable through this door since improvise-955n, since a frame is
|
||
refused before any of it is applied, by `commands`. It is still matched because
|
||
`Applied` is the shape `Sessions::apply` answers in and a host that wants the
|
||
`Dirty` must name every case (gotcha 39). The answer is the same one `commands`
|
||
gives: nothing was applied, so nothing is owed to anybody else, and the asker is
|
||
told only that its batch did not land and which class that was.
|
||
|
||
### `Host::relay` / `Host::project`
|
||
|
||
Who hears a relayed command is the relay's own business, not the host's guess:
|
||
`Broadcast` names its audience and the match is what makes the host honour it
|
||
(improvise-gvnb).
|
||
|
||
`project` drains the outbound projections — the ticket's "projection queue", which
|
||
turned out to be a call rather than a queue. Clients with nothing to hear are
|
||
omitted by `project_all` and so get no frame.
|
||
|
||
---
|
||
|
||
## `ui/viewport.rs` and `ui/cat_tree.rs` — the residue of two shared derivations
|
||
|
||
### `viewport.rs`
|
||
|
||
`MAX_ROW_HEADER_W`/`MIN_ROW_HEADER_W` moved to `improvise-protocol` with the rule
|
||
that clamps to them (improvise-1125): a browser sizing its own row header must
|
||
reach the same numbers, and it cannot depend on this crate.
|
||
|
||
`compute_col_widths` computes adaptive column widths for pivot mode (header labels
|
||
+ cell values). Header widths use the widest *individual* level label, **not** the
|
||
joined multi-level string, matching how the grid renderer draws each level on its
|
||
own row with repeat-suppression.
|
||
|
||
`compute_row_header_width` is the total row-header width for a layout. The rule
|
||
itself is `improvise_protocol::header_level_widths`, which is where both front ends
|
||
reach it (improvise-1125). This is the one caller that holds a `GridLayout` rather
|
||
than a projected frame — `App::visible_cols`, deciding how many columns fit — and it
|
||
is on the keypress path, so it hands the rule **borrowed** tuples rather than
|
||
building a frame and cloning every row. It does not reach for a `View` either,
|
||
because a collapsed flag cannot change a width.
|
||
|
||
### `cat_tree.rs`
|
||
|
||
The derivation itself moved to `improvise_protocol::viewmodel` (improvise-edp),
|
||
because the browser client needs it and can only reach the protocol crate. What is
|
||
left here is the path that call sites in this crate and in the TUI already spell,
|
||
and the one convenience that needs an `App`: building the cache before flattening
|
||
it.
|
||
|
||
`build_cat_tree` builds the flattened tree straight from the model, for a host that
|
||
holds the authoritative state rather than a folded cache — the same shortcut
|
||
`project::category_tree_of` is for the widgets.
|
||
|
||
---
|
||
|
||
## `command/keymap.rs` — the dispatch/registry half
|
||
|
||
### What is here and what is not
|
||
|
||
The **table** — `KeyPattern`, `Binding`, `Keymap`, `ModeKey`, `TextEntry`,
|
||
`PanelKeymap` and `Keymaps::default_keymaps()` — lives in
|
||
`improvise_protocol::keymap`, so the browser client looks up the same bindings the
|
||
terminal does rather than mirroring them. Read that module's docs for why.
|
||
|
||
What is here is what a thin client cannot have: resolving a bound *name* into
|
||
`Effect`s through a `CmdRegistry`, and the `SetTransientPrefix` effect that arms a
|
||
prefix key. **This file is the authority on that split** — the doc review named it
|
||
as such — so a claim about where the keymap lives belongs here and in
|
||
`improvise-protocol::keymap`, and nowhere else.
|
||
|
||
### `SetTransientPrefix` (improvise-jc8)
|
||
|
||
Arms a prefix key, so the next keypress is dispatched against that prefix's
|
||
sub-keymap. It records the *address* of the prefix — the key that opened it —
|
||
rather than the sub-keymap itself. `KeymapSet` already owns every keymap, so
|
||
holding an `Arc<Keymap>` in `ViewState` would only put a non-portable type into what
|
||
is meant to become portable, shared state (gotcha 13).
|
||
|
||
### `effects_of` — why a `Sequence` flattens, and why it stays that way (improvise-0hb)
|
||
|
||
The one place a keymap lookup becomes a mutation, and the reason `Binding` itself
|
||
can be registry-free data.
|
||
|
||
Every step is executed against the **same pre-dispatch `ctx`**, and the effects are
|
||
concatenated into one vec that `App::apply_effects` runs as one batch. So a later
|
||
*command* cannot observe an earlier one's effects — only a later *effect* can.
|
||
improvise-0hb proposed closing that: apply effects so far between steps, rebuild the
|
||
layout, build a fresh `CmdContext`, dispatch the next. The design pass says **no**,
|
||
on three findings.
|
||
|
||
**It would silently reopen improvise-8tk, a P1 data loss.** `apply_effects` resets
|
||
`abort_effects` at entry *and* again when it breaks, because the flag is per batch.
|
||
Applying between steps therefore dispatches step 2 into a fresh batch with the flag
|
||
cleared, so the `Quit` after a failed `Save` would land — the exact silent discard
|
||
8tk fixed. Gotcha 21's claim that the abort "carries across sequence steps" is true
|
||
*because* of this flattening, not in spite of it. It was also untested until
|
||
recently: no shipped `Sequence` has a first step that can abort, and `ZZ` is a
|
||
prefix onto the single `wq` command.
|
||
`app.rs::a_sequence_whose_first_step_aborts_does_not_run_the_second` is the guard,
|
||
and it fails when the abort is made per-step.
|
||
|
||
**Almost nothing would benefit.** Of the shipped `Sequence` bindings, all but one
|
||
come from `TextEntry` and are either `[<commit>, clear-buffer <buf>]` or
|
||
`[clear-buffer <buf>, enter-mode <exit>]` — `clear-buffer` and `enter-mode` take
|
||
their operand as a literal arg and read nothing from `ctx`, so a refreshed `ctx`
|
||
would change no outcome. The single genuine composition is records `o` =
|
||
`[add-record-row, enter-edit-at-cursor]`, and it already works, because
|
||
`EnterEditAtCursorCmd` fronts its effect with `RebuildLayout` and the effect
|
||
re-reads the layout itself (gotcha 23). Every Esc and every commit keypress would
|
||
pay a layout rebuild and a `CmdContext` construction — which recomputes column
|
||
widths — to buy that nothing.
|
||
|
||
**And the ticket's motivating rewrite does not typecheck as a binding anyway.** Its
|
||
`[(w, []), (force-quit, [])]` cannot dispatch: `w` is `register_pure`, whose
|
||
interactive constructor rejects an empty arg list, so `run` returns `None` and the
|
||
whole binding is dead. The nullary spelling is `save`.
|
||
`every_bound_command_name_is_registered` checks exactly this (gotcha 15) and would
|
||
have caught it.
|
||
|
||
If command-level transactionality is ever wanted, it wants its own campaign: the
|
||
abort must become a property of the *sequence* rather than of the batch before any
|
||
step is applied separately.
|
||
|
||
---
|
||
|
||
## `command/` — the command layer
|
||
|
||
### `lib.rs` / `command/mod.rs` / `command/parse.rs`
|
||
|
||
The crate is the `Cmd`/`Effect` pipeline, the keymaps and the `App` state machine,
|
||
with no terminal dependency; rendering lives in the root `improvise` crate. All
|
||
model mutations go through the command layer so they can be replayed, scripted and
|
||
tested without the TUI: commands are trait objects (`dyn Cmd`) producing effects
|
||
(`dyn Effect`), and the headless CLI (`--cmd` / `--script`) parses quasi-lisp text
|
||
into effects and applies them directly.
|
||
|
||
`improvise_protocol` is re-exported as `crate::protocol` so a host — the ratatui
|
||
binary, a wasm worker — can name a widget cache without taking its own
|
||
`improvise-protocol` dependency, and so the cache types a host renders are the same
|
||
ones `ui::project` builds (improvise-rkeq).
|
||
|
||
`command::key` re-exports `improvise_protocol::key` (improvise-cqi): the key types
|
||
ride in `ViewState::transient_prefix`, so they must live where the wire types do,
|
||
and the re-export keeps `crate::command::key::…` paths working.
|
||
|
||
`parse.rs` is the quasi-lisp prefix parser: `word arg1 arg2 …`, several commands per
|
||
line separated by `.`, coordinate pairs written `Category/Item`, and quoted strings
|
||
(`"Profit = Revenue - Cost"`).
|
||
|
||
### `cmd/core.rs`
|
||
|
||
`CmdContext` is the read-only context commands decide against. Commands receive a
|
||
`&Model` (pure data) and a `&View` (the active view); the full `&Workbook` is also
|
||
available for the rare command that needs to enumerate all views, such as the view
|
||
panel.
|
||
|
||
The interactive arm (keymap dispatch, scripts, the `:` line) is built **from**
|
||
`parse`: `register` takes `parse`, a `contextualize` that fills context-only
|
||
fields of the command `parse` built without seeing the args, and an optional
|
||
`fallback` reached only for argument lists `parse` refused. So an accepted
|
||
argument list is never overridden by context (improvise-2bac; the argument is
|
||
`context/code/cli.md` §8). `register_pure` is the case with nothing to
|
||
contextualize and no fallback: both arms are `parse`.
|
||
|
||
`write_buffer` is the effect that writes a named buffer — the mirror of
|
||
`read_buffer`, and the only place that knows "search" is not an entry in the buffers
|
||
map but its own `ViewState` field (improvise-gdl, gotcha 11). That knowledge used to
|
||
sit inside the `SetBuffer` *effect*, where nothing paired it with the read side and
|
||
every caller had to know that passing `"search"` meant something else than it said.
|
||
|
||
### `cmd/cell.rs` / `cmd/navigation.rs` — the explicit-state convention
|
||
|
||
All cell commands take an explicit `CellKey`; the interactive spec fills it from
|
||
`ctx.cell_key()` and the parser fills it from `Cat/Item` coordinate args. Navigation
|
||
commands take explicit cursor state the same way: the interactive spec fills
|
||
position and bounds from context, and the parser accepts them as args.
|
||
|
||
`page_cat_data` gathers `(cat_name, items, current_idx)` for page-axis categories,
|
||
deliberately using `categories_on(Axis::Page)` and **not** `filtering_cats()`:
|
||
`Filter` coordinates constrain the data but are not navigable, so `[` and `]` must
|
||
not page them (improvise-dz8, gotcha 27).
|
||
|
||
At bottom-right `EnterAdvance` has no place to go, so it emits a single `AbortChain`
|
||
— the advance premise no longer holds, and the caller's trailing effects (a
|
||
`CommitAndAdvance`'s `EnterEditAtCursor`) are skipped.
|
||
|
||
### `cmd/commit.rs` — the two commit paths (gotcha 19)
|
||
|
||
`commit_regular_cell_value` commits a cell value: for synthetic records keys it
|
||
stages in drill pending edits while in drill mode, or applies directly in plain
|
||
records mode; for real keys it writes to the model.
|
||
|
||
`rename_record_coordinate` renames one coordinate of a records row. In wide format a
|
||
row is a group of cells — one per measure — so the rename has to move **every** one
|
||
of them, not just the cell under the cursor (improvise-c5v, gotcha 1). Every cell in
|
||
the row moves, and a row seeded by `add-record-row` has no measure cells yet: its
|
||
coordinates live on a lone measure-less cell, which must move too or the row loses
|
||
its identity.
|
||
|
||
`add_measure`: typing a name into the trailing column creates that measure, which
|
||
gives it a real column to type values into. Adding a measure is a schema change, so
|
||
it applies immediately even inside a drill.
|
||
|
||
`add_category` (improvise-2zq): typing a name into the `+cat` column creates that
|
||
category, which gives the row a coordinate column to type into. **It creates the
|
||
column and does not set the coordinate on the current row** — the deliberate answer
|
||
to the ticket's third question, for two reasons. It is symmetric with the measure
|
||
`+`, which creates an empty column and writes no value. And setting it would move
|
||
*every* measure cell in the row onto the new coordinate (gotcha 1): a data migration
|
||
across N cells attached to the same Enter that made a schema change, in an app with
|
||
no undo. The follow-up edit is one `l` away in the column that just appeared. Like
|
||
`add_measure` it is a schema change, so it applies immediately rather than being
|
||
staged.
|
||
|
||
`inserts_a_column` asks whether committing `value` at `key` inserts a column,
|
||
invalidating every column index to its right. It is asked of the *layout* —
|
||
`record_col` resolves the synthetic `_Dim` position into what the column actually is
|
||
— rather than re-derived from the cursor, so there is one answer to "what is this
|
||
column" (gotcha 1). It takes the value because **an empty name creates nothing**:
|
||
`created_name` is the one place that rule lives, read by `add_measure`,
|
||
`add_category` and this predicate, so a commit that inserts no column cannot claim
|
||
to (improvise-0ygd).
|
||
|
||
`exit_mode_for` returns the normal-mode counterpart of an editing mode, used by
|
||
`CommitAndAdvance` to compute the mode to land in if the advance aborts.
|
||
|
||
**`CommitAndAdvance`** commits a cell edit, advances the cursor and re-enters edit
|
||
mode, subsuming the old `CommitCellEdit` (Down) and `CommitAndAdvanceRight` (Right).
|
||
`edit_mode` is the editing mode to re-enter after advancing, supplied by the keymap
|
||
binding — the editing-mode keymap passes `editing` and the records-editing keymap
|
||
passes `records-editing`. The command itself never inspects `ctx.mode`.
|
||
|
||
Two orderings inside it are load-bearing.
|
||
|
||
**The pre-emptive drop to normal goes first.** It drops to the normal counterpart of
|
||
`edit_mode`; if the advance succeeds the trailing `EnterEditAtCursor` lifts back
|
||
into editing on the new cell, and if the advance aborts (already at bottom-right on
|
||
Enter) that effect is skipped and the user lands in normal mode — the desired "Enter
|
||
at bottom-right commits and exits" behaviour. It goes **before** the commit, not
|
||
after (improvise-2zq): the commit can abort the batch itself — `AddCategory` does so
|
||
at `MAX_CATEGORIES` — and an abort before this ran left the app in the editing mode,
|
||
where `draw_bottom_bar` renders the minibuffer instead of the status line and the
|
||
refusal the effect had just written was never drawn. Tidy-up goes where an abort
|
||
cannot reach it; the same rule as `ExecuteCommand`'s (gotcha 21).
|
||
|
||
**A schema change that inserts a column suppresses the advance.** Inserting moves
|
||
every index to its right, and the advance is computed from `ctx` — the *pre-commit*
|
||
layout, because `apply_effects` rebuilds only after the whole batch (gotcha 23). On
|
||
the last row `Enter` wraps to `col + 1`, which after the insert is `+cat` again, so
|
||
the editor reopened category creation and a user naming two categories in a row
|
||
never reached the column they had made. `Tab` reached it the same way. Not advancing
|
||
is also the *useful* answer rather than merely the safe one: `add_category` inserts
|
||
the new column exactly where the cursor already is, so staying put lands on it.
|
||
|
||
**The measure `+` is the same case, and this paragraph said it was not** (corrected
|
||
by improvise-0ygd; see `context/test-evidence/command.md`). The claim was that
|
||
`+measure` has no column to its right for an index to shift into — false, because
|
||
the column that shifts is `+measure` itself: naming a measure inserts the new column
|
||
at the cursor's own index and pushes the `+` one to the right, exactly as `+cat`
|
||
does. The claim that "both advances are already special-cased at the bottom-right
|
||
edge" was worse than wrong, since that special case *is* the bug: `Tab` there ran
|
||
`AddRecordRow` and jumped to `(row + 1, 0)`, manufacturing a record row and leaving
|
||
the measure the user had just named unreachable, while `Enter` survived only because
|
||
`EnterAdvance` aborts at the bottom-right and the `SetSelected` restore above is what
|
||
then held the cursor. So `inserts_a_column` covers both creating columns and both
|
||
advances suppress.
|
||
|
||
What the special case is genuinely for is bulk entry, and the fix keeps it by
|
||
splitting on the **buffer** rather than the column. `+measure` is always the
|
||
rightmost column, so `cursor.col >= col_max` in records mode names that one cell and
|
||
no other — the "Tab at the bottom-right starts the next record" path is reachable
|
||
only from it, by tabbing off the end of a row onto the `+` and pressing Tab with
|
||
nothing typed. An empty commit creates no column, so it still starts the row; a
|
||
commit that names a measure does not.
|
||
|
||
**`CommitKind`** is what a minibuffer commit produces. The variants differ only in
|
||
the model mutation they build and in where they leave the user — reading the buffer,
|
||
trimming it, bailing out when it is empty, marking the model dirty and reporting are
|
||
shared by `CommitBuffer` (gotcha 20). `Item` carries its target category, resolved
|
||
from `AppMode::ItemAdd` when the command is constructed, so `execute` stays a pure
|
||
function of the kind and its buffer. `parse_commit_kind` resolves the argument
|
||
against the current mode: `Item` is the only kind that needs anything from the mode,
|
||
so a script line (which has no live mode) can build the other two.
|
||
|
||
`CommitKind::on_commit` says where a *successful* commit lands the user. `None`
|
||
keeps the minibuffer open for the next entry — categories and items are entered in
|
||
batches, formulas are not.
|
||
|
||
**`CommitBuffer`** commits the contents of a minibuffer as a formula, a category or
|
||
an item; buffer clearing is the keymap's (Enter → `[commit, clear-buffer]`). The add
|
||
effect goes **last** on purpose: it is the only party that knows whether it worked,
|
||
so the optimistic status must not clobber the error it reports (improvise-6t3).
|
||
|
||
**`CommitSave`** commits a save path from the save buffer. It is the sibling of
|
||
`CommitExport` and deliberately not the same command: the export prompt writes CSV,
|
||
this one writes the *model* through `SaveAs`, which also adopts the path so a later
|
||
plain `Save` has one (improvise-lae). An empty path bails rather than reaching
|
||
`SaveAs`, which would otherwise ask the storage backend to write to `""` and report
|
||
its error instead of the one the user can act on.
|
||
|
||
Its mode change comes **first**, and that ordering is the whole reason the user sees
|
||
a failed save (found by Copilot on PR #27). `SaveAs` aborts the batch when the write
|
||
fails (improvise-8tk's other arm), so anything after it is skipped. With the order
|
||
reversed the batch stopped while still in `SavePrompt` — and `draw_bottom_bar`
|
||
renders the minibuffer *instead of* the status line, so the "Save error:" set by the
|
||
effect was never drawn. Leading with the mode change means the error lands in
|
||
`Normal`, where the status bar is what the bottom row draws. It costs the typed
|
||
path, which the welded `clear-buffer` would have taken on a success anyway and which
|
||
`EnterMode` clears on the next Ctrl-W regardless (gotcha 11) — so nothing durable is
|
||
lost, and the convention that the side effect runs last and its message wins is the
|
||
one gotcha 20 already states.
|
||
|
||
### `cmd/mode.rs`
|
||
|
||
**`EnterMode`** clears the corresponding buffer when entering a text-entry mode,
|
||
through `write_buffer` — the only code that knows a runtime-named buffer may not be
|
||
an entry in the buffers map (improvise-gdl).
|
||
|
||
**`SaveAndQuit`** saves then quits; the `Quit` only lands if the `Save` succeeds,
|
||
because a failed save aborts the effect chain, so `wq`/`ZZ` leaves the user in the
|
||
app with the error visible rather than discarding the model (improvise-8tk).
|
||
|
||
**It looks like an `AAndB` command and is not one.** `wq` is the *user's* name for
|
||
the composition, not an invented internal one: it is registered, so `:wq` reaches
|
||
it, and `Quit`'s unsaved-changes message advertises that spelling. A keymap
|
||
`Binding::Sequence` cannot be reached from the `:` line, so replacing this with one
|
||
would either delete the ex-command or leave two independently-written spellings of
|
||
"save then quit". One definition behind two surfaces is what this already is. Pinned
|
||
by `text_buffer::tests::colon_wq_saves_then_quits`.
|
||
|
||
**`EditOrDrill`** is the smart dispatch for `i`/`a`: if the cursor is on an
|
||
aggregated pivot cell (categories on `Axis::None` and the cell is not a synthetic
|
||
records-mode row) it drills instead of editing; otherwise it pre-fills the edit
|
||
buffer with the displayed cell value and enters `edit_mode`. `edit_mode` is supplied
|
||
by the keymap binding, so the command itself is mode-agnostic. Only **regular**
|
||
(non-virtual, non-label) categories on None count as true aggregation: virtuals like
|
||
`_Index`/`_Dim` are always None in pivot mode and imply nothing.
|
||
|
||
`EditOrDrill::nothing_at_cursor` is what to say when there is no cell under the
|
||
cursor at all. improvise-ct5: it used to say "cannot drill — no cell at cursor" in
|
||
both modes. Drilling is a pivot-mode concept, and in records mode — where every
|
||
regular category sits on `Axis::None`, so this branch is the one an empty grid
|
||
always takes — the user's model is "a table I type into", with `o` one key away to
|
||
add a row. Naming drilling there described a mechanism the user is not using instead
|
||
of the key that would help. The choice reads `edit_mode`, the argument the *keymap*
|
||
supplies, rather than asking `ctx` what mode it is in, so the command stays as
|
||
mode-agnostic as it claims and no runtime `is_records()` check comes back
|
||
(improvise-4ju).
|
||
|
||
**`EnterEditAtCursorCmd`** is a thin command wrapper around the `EnterEditAtCursor`
|
||
effect so it can participate in `Binding::Sequence`; `target_mode` is the keymap
|
||
binding's argument.
|
||
|
||
### `cmd/grid.rs`
|
||
|
||
**`DrillIntoCell`** creates a `_Drill` view with `_Index` on Row and `_Dim` on
|
||
Column (records/long-format view); fixed coordinates from the drilled cell become
|
||
filters.
|
||
|
||
If drilling into a formula cell, the formula target is stripped from the key so
|
||
`matching_cells` finds the underlying raw data records instead of returning nothing
|
||
(gotcha 10).
|
||
|
||
Fixed coords go to **`Filter`**, an immutable constraint of this drill rather than a
|
||
dimension to page through (improvise-dz8, gotcha 27). Previously-aggregated
|
||
categories (`none_cats`) stay on `Axis::None` so they do not filter records; they
|
||
appear as columns in records mode. Virtual categories are skipped, `_Index`/`_Dim`
|
||
having been set already.
|
||
|
||
**`ToggleRecordsMode`** toggles between records and pivot mode using the view stack:
|
||
entering creates a `_Records` view and switches to it, leaving navigates back.
|
||
Leaving first cleans up any records with empty `CellKey`s — produced by
|
||
`AddRecordRow` when no page filters are set — which is the inverse of the `SortData`
|
||
that runs on entry.
|
||
|
||
### `cmd/panel.rs`
|
||
|
||
**`CyclePanelFocus`** tabs through open panels in order Formula → Category → View,
|
||
wrapping. The starting point is the panel currently focused (`ctx.mode`); from a
|
||
non-panel mode focus goes to the first open panel. With a single open panel it
|
||
self-transitions back to it; with none open it is a no-op.
|
||
|
||
**`DeleteCategoryAtCursor`** deletes the category or item at the panel cursor: on a
|
||
category header the whole category, on an item row just that item.
|
||
|
||
**`CreateAndSwitchView`** creates a new view, switches to it and returns to Normal.
|
||
**Not a shim, despite the name** (improvise-aaw). The new view's name is derived from
|
||
`ctx` — `format!("View {}", views.len() + 1)` — and a keymap `Binding::Sequence` can
|
||
only carry literal args, so the proposed
|
||
`[create-view X, switch-view X, enter-mode normal]` would hard-code one name and the
|
||
second `n` in the view panel would collide with the first. The derivation is the
|
||
decision that makes this a command.
|
||
|
||
The ticket's two fallbacks are both behaviour changes wanting a product decision,
|
||
not refactors: prompting for a name is a new feature, and making `create-view`
|
||
switch by default changes `:create-view` for scripts and duplicates what `grid.rs`
|
||
does by hand for `_Drill`/`_Records`.
|
||
|
||
### `cmd/registry.rs`
|
||
|
||
`parse_mode_name` decodes a mode-name string (as supplied by
|
||
`enter-mode`/`edit-or-drill` keymap bindings) into an `AppMode`. The table itself is
|
||
`AppMode::from_name`, beside `AppMode` in the protocol crate: the spellings are
|
||
keymap vocabulary and the keymap is shared, so the browser client decodes
|
||
`enter-mode` from the same list (gotcha 16).
|
||
|
||
The commit registrations take a mode-name arg (`"editing"`, `"records-editing"`) as
|
||
`args[0]`; the keymap supplies it and the command never inspects `ctx.mode`.
|
||
|
||
### `cmd/text_buffer.rs`
|
||
|
||
**`ExecuteCommand`** executes the `:` command buffer by delegating to the command
|
||
registry — the `:` prompt is just another front end to the scripting language, using
|
||
the same parser as `improvise script`.
|
||
|
||
Its batch **leads** with `ReturnToNormalUnlessMoved`, before anything that can fail
|
||
runs. The mode is still decided at *apply* time rather than up front, because
|
||
`ViewBack`, `ViewForward` and `StartImportWizard` move the mode only on some runs and
|
||
no up-front answer is right (improvise-toc, gotcha 31). Running it at the head does
|
||
not weaken that: whatever a later effect sets, it sets *after* this, so the mode a
|
||
`:` line ends in is unchanged in every case — the only difference is what happens
|
||
when the batch never reaches the end.
|
||
|
||
Which is the point. A failed `Save`/`SaveAs` sets `abort_effects` (gotcha 21), and an
|
||
abort at the tail ate this restoration too, stranding the user in `CommandMode` with
|
||
the stale command line drawn — where `draw_bottom_bar` renders the minibuffer
|
||
*instead of* the status line, so the error the effect had just set was invisible. The
|
||
abort must stop the *chain* (`:w bad . q` must not quit) and must not stop the
|
||
*bookkeeping*, and ordering answers that without giving any effect a "runs anyway"
|
||
flag: tidy-up goes where an abort cannot reach it.
|
||
|
||
### `cmd/tile.rs`
|
||
|
||
**`TileAxisOp`** cycles or sets the axis for the category at the tile cursor and
|
||
stays in `TileSelect` mode so the user can adjust several tiles. `axis: None` cycles;
|
||
`axis: Some(a)` sets.
|
||
|
||
### `cmd/effect_cmds.rs`
|
||
|
||
The parseable model-mutation commands are thin `Cmd` wrappers around effects,
|
||
constructible from string args, sharing the same execution path as
|
||
keymap-dispatched commands (gotcha 16).
|
||
|
||
### `cmd/wizard.rs` (improvise-2wm, improvise-907)
|
||
|
||
Import-wizard key handling. The wizard is a small step machine, and which key means
|
||
what depends on the step it is in. That decision is pure, so it lives here: the
|
||
command reads the wizard out of `CmdContext` and emits the discrete effects
|
||
(`WizardApply`, `CloseWizard`, `AcceptWizardModel`) that carry it out.
|
||
|
||
`done_keys` is the confirmation step: the workbook is built and waiting, Enter
|
||
installs it, and both Esc and Left return to `NameModel` — unlike every other step,
|
||
Esc here does not throw the import away.
|
||
|
||
`back_key`: Left steps back to the previous step, on every step. It is the one key
|
||
free everywhere — Backspace edits the name and the formula buffer, and `h` is a
|
||
printable character the name step needs.
|
||
|
||
---
|
||
|
||
## `command/cmd/fit.rs` — `fit-measure`
|
||
|
||
Evolve a decision tree over what the active view shows and store it as an ordinary
|
||
improvise formula. After this command there is nothing GP-specific left in the
|
||
document — the rule is a `## Formulas` line like any other, recomputed by the
|
||
fixed-point evaluator, editable in the formula panel, drillable, and saved with no
|
||
new grammar (improvise-21d's enabling fact, and improvise-7na's answer).
|
||
|
||
### The fit runs inside `execute`, and that is the architecture rather than a shortcut
|
||
|
||
A `Cmd` is pure over `&CmdContext` and returns effects; an `Effect` is the only
|
||
mutation site. A fit is **decision logic** — a pure function of
|
||
`(Model, View, target, seed, FitConfig)` that mutates nothing — so it belongs on the
|
||
command side, and what the effects carry is its *result*. Putting the search inside
|
||
an `Effect::apply` would have mixed an expensive stochastic computation into the one
|
||
place the app mutates, which is the split's whole point.
|
||
|
||
What this does **not** buy is asynchrony. The command runs on the thread that draws
|
||
the next frame, so a fit is bounded rather than cancellable: `FitRequest::default`
|
||
is a deliberately smaller search than the CLI's (improvise-vx3's AC 4 asks for
|
||
"cancellable **or** bounded"). Moving it off the main loop is a change to `draw.rs`'s
|
||
event loop, not to this file, and nothing here forecloses it — the fit is already a
|
||
pure function that could be handed to a worker and have its `FitOutcome` posted back
|
||
as the same effects.
|
||
|
||
### What a refusal is for
|
||
|
||
`improvise_gp::fit_measure` fails rather than storing something misleading — most
|
||
sharply when the champion splits on an encoding artefact like `Region=North`, which
|
||
is not an item of anyone's document, so every cell of the stored measure would be
|
||
`Error("ref:…")` (improvise-8xhy, gotcha 37d). This command is where a user actually
|
||
meets that restriction, so the refusal goes to the status line **whole**: it names
|
||
the features that could not be resolved, because "the fit failed" is not something a
|
||
user can act on and "it split on Region, which is a category rather than a measure"
|
||
is.
|
||
|
||
Both the success and the failure lines are flattened to one line: several refusals
|
||
interpolate a document name, and a name may legally contain a newline (Copilot on
|
||
PR #25). A failure is when a user is most reading the status bar, so it is the last
|
||
place to let one split. The report goes first and `AddFormula` last, so a formula the
|
||
emitter spelled but the parser will not take leaves *its* message on screen rather
|
||
than this command's optimistic one (gotcha 20).
|
||
|
||
### What registering it costs the browser, measured
|
||
|
||
`improvise-command` is in the **worker** bundle, and the path from `App::new` →
|
||
`default_keymaps` → `default_registry` → this command's prototype is live, so
|
||
`gp-tree` links into it whether or not a browser ever invokes `fit-measure`. `kb.pl`
|
||
names keeping GP out of the wasm bundle as `improvise-gp`'s reason to exist, so this
|
||
is a cost to state rather than to discover: built with `wasm-pack --release`,
|
||
`improvise_worker_server_bg.wasm` goes **1,341,130 → 1,420,470 bytes** (+79 KB,
|
||
+5.9%), or **498 → 531 KB gzipped** (+6.5%). `improvise-wasm-client` — the
|
||
main-thread bundle — does not depend on `improvise-command` at all and is
|
||
**unchanged**.
|
||
|
||
Accepted rather than feature-gated, for two reasons. The worker already carries the
|
||
whole `App`, persistence and the import wizard, so this is a 6% addition to something
|
||
an order of magnitude larger; and a browser fit is a *wanted* end state, not dead
|
||
weight — `improvise_gp::fit` exists where it does precisely so a second host can run
|
||
the same sequence. If the bundle ever needs shrinking, a default-on cargo feature
|
||
that `web/build.sh` turns off is the lever, and these are the numbers to beat.
|
||
|
||
### `parse_seed` and the seed argument
|
||
|
||
A seed as the status line spells it (`0x…`, either case) or in decimal. **The hex arm
|
||
is not a convenience**: `FitOutcome::status_line` prints `{:#x}` and tells the user
|
||
that is what reproduces the fit, so accepting only decimal made the advertised round
|
||
trip fail on the advertised value.
|
||
|
||
The seed is an argument rather than a clock read for the reason the CLI fixes its own
|
||
default: two fits of the same document should agree, and a user who wants a different
|
||
search asks for one (gotcha 37b).
|
||
|
||
## The active view is split (improvise-jytm)
|
||
|
||
`Workbook::active_view` is the document's **default** — what a freshly connected
|
||
client opens, and what `.improv` writes as `Initial View`. `ViewState::active_view`
|
||
is authoritative for the client that holds it. Two clients may sit on different
|
||
views of one document: the model is shared and the UI is not (improvise-g0f, user
|
||
ruling 2026-09-19).
|
||
|
||
The old guard, `the_two_active_view_names_never_diverge`, is *replaced* rather than
|
||
deleted, by `the_documents_default_view_outlives_this_clients_switches` and by
|
||
`a_fresh_session_opens_the_documents_default_view` — a deliberate split with no
|
||
guard is how this silently regresses.
|
||
|
||
### What the split moved
|
||
|
||
`App::switch_to_view` writes only `view_state.active_view`, and only for a view the
|
||
document has. **`App::resolve_view` is the one answer** to "what does this name name
|
||
in this document", falling back to the default so a slice naming a deleted view
|
||
cannot panic in `rebuild_layout`; `view()`, `view_mut()`, `view_name()` and
|
||
`project::grid_in` all go through it rather than each testing `views.contains_key`.
|
||
Two copies of that fallback would be a derivation done twice — they would agree
|
||
until one of them learned about `_Drill`. Every effect that used to write `workbook.active_view_mut()`
|
||
now writes `app.view_mut()` — the *View* is still document state shared by everyone
|
||
on it; what changed is which one this client means.
|
||
|
||
`effect::SwitchView`, `effect::ViewBack` and `effect::ViewForward` are therefore
|
||
`EffectKind::ViewOnly`. Gotcha 3 says a mis-declared kind is silent, so this was
|
||
re-derived rather than assumed: after the split each of the three writes
|
||
`view_state` and nothing else, and a `ViewOnly` effect is exactly one that reaches
|
||
no other client. **A sound derivation is not a guard**, though, and review found that
|
||
only `SwitchView` had one — putting either sibling back to `Model(LayoutGeometry)` was
|
||
invisible across the whole suite. `walking_the_view_history_moves_no_document_state`
|
||
closes that, and it asserts the *consequences* rather than the declaration because
|
||
those are what a reader can price: a mis-declared view-back raises `model_state.dirty`
|
||
(so the browser worker rewrites the whole OPFS document, gotchas 41 and 45) and bumps
|
||
`layout_version` (so every other client's next `Command::Model` is refused
|
||
`LayoutMoved`, ADR 001) for a move only one client made. That re-classification is also what takes `ViewBack`/`ViewForward`
|
||
off the wire, since `ModelCommand` is derived from the `Model(..)` effects — and it
|
||
is the right answer for a second reason, the one that removed `ApplyAndClearDrill`:
|
||
their operand is `ViewState::view_back_stack`, session-local residue belonging to
|
||
whoever navigated (gotchas 30, 38).
|
||
|
||
### What projects per client
|
||
|
||
Every projection is view-dependent except the formula list and the view list, so
|
||
`grid_in`, `category_tree_in` and `tile_bar_in` take the view and
|
||
`ClientSession::project_diff` passes its own. **All three need guarding, not just the
|
||
grid**: review found that only `grid_in` had a check, and pointing
|
||
`ClientSession::view_in` at the host's view left the whole suite green. The one that
|
||
matters most is the tile bar — a category's axis is what that widget *is*, so a client
|
||
on another view would be shown the wrong axis in the control it reassigns axes with,
|
||
beside a grid that was right. `grid_in` reuses `App::layout` when
|
||
the session names the view the host is on — which is the common case and the only
|
||
one that can carry a drill snapshot — and derives a `GridLayout` otherwise.
|
||
|
||
A session on a view that is *deleted* is the other half, and it is answered in
|
||
`adopt_deleted_views` rather than here: `project` can only describe a view, it
|
||
cannot move a client off one.
|
||
|
||
**Testing any of this needs a bystander that can be moved.** Both the refused-switch
|
||
and the deleted-view tests first sat their bystander on the document default, where a
|
||
relay addressed to everybody folds to a no-op — so the assertion read as coverage and
|
||
caught nothing, which review demonstrated by injecting `ToAll` twice. Both now put the
|
||
bystander on a view it can be moved *off*, and both read the audience off the
|
||
`Broadcast` itself rather than inferring it from folded state. The audience is the
|
||
message; that is what makes it the thing to assert.
|
||
|
||
A switch is `ViewOnly`, so `Dirty` cannot carry it. `ClientSession` therefore
|
||
**observes** its own move: `projected_view` records the view the mirror's grid was
|
||
built from, and a mismatch projects `Dirty::EVERYTHING` for that session alone.
|
||
This is the `cursor_epoch` shape (gotcha 24) — watch the state move rather than keep
|
||
a second table of which commands move it.
|
||
|
||
### Two gaps this opens, both filed rather than smuggled
|
||
|
||
**A view-scoped `ModelCommand` names no view** (improvise-lltt), and it is deferred
|
||
on a *condition*, not a judgement: it is unreachable only because no shipped client
|
||
emits `Command::SwitchView`, while the browser already emits `TransposeAxes`. `SetAxis`, `CycleAxis`,
|
||
`TogglePruneEmpty`, `SetNumberFormat` and their siblings act on "the active view",
|
||
which is now ambiguous: `apply_model` runs them against the `App`'s own slice, so a
|
||
*connected* client on another view moves the host's view rather than its own. That
|
||
was consistent before the split, because every client was on the same view. The
|
||
honest fix is for those variants to name their view, which is a protocol change.
|
||
|
||
**`Initial View` no longer follows the TUI user** (improvise-azhb). A switch moves no document state,
|
||
so opening a document, switching view and saving writes the view it was loaded with.
|
||
Whether a host's own switch should also set the document's default is a product
|
||
question, not something to decide inside this ticket.
|
||
## "What text is in this cell" has one answer, plus an overlay (improvise-tom1)
|
||
|
||
The user has ruled: **a remote client must not see another client's staged
|
||
drill edits.** They are uncommitted writes belonging to whoever staged them,
|
||
which is why gotcha 30 keeps `drill_state` off `ViewState` and why gotcha 38
|
||
took `ApplyAndClearDrill` off the wire. They become everyone's business at
|
||
`ApplyAndClearDrill`, which is `CellData` and already reaches every client. So
|
||
the invisibility is correct and was never the defect.
|
||
|
||
The defect was that three places answered the question. Two of them are now
|
||
one, and the third turns out to have stopped being an answer.
|
||
|
||
**`GridLayout::records_value` is the resolution.** It returns the `CellValue`,
|
||
and `records_display` is one `to_string` over it, so the text can never
|
||
disagree with the value it is a spelling of. `project::cell_display` used to
|
||
call `records_display` and then **guess** numeric-ness with
|
||
`text.parse::<f64>()` — the one place the producer guessed rather than knew,
|
||
and it was wrong in a way nothing caught: a coordinate column holds an item
|
||
*name*, so a `Year` category with items `2024`/`2025` projected a coordinate
|
||
the browser right-aligned as a number. It now reads the value and matches.
|
||
|
||
**A coordinate is `CellValue::Text` by construction**, not by a check, which is
|
||
why the fix is in `records_value` rather than in a guard at the producer: there
|
||
is no spelling of an item name that could route it to the numeric branch.
|
||
|
||
**`src/ui/grid.rs` was not the third answer.** Since gotcha 44 the TUI grid
|
||
takes its text off `GridCache` and then *overlays* `drill_state.pending_edits`
|
||
— that is the one derivation plus a session-local overlay, which is exactly
|
||
the shape the ruling wants, and `App::display_value_of` does the same for the
|
||
editor. The ticket's "three answers, two of which agree" was written before
|
||
anr9 landed and is worth correcting rather than repeating.
|
||
|
||
What must stay true, and is now asserted: the session that staged an edit sees
|
||
it through `display_value_of`, and `grid_of` projects the committed value to
|
||
everyone, **including that session's own grid**. The producer never reads
|
||
`drill_state`, and teaching it to would put one client's uncommitted typing on
|
||
another's screen.
|
||
|
||
## A bound key that builds nothing (improvise-55l5)
|
||
|
||
`KeymapSet::dispatch` returns `Option<Vec<Box<dyn Effect>>>`, and until this
|
||
ticket that option carried **two** facts under one shape: *no binding matched*,
|
||
and *a binding matched but its command would not construct*. `run` spelled the
|
||
second `registry.interactive(name, args, ctx).ok()?`, and `App::dispatch_key`
|
||
answers `None` with `Dirty::NOTHING` — so a bound key could be a black hole.
|
||
|
||
The interesting half is which errors that actually hid, because a fix that
|
||
surfaces errors nobody can act on is noise.
|
||
|
||
**The class that reaches a user is `no cell at cursor`.** Six registrations in
|
||
`cmd/registry.rs` read `ctx.cell_key()` and refuse without one: `clear-cell`,
|
||
`yank-cell`, `paste-cell`, `drill-into-cell`, `commit-cell-edit` and
|
||
`commit-and-advance-right`. It is *state-dependent*, so nothing checkable at
|
||
registration time can see it, and the two commit entries are the worst case —
|
||
the user is in `Editing`, has typed a value, presses Enter, and the batch is
|
||
discarded with the minibuffer still up (which is where `draw_bottom_bar` paints
|
||
instead of the status line, gotcha 21).
|
||
|
||
**Everything else the constructors can return is a keymap-authoring error**:
|
||
`Unknown command`, `require_args`, `Unknown mode` from `parse_mode_name`, and
|
||
`commit-buffer item requires item-add mode` — which is the repro the ticket
|
||
itself proposes. `every_bound_command_name_is_registered` already refuses all of
|
||
these at test time, checking *both* halves of the old expression precisely
|
||
because improvise-s13 made a typo'd mode name compile into a dead key
|
||
(gotcha 15). They are therefore unreachable from a shipped keymap, and they
|
||
surface now only because the mechanism is one rule rather than a list of
|
||
blessed errors — which is the right direction: a list would be a second
|
||
description of which failures matter, free to disagree with the first.
|
||
|
||
So the rule is: **`None` means no binding matched, and nothing else.** A
|
||
construction failure becomes `Some(vec![set_status(format!("{name}: {e}"))])`.
|
||
The command name is in the message because the two classes need different
|
||
readers — a user reads "no cell at cursor", a maintainer reads which binding
|
||
produced it — and one string serves both without a second channel.
|
||
|
||
A `Sequence` whose later step fails reports and runs **none** of the batch.
|
||
That is `try_fold`'s shape and it is not a new rule: gotcha 21's abort is about
|
||
effects that *ran*, this is about effects that were never built, and there is
|
||
nothing to roll back.
|
||
|
||
What this does **not** do, and should not: report an unbound key. `an_unbound_key_stays_silent`
|
||
is the control, and the injected rival that makes `lookup`'s `None` a status
|
||
fails it. Every unmapped character in every mode would otherwise write one.
|
||
|
||
## `add-record-row` names the row it lands on (improvise-o9uq)
|
||
|
||
`o` in records mode is `[add-record-row, enter-edit-at-cursor records-editing]`,
|
||
and `AddRecordRow::execute` returned `SetCell` plus a status and **never placed
|
||
the cursor**. The result was silent corruption of an unrelated record: appending
|
||
a row changes `view_frame`'s hash, the frame observation inside `apply_effects`
|
||
invalidates the cursor to the origin *mid-batch* (gotcha 24), and
|
||
`EnterEditAtCursor` then pre-fills the editor with row 0's category coordinate.
|
||
Enter commits through `rename_record_coordinate`, which by gotcha 1 moves every
|
||
measure cell of that row.
|
||
|
||
Two things are worth carrying past this fix.
|
||
|
||
**The order is the whole repair, and the acceptance criteria did not say so.**
|
||
"Append `viewport_effects` after its `SetCell`" is only correct because the
|
||
observation runs inside the loop; the same two effects in the other order are
|
||
the bug verbatim, and the two are indistinguishable by reading. The injected
|
||
rival for the regression test is therefore the *swap*, not the omission.
|
||
|
||
**`CommitAndAdvance`'s Tab path already did this**, and that is the tell to
|
||
generalise: one of two call sites placed the cursor and the other did not, so
|
||
the placement was a property of the *caller* where it is a property of the
|
||
command. It is now the command's, and the Tab site's own later
|
||
`viewport_effects` still wins because it is emitted after.
|
||
|
||
**Where it lands** is `row_count()` — an append is last — unless the record
|
||
already exists, in which case it is that record's index, found by scanning
|
||
`record_row(r).coords`. Predicting `row_count()` unconditionally would put the
|
||
cursor one past the end in that arm. That arm also stops claiming "Added new
|
||
record row" for a row it did not add; it is not made a *refusal*, because the
|
||
Tab path depends on seeding an empty-coordinate row and a refusal there would
|
||
break bulk entry.
|
||
|
||
**The deeper defect is untouched and filed.** `view_frame` hashes `entries.len()`
|
||
and every entry, so *any* append reframes and throws the cursor to the origin
|
||
although nothing was renumbered. Placing the cursor masks that at this call
|
||
site. Distinguishing "rows appended" from "rows renumbered" is a larger question
|
||
and does not belong in a P1 data-loss fix.
|