docs(repo-map): three waves of debt, and a row pointing at a deleted file
The 'Change a browser<->server frame' row still named improvise-ws-server/src/wire.rs, which improvise-xks6 deleted -- the first place the next agent looks. Flagged by a reviewer and missed twice. improvise-worker-server appeared nowhere in the map at all: not the crate list, not the how-to-find table, not the inventory, and the Wasm section still said six bundle crates. Gotchas 45-47 for the three undocumented waves -- the worker host (and the three gaps it found in the SHARED layer, which the socket server also has), the page's two transports plus the assertion the demo was built against, and client-side key resolution. Each was proposed by its agent and never applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EoW5BB87ZZdm96SvBTWawH
This commit is contained in:
Edward Langley
+41
-2
@@ -11,6 +11,7 @@ Apache-2.0. Root binary+lib `improvise`; sub-crates under `crates/`:
|
||||
- `gp-tree` — problem-agnostic genetic-programming regression trees (`Tree`, `Spec`, `breed`, `simplify`, `render`). Knows nothing about improvise.
|
||||
- `improvise-gp` — the one seam between the two: emits a fitted `gp_tree::Tree` as improvise formula text (`formula_text` / `fitted_formula`). Depends on `gp-tree`, `improvise-formula` **and `improvise-core`** — `dataset.rs` builds a feature matrix from a `View`'s records (improvise-4s8) and `fitted_formula` takes the `&Model` it must resolve against (improvise-8xhy). Nothing depends on it yet.
|
||||
- `improvise-ws-server` — the host: tokio + tungstenite, a `Registry` of `Host`s keyed by `DocumentId` (multi-document, ADR 001), `Sessions` fan-out, and the socket framing (`wire.rs`). Native only.
|
||||
- `improvise-worker-server` — the *other* host: the same `App`, `Sessions` and `Host` as the socket server, behind `self.onmessage`/`self.postMessage` with OPFS underneath. wasm32 only in anger; `worker.rs`/`vfs.rs` are ordinary Rust and are where every decision and every test is. Nothing depends on it.
|
||||
- `improvise-wasm-client` — the browser client: the fold, the key classifier, the socket framing it mirrors (`socket.rs`), and the render model the DOM paints (`render.rs`). Non-dev deps are `improvise-protocol` + serde + wasm-bindgen and nothing else. The page itself is `web/`.
|
||||
|
||||
The main crate (and `improvise-command`) re-export the sub-crates as `crate::{model, view, workbook, format, formula, persistence, import}` so consumer paths stay stable when crates shuffle. The main crate also re-exports `improvise_command::command` as `crate::command` and `improvise_command::ui::{app, cat_tree, effect, viewport}` under `crate::ui`. `ui::app` re-exports the protocol's `AppMode`, `ViewState`, `ViewCursor`, `ViewFrame`, `Panel`, `MinibufferConfig`, `MinibufColor` and `HELP_PAGE_COUNT`, and `command::key` re-exports the protocol's key types, so every pre-existing path still resolves.
|
||||
@@ -44,7 +45,11 @@ Architectural intent lives in `context/design-principles.md` — read that for t
|
||||
| Change import-wizard keys/steps | `command/cmd/wizard.rs` (keys) + `import/wizard.rs` (steps) |
|
||||
| Change frame layout | `draw.rs` → `draw()` |
|
||||
| Change what the browser page draws | `improvise-wasm-client/src/render.rs` → `RenderModel` |
|
||||
| Change a browser↔server frame | `improvise-wasm-client/src/socket.rs` **and** `improvise-ws-server/src/wire.rs` — both, they are mirrors |
|
||||
| Change a browser↔server frame | `improvise-protocol/src/frame.rs` → `ClientFrame`/`ServerFrame`. The **one** pair; both ends `pub use` it and neither defines one (improvise-xks6) |
|
||||
| Change the worker's message loop | `improvise-worker-server/src/worker.rs` → `Worker::on_frame` |
|
||||
| Change the session loop (either host) | `improvise-command/src/ui/host.rs` → `Host::handle` |
|
||||
| Change how the browser opens or seeds a document | `improvise-worker-server/src/vfs.rs` → `open` / `documents` |
|
||||
| Change what a browser resolves without the host | `improvise-wasm-client/src/keys.rs` → `resolve` |
|
||||
| Change the browser page itself | `web/app.js` (thin), `web/index.html`, `web/style.css` |
|
||||
| Host another document | `improvise-ws-server/src/registry.rs` → `Registry`, `ClientId` |
|
||||
| Change app state / modes | `ui/app.rs` → `App`, `AppMode` |
|
||||
@@ -227,6 +232,24 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
|
||||
|
||||
44. **The TUI grid draws from its cache, but the cache does not frame it.** Since improvise-n10 `GridWidget` holds no `&Model`: cell text, aggregate-ness (`CellId::Aggregate`, ADR 002 — the widget used to walk `none_cats` × `kind.is_regular()`, a second hand-rolled copy of `project::cell_id`), column widths, the page bar and the totals strip all come off `GridCache`, and the TUI builds that cache over the **whole** grid (offsets zero), so a cache position is an absolute position — the browser's `Viewport` is a real window, the TUI's is not, and the two clients therefore use it differently. Column totals **must** be projected rather than client-derived, because the sum runs over every row *including the ones a viewport scrolled past*. Six facts still do not come off the cache: `row_cats`/`col_cats` (how many header levels), `row_items` (multi-level label tuples **and** inline group headers), `col_group_for` (column group spans), `is_group_collapsed` (▼/▶), and `record_col` — the last of which is **correct** to keep off, being session-local and what keys pending drill edits (gotcha 1). The other five are a **gap, and a visible one**: `GridCache::row_labels` is `GridLayout::row_label`, which slash-joins a multi-level tuple — the exact rendering `two_row_categories_produce_cross_product_labels` asserts must *not* appear — so the browser grid shows `Food/Alice` where the TUI shows two header columns, with no groups and no collapse indicators. Closing it changes the *type* of `row_labels`/`col_labels`, which `improvise-wasm-client` consumes, so it is a co-ordinated change, not an additive one; adding a `row_levels` beside `row_labels` is the wrong fix, because two answers to "what are the labels" is the disease (improvise-edp). Also still derived in the widget, and so a second rule waiting for a browser to invent: the **row-header width** (`compute_col_widths` moved to `ui::viewport`; its sibling did not). And `project::grid_of` now evaluates every cell per frame where the widget evaluated only the visible ones — a widening of an existing whole-grid cost, unbenchmarked.
|
||||
|
||||
45. **One session loop, two transports — and the browser saves itself.** `Host` is `App` + `Sessions` + `handle(id, ClientFrame) -> Vec<Addressed>` and lives in `improvise_command::ui::host`, **not** in `improvise-ws-server`, which re-exports it. It moved for improvise-9x6: the worker hosts the same authoritative `App` over a `MessagePort` and cannot depend on tokio, and a second copy would have been exactly the mirror improvise-xks6 had just deleted for the frames — a drifting `Applied`/`Broadcast` match is invisible until a client sees it. `registry` stayed on the server, because a worker hosts **one** document. What the worker adds is only what a `MessagePort` differs in: **one client by construction** (the `Addressed` filter is honoured anyway, against a future `SharedWorker`), and **a handshake with no close** — `ClientFrame::Viewport` must still come first, but a pre-handshake frame is *dropped* rather than closing a connection that does not exist.
|
||||
|
||||
**Saving is not on the wire, and nothing marks a wire edit dirty.** `effect::Save` is `ViewOnly` (gotcha 3) — correctly, since it writes *this host's* file path — so the derivation that builds `ModelCommand` from the model effects cannot name it: **no client on any transport can ask its host to save**, the socket server included. Worse, `effect::SetCell` does not set `model_state.dirty` either — dirtiness is a *separate* `MarkDirty` effect that only the command/key path emits — so "save if dirty" is a trigger that **never fires**, and a host built on it loses every client edit silently. That is a derivation which was correct per element and wrong for a set: improvise-kmgs mapped effects one at a time, so the wire carries `SetCell` without its `MarkDirty` companion. `improvise-worker-server::worker::touches_document` therefore keys off the command **variant** (`Command::Model | Command::SwitchView`, skipped on `Rejected`), a deliberate over-approximation costing one redundant whole-document write. Right for a browser on its own terms — there is no file the user owns and no Ctrl-S semantics, the document *is* the OPFS entry — but a **gap on the socket**, not a browser peculiarity.
|
||||
|
||||
**The async seam does not survive being boxed.** `OpfsFs::open`/`attach`/`refresh`/`commit_deletes` are on the struct, not on `Storage`, and `App::storage` is a `Box<dyn Storage>` — so handing the backend to the `App` puts all four permanently out of reach. `impl Storage for Rc<OpfsFs>` is two foreign types and the orphan rule refuses it, so every worker needs a local newtype. **And the flat root is load-bearing**: `OpfsFs::list` returns *bare* names where `MemoryFs`/`PhysicalFs` return `dir`-joined ones, so `Path::new("")` is the one `dir` at which the three agree and the only one at which `storage_contract` describes all of them. A worker that used `docs/` would get an empty picker in a browser and a full one in every test.
|
||||
|
||||
**What no test here can see**, on the same terms as gotcha 42: `bindings.rs` and `web/worker.js`. `createSyncAccessHandle` exists in no harness, so **`OpfsFs` remains compile-checked and never run** — if persistence misbehaves in a browser it is the first place to look. What *is* verified is everything above the `Storage` trait: `tests/post_message.rs` drives a real `Worker` over a real `App` with nothing but the JSON a `MessagePort` carries. One build note: the worker bundle needs `--enable-nontrapping-float-to-int` **as well as** `--enable-bulk-memory`, because it carries the whole `App` and so chrono's date arithmetic.
|
||||
|
||||
46. **The page picks a peer, and the demo was built against an assertion.** `web/app.js` holds two transports and no protocol knowledge: `?ws=` selects `improvise-ws-server`, its absence spawns `new Worker('./worker.js?doc=&seed=', { type: "module" })` (improvise-djm). `type: "module"` is mandatory — `worker.js` `import`s the bundle — and there is **no `MessageChannel`**: a dedicated worker's own `postMessage`/`onmessage` *is* the port, so the ticket's channel wording was a date rather than a decision. The websocket path is **kept**, because the ws-server is how a TUI-hosted document is shared and nothing else in the browser can reach it; the cost is contained because the two differ only in *where strings come from* — handshake, fold, repaint and keyboard are all above the seam — and **the presence of the parameter is the whole selection mechanism**, a second knob being a second thing to get wrong. `#status` composes **two** facts, the transport's and the document's, because a single source let a repaint silently erase a connection error. **A worker that rejects in `start()` fires no `error` on the main thread** — `worker.js` catches its own — so the guard is a **first-frame watchdog**, deliberately *not* per-transport: "the peer never answered the handshake" is the one failure that means the same thing over a socket and over a port, and a check aimed at each peer's own failure modes would have missed exactly this one.
|
||||
|
||||
**The demo is a model built to satisfy a check, not an axis flipped and hoped for.** `examples/demo.improv` leaves `Customer`/`Date` on None, so every cell is an `Aggregate` and nothing is editable — and the naive fix does not work either, since `Product × Region` by `Date_Month × Measure` still aggregates. `web/demo.improv` puts **exactly one category on each visible axis** (`Product` row, `_Measure` column, `Quarter` page, nothing regular on None) and carries `Profit = Revenue - Cost`, so the payoff is on one screen with no keystroke. **Settability is asked of `grid_of` as a `CellId` variant**, never re-derived from the axes — a second derivation could agree with itself while disagreeing with `render.rs::editable`, which is the whole point of ADR 002 — and the assertion was proved discriminating by pointing it at `examples/demo.improv`, where it names the aggregate at (0,0). **One category per axis is a property of the demo, not of the format**: `GridCache` slash-joins a multi-level tuple (gotcha 44), so a second category on an axis puts that known gap on the front page. `web/*.improv` is un-ignored in `.gitignore` (`*.improv` is ignored repo-wide) and the CI assemble step copies it: leaving it out is an empty grid, not a 404 the page could report. One wart it now puts on the front page: a formula cell **is** a `Record`, so it is offered as editable and a write to it reverts on recompute — pre-existing and matching the TUI, newly visible.
|
||||
|
||||
47. **Keys become commands in the browser, and the server must not know the keymap.** The user's ruling on improvise-bpq2, following from "the model is shared, the UI is not" — and it buys two clients different bindings over one document. What made it urgent is **not** latency. `a_key_resolved_on_the_host_moves_state_no_client_can_hear` shows nothing in the projection stream carries the host's own `ViewState`, so host-side resolution **diverges**; and `Host::handle` answers `ClientFrame::Key` with an explicit empty vec, so **an upstream key is a black hole, not a slow path** — every `KeyOutcome::Upstream` names a binding that does nothing at all in a browser. That test still holds and was not flipped: bpq2 closes the divergence by *avoiding* it. **The method is the ticket's** — enumerate the **bindings**, not the commands (`doctrine(binding_dump_diff)`) — and where a client cannot compose the `Command`, **enrich the projection** rather than moving resolution back.
|
||||
|
||||
Three things that cost. **`GridExtent` on `GridCache`** — the whole grid, of which the labels are one window — because a viewport-indexed cache cannot tell "the last visible row" from "the last row". It is deliberately **not framed by the labels** (`fit_to_labels` leaves it alone) and its batch position is not load-bearing the way `RowLabels`-before-`Cells` is, since it addresses no position; the producer reads `GridLayout::row_count()`, and reading it off the projected ranges is **invisible in the TUI** (which builds at offsets zero, gotcha 44) and is exactly the browser bug. **The window is the client's own fact**, recorded by `Client::set_viewport` from what it announced — *not derivable*, since `row_labels.len()` is the window **clipped to the grid** and so differs precisely at the bottom edge where the scroll arithmetic matters. `Client::viewport` (associated, no recording) still exists and is the one that quietly turns cursor keys off; `web/app.js` must call `set_viewport`. **`ClientFrame::Commands(Vec<Command>)`** replaces the singleton, because one cursor key is `SetSelected` plus the offsets `viewport_effects` returns and a host that folds the selection but not the offset projects the wrong window; `Host::commands` applies in order and **abandons at the first rejection**, which leaves a hole ADR 001's "not even partially" does not have — `Checked` is per-command and a frame is a sequence of them (unreachable from any client here today).
|
||||
|
||||
Two corrections to the ticket's own gap list, both worth more than the code. **Panel cursor movement was never a gap**: improvise-edp had already made `viewmodel::flatten_cat_tree` the one home both ends call, so it is the *shared* derivation, not a second one — worth re-checking the "known gaps" in other old tickets against what has landed since. And the "unmodified characters only" rule was **mis-scoped, not wrong**: it was about `KeyPattern::AnyChar`, which is bound only in the text-entry modes, so `Ctrl-d` in Normal has an exact binding and mirroring it is a faithful subset. Each resolver mirrors its `Cmd` **line for line, including where siblings disagree** — `Move` and `MoveTileCursor` emit even when they clamp, `MovePanelCursor` returns nothing when the cursor does not move. Tidying that into one rule would make the client a second derivation, which is what `tests/lockstep.rs` exists to rule out. What still needs the host is a **named list** in `keys.rs` and in `the_normal_mode_keys_that_still_need_the_host_are_these`, so closing one has to come there and say so.
|
||||
|
||||
---
|
||||
|
||||
## File Inventory
|
||||
@@ -380,6 +403,21 @@ web/build.sh wasm-pack build → web/pkg (gitignored). N
|
||||
wasm-opt rejects LLVM's memory.copy
|
||||
```
|
||||
|
||||
### Browser worker host (`crates/improvise-worker-server/`)
|
||||
```
|
||||
improvise-worker-server/src/worker.rs 600 / 11t Worker: one port, one client; handshake, Addressed
|
||||
filter, persist trigger. Every decision, every test
|
||||
improvise-worker-server/src/vfs.rs 243 / 5t documents/open: seeding is a Storage::write
|
||||
improvise-worker-server/src/bindings.rs 187 / 0t wasm32 shim: OpfsFs awaits + postMessage. NEVER RUN
|
||||
improvise-worker-server/tests/post_message.rs 375 / 7t main-thread traffic, strings only
|
||||
improvise-command/src/ui/host.rs 842 / 17t Host (moved from ws-server): one App + one Sessions
|
||||
web/worker.js 85 three string calls, no protocol
|
||||
knowledge. NEVER RUN
|
||||
web/demo.improv the document the site ships and
|
||||
seeds from. Every cell a Record —
|
||||
asserted (gotcha 46)
|
||||
```
|
||||
|
||||
### Examples
|
||||
```
|
||||
examples/gen-grammar.rs Grammar-walking random .improv generator (pest_meta)
|
||||
@@ -411,7 +449,8 @@ docs/design-notes.md Product vision & non-goals
|
||||
## Wasm
|
||||
|
||||
The browser bundle is `improvise-core`, `improvise-formula`, `improvise-io`,
|
||||
`improvise-protocol`, `improvise-command` and `improvise-wasm-client`. All six
|
||||
`improvise-protocol`, `improvise-command`, `improvise-wasm-client` and
|
||||
`improvise-worker-server`. All seven
|
||||
`cargo check --target wasm32-unknown-unknown` clean with no feature flags
|
||||
(improvise-ywd), gated by the `wasm` job in `.github/workflows/ci.yml`.
|
||||
`improvise-ws-server` is the host and is native only — tokio, tungstenite and
|
||||
|
||||
Reference in New Issue
Block a user