From 5e127fe9bfcb61f009a0253ed08f919d0db4b1d5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 19:34:19 -0700 Subject: [PATCH] test(project): guard the formula a target ITEM's deletion takes with it (improvise-en4s) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Dirty::of(ModelChange::CellData)` reports `formulas: true`. That was deliberate over-reporting in the cheap direction, costing one redundant `Projection` and buying nothing — until improvise-g3no made `Model::remove_item` drop the formulas targeting the item. `effect::RemoveItem` is `CellData`, so a deletion can now take a formula out of the document, and tightening that arm would leave every connected client's formula panel naming a formula the document no longer has, forever, with nothing in the stream to correct it. `removing_a_category_empties_the_clients_formula_panel` covers the `LayoutGeometry` twin only. This is its item-level sibling. The setup is deliberately not a mirror of the sibling's. Written that way — connect, `AddFormula`, `RemoveItem` — it does fail under the rival, but on the SETUP assertion, because `AddFormula` is itself `CellData` and with the arm tightened the formula never reaches the client at all. That answers whether `AddFormula`'s projection arrived, which is the neighbouring question. So the formula is added before `Sessions::connect` and the client takes it from the bootstrap seed, which no `Dirty` gates; the rival then lands on the assertion that means something. The sibling does not have this problem against its own rival and is left alone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 41 +++++++++++++++++ crates/improvise-command/src/ui/project.rs | 53 +++++++++++++++++++++- 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 48a66083..db3658a8 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4591,3 +4591,44 @@ a time. A first version of this test called `apply_effects` per command; each single-`Move` batch reverses to the same cursor, so the reversed fold was invisible and the test passed against it. One batch is also the shape a `Binding::Sequence` really has. + +## `ui/project.rs` — a formula lost with its target ITEM (improvise-en4s) + +`Dirty::of(ModelChange::CellData)` reports `formulas: true`. That was written as +deliberate over-reporting in the cheap direction — a cell write cannot change a +formula, so the entry cost one redundant `Projection` and bought nothing. +improvise-g3no made `Model::remove_item` drop the formulas targeting the item, +and `effect::RemoveItem` is `CellData`, so a deletion really can take a formula +out of the document. Tightening that arm would leave every connected client's +formula panel showing a formula the document no longer has, **forever**, with +nothing in the stream to correct it — gotcha 35's expensive direction, and the +exact failure that put `formulas` on the `LayoutGeometry` arm in the first place. + +### removing_a_formulas_target_item_empties_the_clients_formula_panel + +The item-level twin of `removing_a_category_empties_the_clients_formula_panel`, +which covers the `LayoutGeometry` arm only. + +Fault injected: `formulas: false` on the `CellData` arm of `Dirty::of`. The test +fails on the final assertion — the client still names `Total = Revenue` after the +document has dropped it. + +**The setup had to be restructured so it could fail there.** Written the obvious +way, mirroring its sibling — connect, then `AddFormula`, then `RemoveItem` — it +does fail under the rival, but at the *setup* assertion "the client must be +holding the formula before it can lose it", because `AddFormula` is **itself** +`CellData`, so with the arm tightened the formula never reaches the client at +all. That answers "did `AddFormula`'s projection arrive", which is the +neighbouring question; the property is about a *deletion*'s projection. So the +formula is added **before** `Sessions::connect` and the client picks it up from +the bootstrap seed, which no `Dirty` gates. Both assertions are then reachable +independently and the rival lands on the one that means something. + +The sibling does not have this problem against its *own* rival: `formulas: false` +on the `LayoutGeometry` arm leaves `AddFormula` delivering normally, so its setup +holds and only its final assertion moves. It is left alone. + +The test also asserts `RemoveItem::kind()` is `Model(ModelChange::CellData)` +before applying it, so reclassifying that effect fails here rather than silently +turning this into a test of the `LayoutGeometry` arm that its sibling already +covers. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 0d19ac29..7d33657b 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -1124,7 +1124,9 @@ fn fresh_view(app: &App) -> ViewState { mod tests { use super::*; use crate::model::cell::{CellKey, CellValue}; - use crate::ui::effect::{AddFormula, Effect, RemoveCategory, SetAxis, SetCell, ViewTarget}; + use crate::ui::effect::{ + AddFormula, Effect, RemoveCategory, RemoveItem, SetAxis, SetCell, ViewTarget, + }; use crate::workbook::Workbook; use improvise_protocol::{ BasisCache, CategoryTreeCache, DocumentCache, FormulaCache, TileBarCache, ViewListCache, @@ -1696,6 +1698,55 @@ mod tests { ); } + /// The item-level twin of the test above. Evidence: + /// `context/test-evidence/command.md`. + #[test] + fn removing_a_formulas_target_item_empties_the_clients_formula_panel() { + let mut app = app_with_data(); + apply( + &mut app, + vec![Box::new(AddFormula { + raw: "Total = Revenue".to_string(), + target_category: "_Measure".to_string(), + })], + ); + + // The formula predates the session, so the client holds it from the + // BOOTSTRAP SEED and not from a CellData projection (improvise-en4s). + let mut sessions = Sessions::default(); + let (_, snapshot) = sessions.connect(&app, 100, 100); + let mut client = ClientCaches::default(); + client.fold(&snapshot.seed); + assert_eq!( + client.formulas.formulas, + vec!["Total = Revenue".to_string()], + "the client must be holding the formula before it can lose it" + ); + + let effect = RemoveItem { + category: "_Measure".to_string(), + item: "Total".to_string(), + }; + assert_eq!( + effect.kind(), + EffectKind::Model(ModelChange::CellData), + "this test is about the CellData arm; reclassify it and it stops being that" + ); + let dirty = apply(&mut app, vec![Box::new(effect)]); + for (_, projections) in sessions.project_all(&app, dirty) { + client.fold(&projections); + } + assert!( + app.model_state.workbook.model.formulas().is_empty(), + "the document dropped the formula with its target item" + ); + assert_eq!( + client.formulas.formulas, + Vec::::new(), + "the client is still showing a formula the document no longer has" + ); + } + /// Bootstrap is a compaction, not a replay: a late client folding only the seed /// lands where one that followed the whole stream is. #[test]