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/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 313b725..ead7cea 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; @@ -125,10 +128,22 @@ 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 ?? []; + // 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(); @@ -226,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( @@ -259,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, @@ -292,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) => @@ -361,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) { @@ -432,64 +478,121 @@ 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); - if (guardReview) { - shaToReviewId.set(group.sha, guardReview.id); - newReviews.push(guardReview); - continue; - } + const isFirstGroup = group.sha === targets[0].sha; + const body = isFirstGroup ? submissionBody : ""; + const guardReview = + group.lineItems.length > 0 + ? await findGuardReview(group.sha, group.lineItems) + : null; 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 + 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, + pr.number, + { + commit_id: group.sha, + event, + body, + 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); + } + // 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, + 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) + ); + } + // Drop the group's line comments locally so a retried submission // cannot send them again. const submittedIds = new Set( - group.items.map(({ comment }) => comment.id) + group.lineItems.map(({ comment }) => comment.id) ); store.setPendingComments( store @@ -525,6 +628,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 @@ -588,6 +698,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 7714850..58fda7a 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,13 +131,44 @@ describe("prepareGroupComments", () => { ], files ); - expect(prepared[0].payload.line).toBe(4); + 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("file-level payload ends with a blob permalink 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( + "https://github.com/o/r/blob/abc123/src/first.ts#L40" + ); + expect(prepared[1].payload.body).toContain( + "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( - "originally on line 40, which is outside the diff" + "" ); - // 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"); }); 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..e4453fb 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,14 +70,25 @@ export function groupPendingCommentsByTarget( ); } +/** Repo context for building permalinks in file-level comment bodies. */ +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. */ + * 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[] + files: PullRequestFile[], + permalink?: PermalinkContext ): PreparedComment[] { if (comments.some((c) => c.path === ":commit") && files.length === 0) { throw new Error( @@ -106,6 +120,38 @@ export function prepareGroupComments( }, file?.patch ); + 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 + ? `#L${comment.start_line}-L${comment.line}` + : `#L${comment.line}`; + const parts = [ + buildOutOfDiffMarker( + comment.line, + comment.start_line, + comment.side, + permalink?.sha + ), + comment.body, + ]; + if (permalink) { + parts.push( + `https://github.com/${permalink.owner}/${permalink.repo}/blob/${permalink.sha}/${comment.path}${anchorPart}` + ); + } + return { + comment, + payload: { + path: comment.path, + body: parts.join("\n\n"), + side: comment.side, + subject_type: "file" as const, + }, + }; + } return { comment, payload: { @@ -114,9 +160,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: comment.body, }, }; }); 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 new file mode 100644 index 0000000..cc258ef --- /dev/null +++ b/src/shared/out-of-diff.test.ts @@ -0,0 +1,77 @@ +import { test, expect } from "bun:test"; +import { + parseOutOfDiffMarker, + buildOutOfDiffMarker, + isOutOfDiffComment, + stripOutOfDiffPermalink, + stripOutOfDiffPermalinkHtml, +} 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("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") + ).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(); +}); + +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 new file mode 100644 index 0000000..489ea53 --- /dev/null +++ b/src/shared/out-of-diff.ts @@ -0,0 +1,69 @@ +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[2], 10), + startLine: match[3] ? parseInt(match[3], 10) : undefined, + side: match[4] as "LEFT" | "RIGHT", + ...(match[1] ? { sha: match[1] } : {}), + }; +} + +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", + sha?: string +): string { + const withSha = sha ? ` sha=${sha}` : ""; + const start = startLine !== undefined ? ` start_line=${startLine}` : ""; + 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(); +}