diff --git a/crates/dv/src/automation/state.rs b/crates/dv/src/automation/state.rs index 34ddcd8..d837f62 100644 --- a/crates/dv/src/automation/state.rs +++ b/crates/dv/src/automation/state.rs @@ -84,7 +84,7 @@ fn badge(badge: Option<&ReviewBadge>) -> Value { /// `state.switcher`. pub fn switcher(shell: &Shell, cx: &App) -> Value { let Some(switcher) = shell.switcher() else { - return json!({"open": false, "mode": null, "query": null, "selected": null, "items": []}); + return json!({"open": false, "mode": null, "query": null, "selected": null, "items": [], "menu": null}); }; let switcher = switcher.read(cx); let listing = switcher.listing(); @@ -108,12 +108,19 @@ pub fn switcher(shell: &Shell, cx: &App) -> Value { }) }) .collect(); + let menu = switcher.menu().map(|menu| { + json!({ + "repo": menu.location.display_name(), + "removable": switcher.can_remove(&menu.location), + }) + }); json!({ "open": true, "mode": switcher.mode().key(), "query": switcher.query(), "selected": switcher.selected_key(), "items": items, + "menu": menu, }) } diff --git a/crates/dv/src/automation/tests.rs b/crates/dv/src/automation/tests.rs index df28027..ccb17ff 100644 --- a/crates/dv/src/automation/tests.rs +++ b/crates/dv/src/automation/tests.rs @@ -228,7 +228,7 @@ fn state_describes_the_empty_shell(cx: &mut TestAppContext) { assert_eq!(state["backends"], json!([])); assert_eq!( state["switcher"], - json!({"open": false, "mode": null, "query": null, "selected": null, "items": []}) + json!({"open": false, "mode": null, "query": null, "selected": null, "items": [], "menu": null}) ); assert_eq!( state["source_picker"], diff --git a/crates/dv_ui/src/actions.rs b/crates/dv_ui/src/actions.rs index ea15a34..64356d8 100644 --- a/crates/dv_ui/src/actions.rs +++ b/crates/dv_ui/src/actions.rs @@ -112,6 +112,8 @@ pub mod switcher { SelectPrevious, /// Open what the selected row stands for (`Enter`). Confirm, + /// Remove the selected row's repository from the list (`Ctrl Shift Backspace`). + RemoveRepo, /// Close without opening anything (`Esc`). Dismiss, ] diff --git a/crates/dv_ui/src/keymap.rs b/crates/dv_ui/src/keymap.rs index 71d6dc3..7e53884 100644 --- a/crates/dv_ui/src/keymap.rs +++ b/crates/dv_ui/src/keymap.rs @@ -150,6 +150,14 @@ pub const SHORTCUTS: &[Shortcut] = &[ shortcut!("down", SWITCHER, switcher::SelectNext, AheadOfInputs), shortcut!("up", SWITCHER, switcher::SelectPrevious, AheadOfInputs), shortcut!("enter", SWITCHER, switcher::Confirm), + // The filter field deletes text with Backspace and its Ctrl, Alt and Shift forms, never + // with Ctrl Shift Backspace, which the field would otherwise swallow. + shortcut!( + "secondary-shift-backspace", + SWITCHER, + switcher::RemoveRepo, + AheadOfInputs + ), shortcut!("escape", SWITCHER, switcher::Dismiss), // The file finder: as the switcher, its filter field has the keyboard. shortcut!("down", FILE_FINDER, file_finder::SelectNext, AheadOfInputs), diff --git a/crates/dv_ui/src/services/reviews.rs b/crates/dv_ui/src/services/reviews.rs index 460ab1a..1e875af 100644 --- a/crates/dv_ui/src/services/reviews.rs +++ b/crates/dv_ui/src/services/reviews.rs @@ -755,6 +755,22 @@ impl Reviews { location } + /// Forgets the known repository at `location` ("Remove from list", DESIGN §4.1): its row + /// and any refresh of it go, and `known_repos.json` is saved without it. Nothing in the + /// repository changes, its reviews included; opening it again adds it back with them. A + /// refresh still running for it finds no row and is dropped. `false` when it wasn't known. + pub fn remove_repo(&mut self, location: &Location, cx: &mut Context) -> bool { + if !self.known.remove(location) { + return false; + } + self.rows.remove(location); + self.refreshing.remove(location); + self.save(cx); + cx.emit(ReviewsEvent::Changed); + cx.notify(); + true + } + /// A listing or an open of `given` reached the repository whose worktree root is `root` /// (`RepoInfo::location`, its distro spelled WSL's way): every known location that names /// it otherwise — `given` (a subfolder, another spelling of the distro) and any other diff --git a/crates/dv_ui/src/services/reviews/tests.rs b/crates/dv_ui/src/services/reviews/tests.rs index 721c59b..c38356e 100644 --- a/crates/dv_ui/src/services/reviews/tests.rs +++ b/crates/dv_ui/src/services/reviews/tests.rs @@ -875,3 +875,73 @@ fn a_known_repo_spelled_otherwise_becomes_the_spelling_wsl_resolves(cx: &mut Tes }); assert_eq!(reviews.read_with(cx, |reviews, _| reviews.rows().len()), 1); } + +#[gpui::test] +async fn a_removed_repo_leaves_the_list_and_the_file_keeping_its_reviews(cx: &mut TestAppContext) { + let data = tempfile::tempdir().unwrap(); + let repo = fixture(); + let other = fixture(); + let (locations, reviews) = services(cx, data.path()); + locations.update(cx, |locations, _| { + locations.register_env(repo.path().to_path_buf(), repo.env_overlay()); + locations.register_env(other.path().to_path_buf(), other.env_overlay()); + }); + let location = Location::Local(repo.path().to_path_buf()); + let kept = Location::Local(other.path().to_path_buf()); + for added in [&location, &kept] { + reviews + .update(cx, |reviews, cx| reviews.add_repo(added.clone(), cx)) + .await + .expect("a repo"); + } + let opened = reviews + .update(cx, |reviews, cx| { + reviews.open_review(location.clone(), Target::Working, cx) + }) + .await + .expect("a review"); + settle(cx, &reviews); + + // Removed while a refresh of it runs: the row goes at once, and the refresh lands nowhere. + let removed = reviews.update(cx, |reviews, cx| { + reviews.refresh(location.clone(), cx); + reviews.remove_repo(&location, cx) + }); + assert!(removed); + settle(cx, &reviews); + reviews.read_with(cx, |reviews, _| { + assert!(reviews.row(&location).is_none()); + assert!(reviews.known().get(&location).is_none()); + assert_eq!(reviews.rows().len(), 1); + assert_eq!(reviews.rows()[0].location, kept); + }); + let saved: Value = + serde_json::from_str(&std::fs::read_to_string(data.path().join(KNOWN_REPOS_FILE)).unwrap()) + .unwrap(); + assert_eq!( + saved["repos"].as_array().unwrap().len(), + 1, + "saved without it" + ); + assert_eq!( + saved["repos"][0]["location"], + serde_json::to_value(&kept).unwrap() + ); + // Not known any more, so nothing to remove. + assert!(!reviews.update(cx, |reviews, cx| reviews.remove_repo(&location, cx))); + + // Added again, it lists its review: nothing in the repository was touched. + reviews + .update(cx, |reviews, cx| reviews.add_repo(location.clone(), cx)) + .await + .expect("added again"); + settle(cx, &reviews); + reviews.read_with(cx, |reviews, _| { + let row = reviews.row(&location).expect("listed again"); + assert_eq!( + row.open_review_of(&Target::Working) + .map(|summary| &summary.id), + Some(&opened.loaded.review.id) + ); + }); +} diff --git a/crates/dv_ui/src/shell/mod.rs b/crates/dv_ui/src/shell/mod.rs index 5bdf948..b4efac2 100644 --- a/crates/dv_ui/src/shell/mod.rs +++ b/crates/dv_ui/src/shell/mod.rs @@ -841,7 +841,11 @@ impl Shell { .session .as_ref() .map(|session| session.read(cx).review_id().clone()); - let view = cx.new(|cx| Switcher::new(reviews, focus, current.as_ref(), window, cx)); + let in_use = self + .session + .as_ref() + .map(|session| session.read(cx).origin().clone()); + let view = cx.new(|cx| Switcher::new(reviews, focus, current.as_ref(), in_use, window, cx)); let subscription = cx.subscribe_in(&view, window, Self::on_switcher_event); view.update(cx, |switcher, cx| switcher.focus(window, cx)); self.switcher = Some(Overlay { @@ -1162,6 +1166,10 @@ impl Shell { .update(cx, |reviews, cx| reviews.start(location.clone(), cx)) .detach(); } + SwitcherEvent::Remove { location } => { + let reviews = cx.services().reviews.clone(); + reviews.update(cx, |reviews, cx| reviews.remove_repo(location, cx)); + } SwitcherEvent::Dismiss => { self.close_menu(window, cx); } diff --git a/crates/dv_ui/src/switcher/items.rs b/crates/dv_ui/src/switcher/items.rs index dcf2b41..d3cb6a1 100644 --- a/crates/dv_ui/src/switcher/items.rs +++ b/crates/dv_ui/src/switcher/items.rs @@ -300,12 +300,26 @@ impl Listing { Some((location, request)) } - /// The repository of `item`. + /// The repository of `item`; `None` for "Open folder…". pub fn location_of(&self, item: usize) -> Option<&Location> { let item = self.items.get(item)?; Some(&self.repos.get(item.repo?)?.location) } + /// The first row of the repository listed after `location`'s, else of the one before it, + /// else "Open folder…": where the selection goes when `location` is removed. + pub fn row_after_removing(&self, location: &Location) -> Option<&str> { + let repo = self + .repos + .iter() + .position(|repo| &repo.location == location)?; + let first_of = |repo: usize| self.items.iter().find(|item| item.repo == Some(repo)); + first_of(repo + 1) + .or_else(|| repo.checked_sub(1).and_then(first_of)) + .or_else(|| self.items.last()) + .map(|item| item.key.as_str()) + } + /// The repository picking `item` connects to, when it is a **Start** or **Retry** row /// (whose start doesn't run already). pub fn start(&self, item: usize) -> Option { diff --git a/crates/dv_ui/src/switcher/mod.rs b/crates/dv_ui/src/switcher/mod.rs index eef27a6..90ed0fc 100644 --- a/crates/dv_ui/src/switcher/mod.rs +++ b/crates/dv_ui/src/switcher/mod.rs @@ -18,10 +18,18 @@ //! opening or resuming its `branch` review. There `Esc` steps back (bases → branches → the //! repositories, when the picker was entered from them) before it closes. //! +//! A repository's header has a menu (a right-click, or its `⋯` under the pointer) with **Remove +//! from list**, which `Ctrl Shift Backspace` does for the selected row's repository: the +//! repository leaves the known ones (`Reviews::remove_repo`), the selection moving to the next +//! one's first row. The open review's repository can't be removed (its menu item is disabled, +//! the key does nothing): the title bar's source picker lists its targets. The branch picker +//! has neither the menu nor the key. +//! //! [`source`] is the title bar's other menu, the current repository's targets. //! -//! Automation targets: `switcher`, `switcher/filter`, `switcher/repo/`, `switcher/` -//! for each row ([`items::Item::key`]). +//! Automation targets: `switcher`, `switcher/filter`, `switcher/repo/`, +//! `switcher/repo//more` (the header's `⋯`), `switcher/menu`, `switcher/menu/remove`, +//! `switcher/` for each row ([`items::Item::key`]). pub mod branches; pub mod items; @@ -39,7 +47,7 @@ use gpui::{ }; use gpui_component::input::{InputEvent, InputState}; -use crate::actions::switcher::{Confirm, Dismiss, SelectNext, SelectPrevious}; +use crate::actions::switcher::{Confirm, Dismiss, RemoveRepo, SelectNext, SelectPrevious}; use crate::services::reviews::{OpenRequest, Reviews}; use branches::Mode; use items::{ItemKind, Listing}; @@ -62,10 +70,19 @@ pub enum SwitcherEvent { /// **Start** on the repository at `location`, whose WSL distro isn't running, or **Retry** /// on one whose connection failed: both connect explicitly (`Reviews::start`). Start { location: Location }, + /// **Remove from list** on the repository at `location` (`Reviews::remove_repo`). + Remove { location: Location }, /// Closed without picking (`Esc`). Dismiss, } +/// A repository header's menu: whose, and where it opened. +#[derive(Clone, Debug, PartialEq)] +pub struct RepoMenu { + pub location: Location, + pub position: gpui::Point, +} + /// The switcher (see the module docs). pub struct Switcher { reviews: Entity, @@ -81,6 +98,12 @@ pub struct Switcher { /// The repository a **Start** or **Retry** was picked on: its first row takes the /// selection once it lists one. started: Option, + /// The open review's repository (the one it was opened from), which can't be removed. + in_use: Option, + /// A repository header's menu, while open. + menu: Option, + /// The row a removal moves the selection to, once the listing drops the repository. + after_remove: Option, scroll: ScrollHandle, _subscriptions: Vec, } @@ -90,10 +113,12 @@ impl EventEmitter for Switcher {} impl Switcher { /// A switcher over the known repositories, its selection on `focus`'s first row (the /// repository `dv open` named), else on the open review `current`, else on the first row. + /// `in_use` is the open review's repository, which it won't remove. pub fn new( reviews: Entity, focus: Option<&Location>, current: Option<&ReviewId>, + in_use: Option, window: &mut Window, cx: &mut Context, ) -> Self { @@ -117,6 +142,9 @@ impl Switcher { listing: Listing::default(), selected: 0, started: None, + in_use, + menu: None, + after_remove: None, scroll: ScrollHandle::new(), _subscriptions: subscriptions, }; @@ -173,6 +201,7 @@ impl Switcher { cx: &mut Context, ) { self.mode = mode; + self.menu = None; self.query.clear(); let placeholder = self.mode.placeholder(); self.input.update(cx, |input, cx| { @@ -195,6 +224,70 @@ impl Switcher { .map(|item| item.key.as_str()) } + /// The open header menu. + pub fn menu(&self) -> Option<&RepoMenu> { + self.menu.as_ref() + } + + /// Whether the repository at `location` can be removed: it isn't the open review's. + pub fn can_remove(&self, location: &Location) -> bool { + self.in_use.as_ref() != Some(location) + } + + /// The repository `Ctrl Shift Backspace` would remove: the selected row's, when it can be + /// and the switcher lists the repositories (not the branch picker). + pub fn removable(&self) -> Option<&Location> { + if self.mode != Mode::Repos { + return None; + } + self.listing + .location_of(self.selected) + .filter(|location| self.can_remove(location)) + } + + /// Opens the header menu of the repository at `location`, at `position`. + pub fn open_menu( + &mut self, + location: Location, + position: gpui::Point, + cx: &mut Context, + ) { + if self.mode != Mode::Repos { + return; + } + self.menu = Some(RepoMenu { location, position }); + cx.notify(); + } + + pub fn close_menu(&mut self, cx: &mut Context) { + if self.menu.take().is_some() { + cx.notify(); + } + } + + /// Removes the repository at `location` from the list, when it can be (module docs). + pub fn remove(&mut self, location: &Location, cx: &mut Context) { + self.menu = None; + if !self.can_remove(location) { + cx.notify(); + return; + } + self.after_remove = self + .listing + .row_after_removing(location) + .map(str::to_string); + cx.emit(SwitcherEvent::Remove { + location: location.clone(), + }); + cx.notify(); + } + + fn remove_selected(&mut self, _: &RemoveRepo, _: &mut Window, cx: &mut Context) { + if let Some(location) = self.removable().cloned() { + self.remove(&location, cx); + } + } + /// Selects the row `key`; `false` when it isn't listed. pub fn select_key(&mut self, key: &str, cx: &mut Context) -> bool { match self.listing.position(key) { @@ -237,7 +330,12 @@ impl Switcher { if started.is_some() { self.started = None; } + let removed_to = self + .after_remove + .take() + .and_then(|key| self.listing.position(&key)); self.selected = started + .or(removed_to) .or_else(|| keep.and_then(|key| self.listing.position(&key))) .unwrap_or(0); self.scroll_to_selected(); @@ -286,8 +384,13 @@ impl Switcher { self.pick(self.selected, window, cx); } - /// `Esc`: a step back in the branch picker (module docs), else closes. + /// `Esc`: closes the header menu when it is open, else a step back in the branch picker + /// (module docs), else closes. fn dismiss(&mut self, _: &Dismiss, window: &mut Window, cx: &mut Context) { + if self.menu.take().is_some() { + cx.notify(); + return; + } let repo_key = self.listing.repos.first().map(|repo| repo.key.clone()); match self.mode.clone() { Mode::Bases { location, branch } => { diff --git a/crates/dv_ui/src/switcher/render.rs b/crates/dv_ui/src/switcher/render.rs index 599fb79..1b9f604 100644 --- a/crates/dv_ui/src/switcher/render.rs +++ b/crates/dv_ui/src/switcher/render.rs @@ -3,17 +3,22 @@ //! keys. use gpui::{ - AnyElement, Context, ElementId, FontWeight, InteractiveElement as _, IntoElement, - ParentElement as _, Render, SharedString, StatefulInteractiveElement as _, Styled as _, Window, - div, prelude::FluentBuilder as _, px, + AnyElement, Context, ElementId, FontWeight, InteractiveElement as _, IntoElement, MouseButton, + MouseDownEvent, ParentElement as _, Render, SharedString, StatefulInteractiveElement as _, + Styled as _, Window, anchored, deferred, div, prelude::FluentBuilder as _, px, }; use gpui_component::input::Input; use super::branches::Mode; use super::items::{Entry, Item, ItemKind, RepoHeader, RepoSummary, TargetKind}; -use super::{SWITCHER_WIDTH, Switcher}; +use super::{RepoMenu, SWITCHER_WIDTH, Switcher}; + +/// The header menu's layer: above the switcher, itself at the shell's menu priority (4). +const REPO_MENU_PRIORITY: usize = 5; use crate::automation::AutomationTargetExt as _; -use crate::components::{count_badge, icon, kbd, location_chip, skeleton, status_chip, totals}; +use crate::components::{ + count_badge, icon, kbd, location_chip, menu_item, menu_panel, skeleton, status_chip, totals, +}; use crate::icons::DvIcon; use crate::keymap; use crate::theme::{ActiveTheme as _, DvTheme, StyledText as _}; @@ -30,6 +35,7 @@ impl Render for Switcher { .on_action(cx.listener(Self::select_previous)) .on_action(cx.listener(Self::confirm)) .on_action(cx.listener(Self::dismiss)) + .on_action(cx.listener(Self::remove_selected)) .w(SWITCHER_WIDTH) .max_h(max_height) .flex() @@ -46,6 +52,11 @@ impl Render for Switcher { .child(self.render_filter(&theme)) .child(self.render_list(&theme, cx)) .child(self.render_footer(&theme, cx)) + .children( + self.menu + .clone() + .map(|menu| self.render_menu(menu, &theme, cx)), + ) .automation_target("switcher") } } @@ -94,7 +105,7 @@ impl Switcher { format!("No branches match “{}”", self.query.trim()) }; return list - .child(repo_header(&self.listing.repos[0], true, theme)) + .child(repo_header(&self.listing.repos[0], true, None, theme)) .child( div() .flex() @@ -146,7 +157,7 @@ impl Switcher { .listing .repos .get(repo) - .map(|header| repo_header(header, line == 0, theme)), + .map(|header| self.repo_header(header, line == 0, theme, cx)), Entry::Item(ix) => self .listing .items @@ -304,6 +315,9 @@ impl Switcher { .pr(theme.spacing.sm) .text_spec(&theme.text.secondary) .text_color(c.fg3) + .when(self.removable().is_some(), |this| { + this.child(hint(vec!["secondary-shift-backspace"], "remove")) + }) .child(hint(vec!["up", "down"], "move")) .child(hint( vec!["enter"], @@ -324,11 +338,132 @@ impl Switcher { ) .into_any_element() } + + /// A repository header's menu at the pointer: **Remove from list**, disabled for the open + /// review's repository with the reason under it. + fn render_menu(&self, menu: RepoMenu, theme: &DvTheme, cx: &mut Context) -> AnyElement { + let c = &theme.colors; + let removable = self.can_remove(&menu.location); + let location = menu.location.clone(); + let remove = menu_item( + "switcher-menu-remove", + "Remove from list", + DvIcon::X, + removable, + theme, + ) + .when(removable, |this| { + this.on_click(cx.listener(move |this, _, _, cx| this.remove(&location, cx))) + }) + .automation_target("switcher/menu/remove"); + let panel = menu_panel("switcher-menu", theme) + .on_mouse_down_out(cx.listener(|this, _, _, cx| this.close_menu(cx))) + .child(remove) + .when(!removable, |this| { + this.child( + div() + .px(theme.spacing.md) + .pb(theme.spacing.xs) + .text_spec(&theme.text.secondary) + .text_color(c.fg3) + .child("Its review is open"), + ) + }) + .automation_target("switcher/menu"); + deferred( + anchored() + .position(menu.position) + .snap_to_window_with_margin(px(8.)) + .child(panel), + ) + .with_priority(REPO_MENU_PRIORITY) + .into_any_element() + } + + /// A repository's header: its name, its path dimmed, its location chip, and a line on its + /// uncommitted changes or why it can't be opened; a right-click, or its `⋯` under the + /// pointer, opens its menu. + fn repo_header( + &self, + header: &RepoHeader, + first: bool, + theme: &DvTheme, + cx: &mut Context, + ) -> AnyElement { + // The branch picker's header has no menu (module docs). + if self.mode != Mode::Repos { + return repo_header(header, first, None, theme); + } + let group = SharedString::from(format!("switcher-repo-{}", header.key)); + let menu_open = self + .menu + .as_ref() + .is_some_and(|menu| menu.location == header.location); + let c = &theme.colors; + let hover = c.control_hover; + let more = div() + .id(ElementId::Name( + format!("switcher-more-{}", header.key).into(), + )) + .flex_none() + .size(px(22.)) + .flex() + .items_center() + .justify_center() + .rounded(theme.radii.control) + .cursor_pointer() + .hover(move |style| style.bg(hover)) + .when(!menu_open, |this| { + this.opacity(0.) + .group_hover(group.clone(), |style| style.opacity(1.)) + }) + .when(menu_open, |this| this.bg(hover)) + .child(icon(DvIcon::Ellipsis, px(14.), c.fg2)) + .on_mouse_down(MouseButton::Left, { + let location = header.location.clone(); + cx.listener(move |this, event: &MouseDownEvent, _, cx| { + cx.stop_propagation(); + if this + .menu + .as_ref() + .is_some_and(|menu| menu.location == location) + { + this.close_menu(cx); + } else { + this.open_menu(location.clone(), event.position, cx); + } + }) + }) + .automation_target(format!("switcher/repo/{}/more", header.key)); + let location = header.location.clone(); + div() + .id(ElementId::Name(group.clone())) + .group(group) + .child(repo_header( + header, + first, + Some(more.into_any_element()), + theme, + )) + .on_mouse_down( + MouseButton::Right, + cx.listener(move |this, event: &MouseDownEvent, _, cx| { + cx.stop_propagation(); + this.open_menu(location.clone(), event.position, cx); + }), + ) + .into_any_element() + } } -/// A repository's header: its name, its path dimmed, its location chip, and a line on its -/// uncommitted changes or why it can't be opened. -fn repo_header(header: &RepoHeader, first: bool, theme: &DvTheme) -> AnyElement { +/// A repository's header: its name, its path dimmed, its location chip, `more` at the end of +/// its first line, and a line on its uncommitted changes or why it can't be opened. +fn repo_header( + header: &RepoHeader, + first: bool, + more: Option, + theme: &DvTheme, +) -> AnyElement { let c = &theme.colors; let summary: AnyElement = match &header.summary { RepoSummary::Loading => skeleton(px(160.), px(10.), theme), @@ -406,7 +541,8 @@ fn repo_header(header: &RepoHeader, first: bool, theme: &DvTheme) -> AnyElement .text_spec(&theme.text.secondary) .text_color(c.fg3) .child(SharedString::from(header.shown_path.clone())), - ), + ) + .children(more), ) .child( div() diff --git a/crates/dv_ui/src/switcher/tests.rs b/crates/dv_ui/src/switcher/tests.rs index 2770d68..88b91e2 100644 --- a/crates/dv_ui/src/switcher/tests.rs +++ b/crates/dv_ui/src/switcher/tests.rs @@ -965,6 +965,13 @@ fn the_branch_picker_opens_a_stacked_branch_against_its_base(cx: &mut TestAppCon selected(&shell, cx).as_deref(), Some("api-server/pick/feat-b") ); + // The branch picker removes nothing: Ctrl Shift Backspace leaves the repository listed. + cx.simulate_keystrokes("secondary-shift-backspace"); + assert!(view.read_with(cx, |view, _| view.removable().is_none())); + shell.read_with(cx, |_, cx| { + use crate::services::ActiveServices as _; + assert_eq!(cx.services().reviews.read(cx).rows().len(), 1); + }); // Enter lists its bases, the stack base first; Esc steps back to the branches, and again // to the repositories, where "Compare branches…" keeps the selection. @@ -1055,3 +1062,114 @@ fn the_source_picker_opens_the_branch_picker_and_esc_closes_it(cx: &mut TestAppC cx.simulate_keystrokes("escape"); assert!(switcher(&shell, cx).is_none()); } + +#[gpui::test] +fn a_repo_is_removed_by_key_or_menu_but_not_the_open_reviews(cx: &mut TestAppContext) { + use crate::services::ActiveServices as _; + let (_dir, shell, cx) = open_shell(cx); + let repos = tempfile::tempdir().unwrap(); + let api = fixture_at(&repos.path().join("api-server")); + let web = fixture_at(&repos.path().join("web")); + let docs = fixture_at(&repos.path().join("docs")); + for repo in [&api, &web, &docs] { + add_repo(repo, &shell, cx); + } + let api_location = Location::Local(api.path().to_path_buf()); + let task = cx.update(|window, cx| { + shell.update(cx, |shell, cx| { + shell.open( + api_location.clone(), + OpenRequest::Target(Target::Working), + window, + cx, + ) + }) + }); + cx.run_until_parked(); + settle(cx, &shell, "the review", |shell, _| { + shell.session().is_some() + }); + cx.foreground_executor().block_test(task).unwrap(); + let known = |shell: &Entity, cx: &mut VisualTestContext| { + shell.read_with(cx, |_, cx| { + let mut names: Vec = cx + .services() + .reviews + .read(cx) + .rows() + .iter() + .map(|row| row.name.clone()) + .collect(); + names.sort(); + names + }) + }; + + cx.simulate_keystrokes("secondary-o"); + settle(cx, &shell, "the rows", |_, _| true); + let view = switcher(&shell, cx).expect("Ctrl O opens the switcher"); + // The open review's repository: the key does nothing, and its menu's item is disabled. + assert_eq!(selected(&shell, cx).as_deref(), Some("api-server/working")); + cx.simulate_keystrokes("secondary-shift-backspace"); + assert_eq!(known(&shell, cx), ["api-server", "docs", "web"]); + view.update(cx, |view, cx| { + assert_eq!(view.removable(), None); + view.open_menu( + api_location.clone(), + gpui::point(gpui::px(10.), gpui::px(10.)), + cx, + ); + view.remove(&api_location, cx); + assert!(view.menu().is_none(), "the menu closes"); + }); + cx.run_until_parked(); + assert_eq!(known(&shell, cx), ["api-server", "docs", "web"]); + + // From the filter field, Ctrl Shift Backspace removes the selected row's repository; the + // selection moves to the next repository's first row. + let order: Vec = view.read_with(cx, |view, _| { + view.listing() + .repos + .iter() + .map(|repo| repo.key.clone()) + .collect() + }); + let (middle, after) = (order[1].clone(), order[2].clone()); + assert!(view.update(cx, |view, cx| { + view.select_key(&format!("{middle}/staged"), cx) + })); + assert!(cx.update(|window, cx| view.read(cx).is_focused(window, cx))); + cx.simulate_keystrokes("secondary-shift-backspace"); + settle(cx, &shell, "the removal", |_, _| true); + let mut left = vec!["api-server".to_string(), after.clone()]; + left.sort(); + assert_eq!(known(&shell, cx), left); + assert_eq!(selected(&shell, cx), Some(format!("{after}/working"))); + + // The menu's Remove from list does the same; Esc closes an open menu first. + let after_location = view.read_with(cx, |view, _| { + view.listing() + .repos + .iter() + .find(|repo| repo.key == after) + .unwrap() + .location + .clone() + }); + view.update(cx, |view, cx| { + view.open_menu( + after_location.clone(), + gpui::point(gpui::px(10.), gpui::px(10.)), + cx, + ) + }); + cx.simulate_keystrokes("escape"); + assert!(view.read_with(cx, |view, _| view.menu().is_none())); + assert!( + switcher(&shell, cx).is_some(), + "Esc closed the menu, not the switcher" + ); + view.update(cx, |view, cx| view.remove(&after_location, cx)); + settle(cx, &shell, "the removal", |_, _| true); + assert_eq!(known(&shell, cx), ["api-server"]); +} diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 2026c30..061f172 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -203,7 +203,18 @@ inactive: material falls back to its inactive appearance (OS-managed); dv dims n beside its open review (status chip, "round n", open-comment badge; "New review" on the selected row without one), then the repo's other open reviews by their target label, then, when it has a branch other than the default one, **Compare branches…** (arrows glyph). Picking a row opens or - resumes that review (PLAN §2.4). A repo that can't be opened offers no rows. + resumes that review (PLAN §2.4). A repo that can't be opened offers no rows. A header's menu + (a right-click on it, or its `⋯`, a 22-square ghost button at the end of its first line shown + under the pointer and while its menu is open; the menu panel at the pointer, as the comments + row menu's) holds **Remove from list** (x glyph): the repo leaves the switcher and + `known_repos.json`, nothing in it changing (its reviews show again once it is opened again), and + the selection moves to the next repo's first row, else the previous one's, else **Open + folder…**. For the open review's repo the item is disabled, "Its review is open" in `fg3` under + it. `Ctrl Shift Backspace` removes the selected row's repo the same way, from the filter field + too (which deletes text with Backspace's other forms), and does nothing on the open review's; + the footer's hints start with it ("remove") while the selected row's repo can be removed. `Esc` + closes an open menu before the switcher. The branch picker (below) has neither the menu nor the + key. **Compare branches…** turns the switcher into the branch picker for that repo, in the same panel: the field cleared, its placeholder "Pick a branch to review", the repo's header over one row per local branch but the default one (branch glyph, the most recently committed first): the name, then @@ -878,6 +889,7 @@ Setup unchanged. | `Ctrl Enter` | finish review / submit: the "Finish round" sheet, while no text field has the keyboard (in the composer: post comment) | | `Ctrl K` | the palette: commands, reviews and, as you type, the review's files (§4.1; again, or `Esc`, closes it) | | `Ctrl O` | switch repo / review (the switcher; again, or `Esc`, closes it) | +| `Ctrl Shift Backspace` | in the switcher: remove the selected row's repo from the list (not the open review's, §4.1) | | `Ctrl G` | the PR picker (§4.5) | | `Ctrl R` | on a PR review: refresh it from GitHub (fetch, threads, checks, Viewed; PLAN §9.4) | | `Ctrl B` | toggle panel | diff --git a/docs/PLAN.md b/docs/PLAN.md index 0c05bc8..df1bd24 100644 --- a/docs/PLAN.md +++ b/docs/PLAN.md @@ -2083,7 +2083,9 @@ at all and the worktree root its reviews name; a known repo is a worktree root, spelled WSL's way, and one added before it could be resolved (a subfolder added while its connection was down, so its row could say why) or under another spelling of its distro becomes that root, row and `known_repos.json` re-keyed (or dropped when the root is known already), the -first time a listing or an open resolves it; repos compare with the distro folded +first time a listing or an open resolves it; "Remove from list" (`remove_repo`, DESIGN §4.1) forgets +one, its row, any refresh of it and its entry in `known_repos.json` going, nothing in the repo +touched, and never the open review's, whose targets the source picker lists; repos compare with the distro folded (`Location::same_place`), so a spelling WSL couldn't confirm never makes a second row; so the switcher shows each target beside its open review by the rule `open_review` resumes by (`review::ops::is_review_of`); it watches nothing itself), `ReviewUi` (what the reviewer chose per review: split or unified), `GitHub` (gh calls, the diff --git a/tests/e2e/README.md b/tests/e2e/README.md index 25e5273..f32be46 100644 --- a/tests/e2e/README.md +++ b/tests/e2e/README.md @@ -226,6 +226,7 @@ Ubuntu, from the Linux GUI target dir (`~/.cache/dv-target-gui`), whose | `setup` | Windows only. Setup (PLAN §12, DESIGN §4.3) on a fresh data dir with `DV_FIRST_RUN=1` and `mock_gh` signed in (`tests/fixtures/gh/auth_status.json`): the first-run screen over this machine's checks; a fake Claude Code folder (`home_file`), Check again, its skill's Install; Add to PATH writing the user `Path`'s stand-in; the git command copied through the clipboard seam; WSL's stand-in naming Ubuntu (listed, not reached), Debian (stopped; Start refused: the run reaches no distro but its sandbox's, why under its name) and `docker-desktop` and `podman-machine-default` (container tools' VMs, not listed); Continue (`Enter`) writing the marker and a review opening; Settings (`Ctrl ,`) showing a skill edited since as out of date and Update fixing it, `Esc` closing it; after a `restart`, no first run and the status bar's notice "dv-review skill out of date on Windows", whose Update fixes it; screenshots of the first-run screen, with the distros, Settings and the notice | | `setup_linux` | Linux only, `setup`'s sibling: the first-run screen over this machine's checks alone (no distros on Linux; the header empties `WSL_DISTRO_NAME`, so a run inside WSL names the machine `Linux` as a desktop would), in the scratch home with `mock_gh` signed in; a fake Claude Code folder, Check again, Install; Add to PATH linking `~/.local/bin/dv` to this dv and putting `~/.local/bin` on the shell's `PATH` through its startup file, the shell then asked again with the scratch home; the git command copied through the clipboard seam; Continue and a review opening; after a `restart` the notice "dv-review skill out of date on Linux" and its Update; screenshots of the first-run screen (dark, and set up in dark and light) and the notice | | `switcher_branches` | the branch picker (DESIGN §4.1 "Compare branches…", PLAN §2.2): a stacked repo (`main → feat-a → feat-b`, and `side` off `main`) offers **Compare branches…** after its targets; it lists the branches newest first, each against its stack base (`feat-b` vs `feat-a`, the others vs `main`), a click on `feat-b` lists its bases (`feat-a` "stack base", `main` "default branch", then `side`), `Esc` steps back with the selection on `feat-b`, and `Enter` twice opens `feat-b` vs `feat-a` with only `b.txt`; the source picker's **Compare branches…** opens the picker on the repo, the branch's open review beside it, and `Esc` closes it; screenshots of the branches and the bases in dark, the stacked review, and the branches in light | +| `switcher_remove_repo` | Remove from list (DESIGN §4.1): with two seeded repos and `api`'s review open, `web`'s `⋯` (`switcher/repo/web/more`) opens its header menu and **Remove from list** (`switcher/menu/remove`) drops it from the switcher and `repos` (`known_repos.json`), the switcher staying open; a right-click on `api`'s header opens its menu with the item disabled ("Its review is open", `menu.removable` `false`), `Esc` closes the menu and leaves the switcher open, and `Ctrl Shift Backspace` on `api` removes nothing; screenshots of both menus | | `update` | dv's own update (PLAN D9, DESIGN §4.1): nothing shows in an automation run (it checks no feed); `update_ready` through the seam shows "dv is ready" and Restart to update at the status bar's right beside an open review; with two other dv processes running from the install (the stand-in's `others`) its click opens the confirmation (`update.confirm`, `status/update/confirm`) and Not now keeps everything; a restart the stand-in fails reads "Update failed · details" (`update.failure`, its tooltip) with Retry; with an unsent comment (`J`, `C`, typed) the confirmation names it and Restart anyway records the restart (`update.restart_requested`) while dv keeps running, the draft intact; screenshots of the item in dark and light, both confirmations and the failure | | `backend_loopback` | a repo reached through the loopback `dv backend --stdio` (this `dv`'s own server, a child of the app), as a WSL repo is through its distro's (PLAN §5.2): `seed_repo` with `"backend": "loopback"`, its review opening and a comment posting through the connection (`state.backends`), the remote watch following the agent's edit, a `backend_kill` that reconnects by itself (the status bar's "loopback · reconnecting…", no banner) and a file written while it was down listed after the synthetic rescan, then `backend_block` and a kill that runs out of respawns (attempts 2 and 3: a death soon after a reconnect continues its count): `failed`, "loopback · disconnected" and the backend-down banner (Details, Copy diagnostics); meanwhile the open switcher's row reads "reconnecting…" keeping its rows, then offers **Retry** once the connection failed; unblocked, the banner's Retry connects again and the watch follows the agent once more; then `backend_stop` (the host stopped, as `wsl --shutdown` stops a distro): the respawn starts nothing, `stopped`, "loopback · stopped", the banner's **Start loopback** and the switcher's Start row, and the banner's Start boots it again; stopped once more, the switcher's Start row started with the start held (`backend_hold`): the row stays, inert (`starting`), with the selection, and `Enter` again does nothing, until the repo lists and its first row takes the selection; a review picked from the switcher while the connection reconnects opens once it is back, with no banner; one picked while the reconnect then fails waits as `shell.pending` (the backend banner the only banner, `shell.banner` `null`); with a round summary typed in the sheet meanwhile it waits on after the switcher's Retry row brings the connection back (the review under the sheet stays, the summary kept), and opens by itself once the sheet is cancelled, no banner left; and after a `restart` with `app_args: ["open", ]` the repo is reached through the loopback again, where a review picked with no review open and its server killed at once (under the open) opens once the connection is back (generation 2, no banner); screenshots of the review over the loopback, while reconnecting, the switcher reconnecting and offering Retry, the banner and status bar in dark Mica, dark Opaque and light, the review reconnected, the stopped banner and switcher row, the switcher while its Start runs, and the pick waiting under the backend banner and then for the sheet | | `review_screen` | the diff stream (DESIGN §4.1, §5): the board's `api-server` with a CRLF README under `core.autocrlf`, a binary, a file too large to show, a rename, an untracked file, a deletion and a long file with gaps, opened from the switcher by keyboard; `J`/`K`, `N`/`P`, the cursor (`↑`/`↓`, `←`/`→`), a `Shift` selection and `Esc`, `S` to unified and back, gap rows and `E`/`Shift E`, `PageDown` with the pinned header, the agent staging everything under the reader (the gaps stay open, the place stays), the sideways scroll, line-number clicks and shift-clicks, the file menu's Copy path, `V` collapsing a viewed file and its chevron, "Load anyway"; screenshots of each state in dark Mica, dark Opaque and light | @@ -281,7 +282,7 @@ assert on them rather than on screenshots, which are for style. | `system.requests` | what the app asked the OS to do, oldest first, recorded instead of performed: `{"kind": "open_url", "url"}`, `{"kind": "reveal", "path"}`, `{"kind": "pick_folder", "prompt"}`, `{"kind": "clipboard", "text"}` (nothing reaches the real clipboard) | | `system.answers_left` | folder-picker answers still queued | | `repos` | the known repositories (`/known_repos.json`), most recently used first: `{path, name, location, loading, error, stopped, retry, reconnecting, starting, working, reviews}`. `location` is the stored shape (`{"local": "D:\\…"}`, `{"wsl": {"distro", "path"}}`); `error` is `null` or `{message, detail}` (a WSL location's says why it can't be opened: `"Ubuntu isn't running"`, or its connection's failure, kept when the distro still isn't running after a failed start); `stopped` is the distro (or connection) of a repo whose host isn't running, whatever its connection did before (its row offers **Start**), else `null`; `retry` whether its connection failed with its host running (its row offers **Retry**); `reconnecting` whether its connection is reconnecting (the row keeps `working` and `reviews` from its last listing); `starting` whether a Start or Retry is connecting; `working` is `{files, additions, deletions}` of the uncommitted changes, `null` until computed (on `open`, and when the switcher opens) or when it can't be; `reviews` newest first, each `{id, target, target_label, status, round, threads: {open, addressed, resolved, annotation}}`, or `{id, error}` for a log that doesn't load | -| `switcher` | the switcher (`Ctrl O`): `{open, mode, query, selected, items}`; closed it is `{open: false, mode: null, query: null, selected: null, items: []}`. `mode` is `"repos"`, or the branch picker's `"branches"` (a repo's branches) and `"bases"` (the bases the picked branch can be compared with, DESIGN §4.1). `items` are its pickable rows in order, each `{key, kind, target, label, detail, starting, repo, review}`: `key` names the row (`/working`, `/staged`, `/branch`, `/review/`, `/start`, `/retry`, `/compare`, `open_folder`; in the branch picker `/pick/` and `/base/`; `` is the repo's name, its full path when two known repos share a name), `kind` is `"target"`, `"review"`, `"start"` (**Start Ubuntu**, a stopped distro's one row), `"retry"` (**Retry**, a failed connection's one row), `"open_folder"`, `"compare"` (**Compare branches…**), `"branch"` or `"base"`, `starting` whether it is a Start or Retry whose start runs (drawn inert; picking it does nothing), `target` the target or base row's `"working"`/`"staged"`/`"branch"` (else `null`), `detail` a branch row's `"vs "` or a base row's `"stack base"` / `"default branch"` (else `null`), `repo` its path, `review` its open review `{id, status, round, open_threads}` or `null`; `selected` is a `key` | +| `switcher` | the switcher (`Ctrl O`): `{open, mode, query, selected, items, menu}`; closed it is `{open: false, mode: null, query: null, selected: null, items: [], menu: null}`. `menu` is a repo header's open menu (a right-click on `switcher/repo/`, or a click on its `⋯`, `switcher/repo//more`), `{repo, removable}` (`repo` its path; `removable` `false` for the open review's repo, whose **Remove from list**, `switcher/menu/remove`, is disabled), else `null`. `mode` is `"repos"`, or the branch picker's `"branches"` (a repo's branches) and `"bases"` (the bases the picked branch can be compared with, DESIGN §4.1). `items` are its pickable rows in order, each `{key, kind, target, label, detail, starting, repo, review}`: `key` names the row (`/working`, `/staged`, `/branch`, `/review/`, `/start`, `/retry`, `/compare`, `open_folder`; in the branch picker `/pick/` and `/base/`; `` is the repo's name, its full path when two known repos share a name), `kind` is `"target"`, `"review"`, `"start"` (**Start Ubuntu**, a stopped distro's one row), `"retry"` (**Retry**, a failed connection's one row), `"open_folder"`, `"compare"` (**Compare branches…**), `"branch"` or `"base"`, `starting` whether it is a Start or Retry whose start runs (drawn inert; picking it does nothing), `target` the target or base row's `"working"`/`"staged"`/`"branch"` (else `null`), `detail` a branch row's `"vs "` or a base row's `"stack base"` / `"default branch"` (else `null`), `repo` its path, `review` its open review `{id, status, round, open_threads}` or `null`; `selected` is a `key` | | `source_picker` | the source picker (the title bar's `⇄ ▾`): `{open, selected, items: [{kind, label, review}], compare}`, `selected` a `kind` or `"compare"`, `compare` whether it offers **Compare branches…** (`source_picker/compare`) after the items | | `file_list` | the panel's file list: `{query, shown, current_file}`; `shown` the paths the filter leaves, in list order; `current_file` the stream's current file (the highlighted row) | | `session` | the open review, or `null`: `review_id`, `repo` (the root of the review's home worktree, where its target resolves: PLAN §2.2; another worktree's than the row it was opened from for a working review made in a linked worktree, or a branch review of a branch checked out in one), `target` (stored shape: `"working"`, `"staged"`, `{"branch": {…}}`, …), `target_label` (`"working tree vs main"`), `status`, `round` (the one in progress; for a finished review the one that finished it, as the status bar says), `read_only` (approved, closed or submitted), `loading` (the file list is loading), `error` (`null` or `{message, detail}`), `viewed_error` (why the last viewed toggle failed, shown as `banner/viewed`; `null` once one lands or it is dismissed), `watch_notice` (why the worktree isn't watched, or `null`), `generation` (the load generation), `warnings` (problems in the log), `viewed` / `total` (files viewed this round / listed), `current_file` (the stream's current file: the cursor's while the cursor is on screen, else the file at the top), `view_mode` (`"split"` or `"unified"`, remembered per review in `/review_ui.json`), `pane` (`"stream"` or `"file_view"`: the pane `cursor`, `selection`, `composer` and `focused_thread` describe, the one the main area shows), `cursor` (`null` or `{path, side, line}`: `side` `"old"`/`"new"`, `line` its number on that side), `selection` (`null` or `{path, side, start_line, end_line}`, one file and one side) | diff --git a/tests/e2e/switcher_remove_repo.jsonl b/tests/e2e/switcher_remove_repo.jsonl new file mode 100644 index 0000000..b6db87d --- /dev/null +++ b/tests/e2e/switcher_remove_repo.jsonl @@ -0,0 +1,30 @@ +{"scenario": {"window_size": "1440x900", "description": "Remove from list (DESIGN §4.1): a repo header's `⋯` opens its menu, and Remove from list drops the repo from the switcher and `known_repos.json`, the selection moving on; the open review's repo has the item disabled (\"Its review is open\") and Ctrl Shift Backspace does nothing on it; Esc closes the menu before the switcher; screenshots of both menus."}} +{"cmd": "wait_ready", "timeout_ms": 20000} +{"cmd": "set_setting", "key": "mode", "value": "dark"} +{"cmd": "settle"} +{"cmd": "seed_repo", "name": "api", "steps": [{"write": "app.ts", "text": "export const app = 1;\n"}, {"commit": "base"}, {"write": "app.ts", "text": "export const app = 2;\n"}]} +{"cmd": "seed_repo", "name": "web", "steps": [{"write": "index.ts", "text": "export const web = 1;\n"}, {"commit": "base"}]} +{"cmd": "open", "path": "${data_dir}/repos/web"} +{"cmd": "open_review", "path": "${data_dir}/repos/api"} +{"cmd": "settle", "expect": [{"pointer": "/session/total", "equals": 1}, {"pointer": "/repos", "len": 2}]} + +// The switcher; web's `⋯` opens its menu, Remove from list enabled. +{"cmd": "key", "keystroke": "secondary-o"} +{"cmd": "settle", "expect": [{"pointer": "/switcher/open", "equals": true}, {"pointer": "/switcher/menu", "equals": null}, {"pointer": "/switcher/selected", "equals": "api/working"}]} +{"cmd": "click", "target": "switcher/repo/web/more"} +{"cmd": "settle", "expect": [{"pointer": "/switcher/menu/repo", "equals": "${data_dir}/repos/web"}, {"pointer": "/switcher/menu/removable", "equals": true}]} +{"cmd": "screenshot", "name": "menu"} +{"cmd": "click", "target": "switcher/menu/remove"} +{"cmd": "settle", "expect": [{"pointer": "/switcher/open", "equals": true}, {"pointer": "/switcher/menu", "equals": null}, {"pointer": "/repos", "len": 1}, {"pointer": "/repos/0/name", "equals": "api"}, {"pointer": "/switcher/items", "contains": {"key": "api/working"}}, {"pointer": "/switcher/selected", "equals": "api/working"}]} + +// The open review's repo: a right-click opens its menu with the item disabled, Esc closes the +// menu and leaves the switcher open, and Ctrl Shift Backspace does nothing. +{"cmd": "click", "target": "switcher/repo/api", "button": "right"} +{"cmd": "settle", "expect": [{"pointer": "/switcher/menu/repo", "equals": "${data_dir}/repos/api"}, {"pointer": "/switcher/menu/removable", "equals": false}]} +{"cmd": "screenshot", "name": "menu-open-review"} +{"cmd": "key", "keystroke": "escape"} +{"cmd": "settle", "expect": [{"pointer": "/switcher/menu", "equals": null}, {"pointer": "/switcher/open", "equals": true}]} +{"cmd": "key", "keystroke": "secondary-shift-backspace"} +{"cmd": "settle", "expect": [{"pointer": "/repos", "len": 1}, {"pointer": "/switcher/open", "equals": true}]} +{"cmd": "key", "keystroke": "escape"} +{"cmd": "settle", "expect": [{"pointer": "/switcher/open", "equals": false}]}