Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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": "Bug Fix",
"description": "/review: Improved error handling for code fix operations"
}
7 changes: 7 additions & 0 deletions packages/core/resources/css/securityIssue.css
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,13 @@ pre.center {

pre.error {
color: var(--vscode-diffEditorOverview-removedForeground);
background-color: var(--vscode-diffEditor-removedTextBackground);
white-space: initial;
}

a.cursor {
cursor: pointer;
text-decoration: none;
}

.dot-typing {
Expand Down
25 changes: 14 additions & 11 deletions packages/core/src/codewhisperer/commands/basicCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import { once } from '../../shared/utilities/functionUtils'
import { focusAmazonQPanel } from '../../codewhispererChat/commands/registerCommands'
import { removeDiagnostic } from '../service/diagnosticsProvider'
import { SsoAccessTokenProvider } from '../../auth/sso/ssoAccessTokenProvider'
import { ToolkitError, getTelemetryReason, getTelemetryReasonDesc } from '../../shared/errors'
import { ToolkitError, getErrorMsg, getTelemetryReason, getTelemetryReasonDesc } from '../../shared/errors'
import { isRemoteWorkspace } from '../../shared/vscode/env'
import { isBuilderIdConnection } from '../../auth/connection'
import globals from '../../shared/extensionGlobals'
Expand All @@ -67,6 +67,7 @@ import { startCodeFixGeneration } from './startCodeFixGeneration'
import { DefaultAmazonQAppInitContext } from '../../amazonq/apps/initContext'
import path from 'path'
import { parsePatch } from 'diff'
import type { AWSError } from 'aws-sdk'

const MessageTimeOut = 5_000

Expand Down Expand Up @@ -677,7 +678,8 @@ export const generateFix = Commands.declare(
})
await updateSecurityIssueWebview({
isGenerateFixLoading: true,
isGenerateFixError: false,
// eslint-disable-next-line unicorn/no-null
generateFixError: null,
context: context.extensionContext,
filePath: targetFilePath,
shouldRefreshView: false,
Expand Down Expand Up @@ -737,22 +739,23 @@ export const generateFix = Commands.declare(
await updateSecurityIssueWebview({
issue: targetIssue,
isGenerateFixLoading: false,
isGenerateFixError: true,
generateFixError: getErrorMsg(err as AWSError, true),
Copy link
Contributor

@ashishrp-aws ashishrp-aws Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctlai95 can we use user friendly error messages instead passing the service error straight to user? Like how we handle code review errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look to add in the following before to avoid needing to cast:

if (!(err instanceof Error)) {
  throw new Error('Unexpected non error', err) // modify this as needed
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using isAwsError may be better:

export function isAwsError(error: unknown): error is AWSError & { error_description?: string } {

filePath: targetFilePath,
context: context.extensionContext,
shouldRefreshView: true,
shouldRefreshView: false,
})
SecurityIssueProvider.instance.updateIssue(targetIssue, targetFilePath)
SecurityIssueTreeViewProvider.instance.refresh()
throw err
} finally {
telemetry.record({
component: targetSource,
detectorId: targetIssue.detectorId,
findingId: targetIssue.findingId,
ruleId: targetIssue.ruleId,
variant: refresh ? 'refresh' : undefined,
})
}
telemetry.record({
component: targetSource,
detectorId: targetIssue.detectorId,
findingId: targetIssue.findingId,
ruleId: targetIssue.ruleId,
variant: refresh ? 'refresh' : undefined,
})
})
}
)
Expand Down
15 changes: 5 additions & 10 deletions packages/core/src/codewhisperer/service/codeFixHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@ import * as CodeWhispererConstants from '../models/constants'
import { codeFixState } from '../models/model'
import { getLogger, sleep } from '../../shared'
import { ArtifactMap, CreateUploadUrlRequest, DefaultCodeWhispererClient } from '../client/codewhisperer'
import {
CodeFixJobStoppedError,
CodeFixJobTimedOutError,
CreateCodeFixError,
CreateUploadUrlError,
} from '../models/errors'
import { CodeFixJobStoppedError, CodeFixJobTimedOutError } from '../models/errors'
import { uploadArtifactToS3 } from './securityScanHandler'

export async function getPresignedUrlAndUpload(
Expand All @@ -28,8 +23,8 @@ export async function getPresignedUrlAndUpload(
}
getLogger().verbose(`Prepare for uploading src context...`)
const srcResp = await client.createUploadUrl(srcReq).catch((err) => {
getLogger().error(`Failed getting presigned url for uploading src context. Request id: ${err.requestId}`)
throw new CreateUploadUrlError(err)
getLogger().error('Failed getting presigned url for uploading src context. %O', err)
throw err
})
getLogger().verbose(`CreateUploadUrlRequest requestId: ${srcResp.$response.requestId}`)
getLogger().verbose(`Complete Getting presigned Url for uploading src context.`)
Expand Down Expand Up @@ -60,8 +55,8 @@ export async function createCodeFixJob(
}

const resp = await client.startCodeFixJob(req).catch((err) => {
getLogger().error(`Failed creating code fix job. Request id: ${err.requestId}`)
throw new CreateCodeFixError()
getLogger().error('Failed creating code fix job. %O', err)
throw err
})
getLogger().info(`AmazonQ generate fix Request id: ${resp.$response.requestId}`)
return resp
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ export class SecurityIssueWebview extends VueWebview {
public readonly onChangeIssue = new vscode.EventEmitter<CodeScanIssue | undefined>()
public readonly onChangeFilePath = new vscode.EventEmitter<string | undefined>()
public readonly onChangeGenerateFixLoading = new vscode.EventEmitter<boolean>()
public readonly onChangeGenerateFixError = new vscode.EventEmitter<boolean>()
public readonly onChangeGenerateFixError = new vscode.EventEmitter<string | null | undefined>()

private issue: CodeScanIssue | undefined
private filePath: string | undefined
private isGenerateFixLoading: boolean = false
private isGenerateFixError: boolean = false
private generateFixError: string | null | undefined = undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try

Suggested change
private generateFixError: string | null | undefined = undefined
private generateFixError: string | undefined = undefined


public constructor() {
super(SecurityIssueWebview.sourcePath)
Expand Down Expand Up @@ -99,13 +99,13 @@ export class SecurityIssueWebview extends VueWebview {
this.onChangeGenerateFixLoading.fire(isGenerateFixLoading)
}

public getIsGenerateFixError() {
return this.isGenerateFixError
public getGenerateFixError() {
return this.generateFixError
}

public setIsGenerateFixError(isGenerateFixError: boolean) {
this.isGenerateFixError = isGenerateFixError
this.onChangeGenerateFixError.fire(isGenerateFixError)
public setGenerateFixError(generateFixError: string | null | undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably drop null

this.generateFixError = generateFixError
this.onChangeGenerateFixError.fire(generateFixError)
}

public generateFix() {
Expand Down Expand Up @@ -201,7 +201,7 @@ export async function showSecurityIssueWebview(ctx: vscode.ExtensionContext, iss
activePanel.server.setIssue(issue)
activePanel.server.setFilePath(filePath)
activePanel.server.setIsGenerateFixLoading(false)
activePanel.server.setIsGenerateFixError(false)
activePanel.server.setGenerateFixError(undefined)

const webviewPanel = await activePanel.show({
title: amazonqCodeIssueDetailsTabTitle,
Expand Down Expand Up @@ -247,15 +247,15 @@ type WebviewParams = {
issue?: CodeScanIssue
filePath?: string
isGenerateFixLoading?: boolean
isGenerateFixError?: boolean
generateFixError?: string | null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be good enough

Suggested change
generateFixError?: string | null
generateFixError?: string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think null is required for this since I want to maintain this behavior where the following will remove the error from the webview:

updateSecurityIssueWebview({
  ...
  generateFixError: null
})

while the absence of the field should not remove it

updateSecurityIssueWebview({
  ...
  issue: newIssue
})

shouldRefreshView: boolean
context: vscode.ExtensionContext
}
export async function updateSecurityIssueWebview({
issue,
filePath,
isGenerateFixLoading,
isGenerateFixError,
generateFixError,
shouldRefreshView,
context,
}: WebviewParams): Promise<void> {
Expand All @@ -271,8 +271,8 @@ export async function updateSecurityIssueWebview({
if (isGenerateFixLoading !== undefined) {
activePanel.server.setIsGenerateFixLoading(isGenerateFixLoading)
}
if (isGenerateFixError !== undefined) {
activePanel.server.setIsGenerateFixError(isGenerateFixError)
if (generateFixError !== undefined) {
activePanel.server.setGenerateFixError(generateFixError)
}
if (shouldRefreshView && filePath && issue) {
await showSecurityIssueWebview(context, issue, filePath)
Expand Down
20 changes: 9 additions & 11 deletions packages/core/src/codewhisperer/views/securityIssue/vue/root.vue
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,14 @@
</div>

<div
v-if="isFixAvailable || isGenerateFixLoading || isGenerateFixError || isFixDescriptionAvailable"
v-if="isFixAvailable || isGenerateFixLoading || generateFixError || isFixDescriptionAvailable"
ref="codeFixSection"
>
<hr />

<h3>Suggested code fix preview</h3>
<pre v-if="isGenerateFixLoading" class="center"><div class="dot-typing"></div></pre>
<pre v-if="isGenerateFixError" class="center error">
Something went wrong. <a @click="regenerateFix">Retry</a>
</pre>
<pre v-if="generateFixError" class="center error">{{ generateFixError }}</pre>
<div class="code-block">
<span v-if="isFixAvailable" v-html="suggestedFixHtml"></span>
<div v-if="isFixAvailable" class="code-diff-actions" ref="codeFixAction">
Expand Down Expand Up @@ -195,7 +193,7 @@ export default defineComponent({
endLine: 0,
relativePath: '',
isGenerateFixLoading: false,
isGenerateFixError: false,
generateFixError: undefined as string | null | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be good enough. I think the types upstream may need to be modified

Suggested change
generateFixError: undefined as string | null | undefined,
generateFixError: undefined

languageId: 'plaintext',
fixedCode: '',
referenceText: '',
Expand All @@ -218,8 +216,8 @@ export default defineComponent({
const relativePath = await client.getRelativePath()
this.updateRelativePath(relativePath)
const isGenerateFixLoading = await client.getIsGenerateFixLoading()
const isGenerateFixError = await client.getIsGenerateFixError()
this.updateGenerateFixState(isGenerateFixLoading, isGenerateFixError)
const generateFixError = await client.getGenerateFixError()
this.updateGenerateFixState(isGenerateFixLoading, generateFixError)
const languageId = await client.getLanguageId()
if (languageId) {
this.updateLanguageId(languageId)
Expand Down Expand Up @@ -249,16 +247,16 @@ export default defineComponent({
this.isGenerateFixLoading = isGenerateFixLoading
this.scrollTo('codeFixSection')
})
client.onChangeGenerateFixError((isGenerateFixError) => {
this.isGenerateFixError = isGenerateFixError
client.onChangeGenerateFixError((generateFixError) => {
this.generateFixError = generateFixError
})
},
updateRelativePath(relativePath: string) {
this.relativePath = relativePath
},
updateGenerateFixState(isGenerateFixLoading: boolean, isGenerateFixError: boolean) {
updateGenerateFixState(isGenerateFixLoading: boolean, generateFixError: string | null | undefined) {
this.isGenerateFixLoading = isGenerateFixLoading
this.isGenerateFixError = isGenerateFixError
this.generateFixError = generateFixError
},
updateLanguageId(languageId: string) {
this.languageId = languageId
Expand Down
Loading
Loading