From 1ccca9f079fc0b66f817c5a2aa47501f9c08a63b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Lehmann?= Date: Wed, 23 Sep 2026 16:29:29 +0200 Subject: [PATCH 1/4] fix: link exact lines in moved-comment notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comments anchored to lines outside the diff hunks are rejected by every public GitHub API path (422 "Line could not be resolved" on REST review batches, standalone comments and GraphQL thread creation — verified by probing), while GitHub's web UI anchors them exactly via an internal endpoint. Pulldash therefore must keep snapping to the nearest diff line, but the moved-comment note now links the original lines at the target commit so readers can navigate to where the comment belongs. --- .../contexts/pr-review/useReviewActions.ts | 6 +++- src/browser/lib/review-submit.test.ts | 26 ++++++++++++-- src/browser/lib/review-submit.ts | 35 ++++++++++++++++--- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/src/browser/contexts/pr-review/useReviewActions.ts b/src/browser/contexts/pr-review/useReviewActions.ts index 313b725..bcc48ab 100644 --- a/src/browser/contexts/pr-review/useReviewActions.ts +++ b/src/browser/contexts/pr-review/useReviewActions.ts @@ -125,7 +125,11 @@ export function useReviewActions() { ); const preparedGroups = groups.map(({ sha, comments }) => ({ sha, - items: prepareGroupComments(comments, filesBySha.get(sha) ?? []), + items: prepareGroupComments(comments, filesBySha.get(sha) ?? [], { + owner, + repo, + sha, + }), })); const headItems = preparedGroups[0]?.items ?? []; diff --git a/src/browser/lib/review-submit.test.ts b/src/browser/lib/review-submit.test.ts index 7714850..5916b83 100644 --- a/src/browser/lib/review-submit.test.ts +++ b/src/browser/lib/review-submit.test.ts @@ -133,11 +133,33 @@ describe("prepareGroupComments", () => { ); expect(prepared[0].payload.line).toBe(4); expect(prepared[0].payload.body).toContain( - "originally on line 40, which is outside the diff" + "originally on line 40 of `src/first.ts`" ); // Multi-line ranges that cannot stay intact collapse to a single line. expect(prepared[1].payload.start_line).toBeUndefined(); - expect(prepared[1].payload.body).toContain("originally on line 40"); + expect(prepared[1].payload.body).toContain("originally on lines 38-40"); + }); + + test("moved-comment note links the original lines at the target commit", () => { + const prepared = prepareGroupComments( + [ + makeComment({ id: "1", path: "src/first.ts", line: 40 }), + makeComment({ + id: "2", + path: "src/first.ts", + line: 40, + start_line: 38, + }), + ], + files, + { owner: "o", repo: "r", sha: "abc123" } + ); + expect(prepared[0].payload.body).toContain( + "[line 40 of `src/first.ts`](https://github.com/o/r/blob/abc123/src/first.ts#L40)" + ); + expect(prepared[1].payload.body).toContain( + "[lines 38-40 of `src/first.ts`](https://github.com/o/r/blob/abc123/src/first.ts#L38-L40)" + ); }); test("leaves comments for files missing from the diff unsnapped", () => { diff --git a/src/browser/lib/review-submit.ts b/src/browser/lib/review-submit.ts index dd7ad6b..5fed7a2 100644 --- a/src/browser/lib/review-submit.ts +++ b/src/browser/lib/review-submit.ts @@ -67,14 +67,24 @@ export function groupPendingCommentsByTarget( ); } +/** Repo context for building permalinks in moved-comment notes. */ +export interface PermalinkContext { + owner: string; + repo: string; + sha: string; +} + /** Prepare REST payloads for one commit group. :commit metadata comments * redirect to the first file of the group's diff; comments on lines outside * the diff snap to the nearest line GitHub will accept. GitHub validates * every comment of a review against the cumulative diff at the review's - * commit and rejects the whole review otherwise, so lines must be pre-snapped. */ + * commit and rejects the whole review otherwise, so lines must be pre-snapped. + * (GitHub's web UI anchors out-of-diff comments exactly because it uses an + * internal endpoint; the public API always rejects them with 422.) */ export function prepareGroupComments( comments: PendingCommentInput[], - files: PullRequestFile[] + files: PullRequestFile[], + permalink?: PermalinkContext ): PreparedComment[] { if (comments.some((c) => c.path === ":commit") && files.length === 0) { throw new Error( @@ -106,6 +116,23 @@ export function prepareGroupComments( }, file?.patch ); + const movedNote = ((): string | null => { + if (!anchor.adjusted) return null; + const range = + comment.start_line !== undefined && comment.start_line !== comment.line + ? `lines ${comment.start_line}-${comment.line}` + : `line ${comment.line}`; + if (permalink) { + const anchorPart = + comment.start_line !== undefined && + comment.start_line !== comment.line + ? `#L${comment.start_line}-L${comment.line}` + : `#L${comment.line}`; + const href = `https://github.com/${permalink.owner}/${permalink.repo}/blob/${permalink.sha}/${comment.path}${anchorPart}`; + return `_This comment was originally on [${range} of \`${comment.path}\`](${href}), which is outside the diff — GitHub's API can only anchor review comments to diff lines, so it was moved to the nearest one._`; + } + return `_This comment was originally on ${range} of \`${comment.path}\`, which is outside the diff — GitHub's API can only anchor review comments to diff lines, so it was moved to the nearest one._`; + })(); return { comment, payload: { @@ -114,9 +141,7 @@ export function prepareGroupComments( start_line: anchor.start_line, start_side: anchor.start_line === undefined ? undefined : comment.side, side: comment.side, - body: anchor.adjusted - ? `_This comment was originally on line ${comment.line}, which is outside the diff; it was moved to the nearest diff line when submitting._\n\n${comment.body}` - : comment.body, + body: movedNote ? `${movedNote}\n\n${comment.body}` : comment.body, }, }; }); From e4a3adcf524c57d290ca740dfe71678573ee37e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Lehmann?= Date: Wed, 23 Sep 2026 16:29:29 +0200 Subject: [PATCH 2/4] feat: submit out-of-diff comments as file-level comments with exact-range permalinks GitHub's API rejects out-of-diff line anchors in every path, so instead of snapping those comments to the nearest diff line they now go out as standalone file-level review comments (subject_type: file). The body carries a hidden marker with the real position (line/start_line/side) plus a blob permalink that GitHub renders as an embedded code snippet. In pulldash, the marker re-anchors the comment (and its replies) to the exact range while that commit is being viewed, so it reads as a comment made on the chosen code. Drafts on out-of-diff lines skip the doomed pending-review sync, and the group marker for multi-commit sessions always lands on a line comment. --- src/browser/components/pr-review.tsx | 58 ++++- src/browser/contexts/github.tsx | 32 ++- .../contexts/pr-review/useCommentActions.ts | 14 +- .../contexts/pr-review/useReviewActions.ts | 199 ++++++++++++------ src/browser/lib/review-submit.test.ts | 28 ++- src/browser/lib/review-submit.ts | 60 ++++-- src/shared/out-of-diff.test.ts | 37 ++++ src/shared/out-of-diff.ts | 36 ++++ 8 files changed, 365 insertions(+), 99 deletions(-) create mode 100644 src/shared/out-of-diff.test.ts create mode 100644 src/shared/out-of-diff.ts diff --git a/src/browser/components/pr-review.tsx b/src/browser/components/pr-review.tsx index 94b2173..08ec34b 100644 --- a/src/browser/components/pr-review.tsx +++ b/src/browser/components/pr-review.tsx @@ -105,6 +105,10 @@ import { parseCommitMetadataMarker, } from "../../shared/commit-metadata"; import { stripReviewGroupMarker } from "../../shared/review-group"; +import { + parseOutOfDiffMarker, + type OutOfDiffInfo, +} from "../../shared/out-of-diff"; import { resolveCommentLine } from "../lib/comment-anchor"; import { groupCommentsByLineSide } from "../lib/reviews"; import { @@ -1876,10 +1880,49 @@ const DiffViewer = memo(function DiffViewer({ const [isDraggingState, setIsDraggingState] = useState(false); // Pre-compute comment lookup maps for O(1) access + const outOfDiffMarkers = useMemo(() => { + // File-level comments created by pulldash for lines outside the diff + // hunks carry a marker with the real position; replies inherit it. + const markers = new Map(); + for (const comment of comments) { + if (comment.subject_type !== "file" || comment.in_reply_to_id) continue; + const info = parseOutOfDiffMarker(comment.body); + if (info) markers.set(comment.id, info); + } + return markers; + }, [comments]); + const commentsByLine = useMemo(() => { const map = new Map(); for (const comment of comments) { - if (comment.subject_type === "file") continue; + if (comment.subject_type === "file") { + // Pseudo file-level comments carry the real line in their marker and + // render at it; genuine file-level comments render above the diff. + const info = + outOfDiffMarkers.get(comment.id) ?? + (comment.in_reply_to_id + ? outOfDiffMarkers.get(comment.in_reply_to_id) + : undefined); + if (!info) continue; + // Marker lines are in the comment's commit coordinates; only anchor + // while that commit is being viewed. + if ( + (comment.commit_id ?? "").slice(0, 7) !== viewedCommitSha.slice(0, 7) + ) { + continue; + } + // Shim the marker's position onto the comment so line maps, ranges + // and side rendering all use the real anchoring. + const existing = map.get(info.line) || []; + existing.push({ + ...comment, + line: info.line, + start_line: info.startLine, + side: info.side, + }); + map.set(info.line, existing); + continue; + } const line = resolveCommentLine( { commitId: comment.commit_id, @@ -1900,15 +1943,24 @@ const DiffViewer = memo(function DiffViewer({ } } return map; - }, [comments, diff, viewedCommitSha]); + }, [comments, outOfDiffMarkers, diff, viewedCommitSha]); // File-level comments (subject_type === "file") aren't tied to a diff line. // GitHub still returns them with line=1, but they belong above the diff // rather than at line 1 (which often isn't in the diff at all). + // Out-of-diff comments (marker'd, with replies) render at their real lines + // in commentsByLine instead. const fileLevelThreads = useMemo(() => { const threadMap = new Map(); for (const comment of comments) { if (comment.subject_type !== "file") continue; + if (outOfDiffMarkers.has(comment.id)) continue; + if ( + comment.in_reply_to_id && + outOfDiffMarkers.has(comment.in_reply_to_id) + ) { + continue; + } if (!comment.in_reply_to_id) { threadMap.set(comment.id, [comment]); } @@ -1921,7 +1973,7 @@ const DiffViewer = memo(function DiffViewer({ } } return [...threadMap.values()]; - }, [comments]); + }, [comments, outOfDiffMarkers]); // Anchor lines derived from re-anchored comments + pending comments, // replacing the store's raw-line-based commentAnchorLookup diff --git a/src/browser/contexts/github.tsx b/src/browser/contexts/github.tsx index d60ddef..47f7953 100644 --- a/src/browser/contexts/github.tsx +++ b/src/browser/contexts/github.tsx @@ -917,13 +917,40 @@ function createGitHubStore() { ); result = data; } - queryClient.invalidateQueries({ queryKey: queries.pullRequestComments(owner, repo, number).queryKey, }); return result; } + /** File-level review comment (subject_type: file, no line anchor). Used for + * comments on lines outside the diff hunks, which GitHub's API cannot + * line-anchor in any submission path. */ + async function createFileLevelComment( + owner: string, + repo: string, + number: number, + options: { commitId: string; path: string; body: string } + ): Promise { + if (!octokit) throw new Error("Not initialized"); + const { data } = await octokit.request( + "POST /repos/{owner}/{repo}/pulls/{pull_number}/comments", + { + owner, + repo, + pull_number: number, + commit_id: options.commitId, + path: options.path, + subject_type: "file", + body: options.body, + } + ); + queryClient.invalidateQueries({ + queryKey: queries.pullRequestComments(owner, repo, number).queryKey, + }); + return data; + } + function getPRReviews( owner: string, repo: string, @@ -982,7 +1009,7 @@ function createGitHubStore() { body?: string; comments?: Array<{ path: string; - line: number; + line: number | undefined; body: string; side?: "LEFT" | "RIGHT"; start_line?: number; @@ -2906,6 +2933,7 @@ function createGitHubStore() { getRawCompareDiff, getPRComments, createPRComment, + createFileLevelComment, getPRReviews, getPRReviewsFresh, getReviewComments, diff --git a/src/browser/contexts/pr-review/useCommentActions.ts b/src/browser/contexts/pr-review/useCommentActions.ts index c53519c..9dd3fdb 100644 --- a/src/browser/contexts/pr-review/useCommentActions.ts +++ b/src/browser/contexts/pr-review/useCommentActions.ts @@ -7,6 +7,7 @@ import { } from "."; import { getCommitFieldLabel } from "./useCurrentDiff"; import { withReviewGroupMarker } from "@/shared/review-group"; +import { resolveCommentPosition } from "@/browser/lib/reviews"; export function useCommentActions() { const store = usePRReviewStore(); @@ -79,7 +80,18 @@ export function useCommentActions() { // Comments made on a non-head diff skip the sync: GitHub's thread // mutation can only anchor to the head diff, so they stay local until // submission groups them by commit. - if (!targetSha || targetSha === pr.head.sha) { + // Out-of-diff lines (context outside the hunks) are skipped too: GitHub + // rejects out-of-diff threads on the pending review, and they are + // submitted as standalone file-level comments instead. + const patch = + state.files.find((f) => f.filename === state.selectedFile)?.patch ?? null; + const outOfDiff = + !!patch && + resolveCommentPosition( + { line, start_line: startLine, side: commentSide }, + patch + ).adjusted; + if (!outOfDiff && (!targetSha || targetSha === pr.head.sha)) { try { const result = await github.addPendingComment(owner, repo, pr.number, { path: githubPath, diff --git a/src/browser/contexts/pr-review/useReviewActions.ts b/src/browser/contexts/pr-review/useReviewActions.ts index bcc48ab..7f453a9 100644 --- a/src/browser/contexts/pr-review/useReviewActions.ts +++ b/src/browser/contexts/pr-review/useReviewActions.ts @@ -84,6 +84,9 @@ export function useReviewActions() { const state = store.getSnapshot(); store.setSubmittingReview(true); const newReviews: Review[] = []; + // Standalone file-level comments created during this submission; merged + // into the refetched comments if GitHub's list lags behind. + const optimisticFileComments: ReviewComment[] = []; try { const headSha = pr.head.sha; @@ -133,6 +136,14 @@ export function useReviewActions() { })); const headItems = preparedGroups[0]?.items ?? []; + // Comments on lines outside the diff hunks cannot be line-anchored; + // they are submitted as standalone file-level comments instead of + // being part of a review batch. + const isFileLevel = (item: PreparedComment) => + item.payload.subject_type === "file"; + const headFileItems = headItems.filter(isFileLevel); + const headLineItems = headItems.filter((item) => !isFileLevel(item)); + // Review id (REST database id) per target commit, so optimistic // threads can reference the review they were submitted under. const shaToReviewId = new Map(); @@ -230,9 +241,12 @@ export function useReviewActions() { const usedPendingCommentIds = new Set(); let syncFailed = false; + // File-level comments (out-of-diff lines) are never part of the + // pending review — GitHub rejects out-of-diff threads on it. They + // stay local and are submitted as standalone comments afterwards. for (const { comment, payload } of submittedViaGraphQL ? [] - : headItems) { + : headLineItems) { if (comment.databaseId) continue; const existing = pendingReview?.comments.nodes.find( @@ -263,7 +277,7 @@ export function useReviewActions() { pr.number, { path: payload.path, - line: payload.line, + line: payload.line!, body: payload.body, side: payload.side, startLine: payload.start_line, @@ -345,6 +359,21 @@ export function useReviewActions() { submitted_at: submitted.submittedAt, } as Review); submittedViaGraphQL = true; + // Out-of-diff comments are not part of the pending review; they + // go out as standalone file-level comments on the head commit. + for (const item of headFileItems) { + const created = await github.createFileLevelComment( + owner, + repo, + pr.number, + { + commitId: headSha, + path: item.payload.path, + body: item.payload.body, + } + ); + optimisticFileComments.push(created); + } } catch (submitError) { // A failed submit can still mean the review went through (e.g. a // retry after a partial failure). Only fall back to REST while the @@ -436,78 +465,120 @@ export function useReviewActions() { ]; // Multi-commit sessions embed a hidden group marker in each group's - // first comment so the overview can render the batch as one review - // card. GitHub renders HTML comments as nothing; review bodies stay - // untouched. Injecting into the payloads means the guard (which - // strips markers) and the optimistic threads stay consistent. + // first line comment so the overview can render the batch as one + // review card. GitHub renders HTML comments as nothing; review bodies + // stay untouched. Injecting into the payloads means the guard (which + // strips markers) and the optimistic threads stay consistent. The + // marker always lands on a line comment — file-level ones are not + // part of the review batch. const groupToken = targets.length > 1 ? Math.random().toString(36).slice(2, 10) : null; - const markedGroups: Array<{ sha: string; items: PreparedComment[] }> = - groupToken - ? targets.map((group, index) => ({ - ...group, - items: group.items.map((item, itemIndex) => - itemIndex === 0 - ? { - ...item, - payload: { - ...item.payload, - body: `${reviewGroupMarker(groupToken, index, targets.length)}\n${item.payload.body}`, - }, - } - : item - ), - })) - : targets; + const markedGroups: Array<{ + sha: string; + lineItems: PreparedComment[]; + fileItems: PreparedComment[]; + }> = targets.map((group, index) => { + const fileItems = group.items.filter(isFileLevel); + let lineItems = group.items.filter((item) => !isFileLevel(item)); + if (groupToken && lineItems.length > 0) { + lineItems = [ + { + ...lineItems[0], + payload: { + ...lineItems[0].payload, + body: `${reviewGroupMarker(groupToken, index, targets.length)}\n${lineItems[0].payload.body}`, + }, + }, + ...lineItems.slice(1), + ]; + } + return { sha: group.sha, lineItems, fileItems }; + }); if (groupToken) { for (const group of markedGroups) { - const first = group.items[0]; + const first = group.lineItems[0]; if (first) markedPayloads.set(first.comment.id, first.payload); } } for (const group of markedGroups) { - const guardReview = await findGuardReview(group.sha, group.items); + const guardReview = + group.lineItems.length > 0 + ? await findGuardReview(group.sha, group.lineItems) + : null; if (guardReview) { shaToReviewId.set(group.sha, guardReview.id); newReviews.push(guardReview); - continue; + } else { + try { + const review = await github.createPRReview( + owner, + repo, + pr.number, + { + commit_id: group.sha, + event, + body: group.sha === targets[0].sha ? submissionBody : "", + comments: group.lineItems.map(({ payload }) => ({ + path: payload.path, + line: payload.line, + body: payload.body, + side: payload.side, + start_line: payload.start_line, + start_side: payload.start_side, + })), + } + ); + shaToReviewId.set(group.sha, review.id); + newReviews.push(review); + } catch (error) { + console.error("Failed to submit review group:", error); + // Groups submitted so far persist on GitHub. Surface the error; + // retrying skips them via the local strip below. + if (newReviews.length === 0) throw error; + github.invalidatePR(owner, repo, pr.number); + throw error; + } } - try { - const review = await github.createPRReview(owner, repo, pr.number, { - commit_id: group.sha, - event, - body: group.sha === targets[0].sha ? submissionBody : "", - comments: group.items.map(({ payload }) => ({ - path: payload.path, - line: payload.line, - body: payload.body, - side: payload.side, - start_line: payload.start_line, - start_side: payload.start_side, - })), - }); - shaToReviewId.set(group.sha, review.id); - newReviews.push(review); - // Drop the group's comments locally so a retried submission - // cannot send them again. - const submittedIds = new Set( - group.items.map(({ comment }) => comment.id) - ); - store.setPendingComments( - store - .getSnapshot() - .pendingComments.filter((c) => !submittedIds.has(c.id)) - ); - } catch (error) { - console.error("Failed to submit review group:", error); - // Groups submitted so far persist on GitHub. Surface the error; - // retrying skips them via the local strip above. - if (newReviews.length === 0) throw error; - github.invalidatePR(owner, repo, pr.number); - throw error; + // Out-of-diff comments become standalone file-level comments on + // the group's commit. Each one is stripped locally as it lands so + // a retry only re-posts the ones that failed. + for (const item of group.fileItems) { + try { + const created = await github.createFileLevelComment( + owner, + repo, + pr.number, + { + commitId: group.sha, + path: item.payload.path, + body: item.payload.body, + } + ); + optimisticFileComments.push(created); + store.setPendingComments( + store + .getSnapshot() + .pendingComments.filter((c) => c.id !== item.comment.id) + ); + } catch (error) { + console.error("Failed to submit file-level comment:", error); + if (newReviews.length === 0) throw error; + github.invalidatePR(owner, repo, pr.number); + throw error; + } } + // Drop the group's line comments locally so a retried submission + // cannot send them again. + const submittedIds = new Set( + group.lineItems.map(({ comment }) => comment.id) + ); + store.setPendingComments( + store + .getSnapshot() + .pendingComments.filter((c) => !submittedIds.has(c.id)) + ); } } @@ -529,6 +600,13 @@ export function useReviewActions() { ] ); + // File-level comments can lag behind the submit as well. Merge ours in + // when the refetch missed them so the diff shows them immediately. + const fetchedCommentIds = new Set(newComments.map((c) => c.id)); + for (const created of optimisticFileComments) { + if (!fetchedCommentIds.has(created.id)) newComments.unshift(created); + } + // If a review we just submitted isn't in the re-fetched data yet // (eventual consistency), add it manually so it appears immediately. // The timeline is ascending, so just-created reviews go last. The two @@ -592,6 +670,9 @@ export function useReviewActions() { for (const { comment, payload } of preparedGroups.flatMap( (g) => g.items )) { + // File-level comments are not review threads; they render via + // optimisticFileComments above. + if (payload.subject_type === "file") continue; const fresh = freshById.get(comment.id) ?? comment; const threadPayload = markedPayloads.get(comment.id) ?? payload; if ( diff --git a/src/browser/lib/review-submit.test.ts b/src/browser/lib/review-submit.test.ts index 5916b83..ad3565c 100644 --- a/src/browser/lib/review-submit.test.ts +++ b/src/browser/lib/review-submit.test.ts @@ -117,7 +117,7 @@ describe("prepareGroupComments", () => { }); }); - test("snaps lines outside the diff to the nearest diff line and notes it", () => { + test("out-of-diff comments become file-level payloads carrying the real position", () => { const prepared = prepareGroupComments( [ makeComment({ id: "1", path: "src/first.ts", line: 40 }), @@ -131,16 +131,21 @@ describe("prepareGroupComments", () => { ], files ); - expect(prepared[0].payload.line).toBe(4); - expect(prepared[0].payload.body).toContain( - "originally on line 40 of `src/first.ts`" - ); - // Multi-line ranges that cannot stay intact collapse to a single line. - expect(prepared[1].payload.start_line).toBeUndefined(); - expect(prepared[1].payload.body).toContain("originally on lines 38-40"); + expect(prepared[0].payload).toEqual({ + path: "src/first.ts", + body: "\n\ntest", + side: "RIGHT", + subject_type: "file", + }); + expect(prepared[1].payload).toEqual({ + path: "src/first.ts", + body: "\n\ntest", + side: "LEFT", + subject_type: "file", + }); }); - test("moved-comment note links the original lines at the target commit", () => { + test("file-level payload ends with a blob permalink at the target commit", () => { const prepared = prepareGroupComments( [ makeComment({ id: "1", path: "src/first.ts", line: 40 }), @@ -155,11 +160,12 @@ describe("prepareGroupComments", () => { { owner: "o", repo: "r", sha: "abc123" } ); expect(prepared[0].payload.body).toContain( - "[line 40 of `src/first.ts`](https://github.com/o/r/blob/abc123/src/first.ts#L40)" + "https://github.com/o/r/blob/abc123/src/first.ts#L40" ); expect(prepared[1].payload.body).toContain( - "[lines 38-40 of `src/first.ts`](https://github.com/o/r/blob/abc123/src/first.ts#L38-L40)" + "https://github.com/o/r/blob/abc123/src/first.ts#L38-L40" ); + expect(prepared[0].payload.subject_type).toBe("file"); }); test("leaves comments for files missing from the diff unsnapped", () => { diff --git a/src/browser/lib/review-submit.ts b/src/browser/lib/review-submit.ts index 5fed7a2..9df5c03 100644 --- a/src/browser/lib/review-submit.ts +++ b/src/browser/lib/review-submit.ts @@ -1,14 +1,17 @@ import type { PullRequestFile, ReviewComment } from "@/api/types"; import { resolveCommentPosition } from "./reviews"; import { stripReviewGroupMarker } from "@/shared/review-group"; +import { buildOutOfDiffMarker } from "@/shared/out-of-diff"; export interface SubmitCommentPayload { path: string; - line: number; + /** Undefined for file-level comments (lines outside the diff hunks). */ + line?: number; start_line?: number; start_side?: "LEFT" | "RIGHT"; side: "LEFT" | "RIGHT"; body: string; + subject_type?: "file"; } /** Local pending comment fields needed for submission. */ @@ -67,7 +70,7 @@ export function groupPendingCommentsByTarget( ); } -/** Repo context for building permalinks in moved-comment notes. */ +/** Repo context for building permalinks in file-level comment bodies. */ export interface PermalinkContext { owner: string; repo: string; @@ -75,12 +78,13 @@ export interface PermalinkContext { } /** Prepare REST payloads for one commit group. :commit metadata comments - * redirect to the first file of the group's diff; comments on lines outside - * the diff snap to the nearest line GitHub will accept. GitHub validates - * every comment of a review against the cumulative diff at the review's - * commit and rejects the whole review otherwise, so lines must be pre-snapped. - * (GitHub's web UI anchors out-of-diff comments exactly because it uses an - * internal endpoint; the public API always rejects them with 422.) */ + * redirect to the first file of the group's diff. Comments on lines outside + * the diff hunks cannot be line-anchored — GitHub's API rejects them with + * 422 ("Line could not be resolved") in every submission path, and its web + * UI anchors them exactly through an internal endpoint. They are submitted + * as file-level comments (subject_type: file, no line); the body carries a + * hidden marker with the real position for pulldash to re-anchor, plus a + * blob permalink GitHub renders as an embedded code snippet. */ export function prepareGroupComments( comments: PendingCommentInput[], files: PullRequestFile[], @@ -116,23 +120,33 @@ export function prepareGroupComments( }, file?.patch ); - const movedNote = ((): string | null => { - if (!anchor.adjusted) return null; - const range = + if (anchor.adjusted) { + // Out-of-diff line: submit as a file-level comment carrying the real + // position in a hidden marker, plus a blob permalink (GitHub embeds the + // referenced code range in the rendered comment). + const anchorPart = comment.start_line !== undefined && comment.start_line !== comment.line - ? `lines ${comment.start_line}-${comment.line}` - : `line ${comment.line}`; + ? `#L${comment.start_line}-L${comment.line}` + : `#L${comment.line}`; + const parts = [ + buildOutOfDiffMarker(comment.line, comment.start_line, comment.side), + comment.body, + ]; if (permalink) { - const anchorPart = - comment.start_line !== undefined && - comment.start_line !== comment.line - ? `#L${comment.start_line}-L${comment.line}` - : `#L${comment.line}`; - const href = `https://github.com/${permalink.owner}/${permalink.repo}/blob/${permalink.sha}/${comment.path}${anchorPart}`; - return `_This comment was originally on [${range} of \`${comment.path}\`](${href}), which is outside the diff — GitHub's API can only anchor review comments to diff lines, so it was moved to the nearest one._`; + parts.push( + `https://github.com/${permalink.owner}/${permalink.repo}/blob/${permalink.sha}/${comment.path}${anchorPart}` + ); } - return `_This comment was originally on ${range} of \`${comment.path}\`, which is outside the diff — GitHub's API can only anchor review comments to diff lines, so it was moved to the nearest one._`; - })(); + return { + comment, + payload: { + path: comment.path, + body: parts.join("\n\n"), + side: comment.side, + subject_type: "file" as const, + }, + }; + } return { comment, payload: { @@ -141,7 +155,7 @@ export function prepareGroupComments( start_line: anchor.start_line, start_side: anchor.start_line === undefined ? undefined : comment.side, side: comment.side, - body: movedNote ? `${movedNote}\n\n${comment.body}` : comment.body, + body: comment.body, }, }; }); diff --git a/src/shared/out-of-diff.test.ts b/src/shared/out-of-diff.test.ts new file mode 100644 index 0000000..97cb859 --- /dev/null +++ b/src/shared/out-of-diff.test.ts @@ -0,0 +1,37 @@ +import { test, expect } from "bun:test"; +import { + parseOutOfDiffMarker, + buildOutOfDiffMarker, + isOutOfDiffComment, +} from "./out-of-diff"; + +test("marker round-trips with and without a start line", () => { + const single = buildOutOfDiffMarker(415, undefined, "RIGHT"); + expect(parseOutOfDiffMarker(single)).toEqual({ + line: 415, + startLine: undefined, + side: "RIGHT", + }); + + const range = buildOutOfDiffMarker(415, 413, "LEFT"); + expect(parseOutOfDiffMarker(range)).toEqual({ + line: 415, + startLine: 413, + side: "LEFT", + }); +}); + +test("detects the marker anywhere in the body", () => { + expect( + isOutOfDiffComment("x\n\ny") + ).toBe(true); + expect(isOutOfDiffComment("plain body")).toBe(false); + expect(isOutOfDiffComment(undefined)).toBe(false); +}); + +test("returns null for other pulldash markers or malformed ones", () => { + expect( + parseOutOfDiffMarker("") + ).toBeNull(); + expect(parseOutOfDiffMarker("")).toBeNull(); +}); diff --git a/src/shared/out-of-diff.ts b/src/shared/out-of-diff.ts new file mode 100644 index 0000000..e03db2a --- /dev/null +++ b/src/shared/out-of-diff.ts @@ -0,0 +1,36 @@ +export const OUT_OF_DIFF_MARKER = "/; + +export function parseOutOfDiffMarker(body: string): OutOfDiffInfo | null { + const match = body.match(MARKER_RE); + if (!match) return null; + return { + line: parseInt(match[1], 10), + startLine: match[2] ? parseInt(match[2], 10) : undefined, + side: match[3] as "LEFT" | "RIGHT", + }; +} + +export function isOutOfDiffComment(body?: string | null): boolean { + return !!body?.includes(OUT_OF_DIFF_MARKER); +} + +export function buildOutOfDiffMarker( + line: number, + startLine: number | undefined, + side: "LEFT" | "RIGHT" +): string { + const start = startLine !== undefined ? ` start_line=${startLine}` : ""; + return ``; +} From 38517fca96ab29cfe5e5bb9729ff08a293592b7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Lehmann?= Date: Wed, 23 Sep 2026 16:29:29 +0200 Subject: [PATCH 3/4] fix: keep out-of-diff drafts on failed standalone posts If the GraphQL review submitted but a standalone file-level comment post failed, the error was swallowed and clearReviewState wiped the still -unposted draft. The posting now runs as a single post-submit step (also reached by the dedup branches on retry), strips each pending comment as it lands, and throws before the refetch so drafts survive retries. The pending-review recovery loop only matches line comments, since file-level ones cannot exist in a pending review. --- .../contexts/pr-review/useReviewActions.ts | 128 +++++++++++------- 1 file changed, 78 insertions(+), 50 deletions(-) diff --git a/src/browser/contexts/pr-review/useReviewActions.ts b/src/browser/contexts/pr-review/useReviewActions.ts index 7f453a9..ead7cea 100644 --- a/src/browser/contexts/pr-review/useReviewActions.ts +++ b/src/browser/contexts/pr-review/useReviewActions.ts @@ -310,7 +310,8 @@ export function useReviewActions() { reviewNodeId = recoveredReview.id; pendingReview = recoveredReview; store.setPendingReviewNodeId(reviewNodeId); - for (const { comment, payload } of headItems) { + // File-level comments cannot exist in a pending review. + for (const { comment, payload } of headLineItems) { if (comment.databaseId) continue; const existing = pendingReview.comments.nodes.find( (candidate) => @@ -359,21 +360,6 @@ export function useReviewActions() { submitted_at: submitted.submittedAt, } as Review); submittedViaGraphQL = true; - // Out-of-diff comments are not part of the pending review; they - // go out as standalone file-level comments on the head commit. - for (const item of headFileItems) { - const created = await github.createFileLevelComment( - owner, - repo, - pr.number, - { - commitId: headSha, - path: item.payload.path, - body: item.payload.body, - } - ); - optimisticFileComments.push(created); - } } catch (submitError) { // A failed submit can still mean the review went through (e.g. a // retry after a partial failure). Only fall back to REST while the @@ -394,6 +380,33 @@ export function useReviewActions() { } } } + + // Out-of-diff comments are not part of the pending review; they go + // out as standalone file-level comments on the head commit. Runs for + // every GraphQL success path (including the dedup branches above so + // a retry still posts them). Each one is stripped locally as it + // lands, and a failure throws before the refetch below clears + // pending state, so nothing is lost for a retry. + if (submittedViaGraphQL) { + for (const item of headFileItems) { + const created = await github.createFileLevelComment( + owner, + repo, + pr.number, + { + commitId: headSha, + path: item.payload.path, + body: item.payload.body, + } + ); + optimisticFileComments.push(created); + store.setPendingComments( + store + .getSnapshot() + .pendingComments.filter((c) => c.id !== item.comment.id) + ); + } + } } if (!submittedViaGraphQL) { @@ -503,15 +516,37 @@ export function useReviewActions() { } for (const group of markedGroups) { + const isFirstGroup = group.sha === targets[0].sha; + const body = isFirstGroup ? submissionBody : ""; const guardReview = group.lineItems.length > 0 ? await findGuardReview(group.sha, group.lineItems) : null; - if (guardReview) { - shaToReviewId.set(group.sha, guardReview.id); - newReviews.push(guardReview); - } else { - try { + try { + if (guardReview) { + shaToReviewId.set(group.sha, guardReview.id); + newReviews.push(guardReview); + } else if ( + event === "COMMENT" && + group.lineItems.length === 0 && + !body.trim() + ) { + // GitHub rejects COMMENT reviews with no comments and no real + // body; the file-level comments below carry the content. + } else if ( + event === "COMMENT" && + group.lineItems.length === 0 && + !!body.trim() + ) { + // A summary with only out-of-diff comments: GitHub's own UI + // posts the summary as a conversation comment in this case. + await github.createPRConversationComment( + owner, + repo, + pr.number, + body + ); + } else { const review = await github.createPRReview( owner, repo, @@ -519,7 +554,7 @@ export function useReviewActions() { { commit_id: group.sha, event, - body: group.sha === targets[0].sha ? submissionBody : "", + body, comments: group.lineItems.map(({ payload }) => ({ path: payload.path, line: payload.line, @@ -532,20 +567,11 @@ export function useReviewActions() { ); shaToReviewId.set(group.sha, review.id); newReviews.push(review); - } catch (error) { - console.error("Failed to submit review group:", error); - // Groups submitted so far persist on GitHub. Surface the error; - // retrying skips them via the local strip below. - if (newReviews.length === 0) throw error; - github.invalidatePR(owner, repo, pr.number); - throw error; } - } - // Out-of-diff comments become standalone file-level comments on - // the group's commit. Each one is stripped locally as it lands so - // a retry only re-posts the ones that failed. - for (const item of group.fileItems) { - try { + // Out-of-diff comments become standalone file-level comments on + // the group's commit. Each one is stripped locally as it lands so + // a retry only re-posts the ones that failed. + for (const item of group.fileItems) { const created = await github.createFileLevelComment( owner, repo, @@ -562,23 +588,25 @@ export function useReviewActions() { .getSnapshot() .pendingComments.filter((c) => c.id !== item.comment.id) ); - } catch (error) { - console.error("Failed to submit file-level comment:", error); - if (newReviews.length === 0) throw error; - github.invalidatePR(owner, repo, pr.number); - throw error; } + // Drop the group's line comments locally so a retried submission + // cannot send them again. + const submittedIds = new Set( + group.lineItems.map(({ comment }) => comment.id) + ); + store.setPendingComments( + store + .getSnapshot() + .pendingComments.filter((c) => !submittedIds.has(c.id)) + ); + } catch (error) { + console.error("Failed to submit review group:", error); + // Groups submitted so far persist on GitHub. Surface the error; + // retrying skips them via the local strip above. + if (newReviews.length === 0) throw error; + github.invalidatePR(owner, repo, pr.number); + throw error; } - // Drop the group's line comments locally so a retried submission - // cannot send them again. - const submittedIds = new Set( - group.lineItems.map(({ comment }) => comment.id) - ); - store.setPendingComments( - store - .getSnapshot() - .pendingComments.filter((c) => !submittedIds.has(c.id)) - ); } } From 7118a2733c988573d5a2bbaa6bbee4ba3a4cbfdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Lehmann?= Date: Wed, 23 Sep 2026 16:29:29 +0200 Subject: [PATCH 4/4] feat: render out-of-diff threads with real code context in the overview File-level comments (out-of-diff lines) rendered with L=null and no code context. The position marker now carries the anchor commit, and the thread synthesizes an all-context hunk from the file content at that commit (base version for LEFT-side markers) covering exactly the marked range, so the overview shows the referenced range with syntax highlighting like a normal diff comment. The header link targets the real line, and the blob permalink (only there for GitHub's embedded snippet) is stripped from pulldash's rendered body. --- src/browser/components/pr-overview.tsx | 150 ++++++++++++++++++++++--- src/browser/lib/review-submit.test.ts | 3 + src/browser/lib/review-submit.ts | 7 +- src/browser/lib/reviews.test.ts | 14 +++ src/browser/lib/reviews.ts | 12 ++ src/shared/out-of-diff.test.ts | 40 +++++++ src/shared/out-of-diff.ts | 45 +++++++- 7 files changed, 247 insertions(+), 24 deletions(-) diff --git a/src/browser/components/pr-overview.tsx b/src/browser/components/pr-overview.tsx index 5944c9f..a260a93 100644 --- a/src/browser/components/pr-overview.tsx +++ b/src/browser/components/pr-overview.tsx @@ -37,6 +37,7 @@ import { Lock, Unlock, GitBranch, + Hourglass, Users, UserPlus, UserMinus, @@ -60,7 +61,11 @@ import { import { getTimeAgo, formatDateTime } from "../lib/dates"; import { parseDiffCached, type ParsedDiff } from "../lib/diff"; import { discussionUrl } from "../lib/pr-url"; -import { getLatestReviewsByUser, getLatestReviewByUser } from "../lib/reviews"; +import { + getLatestReviewsByUser, + getLatestReviewByUser, + isReviewStale, +} from "../lib/reviews"; import type { ReviewComment } from "@/api/types"; import type { components } from "@octokit/openapi-types"; import { useQuery } from "@tanstack/react-query"; @@ -97,6 +102,11 @@ import { withReviewGroupMarker, type MarkedReview, } from "../../shared/review-group"; +import { + parseOutOfDiffMarker, + stripOutOfDiffPermalink, + stripOutOfDiffPermalinkHtml, +} from "../../shared/out-of-diff"; import { buildMetadataLines } from "../contexts/pr-review/useCurrentDiff"; // ============================================================================ @@ -1162,22 +1172,25 @@ export const PROverview = memo(function PROverview() { avatar_url: string; state: Review["state"] | "PENDING"; isTeam?: boolean; + stale?: boolean; }> = []; const byUser = getLatestReviewByUser(reviews); const requestedLogins = new Set( pr.requested_reviewers?.map((r) => r.login) ?? [] ); + const headSha = pr.head?.sha; const addReviewer = ( login: string, avatar_url: string, state: Review["state"] | "PENDING", - isTeam?: boolean + isTeam?: boolean, + stale?: boolean ) => { if (seen.has(login)) return; seen.add(login); - result.push({ login, avatar_url, state, isTeam }); + result.push({ login, avatar_url, state, isTeam, stale }); }; // Priority order function @@ -1195,7 +1208,13 @@ export const PROverview = memo(function PROverview() { if (r.user) { // Skip re-requested reviewers — they'll show as PENDING instead if (requestedLogins.has(r.user.login)) continue; - addReviewer(r.user.login, r.user.avatar_url, r.state); + addReviewer( + r.user.login, + r.user.avatar_url, + r.state, + undefined, + isReviewStale(r, headSha) + ); } } // Then pending reviewers who haven't submitted any review if (pr.requested_reviewers) { @@ -1212,7 +1231,7 @@ export const PROverview = memo(function PROverview() { result.sort((a, b) => priority(a.state) - priority(b.state)); return result; - }, [reviews, pr.requested_reviewers, pr.requested_teams]); + }, [reviews, pr.requested_reviewers, pr.requested_teams, pr.head?.sha]); // Tab counts const checksCount = checks @@ -2038,6 +2057,7 @@ export const PROverview = memo(function PROverview() { mergeError={mergeError} latestReviews={latestReviews} reviewStates={reviewerStates} + headSha={pr.head?.sha} repoAllowMergeCommit={repoAllowMergeCommit} repoAllowSquashMerge={repoAllowSquashMerge} repoAllowRebaseMerge={repoAllowRebaseMerge} @@ -2441,7 +2461,11 @@ export const PROverview = memo(function PROverview() { )} - + )} @@ -3266,6 +3290,8 @@ function ReviewBox({ const cachedKey = review.id ? `review-${review.id}` : null; const cachedReactions = cachedKey ? parentReactions[cachedKey] : undefined; const [reactions, setReactions] = useState(cachedReactions ?? []); + const headSha = usePRReviewSelector((s) => s.pr.head?.sha); + const stale = isReviewStale(review, headSha); // Fetch reactions via GraphQL if not cached by the parent batch useEffect(() => { @@ -3371,7 +3397,7 @@ function ReviewBox({ iconColor )} > - +
@@ -3472,9 +3498,12 @@ function ReviewBox({ function ReviewStateIcon({ state, showTooltip = false, + stale = false, }: { state: string; showTooltip?: boolean; + /** Gitea-style hourglass: new changes were pushed since this review. */ + stale?: boolean; }) { const getIconAndTooltip = () => { switch (state) { @@ -3508,14 +3537,30 @@ function ReviewStateIcon({ const { icon, tooltip } = getIconAndTooltip(); + const content = stale ? ( + + {icon} + + + + + + + New changes since this review + + + ) : ( + icon + ); + if (!showTooltip) { - return icon; + return content; } return ( - {icon} + {content} {tooltip} @@ -3585,7 +3630,63 @@ function ReviewThreadBox({ isSingleCommentMetadata(c.body) ); const store = usePRReviewStore(); + const github = useGitHub(); const prHtmlUrl = usePRReviewSelector((s) => s.pr.html_url); + // Out-of-diff comments (submitted as file-level with a position marker) + // render their referenced code range as the context block. + const outOfDiff = useMemo( + () => (firstComment ? parseOutOfDiffMarker(firstComment.body) : null), + [firstComment?.body] + ); + const outOfDiffSha = useMemo(() => { + if (outOfDiff?.sha) return outOfDiff.sha; + const match = firstComment?.body.match( + /https:\/\/github\.com\/[^/\s]+\/[^/\s]+\/blob\/([0-9a-f]{7,40})\// + ); + return match?.[1] ?? null; + }, [outOfDiff, firstComment?.body]); + const [outOfDiffContent, setOutOfDiffContent] = useState(null); + useEffect(() => { + if (!outOfDiff || !outOfDiffSha || !filePath) { + setOutOfDiffContent(null); + return; + } + let cancelled = false; + // RIGHT-side markers are in the anchored commit's file coordinates; + // LEFT-side ones are in the base version's. + const state = store.getSnapshot(); + const ref = outOfDiff.side === "LEFT" ? state.pr.base.sha : outOfDiffSha!; + github + .getFileContent( + state.owner, + state.repo, + filePath, + ref, + `${state.owner}/${state.repo}/${state.pr.number}` + ) + .then((content) => { + if (!cancelled) setOutOfDiffContent(content); + }) + .catch(() => { + if (!cancelled) setOutOfDiffContent(null); + }); + return () => { + cancelled = true; + }; + }, [outOfDiff, outOfDiffSha, filePath, github, store]); + const outOfDiffHunk = useMemo(() => { + if (!outOfDiff || !outOfDiffContent) return null; + const lines = outOfDiffContent.split("\n"); + const contextStart = Math.max(1, outOfDiff.startLine ?? outOfDiff.line); + const contextEnd = Math.min(lines.length, outOfDiff.line); + const count = contextEnd - contextStart + 1; + // Synthesize an all-context hunk; parseDiffCached numbers old and new + // sides identically, matching file coordinates. + return [ + `@@ -${contextStart},${count} +${contextStart},${count} @@`, + ...lines.slice(contextStart - 1, contextEnd).map((l) => ` ${l}`), + ].join("\n"); + }, [outOfDiff, outOfDiffContent]); const metadataContext = useMemo(() => { if (!isMetadataComment) return null; const info = parseCommitMetadataMarker(firstComment?.body ?? ""); @@ -3613,15 +3714,14 @@ function ReviewThreadBox({ // Parse diff hunk with syntax highlighting using the worker // Note: The worker already adds git diff headers, so we pass diffHunk directly useEffect(() => { - if (!diffHunk || !filePath) { + const text = outOfDiffHunk ?? diffHunk; + if (!text || !filePath) { setParsedDiff(null); return; } - parseDiffCached(diffHunk, filePath) - .then(setParsedDiff) - .catch(console.error); - }, [diffHunk, filePath]); + parseDiffCached(text, filePath).then(setParsedDiff).catch(console.error); + }, [diffHunk, outOfDiffHunk, filePath]); // Get diff lines from parsed diff (first hunk), filtered to show only relevant lines // GitHub's UI shows ~10 lines of context around the comment, not the entire diff hunk @@ -3739,14 +3839,14 @@ function ReviewThreadBox({ ) : ( <> {filePath} {firstComment.originalCommit?.oid && ( @@ -3961,6 +4061,16 @@ function ReviewThreadBox({
) : isMetadataComment ? ( {stripCommitMetadataPrefix(comment.body)} + ) : comment === firstComment && outOfDiff ? ( + + {stripOutOfDiffPermalink(comment.body ?? "")} + ) : ( {comment.body} )} @@ -4191,6 +4301,7 @@ function MergeSection({ mergeError, latestReviews, reviewStates, + headSha, repoAllowMergeCommit, repoAllowSquashMerge, repoAllowRebaseMerge, @@ -4227,6 +4338,7 @@ function MergeSection({ mergeError: string | null; latestReviews: Review[]; reviewStates: Review[]; + headSha?: string; repoAllowMergeCommit: boolean; repoAllowSquashMerge: boolean; repoAllowRebaseMerge: boolean; @@ -4449,7 +4561,11 @@ function MergeSection({ className="w-5 h-5 rounded-full" /> {review.user?.login ?? ""} - + ))} diff --git a/src/browser/lib/review-submit.test.ts b/src/browser/lib/review-submit.test.ts index ad3565c..58fda7a 100644 --- a/src/browser/lib/review-submit.test.ts +++ b/src/browser/lib/review-submit.test.ts @@ -166,6 +166,9 @@ describe("prepareGroupComments", () => { "https://github.com/o/r/blob/abc123/src/first.ts#L38-L40" ); expect(prepared[0].payload.subject_type).toBe("file"); + expect(prepared[0].payload.body).toContain( + "" + ); }); test("leaves comments for files missing from the diff unsnapped", () => { diff --git a/src/browser/lib/review-submit.ts b/src/browser/lib/review-submit.ts index 9df5c03..e4453fb 100644 --- a/src/browser/lib/review-submit.ts +++ b/src/browser/lib/review-submit.ts @@ -129,7 +129,12 @@ export function prepareGroupComments( ? `#L${comment.start_line}-L${comment.line}` : `#L${comment.line}`; const parts = [ - buildOutOfDiffMarker(comment.line, comment.start_line, comment.side), + buildOutOfDiffMarker( + comment.line, + comment.start_line, + comment.side, + permalink?.sha + ), comment.body, ]; if (permalink) { diff --git a/src/browser/lib/reviews.test.ts b/src/browser/lib/reviews.test.ts index ec205eb..5bd5808 100644 --- a/src/browser/lib/reviews.test.ts +++ b/src/browser/lib/reviews.test.ts @@ -2,6 +2,7 @@ import { test, expect } from "bun:test"; import { getLatestReviewByUser, getLatestReviewsByUser, + isReviewStale, groupCommentsByLineSide, resolveCommentPosition, } from "./reviews"; @@ -65,6 +66,19 @@ test("comment review downgrades the badge but not the merge decision", () => { expect(getLatestReviewsByUser(badgeFixture)[0].state).toBe("APPROVED"); }); +test("reviews submitted on an older head are stale", () => { + const approved = { + ...review("a", "APPROVED", "2026-01-01T00:00:00Z", 1), + commit_id: "oldsha", + }; + expect(isReviewStale(approved, "newsha")).toBe(true); + expect(isReviewStale(approved, "oldsha")).toBe(false); + expect(isReviewStale(approved, null)).toBe(false); + expect( + isReviewStale(review("a", "APPROVED", "2026-01-01T00:00:00Z", 2), "x") + ).toBe(false); +}); + test("requesting changes overrides an earlier approval", () => { const reviews = [ review("a", "APPROVED", "2026-01-01T00:00:00Z", 1), diff --git a/src/browser/lib/reviews.ts b/src/browser/lib/reviews.ts index f15baa0..0f5eab4 100644 --- a/src/browser/lib/reviews.ts +++ b/src/browser/lib/reviews.ts @@ -27,6 +27,18 @@ export function getLatestReviewByUser(reviews: Review[]): Map { return byUser; } +/** + * Whether a review predates the current head — new changes were pushed since + * it was submitted (Gitea's stale-review hourglass). A missing commit_id or + * head SHA means the check can't be made. + */ +export function isReviewStale( + review: Review, + headSha?: string | null +): boolean { + return !!headSha && !!review.commit_id && review.commit_id !== headSha; +} + /** * Latest opinionated (APPROVED/CHANGES_REQUESTED) review per user — GitHub's * `latestOpinionatedReviews`: a later COMMENTED review does not mask the diff --git a/src/shared/out-of-diff.test.ts b/src/shared/out-of-diff.test.ts index 97cb859..cc258ef 100644 --- a/src/shared/out-of-diff.test.ts +++ b/src/shared/out-of-diff.test.ts @@ -3,6 +3,8 @@ import { parseOutOfDiffMarker, buildOutOfDiffMarker, isOutOfDiffComment, + stripOutOfDiffPermalink, + stripOutOfDiffPermalinkHtml, } from "./out-of-diff"; test("marker round-trips with and without a start line", () => { @@ -21,6 +23,17 @@ test("marker round-trips with and without a start line", () => { }); }); +test("marker carries the anchor commit when provided", () => { + const withSha = buildOutOfDiffMarker(415, 413, "RIGHT", "f9a1167bbc9e"); + expect(parseOutOfDiffMarker(withSha)).toEqual({ + line: 415, + startLine: 413, + side: "RIGHT", + sha: "f9a1167bbc9e", + }); + expect(isOutOfDiffComment(withSha)).toBe(true); +}); + test("detects the marker anywhere in the body", () => { expect( isOutOfDiffComment("x\n\ny") @@ -35,3 +48,30 @@ test("returns null for other pulldash markers or malformed ones", () => { ).toBeNull(); expect(parseOutOfDiffMarker("")).toBeNull(); }); + +test("strips the trailing permalink from raw bodies", () => { + const body = + "another out-of-diff comment\n\nhttps://github.com/o/r/blob/f9a1167bbc9e3d2b6047566e61e4e1b1c44af8d8/src/a.ts#L18-L20"; + expect(stripOutOfDiffPermalink(body)).toBe("another out-of-diff comment"); +}); + +test("strips the embedded snippet block from rendered HTML", () => { + const html = [ + '

another out-of-diff comment

', + '

', + '

x

', + '
', + "
code
", + "
", + "
", + "

", + ].join("\n"); + expect(stripOutOfDiffPermalinkHtml(html)).toBe( + '

another out-of-diff comment

' + ); +}); + +test("leaves bodies without a permalink untouched", () => { + expect(stripOutOfDiffPermalink("plain")).toBe("plain"); + expect(stripOutOfDiffPermalinkHtml("

plain

")).toBe("

plain

"); +}); diff --git a/src/shared/out-of-diff.ts b/src/shared/out-of-diff.ts index e03db2a..489ea53 100644 --- a/src/shared/out-of-diff.ts +++ b/src/shared/out-of-diff.ts @@ -7,18 +7,22 @@ export interface OutOfDiffInfo { line: number; startLine?: number; side: "LEFT" | "RIGHT"; + /** Commit the position refers to. Older markers (pre-sha) omit it; the + * blob permalink embedded in the body is the fallback source. */ + sha?: string; } const MARKER_RE = - //; + //; export function parseOutOfDiffMarker(body: string): OutOfDiffInfo | null { const match = body.match(MARKER_RE); if (!match) return null; return { - line: parseInt(match[1], 10), - startLine: match[2] ? parseInt(match[2], 10) : undefined, - side: match[3] as "LEFT" | "RIGHT", + line: parseInt(match[2], 10), + startLine: match[3] ? parseInt(match[3], 10) : undefined, + side: match[4] as "LEFT" | "RIGHT", + ...(match[1] ? { sha: match[1] } : {}), }; } @@ -29,8 +33,37 @@ export function isOutOfDiffComment(body?: string | null): boolean { export function buildOutOfDiffMarker( line: number, startLine: number | undefined, - side: "LEFT" | "RIGHT" + side: "LEFT" | "RIGHT", + sha?: string ): string { + const withSha = sha ? ` sha=${sha}` : ""; const start = startLine !== undefined ? ` start_line=${startLine}` : ""; - return ``; + return ``; +} + +/** The blob permalink appended to out-of-diff comments, matched anywhere in + * a body (raw markdown or pre-rendered HTML). */ +const PERMALINK_RE = + /(?:^|\n|]*>)\s*(?:]*>)?https:\/\/github\.com\/[^/\s<]+\/[^/\s<]+\/blob\/[0-9a-f]+\/\S+#[^\s<]*(?:<\/a>)?\s*(?:<\/p>)?\s*$/; + +/** Remove the trailing blob permalink from a raw body. Pulldash renders the + * referenced range itself; the permalink is only there so GitHub embeds the + * code snippet. */ +export function stripOutOfDiffPermalink(body: string): string { + const stripped = body.replace( + /(?:^|\n)\s*https:\/\/github\.com\/[^/\s]+\/[^/\s]+\/blob\/[0-9a-f]+\/\S+#[^\s]*\s*$/, + "" + ); + return stripped === body ? body.replace(PERMALINK_RE, "") : stripped; +} + +/** Remove the embedded code-snippet block GitHub renders for the permalink + * (an empty paragraph, the condensed Box, and a trailing empty paragraph). */ +export function stripOutOfDiffPermalinkHtml(bodyHtml: string): string { + return bodyHtml + .replace( + /]*>\s*<\/p>\s*
[\s\S]*?<\/table>\s*<\/div>\s*<\/div>\s*]*>\s*<\/p>\s*$/, + "" + ) + .trimEnd(); }