diff --git a/context/code/clients.md b/context/code/clients.md index aed8eee8..c9daea61 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1237,6 +1237,55 @@ a fact about a browser window, which is why no `Command` carries it and why it i not on `ViewState`. `(0, 0)` means this client has not announced one yet, and a client in that state resolves no cursor key — it has been sent no grid either. +### The optimistic fold is put back by *the fold*, not by a saved cache (improvise-nxah) + +`Client::key` folds a locally-resolved batch **before** sending it, so the page +redraws on the keystroke rather than on the round trip. If the host then refuses +the frame — ADR 001 clause 4 means it applied *none* of it, and restores every +mirror to the pre-frame state — the client has moved and the host has not. +Measured against a real `Host`: after a refused commit the client sat one row +down in `Normal`-ish re-entry with its edit buffer already cleared for the next +cell, while the host's mirror of that same client was still at the origin +holding the user's typing. + +Three things decide the shape, and the first is the one that is easy to get +wrong. **What rolls back is `ViewState`, the thing the fold produces — never a +cache.** Restoring a cache would be a second answer to "what does this client +hold" beside the fold's, which is the defect design-principles §5 names and +`improvise-edp` is the worked example of. The caches need nothing anyway: a +`Command::Model` is a **no-op in `reduce_view`**, so the optimistic fold never +touched them, and everything a *successful* frame does to them arrives as +projections. Note the corollary that makes the bug report's own evidence wrong: +`page_coords` cannot diverge this way, because it moves only under +`Projection::Grid`. What diverges is the mode, the cursor, the status and the +buffers — the view commands that ride along **with** the model command. + +**The whole slice goes back, and that is what serves improvise-3al1 rather than +fighting it.** The 2026-09-21 wave recorded these two tickets as in direct +contradiction, on the ground that restoring a snapshot would wipe +`ViewState::buffers` — the user's typing that 3al1 requires stay staged. That is +measurably not so, and the reason is the first paragraph of §4 in +`context/code/protocol.md`: **only a frame carrying a `Command::Model` can be +refused.** The typing frames are `SetBuffer`s, carry no model command, are never +refused, and have already settled. Rolling back the *commit* therefore restores +the buffer to its pre-commit value — which is the typing, intact. The test +asserts that explicitly rather than leaving it as an inference. + +**A refusal drops one frame and replays the rest.** `ClientState` keeps a +`settled` slice — everything the host has answered — and an `in_flight` queue of +the frames sent since, each marked `awaited` exactly when the host will answer +it. A refusal pops the head and rebuilds `view` from `settled` plus what remains. +Restoring a single saved snapshot instead is **unsound under pipelining**: two +mutating frames may be outstanding over a socket, and a snapshot taken before +the first discards the second, which the host *did* apply. A frame the host will +not answer settles as soon as nothing older is waiting — only from the front, +because settling it out of turn would lose the order. + +What this does **not** do is force. A rejected batch now keeps its values and +leaves the user where they were; re-composing the same intent against a current +basis at one keystroke is still `improvise-3al1`, and it is unblocked rather +than done. + ### `socket.rs` — the frames, the wording, and the one composition **Where the frames live: not here.** `ClientFrame`/`ServerFrame` are diff --git a/context/code/command.md b/context/code/command.md index 572169fb..4a3ed7ad 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2510,11 +2510,14 @@ no undo. The follow-up edit is one `l` away in the column that just appeared. Li `add_measure` it is a schema change, so it applies immediately rather than being staged. -`inserts_a_column` asks whether committing `key` inserts a column, invalidating -every column index to its right. It is asked of the *layout* — `record_col` resolves -the synthetic `_Dim` position into what the column actually is — rather than -re-derived from the value or the cursor, so there is one answer to "what is this -column" (gotcha 1). +`inserts_a_column` asks whether committing `value` at `key` inserts a column, +invalidating every column index to its right. It is asked of the *layout* — +`record_col` resolves the synthetic `_Dim` position into what the column actually is +— rather than re-derived from the cursor, so there is one answer to "what is this +column" (gotcha 1). It takes the value because **an empty name creates nothing**: +`created_name` is the one place that rule lives, read by `add_measure`, +`add_category` and this predicate, so a commit that inserts no column cannot claim +to (improvise-0ygd). `exit_mode_for` returns the normal-mode counterpart of an editing mode, used by `CommitAndAdvance` to compute the mode to land in if the advance aborts. @@ -2545,10 +2548,28 @@ the last row `Enter` wraps to `col + 1`, which after the insert is `+cat` again, the editor reopened category creation and a user naming two categories in a row never reached the column they had made. `Tab` reached it the same way. Not advancing is also the *useful* answer rather than merely the safe one: `add_category` inserts -the new column exactly where the cursor already is, so staying put lands on it. The -measure `+` shares none of this, and not by luck — it is the last column, so there is -no column to its right for an index to shift into, and both advances are already -special-cased at the bottom-right edge. +the new column exactly where the cursor already is, so staying put lands on it. + +**The measure `+` is the same case, and this paragraph said it was not** (corrected +by improvise-0ygd; see `context/test-evidence/command.md`). The claim was that +`+measure` has no column to its right for an index to shift into — false, because +the column that shifts is `+measure` itself: naming a measure inserts the new column +at the cursor's own index and pushes the `+` one to the right, exactly as `+cat` +does. The claim that "both advances are already special-cased at the bottom-right +edge" was worse than wrong, since that special case *is* the bug: `Tab` there ran +`AddRecordRow` and jumped to `(row + 1, 0)`, manufacturing a record row and leaving +the measure the user had just named unreachable, while `Enter` survived only because +`EnterAdvance` aborts at the bottom-right and the `SetSelected` restore above is what +then held the cursor. So `inserts_a_column` covers both creating columns and both +advances suppress. + +What the special case is genuinely for is bulk entry, and the fix keeps it by +splitting on the **buffer** rather than the column. `+measure` is always the +rightmost column, so `cursor.col >= col_max` in records mode names that one cell and +no other — the "Tab at the bottom-right starts the next record" path is reachable +only from it, by tabbing off the end of a row onto the `+` and pressing Tab with +nothing typed. An empty commit creates no column, so it still starts the row; a +commit that names a measure does not. **`CommitKind`** is what a minibuffer commit produces. The variants differ only in the model mutation they build and in where they leave the user — reading the buffer, diff --git a/context/code/protocol.md b/context/code/protocol.md index a2b49452..39b71581 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1227,6 +1227,35 @@ already holds what is current through the projections it has been folding all along. That is what makes rejection cheap enough to be a *normal outcome* rather than an error. +### `Accepted`, and why a rejection alone is not a protocol (improvise-nxah) + +`ServerFrame::Rejected` told a client that a frame did nothing. Nothing told it +that a frame *did* something. That asymmetry is not cosmetic, because the +browser folds a frame's view commands **optimistically** before sending it: a +refusal must put them back, and to know *which* frame's folds to put back the +client has to be able to retire the ones that landed. With only a refusal on the +wire, a client holding two frames in flight cannot tell which of them a +`Rejected` answers — so the rollback either addresses the wrong frame or never +happens. This is the same shape as the publication gap ADR 001 originally left: +it said a batch carries a basis and never said how a client learns one, and +`Projection::Basis` had to close it. Here it said a batch may be refused and +never said how a client learns it was not. + +So a **mutating** frame is answered exactly once, `Accepted` or `Rejected`, and +the two facts that make that a usable rule are already load-bearing elsewhere. +*Only* `Command::Model` can be refused — `Checked::against` says so exhaustively +and `Host::commands` only enters `mutating_frame` when one is present — so a +frame carrying no `Command::Model` is answered by neither, and a client can tell +by inspecting the frame it is about to send rather than by remembering a rule. +And `Rejection::refused_an_edit` already separates the two `Save` refusals from +the batch refusals, so a failed write cannot consume the answer owed to an edit. + +`Accepted` carries **no state**, and it goes **last** in the frame's answer, for +the reason a published basis goes last (§5): everything the frame moved has +already arrived as projections, so a client discarding its undo knows exactly +what it is discarding. A client that folded the acknowledgement first would be +retiring a frame whose effects it had not yet been shown. + --- ## 5. `basis.rs` — what a batch was composed against diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index b109d916..66f9975d 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3813,6 +3813,85 @@ vacuous-assertion shape design-principles §7 names. The same test is what makes the refusal arm safe to leave unreachable instead of inventing a second frame for it. +--- + +## The optimistic fold's rollback (improvise-nxah) + +The design argument is `context/code/clients.md`, "The optimistic fold is put +back by *the fold*"; the wire half is `context/code/protocol.md` §4. + +### a_refused_commit_leaves_the_client_where_the_host_says_it_is + +**The bug.** `Client::key` folded a locally-resolved batch before sending it and +`fold_frame` answered `ServerFrame::Rejected` with a sentence and nothing else, +so a refused commit left the client's view slice ahead of the host's. Measured +at `22ab58f` against a real `Host`: the client sat at cursor `(1, 0)` with +`buffers {"edit": ""}` while the host's mirror of that same client was at the +origin with `{"edit": "12007500"}` — the user's typing. + +**Why it is not vacuous.** The comparison is against +`Host::sessions().get(id).view()`, the host's **own** mirror, which ADR 001 +clause 4 restores to the pre-frame state on a refusal — so the test asserts the +client landed where the authority says it is, not merely that two derivations +agree (improvise-vw47). It is total over `ViewState` rather than a hand-kept +list of fields: everything but `status_msg` is compared, and that one is carved +out with its own assertion because the rejection sentence is this client's own +notification and the mirror has no way to hold one. The fixture first asserts +the two ends **agree before** the refusal, so a test that could never have +disagreed is not passing by accident. + +**The fault injected.** Dropping the `self.replay()` call from +`ClientState::refused` — keeping the queue bookkeeping and doing no rollback — +fails it. + +**What it does not cover.** That the *ordering* of a rollback is right under +pipelining; that is the next entry. + +### refusing_one_of_two_frames_in_flight_keeps_the_other + +**What it separates.** A single saved snapshot from a queue. Two mutating frames +are folded before either is answered; the host refuses the first and accepts the +second. Restoring a snapshot taken before the first would discard the second, +which the host *did* apply — so the client would diverge in the other direction. +The assertion is not "the state changed": it is equality against a second, +independently built client that only ever sent the accepted frame, which is what +"right" means here rather than "the two ends match". + +**The fault injected.** Making `ClientState::replay` rebuild from `settled` and +replay nothing — the single-snapshot design — fails this test and no other in +the file, which is what shows it is aimed at the ordering and not at the +rollback. + +### a_frame_the_host_will_not_answer_settles_without_one + +**The invariant the queue rests on.** Only `Command::Model` can be refused, so a +frame carrying none is answered by neither `Accepted` nor `Rejected`; holding +such a frame in the queue would make the next answer pop the wrong entry. The +test sends a `SetStatus`-only frame, asserts nothing is held, then sends a +commit and refuses it, asserting the earlier frame survived. + +**The fault injected.** Hard-coding `awaited = true` in +`ClientState::fold_outgoing` fails it. + +### a_refused_save_does_not_roll_back_a_frame_it_never_answered + +**The second way the queue could pop the wrong entry.** `ClientFrame::Save` is a +request, not a `Command` (improvise-368h), and its two refusals answer no +`Commands` frame. `Rejection::refused_an_edit` is the existing predicate that +separates them, and `refused` consults it before popping. + +**The fault injected.** Replacing `if rejection.refused_an_edit()` with +`if true` fails it: a document with nowhere to write undoes an unrelated edit. + +**And at the transport.** `a_client_on_a_socket_is_seeded_and_gets_its_edit_back` +(its own entry is above) was **extended, not added to**: after the edit comes back down the socket the test now +asserts the next frame is `ServerFrame::Accepted` — so the acknowledgement is +evidenced at the transport, in its position (last, after everything the frame +produced), and not only in the host's unit tests. Deleting the `Accepted` push +from `Host::mutating_frame` fails it. + +--- + ## A narrow window does not widen the document (improvise-tg27) `harness/tests/narrow.spec.js`. Reproduced at `22ab58f` before anything was diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 2602bf7d..2529868f 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4452,3 +4452,47 @@ What it cannot see, stated because the gap is one edit away from mattering: `draw.rs::draw_welcome` builds its lines as a local array inside a ratatui render function, so no walk reaches them. Its `:formula` line was corrected by hand in the same change. + +### tab_naming_a_measure_lands_on_the_column_it_just_created + +BUG improvise-0ygd. Typing a name into the trailing `+` column and pressing +**Tab** created the measure and then threw the user across the grid: it added a +record row nobody asked for and parked the cursor at `(row + 1, 0)`, so the +column the commit had just made was never reached. + +The cause is a **stale index**, not an abort and not an ordering. `AdvanceDir::Right` +computes `at_bottom_right` from `CursorState::col_count`, which is the *pre-commit* +column count — `apply_effects` rebuilds the layout only after the whole batch +(gotcha 23). Naming a measure **inserts a column** at the cursor's own index and +pushes `+measure` one to the right, so after the commit the cursor is no longer at +the right edge and the bulk-entry branch is answering a question about a grid that +no longer exists. + +`context/code/command.md` stated the opposite in prose — "The measure `+` shares +none of this, and not by luck — it is the last column, so there is no column to its +right for an index to shift into" — which is false: the column that shifts is +`+measure` itself. That sentence is corrected in the same change. + +Two faults were injected against the fix: + +- **Revert the new-measure arm** of `inserts_a_column` (back to `NewCategory` + only). This test fails on the row count and the cursor; the bulk-entry sibling + below still passes, which is the point — a fixture on an ordinary measure column + cannot see a `+measure` bug, because `+measure` is *always* the last column and + so is the only cell from which the bottom-right branch is reachable at all. +- **Drop the `created_name` half**, making the predicate value-blind. This test + passes; the sibling fails. That is the rival worth naming: it fixes the case the + ticket describes while eating the contract the branch exists for. + +### tab_on_an_empty_new_measure_column_still_starts_a_record_row + +The companion, and the one that stops the fix over-firing. `+measure` is always +the rightmost column, so `cursor.col >= col_max` in records mode means *the cursor +is on `+measure`* and nothing else — the "Tab at the bottom-right starts the next +record" contract is reachable from that one cell. Bulk entry uses it by tabbing off +the end of a row onto `+measure` with an empty buffer and pressing Tab again. + +So the discriminating input is the **buffer**, not the column: an empty commit +creates nothing, inserts no column, and must still start the next row. Injecting +the value-blind predicate above turns the branch off entirely and this test fails +with `row_count` 1 where 2 is owed, while the headline test above stays green. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index b454bd63..ce9511ca 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -337,6 +337,134 @@ mod tests { ); assert!(dbg.contains("SetStatus"), "Expected a status, got: {dbg}"); } + + /// A one-record records grid with `Region` as its only category and `Revenue` as + /// its only measure: columns are `[Region, +cat, Revenue, +measure]`. + fn one_record_grid() -> crate::ui::app::App { + use crate::model::cell::{CellKey, CellValue}; + use crate::ui::app::App; + use crate::view::Axis; + use crate::workbook::Workbook; + + let mut wb = Workbook::new("Records"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(100.0), + ); + { + let v = wb.active_view_mut(); + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + v.set_axis("Region", Axis::None); + v.set_axis("_Measure", Axis::None); + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert!( + app.layout.is_records_mode(), + "the fixture is not records mode" + ); + app + } + + /// Put the cursor on the trailing `+` column of the last row, in records-normal. + fn park_on_new_measure(app: &mut crate::ui::app::App) -> (usize, usize) { + let bottom = app.layout.row_count() - 1; + let right = app.layout.col_count() - 1; + assert!( + matches!(app.layout.record_col(right), Some(RecordCol::NewMeasure)), + "the last column is not the new-measure column, so this proves nothing" + ); + app.cursor_mut().selected = (bottom, right); + app.view_state.mode = AppMode::RecordsNormal; + (bottom, right) + } + + fn press(app: &mut crate::ui::app::App, code: improvise_protocol::key::KeyCode) { + use improvise_protocol::key::{KeyEvent, KeyModifiers}; + app.handle_key(KeyEvent { + code, + modifiers: KeyModifiers::NONE, + }) + .unwrap(); + } + + /// BUG (improvise-0ygd) — see `context/test-evidence/command.md`. + #[test] + fn tab_naming_a_measure_lands_on_the_column_it_just_created() { + use improvise_protocol::key::KeyCode; + + let mut app = one_record_grid(); + let (bottom, right) = park_on_new_measure(&mut app); + let rows_before = app.layout.row_count(); + + press(&mut app, KeyCode::Char('i')); + for ch in "Cost".chars() { + press(&mut app, KeyCode::Char(ch)); + } + press(&mut app, KeyCode::Tab); + + assert!( + app.model_state + .workbook + .model + .measure_item_names() + .contains(&"Cost".to_string()), + "the measure was not created, so the cursor question is moot" + ); + assert_eq!( + app.layout.row_count(), + rows_before, + "Tab on `+measure` manufactured a record row the user never asked for" + ); + assert_eq!( + app.cursor().selected, + (bottom, right), + "Tab must land on the inserted measure column, not jump home" + ); + assert_eq!( + app.layout.record_col(app.cursor().selected.1), + Some(&RecordCol::Measure("Cost".to_string())), + "the cursor is not on the measure the commit just created" + ); + } + + /// The bulk-entry contract the fix must not eat: an EMPTY `+` commit creates no + /// column, so Tab there still starts the next record row. + #[test] + fn tab_on_an_empty_new_measure_column_still_starts_a_record_row() { + use improvise_protocol::key::KeyCode; + + let mut app = one_record_grid(); + park_on_new_measure(&mut app); + let rows_before = app.layout.row_count(); + let measures_before = app.model_state.workbook.model.measure_item_names().len(); + + press(&mut app, KeyCode::Char('i')); + press(&mut app, KeyCode::Tab); + + assert_eq!( + app.model_state.workbook.model.measure_item_names().len(), + measures_before, + "an empty `+` commit must create no measure" + ); + assert_eq!( + app.layout.row_count(), + rows_before + 1, + "Tab at the bottom-right of a records grid must start the next record row" + ); + assert_eq!( + app.cursor().selected, + (rows_before, 0), + "bulk entry lands at the start of the new row" + ); + } } // ── Commit commands (mode-specific buffer consumers) ──────────────────────── @@ -424,27 +552,29 @@ fn commit_plain_records_edit( } } +/// The name a creating column's buffer holds, or `None` when it names nothing and so +/// creates no column. The one rule; `inserts_a_column` reads the same answer. +fn created_name(value: &str) -> Option<&str> { + Some(value.trim()).filter(|name| !name.is_empty()) +} + /// Typing a name into the trailing column creates that measure. A schema change, so /// it applies immediately even inside a drill. fn add_measure(value: &str, effects: &mut Vec>) { - let name = value.trim(); - if name.is_empty() { - return; + if let Some(name) = created_name(value) { + effects.push(Box::new(effect::AddItem { + category: MEASURE_CATEGORY.to_string(), + item: name.to_string(), + })); } - effects.push(Box::new(effect::AddItem { - category: MEASURE_CATEGORY.to_string(), - item: name.to_string(), - })); } /// Typing a name into `+cat` creates the category **and its column**, and writes no /// coordinate on the current row (improvise-2zq) — see `context/code/command.md`. fn add_category(value: &str, effects: &mut Vec>) { - let name = value.trim(); - if name.is_empty() { - return; + if let Some(name) = created_name(value) { + effects.push(Box::new(effect::AddCategory(name.to_string()))); } - effects.push(Box::new(effect::AddCategory(name.to_string()))); } fn commit_cell_value( @@ -471,12 +601,13 @@ fn commit_cell_value( commit_regular_cell_value(key, value, effects); } -/// Whether committing `key` inserts a column, invalidating every index to its right. -/// Asked of the *layout*, so there is one answer to "what is this column" (gotcha 1). -fn inserts_a_column(ctx: &CmdContext, key: &CellKey) -> bool { - crate::view::synthetic_record_info(key) - .and_then(|(_, col_idx)| ctx.layout.record_col(col_idx)) - .is_some_and(|col| matches!(col, RecordCol::NewCategory)) +/// Whether committing `value` at `key` inserts a column, invalidating every index to +/// its right. Asked of the *layout* — one answer to "what is this column" (gotcha 1). +fn inserts_a_column(ctx: &CmdContext, key: &CellKey, value: &str) -> bool { + created_name(value).is_some() + && crate::view::synthetic_record_info(key) + .and_then(|(_, col_idx)| ctx.layout.record_col(col_idx)) + .is_some_and(|col| matches!(col, RecordCol::NewCategory | RecordCol::NewMeasure)) } /// Direction to advance after committing a cell edit. @@ -526,7 +657,7 @@ impl Cmd for CommitAndAdvance { self.cursor.row, self.cursor.col, ))); - if inserts_a_column(ctx, &self.key) { + if inserts_a_column(ctx, &self.key, &self.value) { // Staying put is what lands on the inserted column (improvise-2zq); the // restore above is what makes "staying put" something we do, not inherit. effects.push(Box::new(effect::RebuildLayout)); diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 387ed955..8044f881 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -46,6 +46,12 @@ impl Host { self.sessions.is_empty() } + /// The mirrors, read-only. Not a second door: `&Sessions` exposes no way to + /// fold, so a client's command still arrives through [`Host::handle`]. + pub fn sessions(&self) -> &Sessions { + &self.sessions + } + /// Attach a client that has reported an extent, and hand back its bootstrap. /// Total, and nothing is owed to the other clients. pub fn connect(&mut self, rows: usize, cols: usize) -> (SessionId, Addressed) { @@ -168,6 +174,12 @@ impl Host { return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused); } } + // Last, after everything the frame owed: a client folds the answer to + // discard its undo, and the projections describe what it is discarding. + out.push(Addressed { + to: id, + msg: ServerFrame::Accepted, + }); out } @@ -371,9 +383,9 @@ mod tests { } } } - // A rejection carries no state: nothing was applied, so - // there is nothing for a grid to fold. - ServerFrame::Rejected(_) => {} + // Neither answer carries state: one applied nothing, and what + // the other applied arrived as projections before it. + ServerFrame::Rejected(_) | ServerFrame::Accepted => {} } } grid diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index c853db9a..3abdd8eb 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -897,6 +897,12 @@ impl Sessions { self.sessions.get_mut(&id) } + /// One mirror, read-only. `ClientSession::fold` is private, so this hands + /// out the server's picture of a client without handing out a way to move it. + pub fn get(&self, id: SessionId) -> Option<&ClientSession> { + self.sessions.get(&id) + } + /// One command answered and applied. A host answers the whole frame first, so /// this is the tests' door, not a host's (improvise-s17o, `context/code/command.md`). #[cfg(test)] diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index fd09185b..8010e920 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -35,6 +35,22 @@ pub struct ClientState { /// Bumped by every fold. A DOM renderer polls this instead of /// re-serializing the whole state to find out whether anything moved. pub revision: u64, + /// The view slice as the host has answered it: everything this client sent + /// and heard back about, and nothing it is still waiting on. + #[serde(skip)] + pub settled: ViewState, + /// Every frame sent since, oldest first, so a refusal can drop one and + /// replay the rest. Why a queue: `context/code/clients.md`. + #[serde(skip)] + pub in_flight: std::collections::VecDeque, +} + +/// One frame this client has sent. `awaited` is true exactly when the host +/// will answer it, which is exactly when it can be refused — `clients.md`. +#[derive(Debug, Clone, PartialEq)] +pub struct Sent { + pub cmds: Vec, + pub awaited: bool, } impl ClientState { @@ -50,6 +66,10 @@ impl ClientState { self.tiles = TileBarCache::default(); self.document = DocumentCache::default(); self.basis = BasisCache::default(); + // A rebase replaces the document, so every frame still in flight names + // a picture that has gone; there is nothing left to put back. + self.in_flight.clear(); + self.settled = self.view.clone(); for projection in &snapshot.seed { self.apply_projection(projection); } @@ -68,6 +88,70 @@ impl ClientState { self.revision += 1; } + /// Fold a batch this client is about to **send**, keeping it so a refusal + /// can drop it. The one door for an outgoing frame — `clients.md`. + pub fn fold_outgoing(&mut self, cmds: &[Command]) { + let awaited = cmds.iter().any(|c| matches!(c, Command::Model { .. })); + self.in_flight.push_back(Sent { + cmds: cmds.to_vec(), + awaited, + }); + self.settle_sent(); + for cmd in cmds { + self.fold(cmd); + } + } + + /// The host applied the oldest awaited frame whole, so it is the settled + /// slice's now. + pub fn accepted(&mut self) { + if let Some(sent) = self.in_flight.pop_front() { + for cmd in &sent.cmds { + reduce_view(&mut self.settled, cmd); + } + } + self.settle_sent(); + } + + /// The host applied **none** of the oldest awaited frame (ADR 001 clause + /// 4), so neither does this client. Why the whole slice: `clients.md`. + pub fn refused(&mut self, rejection: Rejection) { + // A refused *save* answers no `Commands` frame, so it must not consume + // one's entry — `Rejection::refused_an_edit` is that question. + if rejection.refused_an_edit() { + self.in_flight.pop_front(); + self.replay(); + } + self.last_rejection = Some(rejection); + self.settle_mode(); + self.revision += 1; + } + + /// The settled slice plus every frame still in flight, in order. What a + /// dropped frame leaves behind is the *rest* of them, not nothing. + fn replay(&mut self) { + self.view = self.settled.clone(); + let pending: Vec = self + .in_flight + .iter() + .flat_map(|sent| sent.cmds.iter().cloned()) + .collect(); + for cmd in &pending { + reduce_view(&mut self.view, cmd); + } + } + + /// A frame the host will not answer settles as soon as nothing older is + /// waiting: only the front may settle, or the order would be lost. + fn settle_sent(&mut self) { + while self.in_flight.front().is_some_and(|sent| !sent.awaited) { + let sent = self.in_flight.pop_front().expect("just peeked"); + for cmd in &sent.cmds { + reduce_view(&mut self.settled, cmd); + } + } + } + /// Settle the `Normal` pair against the grid this client **holds**, after /// every fold. Observed, never composed — `context/code/clients.md`. fn settle_mode(&mut self) { @@ -253,6 +337,84 @@ mod tests { assert_eq!(state.view.status_msg, "saved"); } + /// A mutating frame, composed the only way there is (gotcha 43), paired + /// with the view command that rides along with it. + fn commit_like(status: &str) -> Vec { + let mut cache = crate::BasisCache::default(); + crate::WidgetCache::apply( + &mut cache, + &crate::basis::published(0, 0, Default::default()), + ); + vec![ + Command::Model { + basis: cache.claim([]).expect("seeded"), + cmd: crate::ModelCommand::CleanEmptyRecords, + }, + Command::SetStatus(status.into()), + ] + } + + /// Two frames in flight and the **first** refused: the second was applied + /// by the host and must survive. See `context/test-evidence/clients.md`. + #[test] + fn refusing_one_of_two_frames_in_flight_keeps_the_other() { + let mut state = ClientState::default(); + state.fold_outgoing(&commit_like("first")); + state.fold_outgoing(&commit_like("second")); + + state.refused(Rejection::LayoutMoved); + state.accepted(); + + let mut only_second = ClientState::default(); + only_second.fold_outgoing(&commit_like("second")); + assert_eq!( + state.view, only_second.view, + "dropping the refused frame must leave the client where a client \ + that had only ever sent the accepted one stands" + ); + assert!( + state.in_flight.is_empty(), + "both frames were answered; nothing may still be held" + ); + } + + /// A frame the host does not answer must not consume an answer, or the + /// queue addresses the wrong frame. Evidence file. + #[test] + fn a_frame_the_host_will_not_answer_settles_without_one() { + let mut state = ClientState::default(); + state.fold_outgoing(&[Command::SetStatus("typing".into())]); + assert!( + state.in_flight.is_empty(), + "a frame carrying no Command::Model is never refused, so holding \ + it would make the next answer pop the wrong entry" + ); + assert_eq!(state.settled.status_msg, "typing"); + + state.fold_outgoing(&commit_like("committed")); + state.refused(Rejection::LayoutMoved); + assert_eq!( + state.view.status_msg, "typing", + "the refusal drops the commit and keeps what already settled" + ); + } + + /// A refused *save* answers no `Commands` frame (improvise-368h), so it + /// must not consume one's entry. Evidence file. + #[test] + fn a_refused_save_does_not_roll_back_a_frame_it_never_answered() { + let mut state = ClientState::default(); + state.fold_outgoing(&commit_like("committed")); + + state.refused(Rejection::NoDocumentPath); + + assert_eq!(state.in_flight.len(), 1, "the commit is still unanswered"); + assert_eq!( + state.view.status_msg, "committed", + "a save that could not be written undid an edit it has no bearing on" + ); + } + #[test] fn every_fold_bumps_the_revision_so_a_renderer_can_poll_it() { let mut state = ClientState::default(); diff --git a/crates/improvise-protocol/src/frame.rs b/crates/improvise-protocol/src/frame.rs index 78aa1815..8b793750 100644 --- a/crates/improvise-protocol/src/frame.rs +++ b/crates/improvise-protocol/src/frame.rs @@ -39,8 +39,8 @@ impl ClientFrame { } } -/// A frame to a client. Three variants for the three shapes -/// `Sessions::apply` hands back, and no fourth (gotcha 39). +/// A frame to a client. Three of the four are the shapes `Sessions::apply` +/// hands back (gotcha 39); the fourth answers a frame — `protocol.md` §4. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum ServerFrame { /// Drop everything and start from this: on connect, and again to *every* @@ -52,6 +52,9 @@ pub enum ServerFrame { /// Nothing was applied, and here is why. Addressed to the asker alone /// and carrying no new state — the [`Rejection`] variants are the list. Rejected(Rejection), + /// The asker's mutating frame landed whole. Carries no state: everything + /// it moved arrived as projections first — `context/code/protocol.md` §4. + Accepted, } impl ServerFrame { @@ -101,6 +104,7 @@ mod tests { GridUpdate::RowFrame(AxisFrame::flat(["North"])), ))]), ServerFrame::Rejected(Rejection::LayoutMoved), + ServerFrame::Accepted, ] { let json = serde_json::to_string(&frame).expect("serializes"); assert_eq!( diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index d74dfb84..63043c42 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -18,7 +18,7 @@ pub mod variants; pub mod viewmodel; pub use basis::{Basis, BasisCache, Rejection}; -pub use client::ClientState; +pub use client::{ClientState, Sent}; pub use frame::{ClientFrame, ServerFrame}; pub use help::{HelpLine, Hint, hint_line, hints, mode_label}; pub use key::{KeyCode, KeyEvent, KeyModifiers}; diff --git a/crates/improvise-wasm-client/src/lib.rs b/crates/improvise-wasm-client/src/lib.rs index ef1b550f..ebf73c08 100644 --- a/crates/improvise-wasm-client/src/lib.rs +++ b/crates/improvise-wasm-client/src/lib.rs @@ -102,7 +102,7 @@ impl Client { /// position", or `None` — which is a race, not a normal path. #[wasm_bindgen] pub fn typed_cell( - &self, + &mut self, row: usize, col: usize, text: &str, @@ -191,9 +191,7 @@ impl Client { if cmds.is_empty() { return None; } - for cmd in &cmds { - self.state.fold(cmd); - } + self.state.fold_outgoing(&cmds); Some(ClientFrame::Commands(cmds)) } @@ -209,9 +207,7 @@ impl Client { let event = browser.to_key_event()?; match resolve(&self.state, event) { KeyOutcome::Local(cmds) => { - for cmd in &cmds { - self.state.fold(cmd); - } + self.state.fold_outgoing(&cmds); Some(ClientFrame::Commands(cmds)) } // Nothing to fold: a request moves no state of this client's, and diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 3f96c301..2d5a21b0 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -55,11 +55,14 @@ impl Client { } } ServerFrame::Rejected(rejection) => { + let message = rejection_message(&rejection); let state = self.state_mut(); - state.view.status_msg = rejection_message(&rejection); - state.last_rejection = Some(rejection); - state.revision += 1; + // The sentence goes on *after* the rollback: it is this + // client's own notification, and the slice it restores has none. + state.refused(rejection); + state.view.status_msg = message; } + ServerFrame::Accepted => self.state_mut().accepted(), } self.revision() } @@ -72,13 +75,17 @@ impl Client { /// Compose the write a user made by typing `text` into a viewport position. /// The four `None` shapes, and why nothing folds here: clients.md. - pub fn typed_cell_frame(&self, row: usize, col: usize, text: &str) -> Option { + pub fn typed_cell_frame(&mut self, row: usize, col: usize, text: &str) -> Option { let cell = self.record_cell_at(row, col)?; let basis = self.state().basis.claim([cell.record])?; - Some(ClientFrame::command(Command::Model { + let cmds = vec![Command::Model { basis, cmd: typed_cell_command(&improvise_protocol::CellId::Record(cell), text)?, - })) + }]; + // Folds nothing — a `Command::Model` is not the view slice's business — + // but it **registers**, or the host's answers address the wrong frame. + self.state_mut().fold_outgoing(&cmds); + Some(ClientFrame::Commands(cmds)) } } @@ -246,7 +253,7 @@ mod tests { /// classification is `protocol::typed`'s alone (improvise-j7v6). #[test] fn a_typed_string_composes_the_frame_that_carries_it() { - let client = client_showing(Some(CellId::Record(record("Revenue")))); + let mut client = client_showing(Some(CellId::Record(record("Revenue")))); assert_eq!( client.typed_cell_frame(0, 0, "42.5"), Some(ClientFrame::command(Command::Model { diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index edb1f6f8..4df4e14c 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -7,7 +7,7 @@ use improvise_core::view::Axis; use improvise_core::workbook::Workbook; use improvise_protocol::ClientFrame; use improvise_wasm_client::render::RenderRow; -use improvise_wasm_client::{Client, RenderModel}; +use improvise_wasm_client::{BrowserKey, Client, RenderModel}; use improvise_ws_server::host::{Addressed, Host}; /// The document the server opens: two regions by two measures, one cell @@ -560,3 +560,77 @@ fn ctrl_s_in_the_browser_writes_the_hosts_document() { "a save that landed is not a rejection: {answer:?}" ); } + +/// A refused frame must leave this client where the **host** says it is, and +/// the host says so in its own mirror. See `context/test-evidence/clients.md`. +#[test] +fn a_refused_commit_leaves_the_client_where_the_host_says_it_is() { + let mut host = host(); + let mut client = Client::new(); + + let handshake: ClientFrame = + serde_json::from_str(&client.set_viewport(10, 10)).expect("a viewport frame"); + let ClientFrame::Viewport { rows, cols } = handshake else { + panic!("set_viewport must produce a Viewport frame"); + }; + let (id, snapshot) = host.connect(rows, cols); + deliver(&mut client, &[snapshot]); + + // Typing carries no `Command::Model`, so none of it is refusable: each + // frame lands on the host and moves both ends together. + for key in ["i", "7", "5", "0", "0"] { + let frame = client + .key(&BrowserKey::new(key)) + .expect("every key in this run resolves locally"); + let answers = send(&mut host, id, &serde_json::to_string(&frame).expect("json")); + deliver(&mut client, &answers); + } + assert_eq!( + client.state().view.mode, + host.sessions().get(id).expect("attached").view().mode, + "the two ends must agree BEFORE the refusal, or the refusal proves nothing" + ); + + // The host's layout moves under a client that is told nothing, so the + // commit it is about to compose claims a basis that has gone. + host.app_mut() + .apply_effects(vec![Box::new(improvise_command::ui::effect::AddCategory( + "Channel".into(), + ))]); + + let commit = client + .key(&BrowserKey::new("Enter")) + .expect("a commit frame"); + let answers = send( + &mut host, + id, + &serde_json::to_string(&commit).expect("json"), + ); + assert!( + answers + .iter() + .any(|a| matches!(a.msg, improvise_protocol::ServerFrame::Rejected(_))), + "this fixture exists to be refused; it was answered {answers:?}" + ); + deliver(&mut client, &answers); + + // Everything but the sentence, which is this client's own notification and + // is written after the rollback: the mirror has no way to hold one. + let mut told = client.state().view.clone(); + let announced = std::mem::take(&mut told.status_msg); + assert_eq!( + &told, + host.sessions().get(id).expect("attached").view(), + "a refused frame moved the host's mirror not at all, so a client that \ + kept its optimistic fold holds a view slice the host disowns" + ); + assert!( + announced.contains("not applied"), + "and the user must be told, not silently rewound: {announced:?}" + ); + assert_eq!( + told.buffers.get("edit").map(String::as_str), + Some("12007500"), + "the rollback restores the typing rather than costing it (improvise-3al1)" + ); +} diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 1af2ab76..83d63d97 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1274,7 +1274,8 @@ fn deliver( client.fold(cmd); } } - improvise_protocol::ServerFrame::Rejected(_) => {} + improvise_protocol::ServerFrame::Rejected(r) => client.refused(r), + improvise_protocol::ServerFrame::Accepted => client.accepted(), } } } diff --git a/crates/improvise-worker-server/src/worker.rs b/crates/improvise-worker-server/src/worker.rs index da8edd75..472d3de0 100644 --- a/crates/improvise-worker-server/src/worker.rs +++ b/crates/improvise-worker-server/src/worker.rs @@ -218,7 +218,7 @@ mod tests { } } } - ServerFrame::Rejected(_) => {} + ServerFrame::Rejected(_) | ServerFrame::Accepted => {} } } grid.clone() diff --git a/crates/improvise-worker-server/tests/post_message.rs b/crates/improvise-worker-server/tests/post_message.rs index 88763541..f6d5dcbc 100644 --- a/crates/improvise-worker-server/tests/post_message.rs +++ b/crates/improvise-worker-server/tests/post_message.rs @@ -112,7 +112,7 @@ impl MainThread { } } } - ServerFrame::Rejected(_) => {} + ServerFrame::Rejected(_) | ServerFrame::Accepted => {} } } diff --git a/crates/improvise-ws-server/tests/socket.rs b/crates/improvise-ws-server/tests/socket.rs index 53eb59cc..e3b7b383 100644 --- a/crates/improvise-ws-server/tests/socket.rs +++ b/crates/improvise-ws-server/tests/socket.rs @@ -136,6 +136,13 @@ fn a_client_on_a_socket_is_seeded_and_gets_its_edit_back() { grid.cells[0][0].text, "12.00", "the edit never came back down the socket" ); + // The acknowledgement follows everything the frame produced, so a + // client discards its undo knowing what it is discarding (nxah). + assert_eq!( + recv(&mut ws).await, + ServerFrame::Accepted, + "a mutating frame that landed must be acknowledged, and last" + ); // A frame the vocabulary does not cover is skipped, not fatal. The // resize that follows genuinely reframes a 2x2 fixture to 1x1.