diff --git a/context/code/clients.md b/context/code/clients.md index cbc6d43a..f3f3b0cc 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -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 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 ``, 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. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index c89b06e2..28a6935d 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -144,7 +144,7 @@ would be a client claiming to have seen something it was never sent. ## `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 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 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 (ADR 001). The tiers are the ones lockstep never checks — the host applies the keypress directly rather than the client's `Command::Model` — but `BasisCache::claim` withholds until it has *something*, so a client without one 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 `` 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. diff --git a/crates/improvise-ws-server/src/main.rs b/crates/improvise-ws-server/src/main.rs index 2dc9b27f..d79cf508 100644 --- a/crates/improvise-ws-server/src/main.rs +++ b/crates/improvise-ws-server/src/main.rs @@ -1,9 +1,5 @@ -//! `improvise-ws-server` — serve improvise documents over a websocket. -//! ```text -//! 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`. +//! `improvise-ws-server model.improv [q3.improv …] --port 8787 --bind ADDR` — +//! serve documents over a websocket. Naming and limits: context/code/clients.md. use std::path::{Path, PathBuf}; diff --git a/harness/opfs-contract/src/lib.rs b/harness/opfs-contract/src/lib.rs index c5fb5faa..ee94335b 100644 --- a/harness/opfs-contract/src/lib.rs +++ b/harness/opfs-contract/src/lib.rs @@ -1,25 +1,5 @@ -//! Run `improvise_io::storage::contract::storage_contract` against `OpfsFs`, -//! inside a dedicated Web Worker, in a real browser. -//! -//! `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. +//! Run `storage_contract` against `OpfsFs`, inside a dedicated Web Worker, in +//! a real browser. Why a worker and why a report: `context/code/clients.md`. use std::path::Path; @@ -64,11 +44,8 @@ impl Report { } } -/// The origin's root directory, reached the way `OpfsFs::open` reaches 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. +/// The origin's root directory. Only the harness needs it: `OpfsFs` has no +/// directory vocabulary, so making one has to happen outside it. async fn opfs_root() -> Result { let scope = js_sys::global() .dyn_into::() @@ -102,18 +79,8 @@ async fn drop_directory(name: &str) -> Result<()> { Ok(()) } -/// Remove every *document* in the origin root, so a run does not inherit the -/// last one's leftovers. -/// -/// 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. +/// Remove every *document* in the origin root, using only the backend's own +/// vocabulary. What it cannot clear, and why: `context/code/clients.md`. async fn wipe(fs: &OpfsFs) -> Result { fs.refresh().await?; let names = fs.list(Path::new(""))?; @@ -156,12 +123,8 @@ pub async fn run_checks() -> Result { ) .await; - // `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). + // An attached-but-unwritten name is zero bytes and NOT a document; listing + // it is the improvise-9x6 browser failure. Evidence file. report .run( "an_attached_but_unwritten_entry_is_not_a_document", @@ -174,9 +137,8 @@ pub async fn run_checks() -> Result { "attach listed {listed:?} before anything was written" ); - // ...and a second instance, which shares no index, must not see - // it either: the filter has to be in `refresh`, not only in the - // omission from `attach`. + // ...and a second instance, sharing no index, must not see it: + // the filter has to be in `refresh` too, not only in `attach`. let other = OpfsFs::open().await?; other.refresh().await?; let seen = other.list(Path::new(""))?; @@ -197,12 +159,8 @@ pub async fn run_checks() -> Result { ) .await; - // `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 - // (found by Copilot on PR #24). `OpfsFs` never creates a directory, so - // the harness makes one itself. + // `keys()` yields DIRECTORIES too, and one must not be listed as a document + // (Copilot, PR #24). See `context/test-evidence/clients.md`. report .run("a_directory_entry_is_not_a_document", async || { wipe(&fs).await?; @@ -217,8 +175,7 @@ pub async fn run_checks() -> Result { ); // The precondition, so this cannot pass by the directory never - // having been created: a real file beside it IS listed, which - // also says `keys()` walked the root at all. + // having been created: a real file beside it IS listed. fs.attach(Path::new("beside.improv")).await?; fs.write(Path::new("beside.improv"), b"a real document")?; let again = OpfsFs::open().await?; @@ -234,14 +191,12 @@ pub async fn run_checks() -> Result { }) .await; - // 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. + // The hand-rolled `keys()` walk is the least certain code in opfs.rs: one + // entry could pass by accident, several out of order cannot. report .run("refresh_walks_every_key_and_sorts_them", async || { - // 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 the directory check above did. + // Wipes first, like every other check: a battery whose steps depend + // on the step before breaks when one is inserted. wipe(&fs).await?; for name in ["c.improv", "a.improv", "b.improv"] { fs.attach(Path::new(name)).await?; @@ -259,10 +214,8 @@ pub async fn run_checks() -> Result { }) .await; - // `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. + // `FileSystemReadWriteOptions`' `at` handling: invisible on a single short + // write, obvious across a long one followed by a short one. report .run("read_and_write_are_absolute_at_offset_zero", async || { wipe(&fs).await?; @@ -285,8 +238,7 @@ pub async fn run_checks() -> Result { .await; // 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. + // acquiring a handle is the async half and `Storage::write` cannot await. for dir in ["", "docs"] { report .run(&format!("storage_contract_at_{:?}", dir), async || { @@ -300,9 +252,8 @@ pub async fn run_checks() -> Result { .await; } - // Gotcha 28: a delete is visible to `read` and `list` at once and lands in - // OPFS at the end of the batch. Both halves, because either one alone - // would pass against a backend that got the timing exactly backwards. + // Gotcha 28: a delete is visible at once and lands in OPFS at the end of + // the batch. Both halves, or a backwards backend would pass. report .run( "a_delete_is_visible_at_once_and_lands_on_commit", @@ -341,10 +292,8 @@ pub async fn run_checks() -> Result { Ok(report.finish()) } -/// Write the state the reload check expects, then hand control back. -/// -/// 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. +/// Write the state the reload check expects, then hand control back. Split +/// from [`after_reload`] because only a reload can ask whether OPFS persisted. #[wasm_bindgen] pub async fn before_reload() -> Result { inner_before().await.map_err(js) @@ -398,13 +347,8 @@ fn js(e: anyhow::Error) -> JsError { JsError::new(&format!("{e:#}")) } -/// Deliberately fails, so the harness can prove it can see a failure. -/// -/// 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. +/// Deliberately fails, so the harness can prove it can see a failure — through +/// the *same* reporting path every real check uses. Evidence file. #[wasm_bindgen] pub async fn run_canary() -> String { let mut report = Report::default(); diff --git a/harness/playwright.config.js b/harness/playwright.config.js index 907ea7e7..094c67f5 100644 --- a/harness/playwright.config.js +++ b/harness/playwright.config.js @@ -1,31 +1,7 @@ import { defineConfig, devices } from "@playwright/test"; -// `python3 -m http.server` and one spec directory. 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`. -// -// # 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. +// The one place a line of web/*.js is executed, and where a change to either +// file belongs. Why, and why Playwright: context/code/clients.md. export default defineConfig({ testDir: "./tests", fullyParallel: false, @@ -38,23 +14,8 @@ export default defineConfig({ baseURL: "http://127.0.0.1:8099", trace: "retain-on-failure", }, - // improvise-xf1's acceptance criterion names Chrome, Firefox and Safari, - // and the OPFS spec is the reason: `createSyncAccessHandle` is the one API - // 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. + // Chromium + Firefox run the OPFS battery; WebKit has no OPFS at all and is + // kept for the day it does. The shell spec is Chromium-only: clients.md. projects: [ { name: "chromium", use: { ...devices["Desktop Chrome"] } }, { diff --git a/harness/tests/opfs.spec.js b/harness/tests/opfs.spec.js index 30288acb..aa0bfcf5 100644 --- a/harness/tests/opfs.spec.js +++ b/harness/tests/opfs.spec.js @@ -1,14 +1,5 @@ -// `storage_contract` against `OpfsFs`, in a browser (improvise-vvl3/xf1). -// -// `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. +// `storage_contract` against `OpfsFs`, in a browser (improvise-vvl3/xf1). The +// assertions here are about *the report*: context/test-evidence/clients.md. 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)); } -/** The checks the report must contain, in order. Named here 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. */ +/** The checks the report must contain, in order. Named here as well as in Rust + * on purpose — see context/test-evidence/clients.md. */ const EXPECTED = [ "open_and_refresh_see_an_origin_root_holding_no_documents", "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) => { 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 - // cannot quietly swallow a real regression: if Chromium or Firefox ever - // 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. + // cannot quietly swallow a real regression. See clients.md. const opfs = await page.evaluate(async () => { try { 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 - // never started is the failure this repo has a doctrine about. This drives - // the *same* reporting path every real check uses. + // never started is the failure this repo has a doctrine about. test("the harness can see a failure", async ({ page }) => { const report = await run(page, "run_canary"); expect(report).toHaveLength(1); @@ -88,10 +67,8 @@ test.describe("OpfsFs in a real browser", () => { ).toEqual([]); }); - // The one thing no in-page check can establish. gotcha 28: 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 one thing no in-page check can establish: "commit_deletes actually + // removed it" is only answered by a fresh page. test("what OPFS held survives a reload, and a committed delete does not", async ({ page, }) => { diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index e675ec92..447c789e 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -1,37 +1,21 @@ -// The browser shell: 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 (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. +// The browser shell: the three assertions improvise-hwpi could not write, plus +// the watchdog. The history is in context/test-evidence/clients.md. import { test, expect } from "@playwright/test"; const status = (page) => page.locator("#status"); /// 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, 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. +/// upgrade — deterministic where a closed port is not (evidence file). const NO_WEBSOCKET_HERE = "127.0.0.1:8099"; const json = (v) => JSON.stringify(v); -/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Named here because two tests have -/// to outlast it, and a number that silently disagrees with the page would -/// make them pass for the wrong reason. +/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Two tests have to outlast it, and a +/// number that silently disagreed with the page would pass for the wrong reason. const HANDSHAKE_TIMEOUT_MS = 8_000; -/// Record every distinct #status the page shows, not just the last one. -/// Several of these are about a message being REPLACED, which the final value -/// cannot see. +/// Record every distinct #status the page shows, not just the last one: several +/// of these are about a message being REPLACED. function logStatus() { window.__statusLog = []; new MutationObserver(() => { @@ -41,13 +25,8 @@ function logStatus() { } test.describe("the shell reports what its peer is doing", () => { - // 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. + // improvise-hwpi. The fault is injected at the *peer*, not in the page, so + // this exercises the real `on_frame` and the real catch. test("a frame the client cannot fold says so on screen", async ({ page }) => { await page.routeWebSocket("ws://127.0.0.1:8787/", (ws) => { 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 }); }); - // 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". 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. + // improvise-hwpi, plus the watchdog half on the same page load because it + // needs the same nine-second wait. See the evidence file. test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({ 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 - // 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 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. + // The other arm, which must keep working when the first is fixed. It runs + // past HANDSHAKE_TIMEOUT_MS on purpose — see the evidence file. test("a socket that opened and then dropped reads as disconnected, and stays that way", async ({ 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. - // The history is worth more than the assertions would have been, because - // 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. + // Two bugs this file once described as live, one of which never existed in + // the merged tree: context/test-evidence/clients.md keeps that history. - // The first-frame watchdog (gotcha 46), 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 first-frame watchdog, found by reading: no per-transport check would + // catch it. The mock accepts the connection and never answers. test("a peer that accepts and then says nothing trips the watchdog", async ({ page, }) => { @@ -172,9 +94,8 @@ test.describe("the shell reports what its peer is doing", () => { }); test.describe("the standalone deployment", () => { - // 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. + // No `?ws=`, so the page spawns the in-page worker over OPFS. If this draws + // a grid, worker.js, bindings.rs, OpfsFs and the seed fetch all ran. test("the page draws the demo document out of the worker", async ({ page }) => { await page.goto("/index.html"); 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(); }); - // 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 name says - // what is checked and this comment says what is not. + // What lands on screen is the HOST's value, not the string typed. What this + // deliberately does NOT cover: context/test-evidence/clients.md. test("committing an edit sends one frame and shows the host's value", async ({ page, }) => { @@ -219,10 +123,8 @@ test.describe("the standalone deployment", () => { await expect(cell).toHaveText("20,000", { timeout: 10_000 }); expect(before).not.toBe("20,000"); - // The editor is gone and did not come back. This is the half of the - // re-entry guard that IS observable: whatever `close` ran, no stray - // is left behind and the host's value is not overwritten by the - // `original` text the second close writes before repainting. + // The editor is gone and did not come back: the half of the re-entry guard + // that IS observable from the DOM. await expect(page.locator("#grid input")).toHaveCount(0); }); }); diff --git a/harness/www/opfs/worker.js b/harness/www/opfs/worker.js index 71b7d31a..612ad515 100644 --- a/harness/www/opfs/worker.js +++ b/harness/www/opfs/worker.js @@ -1,10 +1,5 @@ -// The dedicated Web Worker the OPFS contract runs inside. -// -// `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. +// The dedicated Web Worker the OPFS contract runs inside: `createSyncAccessHandle` +// exists only here. Every decision is in Rust; this moves strings. import init, { run_checks, diff --git a/web/app.js b/web/app.js index 0740f05a..7b58d76f 100644 --- a/web/app.js +++ b/web/app.js @@ -1,47 +1,5 @@ -// 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 — -// -// 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. +// The browser half of improvise-cr3, talking to either of two peers: five +// string calls, no protocol knowledge. See context/code/clients.md. 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`. const WS_PEER = params.get("ws"); -/** - * The websocket URL for `?ws=`, defaulting to TLS. - * - * `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. - */ +/** The websocket URL for `?ws=`, defaulting to TLS. Why, and when an explicit + * `ws://` is upgraded rather than honoured: context/code/clients.md. */ function socketUrl(peer) { const secure = location.protocol === "https:"; if (/^wss:\/\//i.test(peer)) return peer; @@ -95,9 +38,8 @@ const el = { await init(); const client = new Client(); -// `#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. +// `#status` carries two independent facts, composed rather than overwritten: +// a repaint must not quietly erase a connection error. let shellStatus = ""; let modelStatus = ""; function paintStatus() { @@ -108,19 +50,8 @@ function setShellStatus(text) { paintStatus(); } -// 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 -// 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`. +// What the grid's own frame costs, in whole cells, as of the last repaint — +// measured off the painted model, never predicted. context/code/clients.md. let frameCost = { rows: 1, cols: 1 }; /** The extent this window can show, in whole DATA cells. */ @@ -135,22 +66,12 @@ function extent() { function sendViewport() { const { rows, cols } = extent(); // `client.set_viewport`, NOT the associated `Client.viewport`: the same - // frame, but it also RECORDS the window. A client learns its window only - // 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). + // frame, but it also RECORDS the window. Why that matters: clients.md. transport.send(client.set_viewport(rows, cols)); } -// A peer that never answers the handshake. The socket has an `error` event -// for this; 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 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. +// A peer that never answers the handshake. Deliberately about the *first +// frame*, not either peer's own failure modes — context/code/clients.md. const HANDSHAKE_TIMEOUT_MS = 8000; let watchdog = null; function armWatchdog(message) { @@ -158,10 +79,8 @@ function armWatchdog(message) { watchdog = setTimeout(() => setShellStatus(message), HANDSHAKE_TIMEOUT_MS); } -// A peer that failed *loudly* has already said something more precise than -// the watchdog's guess, 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. +// A peer that failed *loudly* has already said something more precise, so the +// watchdog must be DISARMED and not merely pre-empted (PR #13 review). function setShellFailure(text) { clearTimeout(watchdog); watchdog = null; @@ -174,9 +93,8 @@ function onFrame(text) { try { client.on_frame(text); } catch (err) { - // `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 - // simply stopped updating with nothing on screen saying why (improvise-hwpi). + // `on_frame` returns a Result, which wasm-bindgen throws; thrown in a bare + // listener it reached the console alone and the grid just stopped. setShellStatus("bad frame — the grid has stopped updating (see console)"); console.error("improvise: frame rejected", text, err); return; @@ -184,7 +102,7 @@ function onFrame(text) { repaint(); } -// ── transports ─────────────────────────────────────────────────────────── +// Transports. /** The in-page worker: the standalone deployment, no server process. */ function workerTransport() { @@ -202,7 +120,7 @@ function workerTransport() { }); setShellStatus(`in-page worker · ${DOC}`); // 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() }; } @@ -210,12 +128,8 @@ function workerTransport() { function socketTransport() { const socket = new WebSocket(WS_URL); const unreachable = `cannot reach ${WS_URL} — is improvise-ws-server running?`; - // 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. + // Did this socket ever open? The page must record it itself: nothing on the + // close event can discriminate. See context/code/clients.md. let opened = false; const ready = new Promise((resolve) => { socket.addEventListener("open", () => { @@ -226,25 +140,10 @@ function socketTransport() { }); socket.addEventListener("message", (event) => onFrame(event.data)); // 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)); - // 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" 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. + // A socket that never opened did not "disconnect" (improvise-taxx). BOTH arms + // route through `setShellFailure`, which disarms the watchdog — clients.md. socket.addEventListener("close", () => setShellFailure(opened ? "disconnected" : unreachable), ); @@ -259,7 +158,7 @@ function socketTransport() { const transport = WS_URL ? socketTransport() : workerTransport(); -// ── painting ───────────────────────────────────────────────────────────── +// Painting. let painted = -1; 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.categories.replaceChildren(categoryList(model)); - // What the frame costs, so the NEXT viewport announcement is honest: the - // header lines, a group line if there is one, the row header's width in - // 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. + // What the frame costs, so the NEXT viewport announcement is honest. + // Inline group ROWS are deliberately NOT counted — context/code/clients.md. const next = { rows: model.col_levels.length + (model.col_groups.length > 0 ? 1 : 0), cols: model.row_levels, @@ -338,9 +219,8 @@ function gridTable(model) { for (const group of model.col_groups) { const th = document.createElement("th"); th.colSpan = group.span; - // `label` is null for a run in no group. An empty STRING is a legal - // group name and still gets its indicator, which is why this is a null - // check and not a truthiness test. + // `label` is null for a run in no group; an empty STRING is a legal group + // name, so this is a null check and not a truthiness test. th.textContent = group.label === null ? "" : indicator(group.collapsed) + " " + group.label; th.classList.add("group"); @@ -361,9 +241,8 @@ function gridTable(model) { } const body = table.createTBody(); - // The cell coordinates a click sends are DATA row indices, and a group - // header is not a data row -- so this counts rows rather than trusting the - // position in `model.rows`. + // The cell coordinates a click sends are DATA row indices and a group header + // is not a data row, so this counts rather than trusting `model.rows`. let r = -1; for (const row of model.rows) { const tr = body.insertRow(); @@ -389,21 +268,16 @@ function gridTable(model) { row.cells.forEach((cell, c) => { const td = tr.insertCell(); - // A keyboard edit is open on the selected cell when `model.editing` is - // a string; it holds what has been typed so far. Both facts are - // 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. + // A keyboard edit is open when `model.editing` is a string, and it holds + // what has been typed. Both facts are decided in Rust. const typing = cell.selected && typeof model.editing === "string"; td.textContent = typing ? model.editing + "\u258f" : cell.text; td.classList.toggle("numeric", cell.numeric && !typing); td.classList.toggle("error", cell.error); td.classList.toggle("selected", cell.selected); td.classList.toggle("typing", typing); - // `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 here. + // `editable` is the server's answer relayed through Rust: a stored cell. + // Drill is not on the wire yet, so an aggregate is not offered. td.classList.toggle("editable", cell.editable); if (cell.editable) td.addEventListener("click", () => edit(td, dataRow, c, cell.edit_text)); @@ -423,9 +297,8 @@ function indicator(collapsed) { function edit(td, row, col, seed) { if (editing) return; editing = { row, col }; - // `seed` is `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". + // `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn one: + // `td.textContent` has been through the number format. const original = seed; const input = document.createElement("input"); input.value = original; @@ -434,7 +307,7 @@ function edit(td, row, col, seed) { input.select(); // Enter removes the focused , 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; const close = (commit) => { if (closed) return; @@ -475,16 +348,8 @@ function categoryList(model) { return list; } -// ── 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` (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. +// 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). window.addEventListener("keydown", (e) => { // 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); if (frame) { - // A frame means the page claimed the key, 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 this - // client resolves nothing under them anyway. + // A frame means the page claimed the key, so the browser must not also act + // on it. Ctrl/Alt combinations are left alone: they are the browser's. if (!e.ctrlKey && !e.altKey) e.preventDefault(); transport.send(frame); } repaint(); }); -// ── start ──────────────────────────────────────────────────────────────── +// Start. await transport.ready; sendViewport(); // the handshake: the snapshot is built at this extent diff --git a/web/worker.js b/web/worker.js index 039222c2..5238ef52 100644 --- a/web/worker.js +++ b/web/worker.js @@ -1,45 +1,12 @@ -// The worker half of the standalone deployment (improvise-9x6). -// -// 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). +// 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. import init, { start } from "./worker-pkg/improvise_worker_server.js"; const params = new URLSearchParams(self.location.search); const DOC = params.get("doc") || "demo.improv"; -// Where the first copy of the document comes from on an origin that has never -// seen one. It 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. +// Where the first copy of the document comes from on a fresh origin: fetched +// and WRITTEN through the VFS, never injected past storage. const SEED = params.get("seed") || "demo.improv"; // 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(); } -// Startup, and the head of the chain. The client sends its `Viewport` as soon -// as it loads, so frames arrive before this settles; chaining onto it is what -// holds them, in arrival order, without a queue to drain. +// Startup, and the head of the chain: frames arrive before this settles, and +// chaining onto it holds them in arrival order with no queue to drain. const started = (async () => { await init(); 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 -// `async`. It appends to the chain and returns, so the ordering does not -// depend on when the event loop chooses to resume a suspended listener. +// `async` — see context/code/clients.md. let tail = started.catch(() => {}); self.onmessage = (event) => { @@ -91,10 +56,8 @@ self.onmessage = (event) => { await handle(event.data); }) .catch((err) => { - // An unhandled rejection inside a `message` listener reaches the - // devtools console and nothing else. Say which frame it was; the main - // 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. + // An unhandled rejection here reaches the devtools console and nothing + // else. Caught, so one bad frame does not poison every later one. console.error("improvise worker: frame rejected", event.data, err); }); };