diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index d964defd..eca8bee0 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 @@ -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. 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/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", + ); + }); +}); 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);