Fix the valid CodeRabbit findings on the list view

README.md

@@ -93,8 +93,8 @@ If a skill you ask for is already installed globally and not in the project, `in
 and asks before installing it into the project too (`-y` goes ahead).
 
 Scope for `list`, `uninstall`, `reset` and `prune`: a `PATH` (or `-p PATH`) means that project only,
-`-g` means global only, neither means both (project = cwd). `install` and `update` act on one
-project, or with `-g` on the user's installs; `use`, `run` and `profile` always act on one project. A project that is your home directory counts as global. A `list` PATH must
+`-g` means global only, neither means both (project = cwd). `install`, `update` and `use` act on one
+project, or with `-g` on the user's installs; `run` and `profile` always act on one project. A project that is your home directory counts as global. A `list` PATH must
 start with `.`, `..` or `/` (`./sub`, not `sub`); any other word is a query.
 
 ## From the list to the command line

src/app/rule.rs

@@ -1,14 +1,16 @@
 //! A horizontal rule with a label in the middle: `──────── global ────────`. The list's divider
 //! and the command line's group headings are both drawn with it.
 
+use unicode_width::UnicodeWidthStr;
+
 /// How wide the command line draws a rule: the width of a terminal that is not asked.
 pub const RULE_WIDTH: usize = 72;
 
 /// `label` between two runs of `rule`, `width` cells in all and the label centred. A label too
 /// wide for that stands alone.
 pub fn centered_rule(label: &str, width: usize, rule: &str) -> String {
     let label = format!(" {label} ");
-    let room = width.saturating_sub(label.chars().count());
+    let room = width.saturating_sub(label.width());
     let (left, right) = (room / 2, room - room / 2);
     format!("{}{label}{}", rule.repeat(left), rule.repeat(right))
 }
@@ -28,6 +30,13 @@ mod tests {
         assert!(line.contains(" global "));
     }
 
+    #[test]
+    fn a_label_of_wide_characters_is_measured_in_terminal_cells() {
+        let line = centered_rule("日本", 20, "─");
+
+        assert_eq!(line.width(), 20);
+    }
+
     #[test]
     fn a_label_wider_than_the_width_is_kept_whole() {
         assert_eq!(centered_rule("a-long-name", 4, "─"), " a-long-name ");

src/cli/doctor.rs

@@ -22,7 +22,7 @@ pub(super) fn doctor(deps: &Deps, args: &DoctorArgs, out: &mut dyn Write) -> any
     if args.json {
         return found.write_json(out);
     }
-    report(&found, &deps.home)
+    report(&found, &env.home)
         .iter()
         .try_for_each(|line| writeln!(out, "{line}"))?;
     Ok(())
@@ -210,4 +210,35 @@ mod tests {
                 .any(|l| l.contains(&home.path().display().to_string()))
         );
     }
+
+    #[test]
+    fn a_symlinked_home_is_still_shown_as_tilde() {
+        let base = tempfile::tempdir().unwrap();
+        let real = base.path().join("real");
+        let linked = base.path().join("home");
+        std::os::unix::fs::symlink(&real, &linked).unwrap();
+        let skills = real.join(".agents/skills");
+        skill(&skills.join("tdd"), "tdd");
+        skill(&skills.join("work/old"), "tdd");
+        let runner = crate::app::install::test_support::FakeRunner::new(|_| {});
+        let prompter = crate::app::install::test_support::ScriptedPrompter::new(&[], true);
+        let deps = Deps::for_test(
+            &linked,
+            skillz_harness::agents::registry(),
+            (&runner, &prompter),
+        );
+        let args = DoctorArgs {
+            project: None,
+            global: true,
+            json: false,
+        };
+        let mut out = Vec::new();
+
+        doctor(&deps, &args, &mut out).unwrap();
+
+        let text = String::from_utf8(out).unwrap();
+        let canonical = real.canonicalize().unwrap().display().to_string();
+        assert!(!text.contains(&canonical));
+        assert!(text.contains("~/.agents/skills/work/old"));
+    }
 }

src/cli/library/rows.rs

@@ -46,21 +46,21 @@ fn group_of(item: &LibraryItem) -> (u8, String) {
 
 /// The rows under a rule per package or folder, `~` for the home directory, one line per item.
 pub(in crate::cli) fn grouped(rows: &[Row], home: &Path, width: usize) -> Vec<String> {
-    let mut groups: BTreeMap<(u8, String), Vec<&Row>> = BTreeMap::new();
+    let mut groups: BTreeMap<(u8, String, String), Vec<&Row>> = BTreeMap::new();
     for row in rows {
         let (rank, title) = group_of(row.item);
         groups
-            .entry((rank, title.to_lowercase()))
+            .entry((rank, title.to_lowercase(), title))
             .or_default()
             .push(row);
     }
     let mut lines = Vec::new();
-    for (_, mut members) in groups {
+    for ((_, _, title), mut members) in groups {
         members.sort_by_key(|row| (row.item.kind(), row.item.short_label().to_lowercase()));
         if !lines.is_empty() {
             lines.push(String::new());
         }
-        let title = tilde(&redact(&group_of(members[0].item).1), home);
+        let title = tilde(&redact(&title), home);
         lines.push(centered_rule(&title, width, "─"));
         lines.extend(members.iter().map(|row| row.entry_line(home)));
     }
@@ -181,6 +181,18 @@ mod tests {
         assert!(lines[at_rule + 1].contains("grill") && lines[at_rule + 2].contains("tdd"));
     }
 
+    #[test]
+    fn sources_that_differ_only_in_case_stay_in_separate_groups() {
+        let (upper, lower) = (at("/home/me/Skills", "tdd"), at("/home/me/skills", "grill"));
+        let held = Held::of(&[], &[]);
+        let rows = rows(vec![&upper, &lower], (&held, &Disk));
+
+        let lines = grouped(&rows, Path::new("/nowhere"), 40);
+
+        let rules = lines.iter().filter(|l| l.starts_with('─')).count();
+        assert_eq!(rules, 2);
+    }
+
     #[test]
     fn json_keeps_the_paths_as_they_are() {
         let item = at("/home/me/code/skills", "tdd");

src/cli/session/change.rs

@@ -100,7 +100,6 @@ pub(in crate::cli) fn change(
         if switch.is_empty() && targets.as_ref().is_none_or(|t| t.agents.is_empty()) {
             return Ok(Some(Notice::UpToDate));
         }
-        let mut view = plan_view(&switch, job.scope);
         if let Some(chosen) = &mut targets {
             // The plan lists every item with a box; what is unchecked is not done.
             chosen.sync_items(item_lines(&switch));
@@ -109,6 +108,7 @@ pub(in crate::cli) fn change(
                 .retain(|(entry, _)| chosen.includes(&entry.to_string()));
             switch.removals.retain(|item| chosen.includes(&item.id));
         }
+        let mut view = plan_view(&switch, job.scope);
         view.targets.clone_from(&targets);
         view.heading = what.heading(job.scope);
         match deps.prompter.plan(&view)? {

src/cli/session/change/selection_tests.rs

@@ -259,3 +259,52 @@ fn an_item_the_user_unchecks_is_neither_installed_nor_removed() {
     assert_eq!(adds.len(), 1);
     assert!(dir.path().join(".agents/skills/tdd").exists());
 }
+
+/// Unchecks every removal in the first plan, notes whether each plan shown is destructive, then approves.
+struct UnchecksRemovals {
+    destructive: std::cell::RefCell<Vec<bool>>,
+}
+
+impl crate::app::prompt::Prompter for UnchecksRemovals {
+    fn multi_select(&self, _title: &str, _options: &[String]) -> anyhow::Result<Vec<usize>> {
+        Ok(vec![])
+    }
+
+    fn confirm(&self, _question: &str) -> anyhow::Result<bool> {
+        Ok(true)
+    }
+
+    fn plan(&self, view: &crate::app::review::PlanView) -> anyhow::Result<Verdict> {
+        self.destructive.borrow_mut().push(view.destructive());
+        if self.destructive.borrow().len() > 1 {
+            return Ok(Verdict::Apply);
+        }
+        let mut targets = view.targets.clone().expect("a selection offers its items");
+        for pick in &mut targets.items {
+            pick.on = pick.group != crate::app::review::Group::Remove;
+        }
+        Ok(Verdict::Retarget(targets))
+    }
+}
+
+#[test]
+fn a_plan_whose_removals_are_all_unchecked_is_no_longer_destructive() {
+    let dir = tempfile::tempdir().unwrap();
+    project_with_skill(dir.path(), "tdd");
+    let ids = [skill_id(dir.path())];
+    let entries = [skill_entry("grill")];
+    let runner = crate::app::install::test_support::FakeRunner::new(|_| {});
+    let prompter = UnchecksRemovals {
+        destructive: Default::default(),
+    };
+    let deps = Deps::for_test(dir.path(), registry(), (&runner, &prompter));
+
+    change(
+        &session(&deps, dir.path()),
+        Scope::Project,
+        &Change::Selection(&selection(&entries, &ids)),
+    )
+    .unwrap();
+
+    assert_eq!(*prompter.destructive.borrow(), [true, false]);
+}

src/tui/widgets/text_modal.rs

@@ -74,6 +74,9 @@ impl TextModal {
     }
 
     pub fn render(&mut self, frame: &mut Frame, theme: &Theme, page: Page) {
+        let before_reveal = page.reveal.map_or_else(Vec::new, |at| {
+            page.lines[..at.min(page.lines.len())].to_vec()
+        });
         let paragraph = Paragraph::new(page.lines).wrap(Wrap { trim: false });
         let width = WIDTH.min(frame.area().width.saturating_sub(2 * PAD_X));
         let lines = paragraph.line_count(width.saturating_sub(2));
@@ -97,8 +100,11 @@ impl TextModal {
         .areas(inner);
         self.lines = paragraph.line_count(body.width);
         self.page = usize::from(body.height);
-        if let Some(line) = page.reveal {
-            self.reveal(line);
+        if page.reveal.is_some() {
+            let row = Paragraph::new(before_reveal)
+                .wrap(Wrap { trim: false })
+                .line_count(body.width);
+            self.reveal(row);
         }
         self.clamp();
         let offset = u16::try_from(self.offset).unwrap_or(u16::MAX);
@@ -115,3 +121,48 @@ impl TextModal {
         self.offset = self.offset.min(self.lines.saturating_sub(self.page.max(1)));
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::tui::widgets::testing::render_to_buffer;
+
+    fn page(lines: Vec<Line<'static>>, reveal: usize) -> Page {
+        Page {
+            panel: Panel::new("t"),
+            lines,
+            buttons: None,
+            help: None,
+            reveal: Some(reveal),
+        }
+    }
+
+    #[test]
+    fn revealing_a_line_below_wrapped_ones_scrolls_to_its_wrapped_row() {
+        let long = "word ".repeat(60);
+        let mut lines: Vec<Line<'static>> = (0..4).map(|_| Line::from(long.clone())).collect();
+        lines.push(Line::from("target"));
+        let at = lines.len() - 1;
+        let mut modal = TextModal::default();
+
+        render_to_buffer(40, 12, |frame| {
+            modal.render(frame, &Theme::dark(), page(lines.clone(), at));
+        });
+
+        let row = modal.lines - 1;
+        assert!(row >= at);
+        assert!((modal.offset..modal.offset + modal.page).contains(&row));
+    }
+
+    #[test]
+    fn a_reveal_past_the_last_line_does_not_panic() {
+        let lines: Vec<Line<'static>> = (0..30).map(|n| Line::from(format!("line {n}"))).collect();
+        let mut modal = TextModal::default();
+
+        render_to_buffer(40, 12, |frame| {
+            modal.render(frame, &Theme::dark(), page(lines.clone(), 99));
+        });
+
+        assert_eq!(modal.offset + modal.page, modal.lines);
+    }
+}

src/tui/workspace/plugins/rows.rs

@@ -159,9 +159,19 @@ fn packages(items: &[&LibraryItem], installed: &[Installed], now: u64) -> Vec<Pl
         .iter()
         .flat_map(|row| row.installed.iter().map(String::as_str))
         .collect();
+    // A marketplace whose plugins are listed in other rows has no row of its own: flipping it
+    // would remove those plugins with it.
     let rest: Vec<&Installed> = installed
         .iter()
         .filter(|item| !claimed.contains(item.id.as_str()))
+        .filter(|item| {
+            item.kind != Kind::Marketplace
+                || !installed.iter().any(|plugin| {
+                    plugin.kind == Kind::Plugin
+                        && plugin.group == item.group
+                        && claimed.contains(plugin.id.as_str())
+                })
+        })
         .collect();
     for (package, items) in group_by(rest, |item| Package::of(item)) {
         if !listed(&package) {
@@ -198,13 +208,21 @@ fn packages(items: &[&LibraryItem], installed: &[Installed], now: u64) -> Vec<Pl
     rows
 }
 
-/// `held` with the marketplaces its plugins came from: a plugin is removed with its marketplace,
-/// which holds no key of its own to match an entry by.
+/// `held` with the marketplaces whose installed plugins `held` has all of: removing a marketplace
+/// removes every plugin installed from it, so it goes only with the last of them. It holds no key of
+/// its own to match an entry by.
 fn with_marketplaces<'a>(mut held: Vec<&'a Installed>, all: &'a [Installed]) -> Vec<&'a Installed> {
+    let held_ids: HashSet<&str> = held.iter().map(|item| item.id.as_str()).collect();
+    let remaining: HashSet<&str> = all
+        .iter()
+        .filter(|item| item.kind == Kind::Plugin && !held_ids.contains(item.id.as_str()))
+        .map(|item| item.group.as_str())
+        .collect();
     let groups: HashSet<&str> = held
         .iter()
         .filter(|item| item.kind == Kind::Plugin)
         .map(|item| item.group.as_str())
+        .filter(|group| !remaining.contains(group))
         .collect();
     held.extend(
         all.iter()

src/tui/workspace/plugins/tests.rs

@@ -755,3 +755,75 @@ fn flipping_a_package_installed_in_both_scopes_removes_only_the_projects_copy()
         })
     );
 }
+
+fn plugin_entry(source: &str, name: &str) -> Spec {
+    Spec::Plugin {
+        source: source.into(),
+        marketplace: "mp".into(),
+        name: name.into(),
+        agents: vec![],
+    }
+}
+
+#[test]
+fn removing_one_of_two_plugins_of_a_marketplace_keeps_the_marketplace_and_the_other_plugin() {
+    let mut tab = tab(Model {
+        library: library(vec![plugin_entry("a/one", "p"), plugin_entry("a/two", "q")]),
+        installed: vec![
+            installed("p@mp", Kind::Plugin, "mp", None),
+            installed("q@mp", Kind::Plugin, "mp", None),
+            installed("mp", Kind::Marketplace, "mp", None),
+        ],
+        ..Model::default()
+    });
+
+    let act = flip_and_apply(&mut tab);
+
+    assert_eq!(
+        act,
+        Some(Act::Change {
+            scope: Scope::Project,
+            install: vec![],
+            remove: vec!["plugin:p@mp".into()],
+        })
+    );
+}
+
+fn rows_removing(rows: &[Row], id: &str) -> usize {
+    rows.iter()
+        .filter(
+            |row| matches!(row, Row::Own(plugin) if plugin.installed.iter().any(|held| held == id)),
+        )
+        .count()
+}
+
+#[test]
+fn a_marketplace_whose_plugins_are_listed_in_other_rows_has_no_row_of_its_own() {
+    let model = Model {
+        library: library(vec![plugin_entry("a/one", "p"), plugin_entry("a/two", "q")]),
+        installed: vec![
+            installed("p@mp", Kind::Plugin, "mp", None),
+            installed("q@mp", Kind::Plugin, "mp", None),
+            installed("mp", Kind::Marketplace, "mp", None),
+        ],
+        ..Model::default()
+    };
+
+    let rows = super::rows::build(&model, Scope::Project);
+
+    let rows_removing_it = rows_removing(&rows, "marketplace:mp");
+    assert_eq!(rows_removing_it, 0);
+}
+
+#[test]
+fn a_marketplace_left_without_plugins_still_has_a_row_to_remove_it_from() {
+    let model = Model {
+        installed: vec![installed("mp", Kind::Marketplace, "mp", None)],
+        ..Model::default()
+    };
+
+    let rows = super::rows::build(&model, Scope::Project);
+
+    let rows_removing_it = rows_removing(&rows, "marketplace:mp");
+    assert_eq!(rows_removing_it, 1);
+}