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:
@@ -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]
|
||||
|
||||
@@ -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() {
|
||||
|
||||
Reference in New Issue
Block a user