Skip to content

Commit 1e3df15

Browse files
anc95anc95
authored
Merge pull request #204 from anc95/chao
Optimize comment position
2 parents 6fdbaea + 96ae8f8 commit 1e3df15

4 files changed

Lines changed: 203 additions & 49 deletions

File tree

‎action/index.cjs‎

Lines changed: 92 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -150453,18 +150453,56 @@ const robot = (app) => {
150453150453
head: context.payload.pull_request.head.sha,
150454150454
});
150455150455
let { files: changedFiles, commits } = data.data;
150456-
loglevel_1.default.debug("compareCommits, base:", context.payload.pull_request.base.sha, "head:", context.payload.pull_request.head.sha);
150457-
loglevel_1.default.debug("compareCommits.commits:", commits);
150458-
loglevel_1.default.debug("compareCommits.files", changedFiles);
150459-
if (context.payload.action === 'synchronize' && commits.length >= 2) {
150460-
const { data: { files }, } = await context.octokit.repos.compareCommits({
150461-
owner: repo.owner,
150462-
repo: repo.repo,
150463-
base: commits[commits.length - 2].sha,
150464-
head: commits[commits.length - 1].sha,
150465-
});
150466-
changedFiles = files;
150456+
if (context.payload.action === 'synchronize') {
150457+
// Try to detect the last commit we reviewed (by looking for our previous review)
150458+
try {
150459+
const reviewsResp = await context.octokit.pulls.listReviews({
150460+
owner: repo.owner,
150461+
repo: repo.repo,
150462+
pull_number: context.pullRequest().pull_number,
150463+
});
150464+
const reviews = reviewsResp.data || [];
150465+
// Find the most recent review created by this bot (we mark our reviews with a body)
150466+
const botReview = reviews
150467+
.slice()
150468+
.reverse()
150469+
.find((r) => r.body && (r.body.startsWith('Code review by ChatGPT') || r.body.startsWith('LGTM')));
150470+
if (botReview?.commit_id) {
150471+
const { data: { files, commits: newCommits }, } = await context.octokit.repos.compareCommits({
150472+
owner: repo.owner,
150473+
repo: repo.repo,
150474+
base: botReview.commit_id,
150475+
head: context.payload.pull_request.head.sha,
150476+
});
150477+
changedFiles = files;
150478+
commits = newCommits;
150479+
}
150480+
else if (commits.length >= 2) {
150481+
// fallback: compare last two commits in the PR
150482+
const { data: { files }, } = await context.octokit.repos.compareCommits({
150483+
owner: repo.owner,
150484+
repo: repo.repo,
150485+
base: commits[commits.length - 2].sha,
150486+
head: commits[commits.length - 1].sha,
150487+
});
150488+
changedFiles = files;
150489+
}
150490+
}
150491+
catch (err) {
150492+
loglevel_1.default.debug('failed to detect previous bot review, falling back', err);
150493+
if (commits.length >= 2) {
150494+
const { data: { files }, } = await context.octokit.repos.compareCommits({
150495+
owner: repo.owner,
150496+
repo: repo.repo,
150497+
base: commits[commits.length - 2].sha,
150498+
head: commits[commits.length - 1].sha,
150499+
});
150500+
changedFiles = files;
150501+
}
150502+
}
150467150503
}
150504+
loglevel_1.default.debug('changedFiles:', changedFiles);
150505+
loglevel_1.default.debug;
150468150506
const ignoreList = (process.env.IGNORE || process.env.ignore || '')
150469150507
.split('\n')
150470150508
.filter((v) => v !== '');
@@ -150507,16 +150545,36 @@ const robot = (app) => {
150507150545
}
150508150546
try {
150509150547
const res = await chat?.codeReview(patch);
150510-
if (!res.lgtm && !!res.review_comment) {
150511-
ress.push({
150512-
path: file.filename,
150513-
body: res.review_comment,
150514-
position: patch.split('\n').length - 1,
150515-
});
150548+
// res can be a single review or an array of reviews (one for each hunk)
150549+
const reviews = Array.isArray(res) ? res : [res];
150550+
for (const review of reviews) {
150551+
if (!review.lgtm && !!review.review_comment) {
150552+
let line;
150553+
let side = 'RIGHT';
150554+
// Extract line number from hunk header if available
150555+
if (review.hunk_header) {
150556+
const c = review.hunk_header.match(/\+\s*(\d+),(\d+)/);
150557+
if (c) {
150558+
const [, start, count] = c;
150559+
line = Number(start) + Number(count) - 1;
150560+
}
150561+
else {
150562+
loglevel_1.default.error(`Failed to parse hunk header: ${review.hunk_header}`);
150563+
continue;
150564+
}
150565+
}
150566+
ress.push({
150567+
path: file.filename,
150568+
body: review.review_comment,
150569+
line: line,
150570+
side: side,
150571+
});
150572+
}
150516150573
}
150517150574
}
150518150575
catch (e) {
150519150576
loglevel_1.default.info(`review ${file.filename} failed`, e);
150577+
throw e;
150520150578
}
150521150579
}
150522150580
try {
@@ -150526,12 +150584,13 @@ const robot = (app) => {
150526150584
pull_number: context.pullRequest().pull_number,
150527150585
body: ress.length ? "Code review by ChatGPT" : "LGTM 👍",
150528150586
event: 'COMMENT',
150529-
commit_id: commits[commits.length - 1].sha,
150587+
commit_id: context.payload.pull_request.head.sha,
150530150588
comments: ress,
150531150589
});
150532150590
}
150533150591
catch (e) {
150534150592
loglevel_1.default.info(`Failed to create review`, e);
150593+
throw e;
150535150594
}
150536150595
console.timeEnd('gpt cost');
150537150596
loglevel_1.default.info('successfully reviewed', context.payload.pull_request.html_url);
@@ -150598,10 +150657,16 @@ class Chat {
150598150657
const userPrompt = process.env.PROMPT || 'Please review the following code patch. Focus on potential bugs, risks, and improvement suggestions.';
150599150658
const jsonFormatRequirement = '\nProvide your feedback in a strict JSON format with the following structure:\n' +
150600150659
'{\n' +
150601-
' "lgtm": boolean, // true if the code looks good to merge, false if there are concerns\n' +
150602-
' "review_comment": string // Your detailed review comments. You can use markdown syntax in this string, but the overall response must be a valid JSON\n' +
150660+
' "reviews": [\n' +
150661+
' {\n' +
150662+
' "hunk_header": string, // The @@ hunk header (e.g., "@@ -10,5 +10,7 @@"), optional\n' +
150663+
' "lgtm": boolean, // true if this hunk looks good, false if there are concerns\n' +
150664+
' "review_comment": string // Your detailed review comments for this hunk. Can use markdown syntax. Empty string if lgtm is true.\n' +
150665+
' }\n' +
150666+
' ]\n' +
150603150667
'}\n' +
150604-
'Ensure your response is a valid JSON object.\n';
150668+
'Review each hunk (marked by @@) separately and provide feedback for hunks that need improvement.\n' +
150669+
'Ensure your response is a valid JSON object with a reviews array.\n';
150605150670
return `${userPrompt}${jsonFormatRequirement} ${answerLanguage}:
150606150671
${patch}
150607150672
`;
@@ -150634,11 +150699,17 @@ class Chat {
150634150699
if (res.choices.length) {
150635150700
try {
150636150701
const json = JSON.parse(res.choices[0].message.content || "");
150702+
// If response has a 'reviews' array, return it directly
150703+
if (json.reviews && Array.isArray(json.reviews)) {
150704+
return json.reviews;
150705+
}
150706+
// Otherwise, treat as a single review response
150637150707
return json;
150638150708
}
150639150709
catch (e) {
150640150710
return {
150641150711
lgtm: false,
150712+
hunk_header: patch.split('\n')[0].startsWith('@@') ? patch.split('\n')[0] : undefined,
150642150713
review_comment: res.choices[0].message.content || ""
150643150714
};
150644150715
}

‎action/src/chat.d.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,13 @@ export declare class Chat {
44
private isGithubModels;
55
constructor(apikey: string);
66
private generatePrompt;
7-
codeReview: (patch: string) => Promise<{
7+
codeReview: (patch: string) => Promise<Array<{
88
lgtm: boolean;
99
review_comment: string;
10+
hunk_header?: string;
11+
}> | {
12+
lgtm: boolean;
13+
review_comment: string;
14+
hunk_header?: string;
1015
}>;
1116
}

‎src/bot.ts‎

Lines changed: 88 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -89,23 +89,67 @@ export const robot = (app: Probot) => {
8989

9090
let { files: changedFiles, commits } = data.data;
9191

92-
log.debug("compareCommits, base:", context.payload.pull_request.base.sha, "head:", context.payload.pull_request.head.sha)
93-
log.debug("compareCommits.commits:", commits)
94-
log.debug("compareCommits.files", changedFiles)
95-
96-
if (context.payload.action === 'synchronize' && commits.length >= 2) {
97-
const {
98-
data: { files },
99-
} = await context.octokit.repos.compareCommits({
100-
owner: repo.owner,
101-
repo: repo.repo,
102-
base: commits[commits.length - 2].sha,
103-
head: commits[commits.length - 1].sha,
104-
});
105-
106-
changedFiles = files
92+
if (context.payload.action === 'synchronize') {
93+
// Try to detect the last commit we reviewed (by looking for our previous review)
94+
try {
95+
const reviewsResp = await context.octokit.pulls.listReviews({
96+
owner: repo.owner,
97+
repo: repo.repo,
98+
pull_number: context.pullRequest().pull_number,
99+
});
100+
101+
const reviews = reviewsResp.data || [];
102+
// Find the most recent review created by this bot (we mark our reviews with a body)
103+
const botReview = reviews
104+
.slice()
105+
.reverse()
106+
.find((r) => r.body && (r.body.startsWith('Code review by ChatGPT') || r.body.startsWith('LGTM')));
107+
108+
if (botReview?.commit_id) {
109+
const {
110+
data: { files, commits: newCommits },
111+
} = await context.octokit.repos.compareCommits({
112+
owner: repo.owner,
113+
repo: repo.repo,
114+
base: botReview.commit_id,
115+
head: context.payload.pull_request.head.sha,
116+
});
117+
118+
changedFiles = files;
119+
commits = newCommits;
120+
} else if (commits.length >= 2) {
121+
// fallback: compare last two commits in the PR
122+
const {
123+
data: { files },
124+
} = await context.octokit.repos.compareCommits({
125+
owner: repo.owner,
126+
repo: repo.repo,
127+
base: commits[commits.length - 2].sha,
128+
head: commits[commits.length - 1].sha,
129+
});
130+
131+
changedFiles = files;
132+
}
133+
} catch (err) {
134+
log.debug('failed to detect previous bot review, falling back', err);
135+
if (commits.length >= 2) {
136+
const {
137+
data: { files },
138+
} = await context.octokit.repos.compareCommits({
139+
owner: repo.owner,
140+
repo: repo.repo,
141+
base: commits[commits.length - 2].sha,
142+
head: commits[commits.length - 1].sha,
143+
});
144+
145+
changedFiles = files;
146+
}
147+
}
107148
}
108149

150+
log.debug('changedFiles:', changedFiles);
151+
log.debug
152+
109153
const ignoreList = (process.env.IGNORE || process.env.ignore || '')
110154
.split('\n')
111155
.filter((v) => v !== '');
@@ -162,12 +206,34 @@ export const robot = (app: Probot) => {
162206
}
163207
try {
164208
const res = await chat?.codeReview(patch);
165-
if (!res.lgtm && !!res.review_comment) {
166-
ress.push({
167-
path: file.filename,
168-
body: res.review_comment,
169-
position: patch.split('\n').length - 1,
170-
})
209+
// res can be a single review or an array of reviews (one for each hunk)
210+
const reviews = Array.isArray(res) ? res : [res];
211+
212+
for (const review of reviews) {
213+
if (!review.lgtm && !!review.review_comment) {
214+
let line: number | undefined;
215+
let side: 'LEFT' | 'RIGHT' = 'RIGHT';
216+
217+
// Extract line number from hunk header if available
218+
if (review.hunk_header) {
219+
const c = review.hunk_header.match(/\+\s*(\d+),(\d+)/);
220+
if (c) {
221+
const [, start, count] = c;
222+
line = Number(start) + Number(count) - 1;
223+
}
224+
else {
225+
log.error(`Failed to parse hunk header: ${review.hunk_header}`);
226+
continue;
227+
}
228+
}
229+
230+
ress.push({
231+
path: file.filename,
232+
body: review.review_comment,
233+
line: line,
234+
side: side,
235+
})
236+
}
171237
}
172238
} catch (e) {
173239
log.info(`review ${file.filename} failed`, e);
@@ -181,7 +247,7 @@ export const robot = (app: Probot) => {
181247
pull_number: context.pullRequest().pull_number,
182248
body: ress.length ? "Code review by ChatGPT" : "LGTM 👍",
183249
event: 'COMMENT',
184-
commit_id: commits[commits.length - 1].sha,
250+
commit_id: context.payload.pull_request.head.sha,
185251
comments: ress,
186252
});
187253
} catch (e) {

‎src/chat.ts‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,17 +38,23 @@ export class Chat {
3838

3939
const jsonFormatRequirement = '\nProvide your feedback in a strict JSON format with the following structure:\n' +
4040
'{\n' +
41-
' "lgtm": boolean, // true if the code looks good to merge, false if there are concerns\n' +
42-
' "review_comment": string // Your detailed review comments. You can use markdown syntax in this string, but the overall response must be a valid JSON\n' +
41+
' "reviews": [\n' +
42+
' {\n' +
43+
' "hunk_header": string, // The @@ hunk header (e.g., "@@ -10,5 +10,7 @@"), optional\n' +
44+
' "lgtm": boolean, // true if this hunk looks good, false if there are concerns\n' +
45+
' "review_comment": string // Your detailed review comments for this hunk. Can use markdown syntax. Empty string if lgtm is true.\n' +
46+
' }\n' +
47+
' ]\n' +
4348
'}\n' +
44-
'Ensure your response is a valid JSON object.\n';
49+
'Review each hunk (marked by @@) separately and provide feedback for hunks that need improvement.\n' +
50+
'Ensure your response is a valid JSON object with a reviews array.\n';
4551

4652
return `${userPrompt}${jsonFormatRequirement} ${answerLanguage}:
4753
${patch}
4854
`;
4955
};
5056

51-
public codeReview = async (patch: string): Promise<{ lgtm: boolean, review_comment: string }> => {
57+
public codeReview = async (patch: string): Promise<Array<{ lgtm: boolean, review_comment: string, hunk_header?: string }> | { lgtm: boolean, review_comment: string, hunk_header?: string }> => {
5258
if (!patch) {
5359
return {
5460
lgtm: true,
@@ -80,10 +86,16 @@ export class Chat {
8086
if (res.choices.length) {
8187
try {
8288
const json = JSON.parse(res.choices[0].message.content || "");
83-
return json
89+
// If response has a 'reviews' array, return it directly
90+
if (json.reviews && Array.isArray(json.reviews)) {
91+
return json.reviews;
92+
}
93+
// Otherwise, treat as a single review response
94+
return json;
8495
} catch (e) {
8596
return {
8697
lgtm: false,
98+
hunk_header: patch.split('\n')[0].startsWith('@@') ? patch.split('\n')[0] : undefined,
8799
review_comment: res.choices[0].message.content || ""
88100
}
89101
}

0 commit comments

Comments
 (0)