a7ec482552
Resolutions: - client.rs imports: EditorSession (main) and ModelCommand (#251). - client.rs refused(): #251 keeps the popped frame to learn which record it renamed; main (#279/3kfc) calls settle_sent() after the pop. Both kept, in the order pop, settle_sent, replay, then #251's Opened::Retry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
4078 lines
239 KiB
Markdown
4078 lines
239 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.
|
||
|
||
### `records_at_stake` — `records_named` plus a rename's destination
|
||
|
||
`ui/reduce.rs::records_at_stake(app, cmd)` is what `stale_basis`,
|
||
`stale_records_in_a_frame` and `caught_up` ask. It is `records_named(cmd)`, plus
|
||
one extra record for `RenameRecordCoordinate`: the **destination**, meaning the
|
||
source's coordinates with the category replaced by the new item. That record is
|
||
added only when it exists in the document and differs from the source
|
||
(improvise-milr). A rename onto an existing record overwrites it
|
||
(improvise-kndq), so without this a write another client made to the
|
||
destination was lost without any check.
|
||
|
||
**Why not in `records_named` itself,** which is what the ticket asked for.
|
||
`records_named` is pure and lives in `improvise-protocol`: `&ModelCommand ->
|
||
Vec<RecordHash>`, with no document. The destination is not in the command.
|
||
Deriving it needs the source's coordinates, which only the document holds,
|
||
and "when it exists" is also a question about the document. Moving
|
||
`records_named` behind an `&App` would take away what makes it useful to the
|
||
client. `BasisCache::claim` and `caught_up` rely on it being pure, so a client
|
||
can claim exactly what the server will check. So the protocol keeps the
|
||
command's own records, and the server adds what only the document can say.
|
||
|
||
The price is on the client side. It cannot compute the destination's hash (ADR
|
||
002: a client never computes a surrogate), so it claims every record counter it
|
||
held when the editor opened (`context/code/clients.md`, "A coordinate rename").
|
||
An existing destination the client was never shown is refused with
|
||
`RecordUnstated`. That is a refusal, not an overwrite, and it is the right
|
||
direction to err. `broken_chain` still asks `records_named`: whether a later
|
||
command's record was renamed away by an earlier one in the frame is a question
|
||
about the source only.
|
||
|
||
### `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.
|
||
|
||
### A model effect that cannot do what it names refuses: `App::refuse`, `on_view` and `refuse_and_stop` (improvise-bok4, cz8d)
|
||
|
||
`AddCategory` was the only wire-reachable model effect that aborted when it
|
||
failed. The others did one of two things:
|
||
|
||
- **Narrated.** `AddItem`, `AddItemInGroup`, `AddFormula`, `LoadModel` and
|
||
`ImportJsonHeadless` wrote a sentence to the *host's* `status_msg` and
|
||
carried on.
|
||
- **Swallowed.** Every view-addressed effect (`SetAxis`, `SetPageSelection`,
|
||
`ToggleGroup`, `HideItem`, `ShowItem`, `TransposeAxes`, `CycleAxis`,
|
||
`SetNumberFormat`, `TogglePruneEmpty`) did nothing when `view_mut_of` found
|
||
no such view. `DeleteView` discarded "cannot delete the last view" with
|
||
`let _`, and answered `Ok` for a view the document did not have
|
||
(improvise-40j4). `AddFormula` stored a formula for a category that was gone,
|
||
through `let _ = add_item(…)` (improvise-40j4).
|
||
|
||
Either way the batch ran to its end, and `Host::mutating_frame` reported the
|
||
frame **accepted**. That is clause 4's partial-and-silent shape, one layer below
|
||
the record tier that `broken_chain` guards. The reviewer measured two cases.
|
||
`[DeleteView("Scratch"), SetNumberFormat { view: "Scratch" }]` was accepted with
|
||
half of it a no-op. `[RemoveCategory("Region"), AddItem { category: "Region" }]`
|
||
was accepted too, and its reason went only to the host's own status line, so
|
||
the client was told nothing at all.
|
||
|
||
**One writer, and the abort is separate (improvise-cz8d).** improvise-c5ip
|
||
(PR #185) had meanwhile added `App::refuse`, which writes the reason, sets
|
||
`batch_refused` and does **not** abort. The combination is now:
|
||
|
||
- **`App::refuse` is the only writer** of a refusal. bok4's own `refused()`
|
||
helper, which aborted, is deleted.
|
||
- **`on_view(app, target, write)`** writes the named view, or calls
|
||
`App::refuse` naming the missing one.
|
||
- **`Host::mutating_frame` rejects on `last_batch_refused() ||
|
||
last_batch_aborted()`.** That is sound because each `ModelCommand` maps to a
|
||
one-effect batch, so either flag belongs to the mutation that raised it.
|
||
- **Aborting is an explicit extra step**, taken only where a later effect in a
|
||
key-path batch assumes the failed one happened. `refuse_and_stop` does it for
|
||
`LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and
|
||
`import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its
|
||
own inline abort (gotcha 21). The item and view effects, and `AddFormula`'s
|
||
missing-category arm, only refuse: the frame is rolled back by the refusal
|
||
alone. On the key path, a failed `AddItem` or view edit no longer stops the
|
||
rest of its batch, which is what it did before bok4.
|
||
- **`AddFormula`'s parse-error arm also stops (improvise-v4hg).** It is the one
|
||
item-or-formula failure a key-path batch was found to depend on. Enter in
|
||
`FormulaEdit` is `[SetStatus, AddFormula, ChangeMode(FormulaPanel),
|
||
ClearBuffer("formula")]`, and the last two assume the commit worked. Without
|
||
the stop, `Total = (` was reported and then the editor closed and the typed
|
||
text was cleared. With it, the user stays in `FormulaEdit` with the text, and
|
||
the error is drawn beside the prompt. `fit-measure` emits `AddFormula` last, so
|
||
there is nothing after it to stop. The missing-category arm is reachable only
|
||
from a wire frame or a `:` line naming another category, and nothing in either
|
||
depends on it, so it does not stop. PR #181's review searched for an
|
||
`AddItem` case the same way and found none, so `AddItem` does not stop either.
|
||
An abort is added where a later effect depends on the failed one, never for
|
||
symmetry.
|
||
- **The `|| last_batch_aborted()` arm is kept, and the invariant it guards is
|
||
pinned (improvise-we1a).** Every model effect that aborts today also refuses,
|
||
so deleting the arm left the workspace green: nothing tested it. There were
|
||
two ways to close that. One was to make an abort without a refusal
|
||
impossible to write. The other, taken here, was to test the invariant with
|
||
the arm left in place. `every_failing_model_effect_that_aborts_also_refuses`
|
||
applies each failing `ModelCommand`'s own effects to an `App`, bypassing
|
||
`Host`, and asserts it refused. It also pins which cases abort, so it cannot
|
||
pass on a table that aborts nothing. The wire table cannot hold this
|
||
invariant, because the arm itself rejects a frame that aborted without
|
||
refusing. With AddCategory's arm reverted to status-only, the wire table
|
||
stays green. The option not taken is right for today's rules and wrong for
|
||
the future:
|
||
- `AbortChain` aborts without refusing, correctly. It is `ViewOnly`, and it
|
||
stops a chain whose premise no longer holds, which is not an error. A
|
||
type-level rule of "every abort refuses" would first have to exempt it.
|
||
- Making it refuse would change which `:` lines the headless CLI reports as
|
||
failures.
|
||
- Keeping the raw `abort_effects` setter away from `Model` effects alone would
|
||
be a new visibility split, and today no effect needs it.
|
||
|
||
So the arm stays, as defence for a future `Model` effect that aborts on
|
||
something other than an error.
|
||
|
||
**The table alone did not hold the invariant (improvise-lasd).** It lists
|
||
failing commands, so an abort at a site it does not list got past it. With
|
||
`on_view`'s missing-view branch changed to abort without refusing, the
|
||
workspace stayed green, and only the arm caught it. So `App::apply_effects`
|
||
now carries the rule itself. When the abort flag is set after a `Model`
|
||
effect, a `debug_assert!` requires that the batch has refused. `ViewOnly`
|
||
effects are exempt, so `AbortChain` is untouched. That answers the objection
|
||
above: the rule is on `EffectKind`, not a new visibility split.
|
||
- **Coverage:** it covers every model effect that any test drives to an
|
||
abort, listed or not. Under the `on_view` rival it fails two existing
|
||
tests.
|
||
- **Where it stops:** it is a debug assertion, so release builds rely on the
|
||
arm. It reads the batch-wide `batch_refused`, so a model effect that aborts
|
||
without refusing after an earlier effect in the same batch has refused
|
||
still gets past it.
|
||
- **Why the table stays:** it drives every failing command, and so it
|
||
exercises the assertion.
|
||
|
||
In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame
|
||
is undone whole, and the client is told, which is the channel the narrated
|
||
reason never had.
|
||
|
||
**This takes the ticket's first option, scoped to the effects the wire can
|
||
reach.** Failure is *reported* by the effect that knows it failed. The second
|
||
option, growing `broken_chain` to ask whether every view, category and item
|
||
target still resolves, would have been a second description of which targets
|
||
each effect needs. That second copy would be free to disagree with the effect
|
||
itself, which is gotcha 24's reason for observing rather than predicting.
|
||
|
||
Consequences:
|
||
|
||
- **A failed load is refused**, where it used to be accepted with a host status
|
||
line. So `a_load_that_fails_is_not_a_rebase` now witnesses the failure by the
|
||
refusal, not by `status_msg`. The frame's rollback also rolls the host's
|
||
status line back.
|
||
- **Every refusal site has a test that fails when it is reverted** to a
|
||
status-only write (improvise-9fpl, improvise-0v8l). Two tables carry most of
|
||
them. `every_failing_model_command_refuses_its_whole_frame` covers the wire.
|
||
`every_refusal_site_refuses_its_failure_and_only_its_failure` covers the key
|
||
path and the `:` line, which is what the headless exit code reads. The wire
|
||
table cannot see the refusal half of a `refuse_and_stop` site, because
|
||
`Host::mutating_frame` rejects on `refused || aborted`, and the abort alone
|
||
suffices. So each `refuse_and_stop` site needs a key-path assertion for its
|
||
refusal as well as one for its abort. The abort is
|
||
`a_failed_load_or_import_does_not_run_the_quit_after_it` (improvise-514o).
|
||
Every `refuse_and_stop` site now has a row in
|
||
`every_refusal_site_refuses_its_failure_and_only_its_failure`. The last three
|
||
were added by improvise-we1a: `load`, `import-json absent.json`, and
|
||
`AddFormula`'s parse arm. Before that, only indirect assertions held them.
|
||
The quit test's "setup:" assertion held `load` and `import-json absent.json`.
|
||
`a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text`
|
||
held the parse arm (improvise-rtud). Those assertions remain.
|
||
Each case in that table has a succeeding twin, so a site that refused
|
||
unconditionally would fail too. That matters because these are exactly the
|
||
lines a merge conflict could silently drop.
|
||
- **Not covered:** a no-op *inside* a view that does resolve. Examples are
|
||
`SetAxis` on a category the view does not have, `HideItem` of an absent
|
||
item, and `RemoveItem`/`RemoveCategory`/`RemoveFormula` of something already
|
||
gone. The removals are idempotent by intent. The others are the remaining
|
||
instances of this class, and each needs its own `View` method to say whether
|
||
it changed anything.
|
||
|
||
### `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.
|
||
|
||
It opens nothing on a cell `i` declines, and says why (improvise-enz6). The
|
||
question is `cmd::core::uneditable`, the one `committable_cell_key` asks. A
|
||
fold or another aggregate, or a formula's result, gets its refusal text in
|
||
the status line. The mode stays where the batch put it. `CommitAndAdvance`
|
||
already dropped to normal at its head (gotcha 21), so the user lands on the
|
||
fold in normal mode. It is a status line and not `App::refuse`, because the
|
||
commit before it succeeded. A `commit-cell-edit` script line whose advance lands on a
|
||
fold must not report its write as refused.
|
||
|
||
### 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.
|
||
|
||
`ViewBack` and `ViewForward` have the same shape now (improvise-8nd9). They used to
|
||
push the current view onto the opposite stack **before** a switch they could not be
|
||
sure of, and were safe only because `ViewState::forget_view` purges both stacks when
|
||
a view is deleted — a guard living one level away from the hazard. improvise-lrrx
|
||
proved the guard incomplete: `App::install_workbook` (`LoadModel`,
|
||
`AcceptWizardModel`) cleared `cursors` and not the history, so `<` after a load
|
||
walked the **outgoing** document's frames, landing on a view of the same name in the
|
||
incoming one that this client had never visited, or on a refused switch that still
|
||
left a phantom frame behind. Both halves are fixed, so neither relies on the other:
|
||
`install_workbook` clears both stacks beside `cursors`, for the same reason, and the
|
||
two steps go through `stepped_to`, which switches, observes whether the document
|
||
**accepted** the view (`App::switch_to_view` now returns that), and only then
|
||
restores the frame's mode and hands back the frame just left. A frame the document
|
||
refuses is consumed and leaves nothing.
|
||
|
||
The observation is *accepted*, not *moved*, and that is the one place the sibling
|
||
shapes differ. `SwitchView` asks "did `active_view` change" because asking for the
|
||
view you are on is not a move. A history frame is a **place**, meaning a view and a
|
||
mode, so a live frame naming the view you are on still restores its mode. Such
|
||
frames exist: `forget_view` collapses `[A, B, A]` to `[A, A]`. The client's
|
||
`reduce_view::stepped` also steps onto them unconditionally. A "moved" test
|
||
would therefore drop a frame the client keeps, and the host and the client
|
||
would disagree. The first version of this fix made exactly that mistake.
|
||
`colon_view_back_respects_whether_history_exists` caught it, because its
|
||
fixture is such a frame. `reduce_view`'s `stepped` is unchanged, because the
|
||
client holds no `Workbook` to observe against. It is safe only while no client
|
||
stack holds a frame the document would refuse, which rests on three paths:
|
||
`ForgetView` purges a deleted view from every mirror, a load arrives as a fresh
|
||
`Snapshot`, and that snapshot's history starts **empty**. The third was false
|
||
until improvise-lxg0. `project.rs::fresh_view` used to start from
|
||
`ViewState::default()` and fold `SwitchView(active)`, which recorded a visit to
|
||
the literal `Default`. So every connected or rebased client carried a phantom
|
||
`{Default, Normal}` back-frame, whether or not the document had such a view.
|
||
It now builds the slice pointed at the document's active view and folds
|
||
nothing, as `App::with_storage` does. The guard is still one level away from
|
||
the hazard. A new path that seeds or replaces a client's history without
|
||
emptying it reopens the divergence, and nothing but
|
||
`a_fresh_session_has_no_history_to_step_back_through` and its rebase twin
|
||
would notice.
|
||
|
||
### `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.
|
||
|
||
### `RecordRewrite` — a row's edits composed, not replayed (improvise-uo2p)
|
||
|
||
A staged drill edit is a statement about the record **as the snapshot showed
|
||
it**. It is not a statement about the model as the previous edit left it. The
|
||
edits in one row do not commute when they are replayed one at a time. A rename
|
||
moves every cell in the row (gotcha 1). A measure edit replayed after it writes
|
||
the old key, which leaves a ghost record there and a stale value in the moved
|
||
record. A second rename replayed after the first finds nothing at the old keys,
|
||
so it is dropped. `ApplyAndClearDrill` replayed them in `HashMap` order, so the
|
||
outcome was different from one App to the next. That is gotcha 18's class of
|
||
bug, found in an effect rather than an index.
|
||
|
||
**Sorting is not the fix.** "Measures before renames" repairs the ticket's own
|
||
case and still drops a second rename every time (rival 1 in the evidence). Any
|
||
fixed order makes the corruption reproducible instead of removing it. So the
|
||
rule is **build each row's final record once, then write it**:
|
||
|
||
- `RecordRewrite::of(model, row, edits)` collects the row's renames and measure
|
||
edits into maps keyed by column, and reads each unedited value off the model
|
||
**before anything is written**. It then states the rewrite: clear every old
|
||
key, and write every cell at the key with *all* renames applied. A measure
|
||
edit that is staged replaces the carried value. The result does not depend on
|
||
the order of the edits. A drill stages at most one edit per `(row, column)`,
|
||
so no two edits in a row name the same column. A blank rename is refused, and
|
||
the row's other edits still land at the unrenamed coordinates, as before.
|
||
- `RecordRewrite::merged` concatenates the rows' rewrites **by phase**: every
|
||
refusal, then every item added, then every clear, then every write. The clears
|
||
come first so that a swap is a swap. Rows are taken in snapshot row order
|
||
(`BTreeMap`), so where two final records land on one cell, **the later row
|
||
wins**. That is the answer plain records mode gives for the same renames made
|
||
top to bottom.
|
||
|
||
The collision rule is a **choice**, and it still loses the earlier row's values
|
||
silently. Plain records mode loses them the same way when a rename lands on an
|
||
existing record. Refusing the colliding rows instead would be a change to both
|
||
paths.
|
||
|
||
**One copy.** The plain records path (`commit.rs::commit_plain_records_edit`)
|
||
calls the same `RecordRewrite::of` with its one edit, so `rename_record_coordinate`
|
||
and the drill's `apply_coordinate_rename`/`apply_measure_edit` are gone. This is
|
||
the consolidation improvise-fdk1 asked for, "regardless" of its wire half. For a
|
||
single edit the output is the effects the plain path used to emit, reordered from
|
||
`clear, set` per cell to all clears then all sets, which touches distinct keys.
|
||
One behaviour moved on the drill side: it used to discard `add_item`'s error, and
|
||
now goes through `effect::AddItem`, which refuses when the category has gone.
|
||
|
||
**A row naming a category the model no longer has is unwritable, and one
|
||
unwritable row makes the whole drill commit write nothing** (improvise-qz9p).
|
||
Another client's `RemoveCategory` can land while a drill is open. That removes the
|
||
category and every cell under it, and the drill's staged edits then address
|
||
coordinates nothing owns. The fix has two parts.
|
||
|
||
- **The check is structural, not a reaction to `AddItem` refusing.** The ticket
|
||
framed it as "should a failed `AddItem` drop the row's writes", but that misses
|
||
the plainer case. A **measure edit alone** on such a row stages no `AddItem`, so
|
||
nothing refuses, and it still writes an orphan cell keyed under the removed
|
||
category. `RecordRewrite::of` therefore asks the model whether every category
|
||
the row names still exists, both its coordinates and its renames (a sparse
|
||
row with no coordinate for a category it renames reaches the check only
|
||
through the renames; improvise-6r2y). If any is
|
||
gone, the row is `unwritable`: it refuses with `Unknown category '…'` and
|
||
carries no writes. This is ADR 001's "force never creates structure", applied
|
||
inside the drill.
|
||
- **Why the whole commit and not just that row.** The drill's contract is atomic
|
||
(`design-principles.md` §4: "`ApplyAndClearDrill` writes them all atomically …
|
||
No partial writes reach the model"). A per-row skip would commit the other
|
||
rows' edits and drop this one's, a partial write of exactly the kind that
|
||
contract rules out. So when any row is unwritable, `merged` writes nothing.
|
||
- **What the user is told (improvise-o9q7).** `merged` first kept every row's
|
||
refusals. Refusals are applied in row order and each one overwrites the
|
||
status line, so a later writable row's own refusal, such as a blank
|
||
coordinate, replaced the one that mattered. The user read "Record coordinates
|
||
cannot be empty" when in fact every staged edit had been thrown away. So when
|
||
any row is unwritable, `merged` keeps only the unwritable rows' reasons and
|
||
combines them into one refusal that says so:
|
||
`Unknown category 'Type': every staged edit was discarded`. Sibling refusals
|
||
are dropped because nothing those rows staged is written, so their reasons
|
||
describe edits that are no longer pending.
|
||
- **`refuse`, not `refuse_and_stop`.** This follows #181's rule (gotcha 21):
|
||
abort only where a later effect in the batch assumes the failed one happened.
|
||
The effect after `ApplyAndClearDrill` in `view-back`'s batch is `ViewBack`,
|
||
which assumes nothing about the writes. Stopping would also keep the user in a
|
||
drill they cannot fix, because the category cannot come back through it, so
|
||
every exit would fail again. The staged edits are discarded, as on cancel, and
|
||
the refusal says why.
|
||
|
||
A blank coordinate edit is a different kind of refusal. It is refused **alone**,
|
||
and the row's other edits still land, as before: that edit is invalid input, not
|
||
a missing structure.
|
||
|
||
**The wire half: `effect::RenameRecordCoordinate` (improvise-fdk1).** One effect
|
||
serves both doors. `model_effects` maps `ModelCommand::RenameRecordCoordinate` to
|
||
it, after resolving the surrogate. The plain records key path emits it for a
|
||
category column. It reads the record's row off the store at apply time
|
||
(`view::record_row`, which groups the same way the records layout does) and
|
||
applies `RecordRewrite::of` with the one edit. The key path used to call
|
||
`RecordRewrite::of` itself, with the layout's row. In plain records mode that
|
||
row is the store's, so behaviour is unchanged. What changes is that the key path
|
||
and the wire now take one door, so a host and a browser renaming the same record
|
||
cannot drift. The name matches the variant, as
|
||
`every_model_command_names_the_effect_of_the_same_name` requires. The collision
|
||
rule is kept (improvise-kndq): a rename onto an existing record still
|
||
overwrites it, on both paths.
|
||
|
||
**What `[RenameRecordCoordinate(r), SetCell(r)]` means depends on the host's
|
||
view, and that is filed, not fixed (improvise-8id2).** A surrogate names
|
||
coordinates, not an object. After the rename, `r` names the vacated coordinates.
|
||
On a **records** host the layout names positions, so `resolve_record` has no
|
||
fallback, `broken_chain` answers `ChainBroken(r)`, and clause 4 undoes the
|
||
rename too. That case is pinned by
|
||
`a_write_to_a_record_its_own_frame_renamed_is_a_broken_chain`. On a **pivot**
|
||
host the empty cell is still drawn, so `record_in_layout` resolves `r` and the
|
||
write re-creates a record at the old coordinates: measured, not pinned. The
|
||
shipped browser composes one rename per frame, so only a hand-built client sends
|
||
this pair. It is the same fallback as improvise-ciw8.
|
||
|
||
### `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. It is **not** the refusal flag: `AbortChain` aborts without
|
||
refusing and `AddItem` refuses without aborting, so "did the request happen" is
|
||
`App::last_batch_refused`, written only by `App::refuse` (`context/code/cli.md` §10).
|
||
|
||
### `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). It is also **not**
|
||
`CellValue`'s `Display`, which rounds to four places: both arms spell the value
|
||
with `CellValue::edit_text` (improvise-umi4, `core.md` §17). 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 totals left the walk (improvise-wm20).** A column's total used to be the
|
||
walk's running sum of its cells, which is wrong for everything except a stored
|
||
measure under Sum or Count. It gave a ratio column the sum of its rows' ratios,
|
||
an Avg measure the sum of its rows' means, and a text column `0.00`. The user's
|
||
rule is that **a column's total is its cell as if every drawn row were one
|
||
fold**. `GridLayout::column_total` asks `Model::evaluate_spanning` for the
|
||
column key with the row categories spanned, so a formula is evaluated over the
|
||
pooled rows (Margin = total Profit / total Revenue) and a stored measure folds
|
||
its records under its own aggregator. The user ruled on three cases:
|
||
|
||
- **Text** follows the unanimity rule (gotcha 7): the text if every row agrees,
|
||
otherwise the `mixed` error.
|
||
- **An error** in any row makes the total an error, which is what that rule
|
||
answers for an error among numbers. That holds for a **stored** error. A
|
||
formula column is evaluated over the pooled inputs, so a row's `div/0` is not
|
||
an input and does not propagate. Only the fold's own error does. Rulings (1)
|
||
and (2) disagree here: the column shows an error in one row and a clean
|
||
number under it. The user chose the **pooled ratio** (improvise-b5dp). Margin
|
||
over rows 25/30, 27/30 and -4/0 totals 48/60 = 0.80, not `div/0`.
|
||
- **With `_Measure` on the row axis** the strip is empty, because a column adds
|
||
different measures together. An empty strip already means "no totals"
|
||
(gotcha 33).
|
||
|
||
The coverage is the **drawn** rows, as a fold's is (improvise-f2oo): a hidden
|
||
row stays out, and so does a record that lacks a row coordinate. That
|
||
deliberately differs from `Axis::None`, which counts both. The browser and the
|
||
TUI agree by construction, since both draw the projected `GridCache.totals`.
|
||
|
||
The cost is one more evaluation per column, and it is not free. The same
|
||
`cargo bench --bench grid_projection` on the orchestrator's Mac (Apple M4 Max),
|
||
before and after:
|
||
|
||
| case | whole grid before | after | `col_widths` before | after |
|
||
|---|---|---|---|---|
|
||
| typical (400) | 0.86 ms | 1.24 ms | 0.53 ms | 0.92 ms |
|
||
| 18k | 114 ms | 177 ms | 75 ms | 142 ms |
|
||
| pathological (90,000) | 1.54 s | 3.49 s | 1.07 s | 1.96 s |
|
||
|
||
`col_widths` is the keypress path, and it pays too, because a total's text sizes
|
||
its column. The event loop draws once per key, so every key pays the fold twice:
|
||
once in `App::cmd_context` for `visible_cols`, and once in the draw's `grid_of`.
|
||
At 4.8k cells that took a held arrow key from about 31 ms to about 52 ms, past
|
||
the ~33 ms key-repeat interval (improvise-yb4b). The walk no longer carries a
|
||
sum, so the window-into-totals fault below cannot happen any more. The test
|
||
still guards the widths half.
|
||
|
||
**The fold scans instead of enumerating (improvise-yb4b).** The cost was not the
|
||
fold's arithmetic. It was *how the fold found its records*: `fold_stored` built
|
||
one `CellKey` per member, the cross product of every spanned category's covered
|
||
items (3,000 keys for an 18k column), and looked each one up. Now
|
||
`group_expansion` returns the expansion as data: a `base` key and the covered
|
||
items of each expanded category. `fold_stored` hands that to
|
||
`DataStore::spanned_values`, which narrows through the index and filters each
|
||
candidate by membership. In `Exact` mode a candidate must also carry no
|
||
coordinate beyond those, which is what `get(member)` answered. Only a formula
|
||
still enumerates members (`contexts_of`), since its references are evaluated
|
||
per context.
|
||
|
||
**How it narrows is a choice, and the first version got it wrong
|
||
(improvise-jq0m).** Narrowing by `base` alone is right for a column total, which
|
||
spans *every* item of each row category. It is wrong for a collapsed group,
|
||
which spans two or seven items of a category with hundreds: `base` drops the
|
||
grouped category entirely, so every fold cell scanned every record of the
|
||
column, and a grid of small groups went quadratic. The review measured 1000
|
||
days in pairs at 11–15.7 s a keypress, against 0.44–0.51 s before #239. So
|
||
`spanned_positions` now prices both narrowings off the index's own set sizes
|
||
and takes the cheaper: once by `base`, or once per covered item of the span
|
||
with the fewest items, each intersected with `base`. The per-item positions are
|
||
disjoint (a record has one item per category) and are sorted afterwards, so
|
||
both paths return the store's order. A `base` naming a name the store never
|
||
interned matches nothing on both paths.
|
||
|
||
The bench now carries the grouped cases, which it could not see before: Day in
|
||
collapsed groups on Row, with Product, and `_Measure` on Column. The machine is
|
||
an Apple M4 Max (16 cores, 64 GB) at load average 15–63. The figures are
|
||
keypress (`whole-grid + col_widths`), min-of-N, from two alternating rounds,
|
||
the second reversed. Each cell gives both rounds:
|
||
|
||
| case | before #239 (`5932ad74`) | `f668ddc9` | jq0m | jq0m ÷ before |
|
||
|---|---|---|---|---|
|
||
| 4.8k dense | 33.9 / 39.1 ms | 40.7 / 40.2 ms | 36.3 / 33.3 ms | 1.07 / 0.85 |
|
||
| 18k dense | 237 / 239 ms | 238 / 232 ms | 196 / 211 ms | 0.83 / 0.88 |
|
||
| 90k dense | 3.04 / 3.46 s | 3.24 / 2.70 s | 2.68 / 2.84 s | 0.88 / 0.82 |
|
||
| 364 days in weeks | 483 / 415 ms | 1.42 / 1.18 s | 356 / 399 ms | 0.74 / 0.96 |
|
||
| 364 days in pairs | 715 / 539 ms | 5.02 / 4.62 s | 471 / 518 ms | 0.66 / 0.96 |
|
||
| 1000 days in pairs | 406 / 399 ms | 10.8 / 20.2 s | 352 / 364 ms | 0.87 / 0.91 |
|
||
|
||
The grouped cases now come in **below** the pre-#239 figures. The old
|
||
per-member path looked up each member through a full `CellKey`, and the
|
||
per-item narrowing intersects index sets directly.
|
||
|
||
Two levers were **not** taken. Combining the walk's row values for decomposable
|
||
aggregators was rejected under §5 as a second derivation of the total. It is
|
||
also wrong wherever a formula owns a row-axis item: the row reads the formula,
|
||
while the spanning fold reads the store. Sharing one total between
|
||
`cmd_context` and the draw was also rejected: a key's effects run between the
|
||
two, so sharing needs an invalidation story. Dropping the totals from the widths
|
||
would make the two width computations disagree about `visible_cols`.
|
||
|
||
**A grid-wide fold memo (improvise-h6z1)** is the other half, and it answers a
|
||
different cost. It removes the repeated **formula** chains that a fold row and a
|
||
total re-derive, about 2x on a folded five-formula chain. It does **not** move
|
||
the table below. Those grids hold only stored measures, a stored fold is never
|
||
memoised, and each total reads its column's records once. That one read per
|
||
column is what the scan above made cheap.
|
||
|
||
**What it changes besides speed:** a stored fold's values now arrive in the
|
||
store's insertion order, not member order. That is the order gotcha 18 names as
|
||
the store's answer. Min, Max, Count and text unanimity are order-free. A Sum or
|
||
Avg over non-integral values may differ in the last bits.
|
||
|
||
Measured by alternating three builds of `cargo bench --bench grid_projection` on
|
||
an Apple M4 Max (16 cores, 64 GB) under heavy shared load (load average 30–90).
|
||
This is min-of-N, three rounds, with the order reversed in the third. The
|
||
keypress figure is `whole-grid + col_widths`. The millisecond columns are
|
||
ranges over the rounds. The ratio columns are each round's own pair, in round
|
||
order:
|
||
|
||
| cells | before #239 (`5932ad74`) | #239 (`9948aaf5`) | yb4b | #239 ÷ before | yb4b ÷ before |
|
||
|---|---|---|---|---|---|
|
||
| 400 | 1.36–1.77 ms | 2.36–2.76 ms | 1.69–1.82 ms | 2.03, 1.34, 1.78 | 1.24, 0.98, 1.25 |
|
||
| 1.2k | 4.1–5.2 ms | 7.2–8.2 ms | 5.0–5.3 ms | 2.00, 1.39, 1.85 | 1.23, 0.96, 1.25 |
|
||
| 2.4k | 10.8–13.6 ms | 18.4–21.5 ms | 12.0–13.9 ms | 1.98, 1.36, 1.91 | 1.18, 0.88, 1.29 |
|
||
| 4.8k | 32.8–41.8 ms | 55.4–64.1 ms | 34.5–42.0 ms | 1.77, 1.32, 1.92 | 1.15, 0.82, 1.26 |
|
||
| 9.6k | 102.8–121.2 ms | 190.7–209.6 ms | 107.2–140.0 ms | 2.04, 1.57, 1.92 | 1.36, 0.88, 1.20 |
|
||
| 18k | 192.9–208.3 ms | 338.6–448.0 ms | 230.4–273.3 ms | 2.32, 1.63, 1.72 | 1.42, 1.11, 1.23 |
|
||
| 90k | 2.68–3.26 s | 4.92–6.49 s | 3.18–4.00 s | 1.99, 1.84, 1.79 | 1.23, 1.47, 1.14 |
|
||
|
||
Read the ratio columns, not the milliseconds. Round 2's `before` ran at load
|
||
86 against about 31 for its `yb4b`, which is why its ratios fall below 1. The
|
||
two clean rounds put #239 at about 1.8–2.3× the pre-#239 keypress, and put
|
||
yb4b at about 1.15–1.4×. At 4.8k a held key is back under the ~33 ms repeat
|
||
interval in the quietest round (34.5 ms against 32.8 ms before).
|
||
|
||
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).
|
||
|
||
- **The open run at each level 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 header of every run still open above the first
|
||
in-window item is carried: one per level, never the closed ones, or the payload
|
||
grows with the groups a deep scroll passed. It used to be the single nearest
|
||
header, which lost `Upper` for a window opening inside `North/Q1`
|
||
(improvise-e265, `a_window_opening_inside_a_nested_run_carries_every_level`).
|
||
`AxisFrame::body_from` then declines to *paint* them, 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 open run at each level 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 every session on a reframed view, each addressed with
|
||
`Broadcast::To` (improvise-nynk), and the asker is not an exception: a transpose
|
||
invalidates its own position too. What an earlier version of this paragraph got
|
||
wrong is the claim that the asker "folded nothing optimistically". A
|
||
`Command::Model` is not something a client can apply to itself, but the **rest of
|
||
its frame** is, and a browser frame routinely places the cursor *after* the
|
||
mutation. The `+cat` commit is `[ChangeMode, AddCategory, SetSelected]`. The
|
||
mirror folds that in order: reset, then placed, which is exactly the order the TUI
|
||
applies the same batch in. The client folded all of it **before** hearing the
|
||
reset, so the relayed `ResetCursor` arrived last and threw the placement away.
|
||
The client sat at the origin while its mirror, and every projection built from
|
||
it, sat at the placement (improvise-wdmp).
|
||
|
||
So the host answers the asker in the order the mirror saw. Once the asker has been
|
||
told `ResetCursor`, `Host::mutating_frame` relays back to it every **later**
|
||
cursor-placing command of the same frame (`SetSelected`, `SetRowOffset`,
|
||
`SetColOffset`), ahead of that command's projections. All three are absolute, so
|
||
folding them twice is idempotent. Excluding the asker from the reset, which the
|
||
ticket proposed, was rejected. For a frame that places nothing after the reframe
|
||
it is gotcha 24's own "strictly worse than nothing": the mirror is reset and the
|
||
client never hears it. `a_frame_that_reframes_and_then_places_the_cursor_leaves_the_asker_placed`
|
||
runs both shapes. One residue is stated rather than handled: a frame that
|
||
reframes and then *switches views* before placing would reset the view the
|
||
client ends on rather than the one it reframed. No resolver composes that frame
|
||
today.
|
||
|
||
The echo has two limits, and each is asserted because each has a plausible
|
||
wrong neighbour (improvise-hv0r). **It goes to the asker alone.** A bystander on
|
||
the reframed view is owed the reset, but never the asker's placement, since
|
||
that would be another client moving where I am looking. **It covers every
|
||
placing kind**, not only `SetSelected`. A records `+` commit carries an offset
|
||
after its mutation, so an echo narrowed to the selection would leave the
|
||
client's offset reset and its mirror's set.
|
||
`a_reframing_frame_echoes_only_its_later_placements_and_only_to_the_asker`
|
||
asserts the exact relay list for each session. The wdmp test above cannot see
|
||
either rival.
|
||
|
||
The echo also needs the client to *keep* what it hears. A relay is applied to
|
||
the mirror at once, so the client's own rollback must not undo it: see
|
||
`context/code/clients.md`, "What the host *relays* is settled too"
|
||
(improvise-jpba).
|
||
|
||
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` and an optional `fallback` reached only for
|
||
argument lists `parse` refused, and `register_at_cursor` additionally has the
|
||
registry set the command's `CursorState` to the live cursor. No registration
|
||
writes any other field, so an accepted argument list is never overridden by
|
||
context (improvise-2bac, improvise-yf7p; the argument is `context/code/cli.md`
|
||
§8). `register_pure` is the case with 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.
|
||
|
||
`commit_plain_records_edit` hands a coordinate or measure edit to
|
||
`effect::RecordRewrite::of`, the one home it shares with the drill (improvise-uo2p;
|
||
see `RecordRewrite` above). It replaced `rename_record_coordinate`. In wide format a
|
||
row is a group of cells — one per measure — so a 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.
|
||
|
||
**The tile bar offers `_Index` and `_Dim`, and that is deliberate (improvise-jgzw).**
|
||
Every tile call site passes `Model::category_names()`, virtual categories included,
|
||
so tile cursor 0 is `_Index`. The ticket asked whether that was a leak. It is not,
|
||
for three reasons, each checked rather than argued:
|
||
|
||
- **It is a decision the user already made.** improvise-kos: "`_Dim` and `_Index`
|
||
should work as pivot axes outside Records/Drill view". A category that works as a
|
||
pivot axis is one the axis picker offers.
|
||
- **The tile bar is the only way out of a records layout that was saved.**
|
||
improvise-jgzw's "check what it does" found that hand-assigning `_Index` → Row and
|
||
`_Dim` → Column in a real view (tile bar or `:set-axis`) is **written** to the
|
||
`.improv`, because a real view persists every axis, and it reopens as records mode.
|
||
(`VIRTUAL_VIEW_NAMES`' "never-persisted `_Index`/`_Dim` on axes" is true only of
|
||
`_Records`/`_Drill`.) In that document, `R` (`toggle-records-mode`) emits
|
||
`ViewBack` on an empty back stack, so it leaves the layout as it was and sets the
|
||
status to "Pivot mode". That is filed as improvise-gj5p. Moving the two axes back through
|
||
the tile bar is what gets out. In the browser, which declines
|
||
`toggle-records-mode`, the tile bar is also the only way in, and improvise-bd9o
|
||
already made that route safe instead of closing it
|
||
(`the_tile_bar_route_into_records_mode_does_not_strand_the_user`).
|
||
- **What filtering would cost was measured.** With `View::tileable_cats` filtering
|
||
on `CategoryKind::is_index_or_dim`, that bd9o test and the TUI's
|
||
`renders_each_tile_with_its_axis_label` fail, as well as the two characterization
|
||
tests here.
|
||
|
||
**Where the tile cursor opens (improvise-oplp, improvise-bqm0).** It used to
|
||
open on tile 0, which is `_Index`, so one cycle key (`T Enter`, read as
|
||
"confirm", or `T r`) put `_Index` alone on Row and emptied a pivot grid (0 rows
|
||
on `web/demo.improv`). The browser worker saves every accepted model command, so
|
||
there the empty grid became the stored document at once.
|
||
|
||
The fix places the cursor and keeps the order. Both ends call
|
||
`improvise_protocol::opening_tile`, which opens on the best-ranked tile (the
|
||
leftmost on a tie): a Regular category first, then `_Measure`, then a Label, and
|
||
`_Index`/`_Dim` last. **Placing the cursor beat reordering the tiles** because
|
||
the order is itself a decided, tested output. The browser's click targets are
|
||
tile *indices*, so reordering would move every click position. And
|
||
`the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included`
|
||
pins the offered list, which is the characterization #192 made. Moving the cursor
|
||
changes only where one keypress lands.
|
||
|
||
**The ranking, and why it is an enum and not a bool.** `TileEntry` carries the
|
||
host's `CategoryKind`, because a client cannot tell `_Index` from a user category
|
||
without sniffing names (design-principles §2). #231 first carried
|
||
`regular: bool`, which answers only one question. It could not say what to open
|
||
on when a view has *no* Regular category, and so fell back to tile 0, `_Index`,
|
||
which is the original bug (improvise-bqm0, confirmed on a document holding only
|
||
`_Measure` data). The rank is an exhaustive `match`, so a new kind forces a
|
||
decision. `_Measure` is the fallback, not the primary: it is on an axis in every
|
||
shipped document, and `Enter` reads as "confirm" on the user's own dimension. It
|
||
ranks above a Label because a Label is a high-cardinality per-row field, and
|
||
cycling it onto Row would explode the grid rather than empty it. The fallback is
|
||
**not** always `_Measure` (improvise-h3cv). In the `_Drill` view `drill_steps`
|
||
puts every coordinate of the drilled key on `Axis::Filter`, `_Measure` included,
|
||
and `tileable_cats` drops Filter, so `_Measure` is not a tile there. A drill's
|
||
tiles are `_Index`, `_Dim` and whatever sits on None, so the Label rank and the
|
||
`_Index` fallback are live exactly in a drill: a Label opens if there is one,
|
||
and otherwise `_Index`, because nothing better is on offer.
|
||
`in_a_drill_tile_select_opens_on_a_label_not_on_index` and
|
||
`in_a_drill_with_nothing_better_tile_select_opens_on_index` drive both through
|
||
the TUI's own keys. They are not lockstep tests, because a browser declines `>`.
|
||
|
||
**The tests had to separate the rule from its neighbours** (improvise-52od). In
|
||
every early fixture the first Regular tile was also the first tile on Row, so a
|
||
rival `position(axis == Row)` passed the whole workspace. Records mode is where
|
||
they part: `_Index` holds Row there, and the rival cycles it, dropping the user
|
||
out of records mode. `tile_select_opens_on_a_regular_category_even_when_a_virtual_holds_row`
|
||
and `in_records_mode_tile_select_opens_on_a_regular_category_on_both_ends` pin
|
||
that; the evidence is in `context/test-evidence/protocol.md` and
|
||
`clients.md`.
|
||
|
||
**`_Measure` must stay tileable.** It is on an axis in every shipped document, so
|
||
`regular_category_names()`, the advice for *prompts*, is the wrong filter here.
|
||
`the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included`
|
||
names the whole offered list with `_Measure` on a column, and fails if either
|
||
decision changes.
|
||
|
||
### `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` (since improvise-uo2p,
|
||
`RecordRewrite::of`), 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.
|
||
|
||
## Paste lands on a record or nowhere (improvise-6p8q, improvise-l4mp)
|
||
|
||
`YankCell` and `PasteCell` carry a `CellKey` like every cell command, and in
|
||
records mode the cursor's key is the **synthetic** `(_Index, _Dim)` pair — a
|
||
row index and a column index (gotcha 1). Both commands used it as though it
|
||
named a cell. `yy` evaluated it and got `None` on every records cell; `p` wrote
|
||
a *new* cell keyed `_Dim=|3|, _Index=|0|`, which `format_md` then persisted into
|
||
`## Data`. One keystroke corrupted the saved document, and the browser, which
|
||
composes its paste against the projected `CellId::Record`, never had the bug.
|
||
|
||
**Yank reads what the grid draws.** A synthetic key is answered by
|
||
`GridLayout::records_value`, the one resolution of a records cell
|
||
(improvise-tom1), so a coordinate column yanks its item as text and a measure
|
||
column its stored value. That is also what the browser yanks, since
|
||
improvise-dsxq made it copy `CellDisplay::value`, which the projection fills
|
||
from the same call. One consequence is shared by both ends and is not decided
|
||
here: a blank — a creating column, or a measure the record lacks — yanks
|
||
`Text("")` where an empty pivot cell yanks `None`. Filtering it in the TUI alone
|
||
would be a second derivation, so it is left to the producer (improvise-s1lx).
|
||
|
||
**Paste writes only where a value is stored.** A synthetic key resolves through
|
||
`GridLayout::record_cell_key`; the coordinate columns and both creating columns
|
||
store no value, so the paste is declined out loud rather than guessed at. A
|
||
coordinate column is deliberately *not* a rename: renaming moves every measure
|
||
cell in the row, a different intent from `SetCell`, and the browser does not
|
||
offer it either (`project::cell_id` answers `None` there).
|
||
|
||
**Inside a drill the paste is declined, not staged.** Gotcha 19 asks the
|
||
records branch to follow the commit path, whose drill arm stages the edit — but
|
||
`pending_edits` holds *strings* and `CellValue`'s `Display` rounds a number to
|
||
four places, so staging a pasted `1.23456789` would commit `1.2346`. Writing
|
||
straight through would break the drill's atomicity. Declining is the only arm
|
||
that loses nothing; a typed staging map is the real fix if drill paste is
|
||
wanted.
|
||
|
||
**An aggregate is a key that leaves out a regular category the layout
|
||
aggregates over** (improvise-l4mp). That is asked of the *key*, not of the view,
|
||
so `paste Region=West _Measure=Revenue Product=Gadget` from a script still
|
||
writes where the current view happens to aggregate `Product` away: a key that
|
||
names every coordinate is a record whatever the view shows. For the cursor's
|
||
key the two questions agree, because a cursor key never carries a None-axis
|
||
category. The **collapsed-group fold** half of improvise-l4mp cannot be seen by
|
||
any question about the key alone, because the key carries the group's name as an
|
||
ordinary coordinate. It is answered at the cursor instead, by the next section.
|
||
**The first version of that section wrote a false sentence here**: that the
|
||
checks in `paste_into` were reached "only by a key a script spells out". The
|
||
cursor path handed `PasteCell` the resolved record key, and `paste_into` ran
|
||
`aggregates` over it **again**. That is a second derivation beside `cell_id`, and
|
||
it reads a record lacking a later category (every record, once `+cat` adds one)
|
||
as an aggregate, so `p` refused where gh/main had pasted and the browser still
|
||
wrote (improvise-q9eg). The distinction is now a type: `PasteAt::Resolved`
|
||
carries a key the door answered and is written as it stands, and
|
||
`PasteAt::Spelled` carries a key a script typed, which only its shape can vet.
|
||
Nothing is asked twice.
|
||
|
||
## The cursor's cell has three doors (improvise-gw18, lx7w, 54g1, yk2s)
|
||
|
||
**The lesson of improvise-gw18 is that guarding one command did not close the
|
||
path.** PR #176 taught `EditOrDrill` to refuse a collapsed group's fold, and `>`
|
||
still drilled into it. The drill was empty, and `o` inside it wrote
|
||
`Month=Q1, _Measure=Revenue`, a coordinate that is not an item, into `## Data`.
|
||
The same shape turned up three more times, each in a command nobody had
|
||
listed. `p` wrote onto the fold (the fold half of improvise-l4mp). `x` in records
|
||
mode cleared the synthetic `(_Index, _Dim)` key, a silent no-op, while the
|
||
browser cleared the real record (improvise-lx7w). `p` onto a formula's cell
|
||
stored a value under the formula that survived in `## Data`, while the browser
|
||
declined (improvise-54g1). Each fix was one more guard in one more command. So
|
||
the answer is the **accessor**, not the commands: a command gets the cursor's
|
||
cell through a door that matches what it does with it.
|
||
|
||
Every command that takes the cursor's cell, and its door:
|
||
|
||
| command | door | on a fold | on a formula's cell | in records mode |
|
||
|---|---|---|---|---|
|
||
| `yank` | `cell_key` (reads) | yanks the fold's value | yanks the result | `YankCell` resolves the synthetic key |
|
||
| `paste`, `clear-cell` | `value_cell_key` | refused | refused | resolved to the real record; a coordinate or creating column and a drill are refused |
|
||
| `commit-cell-edit`, `commit-and-advance-right` | `committable_cell_key` | refused | refused | the synthetic key, which `commit_cell_value` resolves itself (gotcha 19) |
|
||
| `drill-into-cell` | `unfolded_cell_key` | refused | drills | unchanged |
|
||
| `edit-or-drill` | its own check, `folds_a_group` then `owning_formula` | refused | refused | unchanged |
|
||
| `hide-selected-row-item` (`H`) | `GridLayout::row_folds_a_group` | refused | n/a (hides a row item) | n/a |
|
||
| `add-record-row` (`o`) | none, it takes no cell | n/a | n/a | closed by the drill door: no drill of a fold exists to add a row in |
|
||
|
||
**`value_cell_key` asks the browser's own question.** It calls
|
||
`project::cell_id`, the function that fills `CellDisplay::id` for the browser
|
||
(ADR 002), and writes only where the answer is `CellId::Record`. The browser's
|
||
`record_cell_at_cursor` is that same match on the projected id. The two ends
|
||
therefore agree because they read one answer, not because two derivations
|
||
happen to match. `tests/lockstep.rs` holds them together for all three tickets.
|
||
`cell_id` took `&App` only to reach the model, so it now takes `&Model`. That is
|
||
what let `CmdContext` ask it.
|
||
|
||
**Commit has a door of its own**, and not `value_cell_key`, because commit is
|
||
the one writer that must see the synthetic key: renaming a coordinate column,
|
||
creating a column and staging a drill edit all resolve the records position
|
||
inside `commit_cell_value`. Its door refuses the two arms `i` refuses, an
|
||
aggregate (a fold included) and a formula's result. It needed one because
|
||
`Enter` and `Tab` advance the editor, and `EnterEditAtCursor` re-opened it at the
|
||
new cursor with no guard. `i` declines to edit `Profit`, yet `Tab` from `Cost`
|
||
landed the editor on it, and the next commit stored a value under the formula.
|
||
That is a fourth door nobody had listed. **The entry half is closed too**
|
||
(improvise-enz6). The door's two arms are now one function,
|
||
`core::uneditable`, and `EnterEditAtCursor` asks it before opening. So the
|
||
re-opened editor and the commit that would follow it cannot disagree about which
|
||
cells they decline. The commit door stays: a `:commit-cell-edit` line runs it at
|
||
the cursor with no editor opened there. Since the advance no longer reaches it,
|
||
the door has tests of its own that seat the editor directly
|
||
(`an_editor_already_open_on_a_fold_commits_nothing_there` and its formula twin).
|
||
|
||
**Drill keeps the narrowest door** because drilling into an aggregate is its
|
||
purpose. Only a fold is refused, since no record carries a group's name. The
|
||
ticket's alternative, expanding the fold into its members' records, would be the
|
||
more useful drill and is not built.
|
||
|
||
**A text-path key is not covered.** `paste Month=Q1 _Measure=Revenue` typed as
|
||
coordinates still reaches `PasteCell` with a group name. The doors are about the
|
||
**cursor**, and a key a user spells out is a claim about the document that no
|
||
layout checked. The same is true of `set-cell`.
|
||
|
||
## A command is a function of its context — the determinism law (improvise-hzjo)
|
||
|
||
`cmd/determinism.rs` holds the law the old `law_toggle_group_involution` claimed
|
||
and could not state. For **every** registered command and every argument list
|
||
that builds it, the effects `execute` returns must be equal, by `Debug` text,
|
||
across contexts built **independently from equal data**: four `App`s, each
|
||
parsed from `web/demo.improv` and given the same buffers, expanded categories,
|
||
yank, panels and cursor.
|
||
|
||
**Why the instances must be distinct.** The old test called `execute` twice
|
||
against one `CmdContext`. Every `HashMap` it reached was the same instance with
|
||
the same `RandomState`, so it iterated the same way both times: `f(x) == f(x)`,
|
||
which nothing can fail. Separately built `App`s give every map its own seed. So a
|
||
command whose effects follow hash iteration order disagrees with itself. Hash
|
||
order is the one kind of nondeterminism this repo has actually shipped (gotcha
|
||
18). This law can see it where a command's `execute` builds it, as the drill
|
||
snapshot's record order was. It cannot see the other shipped instance, the
|
||
analyzer's `HashSet`: `analyze_records` is reached only from inside
|
||
`Effect::apply`, which is outside this law (see below). The companion test
|
||
`only_distinct_instances_can_see_a_command_that_follows_hash_order` keeps that
|
||
property checked: a test-only command that emits `ctx.buffers` in iteration
|
||
order must agree with itself on one context and disagree across two.
|
||
|
||
**Four instances, not two.** A rival that takes *the first* entry of a map, not
|
||
all of them, agrees across two seeds whenever both happen to put the same entry
|
||
first. With 21 buffers that is roughly one chance in 21, and three comparisons
|
||
make it negligible. The fixture's values are also chosen to stay distinct under
|
||
the edits commands make. A first version used `v0`…`v15`, which `pop()` collapses
|
||
to `v`, and so it let `command-mode-backspace` reading an arbitrary buffer pass.
|
||
|
||
**The store needs several records to order, so the drills come from the
|
||
document (improvise-e1sm).** The registry's `candidate_args` are extended with
|
||
names the demo really has, because coordinates that match nothing never reach
|
||
the records a drill or a fit would order. A hand-picked name is not enough. The
|
||
first version's only real drill was `Product/Latte, Quarter/Q2`: one record, two
|
||
cells. With the store's sort removed, two contexts put those two cells in the
|
||
same order about half the time, and with three comparisons the law passed about
|
||
one run in eight (measured: 46 of 400). So the arguments now include **every
|
||
item of every regular category as a one-coordinate drill**, read off the parsed
|
||
document. A one-coordinate drill spans every other category whole, so it matches
|
||
the whole slice of records under that item, not one record picked by name. In
|
||
the demo that is four records and eight cells for each of the eight drills. With
|
||
the store's sort removed, the law fails 400 of 400 runs.
|
||
|
||
**The width guard measures what the comparison compared (improvise-b9f2,
|
||
improvise-btyw).** The test asserts that some `drill-into-cell` the comparison
|
||
actually built and compared froze at least four records. For each one it applies
|
||
those very effects to one scratch `App` and counts the records in its
|
||
`drill_state`. It counts them only when their `Debug` text, which carries their
|
||
order, appears in the text the comparison compared. The comparison reads `Debug`
|
||
while the scratch `App` reads the applied state, and those are two
|
||
representations. The review showed they can disagree: give `StartDrill` a
|
||
`Debug` that prints only a row count, and the frozen records are still four
|
||
while the compared text no longer holds their order. With the store's sort
|
||
removed the law then passed 300 of 300 runs, with the guard green. Requiring
|
||
the ordered records in the compared text ties the guard to what the comparison
|
||
could actually see. A first
|
||
version measured what the *argument lists* match in the store instead. That was
|
||
a parallel derivation, and the review beat it with a `drill-into-cell` parser
|
||
that refuses fewer than two arguments. Every one-coordinate drill then went to
|
||
the one-record cursor fallback, the argument lists still matched four records,
|
||
and with the store's sort removed the law passed 38 of 300 runs.
|
||
|
||
Counting frozen records is not enough by itself. A drill over no coordinates
|
||
(any candidate with no `Cat/Item` argument, such as `["1", "1"]`) freezes
|
||
**every** record, and it reads them through `iter_cells` in insertion order,
|
||
never through the narrowing index where gotcha 18 lives. Under the parser rival
|
||
that whole-store drill still froze 16 records, and a guard counting it stayed
|
||
silent. So only a drill that **narrowed** the store counts: one that froze fewer
|
||
records than the store holds. With the parser rival in place, the guard fires on
|
||
every run ("froze 1 records"), and it fires on every run under the `Debug` rival
|
||
too ("froze 0 records").
|
||
|
||
What the guard still cannot see is a narrowing drill whose records are frozen in
|
||
an order that does not come from the store. It checks that the law had a wide
|
||
drill to compare, not why the drill's order would differ.
|
||
|
||
**What it cannot see.** Only commands and arguments the fixture reaches. A
|
||
command whose nondeterminism needs some other state, such as records mode, a
|
||
wizard or a drill in progress, is outside this fixture. And `Debug` equality is
|
||
text equality: two effects that differ only in something their `Debug` omits are
|
||
equal here.
|
||
|
||
**Its scope is what `execute` returns, not what `Effect::apply` does.** The law
|
||
compares the effects a command hands back. Many effects are unit structs that
|
||
decide all their work inside `apply`: `ApplyAndClearDrill`,
|
||
`SortData`, `CleanEmptyRecords`, `Save`, `RebuildLayout`, `AcceptWizardModel`.
|
||
Hash-order iteration inside an `apply` is therefore invisible here, however
|
||
often the law runs. improvise-uo2p is the worked example.
|
||
`ApplyAndClearDrill::apply` walks `drill.pending_edits`, a `HashMap`, in hash
|
||
order. A measure edit and a category rename staged on one row can then write a
|
||
ghost record. The command that emits it passes this law, because the effect's
|
||
`Debug` is just its name. So "the one kind of nondeterminism this repo has
|
||
shipped" is guarded here only on the `execute` side. The `apply` side needs its
|
||
own test per effect, or a law over `App` state after `apply_effects`.
|
||
|
||
**The exemption list.** `OUTSIDE_THE_CONTEXT` names commands that legitimately
|
||
depend on something outside the context, such as a clock or an entropy source,
|
||
each with its reason. It is empty: `fit-measure`'s seed is an argument with a
|
||
fixed default, not a clock read. The list is checked in both directions. An
|
||
unlisted command that diverges fails the test, and so does a listed one that
|
||
does not diverge. A stale entry therefore cannot linger, and a new registration
|
||
is covered the day it lands.
|
||
|
||
## Editing a formula: the capture is on the mode, read in `execute` (improvise-km71)
|
||
|
||
`edit-formula-at-cursor` (Enter or `e` in the formula panel) opens
|
||
`AppMode::FormulaEdit` with `slot: FormulaSlot::Existing { id, was }` and seeds
|
||
the `formula` buffer with the formula's `raw`. That slot is the capture: which
|
||
formula, and what it read when the editor opened. Nothing at commit time looks
|
||
at the cursor again.
|
||
|
||
**Where `commit-buffer formula` reads the slot matters, and the registry is the
|
||
wrong place.** The obvious move was to resolve it in `parse_commit_kind`, the
|
||
way `Item` takes its category from `AppMode::ItemAdd`. That does not work, and
|
||
it fails silently: the interactive arm is built **from `parse`**
|
||
(`context/code/cli.md` §8), and `parse` accepts `formula` under
|
||
`AppMode::Normal`, so the mode-reading fallback is never reached. `Item` only
|
||
works that way because `parse` *refuses* `item`. The first build of km71 did
|
||
exactly this, and every edit became an add. `add_formula` upserts by target, so
|
||
an edit that kept its target still landed in place, and only the rename tests
|
||
and the concurrency tests showed it. Making the interactive arm read the mode
|
||
first would break improvise-yf7p's guarantee that an accepted argument list is
|
||
never overridden by context. So `CommitBuffer::execute` asks
|
||
`CommitKind::in_mode(ctx.mode)`. That is a read of context in `execute`, which
|
||
is the answer yf7p gave to `toggle-panel-visibility`. A script line stays an
|
||
add, and cannot name an edit, because the slot does not exist outside the
|
||
editor.
|
||
|
||
**`effect::ReplaceFormula` refuses with `refuse_and_stop`, not `refuse`.** The
|
||
Enter binding is `[commit-buffer formula, clear-buffer formula]`, and
|
||
`CommitBuffer` puts the change back to `FormulaPanel` *after* the mutation. A
|
||
refusal that stops the batch therefore leaves the editor open with the user's
|
||
text and the reason in the minibuffer line (`draw_bottom_bar` draws the status
|
||
beside the minibuffer). That covers all four refusals: the formula is gone, it
|
||
was rewritten since the editor opened (`was`), the new text does not parse, or
|
||
the new target belongs to another formula, or the rename is of a formula
|
||
outside `_Measure` (improvise-kb1d, below). `AddFormula`'s parse error stops the
|
||
batch the same way since improvise-v4hg (#213), so a bad *new* formula also
|
||
leaves the editor open with its text; adding and editing now agree.
|
||
|
||
**A target change outside `_Measure` is refused (improvise-kb1d).** A regular
|
||
category's formula target is a real item of that category, not a derived one.
|
||
`replace_formula` added the new target item and left the old one, so renaming
|
||
`Total = East + West [Region]` to `Sum` left `Total` as an empty item and
|
||
appended `Sum` at the end. Renaming the item in place was the alternative and
|
||
was not taken: an item's name is its identity in cells, views, hidden lists and
|
||
groups, so that rename is the whole of improvise-sboz, not a small fix. Editing
|
||
such a formula without changing its target still works.
|
||
|
||
On success the command's optimistic "Formula updated" stands, except on a
|
||
rename, where the effect rewrites the status to name the old and new targets
|
||
and any formula that still mentions the old one (`context/code/core.md` §18).
|
||
|
||
## A wire write reaches only a cell the host projects as a Record (improvise-ciw8)
|
||
|
||
`App::resolve_record` answers "which record does this surrogate name". Its
|
||
fallback, `record_in_layout`, accepts any key the current layout names, and that
|
||
includes a collapsed group's fold and a key the None axis aggregates over. It
|
||
has to stay that permissive: it is also the basis check's question, and a record
|
||
nobody has stored yet (the empty cell a client is about to fill) is found only
|
||
there. But resolving is not the same as being writable. A hand-rolled frame
|
||
`SetCell { record: RecordHash::of({Month=Q2}), .. }` was answered `Accepted` and
|
||
stored a cell at `Month=Q2`, while the host's own projection called that
|
||
position `CellId::Aggregate`. A write under a formula (`Derived`) was accepted
|
||
the same way.
|
||
|
||
So `model_effects` asks `App::unwritable` before a wire `SetCell` or `ClearCell`
|
||
builds its effect, and refuses through `effect::refuse`. That is the #181 path:
|
||
the refusal marks the batch, `Host::mutating_frame` undoes the frame, the asker
|
||
gets `Rejected(MutationRefused)`, and the reason is on the host's status line.
|
||
The question it asks is the projection's own, never a second derivation of it:
|
||
|
||
- A coordinate that names **no item of its category** is refused, in any view
|
||
(improvise-b114, then improvise-nacd). This rule asks the document, not the
|
||
layout, and runs first, because a fold in the measure half hides from both
|
||
rules below.
|
||
|
||
**It is stated positively, and that is the point of the nacd change.** The
|
||
b114 rule was negative: refuse a name that *is a group*. The invariant it
|
||
protects (improvise-gw18) is positive: every coordinate names an item. A
|
||
negative rule covers only the ways of not being an item that someone listed,
|
||
and review found two more within a day. `Bogus` is no item and no group. A
|
||
parent group such as `Totals`, above `Money`, holds no item directly, so
|
||
`items_in_group("Totals")` is empty and the group test missed it. Both wrote
|
||
`{Month=Feb, _Measure=…}` onto a stored record and were accepted. The
|
||
positive rule covers both, and any shape not yet thought of, because it asks
|
||
whether the name *is* an item rather than what else it might be.
|
||
|
||
"Item" means `Model::effective_item_names`: the category's items, plus the
|
||
formula targets for `_Measure`. The formula targets are included so that a
|
||
write under a formula passes this rule and is refused by the next one with
|
||
the reason that is actually true ("computed by a formula"), not as a non-item.
|
||
The same-name case falls out: an item called `Money` beside a group called
|
||
`Money` is an item, and is writable (improvise-pr8t). The b114 rule needed an
|
||
explicit exemption for that case, and nothing tested the exemption.
|
||
|
||
A group has no cell of its own, so writing one stores a non-item
|
||
coordinate. Take
|
||
`{Month=Feb, _Measure=Money}`, where `Money` groups Revenue and Cost. Its
|
||
record `{Month=Feb}` is stored, so the second rule below accepts it and never
|
||
asks the projection. And with `Money` expanded the host draws no such cell at
|
||
all, so the third rule has no position to ask. A fold on the row axis is
|
||
refused here too, so the `Aggregate` arm below now guards only the None-axis
|
||
aggregate. It also closes improvise-xur5: a fold's record `{Month=Q2}` written
|
||
with a measure the host draws nowhere (hidden, unknown or absent) has no
|
||
position for the third rule to ask, and was accepted.
|
||
- A cell a formula owns is refused. That is `cell_id`'s own `Derived` rule,
|
||
`formula_for(&key)`. It is asked directly because a stored record may sit on
|
||
no position in the host's current view.
|
||
- A record found **in the store** is a record, whatever view anyone is on.
|
||
`RecordHash` is document-scoped (gotcha 44).
|
||
- A record found **only through the layout** is asked of `project::cell_id` at
|
||
the position of **its cell**: the one whose key is the whole key the write
|
||
names, record and measure together. `Aggregate`, a fold or a sum over a
|
||
None-axis category, is refused. It must be the whole key, not the first
|
||
position carrying the record (improvise-sjdb). `RecordHash` drops `_Measure`,
|
||
so every column of a row carries the same record, and with a collapsed
|
||
`_Measure` group the first of them can be the fold. That refused the empty
|
||
`Units` cell beside a `Money` fold, which the host itself projects as a
|
||
`Record`. A write whose whole key the host's grid does not draw is not
|
||
refused here, as it was not before this rule existed.
|
||
|
||
**The projection asks the first rule too (improvise-gk51).** The non-item rule
|
||
asked the document, while `project::cell_id` did not ask at all. In records mode
|
||
every measure cell of a stored record was a `Record`, including a record whose
|
||
`Region=Mars` names no item. `set-cell`, an undeclared `## Data` item and a drill
|
||
commit after an item was removed all make such records. So the host offered a
|
||
write it then refused. The two ends now share one predicate,
|
||
`Model::first_non_item`: `App::non_item_refusal` refuses with it, and
|
||
`cell_id` declines with it, in both branches. The pivot branch needs it as well.
|
||
A page selection naming no item (`set_page_selection` does not validate) puts
|
||
the stale item into every cell's key, and that too was a `Record` the host
|
||
refused.
|
||
|
||
Such a cell projects as a new kind, **`CellId::NonItem { category, item }`**,
|
||
and not as one of the existing ones. Each existing kind would lie about it:
|
||
|
||
- `Aggregate` invites a drill, and its refusal says the cell "combines several
|
||
records". This is one stored cell.
|
||
- `Coordinate` tells the user that renaming is not built yet.
|
||
- `Derived` names a formula that does not exist.
|
||
- `None` means "no cell" (`protocol.md` §3), and this cell holds a value.
|
||
|
||
The variant carries the offending coordinate, so every decline says the same
|
||
sentence: `improvise_protocol::non_item_refusal`, which the host's refusal uses
|
||
too. The two front ends agree because the TUI's `value_cell_key` and
|
||
`committable_cell_key` also read `cursor_cell_id`: `p`, `x` and a commit on
|
||
such a cell are refused with that sentence in the terminal as well. A
|
||
`NonItem` publishes no record version, since no basis can claim a write it
|
||
could never make.
|
||
|
||
**`i` refuses at open, as the browser does (improvise-yc9r).** `EditOrDrill`
|
||
first asked only about folds, aggregates and formulas. So `i` opened an editor
|
||
on a `NonItem` cell, and the refusal came at commit. There, the minibuffer
|
||
draws in place of the status line (gotcha 21), so the user saw nothing happen.
|
||
`EditOrDrill` now asks `cursor_cell_id` after the formula check, which is the
|
||
order `cell_id` asks in. The commit guard stays, and it is still reachable:
|
||
`:enter-mode editing` opens an editor without passing through `EditOrDrill`.
|
||
Each of the four declines has its own test. Before these, three rivals passed
|
||
the whole workspace: writable in `value_cell_key`, a `Record` in
|
||
`committable_cell_key`, and `None` in the browser's open-editor arm.
|
||
|
||
**Membership is a lookup, not a list (improvise-4daa).** `first_non_item` runs
|
||
for every coordinate of every projected cell, and the TUI projects the whole
|
||
grid every frame (gotcha 44). It used to build `effective_item_names` (a fresh
|
||
`Vec<String>` of every item) for each coordinate. That made `grid_of` quadratic
|
||
in a label category: 145 ms against 1.9 ms for 2000 records. `Model::is_item`
|
||
answers the same question with `contains_key`, plus the formula targets for a
|
||
measure category, which is where `effective_item_names` differs from the
|
||
category's own items. `is_item_agrees_with_effective_item_names_everywhere`
|
||
holds the two answers equal.
|
||
|
||
Option (b) on the bead, closing the sources, is not done here. It is
|
||
improvise-98pd. With (a) in place, a source that still writes a non-item
|
||
produces a cell that is declined everywhere, not one that is offered and then
|
||
refused.
|
||
|
||
**Why the `vec![]` arm stays.** A reviewer asked whether a `SetCell` whose cell
|
||
does not resolve at all, which `model_effects` turns into `vec![]` and so
|
||
applies nothing, should also be a `Rejection`. Through a `Host`, it already is,
|
||
before that arm is reached. A lone write naming a record that never existed gets
|
||
`RecordGone` up front, whether or not the basis claims it, because the create
|
||
exemption covers only a *later* command in a frame
|
||
(`a_wire_write_to_a_record_that_never_existed_is_rejected`). A later write whose
|
||
record nothing in the frame made resolvable gets `ChainBroken` from
|
||
`broken_chain`
|
||
(`a_frame_whose_write_would_land_nowhere_is_refused_rather_than_dropped`). So
|
||
the arm is unreachable from the wire. Making it a refusal would add a second
|
||
answer to "does this resolve" beside `broken_chain`, and that answer could only
|
||
ever fire after the first had already refused. It is left as the mutation
|
||
path's own behaviour for callers that do not go through the session door.
|