From f04fe517aea4c30c990b456acb9b9fe0f66f297d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 9 Jun 2026 21:43:13 -0700 Subject: [PATCH] fix(core): improve view robustness and axis management `records_display` now returns `None` for out-of-bounds columns. Added `try_axis_of` to `View` for non-panicking axis retrieval. `cycle_axis` now uses `try_axis_of` to avoid panicking on unknown categories. Add regression tests for out-of-bounds access and unknown category cycling. Co-Authored-By: fiddlerwoaroof/git-smart-commit (unsloth/gemma-4-26B-A4B-it-GGUF:UD-Q5_K_XL) --- crates/improvise-core/src/view/layout.rs | 23 +++++++- crates/improvise-core/src/view/types.rs | 71 +++++++++++++++++++----- 2 files changed, 79 insertions(+), 15 deletions(-) diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 4b3fe1c..9306c76 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -182,11 +182,15 @@ impl GridLayout { } /// Get the display string for the cell at (row, col) in records mode. - /// Returns None for normal (non-records) layouts. + /// Returns None for normal (non-records) layouts and for out-of-bounds + /// rows/columns (symmetric with `cell_key`). pub fn records_display(&self, row: usize, col: usize) -> Option { let records = self.records.as_ref()?; let record = records.get(row)?; let col_item = self.col_label(col); + if col_item.is_empty() { + return None; + } if col_item == "Value" { Some(record.1.to_string()) } else { @@ -806,6 +810,23 @@ mod tests { assert_eq!(last_value, "999", "new record should be the last row"); } + /// Regression test for improvise-byz: `records_display` for an + /// out-of-bounds column called `col_label(col)`, which returns `""`; + /// `""` fails the `== "Value"` check, gets looked up as a category name + /// in the record (never found), and the function returned `Some("")` — + /// masking view/data mismatches. It must return `None`, symmetric with + /// `cell_key`, which explicitly returns `None` for an empty col label. + #[test] + fn records_display_out_of_bounds_col_returns_none() { + let mut wb = records_workbook(); + let v = wb.active_view_mut(); + v.set_axis("_Index", Axis::Row); + v.set_axis("_Dim", Axis::Column); + let layout = GridLayout::new(&wb.model, wb.active_view()); + assert!(layout.is_records_mode()); + assert_eq!(layout.records_display(0, layout.col_count()), None); + } + fn coord(pairs: &[(&str, &str)]) -> CellKey { CellKey::new( pairs diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index 1e6f1f1..b461c27 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -111,13 +111,28 @@ impl View { } } + /// Axis assignment for a registered category. + /// + /// # Panics + /// + /// Panics if `cat_name` is not registered with this view. Registration + /// is guaranteed for every model category: `Workbook::new`, + /// `Workbook::add_category`, `Workbook::add_label_category`, and + /// `Workbook::create_view` all call `View::on_category_added` for every + /// view, so any name obtained from `Model::categories` or from this + /// view's own `category_axes` is safe. Use [`View::try_axis_of`] when + /// the name comes from anywhere else. pub fn axis_of(&self, cat_name: &str) -> Axis { - *self - .category_axes - .get(cat_name) + self.try_axis_of(cat_name) .expect("axis_of called for category not registered with this view") } + /// Non-panicking variant of [`View::axis_of`]: returns `None` when the + /// category is not registered with this view. + pub fn try_axis_of(&self, cat_name: &str) -> Option { + self.category_axes.get(cat_name).copied() + } + pub fn categories_on(&self, axis: Axis) -> Vec<&str> { self.category_axes .iter() @@ -208,18 +223,21 @@ impl View { self.col_offset = 0; } - /// Cycle axis for a category: Row → Column → Page → None → Row + /// Cycle axis for a category: Row → Column → Page → None → Row. + /// No-op for an unregistered category, consistent with `set_axis`. pub fn cycle_axis(&mut self, cat_name: &str) { - let next = match self.axis_of(cat_name) { - Axis::Row => Axis::Column, - Axis::Column => Axis::Page, - Axis::Page => Axis::None, - Axis::None => Axis::Row, - }; - self.set_axis(cat_name, next); - self.selected = (0, 0); - self.row_offset = 0; - self.col_offset = 0; + if let Some(current) = self.try_axis_of(cat_name) { + let next = match current { + Axis::Row => Axis::Column, + Axis::Column => Axis::Page, + Axis::Page => Axis::None, + Axis::None => Axis::Row, + }; + self.set_axis(cat_name, next); + self.selected = (0, 0); + self.row_offset = 0; + self.col_offset = 0; + } } } @@ -305,6 +323,31 @@ mod tests { v.axis_of("Ghost"); } + /// Regression test for improvise-t8s: `cycle_axis` panicked (via the + /// `expect` in `axis_of`) when given a category not registered with the + /// view, while `set_axis` silently no-ops on the same input. `cycle_axis` + /// now goes through `try_axis_of` and no-ops on unknown categories, + /// consistent with `set_axis`. + #[test] + fn cycle_axis_unknown_category_is_noop() { + let mut v = view_with_cats(&["Region"]); + v.cycle_axis("Ghost"); + assert_eq!(v.axis_of("Region"), Axis::Row); + assert_eq!(v.try_axis_of("Ghost"), None); + } + + #[test] + fn try_axis_of_known_category_returns_some() { + let v = view_with_cats(&["Region"]); + assert_eq!(v.try_axis_of("Region"), Some(Axis::Row)); + } + + #[test] + fn try_axis_of_unknown_category_returns_none() { + let v = View::new("Test"); + assert_eq!(v.try_axis_of("Ghost"), None); + } + #[test] fn page_selection_set_and_get() { let mut v = view_with_cats(&["Region", "Product", "Time"]);