Skip to content
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.

Sign up for GitHub

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

[Dashboard] Fix alias redirect & update error handling #159742

Merged
merged 3 commits into from
Jun 15, 2023
Merged

[Dashboard] Fix alias redirect & update error handling #159742

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c4579a
Fix alias redirect, update error handling
ThomThomson Jun 14, 2023
a1d33c9
set loading state to false on error
ThomThomson Jun 15, 2023
7228074
Review feedback, function for rendering
ThomThomson Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

Expand Down
9 changes: 6 additions & 3 deletions src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container_factory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}
};
}
72 changes: 72 additions & 0 deletions src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(<DashboardRenderer savedObjectId="saved_object_kibanana" />);
});

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(<DashboardRenderer savedObjectId="saved_object_kibanana" />);
});
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);
});
});
49 changes: 31 additions & 18 deletions src/plugins/dashboard/public/dashboard_container/external_api/dashboard_renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -47,6 +49,7 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende
const [loading, setLoading] = useState(true);
const [screenshotMode, setScreenshotMode] = useState(false);
const [dashboardContainer, setDashboardContainer] = useState<DashboardContainer>();
const [fatalError, setFatalError] = useState<ErrorEmbeddable | undefined>();

useImperativeHandle(
ref,
Expand All @@ -65,23 +68,22 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende
})();
}, []);

useEffect(() => {
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;
}
setLoading(true);

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.
Expand All @@ -91,33 +93,42 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende
const dashboardFactory = embeddable.getEmbeddableFactory(
DASHBOARD_CONTAINER_TYPE
) as DashboardContainerFactory & { create: DashboardContainerFactoryDefinition['create'] };
const container = (await dashboardFactory?.create(
const container = await dashboardFactory?.create(
{ id } as unknown as DashboardContainerInput, // Input from creationOptions is used instead.
undefined,
creationOptions,
savedObjectId
)) as DashboardContainer;
);

if (canceled) {
container.destroy();
return;
}

setLoading(false);

if (isErrorEmbeddable(container)) {
Copy link
Contributor

@nreese nreese Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't setLoading(false); get called if container is an error embeddable? The container finished loading, even if it loaded in an error state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I didn't catch that because the error was rendered if it was present no matter if the loading state was true or not. I've straightened that out!

setFatalError(container);
return;
}

if (dashboardRoot.current) {
container.render(dashboardRoot.current);
}

setDashboardContainer(container);
destroyContainer = () => 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',
Expand All @@ -133,7 +144,9 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende

return (
<div className={viewportClasses}>
{loading ? loadingSpinner : <div ref={dashboardRoot} />}
{loading && loadingSpinner}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, there should never be a case where loadingSpinner and fatalError.render() are displayed but there could be if both loading and fatalError are set. Would it be clearer to move this logic to a function and use early returns so only a single item is rendered. For example

function renderContent() {
  if (loading) {
    return loadingSpinner;
  }

  if (fatalError) {
    return fatalError.render();
  }

  return (<div ref={dashboardRoot} />);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. Even if it wouldn't necessarily happen, it's still much cleaner and more understandable to do it this way.

{fatalError && fatalError.render()}
{!fatalError && !loading && <div ref={dashboardRoot} />}
</div>
);
}
Expand Down