diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index 05796283b9b51483b022fa4332335968a72b67a1..81033e2e0d9458141dd29dcdf091f9d2e611e0b5 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -97,6 +97,40 @@ describe("pull request list decoding", () => { ]); }); + it("takes the verdict from the latest reviews when GitHub summarizes none, as for a bot's approval", () => { + const batch = expectSuccess( + decodePullRequestListJson( + listJson([ + { + reviewDecision: null, + latestReviews: [{ author: { login: "macroscopeapp" }, state: "APPROVED" }], + }, + { + reviewDecision: "REVIEW_REQUIRED", + latestReviews: [ + { author: { login: "octocat" }, state: "APPROVED" }, + { author: { login: "hubot" }, state: "CHANGES_REQUESTED" }, + ], + }, + { + reviewDecision: "APPROVED", + latestReviews: [{ author: { login: "hubot" }, state: "CHANGES_REQUESTED" }], + }, + { + reviewDecision: null, + latestReviews: [{ author: { login: "octocat" }, state: "COMMENTED" }], + }, + ]), + ), + ); + expect(batch.items.map((entry) => entry.reviewDecision)).toEqual([ + "approved", + "changes-requested", + "approved", + null, + ]); + }); + it("rolls the head commit's checks up to the one word a row has space for", () => { const batch = expectSuccess( decodePullRequestListJson( diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 8a1434224ad2a13d35b5a88044a4a17d79368c5f..eaaa4e34da84133e72b9fc35a1d84f212dee235d 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -68,6 +68,12 @@ const RawReviewRequestSchema = Schema.Struct({ name: Schema.optional(Schema.NullOr(Schema.String)), }); +/** One reviewer's most recent review: the state is all the verdict needs, the author is for who. */ +const RawLatestReviewSchema = Schema.Struct({ + author: Schema.optional(Schema.NullOr(RawActorSchema)), + state: Schema.optional(Schema.NullOr(Schema.String)), +}); + const RawCheckSchema = Schema.Struct({ __typename: Schema.optional(Schema.String), name: Schema.optional(Schema.NullOr(Schema.String)), @@ -106,6 +112,7 @@ const RawListItemSchema = Schema.Struct({ updatedAt: Schema.String, mergedAt: Schema.optional(Schema.NullOr(Schema.String)), reviewRequests: Schema.optional(Schema.Array(RawReviewRequestSchema)), + latestReviews: Schema.optional(Schema.NullOr(Schema.Array(RawLatestReviewSchema))), labels: Schema.optional(Schema.Array(RawLabelSchema)), /** * Every check of the head commit, which is the only rollup `gh pr list --json` can give: there @@ -143,6 +150,9 @@ const RawSearchItemSchema = Schema.Struct({ isDraft: Schema.optional(Schema.Boolean), mergeable: Schema.optional(Schema.NullOr(Schema.String)), reviewDecision: Schema.optional(Schema.NullOr(Schema.String)), + latestReviews: Schema.optional( + Schema.NullOr(Schema.Struct({ nodes: Schema.Array(Schema.NullOr(RawLatestReviewSchema)) })), + ), createdAt: Schema.String, updatedAt: Schema.String, mergedAt: Schema.optional(Schema.NullOr(Schema.String)), @@ -514,13 +524,7 @@ const RawReviewThreadsSchema = Schema.Struct({ ), ), latestReviews: Schema.optional( - Schema.NullOr( - Schema.Struct({ - nodes: Schema.Array( - Schema.Struct({ author: Schema.optional(Schema.NullOr(RawActorSchema)) }), - ), - }), - ), + Schema.NullOr(Schema.Struct({ nodes: Schema.Array(RawLatestReviewSchema) })), ), reviewDismissals: Schema.optional( Schema.NullOr( @@ -698,7 +702,7 @@ export function decodeActorAvatarsJson( } export const PULL_REQUEST_LIST_JSON_FIELDS = - "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,labels,statusCheckRollup"; + "number,title,url,author,headRefName,baseRefName,state,isDraft,mergeable,reviewDecision,additions,deletions,createdAt,updatedAt,mergedAt,reviewRequests,latestReviews,labels,statusCheckRollup"; export const PULL_REQUEST_DETAIL_JSON_FIELDS = `${PULL_REQUEST_LIST_JSON_FIELDS},body,changedFiles,closedAt,isCrossRepository,headRepositoryOwner,headRefOid,autoMergeRequest`; @@ -817,6 +821,7 @@ export function pullRequestSearchGraphQlQuery(rows: number, includeStacks = fals isDraft mergeable reviewDecision + latestReviews(first: 20) { nodes { state author { login } } } createdAt updatedAt mergedAt @@ -891,7 +896,7 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin } } latestReviews(first: 50) { - nodes { author { __typename login avatarUrl } } + nodes { state author { __typename login avatarUrl } } } reviewDismissals: timelineItems(itemTypes: [REVIEW_DISMISSED_EVENT], first: ${GRAPHQL_PAGE_SIZE}) { pageInfo { hasNextPage endCursor } @@ -1325,6 +1330,35 @@ function toMergeMethod(value: string | null | undefined): PullRequestMergeMethod } } +/** + * GitHub's own `reviewDecision` counts only reviews that satisfy the branch rules, so an + * approval from an app (a review bot) or from anyone without the required permission leaves it + * empty. The reviewers still said something, and a row should show it: when GitHub reports no + * verdict, the latest review per reviewer decides, changes requested outranking approval. + */ +function toReviewDecisionWithReviews( + value: string | null | undefined, + // `gh pr list` hands the reviews as an array; the GraphQL reads hand a connection. + latestReviews: + | ReadonlyArray> + | { readonly nodes: ReadonlyArray> } + | null + | undefined, +): PullRequestReviewDecision | null { + const summarized = toReviewDecision(value); + if (summarized === "approved" || summarized === "changes-requested") return summarized; + const reviews = + latestReviews === null || latestReviews === undefined + ? [] + : "nodes" in latestReviews + ? latestReviews.nodes + : latestReviews; + const states = new Set(reviews.map((review) => review.state?.trim().toUpperCase() ?? "")); + if (states.has("CHANGES_REQUESTED")) return "changes-requested"; + if (states.has("APPROVED")) return "approved"; + return summarized; +} + function toReviewDecision(value: string | null | undefined): PullRequestReviewDecision | null { switch (value?.trim().toUpperCase()) { case "APPROVED": @@ -1558,7 +1592,7 @@ function toListItem(raw: Schema.Schema.Type): GitHubPu state: toState(raw), isDraft: raw.isDraft ?? false, mergeability: toMergeability(raw.mergeable), - reviewDecision: toReviewDecision(raw.reviewDecision), + reviewDecision: toReviewDecisionWithReviews(raw.reviewDecision, raw.latestReviews), additions: raw.additions ?? 0, deletions: raw.deletions ?? 0, createdAt: raw.createdAt, @@ -1681,6 +1715,9 @@ export function decodePullRequestSearchJson( items.push({ ...toListItem({ ...node, + latestReviews: (node.latestReviews?.nodes ?? []).flatMap((review) => + review === null ? [] : [review], + ), reviewRequests: (node.reviewRequests?.nodes ?? []).flatMap((request) => { const login = trimmed(request?.requestedReviewer?.login); return login === null ? [] : [{ login }]; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 98d172f53eb51222b38a6facf033896b21cea56d..44b70f2e18ffbe2320fbc8784bc2a5d596aeae53 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -50,6 +50,7 @@ import { useMemo, useRef, useState, + type ReactNode, } from "react"; import { type DraftId, useComposerDraftStore } from "~/composerDraftStore"; @@ -344,6 +345,8 @@ function PullRequestBaseFreshnessWarning({ pending, onUpdate, iconClassName, + className, + children, }: { readonly baseBranch: string; readonly freshness: { @@ -353,6 +356,9 @@ function PullRequestBaseFreshnessWarning({ readonly pending: boolean; readonly onUpdate: (method: PullRequestUpdateMethod) => void; readonly iconClassName?: string; + readonly className?: string; + /** What the warning is about, drawn in the same amber before the mark: the base branch. */ + readonly children?: ReactNode; }) { const behind = freshness.behindBy === null @@ -371,11 +377,15 @@ function PullRequestBaseFreshnessWarning({ ); } diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index 4470e6c307c0bf8620fb86a1b4cf288ff2e1aeee..b461e4d5859d5460653bd26edc92b64ec789892d 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -30,8 +30,9 @@ import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { PullRequestActorLabel, PullRequestCheckStatusIcon, - PullRequestReviewOutcomeBadge, pullRequestCheckStatusLabel, + PullRequestLabelChip, + PullRequestReviewOutcomeBadge, pullRequestReviewOutcomeLabel, pullRequestReviewOutcomeRingClassName, pullRequestReviewOutcomeStaleLabel, @@ -56,7 +57,6 @@ import { PullRequestCommentBody } from "./PullRequestCommentBody"; import { PullRequestMarkdownEditor } from "./PullRequestMarkdownEditor"; import { PullRequestReactionBar } from "./PullRequestReactions"; import { PullRequestConversationGhost } from "./PullRequestGhosts"; -import { pullRequestLabelColor } from "./pullRequestList.logic"; import { sectionCollapseAnchorScrollTop } from "./pullRequestSummaryScroll.logic"; /** One reviewer, however a host happens to have cased their login this time. */ @@ -809,22 +809,14 @@ export function PullRequestSummaryTab({ {detail.labels.length === 0 ? ( None ) : ( - detail.labels.map((label) => { - const dot = pullRequestLabelColor(label.color); - return ( - - - {label.name} - - ); - }) + detail.labels.map((label) => ( + + )) )} {detail.capabilities.labels === true ? ( ) : ( - - - - + )} openPrLink(event, link.url, threadRef)} - className="min-w-0 flex-1" + className="flex min-w-0 flex-1" > - - - - } - > - #{link.number} - - - {SOURCE_LABELS[link.source]} · {formatRelativeTimeLabel(link.linkedAt)} - - - - {snapshot?.title ?? link.repository} - - {/* Match the full PR list: review verdict, checks, then diff counts. - Each is absent rather than neutral when the - host said nothing, so a row without them reads as unknown, not as fine. */} - - {snapshot?.state === "open" && - (snapshot.reviewDecision === "approved" || - snapshot.reviewDecision === "changes-requested") ? ( - snapshot.reviewDecision === "approved" ? ( - - ) : ( - Changes requested - ) - ) : null} - {snapshot?.checksState ? : null} - - - - - {stack ? ( + - - } - > - - {stack.size} + }> + #{link.number} - {stack.kind === "native" - ? `GitHub stack of ${stack.size}: merging a layer lands the ones below it.` - : `${stack.size} pull requests chained by base branch.`} + {SOURCE_LABELS[link.source]} · {formatRelativeTimeLabel(link.linkedAt)} - ) : null} - {snapshot?.author ? ( - - - {snapshot.author.login} - - ) : null} - - {snapshot !== null - ? `${snapshot.headBranch} → ${snapshot.baseBranch}` - : `${link.host}/${link.repository}`} - - {snapshot?.updatedAt ? ( - {formatRelativeTimeLabel(snapshot.updatedAt)} - ) : null} - - - - + {snapshot.checksState ? : null} + {snapshot.reviewDecision ? ( + + ) : null} + + ) : null + } + // Match the full PR list: diff counts up top, checks under the lifecycle glyph, the + // verdict by the author. Each is absent rather than neutral when the host said + // nothing, so a row without them reads as unknown, not as fine. + status={ + + } + meta={ + <> + {stack ? ( + + + } + > + + {stack.size} + + + {stack.kind === "native" + ? `GitHub stack of ${stack.size}: merging a layer lands the ones below it.` + : `${stack.size} pull requests chained by base branch.`} + + + ) : null} + {snapshot?.author ? ( + + ) : null} + {snapshot !== null ? ( + + ) : ( + + {link.host}/{link.repository} + )} - > - - + } + updatedAt={snapshot?.updatedAt} /> - - void writeTextToClipboard(link.url, "link")}> - - Copy link - - openPrLink(event, link.url, threadRef)}> - - Open - - onUnlink(link)}> - - {link.source === "stack" ? "Dismiss from thread" : "Unlink from thread"} - - - + + {/* Out of the row's flow, so no row reserves a column for a button only the hovered one + shows. It sits over the right end of the second line on the row's own hover color, + fading in from the left, so it covers the time and leaves the diff counts alone. */} +