fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(command): unify minibuffer commits into CommitBuffer(kind) (improvise-6t3)

Browse Source 
CommitFormula, CommitCategoryAdd and CommitItemAdd collapse into a single
CommitBuffer { kind: CommitKind }, registered as
`commit-buffer <formula|category|item>`. The five keymap bindings pass
the kind as an argument, giving improvise-w3q a uniform primitive to
build its text-entry template on.

The three did not have identical shape — they differed in buffer name,
empty-buffer landing mode, post-commit landing mode, status text and add
effect, and two of those are semantic (formula returns to its panel;
category/item stay open for batch entry). CommitKind carries those as
methods rather than flattening them, so this costs ~24 lines rather than
saving 40. The win is one home for minibuffer commit semantics.

CommitKind::Item carries its target category, resolved from
AppMode::ItemAdd by parse_commit_kind at registry time. execute() is
therefore a pure function of kind + buffer, the shared helper's
Option<Box<dyn Effect>> is gone, and the old silent no-op outside
ItemAdd mode becomes a real error on the `:` line.

Fixes a bug found on the way: the commit pushed set_status("Formula
added") after AddFormula, clobbering the parse error that
AddFormula::apply writes to the status bar — an unparseable formula was
dropped while the UI reported success. The add effect now goes last, so
the only party that knows whether it worked gets the last word.

Also adds a keymap test that dispatches Enter in each minibuffer mode:
Keymap::dispatch swallows an unresolvable command name with .ok()?, so
the existing lookup-only smoke tests would have passed even had the
rename missed the registry.

Filed improvise-djn: AddCategory still swallows its Result.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-08-02 10:28:59 -07:00
parent c7a06f3720
commit 46b5c7736d
4 changed files with 218 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/repo-map.md
+5 -4
View File
@@ -145,7 +145,8 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
10. **Float equality inside formula eval is currently mixed** — `=`/`!=` use a `1e-10` epsilon but the div-by-zero guard checks `rv == 0.0` exactly. Don't assume one or the other; tracked for cleanup.
11. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this.
12. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior.
13. **`app.layout` goes stale mid-batch.** `apply_effects` rebuilds only *after* the whole batch, so an effect that reads the layout after an earlier effect changed the grid needs a `RebuildLayout` effect in front of it — the command decides the order (see `EnterEditAtCursorCmd`, `CommitAndAdvance`). Read the cursor's cell text via `App::display_value_at_cursor()`; don't build a `CmdContext` inside `apply` (it recomputes column widths).
13. **Minibuffer commits go through one command.** `commit-buffer <formula|category|item>` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line.
14. **`app.layout` goes stale mid-batch.** `apply_effects` rebuilds only *after* the whole batch, so an effect that reads the layout after an earlier effect changed the grid needs a `RebuildLayout` effect in front of it — the command decides the order (see `EnterEditAtCursorCmd`, `CommitAndAdvance`). Read the cursor's cell text via `App::display_value_at_cursor()`; don't build a `CmdContext` inside `apply` (it recomputes column widths).
---
@@ -184,10 +185,10 @@ import/csv_parser.rs 300 / 8t CSV parsing, multi-file merge
### Command layer (`crates/improvise-command/src/command/`)
```
cmd/core.rs 319 / 2t Cmd trait, CmdContext, CmdRegistry, parse helpers
cmd/registry.rs 601 / 0t default_registry() — all registrations (no tests yet)
cmd/registry.rs 604 / 0t default_registry() — all registrations (no tests yet)
cmd/navigation.rs 501 / 10t Move, EnterAdvance, Page*
cmd/cell.rs 198 / 6t ClearCell, YankCell, PasteCell, TransposeAxes, SaveCmd
cmd/commit.rs 455 / 7t CommitFormula, CommitCategoryAdd/ItemAdd, CommitExport
cmd/commit.rs 517 / 9t CommitAndAdvance, CommitBuffer(CommitKind), CommitExport
cmd/effect_cmds.rs 466 / 5t effect_cmd! macro, 25+ simple wrappers
cmd/grid.rs 494 / 7t ToggleGroup, ViewNavigate, DrillIntoCell, TogglePruneEmpty
cmd/mode.rs 354 / 8t EnterMode, Quit, EditOrDrill, EnterTileSelect
@@ -196,7 +197,7 @@ cmd/search.rs 202 / 4t SearchNavigate, SearchOrCategoryAdd, ExitSea
cmd/text_buffer.rs 240 / 7t AppendChar, PopChar, CommandModeBackspace, ExecuteCommand
cmd/tile.rs 160 / 5t MoveTileCursor, TileAxisOp
cmd/wizard.rs 199 / 4t HandleWizardKey — per-step import-wizard key tables
keymap.rs 1248 / 22t KeyPattern, Binding, Keymap, ModeKey, 16 mode keymaps
keymap.rs 1290 / 23t KeyPattern, Binding, Keymap, ModeKey, 16 mode keymaps
parse.rs 236 / 19t Script/command-line parser (prefix syntax)
```
crates/improvise-command/src/command/cmd/commit.rs
+149 -88
View File
@@ -2,7 +2,7 @@ use crate::model::cell::{CellKey, CellValue};
use crate::ui::app::AppMode;
use crate::ui::effect::{self, Effect};
use super::core::{Cmd, CmdContext};
use super::core::{Cmd, CmdContext, read_buffer};
use super::grid::AddRecordRow;
use super::navigation::{CursorState, EnterAdvance, viewport_effects};
@@ -14,6 +14,10 @@ mod tests {
use crate::command::cmd::test_helpers::*;
use crate::workbook::Workbook;
fn commit(kind: CommitKind) -> CommitBuffer {
CommitBuffer { kind }
}
#[test]
fn commit_formula_with_categories_adds_formula() {
let m = two_cat_model();
@@ -23,7 +27,7 @@ mod tests {
bufs.insert("formula".to_string(), "Profit = Revenue - Cost".to_string());
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.buffers = &bufs;
let effects = CommitFormula.execute(&ctx);
let effects = commit(CommitKind::Formula).execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("AddFormula"),
@@ -46,7 +50,7 @@ mod tests {
bufs.insert("formula".to_string(), "X = Y + Z".to_string());
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.buffers = &bufs;
let effects = CommitFormula.execute(&ctx);
let effects = commit(CommitKind::Formula).execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("AddFormula"),
@@ -58,6 +62,33 @@ mod tests {
);
}
/// improvise-6t3: the commit pushed `set_status("Formula added")` *after*
/// `AddFormula`, so the parse error that `AddFormula::apply` writes to the
/// status bar was immediately overwritten. An unparseable formula was
/// dropped while the UI reported success.
///
/// The add effect is the only party that knows whether it worked, so it has
/// to get the last word on the status line.
#[test]
fn invalid_formula_reports_the_parse_error_not_success() {
let m = two_cat_model();
let layout = make_layout(&m);
let reg = make_registry();
let mut bufs = HashMap::new();
bufs.insert("formula".to_string(), "Profit = = ".to_string());
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.buffers = &bufs;
let effects = commit(CommitKind::Formula).execute(&ctx);
let mut app = crate::ui::app::App::new(two_cat_model(), None);
app.apply_effects(effects);
assert!(
app.view_state.status_msg.contains("Formula error"),
"Expected the parse error to reach the status bar, got: {:?}",
app.view_state.status_msg
);
}
#[test]
fn commit_category_add_with_name_produces_add_effect() {
let m = two_cat_model();
@@ -67,7 +98,7 @@ mod tests {
bufs.insert("category".to_string(), "Region".to_string());
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.buffers = &bufs;
let effects = CommitCategoryAdd.execute(&ctx);
let effects = commit(CommitKind::Category).execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("AddCategory"),
@@ -84,7 +115,7 @@ mod tests {
bufs.insert("category".to_string(), "".to_string());
let mut ctx = make_ctx(&m, &layout, &reg);
ctx.buffers = &bufs;
let effects = CommitCategoryAdd.execute(&ctx);
let effects = commit(CommitKind::Category).execute(&ctx);
let dbg = effects_debug(&effects);
assert!(
dbg.contains("CategoryPanel"),
@@ -92,8 +123,10 @@ mod tests {
);
}
/// `commit-buffer item` takes its target category off `AppMode::ItemAdd`
/// at construction time, so the command itself never reads the mode.
#[test]
fn commit_item_add_with_name_produces_add_item() {
fn commit_item_add_files_the_item_under_the_mode_s_category() {
let m = two_cat_model();
let layout = make_layout(&m);
let reg = make_registry();
@@ -103,19 +136,24 @@ mod tests {
let item_add_mode = AppMode::item_add("Month".to_string());
ctx.mode = &item_add_mode;
ctx.buffers = &bufs;
let effects = CommitItemAdd.execute(&ctx);
let dbg = effects_debug(&effects);
let cmd = reg
.interactive("commit-buffer", &["item".to_string()], &ctx)
.expect("item-add mode should supply the category");
let dbg = effects_debug(&cmd.execute(&ctx));
assert!(dbg.contains("AddItem"), "Expected AddItem, got: {dbg}");
assert!(dbg.contains("Month"), "Expected category Month, got: {dbg}");
}
#[test]
fn commit_item_add_outside_item_add_mode_returns_empty() {
fn commit_item_add_outside_item_add_mode_is_a_construction_error() {
let m = two_cat_model();
let layout = make_layout(&m);
let reg = make_registry();
let ctx = make_ctx(&m, &layout, &reg);
let effects = CommitItemAdd.execute(&ctx);
assert!(effects.is_empty());
assert!(
reg.interactive("commit-buffer", &["item".to_string()], &ctx)
.is_err()
);
}
/// `CommitAndAdvance` must thread its `edit_mode` through to the
@@ -349,95 +387,118 @@ impl Cmd for CommitAndAdvance {
}
}
/// Commit a formula from the formula edit buffer.
#[derive(Debug)]
pub struct CommitFormula;
impl Cmd for CommitFormula {
fn name(&self) -> &'static str {
"commit-formula"
/// What a minibuffer commit produces. The variants differ only in the model
/// mutation they build and in where they leave the user — reading the buffer,
/// trimming it, bailing out when it is empty, marking the model dirty and
/// reporting are shared by [`CommitBuffer`].
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum CommitKind {
Formula,
Category,
/// Add an item to this category. Resolved from [`AppMode::ItemAdd`] when
/// the command is constructed, so `execute` stays a pure function of the
/// kind and its buffer.
Item {
category: String,
},
}
/// Resolve a `commit-buffer` argument against the current mode. `Item` is the
/// only kind that needs anything from the mode — its target category — so a
/// script line (which has no live mode) can build the other two.
pub fn parse_commit_kind(arg: &str, mode: &AppMode) -> Result<CommitKind, String> {
match (arg, mode) {
("formula", _) => Ok(CommitKind::Formula),
("category", _) => Ok(CommitKind::Category),
("item", AppMode::ItemAdd { category, .. }) => Ok(CommitKind::Item {
category: category.clone(),
}),
("item", _) => Err("commit-buffer item requires item-add mode".into()),
(other, _) => Err(format!("Unknown commit kind: {other}")),
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let buf = ctx.buffers.get("formula").cloned().unwrap_or_default();
// Default formula target to _Measure (the virtual measure category).
// _Measure dynamically includes all formula targets.
vec![
Box::new(effect::AddFormula {
raw: buf,
}
impl CommitKind {
/// The minibuffer this kind consumes.
pub fn buffer(&self) -> &'static str {
match self {
Self::Formula => "formula",
Self::Category => "category",
Self::Item { .. } => "item",
}
}
/// Where an empty buffer lands the user: back in the panel that opened the
/// minibuffer.
fn on_empty(&self) -> AppMode {
match self {
Self::Formula => AppMode::FormulaPanel,
Self::Category | Self::Item { .. } => AppMode::CategoryPanel,
}
}
/// Where a *successful* commit lands the user. `None` keeps the minibuffer
/// open for the next entry — categories and items are entered in batches,
/// formulas are not.
fn on_commit(&self) -> Option<AppMode> {
match self {
Self::Formula => Some(AppMode::FormulaPanel),
Self::Category | Self::Item { .. } => None,
}
}
fn status(&self, name: &str) -> String {
match self {
Self::Formula => "Formula added".to_string(),
Self::Category => format!("Added category \"{name}\""),
Self::Item { .. } => format!("Added \"{name}\""),
}
}
fn add_effect(&self, name: &str) -> Box<dyn Effect> {
match self {
// Formulas default to _Measure, the virtual measure category, which
// dynamically includes every formula target.
Self::Formula => Box::new(effect::AddFormula {
raw: name.to_string(),
target_category: "_Measure".to_string(),
}),
effect::mark_dirty(),
effect::set_status("Formula added"),
effect::change_mode(AppMode::FormulaPanel),
]
Self::Category => Box::new(effect::AddCategory(name.to_string())),
Self::Item { category } => Box::new(effect::AddItem {
category: category.clone(),
item: name.to_string(),
}),
}
}
}
/// Shared helper: read a buffer, trim it, and if non-empty, produce add + dirty
/// + status effects. If empty, return to CategoryPanel.
/// Commit the contents of a minibuffer as a formula, a category or an item.
///
/// Buffer clearing is handled by the keymap (Enter → [commit, clear-buffer]).
fn commit_add_from_buffer(
ctx: &CmdContext,
buffer_name: &str,
add_effect: impl FnOnce(&str) -> Option<Box<dyn Effect>>,
status_msg: impl FnOnce(&str) -> String,
) -> Vec<Box<dyn Effect>> {
let buf = ctx.buffers.get(buffer_name).cloned().unwrap_or_default();
let trimmed = buf.trim().to_string();
if trimmed.is_empty() {
return vec![effect::change_mode(AppMode::CategoryPanel)];
}
let Some(add) = add_effect(&trimmed) else {
return vec![];
};
vec![
add,
effect::mark_dirty(),
effect::set_status(status_msg(&trimmed)),
]
}
/// Commit adding a category, staying in CategoryAdd mode for the next entry.
#[derive(Debug)]
pub struct CommitCategoryAdd;
impl Cmd for CommitCategoryAdd {
pub struct CommitBuffer {
pub kind: CommitKind,
}
impl Cmd for CommitBuffer {
fn name(&self) -> &'static str {
"commit-category-add"
"commit-buffer"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
commit_add_from_buffer(
ctx,
"category",
|name| Some(Box::new(effect::AddCategory(name.to_string()))),
|name| format!("Added category \"{name}\""),
)
}
}
/// Commit adding an item, staying in ItemAdd mode for the next entry.
#[derive(Debug)]
pub struct CommitItemAdd;
impl Cmd for CommitItemAdd {
fn name(&self) -> &'static str {
"commit-item-add"
}
fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
let category = if let AppMode::ItemAdd { category, .. } = ctx.mode {
category.clone()
} else {
return vec![];
};
commit_add_from_buffer(
ctx,
"item",
|name| {
Some(Box::new(effect::AddItem {
category: category.clone(),
item: name.to_string(),
}))
},
|name| format!("Added \"{name}\""),
)
let raw = read_buffer(ctx, self.kind.buffer());
let name = raw.trim();
if name.is_empty() {
return vec![effect::change_mode(self.kind.on_empty())];
}
let mut effects: Vec<Box<dyn Effect>> = vec![
effect::mark_dirty(),
effect::set_status(self.kind.status(name)),
// The add goes last on purpose: it is the only party that knows
// whether it worked, so the optimistic status above must not
// clobber the error it reports (improvise-6t3).
self.kind.add_effect(name),
];
effects.extend(self.kind.on_commit().map(effect::change_mode));
effects
}
}
crates/improvise-command/src/command/cmd/registry.rs
+15 -3
View File
@@ -573,9 +573,21 @@ pub fn default_registry() -> CmdRegistry {
}))
},
);
r.register_nullary(|| Box::new(CommitFormula));
r.register_nullary(|| Box::new(CommitCategoryAdd));
r.register_nullary(|| Box::new(CommitItemAdd));
r.register(
&CommitBuffer {
kind: CommitKind::Category,
},
|args| {
require_args("commit-buffer", args, 1)?;
let kind = parse_commit_kind(&args[0], &AppMode::Normal)?;
Ok(Box::new(CommitBuffer { kind }))
},
|args, ctx| {
require_args("commit-buffer", args, 1)?;
let kind = parse_commit_kind(&args[0], ctx.mode)?;
Ok(Box::new(CommitBuffer { kind }))
},
);
r.register_nullary(|| Box::new(CommitExport));
r.register_nullary(|| Box::new(ExecuteCommand));
crates/improvise-command/src/command/keymap.rs
+49 -5
View File
@@ -842,7 +842,7 @@ impl KeymapSet {
KeyCode::Enter,
none,
vec![
("commit-formula", vec![]),
("commit-buffer", vec!["formula".into()]),
("clear-buffer", vec!["formula".into()]),
],
);
@@ -864,7 +864,7 @@ impl KeymapSet {
KeyCode::Enter,
none,
vec![
("commit-category-add", vec![]),
("commit-buffer", vec!["category".into()]),
("clear-buffer", vec!["category".into()]),
],
);
@@ -872,7 +872,7 @@ impl KeymapSet {
KeyCode::Tab,
none,
vec![
("commit-category-add", vec![]),
("commit-buffer", vec!["category".into()]),
("clear-buffer", vec!["category".into()]),
],
);
@@ -899,7 +899,7 @@ impl KeymapSet {
KeyCode::Enter,
none,
vec![
("commit-item-add", vec![]),
("commit-buffer", vec!["item".into()]),
("clear-buffer", vec!["item".into()]),
],
);
@@ -907,7 +907,7 @@ impl KeymapSet {
KeyCode::Tab,
none,
vec![
("commit-item-add", vec![]),
("commit-buffer", vec!["item".into()]),
("clear-buffer", vec!["item".into()]),
],
);
@@ -1240,6 +1240,50 @@ mod tests {
assert!(search.lookup(KeyCode::Esc, KeyModifiers::NONE).is_some());
}
/// improvise-6t3: the three commit commands collapsed into a single
/// `commit-buffer <kind>`. `dispatch` swallows an unresolvable command name
/// with `.ok()?`, so a lookup-only assertion would still pass had the
/// rename missed the registry — Enter has to come back with real effects.
#[test]
fn minibuffer_enter_dispatches_to_a_registered_commit() {
use crate::command::cmd::test_helpers::*;
use crate::ui::app::AppMode;
let ks = KeymapSet::default_keymaps();
let wb = two_cat_model();
let layout = make_layout(&wb);
let reg = make_registry();
let mut bufs = HashMap::new();
bufs.insert("formula".to_string(), "Profit = Revenue - Cost".to_string());
bufs.insert("category".to_string(), "Region".to_string());
bufs.insert("item".to_string(), "March".to_string());
let item_add = AppMode::item_add("Month".to_string());
for (mode_key, mode, expected) in [
(ModeKey::FormulaEdit, &AppMode::formula_edit(), "AddFormula"),
(
ModeKey::CategoryAdd,
&AppMode::category_add(),
"AddCategory",
),
(ModeKey::ItemAdd, &item_add, "AddItem"),
] {
let mut ctx = make_ctx(&wb, &layout, &reg);
ctx.mode = mode;
ctx.buffers = &bufs;
let km = ks.mode_maps.get(&mode_key).unwrap();
let effects = km
.dispatch(&reg, &ctx, KeyCode::Enter, KeyModifiers::NONE)
.unwrap_or_else(|| panic!("{mode_key:?}: Enter did not dispatch"));
let dbg = effects_debug(&effects);
assert!(
dbg.contains(expected),
"{mode_key:?}: expected {expected}, got: {dbg}"
);
}
}
#[test]
fn import_wizard_has_any_catchall() {
let ks = KeymapSet::default_keymaps();