diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.browser.test.tsx index 4e847e9cc7cb..281bd42dc41a 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.browser.test.tsx @@ -90,7 +90,14 @@ vi.mock(import("~/state/pullRequests"), async (original) => { vi.mock(import("~/state/query"), async (original) => ({ ...(await original()), useEnvironmentQuery: ((query: unknown) => ({ - data: query === "detail" ? currentDetail : query === queries.diff ? diff : null, + data: + query === "detail" + ? (activityDetail ?? currentDetail) + : query === "activity" + ? activityDetail + : query === queries.diff + ? diff + : null, dataUpdatedAt: 0, failure: null, isPending: false, @@ -109,33 +116,41 @@ vi.mock(import("~/state/usePullRequestStack"), async (original) => ({ refresh, })) as never, })); -// The summary stand-in exposes its panel actions and nested scrolling. -vi.mock(import("./PullRequestSummaryTab"), async (original) => ({ - ...(await original()), - PullRequestSummaryTab: (({ - onFixFinding, - scrollerRef, - }: ComponentProps) => ( -
- -
-
-
-
-
- )) as never, -})); - +// Action and scroll tests use stand-ins; activity tests render the real Summary. +vi.mock(import("./PullRequestSummaryTab"), async (original) => { + const actual = await original(); + return { + ...actual, + PullRequestSummaryTab: ((props: ComponentProps) => { + if (activityDetail !== null) return ; + const { onFixFinding, scrollerRef } = props; + return ( +
+ +
+
+
+
+
+ ); + }) as never, + }; +}); vi.mock(import("../ui/toast"), async (original) => { const actual = await original(); return { @@ -201,6 +216,7 @@ const detail: PullRequestDetailView = { }; let currentDetail = detail; +let activityDetail: PullRequestDetailView | null = null; // Enough lines to exercise the panel's title folding with the real virtualized Code tab. const diff = { @@ -256,6 +272,7 @@ function render(props: Partial> = } beforeEach(() => { + activityDetail = null; vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); currentDetail = detail; useComposerDraftStore.setState({ draftsByThreadKey: {} }); @@ -607,3 +624,107 @@ it("shows the current checkout recovery detail on the first failure and on retry second.detail, ]); }); + +it("keeps discussion counts and Timeline expansion in sync with activity refreshes", async () => { + activityDetail = { + ...detail, + provider: SourceControlProviderKind.make("gitlab"), + url: "https://gitlab.com/owner/repo/-/merge_requests/1", + comments: [], + commentCount: 0, + commentsTruncated: true, + reviewThreads: [ + { + id: "discussion-1", + path: "src/index.ts", + line: 1, + side: "right", + isResolved: false, + isOutdated: false, + comments: [ + { + id: "note-1", + author: detail.author, + body: "Discussion finding", + createdAt: "2026-09-01T00:01:00Z", + url: "https://gitlab.com/owner/repo/-/merge_requests/1#note_1", + }, + { + id: "note-2", + author: detail.author, + body: "Discussion reply", + createdAt: "2026-09-01T00:02:00Z", + url: "https://gitlab.com/owner/repo/-/merge_requests/1#note_2", + }, + ], + }, + ], + }; + await render({ reference: activityDetail }); + await expect.element(page.getByText("Discussion finding", { exact: true })).toBeVisible(); + await expect.element(page.getByText("Discussion reply", { exact: true })).toBeVisible(); + expect + .soft(page.getByRole("button", { name: /^Comments \(/ }).element().textContent) + .toBe("Comments (2)"); + const warning = page.getByText(/open it on the host to read the rest/).element().textContent; + expect.soft(warning).toMatch(/\b2\b/); + expect.soft(warning).not.toMatch(/most recent/i); + + await tab("Timeline").click(); + await expect.element(page.getByText("Discussion finding", { exact: true }).last()).toBeVisible(); + await expect.element(page.getByText("Discussion reply", { exact: true }).last()).toBeVisible(); + expect.soft(page.getByLabelText(/^[0-9]+ comments?$/).element().textContent).toBe("2"); + + const threadToggle = page.getByRole("button", { name: /^src\/index\.ts:1/ }); + const expectThread = (expanded: boolean, status: string | null) => { + const toggle = threadToggle.element(); + if (status) expect(toggle.textContent).toContain(status); + else expect(toggle.textContent).not.toMatch(/Resolved|Review dismissed/); + const visibleBody = (toggle.parentElement as HTMLElement).innerText; + expect({ + expanded: toggle.getAttribute("aria-expanded"), + findingVisible: visibleBody.includes("Discussion finding"), + replyVisible: visibleBody.includes("Discussion reply"), + }).toEqual({ + expanded: String(expanded), + findingVisible: expanded, + replyVisible: expanded, + }); + }; + const refreshActivity = async (isResolved: boolean, reviewState: string | null = null) => { + await tab("Code").click(); + await expect + .element(page.getByRole("button", { name: "Expand all files", exact: true })) + .toBeVisible(); + const current = activityDetail!; + activityDetail = { + ...current, + comments: current.reviewThreads.flatMap((thread) => + thread.comments.map((comment) => ({ + ...comment, + kind: "review-comment" as const, + path: thread.path, + reviewState, + })), + ), + reviewThreads: current.reviewThreads.map((thread) => ({ ...thread, isResolved })), + }; + await tab("Timeline").click(); + }; + + expectThread(true, null); + await refreshActivity(true); + expectThread(false, "Resolved"); + await threadToggle.click(); + expectThread(true, "Resolved"); + await refreshActivity(true); + expectThread(true, "Resolved"); + await threadToggle.click(); + expectThread(false, "Resolved"); + await refreshActivity(false); + expectThread(true, null); + await refreshActivity(false, "DISMISSED"); + expectThread(false, "Review dismissed"); + await refreshActivity(false); + expectThread(true, null); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index 2bb75bcada89..5607e3c61cc8 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -533,7 +533,15 @@ function usePullRequestDetail( author: activity?.author ?? coreDetail.author, reviewers: activity?.reviewers ?? coreDetail.reviewers, comments: activity?.comments ?? [], - commentCount: activity?.commentCount ?? 0, + commentCount: Math.max( + activity?.commentCount ?? 0, + new Set([ + ...(activity?.comments.map((comment) => comment.id) ?? []), + ...(activity?.reviewThreads.flatMap((thread) => + thread.comments.map((comment) => comment.id), + ) ?? []), + ]).size, + ), commentsTruncated: activity?.commentsTruncated ?? false, reviewThreads: activity?.reviewThreads ?? [], commits: activity?.commits ?? [], diff --git a/apps/web/src/components/pullRequest/PullRequestMoreReplies.tsx b/apps/web/src/components/pullRequest/PullRequestMoreReplies.tsx new file mode 100644 index 000000000000..0b5e3bdffe54 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestMoreReplies.tsx @@ -0,0 +1,28 @@ +import type { PullRequestReplyAvailability } from "./pullRequestDetail.logic"; + +export function PullRequestMoreReplies({ + moreReplies, + hasMoreReplies, + url, +}: PullRequestReplyAvailability) { + if (!hasMoreReplies) return null; + return ( +
+ + {moreReplies > 0 + ? `${moreReplies.toLocaleString()} more ${moreReplies === 1 ? "reply" : "replies"}` + : "More replies"} + + {url ? ( + + View on host + + ) : null} +
+ ); +} diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.browser.test.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.browser.test.tsx index b7062a146130..1d518deec477 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.browser.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.browser.test.tsx @@ -1,5 +1,10 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; -import { EnvironmentId, ProjectId, type PullRequestDetailView } from "@t3tools/contracts"; +import { + EnvironmentId, + ProjectId, + type PullRequestDetailView, + type PullRequestComment, +} from "@t3tools/contracts"; import { act } from "react"; import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, expect, it, vi } from "vite-plus/test"; @@ -219,3 +224,62 @@ it("hides bot and resolved comments until the filter shows them, and remembers t await userEvent.click(filterButton()); expect(filterItem("Bot comments").getAttribute("aria-checked")).toBe("true"); }); + +it("reveals whole older conversations with honest partial reply counts and a host link", async () => { + const comment = (index: number): PullRequestComment => ({ + id: `comment-${index}`, + kind: "issue-comment", + author: detail.author, + body: `Comment ${index}`, + createdAt: `2026-09-01T00:00:${String(index).padStart(2, "0")}Z`, + url: `${detail.url}#comment-${index}`, + path: null, + reviewState: null, + }); + const finding = { ...comment(0), kind: "review-comment" as const, body: "Review finding" }; + const reply = { ...comment(12), kind: "review-comment" as const, body: "Loaded reply" }; + const thread = { + id: "partial-thread", + path: "src/index.ts", + line: 1, + side: "right" as const, + isResolved: true, + isOutdated: false, + comments: [finding, reply], + nextCommentsCursor: "next-page", + }; + const value: PullRequestDetailView = { + ...detail, + comments: [finding, ...Array.from({ length: 10 }, (_, index) => comment(index + 1)), reply], + commentCount: 15, + reviewThreads: [{ ...thread, commentCount: 5 }], + }; + await render(value); + await toggleFilter("Resolved or dismissed"); + await userEvent.keyboard("{Escape}"); + const older = [...container.querySelectorAll("button")].find((button) => + button.textContent?.startsWith("Show 1 older"), + )!; + expect(older).toBeDefined(); + expect.soft(older.textContent).toMatch(/1 older conversation.*1 hidden/); + expect(commentTexts()).not.toContain(reply.body); + await userEvent.click(older); + const resolved = [...container.querySelectorAll("button")].find( + (button) => button.textContent === "Resolved", + )!; + expect(resolved.getAttribute("aria-expanded")).toBe("false"); + expect(commentTexts()).not.toContain(reply.body); + expect.soft(visibleText()).toContain("4 replies"); + await userEvent.click(resolved); + expect(commentTexts().slice(-2)).toEqual([finding.body, reply.body]); + expect.soft(visibleText()).toContain("3 more replies"); + const hostLink = [...container.querySelectorAll("a")].find( + (link) => + link.href === finding.url && /host/i.test(link.textContent ?? "") && link.checkVisibility(), + ); + expect.soft(hostLink, "visible link to the unloaded replies on the host").toBeDefined(); + + await render({ ...value, reviewThreads: [thread] }); + expect.soft(visibleText()).toMatch(/more replies/i); + expect(visibleText()).not.toContain("3 more replies"); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index cd2633d416c5..0e18371d8632 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -53,11 +53,17 @@ import { PullRequestLabelPicker } from "./PullRequestLabelPicker"; import { PullRequestReviewerPicker } from "./PullRequestReviewerPicker"; import { PullRequestActivityUnavailableState } from "./PullRequestActivityUnavailableState"; import { + filterPullRequestConversationItems, + isPullRequestCommentFinished, latestPullRequestReviewOutcomes, orderPullRequestComments, + pagePullRequestConversationItems, + pullRequestConversationItemComments, + pullRequestConversationItems, pullRequestFindingKey, pullRequestReviewOutcome, visibleBody, + type PullRequestConversationItem, type PullRequestFinding, } from "./pullRequestDetail.logic"; import { @@ -66,6 +72,7 @@ import { } from "./pullRequestEditing.logic"; import { PullRequestMarkdown } from "./PullRequestMarkdown"; import { PullRequestCommentBody } from "./PullRequestCommentBody"; +import { PullRequestMoreReplies } from "./PullRequestMoreReplies"; import { PullRequestMarkdownEditor } from "./PullRequestMarkdownEditor"; import { PullRequestReactionBar } from "./PullRequestReactions"; import { PullRequestConversationGhost } from "./PullRequestGhosts"; @@ -203,6 +210,9 @@ function CollapsedComment({ reactionBar, detail, thread, + replyCount = 0, + replies = null, + moreReplies = null, }: { comment: PullRequestComment; editing: CommentEditing; @@ -211,6 +221,9 @@ function CollapsedComment({ reactionBar: ReactNode; detail: PullRequestDetailView; thread: PullRequestReviewThread | undefined; + replyCount?: number; + replies?: ReactNode; + moreReplies?: ReactNode; }) { const [open, setOpen] = useState(false); const statusTriggerRef = useRef(null); @@ -220,6 +233,11 @@ function CollapsedComment({
+ {replyCount > 0 ? ( + + {replyCount} {replyCount === 1 ? "reply" : "replies"} + + ) : null} )} + {replies ?
{replies}
: null}
) : null} + {moreReplies ?
{moreReplies}
: null} ); @@ -385,44 +405,40 @@ export function PullRequestSummaryTab({ onRefreshChecks?: () => void; }) { const [shown, setShown] = useState({ url: detail.url, count: COMMENT_PAGE }); - const shownComments = shown.url === detail.url ? shown.count : COMMENT_PAGE; + const shownConversations = shown.url === detail.url ? shown.count : COMMENT_PAGE; const [commentFilter, setCommentFilter] = useLocalStorage( COMMENT_FILTER_STORAGE_KEY, DEFAULT_COMMENT_FILTER, CommentFilterSchema, ); - const threadByCommentId = new Map( - detail.reviewThreads.flatMap((thread) => - thread.comments.map((comment) => [comment.id, thread] as const), - ), - ); - - const isFinished = (comment: PullRequestComment) => - threadByCommentId.get(comment.id)?.isResolved === true || - pullRequestReviewOutcome(comment.reviewState) === "dismissed"; - const isBot = (comment: PullRequestComment) => - comment.author?.isBot === true || comment.author?.login.endsWith("[bot]") === true; - const filteredComments = detail.comments.filter( - (comment) => - (commentFilter.bots || !isBot(comment)) && (commentFilter.resolved || !isFinished(comment)), + const items = pullRequestConversationItems(detail.comments, detail.reviewThreads); + const filteredItems = filterPullRequestConversationItems(items, commentFilter); + const commentCount = items.reduce( + (count, item) => count + pullRequestConversationItemComments(item).length, + 0, ); - const filteredOutCount = detail.comments.length - filteredComments.length; - const recentComments = filteredComments.slice( - Math.max(0, filteredComments.length - shownComments), + const filteredOutCount = + commentCount - + filteredItems.reduce( + (count, item) => count + pullRequestConversationItemComments(item).length, + 0, + ); + const { shown: recentItems, hidden: hiddenConversationCount } = pagePullRequestConversationItems( + filteredItems, + shownConversations, ); - const hiddenCommentCount = filteredComments.length - recentComments.length; const [commentOrder, setCommentOrder] = useState<"newest" | "oldest">("newest"); - const visibleComments = orderPullRequestComments(recentComments, commentOrder); + const visibleItems = orderPullRequestComments(recentItems, commentOrder); const showOldestCommentsButton = - hiddenCommentCount > 0 ? ( + hiddenConversationCount > 0 ? ( ) : null; // Read from the whole conversation, not the window shown below it: a verdict older than the @@ -526,53 +542,35 @@ export function PullRequestSummaryTab({ }, }; - const renderComment = (comment: PullRequestComment) => { - const thread = threadByCommentId.get(comment.id); + const reactionBarFor = (comment: PullRequestComment) => ( + + ); + + const renderCommentCard = ( + comment: PullRequestComment, + thread: PullRequestReviewThread | undefined, + { reply = false }: { reply?: boolean } = {}, + ) => { const body = visibleBody(comment.body); - if (isFinished(comment)) { - return ( - - } - /> - ); - } const outcome = pullRequestReviewOutcome(comment.reviewState); const finding: PullRequestFinding | null = - (comment.kind !== "review" && comment.kind !== "review-comment") || outcome === "approved" + reply || + (comment.kind !== "review" && comment.kind !== "review-comment") || + outcome === "approved" ? null : thread === undefined ? body === null ? null : { kind: "comment", comment } : { kind: "thread", thread }; - const reactionBar = ( - - ); return (
) : null} - {reactionBar} -
-
- + {reactionBarFor(comment)}
+ {reply ? null : ( +
+ +
+ )} {body === null && !commentEditing.canEdit(comment) ? null : ( )} @@ -620,6 +620,44 @@ export function PullRequestSummaryTab({ ); }; + const renderItem = (item: PullRequestConversationItem) => { + const comment = item.kind === "comment" ? item.comment : item.root; + const thread = item.kind === "thread" ? item.thread : undefined; + const replies = + item.kind === "thread" && item.replies.length > 0 ? ( +
+ {item.replies.map((reply) => renderCommentCard(reply, undefined, { reply: true }))} +
+ ) : null; + const moreReplies = + item.kind === "thread" && item.hasMoreReplies ? : null; + if (isPullRequestCommentFinished(comment, thread)) { + return ( + + ); + } + if (item.kind === "comment") return renderCommentCard(comment, undefined); + return ( +
+ {renderCommentCard(comment, thread)} + {replies} + {moreReplies} +
+ ); + }; + return (
@@ -869,13 +907,13 @@ export function PullRequestSummaryTab({ <> {detail.commentsTruncated ? (

- This conversation is longer than this page reads in one go. The most recent{" "} - {detail.comments.length} are here; open it on the host to read the rest. + This conversation is only partially loaded. {commentCount} comment + {commentCount === 1 ? " is" : "s are"} loaded; open it on the host to read the rest.

) : null} - {detail.comments.length === 0 ? ( + {items.length === 0 ? (

No comments yet.

- ) : filteredComments.length === 0 ? ( + ) : filteredItems.length === 0 ? (

{filteredOutCount === 1 ? "The only comment is hidden by the filter." @@ -884,16 +922,16 @@ export function PullRequestSummaryTab({ ) : (

{commentOrder === "oldest" ? showOldestCommentsButton : null} - {visibleComments.map(renderComment)} + {visibleItems.map(renderItem)} {commentOrder === "newest" ? showOldestCommentsButton : null} - {shownComments > COMMENT_PAGE ? ( + {shownConversations > COMMENT_PAGE ? ( ) : null}
diff --git a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx index 6b5b26a83116..695bee10583f 100644 --- a/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx @@ -9,6 +9,7 @@ import type { } from "@t3tools/contracts"; import { ChevronDownIcon, + CircleCheckIcon, ExternalLinkIcon, FileCode2Icon, GitCommitHorizontalIcon, @@ -34,9 +35,11 @@ import { pullRequestReviewOutcome, type PullRequestReviewOutcome, type PullRequestTimelineEvent, + type PullRequestTimelineThread, } from "./pullRequestDetail.logic"; import { canEditPullRequestComment } from "./pullRequestEditing.logic"; import { PullRequestMarkdown } from "./PullRequestMarkdown"; +import { PullRequestMoreReplies } from "./PullRequestMoreReplies"; import { PullRequestMarkdownEditor } from "./PullRequestMarkdownEditor"; import { PullRequestReactionBar } from "./PullRequestReactions"; import { @@ -178,12 +181,14 @@ function ConversationCard({ cwd, onOpen, reactions, + showPath = true, }: { event: PullRequestTimelineEvent; editable: PullRequestComment | null; cwd: string; onOpen: (url: string) => void; reactions: ReactionSurface; + showPath?: boolean; }) { const [editing, setEditing] = useState(false); const [saving, setSaving] = useState(false); @@ -223,7 +228,7 @@ function ConversationCard({ - {event.path ? ( + {showPath && event.path ? ( {event.path} @@ -365,6 +370,103 @@ function ConversationGroup({ ); } +function ReviewThreadEvent({ + event, + thread, + editable, + cwd, + onOpen, + reactions, +}: { + event: PullRequestTimelineEvent; + thread: PullRequestTimelineThread; + editable: ReadonlyMap; + cwd: string; + onOpen: (url: string) => void; + reactions: ReactionSurface; +}) { + const dismissed = pullRequestReviewOutcome(event.reviewState) === "dismissed"; + const [expanded, setExpanded] = useState(!thread.isResolved && !dismissed); + const replyCount = thread.replies.length + thread.moreReplies; + + return ( +
+ } + muted={!expanded} + /> + + {expanded ? ( + <> + + {thread.replies.length > 0 ? ( +
+ {thread.replies.map((reply) => ( + + ))} +
+ ) : null} + + ) : null} + {thread.hasMoreReplies ? ( +
+ +
+ ) : null} +
+ ); +} + function CommitEvent({ event, onOpen, @@ -554,7 +656,9 @@ export function PullRequestTimelineTab({ onRefresh, }; const editable = new Map( - detail.comments + events + .flatMap((event) => [event, ...(event.thread?.replies ?? [])]) + .flatMap((event) => (event.comment ? [event.comment] : [])) .filter((comment) => canEditPullRequestComment(detail, comment)) .map((comment) => [comment.id, comment] as const), ); @@ -583,6 +687,19 @@ export function PullRequestTimelineTab({ ); } const event = row.event; + if (event.thread !== undefined) { + return ( + + ); + } if (event.kind === "commit") { return ; } diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 42a5017fcd90..cae49bbeb8cf 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -33,6 +33,10 @@ import { canDeletePullRequestBranch, buildFixFindingsHandoff, groupPullRequestTimelineConversations, + filterPullRequestConversationItems, + pagePullRequestConversationItems, + pullRequestConversationItemComments, + pullRequestConversationItems, handoffPrompt, handoffReviewComments, stripPullRequestHandoffReferences, @@ -723,6 +727,144 @@ describe("pull request timeline", () => { }); }); +describe("review thread conversations", () => { + const bot = { login: "review[bot]", name: null, avatarUrl: null, isBot: true }; + const comment = (id: string, day: number): PullRequestComment => ({ + ...TIMELINE_SOURCE.comments[0]!, + id, + kind: "review-comment", + body: `Current ${id}`, + createdAt: `2026-07-0${day}T12:00:00Z`, + url: `https://example.test/pull/1#${id}`, + path: "src/a.ts", + }); + const finding = { ...comment("finding", 2), author: bot }; + const reply = { + ...comment("reply", 4), + reactions: [ + { content: "thumbs-up" as const, count: 2, actors: ["author"], viewerHasReacted: true }, + ], + }; + const lastReply = comment("last-reply", 6); + const loose = TIMELINE_SOURCE.comments[0]!; + const comments = [finding, loose, reply, lastReply]; + const thread: PullRequestReviewThread = { + id: "thread", + path: "src/a.ts", + line: 7, + side: "right", + isResolved: false, + isOutdated: true, + comments: [lastReply, { ...finding, body: "Stale finding" }, reply], + }; + + it("keeps a timeline finding and its replies in one row at the finding time", () => { + const events = buildPullRequestTimeline({ + ...TIMELINE_SOURCE, + comments, + reviewThreads: [thread], + }); + expect(events.map((event) => event.id)).toEqual(["c1", "finding", "1baf7bdcafe", "created"]); + expect(events[1]).toMatchObject({ + id: finding.id, + at: finding.createdAt, + body: finding.body, + url: finding.url, + thread: { + path: thread.path, + line: 7, + isOutdated: true, + replies: [ + { id: reply.id, body: reply.body, url: reply.url, reactions: reply.reactions }, + { id: lastReply.id, body: lastReply.body, url: lastReply.url }, + ], + }, + }); + expect(groupPullRequestTimelineConversations(events).map((row) => row.kind)).toEqual([ + "comments", + "event", + "event", + "event", + ]); + expect(events[0]).toEqual(buildPullRequestTimeline(TIMELINE_SOURCE)[0]); + }); + + it("orders and pages whole Summary conversations while retaining editable comment copies", () => { + const earlier = { ...loose, id: "earlier", createdAt: "2026-07-01T00:00:00Z" }; + const items = pullRequestConversationItems([earlier, ...comments], [thread]); + expect(orderPullRequestComments(items, "oldest").map((item) => item.id)).toEqual([ + "earlier", + "thread", + "c1", + ]); + expect(orderPullRequestComments(items, "newest").map((item) => item.id)).toEqual([ + "c1", + "thread", + "earlier", + ]); + const page = pagePullRequestConversationItems(items, 2); + expect(page.hidden).toBe(1); + const grouped = page.shown[0]!; + expect(grouped.createdAt).toBe(finding.createdAt); + expect(pullRequestConversationItemComments(grouped)).toEqual([finding, reply, lastReply]); + expect(pullRequestConversationItemComments(grouped)[0]).toBe(finding); + expect(pullRequestConversationItemComments(grouped)[1]).toBe(reply); + expect(pagePullRequestConversationItems(items, 1)).toEqual({ shown: [items[2]], hidden: 2 }); + expect(pullRequestConversationItemComments(items[2]!)).toEqual([loose]); + expect(pullRequestConversationItems(comments, []).map((item) => item.id)).toEqual( + comments.map((entry) => entry.id), + ); + }); + + it("filters whole conversations when any member matches, including resolved and dismissed reviews", () => { + const botOnly = { ...comment("bot-only", 7), author: bot }; + const dismissed = { + ...comment("dismissed", 8), + kind: "review" as const, + reviewState: "DISMISSED", + }; + const items = pullRequestConversationItems( + [...comments, botOnly, dismissed], + [thread, { ...thread, id: "bot-thread", comments: [botOnly] }], + ); + const kept = filterPullRequestConversationItems(items, { bots: false, resolved: false }); + expect(kept.map((item) => item.id)).toEqual(["thread", "c1"]); + expect(pullRequestConversationItemComments(kept[0]!)).toEqual([finding, reply, lastReply]); + expect(filterPullRequestConversationItems(items, { bots: true, resolved: true })).toEqual( + items, + ); + const resolved = pullRequestConversationItems(comments, [{ ...thread, isResolved: true }]); + expect( + filterPullRequestConversationItems(resolved, { bots: true, resolved: false }).map( + (item) => item.id, + ), + ).toEqual(["c1"]); + expect(filterPullRequestConversationItems(resolved, { bots: false, resolved: true })).toEqual( + resolved, + ); + }); + + it("distinguishes unloaded replies from complete threads, even when the host only supplies a cursor", () => { + const timeline = (reviewThread: PullRequestReviewThread) => + buildPullRequestTimeline({ + ...TIMELINE_SOURCE, + comments, + reviewThreads: [reviewThread], + }).find((event) => event.id === finding.id)?.thread; + expect(timeline({ ...thread, commentCount: 5 })).toMatchObject({ + moreReplies: 2, + hasMoreReplies: true, + url: finding.url, + }); + expect(timeline({ ...thread, nextCommentsCursor: "next" })).toMatchObject({ + moreReplies: 0, + hasMoreReplies: true, + url: finding.url, + }); + expect(timeline(thread)).toMatchObject({ moreReplies: 0, hasMoreReplies: false }); + }); +}); + describe("fix findings handoff", () => { const base = { number: 42, diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index cc20657d5673..d0a4e506b46f 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -485,6 +485,21 @@ export interface PullRequestTimelineEvent { readonly path: string | null; readonly reviewState: string | null; readonly reactions: ReadonlyArray; + readonly comment?: PullRequestComment; + readonly thread?: PullRequestTimelineThread; +} + +export interface PullRequestReplyAvailability { + readonly moreReplies: number; + readonly hasMoreReplies: boolean; + readonly url: string | null; +} + +export interface PullRequestTimelineThread + extends + Pick, + PullRequestReplyAvailability { + readonly replies: ReadonlyArray; } export type PullRequestTimelineRow = @@ -498,6 +513,7 @@ export function groupPullRequestTimelineConversations( for (const event of events) { if ( (event.kind === "comment" || event.kind === "review") && + event.thread === undefined && pullRequestReviewOutcome(event.reviewState) === null ) { const last = rows.at(-1); @@ -523,6 +539,136 @@ export function visibleBody(body: string): string | null { return body.replace(//gu, "").trim().length === 0 ? null : body.trim(); } +function commentTimelineEvent(comment: PullRequestComment): PullRequestTimelineEvent { + return { + comment, + id: comment.id, + at: comment.createdAt, + kind: comment.kind === "review" ? "review" : "comment", + title: comment.kind === "review" ? "reviewed" : "commented", + body: visibleBody(comment.body), + markdown: true, + url: comment.url, + actor: comment.author, + commitAuthors: [], + additions: null, + deletions: null, + path: comment.path, + reviewState: comment.reviewState, + reactions: comment.reactions ?? [], + }; +} + +export type PullRequestConversationItem = + | { + readonly kind: "comment"; + readonly id: string; + readonly createdAt: string; + readonly comment: PullRequestComment; + } + | ({ + readonly kind: "thread"; + readonly id: string; + readonly createdAt: string; + readonly root: PullRequestComment; + readonly replies: ReadonlyArray; + readonly thread: PullRequestReviewThread; + } & PullRequestReplyAvailability); + +export function pullRequestConversationItems( + comments: ReadonlyArray, + reviewThreads: ReadonlyArray, +): ReadonlyArray { + const byId = new Map(comments.map((comment) => [comment.id, comment])); + const claimed = new Set(); + const threads: PullRequestConversationItem[] = []; + for (const thread of reviewThreads) { + // Keep the conversation's objects for editing/reactions; some hosts only include a + // comment in the thread, so normalize that copy when the flat conversation lacks it. + const ordered = thread.comments + .map( + (comment): PullRequestComment => + byId.get(comment.id) ?? { + ...comment, + kind: "review-comment", + path: thread.path, + reviewState: null, + }, + ) + .toSorted((left, right) => instant(left.createdAt) - instant(right.createdAt)); + const [root, ...replies] = ordered; + if (root === undefined) continue; + for (const comment of ordered) claimed.add(comment.id); + const moreReplies = Math.max(0, (thread.commentCount ?? ordered.length) - ordered.length); + threads.push({ + kind: "thread", + id: thread.id, + createdAt: root.createdAt, + root, + replies, + thread, + moreReplies, + hasMoreReplies: moreReplies > 0 || thread.nextCommentsCursor !== undefined, + url: ordered.find((comment) => comment.url !== null)?.url ?? null, + }); + } + threads.sort((left, right) => instant(left.createdAt) - instant(right.createdAt)); + const items: PullRequestConversationItem[] = []; + let next = 0; + // Preserve loose comments' existing order, inserting each thread at its finding's time. + for (const comment of comments) { + if (claimed.has(comment.id)) continue; + while ( + next < threads.length && + instant(threads[next]!.createdAt) < instant(comment.createdAt) + ) { + items.push(threads[next++]!); + } + items.push({ kind: "comment", id: comment.id, createdAt: comment.createdAt, comment }); + } + items.push(...threads.slice(next)); + return items; +} + +export function pullRequestConversationItemComments( + item: PullRequestConversationItem, +): ReadonlyArray { + return item.kind === "comment" ? [item.comment] : [item.root, ...item.replies]; +} + +export function isPullRequestCommentFinished( + comment: PullRequestComment, + thread?: PullRequestReviewThread, +): boolean { + return ( + thread?.isResolved === true || pullRequestReviewOutcome(comment.reviewState) === "dismissed" + ); +} + +export function filterPullRequestConversationItems( + items: ReadonlyArray, + filter: { readonly bots: boolean; readonly resolved: boolean }, +): ReadonlyArray { + return items.filter((item) => + // Matching any member keeps the finding beside a human reply to a bot. + pullRequestConversationItemComments(item).some( + (comment) => + (filter.bots || + !(comment.author?.isBot === true || comment.author?.login.endsWith("[bot]"))) && + (filter.resolved || + !isPullRequestCommentFinished(comment, item.kind === "thread" ? item.thread : undefined)), + ), + ); +} + +export function pagePullRequestConversationItems( + items: ReadonlyArray, + count: number, +): { readonly shown: ReadonlyArray; readonly hidden: number } { + const shown = items.slice(Math.max(0, items.length - count)); + return { shown, hidden: items.length - shown.length }; +} + /** * Merged wins over closed: GitHub sets both timestamps on a merge, and reporting "closed" for a * merged pull request would misstate what happened. @@ -531,7 +677,8 @@ export function buildPullRequestTimeline( detail: Pick< PullRequestDetailView, "createdAt" | "author" | "commits" | "comments" | "mergedAt" | "closedAt" - >, + > & + Partial>, ): ReadonlyArray { return [ { @@ -566,22 +713,22 @@ export function buildPullRequestTimeline( reviewState: null, reactions: [], })), - ...detail.comments.map((comment) => ({ - id: comment.id, - at: comment.createdAt, - kind: comment.kind === "review" ? ("review" as const) : ("comment" as const), - title: comment.kind === "review" ? "reviewed" : "commented", - body: visibleBody(comment.body), - markdown: true, - url: comment.url, - actor: comment.author, - commitAuthors: [], - additions: null, - deletions: null, - path: comment.path, - reviewState: comment.reviewState, - reactions: comment.reactions ?? [], - })), + ...pullRequestConversationItems(detail.comments, detail.reviewThreads ?? []).map((item) => { + if (item.kind === "comment") return commentTimelineEvent(item.comment); + return { + ...commentTimelineEvent(item.root), + thread: { + path: item.thread.path, + line: item.thread.line, + isResolved: item.thread.isResolved, + isOutdated: item.thread.isOutdated, + replies: item.replies.map(commentTimelineEvent), + moreReplies: item.moreReplies, + hasMoreReplies: item.hasMoreReplies, + url: item.url, + }, + }; + }), ...(detail.mergedAt ? [ {