docs(web,harness): cap the JavaScript and the browser harness

web/app.js's transport/watchdog/frameCost arguments and web/worker.js's two
ordering rules — including the one nothing here can test, that an `async`
message listener does not serialize — are now prose in context/code/clients.md.
The harness keeps its own section: why a worker, why a report, the engine
coverage and what `wipe` deliberately cannot clear.

harness/tests/*.spec.js's comments were the record of what each assertion
proves; they are in context/test-evidence/clients.md under the exact test
titles, including the `test.fail` measurement that could NOT become a CI guard
because its trigger is the host's network stack, and the closed-invalid
improvise-xps8 report that a simulated fix appeared to confirm.

ws-server/src/main.rs's ```text fence was prose claiming the doctest
exemption, not a runnable doctest; the synopsis is now two lines and the full
form is in the prose file. No fences remain in this unit.

Verified: node --check on every JS file touched; the non-comment diff for both
'*.rs' and '*.js' is empty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-20 10:02:03 -07:00
parent 8ed3df8ce3
commit 3d3bec43e0
10 changed files with 603 additions and 517 deletions
+308
View File
@@ -741,3 +741,311 @@ 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 `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. the per-position accessors scan and asking them inside the row loop is quadratic.
---
## `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.
### 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 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.** So **a change to `web/*.js` belongs in this spec directory**, not in a
second ad-hoc surface beside it.
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. 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.
+180 -2
View File
@@ -144,7 +144,7 @@ would be a client claiming to have seen something it was never sent.
## `improvise-wasm-client/src/keys.rs` ## `improvise-wasm-client/src/keys.rs`
### text_entry_modes #### text_entry_modes (a fixture, not a test)
Not a test — a fixture, and its limitation is the evidence. The comment here Not a test — a fixture, and its limitation is the evidence. The comment here
used to claim a new mode "shows up as a missing line rather than as untested used to claim a new mode "shows up as a missing line rather than as untested
@@ -480,10 +480,188 @@ the effect pipeline. The write's composition is asserted in `keys.rs` against
unit test cannot see, which is that the advance and the re-entry land in the same unit test cannot see, which is that the advance and the re-entry land in the same
place. place.
### client_ready_to_edit #### client_ready_to_edit (a fixture, not a test)
The published basis a real client folds before it is allowed to compose a write The published basis a real client folds before it is allowed to compose a write
(ADR 001). The tiers are the ones lockstep never checks — the host applies the (ADR 001). The tiers are the ones lockstep never checks — the host applies the
keypress directly rather than the client's `Command::Model` — but keypress directly rather than the client's `Command::Model` — but
`BasisCache::claim` withholds until it has *something*, so a client without one `BasisCache::claim` withholds until it has *something*, so a client without one
resolves no commit at all. resolves no commit at all.
---
## `harness/` — the browser specs
These are Playwright tests, so the checker's `--check-evidence` pass (which
scans Rust `fn` names) cannot see their names. They are recorded here under
their exact `test("…")` titles anyway, because they are where the JavaScript
evidence lives.
#### harness/tests/shell.spec.js — the file's own scope
The three assertions improvise-hwpi could not write. That ticket fixed four
things and said of itself that the fixes were "as unverified as the bugs",
because three of them are one-assertion tests that needed a browser. These are
those three, plus the watchdog — which was found by reading, and only because
someone went looking. Until this file, `web/app.js` and `web/worker.js` had
never been rendered or run.
Two fixtures are load-bearing. `NO_WEBSOCKET_HERE` is a peer that accepts the
TCP connection and then refuses the WebSocket upgrade — the harness's own static
file server — **deterministic where a closed port is not**: a refused port's
`close` timing is a property of the host's network stack, and on the GitHub
runner it did not arrive inside the window. The page cannot tell the two apart
and neither can `web/app.js`: in both cases the socket never opened.
`HANDSHAKE_TIMEOUT_MS` restates `web/app.js`'s own constant, because two tests
have to outlast it and a number that silently disagreed with the page would make
them pass for the wrong reason. `logStatus` records **every distinct `#status`**,
not just the last one, because several of these tests are about a message being
*replaced*, which the final value cannot see.
#### "a frame the client cannot fold says so on screen"
improvise-hwpi: `on_frame` returns a `Result`, which wasm-bindgen throws. Thrown
in a bare listener it reached the devtools console and nothing else — the grid
simply stopped updating with nothing on screen saying why. **The fault is
injected at the *peer*, not in the page** — a mocked socket that answers the
handshake with a frame the client cannot fold — so this exercises the real
`on_frame` and the real catch.
#### "a peer that cannot be reached is named, and the watchdog stays out of it"
improvise-hwpi: without an `error` listener, a connection that never opened left
`#status` blank, and "the server is not running" read exactly like "still
connecting". The watchdog half is asserted on the same page load rather than in
its own test, because it needs the same nine-second wait. It holds for a reason
its own comment does not lead you to expect: `armWatchdog` runs only after
`transport.ready` resolves, which happens on `open` — so on a socket that never
opened the watchdog is never armed at all.
#### "a socket that opened and then dropped reads as disconnected, and stays that way"
The other arm, which must keep working when the first is fixed: a socket that
really did open and then dropped IS a disconnect, and saying so is the whole
reason `close` cannot simply be suppressed.
**It runs past `HANDSHAKE_TIMEOUT_MS` on purpose.** On this arm the watchdog IS
armed — `armWatchdog` runs after `transport.ready` resolves, i.e. on `open` — so
"disconnected" is written while a timer is pending, and the only reason it
survives is that `close` goes through `setShellFailure`, which clears it. That
was argued in a comment and asserted nowhere, and **the argument was got wrong
from the outside**: a bug report (improvise-xps8) claimed the timer fired 8s
later and overwrote "disconnected" with "sent no document". It had been measured,
honestly, but against a tree predating PR #21 — and the "confirmation" that it
survived PR #21 simulated only that PR's `opened` flag and **not its other half**,
routing BOTH close arms through `setShellFailure`. *A simulated fix that is not
the fix you named reproduces a bug the real code does not have, and reads exactly
like a confirmation.* The ticket was filed and closed **invalid**. Hence this
assertion rather than another paragraph — proved discriminating by making
`setShellFailure` skip its `clearTimeout`, which makes it fail. Same rule as
`doctrine(an_injected_fault_must_be_the_fault_you_named)`, one level up.
**The measurement that could not become a CI guard.** Both of the above were once
asserted as `test.fail` expected-failures and **had to be removed**: neither
reproduced on the GitHub runner, where `close` does not follow `error` within the
window. An expected-failure is green when the bug is present and red when it is
absent, so one whose trigger is platform-dependent does not merely flake — **it
inverts CI's meaning**, and it failed the build twice saying "expected to fail,
but passed". The lesson stands even though the second bug did not: **assert the
PROPERTY THAT MUST HOLD, which is portable, rather than the fault, whose trigger
is the host's network stack.**
The first of the two, for the record, was real: the socket's `close` listener was
unconditional, so on a connection that never opened it overwrote "cannot reach …"
with "disconnected". FIXED by PR #21. An `opened` flag is the only discriminator
— `readyState` at close time cannot tell the two apart, and neither can the
`CloseEvent`: both cases are code 1006, `wasClean: false`.
#### "a peer that accepts and then says nothing trips the watchdog"
The first-frame watchdog, found by reading. The condition is one **no
per-transport check would catch**: the peer accepted the connection and then said
nothing. The mock accepts and never answers.
#### "the page draws the demo document out of the worker"
No `?ws=`, so `web/app.js` spawns the in-page worker over OPFS: the whole
application, with no server process. If this draws a grid, then `worker.js`,
`bindings.rs`, `OpfsFs`, `vfs::open` and the seed fetch all ran.
#### "committing an edit sends one frame and shows the host's value"
A commit round-trips through the host, and what lands on screen is the **host's**
value rather than the string that was typed. `20000` is typed and `20,000`
asserted: the number format is applied by the host and relayed back, so a page
that echoed the input would fail.
**This is deliberately NOT named for improvise-hwpi's third fix** (the `closed`
re-entry guard in `edit`), which Copilot asked for on PR #24. That guard is **not
observable from the DOM**, measured rather than assumed: with it removed, `close`
still runs exactly twice, the frame is still sent exactly once, `#grid` still
takes exactly two `childList` mutations, and the final text is identical. The only
difference is one extra *entry* into `repaint()` that produces no output — because
the second close is `close(false)`, so it does not re-send, and everything else it
does is idempotent. That is precisely why hwpi called it "harmless before, but
only by accident". Counting `repaint()` entries would need a test hook inside
`web/app.js`, which would be a page that knows it is being tested. So the test
name says what is checked and this entry says what is not. What *is* observable is
asserted: the editor is gone and did not come back, so no stray `<input>` is left
behind and the host's value is not overwritten by the `original` text the second
close writes before repainting.
#### "the harness can see a failure"
Check the checker, and it runs **first**: a green run against a worker that never
started is the failure this repo has a doctrine about. `run_canary` drives the
*same* reporting path every real check uses, and the spec asserts it is reported
as a failure with its message intact.
#### "every check in the battery runs and passes"
The battery is asserted **complete before any of it is believed**: the report's
names must equal `EXPECTED` exactly, so a build that dropped a check fails rather
than passing with fewer.
#### "what OPFS held survives a reload, and a committed delete does not"
The one thing no in-page check can establish. A delete is visible immediately and
lands in OPFS at the end of the batch — so "`commit_deletes` actually removed it"
is only answered by a fresh page. This is also the storage half of improvise-bck's
persistence criterion.
#### The opfs-contract checks (`harness/opfs-contract/src/lib.rs`)
- **`an_attached_but_unwritten_entry_is_not_a_document`** — `attach` must create
the file to obtain a sync access handle for a document that does not exist yet,
and that created file is zero bytes, which is NOT a document. `Storage::list`
promises document names, so an attached-but-unwritten name must not be listed.
Indexing it is exactly the improvise-9x6 browser failure: the seed was skipped
as "already there" and `load_from` got an empty buffer. The check also opens a
**second instance**, which shares no index, because the filter has to be in
`refresh` and not only in the omission from `attach` — and then writes through
the handle to show it is real and the name IS then a document.
- **`a_directory_entry_is_not_a_document`** — `keys()` yields DIRECTORIES as well
as files, and `getFileHandle` on a directory rejects with `TypeMismatchError`.
Treating that as "could not measure, so assume a document" would list an entry
that can never be attached or read — the same picker bug the zero-byte filter
removes (Copilot, PR #24). `OpfsFs` never creates a directory, so the harness
makes one itself, and asserts a real file beside it IS listed — the
precondition, so the check cannot pass by the directory never having been
created, and evidence that `keys()` walked the root at all.
- **`refresh_walks_every_key_and_sorts_them`** — the hand-rolled `keys()`
async-iterator walk in `refresh` is the least certain code in `opfs.rs`
(improvise-xf1 named it). One entry could pass by accident; several, out of
order, cannot. It wipes first, like every other check: a battery whose steps
depend on the leftovers of the step before breaks when one is inserted, which
is exactly what inserting the directory check did.
- **`read_and_write_are_absolute_at_offset_zero`** — `FileSystemReadWriteOptions`'
`at` handling, the other thing xf1 called out. A `write` that appended instead
of overwriting, or a `read` that started anywhere but 0, is invisible on a
single short write and obvious across a long one followed by a short one.
- **`storage_contract_at_…`** — the point of the whole exercise. `attach` is
hoisted out because acquiring a handle is the async half and `Storage::write`
cannot await; **the contract itself is the shared function, not a transcription
of it**.
- **`a_delete_is_visible_at_once_and_lands_on_commit`** — both halves, because
either one alone would pass against a backend that got the timing exactly
backwards.
+2 -6
View File
@@ -1,9 +1,5 @@
//! `improvise-ws-server` — serve improvise documents over a websocket. //! `improvise-ws-server model.improv [q3.improv …] --port 8787 --bind ADDR` —
//! ```text //! serve documents over a websocket. Naming and limits: context/code/clients.md.
//! improvise-ws-server examples/demo.improv --port 8787
//! improvise-ws-server examples/demo.improv q3.improv # two documents
//! ```
//! Naming, startup-only opening and the MVP limits: `context/code/clients.md`.
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
+26 -82
View File
@@ -1,25 +1,5 @@
//! Run `improvise_io::storage::contract::storage_contract` against `OpfsFs`, //! Run `storage_contract` against `OpfsFs`, inside a dedicated Web Worker, in
//! inside a dedicated Web Worker, in a real browser. //! a real browser. Why a worker and why a report: `context/code/clients.md`.
//!
//! `OpfsFs` had been compile-checked and never run (gotcha 45). This is the
//! instrument that changed that (improvise-vvl3, absorbing improvise-xf1). It
//! is a *harness* crate, not part of the application: it ships in no bundle
//! and is not a workspace member.
//!
//! # Why a worker
//!
//! `FileSystemFileHandle.createSyncAccessHandle()` exists only in a dedicated
//! Web Worker, and it is the whole reason `Storage` could stay synchronous
//! (gotcha 28). 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.
use std::path::Path; use std::path::Path;
@@ -64,11 +44,8 @@ impl Report {
} }
} }
/// The origin's root directory, reached the way `OpfsFs::open` reaches it. /// The origin's root directory. Only the harness needs it: `OpfsFs` has no
/// /// directory vocabulary, so making one has to happen outside it.
/// Only the harness needs this. `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.
async fn opfs_root() -> Result<web_sys::FileSystemDirectoryHandle> { async fn opfs_root() -> Result<web_sys::FileSystemDirectoryHandle> {
let scope = js_sys::global() let scope = js_sys::global()
.dyn_into::<web_sys::WorkerGlobalScope>() .dyn_into::<web_sys::WorkerGlobalScope>()
@@ -102,18 +79,8 @@ async fn drop_directory(name: &str) -> Result<()> {
Ok(()) Ok(())
} }
/// Remove every *document* in the origin root, so a run does not inherit the /// Remove every *document* in the origin root, using only the backend's own
/// last one's leftovers. /// vocabulary. What it cannot clear, and why: `context/code/clients.md`.
///
/// Uses only the backend's own vocabulary — `refresh`, `delete`,
/// `commit_deletes` — rather than reaching for `removeEntry` directly, so a
/// broken `delete` shows up here 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 here for the same reason it is
/// invisible to a document picker.
async fn wipe(fs: &OpfsFs) -> Result<usize> { async fn wipe(fs: &OpfsFs) -> Result<usize> {
fs.refresh().await?; fs.refresh().await?;
let names = fs.list(Path::new(""))?; let names = fs.list(Path::new(""))?;
@@ -156,12 +123,8 @@ pub async fn run_checks() -> Result<String, JsError> {
) )
.await; .await;
// `attach` must create the file to obtain a sync access handle for a // An attached-but-unwritten name is zero bytes and NOT a document; listing
// document that does not exist yet — and that created file is zero bytes, // it is the improvise-9x6 browser failure. Evidence file.
// which is NOT a document. `Storage::list` promises document names, so an
// attached-but-unwritten name must not be listed. Indexing it is exactly
// the improvise-9x6 browser failure (the seed was skipped as "already
// there" and `load_from` got an empty buffer).
report report
.run( .run(
"an_attached_but_unwritten_entry_is_not_a_document", "an_attached_but_unwritten_entry_is_not_a_document",
@@ -174,9 +137,8 @@ pub async fn run_checks() -> Result<String, JsError> {
"attach listed {listed:?} before anything was written" "attach listed {listed:?} before anything was written"
); );
// ...and a second instance, which shares no index, must not see // ...and a second instance, sharing no index, must not see it:
// it either: the filter has to be in `refresh`, not only in the // the filter has to be in `refresh` too, not only in `attach`.
// omission from `attach`.
let other = OpfsFs::open().await?; let other = OpfsFs::open().await?;
other.refresh().await?; other.refresh().await?;
let seen = other.list(Path::new(""))?; let seen = other.list(Path::new(""))?;
@@ -197,12 +159,8 @@ pub async fn run_checks() -> Result<String, JsError> {
) )
.await; .await;
// `keys()` yields DIRECTORIES as well as files, and `getFileHandle` on a // `keys()` yields DIRECTORIES too, and one must not be listed as a document
// directory rejects with `TypeMismatchError`. Treating that as "could not // (Copilot, PR #24). See `context/test-evidence/clients.md`.
// measure, so assume a document" would list an entry that can never be
// attached or read — the same picker bug the zero-byte filter removes
// (found by Copilot on PR #24). `OpfsFs` never creates a directory, so
// the harness makes one itself.
report report
.run("a_directory_entry_is_not_a_document", async || { .run("a_directory_entry_is_not_a_document", async || {
wipe(&fs).await?; wipe(&fs).await?;
@@ -217,8 +175,7 @@ pub async fn run_checks() -> Result<String, JsError> {
); );
// The precondition, so this cannot pass by the directory never // The precondition, so this cannot pass by the directory never
// having been created: a real file beside it IS listed, which // having been created: a real file beside it IS listed.
// also says `keys()` walked the root at all.
fs.attach(Path::new("beside.improv")).await?; fs.attach(Path::new("beside.improv")).await?;
fs.write(Path::new("beside.improv"), b"a real document")?; fs.write(Path::new("beside.improv"), b"a real document")?;
let again = OpfsFs::open().await?; let again = OpfsFs::open().await?;
@@ -234,14 +191,12 @@ pub async fn run_checks() -> Result<String, JsError> {
}) })
.await; .await;
// The hand-rolled `keys()` async-iterator walk in `refresh` is the least // The hand-rolled `keys()` walk is the least certain code in opfs.rs: one
// certain code in opfs.rs (improvise-xf1 named it). One entry could pass // entry could pass by accident, several out of order cannot.
// by accident; several, out of order, cannot.
report report
.run("refresh_walks_every_key_and_sorts_them", async || { .run("refresh_walks_every_key_and_sorts_them", async || {
// Wipes first, like every other check. A battery whose steps // Wipes first, like every other check: a battery whose steps depend
// depend on the leftovers of the step before breaks when one is // on the step before breaks when one is inserted.
// inserted, which is exactly what the directory check above did.
wipe(&fs).await?; wipe(&fs).await?;
for name in ["c.improv", "a.improv", "b.improv"] { for name in ["c.improv", "a.improv", "b.improv"] {
fs.attach(Path::new(name)).await?; fs.attach(Path::new(name)).await?;
@@ -259,10 +214,8 @@ pub async fn run_checks() -> Result<String, JsError> {
}) })
.await; .await;
// `FileSystemReadWriteOptions`' `at` handling, the other thing xf1 called // `FileSystemReadWriteOptions`' `at` handling: invisible on a single short
// out. A `write` that appended instead of overwriting, or a `read` that // write, obvious across a long one followed by a short one.
// started anywhere but 0, is invisible on a single short write and obvious
// across a long one followed by a short one.
report report
.run("read_and_write_are_absolute_at_offset_zero", async || { .run("read_and_write_are_absolute_at_offset_zero", async || {
wipe(&fs).await?; wipe(&fs).await?;
@@ -285,8 +238,7 @@ pub async fn run_checks() -> Result<String, JsError> {
.await; .await;
// The point of the whole exercise. `attach` is hoisted out because // The point of the whole exercise. `attach` is hoisted out because
// acquiring a handle is the async half and `Storage::write` cannot await; // acquiring a handle is the async half and `Storage::write` cannot await.
// the contract itself is the shared function, not a transcription of it.
for dir in ["", "docs"] { for dir in ["", "docs"] {
report report
.run(&format!("storage_contract_at_{:?}", dir), async || { .run(&format!("storage_contract_at_{:?}", dir), async || {
@@ -300,9 +252,8 @@ pub async fn run_checks() -> Result<String, JsError> {
.await; .await;
} }
// Gotcha 28: a delete is visible to `read` and `list` at once and lands in // Gotcha 28: a delete is visible at once and lands in OPFS at the end of
// OPFS at the end of the batch. Both halves, because either one alone // the batch. Both halves, or a backwards backend would pass.
// would pass against a backend that got the timing exactly backwards.
report report
.run( .run(
"a_delete_is_visible_at_once_and_lands_on_commit", "a_delete_is_visible_at_once_and_lands_on_commit",
@@ -341,10 +292,8 @@ pub async fn run_checks() -> Result<String, JsError> {
Ok(report.finish()) Ok(report.finish())
} }
/// Write the state the reload check expects, then hand control back. /// Write the state the reload check expects, then hand control back. Split
/// /// from [`after_reload`] because only a reload can ask whether OPFS persisted.
/// Split from [`after_reload`] because the one thing no in-page check can
/// establish is that OPFS *persisted* — and a reload is the only way to ask.
#[wasm_bindgen] #[wasm_bindgen]
pub async fn before_reload() -> Result<String, JsError> { pub async fn before_reload() -> Result<String, JsError> {
inner_before().await.map_err(js) inner_before().await.map_err(js)
@@ -398,13 +347,8 @@ fn js(e: anyhow::Error) -> JsError {
JsError::new(&format!("{e:#}")) JsError::new(&format!("{e:#}"))
} }
/// Deliberately fails, so the harness can prove it can see a failure. /// Deliberately fails, so the harness can prove it can see a failure — through
/// /// the *same* reporting path every real check uses. Evidence file.
/// A green run against a stub, or one that silently skipped because the worker
/// never started, is exactly the check-the-checker failure this repo has a
/// doctrine about. The spec runs this and asserts it is reported as a failure
/// with its message intact, which exercises the *same* reporting path every
/// real check uses.
#[wasm_bindgen] #[wasm_bindgen]
pub async fn run_canary() -> String { pub async fn run_canary() -> String {
let mut report = Report::default(); let mut report = Report::default();
+4 -43
View File
@@ -1,31 +1,7 @@
import { defineConfig, devices } from "@playwright/test"; import { defineConfig, devices } from "@playwright/test";
// `python3 -m http.server` and one spec directory. Deliberately small // The one place a line of web/*.js is executed, and where a change to either
// (improvise-vvl3): the point is to be able to run `storage_contract` against // file belongs. Why, and why Playwright: context/code/clients.md.
// `OpfsFs` and to read `#status` out of a real page, not to grow a second
// test framework beside `cargo test`.
//
// # This is the one place a line of web/*.js is executed
//
// It is not only the OPFS harness. Before this 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 (gotcha 48) 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:
// see `tests/shell.spec.js`.
//
// So a change to `web/*.js` belongs in this spec directory, not in a second
// ad-hoc surface beside it — two answers to one question is the defect this
// codebase keeps curing. Playwright is the right vehicle rather than a
// headless-DOM runner (jsdom and friends) because the things that go wrong in
// these files are a real Worker, a real WebSocket, real OPFS and real event
// ordering, none of which a DOM shim has.
//
// The server root is `harness/serve`, built by `build.sh` — the assembled
// site plus this harness's own page. A module script will not load from
// `file://`, so an http server is not optional.
export default defineConfig({ export default defineConfig({
testDir: "./tests", testDir: "./tests",
fullyParallel: false, fullyParallel: false,
@@ -38,23 +14,8 @@ export default defineConfig({
baseURL: "http://127.0.0.1:8099", baseURL: "http://127.0.0.1:8099",
trace: "retain-on-failure", trace: "retain-on-failure",
}, },
// improvise-xf1's acceptance criterion names Chrome, Firefox and Safari, // Chromium + Firefox run the OPFS battery; WebKit has no OPFS at all and is
// and the OPFS spec is the reason: `createSyncAccessHandle` is the one API // kept for the day it does. The shell spec is Chromium-only: clients.md.
// this whole design rests on (gotcha 28), 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.
//
// 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.
//
// 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, and an unexpected skip on any
// other engine is a failure.
projects: [ projects: [
{ name: "chromium", use: { ...devices["Desktop Chrome"] } }, { name: "chromium", use: { ...devices["Desktop Chrome"] } },
{ {
+8 -31
View File
@@ -1,14 +1,5 @@
// `storage_contract` against `OpfsFs`, in a browser (improvise-vvl3/xf1). // `storage_contract` against `OpfsFs`, in a browser (improvise-vvl3/xf1). The
// // assertions here are about *the report*: context/test-evidence/clients.md.
// `OpfsFs` had been compile-checked and never run. Everything found in a
// browser so far was found by a person pressing keys, which is not a
// repeatable check. This is.
//
// The checks themselves are in Rust (harness/opfs-contract) because that is
// where `OpfsFs` is and where the contract is; this file drives them and reads
// the report. Assertions here are about *the report*, deliberately, so a
// worker that never started or a build that dropped a check fails rather than
// passing quietly.
import { test, expect } from "@playwright/test"; import { test, expect } from "@playwright/test";
@@ -17,9 +8,8 @@ async function run(page, cmd) {
return JSON.parse(await page.evaluate((c) => window.opfs(c), cmd)); return JSON.parse(await page.evaluate((c) => window.opfs(c), cmd));
} }
/** The checks the report must contain, in order. Named here as well as in /** The checks the report must contain, in order. Named here as well as in Rust
* Rust on purpose: a check deleted on one side has to be deleted on both, * on purpose — see context/test-evidence/clients.md. */
* which is what stops the battery quietly shrinking. */
const EXPECTED = [ const EXPECTED = [
"open_and_refresh_see_an_origin_root_holding_no_documents", "open_and_refresh_see_an_origin_root_holding_no_documents",
"an_attached_but_unwritten_entry_is_not_a_document", "an_attached_but_unwritten_entry_is_not_a_document",
@@ -35,18 +25,8 @@ test.describe("OpfsFs in a real browser", () => {
test.beforeEach(async ({ page, browserName }, testInfo) => { test.beforeEach(async ({ page, browserName }, testInfo) => {
await page.goto("/opfs/index.html"); await page.goto("/opfs/index.html");
// improvise-xf1's acceptance criterion names Chrome, Firefox and Safari.
// Chromium and Firefox run the whole battery. Playwright's WebKit build
// provides **no OPFS at all** — `navigator.storage.getDirectory()` itself
// rejects with `UnknownError` — so there is nothing there for `OpfsFs` to
// be right or wrong about, and Safari stays uncovered. See the hand-back
// and `playwright.config.js`.
//
// The skip is conditional on the PROBE, not on the browser name, so it // The skip is conditional on the PROBE, not on the browser name, so it
// cannot quietly swallow a real regression: if Chromium or Firefox ever // cannot quietly swallow a real regression. See clients.md.
// stops providing OPFS, this fails rather than skipping. A silently
// skipped check is exactly the failure this harness exists to prevent, so
// an unexpected skip also fails.
const opfs = await page.evaluate(async () => { const opfs = await page.evaluate(async () => {
try { try {
await navigator.storage.getDirectory(); await navigator.storage.getDirectory();
@@ -65,8 +45,7 @@ test.describe("OpfsFs in a real browser", () => {
}); });
// Check the checker, and do it first: a green run against a worker that // Check the checker, and do it first: a green run against a worker that
// never started is the failure this repo has a doctrine about. This drives // never started is the failure this repo has a doctrine about.
// the *same* reporting path every real check uses.
test("the harness can see a failure", async ({ page }) => { test("the harness can see a failure", async ({ page }) => {
const report = await run(page, "run_canary"); const report = await run(page, "run_canary");
expect(report).toHaveLength(1); expect(report).toHaveLength(1);
@@ -88,10 +67,8 @@ test.describe("OpfsFs in a real browser", () => {
).toEqual([]); ).toEqual([]);
}); });
// The one thing no in-page check can establish. gotcha 28: a delete is // The one thing no in-page check can establish: "commit_deletes actually
// visible immediately and lands in OPFS at the end of the batch — so // removed it" is only answered by a fresh page.
// "commit_deletes actually removed it" is only answered by a fresh page.
// This is also the storage half of improvise-bck's persistence criterion.
test("what OPFS held survives a reload, and a committed delete does not", async ({ test("what OPFS held survives a reload, and a committed delete does not", async ({
page, page,
}) => { }) => {
+23 -121
View File
@@ -1,37 +1,21 @@
// The browser shell: the three assertions improvise-hwpi could not write. // The browser shell: the three assertions improvise-hwpi could not write, plus
// // the watchdog. The history is in context/test-evidence/clients.md.
// That ticket fixed four things and said of itself that the fixes were "as
// unverified as the bugs", because three of them are one-assertion tests that
// needed a browser. These are those three, plus the watchdog — which was found
// by reading, and only because someone went looking.
//
// Until this file, `web/app.js` and `web/worker.js` had never been rendered or
// run (gotcha 48). Everything below is the first execution of either — so this
// is the one place in the repo where "there is no browser here" is false, and
// the statement does not belong inside the artefact that falsifies it.
import { test, expect } from "@playwright/test"; import { test, expect } from "@playwright/test";
const status = (page) => page.locator("#status"); const status = (page) => page.locator("#status");
/// A peer that accepts the TCP connection and then refuses the WebSocket /// A peer that accepts the TCP connection and then refuses the WebSocket
/// upgrade — the harness's own static file server. Deterministic where a /// upgrade — deterministic where a closed port is not (evidence file).
/// closed port is not: a refused port's `close` timing is a property of the
/// host's network stack, and on the GitHub runner it did not arrive inside
/// the window, which made two expected-failure tests report "expected to
/// fail, but passed". The page cannot tell the two apart and neither can
/// `web/app.js`: in both cases the socket never opened.
const NO_WEBSOCKET_HERE = "127.0.0.1:8099"; const NO_WEBSOCKET_HERE = "127.0.0.1:8099";
const json = (v) => JSON.stringify(v); const json = (v) => JSON.stringify(v);
/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Named here because two tests have /// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Two tests have to outlast it, and a
/// to outlast it, and a number that silently disagrees with the page would /// number that silently disagreed with the page would pass for the wrong reason.
/// make them pass for the wrong reason.
const HANDSHAKE_TIMEOUT_MS = 8_000; const HANDSHAKE_TIMEOUT_MS = 8_000;
/// Record every distinct #status the page shows, not just the last one. /// Record every distinct #status the page shows, not just the last one: several
/// Several of these are about a message being REPLACED, which the final value /// of these are about a message being REPLACED.
/// cannot see.
function logStatus() { function logStatus() {
window.__statusLog = []; window.__statusLog = [];
new MutationObserver(() => { new MutationObserver(() => {
@@ -41,13 +25,8 @@ function logStatus() {
} }
test.describe("the shell reports what its peer is doing", () => { test.describe("the shell reports what its peer is doing", () => {
// improvise-hwpi: `on_frame` returns a Result, which wasm-bindgen throws. // improvise-hwpi. The fault is injected at the *peer*, not in the page, so
// Thrown in a bare listener it reached the devtools console and nothing // this exercises the real `on_frame` and the real catch.
// else: the grid simply stopped updating with nothing on screen saying why.
//
// The fault is injected at the *peer*, not in the page — a mocked socket
// that answers the handshake with a frame the client cannot fold — so this
// exercises the real `on_frame` and the real catch.
test("a frame the client cannot fold says so on screen", async ({ page }) => { test("a frame the client cannot fold says so on screen", async ({ page }) => {
await page.routeWebSocket("ws://127.0.0.1:8787/", (ws) => { await page.routeWebSocket("ws://127.0.0.1:8787/", (ws) => {
ws.onMessage(() => ws.send("{\"NotAFrame\":{}}")); ws.onMessage(() => ws.send("{\"NotAFrame\":{}}"));
@@ -56,15 +35,8 @@ test.describe("the shell reports what its peer is doing", () => {
await expect(status(page)).toContainText("bad frame", { timeout: 15_000 }); await expect(status(page)).toContainText("bad frame", { timeout: 15_000 });
}); });
// improvise-hwpi: without an `error` listener, a connection that never // improvise-hwpi, plus the watchdog half on the same page load because it
// opened left #status blank, and "the server is not running" read exactly // needs the same nine-second wait. See the evidence file.
// like "still connecting". Port 9 (discard) refuses immediately.
//
// The watchdog half is asserted on the same page load rather than in its
// own test, because it needs the same 9-second wait. It holds for a reason
// its own comment does not lead you to expect: `armWatchdog` runs only
// after `transport.ready` resolves, which happens on `open` — so on a
// socket that never opened the watchdog is never armed at all.
test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({ test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({
page, page,
}) => { }) => {
@@ -82,24 +54,8 @@ test.describe("the shell reports what its peer is doing", () => {
); );
}); });
// The other arm, which must keep working when the first one is fixed: a // The other arm, which must keep working when the first is fixed. It runs
// socket that really did open and then dropped IS a disconnect, and saying // past HANDSHAKE_TIMEOUT_MS on purpose — see the evidence file.
// so is the whole reason `close` cannot simply be suppressed.
//
// It runs past HANDSHAKE_TIMEOUT_MS on purpose. On this arm the watchdog IS
// armed — `armWatchdog` runs after `transport.ready` resolves, i.e. on
// `open` — so "disconnected" is written while a timer is pending, and the
// only reason it survives is that `close` goes through `setShellFailure`,
// which clears it. That was argued in a comment and asserted nowhere, and
// the argument was got wrong from the outside: a bug report claimed the
// timer fired 8s later and overwrote "disconnected" with "sent no
// document". It had been measured, but against a tree predating PR #21, and
// the "confirmation" that it survived PR #21 simulated only that PR's
// `opened` flag and not its other half — routing BOTH close arms through
// `setShellFailure`. A simulated fix that is not the fix you named
// reproduces a bug the real code does not have. Hence this assertion rather
// than another paragraph: proved discriminating by making `setShellFailure`
// skip its `clearTimeout`, which makes it fail.
test("a socket that opened and then dropped reads as disconnected, and stays that way", async ({ test("a socket that opened and then dropped reads as disconnected, and stays that way", async ({
page, page,
}) => { }) => {
@@ -123,45 +79,11 @@ test.describe("the shell reports what its peer is doing", () => {
); );
}); });
// TWO BUGS THIS FILE ONCE DESCRIBED AS LIVE, AND WHAT HAPPENED TO EACH. // Two bugs this file once described as live, one of which never existed in
// The history is worth more than the assertions would have been, because // the merged tree: context/test-evidence/clients.md keeps that history.
// one of them turned out never to have existed in the merged tree.
//
// 1. The socket's `close` listener was unconditional, so on a connection
// that never opened it overwrote "cannot reach …" with "disconnected".
// FIXED by PR #21. An `opened` flag is the only discriminator —
// `readyState` at close time cannot tell the two apart, and neither can
// the CloseEvent: both cases are code 1006, `wasClean: false`.
// 2. "On a connection that DID open and then dropped, nothing disarms the
// handshake watchdog, so 8s later 'disconnected' is replaced by
// 'accepted the connection but sent no document'." NOT A BUG, and never
// was on any tree that had PR #21. `close` routes both arms through
// `setShellFailure` (`app.js:248-250`), which clears the timer
// synchronously (`app.js:165-169`). It is now asserted directly by the
// open-then-dropped test above rather than described here.
//
// WHY THE SECOND ONE WAS BELIEVED, which is the part to keep: it was
// genuinely measured on macOS/Chromium — but against a tree based before
// PR #21. It was then reported as "surviving" that PR on the strength of a
// simulation that re-created only the `opened` flag, not the routing of
// both arms through `setShellFailure`. A simulated fix that is not the fix
// you named reproduces a bug the real code does not have, and it reads
// exactly like a confirmation. That is the same rule as
// doctrine(an_injected_fault_must_be_the_fault_you_named), one level up.
//
// Both were once asserted here as `test.fail` expected-failures and **had
// to be removed**: neither reproduced on the GitHub runner, where `close`
// does not follow `error` within the window. An expected-failure is green
// when the bug is present and red when it is absent, so one whose trigger
// is platform-dependent does not merely flake — it **inverts CI's
// meaning**, and it failed the build twice saying "expected to fail, but
// passed". The lesson stands even though the second bug did not: assert the
// PROPERTY THAT MUST HOLD, which is portable, rather than the fault, whose
// trigger is the host's network stack.
// The first-frame watchdog (gotcha 46), found by reading. The condition is // The first-frame watchdog, found by reading: no per-transport check would
// one no per-transport check would catch: the peer accepted the connection // catch it. The mock accepts the connection and never answers.
// and then said nothing. The mock accepts and never answers.
test("a peer that accepts and then says nothing trips the watchdog", async ({ test("a peer that accepts and then says nothing trips the watchdog", async ({
page, page,
}) => { }) => {
@@ -172,9 +94,8 @@ test.describe("the shell reports what its peer is doing", () => {
}); });
test.describe("the standalone deployment", () => { test.describe("the standalone deployment", () => {
// No `?ws=`, so `web/app.js` spawns the in-page worker over OPFS: the whole // No `?ws=`, so the page spawns the in-page worker over OPFS. If this draws
// application, with no server process. If this draws a grid, then // a grid, worker.js, bindings.rs, OpfsFs and the seed fetch all ran.
// worker.js, bindings.rs, OpfsFs, vfs::open and the seed fetch all ran.
test("the page draws the demo document out of the worker", async ({ page }) => { test("the page draws the demo document out of the worker", async ({ page }) => {
await page.goto("/index.html"); await page.goto("/index.html");
await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 });
@@ -184,25 +105,8 @@ test.describe("the standalone deployment", () => {
await expect(page.locator("#grid td.editable").first()).toBeVisible(); await expect(page.locator("#grid td.editable").first()).toBeVisible();
}); });
// A commit round-trips through the host, and what lands on screen is the // What lands on screen is the HOST's value, not the string typed. What this
// HOST's value rather than the string that was typed. `20000` is typed and // deliberately does NOT cover: context/test-evidence/clients.md.
// `20,000` asserted: the number format is applied by the host and relayed
// back, so a page that echoed the input would fail.
//
// This is deliberately NOT named for improvise-hwpi's third fix (the
// `closed` re-entry guard in `edit`), which Copilot asked for on PR #24.
// That guard is **not observable from the DOM**, measured rather than
// assumed: with it removed, `close` still runs exactly twice, the frame is
// still sent exactly once, `#grid` still takes exactly two childList
// mutations, and the final text is identical. The only difference is one
// extra *entry* into `repaint()` that produces no output — because the
// second close is `close(false)`, so it does not re-send, and everything
// else it does is idempotent. That is precisely why hwpi called it
// "harmless before, but only by accident".
//
// Counting repaint() entries would need a test hook inside `web/app.js`,
// which would be a page that knows it is being tested. So the name says
// what is checked and this comment says what is not.
test("committing an edit sends one frame and shows the host's value", async ({ test("committing an edit sends one frame and shows the host's value", async ({
page, page,
}) => { }) => {
@@ -219,10 +123,8 @@ test.describe("the standalone deployment", () => {
await expect(cell).toHaveText("20,000", { timeout: 10_000 }); await expect(cell).toHaveText("20,000", { timeout: 10_000 });
expect(before).not.toBe("20,000"); expect(before).not.toBe("20,000");
// The editor is gone and did not come back. This is the half of the // The editor is gone and did not come back: the half of the re-entry guard
// re-entry guard that IS observable: whatever `close` ran, no stray // that IS observable from the DOM.
// <input> is left behind and the host's value is not overwritten by the
// `original` text the second close writes before repainting.
await expect(page.locator("#grid input")).toHaveCount(0); await expect(page.locator("#grid input")).toHaveCount(0);
}); });
}); });
+2 -7
View File
@@ -1,10 +1,5 @@
// The dedicated Web Worker the OPFS contract runs inside. // The dedicated Web Worker the OPFS contract runs inside: `createSyncAccessHandle`
// // exists only here. Every decision is in Rust; this moves strings.
// `createSyncAccessHandle` exists only here — that is the whole reason
// `Storage` could stay synchronous (gotcha 28), and the reason this file
// exists rather than the page calling the bundle directly.
//
// Every decision is in Rust (harness/opfs-contract). This moves strings.
import init, { import init, {
run_checks, run_checks,
+41 -179
View File
@@ -1,47 +1,5 @@
// The browser half of improvise-cr3, kept as thin as it can be made, now // The browser half of improvise-cr3, talking to either of two peers: five
// talking to either of two peers (improvise-djm). // string calls, no protocol knowledge. See context/code/clients.md.
//
// 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 —
//
// 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
//
// — and 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.
//
// # 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 this 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. The scheme defaults to `wss://` — see `socketUrl`.
//
// 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.
import init, { Client } from "./pkg/improvise_wasm_client.js"; import init, { Client } from "./pkg/improvise_wasm_client.js";
@@ -50,23 +8,8 @@ const params = new URLSearchParams(location.search);
// the scheme is NOT the caller's to get wrong: see `socketUrl`. // the scheme is NOT the caller's to get wrong: see `socketUrl`.
const WS_PEER = params.get("ws"); const WS_PEER = params.get("ws");
/** /** The websocket URL for `?ws=`, defaulting to TLS. Why, and when an explicit
* The websocket URL for `?ws=`, defaulting to TLS. * `ws://` is upgraded rather than honoured: context/code/clients.md. */
*
* `wss://` is the default and a bare `host:port` gets it, because the address
* that works from where this page is actually served is the one it should be
* easiest to write. This 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.
*/
function socketUrl(peer) { function socketUrl(peer) {
const secure = location.protocol === "https:"; const secure = location.protocol === "https:";
if (/^wss:\/\//i.test(peer)) return peer; if (/^wss:\/\//i.test(peer)) return peer;
@@ -95,9 +38,8 @@ const el = {
await init(); await init();
const client = new Client(); const client = new Client();
// `#status` carries two independent facts — what the transport is doing, and // `#status` carries two independent facts, composed rather than overwritten:
// what the document last said — so it composes them rather than letting a // a repaint must not quietly erase a connection error.
// repaint quietly erase a connection error.
let shellStatus = ""; let shellStatus = "";
let modelStatus = ""; let modelStatus = "";
function paintStatus() { function paintStatus() {
@@ -108,19 +50,8 @@ function setShellStatus(text) {
paintStatus(); paintStatus();
} }
// What the grid's own frame costs, in whole cells, as of the last repaint. // What the grid's own frame costs, in whole cells, as of the last repaint —
// One header row and one header column was right until improvise-anr9: the // measured off the painted model, never predicted. context/code/clients.md.
// 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. What it counts, and the one thing it deliberately
// does not, is at the tail of `repaint`.
let frameCost = { rows: 1, cols: 1 }; let frameCost = { rows: 1, cols: 1 };
/** The extent this window can show, in whole DATA cells. */ /** The extent this window can show, in whole DATA cells. */
@@ -135,22 +66,12 @@ function extent() {
function sendViewport() { function sendViewport() {
const { rows, cols } = extent(); const { rows, cols } = extent();
// `client.set_viewport`, NOT the associated `Client.viewport`: the same // `client.set_viewport`, NOT the associated `Client.viewport`: the same
// frame, but it also RECORDS the window. A client learns its window only // frame, but it also RECORDS the window. Why that matters: clients.md.
// from what it announced -- `row_frame.len()` is the window clipped to the
// grid, so the two differ exactly at the bottom edge, which is where the
// scroll arithmetic matters. Without this 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).
transport.send(client.set_viewport(rows, cols)); transport.send(client.set_viewport(rows, cols));
} }
// A peer that never answers the handshake. The socket has an `error` event // A peer that never answers the handshake. Deliberately about the *first
// for this; the worker has none that fires, because worker.js catches its own // frame*, not either peer's own failure modes — context/code/clients.md.
// `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 watchdog is deliberately about the *first frame* rather
// than about either peer's own failure modes: it is the one condition that
// means the same thing in both deployments.
const HANDSHAKE_TIMEOUT_MS = 8000; const HANDSHAKE_TIMEOUT_MS = 8000;
let watchdog = null; let watchdog = null;
function armWatchdog(message) { function armWatchdog(message) {
@@ -158,10 +79,8 @@ function armWatchdog(message) {
watchdog = setTimeout(() => setShellStatus(message), HANDSHAKE_TIMEOUT_MS); watchdog = setTimeout(() => setShellStatus(message), HANDSHAKE_TIMEOUT_MS);
} }
// A peer that failed *loudly* has already said something more precise than // A peer that failed *loudly* has already said something more precise, so the
// the watchdog's guess, so the watchdog must be disarmed and not merely // watchdog must be DISARMED and not merely pre-empted (PR #13 review).
// pre-empted: it fires later and would overwrite the better message with
// "did not answer" (PR #13 review). The watchdog is for silence only.
function setShellFailure(text) { function setShellFailure(text) {
clearTimeout(watchdog); clearTimeout(watchdog);
watchdog = null; watchdog = null;
@@ -174,9 +93,8 @@ function onFrame(text) {
try { try {
client.on_frame(text); client.on_frame(text);
} catch (err) { } catch (err) {
// `on_frame` returns a Result, which wasm-bindgen throws. Thrown in a bare // `on_frame` returns a Result, which wasm-bindgen throws; thrown in a bare
// listener it reached the devtools console and nothing else, and the grid // listener it reached the console alone and the grid just stopped.
// simply stopped updating with nothing on screen saying why (improvise-hwpi).
setShellStatus("bad frame — the grid has stopped updating (see console)"); setShellStatus("bad frame — the grid has stopped updating (see console)");
console.error("improvise: frame rejected", text, err); console.error("improvise: frame rejected", text, err);
return; return;
@@ -184,7 +102,7 @@ function onFrame(text) {
repaint(); repaint();
} }
// ── transports ─────────────────────────────────────────────────────────── // Transports.
/** The in-page worker: the standalone deployment, no server process. */ /** The in-page worker: the standalone deployment, no server process. */
function workerTransport() { function workerTransport() {
@@ -202,7 +120,7 @@ function workerTransport() {
}); });
setShellStatus(`in-page worker · ${DOC}`); setShellStatus(`in-page worker · ${DOC}`);
// Frames posted before the worker has finished `start()` are held by // Frames posted before the worker has finished `start()` are held by
// worker.js and replayed in order, so the handshake can go out immediately. // worker.js in arrival order, so the handshake can go out immediately.
return { send: (text) => worker.postMessage(text), ready: Promise.resolve() }; return { send: (text) => worker.postMessage(text), ready: Promise.resolve() };
} }
@@ -210,12 +128,8 @@ function workerTransport() {
function socketTransport() { function socketTransport() {
const socket = new WebSocket(WS_URL); const socket = new WebSocket(WS_URL);
const unreachable = `cannot reach ${WS_URL} — is improvise-ws-server running?`; const unreachable = `cannot reach ${WS_URL} — is improvise-ws-server running?`;
// Did this socket ever open? The page has to record that itself, because // Did this socket ever open? The page must record it itself: nothing on the
// nothing on the close event carries it: `readyState` is already CLOSED by // close event can discriminate. See context/code/clients.md.
// 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.
let opened = false; let opened = false;
const ready = new Promise((resolve) => { const ready = new Promise((resolve) => {
socket.addEventListener("open", () => { socket.addEventListener("open", () => {
@@ -226,25 +140,10 @@ function socketTransport() {
}); });
socket.addEventListener("message", (event) => onFrame(event.data)); socket.addEventListener("message", (event) => onFrame(event.data));
// Without this, a connection that never opens left #status blank and // Without this, a connection that never opens left #status blank and
// "server not running" read exactly like "still connecting" (improvise-hwpi). // "server not running" read exactly like "still connecting".
socket.addEventListener("error", () => setShellFailure(unreachable)); socket.addEventListener("error", () => setShellFailure(unreachable));
// A socket that never opened did not "disconnect" — it failed to connect, // A socket that never opened did not "disconnect" (improvise-taxx). BOTH arms
// and the close event is the tail of that failure rather than a second, // route through `setShellFailure`, which disarms the watchdog — clients.md.
// vaguer thing that happened (improvise-taxx). Writing "disconnected" here
// unconditionally replaced the `error` listener's precise message with the
// least informative of the two facts.
//
// 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 this listener is really for. Hence the split rather than a guard.
// Both arms go through `setShellFailure`, which DISARMS the watchdog. That
// 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 here 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.
socket.addEventListener("close", () => socket.addEventListener("close", () =>
setShellFailure(opened ? "disconnected" : unreachable), setShellFailure(opened ? "disconnected" : unreachable),
); );
@@ -259,7 +158,7 @@ function socketTransport() {
const transport = WS_URL ? socketTransport() : workerTransport(); const transport = WS_URL ? socketTransport() : workerTransport();
// ── painting ───────────────────────────────────────────────────────────── // Painting.
let painted = -1; let painted = -1;
let editing = null; // {row, col} while an input is open, so a repaint cannot steal it let editing = null; // {row, col} while an input is open, so a repaint cannot steal it
@@ -287,26 +186,8 @@ function repaint() {
el.grid.replaceChildren(gridTable(model)); el.grid.replaceChildren(gridTable(model));
el.categories.replaceChildren(categoryList(model)); el.categories.replaceChildren(categoryList(model));
// What the frame costs, so the NEXT viewport announcement is honest: the // What the frame costs, so the NEXT viewport announcement is honest.
// header lines, a group line if there is one, the row header's width in // Inline group ROWS are deliberately NOT counted — context/code/clients.md.
// columns, and every inline group row.
//
// Inline group ROWS are NOT reserved, and that is a decision rather than
// an oversight. 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 on this PR.
//
// What is left is an over-request bounded by the groups in view. The cost
// is that 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: nothing in this page calls
// `scrollIntoView` at all, so a selection the client has not yet scrolled
// to can already sit below the fold. Filed as its own ticket rather than
// answered with a heuristic in a file no test in this repo can execute.
const next = { const next = {
rows: model.col_levels.length + (model.col_groups.length > 0 ? 1 : 0), rows: model.col_levels.length + (model.col_groups.length > 0 ? 1 : 0),
cols: model.row_levels, cols: model.row_levels,
@@ -338,9 +219,8 @@ function gridTable(model) {
for (const group of model.col_groups) { for (const group of model.col_groups) {
const th = document.createElement("th"); const th = document.createElement("th");
th.colSpan = group.span; th.colSpan = group.span;
// `label` is null for a run in no group. An empty STRING is a legal // `label` is null for a run in no group; an empty STRING is a legal group
// group name and still gets its indicator, which is why this is a null // name, so this is a null check and not a truthiness test.
// check and not a truthiness test.
th.textContent = th.textContent =
group.label === null ? "" : indicator(group.collapsed) + " " + group.label; group.label === null ? "" : indicator(group.collapsed) + " " + group.label;
th.classList.add("group"); th.classList.add("group");
@@ -361,9 +241,8 @@ function gridTable(model) {
} }
const body = table.createTBody(); const body = table.createTBody();
// The cell coordinates a click sends are DATA row indices, and a group // The cell coordinates a click sends are DATA row indices and a group header
// header is not a data row -- so this counts rows rather than trusting the // is not a data row, so this counts rather than trusting `model.rows`.
// position in `model.rows`.
let r = -1; let r = -1;
for (const row of model.rows) { for (const row of model.rows) {
const tr = body.insertRow(); const tr = body.insertRow();
@@ -389,21 +268,16 @@ function gridTable(model) {
row.cells.forEach((cell, c) => { row.cells.forEach((cell, c) => {
const td = tr.insertCell(); const td = tr.insertCell();
// A keyboard edit is open on the selected cell when `model.editing` is // A keyboard edit is open when `model.editing` is a string, and it holds
// a string; it holds what has been typed so far. Both facts are // what has been typed. Both facts are decided in Rust.
// decided in Rust (`RenderModel::editing`) — the page does not know
// which modes are editing modes or which buffer they type into.
// Without this the mode indicator would say INSERT while every
// keystroke vanished into a buffer nobody drew.
const typing = cell.selected && typeof model.editing === "string"; const typing = cell.selected && typeof model.editing === "string";
td.textContent = typing ? model.editing + "\u258f" : cell.text; td.textContent = typing ? model.editing + "\u258f" : cell.text;
td.classList.toggle("numeric", cell.numeric && !typing); td.classList.toggle("numeric", cell.numeric && !typing);
td.classList.toggle("error", cell.error); td.classList.toggle("error", cell.error);
td.classList.toggle("selected", cell.selected); td.classList.toggle("selected", cell.selected);
td.classList.toggle("typing", typing); td.classList.toggle("typing", typing);
// `editable` is the server's answer relayed through Rust: a stored // `editable` is the server's answer relayed through Rust: a stored cell.
// cell. An aggregate is a drill target and drill is not on the wire // Drill is not on the wire yet, so an aggregate is not offered.
// yet, so it is simply not offered here.
td.classList.toggle("editable", cell.editable); td.classList.toggle("editable", cell.editable);
if (cell.editable) if (cell.editable)
td.addEventListener("click", () => edit(td, dataRow, c, cell.edit_text)); td.addEventListener("click", () => edit(td, dataRow, c, cell.edit_text));
@@ -423,9 +297,8 @@ function indicator(collapsed) {
function edit(td, row, col, seed) { function edit(td, row, col, seed) {
if (editing) return; if (editing) return;
editing = { row, col }; editing = { row, col };
// `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn // `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn one:
// one. `td.textContent` has been through the number format, so a stored // `td.textContent` has been through the number format.
// 1200 reads "1,200" and committing that stores the string "1,200".
const original = seed; const original = seed;
const input = document.createElement("input"); const input = document.createElement("input");
input.value = original; input.value = original;
@@ -434,7 +307,7 @@ function edit(td, row, col, seed) {
input.select(); input.select();
// Enter removes the focused <input>, which fires `blur`, which calls // Enter removes the focused <input>, which fires `blur`, which calls
// `close` again. Harmless before, but only by accident (improvise-hwpi). // `close` again. Harmless before, but only by accident.
let closed = false; let closed = false;
const close = (commit) => { const close = (commit) => {
if (closed) return; if (closed) return;
@@ -475,16 +348,8 @@ function categoryList(model) {
return list; return list;
} }
// ── the keyboard ───────────────────────────────────────────────────────── // The keyboard. `Client::on_key_event` folds locally where it can; what it
// // cannot answer is sent and does nothing, by design (context/code/clients.md).
// `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` (keys.rs carries the list, and what is still
// upstream). 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 this client, not that arm.
window.addEventListener("keydown", (e) => { window.addEventListener("keydown", (e) => {
// The inline editor owns the keyboard while it is open. It stops // The inline editor owns the keyboard while it is open. It stops
@@ -496,18 +361,15 @@ window.addEventListener("keydown", (e) => {
const frame = client.on_key_event(e.key, e.ctrlKey, e.altKey, e.shiftKey, e.metaKey); const frame = client.on_key_event(e.key, e.ctrlKey, e.altKey, e.shiftKey, e.metaKey);
if (frame) { if (frame) {
// A frame means the page claimed the key, so the browser must not also // A frame means the page claimed the key, so the browser must not also act
// act on it — an arrow that both moves the cursor and scrolls the page is // on it. Ctrl/Alt combinations are left alone: they are the browser's.
// the worse of the two errors. Combinations under Ctrl or Alt are left
// alone regardless: those are the browser's own shortcuts, and this
// client resolves nothing under them anyway.
if (!e.ctrlKey && !e.altKey) e.preventDefault(); if (!e.ctrlKey && !e.altKey) e.preventDefault();
transport.send(frame); transport.send(frame);
} }
repaint(); repaint();
}); });
// ── start ──────────────────────────────────────────────────────────────── // Start.
await transport.ready; await transport.ready;
sendViewport(); // the handshake: the snapshot is built at this extent sendViewport(); // the handshake: the snapshot is built at this extent
+9 -46
View File
@@ -1,45 +1,12 @@
// The worker half of the standalone deployment (improvise-9x6). // The worker half of the standalone deployment (improvise-9x6): strings only,
// // every decision in Rust. The two rules it must not break: context/code/clients.md.
// It hosts the authoritative App. Everything it decides is decided in Rust —
// this 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: JS here is untested (there is no
// browser in this repository), so the less of it 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 (gotcha 33). 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 here (gotcha 28).
//
// 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
// below 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 document is named in this 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).
import init, { start } from "./worker-pkg/improvise_worker_server.js"; import init, { start } from "./worker-pkg/improvise_worker_server.js";
const params = new URLSearchParams(self.location.search); const params = new URLSearchParams(self.location.search);
const DOC = params.get("doc") || "demo.improv"; const DOC = params.get("doc") || "demo.improv";
// Where the first copy of the document comes from on an origin that has never // Where the first copy of the document comes from on a fresh origin: fetched
// seen one. It is fetched and handed to Rust, which WRITES it through the VFS // and WRITTEN through the VFS, never injected past storage.
// and then reads it back like any other document — not injected past storage.
const SEED = params.get("seed") || "demo.improv"; const SEED = params.get("seed") || "demo.improv";
// Fetch the seed if there is one to fetch. A missing seed is not an error: // Fetch the seed if there is one to fetch. A missing seed is not an error:
@@ -66,9 +33,8 @@ async function handle(data) {
await server.flush(); await server.flush();
} }
// Startup, and the head of the chain. The client sends its `Viewport` as soon // Startup, and the head of the chain: frames arrive before this settles, and
// as it loads, so frames arrive before this settles; chaining onto it is what // chaining onto it holds them in arrival order with no queue to drain.
// holds them, in arrival order, without a queue to drain.
const started = (async () => { const started = (async () => {
await init(); await init();
server = await start(DOC, await seedBytes()); server = await start(DOC, await seedBytes());
@@ -78,8 +44,7 @@ started.catch((err) => {
}); });
// One frame at a time, in arrival order: the listener is deliberately NOT // One frame at a time, in arrival order: the listener is deliberately NOT
// `async`. It appends to the chain and returns, so the ordering does not // `async` — see context/code/clients.md.
// depend on when the event loop chooses to resume a suspended listener.
let tail = started.catch(() => {}); let tail = started.catch(() => {});
self.onmessage = (event) => { self.onmessage = (event) => {
@@ -91,10 +56,8 @@ self.onmessage = (event) => {
await handle(event.data); await handle(event.data);
}) })
.catch((err) => { .catch((err) => {
// An unhandled rejection inside a `message` listener reaches the // An unhandled rejection here reaches the devtools console and nothing
// devtools console and nothing else. Say which frame it was; the main // else. Caught, so one bad frame does not poison every later one.
// thread cannot see this and has no way to ask. Caught here rather than
// left on the chain so one bad frame does not poison every later one.
console.error("improvise worker: frame rejected", event.data, err); console.error("improvise worker: frame rejected", event.data, err);
}); });
}; };