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{