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" + ); +}