diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 88da0eb2bc..9e4705bf6b 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -250,7 +250,8 @@ impl ApprovalOverlay { fn patch_selected_index(&self) -> Option { self.patch_state() - .and_then(PatchOverlayState::selected_index) + .map(|_| self.list.scroll_state().selected_idx) + .flatten() } fn patch_note_text(&self) -> String { @@ -260,28 +261,23 @@ impl ApprovalOverlay { } fn patch_layout(&self, width: u16) -> Option { - self.current_request - .as_ref() - .and_then(|request| PatchLayout::new(request, &self.options, self.patch_state(), width)) - } - - fn patch_select_index(&mut self, idx: usize) { - let len = self.options.len(); - let idx = idx.min(len.saturating_sub(1)); - if let Some(state) = self.patch_state_mut() { - state.options_state.selected_idx = Some(idx); - if idx != PATCH_REJECT_OPTION_INDEX { - state.notes_visible = false; - } - state.note_submit_attempted = false; - } + self.current_request.as_ref().and_then(|request| { + PatchLayout::new( + request, + &self.options, + self.list.scroll_state(), + self.patch_state(), + width, + ) + }) } fn open_patch_notes(&mut self) { if self.options.len() <= PATCH_REJECT_OPTION_INDEX { return; } - self.patch_select_index(PATCH_REJECT_OPTION_INDEX); + self.list + .set_selected_visible_index(PATCH_REJECT_OPTION_INDEX); if let Some(state) = self.patch_state_mut() { state.notes_visible = true; state.focus = PatchFocus::Notes; @@ -290,21 +286,6 @@ impl ApprovalOverlay { } } - fn move_patch_selection(&mut self, move_down: bool) { - let options_len = self.options.len(); - if let Some(state) = self.patch_state_mut() { - if move_down { - state.options_state.move_down_wrap(options_len); - } else { - state.options_state.move_up_wrap(options_len); - } - if state.options_state.selected_idx != Some(PATCH_REJECT_OPTION_INDEX) { - state.notes_visible = false; - } - state.note_submit_attempted = false; - } - } - fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) { let Some(request) = self.current_request.as_ref() else { return; @@ -409,160 +390,132 @@ impl ApprovalOverlay { } } - fn try_handle_option_shortcut(&mut self, key_event: &KeyEvent) -> bool { + fn try_handle_shortcut(&mut self, key_event: &KeyEvent) -> bool { + if self.try_handle_global_shortcut(key_event) { + return true; + } + if self + .patch_state() + .is_some_and(PatchOverlayState::focus_is_notes) + { + return false; + } self.options .iter() .position(|opt| { opt.shortcuts() .any(|shortcut| shortcut.is_press(*key_event)) }) - .map(|idx| { - self.apply_selection(idx); - }) + .map(|idx| self.apply_selection(idx)) .is_some() } - fn handle_patch_key_event(&mut self, key_event: KeyEvent) { + fn sync_patch_state_with_list_selection(&mut self) { + let selected_idx = self.patch_selected_index(); + if let Some(state) = self.patch_state_mut() { + if selected_idx != Some(PATCH_REJECT_OPTION_INDEX) { + state.notes_visible = false; + } + state.note_submit_attempted = false; + } + } + + fn handle_patch_notes_key_event(&mut self, key_event: KeyEvent) -> bool { if key_event.kind == KeyEventKind::Release { - return; + return true; } if self.done || self.current_complete { - return; + return true; + } + if !self + .patch_state() + .is_some_and(PatchOverlayState::focus_is_notes) + { + return false; } - if self.try_handle_global_shortcut(&key_event) { - return; + return true; } - match self.patch_state().map(|state| state.focus) { - Some(PatchFocus::Options) => match key_event { - KeyEvent { - code: KeyCode::Char('y'), - modifiers: KeyModifiers::NONE, - .. - } => self.apply_selection(0), - KeyEvent { - code: KeyCode::Char('a'), - modifiers: KeyModifiers::NONE, - .. - } => self.apply_selection(1), - KeyEvent { - code: KeyCode::Char('n'), - modifiers: KeyModifiers::NONE, - .. - } - | KeyEvent { - code: KeyCode::Tab, - modifiers: KeyModifiers::NONE, - .. - } => self.open_patch_notes(), - KeyEvent { - code: KeyCode::Char('1'), - modifiers: KeyModifiers::NONE, - .. - } => self.apply_selection(0), - KeyEvent { - code: KeyCode::Char('2'), - modifiers: KeyModifiers::NONE, - .. - } => self.apply_selection(1), - KeyEvent { - code: KeyCode::Char('3'), - modifiers: KeyModifiers::NONE, - .. - } => self.open_patch_notes(), - KeyEvent { - code: KeyCode::Up | KeyCode::Char('k'), - modifiers: KeyModifiers::NONE, - .. - } => self.move_patch_selection(false), - KeyEvent { - code: KeyCode::Down | KeyCode::Char('j'), - modifiers: KeyModifiers::NONE, - .. - } => self.move_patch_selection(true), - KeyEvent { - code: KeyCode::Enter, - modifiers: KeyModifiers::NONE, - .. - } => match self.patch_selected_index() { - Some(PATCH_REJECT_OPTION_INDEX) => self.open_patch_notes(), - Some(idx) => self.apply_selection(idx), - None => {} - }, - _ => {} - }, - Some(PatchFocus::Notes) => { - if matches!( - key_event, - KeyEvent { - code: KeyCode::Tab, - modifiers: KeyModifiers::NONE, - .. - } - ) { - if let Some(state) = self.patch_state_mut() { - state.focus = PatchFocus::Options; - state.note_submit_attempted = false; - } - return; - } - if matches!( - key_event, - KeyEvent { - code: KeyCode::Enter, - modifiers: KeyModifiers::NONE, - .. - } - ) { - let text = self.patch_note_text(); - if text.trim().is_empty() { - if let Some(state) = self.patch_state_mut() { - state.note_submit_attempted = true; - } - return; - } - if let Some(ApprovalRequest::ApplyPatch { thread_id, id, .. }) = - self.current_request.as_ref() - { - self.app_event_tx - .send(AppEvent::RejectPatchApprovalWithNotes { - thread_id: *thread_id, - approval_id: id.clone(), - text, - }); - self.current_complete = true; - self.advance_queue(); - } - return; - } - if let Some(state) = self.patch_state_mut() { - let _ = state.composer.handle_key_event(key_event); - if !state.composer.current_text_with_pending().trim().is_empty() { - state.note_submit_attempted = false; - } - } + if matches!( + key_event, + KeyEvent { + code: KeyCode::Tab, + modifiers: KeyModifiers::NONE, + .. } - None => {} + ) { + if let Some(state) = self.patch_state_mut() { + state.focus = PatchFocus::Options; + state.note_submit_attempted = false; + } + return true; } + if matches!( + key_event, + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } + ) { + let text = self.patch_note_text(); + if text.trim().is_empty() { + if let Some(state) = self.patch_state_mut() { + state.note_submit_attempted = true; + } + return true; + } + if let Some(ApprovalRequest::ApplyPatch { thread_id, id, .. }) = + self.current_request.as_ref() + { + self.app_event_tx + .send(AppEvent::RejectPatchApprovalWithNotes { + thread_id: *thread_id, + approval_id: id.clone(), + text, + }); + self.current_complete = true; + self.advance_queue(); + } + return true; + } + if let Some(state) = self.patch_state_mut() { + let _ = state.composer.handle_key_event(key_event); + if !state.composer.current_text_with_pending().trim().is_empty() { + state.note_submit_attempted = false; + } + } + true } } impl BottomPaneView for ApprovalOverlay { fn handle_key_event(&mut self, key_event: KeyEvent) { + if self.handle_patch_notes_key_event(key_event) { + return; + } if matches!( self.current_request, Some(ApprovalRequest::ApplyPatch { .. }) + ) && matches!( + key_event, + KeyEvent { + code: KeyCode::Tab, + modifiers: KeyModifiers::NONE, + .. + } ) { - self.handle_patch_key_event(key_event); + self.open_patch_notes(); return; } - if self.try_handle_global_shortcut(&key_event) - || self.try_handle_option_shortcut(&key_event) - { + if self.try_handle_shortcut(&key_event) { return; } self.list.handle_key_event(key_event); + if self.patch_state().is_some() { + self.sync_patch_state_with_list_selection(); + } if let Some(idx) = self.list.take_last_selected_index() { self.apply_selection(idx); } @@ -662,7 +615,7 @@ impl Renderable for ApprovalOverlay { let Some(state) = self.patch_state() else { return self.list.cursor_pos(area); }; - if !state.focus_is_notes() || !state.notes_visible() { + if !state.focus_is_notes() || !state.notes_visible(self.patch_selected_index()) { return self.list.cursor_pos(area); } self.patch_layout(area.width) diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay/patch_ui.rs b/codex-rs/tui/src/bottom_pane/approval_overlay/patch_ui.rs index 01ea706ad3..6aa9a0eba9 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay/patch_ui.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay/patch_ui.rs @@ -40,7 +40,6 @@ pub(super) enum PatchFocus { pub(super) struct PatchOverlayState { pub(super) focus: PatchFocus, - pub(super) options_state: ScrollState, pub(super) composer: ChatComposer, pub(super) notes_visible: bool, pub(super) note_submit_attempted: bool, @@ -59,20 +58,12 @@ impl PatchOverlayState { composer.set_footer_hint_override(Some(Vec::new())); Self { focus: PatchFocus::Options, - options_state: ScrollState { - selected_idx: Some(0), - ..Default::default() - }, composer, notes_visible: false, note_submit_attempted: false, } } - pub(super) fn selected_index(&self) -> Option { - self.options_state.selected_idx - } - pub(super) fn focus_is_notes(&self) -> bool { matches!(self.focus, PatchFocus::Notes) } @@ -81,13 +72,15 @@ impl PatchOverlayState { self.composer.current_text_with_pending() } - pub(super) fn notes_visible(&self) -> bool { - self.options_state.selected_idx == Some(PATCH_REJECT_OPTION_INDEX) + pub(super) fn notes_visible(&self, selected_idx: Option) -> bool { + selected_idx == Some(PATCH_REJECT_OPTION_INDEX) && (self.notes_visible || !self.note_text().trim().is_empty()) } - pub(super) fn note_error_visible(&self) -> bool { - self.notes_visible() && self.note_submit_attempted && self.note_text().trim().is_empty() + pub(super) fn note_error_visible(&self, selected_idx: Option) -> bool { + self.notes_visible(selected_idx) + && self.note_submit_attempted + && self.note_text().trim().is_empty() } fn notes_input_height(&self, width: u16) -> u16 { @@ -114,6 +107,7 @@ impl PatchLayout { pub(super) fn new( request: &ApprovalRequest, options: &[ApprovalOption], + options_state: ScrollState, state: Option<&PatchOverlayState>, width: u16, ) -> Option { @@ -127,16 +121,16 @@ impl PatchLayout { let title_lines = wrap_patch_title(width); let header = build_header(request); let header_height = header.desired_height(width); - let mut options_state = state.map(|state| state.options_state).unwrap_or_default(); + let mut options_state = options_state; if options_state.selected_idx.is_none() { options_state.selected_idx = Some(0); } let rows = patch_option_rows(options, options_state.selected_idx); let options_height = measure_rows_height(&rows, &options_state, rows.len().max(1), width.max(1)); - let hint_lines = patch_hint_lines(request, state, width); - let validation_lines = patch_validation_lines(state, width); - let show_notes = state.is_some_and(PatchOverlayState::notes_visible); + let hint_lines = patch_hint_lines(request, options_state.selected_idx, state, width); + let validation_lines = patch_validation_lines(options_state.selected_idx, state, width); + let show_notes = state.is_some_and(|state| state.notes_visible(options_state.selected_idx)); let notes_height = if show_notes { state .map(|state| state.notes_input_height(width)) @@ -329,16 +323,17 @@ fn patch_option_rows( fn patch_hint_lines( request: &ApprovalRequest, + selected_idx: Option, state: Option<&PatchOverlayState>, width: u16, ) -> Vec> { - let mut hint = if state.is_some_and(PatchOverlayState::notes_visible) { + let mut hint = if state.is_some_and(|state| state.notes_visible(selected_idx)) { if state.is_some_and(PatchOverlayState::focus_is_notes) { "enter to send | tab to go back | esc to interrupt".to_string() } else { "enter or tab to edit follow up | esc to interrupt".to_string() } - } else if state.and_then(PatchOverlayState::selected_index) == Some(PATCH_REJECT_OPTION_INDEX) { + } else if selected_idx == Some(PATCH_REJECT_OPTION_INDEX) { "tab to follow up | esc to interrupt".to_string() } else { "esc to interrupt".to_string() @@ -353,8 +348,12 @@ fn patch_hint_lines( .collect() } -fn patch_validation_lines(state: Option<&PatchOverlayState>, width: u16) -> Vec> { - if !state.is_some_and(PatchOverlayState::note_error_visible) { +fn patch_validation_lines( + selected_idx: Option, + state: Option<&PatchOverlayState>, + width: u16, +) -> Vec> { + if !state.is_some_and(|state| state.note_error_visible(selected_idx)) { return Vec::new(); } diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 9a50a07831..0eee967100 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -476,6 +476,26 @@ impl ListSelectionView { self.last_selected_actual_idx.take() } + pub(crate) fn scroll_state(&self) -> ScrollState { + self.state + } + + pub(crate) fn set_selected_visible_index(&mut self, idx: usize) { + let before = self.selected_actual_idx(); + let len = self.visible_len(); + if len == 0 { + self.state.reset(); + return; + } + self.state.selected_idx = Some(idx.min(len - 1)); + let visible = Self::max_visible_rows(len); + self.state.clamp_selection(len); + self.state.ensure_visible(len, visible); + if self.selected_actual_idx() != before { + self.fire_selection_changed(); + } + } + fn rows_width(total_width: u16) -> u16 { total_width.saturating_sub(2) }