From 15ee03008188d4345bb6d0be2674bb8c161985d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Fri, 30 Oct 2026 13:29:15 +0100 Subject: [PATCH 1/9] Approve the workflow runs of pull requests from forks GitHub holds the workflow runs of a fork's pull request until a maintainer approves them. Like T3 Code, the detail read lists the runs of the head waiting for approval (actions/runs?status=action_required) and shows them as checks; the checks poll revalidates the fork's runs with ETags too. The Summary tab offers "Approve workflows to run" in place of the checks while there are some (two clicks); approving reads the head again before each run and refuses a head other pull requests share. The checks popover lists the checks needing attention first, then the running ones, the completed ones behind "Show all". --- crates/github/src/convert/detail.rs | 3 + crates/github/src/error.rs | 4 + crates/github/src/model/checks.rs | 3 + crates/github/src/model/pull_request.rs | 8 + crates/github/src/ops/actions.rs | 1 + crates/github/src/ops/checks.rs | 48 ++- crates/github/src/ops/detail.rs | 11 +- crates/github/src/ops/mod.rs | 1 + crates/github/src/ops/workflows.rs | 299 ++++++++++++++++++ crates/github/tests/operations.rs | 176 ++++++++++- crates/ui_gpui/assets/icons/columns_2.svg | 14 + crates/ui_gpui/assets/icons/folder_tree.svg | 16 + crates/ui_gpui/assets/icons/pilcrow.svg | 15 + crates/ui_gpui/assets/icons/play.svg | 13 + crates/ui_gpui/assets/icons/refresh_cw.svg | 16 + crates/ui_gpui/assets/icons/rows_3.svg | 15 + crates/ui_gpui/assets/icons/text_wrap.svg | 16 + .../right_panel/pull_request/actions.rs | 47 ++- .../right_panel/pull_request/data.rs | 1 + .../right_panel/pull_request/detail.rs | 99 +++++- .../right_panel/pull_request/fixtures.rs | 1 + .../right_panel/pull_request/header.rs | 2 +- .../right_panel/pull_request/presentation.rs | 46 +++ .../right_panel/pull_request/summary.rs | 17 +- .../right_panel/pull_request/tab_bar.rs | 199 +++++++++--- 25 files changed, 998 insertions(+), 73 deletions(-) create mode 100644 crates/github/src/ops/workflows.rs create mode 100644 crates/ui_gpui/assets/icons/columns_2.svg create mode 100644 crates/ui_gpui/assets/icons/folder_tree.svg create mode 100644 crates/ui_gpui/assets/icons/pilcrow.svg create mode 100644 crates/ui_gpui/assets/icons/play.svg create mode 100644 crates/ui_gpui/assets/icons/refresh_cw.svg create mode 100644 crates/ui_gpui/assets/icons/rows_3.svg create mode 100644 crates/ui_gpui/assets/icons/text_wrap.svg diff --git a/crates/github/src/convert/detail.rs b/crates/github/src/convert/detail.rs index babb175d..e8151dae 100644 --- a/crates/github/src/convert/detail.rs +++ b/crates/github/src/convert/detail.rs @@ -56,6 +56,7 @@ pub(crate) fn viewer_permissions(access: &ViewerAccess) -> ViewerPermissions { } if access.can_write { actions.push(PullRequestAction::Merge); + actions.push(PullRequestAction::ApproveWorkflows); if access.auto_merge_allowed && !access.can_merge_now { actions.push(PullRequestAction::EnableAutoMerge); } @@ -391,6 +392,7 @@ pub(crate) fn core(data: CoreData, host: &str) -> Option<(Detail, bool)> { .as_ref() .and_then(|r| merge_method(r.merge_method.as_deref())), maintainer_can_modify: pr.maintainer_can_modify, + workflow_approvals: None, }; Some((detail, more_checks)) } @@ -529,6 +531,7 @@ mod tests { [ PullRequestAction::DeleteBranch, PullRequestAction::Merge, + PullRequestAction::ApproveWorkflows, PullRequestAction::EnableAutoMerge, PullRequestAction::DisableAutoMerge, PullRequestAction::Ready, diff --git a/crates/github/src/error.rs b/crates/github/src/error.rs index 139c66e1..82423c2b 100644 --- a/crates/github/src/error.rs +++ b/crates/github/src/error.rs @@ -103,6 +103,10 @@ pub enum Error { #[error("The named subject does not belong to this pull request.")] SubjectScope, + /// Not asked of GitHub: what it would do is not what was meant. + #[error("{0}")] + Refused(String), + #[error("{}", branch_deletion_message(*.0))] BranchDeletionRefused(BranchDeletionRefusal), diff --git a/crates/github/src/model/checks.rs b/crates/github/src/model/checks.rs index f9c51775..b87c8cbb 100644 --- a/crates/github/src/model/checks.rs +++ b/crates/github/src/model/checks.rs @@ -43,4 +43,7 @@ pub struct Checks { pub head_sha: Option, pub checks: Vec, pub checks_state: Option, + /// See `Detail::workflow_approvals`. + #[serde(default)] + pub workflow_approvals: Option, } diff --git a/crates/github/src/model/pull_request.rs b/crates/github/src/model/pull_request.rs index 44123608..6e862d12 100644 --- a/crates/github/src/model/pull_request.rs +++ b/crates/github/src/model/pull_request.rs @@ -155,6 +155,7 @@ pub enum PullRequestAction { UpdateBranch, EnableAutoMerge, DisableAutoMerge, + ApproveWorkflows, } /// An action on a pull request with what it needs. @@ -174,6 +175,8 @@ pub enum Action { Reopen, /// Deletes the source branch of a closed or merged pull request. DeleteBranch, + /// Lets the workflow runs of a pull request from a fork run. + ApproveWorkflows, } impl Action { @@ -188,6 +191,7 @@ impl Action { Action::Close => PullRequestAction::Close, Action::Reopen => PullRequestAction::Reopen, Action::DeleteBranch => PullRequestAction::DeleteBranch, + Action::ApproveWorkflows => PullRequestAction::ApproveWorkflows, } } } @@ -250,6 +254,10 @@ pub struct Detail { pub auto_merge_enabled: bool, pub auto_merge_method: Option, pub maintainer_can_modify: bool, + /// Workflow runs of an open pull request from a fork waiting for a + /// maintainer's approval; `None` when not asked or not known. + #[serde(default)] + pub workflow_approvals: Option, } /// Fields of a pull request to rewrite; `None` leaves one as it is. diff --git a/crates/github/src/ops/actions.rs b/crates/github/src/ops/actions.rs index 226c12df..806e5d88 100644 --- a/crates/github/src/ops/actions.rs +++ b/crates/github/src/ops/actions.rs @@ -74,6 +74,7 @@ impl Client { } match action { Action::DeleteBranch => self.delete_branch(pr).await, + Action::ApproveWorkflows => self.approve_workflows(pr).await, Action::UpdateBranch(method) => self.update_branch(pr, method).await, Action::Merge(method) | Action::EnableAutoMerge(method) => { let state = self.action_state(pr).await?; diff --git a/crates/github/src/ops/checks.rs b/crates/github/src/ops/checks.rs index 9b4adce3..462e9009 100644 --- a/crates/github/src/ops/checks.rs +++ b/crates/github/src/ops/checks.rs @@ -1,5 +1,6 @@ //! Polling checks cheaply. Before the full read, the pull request, its -//! head's check runs and its head's statuses are revalidated with ETags; a +//! head's check runs and its head's statuses (and for a fork its workflow +//! runs, which may wait for approval) are revalidated with ETags; a //! 304 costs no quota. When none of them changed and the last read is //! recent, that read is the answer. Hosts without these endpoints fall back //! to the full read. @@ -135,17 +136,34 @@ impl Client { } }; #[derive(Deserialize)] + struct Repo { + id: u64, + } + #[derive(Deserialize)] struct Head { sha: String, + repo: Option, + } + #[derive(Deserialize)] + struct Base { + repo: Option, } #[derive(Deserialize)] struct Pull { head: Head, + base: Option, } - let head = serde_json::from_slice::(&body) - .map_err(|e| Error::decode("revalidateChecks", e))? - .head - .sha; + let pull = serde_json::from_slice::(&body) + .map_err(|e| Error::decode("revalidateChecks", e))?; + // A fork's runs may wait for approval: those are no check runs yet. + let fork = match ( + &pull.head.repo, + pull.base.as_ref().and_then(|b| b.repo.as_ref()), + ) { + (Some(head), Some(base)) => head.id != base.id, + _ => false, + }; + let head = pull.head.sha; if entry.head_sha.as_deref() != Some(head.as_str()) { changed = true; entry.head_sha = Some(head.clone()); @@ -153,15 +171,22 @@ impl Client { let runs = format!("{root}/commits/{head}/check-runs?filter=all"); let statuses = format!("{root}/commits/{head}/status"); - let (runs, statuses) = futures::try_join!( + let workflows = format!("{root}/actions/runs?head_sha={head}&event=pull_request"); + let (runs, statuses, workflows) = futures::try_join!( self.probe_pages(host, &runs), - self.probe_pages(host, &statuses) + self.probe_pages(host, &statuses), + async { + match fork { + true => self.probe_pages(host, &workflows).await, + false => Ok(Ok(false)), + } + } )?; - match (runs, statuses) { - (Ok(runs_changed), Ok(statuses_changed)) => { - changed |= runs_changed || statuses_changed; + match (runs, statuses, workflows) { + (Ok(runs_changed), Ok(statuses_changed), Ok(workflows_changed)) => { + changed |= runs_changed || statuses_changed || workflows_changed; } - (Err(unsupported), _) | (_, Err(unsupported)) => { + (Err(unsupported), _, _) | (_, Err(unsupported), _) | (_, _, Err(unsupported)) => { entry.unsupported = unsupported; entry.value = None; return self.read_checks(pr).await; @@ -189,6 +214,7 @@ impl Client { head_sha: detail.head_sha, checks: detail.checks, checks_state: detail.checks_state, + workflow_approvals: detail.workflow_approvals, }) } } diff --git a/crates/github/src/ops/detail.rs b/crates/github/src/ops/detail.rs index 0d57a911..9f08f334 100644 --- a/crates/github/src/ops/detail.rs +++ b/crates/github/src/ops/detail.rs @@ -45,9 +45,16 @@ fn asks_required(host: &str) -> bool { } impl Client { - /// Everything about one pull request: state, people, labels, checks, - /// merge settings and what the viewer may do. + /// Everything about one pull request: state, people, labels, checks + /// (with the workflow runs of a fork waiting for approval), merge + /// settings and what the viewer may do. pub async fn detail(&self, pr: &PullRequestRef) -> Result { + let mut detail = self.detail_without_approvals(pr).await?; + self.add_workflow_approvals(pr, &mut detail).await?; + Ok(detail) + } + + pub(crate) async fn detail_without_approvals(&self, pr: &PullRequestRef) -> Result { let host = &pr.repo.host; let variables = |requested_reviewers: u32| -> Result { Ok(with( diff --git a/crates/github/src/ops/mod.rs b/crates/github/src/ops/mod.rs index 50bddfcf..6a566d5b 100644 --- a/crates/github/src/ops/mod.rs +++ b/crates/github/src/ops/mod.rs @@ -8,6 +8,7 @@ mod detail; mod diff; mod files; mod list; +mod workflows; mod write; use crate::client::Client; diff --git a/crates/github/src/ops/workflows.rs b/crates/github/src/ops/workflows.rs new file mode 100644 index 00000000..f9e7e077 --- /dev/null +++ b/crates/github/src/ops/workflows.rs @@ -0,0 +1,299 @@ +//! Workflow runs of a pull request from a fork that wait for a maintainer +//! to approve them, and approving them. Ported from T3 Code: the runs show +//! as checks ("Awaiting approval"), and approving re-reads the head before +//! each run so a push made meanwhile is never approved by accident. + +use super::ensure_addressable; +use crate::PullRequestRef; +use crate::client::{Client, RestCall}; +use crate::error::{Error, Result}; +use crate::model::{Check, CheckStatus, ChecksState, Detail, PullRequestState}; +use crate::transport::Method; +use serde::Deserialize; + +const RUNS_PAGE_SIZE: usize = 100; +/// Runs read at most; past it, approving is refused. +const RUNS_LIMIT: usize = 1000; + +/// A workflow run waiting for approval. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct WorkflowRun { + pub id: u64, + pub name: String, + pub url: Option, +} + +/// The id in a workflow run's URL (`…/actions/runs/123…`). +pub(crate) fn run_id_of(url: &str) -> Option { + let rest = &url[url.find("/actions/runs/")? + "/actions/runs/".len()..]; + let digits: String = rest.chars().take_while(char::is_ascii_digit).collect(); + digits.parse().ok() +} + +/// The head a fork's pull request is approved for. +struct ForkHead<'a> { + sha: &'a str, + branch: &'a str, + owner: &'a str, +} + +fn fork_head(detail: &Detail) -> Option> { + Some(ForkHead { + sha: detail.head_sha.as_deref()?, + branch: Some(detail.head_branch.as_str()).filter(|b| !b.is_empty())?, + owner: detail.head_repository_owner.as_deref()?, + }) +} + +/// Add the runs awaiting approval to `detail`'s checks (those its checks do +/// not show already) and count them; `None` leaves the count unknown and +/// says so with a check of its own. +pub(crate) fn apply_workflow_runs(detail: &mut Detail, runs: Option<&[WorkflowRun]>) { + let Some(runs) = runs else { + detail.workflow_approvals = None; + detail.checks.push(Check { + name: "Workflow approval status".into(), + status: CheckStatus::ActionRequired, + description: Some( + "GitHub could not determine whether workflows are awaiting approval.".into(), + ), + url: None, + required: None, + }); + detail.checks_state = merged_state(detail.checks_state); + return; + }; + detail.workflow_approvals = Some(runs.len() as u64); + let shown: Vec = detail + .checks + .iter() + .filter(|check| check.status == CheckStatus::ActionRequired) + .filter_map(|check| check.url.as_deref().and_then(run_id_of)) + .collect(); + let mut added = false; + for run in runs.iter().filter(|run| !shown.contains(&run.id)) { + detail.checks.push(Check { + name: run.name.clone(), + status: CheckStatus::ActionRequired, + description: Some("A maintainer must approve this workflow before it can run.".into()), + url: run.url.clone(), + required: None, + }); + added = true; + } + if added { + detail.checks_state = merged_state(detail.checks_state); + } +} + +/// The verdict once a check awaiting action joins: pending unless failing. +fn merged_state(state: Option) -> Option { + match state { + Some(ChecksState::Failing) => Some(ChecksState::Failing), + _ => Some(ChecksState::Pending), + } +} + +impl Client { + /// The runs of `head` on `branch` that wait for approval. + pub(crate) async fn workflow_runs_awaiting_approval( + &self, + pr: &PullRequestRef, + head: &str, + branch: &str, + ) -> Result> { + ensure_addressable(&pr.repo)?; + #[derive(Deserialize, Default)] + #[serde(default)] + struct RawRun { + id: Option, + name: Option, + html_url: Option, + } + #[derive(Deserialize, Default)] + #[serde(default)] + struct Runs { + workflow_runs: Vec, + } + let mut runs = Vec::new(); + for page in 1.. { + let path = format!( + "repos/{}/{}/actions/runs?head_sha={}&branch={}&event=pull_request&status=action_required&per_page={RUNS_PAGE_SIZE}&page={page}", + pr.repo.owner, + pr.repo.name, + urlencoding::encode(head), + urlencoding::encode(branch), + ); + let response = self + .rest(RestCall::get(&pr.repo.host, "workflowRuns", path)) + .await?; + let raw: Runs = serde_json::from_slice(&response.body) + .map_err(|e| Error::decode("workflowRuns", e))?; + let count = raw.workflow_runs.len(); + runs.extend(raw.workflow_runs.into_iter().filter_map(|run| { + let id = run.id?; + Some(WorkflowRun { + id, + name: run + .name + .map(|name| name.trim().to_string()) + .filter(|name| !name.is_empty()) + .unwrap_or_else(|| format!("Workflow run {id}")), + url: run.html_url.filter(|url| !url.trim().is_empty()), + }) + })); + if count < RUNS_PAGE_SIZE { + break; + } + if runs.len() >= RUNS_LIMIT { + return Err(Error::Refused( + "GitHub returned more than 1000 workflow runs awaiting approval.".into(), + )); + } + } + Ok(runs) + } + + /// For an open pull request from a fork: its runs awaiting approval, + /// added to its checks. A failed read leaves the count unknown, unless + /// GitHub limited the rate (that fails the read). + pub(crate) async fn add_workflow_approvals( + &self, + pr: &PullRequestRef, + detail: &mut Detail, + ) -> Result<()> { + if detail.state != PullRequestState::Open || !detail.is_cross_repository { + return Ok(()); + } + let runs = match fork_head(detail) { + Some(head) => { + match self + .workflow_runs_awaiting_approval(pr, head.sha, head.branch) + .await + { + Ok(runs) => Some(runs), + Err(e) if e.is_rate_limited() => return Err(e), + Err(e) => { + tracing::debug!("workflow runs awaiting approval not read: {e}"); + None + } + } + } + None => None, + }; + apply_workflow_runs(detail, runs.as_deref()); + Ok(()) + } + + /// Approve the workflow runs of a pull request from a fork. Before + /// each run the head is read again: a new push, or a head that names + /// another pull request too, is refused rather than approved. + pub(crate) async fn approve_workflows(&self, pr: &PullRequestRef) -> Result<()> { + let detail = self.detail_without_approvals(pr).await?; + if !detail.is_cross_repository { + return Ok(()); + } + let unavailable = || { + Error::Refused(format!( + "GitHub did not report a complete head revision for #{}.", + pr.number + )) + }; + let head = fork_head(&detail).ok_or_else(unavailable)?; + let (sha, branch, owner) = ( + head.sha.to_string(), + head.branch.to_string(), + head.owner.to_string(), + ); + self.ensure_unique_head(pr, &sha, &branch, &owner).await?; + let runs = self + .workflow_runs_awaiting_approval(pr, &sha, &branch) + .await?; + for run in runs { + let now = self.detail_without_approvals(pr).await?; + let same = now.is_cross_repository + && now.head_sha.as_deref() == Some(sha.as_str()) + && now.head_branch == branch + && now + .head_repository_owner + .as_deref() + .is_some_and(|o| o.eq_ignore_ascii_case(&owner)); + if !same { + return Err(Error::Refused(format!( + "The head revision of #{} changed before its workflows could be approved.", + pr.number + ))); + } + let still = self + .workflow_runs_awaiting_approval(pr, &sha, &branch) + .await?; + if !still.iter().any(|r| r.id == run.id) { + continue; + } + self.rest(RestCall::new( + &pr.repo.host, + "approveWorkflowRun", + Method::Post, + format!( + "repos/{}/{}/actions/runs/{}/approve", + pr.repo.owner, pr.repo.name, run.id + ), + )) + .await?; + } + Ok(()) + } + + /// Exactly one open pull request from that fork may have this head: + /// approving runs another one's workflows too otherwise. + async fn ensure_unique_head( + &self, + pr: &PullRequestRef, + sha: &str, + branch: &str, + owner: &str, + ) -> Result<()> { + let heads = self.pull_requests_for_head(&pr.repo, branch).await?; + let matching: Vec = heads + .iter() + .filter(|head| { + head.state == PullRequestState::Open + && head.is_cross_repository + && head.head_sha.as_deref() == Some(sha) + && head + .head_repository + .as_deref() + .and_then(|r| r.split('/').next()) + .is_some_and(|o| o.eq_ignore_ascii_case(owner)) + }) + .map(|head| head.number) + .collect(); + if matching != [pr.number] { + return Err(Error::Refused(format!( + "The head revision matched {} pull requests instead of uniquely matching #{}.", + matching.len(), + pr.number + ))); + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reads_the_run_id_from_its_url() { + assert_eq!( + run_id_of("https://github.com/o/n/actions/runs/123/job/4"), + Some(123) + ); + assert_eq!( + run_id_of("https://github.com/o/n/actions/runs/77"), + Some(77) + ); + assert_eq!(run_id_of("https://github.com/o/n/runs/77"), None); + assert_eq!(run_id_of("https://github.com/o/n/actions/runs/x"), None); + } +} diff --git a/crates/github/tests/operations.rs b/crates/github/tests/operations.rs index f68058a0..2141d7ae 100644 --- a/crates/github/tests/operations.rs +++ b/crates/github/tests/operations.rs @@ -2,8 +2,8 @@ use github::fake::{FakeResponse, FakeTransport}; use github::model::{ - Action, CheckStatus, DiffSide, EditableCommentKind, FileChange, Involvement, ListQuery, - ListState, MergeMethod, PullRequestState, PullRequestUpdate, ReactionContent, + Action, CheckStatus, ChecksState, DiffSide, EditableCommentKind, FileChange, Involvement, + ListQuery, ListState, MergeMethod, PullRequestState, PullRequestUpdate, ReactionContent, ReviewCommentDraft, ReviewSubmission, ReviewVerdict, Reviewer, ReviewerKind, UpdateMethod, }; use github::transport::Method; @@ -26,13 +26,20 @@ fn fixture(name: &str) -> Value { const HEAD: &str = "0123456789abcdef0123456789abcdef01234567"; +/// The fixture's pull request, from a branch of the repository itself (no +/// workflow runs of a fork to read). +fn own_branch_core() -> Value { + let mut core = fixture("pull_request_core.json"); + core["repository"]["pullRequest"]["isCrossRepository"] = false.into(); + core +} + // --- Detail ------------------------------------------------------------------ #[tokio::test] async fn reads_the_detail_with_the_variables_the_document_needs() { let fake = FakeTransport::new(); - fake.on_graphql("PullRequestCore") - .data(fixture("pull_request_core.json")); + fake.on_graphql("PullRequestCore").data(own_branch_core()); let detail = fake.client().detail(&pr()).await.unwrap(); assert_eq!(detail.number, 291); assert_eq!(detail.checks.len(), 3); @@ -67,7 +74,7 @@ async fn reads_the_detail_again_without_reviewers_when_a_team_is_refused() { #[tokio::test] async fn walks_every_check_context_past_the_first_hundred() { let fake = FakeTransport::new(); - let mut core = fixture("pull_request_core.json"); + let mut core = own_branch_core(); core["repository"]["pullRequest"]["commits"]["nodes"][0]["commit"]["statusCheckRollup"]["contexts"] ["pageInfo"]["hasNextPage"] = true.into(); fake.on_graphql("PullRequestCore").data(core); @@ -159,8 +166,7 @@ fn script_checks_endpoints(fake: &FakeTransport) { async fn polls_checks_with_etags_and_reads_in_full_only_when_something_changed() { let fake = FakeTransport::new(); script_checks_endpoints(&fake); - fake.on_graphql("PullRequestCore") - .data(fixture("pull_request_core.json")); + fake.on_graphql("PullRequestCore").data(own_branch_core()); let client = fake.client(); let first = client.checks(&pr()).await.unwrap(); @@ -191,8 +197,7 @@ async fn reads_checks_in_full_where_the_host_lacks_the_endpoints() { let fake = FakeTransport::new(); fake.on_rest(Method::Get, "repos/stippi/code-assistant/pulls/291") .respond(FakeResponse::status(404)); - fake.on_graphql("PullRequestCore") - .data(fixture("pull_request_core.json")); + fake.on_graphql("PullRequestCore").data(own_branch_core()); let client = fake.client(); client.checks(&pr()).await.unwrap(); client.checks(&pr()).await.unwrap(); @@ -962,3 +967,156 @@ async fn refuses_to_delete_the_branch_of_an_open_pull_request_or_a_default_branc .contains(&"DeletePullRequestBranch".to_string()) ); } + +// --- Workflow approvals ---------------------------------------------------- + +/// The runs of the fixture's fork head awaiting approval, page 1. +fn awaiting_runs_path() -> String { + format!( + "repos/stippi/code-assistant/actions/runs?head_sha={HEAD}&branch=feature%2Fpr-view&event=pull_request&status=action_required&per_page=100&page=1" + ) +} + +fn runs(ids: &[u64]) -> Value { + json!({"workflow_runs": ids.iter().map(|id| json!({ + "id": id, + "name": format!("CI {id}"), + "html_url": format!("https://github.com/stippi/code-assistant/actions/runs/{id}"), + })).collect::>()}) +} + +#[tokio::test] +async fn shows_the_runs_of_a_fork_awaiting_approval_as_checks() { + let fake = FakeTransport::new(); + fake.on_graphql("PullRequestCore") + .data(fixture("pull_request_core.json")); + fake.on_rest(Method::Get, &awaiting_runs_path()) + .json(runs(&[7, 8])); + let detail = fake.client().detail(&pr()).await.unwrap(); + assert_eq!(detail.workflow_approvals, Some(2)); + let added = &detail.checks[detail.checks.len() - 2..]; + assert_eq!(added[0].name, "CI 7"); + assert_eq!(added[0].status, CheckStatus::ActionRequired); + assert_eq!( + added[1].description.as_deref(), + Some("A maintainer must approve this workflow before it can run.") + ); + assert_ne!(detail.checks_state, Some(ChecksState::Passing)); +} + +#[tokio::test] +async fn a_failed_read_of_the_runs_leaves_their_count_unknown() { + let fake = FakeTransport::new(); + fake.on_graphql("PullRequestCore") + .data(fixture("pull_request_core.json")); + fake.on_rest(Method::Get, &awaiting_runs_path()) + .respond(FakeResponse::status(500)); + let detail = fake.client().detail(&pr()).await.unwrap(); + assert_eq!(detail.workflow_approvals, None); + assert_eq!( + detail.checks.last().map(|c| c.name.as_str()), + Some("Workflow approval status") + ); +} + +fn fork_heads(numbers: &[u64]) -> Value { + json!({"repository": {"pullRequests": {"nodes": numbers.iter().map(|number| json!({ + "number": number, "title": "From a fork", "url": "https://github.com/stippi/code-assistant/pull/1", + "baseRefName": "main", "headRefName": "feature/pr-view", "headRefOid": HEAD, + "state": "OPEN", "isDraft": false, "updatedAt": "2026-10-30T10:00:00Z", + "isCrossRepository": true, + "headRepository": {"name": "code-assistant", "nameWithOwner": "contributor/code-assistant"}, + "headRepositoryOwner": {"login": "contributor"} + })).collect::>()}}}) +} + +#[tokio::test] +async fn approves_each_run_still_waiting_on_the_same_head() { + let fake = FakeTransport::new(); + fake.on_graphql("PullRequestCore") + .data(fixture("pull_request_core.json")); + fake.on_graphql("PullRequestsByHead") + .data(fork_heads(&[291])); + // Run 7 is approved elsewhere before its turn comes. + fake.on_rest(Method::Get, &awaiting_runs_path()) + .once() + .json(runs(&[7, 8])); + fake.on_rest(Method::Get, &awaiting_runs_path()) + .json(runs(&[8])); + fake.on_rest( + Method::Post, + "repos/stippi/code-assistant/actions/runs/8/approve", + ) + .respond(FakeResponse::status(201)); + fake.client() + .run_action(&pr(), Action::ApproveWorkflows) + .await + .unwrap(); + let posts: Vec = fake + .requests() + .into_iter() + .filter(|r| r.method == Method::Post && r.operation.is_none()) + .map(|r| r.path) + .collect(); + assert_eq!( + posts, + ["repos/stippi/code-assistant/actions/runs/8/approve"] + ); +} + +#[tokio::test] +async fn refuses_to_approve_a_head_other_pull_requests_share() { + let fake = FakeTransport::new(); + fake.on_graphql("PullRequestCore") + .data(fixture("pull_request_core.json")); + fake.on_graphql("PullRequestsByHead") + .data(fork_heads(&[291, 300])); + fake.on_rest(Method::Get, &awaiting_runs_path()) + .json(runs(&[7])); + let refused = fake + .client() + .run_action(&pr(), Action::ApproveWorkflows) + .await + .unwrap_err(); + assert_eq!( + refused.to_string(), + "The head revision matched 2 pull requests instead of uniquely matching #291." + ); + assert!( + fake.requests() + .iter() + .all(|r| r.method != Method::Post || r.operation.is_some()) + ); +} + +#[tokio::test] +async fn polling_a_forks_checks_revalidates_its_workflow_runs_too() { + let fake = FakeTransport::new(); + let root = "repos/stippi/code-assistant"; + fake.on_rest(Method::Get, &format!("{root}/pulls/291")) + .json(json!({ + "head": {"sha": HEAD, "repo": {"id": 2}}, + "base": {"repo": {"id": 1}} + })); + fake.on_rest( + Method::Get, + &format!("{root}/commits/{HEAD}/check-runs?filter=all&per_page=100&page=1"), + ) + .json(json!({"check_runs": []})); + fake.on_rest( + Method::Get, + &format!("{root}/commits/{HEAD}/status?per_page=100&page=1"), + ) + .json(json!({"state": "success"})); + let workflows = + format!("{root}/actions/runs?head_sha={HEAD}&event=pull_request&per_page=100&page=1"); + fake.on_rest(Method::Get, &workflows) + .json(json!({"workflow_runs": []})); + fake.on_graphql("PullRequestCore") + .data(fixture("pull_request_core.json")); + fake.on_rest(Method::Get, &awaiting_runs_path()) + .json(runs(&[])); + let checks = fake.client().checks(&pr()).await.unwrap(); + assert_eq!(checks.workflow_approvals, Some(0)); + assert!(fake.requests().iter().any(|r| r.path == workflows)); +} diff --git a/crates/ui_gpui/assets/icons/columns_2.svg b/crates/ui_gpui/assets/icons/columns_2.svg new file mode 100644 index 00000000..79d8f66f --- /dev/null +++ b/crates/ui_gpui/assets/icons/columns_2.svg @@ -0,0 +1,14 @@ + + + + diff --git a/crates/ui_gpui/assets/icons/folder_tree.svg b/crates/ui_gpui/assets/icons/folder_tree.svg new file mode 100644 index 00000000..9c0aedfd --- /dev/null +++ b/crates/ui_gpui/assets/icons/folder_tree.svg @@ -0,0 +1,16 @@ + + + + + + diff --git a/crates/ui_gpui/assets/icons/pilcrow.svg b/crates/ui_gpui/assets/icons/pilcrow.svg new file mode 100644 index 00000000..b290942b --- /dev/null +++ b/crates/ui_gpui/assets/icons/pilcrow.svg @@ -0,0 +1,15 @@ + + + + + diff --git a/crates/ui_gpui/assets/icons/play.svg b/crates/ui_gpui/assets/icons/play.svg new file mode 100644 index 00000000..5bd511c1 --- /dev/null +++ b/crates/ui_gpui/assets/icons/play.svg @@ -0,0 +1,13 @@ + + + diff --git a/crates/ui_gpui/assets/icons/refresh_cw.svg b/crates/ui_gpui/assets/icons/refresh_cw.svg new file mode 100644 index 00000000..c27b3a96 --- /dev/null +++ b/crates/ui_gpui/assets/icons/refresh_cw.svg @@ -0,0 +1,16 @@ + + + + + + diff --git a/crates/ui_gpui/assets/icons/rows_3.svg b/crates/ui_gpui/assets/icons/rows_3.svg new file mode 100644 index 00000000..638a8b4f --- /dev/null +++ b/crates/ui_gpui/assets/icons/rows_3.svg @@ -0,0 +1,15 @@ + + + + + diff --git a/crates/ui_gpui/assets/icons/text_wrap.svg b/crates/ui_gpui/assets/icons/text_wrap.svg new file mode 100644 index 00000000..64f3208c --- /dev/null +++ b/crates/ui_gpui/assets/icons/text_wrap.svg @@ -0,0 +1,16 @@ + + + + + + diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs index 6d362ae3..b7d04a2a 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs @@ -11,8 +11,8 @@ use super::PullRequestIntent; use super::header::DetailHeader; use super::presentation::{Tone, merge_method_label}; use github::model::{ - Action, ChecksState, Detail, MergeMethod, Mergeability, PullRequestAction, PullRequestState, - PullRequestUpdate, UpdateMethod, + Action, Checks, ChecksState, Detail, MergeMethod, Mergeability, PullRequestAction, + PullRequestState, PullRequestUpdate, UpdateMethod, }; use gpui_kit::component::button::{Button, ButtonVariants}; use gpui_kit::component::menu::{DropdownMenu, PopupMenu, PopupMenuItem}; @@ -40,6 +40,19 @@ pub fn can(detail: &Detail, action: PullRequestAction) -> bool { detail.viewer_permissions.actions.contains(&action) } +/// How many workflow runs of an open pull request from a fork wait for +/// the viewer to approve them (0 when there is nothing they may approve). +/// `checks` is the newer read of the runs, when the checks poll made one. +pub fn workflow_approvals(detail: &Detail, checks: Option<&Checks>) -> u64 { + if detail.state != PullRequestState::Open || !can(detail, PullRequestAction::ApproveWorkflows) { + return 0; + } + checks + .and_then(|checks| checks.workflow_approvals) + .or(detail.workflow_approvals) + .unwrap_or(0) +} + /// Merge methods the repository allows, in GitHub's order. pub fn allowed_merge_methods(detail: &Detail) -> Vec { let allows = detail.merge_capabilities; @@ -164,6 +177,7 @@ pub enum Confirmable { Merge, EnableAutoMerge, Close, + ApproveWorkflows, } /// Which control waits for its confirming click. @@ -569,6 +583,35 @@ mod tests { ) } + #[test] + fn only_an_open_forks_runs_the_viewer_may_approve_are_offered() { + let waiting = |d: &mut Detail| { + d.workflow_approvals = Some(2); + d.viewer_permissions + .actions + .push(PullRequestAction::ApproveWorkflows); + }; + assert_eq!(workflow_approvals(&detail_with(1, waiting), None), 2); + let polled = Checks { + state: PullRequestState::Open, + head_sha: None, + checks: Vec::new(), + checks_state: None, + workflow_approvals: Some(0), + }; + assert_eq!( + workflow_approvals(&detail_with(1, waiting), Some(&polled)), + 0 + ); + let merged = detail_with(1, |d| { + waiting(d); + d.state = PullRequestState::Merged; + }); + assert_eq!(workflow_approvals(&merged, None), 0); + let reader = detail_with(1, |d| d.workflow_approvals = Some(2)); + assert_eq!(workflow_approvals(&reader, None), 0); + } + #[test] fn settled_pull_requests_show_their_state() { let merged = detail_with(1, |d| d.state = PullRequestState::Merged); diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs index 9cdcadda..5dd2e648 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs @@ -342,6 +342,7 @@ mod tests { head_sha: None, checks: vec![check("ci", CheckStatus::Success)], checks_state: None, + workflow_approvals: None, })); }); data.read_with(cx, |data, _| { diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs index 28aa3d36..da3eafbe 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs @@ -5,14 +5,17 @@ //! pull request (actions, updates, refresh). use super::PullRequestIntent; +use super::actions::{Confirmable, InlineConfirm, workflow_approvals}; use super::code::CodeTab; use super::composer::Composer; use super::data::PullRequestData; +use super::header::CONFIRM_TIMEOUT; use super::header::DetailHeader; use super::preferences; use super::summary::SummaryTab; -use super::tab_bar::{checks_slot, render_tab_row, timeline_slot}; +use super::tab_bar::{approve_workflows_slot, checks_slot, render_tab_row, timeline_slot}; use super::timeline::{OpenCommit, TimelineTab}; +use anyhow::Context as _; use github::PullRequestRef; use github::model::{Action, ListEntry, PullRequestUpdate}; use gpui_kit::component::{ActiveTheme, Icon, Sizable, button::Button, button::ButtonVariants}; @@ -48,6 +51,12 @@ pub struct DetailView { timeline: Entity, code: Entity, composer: Entity, + /// The tab row's "Approve workflows to run" waits for its second click. + confirm: InlineConfirm, + _confirm_timeout: Option>, + approving: bool, + /// The checks popover lists the completed checks too. + checks_show_all: bool, _subscriptions: Vec, } @@ -107,6 +116,10 @@ impl DetailView { timeline, code, composer, + confirm: InlineConfirm::default(), + _confirm_timeout: None, + approving: false, + checks_show_all: false, _subscriptions: subscriptions, } } @@ -195,11 +208,69 @@ impl DetailView { PullRequestData::refresh(&self.data, cx); } + /// The first click on "Approve workflows to run" arms it for a while, + /// the second approves. + pub fn click_approve_workflows(&mut self, cx: &mut Context) { + if self.approving { + return; + } + if !self.confirm.click(Confirmable::ApproveWorkflows) { + self._confirm_timeout = Some(cx.spawn(async move |this, cx| { + cx.background_executor().timer(CONFIRM_TIMEOUT).await; + this.update(cx, |this, cx| this.disarm(cx)).ok(); + })); + cx.notify(); + return; + } + self._confirm_timeout = None; + self.approving = true; + let write = PullRequestData::write(&self.data, cx, |service, pr| async move { + service + .pull_request_action(pr, Action::ApproveWorkflows) + .await + .context("Could not approve workflows") + }); + cx.spawn(async move |this, cx| { + write.await.ok(); + this.update(cx, |this, cx| { + this.approving = false; + cx.notify(); + }) + .ok(); + }) + .detach(); + cx.notify(); + } + + pub fn checks_show_all(&self) -> bool { + self.checks_show_all + } + + pub fn toggle_checks_show_all(&mut self, cx: &mut Context) { + self.checks_show_all = !self.checks_show_all; + cx.notify(); + } + + pub fn disarm(&mut self, cx: &mut Context) { + self._confirm_timeout = None; + if self.confirm.disarm() { + cx.notify(); + } + } + /// What the tab row shows on its right for the shown tab. fn tab_slot(&self, cx: &mut Context) -> Option { match self.tab { DetailTab::Summary => { let data = self.data.read(cx); + let approvals = data.detail.as_deref().map_or(0, |detail| { + workflow_approvals(detail, data.checks.as_deref()) + }); + if approvals > 0 || self.approving { + let armed = self.confirm.is_armed(Confirmable::ApproveWorkflows); + let view = cx.entity().downgrade(); + return Some(approve_workflows_slot(armed, self.approving, view, cx)); + } let checks = data.current_checks().to_vec(); let state = data.checks_state(); checks_slot(&checks, state, cx.entity().downgrade(), cx) @@ -415,6 +486,32 @@ mod tests { header.read_with(cx, |header, _| assert!(!header.pending())); } + #[gpui_kit::test] + fn runs_of_a_fork_are_approved_with_a_second_click(cx: &mut TestAppContext) { + let (view, cx) = page(cx); + assert!(cx.debug_bounds("pr-approve-workflows").is_none()); + let data = view.read_with(cx, |view, _| view.data().clone()); + data.update(cx, |data, cx| { + let mut waiting = detail(290); + waiting.workflow_approvals = Some(2); + waiting + .viewer_permissions + .actions + .push(github::model::PullRequestAction::ApproveWorkflows); + data.detail = Some(Arc::new(waiting)); + cx.notify(); + }); + cx.run_until_parked(); + click(cx, "pr-approve-workflows"); + view.read_with(cx, |view, _| { + assert!(view.confirm.is_armed(Confirmable::ApproveWorkflows)) + }); + click(cx, "pr-approve-workflows"); + // No session service: refused, shown, and the button is free again. + assert!(cx.debug_bounds("pr-error").is_some()); + view.read_with(cx, |view, _| assert!(!view.approving)); + } + #[gpui_kit::test] fn a_refused_title_is_taken_back(cx: &mut TestAppContext) { let (view, cx) = page(cx); diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/fixtures.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/fixtures.rs index 05c2a0d0..0658b4f7 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/fixtures.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/fixtures.rs @@ -137,6 +137,7 @@ pub fn detail(number: u64) -> Detail { auto_merge_enabled: false, auto_merge_method: None, maintainer_can_modify: false, + workflow_approvals: None, } } 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 aa8345ea..bf96b1c7 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 @@ -37,7 +37,7 @@ use std::sync::Arc; use std::time::Duration; /// How long an armed control waits for its confirming click. -const CONFIRM_TIMEOUT: Duration = Duration::from_secs(3); +pub const CONFIRM_TIMEOUT: Duration = Duration::from_secs(3); struct TitleEditor { input: Entity, diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/presentation.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/presentation.rs index 50393bab..d3336c3a 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/presentation.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/presentation.rs @@ -255,6 +255,36 @@ fn plural(count: usize, one: &str, many: &str) -> String { } /// "5 of 6 passing", "1 of 6 failing", "2 of 6 running" and the like. +/// Checks as T3 Code's popover orders them. +pub struct CheckGroups<'a> { + /// Failed, cancelled or waiting for someone. + pub attention: Vec<&'a Check>, + pub running: Vec<&'a Check>, + pub completed: Vec<&'a Check>, +} + +pub fn group_checks(checks: &[Check]) -> CheckGroups<'_> { + let of = |statuses: &[CheckStatus]| { + checks + .iter() + .filter(|check| statuses.contains(&check.status)) + .collect::>() + }; + CheckGroups { + attention: of(&[ + CheckStatus::Failure, + CheckStatus::Cancelled, + CheckStatus::ActionRequired, + ]), + running: of(&[CheckStatus::Pending]), + completed: of(&[ + CheckStatus::Success, + CheckStatus::Skipped, + CheckStatus::Neutral, + ]), + } +} + pub fn summarize_checks(checks: &[Check]) -> String { let total = checks.len(); if total == 0 { @@ -366,6 +396,22 @@ mod tests { use crate::main_screen::right_panel::pull_request::fixtures::{check, hours_ago, now}; use chrono::Duration; + #[test] + fn checks_needing_attention_come_first_then_running_then_completed() { + let checks = [ + check("lint", CheckStatus::Success), + check("build", CheckStatus::Pending), + check("test", CheckStatus::Failure), + check("ci", CheckStatus::ActionRequired), + check("docs", CheckStatus::Skipped), + ]; + let names = |group: &[&Check]| group.iter().map(|c| c.name.clone()).collect::>(); + let groups = group_checks(&checks); + assert_eq!(names(&groups.attention), ["test", "ci"]); + assert_eq!(names(&groups.running), ["build"]); + assert_eq!(names(&groups.completed), ["lint", "docs"]); + } + #[gpui_kit::test] fn only_a_running_check_spins(cx: &mut gpui_kit::TestAppContext) { cx.update(|cx| { diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/summary.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/summary.rs index 4698edb1..f33dd753 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/summary.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/summary.rs @@ -31,7 +31,7 @@ use super::conversation::{ use super::data::PullRequestData; use super::detail::TabScroll; use super::preferences; -use super::presentation::{GLYPH_SIZE, check_status_glyph}; +use super::presentation::{GLYPH_SIZE, check_status_glyph, group_checks}; use candidates::{OpenPicker, PickerKind}; use chrono::{DateTime, Utc}; use github::model::{Activity, CheckStatus, Detail, PullRequestUpdate, Reaction, ReactionContent}; @@ -654,7 +654,15 @@ impl SummaryTab { fn render_checks(&self, cx: &mut Context) -> AnyElement { let header = self.checks_header(false, cx); - let checks = self.data.read(cx).current_checks().to_vec(); + // In the popover's order: needing attention, running, completed. + let checks: Vec<_> = { + let groups = group_checks(self.data.read(cx).current_checks()); + (groups.attention.into_iter()) + .chain(groups.running) + .chain(groups.completed) + .cloned() + .collect() + }; let pr = self.data.read(cx).pr.clone(); let theme = cx.theme(); let muted = theme.muted_foreground; @@ -1238,8 +1246,9 @@ mod tests { let (_, cx) = window(cx, move |cx| SummaryTab::new(data, cx)); let inbox = agent_inbox(cx); click(cx, "pr-section-Checks"); - assert!(cx.debug_bounds("pr-check-agent-0").is_none(), "it passed"); - click(cx, "pr-check-agent-1"); + // The failed check is listed first, the passed one has no button. + assert!(cx.debug_bounds("pr-check-agent-1").is_none(), "it passed"); + click(cx, "pr-check-agent-0"); assert_eq!( inbox.borrow().as_slice(), ["Check \"test\" on stippi/code-assistant#290: Failed"] diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_bar.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_bar.rs index e135c152..2a5ee585 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_bar.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/tab_bar.rs @@ -5,7 +5,9 @@ use super::PullRequestIntent; use super::detail::{DetailTab, DetailView}; -use super::presentation::{GLYPH_SIZE, check_status_glyph, checks_glyph, summarize_checks}; +use super::presentation::{ + GLYPH_SIZE, Glyph, check_status_glyph, checks_glyph, group_checks, summarize_checks, +}; use super::timeline::TimelineTab; use crate::shared::segmented_switch::segmented_switch; use github::model::{Activity, Check, ChecksState}; @@ -46,7 +48,9 @@ pub fn render_tab_row( .children(right) } -/// "✓ 5 of 6 passing"; a click lists the checks. +/// "✓ 5 of 6 passing"; a click lists the checks: those needing attention +/// first, then the running ones, the completed ones behind "Show all" +/// while there are others (T3 Code's `PullRequestChecksPopover`). pub fn checks_slot( checks: &[Check], state: Option, @@ -66,62 +70,171 @@ pub fn checks_slot( Button::new("pr-checks-trigger") .ghost() .xsmall() - .when_some(glyph, |button, glyph| { + .when_some(glyph.clone(), |button, glyph| { button.icon(Icon::default().path(glyph.icon).text_color(glyph.color)) }) - .label(summary), + .label(summary.clone()), ) - .content(move |_, _, cx| checks_list(&checks, &view, cx)) + .content(move |_, _, cx| checks_list(&checks, glyph.as_ref(), &summary, &view, cx)) .into_any_element(), ) } -fn checks_list(checks: &[Check], view: &WeakEntity, cx: &App) -> AnyElement { - div() - .w(px(320.)) - .max_h(px(360.)) - .flex() - .flex_col() - .gap_0p5() - .children(checks.iter().enumerate().map(|(i, check)| { - let glyph = check_status_glyph(check, cx); - let url = check.url.clone(); - let view = view.clone(); - div() - .id(("pr-check", i)) - .px_1() - .py_0p5() - .rounded_sm() - .flex() - .items_center() - .gap_2() - .text_xs() - .child(glyph.render(GLYPH_SIZE)) - .child( - div() - .flex_1() - .min_w_0() - .truncate() - .text_color(cx.theme().foreground) - .child(check.name.clone()), - ) - .child( +fn checks_list( + checks: &[Check], + glyph: Option<&Glyph>, + summary: &str, + view: &WeakEntity, + cx: &App, +) -> AnyElement { + let theme = cx.theme(); + let show_all = view + .upgrade() + .is_some_and(|view| view.read(cx).checks_show_all()); + let groups = group_checks(checks); + let collapsible = !groups.attention.is_empty() || !groups.running.is_empty(); + let collapsible = collapsible && !groups.completed.is_empty(); + let shown = groups.attention.into_iter().chain(groups.running).chain( + groups + .completed + .into_iter() + .filter(|_| show_all || !collapsible), + ); + let rows = shown.enumerate().map(|(i, check)| { + let glyph = check_status_glyph(check, cx); + let url = check.url.clone(); + let tooltip: SharedString = check + .description + .clone() + .unwrap_or_else(|| check.name.clone()) + .into(); + let view = view.clone(); + div() + .id(("pr-check", i)) + .py_0p5() + .flex() + .items_center() + .gap_2() + .text_xs() + .child(glyph.render(GLYPH_SIZE)) + .child( + div() + .id(("pr-check-name", i)) + .flex_1() + .min_w_0() + .truncate() + .text_color(theme.foreground) + .child(check.name.clone()) + .tooltip(move |window, cx| { + gpui_kit::component::tooltip::Tooltip::new(tooltip.clone()) + .build(window, cx) + }), + ) + .child( + div() + .flex_none() + .text_color(theme.muted_foreground) + .child(glyph.label.clone()), + ) + .when_some(url, |row, url| { + row.child( div() + .id(("pr-check-details", i)) .flex_none() - .text_color(cx.theme().muted_foreground) - .child(glyph.label.clone()), - ) - .when_some(url, |row, url| { - row.cursor_pointer() - .hover(|s| s.bg(cx.theme().muted)) + .text_color(theme.link) + .cursor_pointer() + .hover(|s| s.underline()) + .child("Details") .on_click(move |_, _, cx| { view.update(cx, |_, cx| { cx.emit(PullRequestIntent::OpenUrl(url.clone())) }) .ok(); - }) - }) + }), + ) + }) + }); + let toggle = view.clone(); + div() + .w(px(360.)) + .flex() + .flex_col() + .gap_1() + .when_some(glyph, |d, glyph| { + d.child( + div() + .text_sm() + .font_weight(gpui_kit::FontWeight::MEDIUM) + .child(glyph.label.clone()), + ) + }) + .child( + div() + .mb_1() + .text_xs() + .text_color(theme.muted_foreground) + .child(summary.to_owned()), + ) + .child( + div() + .id("pr-checks-list") + .max_h(px(256.)) + .overflow_y_scroll() + .flex() + .flex_col() + .children(rows), + ) + .when(collapsible, |d| { + d.child( + Button::new("pr-checks-show-all") + .ghost() + .xsmall() + .label(if show_all { "Show less" } else { "Show all" }) + .on_click(move |_, _, cx| { + toggle + .update(cx, |view, cx| view.toggle_checks_show_all(cx)) + .ok(); + }), + ) + }) + .into_any_element() +} + +/// "▷ Approve workflows to run" in place of the checks while runs of a +/// fork wait for the viewer: the first click arms it ("Confirm +/// approve?"), the second approves. +pub fn approve_workflows_slot( + armed: bool, + approving: bool, + view: WeakEntity, + cx: &mut Context, +) -> AnyElement { + let label = match (approving, armed) { + (true, _) => "Approving...", + (false, true) => "Confirm approve?", + (false, false) => "Approve workflows to run", + }; + let button = Button::new("pr-approve-workflows") + .warning() + .outline() + .xsmall() + .loading(approving) + .icon(Icon::default().path(if armed { + "icons/check.svg" + } else { + "icons/play.svg" })) + .label(label) + .on_click(move |_, _, cx| { + view.update(cx, |view, cx| view.click_approve_workflows(cx)) + .ok(); + }) + .when(armed, |button| { + button.on_mouse_down_out(cx.listener(|view, _, _, cx| view.disarm(cx))) + }); + div() + .debug_selector(|| "pr-approve-workflows".into()) + .child(button) .into_any_element() } From 2e72adf4b9510a56f20006cce9e8b8747f8d9ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Fri, 30 Oct 2026 13:35:34 +0100 Subject: [PATCH 2/9] Diff rows side by side and unwrapped DiffRows paints a row as one cell (stacked) or two (split: the old file's line left, the new file's right, an empty side filled), and either wraps the lines or keeps each on one row, scrolled sideways by an HScroll all rows of a file share while the line numbers stay. The selection and comment marks name flat lines instead of rows of a chunk, since a split chunk's lines are not contiguous. diff::split pairs a hunk's removed and added runs and cuts the rows into chunks that end where something goes below a line. --- crates/ui_gpui/src/diff/mod.rs | 2 + crates/ui_gpui/src/diff/rows.rs | 585 +++++++++++++----- crates/ui_gpui/src/diff/split.rs | 216 +++++++ .../main_screen/right_panel/line_selection.rs | 32 +- .../right_panel/pull_request/code/body.rs | 2 +- .../right_panel/pull_request/code/comments.rs | 4 +- .../main_screen/right_panel/review_view.rs | 3 +- .../right_panel/review_view/comments.rs | 4 +- 8 files changed, 653 insertions(+), 195 deletions(-) create mode 100644 crates/ui_gpui/src/diff/split.rs diff --git a/crates/ui_gpui/src/diff/mod.rs b/crates/ui_gpui/src/diff/mod.rs index 7c2ea802..52b2dadb 100644 --- a/crates/ui_gpui/src/diff/mod.rs +++ b/crates/ui_gpui/src/diff/mod.rs @@ -6,6 +6,7 @@ //! - [`syntax`]: tree-sitter highlighting of both sides //! - [`prepare`]: [`prepare::PreparedFileDiff`], all of the above for one //! file, built off the UI thread +//! - [`split`]: hunks side by side, cut into chunks by rows //! - [`render`] and [`rows`]: rows as one painted element, with a gutter of //! new or old and new line numbers @@ -14,4 +15,5 @@ pub mod model; pub mod prepare; pub mod render; pub mod rows; +pub mod split; pub mod syntax; diff --git a/crates/ui_gpui/src/diff/rows.rs b/crates/ui_gpui/src/diff/rows.rs index b82771af..d43d69c3 100644 --- a/crates/ui_gpui/src/diff/rows.rs +++ b/crates/ui_gpui/src/diff/rows.rs @@ -1,16 +1,22 @@ //! The rows of a diff as one element: backgrounds, gutter numbers and -//! wrapped text painted directly, instead of three or four layout nodes per -//! row. With a diff card in view, laying those nodes out was the largest -//! part of a frame (see docs/frame-profiling.md); here taffy sees one node -//! per chunk and the text system's line cache does the rest. +//! text painted directly, instead of three or four layout nodes per row. +//! With a diff card in view, laying those nodes out was the largest part of +//! a frame (see docs/frame-profiling.md); here taffy sees one node per +//! chunk and the text system's line cache does the rest. +//! +//! A row has one cell (stacked) or two side by side (split: the old file's +//! line left, the new file's right, either may be empty). Text wraps, or +//! stays on one line and scrolls sideways with a [`HScroll`] that all rows +//! of a file share; the line numbers stay in place. use gpui_kit::{ - App, AvailableSpace, Bounds, DispatchPhase, Element, GlobalElementId, HighlightStyle, Hsla, - InspectorElementId, IntoElement, LayoutId, Length, MouseButton, MouseDownEvent, MouseMoveEvent, - MouseUpEvent, Pixels, Point, ShapedLine, SharedString, Size, Style, TextAlign, TextRun, - TextStyle, Window, WrappedLine, fill, point, px, relative, size, + App, AvailableSpace, Bounds, ContentMask, DispatchPhase, Element, GlobalElementId, + HighlightStyle, Hsla, InspectorElementId, IntoElement, LayoutId, Length, MouseButton, + MouseDownEvent, MouseMoveEvent, MouseUpEvent, Pixels, Point, ScrollWheelEvent, ShapedLine, + SharedString, Size, Style, TextAlign, TextRun, TextStyle, Window, WrappedLine, fill, point, px, + relative, size, }; -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::ops::Range; use std::rc::Rc; @@ -27,7 +33,17 @@ pub(crate) struct DiffRow { pub gutter: Option<(SharedString, Hsla)>, } -/// Horizontal layout of the rows, in pixels. +/// One side of a row and the line it shows, counted from +/// [`RowSelection::base_line`]. +pub(crate) struct RowCell { + pub row: DiffRow, + pub line: usize, +} + +/// A row's cells: the left one only when stacked. +pub(crate) type RowCells = [Option; 2]; + +/// Horizontal layout of the rows (of each column), in pixels. #[derive(Clone, Copy, Debug, PartialEq)] pub(crate) struct RowGeometry { /// Width of the gutter column; zero without one. @@ -45,14 +61,43 @@ impl RowGeometry { } } +/// How far unwrapped rows are scrolled sideways, shared by all elements +/// showing one file, and the widest text they have shown. +#[derive(Clone, Default, Debug)] +pub(crate) struct HScroll { + offset: Rc>, + widest: Rc>, +} + +impl HScroll { + pub fn offset(&self) -> Pixels { + self.offset.get() + } + + fn note_width(&self, width: Pixels) { + if width > self.widest.get() { + self.widest.set(width); + } + } + + /// Scroll by `delta` (positive: the text moves right) where `visible` + /// of the widest text shows; whether the offset changed. + fn scroll_by(&self, delta: Pixels, visible: Pixels) -> bool { + let max = (self.widest.get() - visible).max(px(0.)); + let offset = (self.offset.get() - delta).clamp(px(0.), max); + let changed = offset != self.offset.get(); + self.offset.set(offset); + changed + } +} + /// Makes a [`DiffRows`] participate in whole-line text selection. The element -/// maps a pointer position to a flat line index (`base_line` plus the row -/// under the pointer) and reports it through the callbacks; the owner keeps -/// the actual selection and hands back the local rows (`highlight`) to paint -/// as selected. Selection is line-granular, which suits grabbing code to paste -/// elsewhere and keeps painting trivial over a wrapped, virtualized list. -/// Reports a flat line index (`base_line` + row) when selection starts or -/// extends over a row. +/// maps a pointer position to a flat line index (`base_line` plus the line +/// of the cell under the pointer) and reports it through the callbacks; the +/// owner keeps the actual selection and hands back the flat lines +/// (`highlight`) to paint as selected. Selection is line-granular, which +/// suits grabbing code to paste elsewhere and keeps painting trivial over a +/// wrapped, virtualized list. pub(crate) type LineCallback = Rc; /// Reports that a selection drag has ended. @@ -60,19 +105,20 @@ pub(crate) type EndCallback = Rc; #[derive(Clone)] pub(crate) struct RowSelection { - /// Flat line index of this element's first row (row 0). + /// What a cell's `line` counts from. pub base_line: usize, - /// Local rows (indices into `rows`) to paint as selected. + /// Flat lines to paint as selected. pub highlight: Range, /// Highlight color (usually the theme's selection background). pub color: Hsla, - /// Local rows carrying a comment, marked by a bar at the left edge. + /// Flat lines carrying a comment, marked by a bar at the left edge. pub marked: Vec>, pub mark_color: Hsla, - /// Receives the window bounds of the last highlighted row when painted, - /// so the owner can float things next to the selection's end. - pub anchor: Option>>>>, - /// Pointer pressed on a row: `base_line + row`. + /// Receives the window bounds of the row showing the last highlighted + /// line when painted, so the owner can float things next to the + /// selection's end. + pub anchor: Option>>>>, + /// Pointer pressed on a row: the flat line under it. pub on_start: LineCallback, /// Pointer dragged over a row while the button is held. pub on_drag: LineCallback, @@ -81,25 +127,58 @@ pub(crate) struct RowSelection { } pub(crate) struct DiffRows { - rows: Rc>, + rows: Rc>, + /// 1 (stacked) or 2 (split). + columns: usize, geometry: RowGeometry, + /// Fills the side of a split row that shows no line. + empty: Option, + /// `None`: lines wrap; else they scroll sideways together. + scroll: Option, /// Filled by the measure closure, read by prepaint and paint. cell: LayoutCell, - /// Present when the rows are selectable (the Review panel); drives - /// highlight painting and pointer handling in [`DiffRows::paint`]. + /// Present when the rows are selectable; drives highlight painting and + /// pointer handling in [`DiffRows::paint`]. selection: Option, } impl DiffRows { + /// Stacked rows; row `i` shows line `i`. pub fn new(rows: Vec, geometry: RowGeometry) -> Self { + let rows = rows + .into_iter() + .enumerate() + .map(|(line, row)| [Some(RowCell { row, line }), None]) + .collect(); + Self::with_cells(rows, 1, geometry) + } + + /// Rows of two cells side by side; `empty` fills a missing one. + pub fn split(rows: Vec, geometry: RowGeometry, empty: Hsla) -> Self { + Self { + empty: Some(empty), + ..Self::with_cells(rows, 2, geometry) + } + } + + fn with_cells(rows: Vec, columns: usize, geometry: RowGeometry) -> Self { Self { rows: Rc::new(rows), + columns, geometry, + empty: None, + scroll: None, cell: Rc::default(), selection: None, } } + /// Keep each line on one row; they scroll sideways with `scroll`. + pub(crate) fn unwrapped(mut self, scroll: HScroll) -> Self { + self.scroll = Some(scroll); + self + } + /// Enable whole-line selection on these rows. pub(crate) fn selectable(mut self, selection: RowSelection) -> Self { self.selection = Some(selection); @@ -112,19 +191,24 @@ impl DiffRows { } } -/// Map an absolute `y` to a flat line index: `base` plus the row whose band -/// contains `y`, clamped to `[0, row_count)`. `tops` holds the absolute top of -/// each row plus a final bottom (so it has `row_count + 1` entries). -fn local_line(tops: &[Pixels], base: usize, row_count: usize, y: Pixels) -> usize { +/// The row whose band contains the absolute `y`, clamped to +/// `[0, row_count)`. `tops` holds the absolute top of each row plus a final +/// bottom (so it has `row_count + 1` entries). +fn row_at(tops: &[Pixels], row_count: usize, y: Pixels) -> usize { if row_count == 0 { - return base; + return 0; } - let row = tops[..row_count] + tops[..row_count] .iter() .rposition(|&top| y >= top) .unwrap_or(0) - .min(row_count - 1); - base + row + .min(row_count - 1) +} + +/// The line of the cell in `column`, else of the row's other cell. +fn line_of(cells: &RowCells, column: usize) -> Option { + let pick = |c: usize| cells.get(c).and_then(|cell| cell.as_ref()).map(|c| c.line); + pick(column).or_else(|| pick(1 - column.min(1))) } /// Shaped rows for one width. @@ -134,22 +218,30 @@ pub(crate) struct RowsLayout { pub rows: Vec, pub width: Pixels, pub height: Pixels, + /// The widest cell's text plus its padding. + pub widest: Pixels, } -pub(crate) struct RowLayout { +pub(crate) struct CellLayout { pub lines: Vec, pub gutter: Option, +} + +pub(crate) struct RowLayout { + pub cells: [Option; 2], pub height: Pixels, } impl RowLayout { - /// Visual lines after wrapping. + /// Visual lines of the first cell after wrapping. #[cfg(test)] pub fn line_count(&self) -> usize { - self.lines - .iter() - .map(|line| line.wrap_boundaries.len() + 1) - .sum() + self.cells[0].as_ref().map_or(0, |cell| { + cell.lines + .iter() + .map(|line| line.wrap_boundaries.len() + 1) + .sum() + }) } } @@ -202,9 +294,17 @@ impl RowTextStyle { } } -fn layout_rows( - rows: &[DiffRow], +/// What the rows are laid out by besides the width. +#[derive(Clone, Copy)] +struct Shape { + columns: usize, geometry: RowGeometry, + wrap: bool, +} + +fn layout_rows( + rows: &[RowCells], + shape: Shape, width: Option, text: &RowTextStyle, window: &mut Window, @@ -215,122 +315,158 @@ fn layout_rows( line_height, } = text; let (font_size, line_height) = (*font_size, *line_height); - let wrap_width = width.map(|width| (width - geometry.text_inset()).max(px(0.))); + let geometry = shape.geometry; + let column_width = width.map(|width| width / shape.columns as f32); + let wrap_width = column_width + .filter(|_| shape.wrap) + .map(|width| (width - geometry.text_inset()).max(px(0.))); let mut layout = RowsLayout { line_height, rows: Vec::with_capacity(rows.len()), width: width.unwrap_or_default(), height: px(0.), + widest: px(0.), }; - for row in rows { - let mut style = text_style.clone(); - style.color = row.color; - let runs = row_runs(row.text.len(), &style, &row.highlights); - let lines = window - .text_system() - .shape_text(row.text.clone(), font_size, &runs, wrap_width, None) - .map(|lines| lines.into_vec()) - .unwrap_or_default(); - let gutter = row.gutter.as_ref().map(|(text, color)| { + for cells in rows { + let mut height = line_height; + let mut laid = [None, None]; + for (column, cell) in cells.iter().enumerate().take(shape.columns) { + let Some(RowCell { row, .. }) = cell else { + continue; + }; let mut style = text_style.clone(); - style.color = *color; - window.text_system().shape_line( - text.clone(), - font_size, - &[style.to_run(text.len())], - None, - ) - }); - let text_width = lines - .iter() - .map(|line| line.size(line_height).width) - .fold(px(0.), Pixels::max); - if width.is_none() { - layout.width = layout.width.max(text_width + geometry.text_inset()); - } - let row_layout = RowLayout { - height: lines + style.color = row.color; + let runs = row_runs(row.text.len(), &style, &row.highlights); + let lines = window + .text_system() + .shape_text(row.text.clone(), font_size, &runs, wrap_width, None) + .map(|lines| lines.into_vec()) + .unwrap_or_default(); + let gutter = row.gutter.as_ref().map(|(text, color)| { + let mut style = text_style.clone(); + style.color = *color; + window.text_system().shape_line( + text.clone(), + font_size, + &[style.to_run(text.len())], + None, + ) + }); + let text_width = lines .iter() - .map(|line| line.size(line_height).height) - .fold(px(0.), |a, b| a + b) - .max(line_height), - lines, - gutter, - }; - layout.height += row_layout.height; - layout.rows.push(row_layout); + .map(|line| line.size(line_height).width) + .fold(px(0.), Pixels::max); + layout.widest = layout + .widest + .max(text_width + geometry.content_left + geometry.content_right); + height = height.max( + lines + .iter() + .map(|line| line.size(line_height).height) + .fold(px(0.), |a, b| a + b), + ); + laid[column] = Some(CellLayout { lines, gutter }); + } + layout.height += height; + layout.rows.push(RowLayout { + cells: laid, + height, + }); + } + if width.is_none() { + layout.width = (layout.widest + geometry.gutter_width) * shape.columns as f32; } layout } +/// Where a column's parts go, from the element's bounds. +struct ColumnFrame { + left: Pixels, + width: Pixels, +} + +#[allow(clippy::too_many_arguments)] fn paint_rows( - rows: &[DiffRow], + rows: &[RowCells], layout: &RowsLayout, - geometry: RowGeometry, + shape: Shape, + empty: Option, + scroll: Pixels, bounds: Bounds, selection: Option<&RowSelection>, window: &mut Window, cx: &mut App, ) { + let geometry = shape.geometry; let line_height = layout.line_height; + let column_width = bounds.size.width / shape.columns as f32; + let flat = |line: usize| selection.map_or(line, |sel| sel.base_line + line); let mut y = bounds.origin.y; - for (ix, (row, row_layout)) in rows.iter().zip(&layout.rows).enumerate() { - if let Some(background) = row.background { - window.paint_quad(fill( - Bounds::new( - point(bounds.origin.x, y), - size(bounds.size.width, row_layout.height), - ), - background, - )); - } - if let Some(edge) = row.edge { - window.paint_quad(fill( - Bounds::new(point(bounds.origin.x, y), size(px(2.), row_layout.height)), - edge, - )); - } - // Selection sits above the add/delete tint but below the glyphs, so - // the selected text stays readable. - if let Some(sel) = selection - && sel.highlight.contains(&ix) - { - window.paint_quad(fill( - Bounds::new( - point(bounds.origin.x, y), - size(bounds.size.width, row_layout.height), - ), - sel.color, - )); - } - if let Some(sel) = selection - && sel.marked.iter().any(|range| range.contains(&ix)) - { - window.paint_quad(fill( - Bounds::new(point(bounds.origin.x, y), size(px(3.), row_layout.height)), - sel.mark_color, - )); - } - if let Some(gutter) = &row_layout.gutter { - _ = gutter.paint( - point(bounds.origin.x + geometry.gutter_left, y), - line_height, - TextAlign::Left, - None, - window, - cx, + for (cells, row_layout) in rows.iter().zip(&layout.rows) { + let height = row_layout.height; + for column in 0..shape.columns { + let frame = ColumnFrame { + left: bounds.origin.x + column_width * column as f32, + width: column_width, + }; + let band = |width: Pixels| Bounds::new(point(frame.left, y), size(width, height)); + let (Some(cell), Some(cell_layout)) = (&cells[column], &row_layout.cells[column]) + else { + if let Some(empty) = empty { + window.paint_quad(fill(band(frame.width), empty)); + } + continue; + }; + let row = &cell.row; + let line = flat(cell.line); + if let Some(background) = row.background { + window.paint_quad(fill(band(frame.width), background)); + } + if let Some(edge) = row.edge { + window.paint_quad(fill(band(px(2.)), edge)); + } + // Selection sits above the add/delete tint but below the glyphs, + // so the selected text stays readable. + if let Some(sel) = selection { + if sel.highlight.contains(&line) { + window.paint_quad(fill(band(frame.width), sel.color)); + } + if sel.marked.iter().any(|range| range.contains(&line)) { + window.paint_quad(fill(band(px(3.)), sel.mark_color)); + } + } + if let Some(gutter) = &cell_layout.gutter { + _ = gutter.paint( + point(frame.left + geometry.gutter_left, y), + line_height, + TextAlign::Left, + None, + window, + cx, + ); + } + let code = Bounds::new( + point(frame.left + geometry.gutter_width, y), + size((frame.width - geometry.gutter_width).max(px(0.)), height), ); + let mut origin: Point = + point(code.origin.x + geometry.content_left - scroll, y); + window.with_content_mask(Some(ContentMask { bounds: code }), |window| { + for line in &cell_layout.lines { + _ = line.paint_background( + origin, + line_height, + TextAlign::Left, + None, + window, + cx, + ); + _ = line.paint(origin, line_height, TextAlign::Left, None, window, cx); + origin.y += line.size(line_height).height; + } + }); } - let mut origin: Point = point( - bounds.origin.x + geometry.gutter_width + geometry.content_left, - y, - ); - for line in &row_layout.lines { - _ = line.paint_background(origin, line_height, TextAlign::Left, None, window, cx); - _ = line.paint(origin, line_height, TextAlign::Left, None, window, cx); - origin.y += line.size(line_height).height; - } - y += row_layout.height; + y += height; } } @@ -342,10 +478,20 @@ impl IntoElement for DiffRows { } } +impl DiffRows { + fn shape(&self) -> Shape { + Shape { + columns: self.columns, + geometry: self.geometry, + wrap: self.scroll.is_none(), + } + } +} + impl Element for DiffRows { type RequestLayoutState = LayoutCell; - /// Selectable rows take presses through a hitbox, so something floating - /// above them (a selection pill) keeps its clicks. + /// Selectable or scrolling rows take the pointer through a hitbox, so + /// something floating above them (a selection pill) keeps its clicks. type PrepaintState = Option; fn id(&self) -> Option { @@ -365,7 +511,7 @@ impl Element for DiffRows { ) -> (LayoutId, LayoutCell) { let cell = self.cell.clone(); let rows = self.rows.clone(); - let geometry = self.geometry; + let shape = self.shape(); // The measure closure runs in taffy's layout pass, when the enclosing // elements' text styles are no longer on the window: read them now. let text = RowTextStyle::capture(window); @@ -383,7 +529,7 @@ impl Element for DiffRows { AvailableSpace::Definite(width) => Some(width), _ => None, }); - let layout = layout_rows(&rows, geometry, width, &text, window); + let layout = layout_rows(&rows, shape, width, &text, window); let measured = size(layout.width, layout.height); *cell.borrow_mut() = Some(layout); measured @@ -401,8 +547,7 @@ impl Element for DiffRows { window: &mut Window, _: &mut App, ) -> Option { - self.selection - .is_some() + (self.selection.is_some() || self.scroll.is_some()) .then(|| window.insert_hitbox(bounds, gpui_kit::HitboxBehavior::Normal)) } @@ -420,17 +565,45 @@ impl Element for DiffRows { let Some(layout) = borrow.as_ref() else { return; }; + let shape = self.shape(); + let offset = self.scroll.as_ref().map_or(px(0.), HScroll::offset); paint_rows( &self.rows, layout, - self.geometry, + shape, + self.empty, + offset, bounds, self.selection.as_ref(), window, cx, ); + let column_width = bounds.size.width / shape.columns as f32; + if let Some(scroll) = self.scroll.clone() { + scroll.note_width(layout.widest); + let visible = column_width - shape.geometry.gutter_width; + let line_height = layout.line_height; + let hitbox = hitbox.clone(); + window.on_mouse_event(move |e: &ScrollWheelEvent, phase, window, cx| { + let hovered = hitbox + .as_ref() + .map_or(bounds.contains(&e.position), |h| h.is_hovered(window)); + if phase != DispatchPhase::Bubble || !hovered { + return; + } + let delta = e.delta.pixel_delta(line_height); + // Mostly sideways: the rows take it, the list stays put. + if delta.x.abs() > delta.y.abs() { + if scroll.scroll_by(delta.x, visible) { + window.refresh(); + } + cx.stop_propagation(); + } + }); + } + // Pointer handling for selectable rows. The absolute top of each row // (plus a final bottom) lets the listeners below map a pointer y to a // row without re-reading the layout at event time. @@ -444,11 +617,16 @@ impl Element for DiffRows { tops.push(y); if let Some(anchor) = &sel.anchor && let Some(last) = sel.highlight.end.checked_sub(1) - && last < layout.rows.len() + && let Some(row) = self.rows.iter().position(|cells| { + cells + .iter() + .flatten() + .any(|cell| sel.base_line + cell.line == last) + }) { anchor.set(Some(Bounds::new( - point(bounds.origin.x, tops[last]), - size(bounds.size.width, tops[last + 1] - tops[last]), + point(bounds.origin.x, tops[row]), + size(bounds.size.width, tops[row + 1] - tops[row]), ))); } let tops = Rc::new(tops); @@ -460,8 +638,19 @@ impl Element for DiffRows { sel.on_end.clone(), ); drop(borrow); + // The flat line under a pointer position. + let rows = self.rows.clone(); + let columns = shape.columns; + let line_at = Rc::new(move |position: Point| { + let row = row_at(&tops, row_count, position.y); + let column = (((position.x - bounds.origin.x) / column_width).floor() as usize) + .min(columns - 1); + rows.get(row) + .and_then(|cells| line_of(cells, column)) + .map(|line| base + line) + }); - let tops_down = tops.clone(); + let line_down = line_at.clone(); let hitbox = hitbox.clone(); window.on_mouse_event(move |e: &MouseDownEvent, phase, window, cx| { if phase == DispatchPhase::Bubble @@ -469,26 +658,19 @@ impl Element for DiffRows { && hitbox .as_ref() .map_or(bounds.contains(&e.position), |h| h.is_hovered(window)) + && let Some(line) = line_down(e.position) { - on_start( - local_line(&tops_down, base, row_count, e.position.y), - window, - cx, - ); + on_start(line, window, cx); } }); - let tops_move = tops.clone(); window.on_mouse_event(move |e: &MouseMoveEvent, phase, window, cx| { if phase == DispatchPhase::Bubble && e.pressed_button == Some(MouseButton::Left) && bounds.contains(&e.position) + && let Some(line) = line_at(e.position) { - on_drag( - local_line(&tops_move, base, row_count, e.position.y), - window, - cx, - ); + on_drag(line, window, cx); } }); @@ -510,17 +692,41 @@ mod tests { }; #[test] - fn local_line_maps_y_to_a_row_and_clamps() { - // Three rows at tops 0,10,20 with bottom 30; base 100. + fn a_pointer_maps_to_a_row_and_a_cell() { + // Three rows at tops 0,10,20 with bottom 30. let tops = [px(0.), px(10.), px(20.), px(30.)]; - assert_eq!(local_line(&tops, 100, 3, px(5.)), 100); - assert_eq!(local_line(&tops, 100, 3, px(10.)), 101); - assert_eq!(local_line(&tops, 100, 3, px(25.)), 102); + assert_eq!(row_at(&tops, 3, px(5.)), 0); + assert_eq!(row_at(&tops, 3, px(10.)), 1); + assert_eq!(row_at(&tops, 3, px(25.)), 2); // Above the first row and below the last clamp to the ends. - assert_eq!(local_line(&tops, 100, 3, px(-5.)), 100); - assert_eq!(local_line(&tops, 100, 3, px(999.)), 102); - // No rows: the base line. - assert_eq!(local_line(&[px(0.)], 7, 0, px(5.)), 7); + assert_eq!(row_at(&tops, 3, px(-5.)), 0); + assert_eq!(row_at(&tops, 3, px(999.)), 2); + assert_eq!(row_at(&[px(0.)], 0, px(5.)), 0); + + let cell = |line| { + Some(RowCell { + row: rows(&["x"]).pop().unwrap(), + line, + }) + }; + assert_eq!(line_of(&[cell(4), cell(9)], 1), Some(9)); + // An empty side stands for the other one. + assert_eq!(line_of(&[cell(4), None], 1), Some(4)); + assert_eq!(line_of(&[None, cell(9)], 0), Some(9)); + assert_eq!(line_of(&[None, None], 0), None); + } + + #[test] + fn sideways_scrolling_stays_within_the_widest_text() { + let scroll = HScroll::default(); + scroll.note_width(px(300.)); + assert!(scroll.scroll_by(px(-50.), px(200.))); + assert_eq!(scroll.offset(), px(50.)); + assert!(scroll.scroll_by(px(-500.), px(200.))); + assert_eq!(scroll.offset(), px(100.)); + assert!(!scroll.scroll_by(px(-1.), px(200.))); + assert!(scroll.scroll_by(px(500.), px(200.))); + assert_eq!(scroll.offset(), px(0.)); } #[test] @@ -588,7 +794,8 @@ mod tests { let layout = cell.borrow(); let layout = layout.as_ref().expect("measured"); assert_eq!(layout.line_height, px(15.)); - assert_eq!(layout.rows[0].lines[0].unwrapped_layout.font_size, px(10.)); + let cell = layout.rows[0].cells[0].as_ref().unwrap(); + assert_eq!(cell.lines[0].unwrapped_layout.font_size, px(10.)); } #[gpui_kit::test] @@ -617,4 +824,46 @@ mod tests { assert!(lines > 1, "{lines} lines"); assert_eq!(layout.height, layout.line_height * lines as f32); } + + #[gpui_kit::test] + fn unwrapped_rows_keep_one_line_and_split_rows_take_the_taller_cell(cx: &mut TestAppContext) { + let (_, cx) = cx.add_window_view(|_, _| Root); + let cx: &mut VisualTestContext = cx; + let long = "x".repeat(200); + let (cell, _) = cx.draw(point(px(0.), px(0.)), size(px(120.), px(400.)), |_, _| { + DiffRows::new(rows(&[long.as_str()]), GEOMETRY).unwrapped(HScroll::default()) + }); + { + let layout = cell.borrow(); + let layout = layout.as_ref().expect("measured"); + assert_eq!(layout.rows[0].line_count(), 1); + assert!(layout.widest > px(120.)); + } + + let pair = |left: &str, right: &str| { + let mut texts = rows(&[left, right]); + let right = texts.pop().unwrap(); + let left = texts.pop().unwrap(); + [ + Some(RowCell { row: left, line: 0 }), + Some(RowCell { + row: right, + line: 1, + }), + ] + }; + let (cell, _) = cx.draw(point(px(0.), px(0.)), size(px(240.), px(400.)), |_, _| { + DiffRows::split(vec![pair("short", long.as_str())], GEOMETRY, white()) + }); + let layout = cell.borrow(); + let layout = layout.as_ref().expect("measured"); + let right = layout.rows[0].cells[1].as_ref().unwrap(); + let wrapped: usize = right + .lines + .iter() + .map(|l| l.wrap_boundaries.len() + 1) + .sum(); + assert!(wrapped > 1); + assert_eq!(layout.rows[0].height, layout.line_height * wrapped as f32); + } } diff --git a/crates/ui_gpui/src/diff/split.rs b/crates/ui_gpui/src/diff/split.rs new file mode 100644 index 00000000..ca4240a6 --- /dev/null +++ b/crates/ui_gpui/src/diff/split.rs @@ -0,0 +1,216 @@ +//! Hunks side by side: the old file's lines on the left, the new file's on +//! the right. Inside a hunk, a run of removed lines and the run of added +//! lines right after it are paired line by line; the longer run's rest +//! faces an empty cell. Context lines show on both sides. +//! +//! A split row stands for one or two flat lines (the line indices the +//! stacked layout counts), so the chunks of a split diff are cut by rows: +//! a chunk ends after the row holding a break, so whatever goes below a +//! line (a review thread) goes below the row showing it. + +use super::model::DiffHunk; +use similar::ChangeTag; +use std::ops::Range; + +/// One row: indices into its hunk's lines, `None` for an empty cell. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct SplitRow { + pub left: Option, + pub right: Option, +} + +/// The rows of one hunk's lines (by their tags), paired. +pub fn pair_lines(tags: impl IntoIterator) -> Vec { + let tags: Vec = tags.into_iter().collect(); + let run = |from: usize, tag: ChangeTag| { + from..tags[from..] + .iter() + .position(|&t| t != tag) + .map_or(tags.len(), |len| from + len) + }; + let mut rows = Vec::new(); + let mut ix = 0; + while ix < tags.len() { + if tags[ix] == ChangeTag::Equal { + rows.push(SplitRow { + left: Some(ix), + right: Some(ix), + }); + ix += 1; + continue; + } + let removed = run(ix, ChangeTag::Delete); + let added = run(removed.end, ChangeTag::Insert); + ix = added.end; + for k in 0..removed.len().max(added.len()) { + rows.push(SplitRow { + left: removed.clone().nth(k), + right: added.clone().nth(k), + }); + } + } + rows +} + +/// A slice of one hunk's split rows: an item of a virtualized list. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct SplitChunk { + pub hunk: usize, + pub rows: Range, +} + +/// A file's hunks side by side, cut into chunks. +#[derive(Clone, Debug, Default)] +pub struct SplitHunks { + /// Per hunk, its rows. + pub rows: Vec>, + /// Per hunk, the flat index of its first line. + pub flat_starts: Vec, + pub chunks: Vec, +} + +impl SplitHunks { + /// Pair `hunks` and cut them into chunks of at most `max_rows` rows, + /// ending one after every row showing a flat line in `breaks`. + pub fn new(hunks: &[DiffHunk], max_rows: usize, breaks: &[usize]) -> Self { + let max_rows = max_rows.max(1); + let mut split = Self::default(); + let mut flat = 0; + for (hunk_ix, hunk) in hunks.iter().enumerate() { + let rows = pair_lines(hunk.lines.iter().map(|line| line.tag)); + let mut start = 0; + for (ix, row) in rows.iter().enumerate() { + let breaks_here = [row.left, row.right] + .into_iter() + .flatten() + .any(|line| breaks.contains(&(flat + line))); + if breaks_here || ix + 1 - start == max_rows || ix + 1 == rows.len() { + split.chunks.push(SplitChunk { + hunk: hunk_ix, + rows: start..ix + 1, + }); + start = ix + 1; + } + } + split.flat_starts.push(flat); + split.rows.push(rows); + flat += hunk.lines.len(); + } + split + } + + /// The rows of `chunk`. + pub fn chunk_rows(&self, chunk: &SplitChunk) -> &[SplitRow] { + self.rows + .get(chunk.hunk) + .and_then(|rows| rows.get(chunk.rows.clone())) + .unwrap_or_default() + } + + /// The flat lines `chunk`'s last row shows: what goes below it. + pub fn last_lines(&self, chunk: &SplitChunk) -> Vec { + let flat = self.flat_starts.get(chunk.hunk).copied().unwrap_or(0); + let Some(row) = self.chunk_rows(chunk).last() else { + return Vec::new(); + }; + let mut lines: Vec = [row.left, row.right] + .into_iter() + .flatten() + .map(|line| flat + line) + .collect(); + lines.dedup(); + lines + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::diff::model::DiffLine; + use ChangeTag::*; + + fn row(left: Option, right: Option) -> SplitRow { + SplitRow { left, right } + } + + #[test] + fn removed_and_added_runs_face_each_other() { + assert_eq!( + pair_lines([Equal, Delete, Delete, Insert, Equal]), + [ + row(Some(0), Some(0)), + row(Some(1), Some(3)), + row(Some(2), None), + row(Some(4), Some(4)), + ] + ); + assert_eq!( + pair_lines([Delete, Insert, Insert, Insert]), + [ + row(Some(0), Some(1)), + row(None, Some(2)), + row(None, Some(3)), + ] + ); + } + + #[test] + fn lone_runs_face_empty_cells() { + assert_eq!( + pair_lines([Insert, Insert]), + [row(None, Some(0)), row(None, Some(1))] + ); + assert_eq!( + pair_lines([Delete, Equal, Delete]), + [ + row(Some(0), None), + row(Some(1), Some(1)), + row(Some(2), None) + ] + ); + } + + #[test] + fn removed_lines_after_added_ones_start_a_new_block() { + assert_eq!( + pair_lines([Delete, Insert, Delete, Insert, Insert]), + [ + row(Some(0), Some(1)), + row(Some(2), Some(3)), + row(None, Some(4)), + ] + ); + } + + fn hunk(tags: &[ChangeTag]) -> DiffHunk { + DiffHunk { + old_start: 1, + new_start: 1, + header: None, + lines: tags.iter().map(|&tag| DiffLine::plain(tag, "x")).collect(), + } + } + + #[test] + fn chunks_are_cut_by_rows_and_end_at_breaks() { + let hunks = [ + hunk(&[Delete, Delete, Delete, Insert, Insert, Insert]), + hunk(&[Equal, Equal, Equal]), + ]; + // A thread on the first removed line: below the row pairing it. + let split = SplitHunks::new(&hunks, 2, &[0]); + let shape: Vec<_> = split + .chunks + .iter() + .map(|c| (c.hunk, c.rows.clone())) + .collect(); + assert_eq!(shape, [(0, 0..1), (0, 1..3), (1, 0..2), (1, 2..3)]); + assert_eq!(split.flat_starts, [0, 6]); + assert_eq!(split.last_lines(&split.chunks[0]), [0, 3]); + assert_eq!(split.last_lines(&split.chunks[3]), [8]); + assert_eq!( + split.chunk_rows(&split.chunks[1]), + [row(Some(1), Some(4)), row(Some(2), Some(5))] + ); + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/line_selection.rs b/crates/ui_gpui/src/main_screen/right_panel/line_selection.rs index 10160bfb..11e6017d 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/line_selection.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/line_selection.rs @@ -101,19 +101,14 @@ impl LineSelection { self.dragging = false; } - /// The rows of a chunk (`row_count` rows starting at flat line - /// `base_line` of `key`) to paint as selected, relative to the chunk. - pub fn highlight(&self, key: &K, base_line: usize, row_count: usize) -> Range { - let Some(sel) = self.selected.as_ref().filter(|s| &s.key == key) else { - return 0..0; - }; - let (lo, hi) = sel.range(); - let start = lo.max(base_line); - let end = hi.min(base_line + row_count.saturating_sub(1)); - if row_count > 0 && start <= end { - (start - base_line)..(end - base_line + 1) - } else { - 0..0 + /// The flat lines of `key` to paint as selected. + pub fn highlight(&self, key: &K) -> Range { + match self.selected.as_ref().filter(|s| &s.key == key) { + Some(sel) => { + let (lo, hi) = sel.range(); + lo..hi + 1 + } + None => 0..0, } } } @@ -125,8 +120,8 @@ pub(crate) trait SelectsLines: 'static + Sized { fn line_selection(&mut self) -> &mut LineSelection; } -/// How a chunk paints: the selected rows (from [`LineSelection::highlight`]) -/// and the commented ones, both relative to the chunk. +/// How a chunk paints: the selected flat lines (from +/// [`LineSelection::highlight`]) and the commented ones. pub(crate) struct ChunkMarks { pub highlight: Range, pub color: Hsla, @@ -211,11 +206,8 @@ mod tests { sel.end(); assert!(!sel.extend(&"a", 2), "no drag after release"); assert_eq!(sel.get().unwrap().range(), (5, 8)); - // Chunk of rows 0..6 holds lines 5 and 6 of the selection. - assert_eq!(sel.highlight(&"a", 0, 7), 5..7); - assert_eq!(sel.highlight(&"a", 7, 10), 0..2); - assert_eq!(sel.highlight(&"a", 9, 10), 0..0); - assert_eq!(sel.highlight(&"b", 0, 10), 0..0); + assert_eq!(sel.highlight(&"a"), 5..9); + assert_eq!(sel.highlight(&"b"), 0..0); } #[test] diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs index 260fb31e..8d9c589d 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs @@ -108,7 +108,7 @@ impl CodeTab { let rows = chunk.lines.len(); let selection = self.commit.is_none().then(|| { let marks = ChunkMarks { - highlight: self.selection.highlight(&file.path, chunk.flat_start, rows), + highlight: self.selection.highlight(&file.path), color: theme.selection, marked: self.pending_marks(file, chunk.flat_start, rows, cx), mark_color: theme.warning, diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/comments.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/comments.rs index 864d90ff..9cfcbf32 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/comments.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/comments.rs @@ -234,7 +234,7 @@ impl CodeTab { self.place_annotations(cx); } - /// Rows of a chunk (relative to it) a pending comment is on. + /// Flat lines of a chunk a pending comment is on. pub(super) fn pending_marks( &self, file: &LoadedFile, @@ -259,7 +259,7 @@ impl CodeTab { .and_then(|(side, line)| file.lines.row(super::anchors::LineRef { side, line })) .unwrap_or(end); let covered = start.max(chunk.start)..(end + 1).min(chunk.end); - (covered.start < covered.end).then(|| covered.start - base..covered.end - base) + (covered.start < covered.end).then_some(covered) }) .collect() } diff --git a/crates/ui_gpui/src/main_screen/right_panel/review_view.rs b/crates/ui_gpui/src/main_screen/right_panel/review_view.rs index d707b046..780a3c80 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/review_view.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/review_view.rs @@ -1167,10 +1167,9 @@ impl ReviewView { let rem_size = window.rem_size(); let line_height_px = rems(1.25).to_pixels(rem_size).round(); - // The local rows (relative to this chunk) to paint as selected. let base_line = chunk.flat_start; let marks = ChunkMarks { - highlight: self.selection.highlight(key, base_line, chunk.lines.len()), + highlight: self.selection.highlight(key), color: theme.selection, marked: self.chunk_marks(key, prepared, base_line, chunk.lines.len()), mark_color: theme.warning, diff --git a/crates/ui_gpui/src/main_screen/right_panel/review_view/comments.rs b/crates/ui_gpui/src/main_screen/right_panel/review_view/comments.rs index 8e695241..c11c6f36 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/review_view/comments.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/review_view/comments.rs @@ -34,7 +34,7 @@ impl ReviewView { } } - /// Rows of a chunk (relative to it) covered by a comment. + /// Flat lines of a chunk covered by a comment. pub(super) fn chunk_marks( &self, key: &FileKey, @@ -54,7 +54,7 @@ impl ReviewView { .get(base_line + row) .is_some_and(|&n| comments.iter().any(|c| covers(c, n))) }) - .map(|row| row..row + 1) + .map(|row| base_line + row..base_line + row + 1) .collect() } From 6cd1c7e567c8956ca0977469cbdd4cb7e0b1c553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Fri, 30 Oct 2026 13:54:34 +0100 Subject: [PATCH 3/9] Code tab: split layout, file tree, whitespace, wrap, context The rest of T3 Code's Code tab options: - Stacked | split (remembered): split rows pair a hunk's removed and added runs side by side with old and new line numbers; new and deleted files stay one column. Threads and comments sit below the row showing their line. - Hide whitespace changes (on by default, as in T3): each hunk's sides are diffed again without whitespace, client-side like T3 (GitHub's API has no such diff); parsed once per page, off the UI thread. - Wrap (remembered): unwrapped lines scroll sideways together per file, the line numbers stay. - Expanding context: the gaps between hunks and the unknown tail get T3's buttons (100 lines up / down, or all; shift-click all). The first click reads the file's text through pull_request_file_contents, one read per file at a time, and parses both sides for highlighting. - The file tree on the right (remembered): folders holding one folder drawn as one row, files tinted by their change with its letter; a click unfolds the file and scrolls to its header; more pages load from its footer. - Narrow toolbars fold whitespace, wrap and layout into one menu. --- crates/ui_gpui/assets/icons/settings_2.svg | 16 + crates/ui_gpui/src/diff/mod.rs | 2 + crates/ui_gpui/src/diff/prepare.rs | 2 +- crates/ui_gpui/src/diff/render.rs | 223 ++++++++--- crates/ui_gpui/src/diff/rows.rs | 43 +- crates/ui_gpui/src/diff/whitespace.rs | 149 +++++++ .../right_panel/pull_request/code/aside.rs | 222 +++++++++++ .../right_panel/pull_request/code/body.rs | 173 ++++++-- .../right_panel/pull_request/code/context.rs | 324 +++++++++++++++ .../right_panel/pull_request/code/mod.rs | 370 ++++++++++++++++-- .../right_panel/pull_request/code/pages.rs | 117 +++++- .../right_panel/pull_request/code/rows.rs | 168 ++++---- .../right_panel/pull_request/code/tests.rs | 156 +++++++- .../right_panel/pull_request/code/toolbar.rs | 189 +++++++-- .../right_panel/pull_request/code/tree.rs | 202 ++++++++++ .../pull_request/conversation/fixtures.rs | 1 + .../right_panel/pull_request/preferences.rs | 41 +- 17 files changed, 2127 insertions(+), 271 deletions(-) create mode 100644 crates/ui_gpui/assets/icons/settings_2.svg create mode 100644 crates/ui_gpui/src/diff/whitespace.rs create mode 100644 crates/ui_gpui/src/main_screen/right_panel/pull_request/code/aside.rs create mode 100644 crates/ui_gpui/src/main_screen/right_panel/pull_request/code/context.rs create mode 100644 crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tree.rs diff --git a/crates/ui_gpui/assets/icons/settings_2.svg b/crates/ui_gpui/assets/icons/settings_2.svg new file mode 100644 index 00000000..ad6f98df --- /dev/null +++ b/crates/ui_gpui/assets/icons/settings_2.svg @@ -0,0 +1,16 @@ + + + + + + diff --git a/crates/ui_gpui/src/diff/mod.rs b/crates/ui_gpui/src/diff/mod.rs index 52b2dadb..2a61f641 100644 --- a/crates/ui_gpui/src/diff/mod.rs +++ b/crates/ui_gpui/src/diff/mod.rs @@ -7,6 +7,7 @@ //! - [`prepare`]: [`prepare::PreparedFileDiff`], all of the above for one //! file, built off the UI thread //! - [`split`]: hunks side by side, cut into chunks by rows +//! - [`whitespace`]: hunks with whitespace-only changes shown unchanged //! - [`render`] and [`rows`]: rows as one painted element, with a gutter of //! new or old and new line numbers @@ -17,3 +18,4 @@ pub mod render; pub mod rows; pub mod split; pub mod syntax; +pub mod whitespace; diff --git a/crates/ui_gpui/src/diff/prepare.rs b/crates/ui_gpui/src/diff/prepare.rs index e5e57452..b52a8d48 100644 --- a/crates/ui_gpui/src/diff/prepare.rs +++ b/crates/ui_gpui/src/diff/prepare.rs @@ -15,7 +15,7 @@ const HUNK_CONTEXT_LINES: usize = 3; /// Upper bound for the lines of one list item. A hunk above it (a newly /// added file is a single hunk) is split, so a virtualized list only builds /// what is in view. -const CHUNK_MAX_LINES: usize = 40; +pub const CHUNK_MAX_LINES: usize = 40; /// A file diff reduced to renderable hunks. The expensive parts (line diff, /// syntax parse, line styles) run once on a background thread — the UI only diff --git a/crates/ui_gpui/src/diff/render.rs b/crates/ui_gpui/src/diff/render.rs index 928aa9f5..bd2937c3 100644 --- a/crates/ui_gpui/src/diff/render.rs +++ b/crates/ui_gpui/src/diff/render.rs @@ -5,7 +5,8 @@ use super::chunks::DiffChunk; use super::model::DiffLine; use super::prepare::PreparedFileDiff; -use super::rows::{DiffRow, DiffRows, RowGeometry, RowSelection}; +use super::rows::{DiffRow, DiffRows, HScroll, RowCell, RowGeometry, RowSelection}; +use super::split::{SplitChunk, SplitHunks}; use super::syntax::{DiffSyntax, SyntaxOrigin}; use gpui_kit::component::theme::Theme; use gpui_kit::{ @@ -34,7 +35,7 @@ pub(crate) fn render_diff_chunk( rem_size: Pixels, selection: Option, ) -> AnyElement { - let rows = render_diff_chunk_rows(diff, chunk, numbers, theme, rem_size, selection); + let rows = render_diff_chunk_rows(diff, chunk, numbers, theme, rem_size, selection, None); let Some(hunk) = diff.hunks.get(chunk.hunk) else { return rows; }; @@ -64,7 +65,8 @@ pub(crate) fn render_diff_chunk( } /// The rows of one chunk as [`render_diff_chunk`] draws them, without the -/// separator: for views that draw hunk boundaries themselves. +/// separator: for views that draw hunk boundaries themselves. With +/// `scroll`, lines do not wrap but scroll sideways. pub(crate) fn render_diff_chunk_rows( diff: &PreparedFileDiff, chunk: &DiffChunk, @@ -72,6 +74,7 @@ pub(crate) fn render_diff_chunk_rows( theme: &Theme, rem_size: Pixels, selection: Option, + scroll: Option, ) -> AnyElement { let Some(hunk) = diff.hunks.get(chunk.hunk) else { return div().into_any(); @@ -107,7 +110,82 @@ pub(crate) fn render_diff_chunk_rows( old_width: diff.chunked.old_width, new_width: diff.chunked.new_width, }; - render_diff_rows(lines, theme, Some(gutter), rem_size, syntax, selection) + render_diff_rows( + lines, + theme, + Some(gutter), + rem_size, + syntax, + selection, + scroll, + ) +} + +/// One chunk of a split diff: the old file's lines left with their +/// numbers, the new file's right, a side without a line filled. Cells +/// carry their flat line (`numbers` holds every flat line's numbers), so +/// `selection` counts from 0. +#[allow(clippy::too_many_arguments)] +pub(crate) fn render_split_chunk_rows( + diff: &PreparedFileDiff, + split: &SplitHunks, + chunk: &SplitChunk, + numbers: &[(ChangeTag, usize, usize)], + theme: &Theme, + rem_size: Pixels, + selection: Option, + scroll: Option, +) -> AnyElement { + let Some(hunk) = diff.hunks.get(chunk.hunk) else { + return div().into_any(); + }; + let flat_start = split.flat_starts.get(chunk.hunk).copied().unwrap_or(0); + let (old_width, new_width) = (diff.chunked.old_width, diff.chunked.new_width); + let syntax = diff.syntax.as_deref(); + let cell = |line: Option, left: bool| -> Option { + let dl = hunk.lines.get(line?)?; + let flat = flat_start + line?; + let &(_, old, new) = numbers.get(flat)?; + let gutter = match left { + true => format!("{old:>old_width$}"), + false => format!("{new:>new_width$}"), + }; + let syntax = syntax.map(|syntax| { + let own = if dl.tag == ChangeTag::Delete { + old + } else { + new + }; + match syntax.origin() { + SyntaxOrigin::WholeFile => (syntax, 0, own), + SyntaxOrigin::PerHunk => { + let start = match dl.tag { + ChangeTag::Delete => hunk.old_start, + _ => hunk.new_start, + }; + (syntax, chunk.hunk, own.saturating_sub(start) + 1) + } + } + }); + Some(RowCell { + row: diff_row(dl, Some(gutter.into()), syntax, theme), + line: flat, + }) + }; + let rows = split + .chunk_rows(chunk) + .iter() + .map(|row| [cell(row.left, true), cell(row.right, false)]) + .collect(); + let geometry = row_geometry(Some(old_width.max(new_width)), rem_size); + let mut element = DiffRows::split(rows, geometry, empty_cell_bg(theme)); + if let Some(scroll) = scroll { + element = element.unwrapped(scroll); + } + match selection { + Some(selection) => element.selectable(selection).into_any(), + None => element.into_any(), + } } /// Build the element tree for already-computed diff lines of a snippet. @@ -144,6 +222,7 @@ pub(crate) fn render_diff_lines( start: LineCounter::default(), }), selection, + None, ) } @@ -227,6 +306,63 @@ impl Gutter { } } +/// The horizontal layout of rows with a gutter of `gutter_chars` digits +/// (none without): ~0.5rem per character + 0.75rem padding, rounded to +/// pixels so it aligns to the pixel grid. +fn row_geometry(gutter_chars: Option, rem_size: Pixels) -> RowGeometry { + let gutter_width = gutter_chars.map_or(px(0.), |chars| { + rems(chars as f32 * 0.5 + 0.75).to_pixels(rem_size).round() + }); + RowGeometry { + gutter_width, + gutter_left: rems(0.375).to_pixels(rem_size), + // Without a gutter the text keeps the card's horizontal padding. + content_left: rems(if gutter_chars.is_some() { 0.25 } else { 0.75 }).to_pixels(rem_size), + content_right: rems(0.75).to_pixels(rem_size), + } +} + +/// One line as a row: tinted by its change, with its gutter text, syntax +/// colors (`syntax`: the parse, its segment and the line's number there) +/// and the word emphasis on top. +fn diff_row( + dl: &DiffLine, + gutter: Option, + syntax: Option<(&DiffSyntax, usize, usize)>, + theme: &Theme, +) -> DiffRow { + let (row_bg, gutter_fg) = row_colors(dl.tag, theme); + // Word-level changes get a stronger background via text-run + // highlights, which wrap with the text. Syntax colors come first, the + // word emphasis layers on top. + let syntax_styles = syntax.map_or_else(Vec::new, |(syntax, segment, line)| { + syntax.line_styles(dl.tag, segment, line, &dl.text, &theme.highlight_theme) + }); + let highlights = if dl.emphasis.is_empty() && syntax_styles.is_empty() { + Vec::new() + } else { + let word_bg = word_emphasis_bg(dl.tag, theme); + let emphasis = dl.emphasis.iter().map(|range| { + ( + range.clone(), + gpui_kit::HighlightStyle { + background_color: Some(word_bg), + ..Default::default() + }, + ) + }); + gpui_kit::combine_highlights(syntax_styles, emphasis).collect() + }; + DiffRow { + text: dl.text.clone(), + highlights, + background: row_bg, + edge: row_edge(dl.tag, theme), + color: row_text_color(dl.tag, syntax.is_some(), theme), + gutter: gutter.map(|text| (text, gutter_fg.opacity(0.5))), + } +} + /// Shared row builder: the rows with the `gutter` (when given), as one /// [`DiffRows`] element. fn render_diff_rows( @@ -236,78 +372,28 @@ fn render_diff_rows( rem_size: Pixels, syntax: Option, selection: Option, + scroll: Option, ) -> AnyElement { let mut gutter_lines = gutter.as_ref().map(|g| g.start).unwrap_or_default(); let mut syntax_lines = syntax.as_ref().map(|s| s.start).unwrap_or_default(); - - // Gutter width: compute in rems (~0.5rem per character + 0.75rem - // padding), then convert to rounded pixels so it aligns to the pixel grid. - let gutter_px = gutter.as_ref().map_or(px(0.), |gutter| { - rems(gutter.chars() as f32 * 0.5 + 0.75) - .to_pixels(rem_size) - .round() - }); - let geometry = RowGeometry { - gutter_width: gutter_px, - gutter_left: rems(0.375).to_pixels(rem_size), - // Without a gutter the text keeps the card's horizontal padding. - content_left: rems(if gutter.is_some() { 0.25 } else { 0.75 }).to_pixels(rem_size), - content_right: rems(0.75).to_pixels(rem_size), - }; - + let geometry = row_geometry(gutter.as_ref().map(Gutter::chars), rem_size); let rows = diff_lines .iter() .map(|dl| { - let (row_bg, gutter_fg) = row_colors(dl.tag, theme); let (old_num, new_num) = gutter_lines.step(dl.tag); let syntax_line = syntax_lines.advance(dl.tag); - - let gutter = gutter.as_ref().map(|gutter| { - ( - SharedString::from(gutter.text(old_num, new_num)), - gutter_fg.opacity(0.5), - ) - }); - - // Word-level changes get a stronger background via text-run - // highlights, which wrap with the text. Syntax colors come - // first, the word emphasis layers on top. - let syntax_styles = syntax.as_ref().map_or_else(Vec::new, |s| { - s.syntax.line_styles( - dl.tag, - s.segment, - syntax_line, - &dl.text, - &theme.highlight_theme, - ) - }); - let highlights = if dl.emphasis.is_empty() && syntax_styles.is_empty() { - Vec::new() - } else { - let word_bg = word_emphasis_bg(dl.tag, theme); - let emphasis = dl.emphasis.iter().map(|range| { - ( - range.clone(), - gpui_kit::HighlightStyle { - background_color: Some(word_bg), - ..Default::default() - }, - ) - }); - gpui_kit::combine_highlights(syntax_styles, emphasis).collect() - }; - DiffRow { - text: dl.text.clone(), - highlights, - background: row_bg, - edge: row_edge(dl.tag, theme), - color: row_text_color(dl.tag, syntax.is_some(), theme), - gutter, - } + let gutter_text = gutter + .as_ref() + .map(|gutter| SharedString::from(gutter.text(old_num, new_num))); + let syntax = syntax.as_ref().map(|s| (s.syntax, s.segment, syntax_line)); + diff_row(dl, gutter_text, syntax, theme) }) .collect(); - let element = DiffRows::new(rows, geometry); + let mut element = DiffRows::new(rows, geometry); + if let Some(scroll) = scroll { + element = element.unwrapped(scroll); + } match selection { Some(selection) => element.selectable(selection).into_any(), None => element.into_any(), @@ -332,6 +418,13 @@ fn rgba_color(r: u8, g: u8, b: u8, a: u8) -> Hsla { .into() } +/// The side of a split row that shows no line. +pub(crate) fn empty_cell_bg(theme: &Theme) -> Hsla { + theme + .muted + .opacity(if theme.is_dark() { 0.35 } else { 0.6 }) +} + /// Background of a diff body, behind the rows. pub(crate) fn diff_body_bg(theme: &Theme) -> Hsla { if theme.is_dark() { diff --git a/crates/ui_gpui/src/diff/rows.rs b/crates/ui_gpui/src/diff/rows.rs index d43d69c3..65187927 100644 --- a/crates/ui_gpui/src/diff/rows.rs +++ b/crates/ui_gpui/src/diff/rows.rs @@ -19,6 +19,8 @@ use gpui_kit::{ use std::cell::{Cell, RefCell}; use std::ops::Range; use std::rc::Rc; +use std::sync::Arc; +use std::sync::atomic::{AtomicU32, Ordering}; /// One row, ready to paint. pub(crate) struct DiffRow { @@ -62,31 +64,44 @@ impl RowGeometry { } /// How far unwrapped rows are scrolled sideways, shared by all elements -/// showing one file, and the widest text they have shown. +/// showing one file, and the widest text they have shown. (Atomics: a +/// file's state is built off the UI thread.) #[derive(Clone, Default, Debug)] -pub(crate) struct HScroll { - offset: Rc>, - widest: Rc>, +pub(crate) struct HScroll(Arc); + +#[derive(Default, Debug)] +struct HScrollState { + /// `f32` bits, in pixels. + offset: AtomicU32, + widest: AtomicU32, +} + +fn load(value: &AtomicU32) -> Pixels { + px(f32::from_bits(value.load(Ordering::Relaxed))) +} + +fn store(value: &AtomicU32, pixels: Pixels) { + value.store(f32::from(pixels).to_bits(), Ordering::Relaxed); } impl HScroll { pub fn offset(&self) -> Pixels { - self.offset.get() + load(&self.0.offset) } fn note_width(&self, width: Pixels) { - if width > self.widest.get() { - self.widest.set(width); + if width > load(&self.0.widest) { + store(&self.0.widest, width); } } /// Scroll by `delta` (positive: the text moves right) where `visible` /// of the widest text shows; whether the offset changed. fn scroll_by(&self, delta: Pixels, visible: Pixels) -> bool { - let max = (self.widest.get() - visible).max(px(0.)); - let offset = (self.offset.get() - delta).clamp(px(0.), max); - let changed = offset != self.offset.get(); - self.offset.set(offset); + let max = (load(&self.0.widest) - visible).max(px(0.)); + let offset = (self.offset() - delta).clamp(px(0.), max); + let changed = offset != self.offset(); + store(&self.0.offset, offset); changed } } @@ -404,14 +419,14 @@ fn paint_rows( let mut y = bounds.origin.y; for (cells, row_layout) in rows.iter().zip(&layout.rows) { let height = row_layout.height; - for column in 0..shape.columns { + let columns = cells.iter().zip(&row_layout.cells).take(shape.columns); + for (column, (cell, cell_layout)) in columns.enumerate() { let frame = ColumnFrame { left: bounds.origin.x + column_width * column as f32, width: column_width, }; let band = |width: Pixels| Bounds::new(point(frame.left, y), size(width, height)); - let (Some(cell), Some(cell_layout)) = (&cells[column], &row_layout.cells[column]) - else { + let (Some(cell), Some(cell_layout)) = (cell, cell_layout) else { if let Some(empty) = empty { window.paint_quad(fill(band(frame.width), empty)); } diff --git a/crates/ui_gpui/src/diff/whitespace.rs b/crates/ui_gpui/src/diff/whitespace.rs new file mode 100644 index 00000000..24733bc7 --- /dev/null +++ b/crates/ui_gpui/src/diff/whitespace.rs @@ -0,0 +1,149 @@ +//! Hunks without their whitespace-only changes, ported from T3 Code's +//! `hideWhitespaceChanges`: each hunk's two sides are diffed again with all +//! whitespace taken out of every line, and lines that then match show as +//! unchanged (with the new text). Each side keeps its lines in their +//! order, so line numbers and per-hunk syntax stay valid. + +use super::model::{DiffHunk, DiffLine, push_replace_block}; +use similar::{Algorithm, ChangeTag, DiffOp, capture_diff_slices}; + +fn without_whitespace(text: &str) -> String { + text.chars().filter(|c| !c.is_whitespace()).collect() +} + +/// `hunks` with lines that differ only in whitespace shown as unchanged. +pub fn hide_whitespace_changes(hunks: &[DiffHunk]) -> Vec { + hunks.iter().map(hide_in_hunk).collect() +} + +fn hide_in_hunk(hunk: &DiffHunk) -> DiffHunk { + if hunk.lines.iter().all(|line| line.tag == ChangeTag::Equal) { + return hunk.clone(); + } + let side = |skip: ChangeTag| -> Vec<&str> { + hunk.lines + .iter() + .filter(|line| line.tag != skip) + .map(|line| line.text.as_ref()) + .collect() + }; + let (old, new) = (side(ChangeTag::Insert), side(ChangeTag::Delete)); + let stripped = |lines: &[&str]| lines.iter().map(|l| without_whitespace(l)).collect(); + let (old_stripped, new_stripped): (Vec, Vec) = (stripped(&old), stripped(&new)); + let ops = capture_diff_slices(Algorithm::Myers, &old_stripped, &new_stripped); + + let mut lines = Vec::with_capacity(hunk.lines.len()); + let (mut removed, mut added): (Vec<&str>, Vec<&str>) = (Vec::new(), Vec::new()); + for op in ops { + match op { + DiffOp::Equal { new_index, len, .. } => { + push_replace_block(&removed, &added, &mut lines); + (removed, added) = (Vec::new(), Vec::new()); + lines.extend( + new[new_index..new_index + len] + .iter() + .map(|text| DiffLine::plain(ChangeTag::Equal, text)), + ); + } + DiffOp::Delete { + old_index, old_len, .. + } => removed.extend(&old[old_index..old_index + old_len]), + DiffOp::Insert { + new_index, new_len, .. + } => added.extend(&new[new_index..new_index + new_len]), + DiffOp::Replace { + old_index, + old_len, + new_index, + new_len, + } => { + removed.extend(&old[old_index..old_index + old_len]); + added.extend(&new[new_index..new_index + new_len]); + } + } + } + push_replace_block(&removed, &added, &mut lines); + DiffHunk { + old_start: hunk.old_start, + new_start: hunk.new_start, + header: hunk.header.clone(), + lines, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use ChangeTag::*; + + fn hunk(lines: &[(ChangeTag, &str)]) -> DiffHunk { + DiffHunk { + old_start: 10, + new_start: 12, + header: Some("fn main() {".into()), + lines: lines + .iter() + .map(|&(tag, text)| DiffLine::plain(tag, text)) + .collect(), + } + } + + fn shape(hunk: &DiffHunk) -> Vec<(ChangeTag, String)> { + hunk.lines + .iter() + .map(|line| (line.tag, line.text.to_string())) + .collect() + } + + #[test] + fn reindented_lines_show_unchanged_with_the_new_text() { + let hunks = [hunk(&[ + (Equal, "a"), + (Delete, "foo(1,2)"), + (Delete, "old()"), + (Insert, " foo(1, 2)"), + (Insert, "bar()"), + ])]; + let hidden = hide_whitespace_changes(&hunks); + assert_eq!( + shape(&hidden[0]), + [ + (Equal, "a".into()), + (Equal, " foo(1, 2)".into()), + (Delete, "old()".into()), + (Insert, "bar()".into()), + ] + ); + assert_eq!((hidden[0].old_start, hidden[0].new_start), (10, 12)); + assert_eq!(hidden[0].header.as_deref(), Some("fn main() {")); + } + + #[test] + fn real_changes_and_unchanged_hunks_stay() { + let unchanged = hunk(&[(Equal, "a"), (Equal, "b")]); + assert_eq!( + shape(&hide_whitespace_changes(std::slice::from_ref(&unchanged))[0]), + shape(&unchanged) + ); + let changed = hunk(&[(Delete, "x = 1"), (Insert, "x = 2")]); + assert_eq!( + shape(&hide_whitespace_changes(&[changed])[0]), + [(Delete, "x = 1".into()), (Insert, "x = 2".into())] + ); + } + + #[test] + fn respaced_lines_become_context_but_added_lines_stay() { + let hunks = [hunk(&[ + (Delete, "if x {"), + (Insert, "if x {"), + (Insert, ""), + ])]; + let hidden = hide_whitespace_changes(&hunks); + // A blank line added is still a line added. + assert_eq!( + shape(&hidden[0]), + [(Equal, "if x {".into()), (Insert, "".into())] + ); + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/aside.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/aside.rs new file mode 100644 index 00000000..284461a1 --- /dev/null +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/aside.rs @@ -0,0 +1,222 @@ +//! The file tree beside the Code tab's list, after T3 Code's +//! `DiffFileTree`: "Files" with the count and a collapse / expand all +//! folders button, then the folders and files (a file's name and icon +//! tinted by its change, the change's letter at the end); a click on a +//! file shows it from its header. While more pages follow, a footer loads +//! the next. + +use super::CodeTab; +use super::tree::{TreeRow, tree_rows}; +use crate::shared::file_icons; +use git::ChangeStatus; +use gpui_kit::component::button::{Button, ButtonVariants}; +use gpui_kit::component::{ActiveTheme, Disableable, Icon, Sizable}; +use gpui_kit::{ + AnyElement, Context, Hsla, Pixels, SharedString, Window, div, prelude::*, px, relative, rgb, + uniform_list, +}; +use std::rc::Rc; + +/// A tree row's height. +const ROW_HEIGHT: Pixels = px(22.); + +/// The letter and colour of a change, as `@pierre/trees` draws them. +fn change_mark(status: ChangeStatus, dark: bool) -> (&'static str, Hsla) { + let (letter, light, dark_color) = match status { + ChangeStatus::Added | ChangeStatus::Untracked => ("A", 0x16a994, 0x00cab1), + ChangeStatus::Deleted => ("D", 0xff2e3f, 0xff6762), + ChangeStatus::Renamed | ChangeStatus::Copied => ("R", 0xd5a910, 0xffd452), + ChangeStatus::Modified | ChangeStatus::TypeChanged => ("M", 0x1ca1c7, 0x08c0ef), + }; + (letter, rgb(if dark { dark_color } else { light }).into()) +} + +impl CodeTab { + pub(super) fn render_file_tree( + &mut self, + _window: &mut Window, + cx: &mut Context, + ) -> Option { + if !self.code.file_tree || self.shown.is_empty() { + return None; + } + let theme = cx.theme(); + let (muted, border) = (theme.muted_foreground, theme.border); + let paths: Vec = self + .shown + .iter() + .map(|&s| self.file(s).path.clone()) + .collect(); + let rows = { + let paths: Vec<&str> = paths.iter().map(String::as_str).collect(); + Rc::new(tree_rows(&paths, &self.tree_collapsed)) + }; + let count = self.shown.len(); + let header = div() + .flex_none() + .h(px(32.)) + .px_2() + .flex() + .items_center() + .gap_1() + .text_xs() + .child( + div() + .font_weight(gpui_kit::FontWeight::MEDIUM) + .text_color(theme.foreground) + .child("Files"), + ) + .child(div().text_color(muted).child(count.to_string())) + .child(div().flex_1()) + .child( + Button::new("pr-code-tree-folders") + .ghost() + .xsmall() + .icon(Icon::default().path("icons/chevrons_down_up.svg")) + .tooltip("Collapse or expand all folders") + .on_click(cx.listener(|this, _, _, cx| this.toggle_all_folders(cx))), + ); + let list = uniform_list( + "pr-code-tree", + rows.len(), + cx.processor( + move |this: &mut Self, range: std::ops::Range, _, cx| { + range + .filter_map(|ix| rows.get(ix).map(|row| this.render_tree_row(ix, row, cx))) + .collect() + }, + ), + ) + .track_scroll(&self.tree_scroll) + .flex_1(); + let footer = self + .pages + .last() + .and_then(|page| page.next_cursor.as_ref()) + .map(|_| { + let (label, retry) = match (&self.load_error, &self.loading) { + (Some(_), _) => ("Retry", true), + (None, Some(_)) => ("Loading more files...", false), + (None, None) => ("Load more files", false), + }; + div().flex_none().p_2().child( + Button::new("pr-code-tree-more") + .outline() + .xsmall() + .w_full() + .label(label) + .disabled(self.loading.is_some() && !retry) + .on_click(cx.listener(move |this, _, _, cx| match retry { + true => this.retry(cx), + false => this.load_next(cx), + })), + ) + }); + Some( + div() + .debug_selector(|| "pr-code-tree".into()) + .flex_none() + .w(relative(0.4)) + .max_w(px(320.)) + .min_w(px(192.)) + .h_full() + .flex() + .flex_col() + .border_l_1() + .border_color(border) + .bg(theme.background) + .child(header) + .child(list) + .children(footer) + .into_any_element(), + ) + } + + fn render_tree_row(&self, ix: usize, row: &TreeRow, cx: &mut Context) -> AnyElement { + let theme = cx.theme(); + let indent = |depth: usize| px(8. + depth as f32 * 12.); + let line = div() + .id(("pr-code-tree-row", ix)) + .h(ROW_HEIGHT) + .w_full() + .flex() + .items_center() + .gap_1() + .pr_2() + .text_xs() + .cursor_pointer() + .hover(|s| s.bg(theme.list_hover)); + let name = |name: &str, color: Hsla| { + div() + .flex_1() + .min_w_0() + .overflow_hidden() + .whitespace_nowrap() + .text_ellipsis() + .text_color(color) + .child(SharedString::from(name.to_owned())) + }; + match row { + TreeRow::Folder { + path, + name: folder, + depth, + collapsed, + } => { + let icon = file_icons::get().get_type_icon(if *collapsed { + "collapsed_folder" + } else { + "expanded_folder" + }); + let path = path.clone(); + line.pl(indent(*depth)) + .child( + div().w(px(10.)).flex_none().child( + gpui_kit::svg() + .size(px(10.)) + .path(if *collapsed { + "icons/chevron_right.svg" + } else { + "icons/chevron_down.svg" + }) + .text_color(theme.muted_foreground), + ), + ) + .child(file_icons::render_icon( + &icon, + 14.0, + theme.muted_foreground, + "·", + )) + .child(name(folder, theme.foreground)) + .on_click( + cx.listener(move |this, _, _, cx| this.toggle_folder(path.clone(), cx)), + ) + .into_any_element() + } + TreeRow::File { + file, + name: file_name, + depth, + } => { + let Some(&shown) = self.shown.get(*file) else { + return div().into_any_element(); + }; + let loaded = self.file(shown); + let (letter, color) = change_mark(loaded.status, theme.is_dark()); + let selected = self.tree_selected.as_deref() == Some(loaded.path.as_str()); + let icon = file_icons::get().get_icon_for_filename(file_name); + let file = *file; + let selector = format!("pr-code-tree-file-{file}"); + line.pl(indent(*depth) + px(14.)) + .debug_selector(move || selector) + .when(selected, |d| d.bg(theme.list_active)) + .child(file_icons::render_icon(&icon, 14.0, color, "·")) + .child(name(file_name, color)) + .child(div().flex_none().text_color(color).child(letter)) + .on_click(cx.listener(move |this, _, _, cx| this.reveal_file(file, cx))) + .into_any_element() + } + } + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs index 8d9c589d..ce7b4dfd 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/body.rs @@ -3,12 +3,13 @@ //! states shown instead of the list, the strip of threads not on the diff //! and the header that stays on top while its file scrolls. -use super::rows::{CodeRow, gap, gap_label}; +use super::context::{Reveal, ShownGap, gap_reveals}; +use super::rows::{CodeRow, gap_label}; use super::{CodeTab, Shown}; -use crate::diff::render::{GutterNumbers, render_diff_chunk_rows}; +use crate::diff::render::{GutterNumbers, render_diff_chunk_rows, render_split_chunk_rows}; use crate::main_screen::right_panel::line_selection::{ChunkMarks, row_selection}; -use gpui_kit::component::button::Button; -use gpui_kit::component::{ActiveTheme, Sizable}; +use gpui_kit::component::button::{Button, ButtonVariants}; +use gpui_kit::component::{ActiveTheme, Icon, Sizable}; use gpui_kit::{ AnyElement, App, Context, FontWeight, SharedString, Window, div, prelude::*, px, rems, svg, }; @@ -48,9 +49,13 @@ impl CodeTab { Some(shown) => self.render_chunk(shown, chunk, window, cx), None => div().into_any_element(), }, - CodeRow::Tail { .. } => { - hairline_label("More unchanged context may be available".into(), false, cx) - } + CodeRow::Tail { file, .. } => match shown(file) { + Some(shown) => { + let after = self.file(shown).diff.hunks.len(); + self.render_gap(shown, after, cx) + } + None => div().into_any_element(), + }, CodeRow::Thread { thread, .. } => match self.thread_card(thread, cx) { Some(card) => annotation(card.into_any_element()), None => div().into_any_element(), @@ -81,14 +86,81 @@ impl CodeTab { .into_any_element() } - fn render_gap(&self, shown: Shown, hunk: usize, cx: &App) -> AnyElement { - let hunks = &self.file(shown).diff.hunks; - let label = hunks - .get(hunk) - .and_then(|h| gap(hunk.checked_sub(1).and_then(|p| hunks.get(p)), h)) - .map(|(lines, section)| gap_label(lines, section)) - .unwrap_or_default(); - hairline_label(label.into(), true, cx) + /// The unchanged lines before shown hunk `before` (after the last when + /// it is the hunk count): their count and section on a hairline, and + /// the buttons revealing them where the file can be read. + fn render_gap(&self, shown: Shown, before: usize, cx: &mut Context) -> AnyElement { + let file = self.file(shown); + let Some(gap) = file.gaps.iter().find(|gap| gap.before == before).cloned() else { + return div().into_any_element(); + }; + let label = gap_label(gap.hidden, gap.section.as_deref()); + let reveals = match file.has_context() { + true => gap_reveals(&gap, file.patch_hunks.len()), + false => Vec::new(), + }; + let loading = self.text_reads.contains_key(&file.path); + let buttons: Vec = reveals + .into_iter() + .map(|reveal| self.reveal_button(shown, &gap, reveal, loading, cx)) + .collect(); + div() + .w_full() + .h(px(24.)) + .flex() + .items_center() + .gap_1() + .px_1() + .bg(cx.theme().muted.opacity(0.35)) + .children(buttons) + .child(hairline_label(label.into(), cx)) + .into_any_element() + } + + fn reveal_button( + &self, + shown: Shown, + gap: &ShownGap, + reveal: Reveal, + loading: bool, + cx: &mut Context, + ) -> AnyElement { + let (icon, tooltip, name) = match reveal { + Reveal::Down => ("icons/chevron_down.svg", "Expand down", "down"), + Reveal::Up => ("icons/chevron_up.svg", "Expand up", "up"), + Reveal::All => ("icons/chevron_up_down.svg", "Expand all", "all"), + }; + let gap_ix = gap.gap; + let id = SharedString::from(format!( + "pr-code-expand-{}-{}-{gap_ix}-{name}", + shown.page, shown.file + )); + let selector = id.to_string(); + let button = Button::new(id) + .ghost() + .xsmall() + .loading(loading) + .tooltip(tooltip) + .on_click(cx.listener(move |this, _, window, cx| { + // Shift shows the whole gap, as in T3 Code. + let reveal = match window.modifiers().shift { + true => Reveal::All, + false => reveal, + }; + this.reveal(shown, gap_ix, reveal, cx); + })); + let button = match reveal { + // Between hunks a large gap offers "Expand all" as text. + Reveal::All if gap.hidden.is_some_and(|h| h > super::context::EXPAND_STEP) => { + button.label("Expand all") + } + _ => button.icon(Icon::default().path(icon)), + }; + div() + .flex_none() + .debug_selector(move || selector) + .child(button) + .into_any_element() } /// One chunk of a file's diff, its rows selectable on the whole change. @@ -100,44 +172,76 @@ impl CodeTab { cx: &Context, ) -> AnyElement { let file = self.file(shown); - let Some(chunk) = file.diff.chunked.chunks.get(chunk_ix) else { - return div().into_any_element(); - }; let theme = cx.theme(); let rem_size = window.rem_size(); - let rows = chunk.lines.len(); + // The flat lines the chunk shows, for the marks and the anchor. + let (lines, base_line) = match &file.split { + Some(split) => { + let Some(chunk) = split.chunks.get(chunk_ix) else { + return div().into_any_element(); + }; + let flat = split.flat_starts.get(chunk.hunk).copied().unwrap_or(0); + let rows = split.chunk_rows(chunk); + let lines = rows.iter().flat_map(|row| [row.left, row.right]).flatten(); + let lo = lines.clone().min().unwrap_or(0) + flat; + let hi = lines.max().unwrap_or(0) + flat; + (lo..hi + 1, 0) + } + None => { + let Some(chunk) = file.diff.chunked.chunks.get(chunk_ix) else { + return div().into_any_element(); + }; + let start = chunk.flat_start; + (start..start + chunk.lines.len(), start) + } + }; let selection = self.commit.is_none().then(|| { let marks = ChunkMarks { highlight: self.selection.highlight(&file.path), color: theme.selection, - marked: self.pending_marks(file, chunk.flat_start, rows, cx), + marked: self.pending_marks(file, lines.start, lines.len(), cx), mark_color: theme.warning, anchor: self - .anchors_selection_end(&file.path, chunk.flat_start, rows) + .anchors_selection_end(&file.path, lines.start, lines.len()) .then(|| self.selection_anchor.clone()), }; row_selection( file.path.clone(), - chunk.flat_start, + base_line, marks, self.focus_handle.clone(), cx, ) }); + let scroll = (!self.code.wrap).then(|| file.scroll.clone()); + let rows = match &file.split { + Some(split) => render_split_chunk_rows( + &file.diff, + split, + &split.chunks[chunk_ix], + &file.numbers, + theme, + rem_size, + selection, + scroll, + ), + None => render_diff_chunk_rows( + &file.diff, + &file.diff.chunked.chunks[chunk_ix], + GutterNumbers::New, + theme, + rem_size, + selection, + scroll, + ), + }; div() .w_full() .text_size(rems(0.78125)) .line_height(rems(1.25).to_pixels(rem_size).round()) .font_family(theme.mono_font_family.clone()) .font_weight(FontWeight(400.0)) - .child(render_diff_chunk_rows( - &file.diff, - chunk, - GutterNumbers::New, - theme, - rem_size, - selection, - )) + .child(rows) .into_any_element() } @@ -341,14 +445,13 @@ pub(super) fn annotation(card: AnyElement) -> AnyElement { } /// Centred muted text on a hairline ("30 unmodified lines"). -fn hairline_label(label: SharedString, above: bool, cx: &App) -> AnyElement { +fn hairline_label(label: SharedString, cx: &App) -> AnyElement { let theme = cx.theme(); let line = || div().flex_1().h(px(1.)).bg(theme.border); div() - .w_full() - .px_3() - .when(above, |d| d.pt_2().pb_1()) - .when(!above, |d| d.pt_1().pb_2()) + .flex_1() + .min_w_0() + .px_2() .flex() .items_center() .gap_3() diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/context.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/context.rs new file mode 100644 index 00000000..8155f816 --- /dev/null +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/context.rs @@ -0,0 +1,324 @@ +//! The unchanged lines between a file's hunks, and showing them. GitHub's +//! patch leaves them out; the gaps' sizes follow from the hunk headers, +//! except past the last hunk, which is unknown until the file is read +//! ("More unchanged context may be available"). Like T3 Code (`@pierre/ +//! diffs`), a click reveals 100 lines at a time from the side of a gap it +//! names, or the whole gap; the lines come from the new file's text, read +//! on the first click (`pull_request_file_contents`). A gap shown whole +//! joins the hunks around it. + +use crate::diff::model::{DiffHunk, DiffLine}; +use gpui_kit::SharedString; +use similar::ChangeTag; +use std::collections::BTreeMap; + +/// Lines one click reveals. +pub const EXPAND_STEP: usize = 100; + +/// Lines shown of one gap: from its top (below the hunk above) and from +/// its bottom (above the hunk below). Either may exceed the gap. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct Revealed { + pub top: usize, + pub bottom: usize, +} + +/// What a click on a gap asks for. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Reveal { + /// The lines below the hunk above ("expand down"). + Down, + /// The lines above the hunk below ("expand up"). + Up, + All, +} + +impl Revealed { + pub fn apply(&mut self, reveal: Reveal) { + match reveal { + Reveal::Down => self.top = self.top.saturating_add(EXPAND_STEP), + Reveal::Up => self.bottom = self.bottom.saturating_add(EXPAND_STEP), + Reveal::All => (self.top, self.bottom) = (usize::MAX, usize::MAX), + } + } +} + +/// A gap that still hides lines (or names a section). +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ShownGap { + /// Its index among the patch's gaps: before the patch's hunk `gap`, or + /// after the last one (`gap` = the patch's hunk count). + pub gap: usize, + /// The shown hunk it comes before; the shown hunk count after the last. + pub before: usize, + /// Unchanged lines hidden; `None` past the last hunk until the file's + /// length is known. + pub hidden: Option, + /// The `@@` section of the hunk below. + pub section: Option, +} + +/// The buttons of a gap, after T3 Code: one that shows a small gap whole; +/// for a larger one, the directions there are lines to come from, and +/// "Expand all" between hunks. +pub fn gap_reveals(gap: &ShownGap, patch_hunks: usize) -> Vec { + let leading = gap.gap == 0; + let trailing = gap.gap == patch_hunks; + match gap.hidden { + Some(0) => Vec::new(), + Some(hidden) if hidden <= EXPAND_STEP => vec![Reveal::All], + _ if leading => vec![Reveal::Up], + _ if trailing => vec![Reveal::Down], + _ => vec![Reveal::Down, Reveal::Up, Reveal::All], + } +} + +/// A hunk's first line and the line after it, on the old and new side. +/// A side without lines starts after the line its header names. +fn span(hunk: &DiffHunk) -> ((usize, usize), (usize, usize)) { + let count = |skip: ChangeTag| hunk.lines.iter().filter(|l| l.tag != skip).count(); + let (old, new) = (count(ChangeTag::Insert), count(ChangeTag::Delete)); + let old_first = hunk.old_start + usize::from(old == 0); + let new_first = hunk.new_start + usize::from(new == 0); + ((old_first, old_first + old), (new_first, new_first + new)) +} + +/// The file's hunks with the revealed lines added, and the gaps that +/// still hide some. `new_lines` is the new file's text by line, once read +/// (nothing is revealed before); `tail` whether the file may go on past +/// its last hunk (a modified or renamed file). +pub fn shape( + hunks: &[DiffHunk], + revealed: &BTreeMap, + new_lines: Option<&[SharedString]>, + tail: bool, +) -> (Vec, Vec) { + let mut shown: Vec = Vec::with_capacity(hunks.len()); + let mut gaps = Vec::new(); + let context = |from: usize, count: usize| -> Vec { + let lines = new_lines.unwrap_or_default(); + (from..from + count) + .map(|line| { + let text = lines.get(line - 1).map_or("", |t| t.as_ref()); + DiffLine::plain(ChangeTag::Equal, text) + }) + .collect() + }; + // The lines after the previous hunk, old and new side. + let (mut old_next, mut new_next) = (1, 1); + for (ix, hunk) in hunks.iter().enumerate() { + let ((old_first, old_after), (new_first, new_after)) = span(hunk); + let size = old_first.saturating_sub(old_next); + let wanted = revealed.get(&ix).copied().unwrap_or_default(); + let (top, bottom) = match new_lines { + None => (0, 0), + // Above the first hunk only lines next to it show. + Some(_) if ix == 0 => (0, wanted.bottom.min(size)), + Some(_) => { + let top = wanted.top.min(size); + (top, wanted.bottom.min(size - top)) + } + }; + if let Some(last) = shown.last_mut() { + last.lines.extend(context(new_next, top)); + } + let hidden = size - top - bottom; + if ix > 0 && size > 0 && hidden == 0 { + // Shown whole: one hunk with the one above. + let last = shown.last_mut().expect("a hunk above"); + last.lines.extend(context(new_next + top, bottom)); + last.lines.extend(hunk.lines.iter().cloned()); + } else { + if hidden > 0 || hunk.header.is_some() { + gaps.push(ShownGap { + gap: ix, + before: shown.len(), + hidden: Some(hidden), + section: hunk.header.clone(), + }); + } + let mut lines = context(new_first - bottom, bottom); + lines.extend(hunk.lines.iter().cloned()); + shown.push(DiffHunk { + old_start: if bottom > 0 { + old_first - bottom + } else { + hunk.old_start + }, + new_start: if bottom > 0 { + new_first - bottom + } else { + hunk.new_start + }, + header: hunk.header.clone(), + lines, + }); + } + (old_next, new_next) = (old_after, new_after); + } + if tail && let Some(last) = shown.last_mut() { + let wanted = revealed.get(&hunks.len()).copied().unwrap_or_default(); + let size = new_lines.map(|lines| (lines.len() + 1).saturating_sub(new_next)); + let top = size.map_or(0, |size| wanted.top.min(size)); + last.lines.extend(context(new_next, top)); + let hidden = size.map(|size| size - top); + if hidden != Some(0) { + gaps.push(ShownGap { + gap: hunks.len(), + before: shown.len(), + hidden, + section: None, + }); + } + } + (shown, gaps) +} + +/// A file's text by line, as `shape` takes it. +pub fn lines_of(text: &str) -> Vec { + text.lines() + .map(|line| SharedString::from(line.to_string())) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use ChangeTag::*; + + fn hunk( + old_start: usize, + new_start: usize, + header: Option<&str>, + tags: &[ChangeTag], + ) -> DiffHunk { + DiffHunk { + old_start, + new_start, + header: header.map(SharedString::from), + lines: tags + .iter() + .map(|&tag| DiffLine::plain(tag, "changed")) + .collect(), + } + } + + /// Lines 1..=n of the new file, `"line k"`. + fn file(n: usize) -> Vec { + (1..=n).map(|k| format!("line {k}").into()).collect() + } + + fn texts(hunk: &DiffHunk) -> Vec { + hunk.lines.iter().map(|l| l.text.to_string()).collect() + } + + /// Hunk 0 covers old 11..=13 (new 11..=14), hunk 1 old 31..=31. + fn patch() -> Vec { + vec![ + hunk(11, 11, None, &[Equal, Delete, Insert, Insert, Equal]), + hunk(31, 32, Some("fn main() {"), &[Equal]), + ] + } + + #[test] + fn gaps_are_sized_from_the_headers_and_the_tail_is_unknown() { + let (hunks, gaps) = shape(&patch(), &BTreeMap::new(), None, true); + assert_eq!(hunks.len(), 2); + let summary: Vec<_> = gaps.iter().map(|g| (g.gap, g.before, g.hidden)).collect(); + assert_eq!(summary, [(0, 0, Some(10)), (1, 1, Some(17)), (2, 2, None)]); + assert_eq!(gaps[1].section.as_deref(), Some("fn main() {")); + // Added or deleted files have no tail. + let (_, gaps) = shape(&patch(), &BTreeMap::new(), None, false); + assert_eq!(gaps.len(), 2); + } + + #[test] + fn revealing_needs_the_file_and_takes_lines_from_both_ends() { + let revealed = BTreeMap::from([ + (0, Revealed { top: 5, bottom: 3 }), + (1, Revealed { top: 2, bottom: 4 }), + ]); + let (unread, _) = shape(&patch(), &revealed, None, true); + assert_eq!(unread[0].lines.len(), 5, "nothing without the file"); + + let lines = file(40); + let (hunks, gaps) = shape(&patch(), &revealed, Some(&lines), true); + // Above the first hunk only the lines next to it: new 8..=10. + assert_eq!((hunks[0].old_start, hunks[0].new_start), (8, 8)); + assert_eq!(&texts(&hunks[0])[..3], ["line 8", "line 9", "line 10"]); + // Below hunk 0: new 15, 16; above hunk 1: new 28..=31. + assert_eq!(&texts(&hunks[0])[8..], ["line 15", "line 16"]); + assert_eq!((hunks[1].old_start, hunks[1].new_start), (27, 28)); + assert_eq!( + &texts(&hunks[1])[..4], + ["line 28", "line 29", "line 30", "line 31"] + ); + let hidden: Vec<_> = gaps.iter().map(|g| g.hidden).collect(); + // The tail is known now: new 33..=40. + assert_eq!(hidden, [Some(7), Some(11), Some(8)]); + } + + #[test] + fn a_gap_shown_whole_joins_its_hunks_and_the_tail_ends() { + let lines = file(33); + let revealed = BTreeMap::from([ + ( + 1, + Revealed { + top: usize::MAX, + bottom: 0, + }, + ), + ( + 2, + Revealed { + top: usize::MAX, + bottom: 0, + }, + ), + ]); + let (hunks, gaps) = shape(&patch(), &revealed, Some(&lines), true); + assert_eq!(hunks.len(), 1); + assert_eq!(hunks[0].lines.len(), 5 + 17 + 1 + 1); + assert_eq!(texts(&hunks[0]).last().unwrap(), "line 33"); + let left: Vec<_> = gaps.iter().map(|g| (g.gap, g.hidden)).collect(); + assert_eq!(left, [(0, Some(10))]); + } + + #[test] + fn small_gaps_show_whole_with_one_click_larger_ones_by_side() { + let gap = |gap, hidden| ShownGap { + gap, + before: gap, + hidden, + section: None, + }; + use Reveal::*; + assert_eq!(gap_reveals(&gap(1, Some(30)), 3), [All]); + assert_eq!(gap_reveals(&gap(1, Some(300)), 3), [Down, Up, All]); + assert_eq!(gap_reveals(&gap(0, Some(300)), 3), [Up]); + assert_eq!(gap_reveals(&gap(3, Some(300)), 3), [Down]); + assert_eq!(gap_reveals(&gap(3, None), 3), [Down]); + assert!(gap_reveals(&gap(1, Some(0)), 3).is_empty()); + + let mut revealed = Revealed::default(); + revealed.apply(Down); + revealed.apply(Up); + revealed.apply(Up); + assert_eq!( + revealed, + Revealed { + top: 100, + bottom: 200 + } + ); + } + + #[test] + fn a_hunk_without_old_lines_starts_after_its_header_line() { + // `@@ -7,0 +8,2 @@`: inserted after old line 7. + let hunks = [hunk(7, 8, None, &[Insert, Insert])]; + let (_, gaps) = shape(&hunks, &BTreeMap::new(), None, false); + assert_eq!(gaps[0].hidden, Some(7)); + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/mod.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/mod.rs index 05d94d24..8b21d181 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/mod.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/mod.rs @@ -8,7 +8,11 @@ //! ([`pages`]). A re-read (the core announces a changed diff, or the head //! moved) keeps unchanged pages and everything built from them. //! - **Order**: per page by imports and tiers, then as a file tree would -//! list them ([`file_order`]); tests can be hidden ([`file_kind`]). +//! list them ([`file_order`]); tests can be hidden ([`file_kind`]); the +//! file tree beside the list ([`tree`]) jumps to a file. +//! - **Shape**: stacked or split, wrapped or scrolled sideways, with or +//! without whitespace changes ([`pages`]); unchanged lines between hunks +//! revealed from the file's text, read on the first click ([`context`]). //! - **Viewed**: optimistic ticks, written in batches ([`viewed`]); ticking //! folds a file, a shift-click ticks a range ([`folds`]). //! - **List**: one virtualized list over flat rows ([`rows`]): headers, @@ -22,8 +26,10 @@ //! anchors comments to the whole change. mod anchors; +mod aside; mod body; mod comments; +mod context; mod file_header; mod file_kind; mod file_order; @@ -31,6 +37,7 @@ mod folds; mod pages; mod rows; mod toolbar; +mod tree; mod viewed; use super::PullRequestIntent; @@ -38,28 +45,31 @@ use super::bus::{PullRequestEvent, bus}; use super::conversation::thread::ThreadCard; use super::data::{PullRequestData, session_service}; use super::detail::TabScroll; +use super::preferences::{self, CodePreferences, DiffLayout}; use super::review_draft::{ReviewDrafts, review_drafts}; use crate::comments::AnchorTracker; use crate::diff::prepare::highlight_patch; +use crate::diff::syntax::DiffSyntax; use crate::main_screen::right_panel::line_selection::{LineSelection, Selected, SelectsLines}; use crate::main_screen::right_panel::review_rows::changed_span; use anchors::{Annotation, LineRef, place}; use code_assistant_core::pull_requests::PullRequestPart; use comments::Draft; +use context::{Reveal, lines_of}; use file_order::order_like_tree; use folds::{Folds, viewed_range}; use github::PullRequestRef; -use github::model::{Activity, ReviewThread}; +use github::model::{Activity, FileChange, ReviewThread}; use gpui_kit::component::ActiveTheme; use gpui_kit::{ App, Bounds, Context, Entity, EventEmitter, FocusHandle, Focusable, KeyDownEvent, - ListAlignment, ListState, Pixels, Render, SharedString, Subscription, Task, Window, div, list, - prelude::*, px, + ListAlignment, ListOffset, ListState, Pixels, Render, SharedString, Subscription, Task, + UniformListScrollHandle, Window, div, list, prelude::*, px, }; -use pages::{Arrival, LoadedFile, Page, arrival, fingerprint, parse_page}; -use rows::{CodeRow, FileBody, FileOutline, files_by_proximity, flatten}; +use pages::{Arrival, DiffView, FileText, LoadedFile, Page, arrival, fingerprint, parse_page}; +use rows::{ChunkOutline, CodeRow, FileBody, FileOutline, files_by_proximity, flatten}; use std::cell::Cell; -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::hash::{Hash, Hasher}; use std::rc::Rc; use std::sync::Arc; @@ -75,7 +85,7 @@ const LIST_OVERDRAW: Pixels = px(512.); /// A shown file: its page and its index there. #[derive(Clone, Copy, Debug, PartialEq, Eq)] -struct Shown { +pub(super) struct Shown { page: usize, file: usize, } @@ -102,8 +112,21 @@ pub struct CodeTab { /// The files in the list, in order. shown: Vec, test_files: usize, - hide_tests: bool, + /// Layout, wrap, whitespace, tests, file tree (remembered). + code: CodePreferences, folds: Folds, + /// Files' texts being read, by path: one read per file at a time. + text_reads: HashMap>, + /// Clicks on a gap made before the file's text was read. + queued_reveals: HashMap>, + + /// The file tree's collapsed folders and the file last picked there. + tree_collapsed: HashSet, + tree_selected: Option, + tree_scroll: UniformListScrollHandle, + /// The toolbar's width at the last paint: narrow, it folds the diff + /// options into a menu. + toolbar_width: Rc>, viewed: ViewedFiles, viewed_read: Option>, @@ -187,8 +210,14 @@ impl CodeTab { next_stamp: 0, shown: Vec::new(), test_files: 0, - hide_tests: false, + code: preferences::read(cx).code, folds: Folds::default(), + text_reads: HashMap::new(), + queued_reveals: HashMap::new(), + tree_collapsed: HashSet::new(), + tree_selected: None, + tree_scroll: UniformListScrollHandle::new(), + toolbar_width: Rc::new(Cell::new(px(f32::MAX))), viewed: ViewedFiles::default(), viewed_read: None, viewed_flush: None, @@ -297,6 +326,7 @@ impl CodeTab { return; }; let (pr, commit, scope) = (self.pr.clone(), self.commit.clone(), self.scope); + let view = self.diff_view(); self.loading = Some(cx.spawn(async move |this, cx| { let started = Instant::now(); let result = service.pull_request_diff(pr, commit, cursor.clone()).await; @@ -331,7 +361,9 @@ impl CodeTab { } let (next_cursor, truncated) = (page.next_cursor.clone(), page.truncated); let parsing = Instant::now(); - let files = cx.background_spawn(async move { parse_page(&page) }).await; + let files = cx + .background_spawn(async move { parse_page(&page, view) }) + .await; tracing::debug!( files = files.len(), elapsed = ?parsing.elapsed(), @@ -359,7 +391,11 @@ impl CodeTab { self.loading = None; self.load_error = None; if let Arrival::Place(ix) = arrival(&self.pages, page.cursor.as_deref(), page.fingerprint) { + // Shaped off the UI thread for the view then; it may have + // changed meanwhile. + let view = self.diff_view(); for file in &mut page.files { + file.reshape(view); file.stamp = self.stamp(); } self.pages.truncate(ix); @@ -378,15 +414,19 @@ impl CodeTab { } self.sync_rows(); let top = self.list_state.logical_scroll_top().item_ix; + // A file whose text was read is highlighted from it. let Some(shown) = files_by_proximity(&self.rows, self.shown.len(), top) .into_iter() .map(|ix| self.shown[ix]) - .find(|&shown| !self.file(shown).highlighted) + .find(|&shown| { + let file = self.file(shown); + !file.highlighted && file.text.is_none() + }) else { return; }; let file = self.file(shown); - let (path, hunks) = (file.path.clone(), file.diff.hunks.clone()); + let (path, hunks) = (file.path.clone(), file.patch_hunks.clone()); let (page, print, scope) = (shown.page, self.pages[shown.page].fingerprint, self.scope); // Captured here: the line styles are computed off the UI thread. let theme = cx.theme().highlight_theme.clone(); @@ -405,7 +445,12 @@ impl CodeTab { .filter(|p| p.fingerprint == print) .and_then(|p| p.files.iter_mut().find(|f| f.path == path)) { - file.diff.set_syntax(syntax); + // It fits the hunks without whitespace changes too: + // each side keeps its lines. + if file.text.is_none() { + file.diff.set_syntax(syntax.clone()); + } + file.hunk_syntax = syntax; file.highlighted = true; cx.notify(); } @@ -467,9 +512,10 @@ impl CodeTab { .map(|ix| all[ix]) .collect(); self.test_files = ordered.iter().filter(|&&s| self.file(s).is_test).count(); + let hide_tests = self.code.hide_tests; self.shown = ordered .into_iter() - .filter(|&s| !(self.hide_tests && self.file(s).is_test)) + .filter(|&s| !(hide_tests && self.file(s).is_test)) .collect(); let shown: Vec = self .shown @@ -489,11 +535,253 @@ impl CodeTab { } fn set_hide_tests(&mut self, hide: bool, cx: &mut Context) { - self.hide_tests = hide; + self.change_code(cx, |code| code.hide_tests = hide); self.sync_files(cx); cx.notify(); } + // -- Diff options ------------------------------------------------------ + + fn diff_view(&self) -> DiffView { + DiffView { + hide_whitespace: self.code.hide_whitespace, + layout: self.code.layout, + } + } + + /// Change the Code tab's preferences, here and remembered. + fn change_code(&mut self, cx: &mut Context, change: impl FnOnce(&mut CodePreferences)) { + change(&mut self.code); + let code = self.code; + preferences::update(cx, |preferences| preferences.code = code); + } + + /// Every file gets new rows (their heights change). + fn restamp(&mut self) { + for page in 0..self.pages.len() { + for file in 0..self.pages[page].files.len() { + let stamp = self.stamp(); + self.pages[page].files[file].stamp = stamp; + } + } + self.dirty = true; + } + + fn set_layout(&mut self, layout: DiffLayout, cx: &mut Context) { + if layout == self.code.layout { + return; + } + self.change_code(cx, |code| code.layout = layout); + for file in self.pages.iter_mut().flat_map(|p| &mut p.files) { + let breaks = std::mem::take(&mut file.breaks); + file.cut(&breaks, layout); + } + self.restamp(); + cx.notify(); + } + + fn set_wrap(&mut self, wrap: bool, cx: &mut Context) { + self.change_code(cx, |code| code.wrap = wrap); + self.restamp(); + cx.notify(); + } + + /// Like T3 Code, the selection and an open comment box go: their rows + /// change. + fn set_hide_whitespace(&mut self, hide: bool, cx: &mut Context) { + self.change_code(cx, |code| code.hide_whitespace = hide); + let view = self.diff_view(); + for file in self.pages.iter_mut().flat_map(|p| &mut p.files) { + file.reshape(view); + } + self.restamp(); + self.selection.clear(); + self.draft = None; + self.place_annotations(cx); + } + + fn set_file_tree(&mut self, open: bool, cx: &mut Context) { + self.change_code(cx, |code| code.file_tree = open); + cx.notify(); + } + + // -- Context ----------------------------------------------------------- + + /// A click on a gap of the shown file: reveal its lines, once the + /// file's text is read. + fn reveal(&mut self, shown: Shown, gap: usize, reveal: Reveal, cx: &mut Context) { + let Some(file) = self + .pages + .get_mut(shown.page) + .and_then(|p| p.files.get_mut(shown.file)) + else { + return; + }; + if file.text.is_none() { + self.queued_reveals + .entry(file.path.clone()) + .or_default() + .push((gap, reveal)); + self.read_text(shown, cx); + return; + } + file.revealed.entry(gap).or_default().apply(reveal); + self.file_reshaped(shown, cx); + } + + /// The shown file changed shape: new rows, annotations placed anew. + fn file_reshaped(&mut self, shown: Shown, cx: &mut Context) { + let view = self.diff_view(); + let stamp = self.stamp(); + let file = self.file_mut(shown); + file.reshape(view); + file.stamp = stamp; + self.place_annotations(cx); + } + + /// Read the shown file's text (both sides, parsed for highlighting), + /// one read per file at a time; the clicks queued meanwhile apply then. + fn read_text(&mut self, shown: Shown, cx: &mut Context) { + let file = self.file(shown); + let path = file.path.clone(); + if self.text_reads.contains_key(&path) { + return; + } + let Some(service) = session_service(cx) else { + return; + }; + let change = match file.status { + git::ChangeStatus::Renamed => FileChange::Renamed, + _ => FileChange::Modified, + }; + let old_path = file.orig_path.clone().unwrap_or_else(|| path.clone()); + let (pr, commit, scope) = (self.pr.clone(), self.commit.clone(), self.scope); + let theme = cx.theme().highlight_theme.clone(); + let read = { + let path = path.clone(); + cx.spawn(async move |this, cx| { + let contents = service + .pull_request_file_contents(pr, commit, change, old_path, path.clone()) + .await; + let text = match contents { + Ok(contents) => { + let path = path.clone(); + Ok(cx + .background_spawn(async move { + let syntax = DiffSyntax::parse( + &path, + Some(&contents.old), + Some(&contents.new), + ) + .map(|syntax| { + syntax.prime(&theme); + Arc::new(syntax) + }); + FileText { + new_lines: lines_of(&contents.new), + syntax, + } + }) + .await) + } + Err(error) => Err(error), + }; + this.update(cx, |this, cx| this.text_read(&path, scope, text, cx)) + .ok(); + }) + }; + self.text_reads.insert(path, read); + cx.notify(); + } + + fn text_read( + &mut self, + path: &str, + scope: u64, + text: anyhow::Result, + cx: &mut Context, + ) { + self.text_reads.remove(path); + let reveals = self.queued_reveals.remove(path).unwrap_or_default(); + let text = match text { + Ok(text) => text, + Err(error) => { + // T3 Code only logs it; the next click tries again. + self.data.update(cx, |data, cx| { + data.error = Some(format!("Could not read {path}: {error:#}").into()); + cx.notify(); + }); + cx.notify(); + return; + } + }; + let Some(shown) = self + .shown_ix(path) + .map(|ix| self.shown[ix]) + .filter(|_| self.scope == scope) + else { + return; + }; + let file = self.file_mut(shown); + file.text = Some(text); + for (gap, reveal) in reveals { + file.revealed.entry(gap).or_default().apply(reveal); + } + self.file_reshaped(shown, cx); + } + + // -- File tree --------------------------------------------------------- + + /// Show the shown file `ix` from its header (unfolded), as a click in + /// the file tree asks. + fn reveal_file(&mut self, ix: usize, cx: &mut Context) { + let Some(&shown) = self.shown.get(ix) else { + return; + }; + let path = self.file(shown).path.clone(); + if self.folds.is_folded(&path) { + self.folds.set(&path, false); + self.dirty = true; + } + self.sync_rows(); + if let Some(row) = self + .rows + .iter() + .position(|row| *row == CodeRow::File { file: ix }) + { + self.list_state.scroll_to(ListOffset { + item_ix: row, + offset_in_item: px(0.), + }); + } + self.tree_selected = Some(path); + cx.notify(); + } + + fn toggle_folder(&mut self, path: String, cx: &mut Context) { + if !self.tree_collapsed.remove(&path) { + self.tree_collapsed.insert(path); + } + cx.notify(); + } + + /// Collapse every folder, or expand them all when all are collapsed. + fn toggle_all_folders(&mut self, cx: &mut Context) { + let paths: Vec = self + .shown + .iter() + .map(|&s| self.file(s).path.clone()) + .collect(); + let paths: Vec<&str> = paths.iter().map(String::as_str).collect(); + let folders = tree::folder_paths(&paths); + if !folders.is_empty() && folders.is_subset(&self.tree_collapsed) { + self.tree_collapsed.clear(); + } else { + self.tree_collapsed = folders; + } + cx.notify(); + } + /// Put threads, pending comments and the comment box below their rows /// (in a commit's scope none: its line numbers are not the change's), /// and cut the chunks there. @@ -537,10 +825,9 @@ impl CodeTab { } let breaks: Vec = placed.keys().copied().collect(); if file.breaks != breaks { - let stamp = self.stamp(); + let (stamp, layout) = (self.stamp(), self.code.layout); let file = self.file_mut(shown); - file.diff.rechunk(&breaks); - file.breaks = breaks; + file.cut(&breaks, layout); file.stamp = stamp; } annotations.push(placed); @@ -702,15 +989,35 @@ impl CodeTab { let body = if diff.is_binary || diff.too_large || diff.hunks.is_empty() { FileBody::Note } else { + let chunks = match &file.split { + Some(split) => split + .chunks + .iter() + .map(|chunk| ChunkOutline { + hunk: chunk.hunk, + starts_hunk: chunk.rows.start == 0, + last_lines: split.last_lines(chunk), + }) + .collect(), + None => diff + .chunked + .chunks + .iter() + .map(|chunk| ChunkOutline { + hunk: chunk.hunk, + starts_hunk: chunk.lines.start == 0, + last_lines: vec![ + chunk.flat_start + chunk.lines.len().saturating_sub(1), + ], + }) + .collect(), + }; FileBody::Diff { - hunks: &diff.hunks, - chunks: &diff.chunked.chunks, + hunk_count: diff.hunks.len(), + chunks, + gaps: &file.gaps, annotations, thread_version: &thread_version, - tail: matches!( - file.status, - git::ChangeStatus::Modified | git::ChangeStatus::Renamed - ), } }; FileOutline { @@ -789,12 +1096,19 @@ impl Render for CodeTab { }), ) .size_full(); - div() + let code = div() .relative() .flex_1() - .min_h_0() + .min_w_0() + .h_full() .child(list) - .children(self.render_sticky_header(cx)) + .children(self.render_sticky_header(cx)); + div() + .flex_1() + .min_h_0() + .flex() + .child(code) + .children(self.render_file_tree(window, cx)) .into_any_element() } }; diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/pages.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/pages.rs index 04dd9fc3..34955878 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/pages.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/pages.rs @@ -5,22 +5,66 @@ //! re-read page that did not change keeps everything, a changed one //! replaces itself and drops the pages after it (their cursors point into //! the old diff). +//! +//! A file keeps its hunks as the patch has them and without whitespace +//! changes; what the list shows is shaped from one of them +//! ([`LoadedFile::reshape`]): the unchanged lines revealed around them +//! added, cut into chunks, side by side for the split layout. +use super::super::preferences::DiffLayout; use super::anchors::LineIndex; +use super::context::{Revealed, ShownGap, shape}; use super::file_kind::is_test_file; use super::file_order::order_files; -use crate::diff::prepare::PreparedFileDiff; +use crate::diff::model::DiffHunk; +use crate::diff::prepare::{CHUNK_MAX_LINES, PreparedFileDiff}; +use crate::diff::rows::HScroll; +use crate::diff::split::SplitHunks; +use crate::diff::syntax::DiffSyntax; +use crate::diff::whitespace::hide_whitespace_changes; use git::ChangeStatus; use github::model::DiffPage; +use gpui_kit::SharedString; use similar::ChangeTag; +use std::collections::BTreeMap; use std::hash::{Hash, Hasher}; +use std::sync::Arc; + +/// How the files are shown. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct DiffView { + pub hide_whitespace: bool, + pub layout: DiffLayout, +} + +/// A file's new text, read to reveal unchanged lines, and both sides +/// parsed for highlighting. +pub struct FileText { + pub new_lines: Vec, + pub syntax: Option>, +} /// One changed file of a page. pub struct LoadedFile { pub path: String, pub orig_path: Option, pub status: ChangeStatus, + /// The hunks as the patch has them, and without whitespace changes. + pub patch_hunks: Vec, + pub quiet_hunks: Vec, + /// What the list shows, shaped from one of the above. pub diff: PreparedFileDiff, + /// The gaps between the shown hunks still hiding lines. + pub gaps: Vec, + /// The shown hunks side by side, in the split layout. + pub split: Option, + /// Unchanged lines revealed, by gap. + pub revealed: BTreeMap, + pub text: Option, + /// The patch hunks' syntax, once highlighted. + pub hunk_syntax: Option>, + /// How far the rows scroll sideways when lines do not wrap. + pub scroll: HScroll, /// Each row's change and line numbers (flat line indices). pub numbers: Vec<(ChangeTag, usize, usize)>, pub lines: LineIndex, @@ -36,6 +80,49 @@ pub struct LoadedFile { pub highlighted: bool, } +impl LoadedFile { + /// Whether lines may follow the last hunk, and unchanged lines can be + /// revealed (T3 Code: changed and renamed files). + pub fn has_context(&self) -> bool { + matches!(self.status, ChangeStatus::Modified | ChangeStatus::Renamed) + && !self.diff.is_binary + && !self.patch_hunks.is_empty() + } + + /// Shape what the list shows from the hunks, the revealed lines and + /// `view`; the chunks are cut anew, without breaks. + pub fn reshape(&mut self, view: DiffView) { + let base = match view.hide_whitespace { + true => &self.quiet_hunks, + false => &self.patch_hunks, + }; + let new_lines = self.text.as_ref().map(|text| &text.new_lines[..]); + let (hunks, gaps) = shape(base, &self.revealed, new_lines, self.has_context()); + self.diff.hunks = hunks; + self.gaps = gaps; + self.numbers = self.diff.row_numbers(); + self.lines = LineIndex::new(&self.numbers); + // The patch's syntax fits as long as no line was added to a hunk; + // revealing needs the file, which brings its own. + let syntax = match &self.text { + Some(text) => text.syntax.clone(), + None => self.hunk_syntax.clone(), + }; + self.diff.set_syntax(syntax); + self.cut(&[], view.layout); + } + + /// Cut the chunks so one ends after each flat line in `breaks`. + pub fn cut(&mut self, breaks: &[usize], layout: DiffLayout) { + self.diff.rechunk(breaks); + self.breaks = breaks.to_vec(); + // A new or deleted file has one side only: it stays stacked. + let two_sided = !matches!(self.status, ChangeStatus::Added | ChangeStatus::Deleted); + self.split = (layout == DiffLayout::Split && two_sided) + .then(|| SplitHunks::new(&self.diff.hunks, CHUNK_MAX_LINES, breaks)); + } +} + pub struct Page { /// The cursor this page was read with (`None` for the first). pub cursor: Option, @@ -90,9 +177,9 @@ pub fn fingerprint(page: &DiffPage) -> u64 { } /// The files of `page`, in the order they are listed, with their diffs -/// prepared but not highlighted. Call off the UI thread (a few ms for a -/// big page). Stamps are left for the view to give. -pub fn parse_page(page: &DiffPage) -> Vec { +/// prepared but not highlighted, shaped for `view`. Call off the UI thread +/// (a few ms for a big page). Stamps are left for the view to give. +pub fn parse_page(page: &DiffPage, view: DiffView) -> Vec { let mut files: Vec> = git::parse_patch(&page.patch) .into_iter() .map(|patch| { @@ -108,11 +195,20 @@ pub fn parse_page(page: &DiffPage) -> Vec { { (additions, deletions) = (stat.additions, stat.deletions); } - Some(LoadedFile { + let patch_hunks = diff.hunks.clone(); + let mut file = LoadedFile { is_test: is_test_file(&patch.path), lines: LineIndex::new(&numbers), numbers, + quiet_hunks: hide_whitespace_changes(&patch_hunks), + patch_hunks, diff, + gaps: Vec::new(), + split: None, + revealed: BTreeMap::new(), + text: None, + hunk_syntax: None, + scroll: HScroll::default(), additions, deletions, path: patch.path, @@ -121,7 +217,9 @@ pub fn parse_page(page: &DiffPage) -> Vec { stamp: 0, breaks: Vec::new(), highlighted: false, - }) + }; + file.reshape(view); + Some(file) }) .collect(); let paths: Vec = files @@ -183,6 +281,11 @@ mod tests { assert_eq!(arrival(&pages, Some("2"), 9), Arrival::Place(1)); } + const VIEW: DiffView = DiffView { + hide_whitespace: false, + layout: DiffLayout::Stacked, + }; + const PATCH: &str = "\ diff --git a/src/b.ts b/src/b.ts --- a/src/b.ts @@ -218,7 +321,7 @@ diff --git a/src/z.ts b/src/z.ts deletions: 1, }], }; - let files = parse_page(&page); + let files = parse_page(&page, VIEW); let paths: Vec<&str> = files.iter().map(|f| f.path.as_str()).collect(); // `a.ts` imports `z.ts`; everything else in path order. assert_eq!(paths, ["img.png", "src/b.ts", "src/z.ts", "src/a.ts"]); diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/rows.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/rows.rs index 2f4c177c..ce003f0a 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/rows.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/rows.rs @@ -1,15 +1,14 @@ //! The Code tab as a flat list of rows, the items of its virtualized list: -//! per file a header, then (unless folded) the hunks as chunks, each hunk -//! after a gap row ("30 unmodified lines"), threads and comments right -//! below the row they are on, and the loading footer at the end. Rows that -//! stay the same compare equal, so a change reaches the list as one splice +//! per file a header, then (unless folded) the hunks as chunks (stacked or +//! side by side), each hunk after its gap row ("30 unmodified lines") when +//! it has one, threads and comments right below the row they are on, and +//! the loading footer at the end. Rows that stay the same compare equal, +//! so a change reaches the list as one splice //! ([`changed_span`](crate::main_screen::right_panel::review_rows::changed_span)) //! and the scroll position survives. use super::anchors::Annotation; -use crate::diff::chunks::DiffChunk; -use crate::diff::model::DiffHunk; -use similar::ChangeTag; +use super::context::ShownGap; use std::collections::BTreeMap; /// One item of the list. `file` indexes the shown files; `stamp` names @@ -25,7 +24,7 @@ pub enum CodeRow { file: usize, stamp: u64, }, - /// The unchanged lines before hunk `hunk`, and its section. + /// The unchanged lines before shown hunk `hunk`, and its section. Gap { file: usize, hunk: usize, @@ -36,7 +35,7 @@ pub enum CodeRow { chunk: usize, stamp: u64, }, - /// Below the last hunk of a file that has lines past it. + /// Below the last hunk of a file that may have lines past it. Tail { file: usize, stamp: u64, @@ -57,18 +56,27 @@ pub enum CodeRow { Footer, } +/// A chunk as the rows need it. +pub struct ChunkOutline { + pub hunk: usize, + /// The chunk opens its hunk. + pub starts_hunk: bool, + /// The flat lines its last row shows: what goes below it. + pub last_lines: Vec, +} + /// What a file contributes below its header. pub enum FileBody<'a> { /// Binary, too large or no content change: a note says which. Note, Diff { - hunks: &'a [DiffHunk], - chunks: &'a [DiffChunk], + hunk_count: usize, + chunks: Vec, + gaps: &'a [ShownGap], /// Annotations by the flat line they go below; chunks end there. annotations: &'a BTreeMap>, /// Versions of the threads, by index. thread_version: &'a dyn Fn(usize) -> u64, - tail: bool, }, } @@ -89,44 +97,41 @@ pub fn flatten(files: &[FileOutline], footer: bool) -> Vec { match &outline.body { FileBody::Note => rows.push(CodeRow::Note { file, stamp }), FileBody::Diff { - hunks, + hunk_count, chunks, + gaps, annotations, thread_version, - tail, } => { - let mut prev = None; + let gap_before = |hunk: usize| gaps.iter().any(|gap| gap.before == hunk); for (ix, chunk) in chunks.iter().enumerate() { - if chunk.lines.start == 0 - && let Some(hunk) = hunks.get(chunk.hunk) - && gap(prev, hunk).is_some() - { + if chunk.starts_hunk && gap_before(chunk.hunk) { rows.push(CodeRow::Gap { file, hunk: chunk.hunk, stamp, }); } - prev = hunks.get(chunk.hunk); rows.push(CodeRow::Chunk { file, chunk: ix, stamp, }); - let last = chunk.flat_start + chunk.lines.len().saturating_sub(1); - for annotation in annotations.get(&last).into_iter().flatten() { - rows.push(match *annotation { - Annotation::Thread(thread) => CodeRow::Thread { - file, - thread, - version: thread_version(thread), - }, - Annotation::Pending(id) => CodeRow::Pending { file, id }, - Annotation::Draft => CodeRow::Draft { file }, - }); + for &line in &chunk.last_lines { + for annotation in annotations.get(&line).into_iter().flatten() { + rows.push(match *annotation { + Annotation::Thread(thread) => CodeRow::Thread { + file, + thread, + version: thread_version(thread), + }, + Annotation::Pending(id) => CodeRow::Pending { file, id }, + Annotation::Draft => CodeRow::Draft { file }, + }); + } } } - if *tail && !chunks.is_empty() { + if !chunks.is_empty() && gap_before(*hunk_count) { rows.push(CodeRow::Tail { file, stamp }); } } @@ -167,28 +172,14 @@ pub fn files_by_proximity(rows: &[CodeRow], files: usize, top_row: usize) -> Vec (top..files).chain((0..top).rev()).collect() } -/// What separates `hunk` from the one before (`None` for the first): the -/// unchanged lines between them and its section, when there is either. -pub fn gap<'a>(prev: Option<&DiffHunk>, hunk: &'a DiffHunk) -> Option<(usize, Option<&'a str>)> { - let prev_end = prev.map_or(1, |prev| { - prev.old_start - + prev - .lines - .iter() - .filter(|l| l.tag != ChangeTag::Insert) - .count() - }); - let lines = hunk.old_start.saturating_sub(prev_end); - let section = hunk.header.as_deref(); - (lines > 0 || section.is_some()).then_some((lines, section)) -} - -/// The text of a gap row. -pub fn gap_label(lines: usize, section: Option<&str>) -> String { +/// The text of a gap row; `None` lines past the last hunk while the file's +/// length is unknown. +pub fn gap_label(lines: Option, section: Option<&str>) -> String { let count = match lines { - 0 => None, - 1 => Some("1 unmodified line".to_owned()), - n => Some(format!("{n} unmodified lines")), + None => return "More unchanged context may be available".to_owned(), + Some(0) => None, + Some(1) => Some("1 unmodified line".to_owned()), + Some(n) => Some(format!("{n} unmodified lines")), }; // A section is the raw `@@` context: one line, its spacing collapsed. let section = section @@ -205,32 +196,6 @@ pub fn gap_label(lines: usize, section: Option<&str>) -> String { #[cfg(test)] mod tests { use super::*; - use crate::diff::chunks::chunk_hunks; - use crate::diff::model::DiffLine; - use ChangeTag::*; - - fn hunk(old_start: usize, header: Option<&str>, tags: &[ChangeTag]) -> DiffHunk { - DiffHunk { - old_start, - new_start: old_start, - header: header.map(|h| h.to_owned().into()), - lines: tags.iter().map(|&tag| DiffLine::plain(tag, "x")).collect(), - } - } - - #[test] - fn gaps_count_the_old_files_unchanged_lines() { - let first = hunk(31, None, &[Equal, Insert, Delete, Equal]); - assert_eq!(gap(None, &first), Some((30, None))); - // `first` covers old lines 31..=33. - let second = hunk(40, Some("fn main() {"), &[Equal]); - assert_eq!(gap(Some(&first), &second), Some((6, Some("fn main() {")))); - let adjacent = hunk(34, None, &[Equal]); - assert_eq!(gap(Some(&first), &adjacent), None); - // A new file's hunk starts at old line 0. - assert_eq!(gap(None, &hunk(0, None, &[Insert])), None); - assert_eq!(gap(None, &hunk(1, None, &[Delete])), None); - } #[test] fn files_highlight_from_the_top_of_the_view_outwards() { @@ -258,41 +223,57 @@ mod tests { #[test] fn gap_labels() { - assert_eq!(gap_label(30, None), "30 unmodified lines"); - assert_eq!(gap_label(1, None), "1 unmodified line"); + assert_eq!(gap_label(Some(30), None), "30 unmodified lines"); + assert_eq!(gap_label(Some(1), None), "1 unmodified line"); assert_eq!( - gap_label(4, Some("impl A {")), + gap_label(Some(4), Some("impl A {")), "4 unmodified lines · impl A {" ); - assert_eq!(gap_label(0, Some("impl A {")), "⋯ impl A {"); + assert_eq!(gap_label(Some(0), Some("impl A {")), "⋯ impl A {"); assert_eq!( - gap_label(2, Some(" Layer 5:\t code — CLI ")), + gap_label(Some(2), Some(" Layer 5:\t code — CLI ")), "2 unmodified lines · Layer 5: code — CLI" ); - assert_eq!(gap_label(2, Some(" ")), "2 unmodified lines"); + assert_eq!(gap_label(Some(2), Some(" ")), "2 unmodified lines"); + assert_eq!( + gap_label(None, None), + "More unchanged context may be available" + ); } #[test] fn files_list_gaps_chunks_annotations_and_tail_in_order() { - let hunks = vec![ - hunk(5, None, &[Equal, Insert, Equal]), - hunk(8, None, &[Equal, Delete]), - ]; let mut annotations = BTreeMap::new(); annotations.insert(1, vec![Annotation::Thread(4), Annotation::Draft]); annotations.insert(4, vec![Annotation::Pending(9)]); - let chunked = chunk_hunks(&hunks, 40, &[1]); + let gap = |before, hidden| ShownGap { + gap: before, + before, + hidden, + section: None, + }; + let gaps = [gap(0, Some(4)), gap(1, Some(1)), gap(2, None)]; + let chunk = |hunk, starts_hunk, last_lines: &[usize]| ChunkOutline { + hunk, + starts_hunk, + last_lines: last_lines.to_vec(), + }; let version = |thread: usize| thread as u64 * 10; let files = [ FileOutline { folded: false, stamp: 7, body: FileBody::Diff { - hunks: &hunks, - chunks: &chunked.chunks, + hunk_count: 2, + // A split chunk's last row shows two lines. + chunks: vec![ + chunk(0, true, &[1]), + chunk(0, false, &[2]), + chunk(1, true, &[3, 4]), + ], + gaps: &gaps, annotations: &annotations, thread_version: &version, - tail: true, }, }, FileOutline { @@ -332,7 +313,6 @@ mod tests { chunk: 1, stamp: 7 }, - // Hunk 0 ends at old line 6, hunk 1 starts at 8. Gap { file: 0, hunk: 1, diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tests.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tests.rs index 643051ce..e4699894 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tests.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tests.rs @@ -3,6 +3,7 @@ use super::super::conversation::fixtures::{commit, data, thread, window}; use super::super::fixtures::hours_ago; +use super::super::preferences::DiffLayout; use super::*; use github::model::{DiffPage, DiffSide}; use gpui_kit::{Modifiers, TestAppContext, VisualTestContext}; @@ -50,7 +51,7 @@ fn arrive( omitted_file_stats: Vec::new(), }; tab.update(cx, |tab, cx| { - let files = parse_page(&page); + let files = parse_page(&page, tab.diff_view()); tab.page_arrived( Page { cursor: cursor.map(str::to_owned), @@ -350,7 +351,7 @@ fn rows_show_plain_first_and_highlight_file_by_file(cx: &mut TestAppContext) { fingerprint: fingerprint(&page), next_cursor: None, truncated: false, - files: parse_page(&page), + files: parse_page(&page, tab.diff_view()), }, cx, ); @@ -371,3 +372,154 @@ fn rows_show_plain_first_and_highlight_file_by_file(cx: &mut TestAppContext) { }); assert_eq!(rows(&tab, cx), plain, "highlighting adds no rows"); } + +/// A page with this patch text arrives first. +fn arrive_patch(tab: &Entity, cx: &mut VisualTestContext, patch: &str) { + let page = DiffPage { + patch: patch.to_owned(), + truncated: false, + next_cursor: None, + omitted_file_stats: Vec::new(), + }; + tab.update(cx, |tab, cx| { + let files = parse_page(&page, tab.diff_view()); + tab.page_arrived( + Page { + cursor: None, + fingerprint: fingerprint(&page), + next_cursor: None, + truncated: false, + files, + }, + cx, + ) + }); + cx.run_until_parked(); +} + +/// The first shown file's line tags, `' '`, `-`, `+`. +fn tags(tab: &Entity, cx: &mut VisualTestContext) -> String { + tab.read_with(cx, |tab, _| { + let file = tab.file(tab.shown[0]); + file.diff + .hunks + .iter() + .flat_map(|h| &h.lines) + .map(|l| match l.tag { + similar::ChangeTag::Equal => ' ', + similar::ChangeTag::Delete => '-', + similar::ChangeTag::Insert => '+', + }) + .collect() + }) +} + +#[gpui_kit::test] +fn whitespace_changes_hide_and_show_again(cx: &mut TestAppContext) { + let (tab, _, cx) = tab(cx); + let patch = "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n\ + @@ -1,3 +1,3 @@\n ctx\n-fn f(a,b) {}\n+fn f(a, b) {}\n-x\n+y\n"; + arrive_patch(&tab, cx, patch); + // Hidden by default, as in T3 Code. + assert_eq!(tags(&tab, cx), " -+"); + tab.update(cx, |tab, cx| tab.set_hide_whitespace(false, cx)); + assert_eq!(tags(&tab, cx), " -+-+"); + tab.update(cx, |tab, cx| tab.set_hide_whitespace(true, cx)); + assert_eq!(tags(&tab, cx), " -+"); +} + +#[gpui_kit::test] +fn the_split_layout_pairs_rows_but_keeps_new_files_stacked(cx: &mut TestAppContext) { + let (tab, _, cx) = tab(cx); + let patch = "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n\ + @@ -10,3 +10,3 @@\n ctx\n-old\n+new\n ctx\n\ + diff --git a/b.rs b/b.rs\nnew file mode 100644\n--- /dev/null\n+++ b/b.rs\n\ + @@ -0,0 +1,2 @@\n+one\n+two\n"; + arrive_patch(&tab, cx, patch); + let stacked = rows(&tab, cx); + tab.update(cx, |tab, cx| tab.set_layout(DiffLayout::Split, cx)); + cx.run_until_parked(); + tab.read_with(cx, |tab, _| { + let split: Vec<_> = tab + .shown + .iter() + .map(|&s| tab.file(s).split.as_ref().map(|split| split.rows[0].len())) + .collect(); + // ctx, old|new, ctx: three rows; the new file has one side only. + assert_eq!(split, [Some(3), None]); + }); + assert_ne!(rows(&tab, cx), stacked, "new rows for the new layout"); +} + +#[gpui_kit::test] +fn a_gap_reveals_lines_once_the_file_is_read(cx: &mut TestAppContext) { + let (tab, _, cx) = tab(cx); + arrive(&tab, cx, None, &["src/a.rs"], None); + let shown = tab.read_with(cx, |tab, _| tab.shown[0]); + // The first click queues itself until the text is read. + tab.update(cx, |tab, cx| tab.reveal(shown, 0, Reveal::All, cx)); + tab.read_with(cx, |tab, _| { + assert_eq!(tab.file(shown).gaps[0].hidden, Some(9)); + }); + let text = FileText { + new_lines: (1..=20).map(|n| format!("line {n}").into()).collect(), + syntax: None, + }; + let scope = tab.read_with(cx, |tab, _| tab.scope); + tab.update(cx, |tab, cx| tab.text_read("src/a.rs", scope, Ok(text), cx)); + cx.run_until_parked(); + tab.read_with(cx, |tab, _| { + let file = tab.file(shown); + assert_eq!(file.diff.hunks[0].new_start, 1); + assert_eq!(file.diff.hunks[0].lines[0].text.as_ref(), "line 1"); + // Only the tail is left: new lines 13..=20. + let gaps: Vec<_> = file.gaps.iter().map(|g| (g.gap, g.hidden)).collect(); + assert_eq!(gaps, [(0, Some(0)), (1, Some(8))]); + }); + // A gap shown whole but naming a section keeps its row, without + // buttons; the tail can be revealed. + assert!(cx.debug_bounds("pr-code-expand-0-0-0-all").is_none()); + assert!(cx.debug_bounds("pr-code-expand-0-0-1-all").is_some()); +} + +#[gpui_kit::test] +fn the_file_tree_shows_a_file_from_its_header(cx: &mut TestAppContext) { + let (tab, _, cx) = tab(cx); + let paths: Vec = (0..30).map(|n| format!("src/f{n:02}.rs")).collect(); + let paths: Vec<&str> = paths.iter().map(String::as_str).collect(); + arrive(&tab, cx, None, &paths, None); + assert!(cx.debug_bounds("pr-code-tree").is_none()); + tab.update(cx, |tab, cx| tab.set_file_tree(true, cx)); + cx.run_until_parked(); + assert!(cx.debug_bounds("pr-code-tree").is_some()); + let target = tab.read_with(cx, |tab, _| { + tab.shown + .iter() + .position(|&s| tab.file(s).path == "src/f05.rs") + .unwrap() + }); + tab.update(cx, |tab, cx| tab.toggle_fold(target, cx)); + click( + cx, + &format!("pr-code-tree-file-{target}"), + Modifiers::none(), + ); + tab.read_with(cx, |tab, _| { + assert_eq!(tab.tree_selected.as_deref(), Some("src/f05.rs")); + assert!(!tab.folds.is_folded("src/f05.rs"), "unfolded to show it"); + let top = tab.list_state.logical_scroll_top().item_ix; + assert_eq!(tab.rows[top], CodeRow::File { file: target }); + }); +} + +#[gpui_kit::test] +fn unwrapping_gives_every_file_new_rows(cx: &mut TestAppContext) { + let (tab, _, cx) = tab(cx); + arrive(&tab, cx, None, &["src/a.rs"], None); + let wrapped = rows(&tab, cx); + tab.update(cx, |tab, cx| tab.set_wrap(false, cx)); + cx.run_until_parked(); + let unwrapped = rows(&tab, cx); + assert_eq!(unwrapped.len(), wrapped.len()); + assert_ne!(unwrapped, wrapped, "heights change: rows are measured anew"); +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/toolbar.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/toolbar.rs index fb40e954..821e5ae4 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/toolbar.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/toolbar.rs @@ -1,20 +1,29 @@ //! The Code tab's toolbar: the commit scope ("All commits" or one commit, //! newest first, ten at a time), "6 files · 0 / 6 viewed", hidden tests, -//! and on the right collapse / expand all and the "hide tests" toggle. -//! -//! Not here yet: hiding whitespace changes (GitHub's diff endpoint takes -//! no such option through the core), unwrapped lines (diff rows always -//! wrap), the split layout and the file tree. +//! and on the right, as in T3 Code: hide whitespace changes, stacked | +//! split, wrap (folded into one menu when the toolbar is narrow), collapse +//! / expand all, hide tests and the file tree. +use super::super::preferences::DiffLayout; use super::CodeTab; -use gpui_kit::component::button::{Button, ButtonVariants}; +use gpui_kit::component::button::{Button, ButtonGroup, ButtonVariants}; +use gpui_kit::component::menu::{DropdownMenu, PopupMenuItem}; use gpui_kit::component::popover::{Popover, PopoverState}; use gpui_kit::component::{ActiveTheme, Icon, Selectable, Sizable}; -use gpui_kit::{AnyElement, Context, SharedString, WeakEntity, Window, div, prelude::*, px, svg}; +use gpui_kit::{ + AnyElement, Context, SharedString, WeakEntity, Window, canvas, div, prelude::*, px, svg, +}; /// Commits the scope menu lists at a time. pub const COMMIT_PAGE: usize = 10; +/// Below this width the diff options fold into a menu (T3 Code: 32rem). +const NARROW: f32 = 512.; + +fn icon(path: &'static str) -> Icon { + Icon::default().path(SharedString::from(path)) +} + impl CodeTab { pub(super) fn render_toolbar(&self, cx: &mut Context) -> AnyElement { let scope = self.render_scope(cx); @@ -27,7 +36,11 @@ impl CodeTab { .iter() .filter(|&&s| self.viewed.is_viewed(&self.file(s).path)) .count(); - let hidden_tests = if self.hide_tests { self.test_files } else { 0 }; + let hidden_tests = if self.code.hide_tests { + self.test_files + } else { + 0 + }; let warning = |id: &'static str, tooltip: &'static str| { div() .id(id) @@ -106,17 +119,124 @@ impl CodeTab { }); let all_folded = self.all_folded(); + let code = self.code; + let narrow = self.toolbar_width.get() < px(NARROW); + let view = cx.entity().downgrade(); + let toggle = |id: &'static str, path: &'static str, on: bool, tip: SharedString| { + Button::new(id) + .ghost() + .xsmall() + .selected(on) + .toggled(on) + .icon(icon(path)) + .tooltip(tip) + }; + let options = if narrow { + let view = view.clone(); + vec![ + Button::new("pr-code-options") + .ghost() + .xsmall() + .icon(icon("icons/settings_2.svg")) + .tooltip("Diff options") + .dropdown_menu_with_anchor(gpui_kit::Anchor::TopRight, move |menu, _, _| { + let act = |change: fn(&mut CodeTab, &mut Context)| { + let view = view.clone(); + move |_: &gpui_kit::ClickEvent, + _: &mut Window, + cx: &mut gpui_kit::App| { + view.update(cx, change).ok(); + } + }; + menu.item( + PopupMenuItem::new("Hide whitespace changes") + .checked(code.hide_whitespace) + .on_click(act(|tab, cx| { + tab.set_hide_whitespace(!tab.code.hide_whitespace, cx) + })), + ) + .item( + PopupMenuItem::new("Wrap lines") + .checked(code.wrap) + .on_click(act(|tab, cx| tab.set_wrap(!tab.code.wrap, cx))), + ) + .separator() + .item( + PopupMenuItem::new("Stacked") + .checked(code.layout == DiffLayout::Stacked) + .on_click(act(|tab, cx| tab.set_layout(DiffLayout::Stacked, cx))), + ) + .item( + PopupMenuItem::new("Split") + .checked(code.layout == DiffLayout::Split) + .on_click(act(|tab, cx| tab.set_layout(DiffLayout::Split, cx))), + ) + }) + .into_any_element(), + ] + } else { + let hide = code.hide_whitespace; + let split = code.layout == DiffLayout::Split; + vec![ + toggle( + "pr-code-whitespace", + "icons/pilcrow.svg", + hide, + match hide { + true => "Show whitespace changes".into(), + false => "Hide whitespace changes".into(), + }, + ) + .on_click(cx.listener(move |this, _, _, cx| this.set_hide_whitespace(!hide, cx))) + .into_any_element(), + ButtonGroup::new("pr-code-layout") + .ghost() + .xsmall() + .child( + Button::new("pr-code-stacked") + .icon(icon("icons/rows_3.svg")) + .tooltip("Stacked diff view") + .selected(!split), + ) + .child( + Button::new("pr-code-split") + .icon(icon("icons/columns_2.svg")) + .tooltip("Split diff view") + .selected(split), + ) + .on_click(cx.listener(|this, clicked: &Vec, _, cx| { + let layout = match clicked.first() { + Some(1) => DiffLayout::Split, + _ => DiffLayout::Stacked, + }; + this.set_layout(layout, cx); + })) + .into_any_element(), + toggle( + "pr-code-wrap", + "icons/text_wrap.svg", + code.wrap, + match code.wrap { + true => "Don't wrap lines".into(), + false => "Wrap lines".into(), + }, + ) + .on_click(cx.listener(move |this, _, _, cx| this.set_wrap(!code.wrap, cx))) + .into_any_element(), + ] + }; let right = div() .flex_none() .flex() .items_center() .gap_1() + .children(options) .when(files > 0, |d| { d.child( Button::new("pr-code-fold-all") .ghost() .xsmall() - .icon(Icon::default().path(if all_folded { + .icon(icon(if all_folded { "icons/chevron_up_down.svg" } else { "icons/chevrons_down_up.svg" @@ -132,26 +252,46 @@ impl CodeTab { ) }) .when(self.test_files > 0, |d| { - let hide = self.hide_tests; + let hide = code.hide_tests; d.child( - Button::new("pr-code-hide-tests") - .ghost() - .xsmall() - .selected(hide) - .toggled(hide) - .icon(Icon::default().path("icons/flask_conical_off.svg")) - .tooltip(if hide { - "Show test files" - } else { - "Hide test files" - }) - .on_click( - cx.listener(move |this, _, _, cx| this.set_hide_tests(!hide, cx)), - ), + toggle( + "pr-code-hide-tests", + "icons/flask_conical_off.svg", + hide, + match hide { + true => "Show test files".into(), + false => "Hide test files".into(), + }, + ) + .on_click(cx.listener(move |this, _, _, cx| this.set_hide_tests(!hide, cx))), + ) + }) + .when(files > 0, |d| { + let open = code.file_tree; + d.child( + toggle( + "pr-code-file-tree", + "icons/folder_tree.svg", + open, + match open { + true => "Hide file tree".into(), + false => "Show file tree".into(), + }, + ) + .on_click(cx.listener(move |this, _, _, cx| this.set_file_tree(!open, cx))), ) }); + let width = self.toolbar_width.clone(); + let measure = canvas( + move |bounds, _, _| width.set(bounds.size.width), + |_, _, _, _| {}, + ) + .absolute() + .size_full(); + div() + .relative() .flex_none() .h(px(40.)) .px_4() @@ -163,6 +303,7 @@ impl CodeTab { .border_color(border) .text_xs() .text_color(muted) + .child(measure) .child(left) .child(right) .into_any_element() diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tree.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tree.rs new file mode 100644 index 00000000..2788f84e --- /dev/null +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/code/tree.rs @@ -0,0 +1,202 @@ +//! The Code tab's file tree as rows, after T3 Code's `DiffFileTree` +//! (`@pierre/trees` with `flattenEmptyDirectories`): folders in the order +//! the list shows their files, a folder holding just one folder drawn as +//! one row (`src/main`), collapsed folders hiding what is below. + +use std::collections::HashSet; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum TreeRow { + Folder { + /// The folder's path, which keys its collapsed state. + path: String, + /// Its name, or the names of the folders drawn as one row. + name: String, + depth: usize, + collapsed: bool, + }, + File { + /// The shown file's index. + file: usize, + name: String, + depth: usize, + }, +} + +#[derive(Default)] +struct Node { + name: String, + folders: Vec, + files: Vec<(usize, String)>, + /// Folders and files in the order they first appear. + order: Vec, +} + +#[derive(Clone, Copy)] +enum Entry { + Folder(usize), + File(usize), +} + +impl Node { + fn insert(&mut self, parts: &[&str], file: usize) { + match parts { + [] => {} + [name] => { + self.order.push(Entry::File(self.files.len())); + self.files.push((file, (*name).to_owned())); + } + [folder, rest @ ..] => { + let ix = match self.folders.iter().position(|f| f.name == *folder) { + Some(ix) => ix, + None => { + self.order.push(Entry::Folder(self.folders.len())); + self.folders.push(Node { + name: (*folder).to_owned(), + ..Node::default() + }); + self.folders.len() - 1 + } + }; + self.folders[ix].insert(rest, file); + } + } + } + + fn flatten( + &self, + parent: &str, + depth: usize, + collapsed: &HashSet, + out: &mut Vec, + ) { + for entry in &self.order { + match *entry { + Entry::File(ix) => { + let (file, name) = &self.files[ix]; + out.push(TreeRow::File { + file: *file, + name: name.clone(), + depth, + }); + } + Entry::Folder(ix) => { + let mut folder = &self.folders[ix]; + let mut name = folder.name.clone(); + while folder.files.is_empty() && folder.folders.len() == 1 { + folder = &folder.folders[0]; + name = format!("{name}/{}", folder.name); + } + let path = match parent { + "" => name.clone(), + parent => format!("{parent}/{name}"), + }; + let is_collapsed = collapsed.contains(&path); + out.push(TreeRow::Folder { + path: path.clone(), + name, + depth, + collapsed: is_collapsed, + }); + if !is_collapsed { + folder.flatten(&path, depth + 1, collapsed, out); + } + } + } + } + } +} + +/// The rows for the shown files' `paths` (index = shown file). +pub fn tree_rows(paths: &[&str], collapsed: &HashSet) -> Vec { + let mut root = Node::default(); + for (file, path) in paths.iter().enumerate() { + let parts: Vec<&str> = path.split('/').filter(|p| !p.is_empty()).collect(); + root.insert(&parts, file); + } + let mut rows = Vec::new(); + root.flatten("", 0, collapsed, &mut rows); + rows +} + +/// Every folder path of the rows (with all expanded), for collapsing all. +pub fn folder_paths(paths: &[&str]) -> HashSet { + tree_rows(paths, &HashSet::new()) + .into_iter() + .filter_map(|row| match row { + TreeRow::Folder { path, .. } => Some(path), + TreeRow::File { .. } => None, + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn shape(rows: &[TreeRow]) -> Vec { + rows.iter() + .map(|row| match row { + TreeRow::Folder { + name, + depth, + collapsed, + .. + } => format!( + "{}{name}/{}", + " ".repeat(*depth), + if *collapsed { " +" } else { "" } + ), + TreeRow::File { file, name, depth } => { + format!("{}{name} #{file}", " ".repeat(*depth)) + } + }) + .collect() + } + + const PATHS: [&str; 4] = [ + "crates/github/src/ops/workflows.rs", + "crates/github/src/ops/mod.rs", + "crates/github/tests/operations.rs", + "README.md", + ]; + + #[test] + fn folders_holding_one_folder_are_one_row() { + assert_eq!( + shape(&tree_rows(&PATHS, &HashSet::new())), + [ + "crates/github/", + " src/ops/", + " workflows.rs #0", + " mod.rs #1", + " tests/", + " operations.rs #2", + "README.md #3", + ] + ); + } + + #[test] + fn a_collapsed_folder_hides_what_is_below() { + let collapsed = HashSet::from(["crates/github/src/ops".to_owned()]); + assert_eq!( + shape(&tree_rows(&PATHS, &collapsed)), + [ + "crates/github/", + " src/ops/ +", + " tests/", + " operations.rs #2", + "README.md #3", + ] + ); + assert_eq!( + folder_paths(&PATHS), + HashSet::from([ + "crates/github".to_owned(), + "crates/github/src/ops".to_owned(), + "crates/github/tests".to_owned(), + ]) + ); + } +} diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/conversation/fixtures.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/conversation/fixtures.rs index 358a0d9d..56a20c03 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/conversation/fixtures.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/conversation/fixtures.rs @@ -112,6 +112,7 @@ pub fn window( ) -> (Entity, &mut VisualTestContext) { let window = cx.update(|cx| { gpui_kit::component::init(cx); + crate::shared::file_icons::init(cx); cx.open_window(Default::default(), |_, cx| cx.new(build)) .unwrap() }); 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 eda1b1c3..43d33f10 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,5 +1,6 @@ //! What the pull request view remembers across restarts: how the list is -//! sorted and filtered, and the merge method last used per repository. +//! sorted and filtered, the merge method last used per repository, and how +//! the Code tab shows diffs. //! Kept with the other UI settings ([`UiSettings`](crate::shared::settings::UiSettings)). use super::list_logic::{Filters, SortMode}; @@ -18,6 +19,42 @@ pub struct PullRequestPreferences { pub merge_methods: BTreeMap, /// The Summary's comment filter and order, the Timeline's order. pub conversation: super::conversation::preferences::ConversationPreferences, + pub code: CodePreferences, +} + +/// Stacked rows (old and new lines one below the other) or split (side +/// by side). +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum DiffLayout { + #[default] + Stacked, + Split, +} + +/// How the Code tab shows diffs. T3 Code takes the defaults from its +/// settings (lines wrap, whitespace changes hidden); here the toggles +/// themselves are remembered. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(default)] +pub struct CodePreferences { + pub layout: DiffLayout, + pub wrap: bool, + pub hide_whitespace: bool, + pub hide_tests: bool, + pub file_tree: bool, +} + +impl Default for CodePreferences { + fn default() -> Self { + Self { + layout: DiffLayout::Stacked, + wrap: true, + hide_whitespace: true, + hide_tests: false, + file_tree: false, + } + } } #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] @@ -59,6 +96,8 @@ mod tests { preferences.list.sort = SortMode::Newest; preferences.list.state = ListState::Merged; preferences.list.filters.draft = Some(DraftFilter::Hide); + preferences.code.layout = DiffLayout::Split; + preferences.code.wrap = false; preferences .merge_methods .insert(repo().to_string(), MergeMethod::Squash); From 546b8e2a04bc0e4fb737e623f159a76837b07b08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Fri, 30 Oct 2026 14:09:16 +0100 Subject: [PATCH 4/9] Check out a pull request here or in a worktree of its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Check out ▾" now checks out, as gh pr checkout does (git::pull_request): the head branch fetched from the base repository's remote, a fork's head as refs/pull/N/head without adding a remote, tracking the fork's branch when maintainers may push (else the pull request's ref); a fork's branch named like the default branch becomes owner/branch; an existing branch only fast-forwards. - In a separate worktree: the branch's worktree is reused, else one is made beside the project; a new session for the project works in it, knows the pull request as its own and is shown at once. - In this repository: tracked changes refuse it (as in T3 Code); the session follows its new branch. The page says where it landed in T3's words, or warns when the checkout is not on the pull request's head. Gpui::open_session replaces three copies of the make-current-and-load sequence. --- .../src/pull_requests/checkout.rs | 136 +++++ .../src/pull_requests/mod.rs | 4 + .../src/session/pull_request.rs | 43 +- .../src/session/service/pull_requests.rs | 205 ++++++- crates/git/src/lib.rs | 1 + crates/git/src/pull_request.rs | 508 ++++++++++++++++++ crates/ui_gpui/src/app/commands.rs | 17 +- crates/ui_gpui/src/app/event_bridge.rs | 4 +- .../right_panel/pull_request/actions.rs | 71 ++- .../right_panel/pull_request/data.rs | 17 + .../right_panel/pull_request/detail.rs | 146 ++++- .../right_panel/pull_request/header.rs | 14 +- .../right_panel/pull_request/mod.rs | 41 ++ 13 files changed, 1180 insertions(+), 27 deletions(-) create mode 100644 crates/code_assistant_core/src/pull_requests/checkout.rs create mode 100644 crates/git/src/pull_request.rs diff --git a/crates/code_assistant_core/src/pull_requests/checkout.rs b/crates/code_assistant_core/src/pull_requests/checkout.rs new file mode 100644 index 00000000..0d741ce9 --- /dev/null +++ b/crates/code_assistant_core/src/pull_requests/checkout.rs @@ -0,0 +1,136 @@ +//! Checking out a pull request: its head as git sees it, from what GitHub +//! reports (see [`git::pull_request`] for the checkout itself). + +use git::pull_request::{ForkHead, PullRequestHead}; +use github::RepoRef; +use github::model::Detail; +use std::path::PathBuf; + +/// Where a pull request is checked out. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CheckoutPlace { + /// In the session's repository: switches the branch it works on. + Here, + /// In a worktree of its own, with a session of its own. + Worktree, +} + +/// A checked-out pull request: the session working on it (a new one for a +/// worktree) and where. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct PullRequestCheckout { + pub session_id: String, + pub path: PathBuf, + pub branch: String, + /// At the pull request's head (local commits keep a branch elsewhere). + pub on_head: bool, +} + +/// The head of `detail` for a checkout in `repo`, whose remotes are +/// `(name, url)`: fetched through the remote of the pull request's +/// repository (else `origin`); a fork pushed to over the protocol that +/// remote uses, when its owner lets maintainers push. +pub fn pull_request_head( + detail: &Detail, + base: &RepoRef, + remotes: &[(String, String)], +) -> PullRequestHead { + let remote = remotes + .iter() + .find(|(_, url)| RepoRef::from_remote_url(url).as_ref() == Some(base)) + .or_else(|| remotes.iter().find(|(name, _)| name == "origin")); + let ssh = remote.is_some_and(|(_, url)| url.starts_with("git@") || url.starts_with("ssh://")); + let fork = detail.is_cross_repository.then(|| ForkHead { + owner: detail.head_repository_owner.clone().unwrap_or_default(), + push_url: detail + .head_repository + .as_ref() + .filter(|_| detail.maintainer_can_modify) + .map(|name| match ssh { + true => format!("git@{}:{name}.git", base.host), + false => format!("https://{}/{name}.git", base.host), + }), + }); + PullRequestHead { + number: detail.number, + branch: detail.head_branch.clone(), + sha: detail.head_sha.clone(), + base_branch: detail.base_branch.clone(), + remote: remote.map_or_else(|| "origin".to_owned(), |(name, _)| name.clone()), + fork, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn detail(fork: bool, maintainers: bool) -> Detail { + let mut detail: Detail = serde_json::from_value(serde_json::json!({ + "number": 8, "title": "t", "body": "", "url": "u", "author": null, + "state": "open", "is_draft": false, "mergeability": "mergeable", + "review_decision": null, "additions": 0, "deletions": 0, "changed_files": 0, + "head_branch": "topic", "head_sha": "abc1234", "head_repository": "contributor/r", + "head_repository_owner": "contributor", "is_cross_repository": false, + "base_branch": "main", "created_at": "2026-10-30T10:00:00Z", + "updated_at": "2026-10-30T10:00:00Z", "merged_at": null, "closed_at": null, + "reviewers": [], "labels": [], "checks": [], "checks_state": null, + "checks_truncated": false, + "merge_capabilities": {"merge": true, "squash": true, "rebase": true}, + "auto_merge_allowed": false, + "viewer_permissions": {"actions": [], "comment": true, "resolve": true, + "verdicts": [], "request_reviewers": true, "update_methods": [], + "labels": true, "maintainer_edits": false}, + "base_comparison": "up-to-date", "behind_by": null, + "auto_merge_enabled": false, "auto_merge_method": null, + "maintainer_can_modify": false + })) + .unwrap(); + detail.is_cross_repository = fork; + detail.maintainer_can_modify = maintainers; + detail + } + + fn remotes(urls: &[(&str, &str)]) -> Vec<(String, String)> { + urls.iter() + .map(|(name, url)| (name.to_string(), url.to_string())) + .collect() + } + + #[test] + fn fetches_through_the_remote_of_the_pull_requests_repository() { + let base = RepoRef::new("github.com", "stippi", "r"); + let head = pull_request_head( + &detail(false, false), + &base, + &remotes(&[ + ("origin", "git@github.com:me/r.git"), + ("upstream", "https://github.com/stippi/r.git"), + ]), + ); + assert_eq!(head.remote, "upstream"); + assert!(head.fork.is_none()); + assert_eq!((head.branch.as_str(), head.number), ("topic", 8)); + } + + #[test] + fn a_fork_is_pushed_to_only_when_maintainers_may() { + let base = RepoRef::new("github.com", "stippi", "r"); + let ssh = remotes(&[("origin", "git@github.com:stippi/r.git")]); + let head = pull_request_head(&detail(true, true), &base, &ssh); + let fork = head.fork.unwrap(); + assert_eq!(fork.owner, "contributor"); + assert_eq!( + fork.push_url.as_deref(), + Some("git@github.com:contributor/r.git") + ); + let head = pull_request_head(&detail(true, false), &base, &ssh); + assert_eq!(head.fork.unwrap().push_url, None); + let https = remotes(&[("origin", "https://github.com/stippi/r.git")]); + let head = pull_request_head(&detail(true, true), &base, &https); + assert_eq!( + head.fork.unwrap().push_url.as_deref(), + Some("https://github.com/contributor/r.git") + ); + } +} diff --git a/crates/code_assistant_core/src/pull_requests/mod.rs b/crates/code_assistant_core/src/pull_requests/mod.rs index 4fc80ed4..d8f53bb0 100644 --- a/crates/code_assistant_core/src/pull_requests/mod.rs +++ b/crates/code_assistant_core/src/pull_requests/mod.rs @@ -14,14 +14,18 @@ //! [`UiEvent::PullRequestListChanged`]: crate::ui::UiEvent::PullRequestListChanged mod cache; +mod checkout; mod repo; mod snapshots; mod store; #[cfg(test)] mod tests; +pub use checkout::{CheckoutPlace, PullRequestCheckout}; pub use store::{PullRequestSource, PullRequests}; +pub(crate) use checkout::pull_request_head; + pub(crate) use repo::{github_remote, github_repo, own_pull_request}; use github::RepoRef; diff --git a/crates/code_assistant_core/src/session/pull_request.rs b/crates/code_assistant_core/src/session/pull_request.rs index 20e654cd..0f77fdfc 100644 --- a/crates/code_assistant_core/src/session/pull_request.rs +++ b/crates/code_assistant_core/src/session/pull_request.rs @@ -54,23 +54,52 @@ pub enum ChecksState { impl PullRequestSnapshot { pub fn from_summary(summary: &Summary, fetched_at: SystemTime) -> Self { - let state = match summary.state { - github::model::PullRequestState::Open if summary.is_draft => PullRequestState::Draft, + Self::from_parts( + (summary.number, &summary.url, &summary.title), + summary.state, + summary.is_draft, + summary.review_decision, + summary.checks_state, + fetched_at, + ) + } + + pub fn from_detail(detail: &github::model::Detail, fetched_at: SystemTime) -> Self { + Self::from_parts( + (detail.number, &detail.url, &detail.title), + detail.state, + detail.is_draft, + detail.review_decision, + detail.checks_state, + fetched_at, + ) + } + + fn from_parts( + (number, url, title): (u64, &str, &str), + state: github::model::PullRequestState, + is_draft: bool, + review: Option, + checks: Option, + fetched_at: SystemTime, + ) -> Self { + let state = match state { + github::model::PullRequestState::Open if is_draft => PullRequestState::Draft, github::model::PullRequestState::Open => PullRequestState::Open, github::model::PullRequestState::Merged => PullRequestState::Merged, github::model::PullRequestState::Closed => PullRequestState::Closed, }; Self { - number: summary.number, - url: summary.url.clone(), - title: summary.title.clone(), + number, + url: url.to_owned(), + title: title.to_owned(), state, - review: summary.review_decision.map(|review| match review { + review: review.map(|review| match review { github::model::ReviewDecision::Approved => ReviewDecision::Approved, github::model::ReviewDecision::ChangesRequested => ReviewDecision::ChangesRequested, github::model::ReviewDecision::ReviewRequired => ReviewDecision::ReviewRequired, }), - checks: summary.checks_state.map(|checks| match checks { + checks: checks.map(|checks| match checks { github::model::ChecksState::Passing => ChecksState::Passing, github::model::ChecksState::Failing => ChecksState::Failing, github::model::ChecksState::Pending => ChecksState::Pending, 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 7841bc0b..d817c667 100644 --- a/crates/code_assistant_core/src/session/service/pull_requests.rs +++ b/crates/code_assistant_core/src/session/service/pull_requests.rs @@ -4,13 +4,17 @@ //! session lanes: a slow host never holds up session commands. use super::*; -use crate::pull_requests::{Fetched, SessionPullRequests}; +use crate::pull_requests::{ + CheckoutPlace, Fetched, PullRequestCheckout, SessionPullRequests, pull_request_head, +}; +use crate::session::pull_request::PullRequestSnapshot; use github::model::{ Action, Activity, Checks, Detail, DiffFileContents, DiffPage, DiffStat, EditableCommentKind, FileChange, FilesViewed, LabelCandidates, ListPage, ListQuery, PullRequestUpdate, ReactionContent, ReviewSubmission, Reviewer, ReviewerCandidates, ThreadCommentsPage, Viewer, }; use github::{PullRequestRef, RepoRef}; +use std::time::SystemTime; impl SessionService { /// The GitHub repository of the session's project, its branch and that @@ -44,6 +48,93 @@ 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. + pub async fn pull_request_checkout( + &self, + session_id: String, + pr: PullRequestRef, + place: CheckoutPlace, + ) -> Result { + self.call_io(move |ctx| async move { + let (project_root, here, project, worktree) = { + let manager = ctx.manager.lock().await; + let session = manager + .get_session(&session_id) + .ok_or_else(|| anyhow!("Session {session_id} not found"))?; + let config = &session.session.config; + ( + session_project_root(&manager, &session_id)?, + session_effective_path(&manager, &session_id)?, + config.initial_project.clone(), + config.worktree_path.clone(), + ) + }; + let detail = ctx.pull_requests.detail(pr.clone()).await?.value; + let path = match place { + CheckoutPlace::Here => here, + // Worktrees hang off the repository itself. + CheckoutPlace::Worktree => project_root.clone(), + }; + let repo = git::GitRepository::open(&path).context("Failed to open git repository")?; + let remotes: Vec<(String, String)> = repo + .remotes() + .into_iter() + .map(|remote| (remote.name, remote.url)) + .collect(); + let head = pull_request_head(&detail, &pr.repo, &remotes); + let done = match place { + CheckoutPlace::Here => git::pull_request::checkout_here(&repo, &head).await, + CheckoutPlace::Worktree => { + git::pull_request::checkout_in_worktree(&repo, &head).await + } + } + .with_context(|| format!("Could not check out pull request #{}", pr.number))?; + + let mut manager = ctx.manager.lock().await; + let session_id = match place { + CheckoutPlace::Here => { + manager.set_session_worktree( + &session_id, + worktree, + Some(done.branch.clone()), + )?; + session_id + } + CheckoutPlace::Worktree => { + let mut config = manager.new_session_config(Some(project_root)); + config.initial_project = project; + let model = SessionModelConfig::new(manager.default_model_name()); + let name = format!("#{} {}", detail.number, detail.title); + let id = manager.create_session_with_config( + Some(name), + Some(config), + Some(model), + )?; + manager.set_session_worktree( + &id, + Some(done.path.clone()), + Some(done.branch.clone()), + )?; + id + } + }; + let snapshot = PullRequestSnapshot::from_detail(&detail, SystemTime::now()); + manager.update_session_lifecycle(&session_id, |lifecycle| { + lifecycle.pull_request = Some(snapshot) + })?; + Ok(PullRequestCheckout { + session_id, + path: done.path, + branch: done.branch, + on_head: done.on_head, + }) + }) + .await + } + // Reads: the held value at once (memory, else disk snapshot) and a // background refresh when it is past its time to live; with nothing // held, they wait for GitHub. @@ -389,6 +480,118 @@ mod tests { assert_eq!(service.session_pull_requests(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") + .args([ + "-c", + "user.email=t@t", + "-c", + "user.name=t", + "-c", + "commit.gpgsign=false", + ]) + .args(args) + .current_dir(dir) + .output() + .unwrap(); + assert!(output.status.success(), "git {args:?} failed"); + 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"); + std::fs::create_dir(&upstream).unwrap(); + git_out(&upstream, &["init", "-q", "-b", "main"]); + git_out(&upstream, &["commit", "-q", "--allow-empty", "-m", "init"]); + git_out( + &upstream, + &["commit", "-q", "--allow-empty", "-m", "fork work"], + ); + 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(), + &[ + "clone", + "-q", + upstream.to_str().unwrap(), + project.to_str().unwrap(), + ], + ); + + 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(core); + let source = PullRequestSource { + client: fake.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()), + initial_project: "code-assistant".into(), + ..crate::session::SessionConfig::default() + }; + let id = service + .create_session_with_config(None, config, None) + .await + .unwrap(); + let pr = RepoRef::new("github.com", "stippi", "code-assistant").pull_request(291); + + let done = service + .pull_request_checkout(id.clone(), pr, CheckoutPlace::Worktree) + .await + .unwrap(); + assert_ne!(done.session_id, id); + assert_eq!(done.branch, "feature/pr-view"); + assert!(done.on_head); + assert_eq!(git_out(&done.path, &["rev-parse", "HEAD"]), head); + // The project itself stays where it was. + assert_eq!(git_out(&project, &["branch", "--show-current"]), "main"); + // 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" + ); + + let manager = manager.lock().await; + let session = manager.get_session(&done.session_id).unwrap(); + assert_eq!( + session.session.config.worktree_path.as_ref(), + Some(&done.path) + ); + assert_eq!( + session.session.config.branch.as_deref(), + Some("feature/pr-view") + ); + assert_eq!(session.session.config.initial_project, "code-assistant"); + let lifecycle = manager.session_lifecycles().unwrap(); + assert_eq!( + lifecycle[&done.session_id] + .pull_request + .as_ref() + .map(|pr| pr.number), + Some(291) + ); + } + #[tokio::test] async fn reads_and_writes_reach_github_through_the_service() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/git/src/lib.rs b/crates/git/src/lib.rs index dfb17e61..c857ba8b 100644 --- a/crates/git/src/lib.rs +++ b/crates/git/src/lib.rs @@ -3,6 +3,7 @@ mod branch; mod diff; mod merged; mod patch; +pub mod pull_request; mod repository; mod types; mod watch; diff --git a/crates/git/src/pull_request.rs b/crates/git/src/pull_request.rs new file mode 100644 index 00000000..8c383706 --- /dev/null +++ b/crates/git/src/pull_request.rs @@ -0,0 +1,508 @@ +//! Checking out a pull request's head the way `gh pr checkout` does, here +//! or in a worktree of its own. +//! +//! - **Branch**: named like the head branch; a fork's head named like the +//! base repository's default (or base) branch becomes `owner/branch`. +//! - **Fetch**: a branch of the repository itself from its remote +//! (`+refs/heads/b:refs/remotes/origin/b`); a fork's head as +//! `refs/pull/N/head` from the base repository, no remote added. +//! - **Upstream**: the remote's branch; for a fork, its URL and branch +//! when maintainers may push to it, else the base's `refs/pull/N/head`. +//! - **Updating**: an existing branch only fast-forwards; local commits +//! stay (then the checkout is not on the pull request's head). +//! +//! Here, tracked changes refuse the switch (as in T3 Code). In a worktree, +//! a branch already checked out in another worktree is reused there; one +//! checked out in the repository itself is refused. + +use crate::GitRepository; +use crate::worktree::{list_worktrees, suggest_worktree_path}; +use anyhow::{Context, Result, bail}; +use std::path::{Path, PathBuf}; + +/// The head of a pull request, as GitHub reports it. +#[derive(Clone, Debug)] +pub struct PullRequestHead { + pub number: u64, + /// The branch in the head repository. + pub branch: String, + pub sha: Option, + /// The pull request's base branch. + pub base_branch: String, + /// The base repository's remote here (e.g. `origin`). + pub remote: String, + /// Set when the head lives in a fork. + pub fork: Option, +} + +#[derive(Clone, Debug)] +pub struct ForkHead { + pub owner: String, + /// The fork's clone URL, when its owner lets maintainers push. + pub push_url: Option, +} + +/// Where the pull request was checked out. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct CheckedOut { + pub path: PathBuf, + pub branch: String, + /// The checkout is at the pull request's head (local commits or a + /// diverged branch keep it elsewhere). + pub on_head: bool, +} + +/// The local branch for `head`, as `gh pr checkout` names it. +pub fn local_branch(head: &PullRequestHead, default_branch: Option<&str>) -> String { + match &head.fork { + Some(fork) + if head.branch == head.base_branch || Some(head.branch.as_str()) == default_branch => + { + format!("{}/{}", fork.owner, head.branch) + } + _ => head.branch.clone(), + } +} + +/// What to bring the branch to, and what it tracks. +struct Fetched { + target: String, + upstream: Option<(String, String)>, +} + +async fn git(repo: &GitRepository, dir: &Path, args: &[&str]) -> Result { + repo.git.run(dir, args).await +} + +async fn has_branch(repo: &GitRepository, branch: &str) -> bool { + let full = format!("refs/heads/{branch}"); + git( + repo, + repo.workdir(), + &["rev-parse", "--verify", "--quiet", &full], + ) + .await + .is_ok() +} + +/// The remote's default branch (`origin/HEAD`), when known. +async fn default_branch(repo: &GitRepository, remote: &str) -> Option { + let head = format!("refs/remotes/{remote}/HEAD"); + let name = git( + repo, + repo.workdir(), + &["symbolic-ref", "--quiet", "--short", &head], + ) + .await + .ok()?; + name.strip_prefix(&format!("{remote}/")).map(str::to_owned) +} + +/// Fetch the head: a branch of the repository itself as its remote +/// branch (the pull request's ref when that fails), a fork's head as the +/// pull request's ref. +async fn fetch(repo: &GitRepository, head: &PullRequestHead) -> Result { + let dir = repo.workdir(); + let pull_ref = format!("refs/pull/{}/head", head.number); + if head.fork.is_none() { + let refspec = format!( + "+refs/heads/{0}:refs/remotes/{1}/{0}", + head.branch, head.remote + ); + let fetched = git( + repo, + dir, + &[ + "fetch", + "--quiet", + "--no-tags", + "--", + &head.remote, + &refspec, + ], + ) + .await; + if fetched.is_ok() { + return Ok(Fetched { + target: format!("refs/remotes/{}/{}", head.remote, head.branch), + upstream: Some((head.remote.clone(), format!("refs/heads/{}", head.branch))), + }); + } + } + git( + repo, + dir, + &[ + "fetch", + "--quiet", + "--no-tags", + "--", + &head.remote, + &pull_ref, + ], + ) + .await + .with_context(|| format!("Could not fetch pull request #{}", head.number))?; + let target = git(repo, dir, &["rev-parse", "FETCH_HEAD"]).await?; + let upstream = head.fork.as_ref().map(|fork| match &fork.push_url { + Some(url) => (url.clone(), format!("refs/heads/{}", head.branch)), + None => (head.remote.clone(), pull_ref.clone()), + }); + Ok(Fetched { target, upstream }) +} + +/// Track the upstream like `gh` does: by remote name or URL. +async fn set_upstream(repo: &GitRepository, branch: &str, fetched: &Fetched) -> Result<()> { + let Some((remote, merge)) = &fetched.upstream else { + return Ok(()); + }; + let dir = repo.workdir(); + git( + repo, + dir, + &["config", &format!("branch.{branch}.remote"), remote], + ) + .await?; + git( + repo, + dir, + &["config", &format!("branch.{branch}.merge"), merge], + ) + .await?; + Ok(()) +} + +async fn at(repo: &GitRepository, dir: &Path, sha: Option<&str>) -> bool { + let Some(sha) = sha else { + return false; + }; + git(repo, dir, &["rev-parse", "HEAD"]) + .await + .is_ok_and(|head| head.eq_ignore_ascii_case(sha)) +} + +async fn is_ancestor(repo: &GitRepository, ancestor: &str, of: &str) -> bool { + git( + repo, + repo.workdir(), + &["merge-base", "--is-ancestor", ancestor, of], + ) + .await + .is_ok() +} + +/// Check the pull request out in the repository itself. Tracked changes +/// refuse it. +pub async fn checkout_here(repo: &GitRepository, head: &PullRequestHead) -> Result { + let dir = repo.workdir(); + let changes = git( + repo, + dir, + &["status", "--porcelain", "--untracked-files=no"], + ) + .await?; + if !changes.trim().is_empty() { + bail!( + "This repository has uncommitted changes. Commit or stash them before checking out \ + pull request #{} here, or check it out in its own worktree.", + head.number + ); + } + let default = default_branch(repo, &head.remote).await; + let branch = local_branch(head, default.as_deref()); + let fetched = fetch(repo, head).await?; + if has_branch(repo, &branch).await { + git(repo, dir, &["checkout", "--quiet", &branch, "--"]).await?; + git( + repo, + dir, + &["merge", "--ff-only", "--quiet", &fetched.target], + ) + .await + .with_context(|| { + format!("The branch {branch} has commits the pull request does not have") + })?; + } else { + git( + repo, + dir, + &["checkout", "--quiet", "-b", &branch, &fetched.target, "--"], + ) + .await?; + } + set_upstream(repo, &branch, &fetched).await?; + Ok(CheckedOut { + path: dir.to_path_buf(), + on_head: at(repo, dir, head.sha.as_deref()).await, + branch, + }) +} + +/// Check the pull request out in a worktree of its own: the one that has +/// its branch already, else a new one beside the repository. +pub async fn checkout_in_worktree( + repo: &GitRepository, + head: &PullRequestHead, +) -> Result { + let default = default_branch(repo, &head.remote).await; + let branch = local_branch(head, default.as_deref()); + let full = format!("refs/heads/{branch}"); + let holder = list_worktrees(repo) + .await? + .into_iter() + .find(|worktree| worktree.branch.as_deref() == Some(full.as_str())); + if holder.as_ref().is_some_and(|worktree| worktree.is_main) { + bail!( + "The branch {branch} is checked out in the repository itself. Check the pull \ + request out there, or switch the repository to another branch first." + ); + } + let fetched = fetch(repo, head).await?; + let path = match holder { + Some(worktree) => { + // Moved on only when nothing there would be lost. + let clean = git(repo, &worktree.path, &["status", "--porcelain"]) + .await + .is_ok_and(|status| status.trim().is_empty()); + if clean && is_ancestor(repo, &full, &fetched.target).await { + git( + repo, + &worktree.path, + &["merge", "--ff-only", "--quiet", &fetched.target], + ) + .await?; + } + worktree.path + } + None => { + if !has_branch(repo, &branch).await { + git( + repo, + repo.workdir(), + &["branch", "--", &branch, &fetched.target], + ) + .await?; + } else if is_ancestor(repo, &full, &fetched.target).await { + git( + repo, + repo.workdir(), + &["branch", "--force", "--", &branch, &fetched.target], + ) + .await?; + } + let path = free_path(suggest_worktree_path(repo.workdir(), &branch)); + crate::worktree::create_worktree_for_branch(&repo.git, repo.workdir(), &path, &branch) + .await + .context("Failed to create the worktree")? + } + }; + set_upstream(repo, &branch, &fetched).await?; + Ok(CheckedOut { + on_head: at(repo, &path, head.sha.as_deref()).await, + path, + branch, + }) +} + +/// `path`, or the first of `path-2`, `path-3`, … that does not exist. +fn free_path(path: PathBuf) -> PathBuf { + let mut candidate = path.clone(); + let mut n = 2; + while candidate.exists() { + let mut name = path.file_name().unwrap_or_default().to_os_string(); + name.push(format!("-{n}")); + candidate = path.with_file_name(name); + n += 1; + } + candidate +} + +#[cfg(test)] +mod tests { + use super::*; + use std::process::Command; + + fn run(dir: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .args([ + "-c", + "user.email=t@t", + "-c", + "user.name=T", + "-c", + "commit.gpgsign=false", + ]) + .args(args) + .current_dir(dir) + .output() + .unwrap(); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout).unwrap().trim().to_owned() + } + + fn commit(dir: &Path, file: &str) -> String { + std::fs::write(dir.join(file), file).unwrap(); + run(dir, &["add", file]); + run(dir, &["commit", "-q", "-m", file]); + run(dir, &["rev-parse", "HEAD"]) + } + + /// A "GitHub" repository with `main`, a branch `feature` (#7) and a + /// fork's head at `refs/pull/8/head`, and a clone of it. + struct Setup { + _dir: tempfile::TempDir, + clone: PathBuf, + feature: String, + fork: String, + } + + fn setup() -> Setup { + let dir = tempfile::TempDir::new().unwrap(); + let upstream = dir.path().join("upstream"); + std::fs::create_dir(&upstream).unwrap(); + run(&upstream, &["init", "-q", "-b", "main"]); + commit(&upstream, "a"); + run(&upstream, &["checkout", "-q", "-b", "feature"]); + let feature = commit(&upstream, "b"); + run(&upstream, &["checkout", "-q", "main"]); + run(&upstream, &["checkout", "-q", "-b", "fork-work"]); + let fork = commit(&upstream, "c"); + run(&upstream, &["update-ref", "refs/pull/8/head", &fork]); + run(&upstream, &["checkout", "-q", "main"]); + run(&upstream, &["branch", "-q", "-D", "fork-work"]); + let clone = dir.path().join("clone"); + run( + dir.path(), + &[ + "clone", + "-q", + upstream.to_str().unwrap(), + clone.to_str().unwrap(), + ], + ); + Setup { + _dir: dir, + clone, + feature, + fork, + } + } + + fn own_head(sha: &str) -> PullRequestHead { + PullRequestHead { + number: 7, + branch: "feature".into(), + sha: Some(sha.into()), + base_branch: "main".into(), + remote: "origin".into(), + fork: None, + } + } + + fn fork_head(sha: &str, branch: &str, push_url: Option<&str>) -> PullRequestHead { + PullRequestHead { + number: 8, + branch: branch.into(), + sha: Some(sha.into()), + base_branch: "main".into(), + remote: "origin".into(), + fork: Some(ForkHead { + owner: "contributor".into(), + push_url: push_url.map(str::to_owned), + }), + } + } + + #[tokio::test] + async fn checks_out_a_branch_of_the_repository_here_tracking_its_remote() { + let setup = setup(); + let repo = GitRepository::open(&setup.clone).unwrap(); + let done = checkout_here(&repo, &own_head(&setup.feature)) + .await + .unwrap(); + assert_eq!(done.branch, "feature"); + assert!(done.on_head); + assert_eq!( + run(&setup.clone, &["rev-parse", "--abbrev-ref", "@{u}"]), + "origin/feature" + ); + } + + #[tokio::test] + async fn checks_out_a_forks_head_from_the_pull_request_ref() { + let setup = setup(); + let repo = GitRepository::open(&setup.clone).unwrap(); + // The fork's branch is named like the base's default branch. + let head = fork_head( + &setup.fork, + "main", + Some("https://github.com/contributor/r.git"), + ); + let done = checkout_here(&repo, &head).await.unwrap(); + assert_eq!(done.branch, "contributor/main"); + assert!(done.on_head); + let config = |key: &str| run(&setup.clone, &["config", key]); + assert_eq!( + config("branch.contributor/main.remote"), + "https://github.com/contributor/r.git" + ); + assert_eq!(config("branch.contributor/main.merge"), "refs/heads/main"); + // Without maintainer edits it tracks the pull request's ref. + let head = fork_head(&setup.fork, "topic", None); + let done = checkout_here(&repo, &head).await.unwrap(); + assert_eq!(done.branch, "topic"); + assert_eq!(config("branch.topic.merge"), "refs/pull/8/head"); + } + + #[tokio::test] + async fn tracked_changes_refuse_a_checkout_here() { + let setup = setup(); + std::fs::write(setup.clone.join("a"), "changed").unwrap(); + let repo = GitRepository::open(&setup.clone).unwrap(); + let refused = checkout_here(&repo, &own_head(&setup.feature)) + .await + .unwrap_err(); + assert!( + refused.to_string().contains("uncommitted changes"), + "{refused}" + ); + } + + #[tokio::test] + async fn a_worktree_is_made_once_and_reused() { + let setup = setup(); + let repo = GitRepository::open(&setup.clone).unwrap(); + let first = checkout_in_worktree(&repo, &own_head(&setup.feature)) + .await + .unwrap(); + assert!(first.on_head); + assert_ne!(first.path, setup.clone); + assert_eq!(run(&first.path, &["branch", "--show-current"]), "feature"); + let again = checkout_in_worktree(&repo, &own_head(&setup.feature)) + .await + .unwrap(); + assert_eq!(again.path, first.path); + // The repository itself keeps its branch. + assert_eq!(run(&setup.clone, &["branch", "--show-current"]), "main"); + } + + #[tokio::test] + async fn a_branch_checked_out_in_the_repository_is_not_taken_into_a_worktree() { + let setup = setup(); + let repo = GitRepository::open(&setup.clone).unwrap(); + checkout_here(&repo, &own_head(&setup.feature)) + .await + .unwrap(); + let refused = checkout_in_worktree(&repo, &own_head(&setup.feature)) + .await + .unwrap_err(); + assert!( + refused.to_string().contains("repository itself"), + "{refused}" + ); + } +} diff --git a/crates/ui_gpui/src/app/commands.rs b/crates/ui_gpui/src/app/commands.rs index 296a9ada..4ee4654e 100644 --- a/crates/ui_gpui/src/app/commands.rs +++ b/crates/ui_gpui/src/app/commands.rs @@ -130,16 +130,19 @@ impl Gpui { let gpui = self.clone(); self.dispatch(async move { match service.create_session(name, initial_project).await { - Ok(session_id) => { - *gpui.current_session_id.lock().unwrap() = Some(session_id.clone()); - gpui.cmd_refresh_chat_list(); - gpui.cmd_load_session(session_id, None); - } + Ok(session_id) => gpui.open_session(session_id), Err(e) => gpui.display_error(format!("Failed to create session: {e:#}")), } }); } + /// Make `session_id` the current session and show it. + pub(crate) fn open_session(&self, session_id: String) { + *self.current_session_id.lock().unwrap() = Some(session_id.clone()); + self.cmd_refresh_chat_list(); + self.cmd_load_session(session_id, None); + } + /// Connect a session to the UI: apply the returned snapshot and refresh /// the skill catalog for the `/skill` picker. pub(crate) fn cmd_load_session(&self, session_id: String, edited_node_id: Option) { @@ -966,9 +969,7 @@ impl Gpui { match service.add_project(name.clone(), path).await { Ok(AddProjectOutcome::Added { session_id }) => { info!("Project '{name}' added, initial session: {session_id}"); - *gpui.current_session_id.lock().unwrap() = Some(session_id.clone()); - gpui.cmd_refresh_chat_list(); - gpui.cmd_load_session(session_id, None); + gpui.open_session(session_id); } Ok(AddProjectOutcome::AlreadyExists) => { info!("Project '{name}' already exists — nothing to do"); diff --git a/crates/ui_gpui/src/app/event_bridge.rs b/crates/ui_gpui/src/app/event_bridge.rs index 6969dfce..a53a9520 100644 --- a/crates/ui_gpui/src/app/event_bridge.rs +++ b/crates/ui_gpui/src/app/event_bridge.rs @@ -232,9 +232,7 @@ impl Gpui { } UiEvent::SessionHandedOff { to } => { // Follow the work into the new session. - *self.current_session_id.lock().unwrap() = Some(to.clone()); - self.cmd_refresh_chat_list(); - self.cmd_load_session(to.clone(), None); + self.open_session(to.clone()); } _ => {} } diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs index b7d04a2a..600b8390 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/actions.rs @@ -10,13 +10,14 @@ use super::PullRequestIntent; use super::header::DetailHeader; use super::presentation::{Tone, merge_method_label}; +use code_assistant_core::pull_requests::CheckoutPlace; use github::model::{ Action, Checks, ChecksState, Detail, MergeMethod, Mergeability, PullRequestAction, PullRequestState, PullRequestUpdate, UpdateMethod, }; use gpui_kit::component::button::{Button, ButtonVariants}; use gpui_kit::component::menu::{DropdownMenu, PopupMenu, PopupMenuItem}; -use gpui_kit::component::{ActiveTheme, Icon, Side, Sizable}; +use gpui_kit::component::{ActiveTheme, Disableable, Icon, Side, Sizable}; use gpui_kit::{ AnyElement, App, Context, Hsla, SharedString, WeakEntity, div, prelude::*, px, svg, }; @@ -325,18 +326,53 @@ pub fn render_primary( ) } -/// "Check out ▾": for now the command that checks the branch out. -pub fn render_checkout(number: u64, header: WeakEntity) -> AnyElement { +/// "Check out ▾" (T3 Code's `PullRequestDetailActions`): in a worktree of +/// its own with a session on it, or here; or copy the `gh` command. +/// "Checking out..." while one runs. +pub fn render_checkout( + number: u64, + checking_out: bool, + header: WeakEntity, +) -> AnyElement { Button::new("pr-checkout") .outline() .xsmall() .icon(icon("icons/git_branch.svg")) - .label("Check out") + .label(if checking_out { + "Checking out..." + } else { + "Check out" + }) + .disabled(checking_out) + .tooltip("Check out this pull request") .dropdown_caret(true) .dropdown_menu_with_anchor(gpui_kit::Anchor::TopRight, move |menu, _, _| { - let header = header.clone(); let command = checkout_command(number); + let place = |place: CheckoutPlace| { + let header = header.clone(); + move |_: &gpui_kit::ClickEvent, _: &mut gpui_kit::Window, cx: &mut App| { + send(&header, PullRequestIntent::CheckOut(place), cx) + } + }; + let header = header.clone(); menu.item( + described( + "In a separate worktree", + "Its own folder and session. Nothing you have open moves.", + "icons/folder_tree.svg", + ) + .on_click(place(CheckoutPlace::Worktree)), + ) + .item( + described( + "In this repository", + "Switches the branch you are working in.", + "icons/git_branch.svg", + ) + .on_click(place(CheckoutPlace::Here)), + ) + .separator() + .item( PopupMenuItem::new(format!("Copy “{command}”")) .icon(icon("icons/copy.svg")) .on_click(move |_, _, cx| { @@ -347,6 +383,31 @@ pub fn render_checkout(number: u64, header: WeakEntity) -> AnyElem .into_any_element() } +/// A menu entry with a muted line below its label. +fn described(label: &'static str, detail: &'static str, icon_path: &'static str) -> PopupMenuItem { + PopupMenuItem::element(move |_, cx| { + div() + .flex() + .items_start() + .gap_2() + .child( + svg() + .mt_0p5() + .size(px(14.)) + .path(icon_path) + .text_color(cx.theme().muted_foreground), + ) + .child( + div().flex().flex_col().child(label).child( + div() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(detail), + ), + ) + }) +} + pub fn checkout_command(number: u64) -> String { format!("gh pr checkout {number}") } diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs index 5dd2e648..7ab8a3b3 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/data.rs @@ -30,6 +30,14 @@ pub fn session_service(cx: &App) -> Option { .and_then(|gpui| gpui.session_service()) } +/// A word on what an action came to, a warning when it fell short. +#[derive(Clone, Debug, PartialEq)] +pub struct Notice { + pub title: SharedString, + pub detail: SharedString, + pub warning: bool, +} + /// What the detail page knows about one pull request; the tabs observe it. pub struct PullRequestData { pub pr: PullRequestRef, @@ -41,6 +49,8 @@ pub struct PullRequestData { pub refreshing: bool, /// The last failed read or write, shown by the page until the next success. pub error: Option, + /// What an action came to, shown until dismissed ("Checked out"). + pub notice: Option, refreshing_parts: HashSet, reads: HashMap>, viewer_read: Option>, @@ -68,6 +78,7 @@ impl PullRequestData { viewer: None, refreshing: false, error: None, + notice: None, refreshing_parts: HashSet::new(), reads: HashMap::new(), viewer_read: None, @@ -312,6 +323,12 @@ impl PullRequestData { cx.notify(); } } + + pub fn dismiss_notice(&mut self, cx: &mut Context) { + if self.notice.take().is_some() { + cx.notify(); + } + } } #[cfg(test)] diff --git a/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs b/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs index da3eafbe..9130320a 100644 --- a/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs +++ b/crates/ui_gpui/src/main_screen/right_panel/pull_request/detail.rs @@ -8,7 +8,7 @@ use super::PullRequestIntent; use super::actions::{Confirmable, InlineConfirm, workflow_approvals}; use super::code::CodeTab; use super::composer::Composer; -use super::data::PullRequestData; +use super::data::{Notice, PullRequestData}; use super::header::CONFIRM_TIMEOUT; use super::header::DetailHeader; use super::preferences; @@ -16,6 +16,7 @@ use super::summary::SummaryTab; use super::tab_bar::{approve_workflows_slot, checks_slot, render_tab_row, timeline_slot}; use super::timeline::{OpenCommit, TimelineTab}; use anyhow::Context as _; +use code_assistant_core::pull_requests::{CheckoutPlace, PullRequestCheckout}; use github::PullRequestRef; use github::model::{Action, ListEntry, PullRequestUpdate}; use gpui_kit::component::{ActiveTheme, Icon, Sizable, button::Button, button::ButtonVariants}; @@ -283,6 +284,85 @@ impl DetailView { } } + pub fn set_checking_out(&mut self, checking_out: bool, cx: &mut Context) { + self.header + .update(cx, |header, cx| header.set_checking_out(checking_out, cx)); + } + + /// Say what a checkout came to, in T3 Code's words. + pub fn checked_out( + &mut self, + place: CheckoutPlace, + done: &anyhow::Result, + cx: &mut Context, + ) { + self.data.update(cx, |data, cx| { + match done { + Ok(done) => { + data.error = None; + data.notice = Some(checkout_notice(place, done)); + } + Err(error) => { + data.notice = None; + data.error = Some( + format!( + "Could not prepare the pull request checkout: {error:#}. Check that \ + the branch still exists and is not checked out elsewhere, then \ + try again." + ) + .into(), + ); + } + } + cx.notify(); + }); + } + + fn render_notice(&self, notice: Notice, cx: &mut Context) -> AnyElement { + let color = match notice.warning { + true => cx.theme().warning, + false => cx.theme().info, + }; + div() + .debug_selector(|| "pr-notice".into()) + .flex_none() + .px_4() + .py_1p5() + .flex() + .items_center() + .gap_2() + .bg(color.opacity(0.08)) + .border_b_1() + .border_color(color.opacity(0.25)) + .text_xs() + .child( + div() + .flex_1() + .min_w_0() + .child( + div() + .font_weight(gpui_kit::FontWeight::MEDIUM) + .text_color(color) + .child(notice.title), + ) + .child( + div() + .text_color(cx.theme().muted_foreground) + .child(notice.detail), + ), + ) + .child( + Button::new("pr-notice-dismiss") + .ghost() + .xsmall() + .icon(Icon::default().path("icons/close.svg")) + .on_click(cx.listener(|this, _, _, cx| { + this.data.update(cx, |data, cx| data.dismiss_notice(cx)) + })), + ) + .into_any_element() + } + /// The last failure, until it is dismissed or something succeeds. fn render_error(&self, error: SharedString, cx: &mut Context) -> AnyElement { let danger = cx.theme().danger; @@ -313,6 +393,31 @@ impl DetailView { } } +/// T3 Code's toast after a checkout; a warning when it did not land on the +/// pull request's head. +fn checkout_notice(place: CheckoutPlace, done: &PullRequestCheckout) -> Notice { + let (title, detail) = match (done.on_head, place) { + (false, _) => ( + "Checked out, but not confirmed on the latest commit", + "This checkout could not be confirmed at the pull request's latest commit, so its \ + code may differ. Local commits or uncommitted work keep a checkout where it is.", + ), + (true, CheckoutPlace::Here) => ( + "Checked out here", + "This repository is on the pull request's branch.", + ), + (true, CheckoutPlace::Worktree) => ( + "Checked out", + "The pull request is in its own worktree, with a session open on it.", + ), + }; + Notice { + title: title.into(), + detail: format!("{detail} ({}, {})", done.branch, done.path.display()).into(), + warning: !done.on_head, + } +} + /// How far a tab's list is scrolled, and how far it could be. #[derive(Clone, Copy, Debug, PartialEq)] pub struct TabScroll { @@ -366,7 +471,10 @@ impl Render for DetailView { DetailTab::Timeline => self.timeline.clone().into_any_element(), DetailTab::Code => self.code.clone().into_any_element(), }; - let error = self.data.read(cx).error.clone(); + let (error, notice) = { + let data = self.data.read(cx); + (data.error.clone(), data.notice.clone()) + }; let slot = self.tab_slot(cx); div() .relative() @@ -375,6 +483,7 @@ impl Render for DetailView { .flex_col() .child(self.header.clone()) .children(error.map(|error| self.render_error(error, cx))) + .children(notice.map(|notice| self.render_notice(notice, cx))) .child(render_tab_row(self.tab, slot, window, cx)) .child(div().flex_1().min_h_0().child(body)) .child( @@ -512,6 +621,39 @@ mod tests { view.read_with(cx, |view, _| assert!(!view.approving)); } + #[gpui_kit::test] + fn a_checkout_says_where_it_landed(cx: &mut TestAppContext) { + let (view, cx) = page(cx); + let done = PullRequestCheckout { + session_id: "s".into(), + path: "/w/pr".into(), + branch: "feature".into(), + on_head: true, + }; + view.update(cx, |view, cx| { + view.checked_out(CheckoutPlace::Worktree, &Ok(done.clone()), cx) + }); + cx.run_until_parked(); + let notice = view.read_with(cx, |view, cx| view.data.read(cx).notice.clone()); + let notice = notice.expect("a notice"); + assert_eq!(notice.title.as_ref(), "Checked out"); + assert!(!notice.warning); + assert!(cx.debug_bounds("pr-notice").is_some()); + + let behind = PullRequestCheckout { + on_head: false, + ..done + }; + assert!(checkout_notice(CheckoutPlace::Here, &behind).warning); + + view.update(cx, |view, cx| { + view.checked_out(CheckoutPlace::Here, &Err(anyhow::anyhow!("dirty")), cx) + }); + cx.run_until_parked(); + assert!(cx.debug_bounds("pr-error").is_some()); + assert!(cx.debug_bounds("pr-notice").is_none()); + } + #[gpui_kit::test] fn a_refused_title_is_taken_back(cx: &mut TestAppContext) { let (view, cx) = page(cx); 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 bf96b1c7..9cca5780 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 @@ -67,6 +67,8 @@ pub struct DetailHeader { refreshing: bool, /// An action runs. pending: bool, + /// A checkout runs. + checking_out: bool, seed: Option, /// The list is there to go back to. show_back: bool, @@ -103,6 +105,7 @@ impl DetailHeader { checks_state: None, refreshing: false, pending: false, + checking_out: false, seed, show_back, title_editor: None, @@ -156,6 +159,11 @@ impl DetailHeader { self.pending } + pub fn set_checking_out(&mut self, checking_out: bool, cx: &mut Context) { + self.checking_out = checking_out; + cx.notify(); + } + /// While an action runs, the primary control waits. pub fn set_pending(&mut self, pending: bool, cx: &mut Context) { self.pending = pending; @@ -638,7 +646,11 @@ impl Render for DetailHeader { .flex() .items_center() .gap_1() - .child(actions::render_checkout(facts.number, weak.clone())) + .child(actions::render_checkout( + facts.number, + self.checking_out, + weak.clone(), + )) .children(actions::render_primary(self, detail, cx)) .child(actions::render_more_menu( detail, 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 382b1771..25dc173d 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 @@ -33,6 +33,7 @@ mod tab_bar; mod timeline; use bus::{PullRequestEvent, bus}; +use code_assistant_core::pull_requests::{CheckoutPlace, PullRequestCheckout}; use data::session_service; use detail::DetailView; use github::RepoRef; @@ -74,6 +75,8 @@ pub enum PullRequestIntent { Update(PullRequestUpdate), OpenUrl(String), Copy(String), + /// Check the shown pull request out, here or in a worktree. + CheckOut(CheckoutPlace), } pub struct PullRequestPanel { @@ -419,9 +422,47 @@ impl PullRequestPanel { detail.update(cx, |detail, cx| detail.update(update, cx)); } } + PullRequestIntent::CheckOut(place) => self.check_out(*place, cx), } } + /// Check the shown pull request out; a new worktree's session is + /// shown at once, a checkout here updates the session's branch. + fn check_out(&mut self, place: CheckoutPlace, cx: &mut Context) { + let (Some(detail), Some(session_id), Some(service)) = ( + self.detail.clone(), + self.session_id.clone(), + session_service(cx), + ) else { + return; + }; + let pr = detail.read(cx).pr().clone(); + detail.update(cx, |detail, cx| detail.set_checking_out(true, cx)); + let detail = detail.downgrade(); + cx.spawn(async move |_, cx| { + let done = service + .pull_request_checkout(session_id.clone(), pr, place) + .await; + detail + .update(cx, |detail, cx| { + detail.set_checking_out(false, cx); + detail.checked_out(place, &done, cx); + }) + .ok(); + if let Ok(PullRequestCheckout { session_id: to, .. }) = done { + cx.update(|cx| { + if let Some(gpui) = cx.try_global::() { + match place { + CheckoutPlace::Worktree => gpui.open_session(to), + CheckoutPlace::Here => gpui.cmd_list_branches_and_worktrees(to), + } + } + }); + } + }) + .detach(); + } + fn open(&mut self, number: u64, cx: &mut Context) { let Some(repo) = &self.repo else { return; From 20d60f0a8f2cc9a9d8c551a2a4aec26b32fcb152 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Fri, 30 Oct 2026 14:15:07 +0100 Subject: [PATCH 5/9] Stacked pull requests in the github and git crates Ported from T3 Code: Client::stack reads the stack a pull request is a layer of (repos/o/n/stacks?pull_request=N, ETag-revalidated, then the stack's own read for titles, drafts and heads; 404 means none); listings on github.com mark where their rows sit in a stack (aliased PullRequestStackMembership, failing quietly); merge_stack merges through one layer with merge-async and polls until GitHub settles; stack_rebase checks the stack is as the viewer saw it and every open branch may be pushed to, and hands back what a rebase needs. git::stack_rebase replays each layer onto the rebased one below in a scratch repository and pushes it with a lease on the head it had; the credential goes through GIT_CONFIG_* in the environment. --- Cargo.lock | 1 + crates/git/Cargo.toml | 2 +- crates/git/src/lib.rs | 1 + crates/git/src/stack_rebase.rs | 337 +++++++++++++ crates/github/Cargo.toml | 1 + crates/github/src/convert/list.rs | 1 + crates/github/src/graphql/mod.rs | 11 +- crates/github/src/graphql/stacks.graphql | 29 ++ crates/github/src/model/mod.rs | 2 + crates/github/src/model/pull_request.rs | 3 + crates/github/src/model/stack.rs | 72 +++ crates/github/src/ops/list.rs | 20 +- crates/github/src/ops/mod.rs | 1 + crates/github/src/ops/stacks.rs | 572 +++++++++++++++++++++++ crates/github/tests/operations.rs | 180 ++++++- 15 files changed, 1221 insertions(+), 12 deletions(-) create mode 100644 crates/git/src/stack_rebase.rs create mode 100644 crates/github/src/graphql/stacks.graphql create mode 100644 crates/github/src/model/stack.rs create mode 100644 crates/github/src/ops/stacks.rs diff --git a/Cargo.lock b/Cargo.lock index 4cc47604..d7838dfc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3380,6 +3380,7 @@ name = "github" version = "0.1.0" dependencies = [ "async-trait", + "base64 0.22.1", "chrono", "futures", "github", diff --git a/crates/git/Cargo.toml b/crates/git/Cargo.toml index 6aa609af..a4bc4386 100644 --- a/crates/git/Cargo.toml +++ b/crates/git/Cargo.toml @@ -8,7 +8,7 @@ license = "MIT" anyhow = "1.0.95" notify = { version = "7", features = ["macos_fsevent"] } serde = { version = "1.0.215", features = ["derive"] } -tokio = { version = "1.40.0", features = ["fs", "process"] } +tokio = { version = "1.40.0", features = ["fs", "process", "time"] } tracing = "0.1.40" which = "7.0" diff --git a/crates/git/src/lib.rs b/crates/git/src/lib.rs index c857ba8b..0c40265f 100644 --- a/crates/git/src/lib.rs +++ b/crates/git/src/lib.rs @@ -5,6 +5,7 @@ mod merged; mod patch; pub mod pull_request; mod repository; +pub mod stack_rebase; mod types; mod watch; pub mod worktree; diff --git a/crates/git/src/stack_rebase.rs b/crates/git/src/stack_rebase.rs new file mode 100644 index 00000000..e36a9970 --- /dev/null +++ b/crates/git/src/stack_rebase.rs @@ -0,0 +1,337 @@ +//! Rebasing a stack of branches, ported from T3 Code's +//! `cascadeRebaseStack`: in a scratch repository (the user's checkouts +//! stay untouched) each layer is replayed onto the rebased layer below, +//! bottom to top, and pushed with a lease on the head it had. A layer that +//! conflicts stops the cascade; the layers pushed before it stay. +//! +//! The credential travels in the environment (`GIT_CONFIG_*`), never on a +//! command line. + +use crate::GitBinary; +use std::path::{Path, PathBuf}; +use std::time::Duration; + +/// Each git command may take this long. +const STEP_TIMEOUT: Duration = Duration::from_secs(120); + +/// A branch of the stack and the head it is expected at. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct StackBranch { + pub number: u64, + pub branch: String, + pub head_sha: String, +} + +/// Where a cascade stopped, in T3 Code's words. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum StackRebaseError { + Conflict { + number: u64, + completed: usize, + }, + Git { + step: &'static str, + number: u64, + completed: usize, + detail: String, + }, +} + +impl std::fmt::Display for StackRebaseError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + StackRebaseError::Conflict { number, completed } => write!( + f, + "PR #{number} conflicts with the layer below it. {completed} layers were \ + rebased; resolve #{number} with `gh stack rebase` and push." + ), + StackRebaseError::Git { + step, + number, + completed, + .. + } => write!( + f, + "Stack rebase stopped at PR #{number} while {step}. {completed} layers were \ + rebased." + ), + } + } +} + +impl std::error::Error for StackRebaseError {} + +/// A scratch directory, removed when dropped. +struct Scratch(PathBuf); + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +struct Cascade<'a> { + git: &'a GitBinary, + dir: &'a Path, + /// `GIT_CONFIG_KEY_n` / `VALUE_n` pairs. + config: Vec<(String, String)>, +} + +impl Cascade<'_> { + /// Run `git args`; its stdout, trimmed. + async fn run(&self, args: &[&str]) -> anyhow::Result { + let mut command = self.git.command(self.dir); + command.env("GIT_CONFIG_COUNT", self.config.len().to_string()); + for (ix, (key, value)) in self.config.iter().enumerate() { + command.env(format!("GIT_CONFIG_KEY_{ix}"), key); + command.env(format!("GIT_CONFIG_VALUE_{ix}"), value); + } + command.args(args); + let output = tokio::time::timeout(STEP_TIMEOUT, command.output()) + .await + .map_err(|_| anyhow::anyhow!("git {} timed out", args.join(" ")))??; + if !output.status.success() { + anyhow::bail!( + "git {} failed: {}", + args.first().copied().unwrap_or_default(), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_owned()) + } +} + +/// Rebase `layers` (bottom to top) of the repository at `remote_url` onto +/// `base`: each layer onto the rebased layer below, pushed when it moved. +/// `auth_header` is an `http.extraheader` value for the remote. +pub async fn rebase_stack( + git: &GitBinary, + remote_url: &str, + auth_header: &str, + base: &str, + layers: &[StackBranch], +) -> Result<(), StackRebaseError> { + let Some(first) = layers.first() else { + return Ok(()); + }; + let failed = |step: &'static str, number: u64, completed: usize| { + move |error: anyhow::Error| StackRebaseError::Git { + step, + number, + completed, + detail: format!("{error:#}"), + } + }; + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + let scratch = Scratch(std::env::temp_dir().join(format!( + "code-assistant-stack-rebase-{}-{nanos}", + std::process::id() + ))); + std::fs::create_dir_all(&scratch.0) + .map_err(|e| failed("preparing", first.number, 0)(e.into()))?; + let mut cascade = Cascade { + git, + dir: &scratch.0, + config: vec![( + format!("http.{remote_url}.extraheader"), + auth_header.to_owned(), + )], + }; + cascade + .run(&["init", "--quiet"]) + .await + .map_err(failed("preparing", first.number, 0))?; + // Rebased commits need a committer; the user's own when configured. + if cascade.run(&["config", "user.email"]).await.is_err() { + cascade.config.extend([ + ("user.name".to_owned(), "code-assistant".to_owned()), + ( + "user.email".to_owned(), + "noreply@code-assistant.invalid".to_owned(), + ), + ]); + } + cascade + .run(&["remote", "add", "origin", remote_url]) + .await + .map_err(failed("preparing", first.number, 0))?; + let mut refspecs = vec![format!("+refs/heads/{base}:refs/remotes/origin/{base}")]; + refspecs.extend( + layers + .iter() + .map(|l| format!("+refs/heads/{0}:refs/remotes/origin/{0}", l.branch)), + ); + let mut fetch = vec!["fetch", "--quiet", "--no-tags", "origin"]; + fetch.extend(refspecs.iter().map(String::as_str)); + cascade + .run(&fetch) + .await + .map_err(failed("fetching", first.number, 0))?; + + let base_ref = format!("refs/remotes/origin/{base}"); + let (mut parent_old, mut parent_new) = (base_ref.clone(), base_ref.clone()); + for (completed, layer) in layers.iter().enumerate() { + let number = layer.number; + let upstream = match completed { + 0 => cascade + .run(&["merge-base", &base_ref, &layer.head_sha]) + .await + .map_err(failed("reading the fork point", number, completed))?, + _ => parent_old.clone(), + }; + cascade + .run(&["checkout", "--quiet", "--detach", &layer.head_sha]) + .await + .map_err(failed("checking out", number, completed))?; + if cascade + .run(&["rebase", "--quiet", "--onto", &parent_new, &upstream]) + .await + .is_err() + { + let _ = cascade.run(&["rebase", "--abort"]).await; + return Err(StackRebaseError::Conflict { number, completed }); + } + let rebased = cascade + .run(&["rev-parse", "HEAD"]) + .await + .map_err(failed("rebasing", number, completed))?; + if rebased != layer.head_sha { + let lease = format!( + "--force-with-lease=refs/heads/{}:{}", + layer.branch, layer.head_sha + ); + let target = format!("{rebased}:refs/heads/{}", layer.branch); + cascade + .run(&["push", "--quiet", &lease, "origin", &target]) + .await + .map_err(failed("pushing", number, completed))?; + } + parent_old = layer.head_sha.clone(); + parent_new = rebased; + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::process::Command; + + fn run(dir: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .args([ + "-c", + "user.email=t@t", + "-c", + "user.name=T", + "-c", + "commit.gpgsign=false", + ]) + .args(args) + .current_dir(dir) + .output() + .unwrap(); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout).unwrap().trim().to_owned() + } + + fn commit(dir: &Path, file: &str, text: &str) -> String { + std::fs::write(dir.join(file), text).unwrap(); + run(dir, &["add", file]); + run(dir, &["commit", "-q", "-m", file]); + run(dir, &["rev-parse", "HEAD"]) + } + + /// A bare "GitHub" with `main`, `one` on it and `two` on `one`; then + /// `main` moves on. The work happens in `work`, pushed to the remote. + fn stacked() -> (tempfile::TempDir, PathBuf, Vec) { + let dir = tempfile::TempDir::new().unwrap(); + let remote = dir.path().join("remote.git"); + let work = dir.path().join("work"); + run( + dir.path(), + &[ + "init", + "-q", + "--bare", + "-b", + "main", + remote.to_str().unwrap(), + ], + ); + std::fs::create_dir(&work).unwrap(); + run(&work, &["init", "-q", "-b", "main"]); + commit(&work, "base", "base"); + run(&work, &["checkout", "-q", "-b", "one"]); + let one = commit(&work, "one", "one"); + run(&work, &["checkout", "-q", "-b", "two"]); + let two = commit(&work, "two", "two"); + run(&work, &["checkout", "-q", "main"]); + commit(&work, "later", "later"); + run( + &work, + &["push", "-q", remote.to_str().unwrap(), "main", "one", "two"], + ); + let layers = vec![ + StackBranch { + number: 1, + branch: "one".into(), + head_sha: one, + }, + StackBranch { + number: 2, + branch: "two".into(), + head_sha: two, + }, + ]; + (dir, remote, layers) + } + + #[tokio::test] + async fn every_layer_lands_on_the_rebased_layer_below() { + let (_dir, remote, layers) = stacked(); + let git = GitBinary::new().unwrap(); + let url = remote.to_str().unwrap(); + rebase_stack(&git, url, "AUTHORIZATION: basic x", "main", &layers) + .await + .unwrap(); + let parent = |branch: &str| run(&remote, &["rev-parse", &format!("{branch}~1")]); + let tip = |branch: &str| run(&remote, &["rev-parse", branch]); + assert_eq!(parent("one"), tip("main")); + assert_eq!(parent("two"), tip("one")); + assert_ne!(tip("one"), layers[0].head_sha); + } + + #[tokio::test] + async fn a_branch_pushed_to_meanwhile_is_not_overwritten() { + let (dir, remote, layers) = stacked(); + // Someone pushed to `one` since the stack was read. + let work = dir.path().join("work"); + run(&work, &["checkout", "-q", "one"]); + commit(&work, "extra", "extra"); + run(&work, &["push", "-q", remote.to_str().unwrap(), "one"]); + let git = GitBinary::new().unwrap(); + let stopped = rebase_stack(&git, remote.to_str().unwrap(), "x: y", "main", &layers) + .await + .unwrap_err(); + assert!( + matches!( + stopped, + StackRebaseError::Git { + step: "pushing", + number: 1, + completed: 0, + .. + } + ), + "{stopped:?}" + ); + } +} diff --git a/crates/github/Cargo.toml b/crates/github/Cargo.toml index 2de633c6..6b1675ee 100644 --- a/crates/github/Cargo.toml +++ b/crates/github/Cargo.toml @@ -26,6 +26,7 @@ thiserror = "2.0" tokio = { version = "1.48", features = ["process", "sync", "time"] } tracing = "0.1" urlencoding = "2.1" +base64 = "0.22" [dev-dependencies] github = { path = ".", features = ["test-utils"] } diff --git a/crates/github/src/convert/list.rs b/crates/github/src/convert/list.rs index 3fc5285e..764d7aca 100644 --- a/crates/github/src/convert/list.rs +++ b/crates/github/src/convert/list.rs @@ -160,6 +160,7 @@ fn entry(row: RawRow) -> ListEntry { .any(|r| trimmed(r.login.as_deref()).is_none() && trimmed(r.slug.as_deref()).is_some()), labels: labels(&row.labels), checks_state: checks_state(&rollup_states(&row.commits)), + stack: None, } } diff --git a/crates/github/src/graphql/mod.rs b/crates/github/src/graphql/mod.rs index f09ccfc2..3c6eb936 100644 --- a/crates/github/src/graphql/mod.rs +++ b/crates/github/src/graphql/mod.rs @@ -19,6 +19,7 @@ const DOCUMENTS: &[&str] = &[ include_str!("batches.graphql"), include_str!("mutations.graphql"), include_str!("heads.graphql"), + include_str!("stacks.graphql"), ]; /// Every operation by name, ready to send. @@ -114,6 +115,7 @@ mod tests { "MergePullRequest", "PullRequestActionState", "PullRequestActivity", + "PullRequestBranchAccess", "PullRequestBranchToDelete", "PullRequestCheckContexts", "PullRequestCommitStats", @@ -123,6 +125,7 @@ mod tests { "PullRequestNodeId", "PullRequestSearch", "PullRequestSourceBranch", + "PullRequestStackMembership", "PullRequestStats", "PullRequestSubject", "PullRequestSummary", @@ -146,7 +149,13 @@ mod tests { assert!(OPERATIONS.contains_key(name), "{name} is not defined"); } assert_eq!(OPERATIONS.len(), sent.len(), "an operation is never sent"); - for batched in ["PullRequestSummary", "PullRequestStats", "SetFileViewed"] { + for batched in [ + "PullRequestSummary", + "PullRequestStats", + "SetFileViewed", + "PullRequestStackMembership", + "PullRequestBranchAccess", + ] { alias::aliased_operation(operation(batched), 2); } } diff --git a/crates/github/src/graphql/stacks.graphql b/crates/github/src/graphql/stacks.graphql new file mode 100644 index 00000000..3ea99e43 --- /dev/null +++ b/crates/github/src/graphql/stacks.graphql @@ -0,0 +1,29 @@ +# Stacked pull requests (GitHub's stacks): which stack a pull request is a layer of, and whether +# the viewer may push to a layer's branch before rebasing the stack. + +query PullRequestStackMembership($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + stack { + number + size + baseRefName + } + stackEntry { + position + } + } + } +} + +query PullRequestBranchAccess($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + headRepository { + viewerPermission + } + # A fork's branch is writable without repository access when its owner allows it. + maintainerCanModify + } + } +} diff --git a/crates/github/src/model/mod.rs b/crates/github/src/model/mod.rs index 450e46b0..bee5d940 100644 --- a/crates/github/src/model/mod.rs +++ b/crates/github/src/model/mod.rs @@ -7,6 +7,7 @@ mod common; mod files; mod pull_request; mod review; +mod stack; pub use activity::*; pub use checks::*; @@ -14,3 +15,4 @@ pub use common::*; pub use files::*; pub use pull_request::*; pub use review::*; +pub use stack::*; diff --git a/crates/github/src/model/pull_request.rs b/crates/github/src/model/pull_request.rs index 6e862d12..340ab66d 100644 --- a/crates/github/src/model/pull_request.rs +++ b/crates/github/src/model/pull_request.rs @@ -77,6 +77,9 @@ pub struct ListEntry { pub has_team_review_request: bool, pub labels: Vec