Skip to content

Commit b9af56c

Browse files
ctlai95ctlai95
authored
fix(amazonq): auto-review removes existing issues (#6535)
## Problem Auto-reviews often produce less code issues than manual reviews, but the current behavior is to remove all the issues in the file when processing the new ones. This means that issues discovered by manual reviews but not auto-reviews will silently disappear if auto-reviews is enabled. ## Solution - Auto-reviews should not clear the previous issues, but instead merge in the new results to the existing group. - Fixed a related issue with the `ignoreIssue` command being flaky --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 115df6d commit b9af56c

File tree

6 files changed

+76
-27
lines changed

6 files changed

+76
-27
lines changed

‎packages/amazonq/.changes/next-release/Bug Fix-c8a60497-e42c-4141-bcfe-2c44188bc98b.json‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"type": "Bug Fix",
3+
"description": "/review: Auto-review should not remove issues from manual reviews"
4+
}

‎packages/core/src/codewhisperer/activation.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -614,7 +614,6 @@ function toggleIssuesVisibility(visibleCondition: (issue: CodeScanIssue, filePat
614614
...group,
615615
issues: group.issues.map((issue) => ({ ...issue, visible: visibleCondition(issue, group.filePath) })),
616616
}))
617-
securityScanRender.securityDiagnosticCollection?.clear()
618617
for (const issue of updatedIssues) {
619618
updateSecurityDiagnosticCollection(issue)
620619
}

‎packages/core/src/codewhisperer/service/diagnosticsProvider.ts‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import * as vscode from 'vscode'
7-
import { CodeScanIssue, AggregatedCodeScanIssue, CodeScansState } from '../models/model'
7+
import { CodeScanIssue, AggregatedCodeScanIssue } from '../models/model'
88
import { CodeAnalysisScope, codewhispererDiagnosticSourceLabel } from '../models/constants'
99
import { SecurityIssueTreeViewProvider } from './securityIssueTreeViewProvider'
1010
import { SecurityIssueProvider } from './securityIssueProvider'
@@ -30,24 +30,30 @@ export function initSecurityScanRender(
3030
scope: CodeAnalysisScope
3131
) {
3232
securityScanRender.initialized = false
33-
if ((scope === CodeAnalysisScope.FILE_AUTO || scope === CodeAnalysisScope.FILE_ON_DEMAND) && editor) {
33+
if (scope === CodeAnalysisScope.FILE_ON_DEMAND && editor) {
3434
securityScanRender.securityDiagnosticCollection?.delete(editor.document.uri)
3535
} else if (scope === CodeAnalysisScope.PROJECT) {
3636
securityScanRender.securityDiagnosticCollection?.clear()
3737
}
3838
for (const securityRecommendation of securityRecommendationList) {
3939
updateSecurityDiagnosticCollection(securityRecommendation)
40-
updateSecurityIssuesForProviders(securityRecommendation)
40+
updateSecurityIssuesForProviders(securityRecommendation, scope === CodeAnalysisScope.FILE_AUTO)
4141
}
4242
securityScanRender.initialized = true
4343
}
4444

45-
function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue) {
46-
const updatedSecurityRecommendationList = [
47-
...SecurityIssueProvider.instance.issues.filter((group) => group.filePath !== securityRecommendation.filePath),
48-
securityRecommendation,
49-
]
50-
SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList
45+
function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue, isAutoScope?: boolean) {
46+
if (isAutoScope) {
47+
SecurityIssueProvider.instance.mergeIssues(securityRecommendation)
48+
} else {
49+
const updatedSecurityRecommendationList = [
50+
...SecurityIssueProvider.instance.issues.filter(
51+
(group) => group.filePath !== securityRecommendation.filePath
52+
),
53+
securityRecommendation,
54+
]
55+
SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList
56+
}
5157
SecurityIssueTreeViewProvider.instance.refresh()
5258
}
5359

@@ -58,8 +64,22 @@ export function updateSecurityDiagnosticCollection(securityRecommendation: Aggre
5864
const securityDiagnostics: vscode.Diagnostic[] = vscode.languages
5965
.getDiagnostics(uri)
6066
.filter((diagnostic) => diagnostic.source === codewhispererDiagnosticSourceLabel)
61-
for (const securityIssue of securityRecommendation.issues.filter((securityIssue) => securityIssue.visible)) {
62-
securityDiagnostics.push(createSecurityDiagnostic(securityIssue))
67+
for (const securityIssue of securityRecommendation.issues) {
68+
const existingDiagnosticIndex = securityDiagnostics.findIndex(
69+
(diagnostic) =>
70+
(diagnostic.message === securityIssue.title &&
71+
diagnostic.range.start.line === securityIssue.startLine &&
72+
diagnostic.range.end.line === securityIssue.endLine) ||
73+
(diagnostic.message === 'Re-scan to validate the fix: ' + securityIssue.title &&
74+
diagnostic.range.start.line === securityIssue.startLine &&
75+
diagnostic.range.end.line === securityIssue.startLine)
76+
)
77+
if (existingDiagnosticIndex !== -1) {
78+
securityDiagnostics.splice(existingDiagnosticIndex, 1)
79+
}
80+
if (securityIssue.visible) {
81+
securityDiagnostics.push(createSecurityDiagnostic(securityIssue))
82+
}
6383
}
6484
securityDiagnosticCollection.set(uri, securityDiagnostics)
6585
}
@@ -112,8 +132,7 @@ export function disposeSecurityDiagnostic(event: vscode.TextDocumentChangeEvent)
112132
if (
113133
issue.severity === vscode.DiagnosticSeverity.Warning &&
114134
intersection &&
115-
(/\S/.test(changedText) || changedText === '') &&
116-
!CodeScansState.instance.isScansEnabled()
135+
(/\S/.test(changedText) || changedText === '')
117136
) {
118137
issue.severity = vscode.DiagnosticSeverity.Information
119138
issue.message = 'Re-scan to validate the fix: ' + issue.message

‎packages/core/src/codewhisperer/service/securityIssueProvider.ts‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import * as vscode from 'vscode'
7-
import { AggregatedCodeScanIssue, CodeScanIssue, CodeScansState, SuggestedFix } from '../models/model'
7+
import { AggregatedCodeScanIssue, CodeScanIssue, SuggestedFix } from '../models/model'
88
export class SecurityIssueProvider {
99
static #instance: SecurityIssueProvider
1010
public static get instance() {
@@ -53,11 +53,7 @@ export class SecurityIssueProvider {
5353
event.document.lineAt(issue.endLine - 1)?.range.end.character ?? 0
5454
)
5555
const intersection = changedRange.intersection(range)
56-
return !(
57-
intersection &&
58-
(/\S/.test(changedText) || changedText === '') &&
59-
!CodeScansState.instance.isScansEnabled()
60-
)
56+
return !(intersection && (/\S/.test(changedText) || changedText === ''))
6157
})
6258
.map((issue) => {
6359
if (issue.startLine < changedRange.end.line) {
@@ -132,4 +128,32 @@ export class SecurityIssueProvider {
132128
}
133129
})
134130
}
131+
132+
public mergeIssues(newIssues: AggregatedCodeScanIssue) {
133+
const existingGroup = this._issues.find((group) => group.filePath === newIssues.filePath)
134+
if (!existingGroup) {
135+
this._issues.push(newIssues)
136+
return
137+
}
138+
139+
this._issues = this._issues.map((group) =>
140+
group.filePath !== newIssues.filePath
141+
? group
142+
: {
143+
...group,
144+
issues: [
145+
...group.issues,
146+
...newIssues.issues.filter((issue) => !this.isExistingIssue(issue, newIssues.filePath)),
147+
],
148+
}
149+
)
150+
}
151+
152+
private isExistingIssue(issue: CodeScanIssue, filePath: string) {
153+
return this._issues
154+
.find((group) => group.filePath === filePath)
155+
?.issues.find(
156+
(i) => i.title === issue.title && i.startLine === issue.startLine && i.endLine === issue.endLine
157+
)
158+
}
135159
}

‎packages/core/src/shared/utilities/commentUtils.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,13 +90,16 @@ export function insertCommentAboveLine(document: vscode.TextDocument, line: numb
9090
}
9191

9292
const edit = new vscode.WorkspaceEdit()
93-
const position = new vscode.Position(line, 0)
93+
const previousLine = Math.max(0, line - 1)
94+
const previousLineLength = previousLine > 0 ? document.lineAt(previousLine).text.length : 0
95+
const position = new vscode.Position(previousLine, previousLineLength)
9496
const indent = ' '.repeat(Math.max(0, document.lineAt(line).firstNonWhitespaceCharacterIndex))
97+
const commentPrefix = line === 0 ? '' : '\n'
9598
const commentText = lineComment
96-
? `${indent}${lineComment} ${comment}\n`
99+
? `${commentPrefix}${indent}${lineComment} ${comment}`
97100
: blockComment?.[0] && blockComment[1]
98-
? `${indent}${blockComment[0]} ${comment} ${blockComment[1]}\n`
99-
: `${indent}${defaultCommentConfig.lineComment} ${comment}\n`
101+
? `${commentPrefix}${indent}${blockComment[0]} ${comment} ${blockComment[1]}`
102+
: `${commentPrefix}${indent}${defaultCommentConfig.lineComment} ${comment}`
100103
edit.insert(document.uri, position, commentText)
101104
void vscode.workspace.applyEdit(edit)
102105
}

‎packages/core/src/test/shared/utilities/commentUtils.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ describe('CommentUtils', function () {
7777

7878
const document = createMockDocument('foo = 1\nbar = 2', 'foo.py', 'python')
7979
insertCommentAboveLine(document, 1, 'some-comment')
80-
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), '# some-comment\n'))
80+
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n# some-comment'))
8181
})
8282

8383
it('should indent the comment by the same amount as the current line', function () {
@@ -86,7 +86,7 @@ describe('CommentUtils', function () {
8686

8787
const document = createMockDocument(' foo = 1\n bar = 2', 'foo.py', 'python')
8888
insertCommentAboveLine(document, 1, 'some-comment')
89-
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), ' # some-comment\n'))
89+
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n # some-comment'))
9090
})
9191

9292
it('should fallback to block comment if line comment is undefined', function () {
@@ -95,7 +95,7 @@ describe('CommentUtils', function () {
9595

9696
const document = createMockDocument('<aaa>\n <bbb></bbb>\n</aaa>', 'foo.xml', 'xml')
9797
insertCommentAboveLine(document, 1, 'some-comment')
98-
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), ' <!-- some-comment -->\n'))
98+
assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n <!-- some-comment -->'))
9999
})
100100
})
101101
})

0 commit comments

Comments
 (0)