Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(amazonq): grouping options for code issues #6330

Merged
merged 8 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading