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

Review round 1: green CI, the SortData hole reopened, the paging regression removed

Browse Source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-20 21:03:40 -07:00
parent 7f58800c0f
commit 6f98bfd995
10 changed files with 395 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/code/command.md
+63 -2
View File
@@ -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), <place cursor>]`,
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
context/test-evidence/clients.md
+61 -11
View File
@@ -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.
context/test-evidence/command.md
+96
View File
@@ -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.
crates/improvise-command/src/ui/app.rs
+9 -11
View File
@@ -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()
{
crates/improvise-command/src/ui/effect.rs
+5 -8
View File
@@ -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();
crates/improvise-command/src/ui/project.rs
+153
View File
@@ -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<String> {
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]
crates/improvise-protocol/src/basis.rs
+2 -3
View File
@@ -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
crates/improvise-protocol/src/lib.rs
+2 -3
View File
@@ -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(),
crates/improvise-wasm-client/src/socket.rs
+2 -4
View File
@@ -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"))));
tests/typed_string_agreement.rs
+2 -3
View File
@@ -25,9 +25,8 @@ fn through_a_document(text: &str) -> Option<CellValue> {
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"] {