From 92101471bab7e320ea98e2cf9a0f43e2000e5bbf Mon Sep 17 00:00:00 2001 From: Tryanks Date: Fri, 9 Oct 2026 10:31:43 +0800 Subject: [PATCH 01/15] feat: write to a linked pull request from the host: comment, review from a host-owned draft, reply, resolve, react, edit, labels, reviewers --- crates/core/src/project.rs | 4 + crates/core/src/pull_request.rs | 136 ++++++ crates/protocol/src/command.rs | 23 +- crates/protocol/src/lib.rs | 19 +- crates/protocol/src/pull_request.rs | 186 +++++++- crates/runtime/src/app/authorization.rs | 2 + crates/runtime/src/app/pull_requests.rs | 97 +++- crates/runtime/src/pipe.rs | 22 + crates/services/src/github/mod.rs | 1 + .../src/github/pull_request_actions.rs | 436 ++++++++++++++++++ .../services/src/github/pull_request_reads.rs | 243 +++++++++- crates/ui/src/pull_requests/conversation.rs | 25 +- 12 files changed, 1146 insertions(+), 48 deletions(-) create mode 100644 crates/services/src/github/pull_request_actions.rs diff --git a/crates/core/src/project.rs b/crates/core/src/project.rs index 4bcc172fc..c6ad5ff38 100644 --- a/crates/core/src/project.rs +++ b/crates/core/src/project.rs @@ -84,6 +84,9 @@ pub enum SettledOverride { pub struct SessionMeta { #[serde(default)] pub pull_requests: Vec, + /// Reviews being written in this thread, one per pull request it shows. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub pull_request_reviews: Vec, pub id: String, pub title: String, pub provider: ProviderKind, @@ -275,6 +278,7 @@ impl SessionMeta { id: uuid::Uuid::new_v4().to_string(), title: format!("New {} session", provider.display_name()), pull_requests: Vec::new(), + pull_request_reviews: Vec::new(), provider, profile_id: None, cwd, diff --git a/crates/core/src/pull_request.rs b/crates/core/src/pull_request.rs index 14e9134ec..e379971b1 100644 --- a/crates/core/src/pull_request.rs +++ b/crates/core/src/pull_request.rs @@ -443,6 +443,142 @@ pub fn groups(links: &[ThreadPullRequestLink]) -> Vec> { groups } +/// A review being written on the host. GitHub sees none of it until it is submitted whole, so a +/// draft outlives a client, a device and a moved head alike. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestReviewDraft { + pub key: PullRequestKey, + #[serde(default, skip_serializing_if = "String::is_empty")] + pub body: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub comments: Vec, + /// Never reused, so an edit naming a removed comment cannot reach a later one. + #[serde(default)] + pub next_id: u64, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestReviewDraftComment { + pub id: u64, + /// The head commit whose lines the comment is on. + pub revision: String, + pub path: String, + pub side: crate::session::ReviewSide, + pub start_line: u32, + pub end_line: u32, + pub body: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "type", content = "content", rename_all = "snake_case")] +pub enum PullRequestReviewDraftEdit { + AddComment { + revision: String, + path: String, + side: crate::session::ReviewSide, + start_line: u32, + end_line: u32, + body: String, + }, + RemoveComment { + id: u64, + }, + SetBody { + body: String, + }, + Discard, +} + +/// Applies an edit to the thread's drafts; false when it changed nothing or was malformed. +pub fn edit_review_draft( + drafts: &mut Vec, + key: &PullRequestKey, + edit: PullRequestReviewDraftEdit, +) -> bool { + let index = match drafts.iter().position(|draft| draft.key == *key) { + Some(index) => index, + None if matches!(edit, PullRequestReviewDraftEdit::AddComment { .. }) + || matches!(&edit, PullRequestReviewDraftEdit::SetBody { body } if !body.is_empty()) => + { + drafts.push(PullRequestReviewDraft { + key: key.clone(), + body: String::new(), + comments: Vec::new(), + next_id: 1, + }); + drafts.len() - 1 + } + None => return false, + }; + let draft = &mut drafts[index]; + let changed = match edit { + PullRequestReviewDraftEdit::AddComment { + revision, + path, + side, + start_line, + end_line, + body, + } => { + if path.is_empty() || start_line == 0 || start_line > end_line || body.trim().is_empty() + { + false + } else { + draft.comments.push(PullRequestReviewDraftComment { + id: draft.next_id, + revision, + path, + side, + start_line, + end_line, + body, + }); + draft.next_id += 1; + true + } + } + PullRequestReviewDraftEdit::RemoveComment { id } => { + let before = draft.comments.len(); + draft.comments.retain(|comment| comment.id != id); + draft.comments.len() != before + } + PullRequestReviewDraftEdit::SetBody { body } => { + body != std::mem::replace(&mut draft.body, body.clone()) + } + PullRequestReviewDraftEdit::Discard => { + drafts.remove(index); + return true; + } + }; + if draft.body.is_empty() && draft.comments.is_empty() { + drafts.remove(index); + } + changed +} + +/// Takes what a submission sent out of the draft: its comments by id, and the body only while it +/// still reads as sent, since a body revised meanwhile is new work. +pub fn submitted_review( + drafts: &mut Vec, + key: &PullRequestKey, + comments: &[u64], + body: &str, +) { + let Some(index) = drafts.iter().position(|draft| draft.key == *key) else { + return; + }; + let draft = &mut drafts[index]; + draft + .comments + .retain(|comment| !comments.contains(&comment.id)); + if draft.body == body { + draft.body.clear(); + } + if draft.body.is_empty() && draft.comments.is_empty() { + drafts.remove(index); + } +} + /// A menu visibility hint only; the host forge adapter validates and canonicalizes the target. pub fn is_pull_request_url(value: &str) -> bool { let value = value.split(['?', '#']).next().unwrap_or_default(); diff --git a/crates/protocol/src/command.rs b/crates/protocol/src/command.rs index ee415b166..658cc4b04 100644 --- a/crates/protocol/src/command.rs +++ b/crates/protocol/src/command.rs @@ -117,6 +117,18 @@ pub enum Command { paths: Vec, viewed: bool, }, + /// A write to a linked pull request, answered with [`CommandResponse::PullRequestAction`]. + /// It is never retained for redelivery: a write resent after a lost answer could apply twice. + RunPullRequestAction { + session_id: String, + key: tcode_core::pull_request::PullRequestKey, + action: crate::PullRequestAction, + }, + EditPullRequestReviewDraft { + session_id: String, + key: tcode_core::pull_request::PullRequestKey, + edit: tcode_core::pull_request::PullRequestReviewDraftEdit, + }, SetProfileSecret { profile_id: String, name: String, @@ -487,6 +499,7 @@ pub enum CommandResponse { key: tcode_core::pull_request::PullRequestKey, already_linked: bool, }, + PullRequestAction(crate::PullRequestActionResult), ProjectId(Option), SessionId(Option), PendingRelaunchSection { @@ -505,6 +518,8 @@ impl Command { | Self::WatchPullRequest { session_id, .. } | Self::SetPullRequestFilesViewed { session_id, .. } | Self::RefreshPullRequest { session_id, .. } + | Self::RunPullRequestAction { session_id, .. } + | Self::EditPullRequestReviewDraft { session_id, .. } | Self::OrchestrateTurn { session_id, .. } | Self::RunGitAction { session_id, .. } | Self::SetActiveAcpAgent { session_id, .. } @@ -566,9 +581,10 @@ impl Command { ) } - /// Idempotent controls and reads do not need retained delivery. - /// All other variants are retained writes, including settings assignments: - /// repeating an old assignment after a newer one would undo user intent. + /// Idempotent controls and reads do not need retained delivery, and a + /// pull request action must not have it. All other variants are retained + /// writes, including settings assignments: repeating an old assignment + /// after a newer one would undo user intent. pub fn requires_delivery_key(&self) -> bool { !matches!( self, @@ -577,6 +593,7 @@ impl Command { | Self::ShutdownAllAndFlush | Self::OpenLatestSession | Self::RefreshGitHubCredentials + | Self::RunPullRequestAction { .. } | Self::RefreshProviderStatus | Self::RefreshProviderUsage | Self::CheckProviderVersions diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index e8e70fbda..fb37431e1 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -11,11 +11,14 @@ mod pull_request; mod query; pub use preview::{PreviewRequest, PreviewResponse}; pub use pull_request::{ - MAX_PULL_REQUEST_MEDIA_BYTES, PullRequestActor, PullRequestComment, PullRequestConversation, - PullRequestFile, PullRequestFileText, PullRequestFiles, PullRequestMedia, PullRequestPatch, - PullRequestReaction, PullRequestRead, PullRequestReadResponse, PullRequestReviewAnchor, - PullRequestReviewState, PullRequestReviewThread, PullRequestThreadReplies, - PullRequestViewedFiles, PullRequestViewedState, + MAX_PULL_REQUEST_MEDIA_BYTES, PullRequestAction, PullRequestActionResult, PullRequestActor, + PullRequestComment, PullRequestConversation, PullRequestFile, PullRequestFileText, + PullRequestFiles, PullRequestLabelCandidate, PullRequestLabelCandidates, PullRequestMedia, + PullRequestPatch, PullRequestReaction, PullRequestReactionContent, PullRequestRead, + PullRequestReadResponse, PullRequestRejection, PullRequestReviewAnchor, PullRequestReviewState, + PullRequestReviewThread, PullRequestReviewVerdict, PullRequestReviewer, + PullRequestReviewerCandidate, PullRequestReviewerCandidates, PullRequestReviewerKind, + PullRequestThreadReplies, PullRequestViewedFiles, PullRequestViewedState, }; pub mod terminal; mod wire; @@ -111,6 +114,12 @@ pub use wire::{ // RefreshPullRequest drops the host's reads of one and syncs it. // Unreleased: thread metadata carries pinned_at, pin_order and active_order; // adds the PinSession, UnpinSession, ReorderPinned and ReorderActive commands. +// Unreleased: RunPullRequestAction writes to a linked pull request (comment, +// review, thread reply and resolution, reactions, edits, labels, reviewers) +// and answers PullRequestAction with a typed result; EditPullRequestReviewDraft +// edits the host-owned review draft that thread metadata carries as +// pull_request_reviews; Query::PullRequest reads label and reviewer +// candidates; a reaction's content is a typed name. pub const PROTOCOL_VERSION: u32 = 10; #[cfg(test)] diff --git a/crates/protocol/src/pull_request.rs b/crates/protocol/src/pull_request.rs index 58b939249..bb9169e2b 100644 --- a/crates/protocol/src/pull_request.rs +++ b/crates/protocol/src/pull_request.rs @@ -34,6 +34,11 @@ pub enum PullRequestRead { url: String, validator: Option, }, + /// The repository's labels, with the ones on the pull request marked. + LabelCandidates, + /// Whoever the pull request may be sent to for review, with current requests marked. The + /// organization's teams are not listed: that needs `read:org`, which a token need not carry. + ReviewerCandidates, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -90,10 +95,35 @@ pub struct PullRequestActor { pub avatar_url: Option, } +/// The reactions GitHub offers, in the order its picker shows them. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum PullRequestReactionContent { + ThumbsUp, + ThumbsDown, + Laugh, + Hooray, + Confused, + Heart, + Rocket, + Eyes, +} +impl PullRequestReactionContent { + pub const ALL: [Self; 8] = [ + Self::ThumbsUp, + Self::ThumbsDown, + Self::Laugh, + Self::Hooray, + Self::Confused, + Self::Heart, + Self::Rocket, + Self::Eyes, + ]; +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct PullRequestReaction { - /// GitHub's reaction name, such as `THUMBS_UP`. - pub content: String, + pub content: PullRequestReactionContent, pub count: u64, pub viewer_reacted: bool, } @@ -203,6 +233,54 @@ pub enum PullRequestMedia { Unsupported, } +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestLabelCandidate { + pub name: String, + /// Hex without the `#`. + pub color: Option, + pub description: Option, + pub applied: bool, +} + +/// Labels on the pull request lead, including any the repository no longer lists. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestLabelCandidates { + pub labels: Vec, + /// False when the repository has more labels than one page. + pub complete: bool, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum PullRequestReviewerKind { + User, + /// Named by its slug. + Team, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestReviewer { + pub login: String, + pub kind: PullRequestReviewerKind, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestReviewerCandidate { + pub reviewer: PullRequestReviewer, + pub name: Option, + pub avatar_url: Option, + pub requested: bool, +} + +/// Current requests lead, even for someone GitHub no longer counts assignable; the author is +/// left out, since GitHub refuses to ask them. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestReviewerCandidates { + pub reviewers: Vec, + /// False when the repository has more assignable people than one page. + pub complete: bool, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "type", content = "content", rename_all = "snake_case")] pub enum PullRequestReadResponse { @@ -212,4 +290,108 @@ pub enum PullRequestReadResponse { ThreadReplies(PullRequestThreadReplies), ViewedFiles(PullRequestViewedFiles), Media(PullRequestMedia), + LabelCandidates(PullRequestLabelCandidates), + ReviewerCandidates(PullRequestReviewerCandidates), +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum PullRequestReviewVerdict { + Comment, + Approve, + RequestChanges, +} + +/// A write to a linked pull request. Each is sent once: a result that cannot say whether GitHub +/// applied it is reported as uncertain, never tried again. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "type", content = "content", rename_all = "snake_case")] +pub enum PullRequestAction { + Comment { + body: String, + }, + /// Sends the thread's review draft of the pull request in one submission. `head` is the + /// commit the reviewer read; a pull request whose head has moved since keeps the draft. + SubmitReview { + verdict: PullRequestReviewVerdict, + head: String, + }, + ReplyToThread { + thread_id: String, + body: String, + }, + ResolveThread { + thread_id: String, + resolved: bool, + }, + /// `subject_id` names the pull request itself, a comment or a review. + React { + subject_id: String, + content: PullRequestReactionContent, + reacted: bool, + }, + /// An issue comment or a review comment. + EditComment { + comment_id: String, + body: String, + }, + /// A field left `None` is not sent, so GitHub keeps its text. + Edit { + title: Option, + body: Option, + }, + /// One request for every label. + AddLabels { + labels: Vec, + }, + /// One request per label, in order, stopping at the first that fails. + RemoveLabels { + labels: Vec, + }, + RequestReviewers { + reviewers: Vec, + requested: bool, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "type", content = "content", rename_all = "snake_case")] +pub enum PullRequestActionResult { + Applied, + /// Nothing was written. + Rejected(PullRequestRejection), + /// A write of several requests stopped: `applied` went through, `failure` is what became of + /// the first of `unapplied`, and the rest were not sent. + Partial { + applied: Vec, + unapplied: Vec, + failure: Box, + }, + /// The write was sent and no answer says whether GitHub applied it. + Uncertain, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "type", content = "content", rename_all = "snake_case")] +pub enum PullRequestRejection { + /// The pull request's head is `head`, not the commit the review was written against. + StaleHead { + head: String, + }, + /// The named comment, review or thread belongs to another pull request. + ForeignSubject, + /// A review with nothing in it, an empty comment, or a subject of the wrong kind. + Invalid, + NoCredential, + HostDisabled, + RateLimited { + retry_at: u64, + }, + NotFound, + /// GitHub refused the write, in its own words. + Refused { + messages: Vec, + }, + /// A read the write needed failed, so it was not sent. + Failed, } diff --git a/crates/runtime/src/app/authorization.rs b/crates/runtime/src/app/authorization.rs index 1b812154a..5e62450e3 100644 --- a/crates/runtime/src/app/authorization.rs +++ b/crates/runtime/src/app/authorization.rs @@ -246,6 +246,8 @@ impl AppState { | Command::UnlinkPullRequest { session_id, .. } | Command::SetPullRequestFilesViewed { session_id, .. } | Command::RefreshPullRequest { session_id, .. } + | Command::RunPullRequestAction { session_id, .. } + | Command::EditPullRequestReviewDraft { session_id, .. } | Command::WatchPullRequest { session_id, .. } | Command::Interrupt { session_id } | Command::SetActiveModel { session_id, .. } diff --git a/crates/runtime/src/app/pull_requests.rs b/crates/runtime/src/app/pull_requests.rs index de26120a4..af113fea0 100644 --- a/crates/runtime/src/app/pull_requests.rs +++ b/crates/runtime/src/app/pull_requests.rs @@ -4,7 +4,8 @@ use tcode_core::pull_request::{ PullRequestState, PullRequestSyncError, }; use tcode_protocol::{ - CommandResponse, ProtocolError, PullRequestRead, PullRequestReadResponse, QueryResponse, + CommandResponse, ProtocolError, PullRequestAction, PullRequestActionResult, PullRequestRead, + PullRequestReadResponse, QueryResponse, }; use tcode_services::github::{ CredentialError, GitHubApi, GitHubError, @@ -348,6 +349,14 @@ impl AppState { reads.viewed_files(&key)?, PullRequestReadResponse::ViewedFiles, ), + PullRequestRead::LabelCandidates => reply( + reads.label_candidates(&key)?, + PullRequestReadResponse::LabelCandidates, + ), + PullRequestRead::ReviewerCandidates => reply( + reads.reviewer_candidates(&key)?, + PullRequestReadResponse::ReviewerCandidates, + ), PullRequestRead::Media { url, validator } => { let media = reads.media(&key, &url, validator.as_deref())?; let expires_at = match &media { @@ -408,6 +417,86 @@ impl AppState { .map_err(read_error) }) } + /// A write to the pull request. Whenever GitHub may have applied it, the sync reads the pull + /// request again; a submitted review leaves the draft only once GitHub took it. + pub fn run_pull_request_action( + &mut self, + session_id: &str, + key: PullRequestKey, + action: PullRequestAction, + cx: &mut HostCx, + ) -> HostTask> { + if let Err(error) = self.linked_pull_request(session_id, &key) { + return cx.spawn_background(async move { Err(error) }); + } + let draft = self + .find_meta(session_id) + .and_then(|meta| { + meta.pull_request_reviews + .into_iter() + .find(|draft| draft.key == key) + }) + .unwrap_or_else(|| pull_request::PullRequestReviewDraft { + key: key.clone(), + body: String::new(), + comments: Vec::new(), + next_id: 0, + }); + let reads = self.pull_requests.reads.clone(); + let review = matches!(action, PullRequestAction::SubmitReview { .. }); + let writing = key.clone(); + let sent = draft.clone(); + let task = cx.unblock(move || match &action { + PullRequestAction::SubmitReview { verdict, head } => { + reads.submit_review(&writing, *verdict, head, &sent.body, &sent.comments) + } + action => reads.act(&writing, action), + }); + let host = cx.clone(); + let id = session_id.to_owned(); + cx.spawn_background(async move { + let outcome = task.await; + if !matches!(outcome, PullRequestActionResult::Rejected(_)) { + let applied = outcome == PullRequestActionResult::Applied; + let _ = host + .enqueue_and_wait(move |state, cx| { + if review + && applied + && let Some(mut meta) = state.find_meta(&id) + { + let sent: Vec<_> = + draft.comments.iter().map(|comment| comment.id).collect(); + pull_request::submitted_review( + &mut meta.pull_request_reviews, + &key, + &sent, + &draft.body, + ); + state.save_pull_request_meta(meta, cx); + } + state.request_pull_request_sync(key, cx); + }) + .await; + } + Ok(CommandResponse::PullRequestAction(outcome)) + }) + } + pub fn edit_pull_request_review_draft( + &mut self, + session_id: &str, + key: PullRequestKey, + edit: pull_request::PullRequestReviewDraftEdit, + cx: &mut HostCx, + ) -> Result<(), ProtocolError> { + self.linked_pull_request(session_id, &key)?; + let Some(mut meta) = self.find_meta(session_id) else { + return Ok(()); + }; + if pull_request::edit_review_draft(&mut meta.pull_request_reviews, &key, edit) { + self.save_pull_request_meta(meta, cx); + } + Ok(()) + } fn pull_request_project_cwd(&self, meta: &SessionMeta) -> PathBuf { self.projects .iter() @@ -456,6 +545,9 @@ impl AppState { pub(super) fn save_pull_request_meta(&mut self, meta: SessionMeta, cx: &mut HostCx) { if let Some(resident) = self.meta_mut(&meta.id) { resident.pull_requests.clone_from(&meta.pull_requests); + resident + .pull_request_reviews + .clone_from(&meta.pull_request_reviews); } self.persist_meta(&meta, cx); } @@ -495,6 +587,9 @@ impl AppState { return; } if pull_request::unlink_pull_request(&mut meta.pull_requests, key) { + let links = &meta.pull_requests; + meta.pull_request_reviews + .retain(|draft| pull_request::shown(links, &draft.key)); self.discard_pull_request_wakes(id, Some(key)); self.save_pull_request_meta(meta, cx); } diff --git a/crates/runtime/src/pipe.rs b/crates/runtime/src/pipe.rs index 981200e5e..a2ab50644 100644 --- a/crates/runtime/src/pipe.rs +++ b/crates/runtime/src/pipe.rs @@ -462,6 +462,28 @@ fn dispatch_command(app: &mut AppState, cx: &mut HostCx, command: Command) -> Co cx, )); } + Command::RunPullRequestAction { + session_id, + key, + action, + } => { + return CommandOutcome::Pending(app.run_pull_request_action( + &session_id, + key, + action, + cx, + )); + } + Command::EditPullRequestReviewDraft { + session_id, + key, + edit, + } => { + return CommandOutcome::Immediate( + app.edit_pull_request_review_draft(&session_id, key, edit, cx) + .map(|()| CommandResponse::Unit), + ); + } Command::UnlinkPullRequest { session_id, key } => { app.unlink_pull_request(&session_id, &key, cx); } diff --git a/crates/services/src/github/mod.rs b/crates/services/src/github/mod.rs index f14c07eab..28c8d36bc 100644 --- a/crates/services/src/github/mod.rs +++ b/crates/services/src/github/mod.rs @@ -4,6 +4,7 @@ pub mod api; pub mod credentials; pub mod graphql; pub mod media; +pub mod pull_request_actions; pub mod pull_request_reads; pub mod pull_request_watch; pub mod pull_requests; diff --git a/crates/services/src/github/pull_request_actions.rs b/crates/services/src/github/pull_request_actions.rs new file mode 100644 index 000000000..483d879ff --- /dev/null +++ b/crates/services/src/github/pull_request_actions.rs @@ -0,0 +1,436 @@ +//! Writes to a linked pull request. Each request is sent once and answered in the domain's terms; +//! whatever a write may have changed is dropped from the reads, so the next read sees it. + +use super::{ + CredentialError, GitHubError, RequestOptions, RestRequest, + graphql::Document, + pull_request_reads::{PullRequestReads, Reader, is_revision, percent_encode, reaction_name}, +}; +use serde_json::{Value, json}; +use std::{collections::BTreeMap, time::UNIX_EPOCH}; +use tcode_core::{ + pull_request::{PullRequestKey, PullRequestReviewDraftComment}, + session::ReviewSide, +}; +use tcode_protocol::{ + PullRequestAction, PullRequestActionResult as Outcome, PullRequestRejection as Rejection, + PullRequestReviewVerdict, PullRequestReviewerKind, +}; + +/// The reads a write may change. +#[derive(Clone, Copy)] +enum Affects { + Conversation, + Labels, + Reviewers, +} + +/// Why a write was not sent, or why GitHub refused it. +fn rejection(error: GitHubError) -> Rejection { + match error { + GitHubError::Credential(CredentialError::Disabled) => Rejection::HostDisabled, + GitHubError::Credential(_) | GitHubError::Unauthorized => Rejection::NoCredential, + GitHubError::RateLimited { retry_at, .. } | GitHubError::Paused { retry_at } => { + Rejection::RateLimited { + retry_at: retry_at + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_secs(), + } + } + GitHubError::NotFound => Rejection::NotFound, + GitHubError::Response { messages, .. } => Rejection::Refused { messages }, + GitHubError::InvalidInput => Rejection::Invalid, + _ => Rejection::Failed, + } +} + +/// The answer to a write that was sent. Without an answer, or with a server failure, GitHub may +/// have applied it; a refusal or a rate limit says it did not. +fn answered(result: Result) -> Outcome { + match result { + Ok(_) => Outcome::Applied, + Err( + GitHubError::Request + | GitHubError::Deadline + | GitHubError::BodyTooLarge + | GitHubError::InvalidResponse, + ) => Outcome::Uncertain, + Err(GitHubError::Response { status, .. }) if status >= 500 => Outcome::Uncertain, + Err(error) => Outcome::Rejected(rejection(error)), + } +} + +fn blank(text: &str) -> bool { + text.trim().is_empty() +} + +fn side(side: ReviewSide) -> &'static str { + match side { + ReviewSide::Old => "LEFT", + ReviewSide::New => "RIGHT", + } +} + +impl Reader<'_> { + fn mutate(&self, operation: &'static str, query: &str, variables: Value) -> Outcome { + let Value::Object(variables) = variables else { + unreachable!("mutation variables are an object") + }; + answered(self.api.graphql( + &self.key.host, + &Document { + query: query.to_owned(), + variables: variables.into_iter().collect::>(), + }, + &RequestOptions { + operation, + ..self.options.clone() + }, + )) + } + + fn send( + &self, + operation: &'static str, + method: &str, + rest: &str, + body: Option<&Value>, + ) -> Outcome { + answered(self.api.rest( + &self.key.host, + RestRequest { + method, + path: &self.rest_path(rest), + body, + if_none_match: None, + accept: None, + }, + &RequestOptions { + operation, + ..self.options.clone() + }, + )) + } + + /// The kind of node a client-given id names, once it is known to hang off this pull request: + /// a mutation writes wherever the id belongs, whichever pull request the request names. + fn subject(&self, id: &str) -> Result { + let response = self + .query( + "PullRequestSubject", + "query PullRequestSubject($owner: String!, $name: String!, $number: Int!, $subject: ID!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { id } } node(id: $subject) { __typename id ... on IssueComment { pullRequest { id } } ... on PullRequestReviewComment { pullRequest { id } } ... on PullRequestReview { pullRequest { id } } ... on PullRequestReviewThread { pullRequest { id } } } }".to_owned(), + [("subject", json!(id))], + ) + .map_err(rejection)?; + let data = &response["data"]; + let expected = data["repository"]["pullRequest"]["id"].as_str(); + let node = &data["node"]; + let actual = node["pullRequest"]["id"].as_str().or(node["id"].as_str()); + if expected.is_none() || actual != expected { + return Err(Rejection::ForeignSubject); + } + Ok(node["__typename"].as_str().unwrap_or_default().to_owned()) + } + + fn thread(&self, id: &str) -> Result<(), Rejection> { + match self.subject(id)?.as_str() { + "PullRequestReviewThread" => Ok(()), + _ => Err(Rejection::Invalid), + } + } + + /// Labels come off one request each, since the endpoint names one in its path; the first + /// failure stops the rest. + fn remove_labels(&self, labels: &[String]) -> Outcome { + let issue = format!("issues/{}/labels", self.key.number); + for (index, label) in labels.iter().enumerate() { + let outcome = self.send( + "RemovePullRequestLabel", + "DELETE", + &format!("{issue}/{}", percent_encode(label)), + None, + ); + if outcome == Outcome::Applied { + continue; + } + if index == 0 { + return outcome; + } + return Outcome::Partial { + applied: labels[..index].to_vec(), + unapplied: labels[index..].to_vec(), + failure: Box::new(outcome), + }; + } + Outcome::Applied + } +} + +impl PullRequestReads { + /// Every action but a review, whose content is the runtime's draft: see [`Self::submit_review`]. + pub fn act(&self, key: &PullRequestKey, action: &PullRequestAction) -> Outcome { + let affects = match action { + PullRequestAction::AddLabels { .. } | PullRequestAction::RemoveLabels { .. } => { + Affects::Labels + } + PullRequestAction::RequestReviewers { .. } => Affects::Reviewers, + _ => Affects::Conversation, + }; + let outcome = self.write(key, action).unwrap_or_else(Outcome::Rejected); + self.written(key, affects, &outcome); + outcome + } + + fn write( + &self, + key: &PullRequestKey, + action: &PullRequestAction, + ) -> Result { + let reader = self.reader(key).map_err(rejection)?; + Ok(match action { + PullRequestAction::Comment { body } => { + if blank(body) { + return Err(Rejection::Invalid); + } + let subject = self.node_id(&reader).map_err(rejection)?; + reader.mutate( + "AddPullRequestComment", + "mutation AddPullRequestComment($subjectId: ID!, $body: String!) { addComment(input: { subjectId: $subjectId, body: $body }) { clientMutationId } }", + json!({"subjectId": subject, "body": body}), + ) + } + PullRequestAction::SubmitReview { .. } => return Err(Rejection::Invalid), + PullRequestAction::ReplyToThread { thread_id, body } => { + if blank(body) { + return Err(Rejection::Invalid); + } + reader.thread(thread_id)?; + reader.mutate( + "ReplyToPullRequestThread", + "mutation ReplyToPullRequestThread($threadId: ID!, $body: String!) { addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { comment { id } } }", + json!({"threadId": thread_id, "body": body}), + ) + } + PullRequestAction::ResolveThread { + thread_id, + resolved, + } => { + reader.thread(thread_id)?; + let (operation, query) = if *resolved { + ( + "ResolvePullRequestThread", + "mutation ResolvePullRequestThread($threadId: ID!) { resolveReviewThread(input: { threadId: $threadId }) { thread { isResolved } } }", + ) + } else { + ( + "UnresolvePullRequestThread", + "mutation UnresolvePullRequestThread($threadId: ID!) { unresolveReviewThread(input: { threadId: $threadId }) { thread { isResolved } } }", + ) + }; + reader.mutate(operation, query, json!({"threadId": thread_id})) + } + PullRequestAction::React { + subject_id, + content, + reacted, + } => { + if !matches!( + reader.subject(subject_id)?.as_str(), + "PullRequest" + | "IssueComment" + | "PullRequestReviewComment" + | "PullRequestReview" + ) { + return Err(Rejection::Invalid); + } + let (operation, query) = if *reacted { + ( + "AddPullRequestReaction", + "mutation AddPullRequestReaction($subjectId: ID!, $content: ReactionContent!) { addReaction(input: { subjectId: $subjectId, content: $content }) { reaction { content } } }", + ) + } else { + ( + "RemovePullRequestReaction", + "mutation RemovePullRequestReaction($subjectId: ID!, $content: ReactionContent!) { removeReaction(input: { subjectId: $subjectId, content: $content }) { reaction { content } } }", + ) + }; + reader.mutate( + operation, + query, + json!({"subjectId": subject_id, "content": reaction_name(*content)}), + ) + } + PullRequestAction::EditComment { comment_id, body } => { + if blank(body) { + return Err(Rejection::Invalid); + } + let (operation, query) = match reader.subject(comment_id)?.as_str() { + "IssueComment" => ( + "EditPullRequestComment", + "mutation EditPullRequestComment($commentId: ID!, $body: String!) { updateIssueComment(input: { id: $commentId, body: $body }) { issueComment { id } } }", + ), + "PullRequestReviewComment" => ( + "EditPullRequestReviewComment", + "mutation EditPullRequestReviewComment($commentId: ID!, $body: String!) { updatePullRequestReviewComment(input: { pullRequestReviewCommentId: $commentId, body: $body }) { pullRequestReviewComment { id } } }", + ), + _ => return Err(Rejection::Invalid), + }; + reader.mutate( + operation, + query, + json!({"commentId": comment_id, "body": body}), + ) + } + PullRequestAction::Edit { title, body } => { + if (title.is_none() && body.is_none()) || title.as_deref().is_some_and(blank) { + return Err(Rejection::Invalid); + } + let mut variables = + json!({"pullRequestId": self.node_id(&reader).map_err(rejection)?}); + // A variable left out puts no field in the input, which keeps GitHub's text; + // an empty one would clear it. + if let Some(title) = title { + variables["title"] = json!(title); + } + if let Some(body) = body { + variables["body"] = json!(body); + } + reader.mutate( + "EditPullRequest", + "mutation EditPullRequest($pullRequestId: ID!, $title: String, $body: String) { updatePullRequest(input: { pullRequestId: $pullRequestId, title: $title, body: $body }) { pullRequest { id } } }", + variables, + ) + } + PullRequestAction::AddLabels { labels } => { + if labels.is_empty() { + return Err(Rejection::Invalid); + } + // A pull request is an issue to the labels API, which adds to what is there. + reader.send( + "AddPullRequestLabels", + "POST", + &format!("issues/{}/labels", key.number), + Some(&json!({"labels": labels})), + ) + } + PullRequestAction::RemoveLabels { labels } => { + if labels.is_empty() { + return Err(Rejection::Invalid); + } + reader.remove_labels(labels) + } + PullRequestAction::RequestReviewers { + reviewers, + requested, + } => { + if reviewers.is_empty() { + return Err(Rejection::Invalid); + } + let named = |kind| { + reviewers + .iter() + .filter(|reviewer| reviewer.kind == kind) + .map(|reviewer| reviewer.login.as_str()) + .collect::>() + }; + // GitHub takes a request back from exactly whoever it was made of, so both + // methods send the same body. + reader.send( + "RequestPullRequestReviewers", + if *requested { "POST" } else { "DELETE" }, + &format!("pulls/{}/requested_reviewers", key.number), + Some(&json!({ + "reviewers": named(PullRequestReviewerKind::User), + "team_reviewers": named(PullRequestReviewerKind::Team), + })), + ) + } + }) + } + + /// The whole review in one request, so nothing of it is visible until the verdict is sent. + /// The head is read fresh first: a review read at `head` is not sent once the pull request + /// has moved past it, nor are comments on another commit's lines. + pub fn submit_review( + &self, + key: &PullRequestKey, + verdict: PullRequestReviewVerdict, + head: &str, + body: &str, + comments: &[PullRequestReviewDraftComment], + ) -> Outcome { + let outcome = self + .review(key, verdict, head, body, comments) + .unwrap_or_else(Outcome::Rejected); + self.written(key, Affects::Conversation, &outcome); + outcome + } + + fn review( + &self, + key: &PullRequestKey, + verdict: PullRequestReviewVerdict, + head: &str, + body: &str, + comments: &[PullRequestReviewDraftComment], + ) -> Result { + if !is_revision(head) + || (verdict != PullRequestReviewVerdict::Approve && blank(body) && comments.is_empty()) + { + return Err(Rejection::Invalid); + } + let reader = self.reader(key).map_err(rejection)?; + self.revisions.invalidate(key); + let current = self.revisions(&reader).map_err(rejection)?.head.clone(); + if current != head || comments.iter().any(|comment| comment.revision != current) { + return Err(Rejection::StaleHead { head: current }); + } + let event = match verdict { + PullRequestReviewVerdict::Comment => "COMMENT", + PullRequestReviewVerdict::Approve => "APPROVE", + PullRequestReviewVerdict::RequestChanges => "REQUEST_CHANGES", + }; + let comments: Vec<_> = comments + .iter() + .map(|comment| { + let mut line = json!({ + "path": comment.path, + "line": comment.end_line, + "side": side(comment.side), + "body": comment.body, + }); + if comment.start_line < comment.end_line { + line["start_line"] = json!(comment.start_line); + line["start_side"] = json!(side(comment.side)); + } + line + }) + .collect(); + Ok(reader.send( + "SubmitPullRequestReview", + "POST", + &format!("pulls/{}/reviews", key.number), + Some(&json!({ + "commit_id": current, + "event": event, + "body": body, + "comments": comments, + })), + )) + } + + fn written(&self, key: &PullRequestKey, affects: Affects, outcome: &Outcome) { + if matches!(outcome, Outcome::Rejected(_)) { + return; + } + match affects { + Affects::Conversation => { + self.conversations.invalidate(key); + self.replies.invalidate(key); + } + Affects::Labels => self.labels.invalidate(key), + Affects::Reviewers => self.reviewers.invalidate(key), + } + } +} diff --git a/crates/services/src/github/pull_request_reads.rs b/crates/services/src/github/pull_request_reads.rs index e29ad5d52..0a0317dc3 100644 --- a/crates/services/src/github/pull_request_reads.rs +++ b/crates/services/src/github/pull_request_reads.rs @@ -11,12 +11,18 @@ use super::{ }; use agent::FileChangeKind; use serde_json::{Value, json}; -use std::{collections::BTreeMap, sync::Arc, time::Duration}; +use std::{ + collections::BTreeMap, + sync::{Arc, Mutex}, + time::Duration, +}; use tcode_core::{pull_request::PullRequestKey, session::ReviewSide}; use tcode_protocol::{ PullRequestActor, PullRequestComment, PullRequestConversation, PullRequestFile, - PullRequestFileText, PullRequestFiles, PullRequestMedia, PullRequestPatch, PullRequestReaction, - PullRequestReviewAnchor, PullRequestReviewState, PullRequestReviewThread, + PullRequestFileText, PullRequestFiles, PullRequestLabelCandidate, PullRequestLabelCandidates, + PullRequestMedia, PullRequestPatch, PullRequestReaction, PullRequestReactionContent, + PullRequestReviewAnchor, PullRequestReviewState, PullRequestReviewThread, PullRequestReviewer, + PullRequestReviewerCandidate, PullRequestReviewerCandidates, PullRequestReviewerKind, PullRequestThreadReplies, PullRequestViewedFiles, PullRequestViewedState, }; @@ -33,35 +39,43 @@ const FILES_PER_PAGE: usize = 100; const MAX_PAGES: usize = 10; const VIEWED_PAGES: usize = 5; const EMPTY_BLOB: &str = "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"; +/// Pull requests whose node id is remembered; an id never changes, so this bounds memory only. +const NODE_IDS: usize = 128; const ACTOR: &str = "author { login avatarUrl(size: 64) }"; const REACTIONS: &str = "reactionGroups { content viewerHasReacted reactors { totalCount } }"; #[derive(Debug, Clone)] -struct Revisions { +pub(super) struct Revisions { base: String, - head: String, + pub(super) head: String, changed_files: u64, /// The pull request's GraphQL id, which the viewed mutation names. node_id: String, } +/// The one owner of what the host has read of pull requests; the writes in +/// [`super::pull_request_actions`] drop what they change through it. pub struct PullRequestReads { - api: Arc, - revisions: ReadCache, + pub(super) api: Arc, + pub(super) revisions: ReadCache, files: ReadCache, texts: ReadCache, - conversations: ReadCache, - replies: ReadCache, + pub(super) conversations: ReadCache, + pub(super) replies: ReadCache, viewed: ReadCache, + pub(super) labels: ReadCache, + pub(super) reviewers: ReadCache, + /// Filled only by a successful read, oldest first. + node_ids: Mutex>, } -struct Reader<'a> { - api: &'a GitHubApi, - key: &'a PullRequestKey, - repository: Repository, +pub(super) struct Reader<'a> { + pub(super) api: &'a GitHubApi, + pub(super) key: &'a PullRequestKey, + pub(super) repository: Repository, account: String, - options: RequestOptions, + pub(super) options: RequestOptions, } impl Reader<'_> { @@ -73,7 +87,7 @@ impl Reader<'_> { } } - fn rest_path(&self, rest: &str) -> String { + pub(super) fn rest_path(&self, rest: &str) -> String { format!( "/repos/{}/{}/{rest}", self.repository.owner, self.repository.name @@ -89,7 +103,7 @@ impl Reader<'_> { } } - fn query( + pub(super) fn query( &self, operation: &'static str, query: String, @@ -379,6 +393,100 @@ impl Reader<'_> { complete: !read.truncated, }) } + + fn label_candidates(&self) -> Result { + let response = self.query( + "PullRequestLabelCandidates", + "query PullRequestLabelCandidates($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { labels(first: 100, orderBy: { field: NAME, direction: ASC }) { pageInfo { hasNextPage } nodes { name color description } } pullRequest(number: $number) { labels(first: 100) { nodes { name } } } } }".to_owned(), + [], + )?; + let repository = &response["data"]["repository"]; + if repository["pullRequest"].is_null() { + return Err(GitHubError::NotFound); + } + let applied: Vec<_> = nodes(&repository["pullRequest"]["labels"]) + .filter_map(|label| text(label, "name")) + .collect(); + let listed: Vec<_> = nodes(&repository["labels"]) + .filter_map(|label| { + let name = text(label, "name")?; + Some(PullRequestLabelCandidate { + applied: applied.contains(&name), + color: text(label, "color"), + description: text(label, "description"), + name, + }) + }) + .collect(); + // A label that cannot be seen cannot be taken off, so one the repository no longer lists + // leads anyway. + let missing = applied + .iter() + .filter(|name| listed.iter().all(|label| &label.name != *name)) + .map(|name| PullRequestLabelCandidate { + name: name.clone(), + color: None, + description: None, + applied: true, + }); + Ok(PullRequestLabelCandidates { + labels: missing.chain(listed.iter().cloned()).collect(), + complete: repository["labels"]["pageInfo"]["hasNextPage"].as_bool() != Some(true), + }) + } + + /// `assignableUsers` is the list GitHub's own picker offers; `collaborators` is refused to + /// anyone without push access. + fn reviewer_candidates(&self) -> Result { + let response = self.query( + "PullRequestReviewerCandidates", + "query PullRequestReviewerCandidates($owner: String!, $name: String!, $number: Int!) { repository(owner: $owner, name: $name) { assignableUsers(first: 100) { pageInfo { hasNextPage } nodes { login name avatarUrl } } pullRequest(number: $number) { author { login } reviewRequests(first: 100) { nodes { requestedReviewer { ... on User { login name avatarUrl } ... on Team { slug name avatarUrl } ... on Bot { login avatarUrl } } } } } } }".to_owned(), + [], + )?; + let repository = &response["data"]["repository"]; + let pull_request = &repository["pullRequest"]; + if pull_request.is_null() { + return Err(GitHubError::NotFound); + } + let author = text(&pull_request["author"], "login"); + let mut reviewers: Vec = Vec::new(); + let requests = nodes(&pull_request["reviewRequests"]).map(|node| (node, true)); + let assignable = nodes(&repository["assignableUsers"]).map(|node| (node, false)); + for (node, requested) in requests.chain(assignable) { + let raw = if requested { + &node["requestedReviewer"] + } else { + node + }; + let reviewer = match (text(raw, "slug"), text(raw, "login")) { + (Some(slug), _) => PullRequestReviewer { + login: slug, + kind: PullRequestReviewerKind::Team, + }, + (None, Some(login)) => PullRequestReviewer { + login, + kind: PullRequestReviewerKind::User, + }, + (None, None) => continue, + }; + if (!requested && Some(&reviewer.login) == author.as_ref()) + || reviewers.iter().any(|known| known.reviewer == reviewer) + { + continue; + } + reviewers.push(PullRequestReviewerCandidate { + reviewer, + name: text(raw, "name"), + avatar_url: text(raw, "avatarUrl"), + requested, + }); + } + Ok(PullRequestReviewerCandidates { + reviewers, + complete: repository["assignableUsers"]["pageInfo"]["hasNextPage"].as_bool() + != Some(true), + }) + } } impl PullRequestReads { @@ -391,11 +499,14 @@ impl PullRequestReads { conversations: ReadCache::new(16 * 1024 * 1024), replies: ReadCache::new(4 * 1024 * 1024), viewed: ReadCache::new(4 * 1024 * 1024), + labels: ReadCache::new(1024 * 1024), + reviewers: ReadCache::new(1024 * 1024), + node_ids: Mutex::new(Vec::new()), }) } /// Captures the credential once, so every request of a read is the same account's. - fn reader<'a>(&'a self, key: &'a PullRequestKey) -> Result, GitHubError> { + pub(super) fn reader<'a>(&'a self, key: &'a PullRequestKey) -> Result, GitHubError> { let repository = Repository::from_key(key).ok_or(GitHubError::InvalidInput)?; let credential = self.api.credentials().get(&key.host)?; Ok(Reader { @@ -457,11 +568,33 @@ impl PullRequestReads { ) } - fn revisions(&self, reader: &Reader<'_>) -> Result, GitHubError> { - Ok(self + pub(super) fn revisions(&self, reader: &Reader<'_>) -> Result, GitHubError> { + let revisions = self .revisions .read(reader.read_key("revisions"), || reader.revisions(), |_| 256)? - .value) + .value; + let mut node_ids = self.node_ids.lock().unwrap(); + node_ids.retain(|(key, _)| key != reader.key); + if node_ids.len() >= NODE_IDS { + node_ids.remove(0); + } + node_ids.push((reader.key.clone(), revisions.node_id.clone())); + Ok(revisions) + } + + /// The pull request's GraphQL id, from its REST read the first time it is needed. + pub(super) fn node_id(&self, reader: &Reader<'_>) -> Result { + let held = self + .node_ids + .lock() + .unwrap() + .iter() + .find(|(key, _)| key == reader.key) + .map(|(_, id)| id.clone()); + match held { + Some(id) => Ok(id), + None => Ok(self.revisions(reader)?.node_id.clone()), + } } /// A file's text at a commit of the pull request's repository. @@ -542,7 +675,7 @@ impl PullRequestReads { } fn mark(&self, reader: &Reader<'_>, paths: &[String], viewed: bool) -> Result<(), GitHubError> { - let node_id = self.revisions(reader)?.node_id.clone(); + let node_id = self.node_id(reader)?; let items: Vec<_> = paths .iter() .enumerate() @@ -613,6 +746,42 @@ impl PullRequestReads { media::fetch(&self.api, &source, validator) } + pub fn label_candidates( + &self, + key: &PullRequestKey, + ) -> Result, GitHubError> { + let reader = self.reader(key)?; + self.labels.read( + reader.read_key("labels"), + || Ok((reader.label_candidates()?, READ_TTL)), + |labels| { + labels + .labels + .iter() + .map(|label| label.name.len() + 64) + .sum() + }, + ) + } + + pub fn reviewer_candidates( + &self, + key: &PullRequestKey, + ) -> Result, GitHubError> { + let reader = self.reader(key)?; + self.reviewers.read( + reader.read_key("reviewers"), + || Ok((reader.reviewer_candidates()?, READ_TTL)), + |reviewers| { + reviewers + .reviewers + .iter() + .map(|reviewer| reviewer.reviewer.login.len() + 128) + .sum() + }, + ) + } + /// Drops every answer about the pull request, for a change the sync observed. pub fn invalidate(&self, key: &PullRequestKey) { self.revisions.invalidate(key); @@ -620,6 +789,8 @@ impl PullRequestReads { self.conversations.invalidate(key); self.replies.invalidate(key); self.viewed.invalidate(key); + self.labels.invalidate(key); + self.reviewers.invalidate(key); } } @@ -644,7 +815,7 @@ fn conversation_bytes(conversation: &PullRequestConversation) -> usize { conversation_comments(conversation).map(comment_bytes).sum() } -fn is_revision(value: &str) -> bool { +pub(super) fn is_revision(value: &str) -> bool { matches!(value.len(), 40 | 64) && value .bytes() @@ -659,7 +830,7 @@ fn is_repository_path(path: &str) -> bool { .all(|segment| !matches!(segment, "" | "." | "..")) } -fn percent_encode(segment: &str) -> String { +pub(super) fn percent_encode(segment: &str) -> String { segment .bytes() .map(|byte| { @@ -672,6 +843,27 @@ fn percent_encode(segment: &str) -> String { .collect() } +pub(super) fn reaction_name(content: PullRequestReactionContent) -> &'static str { + match content { + PullRequestReactionContent::ThumbsUp => "THUMBS_UP", + PullRequestReactionContent::ThumbsDown => "THUMBS_DOWN", + PullRequestReactionContent::Laugh => "LAUGH", + PullRequestReactionContent::Hooray => "HOORAY", + PullRequestReactionContent::Confused => "CONFUSED", + PullRequestReactionContent::Heart => "HEART", + PullRequestReactionContent::Rocket => "ROCKET", + PullRequestReactionContent::Eyes => "EYES", + } +} + +fn text(raw: &Value, field: &str) -> Option { + raw[field] + .as_str() + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_owned) +} + fn nodes(connection: &Value) -> impl Iterator { connection["nodes"].as_array().into_iter().flatten() } @@ -707,8 +899,11 @@ fn comment_from( .flatten() .filter_map(|group| { let count = group["reactors"]["totalCount"].as_u64()?; + let content = PullRequestReactionContent::ALL + .into_iter() + .find(|content| Some(reaction_name(*content)) == group["content"].as_str())?; (count > 0).then(|| PullRequestReaction { - content: group["content"].as_str().unwrap_or_default().to_owned(), + content, count, viewer_reacted: group["viewerHasReacted"].as_bool().unwrap_or(false), }) diff --git a/crates/ui/src/pull_requests/conversation.rs b/crates/ui/src/pull_requests/conversation.rs index 5a431be8b..96f3a3bd7 100644 --- a/crates/ui/src/pull_requests/conversation.rs +++ b/crates/ui/src/pull_requests/conversation.rs @@ -10,8 +10,8 @@ use gpui::{ use gpui_base::{Avatar, AvatarFallback, AvatarImage, h_flex, v_flex}; use tcode_core::{pull_request::PullRequestState, session::ReviewSide}; use tcode_protocol::{ - PullRequestComment, PullRequestRead, PullRequestReadResponse, PullRequestReviewState, - PullRequestReviewThread, + PullRequestComment, PullRequestReactionContent, PullRequestRead, PullRequestReadResponse, + PullRequestReviewState, PullRequestReviewThread, }; use super::detail::{PullRequestView, Replies, Tab, ago_rfc3339, reason}; @@ -56,17 +56,16 @@ pub(super) fn visible_body(body: &str) -> Option { (!visible.is_empty()).then_some(visible) } -fn reaction_emoji(content: &str) -> &'static str { +fn reaction_emoji(content: PullRequestReactionContent) -> &'static str { match content { - "THUMBS_UP" => "👍", - "THUMBS_DOWN" => "👎", - "LAUGH" => "😄", - "HOORAY" => "🎉", - "CONFUSED" => "😕", - "HEART" => "❤️", - "ROCKET" => "🚀", - "EYES" => "👀", - _ => "·", + PullRequestReactionContent::ThumbsUp => "👍", + PullRequestReactionContent::ThumbsDown => "👎", + PullRequestReactionContent::Laugh => "😄", + PullRequestReactionContent::Hooray => "🎉", + PullRequestReactionContent::Confused => "😕", + PullRequestReactionContent::Heart => "❤️", + PullRequestReactionContent::Rocket => "🚀", + PullRequestReactionContent::Eyes => "👀", } } @@ -397,7 +396,7 @@ impl PullRequestView { cx.theme().secondary }) .text_size(px(12.)) - .child(reaction_emoji(&reaction.content)) + .child(reaction_emoji(reaction.content)) .child(reaction.count.to_string()) })) .into_any_element(), From e87fcf6a37949c5b5632e59091b1bd6898199acd Mon Sep 17 00:00:00 2001 From: Tryanks Date: Fri, 9 Oct 2026 10:40:22 +0800 Subject: [PATCH 02/15] test: pull request writes through the real client: payloads, subject scope, labels, stale head, no retry, the host's review draft --- crates/client/src/lib.rs | 33 ++ crates/runtime/src/app/command_validation.rs | 2 + crates/runtime/src/app/pull_requests_tests.rs | 174 +++++- crates/services/tests/pull_request_reads.rs | 557 +++++++++++++++++- 4 files changed, 760 insertions(+), 6 deletions(-) diff --git a/crates/client/src/lib.rs b/crates/client/src/lib.rs index c88333799..4ae53a95b 100644 --- a/crates/client/src/lib.rs +++ b/crates/client/src/lib.rs @@ -1376,6 +1376,39 @@ mod tests { assert!(link.inner.pending.lock().unwrap().is_empty()); } + /// A pull request write resent after its answer was lost could post twice, so a lost + /// connection fails it, and reconnecting sends nothing again. + #[test] + fn a_pull_request_action_in_flight_fails_on_disconnect_and_is_not_resent() { + let (to_host, outgoing) = async_channel::unbounded(); + let (_incoming, from_host) = async_channel::unbounded(); + let link = HostLink::new(to_host, from_host); + let mut action = std::pin::pin!(link.command(Command::RunPullRequestAction { + session_id: "one".into(), + key: serde_json::from_value( + serde_json::json!({"host": "github.com", "repository": "octo/repo", "number": 7}), + ) + .unwrap(), + action: tcode_protocol::PullRequestAction::Comment { + body: "Once".into(), + }, + })); + let mut cx = std::task::Context::from_waker(std::task::Waker::noop()); + assert!(action.as_mut().poll(&mut cx).is_pending()); + assert_eq!(request(&outgoing).key, None); + link.set_connection_state(ConnectionState::Reconnecting { + attempt: 1, + reason: None, + }); + match action.as_mut().poll(&mut cx) { + std::task::Poll::Ready(Err(error)) => assert_eq!(error.code, "disconnected"), + other => panic!("the action must fail on disconnect, got {other:?}"), + } + link.set_connection_state(ConnectionState::Syncing { path: None }); + assert!(outgoing.try_recv().is_err(), "nothing was resent"); + assert!(link.pending_commands().is_empty()); + } + #[test] fn full_transport_queue_rejects_requests_without_leaving_a_waiter() { let (outgoing, _receiver) = outgoing::channel(); diff --git a/crates/runtime/src/app/command_validation.rs b/crates/runtime/src/app/command_validation.rs index b706cf273..d582401c7 100644 --- a/crates/runtime/src/app/command_validation.rs +++ b/crates/runtime/src/app/command_validation.rs @@ -253,6 +253,8 @@ impl AppState { | Command::MarkSessionRead { .. } | Command::ForkThread { .. } | Command::WatchPullRequest { .. } + | Command::RunPullRequestAction { .. } + | Command::EditPullRequestReviewDraft { .. } ) { return if self.sessions.iter().any(|meta| meta.id == session_id) || self.resident(session_id).is_some() diff --git a/crates/runtime/src/app/pull_requests_tests.rs b/crates/runtime/src/app/pull_requests_tests.rs index 10165b7b2..2be515587 100644 --- a/crates/runtime/src/app/pull_requests_tests.rs +++ b/crates/runtime/src/app/pull_requests_tests.rs @@ -2,7 +2,8 @@ use super::*; use crate::app::test_support::*; use serde_json::{Value, json}; use std::sync::Mutex; -use tcode_core::pull_request::PullRequestStack; +use tcode_core::pull_request::{PullRequestReviewDraftEdit, PullRequestStack}; +use tcode_protocol::Command; use tcode_services::{github::Credentials, settings::SettingsStore}; use crate::app::test_support::github_fixture as fixture; @@ -1089,3 +1090,174 @@ fn reads_answer_only_a_linked_pull_request_and_a_synced_change_reads_it_fresh() ); assert_eq!(conversations(), 3); } + +#[test] +fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it() { + const OLD: &str = "1111111111111111111111111111111111111111"; + const NEW: &str = "2222222222222222222222222222222222222222"; + let dir = TestStore::new("tcode-pr-review"); + let fixture = fixture::Fixture::new(); + let api = client(&dir, &fixture); + // The head GitHub reports, and every request line and body it saw. + let model = Arc::new(Mutex::new((OLD, Vec::<(String, String)>::new()))); + let responding = model.clone(); + let _server = fixture.serve(move |exchange| { + let line = exchange.request.lines().next().unwrap().to_owned(); + let body = String::from_utf8_lossy(&exchange.body).into_owned(); + let mut model = responding.lock().unwrap(); + model.1.push((line.clone(), body.clone())); + let reply = if line.starts_with("GET /repos/sample/project/pulls/1 ") { + json!({"base": {"sha": "3333333333333333333333333333333333333333"}, + "head": {"sha": model.0}, "changed_files": 1, "node_id": "PR_1"}) + } else if body.contains("PullRequestSummaries") { + json!({"data": {"s0": {"pullRequest": pr(1, "OPEN", false)}}}) + } else { + json!({"id": 1}) + }; + exchange.reply(200, "", &serde_json::to_vec(&reply).unwrap()); + }); + let key = PullRequestKey::new("github.com", "sample/project", 1); + let mut cx = TestAppContext::default(); + let state = cx.new_entity(TestClientState::new((*dir).clone())); + state.update(&mut cx, |state, _| { + state.pull_requests = PullRequestRuntime::new(api.clone()); + let meta = linked("active", 1, false); + dir.upsert_meta(&meta).unwrap(); + state.sessions.push(meta); + }); + let mut next_id = 0; + let mut command = |state: &TestEntity, cx: &mut TestAppContext, command: Command| { + next_id += 1; + state.dispatch_command(cx, next_id, command); + let deadline = std::time::Instant::now() + Duration::from_secs(5); + loop { + cx.run_until_parked(); + if let Some(result) = + cx.drain_outgoing() + .into_iter() + .find_map(|message| match message { + tcode_protocol::HostMessage::Ack { id, result } if id == next_id => { + Some(result) + } + _ => None, + }) + { + return result.unwrap(); + } + assert!( + std::time::Instant::now() < deadline, + "the command was answered" + ); + } + }; + let edit = |edit| Command::EditPullRequestReviewDraft { + session_id: "active".into(), + key: key.clone(), + edit, + }; + let add = |line: u32| { + edit(PullRequestReviewDraftEdit::AddComment { + revision: OLD.into(), + path: "src/lib.rs".into(), + side: tcode_core::session::ReviewSide::New, + start_line: line, + end_line: line, + body: format!("Line {line}"), + }) + }; + command(&state, &mut cx, add(3)); + command(&state, &mut cx, add(8)); + command( + &state, + &mut cx, + edit(PullRequestReviewDraftEdit::RemoveComment { id: 1 }), + ); + command( + &state, + &mut cx, + edit(PullRequestReviewDraftEdit::SetBody { + body: "One thing".into(), + }), + ); + + // A restart: a fresh host over the same data. + state + .update(&mut cx, |state, _| state.close_store()) + .unwrap(); + let store = SessionStore::open_at(dir.root().clone()).unwrap(); + let sessions = store.load_index().unwrap(); + let draft = sessions[0].pull_request_reviews.clone(); + assert_eq!(draft.len(), 1); + assert_eq!( + ( + draft[0].body.as_str(), + draft[0].comments.len(), + draft[0].comments[0].id + ), + ("One thing", 1, 2) + ); + let state = cx.new_entity(TestClientState::new(store.clone())); + state.update(&mut cx, |state, _| { + state.pull_requests = PullRequestRuntime::new(api); + state.sessions = sessions; + }); + let submit = |head: &str| Command::RunPullRequestAction { + session_id: "active".into(), + key: key.clone(), + action: PullRequestAction::SubmitReview { + verdict: tcode_protocol::PullRequestReviewVerdict::Comment, + head: head.into(), + }, + }; + let reviews = || { + model + .lock() + .unwrap() + .1 + .iter() + .filter(|(line, _)| line.starts_with("POST /repos/sample/project/pulls/1/reviews ")) + .count() + }; + + model.lock().unwrap().0 = NEW; + assert_eq!( + command(&state, &mut cx, submit(OLD)), + CommandResponse::PullRequestAction(PullRequestActionResult::Rejected( + tcode_protocol::PullRequestRejection::StaleHead { head: NEW.into() } + )) + ); + assert_eq!(reviews(), 0); + assert_eq!( + state.read(|state| state.find_meta("active").unwrap().pull_request_reviews), + draft, + "the draft written against the old head is kept whole" + ); + + model.lock().unwrap().0 = OLD; + assert_eq!( + command(&state, &mut cx, submit(OLD)), + CommandResponse::PullRequestAction(PullRequestActionResult::Applied) + ); + assert_eq!(reviews(), 1); + assert!( + state.read(|state| state + .find_meta("active") + .unwrap() + .pull_request_reviews + .is_empty()), + "a submitted review leaves the draft" + ); + cx.run_until(|state| !state.pull_requests.syncing && state.pull_requests.requested.is_empty()); + assert!( + model + .lock() + .unwrap() + .1 + .iter() + .any(|(_, body)| body.contains("PullRequestSummaries")), + "the sync reads the pull request after the write" + ); + state + .update(&mut cx, |state, _| state.close_store()) + .unwrap(); +} diff --git a/crates/services/tests/pull_request_reads.rs b/crates/services/tests/pull_request_reads.rs index 400946225..0cb28bac7 100644 --- a/crates/services/tests/pull_request_reads.rs +++ b/crates/services/tests/pull_request_reads.rs @@ -3,10 +3,14 @@ use std::{ sync::{Arc, Mutex}, thread, }; -use tcode_core::{pull_request::PullRequestKey, session::ReviewSide}; +use tcode_core::{ + pull_request::{PullRequestKey, PullRequestReviewDraftComment}, + session::ReviewSide, +}; use tcode_protocol::{ - PullRequestFileText, PullRequestMedia, PullRequestPatch, PullRequestReviewAnchor, - PullRequestViewedState, + PullRequestAction, PullRequestActionResult, PullRequestFileText, PullRequestMedia, + PullRequestPatch, PullRequestReactionContent, PullRequestRejection, PullRequestReviewAnchor, + PullRequestReviewVerdict, PullRequestReviewer, PullRequestReviewerKind, PullRequestViewedState, }; use tcode_services::github::{GitHubApi, GitHubError, pull_request_reads::PullRequestReads}; @@ -68,7 +72,8 @@ impl Seen { type Log = Arc>>; -/// Answers each request with `answer(seen)` and keeps what it saw. +/// Answers each request with `answer(seen)` and keeps what it saw. A status of 0 closes the +/// connection unanswered, as a request lost after it reached GitHub would leave it. fn serve( fixture: Fixture, answer: impl Fn(&Seen) -> (u16, String, Vec) + Send + 'static, @@ -79,7 +84,9 @@ fn serve( let request = Seen::from(&exchange); let (status, headers, body) = answer(&request); seen.lock().unwrap().push(request); - exchange.reply(status, &headers, &body); + if status != 0 { + exchange.reply(status, &headers, &body); + } }); (server, log) } @@ -1032,3 +1039,543 @@ fn media_is_read_only_from_github_hosts_the_conversation_names_and_keeps_the_tok ); assert_eq!(log.lock().unwrap().len(), before); } + +/// Where a node id hangs, as `node(id:)` answers it: the conversation's comments, review, thread +/// and the pull request itself are this one's; `IC_foreign` is another pull request's comment. +fn subject_reply(seen: &Seen) -> (u16, String, Vec) { + let subject = seen.variables()["subject"].as_str().unwrap().to_owned(); + let node = match subject.as_str() { + "PR_node_7" => json!({"__typename": "PullRequest", "id": "PR_node_7"}), + "IC_foreign" => { + json!({"__typename": "IssueComment", "id": subject, "pullRequest": {"id": "PR_other"}}) + } + _ => { + let kind = match &subject[..subject.find('_').unwrap()] { + "IC" => "IssueComment", + "RC" => "PullRequestReviewComment", + "PRR" => "PullRequestReview", + _ => "PullRequestReviewThread", + }; + json!({"__typename": kind, "id": subject, "pullRequest": {"id": "PR_node_7"}}) + } + }; + json_reply(json!({"data": { + "repository": {"pullRequest": {"id": "PR_node_7"}}, + "node": node, + }})) +} + +fn writes_reply(seen: &Seen) -> (u16, String, Vec) { + match seen.operation().as_deref() { + Some("PullRequestSubject") => subject_reply(seen), + Some(_) if seen.body.contains("\"query\":\"mutation") => { + json_reply(json!({"data": {"ok": {"clientMutationId": null}}})) + } + None if seen.line.starts_with("GET /repos/octo/repo/pulls/7 ") => revisions(1), + _ => conversation_reply(seen), + } +} + +fn mutations(log: &Log) -> Vec<(String, Value)> { + log.lock() + .unwrap() + .iter() + .filter(|seen| seen.body.contains("\"query\":\"mutation")) + .map(|seen| (seen.operation().unwrap(), seen.variables())) + .collect() +} + +#[test] +fn conversation_writes_send_their_payloads_once_and_the_conversation_is_read_again() { + let fixture = Fixture::new(); + let store = Store::new(); + let reads = reads(&fixture, &store); + let (_server, log) = serve(fixture, writes_reply); + let count = |operation: &str| { + log.lock() + .unwrap() + .iter() + .filter(|seen| seen.operation().as_deref() == Some(operation)) + .count() + }; + reads.conversation(&key()).unwrap(); + + let actions = [ + PullRequestAction::Comment { + body: "Looks good".into(), + }, + PullRequestAction::Edit { + title: Some("Better title".into()), + body: None, + }, + PullRequestAction::ReplyToThread { + thread_id: "PRRT_1".into(), + body: "Fixed".into(), + }, + PullRequestAction::ResolveThread { + thread_id: "PRRT_1".into(), + resolved: true, + }, + PullRequestAction::EditComment { + comment_id: "RC_1".into(), + body: "Reworded".into(), + }, + PullRequestAction::React { + subject_id: "PRR_2".into(), + content: PullRequestReactionContent::Rocket, + reacted: true, + }, + PullRequestAction::React { + subject_id: "PR_node_7".into(), + content: PullRequestReactionContent::ThumbsUp, + reacted: false, + }, + ]; + for action in &actions { + assert_eq!(reads.act(&key(), action), PullRequestActionResult::Applied); + } + assert_eq!( + mutations(&log), + vec![ + ( + "AddPullRequestComment".into(), + json!({"subjectId": "PR_node_7", "body": "Looks good"}) + ), + ( + "EditPullRequest".into(), + json!({"pullRequestId": "PR_node_7", "title": "Better title"}), + ), + ( + "ReplyToPullRequestThread".into(), + json!({"threadId": "PRRT_1", "body": "Fixed"}) + ), + ( + "ResolvePullRequestThread".into(), + json!({"threadId": "PRRT_1"}) + ), + ( + "EditPullRequestReviewComment".into(), + json!({"commentId": "RC_1", "body": "Reworded"}) + ), + ( + "AddPullRequestReaction".into(), + json!({"subjectId": "PRR_2", "content": "ROCKET"}) + ), + ( + "RemovePullRequestReaction".into(), + json!({"subjectId": "PR_node_7", "content": "THUMBS_UP"}) + ), + ], + "an edit naming only the title leaves the body out, so GitHub keeps it" + ); + let rest_reads = log + .lock() + .unwrap() + .iter() + .filter(|seen| seen.line.starts_with("GET /repos/octo/repo/pulls/7 ")) + .count(); + assert_eq!( + rest_reads, 1, + "the node id comes from one REST read of the pull request" + ); + assert_eq!(count("PullRequestSubject"), 5); + + reads.conversation(&key()).unwrap(); + assert_eq!( + count("PullRequestConversation"), + 2, + "a write drops the conversation it changed" + ); +} + +#[test] +fn a_subject_of_another_pull_request_is_refused_before_any_mutation() { + let fixture = Fixture::new(); + let store = Store::new(); + let reads = reads(&fixture, &store); + let (_server, log) = serve(fixture, writes_reply); + + for action in [ + PullRequestAction::React { + subject_id: "IC_foreign".into(), + content: PullRequestReactionContent::Heart, + reacted: true, + }, + PullRequestAction::EditComment { + comment_id: "IC_foreign".into(), + body: "Not mine to change".into(), + }, + ] { + assert_eq!( + reads.act(&key(), &action), + PullRequestActionResult::Rejected(PullRequestRejection::ForeignSubject) + ); + } + assert_eq!( + reads.act( + &key(), + &PullRequestAction::ReplyToThread { + thread_id: "IC_2".into(), + body: "A comment is not a thread".into(), + } + ), + PullRequestActionResult::Rejected(PullRequestRejection::Invalid) + ); + assert!(mutations(&log).is_empty()); +} + +#[test] +fn labels_are_added_in_one_request_and_removed_one_per_request_until_one_fails() { + let fixture = Fixture::new(); + let store = Store::new(); + let reads = reads(&fixture, &store); + let (_server, log) = serve(fixture, |seen| { + if seen + .line + .starts_with("DELETE /repos/octo/repo/issues/7/labels/needs%2Freview ") + { + return ( + 422, + String::new(), + br#"{"message":"Label does not exist"}"#.to_vec(), + ); + } + match seen.operation().as_deref() { + Some("PullRequestLabelCandidates") => json_reply(json!({"data": {"repository": { + "labels": {"pageInfo": {"hasNextPage": true}, "nodes": [ + {"name": "bug", "color": "d73a4a", "description": "Something is broken"}, + {"name": "docs", "color": "0075ca", "description": null}, + ]}, + "pullRequest": {"labels": {"nodes": [{"name": "docs"}, {"name": "retired"}]}}, + }}})), + _ => json_reply(json!([])), + } + }); + let lines = || { + log.lock() + .unwrap() + .iter() + .filter(|seen| seen.operation().is_none()) + .map(|seen| { + let line = seen.line.rsplit_once(' ').unwrap().0.to_owned(); + (line, seen.body.clone()) + }) + .collect::>() + }; + + let candidates = reads.label_candidates(&key()).unwrap().value; + assert_eq!( + candidates + .labels + .iter() + .map(|label| (label.name.as_str(), label.applied)) + .collect::>(), + vec![("retired", true), ("bug", false), ("docs", true)], + "a label the repository no longer lists leads, so it can still be taken off" + ); + assert!(!candidates.complete); + + assert_eq!( + reads.act( + &key(), + &PullRequestAction::AddLabels { + labels: vec!["bug".into(), "good first issue".into()], + } + ), + PullRequestActionResult::Applied + ); + assert_eq!( + reads.act( + &key(), + &PullRequestAction::RemoveLabels { + labels: vec!["docs".into(), "needs/review".into(), "retired".into()], + } + ), + PullRequestActionResult::Partial { + applied: vec!["docs".into()], + unapplied: vec!["needs/review".into(), "retired".into()], + failure: Box::new(PullRequestActionResult::Rejected( + PullRequestRejection::Refused { + messages: vec!["Label does not exist".into()] + } + )), + } + ); + assert_eq!( + lines(), + vec![ + ( + "POST /repos/octo/repo/issues/7/labels".into(), + r#"{"labels":["bug","good first issue"]}"#.into() + ), + ( + "DELETE /repos/octo/repo/issues/7/labels/docs".into(), + String::new() + ), + ( + "DELETE /repos/octo/repo/issues/7/labels/needs%2Freview".into(), + String::new() + ), + ], + "nothing is sent after the label that failed" + ); + reads.label_candidates(&key()).unwrap(); + assert_eq!( + log.lock() + .unwrap() + .iter() + .filter(|seen| seen.operation().as_deref() == Some("PullRequestLabelCandidates")) + .count(), + 2, + "a label write drops the candidates it changed" + ); +} + +#[test] +fn reviewers_are_offered_without_the_author_and_requested_by_kind() { + let fixture = Fixture::new(); + let store = Store::new(); + let reads = reads(&fixture, &store); + let (_server, log) = serve(fixture, |seen| match seen.operation().as_deref() { + Some("PullRequestReviewerCandidates") => json_reply(json!({"data": {"repository": { + "assignableUsers": {"pageInfo": {"hasNextPage": false}, "nodes": [ + {"login": "octocat", "name": "The Author", "avatarUrl": null}, + {"login": "hubot", "name": null, "avatarUrl": "https://avatars.githubusercontent.com/u/9"}, + {"login": "monalisa", "name": "Mona", "avatarUrl": null}, + ]}, + "pullRequest": {"author": {"login": "octocat"}, "reviewRequests": {"nodes": [ + {"requestedReviewer": {"slug": "core", "name": "Core", "avatarUrl": null}}, + {"requestedReviewer": {"login": "monalisa", "name": "Mona", "avatarUrl": null}}, + ]}}, + }}})), + _ => json_reply(json!({})), + }); + + let candidates = reads.reviewer_candidates(&key()).unwrap().value; + assert_eq!( + candidates + .reviewers + .iter() + .map(|candidate| ( + candidate.reviewer.login.as_str(), + candidate.reviewer.kind, + candidate.requested + )) + .collect::>(), + vec![ + ("core", PullRequestReviewerKind::Team, true), + ("monalisa", PullRequestReviewerKind::User, true), + ("hubot", PullRequestReviewerKind::User, false), + ] + ); + for requested in [true, false] { + assert_eq!( + reads.act( + &key(), + &PullRequestAction::RequestReviewers { + reviewers: vec![ + PullRequestReviewer { + login: "hubot".into(), + kind: PullRequestReviewerKind::User, + }, + PullRequestReviewer { + login: "core".into(), + kind: PullRequestReviewerKind::Team, + }, + ], + requested, + } + ), + PullRequestActionResult::Applied + ); + } + let sent: Vec<_> = log + .lock() + .unwrap() + .iter() + .filter(|seen| seen.operation().is_none()) + .map(|seen| (seen.line.clone(), seen.body.clone())) + .collect(); + let body = r#"{"reviewers":["hubot"],"team_reviewers":["core"]}"#.to_owned(); + assert_eq!( + sent, + vec![ + ( + "POST /repos/octo/repo/pulls/7/requested_reviewers HTTP/1.1".into(), + body.clone() + ), + ( + "DELETE /repos/octo/repo/pulls/7/requested_reviewers HTTP/1.1".into(), + body + ), + ] + ); +} + +fn draft_comment( + id: u64, + revision: &str, + lines: (u32, u32), + side: ReviewSide, +) -> PullRequestReviewDraftComment { + PullRequestReviewDraftComment { + id, + revision: revision.into(), + path: "src/lib.rs".into(), + side, + start_line: lines.0, + end_line: lines.1, + body: format!("Comment {id}"), + } +} + +#[test] +fn a_review_is_one_submission_at_the_head_read_fresh_and_a_moved_head_sends_nothing() { + const MOVED: &str = "3333333333333333333333333333333333333333"; + let fixture = Fixture::new(); + let store = Store::new(); + let reads = reads(&fixture, &store); + let head = Arc::new(Mutex::new(HEAD)); + let current = head.clone(); + let (_server, log) = serve(fixture, move |seen| { + if seen.line.starts_with("GET /repos/octo/repo/pulls/7 ") { + let (status, headers, body) = revisions(1); + let body = String::from_utf8(body) + .unwrap() + .replace(HEAD, ¤t.lock().unwrap()); + return (status, headers, body.into_bytes()); + } + json_reply(json!({"id": 1})) + }); + let reviews = || { + log.lock() + .unwrap() + .iter() + .filter(|seen| { + seen.line + .starts_with("POST /repos/octo/repo/pulls/7/reviews ") + }) + .map(|seen| serde_json::from_str::(&seen.body).unwrap()) + .collect::>() + }; + let comments = [ + draft_comment(1, HEAD, (4, 4), ReviewSide::New), + draft_comment(2, HEAD, (9, 12), ReviewSide::Old), + ]; + // The files were read at HEAD, which the cached revisions still say. + reads.files(&key(), Some(1)).ok(); + *head.lock().unwrap() = MOVED; + + assert_eq!( + reads.submit_review( + &key(), + PullRequestReviewVerdict::RequestChanges, + HEAD, + "Two things", + &comments + ), + PullRequestActionResult::Rejected(PullRequestRejection::StaleHead { head: MOVED.into() }), + "a head that moved after the files were read is seen before anything is sent" + ); + assert!(reviews().is_empty()); + + let moved = [ + draft_comment(1, HEAD, (4, 4), ReviewSide::New), + draft_comment(2, MOVED, (9, 12), ReviewSide::Old), + ]; + assert_eq!( + reads.submit_review( + &key(), + PullRequestReviewVerdict::RequestChanges, + MOVED, + "Two things", + &moved + ), + PullRequestActionResult::Rejected(PullRequestRejection::StaleHead { head: MOVED.into() }), + "a comment on an older commit's lines is not sent at the new head" + ); + assert!(reviews().is_empty()); + + let comments = [ + draft_comment(1, MOVED, (4, 4), ReviewSide::New), + draft_comment(2, MOVED, (9, 12), ReviewSide::Old), + ]; + assert_eq!( + reads.submit_review( + &key(), + PullRequestReviewVerdict::RequestChanges, + MOVED, + "Two things", + &comments + ), + PullRequestActionResult::Applied + ); + assert_eq!( + reviews(), + vec![json!({ + "commit_id": MOVED, + "event": "REQUEST_CHANGES", + "body": "Two things", + "comments": [ + {"path": "src/lib.rs", "line": 4, "side": "RIGHT", "body": "Comment 1"}, + {"path": "src/lib.rs", "start_line": 9, "start_side": "LEFT", "line": 12, + "side": "LEFT", "body": "Comment 2"}, + ], + })] + ); +} + +#[test] +fn a_write_left_unanswered_is_uncertain_and_never_sent_again() { + let fixture = Fixture::new(); + let store = Store::new(); + let reads = reads(&fixture, &store); + let (_server, log) = serve(fixture, |seen| { + if seen + .line + .starts_with("POST /repos/octo/repo/issues/7/labels ") + { + return (502, String::new(), Vec::new()); + } + match seen.operation().as_deref() { + Some("AddPullRequestComment") => (0, String::new(), Vec::new()), + _ => writes_reply(seen), + } + }); + let sent = |prefix: &str| { + log.lock() + .unwrap() + .iter() + .filter(|seen| { + seen.line.starts_with(prefix) || seen.operation().as_deref() == Some(prefix) + }) + .count() + }; + + assert_eq!( + reads.act( + &key(), + &PullRequestAction::Comment { + body: "Once".into() + } + ), + PullRequestActionResult::Uncertain + ); + assert_eq!( + reads.act( + &key(), + &PullRequestAction::AddLabels { + labels: vec!["bug".into()] + } + ), + PullRequestActionResult::Uncertain, + "a server failure may have applied the write" + ); + assert_eq!( + ( + sent("AddPullRequestComment"), + sent("POST /repos/octo/repo/issues/7/labels ") + ), + (1, 1) + ); +} From 897180e404479709b436564844fe4f5121411906 Mon Sep 17 00:00:00 2001 From: Tryanks Date: Fri, 9 Oct 2026 10:53:37 +0800 Subject: [PATCH 03/15] feat: re-anchor a review draft at a moved head, check new pending comments against the diff, and read the account's rights, labels and reviewers --- crates/core/src/pull_request.rs | 99 ++++++++++- crates/protocol/src/lib.rs | 15 +- crates/protocol/src/pull_request.rs | 45 +++++ crates/runtime/src/app/pull_requests.rs | 168 ++++++++++++++---- crates/runtime/src/app/pull_requests_tests.rs | 130 ++++++++++---- crates/runtime/src/pipe.rs | 10 +- .../src/github/pull_request_actions.rs | 140 ++++++++++++++- .../services/src/github/pull_request_reads.rs | 119 +++++++++++-- crates/services/tests/pull_request_reads.rs | 74 ++++++-- 9 files changed, 687 insertions(+), 113 deletions(-) diff --git a/crates/core/src/pull_request.rs b/crates/core/src/pull_request.rs index e379971b1..a07a59d48 100644 --- a/crates/core/src/pull_request.rs +++ b/crates/core/src/pull_request.rs @@ -448,6 +448,9 @@ pub fn groups(links: &[ThreadPullRequestLink]) -> Vec> { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct PullRequestReviewDraft { pub key: PullRequestKey, + /// The head commit the comments are anchored at. + #[serde(default, skip_serializing_if = "String::is_empty")] + pub head: String, #[serde(default, skip_serializing_if = "String::is_empty")] pub body: String, #[serde(default, skip_serializing_if = "Vec::is_empty")] @@ -455,24 +458,43 @@ pub struct PullRequestReviewDraft { /// Never reused, so an edit naming a removed comment cannot reach a later one. #[serde(default)] pub next_id: u64, + /// The last submission got no answer, so it may have been posted; set until the pull request + /// is read again. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub uncertain: bool, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct PullRequestReviewDraftComment { pub id: u64, - /// The head commit whose lines the comment is on. + /// The commit whose text the comment's side shows: the base for the old side, the head for + /// the new. pub revision: String, pub path: String, pub side: crate::session::ReviewSide, pub start_line: u32, pub end_line: u32, pub body: String, + /// False once the head moved and its lines no longer read as they did: kept, never sent. + #[serde(default = "placed", skip_serializing_if = "is_placed")] + pub placed: bool, +} + +fn placed() -> bool { + true +} + +fn is_placed(placed: &bool) -> bool { + *placed } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "type", content = "content", rename_all = "snake_case")] pub enum PullRequestReviewDraftEdit { + /// Lines of the diff read at `head`; a draft anchored at another head takes no new comments + /// until it is moved to this one. AddComment { + head: String, revision: String, path: String, side: crate::session::ReviewSide, @@ -480,16 +502,24 @@ pub enum PullRequestReviewDraftEdit { end_line: u32, body: String, }, + EditComment { + id: u64, + body: String, + }, RemoveComment { id: u64, }, SetBody { body: String, }, + /// Re-anchors the comments at the pull request's current head. The host applies it with + /// [`reanchor_review`], since only it can read whether each comment's lines changed. + MoveToHead, Discard, } -/// Applies an edit to the thread's drafts; false when it changed nothing or was malformed. +/// Applies an edit to the thread's drafts; false when it changed nothing, was malformed, or was +/// [`PullRequestReviewDraftEdit::MoveToHead`]. pub fn edit_review_draft( drafts: &mut Vec, key: &PullRequestKey, @@ -502,9 +532,11 @@ pub fn edit_review_draft( { drafts.push(PullRequestReviewDraft { key: key.clone(), + head: String::new(), body: String::new(), comments: Vec::new(), next_id: 1, + uncertain: false, }); drafts.len() - 1 } @@ -513,6 +545,7 @@ pub fn edit_review_draft( let draft = &mut drafts[index]; let changed = match edit { PullRequestReviewDraftEdit::AddComment { + head, revision, path, side, @@ -520,10 +553,17 @@ pub fn edit_review_draft( end_line, body, } => { - if path.is_empty() || start_line == 0 || start_line > end_line || body.trim().is_empty() + let anchored = draft.comments.is_empty() || draft.head == head; + if !anchored + || head.is_empty() + || path.is_empty() + || start_line == 0 + || start_line > end_line + || body.trim().is_empty() { false } else { + draft.head = head; draft.comments.push(PullRequestReviewDraftComment { id: draft.next_id, revision, @@ -532,11 +572,21 @@ pub fn edit_review_draft( start_line, end_line, body, + placed: true, }); draft.next_id += 1; true } } + PullRequestReviewDraftEdit::EditComment { id, body } => { + match draft.comments.iter_mut().find(|comment| comment.id == id) { + Some(comment) if !body.trim().is_empty() && comment.body != body => { + comment.body = body; + true + } + _ => false, + } + } PullRequestReviewDraftEdit::RemoveComment { id } => { let before = draft.comments.len(); draft.comments.retain(|comment| comment.id != id); @@ -545,6 +595,7 @@ pub fn edit_review_draft( PullRequestReviewDraftEdit::SetBody { body } => { body != std::mem::replace(&mut draft.body, body.clone()) } + PullRequestReviewDraftEdit::MoveToHead => false, PullRequestReviewDraftEdit::Discard => { drafts.remove(index); return true; @@ -556,27 +607,57 @@ pub fn edit_review_draft( changed } -/// Takes what a submission sent out of the draft: its comments by id, and the body only while it -/// still reads as sent, since a body revised meanwhile is new work. +/// Anchors the draft at `head`. `moved` names where each comment's lines are now, or `None` when +/// they changed, which leaves the comment unplaced. +pub fn reanchor_review( + drafts: &mut [PullRequestReviewDraft], + key: &PullRequestKey, + head: &str, + moved: impl Fn(&PullRequestReviewDraftComment) -> Option, +) -> bool { + let Some(draft) = drafts.iter_mut().find(|draft| draft.key == *key) else { + return false; + }; + if draft.head == head { + return false; + } + draft.head = head.to_owned(); + for comment in &mut draft.comments { + match moved(comment) { + Some(revision) if comment.placed => comment.revision = revision, + _ => comment.placed = false, + } + } + true +} + +/// What a submission did to the draft. Once GitHub took it, its comments go by id, and its body +/// only while it still reads as sent, since a body revised meanwhile is new work. An unanswered +/// one marks the draft until the pull request is read again. pub fn submitted_review( drafts: &mut Vec, key: &PullRequestKey, - comments: &[u64], - body: &str, -) { + sent: Option<(&[u64], &str)>, +) -> bool { let Some(index) = drafts.iter().position(|draft| draft.key == *key) else { - return; + return false; }; let draft = &mut drafts[index]; + let Some((comments, body)) = sent else { + draft.uncertain = true; + return true; + }; draft .comments .retain(|comment| !comments.contains(&comment.id)); if draft.body == body { draft.body.clear(); } + draft.uncertain = false; if draft.body.is_empty() && draft.comments.is_empty() { drafts.remove(index); } + true } /// A menu visibility hint only; the host forge adapter validates and canonicalizes the target. diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index fb37431e1..36f8f32e6 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -13,11 +13,12 @@ pub use preview::{PreviewRequest, PreviewResponse}; pub use pull_request::{ MAX_PULL_REQUEST_MEDIA_BYTES, PullRequestAction, PullRequestActionResult, PullRequestActor, PullRequestComment, PullRequestConversation, PullRequestFile, PullRequestFileText, - PullRequestFiles, PullRequestLabelCandidate, PullRequestLabelCandidates, PullRequestMedia, - PullRequestPatch, PullRequestReaction, PullRequestReactionContent, PullRequestRead, - PullRequestReadResponse, PullRequestRejection, PullRequestReviewAnchor, PullRequestReviewState, - PullRequestReviewThread, PullRequestReviewVerdict, PullRequestReviewer, - PullRequestReviewerCandidate, PullRequestReviewerCandidates, PullRequestReviewerKind, + PullRequestFiles, PullRequestLabel, PullRequestLabelCandidate, PullRequestLabelCandidates, + PullRequestMedia, PullRequestPatch, PullRequestPermissions, PullRequestReaction, + PullRequestReactionContent, PullRequestRead, PullRequestReadResponse, PullRequestRejection, + PullRequestReviewAnchor, PullRequestReviewState, PullRequestReviewThread, + PullRequestReviewVerdict, PullRequestReviewer, PullRequestReviewerCandidate, + PullRequestReviewerCandidates, PullRequestReviewerKind, PullRequestReviewerState, PullRequestThreadReplies, PullRequestViewedFiles, PullRequestViewedState, }; pub mod terminal; @@ -119,7 +120,9 @@ pub use wire::{ // and answers PullRequestAction with a typed result; EditPullRequestReviewDraft // edits the host-owned review draft that thread metadata carries as // pull_request_reviews; Query::PullRequest reads label and reviewer -// candidates; a reaction's content is a typed name. +// candidates; a reaction's content is a typed name; the conversation carries +// the account's permissions, labels and reviewers, and comments and threads +// what the account may do to them. pub const PROTOCOL_VERSION: u32 = 10; #[cfg(test)] diff --git a/crates/protocol/src/pull_request.rs b/crates/protocol/src/pull_request.rs index bb9169e2b..987ccf112 100644 --- a/crates/protocol/src/pull_request.rs +++ b/crates/protocol/src/pull_request.rs @@ -150,6 +150,11 @@ pub struct PullRequestComment { /// Set for a review's own body. pub review_state: Option, pub reactions: Vec, + /// GitHub lets the signed-in account change its text: its own, or a maintainer's right. + #[serde(default)] + pub viewer_can_update: bool, + #[serde(default)] + pub viewer_can_react: bool, } /// Where a review thread was left: lines of one side of a file at one commit. @@ -177,6 +182,11 @@ pub struct PullRequestReviewThread { pub total_comments: u64, /// Where [`PullRequestRead::ThreadReplies`] carries on, while replies remain unread. pub replies_after: Option, + #[serde(default)] + pub viewer_can_reply: bool, + /// Whether the signed-in account may resolve it, or unresolve it once resolved. + #[serde(default)] + pub viewer_can_resolve: bool, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -190,6 +200,41 @@ pub struct PullRequestConversation { /// Opaque, and different for each GitHub account the host reads as: a client keys the /// media this conversation shows by it, so no copy outlives an account change. pub account: String, + #[serde(default)] + pub permissions: PullRequestPermissions, + #[serde(default)] + pub labels: Vec, + /// Requested reviewers first, then whoever reviewed without a request outstanding. + #[serde(default)] + pub reviewers: Vec, +} + +/// What the signed-in account may do to the pull request, as GitHub grants it. Per-comment and +/// per-thread rights travel on the comment and thread. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestPermissions { + /// Edit the title and description. + pub update: bool, + /// Empty when the account may not review; the author may only comment. + pub verdicts: Vec, + pub label: bool, + pub request_reviewers: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestLabel { + pub name: String, + /// Hex without the `#`. + pub color: Option, + pub description: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PullRequestReviewerState { + pub reviewer: PullRequestReviewer, + pub avatar_url: Option, + /// `None` while a review is requested and not yet given. + pub verdict: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/crates/runtime/src/app/pull_requests.rs b/crates/runtime/src/app/pull_requests.rs index af113fea0..2353d371f 100644 --- a/crates/runtime/src/app/pull_requests.rs +++ b/crates/runtime/src/app/pull_requests.rs @@ -395,6 +395,16 @@ impl AppState { ) -> Result<(), ProtocolError> { self.linked_pull_request(session_id, &key)?; self.pull_requests.reads.invalidate(&key); + // The pull request is read again, which is what an unanswered review waited for. + if let Some(mut meta) = self.find_meta(session_id) + && let Some(draft) = meta + .pull_request_reviews + .iter_mut() + .find(|draft| draft.key == key && draft.uncertain) + { + draft.uncertain = false; + self.save_pull_request_meta(meta, cx); + } self.request_pull_request_sync(key, cx); Ok(()) } @@ -418,7 +428,7 @@ impl AppState { }) } /// A write to the pull request. Whenever GitHub may have applied it, the sync reads the pull - /// request again; a submitted review leaves the draft only once GitHub took it. + /// request again. A review takes the thread's draft and leaves it only once GitHub took it. pub fn run_pull_request_action( &mut self, session_id: &str, @@ -429,25 +439,19 @@ impl AppState { if let Err(error) = self.linked_pull_request(session_id, &key) { return cx.spawn_background(async move { Err(error) }); } - let draft = self - .find_meta(session_id) - .and_then(|meta| { - meta.pull_request_reviews - .into_iter() - .find(|draft| draft.key == key) - }) - .unwrap_or_else(|| pull_request::PullRequestReviewDraft { - key: key.clone(), - body: String::new(), - comments: Vec::new(), - next_id: 0, - }); + let draft = self.review_draft(session_id, &key); let reads = self.pull_requests.reads.clone(); let review = matches!(action, PullRequestAction::SubmitReview { .. }); let writing = key.clone(); let sent = draft.clone(); let task = cx.unblock(move || match &action { PullRequestAction::SubmitReview { verdict, head } => { + // Comments are anchored at the draft's head, which is what GitHub must still be at. + let head = if sent.comments.is_empty() { + head + } else { + &sent.head + }; reads.submit_review(&writing, *verdict, head, &sent.body, &sent.comments) } action => reads.act(&writing, action), @@ -460,19 +464,16 @@ impl AppState { let applied = outcome == PullRequestActionResult::Applied; let _ = host .enqueue_and_wait(move |state, cx| { - if review - && applied - && let Some(mut meta) = state.find_meta(&id) - { - let sent: Vec<_> = + if review && let Some(mut meta) = state.find_meta(&id) { + let ids: Vec<_> = draft.comments.iter().map(|comment| comment.id).collect(); - pull_request::submitted_review( + if pull_request::submitted_review( &mut meta.pull_request_reviews, &key, - &sent, - &draft.body, - ); - state.save_pull_request_meta(meta, cx); + applied.then_some((ids.as_slice(), draft.body.as_str())), + ) { + state.save_pull_request_meta(meta, cx); + } } state.request_pull_request_sync(key, cx); }) @@ -481,21 +482,124 @@ impl AppState { Ok(CommandResponse::PullRequestAction(outcome)) }) } + fn review_draft( + &self, + session_id: &str, + key: &PullRequestKey, + ) -> pull_request::PullRequestReviewDraft { + self.find_meta(session_id) + .and_then(|meta| { + meta.pull_request_reviews + .into_iter() + .find(|draft| draft.key == *key) + }) + .unwrap_or_else(|| pull_request::PullRequestReviewDraft { + key: key.clone(), + head: String::new(), + body: String::new(), + comments: Vec::new(), + next_id: 0, + uncertain: false, + }) + } + /// A new comment is taken only on lines GitHub would accept at the draft's head, and moving + /// the draft reads where each comment's lines are now; both read the diff first. pub fn edit_pull_request_review_draft( &mut self, session_id: &str, key: PullRequestKey, edit: pull_request::PullRequestReviewDraftEdit, cx: &mut HostCx, - ) -> Result<(), ProtocolError> { - self.linked_pull_request(session_id, &key)?; - let Some(mut meta) = self.find_meta(session_id) else { - return Ok(()); - }; - if pull_request::edit_review_draft(&mut meta.pull_request_reviews, &key, edit) { - self.save_pull_request_meta(meta, cx); + ) -> HostTask> { + use pull_request::PullRequestReviewDraftEdit as Edit; + use tcode_services::github::pull_request_actions::Anchoring; + if let Err(error) = self.linked_pull_request(session_id, &key) { + return cx.spawn_background(async move { Err(error) }); } - Ok(()) + let reads = self.pull_requests.reads.clone(); + let draft = self.review_draft(session_id, &key); + let id = session_id.to_owned(); + let host = cx.clone(); + cx.spawn_background(async move { + let anchor_error = |code: &str, message: &str| ProtocolError { + code: code.into(), + message: message.into(), + }; + let moved = match &edit { + Edit::AddComment { + head, + path, + side, + start_line, + end_line, + .. + } => { + let (reads, key, head, path, side) = ( + reads.clone(), + key.clone(), + head.clone(), + path.clone(), + *side, + ); + let lines = (*start_line, *end_line); + match host + .unblock(move || reads.commentable(&key, &head, &path, side, lines)) + .await + .map_err(read_error)? + { + Anchoring::InDiff => None, + Anchoring::OutsideDiff => { + return Err(anchor_error( + "pull_request_not_in_diff", + "GitHub only accepts comments on lines in the diff.", + )); + } + Anchoring::Moved => { + return Err(anchor_error( + "pull_request_head_changed", + "The pull request's head changed.", + )); + } + } + } + Edit::MoveToHead => { + let (reads, key) = (reads.clone(), key.clone()); + Some( + host.unblock(move || reads.reanchor(&key, &draft.comments)) + .await + .map_err(read_error)?, + ) + } + _ => None, + }; + host.enqueue_and_wait(move |state, cx| { + let Some(mut meta) = state.find_meta(&id) else { + return; + }; + let changed = match moved { + Some((head, moved)) => pull_request::reanchor_review( + &mut meta.pull_request_reviews, + &key, + &head, + |comment| { + moved + .iter() + .find(|(id, _)| *id == comment.id) + .and_then(|(_, revision)| revision.clone()) + }, + ), + None => { + pull_request::edit_review_draft(&mut meta.pull_request_reviews, &key, edit) + } + }; + if changed { + state.save_pull_request_meta(meta, cx); + } + }) + .await + .map_err(|_| failure("Host closed."))?; + Ok(CommandResponse::Unit) + }) } fn pull_request_project_cwd(&self, meta: &SessionMeta) -> PathBuf { self.projects diff --git a/crates/runtime/src/app/pull_requests_tests.rs b/crates/runtime/src/app/pull_requests_tests.rs index 2be515587..f37bcd54d 100644 --- a/crates/runtime/src/app/pull_requests_tests.rs +++ b/crates/runtime/src/app/pull_requests_tests.rs @@ -1092,7 +1092,8 @@ fn reads_answer_only_a_linked_pull_request_and_a_synced_change_reads_it_fresh() } #[test] -fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it() { +fn a_review_draft_is_the_hosts_across_a_restart_and_a_moved_head_keeps_it_until_moved() { + const BASE: &str = "3333333333333333333333333333333333333333"; const OLD: &str = "1111111111111111111111111111111111111111"; const NEW: &str = "2222222222222222222222222222222222222222"; let dir = TestStore::new("tcode-pr-review"); @@ -1103,18 +1104,39 @@ fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it( let responding = model.clone(); let _server = fixture.serve(move |exchange| { let line = exchange.request.lines().next().unwrap().to_owned(); + let diff = exchange.request.contains("application/vnd.github.diff"); let body = String::from_utf8_lossy(&exchange.body).into_owned(); let mut model = responding.lock().unwrap(); model.1.push((line.clone(), body.clone())); - let reply = if line.starts_with("GET /repos/sample/project/pulls/1 ") { - json!({"base": {"sha": "3333333333333333333333333333333333333333"}, - "head": {"sha": model.0}, "changed_files": 1, "node_id": "PR_1"}) + let head = model.0; + // Line 8 is rewritten at the new head; every other line reads the same. + let text = (1..=12) + .map(|line| match (line, head) { + (8, NEW) => "changed".to_owned(), + _ => format!("line {line}"), + }) + .collect::>() + .join("\n"); + let reply = if diff { + "diff --git a/src/lib.rs b/src/lib.rs\nindex 1111111..2222222 100644\n--- a/src/lib.rs\n+++ b/src/lib.rs\n@@ -1,10 +1,10 @@\n line 1\n".to_owned() + } else if line.starts_with("GET /repos/sample/project/pulls/1 ") { + json!({"base": {"sha": BASE}, "head": {"sha": head}, "changed_files": 1, "node_id": "PR_1"}) + .to_string() + } else if line.starts_with("GET /repos/sample/project/contents/src/lib.rs?ref=") { + if line.contains(NEW) { + text + } else { + (1..=12) + .map(|line| format!("line {line}")) + .collect::>() + .join("\n") + } } else if body.contains("PullRequestSummaries") { - json!({"data": {"s0": {"pullRequest": pr(1, "OPEN", false)}}}) + json!({"data": {"s0": {"pullRequest": pr(1, "OPEN", false)}}}).to_string() } else { - json!({"id": 1}) + json!({"id": 1}).to_string() }; - exchange.reply(200, "", &serde_json::to_vec(&reply).unwrap()); + exchange.reply(200, "", reply.as_bytes()); }); let key = PullRequestKey::new("github.com", "sample/project", 1); let mut cx = TestAppContext::default(); @@ -1142,7 +1164,7 @@ fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it( _ => None, }) { - return result.unwrap(); + return result; } assert!( std::time::Instant::now() < deadline, @@ -1157,6 +1179,7 @@ fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it( }; let add = |line: u32| { edit(PullRequestReviewDraftEdit::AddComment { + head: OLD.into(), revision: OLD.into(), path: "src/lib.rs".into(), side: tcode_core::session::ReviewSide::New, @@ -1165,20 +1188,24 @@ fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it( body: format!("Line {line}"), }) }; - command(&state, &mut cx, add(3)); - command(&state, &mut cx, add(8)); - command( - &state, - &mut cx, - edit(PullRequestReviewDraftEdit::RemoveComment { id: 1 }), + let drafts = |state: &TestEntity| { + state.read(|state| state.find_meta("active").unwrap().pull_request_reviews) + }; + command(&state, &mut cx, add(3)).unwrap(); + command(&state, &mut cx, add(8)).unwrap(); + assert_eq!( + command(&state, &mut cx, add(11)).unwrap_err().code, + "pull_request_not_in_diff", + "a line outside every hunk is refused before GitHub would refuse the review" ); command( &state, &mut cx, edit(PullRequestReviewDraftEdit::SetBody { - body: "One thing".into(), + body: "Two things".into(), }), - ); + ) + .unwrap(); // A restart: a fresh host over the same data. state @@ -1190,11 +1217,15 @@ fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it( assert_eq!(draft.len(), 1); assert_eq!( ( + draft[0].head.as_str(), draft[0].body.as_str(), - draft[0].comments.len(), - draft[0].comments[0].id + draft[0] + .comments + .iter() + .map(|comment| comment.end_line) + .collect::>() ), - ("One thing", 1, 2) + (OLD, "Two things", vec![3, 8]) ); let state = cx.new_entity(TestClientState::new(store.clone())); state.update(&mut cx, |state, _| { @@ -1216,35 +1247,70 @@ fn a_review_draft_is_the_hosts_across_a_restart_and_a_stale_submission_keeps_it( .1 .iter() .filter(|(line, _)| line.starts_with("POST /repos/sample/project/pulls/1/reviews ")) - .count() + .map(|(_, body)| serde_json::from_str::(body).unwrap()) + .collect::>() }; model.lock().unwrap().0 = NEW; assert_eq!( - command(&state, &mut cx, submit(OLD)), + command(&state, &mut cx, submit(OLD)).unwrap(), CommandResponse::PullRequestAction(PullRequestActionResult::Rejected( tcode_protocol::PullRequestRejection::StaleHead { head: NEW.into() } )) ); - assert_eq!(reviews(), 0); + assert!(reviews().is_empty()); assert_eq!( - state.read(|state| state.find_meta("active").unwrap().pull_request_reviews), + drafts(&state), draft, - "the draft written against the old head is kept whole" + "a stale submission keeps the draft whole" ); - model.lock().unwrap().0 = OLD; + command( + &state, + &mut cx, + edit(PullRequestReviewDraftEdit::MoveToHead), + ) + .unwrap(); + let moved = drafts(&state); + assert_eq!(moved[0].head, NEW); + assert_eq!( + moved[0] + .comments + .iter() + .map(|comment| (comment.end_line, comment.placed, comment.revision.as_str())) + .collect::>(), + vec![(3, true, NEW), (8, false, OLD)], + "a comment whose line reads the same moves; one whose line changed is kept unplaced" + ); assert_eq!( - command(&state, &mut cx, submit(OLD)), + command(&state, &mut cx, submit(NEW)).unwrap(), + CommandResponse::PullRequestAction(PullRequestActionResult::Rejected( + tcode_protocol::PullRequestRejection::Invalid + )), + "an unplaced comment is never sent" + ); + let unplaced = moved[0].comments[1].id; + command( + &state, + &mut cx, + edit(PullRequestReviewDraftEdit::RemoveComment { id: unplaced }), + ) + .unwrap(); + assert_eq!( + command(&state, &mut cx, submit(NEW)).unwrap(), CommandResponse::PullRequestAction(PullRequestActionResult::Applied) ); - assert_eq!(reviews(), 1); + assert_eq!( + reviews(), + vec![json!({ + "commit_id": NEW, + "event": "COMMENT", + "body": "Two things", + "comments": [{"path": "src/lib.rs", "line": 3, "side": "RIGHT", "body": "Line 3"}], + })] + ); assert!( - state.read(|state| state - .find_meta("active") - .unwrap() - .pull_request_reviews - .is_empty()), + drafts(&state).is_empty(), "a submitted review leaves the draft" ); cx.run_until(|state| !state.pull_requests.syncing && state.pull_requests.requested.is_empty()); diff --git a/crates/runtime/src/pipe.rs b/crates/runtime/src/pipe.rs index a2ab50644..b90d01218 100644 --- a/crates/runtime/src/pipe.rs +++ b/crates/runtime/src/pipe.rs @@ -479,10 +479,12 @@ fn dispatch_command(app: &mut AppState, cx: &mut HostCx, command: Command) -> Co key, edit, } => { - return CommandOutcome::Immediate( - app.edit_pull_request_review_draft(&session_id, key, edit, cx) - .map(|()| CommandResponse::Unit), - ); + return CommandOutcome::Pending(app.edit_pull_request_review_draft( + &session_id, + key, + edit, + cx, + )); } Command::UnlinkPullRequest { session_id, key } => { app.unlink_pull_request(&session_id, &key, cx); diff --git a/crates/services/src/github/pull_request_actions.rs b/crates/services/src/github/pull_request_actions.rs index 483d879ff..5d91fe49d 100644 --- a/crates/services/src/github/pull_request_actions.rs +++ b/crates/services/src/github/pull_request_actions.rs @@ -4,7 +4,9 @@ use super::{ CredentialError, GitHubError, RequestOptions, RestRequest, graphql::Document, - pull_request_reads::{PullRequestReads, Reader, is_revision, percent_encode, reaction_name}, + pull_request_reads::{ + MAX_PAGES, PullRequestReads, Reader, is_revision, percent_encode, reaction_name, + }, }; use serde_json::{Value, json}; use std::{collections::BTreeMap, time::UNIX_EPOCH}; @@ -13,10 +15,55 @@ use tcode_core::{ session::ReviewSide, }; use tcode_protocol::{ - PullRequestAction, PullRequestActionResult as Outcome, PullRequestRejection as Rejection, - PullRequestReviewVerdict, PullRequestReviewerKind, + PullRequestAction, PullRequestActionResult as Outcome, PullRequestFile, PullRequestFileText, + PullRequestPatch, PullRequestRejection as Rejection, PullRequestReviewVerdict, + PullRequestReviewerKind, }; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Anchoring { + InDiff, + OutsideDiff, + /// The pull request is at another head now. + Moved, +} + +/// Whether the lines fall inside one hunk of the file on that side; GitHub refuses a review +/// comment anywhere else. +fn in_hunks( + files: &[PullRequestFile], + path: &str, + side: ReviewSide, + (start, end): (u32, u32), +) -> bool { + let Some(PullRequestPatch::Hunks(hunks)) = files + .iter() + .find(|file| file.path == path) + .map(|file| &file.patch) + else { + return false; + }; + hunks + .lines() + .filter_map(|line| line.strip_prefix("@@ ")) + .any(|header| { + let mut ranges = header.split_whitespace(); + let range = match side { + ReviewSide::Old => ranges.next().and_then(|range| range.strip_prefix('-')), + ReviewSide::New => ranges.nth(1).and_then(|range| range.strip_prefix('+')), + }; + let Some((first, count)) = + range.map(|range| range.split_once(',').unwrap_or((range, "1"))) + else { + return false; + }; + let (Ok(first), Ok(count)) = (first.parse::(), count.parse::()) else { + return false; + }; + count > 0 && first <= start && end < first + count + }) +} + /// The reads a write may change. #[derive(Clone, Copy)] enum Affects { @@ -350,8 +397,8 @@ impl PullRequestReads { } /// The whole review in one request, so nothing of it is visible until the verdict is sent. - /// The head is read fresh first: a review read at `head` is not sent once the pull request - /// has moved past it, nor are comments on another commit's lines. + /// The head is read fresh first: a review anchored at `head` is not sent once the pull + /// request has moved past it, and an unplaced comment is never sent. pub fn submit_review( &self, key: &PullRequestKey, @@ -376,6 +423,7 @@ impl PullRequestReads { comments: &[PullRequestReviewDraftComment], ) -> Result { if !is_revision(head) + || comments.iter().any(|comment| !comment.placed) || (verdict != PullRequestReviewVerdict::Approve && blank(body) && comments.is_empty()) { return Err(Rejection::Invalid); @@ -383,7 +431,7 @@ impl PullRequestReads { let reader = self.reader(key).map_err(rejection)?; self.revisions.invalidate(key); let current = self.revisions(&reader).map_err(rejection)?.head.clone(); - if current != head || comments.iter().any(|comment| comment.revision != current) { + if current != head { return Err(Rejection::StaleHead { head: current }); } let event = match verdict { @@ -420,6 +468,86 @@ impl PullRequestReads { )) } + /// The changed files at the pull request's current revisions, every page of them. + fn diff( + &self, + key: &PullRequestKey, + ) -> Result<(String, String, Vec), GitHubError> { + let first = self.files(key, None)?.value; + let (base, head) = (first.base.clone(), first.head.clone()); + let mut files = first.files.clone(); + let mut next = first.next_page; + while let Some(page) = next.filter(|page| (*page as usize) <= MAX_PAGES) { + let more = self.files(key, Some(page))?.value; + files.extend(more.files.iter().cloned()); + next = more.next_page; + } + Ok((base, head, files)) + } + + /// Whether GitHub takes a review comment on these lines: the pull request is still at + /// `head`, and they lie inside one of the file's hunks on that side. + pub fn commentable( + &self, + key: &PullRequestKey, + head: &str, + path: &str, + side: ReviewSide, + lines: (u32, u32), + ) -> Result { + let (_, current, files) = self.diff(key)?; + if current != head { + return Ok(Anchoring::Moved); + } + Ok(if in_hunks(&files, path, side, lines) { + Anchoring::InDiff + } else { + Anchoring::OutsideDiff + }) + } + + /// The pull request's current head, and where each comment's lines are at it: the revision + /// its side now shows when the lines read the same and are still in the diff, else `None`. + pub fn reanchor( + &self, + key: &PullRequestKey, + comments: &[PullRequestReviewDraftComment], + ) -> Result<(String, Vec<(u64, Option)>), GitHubError> { + let (base, head, files) = self.diff(key)?; + let lines = |revision: &str, comment: &PullRequestReviewDraftComment| { + let text = self.file_text(key, revision, &comment.path)?.value; + Ok::<_, GitHubError>(match &*text { + PullRequestFileText::Text(text) => { + let lines: Vec<_> = text.lines().collect(); + lines + .get(comment.start_line as usize - 1..comment.end_line as usize) + .map(|lines| lines.join("\n")) + } + _ => None, + }) + }; + let mut moved = Vec::new(); + for comment in comments { + let revision = match comment.side { + ReviewSide::Old => &base, + ReviewSide::New => &head, + }; + let kept = comment.placed + && in_hunks( + &files, + &comment.path, + comment.side, + (comment.start_line, comment.end_line), + ) + && (comment.revision == *revision || { + let before = lines(&comment.revision, comment)?; + before.is_some() && before == lines(revision, comment)? + }); + moved.push((comment.id, kept.then(|| revision.clone()))); + } + Ok((head, moved)) + } + fn written(&self, key: &PullRequestKey, affects: Affects, outcome: &Outcome) { if matches!(outcome, Outcome::Rejected(_)) { return; diff --git a/crates/services/src/github/pull_request_reads.rs b/crates/services/src/github/pull_request_reads.rs index 0a0317dc3..9f15dde72 100644 --- a/crates/services/src/github/pull_request_reads.rs +++ b/crates/services/src/github/pull_request_reads.rs @@ -19,11 +19,13 @@ use std::{ use tcode_core::{pull_request::PullRequestKey, session::ReviewSide}; use tcode_protocol::{ PullRequestActor, PullRequestComment, PullRequestConversation, PullRequestFile, - PullRequestFileText, PullRequestFiles, PullRequestLabelCandidate, PullRequestLabelCandidates, - PullRequestMedia, PullRequestPatch, PullRequestReaction, PullRequestReactionContent, - PullRequestReviewAnchor, PullRequestReviewState, PullRequestReviewThread, PullRequestReviewer, + PullRequestFileText, PullRequestFiles, PullRequestLabel, PullRequestLabelCandidate, + PullRequestLabelCandidates, PullRequestMedia, PullRequestPatch, PullRequestPermissions, + PullRequestReaction, PullRequestReactionContent, PullRequestReviewAnchor, + PullRequestReviewState, PullRequestReviewThread, PullRequestReviewVerdict, PullRequestReviewer, PullRequestReviewerCandidate, PullRequestReviewerCandidates, PullRequestReviewerKind, - PullRequestThreadReplies, PullRequestViewedFiles, PullRequestViewedState, + PullRequestReviewerState, PullRequestThreadReplies, PullRequestViewedFiles, + PullRequestViewedState, }; const READ_TTL: Duration = Duration::from_secs(60); @@ -36,7 +38,7 @@ const DIFF_BYTES: usize = 8 * 1024 * 1024; const FILE_BYTES: usize = 1024 * 1024; const FILES_PER_PAGE: usize = 100; /// Past this many requests a list is reported incomplete rather than read on. -const MAX_PAGES: usize = 10; +pub(super) const MAX_PAGES: usize = 10; const VIEWED_PAGES: usize = 5; const EMPTY_BLOB: &str = "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"; /// Pull requests whose node id is remembered; an id never changes, so this bounds memory only. @@ -44,6 +46,7 @@ const NODE_IDS: usize = 128; const ACTOR: &str = "author { login avatarUrl(size: 64) }"; const REACTIONS: &str = "reactionGroups { content viewerHasReacted reactors { totalCount } }"; +const VIEWER: &str = "viewerCanUpdate viewerCanReact"; #[derive(Debug, Clone)] pub(super) struct Revisions { @@ -248,12 +251,13 @@ impl Reader<'_> { } fn conversation(&self) -> Result<(PullRequestConversation, Duration), GitHubError> { - let comment = format!("id body createdAt lastEditedAt url {ACTOR} {REACTIONS}"); + let comment = format!("id body createdAt lastEditedAt url {ACTOR} {REACTIONS} {VIEWER}"); let query = format!( - "query PullRequestConversation($owner: String!, $name: String!, $number: Int!, $head: Boolean!, $withComments: Boolean!, $commentsAfter: String, $withReviews: Boolean!, $reviewsAfter: String) {{ repository(owner: $owner, name: $name) {{ pullRequest(number: $number) {{ ... on PullRequest @include(if: $head) {{ {comment} mergedAt }} comments(first: 100, after: $commentsAfter) @include(if: $withComments) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ {comment} }} }} reviews(first: 100, after: $reviewsAfter) @include(if: $withReviews) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ {comment} state submittedAt }} }} }} }} }}" + "query PullRequestConversation($owner: String!, $name: String!, $number: Int!, $head: Boolean!, $withComments: Boolean!, $commentsAfter: String, $withReviews: Boolean!, $reviewsAfter: String) {{ repository(owner: $owner, name: $name) {{ viewerPermission @include(if: $head) pullRequest(number: $number) {{ ... on PullRequest @include(if: $head) {{ {comment} mergedAt viewerDidAuthor labels(first: 100) {{ nodes {{ name color description }} }} reviewRequests(first: 100) {{ nodes {{ requestedReviewer {{ ... on User {{ login avatarUrl(size: 64) }} ... on Bot {{ login avatarUrl(size: 64) }} ... on Team {{ slug }} }} }} }} latestReviews(first: 100) {{ nodes {{ state {ACTOR} }} }} }} comments(first: 100, after: $commentsAfter) @include(if: $withComments) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ {comment} }} }} reviews(first: 100, after: $reviewsAfter) @include(if: $withReviews) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ {comment} state submittedAt }} }} }} }} }}" ); let mut description = None; let mut merged = false; + let mut head = Value::Null; let mut comments = Vec::new(); let (mut more_comments, mut more_reviews) = (true, true); let (mut comments_after, mut reviews_after) = (Value::Null, Value::Null); @@ -276,6 +280,7 @@ impl Reader<'_> { if page == 0 { merged = pr["mergedAt"].is_string(); description = Some(comment_from(pr, None).ok_or(GitHubError::InvalidResponse)?); + head = response["data"]["repository"].clone(); } if more_comments { comments.extend(nodes(&pr["comments"]).filter_map(|raw| comment_from(raw, None))); @@ -295,7 +300,7 @@ impl Reader<'_> { comments.sort_by(|left, right| left.created_at.cmp(&right.created_at)); let thread_query = format!( - "query PullRequestReviewThreads($owner: String!, $name: String!, $number: Int!, $cursor: String) {{ repository(owner: $owner, name: $name) {{ pullRequest(number: $number) {{ reviewThreads(first: 100, after: $cursor) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ id isResolved isOutdated path line startLine originalLine originalStartLine diffSide startDiffSide comments(first: 10) {{ totalCount pageInfo {{ hasNextPage endCursor }} nodes {{ {comment} diffHunk commit {{ oid }} originalCommit {{ oid }} }} }} }} }} }} }} }}" + "query PullRequestReviewThreads($owner: String!, $name: String!, $number: Int!, $cursor: String) {{ repository(owner: $owner, name: $name) {{ pullRequest(number: $number) {{ reviewThreads(first: 100, after: $cursor) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ id isResolved isOutdated path line startLine originalLine originalStartLine diffSide startDiffSide viewerCanReply viewerCanResolve viewerCanUnresolve comments(first: 10) {{ totalCount pageInfo {{ hasNextPage endCursor }} nodes {{ {comment} diffHunk commit {{ oid }} originalCommit {{ oid }} }} }} }} }} }} }} }}" ); let mut threads = Vec::new(); let mut cursor = Value::Null; @@ -313,13 +318,45 @@ impl Reader<'_> { None => break, } } + let description = description.ok_or(GitHubError::InvalidResponse)?; + let pr = &head["pullRequest"]; + // Triage may label; write may also ask for reviews (study: provider permission map). + let role = head["viewerPermission"].as_str().unwrap_or_default(); + let writes = matches!(role, "ADMIN" | "MAINTAIN" | "WRITE"); + let permissions = PullRequestPermissions { + update: description.viewer_can_update, + // GitHub refuses an approval or a change request from the author. + verdicts: if pr["viewerDidAuthor"].as_bool() == Some(true) { + vec![PullRequestReviewVerdict::Comment] + } else { + vec![ + PullRequestReviewVerdict::Comment, + PullRequestReviewVerdict::Approve, + PullRequestReviewVerdict::RequestChanges, + ] + }, + label: writes || role == "TRIAGE", + request_reviewers: writes, + }; + let labels = nodes(&pr["labels"]) + .filter_map(|label| { + Some(PullRequestLabel { + name: text(label, "name")?, + color: text(label, "color"), + description: text(label, "description"), + }) + }) + .collect(); Ok(( PullRequestConversation { - description: description.ok_or(GitHubError::InvalidResponse)?, + description, comments, threads, complete, account: super::digest(&self.account)[..16].to_owned(), + permissions, + labels, + reviewers: reviewer_states(pr), }, if merged { MERGED_TTL } else { READ_TTL }, )) @@ -331,7 +368,7 @@ impl Reader<'_> { after: &str, ) -> Result { let query = format!( - "query PullRequestThreadReplies($owner: String!, $name: String!, $number: Int!, $thread: ID!, $cursor: String) {{ repository(owner: $owner, name: $name) {{ pullRequest(number: $number) {{ id }} }} node(id: $thread) {{ ... on PullRequestReviewThread {{ pullRequest {{ id }} comments(first: 100, after: $cursor) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ id body createdAt lastEditedAt url {ACTOR} {REACTIONS} }} }} }} }} }}" + "query PullRequestThreadReplies($owner: String!, $name: String!, $number: Int!, $thread: ID!, $cursor: String) {{ repository(owner: $owner, name: $name) {{ pullRequest(number: $number) {{ id }} }} node(id: $thread) {{ ... on PullRequestReviewThread {{ pullRequest {{ id }} comments(first: 100, after: $cursor) {{ pageInfo {{ hasNextPage endCursor }} nodes {{ id body createdAt lastEditedAt url {ACTOR} {REACTIONS} {VIEWER} }} }} }} }} }}" ); let response = self.query( "PullRequestThreadReplies", @@ -535,7 +572,8 @@ impl PullRequestReads { let reader = self.reader(key)?; let revisions = self.revisions(&reader)?; self.files.read( - reader.read_key(format!("files {page:?}")), + // Files are a head's: a moved head is a different read. + reader.read_key(format!("files {} {page:?}", revisions.head)), || { let files = match page { None => reader.whole_diff(&revisions)?, @@ -893,6 +931,8 @@ fn comment_from( edited_at: text("lastEditedAt"), url: text("url").filter(|url| !url.is_empty()), review_state, + viewer_can_update: raw["viewerCanUpdate"].as_bool() == Some(true), + viewer_can_react: raw["viewerCanReact"].as_bool() == Some(true), reactions: raw["reactionGroups"] .as_array() .into_iter() @@ -987,9 +1027,66 @@ fn thread_from(raw: &Value) -> Option { .collect(), total_comments: raw["comments"]["totalCount"].as_u64().unwrap_or(0), replies_after: next_cursor(&raw["comments"]), + viewer_can_reply: raw["viewerCanReply"].as_bool() == Some(true), + viewer_can_resolve: raw[if raw["isResolved"].as_bool() == Some(true) { + "viewerCanUnresolve" + } else { + "viewerCanResolve" + }] + .as_bool() + == Some(true), }) } +/// Requests lead, as GitHub's sidebar lists them; a latest review by someone not asked again +/// follows with its verdict. A dismissed or pending review is no verdict. +fn reviewer_states(pr: &Value) -> Vec { + let mut reviewers: Vec = Vec::new(); + for node in nodes(&pr["reviewRequests"]) { + let raw = &node["requestedReviewer"]; + let reviewer = match (text(raw, "slug"), text(raw, "login")) { + (Some(slug), _) => PullRequestReviewer { + login: slug, + kind: PullRequestReviewerKind::Team, + }, + (None, Some(login)) => PullRequestReviewer { + login, + kind: PullRequestReviewerKind::User, + }, + (None, None) => continue, + }; + reviewers.push(PullRequestReviewerState { + reviewer, + avatar_url: text(raw, "avatarUrl"), + verdict: None, + }); + } + for node in nodes(&pr["latestReviews"]) { + let verdict = match node["state"].as_str() { + Some("APPROVED") => PullRequestReviewState::Approved, + Some("CHANGES_REQUESTED") => PullRequestReviewState::ChangesRequested, + Some("COMMENTED") => PullRequestReviewState::Commented, + _ => continue, + }; + let Some(login) = text(&node["author"], "login") else { + continue; + }; + let reviewer = PullRequestReviewer { + login, + kind: PullRequestReviewerKind::User, + }; + if reviewers.iter().any(|known| known.reviewer == reviewer) { + continue; + } + reviewers.push(PullRequestReviewerState { + reviewer, + avatar_url: text(&node["author"], "avatarUrl"), + verdict: Some(verdict), + }); + } + reviewers +} + fn listed_file(row: &Value) -> Option { let path = row["filename"].as_str()?.to_owned(); let status = row["status"].as_str()?; diff --git a/crates/services/tests/pull_request_reads.rs b/crates/services/tests/pull_request_reads.rs index 0cb28bac7..4a6faa552 100644 --- a/crates/services/tests/pull_request_reads.rs +++ b/crates/services/tests/pull_request_reads.rs @@ -9,8 +9,9 @@ use tcode_core::{ }; use tcode_protocol::{ PullRequestAction, PullRequestActionResult, PullRequestFileText, PullRequestMedia, - PullRequestPatch, PullRequestReactionContent, PullRequestRejection, PullRequestReviewAnchor, - PullRequestReviewVerdict, PullRequestReviewer, PullRequestReviewerKind, PullRequestViewedState, + PullRequestPatch, PullRequestPermissions, PullRequestReactionContent, PullRequestRejection, + PullRequestReviewAnchor, PullRequestReviewState, PullRequestReviewVerdict, PullRequestReviewer, + PullRequestReviewerKind, PullRequestViewedState, }; use tcode_services::github::{GitHubApi, GitHubError, pull_request_reads::PullRequestReads}; @@ -407,8 +408,19 @@ fn conversation_reply(seen: &Seen) -> (u16, String, Vec) { }) }; match seen.operation().as_deref() { - Some("PullRequestConversation") => { - json_reply(json!({"data": {"repository": {"pullRequest": { + Some("PullRequestConversation") => json_reply( + json!({"data": {"repository": {"viewerPermission": "TRIAGE", "pullRequest": { + "viewerCanUpdate": true, "viewerDidAuthor": true, + "labels": {"nodes": [{"name": "bug", "color": "d73a4a", "description": null}]}, + "reviewRequests": {"nodes": [ + {"requestedReviewer": {"slug": "core"}}, + {"requestedReviewer": {"login": "monalisa", "avatarUrl": null}}, + ]}, + "latestReviews": {"nodes": [ + {"state": "CHANGES_REQUESTED", "author": {"login": "monalisa"}}, + {"state": "APPROVED", "author": {"login": "hubot", "avatarUrl": null}}, + {"state": "DISMISSED", "author": {"login": "octocat"}}, + ]}, "id": "PR_7", "body": "Screenshot: ![shot](https://github.com/user-attachments/assets/abc-123)\n