From 9c4579a830558dfe21623917b6095922a0893b9b Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Wed, 14 Jun 2023 14:22:57 -0400 Subject: [PATCH 1/3] Fix alias redirect, update error handling --- .../embeddable/create/create_dashboard.ts | 1 + .../dashboard_container_factory.tsx | 9 ++- .../external_api/dashboard_renderer.test.tsx | 72 +++++++++++++++++++ .../external_api/dashboard_renderer.tsx | 46 +++++++----- 4 files changed, 107 insertions(+), 21 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts index af564b5ac3c68..4fc296acbf1cc 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts @@ -144,6 +144,7 @@ export const initializeDashboard = async ({ validateLoadedSavedObject && !validateLoadedSavedObject(loadDashboardReturn) ) { + // throw error to stop the rest of Dashboard loading and make the factory return an ErrorEmbeddable. throw new Error('Dashboard failed saved object result validation'); } diff --git a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container_factory.tsx b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container_factory.tsx index 83057fdf8c934..67ab39117e9df 100644 --- a/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container_factory.tsx +++ b/src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container_factory.tsx @@ -97,11 +97,14 @@ export class DashboardContainerFactoryDefinition const dashboardCreationStartTime = performance.now(); const { createDashboard } = await import('./create/create_dashboard'); try { - return Promise.resolve( - createDashboard(creationOptions, dashboardCreationStartTime, savedObjectId) + const dashboard = await createDashboard( + creationOptions, + dashboardCreationStartTime, + savedObjectId ); + return dashboard; } catch (e) { - return new ErrorEmbeddable(e.text, { id: e.id }); + return new ErrorEmbeddable(e, { id: e.id }); } }; } diff --git a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.test.tsx b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.test.tsx index 415e62c1953f1..501fab38f186c 100644 --- a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.test.tsx +++ b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.test.tsx @@ -90,4 +90,76 @@ describe('dashboard renderer', () => { 'saved_object_kibanakiwi' ); }); + + test('renders and destroys an error embeddable when the dashboard factory create method throws an error', async () => { + const mockErrorEmbeddable = { + error: 'oh my goodness an error', + destroy: jest.fn(), + render: jest.fn(), + } as unknown as DashboardContainer; + mockDashboardFactory = { + create: jest.fn().mockReturnValue(mockErrorEmbeddable), + } as unknown as DashboardContainerFactory; + pluginServices.getServices().embeddable.getEmbeddableFactory = jest + .fn() + .mockReturnValue(mockDashboardFactory); + + let wrapper: ReactWrapper; + await act(async () => { + wrapper = await mountWithIntl(); + }); + + expect(mockErrorEmbeddable.render).toHaveBeenCalled(); + wrapper!.unmount(); + expect(mockErrorEmbeddable.destroy).toHaveBeenCalledTimes(1); + }); + + test('creates a new dashboard container when the ID changes, and the first created dashboard resulted in an error', async () => { + // ensure that the first attempt at creating a dashboard results in an error embeddable + const mockErrorEmbeddable = { + error: 'oh my goodness an error', + destroy: jest.fn(), + render: jest.fn(), + } as unknown as DashboardContainer; + const mockErrorFactory = { + create: jest.fn().mockReturnValue(mockErrorEmbeddable), + } as unknown as DashboardContainerFactory; + pluginServices.getServices().embeddable.getEmbeddableFactory = jest + .fn() + .mockReturnValue(mockErrorFactory); + + // render the dashboard - it should run into an error and render the error embeddable. + let wrapper: ReactWrapper; + await act(async () => { + wrapper = await mountWithIntl(); + }); + expect(mockErrorEmbeddable.render).toHaveBeenCalled(); + expect(mockErrorFactory.create).toHaveBeenCalledTimes(1); + + // ensure that the next attempt at creating a dashboard is successfull. + const mockSuccessEmbeddable = { + destroy: jest.fn(), + render: jest.fn(), + navigateToDashboard: jest.fn(), + } as unknown as DashboardContainer; + const mockSuccessFactory = { + create: jest.fn().mockReturnValue(mockSuccessEmbeddable), + } as unknown as DashboardContainerFactory; + pluginServices.getServices().embeddable.getEmbeddableFactory = jest + .fn() + .mockReturnValue(mockSuccessFactory); + + // update the saved object id to trigger another dashboard load. + await act(async () => { + await wrapper.setProps({ savedObjectId: 'saved_object_kibanakiwi' }); + }); + + expect(mockErrorEmbeddable.destroy).toHaveBeenCalled(); + + // because a new dashboard container has been created, we should not call navigate. + expect(mockSuccessEmbeddable.navigateToDashboard).not.toHaveBeenCalled(); + + // instead we should call create on the factory again. + expect(mockSuccessFactory.create).toHaveBeenCalledTimes(1); + }); }); diff --git a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx index aff31252af044..9580fcabe6dc8 100644 --- a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx +++ b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx @@ -18,8 +18,10 @@ import React, { } from 'react'; import { v4 as uuidv4 } from 'uuid'; import classNames from 'classnames'; +import useUnmount from 'react-use/lib/useUnmount'; import { EuiLoadingElastic, EuiLoadingSpinner } from '@elastic/eui'; +import { ErrorEmbeddable, isErrorEmbeddable } from '@kbn/embeddable-plugin/public'; import { DashboardAPI, @@ -47,6 +49,7 @@ export const DashboardRenderer = forwardRef(); + const [fatalError, setFatalError] = useState(); useImperativeHandle( ref, @@ -65,23 +68,21 @@ export const DashboardRenderer = forwardRef { - if (!dashboardContainer) return; - - // When a dashboard already exists, don't rebuild it, just set a new id. - dashboardContainer.navigateToDashboard(savedObjectId); - - // Disabling exhaustive deps because this useEffect should only be triggered when the savedObjectId changes. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [savedObjectId]); - const id = useMemo(() => uuidv4(), []); useEffect(() => { - let canceled = false; - let destroyContainer: () => void; + if (dashboardContainer) { + // When a dashboard already exists, don't rebuild it, just set a new id. + dashboardContainer.navigateToDashboard(savedObjectId); + + return; + } + let canceled = false; (async () => { + fatalError?.destroy(); + setFatalError(undefined); + const creationOptions = await getCreationOptions?.(); // Lazy loading all services is required in this component because it is exported and contributes to the bundle size. @@ -91,33 +92,41 @@ export const DashboardRenderer = forwardRef container.destroy(); })(); return () => { canceled = true; - destroyContainer?.(); }; // Disabling exhaustive deps because embeddable should only be created on first render. // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [savedObjectId]); + + useUnmount(() => { + fatalError?.destroy(); + dashboardContainer?.destroy(); + }); const viewportClasses = classNames( 'dashboardViewport', @@ -133,7 +142,8 @@ export const DashboardRenderer = forwardRef - {loading ? loadingSpinner :
} + {loading && !fatalError ? loadingSpinner :
} + {fatalError ? fatalError.render() : null}
); } From a1d33c9a82cd9e9d0088580238be8f930145b050 Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Thu, 15 Jun 2023 09:34:32 -0400 Subject: [PATCH 2/3] set loading state to false on error --- .../external_api/dashboard_renderer.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx index 9580fcabe6dc8..988325da2a96a 100644 --- a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx +++ b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx @@ -77,6 +77,7 @@ export const DashboardRenderer = forwardRef { @@ -104,12 +105,13 @@ export const DashboardRenderer = forwardRef - {loading && !fatalError ? loadingSpinner :
} - {fatalError ? fatalError.render() : null} + {loading && loadingSpinner} + {fatalError && fatalError.render()} + {!fatalError && !loading &&
}
); } From 7228074be27a45c140ce130ea46e9d92bfc661af Mon Sep 17 00:00:00 2001 From: Devon Thomson Date: Thu, 15 Jun 2023 10:36:28 -0400 Subject: [PATCH 3/3] Review feedback, function for rendering --- .../external_api/dashboard_renderer.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx index 988325da2a96a..13a1c82416aed 100644 --- a/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx +++ b/src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx @@ -142,12 +142,12 @@ export const DashboardRenderer = forwardRef ); - return ( -
- {loading && loadingSpinner} - {fatalError && fatalError.render()} - {!fatalError && !loading &&
} -
- ); + const renderDashboardContents = () => { + if (fatalError) return fatalError.render(); + if (loading) return loadingSpinner; + return
; + }; + + return
{renderDashboardContents()}
; } );