From 472f7e5548d77f59adfb256bdaa233e829cc15ee Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:38:08 -0700 Subject: [PATCH 1/9] =?UTF-8?q?command:=20one=20Panel=E2=86=92flag=20mappi?= =?UTF-8?q?ng,=20and=20a=20guard=20that=20can=20see=20a=20permutation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-y4xg and improvise-qsmp are two shapes of one defect. `registry.rs` mapped `Panel` to a `CmdContext` field in two places and `App::cmd_context` copies the three flags out of `ViewState` in a third, and all three were demonstrated unguarded: swapping Formula and Category at the registry sites left the workspace at 1400 passing, and hardcoding the `category_panel_open` propagator to `true` left it at 1394. The registry mapping is gone — both sites call `CmdContext::panel_open(panel)`. The remaining two permutable sites are guarded from a path neither controls: the tests write `view_state`'s three fields **by name** and read back through the enum accessor on a context built by the real `App::cmd_context`. PR #70's review caught the weaker shape of this, where zeroing and reading through the same accessors passed a consistent permutation. Both faults were injected and both fail all three new tests by name. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- .../improvise-command/src/command/cmd/core.rs | 10 ++ .../src/command/cmd/registry.rs | 115 ++++++++++++++++-- 2 files changed, 115 insertions(+), 10 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 56a5d897..04024746 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -81,6 +81,16 @@ impl<'a> CmdContext<'a> { .and_then(crate::view::synthetic_record_info) } + /// Whether `panel` is open. The one `Panel` → flag mapping on this side of + /// the wall, mirroring `ViewState::panel_open` (improvise-y4xg). + pub fn panel_open(&self, panel: Panel) -> bool { + match panel { + Panel::Formula => self.formula_panel_open, + Panel::Category => self.category_panel_open, + Panel::View => self.view_panel_open, + } + } + pub fn row_count(&self) -> usize { self.layout.row_count() } diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index f47b7d35..d288c0bc 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -355,11 +355,7 @@ pub fn default_registry() -> CmdRegistry { require_args("toggle-panel-and-focus", args, 1)?; let panel = parse_panel(&args[0])?; // Default interactive: if already open+focused -> close, else open+focus - let currently_open = match panel { - Panel::Formula => ctx.formula_panel_open, - Panel::Category => ctx.category_panel_open, - Panel::View => ctx.view_panel_open, - }; + let currently_open = ctx.panel_open(panel); let currently_focused = ctx.mode.panel() == Some(panel); let (open, focused) = if currently_open && currently_focused { (false, false) // close @@ -389,11 +385,7 @@ pub fn default_registry() -> CmdRegistry { |args, ctx| { require_args("toggle-panel-visibility", args, 1)?; let panel = parse_panel(&args[0])?; - let currently_open = match panel { - Panel::Formula => ctx.formula_panel_open, - Panel::Category => ctx.category_panel_open, - Panel::View => ctx.view_panel_open, - }; + let currently_open = ctx.panel_open(panel); Ok(Box::new(TogglePanelVisibility { panel, currently_open, @@ -725,3 +717,106 @@ mod tests { } } } + +#[cfg(test)] +mod panel_open_tests { + use super::*; + use crate::command::cmd::test_helpers::two_cat_model; + use crate::command::key::{KeyCode, KeyModifiers}; + use crate::ui::app::{App, AppMode}; + + /// Write the raw field each `Panel` names. Spelling the fields here is what + /// gives the read side a second path — context/test-evidence/command.md. + fn open_only(app: &mut App, panel: Panel) { + app.view_state.formula_panel_open = panel == Panel::Formula; + app.view_state.category_panel_open = panel == Panel::Category; + app.view_state.view_panel_open = panel == Panel::View; + } + + fn panel_mode(panel: Panel) -> AppMode { + match panel { + Panel::Formula => AppMode::FormulaPanel, + Panel::Category => AppMode::CategoryPanel, + Panel::View => AppMode::ViewPanel, + } + } + + #[test] + fn the_context_reports_each_panels_own_open_flag() { + for opened in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + open_only(&mut app, opened); + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + for asked in Panel::ALL { + assert_eq!( + ctx.panel_open(asked), + asked == opened, + "with only {opened:?} open, the context answers wrongly for {asked:?}" + ); + } + } + } + + #[test] + fn toggle_panel_visibility_reads_the_panel_it_names() { + for opened in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + open_only(&mut app, opened); + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + for asked in Panel::ALL { + let args = [asked.name().to_string()]; + let cmd = ctx + .registry + .interactive("toggle-panel-visibility", &args, &ctx) + .expect("toggle-panel-visibility resolves"); + let dbg = format!("{cmd:?}"); + let expected = format!("currently_open: {}", asked == opened); + assert!( + dbg.contains(&expected), + "with only {opened:?} open, toggling {asked:?} read another \ + panel's flag: {dbg}" + ); + } + } + } + + #[test] + fn a_focused_open_panel_closes_and_a_closed_one_opens() { + for panel in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + open_only(&mut app, panel); + app.view_state.mode = panel_mode(panel); + let args = [panel.name().to_string()]; + + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + ); + assert!( + dbg.contains("open: false"), + "{panel:?} is open and focused, so its key must close it: {dbg}" + ); + + let mut app = App::new(two_cat_model(), None); + app.view_state.formula_panel_open = false; + app.view_state.category_panel_open = false; + app.view_state.view_panel_open = false; + app.view_state.mode = panel_mode(panel); + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + ); + assert!( + dbg.contains("open: true"), + "{panel:?} is closed, so its own key must open it rather than \ + leave it unopenable: {dbg}" + ); + } + } +} From 1ea067c9ab4f71455f51073db73969901e2a0427 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:38:18 -0700 Subject: [PATCH 2/9] =?UTF-8?q?command:=20dirtiness,=20aborts=20and=20the?= =?UTF-8?q?=20cursor=20=E2=80=94=20five=20edges=20in=20the=20effect=20laye?= =?UTF-8?q?r?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-748f — a drill exited with no edits, and view-back on a document with no drill, marked it dirty. `Effect::kind` takes no `App`, so the classification cannot be state-dependent; the answer is to observe. The ticket proposed observing the per-record raw-cell fingerprint, and that is **unsound**: five of the ten `CellData` effects change the document without writing a raw cell — `AddItem`, `AddItemInGroup`, `SortData` (order is in the file), `AddFormula` and `RemoveFormula`. Observing raw cells would have left every one reading clean, which is a lost edit rather than a spurious prompt. So `document_config` is widened into `document_content` — cells in iteration order and formulas as well as the categories, items, groups and view configuration it already carried — and `CellData` joins `LayoutGeometry` in being read against it, in the loop, at the position gotcha 45 requires. `WholeWorkbook` stays declared, and that is the remaining over-report: a `LoadModel` does not move `file_path`, so the in-memory document really does differ from the file the path names. `start_drill_and_apply_clear_drill_with_no_edits` pinned the old behaviour and is inverted. improvise-z05r — `CommitExport` did not bail on an empty path where `CommitBuffer`'s three kinds all do, and `ExportCsv` did not abort the batch where `Save` and `SaveAs` both do. All three writers now report through one `report_file_write`, and the test names the three so a fourth has to come there. The commit reorders its batch too: the mode change goes at the head, because an abort at the tail strands the user in `ExportPrompt`, where the minibuffer is drawn *instead of* the status line and the export error is never seen (gotcha 21). improvise-7kvw — `SetPageSelection` renumbers the grid under `prune_empty`, because which rows survive pruning is a function of the page. It now resets the cursor in exactly that case, and only when the selection really moved: sending the cursor home on every `[`/`]` with pruning off is worse than the hole. improvise-pubt — the ticket argued the drill arm of `display_value_in` was probably unreachable with a foreign layout. It is reachable: records mode auto-detects from the axes, so an ordinary saved view gives `GridLayout::new` a records layout whose `cell_key` returns the synthetic key the arm matches. The test fails on the old code. `staged_drill_edit` now consults `drill_state` only for the host's own layout, by pointer identity, which is the question `grid_in` is actually asking. improvise-h074 — `resolve_view`'s fallback carries back the `.expect` message `Workbook::active_view()` had. The second half, the host-side `DeleteView` bypass of `adopt_deleted_views`, is untouched: it is a decision about `Sessions`/`Host`. Every guard here was proved by injecting the fault it names and watching it fail by name; `context/test-evidence/command.md` records each injection. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 202 ++++++++++++ .../src/command/cmd/commit.rs | 38 ++- crates/improvise-command/src/ui/app.rs | 307 ++++++++++++++++-- crates/improvise-command/src/ui/effect.rs | 187 ++++++++--- 4 files changed, 654 insertions(+), 80 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 5d469e4e..bf5d8567 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -403,6 +403,74 @@ than `effect.apply(&mut app)`. That is deliberate: the dirty flag is raised off the `EffectKind` now (improvise-gv9r, gotcha 45), so driving the effect directly would assert a flag nothing in production sets. +### every_file_writing_effect_aborts_the_batch_on_failure + +improvise-z05r, the **third** instance of one rule: improvise-8tk fixed `Save` and +not `SaveAs`; PR #27 found `SaveAs`; `ExportCsv` was the arm still missing, so a +`Binding::Sequence` or a `:` line composing an export with anything after it +continued past a failed write. + +The fix is structural rather than a third patch: all three now report through one +`report_file_write(app, outcome)`, which writes the status message either way and +sets `abort_effects` on `Err`. That is not sufficient by itself — a fourth writer +could still be written without calling it — so the test names the three, and a +fourth has to come here and be named. Naming is the mechanism; the shared function +is what makes the naming cheap to honour. + +Each case drives `App::apply_effects` with `[writer, ChangeMode(Quit)]` against a +path under a directory that does not exist, and asserts both `last_batch_aborted()` +and that the trailing mode change never landed. + +Injected fault: drop `app.abort_effects = true` from `report_file_write`. Fails by +name, along with the four older abort tests — which is the point: they and this one +now share one mechanism. + +Injected fault, narrower: route `ExportCsv` around the reporter and write its status +directly. Only this test fails, naming `ExportCsv`, with 460 passing — the exact +state the ticket describes. + +### a_batch_that_writes_nothing_leaves_the_document_clean + +improvise-748f. `ViewNavigate { forward: false }` emits `ApplyAndClearDrill` +unconditionally, and that effect returns immediately when `drill_state` is `None`. +It declares `Model(CellData)` regardless — `Effect::kind` takes no `App`, so the +classification cannot be state-dependent — so `apply_effects` raised +`model_state.dirty` on a document nothing had touched. Press view-back, press `:q`, +and the quit is refused; autosave arms thirty seconds later. + +The older test here, `start_drill_and_apply_clear_drill_with_no_edits`, pinned the +wrong behaviour in an assertion message arguing it was the same over-approximation a +failed `LoadModel` gets. It is not: a succeeding `LoadModel` leaves a document that +really does differ from the file its path names, and that remains declared. This was +a wrong answer to no divergence at all. + +### a_cell_data_effect_no_raw_cell_records_still_dirties_the_document + +The guard against the cure improvise-748f **proposed**, which is unsound. The ticket +suggested raising `dirty` off `bump_changed_records`' per-record fingerprint of the +raw cells, on the reading that `CellData` means cells. Five of the ten `CellData` +effects change the document without writing a raw cell: `AddItem`, `AddItemInGroup` +and `SortData` (which reorders — and `.improv` carries the order), and `AddFormula` +and `RemoveFormula`, whose results live in `Model::formula_cache` and not in the +store. Observing raw cells would have left every one of them reading clean, which is +a *lost edit* — the opposite direction from the spurious prompt it was fixing. + +So the observation is `document_content`, the former `document_config` widened to +cover the cells in iteration order and the formulas as well as the categories, items, +groups and view configuration it already carried. `CellData` and `LayoutGeometry` are +both read against it, in the loop, at the position gotcha 45 requires. +`WholeWorkbook` stays *declared*, and that is the remaining over-report: a `LoadModel` +whose incoming content happens to be identical still marks the document dirty, and a +failed one does too. Both cost one redundant write. + +The `SortData` case needs two cells inserted out of sorted order, or it reorders +nothing and its case asserts the opposite of what it names. The `AddFormula` case +removes the fixture's formula first, or adding it is a no-op. + +Injected fault: narrow `document_content` back to the configuration it hashed before. +Fails naming `SortData`, with five other dirty-flag tests. + + --- ## `ui/app.rs` @@ -862,6 +930,79 @@ The counters are shared document state on `App`, never on the portable session slice (gotcha 30): a client inheriting another's idea of how far the document has moved is exactly the stale basis this machinery exists to catch. +### a_page_flip_under_prune_empty_invalidates_the_cursor + +improvise-7kvw, the case improvise-9mo9 decided the other nine and deliberately left +open. `SetPageSelection` changes which records are visible and not which categories +frame them, so a grid position normally keeps naming the same cell. Under +`View::prune_empty` that is false: which rows survive pruning is a function of the +page selection, so a page flip renumbers the grid while neither `reset_cursor` nor +`cursor_epoch` moved — gotcha 24's failure in both directions at once, the host's own +cursor stale and no client told. + +9mo9 did not patch it because `[`/`]` paging is constant navigation and sending the +cursor home on every flip is worse than the hole when `prune_empty` is off, which is +the default. So the reset is conditional on `prune_empty`, and on the selection +actually moving. + +The fixture has three regions and two quarters whose non-empty rows differ, so +position 0 names `North` on Q1 and `South` on Q2. The test asserts the row labels +really differ before asserting the epoch moved; the negative arm asserts they really +*do not* differ with pruning off, so neither case can pass vacuously. + +Injected fault: drop the reset. The prune arm fails. Injected fault: make the reset +unconditional. The no-prune arm fails. + +### a_page_flip_without_prune_empty_leaves_the_cursor_alone + +The negative arm of the above, and it is load-bearing for the same reason gotcha 24 +gives for `SetNumberFormat`: without it, "reset on every page flip" passes the +positive test and costs the user their position on every `[`/`]`. + +### a_foreign_layout_is_not_shown_the_hosts_staged_drill_text + +improvise-pubt, filed with an argument for unreachability that the test disproves. +`project::cell_display` fills `edit_text` from `App::display_value_in(layout, key)`, +and since improvise-jytm that layout may be another client's. +`drill_state.pending_edits` are keyed by `(row index, RecordCol)` against the +**host's** drill layout (gotcha 1), and a row index in one layout names nothing in +another. + +The ticket read the arm as unreachable because "a foreign layout built by +`GridLayout::new` carries no frozen records, so it should not produce a synthetic +key". That is wrong: records mode **auto-detects** from the axes (gotcha 1), so an +ordinary saved view with `_Index` on Row and `_Dim` on Column gives `GridLayout::new` +a records layout whose `cell_key` returns exactly the synthetic `(_Index, _Dim)` key +the drill arm matches on. The test fails on the original code, naming the host's +staged text read out of a layout it was never staged against. + +The fix is `App::staged_drill_edit`, which consults `drill_state` only when +`std::ptr::eq(layout, &self.layout)`. A pointer comparison rather than a name +comparison because `grid_in` reuses `&app.layout` by reference where the view names +agree and derives a fresh one otherwise, so identity *is* the question being asked. +Whether the host's pending drill text should reach any other client at all is +improvise-tom1 and is untouched here. + +The positive arm — the host's own layout still shows `STAGED` — is in the same test, +or "never consult the drill" passes, which would blank the TUI's own drill editor. +The drill fixture needs a real `RecordRow`, not an empty `Vec`: the host's layout +iterates the frozen snapshot, so an empty one gives a zero-row grid and the positive +arm cannot be built. + +### a_default_that_names_nothing_says_which_invariant_broke + +improvise-h074. `App::resolve_view` falls back to `workbook.active_view` for a name +the document has not got and then indexes `workbook.views[default]`. Several of these +call sites previously went through `Workbook::active_view()`, which carries +`.expect("active_view always names an existing view")`; `IndexMap`'s `Index` impl +panics with no message at all, so a workbook whose default names nothing — reachable +only by constructing the struct directly, since `create_view`/`delete_view` maintain +it — failed with a bare index panic where it used to name the broken invariant. + +Injected fault: restore `&workbook.views[default]`. The test fails on the expected +substring. + + --- ## `ui/project.rs` @@ -2180,6 +2321,21 @@ An empty prompt must not reach `SaveAs`: `SaveAs(PathBuf::from(""))` asks the backend to write to `""`, and the user then reads whatever I/O error that produces instead of "you typed nothing". +### commit_export_bails_on_an_empty_path + +improvise-z05r's first arm. `CommitExport` did not bail on an empty path where +`CommitBuffer`'s three kinds all do (gotcha 20: buffer to trim to bail-on-empty to +dirty to status is the shared spine), so Enter on an empty export prompt asked the +backend to write to `""` and the user read whatever I/O error that produced. + +The same change reorders the batch: the mode change now comes **first**, because +`ExportCsv` aborts on a failed write as of z05r's other arm, and an abort at the tail +would strand the user in `ExportPrompt` — where `draw_bottom_bar` renders the +minibuffer *instead of* the status line, so the export error is never drawn at all +and the operation appears to do nothing (gotcha 21). That ordering is asserted in +`commit_export_produces_export_and_normal_mode`, by index and not by presence. + + --- ## `command/cmd/fit.rs` @@ -2362,6 +2518,52 @@ second entry under "view-back" and removes "view-forward" from the registry entirely; `parse` takes the first match, so every surviving entry still agrees with its own name. Uniqueness is what catches it. +### the_context_reports_each_panels_own_open_flag + +improvise-y4xg and improvise-qsmp, two shapes of one defect. `registry.rs` mapped +`Panel` to a `CmdContext` field in two places, and `App::cmd_context` copies the +three flags out of `ViewState` in a third. **Both were demonstrated unguarded**: +swapping Formula and Category at the two registry sites left the workspace at 1400 +passing, and hardcoding the `category_panel_open` propagator to `true` left it at +1394. + +Nothing exercised either, because `cmd/mod.rs::test_helpers` sets the panel flags +explicitly on the context it builds — the command tests construct the value they then +assert on, so no test ever drove the real propagator. + +The registry mapping is gone: both sites call `CmdContext::panel_open(panel)`, the +one mapping on this side of the wall. That leaves two permutable sites, and the test +is aimed at both from a path neither controls — it writes `view_state`'s three fields +**by name** and reads back through the enum accessor on a context built by the real +`App::cmd_context`. PR #70's review caught the weaker shape of this: a panel test that +zeroed and read through the same four accessors passed a consistent permutation. + +Injected fault: swap `Panel::Formula` and `Panel::Category` in `panel_open`. Fails. +Injected fault: hardcode the `category_panel_open` propagator to `true`. Fails. + +### toggle_panel_visibility_reads_the_panel_it_names + +The behavioural half, and the one that reaches the registry. Its interactive arm +passes `currently_open` straight through, so with exactly one panel open the command +built for each of the three names carries its own panel's flag. Both injections above +fail here too. + +### a_focused_open_panel_closes_and_a_closed_one_opens + +improvise-qsmp's own scenario: `toggle-panel-and-focus` decides close only when the +panel is *both* open and focused, so the case that separates a right propagator from +a stuck-true one needs the mode set to the panel's own. With the flag stuck true, +pressing a panel's key on a closed panel decides to *close* it and the panel becomes +unopenable by its own key — which is what this test's second arm names. + +### Helper — `open_only` + +Not a test. It spells `formula_panel_open`, `category_panel_open` and +`view_panel_open` as fields rather than looping over `Panel::ALL` through an +accessor, and that is the whole evidentiary value: a permutation in any mapping +between the `Panel` enum and those three booleans has to disagree with this helper. + + --- ## `command/cmd/text_buffer.rs` diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index c3be37c7..803d3f13 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -195,6 +195,32 @@ mod tests { let dbg = effects_debug(&effects); assert!(dbg.contains("ExportCsv"), "Expected ExportCsv, got: {dbg}"); assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}"); + assert!( + dbg.find("Normal").unwrap() < dbg.find("ExportCsv").unwrap(), + "the mode change must precede the write an abort can reach: {dbg}" + ); + } + + /// BUG (improvise-z05r): the fourth commit kind did not bail on an empty + /// path, so Enter on an empty prompt exported to "". + #[test] + fn commit_export_bails_on_an_empty_path() { + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + for buffered in ["", " "] { + let mut bufs = HashMap::new(); + bufs.insert("export".to_string(), buffered.to_string()); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.buffers = &bufs; + let effects = CommitExport.execute(&ctx); + let dbg = effects_debug(&effects); + assert!( + !dbg.contains("ExportCsv"), + "an empty export path must write nothing, got: {dbg}" + ); + assert!(dbg.contains("Normal"), "Expected Normal mode, got: {dbg}"); + } } /// Naming a new measure in the trailing `+` column at the bottom-right sent the @@ -696,10 +722,18 @@ impl Cmd for CommitExport { "commit-export" } fn execute(&self, ctx: &CmdContext) -> Vec> { - let buf = ctx.buffers.get("export").cloned().unwrap_or_default(); + let path = read_buffer(ctx, "export").trim().to_string(); + if path.is_empty() { + return vec![ + effect::set_status("No path given — not exported"), + effect::change_mode(AppMode::Normal), + ]; + } + // The mode change comes **first**, or a failed export aborts with the + // minibuffer still drawn over its error (gotcha 21). vec![ - Box::new(effect::ExportCsv(std::path::PathBuf::from(buf))), effect::change_mode(AppMode::Normal), + Box::new(effect::ExportCsv(std::path::PathBuf::from(path))), ] } } diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index a7e16c2d..dbb9f9c8 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -80,11 +80,20 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap { .collect() } -/// Everything a `LayoutGeometry` effect can write that the file would carry. Not the -/// cell data — `record_fingerprints` answers for that — so it is cheap per batch. -fn document_config(workbook: &Workbook) -> u64 { +/// Everything a model effect can write that the file would carry — cells and +/// formulas included, because `CellData` is wider than its name (improvise-748f). +fn document_content(workbook: &Workbook) -> u64 { use std::hash::{Hash, Hasher}; let mut h = std::collections::hash_map::DefaultHasher::new(); + for (key, value) in workbook.model.data.iter_cells() { + key.hash(&mut h); + hash_value(value, &mut h); + } + for formula in workbook.model.formulas() { + formula.raw.hash(&mut h); + formula.target.hash(&mut h); + formula.target_category.hash(&mut h); + } for (name, cat) in &workbook.model.categories { name.hash(&mut h); cat.kind.hash(&mut h); @@ -131,6 +140,26 @@ fn sorted_pairs( out } +/// Exhaustive, and by bits rather than by `Debug`: two values that print alike +/// must not fingerprint alike. +fn hash_value(value: &CellValue, hasher: &mut impl std::hash::Hasher) { + use std::hash::Hash; + match value { + CellValue::Number(n) => { + 0u8.hash(hasher); + n.to_bits().hash(hasher); + } + CellValue::Text(t) => { + 1u8.hash(hasher); + t.hash(hasher); + } + CellValue::Error(e) => { + 2u8.hash(hasher); + e.hash(hasher); + } + } +} + /// A per-record XOR fingerprint of the **raw** cells it holds. Process-local and /// never on a wire, unlike [`RecordHash`] — see `context/code/command.md`. fn record_fingerprints(data: &DataStore) -> std::collections::HashMap { @@ -139,22 +168,7 @@ fn record_fingerprints(data: &DataStore) -> std::collections::HashMap { - 0u8.hash(&mut hasher); - n.to_bits().hash(&mut hasher); - } - CellValue::Text(t) => { - 1u8.hash(&mut hasher); - t.hash(&mut hasher); - } - CellValue::Error(e) => { - 2u8.hash(&mut hasher); - e.hash(&mut hasher); - } - } + hash_value(value, &mut hasher); *out.entry(RecordHash::of(&key)).or_insert(0) ^= hasher.finish(); } out @@ -413,7 +427,13 @@ impl App { Some((key, view)) => (key.as_str(), view), None => { let default = workbook.active_view.as_str(); - (default, &workbook.views[default]) + // The message `Workbook::active_view()` carried: `IndexMap`'s + // `Index` panics bare, naming no invariant (improvise-h074). + let view = workbook + .views + .get(default) + .expect("active_view always names an existing view"); + (default, view) } } } @@ -488,13 +508,8 @@ impl App { /// The same text against a layout that may be another client's (improvise-jytm). pub(crate) fn display_value_in(&self, layout: &GridLayout, key: &CellKey) -> String { match crate::view::synthetic_record_info(key) { - Some((idx, col)) => layout - .record_col(col) - .and_then(|rc| { - self.drill_state - .as_ref() - .and_then(|s| s.pending_edits.get(&(idx, rc.clone())).cloned()) - }) + Some((idx, col)) => self + .staged_drill_edit(layout, idx, col) .or_else(|| layout.resolve_display(key)) .unwrap_or_default(), None => self @@ -507,6 +522,18 @@ impl App { } } + /// The host's own staged drill text at a records position, and only for the + /// host's own layout — a row index names nothing in another (improvise-pubt). + fn staged_drill_edit(&self, layout: &GridLayout, idx: usize, col: usize) -> Option { + let own = std::ptr::eq(layout, &self.layout); + let col = layout.record_col(col).filter(|_| own)?; + self.drill_state + .as_ref()? + .pending_edits + .get(&(idx, col.clone())) + .cloned() + } + pub fn cmd_context(&self, key: KeyCode, _mods: KeyModifiers) -> CmdContext<'_> { let view = self.view(); let layout = &self.layout; @@ -575,7 +602,7 @@ impl App { .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))) .then(|| record_fingerprints(&self.model_state.workbook.model.data)); let shapes_before = touches_model.then(|| view_shapes(&self.model_state.workbook)); - let mut config_basis = touches_model.then(|| document_config(&self.model_state.workbook)); + let mut content_basis = touches_model.then(|| document_content(&self.model_state.workbook)); let mut dirty = Dirty::NOTHING; for effect in effects { let kind = effect.kind(); @@ -585,11 +612,11 @@ impl App { // rather than defaulting into the observed half by omission. let declared_dirty = match kind { EffectKind::ViewOnly => false, - // Things no structural fingerprint sees: a formula, a sort order, - // a replaced document. - EffectKind::Model(ModelChange::CellData) => true, + // A replaced document differs from the file its path names even + // where the content matches, so it is declared (improvise-748f). EffectKind::Model(ModelChange::WholeWorkbook) => true, - // Observed instead, in `observe_reshape` (improvise-75ld). + // Observed below, against `document_content`. + EffectKind::Model(ModelChange::CellData) => false, EffectKind::Model(ModelChange::LayoutGeometry) => false, }; if declared_dirty { @@ -597,12 +624,14 @@ impl App { } // The observed half, read **inside the loop** so `Save` still clears it // in loop order (gotcha 45) — the position is the property. - if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) - && let Some(basis) = config_basis + if matches!( + kind, + EffectKind::Model(ModelChange::CellData | ModelChange::LayoutGeometry) + ) && let Some(basis) = content_basis { - let now = document_config(&self.model_state.workbook); + let now = document_content(&self.model_state.workbook); self.model_state.dirty |= now != basis; - config_basis = Some(now); + content_basis = Some(now); } if self.abort_effects { // An abort-setting effect asked for the rest to be skipped. Reset @@ -939,6 +968,212 @@ mod tests { ); } + /// BUG (improvise-pubt): `pending_edits` are keyed by a row index into the + /// host's drill, and a position in one layout names nothing in another. + #[test] + fn a_foreign_layout_is_not_shown_the_hosts_staged_drill_text() { + use crate::model::cell::{CellKey, CellValue}; + use crate::view::{Axis, GridLayout, RecordCol}; + use std::rc::Rc; + + let mut wb = Workbook::new("Drill"); + 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(7.0), + ); + wb.create_view("Records"); + { + let v = &mut wb.views["Records"]; + 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); + + let foreign = GridLayout::new( + &app.model_state.workbook.model, + &app.model_state.workbook.views["Records"], + ); + assert!( + foreign.is_records_mode(), + "the other client's view is not records mode, so no synthetic key exists" + ); + let col = (0..foreign.col_count()) + .find( + |c| matches!(foreign.record_col(*c), Some(RecordCol::Measure(m)) if m == "Revenue"), + ) + .expect("the records grid has a Revenue column"); + let key = foreign + .cell_key(0, col) + .expect("the records grid names a cell at (0, Revenue)"); + assert!( + crate::view::synthetic_record_info(&key).is_some(), + "the foreign layout produced no synthetic key, so the drill arm is unreachable" + ); + + let mut pending = std::collections::HashMap::new(); + pending.insert( + (0, RecordCol::Measure("Revenue".into())), + "STAGED".to_string(), + ); + let record = crate::view::RecordRow { + coords: CellKey::new(vec![("Region".into(), "East".into())]), + values: vec![("Revenue".to_string(), CellValue::Number(7.0))], + }; + app.drill_state = Some(DrillState { + records: Rc::new(vec![record]), + pending_edits: pending, + }); + + assert_ne!( + app.display_value_in(&foreign, &key), + "STAGED", + "another client was shown the host's uncommitted drill text against \ + coordinates it was never staged for" + ); + + // The positive arm: on the host's own layout the staged text is exactly + // what the editor must open with, or "never consult the drill" passes. + app.switch_to_view("Records"); + app.rebuild_layout(); + let own_key = app + .layout + .cell_key(0, col) + .expect("the host's own records grid names a cell at (0, Revenue)"); + assert_eq!( + app.display_value_of(&own_key), + "STAGED", + "the host's own staged drill text was dropped" + ); + } + + /// Two pages whose non-empty rows differ, so pruning renumbers the grid when + /// the page flips — improvise-7kvw. `prune_empty` is the caller's choice. + fn paged_model(prune: bool) -> App { + use crate::model::cell::{CellKey, CellValue}; + use crate::view::Axis; + let mut wb = Workbook::new("Paged"); + wb.add_category("Region").unwrap(); + wb.add_category("Quarter").unwrap(); + for item in ["North", "South", "East"] { + wb.add_item("Region", item).unwrap(); + } + for item in ["Q1", "Q2"] { + wb.add_item("Quarter", item).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + for (quarter, region) in [ + ("Q1", "North"), + ("Q1", "South"), + ("Q2", "South"), + ("Q2", "East"), + ] { + wb.model.set_cell( + CellKey::new(vec![ + ("Quarter".into(), quarter.into()), + ("Region".into(), region.into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Number(1.0), + ); + } + { + let v = wb.active_view_mut(); + v.set_axis("Region", Axis::Row); + v.set_axis("_Measure", Axis::Column); + v.set_axis("Quarter", Axis::Page); + v.set_page_selection("Quarter", "Q1"); + v.prune_empty = prune; + } + let mut app = App::new(wb, None); + app.rebuild_layout(); + app + } + + fn row_labels(app: &App) -> Vec { + (0..app.layout.row_count()) + .map(|r| app.layout.row_label(r)) + .collect() + } + + fn flip_to_q2(app: &mut App) { + app.apply_effects(vec![Box::new(super::super::effect::SetPageSelection { + category: "Quarter".to_string(), + item: "Q2".to_string(), + })]); + } + + /// BUG (improvise-7kvw): pruning depends on the page, so a page flip renumbers + /// the grid and left both the host's cursor and every client's stale. + #[test] + fn a_page_flip_under_prune_empty_invalidates_the_cursor() { + let mut app = paged_model(true); + let before_labels = row_labels(&app); + let epoch = app.cursor_epoch(); + app.cursor_mut().selected = (1, 0); + + flip_to_q2(&mut app); + + assert_ne!( + before_labels, + row_labels(&app), + "the two pages prune the same rows, so this case proves nothing" + ); + assert_ne!( + app.cursor_epoch(), + epoch, + "the grid renumbered and no cursor was invalidated" + ); + assert_eq!( + app.cursor().selected, + (0, 0), + "the host's own cursor is stale" + ); + } + + /// The other arm: with pruning off the page names the same rows, so paging + /// must not throw the user's position away on an ordinary `[`/`]`. + #[test] + fn a_page_flip_without_prune_empty_leaves_the_cursor_alone() { + let mut app = paged_model(false); + let before_labels = row_labels(&app); + let epoch = app.cursor_epoch(); + app.cursor_mut().selected = (1, 0); + + flip_to_q2(&mut app); + + assert_eq!( + before_labels, + row_labels(&app), + "the page flip renumbered the grid, so this is not the no-reshape arm" + ); + assert_eq!( + app.cursor_epoch(), + epoch, + "paging with nothing pruned threw a cursor away" + ); + assert_eq!(app.cursor().selected, (1, 0)); + } + + /// improvise-h074: the fallback indexes `views` by the document's default, and + /// a bare `IndexMap` panic names no invariant where `active_view()` did. + #[test] + #[should_panic(expected = "active_view always names an existing view")] + fn a_default_that_names_nothing_says_which_invariant_broke() { + let mut app = two_col_model(); + // Only a hand-built or foreign-deserialized workbook can be here: + // `create_view`/`delete_view` both maintain the invariant. + app.model_state.workbook.active_view = "NoSuchView".to_string(); + let _ = app.resolve_view("AlsoNoSuchView"); + } + /// improvise-8uz: moving the cursor must leave the shared document untouched, /// so one client scrolling cannot move another's viewport. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 3ba27903..38d75abe 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -440,8 +440,14 @@ pub struct SetPageSelection { } impl Effect for SetPageSelection { fn apply(&self, app: &mut App) { + let moved = app.view().page_selection(&self.category) != Some(self.item.as_str()); app.view_mut() .set_page_selection(&self.category, &self.item); + // Which rows survive pruning is a function of the page, so under + // `prune_empty` a page flip renumbers the grid (improvise-7kvw). + if moved && app.view().prune_empty { + app.reset_cursor(); + } } fn kind(&self) -> EffectKind { EffectKind::Model(ModelChange::LayoutGeometry) @@ -744,24 +750,29 @@ impl Effect for SetDrillPendingEdit { pub struct Save; impl Effect for Save { fn apply(&self, app: &mut App) { - match save_to_current_path(&*app.storage, &app.model_state) { - Ok(msg) => { - app.model_state.dirty = false; - app.view_state.status_msg = msg; - } - // A failed save aborts the rest of the batch, so the `Quit` that - // `wq`/`ZZ` chains never lands (improvise-8tk, gotcha 21). - Err(msg) => { - app.view_state.status_msg = msg; - app.abort_effects = true; - } + let outcome = save_to_current_path(&*app.storage, &app.model_state); + if outcome.is_ok() { + app.model_state.dirty = false; } + report_file_write(app, outcome); } fn kind(&self) -> EffectKind { EffectKind::ViewOnly } } +/// Every effect that writes a file reports through here: the status message either +/// way, and a failure aborts the rest of the batch (improvise-z05r, gotcha 21). +fn report_file_write(app: &mut App, outcome: Result) { + match outcome { + Ok(msg) => app.view_state.status_msg = msg, + Err(msg) => { + app.view_state.status_msg = msg; + app.abort_effects = true; + } + } +} + /// Write the workbook to its current file path, returning the status /// message either way. Missing path and I/O failure are both errors. fn save_to_current_path( @@ -782,19 +793,15 @@ fn save_to_current_path( pub struct SaveAs(pub PathBuf); impl Effect for SaveAs { fn apply(&self, app: &mut App) { - match crate::persistence::save_to(&*app.storage, &app.model_state.workbook, &self.0) { - Ok(()) => { - app.model_state.file_path = Some(self.0.clone()); - app.model_state.dirty = false; - app.view_state.status_msg = format!("Saved to {}", self.0.display()); - } - // The same abort `Save` owes: `SaveAs` is the arm improvise-8tk - // left out, so `:w /bad/path . q` quit on a failed write. - Err(e) => { - app.view_state.status_msg = format!("Save error: {e}"); - app.abort_effects = true; - } + let outcome = + crate::persistence::save_to(&*app.storage, &app.model_state.workbook, &self.0) + .map(|()| format!("Saved to {}", self.0.display())) + .map_err(|e| format!("Save error: {e}")); + if outcome.is_ok() { + app.model_state.file_path = Some(self.0.clone()); + app.model_state.dirty = false; } + report_file_write(app, outcome); } fn kind(&self) -> EffectKind { EffectKind::ViewOnly @@ -930,19 +937,15 @@ pub struct ExportCsv(pub PathBuf); impl Effect for ExportCsv { fn apply(&self, app: &mut App) { let view_name = app.view_name().to_string(); - match crate::persistence::export_csv_to( + let outcome = crate::persistence::export_csv_to( &*app.storage, &app.model_state.workbook, &view_name, &self.0, - ) { - Ok(()) => { - app.view_state.status_msg = format!("Exported to {}", self.0.display()); - } - Err(e) => { - app.view_state.status_msg = format!("Export error: {e}"); - } - } + ) + .map(|()| format!("Exported to {}", self.0.display())) + .map_err(|e| format!("Export error: {e}")); + report_file_write(app, outcome); } fn kind(&self) -> EffectKind { EffectKind::ViewOnly @@ -2004,6 +2007,33 @@ mod tests { ); } + /// BUG (improvise-z05r): the third arm of a rule with three. Named, so a + /// fourth file-writing effect has to come here — see the evidence. + #[test] + fn every_file_writing_effect_aborts_the_batch_on_failure() { + let bad = PathBuf::from("/nonexistent-dir-improvise-z05r/out"); + type Writer = fn(&PathBuf) -> Box; + let writers: [(&str, Writer); 3] = [ + ("Save", |_| Box::new(Save)), + ("SaveAs", |p| Box::new(SaveAs(p.clone()))), + ("ExportCsv", |p| Box::new(ExportCsv(p.clone()))), + ]; + for (label, make) in writers { + let mut app = test_app(); + app.model_state.file_path = Some(bad.clone()); + app.apply_effects(vec![make(&bad), Box::new(ChangeMode(AppMode::Quit))]); + assert!( + app.last_batch_aborted(), + "{label} failed to write and did not stop the batch" + ); + assert_ne!( + app.view_state.mode, + AppMode::Quit, + "{label} failed to write and the trailing effect applied anyway" + ); + } + } + /// The positive arm: a `SaveAs` that succeeds must not abort. Without it, /// "always abort" passes the test above. #[test] @@ -2327,19 +2357,92 @@ mod tests { app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); assert!(app.drill_state.is_none()); assert!( - app.model_state.dirty, - "a drill exited with no edits still marks the document dirty, \ - because `apply_effects` raises the flag off the declared \ - `EffectKind` and this effect declares `Model(CellData)` whether \ - or not it wrote anything (improvise-gv9r). That is the same \ - over-approximation a failed `LoadModel` gets, in the same \ - direction: one redundant write rather than a lost edit. It has \ - been true since the flag moved; this test could not see it \ - because it drove `apply` directly and so measured the effect in \ - isolation rather than the path production takes." + !app.model_state.dirty, + "a drill exited with no edits wrote nothing, so the document is \ + unchanged and `:q` must not be refused (improvise-748f)" ); } + /// improvise-748f: `ApplyAndClearDrill` on a document with no drill is what + /// view-back emits unconditionally, and it writes nothing at all. + #[test] + fn a_batch_that_writes_nothing_leaves_the_document_clean() { + let mut app = test_app(); + assert!(app.drill_state.is_none()); + app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); + assert!( + !app.model_state.dirty, + "view-back on a document with no drill open refused the next `:q`" + ); + } + + /// `CellData` is wider than its name, and a raw-cell fingerprint sees none of + /// these — the cheap cure improvise-748f proposed would have lost them. + #[test] + fn a_cell_data_effect_no_raw_cell_records_still_dirties_the_document() { + type Writer = fn() -> Box; + let writers: [(&str, Writer); 5] = [ + ("AddItem", || { + Box::new(AddItem { + category: "Type".into(), + item: "Toys".into(), + }) + }), + ("AddItemInGroup", || { + Box::new(AddItemInGroup { + category: "Type".into(), + item: "Toys".into(), + group: "Leisure".into(), + }) + }), + ("SortData", || Box::new(SortData)), + ("AddFormula", || { + Box::new(AddFormula { + raw: "Profit = Revenue - Cost".into(), + target_category: "_Measure".into(), + }) + }), + ("RemoveFormula", || { + Box::new(RemoveFormula { + target: "Profit".into(), + target_category: "_Measure".into(), + }) + }), + ]; + for (label, make) in writers { + let mut app = test_app(); + app.apply_effects(vec![Box::new(AddFormula { + raw: "Profit = Revenue - Cost".into(), + target_category: "_Measure".into(), + })]); + // Inserted out of sorted order, or `SortData` reorders nothing and + // its case proves the opposite of what it names. + for (n, (ty, month)) in [("Food", "Jan"), ("Clothing", "Feb")].iter().enumerate() { + app.model_state.workbook.model.set_cell( + CellKey::new(vec![ + ("Type".into(), (*ty).into()), + ("Month".into(), (*month).into()), + ]), + CellValue::Number(n as f64), + ); + } + if label == "AddFormula" { + app.apply_effects(vec![Box::new(RemoveFormula { + target: "Profit".into(), + target_category: "_Measure".into(), + })]); + } + app.model_state.dirty = false; + + app.apply_effects(vec![make()]); + assert!( + app.model_state.dirty, + "{label} changed the document and left it reading clean, so the \ + edit is lost on quit and the worker never writes it back" + ); + } + } + /// A staged edit to a measure column writes that one cell (improvise-c5v). #[test] fn apply_and_clear_drill_with_measure_edit() { From 17e59335eb87c8f2909f51aaf381de35f48e4634 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:40:04 -0700 Subject: [PATCH 3/9] fix: one answer to "is this category virtual" (improvise-bidq) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `View::on_category_added`'s early return read cat_name.starts_with('_') || self.is_records_layout() — the same expression mixing both derivations one operator apart, the left a name prefix and the right `CategoryKind`. A reader who checks this line sees the enum being used and has no reason to look at the operand beside it. The second site, `holds_a_regular`, had the same split. `Model::add_category` does not validate names, so a user category literally named `_Foo` was `CategoryKind::Regular` to `records_columns`, `cell_id` and `is_empty_model`, and "virtual" to these two lines — seated on `Axis::None`, never appearing in a pivot, while every other consumer treated it as ordinary. Decided explicitly, as the ticket asks: `add_category` is NOT made to refuse a leading underscore. The three reserved names are the ones `CategoryKind::of_name` knows, `_Foo` is a legal document, and a user-visible restriction is a bigger change than making one answer out of two. Both sites now ask `CategoryKind::of_name(..).is_virtual()`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/core.md | 54 +++++++++++++++++++++++++ crates/improvise-core/src/view/types.rs | 25 +++++++++++- 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index d203bb31..a061384c 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -543,3 +543,57 @@ uses. Fault injected into that arm alone: it fails, `Number(0.0)` against The general shape: an injected fault proves coverage only of the code path it is injected into. Two arms of the same rule need two injections, and if one of them turns out to be unreachable, that is a fact about the design worth writing down. + + +### a_user_category_whose_name_leads_with_an_underscore_is_seated_as_regular + +improvise-bidq. `View::on_category_added`'s early return read +`cat_name.starts_with('_') || self.is_records_layout()` — the **same +expression** mixing two derivations one operator apart, the left a name prefix +and the right `CategoryKind`. Four lines up, `is_records_layout` asks the enum. + +`Model::add_category` does not validate names, so a user category literally +named `_Foo` was `CategoryKind::Regular` to `records_columns`, `cell_id` and +`is_empty_model`, and "virtual" to these two lines — seated on `Axis::None`, +so it never appeared in a pivot while every other consumer treated it as +ordinary. The second site (`holds_a_regular`) had the same split, which is why +the test asserts a *second* category's seat too: fixing only the first leaves +`_Foo` on Row while `Region` also takes Row. + +Decided explicitly, per the ticket: `add_category` is **not** made to refuse a +leading underscore. The three reserved names are the ones `CategoryKind::of_name` +knows, `_Foo` is a legal document, and a user-visible restriction is a larger +change than making one answer out of two. The fix is that both sites now ask +`CategoryKind::of_name(..).is_virtual()`. + +Fault injected by restoring either `starts_with('_')`: the test fails, `None` +against `Row` on the first site and `Row` against `Column` on the second. + +### an_ungrouped_item_after_a_grouped_run_belongs_to_no_group + +### a_grouped_run_does_not_leak_into_the_next_prefix + +improvise-gd83, the two arms of one defect. `expand_category` emitted a +`GroupHeader` on **entering** a group and no boundary on leaving one, so +`row_group_for`/`col_group_for`'s nearest-preceding walk answered with a group +that had already ended. + +The arms are separate because the two escapes are different and a fix can close +one and miss the other. The first is *within* one expansion — `Jan[Q1]`, +`Feb[Q1]`, `Sep` — closed by emitting `AxisEntry::GroupEnd` on a +`Some -> None` transition. The second is *across* expansions: `cross_product` +expands the inner category once per prefix, each call starting with +`last_group = None`, so `Food/Bob[Family]` is immediately followed by +`Clothing/Carol`, which is ungrouped and inherited Family with no transition +anywhere to hook onto. That one needs a `GroupEnd` at the **end** of an +expansion whose last item was grouped. + +`GroupEnd` is deliberately NOT emitted on `Some(A) -> Some(B)`: B's own header +already ends A's run, and emitting one there moved +`data_row_to_visual_skips_headers` and `data_col_to_visual_skips_headers` for +no gain. Those two are untouched, which is the evidence the boundary is as +narrow as it claims. + +Fault injected by deleting either emission: the corresponding test fails with +`Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and +`the_two_front_ends_draw_the_same_column_header` fails with it. diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index a3d824c6..eace86e6 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -70,13 +70,15 @@ impl View { /// first slot no regular holds. A virtual is **never** bumped (§7). pub fn on_category_added(&mut self, cat_name: &str) { if !self.category_axes.contains_key(cat_name) { - let axis = if cat_name.starts_with('_') || self.is_records_layout() { + let virtual_or_records = + CategoryKind::of_name(cat_name).is_virtual() || self.is_records_layout(); + let axis = if virtual_or_records { Axis::None } else { let holds_a_regular = |axis: Axis| { self.categories_on(axis) .into_iter() - .any(|c| !c.starts_with('_')) + .any(|c| !CategoryKind::of_name(c).is_virtual()) }; match (holds_a_regular(Axis::Row), holds_a_regular(Axis::Column)) { (false, _) => Axis::Row, @@ -292,6 +294,25 @@ mod tests { ); } + /// improvise-bidq: `_Foo` is `CategoryKind::Regular` to every other + /// consumer — see context/test-evidence/core.md. + #[test] + fn a_user_category_whose_name_leads_with_an_underscore_is_seated_as_regular() { + let v = view_with_cats(&["_Foo"]); + assert_eq!( + v.axis_of("_Foo"), + Axis::Row, + "of_name says Regular, so the first slot is Row — not None" + ); + + let v = view_with_cats(&["_Foo", "Region"]); + assert_eq!( + v.axis_of("Region"), + Axis::Column, + "_Foo holds Row as a regular category, so Region takes Column" + ); + } + #[test] fn first_category_assigned_to_row() { let v = view_with_cats(&["Region"]); From fc059c7ce16fc9d476c7ef9329affd2f5d6af179 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:40:28 -0700 Subject: [PATCH 4/9] fix: mode_style matches AppMode exhaustively (improvise-zx8t) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repo map promises of `AppMode` that "add a variant and the compiler names every one you must fill in". That was true of the five enum methods and false of `src/draw.rs::mode_style`, which swept eleven variants into a `_ =>` wildcard — so `SavePrompt` took `ExportPrompt`'s DarkGray by accident rather than by decision, and the next person adding a variant would have trusted the sentence. Of the ticket's two options, the first: `mode_style` is now exhaustive and stays in `draw.rs`, because `Style` is a ratatui type (gotcha 26). The second — a semantic colour method on `AppMode` in `improvise-protocol`, the `AxisColor`/`MinibufColor` shape — is NOT taken here, and deliberately: the ticket says to decide it together with improvise-k099, which asks the same question of `hint_text`/`label` and is not in this unit. The repo map's promise needed exhaustiveness; the semantic enum is a separate, larger answer and stays open. No colour changes. The fourteen default-chrome variants are spelled out with the DarkGray they already had, so this is the wildcard becoming a list. The guard is the compiler, not a test — a test enumerating today's variants would not fail when a fifteenth is added. Verified by deleting the `AppMode::Help` arm: error[E0004]: non-exhaustive patterns: `&AppMode::Help` not covered Also removes the doc comment calling the wildcard "a known defect", which this commit makes false, and updates `context/code/tui.md`, which said the same. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/tui.md | 33 ++++++++++++++++++++++++++++----- src/draw.rs | 31 +++++++++++++++++++++---------- 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/context/code/tui.md b/context/code/tui.md index 07a42210..750436cf 100644 --- a/context/code/tui.md +++ b/context/code/tui.md @@ -121,11 +121,34 @@ enum, and only the last translation is local.** `mode_style(&AppMode) -> Style` stays here for the same reason (`Style` and `Color` are ratatui types and `improvise-command`, where `AppMode` lives, -carries no rendering-framework dependency) — **but it is not the same shape.** -It matches on `AppMode` directly with a `_ =>` wildcard for "the default chrome -for every non-highlighted mode", so a new `AppMode` variant silently inherits -that chrome instead of failing to compile, and there is no semantic enum in the -shared layer to decide it. That is `improvise-zx8t`, filed and not fixed. +carries no rendering-framework dependency), and since `improvise-zx8t` it is +**exhaustive**: the fourteen variants that take the default chrome are spelled +out rather than swept up by a `_ =>` wildcard, so adding an `AppMode` fails the +build here until someone chooses its colour. + +It is still not quite the same shape as `axis_fg` and its two siblings, and the +difference is deliberate rather than pending. Those three map a *semantic enum* +the shared layer owns; `mode_style` matches `AppMode` itself, because there is +no `ModeColor`. `improvise-zx8t` offered making one, and it was declined here +for two reasons: the colour is the only thing at stake, and the ticket says to +decide it together with `improvise-k099`, which asks the same question of +`hint_text`/`label` and is not in this unit. Exhaustiveness is what the +repo map's "the compiler names every one you must fill in" promise needed; a +semantic enum is a separate, larger answer and is still open. + +**The guard is the compiler, not a test.** Nothing asserts the colours, and a +test enumerating the variants would not fail when a fifteenth is added — only +the missing match arm does. Verified by deleting the `AppMode::Help` arm: +`error[E0004]: non-exhaustive patterns: `&AppMode::Help` not covered`. + +### The minibuffer shares the bottom row with the status line + +`draw_bottom_bar` used to render the minibuffer *instead of* `draw_status`, so a +status message written while any text-entry mode was open was never drawn +(`improvise-mai9`, and the stranding gotcha 21 describes). `minibuffer_line` +composes both: the prompt and buffer on the left, any pending `status_msg` +right-aligned. The alternatives and why they lost are in +`context/test-evidence/tui.md`. ### The caches are built here, per frame diff --git a/src/draw.rs b/src/draw.rs index 664ccdaa..38500cff 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -193,17 +193,28 @@ fn draw_popup_frame(f: &mut Frame, popup: Rect, title: &str, border_color: Color inner } -/// Terminal styling for the mode indicator. Its wildcard arm is a known -/// defect (improvise-zx8t) — see `context/code/tui.md`. +/// Terminal styling for the mode indicator. Exhaustive on purpose, so a new +/// `AppMode` fails the build here (improvise-zx8t) — `context/code/tui.md`. fn mode_style(mode: &AppMode) -> Style { - match mode { - AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => { - Style::default().fg(Color::Black).bg(Color::Green) - } - AppMode::CommandMode { .. } => Style::default().fg(Color::Black).bg(Color::Yellow), - AppMode::TileSelect => Style::default().fg(Color::Black).bg(Color::Magenta), - _ => Style::default().fg(Color::Black).bg(Color::DarkGray), - } + let bg = match mode { + AppMode::Editing { .. } | AppMode::RecordsEditing { .. } => Color::Green, + AppMode::CommandMode { .. } => Color::Yellow, + AppMode::TileSelect => Color::Magenta, + AppMode::Normal + | AppMode::RecordsNormal + | AppMode::FormulaEdit { .. } + | AppMode::FormulaPanel + | AppMode::CategoryPanel + | AppMode::CategoryAdd { .. } + | AppMode::ItemAdd { .. } + | AppMode::ViewPanel + | AppMode::ImportWizard + | AppMode::ExportPrompt { .. } + | AppMode::SavePrompt { .. } + | AppMode::Help + | AppMode::Quit => Color::DarkGray, + }; + Style::default().fg(Color::Black).bg(bg) } fn draw(f: &mut Frame, app: &App) { From 94a3d1754b7747fd80c1674ea4f003334855c4f2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:40:39 -0700 Subject: [PATCH 5/9] fix: the minibuffer row carries a pending status message (improvise-mai9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `draw_bottom_bar` rendered the minibuffer INSTEAD of the status line, so a status message written while any text-entry mode was open was never drawn — not truncated, not overwritten, simply never rendered. A test asserting `app.status_msg == "..."` passed in every one of those cases, which is why this survived improvise-6t3's careful ordering of `CommitBuffer`'s effects so a failure message would win: it won a race to a surface nobody could see. Of the ticket's two candidates, the row is shared. `minibuffer_line` composes the prompt and buffer on the left and any pending `status_msg` right-aligned, so the bottom bar stays ONE row and the exact-frame geometry does not move. The alternative — making the caller structurally unable to report while its minibuffer is open — is what `CommitSave` does by hand today, and generalising it means repeating that at every text-entry commit. The ticket calls that a workaround; sharing the row costs one function and covers all eight modes at once, including the `:w /bad/path` path Copilot found on PR #27. The failure is now loud: `a_status_message_set_while_a_minibuffer_is_open_is_ on_the_bottom_row` asserts the composed row, not the field. Its negative arm is not decoration — a composition that appended unconditionally would pass the first test and draw two spaces of nothing on every minibuffer keystroke. Injected fault: the `false` arm returning `String::new()`, i.e. the old composition. The first test fails naming the missing message; the second passes. Known and out of scope: `fill_line` pads on `left.len()`, which is bytes, so the right-hand text sits two cells right of its mark whenever the left carries the multi-byte cursor glyph. `draw_status` has always done this. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- src/draw.rs | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/draw.rs b/src/draw.rs index 38500cff..c00aacbe 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -388,6 +388,17 @@ fn draw_tile_bar(f: &mut Frame, area: Rect, app: &App) { ); } +/// The minibuffer row, with any pending status message riding on its right. +/// The minibuffer owns the bottom row and used to hide it — tui.md §3. +fn minibuffer_line(prompt: &str, buf: &str, status_msg: &str, width: u16) -> String { + let left = format!("{prompt}{buf}▌"); + let right = match status_msg.is_empty() { + true => String::new(), + false => format!(" {status_msg} "), + }; + fill_line(left, &right, width) +} + fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) { if let Some(mb) = app.view_state.mode.minibuffer() { let buf = app @@ -396,7 +407,7 @@ fn draw_bottom_bar(f: &mut Frame, area: Rect, app: &App) { .get(&mb.buffer_key) .map(|s| s.as_str()) .unwrap_or(""); - let text = format!("{}{}▌", mb.prompt, buf); + let text = minibuffer_line(&mb.prompt, buf, &app.view_state.status_msg, area.width); f.render_widget( Paragraph::new(text).style( Style::default() @@ -527,6 +538,36 @@ fn draw_welcome(f: &mut Frame, area: Rect) { } } +#[cfg(test)] +mod bottom_bar_tests { + use super::minibuffer_line; + + /// improvise-mai9: the message used not to be drawn at all — see + /// context/test-evidence/tui.md. + #[test] + fn a_status_message_set_while_a_minibuffer_is_open_is_on_the_bottom_row() { + let line = minibuffer_line("Category: ", "North", "category already exists", 80); + assert!( + line.contains("Category: North"), + "the minibuffer still owns the left of the row: {line:?}" + ); + assert!( + line.contains("category already exists"), + "the status message must reach the user: {line:?}" + ); + } + + #[test] + fn with_no_message_pending_the_minibuffer_row_is_only_the_minibuffer() { + let line = minibuffer_line("Category: ", "North", "", 80); + assert_eq!( + line.trim_end(), + "Category: North\u{258c}", + "an empty status adds nothing but padding" + ); + } +} + #[cfg(test)] mod key_conversion_tests { use super::*; From 8f3a9768b7087cc2a294ca75e49a967c2c7b43fd Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:40:57 -0700 Subject: [PATCH 6/9] fix: group membership, leading headers and one run rule on AxisFrame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-gd83, improvise-0o6x and improvise-8qlr. One commit because they are one change to `AxisFrame`/`AxisEntry` — a variant, a field and a method — and all three move `tests/grid_frame_agreement.rs`, whose two ends must be changed together or the test stops comparing anything. improvise-gd83 — AxisEntry carries a group-end boundary ------------------------------------------------------- `expand_category` emitted a header on ENTERING a group and no boundary on leaving one, so `group_of` answered with the nearest preceding header rather than membership, and the TUI painted that span. Copilot was right on PR #23; this is the filing gotcha 44 said the fix needed. `AxisEntry::GroupEnd` is the boundary, carried onto the wire as `FrameEntry::GroupEnd`. Two arms, and a fix can close one and miss the other: * within one expansion — Jan[Q1], Feb[Q1], Sep — a `Some -> None` transition; * across expansions — `cross_product` expands the inner category once per prefix and each call starts with `last_group = None`, so Food/Bob[Family] is followed directly by Clothing/Carol with no transition to hook onto. That needs a `GroupEnd` at the END of an expansion whose last item was grouped. `GroupEnd` is NOT emitted between two groups: the next header already ends the previous run, and emitting one there moved `data_row_to_visual_skips_headers` and its column twin for no gain. Those two are untouched, which is the evidence the boundary is as narrow as it claims. A trailing marker is trimmed, since the axis itself ends the last run. Nothing draws a `GroupEnd` and it consumes no data index. `THE_MEMBERSHIP_CORRECT_SPANS` and its `assert_ne!` tripwire are gone, and the value they predicted is now asserted element for element — it was right to the element, which is worth recording. improvise-0o6x — a leading group header is drawn ------------------------------------------------ `body_from(idx)` started at the first ITEM, so at the top of an axis a leading `FrameEntry::Group` was dropped on the way to the screen, in both front ends. A collapsed group first on its axis therefore had its rows hidden and its header nowhere: no affordance to get them back. It is a boundary condition as the ticket suspected, but the boundary is not `idx == 0`. The windowing rule carries the nearest header ABOVE a scrolled window for `group_of` and `body_from` discards it — correctly — and a frame cannot tell that from the top of the axis using `idx` alone, because the browser's frame is already windowed and always passes `0`. So `AxisFrame` gained `first`, the absolute data index of its first item, and the rule is `first + idx == 0`. Without it the browser would draw an extra header row at every scrolled position that the TUI does not. RE-PINNED DELIBERATELY, the improvise-q0vb acceptance shape: `GROUPED_FRAME` in `src/ui/grid.rs` moves by exactly one line — the leading `▼ Co…` now draws above `East` and everything below shifts down one. The group line, the column headers, the separator, the data rows and the totals strip are byte-identical, which is the evidence this is the boundary condition and not a reshuffle. `harness/tests/grouped.spec.js` asserted TWO group rows against a fixture with three, naming this ticket on the line and saying the count becoming 3 would be the record of when it was fixed. It is 3, verified in Chromium. Three assertions in that file took "the first tbody row" to be a data row; a `firstDataRow` helper now skips the header, which is the same re-pin. improvise-8qlr — the run rule lives once ----------------------------------------- Column-group run coalescing was derived in `src/ui/grid.rs` and again in `improvise-wasm-client/src/render.rs::col_groups`, held in step by a comment saying so. `AxisFrame::group_runs`/`group_run_heads` is the one home and both ends call it. Runs still coalesce by NAME, which is what both ends did before and is preserved rather than quietly changed. The TUI keeps one divergence, stated: it labels its first VISIBLE column even mid-run, or a scrolled grid draws an unlabelled group line. The agreement test's TUI arm was `group_line.contains("▼ Q1")` — the unpinned arm the ticket names. It now compares the painted group line cell for cell against spans derived from the browser's own runs. The test is fixed, not deleted. Injected faults, each failing by name ------------------------------------- * gd83: delete either `GroupEnd` emission — the two new layout tests fail with the leaked group, and the column-header agreement test fails with them. * 0o6x: force `body_from`'s match to `false` — `the_whole_frame_is_unchanged` and the new leading-header agreement test fail. * 8qlr: the ticket's own recipe, forcing the TUI to emit the group label on every column. `cargo test -p improvise` was GREEN before this commit and now fails with ["▼ Q1","▼ Q1","",...] against ["▼ Q1","","",...]. What the ticket did not predict ------------------------------- A collapsed leading group has no item to carry its header into a window, so it reaches the frame only through `axis_frame`'s `pending` queue. The first version of the windowing change had `GroupEnd` CLEAR `pending` above the window, which deleted exactly that header — the affordance 0o6x exists to restore. `GroupEnd` is now queued like a header and clears nothing. Found by driving the new agreement test in both collapse states; the uncollapsed arm alone passes against the broken version. 1411 Rust tests pass (baseline 1405, +6). Playwright 28 passed + 3 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/protocol.md | 37 ++++++ context/test-evidence/tui.md | 102 ++++++++++++++++ crates/improvise-command/src/ui/project.rs | 7 ++ crates/improvise-command/src/ui/viewport.rs | 2 +- crates/improvise-core/src/view/layout.rs | 127 ++++++++++++++++---- crates/improvise-protocol/src/render.rs | 73 +++++++++-- crates/improvise-wasm-client/src/render.rs | 37 +++--- harness/tests/grouped.spec.js | 32 +++-- src/ui/grid.rs | 24 ++-- tests/grid_frame_agreement.rs | 105 ++++++++++++---- 10 files changed, 448 insertions(+), 98 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index ff3da03b..9084b7e1 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -751,6 +751,43 @@ Nothing is defined for the help overlay: its content is static and --- +## §3a. The frame's group boundaries and its position in the axis + +Two facts `AxisFrame` gained after `improvise-anr9`, both about group runs. + +**`FrameEntry::GroupEnd` closes a run.** `group_of` was documented as answering +"nearest preceding header", which is not membership: `expand_category` emitted a +header on *entering* a group and nothing on leaving one, so an ungrouped item +after a grouped run inherited it (`improvise-gd83`). `AxisEntry::GroupEnd` is +the boundary, carried onto the wire as `FrameEntry::GroupEnd`; `groups()` clears +`current` on it and `group_of` stops walking. It is emitted only on +`Some -> None`, never between two groups — the next header already ends the +previous run — and at the end of an expansion whose last item was grouped, which +is the across-prefix arm `cross_product` creates. + +Nothing draws a `GroupEnd`. Both front ends skip it and it consumes no data +index, so it is invisible except through `group_of`/`groups`. + +**`AxisFrame::first` is the absolute data index of the frame's first item.** The +windowing rule carries the nearest header *above* a scrolled window so +`group_of` can answer, and `body_from` drops it on the way to the screen. At the +top of an axis there is no "above", so that same leading header is content — a +collapsed first group has no other affordance (`improvise-0o6x`). `idx` alone +cannot tell the two apart, because the browser's frame is already windowed and +always passes `0`; `first + idx == 0` can. + +The TUI builds over the whole grid so `first` is always `0` there, and the +browser's comes from the projected window. A frame built by hand in a test must +set it, which is why it is a plain field rather than derived. + +**`group_runs` / `group_run_heads` are the coalescing rule's one home.** The TUI +and `render.rs::col_groups` each coalesced adjacent columns sharing a group, +kept in step by a comment (`improvise-8qlr`). Runs coalesce by group **name**, +so two adjacent runs under two different headers of the same name read as one — +that is what both ends did before and is preserved deliberately. The TUI's one +divergence is that it labels its first *visible* column even mid-run, so a +scrolled grid does not draw an unlabelled group line. + ## 4. `frame.rs` — one envelope pair, two deployments ### The framing adds as little as it can get away with diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index dcc5a092..a4ca56c6 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -414,3 +414,105 @@ a word pool instead of random letters, `pipe_inner` is never empty, `rest_of_line` always produces at least one character, and repetitions (`*`) produce 1–4 items rather than 0. Each invocation is seeded from the current time plus the PID. + + +### a_status_message_set_while_a_minibuffer_is_open_is_on_the_bottom_row + +### with_no_message_pending_the_minibuffer_row_is_only_the_minibuffer + +improvise-mai9. `draw_bottom_bar` rendered the minibuffer **instead of** the +status line, so a message written while a text-entry mode was open was never +drawn — not truncated, not overwritten. A test asserting `app.status_msg` was +set passed in every one of those cases, which is exactly how this survived +improvise-6t3's careful effect ordering and why the guard has to be aimed at +the *composed row* rather than at the field. + +Of the ticket's two candidate fixes, the row is **shared** rather than the +callers being made structurally unable to report: giving the status a second +row changes frame geometry (improvise-q0vb's cost), and "always leave the mode +before reporting" is what `CommitSave` does by hand and would have to be +repeated at every text-entry commit — the thing the ticket calls a workaround. +Sharing the row costs one function and generalises to all eight modes. + +The second test is the negative arm and is not decoration: a composition that +appended the status unconditionally would pass the first and draw two trailing +spaces of nothing on every keystroke of every minibuffer. + +Known and not fixed here: `fill_line` pads on `left.len()`, which is bytes, so +the right-hand message sits two cells further right than intended whenever the +left contains the multi-byte cursor glyph. `draw_status` has always done this +with its own multi-byte content; it is pre-existing and out of scope. + +Fault injected by restoring `format!("{}{}the cursor glyph", prompt, buf)` (here: the +`false` arm returning `String::new()`): the first test fails naming the missing +message and printing the row it did compose. + +### a_group_header_first_on_its_axis_reaches_both_front_ends + +improvise-0o6x. `AxisFrame::body_from(idx)` started at the first **item**, so at +the top of the axis a leading `FrameEntry::Group` was skipped on the way to the +screen — in both front ends. A collapsed group first on its axis therefore had +its rows hidden and its header nowhere, so there was no affordance to get them +back. The frame carried the header correctly; `body_from` dropped it. + +It is a boundary condition, as the ticket suspected, but the boundary is not +`idx == 0`. The windowing rule carries the nearest header ABOVE a scrolled +window for `group_of` and `body_from` discards it — correctly. At the top of +the axis there is no "above", and a frame cannot tell the two apart from `idx` +alone, because the browser's frame is windowed and always passes `0`. So +`AxisFrame` gained `first`, the absolute data index of its first item, and the +rule is `first + idx == 0`. Without it the browser would have drawn an extra +header row at every scrolled position that the TUI does not. + +This test is separate from `the_two_front_ends_draw_the_same_row_header` +because that fixture orders `Carol` before `Family` to dodge this case **on +purpose**, and says so. Both collapse states are driven: the uncollapsed arm +alone passes against a frame that leads with a header only when it has items +under it, which is the shape the windowing bug below produced. + +The windowing half was found by this test and not predicted by the ticket. A +collapsed leading group has NO item to carry its header into the window, so it +reaches the frame only through `axis_frame`'s `pending` queue — and a first +version of the fix had `GroupEnd` **clear** `pending` above the window, which +deleted exactly that header. `GroupEnd` is now queued like a header and clears +nothing. + +Fault injected by forcing `body_from`'s match to `false`: this test fails and +`the_whole_frame_is_unchanged` fails with it. + +### the_whole_frame_is_unchanged + +Re-pinned for improvise-0o6x. `GROUPED_FRAME` moved by exactly one line: the +leading `▼ Co…` header now draws above `East`, and everything below shifts +down one. Nothing else in the buffer changed. That the group line, the column +headers, the separator, the data rows and the totals strip are byte-identical +is the evidence the change is the boundary condition it claims to be and not a +reshuffle of the draw loop. + +### the_two_front_ends_draw_the_same_column_header + +Two changes, from two tickets. + +improvise-gd83: `THE_MEMBERSHIP_CORRECT_SPANS` and its `assert_ne!` tripwire are +gone, and the value they predicted — `[(Q1,2), (None,1), (Q1,2), (None,1)]` — +is now what the test asserts, element for element. `Sep` is ungrouped, so the +run ends before it and resumes under the second measure's own `Q1` header. A +precondition asserts the fixture still contains an ungrouped column, because +without one the test cannot see a leaked group at all. + +improvise-8qlr: the TUI arm was `group_line.contains("▼ Q1")`, which is +one-directional and passes while the TUI labels **every** column. The run rule +now lives once, as `AxisFrame::group_runs`/`group_run_heads`, and both ends +call it; the TUI's group line is compared cell for cell against spans derived +from the browser's own runs. + +Fault injected exactly as the ticket specifies — forcing the TUI to emit the +group label on every column (`head = col_groups.get(ci)`, dropping the run-head +lookup). Before: `cargo test -p improvise` stayed green. After: this test fails +with `["▼ Q1","▼ Q1","","▼ Q1","▼ Q1",""]` against +`["▼ Q1","","","▼ Q1","",""]`. + +One thing the shared rule does **not** flatten: the TUI labels its first +*visible* column even mid-run, because a scrolled grid would otherwise draw an +unlabelled group line. That is a scrolling concession, kept explicitly, and it +is why the comparison is run at `col_offset == 0`. diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 021a2061..b68b7e3d 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -197,6 +197,9 @@ fn axis_frame( collapsed: view.is_group_collapsed(cat_name, group_name), })); } + // Queued like a header, never clearing one: a COLLAPSED group has + // no item to carry it and reaches the frame only through `pending`. + AxisEntry::GroupEnd => pending.push(FrameEntry::GroupEnd), AxisEntry::DataItem(tuple) => { let idx = data_idx; data_idx += 1; @@ -212,6 +215,7 @@ fn axis_frame( } AxisFrame { cats: cats.to_vec(), + first: window.start, entries, } } @@ -1234,6 +1238,7 @@ mod tests { .iter() .map(|e| match e { FrameEntry::Group(g) => format!("[{}]", g.name), + FrameEntry::GroupEnd => "[/]".to_string(), FrameEntry::Item(v) => v.join("+"), }) .collect() @@ -1755,6 +1760,7 @@ mod tests { let cell = |n: &str| CellDisplay::number(n); let mirror = GridCache { row_frame: AxisFrame { + first: 0, cats: vec!["Region".into()], entries: vec![FrameEntry::Item(vec!["North".into()])], }, @@ -1766,6 +1772,7 @@ mod tests { }; let desired = GridCache { row_frame: AxisFrame { + first: 0, cats: vec!["Department".into()], entries: mirror.row_frame.entries.clone(), }, diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index c95f3f38..e3394a21 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -82,7 +82,7 @@ pub fn compute_col_widths( pub fn compute_row_header_width(layout: &GridLayout) -> u16 { let items = layout.row_items.iter().filter_map(|e| match e { AxisEntry::DataItem(v) => Some(v), - AxisEntry::GroupHeader { .. } => None, + AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }); header_level_widths(items, layout.row_cats.len().max(1)) .iter() diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index cffd5543..fc444e75 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -93,6 +93,9 @@ pub enum AxisEntry { cat_name: String, group_name: String, }, + /// The run opened by the last `GroupHeader` ends here (improvise-gd83). + /// Nothing is drawn for it; it exists so membership is not inherited. + GroupEnd, DataItem(Vec), } @@ -101,7 +104,7 @@ pub enum AxisEntry { fn data_items(items: &[AxisEntry]) -> impl Iterator> { items.iter().filter_map(|e| match e { AxisEntry::DataItem(v) => Some(v), - AxisEntry::GroupHeader { .. } => None, + AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }) } @@ -134,6 +137,12 @@ fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { AxisEntry::GroupHeader { .. } => { pending_header = Some(entry); } + // The run it closes may have been pruned away entirely, in which + // case the header is still pending and both are dropped together. + AxisEntry::GroupEnd => match pending_header.take() { + Some(_) => {} + None => kept.push(entry), + }, AxisEntry::DataItem(_) => { if keep(data_idx) { if let Some(h) = pending_header.take() { @@ -529,34 +538,36 @@ impl GridLayout { /// the Nth data row. pub fn row_group_for(&self, data_row: usize) -> Option<(String, String)> { let vi = self.data_row_to_visual(data_row)?; - self.row_items[..vi].iter().rev().find_map(|e| { - if let AxisEntry::GroupHeader { - cat_name, - group_name, - } = e - { - Some((cat_name.clone(), group_name.clone())) - } else { - None - } - }) + self.row_items[..vi] + .iter() + .rev() + .find_map(|e| match e { + AxisEntry::GroupHeader { + cat_name, + group_name, + } => Some(Some((cat_name.clone(), group_name.clone()))), + AxisEntry::GroupEnd => Some(None), + AxisEntry::DataItem(_) => None, + }) + .flatten() } /// The `(cat_name, group_name)` of the nearest `GroupHeader` preceding /// the Nth data column. pub fn col_group_for(&self, data_col: usize) -> Option<(String, String)> { let vi = self.data_col_to_visual(data_col)?; - self.col_items[..vi].iter().rev().find_map(|e| { - if let AxisEntry::GroupHeader { - cat_name, - group_name, - } = e - { - Some((cat_name.clone(), group_name.clone())) - } else { - None - } - }) + self.col_items[..vi] + .iter() + .rev() + .find_map(|e| match e { + AxisEntry::GroupHeader { + cat_name, + group_name, + } => Some(Some((cat_name.clone(), group_name.clone()))), + AxisEntry::GroupEnd => Some(None), + AxisEntry::DataItem(_) => None, + }) + .flatten() } } @@ -585,8 +596,12 @@ fn expand_category( .get(item_name.as_str()) .and_then(|i| i.group.as_deref()); - // Emit a group header at each group boundary. + // A header on entering a group; a `GroupEnd` only on leaving one for + // NO group — another group's header already ends the run. if item_group != last_group { + if last_group.is_some() && item_group.is_none() { + result.push(AxisEntry::GroupEnd); + } if let Some(g) = item_group { result.push(AxisEntry::GroupHeader { cat_name: cat_name.to_string(), @@ -605,6 +620,11 @@ fn expand_category( row.push(item_name.to_string()); result.push(AxisEntry::DataItem(row)); } + // The expansion is the end of the run too: the next prefix's items follow + // immediately and must not inherit it (improvise-gd83). + if last_group.is_some() { + result.push(AxisEntry::GroupEnd); + } result } @@ -620,10 +640,14 @@ fn cross_product(model: &Model, view: &View, cats: &[String]) -> Vec .into_iter() .flat_map(|entry| match entry { AxisEntry::DataItem(prefix) => expand_category(model, view, cat_name, prefix), - header @ AxisEntry::GroupHeader { .. } => vec![header], + marker @ (AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd) => vec![marker], }) .collect(); } + // The axis itself ends the last run; a trailing marker names nothing. + while matches!(result.last(), Some(AxisEntry::GroupEnd)) { + result.pop(); + } result } @@ -1114,6 +1138,59 @@ mod tests { assert!(jan.is_none()); } + /// improvise-gd83: two arms — an ungrouped item AFTER a grouped run, and + /// the same item under a later prefix. context/test-evidence/core.md. + #[test] + fn an_ungrouped_item_after_a_grouped_run_belongs_to_no_group() { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + wb.add_item_in_group("Month", "Jan", "Q1").unwrap(); + wb.add_item_in_group("Month", "Feb", "Q1").unwrap(); + wb.add_item("Month", "Sep").unwrap(); + wb.add_item("Type", "Food").unwrap(); + let layout = GridLayout::new(&wb.model, wb.active_view()); + + assert_eq!( + layout.row_group_for(0), + Some(("Month".into(), "Q1".into())), + "precondition: Jan really is in Q1" + ); + assert_eq!( + layout.row_group_for(2), + None, + "Sep is in no group — the run ended before it" + ); + } + + /// The second arm: the run ends at the end of ONE expansion, so the next + /// prefix's first item does not inherit it. context/test-evidence/core.md. + #[test] + fn a_grouped_run_does_not_leak_into_the_next_prefix() { + let mut wb = Workbook::new("T"); + wb.add_category("Type").unwrap(); + wb.add_category("Recipient").unwrap(); + wb.add_item("Type", "Food").unwrap(); + wb.add_item("Type", "Clothing").unwrap(); + wb.add_item("Recipient", "Carol").unwrap(); + wb.add_item_in_group("Recipient", "Alice", "Family") + .unwrap(); + wb.add_item_in_group("Recipient", "Bob", "Family").unwrap(); + wb.active_view_mut().set_axis("Recipient", Axis::Row); + let layout = GridLayout::new(&wb.model, wb.active_view()); + + assert_eq!( + layout.row_group_for(2), + Some(("Recipient".into(), "Family".into())), + "precondition: Food/Bob is in Family" + ); + assert_eq!( + layout.row_group_for(3), + None, + "Clothing/Carol opens a new prefix and is in no group" + ); + } + #[test] fn ungrouped_items_produce_no_headers() { let wb = two_cat_workbook(); diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index ebbf8f38..e766339d 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -163,6 +163,9 @@ pub struct FrameGroup { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum FrameEntry { Group(FrameGroup), + /// The run opened by the last `Group` ends here — `AxisEntry::GroupEnd` + /// carried onto the wire (improvise-gd83). Nothing is drawn for it. + GroupEnd, /// One label per header level, outermost first. Data indices — the /// `row`/`col` of every other field on [`GridCache`] — count only these. Item(Vec), @@ -175,6 +178,10 @@ pub struct AxisFrame { /// The categories on this axis, outermost first. Empty is a real layout /// (one nameless row or column), not a gap — hence [`AxisFrame::levels`]. pub cats: Vec, + /// The **absolute** data index of this frame's first item. Zero means the + /// frame leads the axis, which is what [`AxisFrame::body_from`] asks. + #[serde(default)] + pub first: usize, pub entries: Vec, } @@ -184,6 +191,7 @@ impl AxisFrame { pub fn flat>(labels: impl IntoIterator) -> Self { Self { cats: Vec::new(), + first: 0, entries: labels .into_iter() .map(|l| FrameEntry::Item(vec![l.into()])) @@ -196,7 +204,7 @@ impl AxisFrame { pub fn items(&self) -> impl Iterator> { self.entries.iter().filter_map(|e| match e { FrameEntry::Item(v) => Some(v), - FrameEntry::Group(_) => None, + FrameEntry::Group(_) | FrameEntry::GroupEnd => None, }) } @@ -293,20 +301,51 @@ impl AxisFrame { for entry in &self.entries { match entry { FrameEntry::Group(g) => current = Some(g), + FrameEntry::GroupEnd => current = None, FrameEntry::Item(_) => out.push(current), } } out } - /// The **nearest preceding** group header — not the item's membership, - /// and deliberately so. **Not for a draw loop**; both points in §3. + /// The item's group **membership** — the nearest preceding header, unless + /// a `GroupEnd` closed it first (improvise-gd83). Not for a draw loop; §3. pub fn group_of(&self, idx: usize) -> Option<&FrameGroup> { let visual = self.visual_of(idx)?; - self.entries[..visual].iter().rev().find_map(|e| match e { - FrameEntry::Group(g) => Some(g), - FrameEntry::Item(_) => None, - }) + self.entries[..visual] + .iter() + .rev() + .find_map(|e| match e { + FrameEntry::Group(g) => Some(Some(g)), + FrameEntry::GroupEnd => Some(None), + FrameEntry::Item(_) => None, + }) + .flatten() + } + + /// The runs of adjacent positions sharing a group, coalesced by NAME — + /// the coalescing rule's **one home**, both front ends (improvise-8qlr). + pub fn group_runs(&self) -> Vec<(Option<&FrameGroup>, usize)> { + let mut runs: Vec<(Option<&FrameGroup>, usize)> = Vec::new(); + for group in self.groups() { + let name = group.map(|g| g.name.as_str()); + match runs.last_mut() { + Some((prev, span)) if prev.map(|p| p.name.as_str()) == name => *span += 1, + _ => runs.push((group, 1)), + } + } + runs + } + + /// The group label to paint above each position, `None` where the run + /// continues. Derived from [`AxisFrame::group_runs`], never beside it. + pub fn group_run_heads(&self) -> Vec> { + self.group_runs() + .into_iter() + .flat_map(|(group, span)| { + std::iter::once(group).chain(std::iter::repeat_n(None, span - 1)) + }) + .collect() } /// Position of the data item `idx` within `entries`, group headers @@ -320,11 +359,16 @@ impl AxisFrame { .nth(idx) } - /// The entries a renderer walks from data item `idx`, dropping the group - /// header above it. The argument is scrolling, not the rule — see §3. + /// The entries a renderer walks from data item `idx`. The header above it + /// is dropped, EXCEPT at the top of the axis — §3a, improvise-0o6x. pub fn body_from(&self, idx: usize) -> &[FrameEntry] { - let start = self.visual_of(idx).unwrap_or(self.entries.len()); - &self.entries[start..] + match self.first + idx == 0 { + true => &self.entries[..], + false => { + let start = self.visual_of(idx).unwrap_or(self.entries.len()); + &self.entries[start..] + } + } } /// The width of each header level, in display cells. Derived rather than @@ -921,6 +965,7 @@ mod tests { CellDisplay::number("3"), ])); grid.apply(&GridUpdate::RowFrame(AxisFrame { + first: 0, cats: vec!["Zone".into()], entries: grid.row_frame.entries.clone(), })); @@ -953,6 +998,7 @@ mod tests { /// A two-level axis: [A,x], [A,y], [B,z], with `B` in a collapsed group. fn two_level_frame() -> AxisFrame { AxisFrame { + first: 0, cats: vec!["Type".into(), "Who".into()], entries: vec![ FrameEntry::Item(vec!["A".into(), "x".into()]), @@ -1034,6 +1080,7 @@ mod tests { #[test] fn header_widths_are_per_level_and_measured_in_display_cells() { let frame = AxisFrame { + first: 0, cats: vec!["A".into(), "B".into()], entries: vec![FrameEntry::Item(vec!["Clothing".into(), "日本".into()])], }; @@ -1071,6 +1118,7 @@ mod tests { #[test] fn an_axis_with_no_categories_still_has_one_level() { let frame = AxisFrame { + first: 0, cats: Vec::new(), entries: vec![FrameEntry::Item(Vec::new())], }; @@ -1200,6 +1248,7 @@ mod tests { })) }; let axis = |cat: &str, labels: [&str; 2]| AxisFrame { + first: 0, cats: vec![cat.into()], entries: labels .iter() @@ -1225,6 +1274,7 @@ mod tests { fn renaming_a_row_category_invalidates_every_cell_under_it() { let mut grid = identified_grid(); let renamed = AxisFrame { + first: 0, cats: vec!["Department".into()], entries: grid.row_frame.entries.clone(), }; @@ -1264,6 +1314,7 @@ mod tests { fn renaming_a_column_category_invalidates_every_cell_under_it() { let mut grid = identified_grid(); let renamed = AxisFrame { + first: 0, cats: vec!["Metric".into()], entries: grid.col_frame.entries.clone(), }; diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index 30a20693..24f7202b 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -216,7 +216,10 @@ pub fn render_model(state: &ClientState) -> RenderModel { .body_from(0) .iter() .scan(0usize, |data_idx, entry| { + // `GroupEnd` is a boundary, not a row: it draws nothing and + // consumes no data index (improvise-gd83). let row = match entry { + FrameEntry::GroupEnd => return Some(None), FrameEntry::Group(group) => RenderRow::Group { label: group.name.clone(), collapsed: group.collapsed, @@ -244,8 +247,9 @@ pub fn render_model(state: &ClientState) -> RenderModel { } } }; - Some(row) + Some(Some(row)) }) + .flatten() .collect(), categories: entries .iter() @@ -443,26 +447,20 @@ fn col_levels(cols: &AxisFrame) -> Vec> { /// The column groups as runs, coalescing equal answers from /// `AxisFrame::group_of`. Nothing at all when no column is in a group. fn col_groups(cols: &AxisFrame) -> Vec { - let mut runs: Vec = Vec::new(); - let mut any = false; - for group in cols.groups() { - any |= group.is_some(); - let label = group.map(|g| g.name.clone()); - let collapsed = group.is_some_and(|g| g.collapsed); - // Coalesced by NAME alone, as `src/ui/grid.rs` does, and the run's - // indicator is its FIRST group's. See `context/code/clients.md`. - match runs.last_mut() { - Some(run) if run.label == label => run.span += 1, - _ => runs.push(RenderColGroup { - label, - collapsed, - span: 1, - }), - } - } + // The coalescing rule is `AxisFrame::group_runs`, which the TUI draws + // from too — one home, not two agreeing copies (improvise-8qlr). + let runs = cols.group_runs(); + let any = runs.iter().any(|(group, _)| group.is_some()); match any { - true => runs, false => Vec::new(), + true => runs + .into_iter() + .map(|(group, span)| RenderColGroup { + label: group.map(|g| g.name.clone()), + collapsed: group.is_some_and(|g| g.collapsed), + span, + }) + .collect(), } } @@ -520,6 +518,7 @@ mod tests { #[test] fn column_groups_coalesce_by_name_as_the_tui_does() { let frame = AxisFrame { + first: 0, cats: vec!["Month".into()], entries: vec![ group("Month", "Q1", false), diff --git a/harness/tests/grouped.spec.js b/harness/tests/grouped.spec.js index 3da05d91..a5fbd8df 100644 --- a/harness/tests/grouped.spec.js +++ b/harness/tests/grouped.spec.js @@ -10,6 +10,15 @@ import { test, expect } from "@playwright/test"; /// answers a missing seed with an EMPTY model -- so everything below asserts /// labels the fixture really carries. "A grid is drawn" would pass on the /// empty document a typo in the filename produces. +/// The first row that carries DATA. Not `tbody tr` first: since improvise-0o6x +/// a group header first on its axis is drawn, and `Coastal` is exactly that in +/// `grouped.improv` -- so the first row is a header alone, with no `td` at all. +const firstDataRow = (page) => + page + .locator("#grid tbody tr") + .filter({ hasNot: page.locator("th.group") }) + .first(); + async function openGrouped(page, doc) { await page.goto(`/app/index.html?doc=${doc}&seed=grouped.improv`); await expect(page.locator("#grid table")).toBeVisible({ timeout: 20_000 }); @@ -20,9 +29,7 @@ async function openGrouped(page, doc) { // half-drawn, and a one-shot read cannot wait. await expect(page.locator("#grid thead tr")).toHaveCount(3); await expect(page.locator("#grid thead th.group").first()).toHaveText("▼ Q1"); - await expect(page.locator("#grid tbody tr").first().locator("th").last()).toHaveText( - "Main", - ); + await expect(firstDataRow(page).locator("th").last()).toHaveText("Main"); await page.locator("#grid").focus(); } @@ -58,7 +65,7 @@ test.describe("a grouped, multi-level document", () => { // Two row categories, so the corner spans two — and the row headers below // must agree with it, or the table is ragged. expect(lines.every((l) => l.cornerSpan === 2)).toBe(true); - await expect(page.locator("#grid tbody tr").first().locator("th")).toHaveCount(2); + await expect(firstDataRow(page).locator("th")).toHaveCount(2); // The level lines. The blanks are repeat suppression, decided in Rust by // `AxisFrame::header_labels` — NOT an absent label, and not the page's @@ -111,7 +118,7 @@ test.describe("a grouped, multi-level document", () => { // that were merely "big enough" passes the pair above and fails here. const columns = lines[2].cells.length; expect(groups.reduce((n, c) => n + c.span, 0)).toBe(columns); - await expect(page.locator("#grid tbody tr").first().locator("td")).toHaveCount(columns); + await expect(firstDataRow(page).locator("td")).toHaveCount(columns); }); // `RenderRow::Group` carries no cells, so the page CANNOT draw one with @@ -125,12 +132,15 @@ test.describe("a grouped, multi-level document", () => { const groupRows = page.locator("#grid tbody tr", { has: page.locator("th.group"), }); - // Two, not three: `Coastal` is the frame's FIRST entry and a leading group - // header is dropped before it reaches the page — improvise-0o6x, open. If - // this count becomes 3, that bug is fixed and this line is the record of - // when. It is not an endorsement. - await expect(groupRows).toHaveCount(2); - await expect(groupRows).toHaveText(["▼ Inland", "▶ Upland"]); + // Three, not two: this WAS two, because `Coastal` is the frame's FIRST + // entry and a leading group header was dropped on the way to the page. + // improvise-0o6x fixed it, and this line is the record of when. + await expect(groupRows).toHaveCount(3); + await expect(groupRows).toHaveText([ + "▼ Coastal", + "▼ Inland", + "▶ Upland", + ]); // The glyph is `indicator(collapsed)`: a page that hard-coded either one // passes on one of these rows and fails on the other. expect(await groupRows.locator("td").count()).toBe(0); diff --git a/src/ui/grid.rs b/src/ui/grid.rs index fa86dd8e..fb844f3c 100644 --- a/src/ui/grid.rs +++ b/src/ui/grid.rs @@ -140,20 +140,23 @@ impl<'a> GridWidget<'a> { format!("{: = None; + // The run rule's one home, plus the TUI's one divergence from + // it at a scrolled offset — `context/code/protocol.md` §3a. + let run_heads = cols.group_run_heads(); for ci in visible_col_range.clone() { let x = col_x_at(ci); if x >= area.x + area.width { break; } let cw = col_w_at(ci) as usize; - let col_group = col_groups.get(ci).copied().flatten(); - let group_name = col_group.map(|g| g.name.clone()); - let label = match (group_name != prev_group, col_group) { - (true, Some(g)) => format!("{} {}", indicator(g.collapsed), g.name), - _ => String::new(), + let head = match ci == visible_col_range.start { + true => col_groups.get(ci).copied().flatten(), + false => run_heads.get(ci).copied().flatten(), + }; + let label = match head { + Some(g) => format!("{} {}", indicator(g.collapsed), g.name), + None => String::new(), }; - prev_group = group_name; buf.set_string( x, y, @@ -231,6 +234,9 @@ impl<'a> GridWidget<'a> { break; } match entry { + // A boundary, not a row: nothing is painted and no line is + // consumed (improvise-gd83). + FrameEntry::GroupEnd => continue, FrameEntry::Group(group) => { let label = format!("{} {}", indicator(group.collapsed), group.name); buf.set_string( @@ -895,12 +901,15 @@ mod tests { assert_eq!(text, GROUPED_FRAME, "\n--- got ---\n{text}\n--- end ---"); } + /// Re-pinned for improvise-0o6x: the leading `▼ Co…` is the ONE line that + /// moved, and its absence was the bug. context/test-evidence/tui.md. const GROUPED_FRAME: &str = concat!( "┌ View: Default ───────────────────────────┐\n", "│ [Payer = Alice] │\n", "│ ▼ Q1 ▼ Q2 │\n", "│ Jan Apr │\n", "│──────────────────────────────────────────│\n", + "│▼ Co…────────── │\n", "│East 100 100 │\n", "│▼ In…────────── │\n", "│West 50 50 │\n", @@ -910,7 +919,6 @@ mod tests { "│ │\n", "│ │\n", "│ │\n", - "│ │\n", "└──────────────────────────────────────────┘", ); } diff --git a/tests/grid_frame_agreement.rs b/tests/grid_frame_agreement.rs index b5160e2c..28334701 100644 --- a/tests/grid_frame_agreement.rs +++ b/tests/grid_frame_agreement.rs @@ -7,7 +7,7 @@ use improvise::ui::grid::GridWidget; use improvise::view::{Axis, GridLayout}; use improvise::workbook::Workbook; use improvise_command::ui::project; -use improvise_protocol::{GridCache, Snapshot, ViewState}; +use improvise_protocol::{FrameEntry, GridCache, Snapshot, ViewState}; use improvise_wasm_client::render::{RenderModel, RenderRow}; use ratatui::{buffer::Buffer, layout::Rect, widgets::Widget}; @@ -278,37 +278,96 @@ fn the_two_front_ends_draw_the_same_column_header() { .iter() .map(|g| (g.label.as_deref(), g.span)) .collect(); + // improvise-gd83: MEMBERSHIP, not nearest-preceding. Sep is ungrouped, so + // the run ends before it and resumes under the next measure's Q1 header. assert_eq!( named, - vec![(Some("Q1"), 6)], - "Q1 covers all six columns, INCLUDING the two under the ungrouped \ - Sep — `expand_category` emits a header on entering a group and no \ - boundary on leaving one, so the nearest-preceding walk keeps \ - answering Q1 (see `AxisFrame::group_of`). That is `GridLayout`'s \ - rule and the TUI paints the same span; it is reproduced here rather \ - than corrected, because correcting it changes what the TUI draws. \ - See `THE_MEMBERSHIP_CORRECT_SPANS` for what improvise-gd83 makes \ - this line say." + vec![(Some("Q1"), 2), (None, 1), (Some("Q1"), 2), (None, 1)], + "the two Q1 runs are separated by the ungrouped Sep; a run that \ + swallowed it would be `expand_category` leaking a group across its \ + own boundary (see `AxisFrame::group_of`)" ); - // A DELIBERATELY WRONG constant and a live tripwire — do not remove it. - // Derived, not guessed: see `context/test-evidence/tui.md`. - const THE_MEMBERSHIP_CORRECT_SPANS: [(Option<&str>, usize); 4] = - [(Some("Q1"), 2), (None, 1), (Some("Q1"), 2), (None, 1)]; - assert_ne!( - named.as_slice(), - THE_MEMBERSHIP_CORRECT_SPANS.as_slice(), - "when this stops differing, improvise-gd83 is fixed and the \ - assertion above should become the constant" + assert!( + named.iter().any(|(label, _)| label.is_none()), + "precondition: the fixture must contain an UNGROUPED column, or this \ + test cannot see a leaked group at all" ); - // And the group line the TUI paints names the same group. + // improvise-8qlr: the TUI's group line, compared EXACTLY. A `contains` + // passes while the TUI labels every column, which is a different grid. let group_line = &lines[rule - levels - 1]; - assert!( - group_line.contains("▼ Q1"), - "the TUI paints the same group above the same columns: {group_line:?}" + let expected: Vec = model + .col_groups + .iter() + .flat_map(|g| { + let head = match &g.label { + Some(name) => format!("{} {name}", indicator(g.collapsed)), + None => String::new(), + }; + std::iter::once(head).chain(std::iter::repeat_n(String::new(), g.span - 1)) + }) + .collect(); + assert_eq!( + &tui_header_cells(group_line, &cache), + &expected, + "the TUI's group line and the browser's runs disagree — one of them \ + coalesces the runs itself instead of asking `group_runs`" ); } +/// improvise-0o6x: a group FIRST on its axis. Separate from `two_level_rows`, +/// which orders Carol first to dodge this case on purpose. +fn leading_group_rows() -> Workbook { + let mut m = Workbook::new("Test"); + m.add_category("Recipient").unwrap(); + m.add_item_in_group("Recipient", "Alice", "Fam").unwrap(); + m.add_item_in_group("Recipient", "Bob", "Fam").unwrap(); + m.add_item("Recipient", "Carol").unwrap(); + m.add_item("_Measure", "Revenue").unwrap(); + m.active_view_mut().set_axis("Recipient", Axis::Row); + m.active_view_mut().set_axis("_Measure", Axis::Column); + for r in ["Alice", "Bob", "Carol"] { + m.model.set_cell( + coord(&[("Recipient", r), ("_Measure", "Revenue")]), + CellValue::Number(1.0), + ); + } + m +} + +/// A collapsed group at position zero is otherwise a trap: its rows are hidden +/// and its header is the only affordance. context/test-evidence/tui.md. +#[test] +fn a_group_header_first_on_its_axis_reaches_both_front_ends() { + for collapse in [false, true] { + let mut wb = leading_group_rows(); + if collapse { + wb.active_view_mut() + .toggle_group_collapse("Recipient", "Fam"); + } + let (_app, cache) = whole_grid(&mut wb); + + assert!( + matches!(cache.row_frame.entries.first(), Some(FrameEntry::Group(g)) if g.name == "Fam"), + "precondition (collapse={collapse}): the frame must LEAD with the \ + header, or body_from has nothing at position zero to drop" + ); + + let browser = browser_rows(&cache); + assert!( + matches!(browser.first(), Some(RenderRow::Group { label, .. }) if label == "Fam"), + "the browser dropped the leading group header: {browser:?}" + ); + + let painted = tui_row_headers(&wb, &cache); + assert_eq!( + painted.first().map(|r| r.join("").trim().to_string()), + Some(format!("{} Fam", indicator(collapse))), + "the TUI dropped the leading group header: {painted:?}" + ); + } +} + /// The bug's own words, separate because the agreement above would also hold /// if *both* ends started slash-joining. #[test] From 87696d8b9b712162cdd7ffcb63ba54486548a8e7 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 20:55:58 -0700 Subject: [PATCH 7/9] command: guard the page-flip no-op, and stop the geometry arm walking the cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on #76, both about code this PR itself introduced. **`SetPageSelection`'s `moved &&` guard was unguarded.** Deleting it left the whole workspace green at 1416, and it is not an edge case: `PageNext`/`PagePrev` are odometer-style and emit a `SetPageSelection` for every page category on every keypress, so the non-carrying categories are always no-ops and a single one-item category re-selects the same item. Unguarded, each such keystroke bumps `cursor_epoch`, sends the host cursor home and broadcasts `ResetCursor` to every client on that view — improvise-75ld's "a no-op that refused everybody else", one tier down. `selecting_the_page_already_selected_invalidates_no_cursor` fails under exactly that deletion, and asserts the fixture is really on Q1 and the row labels really did not move first. **The `LayoutGeometry` arm no longer reads the wide fingerprint.** Pointing both arms at `document_content` was an asymptotic change on the held-key path, not the constant factor the first hand-back claimed. Benchmarked at 18,000 cells, release, 200 batches after warm-up: a `SetCell` batch 8.07 ms → 10.21 ms (+27%, a constant factor, since `record_fingerprints` already walked the cells), but a `SetNumberFormat` batch 78 µs → **2.63 ms, 33×** — O(config) to O(cells+config), covering `[`/`]`, axis cycle, hide/show, group toggle, transpose and prune toggle, ~380/sec where they had been ~12,800/sec. `document_config` comes back for that arm and `document_content` is now `document_config` plus the cells and formulas, read only by `CellData`. It costs no correctness because all five 748f-blind effects are `CellData` — but that is a reading, and a reading is what this PR exists to distrust, so `every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint` asserts it over all thirteen. Dropping `view.number_format` from `document_config` fails it naming `SetNumberFormat`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 64 ++++++++++++- crates/improvise-command/src/ui/app.rs | 79 +++++++++++++--- crates/improvise-command/src/ui/effect.rs | 106 ++++++++++++++++++++++ 3 files changed, 234 insertions(+), 15 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index bf5d8567..c1c17615 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -455,10 +455,19 @@ and `RemoveFormula`, whose results live in `Model::formula_cache` and not in the store. Observing raw cells would have left every one of them reading clean, which is a *lost edit* — the opposite direction from the spurious prompt it was fixing. -So the observation is `document_content`, the former `document_config` widened to -cover the cells in iteration order and the formulas as well as the categories, items, -groups and view configuration it already carried. `CellData` and `LayoutGeometry` are -both read against it, in the loop, at the position gotcha 45 requires. +So the observation is `document_content` — `document_config` plus the cells in +iteration order and the formulas — and **only the `CellData` arm reads it**. The +`LayoutGeometry` arm keeps the narrow `document_config`. A first version pointed both +arms at the wide fingerprint, and review benchmarked what that costs at 18,000 cells +in release over 200 batches after warm-up: one `SetCell` batch 8.07 ms → 10.21 ms +(+27%, a constant factor on a path that was already O(cells) through +`record_fingerprints`), but one `SetNumberFormat` batch **78 µs → 2.63 ms, 33×** — +an *asymptotic* change from O(config) to O(cells + config) on the held-key path that +`[`/`]`, axis cycle, hide/show, group toggle, transpose and prune toggle all take. +~380/sec where they had been ~12,800/sec. The split costs no correctness because all +five blind effects are `CellData`; that every `LayoutGeometry` effect really is +visible to the narrow fingerprint is asserted separately, and not left as a reading. + `WholeWorkbook` stays *declared*, and that is the remaining over-report: a `LoadModel` whose incoming content happens to be identical still marks the document dirty, and a failed one does too. Both cost one redundant write. @@ -470,6 +479,33 @@ removes the fixture's formula first, or adding it is a no-op. Injected fault: narrow `document_content` back to the configuration it hashed before. Fails naming `SortData`, with five other dirty-flag tests. +### every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint + +The guard that makes the two-fingerprint split safe rather than a trap. Splitting +`CellData` (wide) from `LayoutGeometry` (narrow) is only sound while every effect +wearing `LayoutGeometry` writes something `document_config` hashes; an effect it +cannot see would leave the document reading clean **forever**, which is the same +lost-edit failure the wide fingerprint exists to prevent, arriving by the other door. + +The thirteen are enumerated by brace-matching the `impl Effect` blocks, not by +grepping — the naive grep reports fourteen, picking up `HelpPageSet`, which is +`ViewOnly` and is simply the last `pub struct` before `#[cfg(test)]` (gotcha 38 names +this exact artefact). Each case asserts its effect really declares +`Model(LayoutGeometry)` before asserting anything else, so a reclassified effect fails +here rather than silently leaving the list unrepresentative. + +Three cases need a `prepare` step run **directly against the workbook** and not +through an effect, because the interesting state is the precondition: `DeleteView` +needs a second view, `ShowItem` needs a hidden item, `ToggleGroup` needs a real +grouped item (it checks membership and is otherwise a no-op), and `TransposeAxes` +needs two axes that differ. `dirty` is cleared after `prepare`, or the preparation +itself would satisfy the assertion. + +Injected fault: `document_config` returns `0`. Fails, with five other dirty-flag tests +across three files. Injected fault, narrower and the one that proves the test is +pointed at the right thing: drop `view.number_format` from `document_config`. Fails +naming **`SetNumberFormat`**, with only one other test failing anywhere. + --- @@ -959,6 +995,26 @@ The negative arm of the above, and it is load-bearing for the same reason gotcha gives for `SetNumberFormat`: without it, "reset on every page flip" passes the positive test and costs the user their position on every `[`/`]`. +### selecting_the_page_already_selected_invalidates_no_cursor + +The third arm, and the one the user actually hits. `SetPageSelection` resets the +cursor only when the selection **moved**, and that guard shipped untested: deleting +`moved &&` left the whole workspace green at 1416, which review caught. + +It is not an edge case. `PageNext`/`PagePrev` are odometer-style and emit a +`SetPageSelection` for **every** page category on every keypress, so with more than +one page category the non-carrying categories are always no-ops, and a single +one-item category re-selects the same item every time. Unguarded, each such keystroke +bumps `cursor_epoch`, sends the host's cursor to the origin and broadcasts +`Command::ResetCursor` to every client on that view — improvise-75ld's "a no-op that +refused everybody else", one tier down and aimed at the cursor instead of the basis. + +The test asserts the fixture really is on `Q1` and that the row labels really did not +move before asserting the epoch stood still, so it cannot pass by re-selecting a page +that was never selected or by testing a grid that does not prune. + +Injected fault: delete `moved &&`. Fails by name, alone. + ### a_foreign_layout_is_not_shown_the_hosts_staged_drill_text improvise-pubt, filed with an argument for unreachability that the test disproves. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index dbb9f9c8..56a30005 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -80,8 +80,8 @@ fn view_shapes(workbook: &Workbook) -> std::collections::BTreeMap { .collect() } -/// Everything a model effect can write that the file would carry — cells and -/// formulas included, because `CellData` is wider than its name (improvise-748f). +/// `document_config` plus the cells and the formulas. `CellData` is wider than its +/// name (improvise-748f), and only that arm pays the walk — context/code/command.md. fn document_content(workbook: &Workbook) -> u64 { use std::hash::{Hash, Hasher}; let mut h = std::collections::hash_map::DefaultHasher::new(); @@ -94,6 +94,15 @@ fn document_content(workbook: &Workbook) -> u64 { formula.target.hash(&mut h); formula.target_category.hash(&mut h); } + document_config(workbook).hash(&mut h); + h.finish() +} + +/// Everything a `LayoutGeometry` effect can write that the file would carry. Not +/// the cells, so it stays O(config) on the held-key path (improvise-748f). +fn document_config(workbook: &Workbook) -> u64 { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); for (name, cat) in &workbook.model.categories { name.hash(&mut h); cat.kind.hash(&mut h); @@ -597,12 +606,19 @@ impl App { let touches_model = effects .iter() .any(|e| matches!(e.kind(), EffectKind::Model(_))); - let records_before = effects + let writes_cells = effects .iter() - .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))) - .then(|| record_fingerprints(&self.model_state.workbook.model.data)); + .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::CellData))); + let moves_geometry = effects + .iter() + .any(|e| matches!(e.kind(), EffectKind::Model(ModelChange::LayoutGeometry))); + let records_before = + writes_cells.then(|| record_fingerprints(&self.model_state.workbook.model.data)); let shapes_before = touches_model.then(|| view_shapes(&self.model_state.workbook)); - let mut content_basis = touches_model.then(|| document_content(&self.model_state.workbook)); + // Two bases, because the cheap one is the held-key path: `[`/`]`, axis + // cycle and hide/show must not walk 18,000 cells (improvise-748f). + let mut content_basis = writes_cells.then(|| document_content(&self.model_state.workbook)); + let mut config_basis = moves_geometry.then(|| document_config(&self.model_state.workbook)); let mut dirty = Dirty::NOTHING; for effect in effects { let kind = effect.kind(); @@ -615,7 +631,7 @@ impl App { // A replaced document differs from the file its path names even // where the content matches, so it is declared (improvise-748f). EffectKind::Model(ModelChange::WholeWorkbook) => true, - // Observed below, against `document_content`. + // Observed below, each against the fingerprint its tier needs. EffectKind::Model(ModelChange::CellData) => false, EffectKind::Model(ModelChange::LayoutGeometry) => false, }; @@ -624,15 +640,20 @@ impl App { } // The observed half, read **inside the loop** so `Save` still clears it // in loop order (gotcha 45) — the position is the property. - if matches!( - kind, - EffectKind::Model(ModelChange::CellData | ModelChange::LayoutGeometry) - ) && let Some(basis) = content_basis + if matches!(kind, EffectKind::Model(ModelChange::CellData)) + && let Some(basis) = content_basis { let now = document_content(&self.model_state.workbook); self.model_state.dirty |= now != basis; content_basis = Some(now); } + if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) + && let Some(basis) = config_basis + { + let now = document_config(&self.model_state.workbook); + self.model_state.dirty |= now != basis; + config_basis = Some(now); + } if self.abort_effects { // An abort-setting effect asked for the rest to be skipped. Reset // the flag, and record the outcome where a caller can read it. @@ -1138,6 +1159,42 @@ mod tests { ); } + /// The third arm, and the one `[`/`]` hits constantly: odometer paging emits a + /// `SetPageSelection` per page category per keypress, most of them no-ops. + #[test] + fn selecting_the_page_already_selected_invalidates_no_cursor() { + let mut app = paged_model(true); + let before_labels = row_labels(&app); + app.cursor_mut().selected = (1, 0); + let epoch = app.cursor_epoch(); + + app.apply_effects(vec![Box::new(super::super::effect::SetPageSelection { + category: "Quarter".to_string(), + item: "Q1".to_string(), + })]); + + assert_eq!( + app.model_state + .workbook + .active_view() + .page_selection("Quarter"), + Some("Q1"), + "the fixture was not on Q1, so re-selecting it is not the no-op case" + ); + assert_eq!( + before_labels, + row_labels(&app), + "re-selecting the same page renumbered the grid, so this proves nothing" + ); + assert_eq!( + app.cursor_epoch(), + epoch, + "a page that did not move threw every client's cursor away, and \ + odometer paging emits one of these per page category per keypress" + ); + assert_eq!(app.cursor().selected, (1, 0)); + } + /// The other arm: with pruning off the page names the same rows, so paging /// must not throw the user's position away on an ordinary `[`/`]`. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 38d75abe..fdc1d804 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -2443,6 +2443,112 @@ mod tests { } } + /// improvise-748f: the `LayoutGeometry` arm is observed against the **narrow** + /// `document_config`, so an effect it cannot see would read clean forever. + #[test] + fn every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint() { + type Prepare = fn(&mut App); + type Make = fn() -> Box; + let nothing: Prepare = |_| {}; + let cases: [(&str, Prepare, Make); 13] = [ + ("AddCategory", nothing, || { + Box::new(AddCategory("Region".into())) + }), + ("RemoveCategory", nothing, || { + Box::new(RemoveCategory("Month".into())) + }), + ("CreateView", nothing, || { + Box::new(CreateView("Second".into())) + }), + ( + "DeleteView", + |app| { + app.model_state.workbook.create_view("Second"); + }, + || Box::new(DeleteView("Second".into())), + ), + ("SetAxis", nothing, || { + Box::new(SetAxis { + category: "Type".into(), + axis: Axis::Column, + }) + }), + ("SetPageSelection", nothing, || { + Box::new(SetPageSelection { + category: "Type".into(), + item: "Food".into(), + }) + }), + ( + "ToggleGroup", + |app| { + app.model_state + .workbook + .add_item_in_group("Type", "Cake", "Baked") + .unwrap(); + }, + || { + Box::new(ToggleGroup { + category: "Type".into(), + group: "Baked".into(), + }) + }, + ), + ("HideItem", nothing, || { + Box::new(HideItem { + category: "Type".into(), + item: "Food".into(), + }) + }), + ( + "ShowItem", + |app| { + app.view_mut().hide_item("Type", "Food"); + }, + || { + Box::new(ShowItem { + category: "Type".into(), + item: "Food".into(), + }) + }, + ), + ( + "TransposeAxes", + |app| { + app.view_mut().set_axis("Type", Axis::Row); + app.view_mut().set_axis("Month", Axis::Column); + }, + || Box::new(TransposeAxes), + ), + ("CycleAxis", nothing, || Box::new(CycleAxis("Type".into()))), + ("SetNumberFormat", nothing, || { + Box::new(SetNumberFormat(",.4f".into())) + }), + ("TogglePruneEmpty", nothing, || Box::new(TogglePruneEmpty)), + ]; + + for (label, prepare, make) in cases { + let mut app = test_app(); + prepare(&mut app); + app.model_state.dirty = false; + + let effect = make(); + assert!( + matches!( + effect.kind(), + EffectKind::Model(ModelChange::LayoutGeometry) + ), + "{label} is not a LayoutGeometry effect, so this case belongs elsewhere" + ); + app.apply_effects(vec![effect]); + assert!( + app.model_state.dirty, + "{label} changed the document and `document_config` could not see it, \ + so the document reads clean forever" + ); + } + } + /// A staged edit to a measure column writes that one cell (improvise-c5v). #[test] fn apply_and_clear_drill_with_measure_edit() { From bd3967cc7cf6f39dbc449a960a52b9e66136b152 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:01:02 -0700 Subject: [PATCH 8/9] =?UTF-8?q?fix:=20guard=20improvise-mai9=20at=20the=20?= =?UTF-8?q?call=20site,=20and=20correct=20=C2=A73's=20group=5Fof?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of PR #77 found the mai9 fix unprotected and one doc paragraph contradicting another. Both were right; both are addressed here. The guard was aimed at the wrong thing -------------------------------------- The mai9 defect was in the WIRING — `draw_bottom_bar` built `format!("{}{}", mb.prompt, buf)` plus the cursor glyph and never consulted `status_msg`. Both tests called the composing helper `minibuffer_line` DIRECTLY, so nothing exercised the call site. Reverting that one line restored the bug in full with the suite green at 1411, reproduced here before rewriting. The helper did not even go dead: the test module's `use` kept it alive while it asserted nothing about the product. `a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open` now renders `draw_bottom_bar` through a real `Terminal` and reads the row the terminal received. That is the only assertion that can fail for the real defect. `the_minibuffer_prompt_is_not_displaced_by_the_message` is beside it because a "fix" that dropped to `draw_status` would satisfy the first on the message alone. Injected fault: the pre-mai9 `format!` — both fail, the first naming the missing message and printing the row it did compose. And the negative arm could not fail, twice over ----------------------------------------------- `fill_line` computes `pad = width - left.len() - right.len()`, so with an empty status an unconditional `format!(" {msg} ")` produces a BYTE-IDENTICAL row at any width where the line fits. The `match status_msg.is_empty()` is observable only in the band where the prompt fits but `left.len() + 4` does not. Rewriting it against that band was not enough: the first attempt derived its lower bound by calling `minibuffer_line` itself, so the injected fault moved the bound with it and the loop skipped the failing band entirely. The bound is now spelled locally. A bound read off the function under test is not a bound. Injected fault: `right` built unconditionally. Now fails at width 16 with "an empty status still composed 20 columns". context/code/protocol.md contradicted itself, 270 lines apart ------------------------------------------------------------- §3 still said `group_of` answers "nearest preceding header", DELIBERATELY, and that encoding an end boundary "is filed rather than smuggled into" the anr9 migration — while §3a, added by this branch, says `GroupEnd` is the boundary. A reader arriving at §3 first would conclude this PR smuggled in exactly what that paragraph forbids. It did not; improvise-gd83 IS the filed ticket. §3a was written as an addition where it needed to be a CORRECTION, and gotcha 48 turned on its author — worse than the `bindings.rs` case only because both copies sit in one file. §3 now records that the rule changed and which ticket changed it, and §3a points at the correction rather than restating the history. The neighbouring paragraph was stale too: `body_from`'s argument no longer exists "only because the two front ends scroll differently". Filed, not fixed ---------------- * improvise-1wl0: `#[serde(default)]` on `AxisFrame::first` forges the permissive claim — `first: 0` asserts "this frame leads the axis", which is what `body_from` acts on, so a frame that loses the field deserializes as leading. Same argument that gave `Basis` no `Default`. No skew ships today. * improvise-h0iv: `group_runs` coalesces by name alone where `FrameGroup` has `cat` to fix it. Right call for a consolidation — both prior copies did the same, and changing the rule while moving it would have cost the agreement test its evidence — but a single home reads as decided, which is the new risk and the reason to file. Both are cited in `context/code/protocol.md` §3a, so the single home does not read as settled. 1412 tests pass (was 1411; +3 wiring tests, -2 helper-only tests). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/protocol.md | 60 +++++++++++++++++------- context/test-evidence/tui.md | 54 +++++++++++++++------ src/draw.rs | 91 ++++++++++++++++++++++++++++++------ 3 files changed, 158 insertions(+), 47 deletions(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 9084b7e1..c8810e9b 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -477,20 +477,30 @@ the top edge (improvise-jjx). **`body_from(idx)`** drops the group header that owns the first drawn item: a frame opening mid-group carries that header so `group_of` can answer for it, but it is *above* the first drawn row and must not be painted as one. This is -the same cut `GridLayout::data_row_to_visual` has always made. Its argument -exists only because the two front ends scroll differently — a browser's frame -is already windowed and passes `0`, the TUI's covers the whole grid and passes -its row offset (gotcha 44) — **not** because the rule differs. +the same cut `GridLayout::data_row_to_visual` has always made. -**`group_of` answers "nearest preceding header", not membership, and -deliberately so: it is what the TUI has always painted.** `expand_category` -emits a header on *entering* a group and no boundary on leaving one, so an -ungrouped item following a grouped run is answered with the run's group — and -the TUI's column-group row spans it under that label today. Encoding -membership, or an end boundary, means changing `AxisEntry` in -`improvise-core` and changing what the TUI paints: a fix to the layout, not to -this projection of it, and it is filed rather than smuggled into the migration -that made the two ends agree (improvise-anr9, raised by Copilot on PR #23). +Two things about its argument, and the first of them **used to be the whole +story and was wrong**. It exists partly because the two front ends scroll +differently — a browser's frame is already windowed and passes `0`, the TUI's +covers the whole grid and passes its row offset (gotcha 44). But `idx` alone +cannot say whether the frame *leads the axis*, and at the top there is no +header "above" to drop: it is content, and a collapsed first group has no +other affordance. That is `AxisFrame::first` and improvise-0o6x — §3a. + +**`group_of` answers membership**, and the paragraph that stood here said the +opposite: that it answers "nearest preceding header", deliberately, because +that is what the TUI had always painted, and that encoding an end boundary +would mean changing `AxisEntry` in `improvise-core` and was "filed rather than +smuggled into" improvise-anr9. All of that was true when written. **The filed +ticket is improvise-gd83 and it has landed** — `AxisEntry::GroupEnd` closes a +run, `groups()` clears on it, and the TUI's painted span changed with it. The +mechanism and its two emission arms are §3a. + +That correction is here rather than only in §3a on purpose. This paragraph +forbade, in terms, the change §3a describes; a reader arriving at the stale +copy first would conclude the rule had been smuggled past its own prohibition. +When a fact is stated twice, changing it is a grep (gotcha 48) — and the two +copies being in *one file* is what made this worth saying out loud. ### The width rule has one home @@ -755,10 +765,12 @@ Nothing is defined for the help overlay: its content is static and Two facts `AxisFrame` gained after `improvise-anr9`, both about group runs. -**`FrameEntry::GroupEnd` closes a run.** `group_of` was documented as answering -"nearest preceding header", which is not membership: `expand_category` emitted a -header on *entering* a group and nothing on leaving one, so an ungrouped item -after a grouped run inherited it (`improvise-gd83`). `AxisEntry::GroupEnd` is +**`FrameEntry::GroupEnd` closes a run.** `group_of` used to answer "nearest +preceding header", which is not membership: `expand_category` emitted a header +on *entering* a group and nothing on leaving one, so an ungrouped item after a +grouped run inherited it (`improvise-gd83`). The §3 paragraph that recorded the +old rule — and forbade this change — is corrected there, not merely superseded +here. `AxisEntry::GroupEnd` is the boundary, carried onto the wire as `FrameEntry::GroupEnd`; `groups()` clears `current` on it and `group_of` stops walking. It is emitted only on `Some -> None`, never between two groups — the next header already ends the @@ -780,11 +792,23 @@ The TUI builds over the whole grid so `first` is always `0` there, and the browser's comes from the projected window. A frame built by hand in a test must set it, which is why it is a plain field rather than derived. +It carries `#[serde(default)]`, and **that is a defect rather than a decision** +(improvise-1wl0): `first: 0` is not an absence but the claim "this frame leads +the axis", which is the claim `body_from` acts on — so a frame that loses the +field deserializes as leading. No skew ships today, both ends being one +workspace at one commit. Same argument as `Basis` having no `Default`. + **`group_runs` / `group_run_heads` are the coalescing rule's one home.** The TUI and `render.rs::col_groups` each coalesced adjacent columns sharing a group, kept in step by a comment (`improvise-8qlr`). Runs coalesce by group **name**, so two adjacent runs under two different headers of the same name read as one — -that is what both ends did before and is preserved deliberately. The TUI's one +that is what both ends did before and is preserved deliberately, because +changing the rule in the commit that moves it would leave the agreement test +unable to tell "both ends agree" from "both ends changed together". + +`FrameGroup` carries `cat` and could fix it, and **a single home reads as +decided** — which is the new risk and why it is filed as improvise-h0iv rather +than left as a note. The TUI's one divergence is that it labels its first *visible* column even mid-run, so a scrolled grid does not draw an unlabelled group line. diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index a4ca56c6..f4e8df67 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -416,36 +416,62 @@ produce 1–4 items rather than 0. Each invocation is seeded from the current ti plus the PID. -### a_status_message_set_while_a_minibuffer_is_open_is_on_the_bottom_row +### a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open -### with_no_message_pending_the_minibuffer_row_is_only_the_minibuffer +### the_minibuffer_prompt_is_not_displaced_by_the_message + +### an_empty_message_never_pushes_a_row_that_fits_past_its_width improvise-mai9. `draw_bottom_bar` rendered the minibuffer **instead of** the status line, so a message written while a text-entry mode was open was never drawn — not truncated, not overwritten. A test asserting `app.status_msg` was -set passed in every one of those cases, which is exactly how this survived -improvise-6t3's careful effect ordering and why the guard has to be aimed at -the *composed row* rather than at the field. +set passed in every one of those cases, which is how this survived +improvise-6t3's careful effect ordering. Of the ticket's two candidate fixes, the row is **shared** rather than the callers being made structurally unable to report: giving the status a second row changes frame geometry (improvise-q0vb's cost), and "always leave the mode before reporting" is what `CommitSave` does by hand and would have to be repeated at every text-entry commit — the thing the ticket calls a workaround. -Sharing the row costs one function and generalises to all eight modes. -The second test is the negative arm and is not decoration: a composition that -appended the status unconditionally would pass the first and draw two trailing -spaces of nothing on every keystroke of every minibuffer. +**The first version of these tests could not fail for the bug, and that is the +entry worth reading.** They called the composing helper `minibuffer_line` +directly. The defect was in the *wiring* — `draw_bottom_bar` building its own +`format!` and never consulting `status_msg` — so reverting that one line +restored the bug in full with all 1411 tests green. The helper did not even go +dead: the test module's `use` kept it alive while it asserted nothing about the +product. Found by review (PR #77), reproduced here before rewriting. + +So the guard is now `Buffer`-backed: a real `Terminal` renders +`draw_bottom_bar` and the assertion reads the row the terminal received. That +is the only assertion that can fail for mai9's actual defect. Fault injected by +restoring `format!("{}{}", mb.prompt, buf)` plus the cursor glyph — both wiring +tests fail, the first naming the missing message and printing the row it did +compose. + +The second test exists because a "fix" that dropped to `draw_status` would +satisfy the first on the message alone; it pins that the prompt still leads. + +**The third test is the negative arm, and its first version was unfalsifiable +for arithmetic reasons.** `fill_line` computes `pad = width - left.len() - +right.len()`, so with an empty status an unconditional `format!(" {msg} ")` +produces a byte-identical row at any width where the line fits: `left` plus +`width - left.len()` spaces, either way. The `match status_msg.is_empty()` is +observable **only** in the band where the prompt fits but `left.len() + 4` +does not — widths 16..=19 for this fixture, where the unguarded arm returns a +row wider than the terminal. + +That test then failed a second time in a more interesting way: it derived its +lower bound by *calling `minibuffer_line` itself*, so the injected fault moved +the bound with it and the loop skipped the failing band entirely. The bound is +now spelled locally. **A bound read off the function under test is not a +bound** — it is the fault grading its own exam. Known and not fixed here: `fill_line` pads on `left.len()`, which is bytes, so the right-hand message sits two cells further right than intended whenever the left contains the multi-byte cursor glyph. `draw_status` has always done this -with its own multi-byte content; it is pre-existing and out of scope. - -Fault injected by restoring `format!("{}{}the cursor glyph", prompt, buf)` (here: the -`false` arm returning `String::new()`): the first test fails naming the missing -message and printing the row it did compose. +with its own multi-byte content; it is pre-existing and out of scope. It is +also what makes the band above four columns wide rather than two. ### a_group_header_first_on_its_axis_reaches_both_front_ends diff --git a/src/draw.rs b/src/draw.rs index c00aacbe..fb46dfff 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -540,32 +540,93 @@ fn draw_welcome(f: &mut Frame, area: Rect) { #[cfg(test)] mod bottom_bar_tests { - use super::minibuffer_line; + use super::{draw_bottom_bar, minibuffer_line}; + use crate::ui::app::{App, AppMode}; + use crate::workbook::Workbook; + use ratatui::{Terminal, backend::TestBackend, layout::Rect}; - /// improvise-mai9: the message used not to be drawn at all — see - /// context/test-evidence/tui.md. + /// The bottom row as the terminal really receives it, so the assertion is + /// about `draw_bottom_bar`'s wiring and not about a helper it may not call. + fn bottom_row(app: &App, width: u16) -> String { + let mut terminal = Terminal::new(TestBackend::new(width, 1)).expect("test backend"); + terminal + .draw(|f| draw_bottom_bar(f, Rect::new(0, 0, width, 1), app)) + .expect("draw"); + terminal + .backend() + .buffer() + .content() + .iter() + .map(|c| c.symbol()) + .collect() + } + + fn app_with_minibuffer_and_message() -> App { + let mut app = App::new(Workbook::new("T"), None); + app.view_state.mode = AppMode::category_add(); + let key = app + .view_state + .mode + .minibuffer() + .expect("CategoryAdd carries a MinibufferConfig") + .buffer_key + .clone(); + app.view_state.buffers.insert(key, "North".to_string()); + app.view_state.status_msg = "category already exists".to_string(); + app + } + + /// improvise-mai9, and the ONE assertion that can fail for the real defect + /// — see context/test-evidence/tui.md. #[test] - fn a_status_message_set_while_a_minibuffer_is_open_is_on_the_bottom_row() { - let line = minibuffer_line("Category: ", "North", "category already exists", 80); + fn a_status_message_reaches_the_bottom_row_while_a_minibuffer_is_open() { + let app = app_with_minibuffer_and_message(); + let row = bottom_row(&app, 80); + assert!( - line.contains("Category: North"), - "the minibuffer still owns the left of the row: {line:?}" + row.contains("North"), + "the minibuffer must still own the left of the row: {row:?}" ); assert!( - line.contains("category already exists"), - "the status message must reach the user: {line:?}" + row.contains("category already exists"), + "the status message is never drawn — the bug is in draw_bottom_bar, \ + which used to render the minibuffer INSTEAD of the status line, so \ + a test that calls the composing helper directly cannot see it: {row:?}" ); } + /// The minibuffer is still the thing that owns the row: a fix that dropped + /// to `draw_status` would pass the assertion above on the message alone. #[test] - fn with_no_message_pending_the_minibuffer_row_is_only_the_minibuffer() { - let line = minibuffer_line("Category: ", "North", "", 80); - assert_eq!( - line.trim_end(), - "Category: North\u{258c}", - "an empty status adds nothing but padding" + fn the_minibuffer_prompt_is_not_displaced_by_the_message() { + let app = app_with_minibuffer_and_message(); + let row = bottom_row(&app, 80); + let prompt_at = row.find("North").expect("the buffer is drawn"); + let message_at = row.find("category already exists").expect("the message"); + assert!( + prompt_at < message_at, + "the prompt leads and the message follows it: {row:?}" ); } + + /// The guard's ONLY observable effect, in the narrow band where the two + /// arms differ at all. Why the obvious negative arm cannot: tui.md. + #[test] + fn an_empty_message_never_pushes_a_row_that_fits_past_its_width() { + // Spelled here, NOT taken from `minibuffer_line`: a bound read off the + // function under test moves with the fault and skips the failing band. + let fits_from = "Category: North\u{258c}".chars().count() as u16; + for width in fits_from..fits_from + 24 { + let line = minibuffer_line("Category: ", "North", "", width); + assert!( + line.chars().count() <= width as usize, + "width {width}: an empty status still composed {} columns \ + from {line:?} — an unconditional append pads for a message \ + that is not there", + line.chars().count() + ); + } + } } #[cfg(test)] From 970bae8ad5c0919bc0e5dbffbdaf54d45b0d2e7b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Sun, 20 Sep 2026 21:01:06 -0700 Subject: [PATCH 9/9] docs: the geometry-visibility evidence said three and listed four MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `every_layout_geometry_effect_is_visible_to_the_narrow_fingerprint`'s entry counted its `prepare` cases wrong, and the count is the load-bearing part: four of the thirteen assert nothing without a precondition, so an entry that reads "three" invites a reader to find the fourth redundant and delete it. Names why each one is needed — `ToggleGroup` checks membership and is otherwise a no-op, `TransposeAxes` on two identical axes is the identity — and states that clearing `dirty` after `prepare` rather than before is what stops the preparation from satisfying the assertion on its own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index c1c17615..ad73a6df 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -494,12 +494,15 @@ this exact artefact). Each case asserts its effect really declares `Model(LayoutGeometry)` before asserting anything else, so a reclassified effect fails here rather than silently leaving the list unrepresentative. -Three cases need a `prepare` step run **directly against the workbook** and not -through an effect, because the interesting state is the precondition: `DeleteView` -needs a second view, `ShowItem` needs a hidden item, `ToggleGroup` needs a real -grouped item (it checks membership and is otherwise a no-op), and `TransposeAxes` -needs two axes that differ. `dirty` is cleared after `prepare`, or the preparation -itself would satisfy the assertion. +**Four** cases need a `prepare` step run **directly against the workbook** and not +through an effect, because the interesting state is the precondition, and this is +the reason the test is not vacuous: `DeleteView` needs a second view, `ShowItem` +needs a hidden item, `ToggleGroup` needs a real grouped item (it checks membership +and is otherwise a no-op that would assert nothing), and `TransposeAxes` needs two +axes that differ (transposing two identical axes is the identity). + +`dirty` is cleared **after** `prepare` and before the effect, or the preparation +itself satisfies the assertion and all four cases pass against any fingerprint. Injected fault: `document_config` returns `0`. Fails, with five other dirty-flag tests across three files. Injected fault, narrower and the one that proves the test is