From b900c2adf91670aaba3796bb021c43ec59d8c46d Mon Sep 17 00:00:00 2001 From: omartelo Date: Mon, 3 Aug 2026 10:13:37 -0300 Subject: [PATCH] fix: cut a review thread's snippet to the lines it comments on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub sends a review comment with the whole hunk it sits in, and the Conversation tab printed it verbatim: on a file the branch adds, a remark about ten lines arrived under a hundred and twenty of them. The snippet is now parsed and cut to the span the thread covers — the range for a multi-line thread, the line plus a little context for a single one — with a fallback to the hunk's tail for a thread GitHub re-anchored, since the hunk always ends on the line the comment was filed on. The lines carry the diff viewer's own green and red, numbered from the hunk header, and the header's file reference reads the whole range. The diff colours lose their cm- prefix along the way: they are no longer the CodeMirror viewer's alone. Threads GitHub marks outdated null their startLine, so the query asks for originalStartLine too — without it an outdated range collapsed to one line and took the snippet with it. --- CHANGELOG.md | 7 +++ .../src/components/pulls/ReviewThread.tsx | 20 +++++-- frontend/src/components/pulls/ThreadHunk.tsx | 50 +++++++++++++++++ frontend/src/index.css | 15 +++--- frontend/src/lib/codemirror.ts | 8 +-- frontend/src/lib/pulls/thread-hunk.test.ts | 53 +++++++++++++++++++ frontend/src/lib/pulls/thread-hunk.ts | 40 ++++++++++++++ internal/project/prthreads.go | 32 +++++++---- internal/project/prthreads_test.go | 20 +++++-- 9 files changed, 215 insertions(+), 30 deletions(-) create mode 100644 frontend/src/components/pulls/ThreadHunk.tsx create mode 100644 frontend/src/lib/pulls/thread-hunk.test.ts create mode 100644 frontend/src/lib/pulls/thread-hunk.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fad618b20..d516ab6ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **A thread in Conversation shows the lines it is about, not the file.** GitHub + sends a review comment with the whole hunk it sits in, which on a file the + branch adds is the entire file — so a remark about ten lines arrived under a + hundred and twenty, and finding the ones it meant was a scroll. The snippet is + now cut to the commented span (with a little context above a single-line one), + its lines carry the diff viewer's own green and red, and the file reference in + the header reads the whole range: `utils.java:65-74`. - **A refused merge says which kind of refusal it was.** gh writes one sentence for two very different failures and puts the difference in its tail; lich read only the opening and claimed conflicts either way, sending you to look for diff --git a/frontend/src/components/pulls/ReviewThread.tsx b/frontend/src/components/pulls/ReviewThread.tsx index 515347bee..78ef8aeed 100644 --- a/frontend/src/components/pulls/ReviewThread.tsx +++ b/frontend/src/components/pulls/ReviewThread.tsx @@ -13,9 +13,11 @@ import { Markdown } from "@/components/Markdown" import { Button } from "@/components/ui/button" import { IconAction } from "@/components/common/IconAction" import type { DraftReviewComment, ReviewThread as Thread } from "@/lib/api-types" +import { formatLineRef } from "@/lib/git/diff" import { cn, errorText } from "@/lib/utils" import { Byline } from "./Byline" import { CommentBox } from "./CommentBox" +import { ThreadHunk } from "./ThreadHunk" // What a thread card can do to the thread it renders. Both actions are the // screen's, not this component's: it knows how to ask, never which pull request @@ -37,6 +39,14 @@ interface ReviewThreadProps { className?: string } +/** The lines a thread covers, as a file reference reads them: "74" or "65-74". */ +function lineRef(thread: Thread): string { + return formatLineRef({ + start: thread.startLine > 0 ? thread.startLine : thread.line, + end: thread.line, + }) +} + // One review thread: what was said, in order, with a way to answer it and a way // to close it. Rendered twice over — inside the diff, in the gap the widget // opens under the line, and again in the Conversation tab for the threads no @@ -118,7 +128,7 @@ export function ReviewThread({ {standalone ? ( {thread.path} - {thread.line > 0 && `:${thread.line}`} + {thread.line > 0 && `:${lineRef(thread)}`} ) : ( Line {thread.line} @@ -142,10 +152,10 @@ export function ReviewThread({ - {open && standalone && last?.diffHunk && ( - 
-          {last.diffHunk}
-
+ {/* The hunk of the comment the thread was filed on: a reply carries one + too, but the thread's line numbers are written against the first. */} + {open && standalone && comments[0]?.diffHunk && ( + )} {open && ( diff --git a/frontend/src/components/pulls/ThreadHunk.tsx b/frontend/src/components/pulls/ThreadHunk.tsx new file mode 100644 index 000000000..ef30f4c1d --- /dev/null +++ b/frontend/src/components/pulls/ThreadHunk.tsx @@ -0,0 +1,50 @@ +import type { ReviewThread } from "@/lib/api-types" +import { gutterNumber } from "@/lib/git/diff" +import { threadHunk } from "@/lib/pulls/thread-hunk" +import { cn } from "@/lib/utils" + +interface ThreadHunkProps { + /** GitHub's own hunk, as it travels with the comment. */ + diffHunk: string + thread: ReviewThread +} + +// The lines a review thread was filed on, coloured and numbered the way the +// diff viewer colours them. The diff renders them itself where the thread is +// anchored; this is for the Conversation tab, where the file is not on screen. +export function ThreadHunk({ diffHunk, thread }: ThreadHunkProps) { + const lines = threadHunk(diffHunk, thread) + if (lines.length === 0) { + return null + } + + return ( +
+ {/* w-max so a line wider than the card carries its tint the whole scrolled + width instead of stopping at the viewport edge. */} +
+ {lines.map((line) => ( +
+ + {gutterNumber(line)} + + {line.text} +
+ ))} +
+
+ ) +} diff --git a/frontend/src/index.css b/frontend/src/index.css index f6a61c5d6..e7b719008 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -150,28 +150,29 @@ } } -/* Diff viewer (CodeMirror line + gutter decorations). Classic emerald/red for +/* Diff colours, worn by the CodeMirror viewer's line and gutter decorations and + by the review-thread snippets that render outside it. Classic emerald/red for add/del — the footer's counters keep the app's sky/pink identity. The 3px strip lives on the gutter cell (leftmost edge) as an inset box-shadow, which doesn't shift content the way border-left would. */ -.cm-diff-add { +.diff-add { background-color: color-mix(in oklab, var(--color-emerald-500) 12%, transparent); } -.cm-diff-del { +.diff-del { background-color: color-mix(in oklab, var(--color-red-500) 10%, transparent); } -.dark .cm-diff-add { +.dark .diff-add { background-color: color-mix(in oklab, var(--color-emerald-400) 14%, transparent); } -.dark .cm-diff-del { +.dark .diff-del { background-color: color-mix(in oklab, var(--color-red-400) 12%, transparent); } -.cm-diff-gutter-add { +.diff-gutter-add { background-color: color-mix(in oklab, var(--color-emerald-500) 18%, transparent); box-shadow: inset 3px 0 0 0 var(--color-emerald-500); } -.cm-diff-gutter-del { +.diff-gutter-del { background-color: color-mix(in oklab, var(--color-red-500) 15%, transparent); box-shadow: inset 3px 0 0 0 var(--color-red-500); } diff --git a/frontend/src/lib/codemirror.ts b/frontend/src/lib/codemirror.ts index 207f67d06..c56e3ceb4 100644 --- a/frontend/src/lib/codemirror.ts +++ b/frontend/src/lib/codemirror.ts @@ -117,8 +117,8 @@ export function loadLanguage(view: EditorView, filename: string, isAlive: () => } const lineClasses: Partial> = { - add: Decoration.line({ class: "cm-diff-add" }), - del: Decoration.line({ class: "cm-diff-del" }), + add: Decoration.line({ class: "diff-add" }), + del: Decoration.line({ class: "diff-del" }), meta: Decoration.line({ class: "cm-diff-sep" }), } @@ -131,8 +131,8 @@ class LineClassMarker extends GutterMarker { } const gutterMarkers: Partial> = { - add: new LineClassMarker("cm-diff-gutter-add"), - del: new LineClassMarker("cm-diff-gutter-del"), + add: new LineClassMarker("diff-gutter-add"), + del: new LineClassMarker("diff-gutter-del"), } // buildLineDecorations colors added/deleted lines and hunk separators, in the diff --git a/frontend/src/lib/pulls/thread-hunk.test.ts b/frontend/src/lib/pulls/thread-hunk.test.ts new file mode 100644 index 000000000..c43fb76f8 --- /dev/null +++ b/frontend/src/lib/pulls/thread-hunk.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vitest" +import { threadHunk } from "./thread-hunk" + +// A hunk shaped like the one an added file produces: every line new, numbered +// from the header, ending on the line the comment was filed on. +function addedFile(upTo: number): string { + const lines = ["@@ -0,0 +1,120 @@"] + for (let n = 1; n <= upTo; n += 1) { + lines.push(`+line ${n}`) + } + return lines.join("\n") +} + +describe("threadHunk", () => { + it("cuts a multi-line thread down to the span it covers", () => { + const lines = threadHunk(addedFile(74), { line: 74, startLine: 65, side: "RIGHT" }) + expect(lines.map((l) => l.newLine)).toEqual([65, 66, 67, 68, 69, 70, 71, 72, 73, 74]) + expect(lines[0].text).toBe("+line 65") + }) + + it("keeps a little context above a single-line thread", () => { + const lines = threadHunk(addedFile(74), { line: 74, startLine: 0, side: "RIGHT" }) + expect(lines.map((l) => l.newLine)).toEqual([71, 72, 73, 74]) + }) + + it("falls back to the hunk's tail when the anchor moved off it", () => { + // A re-anchored thread: line 900 is nowhere in the hunk, but the hunk still + // ends on the line it was filed on. + const lines = threadHunk(addedFile(74), { line: 900, startLine: 0, side: "RIGHT" }) + expect(lines.map((l) => l.newLine)).toEqual([71, 72, 73, 74]) + }) + + it("caps a span nobody wants to scroll", () => { + const lines = threadHunk(addedFile(74), { line: 74, startLine: 2, side: "RIGHT" }) + expect(lines).toHaveLength(24) + expect(lines[lines.length - 1].newLine).toBe(74) + }) + + it("numbers a deleted line on the side it was deleted from", () => { + const hunk = ["@@ -57,4 +57,3 @@", " kept", "-gone", " after", "+added"].join("\n") + const lines = threadHunk(hunk, { line: 58, startLine: 0, side: "LEFT" }) + expect(lines.map((l) => [l.kind, l.oldLine, l.newLine])).toEqual([ + ["context", 57, 57], + ["del", 58, null], + ["context", 59, 58], + ["add", null, 59], + ]) + }) + + it("survives a hunk it cannot parse", () => { + expect(threadHunk("", { line: 12, startLine: 0, side: "RIGHT" })).toEqual([]) + }) +}) diff --git a/frontend/src/lib/pulls/thread-hunk.ts b/frontend/src/lib/pulls/thread-hunk.ts new file mode 100644 index 000000000..6b9c23164 --- /dev/null +++ b/frontend/src/lib/pulls/thread-hunk.ts @@ -0,0 +1,40 @@ +import { type DiffLine, type DiffSide, parseDiff } from "@/lib/git/diff" + +// GitHub's diffHunk is the whole hunk up to the line the comment was filed on — +// on a file the branch adds, that is the entire file. What the thread is about +// is the tail of it, so the snippet is cut down to the commented span. + +/** Lines of context kept above a thread that covers a single line. */ +const CONTEXT = 3 + +/** The span a thread covers: what a ReviewThread carries about its anchor. */ +export interface ThreadAnchor { + line: number + startLine: number + side: string +} + +/** How many lines a snippet shows at most, whatever the anchor says — a stray + * span must not put the whole file back on screen. */ +const MAX_LINES = 24 + +// threadHunk parses one GitHub diff hunk and returns the lines the thread was +// filed on, numbered. An anchor whose numbers the hunk does not hold (a thread +// GitHub re-anchored onto a rewritten file) falls back to the hunk's last lines: +// the hunk always ends on the commented line, so its tail is the span even when +// the numbers have moved. +export function threadHunk(diffHunk: string, anchor: ThreadAnchor): DiffLine[] { + // parseDiff wants a file header above the hunk; the path is never read back. + const [file] = parseDiff(`diff --git a/hunk b/hunk\n${diffHunk}`) + const lines = file?.hunks[0]?.lines ?? [] + const side: DiffSide = anchor.side === "LEFT" ? "LEFT" : "RIGHT" + const first = anchor.startLine > 0 ? anchor.startLine : anchor.line + const at = lines.findIndex((line) => { + const number = side === "LEFT" ? line.oldLine : line.newLine + return number !== null && number >= first + }) + // A span states its own first line; a single-line thread reads better with a + // few lines above it than alone. + const from = at === -1 ? lines.length - 1 - CONTEXT : at - (anchor.startLine > 0 ? 0 : CONTEXT) + return lines.slice(Math.max(0, from, lines.length - MAX_LINES)) +} diff --git a/internal/project/prthreads.go b/internal/project/prthreads.go index c5825d169..c0e304e23 100644 --- a/internal/project/prthreads.go +++ b/internal/project/prthreads.go @@ -20,7 +20,7 @@ const prConversationQuery = `query($owner:String!,$repo:String!,$number:Int!){ reviews(last:50){nodes{author{login} state body submittedAt}} comments(last:50){nodes{author{login} body createdAt}} reviewThreads(last:100){nodes{ - id isResolved isOutdated path line originalLine startLine diffSide + id isResolved isOutdated path line originalLine startLine originalStartLine diffSide comments(first:50){nodes{databaseId author{login} body createdAt diffHunk}} }} } @@ -135,15 +135,16 @@ type ghIssueComment struct { } type ghReviewThread struct { - ID string `json:"id"` - IsResolved bool `json:"isResolved"` - IsOutdated bool `json:"isOutdated"` - Path string `json:"path"` - Line *int `json:"line"` - OriginalLine *int `json:"originalLine"` - StartLine *int `json:"startLine"` - DiffSide string `json:"diffSide"` - Comments struct { + ID string `json:"id"` + IsResolved bool `json:"isResolved"` + IsOutdated bool `json:"isOutdated"` + Path string `json:"path"` + Line *int `json:"line"` + OriginalLine *int `json:"originalLine"` + StartLine *int `json:"startLine"` + OriginalStartLine *int `json:"originalStartLine"` + DiffSide string `json:"diffSide"` + Comments struct { Nodes []ghThreadComment `json:"nodes"` } `json:"comments"` } @@ -234,7 +235,7 @@ func toThreads(nodes []ghReviewThread) []PRReviewThread { ID: t.ID, Path: t.Path, Line: threadLine(t), - StartLine: deref(t.StartLine), + StartLine: threadStartLine(t), Side: t.DiffSide, IsResolved: t.IsResolved, IsOutdated: t.IsOutdated, @@ -255,6 +256,15 @@ func threadLine(t ghReviewThread) int { return deref(t.OriginalLine) } +// threadStartLine is the same fallback for the range's first line: an outdated +// thread keeps the original, which is the number its diff hunk is written in. +func threadStartLine(t ghReviewThread) int { + if t.StartLine != nil { + return *t.StartLine + } + return deref(t.OriginalStartLine) +} + func deref(v *int) int { if v == nil { return 0 diff --git a/internal/project/prthreads_test.go b/internal/project/prthreads_test.go index de94b0c90..939c15765 100644 --- a/internal/project/prthreads_test.go +++ b/internal/project/prthreads_test.go @@ -28,6 +28,11 @@ const conversationPayload = `{"data":{"repository":{"pullRequest":{ "line":null,"originalLine":18,"startLine":null,"diffSide":"RIGHT", "comments":{"nodes":[ {"databaseId":900003,"author":null,"body":"gone account","createdAt":"2026-07-30T08:00:00Z","diffHunk":"@@ -16,3 +16,5 @@"} + ]}}, + {"id":"PRRT_kw3","isResolved":false,"isOutdated":true,"path":"internal/store/store.go", + "line":null,"originalLine":74,"startLine":null,"originalStartLine":65,"diffSide":"RIGHT", + "comments":{"nodes":[ + {"databaseId":900004,"author":{"login":"reviewer"},"body":"this range","createdAt":"2026-07-30T09:30:00Z","diffHunk":"@@ -60,0 +60,15 @@"} ]}} ]} }}}}` @@ -65,8 +70,8 @@ func TestParseConversation(t *testing.T) { }) t.Run("an anchored thread keeps its line span and both ids", func(t *testing.T) { - if len(convo.Threads) != 2 { - t.Fatalf("threads = %d, want 2", len(convo.Threads)) + if len(convo.Threads) != 3 { + t.Fatalf("threads = %d, want 3", len(convo.Threads)) } anchored := convo.Threads[0] if anchored.ID != "PRRT_kw1" || anchored.Line != 61 || anchored.StartLine != 59 { @@ -100,6 +105,15 @@ func TestParseConversation(t *testing.T) { } }) + t.Run("an outdated range keeps its original first line", func(t *testing.T) { + // Without it the span collapses to one line, and the snippet the + // Conversation tab cuts from the hunk loses the lines it was filed on. + ranged := convo.Threads[2] + if ranged.StartLine != 65 || ranged.Line != 74 { + t.Errorf("span = %d–%d, want the original 65–74", ranged.StartLine, ranged.Line) + } + }) + t.Run("a deleted account leaves an empty login, not a crash", func(t *testing.T) { if got := convo.Threads[1].Comments[0].Author; got != "" { t.Errorf("author = %q, want empty", got) @@ -137,7 +151,7 @@ func TestPullRequestConversationFlow(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if convo == nil || len(convo.Threads) != 2 { + if convo == nil || len(convo.Threads) != 3 { t.Fatalf("wrong conversation: %+v", convo) } want := []string{