Skip to content

Commit

Permalink
feat(amazonq): grouping options for code issues #6330
Browse files Browse the repository at this point in the history
## Problem
Code issues are grouped by severity and while this is useful in some
cases, it can be hard to navigate.

## Solution
Let the user decide how to group the issues.
Grouping strategies to start off with:
- Severity (existing behavior)
- File Location (issues in the same file are grouped together)
  • Loading branch information
ctlai95 authored Jan 17, 2025
1 parent 4b47b04 commit 062d24a
Show file tree
Hide file tree
Showing 15 changed files with 388 additions and 15 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Feature",
"description": "/review: Code issues can be grouped by file location or severity"
}
13 changes: 12 additions & 1 deletion packages/amazonq/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,15 @@
"when": "view == aws.AmazonQChatView || view == aws.amazonq.AmazonCommonAuth",
"group": "y_toolkitMeta@2"
},
{
"command": "aws.amazonq.codescan.showGroupingStrategy",
"when": "view == aws.amazonq.SecurityIssuesTree",
"group": "navigation@1"
},
{
"command": "aws.amazonq.security.showFilters",
"when": "view == aws.amazonq.SecurityIssuesTree",
"group": "navigation"
"group": "navigation@2"
}
],
"view/item/context": [
Expand Down Expand Up @@ -724,6 +729,12 @@
{
"command": "aws.amazonq.security.showFilters",
"title": "%AWS.command.amazonq.filterIssues%",
"icon": "$(filter)",
"enablement": "view == aws.amazonq.SecurityIssuesTree"
},
{
"command": "aws.amazonq.codescan.showGroupingStrategy",
"title": "%AWS.command.amazonq.groupIssues%",
"icon": "$(list-filter)",
"enablement": "view == aws.amazonq.SecurityIssuesTree"
},
Expand Down
103 changes: 102 additions & 1 deletion packages/amazonq/test/unit/codewhisperer/models/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@
*/
import assert from 'assert'
import sinon from 'sinon'
import { SecurityIssueFilters, SecurityTreeViewFilterState } from 'aws-core-vscode/codewhisperer'
import {
CodeIssueGroupingStrategy,
CodeIssueGroupingStrategyState,
SecurityIssueFilters,
SecurityTreeViewFilterState,
} from 'aws-core-vscode/codewhisperer'
import { globals } from 'aws-core-vscode/shared'

describe('model', function () {
Expand Down Expand Up @@ -70,4 +75,100 @@ describe('model', function () {
assert.deepStrictEqual(hiddenSeverities, ['High', 'Low'])
})
})

describe('CodeIssueGroupingStrategyState', function () {
let sandbox: sinon.SinonSandbox
let state: CodeIssueGroupingStrategyState

beforeEach(function () {
sandbox = sinon.createSandbox()
state = CodeIssueGroupingStrategyState.instance
})

afterEach(function () {
sandbox.restore()
})

describe('instance', function () {
it('should return the same instance when called multiple times', function () {
const instance1 = CodeIssueGroupingStrategyState.instance
const instance2 = CodeIssueGroupingStrategyState.instance
assert.strictEqual(instance1, instance2)
})
})

describe('getState', function () {
it('should return fallback when no state is stored', function () {
const result = state.getState()

assert.equal(result, CodeIssueGroupingStrategy.Severity)
})

it('should return stored state when valid', async function () {
const validStrategy = CodeIssueGroupingStrategy.FileLocation
await state.setState(validStrategy)

const result = state.getState()

assert.equal(result, validStrategy)
})

it('should return fallback when stored state is invalid', async function () {
const invalidStrategy = 'invalid'
await state.setState(invalidStrategy)

const result = state.getState()

assert.equal(result, CodeIssueGroupingStrategy.Severity)
})
})

describe('setState', function () {
it('should update state and fire change event for valid strategy', async function () {
const validStrategy = CodeIssueGroupingStrategy.FileLocation

// Create a spy to watch for event emissions
const eventSpy = sandbox.spy()
state.onDidChangeState(eventSpy)

await state.setState(validStrategy)

sinon.assert.calledWith(eventSpy, validStrategy)
})

it('should use fallback and fire change event for invalid strategy', async function () {
const invalidStrategy = 'invalid'

// Create a spy to watch for event emissions
const eventSpy = sandbox.spy()
state.onDidChangeState(eventSpy)

await state.setState(invalidStrategy)

sinon.assert.calledWith(eventSpy, CodeIssueGroupingStrategy.Severity)
})
})

describe('reset', function () {
it('should set state to fallback value', async function () {
const setStateStub = sandbox.stub(state, 'setState').resolves()

await state.reset()

sinon.assert.calledWith(setStateStub, CodeIssueGroupingStrategy.Severity)
})
})

describe('onDidChangeState', function () {
it('should allow subscribing to state changes', async function () {
const listener = sandbox.spy()
const disposable = state.onDidChangeState(listener)

await state.setState(CodeIssueGroupingStrategy.Severity)

sinon.assert.calledWith(listener, CodeIssueGroupingStrategy.Severity)
disposable.dispose()
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,24 @@ import {
SecurityTreeViewFilterState,
SecurityIssueProvider,
SeverityItem,
CodeIssueGroupingStrategyState,
CodeIssueGroupingStrategy,
} from 'aws-core-vscode/codewhisperer'
import { createCodeScanIssue } from 'aws-core-vscode/test'
import assert from 'assert'
import sinon from 'sinon'
import path from 'path'

describe('SecurityIssueTreeViewProvider', function () {
let securityIssueProvider: SecurityIssueProvider
let securityIssueTreeViewProvider: SecurityIssueTreeViewProvider

beforeEach(function () {
securityIssueProvider = SecurityIssueProvider.instance
SecurityIssueProvider.instance.issues = [
{ filePath: 'file/path/a', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/b', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/c', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/d', issues: [createCodeScanIssue(), createCodeScanIssue()] },
]
securityIssueTreeViewProvider = new SecurityIssueTreeViewProvider()
})

Expand All @@ -44,13 +51,6 @@ describe('SecurityIssueTreeViewProvider', function () {

describe('getChildren', function () {
it('should return sorted list of severities if element is undefined', function () {
securityIssueProvider.issues = [
{ filePath: 'file/path/c', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/d', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/a', issues: [createCodeScanIssue(), createCodeScanIssue()] },
{ filePath: 'file/path/b', issues: [createCodeScanIssue(), createCodeScanIssue()] },
]

const element = undefined
const result = securityIssueTreeViewProvider.getChildren(element) as SeverityItem[]
assert.strictEqual(result.length, 5)
Expand Down Expand Up @@ -102,5 +102,55 @@ describe('SecurityIssueTreeViewProvider', function () {
const result = securityIssueTreeViewProvider.getChildren(element) as IssueItem[]
assert.strictEqual(result.length, 0)
})

it('should return severity-grouped items when grouping strategy is Severity', function () {
sinon.stub(CodeIssueGroupingStrategyState.instance, 'getState').returns(CodeIssueGroupingStrategy.Severity)

const severityItems = securityIssueTreeViewProvider.getChildren() as SeverityItem[]
for (const [index, [severity, expectedIssueCount]] of [
['Critical', 0],
['High', 8],
['Medium', 0],
['Low', 0],
['Info', 0],
].entries()) {
const currentSeverityItem = severityItems[index]
assert.strictEqual(currentSeverityItem.label, severity)
assert.strictEqual(currentSeverityItem.issues.length, expectedIssueCount)

const issueItems = securityIssueTreeViewProvider.getChildren(currentSeverityItem) as IssueItem[]
assert.ok(issueItems.every((item) => item.iconPath === undefined))
assert.ok(
issueItems.every((item) => item.description?.toString().startsWith(path.basename(item.filePath)))
)
}
})

it('should return file-grouped items when grouping strategy is FileLocation', function () {
sinon
.stub(CodeIssueGroupingStrategyState.instance, 'getState')
.returns(CodeIssueGroupingStrategy.FileLocation)

const result = securityIssueTreeViewProvider.getChildren() as FileItem[]
for (const [index, [fileName, expectedIssueCount]] of [
['a', 2],
['b', 2],
['c', 2],
['d', 2],
].entries()) {
const currentFileItem = result[index]
assert.strictEqual(currentFileItem.label, fileName)
assert.strictEqual(currentFileItem.issues.length, expectedIssueCount)
assert.strictEqual(currentFileItem.description, 'file/path')

const issueItems = securityIssueTreeViewProvider.getChildren(currentFileItem) as IssueItem[]
assert.ok(
issueItems.every((item) =>
item.iconPath?.toString().includes(`${item.issue.severity.toLowerCase()}.svg`)
)
)
assert.ok(issueItems.every((item) => item.description?.toString().startsWith('[Ln ')))
}
})
})
})
45 changes: 45 additions & 0 deletions packages/amazonq/test/unit/codewhisperer/ui/prompters.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import { createQuickPickPrompterTester, QuickPickPrompterTester } from 'aws-core-vscode/test'
import {
CodeIssueGroupingStrategy,
CodeIssueGroupingStrategyState,
createCodeIssueGroupingStrategyPrompter,
} from 'aws-core-vscode/codewhisperer'
import sinon from 'sinon'
import assert from 'assert'
import vscode from 'vscode'

const severity = { data: CodeIssueGroupingStrategy.Severity, label: 'Severity' }
const fileLocation = { data: CodeIssueGroupingStrategy.FileLocation, label: 'File Location' }

describe('createCodeIssueGroupingStrategyPrompter', function () {
let tester: QuickPickPrompterTester<CodeIssueGroupingStrategy>

beforeEach(function () {
tester = createQuickPickPrompterTester(createCodeIssueGroupingStrategyPrompter())
})

afterEach(function () {
sinon.restore()
})

it('should list grouping strategies', async function () {
tester.assertItems([severity, fileLocation])
tester.hide()
await tester.result()
})

it('should update state on selection', async function () {
const originalState = CodeIssueGroupingStrategyState.instance.getState()
assert.equal(originalState, CodeIssueGroupingStrategy.Severity)

tester.selectItems(fileLocation)
tester.addCallback(() => vscode.commands.executeCommand('workbench.action.acceptSelectedQuickOpenItem'))

await tester.result()
assert.equal(CodeIssueGroupingStrategyState.instance.getState(), fileLocation.data)
})
})
5 changes: 5 additions & 0 deletions packages/core/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@
"AWS.command.amazonq.acceptFix": "Accept Fix",
"AWS.command.amazonq.regenerateFix": "Regenerate Fix",
"AWS.command.amazonq.filterIssues": "Filter Issues",
"AWS.command.amazonq.groupIssues": "Group Issues",
"AWS.command.deploySamApplication": "Deploy SAM Application",
"AWS.command.aboutToolkit": "About",
"AWS.command.downloadLambda": "Download...",
Expand Down Expand Up @@ -309,6 +310,10 @@
"AWS.amazonq.scans.projectScanInProgress": "Workspace review is in progress...",
"AWS.amazonq.scans.fileScanInProgress": "File review is in progress...",
"AWS.amazonq.scans.noGitRepo": "Your workspace is not in a git repository. I'll review your project files for security issues, and your in-flight changes for code quality issues.",
"AWS.amazonq.scans.severity": "Severity",
"AWS.amazonq.scans.fileLocation": "File Location",
"AWS.amazonq.scans.groupIssues": "Group Issues",
"AWS.amazonq.scans.groupIssues.placeholder": "Select how to group code issues",
"AWS.amazonq.featureDev.error.conversationIdNotFoundError": "Conversation id must exist before starting code generation",
"AWS.amazonq.featureDev.error.contentLengthError": "The folder you selected is too large for me to use as context. Please choose a smaller folder to work on. For more information on quotas, see the <a href=\"https://docs.aws.amazon.com/amazonq/latest/qdeveloper-ug/software-dev.html#quotas\" target=\"_blank\">Amazon Q Developer documentation.</a>",
"AWS.amazonq.featureDev.error.illegalStateTransition": "Illegal transition between states, restart the conversation",
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/codewhisperer/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
SecurityTreeViewFilterState,
AggregatedCodeScanIssue,
CodeScanIssue,
CodeIssueGroupingStrategyState,
} from './models/model'
import { invokeRecommendation } from './commands/invokeRecommendation'
import { acceptSuggestion } from './commands/onInlineAcceptance'
Expand Down Expand Up @@ -60,6 +61,7 @@ import {
ignoreAllIssues,
focusIssue,
showExploreAgentsView,
showCodeIssueGroupingQuickPick,
} from './commands/basicCommands'
import { sleep } from '../shared/utilities/timeoutUtils'
import { ReferenceLogViewProvider } from './service/referenceLogViewProvider'
Expand Down Expand Up @@ -289,6 +291,8 @@ export async function activate(context: ExtContext): Promise<void> {
listCodeWhispererCommands.register(),
// quick pick with security issues tree filters
showSecurityIssueFilters.register(),
// quick pick code issue grouping strategy
showCodeIssueGroupingQuickPick.register(),
// reset security issue filters
clearFilters.register(),
// handle security issues tree item clicked
Expand All @@ -297,6 +301,10 @@ export async function activate(context: ExtContext): Promise<void> {
SecurityTreeViewFilterState.instance.onDidChangeState((e) => {
SecurityIssueTreeViewProvider.instance.refresh()
}),
// refresh treeview when grouping strategy changes
CodeIssueGroupingStrategyState.instance.onDidChangeState((e) => {
SecurityIssueTreeViewProvider.instance.refresh()
}),
// show a no match state
SecurityIssueTreeViewProvider.instance.onDidChangeTreeData((e) => {
const noMatches =
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/codewhisperer/client/codewhisperer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ export interface CodeWhispererConfig {
}

export const defaultServiceConfig: CodeWhispererConfig = {
region: 'us-east-1',
endpoint: 'https://codewhisperer.us-east-1.amazonaws.com/',
region: 'us-west-2',
endpoint: 'https://rts.alpha-us-west-2.codewhisperer.ai.aws.dev/',
}

export function getCodewhispererConfig(): CodeWhispererConfig {
Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/codewhisperer/commands/basicCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import { DefaultAmazonQAppInitContext } from '../../amazonq/apps/initContext'
import path from 'path'
import { UserWrittenCodeTracker } from '../tracker/userWrittenCodeTracker'
import { parsePatch } from 'diff'
import { createCodeIssueGroupingStrategyPrompter } from '../ui/prompters'

const MessageTimeOut = 5_000

Expand Down Expand Up @@ -887,6 +888,14 @@ export const showSecurityIssueFilters = Commands.declare({ id: 'aws.amazonq.secu
}
})

export const showCodeIssueGroupingQuickPick = Commands.declare(
{ id: 'aws.amazonq.codescan.showGroupingStrategy' },
() => async () => {
const prompter = createCodeIssueGroupingStrategyPrompter()
await prompter.prompt()
}
)

export const focusIssue = Commands.declare(
{ id: 'aws.amazonq.security.focusIssue' },
() => async (issue: CodeScanIssue, filePath: string) => {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/codewhisperer/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,5 @@ export * as CodeWhispererConstants from '../codewhisperer/models/constants'
export { getSelectedCustomization, setSelectedCustomization, baseCustomization } from './util/customizationUtil'
export { Container } from './service/serviceContainer'
export * from './util/gitUtil'
export * from './ui/prompters'
export { UserWrittenCodeTracker } from './tracker/userWrittenCodeTracker'
Loading

0 comments on commit 062d24a

Please sign in to comment.