fix(web): the view and formula panels are clickable too (improvise-iioi)

`categoryList` got a click listener and its two siblings did not, so `Click`
had three variants and no ViewRow/FormulaRow. Demonstrated in Chromium:
Shift-V, click a row, nothing changes — mode, row list, active marker and
panel cursor all byte-identical.

This is the one a user hits first, and shipping the cure for one panel and not
the other two REPRODUCES THE ORIGINAL REPORT: someone who has just learned
category rows are clickable clicks a view row and reads the silence as broken,
which is literally what "I can't expand categories" meant (improvise-7ybz).

Both follow the existing rule — go there, then do what Enter does there — and
the two panels answer it differently, which is the rule applying rather than
one of them being half done:

  - A VIEW row focuses, moves the cursor and switches, because ViewPanel binds
    Enter to switch-view-at-cursor. It ends in Normal; the panel stays open.
  - A FORMULA row focuses and moves the cursor and NOTHING ELSE, because
    FormulaPanel binds no Enter at all. Opening the editor on a click would be
    an activation invented for the pointer that the keyboard does not have.

The trap is the same one the category click had: the activation must resolve
against the row CLICKED, never the panel cursor, because the SetPanelCursor
ahead of it is not folded yet. Injected and run — reading the cursor instead
gives SwitchView("Default") beside SetPanelCursor { cursor: 1 }, so the cursor
lands on Plan while the document switches to Default. Silent, and wrong in the
half nobody watches. The fixture holds two views and clicks the second for
exactly that reason.

Browser tests proved against the bundle built immediately before the fix: both
new ones fail there while the six existing pointer tests pass.

Also folds in the harness port fix: reuseExistingServer is now opt-in via
IMPROVISE_HARNESS_REUSE rather than on by default. A default-on reuse adopts a
server another checkout left on the port and tests THAT tree — which fired in
both directions in one session, including a false PASS. A path-hash default was
rejected because it still silently reuses when two agents share a worktree
path; an explicit opt-in cannot. CI never reused, so it is unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-20 13:26:35 -07:00
parent 0cb28c8de6
commit d7386b4308
5 changed files with 197 additions and 7 deletions
+62
View File
@@ -1137,3 +1137,65 @@ 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.
### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked
improvise-iioi, and it is the **same trap** as the category click one level
over: the activation must be resolved against the row **clicked**, never
`view_panel_cursor`, because the `SetPanelCursor` ahead of it has not been
folded when the resolver runs. Injecting exactly that — reading the name at the
cursor instead of at `index` — yields
`SwitchView("Default")` beside `SetPanelCursor { cursor: 1 }`, i.e. the cursor
lands on `Plan` and the document switches to `Default`. Silent, and wrong in the
half a user does not watch.
The fixture therefore holds **two** views and clicks the **second**. With one
view, or with a click on the row already under the cursor, the bug is invisible
— the same reason `a_click_focuses_the_panel_moves_its_cursor_and_activates_the_row`
clicks Product rather than `_Index`.
### a_formula_row_click_navigates_and_activates_nothing
The negative arm, and it is a statement about the *rule* rather than about the
formula panel. The rule is "go there, then do what `Enter` does there";
`ModeKey::FormulaPanel`'s keymap binds `a`/`n`/`o` to `enter-mode formula-edit`
and `d`/`Delete` to delete, and **no `Enter` at all**. So the rule yields
navigation and nothing else, and that is the rule applying rather than the fix
stopping half way.
The assertion is explicit that no `FormulaEdit` mode change appears, because the
tempting "improvement" is to open the editor on a click — which would be an
activation **invented** for the pointer that the keyboard does not have, and
therefore a second derivation of what a formula row does.
#### harness/tests/pointer.spec.js — the two panel tests added for improvise-iioi
The browser half. Both press `Escape` after opening the panel, leaving it **open
but unfocused** — the only state in which the *focus* third of the click is
observable, exactly as `openPanelThenLeave` does for categories.
Proved against the bundle built immediately before the fix: both fail there (the
view row does not switch, the formula row does not select) while the six
pre-existing pointer tests pass, so the failure is specific to the two panels
rather than to the harness.
What the view test asserts is deliberately three things — `#view-name`, the
`active` marker and the `selected` cursor — because switching ends in `NORMAL`
and a test that only checked the mode would pass against a click that did
nothing but leave the panel.
**One fact the reviewer's report supplied and this fix leans on**: shipping the
cure for one panel and not its two siblings *reproduces the original report*. A
user who has just learned category rows are clickable clicks a view row and
reads the silence as broken — which is literally what "I can't expand
categories" meant.
#### The harness port: reuse is now opt-in
`reuseExistingServer: !process.env.CI` became `=== "1"` on
`IMPROVISE_HARNESS_REUSE`. A default-on reuse adopts a server another checkout
left on the port and then tests **that tree**, which fired in both directions in
one session — a false negative and, worse, a false *positive*. A path-hash
default was considered and rejected: it still silently reuses when two agents
share a worktree path, where an explicit opt-in cannot. CI is unaffected (it
never reused) and a developer wanting the old speed sets the variable.
+73
View File
@@ -1118,6 +1118,79 @@ mod tests {
fn a_click_past_the_last_row_resolves_to_nothing() {
assert!(click(&with_categories(), Click::CategoryRow(9)).is_empty());
assert!(click(&ClientState::default(), Click::Tile(0)).is_empty());
assert!(click(&ClientState::default(), Click::ViewRow(0)).is_empty());
assert!(click(&ClientState::default(), Click::FormulaRow(0)).is_empty());
}
fn with_panels() -> ClientState {
let mut state = ClientState::default();
state.fold(&Command::Project(Projection::ViewList(vec![
crate::ViewSummary {
name: "Default".into(),
axes: vec![],
},
crate::ViewSummary {
name: "Plan".into(),
axes: vec![],
},
])));
state.fold(&Command::Project(Projection::Formulas(vec![
"Profit = Revenue - Cost".into(),
"Margin = Profit / Revenue".into(),
])));
state
}
/// improvise-iioi: the same three intents the category panel gets, with
/// `switch-view-at-cursor` as the activation.
#[test]
fn a_view_row_click_focuses_moves_and_switches_to_the_row_clicked() {
let state = with_panels();
assert_eq!(
click(&state, Click::ViewRow(1)),
vec![
Command::SetPanelOpen {
panel: Panel::View,
open: true
},
Command::ChangeMode(AppMode::ViewPanel),
Command::SetPanelCursor {
panel: Panel::View,
cursor: 1
},
// `Plan`, the row clicked — not `Default`, where the cursor is.
Command::SwitchView("Plan".into()),
Command::ChangeMode(AppMode::Normal),
]
);
}
/// The formula panel binds no `Enter`, so the rule yields navigation and
/// nothing else. A mode change here would be an invented activation.
#[test]
fn a_formula_row_click_navigates_and_activates_nothing() {
let state = with_panels();
let commands = click(&state, Click::FormulaRow(1));
assert_eq!(
commands,
vec![
Command::SetPanelOpen {
panel: Panel::Formula,
open: true
},
Command::ChangeMode(AppMode::FormulaPanel),
Command::SetPanelCursor {
panel: Panel::Formula,
cursor: 1
},
]
);
assert!(
!commands
.iter()
.any(|c| matches!(c, Command::ChangeMode(AppMode::FormulaEdit { .. }))),
"there is no Enter binding to mirror; opening an editor would be invented"
);
}
/// The pointer and the keyboard are one cursor: a click resolves to the
+4 -3
View File
@@ -2,9 +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.
// Reuse is OPT-IN because adopting another checkout's server tests THAT tree,
// and the symptom is a result contradicting the file you just read.
const PORT = process.env.IMPROVISE_HARNESS_PORT ?? "8099";
const REUSE = process.env.IMPROVISE_HARNESS_REUSE === "1";
export default defineConfig({
testDir: "./tests",
@@ -38,7 +39,7 @@ export default defineConfig({
// 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:${PORT}/app/index.html`,
reuseExistingServer: !process.env.CI,
reuseExistingServer: REUSE,
timeout: 30_000,
},
});
+48
View File
@@ -138,3 +138,51 @@ test.describe("clicking a grid cell", () => {
await expect(cells.nth(3)).toHaveClass(/selected/);
});
});
test.describe("clicking the other two panels", () => {
// improvise-iioi. The category panel got a listener and its two siblings did
// not, so a user who had just learned rows are clickable clicked a view row
// and read the silence as broken — which IS the report behind improvise-7ybz.
test("a click on a view row focuses, moves the cursor and switches", async ({
page,
}) => {
await openApp(page);
await page.keyboard.press("Shift+V");
await expect(page.locator("#views")).toBeVisible();
await expect(page.locator("#view-name")).toHaveText("Plan");
await expect(page.locator("#views li.active")).toContainText("Plan");
// Leave the panel open but unfocused, so the focus third is observable.
await page.keyboard.press("Escape");
await expect(page.locator("#mode")).toHaveText("NORMAL");
await page.locator("#views li").filter({ hasText: "Default" }).click();
// Switching ends in Normal — `switch-view-at-cursor` says so — but the
// cursor and the active marker must both have moved to the clicked row.
await expect(page.locator("#view-name")).toHaveText("Default", {
timeout: 10_000,
});
await expect(page.locator("#views li.active")).toContainText("Default");
await expect(page.locator("#views li.selected")).toContainText("Default");
});
// The formula panel binds NO Enter, so the rule yields navigation and
// nothing else. That is the rule applying, not the fix stopping half way.
test("a click on a formula row moves the cursor and does not edit", async ({
page,
}) => {
await openApp(page);
await page.keyboard.press("Shift+F");
await expect(page.locator("#formulas")).toBeVisible();
await page.keyboard.press("Escape");
await expect(page.locator("#mode")).toHaveText("NORMAL");
await page.locator("#formulas li").first().click();
// Focused and selected — and still FORMULAS, because there is nothing to
// activate. A mode change here would mean an activation was invented.
await expect(page.locator("#mode")).toHaveText("FORMULAS");
await expect(page.locator("#formulas li.selected")).toHaveCount(1);
await expect(page.locator("#grid input")).toHaveCount(0);
});
});
+10 -4
View File
@@ -194,8 +194,10 @@ function repaint() {
el.categories.hidden = !model.category_panel_open;
el.categories.replaceChildren(categoryList(model));
paintTiles(model);
paintPanel(el.views, model.views, viewRow);
paintPanel(el.formulas, model.formulas, formulaRow);
paintPanel(el.views, model.views, viewRow, (i) => client.click_view(i));
paintPanel(el.formulas, model.formulas, formulaRow, (i) =>
client.click_formula(i),
);
// What the frame costs, so the NEXT viewport announcement is honest.
// Inline group ROWS are deliberately NOT counted — context/code/clients.md.
@@ -393,10 +395,14 @@ function edit(td, row, col, seed) {
/** A side panel. An empty row list is what says it is closed — the page asks
* no question about the mode, exactly as `paintHelp` does not. */
function paintPanel(host, rows, build) {
function paintPanel(host, rows, build, clicked) {
host.hidden = rows.length === 0;
const list = document.createElement("ul");
for (const row of rows) list.appendChild(build(row));
rows.forEach((row, index) => {
const li = build(row);
li.addEventListener("click", () => send(clicked(index)));
list.appendChild(li);
});
host.replaceChildren(list);
}