Merge branch 'main' into worktree-improvise-ewi-formula-crate

crates/improvise-core/src/workbook.rs

@@ -138,7 +138,6 @@ impl Workbook {
             view.col_offset = 0;
         }
     }
-
 }
 
 #[cfg(test)]

crates/improvise-io/src/import/wizard.rs

@@ -222,7 +222,8 @@ impl ImportPipeline {
                     if let Some(val) = map.get(&measure.field).and_then(|v| v.as_f64()) {
                         let mut cell_coords = coords.clone();
                         cell_coords.push(("_Measure".to_string(), measure.field.clone()));
-                        wb.model.set_cell(CellKey::new(cell_coords), CellValue::Number(val));
+                        wb.model
+                            .set_cell(CellKey::new(cell_coords), CellValue::Number(val));
                     }
                 }
             }
@@ -1112,6 +1113,9 @@ mod tests {
             ("Date_Month".to_string(), "2026-03".to_string()),
             ("_Measure".to_string(), "Amount".to_string()),
         ]);
-        assert_eq!(wb.model.get_cell(&key).and_then(|v| v.as_f64()), Some(100.0));
+        assert_eq!(
+            wb.model.get_cell(&key).and_then(|v| v.as_f64()),
+            Some(100.0)
+        );
     }
 }

crates/improvise-io/src/persistence/mod.rs

@@ -578,11 +578,7 @@ fn coord_str(key: &CellKey) -> String {
         .join(", ")
 }
 
-pub fn export_csv(
+pub fn export_csv(workbook: &Workbook, view_name: &str, path: &Path) -> Result<()> {
-    workbook: &Workbook,
-    view_name: &str,
-    path: &Path,
-) -> Result<()> {
     let view = workbook
         .views
         .get(view_name)
@@ -1429,10 +1425,7 @@ Type=Food = 42
     fn category_name_with_comma_space_in_data() {
         let mut m = Workbook::new("Test");
         m.add_category("Income, Gross").unwrap();
-        m.model
+        m.model.category_mut("Income, Gross").unwrap().add_item("A");
-            .category_mut("Income, Gross")
-            .unwrap()
-            .add_item("A");
         m.add_category("Month").unwrap();
         m.model.category_mut("Month").unwrap().add_item("Jan");
         m.model.set_cell(
@@ -1602,8 +1595,7 @@ mod parser_prop_tests {
             for (i, value) in values.into_iter().enumerate() {
                 let a = &items1[i % items1.len()];
                 let b = &items2[i % items2.len()];
-                m.model
+                m.model.set_cell(coord(&[("CatA", a), ("CatB", b)]), value);
-                    .set_cell(coord(&[("CatA", a), ("CatB", b)]), value);
             }
 
             m

src/command/cmd/commit.rs

@@ -261,6 +261,16 @@ pub enum AdvanceDir {
     Right,
 }
 
+/// Return the normal-mode counterpart of an editing mode. Used by
+/// `CommitAndAdvance` to compute the mode to land in if the advance
+/// aborts (commit + exit editing at boundary).
+fn exit_mode_for(edit_mode: &AppMode) -> AppMode {
+    match edit_mode {
+        AppMode::RecordsEditing { .. } => AppMode::RecordsNormal,
+        _ => AppMode::Normal,
+    }
+}
+
 /// Commit a cell edit, advance the cursor, and re-enter edit mode.
 /// Subsumes the old `CommitCellEdit` (Down) and `CommitAndAdvanceRight` (Right).
 ///
@@ -286,6 +296,13 @@ impl Cmd for CommitAndAdvance {
     fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
         let mut effects: Vec<Box<dyn Effect>> = Vec::new();
         commit_cell_value(ctx, &self.key, &self.value, &mut effects);
+        // Pre-emptively drop to the normal counterpart of edit_mode. If the
+        // advance succeeds, the trailing `EnterEditAtCursor` below will lift
+        // us back into editing on the new cell. If the advance aborts
+        // (e.g. already at bottom-right on Enter), `EnterEditAtCursor` is
+        // skipped and we land in normal mode — which is the desired
+        // "Enter at bottom-right commits and exits" behavior.
+        effects.push(effect::change_mode(exit_mode_for(&self.edit_mode)));
         match self.advance {
             AdvanceDir::Down => {
                 let adv = EnterAdvance {

src/command/cmd/grid.rs

@@ -132,7 +132,10 @@ mod tests {
         let mut m = Workbook::new("Test");
         m.add_category("Region").unwrap();
         m.model.category_mut("Region").unwrap().add_item("East");
-        m.model.category_mut("_Measure").unwrap().add_item("Revenue");
+        m.model
+            .category_mut("_Measure")
+            .unwrap()
+            .add_item("Revenue");
         m.model.category_mut("_Measure").unwrap().add_item("Cost");
         m.model.set_cell(
             CellKey::new(vec![
@@ -148,7 +151,8 @@ mod tests {
             ]),
             CellValue::Number(600.0),
         );
-        m.model.add_formula(parse_formula("Profit = Revenue - Cost", "_Measure").unwrap());
+        m.model
+            .add_formula(parse_formula("Profit = Revenue - Cost", "_Measure").unwrap());
 
         let layout = make_layout(&m);
         let reg = make_registry();
@@ -408,8 +412,15 @@ impl Cmd for ToggleRecordsMode {
         let is_records = ctx.layout.is_records_mode();
 
         if is_records {
-            // Navigate back to the previous view (restores original axes)
+            // Leaving records mode: clean up any records with empty CellKeys
-            return vec![Box::new(effect::ViewBack), effect::set_status("Pivot mode")];
+            // (produced by AddRecordRow when no page filters are set) before
+            // restoring the previous view. This is the inverse of `SortData`
+            // that runs on entry.
+            return vec![
+                Box::new(effect::CleanEmptyRecords),
+                Box::new(effect::ViewBack),
+                effect::set_status("Pivot mode"),
+            ];
         }
 
         let mut effects: Vec<Box<dyn Effect>> = Vec::new();

src/command/cmd/navigation.rs

@@ -154,7 +154,10 @@ impl Cmd for EnterAdvance {
         } else if c < col_max {
             (0, c + 1)
         } else {
-            (r, c) // already at bottom-right; stay
+            // Already at bottom-right — the advance premise no longer holds.
+            // Abort the rest of the batch so the caller's trailing effects
+            // (e.g. `CommitAndAdvance`'s `EnterEditAtCursor`) are skipped.
+            return vec![Box::new(effect::AbortChain)];
         };
         viewport_effects(
             nr,
@@ -331,6 +334,29 @@ mod tests {
         );
     }
 
+    /// At bottom-right `EnterAdvance` has no place to go, so it emits a
+    /// single `AbortChain` effect. Trailing effects in a `CommitAndAdvance`
+    /// batch (e.g. `EnterEditAtCursor`) are then skipped, which is how
+    /// "Enter at bottom-right commits and exits editing" is realised.
+    #[test]
+    fn enter_advance_at_bottom_right_emits_abort_chain() {
+        let m = two_cat_model();
+        let layout = make_layout(&m);
+        let reg = make_registry();
+        let ctx = make_ctx(&m, &layout, &reg);
+        let mut cursor = CursorState::from_ctx(&ctx);
+        cursor.row = cursor.row_count.saturating_sub(1);
+        cursor.col = cursor.col_count.saturating_sub(1);
+        let cmd = EnterAdvance { cursor };
+        let effects = cmd.execute(&ctx);
+        assert_eq!(effects.len(), 1, "should emit exactly AbortChain");
+        let dbg = format!("{:?}", effects[0]);
+        assert!(
+            dbg.contains("AbortChain"),
+            "Expected AbortChain, got: {dbg}"
+        );
+    }
+
     #[test]
     fn law_move_to_start_idempotent() {
         let m = two_cat_model();

src/draw.rs

@@ -246,8 +246,11 @@ fn draw_content(f: &mut Frame, area: Rect, app: &App) {
         }
         if app.category_panel_open {
             let a = Rect::new(side.x, y, side.width, ph);
-            let content =
+            let content = CategoryContent::new(
-                CategoryContent::new(&app.workbook.model, app.workbook.active_view(), &app.expanded_cats);
+                &app.workbook.model,
+                app.workbook.active_view(),
+                &app.expanded_cats,
+            );
             f.render_widget(Panel::new(content, &app.mode, app.cat_panel_cursor), a);
             y += ph;
         }

src/ui/app.rs

@@ -229,6 +229,12 @@ pub struct App {
     /// Current grid layout, derived from model + view + drill_state.
     /// Rebuilt via `rebuild_layout()` after state changes.
     pub layout: GridLayout,
+    /// When set to true by an effect during `apply_effects`, the remaining
+    /// effects in the batch are skipped. The flag is reset at the start of
+    /// every `apply_effects` call. Use via the `AbortChain` effect — this is
+    /// the mechanism by which e.g. "advance at bottom-right" short-circuits
+    /// the trailing `EnterEditAtCursor` in a `CommitAndAdvance` chain.
+    pub abort_effects: bool,
     keymap_set: KeymapSet,
 }
 
@@ -272,6 +278,7 @@ impl App {
             buffers: HashMap::new(),
             transient_keymap: None,
             layout,
+            abort_effects: false,
             keymap_set: KeymapSet::default_keymaps(),
         }
     }
@@ -338,7 +345,8 @@ impl App {
             visible_rows: (self.term_height as usize).saturating_sub(8),
             visible_cols: {
                 let (fmt_comma, fmt_decimals) = parse_number_format(&view.number_format);
-                let col_widths = compute_col_widths(&self.workbook.model, layout, fmt_comma, fmt_decimals);
+                let col_widths =
+                    compute_col_widths(&self.workbook.model, layout, fmt_comma, fmt_decimals);
                 let row_header_width = compute_row_header_width(layout);
                 compute_visible_cols(
                     &col_widths,
@@ -353,8 +361,16 @@ impl App {
     }
 
     pub fn apply_effects(&mut self, effects: Vec<Box<dyn super::effect::Effect>>) {
+        self.abort_effects = false;
         for effect in effects {
             effect.apply(self);
+            if self.abort_effects {
+                // AbortChain (or another abort-setting effect) requested
+                // that the rest of this batch be skipped. Reset the flag so
+                // the next dispatch starts clean.
+                self.abort_effects = false;
+                break;
+            }
         }
         self.rebuild_layout();
     }
@@ -909,6 +925,73 @@ mod tests {
         app
     }
 
+    /// improvise-3zq (bug #2): `AddRecordRow` creates a cell with an empty
+    /// `CellKey` when no Page-axis categories supply coords — that cell
+    /// serialises as ` = 0` in .improv and re-appears on every records
+    /// toggle. Leaving records mode must clean up any such meaningless
+    /// records (inverse of the `SortData` that runs on entry).
+    #[test]
+    fn leaving_records_mode_cleans_empty_key_cells() {
+        use crate::model::cell::{CellKey, CellValue};
+        let mut app = records_model_with_two_rows();
+        // Simulate Tab-at-bottom-right having produced an empty-key cell.
+        app.workbook
+            .model
+            .set_cell(CellKey::new(vec![]), CellValue::Number(0.0));
+        assert!(
+            app.workbook
+                .model
+                .data
+                .iter_cells()
+                .any(|(k, _)| k.0.is_empty()),
+            "setup: empty-key cell should be present"
+        );
+        // Leave records mode via R.
+        app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
+            .unwrap();
+        assert!(
+            !app.layout.is_records_mode(),
+            "setup: should have left records mode"
+        );
+        assert!(
+            !app.workbook
+                .model
+                .data
+                .iter_cells()
+                .any(|(k, _)| k.0.is_empty()),
+            "empty-key records should be cleaned when leaving records mode"
+        );
+    }
+
+    /// improvise-3zq (bug #1): Enter on the bottom-right cell of records
+    /// view should commit and leave edit mode. Previously `CommitAndAdvance`
+    /// pushed an `EnterEditAtCursor` effect unconditionally, so the cursor
+    /// stayed put and we re-entered editing on the same cell.
+    #[test]
+    fn enter_at_bottom_right_of_records_view_exits_editing() {
+        let mut app = records_model_with_two_rows();
+        let last_row = app.layout.row_count() - 1;
+        let last_col = app.layout.col_count() - 1;
+        app.workbook.active_view_mut().selected = (last_row, last_col);
+
+        app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE))
+            .unwrap();
+        assert!(app.mode.is_editing(), "setup: should be editing");
+
+        app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
+            .unwrap();
+        assert!(
+            !app.mode.is_editing(),
+            "Enter at bottom-right should exit editing, got {:?}",
+            app.mode
+        );
+        assert!(
+            matches!(app.mode, AppMode::RecordsNormal),
+            "should return to RecordsNormal, got {:?}",
+            app.mode
+        );
+    }
+
     /// improvise-hmu: TAB on the bottom-right cell of records view should
     /// insert a new record below and move to the first cell of the new row
     /// in edit mode.
@@ -963,7 +1046,8 @@ mod tests {
             ("Month".to_string(), "Jan".to_string()),
             ("Region".to_string(), "East".to_string()),
         ]);
-        wb.model.set_cell(record_key.clone(), CellValue::Number(1.0));
+        wb.model
+            .set_cell(record_key.clone(), CellValue::Number(1.0));
         let mut app = App::new(wb, None);
 
         app.handle_key(KeyEvent::new(KeyCode::Char('>'), KeyModifiers::NONE))
@@ -1034,7 +1118,12 @@ mod tests {
             .unwrap();
 
         assert!(
-            !app.workbook.model.category("Region").unwrap().items.contains_key(""),
+            !app.workbook
+                .model
+                .category("Region")
+                .unwrap()
+                .items
+                .contains_key(""),
             "records-mode edits should not create empty category items"
         );
     }

src/ui/effect.rs

@@ -548,7 +548,7 @@ pub struct Save;
 impl Effect for Save {
     fn apply(&self, app: &mut App) {
         if let Some(ref path) = app.file_path {
-            match crate::persistence::save(&app.workbook,path) {
+            match crate::persistence::save(&app.workbook, path) {
                 Ok(()) => {
                     app.dirty = false;
                     app.status_msg = format!("Saved to {}", path.display());
@@ -567,7 +567,7 @@ impl Effect for Save {
 pub struct SaveAs(pub PathBuf);
 impl Effect for SaveAs {
     fn apply(&self, app: &mut App) {
-        match crate::persistence::save(&app.workbook,&self.0) {
+        match crate::persistence::save(&app.workbook, &self.0) {
             Ok(()) => {
                 app.file_path = Some(self.0.clone());
                 app.dirty = false;
@@ -927,6 +927,44 @@ impl Effect for SetPanelCursor {
     }
 }
 
+// ── Chain control ────────────────────────────────────────────────────────────
+
+/// Signals `App::apply_effects` to skip the remaining effects in the batch.
+/// The flag is reset at the start of every `apply_effects` call, so each
+/// dispatch starts clean. Use this when a sequence's premise no longer
+/// holds (e.g. "advance to next cell" at bottom-right) and later effects
+/// (e.g. "re-enter editing there") should be short-circuited.
+#[derive(Debug)]
+pub struct AbortChain;
+impl Effect for AbortChain {
+    fn apply(&self, app: &mut App) {
+        app.abort_effects = true;
+    }
+}
+
+// ── Records hygiene ──────────────────────────────────────────────────────────
+
+/// Remove cells whose `CellKey` has no coordinates — these are meaningless
+/// records that can only be produced by `AddRecordRow` when no page
+/// filters are set. Pushed by `ToggleRecordsMode` when leaving records
+/// mode, as the inverse of the `SortData` that runs on entry.
+#[derive(Debug)]
+pub struct CleanEmptyRecords;
+impl Effect for CleanEmptyRecords {
+    fn apply(&self, app: &mut App) {
+        let empties: Vec<CellKey> = app
+            .workbook
+            .model
+            .data
+            .iter_cells()
+            .filter_map(|(k, _)| if k.0.is_empty() { Some(k) } else { None })
+            .collect();
+        for key in empties {
+            app.workbook.model.clear_cell(&key);
+        }
+    }
+}
+
 // ── Convenience constructors ─────────────────────────────────────────────────
 
 pub fn mark_dirty() -> Box<dyn Effect> {
@@ -987,8 +1025,8 @@ pub fn help_page_set(page: usize) -> Box<dyn Effect> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::workbook::Workbook;
     use crate::model::cell::{CellKey, CellValue};
+    use crate::workbook::Workbook;
 
     fn test_app() -> App {
         let mut wb = Workbook::new("Test");
@@ -1048,7 +1086,10 @@ mod tests {
             ("Month".into(), "Jan".into()),
         ]);
         SetCell(key.clone(), CellValue::Number(42.0)).apply(&mut app);
-        assert_eq!(app.workbook.model.get_cell(&key), Some(&CellValue::Number(42.0)));
+        assert_eq!(
+            app.workbook.model.get_cell(&key),
+            Some(&CellValue::Number(42.0))
+        );
 
         ClearCell(key.clone()).apply(&mut app);
         assert_eq!(app.workbook.model.get_cell(&key), None);
@@ -1073,7 +1114,8 @@ mod tests {
         let mut app = test_app();
         // "Margin" does not exist as an item in "Type" before adding the formula
         assert!(
-            !app.workbook.model
+            !app.workbook
+                .model
                 .category("Type")
                 .unwrap()
                 .ordered_item_names()
@@ -1118,7 +1160,8 @@ mod tests {
         );
         // Should NOT be in the category's own items
         assert!(
-            !app.workbook.model
+            !app.workbook
+                .model
                 .category("_Measure")
                 .unwrap()
                 .ordered_item_names()
@@ -1293,6 +1336,69 @@ mod tests {
         assert_eq!(app.mode, AppMode::Help);
     }
 
+    /// `AbortChain` must cause subsequent effects in the same
+    /// `apply_effects` batch to be skipped, and the flag must reset so the
+    /// next dispatch starts clean.
+    #[test]
+    fn abort_chain_short_circuits_apply_effects() {
+        let mut app = test_app();
+        app.status_msg = String::new();
+        let effects: Vec<Box<dyn Effect>> = vec![
+            Box::new(SetStatus("before".into())),
+            Box::new(AbortChain),
+            Box::new(SetStatus("after".into())),
+        ];
+        app.apply_effects(effects);
+        assert_eq!(
+            app.status_msg, "before",
+            "effects after AbortChain must not apply"
+        );
+        assert!(
+            !app.abort_effects,
+            "abort flag should reset at end of apply_effects"
+        );
+        // A subsequent batch must not be affected by the prior abort.
+        app.apply_effects(vec![Box::new(SetStatus("next-batch".into()))]);
+        assert_eq!(app.status_msg, "next-batch");
+    }
+
+    /// `CleanEmptyRecords` removes cells whose `CellKey` has no
+    /// coordinates, and leaves all other cells untouched.
+    #[test]
+    fn clean_empty_records_removes_only_empty_key_cells() {
+        let mut app = test_app();
+        // An empty-key cell (the bug: produced by AddRecordRow when no page
+        // filters are set).
+        app.workbook
+            .model
+            .set_cell(CellKey::new(vec![]), CellValue::Number(0.0));
+        // Plus a well-formed cell that must survive.
+        let valid = CellKey::new(vec![
+            ("Type".to_string(), "Food".to_string()),
+            ("Month".to_string(), "Jan".to_string()),
+        ]);
+        app.workbook
+            .model
+            .set_cell(valid.clone(), CellValue::Number(42.0));
+        assert_eq!(app.workbook.model.data.iter_cells().count(), 2);
+
+        CleanEmptyRecords.apply(&mut app);
+
+        assert!(
+            !app.workbook
+                .model
+                .data
+                .iter_cells()
+                .any(|(k, _)| k.0.is_empty()),
+            "empty-key cell should be gone"
+        );
+        assert_eq!(
+            app.workbook.model.get_cell(&valid),
+            Some(&CellValue::Number(42.0)),
+            "valid cell must survive"
+        );
+    }
+
     /// `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
@@ -1492,7 +1598,9 @@ mod tests {
             ("Month".into(), "Jan".into()),
         ]);
         // Set original cell
-        app.workbook.model.set_cell(key.clone(), CellValue::Number(42.0));
+        app.workbook
+            .model
+            .set_cell(key.clone(), CellValue::Number(42.0));
 
         let records = vec![(key.clone(), CellValue::Number(42.0))];
         StartDrill(records).apply(&mut app);
@@ -1508,7 +1616,10 @@ mod tests {
         ApplyAndClearDrill.apply(&mut app);
         assert!(app.drill_state.is_none());
         assert!(app.dirty);
-        assert_eq!(app.workbook.model.get_cell(&key), Some(&CellValue::Number(99.0)));
+        assert_eq!(
+            app.workbook.model.get_cell(&key),
+            Some(&CellValue::Number(99.0))
+        );
     }
 
     #[test]
@@ -1518,7 +1629,9 @@ mod tests {
             ("Type".into(), "Food".into()),
             ("Month".into(), "Jan".into()),
         ]);
-        app.workbook.model.set_cell(key.clone(), CellValue::Number(42.0));
+        app.workbook
+            .model
+            .set_cell(key.clone(), CellValue::Number(42.0));
 
         let records = vec![(key.clone(), CellValue::Number(42.0))];
         StartDrill(records).apply(&mut app);
@@ -1540,7 +1653,10 @@ mod tests {
             ("Type".into(), "Drink".into()),
             ("Month".into(), "Jan".into()),
         ]);
-        assert_eq!(app.workbook.model.get_cell(&new_key), Some(&CellValue::Number(42.0)));
+        assert_eq!(
+            app.workbook.model.get_cell(&new_key),
+            Some(&CellValue::Number(42.0))
+        );
         // "Drink" should have been added as an item
         let items: Vec<&str> = app
             .workbook
@@ -1560,7 +1676,9 @@ mod tests {
             ("Type".into(), "Food".into()),
             ("Month".into(), "Jan".into()),
         ]);
-        app.workbook.model.set_cell(key.clone(), CellValue::Number(42.0));
+        app.workbook
+            .model
+            .set_cell(key.clone(), CellValue::Number(42.0));
 
         let records = vec![(key.clone(), CellValue::Number(42.0))];
         StartDrill(records).apply(&mut app);
@@ -1640,7 +1758,10 @@ mod tests {
             item: "Food".to_string(),
         }
         .apply(&mut app);
-        assert_eq!(app.workbook.active_view().page_selection("Type"), Some("Food"));
+        assert_eq!(
+            app.workbook.active_view().page_selection("Type"),
+            Some("Food")
+        );
     }
 
     // ── Hide/show items ─────────────────────────────────────────────────

src/ui/grid.rs

@@ -915,7 +915,10 @@ mod tests {
     fn formula_cell_renders_computed_value() {
         let mut m = Workbook::new("Test");
         m.add_category("Region").unwrap(); // → Column
-        m.model.category_mut("_Measure").unwrap().add_item("Revenue");
+        m.model
+            .category_mut("_Measure")
+            .unwrap()
+            .add_item("Revenue");
         m.model.category_mut("_Measure").unwrap().add_item("Cost");
         // Profit is a formula target — dynamically included in _Measure
         m.model.category_mut("Region").unwrap().add_item("East");
@@ -927,7 +930,8 @@ mod tests {
             coord(&[("_Measure", "Cost"), ("Region", "East")]),
             CellValue::Number(600.0),
         );
-        m.model.add_formula(parse_formula("Profit = Revenue - Cost", "_Measure").unwrap());
+        m.model
+            .add_formula(parse_formula("Profit = Revenue - Cost", "_Measure").unwrap());
         m.active_view_mut()
             .set_axis("_Index", crate::view::Axis::None);
         m.active_view_mut()

src/ui/tile_bar.rs

@@ -18,12 +18,7 @@ pub struct TileBar<'a> {
 }
 
 impl<'a> TileBar<'a> {
-    pub fn new(
+    pub fn new(model: &'a Model, view: &'a View, mode: &'a AppMode, tile_cat_idx: usize) -> Self {
-        model: &'a Model,
-        view: &'a View,
-        mode: &'a AppMode,
-        tile_cat_idx: usize,
-    ) -> Self {
         Self {
             model,
             view,