feat: show request execution error status code, close #1049#1082
feat: show request execution error status code, close #1049#1082looplj merged 1 commit intorelease/v0.9.xfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the visibility and handling of request execution errors by introducing the capability to store and display HTTP status codes. The changes span the entire stack, from updating the database schema and GraphQL API to modifying frontend components and backend business logic. This ensures that users receive clearer feedback on why a request failed, while also improving the robustness of internal error management and channel health monitoring. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding the responseStatusCode to failed request executions, which will greatly aid in debugging. The changes are well-implemented across the stack, from the database schema and backend services to the GraphQL API and frontend components. The refactoring of ErrorStatusCode to ResponseStatusCode for consistency is also a welcome improvement. I've included a couple of suggestions to further enhance the UI presentation of errors and to improve the robustness of error status handling in the backend.
| {(execution.errorMessage || (execution.status === 'failed' && execution.responseStatusCode)) && ( | ||
| <div className='bg-destructive/5 border-destructive/20 space-y-3 rounded-lg border p-4'> | ||
| <span className='text-destructive flex items-center gap-2 text-sm font-semibold'> | ||
| <FileText className='h-4 w-4' /> | ||
| {t('common.messages.errorMessage')} | ||
| </span> | ||
| <p className='text-destructive bg-destructive/10 rounded border p-3 text-sm'>{execution.errorMessage}</p> | ||
| <div className='flex items-center justify-between'> | ||
| <span className='text-destructive flex items-center gap-2 text-sm font-semibold'> | ||
| <FileText className='h-4 w-4' /> | ||
| {t('common.messages.errorMessage')} | ||
| </span> | ||
| {execution.status === 'failed' && execution.responseStatusCode && ( | ||
| <Badge variant='destructive'>HTTP {execution.responseStatusCode}</Badge> | ||
| )} | ||
| </div> | ||
| {execution.errorMessage && ( | ||
| <p className='text-destructive bg-destructive/10 rounded border p-3 text-sm'>{execution.errorMessage}</p> | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
In the case where execution.errorMessage is empty but execution.responseStatusCode is present, the UI will display the "Error Message" title without any message body, which could be confusing. It would be better to handle this case explicitly, for example by showing only the status code badge or a more generic message.
{execution.errorMessage ? (
<div className='bg-destructive/5 border-destructive/20 space-y-3 rounded-lg border p-4'>
<div className='flex items-center justify-between'>
<span className='text-destructive flex items-center gap-2 text-sm font-semibold'>
<FileText className='h-4 w-4' />
{t('common.messages.errorMessage')}
</span>
{execution.status === 'failed' && execution.responseStatusCode && (
<Badge variant='destructive'>HTTP {execution.responseStatusCode}</Badge>
)}
</div>
<p className='text-destructive bg-destructive/10 rounded border p-3 text-sm'>{execution.errorMessage}</p>
</div>
) : (
execution.status === 'failed' &&
execution.responseStatusCode && (
<div className='bg-destructive/5 border-destructive/20 rounded-lg p-4'>
<Badge variant='destructive'>HTTP {execution.responseStatusCode}</Badge>
</div>
)
)}
| } | ||
|
|
||
| return s.UpdateRequestExecutionStatus(ctx, executionID, status, rawErr.Error()) | ||
| return s.UpdateRequestExecutionStatus(ctx, executionID, status, rawErr.Error(), nil) |
There was a problem hiding this comment.
This call passes nil for errorInfo, which means if rawErr is an *httpclient.Error, the status code information will be lost. Consider extracting the error info from rawErr here and passing it to UpdateRequestExecutionStatus. This would likely involve moving the new ExtractErrorInfo function from the orchestrator package to the biz package to make it reusable and avoid a circular dependency.
| // ExtractErrorInfo extracts HTTP status code and sanitized response body from error. | ||
| func ExtractErrorInfo(err error) *biz.ExecutionErrorInfo { | ||
| httpErr, ok := xerrors.As[*httpclient.Error](err) | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| return &biz.ExecutionErrorInfo{ | ||
| StatusCode: &httpErr.StatusCode, | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve code organization and reusability, consider moving ExtractErrorInfo to the biz package. This would allow other parts of the business logic, such as UpdateRequestExecutionStatusFromError in internal/server/biz/request.go, to use this utility function without creating circular dependencies.
1 participant
No description provided.