diff --git a/context/code/protocol.md b/context/code/protocol.md index 7f62ad07..f967ee30 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -854,6 +854,36 @@ invariant §3(c) is really about, at **every step** of a fold that widens before its widths arrive and narrows before they shrink — not merely at the end, which is where a message-count assertion looks. +### `GridCache::records` — what kind of grid this is (improvise-e5pf) + +The host answers "is this the wide records layout" with +`GridLayout::is_records_mode()`, which reads the axes. A thin client has no +`GridLayout`, and it was answering the neighbouring question instead — +`ViewState::mode.is_records()` — at three decision points in `resolve.rs`. The +two agree only while the mode and the layout agree, and +`Command::ChangeMode(AppMode::Normal)` is precisely the message that separates +them: the host settles `Normal` against the layout the batch *ends* on +(`App::settle_normal_mode`), and `reduce_view` folds the variant it was handed. + +So the fact is **projected**, not derived: `GridCache::records`, carried by +`GridUpdate::Records`. Deriving it on the client from `row_frame.cats` +containing `_Index` would have been a second implementation of +`is_records_mode` — improvise-edp's disease — and deriving it from the mode is +the bug itself. + +Like `GridUpdate::Extent` it **frames nothing**, so `apply` does not re-fit and +its position in a batch is not load-bearing: it describes what kind of grid the +frames describe, not any position in one. + +The client's own settling has **one home**, `resolve::changed_mode`, which every +composition site that can emit `AppMode::Normal` goes through — including +`entered`, which is how a bound `enter-mode normal` reaches it. The three +decision points now ask `state.grid.records`. What that buys concretely: the +commit at a records cell **declines** rather than composing +`typed_cell_command` against a grid whose columns are +`[coords…, +cat, measures…, +]` (gotcha 1), which was a wrong write and strictly +worse than the black hole it was written to produce. + ## §3a. The frame's group boundaries and its position in the axis Two facts `AxisFrame` gained after `improvise-anr9`, both about group runs. diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 096c291b..1e8e7e81 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -2427,3 +2427,64 @@ does not show — which `claim` would then silently omit, and `Rejection::RecordUnstated` is what the server answers with. The fixtures no longer fabricate, which was the ticket; they do still assume a cursor at the origin. + +### leaving_a_panel_in_records_mode_lands_where_the_host_lands + +improvise-e5pf. `AppMode::Normal` is settled by the host against the layout the +batch ends on; the wire carries `Command::ChangeMode(AppMode::Normal)` and +`reduce_view` folds it verbatim. So a client that opened a panel from records +mode and pressed Esc landed in `Normal` while its host landed in +`RecordsNormal` — and `RecordsNormal` has its own keymap, so the divergence +compounds on the next keypress. + +The reachable path, which the ticket did not name: `Tab` in `RecordsNormal` +resolves locally to `ChangeMode(CategoryPanel)`, and `Esc` there resolves to +`ChangeMode(Normal)`. `c`/`f`/`v` are `Ignored` in `RecordsNormal` and `R` +itself is a `stranding` decline, so the panel keys a first draft of this test +used never separated anything — the test panicked on "the client must answer +this key itself" rather than on the drift. + +**Why lockstep could not see it before.** Its records coverage (`typing_at`, +`records_pair`) set `host.view_state.mode` by assignment and seeded the client +with `client.view = app.view_state.clone()`, so it started where the mode and +the layout already agree, and every divergence here requires them to disagree — +`doctrine(a_guard_seeded_on_agreement_cannot_detect_drift)`. `in_lockstep` +drives the key on both ends and then refreshes the client's grid from a **real +projection**, never from the host's slice, which is the only seeding that can +show the two apart. + +**Fault injected**: `resolve::changed_mode` reduced to +`Command::ChangeMode(mode)`. The test fails on `close: the client and the host +disagree about the mode`, `Normal` against `RecordsNormal`. + +Two existing `resolve.rs` fixtures had the same shape and had to be corrected +with the fix: `furnished` and `a_records_mode_commit_is_the_hosts` put the +client in a records *mode* over a non-records *grid*, which is a state no host +can produce. Both now project `GridUpdate::Records` as well. That they had to +change is the evidence that the substitution was load-bearing rather than +incidental. + +### a_records_commit_is_declined_off_the_grid_and_not_off_the_mode + +improvise-e5pf's damage half. `resolve::commit` is written to **decline** a +records commit it cannot compose (it needs `record_col`, gotcha 1). It asked +`state.view.mode.is_records()`; with the mode drifted to plain `Editing` over a +records grid it did not decline, and composed `typed_cell_command` plus a +`Basis` claim against a grid whose columns are `[coords…, +cat, measures…, +]`. +A wrong write, strictly worse than the black hole it exists to produce. + +The test states the drift rather than reaching it — reaching it takes the key +sequence the test above drives — and asserts no `Command::Model` is composed. + +**The control is the point.** A first version of this test passed against the +*broken* code, because `client_of` publishes no basis and `BasisCache::claim` +returns `None` until one arrives, so `commit` returned `None` for a reason that +had nothing to do with records mode: +`doctrine(an_injection_that_does_not_fail_names_an_untested_path)`, caught by +injecting the old spelling and watching the test still pass. `records_client` +now publishes a basis naming every record the grid projects, and the test +carries a positive control — the same `Editing` mode over a **regular** grid +must still compose a write — so "this client never writes" cannot pass it. + +**Fault injected**: the decline restored to `state.view.mode.is_records()`. The +test fails on `a records commit this client cannot compose was composed anyway`. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 340b6060..5e6ddd20 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -176,6 +176,9 @@ pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache { rows: layout.row_count(), cols: layout.col_count(), }, + // Asked of the layout, the same question the host's own `normal_mode` + // asks, so the two front ends cannot answer it differently (e5pf). + records: layout.is_records_mode(), } } @@ -613,6 +616,9 @@ pub fn grid_updates(mirror: &GridCache, desired: &GridCache) -> Vec if mirror.extent != desired.extent { updates.push(GridUpdate::Extent(desired.extent)); } + if mirror.records != desired.records { + updates.push(GridUpdate::Records(desired.records)); + } if mirror.row_frame != desired.row_frame { updates.push(GridUpdate::RowFrame(desired.row_frame.clone())); } diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index 7c766cee..549ed4c1 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -433,6 +433,9 @@ pub struct GridCache { /// How big the **whole** grid is, of which the frames are one window /// (improvise-bpq2). Not derivable, and not framed — see §3. pub extent: GridExtent, + /// Is this the wide records layout (gotcha 1)? **Projected, never derived + /// from the axes or from a mode** — improvise-e5pf, and §3. + pub records: bool, } /// The size of the whole grid: what a cursor clamps against. A named pair, @@ -464,6 +467,7 @@ impl GridCache { .collect(); vec![ GridUpdate::Extent(self.extent), + GridUpdate::Records(self.records), GridUpdate::RowFrame(self.row_frame.clone()), GridUpdate::ColFrame(self.col_frame.clone()), GridUpdate::ColWidths(self.col_widths.clone()), @@ -575,6 +579,9 @@ pub enum GridUpdate { /// How big the whole grid is (improvise-bpq2). Alone among these arms /// its batch position is **not** load-bearing — see §3. Extent(GridExtent), + /// Whether this is the wide records layout (improvise-e5pf). Frames + /// nothing, like [`GridUpdate::Extent`]. + Records(bool), } impl WidgetCache for GridCache { @@ -613,6 +620,8 @@ impl WidgetCache for GridCache { // No `fit_to_labels`: the extent is the size of the grid the // frames are a window onto, not something they frame. GridUpdate::Extent(extent) => self.extent = *extent, + // Nor this: it is what KIND of grid the frames describe. + GridUpdate::Records(records) => self.records = *records, } } } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 6f2c0533..5b20cbbb 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -290,7 +290,7 @@ fn composed( let mode = AppMode::from_name(args.first()?)?; // `declined` answers this first, and both halves hold the rule: // no path opens a room this client cannot commit. - stranding(&mode).is_none().then(|| entered(mode)) + stranding(&mode).is_none().then(|| entered(state, mode)) } // ── panels ─────────────────────────────────────────────────── @@ -312,10 +312,13 @@ fn composed( panel, open: !closing, }, - Command::ChangeMode(match closing { - true => AppMode::Normal, - false => panel.mode(), - }), + changed_mode( + state, + match closing { + true => AppMode::Normal, + false => panel.mode(), + }, + ), ]) } @@ -374,7 +377,7 @@ fn composed( Some(vec![ model(state, ModelCommand::CreateView(name.clone()))?, Command::SwitchView(name), - Command::ChangeMode(AppMode::Normal), + changed_mode(state, AppMode::Normal), ]) } "delete-view-at-cursor" => delete_view_at_cursor(state), @@ -456,15 +459,25 @@ fn stranding(mode: &AppMode) -> Option<&'static str> { } /// `EnterMode::execute`: clear the incoming mode's minibuffer, then change. -fn entered(mode: AppMode) -> Vec { +fn entered(state: &ClientState, mode: AppMode) -> Vec { let mut commands = Vec::new(); if let Some(mb) = mode.minibuffer() { commands.push(write_buffer(&mb.buffer_key, String::new())); } - commands.push(Command::ChangeMode(mode)); + commands.push(changed_mode(state, mode)); commands } +/// `AppMode::Normal` settled against the GRID, the way `App::settle_normal_mode` +/// settles it against the layout (improvise-e5pf). The **one** home. +fn changed_mode(state: &ClientState, mode: AppMode) -> Command { + let settled = match mode { + AppMode::Normal if state.grid.records => AppMode::RecordsNormal, + other => other, + }; + Command::ChangeMode(settled) +} + // ── the grid cursor ───────────────────────────────────────────────────── /// A mirror of `navigation.rs`'s `MoveKind`, spelled again because that crate @@ -691,14 +704,14 @@ fn commit(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option Option> { // `exit_mode_for`: the tidy-up goes FIRST, where an aborting // `AddCategory` cannot reach it (gotcha 21). - let mut commands = vec![Command::ChangeMode(match state.view.mode.is_records() { - true => AppMode::RecordsNormal, - false => AppMode::Normal, - })]; + let mut commands = vec![changed_mode(state, AppMode::Normal)]; let name = text.trim(); if !name.is_empty() { commands.push(model( @@ -749,11 +759,7 @@ fn advanced(state: &ClientState, edit_mode: AppMode, advance: Advance) -> Option let row_max = rows.saturating_sub(1); let col_max = cols.saturating_sub(1); let (row, col) = state.view.cursor().selected; - if matches!(advance, Advance::Right) - && state.view.mode.is_records() - && row >= row_max - && col >= col_max - { + if matches!(advance, Advance::Right) && state.grid.records && row >= row_max && col >= col_max { return None; } let landed = match advance { @@ -980,7 +986,7 @@ fn switch_view_at(state: &ClientState, index: usize) -> Option> { let summary = state.views.views.get(index)?; Some(vec![ Command::SwitchView(summary.name.clone()), - Command::ChangeMode(AppMode::Normal), + changed_mode(state, AppMode::Normal), ]) } @@ -1662,6 +1668,9 @@ mod tests { /// an **unhandled command** and never a state that said no (improvise-rmvx). fn furnished(mode: AppMode) -> ClientState { let mut state = client_on(record_cell()); + // A records MODE over a non-records GRID is a state no host can + // produce, so the fixture projects both (improvise-e5pf). + crate::WidgetCache::apply(&mut state.grid, &GridUpdate::Records(mode.is_records())); state.view.mode = mode; state.grid.page_coords = vec![crate::PageAxis { cat: "Quarter".into(), @@ -2490,6 +2499,7 @@ mod tests { #[test] fn a_records_mode_commit_is_the_hosts() { let mut client = client_on(record_cell()); + crate::WidgetCache::apply(&mut client.grid, &GridUpdate::Records(true)); client.view.mode = AppMode::records_editing(); assert_eq!( resolve(&client, ev(KeyCode::Enter)), diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index ed3d7df3..6c3b778f 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1094,6 +1094,146 @@ fn records_pair() -> (App, App) { (app, twin) } +/// One key on both ends, the client refreshed from a real projection rather +/// than from the host's own slice — which is what lets them drift. +fn in_lockstep(app: &mut App, client: &mut ClientState, key: KeyEvent, what: &str) { + let KeyOutcome::Local(cmds) = resolve(client, key) else { + panic!("{what}: the client must answer this key itself"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(app, key); + let window = client.window; + let fresh = grid_of( + app, + Viewport { + row_offset: client.view.cursor().row_offset, + col_offset: client.view.cursor().col_offset, + rows: window.0, + cols: window.1, + }, + ); + for update in fresh.seed() { + improvise_protocol::WidgetCache::apply(&mut client.grid, &update); + } +} + +/// improvise-e5pf: `AppMode::Normal` is settled against the GRID, so the client +/// must settle it too. See `context/test-evidence/clients.md`. +#[test] +fn leaving_a_panel_in_records_mode_lands_where_the_host_lands() { + let (mut app, _) = records_pair(); + assert!( + app.view_state.mode.is_records(), + "the fixture must really be in records mode, or nothing separates the \ + two answers" + ); + let mut client = records_client(&app); + assert!( + client.grid.records, + "and the client must be told so by the projection, not by its own mode" + ); + + for (key, what) in [ + (KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE), "open"), + (KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE), "close"), + ] { + in_lockstep(&mut app, &mut client, key, what); + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", app.view_state.mode), + "{what}: the client and the host disagree about the mode" + ); + } +} + +/// improvise-e5pf: the commit declines off the GRID, so a client whose mode has +/// drifted still declines. See `context/test-evidence/clients.md`. +#[test] +fn a_records_commit_is_declined_off_the_grid_and_not_off_the_mode() { + let (app, _) = records_pair(); + let mut drifted = records_client(&app); + // The drift itself, stated rather than reached: a plain `Editing` over a + // records grid is what the substitution produced. + drifted.view.mode = AppMode::editing(); + drifted + .view + .buffers + .insert("edit".into(), "typed".to_string()); + drifted.view.cursor_mut().selected = stored_cell_at(&drifted); + + let enter = KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE); + assert!( + !composes_a_write(&drifted, enter), + "a records commit this client cannot compose was composed anyway" + ); + + // The control: the same mode over a REGULAR grid must still commit, or the + // decline above is "this client never writes" and proves nothing. + let plain = furnished_host(); + let mut ordinary = paged_client(&plain); + ordinary.view.mode = AppMode::editing(); + ordinary + .view + .buffers + .insert("edit".into(), "42".to_string()); + ordinary.view.cursor_mut().selected = stored_cell_at(&ordinary); + assert!(!ordinary.grid.records, "the control must be a regular grid"); + assert!( + composes_a_write(&ordinary, enter), + "the control never wrote either, so the decline above is vacuous" + ); +} + +/// A records client seeded the way a real one is: from a projection, with a +/// basis published, so a declined commit is declined for the stated reason. +fn records_client(app: &App) -> ClientState { + let mut client = client_of(app, (8, 8)); + improvise_protocol::WidgetCache::apply( + &mut client.basis, + &improvise_protocol::basis::published( + app.document_generation(), + app.layout_version(), + client + .grid + .cells + .iter() + .flatten() + .filter_map(|cell| match &cell.id { + Some(improvise_protocol::CellId::Record(rc)) => Some((rc.record, 0)), + _ => None, + }) + .collect(), + ), + ); + client +} + +/// The first cell the client's own projection says is a stored record. +fn stored_cell_at(client: &ClientState) -> (usize, usize) { + client + .grid + .cells + .iter() + .enumerate() + .find_map(|(r, row)| { + row.iter() + .position(|cell| matches!(cell.id, Some(improvise_protocol::CellId::Record(_)))) + .map(|c| (r, c)) + }) + .expect("the fixture must project a stored cell") +} + +fn composes_a_write(client: &ClientState, key: KeyEvent) -> bool { + match resolve(client, key) { + KeyOutcome::Local(cmds) => cmds + .iter() + .any(|c| matches!(c, improvise_protocol::Command::Model { .. })), + _ => false, + } +} + /// Where the client's own projected grid puts a creating column. Asked of the /// projection, never re-derived from the axes — that is ADR 002's whole point. fn creating_cell(client: &ClientState, kind: improvise_protocol::CreateKind) -> (usize, usize) {