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

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) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-20 13:03:14 -07:00
parent 959f249c4b
commit 955c30a864
5 changed files with 95 additions and 11 deletions
+78
View File
@@ -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
+7 -3
View File
@@ -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,
},
+2 -2
View File
@@ -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",
+4 -3
View File
@@ -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",
+4 -3
View File
@@ -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);