mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Use available width for skill names in the toggle view (#32485)
## Why The skill toggle view truncated every display name to 21 characters, even when the popup had enough room to show more. This could hide the part that distinguishes similarly named skills. ## What changed - Pass full skill display names to the shared row renderer so it can fit them to the available width alongside descriptions. - Keep narrow layouts bounded by the renderer's width-aware truncation. ## Testing Added coverage for preserving and filtering full display names, plus snapshots for wide and narrow popup layouts. GitOrigin-RevId: c434237e00440f920d8aa9c365b6bda5e68e0941
This commit is contained in:
@@ -23,7 +23,6 @@ use crate::render::RectExt as _;
|
||||
use crate::render::renderable::ColumnRenderable;
|
||||
use crate::render::renderable::Renderable;
|
||||
use crate::skills_helpers::match_skill;
|
||||
use crate::skills_helpers::truncate_skill_name;
|
||||
use crate::style::user_message_style;
|
||||
|
||||
use super::CancellationEvent;
|
||||
@@ -145,7 +144,7 @@ impl SkillsToggleView {
|
||||
let is_selected = self.state.selected_idx == Some(visible_idx);
|
||||
let prefix = if is_selected { '›' } else { ' ' };
|
||||
let marker = if item.enabled { 'x' } else { ' ' };
|
||||
let item_name = truncate_skill_name(&item.name);
|
||||
let item_name = &item.name;
|
||||
let name = format!("{prefix} [{marker}] {item_name}");
|
||||
GenericDisplayRow {
|
||||
name,
|
||||
@@ -427,6 +426,7 @@ mod tests {
|
||||
use crate::test_support::PathBufExt;
|
||||
use crate::test_support::test_path_buf;
|
||||
use insta::assert_snapshot;
|
||||
use pretty_assertions::assert_eq;
|
||||
use ratatui::layout::Rect;
|
||||
use tokio::sync::mpsc::unbounded_channel;
|
||||
|
||||
@@ -453,6 +453,25 @@ mod tests {
|
||||
lines.join("\n")
|
||||
}
|
||||
|
||||
fn long_name_items() -> Vec<SkillsToggleItem> {
|
||||
vec![
|
||||
SkillsToggleItem {
|
||||
name: "superpowers-systematic-debugging (polish)".to_string(),
|
||||
skill_name: "polish:superpowers-systematic-debugging".to_string(),
|
||||
description: "Find root causes before fixing bugs".to_string(),
|
||||
enabled: true,
|
||||
path: test_path_buf("/tmp/skills/systematic-debugging/SKILL.md").abs(),
|
||||
},
|
||||
SkillsToggleItem {
|
||||
name: "superpowers-verification-before-completion (polish)".to_string(),
|
||||
skill_name: "polish:superpowers-verification-before-completion".to_string(),
|
||||
description: "Verify completion before claiming success".to_string(),
|
||||
enabled: false,
|
||||
path: test_path_buf("/tmp/skills/verification-before-completion/SKILL.md").abs(),
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renders_basic_popup() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
@@ -477,6 +496,87 @@ mod tests {
|
||||
assert_snapshot!("skills_toggle_basic", render_lines(&view, /*width*/ 72));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_rows_preserves_full_skill_display_names() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let view = SkillsToggleView::new(
|
||||
long_name_items(),
|
||||
tx,
|
||||
crate::keymap::RuntimeKeymap::defaults().list,
|
||||
);
|
||||
|
||||
let row_names = view
|
||||
.build_rows()
|
||||
.into_iter()
|
||||
.map(|row| row.name)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
row_names,
|
||||
vec![
|
||||
"› [x] superpowers-systematic-debugging (polish)",
|
||||
" [ ] superpowers-verification-before-completion (polish)",
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn filtering_long_skill_names_preserves_the_matching_row() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let mut view = SkillsToggleView::new(
|
||||
long_name_items(),
|
||||
tx,
|
||||
crate::keymap::RuntimeKeymap::defaults().list,
|
||||
);
|
||||
view.search_query = "completion".to_string();
|
||||
view.apply_filter();
|
||||
|
||||
let row_names = view
|
||||
.build_rows()
|
||||
.into_iter()
|
||||
.map(|row| row.name)
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
row_names,
|
||||
vec!["› [ ] superpowers-verification-before-completion (polish)"]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renders_long_names_using_available_width() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let view = SkillsToggleView::new(
|
||||
long_name_items(),
|
||||
tx,
|
||||
crate::keymap::RuntimeKeymap::defaults().list,
|
||||
);
|
||||
|
||||
assert_snapshot!(
|
||||
"skills_toggle_long_names_use_available_width",
|
||||
render_lines(&view, /*width*/ 96)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renders_long_names_at_narrow_width() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let view = SkillsToggleView::new(
|
||||
long_name_items(),
|
||||
tx,
|
||||
crate::keymap::RuntimeKeymap::defaults().list,
|
||||
);
|
||||
|
||||
assert_snapshot!(
|
||||
"skills_toggle_long_names_at_narrow_width",
|
||||
render_lines(&view, /*width*/ 48)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn footer_hint_uses_list_keymap_accept_and_cancel() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
|
||||
+14
@@ -0,0 +1,14 @@
|
||||
---
|
||||
source: tui/src/bottom_pane/skills_toggle_view.rs
|
||||
expression: "render_lines(&view, 48)"
|
||||
---
|
||||
|
||||
Enable/Disable Skills
|
||||
Turn skills on or off. Your changes are save
|
||||
|
||||
Type to search skills
|
||||
>
|
||||
› [x] superpowers-systematic-d… Find root cau…
|
||||
[ ] superpowers-verification… Verify comple…
|
||||
|
||||
Press space or enter to toggle; esc to close
|
||||
+14
@@ -0,0 +1,14 @@
|
||||
---
|
||||
source: tui/src/bottom_pane/skills_toggle_view.rs
|
||||
expression: "render_lines(&view, 96)"
|
||||
---
|
||||
|
||||
Enable/Disable Skills
|
||||
Turn skills on or off. Your changes are saved automatically.
|
||||
|
||||
Type to search skills
|
||||
>
|
||||
› [x] superpowers-systematic-debugging (polish) Find root causes before fixing bugs
|
||||
[ ] superpowers-verification-before-completion (polish) Verify completion before claiming …
|
||||
|
||||
Press space or enter to toggle; esc to close
|
||||
@@ -1,10 +1,6 @@
|
||||
use codex_core_skills::model::SkillMetadata;
|
||||
use codex_utils_fuzzy_match::fuzzy_match;
|
||||
|
||||
use crate::text_formatting::truncate_text;
|
||||
|
||||
pub(crate) const SKILL_NAME_TRUNCATE_LEN: usize = 21;
|
||||
|
||||
pub(crate) fn skill_display_name(skill: &SkillMetadata) -> String {
|
||||
if let Some(display_name) = skill
|
||||
.interface
|
||||
@@ -33,10 +29,6 @@ pub(crate) fn skill_description(skill: &SkillMetadata) -> &str {
|
||||
.unwrap_or(&skill.description)
|
||||
}
|
||||
|
||||
pub(crate) fn truncate_skill_name(name: &str) -> String {
|
||||
truncate_text(name, SKILL_NAME_TRUNCATE_LEN)
|
||||
}
|
||||
|
||||
pub(crate) fn match_skill(
|
||||
filter: &str,
|
||||
display_name: &str,
|
||||
|
||||
Reference in New Issue
Block a user