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

Merge branch 'main' of github.com:fiddlerwoaroof/improvise into fix/tg27-narrow-window

Browse Source 
# Conflicts:
#	context/test-evidence/clients.md
This commit is contained in:
Edward Langley
2026-09-22 16:00:00 -07:00
parent c8d715346f eefd992538
commit 76f69ce671
18 changed files with 673 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/clients.md
+49
View File
@@ -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
context/code/command.md
+30 -9
View File
@@ -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,
context/code/protocol.md
+29
View File
@@ -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
context/test-evidence/clients.md
+79
View File
@@ -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
context/test-evidence/command.md
+44
View File
@@ -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.
crates/improvise-command/src/command/cmd/commit.rs
+149 -18
View File
@@ -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<Box<dyn Effect>>) {
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<Box<dyn Effect>>) {
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));
crates/improvise-command/src/ui/host.rs
+15 -3
View File
@@ -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
crates/improvise-command/src/ui/project.rs
+6
View File
@@ -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)]
crates/improvise-protocol/src/client.rs
+162
View File
@@ -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<Sent>,
}
/// 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<Command>,
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<Command> = 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<Command> {
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();
crates/improvise-protocol/src/frame.rs
+6 -2
View File
@@ -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!(
crates/improvise-protocol/src/lib.rs
+1 -1
View File
@@ -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};
crates/improvise-wasm-client/src/lib.rs
+3 -7
View File
@@ -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
crates/improvise-wasm-client/src/socket.rs
+14 -7
View File
@@ -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<ClientFrame> {
pub fn typed_cell_frame(&mut self, row: usize, col: usize, text: &str) -> Option<ClientFrame> {
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 {
crates/improvise-wasm-client/tests/browser_round_trip.rs
+75 -1
View File
@@ -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)"
);
}
crates/improvise-wasm-client/tests/lockstep.rs
+2 -1
View File
@@ -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(),
}
}
}
crates/improvise-worker-server/src/worker.rs
+1 -1
View File
@@ -218,7 +218,7 @@ mod tests {
}
}
}
ServerFrame::Rejected(_) => {}
ServerFrame::Rejected(_) | ServerFrame::Accepted => {}
}
}
grid.clone()
crates/improvise-worker-server/tests/post_message.rs
+1 -1
View File
@@ -112,7 +112,7 @@ impl MainThread {
}
}
}
ServerFrame::Rejected(_) => {}
ServerFrame::Rejected(_) | ServerFrame::Accepted => {}
}
}
crates/improvise-ws-server/tests/socket.rs
+7
View File
@@ -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.