-
Notifications
You must be signed in to change notification settings - Fork 6
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
Added Pull Request Comment Handling Infrastructure #13
Conversation
PR SummaryImplemented infrastructure for handling and responding to pull request review comments. Added functionality to track comment threads, filter relevant comments, and generate AI-powered responses to review comments. The system now supports identifying comment threads, managing review comment context, and providing appropriate responses while maintaining the conversation flow. Changes
autogenerated by presubmit.ai |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 7d7f1dd: add skeleton for handling comments
Files Processed (2)
- src/main.ts (2 hunks)
- src/messages.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (1)
-
src/main.ts [13-13]
best practice: "Missing error handling for new function call"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 17e51a5: also add the mew file
Files Processed (1)
- src/pull_request_comment.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (2)
-
src/pull_request_comment.ts [9-12]
best practice: "Event name validation could be more specific"
-
src/pull_request_comment.ts [35-37]
possible issue: "Comment signature check might have false positives"
const { comment, pull_request } = context.payload; | ||
if (!comment) { | ||
warning("`comment` is missing from payload"); | ||
return; | ||
} | ||
if (context.payload.action !== "created") { | ||
warning("only consider newly created comments"); | ||
return; | ||
} | ||
if (!pull_request) { | ||
warning("`pull_request` is missing from payload"); | ||
return; | ||
} | ||
if (isOwnComment(comment.body)) { | ||
info("ignoring own comments"); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@presubmitai can we somehow improve this part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually nevermind. It's fine. No longer needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually you know what, you can actually help me. Can you replace warning statement with console.error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bstanga Sure! You'll need to update the import statement and replace all instances of warning()
with console.error()
. Here's what needs to be changed:
- Remove
warning
from the import statement on line 1 - Replace these lines:
warning("unsupported github event");
warning("`comment` is missing from payload");
warning("only consider newly created comments");
warning("`pull_request` is missing from payload");
with:
console.error("unsupported github event");
console.error("`comment` is missing from payload");
console.error("only consider newly created comments");
console.error("`pull_request` is missing from payload");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show me how to code will look like after I replace those? So that I can simply copy paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bstanga Here's how the code will look after replacing warning
with console.error
:
import { info } from "@actions/core";
import { loadContext } from "./context";
import config from "./config";
import { initOctokit } from "./octokit";
import { COMMENT_SIGNATURE } from "./messages";
export async function handlePullRequestComment() {
const context = await loadContext();
if (context.eventName !== "pull_request_review_comment") {
console.error("unsupported github event");
return;
}
const { comment, pull_request } = context.payload;
if (!comment) {
console.error("`comment` is missing from payload");
return;
}
if (context.payload.action !== "created") {
console.error("only consider newly created comments");
return;
}
if (!pull_request) {
console.error("`pull_request` is missing from payload");
return;
}
if (isOwnComment(comment.body)) {
info("ignoring own comments");
return;
}
// TODO: implement
}
function isOwnComment(comment: string): boolean {
return comment.includes(COMMENT_SIGNATURE);
}
Note that I removed warning
from the import statement since it's no longer needed, but kept info
since it's still being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Files Processed (7)
- dist/index.js (0 hunks)
- src/comments.ts (1 hunk)
- src/context.ts (2 hunks)
- src/diff.ts (6 hunks)
- src/prompts.ts (3 hunks)
- src/pull_request.ts (4 hunks)
- src/pull_request_comment.ts (1 hunk)
Actionable Comments (2)
-
src/comments.ts [36-42]
possible bug: "Potential issue with user login mapping"
-
src/comments.ts [80-82]
possible bug: "Potential issue with comment filtering"
Skipped Comments (1)
-
src/pull_request_comment.ts [78-85]
best practice: "Missing error handling in API call"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 3e06df8: small update
Files Processed (1)
- src/comments.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (3)
-
src/comments.ts [81-81]
possible issue: "Potential issue with comment filtering logic"
-
src/comments.ts [67-74]
enhancement: "Function marked as async but doesn't use any await operations"
-
src/comments.ts [71-73]
enhancement: "Redundant mention checks in comment relevance"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Files Processed (3)
- dist/index.js (0 hunks)
- src/comments.ts (1 hunk)
- src/pull_request_comment.ts (1 hunk)
Actionable Comments (1)
-
src/pull_request_comment.ts [87-87]
possible bug: "Potential runtime error due to undefined property"
Skipped Comments (2)
-
src/comments.ts [78-80]
possible issue: "Potential issue with comment filtering logic"
-
src/pull_request_comment.ts [58-58]
enhancement: "Inefficient array mapping with empty array parameter"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- a49607d: fix presubmit bug
Files Processed (1)
- src/pull_request_comment.ts (1 hunk)
Actionable Comments (1)
-
src/pull_request_comment.ts [76-76]
possible bug: "Potential undefined behavior in response handling"
Skipped Comments (2)
-
src/pull_request_comment.ts [58-58]
performance: "Potential memory inefficiency in file diff processing"
-
src/pull_request_comment.ts [34-34]
best practice: "Potential error handling improvement needed"
}); | ||
|
||
// Submit response if action requested | ||
if (!response.action_requested || !response.response_comment.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition !response.action_requested || !response.response_comment.length
might throw if response_comment
is undefined. Consider adding a null check: !response.action_requested || !response.response_comment?.length
Add skeleton for handling comments