Skip to content

Commit

Permalink
[Security Solution] Fix VisualizationActions React component infinite…
Browse files Browse the repository at this point in the history 
… rerendering (elastic#177770)

**Fixes:** elastic#177734

## Summary

This PR fixes `VisualizationActions` React component infinite rerendering caused by unstable hook dependencies and async state update via `useAsync`.

## Details

Initial problem is described as Rule details page is not responsive and leads to page crash for rule in non-default spaces. Further investigation revealed the problem is caused by infinite React re-rendering of `VisualizationActions` component caused by a number of reasons. It's not an issue in default space because the problematic component isn't rendered at al (it looks like some discrepancy between default and non-default spaces which could be a bug as well).

Besides Rule details page the problem occurs on alerts page as well.

### Promise + unstable dependencies = infinite re-render

React re-renders the app each time a new state is set. It may be beneficial to memoize some intermediate results via `useMemo()`, `useCallback` and etc. These hooks accept a list of dependencies as a second parameter. Whenever a dependency changes the hook returns an updated value. If a dependency changes every render `useMemo()` doesn't give any benefit. In fact it gives a negative impact by consuming memory and burning CPU cycles. In case if a Promise involved and it's also unstable (recreated every render) it will cause infinite re-rendering.

How does it work

- a hook or some function returns a promise
- promise result is awaited in `useEffect` + `.then()` or by using `useAsync`
- after a promise resolves a new state is set (state is needed to rendered the promise result)
- state update leads to a new render where the cycle repeats

### What are the typical mistakes with dependencies

- Accepting an optional parameter and this parameter has a default value as an empty object or array

```ts
function myHook(param = {}) {
  ...

  return useMemo(..., [param]);
}
```

- Accepting a configuration object which is used as dependency

```ts
function myHook(params) {
  ...

  return useMemo(..., [params]);
}

function MyComponent() {
  const result = myHook({
    someOption: 'foo',
  });
}
```

- Returning an object without memoization (while field values could be memoized)

```ts
function myHook(params) {
  const cb = useCallback(() => {...}, []);

  return {
    foo: cb,
  };
}

function MyComponent() {
  const result = myHook();

  const memoizedResult = useMemo(() => result, [result]);
}
```

- Use the whole input properties object as a dependency

Spreading should be used wisely. Properties spreading is definitely an [anti-pattern](https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-props-no-spreading.md).

```ts
function MyComponent(props) {
  const myCallback = useCallback(() => {
    invokeExternalFunction({
      ...props,
      foo: 'bar',
    });
  }, [props]);

  ...
}
```

### Fixes in this PR

This PR updates involved hooks to make them producing stable results (for example object or array isn't regenerated every render).

(cherry picked from commit d45b8d3)
  • Loading branch information
@maximpn
maximpn committed Feb 26, 2024
1 parent e1dbf35 commit 0e9f42e
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 145 deletions.
115 changes: 59 additions & 56 deletions x-pack/plugins/cases/public/common/use_cases_toast.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import type { ErrorToastOptions } from '@kbn/core/public';
import { EuiButtonEmpty, EuiText } from '@elastic/eui';
import React from 'react';
import React, { useMemo } from 'react';
import styled from 'styled-components';
import { toMountPoint } from '@kbn/kibana-react-plugin/public';
import { isValidOwner } from '../../common/utils/owner';
Expand Down Expand Up @@ -124,62 +124,65 @@ export const useCasesToast = () => {

const toasts = useToasts();

return {
showSuccessAttach: ({
theCase,
attachments,
title,
content,
}: {
theCase: CaseUI;
attachments?: CaseAttachmentsWithoutOwner;
title?: string;
content?: string;
}) => {
const appIdToNavigateTo = isValidOwner(theCase.owner)
? OWNER_INFO[theCase.owner].appId
: appId;

const url = getUrlForApp(appIdToNavigateTo, {
deepLinkId: 'cases',
path: generateCaseViewPath({ detailName: theCase.id }),
});

const onViewCaseClick = () => {
navigateToUrl(url);
};

const renderTitle = getToastTitle({ theCase, title, attachments });
const renderContent = getToastContent({ theCase, content, attachments });

return toasts.addSuccess({
color: 'success',
iconType: 'check',
title: toMountPoint(<Title>{renderTitle}</Title>),
text: toMountPoint(
<CaseToastSuccessContent content={renderContent} onViewCaseClick={onViewCaseClick} />
),
});
},
showErrorToast: (error: Error | ServerError, opts?: ErrorToastOptions) => {
if (error.name !== 'AbortError') {
toasts.addError(getError(error), { title: getErrorMessage(error), ...opts });
}
},
showSuccessToast: (title: string) => {
toasts.addSuccess({ title, className: 'eui-textBreakWord' });
},
showDangerToast: (title: string) => {
toasts.addDanger({ title, className: 'eui-textBreakWord' });
},
showInfoToast: (title: string, text?: string) => {
toasts.addInfo({
return useMemo(
() => ({
showSuccessAttach: ({
theCase,
attachments,
title,
text,
className: 'eui-textBreakWord',
});
},
};
content,
}: {
theCase: CaseUI;
attachments?: CaseAttachmentsWithoutOwner;
title?: string;
content?: string;
}) => {
const appIdToNavigateTo = isValidOwner(theCase.owner)
? OWNER_INFO[theCase.owner].appId
: appId;

const url = getUrlForApp(appIdToNavigateTo, {
deepLinkId: 'cases',
path: generateCaseViewPath({ detailName: theCase.id }),
});

const onViewCaseClick = () => {
navigateToUrl(url);
};

const renderTitle = getToastTitle({ theCase, title, attachments });
const renderContent = getToastContent({ theCase, content, attachments });

return toasts.addSuccess({
color: 'success',
iconType: 'check',
title: toMountPoint(<Title>{renderTitle}</Title>),
text: toMountPoint(
<CaseToastSuccessContent content={renderContent} onViewCaseClick={onViewCaseClick} />
),
});
},
showErrorToast: (error: Error | ServerError, opts?: ErrorToastOptions) => {
if (error.name !== 'AbortError') {
toasts.addError(getError(error), { title: getErrorMessage(error), ...opts });
}
},
showSuccessToast: (title: string) => {
toasts.addSuccess({ title, className: 'eui-textBreakWord' });
},
showDangerToast: (title: string) => {
toasts.addDanger({ title, className: 'eui-textBreakWord' });
},
showInfoToast: (title: string, text?: string) => {
toasts.addInfo({
title,
text,
className: 'eui-textBreakWord',
});
},
}),
[appId, getUrlForApp, navigateToUrl, toasts]
);
};

export const CaseToastSuccessContent = ({
Expand Down
56 changes: 34 additions & 22 deletions ...cases/public/components/all_cases/selector_modal/use_cases_add_to_existing_case_modal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,26 @@ export type AddToExistingCaseModalProps = Omit<AllCasesSelectorModalProps, 'onRo
onSuccess?: (theCase: CaseUI) => void;
};

export const useCasesAddToExistingCaseModal = (props: AddToExistingCaseModalProps = {}) => {
const createNewCaseFlyout = useCasesAddToNewCaseFlyout({
onClose: props.onClose,
onSuccess: (theCase?: CaseUI) => {
if (props.onSuccess && theCase) {
return props.onSuccess(theCase);
export const useCasesAddToExistingCaseModal = ({
successToaster,
noAttachmentsToaster,
onSuccess,
onClose,
onCreateCaseClicked,
}: AddToExistingCaseModalProps = {}) => {
const handleSuccess = useCallback(
(theCase?: CaseUI) => {
if (onSuccess && theCase) {
return onSuccess(theCase);
}
},
toastTitle: props.successToaster?.title,
toastContent: props.successToaster?.content,
[onSuccess]
);
const { open: openCreateNewCaseFlyout } = useCasesAddToNewCaseFlyout({
onClose,
onSuccess: handleSuccess,
toastTitle: successToaster?.title,
toastContent: successToaster?.content,
});

const { dispatch, appId } = useCasesContext();
Expand Down Expand Up @@ -69,15 +79,15 @@ export const useCasesAddToExistingCaseModal = (props: AddToExistingCaseModalProp
// the user clicked "create new case"
if (theCase === undefined) {
closeModal();
createNewCaseFlyout.open({ attachments });
openCreateNewCaseFlyout({ attachments });
return;
}

try {
// add attachments to the case
if (attachments === undefined || attachments.length === 0) {
const title = props.noAttachmentsToaster?.title ?? NO_ATTACHMENTS_ADDED;
const content = props.noAttachmentsToaster?.content;
const title = noAttachmentsToaster?.title ?? NO_ATTACHMENTS_ADDED;
const content = noAttachmentsToaster?.content;
casesToasts.showInfoToast(title, content);

return;
Expand All @@ -91,15 +101,13 @@ export const useCasesAddToExistingCaseModal = (props: AddToExistingCaseModalProp
attachments,
});

if (props.onSuccess) {
props.onSuccess(theCase);
}
onSuccess?.(theCase);

casesToasts.showSuccessAttach({
theCase,
attachments,
title: props.successToaster?.title,
content: props.successToaster?.content,
title: successToaster?.title,
content: successToaster?.content,
});
} catch (error) {
// error toast is handled
Expand All @@ -111,8 +119,12 @@ export const useCasesAddToExistingCaseModal = (props: AddToExistingCaseModalProp
casesToasts,
closeModal,
createAttachments,
createNewCaseFlyout,
props,
openCreateNewCaseFlyout,
successToaster?.title,
successToaster?.content,
noAttachmentsToaster?.title,
noAttachmentsToaster?.content,
onSuccess,
startTransaction,
]
);
Expand All @@ -126,22 +138,22 @@ export const useCasesAddToExistingCaseModal = (props: AddToExistingCaseModalProp
dispatch({
type: CasesContextStoreActionsList.OPEN_ADD_TO_CASE_MODAL,
payload: {
...props,
hiddenStatuses: [CaseStatuses.closed],
onCreateCaseClicked,
onRowClick: (theCase?: CaseUI) => {
handleOnRowClick(theCase, getAttachments);
},
onClose: (theCase?: CaseUI, isCreateCase?: boolean) => {
closeModal();

if (props.onClose) {
return props.onClose(theCase, isCreateCase);
if (onClose) {
return onClose(theCase, isCreateCase);
}
},
},
});
},
[closeModal, dispatch, handleOnRowClick, props]
[closeModal, dispatch, handleOnRowClick, onClose, onCreateCaseClicked]
);

return {
Expand Down
40 changes: 29 additions & 11 deletions x-pack/plugins/cases/public/components/create/flyout/use_cases_add_to_new_case_flyout.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@ type AddToNewCaseFlyoutProps = Omit<CreateCaseFlyoutProps, 'attachments'> & {
toastContent?: string;
};

export const useCasesAddToNewCaseFlyout = (props: AddToNewCaseFlyoutProps = {}) => {
export const useCasesAddToNewCaseFlyout = ({
initialValue,
toastTitle,
toastContent,

afterCaseCreated,
onSuccess,
onClose,
}: AddToNewCaseFlyoutProps = {}) => {
const { dispatch } = useCasesContext();
const casesToasts = useCasesToast();

Expand All @@ -37,38 +45,48 @@ export const useCasesAddToNewCaseFlyout = (props: AddToNewCaseFlyoutProps = {})
dispatch({
type: CasesContextStoreActionsList.OPEN_CREATE_CASE_FLYOUT,
payload: {
...props,
initialValue,
attachments,
headerContent,
onClose: () => {
closeFlyout();
if (props.onClose) {
return props.onClose();
if (onClose) {
return onClose();
}
},
onSuccess: async (theCase: CaseUI) => {
if (theCase) {
casesToasts.showSuccessAttach({
theCase,
attachments: attachments ?? [],
title: props.toastTitle,
content: props.toastContent,
title: toastTitle,
content: toastContent,
});
}
if (props.onSuccess) {
return props.onSuccess(theCase);
if (onSuccess) {
return onSuccess(theCase);
}
},
afterCaseCreated: async (...args) => {
closeFlyout();
if (props.afterCaseCreated) {
return props.afterCaseCreated(...args);
if (afterCaseCreated) {
return afterCaseCreated(...args);
}
},
},
});
},
[casesToasts, closeFlyout, dispatch, props]
[
initialValue,
casesToasts,
closeFlyout,
dispatch,
toastTitle,
toastContent,
afterCaseCreated,
onSuccess,
onClose,
]
);
return {
open: openFlyout,
Expand Down
4 changes: 2 additions & 2 deletions ...k/plugins/security_solution/public/common/components/visualization_actions/use_actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ const ACTION_DEFINITION: Record<
export const useActions = ({
attributes,
lensMetadata,
extraActions = [],
extraActions,
inspectActionProps,
timeRange,
withActions = DEFAULT_ACTIONS,
Expand Down Expand Up @@ -186,7 +186,7 @@ export const useActions = ({
canUseEditor() && withActions.includes(VisualizationContextMenuActions.openInLens),
order: 0,
}),
...extraActions,
...(extraActions ?? []),
].map((a, i, totalActions) => {
const order = Math.max(totalActions.length - (1 + i), 0);
return {
Expand Down
Loading

0 comments on commit 0e9f42e

Please sign in to comment.