-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Obs AI Assistant] Show error toast on failure #179335
[Obs AI Assistant] Show error toast on failure #179335
Conversation
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
@@ -41,6 +42,19 @@ export function NavControl({}: {}) { | |||
[service, hasBeenOpened] | |||
); | |||
|
|||
useEffect(() => { |
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.
is L39 not a better place to handle it?
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.
No, because useAbortableAsync
doesn't bubble the error up from the Promise creation function, it sets it as part of the state of the hook and returns it (same as for .value
).
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.
you can add a catch handler before the value is returned though, e.g. service.start({ signal }).catch(notifyError)
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.
Fixed in 60dea26
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 would have to look at the code to see what you're doing. In your latest change, you'll want to rethrow the error after creating the notification, otherwise it looks like it actually succeeded
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.
return hasBeenOpened | ||
? service.start({ signal }).catch((error) => { | ||
notifications.toasts.addError(error, { | ||
title: 'Failed to initialize Observability AI Assistant', |
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.
i18n?
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.
💚 Build Succeeded
Metrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
If the call to
ObservabilityAIAssistantAppService.start
fails, show an error toast to notify the user.