From 955c30a8641a8f7a726d130880a20cdf29f3ff81 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 13:03:14 -0700 Subject: [PATCH 1/2] fix(web): land the review findings main is missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repair-forward for the accidental push at 959f249, which put the PRE-review version of the browser-widget work onto main. The three defects review found are all in the version main currently carries; this is the delta that supersedes it. 1. `categoryList` did not call `indicator()`. It inlined U+25B8/U+25BE, the SMALL triangles, where `indicator()` — the page's declared ONE spelling — the page's own grid group headers, and src/ui/category_panel.rs all draw U+25B6/U+25BC. The browser's category panel disagreed with the TUI's and with its own grid, inside the file whose comment says "only this spelling is the page's". 2. Two specs pinned the divergent pair, so the obvious cure would have failed tests that shipped with the feature. widgets.spec.js AND pointer.spec.js — the latter written on a different branch, which had grown the same wrong assertions independently. Three files converging on a spelling nothing compares is design-principles §5's predicted failure, observed. Proved discriminating by running the corrected specs against the pre-fix bundle, where they fail with `Received string: "▸ Product (4) R"`. The pointer.spec.js line is in this PR and not a later one because main already carries that file: without it this change leaves CI red. It is one assertion corrected by the same rule as its sibling, not feature work. 3. `el.tiles.hidden` was a no-op, removed with its false comment. `#tiles` declares `display: flex`, an author-origin rule that beats the UA sheet's `[hidden] { display: none }`, so the guard never hid anything. Worth knowing before restoring it: `#tiles` being ALWAYS laid out is why its height never changes and it stays out of the ResizeObserver feedback path. Also `playwright.config.js` takes IMPROVISE_HARNESS_PORT (default 8099, so CI is unchanged). The hard-coded port plus `reuseExistingServer` let Playwright silently ADOPT a server left by another worktree and test THAT tree. It fired in both directions in one session: a false negative (a fix reported absent) and — the dangerous half — a false POSITIVE, 5 passed against a tree verified to contain no #tiles at all. A failure sends you looking; a false pass prompts nobody. The evidence prose records all of it, plus the ResizeObserver acyclicity proof that retires "argued, not tested" there: frameCost.cols is row header LEVELS (independent of projected rows) and frameCost.rows derives from the column window, so cols -> rows and nothing depends on rows. No cycle to oscillate. Gates: 1359 Rust tests, rustfmt, comment-run and evidence linters, 21/21 Chromium harness (on a private port, so not another checkout's tree). Co-Authored-By: Claude Opus 5 (1M context) --- context/test-evidence/clients.md | 78 ++++++++++++++++++++++++++++++++ harness/playwright.config.js | 10 ++-- harness/tests/pointer.spec.js | 4 +- harness/tests/widgets.spec.js | 7 +-- web/app.js | 7 +-- 5 files changed, 95 insertions(+), 11 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index d964defd..569049b2 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -923,6 +923,84 @@ because Product then shares the column axis with `_Measure`, so the label appears once per header level — which is itself a small confirmation that gotcha 44's multi-level frame reaches the browser. +**Three review findings, applied.** The second is the one worth carrying. + +**The page had two spellings of the same triangle, inside the file that +declares it has one.** `indicator()` carries the comment *"only this spelling is +the page's (gotcha 26)"* — singular — and `categoryList` did not call it: it +inlined U+25B8/U+25BE (the SMALL triangles) where `indicator()`, the page's own +grid group headers and `src/ui/category_panel.rs` all draw U+25B6/U+25BC. So the +browser's category panel disagreed with the TUI's *and* with its own grid. +`categoryList` now calls `indicator(!row.expanded)`. + +**And the first version of this spec pinned the wrong spelling.** It asserted +`/▸ Product/` and `▾` — the divergent pair — so the obvious cure, which the +file's own comment implies should already be in place, would have *failed a test +this work added*. That is this repo's characteristic defect exactly: not a wrong +line, a test that cements a state the doctrine says is wrong, and it makes the +cure more expensive the longer it sits. The spec now asserts `▶`/`▼`, and the +change was proved discriminating by running it against the pre-fix bundle, where +it fails with `Received string: "▸ Product (4) R"`. + +Note what this does **not** settle: whether glyphs should move to the protocol +the way `short_label()` did. That is **improvise-h04z**, not answered here — +what is fixed is the page contradicting itself. `viewRow` still spells `"▶ "` +as a literal duplicating `src/ui/view_panel.rs:62`; the two agree today, which +is precisely the state `categoryList` was in before it drifted. + +**And the drift was already spreading, which is the evidence that ticket needs.** +`harness/tests/pointer.spec.js`, written on a different branch, had +*independently* grown the same `▸`/`▾` assertions — so the corrected +`indicator()` broke it too. Three files converging on a spelling nothing +compares is the failure design-principles §5 predicts, observed rather than +argued, and it is what answers "is one duplicated literal worth a protocol +change" with data instead of doctrine. + +**`el.tiles.hidden` was a no-op**, removed with its comment. `#tiles` declares +`display: flex`, an author-origin rule that beats the UA sheet's +`[hidden] { display: none }` at any specificity, so the guard never hid +anything and "an empty tile list draws nothing at all" was false. `#views`, +`#formulas` and `#help` declare no `display` and so are genuinely hidden by the +attribute. Worth knowing before anyone "fixes" it: because `#tiles` is always +laid out, its height never changes, so it feeds nothing back into the +`ResizeObserver` — restoring the guard would introduce exactly the first-paint +height change the observer argument assumes away. + +**The `ResizeObserver` loop is no longer "argued, not tested", and the proof is +worth more than the measurement.** It was measured: 820 combinations — four +panel states (none / views / formulas / tile-select) × heights 360-440 in 2px +steps × five widths straddling the `COL_PX = 110` boundary, each counting +`#grid` child replacements in a 500ms quiet window. Every size settled; nothing +repainted twice. But the reason is structural, so the measurement is a check on +it rather than the evidence itself: **the `frameCost` dependency graph is +acyclic.** `frameCost.cols` is `row_levels`, the number of row-axis header +*levels*, which does not depend on how many rows are projected. +`frameCost.rows` derives from `col_levels.length` and `col_groups`, which depend +on the projected *column* window, which depends on width and `frameCost.cols`. +So `cols → rows`, and nothing depends on `rows`. There is no cycle to oscillate. +A change that made either term depend on the projected row count would create +one, and that — not the pixel arithmetic — is the thing to check. + +**A harness port collision can make a run test somebody else's tree, silently.** +`playwright.config.js` hard-coded 8099 with `reuseExistingServer: true`, so a +`python3 -m http.server` left by another worktree is *adopted*, and the run then +exercises that checkout with no warning. The port is now +`IMPROVISE_HARNESS_PORT`, defaulting to 8099 so CI is unchanged. The tell is a +result that disagrees with the file you just read: check +`lsof -a -p -d cwd` on the listener before believing it. + +**It fires in both directions, and the dangerous one is the false PASS.** Two +agents hit it in one session. One got a false *negative* — a glyph fix reported +absent, because the adopted tree did not have it — which is merely confusing, +since a failure sends you looking. The other got `widgets.spec.js` reporting +**5 passed against a tree it had content-verified as having no `#tiles` and no +`paintTiles` at all**, which is impossible and was caught only because that +agent knew what the tree contained and refused to believe the number. **A false +pass prompts nobody.** That is what makes a hard-coded port with +`reuseExistingServer` a correctness bug rather than an inconvenience, and it is +the same rule as the rest of this file: a green result is evidence only once you +know what it measured. + ### a_click_focuses_the_panel_moves_its_cursor_and_activates_the_row improvise-7ybz's whole difficulty in one assertion, and the reason the click diff --git a/harness/playwright.config.js b/harness/playwright.config.js index 34fcb0b4..607879fe 100644 --- a/harness/playwright.config.js +++ b/harness/playwright.config.js @@ -2,6 +2,10 @@ import { defineConfig, devices } from "@playwright/test"; // 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. +// `reuseExistingServer` will happily adopt a server another checkout started on +// this port, and then you test THAT tree. See context/code/clients.md. +const PORT = process.env.IMPROVISE_HARNESS_PORT ?? "8099"; + export default defineConfig({ testDir: "./tests", fullyParallel: false, @@ -11,7 +15,7 @@ export default defineConfig({ reporter: process.env.CI ? [["github"], ["list"]] : "list", timeout: 60_000, use: { - baseURL: "http://127.0.0.1:8099", + baseURL: `http://127.0.0.1:${PORT}`, trace: "retain-on-failure", }, // Chromium + Firefox run the OPFS battery; WebKit has no OPFS at all and is @@ -30,10 +34,10 @@ export default defineConfig({ }, ], webServer: { - command: "python3 -m http.server 8099 --bind 127.0.0.1 --directory serve", + command: `python3 -m http.server ${PORT} --bind 127.0.0.1 --directory serve`, // The APP, not the site root — since improvise-kh8 the root is the landing // page, which would answer 200 with no bundle assembled at all. - url: "http://127.0.0.1:8099/app/index.html", + url: `http://127.0.0.1:${PORT}/app/index.html`, reuseExistingServer: !process.env.CI, timeout: 30_000, }, diff --git a/harness/tests/pointer.spec.js b/harness/tests/pointer.spec.js index 3406fae9..dea2fe17 100644 --- a/harness/tests/pointer.spec.js +++ b/harness/tests/pointer.spec.js @@ -49,7 +49,7 @@ test.describe("clicking the category panel", () => { await openPanelThenLeave(page); const product = page.locator("#categories li").filter({ hasText: "Product" }); - await expect(product).toHaveText(/▸ Product \(4\)/); + await expect(product).toHaveText(/▶ Product \(4\)/); await expect(page.locator("#categories li.selected")).toHaveText(/_Index/); await product.click(); @@ -57,7 +57,7 @@ test.describe("clicking the category panel", () => { // All three, and the third is what the user reported as impossible. await expect(page.locator("#mode")).toHaveText("CATEGORIES"); await expect(page.locator("#categories li.selected")).toHaveText(/Product/); - await expect(product).toHaveText(/▾ Product \(4\)/); + await expect(product).toHaveText(/▼ Product \(4\)/); await expect(page.locator("#categories li.item")).toHaveText([ "Espresso", "Latte", diff --git a/harness/tests/widgets.spec.js b/harness/tests/widgets.spec.js index 052bcac1..79b1cc8d 100644 --- a/harness/tests/widgets.spec.js +++ b/harness/tests/widgets.spec.js @@ -20,12 +20,13 @@ test.describe("the category panel", () => { await page.keyboard.press("Shift+C"); await expect(page.locator("#mode")).toHaveText("CATEGORIES"); // web/demo.improv: _Index, _Dim, _Measure, Product, Quarter. Product is - // the first with items a user would recognise. + // the first with items a user would recognise. The glyphs are + // `indicator()`'s, which is the page's ONE spelling and the TUI's too. for (let i = 0; i < 3; i++) await page.keyboard.press("j"); - await expect(rows.filter({ hasText: "Product" })).toHaveText(/▸ Product \(4\)/); + await expect(rows.filter({ hasText: "Product" })).toHaveText(/▶ Product \(4\)/); await page.keyboard.press("Enter"); - await expect(rows.filter({ hasText: "Product" })).toHaveText(/▾ Product \(4\)/); + await expect(rows.filter({ hasText: "Product" })).toHaveText(/▼ Product \(4\)/); await expect(page.locator("#categories li.item")).toHaveText([ "Espresso", "Latte", diff --git a/web/app.js b/web/app.js index 10793417..50356864 100644 --- a/web/app.js +++ b/web/app.js @@ -427,9 +427,8 @@ function formulaRow(row) { } /** The tile bar. `selected` is only ever true in tile-select mode, which is - * decided in Rust; an empty tile list draws nothing at all. */ + * decided in Rust. The strip is always laid out — context/code/clients.md. */ function paintTiles(model) { - el.tiles.hidden = model.tiles.length === 0; const tiles = model.tiles.map((tile, index) => { const span = document.createElement("span"); span.className = "tile"; @@ -447,8 +446,10 @@ function categoryList(model) { const li = document.createElement("li"); li.className = row.is_category ? "category" : "item"; li.classList.toggle("selected", row.selected); + // `indicator`, not a second pair of triangles: this inlined U+25B8/U+25BE + // where the TUI and the page's own group headers draw U+25B6/U+25BC. li.textContent = row.is_category - ? `${row.expanded ? "▾" : "▸"} ${row.label} (${row.item_count}) ${row.axis}` + ? `${indicator(!row.expanded)} ${row.label} (${row.item_count}) ${row.axis}` : row.label; li.addEventListener("click", () => send(client.click_category(index))); list.appendChild(li); From f7d2214c7adaaa1168261ee509e3bd85560e4560 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 13:14:53 -0700 Subject: [PATCH 2/2] test(harness): drill is refused in the browser, and one refusal is silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourth widget reported, and the one with a different answer: there is no wire vocabulary for drill. reduce.rs contains no Drill variant at all, neither Command nor ModelCommand. That is improvise-gd28, and it is a decision rather than an oversight — drilling writes drill_state, which gotchas 30 and 1 rule session-local, and a Command::Model is routed with no SessionId. Adding ModelCommand::Drill would reintroduce exactly what improvise-3x3u removed. Everything below the command works, verified in the browser: CellId::Aggregate is emitted and received, editable answers false for it, and edit_or_drill reaches the aggregate arm. The TUI is the control and passes 12 drill tests, so this is isolated to the client/wire. The finding gd28 did not record is that the two entry points fail DIFFERENTLY. `i`/`a` declines out loud with a status. `>` — the key the hint line actually advertises as ">:drill" — is a name resolve::step does not handle, so it goes KeyOutcome::Upstream -> ClientFrame::Key -> Host::handle's empty vec: a black hole, not a slow path (gotcha 47). Silent. That asymmetry is why this reads as broken rather than refused. These tests document what is true rather than leaving folklore. They MAKE an aggregate through the tile bar (web/demo.improv has none by construction — gotcha 46) and assert the precondition, so they cannot pass against a grid of Records. Both proved by injection: silencing the aggregate arm fails both, the second on its check_the_checker control; making `>` resolve fails only the second. Evidence added to improvise-gd28 rather than filing a duplicate. Co-Authored-By: Claude Opus 5 (1M context) --- context/test-evidence/clients.md | 63 ++++++++++++++++++++++++++ harness/tests/drill.spec.js | 76 ++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 harness/tests/drill.spec.js diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 569049b2..eca8bee0 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1074,3 +1074,66 @@ the panel** (`PanelKeymap`'s Esc is `enter-mode normal`, nothing more). That is the only state in which the "focus" third of the click is observable — from a freshly opened panel the mode is already `CategoryPanel` and the assertion would pass without the focus commands existing at all. + +#### harness/tests/drill.spec.js — the file's own scope + +The fourth widget the user reported, and the one with a **different answer from +the other three**. The tile bar, view panel and formula panel were each one +`RenderModel` field short — cheap. Drill is not: **there is no wire vocabulary +for it**, which is improvise-gd28, and these tests document the refusal rather +than pretending the gap is a bug to be fixed here. + +**Where it stops, by layer.** Everything below the command exists and was +verified in the browser, not read: `project::grid_of` emits `CellId::Aggregate`, +the client receives it (`{"Aggregate":"750ab1f5ffa96d5e…"}` in `render_state`), +`render.rs::editable` correctly answers false for it, and `resolve.rs` reaches +the aggregate arm. What is missing is one link: `reduce.rs` contains **no +`Drill` variant at all** — not `Command`, not `ModelCommand`. The seam is ready +and the sentence is not sayable. + +**Why it was left out is a decision, not an oversight** (gotcha 38): drilling +creates the `_Drill` view and writes `drill_state`, which gotchas 30 and 1 rule +**session-local** — `pending_edits` are keyed by `(row, RecordCol)` and belong +to whoever staged them. A `Command::Model` is routed with **no `SessionId`**, +which is right exactly when a change is addressed to *no one* session; drill is +addressed to precisely one. That is the same argument that removed +`ApplyAndClearDrill` from the wire in improvise-3x3u, so adding +`ModelCommand::Drill` would reintroduce the bug that removal fixed. It needs the +**addressed** form — ADR 001 layer 2. + +**The two entry points fail differently, and that asymmetry is the finding.** +`i`/`a` on an aggregate declines **out loud**: `edit_or_drill` answers the +`CellId::Aggregate` arm with a status the user can read. But `>` +(`drill-into-cell`) is a name `resolve::step` does not handle, so it becomes +`KeyOutcome::Upstream` → `ClientFrame::Key` → `Host::handle`, whose `Key` arm is +an explicit empty vec — **a black hole, not a slow path** (gotcha 47). Pressing +`>` in the browser does nothing whatsoever: no status, no mode change, nothing. +That is why this reads as "never confirmed working" rather than as a refusal. + +**The TUI is the control and it works**: 12 drill tests pass across +`cmd/grid.rs`, `cmd/mode.rs`, `ui/effect.rs` and `ui/app.rs`. So this is +isolated to the client/wire, not a general regression. + +**A fixture fact that is itself part of the answer.** `web/demo.improv` has **no +aggregate cell at all** — gotcha 46 puts exactly one category on each visible +axis precisely so every cell is a `Record`, and `tests/web_demo_model.rs` +asserts it. So on the shipped demo a user cannot even reach the refusal without +first changing an axis. `anAggregateGrid` therefore *makes* one, through the +tile bar, the way a user would: `Quarter` cycles Page → None and every cell +becomes an aggregate. The precondition is then **asserted** (`td.editable` +count 0) rather than assumed, so neither test can pass against a grid of +Records. + +**Both injections, run.** Replacing the aggregate arm with `None` — silencing +the honest refusal — fails **both** tests, and the second fails on its +*`check_the_checker` control*, which is the point: with nothing answering, the +control can no longer tell "`>` is silent" from "the page is dead", and the test +correctly refuses to pass. Adding `"drill-into-cell" => Some(vec![SetStatus(…)])` +to `step` — making `>` resolve — fails **only** the second test and leaves the +first green. Two faults, two different failure signatures, neither test passing +for the other's reason. + +That control is load-bearing and not decoration: a test asserting *nothing +happened* passes against a page that has stopped listening, so it presses `i` +afterwards and requires `#status` to change. Without that, this file would be +folklore with a green tick. diff --git a/harness/tests/drill.spec.js b/harness/tests/drill.spec.js new file mode 100644 index 00000000..d0feca52 --- /dev/null +++ b/harness/tests/drill.spec.js @@ -0,0 +1,76 @@ +// Drill in the browser: what it does today, which is refuse. improvise-gd28 +// says why, and context/test-evidence/clients.md records the layer it stops at. + +import { test, expect } from "@playwright/test"; + +/// `web/demo.improv` has NO aggregate cell by construction — gotcha 46 puts +/// exactly one category on each visible axis so every cell is a Record, and +/// `tests/web_demo_model.rs` asserts it. So a drill test must MAKE one first. +async function anAggregateGrid(page) { + await page.goto("/app/index.html"); + await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); + await page.locator("#grid").focus(); + await expect(page.locator("#grid td.editable").first()).toBeVisible(); + + // Quarter is on Page; one cycle takes it to None, so every cell aggregates + // over the four quarters. The tile bar is how a user would do it. + await page.keyboard.press("Shift+T"); + const tiles = page.locator("#tiles .tile"); + const quarter = tiles.filter({ hasText: "Quarter" }); + await expect(quarter).toHaveText("Quarter Pag"); + const names = await tiles.allTextContents(); + for (let i = 0; i < names.findIndex((t) => t.startsWith("Quarter")); i++) { + await page.keyboard.press("ArrowRight"); + } + await expect(quarter).toHaveClass(/selected/); + await page.keyboard.press("Enter"); + await expect(quarter).toHaveText("Quarter ·", { timeout: 10_000 }); + await page.keyboard.press("Escape"); + + // The precondition, asserted rather than assumed: nothing is editable now, + // which is `CellId::Aggregate` reaching `render.rs::editable` (ADR 002). + await expect(page.locator("#grid td.editable")).toHaveCount(0); + await expect(page.locator("#grid tbody td").first()).toBeVisible(); +} + +test.describe("drill is refused in the browser, not broken", () => { + // The honest half: `i`/`a` on an aggregate declines OUT LOUD, because + // `edit_or_drill` answers the `CellId::Aggregate` arm with a status. + test("editing an aggregate says drill needs the host", async ({ page }) => { + await anAggregateGrid(page); + + await page.keyboard.press("i"); + await expect(page.locator("#status")).toContainText( + "drilling into an aggregate needs the host", + ); + // And it really is a refusal: no editor opened, no mode change. + await expect(page.locator("#mode")).toHaveText("NORMAL"); + await expect(page.locator("#grid input")).toHaveCount(0); + }); + + // The silent half, and the one worth pinning. `>` is `drill-into-cell`, + // which `resolve::step` does not name, so it becomes `KeyOutcome::Upstream` + // -> `ClientFrame::Key` -> `Host::handle`, whose Key arm is an empty vec + // (gotcha 47). An upstream key is a black hole, not a slow path. + test("the drill key does nothing at all, and the page is not merely dead", async ({ + page, + }) => { + await anAggregateGrid(page); + const before = await page.locator("#status").textContent(); + + await page.keyboard.press(">"); + // Nothing moves: not the mode, not the status, not the grid. + await expect(page.locator("#mode")).toHaveText("NORMAL"); + await expect(page.locator("#status")).toHaveText(before); + await expect(page.locator("#grid td.editable")).toHaveCount(0); + + // check_the_checker: a test that asserts "nothing happened" passes against + // a page that has stopped listening. This proves the keyboard is live, so + // `>`'s silence is specific to `>` — the refusal below DOES change #status. + await page.keyboard.press("i"); + await expect(page.locator("#status")).not.toHaveText(before); + await expect(page.locator("#status")).toContainText( + "drilling into an aggregate needs the host", + ); + }); +});