diff --git a/packages/amazonq/.changes/next-release/Bug Fix-c8a60497-e42c-4141-bcfe-2c44188bc98b.json b/packages/amazonq/.changes/next-release/Bug Fix-c8a60497-e42c-4141-bcfe-2c44188bc98b.json new file mode 100644 index 00000000000..0e344d9186e --- /dev/null +++ b/packages/amazonq/.changes/next-release/Bug Fix-c8a60497-e42c-4141-bcfe-2c44188bc98b.json @@ -0,0 +1,4 @@ +{ + "type": "Bug Fix", + "description": "/review: Auto-review should not remove issues from manual reviews" +} diff --git a/packages/core/src/codewhisperer/activation.ts b/packages/core/src/codewhisperer/activation.ts index 30efb21e0b2..d2fd2bbe213 100644 --- a/packages/core/src/codewhisperer/activation.ts +++ b/packages/core/src/codewhisperer/activation.ts @@ -614,7 +614,6 @@ function toggleIssuesVisibility(visibleCondition: (issue: CodeScanIssue, filePat ...group, issues: group.issues.map((issue) => ({ ...issue, visible: visibleCondition(issue, group.filePath) })), })) - securityScanRender.securityDiagnosticCollection?.clear() for (const issue of updatedIssues) { updateSecurityDiagnosticCollection(issue) } diff --git a/packages/core/src/codewhisperer/service/diagnosticsProvider.ts b/packages/core/src/codewhisperer/service/diagnosticsProvider.ts index 65afc885073..72407cd80f5 100644 --- a/packages/core/src/codewhisperer/service/diagnosticsProvider.ts +++ b/packages/core/src/codewhisperer/service/diagnosticsProvider.ts @@ -4,7 +4,7 @@ */ import * as vscode from 'vscode' -import { CodeScanIssue, AggregatedCodeScanIssue, CodeScansState } from '../models/model' +import { CodeScanIssue, AggregatedCodeScanIssue } from '../models/model' import { CodeAnalysisScope, codewhispererDiagnosticSourceLabel } from '../models/constants' import { SecurityIssueTreeViewProvider } from './securityIssueTreeViewProvider' import { SecurityIssueProvider } from './securityIssueProvider' @@ -30,24 +30,30 @@ export function initSecurityScanRender( scope: CodeAnalysisScope ) { securityScanRender.initialized = false - if ((scope === CodeAnalysisScope.FILE_AUTO || scope === CodeAnalysisScope.FILE_ON_DEMAND) && editor) { + if (scope === CodeAnalysisScope.FILE_ON_DEMAND && editor) { securityScanRender.securityDiagnosticCollection?.delete(editor.document.uri) } else if (scope === CodeAnalysisScope.PROJECT) { securityScanRender.securityDiagnosticCollection?.clear() } for (const securityRecommendation of securityRecommendationList) { updateSecurityDiagnosticCollection(securityRecommendation) - updateSecurityIssuesForProviders(securityRecommendation) + updateSecurityIssuesForProviders(securityRecommendation, scope === CodeAnalysisScope.FILE_AUTO) } securityScanRender.initialized = true } -function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue) { - const updatedSecurityRecommendationList = [ - ...SecurityIssueProvider.instance.issues.filter((group) => group.filePath !== securityRecommendation.filePath), - securityRecommendation, - ] - SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList +function updateSecurityIssuesForProviders(securityRecommendation: AggregatedCodeScanIssue, isAutoScope?: boolean) { + if (isAutoScope) { + SecurityIssueProvider.instance.mergeIssues(securityRecommendation) + } else { + const updatedSecurityRecommendationList = [ + ...SecurityIssueProvider.instance.issues.filter( + (group) => group.filePath !== securityRecommendation.filePath + ), + securityRecommendation, + ] + SecurityIssueProvider.instance.issues = updatedSecurityRecommendationList + } SecurityIssueTreeViewProvider.instance.refresh() } @@ -58,8 +64,22 @@ export function updateSecurityDiagnosticCollection(securityRecommendation: Aggre const securityDiagnostics: vscode.Diagnostic[] = vscode.languages .getDiagnostics(uri) .filter((diagnostic) => diagnostic.source === codewhispererDiagnosticSourceLabel) - for (const securityIssue of securityRecommendation.issues.filter((securityIssue) => securityIssue.visible)) { - securityDiagnostics.push(createSecurityDiagnostic(securityIssue)) + for (const securityIssue of securityRecommendation.issues) { + const existingDiagnosticIndex = securityDiagnostics.findIndex( + (diagnostic) => + (diagnostic.message === securityIssue.title && + diagnostic.range.start.line === securityIssue.startLine && + diagnostic.range.end.line === securityIssue.endLine) || + (diagnostic.message === 'Re-scan to validate the fix: ' + securityIssue.title && + diagnostic.range.start.line === securityIssue.startLine && + diagnostic.range.end.line === securityIssue.startLine) + ) + if (existingDiagnosticIndex !== -1) { + securityDiagnostics.splice(existingDiagnosticIndex, 1) + } + if (securityIssue.visible) { + securityDiagnostics.push(createSecurityDiagnostic(securityIssue)) + } } securityDiagnosticCollection.set(uri, securityDiagnostics) } @@ -112,8 +132,7 @@ export function disposeSecurityDiagnostic(event: vscode.TextDocumentChangeEvent) if ( issue.severity === vscode.DiagnosticSeverity.Warning && intersection && - (/\S/.test(changedText) || changedText === '') && - !CodeScansState.instance.isScansEnabled() + (/\S/.test(changedText) || changedText === '') ) { issue.severity = vscode.DiagnosticSeverity.Information issue.message = 'Re-scan to validate the fix: ' + issue.message diff --git a/packages/core/src/codewhisperer/service/securityIssueProvider.ts b/packages/core/src/codewhisperer/service/securityIssueProvider.ts index edd93de8433..4e1662753e4 100644 --- a/packages/core/src/codewhisperer/service/securityIssueProvider.ts +++ b/packages/core/src/codewhisperer/service/securityIssueProvider.ts @@ -4,7 +4,7 @@ */ import * as vscode from 'vscode' -import { AggregatedCodeScanIssue, CodeScanIssue, CodeScansState, SuggestedFix } from '../models/model' +import { AggregatedCodeScanIssue, CodeScanIssue, SuggestedFix } from '../models/model' export class SecurityIssueProvider { static #instance: SecurityIssueProvider public static get instance() { @@ -53,11 +53,7 @@ export class SecurityIssueProvider { event.document.lineAt(issue.endLine - 1)?.range.end.character ?? 0 ) const intersection = changedRange.intersection(range) - return !( - intersection && - (/\S/.test(changedText) || changedText === '') && - !CodeScansState.instance.isScansEnabled() - ) + return !(intersection && (/\S/.test(changedText) || changedText === '')) }) .map((issue) => { if (issue.startLine < changedRange.end.line) { @@ -132,4 +128,32 @@ export class SecurityIssueProvider { } }) } + + public mergeIssues(newIssues: AggregatedCodeScanIssue) { + const existingGroup = this._issues.find((group) => group.filePath === newIssues.filePath) + if (!existingGroup) { + this._issues.push(newIssues) + return + } + + this._issues = this._issues.map((group) => + group.filePath !== newIssues.filePath + ? group + : { + ...group, + issues: [ + ...group.issues, + ...newIssues.issues.filter((issue) => !this.isExistingIssue(issue, newIssues.filePath)), + ], + } + ) + } + + private isExistingIssue(issue: CodeScanIssue, filePath: string) { + return this._issues + .find((group) => group.filePath === filePath) + ?.issues.find( + (i) => i.title === issue.title && i.startLine === issue.startLine && i.endLine === issue.endLine + ) + } } diff --git a/packages/core/src/shared/utilities/commentUtils.ts b/packages/core/src/shared/utilities/commentUtils.ts index 42854ec313c..37d96cbbe6f 100644 --- a/packages/core/src/shared/utilities/commentUtils.ts +++ b/packages/core/src/shared/utilities/commentUtils.ts @@ -90,13 +90,16 @@ export function insertCommentAboveLine(document: vscode.TextDocument, line: numb } const edit = new vscode.WorkspaceEdit() - const position = new vscode.Position(line, 0) + const previousLine = Math.max(0, line - 1) + const previousLineLength = previousLine > 0 ? document.lineAt(previousLine).text.length : 0 + const position = new vscode.Position(previousLine, previousLineLength) const indent = ' '.repeat(Math.max(0, document.lineAt(line).firstNonWhitespaceCharacterIndex)) + const commentPrefix = line === 0 ? '' : '\n' const commentText = lineComment - ? `${indent}${lineComment} ${comment}\n` + ? `${commentPrefix}${indent}${lineComment} ${comment}` : blockComment?.[0] && blockComment[1] - ? `${indent}${blockComment[0]} ${comment} ${blockComment[1]}\n` - : `${indent}${defaultCommentConfig.lineComment} ${comment}\n` + ? `${commentPrefix}${indent}${blockComment[0]} ${comment} ${blockComment[1]}` + : `${commentPrefix}${indent}${defaultCommentConfig.lineComment} ${comment}` edit.insert(document.uri, position, commentText) void vscode.workspace.applyEdit(edit) } diff --git a/packages/core/src/test/shared/utilities/commentUtils.test.ts b/packages/core/src/test/shared/utilities/commentUtils.test.ts index 35932bdc6ea..a05eeb7f771 100644 --- a/packages/core/src/test/shared/utilities/commentUtils.test.ts +++ b/packages/core/src/test/shared/utilities/commentUtils.test.ts @@ -77,7 +77,7 @@ describe('CommentUtils', function () { const document = createMockDocument('foo = 1\nbar = 2', 'foo.py', 'python') insertCommentAboveLine(document, 1, 'some-comment') - assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), '# some-comment\n')) + assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n# some-comment')) }) it('should indent the comment by the same amount as the current line', function () { @@ -86,7 +86,7 @@ describe('CommentUtils', function () { const document = createMockDocument(' foo = 1\n bar = 2', 'foo.py', 'python') insertCommentAboveLine(document, 1, 'some-comment') - assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), ' # some-comment\n')) + assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n # some-comment')) }) it('should fallback to block comment if line comment is undefined', function () { @@ -95,7 +95,7 @@ describe('CommentUtils', function () { const document = createMockDocument('\n \n', 'foo.xml', 'xml') insertCommentAboveLine(document, 1, 'some-comment') - assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(1, 0), ' \n')) + assert.ok(insertMock.calledOnceWith(document.uri, new vscode.Position(0, 0), '\n ')) }) }) })