Enhanced PR review system to focus on critical comments only #10
Conversation
📖 WalkthroughEnhanced the PR review system to prioritize critical comments and provide a more structured review summary. Added functionality to filter out non-critical comments and generate a detailed review summary that includes commit history, processed files, and categorized comments (actionable vs skipped). Changes
autogenerated by presubmit.ai |
github-actions
bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/pull_request.ts
Outdated
| if (!!comment.end_line && (comment.critical || comment.label === "typo")) { | ||
| lineComments.push(comment); | ||
| } else { | ||
| skippedComments.push(comment); | ||
| } | ||
| } |
There was a problem hiding this comment.
The condition !!comment.end_line is redundant since end_line is already required in the schema. Additionally, the filtering logic for critical comments should be more explicit. Consider:
if (comment.critical || comment.label === 'typo') {
lineComments.push(comment);
} else {
skippedComments.push(comment);
}
src/messages.ts
Outdated
| if (actionableComments.length == 0) { | ||
| body += `✅ **LGTM!**\n\n`; | ||
| } else { | ||
| body += `🚨 **Pull request needs attention.**\n\n`; |
There was a problem hiding this comment.
The condition uses == instead of === for strict equality comparison. While this might work, it's better to use strict equality in TypeScript/JavaScript to prevent potential type coercion issues:
if (actionableComments.length === 0) {
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- 317edc3: only submit critical comments
Files Processed (4)
- src/diff.ts (1 hunk)
- src/messages.ts (2 hunks)
- src/prompts.ts (5 hunks)
- src/pull_request.ts (5 hunks)
Actionable Comments (0)
Skipped Comments (3)
-
src/messages.ts [125-125]
readability: "Variable name could be more descriptive"
-
src/pull_request.ts [253-254]
enhancement: "Redundant condition check"
-
src/messages.ts [169-170]
maintainability: "Duplicate section header in code"
| if (comment.critical || comment.label === "typo") { | ||
| lineComments.push(comment); | ||
| } else { | ||
| skippedComments.push(comment); | ||
| } | ||
| } |
There was a problem hiding this comment.
The filtering criteria for comments (critical or typo) is currently hardcoded. Consider making this configurable through a settings file or environment variables to allow users to customize which types of comments they want to see as actionable vs skipped. This would provide more flexibility for different teams' needs.
| if (actionableComments.length === 0) { | ||
| body += `✅ **LGTM!**\n\n`; | ||
| } else { | ||
| body += `🚨 **Pull request needs attention.**\n\n`; | ||
| } |
There was a problem hiding this comment.
The status message could be more informative by including the count of actionable comments when attention is needed. Consider modifying the message to something like: 🚨 **Pull request needs attention (X actionable comments)**. This gives reviewers an immediate sense of the review's scope.
| let fileText = `- ${diff.filename}`; | ||
| if (diff.status === "renamed") { | ||
| fileText += ` (from ${diff.previous_filename})`; | ||
| } | ||
| fileText += ` _(${diff.hunks.length} ${ | ||
| diff.hunks.length === 1 ? "hunk" : "hunks" | ||
| })_`; | ||
| body += `${fileText}\n`; | ||
| } |
There was a problem hiding this comment.
Consider adding an indicator for the type of change (added/modified/deleted) for each file in the summary. While the code handles renamed files, it would be helpful to show the change type for other files as well. This information helps reviewers quickly understand the scope of changes in each file.
| // Try to submit all comments at once | ||
| try { |
There was a problem hiding this comment.
Consider adding a check for empty lineComments array before attempting to submit the review. While this might not cause an error, it's a good practice to handle edge cases explicitly. You could either skip the submission or log a debug message in such cases.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a885514: address comments
Files Processed (2)
- src/messages.ts (2 hunks)
- src/pull_request.ts (5 hunks)
Actionable Comments (0)
Skipped Comments (3)
-
src/messages.ts [125-129]
enhancement: "Status message could be more informative by including issue count"
-
src/pull_request.ts [253-258]
maintainability: "Extract comment filtering logic into a dedicated function"
-
src/pull_request.ts [260-261]
best practice: "Add validation for empty comments before review submission"
No description provided.