From 60e4d10ad973dc1b5ec859dbae0e56ae2d5ec2fd Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Fri, 23 Oct 2026 23:54:13 +0000 Subject: [PATCH] Clarify skill toggle summaries in the TUI (#57197) ## What changed Omit zero-count actions and use singular or plural wording when summarizing skill changes. For example, show `Disabled 1 skill` instead of `0 skills enabled, 1 skills disabled`. ## Testing Add a snapshot test covering enable-only, disable-only, mixed, and unchanged skill states, including no summary when nothing changes. GitOrigin-RevId: ff68c799f29ceb8736e6941778d29a3f8d500b70 --- codex-rs/tui/src/chatwidget/skills.rs | 14 +++-- ...twidget__tests__manage_skills_summary.snap | 8 +++ codex-rs/tui/src/chatwidget/tests.rs | 2 + .../chatwidget/tests/skills_summary_tests.rs | 54 +++++++++++++++++++ 4 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__manage_skills_summary.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/skills_summary_tests.rs diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index f505fdde38..5c1350cc6c 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -130,10 +130,16 @@ impl ChatWidget { if enabled_count == 0 && disabled_count == 0 { return; } - self.add_info_message( - format!("{enabled_count} skills enabled, {disabled_count} skills disabled"), - /*hint*/ None, - ); + let enabled_suffix = if enabled_count == 1 { "" } else { "s" }; + let disabled_suffix = if disabled_count == 1 { "" } else { "s" }; + let summary = match (enabled_count, disabled_count) { + (0, _) => format!("Disabled {disabled_count} skill{disabled_suffix}"), + (_, 0) => format!("Enabled {enabled_count} skill{enabled_suffix}"), + _ => format!( + "Enabled {enabled_count} skill{enabled_suffix}, disabled {disabled_count} skill{disabled_suffix}" + ), + }; + self.add_info_message(summary, /*hint*/ None); } pub(crate) fn set_skills_from_response(&mut self, response: &SkillsListResponse) { diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__manage_skills_summary.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__manage_skills_summary.snap new file mode 100644 index 0000000000..d165835223 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__manage_skills_summary.snap @@ -0,0 +1,8 @@ +--- +source: tui/src/chatwidget/tests/skills_summary_tests.rs +expression: "summaries.join(\"\\n\")" +--- +enable: • Enabled 2 skills +disable: • Disabled 1 skill +mixed: • Enabled 2 skills, disabled 1 skill +no changes: diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index aae8906ab9..42f292b52b 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -289,6 +289,8 @@ mod review_picker_tests; #[path = "tests/session_model_selection_tests.rs"] mod session_model_selection_tests; mod side; +#[path = "tests/skills_summary_tests.rs"] +mod skills_summary_tests; mod slash_commands; #[path = "tests/sparkle_submission_tests.rs"] mod sparkle_submission_tests; diff --git a/codex-rs/tui/src/chatwidget/tests/skills_summary_tests.rs b/codex-rs/tui/src/chatwidget/tests/skills_summary_tests.rs new file mode 100644 index 0000000000..69d13123a4 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/skills_summary_tests.rs @@ -0,0 +1,54 @@ +#![cfg(test)] + +//! Snapshots change-count wording and silence when skill state is unchanged. + +use super::*; + +#[tokio::test] +async fn manage_skills_summary_snapshot() { + let (mut chat, mut rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + let initial = [false, false, true]; + chat.skills_all = initial + .iter() + .enumerate() + .map(|(index, &enabled)| SkillMetadata { + name: format!("skill-{index}"), + description: "Summary fixture".to_string(), + short_description: None, + interface: None, + dependencies: None, + path: test_path_buf(&format!("/tmp/skills/skill-{index}/SKILL.md")) + .abs() + .into(), + scope: crate::test_support::skill_scope_user(), + enabled, + plugin_id: None, + }) + .collect(); + drain_insert_history(&mut rx); + let mut summaries = Vec::new(); + for (name, current) in [ + ("enable", [true, true, true]), + ("disable", [false, false, false]), + ("mixed", [true, true, false]), + ("no changes", initial), + ] { + chat.skills_initial_state = Some( + chat.skills_all + .iter() + .zip(initial) + .map(|(skill, enabled)| (skill.path.to_inferred_path_uri().unwrap(), enabled)) + .collect(), + ); + for (skill, enabled) in chat.skills_all.iter_mut().zip(current) { + skill.enabled = enabled; + } + chat.handle_manage_skills_closed(); + let lines = drain_insert_history(&mut rx).concat(); + summaries.push(format!( + "{name}: {}", + lines_to_single_string(&lines).trim_end() + )); + } + assert_chatwidget_snapshot!("manage_skills_summary", summaries.join("\n")); +}