From 6cd02e98f7edd78b7bde9ceff26c829476dbeb77 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 9 Oct 2026 16:40:10 -0700 Subject: [PATCH] feat(web): filter PR comments by bots and resolved threads (#17645) Co-authored-by: Claude Opus 5.5 --- .../PullRequestSummaryTab.test.tsx | 147 ++++---- .../pullRequest/PullRequestSummaryTab.tsx | 327 +++++++----------- 2 files changed, 205 insertions(+), 269 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx index 57033b67154c4d7f0955baa004ff34862eb2cd6c..27191a970b3e9d569eda3af305706d41009426c1 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.test.tsx @@ -9,6 +9,14 @@ vi.mock("~/browser/useOpenLink", () => ({ useOpenLink: () => vi.fn() })); vi.mock("./PullRequestMarkdown", () => ({ PullRequestMarkdown: ({ text }: { text: string }) =>

{text}

, })); +vi.mock("../ui/menu", () => ({ + Menu: "div", + MenuCheckboxItem: "div", + MenuGroup: "div", + MenuGroupLabel: "div", + MenuPopup: "div", + MenuTrigger: "div", +})); vi.mock("../ui/tooltip", () => ({ Tooltip: ({ children }: { children: ReactNode }) => children, TooltipTrigger: ({ children }: { children: ReactNode }) => children, @@ -70,7 +78,20 @@ const detail: PullRequestDetailView = { }; let renderer: ReactTestRenderer; -beforeEach(() => vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true)); +beforeEach(() => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + const store = new Map(); + vi.stubGlobal( + "window", + Object.assign(new EventTarget(), { + localStorage: { + getItem: (key: string) => store.get(key) ?? null, + setItem: (key: string, value: string) => store.set(key, value), + removeItem: (key: string) => store.delete(key), + }, + }), + ); +}); afterEach(() => { act(() => renderer?.unmount()); vi.unstubAllGlobals(); @@ -145,70 +166,80 @@ it("keeps an unsaved description when collapsed and reopened", () => { expect(renderer.root.findByType("textarea").props.value).toBe("Unsaved description"); }); -it("opens bot reports in pages without hiding human comments", () => { +function texts() { + return renderer.root + .findAllByType("p") + .map((p) => p.children.join("")) + .filter((text) => text !== detail.body); +} + +function filterItem(label: string) { + return renderer.root.find( + (node) => typeof node.props.onCheckedChange === "function" && node.children.includes(label), + ); +} + +it("hides bot and resolved comments until the filter shows them, and remembers the choice", () => { + const comment = (index: number, login: string, body: string, isBot = false) => ({ + id: `comment-${index}`, + kind: "issue-comment" as const, + author: { login, name: null, avatarUrl: null, isBot }, + body, + createdAt: `2026-09-01T00:00:${String(index).padStart(2, "0")}Z`, + url: null, + path: null, + reviewState: null, + }); + const resolved = { + ...comment(13, "reviewer", "Resolved remark"), + kind: "review-comment" as const, + }; 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, + commentCount: 14, + comments: [ + ...Array.from({ length: 12 }, (_, index) => + comment(index, "review-app", `Bot report ${index}`, true), + ), + comment(12, "human", "Human comment"), + resolved, + ], + reviewThreads: [ + { + id: "thread", + path: "src/index.ts", + line: 1, + side: "right", + isResolved: true, + isOutdated: false, + comments: [resolved], }, - 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(texts()).toEqual(["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() {} })); + renderer.root.findByProps({ "aria-label": "Filter comments" }).children.join(""), + ).toContain("13 hidden"); + + act(() => filterItem("Bot comments").props.onCheckedChange(true)); + // Bots now share the human comment's pages: the ten most recent, the oldest left behind. + expect(texts().filter((text) => text.startsWith("Bot report"))).toHaveLength(9); + expect(texts()).toContain("Human comment"); + expect(texts()).not.toContain("Bot report 0"); + + act(() => filterItem("Resolved or dismissed").props.onCheckedChange(true)); 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); + renderer.root.findAllByType("button").some((button) => button.children.includes("Resolved")), + ).toBe(true); + + // Another pull request, or the same one opened again, keeps the reader's choice. + act(() => renderer.unmount()); + act(() => { + renderer = create(render({ ...value, url: `${value.url}0`, number: 10 })); + }); + expect(filterItem("Bot comments").props.checked).toBe(true); + expect(texts()).toContain("Bot report 11"); }); diff --git a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx index 581c4d0741d7f100433d71ff44d066a09c19b12f..9fae094ce56442ebbf58922abc5a52cb26042895 100644 --- a/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx @@ -12,12 +12,15 @@ import { ChevronDownIcon, ChevronRightIcon, HammerIcon, + ListFilterIcon, TagIcon, UsersIcon, } from "lucide-react"; +import * as Schema from "effect/Schema"; import { useRef, useState, type ReactNode } from "react"; import { pullRequestEnvironment } from "~/state/pullRequests"; +import { useLocalStorage } from "~/hooks/useLocalStorage"; import { cn } from "~/lib/utils"; import { useOpenLink } from "~/browser/useOpenLink"; import { formatRelativeTimeLabel } from "~/timestampFormat"; @@ -25,6 +28,14 @@ import { formatRelativeTimeLabel } from "~/timestampFormat"; import { Button } from "../ui/button"; import { PullRequestEditButton } from "./PullRequestEditButton"; import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible"; +import { + Menu, + MenuCheckboxItem, + MenuGroup, + MenuGroupLabel, + MenuPopup, + MenuTrigger, +} from "../ui/menu"; import { toastManager } from "../ui/toast"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { @@ -349,111 +360,21 @@ 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) => ( - // The ring separates the overlapping faces; it belongs to the stack, not the 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 = 10; +/** + * Which kinds of comment the conversation shows. Kept per browser rather than per pull request: + * whether a reader wants bot reports and finished threads in front of them is a habit, not a + * property of one change. + */ +const COMMENT_FILTER_STORAGE_KEY = "t3code:pull-request-comment-filter:v1"; +const CommentFilterSchema = Schema.Struct({ bots: Schema.Boolean, resolved: Schema.Boolean }); +const DEFAULT_COMMENT_FILTER: typeof CommentFilterSchema.Type = { bots: false, resolved: false }; + export function PullRequestSummaryTab({ environmentId, threadRef, @@ -487,10 +408,12 @@ 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; + const [commentFilter, setCommentFilter] = useLocalStorage( + COMMENT_FILTER_STORAGE_KEY, + DEFAULT_COMMENT_FILTER, + CommentFilterSchema, + ); // 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. @@ -500,22 +423,24 @@ export function PullRequestSummaryTab({ ), ); - 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); - } + 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)), + ); + // The one count the filter shows. Per-kind counts would overlap, since a resolved bot comment + // is both kinds and needs both shown. + const filteredOutCount = detail.comments.length - filteredComments.length; // 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 = 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 recentComments = filteredComments.slice( + Math.max(0, filteredComments.length - shownComments), + ); + const hiddenCommentCount = filteredComments.length - recentComments.length; const [commentOrder, setCommentOrder] = useState<"newest" | "oldest">("newest"); const visibleComments = orderPullRequestComments(recentComments, commentOrder); const showOldestCommentsButton = @@ -638,6 +563,30 @@ export function PullRequestSummaryTab({ const renderComment = (comment: PullRequestComment) => { const thread = threadByCommentId.get(comment.id); const body = visibleBody(comment.body); + if (isFinished(comment)) { + return ( + + } + /> + ); + } const outcome = pullRequestReviewOutcome(comment.reviewState); // An approval is a verdict, not a finding: there is nothing in it to fix. const finding: PullRequestFinding | null = @@ -920,20 +869,51 @@ export function PullRequestSummaryTab({
setCommentOrder((value) => (value === "newest" ? "oldest" : "newest"))} - > - - {commentOrder === "newest" ? "Newest first" : "Oldest first"} - +
+ + } + aria-label="Filter comments" + > + + {filteredOutCount > 0 ? `${filteredOutCount} hidden` : "Filter"} + + + + Show + setCommentFilter((value) => ({ ...value, bots }))} + > + Bot comments + + + setCommentFilter((value) => ({ ...value, resolved })) + } + > + Resolved or dismissed + + + + + +
} > {activityPending ? ( @@ -950,6 +930,12 @@ export function PullRequestSummaryTab({ ) : null} {detail.comments.length === 0 ? (

No comments yet.

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

+ {filteredOutCount === 1 + ? "The only comment is hidden by the filter." + : `All ${filteredOutCount} comments are hidden by the filter.`} +

) : (
{commentOrder === "oldest" ? showOldestCommentsButton : null} @@ -965,87 +951,6 @@ export function PullRequestSummaryTab({ Show only {COMMENT_PAGE} recent comments ) : 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 ( - - } - /> - ); - })} -
- - ) : null}
)} -- 2.54.0