diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 1b2efa7ec70e..4bb86401d8e5 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -2071,6 +2071,7 @@ export const make = Effect.gen(function* () { }); const entries: GitHubReviewThreadEntry[] = []; const avatarsByLogin = new Map(); + const botLogins = new Set(); const commitStats = new Map< string, { readonly additions: number; readonly deletions: number } @@ -2087,6 +2088,7 @@ export const make = Effect.gen(function* () { do { const read: GitHubReviewThreadPage = yield* threadPage(cursor); entries.push(...read.threads); + for (const login of read.botLogins) botLogins.add(login); for (const [login, avatarUrl] of read.avatarsByLogin) avatarsByLogin.set(login, avatarUrl); // The roster, the commits and the viewer's standing travel with every page, and the @@ -2152,6 +2154,7 @@ export const make = Effect.gen(function* () { reactionsById, reviewers, avatarsByLogin, + botLogins, commitStats, commits, viewer, diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index c2571d91d563..f2629f68b956 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -741,6 +741,7 @@ describe("getChangeRequest commits", () => { reactionsById: new Map>(), reviewers: [], avatarsByLogin: new Map(), + botLogins: new Set(), commitStats: new Map(), viewer: { canUpdate: true, didAuthor: false }, }; @@ -807,7 +808,7 @@ describe("getChangeRequestActivity dismissed reviews", () => { const dismissedReview = (body: string) => ({ id: "PRR_1", kind: "review" as const, - author: null, + author: { login: "macroscopeapp", name: null, avatarUrl: null }, body, createdAt: "2026-07-03T00:00:00Z", url: null, @@ -824,6 +825,7 @@ describe("getChangeRequestActivity dismissed reviews", () => { reactionsById: new Map(), reviewers: [], avatarsByLogin: new Map(), + botLogins: new Set(["macroscopeapp"]), commitStats: new Map(), commits: [], viewer: { canUpdate: true, didAuthor: false }, @@ -850,6 +852,7 @@ describe("getChangeRequestActivity dismissed reviews", () => { readActivity.pipe( Effect.map((activity) => { expect(activity.comments[0]?.body).toBe("Dismissing prior approval to re-evaluate 9b66581"); + expect(activity.comments[0]?.author?.isBot).toBe(true); }), Effect.provide(layerFor("")), ), diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index 1d003a970ee8..de7f7fc952f2 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -133,8 +133,11 @@ function withAvatar( actor: PullRequestActor | null, avatarsByLogin: ReadonlyMap, host: string, + botLogins?: ReadonlySet, ): PullRequestActor | null { - if (actor === null || actor.avatarUrl !== null) return actor; + if (actor === null) return actor; + if (botLogins?.has(actor.login)) actor = { ...actor, isBot: true }; + if (actor.avatarUrl !== null) return actor; const avatarUrl = avatarsByLogin.get(actor.login) ?? loginAvatarUrl(actor.login, host); return avatarUrl === null ? actor : { ...actor, avatarUrl }; } @@ -450,6 +453,7 @@ export const make = Effect.gen(function* () { truncated: true, reviewers: [], avatarsByLogin: new Map(), + botLogins: new Set(), commitStats: new Map< string, { readonly additions: number; readonly deletions: number } @@ -463,7 +467,12 @@ export const make = Effect.gen(function* () { ).pipe( Effect.mapError(fail("getChangeRequestActivity")), Effect.map(([pullRequest, reviewThreads]): ProviderChangeRequestActivity => ({ - author: withAvatar(pullRequest.author, reviewThreads.avatarsByLogin, input.host), + author: withAvatar( + pullRequest.author, + reviewThreads.avatarsByLogin, + input.host, + reviewThreads.botLogins, + ), reviewers: reviewThreads.reviewers, reactions: reviewThreads.reactions, commits: (reviewThreads.commits.length > 0 @@ -473,7 +482,13 @@ export const make = Effect.gen(function* () { ...commit, ...reviewThreads.commitStats.get(commit.oid), authors: commit.authors?.map( - (author) => withAvatar(author, reviewThreads.avatarsByLogin, input.host) ?? author, + (author) => + withAvatar( + author, + reviewThreads.avatarsByLogin, + input.host, + reviewThreads.botLogins, + ) ?? author, ), })), comments: [...pullRequest.comments, ...reviewThreads.comments] @@ -489,7 +504,12 @@ export const make = Effect.gen(function* () { rendersEmpty(comment.body) ? (reviewThreads.dismissalsByReviewId.get(comment.id) ?? comment.body) : comment.body, - author: withAvatar(comment.author, reviewThreads.avatarsByLogin, input.host), + author: withAvatar( + comment.author, + reviewThreads.avatarsByLogin, + input.host, + reviewThreads.botLogins, + ), // A comment out of `gh pr view --json` carries none of its own: that read // reports no reaction at all, so they arrive from the GraphQL page by node id. reactions: comment.reactions ?? reviewThreads.reactionsById.get(comment.id) ?? [], @@ -503,7 +523,12 @@ export const make = Effect.gen(function* () { ...thread, comments: thread.comments.map((comment) => ({ ...comment, - author: withAvatar(comment.author, reviewThreads.avatarsByLogin, input.host), + author: withAvatar( + comment.author, + reviewThreads.avatarsByLogin, + input.host, + reviewThreads.botLogins, + ), })), })), })), diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts index f9f6c4ce3d26..1a041dc333b1 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.test.ts @@ -436,14 +436,26 @@ describe("review thread decoding", () => { requested: [{ login: "julius", name: "Julius", avatarUrl: "https://avatars/j.png" }], // An app that has reviewed is no longer an outstanding request, which is why asking // only for requests reported nobody on a pull request a bot had reviewed. - reviewed: [{ login: "macroscopeapp", avatarUrl: "https://avatars/in/900172.png" }], + reviewed: [ + { + __typename: "Bot", + login: "macroscopeapp", + avatarUrl: "https://avatars/in/900172.png", + }, + ], }), ), ); + expect(result.botLogins).toEqual(new Set(["macroscopeapp"])); expect(result.reviewers).toEqual([ { login: "julius", name: "Julius", avatarUrl: "https://avatars/j.png" }, - { login: "macroscopeapp", name: null, avatarUrl: "https://avatars/in/900172.png" }, + { + login: "macroscopeapp", + name: null, + avatarUrl: "https://avatars/in/900172.png", + isBot: true, + }, ]); }); diff --git a/apps/server/src/pullRequest/gitHubPullRequestJson.ts b/apps/server/src/pullRequest/gitHubPullRequestJson.ts index 42fa0ba0e04f..483557b7f11b 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestJson.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestJson.ts @@ -39,6 +39,8 @@ import { dedupeChecks } from "./pullRequestChecks.ts"; * release that adds a conclusion or a review state must not fail the whole payload. */ const RawActorSchema = Schema.Struct({ + __typename: Schema.optional(Schema.String), + is_bot: Schema.optional(Schema.Boolean), /** * Optional because a review can be requested from a team or a mannequin, which the query has * no fragment for and GraphQL answers with an empty object. A reviewer with no login names @@ -480,7 +482,7 @@ const RawReviewThreadsSchema = Schema.Struct({ ), ), /** - * Reviews for their reactions alone: the words and the verdict arrive with + * Reviews for their reactions and actor identity: the words and the verdict arrive with * `gh pr view --json reviews`, which reports no reaction of any kind. */ reviews: Schema.optional( @@ -489,6 +491,7 @@ const RawReviewThreadsSchema = Schema.Struct({ nodes: Schema.Array( Schema.Struct({ id: Schema.optional(Schema.NullOr(Schema.String)), + author: Schema.optional(Schema.NullOr(RawActorSchema)), reactionGroups: RawReactionGroupsSchema, }), ), @@ -676,7 +679,7 @@ export function pullRequestSearchGraphQlQuery(rows: number, includeStacks = fals number title url - author { login avatarUrl ... on User { name } } + author { __typename login avatarUrl ... on User { name } } headRefName baseRefName state @@ -736,28 +739,28 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin comments(first: 10) { totalCount pageInfo { hasNextPage endCursor } - nodes { id author { login avatarUrl } body createdAt url ${REACTION_GROUPS_FIELDS} } + nodes { id author { __typename login avatarUrl } body createdAt url ${REACTION_GROUPS_FIELDS} } } } } viewerCanUpdate viewerDidAuthor - author { login avatarUrl } + author { __typename login avatarUrl } ${REACTION_GROUPS_FIELDS} comments(first: ${GRAPHQL_PAGE_SIZE}) { - nodes { id author { login avatarUrl } ${REACTION_GROUPS_FIELDS} } + nodes { id author { __typename login avatarUrl } ${REACTION_GROUPS_FIELDS} } } - reviews(first: ${GRAPHQL_PAGE_SIZE}) { nodes { id ${REACTION_GROUPS_FIELDS} } } + reviews(first: ${GRAPHQL_PAGE_SIZE}) { nodes { id author { __typename login avatarUrl } ${REACTION_GROUPS_FIELDS} } } reviewRequests(first: 50) { nodes { requestedReviewer { ... on User { login name avatarUrl } - ... on Bot { login avatarUrl } + ... on Bot { __typename login avatarUrl } } } } latestReviews(first: 50) { - nodes { author { login avatarUrl } } + nodes { author { __typename login avatarUrl } } } reviewDismissals: timelineItems(itemTypes: [REVIEW_DISMISSED_EVENT], first: ${GRAPHQL_PAGE_SIZE}) { pageInfo { hasNextPage endCursor } @@ -793,7 +796,7 @@ export const REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY = `query($owner: String!, $nam pullRequest { id } comments(first: ${GRAPHQL_PAGE_SIZE}, after: $cursor) { pageInfo { hasNextPage endCursor } - nodes { id author { login avatarUrl } body createdAt url ${REACTION_GROUPS_FIELDS} } + nodes { id author { __typename login avatarUrl } body createdAt url ${REACTION_GROUPS_FIELDS} } } } } @@ -1134,7 +1137,12 @@ function toActor(raw: Schema.Schema.Type | null | undefin const login = trimmed(raw?.login); return login === null ? null - : { login, name: trimmed(raw?.name), avatarUrl: trimmed(raw?.avatarUrl) }; + : { + login, + name: trimmed(raw?.name), + avatarUrl: trimmed(raw?.avatarUrl), + ...(raw?.__typename === "Bot" || raw?.is_bot === true ? { isBot: true } : {}), + }; } function toCommitActor( @@ -1753,6 +1761,7 @@ export interface GitHubReviewThreadComments { * so an app's avatar arrives the same way a person's does. */ readonly avatarsByLogin: ReadonlyMap; + readonly botLogins: ReadonlySet; /** Per-commit line counts carried by the same bounded pull-request query. */ readonly commitStats: ReadonlyMap< string, @@ -1791,6 +1800,7 @@ export interface GitHubReviewThreadPage { readonly reactionsById: ReadonlyMap>; readonly reviewers: ReadonlyArray; readonly avatarsByLogin: ReadonlyMap; + readonly botLogins: ReadonlySet; readonly commitStats: ReadonlyMap< string, { readonly additions: number; readonly deletions: number } @@ -1928,9 +1938,11 @@ export function decodeReviewThreadsJson( }); const pullRequest = decoded.success.data.repository.pullRequest; const avatarsByLogin = new Map(); + const botLogins = new Set(); for (const raw of [ pullRequest.author, ...(pullRequest.comments?.nodes ?? []).map((node) => node.author), + ...(pullRequest.reviews?.nodes ?? []).map((node) => node.author), ...(pullRequest.reviewRequests?.nodes ?? []).map((node) => node.requestedReviewer), ...(pullRequest.latestReviews?.nodes ?? []).map((node) => node.author), ...threads.nodes.flatMap((thread) => thread.comments.nodes.map((comment) => comment.author)), @@ -1938,6 +1950,7 @@ export function decodeReviewThreadsJson( const login = trimmed(raw?.login); const avatarUrl = trimmed(raw?.avatarUrl); if (login !== null && avatarUrl !== null) avatarsByLogin.set(login, avatarUrl); + if (login !== null && toActor(raw)?.isBot) botLogins.add(login); } const reviewers = new Map(); for (const raw of [ @@ -1996,6 +2009,7 @@ export function decodeReviewThreadsJson( reactionsById, reviewers: [...reviewers.values()], avatarsByLogin, + botLogins, commitStats, commits, viewer: toPullRequestViewerFields(pullRequest), diff --git a/apps/web/src/components/pullRequest/PullRequestCommentBody.tsx b/apps/web/src/components/pullRequest/PullRequestCommentBody.tsx new file mode 100644 index 000000000000..d730c0f859a1 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestCommentBody.tsx @@ -0,0 +1,61 @@ +import { useEffect, useId, useRef, useState, type ComponentProps } from "react"; + +import { cn } from "~/lib/utils"; +import { Button } from "../ui/button"; +import { PullRequestMarkdown } from "./PullRequestMarkdown"; + +/** Keep the complete markdown intact while limiting long reports to a readable preview. */ +export function PullRequestCommentBody({ + className, + ...props +}: ComponentProps) { + const [expanded, setExpanded] = useState(false); + const [overflowing, setOverflowing] = useState(false); + const content = useRef(null); + const id = useId(); + + useEffect(() => { + const element = content.current; + if (!element) return; + const measure = () => setOverflowing(element.getBoundingClientRect().height > 240); + measure(); + const observer = new ResizeObserver(measure); + observer.observe(element); + return () => observer.disconnect(); + }, []); + + return ( +
+
setExpanded(true)} + > +
+ +
+ {overflowing && !expanded ? ( +
+ ) : null} +
+ {overflowing ? ( + + ) : null} +
+ ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestReactions.tsx b/apps/web/src/components/pullRequest/PullRequestReactions.tsx index 2ef90b09a55c..82e80b86c2f7 100644 --- a/apps/web/src/components/pullRequest/PullRequestReactions.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReactions.tsx @@ -34,15 +34,7 @@ function reactionsSignature(reactions: ReadonlyArray): stri .join(" "); } -/** - * The reaction pills under a remark, and the picker that adds one. The same bar serves the - * description, a conversation comment and a review thread's comments: what differs between them - * is only which subject the host is told about. - * - * The add button is revealed by hovering the remark it belongs to, the way GitHub's is, so the - * parent must carry `group`. It stays put once there is something to press it beside, while the - * picker is open, and whenever it is focused — a control only a mouse can find is no control. - */ +/** Reaction counts and an always-visible picker, routed to the supplied host subject. */ export function PullRequestReactionBar({ reactions, canReact, @@ -98,7 +90,7 @@ export function PullRequestReactionBar({ if (shown.length === 0 && !canReact) return null; return ( -
+
{shown.map((reaction) => ( } diff --git a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx index 90a1926d1fad..f1e8955c3d54 100644 --- a/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx +++ b/apps/web/src/components/pullRequest/PullRequestReviewAnnotation.tsx @@ -264,9 +264,18 @@ export function ReviewThreadCard({
{comments.map((comment) => (
-
+
{formatRelativeTimeLabel(comment.createdAt)} +
{editingId === comment.id ? ( )} -
))}
diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx index 6bf532e5aefd..57033b67154c 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx @@ -1,5 +1,5 @@ import { EnvironmentId, ProjectId, type PullRequestDetailView } from "@t3tools/contracts"; -import { act } from "react"; +import { act, type ReactNode } from "react"; import { create, type ReactTestRenderer } from "react-test-renderer"; import { afterEach, beforeEach, expect, it, vi } from "vite-plus/test"; @@ -9,6 +9,11 @@ vi.mock("~/browser/useOpenLink", () => ({ useOpenLink: () => vi.fn() })); vi.mock("./PullRequestMarkdown", () => ({ PullRequestMarkdown: ({ text }: { text: string }) =>

{text}

, })); +vi.mock("../ui/tooltip", () => ({ + Tooltip: ({ children }: { children: ReactNode }) => children, + TooltipTrigger: ({ children }: { children: ReactNode }) => children, + TooltipPopup: () => null, +})); import { PullRequestSummaryTab } from "./PullRequestSummaryTab"; @@ -139,3 +144,71 @@ it("keeps an unsaved description when collapsed and reopened", () => { click("Description"); expect(renderer.root.findByType("textarea").props.value).toBe("Unsaved description"); }); + +it("opens bot reports in pages without hiding human comments", () => { + const value: PullRequestDetailView = { + ...detail, + commentCount: 13, + comments: Array.from({ length: 13 }, (_, index) => ({ + id: `comment-${index}`, + kind: "issue-comment", + author: { + login: index === 12 ? "human" : "review-app", + name: null, + avatarUrl: null, + isBot: index !== 12, + }, + body: index === 12 ? "Human comment" : `Bot report ${index}`, + createdAt: `2026-09-01T00:00:${String(index).padStart(2, "0")}Z`, + url: null, + path: null, + reviewState: null, + })), + }; + act(() => { + renderer = create(render(value)); + }); + expect(renderer.root.findAllByType("p").map((p) => p.children.join(""))).toContain( + "Human comment", + ); + expect( + renderer.root.findAllByType("p").some((p) => p.children.join("").startsWith("Bot report")), + ).toBe(false); + const group = renderer.root + .findAllByType("button") + .find((button) => button.props["aria-label"] === "12 bot comments")!; + act(() => group.props.onClick({ nativeEvent: {}, preventDefault() {}, stopPropagation() {} })); + expect( + renderer.root.findAllByType("p").filter((p) => p.children.join("").startsWith("Bot report")), + ).toHaveLength(10); + expect(renderer.root.findAllByType("p").map((p) => p.children.join(""))).not.toContain( + "Bot report 0", + ); + act(() => + renderer.root + .findAllByType("button") + .find((button) => button.children.includes(" older bot comment"))! + .props.onClick(), + ); + expect( + renderer.root.findAllByType("p").filter((p) => p.children.join("").startsWith("Bot report")), + ).toHaveLength(12); + act(() => group.props.onClick({ nativeEvent: {}, preventDefault() {}, stopPropagation() {} })); + act(() => group.props.onClick({ nativeEvent: {}, preventDefault() {}, stopPropagation() {} })); + expect(renderer.root.findAllByType("p").map((p) => p.children.join(""))).toContain( + "Bot report 0", + ); + act(() => + renderer.root + .findAllByType("button") + .find((button) => button.children.includes(" recent bot comments"))! + .props.onClick(), + ); + expect( + renderer.root.findAllByType("p").filter((p) => p.children.join("").startsWith("Bot report")), + ).toHaveLength(10); + act(() => renderer.update(render({ ...value, url: `${value.url}0`, number: 10 }))); + expect( + renderer.root.findAllByType("p").some((p) => p.children.join("").startsWith("Bot report")), + ).toBe(false); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index 7c7c72d8d163..247f1a18f411 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -1,9 +1,9 @@ import type { EnvironmentId, - PullRequestActor, PullRequestComment, PullRequestDetailView, PullRequestRef, + PullRequestReviewThread, ScopedThreadRef, } from "@t3tools/contracts"; import { @@ -28,7 +28,6 @@ import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collaps import { toastManager } from "../ui/toast"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { - PullRequestActorAvatar, PullRequestActorLabel, PullRequestCheckStatusIcon, PullRequestReviewOutcomeBadge, @@ -53,6 +52,7 @@ import { canEditPullRequestComment, } from "./pullRequestEditing.logic"; import { PullRequestMarkdown } from "./PullRequestMarkdown"; +import { PullRequestCommentBody } from "./PullRequestCommentBody"; import { PullRequestMarkdownEditor } from "./PullRequestMarkdownEditor"; import { PullRequestReactionBar } from "./PullRequestReactions"; import { PullRequestConversationGhost } from "./PullRequestGhosts"; @@ -64,18 +64,69 @@ function reviewerKey(login: string): string { return login.toLowerCase(); } -/** The avatar carries the attribution alone; who it is arrives on hover, like the reviewer row. */ -function CommentAuthor({ actor }: { actor: PullRequestActor | null }) { - const login = actor?.login ?? "ghost"; +function CommentIdentity({ + comment, + detail, +}: { + comment: PullRequestComment; + detail: PullRequestDetailView; +}) { + const actor = comment.author; + const profileUrl = + detail.provider === "github" && actor && !actor.login.endsWith("[bot]") + ? new URL(`/${encodeURIComponent(actor.login)}`, detail.url).toString() + : null; + return ( +
+ + + + ) : ( + + ) + } + > + + + + {new Date(comment.createdAt).toLocaleString()} + {comment.url ? " · Open comment on host" : ""} + + +
+ ); +} + +function CommentLocation({ + comment, + thread, +}: { + comment: PullRequestComment; + thread: PullRequestReviewThread | undefined; +}) { + const path = thread?.path ?? comment.path; + if (!path) return null; + const label = `${path}${thread?.line ? `:${thread.line}` : ""}`; return ( - - }> - - - - {actor?.name && actor.name !== login ? `${actor.name} (@${login})` : login} - - +
+ + }>{label} + {label} + + {thread?.isOutdated ? Outdated : null} +
); } @@ -127,7 +178,8 @@ function CommentBody({ } return (
- (null); return (
- - - - {formatRelativeTimeLabel(comment.createdAt)} - {label} - - - +
+
+ + + {label} + + + {reactionBar} +
+ + {!open && body ? ( + statusTriggerRef.current?.focus({ preventScroll: true })} + > + {body + .replace(//gu, "") + .replace(/^\s*>?\s*\[!\w+\]\s*$/gmu, "") + .replace(/!?(\[([^\]]+)\])\([^)]*\)/gu, "$2") + .replace(/^[\s>#*-]+/gmu, "") + .replace(/[*`]/gu, "") + .replace(/\s+/g, " ") + .trim()} + + ) : null} +
{open ? (
- {comment.path ? ( - - {comment.path}

- } - /> - {comment.path} -
- ) : null} {/* A dismissal carries no more words than an approval does, and an empty markdown block reads as a card somebody forgot to fill in. */} {body === null && !editing.canEdit(comment) ? null : ( )} - {reactionBar}
) : null}
@@ -302,11 +361,107 @@ function Section({ ); } +function CommentGroup({ + label, + comments, + detail, + children, + onOpenChange, +}: { + label: string; + comments: readonly PullRequestComment[]; + detail: PullRequestDetailView; + children: ReactNode; + onOpenChange?: (open: boolean) => void; +}) { + const authors = [ + ...new Map( + comments.map((comment) => [reviewerKey(comment.author?.login ?? "ghost"), comment.author]), + ).values(), + ]; + const fileCount = new Set(comments.flatMap((comment) => (comment.path ? [comment.path] : []))) + .size; + const latest = comments.reduce( + (date, comment) => (date === null || comment.createdAt > date ? comment.createdAt : date), + null, + ); + return ( + +
+
+ {authors.slice(0, 3).map((actor) => ( + + ))} + {authors.length > 3 ? ( + + +{authors.length - 3} + + ) : null} +
+ + + {label} + + + {authors.length} {authors.length === 1 ? "author" : "authors"} + + {fileCount > 0 ? ( + + · {fileCount} {fileCount === 1 ? "file" : "files"} + + ) : null} + {latest ? ( + + · Latest{" "} + + }> + {formatRelativeTimeLabel(latest)} + + {new Date(latest).toLocaleString()} + + + ) : null} + + + + +
+ +
{children}
+
+
+ ); +} + /** * What a first render of the conversation carries. A pull request with two hundred comments is * two hundred markdown documents, and the ones worth arriving for are the recent ones. */ -const COMMENT_PAGE = 30; +const COMMENT_PAGE = 10; export function PullRequestSummaryTab({ environmentId, @@ -337,11 +492,35 @@ export function PullRequestSummaryTab({ // Keyed by the pull request, so opening another one starts at the end of its conversation // rather than wherever the last one had been read back to. const [shown, setShown] = useState({ url: detail.url, count: COMMENT_PAGE }); + const [openedBotGroup, setOpenedBotGroup] = useState(null); + const [shownBots, setShownBots] = useState({ url: detail.url, count: COMMENT_PAGE }); + const shownBotComments = shownBots.url === detail.url ? shownBots.count : COMMENT_PAGE; const shownComments = shown.url === detail.url ? shown.count : COMMENT_PAGE; + // A comment that already lives on a review thread is that thread: the thread carries the line + // and side the bare comment has lost, and a resolved one is finished work nobody should be + // invited to fix again — the same call the whole-review hand-off makes. + const threadByCommentId = new Map( + detail.reviewThreads.flatMap((thread) => + thread.comments.map((comment) => [comment.id, thread] as const), + ), + ); + + const activeComments: PullRequestComment[] = []; + const finishedComments: PullRequestComment[] = []; + const botComments: PullRequestComment[] = []; + for (const comment of detail.comments) { + const finished = + threadByCommentId.get(comment.id)?.isResolved || + pullRequestReviewOutcome(comment.reviewState) === "dismissed"; + const bot = comment.author?.isBot === true || comment.author?.login.endsWith("[bot]"); + (finished ? finishedComments : bot ? botComments : activeComments).push(comment); + } // Windowed by recency regardless of display order: expanding always reaches further back in // time, whether the newest comment currently reads first or last. - const recentComments = detail.comments.slice(Math.max(0, detail.comments.length - shownComments)); - const hiddenCommentCount = detail.comments.length - recentComments.length; + const recentComments = activeComments.slice(Math.max(0, activeComments.length - shownComments)); + const hiddenCommentCount = activeComments.length - recentComments.length; + const recentBotComments = botComments.slice(Math.max(0, botComments.length - shownBotComments)); + const hiddenBotCommentCount = botComments.length - recentBotComments.length; const [commentOrder, setCommentOrder] = useState<"newest" | "oldest">("newest"); const visibleComments = orderPullRequestComments(recentComments, commentOrder); const showOldestCommentsButton = @@ -352,12 +531,12 @@ export function PullRequestSummaryTab({ className="w-full" onClick={() => setShown({ url: detail.url, count: shownComments + COMMENT_PAGE })} > - Show {Math.min(hiddenCommentCount, COMMENT_PAGE)} oldest{" "} - {hiddenCommentCount === 1 ? "comment" : "comments"} + Show {Math.min(hiddenCommentCount, COMMENT_PAGE)} older comment + {hiddenCommentCount === 1 ? "" : "s"} ({hiddenCommentCount} hidden) ) : null; // Read from the whole conversation, not the window shown below it: a verdict older than the - // last thirty comments still stands. + // visible comments still stands. const reviewOutcomes = latestPullRequestReviewOutcomes(detail.comments, detail.commits); // Hosts do not promise one casing for a login across two fields of the same response, and // none of them lets `Octocat` and `octocat` be two people — so matching on the literal string @@ -393,15 +572,6 @@ export function PullRequestSummaryTab({ })), ]; - // A comment that already lives on a review thread is that thread: the thread carries the line - // and side the bare comment has lost, and a resolved one is finished work nobody should be - // invited to fix again — the same call the whole-review hand-off makes. - const threadByCommentId = new Map( - detail.reviewThreads.flatMap((thread) => - thread.comments.map((comment) => [comment.id, thread] as const), - ), - ); - const openLink = useOpenLink(threadRef); const openCheck = (url: string) => { void openLink(url).catch((error: unknown) => { @@ -470,6 +640,78 @@ export function PullRequestSummaryTab({ }, }; + const renderComment = (comment: PullRequestComment) => { + const thread = threadByCommentId.get(comment.id); + const body = visibleBody(comment.body); + const outcome = pullRequestReviewOutcome(comment.reviewState); + // An approval is a verdict, not a finding: there is nothing in it to fix. + const finding: PullRequestFinding | null = + (comment.kind !== "review" && comment.kind !== "review-comment") || outcome === "approved" + ? null + : thread === undefined + ? // Nor is a remark with nothing in it: offering to hand an empty review + // to a thread promises work it does not describe. + body === null + ? null + : { kind: "comment", comment } + : { kind: "thread", thread }; + const reactionBar = ( + + ); + return ( +
+
+
+ + {outcome ? ( + + ) : comment.reviewState ? ( + {reviewStateLabel(comment.reviewState)} + ) : null} +
+ {/* Review remarks only. A plain conversation comment is talk, not a finding, + and offering to fix one would promise more than it says. */} + {onFixFinding && finding ? ( + + ) : null} + {reactionBar} +
+
+ +
+ {/* A verdict usually carries no words, and an empty markdown block reads as + a card somebody forgot to fill in — the badge above already said it. + Kept where this reader may rewrite the remark: the pencil lives in here, + and hiding the block would take away the only way back to it. */} + {body === null && !commentEditing.canEdit(comment) ? null : ( + + )} +
+ ); + }; + return (
@@ -694,7 +936,7 @@ export function PullRequestSummaryTab({
{commentOrder === "oldest" ? showOldestCommentsButton : null} - {visibleComments.map((comment) => { - const thread = threadByCommentId.get(comment.id); - const body = visibleBody(comment.body); - const outcome = pullRequestReviewOutcome(comment.reviewState); - if (thread?.isResolved || outcome === "dismissed") { - return ( - - } - /> - ); - } - // An approval is a verdict, not a finding: there is nothing in it to fix. - const finding: PullRequestFinding | null = - (comment.kind !== "review" && comment.kind !== "review-comment") || - outcome === "approved" - ? null - : thread === undefined - ? // Nor is a remark with nothing in it: offering to hand an empty review - // to a thread promises work it does not describe. - body === null - ? null - : { kind: "comment", comment } - : { kind: "thread", thread }; - // One bar, two homes. Under a card with words in it, it is the row beneath - // them. A bodiless verdict has nothing above it, so a row reserved for an add - // button nobody can see until they hover is a hole — there it rides the header - // line instead, which keeps the affordance every sibling card offers. - const reactionBar = ( - - ); - return ( -
-
- - - {formatRelativeTimeLabel(comment.createdAt)} - {outcome ? ( - - ) : comment.reviewState ? ( - {reviewStateLabel(comment.reviewState)} - ) : null} - {body === null ? reactionBar : null} - - {/* Review remarks only. A plain conversation comment is talk, not a finding, - and offering to fix one would promise more than it says. */} - {onFixFinding && finding ? ( - - ) : null} -
- {comment.path ? ( - - - {comment.path} -

+ {visibleComments.map(renderComment)} + {commentOrder === "newest" ? showOldestCommentsButton : null} + {shownComments > COMMENT_PAGE ? ( + + ) : null} + {botComments.length > 0 ? ( + { + if (open) setOpenedBotGroup(detail.url); + }} + > +
+ {openedBotGroup === detail.url + ? orderPullRequestComments(recentBotComments, commentOrder).map( + renderComment, + ) + : null} + {hiddenBotCommentCount > 0 ? ( + + ) : null} + {shownBotComments > COMMENT_PAGE ? ( + + ) : null} +
+
+ ) : null} + {finishedComments.length > 0 ? ( + +
+ {orderPullRequestComments(finishedComments, commentOrder).map((comment) => { + const thread = threadByCommentId.get(comment.id); + return ( + } /> - {comment.path} - - ) : null} - {/* A verdict usually carries no words, and an empty markdown block reads as - a card somebody forgot to fill in — the badge above already said it. - Kept where this reader may rewrite the remark: the pencil lives in here, - and hiding the block would take away the only way back to it. */} - {body === null && !commentEditing.canEdit(comment) ? null : ( - - )} - {body === null ? null : reactionBar} -
- ); - })} - {commentOrder === "newest" ? showOldestCommentsButton : null} + ); + })} +
+ + ) : null}
)} diff --git a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx index 21037d8bf4c0..b114250549d2 100644 --- a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx @@ -217,7 +217,7 @@ function ConversationCard({ return (
-
+
@@ -245,6 +245,17 @@ function ConversationCard({ ) : null} + {reactions.canReact || event.reactions.length > 0 ? ( + + ) : null}
@@ -272,18 +283,6 @@ function ConversationCard({ />
) : null} - {reactions.canReact || event.reactions.length > 0 ? ( -
- -
- ) : null}
); } @@ -469,14 +468,14 @@ function ReviewVerdictEvent({ }) { return (
- {/* Pinned rather than centred: this row grows with a body and a reaction bar, and a - centred avatar drifts down beside them instead of sitting by the name. */} + {/* Pinned rather than centred: this row grows with a body, and a + centred avatar drifts down beside it instead of sitting by the name. */} } /> -
+
@@ -504,9 +503,6 @@ function ReviewVerdictEvent({ {pullRequestReviewOutcomeStaleLabel(outcome)}
- {/* The reaction bar rides this line rather than taking one of its own. Its add button - is invisible until hovered but still occupies `h-6`, and under a verdict — usually a - single line with no body — a row of that reserved on its own reads as a hole. */}
{formatRelativeTimeLabel(event.at)} @@ -517,31 +513,32 @@ function ReviewVerdictEvent({ ) : null} - {reactions.canReact || event.reactions.length > 0 ? ( - - ) : null}
- {/* An approval usually carries no words. When it does they are the review, so they stay - visible rather than being folded away with the ordinary conversation. */} - {event.body ? ( - - ) : null}
+ {reactions.canReact || event.reactions.length > 0 ? ( + + ) : null}
+ {/* An approval usually carries no words. When it does they are the review, so they stay + visible rather than being folded away with the ordinary conversation. */} + {event.body ? ( + + ) : null}
); } diff --git a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx index 4792cca95ea7..12e2a42af98f 100644 --- a/apps/web/src/components/pullRequest/pullRequestPresentation.tsx +++ b/apps/web/src/components/pullRequest/pullRequestPresentation.tsx @@ -422,7 +422,10 @@ export function PullRequestActorLabel({ > {label} - {profileUrl ? `Open ${login}'s profile` : login} + + {actor?.name && actor.name !== login ? `${actor.name} (@${login})` : login} + {profileUrl ? " · Open profile" : ""} + ); } diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index d2bd6a839d3d..4045fe4126b8 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -121,6 +121,8 @@ export const PullRequestBaseComparison = Schema.Literals(["up-to-date", "behind" export type PullRequestBaseComparison = typeof PullRequestBaseComparison.Type; export const PullRequestActor = Schema.Struct({ + /** Present when the host identifies an automated account. */ + isBot: Schema.optional(Schema.Boolean), login: TrimmedNonEmptyString, name: Schema.NullOr(Schema.String), /** Null where a host does not report one, which is what the initials fall back to. */