Skip to content

Commit 7b0a8c0

Browse files
lfsevergninilfsevergnini
committed
fix: post code review without line comments if there are hunk line errors [ENG-1597]
1 parent 8ec96f9 commit 7b0a8c0

File tree

3 files changed

+45
-130
lines changed

3 files changed

+45
-130
lines changed

‎dist/index.js‎

Lines changed: 22 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -947,69 +947,29 @@ class GitHubService {
947947
const comments = allComments.filter(comment => comment !== null);
948948
core.info(`Submitting review with ${comments.length} comments`);
949949
core.debug(`Review comments: ${JSON.stringify(comments, null, 2)}`);
950-
await this.octokit.pulls.createReview({
951-
owner: this.owner,
952-
repo: this.repo,
953-
pull_number: prNumber,
954-
body: summary,
955-
comments,
956-
event: suggestedAction.toUpperCase()
957-
});
958-
}
959-
/**
960-
* This is a hack to get the position of a line in the diff.
961-
* It's not perfect, but it's better than nothing.
962-
* It's based on the patch file, which is not always available.
963-
* It's also not always accurate, but it's better than nothing.
964-
*
965-
* @deprecated
966-
*/
967-
async getDiffPosition(prNumber, filePath, line) {
968-
const { data: files } = await this.octokit.pulls.listFiles({
969-
owner: this.owner,
970-
repo: this.repo,
971-
pull_number: prNumber
972-
});
973-
const file = files.find(f => f.filename === filePath);
974-
if (!file) {
975-
throw new Error(`File ${filePath} not found in PR diff`);
976-
}
977-
const patch = file.patch || '';
978-
let position = 0;
979-
let oldLine = 0;
980-
let newLine = 0;
981-
for (const patchLine of patch.split('\n')) {
982-
if (patchLine.startsWith('@@')) {
983-
const match = patchLine.match(/@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
984-
if (match) {
985-
oldLine = parseInt(match[1], 10) - 1;
986-
newLine = parseInt(match[2], 10) - 1;
987-
}
988-
continue;
989-
}
990-
position++;
991-
if (patchLine.startsWith('-')) {
992-
oldLine++;
993-
if (oldLine === line) {
994-
return position;
995-
}
996-
}
997-
else if (patchLine.startsWith('+')) {
998-
newLine++;
999-
if (newLine === line) {
1000-
return position;
1001-
}
1002-
}
1003-
else {
1004-
oldLine++;
1005-
newLine++;
1006-
if (newLine === line || oldLine === line) {
1007-
return position;
1008-
}
1009-
}
950+
try {
951+
await this.octokit.pulls.createReview({
952+
owner: this.owner,
953+
repo: this.repo,
954+
pull_number: prNumber,
955+
body: summary,
956+
comments,
957+
event: suggestedAction.toUpperCase()
958+
});
959+
}
960+
catch (error) {
961+
core.warning(`Failed to submit review with comments: ${error}`);
962+
core.info('Retrying without line comments...');
963+
// Retry without comments
964+
await this.octokit.pulls.createReview({
965+
owner: this.owner,
966+
repo: this.repo,
967+
pull_number: prNumber,
968+
body: `${summary}\n\n> Note: Some line comments were omitted due to technical limitations.`,
969+
comments: [],
970+
event: suggestedAction.toUpperCase()
971+
});
1010972
}
1011-
core.error(`Failed to find line ${line} in diff. Last old line: ${oldLine}, last new line: ${newLine}`);
1012-
throw new Error(`Line ${line} not found in diff for ${filePath}`);
1013973
}
1014974
async getLastReviewedCommit(prNumber) {
1015975
const { data: reviews } = await this.octokit.pulls.listReviews({

‎dist/index.js.map‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/services/GitHubService.ts‎

Lines changed: 22 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -82,74 +82,29 @@ export class GitHubService {
8282
core.info(`Submitting review with ${comments.length} comments`);
8383
core.debug(`Review comments: ${JSON.stringify(comments, null, 2)}`);
8484

85-
await this.octokit.pulls.createReview({
86-
owner: this.owner,
87-
repo: this.repo,
88-
pull_number: prNumber,
89-
body: summary,
90-
comments,
91-
event: suggestedAction.toUpperCase() as 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'
92-
});
93-
}
94-
95-
/**
96-
* This is a hack to get the position of a line in the diff.
97-
* It's not perfect, but it's better than nothing.
98-
* It's based on the patch file, which is not always available.
99-
* It's also not always accurate, but it's better than nothing.
100-
*
101-
* @deprecated
102-
*/
103-
private async getDiffPosition(prNumber: number, filePath: string, line: number): Promise<number> {
104-
const { data: files } = await this.octokit.pulls.listFiles({
105-
owner: this.owner,
106-
repo: this.repo,
107-
pull_number: prNumber
108-
});
109-
110-
const file = files.find(f => f.filename === filePath);
111-
if (!file) {
112-
throw new Error(`File ${filePath} not found in PR diff`);
113-
}
114-
115-
const patch = file.patch || '';
116-
let position = 0;
117-
let oldLine = 0;
118-
let newLine = 0;
119-
120-
for (const patchLine of patch.split('\n')) {
121-
if (patchLine.startsWith('@@')) {
122-
const match = patchLine.match(/@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
123-
if (match) {
124-
oldLine = parseInt(match[1], 10) - 1;
125-
newLine = parseInt(match[2], 10) - 1;
126-
}
127-
continue;
128-
}
129-
130-
position++;
131-
132-
if (patchLine.startsWith('-')) {
133-
oldLine++;
134-
if (oldLine === line) {
135-
return position;
136-
}
137-
} else if (patchLine.startsWith('+')) {
138-
newLine++;
139-
if (newLine === line) {
140-
return position;
141-
}
142-
} else {
143-
oldLine++;
144-
newLine++;
145-
if (newLine === line || oldLine === line) {
146-
return position;
147-
}
148-
}
85+
try {
86+
await this.octokit.pulls.createReview({
87+
owner: this.owner,
88+
repo: this.repo,
89+
pull_number: prNumber,
90+
body: summary,
91+
comments,
92+
event: suggestedAction.toUpperCase() as 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'
93+
});
94+
} catch (error) {
95+
core.warning(`Failed to submit review with comments: ${error}`);
96+
core.info('Retrying without line comments...');
97+
98+
// Retry without comments
99+
await this.octokit.pulls.createReview({
100+
owner: this.owner,
101+
repo: this.repo,
102+
pull_number: prNumber,
103+
body: `${summary}\n\n> Note: Some line comments were omitted due to technical limitations.`,
104+
comments: [],
105+
event: suggestedAction.toUpperCase() as 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'
106+
});
149107
}
150-
151-
core.error(`Failed to find line ${line} in diff. Last old line: ${oldLine}, last new line: ${newLine}`);
152-
throw new Error(`Line ${line} not found in diff for ${filePath}`);
153108
}
154109

155110
async getLastReviewedCommit(prNumber: number): Promise<string | null> {

0 commit comments

Comments
 (0)