-
Notifications
You must be signed in to change notification settings - Fork 751
fix(amazonq): codefix does not do error handling #6307
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
Conversation
| issue: targetIssue, | ||
| isGenerateFixLoading: false, | ||
| isGenerateFixError: true, | ||
| generateFixError: getErrorMsg(err as AWSError, true), |
There was a problem hiding this comment.
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.
| issue: targetIssue, | ||
| isGenerateFixLoading: false, | ||
| isGenerateFixError: true, | ||
| generateFixError: getErrorMsg(err as AWSError, true), |
There was a problem hiding this comment.
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
}There was a problem hiding this comment.
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?: string | ||
| isGenerateFixLoading?: boolean | ||
| isGenerateFixError?: boolean | ||
| generateFixError?: string | null |
There was a problem hiding this comment.
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
| generateFixError?: string | null | |
| generateFixError?: string |
There was a problem hiding this comment.
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
})| public setIsGenerateFixError(isGenerateFixError: boolean) { | ||
| this.isGenerateFixError = isGenerateFixError | ||
| this.onChangeGenerateFixError.fire(isGenerateFixError) | ||
| public setGenerateFixError(generateFixError: string | null | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably drop null
| relativePath: '', | ||
| isGenerateFixLoading: false, | ||
| isGenerateFixError: false, | ||
| generateFixError: undefined as string | null | undefined, |
There was a problem hiding this comment.
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
| generateFixError: undefined as string | null | undefined, | |
| generateFixError: undefined |
| private filePath: string | undefined | ||
| private isGenerateFixLoading: boolean = false | ||
| private isGenerateFixError: boolean = false | ||
| private generateFixError: string | null | undefined = undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try
| private generateFixError: string | null | undefined = undefined | |
| private generateFixError: string | undefined = undefined |
## Problem Code fix errors are not handled correctly, only shows a generic error notification and webview is not updated. ## Solution Surface the server error to the UI <img width="658" alt="image" src="https://github.com/user-attachments/assets/6e0e72d1-7de1-415d-a4d4-891d94e6d7ea" /> --- - 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.
## Problem Code fix errors are not handled correctly, only shows a generic error notification and webview is not updated. ## Solution Surface the server error to the UI <img width="658" alt="image" src="https://github.com/user-attachments/assets/6e0e72d1-7de1-415d-a4d4-891d94e6d7ea" /> --- - 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.
## Problem Code fix errors are not handled correctly, only shows a generic error notification and webview is not updated. ## Solution Surface the server error to the UI <img width="658" alt="image" src="https://github.com/user-attachments/assets/6e0e72d1-7de1-415d-a4d4-891d94e6d7ea" /> --- - 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.
## Problem Code fix errors are not handled correctly, only shows a generic error notification and webview is not updated. ## Solution Surface the server error to the UI <img width="658" alt="image" src="https://github.com/user-attachments/assets/6e0e72d1-7de1-415d-a4d4-891d94e6d7ea" /> --- - 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.
Problem
Code fix errors are not handled correctly, only shows a generic error notification and webview is not updated.
Solution
Surface the server error to the UI
feature/xbranches will not be squash-merged at release time.