mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Prevent Esc from dismissing or rewinding /side (#22710)
Addresses #22599 ## Why `/side` currently lets `Esc` return to the parent thread. Multiple users reported that this collides with queued-steer UI that also advertises `Esc`, so a timing-sensitive keypress can dismiss an ephemeral side chat instead of sending the queued prompt. After removing that dismissal shortcut, the same `Esc` path could fall through to main-thread backtrack/edit-previous handling, which is not valid for ephemeral side conversations. This keeps `/side` out of both global `Esc` behaviors. ## What changed - Remove `Esc` from the `/side` return shortcut matcher while keeping the existing `Ctrl+C` and `Ctrl+D` behavior. - Update side-conversation hints and blocked-command copy to advertise `Ctrl+C` as the return shortcut. - Rename the reserved `Esc` keymap label to describe backtracking only. - Block backtrack/edit-previous handling while a side conversation is active and report `Editing previous prompts is unavailable in side conversations.` when that path would have fired. - Keep composer-owned `Esc` behavior, such as Vim insert-mode escape, routed locally. - Refresh focused shortcut assertions and TUI snapshots for the updated footer and new side-conversation error message. ## Verification Manually tested `/side` use cases and `Esc`, `Ctrl+C`, `Ctrl+D`.
This commit is contained in:
committed by
GitHub
Unverified
parent
4f2918dd7f
commit
d1235a0a78
@@ -5,6 +5,9 @@
|
||||
|
||||
use super::*;
|
||||
|
||||
const SIDE_EDIT_PREVIOUS_UNAVAILABLE_MESSAGE: &str =
|
||||
"Editing previous prompts is unavailable in side conversations.";
|
||||
|
||||
impl App {
|
||||
pub(super) async fn launch_external_editor(&mut self, tui: &mut tui::Tui) {
|
||||
let editor_cmd = match external_editor::resolve_editor_command() {
|
||||
@@ -197,6 +200,8 @@ impl App {
|
||||
// handles it.
|
||||
if self.should_handle_backtrack_esc(key_event) {
|
||||
self.handle_backtrack_esc_key(tui);
|
||||
} else if self.should_reject_side_backtrack_esc(key_event) {
|
||||
self.reject_side_backtrack_esc();
|
||||
} else {
|
||||
self.chat_widget.handle_key_event(key_event);
|
||||
}
|
||||
@@ -252,11 +257,25 @@ impl App {
|
||||
}
|
||||
|
||||
pub(super) fn should_handle_backtrack_esc(&self, key_event: KeyEvent) -> bool {
|
||||
self.chat_widget.is_normal_backtrack_mode()
|
||||
!self.chat_widget.side_conversation_active()
|
||||
&& self.chat_widget.is_normal_backtrack_mode()
|
||||
&& self.chat_widget.composer_is_empty()
|
||||
&& !self.chat_widget.should_handle_vim_insert_escape(key_event)
|
||||
}
|
||||
|
||||
pub(super) fn should_reject_side_backtrack_esc(&self, key_event: KeyEvent) -> bool {
|
||||
self.chat_widget.side_conversation_active()
|
||||
&& self.chat_widget.is_normal_backtrack_mode()
|
||||
&& self.chat_widget.composer_is_empty()
|
||||
&& !self.chat_widget.should_handle_vim_insert_escape(key_event)
|
||||
}
|
||||
|
||||
pub(super) fn reject_side_backtrack_esc(&mut self) {
|
||||
self.reset_backtrack_state();
|
||||
self.chat_widget
|
||||
.add_error_message(SIDE_EDIT_PREVIOUS_UNAVAILABLE_MESSAGE.to_string());
|
||||
}
|
||||
|
||||
fn app_keymap_shortcuts_available(&self) -> bool {
|
||||
self.overlay.is_none() && self.chat_widget.no_modal_or_popup_active()
|
||||
}
|
||||
|
||||
@@ -54,24 +54,16 @@ fn send_world_writable_scan_failed(tx: &AppEventSender) {
|
||||
}
|
||||
|
||||
pub(super) fn side_return_shortcut_matches(key_event: KeyEvent) -> bool {
|
||||
match key_event {
|
||||
KeyEvent {
|
||||
code: KeyCode::Esc,
|
||||
kind: KeyEventKind::Press | KeyEventKind::Repeat,
|
||||
..
|
||||
} => true,
|
||||
matches!(
|
||||
key_event,
|
||||
KeyEvent {
|
||||
code: KeyCode::Char(c),
|
||||
modifiers,
|
||||
kind: KeyEventKind::Press,
|
||||
..
|
||||
} if modifiers.contains(KeyModifiers::CONTROL)
|
||||
&& (c.eq_ignore_ascii_case(&'c') || c.eq_ignore_ascii_case(&'d')) =>
|
||||
{
|
||||
true
|
||||
}
|
||||
_ => false,
|
||||
}
|
||||
&& (c.eq_ignore_ascii_case(&'c') || c.eq_ignore_ascii_case(&'d'))
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -79,16 +71,7 @@ mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn side_return_shortcuts_match_esc_ctrl_c_and_ctrl_d() {
|
||||
assert!(side_return_shortcut_matches(KeyEvent::new(
|
||||
KeyCode::Esc,
|
||||
KeyModifiers::NONE,
|
||||
)));
|
||||
assert!(side_return_shortcut_matches(KeyEvent::new_with_kind(
|
||||
KeyCode::Esc,
|
||||
KeyModifiers::NONE,
|
||||
KeyEventKind::Repeat,
|
||||
)));
|
||||
fn side_return_shortcuts_match_ctrl_c_and_ctrl_d() {
|
||||
assert!(side_return_shortcut_matches(KeyEvent::new(
|
||||
KeyCode::Char('c'),
|
||||
KeyModifiers::CONTROL,
|
||||
@@ -105,6 +88,11 @@ mod tests {
|
||||
KeyCode::Char('D'),
|
||||
KeyModifiers::CONTROL,
|
||||
)));
|
||||
assert!(!side_return_shortcut_matches(KeyEvent::new_with_kind(
|
||||
KeyCode::Esc,
|
||||
KeyModifiers::NONE,
|
||||
KeyEventKind::Press,
|
||||
)));
|
||||
assert!(!side_return_shortcut_matches(KeyEvent::new_with_kind(
|
||||
KeyCode::Esc,
|
||||
KeyModifiers::NONE,
|
||||
|
||||
@@ -20,7 +20,7 @@ const SIDE_NO_STARTED_CONVERSATION_MESSAGE: &str = concat!(
|
||||
"Send a message first, then try /side again."
|
||||
);
|
||||
const SIDE_ALREADY_OPEN_MESSAGE: &str =
|
||||
"A side conversation is already open. Press Esc to return before starting another.";
|
||||
"A side conversation is already open. Press Ctrl+C to return before starting another.";
|
||||
const SIDE_BOUNDARY_PROMPT: &str = r#"Side conversation boundary.
|
||||
|
||||
Everything before this boundary is inherited history from the parent thread. It is reference context only. It is not your current task.
|
||||
@@ -247,7 +247,7 @@ impl App {
|
||||
if let Some(parent_status) = parent_status {
|
||||
label_parts.push(parent_status.label(parent_is_main).to_string());
|
||||
}
|
||||
label_parts.push("Esc to return".to_string());
|
||||
label_parts.push("Ctrl+C to return".to_string());
|
||||
self.chat_widget
|
||||
.set_side_conversation_context_label(Some(format!("Side {}", label_parts.join(" · "))));
|
||||
}
|
||||
|
||||
@@ -3169,7 +3169,9 @@ async fn side_start_block_message_tracks_open_side_conversation() {
|
||||
|
||||
assert_eq!(
|
||||
app.side_start_block_message(),
|
||||
Some("A side conversation is already open. Press Esc to return before starting another.")
|
||||
Some(
|
||||
"A side conversation is already open. Press Ctrl+C to return before starting another."
|
||||
)
|
||||
);
|
||||
|
||||
app.side_threads.remove(&side_thread_id);
|
||||
@@ -5248,3 +5250,50 @@ async fn backtrack_esc_does_not_steal_empty_vim_insert_escape() {
|
||||
assert!(!app.chat_widget.should_handle_vim_insert_escape(esc));
|
||||
assert!(app.should_handle_backtrack_esc(esc));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn side_conversations_reject_backtrack_esc_without_stealing_vim_insert_escape() {
|
||||
let mut app = make_test_app().await;
|
||||
let esc = crossterm::event::KeyEvent::new(crossterm::event::KeyCode::Esc, KeyModifiers::NONE);
|
||||
|
||||
app.chat_widget
|
||||
.set_side_conversation_active(/*active*/ true);
|
||||
assert!(app.chat_widget.composer_is_empty());
|
||||
assert!(!app.should_handle_backtrack_esc(esc));
|
||||
assert!(app.should_reject_side_backtrack_esc(esc));
|
||||
|
||||
app.chat_widget.toggle_vim_mode_and_notify();
|
||||
app.chat_widget
|
||||
.handle_key_event(crossterm::event::KeyEvent::new(
|
||||
crossterm::event::KeyCode::Char('i'),
|
||||
KeyModifiers::NONE,
|
||||
));
|
||||
|
||||
assert!(app.chat_widget.should_handle_vim_insert_escape(esc));
|
||||
assert!(!app.should_handle_backtrack_esc(esc));
|
||||
assert!(!app.should_reject_side_backtrack_esc(esc));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn side_backtrack_rejection_reports_unavailable_message_snapshot() {
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
app.backtrack.primed = true;
|
||||
|
||||
app.reject_side_backtrack_esc();
|
||||
|
||||
assert!(!app.backtrack.primed);
|
||||
let cell = match app_event_rx.try_recv() {
|
||||
Ok(AppEvent::InsertHistoryCell(cell)) => cell,
|
||||
other => panic!("expected InsertHistoryCell event, got {other:?}"),
|
||||
};
|
||||
let rendered = cell
|
||||
.display_lines(/*width*/ 80)
|
||||
.into_iter()
|
||||
.map(|line| line.to_string())
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
assert_app_snapshot!(
|
||||
"side_backtrack_rejection_reports_unavailable_message",
|
||||
rendered
|
||||
);
|
||||
}
|
||||
|
||||
@@ -25,6 +25,10 @@ impl ChatWidget {
|
||||
self.bottom_pane.set_side_conversation_active(active);
|
||||
}
|
||||
|
||||
pub(crate) fn side_conversation_active(&self) -> bool {
|
||||
self.active_side_conversation
|
||||
}
|
||||
|
||||
pub(crate) fn set_side_conversation_context_label(&mut self, label: Option<String>) {
|
||||
self.bottom_pane.set_side_conversation_context_label(label);
|
||||
}
|
||||
|
||||
@@ -32,7 +32,8 @@ struct PreparedSlashCommandArgs {
|
||||
const SIDE_STARTING_CONTEXT_LABEL: &str = "Side starting...";
|
||||
const SIDE_REVIEW_UNAVAILABLE_MESSAGE: &str =
|
||||
"'/side' is unavailable while code review is running.";
|
||||
const SIDE_SLASH_COMMAND_UNAVAILABLE_HINT: &str = "Press Esc to return to the main thread first.";
|
||||
const SIDE_SLASH_COMMAND_UNAVAILABLE_HINT: &str =
|
||||
"Press Ctrl+C to return to the main thread first.";
|
||||
const GOAL_USAGE: &str = "Usage: /goal <objective>";
|
||||
const GOAL_USAGE_HINT: &str = "Example: /goal improve benchmark coverage";
|
||||
const RAW_USAGE: &str = "Usage: /raw [on|off]";
|
||||
|
||||
+1
-1
@@ -6,4 +6,4 @@ expression: terminal.backend()
|
||||
" "
|
||||
"› Check recently modified functions for compatibility "
|
||||
" "
|
||||
" gpt-5.5 Side from main thread · Esc to return "
|
||||
" gpt-5.5 Side from main thread · Ctrl+C to return "
|
||||
|
||||
+1
-1
@@ -6,4 +6,4 @@ expression: terminal.backend()
|
||||
" "
|
||||
"› Check recently modified functions for compatibility "
|
||||
" "
|
||||
" gpt-5.5 default · … Side from main thread · main needs input · Esc to return "
|
||||
" gpt-5.5 default… Side from main thread · main needs input · Ctrl+C to return "
|
||||
|
||||
@@ -106,7 +106,7 @@ async fn slash_commands_without_side_flag_are_rejected_for_side_threads() {
|
||||
let rendered = lines_to_single_string(&cell.display_lines(/*width*/ 80));
|
||||
assert!(
|
||||
rendered.contains(
|
||||
"'/review' is unavailable in side conversations. Press Esc to return to the main thread first."
|
||||
"'/review' is unavailable in side conversations. Press Ctrl+C to return to the main thread first."
|
||||
),
|
||||
"expected side conversation slash command error, got {rendered:?}"
|
||||
);
|
||||
@@ -132,7 +132,7 @@ async fn slash_side_is_rejected_for_side_threads() {
|
||||
let rendered = lines_to_single_string(&cell.display_lines(/*width*/ 80));
|
||||
assert!(
|
||||
rendered.contains(
|
||||
"'/side' is unavailable in side conversations. Press Esc to return to the main thread first."
|
||||
"'/side' is unavailable in side conversations. Press Ctrl+C to return to the main thread first."
|
||||
),
|
||||
"expected side conversation slash command error, got {rendered:?}"
|
||||
);
|
||||
@@ -276,7 +276,7 @@ async fn side_context_label_preserves_status_line_snapshot() {
|
||||
chat.refresh_status_line();
|
||||
chat.set_side_conversation_active(/*active*/ true);
|
||||
chat.set_side_conversation_context_label(Some(
|
||||
"Side from main thread · Esc to return".to_string(),
|
||||
"Side from main thread · Ctrl+C to return".to_string(),
|
||||
));
|
||||
|
||||
let width = 80;
|
||||
@@ -297,7 +297,7 @@ async fn side_context_label_shows_parent_status_snapshot() {
|
||||
chat.show_welcome_banner = false;
|
||||
chat.set_side_conversation_active(/*active*/ true);
|
||||
chat.set_side_conversation_context_label(Some(
|
||||
"Side from main thread · main needs input · Esc to return".to_string(),
|
||||
"Side from main thread · main needs input · Ctrl+C to return".to_string(),
|
||||
));
|
||||
|
||||
let width = 80;
|
||||
|
||||
@@ -1445,10 +1445,7 @@ const MAIN_RESERVED_BINDINGS: &[(&str, KeyBinding)] = &[
|
||||
"fixed.cycle_collaboration_mode",
|
||||
key_hint::shift(KeyCode::Tab),
|
||||
),
|
||||
(
|
||||
"fixed.return_from_side_or_backtrack",
|
||||
key_hint::plain(KeyCode::Esc),
|
||||
),
|
||||
("fixed.backtrack", key_hint::plain(KeyCode::Esc)),
|
||||
("fixed.previous_agent", key_hint::alt(KeyCode::Left)),
|
||||
("fixed.next_agent", key_hint::alt(KeyCode::Right)),
|
||||
("fixed.slash_command", key_hint::plain(KeyCode::Char('/'))),
|
||||
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
---
|
||||
source: tui/src/app/tests.rs
|
||||
expression: rendered
|
||||
---
|
||||
■ Editing previous prompts is unavailable in side conversations.
|
||||
Reference in New Issue
Block a user