d1ce2ecbc3
improvise-rmvx, improvise-fdt6, improvise-gcvb — three faces of the same
thing: a browser offering something and then doing nothing.
rmvx. `the_command_names_that_still_need_the_host_are_these` was built with
`in_mode(AppMode::Normal)`, so it only ever asked about Normal-mode bindings:
`add-record-row` (RecordsNormal) and `delete-formula-at-cursor` (FormulaPanel)
were outside its reach entirely and could never have failed it. A named list
that silently scopes itself to one mode is a guard that looks broader than it
is. `every_advertised_command_answers_in_its_own_mode` crosses
`help::advertised` — command NAMES, so no hint string is tokenised — against
`resolve` in every mode, over a fixture furnished enough that an `Upstream` can
only mean an unhandled name. It found a seventh instance the ticket had not
predicted: `view-back` is advertised in RecordsNormal.
Six names now decline out loud, each with its own true subject, out of one
`declined` table; `stranding` returns the subject rather than a bool, so `:`
says the command line is unavailable instead of doing nothing. A declining step
takes its whole `Binding::Sequence` with it, which is what keeps `o` ("add a
record row, then edit it") from opening an editor on a row nobody created.
fdt6. `commit-buffer formula` already resolved; what did not exist was any way
to SEE the typing. `RenderModel::editing` is scoped to the two cell-editing
modes and reads the "edit" buffer by name, so FormulaEdit, CategoryAdd, ItemAdd
and the three prompts opened and swallowed every character. `RenderModel::
minibuffer` is `AppMode::minibuffer()` plus the buffer that config names, read
through `improvise_protocol::read_buffer` so gotcha 11 keeps its one home, and
`web/app.js` paints it.
gcvb. The records-mode early return was broader than its reason. A
`CellId::Create` commits the typed NAME as one existing `ModelCommand`, reads
no `record_col` and writes no cell, so the return narrows rather than
disappears. `+cat` must NOT advance: an inserted column renumbers every index
to its right and the advance was computed from the pre-commit layout, so a
client that advanced anyway would land one column off, silently. Both halves
proved by injection against `CommitAndAdvance` in `tests/lockstep.rs`.
Every new guard was proved by injecting the fault it names. The one worth
repeating: `web/tests/render_surface.test.js` went green over a page that
painted nothing, because the string `model.minibuffer` was inside a function
nothing called. The static check is a completeness check, never an execution
check — `harness/tests/keys.spec.js` caught it in Chromium on the first run.
1394 → 1397 Rust tests; harness 17 → 19 passed, 3 skipped; node 26.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
1523 lines
91 KiB
Markdown
1523 lines
91 KiB
Markdown
# Clients: the browser client, the two hosts, and the harness
|
||
|
||
The arguments that used to live in the comment headers of
|
||
`improvise-wasm-client`, `improvise-ws-server`, `improvise-worker-server`,
|
||
`web/` and `harness/`. Written for a reader with the code open. Bug narratives
|
||
keyed by test name are in `context/test-evidence/clients.md`.
|
||
|
||
Repo-map gotchas 42, 44, 45, 46, 47 and 48 cover the same ground from the
|
||
"where" side; this file is the "why", at the density the comments had.
|
||
|
||
---
|
||
|
||
## `improvise-worker-server/src/worker.rs` — the message loop
|
||
|
||
`Worker` is the whole browser host minus the two things a browser has to
|
||
supply — `self.onmessage`/`self.postMessage`, and OPFS. Both are in
|
||
`crate::bindings`, which is `cfg(target_arch = "wasm32")` and has no decisions
|
||
in it. Everything that could be *wrong* about what the main thread is told is
|
||
in `worker.rs`, where `cargo test` can drive it.
|
||
|
||
### It is the socket's `Host`, not a second one
|
||
|
||
`Host` is `App` + `Sessions` + `handle(id, ClientFrame) -> Vec<Addressed>`.
|
||
Nothing in it is a socket, and improvise-9x6 is what made that load-bearing
|
||
rather than tidy: the worker needs the same session loop and cannot depend on
|
||
`improvise-ws-server`, which is tokio and tungstenite. So `Host` moved down to
|
||
`improvise_command::ui::host` and **both deployments name the one type**.
|
||
Re-implementing it here would have been a mirror, and improvise-xks6 had just
|
||
finished deleting the last one: a second copy of a decision layer drifts, and
|
||
the drift is invisible until a client sees it.
|
||
|
||
What the worker adds on top is only what a `MessagePort` differs in:
|
||
|
||
- **One client, by construction.** A `MessagePort` is point to point, so the
|
||
`SessionId` is not an address the transport has to carry — it is a field.
|
||
`Addressed` is still honoured rather than assumed away (`Worker::frames_for`),
|
||
because a `SharedWorker` would make it matter again and the honest filter
|
||
costs one line. `Host::relay` addresses a broadcast to every session it
|
||
knows, and with one session that is always this one — a loop that assumed it
|
||
away would be silently wrong at that point rather than obviously incomplete.
|
||
- **A handshake with no close.** `ClientFrame::Viewport` must come first, for
|
||
the reason it must on the socket: a `Snapshot` seeds the *visible* cells, so
|
||
one built at extent zero is an empty grid. A socket enforces that by closing
|
||
the connection; a worker has nothing to close, so a frame that arrives before
|
||
the handshake is **dropped**, which is the same outcome without a mechanism
|
||
that does not exist here.
|
||
|
||
### `ClientFrame::Key` is answered with nothing, and that is not laziness
|
||
|
||
A worker holds the authoritative `App`, so `ui::reduce::reduce_key` really is
|
||
callable here where it is not on a socket. Calling it would still be wrong, and
|
||
the reason is not about transports at all: **nothing in the projection stream
|
||
carries the host's own `ViewState`.** A client's view slice moves only by
|
||
folding `Command`s, and a keypress resolved against the `App` moves the
|
||
*`App`'s* slice — its mode, its cursor, its minibuffer — with no `Command`
|
||
produced that any client could fold. `ClientSession::project` emits document
|
||
projections, a grid diff and a basis; none of the three is a mode change.
|
||
|
||
So a half-wired `Key` arm does not under-deliver, it **diverges**: the host
|
||
would believe it is in `AppMode::Editing` while the client draws Normal, and
|
||
`ClientSession::viewport()` reads the *mirror's* cursor, so the grid would not
|
||
even scroll to follow.
|
||
|
||
improvise-bpq2 answered it, and **not** by deriving `Command`s from a
|
||
`ViewState` delta. The user's ruling is that a keymap is UI and the server must
|
||
not know it, so resolution moves to the *client*: a key the client can answer
|
||
arrives here already resolved, as a `ClientFrame::Commands` batch it has folded
|
||
itself, and this arm's emptiness stops being a gap and becomes the statement.
|
||
What remains upstream is the set of bindings a client still cannot compose —
|
||
prefix keys, anything naming a row's category or a group, anything touching
|
||
`Storage` — and those are closed by enriching the projection, one at a time,
|
||
not by wiring this arm up. The list lives in `keys.rs` (see below).
|
||
|
||
### Persistence is the worker's, whether or not anybody asks
|
||
|
||
There is no `ModelCommand::Save`, and that is not an oversight: `effect::Save`
|
||
is `EffectKind::ViewOnly` (it writes the document to *this host's* file path
|
||
and clears *this host's* dirty flag; it mutates no document state), so the
|
||
derivation that produces `ModelCommand` from the model effects cannot reach it
|
||
(`decision(improvise-cqi, command_set_derived_not_invented)`). Saving was
|
||
therefore unreachable from any client on any transport until improvise-368h
|
||
added `ClientFrame::Save` — a *request to the host*, framed rather than
|
||
commanded precisely because it mutates nothing shared.
|
||
|
||
**A browser still never sends it**, and that is the right answer rather than a
|
||
gap: there is no file the user owns and no Ctrl-S semantics to honour, so
|
||
asking would be the wrong question — the document *is* the OPFS entry, and a
|
||
page that loses an edit because nobody pressed a key has simply lost it. So the
|
||
worker persists on its own behalf: `Worker::persist` runs the one existing
|
||
`Save` effect through `App::apply_effects`, so effects remain the only mutation
|
||
site and the save path is not duplicated. A worker with no `file_path` silently
|
||
does nothing, which is what `Save` already does; a failed save leaves its
|
||
message in the status and aborts the batch.
|
||
|
||
**The trigger is an observation of the document, not a table of command
|
||
variants.** It used to be the latter, and the reason was honest: a wire
|
||
`ModelCommand::SetCell` reached `effect::SetCell`, which did not raise
|
||
`model_state.dirty` — that was a separate `MarkDirty` effect only the key path
|
||
composed — so "save if dirty" was a trigger that never fired (improvise-gv9r).
|
||
With dirtiness raised by `App::apply_effects` off the same `EffectKind` the
|
||
`Dirty` is, the flag *is* the observation, and asking it is better than
|
||
enumerating variants for the same reason the cursor reset watches `cursor_epoch`
|
||
rather than matching on `CycleAxis`, and the rebase watches
|
||
`document_generation` rather than matching on `LoadModel`: a table of variants
|
||
is a second description of which commands change the document, free to disagree
|
||
with the first. It is also strictly tighter — a refused batch changes nothing,
|
||
so it raises nothing, and the "was anything rejected" special case that used to
|
||
guard this went with it.
|
||
|
||
One cost remains and is stated rather than hidden: `Storage` is whole-document
|
||
by construction, so OPFS rewrites the file per accepted model command —
|
||
correct, not yet cheap. And a failed write leaves the flag raised, so the next
|
||
frame retries: right for a transient failure, and harmless for a permanent one
|
||
(a worker with no `file_path`) because the only thing a failed `Save` produces
|
||
is a status line, and nothing in a worker renders one.
|
||
|
||
The one frame `on_frame` does **not** auto-persist after is `ClientFrame::Save`:
|
||
a client that asked to save has already been answered, and a failed write leaves
|
||
`dirty` set — so retrying would repeat the I/O and, on a transient failure, tell
|
||
the client `SaveFailed` about a document that is now on disk. The automatic
|
||
write exists for edits *nobody asked* to save.
|
||
|
||
So this is the browser's own answer, and `ClientFrame::Save` is the other one —
|
||
for a socket client editing a TUI-hosted document, where there *is* a file
|
||
somebody owns.
|
||
|
||
### Ordering
|
||
|
||
`Worker::on_frame` returns the frames for one inbound frame, in order, and a
|
||
`ServerFrame::Commands` batch must reach the client **whole and in sequence**:
|
||
a `GridUpdate::Cells` that overtakes the labels framing it is dropped, not
|
||
replayed. That is a property of the fold, so a `postMessage` transport owes it
|
||
exactly as a socket does — never coalesce two batches, never split one.
|
||
|
||
### Malformed frames are loud
|
||
|
||
`Worker::on_message` returns `Err` on a frame that will not parse rather than
|
||
swallowing it. A worker that silently ignores a malformed frame leaves the
|
||
client waiting forever for an answer that is never coming.
|
||
|
||
`Worker` is not `Debug`, for the reason `Host` is not: it owns the whole
|
||
document.
|
||
|
||
---
|
||
|
||
## `improvise-ws-server` — the socket host
|
||
|
||
Three layers, and the split is the point — the frames are a fourth thing this
|
||
crate speaks but does not own:
|
||
|
||
- **the frames are not here.** `ClientFrame`/`ServerFrame` are
|
||
`improvise-protocol`'s, because the same envelopes frame the worker
|
||
deployment and a browser bundle cannot depend on this crate (improvise-xks6).
|
||
This crate speaks them and owns none of them.
|
||
- **`host`** — the session loop: one authoritative `App`, one `Sessions`, and a
|
||
`handle(id, msg) -> Vec<Addressed>` that is where every decision is made and
|
||
every test is pointed. Not here either, and for the same reason the frames are
|
||
not: the browser worker hosts the same loop over a `MessagePort` and cannot
|
||
depend on tokio. `src/host.rs` is a re-export of
|
||
`improvise_command::ui::host`, kept rather than deleted so
|
||
`improvise_ws_server::host::Host` still resolves. `Host` knowing nothing about
|
||
sockets was always true — the file used to say "a websocket is awkward to
|
||
test, so nothing that decides anything lives in one" — and improvise-9x6 made
|
||
it load-bearing. A second copy is a mirror, and a mirror drifts; that is the
|
||
same answer improvise-xks6 gave for the frames one ticket earlier.
|
||
- **`registry`** — the documents: a map of `Host`s keyed by document id, the
|
||
lifecycle rules, and the one place a per-document `SessionId` becomes a
|
||
server-wide address. Socket-free. It stayed on the server because a worker
|
||
hosts exactly one document.
|
||
- **`transport`** — tokio and tungstenite: framing, JSON, the handshake rule,
|
||
and reading the requested document off the URL path. No decisions about what
|
||
a client is told.
|
||
|
||
Native-only: tokio's networking does not build for `wasm32-unknown-unknown`,
|
||
and it should not.
|
||
|
||
### The registry's grain
|
||
|
||
Sessions editing *independent* documents are trivially non-conflicting, which
|
||
is why ADR 001's basis tiers — `document_generation`, `layout_version`,
|
||
`record_versions` — live on the `App` and scope per document **for free**. A
|
||
registry that let two documents share any of that would throw the free scoping
|
||
away, so the registry owns no state of its own beyond the map: there is nothing
|
||
here for two documents to share.
|
||
|
||
**A `SessionId` is not unique server-wide.** `Sessions` mints ids from a counter
|
||
it owns, so every document's first client is `SessionId(0)`. The server-wide
|
||
address is therefore the **pair**, `ClientId`, and `Registry::route` is the one
|
||
place a `Host`'s `SessionId` acquires the document it came out of. That is the
|
||
whole cross-document isolation argument, and it is structural rather than
|
||
checked: a `Host` can only name its own sessions, and the only tag `route` can
|
||
apply is the document whose `Host` produced the frames. In particular an
|
||
`Applied::Rebased` fans a fresh `Snapshot` out to every client of *that*
|
||
document and cannot reach further, because the ids it returns are meaningless
|
||
anywhere else.
|
||
|
||
### Lifecycle
|
||
|
||
**Open** happens at startup, from the documents the host names. A client cannot
|
||
ask the server to open a path it was not told about, and that is not an
|
||
oversight: deciding which paths a client may open is *access control*, which
|
||
improvise-j31x puts explicitly out of scope. Serving only what was named leaves
|
||
the question unanswered rather than answering it badly. A stem collision is
|
||
fatal rather than resolved: two files with the same stem are two documents a
|
||
client could not tell apart, and silently serving one is worse than saying so.
|
||
|
||
**Close does not happen when the last session leaves.** A document stays
|
||
resident for the life of the server. Evicting it was considered and rejected
|
||
twice over:
|
||
|
||
- Unsaved edits would vanish. The server has no autosave obligation, so a
|
||
reconnect would silently roll the document back to what is on disk.
|
||
- It would rewind the basis tiers. `document_generation` and `record_versions`
|
||
start at zero in a fresh `App`, so a client that reconnected holding a *stale*
|
||
basis would have it accepted as current — a correctness hole in the exact
|
||
mechanism ADR 001 is building, paid for with a memory saving nobody asked for.
|
||
|
||
An idle document therefore costs its `App`, and `Registry::is_idle` exists so a
|
||
future eviction policy has somewhere to ask the question from — nothing calls it
|
||
to evict, because nothing evicts.
|
||
|
||
### The transport's two rules
|
||
|
||
**A client names its document in the URL** (improvise-j31x):
|
||
`ws://host:port/quarterly` asks for `quarterly`, and `ws://host:port/` asks for
|
||
whichever one there is when there is exactly one. The path was chosen over a new
|
||
`ClientFrame` variant for two reasons: a document is a *resource*, which is what
|
||
a URL path is for, and `ClientFrame` stays exactly as it was, so every client
|
||
written against the single-document server keeps working unchanged. Which
|
||
document a name resolves to is `Registry::resolve`'s decision, not the
|
||
transport's; all that happens there is `path -> Option<&str>`. It is not a path
|
||
walker — everything after the leading slashes is the name.
|
||
|
||
The other rule is that **the first frame of a connection reports the extent**,
|
||
because the bootstrap `Snapshot` cannot be built without it.
|
||
|
||
A refused connection is closed with a websocket close frame carrying the reason.
|
||
No `ServerFrame::Error` was added: the frame vocabulary is derived from the
|
||
mutations that need it, and "this connection is not happening" is a fact about
|
||
the connection, which the protocol already has a frame for.
|
||
|
||
A `Registry` is not `Send` (a `Host`'s `App` owns a `Box<dyn Storage>`), and
|
||
must not be shared anyway: it holds the authoritative documents, and one owner
|
||
is the point. So it sits in a single task on a `LocalSet` and every connection
|
||
talks to it over a channel. That is also what serializes the clients — commands
|
||
arrive in one order, and the projections each client gets are produced in that
|
||
order, which matters because a `GridUpdate::Cells` that overtakes its labels is
|
||
dropped. One task for the whole registry rather than one per document is a
|
||
*throughput* choice and nothing more: the runtime is single-threaded either way,
|
||
and no state is shared between documents by sharing a queue.
|
||
|
||
**No auth, no TLS, localhost only.** A deliberate MVP limit (improvise-q08), not
|
||
an oversight: anyone who can open a socket to this port can rewrite every
|
||
document it serves, so it binds a loopback address by default and `serve` says
|
||
so. `--bind` exists so the limit is a choice you can see rather than one buried
|
||
in the code. ADR 001 owns what multi-writer safety becomes.
|
||
|
||
---
|
||
|
||
## `improvise-protocol/src/resolve.rs` — one keypress → `Command`s
|
||
|
||
### It is not the browser's module any more (improvise-0yq8)
|
||
|
||
`resolve`/`step` and `ClientState` were `improvise-wasm-client`'s, and nothing
|
||
about either was a browser fact: both depend on `ViewState`, the protocol
|
||
widget caches and the announced window, every one of which is already a
|
||
protocol type. Being in the wasm crate meant a second front end could reach
|
||
them only by depending on a `cdylib` built for a browser, so the move cost no
|
||
new dependency and bought the rule design-principles §5 states — **a new front
|
||
end needs exactly two things**: its own event → `KeyEvent` mapping, and
|
||
painting. `BrowserKey` is the first of those two and is the only thing left in
|
||
`improvise-wasm-client/src/keys.rs`; `ClientState` is now
|
||
`improvise_protocol::client`, and `typed_cell_command` went with `step`, which
|
||
calls it.
|
||
|
||
What did **not** become trivial is `tests/lockstep.rs`. It drives
|
||
`improvise_command::ui::reduce::reduce_key` — the host's `Cmd`/`Effect` path —
|
||
against `resolve`, and those are still two derivations of the same answer, in
|
||
two crates, one of which cannot be reached from a browser. Moving `resolve`
|
||
changed which crate it is spelled in, not that it is a mirror, so
|
||
`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)`
|
||
does not fire and the test stays.
|
||
|
||
### The keymap is not this module's
|
||
|
||
It used to be. improvise-bpq2 gave this file a hand-written classifier that
|
||
decided, per key and per mode, what a keypress meant — a second spelling of
|
||
`default_keymaps()`, kept in step by a lockstep test over a hand-written list.
|
||
The user's ruling is that the web front end uses the **same** keymap as the TUI,
|
||
not a mirror of it, and
|
||
`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)` says
|
||
why: two definitions held honest by a compatibility test are one definition
|
||
waiting to be made.
|
||
|
||
So the table moved to `improvise_protocol::keymap` and this module reads it.
|
||
Which key means what is now correct **by construction**: a rebinding in
|
||
`default_keymaps()` reaches the browser with no change here, and a new binding
|
||
for a command this module can already execute simply works.
|
||
|
||
### What is left here, and why it is the better boundary
|
||
|
||
A lookup yields a command **name and arguments**. Turning that into something
|
||
that runs needs a `CmdContext` — a `Model`, a `GridLayout`, an `App` — which
|
||
this client does not have. So *key → intent* is shared and *intent → `Command`*
|
||
is ours, for the subset we can compose. `step` is that subset, and it is the
|
||
list that replaces the old list of keys. That is a list of names rather than of
|
||
keys, which is strictly better in two ways: it is enumerable, and it does not go
|
||
stale when a binding moves.
|
||
|
||
What this client can execute, and what each one needed:
|
||
|
||
- **Text entry** (`append-char`, `pop-char`, `clear-buffer`). The buffer is the
|
||
binding's own argument now — no longer read off `AppMode::minibuffer()` and
|
||
hoped to agree with what the host's keymap named. `TextEntry::new` derives one
|
||
from the other, so the two were equal; they are now the *same string*. The
|
||
pair `read_buffer`/`write_buffer` mirrors `cmd::core`'s, which are the only
|
||
code on the host that knows a runtime-named buffer may not be an entry in
|
||
`buffers` at all (improvise-gdl): the name `"search"` is
|
||
`ViewState::search_query`. The shared keymap binds `append-char ["search"]` in
|
||
`SearchMode`, so this client needs the same pair — and having it is what lets
|
||
search typing resolve here for the first time.
|
||
- **Mode entry** (`enter-mode`), decoded through `AppMode::from_name`, which
|
||
moved next to `AppMode` for the same reason the keymap did.
|
||
- **Grid, panel and tile cursors**, which needed `GridExtent` and the window
|
||
this client announced.
|
||
- **Entering an edit** (`edit-or-drill`), which needed ADR 002: the branch
|
||
`EditOrDrill` takes on "is this cell aggregated" is exactly the `CellId`
|
||
variant the server already projects.
|
||
- **Committing one** (`commit-cell-edit`, `commit-and-advance-right`), which
|
||
needed the record surrogate — again ADR 002 — and nothing else for the
|
||
plain-grid case.
|
||
|
||
### The second tranche, and the three it could not reach (improvise-e24v)
|
||
|
||
Fifteen more command names resolve here: `cycle-panel-focus`; the category
|
||
panel's `cycle-axis-at-cursor`, `open-item-add-at-cursor`, `filter-to-item`,
|
||
`toggle-cat-expand` and `delete-category-at-cursor`; the view panel's
|
||
`switch-view-at-cursor`, `create-and-switch-view` and `delete-view-at-cursor`;
|
||
tile select's `set-axis-for-tile` and `cycle-axis-for-tile`; the help overlay's
|
||
`help-page-next`/`help-page-prev`; `commit-buffer` in all three of its kinds;
|
||
and the two cell writes `clear-cell` and `paste`.
|
||
|
||
Each is `Cmd`-for-`Cmd`, including the parts that look like noise: the status
|
||
sentence a command writes beside its mutation is *part of the answer*, and a
|
||
client that emitted the mutation alone would diverge on the status line and
|
||
nowhere else — which is precisely the sort of divergence nobody notices. Two
|
||
orderings are load-bearing and copied rather than rationalised.
|
||
`CommitBuffer` writes its optimistic status **first** so the add's own failure
|
||
message wins (improvise-6t3), and `TileAxisOp` writes the axis **first** and
|
||
the status after.
|
||
|
||
**Three of the tranche's own list turned out to need enrichment after all**
|
||
(improvise-4bwa), and the reasons are worth more than the code:
|
||
|
||
- `toggle-prune-empty` words its status off `View::prune_empty`, which is in
|
||
the `Workbook`, in no cache, and not in `ViewState`.
|
||
- `delete-formula-at-cursor` needs `(target, target_category)`; `FormulaCache`
|
||
carries the formula's raw *text*, and recovering the two from it means
|
||
reimplementing `parse_formula` on the client.
|
||
- `view-back` emits `ApplyAndClearDrill` before `ViewBack`, and that effect is
|
||
deliberately off the wire (gotcha 38): its operand is somebody's uncommitted
|
||
drill edits. This one is not an enrichment at all — it is blocked on ADR 001
|
||
layer 2's addressed form, and mirroring it by rebuilding the effect on the
|
||
client would re-open exactly what improvise-3x3u closed.
|
||
|
||
**The lockstep harness these are tested through** (`both_ends` in
|
||
`tests/lockstep.rs`) is worth two sentences, because two of its choices were
|
||
wrong first. It compares the document as **`.improv` text**, not as `Debug`:
|
||
`Debug` prints the symbol table's `HashMap`s, so two byte-identical documents
|
||
print differently per process (gotcha 18), and the first version failed on
|
||
nothing but hash order. And it applies the client's composed commands to a
|
||
**second authoritative host** rather than eyeballing them, restating the basis
|
||
against that host with `caught_up` — the claim was published by the first host,
|
||
and the basis is the only thing that legitimately differs between two hosts
|
||
holding the same document.
|
||
|
||
**How the client claims, and why there is nothing here to keep in step.** A
|
||
client must answer the same question the server does — which records does this
|
||
`ModelCommand` write — to build its `Basis`. The first version of this answered
|
||
it *again*, in two helpers (`structural`, claiming nothing, and `record_write`,
|
||
claiming the cell's own record), and the adversarial review of PR #50 showed
|
||
that nothing tested them: `lockstep.rs`'s `apply_on` restates the basis against
|
||
the twin with `caught_up`, which re-derives it with the **server's**
|
||
`records_named` and throws the client's claim away before anything is compared.
|
||
Emptying `record_write`'s claim left the workspace green.
|
||
|
||
The fix is not a test. `records_named` is pure — `&ModelCommand -> Vec<RecordHash>`,
|
||
no `App` — and `RecordHash` is an `improvise-core` type the protocol already
|
||
re-exports, so it **moved down into `improvise-protocol`** beside the enum it
|
||
reads, and `improvise-command` re-exports it. `resolve.rs` has one `model()`
|
||
that claims `records_named(&cmd)`. The client's claim is now right by
|
||
construction, and the guard is the one that was always there: `records_named`
|
||
is an exhaustive match, so a new record-writing variant fails the build.
|
||
|
||
What the old shape would have cost is worth recording, because it is not a
|
||
compile error: `structural` accepted **any** variant and claimed `[]`, so a new
|
||
record-writing command would have compiled clean on the client with the wrong
|
||
answer, and the symptom is `Rejection::RecordUnstated` — the browser binding
|
||
becomes a dead key, refused, with nothing failing.
|
||
|
||
**`both_ends` compares every cursor**, not only the document, the mode and the
|
||
status. Three of the resolvers' decisions *are* a cursor move and nothing else —
|
||
`delete_view_at_cursor`'s step-back is the sharpest — so a comparison that
|
||
stopped at the document would agree while the panel cursor diverged.
|
||
|
||
### Mirror the `Cmd` line for line, *including where siblings disagree*
|
||
|
||
`doctrine(mirror_the_cmd_line_for_line)`. `Move` emits `SetSelected` even when
|
||
it clamps and `MovePanelCursor` returns nothing when the cursor does not move;
|
||
`MoveTileCursor` emits regardless, like `Move`. **Tidying that into one rule
|
||
would make this a second derivation**, which is exactly what `tests/lockstep.rs`
|
||
exists to rule out. Which widgets say nothing about a move that goes nowhere is
|
||
not a rule of this module — it is whatever the `Cmd` being mirrored does, and
|
||
the three differ.
|
||
|
||
Two more places the mirroring is literal. `panel_cursor` clamps a cursor past
|
||
the end *before* applying the delta, and derives the category panel's length
|
||
from `flatten_cat_tree` — which depends on `expanded_cats`, per-client, which is
|
||
why it is derived rather than sent and why the derivation lives in
|
||
`improvise-protocol` where both ends reach it (improvise-edp). And `commit`
|
||
mirrors `change_mode(exit_mode_for(edit_mode))`, then `EnterAdvance` — which
|
||
**aborts the rest of the batch** at the bottom-right corner, so the re-entry is
|
||
skipped and the user lands in normal mode — then `EnterEditAtCursor`, which
|
||
seeds the `edit` buffer from the cell it landed on. The trailing `clear-buffer`
|
||
the keymap welds to every commit then empties that buffer again. That ordering
|
||
looks wrong and is exactly what the TUI does; mirroring it is the point.
|
||
|
||
`GridMove` is a hand-spelled mirror of `navigation.rs`'s `MoveKind`, because the
|
||
command crate is not on a browser's dependency list and never will be. The
|
||
*keys* that reach it are no longer spelled twice. `scrolled_to` mirrors
|
||
`navigation.rs::viewport_effects` over the window this client announced.
|
||
|
||
### The three outcomes
|
||
|
||
`KeyOutcome::Local(Vec<Command>)` — resolved here; the caller folds them locally
|
||
*and* sends them, as one frame and in this order (the asker's own fold is its
|
||
own; the host forwards only to the others). A `Vec` because one keypress is one
|
||
binding but several `Effect`s: a cursor move is `SetSelected` plus whichever
|
||
scroll offsets follow, and a `Binding::Sequence` is several commands to begin
|
||
with. **Never empty** — a keypress this client can answer with nothing is
|
||
`Ignored`, and `KeyOutcome::of` is the one constructor so that contract is a
|
||
property of the type's users rather than a rule each resolver remembers.
|
||
|
||
`Upstream(KeyEvent)` — bound to a command this client cannot compose.
|
||
`Ignored` — nothing bound, or what is bound resolves to no change. **Nothing
|
||
bound is not "the host might know better"**: the host looks the key up in this
|
||
same table. That is new; it used to go upstream into an empty vec.
|
||
|
||
A `Binding::Sequence` is **all or nothing**: half of `Esc` — clearing the buffer
|
||
without leaving the mode — is worse than none of it. Each step is resolved
|
||
against the *unchanged* state, because the host resolves every step against one
|
||
immutable `CmdContext`; the commands are then folded in order.
|
||
|
||
### What still needs the host
|
||
|
||
`the_command_names_that_still_need_the_host_are_these` names them, and
|
||
`Host::handle` answers a `Key` frame with an empty vec, so each one is a binding
|
||
that **does nothing at all** in a browser
|
||
(`doctrine(an_upstream_key_is_a_black_hole)`).
|
||
|
||
**An upstream key that the app also ADVERTISES is a different severity, and it
|
||
had its own class of defect** (improvise-rmvx). A key nobody offers doing
|
||
nothing is a gap; a key named in the mode's own hint line doing nothing reads as
|
||
*broken*. The two are crossed by `every_advertised_command_answers_in_its_own
|
||
_mode`, which asks `help::advertised` — a table of command **names**, so the
|
||
cross-reference is exact and nothing tokenises the rendered string — against
|
||
`resolve` in every mode, over a fixture furnished enough that an `Upstream` can
|
||
only mean an unhandled name. The old guard asked only about `Normal`, which is
|
||
why `add-record-row` and `delete-formula-at-cursor` were outside its reach
|
||
entirely; `view-back` in RecordsNormal was a seventh instance nobody had
|
||
noticed.
|
||
|
||
`declined` is the answer, and it is one list rather than an arm apiece: six
|
||
command names, each with **its own true subject**. Wording matters here more
|
||
than it looks — the `>` fix was nearly routed through a message about
|
||
aggregates, which would have been true-sounding about the wrong thing and worse
|
||
than silence, because a reader who is told something plausible stops looking.
|
||
`step` checks `declined` first, so `composed` has no arm for any of them and the
|
||
two lists cannot overlap.
|
||
|
||
A declining step takes its **whole sequence** with it: `o` in RecordsNormal is
|
||
`[add-record-row, enter-edit-at-cursor]`, and the second step's premise is false
|
||
once the first has declined. Returning only the decline, rather than the decline
|
||
plus whatever composed before it, is what keeps `Binding::Sequence`'s "every
|
||
step or none of them" true — an abort is not a rollback (gotcha 21), and this
|
||
client has no way to undo a `Command` it has already put on the wire.
|
||
|
||
The classes that remain:
|
||
|
||
- **Prefix keys.** `Command::SetTransientPrefix` exists and this client could
|
||
arm one; what it cannot do is what the *second* key then runs, which is
|
||
`yank`, `wq` and `toggle-col-group-under-cursor`. Arming a prefix whose
|
||
payload is unreachable is worse than not arming it.
|
||
- **Anything naming a row, column, item or group** — `hide-selected-row-item`,
|
||
`toggle-group-under-cursor`, `delete-formula-at-cursor`. The first two are **no
|
||
longer blocked** and are simply not built here yet (improvise-e24v): since
|
||
improvise-anr9 `GridCache` carries an `AxisFrame` per axis, so `cats[0]` +
|
||
`label(idx, 0)` is exactly what `HideSelectedRowItem` reads off `GridLayout`,
|
||
and `group_of(idx)` is exactly `ToggleGroup`'s `(category, group)`. The one
|
||
thing to get right is that the frame is **windowed** and the cursor is
|
||
absolute, so a client must reconcile the scroll offset first, as `render.rs`
|
||
does for the selection. `delete-formula-at-cursor` stays: it names a position
|
||
in the formula panel, not in the grid.
|
||
- **Anything touching storage or an arbitrary command line.** The host's by
|
||
definition.
|
||
- **The records-mode commit** — *except the two creating columns*. A wide-records
|
||
row is one coordinate tuple and editing a category column moves every measure
|
||
cell in it; `commit_plain_records_edit` needs `GridLayout::record_col` to know
|
||
which. A `CellId::Create` does not: it commits the typed NAME as one existing
|
||
`ModelCommand` (`AddItem` into `_Measure`, or `AddCategory`), reads no
|
||
`record_col` and writes no cell, so improvise-gcvb narrowed the early return
|
||
rather than removing it. The `+cat` half **must not advance**: an inserted
|
||
column renumbers every index to its right and the advance was computed from
|
||
the pre-commit layout (gotcha 23), so `CommitAndAdvance` returns straight
|
||
after it and a client that advanced anyway would land one column off,
|
||
silently. `AddCategory` can also be refused by the 12-category limit and sets
|
||
`abort_effects`; the mode change therefore goes **first**, where an abort
|
||
cannot reach it (gotcha 21).
|
||
- **Drill.** Session-local residue, and the one case that does **not** go
|
||
upstream silently — see below.
|
||
|
||
### `stranding`: a prompt this client can enter but cannot commit
|
||
|
||
`enter-mode` is trivially resolvable here, and that is the trap: the user
|
||
reaches the prompt, types a path, presses Enter — and the commit resolves to
|
||
nothing, because `Host::handle` answers a `Key` frame with an empty vec. **A
|
||
dead room is worse than a locked door**: the door costs one inert key, the room
|
||
swallows work the user believes was done.
|
||
|
||
`SavePrompt` and `ExportPrompt` are host-only **by construction**, not by
|
||
omission: `effect::Save`, `SaveAs` and `ExportCsv` are all
|
||
`EffectKind::ViewOnly`, so the derivation that builds `ModelCommand` from the
|
||
model effects cannot name them. Closing this is not a resolver to add; it is a
|
||
wire operation that does not exist. In a browser it is also close to
|
||
meaningless: the worker persists on its own behalf whenever the document is
|
||
dirty, so the document *is* the OPFS entry and there is no Ctrl-S semantics to
|
||
offer.
|
||
|
||
It returns the **subject** rather than a bool (improvise-rmvx): the refusal is
|
||
right and the silence was not, so `:` now says the command line is unavailable
|
||
instead of doing nothing at all. The predicate is named as a property rather
|
||
than matched at the call site (`doctrine(two_questions_one_enum)`): "can this
|
||
client finish what entering the mode starts" is not the same question as "is
|
||
this a text-entry mode", and a third such prompt belongs here. Found by Copilot on PR #27 against the newly
|
||
added `SavePrompt`; `ExportPrompt` had the identical trap already and was fixed
|
||
with it — fixing one arm of a rule with two is the shape
|
||
`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)` names.
|
||
|
||
**`CommandMode` is dead by omission, and listed anyway.** Copilot found it on
|
||
the next round, and it was a false negative in the predicate's *first* version —
|
||
which mattered more than its severity, because
|
||
`a_prompt_this_client_cannot_commit_is_never_entered` used `:` as the negative
|
||
arm that stops "refuse every mode" from passing. An arm asserting something
|
||
false does not merely fail to guard; it **certifies the behaviour it was added
|
||
to rule out.** `:` resolves locally into `CommandMode` and typing appends
|
||
locally, but Enter is `[execute-command, clear-buffer]` and `step` has no
|
||
`execute-command` arm, so `resolve` returns `Upstream` and the frame is dropped.
|
||
Every ex-command behind `:` is inert for the same reason, so refusing the mode
|
||
removes **no working surface**, only a room that swallows typing.
|
||
|
||
The distinction from the two prompts is worth keeping visible, because it says
|
||
who removes that line. They are dead **by construction**: no `ModelCommand` can
|
||
ever name `Save`/`SaveAs`/`ExportCsv`, so no resolver would help. `CommandMode`
|
||
is dead **by omission**: `execute-command` is implementable client-side and
|
||
improvise-e24v is scheduled to do it. Listing it is therefore also the named gap
|
||
gotcha 47 asks for — closing e24v has to come to that line and delete it.
|
||
|
||
### Per-arm notes that would otherwise be re-derived wrongly
|
||
|
||
- **`enter-tile-select`.** `EnterTileSelect` gates on
|
||
`model.category_names().len() > 0`, which counts the **virtual** categories
|
||
too — `_Index`, `_Dim` and `_Measure` always exist — so the gate is
|
||
invariantly true and there is nothing here to mirror it against. Re-deriving
|
||
it from `CategoryTreeCache`, which carries the panel's rows rather than
|
||
`category_names()`, would be a second derivation that disagrees on exactly the
|
||
empty document.
|
||
- **`transpose`.** It used to be emitted alongside a `mark_dirty` this client
|
||
had no wire variant for; since improvise-gv9r dirtiness is raised by the host
|
||
off the effect's own `EffectKind`, so there is nothing to mirror and nothing
|
||
missing. The cursor reset is not this client's to send either: the effect calls
|
||
`App::reset_cursor`, and `adopt_cursor_epoch` broadcasts `Command::ResetCursor`
|
||
to *everyone* in answer — so sending one here would be a second,
|
||
differently-addressed copy.
|
||
- **`toggle-panel-and-focus`.** The interactive arm of `TogglePanelAndFocus`:
|
||
open-and-focus unless this panel is already both, in which case close and
|
||
unfocus. The decision is made in `registry.rs` on the host and here on the
|
||
client, from the same two `ViewState` facts.
|
||
- **`edit-or-drill`.** The host asks whether a regular category sits on
|
||
`Axis::None` and whether the cursor is on a synthetic records cell. This
|
||
client asks neither: the server already published the answer as the `CellId`
|
||
variant, and `doctrine(a_demo_is_checked_against_the_type_not_the_axes)` says
|
||
to read the type rather than re-derive the rule. It reads **`edit_text`, never
|
||
`text`**: the drawn text has been through `format_f64`, so a stored 1200 reads
|
||
`"1,200"` under a comma-grouping format and committing it would store the
|
||
*string* `"1,200"`. `tests/lockstep.rs` found exactly that.
|
||
- **Drill does not go upstream — by *either* door.** Every `Upstream` is a black
|
||
hole, and a key that visibly does nothing is the worst of the three answers.
|
||
Drill is session-local residue and reaching it needs a `CellKey` this client
|
||
cannot build, so the honest answer is to say so: a `SetStatus`, which is a
|
||
view-half command — `Sessions::apply` folds it into the asker's own mirror and
|
||
broadcasts nothing, so it reaches this user and no one else. This argument
|
||
arrived as `edit_or_drill`'s, i.e. about `i`/`a` on an aggregate, and for a
|
||
while it read as doctrine for `>` while `>` was still silent. Both doors now
|
||
refuse, and **they refuse with different sentences on purpose**: "this key is
|
||
not built yet" and "this cell is not yours to write" are different facts.
|
||
- **The commit's write is not spelled again here.** It is
|
||
`Client::typed_cell`'s, the same one click-to-edit uses, so a browser keyboard
|
||
commit and a browser mouse commit cannot disagree about what `""` or `"42"`
|
||
means.
|
||
- **`cell_at_cursor` returns `None` when the selection is scrolled out of the
|
||
window** — not a state the cursor keys can produce, since they scroll to
|
||
follow, but one a projection race can.
|
||
- **`grid_move`'s two empty shapes** are kept apart from "not ours": an empty
|
||
grid has nowhere to move to, and a client that has not announced a viewport
|
||
has been sent no grid either. Neither is a key the host could answer better.
|
||
- **`Quit` has no keymap**, on purpose: `ModeKey::from_app_mode` is exhaustive so
|
||
that a new mode without bindings is a compile error rather than a mode that
|
||
swallows every key.
|
||
|
||
### Browser key → improvise key
|
||
|
||
`to_key_event` returns `None` for two cases the host must never see. A bare
|
||
modifier press (`"Shift"`, `"Control"`, …) is not a keypress. And anything held
|
||
with the platform meta key is left to the browser: `KeyModifiers` has no bit for
|
||
Meta, so forwarding `Cmd-R` would arrive as a plain `r` and type into a cell
|
||
instead of reloading the page.
|
||
|
||
Two conversions are not obvious. A terminal reports **Shift-Tab** as its own
|
||
code and the keymaps bind that code; the browser reports Tab plus a shift flag.
|
||
And **shift is deliberately dropped for character keys**: the browser already
|
||
applied it (`"A"`, not shift+`"a"`), and the shared keymap's `lookup_local`
|
||
retries character keys without modifiers for that very reason, so keeping the
|
||
flag would only make an exact binding miss.
|
||
|
||
### The pointer: `Click`, and the four rules it obeys
|
||
|
||
A click is resolved here for the same reason a keypress is — a front end paints
|
||
and wires, it does not decide (design-principles §5). `resolve::click` takes a
|
||
`Click` naming a widget and an index, and returns the `Command`s. `web/app.js`
|
||
attaches listeners and knows nothing else; there is one entry point per widget
|
||
on `Client`, which was the user's ruling on improvise-7ybz.
|
||
|
||
**Rule 1 — a click navigates to what was clicked, and activates it where
|
||
activation is something the client can express.** Every arm composes the same
|
||
shape: `focus` the panel, `SetPanelCursor` to the row, then whatever `Enter`
|
||
does on that row. The activation is resolved **against the clicked index, never
|
||
against the cursor**, because the `SetPanelCursor` ahead of it has not been
|
||
folded when the resolver runs — the trap that reads as "the cursor lands on
|
||
`Plan` and the document switches to `Default`", silent and wrong in the half a
|
||
user does not watch.
|
||
|
||
The second clause of rule 1 is what makes the formula panel look inconsistent
|
||
and is in fact the rule holding. `ModeKey::ViewPanel` binds `Enter` to
|
||
`switch-view-at-cursor`; `ModeKey::FormulaPanel` binds `a`/`n`/`o` and
|
||
`d`/`Delete` and **no `Enter` at all**. So a view-row click switches and a
|
||
formula-row click only navigates. Opening the editor on a formula click would
|
||
be an activation *invented for the pointer that the keyboard does not have* —
|
||
a second derivation of what a formula row means, which is the disease §5 names.
|
||
**The guard for this must be aimed at the keymap, not at the click**: asserting
|
||
that the click emits no `FormulaEdit` only restates the author's reading, and a
|
||
reviewer demonstrated the gap by adding `Enter -> enter-mode formula-edit` to
|
||
`FormulaPanel` and watching the whole workspace stay green while the click and
|
||
the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`
|
||
asserts the binding's absence and names `click_formula_row` as what must change
|
||
if it ever appears.
|
||
|
||
**Rule 2 — a click is all of its intents or none of them** (improvise-hkr8).
|
||
Each `click_*` returns `Option<Vec<Command>>` and `click` does the single
|
||
`unwrap_or_default`. The case that matters is an activation the client cannot
|
||
compose: `filter_to_item_at` goes through `model`, which goes through
|
||
`BasisCache::claim`, which returns `None` until the server has published a basis
|
||
(gotcha 43). Before the fix the `None` was swallowed per-intent, so the click
|
||
kept its navigation prefix, dropped its activation, and reported success — while
|
||
the *keyboard* handled the identical `None` correctly by answering
|
||
`KeyOutcome::Upstream`. Note that `None` at the `click` level means something
|
||
different and benign: the whole click composed nothing, which is the invariant
|
||
rather than a breach of it.
|
||
|
||
**Rule 3 — a click on an *unselected* tile never mutates the document.** The
|
||
user's ruling on the tile bar. The cost accepted is two clicks for the common
|
||
action and a **deliberate divergence from the keyboard**, where `Enter` cycles
|
||
directly — record it as chosen, not as an inconsistency to tidy, or someone will
|
||
read the binding and "fix" the pointer to match. What it buys is worth more than
|
||
the keystroke: a tile cycle is a `LayoutGeometry` change, which bumps
|
||
`cursor_epoch` and therefore resets **every connected client's cursor** (gotcha
|
||
24), so a stray click used to cost every other viewer their position.
|
||
|
||
**State the rule this precisely, because the looser wordings are false.** The
|
||
implemented rule is "clicking the already-selected tile cycles it; clicking any
|
||
other selects it", where *already selected* is `mode == TileSelect` **and**
|
||
`tile_cat_idx == index`. The tempting summaries — "a click never mutates on its
|
||
own", "a **second** click cycles" — both have the same counterexample, found on
|
||
review: `T` enters `TileSelect` leaving `tile_cat_idx` at **0**, so a click on
|
||
the **leftmost** tile is a click on the already-selected one and cycles on the
|
||
*first* gesture. Two adjacent tiles therefore answer the identical first click
|
||
differently, and that is correct rather than a hole — a click does what `Enter`
|
||
does there, and `Enter` in `TileSelect` cycles the tile under the cursor. The
|
||
resolver cannot see *how* the state arose and must not try to.
|
||
|
||
The mode conjunct is still load-bearing in the other direction: without it,
|
||
`tile_cat_idx == 0` in **Normal** mode would read tile 0 as already selected, and
|
||
the very first click of a session on the leftmost tile would cycle it.
|
||
|
||
**Rule 4 — the shared helper is the idiom, and breaking it is how the two arms
|
||
drift.** `click_category_row` calls `filter_to_item_at`, `click_tile` calls
|
||
`tile_axis_at`, `click_view_row` calls `switch_view_at` — each is `foo_at(index)`
|
||
with the keyboard's `foo_at_cursor` calling `foo_at(cursor)`. `click_view_row`
|
||
originally re-spelled `SwitchView + ChangeMode(Normal)` inline, so a host-side
|
||
change to `SwitchViewAtCursor` was forced into the keyboard arm by
|
||
`tests/lockstep.rs` and left the click behind **silently**. A helper that returns
|
||
only the *name* rather than the whole activation is what invites that.
|
||
|
||
### The category panel opens at launch, in both front ends
|
||
|
||
`ViewState::default()` sets `category_panel_open: true`. The user ruled this
|
||
twice: first that the sidebar be visible at launch, then — asked whether the
|
||
terminal was collateral — that it applies to **both**. `improvise model.improv`
|
||
opening with the sidebar showing is intended, not a side effect of sharing a
|
||
default. It is the **shared** default for the reason wsaw gives: a browser-only
|
||
default would be "the page overrides the flag", which is the same defect as "the
|
||
page ignores the flag" wearing different clothes.
|
||
|
||
**A launch default disarms every test that asserted the thing it now supplies**,
|
||
and that is the expensive part. Three tests asserted `category_panel_open` was
|
||
*true* after doing something to make it true; all three then passed without the
|
||
code under them working. The worst was `set_panel_open_and_cursor`: with it
|
||
vacuous, `effect::SetPanelOpen`'s Category arm could be replaced by a **no-op**
|
||
— `C` doing nothing in the terminal — with the whole workspace green. The rule
|
||
this leaves: **assert the direction the default cannot supply.** Closing the
|
||
panel is the discriminating move; opening it proves nothing.
|
||
|
||
Note the two mappings that had to be repaired separately. `effect::SetPanelOpen`
|
||
and `reduce_view`'s `Command::SetPanelOpen` are two three-way matches in two
|
||
crates — the TUI folds one and the browser the other — so a guard on either says
|
||
nothing about the other. That is a derivation done twice (§5), pre-existing and
|
||
filed rather than fixed here.
|
||
|
||
Two tests, because either alone passes against the bug wsaw fixed: the panel is
|
||
visible with no keypress, *and* `C` still hides it. Injecting "the page ignores
|
||
the flag" fails the second; injecting "the default is false" fails the first.
|
||
A test that only asserts the sidebar is visible cannot tell the flag being
|
||
honoured from the flag being ignored.
|
||
|
||
---
|
||
|
||
## `improvise-wasm-client` — the crate, the fold, the render model
|
||
|
||
### What the crate is, structurally
|
||
|
||
One `wasm32` artifact, reused literally in both deployments: the thin client
|
||
talking to a server over a WebSocket and the standalone app talking to a worker
|
||
that hosts the `App` over the worker's own port. Only the JS bootstrap differs —
|
||
nothing in the crate knows what the transport is.
|
||
|
||
It is a **fold and a key classifier**, and nothing else. It holds a `ViewState`
|
||
plus one widget cache each — there is no aggregate render cache — and it moves
|
||
them by folding the same `Command` stream the host folds. The fold is `state.rs`;
|
||
the key classifier is `keys.rs`.
|
||
|
||
The argument that shaped it: a keymap binding names an **intent** resolved
|
||
against a `CmdContext`; a `Command` is the **resolved mutation**. A thin client
|
||
holds no `Model` and no `GridLayout`, so it cannot do that resolution. (Since
|
||
improvise-bpq2 the *table* is shared and the resolution of the composable subset
|
||
happens here; what is left upstream is the subset `keys.rs` names.)
|
||
|
||
That is why the command crate is a **dev**-dependency and not a dependency: its
|
||
keymap could not be used here even if the bundle could afford `App`, `Workbook`,
|
||
persistence and the import wizard. What the dev-dependency buys is
|
||
`tests/lockstep.rs`, which drives the authoritative host and this client over the
|
||
same keypress and asserts the two `ViewState`s agree — the correspondence is
|
||
checked rather than asserted in prose.
|
||
|
||
**The boundary is JSON**, so the JS side needs no generated structural bindings
|
||
and the same strings go on a socket unchanged. No `std::fs` (it compiles for
|
||
wasm and fails in a browser) and no clock: nothing here calls `Instant::now()`,
|
||
so the `web-time` shim the command crate needs is not needed. Both are
|
||
structural, not disciplined — this crate depends only on `improvise-protocol`,
|
||
`serde`, `serde_json` and `wasm-bindgen`, none of which reaches a filesystem or
|
||
a clock.
|
||
|
||
**Ordering.** The JS bootstrap must deliver frames to `Client::on_frame` in
|
||
arrival order, one at a time. The grid fold is deliberately not commutative — a
|
||
`GridUpdate::Cells` that overtakes the labels framing it is dropped — so batching
|
||
or reordering silently loses cells. For the same reason a locally-resolved
|
||
command and a forwarded keypress must go out on the *same* ordered channel, in
|
||
the order `Client::on_key_event` returned them.
|
||
|
||
**Why the `wasm_bindgen` surface is a thin wrapper over a plain-Rust one.**
|
||
`wasm_bindgen` methods cannot take or return the protocol's types, so there is
|
||
one code path serialized at the edge. The split is not only ergonomic:
|
||
`JsError::new` panics off wasm, so every fallible path has to be *reachable*
|
||
without it or its failure arm cannot be tested at all.
|
||
|
||
**`Client::viewport` vs `Client::set_viewport`.** `viewport` is associated
|
||
rather than a method, because a page must send the handshake *before* it has
|
||
anything to fold. But it builds the frame and forgets it, so a client driven
|
||
only through it never learns its own window and therefore resolves no cursor key
|
||
locally — a truthful subset (those keys go upstream and the host answers
|
||
nothing) but a whole feature quietly off. `set_viewport` is the one a page should
|
||
call. The window is **not derivable** from anything the host sends: the row
|
||
frame's own length is `min(window, grid − offset)`, so it agrees with the window
|
||
except near the bottom edge, which is precisely where the scroll arithmetic
|
||
matters. `viewport` is kept because `web/app.js` already spells it and retiring
|
||
it is a co-ordinated change.
|
||
|
||
### `improvise-protocol/src/client.rs` — the two properties this dispatch owes
|
||
|
||
Both are tested in `state.rs` rather than assumed from the protocol crate's own
|
||
tests, because the dispatch between them is this crate's.
|
||
|
||
- **The stream is not reordered.** Every fold path is a `for` over arrival
|
||
order; there is no batching, sorting or coalescing.
|
||
- **Bootstrap is a compaction, not a replay.** `ClientState::bootstrap` folds
|
||
`Snapshot::seed` exactly as the steady stream is folded, so there is no second
|
||
code path that could disagree with the incremental one. `GridCache::seed` puts
|
||
the labels before the cells for the same ordering reason.
|
||
|
||
**`window` is deliberately not reset by `bootstrap`**: the browser window did
|
||
not change size because the document was replaced, and clearing it would
|
||
silently stop this client resolving its own cursor keys until the next resize.
|
||
|
||
**`last_rejection` is kept as a value, not a sentence.** A rejection is the
|
||
input to *force*, and force is a re-composition against the republished basis —
|
||
which needs to know **which** record and **which** kind. `RecordGone` cannot be
|
||
forced at all; `RecordMoved` can be forced with one keystroke. A string cannot
|
||
carry that. It is session-local: it belongs to whoever composed the refused
|
||
batch, so it is not part of the portable slice and does not survive a bootstrap.
|
||
|
||
`window` is session-local and not portable for the same family of reasons: it is
|
||
a fact about a browser window, which is why no `Command` carries it and why it is
|
||
not on `ViewState`. `(0, 0)` means this client has not announced one yet, and a
|
||
client in that state resolves no cursor key — it has been sent no grid either.
|
||
|
||
### `socket.rs` — the frames, the wording, and the one composition
|
||
|
||
**Where the frames live: not here.** `ClientFrame`/`ServerFrame` are
|
||
`improvise-protocol`'s, and the same pair frames both deployments. This crate
|
||
used to carry two envelope pairs of its own: one *mirroring*
|
||
`improvise-ws-server`, because a native crate cannot be a dependency of a wasm
|
||
bundle, and one written for the worker before the projection architecture
|
||
landed. The mirror was held honest by two tests round-tripping every variant
|
||
through the server's own types; with one definition there is nothing left for
|
||
them to compare, which is what improvise-xks6 was for.
|
||
|
||
**`rejection_message` is spelled here rather than on `Rejection`** for the
|
||
reason `AxisColor` is not a ratatui colour: the protocol carries the *fact*, and
|
||
each front end words it. The wording tracks ADR 001's force table, because "try
|
||
again" and "this cannot be forced" are the two different things a user must be
|
||
able to tell apart — a `RecordGone` has nothing to write to, and offering force
|
||
there would promise something the design refuses to deliver (**force never
|
||
creates structure**). Two wordings are load-bearing: `DocumentReplaced` must not
|
||
say "reloading", because nothing here reloads, and the two save failures are not
|
||
staleness so neither offers a retry — retrying changes nothing about the host's
|
||
storage.
|
||
|
||
**`typed` is a rule that genuinely lives in two places.** It is the same
|
||
three-way split `commit_regular_cell_value` makes — empty clears, a parseable
|
||
number is a number, anything else is text. It is duplicated here for the reason
|
||
the whole client exists: the browser has no `App` to ask, and
|
||
`ModelCommand::SetCell` takes a `CellValue`, so *somebody* on this side must
|
||
decide. Putting it in Rust rather than in the page's JavaScript is what makes it
|
||
testable and keeps one spelling per language boundary instead of two.
|
||
|
||
This is the **only** answer the *client* gives to "what does a typed string
|
||
mean" (improvise-r4lo), and it holds on every deployment including the worker: a
|
||
host's `App` sits behind the frame boundary and is handed a value, not a string,
|
||
so no host ever classifies a string that arrived *over the wire*. The careful
|
||
half: a host does classify its own **local** keystrokes, which is why this is a
|
||
duplicate of that split rather than a call to it — the command crate is
|
||
deliberately not a dependency, and reaching it would drag `App`, `Workbook` and
|
||
persistence into the browser bundle.
|
||
|
||
So `a_typed_string_is_classified_as_the_host_classifies_it` **states the
|
||
obligation without discharging it**: it asserts the three outcomes by hand
|
||
rather than comparing with the host's function, so the two could drift and stay
|
||
green. That is
|
||
`doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)` still
|
||
open, one layer below what improvise-r4lo closed — the real fix is a shared
|
||
split in `improvise-protocol`, which both crates already depend on. Filed rather
|
||
than done.
|
||
|
||
`typed`'s `None` means "clear this cell", which is a *different command*, not a
|
||
different value — hence the `Option` rather than a `CellValue::Text("")`.
|
||
|
||
**`typed_cell_frame` is the composition improvise-q08 found a thin client could
|
||
not do.** It holds no `Model`, so it cannot build a `CellKey` — but it does not
|
||
need to: the id on the cell it is already drawing *is* the address, and it is
|
||
echoed back unread (ADR 002). It takes the user's **string**, not a `CellValue`,
|
||
for the reason above; there is no deployment for which a pre-classified entry
|
||
point would be right. A sibling taking a serialized `CellValue` used to sit
|
||
beside it and claim the opposite; it was deleted rather than re-documented,
|
||
because it could not express `ClearCell` and was therefore a strictly weaker
|
||
duplicate.
|
||
|
||
`None` on **four** shapes, each a different fact the client already holds: the
|
||
position is outside the grid; the position holds no settable cell (`id: None` —
|
||
padding, or a records-mode column that renames a coordinate rather than writing
|
||
one); the cell is an **aggregate**, which is the pattern failing to bind rather
|
||
than a check; or this client has not been told the document's basis and so has
|
||
nothing truthful to say about what it composed against (`BasisCache::claim`
|
||
withholds rather than invents).
|
||
|
||
**Nothing is folded there.** The new value comes back as a `GridUpdate::Cells`
|
||
from the host, which is the side that formats it; guessing it locally is how the
|
||
two ends drift apart.
|
||
|
||
`typed_cell_command` is the one composition shared by the mouse path
|
||
(`typed_cell_frame`) and the keyboard path (`keys::commit`). Writing it twice is
|
||
how a browser commit by Enter and a browser commit by click would come to
|
||
disagree about what an empty string means.
|
||
|
||
### `render.rs` — the four decisions that are silent when wrong
|
||
|
||
The DOM half of this client is JavaScript. The split is drawn to make that
|
||
acceptable rather than comfortable: **every question with a wrong answer is
|
||
answered here, in Rust, under test**, and what crosses to the page is a list of
|
||
rows of cells that each already know whether they are editable, numeric,
|
||
erroneous and selected. The page's job is `document.createElement`.
|
||
|
||
- **Editable is a fact about the id, not about the text.** Only
|
||
`CellId::Record` may be written (ADR 002), and there are now **four** kinds,
|
||
each naming a permission. `CellId::Aggregate` is a *drill* target and `Drill`
|
||
is not on the wire yet, so it renders as non-editable rather than as a control
|
||
that does nothing. `CellId::Derived { target }` is a cell a formula owns
|
||
(improvise-ir7l): `i` on one names the formula rather than opening an editor,
|
||
because a write there lands and is then shadowed forever by `formula_cache`.
|
||
`CellId::Create(kind)` is a records-mode `+`/`+cat` column (improvise-poia),
|
||
where what the user types is a **name**, not a value. And `None` is **not**
|
||
any of those — it is "no projected cell here": a padded slot, a blanked one,
|
||
or a records-mode column that renames a coordinate across the whole row rather
|
||
than writing one.
|
||
- **The selection is absolute and the grid is not.** `GridCache` is indexed by
|
||
position *within the viewport*, while `selected` and `row_offset`/`col_offset`
|
||
are positions within the whole grid. Reconciling the offset is the entire
|
||
conversion, and the trap is that forgetting it looks right at the top of a
|
||
document and wrong after the first scroll. `render.rs` does it by *adding* —
|
||
`r + cursor.row_offset == sel_row` — so there is no underflow path and a
|
||
selection above the window simply never matches.
|
||
- **The category panel is `flatten_cat_tree`**, not a second walk of the cache.
|
||
That function exists in `improvise-protocol` precisely so this end and
|
||
`move-panel-cursor` share one flattening (improvise-edp); the page never sees
|
||
`CategoryTreeCache` at all.
|
||
- **The grid's frame is `AxisFrame`, and every header decision is made here**
|
||
(improvise-anr9). The multi-level header levels, the repeat suppression, the
|
||
group runs and their collapse state come off the same `AxisFrame` methods the
|
||
TUI widget calls, so the page receives labels already blanked where they repeat
|
||
and group runs already spanned. Until this landed the protocol carried
|
||
`GridLayout::row_label`'s slash-joined tuple and the page drew `Food/Alice`
|
||
where the TUI drew two header columns — a derivation the TUI's own test says is
|
||
wrong, shipped to the other front end.
|
||
|
||
Nothing in `RenderModel` is on the wire. It is a *projection of a projection* —
|
||
the client's own caches shaped for one renderer — so it may change freely without
|
||
a protocol conversation.
|
||
|
||
Four smaller decisions that would be re-derived wrongly:
|
||
|
||
- **`RenderRow` is an enum**, not a `label` plus an `is_group` flag, so a group
|
||
header cannot carry cells and a data row cannot carry a collapse state. It is
|
||
serialized with a `kind` tag because the consumer is JavaScript. `labels()` is
|
||
**empty for a group header**, which carries its label in its own variant rather
|
||
than in a one-element tuple — so a caller walking `labels()` over the whole
|
||
model sees the data rows and nothing else, which is exactly what
|
||
`neither_front_end_produces_a_slash_joined_label` wants. A caller that means
|
||
"every label on screen" has to match on the variant.
|
||
- **`RenderColGroup::label` is an `Option`, not an empty string.** An empty
|
||
string is a legal group *name* — `Category::add_item_in_group` accepts one, and
|
||
the TUI draws its indicator like any other — so a sentinel would serialize that
|
||
group identically to an ungrouped run and the page would drop its indicator
|
||
(raised by Copilot on PR #23). `None` for an ungrouped run also keeps the runs
|
||
a *partition* of the columns: the spans sum to the column count, so a page
|
||
laying them out as `colspan` cannot leave a hole.
|
||
- **Column group runs coalesce by NAME alone**, which is what `src/ui/grid.rs`
|
||
does (`group_name != prev_group`), and the two must agree. Collapse is
|
||
category-scoped, so two adjacent groups from different categories may legally
|
||
share a name and differ in it; keying on the pair would start a second run the
|
||
TUI suppresses. The run's indicator is its FIRST group's, again as the TUI
|
||
paints it. `col_groups` returns nothing at all when no column is in a group,
|
||
because "this grid has no column groups" and "every column's group is blank"
|
||
are different statements and the page draws a line for only one of them — the
|
||
same asymmetry `GridCache::totals` has.
|
||
- **`hint` and `editing` are decided here, not in the page.** `hint` is
|
||
`AppMode::hint_text()`, the same string the TUI's footer draws, so the page
|
||
spells no keys of its own — which is what keeps a second front end from growing
|
||
its own idea of the bindings, and makes the modes this client can *enter* but
|
||
not yet *draw* (`Help` above all) state what they are instead of looking like a
|
||
broken grid. `editing` is `AppMode::is_editing()` plus the `edit` buffer:
|
||
without it, a browser that resolved `i` locally would change its mode indicator
|
||
and then swallow every keystroke invisibly.
|
||
- **`minibuffer` is the same argument, for the other six text-entry modes**
|
||
(improvise-fdt6). `editing` drives the *inline cell editor*, so it is scoped to
|
||
the two cell-editing modes and reads the `"edit"` buffer by name — which left
|
||
FormulaEdit, CategoryAdd, ItemAdd, SavePrompt, ExportPrompt and CommandMode
|
||
opening a prompt and swallowing every character typed into it. `minibuffer` is
|
||
`AppMode::minibuffer()` plus **the buffer that config names**, read with
|
||
`improvise_protocol::read_buffer` so the `"search"` special case keeps its one
|
||
home (gotcha 11); `None` is "no text-entry mode is open", so the page asks
|
||
nothing about the mode. The colour stays semantic — `MinibufColor` crosses and
|
||
the CSS class is the page's last translation, exactly as `draw.rs::minibuf_fg`
|
||
is the TUI's (gotcha 26). It is drawn as a line of its own rather than *in
|
||
place of* `#status` as the TUI's bottom bar does, because `#status` also
|
||
carries the transport's failures and a repaint must not erase one (gotcha 48).
|
||
|
||
`RenderCell::edit_text` is the stored value, not the formatted one: seeding an
|
||
editor from `text` and committing turns `1200` into the string `"1,200"`. The
|
||
`numeric`/`error` flags are the server's answers, echoed — the client must not
|
||
re-derive them from the text, which is how the two ends drift.
|
||
|
||
`render_model` builds `header_labels(0)` **once for the whole frame**, because
|
||
the per-position accessors scan and asking them inside the row loop is quadratic.
|
||
|
||
---
|
||
|
||
### `render.rs` and the help overlay (improvise-ktv1)
|
||
|
||
`RenderModel` carries `help`, `help_page` and `help_titles`, and the decision
|
||
the page must not make is *whether the overlay is open*: an **empty `help` is
|
||
what says it is closed**. That is the shape `col_groups` already uses for "this
|
||
grid has no column groups" — an absent list rather than a flag the page has to
|
||
interpret alongside a mode it would then have to understand.
|
||
|
||
The rows themselves are `improvise_protocol::help::pages()`, the same ones
|
||
`src/ui/help.rs` draws. `RenderHelpRow` carries a `kind` string
|
||
(`heading`, `keys`, `accent`, …) which is a *semantic* name the page maps to a
|
||
CSS class — the `AxisColor → axis_fg` shape again, and the reason the page can
|
||
style a heading without knowing what makes a line a heading. Where the content
|
||
comes from and what is derived from the keymap is `context/code/protocol.md` §8.
|
||
|
||
## `web/app.js` — the page
|
||
|
||
The browser half of improvise-cr3, kept as thin as it can be made, now talking
|
||
to either of two peers (improvise-djm). **The boundary is JSON strings in and
|
||
out, so this file holds no protocol knowledge at all**: it never inspects a
|
||
`Command`, never builds a `CellId`, never decides whether a cell is editable or
|
||
whether a typed string is a number. Five calls are the whole harness:
|
||
|
||
| call | what it is |
|
||
|---|---|
|
||
| `client.set_viewport(rows, cols)` | the handshake frame, and every resize; also records the window so keys resolve here |
|
||
| `client.on_frame(json)` | fold one frame, in arrival order |
|
||
| `client.render_model()` | everything to paint, already decided |
|
||
| `client.typed_cell(r, c, s)` | the frame for "the user typed s here" |
|
||
| `client.on_key_event(k,c,a,s,m)` | the frame for a keypress, folded locally first where this client may answer it |
|
||
|
||
**The two rules this file must not break are ordering and framing**: frames are
|
||
folded one at a time in the order they arrive (a cell update that overtakes its
|
||
labels is dropped, not replayed), and the *first* frame out must be the
|
||
viewport, because the server builds the snapshot at it.
|
||
|
||
Everything in this file that is a *decision* rather than a paint now lives in
|
||
`web/shell.js` — `socketUrl`, the `#status` composition, the watchdog, the
|
||
extent arithmetic and `gridPlan` — so that `node --test` can reach it without a
|
||
browser. The sections below describe those rules; the code is one file over.
|
||
|
||
### Two transports, one client
|
||
|
||
The wasm bundle is the SAME artifact either way (improvise-xks6: one envelope
|
||
pair, two deployments). Only where the strings come from differs, so the
|
||
difference is confined to `openTransport` and nothing below it knows which peer
|
||
it has:
|
||
|
||
- **default** — an in-page Web Worker hosting the whole `App` over OPFS
|
||
(improvise-9x6). No server process; this is what makes a GitHub Pages copy of
|
||
the directory a working application.
|
||
- **`?ws=host:port`** — a websocket to `improvise-ws-server`. That process is how
|
||
a document opened in the TUI is shared, so the path is kept; **the presence of
|
||
the parameter is the whole selection mechanism**, because a second knob naming
|
||
the same fact is a second thing to get wrong.
|
||
|
||
**Neither is reachable silently.** Every transport writes what it is doing to
|
||
`#status` — "starting", "connected", "cannot reach …", "disconnected" — so a
|
||
peer that never arrives reads differently from one that went away.
|
||
"disconnected" means exactly the second: a socket that opened and then closed. A
|
||
socket that never opened keeps saying "cannot reach …", because the close event
|
||
there is the tail of the failure and not news.
|
||
|
||
`#status` carries **two independent facts** — what the transport is doing and
|
||
what the document last said — so it composes them rather than letting a repaint
|
||
quietly erase a connection error.
|
||
|
||
### `socketUrl`: `wss://` is the default
|
||
|
||
A bare `host:port` gets `wss://`, because the address that works from where this
|
||
page is actually served is the one it should be easiest to write. The page ships
|
||
to GitHub Pages, which is https, and an https origin refuses a `ws://` address as
|
||
mixed content — so a `ws://` default would hand the browser a URL that cannot
|
||
work and report it as a connection failure, which reads like "the server is not
|
||
running".
|
||
|
||
An explicit `ws://` is honoured only where it can function: a page served over
|
||
plain http, which is the local `python3 -m http.server` case that pairs with
|
||
`improvise-ws-server` (which speaks plain `ws://` only — it has no TLS, so
|
||
reaching it from https means terminating TLS in front of it). From an https page
|
||
an explicit `ws://` is **upgraded** rather than passed through, since passing it
|
||
through can only produce a failure the user cannot act on.
|
||
|
||
### `set_viewport`, not `Client.viewport`
|
||
|
||
Same frame, but it also **records** the window. A client learns its window only
|
||
from what it announced — the row frame's own length is the window clipped to the
|
||
grid, so the two differ exactly at the bottom edge, which is where the scroll
|
||
arithmetic matters. Without it the client cannot clamp, so every cursor key goes
|
||
upstream into `Host::handle`'s empty vec and the arrow keys silently do nothing
|
||
(improvise-bpq2).
|
||
|
||
### `frameCost`: what the grid's own frame costs, in whole cells
|
||
|
||
One header row and one header column was right until improvise-anr9: the column
|
||
header is now one row PER LEVEL plus an optional group row, the row header is
|
||
`row_levels` columns wide, and inline group headers take a row each. Reserving
|
||
one of each announces a viewport the page cannot show, and `#grid` has
|
||
`overflow: auto`, so the DOM would acquire a scroll position of its own while the
|
||
client's cursor offsets were computed from the larger window — putting the
|
||
selected cell off screen.
|
||
|
||
It is **measured off the painted model rather than predicted**, and defaults to
|
||
the old one-and-one before the first paint, which is the only moment there is
|
||
nothing to measure.
|
||
|
||
**Inline group ROWS are deliberately NOT reserved.** They take real height, but
|
||
how many are in view depends on how many rows the window asked for, so every way
|
||
of counting them is either a *feedback loop* (count the current ones: a smaller
|
||
window crosses fewer groups and asks for a larger one) or a *ratchet* (keep the
|
||
maximum: a group-dense grid stores an oversized cost on its first oversized
|
||
projection and never lets it fall — one group header per row clamps the grid to a
|
||
single row, permanently). Both were tried. What is left is an over-request
|
||
bounded by the groups in view: a grouped grid asks for a few rows more than the
|
||
table shows, which matters only for a selection near the bottom edge — and that
|
||
gap is not new and not specific to groups, since nothing in this page calls
|
||
`scrollIntoView` at all. Filed as its own ticket rather than answered with a
|
||
heuristic in a file the harness only partly executes.
|
||
|
||
### The watchdog, and the two `close` arms
|
||
|
||
**The handshake watchdog is about the *first frame*, not about either peer's own
|
||
failure modes.** The socket has an `error` event for a peer that never answers;
|
||
the worker has none that fires, because `worker.js` catches its own `start()`
|
||
rejection — so a browser with no OPFS, or a worker bundle that failed to load,
|
||
would leave the page saying "in-page worker" over an empty grid forever. "The
|
||
peer never answered the handshake" is the one condition that means the same thing
|
||
in both deployments, and a check aimed at each peer's own failure modes would have
|
||
missed exactly this one.
|
||
|
||
**A peer that failed *loudly* has already said something more precise, so the
|
||
watchdog must be DISARMED and not merely pre-empted**: it fires later and would
|
||
overwrite the better message with "did not answer" (PR #13 review). The watchdog
|
||
is for silence only, and `setShellFailure` clears the timer.
|
||
|
||
**Did this socket ever open?** The page has to record that itself, because
|
||
nothing on the close event carries it: `readyState` is already CLOSED by the time
|
||
a close listener runs, whichever happened, and a refused connection and a
|
||
mid-session network drop are BOTH code 1006 with `wasClean: false`. So "never
|
||
connected" and "was connected and went away" are distinguishable only by
|
||
remembering — an `opened` flag is the only discriminator.
|
||
|
||
A socket that never opened did not "disconnect": it failed to connect, and the
|
||
close event is the tail of that failure rather than a second, vaguer thing that
|
||
happened (improvise-taxx). Writing "disconnected" there unconditionally replaced
|
||
the `error` listener's precise message with the least informative of the two
|
||
facts.
|
||
|
||
**That is the same family as the watchdog bug and NOT the same fix.** The
|
||
watchdog is disarmed because it is for silence only, whereas suppressing `close`
|
||
outright would hide a genuine mid-session disconnect, which is the one thing that
|
||
listener is really for. Hence the split rather than a guard. **Both arms go
|
||
through `setShellFailure`, which disarms the watchdog** — and that half is the one
|
||
that keeps getting lost. It matters on the opened path: the watchdog is armed
|
||
after `transport.ready` resolves, i.e. on `open`, so a socket that opened and then
|
||
dropped before its first frame reaches the close listener with the timer still
|
||
pending, and "disconnected" is more precise than the guess it would otherwise fire
|
||
8s later. On the never-opened path there is no timer yet and the clear is a no-op.
|
||
Neither arm leaves one armed.
|
||
|
||
### Painting
|
||
|
||
- The **column-group line** is drawn only when there is one: an empty
|
||
`col_groups` means this grid has no column groups at all, not that every group
|
||
is blank. `label` is `null` for a run in no group, and an empty STRING is a
|
||
legal group name that still gets its indicator — which is why the page does a
|
||
null check and not a truthiness test.
|
||
- **One header line per level**, and a blank entry is *repeat suppression*
|
||
decided in Rust, never an absent label.
|
||
- **The cell coordinates a click sends are DATA row indices**, and a group header
|
||
is not a data row, so the page counts rows rather than trusting the position in
|
||
`model.rows`.
|
||
- `r` is one binding shared by every closure in the row loop, so a click handler
|
||
that captured it would fire with the LAST row's index. It is snapshotted.
|
||
- **A keyboard edit is open when `model.editing` is a string**, and it holds what
|
||
has been typed so far. Both facts are decided in Rust — the page does not know
|
||
which modes are editing modes or which buffer they type into. Without it the
|
||
mode indicator would say INSERT while every keystroke vanished into a buffer
|
||
nobody drew.
|
||
- **`editable` is the server's answer relayed through Rust**: a stored cell. An
|
||
aggregate is a drill target and drill is not on the wire yet, so it is simply
|
||
not offered.
|
||
- The inline editor seeds from `RenderCell::edit_text` — the STORED value, not
|
||
the drawn one: `td.textContent` has been through the number format, so a stored
|
||
1200 reads `"1,200"` and committing that stores the *string* `"1,200"`. Enter
|
||
removes the focused `<input>`, which fires `blur`, which calls `close` again:
|
||
harmless, but only by accident.
|
||
|
||
### The keyboard
|
||
|
||
`Client::on_key_event` classifies the event and folds it locally where this
|
||
client may answer it — text entry, and since improvise-bpq2 the grid, panel and
|
||
tile cursors too, which is why `sendViewport` must call `set_viewport` and not
|
||
`Client.viewport`. Anything it cannot answer becomes a `ClientFrame::Key`, which
|
||
`Host::handle` answers with an empty vec: those keys are sent and do nothing.
|
||
**That is settled rather than unfinished** — the host must not know the keymap —
|
||
and closing one means teaching the client, not that arm.
|
||
|
||
The page does not claim a key it did not answer: a frame means the page claimed
|
||
it, so the browser must not also act on it — an arrow that both moves the cursor
|
||
and scrolls the page is the worse of the two errors. Combinations under Ctrl or
|
||
Alt are left alone regardless: those are the browser's own shortcuts, and the
|
||
client resolves nothing under them. `BrowserKey` drops anything held with the
|
||
platform meta key precisely so `Cmd-R` does not arrive as a bare `r`; the page
|
||
does not claim it either.
|
||
|
||
---
|
||
|
||
## `web/worker.js` — the worker half
|
||
|
||
It hosts the authoritative `App`. Everything it decides is decided in Rust —
|
||
the file only moves strings between `self.onmessage`/`self.postMessage` and
|
||
`WorkerServer::on_message`, which is the same shape `web/app.js` has on the main
|
||
thread and for the same reason: the less JavaScript there is, the less can be
|
||
silently wrong.
|
||
|
||
**Two rules it must not break:**
|
||
|
||
1. **Post every reply, in the order the Rust returned them, one message per
|
||
frame.** A `GridUpdate::Cells` that overtakes the labels framing it is DROPPED
|
||
by the client's fold, not replayed. Never batch two calls' replies together
|
||
and never reorder them.
|
||
2. **Await `flush()` after each message.** `Storage::delete` is visible to
|
||
`read`/`list` immediately but only reaches OPFS there.
|
||
|
||
**Rule 1 is about frames within one call's replies AND about the calls
|
||
themselves**, which is why every message is chained onto one tail promise rather
|
||
than handled by an `async` listener: **an `async` listener does not serialize**.
|
||
The event loop is free to start the next `message` while an earlier one is
|
||
suspended at an `await`, so two `on_message`/`flush` pairs interleave and a later
|
||
frame's replies can be posted before an earlier frame's. The chain is also what
|
||
makes "frames that arrived before `start()` finished" an ordinary case of the
|
||
same rule instead of a second code path with its own drain loop — and that loop
|
||
had the race in it twice over, since `server` was set before it ran.
|
||
|
||
The chaining itself is `web/serial.js::serialize`, which is promise plumbing and
|
||
nothing else, so `web/tests/serial.test.js` can force the interleaving a browser
|
||
cannot be asked to produce on demand — including the negative arm, which runs
|
||
the same two handlers *without* the chain and shows them coming back swapped.
|
||
|
||
**The document is named in the worker's own URL** — `new
|
||
Worker('worker.js?doc=demo.improv')` — the analogue of the socket server naming
|
||
it in the `ws://` path. It is deliberately not a `ClientFrame`: the envelope pair
|
||
is shared with the socket deployment and stays exactly as it is (improvise-xks6).
|
||
The seed is fetched and handed to Rust, which **writes** it through the VFS and
|
||
then reads it back like any other document — not injected past storage. A missing
|
||
seed is not an error: with none, the worker creates an empty model under `DOC`.
|
||
|
||
An unhandled rejection inside a `message` listener reaches the devtools console
|
||
and nothing else, so it is caught there — naming which frame it was, which the
|
||
main thread cannot see and has no way to ask — rather than left on the chain,
|
||
where one bad frame would poison every later one.
|
||
|
||
---
|
||
|
||
## `harness/` — the browser instrument
|
||
|
||
### Why it exists, and what belongs in it
|
||
|
||
Before `harness/` there was no JavaScript test infrastructure in the repository
|
||
at all — no `package.json`, no runner, nothing in CI running a single line of
|
||
`web/app.js` or `web/worker.js` — so every bug in them was found by reading and
|
||
the bug-first rule could not be honoured for a change to either. **PR #21
|
||
declined to add regression tests for its close-listener fix on exactly that
|
||
ground, and that decline is only honest because those assertions live here
|
||
instead.**
|
||
|
||
Playwright rather than a headless-DOM runner because what goes wrong in these
|
||
files is a real Worker, a real WebSocket, real OPFS and real event ordering, none
|
||
of which a DOM shim has. That still holds, and it is why no shim was added; what
|
||
changed is the **scope** — see the section below, which is now the answer to
|
||
"where does a change to `web/*.js` belong". It is deliberately small (improvise-vvl3): the point is
|
||
to be able to run `storage_contract` against `OpfsFs` and to read `#status` out
|
||
of a real page, not to grow a second test framework beside `cargo test`. The
|
||
server root is `harness/serve`, built by `build.sh` — the assembled site plus the
|
||
harness's own page; a module script will not load from `file://`, so an http
|
||
server is not optional.
|
||
|
||
### Engine coverage
|
||
|
||
improvise-xf1's acceptance criterion names Chrome, Firefox and Safari, and the
|
||
OPFS spec is the reason: `createSyncAccessHandle` is the one API the whole design
|
||
rests on, its sync read/write/truncate signatures were added later than the
|
||
handle itself, and each engine implemented it separately. Chromium alone would be
|
||
one engine's opinion.
|
||
|
||
**Result today: Chromium and Firefox run the whole battery and pass.**
|
||
Playwright's WebKit provides **no OPFS at all** — `getDirectory()` itself rejects
|
||
with `UnknownError` — so Safari is NOT covered and xf1's criterion is met for two
|
||
engines of three. The project is kept rather than deleted so that it starts
|
||
running the day that build gains OPFS; `opfs.spec.js` skips on a **runtime
|
||
probe**, not on the browser name, so if Chromium or Firefox ever stops providing
|
||
OPFS it fails rather than skipping. An unexpected skip on any other engine is a
|
||
failure — a silently skipped check is exactly what this harness exists to
|
||
prevent.
|
||
|
||
The shell spec is **Chromium-only on purpose**: it is about `web/app.js`'s own
|
||
logic, which is engine-independent, and `routeWebSocket` plus the worker
|
||
handshake are slower and flakier to no benefit across three.
|
||
|
||
### `harness/opfs-contract`
|
||
|
||
A *harness* crate, not part of the application: it ships in no bundle and is not
|
||
a workspace member. `OpfsFs` had been compile-checked and never run; this is the
|
||
instrument that changed that (improvise-vvl3, absorbing improvise-xf1).
|
||
|
||
**Why a worker.** `FileSystemFileHandle.createSyncAccessHandle()` exists only in
|
||
a dedicated Web Worker, and it is the whole reason `Storage` could stay
|
||
synchronous. A check that ran on the main thread would fail for a reason that has
|
||
nothing to do with `OpfsFs`.
|
||
|
||
**Why a report rather than a panic.** Every check runs and its outcome is
|
||
reported, so a failure names *which* step and the rest still run. More
|
||
importantly it is what lets the spec check the checker: Playwright asserts the
|
||
report contains exactly the checks it expects, so a worker that never started, or
|
||
a build that dropped a check, is a failure rather than a silent pass. The list of
|
||
expected check names is spelled in `opfs.spec.js` **as well as** in Rust on
|
||
purpose: a check deleted on one side has to be deleted on both, which is what
|
||
stops the battery quietly shrinking.
|
||
|
||
**`wipe` uses only the backend's own vocabulary** — `refresh`, `delete`,
|
||
`commit_deletes` — rather than reaching for `removeEntry` directly, so a broken
|
||
`delete` shows up there as well as in the contract. It therefore cannot clear a
|
||
zero-byte entry, because `list` no longer names one. That is deliberate and costs
|
||
nothing: `attach` recreates and `write` fills whatever a previous run left behind,
|
||
and a leftover empty entry is invisible to every assertion for the same reason it
|
||
is invisible to a document picker.
|
||
|
||
`opfs_root`/`make_directory` exist only in the harness: `OpfsFs` has no directory
|
||
vocabulary at all — it stores one flat entry per document — so making a directory,
|
||
to check that `refresh` skips one, has to happen outside it.
|
||
|
||
---
|
||
|
||
## Where a change to `web/*.js` is tested
|
||
|
||
**E2E and integration tests are rare and pin boundary contracts; unit tests
|
||
assume those contracts and test between them.** (User ruling, 2026-09-20.) So
|
||
the question is not "can this assertion be made cheaper" but "which boundary is
|
||
it about", and the browser half has four:
|
||
|
||
| boundary | the contract | where it is pinned |
|
||
|---|---|---|
|
||
| `RenderModel` → DOM | every field the render model projects reaches an element | `web/tests/render_surface.test.js` (statically) **and** `harness/tests/contract.spec.js` (on the screen) |
|
||
| JS → wasm | the calls the page makes exist, with the arity the bundle declares, and their answers are painted | `web/tests/bundle/wasm_surface.test.js` (the surface) **and** `contract.spec.js`'s pointer test (the wiring) |
|
||
| page → peer | the handshake goes out first, a frame is answered whole, a failure is named, **and the page routes each failure through the call that disarms the watchdog** | `harness/tests/shell.spec.js`, `harness/tests/drill.spec.js` |
|
||
| key → screen | a key the mode's hint line ADVERTISES produces something the user can see, and a text-entry mode paints what is typed into it | `harness/tests/keys.spec.js` |
|
||
| worker → OPFS | `storage_contract`, run against a real `OpfsFs` | `harness/tests/opfs.spec.js` |
|
||
|
||
**A unit test of a rule does not test the wiring that reaches it**, and the
|
||
sharpest case in this file is the watchdog. `shell.test.js` proves that
|
||
`createShell.fail()` disarms rather than pre-empts; it cannot prove that
|
||
`app.js`'s socket `close` and `error` listeners call `fail()` rather than
|
||
`setShell()`. That routing is PR #21's other half, the one gotcha 48 says keeps
|
||
getting lost — and PR #61 lost it a third time by keeping the unit test and
|
||
dropping the browser assertion, with 26/26 and 13/13 green. Restored, and the
|
||
rule generalises: **when a unit test replaces a boundary test, ask which of the
|
||
two the regression would have been in.** Here it is the routing, so the guard
|
||
stays in Playwright.
|
||
|
||
**The faked clock is faithful outside the window and silent inside it.**
|
||
`shell.spec.js` walks past the 8s handshake watchdog with `page.clock`, and the
|
||
question that owes an answer is whether collapsing ten seconds into one
|
||
synchronous step *flattens* an ordering rather than merely skipping the wait.
|
||
Probed for the sharpest case — a peer that opens, stays silent past the window
|
||
and is closed afterwards — the `#status` transcript is **byte-identical** under
|
||
the fake clock and under a real nine-second wait: four states, same order, same
|
||
text. What that does not cover is an event landing **strictly inside** the
|
||
collapsed window, which a synchronous `runFor` gives no opportunity to deliver;
|
||
that is a race whose outcome the page does not promise, so there is nothing to
|
||
assert. State it rather than re-derive it.
|
||
|
||
**A negative assertion must demonstrate the denied event is reachable on that
|
||
exact code path** — not on a sibling path, and not in the rule's own unit test.
|
||
This file's worked example cost four review rounds. `shell.arm` runs only after
|
||
`await transport.ready`, so on a socket that never opens the watchdog is never
|
||
armed; a test denying "sent no document" there denies nothing, and passed
|
||
against both the routing revert and the disarm regression. `outlastTheWatchdog`
|
||
now asks the page `timesArmed() > 0` first, which turns that review finding into
|
||
a failure by name. Note *ever armed* and not *armed now*: a correct page has
|
||
already disarmed by the time the disconnect test jumps — that is the property —
|
||
so the obvious spelling would have denied the passing case.
|
||
|
||
**And a constant a test must exceed has one home.** `HANDSHAKE_TIMEOUT_MS` lived
|
||
in `web/app.js` and again in the spec, under a comment saying a silent
|
||
disagreement "would pass for the wrong reason". It did: the spec at 1000 against
|
||
the page's 8000 made the suite green over the reverted routing, because the jump
|
||
landed inside the window. It is exported from `web/shell.js` and imported by
|
||
both, so the disagreement is unconstructible rather than discouraged.
|
||
|
||
Everything **between** those seams is `node --test web/tests`, and it may assume
|
||
the boundary holds rather than re-establishing it: transport selection from
|
||
`?ws=`, the two facts `#status` composes, the watchdog's arm and disarm, the
|
||
`frameCost`/extent arithmetic, the collapse glyph, the grid's plan, and the
|
||
worker's tail-promise ordering. None of it needs a DOM, so none of it is
|
||
Playwright's.
|
||
|
||
**The seam that makes this possible is `web/shell.js` and `web/serial.js`**:
|
||
pure modules that **return a description** rather than take a DOM double.
|
||
`gridPlan(model)` answers every question `gridTable` used to answer while it was
|
||
creating elements — which row index a click carries when group headers are
|
||
interleaved, what a group header spans, whether a `null` group label is a name,
|
||
whether an empty edit buffer means an editor is open — and `app.js` is then a
|
||
loop that creates elements from it. Painting stays untested by Node; the
|
||
decision about *what* to paint stopped being untestable.
|
||
|
||
### Why the four widgets were one bug
|
||
|
||
The tile bar, the view list, the formula panel and the category sidebar
|
||
ignoring `category_panel_open` were reported as four broken features and fixed
|
||
as four. They were **one failure at one boundary**: a `RenderModel` field with
|
||
no element painting it. Four per-feature specs cost four page loads and catch
|
||
the fifth only if somebody writes a fifth spec; one assertion that *every*
|
||
projected field is painted catches it by construction. That assertion is cheap
|
||
enough to make twice, from different sides:
|
||
|
||
- **`render_surface.test.js`** reads the `pub` fields off
|
||
`improvise-wasm-client/src/render.rs` and requires each one to be read by
|
||
`app.js` or `shell.js` — or named in `UNPAINTED` **with a reason**. It needs
|
||
no browser, no bundle and no build, so it fails in seconds. The list is not
|
||
an escape hatch: it is the debt, written down, and it currently holds
|
||
`doc_name`/`doc_dirty`/`doc_stored_as` (improvise-5jna — the fifth instance,
|
||
found by writing this test).
|
||
- **`contract.spec.js`** drives the demo document into each state that fills a
|
||
widget and asserts the element is *on the screen*. Node cannot see that, and
|
||
the moment a reduction would have to weaken it, it stays here.
|
||
|
||
**And the static half is weaker than it reads, which is worth knowing before
|
||
leaning on it.** `render_surface.test.js` asks whether the string
|
||
`model.<field>` occurs anywhere in the page's code — so a field *mentioned* by a
|
||
function nothing ever calls passes. That happened on improvise-fdt6: the new
|
||
`minibuffer` field had a `paintMinibuffer` reading it, and the call site and the
|
||
`getElementById` were both missing, so the node suite went green over a page
|
||
that still painted nothing. `keys.spec.js` failed on its first run, in Chromium,
|
||
naming the hidden element. The static check is a **completeness** check — it
|
||
tells you a field was not forgotten — and never an **execution** check; only
|
||
`harness/` is that (gotcha 48).
|
||
|
||
What is deliberately **not** in Playwright any more is what each key or click
|
||
*means*: `improvise_protocol::resolve` decides that and its tests are where the
|
||
meaning is pinned. One instance of "a resolved change repaints" is kept, because
|
||
the repaint is the boundary; the other seven were the same instance again.
|
||
|
||
### Two numbers, and neither is the other
|
||
|
||
Measured on one machine, in one session, on a private
|
||
`IMPROVISE_HARNESS_PORT`, with **both trees clean** — `1af51c9` reached by a
|
||
full checkout rather than by restoring paths over a working branch (see the
|
||
trap below). Two runs each; the pair is the variance.
|
||
|
||
| suite | chromium | all projects | execution | whole job |
|
||
|---|---|---|---|---|
|
||
| Playwright, before (`1af51c9`) | 23 | 26 passed + 3 skipped | 36.2s / 36.3s | ~4m30s in CI |
|
||
| Playwright, after | 14 | 17 passed + 3 skipped | 15.1s / 14.9s | ~4m30s in CI |
|
||
| `node --test web/tests` | — | 26 | 0.12s | seconds |
|
||
|
||
**Two columns because one number was ambiguous three times.** Every spec but
|
||
`opfs.spec.js` is Chromium-only, so a count taken off a run is three larger than
|
||
the count of tests in the files (Firefox's OPFS battery) with three more skipped
|
||
(WebKit's). The columns must also **agree**: chromium + 3 is the passed total,
|
||
and a row where they do not is a row built from two different trees.
|
||
`npx playwright test --list` prints the per-project truth; quote which one, and
|
||
this is the same "two numbers, not one" the Rust suite learned in
|
||
`design-principles.md` §7.
|
||
|
||
**The trap that made the first `before` figure wrong.** It was taken by
|
||
`git checkout 1af51c9 -- harness web` inside a working branch — which restores
|
||
*tracked* files and leaves untracked ones **where they are**, so the branch's own
|
||
`contract.spec.js` stayed and the "baseline" ran main's 23 tests **plus three of
|
||
the branch's**. A baseline taken by checking paths out over your own work
|
||
measures a tree that never existed. The full checkout above is the repair.
|
||
|
||
**What gave it away was the count, not the clock — and that is the lesson.** The
|
||
contaminated run read 29 passed where a clean `1af51c9` reads 26, because three
|
||
extra tests really were there; but it read 36.9s against the clean tree's 36.2s,
|
||
because those three cost under a second between them. **A wall clock absorbs a
|
||
contaminated tree without a trace; an arithmetic identity does not.** The same
|
||
error survived three separate corrections — it reached PR #61's body as "26
|
||
chromium" (23 + the 3), survived a fix that corrected the chromium column and
|
||
left the run count beside it, and was caught each time only by someone checking
|
||
that the columns add up.
|
||
|
||
**Where the saving actually came from, since the obvious answer is wrong.** The
|
||
eleven per-widget tests that were deleted cost **2.7 seconds of test time
|
||
between them** — restored verbatim and re-measured — against **1.55s** for the
|
||
three contract tests replacing them. Those are the per-test sums, not the two
|
||
wall clocks, which read 3.3s and 3.4s and **flattered the replacements**: they
|
||
were separate `npx playwright test` invocations, so each paid its own cold
|
||
start (~0.6s and ~1.9s) and a near-tie appeared between two things not measured
|
||
the same way. Warm inside a full suite those three cost under a second, which is
|
||
why the contaminated baseline's clock barely moved.
|
||
|
||
Eleven tests at 2.7s cannot buy a twenty-second saving, and they did not.
|
||
Essentially all of 36.2s → 15s was two
|
||
`shell.spec.js` tests that **waited out the 8s watchdog in real time**, and one
|
||
of those two was the routing guard this section explains should never have gone.
|
||
So the deletion of the eleven bought shape, not speed; the speed came from a
|
||
deletion that cost coverage. Both are back at ~75ms each, because `page.clock`
|
||
fakes the page's timers — the honest version of the trade: *the wait was the
|
||
cost, not the test.*
|
||
|
||
**Cutting Playwright tests barely moves the CI number** in any case, because
|
||
roughly four minutes of that job is three `wasm-pack` builds, `npm ci` and a
|
||
browser download — not test execution. The feedback-time win is the `js` job, which
|
||
needs **no Rust toolchain and no wasm bundle at all**, and that is a constraint
|
||
rather than an accident: the moment a `web/tests/*.test.js` file needs
|
||
`web/pkg/`, it inherits the four-minute build and the argument for it
|
||
evaporates. That is why the one test that does need the bundle lives in
|
||
`web/tests/bundle/` and runs in the browser job instead.
|