fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(web): a click is all of its intents, never mutates, and the panel opens

Browse Source 
Four user rulings and three review findings, all in the pointer's one
resolver.

improvise-hkr8 — a click is all of its intents or none of them. Each
`click_*` now returns `Option<Vec<Command>>` and `click` does the single
`unwrap_or_default`. The `None` that matters is `BasisCache::claim`
answering that no basis has been published: the click used to keep its
navigation prefix, drop its activation and report success, while the
keyboard handled the identical `None` correctly as `KeyOutcome::Upstream`.

USER RULING — a tile click SELECTS; a second click on the already-selected
tile CYCLES. Every click in the UI is now non-mutating by default. The
argument that earns the two keystrokes is not tidiness: a cycle is
`LayoutGeometry`, so it bumps `cursor_epoch` and resets EVERY connected
client's cursor (gotcha 24) — the easiest input to trigger by accident was
wired to the one class of change that reaches other people. The divergence
from `Enter`, which still cycles in one keystroke, is chosen and recorded.
"Already selected" is mode AND index, because `tile_cat_idx` is 0 before
anything selects anything and tile 0 would otherwise cycle on first click.

USER RULING — the category panel is open at launch, as the shared
`ViewState::default()` rather than a browser special case. A browser-only
default is "the page overrides the flag", which is the same defect as the
"the page ignores the flag" bug improvise-wsaw fixed. This changes the TUI
too: `src/draw.rs` draws the panel off the same field.

Three review findings, all "a guard that would not catch the drift it is
named for":

- The formula-row `assert!` could never fail — the `assert_eq!` above it
  pins the same vector and aborts first. Replaced by an assertion aimed at
  the keymap itself, which is where the fact lives; the reviewer had shown
  that adding `Enter` to FormulaPanel left 1381 tests green while the click
  and the keyboard disagreed.
- `click_view_row` re-spelled `SwitchView + ChangeMode` inline instead of
  sharing a helper, so a change forced into the keyboard arm by lockstep
  left the click behind silently. Both now call `switch_view_at`.
- `context/test-evidence/clients.md` documented the behaviour PR #59 ships
  as one of the injected faults that disproved it, and had no entry for
  #59's new test. Both fixed; the checker's one-directional blind spot that
  hid it is filed as improvise-lyyc.

Every change is proved by injecting the fault: six injections, six distinct
failure signatures, none passing for another's reason.

Gates: 1389 workspace tests (was 1381), clippy -D warnings, fmt, harness
fmt, all eight wasm crates, 27/27 Chromium harness, 189 cited ticket IDs
resolve, comment runs and evidence headings clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-20 13:45:31 -07:00
parent d7386b4308
commit 50e4a37214
5 changed files with 572 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/clients.md
+93 -6
View File
@@ -552,12 +552,16 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it.
`text`**: the drawn text has been through `format_f64`, so a stored 1200 reads
`"1,200"` under a comma-grouping format and committing it would store the
*string* `"1,200"`. `tests/lockstep.rs` found exactly that.
- **Drill does not go upstream.** Every `Upstream` is a black hole, and a key
that visibly does nothing is the worst of the three answers. Drill is
session-local residue and reaching it needs a `CellKey` this client cannot
build, so the honest answer is to say so: a `SetStatus`, which is a view-half
command — `Sessions::apply` folds it into the asker's own mirror and
broadcasts nothing, so it reaches this user and no one else.
- **Drill does not go upstream — by *either* door.** Every `Upstream` is a black
hole, and a key that visibly does nothing is the worst of the three answers.
Drill is session-local residue and reaching it needs a `CellKey` this client
cannot build, so the honest answer is to say so: a `SetStatus`, which is a
view-half command — `Sessions::apply` folds it into the asker's own mirror and
broadcasts nothing, so it reaches this user and no one else. This argument
arrived as `edit_or_drill`'s, i.e. about `i`/`a` on an aggregate, and for a
while it read as doctrine for `>` while `>` was still silent. Both doors now
refuse, and **they refuse with different sentences on purpose**: "this key is
not built yet" and "this cell is not yours to write" are different facts.
- **The commit's write is not spelled again here.** It is
`Client::typed_cell`'s, the same one click-to-edit uses, so a browser keyboard
commit and a browser mouse commit cannot disagree about what `""` or `"42"`
@@ -587,6 +591,89 @@ applied it (`"A"`, not shift+`"a"`), and the shared keymap's `lookup_local`
retries character keys without modifiers for that very reason, so keeping the
flag would only make an exact binding miss.
### The pointer: `Click`, and the four rules it obeys
A click is resolved here for the same reason a keypress is — a front end paints
and wires, it does not decide (design-principles §5). `resolve::click` takes a
`Click` naming a widget and an index, and returns the `Command`s. `web/app.js`
attaches listeners and knows nothing else; there is one entry point per widget
on `Client`, which was the user's ruling on improvise-7ybz.
**Rule 1 — a click navigates to what was clicked, and activates it where
activation is something the client can express.** Every arm composes the same
shape: `focus` the panel, `SetPanelCursor` to the row, then whatever `Enter`
does on that row. The activation is resolved **against the clicked index, never
against the cursor**, because the `SetPanelCursor` ahead of it has not been
folded when the resolver runs — the trap that reads as "the cursor lands on
`Plan` and the document switches to `Default`", silent and wrong in the half a
user does not watch.
The second clause of rule 1 is what makes the formula panel look inconsistent
and is in fact the rule holding. `ModeKey::ViewPanel` binds `Enter` to
`switch-view-at-cursor`; `ModeKey::FormulaPanel` binds `a`/`n`/`o` and
`d`/`Delete` and **no `Enter` at all**. So a view-row click switches and a
formula-row click only navigates. Opening the editor on a formula click would
be an activation *invented for the pointer that the keyboard does not have* —
a second derivation of what a formula row means, which is the disease §5 names.
**The guard for this must be aimed at the keymap, not at the click**: asserting
that the click emits no `FormulaEdit` only restates the author's reading, and a
reviewer demonstrated the gap by adding `Enter -> enter-mode formula-edit` to
`FormulaPanel` and watching the whole workspace stay green while the click and
the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does`
asserts the binding's absence and names `click_formula_row` as what must change
if it ever appears.
**Rule 2 — a click is all of its intents or none of them** (improvise-hkr8).
Each `click_*` returns `Option<Vec<Command>>` and `click` does the single
`unwrap_or_default`. The case that matters is an activation the client cannot
compose: `filter_to_item_at` goes through `model`, which goes through
`BasisCache::claim`, which returns `None` until the server has published a basis
(gotcha 43). Before the fix the `None` was swallowed per-intent, so the click
kept its navigation prefix, dropped its activation, and reported success — while
the *keyboard* handled the identical `None` correctly by answering
`KeyOutcome::Upstream`. Note that `None` at the `click` level means something
different and benign: the whole click composed nothing, which is the invariant
rather than a breach of it.
**Rule 3 — a click never mutates the document on its own.** The user's ruling on
the tile bar, and it is deliberately stated over *every* click rather than over
tiles: the first click on a tile selects it and enters `TileSelect`, and only a
second click on the **already-selected** tile cycles its axis. The cost accepted
is two clicks for the common action and a **deliberate divergence from the
keyboard**, where `Enter` cycles directly — record it as chosen, not as an
inconsistency to tidy, or someone will read the binding and "fix" the pointer to
match. What it buys is worth more than the keystroke: a tile cycle is a
`LayoutGeometry` change, which bumps `cursor_epoch` and therefore resets **every
connected client's cursor** (gotcha 24), so a single stray click used to cost
every other viewer their position. "Already selected" is `mode == TileSelect`
**and** `tile_cat_idx == index`; the index alone is not enough, because
`tile_cat_idx` is 0 before anything has selected anything and tile 0 would then
cycle on its first click.
**Rule 4 — the shared helper is the idiom, and breaking it is how the two arms
drift.** `click_category_row` calls `filter_to_item_at`, `click_tile` calls
`tile_axis_at`, `click_view_row` calls `switch_view_at` — each is `foo_at(index)`
with the keyboard's `foo_at_cursor` calling `foo_at(cursor)`. `click_view_row`
originally re-spelled `SwitchView + ChangeMode(Normal)` inline, so a host-side
change to `SwitchViewAtCursor` was forced into the keyboard arm by
`tests/lockstep.rs` and left the click behind **silently**. A helper that returns
only the *name* rather than the whole activation is what invites that.
### The category panel opens at launch
`ViewState::default()` sets `category_panel_open: true` — the user's ruling, and
it is the **shared** default rather than a browser one. A browser-only default
would be the same defect as the bug improvise-wsaw fixed, wearing different
clothes: "the page ignores the flag" and "the page overrides the flag" are one
mistake. The consequence is that the **TUI** opens with the panel too, since
`src/draw.rs` draws it off the same field.
Two tests, because either alone passes against the bug wsaw fixed: the panel is
visible with no keypress, *and* `C` still hides it. Injecting "the page ignores
the flag" fails the second; injecting "the default is false" fails the first.
A test that only asserts the sidebar is visible cannot tell the flag being
honoured from the flag being ignored.
---
## `improvise-wasm-client` — the crate, the fold, the render model
context/test-evidence/clients.md
+169 -17
View File
@@ -1101,14 +1101,22 @@ addressed to precisely one. That is the same argument that removed
`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
**The two entry points used to fail differently, and that asymmetry was 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`) was a name `resolve::step` did not handle, so it became
`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.
`>` in the browser did nothing whatsoever: no status, no mode change, nothing.
That is why this read as "never confirmed working" rather than as a refusal.
**That silence is now closed, and the closing is what this file ships.** `step`
answers `"drill-into-cell"` with a `SetStatus` saying drill is not available in
the browser yet — an interim the wording of which is load-bearing, since it must
be true both today and on the day improvise-gd28 lands. The wire vocabulary is
still absent; only the *silence* was fixed. Note for gd28: when the real drill
arrives, a leftover `"drill-into-cell"` arm in `step` would **shadow** it with
nothing failing, because a name that resolves never reaches the host.
**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
@@ -1124,20 +1132,45 @@ 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
**Both injections, run** — against the tree *before* the interim, which is what
made them two faults rather than one. Replacing the aggregate arm with `None` —
silencing the honest refusal — failed **both** tests, and the second failed 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 — failed **only** the second test and left the
first green. Two faults, two different failure signatures, neither test passing
for the other's reason.
for the other's reason. That second injection is now the shipped code, which is
why the test it broke was rewritten rather than kept.
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.
### the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons
The Rust half of the interim, and the assertion that carries the whole weight.
Two keys on the same aggregate cell must both refuse and must **not** refuse
with the same sentence: `>` says drill is unavailable, `i` says drilling into an
aggregate needs the host. Collapsing them into one message would be the
tempting tidy-up and would lose the distinction between "this key does nothing
here yet" and "this cell is not one you may write".
**The injection, and why it is not the obvious one.** Replacing `>`'s sentence
with the *aggregate* sentence looks like it should fail on the first assertion —
and does not: that sentence contains both "drill" and "browser", so it clears
the substring check. What fires is the anti-aggregate assertion lower down. The
claim that the second assertion is the load-bearing one was doubted on review,
tested, and confirmed; record it here because the first assertion reads like the
discriminating one and is not.
The harness sibling (`drill.spec.js`) asserts the same distinction in a browser,
but its `not.toContainText("aggregate")` is **decoration under the only
injection that exists** — Playwright throws at the positive assertion above it
first, so that line is never reached. The Rust assertion is the guard; the
harness one is end-to-end confirmation that the string survives the wire.
### 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
@@ -1163,10 +1196,30 @@ 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.
This test asserts the *exact* command vector, which is the whole of what it can
say. It once carried a second, explanatory `assert!` that no `FormulaEdit` mode
change appears — **which could never have been the failing assertion**, because
the `assert_eq!` above it pins the same three commands and aborts first. A
reviewer demonstrated it by injecting a `ChangeMode(FormulaEdit)` and watching
the panic land on the `assert_eq!` with the explanatory message never printed.
It was a comment wearing an `assert!`, and the argument now lives in
`context/code/clients.md` where a comment run cannot hold it.
### the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does
The guard that the removed assertion was pretending to be. The asymmetry — a
view-row click activates, a formula-row click does not — is a fact about the
**keymap**, and nothing tied the click to it. The reviewer proved the gap by
adding `Enter -> enter-mode formula-edit` to `ModeKey::FormulaPanel` and running
the whole workspace: **1381 passing, nothing failed**, while `click_formula_row`
still activated nothing. The click and the keyboard then disagree about what a
formula row does, silently.
Injecting that same binding now fails this test and only this test. The failure
message names `click_formula_row` as what must change, because the person who
adds an `Enter` binding is not the person reading `resolve.rs`'s click arms. The
positive half (`ViewPanel` *does* bind `Enter`) is there so the test cannot pass
by asserting that no panel binds anything.
#### harness/tests/pointer.spec.js — the two panel tests added for improvise-iioi
@@ -1199,3 +1252,102 @@ 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.
### a_click_that_cannot_compose_its_activation_composes_nothing
improvise-hkr8. `click_category_row` ended with
`filter_to_item_at(state, index).unwrap_or_default()`, and that `None` arm is
not "nothing to do" — it is `BasisCache::claim` answering that the server has
published no basis yet (gotcha 43), so the document change cannot be composed.
The click therefore kept its **navigation prefix** (focus the panel, move the
cursor), dropped its activation, and reported success. The keyboard handles the
identical `None` correctly, by mapping `step(..) == None` to
`KeyOutcome::Upstream`.
The fixture has to do two things a careless one would not. It **expands
`Region`** so that row 1 is an `Item` row — a header row activates via
`ToggleCatExpand`, which needs no basis and so cannot show the bug — and it
asserts both preconditions (`cat_tree_entry_at` is an `Item`, `claim` is `None`)
rather than assuming them, because an empty `ClientState` makes every
state-dependent refusal look like the one under test.
Injecting the old `unwrap_or_default()` fails this test and only this test.
### the_same_click_composes_everything_once_a_basis_is_published
The positive arm of the one above, and it is what stops the fix from being
"return nothing more often". With a basis published, the same click composes all
six commands: open, focus, move the cursor, two `Command::Model`s and the
status. Without this, deleting the activation entirely would pass hkr8's test.
### a_first_tile_click_selects_without_touching_the_document
The user's ruling: a click selects, a second click cycles. The fixture publishes
a basis **deliberately**, so the absence of a `Command::Model` is a decision
rather than a client that could not compose one — pointed at an unpublished
basis this test would pass for hkr8's reason instead of its own.
Both halves are asserted, because "the axis did not change" passes against a
page where nothing happened at all: the positive half requires `SetTileCatIdx`
plus `ChangeMode(TileSelect)`, the negative half requires no `Command::Model`.
**Four injections, four distinct signatures.** "First click also cycles" fails
this test, `the_first_click_on_tile_zero_selects_it_like_any_other` and
`clicking_a_different_tile_selects_it_rather_than_cycling_it` — and **not**
`a_second_click_on_the_selected_tile_cycles_its_axis`. "Second click does not
cycle" fails only that last one. "`already_selected` reads the index alone"
fails only the tile-zero test. Reverting hkr8 fails only the hkr8 test. No
injection passes for another's reason.
### the_first_click_on_tile_zero_selects_it_like_any_other
The trap in the ruling's own wording. "A second click on the already-selected
tile" reads as an index comparison, and `tile_cat_idx` is **0 before anything
has selected anything** — so an index-only test would read tile 0 as already
selected and cycle it on its very first click, making the non-mutation
guarantee true of every tile but one. The condition is `mode == TileSelect`
**and** `tile_cat_idx == index`.
### clicking_a_different_tile_selects_it_rather_than_cycling_it
The other half of the same trap, from the opposite side: without it, "a click
does not mutate" would hold only for the first click of a session, since every
click after the first is a "second click" in the loose reading. Clicking A then
B must select B.
### a_fresh_session_opens_with_the_category_panel
The user's ruling that the category panel is visible at launch, implemented as
the **shared** `ViewState::default()` rather than a browser special case — a
browser-only default is "the page overrides the flag", which is the same defect
as the "the page ignores the flag" bug improvise-wsaw fixed. The consequence,
accepted deliberately, is that the TUI opens with the panel too.
The test also asserts the *other two* panels stay shut, so it cannot pass
against a default that simply opens everything.
Two existing tests had to change, and both were pinning the old decision by
**inheriting** it rather than stating it.
`panel_focus_cycles_from_the_focused_panel_not_the_first` is about skipping a
*closed* panel and took "the category panel is closed" from the default; it now
spells that precondition. `with_categories()` is used by tests about a click
*opening* the panel, so it now shuts it explicitly. Neither is the decision
being re-made — the decision is re-made only in the harness, deliberately.
#### harness/tests/pointer.spec.js — the launch state and the tile ruling
Two states, one mechanism: the sidebar is visible with no keypress, **and** `C`
still hides it. Either alone passes against the bug wsaw fixed. Proved by
injection in a real browser: setting `el.categories.hidden = false` (the page
ignoring the flag) fails on `toBeHidden`; setting the shared default back to
`false` fails on `toBeVisible`. Different signatures, different halves.
Note the first `C` now **focuses** rather than opens, since the panel is already
open — `toggle-panel-and-focus` closes only when the panel is both open and
focused — so the test presses it twice and the hide is the second press.
The tile tests were rewritten rather than adjusted, because the previous one
asserted the behaviour the ruling reverses; its failure against the new code was
itself the injection. The first-click test waits out a timeout before asserting
the axis did not move, since a cycle would arrive asynchronously and an
instantaneous assertion would pass against a slow one.
crates/improvise-protocol/src/lib.rs
+1 -1
View File
@@ -372,7 +372,7 @@ impl Default for ViewState {
search_query: String::new(),
search_mode: false,
formula_panel_open: false,
category_panel_open: false,
category_panel_open: true,
view_panel_open: false,
cat_panel_cursor: 0,
view_panel_cursor: 0,
crates/improvise-protocol/src/resolve.rs
+253 -48
View File
@@ -82,57 +82,52 @@ pub enum Click {
Cell { row: usize, col: usize },
}
/// One rule for all three widgets: **go there, then do what `Enter` does
/// there.** Why that rather than three answers: `context/code/clients.md`.
/// One rule for all the widgets: **go there, then do what `Enter` does
/// there.** Why that rather than one answer each: `context/code/clients.md`.
pub fn click(state: &ClientState, target: Click) -> Vec<Command> {
match target {
// `None` here means the whole click composed nothing, which is the
// invariant rather than a breach of it (improvise-hkr8) — clients.md.
let resolved = match target {
Click::CategoryRow(index) => click_category_row(state, index),
Click::ViewRow(index) => click_view_row(state, index),
Click::FormulaRow(index) => click_formula_row(state, index),
Click::Tile(index) => click_tile(state, index),
Click::Cell { row, col } => click_cell(state, row, col),
}
};
resolved.unwrap_or_default()
}
/// Focus the panel, move its cursor, then switch — what `V` then `Enter` do,
/// in one gesture. `switch-view-at-cursor` is the activation.
fn click_view_row(state: &ClientState, index: usize) -> Vec<Command> {
let Some(summary) = state.views.views.get(index) else {
return Vec::new();
};
fn click_view_row(state: &ClientState, index: usize) -> Option<Vec<Command>> {
let mut commands = focus(state, Panel::View);
commands.push(Command::SetPanelCursor {
panel: Panel::View,
cursor: index,
});
// Off `index`, never the cursor: `SetPanelCursor` is not folded yet.
commands.push(Command::SwitchView(summary.name.clone()));
commands.push(Command::ChangeMode(AppMode::Normal));
commands
commands.extend(switch_view_at(state, index)?);
Some(commands)
}
/// The formula panel binds **no** `Enter`, so the rule yields navigation and
/// nothing else. That is the rule applying, not an omission — clients.md.
fn click_formula_row(state: &ClientState, index: usize) -> Vec<Command> {
if state.formulas.formulas.get(index).is_none() {
return Vec::new();
}
fn click_formula_row(state: &ClientState, index: usize) -> Option<Vec<Command>> {
state.formulas.formulas.get(index)?;
let mut commands = focus(state, Panel::Formula);
commands.push(Command::SetPanelCursor {
panel: Panel::Formula,
cursor: index,
});
commands
Some(commands)
}
/// Focus the panel, move its cursor, then activate — the three things the
/// keyboard needs three keystrokes for, and `filter-to-item` is the activation.
fn click_category_row(state: &ClientState, index: usize) -> Vec<Command> {
fn click_category_row(state: &ClientState, index: usize) -> Option<Vec<Command>> {
// A click past the last row is a click on the panel's empty space, which
// is not a row and must not move the cursor onto one.
if cat_tree_entry_at(state, index).is_none() {
return Vec::new();
}
cat_tree_entry_at(state, index)?;
let mut commands = focus(state, Panel::Category);
commands.push(Command::SetPanelCursor {
panel: Panel::Category,
@@ -140,35 +135,41 @@ fn click_category_row(state: &ClientState, index: usize) -> Vec<Command> {
});
// Against `index`, never the cursor: the `SetPanelCursor` above is not
// folded yet, so the cursor still reads the old row. Evidence file.
commands.extend(filter_to_item_at(state, index).unwrap_or_default());
commands
commands.extend(filter_to_item_at(state, index)?);
Some(commands)
}
/// Select the tile and cycle its axis — `TileSelect` plus what `Enter` does
/// in it. A tile the bar does not have is not a click.
fn click_tile(state: &ClientState, index: usize) -> Vec<Command> {
if state.tiles.tiles.get(index).is_none() {
return Vec::new();
/// Select the tile; a **second** click on the already-selected one cycles its
/// axis. Why a click never mutates on its own: `context/code/clients.md`.
fn click_tile(state: &ClientState, index: usize) -> Option<Vec<Command>> {
state.tiles.tiles.get(index)?;
// Mode *and* index, because `tile_cat_idx` is 0 before anything selects
// anything — index alone would cycle tile 0 on its first click.
let already_selected =
state.view.mode == AppMode::TileSelect && state.view.tile_cat_idx == index;
match already_selected {
true => tile_axis_at(state, index, None),
false => {
let mut commands = vec![Command::SetTileCatIdx(index)];
if state.view.mode != AppMode::TileSelect {
commands.push(Command::ChangeMode(AppMode::TileSelect));
}
Some(commands)
}
}
let mut commands = vec![Command::SetTileCatIdx(index)];
if state.view.mode != AppMode::TileSelect {
commands.push(Command::ChangeMode(AppMode::TileSelect));
}
commands.extend(tile_axis_at(state, index, None).unwrap_or_default());
commands
}
/// Move the selection to a clicked cell. Activation is the page's inline
/// editor, which is the pointer's `i` — `context/code/clients.md`.
fn click_cell(state: &ClientState, row: usize, col: usize) -> Vec<Command> {
fn click_cell(state: &ClientState, row: usize, col: usize) -> Option<Vec<Command>> {
let cursor = state.view.cursor();
let GridExtent { rows, cols } = state.grid.extent;
// The page paints a window; `SetSelected` is absolute (gotcha 42).
let (absolute_row, absolute_col) = (row + cursor.row_offset, col + cursor.col_offset);
if absolute_row >= rows || absolute_col >= cols {
return Vec::new();
return None;
}
scrolled_to(state, absolute_row, absolute_col)
Some(scrolled_to(state, absolute_row, absolute_col))
}
/// Open a panel and focus it, emitting only what is not already true — so a
@@ -310,13 +311,9 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op
"delete-category-at-cursor" => delete_at_cat_cursor(state),
// ── the view panel ───────────────────────────────────────────
"switch-view-at-cursor" => Some(match view_at_cursor(state) {
Some(name) => vec![
Command::SwitchView(name),
Command::ChangeMode(AppMode::Normal),
],
None => Vec::new(),
}),
"switch-view-at-cursor" => {
Some(switch_view_at(state, state.view.view_panel_cursor).unwrap_or_default())
}
"create-and-switch-view" => {
let name = format!("View {}", state.views.views.len() + 1);
Some(vec![
@@ -871,6 +868,16 @@ fn view_at_cursor(state: &ClientState) -> Option<String> {
.map(|summary| summary.name.clone())
}
/// `SwitchViewAtCursor::execute`'s whole answer, at an arbitrary index. The
/// keyboard passes its cursor and a click passes the row — clients.md.
fn switch_view_at(state: &ClientState, index: usize) -> Option<Vec<Command>> {
let summary = state.views.views.get(index)?;
Some(vec![
Command::SwitchView(summary.name.clone()),
Command::ChangeMode(AppMode::Normal),
])
}
/// `DeleteViewAtCursor::execute`, including the cursor step back that only
/// happens away from the top of the list.
fn delete_view_at_cursor(state: &ClientState) -> Option<Vec<Command>> {
@@ -1042,10 +1049,28 @@ mod tests {
}
}
/// The user's ruling: the category panel is open at launch. It is the
/// *shared* default, so both front ends open with it — clients.md.
#[test]
fn a_fresh_session_opens_with_the_category_panel() {
let fresh = ViewState::default();
assert!(
fresh.category_panel_open,
"a client that has pressed nothing can already see its categories"
);
assert!(
!fresh.formula_panel_open && !fresh.view_panel_open,
"and only that one — the ruling names the category panel alone"
);
}
// ── the pointer (improvise-7ybz) ────────────────────────────────
fn with_categories() -> ClientState {
let mut state = ClientState::default();
// Explicitly shut: these tests are about a click *opening* the panel,
// which the launch default would otherwise answer for them.
state.view.category_panel_open = false;
state.fold(&Command::Project(Projection::CategoryTree(
crate::CategoryTreeUpdate::Categories(vec![
crate::CategoryRow {
@@ -1122,6 +1147,164 @@ mod tests {
assert!(click(&ClientState::default(), Click::FormulaRow(0)).is_empty());
}
/// Two tiles and a **published basis**, so a missing `Command::Model` is
/// a decision rather than a client that could not compose one.
fn with_tiles() -> ClientState {
let mut state = ClientState::default();
state.bootstrap(Snapshot {
view: ViewState::default(),
seed: vec![Projection::Basis(published())],
});
state.tiles.tiles = vec![
crate::TileEntry {
category: "Region".into(),
axis: Axis::Row,
},
crate::TileEntry {
category: "Product".into(),
axis: Axis::Column,
},
];
state
}
/// The user's ruling: a click **selects**, it never mutates. The basis is
/// published here, so nothing stops the cycle but the rule itself.
#[test]
fn a_first_tile_click_selects_without_touching_the_document() {
let state = with_tiles();
let commands = click(&state, Click::Tile(1));
assert_eq!(
commands,
vec![
Command::SetTileCatIdx(1),
Command::ChangeMode(AppMode::TileSelect),
],
"select and enter TileSelect, and stop there"
);
assert!(
!commands.iter().any(|c| matches!(c, Command::Model { .. })),
"a click must not change the document; the axis cycle needs a second one"
);
}
/// `tile_cat_idx` is 0 before anything has selected anything, so the
/// already-selected test needs the **mode** as well as the index.
#[test]
fn the_first_click_on_tile_zero_selects_it_like_any_other() {
let state = with_tiles();
assert_eq!(state.view.tile_cat_idx, 0, "the default index is the trap");
let commands = click(&state, Click::Tile(0));
assert_eq!(
commands,
vec![
Command::SetTileCatIdx(0),
Command::ChangeMode(AppMode::TileSelect),
]
);
assert!(
!commands.iter().any(|c| matches!(c, Command::Model { .. })),
"index alone would read tile 0 as already selected and cycle it"
);
}
/// The second click is on the **already-selected** tile, not merely the
/// second click of the session — otherwise the guarantee holds once.
#[test]
fn a_second_click_on_the_selected_tile_cycles_its_axis() {
let mut state = with_tiles();
state.view.mode = AppMode::TileSelect;
state.view.tile_cat_idx = 1;
assert_eq!(
click(&state, Click::Tile(1)),
vec![
Command::Model {
basis: state.basis.claim([]).unwrap(),
cmd: ModelCommand::CycleAxis("Product".into()),
},
// `Column.next()` is `Page` — the *new* axis, as `Enter` says.
Command::SetStatus("Product → Page".into()),
]
);
}
/// Clicking A then B selects B. Cycling B would make "a click does not
/// mutate" true only of the first click in a session.
#[test]
fn clicking_a_different_tile_selects_it_rather_than_cycling_it() {
let mut state = with_tiles();
state.view.mode = AppMode::TileSelect;
state.view.tile_cat_idx = 0;
let commands = click(&state, Click::Tile(1));
assert_eq!(
commands,
vec![Command::SetTileCatIdx(1)],
"already in TileSelect, so only the index moves"
);
assert!(!commands.iter().any(|c| matches!(c, Command::Model { .. })));
}
/// improvise-hkr8: a click is all of its intents or none of them. With no
/// published basis the activation cannot be composed, so nothing is.
#[test]
fn a_click_that_cannot_compose_its_activation_composes_nothing() {
let mut state = with_categories();
state.view.expanded_cats.insert("Region".into());
assert!(
matches!(
cat_tree_entry_at(&state, 1),
Some(CatTreeEntry::Item { .. })
),
"row 1 is an item row, whose activation is a document change"
);
assert!(
state.basis.claim([]).is_none(),
"and this client has been published no basis"
);
assert!(
click(&state, Click::CategoryRow(1)).is_empty(),
"the focus and the cursor move must not survive the activation's failure"
);
}
/// The same click, once the basis has arrived: all four intents.
#[test]
fn the_same_click_composes_everything_once_a_basis_is_published() {
let mut state = with_categories();
state.view.expanded_cats.insert("Region".into());
state.fold(&Command::Project(Projection::Basis(published())));
let basis = state.basis.claim([]).unwrap();
assert_eq!(
click(&state, Click::CategoryRow(1)),
vec![
Command::SetPanelOpen {
panel: Panel::Category,
open: true
},
Command::ChangeMode(AppMode::CategoryPanel),
Command::SetPanelCursor {
panel: Panel::Category,
cursor: 1
},
Command::Model {
basis: basis.clone(),
cmd: ModelCommand::SetAxis {
category: "Region".into(),
axis: Axis::Page
},
},
Command::Model {
basis,
cmd: ModelCommand::SetPageSelection {
category: "Region".into(),
item: "North".into()
},
},
Command::SetStatus("Filter: Region = North".into()),
]
);
}
fn with_panels() -> ClientState {
let mut state = ClientState::default();
state.fold(&Command::Project(Projection::ViewList(vec![
@@ -1185,11 +1368,26 @@ mod tests {
},
]
);
}
/// The asymmetry is a fact about the keymap, so it is asserted against
/// the keymap — not against a reading of it. See `context/code/clients.md`.
#[test]
fn the_formula_panel_has_no_enter_to_mirror_and_the_view_panel_does() {
let keymaps = Keymaps::shared();
assert!(
!commands
.iter()
.any(|c| matches!(c, Command::ChangeMode(AppMode::FormulaEdit { .. }))),
"there is no Enter binding to mirror; opening an editor would be invented"
keymaps
.lookup(&ModeKey::FormulaPanel, KeyCode::Enter, KeyModifiers::NONE)
.is_none(),
"if FormulaPanel gains an Enter binding, click_formula_row must \
activate too — the click mirrors Enter, and this is the only \
thing tying the two together"
);
assert!(
keymaps
.lookup(&ModeKey::ViewPanel, KeyCode::Enter, KeyModifiers::NONE)
.is_some(),
"and the view panel's click activates because its Enter does"
);
}
@@ -1525,9 +1723,12 @@ mod tests {
/// to `enter-mode normal` inside a panel, so no key reaches them.
#[test]
fn panel_focus_cycles_from_the_focused_panel_not_the_first() {
// Spelled out rather than inherited: this test is about skipping a
// *closed* panel, so the default's own answer must not decide it.
let open_two = |mode: AppMode| ViewState {
mode,
formula_panel_open: true,
category_panel_open: false,
view_panel_open: true,
..ViewState::default()
};
@@ -1546,8 +1747,12 @@ mod tests {
vec![Command::ChangeMode(AppMode::FormulaPanel)],
"and from the last, it wraps"
);
let nothing_open = ViewState {
category_panel_open: false,
..ViewState::default()
};
assert!(
cycle_panel_focus(&ViewState::default()).is_empty(),
cycle_panel_focus(&nothing_open).is_empty(),
"with nothing open there is nowhere to go"
);
}
harness/tests/pointer.spec.js
+56 -11
View File
@@ -9,11 +9,12 @@ async function openApp(page) {
await page.locator("#grid").focus();
}
/// Open the category panel the way a user does, and leave the keyboard in
/// Normal — so a later click has a focus change to make.
/// Leave the panel open and the keyboard in Normal — so a later click has a
/// focus change to make. It opens at launch, so `C` here only focuses it.
async function openPanelThenLeave(page) {
await page.keyboard.press("Shift+C");
await expect(page.locator("#categories")).toBeVisible();
await page.keyboard.press("Shift+C");
await expect(page.locator("#mode")).toHaveText("CATEGORIES");
await page.keyboard.press("Escape");
await expect(page.locator("#mode")).toHaveText("NORMAL");
await expect(page.locator("#categories")).toBeVisible();
@@ -22,17 +23,23 @@ async function openPanelThenLeave(page) {
test.describe("the category sidebar obeys category_panel_open", () => {
// improvise-wsaw. The flag existed from the start and app.js never read it,
// so the sidebar was drawn always and `C` changed only the mode label.
test("the sidebar is hidden until C, and C hides it again", async ({ page }) => {
// The launch state is the user's later ruling; the toggle is wsaw's. Both
// are asserted, because either alone passes against the bug wsaw fixed.
test("the sidebar is open at launch, C focuses it, and C again hides it", async ({
page,
}) => {
await openApp(page);
await expect(page.locator("#categories")).toBeHidden();
await expect(page.locator("#categories")).toBeVisible();
await expect(page.locator("#categories li").first()).toBeVisible();
await expect(page.locator("#mode")).toHaveText("NORMAL");
// Open but not focused, so the first press focuses rather than closes.
await page.keyboard.press("Shift+C");
await expect(page.locator("#categories")).toBeVisible();
await expect(page.locator("#mode")).toHaveText("CATEGORIES");
await expect(page.locator("#categories li").first()).toBeVisible();
// The same key closes it, because `toggle-panel-and-focus` closes only
// when the panel is already both open AND focused.
// And now it closes — which is the half that proves the page reads the
// flag rather than drawing the sidebar unconditionally.
await page.keyboard.press("Shift+C");
await expect(page.locator("#categories")).toBeHidden();
await expect(page.locator("#mode")).toHaveText("NORMAL");
@@ -85,17 +92,37 @@ test.describe("clicking the category panel", () => {
});
test.describe("clicking a tile", () => {
// The tile bar is keyboard-reachable only through `T` plus arrows; a click
// is the whole interaction, so it selects AND cycles.
test("a click selects the tile and cycles its axis", async ({ page }) => {
// The user's ruling: a click SELECTS, a second click CYCLES. Every click in
// the UI is non-mutating by default; the cost is two for the common action.
test("the first click selects the tile and changes nothing", async ({ page }) => {
await openApp(page);
const product = page.locator("#tiles .tile").filter({ hasText: "Product" });
await expect(product).toHaveText("Product Row");
await product.click();
// The positive half: something did happen, so a green test is not the
// dead-page shape. `Product` is selected and the mode moved.
await expect(page.locator("#mode")).toHaveText("TILES");
await expect(product).toHaveClass(/selected/);
// And the negative half: the document did not move. Waited out rather
// than asserted instantly, since a cycle would arrive asynchronously.
await page.waitForTimeout(500);
await expect(product).toHaveText("Product Row");
await expect(
page.locator("#grid thead th").filter({ hasText: "Espresso" }),
).toHaveCount(0);
});
test("a second click on the selected tile cycles its axis", async ({ page }) => {
await openApp(page);
const product = page.locator("#tiles .tile").filter({ hasText: "Product" });
await product.click();
await expect(product).toHaveClass(/selected/);
await product.click();
// Row → Column is `Axis::next`; the page does not spell the rotation.
await expect(product).toHaveText("Product Col", { timeout: 10_000 });
// And the grid moved with it, not just the label.
@@ -103,6 +130,24 @@ test.describe("clicking a tile", () => {
page.locator("#grid thead th").filter({ hasText: "Espresso" }).first(),
).toBeVisible();
});
test("clicking a different tile selects it rather than cycling it", async ({
page,
}) => {
await openApp(page);
const product = page.locator("#tiles .tile").filter({ hasText: "Product" });
const quarter = page.locator("#tiles .tile").filter({ hasText: "Quarter" });
await product.click();
await expect(product).toHaveClass(/selected/);
await quarter.click();
// The guarantee has to hold for every click, not only a session's first.
await expect(quarter).toHaveClass(/selected/);
await page.waitForTimeout(500);
await expect(quarter).toHaveText("Quarter Pag");
await expect(product).toHaveText("Product Row");
});
});
test.describe("clicking a grid cell", () => {