Improve PR review diffs (#139)

* Improve PR review diffs

* Clean up

* Add logging
This commit is contained in:
Colin McDonnell
2026-01-21 00:50:19 +00:00
committed by pullfrog[bot]
parent 1edeaa0f4c
commit a3a1530da2
2 changed files with 215 additions and 253 deletions
+90 -98
View File
@@ -121223,43 +121223,56 @@ function CreatePullRequestReviewTool(ctx) {
// mcp/reviewComments.ts // mcp/reviewComments.ts
import { writeFileSync as writeFileSync3 } from "node:fs"; import { writeFileSync as writeFileSync3 } from "node:fs";
import { join as join5 } from "node:path"; import { join as join5 } from "node:path";
var REVIEW_THREADS_QUERY = ` var REPLY_TO_FRAGMENT = `
query ($owner: String!, $repo: String!, $pullNumber: Int!) { replyTo {
repository(owner: $owner, name: $repo) { databaseId
pullRequest(number: $pullNumber) { body
reviewThreads(first: 100) { author { login }
replyTo {
databaseId
body
author { login }
replyTo {
databaseId
body
author { login }
replyTo {
databaseId
body
author { login }
replyTo {
databaseId
body
author { login }
}
}
}
}
}
`;
var REVIEW_QUERY = `
query ($nodeId: ID!) {
node(id: $nodeId) {
... on PullRequestReview {
databaseId
author { login }
comments(first: 100) {
nodes { nodes {
diffSide databaseId
startDiffSide body
comments(first: 100) { path
nodes { line
id startLine
databaseId diffHunk
body url
path author { login }
line createdAt
startLine ${REPLY_TO_FRAGMENT}
url reactionGroups {
author { content
login reactors(first: 10) {
} nodes {
createdAt ... on Actor { login }
updatedAt
pullRequestReview {
databaseId
}
replyTo {
databaseId
}
reactionGroups {
content
reactors(first: 10) {
nodes {
... on Actor {
login
}
}
}
} }
} }
} }
@@ -121270,20 +121283,11 @@ query ($owner: String!, $repo: String!, $pullNumber: Int!) {
} }
`; `;
var MAX_BODY_PREVIEW = 80; var MAX_BODY_PREVIEW = 80;
var MAX_THREAD_DEPTH = 10;
function truncateBody(body) { function truncateBody(body) {
const oneLine = body.replace(/\n/g, " ").trim(); const oneLine = body.replace(/\n/g, " ").trim();
if (oneLine.length <= MAX_BODY_PREVIEW) return oneLine; if (oneLine.length <= MAX_BODY_PREVIEW) return oneLine;
return oneLine.slice(0, MAX_BODY_PREVIEW - 3) + "..."; return oneLine.slice(0, MAX_BODY_PREVIEW - 3) + "...";
} }
function getReplyChain(comment, commentMap, depth = 0) {
if (depth >= MAX_THREAD_DEPTH) return [];
const parentId = comment.replyTo?.databaseId;
if (!parentId) return [];
const parent = commentMap.get(parentId);
if (!parent) return [];
return [...getReplyChain(parent, commentMap, depth + 1), parent];
}
function escapeXml(text) { function escapeXml(text) {
return text.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;"); return text.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;");
} }
@@ -121293,6 +121297,11 @@ function hasThumbsUpFrom(comment, username) {
if (!thumbsUp?.reactors?.nodes) return false; if (!thumbsUp?.reactors?.nodes) return false;
return thumbsUp.reactors.nodes.some((r) => r?.login === username); return thumbsUp.reactors.nodes.some((r) => r?.login === username);
} }
function flattenReplyToChain(replyTo) {
if (!replyTo) return [];
const parent = flattenReplyToChain(replyTo.replyTo ?? null);
return [...parent, { body: replyTo.body, author: replyTo.author?.login ?? "unknown" }];
}
var GetReviewComments = type({ var GetReviewComments = type({
pull_number: type.number.describe("The pull request number"), pull_number: type.number.describe("The pull request number"),
review_id: type.number.describe("The review ID to get comments for"), review_id: type.number.describe("The review ID to get comments for"),
@@ -121304,90 +121313,72 @@ function GetReviewCommentsTool(ctx) {
description: "Get review comments for a pull request review, including thread context. When approved_by is provided, only returns comments that user approved with \u{1F44D}. Returns commentsPath pointing to a file with full comment details in XML format.", description: "Get review comments for a pull request review, including thread context. When approved_by is provided, only returns comments that user approved with \u{1F44D}. Returns commentsPath pointing to a file with full comment details in XML format.",
parameters: GetReviewComments, parameters: GetReviewComments,
execute: execute(async ({ pull_number, review_id, approved_by }) => { execute: execute(async ({ pull_number, review_id, approved_by }) => {
const response = await ctx.octokit.graphql(REVIEW_THREADS_QUERY, { const { data: review } = await ctx.octokit.rest.pulls.getReview({
owner: ctx.repo.owner, owner: ctx.repo.owner,
repo: ctx.repo.name, repo: ctx.repo.name,
pullNumber: pull_number pull_number,
review_id
}); });
const pullRequest = response.repository?.pullRequest; const reviewer = review.user?.login ?? "unknown";
if (!pullRequest?.reviewThreads?.nodes) { const response = await ctx.octokit.graphql(REVIEW_QUERY, {
nodeId: review.node_id
});
const reviewComments = response.node?.comments?.nodes;
if (!reviewComments) {
return { return {
review_id, review_id,
pull_number, pull_number,
reviewer: "unknown", reviewer,
count: 0, count: 0,
commentsPath: null, commentsPath: null,
message: "No review threads found" message: "No comments found for this review"
}; };
} }
const commentMap = /* @__PURE__ */ new Map(); const allComments = reviewComments.filter((c) => c !== null);
for (const thread of pullRequest.reviewThreads.nodes) { const comments = approved_by ? allComments.filter((c) => hasThumbsUpFrom(c, approved_by)) : allComments;
if (!thread?.comments?.nodes) continue; if (comments.length === 0) {
for (const comment of thread.comments.nodes) {
if (comment) commentMap.set(comment.databaseId, comment);
}
}
const leafComments = [];
for (const thread of pullRequest.reviewThreads.nodes) {
if (!thread?.comments?.nodes) continue;
const threadComments = thread.comments.nodes.filter(
(c) => c !== null
);
if (threadComments.length === 0) continue;
for (const comment of threadComments) {
if (comment.pullRequestReview?.databaseId !== review_id) continue;
if (approved_by && !hasThumbsUpFrom(comment, approved_by)) continue;
const replyChain = getReplyChain(comment, commentMap);
leafComments.push({
comment,
thread: replyChain,
side: thread.diffSide
});
}
}
if (leafComments.length === 0) {
return { return {
review_id, review_id,
pull_number, pull_number,
reviewer: "unknown", reviewer,
count: 0, count: 0,
commentsPath: null, commentsPath: null,
message: approved_by ? `No comments with \u{1F44D} from ${approved_by}` : "No comments found for this review" message: approved_by ? `No comments with \u{1F44D} from ${approved_by}` : "No comments found for this review"
}; };
} }
const reviewer = leafComments[0].comment.author?.login ?? "unknown";
const lines = []; const lines = [];
lines.push( lines.push(`<review_comments count="${comments.length}" reviewer="${escapeXml(reviewer)}">`);
`<review_comments count="${leafComments.length}" reviewer="${escapeXml(reviewer)}">`
);
lines.push(""); lines.push("");
lines.push("<summary>"); lines.push("<summary>");
for (const leaf of leafComments) { for (const comment of comments) {
const line = leaf.comment.line ?? leaf.comment.startLine ?? 0; const line = comment.line ?? comment.startLine ?? 0;
const preview = escapeXml(truncateBody(leaf.comment.body)); const preview = escapeXml(truncateBody(comment.body));
lines.push( lines.push(
` <comment id="${leaf.comment.databaseId}" file="${escapeXml(leaf.comment.path)}" line="${line}">${preview}</comment>` ` <comment id="${comment.databaseId}" file="${escapeXml(comment.path)}" line="${line}">${preview}</comment>`
); );
} }
lines.push("</summary>"); lines.push("</summary>");
lines.push(""); lines.push("");
for (const leaf of leafComments) { for (const comment of comments) {
const line = leaf.comment.line ?? leaf.comment.startLine ?? 0; const line = comment.line ?? comment.startLine ?? 0;
const author = leaf.comment.author?.login ?? "unknown"; const author = comment.author?.login ?? "unknown";
lines.push( lines.push(
`<comment id="${leaf.comment.databaseId}" file="${escapeXml(leaf.comment.path)}" line="${line}" author="${escapeXml(author)}">` `<comment id="${comment.databaseId}" file="${escapeXml(comment.path)}" line="${line}" author="${escapeXml(author)}">`
); );
if (leaf.thread.length > 0) { const thread = flattenReplyToChain(comment.replyTo);
if (thread.length > 0) {
lines.push(" <thread>"); lines.push(" <thread>");
for (const msg of leaf.thread) { for (const msg of thread) {
const msgAuthor = msg.author?.login ?? "unknown";
lines.push( lines.push(
` <message id="${msg.databaseId}" author="${escapeXml(msgAuthor)}">${escapeXml(msg.body)}</message>` ` <message author="${escapeXml(msg.author)}">${escapeXml(msg.body)}</message>`
); );
} }
lines.push(" </thread>"); lines.push(" </thread>");
} }
lines.push(` <body>${escapeXml(leaf.comment.body)}</body>`); lines.push(" <diff>");
lines.push(escapeXml(comment.diffHunk));
lines.push(" </diff>");
lines.push(` <body>${escapeXml(comment.body)}</body>`);
lines.push("</comment>"); lines.push("</comment>");
lines.push(""); lines.push("");
} }
@@ -121400,13 +121391,13 @@ function GetReviewCommentsTool(ctx) {
const filename = approved_by ? `review-${review_id}-approved-by-${approved_by}.xml` : `review-${review_id}-comments.xml`; const filename = approved_by ? `review-${review_id}-approved-by-${approved_by}.xml` : `review-${review_id}-comments.xml`;
const commentsPath = join5(tempDir, filename); const commentsPath = join5(tempDir, filename);
writeFileSync3(commentsPath, content); writeFileSync3(commentsPath, content);
log.debug(`wrote ${leafComments.length} comments to ${commentsPath}`); log.info(`wrote ${comments.length} comments to ${commentsPath}`);
log.debug(`content: ${content}`); log.box(content);
return { return {
review_id, review_id,
pull_number, pull_number,
reviewer, reviewer,
count: leafComments.length, count: comments.length,
commentsPath commentsPath
}; };
}) })
@@ -121430,6 +121421,7 @@ function ListPullRequestReviewsTool(ctx) {
pull_number, pull_number,
reviews: reviews.map((review) => ({ reviews: reviews.map((review) => ({
id: review.id, id: review.id,
node_id: review.node_id,
body: review.body, body: review.body,
state: review.state, state: review.state,
user: review.user?.login, user: review.user?.login,
+125 -155
View File
@@ -5,44 +5,59 @@ import { log } from "../utils/log.ts";
import type { ToolContext } from "./server.ts"; import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts"; import { execute, tool } from "./shared.ts";
// graphql query to fetch all review threads with comments, replies, and reactions // fragment for nested replyTo (5 levels deep covers most threads)
const REVIEW_THREADS_QUERY = ` const REPLY_TO_FRAGMENT = `
query ($owner: String!, $repo: String!, $pullNumber: Int!) { replyTo {
repository(owner: $owner, name: $repo) { databaseId
pullRequest(number: $pullNumber) { body
reviewThreads(first: 100) { author { login }
replyTo {
databaseId
body
author { login }
replyTo {
databaseId
body
author { login }
replyTo {
databaseId
body
author { login }
replyTo {
databaseId
body
author { login }
}
}
}
}
}
`;
// fetch specific review by node ID with nested thread context (single efficient query)
const REVIEW_QUERY = `
query ($nodeId: ID!) {
node(id: $nodeId) {
... on PullRequestReview {
databaseId
author { login }
comments(first: 100) {
nodes { nodes {
diffSide databaseId
startDiffSide body
comments(first: 100) { path
nodes { line
id startLine
databaseId diffHunk
body url
path author { login }
line createdAt
startLine ${REPLY_TO_FRAGMENT}
url reactionGroups {
author { content
login reactors(first: 10) {
} nodes {
createdAt ... on Actor { login }
updatedAt
pullRequestReview {
databaseId
}
replyTo {
databaseId
}
reactionGroups {
content
reactors(first: 10) {
nodes {
... on Actor {
login
}
}
}
} }
} }
} }
@@ -53,55 +68,44 @@ query ($owner: String!, $repo: String!, $pullNumber: Int!) {
} }
`; `;
type GraphQLReviewComment = { // nested replyTo type (recursive up to 5 levels)
id: string; type NestedReplyTo = {
databaseId: number;
body: string;
author: { login: string } | null;
replyTo?: NestedReplyTo | null;
} | null;
type ReviewComment = {
databaseId: number; databaseId: number;
body: string; body: string;
path: string; path: string;
line: number | null; line: number | null;
startLine: number | null; startLine: number | null;
diffHunk: string;
url: string; url: string;
author: { author: { login: string } | null;
login: string;
} | null;
createdAt: string; createdAt: string;
updatedAt: string; replyTo: NestedReplyTo;
pullRequestReview: {
databaseId: number;
} | null;
replyTo: {
databaseId: number;
} | null;
reactionGroups: reactionGroups:
| { | {
content: string; content: string;
reactors: { reactors: { nodes: ({ login: string } | null)[] | null };
nodes: ({ login: string } | null)[] | null;
};
}[] }[]
| null; | null;
}; };
type GraphQLReviewThread = { type ReviewQueryResponse = {
diffSide: "LEFT" | "RIGHT"; node: {
startDiffSide: "LEFT" | "RIGHT" | null; databaseId: number;
comments: { author: { login: string } | null;
nodes: (GraphQLReviewComment | null)[] | null; comments: {
} | null; nodes: (ReviewComment | null)[] | null;
} | null;
type GraphQLResponse = {
repository: {
pullRequest: {
reviewThreads: {
nodes: (GraphQLReviewThread | null)[] | null;
} | null;
} | null; } | null;
} | null; } | null;
}; };
const MAX_BODY_PREVIEW = 80; const MAX_BODY_PREVIEW = 80;
const MAX_THREAD_DEPTH = 10;
function truncateBody(body: string): string { function truncateBody(body: string): string {
const oneLine = body.replace(/\n/g, " ").trim(); const oneLine = body.replace(/\n/g, " ").trim();
@@ -109,20 +113,6 @@ function truncateBody(body: string): string {
return oneLine.slice(0, MAX_BODY_PREVIEW - 3) + "..."; return oneLine.slice(0, MAX_BODY_PREVIEW - 3) + "...";
} }
// walk up the replyTo chain to get actual conversation (oldest first)
function getReplyChain(
comment: GraphQLReviewComment,
commentMap: Map<number, GraphQLReviewComment>,
depth = 0
): GraphQLReviewComment[] {
if (depth >= MAX_THREAD_DEPTH) return [];
const parentId = comment.replyTo?.databaseId;
if (!parentId) return [];
const parent = commentMap.get(parentId);
if (!parent) return [];
return [...getReplyChain(parent, commentMap, depth + 1), parent];
}
function escapeXml(text: string): string { function escapeXml(text: string): string {
return text return text
.replace(/&/g, "&amp;") .replace(/&/g, "&amp;")
@@ -131,13 +121,20 @@ function escapeXml(text: string): string {
.replace(/"/g, "&quot;"); .replace(/"/g, "&quot;");
} }
function hasThumbsUpFrom(comment: GraphQLReviewComment, username: string): boolean { function hasThumbsUpFrom(comment: ReviewComment, username: string): boolean {
if (!comment.reactionGroups) return false; if (!comment.reactionGroups) return false;
const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP"); const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP");
if (!thumbsUp?.reactors?.nodes) return false; if (!thumbsUp?.reactors?.nodes) return false;
return thumbsUp.reactors.nodes.some((r) => r?.login === username); return thumbsUp.reactors.nodes.some((r) => r?.login === username);
} }
// flatten nested replyTo chain into array (oldest first)
function flattenReplyToChain(replyTo: NestedReplyTo): Array<{ body: string; author: string }> {
if (!replyTo) return [];
const parent = flattenReplyToChain(replyTo.replyTo ?? null);
return [...parent, { body: replyTo.body, author: replyTo.author?.login ?? "unknown" }];
}
export const GetReviewComments = type({ export const GetReviewComments = type({
pull_number: type.number.describe("The pull request number"), pull_number: type.number.describe("The pull request number"),
review_id: type.number.describe("The review ID to get comments for"), review_id: type.number.describe("The review ID to get comments for"),
@@ -155,73 +152,45 @@ export function GetReviewCommentsTool(ctx: ToolContext) {
"Returns commentsPath pointing to a file with full comment details in XML format.", "Returns commentsPath pointing to a file with full comment details in XML format.",
parameters: GetReviewComments, parameters: GetReviewComments,
execute: execute(async ({ pull_number, review_id, approved_by }) => { execute: execute(async ({ pull_number, review_id, approved_by }) => {
// fetch all review threads using graphql (single API call) // fetch the review to get node_id and reviewer
const response = await ctx.octokit.graphql<GraphQLResponse>(REVIEW_THREADS_QUERY, { const { data: review } = await ctx.octokit.rest.pulls.getReview({
owner: ctx.repo.owner, owner: ctx.repo.owner,
repo: ctx.repo.name, repo: ctx.repo.name,
pullNumber: pull_number, pull_number,
review_id,
}); });
const pullRequest = response.repository?.pullRequest; const reviewer = review.user?.login ?? "unknown";
if (!pullRequest?.reviewThreads?.nodes) {
// fetch comments with nested thread context via GraphQL
const response = await ctx.octokit.graphql<ReviewQueryResponse>(REVIEW_QUERY, {
nodeId: review.node_id,
});
const reviewComments = response.node?.comments?.nodes;
if (!reviewComments) {
return { return {
review_id, review_id,
pull_number, pull_number,
reviewer: "unknown", reviewer,
count: 0, count: 0,
commentsPath: null, commentsPath: null,
message: "No review threads found", message: "No comments found for this review",
}; };
} }
// build a map of all comments for O(1) lookup when walking replyTo chains const allComments = reviewComments.filter((c): c is ReviewComment => c !== null);
const commentMap = new Map<number, GraphQLReviewComment>();
for (const thread of pullRequest.reviewThreads.nodes) {
if (!thread?.comments?.nodes) continue;
for (const comment of thread.comments.nodes) {
if (comment) commentMap.set(comment.databaseId, comment);
}
}
// collect leaf comments (from target review) with their thread context // filter by approved_by if specified
type LeafComment = { const comments = approved_by
comment: GraphQLReviewComment; ? allComments.filter((c) => hasThumbsUpFrom(c, approved_by))
thread: GraphQLReviewComment[]; // parent comments in order (oldest first) : allComments;
side: "LEFT" | "RIGHT";
};
const leafComments: LeafComment[] = [];
for (const thread of pullRequest.reviewThreads.nodes) { if (comments.length === 0) {
if (!thread?.comments?.nodes) continue;
const threadComments = thread.comments.nodes.filter(
(c): c is GraphQLReviewComment => c !== null
);
if (threadComments.length === 0) continue;
// find comments from the target review (these are the "leaf" comments to address)
for (const comment of threadComments) {
if (comment.pullRequestReview?.databaseId !== review_id) continue;
// filter by approved_by if specified
if (approved_by && !hasThumbsUpFrom(comment, approved_by)) continue;
// get thread context by walking up the replyTo chain (not just chronological)
const replyChain = getReplyChain(comment, commentMap);
leafComments.push({
comment,
thread: replyChain,
side: thread.diffSide,
});
}
}
if (leafComments.length === 0) {
return { return {
review_id, review_id,
pull_number, pull_number,
reviewer: "unknown", reviewer,
count: 0, count: 0,
commentsPath: null, commentsPath: null,
message: approved_by message: approved_by
@@ -230,50 +199,50 @@ export function GetReviewCommentsTool(ctx: ToolContext) {
}; };
} }
// derive reviewer from first comment (all comments in a review are from the same user)
const reviewer = leafComments[0].comment.author?.login ?? "unknown";
// build XML output // build XML output
const lines: string[] = []; const lines: string[] = [];
lines.push( lines.push(`<review_comments count="${comments.length}" reviewer="${escapeXml(reviewer)}">`);
`<review_comments count="${leafComments.length}" reviewer="${escapeXml(reviewer)}">`
);
lines.push(""); lines.push("");
// summary section // summary section
lines.push("<summary>"); lines.push("<summary>");
for (const leaf of leafComments) { for (const comment of comments) {
const line = leaf.comment.line ?? leaf.comment.startLine ?? 0; const line = comment.line ?? comment.startLine ?? 0;
const preview = escapeXml(truncateBody(leaf.comment.body)); const preview = escapeXml(truncateBody(comment.body));
lines.push( lines.push(
` <comment id="${leaf.comment.databaseId}" file="${escapeXml(leaf.comment.path)}" line="${line}">${preview}</comment>` ` <comment id="${comment.databaseId}" file="${escapeXml(comment.path)}" line="${line}">${preview}</comment>`
); );
} }
lines.push("</summary>"); lines.push("</summary>");
lines.push(""); lines.push("");
// detailed comments with thread context // detailed comments with thread context
for (const leaf of leafComments) { for (const comment of comments) {
const line = leaf.comment.line ?? leaf.comment.startLine ?? 0; const line = comment.line ?? comment.startLine ?? 0;
const author = leaf.comment.author?.login ?? "unknown"; const author = comment.author?.login ?? "unknown";
lines.push( lines.push(
`<comment id="${leaf.comment.databaseId}" file="${escapeXml(leaf.comment.path)}" line="${line}" author="${escapeXml(author)}">` `<comment id="${comment.databaseId}" file="${escapeXml(comment.path)}" line="${line}" author="${escapeXml(author)}">`
); );
// thread history (parent comments) // thread history (parent comments from nested replyTo)
if (leaf.thread.length > 0) { const thread = flattenReplyToChain(comment.replyTo);
if (thread.length > 0) {
lines.push(" <thread>"); lines.push(" <thread>");
for (const msg of leaf.thread) { for (const msg of thread) {
const msgAuthor = msg.author?.login ?? "unknown";
lines.push( lines.push(
` <message id="${msg.databaseId}" author="${escapeXml(msgAuthor)}">${escapeXml(msg.body)}</message>` ` <message author="${escapeXml(msg.author)}">${escapeXml(msg.body)}</message>`
); );
} }
lines.push(" </thread>"); lines.push(" </thread>");
} }
// diff context
lines.push(" <diff>");
lines.push(escapeXml(comment.diffHunk));
lines.push(" </diff>");
// the actual comment body to address // the actual comment body to address
lines.push(` <body>${escapeXml(leaf.comment.body)}</body>`); lines.push(` <body>${escapeXml(comment.body)}</body>`);
lines.push("</comment>"); lines.push("</comment>");
lines.push(""); lines.push("");
} }
@@ -292,14 +261,14 @@ export function GetReviewCommentsTool(ctx: ToolContext) {
: `review-${review_id}-comments.xml`; : `review-${review_id}-comments.xml`;
const commentsPath = join(tempDir, filename); const commentsPath = join(tempDir, filename);
writeFileSync(commentsPath, content); writeFileSync(commentsPath, content);
log.debug(`wrote ${leafComments.length} comments to ${commentsPath}`); log.info(`wrote ${comments.length} comments to ${commentsPath}`);
log.debug(`content: ${content}`); log.box(content);
return { return {
review_id, review_id,
pull_number, pull_number,
reviewer, reviewer,
count: leafComments.length, count: comments.length,
commentsPath, commentsPath,
}; };
}), }),
@@ -327,6 +296,7 @@ export function ListPullRequestReviewsTool(ctx: ToolContext) {
pull_number, pull_number,
reviews: reviews.map((review) => ({ reviews: reviews.map((review) => ({
id: review.id, id: review.id,
node_id: review.node_id,
body: review.body, body: review.body,
state: review.state, state: review.state,
user: review.user?.login, user: review.user?.login,