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

Conversation

bstanga
Copy link
Contributor

@bstanga bstanga commented Nov 9, 2024

Enhances the pull request review workflow by improving commit message display with clickable SHA links and detailed information. Sets Claude 3.5 Sonnet as the default LLM model and implements smarter overview comment handling by updating existing comments instead of creating duplicates. The changes improve the user interface for displaying commit and file information while streamlining the review process.

Copy link

github-actions bot commented Nov 9, 2024

📖 Walkthrough

Enhances the pull request review workflow by improving commit message display with clickable SHA links and detailed information. Sets Claude 3.5 Sonnet as the default LLM model and implements smarter overview comment handling by updating existing comments instead of creating duplicates. The changes improve the user interface for displaying commit and file information while streamlining the review process.

Changes

File Summary
.github/workflows/presubmit-review.yml Updates workflow name to 'Presubmit.ai' and sets claude-3-5-sonnet-20241022 as default model via environment variable. Renames job from review to Review.
action.yml Changes the action's branding icon from git-merge to check-square while maintaining black color theme.
src/messages.ts Implements improved commit message display with clickable SHA links and better file information formatting. Adds OVERVIEW_MESSAGE_SIGNATURE for comment identification and enhances details display with proper singular/plural hunk counting.
src/pull_request.ts Adds functionality to find and update existing overview comments instead of creating duplicates. Reorganizes comment handling flow and improves logging messages. Updates imports to reflect new message building functions.

autogenerated by presubmit.ai

Copy link

github-actions bot commented Nov 9, 2024

📖 Walkthrough

This PR improves the display of commit messages in the initial PR analysis comment by showing detailed commit information with links to each commit. It also explicitly sets the Claude 3.5 Sonnet model as the default LLM model both in the workflow configuration and environment variables.

Changes

File Summary
.github/workflows/presubmit-review.yml Added LLM_MODEL environment variable set to claude-3-5-sonnet-20241022 in the GitHub workflow configuration.
src/messages.ts Refactored buildInitialMessage function to show detailed commit information including clickable links to each commit, commit SHA, and commit messages. Added GitHub repository context for building commit URLs.
src/pull_request.ts Reorganized the code to fetch commit information before creating the initial comment, and updated the buildInitialMessage function call to pass commit data instead of just base and head SHAs.

@github-actions github-actions bot changed the title show more detailed commit message Enhance commit message display and set Claude model Nov 9, 2024
Copy link

github-actions bot commented Nov 9, 2024

📖 Walkthrough

Enhances the PR review workflow by improving commit message display with detailed information and clickable links. Sets Claude 3.5 Sonnet as the default LLM model both in workflow configuration and environment variables. Implements better handling of overview comments by updating existing ones instead of creating duplicates.

Changes

File Summary
.github/workflows/presubmit-review.yml Renames workflow to 'Presubmit.ai' and job to 'Review'. Sets claude-3-5-sonnet-20241022 as default model via both environment variable and input parameter.
action.yml Changes the action's branding icon from git-merge to check-square while maintaining black color.
src/messages.ts Refactors message building to include detailed commit information with GitHub links. Adds signature for overview messages and improves file summary display with proper singular/plural for hunks.
src/pull_request.ts Adds functionality to find and update existing overview comments instead of creating new ones. Reorganizes the comment creation flow and improves logging messages.

@github-actions github-actions bot changed the title Enhance commit message display and set Claude model Improve commit display and set Claude 3.5 Sonnet as default model Nov 9, 2024
@bstanga bstanga changed the title Improve commit display and set Claude 3.5 Sonnet as default model Enhance PR Review with Better Commit Display and Claude 3.5 Sonnet Default Nov 9, 2024
@bstanga bstanga changed the title Enhance PR Review with Better Commit Display and Claude 3.5 Sonnet Default Enhance PR Review with Claude 3.5 Sonnet and Improved Commit Display Nov 9, 2024
Copy link
Contributor Author

@bstanga bstanga left a comment

Choose a reason for hiding this comment

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

Review submitted

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

Comment on lines +51 to +53
let overviewComment = comments.find((comment) =>
comment.body?.includes(OVERVIEW_MESSAGE_SIGNATURE)
);
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)
 );

@@ -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:

Comment on lines +20 to 23
LLM_MODEL: "claude-3-5-sonnet-20241022"
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
with:
llm_model: "claude-3-5-sonnet-20241022"
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"

@bstanga bstanga merged commit 16cdbff into main Nov 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant