Handle completion separators and popup dismissal (#31191)

## Summary

Autocomplete completion previously inserted a new space even when a
separator was already present, which could leave redundant whitespace
around neighboring text. Popup dismissal also tracked only the query
string, so dismissing one token could suppress a different occurrence
with the same text.

This gives completions one horizontal-separator policy across files,
images, skills, and mentions. Existing separators are reused when
possible, ordinary suffix text remains separated, and line breaks stay
intact. Dismissal now identifies the complete whitespace-delimited token
occurrence, so offset-only edits preserve dismissal while later
identical tokens remain independent. Newly completed values beginning
with `$` or `@` can also remain closed when the next PR's affinity rule
recognizes the token to the left of the cursor.

## Examples

The examples below use `|` to represent the cursor.

### Reuse existing separators

Starting with an existing two-space gap:

```text
@ma|  next
```

After accepting `src/main.rs` and typing `foo`, completion previously
added a third separator and left both original spaces before `next`:

```text
src/main.rs foo|  next
```

After this PR, completion reuses the first existing separator as the
insertion point and preserves the second between the new text and
`next`:

```text
src/main.rs foo| next
```

### Keep a completed sigil-prefixed value closed

Starting before a line break:

```text
@ma|
next
```

and accepting a path whose result is itself prefixed with `@` produces:

```text
@scope/main.rs |
next
```

Without this PR's completion dismissal, the affinity rule in #30463
rediscovers the completed token to the left of the cursor and reopens
its popup:

```text
@scope/main.rs |
^^^^^^^^^^^^^^^ popup reopens
next
```

With this PR, the inserted occurrence is dismissed and the popup remains
closed:

```text
@scope/main.rs |
^^^^^^^^^^^^^^^ popup remains closed
next
```

### Do not dismiss an identical later occurrence

Given two identical tokens:

```text
@scope/main.rs|  @scope/main.rs
```

after dismissing the first popup with Escape and moving to the second
token, query-only dismissal previously suppressed the second popup too:

```text
@scope/main.rs  @scope/main.rs|
                   ^^^^^^^^^^^^^ popup remains closed
```

After this PR, dismissal also matches the token's ordinal among complete
tokens, so the second occurrence opens normally:

```text
@scope/main.rs  @scope/main.rs|
                   ^^^^^^^^^^^^^ popup opens
```

### Keep dismissal across offset-only edits

After dismissing `@ma`, moving to its start, and pasting an email-like
token:

```text
email@ma.com @ma|
```

the `@ma` bytes embedded in `email@ma.com` do not count as another
autocomplete token. The original `@ma` keeps its dismissal even though
its byte range moved.

## Stack

This is PR 2 of 3, stacked on #31190. It relies on the explicit
replacement ranges introduced there and provides the completion
lifecycle used by the targeting fix in #30463.
This commit is contained in:
Charlie Marsh
2026-07-07 18:02:08 -04:00
committed by GitHub
parent 9deb4f9c86
commit 2589f7a52d
2 changed files with 374 additions and 38 deletions

View File

@@ -16,6 +16,21 @@
//! [`ChatComposer::handle_key_event_without_popup`]. After every handled key, we call
//! [`ChatComposer::sync_popups`] so UI state follows the latest buffer/cursor.
//!
//! # Completion and Popup Dismissal
//!
//! Popup selection passes the detected token range directly to the insertion path. After replacing
//! that range, completion leaves the cursor after one horizontal separator. It advances across an
//! existing separator when no suffix would be joined; otherwise it inserts a space, preserving the
//! existing separator before a non-whitespace suffix. It also inserts a space rather than crossing
//! a line break.
//!
//! `Esc` records the active token as dismissed. A completed value that begins with `@` or `$` is
//! also re-dismissed before popup synchronization, because separator affinity can still identify
//! the completed token to the left of the cursor. Synchronization keeps the popup hidden only while
//! the query, complete token text, and ordinal among matching whitespace-delimited tokens remain
//! the same. This preserves dismissal across offset-only edits without suppressing a later
//! identical token.
//!
//! # History Navigation (↑/↓)
//!
//! The Up/Down history path is managed by [`ChatComposerHistory`]. It merges:
@@ -225,6 +240,7 @@ use self::draft_state::DraftState;
use self::footer_state::FooterState;
use self::history_search::HistorySearchSession;
use self::popup_state::ActivePopup;
use self::popup_state::DismissedToken;
use self::popup_state::PopupState;
use self::slash_input::SlashInput;
use self::slash_input::SlashValidation;
@@ -1802,8 +1818,16 @@ impl ChatComposer {
KeyEvent {
code: KeyCode::Esc, ..
} => {
if let Some(tok) = Self::current_at_token(&self.draft.textarea) {
self.popups.dismissed_file_token = Some(tok);
if let Some((range, query)) = Self::current_prefixed_token_range(
&self.draft.textarea,
'@',
/*allow_empty*/ false,
) {
self.popups.dismissed_file_token = Some(DismissedToken::new(
self.draft.textarea.text(),
range,
query,
));
}
self.popups.active = ActivePopup::None;
(InputResult::None, true)
@@ -1879,8 +1903,12 @@ impl ChatComposer {
KeyEvent {
code: KeyCode::Esc, ..
} => {
if let Some(tok) = self.current_mention_token() {
self.popups.dismissed_mention_token = Some(tok);
if let Some((range, query)) = self.current_mention_token_range() {
self.popups.dismissed_mention_token = Some(DismissedToken::new(
self.draft.textarea.text(),
range,
query,
));
}
self.popups.active = ActivePopup::None;
(InputResult::None, true)
@@ -1983,8 +2011,12 @@ impl ChatComposer {
KeyEvent {
code: KeyCode::Esc, ..
} => {
if let Some(tok) = self.current_mentions_v2_token() {
self.popups.dismissed_mention_token = Some(tok);
if let Some((range, query)) = self.current_mentions_v2_token_range() {
self.popups.dismissed_mention_token = Some(DismissedToken::new(
self.draft.textarea.text(),
range,
query,
));
}
self.popups.active = ActivePopup::None;
(InputResult::None, true)
@@ -2044,6 +2076,86 @@ impl ChatComposer {
|| lower.ends_with(".webp")
}
/// Leaves the cursor after one horizontal separator following a completion.
///
/// Another separator is preserved before any non-whitespace suffix so subsequent typing does
/// not merge into it. Line breaks are never reused as separators; a space is inserted before
/// them so subsequent typing stays on the completed token's line.
fn advance_past_completion_separator(&mut self) {
let cursor = self.draft.textarea.cursor();
let existing_separator_len = self.draft.textarea.text()[cursor..]
.chars()
.next()
.filter(|c| {
c.is_whitespace()
&& !matches!(
*c,
'\n' | '\r'
| '\u{000B}'
| '\u{000C}'
| '\u{0085}'
| '\u{2028}'
| '\u{2029}'
)
})
.map(char::len_utf8);
if let Some(separator_len) = existing_separator_len {
let after_separator = cursor + separator_len;
let separator_precedes_suffix = self.draft.textarea.text()[after_separator..]
.chars()
.next()
.is_some_and(|c| !c.is_whitespace());
if separator_precedes_suffix {
self.draft.textarea.insert_str(" ");
} else {
self.draft.textarea.set_cursor(after_separator);
}
} else {
self.draft.textarea.insert_str(" ");
}
}
/// Dismisses popup synchronization only for the exact token occurrence just inserted.
///
/// Matching both range and text prevents an identical token later in the draft from inheriting
/// the completed token's dismissal state.
fn dismiss_completed_prefixed_token(
&mut self,
prefix: char,
inserted_range: Range<usize>,
inserted_text: &str,
) {
let Some(completed_token) = inserted_text.strip_prefix(prefix) else {
return;
};
// Completion leaves the cursor on separator whitespace, where normal token affinity would
// otherwise immediately reopen the popup for sigil-prefixed inserted text.
let Some((current_range, current_token)) = Self::current_prefixed_token_range(
&self.draft.textarea,
prefix,
/*allow_empty*/ true,
) else {
return;
};
if current_range != inserted_range || current_token != completed_token {
return;
}
if prefix == '@' && !self.mentions_v2_enabled {
self.popups.dismissed_file_token = Some(DismissedToken::new(
self.draft.textarea.text(),
current_range,
current_token,
));
} else {
self.popups.dismissed_mention_token = Some(DismissedToken::new(
self.draft.textarea.text(),
current_range,
current_token,
));
}
}
fn insert_selected_file_path(&mut self, token_range: Range<usize>, selected_path: &str) {
if Self::is_image_path(selected_path) {
let path_buf = PathBuf::from(selected_path);
@@ -2054,7 +2166,7 @@ impl ChatComposer {
self.draft.textarea.replace_range(token_range, "");
self.draft.textarea.set_cursor(start_idx);
self.attach_image(path_buf);
self.draft.textarea.insert_str(" ");
self.advance_past_completion_separator();
}
Err(err) => {
tracing::trace!("image dimensions lookup failed: {err}");
@@ -2395,10 +2507,6 @@ impl ChatComposer {
Self::current_prefixed_token_range(&self.draft.textarea, '$', /*allow_empty*/ true)
}
fn current_mention_token(&self) -> Option<String> {
self.current_mention_token_range().map(|(_, token)| token)
}
/// Replace the active `@token` (the one under the cursor) with `path`.
fn insert_selected_path(&mut self, token_range: Range<usize>, path: &str) {
// If the path contains whitespace, wrap it in double quotes so the
@@ -2414,11 +2522,11 @@ impl ChatComposer {
// Replace just the active `@token` so unrelated text elements, such as
// large-paste placeholders, remain atomic and can still expand on submit.
let start_idx = token_range.start;
self.draft
.textarea
.replace_range(token_range, &format!("{inserted} "));
let new_cursor = start_idx.saturating_add(inserted.len()).saturating_add(1);
self.draft.textarea.set_cursor(new_cursor);
self.draft.textarea.replace_range(token_range, &inserted);
let inserted_range = start_idx..start_idx.saturating_add(inserted.len());
self.draft.textarea.set_cursor(inserted_range.end);
self.advance_past_completion_separator();
self.dismiss_completed_prefixed_token('@', inserted_range, &inserted);
}
fn insert_selected_mention(
@@ -2432,6 +2540,7 @@ impl ChatComposer {
self.draft.textarea.replace_range(token_range, "");
self.draft.textarea.set_cursor(start_idx);
let id = self.draft.textarea.insert_element(insert_text);
let inserted_range = start_idx..start_idx.saturating_add(insert_text.len());
if let (Some(path), Some((sigil, mention))) =
(path, Self::mention_token_from_insert_text(insert_text))
@@ -2446,11 +2555,12 @@ impl ChatComposer {
);
}
self.draft.textarea.insert_str(" ");
let new_cursor = start_idx
.saturating_add(insert_text.len())
.saturating_add(1);
self.draft.textarea.set_cursor(new_cursor);
self.advance_past_completion_separator();
if let Some(sigil) = insert_text.chars().next()
&& matches!(sigil, '$' | '@')
{
self.dismiss_completed_prefixed_token(sigil, inserted_range, insert_text);
}
}
fn mention_token_from_insert_text(insert_text: &str) -> Option<(char, String)> {
@@ -3450,11 +3560,11 @@ impl ChatComposer {
self.popups.active = ActivePopup::None;
return;
}
let mentions_v2_token = self.current_mentions_v2_token();
let mentions_v2_token = self.current_mentions_v2_token_range();
let file_token = if self.mentions_v2_enabled {
None
} else {
self.current_editable_at_token()
self.current_editable_at_token_range_with_options(/*allow_empty*/ false)
};
let browsing_history = self
.history
@@ -3470,7 +3580,7 @@ impl ChatComposer {
self.popups.active = ActivePopup::None;
return;
}
let mention_token = self.current_mention_token();
let mention_token = self.current_mention_token_range();
let allow_command_popup = self.slash_commands_enabled()
&& !self.draft.is_bash_mode
@@ -3490,24 +3600,24 @@ impl ChatComposer {
return;
}
if let Some(token) = mentions_v2_token {
self.sync_mentions_v2_popup(token);
if let Some((range, token)) = mentions_v2_token {
self.sync_mentions_v2_popup(range, token);
return;
}
if let Some(token) = mention_token {
if let Some((range, token)) = mention_token {
if self.popups.current_file_query.is_some() {
self.app_event_tx
.send(AppEvent::StartFileSearch(String::new()));
self.popups.current_file_query = None;
}
self.sync_mention_popup(token);
self.sync_mention_popup(range, token);
return;
}
self.popups.dismissed_mention_token = None;
if let Some(token) = file_token {
self.sync_file_search_popup(token);
if let Some((range, token)) = file_token {
self.sync_file_search_popup(range, token);
return;
}
@@ -3581,8 +3691,14 @@ impl ChatComposer {
}
/// Synchronize the legacy file-search popup with the current `@` token.
fn sync_file_search_popup(&mut self, query: String) {
if self.popups.dismissed_file_token.as_ref() == Some(&query) {
fn sync_file_search_popup(&mut self, range: Range<usize>, query: String) {
let text = self.draft.textarea.text();
if self
.popups
.dismissed_file_token
.as_ref()
.is_some_and(|dismissed| dismissed.matches(text, &range, &query))
{
return;
}
@@ -3621,8 +3737,14 @@ impl ChatComposer {
self.popups.dismissed_file_token = None;
}
fn sync_mention_popup(&mut self, query: String) {
if self.popups.dismissed_mention_token.as_ref() == Some(&query) {
fn sync_mention_popup(&mut self, range: Range<usize>, query: String) {
let text = self.draft.textarea.text();
if self
.popups
.dismissed_mention_token
.as_ref()
.is_some_and(|dismissed| dismissed.matches(text, &range, &query))
{
return;
}
@@ -3645,8 +3767,14 @@ impl ChatComposer {
}
}
fn sync_mentions_v2_popup(&mut self, query: String) {
if self.popups.dismissed_mention_token.as_ref() == Some(&query) {
fn sync_mentions_v2_popup(&mut self, range: Range<usize>, query: String) {
let text = self.draft.textarea.text();
if self
.popups
.dismissed_mention_token
.as_ref()
.is_some_and(|dismissed| dismissed.matches(text, &range, &query))
{
return;
}
@@ -4419,8 +4547,24 @@ mod tests {
use crate::bottom_pane::chat_composer::LARGE_PASTE_CHAR_THRESHOLD;
use crate::bottom_pane::textarea::TextArea;
use codex_protocol::models::local_image_label_text;
use tokio::sync::mpsc::UnboundedReceiver;
use tokio::sync::mpsc::unbounded_channel;
fn new_test_composer() -> (ChatComposer, UnboundedReceiver<AppEvent>) {
let (tx, rx) = unbounded_channel::<AppEvent>();
let sender = AppEventSender::new(tx);
(
ChatComposer::new(
/*has_input_focus*/ true,
sender,
/*enhanced_keys_supported*/ false,
"Ask Codex to do anything".to_string(),
/*disable_paste_burst*/ false,
),
rx,
)
}
#[test]
fn footer_hint_row_is_separated_from_composer() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
@@ -8972,6 +9116,156 @@ mod tests {
}
}
fn complete_file(
composer: &mut ChatComposer,
text: &str,
cursor: usize,
query: &str,
selected_path: PathBuf,
) {
composer.set_text_content(text.to_string(), Vec::new(), Vec::new());
composer.draft.textarea.set_cursor(cursor);
composer.sync_popups();
let root = selected_path
.parent()
.filter(|_| selected_path.is_absolute())
.map(std::path::Path::to_path_buf)
.unwrap_or_else(|| PathBuf::from("/tmp"));
composer.on_file_search_result(
query.to_string(),
vec![FileMatch {
score: 1,
path: selected_path,
match_type: codex_file_search::MatchType::File,
root,
indices: None,
}],
);
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE));
}
#[test]
fn file_completion_preserves_separator_before_existing_suffix() {
let (mut composer, _rx) = new_test_composer();
complete_file(
&mut composer,
"@ma next",
/*cursor*/ "@ma".len(),
"ma",
PathBuf::from("src/main.rs"),
);
composer.insert_str("foo");
assert_eq!(composer.current_text(), "src/main.rs foo next");
}
#[test]
fn file_completion_preserves_separator_before_sigiled_suffix() {
for suffix in ["@next", "$next"] {
let (mut composer, _rx) = new_test_composer();
composer.set_text_content(format!("@ma {suffix}"), Vec::new(), Vec::new());
composer.insert_selected_path(0.."@ma".len(), "src/main.rs");
composer.insert_str("foo");
assert_eq!(composer.current_text(), format!("src/main.rs foo {suffix}"));
}
}
#[test]
fn file_completion_inserts_separator_before_line_break() {
let (mut composer, _rx) = new_test_composer();
complete_file(
&mut composer,
"@ma\nnext",
/*cursor*/ "@ma".len(),
"ma",
PathBuf::from("src/main.rs"),
);
composer.insert_str("foo");
assert_eq!(composer.current_text(), "src/main.rs foo\nnext");
}
#[test]
fn file_completion_for_sigil_path_does_not_reopen_popup() {
let (mut composer, _rx) = new_test_composer();
complete_file(
&mut composer,
"@ma\nnext",
/*cursor*/ "@ma".len(),
"ma",
PathBuf::from("@scope/main.rs"),
);
assert_eq!(composer.current_text(), "@scope/main.rs \nnext");
assert!(matches!(composer.popups.active, ActivePopup::None));
let (result, consumed) =
composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
assert!(consumed);
match result {
InputResult::Submitted { text, .. } => assert_eq!(text, "@scope/main.rs \nnext"),
_ => panic!("expected completed path to submit"),
}
}
#[test]
fn file_completion_does_not_dismiss_identical_next_token() {
let (mut composer, _rx) = new_test_composer();
complete_file(
&mut composer,
"@ma @scope/main.rs",
/*cursor*/ "@ma".len(),
"ma",
PathBuf::from("@scope/main.rs"),
);
composer.draft.textarea.set_cursor("@scope/main.rs ".len());
composer.sync_popups();
assert!(matches!(composer.popups.active, ActivePopup::File(_)));
}
#[test]
fn dismissed_file_popup_tracks_token_across_leading_whitespace_edits() {
let (mut composer, _rx) = new_test_composer();
composer.set_text_content("@ma".to_string(), Vec::new(), Vec::new());
composer.draft.textarea.set_cursor("@ma".len());
composer.sync_popups();
assert!(matches!(composer.popups.active, ActivePopup::File(_)));
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
assert!(matches!(composer.popups.active, ActivePopup::None));
composer.draft.textarea.set_cursor(/*pos*/ 0);
composer.insert_str(" ");
assert!(matches!(composer.popups.active, ActivePopup::None));
for _ in 0..3 {
let _ =
composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE));
assert!(matches!(composer.popups.active, ActivePopup::None));
}
}
#[test]
fn dismissed_file_popup_ignores_token_substrings_in_leading_paste() {
let (mut composer, _rx) = new_test_composer();
composer.set_text_content("@ma".to_string(), Vec::new(), Vec::new());
composer.draft.textarea.set_cursor("@ma".len());
composer.sync_popups();
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
assert!(matches!(composer.popups.active, ActivePopup::None));
composer.draft.textarea.set_cursor(/*pos*/ 0);
composer.handle_paste("email@ma.com ".to_string());
assert_eq!(composer.current_text(), "email@ma.com @ma");
assert!(matches!(composer.popups.active, ActivePopup::None));
}
/// Behavior: multiple paste operations can coexist; placeholders should be expanded to their
/// original content on submission.
#[test]

View File

@@ -5,13 +5,55 @@ use crate::bottom_pane::command_popup::CommandPopup;
use crate::bottom_pane::file_search_popup::FileSearchPopup;
use crate::bottom_pane::mentions_v2::MentionV2Popup;
use crate::bottom_pane::skill_popup::SkillPopup;
use std::ops::Range;
/// One token occurrence whose autocomplete popup should remain hidden.
pub(super) struct DismissedToken {
/// Popup query text for the token, excluding its leading sigil.
query: String,
/// Exact token text, including its sigil, captured when the popup was dismissed.
token: String,
/// Zero-based ordinal among identical token strings in the draft at dismissal time.
occurrence: usize,
}
impl DismissedToken {
/// Captures the stable identity of the token at `range`.
pub(super) fn new(text: &str, range: Range<usize>, query: String) -> Self {
let token = text[range.clone()].to_string();
let occurrence = complete_token_occurrences_before(text, &token, range.start);
Self {
query,
token,
occurrence,
}
}
/// Returns whether `range` identifies the same token occurrence in the current draft.
///
/// Byte offsets may shift under offset-only edits, while the token text and its ordinal keep
/// later identical occurrences distinct.
pub(super) fn matches(&self, text: &str, range: &Range<usize>, query: &str) -> bool {
if self.query != query || text.get(range.clone()) != Some(self.token.as_str()) {
return false;
}
complete_token_occurrences_before(text, &self.token, range.start) == self.occurrence
}
}
fn complete_token_occurrences_before(text: &str, token: &str, before: usize) -> usize {
text[..before]
.split_whitespace()
.filter(|candidate| *candidate == token)
.count()
}
#[derive(Default)]
pub(super) struct PopupState {
pub(super) active: ActivePopup,
pub(super) dismissed_file_token: Option<String>,
pub(super) dismissed_file_token: Option<DismissedToken>,
pub(super) current_file_query: Option<String>,
pub(super) dismissed_mention_token: Option<String>,
pub(super) dismissed_mention_token: Option<DismissedToken>,
}
impl PopupState {