mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
Harden embedded composer input handling (#42408)
## What changed - Keep `!`, `/`, and `?` prefixes literal in plain-text composers instead of activating command modes or shortcuts. - Preserve buffered characters when changing Vim mode, cancelling a draft, or pressing Enter during a paste burst. - Show over-limit errors in embedded composer footers, retain the rejected draft, and schedule redraws when the message expires. - Clear stale validation errors when replacing a draft or switching form fields. ## Testing - Add coverage for literal prefixes, buffered typing, single-character paste lines, retained over-limit drafts, and clearing validation errors between fields. GitOrigin-RevId: ed9d5b2c087abdfefd92d93033ea1c20211d56d7
This commit is contained in:
@@ -929,6 +929,13 @@ pub async fn run_main(cli: Cli, _codex_linux_sandbox_exe: Option<PathBuf>) -> an
|
||||
needs_redraw: &mut bool|
|
||||
-> anyhow::Result<()> {
|
||||
if *needs_redraw {
|
||||
if let Some(delay) = app
|
||||
.new_task
|
||||
.as_ref()
|
||||
.and_then(|page| page.composer.footer_flash_delay())
|
||||
{
|
||||
let _ = frame_tx.send(Instant::now() + delay);
|
||||
}
|
||||
terminal.draw(|f| ui::draw(f, app))?;
|
||||
*needs_redraw = false;
|
||||
}
|
||||
@@ -941,7 +948,8 @@ pub async fn run_main(cli: Cli, _codex_linux_sandbox_exe: Option<PathBuf>) -> an
|
||||
Some(()) = redraw_rx.recv() => {
|
||||
// Micro‑flush pending first key held by paste‑burst.
|
||||
if let Some(page) = app.new_task.as_mut() {
|
||||
if page.composer.flush_paste_burst_if_due() { needs_redraw = true; }
|
||||
needs_redraw = true;
|
||||
page.composer.flush_paste_burst_if_due();
|
||||
if page.composer.is_in_paste_burst() {
|
||||
let _ = frame_tx
|
||||
.send(Instant::now() + codex_tui::ComposerInput::recommended_flush_delay());
|
||||
|
||||
@@ -10,6 +10,9 @@
|
||||
//! - Turning raw key streams into explicit paste operations on platforms where terminals
|
||||
//! don't provide reliable bracketed paste (notably Windows).
|
||||
//!
|
||||
//! The plain-text preset keeps command prefixes literal, including `!`, so Enter and Tab
|
||||
//! submit ordinary text without enabling shell mode.
|
||||
//!
|
||||
//! # Mention Menus
|
||||
//!
|
||||
//! By default, `@` lists plugins, filesystem entries, and skills. Skills are hidden when their
|
||||
@@ -477,6 +480,8 @@ pub(crate) struct ChatComposerConfig {
|
||||
pub(crate) popups_enabled: bool,
|
||||
/// Whether `/...` input is parsed and dispatched as slash commands.
|
||||
pub(crate) slash_commands_enabled: bool,
|
||||
/// Whether a leading `!` switches the editor into shell mode.
|
||||
pub(crate) shell_commands_enabled: bool,
|
||||
/// Whether pasting a file path can attach local images.
|
||||
pub(crate) image_paste_enabled: bool,
|
||||
}
|
||||
@@ -486,6 +491,7 @@ impl Default for ChatComposerConfig {
|
||||
Self {
|
||||
popups_enabled: true,
|
||||
slash_commands_enabled: true,
|
||||
shell_commands_enabled: true,
|
||||
image_paste_enabled: true,
|
||||
}
|
||||
}
|
||||
@@ -494,12 +500,13 @@ impl Default for ChatComposerConfig {
|
||||
impl ChatComposerConfig {
|
||||
/// A minimal preset for plain-text inputs embedded in other surfaces.
|
||||
///
|
||||
/// This disables popups, slash commands, and image-path attachment behavior
|
||||
/// This disables popups, slash and shell commands, and image-path attachment behavior
|
||||
/// so the composer behaves like a simple notes field.
|
||||
pub(crate) const fn plain_text() -> Self {
|
||||
Self {
|
||||
popups_enabled: false,
|
||||
slash_commands_enabled: false,
|
||||
shell_commands_enabled: false,
|
||||
image_paste_enabled: false,
|
||||
}
|
||||
}
|
||||
@@ -1389,11 +1396,14 @@ impl ChatComposer {
|
||||
|
||||
/// Enable or disable Vim editing for the composer textarea.
|
||||
///
|
||||
/// The composer clears any in-flight paste-burst state when the mode
|
||||
/// The composer flushes buffered typing and clears paste-burst state when the mode
|
||||
/// changes because Vim normal mode treats rapid character sequences as
|
||||
/// commands, not as candidate literal paste text. It also resets transient
|
||||
/// footer mode so the visible hints match the new editing surface.
|
||||
pub(crate) fn set_vim_enabled(&mut self, enabled: bool) {
|
||||
if let Some(pasted) = self.draft.paste_burst.flush_before_modified_input() {
|
||||
self.handle_paste(pasted);
|
||||
}
|
||||
self.draft.textarea.enable_vim_search();
|
||||
self.draft.textarea.set_vim_enabled(enabled);
|
||||
self.vim_history = VimHistory::default();
|
||||
@@ -1404,6 +1414,11 @@ impl ChatComposer {
|
||||
/// Enable Vim while keeping already-active text entry in insert mode.
|
||||
pub(crate) fn enable_vim_in_insert_mode(&mut self) {
|
||||
self.set_vim_enabled(/*enabled*/ true);
|
||||
self.resume_text_entry();
|
||||
}
|
||||
|
||||
/// Resume text entry after a parent view takes focus, preserving Vim undo history.
|
||||
pub(crate) fn resume_text_entry(&mut self) {
|
||||
self.draft.textarea.enter_vim_insert_mode();
|
||||
}
|
||||
|
||||
@@ -1542,11 +1557,6 @@ impl ChatComposer {
|
||||
urls
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn show_footer_flash(&mut self, line: Line<'static>, duration: Duration) {
|
||||
self.footer.show_flash(line, duration);
|
||||
}
|
||||
|
||||
/// Replace the entire composer content with `text` and reset cursor.
|
||||
///
|
||||
/// This is the "fresh draft" path: it clears pending paste payloads and
|
||||
@@ -1587,6 +1597,7 @@ impl ChatComposer {
|
||||
mention_bindings: Vec<MentionBinding>,
|
||||
) {
|
||||
// Clear any existing content, placeholders, and attachments first.
|
||||
self.footer.flash = None;
|
||||
self.vim_history = VimHistory::default();
|
||||
self.draft.textarea.set_text_clearing_elements("");
|
||||
self.draft.is_bash_mode = false;
|
||||
@@ -1729,7 +1740,9 @@ impl ChatComposer {
|
||||
text: String,
|
||||
text_elements: Vec<TextElement>,
|
||||
) -> (String, Vec<TextElement>) {
|
||||
if let Some(stripped) = text.strip_prefix('!') {
|
||||
if self.config.shell_commands_enabled
|
||||
&& let Some(stripped) = text.strip_prefix('!')
|
||||
{
|
||||
self.draft.is_bash_mode = true;
|
||||
(
|
||||
stripped.to_string(),
|
||||
@@ -1744,7 +1757,11 @@ impl ChatComposer {
|
||||
}
|
||||
}
|
||||
|
||||
/// Flush buffered typing before clearing so cancellation preserves the complete draft in history.
|
||||
pub(crate) fn clear_for_ctrl_c(&mut self) -> Option<String> {
|
||||
if let Some(pasted) = self.draft.paste_burst.flush_before_modified_input() {
|
||||
self.handle_paste(pasted);
|
||||
}
|
||||
if self.is_empty() {
|
||||
return None;
|
||||
}
|
||||
@@ -3115,6 +3132,16 @@ impl ChatComposer {
|
||||
.pending_pastes
|
||||
.clone_from(&original_pending_pastes);
|
||||
self.draft.textarea.set_cursor(original_input.len());
|
||||
// Embedded composers may cover the transcript that receives the full error.
|
||||
if self.footer.hint_override.is_some() {
|
||||
self.show_footer_flash(
|
||||
Line::from(
|
||||
format!("Message too long; limit {MAX_USER_INPUT_TEXT_CHARS} characters")
|
||||
.red(),
|
||||
),
|
||||
Duration::from_secs(5),
|
||||
);
|
||||
}
|
||||
return None;
|
||||
}
|
||||
self.attachments
|
||||
@@ -3301,7 +3328,8 @@ impl ChatComposer {
|
||||
)
|
||||
}
|
||||
} else {
|
||||
// Restore text if submission was suppressed.
|
||||
// Restore suppressed input, preserving validation feedback.
|
||||
let flash = self.footer.flash.take();
|
||||
self.set_text_content_with_mention_bindings(
|
||||
original_input,
|
||||
original_text_elements,
|
||||
@@ -3309,6 +3337,7 @@ impl ChatComposer {
|
||||
original_mention_bindings,
|
||||
);
|
||||
self.draft.pending_pastes = original_pending_pastes;
|
||||
self.footer.flash = flash;
|
||||
(InputResult::None, true)
|
||||
}
|
||||
}
|
||||
@@ -3505,7 +3534,8 @@ impl ChatComposer {
|
||||
{
|
||||
return self.handle_input_basic(key_event);
|
||||
}
|
||||
if self.draft.textarea.is_vim_normal_mode()
|
||||
if self.config.shell_commands_enabled
|
||||
&& self.draft.textarea.is_vim_normal_mode()
|
||||
&& self.is_empty()
|
||||
&& matches!(
|
||||
key_event,
|
||||
@@ -3592,7 +3622,7 @@ impl ChatComposer {
|
||||
}
|
||||
|
||||
fn is_bang_shell_command(&self) -> bool {
|
||||
self.current_text().trim_start().starts_with('!')
|
||||
self.config.shell_commands_enabled && self.current_text().trim_start().starts_with('!')
|
||||
}
|
||||
|
||||
fn shell_mode_footer_line(&self) -> Option<Line<'static>> {
|
||||
@@ -3801,7 +3831,10 @@ impl ChatComposer {
|
||||
}
|
||||
|
||||
fn sync_bash_mode_from_text(&mut self) {
|
||||
if !self.draft.is_bash_mode && self.draft.textarea.text().starts_with('!') {
|
||||
if self.config.shell_commands_enabled
|
||||
&& !self.draft.is_bash_mode
|
||||
&& self.draft.textarea.text().starts_with('!')
|
||||
{
|
||||
self.draft.textarea.replace_range(0..1, "");
|
||||
self.draft.is_bash_mode = true;
|
||||
}
|
||||
@@ -3839,7 +3872,8 @@ impl ChatComposer {
|
||||
return true;
|
||||
}
|
||||
|
||||
let toggles = self.toggle_shortcuts_keys.is_pressed(*key_event)
|
||||
let toggles = self.footer.hint_override.is_none()
|
||||
&& self.toggle_shortcuts_keys.is_pressed(*key_event)
|
||||
&& self.is_empty()
|
||||
&& !self.is_in_paste_burst();
|
||||
|
||||
@@ -5038,6 +5072,10 @@ mod agents_navigation_tests;
|
||||
#[path = "chat_composer_effort_tests.rs"]
|
||||
mod effort_tests;
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "chat_composer/embedded_input_tests.rs"]
|
||||
mod embedded_input_tests;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::attachment_state::AttachedImage;
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
//! Embedded composers preserve literal drafts and show validation feedback in place.
|
||||
|
||||
use super::tests::new_test_composer;
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn plain_text_composer_keeps_command_prefixes_literal() {
|
||||
let (mut composer, _rx) = new_test_composer();
|
||||
composer.config = ChatComposerConfig::plain_text();
|
||||
composer.set_disable_paste_burst(/*disabled*/ true);
|
||||
composer.set_footer_hint_override(Some(Vec::new()));
|
||||
for (prefix, modifiers) in [
|
||||
('!', KeyModifiers::NONE),
|
||||
('/', KeyModifiers::NONE),
|
||||
('?', KeyModifiers::NONE),
|
||||
('?', KeyModifiers::SHIFT),
|
||||
] {
|
||||
composer.handle_key_event(KeyEvent::new(KeyCode::Char(prefix), modifiers));
|
||||
composer.handle_paste("keep this literal".into());
|
||||
assert_eq!(
|
||||
composer.handle_key_event(KeyCode::Tab.into()).0,
|
||||
InputResult::Submitted {
|
||||
text: format!("{prefix}keep this literal"),
|
||||
text_elements: Vec::new(),
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn buffered_typing_survives_vim_changes_and_cancellation() {
|
||||
let (mut composer, _rx) = new_test_composer();
|
||||
composer.handle_key_event(KeyCode::Char('x').into());
|
||||
composer.set_vim_enabled(/*enabled*/ true);
|
||||
assert_eq!(composer.current_text(), "x");
|
||||
composer.set_vim_enabled(/*enabled*/ false);
|
||||
composer.handle_key_event(KeyCode::Char('y').into());
|
||||
assert_eq!(composer.clear_for_ctrl_c(), Some("xy".into()));
|
||||
assert!(!composer.is_in_paste_burst());
|
||||
composer.handle_key_event(KeyCode::Up.into());
|
||||
assert_eq!(composer.current_text(), "xy");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn paste_enter_retains_a_one_character_first_line() {
|
||||
let (mut composer, _rx) = new_test_composer();
|
||||
composer.handle_key_event(KeyCode::Char('x').into());
|
||||
assert!(composer.handle_paste_enter(Instant::now()));
|
||||
assert_eq!(composer.clear_for_ctrl_c(), Some("x\n".into()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn embedded_composer_shows_validation_error_without_losing_draft() {
|
||||
let (mut composer, mut rx) = new_test_composer();
|
||||
composer.config = ChatComposerConfig::plain_text();
|
||||
composer.set_footer_hint_override(Some(vec![("enter".into(), "submit".into())]));
|
||||
composer.set_context_window_pending(/*pending*/ true);
|
||||
let text = "x".repeat(MAX_USER_INPUT_TEXT_CHARS + 1);
|
||||
composer.handle_paste(text.clone());
|
||||
assert_eq!(
|
||||
composer.handle_key_event(KeyCode::Enter.into()).0,
|
||||
InputResult::None
|
||||
);
|
||||
assert_eq!(composer.current_text_with_pending(), text);
|
||||
assert!(matches!(rx.try_recv(), Ok(AppEvent::InsertHistoryCell(_))));
|
||||
let area = Rect::new(
|
||||
/*x*/ 0, /*y*/ 0, /*width*/ 80, /*height*/ 5,
|
||||
);
|
||||
let mut buffer = Buffer::empty(area);
|
||||
composer.render(area, &mut buffer);
|
||||
let footer = (0..area.width)
|
||||
.map(|x| buffer[(x, area.bottom() - 1)].symbol())
|
||||
.collect::<String>();
|
||||
insta::assert_snapshot!(footer.trim_end(), @" Message too long; limit 1048576 characters");
|
||||
assert!(composer.footer_flash_delay().is_some());
|
||||
composer.footer.flash.as_mut().unwrap().expires_at = Instant::now() - Duration::from_secs(1);
|
||||
assert_eq!(composer.footer_flash_delay(), None);
|
||||
}
|
||||
@@ -1,4 +1,5 @@
|
||||
//! Footer and status-row presentation state for the chat composer.
|
||||
//! Owners schedule flash expiry redraws; replacing a draft clears its flash.
|
||||
|
||||
use std::time::Instant;
|
||||
|
||||
@@ -9,9 +10,21 @@ use crate::bottom_pane::footer::FooterMode;
|
||||
use crate::bottom_pane::footer::GoalStatusIndicator;
|
||||
use crate::key_hint::KeyBinding;
|
||||
use crate::key_hint::ShortcutHint;
|
||||
#[cfg(test)]
|
||||
use std::time::Duration;
|
||||
|
||||
impl super::ChatComposer {
|
||||
pub(crate) fn footer_flash_delay(&self) -> Option<Duration> {
|
||||
self.footer
|
||||
.flash
|
||||
.as_ref()
|
||||
.and_then(|flash| flash.expires_at.checked_duration_since(Instant::now()))
|
||||
}
|
||||
|
||||
pub(crate) fn show_footer_flash(&mut self, line: Line<'static>, duration: Duration) {
|
||||
self.footer.show_flash(line, duration);
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) struct FooterState {
|
||||
pub(super) quit_shortcut_expires_at: Option<Instant>,
|
||||
pub(super) quit_shortcut_key: KeyBinding,
|
||||
@@ -54,7 +67,6 @@ impl FooterState {
|
||||
.is_some_and(|flash| Instant::now() < flash.expires_at)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(super) fn show_flash(&mut self, line: Line<'static>, duration: Duration) {
|
||||
let expires_at = Instant::now()
|
||||
.checked_add(duration)
|
||||
|
||||
@@ -1513,6 +1513,10 @@ impl Renderable for McpServerElicitationOverlay {
|
||||
}
|
||||
|
||||
impl BottomPaneView for McpServerElicitationOverlay {
|
||||
fn next_frame_delay(&self) -> Option<std::time::Duration> {
|
||||
self.composer.footer_flash_delay()
|
||||
}
|
||||
|
||||
fn keymap_contexts(&self) -> crate::keymap::KeymapContextSet {
|
||||
if self.current_field_is_select() {
|
||||
crate::keymap::KeymapContextSet::new(crate::keymap::KeymapContext::List)
|
||||
@@ -2265,6 +2269,34 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn switching_fields_clears_length_validation_flash() {
|
||||
let (tx, _rx) = test_sender();
|
||||
let request = from_form_request(
|
||||
ThreadId::default(),
|
||||
form_request(
|
||||
"Two fields",
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {"a": {"type": "string"}, "b": {"type": "string"}},
|
||||
"required": ["a", "b"],
|
||||
}),
|
||||
/*meta*/ None,
|
||||
),
|
||||
)
|
||||
.expect("supported form");
|
||||
let mut overlay = McpServerElicitationOverlay::new(
|
||||
request, tx, /*has_input_focus*/ true, /*enhanced_keys_supported*/ false,
|
||||
/*disable_paste_burst*/ true,
|
||||
);
|
||||
overlay.handle_paste("x".repeat(codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS + 1));
|
||||
overlay.handle_key_event(KeyCode::Enter.into());
|
||||
assert!(overlay.next_frame_delay().is_some());
|
||||
overlay.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::CONTROL));
|
||||
assert_eq!(overlay.next_frame_delay(), None);
|
||||
assert!(overlay.composer.current_text().is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn text_fields_inherit_composer_chord_context_and_submit_binding() {
|
||||
let (tx, _rx) = test_sender();
|
||||
|
||||
@@ -873,10 +873,10 @@ impl BottomPane {
|
||||
}
|
||||
|
||||
fn schedule_active_view_frame(&self) {
|
||||
if let Some(delay) = self
|
||||
.active_view()
|
||||
.and_then(BottomPaneView::next_frame_delay)
|
||||
{
|
||||
if let Some(delay) = self.active_view().map_or_else(
|
||||
|| self.composer.footer_flash_delay(),
|
||||
BottomPaneView::next_frame_delay,
|
||||
) {
|
||||
self.request_redraw_in(delay);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,6 +26,8 @@
|
||||
//! [`PasteBurst::on_plain_char_no_hold`] (non-ASCII/IME).
|
||||
//! - If the decision indicates buffering, the caller appends to `PasteBurst.buffer` via
|
||||
//! [`PasteBurst::append_char_to_buffer`].
|
||||
//! - On Enter, [`PasteBurst::append_newline_if_active`] promotes a held first character into
|
||||
//! the active buffer before appending the newline.
|
||||
//! - On a UI tick, call [`PasteBurst::flush_if_due`]. If it returns [`FlushResult::Typed`], insert
|
||||
//! that char as normal typing. If it returns [`FlushResult::Paste`], treat the returned string as
|
||||
//! an explicit paste.
|
||||
@@ -47,8 +49,8 @@
|
||||
//! - `buffer`: accumulated burst text that will eventually flush as a single `Paste(String)`.
|
||||
//! A non-empty buffer is treated as "in burst context" even if `active` has been cleared.
|
||||
//! - `pending_first_char`: a single held ASCII char used for flicker suppression. The caller must
|
||||
//! not render this char until it either becomes part of a burst (`BeginBufferFromPending`) or
|
||||
//! flushes as a normal typed char (`FlushResult::Typed`).
|
||||
//! not render this char until it joins a burst through `BeginBufferFromPending` or
|
||||
//! `append_newline_if_active`, or flushes as a normal typed char (`FlushResult::Typed`).
|
||||
//! - `last_plain_char_time`/`consecutive_plain_char_burst`: the timing/count heuristic for
|
||||
//! "paste-like" streams.
|
||||
//! - `burst_window_until`: the Enter suppression window ("Enter inserts newline") that outlives the
|
||||
@@ -326,6 +328,10 @@ impl PasteBurst {
|
||||
/// false otherwise.
|
||||
pub fn append_newline_if_active(&mut self, now: Instant) -> bool {
|
||||
if self.is_active() {
|
||||
if let Some((held, _)) = self.pending_first_char.take() {
|
||||
self.buffer.push(held);
|
||||
}
|
||||
self.active = true;
|
||||
self.buffer.push('\n');
|
||||
self.burst_window_until = Some(now + PASTE_ENTER_SUPPRESS_WINDOW);
|
||||
true
|
||||
|
||||
@@ -1523,6 +1523,9 @@ impl BottomPaneView for RequestUserInputOverlay {
|
||||
|
||||
fn next_frame_delay(&self) -> Option<Duration> {
|
||||
self.auto_resolution_next_frame_delay_at(Instant::now())
|
||||
.into_iter()
|
||||
.chain(self.composer.footer_flash_delay())
|
||||
.min()
|
||||
}
|
||||
|
||||
fn try_consume_user_input_request(
|
||||
|
||||
@@ -118,6 +118,11 @@ impl ComposerInput {
|
||||
self.inner.is_in_paste_burst()
|
||||
}
|
||||
|
||||
/// Delay until validation feedback expires and needs a redraw.
|
||||
pub fn footer_flash_delay(&self) -> Option<Duration> {
|
||||
self.inner.footer_flash_delay()
|
||||
}
|
||||
|
||||
/// Flush a pending paste-burst if the inter-key timeout has elapsed.
|
||||
/// Returns true if text changed and a redraw is warranted.
|
||||
pub fn flush_paste_burst_if_due(&mut self) -> bool {
|
||||
|
||||
Reference in New Issue
Block a user