From ab9d3716a87102ce58df23f990b304dfe597fa53 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 1 Aug 2026 17:08:32 +0100 Subject: [PATCH] fix(tui): yank cell/row no longer reports "No row selected" Yank resolved its row from Buffer::get_selected_row(), which was backed by ratatui's TableState - a second source of truth for the crosshair that only row navigation (sync_row_state) and query execution (reset_navigation_state) ever wrote. Loading a file never initialised it, so on a view narrowed to a single row - where j/k can never fire - `yv` and `yy` refused to copy a perfectly visible cell. The `f` text filter made it worse: apply_text_filter_with_refs applied the filter and set a status message, nothing else, unlike the fuzzy filter which resets selection, scroll and the viewport crosshair to the first match. - get_selected_row() now derives from view_state.crosshair_row, the position the table actually renders. None means "no visible rows", and the row is clamped to the view - so a filter that shrinks the results can no longer leave yank reading past the end and copying "NULL". TableState is untouched and still drives rendering. - apply_text_filter_with_refs now resets to the first match like the fuzzy filter does, which is also the better behaviour on its own. Two existing tests asserted the old contract against buffers holding no data at all; they now install data, since an empty buffer having no selection is correct under the new semantics. Co-Authored-By: Claude Opus 5 --- src/buffer.rs | 29 +++++- src/ui/enhanced_tui.rs | 7 +- src/ui/state/state_coordinator.rs | 30 +++++++ tests/main.rs | 3 + tests/test_buffer.rs | 35 +++++++- tests/test_buffer_state_refactor.rs | 22 +++++ tests/test_yank_single_row_repro.rs | 135 ++++++++++++++++++++++++++++ 7 files changed, 254 insertions(+), 7 deletions(-) create mode 100644 tests/test_yank_single_row_repro.rs diff --git a/src/buffer.rs b/src/buffer.rs index 5f65afd9..95f84d94 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -631,9 +631,20 @@ impl BufferAPI for Buffer { // --- Table Navigation --- fn get_selected_row(&self) -> Option { - // For backward compatibility, check if table_state has a selection - // This maintains the old API behavior where None means no selection - self.table_state.selected() + // The crosshair is the position the table actually renders, so derive the + // selection from it rather than from table_state. table_state is only written + // by row navigation and query execution, so it stays None after a file load + // (or after a filter that leaves a single row, where j/k never fires) - which + // used to make yank report "No row selected" on a perfectly visible cell. + // + // None now means "there is no data", and the row is clamped to the current + // view so a filter that shrinks the results can't leave us reading past the end. + let row_count = self.visible_row_count(); + if row_count == 0 { + None + } else { + Some(self.view_state.crosshair_row.min(row_count - 1)) + } } fn set_selected_row(&mut self, row: Option) { @@ -1264,6 +1275,18 @@ impl BufferAPI for Buffer { } impl Buffer { + /// Number of rows currently visible, preferring the `DataView` (which knows about + /// filtering) and falling back to the raw `DataTable` for legacy buffers. + fn visible_row_count(&self) -> usize { + if let Some(ref dataview) = self.dataview { + dataview.row_count() + } else if let Some(ref datatable) = self.datatable { + datatable.row_count() + } else { + 0 + } + } + /// Create a new empty buffer #[must_use] pub fn new(id: usize) -> Self { diff --git a/src/ui/enhanced_tui.rs b/src/ui/enhanced_tui.rs index 25295d71..26b14be8 100644 --- a/src/ui/enhanced_tui.rs +++ b/src/ui/enhanced_tui.rs @@ -4911,8 +4911,11 @@ impl EnhancedTuiApp { // Delegate state coordination to StateCoordinator use crate::ui::state::state_coordinator::StateCoordinator; - let _rows_after = - StateCoordinator::apply_text_filter_with_refs(&mut self.state_container, pattern); + let _rows_after = StateCoordinator::apply_text_filter_with_refs( + &mut self.state_container, + &self.viewport_manager, + pattern, + ); // Update ViewportManager with the filtered DataView // Sync the dataview to both managers diff --git a/src/ui/state/state_coordinator.rs b/src/ui/state/state_coordinator.rs index 9f8ff96a..487ccaae 100644 --- a/src/ui/state/state_coordinator.rs +++ b/src/ui/state/state_coordinator.rs @@ -351,6 +351,7 @@ impl StateCoordinator { /// Returns the number of matching rows pub fn apply_text_filter_with_refs( state_container: &mut AppStateContainer, + viewport_manager: &RefCell>, pattern: &str, ) -> usize { let case_insensitive = state_container.is_case_insensitive(); @@ -375,6 +376,35 @@ impl StateCoordinator { 0 }; + // Reset navigation to the first match, the same way the fuzzy filter does. + // Without this the crosshair keeps pointing at wherever it was before the + // filter, which can now be past the end of the narrowed view. + if rows_after > 0 { + // Preserve horizontal scroll + let col_offset = state_container.get_scroll_offset().1; + + state_container.set_selected_row(Some(0)); + state_container.set_scroll_offset((0, col_offset)); + state_container.set_table_selected_row(Some(0)); + + { + let mut nav = state_container.navigation_mut(); + nav.selected_row = 0; + nav.scroll_offset.0 = 0; + } + + if let Ok(mut vm_borrow) = viewport_manager.try_borrow_mut() { + if let Some(ref mut vm) = *vm_borrow { + vm.set_crosshair_row(0); + vm.set_scroll_offset(0, col_offset); + debug!( + "StateCoordinator: Reset viewport to first match (row 0) with {} total matches", + rows_after + ); + } + } + } + // Update status message let status = if pattern.is_empty() { "Filter cleared".to_string() diff --git a/tests/main.rs b/tests/main.rs index 82bfcc92..3342266c 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -199,6 +199,9 @@ mod test_window_context; #[path = "test_yanked_query.rs"] mod test_yanked_query; +#[path = "test_yank_single_row_repro.rs"] +mod test_yank_single_row_repro; + #[path = "tui_integration_test.rs"] mod tui_integration_test; diff --git a/tests/test_buffer.rs b/tests/test_buffer.rs index 9f63858a..41c59912 100644 --- a/tests/test_buffer.rs +++ b/tests/test_buffer.rs @@ -2,6 +2,24 @@ use serde_json::json; use sql_cli::api_client::{QueryInfo, QueryResponse}; use sql_cli::buffer::{AppMode, Buffer, BufferAPI, SortOrder}; +/// Build a `QueryResponse` with `rows` rows, for tests that need data on screen. +fn make_test_response(rows: usize) -> QueryResponse { + QueryResponse { + data: (0..rows) + .map(|i| json!({"id": i, "name": format!("row{i}")})) + .collect(), + count: rows, + query: QueryInfo { + select: vec!["id".to_string(), "name".to_string()], + where_clause: None, + order_by: None, + }, + source: None, + table: None, + cached: None, + } +} + #[test] fn test_buffer_basic_operations() { let mut buffer = Buffer::new(1); @@ -116,12 +134,25 @@ fn test_buffer_results() { fn test_buffer_navigation() { let mut buffer = Buffer::new(1); - // Test row selection + // The selection is derived from the visible data, so an empty buffer has none + assert_eq!(buffer.get_selected_row(), None); + + // Test row selection against real data + buffer + .set_results_as_datatable(Some(make_test_response(10))) + .unwrap(); + buffer.set_selected_row(Some(5)); assert_eq!(buffer.get_selected_row(), Some(5)); + // Setting None resets the crosshair to the first row rather than clearing the + // selection - with data on screen there is always a selectable row buffer.set_selected_row(None); - assert_eq!(buffer.get_selected_row(), None); + assert_eq!(buffer.get_selected_row(), Some(0)); + + // Out-of-range positions are clamped to the last visible row + buffer.set_selected_row(Some(99)); + assert_eq!(buffer.get_selected_row(), Some(9)); // Test scroll offset buffer.set_scroll_offset((10, 20)); diff --git a/tests/test_buffer_state_refactor.rs b/tests/test_buffer_state_refactor.rs index f41be20e..c1d2b9c9 100644 --- a/tests/test_buffer_state_refactor.rs +++ b/tests/test_buffer_state_refactor.rs @@ -3,6 +3,26 @@ /// using the new proxy-based architecture use sql_cli::app_state_container::AppStateContainer; use sql_cli::buffer::{Buffer, BufferAPI, BufferManager, SelectionMode}; +use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; +use std::sync::Arc; + +/// A `DataTable` with `rows` rows and 8 columns, for tests that need data on screen. +fn make_datatable(rows: usize) -> DataTable { + let mut table = DataTable::new("test"); + for c in 0..8 { + table.add_column(DataColumn::new(format!("col{c}"))); + } + for r in 0..rows { + table + .add_row(DataRow::new( + (0..8) + .map(|c| DataValue::Integer((r * 8 + c) as i64)) + .collect(), + )) + .unwrap(); + } + table +} #[test] #[ignore = "Disabled: test is unreliable due to file system dependencies in CommandHistory::new()"] @@ -176,6 +196,8 @@ fn test_proxy_with_no_buffer() { fn test_direct_buffer_viewstate_access() { // Test that we can also access ViewState directly from Buffer let mut buffer = Buffer::new(1); + // get_selected_row() derives from the visible data, so give the buffer some + buffer.set_datatable(Some(Arc::new(make_datatable(20)))); // Modify ViewState directly buffer.view_state.crosshair_row = 15; diff --git a/tests/test_yank_single_row_repro.rs b/tests/test_yank_single_row_repro.rs new file mode 100644 index 00000000..047af653 --- /dev/null +++ b/tests/test_yank_single_row_repro.rs @@ -0,0 +1,135 @@ +//! Regression tests for yank reporting "No row selected" on a visible cell. +//! +//! The yank handlers resolve their row from `Buffer::get_selected_row()`. That used to +//! be backed by ratatui's `TableState`, which is only written by row navigation and by +//! query execution - so a freshly loaded file, or a filter that leaves a single row +//! (where j/k can never fire), left it as `None` and `yv`/`yy` refused to copy anything. + +use sql_cli::app_state_container::AppStateContainer; +use sql_cli::buffer::{Buffer, BufferAPI}; +use sql_cli::data::data_view::DataView; +use sql_cli::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; +use sql_cli::ui::state::state_coordinator::StateCoordinator; +use sql_cli::ui::viewport_manager::ViewportManager; +use std::cell::RefCell; +use std::sync::Arc; + +fn make_table() -> DataTable { + let mut table = DataTable::new("versions"); + table.add_column(DataColumn::new("project")); + table.add_column(DataColumn::new("version")); + + for (p, v) in [("alpha", "1.0.0"), ("nucleus", "2.3.4"), ("beta", "9.9.9")] { + table + .add_row(DataRow::new(vec![ + DataValue::String(p.to_string()), + DataValue::String(v.to_string()), + ])) + .unwrap(); + } + table +} + +/// Mirrors `EnhancedTuiApp::new_with_dataview` / `add_dataview_with_refs`: +/// a buffer created straight from a loaded file, with no query executed. +fn make_container() -> AppStateContainer { + let table = make_table(); + let mut buffer = Buffer::new(1); + buffer.set_datatable(Some(Arc::new(table.clone()))); + buffer.set_dataview(Some(DataView::new(Arc::new(table)))); + + // Default() rather than new(): it uses CommandHistory::default(), which doesn't + // touch the shared history file and so can't race other tests. + let mut container = AppStateContainer::default(); + container.buffers_mut().add_buffer(buffer); + container.buffers_mut().switch_to(0); + container.update_data_size(3, 2); + container +} + +fn viewport_for(container: &AppStateContainer) -> RefCell> { + RefCell::new(Some(ViewportManager::new(Arc::new( + container.get_buffer_dataview().unwrap().clone(), + )))) +} + +fn selected_row(container: &AppStateContainer) -> Option { + container.current_buffer().unwrap().get_selected_row() +} + +#[test] +fn freshly_loaded_file_has_a_selected_row() { + let container = make_container(); + assert_eq!( + selected_row(&container), + Some(0), + "yank must work immediately after loading a file, without pressing j/k first" + ); +} + +#[test] +fn empty_results_have_no_selected_row() { + let mut container = make_container(); + let vm = viewport_for(&container); + + container.set_fuzzy_filter_pattern("zzzznomatch".to_string()); + let (count, _) = StateCoordinator::apply_fuzzy_filter_with_refs(&mut container, &vm); + + assert_eq!(count, 0); + assert_eq!( + selected_row(&container), + None, + "with no visible rows there is genuinely nothing to yank" + ); +} + +#[test] +fn fuzzy_filter_to_single_row_keeps_a_selected_row() { + let mut container = make_container(); + let vm = viewport_for(&container); + + container.set_fuzzy_filter_pattern("nucleus".to_string()); + let (count, _) = StateCoordinator::apply_fuzzy_filter_with_refs(&mut container, &vm); + + assert_eq!(count, 1); + assert_eq!(selected_row(&container), Some(0)); +} + +#[test] +fn text_filter_to_single_row_keeps_a_selected_row() { + let mut container = make_container(); + let vm = viewport_for(&container); + + let count = StateCoordinator::apply_text_filter_with_refs(&mut container, &vm, "nucleus"); + + assert_eq!(count, 1, "text filter should narrow to the single match"); + assert_eq!( + selected_row(&container), + Some(0), + "the 'f' filter must land on the first match like the fuzzy filter does" + ); +} + +#[test] +fn selection_is_clamped_to_the_filtered_view() { + let mut container = make_container(); + let vm = viewport_for(&container); + + // User navigated down to the last row before filtering. + container.set_selected_row(Some(2)); + + let count = StateCoordinator::apply_text_filter_with_refs(&mut container, &vm, "nucleus"); + assert_eq!(count, 1); + + let row = selected_row(&container).expect("one visible row means one selectable row"); + + // This is the lookup YankManager::yank_cell performs; it must hit the real cell + // rather than running off the end of the narrowed view and copying "NULL". + let view = container.get_buffer_dataview().unwrap(); + assert_eq!(view.row_count(), 1); + assert_eq!( + view.get_cell_value(row, 1), + Some("2.3.4".to_string()), + "yank must read the visible row, not a stale index" + ); +}