Pin Tab's rightmost-column clamp and bound the tile-bar walks
improvise-qnry: a lockstep case compares Tab at the rightmost column with the host's real CommitAndAdvance, and the tile lockstep case now presses past the last tile. improvise-ybum: browser_round_trip's tile and column walks are bounded and fail on a stuck press instead of hanging. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
@@ -3819,6 +3819,23 @@ ticket's transcript spells the route as `T r l c Esc`, which depends on
|
||||
`state().tiles` and walks to it, so the test pins *the route exists*, not a
|
||||
particular ordering the producer is free to change.
|
||||
|
||||
**The walks are bounded (improvise-ybum),** found in the adversarial review
|
||||
of PR #256. `tile_to` and the column walk were `while` loops that pressed `l`
|
||||
or `h` until an index was reached. A resolver that stopped the cursor short
|
||||
never ended the loop: under the left-clamp fault a reviewer killed the binary
|
||||
after 17 minutes, and in CI a hang costs the job timeout and names no test.
|
||||
Both loops now go through `walk_to`. It presses at most `limit` times (the tile
|
||||
count plus one, or the column count plus one), and it fails as soon as a press
|
||||
leaves the index where it was, naming the stuck index and the target.
|
||||
|
||||
- **Rival `left_clamp_one`** (`clamp(1, ..)` in `tile_cursor`): before the
|
||||
change, the binary was still running at a 90-second `timeout` and was killed
|
||||
(exit 124). After it, the test fails in 0.04s with "the tile cursor is stuck
|
||||
at 1: `h` did not move it toward 0".
|
||||
- **Rival `column_cap_one`** (`clamp(0, col_max.min(1))` in the grid move):
|
||||
fails in 0.04s with "the column cursor is stuck at 1: `l` did not move it
|
||||
toward 2", so the column walk is bounded too, not only the tile walk.
|
||||
|
||||
### opening_an_editor_is_declined_off_the_grid_and_not_off_the_mode
|
||||
|
||||
improvise-bd9o, and it exists because **the route test could not state the
|
||||
@@ -5150,3 +5167,42 @@ when the index does not move (gotcha 47).
|
||||
`SetTileCatIdx(3)` over three tiles.
|
||||
- The control matters: without the unclamped move first, a resolver that never
|
||||
moves at all would also pass the two clamped presses.
|
||||
|
||||
### tab_at_the_rightmost_column_stays_there_on_both_ends
|
||||
|
||||
improvise-qnry, found in the adversarial review of PR #256. Dropping the clamp
|
||||
from `advanced`'s `Advance::Right` arm, so that it reads `Some((row, col + 1))`,
|
||||
passed the whole workspace (1942 tests). `tab_in_an_editor_commits_and_lands_one_column_right`
|
||||
sits on a centred cursor on purpose, so it cannot see the edge. Without the
|
||||
clamp, Tab at the rightmost column of a pivot grid sends the selection one
|
||||
column past the grid, and the editor opens on no cell.
|
||||
|
||||
This is a lockstep case: the client is compared with the host's real
|
||||
`CommitAndAdvance`, not with a hand-written expectation. The fixture is
|
||||
`precise_host`, two rows by two columns, with the cursor at (1, 1). That is the
|
||||
rightmost column at a nonzero row, so "the last column" is not "the first
|
||||
cell". The test presses `i` then `Tab` and compares the cursor and the mode on
|
||||
both ends after each key. Its preconditions assert that the grid is not in
|
||||
records mode, where this corner declines instead, and that the editor is open
|
||||
again at the end.
|
||||
|
||||
- **Rival `right_unclamped`** (the ticket's fault): fails only this test across
|
||||
the workspace (1942 passed, 1 failed). The client lands on `(1, 2)` with
|
||||
`col_offset: 1`; the host stays on `(1, 1)`.
|
||||
- improvise-3tt4's acceptance test (Tab at the rightmost column, comparing
|
||||
buffers) would probably also see this fault, but it lives on PR #227's branch,
|
||||
which is not in this base. This test compares the selection and does not
|
||||
depend on #227.
|
||||
|
||||
### a_locally_resolved_tile_cursor_agrees_with_the_host
|
||||
|
||||
Extended for improvise-qnry. The test used to press `l` three times over
|
||||
`tall_host`'s four tiles, starting at 0, so it never reached the upper clamp:
|
||||
any rival that differed only at the last tile passed it. It now presses
|
||||
`count + 1` times and asserts it ends on the last tile, so the final press is
|
||||
checked against the host's real `MoveTileCursor` clamp.
|
||||
|
||||
- **Rival `wraparound`** (`.rem_euclid(count)` in place of the clamp): fails
|
||||
this test at press 3 (client 0, host 3), and also the unit test
|
||||
`resolve::tests::the_tile_cursor_stops_on_the_last_tile`. Nothing else in
|
||||
the workspace fails (1941 passed, 2 failed).
|
||||
|
||||
@@ -431,15 +431,44 @@ fn tile_to(
|
||||
.iter()
|
||||
.position(|t| t.category == category)
|
||||
.unwrap_or_else(|| panic!("the tile bar must carry {category}"));
|
||||
while client.state().view.tile_cat_idx < at {
|
||||
press(host, id, client, "l");
|
||||
}
|
||||
while client.state().view.tile_cat_idx > at {
|
||||
press(host, id, client, "h");
|
||||
}
|
||||
let limit = client.state().tiles.tiles.len() + 1;
|
||||
walk_to(host, id, client, "the tile cursor", at, limit, |c| {
|
||||
c.state().view.tile_cat_idx
|
||||
});
|
||||
press(host, id, client, key);
|
||||
}
|
||||
|
||||
/// Press `l` or `h` until `read` gives `target`, failing on a stuck press or
|
||||
/// after `limit` presses rather than hanging (improvise-ybum). Evidence file.
|
||||
fn walk_to(
|
||||
host: &mut Host,
|
||||
id: improvise_command::ui::project::SessionId,
|
||||
client: &mut Client,
|
||||
what: &str,
|
||||
target: usize,
|
||||
limit: usize,
|
||||
read: impl Fn(&Client) -> usize,
|
||||
) {
|
||||
for _ in 0..limit {
|
||||
let now = read(client);
|
||||
if now == target {
|
||||
return;
|
||||
}
|
||||
let key = if now < target { "l" } else { "h" };
|
||||
press(host, id, client, key);
|
||||
assert_ne!(
|
||||
read(client),
|
||||
now,
|
||||
"{what} is stuck at {now}: `{key}` did not move it toward {target}"
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
read(client),
|
||||
target,
|
||||
"{what} did not reach {target} in {limit} presses"
|
||||
);
|
||||
}
|
||||
|
||||
/// `toggle-records-mode` is declined and the tile bar reaches records mode
|
||||
/// anyway (improvise-bd9o). Evidence file.
|
||||
#[test]
|
||||
@@ -472,9 +501,15 @@ fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() {
|
||||
"the fixture must reach past the coordinate column"
|
||||
);
|
||||
for col in 0..cols {
|
||||
while client.state().view.cursor().selected.1 < col {
|
||||
press(&mut host, id, &mut client, "l");
|
||||
}
|
||||
walk_to(
|
||||
&mut host,
|
||||
id,
|
||||
&mut client,
|
||||
"the column cursor",
|
||||
col,
|
||||
cols + 1,
|
||||
|c| c.state().view.cursor().selected.1,
|
||||
);
|
||||
let kind = format!(
|
||||
"{:?}",
|
||||
client.state().grid.cell(0, col).and_then(|c| c.id.clone())
|
||||
|
||||
@@ -462,7 +462,8 @@ fn a_locally_resolved_tile_cursor_agrees_with_the_host() {
|
||||
"a one-tile fixture clamps immediately and proves nothing"
|
||||
);
|
||||
|
||||
for press in 0..3 {
|
||||
let count = client.tiles.tiles.len();
|
||||
for press in 0..=count {
|
||||
let event = KeyEvent::new(KeyCode::Char('l'), KeyModifiers::NONE);
|
||||
let KeyOutcome::Local(cmds) = resolve(&client, event) else {
|
||||
panic!("press {press}: the tile cursor must resolve locally");
|
||||
@@ -476,7 +477,11 @@ fn a_locally_resolved_tile_cursor_agrees_with_the_host() {
|
||||
"press {press}: client and host disagree about the tile cursor"
|
||||
);
|
||||
}
|
||||
assert!(client.view.tile_cat_idx > 0, "the fixture moved nothing");
|
||||
assert_eq!(
|
||||
client.view.tile_cat_idx,
|
||||
count - 1,
|
||||
"count + 1 presses must end clamped on the last tile (improvise-qnry)"
|
||||
);
|
||||
}
|
||||
|
||||
// The editing path (improvise-bpq2's second half).
|
||||
@@ -671,6 +676,50 @@ fn a_locally_committed_edit_leaves_both_ends_in_the_same_place() {
|
||||
);
|
||||
}
|
||||
|
||||
/// improvise-qnry: Tab at the rightmost column clamps there on both ends, as
|
||||
/// `CommitAndAdvance` does. Evidence: `context/test-evidence/clients.md`.
|
||||
#[test]
|
||||
fn tab_at_the_rightmost_column_stays_there_on_both_ends() {
|
||||
let window = (2, 2);
|
||||
let mut app = precise_host(improvise_protocol::CellValue::Number(4.0));
|
||||
let mut client = client_ready_to_edit(&app, window);
|
||||
let extent = client.grid.extent;
|
||||
assert!(
|
||||
!client.grid.records,
|
||||
"records mode declines this corner instead"
|
||||
);
|
||||
assert_eq!(
|
||||
client.view.cursor().selected,
|
||||
(1, extent.cols - 1),
|
||||
"the cursor must start on the rightmost column at a nonzero row"
|
||||
);
|
||||
|
||||
for key in ["i", "Tab"] {
|
||||
let event = BrowserKey::new(key).to_key_event().unwrap();
|
||||
let KeyOutcome::Local(cmds) = resolve(&client, event) else {
|
||||
panic!("{key}: the client must answer this key");
|
||||
};
|
||||
for cmd in &cmds {
|
||||
client.fold(cmd);
|
||||
}
|
||||
reduce_key(&mut app, event);
|
||||
assert_eq!(
|
||||
client.view.cursor(),
|
||||
app.view_state.cursor(),
|
||||
"{key}: client and host disagree about the cursor"
|
||||
);
|
||||
assert_eq!(
|
||||
format!("{:?}", client.view.mode),
|
||||
format!("{:?}", app.view_state.mode),
|
||||
"{key}: client and host disagree about the mode"
|
||||
);
|
||||
}
|
||||
assert!(
|
||||
app.view_state.mode.is_editing(),
|
||||
"Tab must have reopened the editor, or the walk stopped short"
|
||||
);
|
||||
}
|
||||
|
||||
// The second tranche (improvise-e24v).
|
||||
|
||||
/// A document with two categories, two views and a value, so every widget the
|
||||
|
||||
Reference in New Issue
Block a user