Merge pull request #8 from presubmit/incremental

incremental reviews

src/diff.ts

@@ -16,21 +16,38 @@ export type Hunk = {
   startLine: number;
   endLine: number;
   diff: string;
+  commentChains?: {
+    comments: ReviewComment[];
+  }[];
 };
 
 export type FileDiff = File & {
   hunks: Hunk[];
 };
 
-export function parseFileDiff(file: File): FileDiff {
+type ReviewComment = {
+  path: string;
+  body: string;
+  line?: number;
+  in_reply_to_id?: number;
+  id: number;
+  start_line?: number | null;
+  user: {
+    login: string;
+  };
+};
+export function parseFileDiff(
+  file: File,
+  reviewComments: ReviewComment[]
+): FileDiff {
   if (!file.patch) {
     return {
       ...file,
       hunks: [],
     };
   }
 
-  const hunks: Hunk[] = [];
+  let hunks: Hunk[] = [];
 
   let currentHunk: Hunk | null = null;
   for (const line of file.patch.split("\n")) {
@@ -55,12 +72,48 @@ export function parseFileDiff(file: File): FileDiff {
     hunks.push(currentHunk);
   }
 
+  hunks = hunks.map((hunk) => {
+    return {
+      ...hunk,
+      commentChains: generateCommentChains(file, hunk, reviewComments),
+    };
+  });
+
   return {
     ...file,
     hunks,
   };
 }
 
+function generateCommentChains(
+  file: File,
+  hunk: Hunk,
+  reviewComments: ReviewComment[]
+): { comments: ReviewComment[] }[] {
+  const topLevelComments = reviewComments.filter(
+    (c) =>
+      !c.in_reply_to_id &&
+      c.path === file.filename &&
+      c.body.length &&
+      c.line &&
+      c.line <= hunk.endLine &&
+      c.line >= hunk.startLine &&
+      (!c.start_line ||
+        (c.start_line <= hunk.endLine && c.start_line >= hunk.startLine))
+  );
+
+  return topLevelComments.map((topLevelComment) => {
+    return {
+      comments: [
+        topLevelComment,
+        ...reviewComments.filter(
+          (c) => c.in_reply_to_id === topLevelComment.id
+        ),
+      ],
+    };
+  });
+}
+
 function removeDeletedLines(hunk: Hunk): Hunk {
   return {
     ...hunk,
@@ -101,8 +154,7 @@ function prependLineNumbers(hunk: Hunk): Hunk {
   });
 
   return {
-    startLine: hunk.startLine,
-    endLine: hunk.endLine,
+    ...hunk,
     diff: numberedLines.join("\n"),
   };
 }
@@ -153,6 +205,14 @@ function formatDiffHunk(hunk: Hunk): string {
     output += `__old hunk__\n${oldContent}\n`;
   }
 
+  if (hunk.commentChains?.length) {
+    output += `__comment_chain__\n${hunk.commentChains
+      .map((c) =>
+        c.comments.map((c) => `@${c.user.login}: ${c.body}`).join("\n")
+      )
+      .join("\n\n")}\n`;
+  }
+
   return output || "No changes in this hunk";
 }
 
@@ -187,5 +247,7 @@ export function generateFileCodeDiff(fileDiff: FileDiff): string {
     header += `\n\n${hunksText}`;
   }
 
+  console.log(header);
+
   return header;
 }

src/messages.ts

@@ -5,9 +5,13 @@ import { PullRequestSummary } from "./prompts";
 export const OVERVIEW_MESSAGE_SIGNATURE =
   "\n<!-- presubmit.ai: overview message -->";
 
+export const PAYLOAD_TAG_OPEN = "\n<!-- presubmit.ai: payload --";
+export const PAYLOAD_TAG_CLOSE = "\n-- presubmit.ai: payload -->";
+
 const PRESUBMIT_SIGNATURE = "--- \n_autogenerated by presubmit.ai_";
 
 export function buildLoadingMessage(
+  baseCommit: string,
   commits: {
     sha: string;
     commit: {
@@ -23,10 +27,13 @@ export function buildLoadingMessage(
 
   // Group files by operation
   message += `<details>\n<summary>📥 Commits</summary>\n\n`;
-  message += `Analyzing changes from base commit (\`${commits[0].sha.slice(
+  message += `Analyzing changes from base (\`${baseCommit.slice(
+    0,
+    7
+  )}\`) to latest commit (\`${commits[commits.length - 1].sha.slice(
     0,
     7
-  )}\`) to head commit (\`${commits[commits.length - 1].sha.slice(0, 7)}\`):\n`;
+  )}\`):\n`;
 
   for (const commit of commits.reverse()) {
     message += `- [${commit.sha.slice(
@@ -63,7 +70,10 @@ export function buildLoadingMessage(
   return message;
 }
 
-export function buildWalkthroughMessage(summary: PullRequestSummary): string {
+export function buildWalkthroughMessage(
+  summary: PullRequestSummary,
+  commits: string[]
+): string {
   let message = `# 📖 Walkthrough\n\n`;
 
   // Add description with proper spacing
@@ -83,8 +93,15 @@ export function buildWalkthroughMessage(summary: PullRequestSummary): string {
     message += `| \`${escapedPath}\` | ${escapedSummary} |\n`;
   }
 
+  const payload = {
+    commits: commits,
+  };
+
   message += PRESUBMIT_SIGNATURE;
   message += OVERVIEW_MESSAGE_SIGNATURE;
+  message += PAYLOAD_TAG_OPEN;
+  message += JSON.stringify(payload);
+  message += PAYLOAD_TAG_CLOSE;
 
   return message;
 }

src/prompts.ts

@@ -148,6 +148,13 @@ __old hunk__
  unchanged code line1
 -old code line2 removed in the PR
  unchanged code line3
+ __existing_comment_thread__
+ presubmitai: This is a comment on the code
+ user2: This is a reply to the comment above
+ __existing_comment_thread__
+ presubmitai: This is a comment on some other parts of the code
+ user2: This is a reply to the above comment
+
 
 @@ ... @@ def func2():
 __new hunk__
@@ -163,6 +170,7 @@ __new hunk__
 - We also added line numbers for the '__new hunk__' code, to help you refer to the code lines in your suggestions. These line numbers are not part of the actual code, and should only used for reference.
 - Code lines are prefixed with symbols ('+', '-', ' '). The '+' symbol indicates new code added in the PR, the '-' symbol indicates code removed in the PR, and the ' ' symbol indicates unchanged code. The review should address new code added in the PR code diff (lines starting with '+')
 - Use markdown formatting for your comments.
+- Do not return comments that are already covered by an existing comment chain.
 
 </IMPORTANT INSTRUCTIONS>
     

src/pull_request.ts

@@ -7,6 +7,8 @@ import {
   buildLoadingMessage,
   buildWalkthroughMessage,
   OVERVIEW_MESSAGE_SIGNATURE,
+  PAYLOAD_TAG_CLOSE,
+  PAYLOAD_TAG_OPEN,
 } from "./messages";
 import { parseFileDiff } from "./diff";
 import { Octokit } from "@octokit/action";
@@ -30,14 +32,6 @@ export async function handlePullRequest() {
 
   const octokit = initOctokit(config.githubToken);
 
-  // Get modified files
-  const { data: files } = await octokit.rest.pulls.listFiles({
-    ...context.repo,
-    pull_number: pull_request.number,
-  });
-  const fileDiffs = files.map(parseFileDiff);
-  info(`successfully fetched file diffs`);
-
   // Get commit messages
   const { data: commits } = await octokit.rest.pulls.listCommits({
     ...context.repo,
@@ -53,19 +47,93 @@ export async function handlePullRequest() {
   let overviewComment = existingComments.find((comment) =>
     comment.body?.includes(OVERVIEW_MESSAGE_SIGNATURE)
   );
+  const isIncrementalReview = !!overviewComment;
+
+  // Maybe fetch review comments
+  const reviewComments = isIncrementalReview
+    ? (
+        await octokit.rest.pulls.listReviewComments({
+          ...context.repo,
+          pull_number: pull_request.number,
+        })
+      ).data
+    : [];
+
+  // Get modified files
+  const { data: files } = await octokit.rest.pulls.listFiles({
+    ...context.repo,
+    pull_number: pull_request.number,
+  });
+  let filesToReview = files.map((file) => parseFileDiff(file, reviewComments));
+  info(`successfully fetched file diffs`);
+
+  let commitsReviewed: string[] = [];
+  let lastCommitReviewed: string | null = null;
+  if (overviewComment) {
+    info(`running incremental review`);
+    try {
+      const payload = JSON.parse(
+        overviewComment.body
+          ?.split(PAYLOAD_TAG_OPEN)[1]
+          .split(PAYLOAD_TAG_CLOSE)[0] || "{}"
+      );
+      commitsReviewed = payload.commits;
+    } catch (error) {
+      warning(`error parsing overview payload: ${error}`);
+    }
+
+    // Check if there are any incremental changes
+    lastCommitReviewed =
+      commitsReviewed.length > 0
+        ? commitsReviewed[commitsReviewed.length - 1]
+        : null;
+    const incrementalDiff =
+      lastCommitReviewed && lastCommitReviewed != pull_request.head.sha
+        ? await octokit.rest.repos.compareCommits({
+            ...context.repo,
+            base: lastCommitReviewed,
+            head: pull_request.head.sha,
+          })
+        : null;
+    if (incrementalDiff?.data?.files) {
+      // If incremental review, only consider files that were modified within incremental change.
+      filesToReview = filesToReview.filter((f) =>
+        incrementalDiff.data.files?.some((f2) => f2.filename === f.filename)
+      );
+    }
+  } else {
+    info(`running full review`);
+  }
+
+  const commitsToReview = commitsReviewed.length
+    ? commits.filter((c) => !commitsReviewed.includes(c.sha))
+    : commits;
+  if (commitsToReview.length === 0) {
+    info(`no new commits to review`);
+    return;
+  }
+
   if (overviewComment) {
     await octokit.rest.issues.updateComment({
       ...context.repo,
       comment_id: overviewComment.id,
-      body: buildLoadingMessage(commits, fileDiffs),
+      body: buildLoadingMessage(
+        lastCommitReviewed ?? pull_request.base.sha,
+        commitsToReview,
+        filesToReview
+      ),
     });
     info(`updated existing overview comment`);
   } else {
     overviewComment = (
       await octokit.rest.issues.createComment({
         ...context.repo,
         issue_number: pull_request.number,
-        body: buildLoadingMessage(commits, fileDiffs),
+        body: buildLoadingMessage(
+          pull_request.base.sha,
+          commitsToReview,
+          filesToReview
+        ),
       })
     ).data;
     info(`posted new overview loading comment`);
@@ -80,26 +148,32 @@ export async function handlePullRequest() {
   });
   info(`generated pull request summary: ${summary.title}`);
 
-  // Update PR title and description
-  //   await octokit.rest.pulls.update({
-  //     ...context.repo,
-  //     pull_number: pull_request.number,
-  //     title: summary.title,
-  //     body: summary.description,
-  //   });
-  //   info(`updated pull request title and description`);
+  // Update PR title if @presubmitai is mentioned in the title
+  if (pull_request.title.includes("@presubmitai")) {
+    info(`title contains @presubmitai, so generating a new title`);
+    await octokit.rest.pulls.update({
+      ...context.repo,
+      pull_number: pull_request.number,
+      title: summary.title,
+      // body: summary.description,
+    });
+  }
 
   // Update overview comment with the walkthrough
   await octokit.rest.issues.updateComment({
     ...context.repo,
     comment_id: overviewComment.id,
-    body: buildWalkthroughMessage(summary),
+    body: buildWalkthroughMessage(
+      summary,
+      commits.map((c) => c.sha)
+    ),
   });
   info(`updated overview comment with walkthrough`);
 
-  // Review PR code changes
+  // ======= START REVIEW =======
+
   const review = await runReviewPrompt({
-    files: fileDiffs,
+    files: filesToReview,
     prTitle: pull_request.title,
     prDescription: pull_request.body || "",
     prSummary: summary.description,
@@ -190,7 +264,7 @@ async function submitReview(
       pull_number: pull_request.number,
       review_id: review.data.id,
       event: "COMMENT",
-      body: "Review submitted",
+      body: "Review complete",
     });
   } catch (error) {
     warning(`error submitting review: ${error}`);