From 9710fb534e6590285ed29283bf414f4290bdf84b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 14 Apr 2026 02:32:03 -0700 Subject: [PATCH] feat(cmd): allow direct commit of synthetic records in records mode Allow synthetic record edits to be applied directly to the model when not in drill mode, whereas they remain staged in drill state when a drill snapshot is active. This enables editing of records in plain records view. Additionally, add validation to prevent creating records with empty coordinates in both direct commits and when applying staged drill edits. Includes regression tests for persistence in blank models, drill state staging, and empty coordinate prevention. Co-Authored-By: fiddlerwoaroof/git-smart-commit (gemma-4-31B-it-UD-Q4_K_XL.gguf) --- src/command/cmd/commit.rs | 70 +++++++++++++++++++--- src/command/cmd/core.rs | 2 + src/command/cmd/mod.rs | 1 + src/ui/app.rs | 121 ++++++++++++++++++++++++++++++++++++++ src/ui/effect.rs | 4 ++ 5 files changed, 191 insertions(+), 7 deletions(-) diff --git a/src/command/cmd/commit.rs b/src/command/cmd/commit.rs index fa78d4c..b969be8 100644 --- a/src/command/cmd/commit.rs +++ b/src/command/cmd/commit.rs @@ -137,14 +137,70 @@ mod tests { // ── Commit commands (mode-specific buffer consumers) ──────────────────────── /// Commit a cell value: for synthetic records keys, stage in drill pending edits -/// or apply directly; for real keys, write to the model. -fn commit_cell_value(key: &CellKey, value: &str, effects: &mut Vec>) { +/// in drill mode, or apply directly in plain records mode; for real keys, write +/// to the model. +fn commit_cell_value( + ctx: &CmdContext, + key: &CellKey, + value: &str, + effects: &mut Vec>, +) { if let Some((record_idx, col_name)) = crate::view::synthetic_record_info(key) { - effects.push(Box::new(effect::SetDrillPendingEdit { - record_idx, - col_name, - new_value: value.to_string(), + if ctx.has_drill_state { + effects.push(Box::new(effect::SetDrillPendingEdit { + record_idx, + col_name, + new_value: value.to_string(), + })); + return; + } + + let Some((orig_key, _)) = ctx + .layout + .records + .as_ref() + .and_then(|records| records.get(record_idx)) + else { + return; + }; + + if col_name == "Value" { + if value.is_empty() { + effects.push(Box::new(effect::ClearCell(orig_key.clone()))); + } else if let Ok(n) = value.parse::() { + effects.push(Box::new(effect::SetCell( + orig_key.clone(), + CellValue::Number(n), + ))); + } else { + effects.push(Box::new(effect::SetCell( + orig_key.clone(), + CellValue::Text(value.to_string()), + ))); + } + effects.push(effect::mark_dirty()); + return; + } + + if value.is_empty() { + effects.push(effect::set_status("Record coordinates cannot be empty")); + return; + } + + let Some(existing_value) = ctx.model.get_cell(orig_key).cloned() else { + return; + }; + effects.push(Box::new(effect::ClearCell(orig_key.clone()))); + effects.push(Box::new(effect::AddItem { + category: col_name.clone(), + item: value.to_string(), })); + effects.push(Box::new(effect::SetCell( + orig_key.clone().with(col_name, value), + existing_value, + ))); + effects.push(effect::mark_dirty()); + return; } else if value.is_empty() { effects.push(Box::new(effect::ClearCell(key.clone()))); effects.push(effect::mark_dirty()); @@ -187,7 +243,7 @@ impl Cmd for CommitAndAdvance { } fn execute(&self, ctx: &CmdContext) -> Vec> { let mut effects: Vec> = Vec::new(); - commit_cell_value(&self.key, &self.value, &mut effects); + commit_cell_value(ctx, &self.key, &self.value, &mut effects); match self.advance { AdvanceDir::Down => { let adv = EnterAdvance { diff --git a/src/command/cmd/core.rs b/src/command/cmd/core.rs index fa868c0..d9d2742 100644 --- a/src/command/cmd/core.rs +++ b/src/command/cmd/core.rs @@ -36,6 +36,8 @@ pub struct CmdContext<'a> { /// View navigation stacks (for drill back/forward) pub view_back_stack: &'a [String], pub view_forward_stack: &'a [String], + /// Whether the app currently has an active drill snapshot. + pub has_drill_state: bool, /// Display value at the cursor — works uniformly for pivot and records mode. pub display_value: String, /// How many data rows/cols fit on screen (for viewport scrolling). diff --git a/src/command/cmd/mod.rs b/src/command/cmd/mod.rs index aa0daf3..79191c0 100644 --- a/src/command/cmd/mod.rs +++ b/src/command/cmd/mod.rs @@ -69,6 +69,7 @@ pub(super) mod test_helpers { buffers: &EMPTY_BUFFERS, view_back_stack: &[], view_forward_stack: &[], + has_drill_state: false, display_value: { let key = layout.cell_key(sr, sc); key.as_ref() diff --git a/src/ui/app.rs b/src/ui/app.rs index 1fec708..cd9f411 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -288,6 +288,7 @@ impl App { tile_cat_idx: self.tile_cat_idx, view_back_stack: &self.view_back_stack, view_forward_stack: &self.view_forward_stack, + has_drill_state: self.drill_state.is_some(), display_value: { let key = layout.cell_key(sel_row, sel_col); if let Some(k) = &key { @@ -741,6 +742,126 @@ mod tests { ); } + /// Regression: editing the first row in a blank model's records view + /// should persist the typed value even though plain records mode does not + /// use drill state. + #[test] + fn edit_record_row_in_blank_model_persists_value() { + use crate::model::cell::CellKey; + + let mut app = App::new(Model::new("T"), None); + + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Char('5'), KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + + assert_eq!( + app.model.get_cell(&CellKey::new(vec![])), + Some(&crate::model::cell::CellValue::Number(5.0)), + "editing a synthetic row in plain records mode should write the value" + ); + } + + /// Drill-view edits should stay staged in drill state until the user + /// navigates back, at which point ApplyAndClearDrill writes them through. + #[test] + fn drill_edit_is_staged_until_view_back() { + use crate::model::cell::{CellKey, CellValue}; + + let mut m = Model::new("T"); + m.add_category("Region").unwrap(); + m.add_category("Month").unwrap(); + m.category_mut("Region").unwrap().add_item("East"); + m.category_mut("Month").unwrap().add_item("Jan"); + let record_key = CellKey::new(vec![ + ("Month".to_string(), "Jan".to_string()), + ("Region".to_string(), "East".to_string()), + ]); + m.set_cell(record_key.clone(), CellValue::Number(1.0)); + let mut app = App::new(m, None); + + app.handle_key(KeyEvent::new(KeyCode::Char('>'), KeyModifiers::NONE)) + .unwrap(); + assert!(app.drill_state.is_some(), "drill should create drill state"); + let value_col = (0..app.layout.col_count()) + .find(|&col| app.layout.col_label(col) == "Value") + .expect("drill view should include a Value column"); + app.model.active_view_mut().selected = (0, value_col); + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Char('9'), KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + + assert_eq!( + app.model.get_cell(&record_key), + Some(&CellValue::Number(1.0)), + "drill edit should remain staged until leaving the drill view" + ); + assert_eq!( + app.drill_state + .as_ref() + .and_then(|s| s.pending_edits.get(&(0, "Value".to_string()))), + Some(&"9".to_string()), + "drill edit should be recorded in pending_edits" + ); + + app.handle_key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Char('<'), KeyModifiers::NONE)) + .unwrap(); + + assert_eq!( + app.model.get_cell(&record_key), + Some(&CellValue::Number(9.0)), + "leaving drill view should apply the staged edit" + ); + } + + /// Suspected bug: blanking a records-mode category coordinate should not + /// create an item with an empty name. + #[test] + fn blanking_records_category_does_not_create_empty_item() { + use crate::model::cell::{CellKey, CellValue}; + + let mut m = Model::new("T"); + m.add_category("Region").unwrap(); + m.category_mut("Region").unwrap().add_item("East"); + m.set_cell( + CellKey::new(vec![("Region".to_string(), "East".to_string())]), + CellValue::Number(1.0), + ); + let mut app = App::new(m, None); + + app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE)) + .unwrap(); + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + for _ in 0..4 { + app.handle_key(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)) + .unwrap(); + } + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + + assert!( + !app.model + .category("Region") + .unwrap() + .items + .contains_key(""), + "records-mode edits should not create empty category items" + ); + } + #[test] fn command_mode_buffer_cleared_on_reentry() { use crossterm::event::KeyEvent; diff --git a/src/ui/effect.rs b/src/ui/effect.rs index b00d853..7617632 100644 --- a/src/ui/effect.rs +++ b/src/ui/effect.rs @@ -458,6 +458,10 @@ impl Effect for ApplyAndClearDrill { }; app.model.set_cell(orig_key.clone(), value); } else { + if new_value.is_empty() { + app.status_msg = "Record coordinates cannot be empty".to_string(); + continue; + } // Rename a coordinate: remove old cell, insert new with updated coord let value = match app.model.get_cell(orig_key) { Some(v) => v.clone(),