Merge pull request #57 from fiddlerwoaroof/fix/web-review-findings

fix(web): land the review findings main is missing

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 <pid> -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
@@ -996,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.

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,
   },

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",
+    );
+  });
+});

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",

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",

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);