From 6f98bfd99572524d13bcc019d3dcab7687cefd5d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:03:40 -0700 Subject: [PATCH] Review round 1: green CI, the SortData hole reopened, the paging regression removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the adversarial review of #78. Two blocked; all four are real and one of them was a straight error on my part. 1. CI WAS RED AND I NEVER RAN THE CHECKS. Nine comment runs over the two-line cap, all mine, all introduced this PR; both checks exit 0 at 8a559b2 and run in under a second. Behind it `--check-evidence` also failed, with two orphan pairs that are the direct product of my own renames: the consolidation renamed two tests and left their evidence naming the old names. All thirteen fixed. The renamed entries are rewritten rather than relabelled — each now records what the rename was FOR, which is the thing a relabel would have thrown away. 3. MY SortData CONCLUSION WAS WRONG, and the mechanism I removed was needed. I reported "could not build a fixture where SortData reorders". It reorders trivially. The cause was not the reviewer's guess (already sorted insertion) but better: ToggleRecordsMode pushes effect::SortData as its FIRST effect, so a records-mode fixture built by pressing R has an already-sorted store BY CONSTRUCTION and cannot demonstrate anything order-sensitive. My store was fine all along. The `layout.records` coords hash is restored, and `sorting_the_store_in_records_mode_invalidates_the_cursor` pins it with a fixture that sets the axes directly. It asserts the row COUNT and the LABELS are both unchanged, so it is the sharp case rather than a shape change wearing its name. Removing the hash again fails that test and NOTHING ELSE IN THE WORKSPACE. The process error is the part worth keeping: I concluded "cannot reproduce" from ONE fixture without varying it, then filed that conclusion as a finding. "I could not reproduce it" and "it does not reproduce" are different claims and I filed the second having earned only the first. 4. THE page_selections COMPONENT IS DELETED. With prune_empty off a page flip renumbers nothing and the cursor was still sent home — PR #76's own ruling is that this is worse than the hole improvise-7kvw names. My comment defended it with a basis argument for a cursor decision, which is how it got written. `paging_with_nothing_pruned_invalidates_no_cursor` is the negative arm that was missing; restoring the three lines fails it and nothing else, and the positive arm passes either way because `with_frozen_records` already applies pruning — which is exactly why the negative arm had to exist. ALSO: context/code/command.md gains "Two observations, not one". My ni6k reasoning lived only in a commit message, which design-principles.md §6 explicitly forbids — "a reason recorded only there is a reason nothing can ever find wrong". `view_frame`'s doc pointed at a section that did not exist; now it resolves. The stale "since improvise-9mo9 they call reset_cursor" sentence is corrected: no effect calls it any more. Finding 2 (the #77 merge conflict over AxisEntry::GroupEnd) is NOT in this commit — #77 lands first and I merge main afterwards, handling the variant deliberately rather than letting it fall into a `=> {}` that would make a frame change invisible to the cursor. Filed, not fixed: improvise-5ubs (a cyclic-successor all() can omit a variant while its doc says it cannot — the reviewer's meta-finding, that a consolidation owes a test its surviving derivation is TOTAL) and improvise-9v0j (view_frame vs view_shape separated by example, not by property). improvise-gfvj closed with the correction. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 65 ++++++++- context/test-evidence/clients.md | 72 ++++++++-- context/test-evidence/command.md | 96 +++++++++++++ crates/improvise-command/src/ui/app.rs | 20 ++- crates/improvise-command/src/ui/effect.rs | 13 +- crates/improvise-command/src/ui/project.rs | 153 +++++++++++++++++++++ crates/improvise-protocol/src/basis.rs | 5 +- crates/improvise-protocol/src/lib.rs | 5 +- crates/improvise-wasm-client/src/socket.rs | 6 +- tests/typed_string_agreement.rs | 5 +- 10 files changed, 395 insertions(+), 45 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 4725e0ab..89a58867 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1002,10 +1002,11 @@ to do, and both counters were believing it. Two failures, in opposite directions from the one assumption: - **`AddItem`/`AddItemInGroup`/`RemoveItem` declare `CellData` and reshape the - grid.** Since improvise-9mo9 they call `reset_cursor`, so `cursor_epoch` moved + grid.** improvise-9mo9 made them call `reset_cursor`, so `cursor_epoch` moved while `layout_version` did not — one event moving one of two counters that answer the same question, and a client told its cursor is invalid while its - layout tier still reads current. + layout tier still reads current. (Since improvise-ni6k **no effect calls + `reset_cursor`**; see "two observations" below.) - **A refused or duplicate `AddCategory` declares `LayoutGeometry` and changes nothing.** `Model::add_category` returns `Ok` for a name it already has, so the reachable case does not even abort. Every other client's next `Command::Model` @@ -1026,6 +1027,66 @@ move (`layout_version`), and *which views* moved (`Sessions::adopt_cursor_epoch` improvise-nynk). `layout_version` is therefore bumped **once per batch** rather than once per effect; nothing reads it but "did it move". +### Two observations, not one (improvise-ni6k) + +`view_shape` above is not the only fingerprint taken per batch, and the reason is +the part worth keeping: **the cursor's question and `layout_version`'s question are +different questions, and a single hash cannot answer both.** + +`layout_version` asks *are the coordinates a client's keys hang off still the ones +it composed against*. The cursor asks *does position `(r, c)` still name what it +named*. `AddCategory` of a category with no items separates them exactly: no row +and no column moves, so no cursor is invalid — but every `CellKey` now needs a +coordinate it did not before, so the basis tier must move. Unifying the two broke +tests in **both** directions, which is how the distinction was found rather than +reasoned to; do not re-merge them. + +So `view_frame` sits beside `view_shape`, and `last_reframed_views` beside +`last_reshaped_views`. `view_frame` hashes the **drawn frame** — `row_cats`, +`col_cats`, and the `AxisEntry` sequence of both axes in order, with each group +header's collapse state. Four things about it are load-bearing: + +- It builds through **`GridLayout::with_frozen_records`, never `GridLayout::new`**. + `new` does not apply `prune_empty`; a fingerprint built on it cannot see a pruned + row, which is the entire class improvise-7kvw belongs to. +- It hashes each record's **coordinates** as well as the axis entries, because + records mode numbers its rows by position (gotcha 1): `row_items` is `0,1,2…` + whatever order the store is in. `SortData` changes neither the row count nor any + label and still makes every position name a different record — and drill + `pending_edits` are keyed by row index, so this is gotcha 18's disease exactly. +- It hashes **no page selection**, deliberately. A page flip renumbers the grid + only when `prune_empty` is on, and then `with_frozen_records` already sees it in + the entries; hashing the selection as well homes the cursor on every `[`/`]` with + pruning off, which PR #76 ruled is worse than the hole improvise-7kvw names. Both + arms are tested. The temptation is to argue the selection belongs in a *basis* + because the cells under it differ — true, and irrelevant: that is + `record_versions`' job, and this hash answers the cursor. +- The diff runs **inside the effect loop**, per model effect, for the same reason + `document_config` does. At end-of-batch it overwrites a cursor a *later* effect + in the same batch placed deliberately — `[AddItem(measure), ]`, + which is how naming a measure at the bottom-right of a records grid works, sent + the cursor home. + +**No effect declares any of this.** improvise-9mo9's twelve `reset_cursor` calls +are gone, along with the six inline no-op predicates (`moved`, `drawn`, `real`, +`existed`) that existed only to stop those calls firing on a no-op, and the two +helpers `effective_items` and `item_is_drawn` that served them. That table was the +shape gotcha 24 names as the disease, and it had a hole (improvise-7kvw) the day it +was written; six more were enumerated on the ticket before it was fixed. + +**Cost, measured rather than asserted** (`benches/reshape_observation.rs`, the +fixtures `grid_projection` uses): one `SetCell` batch goes from 172µs to 209µs at +400 cells, 7.09ms to 7.97ms at 18,000, and 35.2ms to 38.0ms at 90,000 — +8% to ++22%, the largest share on the smallest fixture where the batch is cheapest. The +batch is dominated by `rebuild_layout` and `recompute_formulas`, not by the +fingerprint. The ticket required this measurement before landing, because "much +cheaper than `grid_of`" is exactly the kind of claim that criterion exists to +refuse. + +**What is still guarded in one direction only, and is filed**: nothing asserts that +a batch which reframes a view is *always* one that `view_shape` also moves, or the +converse. The separation is argued here and tested by example, not by property. + `document_config` is everything a `LayoutGeometry` effect can write that the file would carry: the categories with their items and groups, and every view's configuration whether it draws or not. It is what raises `model_state.dirty`, and diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 8ab53306..78969b84 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -301,23 +301,49 @@ asserted, because setting the string alone is the plausible half-fix**, and the revision bump is asserted too: a renderer polls the revision, and a rejection it cannot see is a rejection it cannot draw. -### every_rejection_message_says_the_edit_did_not_land +### every_rejection_message_says_what_it_refused Found by the PR #12 Copilot review: `DocumentReplaced` read "the document was replaced — reloading", and nothing in `fold_frame` reloads anything. **A message that names an action the code does not take is worse than a vague one, because -the user waits for it.** The per-variant wording still differs — the classes are -different things to be told — but the one fact they share is the one the ADR -guarantees, so it is asserted over all of them rather than one at a time. +the user waits for it.** -### a_typed_string_is_classified_as_the_host_classifies_it +Renamed and re-scoped by improvise-8uj8, which is the more interesting half. The +test was called `every_rejection_message_says_the_edit_did_not_land` and its own +doc said the fact "is asserted over all of them rather than one at a time" — while +it drove a **hand-written list of five**, written before PR #26 added +`NoDocumentPath` and `SaveFailed`. So two variants were silently exempted from a +rule the test called universal, which is worse than no test: the claim was false +about its own subject. -The three-way split, matching `commit_regular_cell_value`: a number is a number, -anything else is text, and *empty clears* rather than writing an empty string. -**This test states an obligation it does not discharge**: it asserts the three -outcomes by hand rather than comparing with the host's function, so the two -copies could drift and stay green. See `context/code/clients.md` for why the -duplication exists and what the real fix is. +The re-scope is that the claim, not the messages, was wrong. A save failure did +not fail to *apply* anything, so "the edit did not land" is the wrong sentence for +it. `Rejection::refused_an_edit` (exhaustive) splits the two classes and +`Rejection::all` (a cyclic walk, the same shape `AppMode::all` uses) drives them, +so a new variant fails the build rather than slipping out of the list. + +Fault injected: wording `SaveFailed` as `"hmm"` fails the test — the arm the old +five-item list could not have seen. Adding a `Rejection` variant fails the build +at `basis.rs`'s `refused_an_edit` and `after`. + +### a_typed_string_composes_the_frame_that_carries_it + +Was `a_typed_string_is_classified_as_the_host_classifies_it`, a test that **stated +an obligation in its name and did not discharge it** — it asserted three outcomes +by hand rather than comparing against the host's function, so the two copies of +the rule could drift and stay green. improvise-j7v6 removed the duplication rather +than the drift: `commit_regular_cell_value` now calls `improvise_protocol::typed`, +which the browser's compose path already used. + +Per `doctrine(a_consolidation_that_keeps_its_compatibility_test_is_unfinished)` +the test could not survive as a comparison — type identity replaced its claim, and +a hand-written comparison beside the shared function would be the copy coming +back. It is re-scoped to what this crate still owes, which nothing else covers: +the **frame** a typed string composes — basis, cell and command. + +The classification's own obligation moved to `tests/typed_string_agreement.rs`, +where it is checked against the thing it could really disagree with: what +`.improv` calls a number (gotcha 34). ### a_cell_projection_that_overtakes_its_labels_is_lost @@ -2062,3 +2088,27 @@ done The other two losses are deliberate: `pointer.spec.js`'s sidebar test was superseded by the open-at-launch ruling, and the widgets file was merged into this one. + +### the_keys_that_compose_a_switch_view_are_these + +improvise-lltt is deferred on a **premise**, not a judgement: its own note says it +is "unreachable today ONLY because no shipped client emits `Command::SwitchView`", +and asks for a guard asserting exactly that, so the silent failure becomes loud +the moment the premise stops holding. + +The guard failed on its first run. Two live paths in this client's own resolver +(`resolve.rs`): `switch-view-at-cursor` and `create-and-switch-view`. The other +half was already live — the same resolver composes `ModelCommand::TransposeAxes`. +So the scenario lltt describes is reachable today from the shipped view panel, and +the ticket was escalated P2 → P1 rather than fixed here, the honest fix being the +protocol change it describes. + +**It is an `assert_eq` against those two names, not an assert-empty, and the list +is not an exemption list.** It pins what is true so a *third* live path fails, and +its message says so — because a reader who found this passing would otherwise +conclude the premise holds. When lltt is fixed the list empties and this test goes +with it. + +Why the enumeration and not a read: the bug was already written into a passing +test's comment elsewhere (`"Aimed at the host's own view… HideItem writes +view_mut()"`) and had been read as a fixture note for as long as it had been there. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 5d469e4e..fe19a728 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2765,3 +2765,99 @@ The lesson worth carrying: **an observation that is correct about *what* changed can still be wrong about *when* it is read.** Everything above this entry argues that observing beats declaring; this is the cost of that move, and it lands precisely on the one effect that *unsets* what the observation sets. + +### the_published_basis_names_the_records_the_grid_draws_and_no_others + +improvise-23qd. The set of records a `Projection::Basis` carries was derived +**twice** — `ClientSession::basis_update`, the producer, and `server_truth`, a +`#[cfg(test)]` helper that is the **yardstick every fold test measures against**. +So a change to which records get published moved the thing and its own measure +together, and every test stayed green whatever the change was. Not a helper that +fabricates an input; a helper that fabricates the expected *output*. + +Both now call `published_basis`. Note what that alone does **not** buy: the +ticket's stated proof ("alter the filter and watch the basis tests fail") is +impossible once one function feeds both sides — they still move together. A shared +derivation removes the drift and removes the yardstick with it. + +So the yardstick is spelled out rather than derived: this test names North, South +and East by hand for `app_with_a_reshapeable_grid`'s three records over six cells, +and asserts that a grid where every cell is an `Aggregate` publishes **none** — +because an aggregate names no record, so there is nothing to claim a version for. + +Fault injected: `.take(2)` on the one surviving filter — the ticket's exact "change +which records get published" — fails this test **and nothing else in the crate**. +A second injection, `record_version_of(..) + 1`, is caught instead by the two +end-to-end `basis_check` tests, which is the split worth knowing: the counters have +other guards, the membership had none. + +The fixture carries its own premise check. The first version assumed a new category +lands on `Axis::None`; it lands on `Column`, and the assertion said so instead of +the test quietly proving nothing. + +### an_aborted_batch_bumps_layout_version_for_what_it_did_run + +improvise-2avl, with its negative twin `an_aborted_batch_bumps_nothing_for_effects_it_never_ran`. +PR #70 moved the `layout_version` bump from "once per `LayoutGeometry` effect, +inside the loop" to "once per batch, after the loop". Gotcha 40 records the in-loop +position as load-bearing — an aborted batch must not claim effects it never ran — +and the author's argument for why end-of-batch is safe *for `layout_version` +specifically* (nothing ever clears it, so an end comparison sees only what ran, +whereas `dirty` must stay in the loop because `Save` unsets it mid-batch) is sound +and was written into `context/code/command.md`. **An argument is not a test**, and +neither arm had one. + +Both arms are proved **independently**, which is the part that earns them: bumping +unconditionally fails only the second test, and skipping `observe_reshape` on abort +fails only the first. A single injection failing both would not have shown that the +two tests see different things. + +Each fixture asserts `app.last_batch_aborted()` before asserting anything about the +counter, because a batch that did not actually abort would pass both arms while +testing neither. + +### sorting_the_store_in_records_mode_invalidates_the_cursor + +improvise-ni6k names `SortData` as its **sharpest** case: it changes neither the +row count nor any label, and every position still names a different record. A guard +written as "did the shape change" misses it; `pending_edits` are keyed by row index +(gotcha 1), so row 0 being West and then East is gotcha 18's misdirected-write +disease arriving through a different door. + +`view_frame` therefore hashes each record's **coordinates** alongside the axis +entries, because records-mode `row_items` are positional — `0,1,2…` whatever order +the store is in. Fault injected: deleting those three lines fails this test and +**nothing else in the workspace**. + +**The fixture trap, which is the durable part.** PR #78 first shipped without the +coords hash, on the stated ground that no fixture could be built where `SortData` +reorders anything. That was wrong, and the reason is worth more than the fix: +`ToggleRecordsMode` pushes `effect::SortData` as its **first** effect, so a +records-mode fixture built by pressing `R` has an already-sorted store *by +construction* and cannot demonstrate anything order-sensitive. The store itself was +fine all along. So this fixture sets `_Index`/`_Dim` directly rather than going +through the key path. + +The test asserts the count and the labels are **unchanged** before asserting the +cursor moved, so it cannot pass by accidentally becoming an ordinary shape change. + +### paging_with_nothing_pruned_invalidates_no_cursor + +The negative twin of `paging_across_a_prune_that_drops_a_row_invalidates_the_cursor`, +and the reason `view_frame` hashes **no page selection**. + +improvise-7kvw is real only under `prune_empty`: a page flip changes which records +are visible, not which categories frame them, so with pruning off a grid position +keeps naming the same cell. PR #76 ruled that homing the cursor on every `[`/`]` +regardless — `[`/`]` being navigation a user does constantly — is **worse than the +hole**. Hashing the page selection does exactly that. + +The seductive argument for hashing it anyway is that two pages with identical row +labels really are different data. True, and irrelevant: that is `record_versions`' +question, and `view_frame` answers the cursor's. A basis argument applied to a +cursor decision is how this regression got written in the first place. + +Fault injected: restoring the three `page_selections` lines fails this test and +nothing else. The positive arm keeps passing either way, because +`with_frozen_records` applies `prune_empty` and so already sees a dropped row in +the axis entries — which is why the negative arm is the one that had to exist. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 1cfe84d2..8d28fbca 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -70,9 +70,8 @@ fn view_shape(model: &crate::model::types::Model, view: &View) -> u64 { h.finish() } -/// One view's drawn **frame**, as a hash: the axis entries a grid would draw, in -/// order. A different question from [`view_shape`] — `context/code/command.md` -/// §"two observations" (improvise-ni6k). +/// One view's drawn **frame**, as a hash. A different question from [`view_shape`] +/// — `context/code/command.md` §"Two observations, not one". fn view_frame(model: &crate::model::types::Model, view: &View) -> u64 { use std::hash::{Hash, Hasher}; // `with_frozen_records`, not `new`: `new` does NOT apply `prune_empty`, and a @@ -101,11 +100,11 @@ fn view_frame(model: &crate::model::types::Model, view: &View) -> u64 { } } } - // Not a label, but it is what a Page coordinate names: two pages with the - // same row labels are different grids, and the cells under them differ. - let mut pages: Vec<(&String, &String)> = view.page_selections.iter().collect(); - pages.sort_unstable(); - pages.hash(&mut h); + // Records mode numbers rows by POSITION (gotcha 1), so `row_items` is 0,1,2… + // whatever order the store is in — `context/code/command.md`. + for record in layout.records.iter().flat_map(|r| r.iter()) { + record.coords.hash(&mut h); + } h.finish() } @@ -679,9 +678,8 @@ impl App { self.model_state.dirty |= now != basis; config_basis = Some(now); } - // The cursor's observation, **inside the loop** for the same reason: - // a batch that reframes and then places the cursor deliberately must - // keep the placement — `context/code/command.md` (improvise-ni6k). + // Inside the loop, or a batch that reframes and then places the + // cursor loses the placement — `context/code/command.md`. if matches!(kind, EffectKind::Model(_)) && let Some(basis) = frame_basis.as_ref() { diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 2170a340..42224c69 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1257,8 +1257,7 @@ mod tests { } /// Serialized view configuration. Kept for the NEGATIVE arm only, where a - /// stricter hash than `document_config` is what the test wants — the - /// positive arm is `document_config`'s (improvise-1xf4). + /// hash stricter than `document_config` is wanted (improvise-1xf4). fn view_config_fingerprint(app: &App) -> String { serde_json::to_string(&app.model_state.workbook.views).expect("views serialize") } @@ -1469,9 +1468,8 @@ mod tests { } } - /// `test_app` plus the three things the thirteen need in order to have - /// something to change: a second view to delete, a group to collapse, and - /// one already-hidden item, so `ShowItem` is not a no-op. + /// `test_app` plus what the thirteen need to have something to change: a + /// second view, a group, and one hidden item so `ShowItem` is no no-op. fn grouped_app() -> App { let mut app = test_app(); { @@ -1703,9 +1701,8 @@ mod tests { assert!(app.view_state.view_back_stack.is_empty()); } - /// improvise-2avl. PR #70 moved the `layout_version` bump out of the effect - /// loop; gotcha 40 records the in-loop position as load-bearing, and the - /// argument that it is safe HERE was written down and not tested. Both arms. + /// improvise-2avl: the end-of-batch `layout_version` bump was argued safe + /// and never tested. Both arms — `context/test-evidence/command.md`. #[test] fn an_aborted_batch_bumps_layout_version_for_what_it_did_run() { let mut app = test_app(); diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index b11d9432..b2b81d34 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -2501,6 +2501,159 @@ mod tests { } } + /// The other half of improvise-7kvw: with pruning OFF a page flip renumbers + /// nothing. Evidence: `context/test-evidence/command.md`. + #[test] + fn paging_with_nothing_pruned_invalidates_no_cursor() { + use improvise_protocol::ModelCommand; + + let mut app = app_with_a_page_dependent_grid(); + app.model_state.workbook.active_view_mut().prune_empty = false; + app.rebuild_layout(); + let before = grid_shape(&app); + let epoch = app.cursor_epoch(); + let mut sessions = Sessions::default(); + let (asker, _) = sessions.connect(&app, 100, 100); + let (bystander, _) = sessions.connect(&app, 100, 100); + sessions.apply(&mut app, bystander, &Command::SetRowOffset(1)); + + let applied = sessions.apply_caught_up( + &mut app, + asker, + ModelCommand::SetPageSelection { + category: "Quarter".into(), + item: "Q2".into(), + }, + ); + + assert_eq!( + before, + grid_shape(&app), + "with pruning off the page flip must renumber nothing, or the \ + fixture is testing the pruning case again" + ); + assert_eq!( + app.cursor_epoch(), + epoch, + "PR #76 ruled that homing the cursor on every [/] with pruning off \ + is worse than the hole improvise-7kvw names" + ); + assert_eq!( + sessions + .get_mut(bystander) + .unwrap() + .view() + .cursor() + .row_offset, + 1, + "and a bystander who had scrolled keeps their place" + ); + assert!( + !diff(applied) + .1 + .contains(&Broadcast::To(bystander, Command::ResetCursor)), + "nothing moved, so nobody is owed an invalidation" + ); + } + + /// Records mode numbers rows by position, so `row_items` cannot see a + /// reorder. What each position NAMES is the question (gotcha 1). + fn record_order(app: &App) -> Vec { + app.layout + .records + .iter() + .flat_map(|r| r.iter()) + .map(|row| row.coords.0[0].1.clone()) + .collect() + } + + /// A records-mode document whose store is out of key order. The axes are set + /// DIRECTLY — see `context/test-evidence/command.md` for why `R` cannot. + fn records_app_out_of_order() -> App { + let mut wb = Workbook::new("Records"); + wb.add_category("Region").unwrap(); + for item in ["West", "North", "East"] { + wb.add_item("Region", item).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + for region in ["West", "North", "East"] { + wb.model.set_cell( + CellKey::new(vec![ + ("Region".to_string(), region.to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]), + CellValue::Number(1.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 must be in records mode" + ); + app + } + + /// improvise-ni6k's sharpest case: `SortData` changes neither the row count + /// nor any label. Evidence: `context/test-evidence/command.md`. + #[test] + fn sorting_the_store_in_records_mode_invalidates_the_cursor() { + use improvise_protocol::ModelCommand; + + let mut app = records_app_out_of_order(); + let labels_before = grid_shape(&app); + let rows_before = app.layout.row_count(); + let order_before = record_order(&app); + let mut sessions = Sessions::default(); + let (asker, _) = sessions.connect(&app, 100, 100); + let (bystander, _) = sessions.connect(&app, 100, 100); + sessions.apply(&mut app, bystander, &Command::SetRowOffset(1)); + + let applied = sessions.apply_caught_up(&mut app, asker, ModelCommand::SortData); + + assert_eq!( + order_before, + vec!["West", "North", "East"], + "the fixture only says anything if the store starts out of key order" + ); + assert_eq!( + record_order(&app), + vec!["East", "North", "West"], + "and only if sorting really reorders it" + ); + // The two facts that make this the sharp case: a guard written as "did + // the shape change" would see nothing here. + assert_eq!( + rows_before, + app.layout.row_count(), + "the row count is equal" + ); + assert_eq!(labels_before, grid_shape(&app), "and so are the labels"); + assert_eq!( + sessions + .get_mut(bystander) + .unwrap() + .view() + .cursor() + .row_offset, + 0, + "row 0 was West and is now East, and a bystander was not told" + ); + assert!( + diff(applied) + .1 + .contains(&Broadcast::To(bystander, Command::ResetCursor)), + "the bystander's mirror was reset and told nothing about it" + ); + } + /// The second negative arm: a command the document REFUSES reshaped nothing, so /// it owes nobody an invalidation. Every refusable effect, not one of them. #[test] diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index d6900267..e82c4d43 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/crates/improvise-protocol/src/basis.rs @@ -93,9 +93,8 @@ impl WidgetCache for BasisCache { } impl Rejection { - /// True when the batch was refused as **stale** — something was asked for - /// and nothing was applied. False for the two `Save` answers, which - /// refused no edit: `context/code/protocol.md` §5. + /// True when the batch was refused as **stale**. False for the two `Save` + /// answers, which refused no edit: `context/code/protocol.md` §5. pub fn refused_an_edit(&self) -> bool { match self { Self::DocumentReplaced diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 00fb7187..e7e23bed 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -258,9 +258,8 @@ impl AppMode { .collect() } - /// The next variant in the cycle [`AppMode::all`] walks. Total on purpose: - /// with no `None` arm the only way to answer for a new variant is to name - /// another one, which is what splices it into the walk. + /// The next variant in the cycle [`AppMode::all`] walks. Total on purpose — + /// `context/code/protocol.md` says what that does and does not buy. fn after(&self) -> Self { match self { Self::Normal => Self::editing(), diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 0161cc4e..6ec4b26a 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -234,10 +234,8 @@ mod tests { ) } - /// What this crate still owes: the FRAME a typed string composes — basis, - /// cell and command. The classification itself is `protocol::typed`, which - /// `commit_regular_cell_value` now calls too, so there is nothing left here - /// for a hand-written comparison to compare (improvise-j7v6). + /// What this crate still owes: the FRAME a typed string composes. The + /// 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")))); diff --git a/tests/typed_string_agreement.rs b/tests/typed_string_agreement.rs index f4e5286f..1c61b9f4 100644 --- a/tests/typed_string_agreement.rs +++ b/tests/typed_string_agreement.rs @@ -25,9 +25,8 @@ fn through_a_document(text: &str) -> Option { reloaded.model.get_cell(&key).cloned() } -/// The three outcomes, and the reason the check is worth having: a `Text` -/// classification that the writer let read back as a number would make -/// `typed` a third answer wearing the badge of the first. +/// A `Text` the writer let read back as a number would make `typed` a third +/// answer wearing the badge of the first. #[test] fn what_typed_calls_text_does_not_read_back_as_a_number() { for text in ["pending", "N/A", "42abc", "1,200", "SUM", "-", "1.2.3"] {