From 699989af76117efe8d6abea8bdf2e8d4572eae02 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Fri, 17 Apr 2026 21:00:32 -0300 Subject: [PATCH] fix(tui): return to keymap menu after capture Keep the keymap action menu on the bottom-pane stack while capturing or clearing a shortcut so users return to the previous menu instead of the composer. Refresh the active action menu after keymap updates so the displayed binding and clear state match the saved config. --- codex-rs/tui/src/app/event_dispatch.rs | 4 + codex-rs/tui/src/bottom_pane/mod.rs | 5 ++ codex-rs/tui/src/chatwidget.rs | 20 +++++ codex-rs/tui/src/keymap_setup.rs | 108 +++++++++++++++++++++++-- 4 files changed, 131 insertions(+), 6 deletions(-) diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index cd2c49c574..7cbb3ba762 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -1701,6 +1701,8 @@ impl App { self.keymap = runtime_keymap.clone(); self.chat_widget .apply_keymap_update(keymap_config, &runtime_keymap); + self.chat_widget + .refresh_keymap_action_menu(&context, &action, &runtime_keymap); self.chat_widget.add_info_message( format!("Remapped `{context}.{action}` to `{key}`."), /*hint*/ None, @@ -1747,6 +1749,8 @@ impl App { self.keymap = runtime_keymap.clone(); self.chat_widget .apply_keymap_update(keymap_config, &runtime_keymap); + self.chat_widget + .refresh_keymap_action_menu(&context, &action, &runtime_keymap); self.chat_widget.add_info_message( format!("Removed custom shortcut for `{context}.{action}`."), /*hint*/ None, diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index d67cca4b30..919c963bc1 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -978,6 +978,11 @@ impl BottomPane { !self.view_stack.is_empty() } + #[cfg(test)] + pub(crate) fn active_view_id(&self) -> Option<&'static str> { + self.view_stack.last().and_then(|view| view.view_id()) + } + /// Return true when the pane is in the regular composer state without any /// overlays or popups and not running a task. This is the safe context to /// use Esc-Esc for backtracking from the main view. diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 847a170870..866c8669c1 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -7836,6 +7836,26 @@ impl ChatWidget { self.request_redraw(); } + pub(crate) fn refresh_keymap_action_menu( + &mut self, + context: &str, + action: &str, + runtime_keymap: &RuntimeKeymap, + ) { + let params = keymap_setup::build_keymap_action_menu_params( + context.to_string(), + action.to_string(), + runtime_keymap, + &self.config.tui_keymap, + ); + let replaced = self + .bottom_pane + .replace_selection_view_if_active(keymap_setup::KEYMAP_ACTION_MENU_VIEW_ID, params); + if replaced { + self.request_redraw(); + } + } + fn status_line_context_window_size(&self) -> Option { self.token_info .as_ref() diff --git a/codex-rs/tui/src/keymap_setup.rs b/codex-rs/tui/src/keymap_setup.rs index c42d4a1435..7b852f46b3 100644 --- a/codex-rs/tui/src/keymap_setup.rs +++ b/codex-rs/tui/src/keymap_setup.rs @@ -45,6 +45,7 @@ use details::KeymapActionDetailsRenderable; use details::build_action_details; const KEYMAP_PICKER_VIEW_ID: &str = "keymap-picker"; +pub(crate) const KEYMAP_ACTION_MENU_VIEW_ID: &str = "keymap-action-menu"; const KEYMAP_DETAIL_PANEL_WIDTH: u16 = 46; const KEYMAP_DETAIL_PANEL_MIN_WIDTH: u16 = 40; @@ -86,7 +87,6 @@ pub(crate) fn build_keymap_picker_params( action: action.clone(), }); })], - dismiss_on_select: true, search_value: Some(search_value), ..Default::default() } @@ -150,6 +150,7 @@ pub(crate) fn build_keymap_action_menu_params( let remove_action = action.clone(); SelectionViewParams { + view_id: Some(KEYMAP_ACTION_MENU_VIEW_ID), title: Some("Edit Shortcut".to_string()), subtitle: Some(format!("{label} {context}.{action}")), footer_note: Some(Line::from(vec![ @@ -162,16 +163,15 @@ pub(crate) fn build_keymap_action_menu_params( SelectionItem { name: "Set new key".to_string(), description: Some(format!("Current: {current_binding}")), - selected_description: Some( - "Capture one key and replace this action's custom binding.".to_string(), - ), + selected_description: Some(format!( + "Current: {current_binding}. Capture one key and replace this action's custom binding." + )), actions: vec![Box::new(move |tx| { tx.send(AppEvent::OpenKeymapCapture { context: set_context.clone(), action: set_action.clone(), }); })], - dismiss_on_select: true, ..Default::default() }, SelectionItem { @@ -187,7 +187,6 @@ pub(crate) fn build_keymap_action_menu_params( action: remove_action.clone(), }); })], - dismiss_on_select: true, ..Default::default() }, SelectionItem { @@ -477,10 +476,14 @@ fn key_parts_to_config_key_spec( #[cfg(test)] mod tests { use super::*; + use crate::bottom_pane::BottomPane; + use crate::bottom_pane::BottomPaneParams; use crate::bottom_pane::ListSelectionView; + use crate::tui::FrameRequester; use insta::assert_snapshot; use pretty_assertions::assert_eq; use ratatui::buffer::Buffer; + use tokio::sync::mpsc::UnboundedReceiver; use tokio::sync::mpsc::unbounded_channel; fn app_event_sender() -> AppEventSender { @@ -506,7 +509,11 @@ mod tests { let area = Rect::new(0, 0, width, height); let mut buf = Buffer::empty(area); view.render(area, &mut buf); + render_buffer(&buf) + } + fn render_buffer(buf: &Buffer) -> String { + let area = buf.area(); (0..area.height) .map(|row| { let mut line = String::new(); @@ -524,12 +531,32 @@ mod tests { .join("\n") } + fn test_pane() -> (BottomPane, AppEventSender, UnboundedReceiver) { + let (tx_raw, rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let pane = BottomPane::new(BottomPaneParams { + app_event_tx: tx.clone(), + frame_requester: FrameRequester::test_dummy(), + has_input_focus: true, + enhanced_keys_supported: false, + placeholder_text: "Ask Codex to do anything".to_string(), + disable_paste_burst: false, + animations_enabled: false, + skills: Some(Vec::new()), + }); + (pane, tx, rx) + } + #[test] fn picker_covers_every_replaceable_action() { let runtime = RuntimeKeymap::defaults(); let params = build_keymap_picker_params(&runtime, &TuiKeymap::default()); assert_eq!(params.items.len(), KEYMAP_ACTIONS.len()); + assert!( + params.items.iter().all(|item| !item.dismiss_on_select), + "keymap picker should stay open behind the action menu" + ); assert!(KEYMAP_ACTIONS.iter().all(|descriptor| { binding_slot( &mut TuiKeymap::default(), @@ -644,10 +671,23 @@ mod tests { &TuiKeymap::default(), ); + assert_eq!(params.view_id, Some(KEYMAP_ACTION_MENU_VIEW_ID)); assert_eq!( params.items[1].disabled_reason.as_deref(), Some("There is no custom root binding for this action to remove.") ); + assert!( + !params.items[0].dismiss_on_select, + "set-key should keep the action menu under key capture" + ); + assert!( + !params.items[1].dismiss_on_select, + "clear-key should keep the action menu open for refresh" + ); + assert!( + params.items[2].dismiss_on_select, + "cancel should dismiss the action menu" + ); } #[test] @@ -666,6 +706,62 @@ mod tests { ); } + #[test] + fn capture_completion_returns_to_keymap_action_menu() { + let (mut pane, tx, mut rx) = test_pane(); + let runtime = RuntimeKeymap::defaults(); + pane.show_selection_view(build_keymap_action_menu_params( + "composer".to_string(), + "submit".to_string(), + &runtime, + &TuiKeymap::default(), + )); + + pane.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + let AppEvent::OpenKeymapCapture { context, action } = + rx.try_recv().expect("open capture event") + else { + panic!("expected OpenKeymapCapture event"); + }; + assert_eq!(pane.active_view_id(), Some(KEYMAP_ACTION_MENU_VIEW_ID)); + + pane.show_view(Box::new(build_keymap_capture_view( + context, action, &runtime, tx, + ))); + pane.handle_key_event(KeyEvent::new(KeyCode::Char('K'), KeyModifiers::CONTROL)); + + let AppEvent::KeymapCaptured { + context, + action, + key, + } = rx.try_recv().expect("captured key event") + else { + panic!("expected KeymapCaptured event"); + }; + assert_eq!(context, "composer"); + assert_eq!(action, "submit"); + assert_eq!(key, "ctrl-shift-k"); + assert_eq!(pane.active_view_id(), Some(KEYMAP_ACTION_MENU_VIEW_ID)); + + let keymap = + keymap_with_replacement(&TuiKeymap::default(), &context, &action, &key).unwrap(); + let runtime = RuntimeKeymap::from_config(&keymap).unwrap(); + let params = build_keymap_action_menu_params(context, action, &runtime, &keymap); + assert!( + pane.replace_selection_view_if_active(KEYMAP_ACTION_MENU_VIEW_ID, params), + "active action menu should refresh after a successful assignment" + ); + let area = Rect::new(0, 0, 80, pane.desired_height(/*width*/ 80)); + let mut buf = Buffer::empty(area); + pane.render(area, &mut buf); + let rendered = render_buffer(&buf); + assert!( + rendered.contains("Current: ctrl-shift-k"), + "rendered action menu did not include updated binding:\n{rendered}" + ); + } + #[test] fn key_capture_serializes_modifier_order_for_config() { let event = KeyEvent::new(