From 153d452418a2c8127d0d4d5c0e14d11db8bd661c Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 30 Aug 2025 09:16:11 +0100 Subject: [PATCH 1/5] feat: Add cancel_search method to StateCoordinator for proper search mode cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Added cancel_search and cancel_search_with_refs methods to StateCoordinator - These methods properly synchronize all state when exiting search mode - Updated search cancellation in TUI to use StateCoordinator - Ensures AppStateContainer, Buffer, and ShadowState stay synchronized - Fixes issue where Escape during search didn't properly restore state This continues the StateCoordinator migration, centralizing complex state synchronization logic and reducing coupling in the TUI. 🤖 Generated with Claude Code Co-Authored-By: Claude --- sql-cli/src/ui/enhanced_tui.rs | 21 +++++-------- sql-cli/src/ui/state_coordinator.rs | 48 ++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/sql-cli/src/ui/enhanced_tui.rs b/sql-cli/src/ui/enhanced_tui.rs index 13812a9e..6992a2e2 100644 --- a/sql-cli/src/ui/enhanced_tui.rs +++ b/sql-cli/src/ui/enhanced_tui.rs @@ -3228,22 +3228,15 @@ impl EnhancedTuiApp { debug!(target: "search", "Cancel: No saved SQL from widget"); } - // Clear all search navigation state (so n/N keys work properly after escape) - self.state_container.clear_search(); + // Clear vim search adapter state self.vim_search_adapter.borrow_mut().cancel_search(); - // Observe search end - self.shadow_state - .borrow_mut() - .observe_search_end("search_cancelled"); - - // Switch back to Results mode - self.state_container.set_mode(AppMode::Results); - - // Observe mode change - self.shadow_state - .borrow_mut() - .observe_mode_change(AppMode::Results, "return_from_search"); + // Use StateCoordinator to properly cancel search and restore state + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::cancel_search_with_refs( + &mut self.state_container, + &self.shadow_state, + ); } SearchModesAction::NextMatch => { debug!(target: "search", "NextMatch action, current_mode={:?}, widget_mode={:?}", diff --git a/sql-cli/src/ui/state_coordinator.rs b/sql-cli/src/ui/state_coordinator.rs index 54094714..1787acb4 100644 --- a/sql-cli/src/ui/state_coordinator.rs +++ b/sql-cli/src/ui/state_coordinator.rs @@ -3,7 +3,6 @@ use std::rc::Rc; use crate::app_state_container::AppStateContainer; use crate::buffer::{AppMode, Buffer, BufferAPI, BufferManager}; -use crate::data::data_view::DataView; use crate::sql::hybrid_parser::HybridParser; use crate::ui::viewport_manager::ViewportManager; use crate::widgets::search_modes_widget::SearchMode; @@ -186,6 +185,53 @@ impl StateCoordinator { trigger.to_string() } + // ========== SEARCH CANCELLATION ========== + + /// Cancel search and properly restore state + /// This handles all the complex state synchronization when Escape is pressed during search + pub fn cancel_search(&mut self) -> (Option, Option) { + debug!("StateCoordinator::cancel_search: Canceling search and restoring state"); + + // Clear search state in state container + self.state_container.clear_search(); + + // Observe search end in shadow state + self.shadow_state + .borrow_mut() + .observe_search_end("search_cancelled"); + + // Switch back to Results mode with proper synchronization + self.sync_mode(AppMode::Results, "search_cancelled"); + + // Return saved SQL and cursor position for restoration + // This would come from search widget's saved state + (None, None) + } + + /// Static version for delegation pattern + pub fn cancel_search_with_refs( + state_container: &mut AppStateContainer, + shadow_state: &RefCell, + ) { + debug!("StateCoordinator::cancel_search_with_refs: Canceling search"); + + // Clear search state + state_container.clear_search(); + + // Observe search end + shadow_state + .borrow_mut() + .observe_search_end("search_cancelled"); + + // Sync back to Results mode + Self::sync_mode_with_refs( + state_container, + shadow_state, + AppMode::Results, + "search_cancelled", + ); + } + // ========== QUERY EXECUTION SYNCHRONIZATION ========== /// Switch to Results mode after successful query execution From 27c4cce91be29ed3750a9de6f7680f1ee266422b Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 30 Aug 2025 09:23:55 +0100 Subject: [PATCH 2/5] fix: Add Default implementation for AppStateContainer to fix test failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Implemented Default trait for AppStateContainer for testing purposes - Uses CommandHistory::default() which handles file system errors gracefully - Updated test_proxy_with_no_buffer to use AppStateContainer::default() - This avoids file system access issues in test environments - All 3 tests in test_buffer_state_refactor now pass 🤖 Generated with Claude Code Co-Authored-By: Claude --- sql-cli/src/app_state_container.rs | 40 +++++++++++++++++++++ sql-cli/tests/test_buffer_state_refactor.rs | 6 ++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/sql-cli/src/app_state_container.rs b/sql-cli/src/app_state_container.rs index d8f666a8..54775035 100644 --- a/sql-cli/src/app_state_container.rs +++ b/sql-cli/src/app_state_container.rs @@ -6267,6 +6267,46 @@ impl AppStateContainer { } } +impl Default for AppStateContainer { + fn default() -> Self { + // Create a minimal AppStateContainer for testing + // Uses CommandHistory::default() which handles errors gracefully + let command_history = CommandHistory::default(); + let mut widgets = WidgetStates::new(); + widgets.set_history(HistoryWidget::new(command_history.clone())); + + Self { + buffers: BufferManager::new(), + current_buffer_id: 0, + command_input: RefCell::new(InputState::new()), + search: RefCell::new(SearchState::new()), + filter: RefCell::new(FilterState::new()), + column_search: RefCell::new(ColumnSearchState::new()), + history_search: RefCell::new(HistorySearchState::new()), + sort: RefCell::new(SortState::new()), + selection: RefCell::new(SelectionState::new()), + completion: RefCell::new(CompletionState::default()), + widgets, + cache_list: CacheListState::new(), + column_stats: ColumnStatsState::new(), + jump_to_row: JumpToRowState::new(), + navigation: RefCell::new(NavigationState::new()), + command_history: RefCell::new(command_history), + key_press_history: RefCell::new(KeyPressHistory::new(100)), + results: RefCell::new(ResultsState::default()), + clipboard: RefCell::new(ClipboardState::default()), + chord: RefCell::new(ChordState::default()), + undo_redo: RefCell::new(UndoRedoState::default()), + scroll: RefCell::new(ScrollState::default()), + results_cache: ResultsCache::new(100), + mode_stack: Vec::new(), + debug_enabled: false, + debug_service: RefCell::new(None), + help: RefCell::new(HelpState::new()), + } + } +} + impl fmt::Debug for AppStateContainer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("AppStateContainer") diff --git a/sql-cli/tests/test_buffer_state_refactor.rs b/sql-cli/tests/test_buffer_state_refactor.rs index dfd442f3..fd02dd6c 100644 --- a/sql-cli/tests/test_buffer_state_refactor.rs +++ b/sql-cli/tests/test_buffer_state_refactor.rs @@ -156,9 +156,9 @@ fn test_buffer_state_preserved_on_switch() { #[test] fn test_proxy_with_no_buffer() { - // Create empty BufferManager - let buffer_manager = BufferManager::new(); - let state = AppStateContainer::new(buffer_manager).unwrap(); + // Create AppStateContainer with empty BufferManager using Default impl + // This avoids file system access issues in tests + let state = AppStateContainer::default(); // Test that proxies return defaults when no buffer exists let nav_proxy = state.navigation_proxy(); From 4edb6cce5809d9db51b9a0d2ae3c57eeb3dac0c0 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 30 Aug 2025 09:29:40 +0100 Subject: [PATCH 3/5] fix: Properly sync mode when applying and canceling vim search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two critical fixes for vim search mode synchronization: 1. When search is applied (Enter pressed), use sync_mode instead of direct set_mode to ensure all state containers stay synchronized 2. When Escape is pressed during vim search navigation: - Properly detect if vim search is navigating - Exit vim navigation mode - Clear search patterns - Use StateCoordinator to sync mode back to Results This fixes the issue where after search + navigation (/, text, Enter, n, n, Escape), the system would be in a confused state with Command mode active but Results mode displayed, causing keypresses to append to SQL instead of navigate. The root cause was that mode changes weren't going through StateCoordinator's sync_mode method, causing AppStateContainer, Buffer, and ShadowState to become desynchronized. 🤖 Generated with Claude Code Co-Authored-By: Claude --- sql-cli/src/ui/enhanced_tui.rs | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/sql-cli/src/ui/enhanced_tui.rs b/sql-cli/src/ui/enhanced_tui.rs index 6992a2e2..ad7be74d 100644 --- a/sql-cli/src/ui/enhanced_tui.rs +++ b/sql-cli/src/ui/enhanced_tui.rs @@ -2585,11 +2585,33 @@ impl EnhancedTuiApp { } fn handle_results_input(&mut self, key: crossterm::event::KeyEvent) -> Result { - // Simple fix: If Escape is pressed and there's an active search, clear it - if key.code == KeyCode::Esc && !self.state_container.get_search_pattern().is_empty() { - info!("Escape pressed in Results mode with active search - clearing search"); + // Check if vim search is active/navigating + let is_vim_navigating = self.vim_search_adapter.borrow().is_navigating(); + let has_search_pattern = !self.state_container.get_search_pattern().is_empty(); + + // If Escape is pressed and there's an active search or vim navigation, handle it properly + if key.code == KeyCode::Esc && (is_vim_navigating || has_search_pattern) { + info!("Escape pressed in Results mode with active search/navigation - properly clearing and syncing"); + + // Clear vim search navigation state + if is_vim_navigating { + self.vim_search_adapter.borrow_mut().exit_navigation(); + } + + // Clear search pattern self.state_container.set_search_pattern(String::new()); self.state_container.clear_search(); + + // Use StateCoordinator to properly sync mode to Results + // This ensures we stay in Results mode and all state is synchronized + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::sync_mode_with_refs( + &mut self.state_container, + &self.shadow_state, + AppMode::Results, + "vim_search_cancelled", + ); + self.state_container .set_status_message("Search cleared".to_string()); return Ok(false); @@ -3155,10 +3177,8 @@ impl EnhancedTuiApp { } // ALWAYS switch back to Results mode after Apply for all search modes - self.state_container.set_mode(AppMode::Results); - self.shadow_state - .borrow_mut() - .observe_mode_change(AppMode::Results, "filter_applied"); + // Use sync_mode to ensure proper synchronization + self.sync_mode(AppMode::Results, "filter_applied"); // Show status message let filter_msg = match mode { From aed7065535dbe598160a7ba95895829f3c682a5b Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 30 Aug 2025 09:41:10 +0100 Subject: [PATCH 4/5] refactor: Move all search state management to StateCoordinator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major improvements to search state centralization: 1. Enhanced cancel_search_with_refs to handle ALL search cleanup: - Clears vim search adapter state - Clears search patterns - Syncs mode properly - Single point of control for search cancellation 2. Added should_handle_search_navigation method: - Centralizes logic for determining if n/N keys should navigate search - Prevents n/N from navigating when search is cleared - Fixes issue where N wouldn't toggle line numbers after search cancel 3. Updated TUI to delegate to StateCoordinator: - Search cancellation now fully handled by StateCoordinator - n/N key behavior determined by StateCoordinator - Reduces coupling and improves consistency This fixes the long-standing issue where after canceling search with Escape, the N key wouldn't revert to toggling line numbers. The problem was that search state wasn't being fully cleared and checks were scattered across multiple components. Future work: Refactor StateCoordinator to own all state rather than using static delegation methods. 🤖 Generated with Claude Code Co-Authored-By: Claude --- sql-cli/src/ui/enhanced_tui.rs | 44 ++++++++++++++--------------- sql-cli/src/ui/state_coordinator.rs | 31 ++++++++++++++++---- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/sql-cli/src/ui/enhanced_tui.rs b/sql-cli/src/ui/enhanced_tui.rs index ad7be74d..5b028b42 100644 --- a/sql-cli/src/ui/enhanced_tui.rs +++ b/sql-cli/src/ui/enhanced_tui.rs @@ -880,15 +880,21 @@ impl EnhancedTuiApp { } NextSearchMatch => { - // Use VimSearchManager for search navigation - self.vim_search_next(); + // n key: navigate to next search match if search is active + // Use StateCoordinator to determine if search navigation should be handled + use crate::ui::state_coordinator::StateCoordinator; + if StateCoordinator::should_handle_search_navigation(&self.state_container) { + self.vim_search_next(); + } else { + debug!("NextSearchMatch: No active search, ignoring 'n' key"); + } Ok(ActionResult::Handled) } PreviousSearchMatch => { // Shift+N behavior: search navigation if search is active, otherwise toggle row numbers - if self.vim_search_adapter.borrow().is_active() - || self.vim_search_adapter.borrow().get_pattern().is_some() - { + // Use StateCoordinator to determine if search navigation should be handled + use crate::ui::state_coordinator::StateCoordinator; + if StateCoordinator::should_handle_search_navigation(&self.state_container) { self.vim_search_previous(); } else { // Delegate to the ToggleRowNumbers action for consistency @@ -2591,25 +2597,18 @@ impl EnhancedTuiApp { // If Escape is pressed and there's an active search or vim navigation, handle it properly if key.code == KeyCode::Esc && (is_vim_navigating || has_search_pattern) { - info!("Escape pressed in Results mode with active search/navigation - properly clearing and syncing"); - - // Clear vim search navigation state - if is_vim_navigating { - self.vim_search_adapter.borrow_mut().exit_navigation(); - } - - // Clear search pattern - self.state_container.set_search_pattern(String::new()); - self.state_container.clear_search(); + info!("Escape pressed in Results mode with active search/navigation - using StateCoordinator"); + debug!( + "Vim navigating: {}, Has search pattern: {}", + is_vim_navigating, has_search_pattern + ); - // Use StateCoordinator to properly sync mode to Results - // This ensures we stay in Results mode and all state is synchronized + // Use StateCoordinator to handle ALL search cancellation logic use crate::ui::state_coordinator::StateCoordinator; - StateCoordinator::sync_mode_with_refs( + StateCoordinator::cancel_search_with_refs( &mut self.state_container, &self.shadow_state, - AppMode::Results, - "vim_search_cancelled", + Some(&self.vim_search_adapter), ); self.state_container @@ -3248,14 +3247,13 @@ impl EnhancedTuiApp { debug!(target: "search", "Cancel: No saved SQL from widget"); } - // Clear vim search adapter state - self.vim_search_adapter.borrow_mut().cancel_search(); - // Use StateCoordinator to properly cancel search and restore state + // StateCoordinator handles clearing vim search adapter too use crate::ui::state_coordinator::StateCoordinator; StateCoordinator::cancel_search_with_refs( &mut self.state_container, &self.shadow_state, + Some(&self.vim_search_adapter), ); } SearchModesAction::NextMatch => { diff --git a/sql-cli/src/ui/state_coordinator.rs b/sql-cli/src/ui/state_coordinator.rs index 1787acb4..9056ca42 100644 --- a/sql-cli/src/ui/state_coordinator.rs +++ b/sql-cli/src/ui/state_coordinator.rs @@ -208,17 +208,25 @@ impl StateCoordinator { (None, None) } - /// Static version for delegation pattern + /// Static version for delegation pattern with vim search adapter pub fn cancel_search_with_refs( state_container: &mut AppStateContainer, shadow_state: &RefCell, + vim_search_adapter: Option<&RefCell>, ) { - debug!("StateCoordinator::cancel_search_with_refs: Canceling search"); + debug!("StateCoordinator::cancel_search_with_refs: Canceling search and clearing all search state"); - // Clear search state + // Clear vim search adapter if provided + if let Some(adapter) = vim_search_adapter { + debug!("Clearing vim search adapter state"); + adapter.borrow_mut().clear(); + } + + // Clear search pattern in state container + state_container.set_search_pattern(String::new()); state_container.clear_search(); - // Observe search end + // Observe search end in shadow state shadow_state .borrow_mut() .observe_search_end("search_cancelled"); @@ -228,10 +236,23 @@ impl StateCoordinator { state_container, shadow_state, AppMode::Results, - "search_cancelled", + "vim_search_cancelled", ); } + /// Check if search navigation should be handled (for n/N keys) + pub fn should_handle_search_navigation(state_container: &AppStateContainer) -> bool { + // Only handle search navigation if there's an active search pattern + let has_search = !state_container.get_search_pattern().is_empty(); + + debug!( + "StateCoordinator::should_handle_search_navigation: has_search={}", + has_search + ); + + has_search + } + // ========== QUERY EXECUTION SYNCHRONIZATION ========== /// Switch to Results mode after successful query execution From 82be73115c00221e8bc016eb890b3f43ef5e05f1 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 30 Aug 2025 10:00:57 +0100 Subject: [PATCH 5/5] fix: Fix vim search Escape key handling via StateCoordinator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix Escape key not properly clearing search state after search completion - VimSearchAdapter was intercepting Escape and not clearing AppStateContainer - Now Escape falls through to handle_results_input for proper StateCoordinator handling - Added complete_search_with_refs() to keep matches active for n/N after Enter - Added cancel_search_with_refs() to completely clear search on Escape - Split should_handle_next_match/previous_match for separate n/N behavior - Enhanced logging to track Escape handling and state transitions After search (Enter): n/N navigate matches After Escape: search fully cleared, N toggles line numbers 🤖 Generated with Claude Code Co-Authored-By: Claude --- sql-cli/src/ui/enhanced_tui.rs | 90 +++++++++++++++++++++------- sql-cli/src/ui/state_coordinator.rs | 90 ++++++++++++++++++++++++++-- sql-cli/src/ui/vim_search_adapter.rs | 26 +++++--- sql-cli/test_search_escape.sh | 23 +++++++ 4 files changed, 192 insertions(+), 37 deletions(-) create mode 100755 sql-cli/test_search_escape.sh diff --git a/sql-cli/src/ui/enhanced_tui.rs b/sql-cli/src/ui/enhanced_tui.rs index 5b028b42..eef351c1 100644 --- a/sql-cli/src/ui/enhanced_tui.rs +++ b/sql-cli/src/ui/enhanced_tui.rs @@ -880,21 +880,27 @@ impl EnhancedTuiApp { } NextSearchMatch => { - // n key: navigate to next search match if search is active + // n key: navigate to next search match only if search is active (not after Escape) // Use StateCoordinator to determine if search navigation should be handled use crate::ui::state_coordinator::StateCoordinator; - if StateCoordinator::should_handle_search_navigation(&self.state_container) { + if StateCoordinator::should_handle_next_match( + &self.state_container, + Some(&self.vim_search_adapter), + ) { self.vim_search_next(); } else { - debug!("NextSearchMatch: No active search, ignoring 'n' key"); + debug!("NextSearchMatch: No active search (or cancelled with Escape), ignoring 'n' key"); } Ok(ActionResult::Handled) } PreviousSearchMatch => { - // Shift+N behavior: search navigation if search is active, otherwise toggle row numbers + // Shift+N behavior: search navigation only if vim search is active, otherwise toggle row numbers // Use StateCoordinator to determine if search navigation should be handled use crate::ui::state_coordinator::StateCoordinator; - if StateCoordinator::should_handle_search_navigation(&self.state_container) { + if StateCoordinator::should_handle_previous_match( + &self.state_container, + Some(&self.vim_search_adapter), + ) { self.vim_search_previous(); } else { // Delegate to the ToggleRowNumbers action for consistency @@ -2591,29 +2597,60 @@ impl EnhancedTuiApp { } fn handle_results_input(&mut self, key: crossterm::event::KeyEvent) -> Result { + // Log all keys in Results mode to debug Escape handling + debug!( + "handle_results_input: Processing key {:?} in Results mode", + key + ); + // Check if vim search is active/navigating let is_vim_navigating = self.vim_search_adapter.borrow().is_navigating(); + let vim_is_active = self.vim_search_adapter.borrow().is_active(); let has_search_pattern = !self.state_container.get_search_pattern().is_empty(); + debug!( + "Search state check: vim_navigating={}, vim_active={}, has_pattern={}, pattern='{}'", + is_vim_navigating, + vim_is_active, + has_search_pattern, + self.state_container.get_search_pattern() + ); + // If Escape is pressed and there's an active search or vim navigation, handle it properly - if key.code == KeyCode::Esc && (is_vim_navigating || has_search_pattern) { - info!("Escape pressed in Results mode with active search/navigation - using StateCoordinator"); - debug!( - "Vim navigating: {}, Has search pattern: {}", - is_vim_navigating, has_search_pattern - ); + if key.code == KeyCode::Esc { + info!("ESCAPE KEY DETECTED in Results mode!"); - // Use StateCoordinator to handle ALL search cancellation logic - use crate::ui::state_coordinator::StateCoordinator; - StateCoordinator::cancel_search_with_refs( - &mut self.state_container, - &self.shadow_state, - Some(&self.vim_search_adapter), - ); + if is_vim_navigating || vim_is_active || has_search_pattern { + info!("Escape pressed with active search - clearing via StateCoordinator"); + debug!( + "Pre-clear state: vim_navigating={}, vim_active={}, pattern='{}'", + is_vim_navigating, + vim_is_active, + self.state_container.get_search_pattern() + ); - self.state_container - .set_status_message("Search cleared".to_string()); - return Ok(false); + // Use StateCoordinator to handle ALL search cancellation logic + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::cancel_search_with_refs( + &mut self.state_container, + &self.shadow_state, + Some(&self.vim_search_adapter), + ); + + // Verify clearing worked + let post_pattern = self.state_container.get_search_pattern(); + let post_vim_active = self.vim_search_adapter.borrow().is_active(); + info!( + "Post-clear state: pattern='{}', vim_active={}", + post_pattern, post_vim_active + ); + + self.state_container + .set_status_message("Search cleared".to_string()); + return Ok(false); + } else { + info!("Escape pressed but no active search to clear"); + } } let selection_mode = self.state_container.get_selection_mode(); @@ -3176,8 +3213,15 @@ impl EnhancedTuiApp { } // ALWAYS switch back to Results mode after Apply for all search modes - // Use sync_mode to ensure proper synchronization - self.sync_mode(AppMode::Results, "filter_applied"); + // Use StateCoordinator to properly complete search (keeps pattern for n/N) + use crate::ui::state_coordinator::StateCoordinator; + StateCoordinator::complete_search_with_refs( + &mut self.state_container, + &self.shadow_state, + Some(&self.vim_search_adapter), + AppMode::Results, + "search_applied", + ); // Show status message let filter_msg = match mode { diff --git a/sql-cli/src/ui/state_coordinator.rs b/sql-cli/src/ui/state_coordinator.rs index 9056ca42..91f728ed 100644 --- a/sql-cli/src/ui/state_coordinator.rs +++ b/sql-cli/src/ui/state_coordinator.rs @@ -240,17 +240,95 @@ impl StateCoordinator { ); } - /// Check if search navigation should be handled (for n/N keys) - pub fn should_handle_search_navigation(state_container: &AppStateContainer) -> bool { - // Only handle search navigation if there's an active search pattern + /// Check if 'n' key should navigate to next search match + /// Returns true only if there's an active search (not cancelled with Escape) + pub fn should_handle_next_match( + state_container: &AppStateContainer, + vim_search_adapter: Option<&RefCell>, + ) -> bool { + // 'n' should only work if there's a search pattern AND it hasn't been cancelled let has_search = !state_container.get_search_pattern().is_empty(); + let pattern = state_container.get_search_pattern(); + + // Check if vim search is active or navigating + // After Escape, this will be false + let vim_active = if let Some(adapter) = vim_search_adapter { + let adapter_ref = adapter.borrow(); + adapter_ref.is_active() || adapter_ref.is_navigating() + } else { + false + }; debug!( - "StateCoordinator::should_handle_search_navigation: has_search={}", - has_search + "StateCoordinator::should_handle_next_match: pattern='{}', vim_active={}, result={}", + pattern, + vim_active, + has_search && vim_active ); - has_search + // Only handle if search exists AND hasn't been cancelled with Escape + has_search && vim_active + } + + /// Check if 'N' key should navigate to previous search match + /// Returns true only if there's an active search (not cancelled with Escape) + pub fn should_handle_previous_match( + state_container: &AppStateContainer, + vim_search_adapter: Option<&RefCell>, + ) -> bool { + // 'N' should only work if there's a search pattern AND it hasn't been cancelled + let has_search = !state_container.get_search_pattern().is_empty(); + let pattern = state_container.get_search_pattern(); + + // Check if vim search is active or navigating + // After Escape, this will be false + let vim_active = if let Some(adapter) = vim_search_adapter { + let adapter_ref = adapter.borrow(); + adapter_ref.is_active() || adapter_ref.is_navigating() + } else { + false + }; + + debug!( + "StateCoordinator::should_handle_previous_match: pattern='{}', vim_active={}, result={}", + pattern, vim_active, has_search && vim_active + ); + + // Only handle if search exists AND hasn't been cancelled with Escape + has_search && vim_active + } + + /// Complete a search operation (after Apply/Enter is pressed) + /// This keeps the pattern for n/N navigation but marks search as complete + pub fn complete_search_with_refs( + state_container: &mut AppStateContainer, + shadow_state: &RefCell, + vim_search_adapter: Option<&RefCell>, + mode: AppMode, + trigger: &str, + ) { + debug!( + "StateCoordinator::complete_search_with_refs: Completing search, switching to {:?}", + mode + ); + + // Note: We intentionally DO NOT clear the search pattern here + // The pattern remains available for n/N navigation + + // Mark vim search adapter as not actively searching + // but keep the matches for navigation + if let Some(adapter) = vim_search_adapter { + debug!("Marking vim search as complete but keeping matches"); + adapter.borrow_mut().mark_search_complete(); + } + + // Observe search completion in shadow state + shadow_state + .borrow_mut() + .observe_search_end("search_completed"); + + // Switch to the target mode (usually Results) + Self::sync_mode_with_refs(state_container, shadow_state, mode, trigger); } // ========== QUERY EXECUTION SYNCHRONIZATION ========== diff --git a/sql-cli/src/ui/vim_search_adapter.rs b/sql-cli/src/ui/vim_search_adapter.rs index 2f69219d..88c27418 100644 --- a/sql-cli/src/ui/vim_search_adapter.rs +++ b/sql-cli/src/ui/vim_search_adapter.rs @@ -83,19 +83,20 @@ impl VimSearchAdapter { } /// Handle a key press through AppStateContainer (simplified interface) - /// Note: This version requires the caller to handle viewport management + /// Returns true if key was handled, false to let it fall through pub fn handle_key(&mut self, key: KeyCode, state: &mut AppStateContainer) -> bool { let mode = state.get_mode(); - // Handle Escape key based on current mode + // Special handling for Escape key if key == KeyCode::Esc { match mode { - AppMode::Results if self.is_active => { - // In Results mode with active search: just exit search, stay in Results - info!("VimSearchAdapter: Exiting search navigation in Results mode"); - state.set_status_message("Search mode exited".to_string()); - self.clear(); - return true; // We handled it + AppMode::Results if self.is_active || self.is_navigating() => { + // In Results mode with active search: Signal that Escape was pressed + // But DON'T handle it here - return false to let StateCoordinator handle it + info!("VimSearchAdapter: Escape detected in Results mode with active search - signaling for StateCoordinator"); + // Return false so it falls through to handle_results_input + // where StateCoordinator will properly clear everything + return false; } AppMode::Search => { // In Search mode: exit to Results @@ -249,6 +250,15 @@ impl VimSearchAdapter { self.manager.exit_navigation(); } + /// Mark search as complete (after Apply/Enter) + /// Keeps matches for n/N navigation and stays active + pub fn mark_search_complete(&mut self) { + info!("VimSearchAdapter: Marking search as complete, keeping matches for navigation"); + // Keep is_active = true so n/N continue to work + // The manager stays active with matches + // Only clear() or cancel_search() will deactivate + } + /// Navigate to next match pub fn next_match( &mut self, diff --git a/sql-cli/test_search_escape.sh b/sql-cli/test_search_escape.sh new file mode 100755 index 00000000..bbe621d4 --- /dev/null +++ b/sql-cli/test_search_escape.sh @@ -0,0 +1,23 @@ +#!/bin/bash +# Test script to verify search and Escape behavior + +echo "Testing search and Escape behavior..." +echo "" +echo "Steps to test:" +echo "1. Load a CSV file" +echo "2. Press '/' to start vim search" +echo "3. Type a search term (e.g., 'unalloc')" +echo "4. Press Enter to apply search" +echo "5. Press 'n' - should navigate to next match" +echo "6. Press 'N' - should toggle line numbers (NOT navigate)" +echo "7. Press Escape - should clear search completely" +echo "8. Press 'n' - should do nothing (no search active)" +echo "9. Press 'N' - should toggle line numbers" +echo "" +echo "Starting sql-cli..." + +# Enable debug logging for search and StateCoordinator +export RUST_LOG=sql_cli::ui::state_coordinator=debug,sql_cli::ui::vim_search_adapter=info,search=debug + +# Run with a sample file +./target/release/sql-cli ../trades_20k.csv \ No newline at end of file