From 05a28b0d45f4a9730c5015a832c88fea4b2c21fe Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:38:35 -0700 Subject: [PATCH] Point the geometry test at the production hash, and test both abort arms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-1xf4: `layout_geometry_effects_actually_change_view_config` hand-listed 6 of the 13 LayoutGeometry effects and measured them against `view_config_fingerprint`, a THIRD fingerprint existing only in test code beside the production `document_config` and `view_shape`. Since PR #70 decides `model_state.dirty` for LayoutGeometry solely from `document_config`, an effect writing something the test-only hash sees and `document_config` does not would pass and never mark the document dirty — i.e. never be saved. It is now `layout_geometry_effects_actually_change_document_config`, measured against `document_config` itself and listing all 13, counted off the `impl Effect` blocks rather than trusted. Two fixture facts the rewrite exposed and the old six never could: `ShowItem` on a visible item and `ToggleGroup` on a nonexistent group are no-ops, so `grouped_app` supplies a hidden item, a real group and a second view. `view_config_fingerprint` SURVIVES, deliberately, and is not a third derivation any more: it is used only by the NEGATIVE arm (`view_only_effects_leave_shared_state_alone`), where a hash stricter than `document_config` is what that test wants. The doc comment says so. 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 end-of-batch is safe for `layout_version` specifically was written into context/code/command.md and never tested. Both arms now are: a batch that reshaped before aborting must bump, one that aborted before reshaping must not. Proved independently — bumping unconditionally fails only the second, skipping `observe_reshape` on abort fails only the first. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-command/src/ui/app.rs | 2 +- crates/improvise-command/src/ui/effect.rs | 107 ++++++++++++++++++++-- 2 files changed, 100 insertions(+), 9 deletions(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index ecaf320b..60776c0d 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -82,7 +82,7 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap { /// Everything a `LayoutGeometry` effect can write that the file would carry. Not the /// cell data — `record_fingerprints` answers for that — so it is cheap per batch. -fn document_config(workbook: &Workbook) -> u64 { +pub(crate) fn document_config(workbook: &Workbook) -> u64 { use std::hash::{Hash, Hasher}; let mut h = std::collections::hash_map::DefaultHasher::new(); for (name, cat) in &workbook.model.categories { diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 3ba27903..babc9d6a 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1326,7 +1326,9 @@ mod tests { serde_json::to_string(&app.model_state.workbook.model).expect("model serializes") } - /// Serialized view configuration (axes, pages, hidden items, format). + /// 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). fn view_config_fingerprint(app: &App) -> String { serde_json::to_string(&app.model_state.workbook.views).expect("views serialize") } @@ -1482,38 +1484,80 @@ mod tests { } } - /// Effects declaring `LayoutGeometry` must change view configuration — - /// the thing that makes every client re-lay out its grid. + /// Effects declaring `LayoutGeometry` must move `document_config` — the + /// production hash `model_state.dirty` is decided by (improvise-1xf4). #[test] - fn layout_geometry_effects_actually_change_view_config() { + fn layout_geometry_effects_actually_change_document_config() { let cases: Vec> = vec![ Box::new(AddCategory("Region".into())), Box::new(RemoveCategory("Month".into())), + Box::new(CreateView("Second".into())), + Box::new(DeleteView("Default".into())), Box::new(SetAxis { category: "Type".into(), axis: Axis::Page, }), + Box::new(SetPageSelection { + category: "Type".into(), + item: "Clothing".into(), + }), + Box::new(ToggleGroup { + category: "Type".into(), + group: "Wearables".into(), + }), + Box::new(HideItem { + category: "Type".into(), + item: "Food".into(), + }), + Box::new(ShowItem { + category: "Type".into(), + item: "Clothing".into(), + }), Box::new(TransposeAxes), + Box::new(CycleAxis("Type".into())), Box::new(SetNumberFormat(",.3f".into())), Box::new(TogglePruneEmpty), ]; + // All thirteen, counted off the `impl Effect` blocks rather than trusted: + // a fourteenth declaring LayoutGeometry must come here and say so. + assert_eq!(cases.len(), 13, "the LayoutGeometry effects are thirteen"); for effect in cases { assert_eq!( effect.kind(), EffectKind::Model(ModelChange::LayoutGeometry), "{effect:?} is in the LayoutGeometry case list" ); - let mut app = test_app(); - let before = view_config_fingerprint(&app); + let mut app = grouped_app(); + let before = crate::ui::app::document_config(&app.model_state.workbook); effect.apply(&mut app); assert_ne!( - view_config_fingerprint(&app), + crate::ui::app::document_config(&app.model_state.workbook), before, - "{effect:?} declares LayoutGeometry but changed no view config" + "{effect:?} declares LayoutGeometry but did not move document_config, \ + so a batch carrying only it would never mark the document dirty" ); } } + /// `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. + fn grouped_app() -> App { + let mut app = test_app(); + { + let wb = &mut app.model_state.workbook; + wb.model + .category_mut("Type") + .unwrap() + .add_item_in_group("Hats", "Wearables"); + wb.create_view("Spare"); + wb.switch_view("Default").unwrap(); + wb.active_view_mut().hide_item("Type", "Clothing"); + } + app.rebuild_layout(); + app + } + // ── Model mutation effects ────────────────────────────────────────── #[test] @@ -1729,6 +1773,53 @@ 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. + #[test] + fn an_aborted_batch_bumps_layout_version_for_what_it_did_run() { + let mut app = test_app(); + let before = app.layout_version(); + // The reshape lands, THEN the abort. `observe_reshape` runs after the + // break, so it must still see the axis move. + app.apply_effects(vec![ + Box::new(SetAxis { + category: "Type".into(), + axis: Axis::Page, + }), + Box::new(AbortChain), + Box::new(TogglePruneEmpty), + ]); + assert!(app.last_batch_aborted(), "the fixture must actually abort"); + assert_eq!( + app.layout_version(), + before + 1, + "a batch that reshaped before aborting still moved the grid, and a \ + client composing against the old version must be told" + ); + } + + #[test] + fn an_aborted_batch_bumps_nothing_for_effects_it_never_ran() { + let mut app = test_app(); + let before = app.layout_version(); + // The abort comes FIRST, so the reshaping effect behind it never runs. + app.apply_effects(vec![ + Box::new(AbortChain), + Box::new(SetAxis { + category: "Type".into(), + axis: Axis::Page, + }), + ]); + assert!(app.last_batch_aborted(), "the fixture must actually abort"); + assert_eq!( + app.layout_version(), + before, + "nothing reshaped, so no client's basis went stale — an over-bump \ + here is a rejection somebody did not earn" + ); + } + #[test] fn view_back_and_forward() { let mut app = test_app();