Merge pull request #213 from fiddlerwoaroof/wave/refuse-fu

Pin every refusal site, and keep a bad formula in its editor (improvise-v4hg, 514o, 0v8l)

context/code/command.md

@@ -592,10 +592,23 @@ the client was told nothing at all.
   key-path batch assumes the failed one happened. `refuse_and_stop` does it for
   `LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and
   `import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its
-  own inline abort (gotcha 21). The item, formula and view effects only refuse:
-  the frame is rolled back by the refusal alone. On the key path, a failed
-  `AddItem`, `AddFormula` or view edit no longer stops the rest of its batch,
-  which is what it did before bok4.
+  own inline abort (gotcha 21). The item and view effects, and `AddFormula`'s
+  missing-category arm, only refuse: the frame is rolled back by the refusal
+  alone. On the key path, a failed `AddItem` or view edit no longer stops the
+  rest of its batch, which is what it did before bok4.
+- **`AddFormula`'s parse-error arm also stops (improvise-v4hg).** It is the one
+  item-or-formula failure a key-path batch was found to depend on. Enter in
+  `FormulaEdit` is `[SetStatus, AddFormula, ChangeMode(FormulaPanel),
+  ClearBuffer("formula")]`, and the last two assume the commit worked. Without
+  the stop, `Total = (` was reported and then the editor closed and the typed
+  text was cleared. With it, the user stays in `FormulaEdit` with the text, and
+  the error is drawn beside the prompt. `fit-measure` emits `AddFormula` last, so
+  there is nothing after it to stop. The missing-category arm is reachable only
+  from a wire frame or a `:` line naming another category, and nothing in either
+  depends on it, so it does not stop. PR #181's review searched for an
+  `AddItem` case the same way and found none, so `AddItem` does not stop either.
+  An abort is added where a later effect depends on the failed one, never for
+  symmetry.
 
 In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame
 is undone whole, and the client is told, which is the channel the narrated
@@ -615,8 +628,17 @@ Consequences:
   refusal, not by `status_msg`. The frame's rollback also rolls the host's
   status line back.
 - **Every refusal site has a test that fails when it is reverted** to a
-  status-only write (improvise-9fpl). Most are one table,
-  `every_failing_model_command_refuses_its_whole_frame`. That matters because
+  status-only write (improvise-9fpl, improvise-0v8l). Two tables carry most of
+  them. `every_failing_model_command_refuses_its_whole_frame` covers the wire.
+  `every_refusal_site_refuses_its_failure_and_only_its_failure` covers the key
+  path and the `:` line, which is what the headless exit code reads. The wire
+  table cannot see the refusal half of a `refuse_and_stop` site, because
+  `Host::mutating_frame` rejects on `refused || aborted`, and the abort alone
+  suffices. So each `refuse_and_stop` site needs a case in both tables:
+  the key-path table for its refusal, and
+  `a_failed_load_or_import_does_not_run_the_quit_after_it` for its abort
+  (improvise-514o). Each key-path case has a succeeding twin, so a site
+  that refused unconditionally would fail too. That matters because
   these are exactly the lines a merge conflict could silently drop.
 - **Not covered:** a no-op *inside* a view that does resolve. Examples are
   `SetAxis` on a category the view does not have, `HideItem` of an absent

context/test-evidence/command.md

@@ -5316,6 +5316,81 @@ before `refuse_and_stop` was applied to its arms. For `load`, the injected fault
 is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test
 then fails with "`load absent.improv . q` quit having loaded nothing".
 
+#### improvise-514o: every `refuse_and_stop` arm
+
+The case list above covered `load` and the JSON cannot-read arm only. PR #181's
+review reverted each `refuse_and_stop` to `app.refuse` one at a time, which
+keeps the refusal and drops the abort. Five `ImportJsonHeadless` arms failed
+nothing: CSV cannot-read, CSV parse, JSON parse, array path and build. The wire
+table `every_failing_model_command_refuses_its_whole_frame` cannot see the
+abort, because `Host::mutating_frame` rejects on `refused || aborted`, and the
+refusal alone suffices. The list now has one case per arm, using the fixtures
+the wire table uses, which now come from `ui::host::import_fixture_storage`.
+Each case also asserts the arm's own status text, so a fixture that failed in a
+different arm cannot stand in for the one it names. The same scripted sweep,
+rerun after the change, fails this test at all seven sites. Before, it failed
+at two.
+
+### ui::app::tests::a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text
+
+improvise-v4hg. In `FormulaEdit`, Enter on `Total = (` reported the parse error
+and then did what a successful commit does: `CommitBuffer`'s
+`ChangeMode(FormulaPanel)` closed the editor and the keymap's welded
+`ClearBuffer("formula")` cleared the text. The user saw the error with their
+formula gone. Driven through `App::handle_key` from `FormulaPanel`: `a`, the
+characters, then Enter. The test failed first with "got FormulaPanel". The fix is
+`refuse_and_stop` on the parse-error arm, and a `Binding::Sequence` is one
+effect vec, so the stop also covers the `ClearBuffer` step.
+
+The mode assertion alone would be vacuous against a fix that stops the mode
+change but still loses the text. That rival (clear the `formula` buffer, then
+`refuse_and_stop`) was run and fails "and must keep what they typed" with
+`left: None`. The control, `Total = 1`, asserts the ordinary path still stores
+the formula and closes the editor, so a stop on every arm would fail it. After
+the fix, the abort sweep (`refuse_and_stop` back to `app.refuse`) fails this
+test at that site.
+
+### ui::app::tests::every_refusal_site_refuses_its_failure_and_only_its_failure
+
+improvise-0v8l. PR #185 split refusals (`App::refuse`, which the headless exit
+code reads) from reports (`set_status`), and most of the refusal sites were
+unpinned. The ticket counted 23 of 27. At gh/main `608ba6a` (after #181's wire
+table), a script (`/tmp/refuse-fu/sweep.py`, not committed) reverted each of
+the **32** production refusal sites in `ui/effect.rs` and `command/cmd/*.rs`
+alone to a status-only write, and ran `cargo test --workspace --no-fail-fast`.
+For `effect::refuse(x)` the revert is `effect::set_status(x)`. For
+`app.refuse(x)` it is `app.view_state.status_msg = x.into()`. For
+`refuse_and_stop` it is the status write with the abort kept. **13 of 32 failed
+a test. 19 survived:** `AddCategory` at the limit, `apply_coordinate_rename`,
+both `StartImportWizard` arms, the five `ImportJsonHeadless` arms the wire
+table cannot see (the abort alone rejects the frame), `rename_record_coordinate`,
+`CommitExport`, both `ViewNavigate` arms, both `AddRecordRow` arms, both
+`EditOrDrill` arms, `SearchNavigate` and `ExecuteCommand`.
+
+This table has one case per survivor. Each case drives the failure through a
+`:` line (`run_line`, which does what `improvise cmd` does, one batch per
+segment) or through Enter in the site's own minibuffer mode. It asserts that the
+batch refused and that the status names the site's own reason. Each case also
+has a **succeeding twin** that must not refuse, so a site that refused
+unconditionally fails too. That rival (`AddCategory` refusing on `Ok` as well)
+was run and fails "its succeeding twin was refused". Rerun after the change,
+the sweep catches **32 of 32**. The abort half, 10 sites before v4hg's eleventh,
+went from 5 caught to 11 of 11.
+
+Three fixture choices. The second was found when the first run failed: the input meant to be refused was accepted.
+
+- **`AddCategory` past the limit** needs eleven categories and a two-segment
+  line. The first `add-category` fills the twelfth slot and the second is
+  refused, so the succeeding twin is the same fixture with one segment.
+- **`add-record-row` onto an existing row** must page the category on the
+  view the client is actually on. Records mode switches to `_Records`, so
+  writing `Workbook::active_view_mut` (the document's default) changed a view
+  nobody was looking at, and the case "succeeded". It goes through
+  `App::view_mut_of(&ViewTarget::Active)` (gotcha 29).
+- **The drill rename** edits record 1, not record 0, following the
+  cursor-at-index-0 rule in the wave agent rules. Nothing failed here: this is
+  a precaution, not a finding.
+
 ### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename
 
 improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and

crates/improvise-command/src/ui/app.rs

@@ -3028,16 +3028,34 @@ mod tests {
     fn a_failed_load_or_import_does_not_run_the_quit_after_it() {
         use improvise_protocol::keymap::Keymap;
 
-        for (command, path) in [("load", "absent.improv"), ("import-json", "absent.json")] {
+        // One case per `refuse_and_stop` site (improvise-514o).
+        let cases: [(&str, &[&str], &str); 7] = [
+            ("load", &["absent.improv"], "Load error"),
+            ("import-json", &["absent.json"], "Cannot read 'absent.json'"),
+            ("import-json", &["absent.csv"], "Cannot read 'absent.csv'"),
+            ("import-json", &["bad.csv"], "CSV error"),
+            ("import-json", &["bad.json"], "JSON parse error"),
+            (
+                "import-json",
+                &["nested.json", "M", "nope"],
+                "No array at path",
+            ),
+            ("import-json", &["measures.json"], "Import error"),
+        ];
+        for (command, args, arm) in cases {
+            let path = args.join(" ");
             let mut app = App::new(Workbook::new("T"), None);
-            app.storage = Box::new(crate::storage::MemoryFs::new());
+            app.storage = Box::new(crate::ui::host::import_fixture_storage());
             let key = KeyCode::Char('x');
             let mods = KeyModifiers::NONE;
             let mut km = Keymap::new();
             km.bind_seq(
                 key,
                 mods,
-                vec![(command, vec![path.to_string()]), ("force-quit", vec![])],
+                vec![
+                    (command, args.iter().map(|a| a.to_string()).collect()),
+                    ("force-quit", vec![]),
+                ],
             );
             let effects = {
                 let ctx = app.cmd_context(key, mods);
@@ -3047,8 +3065,8 @@ mod tests {
             app.apply_effects(effects);
 
             assert!(
-                app.last_batch_refused(),
-                "setup: `{command}` must actually have failed, got {:?}",
+                app.last_batch_refused() && app.view_state.status_msg.contains(arm),
+                "setup: `{command} {path}` must fail in the {arm:?} arm, got {:?}",
                 app.view_state.status_msg
             );
             assert_ne!(
@@ -3059,6 +3077,392 @@ mod tests {
         }
     }
 
+    /// improvise-v4hg: Enter on a formula that does not parse threw the typed text
+    /// away. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text() {
+        let type_and_commit = |text: &str| {
+            let mut app = two_col_model();
+            app.view_state.mode = AppMode::FormulaPanel;
+            let press = |app: &mut App, code| {
+                app.handle_key(KeyEvent::new(code, KeyModifiers::NONE))
+                    .unwrap()
+            };
+            press(&mut app, KeyCode::Char('a'));
+            assert!(
+                matches!(app.view_state.mode, AppMode::FormulaEdit { .. }),
+                "setup: `a` in the formula panel opens the editor"
+            );
+            for c in text.chars() {
+                press(&mut app, KeyCode::Char(c));
+            }
+            press(&mut app, KeyCode::Enter);
+            app
+        };
+
+        // The control: a formula that parses is stored and the editor closes.
+        let ok = type_and_commit("Total = 1");
+        assert_eq!(ok.view_state.mode, AppMode::FormulaPanel);
+        assert!(
+            ok.model_state
+                .workbook
+                .model
+                .formulas()
+                .iter()
+                .any(|f| f.target == "Total"),
+            "the control formula must land"
+        );
+
+        let bad = type_and_commit("Total = (");
+        assert!(
+            bad.last_batch_refused() && bad.view_state.status_msg.contains("Formula error"),
+            "the parse error must be reported, got {:?}",
+            bad.view_state.status_msg
+        );
+        assert!(
+            matches!(bad.view_state.mode, AppMode::FormulaEdit { .. }),
+            "a refused formula must leave the user in the editor, got {:?}",
+            bad.view_state.mode
+        );
+        assert_eq!(
+            bad.view_state.buffers.get("formula").map(String::as_str),
+            Some("Total = ("),
+            "and must keep what they typed"
+        );
+    }
+
+    /// Run a `:` line the way `improvise cmd` does, one batch per segment; true
+    /// when any segment refused, which is what its exit code reads.
+    fn run_line(app: &mut App, line: &str) -> bool {
+        let mut refused = false;
+        for segment in crate::command::parse::segments(line) {
+            let effects = {
+                let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE);
+                crate::command::parse::parse_segment_interactive(segment, &ctx)
+                    .unwrap_or_else(|e| panic!("`{segment}` must parse: {e}"))
+                    .map(|cmd| cmd.execute(&ctx))
+                    .unwrap_or_default()
+            };
+            app.apply_effects(effects);
+            refused |= app.last_batch_refused();
+        }
+        refused
+    }
+
+    /// Commit `text` in a minibuffer mode with Enter; true when that batch refused.
+    fn commit_in(app: &mut App, mode: AppMode, buffer: &str, text: &str) -> bool {
+        app.view_state.mode = mode;
+        app.view_state
+            .buffers
+            .insert(buffer.to_string(), text.to_string());
+        app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
+            .unwrap();
+        app.last_batch_refused()
+    }
+
+    fn with_import_fixtures(mut app: App) -> App {
+        app.storage = Box::new(crate::ui::host::import_fixture_storage());
+        app
+    }
+
+    /// Eleven regular categories: one short of the limit.
+    fn one_category_short_of_the_limit() -> App {
+        let mut wb = Workbook::new("T");
+        for n in 0..11 {
+            wb.add_category(format!("C{n}")).unwrap();
+        }
+        App::new(wb, None)
+    }
+
+    /// Stage one coordinate rename in a drill over the records fixture, then commit it.
+    fn commit_drill_rename(app: &mut App, to: &str) -> bool {
+        let records = app.layout.records.clone().expect("records mode");
+        let mut pending_edits = HashMap::new();
+        pending_edits.insert(
+            (1, crate::view::RecordCol::Category("Region".into())),
+            to.into(),
+        );
+        app.drill_state = Some(DrillState {
+            records,
+            pending_edits,
+        });
+        app.apply_effects(vec![Box::new(crate::ui::effect::ApplyAndClearDrill)]);
+        app.last_batch_refused()
+    }
+
+    /// Records mode with `Region` paged to `page`: `add-record-row` then names
+    /// the row whose coordinates are exactly that page.
+    fn records_paged_to(app: &mut App, page: &str) {
+        app.model_state.workbook.add_item("Region", "West").unwrap();
+        let view = app
+            .view_mut_of(&ViewTarget::Active)
+            .expect("the active view");
+        view.set_axis("Region", crate::view::Axis::Page);
+        view.set_page_selection("Region", page);
+        app.rebuild_layout();
+    }
+
+    /// A grid with a regular category on None, so every cell is an aggregate.
+    fn aggregated_grid(with_items: bool) -> App {
+        let mut wb = Workbook::new("T");
+        for cat in ["Row", "Col", "Hidden"] {
+            wb.add_category(cat).unwrap();
+            if with_items {
+                wb.add_item(cat, "A").unwrap();
+            }
+        }
+        wb.active_view_mut()
+            .set_axis("Hidden", crate::view::Axis::None);
+        let mut app = App::new(wb, None);
+        app.rebuild_layout();
+        app
+    }
+
+    /// `Region` down, `_Measure` across: `Revenue` at column 0, then `Total`,
+    /// which a formula owns, at column 1.
+    fn a_formula_column() -> App {
+        let mut wb = Workbook::new("T");
+        wb.add_category("Region").unwrap();
+        wb.add_item("Region", "North").unwrap();
+        wb.add_item("_Measure", "Revenue").unwrap();
+        wb.active_view_mut()
+            .set_axis("_Measure", crate::view::Axis::Column);
+        let mut app = App::new(wb, None);
+        app.apply_effects(vec![Box::new(crate::ui::effect::AddFormula {
+            raw: "Total = Revenue * 2".into(),
+            target_category: "_Measure".into(),
+        })]);
+        app
+    }
+
+    fn with_a_cell(mut app: App) -> App {
+        app.model_state.workbook.model.set_cell(
+            CellKey::new(vec![("Row".into(), "A".into()), ("Col".into(), "X".into())]),
+            CellValue::Number(42.0),
+        );
+        app.rebuild_layout();
+        app
+    }
+
+    fn with_a_second_view(mut app: App) -> App {
+        app.model_state.workbook.create_view("Second");
+        app
+    }
+
+    fn switch_to_second(app: &mut App) {
+        app.apply_effects(vec![Box::new(crate::ui::effect::SwitchView(
+            "Second".into(),
+        ))]);
+    }
+
+    /// improvise-0v8l: one case per refusal site no other test pinned, each with a
+    /// succeeding twin. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn every_refusal_site_refuses_its_failure_and_only_its_failure() {
+        type Setup = fn() -> App;
+        type Drive = fn(&mut App) -> bool;
+        let cases: Vec<(&str, Setup, Drive, &str, Setup, Drive)> = vec![
+            (
+                "add-category past the limit",
+                one_category_short_of_the_limit,
+                |app| run_line(app, "add-category A . add-category B"),
+                "Maximum of 12 categories",
+                one_category_short_of_the_limit,
+                |app| run_line(app, "add-category A"),
+            ),
+            (
+                "a drill commit that empties a coordinate",
+                records_app_for_drill,
+                |app| commit_drill_rename(app, ""),
+                crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY,
+                records_app_for_drill,
+                |app| commit_drill_rename(app, "West"),
+            ),
+            (
+                "import of a JSON that does not parse",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import bad.json"),
+                "JSON parse error",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import good.json"),
+            ),
+            (
+                "import of an absent file",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import absent.json"),
+                "Cannot read file",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import good.json"),
+            ),
+            (
+                "import-json of an absent CSV",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json absent.csv"),
+                "Cannot read 'absent.csv'",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json good.json"),
+            ),
+            (
+                "import-json of a CSV that is not text",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json bad.csv"),
+                "CSV error",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json good.json"),
+            ),
+            (
+                "import-json of JSON that does not parse",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json bad.json"),
+                "JSON parse error",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json good.json"),
+            ),
+            (
+                "import-json at a path with no array",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json nested.json M nope"),
+                "No array at path",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json good.json"),
+            ),
+            (
+                "import-json that builds no model",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json measures.json"),
+                "Import error",
+                || with_import_fixtures(two_col_model()),
+                |app| run_line(app, "import-json good.json"),
+            ),
+            (
+                "a records coordinate edited to nothing",
+                records_model_with_two_rows,
+                |app| {
+                    app.cursor_mut().selected = (1, 0);
+                    commit_in(app, AppMode::records_editing(), "edit", "")
+                },
+                crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY,
+                records_model_with_two_rows,
+                |app| {
+                    app.cursor_mut().selected = (1, 0);
+                    commit_in(app, AppMode::records_editing(), "edit", "West")
+                },
+            ),
+            (
+                "an export with no path",
+                || with_import_fixtures(two_col_model()),
+                |app| commit_in(app, AppMode::export_prompt(), "export", ""),
+                "not exported",
+                || with_import_fixtures(two_col_model()),
+                |app| commit_in(app, AppMode::export_prompt(), "export", "out.csv"),
+            ),
+            (
+                "view-forward with nowhere to go",
+                || with_a_second_view(two_col_model()),
+                |app| run_line(app, "view-forward"),
+                "No forward view",
+                || with_a_second_view(two_col_model()),
+                |app| {
+                    switch_to_second(app);
+                    run_line(app, "view-back");
+                    run_line(app, "view-forward")
+                },
+            ),
+            (
+                "view-back with nowhere to go",
+                || with_a_second_view(two_col_model()),
+                |app| run_line(app, "view-back"),
+                "No previous view",
+                || with_a_second_view(two_col_model()),
+                |app| {
+                    switch_to_second(app);
+                    run_line(app, "view-back")
+                },
+            ),
+            (
+                "add-record-row outside records mode",
+                two_col_model,
+                |app| run_line(app, "add-record-row"),
+                "only works in records mode",
+                records_model_with_two_rows,
+                |app| run_line(app, "add-record-row"),
+            ),
+            (
+                "add-record-row onto a row that is already there",
+                records_model_with_two_rows,
+                |app| {
+                    records_paged_to(app, "South");
+                    run_line(app, "add-record-row")
+                },
+                "already here",
+                records_model_with_two_rows,
+                |app| {
+                    records_paged_to(app, "West");
+                    run_line(app, "add-record-row")
+                },
+            ),
+            (
+                "edit-or-drill on an aggregate grid with no cell",
+                || aggregated_grid(false),
+                |app| run_line(app, "edit-or-drill editing"),
+                "No cell at cursor",
+                || aggregated_grid(true),
+                |app| run_line(app, "edit-or-drill editing"),
+            ),
+            (
+                "edit-or-drill on a cell a formula owns",
+                a_formula_column,
+                |app| {
+                    app.cursor_mut().selected = (0, 1);
+                    run_line(app, "edit-or-drill editing")
+                },
+                "computed by a formula",
+                a_formula_column,
+                |app| {
+                    app.cursor_mut().selected = (0, 0);
+                    run_line(app, "edit-or-drill editing")
+                },
+            ),
+            (
+                "a search with no matches",
+                || with_a_cell(two_col_model()),
+                |app| {
+                    app.view_state.search_query = "zzz".into();
+                    run_line(app, "search-navigate forward")
+                },
+                "No matches",
+                || with_a_cell(two_col_model()),
+                |app| {
+                    app.view_state.search_query = "42".into();
+                    run_line(app, "search-navigate forward")
+                },
+            ),
+            (
+                "a : line that does not parse",
+                two_col_model,
+                |app| commit_in(app, AppMode::command_mode(), "command", "no-such-command"),
+                ":no-such-command",
+                two_col_model,
+                |app| commit_in(app, AppMode::command_mode(), "command", "add-category Z"),
+            ),
+        ];
+        for (what, fail_setup, fails, says, ok_setup, succeeds) in cases {
+            let mut app = fail_setup();
+            let refused = fails(&mut app);
+            assert!(
+                refused && app.view_state.status_msg.contains(says),
+                "{what}: expected a refusal saying {says:?}, got refused={refused} {:?}",
+                app.view_state.status_msg
+            );
+            let mut app = ok_setup();
+            assert!(
+                !succeeds(&mut app),
+                "{what}: its succeeding twin was refused: {:?}",
+                app.view_state.status_msg
+            );
+        }
+    }
+
     /// A prefix is consumed whether or not the next key matches, so a stray key
     /// after `g` does nothing rather than acting as a fresh Normal binding.
     #[test]

crates/improvise-command/src/ui/effect.rs

@@ -45,8 +45,8 @@ pub enum ViewTarget {
     Named(String),
 }
 
-/// Refuse AND stop the batch, for a failed document swap: what follows it in
-/// a `:` line assumes it happened, so `load x . q` must not quit (cz8d).
+/// Refuse AND stop the batch, where what follows assumes the change happened:
+/// `load x . q` must not quit, a bad formula must not close its editor (cz8d).
 fn refuse_and_stop(app: &mut App, why: impl Into<String>) {
     app.refuse(why);
     app.abort_effects = true;
@@ -219,9 +219,9 @@ impl Effect for AddFormula {
                 }
                 app.model_state.workbook.model.add_formula(formula);
             }
-            Err(e) => {
-                app.refuse(format!("Formula error: {e}"));
-            }
+            // Stop too: what follows it on the key path closes the editor and
+            // clears the buffer, which would throw the user's text away (v4hg).
+            Err(e) => refuse_and_stop(app, format!("Formula error: {e}")),
         }
     }
     fn kind(&self) -> EffectKind {

crates/improvise-command/src/ui/host.rs

@@ -266,6 +266,27 @@ impl Host {
     }
 }
 
+/// One file per way `ImportJsonHeadless` can fail, plus one that imports; the
+/// `absent.*` paths are left unwritten. Shared by the frame and key-path tests.
+#[cfg(test)]
+pub(crate) fn import_fixture_storage() -> crate::storage::MemoryFs {
+    let storage = crate::storage::MemoryFs::new();
+    let files: [(&str, &[u8]); 5] = [
+        // Every field numeric, so no category is accepted and the build fails.
+        ("measures.json", br#"[{"x": 1}, {"x": 2}]"#),
+        ("bad.json", b"{not json"),
+        ("nested.json", br#"{"data": [{"Region": "North"}]}"#),
+        // A record that is not UTF-8 is a CSV read error, not a warning.
+        ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"),
+        ("good.json", br#"[{"Region": "North", "Revenue": 1}]"#),
+    ];
+    for (name, bytes) in files {
+        crate::storage::Storage::write(&storage, std::path::Path::new(name), bytes)
+            .expect("fixture writes");
+    }
+    storage
+}
+
 #[cfg(test)]
 mod tests {
     use std::path::{Path, PathBuf};
@@ -1946,19 +1967,7 @@ mod tests {
     /// A host whose storage holds one file per way `ImportJsonHeadless` can
     /// fail, plus nothing at the two paths that must fail to read.
     fn host_with_import_fixtures() -> Host {
-        let storage = MemoryFs::new();
-        let files: [(&str, &[u8]); 4] = [
-            // Every field numeric, so no category is accepted and the build fails.
-            ("measures.json", br#"[{"x": 1}, {"x": 2}]"#),
-            ("bad.json", b"{not json"),
-            ("nested.json", br#"{"data": [{"Region": "North"}]}"#),
-            // A record that is not UTF-8 is a CSV read error, not a warning.
-            ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"),
-        ];
-        for (name, bytes) in files {
-            crate::storage::Storage::write(&storage, Path::new(name), bytes)
-                .expect("fixture writes");
-        }
+        let storage = super::import_fixture_storage();
         // A second view, so deleting an absent one is not refused as "the last".
         let mut wb = workbook();
         wb.create_view("Scratch");