Merge gh/main into fix/f9qf-pin-the-shell

context/code/command.md

@@ -2510,11 +2510,14 @@ no undo. The follow-up edit is one `l` away in the column that just appeared. Li
 `add_measure` it is a schema change, so it applies immediately rather than being
 staged.
 
-`inserts_a_column` asks whether committing `key` inserts a column, invalidating
-every column index to its right. It is asked of the *layout* — `record_col` resolves
-the synthetic `_Dim` position into what the column actually is — rather than
-re-derived from the value or the cursor, so there is one answer to "what is this
-column" (gotcha 1).
+`inserts_a_column` asks whether committing `value` at `key` inserts a column,
+invalidating every column index to its right. It is asked of the *layout* —
+`record_col` resolves the synthetic `_Dim` position into what the column actually is
+— rather than re-derived from the cursor, so there is one answer to "what is this
+column" (gotcha 1). It takes the value because **an empty name creates nothing**:
+`created_name` is the one place that rule lives, read by `add_measure`,
+`add_category` and this predicate, so a commit that inserts no column cannot claim
+to (improvise-0ygd).
 
 `exit_mode_for` returns the normal-mode counterpart of an editing mode, used by
 `CommitAndAdvance` to compute the mode to land in if the advance aborts.
@@ -2545,10 +2548,28 @@ the last row `Enter` wraps to `col + 1`, which after the insert is `+cat` again,
 the editor reopened category creation and a user naming two categories in a row
 never reached the column they had made. `Tab` reached it the same way. Not advancing
 is also the *useful* answer rather than merely the safe one: `add_category` inserts
-the new column exactly where the cursor already is, so staying put lands on it. The
-measure `+` shares none of this, and not by luck — it is the last column, so there is
-no column to its right for an index to shift into, and both advances are already
-special-cased at the bottom-right edge.
+the new column exactly where the cursor already is, so staying put lands on it.
+
+**The measure `+` is the same case, and this paragraph said it was not** (corrected
+by improvise-0ygd; see `context/test-evidence/command.md`). The claim was that
+`+measure` has no column to its right for an index to shift into — false, because
+the column that shifts is `+measure` itself: naming a measure inserts the new column
+at the cursor's own index and pushes the `+` one to the right, exactly as `+cat`
+does. The claim that "both advances are already special-cased at the bottom-right
+edge" was worse than wrong, since that special case *is* the bug: `Tab` there ran
+`AddRecordRow` and jumped to `(row + 1, 0)`, manufacturing a record row and leaving
+the measure the user had just named unreachable, while `Enter` survived only because
+`EnterAdvance` aborts at the bottom-right and the `SetSelected` restore above is what
+then held the cursor. So `inserts_a_column` covers both creating columns and both
+advances suppress.
+
+What the special case is genuinely for is bulk entry, and the fix keeps it by
+splitting on the **buffer** rather than the column. `+measure` is always the
+rightmost column, so `cursor.col >= col_max` in records mode names that one cell and
+no other — the "Tab at the bottom-right starts the next record" path is reachable
+only from it, by tabbing off the end of a row onto the `+` and pressing Tab with
+nothing typed. An empty commit creates no column, so it still starts the row; a
+commit that names a measure does not.
 
 **`CommitKind`** is 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,

context/test-evidence/command.md

@@ -4452,3 +4452,47 @@ What it cannot see, stated because the gap is one edit away from mattering:
 `draw.rs::draw_welcome` builds its lines as a local array inside a ratatui
 render function, so no walk reaches them. Its `:formula` line was corrected by
 hand in the same change.
+
+### tab_naming_a_measure_lands_on_the_column_it_just_created
+
+BUG improvise-0ygd. Typing a name into the trailing `+` column and pressing
+**Tab** created the measure and then threw the user across the grid: it added a
+record row nobody asked for and parked the cursor at `(row + 1, 0)`, so the
+column the commit had just made was never reached.
+
+The cause is a **stale index**, not an abort and not an ordering. `AdvanceDir::Right`
+computes `at_bottom_right` from `CursorState::col_count`, which is the *pre-commit*
+column count — `apply_effects` rebuilds the layout only after the whole batch
+(gotcha 23). Naming a measure **inserts a column** at the cursor's own index and
+pushes `+measure` one to the right, so after the commit the cursor is no longer at
+the right edge and the bulk-entry branch is answering a question about a grid that
+no longer exists.
+
+`context/code/command.md` stated the opposite in prose — "The measure `+` shares
+none of this, and not by luck — it is the last column, so there is no column to its
+right for an index to shift into" — which is false: the column that shifts is
+`+measure` itself. That sentence is corrected in the same change.
+
+Two faults were injected against the fix:
+
+- **Revert the new-measure arm** of `inserts_a_column` (back to `NewCategory`
+  only). This test fails on the row count and the cursor; the bulk-entry sibling
+  below still passes, which is the point — a fixture on an ordinary measure column
+  cannot see a `+measure` bug, because `+measure` is *always* the last column and
+  so is the only cell from which the bottom-right branch is reachable at all.
+- **Drop the `created_name` half**, making the predicate value-blind. This test
+  passes; the sibling fails. That is the rival worth naming: it fixes the case the
+  ticket describes while eating the contract the branch exists for.
+
+### tab_on_an_empty_new_measure_column_still_starts_a_record_row
+
+The companion, and the one that stops the fix over-firing. `+measure` is always
+the rightmost column, so `cursor.col >= col_max` in records mode means *the cursor
+is on `+measure`* and nothing else — the "Tab at the bottom-right starts the next
+record" contract is reachable from that one cell. Bulk entry uses it by tabbing off
+the end of a row onto `+measure` with an empty buffer and pressing Tab again.
+
+So the discriminating input is the **buffer**, not the column: an empty commit
+creates nothing, inserts no column, and must still start the next row. Injecting
+the value-blind predicate above turns the branch off entirely and this test fails
+with `row_count` 1 where 2 is owed, while the headline test above stays green.

crates/improvise-command/src/command/cmd/commit.rs

@@ -337,6 +337,134 @@ mod tests {
         );
         assert!(dbg.contains("SetStatus"), "Expected a status, got: {dbg}");
     }
+
+    /// A one-record records grid with `Region` as its only category and `Revenue` as
+    /// its only measure: columns are `[Region, +cat, Revenue, +measure]`.
+    fn one_record_grid() -> crate::ui::app::App {
+        use crate::model::cell::{CellKey, CellValue};
+        use crate::ui::app::App;
+        use crate::view::Axis;
+        use crate::workbook::Workbook;
+
+        let mut wb = Workbook::new("Records");
+        wb.add_category("Region").unwrap();
+        wb.add_item("Region", "East").unwrap();
+        wb.add_item("_Measure", "Revenue").unwrap();
+        wb.model.set_cell(
+            CellKey::new(vec![
+                ("Region".into(), "East".into()),
+                ("_Measure".into(), "Revenue".into()),
+            ]),
+            CellValue::Number(100.0),
+        );
+        {
+            let v = wb.active_view_mut();
+            v.set_axis("_Index", Axis::Row);
+            v.set_axis("_Dim", Axis::Column);
+            v.set_axis("Region", Axis::None);
+            v.set_axis("_Measure", Axis::None);
+        }
+        let mut app = App::new(wb, None);
+        app.rebuild_layout();
+        assert!(
+            app.layout.is_records_mode(),
+            "the fixture is not records mode"
+        );
+        app
+    }
+
+    /// Put the cursor on the trailing `+` column of the last row, in records-normal.
+    fn park_on_new_measure(app: &mut crate::ui::app::App) -> (usize, usize) {
+        let bottom = app.layout.row_count() - 1;
+        let right = app.layout.col_count() - 1;
+        assert!(
+            matches!(app.layout.record_col(right), Some(RecordCol::NewMeasure)),
+            "the last column is not the new-measure column, so this proves nothing"
+        );
+        app.cursor_mut().selected = (bottom, right);
+        app.view_state.mode = AppMode::RecordsNormal;
+        (bottom, right)
+    }
+
+    fn press(app: &mut crate::ui::app::App, code: improvise_protocol::key::KeyCode) {
+        use improvise_protocol::key::{KeyEvent, KeyModifiers};
+        app.handle_key(KeyEvent {
+            code,
+            modifiers: KeyModifiers::NONE,
+        })
+        .unwrap();
+    }
+
+    /// BUG (improvise-0ygd) — see `context/test-evidence/command.md`.
+    #[test]
+    fn tab_naming_a_measure_lands_on_the_column_it_just_created() {
+        use improvise_protocol::key::KeyCode;
+
+        let mut app = one_record_grid();
+        let (bottom, right) = park_on_new_measure(&mut app);
+        let rows_before = app.layout.row_count();
+
+        press(&mut app, KeyCode::Char('i'));
+        for ch in "Cost".chars() {
+            press(&mut app, KeyCode::Char(ch));
+        }
+        press(&mut app, KeyCode::Tab);
+
+        assert!(
+            app.model_state
+                .workbook
+                .model
+                .measure_item_names()
+                .contains(&"Cost".to_string()),
+            "the measure was not created, so the cursor question is moot"
+        );
+        assert_eq!(
+            app.layout.row_count(),
+            rows_before,
+            "Tab on `+measure` manufactured a record row the user never asked for"
+        );
+        assert_eq!(
+            app.cursor().selected,
+            (bottom, right),
+            "Tab must land on the inserted measure column, not jump home"
+        );
+        assert_eq!(
+            app.layout.record_col(app.cursor().selected.1),
+            Some(&RecordCol::Measure("Cost".to_string())),
+            "the cursor is not on the measure the commit just created"
+        );
+    }
+
+    /// The bulk-entry contract the fix must not eat: an EMPTY `+` commit creates no
+    /// column, so Tab there still starts the next record row.
+    #[test]
+    fn tab_on_an_empty_new_measure_column_still_starts_a_record_row() {
+        use improvise_protocol::key::KeyCode;
+
+        let mut app = one_record_grid();
+        park_on_new_measure(&mut app);
+        let rows_before = app.layout.row_count();
+        let measures_before = app.model_state.workbook.model.measure_item_names().len();
+
+        press(&mut app, KeyCode::Char('i'));
+        press(&mut app, KeyCode::Tab);
+
+        assert_eq!(
+            app.model_state.workbook.model.measure_item_names().len(),
+            measures_before,
+            "an empty `+` commit must create no measure"
+        );
+        assert_eq!(
+            app.layout.row_count(),
+            rows_before + 1,
+            "Tab at the bottom-right of a records grid must start the next record row"
+        );
+        assert_eq!(
+            app.cursor().selected,
+            (rows_before, 0),
+            "bulk entry lands at the start of the new row"
+        );
+    }
 }
 
 // ── Commit commands (mode-specific buffer consumers) ────────────────────────
@@ -424,27 +552,29 @@ fn commit_plain_records_edit(
     }
 }
 
+/// The name a creating column's buffer holds, or `None` when it names nothing and so
+/// creates no column. The one rule; `inserts_a_column` reads the same answer.
+fn created_name(value: &str) -> Option<&str> {
+    Some(value.trim()).filter(|name| !name.is_empty())
+}
+
 /// Typing a name into the trailing column creates that measure. A schema change, so
 /// it applies immediately even inside a drill.
 fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
-    let name = value.trim();
-    if name.is_empty() {
-        return;
+    if let Some(name) = created_name(value) {
+        effects.push(Box::new(effect::AddItem {
+            category: MEASURE_CATEGORY.to_string(),
+            item: name.to_string(),
+        }));
     }
-    effects.push(Box::new(effect::AddItem {
-        category: MEASURE_CATEGORY.to_string(),
-        item: name.to_string(),
-    }));
 }
 
 /// Typing a name into `+cat` creates the category **and its column**, and writes no
 /// coordinate on the current row (improvise-2zq) — see `context/code/command.md`.
 fn add_category(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
-    let name = value.trim();
-    if name.is_empty() {
-        return;
+    if let Some(name) = created_name(value) {
+        effects.push(Box::new(effect::AddCategory(name.to_string())));
     }
-    effects.push(Box::new(effect::AddCategory(name.to_string())));
 }
 
 fn commit_cell_value(
@@ -471,12 +601,13 @@ fn commit_cell_value(
     commit_regular_cell_value(key, value, effects);
 }
 
-/// Whether committing `key` inserts a column, invalidating every index to its right.
-/// Asked of the *layout*, so there is one answer to "what is this column" (gotcha 1).
-fn inserts_a_column(ctx: &CmdContext, key: &CellKey) -> bool {
-    crate::view::synthetic_record_info(key)
-        .and_then(|(_, col_idx)| ctx.layout.record_col(col_idx))
-        .is_some_and(|col| matches!(col, RecordCol::NewCategory))
+/// Whether committing `value` at `key` inserts a column, invalidating every index to
+/// its right. Asked of the *layout* — one answer to "what is this column" (gotcha 1).
+fn inserts_a_column(ctx: &CmdContext, key: &CellKey, value: &str) -> bool {
+    created_name(value).is_some()
+        && crate::view::synthetic_record_info(key)
+            .and_then(|(_, col_idx)| ctx.layout.record_col(col_idx))
+            .is_some_and(|col| matches!(col, RecordCol::NewCategory | RecordCol::NewMeasure))
 }
 
 /// Direction to advance after committing a cell edit.
@@ -526,7 +657,7 @@ impl Cmd for CommitAndAdvance {
             self.cursor.row,
             self.cursor.col,
         )));
-        if inserts_a_column(ctx, &self.key) {
+        if inserts_a_column(ctx, &self.key, &self.value) {
             // Staying put is what lands on the inserted column (improvise-2zq); the
             // restore above is what makes "staying put" something we do, not inherit.
             effects.push(Box::new(effect::RebuildLayout));