Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance PR Review with Claude 3.5 Sonnet and Improved Commit Display #5

Merged
merged 3 commits into from
Nov 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/presubmit-review.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Presubmit.ai - Code Review
name: Presubmit.ai

permissions:
contents: read
Expand All @@ -11,12 +11,13 @@ on:
types: [created]

jobs:
review:
Review:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The job name Review uses PascalCase while typical GitHub Actions job names use kebab-case or snake_case for better consistency with other workflows. Consider renaming to:

-  Review:
+  code-review:

runs-on: ubuntu-latest
steps:
- uses: presubmit/ai-reviewer@latest
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
LLM_MODEL: "claude-3-5-sonnet-20241022"
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
with:
llm_model: "claude-3-5-sonnet-20241022"
Comment on lines +20 to 23
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LLM model is specified both as an environment variable and as an input parameter. This creates redundancy and potential for inconsistency. Consider using only one method to specify the model:

         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          LLM_MODEL: "claude-3-5-sonnet-20241022"
           LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
         with:
           llm_model: "claude-3-5-sonnet-20241022"

Expand Down
2 changes: 1 addition & 1 deletion action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: "AI Code Reviewer for Pull Requests"
description: "Review, summarize and auto-fix pull requests with AI"
author: "Presubmit.ai"
branding:
icon: "git-merge"
icon: "check-square"
color: "black"

inputs:
Expand Down
42 changes: 35 additions & 7 deletions src/messages.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,41 @@
import { context } from "@actions/github";
import { FileDiff } from "./diff";
import { PullRequestSummary } from "./prompts";

export function buildInitialMessage(
baseCommit: string,
headCommit: string,
export const OVERVIEW_MESSAGE_SIGNATURE =
"<!-- presubmit.ai: overview message -->";

export function buildOverviewMessage(
commits: {
sha: string;
commit: {
message: string;
};
}[],
fileDiffs: FileDiff[]
): string {
const { owner, repo } = context.repo;

let message = `⏳ **Analyzing changes in this PR...** ⏳\n\n`;
message += `_This might take a few minutes, please wait_\n\n`;

// Group files by operation

message += `<details>\n<summary>📥 Commits</summary>\n\n`;
message += `Analyzing changes from base commit (\`${baseCommit}\`) to head commit (\`${headCommit}\`)\n\n`;
message += "\n</details>\n\n";
message += `Analyzing changes from base commit (\`${commits[0].sha.slice(
0,
7
)}\`) to head commit (\`${commits[commits.length - 1].sha.slice(0, 7)}\`):\n`;

for (const commit of commits.reverse()) {
message += `- [${commit.sha.slice(
0,
7
)}](https://github.com/${owner}/${repo}/commit/${commit.sha}): ${
commit.commit.message
}\n`;
}
Comment on lines +29 to +36
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message formatting could be simplified using a single-line template literal for better readability. Consider refactoring to:

- message += `- [${commit.sha.slice(
-      0,
-      7
-    )}](https://github.com/${owner}/${repo}/commit/${commit.sha}): ${
-      commit.commit.message
-    }\n`;
+ message += `- [${commit.sha.slice(0, 7)}](https://github.com/${owner}/${repo}/commit/${commit.sha}): ${commit.commit.message}\n`;


message += "\n\n</details>\n\n";

message += `<details>\n<summary>📁 Files being considered (${fileDiffs.length})</summary>\n\n`;
for (const diff of fileDiffs) {
Expand All @@ -26,13 +48,17 @@ export function buildInitialMessage(
if (diff.status === "renamed") {
fileText += ` (from ${diff.previous_filename})`;
}
fileText += ` _(${diff.hunks.length} hunks)_`;
fileText += ` _(${diff.hunks.length} ${
diff.hunks.length === 1 ? "hunk" : "hunks"
})_`;
message += `${fileText}\n`;
}
message += "\n</details>\n\n";

message += "--- \n_autogenerated by presubmit.ai_";

message += "\n" + OVERVIEW_MESSAGE_SIGNATURE;

return message;
}

Expand All @@ -56,5 +82,7 @@ export function buildWalkthroughMessage(summary: PullRequestSummary): string {
message += `| \`${escapedPath}\` | ${escapedSummary} |\n`;
}

message += "\n" + OVERVIEW_MESSAGE_SIGNATURE;

return message;
}
49 changes: 34 additions & 15 deletions src/pull_request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ import { Config } from "./config";
import { initOctokit } from "./octokit";
import { loadContext } from "./context";
import runSummaryPrompt from "./prompts";
import { buildInitialMessage, buildWalkthroughMessage } from "./messages";
import {
buildLoadingMessage,
buildWalkthroughMessage,
OVERVIEW_MESSAGE_SIGNATURE,
} from "./messages";
import { parseFileDiff } from "./diff";

export async function handlePullRequest(config: Config) {
Expand Down Expand Up @@ -32,24 +36,39 @@ export async function handlePullRequest(config: Config) {
const fileDiffs = files.map(parseFileDiff);
info(`successfully fetched file diffs`);

// Create initial comment with the summary
const initialComment = await octokit.rest.issues.createComment({
...context.repo,
issue_number: pull_request.number,
body: buildInitialMessage(
pull_request.base.sha,
pull_request.head.sha,
fileDiffs
),
});
info(`posted initial comment`);
// Get commit messages
const { data: commits } = await octokit.rest.pulls.listCommits({
...context.repo,
pull_number: pull_request.number,
});
info(`successfully fetched commit messages`);

// Find or create overview comment with the summary
const { data: comments } = await octokit.rest.issues.listComments({
...context.repo,
issue_number: pull_request.number,
});
let overviewComment = comments.find((comment) =>
comment.body?.includes(OVERVIEW_MESSAGE_SIGNATURE)
);
Comment on lines +51 to +53
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're using the optional chaining operator (?.), it's good practice to be explicit about null/undefined checks when searching for existing comments. Consider adding a null check:

- let overviewComment = comments.find((comment) =>
-   comment.body?.includes(OVERVIEW_MESSAGE_SIGNATURE)
- );
+ let overviewComment = comments.find((comment) =>
   comment.body && comment.body.includes(OVERVIEW_MESSAGE_SIGNATURE)
 );

if (overviewComment) {
await octokit.rest.issues.updateComment({
...context.repo,
comment_id: overviewComment.id,
body: buildLoadingMessage(commits, fileDiffs),
});
info(`updated existing overview comment`);
} else {
overviewComment = (
await octokit.rest.issues.createComment({
...context.repo,
issue_number: pull_request.number,
body: buildLoadingMessage(commits, fileDiffs),
})
).data;
info(`posted new overview loading comment`);
}

// Generate PR summary
const summary = await runSummaryPrompt({
prTitle: pull_request.title,
Expand All @@ -68,11 +87,11 @@ export async function handlePullRequest(config: Config) {
});
info(`updated pull request title and description`);

// Update initial comment with the walkthrough
// Update overview comment with the walkthrough
await octokit.rest.issues.updateComment({
...context.repo,
comment_id: initialComment.data.id,
comment_id: overviewComment.id,
body: buildWalkthroughMessage(summary),
});
info(`posted walkthrough`);
info(`updated overview comment with walkthrough`);
}