From d45b8d3ab3cc8c82f18526678d9a66d22fb9de38 Mon Sep 17 00:00:00 2001 From: Maxim Palenov Date: Tue, 27 Feb 2024 00:06:25 +0100 Subject: [PATCH] [Security Solution] Fix VisualizationActions React component infinite rerendering (#177770) **Fixes:** https://github.com/elastic/kibana/issues/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). --- .../cases/public/common/use_cases_toast.tsx | 115 +++++++++--------- .../use_cases_add_to_existing_case_modal.tsx | 56 +++++---- .../use_cases_add_to_new_case_flyout.tsx | 40 ++++-- .../visualization_actions/use_actions.ts | 4 +- .../use_add_to_existing_case.test.tsx | 37 ++---- .../use_add_to_existing_case.tsx | 6 +- .../use_add_to_new_case.test.tsx | 35 ++---- .../use_add_to_new_case.tsx | 6 +- 8 files changed, 154 insertions(+), 145 deletions(-) diff --git a/x-pack/plugins/cases/public/common/use_cases_toast.tsx b/x-pack/plugins/cases/public/common/use_cases_toast.tsx index 93905eaf64213..baa15f1b47248 100644 --- a/x-pack/plugins/cases/public/common/use_cases_toast.tsx +++ b/x-pack/plugins/cases/public/common/use_cases_toast.tsx @@ -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'; @@ -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({renderTitle}), - text: toMountPoint( - - ), - }); - }, - 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({renderTitle}), + text: toMountPoint( + + ), + }); + }, + 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 = ({ diff --git a/x-pack/plugins/cases/public/components/all_cases/selector_modal/use_cases_add_to_existing_case_modal.tsx b/x-pack/plugins/cases/public/components/all_cases/selector_modal/use_cases_add_to_existing_case_modal.tsx index f311fdad66a47..ea60c43937346 100644 --- a/x-pack/plugins/cases/public/components/all_cases/selector_modal/use_cases_add_to_existing_case_modal.tsx +++ b/x-pack/plugins/cases/public/components/all_cases/selector_modal/use_cases_add_to_existing_case_modal.tsx @@ -30,16 +30,26 @@ export type AddToExistingCaseModalProps = Omit 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(); @@ -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; @@ -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 @@ -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, ] ); @@ -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 { diff --git a/x-pack/plugins/cases/public/components/create/flyout/use_cases_add_to_new_case_flyout.tsx b/x-pack/plugins/cases/public/components/create/flyout/use_cases_add_to_new_case_flyout.tsx index a8e234f9bb96a..e6d5bd2fc2a0b 100644 --- a/x-pack/plugins/cases/public/components/create/flyout/use_cases_add_to_new_case_flyout.tsx +++ b/x-pack/plugins/cases/public/components/create/flyout/use_cases_add_to_new_case_flyout.tsx @@ -19,7 +19,15 @@ type AddToNewCaseFlyoutProps = Omit & { toastContent?: string; }; -export const useCasesAddToNewCaseFlyout = (props: AddToNewCaseFlyoutProps = {}) => { +export const useCasesAddToNewCaseFlyout = ({ + initialValue, + toastTitle, + toastContent, + + afterCaseCreated, + onSuccess, + onClose, +}: AddToNewCaseFlyoutProps = {}) => { const { dispatch } = useCasesContext(); const casesToasts = useCasesToast(); @@ -37,13 +45,13 @@ 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) => { @@ -51,24 +59,34 @@ export const useCasesAddToNewCaseFlyout = (props: AddToNewCaseFlyoutProps = {}) 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, diff --git a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_actions.ts b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_actions.ts index 4d487dcd9186d..1a61042389911 100644 --- a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_actions.ts +++ b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_actions.ts @@ -86,7 +86,7 @@ const ACTION_DEFINITION: Record< export const useActions = ({ attributes, lensMetadata, - extraActions = [], + extraActions, inspectActionProps, timeRange, withActions = DEFAULT_ACTIONS, @@ -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 { diff --git a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.test.tsx b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.test.tsx index 2ed014ee69a37..5181d33370afe 100644 --- a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.test.tsx +++ b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.test.tsx @@ -15,31 +15,15 @@ import { } from '../../../cases_test_utils'; import { AttachmentType } from '@kbn/cases-plugin/common'; -const mockedUseKibana = mockUseKibana(); -const mockGetUseCasesAddToExistingCaseModal = jest.fn(); -const mockCanUseCases = jest.fn(); - -jest.mock('../../lib/kibana', () => { - const original = jest.requireActual('../../lib/kibana'); - - return { - ...original, - useKibana: () => ({ - ...mockedUseKibana, - services: { - ...mockedUseKibana.services, - cases: { - hooks: { - useCasesAddToExistingCaseModal: mockGetUseCasesAddToExistingCaseModal, - }, - helpers: { canUseCases: mockCanUseCases }, - }, - }, - }), - }; -}); +jest.mock('../../lib/kibana'); describe('useAddToExistingCase', () => { + const mockedUseKibana = mockUseKibana(); + const mockCanUseCases = jest.fn(); + const mockUseCasesAddToExistingCaseModal = jest.fn().mockReturnValue({ + open: jest.fn(), + close: jest.fn(), + }); const mockOnAddToCaseClicked = jest.fn(); const timeRange = { from: '2022-03-06T16:00:00.000Z', @@ -48,6 +32,9 @@ describe('useAddToExistingCase', () => { beforeEach(() => { mockCanUseCases.mockReturnValue(allCasesPermissions()); + mockedUseKibana.services.cases.hooks.useCasesAddToExistingCaseModal = + mockUseCasesAddToExistingCaseModal; + mockedUseKibana.services.cases.helpers.canUseCases = mockCanUseCases; }); it('useCasesAddToExistingCaseModal with attachments', () => { @@ -59,7 +46,7 @@ describe('useAddToExistingCase', () => { lensMetadata: undefined, }) ); - expect(mockGetUseCasesAddToExistingCaseModal).toHaveBeenCalledWith({ + expect(mockUseCasesAddToExistingCaseModal).toHaveBeenCalledWith({ onClose: mockOnAddToCaseClicked, successToaster: { title: 'Successfully added visualization to the case', @@ -127,7 +114,7 @@ describe('useAddToExistingCase', () => { description: 'test_description', }; - mockGetUseCasesAddToExistingCaseModal.mockReturnValue({ open: mockOpenCaseModal }); + mockUseCasesAddToExistingCaseModal.mockReturnValue({ open: mockOpenCaseModal }); const { result } = renderHook(() => useAddToExistingCase({ diff --git a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.tsx b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.tsx index 1675802b9953e..aa11ced2603a9 100644 --- a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.tsx +++ b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_existing_case.tsx @@ -40,7 +40,7 @@ export const useAddToExistingCase = ({ ] as CaseAttachmentsWithoutOwner; }, [lensAttributes, lensMetadata, timeRange]); - const selectCaseModal = cases.hooks.useCasesAddToExistingCaseModal({ + const { open: openSelectCaseModal } = cases.hooks.useCasesAddToExistingCaseModal({ onClose: onAddToCaseClicked, successToaster: { title: ADD_TO_CASE_SUCCESS, @@ -51,8 +51,8 @@ export const useAddToExistingCase = ({ if (onAddToCaseClicked) { onAddToCaseClicked(); } - selectCaseModal.open({ getAttachments: () => attachments }); - }, [attachments, onAddToCaseClicked, selectCaseModal]); + openSelectCaseModal({ getAttachments: () => attachments }); + }, [attachments, onAddToCaseClicked, openSelectCaseModal]); return { onAddToExistingCaseClicked, diff --git a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.test.tsx b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.test.tsx index 91347dc9fe073..fceff5e6cdaae 100644 --- a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.test.tsx +++ b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.test.tsx @@ -17,37 +17,26 @@ import { AttachmentType } from '@kbn/cases-plugin/common'; jest.mock('../../lib/kibana/kibana_react'); -const mockedUseKibana = mockUseKibana(); -const mockGetUseCasesAddToNewCaseFlyout = jest.fn(); -const mockCanUseCases = jest.fn(); - -jest.mock('../../lib/kibana', () => { - const original = jest.requireActual('../../lib/kibana'); - - return { - ...original, - useKibana: () => ({ - ...mockedUseKibana, - services: { - ...mockedUseKibana.services, - cases: { - hooks: { - useCasesAddToNewCaseFlyout: mockGetUseCasesAddToNewCaseFlyout, - }, - helpers: { canUseCases: mockCanUseCases }, - }, - }, - }), - }; -}); +jest.mock('../../lib/kibana'); describe('useAddToNewCase', () => { + const mockedUseKibana = mockUseKibana(); + const mockCanUseCases = jest.fn(); + const mockGetUseCasesAddToNewCaseFlyout = jest.fn().mockReturnValue({ + open: jest.fn(), + close: jest.fn(), + }); + const timeRange = { from: '2022-03-06T16:00:00.000Z', to: '2022-03-07T15:59:59.999Z', }; + beforeEach(() => { mockCanUseCases.mockReturnValue(allCasesPermissions()); + mockedUseKibana.services.cases.hooks.useCasesAddToNewCaseFlyout = + mockGetUseCasesAddToNewCaseFlyout; + mockedUseKibana.services.cases.helpers.canUseCases = mockCanUseCases; }); it('useCasesAddToNewCaseFlyout with attachments', () => { diff --git a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.tsx b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.tsx index aca5ba88dc057..c2ac628000fa7 100644 --- a/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.tsx +++ b/x-pack/plugins/security_solution/public/common/components/visualization_actions/use_add_to_new_case.tsx @@ -43,7 +43,7 @@ export const useAddToNewCase = ({ ] as CaseAttachmentsWithoutOwner; }, [lensAttributes, lensMetadata, timeRange]); - const createCaseFlyout = cases.hooks.useCasesAddToNewCaseFlyout({ + const { open: openCreateCaseFlyout } = cases.hooks.useCasesAddToNewCaseFlyout({ toastContent: ADD_TO_CASE_SUCCESS, }); @@ -52,8 +52,8 @@ export const useAddToNewCase = ({ onClick(); } - createCaseFlyout.open({ attachments }); - }, [attachments, createCaseFlyout, onClick]); + openCreateCaseFlyout({ attachments }); + }, [attachments, openCreateCaseFlyout, onClick]); return { onAddToNewCaseClicked,