From 8daba53808aaa9747f7ed6f9351cf401c13e80c6 Mon Sep 17 00:00:00 2001 From: dedrisian-oai Date: Mon, 22 Sep 2025 11:29:39 -0700 Subject: [PATCH] feat: Add view stack to BottomPane (#4026) Adds a "View Stack" to the bottom pane to allow for pushing/popping bottom panels. `esc` will go back instead of dismissing. Benefit: We retain the "selection state" of a parent panel (e.g. the review panel). --- codex-rs/tui/src/app.rs | 3 - codex-rs/tui/src/app_event.rs | 3 - .../src/bottom_pane/approval_modal_view.rs | 11 +- .../tui/src/bottom_pane/bottom_pane_view.rs | 7 +- .../tui/src/bottom_pane/custom_prompt_view.rs | 19 +--- .../src/bottom_pane/list_selection_view.rs | 13 +-- codex-rs/tui/src/bottom_pane/mod.rs | 100 +++++++++--------- codex-rs/tui/src/chatwidget.rs | 5 - codex-rs/tui/src/chatwidget/tests.rs | 18 +--- 9 files changed, 70 insertions(+), 109 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index e9766cb3..9c30afdb 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -363,9 +363,6 @@ impl App { AppEvent::OpenReviewCustomPrompt => { self.chat_widget.show_review_custom_prompt(); } - AppEvent::OpenReviewPopup => { - self.chat_widget.open_review_popup(); - } } Ok(true) } diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 52e9393d..56c66379 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -76,7 +76,4 @@ pub(crate) enum AppEvent { /// Open the custom prompt option from the review popup. OpenReviewCustomPrompt, - - /// Open the top-level review presets popup. - OpenReviewPopup, } diff --git a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs index e204051d..912df6ce 100644 --- a/codex-rs/tui/src/bottom_pane/approval_modal_view.rs +++ b/codex-rs/tui/src/bottom_pane/approval_modal_view.rs @@ -7,7 +7,6 @@ use crate::app_event_sender::AppEventSender; use crate::user_approval_widget::ApprovalRequest; use crate::user_approval_widget::UserApprovalWidget; -use super::BottomPane; use super::BottomPaneView; use super::CancellationEvent; @@ -42,12 +41,12 @@ impl ApprovalModalView { } impl BottomPaneView for ApprovalModalView { - fn handle_key_event(&mut self, _pane: &mut BottomPane, key_event: KeyEvent) { + fn handle_key_event(&mut self, key_event: KeyEvent) { self.current.handle_key_event(key_event); self.maybe_advance(); } - fn on_ctrl_c(&mut self, _pane: &mut BottomPane) -> CancellationEvent { + fn on_ctrl_c(&mut self) -> CancellationEvent { self.current.on_ctrl_c(); self.queue.clear(); CancellationEvent::Handled @@ -75,6 +74,7 @@ impl BottomPaneView for ApprovalModalView { mod tests { use super::*; use crate::app_event::AppEvent; + use crate::bottom_pane::BottomPane; use tokio::sync::mpsc::unbounded_channel; fn make_exec_request() -> ApprovalRequest { @@ -94,7 +94,8 @@ mod tests { view.enqueue_request(make_exec_request()); let (tx2, _rx2) = unbounded_channel::(); - let mut pane = BottomPane::new(super::super::BottomPaneParams { + // Why do we have this? + let _pane = BottomPane::new(super::super::BottomPaneParams { app_event_tx: AppEventSender::new(tx2), frame_requester: crate::tui::FrameRequester::test_dummy(), has_input_focus: true, @@ -102,7 +103,7 @@ mod tests { placeholder_text: "Ask Codex to do anything".to_string(), disable_paste_burst: false, }); - assert_eq!(CancellationEvent::Handled, view.on_ctrl_c(&mut pane)); + assert_eq!(CancellationEvent::Handled, view.on_ctrl_c()); assert!(view.queue.is_empty()); assert!(view.current.is_complete()); assert!(view.is_complete()); diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index de1beaa2..6bc436f9 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -3,14 +3,13 @@ use crossterm::event::KeyEvent; use ratatui::buffer::Buffer; use ratatui::layout::Rect; -use super::BottomPane; use super::CancellationEvent; /// Trait implemented by every view that can be shown in the bottom pane. pub(crate) trait BottomPaneView { /// Handle a key event while the view is active. A redraw is always /// scheduled after this call. - fn handle_key_event(&mut self, _pane: &mut BottomPane, _key_event: KeyEvent) {} + fn handle_key_event(&mut self, _key_event: KeyEvent) {} /// Return `true` if the view has finished and should be removed. fn is_complete(&self) -> bool { @@ -18,7 +17,7 @@ pub(crate) trait BottomPaneView { } /// Handle Ctrl-C while this view is active. - fn on_ctrl_c(&mut self, _pane: &mut BottomPane) -> CancellationEvent { + fn on_ctrl_c(&mut self) -> CancellationEvent { CancellationEvent::NotHandled } @@ -30,7 +29,7 @@ pub(crate) trait BottomPaneView { /// Optional paste handler. Return true if the view modified its state and /// needs a redraw. - fn handle_paste(&mut self, _pane: &mut BottomPane, _pasted: String) -> bool { + fn handle_paste(&mut self, _pasted: String) -> bool { false } diff --git a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs index b498d878..a8084d0d 100644 --- a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs +++ b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs @@ -13,8 +13,6 @@ use ratatui::widgets::Widget; use std::cell::RefCell; use super::popup_consts::STANDARD_POPUP_HINT_LINE; -use crate::app_event_sender::AppEventSender; -use crate::bottom_pane::SelectionAction; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; @@ -30,8 +28,6 @@ pub(crate) struct CustomPromptView { placeholder: String, context_label: Option, on_submit: PromptSubmitted, - app_event_tx: AppEventSender, - on_escape: Option, // UI state textarea: TextArea, @@ -44,8 +40,6 @@ impl CustomPromptView { title: String, placeholder: String, context_label: Option, - app_event_tx: AppEventSender, - on_escape: Option, on_submit: PromptSubmitted, ) -> Self { Self { @@ -53,8 +47,6 @@ impl CustomPromptView { placeholder, context_label, on_submit, - app_event_tx, - on_escape, textarea: TextArea::new(), textarea_state: RefCell::new(TextAreaState::default()), complete: false, @@ -63,12 +55,12 @@ impl CustomPromptView { } impl BottomPaneView for CustomPromptView { - fn handle_key_event(&mut self, _pane: &mut super::BottomPane, key_event: KeyEvent) { + fn handle_key_event(&mut self, key_event: KeyEvent) { match key_event { KeyEvent { code: KeyCode::Esc, .. } => { - self.on_ctrl_c(_pane); + self.on_ctrl_c(); } KeyEvent { code: KeyCode::Enter, @@ -93,11 +85,8 @@ impl BottomPaneView for CustomPromptView { } } - fn on_ctrl_c(&mut self, _pane: &mut super::BottomPane) -> CancellationEvent { + fn on_ctrl_c(&mut self) -> CancellationEvent { self.complete = true; - if let Some(cb) = &self.on_escape { - cb(&self.app_event_tx); - } CancellationEvent::Handled } @@ -212,7 +201,7 @@ impl BottomPaneView for CustomPromptView { } } - fn handle_paste(&mut self, _pane: &mut super::BottomPane, pasted: String) -> bool { + fn handle_paste(&mut self, pasted: String) -> bool { if pasted.is_empty() { return false; } 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 82466bf7..a856a3c6 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -11,7 +11,6 @@ use ratatui::widgets::Widget; use crate::app_event_sender::AppEventSender; -use super::BottomPane; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; use super::popup_consts::MAX_POPUP_ROWS; @@ -40,7 +39,6 @@ pub(crate) struct SelectionViewParams { pub items: Vec, pub is_searchable: bool, pub search_placeholder: Option, - pub on_escape: Option, } pub(crate) struct ListSelectionView { @@ -51,7 +49,6 @@ pub(crate) struct ListSelectionView { state: ScrollState, complete: bool, app_event_tx: AppEventSender, - on_escape: Option, is_searchable: bool, search_query: String, search_placeholder: Option, @@ -77,7 +74,6 @@ impl ListSelectionView { state: ScrollState::new(), complete: false, app_event_tx, - on_escape: params.on_escape, is_searchable: params.is_searchable, search_query: String::new(), search_placeholder: if params.is_searchable { @@ -221,7 +217,7 @@ impl ListSelectionView { } impl BottomPaneView for ListSelectionView { - fn handle_key_event(&mut self, _pane: &mut BottomPane, key_event: KeyEvent) { + fn handle_key_event(&mut self, key_event: KeyEvent) { match key_event { KeyEvent { code: KeyCode::Up, .. @@ -240,7 +236,7 @@ impl BottomPaneView for ListSelectionView { KeyEvent { code: KeyCode::Esc, .. } => { - self.on_ctrl_c(_pane); + self.on_ctrl_c(); } KeyEvent { code: KeyCode::Char(c), @@ -266,11 +262,8 @@ impl BottomPaneView for ListSelectionView { self.complete } - fn on_ctrl_c(&mut self, _pane: &mut BottomPane) -> CancellationEvent { + fn on_ctrl_c(&mut self) -> CancellationEvent { self.complete = true; - if let Some(cb) = &self.on_escape { - cb(&self.app_event_tx); - } CancellationEvent::Handled } diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 06eff112..523043e6 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -7,6 +7,7 @@ use crate::user_approval_widget::ApprovalRequest; use bottom_pane_view::BottomPaneView; use codex_core::protocol::TokenUsageInfo; use codex_file_search::FileMatch; +use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use ratatui::buffer::Buffer; use ratatui::layout::Constraint; @@ -51,8 +52,8 @@ pub(crate) struct BottomPane { /// input state is retained when the view is closed. composer: ChatComposer, - /// If present, this is displayed instead of the `composer` (e.g. modals). - active_view: Option>, + /// Stack of views displayed instead of the composer (e.g. popups/modals). + view_stack: Vec>, app_event_tx: AppEventSender, frame_requester: FrameRequester, @@ -89,7 +90,7 @@ impl BottomPane { params.placeholder_text, params.disable_paste_burst, ), - active_view: None, + view_stack: Vec::new(), app_event_tx: params.app_event_tx, frame_requester: params.frame_requester, has_input_focus: params.has_input_focus, @@ -101,12 +102,21 @@ impl BottomPane { } } + fn active_view(&self) -> Option<&dyn BottomPaneView> { + self.view_stack.last().map(|view| view.as_ref()) + } + + fn push_view(&mut self, view: Box) { + self.view_stack.push(view); + self.request_redraw(); + } + pub fn desired_height(&self, width: u16) -> u16 { // Always reserve one blank row above the pane for visual spacing. let top_margin = 1; // Base height depends on whether a modal/overlay is active. - let base = match self.active_view.as_ref() { + let base = match self.active_view().as_ref() { Some(view) => view.desired_height(width), None => self.composer.desired_height(width).saturating_add( self.status @@ -133,7 +143,7 @@ impl BottomPane { width: area.width, height: area.height - top_margin - bottom_margin, }; - match self.active_view.as_ref() { + match self.active_view() { Some(_) => [Rect::ZERO, area], None => { let status_height = self @@ -151,7 +161,7 @@ impl BottomPane { // In these states the textarea is not interactable, so we should not // show its caret. let [_, content] = self.layout(area); - if let Some(view) = self.active_view.as_ref() { + if let Some(view) = self.active_view() { view.cursor_pos(content) } else { self.composer.cursor_pos(content) @@ -160,12 +170,20 @@ impl BottomPane { /// Forward a key event to the active view or the composer. pub fn handle_key_event(&mut self, key_event: KeyEvent) -> InputResult { - if let Some(mut view) = self.active_view.take() { - view.handle_key_event(self, key_event); - if !view.is_complete() { - self.active_view = Some(view); - } else { + // If a modal/view is active, handle it here; otherwise forward to composer. + if let Some(view) = self.view_stack.last_mut() { + if key_event.code == KeyCode::Esc + && matches!(view.on_ctrl_c(), CancellationEvent::Handled) + && view.is_complete() + { + self.view_stack.pop(); self.on_active_view_complete(); + } else { + view.handle_key_event(key_event); + if view.is_complete() { + self.view_stack.clear(); + self.on_active_view_complete(); + } } self.request_redraw(); InputResult::None @@ -195,43 +213,31 @@ impl BottomPane { /// Handle Ctrl-C in the bottom pane. If a modal view is active it gets a /// chance to consume the event (e.g. to dismiss itself). pub(crate) fn on_ctrl_c(&mut self) -> CancellationEvent { - let mut view = match self.active_view.take() { - Some(view) => view, - None => { - return if self.composer_is_empty() { - CancellationEvent::NotHandled - } else { - self.set_composer_text(String::new()); - self.show_ctrl_c_quit_hint(); - CancellationEvent::Handled - }; - } - }; - - let event = view.on_ctrl_c(self); - match event { - CancellationEvent::Handled => { - if !view.is_complete() { - self.active_view = Some(view); - } else { + if let Some(view) = self.view_stack.last_mut() { + let event = view.on_ctrl_c(); + if matches!(event, CancellationEvent::Handled) { + if view.is_complete() { + self.view_stack.pop(); self.on_active_view_complete(); } self.show_ctrl_c_quit_hint(); } - CancellationEvent::NotHandled => { - self.active_view = Some(view); - } + event + } else if self.composer_is_empty() { + CancellationEvent::NotHandled + } else { + self.view_stack.pop(); + self.set_composer_text(String::new()); + self.show_ctrl_c_quit_hint(); + CancellationEvent::Handled } - event } pub fn handle_paste(&mut self, pasted: String) { - if let Some(mut view) = self.active_view.take() { - let needs_redraw = view.handle_paste(self, pasted); + if let Some(view) = self.view_stack.last_mut() { + let needs_redraw = view.handle_paste(pasted); if view.is_complete() { self.on_active_view_complete(); - } else { - self.active_view = Some(view); } if needs_redraw { self.request_redraw(); @@ -332,7 +338,7 @@ impl BottomPane { /// Show a generic list selection view with the provided items. pub(crate) fn show_selection_view(&mut self, params: list_selection_view::SelectionViewParams) { let view = list_selection_view::ListSelectionView::new(params, self.app_event_tx.clone()); - self.active_view = Some(Box::new(view)); + self.push_view(Box::new(view)); } /// Update the queued messages shown under the status header. @@ -362,7 +368,7 @@ impl BottomPane { /// overlays or popups and not running a task. This is the safe context to /// use Esc-Esc for backtracking from the main view. pub(crate) fn is_normal_backtrack_mode(&self) -> bool { - !self.is_task_running && self.active_view.is_none() && !self.composer.popup_active() + !self.is_task_running && self.view_stack.is_empty() && !self.composer.popup_active() } /// Update the *context-window remaining* indicator in the composer. This @@ -373,13 +379,12 @@ impl BottomPane { } pub(crate) fn show_view(&mut self, view: Box) { - self.active_view = Some(view); - self.request_redraw(); + self.push_view(view); } /// Called when the agent requests user approval. pub fn push_approval_request(&mut self, request: ApprovalRequest) { - let request = if let Some(view) = self.active_view.as_mut() { + let request = if let Some(view) = self.view_stack.last_mut() { match view.try_consume_approval_request(request) { Some(request) => request, None => { @@ -394,8 +399,7 @@ impl BottomPane { // Otherwise create a new approval modal overlay. let modal = ApprovalModalView::new(request, self.app_event_tx.clone()); self.pause_status_timer_for_modal(); - self.active_view = Some(Box::new(modal)); - self.request_redraw() + self.push_view(Box::new(modal)); } fn on_active_view_complete(&mut self) { @@ -464,7 +468,7 @@ impl BottomPane { height: u32, format_label: &str, ) { - if self.active_view.is_none() { + if self.view_stack.is_empty() { self.composer .attach_image(path, width, height, format_label); self.request_redraw(); @@ -481,7 +485,7 @@ impl WidgetRef for &BottomPane { let [status_area, content] = self.layout(area); // When a modal view is active, it owns the whole content area. - if let Some(view) = &self.active_view { + if let Some(view) = self.active_view() { view.render(content, buf); } else { // No active modal: @@ -591,7 +595,7 @@ mod tests { // After denial, since the task is still running, the status indicator should be // visible above the composer. The modal should be gone. assert!( - pane.active_view.is_none(), + pane.view_stack.is_empty(), "no active modal view after denial" ); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index bbcc3df2..480380b2 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1645,7 +1645,6 @@ impl ChatWidget { title: "Select a review preset".into(), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, - on_escape: None, ..Default::default() }); } @@ -1684,7 +1683,6 @@ impl ChatWidget { items, is_searchable: true, search_placeholder: Some("Type to search branches".to_string()), - on_escape: Some(Box::new(|tx| tx.send(AppEvent::OpenReviewPopup))), ..Default::default() }); } @@ -1726,7 +1724,6 @@ impl ChatWidget { items, is_searchable: true, search_placeholder: Some("Type to search commits".to_string()), - on_escape: Some(Box::new(|tx| tx.send(AppEvent::OpenReviewPopup))), ..Default::default() }); } @@ -1737,8 +1734,6 @@ impl ChatWidget { "Custom review instructions".to_string(), "Type instructions and press Enter".to_string(), None, - self.app_event_tx.clone(), - Some(Box::new(|tx| tx.send(AppEvent::OpenReviewPopup))), Box::new(move |prompt: String| { let trimmed = prompt.trim().to_string(); if trimmed.is_empty() { diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 028d909b..284e20f4 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -851,7 +851,7 @@ fn interrupt_exec_marks_failed_snapshot() { /// parent popup, pressing Esc again dismisses all panels (back to normal mode). #[test] fn review_custom_prompt_escape_navigates_back_then_dismisses() { - let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(); // Open the Review presets parent popup. chat.open_review_popup(); @@ -868,13 +868,6 @@ fn review_custom_prompt_escape_navigates_back_then_dismisses() { // Esc once: child view closes, parent (review presets) remains. chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); - // Process emitted app events to reopen the parent review popup. - while let Ok(ev) = rx.try_recv() { - if let AppEvent::OpenReviewPopup = ev { - chat.open_review_popup(); - break; - } - } let header = render_bottom_first_row(&chat, 60); assert!( header.contains("Select a review preset"), @@ -893,7 +886,7 @@ fn review_custom_prompt_escape_navigates_back_then_dismisses() { /// parent popup, pressing Esc again dismisses all panels (back to normal mode). #[tokio::test(flavor = "current_thread")] async fn review_branch_picker_escape_navigates_back_then_dismisses() { - let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(); // Open the Review presets parent popup. chat.open_review_popup(); @@ -911,13 +904,6 @@ async fn review_branch_picker_escape_navigates_back_then_dismisses() { // Esc once: child view closes, parent remains. chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); - // Process emitted app events to reopen the parent review popup. - while let Ok(ev) = rx.try_recv() { - if let AppEvent::OpenReviewPopup = ev { - chat.open_review_popup(); - break; - } - } let header = render_bottom_first_row(&chat, 60); assert!( header.contains("Select a review preset"),