diff --git a/AGENTS.md b/AGENTS.md index a6d3f0bf..f943296c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,6 +152,9 @@ Layer 5 (binary): code_assistant — CLI, config, feature-gated frontend - Client and snapshot dir injected via `AgentRuntimeOptions::pull_requests`; the lifecycle sweep reads sessions' PRs through it in batches; see `docs/pull-request-view.md` +- A project folder holding several checkouts offers a choice of repository + above the list (`session_checkouts`, `right_panel/pull_request/repo_picker.rs`); + tabs are keyed by `PullRequestRef` and stay open when the list moves on - Checkout like `gh pr checkout` (`git::pull_request`, here or in a new worktree session) and stack rebases (`git::stack_rebase`, scratch repo) are git-crate work the core drives diff --git a/crates/code_assistant_core/src/pull_requests/mod.rs b/crates/code_assistant_core/src/pull_requests/mod.rs index 996c1f0e..bc7d7e5a 100644 --- a/crates/code_assistant_core/src/pull_requests/mod.rs +++ b/crates/code_assistant_core/src/pull_requests/mod.rs @@ -26,9 +26,12 @@ pub use store::{PullRequestSource, PullRequests}; pub(crate) use checkout::pull_request_head; -pub(crate) use repo::{github_remote, github_repo, own_pull_request}; +pub(crate) use repo::{ + github_checkouts, github_remote, github_repo, own_pull_request, working_branch, +}; use github::RepoRef; +use std::path::PathBuf; use std::time::SystemTime; /// A cached read: the value plus whether a background refresh is under way @@ -52,6 +55,23 @@ pub enum PullRequestPart { Stack, } +/// A checkout of a GitHub repository. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RepoCheckout { + pub path: PathBuf, + pub repo: RepoRef, + /// The branch checked out, unless it is the base branch. + pub branch: Option, +} + +/// The repositories a session can look at: that of its directory and those +/// checked out directly below it, as in a project folder holding several. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SessionCheckouts { + pub directory: PathBuf, + pub checkouts: Vec, +} + /// Where a session's pull requests live and which one is its own. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SessionPullRequests { diff --git a/crates/code_assistant_core/src/pull_requests/repo.rs b/crates/code_assistant_core/src/pull_requests/repo.rs index 2818ea95..b4a90e70 100644 --- a/crates/code_assistant_core/src/pull_requests/repo.rs +++ b/crates/code_assistant_core/src/pull_requests/repo.rs @@ -3,7 +3,9 @@ use github::RepoRef; use github::model::HeadPullRequest; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; + +use super::RepoCheckout; /// The GitHub repository of a checkout: its `origin` remote, else the first /// remote (by name) whose URL names a GitHub repository. @@ -29,6 +31,53 @@ pub(crate) async fn github_repo(path: PathBuf) -> Option { .flatten() } +/// The branch being worked on in a checkout: the one checked out, unless it +/// is the base branch. +pub(crate) fn working_branch(repo: &git::GitRepository) -> Option { + repo.current_branch().filter(|branch| { + !repo + .default_base_branch() + .is_some_and(|base| crate::session::lifecycle::is_base_branch(branch, &base)) + }) +} + +/// The GitHub repositories at `dir`: its own, then those of the checkouts +/// directly below it, by directory name. Hidden directories are not looked +/// into, so worktrees under `.worktrees` do not count, and a repository +/// checked out twice is listed once. +pub(crate) fn github_checkouts(dir: &Path) -> Vec { + let mut below: Vec = std::fs::read_dir(dir) + .into_iter() + .flatten() + .flatten() + .map(|entry| entry.path()) + .filter(|path| { + let hidden = path + .file_name() + .is_some_and(|name| name.to_string_lossy().starts_with('.')); + !hidden && path.join(".git").exists() + }) + .collect(); + below.sort_by_key(|path| path.to_string_lossy().to_lowercase()); + let mut checkouts: Vec = Vec::new(); + for path in std::iter::once(dir.to_path_buf()).chain(below) { + let Ok(opened) = git::GitRepository::open(&path) else { + continue; + }; + let Some(repo) = github_remote(&opened) else { + continue; + }; + if checkouts.iter().all(|checkout| checkout.repo != repo) { + checkouts.push(RepoCheckout { + branch: working_branch(&opened), + path, + repo, + }); + } + } + checkouts +} + /// Among the pull requests GitHub found for a branch (newest first), the /// newest opened from the repository itself, not from a fork that happens /// to use the same branch name. @@ -61,6 +110,73 @@ mod tests { git::GitRepository::open(dir).unwrap() } + /// A checkout of `github.com/team/` below `dir`. + fn checkout_below(dir: &Path, directory: &str, name: &str) -> std::path::PathBuf { + let path = dir.join(directory); + std::fs::create_dir_all(&path).unwrap(); + let url = format!("git@github.com:team/{name}.git"); + repo_with_remotes(&path, &[("origin", url.as_str())]); + path + } + + fn names(checkouts: &[RepoCheckout]) -> Vec<&str> { + checkouts.iter().map(|c| c.repo.name.as_str()).collect() + } + + #[test] + fn a_folder_of_checkouts_lists_each_by_directory_name() { + let tmp = tempfile::tempdir().unwrap(); + let beta = checkout_below(tmp.path(), "Beta", "beta"); + checkout_below(tmp.path(), "alpha", "alpha"); + // Neither a checkout nor one of GitHub's. + std::fs::create_dir(tmp.path().join("notes")).unwrap(); + let mirror = tmp.path().join("mirror"); + std::fs::create_dir(&mirror).unwrap(); + repo_with_remotes(&mirror, &[("origin", "/srv/mirrors/tool.git")]); + + let found = github_checkouts(tmp.path()); + assert_eq!(names(&found), ["alpha", "beta"]); + assert_eq!(found[1].path, beta); + } + + #[test] + fn the_folders_own_repository_comes_first() { + let tmp = tempfile::tempdir().unwrap(); + repo_with_remotes(tmp.path(), &[("origin", "git@github.com:team/root.git")]); + checkout_below(tmp.path(), "nested", "nested"); + + let found = github_checkouts(tmp.path()); + assert_eq!(names(&found), ["root", "nested"]); + assert_eq!(found[0].path, tmp.path()); + } + + #[test] + fn hidden_directories_and_second_checkouts_do_not_count() { + let tmp = tempfile::tempdir().unwrap(); + checkout_below(tmp.path(), "first", "tool"); + checkout_below(tmp.path(), "second", "tool"); + checkout_below(tmp.path(), ".worktrees", "hidden"); + + assert_eq!(names(&github_checkouts(tmp.path())), ["tool"]); + } + + #[test] + fn a_checkout_on_its_base_branch_has_no_working_branch() { + let tmp = tempfile::tempdir().unwrap(); + let path = checkout_below(tmp.path(), "tool", "tool"); + git(&path, &["config", "user.email", "t@t.t"]); + git(&path, &["config", "user.name", "t"]); + git(&path, &["config", "commit.gpgsign", "false"]); + git(&path, &["commit", "-q", "--allow-empty", "-m", "init"]); + assert_eq!(github_checkouts(tmp.path())[0].branch, None); + + git(&path, &["checkout", "-q", "-b", "feature/x"]); + assert_eq!( + github_checkouts(tmp.path())[0].branch.as_deref(), + Some("feature/x") + ); + } + #[test] fn origin_wins_over_other_github_remotes() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/code_assistant_core/src/pull_requests/store.rs b/crates/code_assistant_core/src/pull_requests/store.rs index abe5233f..01ba402a 100644 --- a/crates/code_assistant_core/src/pull_requests/store.rs +++ b/crates/code_assistant_core/src/pull_requests/store.rs @@ -3,7 +3,9 @@ use super::cache::{Cache, Policy, Refreshed}; use super::snapshots::{Kind, Snapshots}; -use super::{Fetched, PullRequestPart, SessionPullRequests, github_remote, own_pull_request}; +use super::{ + Fetched, PullRequestPart, SessionPullRequests, github_remote, own_pull_request, working_branch, +}; use crate::session::event_stream::EventStream; use crate::ui::UiEvent; use anyhow::Result; @@ -230,12 +232,7 @@ impl PullRequests { let checkout = tokio::task::spawn_blocking(move || { let repo = git::GitRepository::open(&path).ok()?; let github = github_remote(&repo)?; - let checked_out = repo.current_branch().filter(|branch| { - !repo - .default_base_branch() - .is_some_and(|base| crate::session::lifecycle::is_base_branch(branch, &base)) - }); - Some((github, checked_out)) + Some((github, working_branch(&repo))) }) .await?; let Some((repo, checked_out)) = checkout else { diff --git a/crates/code_assistant_core/src/session/service/pull_requests.rs b/crates/code_assistant_core/src/session/service/pull_requests.rs index 34e8b0c1..b9da1591 100644 --- a/crates/code_assistant_core/src/session/service/pull_requests.rs +++ b/crates/code_assistant_core/src/session/service/pull_requests.rs @@ -5,7 +5,8 @@ use super::*; use crate::pull_requests::{ - CheckoutPlace, Fetched, PullRequestCheckout, SessionPullRequests, pull_request_head, + CheckoutPlace, Fetched, PullRequestCheckout, SessionCheckouts, SessionPullRequests, + github_checkouts, pull_request_head, }; use crate::session::pull_request::PullRequestSnapshot; use github::model::{ @@ -49,10 +50,36 @@ impl SessionService { .await } - /// Check `pr` out as `gh pr checkout` does: `Here` in the session's - /// directory (its branch switches), or in a worktree of its own with a - /// new session for the project on it. The session working on it knows - /// it as its pull request. + /// The GitHub repositories of the session's directory: its own and those + /// checked out directly below it. Only local git is read. `None` for a + /// session without a project. + pub async fn session_checkouts(&self, session_id: String) -> Result> { + self.call_io(move |ctx| async move { + let directory = { + let manager = ctx.manager.lock().await; + let session = manager + .get_session(&session_id) + .ok_or_else(|| anyhow!("Session {session_id} not found"))?; + session.session.config.effective_project_path().cloned() + }; + let Some(directory) = directory else { + return Ok(None); + }; + let found = directory.clone(); + let checkouts = tokio::task::spawn_blocking(move || github_checkouts(&found)).await?; + Ok(Some(SessionCheckouts { + directory, + checkouts, + })) + }) + .await + } + + /// Check `pr` out as `gh pr checkout` does, in the checkout of its + /// repository: `Here` in the session's directory (its branch switches) + /// or the checkout below it (the session stays as it is), or in a + /// worktree of its own with a new session for the project on it. The + /// session working on it knows it as its pull request. pub async fn pull_request_checkout( &self, session_id: String, @@ -74,11 +101,22 @@ impl SessionService { ) }; let detail = ctx.pull_requests.detail(pr.clone()).await?.value; - let path = match place { + let directory = match place { CheckoutPlace::Here => here, // Worktrees hang off the repository itself. - CheckoutPlace::Worktree => project_root.clone(), + CheckoutPlace::Worktree => project_root, }; + let path = { + let (directory, wanted) = (directory.clone(), pr.repo.clone()); + tokio::task::spawn_blocking(move || { + github_checkouts(&directory) + .into_iter() + .find(|checkout| checkout.repo == wanted) + .map(|checkout| checkout.path) + }) + .await? + } + .ok_or_else(|| anyhow!("{} is not checked out in this project", pr.repo.full_name()))?; let repo = git::GitRepository::open(&path).context("Failed to open git repository")?; let remotes: Vec<(String, String)> = repo .remotes() @@ -96,6 +134,15 @@ impl SessionService { let mut manager = ctx.manager.lock().await; let session_id = match place { + // A session spanning several repositories has no one branch. + CheckoutPlace::Here if path != directory => { + return Ok(PullRequestCheckout { + session_id, + path: done.path, + branch: done.branch, + on_head: done.on_head, + }); + } CheckoutPlace::Here => { manager.set_session_worktree( &session_id, @@ -105,7 +152,7 @@ impl SessionService { session_id } CheckoutPlace::Worktree => { - let mut config = manager.new_session_config(Some(project_root)); + let mut config = manager.new_session_config(Some(path)); config.initial_project = project; let model = SessionModelConfig::new(manager.default_model_name()); let name = format!("#{} {}", detail.number, detail.title); @@ -503,6 +550,63 @@ mod tests { assert_eq!(service.session_pull_requests(bare).await.unwrap(), None); } + /// A repository at `dir` with a commit and `url` as its `origin`. + fn checkout_with_origin(dir: &std::path::Path, url: &str) { + std::fs::create_dir_all(dir).unwrap(); + git(dir, &["init", "-q", "-b", "main"]); + git(dir, &["config", "user.email", "t@t.t"]); + git(dir, &["config", "user.name", "t"]); + git(dir, &["config", "commit.gpgsign", "false"]); + git(dir, &["commit", "-q", "--allow-empty", "-m", "init"]); + git(dir, &["remote", "add", "origin", url]); + } + + #[tokio::test] + async fn a_project_folder_offers_the_checkouts_below_it() { + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("project"); + checkout_with_origin(&project.join("alpha"), "git@github.com:team/alpha.git"); + checkout_with_origin(&project.join("beta"), "git@github.com:team/beta.git"); + git( + &project.join("beta"), + &["checkout", "-q", "-b", "feature/x"], + ); + let source = PullRequestSource { + client: FakeTransport::new().client(), + snapshot_dir: tmp.path().join("pull-requests"), + }; + let (service, _manager) = + test_service_with_pull_requests(&tmp.path().join("sessions"), source); + let config = crate::session::SessionConfig { + init_path: Some(project.clone()), + ..crate::session::SessionConfig::default() + }; + let id = service + .create_session_with_config(None, config, None) + .await + .unwrap(); + service.load_session(id.clone(), None).await.unwrap(); + + let found = service + .session_checkouts(id.clone()) + .await + .unwrap() + .unwrap(); + assert_eq!(found.directory, project); + let repos: Vec<_> = found + .checkouts + .iter() + .map(|c| (c.repo.name.as_str(), c.branch.as_deref())) + .collect(); + assert_eq!(repos, [("alpha", None), ("beta", Some("feature/x"))]); + // The folder itself is no repository, as far as pull requests go. + assert_eq!(service.session_pull_requests(id).await.unwrap(), None); + + let bare = service.create_session(None, None).await.unwrap(); + service.load_session(bare.clone(), None).await.unwrap(); + assert_eq!(service.session_checkouts(bare).await.unwrap(), None); + } + /// `git` in `dir`, its output. fn git_out(dir: &std::path::Path, args: &[&str]) -> String { let output = std::process::Command::new("git") @@ -522,11 +626,11 @@ mod tests { String::from_utf8(output.stdout).unwrap().trim().to_owned() } - #[tokio::test] - async fn a_forks_pull_request_is_checked_out_in_a_worktree_with_a_session_of_its_own() { - let tmp = tempfile::tempdir().unwrap(); - // "GitHub": main, and the fork's head at refs/pull/291/head. - let upstream = tmp.path().join("upstream"); + /// A clone at `project` of `stippi/code-assistant` that fetches over a + /// stand-in for ssh from a local "GitHub": main, and a fork's head at + /// refs/pull/291/head, which is returned. + fn clone_of_github(tmp: &std::path::Path, project: &std::path::Path) -> String { + let upstream = tmp.join("upstream"); std::fs::create_dir(&upstream).unwrap(); git_out(&upstream, &["init", "-q", "-b", "main"]); git_out(&upstream, &["commit", "-q", "--allow-empty", "-m", "init"]); @@ -537,9 +641,8 @@ mod tests { let head = git_out(&upstream, &["rev-parse", "HEAD"]); git_out(&upstream, &["update-ref", "refs/pull/291/head", &head]); git_out(&upstream, &["reset", "-q", "--hard", "HEAD~1"]); - let project = tmp.path().join("code-assistant"); git_out( - tmp.path(), + tmp, &[ "clone", "-q", @@ -547,20 +650,136 @@ mod tests { project.to_str().unwrap(), ], ); + let ssh = tmp.join("ssh.sh"); + std::fs::write( + &ssh, + format!("#!/bin/sh\nexec git upload-pack '{}'\n", upstream.display()), + ) + .unwrap(); + git_out( + project, + &[ + "config", + "core.sshCommand", + &format!("sh {}", ssh.display()), + ], + ); + git_out( + project, + &[ + "remote", + "set-url", + "origin", + "git@github.com:stippi/code-assistant.git", + ], + ); + head + } + + /// GitHub's answer for that fork's pull request; maintainers may push. + fn fork_pull_request(head: &str) -> serde_json::Value { + let path = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../github/tests/fixtures/pull_request_core.json" + ); + let text = std::fs::read_to_string(path).unwrap(); + let mut data = serde_json::from_str::(&text).unwrap()["data"].take(); + data["repository"]["pullRequest"]["headRefOid"] = head.into(); + data + } + #[tokio::test] + async fn a_pull_request_of_a_checkout_below_is_checked_out_there() { + let tmp = tempfile::tempdir().unwrap(); + let folder = tmp.path().join("folder"); + std::fs::create_dir(&folder).unwrap(); + let tool = folder.join("tool"); + let head = clone_of_github(tmp.path(), &tool); let fake = FakeTransport::new(); - let core = { - let path = concat!( - env!("CARGO_MANIFEST_DIR"), - "/../github/tests/fixtures/pull_request_core.json" - ); - let text = std::fs::read_to_string(path).unwrap(); - let mut data = serde_json::from_str::(&text).unwrap()["data"].take(); - // A fork's pull request, maintainers may push. - data["repository"]["pullRequest"]["headRefOid"] = head.clone().into(); - data + fake.on_graphql("PullRequestCore") + .data(fork_pull_request(&head)); + let source = PullRequestSource { + client: fake.client(), + snapshot_dir: tmp.path().join("pull-requests"), }; - fake.on_graphql("PullRequestCore").data(core); + let (service, manager) = + test_service_with_pull_requests(&tmp.path().join("sessions"), source); + let config = crate::session::SessionConfig { + init_path: Some(folder.clone()), + initial_project: "folder".into(), + ..crate::session::SessionConfig::default() + }; + let id = service + .create_session_with_config(None, config, None) + .await + .unwrap(); + service.load_session(id.clone(), None).await.unwrap(); + let pr = RepoRef::new("github.com", "stippi", "code-assistant").pull_request(291); + + // In a worktree: a session of its own, of the same project. + let done = service + .pull_request_checkout(id.clone(), pr.clone(), CheckoutPlace::Worktree) + .await + .unwrap(); + assert_ne!(done.session_id, id); + assert_eq!(git_out(&done.path, &["rev-parse", "HEAD"]), head); + { + let manager = manager.lock().await; + let config = &manager + .get_session(&done.session_id) + .unwrap() + .session + .config; + assert_eq!(config.init_path.as_ref(), Some(&tool)); + assert_eq!(config.worktree_path.as_ref(), Some(&done.path)); + assert_eq!(config.initial_project, "folder"); + } + let in_worktree = service + .session_checkouts(done.session_id) + .await + .unwrap() + .unwrap(); + assert_eq!( + in_worktree.checkouts.len(), + 1, + "a session on one repository" + ); + + // Here: the checkout below moves, the session spans them still. + git_out(&done.path, &["checkout", "-q", "--detach"]); + let done = service + .pull_request_checkout(id.clone(), pr.clone(), CheckoutPlace::Here) + .await + .unwrap(); + assert_eq!(done.session_id, id); + assert_eq!(done.path, tool); + assert_eq!(git_out(&tool, &["rev-parse", "HEAD"]), head); + let manager = manager.lock().await; + let config = &manager.get_session(&id).unwrap().session.config; + assert_eq!(config.branch, None); + assert_eq!(config.worktree_path, None); + let lifecycle = manager.session_lifecycles().unwrap().remove(&id); + assert!(lifecycle.is_none_or(|lifecycle| lifecycle.pull_request.is_none())); + + // A repository not checked out here says so. + drop(manager); + let other = RepoRef::new("github.com", "stippi", "other").pull_request(291); + let refused = service + .pull_request_checkout(id, other, CheckoutPlace::Here) + .await + .unwrap_err(); + assert!(format!("{refused:#}").contains("stippi/other is not checked out")); + } + + #[tokio::test] + async fn a_forks_pull_request_is_checked_out_in_a_worktree_with_a_session_of_its_own() { + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("code-assistant"); + let head = clone_of_github(tmp.path(), &project); + + let fake = FakeTransport::new(); + fake.on_graphql("PullRequestCore") + .data(fork_pull_request(&head)); let source = PullRequestSource { client: fake.client(), snapshot_dir: tmp.path().join("pull-requests"), @@ -591,7 +810,7 @@ mod tests { // Pushed back to the fork, like `gh pr checkout`. assert_eq!( git_out(&project, &["config", "branch.feature/pr-view.remote"]), - "https://github.com/contributor/code-assistant.git" + "git@github.com:contributor/code-assistant.git" ); let manager = manager.lock().await; diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/header.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/header.rs index 077aef2c..1e1166f2 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/header.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/header.rs @@ -95,6 +95,10 @@ impl EventEmitter for DetailHeader {} impl EventEmitter for DetailHeader {} impl DetailHeader { + pub(super) fn repo(&self) -> &github::RepoRef { + &self.pr.repo + } + /// `merge_method` is the one last used in the repository. pub fn new( pr: PullRequestRef, diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/list.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/list.rs index c9d87dcf..2c77ce17 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/list.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/list.rs @@ -31,8 +31,10 @@ use super::preferences; use super::presentation::{ GLYPH_SIZE, Tone, checks_glyph, conflict_glyph, diff_stat, relative_time, review_glyph, }; +use super::repo_picker::{RepoPicked, RepoPicker}; use super::stack::{LAYERS_ICON, base_row, layer_row, position_label}; use chrono::Utc; +use code_assistant_core::pull_requests::RepoCheckout; use github::RepoRef; use github::model::{ Action, DiffStat, Involvement, ListEntry, ListPage, ListQuery, ListState as ListedState, @@ -104,6 +106,8 @@ pub struct PullRequestList { involvement: Involvement, sort: SortMode, filters: Filters, + repo_picker: Entity, + repo_picker_open: bool, search: Entity, /// The search field's text. query: String, @@ -122,7 +126,14 @@ impl PullRequestList { let search = cx.new(|cx| { InputState::new(window, cx).placeholder("Search pull requests, or label:bug") }); - let subscriptions = vec![cx.subscribe_in(&search, window, Self::on_search_event)]; + let repo_picker = cx.new(|cx| RepoPicker::new(window, cx)); + let subscriptions = vec![ + cx.subscribe_in(&search, window, Self::on_search_event), + cx.subscribe(&repo_picker, |this, _, picked: &RepoPicked, cx| { + this.repo_picker_open = false; + cx.emit(PullRequestIntent::Focus(picked.0.clone())); + }), + ]; let remembered = preferences::read(cx).list; Self { repo: None, @@ -149,6 +160,8 @@ impl PullRequestList { involvement: remembered.involvement, sort: remembered.sort, filters: remembered.filters, + repo_picker, + repo_picker_open: false, search, query: String::new(), listed_text: String::new(), @@ -164,11 +177,15 @@ impl PullRequestList { if self.repo == repo { return; } + self.repo_picker.update(cx, |picker, cx| { + picker.set_selected(repo.clone(), cx); + }); self.repo = repo; self.entries.clear(); self.overrides.clear(); self.stats.clear(); self.stats_asked.clear(); + self.stacks.clear(); self.next_cursor = None; self.loaded = false; self.error = None; @@ -176,6 +193,13 @@ impl PullRequestList { cx.notify(); } + /// The repositories to choose from, recent ones first. + pub fn set_checkouts(&mut self, checkouts: Vec, cx: &mut Context) { + self.repo_picker + .update(cx, |picker, cx| picker.set_checkouts(checkouts, cx)); + cx.notify(); + } + /// The signed-in login, which decides the groups. pub fn set_viewer(&mut self, viewer: Option, cx: &mut Context) { if self.viewer != viewer { @@ -302,6 +326,13 @@ impl PullRequestList { } } + /// Ask for pull request `number` of the listed repository. + fn open(&self, number: u64, cx: &mut Context) { + if let Some(repo) = &self.repo { + cx.emit(PullRequestIntent::Open(repo.pull_request(number))); + } + } + /// A row as shown. pub fn entry(&self, number: u64) -> Option<&ListEntry> { self.shown @@ -656,9 +687,7 @@ impl PullRequestList { .child(line_one) .child(line_two), ) - .on_click( - cx.listener(move |_, _, _, cx| cx.emit(PullRequestIntent::Open(number))), - ), + .on_click(cx.listener(move |this, _, _, cx| this.open(number, cx))), ) .into_any_element() } @@ -703,10 +732,53 @@ impl PullRequestList { .into_any_element() } - /// Search field, then Sort, Filters and refresh. + /// The repository listed, and the popover to choose another. + fn render_repo_trigger(&self, repo: &RepoRef, cx: &mut Context) -> AnyElement { + let this = cx.entity().downgrade(); + let picker = self.repo_picker.clone(); + div() + .flex() + .child( + div().debug_selector(|| "pr-repo-picker".into()).child( + Popover::new("pr-repo-picker") + .anchor(gpui_kit::Anchor::TopLeft) + .open(self.repo_picker_open) + .on_open_change(move |open, window, cx| { + let open = *open; + this.update(cx, |this, cx| { + this.repo_picker_open = open; + if open { + this.repo_picker + .update(cx, |picker, cx| picker.start_search(window, cx)); + } + cx.notify(); + }) + .ok(); + }) + .trigger( + Button::new("pr-repo-picker-trigger") + .outline() + .small() + .label(repo.name.clone()) + .dropdown_caret(true) + .tooltip(repo.full_name()), + ) + .content(move |_, _, _| picker.clone().into_any_element()), + ), + ) + .into_any_element() + } + + /// The repository when there is a choice, the search field, then Sort, + /// Filters and refresh. fn render_toolbar(&self, cx: &mut Context) -> impl IntoElement { let list = cx.entity().downgrade(); let sort_list = list.clone(); + let repo_trigger = self + .repo + .as_ref() + .filter(|_| self.repo_picker.read(cx).offers_a_choice()) + .map(|repo| self.render_repo_trigger(repo, cx)); div() .flex_none() .px_4() @@ -715,6 +787,7 @@ impl PullRequestList { .flex() .flex_col() .gap_2() + .children(repo_trigger) .child( Input::new(&self.search) .with_size(Size::Small) @@ -1131,6 +1204,9 @@ fn toggle_label(filters: &mut Filters, name: String) { impl Render for PullRequestList { fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { + if self.repo.is_none() && self.repo_picker.read(cx).offers_a_choice() { + return self.repo_picker.clone().into_any_element(); + } let note = self.render_note(cx); div() .size_full() @@ -1159,6 +1235,7 @@ impl Render for PullRequestList { .size_full(), ), ) + .into_any_element() } } @@ -1311,8 +1388,7 @@ fn row_stack_menu( popover .update(cx, |state, cx| state.dismiss(window, cx)) .ok(); - list.update(cx, |_, cx| cx.emit(PullRequestIntent::Open(target))) - .ok(); + list.update(cx, |list, cx| list.open(target, cx)).ok(); }) })), ) @@ -1529,7 +1605,10 @@ mod tests { let (_, intents, cx) = list_view(cx); let bounds = cx.debug_bounds("pr-row-107").unwrap(); cx.simulate_click(bounds.center(), gpui_kit::Modifiers::none()); - assert_eq!(intents.borrow().as_slice(), [PullRequestIntent::Open(107)]); + assert_eq!( + intents.borrow().as_slice(), + [PullRequestIntent::Open(repo().pull_request(107))] + ); } #[gpui_kit::test] diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/mod.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/mod.rs index 95c62bb9..90ef4674 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/mod.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/mod.rs @@ -1,6 +1,7 @@ //! The "Pull request" view of the right panel (`docs/pull-request-view.md`): -//! the pull requests of the session's repository, and a tab for each one -//! opened from there ([`tabs`]), the session's own first. +//! the pull requests of one repository of the session, and a tab for each +//! one opened from there ([`tabs`]), the session's own first. Tabs stay open +//! when the list moves to another repository, so they may name several. //! //! The views ([`list::PullRequestList`], [`detail::DetailView`] with its //! header and tab bodies) render model values from the `github` crate and @@ -27,6 +28,7 @@ mod list_overrides; mod markdown; pub mod preferences; mod presentation; +mod repo_picker; pub mod review_draft; mod stack; mod summary; @@ -37,16 +39,18 @@ mod timeline; use crate::shared::ui_state::{PullRequestTabState, PullRequestTabsState}; use bus::{PullRequestEvent, bus}; -use code_assistant_core::pull_requests::{CheckoutPlace, PullRequestCheckout}; +use code_assistant_core::pull_requests::{CheckoutPlace, PullRequestCheckout, SessionCheckouts}; use data::session_service; use detail::{DetailTab, DetailTabChanged, DetailView}; -use github::RepoRef; use github::model::{Action, ListQuery, PullRequestUpdate}; +use github::{PullRequestRef, RepoRef}; use gpui_kit::{ ClipboardItem, Context, Entity, Render, ScrollHandle, Subscription, Task, Window, div, prelude::*, }; use list::PullRequestList; +use repo_picker::{first_focus, recent_first}; +use std::path::PathBuf; use tabs::Tabs; /// How often relative times ("9h ago") are drawn anew. @@ -55,8 +59,10 @@ const CLOCK_TICK: std::time::Duration = std::time::Duration::from_secs(60); /// What the user asks of the pull request view. #[derive(Clone, Debug, PartialEq)] pub enum PullRequestIntent { - /// Show a pull request of the listed repository in its tab. - Open(u64), + /// Show a pull request in its tab. + Open(PullRequestRef), + /// List this repository's pull requests from now on. + Focus(RepoRef), /// List the repository's pull requests anew. List(ListQuery), /// Read the next page of the listing. @@ -87,13 +93,18 @@ pub struct PullRequestPanel { session_id: Option, /// The tab is shown (the detail polls only then). visible: bool, + /// The folder the session looks at, with the repositories it offers. + directory: Option, + /// The repository listed. repo: Option, list: Entity, /// A page per opened pull request; the shown one, or the list. - tabs: Tabs, + tabs: Tabs, strip_scroll: ScrollHandle, /// Finding the session's repository and pull request. resolve: Option>, + /// Looking for repositories anew. + scan: Option>, list_read: Option>, more_read: Option>, viewer_read: Option>, @@ -127,11 +138,13 @@ impl PullRequestPanel { Self { session_id: None, visible: false, + directory: None, repo: None, list, tabs: Tabs::default(), strip_scroll: ScrollHandle::new(), resolve: None, + scan: None, list_read: None, more_read: None, viewer_read: None, @@ -156,9 +169,13 @@ impl PullRequestPanel { if self.session_id.as_deref() == Some(session_id.as_str()) { // Back from hiding: the listing may have changed meanwhile. self.load_list(cx); + self.scan_checkouts(session_id, cx); return; } self.session_id = Some(session_id.clone()); + self.directory = None; + self.list + .update(cx, |list, cx| list.set_checkouts(Vec::new(), cx)); self.set_repository(None, None, cx); self.resolve_repository(session_id, cx); } @@ -181,7 +198,8 @@ impl PullRequestPanel { }) } - /// Ask the core where the session's pull requests live. + /// Ask the core which repositories the session has, then, for the one + /// of its directory, which pull request is its own. fn resolve_repository(&mut self, session_id: String, cx: &mut Context) { let Some(service) = session_service(cx) else { return; @@ -189,25 +207,122 @@ impl PullRequestPanel { self.list .update(cx, |list, cx| list.set_resolving(true, cx)); self.resolve = Some(cx.spawn(async move |this, cx| { - let found = service.session_pull_requests(session_id.clone()).await; + let found = service.session_checkouts(session_id.clone()).await; + let of_directory = this.update(cx, |this, cx| { + if this.session_id.as_deref() != Some(session_id.as_str()) { + return None; + } + match found { + Ok(found) => this.checkouts_found(found, cx), + Err(error) => { + this.resolve_failed(error, cx); + None + } + } + }); + let Ok(Some(repo)) = of_directory else { + return; + }; + let own = service.session_pull_requests(session_id.clone()).await; this.update(cx, |this, cx| { if this.session_id.as_deref() != Some(session_id.as_str()) { return; } - this.list + match own { + Ok(own) => { + this.list + .update(cx, |list, cx| list.set_resolving(false, cx)); + this.set_repository(Some(repo), own.and_then(|own| own.own), cx); + } + Err(error) => this.resolve_failed(error, cx), + } + }) + .ok(); + })); + } + + fn resolve_failed(&mut self, error: anyhow::Error, cx: &mut Context) { + self.list.update(cx, |list, cx| { + list.set_resolving(false, cx); + list.set_error(Some(format!("{error:#}")), cx) + }); + } + + /// Offer the repositories found and list the first one. The one of the + /// session's directory is returned instead: its own pull request is + /// still to be found. + fn checkouts_found( + &mut self, + found: Option, + cx: &mut Context, + ) -> Option { + let Some(found) = found else { + self.list + .update(cx, |list, cx| list.set_resolving(false, cx)); + return None; + }; + let listed_last = self.session_id.as_ref().and_then(|session_id| { + crate::shared::ui_state::read(cx, |store| store.get_pull_request_tabs(session_id).repo) + .flatten() + }); + let preferences = preferences::read(cx); + let recent = preferences.recent_repos(&found.directory); + let first = first_focus(&found, listed_last.as_ref(), recent) + .map(|first| (first.repo.clone(), first.path == found.directory)); + let checkouts = recent_first(found.checkouts, recent); + self.directory = Some(found.directory); + self.list + .update(cx, |list, cx| list.set_checkouts(checkouts, cx)); + match first { + Some((repo, true)) => Some(repo), + other => { + self.list .update(cx, |list, cx| list.set_resolving(false, cx)); - match found { - Ok(Some(found)) => this.set_repository(Some(found.repo), found.own, cx), - Ok(None) => {} - Err(error) => this.list.update(cx, |list, cx| { - list.set_error(Some(format!("{error:#}")), cx) - }), + self.set_repository(other.map(|(repo, _)| repo), None, cx); + None + } + } + } + + /// Look for repositories that came or went while the panel was hidden. + fn scan_checkouts(&mut self, session_id: String, cx: &mut Context) { + let Some(service) = session_service(cx) else { + return; + }; + self.scan = Some(cx.spawn(async move |this, cx| { + let Ok(Some(found)) = service.session_checkouts(session_id.clone()).await else { + return; + }; + this.update(cx, |this, cx| { + if this.session_id.as_deref() == Some(session_id.as_str()) { + let preferences = preferences::read(cx); + let recent = preferences.recent_repos(&found.directory); + let checkouts = recent_first(found.checkouts, recent); + this.list + .update(cx, |list, cx| list.set_checkouts(checkouts, cx)); } }) .ok(); })); } + /// List `repo` from now on; open tabs stay. + fn focus(&mut self, repo: RepoRef, cx: &mut Context) { + if self.repo.as_ref() != Some(&repo) { + self.repo = Some(repo.clone()); + self.more_read = None; + self.list.update(cx, |list, cx| { + list.set_repo(Some(repo.clone()), cx); + list.set_viewer(None, cx); + }); + self.read_viewer(cx); + if let Some(directory) = self.directory.clone() { + preferences::update(cx, |preferences| preferences.chose_repo(&directory, &repo)); + } + } + self.activate(None, cx); + } + /// Where the session's pull requests live and which one is its own. /// The tabs the session had open come back; without any, its own pull /// request opens. @@ -222,23 +337,33 @@ impl PullRequestPanel { self.list.update(cx, |list, cx| list.set_repo(repo, cx)); self.load_list(cx); self.read_viewer(cx); - let remembered = match (&self.session_id, &self.repo) { - (Some(session_id), Some(_)) => { + let remembered = self + .session_id + .as_ref() + .and_then(|session_id| { crate::shared::ui_state::read(cx, |store| store.get_pull_request_tabs(session_id)) - .unwrap_or_default() - } - _ => Default::default(), - }; + }) + .unwrap_or_default(); if remembered.open.is_empty() { - if let Some(number) = own { - self.open(number, cx); + if let Some(pr) = own.and_then(|number| Some(self.repo.as_ref()?.pull_request(number))) + { + self.open(pr, cx); } } else { - for open in &remembered.open { - let tab = open.tab.as_deref().and_then(DetailTab::parse); - self.open_tab(open.number, tab, cx); + let listed = self.repo.clone(); + let opened = |repo: Option, number| { + Some(repo.or_else(|| listed.clone())?.pull_request(number)) + }; + for open in remembered.open { + if let Some(pr) = opened(open.repo, open.number) { + let tab = open.tab.as_deref().and_then(DetailTab::parse); + self.open_tab(pr, tab, cx); + } } - let active = remembered.active.and_then(|n| self.tabs.position(n)); + let active = remembered + .active + .and_then(|number| opened(remembered.active_repo, number)) + .and_then(|pr| self.tabs.position(&pr)); self.tabs.activate(active); self.sync_visibility(cx); } @@ -340,11 +465,15 @@ impl PullRequestPanel { return; }; let prs = numbers.iter().map(|n| repo.pull_request(*n)).collect(); - let list = self.list.downgrade(); cx.spawn( - async move |_, cx| match service.pull_request_list_stats(prs).await { + async move |this, cx| match service.pull_request_list_stats(prs).await { Ok(stats) => { - list.update(cx, |list, cx| list.set_stats(&stats, cx)).ok(); + this.update(cx, |this, cx| { + if this.repo.as_ref() == Some(&repo) { + this.list.update(cx, |list, cx| list.set_stats(&stats, cx)); + } + }) + .ok(); } Err(error) => tracing::debug!("no line counts: {error:#}"), }, @@ -410,7 +539,8 @@ impl PullRequestPanel { fn handle(&mut self, intent: &PullRequestIntent, cx: &mut Context) { match intent { - PullRequestIntent::Open(number) => self.open(*number, cx), + PullRequestIntent::Open(pr) => self.open(pr.clone(), cx), + PullRequestIntent::Focus(repo) => self.focus(repo.clone(), cx), PullRequestIntent::OpenUrl(url) => cx.open_url(url), PullRequestIntent::Copy(text) => { cx.write_to_clipboard(ClipboardItem::new_string(text.clone())) @@ -423,11 +553,15 @@ impl PullRequestPanel { PullRequestIntent::Act(action) => { if let Some(detail) = self.detail() { let action = *action; - let number = detail.read(cx).pr().number; + let pr = detail.read(cx).pr().clone(); + let number = pr.number; // The list shows the outcome already, as after a quick action. - let token = self - .list - .update(cx, |list, cx| list.override_row(number, &action, cx)); + let token = (self.repo.as_ref() == Some(&pr.repo)) + .then(|| { + self.list + .update(cx, |list, cx| list.override_row(number, &action, cx)) + }) + .flatten(); let done = detail.update(cx, |detail, cx| detail.act(action, cx)); self.revert_unless(done, number, token, cx); } @@ -491,28 +625,25 @@ impl PullRequestPanel { .detach(); } - /// Show pull request `number` in its tab, opened when it is not. - fn open(&mut self, number: u64, cx: &mut Context) { - let Some(ix) = self.open_tab(number, None, cx) else { - return; - }; + /// Show `pr` in its tab, opened when it is not. + fn open(&mut self, pr: PullRequestRef, cx: &mut Context) { + let ix = self.open_tab(pr, None, cx); self.tabs.activate(Some(ix)); self.strip_scroll.scroll_to_item(ix + 1); self.tabs_changed(cx); } - /// The tab of `number`, opened (on `tab`) when it is not; shows it. + /// The tab of `pr`, opened (on `tab`) when it is not; shows it. fn open_tab( &mut self, - number: u64, + pr: PullRequestRef, tab: Option, cx: &mut Context, - ) -> Option { - let repo = self.repo.clone()?; - let seed = self.list.read(cx).entry(number).cloned(); - let ix = self.tabs.open(number, || { + ) -> usize { + let seed = self.listed_entry(&pr, cx).cloned(); + self.tabs.open(pr.clone(), || { let view = cx.new(|cx| { - let mut detail = DetailView::new(repo.pull_request(number), seed, cx); + let mut detail = DetailView::new(pr, seed, cx); if let Some(tab) = tab { detail.select_tab(tab, cx); } @@ -529,8 +660,19 @@ impl PullRequestPanel { ], view, } - }); - Some(ix) + }) + } + + /// `pr`'s row in the list, if the list is of its repository. + fn listed_entry<'a>( + &self, + pr: &PullRequestRef, + cx: &'a Context, + ) -> Option<&'a github::model::ListEntry> { + if self.repo.as_ref() != Some(&pr.repo) { + return None; + } + self.list.read(cx).entry(pr.number) } /// Show the tab at `ix`, or the list. @@ -593,16 +735,20 @@ impl PullRequestPanel { let Some(session_id) = self.session_id.clone() else { return; }; + let active = self.tabs.active_item().map(|(pr, _)| pr); let state = PullRequestTabsState { + repo: self.repo.clone(), open: self .tabs .iter() - .map(|(number, tab)| PullRequestTabState { - number, + .map(|(pr, tab)| PullRequestTabState { + repo: Some(pr.repo.clone()), + number: pr.number, tab: Some(tab.view.read(cx).tab().as_str().to_owned()), }) .collect(), - active: self.tabs.active_item().map(|(number, _)| number), + active: active.map(|pr| pr.number), + active_repo: active.map(|pr| pr.repo.clone()), }; crate::shared::ui_state::update(cx, |store| { store.set_pull_request_tabs(&session_id, state) @@ -734,7 +880,9 @@ mod tests { click(cx, "pr-tab-list"); click(cx, "pr-row-290"); let tabs = |panel: &Entity, cx: &mut VisualTestContext| { - panel.read_with(cx, |panel, _| panel.tabs.numbers().collect::>()) + panel.read_with(cx, |panel, _| { + panel.tabs.keys().map(|pr| pr.number).collect::>() + }) }; assert_eq!(tabs(&panel, cx), [281, 290]); first.read_with(cx, |view, _| { @@ -769,7 +917,7 @@ mod tests { panel.update(cx, |panel, cx| { panel.set_repository(Some(repo()), None, cx); for number in [1, 2, 3] { - panel.open(number, cx); + panel.open(repo().pull_request(number), cx); } }); cx.run_until_parked(); @@ -784,11 +932,133 @@ mod tests { cx.simulate_mouse_up(to, gpui_kit::MouseButton::Left, gpui_kit::Modifiers::none()); cx.run_until_parked(); panel.read_with(cx, |panel, _| { - assert_eq!(panel.tabs.numbers().collect::>(), [3, 1, 2]); - assert_eq!(panel.tabs.active_item().map(|(n, _)| n), Some(3)); + let numbers: Vec<_> = panel.tabs.keys().map(|pr| pr.number).collect(); + assert_eq!(numbers, [3, 1, 2]); + assert_eq!(panel.tabs.active_item().map(|(pr, _)| pr.number), Some(3)); }); } + /// A folder holding checkouts of these repositories. + fn folder(names: &[&str]) -> SessionCheckouts { + let directory = PathBuf::from("/folder"); + SessionCheckouts { + checkouts: names + .iter() + .map(|name| code_assistant_core::pull_requests::RepoCheckout { + path: directory.join(name), + repo: RepoRef::new("github.com", "team", *name), + branch: None, + }) + .collect(), + directory, + } + } + + #[gpui_kit::test] + fn a_folder_of_checkouts_asks_which_one_and_tabs_outlast_the_choice(cx: &mut TestAppContext) { + let (panel, cx) = panel(cx); + let found = folder(&["alpha", "beta"]); + panel.update(cx, |panel, cx| { + assert_eq!(panel.checkouts_found(Some(found), cx), None); + }); + cx.run_until_parked(); + assert!(cx.debug_bounds("pr-repo-alpha").is_some(), "asked at first"); + assert!(cx.debug_bounds("pr-list-refresh").is_none()); + + click(cx, "pr-repo-beta"); + let (alpha, beta) = ( + RepoRef::new("github.com", "team", "alpha"), + RepoRef::new("github.com", "team", "beta"), + ); + panel.read_with(cx, |panel, _| assert_eq!(panel.repo, Some(beta.clone()))); + assert!( + cx.debug_bounds("pr-repo-picker").is_some(), + "chosen again here" + ); + assert!(cx.debug_bounds("pr-list-refresh").is_some()); + + // A tab stays open when the list moves on; it is the one of its repository. + panel.update(cx, |panel, cx| panel.open(beta.pull_request(7), cx)); + cx.run_until_parked(); + click(cx, "pr-tab-list"); + click(cx, "pr-repo-picker"); + click(cx, "pr-repo-alpha"); + panel.update(cx, |panel, cx| { + assert_eq!(panel.repo, Some(alpha.clone())); + assert!(panel.detail().is_none(), "the list of the one chosen shows"); + panel.open(alpha.pull_request(7), cx); + let open: Vec<_> = panel.tabs.keys().cloned().collect(); + assert_eq!(open, [beta.pull_request(7), alpha.pull_request(7)]); + let shown = panel.detail().unwrap(); + assert_eq!(shown.read(cx).pr(), &alpha.pull_request(7)); + }); + } + + #[gpui_kit::test] + fn a_long_list_of_repositories_scrolls(cx: &mut TestAppContext) { + fn scrolls(cx: &mut gpui_kit::VisualTestContext) -> bool { + let before = cx.debug_bounds("pr-repo-repo000").unwrap(); + cx.simulate_event(gpui_kit::ScrollWheelEvent { + position: before.center(), + delta: gpui_kit::ScrollDelta::Pixels(gpui_kit::point( + gpui_kit::px(0.), + gpui_kit::px(-40.), + )), + ..Default::default() + }); + cx.run_until_parked(); + cx.debug_bounds("pr-repo-repo000").unwrap().top() < before.top() + } + + let (panel, cx) = panel(cx); + let names: Vec = (0..200).map(|n| format!("repo{n:03}")).collect(); + let names: Vec<&str> = names.iter().map(String::as_str).collect(); + panel.update(cx, |panel, cx| { + assert_eq!(panel.checkouts_found(Some(folder(&names)), cx), None); + }); + cx.run_until_parked(); + assert!(scrolls(cx), "in place of the list"); + + click(cx, "pr-repo-repo001"); + click(cx, "pr-repo-picker"); + let top = cx.debug_bounds("pr-repo-repo000").unwrap().top(); + assert!(scrolls(cx), "in the popover"); + + click(cx, "pr-repo-picker"); + click(cx, "pr-repo-picker"); + let reopened = cx.debug_bounds("pr-repo-repo000").unwrap().top(); + assert_eq!(reopened, top, "opens at the top again"); + } + + #[gpui_kit::test] + fn a_short_list_of_repositories_shows_whole_in_the_popover(cx: &mut TestAppContext) { + let (panel, cx) = panel(cx); + panel.update(cx, |panel, cx| { + let found = folder(&["alpha", "beta", "gamma"]); + assert_eq!(panel.checkouts_found(Some(found), cx), None); + }); + cx.run_until_parked(); + click(cx, "pr-repo-alpha"); + click(cx, "pr-repo-picker"); + + // A click only lands on what the list does not clip. + click(cx, "pr-repo-gamma"); + panel.read_with(cx, |panel, _| { + assert_eq!(panel.repo.as_ref().map(|r| r.name.as_str()), Some("gamma")); + }); + } + + #[gpui_kit::test] + fn one_repository_offers_no_choice(cx: &mut TestAppContext) { + let (panel, cx) = panel(cx); + panel.update(cx, |panel, cx| { + assert_eq!(panel.checkouts_found(Some(folder(&["alpha"])), cx), None); + }); + cx.run_until_parked(); + assert!(cx.debug_bounds("pr-list-refresh").is_some()); + assert!(cx.debug_bounds("pr-repo-picker").is_none()); + } + #[gpui_kit::test] fn the_tab_row_switches_tabs(cx: &mut TestAppContext) { let (panel, cx) = panel(cx); @@ -818,7 +1088,7 @@ mod tests { cx, ) }); - panel.open(290, cx); + panel.open(repo().pull_request(290), cx); }); let listed = |panel: &Entity, cx: &mut VisualTestContext| { panel.read_with(cx, |panel, cx| panel.list().read(cx).shows(290)) diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/preferences.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/preferences.rs index 43d33f10..4e6d111d 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/preferences.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/preferences.rs @@ -1,6 +1,7 @@ //! What the pull request view remembers across restarts: how the list is -//! sorted and filtered, the merge method last used per repository, and how -//! the Code tab shows diffs. +//! sorted and filtered, the merge method last used per repository, the +//! repositories last chosen per folder of several, and how the Code tab +//! shows diffs. //! Kept with the other UI settings ([`UiSettings`](crate::shared::settings::UiSettings)). use super::list_logic::{Filters, SortMode}; @@ -9,6 +10,10 @@ use github::model::{Involvement, ListState, MergeMethod}; use gpui_kit::App; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +/// How many chosen repositories a folder remembers. +const RECENT_REPOS: usize = 8; #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] #[serde(default)] @@ -17,6 +22,10 @@ pub struct PullRequestPreferences { /// The merge method last used, by repository (`host/owner/name`). #[serde(skip_serializing_if = "BTreeMap::is_empty")] pub merge_methods: BTreeMap, + /// The repositories last chosen in a folder of several, by folder, + /// the last one first. + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + pub recent_repos: BTreeMap>, /// The Summary's comment filter and order, the Timeline's order. pub conversation: super::conversation::preferences::ConversationPreferences, pub code: CodePreferences, @@ -70,6 +79,18 @@ impl PullRequestPreferences { pub fn merge_method(&self, repo: &RepoRef) -> Option { self.merge_methods.get(&repo.to_string()).copied() } + + pub fn recent_repos(&self, folder: &Path) -> &[RepoRef] { + self.recent_repos.get(folder).map_or(&[], Vec::as_slice) + } + + /// `repo` was chosen in `folder`. + pub fn chose_repo(&mut self, folder: &Path, repo: &RepoRef) { + let recent = self.recent_repos.entry(folder.to_path_buf()).or_default(); + recent.retain(|other| other != repo); + recent.insert(0, repo.clone()); + recent.truncate(RECENT_REPOS); + } } /// The remembered preferences; the defaults before settings are loaded. @@ -90,6 +111,22 @@ mod tests { use crate::main_screen::right_panel::pull_request::fixtures::repo; use crate::main_screen::right_panel::pull_request::list_logic::DraftFilter; + #[test] + fn a_folder_remembers_its_last_choices_the_last_one_first() { + let mut preferences = PullRequestPreferences::default(); + let folder = Path::new("/folder"); + let named = |name: String| RepoRef::new("github.com", "team", name); + for n in 0..=RECENT_REPOS { + preferences.chose_repo(folder, &named(n.to_string())); + } + preferences.chose_repo(folder, &named("3".into())); + let recent = preferences.recent_repos(folder); + assert_eq!(recent.len(), RECENT_REPOS); + assert_eq!(recent[0], named("3".into())); + assert_eq!(recent[1], named(RECENT_REPOS.to_string())); + assert!(preferences.recent_repos(Path::new("/other")).is_empty()); + } + #[test] fn preferences_survive_a_round_trip_and_default_when_missing() { let mut preferences = PullRequestPreferences::default(); diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/repo_picker.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/repo_picker.rs new file mode 100644 index 00000000..504a2c82 --- /dev/null +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/repo_picker.rs @@ -0,0 +1,349 @@ +//! Which repository the pull request list shows, for a session whose folder +//! holds several checkouts: a search field over them. Shown in place of the +//! list until one is chosen, afterwards in a popover from its toolbar. + +use code_assistant_core::pull_requests::{RepoCheckout, SessionCheckouts}; +use github::RepoRef; +use gpui_kit::component::input::{Input, InputEvent, InputState}; +use gpui_kit::component::scroll::ScrollableElement; +use gpui_kit::component::{ActiveTheme, Sizable, Size}; +use gpui_kit::{ + Context, Entity, EventEmitter, Render, ScrollHandle, Subscription, Window, div, point, + prelude::*, px, svg, +}; + +/// The repository to list instead. +pub struct RepoPicked(pub RepoRef); + +/// The repository a session lists at first: the one it listed last, else +/// the one of its folder itself (as without any others), else the one last +/// chosen in the folder, else the only one. `None` leaves the choice open. +pub fn first_focus<'a>( + found: &'a SessionCheckouts, + listed_last: Option<&RepoRef>, + recent: &[RepoRef], +) -> Option<&'a RepoCheckout> { + let checkouts = &found.checkouts; + let of = |repo: &RepoRef| checkouts.iter().find(|checkout| checkout.repo == *repo); + listed_last + .and_then(of) + .or_else(|| checkouts.iter().find(|c| c.path == found.directory)) + .or_else(|| recent.iter().find_map(of)) + .or(match checkouts.as_slice() { + [only] => Some(only), + _ => None, + }) +} + +/// The ones chosen recently first, the others as they came. +pub fn recent_first(mut checkouts: Vec, recent: &[RepoRef]) -> Vec { + checkouts.sort_by_key(|checkout| { + recent + .iter() + .position(|repo| *repo == checkout.repo) + .unwrap_or(recent.len()) + }); + checkouts +} + +/// The search matches the repository's or the directory's name, its owner +/// or the branch, ignoring case. +fn matches(checkout: &RepoCheckout, query: &str) -> bool { + let query = query.trim().to_lowercase(); + let directory = checkout.path.file_name().map(|name| name.to_string_lossy()); + query.is_empty() + || [ + Some(checkout.repo.full_name().as_str()), + directory.as_deref(), + checkout.branch.as_deref(), + ] + .into_iter() + .flatten() + .any(|text| text.to_lowercase().contains(&query)) +} + +pub struct RepoPicker { + checkouts: Vec, + selected: Option, + search: Entity, + query: String, + list_scroll: ScrollHandle, + _search: Subscription, +} + +impl EventEmitter for RepoPicker {} + +impl RepoPicker { + pub fn new(window: &mut Window, cx: &mut Context) -> Self { + let search = cx.new(|cx| InputState::new(window, cx).placeholder("Search repositories")); + let subscription = cx.subscribe( + &search, + |this, search, event: &InputEvent, cx| match event { + InputEvent::Change => { + this.query = search.read(cx).value().to_string(); + cx.notify(); + } + InputEvent::PressEnter { .. } => { + if let Some(first) = this.shown().next() { + cx.emit(RepoPicked(first.repo.clone())); + } + } + _ => {} + }, + ); + Self { + checkouts: Vec::new(), + selected: None, + search, + query: String::new(), + list_scroll: ScrollHandle::new(), + _search: subscription, + } + } + + pub fn set_checkouts(&mut self, checkouts: Vec, cx: &mut Context) { + self.checkouts = checkouts; + cx.notify(); + } + + pub fn set_selected(&mut self, selected: Option, cx: &mut Context) { + self.selected = selected; + cx.notify(); + } + + pub fn offers_a_choice(&self) -> bool { + self.checkouts.len() > 1 + } + + /// Start a new search, ready for typing. + pub fn start_search(&mut self, window: &mut Window, cx: &mut Context) { + // `set_value` reports no change, so the query does not follow by itself. + self.query.clear(); + self.list_scroll.set_offset(point(px(0.), px(0.))); + self.search.update(cx, |search, cx| { + search.set_value("", window, cx); + search.focus(window, cx); + }); + } + + fn shown(&self) -> impl Iterator { + self.checkouts + .iter() + .filter(|checkout| matches(checkout, &self.query)) + } + + fn render_row( + &self, + checkout: &RepoCheckout, + cx: &mut Context, + ) -> impl IntoElement + use<> { + let theme = cx.theme(); + let muted = theme.muted_foreground; + let repo = checkout.repo.clone(); + let place = match repo.host.as_str() { + "github.com" => repo.owner.clone(), + host => format!("{} · {host}", repo.owner), + }; + let selected = self.selected.as_ref() == Some(&repo); + let selector = format!("pr-repo-{}", repo.name); + let id = repo.to_string(); + div() + .id(id) + .debug_selector(move || selector) + .px_2() + .py_1() + .rounded_md() + .flex() + .items_center() + .gap_2() + .text_xs() + .cursor_pointer() + .hover(|s| s.bg(theme.muted)) + .child( + div() + .flex_none() + .text_color(theme.foreground) + .child(repo.name.clone()), + ) + .child( + div() + .flex_1() + .min_w_0() + .truncate() + .text_color(muted) + .child(place), + ) + .when_some(checkout.branch.clone(), |row, branch| { + row.child( + div() + .min_w_0() + .flex() + .items_center() + .gap_1() + .text_color(muted) + .child( + svg() + .flex_none() + .size(px(12.)) + .path("icons/git_branch.svg") + .text_color(muted), + ) + .child(div().min_w_0().truncate().child(branch)), + ) + }) + .when(selected, |row| { + row.child( + svg() + .flex_none() + .size(px(13.)) + .path("icons/check.svg") + .text_color(theme.foreground), + ) + }) + .on_click(cx.listener(move |_, _, _, cx| cx.emit(RepoPicked(repo.clone())))) + } +} + +impl Render for RepoPicker { + fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { + let muted = cx.theme().muted_foreground; + let shown: Vec = self.shown().cloned().collect(); + // Until one is chosen this is the whole view, else a popover. + let in_place = self.selected.is_none(); + div() + .flex() + .flex_col() + .gap_1() + .when(in_place, |el| el.size_full().px_4().py_3()) + .when(!in_place, |el| el.w(px(320.)).max_h(px(360.))) + .when(in_place, |el| { + el.child( + div() + .pb_1() + .text_sm() + .text_color(muted) + .child("Which repository's pull requests?"), + ) + }) + .child(Input::new(&self.search).with_size(Size::Small)) + .child( + div() + .relative() + .flex_1() + .min_h_0() + .flex() + .flex_col() + .child( + div() + .id("pr-repos") + .flex_1() + .min_h_0() + .track_scroll(&self.list_scroll) + .overflow_y_scroll() + .when(shown.is_empty(), |el| { + el.child( + div() + .px_2() + .py_1() + .text_xs() + .text_color(muted) + .child("No repository matches that."), + ) + }) + .children(shown.iter().map(|checkout| self.render_row(checkout, cx))), + ) + .vertical_scrollbar(&self.list_scroll), + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + fn repo(name: &str) -> RepoRef { + RepoRef::new("github.com", "team", name) + } + + fn checkout(name: &str) -> RepoCheckout { + RepoCheckout { + path: PathBuf::from("/folder").join(name), + repo: repo(name), + branch: None, + } + } + + fn found(checkouts: Vec) -> SessionCheckouts { + SessionCheckouts { + directory: PathBuf::from("/folder"), + checkouts, + } + } + + fn focus(found: &SessionCheckouts, last: Option<&str>, recent: &[&str]) -> Option { + let last = last.map(repo); + let recent: Vec = recent.iter().map(|name| repo(name)).collect(); + first_focus(found, last.as_ref(), &recent).map(|c| c.repo.name.clone()) + } + + #[test] + fn a_folder_of_several_leaves_the_first_choice_open() { + let several = found(vec![checkout("a"), checkout("b")]); + assert_eq!(focus(&several, None, &[]), None); + assert_eq!(focus(&several, None, &["gone"]), None); + } + + #[test] + fn what_was_listed_last_comes_back_then_the_folders_last_choice() { + let several = found(vec![checkout("a"), checkout("b"), checkout("c")]); + assert_eq!(focus(&several, Some("b"), &["c"]).as_deref(), Some("b")); + assert_eq!(focus(&several, None, &["gone", "c"]).as_deref(), Some("c")); + assert_eq!(focus(&several, Some("gone"), &[]), None); + } + + #[test] + fn a_folder_that_is_a_repository_itself_lists_that_one_first() { + let root = RepoCheckout { + path: PathBuf::from("/folder"), + ..checkout("root") + }; + let found = found(vec![root, checkout("nested")]); + assert_eq!(focus(&found, None, &["nested"]).as_deref(), Some("root")); + assert_eq!( + focus(&found, Some("nested"), &[]).as_deref(), + Some("nested") + ); + } + + #[test] + fn a_single_checkout_is_the_focus() { + assert_eq!( + focus(&found(vec![checkout("a")]), None, &[]).as_deref(), + Some("a") + ); + } + + #[test] + fn recent_ones_come_first_the_others_as_they_came() { + let ordered = recent_first( + vec![checkout("a"), checkout("b"), checkout("c"), checkout("d")], + &[repo("c"), repo("gone"), repo("b")], + ); + let names: Vec<_> = ordered.iter().map(|c| c.repo.name.as_str()).collect(); + assert_eq!(names, ["c", "b", "a", "d"]); + } + + #[test] + fn the_search_matches_names_owner_and_branch() { + let tool = RepoCheckout { + path: PathBuf::from("/folder/checked-out-as"), + branch: Some("feature/x".into()), + ..checkout("tool") + }; + for query in ["", " TOOL ", "team/", "checked-out", "feature"] { + assert!(matches(&tool, query), "{query}"); + } + assert!(!matches(&tool, "other")); + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/stack.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/stack.rs index 60edfbae..f4db9a2c 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/stack.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/stack.rs @@ -8,6 +8,7 @@ use super::PullRequestIntent; use super::actions::{allowed_merge_methods, can}; use super::header::DetailHeader; use super::presentation::{Glyph, Tone}; +use github::RepoRef; use github::model::{Detail, PullRequestAction, PullRequestState, Stack, StackHead, StackLayer}; use gpui_kit::component::button::{Button, ButtonVariants}; use gpui_kit::component::popover::{Popover, PopoverState}; @@ -381,6 +382,7 @@ pub fn render_stack_controls( let weak = cx.entity().downgrade(); let stack = stack.clone(); let number = detail.number; + let repo = header.repo().clone(); let notice = view.notice(); Popover::new("pr-stack") .anchor(gpui_kit::Anchor::TopRight) @@ -388,6 +390,7 @@ pub fn render_stack_controls( .content(move |state, window, cx| { stack_menu( &stack, + &repo, number, StackMenuOptions { notice, @@ -450,6 +453,7 @@ struct StackMenuOptions { #[allow(clippy::too_many_arguments)] fn stack_menu( stack: &Stack, + repo: &RepoRef, number: u64, options: StackMenuOptions, header: &WeakEntity, @@ -529,7 +533,10 @@ fn stack_menu( .hover(|d| d.bg(hover)) .child(layer_row(layer, current, false, cx)) .when(!current, |row| { - row.on_click(send(Some(PullRequestIntent::Open(layer.number)), None)) + row.on_click(send( + Some(PullRequestIntent::Open(repo.pull_request(layer.number))), + None, + )) }) })); menu = menu.child(layers).child(base_row(&stack.base, cx)); diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_strip.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_strip.rs index 7c8fc643..e34d0b78 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_strip.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_strip.rs @@ -6,6 +6,7 @@ use super::presentation::Tone; use super::{OpenTab, PullRequestPanel}; +use github::{PullRequestRef, RepoRef}; use gpui_kit::component::button::{Button, ButtonVariants}; use gpui_kit::component::menu::{ContextMenuExt, PopupMenuItem}; use gpui_kit::component::{ActiveTheme, Icon, Sizable}; @@ -78,7 +79,7 @@ impl PullRequestPanel { .tabs .iter() .enumerate() - .map(|(ix, (number, tab))| self.render_tab(ix, number, tab, cx)) + .map(|(ix, (pr, tab))| self.render_tab(ix, pr, tab, cx)) .collect(); let overflow = self.strip_scroll.max_offset().x > px(0.); let arrow = @@ -131,24 +132,25 @@ impl PullRequestPanel { fn render_tab( &self, ix: usize, - number: u64, + pr: &PullRequestRef, tab: &OpenTab, cx: &mut Context, ) -> AnyElement { let theme = cx.theme(); let active = self.tabs.active() == Some(ix); - let label: SharedString = format!("#{number}").into(); - let (icon, color) = match tab_tone(self, number, tab, cx) { + let number = pr.number; + let label = tab_label(pr, self.repo.as_ref(), self.tabs.iter().map(|(pr, _)| pr)); + let (icon, color) = match tab_tone(self, pr, tab, cx) { Some(tone) => (tone.icon(), tone.color(cx)), None => ( "icons/git_pull_request.svg", theme.muted_foreground.opacity(0.6), ), }; - let group: SharedString = format!("pr-tab-{number}").into(); + let group: SharedString = format!("pr-tab-{pr}").into(); let line = theme.drag_border; let panel = cx.entity().downgrade(); - let close = Button::new(SharedString::from(format!("pr-tab-close-{number}"))) + let close = Button::new(SharedString::from(format!("pr-tab-close-{pr}"))) .ghost() .xsmall() .icon(Icon::default().path("icons/close.svg")) @@ -186,7 +188,7 @@ impl PullRequestPanel { .group(group) .debug_selector(move || format!("pr-tab-{number}")) .flex_none() - .max_w(px(144.)) + .max_w(px(192.)) .h(px(24.)) .pl_1() .pr_2() @@ -249,9 +251,49 @@ impl PullRequestPanel { } /// A tab's state glyph: from its page once read, else the list's row. -fn tab_tone(panel: &PullRequestPanel, number: u64, tab: &OpenTab, cx: &App) -> Option { +fn tab_tone( + panel: &PullRequestPanel, + pr: &PullRequestRef, + tab: &OpenTab, + cx: &App, +) -> Option { tab.view.read(cx).tone(cx).or_else(|| { - let entry = panel.list.read(cx).entry(number)?; + if panel.repo.as_ref() != Some(&pr.repo) { + return None; + } + let entry = panel.list.read(cx).entry(pr.number)?; Some(Tone::of(entry.state, entry.is_draft)) }) } + +/// "#290", or "tool #290" as soon as a number alone would not say which +/// repository a tab is of: some tab is not of the one listed. +fn tab_label<'a>( + pr: &PullRequestRef, + listed: Option<&RepoRef>, + mut open: impl Iterator, +) -> SharedString { + if open.all(|other| Some(&other.repo) == listed) { + format!("#{}", pr.number).into() + } else { + format!("{} #{}", pr.repo.name, pr.number).into() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_tab_names_its_repository_once_a_number_alone_would_not_say() { + let (tool, other) = ( + RepoRef::new("github.com", "team", "tool"), + RepoRef::new("github.com", "team", "other"), + ); + let (a, b) = (tool.pull_request(1), other.pull_request(2)); + assert_eq!(tab_label(&a, Some(&tool), [&a].into_iter()), "#1"); + assert_eq!(tab_label(&a, Some(&other), [&a].into_iter()), "tool #1"); + assert_eq!(tab_label(&a, Some(&tool), [&a, &b].into_iter()), "tool #1"); + assert_eq!(tab_label(&a, None, [&a].into_iter()), "tool #1"); + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/tabs.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/tabs.rs index d7c5778f..a7b649cb 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/tabs.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/tabs.rs @@ -4,15 +4,14 @@ //! else the new last, else the list; tabs move by drag and drop. Each tab //! keeps its page (and so its scroll, sub-tab, drafts) while it is open. -/// Open tabs by pull request number, and which one shows (`None`: the -/// list). +/// Open tabs by pull request `K`, and which one shows (`None`: the list). #[derive(Debug)] -pub struct Tabs { - items: Vec<(u64, T)>, +pub struct Tabs { + items: Vec<(K, T)>, active: Option, } -impl Default for Tabs { +impl Default for Tabs { fn default() -> Self { Self { items: Vec::new(), @@ -21,18 +20,18 @@ impl Default for Tabs { } } -impl Tabs { +impl Tabs { #[cfg(test)] - pub fn numbers(&self) -> impl Iterator + '_ { - self.items.iter().map(|(number, _)| *number) + pub fn keys(&self) -> impl Iterator { + self.items.iter().map(|(key, _)| key) } - pub fn iter(&self) -> impl Iterator { - self.items.iter().map(|(number, item)| (*number, item)) + pub fn iter(&self) -> impl Iterator { + self.items.iter().map(|(key, item)| (key, item)) } - pub fn position(&self, number: u64) -> Option { - self.items.iter().position(|(n, _)| *n == number) + pub fn position(&self, key: &K) -> Option { + self.items.iter().position(|(k, _)| k == key) } /// The shown tab's index; `None` while the list shows. @@ -40,16 +39,16 @@ impl Tabs { self.active } - pub fn active_item(&self) -> Option<(u64, &T)> { - let (number, item) = self.items.get(self.active?)?; - Some((*number, item)) + pub fn active_item(&self) -> Option<(&K, &T)> { + let (key, item) = self.items.get(self.active?)?; + Some((key, item)) } - /// Show the tab of `number`, made by `make` when it is not open yet + /// Show the tab of `key`, made by `make` when it is not open yet /// (appended at the end). - pub fn open(&mut self, number: u64, make: impl FnOnce() -> T) -> usize { - let ix = self.position(number).unwrap_or_else(|| { - self.items.push((number, make())); + pub fn open(&mut self, key: K, make: impl FnOnce() -> T) -> usize { + let ix = self.position(&key).unwrap_or_else(|| { + self.items.push((key, make())); self.items.len() - 1 }); self.active = Some(ix); @@ -82,8 +81,8 @@ impl Tabs { /// Close every tab `keep` rejects; the shown one stays shown if kept, /// else the one clicked (`fallback`) shows. fn close_where(&mut self, fallback: usize, mut keep: impl FnMut(usize) -> bool) -> Vec { - let shown = self.active.map(|ix| self.items[ix].0); - let clicked = self.items.get(fallback).map(|(n, _)| *n); + let shown = self.active.map(|ix| self.items[ix].0.clone()); + let clicked = self.items.get(fallback).map(|(key, _)| key.clone()); let mut closed = Vec::new(); let mut kept = Vec::new(); for (ix, item) in std::mem::take(&mut self.items).into_iter().enumerate() { @@ -95,8 +94,8 @@ impl Tabs { self.items = kept; // The list stays shown; a closed tab hands over to the clicked one. self.active = shown.and_then(|shown| { - self.position(shown) - .or_else(|| clicked.and_then(|n| self.position(n))) + self.position(&shown) + .or_else(|| clicked.and_then(|key| self.position(&key))) .or_else(|| self.items.len().checked_sub(1)) }); closed @@ -124,10 +123,10 @@ impl Tabs { return; } let to = to.min(self.items.len() - 1); - let shown = self.active.map(|ix| self.items[ix].0); + let shown = self.active.map(|ix| self.items[ix].0.clone()); let item = self.items.remove(from); self.items.insert(to, item); - self.active = shown.and_then(|n| self.position(n)); + self.active = shown.and_then(|key| self.position(&key)); } } @@ -135,7 +134,7 @@ impl Tabs { mod tests { use super::*; - fn tabs(numbers: &[u64]) -> Tabs<()> { + fn tabs(numbers: &[u64]) -> Tabs { let mut tabs = Tabs::default(); for &n in numbers { tabs.open(n, || ()); @@ -143,8 +142,11 @@ mod tests { tabs } - fn shape(tabs: &Tabs<()>) -> (Vec, Option) { - (tabs.numbers().collect(), tabs.active_item().map(|(n, _)| n)) + fn shape(tabs: &Tabs) -> (Vec, Option) { + ( + tabs.keys().copied().collect(), + tabs.active_item().map(|(n, _)| *n), + ) } #[test] diff --git a/crates/ui_gpui/src/main_screen/tests/pull_requests.rs b/crates/ui_gpui/src/main_screen/tests/pull_requests.rs index 4d0f5355..3004cf54 100644 --- a/crates/ui_gpui/src/main_screen/tests/pull_requests.rs +++ b/crates/ui_gpui/src/main_screen/tests/pull_requests.rs @@ -123,6 +123,85 @@ fn the_tab_lists_the_projects_pull_requests_and_refreshes_them(cx: &mut TestAppC test.wait_until("the list was read anew", |_| searches(&fake) > read); } +#[gpui_kit::test] +fn a_folder_of_checkouts_lists_the_repository_chosen(cx: &mut TestAppContext) { + let fake = scripted_github(None); + let folder = tempfile::tempdir().unwrap(); + for name in ["alpha", "beta"] { + let checkout = folder.path().join(name); + std::fs::create_dir(&checkout).unwrap(); + git(&checkout, &["init", "-q", "-b", "main"]); + let remote = format!("git@github.com:team/{name}.git"); + git(&checkout, &["remote", "add", "origin", &remote]); + } + let core = TestCore::with_github(PendingLLMProvider::default().into_factory(), &fake); + let config = SessionConfig { + init_path: Some(folder.path().to_path_buf()), + ..SessionConfig::default() + }; + let session_id = core + .block_on(core.service.create_session_with_config(None, config, None)) + .unwrap(); + let other = core + .block_on(core.service.create_session(None, None)) + .unwrap(); + let mut test = MainScreenTest::with_core(cx, core); + test.gpui.cmd_load_session(session_id.clone(), None); + test.wait_until("the session is shown", |test| { + test.gpui.get_current_session_id().as_deref() == Some(session_id.as_str()) + }); + let searched = |name: &str| { + let repo = format!("repo:team/{name}"); + fake.requests().iter().any(|request| { + request.operation.as_deref() == Some("PullRequestSearch") + && request + .variables + .as_ref() + .is_some_and(|variables| variables.to_string().contains(&repo)) + }) + }; + + test.click("right-panel-view-PR"); + test.wait_until("the choice is offered", |test| test.shows("pr-repo-beta")); + assert!(searches(&fake) == 0, "nothing is read before a choice"); + + test.click("pr-repo-beta"); + test.wait_until("beta is listed", |_| searched("beta")); + assert!(!searched("alpha")); + test.wait_until("the list shows", |test| test.shows("pr-row-291")); + + test.click("pr-repo-picker"); + test.wait_until("the others are offered", |test| test.shows("pr-repo-alpha")); + + // A search does not outlive the popover it was typed in. + test.cx.simulate_input("beta"); + test.wait_until("the search narrows them", |test| { + !test.shows("pr-repo-alpha") + }); + test.click("pr-repo-picker"); + test.wait_until("the popover closes", |test| !test.shows("pr-repo-beta")); + test.click("pr-repo-picker"); + test.wait_until("the popover opens again", |test| test.shows("pr-repo-beta")); + test.wait_until("all are offered again", |test| test.shows("pr-repo-alpha")); + + test.click("pr-repo-alpha"); + test.wait_until("alpha is listed", |_| searched("alpha")); + + // The session comes back to the repository it listed, with no question. + test.gpui.cmd_load_session(other.clone(), None); + test.wait_until("the other session is shown", |test| { + test.gpui.get_current_session_id().as_deref() == Some(other.as_str()) + }); + test.gpui.cmd_load_session(session_id.clone(), None); + test.wait_until("the session is shown again", |test| { + test.gpui.get_current_session_id().as_deref() == Some(session_id.as_str()) + }); + test.wait_until("its repository is listed", |test| { + test.shows("pr-repo-picker") + }); + assert!(!test.shows("pr-repo-alpha"), "no question"); +} + #[gpui_kit::test] fn the_sessions_pull_request_shows_and_takes_a_comment(cx: &mut TestAppContext) { let fake = scripted_github(Some(291)); diff --git a/crates/ui_gpui/src/shared/ui_state.rs b/crates/ui_gpui/src/shared/ui_state.rs index 40b8faee..018d26a6 100644 --- a/crates/ui_gpui/src/shared/ui_state.rs +++ b/crates/ui_gpui/src/shared/ui_state.rs @@ -12,6 +12,7 @@ use anyhow::Result; use code_assistant_core::persistence::SessionLayout; +use github::RepoRef; use gpui_kit::App; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap, HashSet}; @@ -98,23 +99,31 @@ pub struct UiSessionState { pub pull_request_tabs: PullRequestTabsState, } -/// The right panel's pull request tabs for one session: in order, each -/// with its sub-tab, and which one shows (`None`: the list). +/// The right panel's pull request view for one session: the repository +/// listed, the tabs in order, each with its sub-tab, and which one shows +/// (`None`: the list). A tab without a repository, from before tabs could +/// show several, belongs to the repository listed. #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct PullRequestTabsState { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub repo: Option, pub open: Vec, #[serde(default, skip_serializing_if = "Option::is_none")] pub active: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub active_repo: Option, } impl PullRequestTabsState { fn is_empty(&self) -> bool { - self.open.is_empty() + self.repo.is_none() && self.open.is_empty() } } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct PullRequestTabState { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub repo: Option, pub number: u64, /// "summary", "timeline" or "code". #[serde(default, skip_serializing_if = "Option::is_none")] @@ -483,6 +492,17 @@ mod tests { ) } + #[test] + fn pull_request_tabs_stored_before_they_named_a_repository_still_read() { + let stored = r#"{"pull_request_tabs":{"open":[{"number":5,"tab":"code"}],"active":5}}"#; + let state: UiSessionState = serde_json::from_str(stored).unwrap(); + let tabs = state.pull_request_tabs; + assert_eq!(tabs.repo, None); + assert_eq!(tabs.open[0].repo, None); + assert_eq!(tabs.open[0].number, 5); + assert_eq!(tabs.active, Some(5)); + } + #[test] fn test_get_returns_default_for_unknown_session() { let (mut store, _) = test_store(); diff --git a/docs/pull-request-view.md b/docs/pull-request-view.md index 7d63fc1a..3f2eb315 100644 --- a/docs/pull-request-view.md +++ b/docs/pull-request-view.md @@ -116,6 +116,13 @@ deleted modes, binary markers, `\ No newline at end of file`, CRLF) and unless it is the base branch. The session's own PR comes from the lifecycle snapshot (number) or a head-branch lookup (newest PR not from a fork, cached 60 s). +- **Several repositories**: a project folder may hold checkouts instead of + being one (`session_checkouts`, `pull_requests::github_checkouts`): the + folder's own repository, then the checkouts directly below it by directory + name; hidden directories (so `.worktrees`) are not looked into, a + repository checked out twice counts once. Only local git is read. + `session_pull_requests` stays about the session's directory itself; a + checkout below has no pull request "of the session". - **Client**: one `PullRequests` store per `SessionService`, built from `AgentRuntimeOptions::pull_requests` (`PullRequestSource {client, snapshot_dir}`; `None` = `github::Client::new()` and @@ -184,10 +191,13 @@ deleted modes, binary markers, `\ No newline at end of file`, CRLF) and (no remote added), tracking the fork's URL and branch when maintainers may push, else the pull request's ref; a fork's branch named like the default (or base) branch becomes `owner/branch`; an existing branch only - fast-forwards. *Here* refuses tracked changes; *in a worktree* reuses the - branch's worktree or adds one beside the project, and a new session for - the project works in it. The session knows the pull request as its own - (lifecycle snapshot). + fast-forwards. It happens in the checkout of the pull request's + repository, found as above. *Here* refuses tracked changes; *in a worktree* + reuses the branch's worktree or adds one beside the checkout, and a new + session for the project works in it, on that one repository. The session + knows the pull request as its own (lifecycle snapshot), except after *here* + in a checkout below its folder: a session spanning several repositories + has no one branch. - **Stack rebase** (`git::stack_rebase`, after T3's cascade): in a scratch repository each open layer is replayed onto the rebased one below and pushed with a lease on the head it had; the credential goes through @@ -210,8 +220,13 @@ pub struct Fetched { pub value: T, pub fetched_at: SystemTime, pub refreshing pub enum PullRequestPart { Detail, Checks, Activity, Diff, FilesViewed, Stack } -// Where a session's pull requests live and which one is its own; `None` -// without a project or a GitHub remote. +// The repositories of the session's folder; `None` without a project. +async fn session_checkouts(&self, session_id: String) -> Result>; +// SessionCheckouts { directory: PathBuf, checkouts: Vec } +// RepoCheckout { path: PathBuf, repo: RepoRef, branch: Option } + +// Where the pull requests of the session's directory live and which one is +// its own; `None` without a project or a GitHub remote. async fn session_pull_requests(&self, session_id: String) -> Result>; // SessionPullRequests { repo: RepoRef, head_branch: Option, own: Option } @@ -267,6 +282,19 @@ and their sub-tabs are remembered per session; without any, the session's own PR opens. Detail tabs: **Summary**, **Timeline**, **Code** (T3's Guide tab, an agent-written review tour, is out of scope). +A folder of several checkouts offers a choice of repository +(`repo_picker.rs`), one repository none at all. The first time, the choice +fills the view: a search field over the checkouts (Enter takes the first), +each row with name, owner, a host other than github.com, and the branch +unless it is the base branch. Afterwards a button above the list's search +field opens it as a popover, recently chosen ones first. The repository +listed is remembered per session (with the tabs); a new session starts on +the folder's own repository if it has one, else the one last chosen in the +folder (`PullRequestPreferences::recent_repos`). Choosing another moves the +list only, tabs stay open, as a change often spans repositories; as soon as +one is not of the repository listed, tabs read "tool #12". The checkouts are +read again when the tab shows again. + - **Header**: repo · `#N ↗` in the state colour; editable title; author · updated; `base ← head` (copy), files, `+A −D`; behind-by warning with "Update branch"; primary action (Merge with method / Ready for review /