Test both arms of the records-mode settle (improvise-4dpj)

settle_normal_mode matches `Normal | RecordsNormal`, and replacing that with a
one-way `Normal` left the ENTIRE workspace suite passing: the existing crossing
test drives the layout both ways, but both branches end in `enter-mode normal`,
so the settle is handed `AppMode::Normal` either way. It discriminates "settles
at all" and "settles against the ending layout", never "settles from either
mode".

Gap 1, a_batch_that_leaves_records_settles_out_of_records_normal: the
production repro -- SetAxis { "_Index", Axis::None } from a records grid.
Injection is the one-way guard; it is the only test in the workspace that fails.

Gap 2, switching_views_across_records_ness_settles_against_the_view_switched_to
(raised by ChatGPT Codex on PR #91, verified correct on main first): the batch
SwitchViewAtCursor emits, both ways. A genuinely different path -- records-ness
is auto-detected from the axes, so nothing in the batch says "records".
Injection is moving the settle back above rebuild_layout; both fail by name.
Scope note in the evidence: this drives the effects, not the view-panel key
path, which needs a CmdContext in another wave's ground this round.

Also carries the command.md half of improvise-hsoe's three duplicate headings.

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-21 10:19:10 -07:00
parent 383e33a1a0
commit 78ca25b5e0
3 changed files with 214 additions and 10 deletions
+47
View File
@@ -732,6 +732,24 @@ layout (gotcha 23), so a batch that **crosses** — `:toggle-records-mode . ente
grid. One edit, after the rebuild, has no crossing case to get wrong, and it
also removes the per-edit coverage question three separate writers created.
**The guard is two-armed and the crossing test could only see one arm**
(improvise-4dpj). `settle_normal_mode` matches `Normal | RecordsNormal`, and
replacing that with a one-way `Normal` left the whole workspace green: the `:`
crossing drives the layout both ways, but both of its branches end in
`enter-mode normal`, so the mode handed to the settle is `AppMode::Normal`
either way. It discriminates "settles at all" and "settles against the ending
layout", and not "settles from **either** mode" — three questions that look
like one until an injection separates them. The `RecordsNormal` arm is live:
`SetAxis { "_Index", Axis::None }` applied from a records grid leaves
`is_records_mode()` false with the mode still `RecordsNormal`.
**And records-ness is auto-detected from the axes (gotcha 1), so a view switch
crosses it too.** `SwitchViewAtCursor` emits `[SwitchView(target),
ChangeMode(Normal)]`, which is a genuinely different path from the toggle the
`:` test drives — nothing in it says "records", the crossing falls out of the
target view's axis map. Moving the settle back before `rebuild_layout` is the
injection for both, and it fails each by name.
**This fixes the TUI and not the browser, and that is LIVE (improvise-e5pf).**
`Command::ChangeMode(AppMode::Normal)` is on the wire and `reduce.rs` folds it
as `view.mode = mode.clone()` with no resolution, so the browser front end still
@@ -1323,6 +1341,35 @@ cells will bump their records when it shares a batch with a `CellData` one — a
over-bump, costing a rejection that need not have happened, which is the same
asymmetry `Dirty::of` is deliberately sloppy in.
**The case list is checked for completeness, not for length** (improvise-26dm).
`layout_geometry_effects_actually_change_document_config` walked a hand-built
list under an `assert_eq!(cases.len(), 13)`, and a count catches a case being
*deleted* and never one being *absent* — the two are written identically. The
adversarial review of PR #78 added a fourteenth `pub struct` declaring
`LayoutGeometry` and the suite stayed green.
`doctrine(a_list_checked_for_length_reads_like_a_list_checked_for_completeness)`.
improvise-5ubs's answer — read serde's derive-generated variant list out of its
`unknown variant … expected one of` message — cannot reach this one: an effect
is a struct implementing a trait, so there is no derived enumeration of effects
at all. But there is one of the **wire vocabulary**, and gotcha 38 says
`ModelCommand` is derived from the model effects with `model_effects` mapping
each variant back to the effect it came from. So the list is now diffed against
the `LayoutGeometry` effects *reachable through `ModelCommand`*, which is an
enumeration something else maintains, and the failure names the missing effect
rather than a number. The two lists agree exactly today, which is itself the
evidence the substitution lost nothing.
**Its bound is stated rather than assumed**: this closes the effects on the
wire, and an effect declaring `LayoutGeometry` that no `ModelCommand` names is
still invisible — which is precisely the probe variant PR #78 injected. That is
an honest residual and not a fix, and it is the reason the ticket asked for a
compiler-maintained enumeration of *effects*: none exists without a proc macro
or inventory-style registration, and a fourth hand-rolled list in a fourth
place is the disease. A second `[…; 13]` array survives one function away in
`every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint` and is
filed, not smuggled in.
On abort, the loop resets `abort_effects` so the next dispatch starts clean and
records `batch_aborted` where a caller can still read it.
+84 -10
View File
@@ -1017,6 +1017,15 @@ Tier 2 moves, tier 3 does not. A shape change invalidates every in-flight
`CellData` batch through `layout_version` alone; it must not also masquerade as a
write to any record.
Fixture repaired under improvise-75ld: it asked for
`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where
`on_category_added` had already seated `Month` on Column — so the "layout change"
was a no-op, and the test passed only because the tier was bumped off the
declaration. It now asserts the axis before and after, and asks for `Axis::Row`.
This is the failure that turned "reclassify three effects" into "stop reading the
tier off the classification". Injected fault: the same as
`both_shape_counters_move_for_every_reshaping_command` — fails naming `SetAxis`.
### a_loaded_document_is_dirty_because_its_path_still_names_the_old_one
**A loaded document is dirty, and that is a behaviour change with a reason**
@@ -3076,16 +3085,6 @@ that reshapes nothing fails loudly rather than proving nothing.
Injected fault: as above. Fails naming `SetAxis`, then `AddItem` once the earlier
cases are removed.
### a_layout_change_moves_the_layout_version_and_no_record
Pre-existing test, fixture repaired under improvise-75ld. It asked for
`SetAxis { category: "Month", axis: Axis::Column }` against a fixture where
`on_category_added` had already seated `Month` on Column — so the "layout change"
was a no-op, and the test passed only because the tier was bumped off the
declaration. It now asserts the axis before and after, and asks for `Axis::Row`.
This is the failure that turned "reclassify three effects" into "stop reading the
tier off the classification".
### a_configuration_change_that_draws_nothing_is_still_a_document_change
The third case the shape/config split creates, and the one that forced two
@@ -3496,3 +3495,78 @@ The fix is not local and none is chosen: `grid_in` takes `&App` and
`recompute_formulas` needs `&mut Model`, so `grid_in` cannot recompute for the
view it is projecting. Candidates are on the ticket.
### a_batch_that_leaves_records_settles_out_of_records_normal
improvise-4dpj, gap 1. `settle_normal_mode` matches `Normal | RecordsNormal`,
and replacing that with a one-way `AppMode::Normal` left the **entire workspace
suite passing**. The existing crossing test,
`a_batch_that_leaves_records_and_then_sets_normal_lands_in_normal`, drives the
layout both ways, but both of its branches end in `enter-mode normal`, so the
mode handed to the settle is `AppMode::Normal` in both directions. It
discriminates "settles at all" and "settles against the ending layout", and not
"settles from **either** mode".
This drives the production repro: from a records grid,
`apply_effects([SetAxis { "_Index", Axis::None }])` leaves
`layout.is_records_mode()` false with the mode still `RecordsNormal`, so the
`RecordsNormal` arm is live and not defensive. Both the records-ness of the
grid and the mode before the batch are asserted as preconditions, so a fixture
that stopped entering records mode fails loudly rather than proving nothing.
Injected fault: `if matches!(self.view_state.mode, AppMode::Normal)`. Fails by
name, `left: RecordsNormal, right: Normal`. It is the only test in the
workspace that fails.
### switching_views_across_records_ness_settles_against_the_view_switched_to
improvise-4dpj, gap 2 — raised by ChatGPT Codex against PR #91 and verified
correct on `main` before being tested. `SwitchViewAtCursor` emits
`[SwitchView(target), ChangeMode(Normal)]`, and this is a **different path**
from the one gap 1's siblings drive: that test toggles records mode explicitly,
where a view switch crosses records-ness only because it is auto-detected from
the axes (gotcha 1). Nothing in the batch says "records".
Driven in both directions, with the crossing asserted before and after so a
fixture that stopped crossing fails rather than passes. Injected fault: move
`settle_normal_mode()` back above `rebuild_layout()` at the end of
`apply_effects` — the exact regression PR #91's round 2 fixed. This test fails
by name; so does gap 1's, along with five pre-existing records-mode tests.
**Scope note, stated because an overclaim is worse than a gap**: this drives
the *effects* `SwitchViewAtCursor` emits, not the view-panel key path that
composes them. Reaching the panel needs a `view_panel_cursor` on a `CmdContext`
built in `command/cmd/panel.rs`, which is another wave's ground this round. The
command's own coverage lives there; what is unexercised is the two joined.
### the_layout_geometry_cases_are_every_wire_effect_of_that_kind
improvise-26dm. `layout_geometry_effects_actually_change_document_config` was
guarded by `assert_eq!(cases.len(), 13)`, which catches a case being *deleted*
from the list and never one being *absent* from it — and the two are written
identically as an `assert_eq` over a number. The adversarial review of PR #78
added a fourteenth `pub struct` declaring `LayoutGeometry`, pointed at by
nothing, and the whole suite stayed green.
improvise-5ubs's mechanism cannot reach it: an effect is a struct implementing
a trait, so serde has no derived enumeration to diff against. The **wire
vocabulary** does have one, and `model_effects` maps each `ModelCommand`
variant back to the effect it came from (gotcha 38), so the list is diffed
against the `LayoutGeometry` effects reachable through `ModelCommand`. The two
sets are equal today, which is the evidence the substitution lost nothing.
Injected fault: drop `TogglePruneEmpty` from `layout_geometry_cases()`. The old
count said `12 != 13`; this says
`right: [… "TogglePruneEmpty" …]` and **names the effect**.
**Residual, stated rather than closed**: this is total over the wire and not
over the effects. An effect declaring `LayoutGeometry` that no `ModelCommand`
names is still invisible — which is precisely PR #78's probe. Closing that
needs an enumeration of effects the compiler maintains, and none exists without
a proc macro or inventory-style registration.
The name reader is shared, not copied: `reduce.rs`'s `effect_type_name` and its
own `the_effect_name_reader_reads_all_three_debug_shapes` are what this leans
on, because a second reader here would be a derivation done twice and could
drift while one copy was repaired. It reads the leading identifier off the
derived `Debug`, `Effect` being a trait object that `type_name` cannot reach.
+83
View File
@@ -2604,6 +2604,89 @@ mod tests {
}
}
/// The settle's `RecordsNormal` arm, driven from the mode it names. The
/// `:` crossing above enters the settle in `Normal` both ways — the evidence.
#[test]
fn a_batch_that_leaves_records_settles_out_of_records_normal() {
use crate::view::Axis;
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_item("Region", "East").unwrap();
let mut app = App::new(wb, None);
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
.unwrap();
assert!(app.layout.is_records_mode(), "setup: the grid is records");
assert_eq!(
app.view_state.mode,
AppMode::RecordsNormal,
"setup: the mode handed to the settle must be RecordsNormal"
);
app.apply_effects(vec![Box::new(super::super::effect::SetAxis {
category: "_Index".into(),
axis: Axis::None,
})]);
assert!(
!app.layout.is_records_mode(),
"setup: the batch has to leave records, or nothing crossed"
);
assert_eq!(
app.view_state.mode,
AppMode::Normal,
"a batch that left records stayed in RecordsNormal (improvise-4dpj)"
);
}
/// The batch `SwitchViewAtCursor` emits, both ways across records-ness:
/// records-ness is auto-detected from the axes, so a view switch moves it.
#[test]
fn switching_views_across_records_ness_settles_against_the_view_switched_to() {
for to_records in [false, true] {
let mut wb = Workbook::new("T");
wb.add_category("Region").unwrap();
wb.add_item("Region", "East").unwrap();
let mut app = App::new(wb, None);
// `R` creates and enters `_Records`; the pivot view it left is the
// other end of every crossing below.
app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
.unwrap();
let records_view = app.view_state.active_view.clone();
let (from, to) = match to_records {
true => ("Default", records_view.as_str()),
false => (records_view.as_str(), "Default"),
};
app.apply_effects(vec![Box::new(super::super::effect::SwitchView(
from.into(),
))]);
assert_eq!(
app.layout.is_records_mode(),
!to_records,
"setup: the switch has to CROSS, so it must start on the other side"
);
app.apply_effects(vec![
Box::new(super::super::effect::SwitchView(to.into())),
super::super::effect::change_mode(AppMode::Normal),
]);
assert_eq!(
app.layout.is_records_mode(),
to_records,
"setup: the switch has to land, or nothing crossed"
);
let expected = match to_records {
true => AppMode::RecordsNormal,
false => AppMode::Normal,
};
assert_eq!(
app.view_state.mode, expected,
"the batch settled against the view it LEFT, not the one it \
switched to (improvise-4dpj)"
);
}
}
/// A `:` line that really runs settles through `ReturnToNormalUnlessMoved`,
/// which an empty line never reaches. See the evidence.
#[test]