refactor(command): parameterize mode-related commands and effects

Make mode-related commands and effects mode-agnostic by passing the target
mode as an argument instead of inspecting the current application mode.

- `CommitAndAdvance` now accepts `edit_mode` .
- `EditOrDrill` now accepts `edit_mode` .
- `EnterEditAtCursorCmd` now accepts `target_mode` .
- `EnterEditAtCursor` effect now accepts `target_mode` .

Update the command registry to parse mode names from arguments and pass
them to the corresponding commands.

Add tests to verify the new mode-passing behavior.

Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-26B-A4B-it-UD-Q5_K_XL.gguf)

src/command/cmd/commit.rs

@@ -118,6 +118,35 @@ mod tests {
         assert!(effects.is_empty());
     }
 
+    /// `CommitAndAdvance` must thread its `edit_mode` through to the
+    /// trailing `EnterEditAtCursor` effect so the post-commit re-edit lands
+    /// in the mode the keymap requested. The command never reads ctx.mode.
+    #[test]
+    fn commit_and_advance_threads_edit_mode_to_enter_edit_at_cursor() {
+        let m = two_cat_model();
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        let mut bufs = HashMap::new();
+        bufs.insert("edit".to_string(), "42".to_string());
+        let mut ctx = make_ctx(&m, &layout, &reg);
+        ctx.buffers = &bufs;
+        // ctx.mode stays Normal — the command must not look at it.
+        let key = ctx.cell_key().unwrap();
+        let cmd = CommitAndAdvance {
+            key,
+            value: "42".to_string(),
+            advance: super::AdvanceDir::Down,
+            cursor: super::CursorState::from_ctx(&ctx),
+            edit_mode: AppMode::records_editing(),
+        };
+        let effects = cmd.execute(&ctx);
+        let dbg = effects_debug(&effects);
+        assert!(
+            dbg.contains("EnterEditAtCursor") && dbg.contains("RecordsEditing"),
+            "Expected trailing EnterEditAtCursor with RecordsEditing target, got: {dbg}"
+        );
+    }
+
     #[test]
     fn commit_export_produces_export_and_normal_mode() {
         let m = two_cat_model();
@@ -234,12 +263,18 @@ pub enum AdvanceDir {
 
 /// Commit a cell edit, advance the cursor, and re-enter edit mode.
 /// Subsumes the old `CommitCellEdit` (Down) and `CommitAndAdvanceRight` (Right).
+///
+/// `edit_mode` is the editing mode to re-enter after advancing. The keymap
+/// binding supplies this — the editing-mode keymap passes `editing` and the
+/// records-editing keymap passes `records-editing`. The command itself
+/// never inspects `ctx.mode`.
 #[derive(Debug)]
 pub struct CommitAndAdvance {
     pub key: CellKey,
     pub value: String,
     pub advance: AdvanceDir,
     pub cursor: CursorState,
+    pub edit_mode: AppMode,
 }
 impl Cmd for CommitAndAdvance {
     fn name(&self) -> &'static str {
@@ -287,7 +322,9 @@ impl Cmd for CommitAndAdvance {
                 }
             }
         }
-        effects.push(Box::new(effect::EnterEditAtCursor));
+        effects.push(Box::new(effect::EnterEditAtCursor {
+            target_mode: self.edit_mode.clone(),
+        }));
         effects
     }
 }

src/command/cmd/mode.rs

@@ -10,21 +10,6 @@ mod tests {
     use crate::command::cmd::test_helpers::*;
     use crate::model::Model;
 
-    #[test]
-    fn enter_edit_mode_produces_editing_mode() {
-        let m = two_cat_model();
-        let layout = make_layout(&m);
-        let reg = make_registry();
-        let ctx = make_ctx(&m, &layout, &reg);
-        let cmd = EnterEditMode {
-            initial_value: String::new(),
-        };
-        let effects = cmd.execute(&ctx);
-        assert_eq!(effects.len(), 2);
-        let dbg = format!("{:?}", effects[1]);
-        assert!(dbg.contains("Editing"), "Expected Editing mode, got: {dbg}");
-    }
-
     #[test]
     fn enter_tile_select_with_categories() {
         let m = two_cat_model();
@@ -98,11 +83,80 @@ mod tests {
         let layout = make_layout(&m);
         let reg = make_registry();
         let ctx = make_ctx(&m, &layout, &reg);
-        let effects = EditOrDrill.execute(&ctx);
+        let effects = EditOrDrill {
+            edit_mode: AppMode::editing(),
+        }
+        .execute(&ctx);
+        assert_eq!(effects.len(), 2);
         let dbg = effects_debug(&effects);
         assert!(dbg.contains("Editing"), "Expected Editing mode, got: {dbg}");
     }
 
+    /// EditOrDrill must trust its `edit_mode` parameter rather than checking
+    /// `ctx.mode` — the records-normal keymap supplies `records-editing`,
+    /// but the command itself never inspects the runtime mode. This is the
+    /// parallel of the (deleted) `enter_edit_mode_produces_editing_mode`
+    /// test for the records branch.
+    #[test]
+    fn edit_or_drill_passes_records_editing_mode_through() {
+        let m = two_cat_model();
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        // Note: ctx.mode is still Normal here — the command must not look at it.
+        let ctx = make_ctx(&m, &layout, &reg);
+        let effects = EditOrDrill {
+            edit_mode: AppMode::records_editing(),
+        }
+        .execute(&ctx);
+        assert_eq!(effects.len(), 2);
+        let dbg = effects_debug(&effects);
+        assert!(
+            dbg.contains("RecordsEditing"),
+            "Expected RecordsEditing mode, got: {dbg}"
+        );
+    }
+
+    /// `EnterEditAtCursorCmd` must hand its `target_mode` straight through
+    /// to the `EnterEditAtCursor` effect — the keymap (records `o` sequence
+    /// or commit-and-advance) decides; the command never inspects ctx.
+    #[test]
+    fn enter_edit_at_cursor_cmd_passes_target_mode_to_effect() {
+        let m = two_cat_model();
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        let ctx = make_ctx(&m, &layout, &reg);
+        let effects = EnterEditAtCursorCmd {
+            target_mode: AppMode::records_editing(),
+        }
+        .execute(&ctx);
+        assert_eq!(effects.len(), 1);
+        let dbg = format!("{:?}", effects[0]);
+        assert!(
+            dbg.contains("RecordsEditing"),
+            "Expected RecordsEditing target_mode, got: {dbg}"
+        );
+    }
+
+    /// The edit branch pre-fills the `edit` buffer with the cell's current
+    /// display value so the user can modify rather than retype.
+    #[test]
+    fn edit_or_drill_pre_fills_edit_buffer_with_display_value() {
+        let m = two_cat_model();
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        let mut ctx = make_ctx(&m, &layout, &reg);
+        ctx.display_value = "42".to_string();
+        let effects = EditOrDrill {
+            edit_mode: AppMode::editing(),
+        }
+        .execute(&ctx);
+        let dbg = effects_debug(&effects);
+        assert!(
+            dbg.contains("SetBuffer") && dbg.contains("\"edit\"") && dbg.contains("\"42\""),
+            "Expected SetBuffer(\"edit\", \"42\"), got: {dbg}"
+        );
+    }
+
     #[test]
     fn enter_search_mode_sets_flag_and_clears_query() {
         let m = two_cat_model();
@@ -188,36 +242,18 @@ impl Cmd for SaveAndQuit {
 
 // ── Editing entry ───────────────────────────────────────────────────────
 
-/// Enter editing mode with an initial buffer value.
-#[derive(Debug)]
-pub struct EnterEditMode {
-    pub initial_value: String,
-}
-impl Cmd for EnterEditMode {
-    fn name(&self) -> &'static str {
-        "enter-edit-mode"
-    }
-    fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
-        let edit_mode = if ctx.mode.is_records() {
-            AppMode::records_editing()
-        } else {
-            AppMode::editing()
-        };
-        vec![
-            Box::new(effect::SetBuffer {
-                name: "edit".to_string(),
-                value: self.initial_value.clone(),
-            }),
-            effect::change_mode(edit_mode),
-        ]
-    }
-}
-
 /// Smart dispatch for i/a: if the cursor is on an aggregated pivot cell
-/// (categories on `Axis::None`, no records mode), drill into it instead of
+/// (categories on `Axis::None` and the cell is not a synthetic records-mode
-/// editing. Otherwise enter edit mode with the current displayed value.
+/// row), drill into it instead of editing. Otherwise pre-fill the edit
+/// buffer with the displayed cell value and enter `edit_mode`.
+///
+/// `edit_mode` is supplied by the keymap binding — the command itself is
+/// mode-agnostic, so the records-normal keymap passes `records-editing`
+/// while the normal keymap passes `editing`.
 #[derive(Debug)]
-pub struct EditOrDrill;
+pub struct EditOrDrill {
+    pub edit_mode: AppMode,
+}
 impl Cmd for EditOrDrill {
     fn name(&self) -> &'static str {
         "edit-or-drill"
@@ -232,7 +268,8 @@ impl Cmd for EditOrDrill {
                 .map(|cat| cat.kind.is_regular())
                 .unwrap_or(false)
         });
-        // In records mode (synthetic key), always edit directly — no drilling.
+        // Synthetic records-mode cells are never aggregated — edit directly.
+        // (This is a layout property, not a mode flag.)
         let is_synthetic = ctx.synthetic_record_at_cursor().is_some();
         let is_aggregated = !is_synthetic && regular_none;
         if is_aggregated {
@@ -241,23 +278,31 @@ impl Cmd for EditOrDrill {
             };
             return DrillIntoCell { key }.execute(ctx);
         }
-        EnterEditMode {
+        vec![
-            initial_value: ctx.display_value.clone(),
+            Box::new(effect::SetBuffer {
-        }
+                name: "edit".to_string(),
-        .execute(ctx)
+                value: ctx.display_value.clone(),
+            }),
+            effect::change_mode(self.edit_mode.clone()),
+        ]
     }
 }
 
 /// Thin command wrapper around the `EnterEditAtCursor` effect so it can
-/// participate in `Binding::Sequence`.
+/// participate in `Binding::Sequence`. `target_mode` is supplied as the
+/// command argument by the keymap binding.
 #[derive(Debug)]
-pub struct EnterEditAtCursorCmd;
+pub struct EnterEditAtCursorCmd {
+    pub target_mode: AppMode,
+}
 impl Cmd for EnterEditAtCursorCmd {
     fn name(&self) -> &'static str {
         "enter-edit-at-cursor"
     }
     fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
-        vec![Box::new(effect::EnterEditAtCursor)]
+        vec![Box::new(effect::EnterEditAtCursor {
+            target_mode: self.target_mode.clone(),
+        })]
     }
 }
 

src/command/cmd/registry.rs

@@ -2,6 +2,27 @@ use crate::model::cell::CellKey;
 use crate::ui::app::AppMode;
 use crate::ui::effect::Panel;
 
+/// Decode a mode-name string (as supplied by `enter-mode`/`edit-or-drill`
+/// keymap bindings) into an `AppMode`.
+fn parse_mode_name(s: &str) -> Result<AppMode, String> {
+    match s {
+        "normal" => Ok(AppMode::Normal),
+        "help" => Ok(AppMode::Help),
+        "formula-panel" => Ok(AppMode::FormulaPanel),
+        "category-panel" => Ok(AppMode::CategoryPanel),
+        "view-panel" => Ok(AppMode::ViewPanel),
+        "tile-select" => Ok(AppMode::TileSelect),
+        "command" => Ok(AppMode::command_mode()),
+        "category-add" => Ok(AppMode::category_add()),
+        "editing" => Ok(AppMode::editing()),
+        "records-normal" => Ok(AppMode::RecordsNormal),
+        "records-editing" => Ok(AppMode::records_editing()),
+        "formula-edit" => Ok(AppMode::formula_edit()),
+        "export-prompt" => Ok(AppMode::export_prompt()),
+        other => Err(format!("Unknown mode: {other}")),
+    }
+}
+
 use super::cell::*;
 use super::commit::*;
 use super::core::*;
@@ -266,22 +287,16 @@ pub fn default_registry() -> CmdRegistry {
     r.register_nullary(|| Box::new(SaveAndQuit));
     r.register_nullary(|| Box::new(SaveCmd));
     r.register_nullary(|| Box::new(EnterSearchMode));
-    r.register(
+    r.register_pure(&NamedCmd("edit-or-drill"), |args| {
-        &EnterEditMode {
+        require_args("edit-or-drill", args, 1)?;
-            initial_value: String::new(),
+        let edit_mode = parse_mode_name(&args[0])?;
-        },
+        Ok(Box::new(EditOrDrill { edit_mode }))
-        |args| {
+    });
-            let val = args.first().cloned().unwrap_or_default();
+    r.register_pure(&NamedCmd("enter-edit-at-cursor"), |args| {
-            Ok(Box::new(EnterEditMode { initial_value: val }))
+        require_args("enter-edit-at-cursor", args, 1)?;
-        },
+        let target_mode = parse_mode_name(&args[0])?;
-        |_args, ctx| {
+        Ok(Box::new(EnterEditAtCursorCmd { target_mode }))
-            Ok(Box::new(EnterEditMode {
+    });
-                initial_value: ctx.display_value.clone(),
-            }))
-        },
-    );
-    r.register_nullary(|| Box::new(EditOrDrill));
-    r.register_nullary(|| Box::new(EnterEditAtCursorCmd));
     r.register_nullary(|| Box::new(EnterExportPrompt));
     r.register_nullary(|| Box::new(EnterFormulaEdit));
     r.register_nullary(|| Box::new(EnterTileSelect));
@@ -310,23 +325,7 @@ pub fn default_registry() -> CmdRegistry {
     );
     r.register_pure(&NamedCmd("enter-mode"), |args| {
         require_args("enter-mode", args, 1)?;
-        let mode = match args[0].as_str() {
+        Ok(Box::new(EnterMode(parse_mode_name(&args[0])?)))
-            "normal" => AppMode::Normal,
-            "help" => AppMode::Help,
-            "formula-panel" => AppMode::FormulaPanel,
-            "category-panel" => AppMode::CategoryPanel,
-            "view-panel" => AppMode::ViewPanel,
-            "tile-select" => AppMode::TileSelect,
-            "command" => AppMode::command_mode(),
-            "category-add" => AppMode::category_add(),
-            "editing" => AppMode::editing(),
-            "records-normal" => AppMode::RecordsNormal,
-            "records-editing" => AppMode::records_editing(),
-            "formula-edit" => AppMode::formula_edit(),
-            "export-prompt" => AppMode::export_prompt(),
-            other => return Err(format!("Unknown mode: {other}")),
-        };
-        Ok(Box::new(EnterMode(mode)))
     });
 
     // ── Search ───────────────────────────────────────────────────────────
@@ -522,25 +521,33 @@ pub fn default_registry() -> CmdRegistry {
     r.register_nullary(|| Box::new(CommandModeBackspace));
 
     // ── Commit ───────────────────────────────────────────────────────────
+    // commit-cell-edit / commit-and-advance-right take a mode-name arg
+    // (e.g. "editing" or "records-editing") as args[0]. The keymap supplies
+    // it; the command never inspects ctx.mode.
     r.register(
         &CommitAndAdvance {
             key: CellKey::new(vec![]),
             value: String::new(),
             advance: AdvanceDir::Down,
             cursor: CursorState::default(),
+            edit_mode: AppMode::editing(),
         },
         |args| {
-            if args.len() < 2 {
+            if args.len() < 3 {
-                return Err("commit-cell-edit requires a value and coords".into());
+                return Err("commit-cell-edit requires a mode, value, and coords".into());
             }
+            let edit_mode = parse_mode_name(&args[0])?;
             Ok(Box::new(CommitAndAdvance {
-                key: parse_cell_key_from_args(&args[1..]),
+                key: parse_cell_key_from_args(&args[2..]),
-                value: args[0].clone(),
+                value: args[1].clone(),
                 advance: AdvanceDir::Down,
                 cursor: CursorState::default(),
+                edit_mode,
             }))
         },
-        |_args, ctx| {
+        |args, ctx| {
+            require_args("commit-cell-edit", args, 1)?;
+            let edit_mode = parse_mode_name(&args[0])?;
             let value = read_buffer(ctx, "edit");
             let key = ctx.cell_key().clone().ok_or("no cell at cursor")?;
             Ok(Box::new(CommitAndAdvance {
@@ -548,6 +555,7 @@ pub fn default_registry() -> CmdRegistry {
                 value,
                 advance: AdvanceDir::Down,
                 cursor: CursorState::from_ctx(ctx),
+                edit_mode,
             }))
         },
     );
@@ -557,9 +565,12 @@ pub fn default_registry() -> CmdRegistry {
             value: String::new(),
             advance: AdvanceDir::Right,
             cursor: CursorState::default(),
+            edit_mode: AppMode::editing(),
         },
         |_| Err("commit-and-advance-right requires context".into()),
-        |_args, ctx| {
+        |args, ctx| {
+            require_args("commit-and-advance-right", args, 1)?;
+            let edit_mode = parse_mode_name(&args[0])?;
             let value = read_buffer(ctx, "edit");
             let key = ctx.cell_key().clone().ok_or("no cell at cursor")?;
             Ok(Box::new(CommitAndAdvance {
@@ -567,6 +578,7 @@ pub fn default_registry() -> CmdRegistry {
                 value,
                 advance: AdvanceDir::Right,
                 cursor: CursorState::from_ctx(ctx),
+                edit_mode,
             }))
         },
     );

src/ui/effect.rs

@@ -123,10 +123,19 @@ impl Effect for RemoveFormula {
 
 /// Re-enter edit mode by reading the cell value at the current cursor.
 /// Used after commit+advance to continue data entry.
+///
+/// `target_mode` is supplied by the caller (keymap binding via
+/// `EnterEditAtCursorCmd`, or `CommitAndAdvance` from its own `edit_mode`
+/// field). The effect itself never inspects `app.mode` — the mode is decided
+/// statically by whoever invoked us.
 #[derive(Debug)]
-pub struct EnterEditAtCursor;
+pub struct EnterEditAtCursor {
+    pub target_mode: AppMode,
+}
 impl Effect for EnterEditAtCursor {
     fn apply(&self, app: &mut App) {
+        // Layout may be stale relative to prior effects in this batch (e.g.
+        // AddRecordRow added a row); rebuild before reading display_value.
         app.rebuild_layout();
         let ctx = app.cmd_context(
             crossterm::event::KeyCode::Null,
@@ -135,11 +144,7 @@ impl Effect for EnterEditAtCursor {
         let value = ctx.display_value.clone();
         drop(ctx);
         app.buffers.insert("edit".to_string(), value);
-        app.mode = if app.mode.is_records() {
+        app.mode = self.target_mode.clone();
-            AppMode::records_editing()
-        } else {
-            AppMode::editing()
-        };
     }
 }
 
@@ -1284,6 +1289,41 @@ mod tests {
         assert_eq!(app.mode, AppMode::Help);
     }
 
+    /// `EnterEditAtCursor` must use its `target_mode` field, *not* whatever
+    /// `app.mode` happens to be when applied. Previous implementation
+    /// branched on `app.mode.is_records()` — the parameterized version
+    /// trusts the caller (keymap or composing command).
+    #[test]
+    fn enter_edit_at_cursor_uses_target_mode_not_app_mode() {
+        let mut app = test_app();
+        // App starts in Normal mode — but caller has decided we want
+        // RecordsEditing (e.g. records-mode `o` sequence).
+        assert_eq!(app.mode, AppMode::Normal);
+        EnterEditAtCursor {
+            target_mode: AppMode::records_editing(),
+        }
+        .apply(&mut app);
+        assert!(
+            matches!(app.mode, AppMode::RecordsEditing { .. }),
+            "Expected RecordsEditing, got {:?}",
+            app.mode
+        );
+
+        // Same effect with editing target — should land in plain Editing
+        // even if app.mode was something else.
+        let mut app2 = test_app();
+        app2.mode = AppMode::RecordsNormal;
+        EnterEditAtCursor {
+            target_mode: AppMode::editing(),
+        }
+        .apply(&mut app2);
+        assert!(
+            matches!(app2.mode, AppMode::Editing { .. }),
+            "Expected Editing, got {:?}",
+            app2.mode
+        );
+    }
+
     /// SetBuffer with empty value clears the buffer (used by clear-buffer command
     /// in keymap sequences after commit).
     #[test]