From d5201d984b104c02d1dbe01b06ca42e5c6d7c2ac Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 15:43:09 -0700 Subject: [PATCH] fix(command): naming a measure inserts a column, so Tab must not advance (improvise-0ygd) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typing a name into the trailing `+` column of a records grid 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), leaving the column the commit had just made unreachable. Enter on the same cell was correct. The cause is a stale index, not an abort and not an ordering. `AdvanceDir::Right` reads `at_bottom_right` off `CursorState::col_count`, the PRE-commit column count — `apply_effects` rebuilds the layout only after the whole batch (gotcha 23). A `+measure` commit inserts its column at the cursor's own index and pushes the `+` one to the right, exactly as `+cat` does, so after the commit the cursor is no longer at the right edge. `context/code/command.md` argued the opposite in prose: that the measure `+` has no column to its right for an index to shift into, and that both advances are already special-cased at the bottom-right edge. The first is false — the column that shifts is `+measure` itself — and the second names the bug rather than a safeguard. That paragraph is corrected here. `inserts_a_column` therefore covers both creating columns, and takes the value, because an empty name creates nothing: `created_name` is now the one home of that rule, read by `add_measure`, `add_category` and the predicate. Splitting on the buffer rather than the column is what keeps bulk entry, whose Tab-at-the-bottom- right path is reachable only from `+measure` — it is always the rightmost column — by tabbing onto the `+` with nothing typed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 39 +++- context/test-evidence/command.md | 44 +++++ .../src/command/cmd/commit.rs | 167 ++++++++++++++++-- 3 files changed, 223 insertions(+), 27 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 572169fb..4a3ed7ad 100644 --- a/context/code/command.md +++ b/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, diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 2602bf7d..2529868f 100644 --- a/context/test-evidence/command.md +++ b/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. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index b454bd63..ce9511ca 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/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>) { - 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>) { - 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));