From e4abdaf54efb31ec26cf8cf02f2d6c182504d05a Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:55:04 +0000 Subject: [PATCH 1/2] fix: collapse resolved comment threads by default in diff view --- src/browser/components/pr-overview.tsx | 33 ++------ src/browser/components/pr-review.tsx | 22 +++-- .../contexts/pr-review/enrichComments.test.ts | 80 +++++++++++++++++++ .../contexts/pr-review/enrichComments.ts | 37 +++++++++ src/browser/contexts/pr-review/index.tsx | 1 + .../contexts/pr-review/useReviewActions.ts | 24 ++++-- .../contexts/pr-review/useThreadActions.ts | 66 ++++++++------- 7 files changed, 192 insertions(+), 71 deletions(-) create mode 100644 src/browser/contexts/pr-review/enrichComments.test.ts create mode 100644 src/browser/contexts/pr-review/enrichComments.ts diff --git a/src/browser/components/pr-overview.tsx b/src/browser/components/pr-overview.tsx index 68b258f..3b763ef 100644 --- a/src/browser/components/pr-overview.tsx +++ b/src/browser/components/pr-overview.tsx @@ -55,6 +55,7 @@ import { import { usePRReviewSelector, usePRReviewStore, + useThreadActions, getTimeAgo, } from "../contexts/pr-review"; import { parseDiffCached, type ParsedDiff } from "../lib/diff"; @@ -794,34 +795,10 @@ export const PROverview = memo(function PROverview() { [github, owner, repo, pr.number, store] ); - const handleResolveThread = useCallback( - async (threadId: string) => { - try { - await github.resolveThread(threadId); - // Update local state - store.updateReviewThread(threadId, (t) => ({ ...t, isResolved: true })); - } catch (error) { - console.error("Failed to resolve thread:", error); - } - }, - [github, store] - ); - - const handleUnresolveThread = useCallback( - async (threadId: string) => { - try { - await github.unresolveThread(threadId); - // Update local state - store.updateReviewThread(threadId, (t) => ({ - ...t, - isResolved: false, - })); - } catch (error) { - console.error("Failed to unresolve thread:", error); - } - }, - [github, store] - ); + const { + resolveThread: handleResolveThread, + unresolveThread: handleUnresolveThread, + } = useThreadActions(); // Calculate check status const checkStatus = calculateCheckStatus( diff --git a/src/browser/components/pr-review.tsx b/src/browser/components/pr-review.tsx index d8fcc90..acdfd26 100644 --- a/src/browser/components/pr-review.tsx +++ b/src/browser/components/pr-review.tsx @@ -72,6 +72,7 @@ import { useFileCopyActions, useSkipBlockExpansion, useThreadActions, + enrichCommentsWithThreads, useCurrentFile, useCurrentDiff, useIsCurrentFileLoading, @@ -227,7 +228,12 @@ export function PRReviewContent({ setPr(prData); setFiles(filesData); - setComments(commentsData as ReviewComment[]); + setComments( + enrichCommentsWithThreads( + commentsData as ReviewComment[], + reviewThreadsResult.threads + ) + ); setViewerPermission(reviewThreadsResult.viewerPermission); setViewerCanMergeAsAdmin(reviewThreadsResult.viewerCanMergeAsAdmin); @@ -2499,19 +2505,19 @@ const CommentThread = memo(function CommentThread({ const repo = usePRReviewSelector((s) => s.repo); const { replyToComment, updateComment, deleteComment } = useCommentActions(); const { resolveThread, unresolveThread } = useThreadActions(); + // Get resolution info from first comment (all comments in thread share same resolution status) + const firstComment = comments[0]; + const isResolved = firstComment?.is_resolved ?? false; + const threadId = firstComment?.pull_request_review_thread_id; + const [replyText, setReplyText] = useState(""); const [submitting, setSubmitting] = useState(false); - const [isCollapsed, setIsCollapsed] = useState(false); + const [isCollapsed, setIsCollapsed] = useState(isResolved); const [resolving, setResolving] = useState(false); const replyingTo = comments.find((c) => c.id === replyingToCommentId)?.id ?? null; - // Get resolution info from first comment (all comments in thread share same resolution status) - const firstComment = comments[0]; - const isResolved = firstComment?.is_resolved ?? false; - const threadId = firstComment?.pull_request_review_thread_id; - const handleSubmitReply = useCallback(async () => { if (!replyText.trim() || !replyingTo) return; @@ -2604,7 +2610,7 @@ const CommentThread = memo(function CommentThread({ {isResolved && isCollapsed && ( - by {firstComment.user.login} + by {firstComment.resolved_by?.login ?? firstComment.user.login} )} diff --git a/src/browser/contexts/pr-review/enrichComments.test.ts b/src/browser/contexts/pr-review/enrichComments.test.ts new file mode 100644 index 0000000..31f788b --- /dev/null +++ b/src/browser/contexts/pr-review/enrichComments.test.ts @@ -0,0 +1,80 @@ +import { test, expect } from "bun:test"; +import type { ReviewComment } from "@/api/types"; +import type { ReviewThread } from "@/browser/contexts/github"; +import { enrichCommentsWithThreads } from "./enrichComments"; + +function createComment(id: number): ReviewComment { + return { id, body: `comment ${id}` } as ReviewComment; +} + +function createThread( + id: string, + commentIds: number[], + options?: { + isResolved?: boolean; + resolvedBy?: { login: string; avatarUrl: string } | null; + } +): ReviewThread { + return { + id, + isResolved: options?.isResolved ?? false, + resolvedBy: options?.resolvedBy ?? null, + pullRequestReview: null, + comments: { + nodes: commentIds.map((databaseId) => ({ + id: `gql-${databaseId}`, + databaseId, + body: "", + path: "file.ts", + line: 1, + originalLine: 1, + startLine: null, + diffHunk: null, + author: null, + createdAt: "", + updatedAt: "", + replyTo: null, + })), + }, + }; +} + +test("enrichCommentsWithThreads attaches resolution info to every comment in a thread", () => { + const comments = [createComment(1), createComment(2), createComment(3)]; + const threads = [ + createThread("THREAD_A", [1, 2], { + isResolved: true, + resolvedBy: { login: "alice", avatarUrl: "https://a.png" }, + }), + createThread("THREAD_B", [3]), + ]; + + const enriched = enrichCommentsWithThreads(comments, threads); + + expect(enriched[0]?.pull_request_review_thread_id).toBe("THREAD_A"); + expect(enriched[0]?.is_resolved).toBe(true); + expect(enriched[0]?.resolved_by).toEqual({ + login: "alice", + avatar_url: "https://a.png", + }); + expect(enriched[1]?.is_resolved).toBe(true); + expect(enriched[2]?.pull_request_review_thread_id).toBe("THREAD_B"); + expect(enriched[2]?.is_resolved).toBe(false); + expect(enriched[2]?.resolved_by).toBeNull(); +}); + +test("enrichCommentsWithThreads leaves comments without a matching thread untouched", () => { + const comments = [createComment(1), createComment(99)]; + const threads = [createThread("THREAD_A", [1], { isResolved: true })]; + + const enriched = enrichCommentsWithThreads(comments, threads); + + expect(enriched[0]?.is_resolved).toBe(true); + expect(enriched[1]).toBe(comments[1]!); + expect(enriched[1]?.is_resolved).toBeUndefined(); +}); + +test("enrichCommentsWithThreads returns comments as-is when there are no threads", () => { + const comments = [createComment(1)]; + expect(enrichCommentsWithThreads(comments, [])).toBe(comments); +}); diff --git a/src/browser/contexts/pr-review/enrichComments.ts b/src/browser/contexts/pr-review/enrichComments.ts new file mode 100644 index 0000000..6135e24 --- /dev/null +++ b/src/browser/contexts/pr-review/enrichComments.ts @@ -0,0 +1,37 @@ +import type { ReviewComment } from "@/api/types"; +import type { ReviewThread } from "@/browser/contexts/github"; + +/** + * Join GraphQL review-thread resolution info onto REST review comments. + * REST comments carry no thread ID or resolution state, so without this + * enrichment resolved threads render as unresolved. + */ +export function enrichCommentsWithThreads( + comments: ReviewComment[], + threads: ReviewThread[] +): ReviewComment[] { + if (threads.length === 0) return comments; + + const threadByCommentId = new Map(); + for (const thread of threads) { + for (const comment of thread.comments.nodes) { + threadByCommentId.set(comment.databaseId, thread); + } + } + + return comments.map((comment) => { + const thread = threadByCommentId.get(comment.id); + if (!thread) return comment; + return { + ...comment, + pull_request_review_thread_id: thread.id, + is_resolved: thread.isResolved, + resolved_by: thread.resolvedBy + ? { + login: thread.resolvedBy.login, + avatar_url: thread.resolvedBy.avatarUrl, + } + : null, + }; + }); +} diff --git a/src/browser/contexts/pr-review/index.tsx b/src/browser/contexts/pr-review/index.tsx index b49cbb5..4919673 100644 --- a/src/browser/contexts/pr-review/index.tsx +++ b/src/browser/contexts/pr-review/index.tsx @@ -2758,6 +2758,7 @@ export { useDiffLoader } from "./useDiffLoader"; export { useCurrentUserLoader } from "./useCurrentUserLoader"; export { usePendingReviewLoader } from "./usePendingReviewLoader"; export { useThreadActions } from "./useThreadActions"; +export { enrichCommentsWithThreads } from "./enrichComments"; export { useCommentActions } from "./useCommentActions"; export { useReviewActions } from "./useReviewActions"; export { useSkipBlockExpansion } from "./useSkipBlockExpansion"; diff --git a/src/browser/contexts/pr-review/useReviewActions.ts b/src/browser/contexts/pr-review/useReviewActions.ts index 790611e..fd45cf6 100644 --- a/src/browser/contexts/pr-review/useReviewActions.ts +++ b/src/browser/contexts/pr-review/useReviewActions.ts @@ -2,6 +2,7 @@ import type { ReviewComment } from "@/api/types"; import { useGitHub, type Review } from "@/browser/contexts/github"; import { useTelemetry } from "@/browser/contexts/telemetry"; import { usePRReviewStore, usePRReviewSelector } from "."; +import { enrichCommentsWithThreads } from "./enrichComments"; export function useReviewActions() { const store = usePRReviewStore(); @@ -65,13 +66,22 @@ export function useReviewActions() { // Invalidate timeline cache so we get fresh data github.invalidateCache(`pr:${owner}/${repo}/${pr.number}:timeline`); - // Refresh comments, reviews, and timeline - const [newComments, reviews, timeline] = await Promise.all([ - github.getPRComments(owner, repo, pr.number), - github.getPRReviews(owner, repo, pr.number), - github.getPRTimeline(owner, repo, pr.number), - ]); - store.setComments(newComments as ReviewComment[]); + // Refresh comments, reviews, timeline, and review threads + const [newComments, reviews, timeline, reviewThreadsResult] = + await Promise.all([ + github.getPRComments(owner, repo, pr.number), + github.getPRReviews(owner, repo, pr.number), + github.getPRTimeline(owner, repo, pr.number), + github.getReviewThreads(owner, repo, pr.number).catch(() => null), + ]); + const threads = + reviewThreadsResult?.threads ?? store.getSnapshot().reviewThreads; + store.setComments( + enrichCommentsWithThreads(newComments as ReviewComment[], threads) + ); + if (reviewThreadsResult) { + store.setReviewThreads(reviewThreadsResult.threads); + } store.setReviews(reviews); store.setTimeline(timeline); diff --git a/src/browser/contexts/pr-review/useThreadActions.ts b/src/browser/contexts/pr-review/useThreadActions.ts index 68dfc5f..aba65d3 100644 --- a/src/browser/contexts/pr-review/useThreadActions.ts +++ b/src/browser/contexts/pr-review/useThreadActions.ts @@ -1,3 +1,4 @@ +import { useCallback } from "react"; import { useGitHub } from "@/browser/contexts/github"; import { usePRReviewStore } from "."; @@ -5,37 +6,46 @@ export function useThreadActions() { const store = usePRReviewStore(); const github = useGitHub(); - const resolveThread = async (threadId: string) => { - try { - await github.resolveThread(threadId); - // Update local state - mark all comments in this thread as resolved + // Update both comments (diff view) and reviewThreads (overview) so + // resolution state stays in sync across views. + const setThreadResolved = useCallback( + (threadId: string, isResolved: boolean) => { const state = store.getSnapshot(); - const updatedComments = state.comments.map((c) => - c.pull_request_review_thread_id === threadId - ? { ...c, is_resolved: true } - : c + store.setComments( + state.comments.map((c) => + c.pull_request_review_thread_id === threadId + ? { ...c, is_resolved: isResolved } + : c + ) ); - store.setComments(updatedComments); - } catch (error) { - console.error("Failed to resolve thread:", error); - } - }; + store.updateReviewThread(threadId, (t) => ({ ...t, isResolved })); + }, + [store] + ); - const unresolveThread = async (threadId: string) => { - try { - await github.unresolveThread(threadId); - // Update local state - mark all comments in this thread as unresolved - const state = store.getSnapshot(); - const updatedComments = state.comments.map((c) => - c.pull_request_review_thread_id === threadId - ? { ...c, is_resolved: false } - : c - ); - store.setComments(updatedComments); - } catch (error) { - console.error("Failed to unresolve thread:", error); - } - }; + const resolveThread = useCallback( + async (threadId: string) => { + try { + await github.resolveThread(threadId); + setThreadResolved(threadId, true); + } catch (error) { + console.error("Failed to resolve thread:", error); + } + }, + [github, setThreadResolved] + ); + + const unresolveThread = useCallback( + async (threadId: string) => { + try { + await github.unresolveThread(threadId); + setThreadResolved(threadId, false); + } catch (error) { + console.error("Failed to unresolve thread:", error); + } + }, + [github, setThreadResolved] + ); return { resolveThread, unresolveThread }; } From 0ee8d9077f88ab636905afb7d15b00094e7abc54 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:59:54 +0000 Subject: [PATCH 2/2] fix: address cleanup audit findings for resolved thread collapse --- src/browser/components/pr-review.tsx | 13 ++++--------- src/browser/contexts/pr-review/useReviewActions.ts | 1 - 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/browser/components/pr-review.tsx b/src/browser/components/pr-review.tsx index acdfd26..1d225cf 100644 --- a/src/browser/components/pr-review.tsx +++ b/src/browser/components/pr-review.tsx @@ -2555,8 +2555,6 @@ const CommentThread = memo(function CommentThread({ setResolving(true); try { await resolveThread(threadId); - // Auto-collapse when resolved - setIsCollapsed(true); } finally { setResolving(false); } @@ -2567,17 +2565,14 @@ const CommentThread = memo(function CommentThread({ setResolving(true); try { await unresolveThread(threadId); - setIsCollapsed(false); } finally { setResolving(false); } }, [threadId, unresolveThread]); - // Auto-collapse resolved threads + // Sync collapse state on resolution changes, including from other views useEffect(() => { - if (isResolved) { - setIsCollapsed(true); - } + setIsCollapsed(isResolved); }, [isResolved]); return ( @@ -2608,9 +2603,9 @@ const CommentThread = memo(function CommentThread({ ? "Resolved" : `${comments.length} comment${comments.length !== 1 ? "s" : ""}`} - {isResolved && isCollapsed && ( + {isResolved && isCollapsed && firstComment.resolved_by?.login && ( - by {firstComment.resolved_by?.login ?? firstComment.user.login} + by {firstComment.resolved_by.login} )} diff --git a/src/browser/contexts/pr-review/useReviewActions.ts b/src/browser/contexts/pr-review/useReviewActions.ts index fd45cf6..9fb0bef 100644 --- a/src/browser/contexts/pr-review/useReviewActions.ts +++ b/src/browser/contexts/pr-review/useReviewActions.ts @@ -66,7 +66,6 @@ export function useReviewActions() { // Invalidate timeline cache so we get fresh data github.invalidateCache(`pr:${owner}/${repo}/${pr.number}:timeline`); - // Refresh comments, reviews, timeline, and review threads const [newComments, reviews, timeline, reviewThreadsResult] = await Promise.all([ github.getPRComments(owner, repo, pr.number),