diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index b07e839453..bd7b963e2b 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -18,18 +18,20 @@ //! //! # 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. +//! Popup targeting resolves an editable token range around the cursor and treats atomic text +//! elements as hard boundaries. When that range begins immediately after an atomic element, the +//! insertion path first adds a horizontal separator and shifts the range so the completion cannot +//! merge with the atomic element. After replacing the 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. +//! the query, complete token text, and ordinal among matching occurrences remain the same. +//! Whitespace and atomic-element edges delimit occurrences, while nested sigils do not. This +//! preserves dismissal across offset-only edits without suppressing a later identical token. //! //! # History Navigation (↑/↓) //! @@ -229,6 +231,7 @@ use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS; use codex_protocol::user_input::TextElement; mod attachment_state; +mod completion_target; mod draft_state; mod footer_state; mod history_search; @@ -1820,16 +1823,13 @@ impl ChatComposer { KeyEvent { code: KeyCode::Esc, .. } => { - if let Some((range, query)) = Self::current_prefixed_token_range( + if let Some((range, query)) = completion_target::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.dismissed_file_token = + Some(DismissedToken::new(&self.draft.textarea, range, query)); } self.popups.active = ActivePopup::None; (InputResult::None, true) @@ -1906,11 +1906,8 @@ impl ChatComposer { code: KeyCode::Esc, .. } => { 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.dismissed_mention_token = + Some(DismissedToken::new(&self.draft.textarea, range, query)); } self.popups.active = ActivePopup::None; (InputResult::None, true) @@ -2014,11 +2011,8 @@ impl ChatComposer { code: KeyCode::Esc, .. } => { 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.dismissed_mention_token = + Some(DismissedToken::new(&self.draft.textarea, range, query)); } self.popups.active = ActivePopup::None; (InputResult::None, true) @@ -2132,7 +2126,7 @@ impl ChatComposer { }; // 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( + let Some((current_range, current_token)) = completion_target::current_prefixed_token_range( &self.draft.textarea, prefix, /*allow_empty*/ true, @@ -2145,13 +2139,13 @@ impl ChatComposer { if prefix == '@' && !self.mentions_v2_enabled { self.popups.dismissed_file_token = Some(DismissedToken::new( - self.draft.textarea.text(), + &self.draft.textarea, current_range, current_token, )); } else { self.popups.dismissed_mention_token = Some(DismissedToken::new( - self.draft.textarea.text(), + &self.draft.textarea, current_range, current_token, )); @@ -2159,6 +2153,7 @@ impl ChatComposer { } fn insert_selected_file_path(&mut self, token_range: Range, selected_path: &str) { + let token_range = self.separate_completion_from_adjacent_element(token_range); if Self::is_image_path(selected_path) { let path_buf = PathBuf::from(selected_path); match image::image_dimensions(&path_buf) { @@ -2310,132 +2305,13 @@ impl ChatComposer { skills_ready || plugins_ready || connectors_ready } - /// Extract a token prefixed with `prefix` under the cursor, if any. - /// - /// The returned string **does not** include the prefix. - /// - /// Behavior: - /// - The cursor may be anywhere *inside* the token (including on the - /// leading prefix). It does **not** need to be at the end of the line. - /// - A token is delimited by ASCII whitespace (space, tab, newline). - /// - If the cursor is on `prefix` inside an existing token (for example the - /// second `@` in `@scope/pkg@latest`), keep treating the surrounding - /// whitespace-delimited token as the active token rather than starting a - /// new token at that nested prefix. - /// - If the token under the cursor starts with `prefix`, its byte range and - /// text without the leading prefix are returned. When `allow_empty` is - /// true, a lone prefix character yields `Some(String::new())` to surface hints. - fn current_prefixed_token_range( - textarea: &TextArea, - prefix: char, - allow_empty: bool, - ) -> Option<(Range, String)> { - let cursor_offset = textarea.cursor(); - let text = textarea.text(); - - // Adjust the provided byte offset to the nearest valid char boundary at or before it. - let mut safe_cursor = cursor_offset.min(text.len()); - // If we're not on a char boundary, move back to the start of the current char. - if safe_cursor < text.len() && !text.is_char_boundary(safe_cursor) { - // Find the last valid boundary <= cursor_offset. - safe_cursor = text - .char_indices() - .map(|(i, _)| i) - .take_while(|&i| i <= cursor_offset) - .last() - .unwrap_or(0); - } - - // Split the line around the (now safe) cursor position. - let before_cursor = &text[..safe_cursor]; - let after_cursor = &text[safe_cursor..]; - - // Detect whether we're on whitespace at the cursor boundary. - let at_whitespace = if safe_cursor < text.len() { - text[safe_cursor..] - .chars() - .next() - .map(char::is_whitespace) - .unwrap_or(false) - } else { - false - }; - - // Left candidate: token containing the cursor position. - let start_left = before_cursor - .char_indices() - .rfind(|(_, c)| c.is_whitespace()) - .map(|(idx, c)| idx + c.len_utf8()) - .unwrap_or(0); - let end_left_rel = after_cursor - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(after_cursor.len()); - let end_left = safe_cursor + end_left_rel; - let token_left = if start_left < end_left { - Some(&text[start_left..end_left]) - } else { - None - }; - - // Right candidate: token immediately after any whitespace from the cursor. - let ws_len_right: usize = after_cursor - .chars() - .take_while(|c| c.is_whitespace()) - .map(char::len_utf8) - .sum(); - let start_right = safe_cursor + ws_len_right; - let end_right_rel = text[start_right..] - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(text.len() - start_right); - let end_right = start_right + end_right_rel; - let token_right = if start_right < end_right { - Some(&text[start_right..end_right]) - } else { - None - }; - - let prefix_str = prefix.to_string(); - let left_match = token_left.filter(|t| t.starts_with(prefix)); - let right_match = token_right.filter(|t| t.starts_with(prefix)); - - let left_prefixed = - left_match.map(|t| (start_left..end_left, t[prefix.len_utf8()..].to_string())); - let right_prefixed = - right_match.map(|t| (start_right..end_right, t[prefix.len_utf8()..].to_string())); - - if at_whitespace { - if right_prefixed.is_some() { - return right_prefixed; - } - if token_left.is_some_and(|t| t == prefix_str) { - return allow_empty.then(|| (start_left..end_left, String::new())); - } - return left_prefixed; - } - if after_cursor.starts_with(prefix) { - let prefix_starts_token = before_cursor - .chars() - .next_back() - .is_none_or(char::is_whitespace); - return if prefix_starts_token { - right_prefixed.or(left_prefixed) - } else { - left_prefixed - }; - } - left_prefixed.or(right_prefixed) - } - fn current_prefixed_token( textarea: &TextArea, prefix: char, allow_empty: bool, ) -> Option { - Self::current_prefixed_token_range(textarea, prefix, allow_empty).map(|(_, token)| token) + completion_target::current_prefixed_token_range(textarea, prefix, allow_empty) + .map(|(_, token)| token) } /// Extract the `@token` that the cursor is currently positioned on, if any. @@ -2445,40 +2321,34 @@ impl ChatComposer { Self::current_prefixed_token(textarea, '@', /*allow_empty*/ false) } + /// Returns the active prefixed token only when its sigil and name remain editable plaintext. + /// + /// Atomic elements and tokens whose mention prefix is already atomic are excluded so bound + /// mentions are not offered for completion again. + fn current_editable_prefixed_token_range( + &self, + prefix: char, + allow_empty: bool, + ) -> Option<(Range, String)> { + let (range, token) = completion_target::current_prefixed_token_range( + &self.draft.textarea, + prefix, + allow_empty, + )?; + completion_target::prefixed_token_range_is_editable( + &self.draft.textarea, + prefix, + &range, + &token, + ) + .then_some((range, token)) + } + fn current_editable_at_token_range_with_options( &self, allow_empty: bool, ) -> Option<(Range, String)> { - let (range, token) = - Self::current_prefixed_token_range(&self.draft.textarea, '@', allow_empty)?; - if self - .draft - .textarea - .element_id_for_exact_range(range.clone()) - .is_some() - { - return None; - } - - let name_len = token - .as_bytes() - .iter() - .take_while(|byte| is_mention_name_char(**byte)) - .count(); - let mention_end = range.start + '@'.len_utf8() + name_len; - if name_len > 0 - && mention_end < range.end - && ends_plaintext_at_mention(self.draft.textarea.text().as_bytes(), mention_end) - && self - .draft - .textarea - .element_id_for_exact_range(range.start..mention_end) - .is_some() - { - return None; - } - - Some((range, token)) + self.current_editable_prefixed_token_range('@', allow_empty) } fn current_editable_at_token_with_options(&self, allow_empty: bool) -> Option { @@ -2506,7 +2376,10 @@ impl ChatComposer { if !self.mentions_enabled() { return None; } - Self::current_prefixed_token_range(&self.draft.textarea, '$', /*allow_empty*/ true) + let (range, token) = + self.current_editable_prefixed_token_range('$', /*allow_empty*/ true)?; + + completion_target::dollar_query_is_completable(&token).then_some((range, token)) } /// Replace the active `@token` (the one under the cursor) with `path`. @@ -2537,6 +2410,7 @@ impl ChatComposer { insert_text: &str, path: Option<&str>, ) { + let token_range = self.separate_completion_from_adjacent_element(token_range); // Remove the active token and insert the selected mention as an atomic element. let start_idx = token_range.start; self.draft.textarea.replace_range(token_range, ""); @@ -2565,6 +2439,28 @@ impl ChatComposer { } } + /// Inserts a leading separator when a completion starts directly after an atomic element. + /// + /// The returned range is shifted to keep replacing the same editable token after insertion. + fn separate_completion_from_adjacent_element( + &mut self, + mut token_range: Range, + ) -> Range { + let starts_after_element = self + .draft + .textarea + .text_element_ranges() + .any(|range| range.end == token_range.start); + if starts_after_element { + self.draft + .textarea + .replace_range(token_range.start..token_range.start, " "); + token_range.start += 1; + token_range.end += 1; + } + token_range + } + fn mention_token_from_insert_text(insert_text: &str) -> Option<(char, String)> { let sigil = insert_text.chars().next()?; if !matches!(sigil, '$' | '@') { @@ -3562,8 +3458,8 @@ impl ChatComposer { self.popups.active = ActivePopup::None; return; } - let mentions_v2_token = self.current_mentions_v2_token_range(); - let file_token = if self.mentions_v2_enabled { + let mut mentions_v2_token = self.current_mentions_v2_token_range(); + let mut file_token = if self.mentions_v2_enabled { None } else { self.current_editable_at_token_range_with_options(/*allow_empty*/ false) @@ -3582,7 +3478,22 @@ impl ChatComposer { self.popups.active = ActivePopup::None; return; } - let mention_token = self.current_mention_token_range(); + let mut mention_token = self.current_mention_token_range(); + let at_token_start = mentions_v2_token + .as_ref() + .or(file_token.as_ref()) + .map(|(range, _)| range.start); + let mention_token_start = mention_token.as_ref().map(|(range, _)| range.start); + if let (Some(at_token_start), Some(mention_token_start)) = + (at_token_start, mention_token_start) + { + if at_token_start > mention_token_start { + mention_token = None; + } else if mention_token_start > at_token_start { + mentions_v2_token = None; + file_token = None; + } + } let allow_command_popup = self.slash_commands_enabled() && !self.draft.is_bash_mode @@ -3694,12 +3605,11 @@ impl ChatComposer { /// Synchronize the legacy file-search popup with the current `@` token. fn sync_file_search_popup(&mut self, range: Range, 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)) + .is_some_and(|dismissed| dismissed.matches(&self.draft.textarea, &range, &query)) { return; } @@ -3740,12 +3650,11 @@ impl ChatComposer { } fn sync_mention_popup(&mut self, range: Range, 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)) + .is_some_and(|dismissed| dismissed.matches(&self.draft.textarea, &range, &query)) { return; } @@ -3770,12 +3679,11 @@ impl ChatComposer { } fn sync_mentions_v2_popup(&mut self, range: Range, 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)) + .is_some_and(|dismissed| dismissed.matches(&self.draft.textarea, &range, &query)) { return; } @@ -6314,6 +6222,435 @@ mod tests { assert_eq!(mention.path, Some("plugin://sample@test".to_string())); } + fn test_skill_metadata(name: &str) -> SkillMetadata { + SkillMetadata { + name: name.to_string(), + description: "Example skill used in tests.".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(), + scope: crate::test_support::skill_scope_user(), + plugin_id: None, + } + } + + fn test_skill_binding(name: &str) -> MentionBinding { + MentionBinding { + sigil: '$', + mention: name.to_string(), + path: test_path_buf(&format!("/tmp/{name}/SKILL.md")) + .abs() + .display() + .to_string(), + } + } + + fn test_plugin_binding(name: &str) -> MentionBinding { + MentionBinding { + sigil: '@', + mention: name.to_string(), + path: format!("plugin://{name}@test"), + } + } + + fn test_plugin_summary(name: &str, description: &str) -> PluginCapabilitySummary { + PluginCapabilitySummary { + config_name: format!("{name}@test"), + display_name: name.to_string(), + description: Some(description.to_string()), + has_skills: false, + mcp_server_names: vec![name.to_string()], + app_connector_ids: Vec::new(), + } + } + + fn configure_partially_bound_skill_mentions(composer: &mut ChatComposer) { + composer.set_skill_mentions(Some(vec![test_skill_metadata("unbound-skill")])); + + composer.set_text_content_with_mention_bindings( + "$unbound-skill $bound-skill continue".to_string(), + Vec::new(), + Vec::new(), + vec![test_skill_binding("bound-skill")], + ); + composer.draft.textarea.set_cursor("$unbound-skill ".len()); + composer.sync_popups(); + } + + fn configure_bound_skill_left_of_unbound_skill(composer: &mut ChatComposer) { + composer.set_skill_mentions(Some(vec![test_skill_metadata("unbound-skill")])); + composer.set_text_content_with_mention_bindings( + "$bound-skill$unbound-skill".to_string(), + Vec::new(), + Vec::new(), + vec![test_skill_binding("bound-skill")], + ); + composer.draft.textarea.set_cursor("$bound-skill".len()); + composer.sync_popups(); + } + + fn configure_skill_target_between_bound_mentions(composer: &mut ChatComposer) { + composer.set_skill_mentions(Some(vec![test_skill_metadata("other")])); + composer.set_text_content_with_mention_bindings( + "$bound1 $oth $bound2".to_string(), + Vec::new(), + Vec::new(), + vec![test_skill_binding("bound1"), test_skill_binding("bound2")], + ); + composer + .draft + .textarea + .replace_range("$bound1".len().."$bound1 ".len(), ""); + composer + .draft + .textarea + .replace_range("$bound1$oth".len().."$bound1$oth ".len(), ""); + composer.draft.textarea.set_cursor("$bound1$o".len()); + composer.sync_popups(); + } + + fn configure_skill_mention_with_trailing_space(composer: &mut ChatComposer) { + composer.set_skill_mentions(Some(vec![test_skill_metadata("figma")])); + let text = "$figma "; + composer.set_text_content(text.to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor(text.len()); + composer.sync_popups(); + } + + fn configure_bound_plugin_left_of_unbound_plugin(composer: &mut ChatComposer) { + composer.set_mentions_v2_enabled(/*enabled*/ true); + composer.set_plugin_mentions(Some(vec![test_plugin_summary( + "other", + "Plugin used to test adjacent mention targeting.", + )])); + composer.set_text_content_with_mention_bindings( + "@sample@other".to_string(), + Vec::new(), + Vec::new(), + vec![test_plugin_binding("sample")], + ); + composer.draft.textarea.set_cursor("@sample".len()); + composer.sync_popups(); + } + + fn configure_unbound_plugin_left_of_bound_plugin(composer: &mut ChatComposer) { + composer.set_mentions_v2_enabled(/*enabled*/ true); + composer.set_plugin_mentions(Some(vec![test_plugin_summary( + "left", + "Plugin used to test bound mention fallback.", + )])); + composer.set_text_content_with_mention_bindings( + "@left @bound".to_string(), + Vec::new(), + Vec::new(), + vec![test_plugin_binding("bound")], + ); + composer.draft.textarea.set_cursor("@left ".len()); + composer.sync_popups(); + } + + fn configure_plugin_target_between_bound_mentions(composer: &mut ChatComposer) { + composer.set_mentions_v2_enabled(/*enabled*/ true); + composer.set_plugin_mentions(Some(vec![test_plugin_summary( + "other", + "Plugin used to test middle mention targeting.", + )])); + composer.set_text_content_with_mention_bindings( + "@bound1 @oth @bound2".to_string(), + Vec::new(), + Vec::new(), + vec![test_plugin_binding("bound1"), test_plugin_binding("bound2")], + ); + composer + .draft + .textarea + .replace_range("@bound1".len().."@bound1 ".len(), ""); + composer + .draft + .textarea + .replace_range("@bound1@oth".len().."@bound1@oth ".len(), ""); + composer.draft.textarea.set_cursor("@bound1@o".len()); + composer.sync_popups(); + } + + #[test] + fn skill_popup_targets_unbound_mention_left_of_bound_mention() { + let (mut composer, _rx) = new_test_composer(); + configure_partially_bound_skill_mentions(&mut composer); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert_eq!( + composer.current_text(), + "$unbound-skill $bound-skill continue" + ); + assert_eq!( + composer.mention_bindings(), + vec![ + test_skill_binding("unbound-skill"), + test_skill_binding("bound-skill"), + ] + ); + + composer.insert_str("foo"); + assert_eq!( + composer.current_text(), + "$unbound-skill foo $bound-skill continue" + ); + } + + #[test] + fn skill_popup_targets_unbound_mention_left_of_bound_mention_snapshot() { + snapshot_composer_state( + "skill_popup_targets_unbound_mention_left_of_bound_mention", + /*enhanced_keys_supported*/ false, + configure_partially_bound_skill_mentions, + ); + } + + #[test] + fn skill_popup_targets_unbound_mention_right_of_adjacent_bound_mention_snapshot() { + snapshot_composer_state( + "skill_popup_targets_unbound_mention_right_of_adjacent_bound_mention", + /*enhanced_keys_supported*/ false, + configure_bound_skill_left_of_unbound_skill, + ); + } + + #[test] + fn skill_popup_closes_after_trailing_space_snapshot() { + snapshot_composer_state( + "skill_popup_closes_after_trailing_space", + /*enhanced_keys_supported*/ false, + configure_skill_mention_with_trailing_space, + ); + } + + #[test] + fn unified_mention_popup_falls_back_from_bound_plugin_on_right_snapshot() { + snapshot_composer_state( + "unified_mention_popup_falls_back_from_bound_plugin_on_right", + /*enhanced_keys_supported*/ false, + configure_unbound_plugin_left_of_bound_plugin, + ); + } + + #[test] + fn adjacent_plugin_completion_inserts_separator_snapshot() { + snapshot_composer_state( + "adjacent_plugin_completion_inserts_separator", + /*enhanced_keys_supported*/ false, + |composer| { + configure_bound_plugin_left_of_unbound_plugin(composer); + let _ = + composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + }, + ); + } + + #[test] + fn skill_completion_replaces_only_middle_adjacent_target() { + let (mut composer, _rx) = new_test_composer(); + configure_skill_target_between_bound_mentions(&mut composer); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!(composer.current_text(), "$bound1 $other $bound2"); + assert_eq!( + composer.mention_bindings(), + vec![ + test_skill_binding("bound1"), + test_skill_binding("other"), + test_skill_binding("bound2"), + ] + ); + } + + #[test] + fn unified_completion_replaces_only_middle_adjacent_target() { + let (mut composer, _rx) = new_test_composer(); + configure_plugin_target_between_bound_mentions(&mut composer); + + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!(composer.current_text(), "@bound1 @other @bound2"); + assert_eq!( + composer.mention_bindings(), + vec![ + test_plugin_binding("bound1"), + test_plugin_binding("other"), + test_plugin_binding("bound2"), + ] + ); + } + + #[test] + fn skill_popup_falls_back_from_shell_variable_to_skill_on_right() { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("rustdoc")])); + composer.set_text_content("$HOME $rustdoc".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("$HOME ".len()); + composer.sync_popups(); + + let ActivePopup::Skill(popup) = &composer.popups.active else { + panic!("expected skill popup to fall back to the right token"); + }; + assert_eq!( + popup + .selected_mention() + .expect("expected rustdoc selection") + .insert_text, + "$rustdoc" + ); + } + + #[test] + fn skill_popup_accepts_lowercase_skill_named_like_env_var() { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("home")])); + composer.set_text_content("$home".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("$home".len()); + composer.sync_popups(); + + let ActivePopup::Skill(popup) = &composer.popups.active else { + panic!("expected skill popup for lowercase home skill"); + }; + assert_eq!( + popup + .selected_mention() + .expect("expected home skill selection") + .insert_text, + "$home" + ); + } + + #[test] + fn legacy_file_popup_falls_back_when_right_skill_is_unavailable() { + let (mut composer, _rx) = new_test_composer(); + composer.set_text_content("@src $missing".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("@src ".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::File(_))); + } + + #[test] + fn unified_popup_falls_back_when_right_skill_is_unavailable() { + let (mut composer, _rx) = new_test_composer(); + composer.set_mentions_v2_enabled(/*enabled*/ true); + composer.set_text_content("@src $missing".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("@src ".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::MentionV2(_))); + } + + #[test] + fn skill_popup_falls_back_when_right_at_mention_is_bound() { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("old")])); + composer.set_text_content_with_mention_bindings( + "$old @bound".to_string(), + Vec::new(), + Vec::new(), + vec![MentionBinding { + sigil: '@', + mention: "bound".to_string(), + path: "plugin://bound@test".to_string(), + }], + ); + composer.draft.textarea.set_cursor("$old ".len()); + composer.sync_popups(); + + let ActivePopup::Skill(popup) = &composer.popups.active else { + panic!("expected the left skill popup"); + }; + assert_eq!( + popup + .selected_mention() + .expect("expected old skill selection") + .insert_text, + "$old" + ); + } + + #[test] + fn legacy_popup_prefers_right_at_token_over_left_skill() { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("old")])); + composer.set_text_content("$old @new".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("$old ".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::File(_))); + } + + #[test] + fn unified_popup_prefers_right_skill_over_left_at_token() { + let (mut composer, _rx) = new_test_composer(); + composer.set_mentions_v2_enabled(/*enabled*/ true); + composer.set_skill_mentions(Some(vec![test_skill_metadata("new")])); + composer.set_text_content("@old $new".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("@old ".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::Skill(_))); + } + + #[test] + fn skill_popup_closes_at_plain_text_after_whitespace() { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("old")])); + composer.set_text_content("$old word".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("$old ".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::None)); + } + + #[test] + fn skill_popup_closes_between_spaces_before_plain_text_snapshot() { + snapshot_composer_state( + "skill_popup_closes_between_spaces_before_plain_text", + /*enhanced_keys_supported*/ false, + |composer| { + composer.set_skill_mentions(Some(vec![test_skill_metadata("old")])); + composer.set_text_content("$old word".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("$old ".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::None)); + }, + ); + } + + fn assert_typed_skill_prefix_starts_new_mention(inserted: &str) { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("rustdoc")])); + composer.set_text_content("$simplify-code".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor(/*pos*/ 0); + + composer.insert_str(inserted); + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert_eq!(composer.current_text(), "$rustdoc $simplify-code"); + assert_eq!( + composer.mention_bindings(), + vec![test_skill_binding("rustdoc")] + ); + } + + #[test] + fn typing_empty_skill_prefix_before_existing_skill_starts_new_mention() { + assert_typed_skill_prefix_starts_new_mention("$"); + } + + #[test] + fn typing_partial_skill_prefix_before_existing_skill_starts_new_mention() { + assert_typed_skill_prefix_starts_new_mention("$r"); + } + #[test] fn set_skill_mentions_refreshes_open_mention_popup() { let (tx, _rx) = unbounded_channel::(); @@ -9148,6 +9485,60 @@ mod tests { let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)); } + #[test] + fn legacy_file_completion_ignores_shell_variable_to_the_right() { + let (mut composer, _rx) = new_test_composer(); + composer.set_skill_mentions(Some(vec![test_skill_metadata("available")])); + complete_file( + &mut composer, + "@src $HOME", + /*cursor*/ "@src ".len(), + "src", + PathBuf::from("src/main.rs"), + ); + + assert_eq!(composer.current_text(), "src/main.rs $HOME"); + } + + #[test] + fn unified_popup_preserves_empty_at_before_existing_text() { + let (mut composer, _rx) = new_test_composer(); + composer.set_mentions_v2_enabled(/*enabled*/ true); + composer.set_text_content("@ word".to_string(), Vec::new(), Vec::new()); + composer.draft.textarea.set_cursor("@".len()); + composer.sync_popups(); + + assert!(matches!(composer.popups.active, ActivePopup::MentionV2(_))); + } + + #[test] + fn adjacent_file_completion_inserts_leading_separator() { + let (mut composer, _rx) = new_test_composer(); + configure_bound_plugin_left_of_unbound_plugin(&mut composer); + composer.insert_selected_file_path("@sample".len().."@sample@other".len(), "src/main.rs"); + + assert_eq!(composer.current_text(), "@sample src/main.rs "); + } + + #[test] + fn adjacent_image_completion_inserts_leading_separator() { + let tmp = tempdir().expect("create TempDir"); + let image_path = tmp.path().join("image.png"); + let image: ImageBuffer, Vec> = + ImageBuffer::from_fn(3, 2, |_x, _y| Rgba([1, 2, 3, 255])); + image.save(&image_path).expect("write temp png"); + + let (mut composer, _rx) = new_test_composer(); + configure_bound_plugin_left_of_unbound_plugin(&mut composer); + composer.insert_selected_file_path( + "@sample".len().."@sample@other".len(), + image_path.to_str().expect("UTF-8 path"), + ); + + assert_eq!(composer.current_text(), "@sample [Image #1] "); + assert_eq!(composer.local_image_paths(), vec![image_path]); + } + #[test] fn file_completion_preserves_separator_before_existing_suffix() { let (mut composer, _rx) = new_test_composer(); diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/completion_target.rs b/codex-rs/tui/src/bottom_pane/chat_composer/completion_target.rs new file mode 100644 index 0000000000..919d4d4644 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/chat_composer/completion_target.rs @@ -0,0 +1,298 @@ +//! Cursor-neighborhood resolution for sigil-prefixed composer completions. + +use super::ends_plaintext_at_mention; +use super::is_mention_name_char; +use crate::bottom_pane::textarea::TextArea; +use crate::mention_codec::is_common_env_var; +use std::ops::Range; + +/// Narrows one whitespace-delimited candidate to the editable segment around `anchor`. +/// +/// Atomic elements form hard boundaries. If the anchor is inside an element, the raw candidate is +/// retained only when it is needed to identify that the candidate is already bound. +fn prefixed_candidate_range( + textarea: &TextArea, + range: Range, + anchor: usize, + prefix: char, + allow_empty: bool, +) -> Option<(Range, String)> { + let text = textarea.text(); + let prefix_len = prefix.len_utf8(); + let raw_prefixed = text + .get(range.clone()) + .filter(|token| token.starts_with(prefix)) + .map(|token| (range.clone(), token[prefix_len..].to_string())); + let mut segment_start = range.start; + let mut segment_end = range.end; + let mut has_element_boundary = false; + let anchor = anchor.clamp(range.start, range.end); + + for element in textarea.text_element_ranges_overlapping(range) { + has_element_boundary = true; + if element.end <= anchor { + segment_start = element.end; + continue; + } + if anchor <= element.start { + segment_end = element.start; + } else { + segment_start = anchor; + segment_end = anchor; + } + break; + } + + if !has_element_boundary { + return raw_prefixed; + } + + let segmented_prefixed = text + .get(segment_start..segment_end) + .and_then(|token| token.strip_prefix(prefix)) + .filter(|query| allow_empty || !query.is_empty()) + .map(|query| (segment_start..segment_end, query.to_string())); + if segmented_prefixed.is_some() + || raw_prefixed.as_ref().is_none_or(|(range, query)| { + prefixed_token_range_is_editable(textarea, prefix, range, query) + }) + { + segmented_prefixed + } else { + raw_prefixed + } +} + +/// Extracts a token prefixed with `prefix` under the cursor, if any. +/// +/// The returned string does not include the prefix. Resolution considers the tokens on both sides +/// of same-line separator whitespace and preserves their ranges for cross-sigil arbitration. It +/// stops editable candidates at atomic text elements, and line breaks never provide affinity. +pub(super) fn current_prefixed_token_range( + textarea: &TextArea, + prefix: char, + allow_empty: bool, +) -> Option<(Range, String)> { + let cursor_offset = textarea.cursor(); + let text = textarea.text(); + + // Adjust the provided byte offset to the nearest valid char boundary at or before it. + let mut safe_cursor = cursor_offset.min(text.len()); + if safe_cursor < text.len() && !text.is_char_boundary(safe_cursor) { + safe_cursor = text + .char_indices() + .map(|(i, _)| i) + .take_while(|&i| i <= cursor_offset) + .last() + .unwrap_or(0); + } + + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; + let is_horizontal_whitespace = |c: char| { + c.is_whitespace() + && !matches!( + c, + '\n' | '\r' | '\u{000B}' | '\u{000C}' | '\u{0085}' | '\u{2028}' | '\u{2029}' + ) + }; + + let at_whitespace = after_cursor.chars().next().is_some_and(char::is_whitespace); + let after_horizontal_whitespace = before_cursor + .chars() + .next_back() + .is_some_and(is_horizontal_whitespace); + let cursor_starts_token = after_horizontal_whitespace && !at_whitespace; + let next_non_separator = after_cursor.chars().find(|c| !is_horizontal_whitespace(*c)); + let separator_precedes_token = next_non_separator.is_some_and(|c| !c.is_whitespace()); + let separator_precedes_completion = next_non_separator.is_some_and(|c| matches!(c, '$' | '@')); + let at_separator = (at_whitespace || after_horizontal_whitespace) && separator_precedes_token; + + let end_left = if at_separator { + before_cursor + .trim_end_matches(is_horizontal_whitespace) + .len() + } else { + let end_left_rel = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + safe_cursor + end_left_rel + }; + let start_left = text[..end_left] + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); + + let ws_len_right: usize = after_cursor + .chars() + .take_while(|c| is_horizontal_whitespace(*c)) + .map(char::len_utf8) + .sum(); + let start_right = safe_cursor + ws_len_right; + let end_right_rel = text[start_right..] + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(text.len() - start_right); + let end_right = start_right + end_right_rel; + let token_right = if start_right < end_right { + Some(&text[start_right..end_right]) + } else { + None + }; + + let left_prefixed = prefixed_candidate_range( + textarea, + start_left..end_left, + safe_cursor, + prefix, + allow_empty, + ); + let right_prefixed = prefixed_candidate_range( + textarea, + start_right..end_right, + start_right, + prefix, + allow_empty, + ); + + if cursor_starts_token { + let right_is_bound = token_right + .and_then(|token| { + let prefix = token.chars().next()?; + matches!(prefix, '$' | '@').then_some((prefix, &token[prefix.len_utf8()..])) + }) + .is_some_and(|(prefix, query)| { + !prefixed_token_range_is_editable( + textarea, + prefix, + &(start_right..end_right), + query, + ) + }); + return if right_is_bound { + left_prefixed + } else { + right_prefixed + }; + } + + if allow_empty && after_cursor.starts_with(prefix) { + let left_fragment = &text[start_left..safe_cursor]; + if let Some(left_token) = left_fragment.strip_prefix(prefix) + && left_token + .as_bytes() + .iter() + .all(|byte| is_mention_name_char(*byte)) + { + let left_range = start_left..safe_cursor; + let left_is_editable = + prefixed_token_range_is_editable(textarea, prefix, &left_range, left_token); + if left_token.is_empty() || prefix == '$' && left_is_editable { + return Some((left_range, left_token.to_string())); + } + if !left_is_editable { + return left_prefixed.or(right_prefixed); + } + } + } + + if at_separator { + if after_horizontal_whitespace && !separator_precedes_completion { + return right_prefixed; + } + if prefix == '$' + && left_prefixed + .as_ref() + .is_some_and(|(_, query)| !dollar_query_is_completable(query)) + && right_prefixed + .as_ref() + .is_some_and(|(_, query)| dollar_query_is_completable(query)) + { + return right_prefixed; + } + if prefix == '@' + && right_prefixed.as_ref().is_some_and(|(range, query)| { + prefixed_token_range_is_editable(textarea, prefix, range, query) + }) + { + return right_prefixed; + } + if left_prefixed.as_ref().is_some_and(|(range, token)| { + !prefixed_token_range_is_editable(textarea, prefix, range, token) + }) { + return right_prefixed.or(left_prefixed); + } + if left_prefixed + .as_ref() + .is_some_and(|(_, token)| token.is_empty()) + && !allow_empty + { + return right_prefixed; + } + return left_prefixed.or(right_prefixed); + } + if after_cursor.starts_with(prefix) { + let prefix_starts_token = before_cursor + .chars() + .next_back() + .is_none_or(char::is_whitespace); + return if prefix_starts_token { + right_prefixed.or(left_prefixed) + } else { + left_prefixed + }; + } + left_prefixed.or(right_prefixed) +} + +/// Returns whether a token candidate's sigil and mention name are editable plaintext. +/// +/// A candidate is bound when either its entire range is atomic or its mention-name prefix is +/// atomic and the remaining text begins with punctuation that terminates the mention. +pub(super) fn prefixed_token_range_is_editable( + textarea: &TextArea, + prefix: char, + range: &Range, + token: &str, +) -> bool { + if textarea.element_id_for_exact_range(range.clone()).is_some() { + return false; + } + + let name_len = token + .as_bytes() + .iter() + .take_while(|byte| is_mention_name_char(**byte)) + .count(); + let mention_end = range.start + prefix.len_utf8() + name_len; + !(name_len > 0 + && mention_end < range.end + && ends_plaintext_at_mention(textarea.text().as_bytes(), mention_end) + && textarea + .element_id_for_exact_range(range.start..mention_end) + .is_some()) +} + +/// Rejects shell-like dollar syntax while preserving lowercase and plugin-qualified skill queries. +/// +/// Only uppercase common environment-variable spellings are excluded. The check uses the complete +/// colon-qualified name so a skill such as `home` or `home:search` is not mistaken for `$HOME`. +pub(super) fn dollar_query_is_completable(query: &str) -> bool { + let name_end = query + .as_bytes() + .iter() + .take_while(|byte| is_mention_name_char(**byte) || **byte == b':') + .count(); + let name = &query[..name_end]; + let is_shell_var = + name.bytes().all(|byte| !byte.is_ascii_lowercase()) && is_common_env_var(name); + query.is_empty() || name_end > 0 && !is_shell_var +} + +#[cfg(test)] +#[path = "completion_target_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/completion_target_tests.rs b/codex-rs/tui/src/bottom_pane/chat_composer/completion_target_tests.rs new file mode 100644 index 0000000000..b5a78ad6c6 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/chat_composer/completion_target_tests.rs @@ -0,0 +1,187 @@ +use super::*; +use crate::bottom_pane::textarea::TextArea; +use pretty_assertions::assert_eq; + +#[test] +fn current_prefixed_token_affinity_does_not_cross_line_break() { + for (text, prefix, allow_empty) in [ + ("@file\n continue", '@', false), + ("continue \n@file", '@', false), + ("$skill\n continue", '$', true), + ("continue \n$skill", '$', true), + ] { + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.set_cursor(text.find(" ").expect("indentation present") + 1); + + assert_eq!( + current_prefixed_token_range(&textarea, prefix, allow_empty), + None + ); + } +} + +#[test] +fn current_prefixed_token_affinity_does_not_cross_separator_before_plain_text() { + for (text, prefix, allow_empty) in [("@old word", '@', false), ("$old word", '$', true)] { + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.set_cursor(text.find(" ").expect("separator present") + 1); + + assert_eq!( + current_prefixed_token_range(&textarea, prefix, allow_empty), + None + ); + } +} + +#[test] +fn current_prefixed_token_prefers_right_after_adjacent_target() { + let bound = "@bound"; + let left = "@bound@old"; + let text = "@bound@old @new"; + let right_start = left.len() + 2; + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.add_element_range(0..bound.len()); + textarea.set_cursor(left.len()); + + assert_eq!( + current_prefixed_token_range(&textarea, '@', /*allow_empty*/ false), + Some((right_start..text.len(), "new".to_string())) + ); +} + +#[test] +fn current_prefixed_token_prefers_right_at_token_start() { + let text = "$old $new"; + let right_start = "$old ".len(); + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.set_cursor(right_start); + + assert_eq!( + current_prefixed_token_range(&textarea, '$', /*allow_empty*/ true), + Some((right_start..text.len(), "new".to_string())) + ); +} + +#[test] +fn current_prefixed_token_falls_back_from_bound_at_token_on_right() { + let text = "@left @bound"; + let bound_start = text.find("@bound").expect("bound mention"); + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.add_element_range(bound_start..text.len()); + textarea.set_cursor("@left ".len()); + + assert_eq!( + current_prefixed_token_range(&textarea, '@', /*allow_empty*/ true), + Some((0.."@left".len(), "left".to_string())) + ); +} + +#[test] +fn current_prefixed_token_targets_later_segment_between_bound_elements() { + let text = "$bound1$one$bound2$two$bound3"; + let bound2_start = text.find("$bound2").expect("second bound mention"); + let two_start = text.find("$two").expect("later plaintext target"); + let bound3_start = text.find("$bound3").expect("third bound mention"); + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.add_element_range(0.."$bound1".len()); + textarea.add_element_range(bound2_start..two_start); + textarea.add_element_range(bound3_start..text.len()); + textarea.set_cursor(two_start + "$t".len()); + + assert_eq!( + current_prefixed_token_range(&textarea, '$', /*allow_empty*/ true), + Some((two_start..bound3_start, "two".to_string())) + ); +} + +#[test] +fn current_prefixed_token_bounds_right_candidate_before_bound_element() { + let text = "continue @other@bound"; + let other_start = text.find("@other").expect("editable mention"); + let bound_start = text.find("@bound").expect("bound mention"); + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.add_element_range(bound_start..text.len()); + textarea.set_cursor(other_start); + + assert_eq!( + current_prefixed_token_range(&textarea, '@', /*allow_empty*/ false), + Some((other_start..bound_start, "other".to_string())) + ); +} + +#[test] +fn current_prefixed_token_stops_before_overlapping_element() { + let editable = "$other"; + let placeholder = "[Pasted Content 1001 chars]"; + let text = format!("{editable}{placeholder}"); + let mut textarea = TextArea::new(); + textarea.insert_str(&text); + textarea.add_element_range(editable.len()..text.len()); + textarea.set_cursor(editable.len()); + + assert_eq!( + current_prefixed_token_range(&textarea, '$', /*allow_empty*/ true), + Some((0..editable.len(), "other".to_string())) + ); +} + +#[test] +fn current_prefixed_token_stops_at_trailing_space_before_line_break() { + let text = "$figma \nnext"; + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.set_cursor("$figma ".len()); + + assert_eq!( + current_prefixed_token_range(&textarea, '$', /*allow_empty*/ true), + None + ); +} + +#[test] +fn current_prefixed_token_keeps_nested_dollar_prefix_in_same_token() { + let text = "$HOME/$USER"; + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.set_cursor(text.find("$USER").expect("nested prefix present")); + + assert_eq!( + current_prefixed_token_range(&textarea, '$', /*allow_empty*/ true), + Some((0..text.len(), "HOME/$USER".to_string())) + ); +} + +#[test] +fn typed_qualified_suffix_preserves_bound_plain_mention() { + // Qualified skill bindings and history persistence are intentionally deferred. Typing a + // suffix after a bound plain mention must not reinterpret it as an editable qualified one. + let bound = "$google-calendar"; + let text = "$google-calendar:availability"; + let mut textarea = TextArea::new(); + textarea.insert_str(text); + textarea.add_element_range(0..bound.len()); + textarea.set_cursor(text.len()); + + let (range, token) = current_prefixed_token_range(&textarea, '$', /*allow_empty*/ true) + .expect("dollar token should be present"); + assert_eq!((range.clone(), token.as_str()), (0..text.len(), &text[1..])); + assert!(!prefixed_token_range_is_editable( + &textarea, '$', &range, &token, + )); +} + +#[test] +fn dollar_query_classifies_shell_and_skill_syntax() { + assert!(!dollar_query_is_completable("{CODEX_HOME}/config.toml")); + assert!(!dollar_query_is_completable("(pwd)")); + assert!(dollar_query_is_completable("home:search")); + assert!(dollar_query_is_completable("home")); + assert!(!dollar_query_is_completable("HOME")); +} diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/popup_state.rs b/codex-rs/tui/src/bottom_pane/chat_composer/popup_state.rs index 71e17cc4fa..29a216cd1e 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer/popup_state.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer/popup_state.rs @@ -5,6 +5,7 @@ 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 crate::bottom_pane::textarea::TextArea; use std::ops::Range; /// One token occurrence whose autocomplete popup should remain hidden. @@ -19,9 +20,10 @@ pub(super) struct DismissedToken { impl DismissedToken { /// Captures the stable identity of the token at `range`. - pub(super) fn new(text: &str, range: Range, query: String) -> Self { + pub(super) fn new(textarea: &TextArea, range: Range, query: String) -> Self { + let text = textarea.text(); let token = text[range.clone()].to_string(); - let occurrence = complete_token_occurrences_before(text, &token, range.start); + let occurrence = complete_token_occurrences_before(textarea, &token, range.start); Self { query, token, @@ -33,18 +35,54 @@ impl DismissedToken { /// /// 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, query: &str) -> bool { + pub(super) fn matches(&self, textarea: &TextArea, range: &Range, query: &str) -> bool { + let text = textarea.text(); 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 + complete_token_occurrences_before(textarea, &self.token, range.start) == self.occurrence } } -fn complete_token_occurrences_before(text: &str, token: &str, before: usize) -> usize { +/// Counts complete editable-token occurrences before `before` in a single ordered pass. +/// +/// Whitespace and atomic element edges delimit occurrences; bare nested sigils do not. This keeps +/// dismissal identity aligned with the completion resolver without rescanning every element for +/// each match. +fn complete_token_occurrences_before(textarea: &TextArea, token: &str, before: usize) -> usize { + let text = textarea.text(); + let mut elements_ending_before_matches = textarea.text_element_ranges().peekable(); + let mut elements_starting_after_matches = textarea.text_element_ranges().peekable(); text[..before] - .split_whitespace() - .filter(|candidate| *candidate == token) + .match_indices(token) + .filter(|(start, _)| { + let end = start + token.len(); + while elements_ending_before_matches + .peek() + .is_some_and(|element| element.end < *start) + { + elements_ending_before_matches.next(); + } + while elements_starting_after_matches + .peek() + .is_some_and(|element| element.start < end) + { + elements_starting_after_matches.next(); + } + let ends_at_boundary = text[end..].chars().next().is_none_or(char::is_whitespace) + || elements_starting_after_matches + .peek() + .is_some_and(|element| element.start == end); + let starts_at_boundary = *start == 0 + || text[..*start] + .chars() + .next_back() + .is_some_and(char::is_whitespace) + || elements_ending_before_matches + .peek() + .is_some_and(|element| element.end == *start); + starts_at_boundary && ends_at_boundary + }) .count() } @@ -72,3 +110,7 @@ pub(super) enum ActivePopup { Skill(SkillPopup), MentionV2(MentionV2Popup), } + +#[cfg(test)] +#[path = "popup_state_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/popup_state_tests.rs b/codex-rs/tui/src/bottom_pane/chat_composer/popup_state_tests.rs new file mode 100644 index 0000000000..8ab143675d --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/chat_composer/popup_state_tests.rs @@ -0,0 +1,60 @@ +use super::*; +use crate::bottom_pane::textarea::TextArea; +use pretty_assertions::assert_eq; + +#[test] +fn dismissed_tokens_after_adjacent_elements_are_occurrence_scoped() { + let first_bound = "$bound1"; + let second_bound = "$bound2"; + let token = "$other"; + let text = format!("{first_bound}{token} {second_bound}{token}"); + let first_start = first_bound.len(); + let second_start = text.rfind(token).expect("second token"); + let second_bound_start = text.find(second_bound).expect("second bound mention"); + let mut textarea = TextArea::new(); + textarea.insert_str(&text); + textarea.add_element_range(0..first_start); + textarea.add_element_range(second_bound_start..second_bound_start + second_bound.len()); + let dismissed = DismissedToken::new( + &textarea, + first_start..first_start + token.len(), + "other".to_string(), + ); + + assert!(!dismissed.matches( + &textarea, + &(second_start..second_start + token.len()), + "other", + )); +} + +#[test] +fn nested_at_query_does_not_count_as_complete_dismissed_token() { + let text = "@ma@latest @ma"; + let second_start = text.rfind("@ma").expect("second token"); + let mut textarea = TextArea::new(); + textarea.insert_str(text); + + assert_eq!( + complete_token_occurrences_before(&textarea, "@ma", second_start), + 0 + ); +} + +#[test] +fn non_sigil_elements_delimit_dismissed_token_occurrences() { + let token = "$other"; + let first_element = "[one]"; + let text = format!("{token}{first_element} {token}"); + let second_start = text.rfind(token).expect("second token"); + let mut textarea = TextArea::new(); + textarea.insert_str(&text); + textarea.add_element_range(token.len()..token.len() + first_element.len()); + let dismissed = DismissedToken::new(&textarea, 0..token.len(), "other".to_string()); + + assert!(!dismissed.matches( + &textarea, + &(second_start..second_start + token.len()), + "other", + )); +} diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__adjacent_plugin_completion_inserts_separator.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__adjacent_plugin_completion_inserts_separator.snap new file mode 100644 index 0000000000..c962377f5b --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__adjacent_plugin_completion_inserts_separator.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› @sample @other " +" " +" " +" " +" " +" " +" " +" 100% context left " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_closes_after_trailing_space.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_closes_after_trailing_space.snap new file mode 100644 index 0000000000..f2cc29cf1c --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_closes_after_trailing_space.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› $figma " +" " +" " +" " +" " +" " +" " +" 100% context left " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_closes_between_spaces_before_plain_text.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_closes_between_spaces_before_plain_text.snap new file mode 100644 index 0000000000..e27fb9b1f4 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_closes_between_spaces_before_plain_text.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› $old word " +" " +" " +" " +" " +" " +" " +" 100% context left " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_targets_unbound_mention_left_of_bound_mention.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_targets_unbound_mention_left_of_bound_mention.snap new file mode 100644 index 0000000000..d2fe3b11e0 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_targets_unbound_mention_left_of_bound_mention.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› $unbound-skill $bound-skill continue " +" " +" " +" " +" " +" unbound-skill [Skill] Example skill used in tests. " +" " +" Press enter to insert or esc to close " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_targets_unbound_mention_right_of_adjacent_bound_mention.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_targets_unbound_mention_right_of_adjacent_bound_mention.snap new file mode 100644 index 0000000000..a3423c8bba --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__skill_popup_targets_unbound_mention_right_of_adjacent_bound_mention.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› $bound-skill$unbound-skill " +" " +" " +" " +" " +" unbound-skill [Skill] Example skill used in tests. " +" " +" Press enter to insert or esc to close " diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__unified_mention_popup_falls_back_from_bound_plugin_on_right.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__unified_mention_popup_falls_back_from_bound_plugin_on_right.snap new file mode 100644 index 0000000000..6bd3a5ba46 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__unified_mention_popup_falls_back_from_bound_plugin_on_right.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/bottom_pane/chat_composer.rs +expression: terminal.backend() +--- +" " +"› @left @bound " +" " +"> left Plugin used to test bound mention fallback. Plugin" +" " +" " +" " +" " +" enter insert · esc close · ←/→ switch search modes [All Results] Filesystem Only Plugins " diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index f727ca3480..22704f8706 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -1382,6 +1382,27 @@ impl TextArea { .collect() } + /// Iterates borrowed atomic element ranges in ascending start order. + pub(crate) fn text_element_ranges(&self) -> impl Iterator> { + self.elements.iter().map(|element| &element.range) + } + + /// Iterates ordered atomic element ranges that overlap `range`. + /// + /// Elements ending exactly at the range start or starting exactly at its end are excluded. + pub(crate) fn text_element_ranges_overlapping( + &self, + range: Range, + ) -> impl Iterator> { + let first = self + .elements + .partition_point(|element| element.range.end <= range.start); + self.elements[first..] + .iter() + .take_while(move |element| element.range.start < range.end) + .map(|element| &element.range) + } + pub(crate) fn element_id_for_exact_range(&self, range: Range) -> Option { self.elements .iter() diff --git a/codex-rs/tui/src/mention_codec.rs b/codex-rs/tui/src/mention_codec.rs index 520a6712ea..eb3a0d1ea5 100644 --- a/codex-rs/tui/src/mention_codec.rs +++ b/codex-rs/tui/src/mention_codec.rs @@ -251,7 +251,7 @@ fn is_mention_name_char_char(ch: char) -> bool { ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-') } -fn is_common_env_var(name: &str) -> bool { +pub(crate) fn is_common_env_var(name: &str) -> bool { let upper = name.to_ascii_uppercase(); matches!( upper.as_str(),