[Time to Visualize] Embeddable Error Handling Without ReplacePanel#82201
[Time to Visualize] Embeddable Error Handling Without ReplacePanel#82201ThomThomson merged 6 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
@elasticmachine merge upstream |
| }); | ||
| }, | ||
| (error) => { | ||
| this.props.embeddable.destroy(); |
There was a problem hiding this comment.
The approach LGTM,
One weirdness I noticed:
- Load a "broken" dashboard in view move. See Lens embeddable with error
- Go to edit mode and try to delete it. There is no option to "DELETE" an embeddable. O_O.
I think it is important that it still possible to delete an embeddable in this scenario.
Probably we shouldn't call destroy in this scenario, because embeddable still needs to receive update from parent (viewMode in this case) that will be used during context menu rendering?
Probably we could rely on destroy being called when this component unmounts?
There was a problem hiding this comment.
This is a really good catch. I avoided destroying the embeddable, and added clauses in the isCompatible methods of a few problematic actions like Clone, add to library, and unlink from library. Now you should be able to properly remove or replace errorEmbeddables.
| this.props.embeddable.destroy(); | ||
| if (this.embeddableRoot.current) { | ||
| const errorEmbeddable = new ErrorEmbeddable(error, { id: this.props.embeddable.id }); | ||
| errorEmbeddable.render(this.embeddableRoot.current); |
There was a problem hiding this comment.
This embeddable is created but destroy is never called on it. I wonder if this is causing memory leaks or if there is nothing to leak for this simple embeddable.
There was a problem hiding this comment.
I think that the ErrorEmbeddable is simple enough not to cause any memory leaks, but it's still best practice to destroy it, so I added it to the state, and destroyed it during the unmount
…beddables. Added tests. Embeddable panel does not destroy embeddable on error and destroys error embeddable on unmount.
…n/kibana into fix/embeddableErrorReporting2
Dosant
left a comment
There was a problem hiding this comment.
LGTM,
I retested the edge case with removing broken panel
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
flash1293
left a comment
There was a problem hiding this comment.
Tested in Chrome, Lens changes LGTM - if loading a visualization throws an error, it's shown in the panel.
Side note: I first tried to test the error case by turning off the internet connection, then adding a panel to a dashboard. The saved object client request fails, but to show the error message it actually requests an async chunk (which doesn't work as well of course).
This results in the same behavior as before (empty panel is shown) and this error in the console:
Seems like a good fallback to me.
poffdeluxe
left a comment
There was a problem hiding this comment.
This looks good to me 👍
…lastic#82201) Fixed embeddable error handling so that fatal errors are caught and displayed with an errorEmbeddable no matter when they occur.
…lastic#82201) Fixed embeddable error handling so that fatal errors are caught and displayed with an errorEmbeddable no matter when they occur. # Conflicts: # docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.embeddable.md # docs/development/plugins/embeddable/public/kibana-plugin-plugins-embeddable-public.iembeddable.md # src/plugins/dashboard/public/application/actions/library_notification_action.tsx # src/plugins/embeddable/public/public.api.md # x-pack/plugins/lens/public/lens_attribute_service.ts
Summary
Fixes #81399.
This PR uses the
output$Observable as described in #81399 (comment) to notify the embeddable panel when a fatal error has occurred.This PR does not use
replacePanelor implement a factory for the error embeddable because of the potential for it to allow the user to save a dashboard with an errorEmbeddable in place - which means that it would not react if the saved object was subsequently restored.This implementation exists purely in the embeddable panel. The implementation for
isErrorEmbeddablehas also been subtly changed so it returns true for embeddables that have previously had afatalErrorHow to Test
For maintainers