fix(effect): SaveAs must abort the batch too — the other arm of improvise-8tk

Found reviewing improvise-lae. improvise-8tk fixed one arm of a rule that
has two: a failed `Save` sets `abort_effects` so the `Quit` chained after
it never lands, but `SaveAs` only wrote "Save error:" to the status bar
and let the batch continue. Three tests guard the `Save` arm and none
guarded this one —
`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`,
the same shape that doctrine was written for.

It is reachable today without any of this branch's work: the `:` line
composes on ` . ` and `ExecuteCommand` concatenates the commands' effects
into one batch, so `:w /bad/path . q` quits and discards the model on a
failed write. That is improvise-8tk's silent data loss on the other path.

It also matters more now, because improvise-lae's save prompt commits to
`SaveAs`. With the abort, a failed write skips both the `ChangeMode`
`CommitSave` emits and the `clear-buffer` the keymap welds after it, so
the user stays in the prompt with the path they typed still there to
correct, and the error on screen. Without it they were dropped to Normal
with an empty buffer.

Evidence: `failed_save_as_aborts_the_chain` was watched failing on the
old code; `successful_save_as_lets_the_chain_continue` is the positive
arm, without which "always abort" would pass and every `:w <path>` would
swallow what followed it — it passed before the change, so it is not
circular. `ctrl_w_prompts_for_a_path_and_a_failed_write_keeps_the_prompt_open`
drives the whole prompt through real keypresses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 22:46:53 -07:00
parent f129305e31
commit f0f400d29a
2 changed files with 116 additions and 0 deletions
+50
View File
@@ -1615,6 +1615,56 @@ mod tests {
);
}
/// improvise-lae end to end: `Ctrl-W`, a path, `Enter`.
///
/// The failing case is the interesting one, and it is what `SaveAs`
/// gaining `Save`'s abort buys the prompt. `CommitSave` emits
/// `[SaveAs, ChangeMode(Normal)]` and the keymap welds a trailing
/// `clear-buffer`, so an aborting `SaveAs` skips both: the user is left
/// in the prompt, with the path they typed still there to correct, and
/// the error on screen. Without the abort they would have been dropped
/// back to Normal with an error and an empty buffer.
#[test]
fn ctrl_w_prompts_for_a_path_and_a_failed_write_keeps_the_prompt_open() {
let mut app = App::new(Workbook::new("T"), None);
app.handle_key(KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL))
.unwrap();
assert!(
matches!(app.view_state.mode, AppMode::SavePrompt { .. }),
"Ctrl-W should open the save prompt, got {:?}",
app.view_state.mode
);
let path = "/nonexistent-dir-improvise-lae/m.improv";
for c in path.chars() {
app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
.unwrap();
}
assert_eq!(
app.view_state.buffers.get("save").map(String::as_str),
Some(path),
"typing must reach the buffer the prompt names"
);
app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
.unwrap();
assert!(
app.view_state.status_msg.starts_with("Save error:"),
"got {:?}",
app.view_state.status_msg
);
assert!(
matches!(app.view_state.mode, AppMode::SavePrompt { .. }),
"a failed write must leave the prompt open, got {:?}",
app.view_state.mode
);
assert_eq!(
app.view_state.buffers.get("save").map(String::as_str),
Some(path),
"the path the user typed must survive for them to correct"
);
}
/// `ZZ` is save-and-quit. With no file path the save fails and aborts the
/// chain (improvise-8tk), which also proves the second Z reached `wq`.
#[test]
+66
View File
@@ -788,8 +788,12 @@ impl Effect for SaveAs {
app.model_state.dirty = false;
app.view_state.status_msg = format!("Saved to {}", self.0.display());
}
// The same abort `Save` owes (improvise-8tk). `SaveAs` is the
// other arm of that rule and was left out of the original fix,
// so `:w /bad/path . q` quit on a failed write.
Err(e) => {
app.view_state.status_msg = format!("Save error: {e}");
app.abort_effects = true;
}
}
}
@@ -1961,6 +1965,68 @@ mod tests {
);
}
/// BUG (improvise-lae, found reviewing it): improvise-8tk fixed **one
/// arm** of a rule with two. `Save` aborts the batch when the write
/// fails; `SaveAs` reported the error to the status bar and let the
/// batch continue, which is the same silent discard 8tk closed — the
/// shape `doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`
/// names, and the three tests above are evidence about `Save` only.
///
/// It is reachable today: the `:` line composes with ` . `, so
/// `:w /bad/path . q` builds exactly this batch, and
/// `ExecuteCommand` concatenates the two commands' effects into one.
/// improvise-lae's new save prompt also commits to `SaveAs`, so the
/// arm was about to carry more traffic.
#[test]
fn failed_save_as_aborts_the_chain() {
let mut app = test_app();
app.model_state.dirty = true;
app.apply_effects(vec![
Box::new(SaveAs(PathBuf::from(
"/nonexistent-dir-improvise-lae/model.improv",
))),
Box::new(ChangeMode(AppMode::Quit)),
]);
assert_ne!(
app.view_state.mode,
AppMode::Quit,
"a failed save-as must not let the trailing Quit apply"
);
assert!(
app.model_state.dirty,
"a failed save-as must leave the model dirty"
);
assert!(
app.view_state.status_msg.starts_with("Save error:"),
"expected the save error to stay on screen, got: {}",
app.view_state.status_msg
);
}
/// The positive arm: a `SaveAs` that succeeds must not abort, or every
/// `:w <path>` would swallow whatever followed it. Without this,
/// "always abort" passes the test above.
#[test]
fn successful_save_as_lets_the_chain_continue() {
let dir = std::env::temp_dir().join("improvise-lae-save-as-ok");
std::fs::create_dir_all(&dir).unwrap();
let path = dir.join("model.improv");
let mut app = test_app();
app.model_state.dirty = true;
app.apply_effects(vec![
Box::new(SaveAs(path.clone())),
Box::new(ChangeMode(AppMode::Quit)),
]);
assert_eq!(app.view_state.mode, AppMode::Quit);
assert!(!app.model_state.dirty);
assert_eq!(
app.model_state.file_path.as_deref(),
Some(path.as_path()),
"a successful save-as adopts the path"
);
let _ = std::fs::remove_file(&path);
}
/// A successful save must not abort — `wq` still quits.
#[test]
fn successful_save_lets_the_chain_continue() {