From 3d8cdac7978d6414a6164399d7b0d6c9f44c8b33 Mon Sep 17 00:00:00 2001 From: fcoury-oai Date: Fri, 3 Apr 2026 18:09:30 -0300 Subject: [PATCH] fix(tui): sort skill mentions by display name first (#16710) ## Summary The skill list opened by '$' shows `interface.display_name` preferably if available but the sorting order of the search results use the `skill.name` for sorting the results regardless. This can be clearly seen in this example below: I expected with "pr" as the search term to have "PR Babysitter" be the first item, but instead it's way down the list. The reason is because "PR Babysitter" skill name is "babysit-pr" and therefore it doesn't rank as high as "pr-review-triage". This PR fixes this behavior. | Before | After | | --- | --- | | image | image | ## Testing - `just fmt` - `cargo test -p codex-tui bottom_pane::skill_popup::tests::display_name_match_sorting_beats_worse_secondary_search_term_matches --lib -- --exact` - `cargo test -p codex-tui` --- codex-rs/tui/src/bottom_pane/skill_popup.rs | 167 ++++++++++++++++---- 1 file changed, 133 insertions(+), 34 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/skill_popup.rs b/codex-rs/tui/src/bottom_pane/skill_popup.rs index 2e5c7c70f..836bad636 100644 --- a/codex-rs/tui/src/bottom_pane/skill_popup.rs +++ b/codex-rs/tui/src/bottom_pane/skill_popup.rs @@ -137,31 +137,18 @@ impl SkillPopup { continue; } - let mut best_match: Option<(Option>, i32)> = None; - - if let Some((indices, score)) = fuzzy_match(&mention.display_name, filter) { - best_match = Some((Some(indices), score)); - } - - for term in &mention.search_terms { - if term == &mention.display_name { - continue; - } - - if let Some((_indices, score)) = fuzzy_match(term, filter) { - match best_match.as_mut() { - Some((best_indices, best_score)) => { - if score > *best_score { - *best_score = score; - *best_indices = None; - } - } - None => { - best_match = Some((None, score)); - } - } - } - } + let best_match = + if let Some((indices, score)) = fuzzy_match(&mention.display_name, filter) { + Some((Some(indices), score)) + } else { + mention + .search_terms + .iter() + .filter(|term| *term != &mention.display_name) + .filter_map(|term| fuzzy_match(term, filter).map(|(_indices, score)| score)) + .min() + .map(|score| (None, score)) + }; if let Some((indices, score)) = best_match { out.push((idx, indices, score)); @@ -169,15 +156,25 @@ impl SkillPopup { } out.sort_by(|a, b| { - self.mentions[a.0] - .sort_rank - .cmp(&self.mentions[b.0].sort_rank) - .then_with(|| a.2.cmp(&b.2)) - .then_with(|| { - let an = self.mentions[a.0].display_name.as_str(); - let bn = self.mentions[b.0].display_name.as_str(); - an.cmp(bn) - }) + if filter.is_empty() { + self.mentions[a.0] + .sort_rank + .cmp(&self.mentions[b.0].sort_rank) + } else { + a.1.is_none() + .cmp(&b.1.is_none()) + .then_with(|| a.2.cmp(&b.2)) + .then_with(|| { + self.mentions[a.0] + .sort_rank + .cmp(&self.mentions[b.0].sort_rank) + }) + } + .then_with(|| { + let an = self.mentions[a.0].display_name.as_str(); + let bn = self.mentions[b.0].display_name.as_str(); + an.cmp(bn) + }) }); out @@ -249,6 +246,34 @@ mod tests { } } + fn ranked_mention_item( + display_name: &str, + search_terms: &[&str], + category_tag: &str, + sort_rank: u8, + ) -> MentionItem { + MentionItem { + display_name: display_name.to_string(), + description: None, + insert_text: format!("${display_name}"), + search_terms: search_terms + .iter() + .map(|term| (*term).to_string()) + .collect(), + path: None, + category_tag: Some(category_tag.to_string()), + sort_rank, + } + } + + fn named_mention_item(display_name: &str, search_terms: &[&str]) -> MentionItem { + ranked_mention_item(display_name, search_terms, "[Skill]", /*sort_rank*/ 1) + } + + fn plugin_mention_item(display_name: &str, search_terms: &[&str]) -> MentionItem { + ranked_mention_item(display_name, search_terms, "[Plugin]", /*sort_rank*/ 0) + } + #[test] fn filtered_mentions_preserve_results_beyond_popup_height() { let popup = SkillPopup::new((0..(MAX_POPUP_ROWS + 2)).map(mention_item).collect()); @@ -288,4 +313,78 @@ mod tests { insta::assert_snapshot!("skill_popup_scrolled", render_popup(&popup, /*width*/ 72)); } + + #[test] + fn display_name_match_sorting_beats_worse_secondary_search_term_matches() { + let mut popup = SkillPopup::new(vec![ + named_mention_item("pr-review-triage", &["pr-review-triage"]), + named_mention_item("prd", &["prd"]), + named_mention_item("PR Babysitter", &["babysit-pr", "PR Babysitter"]), + named_mention_item("Plugin Creator", &["plugin-creator", "Plugin Creator"]), + named_mention_item( + "Logging Best Practices", + &["logging-best-practices", "Logging Best Practices"], + ), + ]); + popup.set_query("pr"); + + let filtered_names: Vec = popup + .filtered_items() + .into_iter() + .map(|idx| popup.mentions[idx].display_name.clone()) + .collect(); + + assert_eq!( + filtered_names, + vec![ + "PR Babysitter".to_string(), + "pr-review-triage".to_string(), + "prd".to_string(), + "Plugin Creator".to_string(), + "Logging Best Practices".to_string(), + ] + ); + } + + #[test] + fn query_match_score_sorts_before_plugin_rank_bias() { + let mut popup = SkillPopup::new(vec![ + plugin_mention_item("GitHub", &["github", "pull requests", "pr"]), + named_mention_item("pr-review-triage", &["pr-review-triage"]), + named_mention_item("prd", &["prd"]), + named_mention_item("Plugin Creator", &["plugin-creator", "Plugin Creator"]), + named_mention_item( + "Logging Best Practices", + &["logging-best-practices", "Logging Best Practices"], + ), + named_mention_item("PR Babysitter", &["babysit-pr", "PR Babysitter"]), + ]); + popup.set_query("pr"); + + let filtered_items: Vec<(String, Option)> = popup + .filtered_items() + .into_iter() + .map(|idx| { + ( + popup.mentions[idx].display_name.clone(), + popup.mentions[idx].category_tag.clone(), + ) + }) + .collect(); + + assert_eq!( + filtered_items, + vec![ + ("PR Babysitter".to_string(), Some("[Skill]".to_string())), + ("pr-review-triage".to_string(), Some("[Skill]".to_string())), + ("prd".to_string(), Some("[Skill]".to_string())), + ("Plugin Creator".to_string(), Some("[Skill]".to_string())), + ( + "Logging Best Practices".to_string(), + Some("[Skill]".to_string()) + ), + ("GitHub".to_string(), Some("[Plugin]".to_string())), + ] + ); + } }